[PATCH 04/11] mmc: fsl_esdhc_imx: clean up bus width configuration code

[Sean Anderson]

On 11/9/21 2:19 AM, Jaehoon Chung wrote:
> On 11/6/21 2:39 AM, Sean Anderson wrote:
>> [ fsl_esdhc commit 07bae1de382723b94244096953b05225572728cd ]
>>
>> This patch is to clean up bus width setting code.
>>
>> - For DM_MMC, remove getting "bus-width" from device tree.
>>   This has been done in mmc_of_parse().
>>
>> - For non-DM_MMC, move bus width configuration from fsl_esdhc_init()
>>   to fsl_esdhc_initialize() which is non-DM_MMC specific.
>>   And fix up bus width configuration to support only 1-bit, 4-bit,
>>   or 8-bit. Keep using 8-bit if it's not set because many platforms
>>   use driver without providing max bus width.
>>
>> - Remove bus_width member from fsl_esdhc_priv structure.
>>
>> Signed-off-by: Yangbo Lu <yangbo.lu at nxp.com>
>> Signed-off-by: Sean Anderson <sean.anderson at seco.com>
>> ---
>>
>>  drivers/mmc/fsl_esdhc_imx.c | 80 +++++++++++--------------------------
>>  1 file changed, 23 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c
>> index b91dda27f9..d3daf4db59 100644
>> --- a/drivers/mmc/fsl_esdhc_imx.c
>> +++ b/drivers/mmc/fsl_esdhc_imx.c
>> @@ -126,7 +126,6 @@ struct esdhc_soc_data {
>>   *
>>   * @esdhc_regs: registers of the sdhc controller
>>   * @sdhc_clk: Current clk of the sdhc controller
>> - * @bus_width: bus width, 1bit, 4bit or 8bit
>>   * @cfg: mmc config
>>   * @mmc: mmc
>>   * Following is used when Driver Model is enabled for MMC
>> @@ -151,7 +150,6 @@ struct fsl_esdhc_priv {
>>  	struct clk per_clk;
>>  	unsigned int clock;
>>  	unsigned int mode;
>> -	unsigned int bus_width;
>>  #if !CONFIG_IS_ENABLED(DM_MMC)
>>  	struct mmc *mmc;
>>  #endif
>> @@ -1232,31 +1230,13 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv,
>>  #if !CONFIG_IS_ENABLED(DM_MMC)
>>  	cfg->ops = &esdhc_ops;
>>  #endif
>> -	if (priv->bus_width == 8)
>> -		cfg->host_caps = MMC_MODE_4BIT | MMC_MODE_8BIT;
>> -	else if (priv->bus_width == 4)
>> -		cfg->host_caps = MMC_MODE_4BIT;
>> -
>> -	cfg->host_caps = MMC_MODE_4BIT | MMC_MODE_8BIT;
>>  #ifdef CONFIG_SYS_FSL_ESDHC_HAS_DDR_MODE
>>  	cfg->host_caps |= MMC_MODE_DDR_52MHz;
>>  #endif
>>
>> -	if (priv->bus_width > 0) {
>> -		if (priv->bus_width < 8)
>> -			cfg->host_caps &= ~MMC_MODE_8BIT;
>> -		if (priv->bus_width < 4)
>> -			cfg->host_caps &= ~MMC_MODE_4BIT;
>> -	}
>> -
>>  	if (caps & HOSTCAPBLT_HSS)
>>  		cfg->host_caps |= MMC_MODE_HS_52MHz | MMC_MODE_HS;
>>
>> -#ifdef CONFIG_ESDHC_DETECT_8_BIT_QUIRK
>> -	if (CONFIG_ESDHC_DETECT_8_BIT_QUIRK)
>> -		cfg->host_caps &= ~MMC_MODE_8BIT;
>> -#endif
>> -
>>  	cfg->host_caps |= priv->caps;
>>
>>  	cfg->f_min = 400000;
>> @@ -1294,25 +1274,11 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv,
>>  }
>>
>>  #if !CONFIG_IS_ENABLED(DM_MMC)
>> -static int fsl_esdhc_cfg_to_priv(struct fsl_esdhc_cfg *cfg,
>> -				 struct fsl_esdhc_priv *priv)
>> -{
>> -	if (!cfg || !priv)
>> -		return -EINVAL;
>> -
>> -	priv->esdhc_regs = (struct fsl_esdhc *)(unsigned long)(cfg->esdhc_base);
>> -	priv->bus_width = cfg->max_bus_width;
>> -	priv->sdhc_clk = cfg->sdhc_clk;
>> -	priv->wp_enable  = cfg->wp_enable;
>> -	priv->vs18_enable  = cfg->vs18_enable;
>> -
>> -	return 0;
>> -};
>> -
>>  int fsl_esdhc_initialize(struct bd_info *bis, struct fsl_esdhc_cfg *cfg)
>>  {
>>  	struct fsl_esdhc_plat *plat;
>>  	struct fsl_esdhc_priv *priv;
>> +	struct mmc_config *mmc_cfg;
>>  	struct mmc *mmc;
>>  	int ret;
>>
>> @@ -1328,14 +1294,30 @@ int fsl_esdhc_initialize(struct bd_info *bis, struct fsl_esdhc_cfg *cfg)
>>  		return -ENOMEM;
>>  	}
>>
>> -	ret = fsl_esdhc_cfg_to_priv(cfg, priv);
>> -	if (ret) {
>> -		debug("%s xlate failure\n", __func__);
>> -		free(plat);
>> -		free(priv);
>> -		return ret;
>> +	priv->esdhc_regs = (struct fsl_esdhc *)(unsigned long)(cfg->esdhc_base);
>> +	priv->sdhc_clk = cfg->sdhc_clk;
>> +	priv->wp_enable  = cfg->wp_enable;
>> +
>> +	mmc_cfg = &plat->cfg;
>> +
>> +	if (cfg->max_bus_width == 8) {
>> +		mmc_cfg->host_caps |= MMC_MODE_1BIT | MMC_MODE_4BIT |
>> +				      MMC_MODE_8BIT;
>> +	} else if (cfg->max_bus_width == 4) {
>> +		mmc_cfg->host_caps |= MMC_MODE_1BIT | MMC_MODE_4BIT;
>> +	} else if (cfg->max_bus_width == 1) {
>> +		mmc_cfg->host_caps |= MMC_MODE_1BIT;
>> +	} else {
>> +		mmc_cfg->host_caps |= MMC_MODE_1BIT | MMC_MODE_4BIT |
>> +				      MMC_MODE_8BIT;
>> +		printf("No max bus width provided. Assume 8-bit supported.\n");
>
> Why assume that 8-bit buswidth is supported? It needs to display an invalid max_bus_width value.
> And set to 1-Bit mode by default.

This is just setting the capabilities of the host. The actual bus width
selected depends on the card which is plugged in. See e.g.
mmc_select_mode_and_width where the card caps are limited by the host
caps. Many users of this driver don't set max_bus_width, so changing
this default would likely cause a performance regression. Note that
fsl_esdhc also shares this logic.

>>  	}
>>
>> +#ifdef CONFIG_ESDHC_DETECT_8_BIT_QUIRK
>> +	if (CONFIG_ESDHC_DETECT_8_BIT_QUIRK)
>
> How about also changing this at this time?
>
> if (IS_ENABLED(CONFIG_ESDCH..))?
>

This is left for patch 10/11 to keep things aligned with the original
commits.

--Sean