* [PATCH] mmc: sdhci-of-dwcmshc: Refactor Rockchip platform data for controller revisions @ 2026-03-27 7:51 Shawn Lin 2026-03-27 9:29 ` Adrian Hunter 0 siblings, 1 reply; 6+ messages in thread From: Shawn Lin @ 2026-03-27 7:51 UTC (permalink / raw) To: Ulf Hansson; +Cc: linux-mmc, linux-rockchip, Adrian Hunter, Shawn Lin The driver previously used an enum (dwcmshc_rk_type) to distinguish platforms like the RK3568 and RK3588 based on DT compatible names. This approach is inflexible, scales poorly for future revisions or mixed-revision platforms, and conflates SoC names with controller revisions. Introduces a new struct rockchip_pltfm_data containing a dedicated revision field. The old enum is removed, and all code paths are updated to use the revision-based data. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/mmc/host/sdhci-of-dwcmshc.c | 87 ++++++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 35 deletions(-) diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c index a91b3e7..6b43ba9 100644 --- a/drivers/mmc/host/sdhci-of-dwcmshc.c +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c @@ -278,14 +278,8 @@ #define PHY_DELAY_CODE_EMMC 0x17 #define PHY_DELAY_CODE_SD 0x55 -enum dwcmshc_rk_type { - DWCMSHC_RK3568, - DWCMSHC_RK3588, -}; - struct rk35xx_priv { struct reset_control *reset; - enum dwcmshc_rk_type devtype; u8 txclk_tapnum; }; @@ -330,6 +324,11 @@ struct k230_pltfm_data { u32 write_prot_bit; }; +struct rockchip_pltfm_data { + struct dwcmshc_pltfm_data dwcmshc_pdata; + int revision; +}; + static void dwcmshc_enable_card_clk(struct sdhci_host *host) { u16 ctrl; @@ -749,6 +748,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host); struct rk35xx_priv *priv = dwc_priv->priv; + const struct rockchip_pltfm_data *rockchip_pdata = to_pltfm_data(dwc_priv, rockchip); u8 txclk_tapnum = DLL_TXCLK_TAPNUM_DEFAULT; u32 extra, reg; int err; @@ -816,7 +816,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock * we must set it in higher speed mode. */ extra = DWCMSHC_EMMC_DLL_DLYENA; - if (priv->devtype == DWCMSHC_RK3568) + if (rockchip_pdata->revision == 0) extra |= DLL_RXCLK_NO_INVERTER << DWCMSHC_EMMC_DLL_RXCLK_SRCSEL; sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_RXCLK); @@ -842,7 +842,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock host->mmc->ios.timing == MMC_TIMING_MMC_HS400) txclk_tapnum = priv->txclk_tapnum; - if ((priv->devtype == DWCMSHC_RK3588) && host->mmc->ios.timing == MMC_TIMING_MMC_HS400) { + if ((rockchip_pdata->revision == 1) && host->mmc->ios.timing == MMC_TIMING_MMC_HS400) { txclk_tapnum = DLL_TXCLK_TAPNUM_90_DEGREES; extra = DLL_CMDOUT_SRC_CLK_NEG | @@ -898,11 +898,6 @@ static int dwcmshc_rk35xx_init(struct device *dev, struct sdhci_host *host, if (!priv) return -ENOMEM; - if (of_device_is_compatible(dev->of_node, "rockchip,rk3588-dwcmshc")) - priv->devtype = DWCMSHC_RK3588; - else - priv->devtype = DWCMSHC_RK3568; - priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc)); if (IS_ERR(priv->reset)) { err = PTR_ERR(priv->reset); @@ -2115,30 +2110,52 @@ static const struct cqhci_host_ops rk35xx_cqhci_ops = { .set_tran_desc = dwcmshc_set_tran_desc, }; -static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk35xx_pdata = { - .pdata = { - .ops = &sdhci_dwcmshc_rk35xx_ops, - .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | - SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, - .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3568_pdata = { + .dwcmshc_pdata = { + .pdata = { + .ops = &sdhci_dwcmshc_rk35xx_ops, + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, + }, + .cqhci_host_ops = &rk35xx_cqhci_ops, + .init = dwcmshc_rk35xx_init, + .postinit = dwcmshc_rk35xx_postinit, }, - .cqhci_host_ops = &rk35xx_cqhci_ops, - .init = dwcmshc_rk35xx_init, - .postinit = dwcmshc_rk35xx_postinit, + .revision = 0, }; -static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk3576_pdata = { - .pdata = { - .ops = &sdhci_dwcmshc_rk35xx_ops, - .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | - SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, - .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3588_pdata = { + .dwcmshc_pdata = { + .pdata = { + .ops = &sdhci_dwcmshc_rk35xx_ops, + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, + }, + .cqhci_host_ops = &rk35xx_cqhci_ops, + .init = dwcmshc_rk35xx_init, + .postinit = dwcmshc_rk35xx_postinit, + }, + .revision = 1, +}; + +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3576_pdata = { + .dwcmshc_pdata = { + .pdata = { + .ops = &sdhci_dwcmshc_rk35xx_ops, + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, + }, + .cqhci_host_ops = &rk35xx_cqhci_ops, + .init = dwcmshc_rk35xx_init, + .postinit = dwcmshc_rk3576_postinit, }, - .cqhci_host_ops = &rk35xx_cqhci_ops, - .init = dwcmshc_rk35xx_init, - .postinit = dwcmshc_rk3576_postinit, + .revision = 1, }; static const struct dwcmshc_pltfm_data sdhci_dwcmshc_th1520_pdata = { @@ -2309,7 +2326,7 @@ static const struct of_device_id sdhci_dwcmshc_dt_ids[] = { }, { .compatible = "rockchip,rk3588-dwcmshc", - .data = &sdhci_dwcmshc_rk35xx_pdata, + .data = &sdhci_dwcmshc_rk3588_pdata, }, { .compatible = "rockchip,rk3576-dwcmshc", @@ -2317,7 +2334,7 @@ static const struct of_device_id sdhci_dwcmshc_dt_ids[] = { }, { .compatible = "rockchip,rk3568-dwcmshc", - .data = &sdhci_dwcmshc_rk35xx_pdata, + .data = &sdhci_dwcmshc_rk3568_pdata, }, { .compatible = "snps,dwcmshc-sdhci", -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: sdhci-of-dwcmshc: Refactor Rockchip platform data for controller revisions 2026-03-27 7:51 [PATCH] mmc: sdhci-of-dwcmshc: Refactor Rockchip platform data for controller revisions Shawn Lin @ 2026-03-27 9:29 ` Adrian Hunter 2026-03-27 11:28 ` Shawn Lin 0 siblings, 1 reply; 6+ messages in thread From: Adrian Hunter @ 2026-03-27 9:29 UTC (permalink / raw) To: Shawn Lin; +Cc: linux-mmc, linux-rockchip, Adrian Hunter On 27/03/2026 09:51, Shawn Lin wrote: > The driver previously used an enum (dwcmshc_rk_type) to distinguish > platforms like the RK3568 and RK3588 based on DT compatible names. > This approach is inflexible, scales poorly for future revisions or > mixed-revision platforms, and conflates SoC names with controller > revisions. > > Introduces a new struct rockchip_pltfm_data containing a dedicated > revision field. The old enum is removed, and all code paths are > updated to use the revision-based data. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > --- > > drivers/mmc/host/sdhci-of-dwcmshc.c | 87 ++++++++++++++++++++++--------------- > 1 file changed, 52 insertions(+), 35 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c > index a91b3e7..6b43ba9 100644 > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c > @@ -278,14 +278,8 @@ > #define PHY_DELAY_CODE_EMMC 0x17 > #define PHY_DELAY_CODE_SD 0x55 > > -enum dwcmshc_rk_type { > - DWCMSHC_RK3568, > - DWCMSHC_RK3588, > -}; > - > struct rk35xx_priv { > struct reset_control *reset; > - enum dwcmshc_rk_type devtype; > u8 txclk_tapnum; > }; > > @@ -330,6 +324,11 @@ struct k230_pltfm_data { > u32 write_prot_bit; > }; > > +struct rockchip_pltfm_data { > + struct dwcmshc_pltfm_data dwcmshc_pdata; > + int revision; Does this "revision" match HW specs, otherwise maybe need to clarify where it comes from. > +}; > + > static void dwcmshc_enable_card_clk(struct sdhci_host *host) > { > u16 ctrl; > @@ -749,6 +748,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock > struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); > struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host); > struct rk35xx_priv *priv = dwc_priv->priv; > + const struct rockchip_pltfm_data *rockchip_pdata = to_pltfm_data(dwc_priv, rockchip); > u8 txclk_tapnum = DLL_TXCLK_TAPNUM_DEFAULT; > u32 extra, reg; > int err; > @@ -816,7 +816,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock > * we must set it in higher speed mode. > */ > extra = DWCMSHC_EMMC_DLL_DLYENA; > - if (priv->devtype == DWCMSHC_RK3568) > + if (rockchip_pdata->revision == 0) > extra |= DLL_RXCLK_NO_INVERTER << DWCMSHC_EMMC_DLL_RXCLK_SRCSEL; > sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_RXCLK); > > @@ -842,7 +842,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock > host->mmc->ios.timing == MMC_TIMING_MMC_HS400) > txclk_tapnum = priv->txclk_tapnum; > > - if ((priv->devtype == DWCMSHC_RK3588) && host->mmc->ios.timing == MMC_TIMING_MMC_HS400) { > + if ((rockchip_pdata->revision == 1) && host->mmc->ios.timing == MMC_TIMING_MMC_HS400) { > txclk_tapnum = DLL_TXCLK_TAPNUM_90_DEGREES; > > extra = DLL_CMDOUT_SRC_CLK_NEG | > @@ -898,11 +898,6 @@ static int dwcmshc_rk35xx_init(struct device *dev, struct sdhci_host *host, > if (!priv) > return -ENOMEM; > > - if (of_device_is_compatible(dev->of_node, "rockchip,rk3588-dwcmshc")) > - priv->devtype = DWCMSHC_RK3588; > - else > - priv->devtype = DWCMSHC_RK3568; > - > priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc)); > if (IS_ERR(priv->reset)) { > err = PTR_ERR(priv->reset); > @@ -2115,30 +2110,52 @@ static const struct cqhci_host_ops rk35xx_cqhci_ops = { > .set_tran_desc = dwcmshc_set_tran_desc, > }; > > -static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk35xx_pdata = { > - .pdata = { > - .ops = &sdhci_dwcmshc_rk35xx_ops, > - .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | > - SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, > - .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | > - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, > +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3568_pdata = { > + .dwcmshc_pdata = { > + .pdata = { > + .ops = &sdhci_dwcmshc_rk35xx_ops, > + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | > + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, > + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | > + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, > + }, > + .cqhci_host_ops = &rk35xx_cqhci_ops, > + .init = dwcmshc_rk35xx_init, > + .postinit = dwcmshc_rk35xx_postinit, > }, > - .cqhci_host_ops = &rk35xx_cqhci_ops, > - .init = dwcmshc_rk35xx_init, > - .postinit = dwcmshc_rk35xx_postinit, > + .revision = 0, > }; > > -static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk3576_pdata = { > - .pdata = { > - .ops = &sdhci_dwcmshc_rk35xx_ops, > - .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | > - SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, > - .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | > - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, > +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3588_pdata = { Better order: sdhci_dwcmshc_rk3568_pdata, sdhci_dwcmshc_rk3576_pdata, sdhci_dwcmshc_rk3588_pdata > + .dwcmshc_pdata = { > + .pdata = { > + .ops = &sdhci_dwcmshc_rk35xx_ops, > + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | > + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, > + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | > + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, > + }, > + .cqhci_host_ops = &rk35xx_cqhci_ops, > + .init = dwcmshc_rk35xx_init, > + .postinit = dwcmshc_rk35xx_postinit, > + }, > + .revision = 1, > +}; > + > +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3576_pdata = { > + .dwcmshc_pdata = { > + .pdata = { > + .ops = &sdhci_dwcmshc_rk35xx_ops, > + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | > + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, > + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | > + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, > + }, > + .cqhci_host_ops = &rk35xx_cqhci_ops, > + .init = dwcmshc_rk35xx_init, > + .postinit = dwcmshc_rk3576_postinit, > }, > - .cqhci_host_ops = &rk35xx_cqhci_ops, > - .init = dwcmshc_rk35xx_init, > - .postinit = dwcmshc_rk3576_postinit, > + .revision = 1, Isn't rk3576 revision = 0 ? > }; > > static const struct dwcmshc_pltfm_data sdhci_dwcmshc_th1520_pdata = { > @@ -2309,7 +2326,7 @@ static const struct of_device_id sdhci_dwcmshc_dt_ids[] = { > }, > { > .compatible = "rockchip,rk3588-dwcmshc", > - .data = &sdhci_dwcmshc_rk35xx_pdata, > + .data = &sdhci_dwcmshc_rk3588_pdata, > }, > { > .compatible = "rockchip,rk3576-dwcmshc", > @@ -2317,7 +2334,7 @@ static const struct of_device_id sdhci_dwcmshc_dt_ids[] = { > }, > { > .compatible = "rockchip,rk3568-dwcmshc", > - .data = &sdhci_dwcmshc_rk35xx_pdata, > + .data = &sdhci_dwcmshc_rk3568_pdata, > }, > { > .compatible = "snps,dwcmshc-sdhci", ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: sdhci-of-dwcmshc: Refactor Rockchip platform data for controller revisions 2026-03-27 9:29 ` Adrian Hunter @ 2026-03-27 11:28 ` Shawn Lin 2026-03-27 11:34 ` Adrian Hunter 0 siblings, 1 reply; 6+ messages in thread From: Shawn Lin @ 2026-03-27 11:28 UTC (permalink / raw) To: Adrian Hunter; +Cc: shawn.lin, linux-mmc, linux-rockchip Hi Adrian 在 2026/03/27 星期五 17:29, Adrian Hunter 写道: > On 27/03/2026 09:51, Shawn Lin wrote: >> The driver previously used an enum (dwcmshc_rk_type) to distinguish >> platforms like the RK3568 and RK3588 based on DT compatible names. >> This approach is inflexible, scales poorly for future revisions or >> mixed-revision platforms, and conflates SoC names with controller >> revisions. >> >> Introduces a new struct rockchip_pltfm_data containing a dedicated >> revision field. The old enum is removed, and all code paths are >> updated to use the revision-based data. >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> --- >> >> drivers/mmc/host/sdhci-of-dwcmshc.c | 87 ++++++++++++++++++++++--------------- >> 1 file changed, 52 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c >> index a91b3e7..6b43ba9 100644 >> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c >> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c >> @@ -278,14 +278,8 @@ >> #define PHY_DELAY_CODE_EMMC 0x17 >> #define PHY_DELAY_CODE_SD 0x55 >> >> -enum dwcmshc_rk_type { >> - DWCMSHC_RK3568, >> - DWCMSHC_RK3588, >> -}; >> - >> struct rk35xx_priv { >> struct reset_control *reset; >> - enum dwcmshc_rk_type devtype; >> u8 txclk_tapnum; >> }; >> >> @@ -330,6 +324,11 @@ struct k230_pltfm_data { >> u32 write_prot_bit; >> }; >> >> +struct rockchip_pltfm_data { >> + struct dwcmshc_pltfm_data dwcmshc_pdata; >> + int revision; > > Does this "revision" match HW specs, otherwise maybe need > to clarify where it comes from. > This "revision" originates from our internal IP development documentation, not the public chip specifications. The public manuals do not label these specific IP versions. Revision 0 (Initial IP): This is the initial version of the IP used in the RK3568 and RK3566. Post-silicon validation identified certain errata requiring workarounds, and this version lacks HS400 support. Revision 1 (Fixed IP): This is the updated IP version (with documentation number updated internally). It fixes the previous issues and adds HS400 support. This revision is used in all subsequent chips (RK3576, RK3588, etc.). Since the public specs do not expose this distinction, I believe using the internal IP development document revision is the most accurate way to differentiate the hardware behavior in software. I will add a comment in the code to clarify this origin and the specific differences between the revisions. >> +}; >> + >> static void dwcmshc_enable_card_clk(struct sdhci_host *host) >> { >> u16 ctrl; >> @@ -749,6 +748,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock >> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >> struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host); >> struct rk35xx_priv *priv = dwc_priv->priv; >> + const struct rockchip_pltfm_data *rockchip_pdata = to_pltfm_data(dwc_priv, rockchip); >> u8 txclk_tapnum = DLL_TXCLK_TAPNUM_DEFAULT; >> u32 extra, reg; >> int err; >> @@ -816,7 +816,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock >> * we must set it in higher speed mode. >> */ >> extra = DWCMSHC_EMMC_DLL_DLYENA; >> - if (priv->devtype == DWCMSHC_RK3568) >> + if (rockchip_pdata->revision == 0) >> extra |= DLL_RXCLK_NO_INVERTER << DWCMSHC_EMMC_DLL_RXCLK_SRCSEL; >> sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_RXCLK); >> >> @@ -842,7 +842,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock >> host->mmc->ios.timing == MMC_TIMING_MMC_HS400) >> txclk_tapnum = priv->txclk_tapnum; >> >> - if ((priv->devtype == DWCMSHC_RK3588) && host->mmc->ios.timing == MMC_TIMING_MMC_HS400) { >> + if ((rockchip_pdata->revision == 1) && host->mmc->ios.timing == MMC_TIMING_MMC_HS400) { >> txclk_tapnum = DLL_TXCLK_TAPNUM_90_DEGREES; >> >> extra = DLL_CMDOUT_SRC_CLK_NEG | >> @@ -898,11 +898,6 @@ static int dwcmshc_rk35xx_init(struct device *dev, struct sdhci_host *host, >> if (!priv) >> return -ENOMEM; >> >> - if (of_device_is_compatible(dev->of_node, "rockchip,rk3588-dwcmshc")) >> - priv->devtype = DWCMSHC_RK3588; >> - else >> - priv->devtype = DWCMSHC_RK3568; >> - >> priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc)); >> if (IS_ERR(priv->reset)) { >> err = PTR_ERR(priv->reset); >> @@ -2115,30 +2110,52 @@ static const struct cqhci_host_ops rk35xx_cqhci_ops = { >> .set_tran_desc = dwcmshc_set_tran_desc, >> }; >> >> -static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk35xx_pdata = { >> - .pdata = { >> - .ops = &sdhci_dwcmshc_rk35xx_ops, >> - .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >> - SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >> - .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >> +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3568_pdata = { >> + .dwcmshc_pdata = { >> + .pdata = { >> + .ops = &sdhci_dwcmshc_rk35xx_ops, >> + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >> + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >> + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >> + }, >> + .cqhci_host_ops = &rk35xx_cqhci_ops, >> + .init = dwcmshc_rk35xx_init, >> + .postinit = dwcmshc_rk35xx_postinit, >> }, >> - .cqhci_host_ops = &rk35xx_cqhci_ops, >> - .init = dwcmshc_rk35xx_init, >> - .postinit = dwcmshc_rk35xx_postinit, >> + .revision = 0, >> }; >> >> -static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk3576_pdata = { >> - .pdata = { >> - .ops = &sdhci_dwcmshc_rk35xx_ops, >> - .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >> - SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >> - .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >> +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3588_pdata = { > > Better order: sdhci_dwcmshc_rk3568_pdata, sdhci_dwcmshc_rk3576_pdata, > sdhci_dwcmshc_rk3588_pdata Sure， will do. > >> + .dwcmshc_pdata = { >> + .pdata = { >> + .ops = &sdhci_dwcmshc_rk35xx_ops, >> + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >> + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >> + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >> + }, >> + .cqhci_host_ops = &rk35xx_cqhci_ops, >> + .init = dwcmshc_rk35xx_init, >> + .postinit = dwcmshc_rk35xx_postinit, >> + }, >> + .revision = 1, >> +}; >> + >> +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3576_pdata = { >> + .dwcmshc_pdata = { >> + .pdata = { >> + .ops = &sdhci_dwcmshc_rk35xx_ops, >> + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >> + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >> + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >> + }, >> + .cqhci_host_ops = &rk35xx_cqhci_ops, >> + .init = dwcmshc_rk35xx_init, >> + .postinit = dwcmshc_rk3576_postinit, >> }, >> - .cqhci_host_ops = &rk35xx_cqhci_ops, >> - .init = dwcmshc_rk35xx_init, >> - .postinit = dwcmshc_rk3576_postinit, >> + .revision = 1, > > Isn't rk3576 revision = 0 ? As explained above, rk3576 is revision 1. > >> }; >> >> static const struct dwcmshc_pltfm_data sdhci_dwcmshc_th1520_pdata = { >> @@ -2309,7 +2326,7 @@ static const struct of_device_id sdhci_dwcmshc_dt_ids[] = { >> }, >> { >> .compatible = "rockchip,rk3588-dwcmshc", >> - .data = &sdhci_dwcmshc_rk35xx_pdata, >> + .data = &sdhci_dwcmshc_rk3588_pdata, >> }, >> { >> .compatible = "rockchip,rk3576-dwcmshc", >> @@ -2317,7 +2334,7 @@ static const struct of_device_id sdhci_dwcmshc_dt_ids[] = { >> }, >> { >> .compatible = "rockchip,rk3568-dwcmshc", >> - .data = &sdhci_dwcmshc_rk35xx_pdata, >> + .data = &sdhci_dwcmshc_rk3568_pdata, >> }, >> { >> .compatible = "snps,dwcmshc-sdhci", > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: sdhci-of-dwcmshc: Refactor Rockchip platform data for controller revisions 2026-03-27 11:28 ` Shawn Lin @ 2026-03-27 11:34 ` Adrian Hunter 2026-03-27 13:33 ` Shawn Lin 0 siblings, 1 reply; 6+ messages in thread From: Adrian Hunter @ 2026-03-27 11:34 UTC (permalink / raw) To: Shawn Lin; +Cc: linux-mmc, linux-rockchip On 27/03/2026 13:28, Shawn Lin wrote: > Hi Adrian > > 在 2026/03/27 星期五 17:29, Adrian Hunter 写道: >> On 27/03/2026 09:51, Shawn Lin wrote: >>> The driver previously used an enum (dwcmshc_rk_type) to distinguish >>> platforms like the RK3568 and RK3588 based on DT compatible names. >>> This approach is inflexible, scales poorly for future revisions or >>> mixed-revision platforms, and conflates SoC names with controller >>> revisions. >>> >>> Introduces a new struct rockchip_pltfm_data containing a dedicated >>> revision field. The old enum is removed, and all code paths are >>> updated to use the revision-based data. >>> >>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >>> --- >>> >>> drivers/mmc/host/sdhci-of-dwcmshc.c | 87 ++++++++++++++++++++++--------------- >>> 1 file changed, 52 insertions(+), 35 deletions(-) >>> >>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c >>> index a91b3e7..6b43ba9 100644 >>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c >>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c >>> @@ -278,14 +278,8 @@ >>> #define PHY_DELAY_CODE_EMMC 0x17 >>> #define PHY_DELAY_CODE_SD 0x55 >>> -enum dwcmshc_rk_type { >>> - DWCMSHC_RK3568, >>> - DWCMSHC_RK3588, >>> -}; >>> - >>> struct rk35xx_priv { >>> struct reset_control *reset; >>> - enum dwcmshc_rk_type devtype; >>> u8 txclk_tapnum; >>> }; >>> @@ -330,6 +324,11 @@ struct k230_pltfm_data { >>> u32 write_prot_bit; >>> }; >>> +struct rockchip_pltfm_data { >>> + struct dwcmshc_pltfm_data dwcmshc_pdata; >>> + int revision; >> >> Does this "revision" match HW specs, otherwise maybe need >> to clarify where it comes from. >> > > This "revision" originates from our internal IP development > documentation, not the public chip specifications. The public manuals do not label these specific IP versions. > > Revision 0 (Initial IP): This is the initial version of the IP used in > the RK3568 and RK3566. Post-silicon validation identified certain errata > requiring workarounds, and this version lacks HS400 support. > > Revision 1 (Fixed IP): This is the updated IP version (with > documentation number updated internally). It fixes the previous issues > and adds HS400 support. This revision is used in all subsequent chips > (RK3576, RK3588, etc.). > > Since the public specs do not expose this distinction, I believe using > the internal IP development document revision is the most accurate way > to differentiate the hardware behavior in software. I will add a comment > in the code to clarify this origin and the specific differences between > the revisions. > >>> +}; >>> + >>> static void dwcmshc_enable_card_clk(struct sdhci_host *host) >>> { >>> u16 ctrl; >>> @@ -749,6 +748,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock >>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>> struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host); >>> struct rk35xx_priv *priv = dwc_priv->priv; >>> + const struct rockchip_pltfm_data *rockchip_pdata = to_pltfm_data(dwc_priv, rockchip); >>> u8 txclk_tapnum = DLL_TXCLK_TAPNUM_DEFAULT; >>> u32 extra, reg; >>> int err; >>> @@ -816,7 +816,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock >>> * we must set it in higher speed mode. >>> */ >>> extra = DWCMSHC_EMMC_DLL_DLYENA; >>> - if (priv->devtype == DWCMSHC_RK3568) >>> + if (rockchip_pdata->revision == 0) >>> extra |= DLL_RXCLK_NO_INVERTER << DWCMSHC_EMMC_DLL_RXCLK_SRCSEL; >>> sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_RXCLK); >>> @@ -842,7 +842,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock >>> host->mmc->ios.timing == MMC_TIMING_MMC_HS400) >>> txclk_tapnum = priv->txclk_tapnum; >>> - if ((priv->devtype == DWCMSHC_RK3588) && host->mmc->ios.timing == MMC_TIMING_MMC_HS400) { >>> + if ((rockchip_pdata->revision == 1) && host->mmc->ios.timing == MMC_TIMING_MMC_HS400) { >>> txclk_tapnum = DLL_TXCLK_TAPNUM_90_DEGREES; >>> extra = DLL_CMDOUT_SRC_CLK_NEG | >>> @@ -898,11 +898,6 @@ static int dwcmshc_rk35xx_init(struct device *dev, struct sdhci_host *host, >>> if (!priv) >>> return -ENOMEM; >>> - if (of_device_is_compatible(dev->of_node, "rockchip,rk3588-dwcmshc")) >>> - priv->devtype = DWCMSHC_RK3588; >>> - else >>> - priv->devtype = DWCMSHC_RK3568; >>> - >>> priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc)); >>> if (IS_ERR(priv->reset)) { >>> err = PTR_ERR(priv->reset); >>> @@ -2115,30 +2110,52 @@ static const struct cqhci_host_ops rk35xx_cqhci_ops = { >>> .set_tran_desc = dwcmshc_set_tran_desc, >>> }; >>> -static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk35xx_pdata = { >>> - .pdata = { >>> - .ops = &sdhci_dwcmshc_rk35xx_ops, >>> - .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >>> - SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >>> - .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >>> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >>> +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3568_pdata = { >>> + .dwcmshc_pdata = { >>> + .pdata = { >>> + .ops = &sdhci_dwcmshc_rk35xx_ops, >>> + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >>> + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >>> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >>> + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >>> + }, >>> + .cqhci_host_ops = &rk35xx_cqhci_ops, >>> + .init = dwcmshc_rk35xx_init, >>> + .postinit = dwcmshc_rk35xx_postinit, >>> }, >>> - .cqhci_host_ops = &rk35xx_cqhci_ops, >>> - .init = dwcmshc_rk35xx_init, >>> - .postinit = dwcmshc_rk35xx_postinit, >>> + .revision = 0, >>> }; >>> -static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk3576_pdata = { >>> - .pdata = { >>> - .ops = &sdhci_dwcmshc_rk35xx_ops, >>> - .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >>> - SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >>> - .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >>> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >>> +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3588_pdata = { >> >> Better order: sdhci_dwcmshc_rk3568_pdata, sdhci_dwcmshc_rk3576_pdata, >> sdhci_dwcmshc_rk3588_pdata > > > Sure， will do. > >> >>> + .dwcmshc_pdata = { >>> + .pdata = { >>> + .ops = &sdhci_dwcmshc_rk35xx_ops, >>> + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >>> + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >>> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >>> + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >>> + }, >>> + .cqhci_host_ops = &rk35xx_cqhci_ops, >>> + .init = dwcmshc_rk35xx_init, >>> + .postinit = dwcmshc_rk35xx_postinit, >>> + }, >>> + .revision = 1, >>> +}; >>> + >>> +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3576_pdata = { >>> + .dwcmshc_pdata = { >>> + .pdata = { >>> + .ops = &sdhci_dwcmshc_rk35xx_ops, >>> + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >>> + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >>> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >>> + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >>> + }, >>> + .cqhci_host_ops = &rk35xx_cqhci_ops, >>> + .init = dwcmshc_rk35xx_init, >>> + .postinit = dwcmshc_rk3576_postinit, >>> }, >>> - .cqhci_host_ops = &rk35xx_cqhci_ops, >>> - .init = dwcmshc_rk35xx_init, >>> - .postinit = dwcmshc_rk3576_postinit, >>> + .revision = 1, >> >> Isn't rk3576 revision = 0 ? > > As explained above, rk3576 is revision 1. Ok, but was it behaving that way before, since the code was: if (of_device_is_compatible(dev->of_node, "rockchip,rk3588-dwcmshc")) priv->devtype = DWCMSHC_RK3588; else priv->devtype = DWCMSHC_RK3568; > >> >>> }; >>> static const struct dwcmshc_pltfm_data sdhci_dwcmshc_th1520_pdata = { >>> @@ -2309,7 +2326,7 @@ static const struct of_device_id sdhci_dwcmshc_dt_ids[] = { >>> }, >>> { >>> .compatible = "rockchip,rk3588-dwcmshc", >>> - .data = &sdhci_dwcmshc_rk35xx_pdata, >>> + .data = &sdhci_dwcmshc_rk3588_pdata, >>> }, >>> { >>> .compatible = "rockchip,rk3576-dwcmshc", >>> @@ -2317,7 +2334,7 @@ static const struct of_device_id sdhci_dwcmshc_dt_ids[] = { >>> }, >>> { >>> .compatible = "rockchip,rk3568-dwcmshc", >>> - .data = &sdhci_dwcmshc_rk35xx_pdata, >>> + .data = &sdhci_dwcmshc_rk3568_pdata, >>> }, >>> { >>> .compatible = "snps,dwcmshc-sdhci", >> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: sdhci-of-dwcmshc: Refactor Rockchip platform data for controller revisions 2026-03-27 11:34 ` Adrian Hunter @ 2026-03-27 13:33 ` Shawn Lin 2026-03-27 13:43 ` Adrian Hunter 0 siblings, 1 reply; 6+ messages in thread From: Shawn Lin @ 2026-03-27 13:33 UTC (permalink / raw) To: Adrian Hunter; +Cc: shawn.lin, linux-mmc, linux-rockchip, Ulf Hansson 在 2026/03/27 星期五 19:34, Adrian Hunter 写道: > On 27/03/2026 13:28, Shawn Lin wrote: >> Hi Adrian >> >> 在 2026/03/27 星期五 17:29, Adrian Hunter 写道: >>> On 27/03/2026 09:51, Shawn Lin wrote: >>>> The driver previously used an enum (dwcmshc_rk_type) to distinguish >>>> platforms like the RK3568 and RK3588 based on DT compatible names. >>>> This approach is inflexible, scales poorly for future revisions or >>>> mixed-revision platforms, and conflates SoC names with controller >>>> revisions. >>>> >>>> Introduces a new struct rockchip_pltfm_data containing a dedicated >>>> revision field. The old enum is removed, and all code paths are >>>> updated to use the revision-based data. >>>> >>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >>>> --- >>>> >>>> drivers/mmc/host/sdhci-of-dwcmshc.c | 87 ++++++++++++++++++++++--------------- >>>> 1 file changed, 52 insertions(+), 35 deletions(-) >>>> >>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c >>>> index a91b3e7..6b43ba9 100644 >>>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c >>>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c >>>> @@ -278,14 +278,8 @@ >>>> #define PHY_DELAY_CODE_EMMC 0x17 >>>> #define PHY_DELAY_CODE_SD 0x55 >>>> -enum dwcmshc_rk_type { >>>> - DWCMSHC_RK3568, >>>> - DWCMSHC_RK3588, >>>> -}; >>>> - >>>> struct rk35xx_priv { >>>> struct reset_control *reset; >>>> - enum dwcmshc_rk_type devtype; >>>> u8 txclk_tapnum; >>>> }; >>>> @@ -330,6 +324,11 @@ struct k230_pltfm_data { >>>> u32 write_prot_bit; >>>> }; >>>> +struct rockchip_pltfm_data { >>>> + struct dwcmshc_pltfm_data dwcmshc_pdata; >>>> + int revision; >>> >>> Does this "revision" match HW specs, otherwise maybe need >>> to clarify where it comes from. >>> >> >> This "revision" originates from our internal IP development >> documentation, not the public chip specifications. The public manuals do not label these specific IP versions. >> >> Revision 0 (Initial IP): This is the initial version of the IP used in >> the RK3568 and RK3566. Post-silicon validation identified certain errata >> requiring workarounds, and this version lacks HS400 support. >> >> Revision 1 (Fixed IP): This is the updated IP version (with >> documentation number updated internally). It fixes the previous issues >> and adds HS400 support. This revision is used in all subsequent chips >> (RK3576, RK3588, etc.). >> >> Since the public specs do not expose this distinction, I believe using >> the internal IP development document revision is the most accurate way >> to differentiate the hardware behavior in software. I will add a comment >> in the code to clarify this origin and the specific differences between >> the revisions. >> >>>> +}; >>>> + >>>> static void dwcmshc_enable_card_clk(struct sdhci_host *host) >>>> { >>>> u16 ctrl; >>>> @@ -749,6 +748,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock >>>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>> struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host); >>>> struct rk35xx_priv *priv = dwc_priv->priv; >>>> + const struct rockchip_pltfm_data *rockchip_pdata = to_pltfm_data(dwc_priv, rockchip); >>>> u8 txclk_tapnum = DLL_TXCLK_TAPNUM_DEFAULT; >>>> u32 extra, reg; >>>> int err; >>>> @@ -816,7 +816,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock >>>> * we must set it in higher speed mode. >>>> */ >>>> extra = DWCMSHC_EMMC_DLL_DLYENA; >>>> - if (priv->devtype == DWCMSHC_RK3568) >>>> + if (rockchip_pdata->revision == 0) >>>> extra |= DLL_RXCLK_NO_INVERTER << DWCMSHC_EMMC_DLL_RXCLK_SRCSEL; >>>> sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_RXCLK); >>>> @@ -842,7 +842,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock >>>> host->mmc->ios.timing == MMC_TIMING_MMC_HS400) >>>> txclk_tapnum = priv->txclk_tapnum; >>>> - if ((priv->devtype == DWCMSHC_RK3588) && host->mmc->ios.timing == MMC_TIMING_MMC_HS400) { >>>> + if ((rockchip_pdata->revision == 1) && host->mmc->ios.timing == MMC_TIMING_MMC_HS400) { >>>> txclk_tapnum = DLL_TXCLK_TAPNUM_90_DEGREES; >>>> extra = DLL_CMDOUT_SRC_CLK_NEG | >>>> @@ -898,11 +898,6 @@ static int dwcmshc_rk35xx_init(struct device *dev, struct sdhci_host *host, >>>> if (!priv) >>>> return -ENOMEM; >>>> - if (of_device_is_compatible(dev->of_node, "rockchip,rk3588-dwcmshc")) >>>> - priv->devtype = DWCMSHC_RK3588; >>>> - else >>>> - priv->devtype = DWCMSHC_RK3568; >>>> - >>>> priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc)); >>>> if (IS_ERR(priv->reset)) { >>>> err = PTR_ERR(priv->reset); >>>> @@ -2115,30 +2110,52 @@ static const struct cqhci_host_ops rk35xx_cqhci_ops = { >>>> .set_tran_desc = dwcmshc_set_tran_desc, >>>> }; >>>> -static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk35xx_pdata = { >>>> - .pdata = { >>>> - .ops = &sdhci_dwcmshc_rk35xx_ops, >>>> - .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >>>> - SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >>>> - .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >>>> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >>>> +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3568_pdata = { >>>> + .dwcmshc_pdata = { >>>> + .pdata = { >>>> + .ops = &sdhci_dwcmshc_rk35xx_ops, >>>> + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >>>> + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >>>> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >>>> + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >>>> + }, >>>> + .cqhci_host_ops = &rk35xx_cqhci_ops, >>>> + .init = dwcmshc_rk35xx_init, >>>> + .postinit = dwcmshc_rk35xx_postinit, >>>> }, >>>> - .cqhci_host_ops = &rk35xx_cqhci_ops, >>>> - .init = dwcmshc_rk35xx_init, >>>> - .postinit = dwcmshc_rk35xx_postinit, >>>> + .revision = 0, >>>> }; >>>> -static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk3576_pdata = { >>>> - .pdata = { >>>> - .ops = &sdhci_dwcmshc_rk35xx_ops, >>>> - .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >>>> - SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >>>> - .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >>>> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >>>> +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3588_pdata = { >>> >>> Better order: sdhci_dwcmshc_rk3568_pdata, sdhci_dwcmshc_rk3576_pdata, >>> sdhci_dwcmshc_rk3588_pdata >> >> >> Sure， will do. >> >>> >>>> + .dwcmshc_pdata = { >>>> + .pdata = { >>>> + .ops = &sdhci_dwcmshc_rk35xx_ops, >>>> + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >>>> + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >>>> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >>>> + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >>>> + }, >>>> + .cqhci_host_ops = &rk35xx_cqhci_ops, >>>> + .init = dwcmshc_rk35xx_init, >>>> + .postinit = dwcmshc_rk35xx_postinit, >>>> + }, >>>> + .revision = 1, >>>> +}; >>>> + >>>> +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3576_pdata = { >>>> + .dwcmshc_pdata = { >>>> + .pdata = { >>>> + .ops = &sdhci_dwcmshc_rk35xx_ops, >>>> + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >>>> + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >>>> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >>>> + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >>>> + }, >>>> + .cqhci_host_ops = &rk35xx_cqhci_ops, >>>> + .init = dwcmshc_rk35xx_init, >>>> + .postinit = dwcmshc_rk3576_postinit, >>>> }, >>>> - .cqhci_host_ops = &rk35xx_cqhci_ops, >>>> - .init = dwcmshc_rk35xx_init, >>>> - .postinit = dwcmshc_rk3576_postinit, >>>> + .revision = 1, >>> >>> Isn't rk3576 revision = 0 ? >> >> As explained above, rk3576 is revision 1. > > Ok, but was it behaving that way before, since the code was: > > if (of_device_is_compatible(dev->of_node, "rockchip,rk3588-dwcmshc")) > priv->devtype = DWCMSHC_RK3588; > else > priv->devtype = DWCMSHC_RK3568; > Yes, you are absolutely right to be confused，the old code logic was indeed the root of the problem, which is why this cleanup is necessary. As you can see in the Device Tree [1], RK3576 explicitly lists "rockchip,rk3588-dwcmshc" as a secondary compatible string to select DWCMSHC_RK3588(now revision 1) as the devtype. [1] https://elixir.bootlin.com/linux/v7.0-rc5/source/arch/arm64/boot/dts/rockchip/rk3576.dtsi#L1922 >> >>> >>>> }; >>>> static const struct dwcmshc_pltfm_data sdhci_dwcmshc_th1520_pdata = { >>>> @@ -2309,7 +2326,7 @@ static const struct of_device_id sdhci_dwcmshc_dt_ids[] = { >>>> }, >>>> { >>>> .compatible = "rockchip,rk3588-dwcmshc", >>>> - .data = &sdhci_dwcmshc_rk35xx_pdata, >>>> + .data = &sdhci_dwcmshc_rk3588_pdata, >>>> }, >>>> { >>>> .compatible = "rockchip,rk3576-dwcmshc", >>>> @@ -2317,7 +2334,7 @@ static const struct of_device_id sdhci_dwcmshc_dt_ids[] = { >>>> }, >>>> { >>>> .compatible = "rockchip,rk3568-dwcmshc", >>>> - .data = &sdhci_dwcmshc_rk35xx_pdata, >>>> + .data = &sdhci_dwcmshc_rk3568_pdata, >>>> }, >>>> { >>>> .compatible = "snps,dwcmshc-sdhci", >>> >>> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] mmc: sdhci-of-dwcmshc: Refactor Rockchip platform data for controller revisions 2026-03-27 13:33 ` Shawn Lin @ 2026-03-27 13:43 ` Adrian Hunter 0 siblings, 0 replies; 6+ messages in thread From: Adrian Hunter @ 2026-03-27 13:43 UTC (permalink / raw) To: Shawn Lin; +Cc: linux-mmc, linux-rockchip, Ulf Hansson On 27/03/2026 15:33, Shawn Lin wrote: > 在 2026/03/27 星期五 19:34, Adrian Hunter 写道: >> On 27/03/2026 13:28, Shawn Lin wrote: >>> Hi Adrian >>> >>> 在 2026/03/27 星期五 17:29, Adrian Hunter 写道: >>>> On 27/03/2026 09:51, Shawn Lin wrote: >>>>> The driver previously used an enum (dwcmshc_rk_type) to distinguish >>>>> platforms like the RK3568 and RK3588 based on DT compatible names. >>>>> This approach is inflexible, scales poorly for future revisions or >>>>> mixed-revision platforms, and conflates SoC names with controller >>>>> revisions. >>>>> >>>>> Introduces a new struct rockchip_pltfm_data containing a dedicated >>>>> revision field. The old enum is removed, and all code paths are >>>>> updated to use the revision-based data. >>>>> >>>>> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >>>>> --- >>>>> >>>>> drivers/mmc/host/sdhci-of-dwcmshc.c | 87 ++++++++++++++++++++++--------------- >>>>> 1 file changed, 52 insertions(+), 35 deletions(-) >>>>> >>>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c >>>>> index a91b3e7..6b43ba9 100644 >>>>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c >>>>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c >>>>> @@ -278,14 +278,8 @@ >>>>> #define PHY_DELAY_CODE_EMMC 0x17 >>>>> #define PHY_DELAY_CODE_SD 0x55 >>>>> -enum dwcmshc_rk_type { >>>>> - DWCMSHC_RK3568, >>>>> - DWCMSHC_RK3588, >>>>> -}; >>>>> - >>>>> struct rk35xx_priv { >>>>> struct reset_control *reset; >>>>> - enum dwcmshc_rk_type devtype; >>>>> u8 txclk_tapnum; >>>>> }; >>>>> @@ -330,6 +324,11 @@ struct k230_pltfm_data { >>>>> u32 write_prot_bit; >>>>> }; >>>>> +struct rockchip_pltfm_data { >>>>> + struct dwcmshc_pltfm_data dwcmshc_pdata; >>>>> + int revision; >>>> >>>> Does this "revision" match HW specs, otherwise maybe need >>>> to clarify where it comes from. >>>> >>> >>> This "revision" originates from our internal IP development >>> documentation, not the public chip specifications. The public manuals do not label these specific IP versions. >>> >>> Revision 0 (Initial IP): This is the initial version of the IP used in >>> the RK3568 and RK3566. Post-silicon validation identified certain errata >>> requiring workarounds, and this version lacks HS400 support. >>> >>> Revision 1 (Fixed IP): This is the updated IP version (with >>> documentation number updated internally). It fixes the previous issues >>> and adds HS400 support. This revision is used in all subsequent chips >>> (RK3576, RK3588, etc.). >>> >>> Since the public specs do not expose this distinction, I believe using >>> the internal IP development document revision is the most accurate way >>> to differentiate the hardware behavior in software. I will add a comment >>> in the code to clarify this origin and the specific differences between >>> the revisions. >>> >>>>> +}; >>>>> + >>>>> static void dwcmshc_enable_card_clk(struct sdhci_host *host) >>>>> { >>>>> u16 ctrl; >>>>> @@ -749,6 +748,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock >>>>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host); >>>>> struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host); >>>>> struct rk35xx_priv *priv = dwc_priv->priv; >>>>> + const struct rockchip_pltfm_data *rockchip_pdata = to_pltfm_data(dwc_priv, rockchip); >>>>> u8 txclk_tapnum = DLL_TXCLK_TAPNUM_DEFAULT; >>>>> u32 extra, reg; >>>>> int err; >>>>> @@ -816,7 +816,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock >>>>> * we must set it in higher speed mode. >>>>> */ >>>>> extra = DWCMSHC_EMMC_DLL_DLYENA; >>>>> - if (priv->devtype == DWCMSHC_RK3568) >>>>> + if (rockchip_pdata->revision == 0) >>>>> extra |= DLL_RXCLK_NO_INVERTER << DWCMSHC_EMMC_DLL_RXCLK_SRCSEL; >>>>> sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_RXCLK); >>>>> @@ -842,7 +842,7 @@ static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock >>>>> host->mmc->ios.timing == MMC_TIMING_MMC_HS400) >>>>> txclk_tapnum = priv->txclk_tapnum; >>>>> - if ((priv->devtype == DWCMSHC_RK3588) && host->mmc->ios.timing == MMC_TIMING_MMC_HS400) { >>>>> + if ((rockchip_pdata->revision == 1) && host->mmc->ios.timing == MMC_TIMING_MMC_HS400) { >>>>> txclk_tapnum = DLL_TXCLK_TAPNUM_90_DEGREES; >>>>> extra = DLL_CMDOUT_SRC_CLK_NEG | >>>>> @@ -898,11 +898,6 @@ static int dwcmshc_rk35xx_init(struct device *dev, struct sdhci_host *host, >>>>> if (!priv) >>>>> return -ENOMEM; >>>>> - if (of_device_is_compatible(dev->of_node, "rockchip,rk3588-dwcmshc")) >>>>> - priv->devtype = DWCMSHC_RK3588; >>>>> - else >>>>> - priv->devtype = DWCMSHC_RK3568; >>>>> - >>>>> priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc)); >>>>> if (IS_ERR(priv->reset)) { >>>>> err = PTR_ERR(priv->reset); >>>>> @@ -2115,30 +2110,52 @@ static const struct cqhci_host_ops rk35xx_cqhci_ops = { >>>>> .set_tran_desc = dwcmshc_set_tran_desc, >>>>> }; >>>>> -static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk35xx_pdata = { >>>>> - .pdata = { >>>>> - .ops = &sdhci_dwcmshc_rk35xx_ops, >>>>> - .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >>>>> - SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >>>>> - .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >>>>> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >>>>> +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3568_pdata = { >>>>> + .dwcmshc_pdata = { >>>>> + .pdata = { >>>>> + .ops = &sdhci_dwcmshc_rk35xx_ops, >>>>> + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >>>>> + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >>>>> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >>>>> + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >>>>> + }, >>>>> + .cqhci_host_ops = &rk35xx_cqhci_ops, >>>>> + .init = dwcmshc_rk35xx_init, >>>>> + .postinit = dwcmshc_rk35xx_postinit, >>>>> }, >>>>> - .cqhci_host_ops = &rk35xx_cqhci_ops, >>>>> - .init = dwcmshc_rk35xx_init, >>>>> - .postinit = dwcmshc_rk35xx_postinit, >>>>> + .revision = 0, >>>>> }; >>>>> -static const struct dwcmshc_pltfm_data sdhci_dwcmshc_rk3576_pdata = { >>>>> - .pdata = { >>>>> - .ops = &sdhci_dwcmshc_rk35xx_ops, >>>>> - .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >>>>> - SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >>>>> - .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >>>>> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >>>>> +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3588_pdata = { >>>> >>>> Better order: sdhci_dwcmshc_rk3568_pdata, sdhci_dwcmshc_rk3576_pdata, >>>> sdhci_dwcmshc_rk3588_pdata >>> >>> >>> Sure， will do. >>> >>>> >>>>> + .dwcmshc_pdata = { >>>>> + .pdata = { >>>>> + .ops = &sdhci_dwcmshc_rk35xx_ops, >>>>> + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >>>>> + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >>>>> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >>>>> + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >>>>> + }, >>>>> + .cqhci_host_ops = &rk35xx_cqhci_ops, >>>>> + .init = dwcmshc_rk35xx_init, >>>>> + .postinit = dwcmshc_rk35xx_postinit, >>>>> + }, >>>>> + .revision = 1, >>>>> +}; >>>>> + >>>>> +static const struct rockchip_pltfm_data sdhci_dwcmshc_rk3576_pdata = { >>>>> + .dwcmshc_pdata = { >>>>> + .pdata = { >>>>> + .ops = &sdhci_dwcmshc_rk35xx_ops, >>>>> + .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN | >>>>> + SDHCI_QUIRK_BROKEN_TIMEOUT_VAL, >>>>> + .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN | >>>>> + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN, >>>>> + }, >>>>> + .cqhci_host_ops = &rk35xx_cqhci_ops, >>>>> + .init = dwcmshc_rk35xx_init, >>>>> + .postinit = dwcmshc_rk3576_postinit, >>>>> }, >>>>> - .cqhci_host_ops = &rk35xx_cqhci_ops, >>>>> - .init = dwcmshc_rk35xx_init, >>>>> - .postinit = dwcmshc_rk3576_postinit, >>>>> + .revision = 1, >>>> >>>> Isn't rk3576 revision = 0 ? >>> >>> As explained above, rk3576 is revision 1. >> >> Ok, but was it behaving that way before, since the code was: >> >> if (of_device_is_compatible(dev->of_node, "rockchip,rk3588-dwcmshc")) >> priv->devtype = DWCMSHC_RK3588; >> else >> priv->devtype = DWCMSHC_RK3568; >> > > Yes, you are absolutely right to be confused，the old code logic was indeed the root of the problem, which is why this cleanup is necessary. > > As you can see in the Device Tree [1], RK3576 explicitly lists "rockchip,rk3588-dwcmshc" as a secondary compatible string to select > DWCMSHC_RK3588(now revision 1) as the devtype. > > [1] https://elixir.bootlin.com/linux/v7.0-rc5/source/arch/arm64/boot/dts/rockchip/rk3576.dtsi#L1922 > I guess you'll add something about that to the commit message > > >>> >>>> >>>>> }; >>>>> static const struct dwcmshc_pltfm_data sdhci_dwcmshc_th1520_pdata = { >>>>> @@ -2309,7 +2326,7 @@ static const struct of_device_id sdhci_dwcmshc_dt_ids[] = { >>>>> }, >>>>> { >>>>> .compatible = "rockchip,rk3588-dwcmshc", >>>>> - .data = &sdhci_dwcmshc_rk35xx_pdata, >>>>> + .data = &sdhci_dwcmshc_rk3588_pdata, >>>>> }, >>>>> { >>>>> .compatible = "rockchip,rk3576-dwcmshc", >>>>> @@ -2317,7 +2334,7 @@ static const struct of_device_id sdhci_dwcmshc_dt_ids[] = { >>>>> }, >>>>> { >>>>> .compatible = "rockchip,rk3568-dwcmshc", >>>>> - .data = &sdhci_dwcmshc_rk35xx_pdata, >>>>> + .data = &sdhci_dwcmshc_rk3568_pdata, >>>>> }, >>>>> { >>>>> .compatible = "snps,dwcmshc-sdhci", >>>> >>>> >> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-03-27 15:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-03-27 7:51 [PATCH] mmc: sdhci-of-dwcmshc: Refactor Rockchip platform data for controller revisions Shawn Lin 2026-03-27 9:29 ` Adrian Hunter 2026-03-27 11:28 ` Shawn Lin 2026-03-27 11:34 ` Adrian Hunter 2026-03-27 13:33 ` Shawn Lin 2026-03-27 13:43 ` Adrian Hunter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox