Re: [PATCH v2 2/2] mmc: sdhci-of-k1: add support for SpacemiT K1 SoC

Yixun Lan
2025-05-22T11:03:46Z

Hi Ulf,

On 13:34 Mon 19 May , Ulf Hansson wrote:
> On Thu, 1 May 2025 at 10:51, Yixun Lan <dlan@gentoo.org> wrote:
> >
> > The SDHCI controller found in SpacemiT K1 SoC features SD,
> > SDIO, eMMC support, such as:
> >
> > - Compatible for 4-bit SDIO 3.0 UHS-I protocol, up to SDR104
> > - Compatible for 4-bit SD 3.0 UHS-I protocol, up to SDR104
> > - Compatible for 8bit eMMC5.1, up to HS400
> >
> > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > ---
> > drivers/mmc/host/Kconfig | 14 ++
> > drivers/mmc/host/Makefile | 1 +
> > drivers/mmc/host/sdhci-of-k1.c | 306 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 321 insertions(+)
> >
> > [...]
> > +
> > +#include "sdhci.h"
> > +#include "sdhci-pltfm.h"
> > +
> > +#define SDHC_MMC_CTRL_REG 0x114
I will add 'SPACEMIT_' prefix for the register definitions,
AFAIK, vendor will continue to reuse this IP for their next generation SoC

> > +#define MISC_INT_EN BIT(1)
> > +#define MISC_INT BIT(2)
for BITs definition, it's also quite generic.. I could add
'SDHC_' prefix to make them slightly unique, and as all those registers
fall into SDHC category..

>
> These define-names look a bit too generic to me. Please add some
> additional prefixes so it becomes more clear what they are.
>
Initially, I've followed the datasheet closely for creating those naming..

https://developer.spacemit.com/documentation?token=WZNvwFDkYinYx0k9jzPcMK5WnIe#12.5.4.36-sdhc_mmc_ctrl_reg-register

> This applies to all the others below too.
>
> > +#define ENHANCE_STROBE_EN BIT(8)
> > +#define MMC_HS400 BIT(9)
> > +#define MMC_HS200 BIT(10)
> > +#define MMC_CARD_MODE BIT(12)
> > +
> > +#define SDHC_TX_CFG_REG 0x11C
> > +#define TX_INT_CLK_SEL BIT(30)
> > +#define TX_MUX_SEL BIT(31)
> > +
> > +#define SDHC_PHY_CTRL_REG 0x160
> > +#define PHY_FUNC_EN BIT(0)
> > +#define PHY_PLL_LOCK BIT(1)
> > +#define HOST_LEGACY_MODE BIT(31)
> > +
> > +#define SDHC_PHY_FUNC_REG 0x164
> > +#define PHY_TEST_EN BIT(7)
> > +#define HS200_USE_RFIFO BIT(15)
> > +
> > +#define SDHC_PHY_DLLCFG 0x168
> > +#define DLL_PREDLY_NUM GENMASK(3, 2)
> > +#define DLL_FULLDLY_RANGE GENMASK(5, 4)
> > +#define DLL_VREG_CTRL GENMASK(7, 6)
> > +#define DLL_ENABLE BIT(31)
> > +
> > +#define SDHC_PHY_DLLCFG1 0x16C
> > +#define DLL_REG1_CTRL GENMASK(7, 0)
> > +#define DLL_REG2_CTRL GENMASK(15, 8)
> > +#define DLL_REG3_CTRL GENMASK(23, 16)
> > +#define DLL_REG4_CTRL GENMASK(31, 24)
> > +
> > +#define SDHC_PHY_DLLSTS 0x170
> > +#define DLL_LOCK_STATE BIT(0)
> > +
> > +#define SDHC_PHY_PADCFG_REG 0x178
> > +#define PHY_DRIVE_SEL GENMASK(2, 0)
> > +#define RX_BIAS_CTRL BIT(5)
>
> [...]
>
> > +
> > +static int spacemit_sdhci_pre_select_hs400(struct mmc_host *mmc)
> > +{
> > + struct sdhci_host *host = mmc_priv(mmc);
> > +
> > + spacemit_sdhci_setbits(host, MMC_HS400, SDHC_MMC_CTRL_REG);
> > + host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
>
> At least this deserves a comment. Isn't MMC_CAP_WAIT_WHILE_BUSY
> working for all cases?
>
I mostly copy the logic from vendor driver while refactoring the code,
and again check the logic, it sounds a little bit weird that the flag
is enabled in pre_select_hs400(), then disabled in post_select_hs400(),
I really don't understand the logic behind this, or even any quirk?..

while, I've tested both cases of enabling or disabling the flag globally,
they both works fine as result.. so to be conservative, I plan to drop
this "enable-and-disable" logic, and would revisit them once adding
"SD card/SDIO" support in the future

> > +
> > + return 0;
> > +}
> > +
> > +static void spacemit_sdhci_post_select_hs400(struct mmc_host *mmc)
> > +{
> > + struct sdhci_host *host = mmc_priv(mmc);
> > +
> > + spacemit_sdhci_phy_dll_init(host);
> > + host->mmc->caps &= ~MMC_CAP_WAIT_WHILE_BUSY;
>
> Dito.
>
> [...]
>
> Kind regards
> Uffe

--
Yixun Lan (dlan)