[PATCH v2 1/3] rockchip: rk35xx: Remove use of eMMC DDR52 mode

[Quentin Schulz]

Hi Dragan,

On 2/5/24 14:40, Dragan Simic wrote:
> [You don't often get email from dsimic at manjaro.org. Learn why this is 
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On 2024-02-05 14:24, Jonas Karlman wrote:
>> On 2024-02-05 10:40, Quentin Schulz wrote:
>>> On 2/4/24 21:53, Jonas Karlman wrote:
>>>> Testing has shown that writing to eMMC using DDR52 mode does not seem
>>>> to
>>>> work on RK356x and RK3588 boards.
>>>>
>>>> A simple test of writing a single block to e.g. sector 0x4000 fails:
>>>>
>>>>    # Rescan using DDR52 mode
>>>>    => mmc rescan 4
>>>>
>>>>    # Write a single block to sector 0x4000 fails with ERROR
>>>>    => mmc write 20000000 4000 1
>>>>
>>>> With the MMC_SPEED_MODE_SET Kconfig option enabled.
>>>>
>>>> Fix this by removing the mmc-ddr-1_8v prop from sdhci nodes in
>>>> affected
>>>> board u-boot.dtsi files.
>>>>
>>>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>>>> ---
>>>> Changes in v2:
>>>> - Update commit message
>>>>
>>>> Link to v1: 
>>>> https://patchwork.ozlabs.org/patch/1891695/
>>>> ---
>>>>   arch/arm/dts/rk3566-quartz64-a-u-boot.dtsi   | 1 -
>>>>   arch/arm/dts/rk3566-quartz64-b-u-boot.dtsi   | 1 -
>>>>   arch/arm/dts/rk3566-radxa-cm3-io-u-boot.dtsi | 1 -
>>>>   arch/arm/dts/rk3566-soquartz-u-boot.dtsi     | 1 -
>>>>   arch/arm/dts/rk3568-lubancat-2-u-boot.dtsi   | 1 -
>>>>   arch/arm/dts/rk3568-nanopi-r5s-u-boot.dtsi   | 1 -
>>>>   arch/arm/dts/rk3568-odroid-m1-u-boot.dtsi    | 1 -
>>>>   arch/arm/dts/rk3568-radxa-e25-u-boot.dtsi    | 1 -
>>>>   arch/arm/dts/rk3568-rock-3a-u-boot.dtsi      | 1 -
>>>>   arch/arm/dts/rk3588-rock-5b-u-boot.dtsi      | 1 -
>>>>   arch/arm/dts/rk3588-turing-rk1-u-boot.dtsi   | 1 -
>>>>   arch/arm/dts/rk3588s-rock-5a-u-boot.dtsi     | 1 -
>>>>   12 files changed, 12 deletions(-)
>>>>
>>>> diff --git a/arch/arm/dts/rk3566-quartz64-a-u-boot.dtsi
>>>> b/arch/arm/dts/rk3566-quartz64-a-u-boot.dtsi
>>>> index 11976fd3a6e0..930d660868bb 100644
>>>> --- a/arch/arm/dts/rk3566-quartz64-a-u-boot.dtsi
>>>> +++ b/arch/arm/dts/rk3566-quartz64-a-u-boot.dtsi
>>>> @@ -8,7 +8,6 @@
>>>>
>>>>   &sdhci {
>>>>     cap-mmc-highspeed;
>>>
>>> Shouldn't we also remove cap-mmc-highspeed?
>>>
>>> I assume this is for MMC_HS? Which we discovered doesn't work
>>> reliably?
>>
>> Writing in lower speeds on RK356x does work better then on RK3588,
>> however HS200 still seem to be more reliable overall and give more
>> speed.
>>
>> For RK3588 they could possible be removed, but since reading does work
>> and similar issue also existed in the fallback MMC legacy mode, I did
>> not see that cleanup as important, or maybe more correctly it did not
>> cross my mind to also remove these props :-), the fully broken ddr52
>> felt more important.
>>
>>>
>>> Since it could be in a different patch if you wanted to, for the
>>> changes
>>> in this patch:
>>
>> Agree, a possible cleanup of cap-mmc-highspeed props could be done in a
>> separate patch.
>>
>> Alternatively any missing and appropriate modes currently in
>> u-boot.dtsi
>> should be added to linux DT and they can then be synced back to U-Boot
>> and any override/addition dropped from u-boot.dtsi files.
> 
> How about waiting until I fix the mode selection in the Linux kernel
> drivers?  As I noted already, that's already on my TODO list, and
> perhaps it would be good to test a bit again under Linux with those
> fixes applied, before actually removing the buggy modes from the
> kernel's RK356x and RK3588(s) SoC dtsi files.
> 

While I appreciate there's some work that can be done in the Linux 
kernel for those low-speed modes, the properties in question are removed 
only from U-Boot's DTS and your future patches for the kernel wouldn't 
impact the bootloader anyway.

The issue right now is that U-Boot is currently broken because of those 
properties, which are only in U-Boot's DTS.

Not sure exactly why we should waiting on Linux kernel patches for 
merging work-arounds for U-Boot? What exactly are you suggesting we wait 
for? What can improve the situation?

Cheers,
Quentin