Re: [PATCH v4 04/10] mmc: k1: add sdhci platform driver

[Yao Zi <me@ziyao.cc>]

On Tue, Jul 07, 2026 at 11:21:45PM +0800, Eric Chung wrote:
> Add SDHCI platform driver support for SpacemiT K1 SoC. This driver
> implements the necessary platform-specific operations for the SDHCI
> controller, enabling MMC/SD card functionality on K1-based platforms.
> 
> Signed-off-by: Eric Chung <eric.chung@riscstar.com>
> 
> ---
> v4:
> - Add bulk release operations on reset and clock.
> v3:
> - Enable CMD23 in capability.
> v2:
> - Enable ADMA mode support.
> - Use CMD23 for multi-block read/write.
> - Move ASR/AIB register into pinctrl driver.
> - Correct pinctrl state from "fast" to "uhs".
> - Migrate tuning support from the spacemit linux driver.
> ---
>  drivers/mmc/Kconfig          |   7 +
>  drivers/mmc/Makefile         |   1 +
>  drivers/mmc/spacemit_sdhci.c | 685 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 693 insertions(+)

...

> diff --git a/drivers/mmc/spacemit_sdhci.c b/drivers/mmc/spacemit_sdhci.c
> new file mode 100644
> index 00000000000..1c7dd3a5870
> --- /dev/null
> +++ b/drivers/mmc/spacemit_sdhci.c
> @@ -0,0 +1,685 @@

...

> +#define SPACEMIT_SDHC_RX_CFG_REG        0x118
> +#define  SDHC_RX_SDCLK_SEL0_MASK        GENMASK(1, 0)
> +#define  SDHC_RX_SDCLK_SEL1_MASK        GENMASK(3, 2)
> +#define  SDHC_RX_SDCLK_SEL1             FIELD_PREP(SDHC_RX_SDCLK_SEL1_MASK, 1)

Space and TABs are both used in definitions of macros in this file,
please choose a consistent style.

...

> +static int spacemit_sdhci_set_vqmmc_voltage(struct mmc *mmc, int voltage)
> +{
> +#if CONFIG_IS_ENABLED(DM_REGULATOR)
> +	int ret;
> +
> +	if (!mmc->vqmmc_supply)
> +		return 0;
> +
> +	ret = regulator_set_value(mmc->vqmmc_supply, voltage);
> +	if (ret) {
> +		log_err("failed to set vqmmc voltage to %d.%dV\n",
> +			voltage / 1000000, (voltage / 100000) % 10);
> +		return ret;
> +	}
> +	ret = regulator_set_enable_if_allowed(mmc->vqmmc_supply, true);
> +	if (ret) {
> +		log_err("failed to enable vqmmc supply\n");
> +		return ret;
> +	}
> +#endif
> +	return 0;
> +}
> +
> +static void spacemit_sdhci_set_voltage(struct sdhci_host *host)
> +{
> +	if (IS_ENABLED(CONFIG_MMC_IO_VOLTAGE)) {
> +		struct mmc *mmc = host->mmc;
> +		u32 ctrl;
> +
> +		ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +
> +		switch (mmc->signal_voltage) {
> +		case MMC_SIGNAL_VOLTAGE_330:
> +		case MMC_SIGNAL_VOLTAGE_180: {
> +			bool to_180 = mmc->signal_voltage ==
> +				      MMC_SIGNAL_VOLTAGE_180;
> +			bool ok;
> +			int voltage_mv = to_180 ? 1800000 : 3300000;
> +
> +			if (spacemit_sdhci_set_vqmmc_voltage(mmc, voltage_mv))
> +				return;
> +			if (!IS_SD(mmc))
> +				return;
> +			if (to_180)
> +				ctrl |= SDHCI_CTRL_VDD_180;
> +			else
> +				ctrl &= ~SDHCI_CTRL_VDD_180;
> +			sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> +
> +			mdelay(5);
> +
> +			ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +			ok = !!(ctrl & SDHCI_CTRL_VDD_180) == to_180;
> +			if (ok)
> +				return;
> +
> +			log_err("%d.%dV regulator output not stable\n",
> +				voltage_mv / 1000000,
> +				(voltage_mv / 100000) % 10);
> +			break;
> +		}
> +		default:
> +			/* No signal voltage switch required */
> +			return;
> +		}
> +	}

It seems the only difference between spacemit_sdhci_set_voltage() and
sdhci_set_voltage() is that the earlier doesn't disable vqmmc before
changing its voltage, is this really necessary, since I don't see
similar behavior in the kernel MMC driver? And if it is, please point
the difference out in comment, and explain why if possible.

> +}

...

> +static int spacemit_sdhci_wait_dat0(struct udevice *dev, int state,
> +				    int timeout_us)
> +{
> +	struct mmc *mmc = mmc_get_mmc_dev(dev);
> +	struct sdhci_host *host = mmc->priv;
> +	unsigned long timeout = timer_get_us() + timeout_us;
> +	u32 tmp;
> +
> +	/*
> +	 * readx_poll_timeout is unsuitable because sdhci_readl accepts
> +	 * two arguments
> +	 */

But read_poll_timeout() accepts op with arbitrary number of arguments,
could it be used?

> +	do {
> +		tmp = sdhci_readl(host, SDHCI_PRESENT_STATE);
> +		if (!!(tmp & SDHCI_DATA_0_LVL_MASK) == !!state) {
> +			if (spacemit_sdhci_is_voltage_switch_cmd(host))
> +				spacemit_sdhci_set_clk_gate(host, 1);
> +			return 0;
> +		}
> +	} while (!timeout_us || !time_after(timer_get_us(), timeout));
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static void spacemit_sdhci_set_control_reg(struct sdhci_host *host)
> +{

...

> +	/* Set pinctrl state */
> +	if (IS_ENABLED(CONFIG_PINCTRL)) {
> +		if (mmc->clock >= 200000000)
> +			pinctrl_select_state(mmc->dev, "uhs");
> +		else
> +			pinctrl_select_state(mmc->dev, "default");

Shouldn't pinctrl setting be switched based on mode (whether it's
UHS or not) instead of clock frequency, as what has been done in the
kernel side?

In v1 of this series, the spacemit_sdhci_set_aib_mmc1_io() is also
called with the selected signal voltage, so I think voltage switching
logic is incorrect here.

...

> +#if CONFIG_IS_ENABLED(MMC_HS400_ES_SUPPORT)
> +static int spacemit_sdhci_phy_dll_init(struct sdhci_host *host)
> +{
> +	u32 reg;
> +	int i;
> +
> +	/* Configure DLL predly, fulldly, and vreg */
> +	spacemit_sdhci_clrsetbits(host, SDHC_DLL_PREDLY_NUM |
> +				  SDHC_DLL_FULLDLY_RANGE |
> +				  SDHC_DLL_VREG_CTRL,
> +				  FIELD_PREP(SDHC_DLL_PREDLY_NUM, 1) |
> +				  FIELD_PREP(SDHC_DLL_FULLDLY_RANGE, 1) |
> +				  FIELD_PREP(SDHC_DLL_VREG_CTRL, 1),
> +				  SPACEMIT_SDHC_PHY_DLLCFG);
> +
> +	reg = sdhci_readl(host, SPACEMIT_SDHC_PHY_DLLCFG1);
> +	reg |= FIELD_PREP(SDHC_DLL_REG1_CTRL, 0x92);

What does 0x92 mean here? Please define it as a macro with meaningful
name if possible, like SPACEMIT_SDHC_PHY_DLLCFG's case.

> +	sdhci_writel(host, reg, SPACEMIT_SDHC_PHY_DLLCFG1);

...

> +	/* Wait for DLL lock */
> +	i = 0;
> +	while (i++ < 100) {
> +		if (sdhci_readl(host, SPACEMIT_SDHC_PHY_DLLSTS) & SDHC_DLL_LOCK_STATE)
> +			break;
> +		udelay(10);
> +	}
> +	if (i == 100) {
> +		log_err("%s: phy dll lock timeout\n", host->name);
> +		return -ETIMEDOUT;
> +	}

Please use read*_poll_timeout() family to replace the loop and the
following check.

> +
> +	return 0;
> +}

...

> +static int spacemit_sdhci_probe(struct udevice *dev)
> +{

...

> +	/* Set quirks */

I think it's obvious, and there's no need to comment it, but

> +	if (IS_ENABLED(CONFIG_SPL_BUILD))
> +		host->quirks = SDHCI_QUIRK_WAIT_SEND_CMD;
> +	else
> +		host->quirks = SDHCI_QUIRK_WAIT_SEND_CMD |
> +				SDHCI_QUIRK_32BIT_DMA_ADDR;

Why SDHCI_QUIRK_32BIT_DMA_ADDR is only enabled in SPL build? Even it
isn't used by any code, it shouldn't hurt to set the flag, which is
less confusing.

...

> +static const struct udevice_id spacemit_sdhci_ids[] = {
> +	{
> +		.compatible = "spacemit,k1-sdhci",
> +		.data = 0,

Uninitialized fields of static variables are automatically zeroed in C,
so please remove this assignment.

> +	}, {
> +	}
> +};

Regards,
Yao Zi