Re: [PATCH v3 2/3] mmc: sdhci-dwcmshc: Add Canaan K230 DWCMSHC controller support

[Jiayu Du <jiayu.riscv@isrc.iscas.ac.cn>]

On Wed, Mar 11, 2026 at 04:04:03PM +0200, Adrian Hunter wrote:
> On 10/03/2026 08:45, Jiayu Du wrote:
> > Add SDHCI controller driver for Canaan k230 SoC. Implement custom
> > sdhci_ops for set_clock, phy init, init and reset.
> > 
> > Signed-off-by: Jiayu Du <jiayu.riscv@isrc.iscas.ac.cn>
> > ---
> >  drivers/mmc/host/sdhci-of-dwcmshc.c | 287 ++++++++++++++++++++++++++++
> >  1 file changed, 287 insertions(+)
> > 
> > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > index 2b75a36c096b..39d28574cfa8 100644
> > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > @@ -128,9 +128,11 @@
> >  #define PHY_CNFG_PHY_PWRGOOD_MASK	BIT_MASK(1) /* bit [1] */
> >  #define PHY_CNFG_PAD_SP_MASK		GENMASK(19, 16) /* bits [19:16] */
> >  #define PHY_CNFG_PAD_SP			0x0c /* PMOS TX drive strength */
> > +#define PHY_CNFG_PAD_SP_k230		0x09 /* PMOS TX drive strength for k230 */
> >  #define PHY_CNFG_PAD_SP_SG2042		0x09 /* PMOS TX drive strength for SG2042 */
> >  #define PHY_CNFG_PAD_SN_MASK		GENMASK(23, 20) /* bits [23:20] */
> >  #define PHY_CNFG_PAD_SN			0x0c /* NMOS TX drive strength */
> > +#define PHY_CNFG_PAD_SN_k230		0x08 /* NMOS TX drive strength for k230 */
> >  #define PHY_CNFG_PAD_SN_SG2042		0x08 /* NMOS TX drive strength for SG2042 */
> >  
> >  /* PHY command/response pad settings */
> > @@ -153,14 +155,22 @@
> >  #define PHY_PAD_RXSEL_3V3		0x2 /* Receiver type select for 3.3V */
> >  
> >  #define PHY_PAD_WEAKPULL_MASK		GENMASK(4, 3) /* bits [4:3] */
> > +#define PHY_PAD_WEAKPULL_DISABLED	0x0 /* Weak pull up and pull down disabled */
> >  #define PHY_PAD_WEAKPULL_PULLUP		0x1 /* Weak pull up enabled */
> >  #define PHY_PAD_WEAKPULL_PULLDOWN	0x2 /* Weak pull down enabled */
> >  
> >  #define PHY_PAD_TXSLEW_CTRL_P_MASK	GENMASK(8, 5) /* bits [8:5] */
> >  #define PHY_PAD_TXSLEW_CTRL_P		0x3 /* Slew control for P-Type pad TX */
> > +#define PHY_PAD_TXSLEW_CTRL_P_k230_VAL2	0x2 /* Slew control for P-Type pad TX for k230 */
> >  #define PHY_PAD_TXSLEW_CTRL_N_MASK	GENMASK(12, 9) /* bits [12:9] */
> >  #define PHY_PAD_TXSLEW_CTRL_N		0x3 /* Slew control for N-Type pad TX */
> >  #define PHY_PAD_TXSLEW_CTRL_N_SG2042	0x2 /* Slew control for N-Type pad TX for SG2042 */
> > +#define PHY_PAD_TXSLEW_CTRL_N_k230_VAL2	0x2 /* Slew control for N-Type pad TX for k230 */
> > +#define PHY_PAD_TXSLEW_CTRL_N_k230_VAL1	0x1 /* Slew control for N-Type pad TX for k230 */
> > +
> > +/* PHY Common DelayLine config settings */
> > +#define PHY_COMMDL_CNFG			(DWC_MSHC_PTR_PHY_R + 0x1c)
> > +#define PHY_COMMDL_CNFG_DLSTEP_SEL	BIT(0) /* DelayLine outputs on PAD enabled */
> >  
> >  /* PHY CLK delay line settings */
> >  #define PHY_SDCLKDL_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x1d)
> > @@ -174,7 +184,10 @@
> >  #define PHY_SDCLKDL_DC_HS400		0x18 /* delay code for HS400 mode */
> >  
> >  #define PHY_SMPLDL_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x20)
> > +#define PHY_SMPLDL_CNFG_EXTDLY_EN	BIT(0)
> >  #define PHY_SMPLDL_CNFG_BYPASS_EN	BIT(1)
> > +#define PHY_SMPLDL_CNFG_INPSEL_MASK	GENMASK(3, 2) /* bits [3:2] */
> > +#define PHY_SMPLDL_CNFG_INPSEL		0x3 /* delay line input source */
> >  
> >  /* PHY drift_cclk_rx delay line configuration setting */
> >  #define PHY_ATDL_CNFG_R			(DWC_MSHC_PTR_PHY_R + 0x21)
> > @@ -227,6 +240,14 @@
> >  /* SMC call for BlueField-3 eMMC RST_N */
> >  #define BLUEFIELD_SMC_SET_EMMC_RST_N	0x82000007
> >  
> > +/* Canaan specific Registers */
> > +#define SD0_CTRL			0x00
> > +#define SD0_HOST_REG_VOL_STABLE		BIT(4)
> > +#define SD0_CARD_WRITE_PROT		BIT(6)
> > +#define SD1_CTRL			0x08
> > +#define SD1_HOST_REG_VOL_STABLE		BIT(0)
> > +#define SD1_CARD_WRITE_PROT		BIT(2)
> > +
> >  /* Eswin specific Registers */
> >  #define EIC7700_CARD_CLK_STABLE		BIT(28)
> >  #define EIC7700_INT_BCLK_STABLE		BIT(16)
> > @@ -268,6 +289,12 @@ struct eic7700_priv {
> >  	unsigned int drive_impedance;
> >  };
> >  
> > +struct k230_priv  {
> > +	/* Kendryte k230 specific */
> > +	struct regmap *hi_sys_regmap;
> > +	const struct dwcmshc_k230_match_data *match_data;
> > +};
> > +
> >  #define DWCMSHC_MAX_OTHER_CLKS 3
> >  
> >  struct dwcmshc_priv {
> > @@ -275,6 +302,7 @@ struct dwcmshc_priv {
> >  	int vendor_specific_area1; /* P_VENDOR_SPECIFIC_AREA1 reg */
> >  	int vendor_specific_area2; /* P_VENDOR_SPECIFIC_AREA2 reg */
> >  
> > +	const void *match_data;
> >  	int num_other_clks;
> >  	struct clk_bulk_data other_clks[DWCMSHC_MAX_OTHER_CLKS];
> >  
> > @@ -284,12 +312,34 @@ struct dwcmshc_priv {
> >  };
> >  
> >  struct dwcmshc_pltfm_data {
> > +	const void *match_data;
> >  	const struct sdhci_pltfm_data pdata;
> >  	const struct cqhci_host_ops *cqhci_host_ops;
> >  	int (*init)(struct device *dev, struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
> >  	void (*postinit)(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
> >  };
> >  
> > +struct dwcmshc_k230_match_data {
> > +	bool is_emmc;
> > +	u32 ctrl_reg;
> > +	u32 vol_stable_bit;
> > +	u32 write_prot_bit;
> > +};
> > +
> > +static const struct dwcmshc_k230_match_data k230_emmc_match_data = {
> > +	.is_emmc = true,
> > +	.ctrl_reg = SD0_CTRL,
> > +	.vol_stable_bit = SD0_HOST_REG_VOL_STABLE,
> > +	.write_prot_bit = SD0_CARD_WRITE_PROT,
> > +};
> > +
> > +static const struct dwcmshc_k230_match_data k230_sdio_match_data = {
> > +	.is_emmc = false,
> > +	.ctrl_reg = SD1_CTRL,
> > +	.vol_stable_bit = SD1_HOST_REG_VOL_STABLE,
> > +	.write_prot_bit = SD1_CARD_WRITE_PROT,
> > +};
> > +
> >  static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> >  {
> >  	u16 ctrl;
> > @@ -1656,6 +1706,205 @@ static int eic7700_init(struct device *dev, struct sdhci_host *host, struct dwcm
> >  	return 0;
> >  }
> >  
> > +static void dwcmshc_k230_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> > +{
> > +	u16 clk;
> > +
> > +	sdhci_set_clock(host, clock);
> > +
> > +	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> > +	/*
> > +	 * It is necessary to enable SDHCI_PROG_CLOCK_MODE. This is a
> > +	 * vendor-specific quirk. If this is not done, the eMMC will be
> > +	 * unable to read or write.
> > +	 */
> > +	clk |= SDHCI_PROG_CLOCK_MODE;
> > +	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> > +}
> > +
> > +static void sdhci_k230_config_phy_delay(struct sdhci_host *host)
> > +{
> > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +	struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
> > +	u32 val;
> > +
> > +	sdhci_writeb(host, PHY_COMMDL_CNFG_DLSTEP_SEL, PHY_COMMDL_CNFG);
> > +	sdhci_writeb(host, 0x0, PHY_SDCLKDL_CNFG_R);
> > +	sdhci_writeb(host, PHY_SDCLKDL_DC_INITIAL,
> > +		     PHY_SDCLKDL_DC_R);
> 
> Here and elsewhere.  Please avoid unnecessary line wrapping.
> You can use 100 columns.

Thank you. I will correct all unnecessary line wrapping.

> > +
> > +	val = PHY_SMPLDL_CNFG_EXTDLY_EN;
> > +	val |= FIELD_PREP(PHY_SMPLDL_CNFG_INPSEL_MASK,
> > +			  PHY_SMPLDL_CNFG_INPSEL);
> > +	sdhci_writeb(host, val, PHY_SMPLDL_CNFG_R);
> > +
> > +	sdhci_writeb(host, FIELD_PREP(PHY_ATDL_CNFG_INPSEL_MASK,
> > +		     PHY_ATDL_CNFG_INPSEL), PHY_ATDL_CNFG_R);
> 
> Can keep FIELD_PREP() all on one line:
> 
> 	sdhci_writeb(host, FIELD_PREP(PHY_ATDL_CNFG_INPSEL_MASK, PHY_ATDL_CNFG_INPSEL),
> 		     PHY_ATDL_CNFG_R);
> 
> > +
> > +	ret = read_poll_timeout(sdhci_readl, reg,
> > +				(reg & FIELD_PREP(
> > +					PHY_CNFG_PHY_PWRGOOD_MASK, 1)),
> 
> Need not line wrap after "FIELD_PREP("
> 
> > +	if (mask == SDHCI_RESET_ALL) {
> 
> Can reduce the indent by instead:
> 
> 	if (mask != SDHCI_RESET_ALL)
> 		return;
> 
> Then the remaining lines in this function do not need to be
> wrapped.
> 
> > @@ -1834,6 +2092,26 @@ static const struct dwcmshc_pltfm_data sdhci_dwcmshc_eic7700_pdata = {
> >  	.init = eic7700_init,
> >  };
> >  
> > +static const struct dwcmshc_pltfm_data sdhci_dwcmshc_k230_emmc_pdata = {
> > +	.pdata = {
> > +		.ops = &sdhci_dwcmshc_k230_ops,
> > +		.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
> > +			  SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
> > +	},
> > +	.init = dwcmshc_k230_init,
> > +	.match_data = (void *)&k230_emmc_match_data,
> > +};
> 
> Just an option if you like it:
> An alternative is to define your own platform data e.g.
> 
> struct k230_pltfm_data {
> 	struct dwcmshc_pltfm_data dwcmshc_pdata;
> 	bool is_emmc;
> 	u32 ctrl_reg;
> 	u32 vol_stable_bit;
> 	u32 write_prot_bit;
> };
> 
> static const struct k230_pltfm_data k230_emmc_data = {
> 	.dwcmshc_pdata = {
> 		.pdata = {
> 			.ops = &sdhci_dwcmshc_k230_ops,
> 			.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
> 				  SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
> 		},
> 		.init = dwcmshc_k230_init,
> 	},
> 	.is_emmc = true,
> 	.ctrl_reg = SD0_CTRL,
> 	.vol_stable_bit = SD0_HOST_REG_VOL_STABLE,
> 	.write_prot_bit = SD0_CARD_WRITE_PROT,
> };
> 
> static const struct k230_pltfm_data k230_sdio_data = {
> 	.dwcmshc_pdata = {
> 		.pdata = {
> 			.ops = &sdhci_dwcmshc_k230_ops,
> 			.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
> 				  SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
> 		},
> 		.init = dwcmshc_k230_init,
> 	},
> 	.is_emmc = false,
> 	.ctrl_reg = SD1_CTRL,
> 	.vol_stable_bit = SD1_HOST_REG_VOL_STABLE,
> 	.write_prot_bit = SD1_CARD_WRITE_PROT,
> };
> 
> Then:
> 		.compatible = "canaan,k230-emmc",
> 		.data = &k230_emmc_data.dwcmshc_pdata,
> and
> 		.compatible = "canaan,k230-sdio",
> 		.data = &k230_sdio_data.dwcmshc_pdata,
> 
> Although you would still need to add a pointer 
> to struct dwcmshc_pltfm_data from struct dwcmshc_priv,
> but then you could access it like:
> 
> #define to_k230_pdata(pd) container_of(pd, struct k230_pltfm_data, pd)
> 
> 	struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
> 	struct dwcmshc_pltfm_data *dwcmshc_pdata = dwc_priv->dwcmshc_pdata;
> 	struct k230_pltfm_data *k230_pdata = to_k230_pdata(dwcmshc_pdata);
> 
Thank you for your suggestion. This looks better. I will make the changes.

Grateful regards,
Jiayu Du