Re: [PATCH] arm64: dts: rockchip: add enable-strobe-pulldown to emmc phy on rk3399

[Dragan Simic]

Hello,

On 2024-02-27 11:38, Christopher Obbard wrote:
> On Tue, 2024-02-27 at 10:11 +0000, Folker Schwesinger wrote:
> > On Tue Feb 27, 2024 at 3:05 AM CET, Alban Browaeys wrote:
> > > Le mercredi 24 août 2022 à 07:57 -0700, Doug Anderson a écrit :
> > > > On Tue, Aug 23, 2022 at 8:11 PM Jensen Huang
> > > > <jensenhuang@xxxxxxxxxxxxxxx> wrote:
> > > > > I realized that only some devices may be affected, so I considered
> > > > > modifying rk3399-nanopi4.dtsi only,
> > > > > but other boards without external pull-down should still need this
> > > > > patch.
> > > >
> > > > I guess the other alternative would be to change how the dt property
> > > > works. You could say:
> > > >
> > > > 1. If `enable-strobe-pulldown` is set then enable the strobe
> > > > pulldown.
> > > >
> > > > 2. If `enable-strobe-pulldown` is not set then don't touch the pin in
> > > > the kernel.
> > > >
> > > > 3. If someone later needs to explicitly disable the strobe pulldown
> > > > they could add a new property like `disable-strobe-pulldown`.
> > > >
> > > >
> > > > Obviously there are tradeoffs between that and what you've done and
> > > > I'm happy to let others make the call of which they'd prefer.
> > > >
> > >
> > > Christopher could you try "ROCK Pi 4" and "ROCK Pi 4+" with
> > > "enable-strobe-pulldown" instead of disabling HS400 as you did in July
> > > 2023?
> > >
> >
> > with the following applied, the EMMC related errors are gone. dmesg 
> > only
> > shows "Purging ... bytes" during my tests:
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi
> > b/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi
> > index f2279aa6ca9e..ae0fb87e1a8b 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi
> > @@ -647,8 +647,10 @@ &saradc {
> >  &sdhci {
> >         max-frequency = <150000000>;
> >         bus-width = <8>;
> > -       mmc-hs200-1_8v;
> > +       mmc-hs400-1_8v;
> > +       mmc-hs400-enhanced-strobe;
> >         non-removable;
> > +       rockchip,enable-strobe-pulldown;
> >         status = "okay";
> >  };
> >
> > For testing I ran dd three times in a row:
> >
> > dd if=/dev/zero of=./zero.bin bs=1M count=5000
> >
> > I tested this on both a Rock 4SE board and a Rock Pi 4B+ board with 
> > the
> > same results.
>
> Just for the record, all Rock 4 board schematics have no external 
> strobe
> pulldown resistor on the board, so we should be good to enable this.
>
> I wonder what other boards this could be enabled for ?
>
> It seemed to be the case on some eMMC it would work, others it 
> wouldn't.
>
> I haven't yet tested the above diff here as yet, but I can do that this 
> week
> on multiple boards & eMMC combos.
>
> The diff above is also missing a fixes tag (and also be fixed for 
> rk3399-rock-
> 4c-plus.dts).

When it comes to HS400 support on the RockPro64 and the Pinebook Pro,
they're unfortunately miswired in a hopeless way, causing the DATA
STROBE signal from the eMMC chip (i.e. the eMMC module) not to even
reach the right ball on the RK3399 SoC.

Here are a few screenshots from the schematics that illustrate this
issue (the first one is from the eMMC module schematic, the remaining
two are from the RockPro64 schematic, which are pretty much exactly
the same in the Pinebook Pro schematic):

- https://i.imgur.com/MqD7rcG.png
- https://i.imgur.com/hrlQBck.png
- https://i.imgur.com/mtYvc9s.png

There have been some Pine64 community attempts to have this fixed in
new RockPro64 and Pinebook Pro hardware revisions, but such attempts
unfortunately failed. :/

Thus, we'll unfortunately have to deal with the whole thing on the
board level, instead of making changes on the SoC level.