On 11/10/21 7:47 AM, Sean Anderson wrote:
> 
> 
> On 11/9/21 5:41 PM, Jaehoon Chung wrote:
>> 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...@nxp.com>
>>>>> Signed-off-by: Sean Anderson <sean.ander...@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"
> 
> It's not that they set the width to an invalid value, but rather that
> they do not set it at all. For example, in
> board/freescale/mx6sabresd/mx6sabresd.c, the config is created like
> 
>     struct fsl_esdhc_cfg usdhc_cfg[3] = {
>         {USDHC2_BASE_ADDR},
>         {USDHC3_BASE_ADDR},
>         {USDHC4_BASE_ADDR},
>     };
> 
> because max_bus_width is not explicitly initialized, it defaults to 0.
> Perhaps better logic would be something like
> 
>     switch (cfg->max_bus_width) {
>     case 0: /* unset, support everything */
>     case 8:
>         mmc_cfg->host_caps |= MMC_MODE_8BIT;
>     case 4:
>         mmc_cfg->host_caps |= MMC_MODE_4BIT;
>     case 1:
>         mmc_cfg->host_caps |= MMC_MODE_1BIT;
>         break;
>     default:
>         log_err("Invalid bus width %u\n", cfg->max_bus_width);
>         return -EINVAL;
>     }
> 
> which would likely preserve compatibility for existing users.

Agreed.

Best Regards,
Jaehoon Chung

> 
> --Sean
> 

Reply via email to