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

[Shawn Lin]

在 2024/2/27 10:05, 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 at 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 believe 99% of the boards are now broken with this new internal
> pulldown behavior (all  these with internal pulldown). More on that

+1

Just find RK3399 EVB to be broken when moving to new kernel two month 
ago. And I believe it will break more custom boards which follow the
vendor design.

> 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?

It's recommended to use internal pulldown, so the default register value
already reflects the fact. It will let the board running HS400 all the
way from loader to kernel. If external pulldown is stuffed for certain
case, what we actually need is disable-strobe-pulldown, isn't it?

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