* [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
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ 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",
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ 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",
>
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ 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",
>>
>>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ 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",
>>>
>>>
>
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ 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",
>>>>
>>>>
>>
>>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-03-27 13:43 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