Re: [PATCH 8/8] arm64: dts: rockchip: Enable SD-card interface on Radxa E20C

[Yao Zi]

On Sun, Mar 02, 2025 at 12:56:42PM +0100, Jonas Karlman wrote:
> Hi Yao Zi,
> 
> On 2025-03-01 16:15, Yao Zi wrote:
> > On Sat, Mar 01, 2025 at 02:01:05PM +0100, Jonas Karlman wrote:
> >> Hi,
> >>
> >> On 2025-03-01 11:48, Yao Zi wrote:
> >>> SD-card is available on Radxa E20C board.
> >>>
> >>> Signed-off-by: Yao Zi <ziyao@xxxxxxxxxxx>
> >>> ---
> >>>  arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts | 14 ++++++++++++++
> >>>  1 file changed, 14 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> >>> index d2cdb63d4a9d..473065aa4228 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> >>> +++ b/arch/arm64/boot/dts/rockchip/rk3528-radxa-e20c.dts
> >>> @@ -12,6 +12,10 @@ / {
> >>>  	model = "Radxa E20C";
> >>>  	compatible = "radxa,e20c", "rockchip,rk3528";
> >>>  
> >>> +	aliases {
> >>> +		mmc0 = &sdmmc;
> >>
> >> Suggest using mmc1 for sd-card because the e20c typically have onboard
> >> emmc, compared to removable sd-card.
> > 
> > My board doesn't have an eMMC: it's optional as well, but all variants
> > of Radxa E20C come with an SD-card interface. The vendor devicetree sets
> > sdmmc as mmc0 as well[1].
> 
> This is strange as Radxa typically want to align with mmc0=emmc and
> mmc1=sd-card, as seen in [3] and [4].
> 
>   Align with other Radxa products.
>   - mmc0 is eMMC
>   - mmc1 is microSD
> 
> Also mainline U-Boot for Rockchip SoCs typically always treat mmc0 as
> emmc and mmc1 as sd-card, and for most SoCs it will even override the
> board aliases to have some predictability across boards.
> 
> > 
> > I won't insist on it and am willing to take the change if you still
> > consider mmc0 is better.
> 
> Yes, my position is that we should use following:

Ack. I got your point but there's a typo (s/mmc0/mmc1) in my reply.

>   mmc0 = &sdhci;
>   mmc1 = &sdmmc;
> 
> I will send out a short sdhci series based on top of v2 of this series.
> Driver changes was not needed to get basic sdhci working on RK3528 and
> is only required to get HS400 modes working.
> 
> [3] https://lore.kernel.org/r/20240620224435.2752-1-naoki@xxxxxxxxx
> [4] https://lore.kernel.org/r/20240619050047.1217-2-naoki@xxxxxxxxx
> 
> > 
> >>> +	};
> >>> +
> >>>  	chosen {
> >>>  		stdout-path = "serial0:1500000n8";
> >>>  	};
> >>> @@ -20,3 +24,13 @@ chosen {
> >>>  &uart0 {
> >>>  	status = "okay";
> >>>  };
> >>> +
> >>> +&sdmmc {
> 
> This node should be placed above &uart0 to be in alphabetical order.
> 

The original patch keeps the order of nodes in the SoC devicetree
(sorted by MMIO address), but alphabetical order seems more common. Will
fix in v2, thanks.

> >>> +	bus-width = <4>;
> >>> +	cap-mmc-highspeed;
> >>> +	cap-sd-highspeed;
> >>> +	disable-wp;
> >>> +	rockchip,default-sample-phase = <90>;
> >>> +	sd-uhs-sdr104;
 
Thanks,
Yao Zi