[PATCH] mmc: rockchip_sdhci: Set xx_TAP_VALUE for RK3528

[Quentin Schulz]

Hi Jonas,

On 7/14/25 6:36 PM, Jonas Karlman wrote:
> Hi Quentin,
> 
> On 7/14/2025 5:38 PM, Quentin Schulz wrote:
>> Hi Jonas,
>>
>> On 7/10/25 6:21 PM, Jonas Karlman wrote:
>>> eMMC erase and write support on RK3528 is somewhat unreliable, sometime
>>> e.g. mmc erase and write commands will fail with an error.
>>>
>>> Use the delay line lock value for half card clock cycle, DLL_LOCK_VALUE,
>>> to set a manual xx_TAP_VALUE to fix the unreliable eMMC support.
>>>
>>> This is only enabled for RK3528, remaining SoCs still use the automatic
>>> tap value, (DLL_LOCK_VALUE * 2) % 256, same value we configure manually
>>> for RK3528.
>>>

Ah, I read that too quickly!

We are just trying to make the RK3528 follow the exact same formula as 
used on other SoCs but for some reason the default isn't good enough for 
RK3528 (buggy silicon?).

>>> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
>>> ---
>>>    drivers/mmc/rockchip_sdhci.c | 28 +++++++++++++++++++++++-----
>>>    1 file changed, 23 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
>>> index 761e3619329c..f15ea7c9c6b7 100644
>>> --- a/drivers/mmc/rockchip_sdhci.c
>>> +++ b/drivers/mmc/rockchip_sdhci.c
>>> @@ -86,13 +86,19 @@
>>>    #define DLL_CMDOUT_SRC_CLK_NEG		BIT(28)
>>>    #define DLL_CMDOUT_EN_SRC_CLK_NEG	BIT(29)
>>>    #define DLL_CMDOUT_BOTH_CLK_EDGE	BIT(30)
>>> +#define DLL_TAP_VALUE_SEL		BIT(25)
>>
>> Maybe match the name for the other tap bitfields?
> 
> I went with a single define because TXCLK, RXCLK, STRBIN and CMDOUT all
> share similar fields, the xx_TAP_VALUE_SEL field is at bit 25 in all of
> the EMMC_DLL_xx regs.
> 
> Not really sure why we have multiple defines for the other fields when
> these regs mostly share similar fields.
> 
>>
>> We have DLL_TXCLK_TAPNUM_FROM_SW for example, so something like
>>
>> DLL_TAPVALUE_FROM_SW
>>
>> ?
> 
> I was switching between the field name and the function name during
> testing, and you are correct it should be the function name not the
> field name. Will change in a v2.
> 
>>
>>> +#define DLL_TAP_VALUE_OFFSET		8
>>> +#define DLL_LOCK_VALUE_MASK		0xFF
>>>    
>>> +#define DLL_LOCK_VALUE(x) \
>>> +	((((x) & DLL_LOCK_VALUE_MASK) * 2) & DLL_LOCK_VALUE_MASK)
>>
>> This seems very odd, why do we need to binary AND before and after
>> multiplying by 2? Trying to avoid an overflow somehow?
>> Also, where did you get you need to multiply by 2? The RK3588 TRM
>> doesn't seem to be hinting at that being required?
> 
>  From the field definition for the different fields:
> 
> """
> xx_TAP_VALUE_SEL[25]
> Tap value selection for receive clock/transmit clock/strobe input/command output.
> 1'b0: Tap value equals to (DLL_LOCK_VALUE*2)%256
> 1'b1: Tap value comes from software (xx_TAP_VALUE)
> 
> xx_TAP_VALUE[15:8]
> Tap value for receive clock/transmit clock/strobe input/command output.
> It denotes delay element number for receive clock/transmit clock/strobe input/command output.
> 
> DLL_LOCK[8]
> Delay line lock indication.
> 1'b0: Delay line is not locked
> 1'b1: Delay line is locked
> 
> DLL_LOCK_VALUE[7:0]
> Delay line lock value for half card clock cycle.
> It denotes the delay element number needed for delay line locked. It is valid when DLL_LOCK is high.
> """
> 
> And from 4.6.1 Clock Adjustment of RK3588 TRM:
> 
> """
> After phase detection, the clock’s precision is shown in EMMC_DLL_STATUS0[7:0] (DLL_LOCK_VALUE) based on delay element.
> We can calculate the delay element numbers requirement for transmit clock (TXCLK), receive clock (RXCLK), data strobe input (STRBIN), command output (CMDOUT), denoted as “XX” as follows:
> (1) Get tap value: if XX_TAP_VALUE_SEL=0, tap value equals to (DLL_LOCK_VALUE*2)%256; else tap value is set by software, XX_TAP_VALUE;
> (2) Get tap number: if XX_TAP_NUM_SEL=0, tap number comes from Host Controller; else tap number is set by software, XX_TAP_NUM;
> (3) Get required delay element numbers: if XX_DELAY_NUM_SEL=0, delay element number equals to tap value* tap number; else delay element number is set by software, XX_DELAY_NUM.
> """
> 
> The DLL_LOCK_VALUE at EMMC_DLL_STATUS0[7:0] is the delay line lock value
> for half card clock cycle, to get a value that can be set in
> xx_TAP_VALUE we must double it for full clock cycle.
> 

I misread the commit log and expoected that the default 
(DLL_LOCK_VALUE*2)%256) wasn't appropriate for RK3528 but it's rather 
that RK3528 doesn't use the default (DLL_LOCK_VALUE*2)%256 so we're just 
trying to replicate it so it works as for other SoCs?

I assume this only needs to be done during the configuration of the card 
and cannot vary after it? My worry being that using xx_TAP_VALUE_SEL=0 
is likely changing the tap value directly on the silicon level so if 
DLL_LOCK_VALUE changes, the actual tap value does as well, without human 
intervention. If we go for manual, then if this DLL_LOCK_VALUE changes, 
we need to update the manual xx_TAP_VALUE bitfield whenever it does. But 
I assume this is stable once the MMC speed mode and whatnot is selected 
and the clocks are stable?

Looks reasonable to me otherwise!

> Something like this is also what vendor kernel and u-boot does for all
> SoCs, at least for linux-6.1-stan-rkr5.
> 

I don't see patches for upstream Linux though, did I miss them or should 
we have some as well there?

>>
>> I believe you could also simply use
>>
>> #define DLL_LOCK_VALUE(x) FIELD_PREP((x) * 2, 0xff00)
>>
>> and avoid having to carry the mask or the offset of the bitfield in the
>> code (not tested though).
> 
> Something like following could possible work:
> 
> #define DLL_TAPVALUE_FROM_SW	BIT(25)
> #define DLL_LOCK_VALUE_FIELD	GENMASK(7, 0)
> #define DLL_LOCK_VALUE(x)	FIELD_GET(DLL_LOCK_VALUE_FIELD, (x))
> #define DLL_TAP_VALUE_FIELD	GENMASK(15, 8)
> #define DLL_TAP_VALUE(x)	FIELD_PREP(DLL_TAP_VALUE_FIELD, DLL_LOCK_VALUE((x)) * 2)
> 
> dll_tap_value = DLL_TAPVALUE_FROM_SW | DLL_TAP_VALUE(val);
> 
> If that makes it any more clear what is happening?
> 

I would actually not hide the * 2 in there, also because it could be 
actually configurable if we wanted to.

Something like

#define DLL_TAPVALUE_FROM_SW	BIT(25)
#define DLL_LOCK_VALUE_FIELD	GENMASK(7, 0)
#define DLL_LOCK_VALUE(x)	FIELD_GET(DLL_LOCK_VALUE_FIELD, (x))
#define DLL_TAP_VALUE_FIELD	GENMASK(15, 8)
#define DLL_TAP_VALUE(x)	FIELD_PREP(DLL_TAP_VALUE_FIELD, (x))

dll_tap_value = DLL_TAPVALUE_FROM_SW | DLL_TAP_VALUE(DLL_LOCK_VALUE(val) 
* 2);

instead maybe? With a comment just above stating we're replicating 4.6.1 
Clock Adjustment as if it was automatic but manually due to some bug for 
RK3528.

What do you think?

Cheers,
Quentin