[PATCH 13/17] mmc: rockchip_sdhci: Add support for RK3588

Philipp Tomsich philipp.tomsich at vrull.eu
Tue Apr 4 01:12:42 CEST 2023


On Mon, 3 Apr 2023 at 22:48, Jonas Karlman <jonas at kwiboo.se> wrote:
>
> Add support for RK3588 to the sdhci driver. RK3588 has the inverter flag
> in TXCLK reg instead of RXCLK and also make use of a new CMDOUT reg.
> Add and use a quirks field to support such quirks.
>
> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
> ---
>  drivers/mmc/rockchip_sdhci.c | 62 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 59 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
> index 12a616d3dfb8..9178bc00b6b8 100644
> --- a/drivers/mmc/rockchip_sdhci.c
> +++ b/drivers/mmc/rockchip_sdhci.c
> @@ -56,6 +56,7 @@
>  #define DWCMSHC_EMMC_DLL_RXCLK         0x804
>  #define DWCMSHC_EMMC_DLL_TXCLK         0x808
>  #define DWCMSHC_EMMC_DLL_STRBIN                0x80c
> +#define DWCMSHC_EMMC_DLL_CMDOUT                0x810
>  #define DWCMSHC_EMMC_DLL_STATUS0       0x840
>  #define DWCMSHC_EMMC_DLL_STATUS1       0x844
>  #define DWCMSHC_EMMC_DLL_START         BIT(0)
> @@ -70,18 +71,28 @@
>  #define DLL_RXCLK_NO_INVERTER          BIT(29)
>  #define DLL_RXCLK_ORI_GATE             BIT(31)
>  #define DLL_TXCLK_TAPNUM_DEFAULT       0x10
> +#define DLL_TXCLK_TAPNUM_90_DEGREES    0x9
>  #define DLL_TXCLK_TAPNUM_FROM_SW       BIT(24)
> +#define DLL_TXCLK_NO_INVERTER          BIT(29)
>  #define DLL_STRBIN_TAPNUM_DEFAULT      0x4
>  #define DLL_STRBIN_TAPNUM_FROM_SW      BIT(24)
>  #define DLL_STRBIN_DELAY_NUM_SEL       BIT(26)
>  #define DLL_STRBIN_DELAY_NUM_OFFSET    16
>  #define DLL_STRBIN_DELAY_NUM_DEFAULT   0x10
> +#define DLL_CMDOUT_TAPNUM_90_DEGREES   0x8
> +#define DLL_CMDOUT_TAPNUM_FROM_SW      BIT(24)
> +#define DLL_CMDOUT_SRC_CLK_NEG         BIT(28)
> +#define DLL_CMDOUT_EN_SRC_CLK_NEG      BIT(29)
> +#define DLL_CMDOUT_BOTH_CLK_EDGE       BIT(30)
>
>  #define DLL_LOCK_WO_TMOUT(x) \
>         ((((x) & DWCMSHC_EMMC_DLL_LOCKED) == DWCMSHC_EMMC_DLL_LOCKED) && \
>         (((x) & DWCMSHC_EMMC_DLL_TIMEOUT) == 0))
>  #define ROCKCHIP_MAX_CLKS              3
>
> +#define QUIRK_INVERTER_FLAG_IN_RXCLK   BIT(0)
> +#define QUIRK_HAS_DLL_CMDOUT           BIT(1)
> +
>  struct rockchip_sdhc_plat {
>         struct mmc_config cfg;
>         struct mmc mmc;
> @@ -99,6 +110,7 @@ struct rockchip_sdhc {
>         void *base;
>         struct rockchip_emmc_phy *phy;
>         struct clk emmc_clk;
> +       u8 txclk_tapnum;
>  };
>
>  struct sdhci_data {
> @@ -144,6 +156,8 @@ struct sdhci_data {
>          * Return: 0 if successful, -ve on error
>          */
>         int (*set_enhanced_strobe)(struct sdhci_host *host);
> +
> +       u32 quirks;

As these are not really quirks (i.e., errata), it would be nicer to
add new function pointers to abstract away the difference in
behaviour.

>  };
>
>  static void rk3399_emmc_phy_power_on(struct rockchip_emmc_phy *phy, u32 clock)
> @@ -294,6 +308,10 @@ static void rk3568_sdhci_set_clock(struct sdhci_host *host, u32 div)
>
>  static int rk3568_sdhci_config_dll(struct sdhci_host *host, u32 clock, bool enable)
>  {
> +       struct rockchip_sdhc *priv = container_of(host, struct rockchip_sdhc, host);
> +       struct sdhci_data *data = (struct sdhci_data *)dev_get_driver_data(priv->dev);
> +       struct mmc *mmc = host->mmc;
> +       u8 txclk_tapnum = DLL_TXCLK_TAPNUM_DEFAULT;
>         int val, ret;
>         u32 extra;
>
> @@ -318,12 +336,33 @@ static int rk3568_sdhci_config_dll(struct sdhci_host *host, u32 clock, bool enab
>                 if (ret)
>                         return ret;
>
> -               extra = DWCMSHC_EMMC_DLL_DLYENA | DLL_RXCLK_NO_INVERTER;
> +               extra = DWCMSHC_EMMC_DLL_DLYENA | DLL_RXCLK_ORI_GATE;

Adding DLL_RXCLK_ORI_GATE here is not documented in the commit message.
Was this missing before?

> +               if (data->quirks & QUIRK_INVERTER_FLAG_IN_RXCLK)
> +                       extra |= DLL_RXCLK_NO_INVERTER;
>                 sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_RXCLK);

This could be abstracted out as
   if (data->enable_rxclk)
      data->enable_rxclk();
or even as (a new function) rockchip_sdhci_enable_rxclk(host) ... and
then hiding the indirect function call in that function.

>
> +               if (mmc->selected_mode == MMC_HS_200 ||
> +                   mmc->selected_mode == MMC_HS_400 ||
> +                   mmc->selected_mode == MMC_HS_400_ES)
> +                       txclk_tapnum = priv->txclk_tapnum;
> +
> +               if ((data->quirks & QUIRK_HAS_DLL_CMDOUT) &&
> +                   (mmc->selected_mode == MMC_HS_400 ||
> +                    mmc->selected_mode == MMC_HS_400_ES)) {
> +                       txclk_tapnum = DLL_TXCLK_TAPNUM_90_DEGREES;

This overwrites txclk_tapnum (just set a few lines above).  Is this
intentional or did you intend to OR this in?

> +
> +                       extra = DLL_CMDOUT_SRC_CLK_NEG |
> +                               DLL_CMDOUT_BOTH_CLK_EDGE |
> +                               DWCMSHC_EMMC_DLL_DLYENA |
> +                               DLL_CMDOUT_TAPNUM_90_DEGREES |
> +                               DLL_CMDOUT_TAPNUM_FROM_SW;
> +                       sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_CMDOUT);
> +               }
> +
>                 extra = DWCMSHC_EMMC_DLL_DLYENA |
> -                       DLL_TXCLK_TAPNUM_DEFAULT |
> -                       DLL_TXCLK_TAPNUM_FROM_SW;
> +                       DLL_TXCLK_TAPNUM_FROM_SW |
> +                       DLL_TXCLK_NO_INVERTER |
> +                       txclk_tapnum;
>                 sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_TXCLK);
>
>                 extra = DWCMSHC_EMMC_DLL_DLYENA |
> @@ -339,6 +378,8 @@ static int rk3568_sdhci_config_dll(struct sdhci_host *host, u32 clock, bool enab
>                 sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_CTRL);
>                 sdhci_writel(host, DLL_RXCLK_ORI_GATE, DWCMSHC_EMMC_DLL_RXCLK);
>                 sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
> +               if (data->quirks & QUIRK_HAS_DLL_CMDOUT)
> +                       sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_CMDOUT);
>                 /*
>                  * Before switching to hs400es mode, the driver will enable
>                  * enhanced strobe first. PHY needs to configure the parameters
> @@ -573,6 +614,9 @@ static int rockchip_sdhci_of_to_plat(struct udevice *dev)
>         if (ret)
>                 return ret;
>
> +       priv->txclk_tapnum = dev_read_u8_default(dev, "rockchip,txclk-tapnum",
> +                                                DLL_TXCLK_TAPNUM_DEFAULT);
> +
>         return 0;
>  }
>
> @@ -594,6 +638,14 @@ static const struct sdhci_data rk3568_data = {
>         .set_ios_post = rk3568_sdhci_set_ios_post,
>         .set_clock = rk3568_sdhci_set_clock,
>         .config_dll = rk3568_sdhci_config_dll,
> +       .quirks = QUIRK_INVERTER_FLAG_IN_RXCLK,
> +};
> +
> +static const struct sdhci_data rk3588_data = {
> +       .set_ios_post = rk3568_sdhci_set_ios_post,
> +       .set_clock = rk3568_sdhci_set_clock,
> +       .config_dll = rk3568_sdhci_config_dll,
> +       .quirks = QUIRK_HAS_DLL_CMDOUT,
>  };
>
>  static const struct udevice_id sdhci_ids[] = {
> @@ -605,6 +657,10 @@ static const struct udevice_id sdhci_ids[] = {
>                 .compatible = "rockchip,rk3568-dwcmshc",
>                 .data = (ulong)&rk3568_data,
>         },
> +       {
> +               .compatible = "rockchip,rk3588-dwcmshc",
> +               .data = (ulong)&rk3588_data,
> +       },
>         { }
>  };
>
> --
> 2.40.0
>


More information about the U-Boot mailing list