Re: [PATCH 2/8] mmc: k1: add sdhci platform driver

[Yao Zi <me@ziyao.cc>]

On Fri, Jun 12, 2026 at 04:18:55PM -0400, Raymond Mao wrote:
> From: Guodong Xu <guodong@riscstar.com>
> 
> 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.

Is there any reason not to re-use Linux-side K1 MMC driver,
sdhci-of-k1.c, and its companion ABI? Since Linux commit e9cb83c10071
(mmc: sdhci-of-k1: add comprehensive SDR tuning support, 2026-05-11)
it is now capable of operating on SD cards, too.

> Signed-off-by: Guodong Xu <guodong@riscstar.com>
> Signed-off-by: Raymond Mao <raymond.mao@riscstar.com>
> ---
>  drivers/mmc/Kconfig          |   7 +
>  drivers/mmc/Makefile         |   1 +
>  drivers/mmc/spacemit_sdhci.c | 934 +++++++++++++++++++++++++++++++++++
>  3 files changed, 942 insertions(+)
>  create mode 100644 drivers/mmc/spacemit_sdhci.c

...

> diff --git a/drivers/mmc/spacemit_sdhci.c b/drivers/mmc/spacemit_sdhci.c
> new file mode 100644
> index 00000000000..392ca389fa9
> --- /dev/null
> +++ b/drivers/mmc/spacemit_sdhci.c

...

> +static void spacemit_sdhci_set_aib_mmc1_io(struct sdhci_host *host, int voltage)
> +{
> +	struct mmc *mmc = host->mmc;
> +	struct spacemit_sdhci_plat *plat = dev_get_plat(mmc->dev);
> +	void __iomem *aib_mmc1_io, *apbc_asfar, *apbc_assar;
> +	u32 reg;
> +
> +	if (!plat->aib_mmc1_io_reg || !plat->apbc_asfar_reg ||
> +	    !plat->apbc_assar_reg)
> +		return;
> +
> +	aib_mmc1_io = map_sysmem((uintptr_t)plat->aib_mmc1_io_reg,
> +				 sizeof(u32));
> +	apbc_asfar = map_sysmem((uintptr_t)plat->apbc_asfar_reg,
> +				sizeof(u32));
> +	apbc_assar = map_sysmem((uintptr_t)plat->apbc_assar_reg,
> +				sizeof(u32));

AFAIK aib_mmc1_io region is related to pinctrl settings, and shouldn't
be handled by the MMC driver directly. Quoting my own reply[1] to the
series adding SD support to sdhci-of-k1.c in Linux side,

> Also, the pin controller on K1 SoC seems to have some undocumented
> registers to select the IO voltage of SD pins, which should be
> adjusted when switching IO voltage.
>
> I think these pins should be implemented in the pinctrl driver, then
> you could create two pinctrl states, one for 1.8v operation, one for
> 3.3v, and switch between them through pinctrl_lookup_state() when
> changing IO voltage.

This idea is later confirmed by SpacemiT guy's series[2],

> On K1, IO domain power control registers determine whether a GPIO bank
> operates at 1.8V or 3.3V. These registers default to 3.3V operation,
> which may lead to functional failures when GPIO banks are externally
> supplied with 1.8V but internally remain configured for 3.3V.
>
> The IO power domain registers are implemented as secure registers and
> require an explicit unlock sequence via the AIB Secure Access Register
> (ASAR), located in the APBC register space.

Thus I think you should move the logic to pinctrl driver instead, like
what has been done in the Linux upstream driver.

> +	writel(AKEY_ASFAR, apbc_asfar);
> +	writel(AKEY_ASSAR, apbc_assar);
> +	reg = readl(aib_mmc1_io);
> +
> +	switch (voltage) {
> +	case MMC_SIGNAL_VOLTAGE_180:
> +		reg |= MMC1_IO_V18EN;
> +		break;
> +	default:
> +		reg &= ~MMC1_IO_V18EN;
> +		break;
> +	}
> +	writel(AKEY_ASFAR, apbc_asfar);
> +	writel(AKEY_ASSAR, apbc_assar);
> +	writel(reg, aib_mmc1_io);
> +}

...

> +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, "fast");
> +		else
> +			pinctrl_select_state(mmc->dev, "default");
> +	}

This doesn't match Linux side ABI, where when card operates in UHS mode,
pinctrl state "uhs" is selected.

Regards,
Yao Zi

[1]: https://lore.kernel.org/linux-riscv/aUDwDkd8k_4gD1yc@pie/
[2]: https://lore.kernel.org/linux-riscv/20260108-kx-pinctrl-aib-io-pwr-domain-v2-0-6bcb46146e53@linux.spacemit.com