Enable internal strobe pulldown on Radxa ROCK 4 SBCs

[PATCH 1/2] arm64: dts: rockchip: Add enable-strobe-pulldown to emmc phy on ROCK Pi 4

Posted by Folker Schwesinger 1 year, 10 months ago

Commit 8b5c2b45b8f0 disabled the internal pull-down for the strobe line
causing I/O errors in HS400 mode for various eMMC modules.

Enable the internal strobe pull-down for ROCK Pi 4 boards. Also re-enable
HS400 mode, that was replaced with HS200 mode as a workaround for the
stability issues in:
cee572756aa2 ("arm64: dts: rockchip: Disable HS400 for eMMC on ROCK Pi 4").

This was tested on ROCK 4SE and ROCK Pi 4B+.

Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
---
 arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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 281a12180703..b9d6284bb804 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi
@@ -194,6 +194,7 @@ &cpu_b1 {
 };
 
 &emmc_phy {
+	rockchip,enable-strobe-pulldown;
 	status = "okay";
 };
 
@@ -648,7 +649,8 @@ &saradc {
 &sdhci {
 	max-frequency = <150000000>;
 	bus-width = <8>;
-	mmc-hs200-1_8v;
+	mmc-hs400-1_8v;
+	mmc-hs400-enhanced-strobe;
 	non-removable;
 	status = "okay";
 };
-- 
2.44.0

Re: [PATCH 1/2] arm64: dts: rockchip: Add enable-strobe-pulldown to emmc phy on ROCK Pi 4

Posted by Dragan Simic 1 year, 10 months ago

On 2024-03-27 20:26, Folker Schwesinger wrote:
> Commit 8b5c2b45b8f0 disabled the internal pull-down for the strobe line
> causing I/O errors in HS400 mode for various eMMC modules.
> 
> Enable the internal strobe pull-down for ROCK Pi 4 boards. Also 
> re-enable
> HS400 mode, that was replaced with HS200 mode as a workaround for the
> stability issues in:
> cee572756aa2 ("arm64: dts: rockchip: Disable HS400 for eMMC on ROCK Pi 
> 4").
> 
> This was tested on ROCK 4SE and ROCK Pi 4B+.
> 
> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in 
> dts")
> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>

Looking good to me.

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

> ---
>  arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> 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 281a12180703..b9d6284bb804 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi
> @@ -194,6 +194,7 @@ &cpu_b1 {
>  };
> 
>  &emmc_phy {
> +	rockchip,enable-strobe-pulldown;
>  	status = "okay";
>  };
> 
> @@ -648,7 +649,8 @@ &saradc {
>  &sdhci {
>  	max-frequency = <150000000>;
>  	bus-width = <8>;
> -	mmc-hs200-1_8v;
> +	mmc-hs400-1_8v;
> +	mmc-hs400-enhanced-strobe;
>  	non-removable;
>  	status = "okay";
>  };

Re: [PATCH 1/2] arm64: dts: rockchip: Add enable-strobe-pulldown to emmc phy on ROCK Pi 4

Posted by Chen-Yu Tsai 1 year, 10 months ago

On Thu, Mar 28, 2024 at 4:33 AM Folker Schwesinger
<dev@folker-schwesinger.de> wrote:
>
> Commit 8b5c2b45b8f0 disabled the internal pull-down for the strobe line
> causing I/O errors in HS400 mode for various eMMC modules.
>
> Enable the internal strobe pull-down for ROCK Pi 4 boards. Also re-enable
> HS400 mode, that was replaced with HS200 mode as a workaround for the
> stability issues in:
> cee572756aa2 ("arm64: dts: rockchip: Disable HS400 for eMMC on ROCK Pi 4").
>
> This was tested on ROCK 4SE and ROCK Pi 4B+.
>
> Fixes: 8b5c2b45b8f0 ("phy: rockchip: set pulldown for strobe line in dts")
> Signed-off-by: Folker Schwesinger <dev@folker-schwesinger.de>
> ---
>  arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> 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 281a12180703..b9d6284bb804 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-rock-pi-4.dtsi
> @@ -194,6 +194,7 @@ &cpu_b1 {
>  };
>
>  &emmc_phy {
> +       rockchip,enable-strobe-pulldown;
>         status = "okay";
>  };
>
> @@ -648,7 +649,8 @@ &saradc {
>  &sdhci {
>         max-frequency = <150000000>;
>         bus-width = <8>;
> -       mmc-hs200-1_8v;

Shouldn't this be kept around? The node should list all supported modes,
not just the highest one. Same for the other patch.

ChenYu

> +       mmc-hs400-1_8v;
> +       mmc-hs400-enhanced-strobe;
>         non-removable;
>         status = "okay";
>  };
> --
> 2.44.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] arm64: dts: rockchip: Add enable-strobe-pulldown to emmc phy on ROCK Pi 4

Posted by Folker Schwesinger 1 year, 10 months ago

On Thu Mar 28, 2024 at 6:39 AM CET, Chen-Yu Tsai wrote:
> > @@ -648,7 +649,8 @@ &saradc {
> >  &sdhci {
> >         max-frequency = <150000000>;
> >         bus-width = <8>;
> > -       mmc-hs200-1_8v;
>
> Shouldn't this be kept around? The node should list all supported modes,
> not just the highest one. Same for the other patch.
>

This is not necessary, mmc-hs400-1_8v implicitly activates the
corresponding HS200 capability, see drivers/mmc/core/host.c:

if (device_property_read_bool(dev, "mmc-hs200-1_8v"))
	host->caps2 |= MMC_CAP2_HS200_1_8V_SDR;
/* ... */
if (device_property_read_bool(dev, "mmc-hs400-1_8v"))
	host->caps2 |= MMC_CAP2_HS400_1_8V | MMC_CAP2_HS200_1_8V_SDR;

Kind regards

Folker

Re: [PATCH 1/2] arm64: dts: rockchip: Add enable-strobe-pulldown to emmc phy on ROCK Pi 4

Posted by Chen-Yu Tsai 1 year, 10 months ago

On Thu, Mar 28, 2024 at 3:09 PM Folker Schwesinger
<dev@folker-schwesinger.de> wrote:
>
> On Thu Mar 28, 2024 at 6:39 AM CET, Chen-Yu Tsai wrote:
> > > @@ -648,7 +649,8 @@ &saradc {
> > >  &sdhci {
> > >         max-frequency  <150000000>;
> > >         bus-width  <8>;
> > > -       mmc-hs200-1_8v;
> >
> > Shouldn't this be kept around? The node should list all supported modes,
> > not just the highest one. Same for the other patch.
> >
>
> This is not necessary, mmc-hs400-1_8v implicitly activates the
> corresponding HS200 capability, see drivers/mmc/core/host.c:
>
> if (device_property_read_bool(dev, "mmc-hs200-1_8v"))
>         host->caps2 | MMC_CAP2_HS200_1_8V_SDR;
> /* ... */
> if (device_property_read_bool(dev, "mmc-hs400-1_8v"))
>         host->caps2 | MMC_CAP2_HS400_1_8V | MMC_CAP2_HS200_1_8V_SDR;

Looking at the initial commit for adding the hs400 properties,
it was also mentioned that hs400 support implies hs200 support.
It just wasn't explicitly spelled out in the bindings.

In that case, I think we're good here for this particular case.

> Kind regards
>
> Folker
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel