Re: [PATCH v2] mmc: cv1800b_sdhci: configure SDHCI PHY

[Hiago De Franco <hfranco@baylibre.com>]

Hi Andrei,

On Wed, Sep 09, 2026 at 05:21:12PM +0200, Andrei Lalaev wrote:
> Some samples of the Milk-V Duo and Duo 256M have issues with SD card
> communication. As a result, the SD card is not detected, or the correct
> mode is not selected.
> 
> Configure SDHCI PHY in the same way as in downstream, to ensure
> that the PHY is initialized properly.

Thanks for the v2 patch.

I tested this locally with my Milk-V Duo 256MB, same tests as before, and
it works without issues. So:

Tested-by: Hiago De Franco <hfranco@baylibre.com> # Milk-V Duo 256M

My only minor comment would be related to the 'CV18XX_*' defines, the
others do not see to have this prefix, so I would drop them and keep it
like 'SDHCI_MSHC_CTRL'.

However, as usual, I also asked and AI agent (Claude Sonnet 5 - high, in
this case) to review this patch and looks like it found something
useful:

```
This always sets PHY_TX_BPS and writes TX_SRC_INVERT | RX_SRC_INVERT,
and it only runs once, from probe(). Sophgo's own driver treats this
as mode-dependent, not a fixed value - cvi_general_reset() in their
tree branches on the live SDHCI_HOST_CONTROL2 UHS bits:

    if SDR104:
        PHY_CONFIG    &= ~BIT(0)                 /* TX_BPS off */
        PHY_TX_RX_DLY  = 0x00000100              /* TX_SRC_INVERT only, tap=0 */
    else:
        PHY_CONFIG    |= BIT(0)                  /* TX_BPS on */
        PHY_TX_RX_DLY  = 0x01000100              /* TX_SRC_INVERT | RX_SRC_INVERT */

and it does this on every reset, not once. With this patch, a board
that actually negotiates SDR104 (LicheeRV Nano, or any cv1800b board
without no-1-8-v in the DT) would go into the tuning sweep with
TX_BPS stuck on - cv1800b_execute_tuning() never touches offset 0x24c.

I don't think this affects Duo or Duo 256M today - both set no-1-8-v,
so they stay in HS and only ever want the else-branch values, which is
exactly what's tested above. But since cv1800b_sdhci.c is shared with
LicheeRV Nano, it seemed worth flagging rather than silently baking in
a value that's only correct for one mode."
```

I can't test this since I do not have other boards apart from Milk-V duo
with me, maybe you can take a look at this review to check if it is
valid or not?

According to the agent, a possible fix would be:

```
diff --git i/drivers/mmc/cv1800b_sdhci.c w/drivers/mmc/cv1800b_sdhci.c
index 440421813e3..27203613e5f 100644
--- i/drivers/mmc/cv1800b_sdhci.c
+++ w/drivers/mmc/cv1800b_sdhci.c
@@ -70,25 +70,30 @@ static int cv1800b_execute_tuning(struct mmc *mmc, u8 opcode)
 }
 #endif
 
-static int cv1800b_configure_phy(struct sdhci_host *host)
+static int cv1800b_sdhci_set_ios_post(struct sdhci_host *host)
 {
-	u32 val;
+	u32 mshc_ctrl = sdhci_readl(host, CV18XX_SDHCI_MSHC_CTRL);
+	u32 phy_config = sdhci_readl(host, CV18XX_SDHCI_PHY_CONFIG);
 
-	val = sdhci_readl(host, CV18XX_SDHCI_MSHC_CTRL);
-	val |= CV18XX_LATANCY_1T;
-	sdhci_writel(host, val, CV18XX_SDHCI_MSHC_CTRL);
+	if (host->mmc->selected_mode == UHS_SDR104) {
+		mshc_ctrl &= ~CV18XX_LATANCY_1T;
+		phy_config &= ~CV18XX_PHY_TX_BPS;
+		/* tap delay is set by cv1800b_execute_tuning() right after this */
+		sdhci_writel(host, PHY_TX_SRC_INVERT, SDHCI_PHY_TX_RX_DLY);
+	} else {
+		mshc_ctrl |= CV18XX_LATANCY_1T;
+		phy_config |= CV18XX_PHY_TX_BPS;
+		sdhci_writel(host, PHY_TX_SRC_INVERT | PHY_RX_SRC_INVERT, SDHCI_PHY_TX_RX_DLY);
+	}
 
-	val = sdhci_readl(host, CV18XX_SDHCI_PHY_CONFIG);
-	val |= CV18XX_PHY_TX_BPS;
-	sdhci_writel(host, val, CV18XX_SDHCI_PHY_CONFIG);
-
-	val = PHY_TX_SRC_INVERT | PHY_RX_SRC_INVERT;
-	sdhci_writel(host, val, SDHCI_PHY_TX_RX_DLY);
+	sdhci_writel(host, mshc_ctrl, CV18XX_SDHCI_MSHC_CTRL);
+	sdhci_writel(host, phy_config, CV18XX_SDHCI_PHY_CONFIG);
 
 	return 0;
 }
 
 const struct sdhci_ops cv1800b_sdhci_sd_ops = {
+	.set_ios_post = cv1800b_sdhci_set_ios_post,
 #if CONFIG_IS_ENABLED(MMC_SUPPORTS_TUNING)
 	.platform_execute_tuning = cv1800b_execute_tuning,
 #endif
@@ -129,11 +134,7 @@ static int cv1800b_sdhci_probe(struct udevice *dev)
 	if (ret)
 		return ret;
 
-	ret = sdhci_probe(dev);
-	if (ret)
-		return ret;
-
-	return cv1800b_configure_phy(host);
+	return sdhci_probe(dev);
 }
 
 static const struct udevice_id cv1800b_sdhci_match[] = {
```

Regards,
-- 
Hiago