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 >