[PATCH] mmc: sdhci-of-dwcmshc: Refactor Rockchip platform data for controller revisions
Shawn Lin
shawn.lin at rock-chips.com
Fri Mar 27 04:28:53 PDT 2026
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 at 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",
>
>
More information about the Linux-rockchip
mailing list