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

[Svyatoslav Ryhel]

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.

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