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

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

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

On 03/02/2023 09:19, William Qiu wrote:
> This adds the mmc node for the StarFive JH7110 SoC.

Do not use "This xxx". Use imperative mode.
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> Set mmco node to emmc and set mmc1 node to sd.
> 
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>


> +
>  &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 64d260ea1f29..ae1a664e7af5 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -370,6 +370,11 @@ syscrg: clock-controller@13020000 {
>  			#reset-cells = <1>;
>  		};
>  
> +		sysreg: syscon@13030000 {
> +			compatible = "starfive,sysreg", "syscon";

No:
1. Undocumented.
2. A bit too generic. You should have here SoC specific compatible as
well (either as second or third compatible, if all your SoCs share
register layout).

> +			reg = <0x0 0x13030000 0x0 0x1000>;
> +		};
> +

Best regards,
Krzysztof
  

On 2023/2/3 17:02, Krzysztof Kozlowski wrote:
> On 03/02/2023 09:19, William Qiu wrote:
>> This adds the mmc node for the StarFive JH7110 SoC.
> 
> Do not use "This xxx". Use imperative mode.
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> 
>> Set mmco node to emmc and set mmc1 node to sd.
>> 
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> 
> 
>> +
>>  &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 64d260ea1f29..ae1a664e7af5 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> @@ -370,6 +370,11 @@ syscrg: clock-controller@13020000 {
>>  			#reset-cells = <1>;
>>  		};
>>  
>> +		sysreg: syscon@13030000 {
>> +			compatible = "starfive,sysreg", "syscon";
> 
> No:
> 1. Undocumented.
> 2. A bit too generic. You should have here SoC specific compatible as
> well (either as second or third compatible, if all your SoCs share
> register layout).
> 

Hi Krzysztof,

As for the compatible, I will change it to "starfive,jh7110-sysreg"
in next version,but for undocumented, I don't get it, can you
clarify that.
Thank you anyway.

Best regards,
William Qiu
>> +			reg = <0x0 0x13030000 0x0 0x1000>;
>> +		};
>> +
> 
> Best regards,
> Krzysztof
>
  

On 03/02/2023 10:23, William Qiu wrote:
> 
> 
> On 2023/2/3 17:02, Krzysztof Kozlowski wrote:
>> On 03/02/2023 09:19, William Qiu wrote:
>>> This adds the mmc node for the StarFive JH7110 SoC.
>>
>> Do not use "This xxx". Use imperative mode.
>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>>
>>> Set mmco node to emmc and set mmc1 node to sd.
>>>
>>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>>
>>
>>> +
>>>  &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 64d260ea1f29..ae1a664e7af5 100644
>>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>> @@ -370,6 +370,11 @@ syscrg: clock-controller@13020000 {
>>>  			#reset-cells = <1>;
>>>  		};
>>>  
>>> +		sysreg: syscon@13030000 {
>>> +			compatible = "starfive,sysreg", "syscon";
>>
>> No:
>> 1. Undocumented.
>> 2. A bit too generic. You should have here SoC specific compatible as
>> well (either as second or third compatible, if all your SoCs share
>> register layout).
>>
> 
> Hi Krzysztof,
> 
> As for the compatible, I will change it to "starfive,jh7110-sysreg"
> in next version,but for undocumented, I don't get it, can you
> clarify that.
> Thank you anyway.

You need bindings for it. I don't see it in linux-next, cover letter
dependencies nor here.

Best regards,
Krzysztof
  

On 2023/2/3 17:30, Krzysztof Kozlowski wrote:
> On 03/02/2023 10:23, William Qiu wrote:
>> 
>> 
>> On 2023/2/3 17:02, Krzysztof Kozlowski wrote:
>>> On 03/02/2023 09:19, William Qiu wrote:
>>>> This adds the mmc node for the StarFive JH7110 SoC.
>>>
>>> Do not use "This xxx". Use imperative mode.
>>> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>>>
>>>> Set mmco node to emmc and set mmc1 node to sd.
>>>>
>>>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>>>
>>>
>>>> +
>>>>  &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 64d260ea1f29..ae1a664e7af5 100644
>>>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>>> @@ -370,6 +370,11 @@ syscrg: clock-controller@13020000 {
>>>>  			#reset-cells = <1>;
>>>>  		};
>>>>  
>>>> +		sysreg: syscon@13030000 {
>>>> +			compatible = "starfive,sysreg", "syscon";
>>>
>>> No:
>>> 1. Undocumented.
>>> 2. A bit too generic. You should have here SoC specific compatible as
>>> well (either as second or third compatible, if all your SoCs share
>>> register layout).
>>>
>> 
>> Hi Krzysztof,
>> 
>> As for the compatible, I will change it to "starfive,jh7110-sysreg"
>> in next version,but for undocumented, I don't get it, can you
>> clarify that.
>> Thank you anyway.
> 
> You need bindings for it. I don't see it in linux-next, cover letter
> dependencies nor here.
> 

I see, I will update it in next version.
Thank you for taking time to review and provide helpful comments for this patch.

Best regards,
William Qiu
> Best regards,
> Krzysztof
>
  

On Fri, 3 Feb 2023 at 09:21, William Qiu <william.qiu@starfivetech.com> wrote:
>
> This adds the mmc node for the StarFive JH7110 SoC.
> Set mmco node to emmc and set mmc1 node to sd.
>
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> ---
>  .../jh7110-starfive-visionfive-2.dtsi         | 23 ++++++++++++
>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 37 +++++++++++++++++++
>  2 files changed, 60 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> index c60280b89c73..e1a0248e907f 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> @@ -42,6 +42,29 @@ &rtc_osc {
>         clock-frequency = <32768>;
>  };
>
> +&mmc0 {
> +       max-frequency = <100000000>;
> +       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";
> +};
> +
> +&mmc1 {
> +       max-frequency = <100000000>;
> +       bus-width = <4>;
> +       no-sdio;ru
> +       no-mmc;
> +       broken-cd;
> +       cap-sd-highspeed;
> +       post-power-on-delay-ms = <200>;
> +       status = "okay";
> +};

Please add these so they're sorted alphabetically (but keep the clocks
at the top), so there's at least some sort of system.

>  &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 64d260ea1f29..ae1a664e7af5 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -370,6 +370,11 @@ syscrg: clock-controller@13020000 {
>                         #reset-cells = <1>;
>                 };
>
> +               sysreg: syscon@13030000 {
> +                       compatible = "starfive,sysreg", "syscon";
> +                       reg = <0x0 0x13030000 0x0 0x1000>;
> +               };
> +
>                 gpio: gpio@13040000 {
>                         compatible = "starfive,jh7110-sys-pinctrl";
>                         reg = <0x0 0x13040000 0x0 0x10000>;
> @@ -407,5 +412,37 @@ gpioa: gpio@17020000 {
>                         gpio-controller;
>                         #gpio-cells = <2>;
>                 };
> +
> +               mmc0: mmc@16010000 {
> +                       compatible = "starfive,jh7110-mmc";
> +                       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,sysreg = <&sysreg 0x14 0x1a 0x7c000000>;

This may need updating depending on whether you fix the driver or bindings.

> +                       status = "disabled";
> +               };
> +
> +               mmc1: mmc@16020000 {
> +                       compatible = "starfive,jh7110-mmc";
> +                       reg = <0x0 0x16020000 0x0 0x10000>;
> +                       clocks = <&syscrg JH7110_SYSCLK_SDIO1_AHB>,
> +                                <&syscrg JH7110_SYSCLK_SDIO1_SDCARD>;
> +                       clock-names = "biu","ciu";
> +                       resets = <&syscrg JH7110_SYSRST_SDIO1_AHB>;
> +                       reset-names = "reset";
> +                       interrupts = <75>;
> +                       fifo-depth = <32>;
> +                       fifo-watermark-aligned;
> +                       data-addr = <0>;
> +                       starfive,sysreg = <&sysreg 0x9c 0x1 0x3e>;
> +                       status = "disabled";
> +               };
>         };
>  };
> --
> 2.34.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
  

On 2023/2/15 18:31, Emil Renner Berthing wrote:
> On Fri, 3 Feb 2023 at 09:21, William Qiu <william.qiu@starfivetech.com> wrote:
>>
>> This adds the mmc node for the StarFive JH7110 SoC.
>> Set mmco node to emmc and set mmc1 node to sd.
>>
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> ---
>>  .../jh7110-starfive-visionfive-2.dtsi         | 23 ++++++++++++
>>  arch/riscv/boot/dts/starfive/jh7110.dtsi      | 37 +++++++++++++++++++
>>  2 files changed, 60 insertions(+)
>>
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>> index c60280b89c73..e1a0248e907f 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
>> @@ -42,6 +42,29 @@ &rtc_osc {
>>         clock-frequency = <32768>;
>>  };
>>
>> +&mmc0 {
>> +       max-frequency = <100000000>;
>> +       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";
>> +};
>> +
>> +&mmc1 {
>> +       max-frequency = <100000000>;
>> +       bus-width = <4>;
>> +       no-sdio;ru
>> +       no-mmc;
>> +       broken-cd;
>> +       cap-sd-highspeed;
>> +       post-power-on-delay-ms = <200>;
>> +       status = "okay";
>> +};
> 
> Please add these so they're sorted alphabetically (but keep the clocks
> at the top), so there's at least some sort of system.
> 
>>  &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 64d260ea1f29..ae1a664e7af5 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> @@ -370,6 +370,11 @@ syscrg: clock-controller@13020000 {
>>                         #reset-cells = <1>;
>>                 };
>>
>> +               sysreg: syscon@13030000 {
>> +                       compatible = "starfive,sysreg", "syscon";
>> +                       reg = <0x0 0x13030000 0x0 0x1000>;
>> +               };
>> +
>>                 gpio: gpio@13040000 {
>>                         compatible = "starfive,jh7110-sys-pinctrl";
>>                         reg = <0x0 0x13040000 0x0 0x10000>;
>> @@ -407,5 +412,37 @@ gpioa: gpio@17020000 {
>>                         gpio-controller;
>>                         #gpio-cells = <2>;
>>                 };
>> +
>> +               mmc0: mmc@16010000 {
>> +                       compatible = "starfive,jh7110-mmc";
>> +                       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,sysreg = <&sysreg 0x14 0x1a 0x7c000000>;
> 
> This may need updating depending on whether you fix the driver or bindings.
> 
I'll do it then
>> +                       status = "disabled";
>> +               };
>> +
>> +               mmc1: mmc@16020000 {
>> +                       compatible = "starfive,jh7110-mmc";
>> +                       reg = <0x0 0x16020000 0x0 0x10000>;
>> +                       clocks = <&syscrg JH7110_SYSCLK_SDIO1_AHB>,
>> +                                <&syscrg JH7110_SYSCLK_SDIO1_SDCARD>;
>> +                       clock-names = "biu","ciu";
>> +                       resets = <&syscrg JH7110_SYSRST_SDIO1_AHB>;
>> +                       reset-names = "reset";
>> +                       interrupts = <75>;
>> +                       fifo-depth = <32>;
>> +                       fifo-watermark-aligned;
>> +                       data-addr = <0>;
>> +                       starfive,sysreg = <&sysreg 0x9c 0x1 0x3e>;
>> +                       status = "disabled";
>> +               };
>>         };
>>  };
>> --
>> 2.34.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv