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

Sean Anderson sean.anderson at seco.com
Tue Nov 9 23:47:19 CET 2021



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

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.

--Sean


More information about the U-Boot mailing list