On Mon, Jul 20, 2026 at 1:47 AM Yao Zi <[email protected]> wrote:
>
> 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 <[email protected]>
> >
> > ---
> > 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.
>
I can't find the code of disabling vqmmc before changing its voltage in kernel.
mmc_regulator_set_vqmmc() only sets the voltage directly.
> > +}
>
> ...
>
> > +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?
>
OK. I'll switch it based on mode.
> 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.
>
I moved the content of spacemit_sdhci_set_aib_mmc1_io() into pinctrl driver.
> ...
>
> > +#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
