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