[PATCH] mmc: rockchip_sdhci: Set xx_TAP_VALUE for RK3528
Jonas Karlman
jonas at kwiboo.se
Mon Jul 14 18:36:10 CEST 2025
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.
>>
>> 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.
Something like this is also what vendor kernel and u-boot does for all
SoCs, at least for linux-6.1-stan-rkr5.
>
> 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?
Regards,
Jonas
>
>> #define DLL_LOCK_WO_TMOUT(x) \
>> ((((x) & DWCMSHC_EMMC_DLL_LOCKED) == DWCMSHC_EMMC_DLL_LOCKED) && \
>> (((x) & DWCMSHC_EMMC_DLL_TIMEOUT) == 0))
>> #define ROCKCHIP_MAX_CLKS 3
>>
>> #define FLAG_INVERTER_FLAG_IN_RXCLK BIT(0)
>> +#define FLAG_TAP_VALUE_SEL BIT(1)
>>
>> struct rockchip_sdhc_plat {
>> struct mmc_config cfg;
>> @@ -317,7 +323,7 @@ static int rk3568_sdhci_config_dll(struct sdhci_host *host, u32 clock, bool enab
>> struct sdhci_data *data = (struct sdhci_data *)dev_get_driver_data(priv->dev);
>> struct mmc *mmc = host->mmc;
>> int val, ret;
>> - u32 extra, txclk_tapnum;
>> + u32 extra, txclk_tapnum, dll_tap_value;
>>
>> if (!enable) {
>> sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_CTRL);
>> @@ -347,7 +353,15 @@ static int rk3568_sdhci_config_dll(struct sdhci_host *host, u32 clock, bool enab
>> if (ret)
>> return ret;
>>
>> - extra = DWCMSHC_EMMC_DLL_DLYENA | DLL_RXCLK_ORI_GATE;
>> + if (data->flags & FLAG_TAP_VALUE_SEL)
>> + dll_tap_value = DLL_TAP_VALUE_SEL |
>> + DLL_LOCK_VALUE(val) << DLL_TAP_VALUE_OFFSET;
>> + else
>> + dll_tap_value = 0;
>> +
>> + extra = DWCMSHC_EMMC_DLL_DLYENA |
>> + DLL_RXCLK_ORI_GATE |
>> + dll_tap_value;
>> if (data->flags & FLAG_INVERTER_FLAG_IN_RXCLK)
>> extra |= DLL_RXCLK_NO_INVERTER;
>> sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_RXCLK);
>> @@ -361,19 +375,22 @@ static int rk3568_sdhci_config_dll(struct sdhci_host *host, u32 clock, bool enab
>> DLL_CMDOUT_BOTH_CLK_EDGE |
>> DWCMSHC_EMMC_DLL_DLYENA |
>> data->hs400_cmdout_tapnum |
>> - DLL_CMDOUT_TAPNUM_FROM_SW;
>> + DLL_CMDOUT_TAPNUM_FROM_SW |
>> + dll_tap_value;
>> sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_CMDOUT);
>> }
>>
>> extra = DWCMSHC_EMMC_DLL_DLYENA |
>> DLL_TXCLK_TAPNUM_FROM_SW |
>> DLL_TXCLK_NO_INVERTER |
>> - txclk_tapnum;
>> + txclk_tapnum |
>> + dll_tap_value;
>> sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_TXCLK);
>>
>> extra = DWCMSHC_EMMC_DLL_DLYENA |
>> data->hs400_strbin_tapnum |
>> - DLL_STRBIN_TAPNUM_FROM_SW;
>> + DLL_STRBIN_TAPNUM_FROM_SW |
>> + dll_tap_value;
>> sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_STRBIN);
>> } else {
>> /*
>> @@ -663,6 +680,7 @@ static const struct sdhci_data rk3528_data = {
>> .set_ios_post = rk3568_sdhci_set_ios_post,
>> .set_clock = rk3568_sdhci_set_clock,
>> .config_dll = rk3568_sdhci_config_dll,
>> + .flags = FLAG_TAP_VALUE_SEL,
>> .hs200_txclk_tapnum = 0xc,
>> .hs400_txclk_tapnum = 0x6,
>> .hs400_cmdout_tapnum = 0x6,
>
More information about the U-Boot
mailing list