Re: [PATCH v2 3/3] riscv: dts: starfive: Add mmc node

[William Qiu <william.qiu@starfivetech.com>]

On 2023/1/5 0:05, Ulf Hansson wrote:
> On Wed, 4 Jan 2023 at 07:08, William Qiu <william.qiu@starfivetech.com> wrote:
>>
>>
>>
>> On 2023/1/2 22:03, Ulf Hansson wrote:
>> > On Tue, 27 Dec 2022 at 13:22, William Qiu <william.qiu@starfivetech.com> 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..d8244fd1f5a0 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>;
>> >>  };
>> >>
>> >> +&mmc0 {
>> >> +       max-frequency = <100000000>;
>> >> +       card-detect-delay = <300>;
>> >
>> > Nitpick:  This seems redundant for a non-removable card!?
>> >
>>
>> Will drop
>>
>> >> +       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>;
>> >> +       card-detect-delay = <300>;
>> >
>> > Nitpick: This looks redundant for polling based card detection
>> > (broken-cd is set a few lines below).
>> >
>>
>> Will drop
>>
>> >> +       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..08a780d2c0f4 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>;
>> >>                 };
>> >>
>> >> +               syscon: syscon@13030000 {
>> >> +                       compatible = "starfive,syscon", "syscon";
>> >> +                       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 */
>> >
>> > Don't confuse the mmc0 node name with mmcblk0. There is no guarantee
>> > that this is true, unless you also specify an alias.
>> >
>>
>> Hi Ulf,
>>
>> Thank you for taking time to review and provide helpful comments for this patch.
>> Actually we define mmc0 as eMMC, which is mmcblk0 in the kernel, and define mmc1 as SDIO,
>> which is mmcblk1 in the kernel, so it's not confuse.
>>
> 
> My point is, mmc0 from DT node perspective doesn't necessarily need to
> map to mmc0, as that depends on the "probe" order of the devices. At
> least for the Linux kernel, mmc0 from DT point of view, could end up
> being mmc1.
> 
> To avoid confusion, please drop the "mmcblk*" here. It's anyway a
> Linux specific thing. Don't get me wrong, feel free to keep the
> information about eMMC and SDIO for the corresponding mmc controller
> node.
> 
> Moreover, if you can't use PARTID/UUID to find the rootfs device -
> then you may use an aliases node, to let mmc0 to be enumerated as
> mmc0, for example. See below.
> 
> aliases {
>      mmc0 = &mmc0;
> }
> 
> Kind regards
> Uffe

Hi Uffe,

Thank you for taking time to review.
I'll take your suggestion into consideration and drop the "mmcblk*".

Best Regards
William Qiu