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

[Jaehoon Chung]

Hi Sean,

On 11/9/21 11:42 PM, Sean Anderson wrote:
> 
> 
> 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.

Thanks for kindly explanation. 

I didn't know that fsl_esdhc is sharing this logic.
If max_bus_width is set to wrong value, then user needs to know that it's using invalid value.
But It will be working fine with default capabilities.
(It will be working with one buswidth mode of them.)

AFAIK, it's using 1-bit buswith in mmc_of_parse() of kernel and u-boot when user is setting to invalid buswidth value.
Of course, performance regression will be caused,  if 1bit buswidth is using by default when invalid max_bus_width value is set.
And I think that If fsl_esdhc is sharing this logic, it doesn't need to use max_bus_width.

If fsh_esdhc is using this logic, I want to change a message more clear than now.
"No max bus with" -> "Invalid max_ bus with"

> 
>>>      }
>>>
>>> +#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.

Thanks. I have checked it in patch 10/11.

Best Regards,
Jaehoon Chung

> 
> --Sean
>