[PATCH 04/11] mmc: fsl_esdhc_imx: clean up bus width configuration code
Jaehoon Chung
jh80.chung at samsung.com
Wed Nov 10 00:14:11 CET 2021
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.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.
Agreed.
Best Regards,
Jaehoon Chung
>
> --Sean
>
More information about the U-Boot
mailing list