On 2023/1/5 0:05, Ulf Hansson wrote:
> On Wed, 4 Jan 2023 at 07:08, William Qiu <william.qiu@xxxxxxxxxxxxxxxx> wrote:
>>
>>
>>
>> On 2023/1/2 22:03, Ulf Hansson wrote:
>> > On Tue, 27 Dec 2022 at 13:22, William Qiu <william.qiu@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
>> >> ---
>> >> .../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
