Re: [PATCH] mmc: sdhci-of-dwcmshc: Refactor Rockchip platform data for controller revisions

[Adrian Hunter <adrian.hunter@intel.com>]

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",
>>
>>