diff mbox series

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

Message ID 20240327192641.14220-2-dev@folker-schwesinger.de (mailing list archive)
State New
Headers show
Series Enable internal strobe pulldown on Radxa ROCK 4 SBCs | expand

Commit Message

Folker Schwesinger March 27, 2024, 7:26 p.m. UTC
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(-)

Comments

Chen-Yu Tsai March 28, 2024, 5:39 a.m. UTC | #1
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
Folker Schwesinger March 28, 2024, 7:08 a.m. UTC | #2
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
Chen-Yu Tsai March 28, 2024, 7:44 a.m. UTC | #3
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
Dragan Simic March 31, 2024, 7:14 p.m. UTC | #4
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";
>  };
diff mbox series

Patch

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";
 };