Re: [PATCH v1 1/1] mmc: tegra: get default-tap and default-trim from device tree

[Thierry Reding <thierry.reding@gmail.com>]

[-- 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 --]