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 <[email protected]>
---
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
