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
@@ -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 */
+ bool have_phy;
+ struct regmap *hi_sys_regmap;
+};
+
#define DWCMSHC_MAX_OTHER_CLKS 3
struct dwcmshc_priv {
@@ -1650,6 +1677,201 @@ 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);
+ 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);
+
+ 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);
+
+ val = sdhci_readl(host, dwc_priv->vendor_specific_area1 +
+ DWCMSHC_EMMC_ATCTRL);
+ val |= AT_CTRL_TUNE_CLK_STOP_EN;
+ val |= FIELD_PREP(AT_CTRL_PRE_CHANGE_DLY_MASK,
+ AT_CTRL_PRE_CHANGE_DLY);
+ val |= FIELD_PREP(AT_CTRL_POST_CHANGE_DLY_MASK,
+ AT_CTRL_POST_CHANGE_DLY);
+ sdhci_writel(host, val, dwc_priv->vendor_specific_area1 +
+ DWCMSHC_EMMC_ATCTRL);
+ sdhci_writel(host, 0x0, dwc_priv->vendor_specific_area1 +
+ DWCMSHC_AT_STAT);
+}
+
+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;
+
+ val = rxsel;
+ val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_P_MASK, PHY_PAD_TXSLEW_CTRL_P_k230_VAL2);
+ val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_N_MASK, PHY_PAD_TXSLEW_CTRL_N_k230_VAL2);
+ val |= FIELD_PREP(PHY_PAD_WEAKPULL_MASK, PHY_PAD_WEAKPULL_PULLUP);
+
+ sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
+ sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
+ sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
+
+ val = rxsel;
+ val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_P_MASK, PHY_PAD_TXSLEW_CTRL_P_k230_VAL2);
+ val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_N_MASK, PHY_PAD_TXSLEW_CTRL_N_k230_VAL2);
+ sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
+
+ val = rxsel;
+ val |= FIELD_PREP(PHY_PAD_WEAKPULL_MASK, PHY_PAD_WEAKPULL_PULLDOWN);
+ val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_P_MASK, PHY_PAD_TXSLEW_CTRL_P_k230_VAL2);
+ val |= FIELD_PREP(PHY_PAD_TXSLEW_CTRL_N_MASK, PHY_PAD_TXSLEW_CTRL_N_k230_VAL2);
+ sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
+
+ sdhci_k230_config_phy_delay(host);
+
+ /* 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);
+ }
+
+ 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 void dwcmshc_k230_sdhci_reset(struct sdhci_host *host, u8 mask)
+{
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
+ struct k230_priv *k230_priv = dwc_priv->priv;
+ u8 emmc_ctrl;
+
+ dwcmshc_reset(host, mask);
+
+ if (mask == SDHCI_RESET_ALL) {
+ emmc_ctrl = sdhci_readw(host,
+ dwc_priv->vendor_specific_area1 +
+ DWCMSHC_EMMC_CONTROL);
+ sdhci_writeb(host, emmc_ctrl,
+ dwc_priv->vendor_specific_area1 +
+ DWCMSHC_EMMC_CONTROL);
+
+ if (k230_priv->have_phy)
+ dwcmshc_k230_phy_init(host);
+ else
+ sdhci_writel(host, 0x0,
+ dwc_priv->vendor_specific_area1 +
+ DWCMSHC_HOST_CTRL3);
+ }
+}
+
+static int dwcmshc_k230_init(struct device *dev, struct sdhci_host *host,
+ struct dwcmshc_priv *dwc_priv)
+{
+ static const char * const clk_ids[] = {"base", "timer", "ahb"};
+ struct device_node *usb_phy_node;
+ struct k230_priv *k230_priv;
+ u32 data;
+ int ret;
+
+ k230_priv = devm_kzalloc(dev, sizeof(struct k230_priv), GFP_KERNEL);
+ if (!k230_priv)
+ return -ENOMEM;
+ dwc_priv->priv = k230_priv;
+
+ usb_phy_node = of_find_compatible_node(NULL, NULL, "canaan,k230-usb-phy");
+ if (!usb_phy_node) {
+ return dev_err_probe(dev, -ENODEV,
+ "Failed to find k230-usb-phy node\n");
+ }
+
+ 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");
+ }
+
+ ret = dwcmshc_get_enable_other_clks(mmc_dev(host->mmc), dwc_priv,
+ ARRAY_SIZE(clk_ids), clk_ids);
+ if (ret) {
+ return dev_err_probe(dev, ret,
+ "Failed to get/enable k230 mmc other clocks\n");
+ }
+
+ if (of_device_is_compatible(dev->of_node, "canaan,k230-sdio")) {
+ k230_priv->have_phy = false;
+ host->mmc->caps |= MMC_CAP_SD_HIGHSPEED;
+ host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
+ ret = regmap_read(k230_priv->hi_sys_regmap, SD1_CTRL, &data);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to read SD1_CTRL\n");
+
+ data |= SD1_CARD_WRITE_PROT | SD1_HOST_REG_VOL_STABLE;
+ ret = regmap_write(k230_priv->hi_sys_regmap, SD1_CTRL, data);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to write SD1_CTRL\n");
+ } else {
+ k230_priv->have_phy = true;
+ host->flags &= ~SDHCI_SIGNALING_330;
+ dwc_priv->flags |= FLAG_IO_FIXED_1V8;
+ ret = regmap_read(k230_priv->hi_sys_regmap, SD0_CTRL, &data);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to read SD0_CTRL\n");
+
+ data |= SD0_CARD_WRITE_PROT | SD0_HOST_REG_VOL_STABLE;
+ ret = regmap_write(k230_priv->hi_sys_regmap, SD0_CTRL, data);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to write SD0_CTRL\n");
+ }
+ host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
+
+ return 0;
+}
+
static const struct sdhci_ops sdhci_dwcmshc_ops = {
.set_clock = sdhci_set_clock,
.set_bus_width = sdhci_set_bus_width,
@@ -1736,6 +1958,15 @@ static const struct sdhci_ops sdhci_dwcmshc_eic7700_ops = {
.platform_execute_tuning = sdhci_eic7700_executing_tuning,
};
+static const struct sdhci_ops sdhci_dwcmshc_k230_ops = {
+ .set_clock = dwcmshc_k230_sdhci_set_clock,
+ .set_bus_width = sdhci_set_bus_width,
+ .set_uhs_signaling = dwcmshc_set_uhs_signaling,
+ .get_max_clock = sdhci_pltfm_clk_get_max_clock,
+ .reset = dwcmshc_k230_sdhci_reset,
+ .adma_write_desc = dwcmshc_adma_write_desc,
+};
+
static const struct dwcmshc_pltfm_data sdhci_dwcmshc_pdata = {
.pdata = {
.ops = &sdhci_dwcmshc_ops,
@@ -1827,6 +2058,14 @@ static const struct dwcmshc_pltfm_data sdhci_dwcmshc_eic7700_pdata = {
.init = eic7700_init,
};
+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,
+};
+
static const struct cqhci_host_ops dwcmshc_cqhci_ops = {
.enable = dwcmshc_sdhci_cqe_enable,
.disable = sdhci_cqe_disable,
@@ -1899,6 +2138,14 @@ static void dwcmshc_cqhci_init(struct sdhci_host *host, struct platform_device *
}
static const struct of_device_id sdhci_dwcmshc_dt_ids[] = {
+ {
+ .compatible = "canaan,k230-emmc",
+ .data = &sdhci_dwcmshc_k230_pdata,
+ },
+ {
+ .compatible = "canaan,k230-sdio",
+ .data = &sdhci_dwcmshc_k230_pdata,
+ },
{
.compatible = "rockchip,rk3588-dwcmshc",
.data = &sdhci_dwcmshc_rk35xx_pdata,
--
2.52.0On 04/02/2026 09:29, Jiayu Du wrote:
> +static int dwcmshc_k230_init(struct device *dev, struct sdhci_host *host,
> + struct dwcmshc_priv *dwc_priv)
> +{
> + static const char * const clk_ids[] = {"base", "timer", "ahb"};
> + struct device_node *usb_phy_node;
> + struct k230_priv *k230_priv;
> + u32 data;
> + int ret;
> +
> + k230_priv = devm_kzalloc(dev, sizeof(struct k230_priv), GFP_KERNEL);
> + if (!k230_priv)
> + return -ENOMEM;
> + dwc_priv->priv = k230_priv;
> +
> + usb_phy_node = of_find_compatible_node(NULL, NULL, "canaan,k230-usb-phy");
Hm? You should use phandles, not look for various nodes.
> + if (!usb_phy_node) {
Please follow Linux coding style.
> + return dev_err_probe(dev, -ENODEV,
> + "Failed to find k230-usb-phy node\n");
> + }
> +
> + 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");
> + }
> +
> + ret = dwcmshc_get_enable_other_clks(mmc_dev(host->mmc), dwc_priv,
> + ARRAY_SIZE(clk_ids), clk_ids);
> + if (ret) {
> + return dev_err_probe(dev, ret,
> + "Failed to get/enable k230 mmc other clocks\n");
> + }
> +
> + if (of_device_is_compatible(dev->of_node, "canaan,k230-sdio")) {
Driver match data is for this.
Best regards,
Krzysztof
On Fri, Feb 06, 2026 at 02:26:40PM +0100, Krzysztof Kozlowski wrote:
> On 04/02/2026 09:29, Jiayu Du wrote:
> > +static int dwcmshc_k230_init(struct device *dev, struct sdhci_host *host,
> > + struct dwcmshc_priv *dwc_priv)
> > +{
> > + static const char * const clk_ids[] = {"base", "timer", "ahb"};
> > + struct device_node *usb_phy_node;
> > + struct k230_priv *k230_priv;
> > + u32 data;
> > + int ret;
> > +
> > + k230_priv = devm_kzalloc(dev, sizeof(struct k230_priv), GFP_KERNEL);
> > + if (!k230_priv)
> > + return -ENOMEM;
> > + dwc_priv->priv = k230_priv;
> > +
> > + usb_phy_node = of_find_compatible_node(NULL, NULL, "canaan,k230-usb-phy");
>
> Hm? You should use phandles, not look for various nodes.
Only one usbphy node has the canaan, k230-usb-phy compatibility.
So in this situation, is it ok to continue using of_find_compatible_node?
> > + if (!usb_phy_node) {
>
> Please follow Linux coding style.
I will fix it in next version.
> > + return dev_err_probe(dev, -ENODEV,
> > + "Failed to find k230-usb-phy node\n");
> > + }
> > +
> > + 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");
> > + }
> > +
> > + ret = dwcmshc_get_enable_other_clks(mmc_dev(host->mmc), dwc_priv,
> > + ARRAY_SIZE(clk_ids), clk_ids);
> > + if (ret) {
> > + return dev_err_probe(dev, ret,
> > + "Failed to get/enable k230 mmc other clocks\n");
> > + }
> > +
> > + if (of_device_is_compatible(dev->of_node, "canaan,k230-sdio")) {
>
> Driver match data is for this.
What you mean is that I shouldn't use of_find_compatible_node, but I can
use device_get_match_data instead? Then I can continue to distinguish
between SDIO and eMMC to do parameter configuration
Or do you mean that I should put the parameters to be adjusted into the
pdata structure? But currently, the dwcmshc structure is not suitable for
containing vendor-specific properties.
Regards,
Jiayu Du
>
> Best regards,
> Krzysztof
On 07/02/2026 09:45, Jiayu Du wrote:
> On Fri, Feb 06, 2026 at 02:26:40PM +0100, Krzysztof Kozlowski wrote:
>> On 04/02/2026 09:29, Jiayu Du wrote:
>>> +static int dwcmshc_k230_init(struct device *dev, struct sdhci_host *host,
>>> + struct dwcmshc_priv *dwc_priv)
>>> +{
>>> + static const char * const clk_ids[] = {"base", "timer", "ahb"};
>>> + struct device_node *usb_phy_node;
>>> + struct k230_priv *k230_priv;
>>> + u32 data;
>>> + int ret;
>>> +
>>> + k230_priv = devm_kzalloc(dev, sizeof(struct k230_priv), GFP_KERNEL);
>>> + if (!k230_priv)
>>> + return -ENOMEM;
>>> + dwc_priv->priv = k230_priv;
>>> +
>>> + usb_phy_node = of_find_compatible_node(NULL, NULL, "canaan,k230-usb-phy");
>>
>> Hm? You should use phandles, not look for various nodes.
>
> Only one usbphy node has the canaan, k230-usb-phy compatibility.
> So in this situation, is it ok to continue using of_find_compatible_node?
Amount of nodes does not matter. This is not how you express
links/dependencies between devices. Phandle is for this. This is wrong
on many levels, including missing device links, bypassing kernel API/layers.
>
>>> + if (!usb_phy_node) {
>>
>> Please follow Linux coding style.
>
> I will fix it in next version.
>
>>> + return dev_err_probe(dev, -ENODEV,
>>> + "Failed to find k230-usb-phy node\n");
>>> + }
>>> +
>>> + 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");
>>> + }
>>> +
>>> + ret = dwcmshc_get_enable_other_clks(mmc_dev(host->mmc), dwc_priv,
>>> + ARRAY_SIZE(clk_ids), clk_ids);
>>> + if (ret) {
>>> + return dev_err_probe(dev, ret,
>>> + "Failed to get/enable k230 mmc other clocks\n");
>>> + }
>>> +
>>> + if (of_device_is_compatible(dev->of_node, "canaan,k230-sdio")) {
>>
>> Driver match data is for this.
>
> What you mean is that I shouldn't use of_find_compatible_node, but I can
> use device_get_match_data instead? Then I can continue to distinguish
> between SDIO and eMMC to do parameter configuration
>
> Or do you mean that I should put the parameters to be adjusted into the
> pdata structure? But currently, the dwcmshc structure is not suitable for
> containing vendor-specific properties.
Parameters should go to driver match data. I already requested this for
some other driver and this has to be fixed.
Best regards,
Krzysztof
On Sat, Feb 07, 2026 at 10:32:55AM +0100, Krzysztof Kozlowski wrote:
> On 07/02/2026 09:45, Jiayu Du wrote:
> > On Fri, Feb 06, 2026 at 02:26:40PM +0100, Krzysztof Kozlowski wrote:
> >> On 04/02/2026 09:29, Jiayu Du wrote:
> >>> +static int dwcmshc_k230_init(struct device *dev, struct sdhci_host *host,
> >>> + struct dwcmshc_priv *dwc_priv)
> >>> +{
> >>> + static const char * const clk_ids[] = {"base", "timer", "ahb"};
> >>> + struct device_node *usb_phy_node;
> >>> + struct k230_priv *k230_priv;
> >>> + u32 data;
> >>> + int ret;
> >>> +
> >>> + k230_priv = devm_kzalloc(dev, sizeof(struct k230_priv), GFP_KERNEL);
> >>> + if (!k230_priv)
> >>> + return -ENOMEM;
> >>> + dwc_priv->priv = k230_priv;
> >>> +
> >>> + usb_phy_node = of_find_compatible_node(NULL, NULL, "canaan,k230-usb-phy");
> >>
> >> Hm? You should use phandles, not look for various nodes.
> >
> > Only one usbphy node has the canaan, k230-usb-phy compatibility.
> > So in this situation, is it ok to continue using of_find_compatible_node?
>
> Amount of nodes does not matter. This is not how you express
> links/dependencies between devices. Phandle is for this. This is wrong
> on many levels, including missing device links, bypassing kernel API/layers.
Thank you for your review. I will fix it.
>
>
> >
> >>> + if (!usb_phy_node) {
> >>
> >> Please follow Linux coding style.
> >
> > I will fix it in next version.
> >
> >>> + return dev_err_probe(dev, -ENODEV,
> >>> + "Failed to find k230-usb-phy node\n");
> >>> + }
> >>> +
> >>> + 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");
> >>> + }
> >>> +
> >>> + ret = dwcmshc_get_enable_other_clks(mmc_dev(host->mmc), dwc_priv,
> >>> + ARRAY_SIZE(clk_ids), clk_ids);
> >>> + if (ret) {
> >>> + return dev_err_probe(dev, ret,
> >>> + "Failed to get/enable k230 mmc other clocks\n");
> >>> + }
> >>> +
> >>> + if (of_device_is_compatible(dev->of_node, "canaan,k230-sdio")) {
> >>
> >> Driver match data is for this.
> >
> > What you mean is that I shouldn't use of_find_compatible_node, but I can
> > use device_get_match_data instead? Then I can continue to distinguish
> > between SDIO and eMMC to do parameter configuration
> >
> > Or do you mean that I should put the parameters to be adjusted into the
> > pdata structure? But currently, the dwcmshc structure is not suitable for
> > containing vendor-specific properties.
>
> Parameters should go to driver match data. I already requested this for
> some other driver and this has to be fixed.
I will make fix to enable dwcmshc_pltfm_data to support the addition of
vendor-specific properties. And if possible, could you give me with some
examples? I would be very grateful.
>
> Best regards,
> Krzysztof
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
© 2016 - 2026 Red Hat, Inc.