[PATCH 1/3] arm64: dts: rockchip: Add supported UHS-I rates to sdmmc0 on rock-3b

[Tamás Szűcs]

Hi Jonas,

On Tue, Nov 12, 2024 at 11:37 PM Jonas Karlman <jonas at kwiboo.se> wrote:
>
> Hi Tamás,
>
> On 2024-11-12 15:36, Tamás Szűcs wrote:
> > Hi Jonas,
> >
> > Thank you for pointing this out! I haven't noticed this before. I've
> > done some testing and I believe I am able to reproduce the issue you
> > described, although I cannot confirm the reason.
> > The only occasion I encounter any problems is when a UHS SD card or
> > SDIO device is connected to sdmmc0 during bootup. Sometimes the device
> > is recognized as HS only. Obviously no tuning value reported. Also,
> > sdmmc2 cuts out completely. I'm booting from eMMC and when the SD card
> > is removed in this state I lose my rootfs. Certainly, this needs more
> > attention but it seems to be unrelated to the changes here.
> >
> > I need more time to check but are you sure this SD card during bootup
> > issue is gone with UHS-I disabled?
>
> Yes, the issue is that the io voltage domain must be configured to match
> the io signal voltage used, and to use uhs the voltage changes from 3v3
> to 1v8. Causing a miss-match between io voltage domain config and the
> regulator voltage used during initial probe, unless io-domain driver
> happens to be fully loaded before mmc devices are probed.
>
> >
> > Also, in every other case, when you connect any device to sdmmc0 after
> > bootup, performance and stability is perfect.
> > Interestingly I also don't experience this behavior with an eMMC
> > device and / or an SDIO device connected to sdmmc2 during bootup. Only
> > sdmmc0 is problematic and only during bootup.
>
> Yes, as you have discovered, inserting the sd-card after system has
> booted and io-domain driver has been loaded, everything can work as
> expected with uhs speeds.
>
> Until this probe race condition has been solved booting with a sd-card
> inserted may or may not result in wrong tuning or other related issues.
>
> Because of this I advice not to enable uhs mode for sdmmc0 at this time.

All right, and thank you for the explanation. My hands are full at the
moment but let me think about this.

>
> Regards,
> Jonas
>
> >
> > Any more thoughts on this are very welcome.
> >
> > Kind regards,
> > Tamas
> >
> >
> >
> > Tamás Szűcs
> > tszucs at linux.com
> >
> > On Mon, Nov 11, 2024 at 8:00 PM Jonas Karlman <jonas at kwiboo.se> wrote:
> >>
> >> Hi Tamás,
> >>
> >> On 2024-11-11 19:17, Tamás Szűcs wrote:
> >>> Add all supported UHS-I rates to sdmmc0 and allow 200 MHz maximum clock to
> >>> benefit modern SD cards.
> >>>
> >>> Signed-off-by: Tamás Szűcs <tszucs at linux.com>
> >>> ---
> >>>  arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> >>> index 3d0c1ccfaa79..242af5337cdf 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> >>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-rock-3b.dts
> >>> @@ -670,8 +670,14 @@ &sdmmc0 {
> >>>       bus-width = <4>;
> >>>       cap-sd-highspeed;
> >>>       disable-wp;
> >>> +     max-frequency = <200000000>;
> >>>       pinctrl-names = "default";
> >>>       pinctrl-0 = <&sdmmc0_bus4 &sdmmc0_clk &sdmmc0_cmd &sdmmc0_det>;
> >>> +     sd-uhs-sdr12;
> >>> +     sd-uhs-sdr25;
> >>> +     sd-uhs-sdr50;
> >>> +     sd-uhs-sdr104;
> >>> +     sd-uhs-ddr50;
> >>
> >> There is an issue with io-domain driver not always being probed before
> >> mmc driver, this typically result in io-domain being configured wrong,
> >> and mmc tuning happen before io-domain is correctly configured.
> >>
> >> You can usually observe this by looking at the tuning value during boot
> >> and comparing it to the tuning value after removing and re-insering a
> >> sd-card.
> >>
> >> Because of this uhs modes was left out from initial DT submission, some
> >> cards will work others wont, sd-uhs-sdr50 is known to be working with
> >> most cards even with the probe order issue.
> >>
> >> Also I thought that lower speeds where implied?
> >>
> >> Regards,
> >> Jonas
> >>
> >>>       vmmc-supply = <&vcc3v3_sd>;
> >>>       vqmmc-supply = <&vccio_sd>;
> >>>       status = "okay";
> >>
>