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

[Andrei Lalaev <andrey.lalaev@gmail.com>]

Hi Hiago,

On 09.09.26 22:18, Hiago De Franco wrote:
> 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

Thank you for the tag :)

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

To be honest, I would rename the other defines as they are not generic SDHCI,
but CV1800B-specific, so I think it makes sense to prepend CV18XX.
No strong opinion, though.

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

I don't have a LicheeRC Nano either, but both DTSes have 'no-1-8-v' property:
  - arch/riscv/dts/sg2002-licheerv-nano-b.dts +38
  - dts/upstream/src/riscv/sophgo/sg2002-licheerv-nano-b.dts +85

So it shouldn't break the board.

That said, since I'd prefer not to introduce changes I can't test
myself (like SDR104 mode), we can do something like this instead:

```
diff --git a/drivers/mmc/cv1800b_sdhci.c b/drivers/mmc/cv1800b_sdhci.c
index b756649f90f3..2512352383b2 100644
--- a/drivers/mmc/cv1800b_sdhci.c
+++ b/drivers/mmc/cv1800b_sdhci.c
@@ -8,11 +8,17 @@
 #include <sdhci.h>
 #include <linux/delay.h>
 
+#define SDHCI_MSHC_CTRL      0x200
+#define SDHCI_PHY_CONFIG     0x24c
 #define SDHCI_PHY_TX_RX_DLY  0x240
 #define MMC_MAX_CLOCK        375000000
 #define TUNE_MAX_PHCODE      128
 
 #define PHY_TX_SRC_INVERT  BIT(8)
+#define PHY_RX_SRC_INVERT  BIT(24)
+
+#define PHY_LATANCY_1T BIT(1)
+#define PHY_TX_BPS BIT(0)
 
 struct cv1800b_sdhci_plat {
 	struct mmc_config cfg;
@@ -64,10 +70,34 @@ static int cv1800b_execute_tuning(struct mmc *mmc, u8 opcode)
 }
 #endif
 
+static int cv1800b_set_ios_post(struct sdhci_host *host)
+{
+	struct mmc *mmc = host->mmc;
+	u32 val;
+
+	if (mmc->selected_mode != MMC_LEGACY && mmc->selected_mode != MMC_HS &&
+	    mmc->selected_mode != SD_HS)
+		return 0;
+
+	val = sdhci_readl(host, SDHCI_MSHC_CTRL);
+	val |= PHY_LATANCY_1T;
+	sdhci_writel(host, val, SDHCI_MSHC_CTRL);
+
+	val = sdhci_readl(host, SDHCI_PHY_CONFIG);
+	val |= PHY_TX_BPS;
+	sdhci_writel(host, val, SDHCI_PHY_CONFIG);
+
+	val = PHY_TX_SRC_INVERT | PHY_RX_SRC_INVERT;
+	sdhci_writel(host, val, SDHCI_PHY_TX_RX_DLY);
+
+	return 0;
+}
+
 const struct sdhci_ops cv1800b_sdhci_sd_ops = {
 #if CONFIG_IS_ENABLED(MMC_SUPPORTS_TUNING)
 	.platform_execute_tuning = cv1800b_execute_tuning,
 #endif
+	.set_ios_post = cv1800b_set_ios_post,
 };
 
 static int cv1800b_sdhci_bind(struct udevice *dev)
```


> According to the agent, a possible fix would be:
> ...
> 
> Regards,


-- 
Best regards,
Andrei Lalaev