[PATCH v1 0/1] tegra_mmc: get tap and trim from dts
Default-tap and default-trim values are used for eMMC setup mostly on T114+ devices. As for now, those values are hardcoded for T210 and ignored for all other Tegra generations. Fix this by passing tap and trim values from dts.
This commit will cause regression on T210 (emmc may not work), fix is applied in tegra210.dtsi and will be sent next week.
Svyatoslav Ryhel (1): mmc: tegra: get default-tap and default-trim from device tree
arch/arm/include/asm/arch-tegra/tegra_mmc.h | 17 ++++---- drivers/mmc/tegra_mmc.c | 46 ++++++++++----------- 2 files changed, 30 insertions(+), 33 deletions(-)
Default-tap and default-trim values are used for eMMC setup mostly on T114+ devices. As for now, those values are hardcoded for T210 and ignored for all other Tegra generations. Fix this by passing tap and trim values from dts.
Tested-by: Svyatoslav Ryhel clamor95@gmail.com # ASUS TF701T Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com --- arch/arm/include/asm/arch-tegra/tegra_mmc.h | 17 ++++---- drivers/mmc/tegra_mmc.c | 46 ++++++++++----------- 2 files changed, 30 insertions(+), 33 deletions(-)
diff --git a/arch/arm/include/asm/arch-tegra/tegra_mmc.h b/arch/arm/include/asm/arch-tegra/tegra_mmc.h index d6a55764ba..750c7d809e 100644 --- a/arch/arm/include/asm/arch-tegra/tegra_mmc.h +++ b/arch/arm/include/asm/arch-tegra/tegra_mmc.h @@ -128,21 +128,22 @@ struct tegra_mmc {
/* SDMMC1/3 settings from SDMMCx Initialization Sequence of TRM */ #define MEMCOMP_PADCTRL_VREF 7 -#define AUTO_CAL_ENABLE (1 << 29) -#define AUTO_CAL_ACTIVE (1 << 31) -#define AUTO_CAL_START (1 << 31) +#define AUTO_CAL_ENABLE BIT(29) +#define AUTO_CAL_ACTIVE BIT(31) +#define AUTO_CAL_START BIT(31) + #if defined(CONFIG_TEGRA210) #define AUTO_CAL_PD_OFFSET (0x7D << 8) #define AUTO_CAL_PU_OFFSET (0 << 0) -#define IO_TRIM_BYPASS_MASK (1 << 2) -#define TRIM_VAL_SHIFT 24 -#define TRIM_VAL_MASK (0x1F << TRIM_VAL_SHIFT) -#define TAP_VAL_SHIFT 16 -#define TAP_VAL_MASK (0xFF << TAP_VAL_SHIFT) #else #define AUTO_CAL_PD_OFFSET (0x70 << 8) #define AUTO_CAL_PU_OFFSET (0x62 << 0) #endif
+#define TRIM_VAL_SHIFT 24 +#define TRIM_VAL_MASK (0x1F << TRIM_VAL_SHIFT) +#define TAP_VAL_SHIFT 16 +#define TAP_VAL_MASK (0xFF << TAP_VAL_SHIFT) + #endif /* __ASSEMBLY__ */ #endif /* __TEGRA_MMC_H_ */ diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c index f76fee3ea0..7627800261 100644 --- a/drivers/mmc/tegra_mmc.c +++ b/drivers/mmc/tegra_mmc.c @@ -37,6 +37,9 @@ struct tegra_mmc_priv { unsigned int version; /* SDHCI spec. version */ unsigned int clock; /* Current clock (MHz) */ int mmc_id; /* peripheral id */ + + u32 tap_value; + u32 trim_value; };
static void tegra_mmc_set_power(struct tegra_mmc_priv *priv, @@ -526,31 +529,6 @@ static void tegra_mmc_pad_init(struct tegra_mmc_priv *priv) printf("%s: Warning: Autocal timed out!\n", __func__); /* TBD: Set CFG2TMC_SDMMC1_PAD_CAL_DRV* regs here */ } - -#if defined(CONFIG_TEGRA210) - u32 tap_value, trim_value; - - /* Set tap/trim values for SDMMC1/3 @ <48MHz here */ - val = readl(&priv->reg->venspictl); /* aka VENDOR_SYS_SW_CNTL */ - val &= IO_TRIM_BYPASS_MASK; - if (id == PERIPH_ID_SDMMC1) { - tap_value = 4; /* default */ - if (val) - tap_value = 3; - trim_value = 2; - } else { /* SDMMC3 */ - tap_value = 3; - trim_value = 3; - } - - val = readl(&priv->reg->venclkctl); - val &= ~TRIM_VAL_MASK; - val |= (trim_value << TRIM_VAL_SHIFT); - val &= ~TAP_VAL_MASK; - val |= (tap_value << TAP_VAL_SHIFT); - writel(val, &priv->reg->venclkctl); - debug("%s: VENDOR_CLOCK_CNTRL = 0x%08X\n", __func__, val); -#endif /* T210 */ #endif /* T30/T210 */ }
@@ -588,6 +566,21 @@ static void tegra_mmc_reset(struct tegra_mmc_priv *priv, struct mmc *mmc)
/* Make sure SDIO pads are set up */ tegra_mmc_pad_init(priv); + + if (priv->tap_value || priv->trim_value) { + u32 val; + + val = readl(&priv->reg->venclkctl); + + val &= ~TRIM_VAL_MASK; + val |= (priv->trim_value << TRIM_VAL_SHIFT); + + val &= ~TAP_VAL_MASK; + val |= (priv->tap_value << TAP_VAL_SHIFT); + + writel(val, &priv->reg->venclkctl); + debug("%s: VENDOR_CLOCK_CNTRL = 0x%08X\n", __func__, val); + } }
static int tegra_mmc_init(struct udevice *dev) @@ -742,6 +735,9 @@ static int tegra_mmc_probe(struct udevice *dev) if (dm_gpio_is_valid(&priv->pwr_gpio)) dm_gpio_set_value(&priv->pwr_gpio, 1);
+ priv->tap_value = dev_read_u32_default(dev, "nvidia,default-tap", 0); + priv->trim_value = dev_read_u32_default(dev, "nvidia,default-trim", 0); + upriv->mmc = &plat->mmc;
return tegra_mmc_init(dev);
On Sat, Aug 19, 2023 at 06:35:01PM +0300, Svyatoslav Ryhel wrote:
Default-tap and default-trim values are used for eMMC setup mostly on T114+ devices. As for now, those values are hardcoded for T210 and ignored for all other Tegra generations. Fix this by passing tap and trim values from dts.
Tested-by: Svyatoslav Ryhel clamor95@gmail.com # ASUS TF701T Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
arch/arm/include/asm/arch-tegra/tegra_mmc.h | 17 ++++---- drivers/mmc/tegra_mmc.c | 46 ++++++++++----------- 2 files changed, 30 insertions(+), 33 deletions(-)
diff --git a/arch/arm/include/asm/arch-tegra/tegra_mmc.h b/arch/arm/include/asm/arch-tegra/tegra_mmc.h index d6a55764ba..750c7d809e 100644 --- a/arch/arm/include/asm/arch-tegra/tegra_mmc.h +++ b/arch/arm/include/asm/arch-tegra/tegra_mmc.h @@ -128,21 +128,22 @@ struct tegra_mmc {
/* SDMMC1/3 settings from SDMMCx Initialization Sequence of TRM */ #define MEMCOMP_PADCTRL_VREF 7 -#define AUTO_CAL_ENABLE (1 << 29) -#define AUTO_CAL_ACTIVE (1 << 31) -#define AUTO_CAL_START (1 << 31) +#define AUTO_CAL_ENABLE BIT(29) +#define AUTO_CAL_ACTIVE BIT(31) +#define AUTO_CAL_START BIT(31)
#if defined(CONFIG_TEGRA210) #define AUTO_CAL_PD_OFFSET (0x7D << 8) #define AUTO_CAL_PU_OFFSET (0 << 0) -#define IO_TRIM_BYPASS_MASK (1 << 2) -#define TRIM_VAL_SHIFT 24 -#define TRIM_VAL_MASK (0x1F << TRIM_VAL_SHIFT) -#define TAP_VAL_SHIFT 16 -#define TAP_VAL_MASK (0xFF << TAP_VAL_SHIFT) #else #define AUTO_CAL_PD_OFFSET (0x70 << 8) #define AUTO_CAL_PU_OFFSET (0x62 << 0) #endif
+#define TRIM_VAL_SHIFT 24 +#define TRIM_VAL_MASK (0x1F << TRIM_VAL_SHIFT) +#define TAP_VAL_SHIFT 16 +#define TAP_VAL_MASK (0xFF << TAP_VAL_SHIFT)
#endif /* __ASSEMBLY__ */ #endif /* __TEGRA_MMC_H_ */ diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c index f76fee3ea0..7627800261 100644 --- a/drivers/mmc/tegra_mmc.c +++ b/drivers/mmc/tegra_mmc.c @@ -37,6 +37,9 @@ struct tegra_mmc_priv { unsigned int version; /* SDHCI spec. version */ unsigned int clock; /* Current clock (MHz) */ int mmc_id; /* peripheral id */
- u32 tap_value;
- u32 trim_value;
};
static void tegra_mmc_set_power(struct tegra_mmc_priv *priv, @@ -526,31 +529,6 @@ static void tegra_mmc_pad_init(struct tegra_mmc_priv *priv) printf("%s: Warning: Autocal timed out!\n", __func__); /* TBD: Set CFG2TMC_SDMMC1_PAD_CAL_DRV* regs here */ }
-#if defined(CONFIG_TEGRA210)
- u32 tap_value, trim_value;
- /* Set tap/trim values for SDMMC1/3 @ <48MHz here */
- val = readl(&priv->reg->venspictl); /* aka VENDOR_SYS_SW_CNTL */
- val &= IO_TRIM_BYPASS_MASK;
- if (id == PERIPH_ID_SDMMC1) {
tap_value = 4; /* default */if (val)tap_value = 3;trim_value = 2;- } else { /* SDMMC3 */
tap_value = 3;trim_value = 3;- }
- val = readl(&priv->reg->venclkctl);
- val &= ~TRIM_VAL_MASK;
- val |= (trim_value << TRIM_VAL_SHIFT);
- val &= ~TAP_VAL_MASK;
- val |= (tap_value << TAP_VAL_SHIFT);
- writel(val, &priv->reg->venclkctl);
- debug("%s: VENDOR_CLOCK_CNTRL = 0x%08X\n", __func__, val);
-#endif /* T210 */ #endif /* T30/T210 */ }
@@ -588,6 +566,21 @@ static void tegra_mmc_reset(struct tegra_mmc_priv *priv, struct mmc *mmc)
/* Make sure SDIO pads are set up */ tegra_mmc_pad_init(priv);
- if (priv->tap_value || priv->trim_value) {
I think 0 is a valid value for both tap and trim, so you want to be able to write that. I suggest getting rid of the conditional and always writing these values and rely on defaults to make sure that a good value is always programmed.
Alternatively if you really only want to program these when they've been specified, use an extra variable (or something like -1 as a default value) to discriminate.
The former is superior, in my opinion, because it also allows to avoid to regression.
Thierry
u32 val;val = readl(&priv->reg->venclkctl);val &= ~TRIM_VAL_MASK;val |= (priv->trim_value << TRIM_VAL_SHIFT);val &= ~TAP_VAL_MASK;val |= (priv->tap_value << TAP_VAL_SHIFT);writel(val, &priv->reg->venclkctl);debug("%s: VENDOR_CLOCK_CNTRL = 0x%08X\n", __func__, val);- }
}
static int tegra_mmc_init(struct udevice *dev) @@ -742,6 +735,9 @@ static int tegra_mmc_probe(struct udevice *dev) if (dm_gpio_is_valid(&priv->pwr_gpio)) dm_gpio_set_value(&priv->pwr_gpio, 1);
priv->tap_value = dev_read_u32_default(dev, "nvidia,default-tap", 0);
priv->trim_value = dev_read_u32_default(dev, "nvidia,default-trim", 0);
upriv->mmc = &plat->mmc;
return tegra_mmc_init(dev);
-- 2.39.2
23 серпня 2023 р. 14:03:25 GMT+03:00, Thierry Reding thierry.reding@gmail.com написав(-ла):
On Sat, Aug 19, 2023 at 06:35:01PM +0300, Svyatoslav Ryhel wrote:
Default-tap and default-trim values are used for eMMC setup mostly on T114+ devices. As for now, those values are hardcoded for T210 and ignored for all other Tegra generations. Fix this by passing tap and trim values from dts.
Tested-by: Svyatoslav Ryhel clamor95@gmail.com # ASUS TF701T Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
arch/arm/include/asm/arch-tegra/tegra_mmc.h | 17 ++++---- drivers/mmc/tegra_mmc.c | 46 ++++++++++----------- 2 files changed, 30 insertions(+), 33 deletions(-)
diff --git a/arch/arm/include/asm/arch-tegra/tegra_mmc.h b/arch/arm/include/asm/arch-tegra/tegra_mmc.h index d6a55764ba..750c7d809e 100644 --- a/arch/arm/include/asm/arch-tegra/tegra_mmc.h +++ b/arch/arm/include/asm/arch-tegra/tegra_mmc.h @@ -128,21 +128,22 @@ struct tegra_mmc {
/* SDMMC1/3 settings from SDMMCx Initialization Sequence of TRM */ #define MEMCOMP_PADCTRL_VREF 7 -#define AUTO_CAL_ENABLE (1 << 29) -#define AUTO_CAL_ACTIVE (1 << 31) -#define AUTO_CAL_START (1 << 31) +#define AUTO_CAL_ENABLE BIT(29) +#define AUTO_CAL_ACTIVE BIT(31) +#define AUTO_CAL_START BIT(31)
#if defined(CONFIG_TEGRA210) #define AUTO_CAL_PD_OFFSET (0x7D << 8) #define AUTO_CAL_PU_OFFSET (0 << 0) -#define IO_TRIM_BYPASS_MASK (1 << 2) -#define TRIM_VAL_SHIFT 24 -#define TRIM_VAL_MASK (0x1F << TRIM_VAL_SHIFT) -#define TAP_VAL_SHIFT 16 -#define TAP_VAL_MASK (0xFF << TAP_VAL_SHIFT) #else #define AUTO_CAL_PD_OFFSET (0x70 << 8) #define AUTO_CAL_PU_OFFSET (0x62 << 0) #endif
+#define TRIM_VAL_SHIFT 24 +#define TRIM_VAL_MASK (0x1F << TRIM_VAL_SHIFT) +#define TAP_VAL_SHIFT 16 +#define TAP_VAL_MASK (0xFF << TAP_VAL_SHIFT)
#endif /* __ASSEMBLY__ */ #endif /* __TEGRA_MMC_H_ */ diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c index f76fee3ea0..7627800261 100644 --- a/drivers/mmc/tegra_mmc.c +++ b/drivers/mmc/tegra_mmc.c @@ -37,6 +37,9 @@ struct tegra_mmc_priv { unsigned int version; /* SDHCI spec. version */ unsigned int clock; /* Current clock (MHz) */ int mmc_id; /* peripheral id */
- u32 tap_value;
- u32 trim_value;
};
static void tegra_mmc_set_power(struct tegra_mmc_priv *priv, @@ -526,31 +529,6 @@ static void tegra_mmc_pad_init(struct tegra_mmc_priv *priv) printf("%s: Warning: Autocal timed out!\n", __func__); /* TBD: Set CFG2TMC_SDMMC1_PAD_CAL_DRV* regs here */ }
-#if defined(CONFIG_TEGRA210)
- u32 tap_value, trim_value;
- /* Set tap/trim values for SDMMC1/3 @ <48MHz here */
- val = readl(&priv->reg->venspictl); /* aka VENDOR_SYS_SW_CNTL */
- val &= IO_TRIM_BYPASS_MASK;
- if (id == PERIPH_ID_SDMMC1) {
tap_value = 4; /* default */if (val)tap_value = 3;trim_value = 2;- } else { /* SDMMC3 */
tap_value = 3;trim_value = 3;- }
- val = readl(&priv->reg->venclkctl);
- val &= ~TRIM_VAL_MASK;
- val |= (trim_value << TRIM_VAL_SHIFT);
- val &= ~TAP_VAL_MASK;
- val |= (tap_value << TAP_VAL_SHIFT);
- writel(val, &priv->reg->venclkctl);
- debug("%s: VENDOR_CLOCK_CNTRL = 0x%08X\n", __func__, val);
-#endif /* T210 */ #endif /* T30/T210 */ }
@@ -588,6 +566,21 @@ static void tegra_mmc_reset(struct tegra_mmc_priv *priv, struct mmc *mmc)
/* Make sure SDIO pads are set up */ tegra_mmc_pad_init(priv);
- if (priv->tap_value || priv->trim_value) {
I think 0 is a valid value for both tap and trim, so you want to be able to write that. I suggest getting rid of the conditional and always writing these values and rely on defaults to make sure that a good value is always programmed.
Tegra 3 uses 0x0F on all my devices which is not 0.
Alternatively if you really only want to program these when they've been specified, use an extra variable (or something like -1 as a default value) to discriminate.
Why 0 value cannot be used as "flag" to skip tap and trim setup if values are not set? Using any value but 0 will cause additional comparation in condition and will definitely not improve readability.
The former is superior, in my opinion, because it also allows to avoid to regression.
Regression can be avoided if merge "General tegra and board improvements" first.
Thierry
u32 val;val = readl(&priv->reg->venclkctl);val &= ~TRIM_VAL_MASK;val |= (priv->trim_value << TRIM_VAL_SHIFT);val &= ~TAP_VAL_MASK;val |= (priv->tap_value << TAP_VAL_SHIFT);writel(val, &priv->reg->venclkctl);debug("%s: VENDOR_CLOCK_CNTRL = 0x%08X\n", __func__, val);- }
}
static int tegra_mmc_init(struct udevice *dev) @@ -742,6 +735,9 @@ static int tegra_mmc_probe(struct udevice *dev) if (dm_gpio_is_valid(&priv->pwr_gpio)) dm_gpio_set_value(&priv->pwr_gpio, 1);
priv->tap_value = dev_read_u32_default(dev, "nvidia,default-tap", 0);
priv->trim_value = dev_read_u32_default(dev, "nvidia,default-trim", 0);
upriv->mmc = &plat->mmc;
return tegra_mmc_init(dev);
-- 2.39.2
On Wed, Aug 23, 2023 at 02:38:48PM +0300, Svyatoslav Ryhel wrote:
23 серпня 2023 р. 14:03:25 GMT+03:00, Thierry Reding thierry.reding@gmail.com написав(-ла):
On Sat, Aug 19, 2023 at 06:35:01PM +0300, Svyatoslav Ryhel wrote:
Default-tap and default-trim values are used for eMMC setup mostly on T114+ devices. As for now, those values are hardcoded for T210 and ignored for all other Tegra generations. Fix this by passing tap and trim values from dts.
Tested-by: Svyatoslav Ryhel clamor95@gmail.com # ASUS TF701T Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
arch/arm/include/asm/arch-tegra/tegra_mmc.h | 17 ++++---- drivers/mmc/tegra_mmc.c | 46 ++++++++++----------- 2 files changed, 30 insertions(+), 33 deletions(-)
diff --git a/arch/arm/include/asm/arch-tegra/tegra_mmc.h b/arch/arm/include/asm/arch-tegra/tegra_mmc.h index d6a55764ba..750c7d809e 100644 --- a/arch/arm/include/asm/arch-tegra/tegra_mmc.h +++ b/arch/arm/include/asm/arch-tegra/tegra_mmc.h @@ -128,21 +128,22 @@ struct tegra_mmc {
/* SDMMC1/3 settings from SDMMCx Initialization Sequence of TRM */ #define MEMCOMP_PADCTRL_VREF 7 -#define AUTO_CAL_ENABLE (1 << 29) -#define AUTO_CAL_ACTIVE (1 << 31) -#define AUTO_CAL_START (1 << 31) +#define AUTO_CAL_ENABLE BIT(29) +#define AUTO_CAL_ACTIVE BIT(31) +#define AUTO_CAL_START BIT(31)
#if defined(CONFIG_TEGRA210) #define AUTO_CAL_PD_OFFSET (0x7D << 8) #define AUTO_CAL_PU_OFFSET (0 << 0) -#define IO_TRIM_BYPASS_MASK (1 << 2) -#define TRIM_VAL_SHIFT 24 -#define TRIM_VAL_MASK (0x1F << TRIM_VAL_SHIFT) -#define TAP_VAL_SHIFT 16 -#define TAP_VAL_MASK (0xFF << TAP_VAL_SHIFT) #else #define AUTO_CAL_PD_OFFSET (0x70 << 8) #define AUTO_CAL_PU_OFFSET (0x62 << 0) #endif
+#define TRIM_VAL_SHIFT 24 +#define TRIM_VAL_MASK (0x1F << TRIM_VAL_SHIFT) +#define TAP_VAL_SHIFT 16 +#define TAP_VAL_MASK (0xFF << TAP_VAL_SHIFT)
#endif /* __ASSEMBLY__ */ #endif /* __TEGRA_MMC_H_ */ diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c index f76fee3ea0..7627800261 100644 --- a/drivers/mmc/tegra_mmc.c +++ b/drivers/mmc/tegra_mmc.c @@ -37,6 +37,9 @@ struct tegra_mmc_priv { unsigned int version; /* SDHCI spec. version */ unsigned int clock; /* Current clock (MHz) */ int mmc_id; /* peripheral id */
- u32 tap_value;
- u32 trim_value;
};
static void tegra_mmc_set_power(struct tegra_mmc_priv *priv, @@ -526,31 +529,6 @@ static void tegra_mmc_pad_init(struct tegra_mmc_priv *priv) printf("%s: Warning: Autocal timed out!\n", __func__); /* TBD: Set CFG2TMC_SDMMC1_PAD_CAL_DRV* regs here */ }
-#if defined(CONFIG_TEGRA210)
- u32 tap_value, trim_value;
- /* Set tap/trim values for SDMMC1/3 @ <48MHz here */
- val = readl(&priv->reg->venspictl); /* aka VENDOR_SYS_SW_CNTL */
- val &= IO_TRIM_BYPASS_MASK;
- if (id == PERIPH_ID_SDMMC1) {
tap_value = 4; /* default */if (val)tap_value = 3;trim_value = 2;- } else { /* SDMMC3 */
tap_value = 3;trim_value = 3;- }
- val = readl(&priv->reg->venclkctl);
- val &= ~TRIM_VAL_MASK;
- val |= (trim_value << TRIM_VAL_SHIFT);
- val &= ~TAP_VAL_MASK;
- val |= (tap_value << TAP_VAL_SHIFT);
- writel(val, &priv->reg->venclkctl);
- debug("%s: VENDOR_CLOCK_CNTRL = 0x%08X\n", __func__, val);
-#endif /* T210 */ #endif /* T30/T210 */ }
@@ -588,6 +566,21 @@ static void tegra_mmc_reset(struct tegra_mmc_priv *priv, struct mmc *mmc)
/* Make sure SDIO pads are set up */ tegra_mmc_pad_init(priv);
- if (priv->tap_value || priv->trim_value) {
I think 0 is a valid value for both tap and trim, so you want to be able to write that. I suggest getting rid of the conditional and always writing these values and rely on defaults to make sure that a good value is always programmed.
Tegra 3 uses 0x0F on all my devices which is not 0.
Alternatively if you really only want to program these when they've been specified, use an extra variable (or something like -1 as a default value) to discriminate.
Why 0 value cannot be used as "flag" to skip tap and trim setup if values are not set? Using any value but 0 will cause additional comparation in condition and will definitely not improve readability.
If you use 0 as a flag it implies that you can never set 0 as a value explicitly. So if some SoC or board ever needs to set that exact value it can't.
Thierry
24 серпня 2023 р. 16:29:22 GMT+03:00, Thierry Reding thierry.reding@gmail.com написав(-ла):
On Wed, Aug 23, 2023 at 02:38:48PM +0300, Svyatoslav Ryhel wrote:
23 серпня 2023 р. 14:03:25 GMT+03:00, Thierry Reding thierry.reding@gmail.com написав(-ла):
On Sat, Aug 19, 2023 at 06:35:01PM +0300, Svyatoslav Ryhel wrote:
Default-tap and default-trim values are used for eMMC setup mostly on T114+ devices. As for now, those values are hardcoded for T210 and ignored for all other Tegra generations. Fix this by passing tap and trim values from dts.
Tested-by: Svyatoslav Ryhel clamor95@gmail.com # ASUS TF701T Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
arch/arm/include/asm/arch-tegra/tegra_mmc.h | 17 ++++---- drivers/mmc/tegra_mmc.c | 46 ++++++++++----------- 2 files changed, 30 insertions(+), 33 deletions(-)
diff --git a/arch/arm/include/asm/arch-tegra/tegra_mmc.h b/arch/arm/include/asm/arch-tegra/tegra_mmc.h index d6a55764ba..750c7d809e 100644 --- a/arch/arm/include/asm/arch-tegra/tegra_mmc.h +++ b/arch/arm/include/asm/arch-tegra/tegra_mmc.h @@ -128,21 +128,22 @@ struct tegra_mmc {
/* SDMMC1/3 settings from SDMMCx Initialization Sequence of TRM */ #define MEMCOMP_PADCTRL_VREF 7 -#define AUTO_CAL_ENABLE (1 << 29) -#define AUTO_CAL_ACTIVE (1 << 31) -#define AUTO_CAL_START (1 << 31) +#define AUTO_CAL_ENABLE BIT(29) +#define AUTO_CAL_ACTIVE BIT(31) +#define AUTO_CAL_START BIT(31)
#if defined(CONFIG_TEGRA210) #define AUTO_CAL_PD_OFFSET (0x7D << 8) #define AUTO_CAL_PU_OFFSET (0 << 0) -#define IO_TRIM_BYPASS_MASK (1 << 2) -#define TRIM_VAL_SHIFT 24 -#define TRIM_VAL_MASK (0x1F << TRIM_VAL_SHIFT) -#define TAP_VAL_SHIFT 16 -#define TAP_VAL_MASK (0xFF << TAP_VAL_SHIFT) #else #define AUTO_CAL_PD_OFFSET (0x70 << 8) #define AUTO_CAL_PU_OFFSET (0x62 << 0) #endif
+#define TRIM_VAL_SHIFT 24 +#define TRIM_VAL_MASK (0x1F << TRIM_VAL_SHIFT) +#define TAP_VAL_SHIFT 16 +#define TAP_VAL_MASK (0xFF << TAP_VAL_SHIFT)
#endif /* __ASSEMBLY__ */ #endif /* __TEGRA_MMC_H_ */ diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c index f76fee3ea0..7627800261 100644 --- a/drivers/mmc/tegra_mmc.c +++ b/drivers/mmc/tegra_mmc.c @@ -37,6 +37,9 @@ struct tegra_mmc_priv { unsigned int version; /* SDHCI spec. version */ unsigned int clock; /* Current clock (MHz) */ int mmc_id; /* peripheral id */
- u32 tap_value;
- u32 trim_value;
};
static void tegra_mmc_set_power(struct tegra_mmc_priv *priv, @@ -526,31 +529,6 @@ static void tegra_mmc_pad_init(struct tegra_mmc_priv *priv) printf("%s: Warning: Autocal timed out!\n", __func__); /* TBD: Set CFG2TMC_SDMMC1_PAD_CAL_DRV* regs here */ }
-#if defined(CONFIG_TEGRA210)
- u32 tap_value, trim_value;
- /* Set tap/trim values for SDMMC1/3 @ <48MHz here */
- val = readl(&priv->reg->venspictl); /* aka VENDOR_SYS_SW_CNTL */
- val &= IO_TRIM_BYPASS_MASK;
- if (id == PERIPH_ID_SDMMC1) {
tap_value = 4; /* default */if (val)tap_value = 3;trim_value = 2;- } else { /* SDMMC3 */
tap_value = 3;trim_value = 3;- }
- val = readl(&priv->reg->venclkctl);
- val &= ~TRIM_VAL_MASK;
- val |= (trim_value << TRIM_VAL_SHIFT);
- val &= ~TAP_VAL_MASK;
- val |= (tap_value << TAP_VAL_SHIFT);
- writel(val, &priv->reg->venclkctl);
- debug("%s: VENDOR_CLOCK_CNTRL = 0x%08X\n", __func__, val);
-#endif /* T210 */ #endif /* T30/T210 */ }
@@ -588,6 +566,21 @@ static void tegra_mmc_reset(struct tegra_mmc_priv *priv, struct mmc *mmc)
/* Make sure SDIO pads are set up */ tegra_mmc_pad_init(priv);
- if (priv->tap_value || priv->trim_value) {
I think 0 is a valid value for both tap and trim, so you want to be able to write that. I suggest getting rid of the conditional and always writing these values and rely on defaults to make sure that a good value is always programmed.
Tegra 3 uses 0x0F on all my devices which is not 0.
Alternatively if you really only want to program these when they've been specified, use an extra variable (or something like -1 as a default value) to discriminate.
Why 0 value cannot be used as "flag" to skip tap and trim setup if values are not set? Using any value but 0 will cause additional comparation in condition and will definitely not improve readability.
If you use 0 as a flag it implies that you can never set 0 as a value explicitly. So if some SoC or board ever needs to set that exact value it can't.
I got this same thought after I sent this answer. It is valid point and use of extra variable seems valid. Thanks.
Thierry
23 серпня 2023 р. 14:03:25 GMT+03:00, Thierry Reding thierry.reding@gmail.com написав(-ла):
On Sat, Aug 19, 2023 at 06:35:01PM +0300, Svyatoslav Ryhel wrote:
Default-tap and default-trim values are used for eMMC setup mostly on T114+ devices. As for now, those values are hardcoded for T210 and ignored for all other Tegra generations. Fix this by passing tap and trim values from dts.
Tested-by: Svyatoslav Ryhel clamor95@gmail.com # ASUS TF701T Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
arch/arm/include/asm/arch-tegra/tegra_mmc.h | 17 ++++---- drivers/mmc/tegra_mmc.c | 46 ++++++++++----------- 2 files changed, 30 insertions(+), 33 deletions(-)
diff --git a/arch/arm/include/asm/arch-tegra/tegra_mmc.h b/arch/arm/include/asm/arch-tegra/tegra_mmc.h index d6a55764ba..750c7d809e 100644 --- a/arch/arm/include/asm/arch-tegra/tegra_mmc.h +++ b/arch/arm/include/asm/arch-tegra/tegra_mmc.h @@ -128,21 +128,22 @@ struct tegra_mmc {
/* SDMMC1/3 settings from SDMMCx Initialization Sequence of TRM */ #define MEMCOMP_PADCTRL_VREF 7 -#define AUTO_CAL_ENABLE (1 << 29) -#define AUTO_CAL_ACTIVE (1 << 31) -#define AUTO_CAL_START (1 << 31) +#define AUTO_CAL_ENABLE BIT(29) +#define AUTO_CAL_ACTIVE BIT(31) +#define AUTO_CAL_START BIT(31)
#if defined(CONFIG_TEGRA210) #define AUTO_CAL_PD_OFFSET (0x7D << 8) #define AUTO_CAL_PU_OFFSET (0 << 0) -#define IO_TRIM_BYPASS_MASK (1 << 2) -#define TRIM_VAL_SHIFT 24 -#define TRIM_VAL_MASK (0x1F << TRIM_VAL_SHIFT) -#define TAP_VAL_SHIFT 16 -#define TAP_VAL_MASK (0xFF << TAP_VAL_SHIFT) #else #define AUTO_CAL_PD_OFFSET (0x70 << 8) #define AUTO_CAL_PU_OFFSET (0x62 << 0) #endif
+#define TRIM_VAL_SHIFT 24 +#define TRIM_VAL_MASK (0x1F << TRIM_VAL_SHIFT) +#define TAP_VAL_SHIFT 16 +#define TAP_VAL_MASK (0xFF << TAP_VAL_SHIFT)
#endif /* __ASSEMBLY__ */ #endif /* __TEGRA_MMC_H_ */ diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c index f76fee3ea0..7627800261 100644 --- a/drivers/mmc/tegra_mmc.c +++ b/drivers/mmc/tegra_mmc.c @@ -37,6 +37,9 @@ struct tegra_mmc_priv { unsigned int version; /* SDHCI spec. version */ unsigned int clock; /* Current clock (MHz) */ int mmc_id; /* peripheral id */
- u32 tap_value;
- u32 trim_value;
};
static void tegra_mmc_set_power(struct tegra_mmc_priv *priv, @@ -526,31 +529,6 @@ static void tegra_mmc_pad_init(struct tegra_mmc_priv *priv) printf("%s: Warning: Autocal timed out!\n", __func__); /* TBD: Set CFG2TMC_SDMMC1_PAD_CAL_DRV* regs here */ }
-#if defined(CONFIG_TEGRA210)
- u32 tap_value, trim_value;
- /* Set tap/trim values for SDMMC1/3 @ <48MHz here */
- val = readl(&priv->reg->venspictl); /* aka VENDOR_SYS_SW_CNTL */
- val &= IO_TRIM_BYPASS_MASK;
- if (id == PERIPH_ID_SDMMC1) {
tap_value = 4; /* default */if (val)tap_value = 3;trim_value = 2;- } else { /* SDMMC3 */
tap_value = 3;trim_value = 3;- }
- val = readl(&priv->reg->venclkctl);
- val &= ~TRIM_VAL_MASK;
- val |= (trim_value << TRIM_VAL_SHIFT);
- val &= ~TAP_VAL_MASK;
- val |= (tap_value << TAP_VAL_SHIFT);
- writel(val, &priv->reg->venclkctl);
- debug("%s: VENDOR_CLOCK_CNTRL = 0x%08X\n", __func__, val);
-#endif /* T210 */ #endif /* T30/T210 */ }
@@ -588,6 +566,21 @@ static void tegra_mmc_reset(struct tegra_mmc_priv *priv, struct mmc *mmc)
/* Make sure SDIO pads are set up */ tegra_mmc_pad_init(priv);
- if (priv->tap_value || priv->trim_value) {
I think 0 is a valid value for both tap and trim, so you want to be able to write that. I suggest getting rid of the conditional and always writing these values and rely on defaults to make sure that a good value is always programmed.
Alternatively if you really only want to program these when they've been specified, use an extra variable (or something like -1 as a default value) to discriminate.
I may propose a compromise. Do not set default value at all and when time comes just check if tap or trim is not error.
The former is superior, in my opinion, because it also allows to avoid to regression.
Thierry
u32 val;val = readl(&priv->reg->venclkctl);val &= ~TRIM_VAL_MASK;val |= (priv->trim_value << TRIM_VAL_SHIFT);val &= ~TAP_VAL_MASK;val |= (priv->tap_value << TAP_VAL_SHIFT);writel(val, &priv->reg->venclkctl);debug("%s: VENDOR_CLOCK_CNTRL = 0x%08X\n", __func__, val);- }
}
static int tegra_mmc_init(struct udevice *dev) @@ -742,6 +735,9 @@ static int tegra_mmc_probe(struct udevice *dev) if (dm_gpio_is_valid(&priv->pwr_gpio)) dm_gpio_set_value(&priv->pwr_gpio, 1);
priv->tap_value = dev_read_u32_default(dev, "nvidia,default-tap", 0);
priv->trim_value = dev_read_u32_default(dev, "nvidia,default-trim", 0);
upriv->mmc = &plat->mmc;
return tegra_mmc_init(dev);
-- 2.39.2
On Wed, Aug 23, 2023 at 03:02:42PM +0300, Svyatoslav Ryhel wrote:
23 серпня 2023 р. 14:03:25 GMT+03:00, Thierry Reding thierry.reding@gmail.com написав(-ла):
On Sat, Aug 19, 2023 at 06:35:01PM +0300, Svyatoslav Ryhel wrote:
Default-tap and default-trim values are used for eMMC setup mostly on T114+ devices. As for now, those values are hardcoded for T210 and ignored for all other Tegra generations. Fix this by passing tap and trim values from dts.
Tested-by: Svyatoslav Ryhel clamor95@gmail.com # ASUS TF701T Signed-off-by: Svyatoslav Ryhel clamor95@gmail.com
arch/arm/include/asm/arch-tegra/tegra_mmc.h | 17 ++++---- drivers/mmc/tegra_mmc.c | 46 ++++++++++----------- 2 files changed, 30 insertions(+), 33 deletions(-)
diff --git a/arch/arm/include/asm/arch-tegra/tegra_mmc.h b/arch/arm/include/asm/arch-tegra/tegra_mmc.h index d6a55764ba..750c7d809e 100644 --- a/arch/arm/include/asm/arch-tegra/tegra_mmc.h +++ b/arch/arm/include/asm/arch-tegra/tegra_mmc.h @@ -128,21 +128,22 @@ struct tegra_mmc {
/* SDMMC1/3 settings from SDMMCx Initialization Sequence of TRM */ #define MEMCOMP_PADCTRL_VREF 7 -#define AUTO_CAL_ENABLE (1 << 29) -#define AUTO_CAL_ACTIVE (1 << 31) -#define AUTO_CAL_START (1 << 31) +#define AUTO_CAL_ENABLE BIT(29) +#define AUTO_CAL_ACTIVE BIT(31) +#define AUTO_CAL_START BIT(31)
#if defined(CONFIG_TEGRA210) #define AUTO_CAL_PD_OFFSET (0x7D << 8) #define AUTO_CAL_PU_OFFSET (0 << 0) -#define IO_TRIM_BYPASS_MASK (1 << 2) -#define TRIM_VAL_SHIFT 24 -#define TRIM_VAL_MASK (0x1F << TRIM_VAL_SHIFT) -#define TAP_VAL_SHIFT 16 -#define TAP_VAL_MASK (0xFF << TAP_VAL_SHIFT) #else #define AUTO_CAL_PD_OFFSET (0x70 << 8) #define AUTO_CAL_PU_OFFSET (0x62 << 0) #endif
+#define TRIM_VAL_SHIFT 24 +#define TRIM_VAL_MASK (0x1F << TRIM_VAL_SHIFT) +#define TAP_VAL_SHIFT 16 +#define TAP_VAL_MASK (0xFF << TAP_VAL_SHIFT)
#endif /* __ASSEMBLY__ */ #endif /* __TEGRA_MMC_H_ */ diff --git a/drivers/mmc/tegra_mmc.c b/drivers/mmc/tegra_mmc.c index f76fee3ea0..7627800261 100644 --- a/drivers/mmc/tegra_mmc.c +++ b/drivers/mmc/tegra_mmc.c @@ -37,6 +37,9 @@ struct tegra_mmc_priv { unsigned int version; /* SDHCI spec. version */ unsigned int clock; /* Current clock (MHz) */ int mmc_id; /* peripheral id */
- u32 tap_value;
- u32 trim_value;
};
static void tegra_mmc_set_power(struct tegra_mmc_priv *priv, @@ -526,31 +529,6 @@ static void tegra_mmc_pad_init(struct tegra_mmc_priv *priv) printf("%s: Warning: Autocal timed out!\n", __func__); /* TBD: Set CFG2TMC_SDMMC1_PAD_CAL_DRV* regs here */ }
-#if defined(CONFIG_TEGRA210)
- u32 tap_value, trim_value;
- /* Set tap/trim values for SDMMC1/3 @ <48MHz here */
- val = readl(&priv->reg->venspictl); /* aka VENDOR_SYS_SW_CNTL */
- val &= IO_TRIM_BYPASS_MASK;
- if (id == PERIPH_ID_SDMMC1) {
tap_value = 4; /* default */if (val)tap_value = 3;trim_value = 2;- } else { /* SDMMC3 */
tap_value = 3;trim_value = 3;- }
- val = readl(&priv->reg->venclkctl);
- val &= ~TRIM_VAL_MASK;
- val |= (trim_value << TRIM_VAL_SHIFT);
- val &= ~TAP_VAL_MASK;
- val |= (tap_value << TAP_VAL_SHIFT);
- writel(val, &priv->reg->venclkctl);
- debug("%s: VENDOR_CLOCK_CNTRL = 0x%08X\n", __func__, val);
-#endif /* T210 */ #endif /* T30/T210 */ }
@@ -588,6 +566,21 @@ static void tegra_mmc_reset(struct tegra_mmc_priv *priv, struct mmc *mmc)
/* Make sure SDIO pads are set up */ tegra_mmc_pad_init(priv);
- if (priv->tap_value || priv->trim_value) {
I think 0 is a valid value for both tap and trim, so you want to be able to write that. I suggest getting rid of the conditional and always writing these values and rely on defaults to make sure that a good value is always programmed.
Alternatively if you really only want to program these when they've been specified, use an extra variable (or something like -1 as a default value) to discriminate.
I may propose a compromise. Do not set default value at all and when time comes just check if tap or trim is not error.
Yes, that's essentially a variant of the second option and it should work.
Thierry
On Sat, Aug 19, 2023 at 06:35:00PM +0300, Svyatoslav Ryhel wrote:
Default-tap and default-trim values are used for eMMC setup mostly on T114+ devices. As for now, those values are hardcoded for T210 and ignored for all other Tegra generations. Fix this by passing tap and trim values from dts.
This commit will cause regression on T210 (emmc may not work), fix is applied in tegra210.dtsi and will be sent next week.
Heh... I don't think so. If you already know that this will cause a regression on Tegra210 you need to rework this. Adding regressions accidentally is already bad enough, but doing so knowingly is a big no-no.
Thierry
23 серпня 2023 р. 13:53:26 GMT+03:00, Thierry Reding thierry.reding@gmail.com написав(-ла):
On Sat, Aug 19, 2023 at 06:35:00PM +0300, Svyatoslav Ryhel wrote:
Default-tap and default-trim values are used for eMMC setup mostly on T114+ devices. As for now, those values are hardcoded for T210 and ignored for all other Tegra generations. Fix this by passing tap and trim values from dts.
This commit will cause regression on T210 (emmc may not work), fix is applied in tegra210.dtsi and will be sent next week.
Heh... I don't think so. If you already know that this will cause a regression on Tegra210 you need to rework this. Adding regressions accidentally is already bad enough, but doing so knowingly is a big no-no.
DTS change for t210 was sent in "General tegra and board improvements" patchset. This is why this pathset contains only 1 (one) patch and a warning. It has a dependency.
Thierry
On Wed, Aug 23, 2023 at 02:30:48PM +0300, Svyatoslav Ryhel wrote:
23 серпня 2023 р. 13:53:26 GMT+03:00, Thierry Reding thierry.reding@gmail.com написав(-ла):
On Sat, Aug 19, 2023 at 06:35:00PM +0300, Svyatoslav Ryhel wrote:
Default-tap and default-trim values are used for eMMC setup mostly on T114+ devices. As for now, those values are hardcoded for T210 and ignored for all other Tegra generations. Fix this by passing tap and trim values from dts.
This commit will cause regression on T210 (emmc may not work), fix is applied in tegra210.dtsi and will be sent next week.
Heh... I don't think so. If you already know that this will cause a regression on Tegra210 you need to rework this. Adding regressions accidentally is already bad enough, but doing so knowingly is a big no-no.
DTS change for t210 was sent in "General tegra and board improvements" patchset. This is why this pathset contains only 1 (one) patch and a warning. It has a dependency.
For U-Boot this may not matter as much because the control DTB is always linked into the binary, but for Linux for example we need to make sure that the kernel always remains backwards compatible with older device trees. I think that's good to follow in general, so making sure there are sensible defaults to fall back to if the DTB is missing some properties is good practice.
Anyway, the way you posted these there was a future dependency, so if someone had applied the driver change without the DTB change having been applied first this would've caused a regression, which is always bad if it happens at random points because it makes things like bisections very painful.
So unless the patch is fixed to fall back to defaults for Tegra210 we need to carefully coordinate in which order these patches go in. The DT change needs to go in first, followed by the driver change that relies on the DTB update.
Thierry
24 серпня 2023 р. 16:27:19 GMT+03:00, Thierry Reding thierry.reding@gmail.com написав(-ла):
On Wed, Aug 23, 2023 at 02:30:48PM +0300, Svyatoslav Ryhel wrote:
23 серпня 2023 р. 13:53:26 GMT+03:00, Thierry Reding thierry.reding@gmail.com написав(-ла):
On Sat, Aug 19, 2023 at 06:35:00PM +0300, Svyatoslav Ryhel wrote:
Default-tap and default-trim values are used for eMMC setup mostly on T114+ devices. As for now, those values are hardcoded for T210 and ignored for all other Tegra generations. Fix this by passing tap and trim values from dts.
This commit will cause regression on T210 (emmc may not work), fix is applied in tegra210.dtsi and will be sent next week.
Heh... I don't think so. If you already know that this will cause a regression on Tegra210 you need to rework this. Adding regressions accidentally is already bad enough, but doing so knowingly is a big no-no.
DTS change for t210 was sent in "General tegra and board improvements" patchset. This is why this pathset contains only 1 (one) patch and a warning. It has a dependency.
For U-Boot this may not matter as much because the control DTB is always linked into the binary, but for Linux for example we need to make sure that the kernel always remains backwards compatible with older device trees. I think that's good to follow in general, so making sure there are sensible defaults to fall back to if the DTB is missing some properties is good practice.
Anyway, the way you posted these there was a future dependency, so if someone had applied the driver change without the DTB change having been applied first this would've caused a regression, which is always bad if it happens at random points because it makes things like bisections very painful.
So unless the patch is fixed to fall back to defaults for Tegra210 we need to carefully coordinate in which order these patches go in. The DT change needs to go in first, followed by the driver change that relies on the DTB update.
You are right, it was my miss that I did not include tegra210 fix here. I will add it in v2. Thanks.
Thierry
participants (2)
-
Svyatoslav Ryhel -
Thierry Reding