On 2023-04-04 01:12, Philipp Tomsich wrote:
On Mon, 3 Apr 2023 at 22:48, Jonas Karlman jonas@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@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.
It may not be real errata but it is different behaviour, I could call it flags/features/version if that helps.
};
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?
This is a new flag for the hw revision used in RK3588 (and others). It was prematurely added for the clk <= 52 MHz case in commit 2321a991bbb5 ("rockchip: sdhci: rk3568: bypass DLL when clk <= 52 MHz").
Since this is not a conflicting bit, it gets set for all.
RK3568 TRM: reserved RK3588 TRM: Receive original clock source gating enable.
I could not detect anything different with this flag set or unset, vendor linux/u-boot sets this flag, so I opted to also set it.
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.
That seem a little bit excessive to handle a minor quirk (lack of feature) in some hw revisions of this block.
The hw revision used in RK3588 gain support for "Transmit clock source selection" and this also changes the "Receive clock source selection" meaning. For this hw revision we need to set "original clock input" / no-inverter flag in the TXCLK reg instead of the RXCLK reg.
RK3568 TRM: RXCLK: Receive clock source selection. 0: Receive clock source is inverted 1: Receive clock source is no-inverted This bit should be set to 1 for normal operation. TXCLK: reserved
RK3588 TRM: RXCLK: Receive clock source selection. 0: Receive clock source is transmit clock output 1: Receive clock source is original clock input TXCLK: Transmit clock source selection. 0: Transmit clock source is invertion of original clock input 1: Transmit clock source is original clock input
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?
This is intentional, the whole rockchip,txclk-tapnum handling was copied from linux, but this does not fully match what we need to configure in the regs, so maybe we should just drop it.
In vendor linux/u-boot this is handled with driver data and we can change to use a similar approach.
RK3568 transmit tap value: HS200 and HS400: 16
RK3588 transmit tap value: HS200: 16 HS400: 9
Tap values and other flags was picked from latest available vendor linux 5.10 [1] and vendor u-boot [2].
[1] https://github.com/rockchip-toybrick/edge/blob/main/kernel/linux-5.10/driver... [2] https://github.com/JeffyCN/rockchip_mirrors/blob/u-boot/drivers/mmc/rockchip...
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 |
Here we set "Transmit clock source is original clock input" for new hw revision, for RK3568 this has no impact (reserved).
To summary, the only special handling we need in order to support both RK3568 and RK3588 are: 1. Set of "no-inverter" flag in RXCLK for RK3568 and in TXCLK on RK3588. 2. Use a different tx tap value for HS400 modes on RK3588. 3. CMDOUT reg must be configured for HS400 modess on RK3588.
We also know that other SoCs, RK3528 and RK3562, need to use different tap values, configure no-inverter flag in rxclk reg and configure rx sw tuning values. I have no hw or TRM for these SoCs so I can not fully confirm this.
Regards, Jonas
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