| Message ID | 20211105173945.92446-5-sean.anderson@seco.com |
|---|---|
| State | Superseded |
| Delegated to: | Peng Fan |
| Headers | show |
| Series | fsl_esdhc_imx: port several patches from fsl_esdhc | expand |
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@nxp.com> > Signed-off-by: Sean Anderson <sean.anderson@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. > } > > +#ifdef CONFIG_ESDHC_DETECT_8_BIT_QUIRK > + if (CONFIG_ESDHC_DETECT_8_BIT_QUIRK) How about also changing this at this time? if (IS_ENABLED(CONFIG_ESDCH..))? Best Regards, Jaehoon Chung > + mmc_cfg->host_caps &= ~MMC_MODE_8BIT; > +#endif > + > ret = fsl_esdhc_init(priv, plat); > if (ret) { > debug("%s init failure\n", __func__); > @@ -1416,14 +1398,6 @@ static int fsl_esdhc_of_to_plat(struct udevice *dev) > priv->dev = dev; > priv->mode = -1; > > - val = dev_read_u32_default(dev, "bus-width", -1); > - if (val == 8) > - priv->bus_width = 8; > - else if (val == 4) > - priv->bus_width = 4; > - else > - priv->bus_width = 1; > - > val = fdtdec_get_int(fdt, node, "fsl,tuning-step", 1); > priv->tuning_step = val; > val = fdtdec_get_int(fdt, node, "fsl,tuning-start-tap", > @@ -1496,16 +1470,8 @@ static int fsl_esdhc_probe(struct udevice *dev) > > #if CONFIG_IS_ENABLED(OF_PLATDATA) > struct dtd_fsl_esdhc *dtplat = &plat->dtplat; > - unsigned int val; > > priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]); > - val = plat->dtplat.bus_width; > - if (val == 8) > - priv->bus_width = 8; > - else if (val == 4) > - priv->bus_width = 4; > - else > - priv->bus_width = 1; > > if (dtplat->non_removable) > priv->non_removable = 1; >
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@nxp.com> >> Signed-off-by: Sean Anderson <sean.anderson@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. >> } >> >> +#ifdef CONFIG_ESDHC_DETECT_8_BIT_QUIRK >> + if (CONFIG_ESDHC_DETECT_8_BIT_QUIRK) > > How about also changing this at this time? > > if (IS_ENABLED(CONFIG_ESDCH..))? > This is left for patch 10/11 to keep things aligned with the original commits. --Sean
On Fri, Nov 5, 2021 at 12:41 PM Sean Anderson <sean.anderson@seco.com> 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@nxp.com> > Signed-off-by: Sean Anderson <sean.anderson@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"); I wonder if a switch statement would be cleaner looking, and the 8-bit quirk can be integrated into the case 8 statement to something like: switch (cfg->max_bus_width) case 8: if (!CONFIG_IS_ENABLED(ESDHC_DETECT_8_BIT_QUIRK) mmc_cfg->host_caps |= MMC_MODE_8BIT; fallthrough; case 4: mmc_cfg->host_caps |= MMC_MODE_4BIT; fallthrough; case 1: mmc_cfg->host_caps |= MMC_MODE_1BIT; break; default: mmc_cfg->host_caps |= MMC_MODE_1BIT | MMC_MODE_4BIT | MMC_MODE_8BIT; printf("No max bus width provided. Assume 8-bit supported.\n"); } > > +#ifdef CONFIG_ESDHC_DETECT_8_BIT_QUIRK > + if (CONFIG_ESDHC_DETECT_8_BIT_QUIRK) > + mmc_cfg->host_caps &= ~MMC_MODE_8BIT; > +#endif > + > ret = fsl_esdhc_init(priv, plat); > if (ret) { > debug("%s init failure\n", __func__); > @@ -1416,14 +1398,6 @@ static int fsl_esdhc_of_to_plat(struct udevice *dev) > priv->dev = dev; > priv->mode = -1; > > - val = dev_read_u32_default(dev, "bus-width", -1); > - if (val == 8) > - priv->bus_width = 8; > - else if (val == 4) > - priv->bus_width = 4; > - else > - priv->bus_width = 1; > - > val = fdtdec_get_int(fdt, node, "fsl,tuning-step", 1); > priv->tuning_step = val; > val = fdtdec_get_int(fdt, node, "fsl,tuning-start-tap", > @@ -1496,16 +1470,8 @@ static int fsl_esdhc_probe(struct udevice *dev) > > #if CONFIG_IS_ENABLED(OF_PLATDATA) > struct dtd_fsl_esdhc *dtplat = &plat->dtplat; > - unsigned int val; > > priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]); > - val = plat->dtplat.bus_width; > - if (val == 8) > - priv->bus_width = 8; > - else if (val == 4) > - priv->bus_width = 4; > - else > - priv->bus_width = 1; > > if (dtplat->non_removable) > priv->non_removable = 1; > -- > 2.25.1 >
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@nxp.com> >>> Signed-off-by: Sean Anderson <sean.anderson@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" > >>> } >>> >>> +#ifdef CONFIG_ESDHC_DETECT_8_BIT_QUIRK >>> + if (CONFIG_ESDHC_DETECT_8_BIT_QUIRK) >> >> How about also changing this at this time? >> >> if (IS_ENABLED(CONFIG_ESDCH..))? >> > > This is left for patch 10/11 to keep things aligned with the original > commits. Thanks. I have checked it in patch 10/11. Best Regards, Jaehoon Chung > > --Sean >
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@nxp.com> >>>> Signed-off-by: Sean Anderson <sean.anderson@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
On 11/10/21 1:13 AM, Adam Ford wrote: > On Fri, Nov 5, 2021 at 12:41 PM Sean Anderson <sean.anderson@seco.com> 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@nxp.com> >> Signed-off-by: Sean Anderson <sean.anderson@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"); > > I wonder if a switch statement would be cleaner looking, and the 8-bit > quirk can be integrated into the case 8 statement to something like: Agreed. > > switch (cfg->max_bus_width) > > case 8: > if (!CONFIG_IS_ENABLED(ESDHC_DETECT_8_BIT_QUIRK) > mmc_cfg->host_caps |= MMC_MODE_8BIT; I think that quirk code needs to be located in outside of switch state if default is using all buswidth modes. > fallthrough; > case 4: > mmc_cfg->host_caps |= MMC_MODE_4BIT; > fallthrough; > case 1: > mmc_cfg->host_caps |= MMC_MODE_1BIT; > break; > default: > mmc_cfg->host_caps |= MMC_MODE_1BIT | MMC_MODE_4BIT | > MMC_MODE_8BIT; > printf("No max bus width provided. Assume 8-bit supported.\n"); > } >> >> +#ifdef CONFIG_ESDHC_DETECT_8_BIT_QUIRK >> + if (CONFIG_ESDHC_DETECT_8_BIT_QUIRK) >> + mmc_cfg->host_caps &= ~MMC_MODE_8BIT; >> +#endif >> + >> ret = fsl_esdhc_init(priv, plat); >> if (ret) { >> debug("%s init failure\n", __func__); >> @@ -1416,14 +1398,6 @@ static int fsl_esdhc_of_to_plat(struct udevice *dev) >> priv->dev = dev; >> priv->mode = -1; >> >> - val = dev_read_u32_default(dev, "bus-width", -1); >> - if (val == 8) >> - priv->bus_width = 8; >> - else if (val == 4) >> - priv->bus_width = 4; >> - else >> - priv->bus_width = 1; >> - >> val = fdtdec_get_int(fdt, node, "fsl,tuning-step", 1); >> priv->tuning_step = val; >> val = fdtdec_get_int(fdt, node, "fsl,tuning-start-tap", >> @@ -1496,16 +1470,8 @@ static int fsl_esdhc_probe(struct udevice *dev) >> >> #if CONFIG_IS_ENABLED(OF_PLATDATA) >> struct dtd_fsl_esdhc *dtplat = &plat->dtplat; >> - unsigned int val; >> >> priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]); >> - val = plat->dtplat.bus_width; >> - if (val == 8) >> - priv->bus_width = 8; >> - else if (val == 4) >> - priv->bus_width = 4; >> - else >> - priv->bus_width = 1; >> >> if (dtplat->non_removable) >> priv->non_removable = 1; >> -- >> 2.25.1 >> >
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@nxp.com> >>>>> Signed-off-by: Sean Anderson <sean.anderson@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 >
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"); } +#ifdef CONFIG_ESDHC_DETECT_8_BIT_QUIRK + if (CONFIG_ESDHC_DETECT_8_BIT_QUIRK) + mmc_cfg->host_caps &= ~MMC_MODE_8BIT; +#endif + ret = fsl_esdhc_init(priv, plat); if (ret) { debug("%s init failure\n", __func__); @@ -1416,14 +1398,6 @@ static int fsl_esdhc_of_to_plat(struct udevice *dev) priv->dev = dev; priv->mode = -1; - val = dev_read_u32_default(dev, "bus-width", -1); - if (val == 8) - priv->bus_width = 8; - else if (val == 4) - priv->bus_width = 4; - else - priv->bus_width = 1; - val = fdtdec_get_int(fdt, node, "fsl,tuning-step", 1); priv->tuning_step = val; val = fdtdec_get_int(fdt, node, "fsl,tuning-start-tap", @@ -1496,16 +1470,8 @@ static int fsl_esdhc_probe(struct udevice *dev) #if CONFIG_IS_ENABLED(OF_PLATDATA) struct dtd_fsl_esdhc *dtplat = &plat->dtplat; - unsigned int val; priv->esdhc_regs = map_sysmem(dtplat->reg[0], dtplat->reg[1]); - val = plat->dtplat.bus_width; - if (val == 8) - priv->bus_width = 8; - else if (val == 4) - priv->bus_width = 4; - else - priv->bus_width = 1; if (dtplat->non_removable) priv->non_removable = 1;