[PATCH 00/11] fsl_esdhc_imx: port several patches from fsl_esdhc
This series ports some of the patches from fsl_esdhc to fsl_esdhc_imx. Because these drivers share a common lineage, many of these patches apply with minor changes. For each one, I have noted the originating commit in the style of linux stable backports.
In fa33d20749 ("mmc: split fsl_esdhc driver for i.MX"), Yangbo says
For the two series processors, the eSDHCs are becoming more and more different
However, these drivers are still extremely similar; the differences between them are not major. However, NXP has not done a good job of porting patches which apply to both drivers. This causes the fsl_esdhc_imx driver to rot, as the fsl_esdhc gets more general fixes. For this reason, I think that the fsl_esdhc_imx driver should be removed unless NXP can commit to creating series like this which port patches which apply to both drivers.
Michael Walle (4): mmc: fsl_esdhc_imx: simplify 64bit check for SDMA transfers mmc: fsl_esdhc_imx: use dma-mapping API mmc: fsl_esdhc_imx: simplify esdhc_setup_data() mmc: fsl_esdhc_imx: replace most #ifdefs by IS_ENABLED()
Sean Anderson (5): mmc: fsl_esdhc_imx: make BLK as hard requirement of DM_MMC mmc: fsl_esdhc_imx: remove redundant DM_MMC checking mmc: fsl_esdhc_imx: fix voltage validation mmc: fsl_esdhc_imx: clean up bus width configuration code mmc: fsl_esdhc_imx: fix mmc->clock with actual clock
Yangbo Lu (2): mmc: fsl_esdhc_imx: drop redundant code for non-removable feature mmc: fsl_esdhc_imx: set sysctl register for clock initialization
drivers/mmc/Kconfig | 2 + drivers/mmc/fsl_esdhc_imx.c | 484 ++++++++++++++---------------------- include/fsl_esdhc_imx.h | 14 +- 3 files changed, 191 insertions(+), 309 deletions(-)
[ fsl_esdhc commit 41dec2fe99512e941261594f522b2e7d485c314b ]
U-boot prefers DM_MMC + BLK for MMC. Now eSDHC driver has already support it, so let's force to use it.
- Drop non-BLK support for DM_MMC introduced by below patch. 66fa035 mmc: fsl_esdhc: fix probe issue without CONFIG_BLK enabled
- Support only DM_MMC + BLK (assuming BLK is always enabled for DM_MMC).
- Use DM_MMC instead of BLK for conditional compile.
Signed-off-by: Yangbo Lu yangbo.lu@nxp.com Signed-off-by: Sean Anderson sean.anderson@seco.com ---
drivers/mmc/Kconfig | 2 ++ drivers/mmc/fsl_esdhc_imx.c | 33 +-------------------------------- 2 files changed, 3 insertions(+), 32 deletions(-)
diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig index 1569e8c44a..313244682a 100644 --- a/drivers/mmc/Kconfig +++ b/drivers/mmc/Kconfig @@ -790,6 +790,7 @@ endif
config FSL_ESDHC bool "Freescale/NXP eSDHC controller support" + depends on BLK help This selects support for the eSDHC (Enhanced Secure Digital Host Controller) found on numerous Freescale/NXP SoCs. @@ -826,6 +827,7 @@ config FSL_ESDHC_VS33_NOT_SUPPORT
config FSL_ESDHC_IMX bool "Freescale/NXP i.MX eSDHC controller support" + depends on BLK help This selects support for the i.MX eSDHC (Enhanced Secure Digital Host Controller) found on numerous Freescale/NXP SoCs. diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index aabf39535f..0a7f7e61cb 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -39,10 +39,6 @@ #include <dm/ofnode.h> #include <linux/iopoll.h>
-#if !CONFIG_IS_ENABLED(BLK) -#include "mmc_private.h" -#endif - #ifndef ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE #ifdef CONFIG_FSL_USDHC #define ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE 1 @@ -58,7 +54,6 @@ DECLARE_GLOBAL_DATA_PTR; IRQSTATEN_DEBE | IRQSTATEN_BRR | IRQSTATEN_BWR | \ IRQSTATEN_DINT) #define MAX_TUNING_LOOP 40 -#define ESDHC_DRIVER_STAGE_VALUE 0xffffffff
struct fsl_esdhc { uint dsaddr; /* SDMA system address register */ @@ -157,7 +152,7 @@ struct fsl_esdhc_priv { unsigned int clock; unsigned int mode; unsigned int bus_width; -#if !CONFIG_IS_ENABLED(BLK) +#if !CONFIG_IS_ENABLED(DM_MMC) struct mmc *mmc; #endif struct udevice *dev; @@ -1506,9 +1501,6 @@ static int fsl_esdhc_probe(struct udevice *dev) struct esdhc_soc_data *data = (struct esdhc_soc_data *)dev_get_driver_data(dev); struct mmc *mmc; -#if !CONFIG_IS_ENABLED(BLK) - struct blk_desc *bdesc; -#endif int ret;
#if CONFIG_IS_ENABLED(OF_PLATDATA) @@ -1607,25 +1599,6 @@ static int fsl_esdhc_probe(struct udevice *dev) mmc = &plat->mmc; mmc->cfg = &plat->cfg; mmc->dev = dev; -#if !CONFIG_IS_ENABLED(BLK) - mmc->priv = priv; - - /* Setup dsr related values */ - mmc->dsr_imp = 0; - mmc->dsr = ESDHC_DRIVER_STAGE_VALUE; - /* Setup the universal parts of the block interface just once */ - bdesc = mmc_get_blk_desc(mmc); - bdesc->if_type = IF_TYPE_MMC; - bdesc->removable = 1; - bdesc->devnum = mmc_get_next_devnum(); - bdesc->block_read = mmc_bread; - bdesc->block_write = mmc_bwrite; - bdesc->block_erase = mmc_berase; - - /* setup initial part type */ - bdesc->part_type = mmc->cfg->part_type; - mmc_list_add(mmc); -#endif
upriv->mmc = mmc;
@@ -1730,14 +1703,12 @@ static const struct udevice_id fsl_esdhc_ids[] = { { /* sentinel */ } };
-#if CONFIG_IS_ENABLED(BLK) static int fsl_esdhc_bind(struct udevice *dev) { struct fsl_esdhc_plat *plat = dev_get_plat(dev);
return mmc_bind(dev, &plat->mmc, &plat->cfg); } -#endif
U_BOOT_DRIVER(fsl_esdhc) = { .name = "fsl_esdhc", @@ -1745,9 +1716,7 @@ U_BOOT_DRIVER(fsl_esdhc) = { .of_match = fsl_esdhc_ids, .of_to_plat = fsl_esdhc_of_to_plat, .ops = &fsl_esdhc_ops, -#if CONFIG_IS_ENABLED(BLK) .bind = fsl_esdhc_bind, -#endif .probe = fsl_esdhc_probe, .plat_auto = sizeof(struct fsl_esdhc_plat), .priv_auto = sizeof(struct fsl_esdhc_priv),
[ fsl_esdhc commit 2913926f3b3dec282f8773e3c02377c9600d8267 ]
Remove redundant DM_MMC checking which is already in DM_MMC conditional compile block.
Signed-off-by: Yangbo Lu yangbo.lu@nxp.com Signed-off-by: Sean Anderson sean.anderson@seco.com ---
drivers/mmc/fsl_esdhc_imx.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index 0a7f7e61cb..40886f37aa 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -1605,7 +1605,6 @@ static int fsl_esdhc_probe(struct udevice *dev) return esdhc_init_common(priv, mmc); }
-#if CONFIG_IS_ENABLED(DM_MMC) static int fsl_esdhc_get_cd(struct udevice *dev) { struct fsl_esdhc_priv *priv = dev_get_priv(dev); @@ -1671,7 +1670,6 @@ static const struct dm_mmc_ops fsl_esdhc_ops = { #endif .wait_dat0 = fsl_esdhc_wait_dat0, }; -#endif
static struct esdhc_soc_data usdhc_imx7d_data = { .flags = ESDHC_FLAG_USDHC | ESDHC_FLAG_STD_TUNING
[ fsl_esdhc commit 5b05fc0310cd933acf76ee661577c6b07a95e684 ]
Voltage validation should be done by CMD8. Current comparison between mmc_cfg voltages and host voltage capabilities is meaningless. So drop current comparison and let voltage validation is through CMD8.
Signed-off-by: Yangbo Lu yangbo.lu@nxp.com Signed-off-by: Sean Anderson sean.anderson@seco.com ---
drivers/mmc/fsl_esdhc_imx.c | 35 +++++++++++++---------------------- include/fsl_esdhc_imx.h | 12 ++++++------ 2 files changed, 19 insertions(+), 28 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index 40886f37aa..b91dda27f9 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -1164,7 +1164,7 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv, { struct mmc_config *cfg; struct fsl_esdhc *regs; - u32 caps, voltage_caps; + u32 caps; int ret;
if (!priv) @@ -1203,9 +1203,7 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv, memset(cfg, '\0', sizeof(*cfg)); #endif
- voltage_caps = 0; caps = esdhc_read32(®s->hostcapblt); - #ifdef CONFIG_MCF5441x /* * MCF5441x RM declares in more points that sdhc clock speed must @@ -1216,31 +1214,24 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv, #endif
#ifdef CONFIG_SYS_FSL_ERRATUM_ESDHC135 - caps = caps & ~(ESDHC_HOSTCAPBLT_SRS | - ESDHC_HOSTCAPBLT_VS18 | ESDHC_HOSTCAPBLT_VS30); + caps &= ~(HOSTCAPBLT_SRS | HOSTCAPBLT_VS18 | HOSTCAPBLT_VS30); #endif
- if (caps & ESDHC_HOSTCAPBLT_VS18) - voltage_caps |= MMC_VDD_165_195; - if (caps & ESDHC_HOSTCAPBLT_VS30) - voltage_caps |= MMC_VDD_29_30 | MMC_VDD_30_31; - if (caps & ESDHC_HOSTCAPBLT_VS33) - voltage_caps |= MMC_VDD_32_33 | MMC_VDD_33_34; +#ifdef CONFIG_SYS_FSL_MMC_HAS_CAPBLT_VS33 + caps |= HOSTCAPBLT_VS33; +#endif + + if (caps & HOSTCAPBLT_VS18) + cfg->voltages |= MMC_VDD_165_195; + if (caps & HOSTCAPBLT_VS30) + cfg->voltages |= MMC_VDD_29_30 | MMC_VDD_30_31; + if (caps & HOSTCAPBLT_VS33) + cfg->voltages |= MMC_VDD_32_33 | MMC_VDD_33_34;
cfg->name = "FSL_SDHC"; #if !CONFIG_IS_ENABLED(DM_MMC) cfg->ops = &esdhc_ops; #endif -#ifdef CONFIG_SYS_SD_VOLTAGE - cfg->voltages = CONFIG_SYS_SD_VOLTAGE; -#else - cfg->voltages = MMC_VDD_32_33 | MMC_VDD_33_34; -#endif - if ((cfg->voltages & voltage_caps) == 0) { - printf("voltage not supported by controller\n"); - return -1; - } - if (priv->bus_width == 8) cfg->host_caps = MMC_MODE_4BIT | MMC_MODE_8BIT; else if (priv->bus_width == 4) @@ -1258,7 +1249,7 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv, cfg->host_caps &= ~MMC_MODE_4BIT; }
- if (caps & ESDHC_HOSTCAPBLT_HSS) + if (caps & HOSTCAPBLT_HSS) cfg->host_caps |= MMC_MODE_HS_52MHz | MMC_MODE_HS;
#ifdef CONFIG_ESDHC_DETECT_8_BIT_QUIRK diff --git a/include/fsl_esdhc_imx.h b/include/fsl_esdhc_imx.h index 45ed635a77..1529b8bba3 100644 --- a/include/fsl_esdhc_imx.h +++ b/include/fsl_esdhc_imx.h @@ -164,12 +164,12 @@ #define BLKATTR_SIZE(x) (x & 0x1fff) #define MAX_BLK_CNT 0x7fff /* so malloc will have enough room with 32M */
-#define ESDHC_HOSTCAPBLT_VS18 0x04000000 -#define ESDHC_HOSTCAPBLT_VS30 0x02000000 -#define ESDHC_HOSTCAPBLT_VS33 0x01000000 -#define ESDHC_HOSTCAPBLT_SRS 0x00800000 -#define ESDHC_HOSTCAPBLT_DMAS 0x00400000 -#define ESDHC_HOSTCAPBLT_HSS 0x00200000 +#define HOSTCAPBLT_VS18 0x04000000 +#define HOSTCAPBLT_VS30 0x02000000 +#define HOSTCAPBLT_VS33 0x01000000 +#define HOSTCAPBLT_SRS 0x00800000 +#define HOSTCAPBLT_DMAS 0x00400000 +#define HOSTCAPBLT_HSS 0x00200000
#define ESDHC_VENDORSPEC_VSELECT 0x00000002 /* Use 1.8V */
[ 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"); }
+#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;
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
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 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
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;elsepriv->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;elsepriv->bus_width = 1; if (dtplat->non_removable) priv->non_removable = 1;-- 2.25.1
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;elsepriv->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;elsepriv->bus_width = 1; if (dtplat->non_removable) priv->non_removable = 1;-- 2.25.1
From: Yangbo Lu yangbo.lu@nxp.com
[ fsl_esdhc commit commit 08197cb8dff7cd097ab07a325093043c39d19bbd ]
Drop redundant code for non-removable feature. "non-removable" property has been read in mmc_of_parse().
Signed-off-by: Yangbo Lu yangbo.lu@nxp.com Signed-off-by: Sean Anderson sean.anderson@seco.com ---
drivers/mmc/fsl_esdhc_imx.c | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index d3daf4db59..a3cd9e5be1 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -130,7 +130,6 @@ struct esdhc_soc_data { * @mmc: mmc * Following is used when Driver Model is enabled for MMC * @dev: pointer for the device - * @non_removable: 0: removable; 1: non-removable * @broken_cd: 0: use GPIO for card detect; 1: Do not use GPIO for card detect * @wp_enable: 1: enable checking wp; 0: no check * @vs18_enable: 1: use 1.8V voltage; 0: use 3.3V @@ -154,7 +153,6 @@ struct fsl_esdhc_priv { struct mmc *mmc; #endif struct udevice *dev; - int non_removable; int broken_cd; int wp_enable; int vs18_enable; @@ -1083,9 +1081,6 @@ static int esdhc_getcd_common(struct fsl_esdhc_priv *priv) #endif
#if CONFIG_IS_ENABLED(DM_MMC) - if (priv->non_removable) - return 1; - if (priv->broken_cd) return 1; #if CONFIG_IS_ENABLED(DM_GPIO) @@ -1412,25 +1407,18 @@ static int fsl_esdhc_of_to_plat(struct udevice *dev) if (dev_read_bool(dev, "broken-cd")) priv->broken_cd = 1;
- if (dev_read_bool(dev, "non-removable")) { - priv->non_removable = 1; - } else { - priv->non_removable = 0; -#if CONFIG_IS_ENABLED(DM_GPIO) - gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, - GPIOD_IS_IN); -#endif - } - if (dev_read_prop(dev, "fsl,wp-controller", NULL)) { priv->wp_enable = 1; } else { priv->wp_enable = 0; + } + #if CONFIG_IS_ENABLED(DM_GPIO) - gpio_request_by_name(dev, "wp-gpios", 0, &priv->wp_gpio, - GPIOD_IS_IN); + gpio_request_by_name(dev, "cd-gpios", 0, &priv->cd_gpio, + GPIOD_IS_IN); + gpio_request_by_name(dev, "wp-gpios", 0, &priv->wp_gpio, + GPIOD_IS_IN); #endif - }
priv->vs18_enable = 0;
@@ -1564,8 +1552,12 @@ static int fsl_esdhc_probe(struct udevice *dev)
static int fsl_esdhc_get_cd(struct udevice *dev) { + struct fsl_esdhc_plat *plat = dev_get_plat(dev); struct fsl_esdhc_priv *priv = dev_get_priv(dev);
+ if (plat->cfg.host_caps & MMC_CAP_NONREMOVABLE) + return 1; + return esdhc_getcd_common(priv); }
[ fsl_esdhc commit 30f6444d024a74ee48aa6969c1531aecd3c59deb ]
Fix mmc->clock with actual clock which is divided by the controller, and record it with priv->clock.
Signed-off-by: Yangbo Lu yangbo.lu@nxp.com Signed-off-by: Sean Anderson sean.anderson@seco.com ---
drivers/mmc/fsl_esdhc_imx.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index a3cd9e5be1..dc71e39f8e 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -665,6 +665,7 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc *mmc, uint clock) esdhc_setbits32(®s->sysctl, SYSCTL_PEREN | SYSCTL_CKEN); #endif
+ mmc->clock = sdhc_clk / pre_div / div; priv->clock = clock; }
On 11/6/21 2:39 AM, Sean Anderson wrote:
[ fsl_esdhc commit 30f6444d024a74ee48aa6969c1531aecd3c59deb ]
Fix mmc->clock with actual clock which is divided by the controller, and record it with priv->clock.
Signed-off-by: Yangbo Lu yangbo.lu@nxp.com Signed-off-by: Sean Anderson sean.anderson@seco.com
drivers/mmc/fsl_esdhc_imx.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index a3cd9e5be1..dc71e39f8e 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -665,6 +665,7 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc *mmc, uint clock) esdhc_setbits32(®s->sysctl, SYSCTL_PEREN | SYSCTL_CKEN); #endif
- mmc->clock = sdhc_clk / pre_div / div;
Are there cases about "division by zero"?
Best Regards, Jaehoon Chung
priv->clock = clock; }
On 11/9/21 2:23 AM, Jaehoon Chung wrote:
On 11/6/21 2:39 AM, Sean Anderson wrote:
[ fsl_esdhc commit 30f6444d024a74ee48aa6969c1531aecd3c59deb ]
Fix mmc->clock with actual clock which is divided by the controller, and record it with priv->clock.
Signed-off-by: Yangbo Lu yangbo.lu@nxp.com Signed-off-by: Sean Anderson sean.anderson@seco.com
drivers/mmc/fsl_esdhc_imx.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index a3cd9e5be1..dc71e39f8e 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -665,6 +665,7 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc *mmc, uint clock) esdhc_setbits32(®s->sysctl, SYSCTL_PEREN | SYSCTL_CKEN); #endif
- mmc->clock = sdhc_clk / pre_div / div;
Are there cases about "division by zero"?
pre_div and div are only ever set to positive numbers or incremented.
--Sean
From: Michael Walle michael@walle.cc
[ fsl_esdhc commit da86e8cfcb03ed5c1d8e0718bc8bc8583e60ced8 ]
SDMA can only do DMA with 32 bit addresses. This is true for all architectures (just doesn't apply to 32 bit ones). Simplify the code and remove unnecessary CONFIG_FSL_LAYERSCAPE.
Also make the error message more concise.
Signed-off-by: Michael Walle michael@walle.cc Signed-off-by: Sean Anderson sean.anderson@seco.com ---
drivers/mmc/fsl_esdhc_imx.c | 33 ++++++--------------------------- 1 file changed, 6 insertions(+), 27 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index dc71e39f8e..7c070ed1c2 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -282,10 +282,7 @@ static int esdhc_setup_data(struct fsl_esdhc_priv *priv, struct mmc *mmc, { int timeout; struct fsl_esdhc *regs = priv->esdhc_regs; -#if defined(CONFIG_S32V234) || defined(CONFIG_IMX8) || defined(CONFIG_IMX8M) || \ - defined(CONFIG_IMX8ULP) dma_addr_t addr; -#endif uint wml_value;
wml_value = data->blocksize/4; @@ -296,16 +293,10 @@ static int esdhc_setup_data(struct fsl_esdhc_priv *priv, struct mmc *mmc,
esdhc_clrsetbits32(®s->wml, WML_RD_WML_MASK, wml_value); #ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO -#if defined(CONFIG_S32V234) || defined(CONFIG_IMX8) || defined(CONFIG_IMX8M) || \ - defined(CONFIG_IMX8ULP) addr = virt_to_phys((void *)(data->dest)); if (upper_32_bits(addr)) - printf("Error found for upper 32 bits\n"); - else - esdhc_write32(®s->dsaddr, lower_32_bits(addr)); -#else - esdhc_write32(®s->dsaddr, (u32)data->dest); -#endif + printf("Cannot use 64 bit addresses with SDMA\n"); + esdhc_write32(®s->dsaddr, lower_32_bits(addr)); #endif } else { #ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO @@ -334,16 +325,10 @@ static int esdhc_setup_data(struct fsl_esdhc_priv *priv, struct mmc *mmc, esdhc_clrsetbits32(®s->wml, WML_WR_WML_MASK, wml_value << 16); #ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO -#if defined(CONFIG_S32V234) || defined(CONFIG_IMX8) || defined(CONFIG_IMX8M) || \ - defined(CONFIG_IMX8ULP) addr = virt_to_phys((void *)(data->src)); if (upper_32_bits(addr)) - printf("Error found for upper 32 bits\n"); - else - esdhc_write32(®s->dsaddr, lower_32_bits(addr)); -#else - esdhc_write32(®s->dsaddr, (u32)data->src); -#endif + printf("Cannot use 64 bit addresses with SDMA\n"); + esdhc_write32(®s->dsaddr, lower_32_bits(addr)); #endif }
@@ -400,18 +385,12 @@ static void check_and_invalidate_dcache_range unsigned end = 0; unsigned size = roundup(ARCH_DMA_MINALIGN, data->blocks*data->blocksize); -#if defined(CONFIG_S32V234) || defined(CONFIG_IMX8) || defined(CONFIG_IMX8M) || \ - defined(CONFIG_IMX8ULP) dma_addr_t addr;
addr = virt_to_phys((void *)(data->dest)); if (upper_32_bits(addr)) - printf("Error found for upper 32 bits\n"); - else - start = lower_32_bits(addr); -#else - start = (unsigned)data->dest; -#endif + printf("Cannot use 64 bit addresses with SDMA\n"); + start = lower_32_bits(addr); end = start + size; invalidate_dcache_range(start, end); }
From: Michael Walle michael@walle.cc
[ fsl_esdhc commit b1ba1460a445bcc67972a617625d0349e4f22b31 ]
Use the dma_{map,unmap}_single() calls. These will take care of the flushing and invalidation of caches.
Signed-off-by: Michael Walle michael@walle.cc Signed-off-by: Sean Anderson sean.anderson@seco.com ---
drivers/mmc/fsl_esdhc_imx.c | 50 +++++++++++-------------------------- 1 file changed, 15 insertions(+), 35 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index 7c070ed1c2..3d65094f81 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -38,6 +38,7 @@ #include <mapmem.h> #include <dm/ofnode.h> #include <linux/iopoll.h> +#include <linux/dma-mapping.h>
#ifndef ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE #ifdef CONFIG_FSL_USDHC @@ -171,6 +172,7 @@ struct fsl_esdhc_priv { struct gpio_desc cd_gpio; struct gpio_desc wp_gpio; #endif + dma_addr_t dma_addr; };
/* Return the XFERTYP flags for a given command and data packet */ @@ -281,8 +283,8 @@ static int esdhc_setup_data(struct fsl_esdhc_priv *priv, struct mmc *mmc, struct mmc_data *data) { int timeout; + uint trans_bytes = data->blocksize * data->blocks; struct fsl_esdhc *regs = priv->esdhc_regs; - dma_addr_t addr; uint wml_value;
wml_value = data->blocksize/4; @@ -293,17 +295,13 @@ static int esdhc_setup_data(struct fsl_esdhc_priv *priv, struct mmc *mmc,
esdhc_clrsetbits32(®s->wml, WML_RD_WML_MASK, wml_value); #ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO - addr = virt_to_phys((void *)(data->dest)); - if (upper_32_bits(addr)) + priv->dma_addr = dma_map_single(data->dest, trans_bytes, + mmc_get_dma_dir(data)); + if (upper_32_bits(priv->dma_addr)) printf("Cannot use 64 bit addresses with SDMA\n"); - esdhc_write32(®s->dsaddr, lower_32_bits(addr)); + esdhc_write32(®s->dsaddr, lower_32_bits(priv->dma_addr)); #endif } else { -#ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO - flush_dcache_range((ulong)data->src, - (ulong)data->src+data->blocks - *data->blocksize); -#endif if (wml_value > WML_WR_WML_MAX) wml_value = WML_WR_WML_MAX_VAL; if (priv->wp_enable) { @@ -325,10 +323,11 @@ static int esdhc_setup_data(struct fsl_esdhc_priv *priv, struct mmc *mmc, esdhc_clrsetbits32(®s->wml, WML_WR_WML_MASK, wml_value << 16); #ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO - addr = virt_to_phys((void *)(data->src)); - if (upper_32_bits(addr)) + priv->dma_addr = dma_map_single((void *)data->src, trans_bytes, + mmc_get_dma_dir(data)); + if (upper_32_bits(priv->dma_addr)) printf("Cannot use 64 bit addresses with SDMA\n"); - esdhc_write32(®s->dsaddr, lower_32_bits(addr)); + esdhc_write32(®s->dsaddr, lower_32_bits(priv->dma_addr)); #endif }
@@ -378,23 +377,6 @@ static int esdhc_setup_data(struct fsl_esdhc_priv *priv, struct mmc *mmc, return 0; }
-static void check_and_invalidate_dcache_range - (struct mmc_cmd *cmd, - struct mmc_data *data) { - unsigned start = 0; - unsigned end = 0; - unsigned size = roundup(ARCH_DMA_MINALIGN, - data->blocks*data->blocksize); - dma_addr_t addr; - - addr = virt_to_phys((void *)(data->dest)); - if (upper_32_bits(addr)) - printf("Cannot use 64 bit addresses with SDMA\n"); - start = lower_32_bits(addr); - end = start + size; - invalidate_dcache_range(start, end); -} - #ifdef CONFIG_MCF5441x /* * Swaps 32-bit words to little-endian byte order. @@ -450,9 +432,6 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc, err = esdhc_setup_data(priv, mmc, data); if(err) return err; - - if (data->flags & MMC_DATA_READ) - check_and_invalidate_dcache_range(cmd, data); }
/* Figure out the transfer arguments */ @@ -560,12 +539,13 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc, * cache-fill during the DMA operations such as the * speculative pre-fetching etc. */ - if (data->flags & MMC_DATA_READ) { - check_and_invalidate_dcache_range(cmd, data); + dma_unmap_single(priv->dma_addr, + data->blocks * data->blocksize, + mmc_get_dma_dir(data)); #ifdef CONFIG_MCF5441x + if (data->flags & MMC_DATA_READ) sd_swap_dma_buff(data); #endif - } #endif }
From: Michael Walle michael@walle.cc
[ fsl_esdhc commit 7e48a028a42c111ba38a90b86e5f57dace980fa0 ]
First, we need the waterlevel setting for PIO mode only. Secondy, both DMA setup code is identical for both directions, except for the data pointer. Thus, unify them.
Signed-off-by: Michael Walle michael@walle.cc Signed-off-by: Sean Anderson sean.anderson@seco.com ---
drivers/mmc/fsl_esdhc_imx.c | 89 ++++++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 37 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index 3d65094f81..4c8d74a070 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -279,59 +279,74 @@ static void esdhc_pio_read_write(struct fsl_esdhc_priv *priv, } #endif
-static int esdhc_setup_data(struct fsl_esdhc_priv *priv, struct mmc *mmc, - struct mmc_data *data) +#ifdef CONFIG_SYS_FSL_ESDHC_USE_PIO +static void esdhc_setup_watermark_level(struct fsl_esdhc_priv *priv, + struct mmc_data *data) { - int timeout; - uint trans_bytes = data->blocksize * data->blocks; struct fsl_esdhc *regs = priv->esdhc_regs; - uint wml_value; - - wml_value = data->blocksize/4; + uint wml_value = data->blocksize / 4;
if (data->flags & MMC_DATA_READ) { if (wml_value > WML_RD_WML_MAX) wml_value = WML_RD_WML_MAX_VAL;
esdhc_clrsetbits32(®s->wml, WML_RD_WML_MASK, wml_value); -#ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO - priv->dma_addr = dma_map_single(data->dest, trans_bytes, - mmc_get_dma_dir(data)); - if (upper_32_bits(priv->dma_addr)) - printf("Cannot use 64 bit addresses with SDMA\n"); - esdhc_write32(®s->dsaddr, lower_32_bits(priv->dma_addr)); -#endif } else { if (wml_value > WML_WR_WML_MAX) wml_value = WML_WR_WML_MAX_VAL; - if (priv->wp_enable) { - if ((esdhc_read32(®s->prsstat) & - PRSSTAT_WPSPL) == 0) { - printf("\nThe SD card is locked. Can not write to a locked card.\n\n"); - return -ETIMEDOUT; - } - } else { -#if CONFIG_IS_ENABLED(DM_GPIO) - if (dm_gpio_is_valid(&priv->wp_gpio) && - dm_gpio_get_value(&priv->wp_gpio)) { - printf("\nThe SD card is locked. Can not write to a locked card.\n\n"); - return -ETIMEDOUT; - } -#endif - }
esdhc_clrsetbits32(®s->wml, WML_WR_WML_MASK, - wml_value << 16); -#ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO - priv->dma_addr = dma_map_single((void *)data->src, trans_bytes, - mmc_get_dma_dir(data)); - if (upper_32_bits(priv->dma_addr)) - printf("Cannot use 64 bit addresses with SDMA\n"); - esdhc_write32(®s->dsaddr, lower_32_bits(priv->dma_addr)); -#endif + wml_value << 16); } +} +#endif
+static void esdhc_setup_dma(struct fsl_esdhc_priv *priv, struct mmc_data *data) +{ + uint trans_bytes = data->blocksize * data->blocks; + struct fsl_esdhc *regs = priv->esdhc_regs; + void *buf; + + if (data->flags & MMC_DATA_WRITE) + buf = (void *)data->src; + else + buf = data->dest; + + priv->dma_addr = dma_map_single(buf, trans_bytes, + mmc_get_dma_dir(data)); + if (upper_32_bits(priv->dma_addr)) + printf("Cannot use 64 bit addresses with SDMA\n"); + esdhc_write32(®s->dsaddr, lower_32_bits(priv->dma_addr)); esdhc_write32(®s->blkattr, data->blocks << 16 | data->blocksize); +} + +static int esdhc_setup_data(struct fsl_esdhc_priv *priv, struct mmc *mmc, + struct mmc_data *data) +{ + int timeout; + bool is_write = data->flags & MMC_DATA_WRITE; + struct fsl_esdhc *regs = priv->esdhc_regs; + + if (is_write) { + if (priv->wp_enable && !(esdhc_read32(®s->prsstat) & PRSSTAT_WPSPL)) { + printf("Cannot write to locked SD card.\n"); + return -EINVAL; + } else { +#if CONFIG_IS_ENABLED(DM_GPIO) + if (dm_gpio_is_valid(&priv->wp_gpio) && + dm_gpio_get_value(&priv->wp_gpio)) { + printf("Cannot write to locked SD card.\n"); + return -EINVAL; + } +#endif + } + } + +#ifdef CONFIG_SYS_FSL_ESDHC_USE_PIO + esdhc_setup_watermark_level(priv, data); +#else + esdhc_setup_dma(priv, data); +#endif
/* Calculate the timeout period for data transactions */ /*
From: Michael Walle michael@walle.cc
[ fsl_esdhc commit 52faec31827ec1a1837977e29c067424426634c5 ]
Make the code cleaner and drop the old-style #ifdef constructs where it is possible.
Signed-off-by: Michael Walle michael@walle.cc Signed-off-by: Sean Anderson sean.anderson@seco.com ---
drivers/mmc/fsl_esdhc_imx.c | 209 +++++++++++++++++------------------- include/fsl_esdhc_imx.h | 2 - 2 files changed, 100 insertions(+), 111 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index 4c8d74a070..886a489777 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -182,15 +182,15 @@ static uint esdhc_xfertyp(struct mmc_cmd *cmd, struct mmc_data *data)
if (data) { xfertyp |= XFERTYP_DPSEL; -#ifndef CONFIG_SYS_FSL_ESDHC_USE_PIO - xfertyp |= XFERTYP_DMAEN; -#endif + if (!IS_ENABLED(CONFIG_SYS_FSL_ESDHC_USE_PIO) && + cmd->cmdidx != MMC_CMD_SEND_TUNING_BLOCK && + cmd->cmdidx != MMC_CMD_SEND_TUNING_BLOCK_HS200) + xfertyp |= XFERTYP_DMAEN; if (data->blocks > 1) { xfertyp |= XFERTYP_MSBSEL; xfertyp |= XFERTYP_BCEN; -#ifdef CONFIG_SYS_FSL_ERRATUM_ESDHC111 - xfertyp |= XFERTYP_AC12EN; -#endif + if (IS_ENABLED(CONFIG_SYS_FSL_ERRATUM_ESDHC111)) + xfertyp |= XFERTYP_AC12EN; }
if (data->flags & MMC_DATA_READ) @@ -214,7 +214,6 @@ static uint esdhc_xfertyp(struct mmc_cmd *cmd, struct mmc_data *data) return XFERTYP_CMD(cmd->cmdidx) | xfertyp; }
-#ifdef CONFIG_SYS_FSL_ESDHC_USE_PIO /* * PIO Read/Write Mode reduce the performace as DMA is not used in this mode. */ @@ -277,9 +276,7 @@ static void esdhc_pio_read_write(struct fsl_esdhc_priv *priv, } } } -#endif
-#ifdef CONFIG_SYS_FSL_ESDHC_USE_PIO static void esdhc_setup_watermark_level(struct fsl_esdhc_priv *priv, struct mmc_data *data) { @@ -299,7 +296,6 @@ static void esdhc_setup_watermark_level(struct fsl_esdhc_priv *priv, wml_value << 16); } } -#endif
static void esdhc_setup_dma(struct fsl_esdhc_priv *priv, struct mmc_data *data) { @@ -342,11 +338,10 @@ static int esdhc_setup_data(struct fsl_esdhc_priv *priv, struct mmc *mmc, } }
-#ifdef CONFIG_SYS_FSL_ESDHC_USE_PIO - esdhc_setup_watermark_level(priv, data); -#else - esdhc_setup_dma(priv, data); -#endif + if (IS_ENABLED(CONFIG_SYS_FSL_ESDHC_USE_PIO)) + esdhc_setup_watermark_level(priv, data); + else + esdhc_setup_dma(priv, data);
/* Calculate the timeout period for data transactions */ /* @@ -379,14 +374,13 @@ static int esdhc_setup_data(struct fsl_esdhc_priv *priv, struct mmc *mmc, if (timeout < 0) timeout = 0;
-#ifdef CONFIG_SYS_FSL_ERRATUM_ESDHC_A001 - if ((timeout == 4) || (timeout == 8) || (timeout == 12)) + if (IS_ENABLED(CONFIG_SYS_FSL_ERRATUM_ESDHC_A001) && + (timeout == 4 || timeout == 8 || timeout == 12)) timeout++; -#endif
-#ifdef ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE - timeout = 0xE; -#endif + if (IS_ENABLED(ESDHCI_QUIRK_BROKEN_TIMEOUT_VALUE)) + timeout = 0xE; + esdhc_clrsetbits32(®s->sysctl, SYSCTL_TIMEOUT_MASK, timeout << 16);
return 0; @@ -409,6 +403,11 @@ static inline void sd_swap_dma_buff(struct mmc_data *data) } } } +#else +static inline void sd_swap_dma_buff(struct mmc_data *data) +{ + return; +} #endif
/* @@ -425,10 +424,9 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc, struct fsl_esdhc *regs = priv->esdhc_regs; unsigned long start;
-#ifdef CONFIG_SYS_FSL_ERRATUM_ESDHC111 - if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION) + if (IS_ENABLED(CONFIG_SYS_FSL_ERRATUM_ESDHC111) && + cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION) return 0; -#endif
esdhc_write32(®s->irqstat, -1);
@@ -526,42 +524,40 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc,
/* Wait until all of the blocks are transferred */ if (data) { -#ifdef CONFIG_SYS_FSL_ESDHC_USE_PIO - esdhc_pio_read_write(priv, data); -#else - flags = DATA_COMPLETE; - if ((cmd->cmdidx == MMC_CMD_SEND_TUNING_BLOCK) || - (cmd->cmdidx == MMC_CMD_SEND_TUNING_BLOCK_HS200)) { - flags = IRQSTAT_BRR; + if (IS_ENABLED(CONFIG_SYS_FSL_ESDHC_USE_PIO)) { + esdhc_pio_read_write(priv, data); + } else { + flags = DATA_COMPLETE; + if (cmd->cmdidx == MMC_CMD_SEND_TUNING_BLOCK || + cmd->cmdidx == MMC_CMD_SEND_TUNING_BLOCK_HS200) + flags = IRQSTAT_BRR; + + do { + irqstat = esdhc_read32(®s->irqstat); + + if (irqstat & IRQSTAT_DTOE) { + err = -ETIMEDOUT; + goto out; + } + + if (irqstat & DATA_ERR) { + err = -ECOMM; + goto out; + } + } while ((irqstat & flags) != flags); + + /* + * Need invalidate the dcache here again to avoid any + * cache-fill during the DMA operations such as the + * speculative pre-fetching etc. + */ + dma_unmap_single(priv->dma_addr, + data->blocks * data->blocksize, + mmc_get_dma_dir(data)); + if (IS_ENABLED(CONFIG_MCF5441x) && + (data->flags & MMC_DATA_READ)) + sd_swap_dma_buff(data); } - - do { - irqstat = esdhc_read32(®s->irqstat); - - if (irqstat & IRQSTAT_DTOE) { - err = -ETIMEDOUT; - goto out; - } - - if (irqstat & DATA_ERR) { - err = -ECOMM; - goto out; - } - } while ((irqstat & flags) != flags); - - /* - * Need invalidate the dcache here again to avoid any - * cache-fill during the DMA operations such as the - * speculative pre-fetching etc. - */ - dma_unmap_single(priv->dma_addr, - data->blocks * data->blocksize, - mmc_get_dma_dir(data)); -#ifdef CONFIG_MCF5441x - if (data->flags & MMC_DATA_READ) - sd_swap_dma_buff(data); -#endif -#endif }
out: @@ -595,21 +591,22 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc *mmc, uint clock) struct fsl_esdhc *regs = priv->esdhc_regs; int div = 1; u32 tmp; - int ret; -#ifdef ARCH_MXC -#ifdef CONFIG_MX53 - /* For i.MX53 eSDHCv3, SYSCTL.SDCLKFS may not be set to 0. */ - int pre_div = (regs == (struct fsl_esdhc *)MMC_SDHC3_BASE_ADDR) ? 2 : 1; -#else - int pre_div = 1; -#endif -#else - int pre_div = 2; -#endif + int ret, pre_div; int ddr_pre_div = mmc->ddr_mode ? 2 : 1; int sdhc_clk = priv->sdhc_clk; uint clk;
+ if (IS_ENABLED(ARCH_MXC)) { +#ifdef CONFIG_MX53 + /* For i.MX53 eSDHCv3, SYSCTL.SDCLKFS may not be set to 0. */ + pre_div = (regs == (struct fsl_esdhc *)MMC_SDHC3_BASE_ADDR) ? 2 : 1; +#else + pre_div = 1; +#endif + } else { + pre_div = 2; + } + while (sdhc_clk / (16 * pre_div * ddr_pre_div) > clock && pre_div < 256) pre_div *= 2;
@@ -621,11 +618,10 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc *mmc, uint clock)
clk = (pre_div << 8) | (div << 4);
-#ifdef CONFIG_FSL_USDHC - esdhc_clrbits32(®s->vendorspec, VENDORSPEC_CKEN); -#else - esdhc_clrbits32(®s->sysctl, SYSCTL_CKEN); -#endif + if (IS_ENABLED(CONFIG_FSL_USDHC)) + esdhc_clrbits32(®s->vendorspec, VENDORSPEC_CKEN); + else + esdhc_clrbits32(®s->sysctl, SYSCTL_CKEN);
esdhc_clrsetbits32(®s->sysctl, SYSCTL_CLOCK_MASK, clk);
@@ -633,11 +629,10 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc *mmc, uint clock) if (ret) pr_warn("fsl_esdhc_imx: Internal clock never stabilised.\n");
-#ifdef CONFIG_FSL_USDHC - esdhc_setbits32(®s->vendorspec, VENDORSPEC_PEREN | VENDORSPEC_CKEN); -#else - esdhc_setbits32(®s->sysctl, SYSCTL_PEREN | SYSCTL_CKEN); -#endif + if (IS_ENABLED(CONFIG_FSL_USDHC)) + esdhc_setbits32(®s->vendorspec, VENDORSPEC_PEREN | VENDORSPEC_CKEN); + else + esdhc_setbits32(®s->sysctl, SYSCTL_PEREN | SYSCTL_CKEN);
mmc->clock = sdhc_clk / pre_div / div; priv->clock = clock; @@ -1145,22 +1140,21 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv, if (ret) return ret;
-#ifdef CONFIG_MCF5441x /* ColdFire, using SDHC_DATA[3] for card detection */ - esdhc_write32(®s->proctl, PROCTL_INIT | PROCTL_D3CD); -#endif + if (IS_ENABLED(CONFIG_MCF5441x)) + esdhc_write32(®s->proctl, PROCTL_INIT | PROCTL_D3CD);
-#ifndef CONFIG_FSL_USDHC - esdhc_setbits32(®s->sysctl, SYSCTL_PEREN | SYSCTL_HCKEN - | SYSCTL_IPGEN | SYSCTL_CKEN); - /* Clearing tuning bits in case ROM has set it already */ - esdhc_write32(®s->mixctrl, 0); - esdhc_write32(®s->autoc12err, 0); - esdhc_write32(®s->clktunectrlstatus, 0); -#else - esdhc_setbits32(®s->vendorspec, VENDORSPEC_PEREN | - VENDORSPEC_HCKEN | VENDORSPEC_IPGEN | VENDORSPEC_CKEN); -#endif + if (IS_ENABLED(CONFIG_FSL_USDHC)) { + esdhc_setbits32(®s->vendorspec, VENDORSPEC_PEREN | + VENDORSPEC_HCKEN | VENDORSPEC_IPGEN | VENDORSPEC_CKEN); + } else { + esdhc_setbits32(®s->sysctl, SYSCTL_PEREN | SYSCTL_HCKEN + | SYSCTL_IPGEN | SYSCTL_CKEN); + /* Clearing tuning bits in case ROM has set it already */ + esdhc_write32(®s->mixctrl, 0); + esdhc_write32(®s->autoc12err, 0); + esdhc_write32(®s->clktunectrlstatus, 0); + }
if (priv->vs18_enable) esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); @@ -1172,22 +1166,20 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv, #endif
caps = esdhc_read32(®s->hostcapblt); -#ifdef CONFIG_MCF5441x + /* * MCF5441x RM declares in more points that sdhc clock speed must * never exceed 25 Mhz. From this, the HS bit needs to be disabled * from host capabilities. */ - caps &= ~ESDHC_HOSTCAPBLT_HSS; -#endif + if (IS_ENABLED(CONFIG_MCF5441x)) + caps &= ~HOSTCAPBLT_HSS;
-#ifdef CONFIG_SYS_FSL_ERRATUM_ESDHC135 - caps &= ~(HOSTCAPBLT_SRS | HOSTCAPBLT_VS18 | HOSTCAPBLT_VS30); -#endif + if (IS_ENABLED(CONFIG_SYS_FSL_ERRATUM_ESDHC135)) + caps &= ~(HOSTCAPBLT_SRS | HOSTCAPBLT_VS18 | HOSTCAPBLT_VS30);
-#ifdef CONFIG_SYS_FSL_MMC_HAS_CAPBLT_VS33 - caps |= HOSTCAPBLT_VS33; -#endif + if (IS_ENABLED(CONFIG_SYS_FSL_MMC_HAS_CAPBLT_VS33)) + caps |= HOSTCAPBLT_VS33;
if (caps & HOSTCAPBLT_VS18) cfg->voltages |= MMC_VDD_165_195; @@ -1197,12 +1189,13 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv, cfg->voltages |= MMC_VDD_32_33 | MMC_VDD_33_34;
cfg->name = "FSL_SDHC"; + #if !CONFIG_IS_ENABLED(DM_MMC) cfg->ops = &esdhc_ops; #endif -#ifdef CONFIG_SYS_FSL_ESDHC_HAS_DDR_MODE - cfg->host_caps |= MMC_MODE_DDR_52MHz; -#endif + + if (IS_ENABLED(CONFIG_SYS_FSL_ESDHC_HAS_DDR_MODE)) + cfg->host_caps |= MMC_MODE_DDR_52MHz;
if (caps & HOSTCAPBLT_HSS) cfg->host_caps |= MMC_MODE_HS_52MHz | MMC_MODE_HS; @@ -1283,10 +1276,8 @@ int fsl_esdhc_initialize(struct bd_info *bis, struct fsl_esdhc_cfg *cfg) 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) + if (IS_ENABLED(CONFIG_ESDHC_DETECT_8_BIT_QUIRK)) mmc_cfg->host_caps &= ~MMC_MODE_8BIT; -#endif
ret = fsl_esdhc_init(priv, plat); if (ret) { diff --git a/include/fsl_esdhc_imx.h b/include/fsl_esdhc_imx.h index 1529b8bba3..b958a6b3bb 100644 --- a/include/fsl_esdhc_imx.h +++ b/include/fsl_esdhc_imx.h @@ -24,12 +24,10 @@ #define SYSCTL_INITA 0x08000000 #define SYSCTL_TIMEOUT_MASK 0x000f0000 #define SYSCTL_CLOCK_MASK 0x0000fff0 -#if !defined(CONFIG_FSL_USDHC) #define SYSCTL_CKEN 0x00000008 #define SYSCTL_PEREN 0x00000004 #define SYSCTL_HCKEN 0x00000002 #define SYSCTL_IPGEN 0x00000001 -#endif #define SYSCTL_RSTA 0x01000000 #define SYSCTL_RSTC 0x02000000 #define SYSCTL_RSTD 0x04000000
From: Yangbo Lu yangbo.lu@nxp.com
[ fsl_esdhc commit 263ddfc3454ead3a988adef39b962479adce2b28 ]
The initial clock setting should be through sysctl register only, while the mmc_set_clock() will call mmc_set_ios() introduce other configurations like bus width, mode, and so on.
Signed-off-by: Yangbo Lu yangbo.lu@nxp.com Reviewed-by: Jaehoon Chung jh80.chung@samsung.com Signed-off-by: Sean Anderson sean.anderson@seco.com ---
drivers/mmc/fsl_esdhc_imx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index 886a489777..fc92ac3826 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -1022,7 +1022,7 @@ static int esdhc_init_common(struct fsl_esdhc_priv *priv, struct mmc *mmc) #endif
/* Set the initial clock speed */ - mmc_set_clock(mmc, 400000, MMC_CLK_ENABLE); + set_sysctl(priv, mmc, 400000);
/* Disable the BRR and BWR bits in IRQSTAT */ esdhc_clrbits32(®s->irqstaten, IRQSTATEN_BRR | IRQSTATEN_BWR);
Am 2021-11-05 18:39, schrieb Sean Anderson:
This series ports some of the patches from fsl_esdhc to fsl_esdhc_imx. Because these drivers share a common lineage, many of these patches apply with minor changes. For each one, I have noted the originating commit in the style of linux stable backports.
In fa33d20749 ("mmc: split fsl_esdhc driver for i.MX"), Yangbo says
For the two series processors, the eSDHCs are becoming more and more different
However, these drivers are still extremely similar; the differences between them are not major. However, NXP has not done a good job of porting patches which apply to both drivers. This causes the fsl_esdhc_imx driver to rot, as the fsl_esdhc gets more general fixes. For this reason, I think that the fsl_esdhc_imx driver should be removed unless NXP can commit to creating series like this which port patches which apply to both drivers.
But you are still doing patches for it? ;) Wouldn't it be easier to just merge them again? Funny enough, I ported some changes from the imx version to the non-imx version.
So yes I agree, this situation isn't optimal. And I don't think it makes any sense to port patches between these two. IMHO nobody will care for both at the same time, let alone being able to test it.
-michael
On 11/5/21 7:32 PM, Michael Walle wrote:
Am 2021-11-05 18:39, schrieb Sean Anderson:
This series ports some of the patches from fsl_esdhc to fsl_esdhc_imx. Because these drivers share a common lineage, many of these patches apply with minor changes. For each one, I have noted the originating commit in the style of linux stable backports.
In fa33d20749 ("mmc: split fsl_esdhc driver for i.MX"), Yangbo says
For the two series processors, the eSDHCs are becoming more and more different
However, these drivers are still extremely similar; the differences between them are not major. However, NXP has not done a good job of porting patches which apply to both drivers. This causes the fsl_esdhc_imx driver to rot, as the fsl_esdhc gets more general fixes. For this reason, I think that the fsl_esdhc_imx driver should be removed unless NXP can commit to creating series like this which port patches which apply to both drivers.
But you are still doing patches for it? ;) Wouldn't it be easier to just merge them again?
Well, I actually did all the work for these patches back in April but never got around to posting them. So it's much easier for me to post these than to merge the drivers :)
Unfortunately, there *are* feature differences that make it non-trivial to un-split these drivers.
Funny enough, I ported some changes from the imx version to the non-imx version.
Yeah, this is the other half of the coin; now we have to have two versions of many patches.
So yes I agree, this situation isn't optimal. And I don't think it makes any sense to port patches between these two. IMHO nobody will care for both at the same time, let alone being able to test it.
Yeah. There's a bit of an implicit promise of that from NXP, but they haven't followed through.
--Sean
participants (5)
-
Adam Ford -
Jaehoon Chung -
Michael Walle -
Sean Anderson -
Sean Anderson