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

From: Yao Zi <me@ziyao.cc>
To: Jiayu Du <jiayu.riscv@isrc.iscas.ac.cn>,
	ulf.hansson@linaro.org, adrian.hunter@intel.com, robh@kernel.org,
	krzk+dt@kernel.org, conor+dt@kernel.org
Cc: pjw@kernel.org, palmer@dabbelt.com, aou@eecs.berkeley.edu,
	linux-mmc@vger.kernel.org, devicetree@vger.kernel.org,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	gaohan@iscas.ac.cn
Subject: Re: [PATCH 2/3] mmc: sdhci-dwcmshc: Add Canaan K230 DWCMSHC controller support
Date: Wed, 4 Feb 2026 09:43:03 +0000	[thread overview]
Message-ID: <aYMUp6maC70zDrAP@pie> (raw)
In-Reply-To: <20260204082908.27501-3-jiayu.riscv@isrc.iscas.ac.cn>

On Wed, Feb 04, 2026 at 04:29:07PM +0800, 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 | 247 ++++++++++++++++++++++++++++
>  1 file changed, 247 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 204830b40587..bc427bfbba25 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c

...

> +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);
> +	clk |= SDHCI_PROG_CLOCK_MODE;
> +	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);

Why it's necessary to always set SDHCI_PROG_CLOCK_MODE? If it's a vendor
quirk, it deserves a comment to explain what's happening here.

> +}

...

> +static int dwcmshc_k230_phy_init(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	u32 rxsel = PHY_PAD_RXSEL_3V3;
> +	unsigned int timeout = 15000;
> +	u32 val;
> +	u32 reg;
> +
> +	/* reset phy */
> +	sdhci_writew(host, 0, PHY_CNFG_R);
> +
> +	/* Disable the clock */
> +	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> +
> +	if (priv->flags & FLAG_IO_FIXED_1V8)
> +		rxsel = PHY_PAD_RXSEL_1V8;

This may be a little nitpick, but the logic looks cleaner to me if you
do

	rxsel = priv->flags & FLAG_IO_FIXED_1V8 ?
			PHY_PAD_RXSEL_1V8 : PHY_PAD_RXSEL_3V3;

...

> +	/* Wait max 150 ms */
> +	while (1) {
> +		reg = sdhci_readl(host, PHY_CNFG_R);
> +		if (reg & FIELD_PREP(PHY_CNFG_PHY_PWRGOOD_MASK, 1))
> +			break;
> +		if (!timeout)
> +			return -ETIMEDOUT;
> +		timeout--;
> +		usleep_range(10, 15);
> +	}

You could use readx_poll_timeout() with sdhci_readl() to simplify this
loop.

> +	reg = FIELD_PREP(PHY_CNFG_PAD_SN_MASK, PHY_CNFG_PAD_SN_k230) |
> +	      FIELD_PREP(PHY_CNFG_PAD_SP_MASK, PHY_CNFG_PAD_SP_k230);
> +	sdhci_writel(host, reg, PHY_CNFG_R);
> +
> +	/* de-assert the phy */
> +	reg |= PHY_CNFG_RSTN_DEASSERT;
> +	sdhci_writel(host, reg, PHY_CNFG_R);
> +
> +	return 0;
> +}
> +static int dwcmshc_k230_init(struct device *dev, struct sdhci_host *host,
> +			     struct dwcmshc_priv *dwc_priv)
> +{

...

> +	if (!usb_phy_node) {
> +		return dev_err_probe(dev, -ENODEV,
> +				     "Failed to find k230-usb-phy node\n");
> +	}

Please drop the curly braces, if there's only one statement in the body
of an if, they're not necessary. This is also preferred in kernel coding
style,

> Do not unnecessarily use braces where a single statement will do.[1]

Same for branches below.

> +	k230_priv->hi_sys_regmap = device_node_to_regmap(usb_phy_node);
> +	of_node_put(usb_phy_node);
> +	if (IS_ERR(k230_priv->hi_sys_regmap)) {
> +		return dev_err_probe(dev, PTR_ERR(k230_priv->hi_sys_regmap),
> +				     "Failed to get k230-usb-phy regmap\n");
> +	}

...

> +	host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;

Why not set it in sdhci_dwcmshc_k230_data.pdata.quirks?

> +	return 0;
> +}

...

> +static const struct dwcmshc_pltfm_data sdhci_dwcmshc_k230_pdata = {
> +	.pdata = {
> +		.ops = &sdhci_dwcmshc_k230_ops,
> +		.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> +	},
> +	.init = dwcmshc_k230_init,
> +};

Best regards,
Yao Zi

[1]: https://elixir.bootlin.com/linux/v6.19-rc5/source/Documentation/process/coding-style.rst#L197