* [PATCH v1 0/1] tegra_mmc: get tap and trim from dts @ 2023-08-19 15:35 Svyatoslav Ryhel 2023-08-19 15:35 ` [PATCH v1 1/1] mmc: tegra: get default-tap and default-trim from device tree Svyatoslav Ryhel 2023-08-23 10:53 ` [PATCH v1 0/1] tegra_mmc: get tap and trim from dts Thierry Reding 0 siblings, 2 replies; 12+ messages in thread From: Svyatoslav Ryhel @ 2023-08-19 15:35 UTC (permalink / raw) To: Thierry Reding, Peng Fan, Jaehoon Chung, Svyatoslav Ryhel; +Cc: u-boot 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(-) -- 2.39.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1 1/1] mmc: tegra: get default-tap and default-trim from device tree 2023-08-19 15:35 [PATCH v1 0/1] tegra_mmc: get tap and trim from dts Svyatoslav Ryhel @ 2023-08-19 15:35 ` Svyatoslav Ryhel 2023-08-23 11:03 ` Thierry Reding 2023-08-23 10:53 ` [PATCH v1 0/1] tegra_mmc: get tap and trim from dts Thierry Reding 1 sibling, 1 reply; 12+ messages in thread From: Svyatoslav Ryhel @ 2023-08-19 15:35 UTC (permalink / raw) To: Thierry Reding, Peng Fan, Jaehoon Chung, Svyatoslav Ryhel; +Cc: u-boot 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); -- 2.39.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/1] mmc: tegra: get default-tap and default-trim from device tree 2023-08-19 15:35 ` [PATCH v1 1/1] mmc: tegra: get default-tap and default-trim from device tree Svyatoslav Ryhel @ 2023-08-23 11:03 ` Thierry Reding 2023-08-23 11:38 ` Svyatoslav Ryhel 2023-08-23 12:02 ` Svyatoslav Ryhel 0 siblings, 2 replies; 12+ messages in thread From: Thierry Reding @ 2023-08-23 11:03 UTC (permalink / raw) To: Svyatoslav Ryhel; +Cc: Thierry Reding, Peng Fan, Jaehoon Chung, u-boot [-- Attachment #1: Type: text/plain, Size: 4989 bytes --] 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 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/1] mmc: tegra: get default-tap and default-trim from device tree 2023-08-23 11:03 ` Thierry Reding @ 2023-08-23 11:38 ` Svyatoslav Ryhel 2023-08-24 13:29 ` Thierry Reding 2023-08-23 12:02 ` Svyatoslav Ryhel 1 sibling, 1 reply; 12+ messages in thread From: Svyatoslav Ryhel @ 2023-08-23 11:38 UTC (permalink / raw) To: Thierry Reding; +Cc: Thierry Reding, Peng Fan, Jaehoon Chung, u-boot 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 >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/1] mmc: tegra: get default-tap and default-trim from device tree 2023-08-23 11:38 ` Svyatoslav Ryhel @ 2023-08-24 13:29 ` Thierry Reding 2023-08-24 13:33 ` Svyatoslav Ryhel 0 siblings, 1 reply; 12+ messages in thread From: Thierry Reding @ 2023-08-24 13:29 UTC (permalink / raw) To: Svyatoslav Ryhel; +Cc: Thierry Reding, Peng Fan, Jaehoon Chung, u-boot [-- Attachment #1: Type: text/plain, Size: 5013 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/1] mmc: tegra: get default-tap and default-trim from device tree 2023-08-24 13:29 ` Thierry Reding @ 2023-08-24 13:33 ` Svyatoslav Ryhel 0 siblings, 0 replies; 12+ messages in thread From: Svyatoslav Ryhel @ 2023-08-24 13:33 UTC (permalink / raw) To: Thierry Reding; +Cc: Thierry Reding, Peng Fan, Jaehoon Chung, u-boot 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/1] mmc: tegra: get default-tap and default-trim from device tree 2023-08-23 11:03 ` Thierry Reding 2023-08-23 11:38 ` Svyatoslav Ryhel @ 2023-08-23 12:02 ` Svyatoslav Ryhel 2023-08-24 13:30 ` Thierry Reding 1 sibling, 1 reply; 12+ messages in thread From: Svyatoslav Ryhel @ 2023-08-23 12:02 UTC (permalink / raw) To: Thierry Reding; +Cc: Thierry Reding, Peng Fan, Jaehoon Chung, u-boot 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 >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/1] mmc: tegra: get default-tap and default-trim from device tree 2023-08-23 12:02 ` Svyatoslav Ryhel @ 2023-08-24 13:30 ` Thierry Reding 0 siblings, 0 replies; 12+ messages in thread From: Thierry Reding @ 2023-08-24 13:30 UTC (permalink / raw) To: Svyatoslav Ryhel; +Cc: Thierry Reding, Peng Fan, Jaehoon Chung, u-boot [-- Attachment #1: Type: text/plain, Size: 4797 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 0/1] tegra_mmc: get tap and trim from dts 2023-08-19 15:35 [PATCH v1 0/1] tegra_mmc: get tap and trim from dts Svyatoslav Ryhel 2023-08-19 15:35 ` [PATCH v1 1/1] mmc: tegra: get default-tap and default-trim from device tree Svyatoslav Ryhel @ 2023-08-23 10:53 ` Thierry Reding 2023-08-23 11:30 ` Svyatoslav Ryhel 1 sibling, 1 reply; 12+ messages in thread From: Thierry Reding @ 2023-08-23 10:53 UTC (permalink / raw) To: Svyatoslav Ryhel; +Cc: Thierry Reding, Peng Fan, Jaehoon Chung, u-boot [-- Attachment #1: Type: text/plain, Size: 667 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 0/1] tegra_mmc: get tap and trim from dts 2023-08-23 10:53 ` [PATCH v1 0/1] tegra_mmc: get tap and trim from dts Thierry Reding @ 2023-08-23 11:30 ` Svyatoslav Ryhel 2023-08-24 13:27 ` Thierry Reding 0 siblings, 1 reply; 12+ messages in thread From: Svyatoslav Ryhel @ 2023-08-23 11:30 UTC (permalink / raw) To: Thierry Reding; +Cc: Thierry Reding, Peng Fan, Jaehoon Chung, u-boot 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 0/1] tegra_mmc: get tap and trim from dts 2023-08-23 11:30 ` Svyatoslav Ryhel @ 2023-08-24 13:27 ` Thierry Reding 2023-08-24 13:31 ` Svyatoslav Ryhel 0 siblings, 1 reply; 12+ messages in thread From: Thierry Reding @ 2023-08-24 13:27 UTC (permalink / raw) To: Svyatoslav Ryhel; +Cc: Thierry Reding, Peng Fan, Jaehoon Chung, u-boot [-- Attachment #1: Type: text/plain, Size: 2005 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 0/1] tegra_mmc: get tap and trim from dts 2023-08-24 13:27 ` Thierry Reding @ 2023-08-24 13:31 ` Svyatoslav Ryhel 0 siblings, 0 replies; 12+ messages in thread From: Svyatoslav Ryhel @ 2023-08-24 13:31 UTC (permalink / raw) To: Thierry Reding; +Cc: Thierry Reding, Peng Fan, Jaehoon Chung, u-boot 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-08-24 13:33 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-08-19 15:35 [PATCH v1 0/1] tegra_mmc: get tap and trim from dts Svyatoslav Ryhel 2023-08-19 15:35 ` [PATCH v1 1/1] mmc: tegra: get default-tap and default-trim from device tree Svyatoslav Ryhel 2023-08-23 11:03 ` Thierry Reding 2023-08-23 11:38 ` Svyatoslav Ryhel 2023-08-24 13:29 ` Thierry Reding 2023-08-24 13:33 ` Svyatoslav Ryhel 2023-08-23 12:02 ` Svyatoslav Ryhel 2023-08-24 13:30 ` Thierry Reding 2023-08-23 10:53 ` [PATCH v1 0/1] tegra_mmc: get tap and trim from dts Thierry Reding 2023-08-23 11:30 ` Svyatoslav Ryhel 2023-08-24 13:27 ` Thierry Reding 2023-08-24 13:31 ` Svyatoslav Ryhel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox