On Mon, 3 Apr 2023 at 22:48, Jonas Karlman <jo...@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 <jo...@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 >