[PATCH] mmc: sdhci-of-dwcmshc: Refactor Rockchip platform data for controller revisions
Adrian Hunter
adrian.hunter at intel.com
Fri Mar 27 06:43:15 PDT 2026
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 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.
>>
>> 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",
>>>>
>>>>
>>
>>
More information about the Linux-rockchip
mailing list