[PATCH] mmc: sdhci-of-dwcmshc: Refactor Rockchip platform data for controller revisions
Shawn Lin
shawn.lin at rock-chips.com
Fri Mar 27 06:33:36 PDT 2026
在 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
>>
>>>
>>>> };
>>>> 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