Re: [PATCH v2 23/23] rockchip: rk3588: Enable use of eMMC HS200 mode on a few missed boards

[Dragan Simic <dsimic@manjaro.org>]

On 2024-04-15 10:23, Jonas Karlman wrote:
> On 2024-04-15 09:00, Dragan Simic wrote:
>> Please see my comments below.
>> 
>> On 2024-04-13 20:13, Jonas Karlman wrote:
>>> A few RK3588 boards are missing a mmc-hs200 prop to signal that HS200
>>> mode is supported in addition to HS400, this lead to bad performance
>>> reading FIT in SPL and broken MMC write.
>>> 
>>> Add cap-mmc-highspeed and mmc-hs200-1_8v to affected boards to enable
>>> use of eMMC HS200 mode.
>>> 
>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> 
>> Shouldn't it be better to follow the Linux kernel's logic in
>> the MMC driver, and enable the HS200 mode when the DT indicates
>> that the HS400 mode is suported?  In other words, exactly what
>> you already submitted to the mailing list. [1]
> 
> Yes, this is somewhat what I tried to make aware of with the note and
> link under "v2: New patch" :-)

Wow, I totally missed that paragraph. :)  Though, that also means
that we're on the same page. :)

>> That would allow a significant cleanup of the U-Boot dtsi files
>> to be performed.
>> 
>> Until that's resolved, the patch is looking good to me.
> 
> Fully agree, this will fix the bad performance in SPL and fix MMC write
> for existing boards making them work same as other RK3588 boards until
> the MMC maintainer can weigh in on the linked patch.

That will be nice.

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

Thank you for all these nice cleanup patches!

>> [1] 
>> https://lore.kernel.org/u-boot/20240408210619.3749231-1-jonas@kwiboo.se/
>> 
>>> ---
>>> v2: New patch
>>> 
>>> All these mmc-hs200 prop overrides may not be needed if the patch 
>>> "mmc:
>>> Imply HS200 cap with mmc-hs400 prop to match linux" [1] gets applied,
>>> until such time this matches what has been done for all other RK35xx
>>> boards.
>>> 
>>> [1] https://patchwork.ozlabs.org/patch/1921039/
>>> ---
>>>  arch/arm/dts/rk3588-edgeble-neu6a-io-u-boot.dtsi | 5 +++++
>>>  arch/arm/dts/rk3588-edgeble-neu6b-io-u-boot.dtsi | 5 +++++
>>>  arch/arm/dts/rk3588-evb1-v10-u-boot.dtsi         | 5 +++++
>>>  arch/arm/dts/rk3588-nanopc-t6-u-boot.dtsi        | 5 +++++
>>>  arch/arm/dts/rk3588-quartzpro64-u-boot.dtsi      | 5 +++++
>>>  arch/arm/dts/rk3588-toybrick-x0-u-boot.dtsi      | 5 +++++
>>>  6 files changed, 30 insertions(+)
>>> 
>>> diff --git a/arch/arm/dts/rk3588-edgeble-neu6a-io-u-boot.dtsi
>>> b/arch/arm/dts/rk3588-edgeble-neu6a-io-u-boot.dtsi
>>> index dd0058262b7f..fc4b13628b30 100644
>>> --- a/arch/arm/dts/rk3588-edgeble-neu6a-io-u-boot.dtsi
>>> +++ b/arch/arm/dts/rk3588-edgeble-neu6a-io-u-boot.dtsi
>>> @@ -15,6 +15,11 @@
>>>  	};
>>>  };
>>> 
>>> +&sdhci {
>>> +	cap-mmc-highspeed;
>>> +	mmc-hs200-1_8v;
>>> +};
>>> +
>>>  &sdmmc {
>>>  	bus-width = <4>;
>>>  	status = "okay";
>>> diff --git a/arch/arm/dts/rk3588-edgeble-neu6b-io-u-boot.dtsi
>>> b/arch/arm/dts/rk3588-edgeble-neu6b-io-u-boot.dtsi
>>> index a45b3f5e86ad..a0c62072c34e 100644
>>> --- a/arch/arm/dts/rk3588-edgeble-neu6b-io-u-boot.dtsi
>>> +++ b/arch/arm/dts/rk3588-edgeble-neu6b-io-u-boot.dtsi
>>> @@ -14,3 +14,8 @@
>>>  		u-boot,spl-boot-order = &sdmmc;
>>>  	};
>>>  };
>>> +
>>> +&sdhci {
>>> +	cap-mmc-highspeed;
>>> +	mmc-hs200-1_8v;
>>> +};
>>> diff --git a/arch/arm/dts/rk3588-evb1-v10-u-boot.dtsi
>>> b/arch/arm/dts/rk3588-evb1-v10-u-boot.dtsi
>>> index e8566785e965..354e4a8d39e3 100644
>>> --- a/arch/arm/dts/rk3588-evb1-v10-u-boot.dtsi
>>> +++ b/arch/arm/dts/rk3588-evb1-v10-u-boot.dtsi
>>> @@ -10,3 +10,8 @@
>>>  		u-boot,spl-boot-order = "same-as-spl", &sdhci;
>>>  	};
>>>  };
>>> +
>>> +&sdhci {
>>> +	cap-mmc-highspeed;
>>> +	mmc-hs200-1_8v;
>>> +};
>>> diff --git a/arch/arm/dts/rk3588-nanopc-t6-u-boot.dtsi
>>> b/arch/arm/dts/rk3588-nanopc-t6-u-boot.dtsi
>>> index 968385622fa5..7948464ac85f 100644
>>> --- a/arch/arm/dts/rk3588-nanopc-t6-u-boot.dtsi
>>> +++ b/arch/arm/dts/rk3588-nanopc-t6-u-boot.dtsi
>>> @@ -11,6 +11,11 @@
>>>  	bootph-some-ram;
>>>  };
>>> 
>>> +&sdhci {
>>> +	cap-mmc-highspeed;
>>> +	mmc-hs200-1_8v;
>>> +};
>>> +
>>>  &sfc {
>>>  	pinctrl-names = "default";
>>>  	pinctrl-0 = <&fspim1_pins>;
>>> diff --git a/arch/arm/dts/rk3588-quartzpro64-u-boot.dtsi
>>> b/arch/arm/dts/rk3588-quartzpro64-u-boot.dtsi
>>> index 7b937943a53c..d764135ab661 100644
>>> --- a/arch/arm/dts/rk3588-quartzpro64-u-boot.dtsi
>>> +++ b/arch/arm/dts/rk3588-quartzpro64-u-boot.dtsi
>>> @@ -4,3 +4,8 @@
>>>   */
>>> 
>>>  #include "rk3588-u-boot.dtsi"
>>> +
>>> +&sdhci {
>>> +	cap-mmc-highspeed;
>>> +	mmc-hs200-1_8v;
>>> +};
>>> diff --git a/arch/arm/dts/rk3588-toybrick-x0-u-boot.dtsi
>>> b/arch/arm/dts/rk3588-toybrick-x0-u-boot.dtsi
>>> index 1aeb5410e43e..c5c94b421d1e 100644
>>> --- a/arch/arm/dts/rk3588-toybrick-x0-u-boot.dtsi
>>> +++ b/arch/arm/dts/rk3588-toybrick-x0-u-boot.dtsi
>>> @@ -10,3 +10,8 @@
>>>  		u-boot,spl-boot-order = "same-as-spl", &sdhci;
>>>  	};
>>>  };
>>> +
>>> +&sdhci {
>>> +	cap-mmc-highspeed;
>>> +	mmc-hs200-1_8v;
>>> +};