[v1,3/3] riscv: dts: starfive: Add mmc node

  This adds the mmc node for the StarFive JH7110 SoC.
Set sdioo node to emmc and set sdio1 node to sd.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
---
 .../jh7110-starfive-visionfive-v2.dts         | 25 ++++++++++++
 arch/riscv/boot/dts/starfive/jh7110.dtsi      | 38 +++++++++++++++++++
 2 files changed, 63 insertions(+)
  

On 07/12/2022 14:17, William Qiu wrote:
> This adds the mmc node for the StarFive JH7110 SoC.
> Set sdioo node to emmc and set sdio1 node to sd.
> 
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> ---
>  .../jh7110-starfive-visionfive-v2.dts         | 25 ++++++++++++
>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 38 +++++++++++++++++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> index c8946cf3a268..6ef8e303c2e6 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> @@ -47,6 +47,31 @@ &clk_rtc {
>  	clock-frequency = <32768>;
>  };
>  
> +&sdio0 {
> +	max-frequency = <100000000>;
> +	card-detect-delay = <300>;
> +	bus-width = <8>;
> +	cap-mmc-highspeed;
> +	mmc-ddr-1_8v;
> +	mmc-hs200-1_8v;
> +	non-removable;
> +	cap-mmc-hw-reset;
> +	post-power-on-delay-ms = <200>;
> +	status = "okay";
> +};
> +
> +&sdio1 {
> +	max-frequency = <100000000>;
> +	card-detect-delay = <300>;
> +	bus-width = <4>;
> +	no-sdio;
> +	no-mmc;
> +	broken-cd;
> +	cap-sd-highspeed;
> +	post-power-on-delay-ms = <200>;
> +	status = "okay";
> +};
> +
>  &gmac0_rmii_refin {
>  	clock-frequency = <50000000>;
>  };
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index c22e8f1d2640..e90b085d7e41 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -331,6 +331,11 @@ aoncrg: clock-controller@17000000 {
>  			#reset-cells = <1>;
>  		};
>  
> +		sys_syscon: sys_syscon@13030000 {

No underscores in node names, generic node names (syscon or
system-controller)

> +			compatible = "syscon";

This is not allowed alone.

> +			reg = <0x0 0x13030000 0x0 0x1000>;
> +		};
> +
>  		gpio: gpio@13040000 {
>  			compatible = "starfive,jh7110-sys-pinctrl";
>  			reg = <0x0 0x13040000 0x0 0x10000>;
> @@ -433,5 +438,38 @@ uart5: serial@12020000 {
>  			reg-shift = <2>;
>  			status = "disabled";
>  		};
> +
> +		/* unremovable emmc as mmcblk0 */
> +		sdio0: mmc@16010000 {
> +			compatible = "starfive,jh7110-sdio";
> +			reg = <0x0 0x16010000 0x0 0x10000>;
> +			clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>,
> +				 <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>;
> +			clock-names = "biu","ciu";
> +			resets = <&syscrg JH7110_SYSRST_SDIO0_AHB>;
> +			reset-names = "reset";
> +			interrupts = <74>;
> +			fifo-depth = <32>;
> +			fifo-watermark-aligned;
> +			data-addr = <0>;
> +			starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>;

This does not match your bindings at all. "&sys_syscon" is a phandle,
not a number of tuning retries, as you expect in your bindings.


Best regards,
Krzysztof
  

On Wed, Dec 07, 2022 at 04:14:53PM +0100, Krzysztof Kozlowski wrote:
> On 07/12/2022 14:17, William Qiu wrote:
> > This adds the mmc node for the StarFive JH7110 SoC.
> > Set sdioo node to emmc and set sdio1 node to sd.
> > 
> > Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> > ---
> >  .../jh7110-starfive-visionfive-v2.dts         | 25 ++++++++++++
> >  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 38 +++++++++++++++++++
> >  2 files changed, 63 insertions(+)
> > 
> > diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > index c8946cf3a268..6ef8e303c2e6 100644
> > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
> > @@ -47,6 +47,31 @@ &clk_rtc {
> >  	clock-frequency = <32768>;
> >  };
> >  
> > +&sdio0 {
> > +	max-frequency = <100000000>;
> > +	card-detect-delay = <300>;
> > +	bus-width = <8>;
> > +	cap-mmc-highspeed;
> > +	mmc-ddr-1_8v;
> > +	mmc-hs200-1_8v;
> > +	non-removable;
> > +	cap-mmc-hw-reset;
> > +	post-power-on-delay-ms = <200>;
> > +	status = "okay";
> > +};
> > +
> > +&sdio1 {
> > +	max-frequency = <100000000>;
> > +	card-detect-delay = <300>;
> > +	bus-width = <4>;
> > +	no-sdio;
> > +	no-mmc;
> > +	broken-cd;
> > +	cap-sd-highspeed;
> > +	post-power-on-delay-ms = <200>;
> > +	status = "okay";
> > +};
> > +
> >  &gmac0_rmii_refin {
> >  	clock-frequency = <50000000>;
> >  };
> > diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > index c22e8f1d2640..e90b085d7e41 100644
> > --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> > @@ -331,6 +331,11 @@ aoncrg: clock-controller@17000000 {
> >  			#reset-cells = <1>;
> >  		};
> >  
> > +		sys_syscon: sys_syscon@13030000 {
> 
> No underscores in node names, generic node names (syscon or
> system-controller)
> 
> > +			compatible = "syscon";
> 
> This is not allowed alone.
> 
> > +			reg = <0x0 0x13030000 0x0 0x1000>;
> > +		};
> > +
> >  		gpio: gpio@13040000 {
> >  			compatible = "starfive,jh7110-sys-pinctrl";
> >  			reg = <0x0 0x13040000 0x0 0x10000>;
> > @@ -433,5 +438,38 @@ uart5: serial@12020000 {
> >  			reg-shift = <2>;
> >  			status = "disabled";
> >  		};
> > +
> > +		/* unremovable emmc as mmcblk0 */
> > +		sdio0: mmc@16010000 {
> > +			compatible = "starfive,jh7110-sdio";
> > +			reg = <0x0 0x16010000 0x0 0x10000>;
> > +			clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>,
> > +				 <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>;
> > +			clock-names = "biu","ciu";
> > +			resets = <&syscrg JH7110_SYSRST_SDIO0_AHB>;
> > +			reset-names = "reset";
> > +			interrupts = <74>;
> > +			fifo-depth = <32>;
> > +			fifo-watermark-aligned;
> > +			data-addr = <0>;
> > +			starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>;
> 
> This does not match your bindings at all. "&sys_syscon" is a phandle,
> not a number of tuning retries, as you expect in your bindings.

Additionally, a Link: to the documentation for where-ever these "random"
numbers that are being used would be nice.

+static int dw_mci_starfive_parse_dt(struct dw_mci *host)
+{
+	struct of_phandle_args args;
+	struct starfive_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	ret = of_parse_phandle_with_fixed_args(host->dev->of_node,
+						"starfive,sys-syscon", 3, 0, &args);
+	if (ret) {
+		dev_err(host->dev, "Failed to parse starfive,sys-syscon\n");
+		return -EINVAL;
+	}
+
+	priv->reg_syscon = syscon_node_to_regmap(args.np);
+	of_node_put(args.np);
+	if (IS_ERR(priv->reg_syscon))
+		return PTR_ERR(priv->reg_syscon);
+
+	priv->syscon_offset = args.args[0];
+	priv->syscon_shift  = args.args[1];
+	priv->syscon_mask   = args.args[2];

Given the driver, the property description just seems incorrect and this
is actually the bit of the syscon that is relevant to the tuning process
(perhaps where the find the tuning values?). Without public docs or a
better description it is hard for (me at least) to know :)

+
+	host->priv = priv;
+
+	return 0;
+}
  

On 2022/12/7 23:14, Krzysztof Kozlowski wrote:
> On 07/12/2022 14:17, William Qiu wrote:
>> This adds the mmc node for the StarFive JH7110 SoC.
>> Set sdioo node to emmc and set sdio1 node to sd.
>> 
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> ---
>>  .../jh7110-starfive-visionfive-v2.dts         | 25 ++++++++++++
>>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 38 +++++++++++++++++++
>>  2 files changed, 63 insertions(+)
>> 
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
>> index c8946cf3a268..6ef8e303c2e6 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
>> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
>> @@ -47,6 +47,31 @@ &clk_rtc {
>>  	clock-frequency = <32768>;
>>  };
>>  
>> +&sdio0 {
>> +	max-frequency = <100000000>;
>> +	card-detect-delay = <300>;
>> +	bus-width = <8>;
>> +	cap-mmc-highspeed;
>> +	mmc-ddr-1_8v;
>> +	mmc-hs200-1_8v;
>> +	non-removable;
>> +	cap-mmc-hw-reset;
>> +	post-power-on-delay-ms = <200>;
>> +	status = "okay";
>> +};
>> +
>> +&sdio1 {
>> +	max-frequency = <100000000>;
>> +	card-detect-delay = <300>;
>> +	bus-width = <4>;
>> +	no-sdio;
>> +	no-mmc;
>> +	broken-cd;
>> +	cap-sd-highspeed;
>> +	post-power-on-delay-ms = <200>;
>> +	status = "okay";
>> +};
>> +
>>  &gmac0_rmii_refin {
>>  	clock-frequency = <50000000>;
>>  };
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> index c22e8f1d2640..e90b085d7e41 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> @@ -331,6 +331,11 @@ aoncrg: clock-controller@17000000 {
>>  			#reset-cells = <1>;
>>  		};
>>  
>> +		sys_syscon: sys_syscon@13030000 {
> 
> No underscores in node names, generic node names (syscon or
> system-controller)

Will fix.

> 
>> +			compatible = "syscon";
> 
> This is not allowed alone.

Will fix.

> 
>> +			reg = <0x0 0x13030000 0x0 0x1000>;
>> +		};
>> +
>>  		gpio: gpio@13040000 {
>>  			compatible = "starfive,jh7110-sys-pinctrl";
>>  			reg = <0x0 0x13040000 0x0 0x10000>;
>> @@ -433,5 +438,38 @@ uart5: serial@12020000 {
>>  			reg-shift = <2>;
>>  			status = "disabled";
>>  		};
>> +
>> +		/* unremovable emmc as mmcblk0 */
>> +		sdio0: mmc@16010000 {
>> +			compatible = "starfive,jh7110-sdio";
>> +			reg = <0x0 0x16010000 0x0 0x10000>;
>> +			clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>,
>> +				 <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>;
>> +			clock-names = "biu","ciu";
>> +			resets = <&syscrg JH7110_SYSRST_SDIO0_AHB>;
>> +			reset-names = "reset";
>> +			interrupts = <74>;
>> +			fifo-depth = <32>;
>> +			fifo-watermark-aligned;
>> +			data-addr = <0>;
>> +			starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>;
> 
> This does not match your bindings at all. "&sys_syscon" is a phandle,
> not a number of tuning retries, as you expect in your bindings.
> 

Hi Krzysztof,

The sys_syscon is one of system controller registers, and it is used to be the data strobe
delay chain select. I will update its type and description in next version.

Thanks,
William Qiu

> 
> Best regards,
> Krzysztof
>
  

On 2022/12/8 0:31, Conor Dooley wrote:
> On Wed, Dec 07, 2022 at 04:14:53PM +0100, Krzysztof Kozlowski wrote:
>> On 07/12/2022 14:17, William Qiu wrote:
>> > This adds the mmc node for the StarFive JH7110 SoC.
>> > Set sdioo node to emmc and set sdio1 node to sd.
>> > 
>> > Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> > ---
>> >  .../jh7110-starfive-visionfive-v2.dts         | 25 ++++++++++++
>> >  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 38 +++++++++++++++++++
>> >  2 files changed, 63 insertions(+)
>> > 
>> > diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
>> > index c8946cf3a268..6ef8e303c2e6 100644
>> > --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
>> > +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-v2.dts
>> > @@ -47,6 +47,31 @@ &clk_rtc {
>> >  	clock-frequency = <32768>;
>> >  };
>> >  
>> > +&sdio0 {
>> > +	max-frequency = <100000000>;
>> > +	card-detect-delay = <300>;
>> > +	bus-width = <8>;
>> > +	cap-mmc-highspeed;
>> > +	mmc-ddr-1_8v;
>> > +	mmc-hs200-1_8v;
>> > +	non-removable;
>> > +	cap-mmc-hw-reset;
>> > +	post-power-on-delay-ms = <200>;
>> > +	status = "okay";
>> > +};
>> > +
>> > +&sdio1 {
>> > +	max-frequency = <100000000>;
>> > +	card-detect-delay = <300>;
>> > +	bus-width = <4>;
>> > +	no-sdio;
>> > +	no-mmc;
>> > +	broken-cd;
>> > +	cap-sd-highspeed;
>> > +	post-power-on-delay-ms = <200>;
>> > +	status = "okay";
>> > +};
>> > +
>> >  &gmac0_rmii_refin {
>> >  	clock-frequency = <50000000>;
>> >  };
>> > diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> > index c22e8f1d2640..e90b085d7e41 100644
>> > --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> > +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> > @@ -331,6 +331,11 @@ aoncrg: clock-controller@17000000 {
>> >  			#reset-cells = <1>;
>> >  		};
>> >  
>> > +		sys_syscon: sys_syscon@13030000 {
>> 
>> No underscores in node names, generic node names (syscon or
>> system-controller)
>> 
>> > +			compatible = "syscon";
>> 
>> This is not allowed alone.
>> 
>> > +			reg = <0x0 0x13030000 0x0 0x1000>;
>> > +		};
>> > +
>> >  		gpio: gpio@13040000 {
>> >  			compatible = "starfive,jh7110-sys-pinctrl";
>> >  			reg = <0x0 0x13040000 0x0 0x10000>;
>> > @@ -433,5 +438,38 @@ uart5: serial@12020000 {
>> >  			reg-shift = <2>;
>> >  			status = "disabled";
>> >  		};
>> > +
>> > +		/* unremovable emmc as mmcblk0 */
>> > +		sdio0: mmc@16010000 {
>> > +			compatible = "starfive,jh7110-sdio";
>> > +			reg = <0x0 0x16010000 0x0 0x10000>;
>> > +			clocks = <&syscrg JH7110_SYSCLK_SDIO0_AHB>,
>> > +				 <&syscrg JH7110_SYSCLK_SDIO0_SDCARD>;
>> > +			clock-names = "biu","ciu";
>> > +			resets = <&syscrg JH7110_SYSRST_SDIO0_AHB>;
>> > +			reset-names = "reset";
>> > +			interrupts = <74>;
>> > +			fifo-depth = <32>;
>> > +			fifo-watermark-aligned;
>> > +			data-addr = <0>;
>> > +			starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>;
>> 
>> This does not match your bindings at all. "&sys_syscon" is a phandle,
>> not a number of tuning retries, as you expect in your bindings.
> 
> Additionally, a Link: to the documentation for where-ever these "random"
> numbers that are being used would be nice.
> 

I will update in next version.

> +static int dw_mci_starfive_parse_dt(struct dw_mci *host)
> +{
> +	struct of_phandle_args args;
> +	struct starfive_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(host->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	ret = of_parse_phandle_with_fixed_args(host->dev->of_node,
> +						"starfive,sys-syscon", 3, 0, &args);
> +	if (ret) {
> +		dev_err(host->dev, "Failed to parse starfive,sys-syscon\n");
> +		return -EINVAL;
> +	}
> +
> +	priv->reg_syscon = syscon_node_to_regmap(args.np);
> +	of_node_put(args.np);
> +	if (IS_ERR(priv->reg_syscon))
> +		return PTR_ERR(priv->reg_syscon);
> +
> +	priv->syscon_offset = args.args[0];
> +	priv->syscon_shift  = args.args[1];
> +	priv->syscon_mask   = args.args[2];
> 
> Given the driver, the property description just seems incorrect and this
> is actually the bit of the syscon that is relevant to the tuning process
> (perhaps where the find the tuning values?). Without public docs or a
> better description it is hard for (me at least) to know :)
> 

Hi Conor,

Thank you for taking time to review and provide helpful comments for this patch.
I'll make the description better and more detailed in next version.

Best regards,
William Qiu
> +
> +	host->priv = priv;
> +
> +	return 0;
> +}
  

Hi William,

thanks for your patch!

On Wed, Dec 7, 2022 at 2:17 PM William Qiu <william.qiu@starfivetech.com> wrote:

> +                       starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>;
(...)
> +                       starfive,sys-syscon = <&sys_syscon 0x9c 0x1 0x3e>;

These are registster offsets and shifts. Don't define such stuff in your device
tree, use #defines in the code and just provide the phandle.

Yours,
Linus Walleij
  

On 2022/12/9 5:15, Linus Walleij wrote:
> Hi William,
> 
> thanks for your patch!
> 
> On Wed, Dec 7, 2022 at 2:17 PM William Qiu <william.qiu@starfivetech.com> wrote:
> 
>> +                       starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>;
> (...)
>> +                       starfive,sys-syscon = <&sys_syscon 0x9c 0x1 0x3e>;
> 
> These are registster offsets and shifts. Don't define such stuff in your device
> tree, use #defines in the code and just provide the phandle.
> 

Hi Linus,

I will use #define in code instead in device tree.
Thank you for taking time to review and provide helpful comments for this patch.

Best Regards,
William Qiu
> Yours,
> Linus Walleij
  

On 2022/12/9 5:15, Linus Walleij wrote:
> Hi William,
> 
> thanks for your patch!
> 
> On Wed, Dec 7, 2022 at 2:17 PM William Qiu <william.qiu@starfivetech.com> wrote:
> 
>> +                       starfive,sys-syscon = <&sys_syscon 0x14 0x1a 0x7c000000>;
> (...)
>> +                       starfive,sys-syscon = <&sys_syscon 0x9c 0x1 0x3e>;
> 
> These are registster offsets and shifts. Don't define such stuff in your device
> tree, use #defines in the code and just provide the phandle.
> 
Hi Linus,

I'm sorry to bother you, but as for the definition of syscon, after discussing with 
my colleagues, we think it is easier to distinguish SDIO0 and SDIO1 by defining it in
the device tree, and the code compatibility is better. I expected to discuss this issue
with you in V2,, but I am not sure whether you have seen V2, so I can only ask for
your advice in V1.

Best Regards
William Qiu
> Yours,
> Linus Walleij