Re: [PATCH v1 1/1] mmc: tegra: get default-tap and default-trim from device tree
Svyatoslav Ryhel
clamor95 at gmail.com
Thu Aug 24 15:33:20 CEST 2023
24 серпня 2023 р. 16:29:22 GMT+03:00, Thierry Reding <thierry.reding at 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 at 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 at gmail.com> # ASUS TF701T
>> >> Signed-off-by: Svyatoslav Ryhel <clamor95 at 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
More information about the U-Boot
mailing list