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

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

[Jensen Huang]

Internal pull-down for strobe line (GRF_EMMCPHY_CON2[9]) was disabled
by commit 8b5c2b45b8f0, which causes I/O error in HS400 mode.

Tested on NanoPC-T4.

Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
Signed-off-by: Jensen Huang <jensenhuang@friendlyarm.com>

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 9d5b0e8c9cca..9491cafbbaa3 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -1561,6 +1561,7 @@ emmc_phy: phy@f780 {
 			clock-names = "emmcclk";
 			drive-impedance-ohm = <50>;
 			#phy-cells = <0>;
+			rockchip,enable-strobe-pulldown;
 			status = "disabled";
 		};
 
-- 
2.37.0

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

[Heiko Stübner]

Am Montag, 22. August 2022, 09:41:39 CEST schrieb Jensen Huang:
> Internal pull-down for strobe line (GRF_EMMCPHY_CON2[9]) was disabled
> by commit 8b5c2b45b8f0, which causes I/O error in HS400 mode.
> 
> Tested on NanoPC-T4.
> 
> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
> Signed-off-by: Jensen Huang <jensenhuang@friendlyarm.com>

ok, so this looks like it restores previous functionality.

I'm just wondering as the "offending" patch is from 2020, why this
only turns up now. Any ideas?

> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index 9d5b0e8c9cca..9491cafbbaa3 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -1561,6 +1561,7 @@ emmc_phy: phy@f780 {
>  			clock-names = "emmcclk";
>  			drive-impedance-ohm = <50>;
>  			#phy-cells = <0>;
> +			rockchip,enable-strobe-pulldown;
>  			status = "disabled";
>  		};
>  
> 

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

[Doug Anderson]

Hi,

On Tue, Aug 23, 2022 at 4:53 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Montag, 22. August 2022, 09:41:39 CEST schrieb Jensen Huang:
> > Internal pull-down for strobe line (GRF_EMMCPHY_CON2[9]) was disabled
> > by commit 8b5c2b45b8f0, which causes I/O error in HS400 mode.
> >
> > Tested on NanoPC-T4.
> >
> > Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
> > Signed-off-by: Jensen Huang <jensenhuang@friendlyarm.com>
>
> ok, so this looks like it restores previous functionality.
>
> I'm just wondering as the "offending" patch is from 2020, why this
> only turns up now. Any ideas?

Ah, I see. So before the offending patch we used to just leave the
pull state at whatever the default was when the kernel was booted.
After the offending patch we chose a default.

On kevin I see an external pull down on this line. Enabling both the
internal and external is probably not a huge deal, it'll just affect
the strength of the pull.

On bob I _think_ the external pull down is also stuffed.

...so I guess that would explain why it didn't cause a problem for at
least those two boards?

-Doug

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

[Jensen Huang]

Hi,

Sorry for sending an email in HTML format.

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.


BR,
Jensen

On Tue, Aug 23, 2022 at 10:13 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Aug 23, 2022 at 4:53 AM Heiko Stübner <heiko@sntech.de> wrote:
> >
> > Am Montag, 22. August 2022, 09:41:39 CEST schrieb Jensen Huang:
> > > Internal pull-down for strobe line (GRF_EMMCPHY_CON2[9]) was disabled
> > > by commit 8b5c2b45b8f0, which causes I/O error in HS400 mode.
> > >
> > > Tested on NanoPC-T4.
> > >
> > > Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
> > > Signed-off-by: Jensen Huang <jensenhuang@friendlyarm.com>
> >
> > ok, so this looks like it restores previous functionality.
> >
> > I'm just wondering as the "offending" patch is from 2020, why this
> > only turns up now. Any ideas?
>
> Ah, I see. So before the offending patch we used to just leave the
> pull state at whatever the default was when the kernel was booted.
> After the offending patch we chose a default.
>
> On kevin I see an external pull down on this line. Enabling both the
> internal and external is probably not a huge deal, it'll just affect
> the strength of the pull.
>
> On bob I _think_ the external pull down is also stuffed.
>
> ...so I guess that would explain why it didn't cause a problem for at
> least those two boards?
>
> -Doug

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

[Doug Anderson]

Hi,

On Tue, Aug 23, 2022 at 8:11 PM Jensen Huang
<jensenhuang@friendlyarm.com> wrote:
>
> Hi,
>
> Sorry for sending an email in HTML format.
>
> 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.


> BR,
> Jensen
>
> On Tue, Aug 23, 2022 at 10:13 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Tue, Aug 23, 2022 at 4:53 AM Heiko Stübner <heiko@sntech.de> wrote:
> > >
> > > Am Montag, 22. August 2022, 09:41:39 CEST schrieb Jensen Huang:
> > > > Internal pull-down for strobe line (GRF_EMMCPHY_CON2[9]) was disabled
> > > > by commit 8b5c2b45b8f0, which causes I/O error in HS400 mode.
> > > >
> > > > Tested on NanoPC-T4.
> > > >
> > > > Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
> > > > Signed-off-by: Jensen Huang <jensenhuang@friendlyarm.com>
> > >
> > > ok, so this looks like it restores previous functionality.
> > >
> > > I'm just wondering as the "offending" patch is from 2020, why this
> > > only turns up now. Any ideas?
> >
> > Ah, I see. So before the offending patch we used to just leave the
> > pull state at whatever the default was when the kernel was booted.
> > After the offending patch we chose a default.
> >
> > On kevin I see an external pull down on this line. Enabling both the
> > internal and external is probably not a huge deal, it'll just affect
> > the strength of the pull.
> >
> > On bob I _think_ the external pull down is also stuffed.
> >
> > ...so I guess that would explain why it didn't cause a problem for at
> > least those two boards?
> >
> > -Doug

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

[Jensen Huang]

Hi,

Thanks!
I understand that this patch does potentially affect boards with
external pull down.
To avoid this, I will move `enable-strobe-pulldown` to
rk3399-nanopi4.dtsi and send patch v2.


BR,
Jensen

On Wed, Aug 24, 2022 at 10:58 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Aug 23, 2022 at 8:11 PM Jensen Huang
> <jensenhuang@friendlyarm.com> wrote:
> >
> > Hi,
> >
> > Sorry for sending an email in HTML format.
> >
> > 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.
>
>
> > BR,
> > Jensen
> >
> > On Tue, Aug 23, 2022 at 10:13 PM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Aug 23, 2022 at 4:53 AM Heiko Stübner <heiko@sntech.de> wrote:
> > > >
> > > > Am Montag, 22. August 2022, 09:41:39 CEST schrieb Jensen Huang:
> > > > > Internal pull-down for strobe line (GRF_EMMCPHY_CON2[9]) was disabled
> > > > > by commit 8b5c2b45b8f0, which causes I/O error in HS400 mode.
> > > > >
> > > > > Tested on NanoPC-T4.
> > > > >
> > > > > Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
> > > > > Signed-off-by: Jensen Huang <jensenhuang@friendlyarm.com>
> > > >
> > > > ok, so this looks like it restores previous functionality.
> > > >
> > > > I'm just wondering as the "offending" patch is from 2020, why this
> > > > only turns up now. Any ideas?
> > >
> > > Ah, I see. So before the offending patch we used to just leave the
> > > pull state at whatever the default was when the kernel was booted.
> > > After the offending patch we chose a default.
> > >
> > > On kevin I see an external pull down on this line. Enabling both the
> > > internal and external is probably not a huge deal, it'll just affect
> > > the strength of the pull.
> > >
> > > On bob I _think_ the external pull down is also stuffed.
> > >
> > > ...so I guess that would explain why it didn't cause a problem for at
> > > least those two boards?
> > >
> > > -Doug

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

[Alban Browaeys]

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@friendlyarm.com> 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?


Could the behavior be reverted to let the vendor kernel default for the
default case (ie enable pulldown)?




I believe 99% of the boards are now broken with this new internal
pulldown behavior (all  these with internal pulldown). More on that
later but to sum up, nobody  complained because downstream kernels like
Armbian all disabled HS400 for all boards, at least for rk3399.


Do we really want to ask 99% of the board dts to add this "enable-
strobe-pulldown" in their dts?
Chris, was your custom board not working with the vender kernel default
to enable unconditionaly?
What was the rationale to  choose the opposite default from the vendor
kernel one?


As told in the commit that introduced this new behavior the default for
the vendor kernel was the opposite of what was introduced in the Linux
kernel:
"
https://github.com/torvalds/linux/commit/8b5c2b45b8f0a11c9072da0f7baf9ee986d3151e

commit 8b5c2b45b8f0a11c9072da0f7baf9ee986d3151e
Author: Chris Ruehl <chris.ruehl@gtsys.com.hk>
Date:   Sun Nov 29 13:44:14 2020 +0800

phy: rockchip: set pulldown for strobe line in dts

This patch add support to set the internal pulldown via dt property
and allow simplify the board design for the trace from emmc-phy to
the eMMC chipset.
Default to not set the pull-down.

This patch was inspired from the 4.4 tree of the
Rockchip SDK, where it is enabled unconditional.
The patch had been tested with our rk3399 customized board.
"



For RK3588 I see this commit which makes me believe the internal
pulldown case is the most common "
commit 37f3d6108730713c411827ab4af764909f4dfc78
Author: Sam Edwards <cfsworks@gmail.com>
Date:   Tue Dec 5 12:29:00 2023 -0800


arm64: dts: rockchip: Fix eMMC Data Strobe PD on rk3588

JEDEC standard JESD84-B51 defines the eMMC Data Strobe line, which is
currently used only in HS400 mode, as a device->host clock signal that
"is used only in read operation. The Data Strobe is always High-Z (not
driven by the device and pulled down by RDS) or Driven Low in write
operation, except during CRC status response." RDS is a pull-down
resistor specified in the 10K-100K ohm range. Thus per the standard,
the
Data Strobe is always pulled to ground (by the eMMC and/or RDS) during
write operations.

Evidently, the eMMC host controller in the RK3588 considers an active
voltage on the eMMC-DS line during a write to be an error.

The default (i.e. hardware reset, and Rockchip BSP) behavior for the
RK3588 is to activate the eMMC-DS pin's builtin pull-down. As a result,
many RK3588 board designers do not bother adding a dedicated RDS
resistor, instead relying on the RK3588's internal bias. The current
devicetree, however, disables this bias (`pcfg_pull_none`), breaking
HS400-mode writes for boards without a dedicated RDS, but with an eMMC
chip that chooses to High-Z (instead of drive-low) the eMMC-DS line.
(The Turing RK1 is one such board.)

Fix this by changing the bias in the (common) emmc_data_strobe case to
reflect the expected hardware/BSP behavior. This is unlikely to cause
regressions elsewhere: the pull-down is only relevant for High-Z eMMCs,
and if this is redundant with a (dedicated) RDS resistor, the effective
result is only a lower resistance to ground -- where the range of
tolerance is quite high. If it does, it's better fixed in the specific
devicetrees.
"






Lately two other upstream dts disabled HS400 due to this new behavior I
believe:
"
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/arm64/boot/dts/rockchip?id=2bd1d2dd808c60532283e9cf05110bf1bf2f9079
author	Christopher Obbard <chris.obbard@collabora.com>	2023-
07-05 15:42:55 +0100
committer	Heiko Stuebner <heiko@sntech.de>	2023-07-10
15:43:24 +0200
commit	2bd1d2dd808c60532283e9cf05110bf1bf2f9079 (patch)
tree	57cbf7eaa91deb68f143577d5d1dbc0d9141480e
/arch/arm64/boot/dts/rockchip
parent	cee572756aa2cb46e959e9797ad4b730b78a050b (diff)
download	linux-2bd1d2dd808c60532283e9cf05110bf1bf2f9079.tar.gz
arm64: dts: rockchip: Disable HS400 for eMMC on ROCK 4C+


https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/arm64/boot/dts/rockchip?id=cee572756aa2cb46e959e9797ad4b730b78a050b
author	Christopher Obbard <chris.obbard@collabora.com>	2023-
07-05 15:42:54 +0100
committer	Heiko Stuebner <heiko@sntech.de>	2023-07-10
15:43:24 +0200
commit	cee572756aa2cb46e959e9797ad4b730b78a050b (patch)
tree	cf3ed8ff6230cbde04353503417c1a75ba15c249
/arch/arm64/boot/dts/rockchip
parent	5ce6971e5279c569defc2f2ac800692049bbaa90 (diff)
download	linux-cee572756aa2cb46e959e9797ad4b730b78a050b.tar.gz
arm64: dts: rockchip: Disable HS400 for eMMC on ROCK Pi 4
"


All Armbian RK3399 boards, as far as I know, had to disable HS400, I
also believe due to this commit.

You can also search google for "running cqe recovery rk3399 armbian".


This was never reported upstream though. But as HS400 is disabled
everywhere nobody notice the regression nowadays.


> 
> > BR,
> > Jensen
> > 
> > On Tue, Aug 23, 2022 at 10:13 PM Doug Anderson
> > <dianders@chromium.org> wrote:
> > > 
> > > Hi,
> > > 
> > > On Tue, Aug 23, 2022 at 4:53 AM Heiko Stübner <heiko@sntech.de>
> > > wrote:
> > > > 
> > > > Am Montag, 22. August 2022, 09:41:39 CEST schrieb Jensen Huang:
> > > > > Internal pull-down for strobe line (GRF_EMMCPHY_CON2[9]) was
> > > > > disabled
> > > > > by commit 8b5c2b45b8f0, which causes I/O error in HS400 mode.
> > > > > 
> > > > > Tested on NanoPC-T4.
> > > > > 
> > > > > Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe
> > > > > line in dts")
> > > > > Signed-off-by: Jensen Huang <jensenhuang@friendlyarm.com>
> > > > 
> > > > ok, so this looks like it restores previous functionality.
> > > > 
> > > > I'm just wondering as the "offending" patch is from 2020, why
> > > > this
> > > > only turns up now. Any ideas?
> > > 

Probbaly because the introduction of PROBE_DEFERRED in regulator core
before that (in 5.10.60) already broke at least my board HS400 due to
double init. Thus why it took me so long to see this new pulldown
behavior commit. I was testing every regulator core double init fixup
patchset while not understanding why reverting the PROBE_DEFERRED
commit on 5.10.60 worked but not on newer kernels (ie this new pulldown
behavior was introduced in 5.11...).




> > > Ah, I see. So before the offending patch we used to just leave
> > > the
> > > pull state at whatever the default was when the kernel was
> > > booted.
> > > After the offending patch we chose a default.
> > > 
> > > On kevin I see an external pull down on this line. Enabling both
> > > the
> > > internal and external is probably not a huge deal, it'll just
> > > affect
> > > the strength of the pull.
> > > 
> > > On bob I _think_ the external pull down is also stuffed.
> > > 
> > > ...so I guess that would explain why it didn't cause a problem
> > > for at
> > > least those two boards?
> > > 
> > > -Doug


In my opinion it is about these board already being broken by the
regulator core change, so nobody noticed the second regression. When
the first regression was fixed, it was very hard to correlate the still
broken behavior to the second regression.


I confirm that on Helios64, setting "enable-strobe-pulldown" fixes the
EMMC error I had when writing with HS400ES enabled:
mmc1: running CQE recovery 
mmc1: cqhci: spurious TCN for tag 12

It also took me so long to report upstream as my board code (rk3399-
kobol-helios64.dts) is not completely upstreamed yet so I use an
Armbian patched kernel.



Alban

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

[Folker Schwesinger]

[-- Attachment #1: Type: text/plain, Size: 10325 bytes --]

Hi,

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@friendlyarm.com> 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.

>
> Could the behavior be reverted to let the vendor kernel default for the
> default case (ie enable pulldown)?
>
>
>
>
> I believe 99% of the boards are now broken with this new internal
> pulldown behavior (all  these with internal pulldown). More on that
> later but to sum up, nobody  complained because downstream kernels like
> Armbian all disabled HS400 for all boards, at least for rk3399.
>
>
> Do we really want to ask 99% of the board dts to add this "enable-
> strobe-pulldown" in their dts?
> Chris, was your custom board not working with the vender kernel default
> to enable unconditionaly?
> What was the rationale to  choose the opposite default from the vendor
> kernel one?
>
>
> As told in the commit that introduced this new behavior the default for
> the vendor kernel was the opposite of what was introduced in the Linux
> kernel:
> "
> https://github.com/torvalds/linux/commit/8b5c2b45b8f0a11c9072da0f7baf9ee986d3151e
>
> commit 8b5c2b45b8f0a11c9072da0f7baf9ee986d3151e
> Author: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> Date:   Sun Nov 29 13:44:14 2020 +0800
>
> phy: rockchip: set pulldown for strobe line in dts
>
> This patch add support to set the internal pulldown via dt property
> and allow simplify the board design for the trace from emmc-phy to
> the eMMC chipset.
> Default to not set the pull-down.
>
> This patch was inspired from the 4.4 tree of the
> Rockchip SDK, where it is enabled unconditional.
> The patch had been tested with our rk3399 customized board.
> "
>
>
>
> For RK3588 I see this commit which makes me believe the internal
> pulldown case is the most common "
> commit 37f3d6108730713c411827ab4af764909f4dfc78
> Author: Sam Edwards <cfsworks@gmail.com>
> Date:   Tue Dec 5 12:29:00 2023 -0800
>
>
> arm64: dts: rockchip: Fix eMMC Data Strobe PD on rk3588
>
> JEDEC standard JESD84-B51 defines the eMMC Data Strobe line, which is
> currently used only in HS400 mode, as a device->host clock signal that
> "is used only in read operation. The Data Strobe is always High-Z (not
> driven by the device and pulled down by RDS) or Driven Low in write
> operation, except during CRC status response." RDS is a pull-down
> resistor specified in the 10K-100K ohm range. Thus per the standard,
> the
> Data Strobe is always pulled to ground (by the eMMC and/or RDS) during
> write operations.
>
> Evidently, the eMMC host controller in the RK3588 considers an active
> voltage on the eMMC-DS line during a write to be an error.
>
> The default (i.e. hardware reset, and Rockchip BSP) behavior for the
> RK3588 is to activate the eMMC-DS pin's builtin pull-down. As a result,
> many RK3588 board designers do not bother adding a dedicated RDS
> resistor, instead relying on the RK3588's internal bias. The current
> devicetree, however, disables this bias (`pcfg_pull_none`), breaking
> HS400-mode writes for boards without a dedicated RDS, but with an eMMC
> chip that chooses to High-Z (instead of drive-low) the eMMC-DS line.
> (The Turing RK1 is one such board.)
>
> Fix this by changing the bias in the (common) emmc_data_strobe case to
> reflect the expected hardware/BSP behavior. This is unlikely to cause
> regressions elsewhere: the pull-down is only relevant for High-Z eMMCs,
> and if this is redundant with a (dedicated) RDS resistor, the effective
> result is only a lower resistance to ground -- where the range of
> tolerance is quite high. If it does, it's better fixed in the specific
> devicetrees.
> "
>
>
>
>
>
>
> Lately two other upstream dts disabled HS400 due to this new behavior I
> believe:
> "
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/arm64/boot/dts/rockchip?id=2bd1d2dd808c60532283e9cf05110bf1bf2f9079
> author	Christopher Obbard <chris.obbard@collabora.com>	2023-
> 07-05 15:42:55 +0100
> committer	Heiko Stuebner <heiko@sntech.de>	2023-07-10
> 15:43:24 +0200
> commit	2bd1d2dd808c60532283e9cf05110bf1bf2f9079 (patch)
> tree	57cbf7eaa91deb68f143577d5d1dbc0d9141480e
> /arch/arm64/boot/dts/rockchip
> parent	cee572756aa2cb46e959e9797ad4b730b78a050b (diff)
> download	linux-2bd1d2dd808c60532283e9cf05110bf1bf2f9079.tar.gz
> arm64: dts: rockchip: Disable HS400 for eMMC on ROCK 4C+
>
>
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/arm64/boot/dts/rockchip?id=cee572756aa2cb46e959e9797ad4b730b78a050b
> author	Christopher Obbard <chris.obbard@collabora.com>	2023-
> 07-05 15:42:54 +0100
> committer	Heiko Stuebner <heiko@sntech.de>	2023-07-10
> 15:43:24 +0200
> commit	cee572756aa2cb46e959e9797ad4b730b78a050b (patch)
> tree	cf3ed8ff6230cbde04353503417c1a75ba15c249
> /arch/arm64/boot/dts/rockchip
> parent	5ce6971e5279c569defc2f2ac800692049bbaa90 (diff)
> download	linux-cee572756aa2cb46e959e9797ad4b730b78a050b.tar.gz
> arm64: dts: rockchip: Disable HS400 for eMMC on ROCK Pi 4
> "
>
>
> All Armbian RK3399 boards, as far as I know, had to disable HS400, I
> also believe due to this commit.
>
> You can also search google for "running cqe recovery rk3399 armbian".
>
>
> This was never reported upstream though. But as HS400 is disabled
> everywhere nobody notice the regression nowadays.
>
>
> > 
> > > BR,
> > > Jensen
> > > 
> > > On Tue, Aug 23, 2022 at 10:13 PM Doug Anderson
> > > <dianders@chromium.org> wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > On Tue, Aug 23, 2022 at 4:53 AM Heiko Stübner <heiko@sntech.de>
> > > > wrote:
> > > > > 
> > > > > Am Montag, 22. August 2022, 09:41:39 CEST schrieb Jensen Huang:
> > > > > > Internal pull-down for strobe line (GRF_EMMCPHY_CON2[9]) was
> > > > > > disabled
> > > > > > by commit 8b5c2b45b8f0, which causes I/O error in HS400 mode.
> > > > > > 
> > > > > > Tested on NanoPC-T4.
> > > > > > 
> > > > > > Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe
> > > > > > line in dts")
> > > > > > Signed-off-by: Jensen Huang <jensenhuang@friendlyarm.com>
> > > > > 
> > > > > ok, so this looks like it restores previous functionality.
> > > > > 
> > > > > I'm just wondering as the "offending" patch is from 2020, why
> > > > > this
> > > > > only turns up now. Any ideas?
> > > > 
>
> Probbaly because the introduction of PROBE_DEFERRED in regulator core
> before that (in 5.10.60) already broke at least my board HS400 due to
> double init. Thus why it took me so long to see this new pulldown
> behavior commit. I was testing every regulator core double init fixup
> patchset while not understanding why reverting the PROBE_DEFERRED
> commit on 5.10.60 worked but not on newer kernels (ie this new pulldown
> behavior was introduced in 5.11...).
>
>
>
>
> > > > Ah, I see. So before the offending patch we used to just leave
> > > > the
> > > > pull state at whatever the default was when the kernel was
> > > > booted.
> > > > After the offending patch we chose a default.
> > > > 
> > > > On kevin I see an external pull down on this line. Enabling both
> > > > the
> > > > internal and external is probably not a huge deal, it'll just
> > > > affect
> > > > the strength of the pull.
> > > > 
> > > > On bob I _think_ the external pull down is also stuffed.
> > > > 
> > > > ...so I guess that would explain why it didn't cause a problem
> > > > for at
> > > > least those two boards?
> > > > 
> > > > -Doug
>
>
> In my opinion it is about these board already being broken by the
> regulator core change, so nobody noticed the second regression. When
> the first regression was fixed, it was very hard to correlate the still
> broken behavior to the second regression.
>
>
> I confirm that on Helios64, setting "enable-strobe-pulldown" fixes the
> EMMC error I had when writing with HS400ES enabled:
> mmc1: running CQE recovery 
> mmc1: cqhci: spurious TCN for tag 12
>
> It also took me so long to report upstream as my board code (rk3399-
> kobol-helios64.dts) is not completely upstreamed yet so I use an
> Armbian patched kernel.
>
>
>
> Alban
>
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

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

[Christopher Obbard]

Hi,

On Tue, 2024-02-27 at 10:11 +0000, Folker Schwesinger wrote:
> Hi,
> 
> 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@friendlyarm.com> 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).


> 
> > 
> > Could the behavior be reverted to let the vendor kernel default for the
> > default case (ie enable pulldown)?
> > 
> > 
> > 
> > 
> > I believe 99% of the boards are now broken with this new internal
> > pulldown behavior (all  these with internal pulldown). More on that
> > later but to sum up, nobody  complained because downstream kernels like
> > Armbian all disabled HS400 for all boards, at least for rk3399.
> > 
> > 
> > Do we really want to ask 99% of the board dts to add this "enable-
> > strobe-pulldown" in their dts?
> > Chris, was your custom board not working with the vender kernel default
> > to enable unconditionaly?
> > What was the rationale to  choose the opposite default from the vendor
> > kernel one?
> > 
> > 
> > As told in the commit that introduced this new behavior the default for
> > the vendor kernel was the opposite of what was introduced in the Linux
> > kernel:
> > "
> > https://github.com/torvalds/linux/commit/8b5c2b45b8f0a11c9072da0f7baf9ee986d3151e
> > 
> > commit 8b5c2b45b8f0a11c9072da0f7baf9ee986d3151e
> > Author: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> > Date:   Sun Nov 29 13:44:14 2020 +0800
> > 
> > phy: rockchip: set pulldown for strobe line in dts
> > 
> > This patch add support to set the internal pulldown via dt property
> > and allow simplify the board design for the trace from emmc-phy to
> > the eMMC chipset.
> > Default to not set the pull-down.
> > 
> > This patch was inspired from the 4.4 tree of the
> > Rockchip SDK, where it is enabled unconditional.
> > The patch had been tested with our rk3399 customized board.
> > "
> > 
> > 
> > 
> > For RK3588 I see this commit which makes me believe the internal
> > pulldown case is the most common "
> > commit 37f3d6108730713c411827ab4af764909f4dfc78
> > Author: Sam Edwards <cfsworks@gmail.com>
> > Date:   Tue Dec 5 12:29:00 2023 -0800
> > 
> > 
> > arm64: dts: rockchip: Fix eMMC Data Strobe PD on rk3588
> > 
> > JEDEC standard JESD84-B51 defines the eMMC Data Strobe line, which is
> > currently used only in HS400 mode, as a device->host clock signal that
> > "is used only in read operation. The Data Strobe is always High-Z (not
> > driven by the device and pulled down by RDS) or Driven Low in write
> > operation, except during CRC status response." RDS is a pull-down
> > resistor specified in the 10K-100K ohm range. Thus per the standard,
> > the
> > Data Strobe is always pulled to ground (by the eMMC and/or RDS) during
> > write operations.
> > 
> > Evidently, the eMMC host controller in the RK3588 considers an active
> > voltage on the eMMC-DS line during a write to be an error.
> > 
> > The default (i.e. hardware reset, and Rockchip BSP) behavior for the
> > RK3588 is to activate the eMMC-DS pin's builtin pull-down. As a result,
> > many RK3588 board designers do not bother adding a dedicated RDS
> > resistor, instead relying on the RK3588's internal bias. The current
> > devicetree, however, disables this bias (`pcfg_pull_none`), breaking
> > HS400-mode writes for boards without a dedicated RDS, but with an eMMC
> > chip that chooses to High-Z (instead of drive-low) the eMMC-DS line.
> > (The Turing RK1 is one such board.)
> > 
> > Fix this by changing the bias in the (common) emmc_data_strobe case to
> > reflect the expected hardware/BSP behavior. This is unlikely to cause
> > regressions elsewhere: the pull-down is only relevant for High-Z eMMCs,
> > and if this is redundant with a (dedicated) RDS resistor, the effective
> > result is only a lower resistance to ground -- where the range of
> > tolerance is quite high. If it does, it's better fixed in the specific
> > devicetrees.
> > "
> > 
> > 
> > 
> > 
> > 
> > 
> > Lately two other upstream dts disabled HS400 due to this new behavior I
> > believe:
> > "
> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/arm64/boot/dts/rockchip?id=2bd1d2dd808c60532283e9cf05110bf1bf2f9079
> > author	Christopher Obbard <chris.obbard@collabora.com>	2023-
> > 07-05 15:42:55 +0100
> > committer	Heiko Stuebner <heiko@sntech.de>	2023-07-10
> > 15:43:24 +0200
> > commit	2bd1d2dd808c60532283e9cf05110bf1bf2f9079 (patch)
> > tree	57cbf7eaa91deb68f143577d5d1dbc0d9141480e
> > /arch/arm64/boot/dts/rockchip
> > parent	cee572756aa2cb46e959e9797ad4b730b78a050b (diff)
> > download	linux-2bd1d2dd808c60532283e9cf05110bf1bf2f9079.tar.gz
> > arm64: dts: rockchip: Disable HS400 for eMMC on ROCK 4C+
> > 
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/arm64/boot/dts/rockchip?id=cee572756aa2cb46e959e9797ad4b730b78a050b
> > author	Christopher Obbard <chris.obbard@collabora.com>	2023-
> > 07-05 15:42:54 +0100
> > committer	Heiko Stuebner <heiko@sntech.de>	2023-07-10
> > 15:43:24 +0200
> > commit	cee572756aa2cb46e959e9797ad4b730b78a050b (patch)
> > tree	cf3ed8ff6230cbde04353503417c1a75ba15c249
> > /arch/arm64/boot/dts/rockchip
> > parent	5ce6971e5279c569defc2f2ac800692049bbaa90 (diff)
> > download	linux-cee572756aa2cb46e959e9797ad4b730b78a050b.tar.gz
> > arm64: dts: rockchip: Disable HS400 for eMMC on ROCK Pi 4
> > "
> > 
> > 
> > All Armbian RK3399 boards, as far as I know, had to disable HS400, I
> > also believe due to this commit.
> > 
> > You can also search google for "running cqe recovery rk3399 armbian".
> > 
> > 
> > This was never reported upstream though. But as HS400 is disabled
> > everywhere nobody notice the regression nowadays.
> > 
> > 
> > > 
> > > > BR,
> > > > Jensen
> > > > 
> > > > On Tue, Aug 23, 2022 at 10:13 PM Doug Anderson
> > > > <dianders@chromium.org> wrote:
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > On Tue, Aug 23, 2022 at 4:53 AM Heiko Stübner <heiko@sntech.de>
> > > > > wrote:
> > > > > > 
> > > > > > Am Montag, 22. August 2022, 09:41:39 CEST schrieb Jensen Huang:
> > > > > > > Internal pull-down for strobe line (GRF_EMMCPHY_CON2[9]) was
> > > > > > > disabled
> > > > > > > by commit 8b5c2b45b8f0, which causes I/O error in HS400 mode.
> > > > > > > 
> > > > > > > Tested on NanoPC-T4.
> > > > > > > 
> > > > > > > Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe
> > > > > > > line in dts")
> > > > > > > Signed-off-by: Jensen Huang <jensenhuang@friendlyarm.com>
> > > > > > 
> > > > > > ok, so this looks like it restores previous functionality.
> > > > > > 
> > > > > > I'm just wondering as the "offending" patch is from 2020, why
> > > > > > this
> > > > > > only turns up now. Any ideas?
> > > > > 
> > 
> > Probbaly because the introduction of PROBE_DEFERRED in regulator core
> > before that (in 5.10.60) already broke at least my board HS400 due to
> > double init. Thus why it took me so long to see this new pulldown
> > behavior commit. I was testing every regulator core double init fixup
> > patchset while not understanding why reverting the PROBE_DEFERRED
> > commit on 5.10.60 worked but not on newer kernels (ie this new pulldown
> > behavior was introduced in 5.11...).
> > 
> > 
> > 
> > 
> > > > > Ah, I see. So before the offending patch we used to just leave
> > > > > the
> > > > > pull state at whatever the default was when the kernel was
> > > > > booted.
> > > > > After the offending patch we chose a default.
> > > > > 
> > > > > On kevin I see an external pull down on this line. Enabling both
> > > > > the
> > > > > internal and external is probably not a huge deal, it'll just
> > > > > affect
> > > > > the strength of the pull.
> > > > > 
> > > > > On bob I _think_ the external pull down is also stuffed.
> > > > > 
> > > > > ...so I guess that would explain why it didn't cause a problem
> > > > > for at
> > > > > least those two boards?
> > > > > 
> > > > > -Doug
> > 
> > 
> > In my opinion it is about these board already being broken by the
> > regulator core change, so nobody noticed the second regression. When
> > the first regression was fixed, it was very hard to correlate the still
> > broken behavior to the second regression.
> > 
> > 
> > I confirm that on Helios64, setting "enable-strobe-pulldown" fixes the
> > EMMC error I had when writing with HS400ES enabled:
> > mmc1: running CQE recovery 
> > mmc1: cqhci: spurious TCN for tag 12
> > 
> > It also took me so long to report upstream as my board code (rk3399-
> > kobol-helios64.dts) is not completely upstreamed yet so I use an
> > Armbian patched kernel.
> > 
> > 
> > 
> > Alban
> > 
> > 
> > 
> > _______________________________________________
> > Linux-rockchip mailing list
> > Linux-rockchip@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 

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

[Folker Schwesinger]

[-- Attachment #1: Type: text/plain, Size: 13197 bytes --]

Hi,

On Tue Feb 27, 2024 at 11:38 AM CET, Christopher Obbard wrote:
> Hi,
>
> On Tue, 2024-02-27 at 10:11 +0000, Folker Schwesinger wrote:
> > Hi,
> > 
> > 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@friendlyarm.com> 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 remember the different reports from the Radxa forums. Personally, I
did test different eMMCs (3 Foresee, 1 Samsung) last year and all
those did not work.
https://lists.infradead.org/pipermail/linux-rockchip/2023-July/039567.html

The enable-strobe-pulldown test above was done using one of the Foresee
modules and with the on-board eMMC of the 4B+.
Unfortunately, I don't have all the eMMCs from last year any more.
However, I could test with another Foresee, in case this is regarded a
valuable data point.

> 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).

I only included the diff to explicitly show what I did for testing. It
was not meant to be a patch for inclusion as the below question still
remains:
 
> > > Could the behavior be reverted to let the vendor kernel default for the
> > > default case (ie enable pulldown)?

If there's consensus on whether to enable the pulldown by default in the
driver or in all the respective DTS, I'd be happy to offer a proper patch.

> > > 
> > > I believe 99% of the boards are now broken with this new internal
> > > pulldown behavior (all  these with internal pulldown). More on that
> > > later but to sum up, nobody  complained because downstream kernels like
> > > Armbian all disabled HS400 for all boards, at least for rk3399.
> > > 
> > > 
> > > Do we really want to ask 99% of the board dts to add this "enable-
> > > strobe-pulldown" in their dts?
> > > Chris, was your custom board not working with the vender kernel default
> > > to enable unconditionaly?
> > > What was the rationale to  choose the opposite default from the vendor
> > > kernel one?
> > > 
> > > 
> > > As told in the commit that introduced this new behavior the default for
> > > the vendor kernel was the opposite of what was introduced in the Linux
> > > kernel:
> > > "
> > > https://github.com/torvalds/linux/commit/8b5c2b45b8f0a11c9072da0f7baf9ee986d3151e
> > > 
> > > commit 8b5c2b45b8f0a11c9072da0f7baf9ee986d3151e
> > > Author: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> > > Date:   Sun Nov 29 13:44:14 2020 +0800
> > > 
> > > phy: rockchip: set pulldown for strobe line in dts
> > > 
> > > This patch add support to set the internal pulldown via dt property
> > > and allow simplify the board design for the trace from emmc-phy to
> > > the eMMC chipset.
> > > Default to not set the pull-down.
> > > 
> > > This patch was inspired from the 4.4 tree of the
> > > Rockchip SDK, where it is enabled unconditional.
> > > The patch had been tested with our rk3399 customized board.
> > > "
> > > 
> > > 
> > > 
> > > For RK3588 I see this commit which makes me believe the internal
> > > pulldown case is the most common "
> > > commit 37f3d6108730713c411827ab4af764909f4dfc78
> > > Author: Sam Edwards <cfsworks@gmail.com>
> > > Date:   Tue Dec 5 12:29:00 2023 -0800
> > > 
> > > 
> > > arm64: dts: rockchip: Fix eMMC Data Strobe PD on rk3588
> > > 
> > > JEDEC standard JESD84-B51 defines the eMMC Data Strobe line, which is
> > > currently used only in HS400 mode, as a device->host clock signal that
> > > "is used only in read operation. The Data Strobe is always High-Z (not
> > > driven by the device and pulled down by RDS) or Driven Low in write
> > > operation, except during CRC status response." RDS is a pull-down
> > > resistor specified in the 10K-100K ohm range. Thus per the standard,
> > > the
> > > Data Strobe is always pulled to ground (by the eMMC and/or RDS) during
> > > write operations.
> > > 
> > > Evidently, the eMMC host controller in the RK3588 considers an active
> > > voltage on the eMMC-DS line during a write to be an error.
> > > 
> > > The default (i.e. hardware reset, and Rockchip BSP) behavior for the
> > > RK3588 is to activate the eMMC-DS pin's builtin pull-down. As a result,
> > > many RK3588 board designers do not bother adding a dedicated RDS
> > > resistor, instead relying on the RK3588's internal bias. The current
> > > devicetree, however, disables this bias (`pcfg_pull_none`), breaking
> > > HS400-mode writes for boards without a dedicated RDS, but with an eMMC
> > > chip that chooses to High-Z (instead of drive-low) the eMMC-DS line.
> > > (The Turing RK1 is one such board.)
> > > 
> > > Fix this by changing the bias in the (common) emmc_data_strobe case to
> > > reflect the expected hardware/BSP behavior. This is unlikely to cause
> > > regressions elsewhere: the pull-down is only relevant for High-Z eMMCs,
> > > and if this is redundant with a (dedicated) RDS resistor, the effective
> > > result is only a lower resistance to ground -- where the range of
> > > tolerance is quite high. If it does, it's better fixed in the specific
> > > devicetrees.
> > > "
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > Lately two other upstream dts disabled HS400 due to this new behavior I
> > > believe:
> > > "
> > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/arm64/boot/dts/rockchip?id=2bd1d2dd808c60532283e9cf05110bf1bf2f9079
> > > author	Christopher Obbard <chris.obbard@collabora.com>	2023-
> > > 07-05 15:42:55 +0100
> > > committer	Heiko Stuebner <heiko@sntech.de>	2023-07-10
> > > 15:43:24 +0200
> > > commit	2bd1d2dd808c60532283e9cf05110bf1bf2f9079 (patch)
> > > tree	57cbf7eaa91deb68f143577d5d1dbc0d9141480e
> > > /arch/arm64/boot/dts/rockchip
> > > parent	cee572756aa2cb46e959e9797ad4b730b78a050b (diff)
> > > download	linux-2bd1d2dd808c60532283e9cf05110bf1bf2f9079.tar.gz
> > > arm64: dts: rockchip: Disable HS400 for eMMC on ROCK 4C+
> > > 
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/arm64/boot/dts/rockchip?id=cee572756aa2cb46e959e9797ad4b730b78a050b
> > > author	Christopher Obbard <chris.obbard@collabora.com>	2023-
> > > 07-05 15:42:54 +0100
> > > committer	Heiko Stuebner <heiko@sntech.de>	2023-07-10
> > > 15:43:24 +0200
> > > commit	cee572756aa2cb46e959e9797ad4b730b78a050b (patch)
> > > tree	cf3ed8ff6230cbde04353503417c1a75ba15c249
> > > /arch/arm64/boot/dts/rockchip
> > > parent	5ce6971e5279c569defc2f2ac800692049bbaa90 (diff)
> > > download	linux-cee572756aa2cb46e959e9797ad4b730b78a050b.tar.gz
> > > arm64: dts: rockchip: Disable HS400 for eMMC on ROCK Pi 4
> > > "
> > > 
> > > 
> > > All Armbian RK3399 boards, as far as I know, had to disable HS400, I
> > > also believe due to this commit.
> > > 
> > > You can also search google for "running cqe recovery rk3399 armbian".
> > > 
> > > 
> > > This was never reported upstream though. But as HS400 is disabled
> > > everywhere nobody notice the regression nowadays.
> > > 
> > > 
> > > > 
> > > > > BR,
> > > > > Jensen
> > > > > 
> > > > > On Tue, Aug 23, 2022 at 10:13 PM Doug Anderson
> > > > > <dianders@chromium.org> wrote:
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > On Tue, Aug 23, 2022 at 4:53 AM Heiko Stübner <heiko@sntech.de>
> > > > > > wrote:
> > > > > > > 
> > > > > > > Am Montag, 22. August 2022, 09:41:39 CEST schrieb Jensen Huang:
> > > > > > > > Internal pull-down for strobe line (GRF_EMMCPHY_CON2[9]) was
> > > > > > > > disabled
> > > > > > > > by commit 8b5c2b45b8f0, which causes I/O error in HS400 mode.
> > > > > > > > 
> > > > > > > > Tested on NanoPC-T4.
> > > > > > > > 
> > > > > > > > Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe
> > > > > > > > line in dts")
> > > > > > > > Signed-off-by: Jensen Huang <jensenhuang@friendlyarm.com>
> > > > > > > 
> > > > > > > ok, so this looks like it restores previous functionality.
> > > > > > > 
> > > > > > > I'm just wondering as the "offending" patch is from 2020, why
> > > > > > > this
> > > > > > > only turns up now. Any ideas?
> > > > > > 
> > > 
> > > Probbaly because the introduction of PROBE_DEFERRED in regulator core
> > > before that (in 5.10.60) already broke at least my board HS400 due to
> > > double init. Thus why it took me so long to see this new pulldown
> > > behavior commit. I was testing every regulator core double init fixup
> > > patchset while not understanding why reverting the PROBE_DEFERRED
> > > commit on 5.10.60 worked but not on newer kernels (ie this new pulldown
> > > behavior was introduced in 5.11...).
> > > 
> > > 
> > > 
> > > 
> > > > > > Ah, I see. So before the offending patch we used to just leave
> > > > > > the
> > > > > > pull state at whatever the default was when the kernel was
> > > > > > booted.
> > > > > > After the offending patch we chose a default.
> > > > > > 
> > > > > > On kevin I see an external pull down on this line. Enabling both
> > > > > > the
> > > > > > internal and external is probably not a huge deal, it'll just
> > > > > > affect
> > > > > > the strength of the pull.
> > > > > > 
> > > > > > On bob I _think_ the external pull down is also stuffed.
> > > > > > 
> > > > > > ...so I guess that would explain why it didn't cause a problem
> > > > > > for at
> > > > > > least those two boards?
> > > > > > 
> > > > > > -Doug
> > > 
> > > 
> > > In my opinion it is about these board already being broken by the
> > > regulator core change, so nobody noticed the second regression. When
> > > the first regression was fixed, it was very hard to correlate the still
> > > broken behavior to the second regression.
> > > 
> > > 
> > > I confirm that on Helios64, setting "enable-strobe-pulldown" fixes the
> > > EMMC error I had when writing with HS400ES enabled:
> > > mmc1: running CQE recovery 
> > > mmc1: cqhci: spurious TCN for tag 12
> > > 
> > > It also took me so long to report upstream as my board code (rk3399-
> > > kobol-helios64.dts) is not completely upstreamed yet so I use an
> > > Armbian patched kernel.
> > > 
> > > 
> > > 
> > > Alban
> > > 
> > > 
> > > 
> > > _______________________________________________
> > > Linux-rockchip mailing list
> > > Linux-rockchip@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-rockchip
> > 
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

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@friendlyarm.com> 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.