[PATCH] mmc: sunxi: mask DATA0 during H6-family clock updates

[Andre Przywara]

Hi James,


On 7/3/26 21:47, James Hilliard wrote:
> Linux marks the H6/A100-style controllers with mask_data0 and sets the
> CLKCR MASK_DATA0 bit while issuing update-clock commands. Without this,
> the controller can sample DAT0 as busy around clock updates before the
> first card command, so card detection may fail with -ETIMEDOUT.

Can you please say *WHY* this change is needed? Is this "card detection 
may fail" a theoretical assumption, or did this happen to you in 
practice? I wouldn't be aware of any reports about issues with MMC in 
U-Boot, so I'd be curious to hear about that, with a concrete example, 
and how often this happens, if you experienced this. Also if that 
applies to the SPL or to U-Boot proper only.

> Mirror the Linux behavior for H6-family and NCAT2-family controllers while
> updating the card clock.
> 
> Signed-off-by: James Hilliard <james.hilliard1 at gmail.com>
> ---
>   drivers/mmc/sunxi_mmc.c | 30 +++++++++++++++++++++++++++---
>   drivers/mmc/sunxi_mmc.h |  1 +
>   2 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c
> index e28c81afffe..145d6bd7c9d 100644
> --- a/drivers/mmc/sunxi_mmc.c
> +++ b/drivers/mmc/sunxi_mmc.c
> @@ -64,6 +64,12 @@ static bool sunxi_mmc_can_calibrate(void)
>   	       IS_ENABLED(CONFIG_MACH_SUN8I_R40);
>   }
>   
> +static bool sunxi_mmc_needs_data0_mask(void)
> +{
> +	return IS_ENABLED(CONFIG_SUN50I_GEN_H6) ||
> +	       IS_ENABLED(CONFIG_SUNXI_GEN_NCAT2);

Meh, I see that this is a pragmatic approach, but I wonder if this 
should come from the DT when in U-Boot proper (or when using DM_MMC in 
general).

> +}
> +
>   static int mmc_set_mod_clk(struct sunxi_mmc_priv *priv, unsigned int hz)
>   {
>   	unsigned int pll, pll_hz, div, n, oclk_dly, sclk_dly;
> @@ -201,14 +207,31 @@ static int mmc_update_clk(struct sunxi_mmc_priv *priv)
>   	return 0;
>   }
>   
> +static int mmc_update_clk_with_data0_mask(struct sunxi_mmc_priv *priv)

If I see this correctly, then this function is only called (twice) from 
mmc_config_clock() below, but you don't change the other user in the 
error path of sunxi_mmc_send_cmd_common(), right? It's probably worth 
mentioning this in the commit message.

And if that's true, please fold the functionality into 
mmc_config_clock(). This one knows the value of clkcr already, so no 
need for two read/modify/write operations, and no need for a function 
with a lengthy name.

> +{
> +	bool mask_data0 = sunxi_mmc_needs_data0_mask();
> +	int ret;
> +
> +	if (mask_data0)
> +		setbits_le32(&priv->reg->clkcr, SUNXI_MMC_CLK_MASK_DATA0);
> +
> +	ret = mmc_update_clk(priv);
> +
> +	if (mask_data0)
> +		clrbits_le32(&priv->reg->clkcr, SUNXI_MMC_CLK_MASK_DATA0);
> +
> +	return ret;
> +}
> +
>   static int mmc_config_clock(struct sunxi_mmc_priv *priv, struct mmc *mmc)
>   {
>   	unsigned rval = readl(&priv->reg->clkcr);
>   
>   	/* Disable Clock */
> -	rval &= ~SUNXI_MMC_CLK_ENABLE;
> +	rval &= ~(SUNXI_MMC_CLK_ENABLE | SUNXI_MMC_CLK_POWERSAVE |

What about this POWERSAVE bit? If you change this as well, please 
mention this in the commit message, and say why this is needed (as it 
wasn't used before in the driver).

Cheers,
Andre

> +		  SUNXI_MMC_CLK_MASK_DATA0);
>   	writel(rval, &priv->reg->clkcr);
> -	if (mmc_update_clk(priv))
> +	if (mmc_update_clk_with_data0_mask(priv))
>   		return -1;
>   
>   	/* Set mod_clk to new rate */
> @@ -231,9 +254,10 @@ static int mmc_config_clock(struct sunxi_mmc_priv *priv, struct mmc *mmc)
>   #endif
>   
>   	/* Re-enable Clock */
> +	rval &= ~(SUNXI_MMC_CLK_POWERSAVE | SUNXI_MMC_CLK_MASK_DATA0);
>   	rval |= SUNXI_MMC_CLK_ENABLE;
>   	writel(rval, &priv->reg->clkcr);
> -	if (mmc_update_clk(priv))
> +	if (mmc_update_clk_with_data0_mask(priv))
>   		return -1;
>   
>   	return 0;
> diff --git a/drivers/mmc/sunxi_mmc.h b/drivers/mmc/sunxi_mmc.h
> index 71865160319..c9a0f3833a2 100644
> --- a/drivers/mmc/sunxi_mmc.h
> +++ b/drivers/mmc/sunxi_mmc.h
> @@ -58,6 +58,7 @@ struct sunxi_mmc {
>   
>   #define SUNXI_MMC_CLK_POWERSAVE		(0x1 << 17)
>   #define SUNXI_MMC_CLK_ENABLE		(0x1 << 16)
> +#define SUNXI_MMC_CLK_MASK_DATA0	(0x1U << 31)
>   #define SUNXI_MMC_CLK_DIVIDER_MASK	(0xff)
>   
>   #define SUNXI_MMC_GCTRL		0x000