[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