[PATCH] mmc: rockchip_sdhci: Set xx_TAP_VALUE for RK3528

[Quentin Schulz]

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?

We have DLL_TXCLK_TAPNUM_FROM_SW for example, so something like

DLL_TAPVALUE_FROM_SW

?

> +#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?

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).

>   #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,