Message-Id: <CZFS45VOLIKW.2VS3M3VOMBT8V@folker-schwesinger.de>
Date: Tue, 27 Feb 2024 10:11:48 +0000
From: "Folker Schwesinger" <dev@...ker-schwesinger.de>
To: "Alban Browaeys" <alban.browaeys@...il.com>, "Doug Anderson"
 <dianders@...omium.org>, "Jensen Huang" <jensenhuang@...endlyarm.com>
Cc: Heiko Stübner <heiko@...ech.de>, "Rob Herring"
 <robh+dt@...nel.org>, "Krzysztof Kozlowski"
 <krzysztof.kozlowski+dt@...aro.org>, "Vinod Koul" <vkoul@...nel.org>,
 "Chris Ruehl" <chris.ruehl@...ys.com.hk>, "Brian Norris"
 <briannorris@...omium.org>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE
 TREE BINDINGS" <devicetree@...r.kernel.org>, "Linux ARM"
 <linux-arm-kernel@...ts.infradead.org>, "open list:ARM/Rockchip SoC..."
 <linux-rockchip@...ts.infradead.org>, "LKML"
 <linux-kernel@...r.kernel.org>, "Kishon Vijay Abraham I"
 <kishon@...nel.org>, "Christopher Obbard" <chris.obbard@...labora.com>
Subject: Re: [PATCH] arm64: dts: rockchip: add enable-strobe-pulldown to
 emmc phy on rk3399

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@...endlyarm.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@...ys.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@...il.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@...labora.com>	2023-
> 07-05 15:42:55 +0100
> committer	Heiko Stuebner <heiko@...ech.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@...labora.com>	2023-
> 07-05 15:42:54 +0100
> committer	Heiko Stuebner <heiko@...ech.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@...omium.org> wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > On Tue, Aug 23, 2022 at 4:53 AM Heiko Stübner <heiko@...ech.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@...endlyarm.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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip


Download attachment "signature.asc" of type "application/pgp-signature" (266 bytes)

Powered by blists - more mailing lists