[PATCH 1/2] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output
From: Haibo Chen haibo.chen@nxp.com
For FSL_USDHC, it do not implement VENDORSPEC_CKEN/PEREN/HCKEN/IPGEN, these are reserved bits. Instead, use VENDORSPEC_FRC_SDCLK_ON to gate on/off the card clock output.
After commit b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0() support"), we meet SD3.0 card can't work at UHS mode, mmc_switch_voltage() fail because the second mmc_wait_dat0 return -ETIMEDOUT. According to SD spec, during voltage switch, need to gate off/on the card clock. If not set the FRC_SDCLK_ON, after CMD11, hardware will gate off the card clock automatically, so card do not detect the clock off/on behavior, so will draw the data0 line low until next command.
Fixes: b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0() support") Tested-by: Tim Harvey tharvey@gateworks.com Signed-off-by: Haibo Chen haibo.chen@nxp.com --- drivers/mmc/fsl_esdhc_imx.c | 29 +++++++++++++++++++++-------- include/fsl_esdhc_imx.h | 2 ++ 2 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index e0e132698e..af36558b3c 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -654,7 +654,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); + esdhc_clrbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON); + ret = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp, tmp & PRSSTAT_SDOFF, 100); + if (ret) + pr_warn("fsl_esdhc_imx: Internal clock never gate off.\n"); #else esdhc_clrbits32(®s->sysctl, SYSCTL_CKEN); #endif @@ -666,7 +669,7 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc *mmc, uint clock) pr_warn("fsl_esdhc_imx: Internal clock never stabilised.\n");
#ifdef CONFIG_FSL_USDHC - esdhc_setbits32(®s->vendorspec, VENDORSPEC_PEREN | VENDORSPEC_CKEN); + esdhc_setbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON); #else esdhc_setbits32(®s->sysctl, SYSCTL_PEREN | SYSCTL_CKEN); #endif @@ -721,8 +724,14 @@ static void esdhc_set_strobe_dll(struct mmc *mmc) struct fsl_esdhc_priv *priv = dev_get_priv(mmc->dev); struct fsl_esdhc *regs = priv->esdhc_regs; u32 val; + u32 tmp; + int ret;
if (priv->clock > ESDHC_STROBE_DLL_CLK_FREQ) { + esdhc_clrbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON); + ret = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp, tmp & PRSSTAT_SDOFF, 100); + if (ret) + pr_warn("fsl_esdhc_imx: Internal clock never gate off.\n"); esdhc_write32(®s->strobe_dllctrl, ESDHC_STROBE_DLL_CTRL_RESET);
/* @@ -740,6 +749,7 @@ static void esdhc_set_strobe_dll(struct mmc *mmc) pr_warn("HS400 strobe DLL status REF not lock!\n"); if (!(val & ESDHC_STROBE_DLL_STS_SLV_LOCK)) pr_warn("HS400 strobe DLL status SLV not lock!\n"); + esdhc_setbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON); } }
@@ -963,14 +973,18 @@ static int esdhc_set_ios_common(struct fsl_esdhc_priv *priv, struct mmc *mmc) #ifdef MMC_SUPPORTS_TUNING if (mmc->clk_disable) { #ifdef CONFIG_FSL_USDHC - esdhc_clrbits32(®s->vendorspec, VENDORSPEC_CKEN); + u32 tmp; + + esdhc_clrbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON); + ret = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp, tmp & PRSSTAT_SDOFF, 100); + if (ret) + pr_warn("fsl_esdhc_imx: Internal clock never gate off.\n"); #else esdhc_clrbits32(®s->sysctl, SYSCTL_CKEN); #endif } else { #ifdef CONFIG_FSL_USDHC - esdhc_setbits32(®s->vendorspec, VENDORSPEC_PEREN | - VENDORSPEC_CKEN); + esdhc_setbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON); #else esdhc_setbits32(®s->sysctl, SYSCTL_PEREN | SYSCTL_CKEN); #endif @@ -1046,7 +1060,7 @@ static int esdhc_init_common(struct fsl_esdhc_priv *priv, struct mmc *mmc) #ifndef CONFIG_FSL_USDHC esdhc_setbits32(®s->sysctl, SYSCTL_HCKEN | SYSCTL_IPGEN); #else - esdhc_setbits32(®s->vendorspec, VENDORSPEC_HCKEN | VENDORSPEC_IPGEN); + esdhc_setbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON); #endif
/* Set the initial clock speed */ @@ -1184,8 +1198,7 @@ static int fsl_esdhc_init(struct fsl_esdhc_priv *priv, esdhc_write32(®s->autoc12err, 0); esdhc_write32(®s->clktunectrlstatus, 0); #else - esdhc_setbits32(®s->vendorspec, VENDORSPEC_PEREN | - VENDORSPEC_HCKEN | VENDORSPEC_IPGEN | VENDORSPEC_CKEN); + esdhc_setbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON); #endif
if (priv->vs18_enable) diff --git a/include/fsl_esdhc_imx.h b/include/fsl_esdhc_imx.h index 45ed635a77..b092034464 100644 --- a/include/fsl_esdhc_imx.h +++ b/include/fsl_esdhc_imx.h @@ -39,6 +39,7 @@ #define VENDORSPEC_HCKEN 0x00001000 #define VENDORSPEC_IPGEN 0x00000800 #define VENDORSPEC_INIT 0x20007809 +#define VENDORSPEC_FRC_SDCLK_ON 0x00000100
#define IRQSTAT 0x0002e030 #define IRQSTAT_DMAE (0x10000000) @@ -96,6 +97,7 @@ #define PRSSTAT_CINS (0x00010000) #define PRSSTAT_BREN (0x00000800) #define PRSSTAT_BWEN (0x00000400) +#define PRSSTAT_SDOFF (0x00000080) #define PRSSTAT_SDSTB (0X00000008) #define PRSSTAT_DLA (0x00000004) #define PRSSTAT_CICHB (0x00000002)
From: Haibo Chen haibo.chen@nxp.com
Common code already handle the voltage switch sequence based on spec, so remove the redundant voltage switch code.
Signed-off-by: Haibo Chen haibo.chen@nxp.com --- drivers/mmc/fsl_esdhc_imx.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index af36558b3c..a199cf3df6 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc, goto out; }
- /* Switch voltage to 1.8V if CMD11 succeeded */ - if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) { - esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); - - printf("Run CMD11 1.8V switch\n"); - /* Sleep for 5 ms - max time for card to switch to 1.8V */ - udelay(5000); - } - /* Workaround for ESDHC errata ENGcm03648 */ if (!data && (cmd->resp_type & MMC_RSP_BUSY)) { int timeout = 50000; @@ -839,6 +830,7 @@ static int esdhc_set_voltage(struct mmc *mmc) } #endif esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); + mdelay(5); if (esdhc_read32(®s->vendorspec) & ESDHC_VENDORSPEC_VSELECT) return 0;
On Wed, Mar 3, 2021 at 3:18 AM haibo.chen@nxp.com wrote:
From: Haibo Chen haibo.chen@nxp.com
Common code already handle the voltage switch sequence based on spec, so remove the redundant voltage switch code.
Nice! Thank you. This lets my eMMC run at HS400ES and the microSD card run as SDR104.
For the series,
Tested-by: Adam Ford aford173@gmail.com #imx8mn_beacon
adam
Signed-off-by: Haibo Chen haibo.chen@nxp.com
drivers/mmc/fsl_esdhc_imx.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index af36558b3c..a199cf3df6 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc, goto out; }
/* Switch voltage to 1.8V if CMD11 succeeded */if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT);printf("Run CMD11 1.8V switch\n");/* Sleep for 5 ms - max time for card to switch to 1.8V */udelay(5000);}/* Workaround for ESDHC errata ENGcm03648 */ if (!data && (cmd->resp_type & MMC_RSP_BUSY)) { int timeout = 50000;@@ -839,6 +830,7 @@ static int esdhc_set_voltage(struct mmc *mmc) } #endif esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT);
mdelay(5); if (esdhc_read32(®s->vendorspec) & ESDHC_VENDORSPEC_VSELECT) return 0;-- 2.17.1
Hello Haibo,
-----Original Message----- From: haibo.chen@nxp.com haibo.chen@nxp.com Sent: Wednesday, March 3, 2021 10:06 AM To: peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: haibo.chen@nxp.com; uboot-imx@nxp.com; tharvey@gateworks.com; ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com; festevam@gmail.com; ye.li@nxp.com Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
From: Haibo Chen haibo.chen@nxp.com
Common code already handle the voltage switch sequence based on spec, so remove the redundant voltage switch code.
Signed-off-by: Haibo Chen haibo.chen@nxp.com
drivers/mmc/fsl_esdhc_imx.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index af36558b3c..a199cf3df6 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc, goto out; }
/* Switch voltage to 1.8V if CMD11 succeeded */if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT);printf("Run CMD11 1.8V switch\n");/* Sleep for 5 ms - max time for card to switch to 1.8V */udelay(5000);}/* Workaround for ESDHC errata ENGcm03648 */ if (!data && (cmd->resp_type & MMC_RSP_BUSY)) { int timeout = 50000;@@ -839,6 +830,7 @@ static int esdhc_set_voltage(struct mmc *mmc) } #endif esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT);
mdelay(5); if (esdhc_read32(®s->vendorspec) & ESDHC_VENDORSPEC_VSELECT) return 0;-- 2.17.1
Just tested the whole series on i.MX8M Mini EVK, boot log with MMC info: --------------- U-Boot SPL 2021.04-rc3-00009-g1333570cee (Mar 03 2021 - 11:34:24 +0100) Normal Boot WDT: Started with servicing (60s timeout) Trying to boot from MMC1 NOTICE: BL31: v2.2(release):rel_imx_5.4.70_2.3.0-0-gf1d7187f2 NOTICE: BL31: Built : 22:29:05, Jan 17 2021
U-Boot 2021.04-rc3-00009-g1333570cee (Mar 03 2021 - 11:34:24 +0100)
CPU: Freescale i.MX8MMQ rev1.0 at 1200 MHz Reset cause: POR Model: FSL i.MX8MM EVK board DRAM: 2 GiB WDT: Started with servicing (60s timeout) MMC: FSL_SDHC: 1, FSL_SDHC: 2 Loading Environment from MMC... *** Warning - bad CRC, using default environment
In: serial Out: serial Err: serial Net: eth0: ethernet@30be0000 Hit any key to stop autoboot: 0 u-boot=> mmc dev 1 switch to partitions #0, OK mmc1 is current device u-boot=> mmc info Device: FSL_SDHC Manufacturer ID: 41 OEM: 3432 Name: SD32G Bus Speed: 200000000 Mode: UHS SDR104 (208MHz) Rd Block Len: 512 SD version 3.0 High Capacity: Yes Capacity: 29.3 GiB Bus Width: 4-bit Erase Group Size: 512 Bytes u-boot=> mmc dev 2 switch to partitions #0, OK mmc2(part 0) is current device u-boot=> mmc info Device: FSL_SDHC Manufacturer ID: 45 OEM: 100 Name: DG401 Bus Speed: 200000000 Mode: HS400ES (200MHz) Rd Block Len: 512 MMC version 5.1 High Capacity: Yes Capacity: 14.7 GiB Bus Width: 8-bit DDR Erase Group Size: 512 KiB HC WP Group Size: 8 MiB User Capacity: 14.7 GiB WRREL Boot Capacity: 4 MiB ENH RPMB Capacity: 4 MiB ENH Boot area 0 is not write protected Boot area 1 is not write protected u-boot=> ------------
For the series: Tested-by: Andrey Zhizhikin andrey.zhizhikin@leica-geosystems.com # imx8mm_evk
-- andrey
-----Original Message----- From: haibo.chen@nxp.com haibo.chen@nxp.com Sent: Wednesday, March 3, 2021 10:06 AM To: peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: haibo.chen@nxp.com; uboot-imx@nxp.com; tharvey@gateworks.com; ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com; festevam@gmail.com; ye.li@nxp.com Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
From: Haibo Chen haibo.chen@nxp.com
Common code already handle the voltage switch sequence based on spec, so remove the redundant voltage switch code.
Signed-off-by: Haibo Chen haibo.chen@nxp.com
drivers/mmc/fsl_esdhc_imx.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index af36558b3c..a199cf3df6 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc, goto out; }
/* Switch voltage to 1.8V if CMD11 succeeded */if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT);printf("Run CMD11 1.8V switch\n");/* Sleep for 5 ms - max time for card to switch to 1.8V */udelay(5000);}/* Workaround for ESDHC errata ENGcm03648 */ if (!data && (cmd->resp_type & MMC_RSP_BUSY)) { int timeout = 50000;@@ -839,6 +830,7 @@ static int esdhc_set_voltage(struct mmc *mmc) } #endif esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT);
mdelay(5);
Why is this delay introduced here? It is not clear from the commit message whether and why it is required here.
If this is kept from the removed block - maybe it is better to move the corresponding comment here as well.
if (esdhc_read32(®s->vendorspec) & ESDHC_VENDORSPEC_VSELECT) return 0;-- 2.17.1
-- andrey
-----Original Message----- From: ZHIZHIKIN Andrey [mailto:andrey.zhizhikin@leica-geosystems.com] Sent: 2021年3月3日 19:00 To: Bough Chen haibo.chen@nxp.com; Peng Fan peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: dl-uboot-imx uboot-imx@nxp.com; tharvey@gateworks.com; festevam@gmail.com; Ye Li ye.li@nxp.com Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
-----Original Message----- From: haibo.chen@nxp.com haibo.chen@nxp.com Sent: Wednesday, March 3, 2021 10:06 AM To: peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: haibo.chen@nxp.com; uboot-imx@nxp.com; tharvey@gateworks.com; ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com; festevam@gmail.com; ye.li@nxp.com Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
From: Haibo Chen haibo.chen@nxp.com
Common code already handle the voltage switch sequence based on spec, so remove the redundant voltage switch code.
Signed-off-by: Haibo Chen haibo.chen@nxp.com
drivers/mmc/fsl_esdhc_imx.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index af36558b3c..a199cf3df6 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc, goto out; }
/* Switch voltage to 1.8V if CMD11 succeeded */if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {esdhc_setbits32(®s->vendorspec,ESDHC_VENDORSPEC_VSELECT);
printf("Run CMD11 1.8V switch\n");/* Sleep for 5 ms - max time for card to switch to 1.8V */udelay(5000);}/* Workaround for ESDHC errata ENGcm03648 */ if (!data && (cmd->resp_type & MMC_RSP_BUSY)) { int timeout = 50000;@@ -839,6 +830,7 @@ static int esdhc_set_voltage(struct mmc *mmc) } #endif esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT);
mdelay(5);Why is this delay introduced here? It is not clear from the commit message whether and why it is required here.
If this is kept from the removed block - maybe it is better to move the corresponding comment here as well.
Hi Andrev,
Thanks for your careful review! Without this 5ms delay, with patch1 and remove the upper redundant cmd11 related code, mmc_switch_voltage() will fail, due to the second mmc_wait_dat0() return timeout. Seems for usdhc, gate off clock for 10ms is not enough. If total delay 15ms, then the second mmc_wait_dat0() can return normal. So I add 5ms delay here. Yes, I should add a comment for this 5ms in the code. You can also do the test on your side.
Best Regards Haibo
if (esdhc_read32(®s->vendorspec) &ESDHC_VENDORSPEC_VSELECT)
return 0;-- 2.17.1
-- andrey
Hello Haibo,
-----Original Message----- From: Bough Chen haibo.chen@nxp.com Sent: Wednesday, March 3, 2021 12:27 PM To: ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com; Peng Fan peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: dl-uboot-imx uboot-imx@nxp.com; tharvey@gateworks.com; festevam@gmail.com; Ye Li ye.li@nxp.com Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
-----Original Message----- From: ZHIZHIKIN Andrey [mailto:andrey.zhizhikin@leica-geosystems.com] Sent: 2021年3月3日 19:00 To: Bough Chen haibo.chen@nxp.com; Peng Fan peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: dl-uboot-imx uboot-imx@nxp.com; tharvey@gateworks.com; festevam@gmail.com; Ye Li ye.li@nxp.com Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
-----Original Message----- From: haibo.chen@nxp.com haibo.chen@nxp.com Sent: Wednesday, March 3, 2021 10:06 AM To: peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: haibo.chen@nxp.com; uboot-imx@nxp.com; tharvey@gateworks.com; ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com; festevam@gmail.com; ye.li@nxp.com Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
From: Haibo Chen haibo.chen@nxp.com
Common code already handle the voltage switch sequence based on spec, so remove the redundant voltage switch code.
Signed-off-by: Haibo Chen haibo.chen@nxp.com
drivers/mmc/fsl_esdhc_imx.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index af36558b3c..a199cf3df6 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc, goto out; }
/* Switch voltage to 1.8V if CMD11 succeeded */if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {esdhc_setbits32(®s->vendorspec,ESDHC_VENDORSPEC_VSELECT);
printf("Run CMD11 1.8V switch\n");/* Sleep for 5 ms - max time for card to switch to 1.8V */udelay(5000);}/* Workaround for ESDHC errata ENGcm03648 */ if (!data && (cmd->resp_type & MMC_RSP_BUSY)) { int timeout = 50000;@@ -839,6 +830,7 @@ static int esdhc_set_voltage(struct mmc *mmc) } #endif esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT);
mdelay(5);Why is this delay introduced here? It is not clear from the commit message whether and why it is required here.
If this is kept from the removed block - maybe it is better to move the corresponding comment here as well.
Hi Andrev,
Thanks for your careful review!
Thanks for the patch series on the first place! This allows to switch uSDHC into high-speed modes, which is quite valuable.
Without this 5ms delay, with patch1 and remove the upper redundant cmd11 related code, mmc_switch_voltage() will fail, due to the second mmc_wait_dat0() return timeout. Seems for usdhc, gate off clock for 10ms is not enough. If total delay 15ms, then the second mmc_wait_dat0() can return normal. So I add 5ms delay here. Yes, I should add a comment for this 5ms in the code.
Exactly this information is missing with the patch, as later on it would be quite difficult to grasp on why this mdelay() was added on the first place.
You can also do the test on your side.
I've already did and reported with the boot log, mmc info, and my Tested-by: tag.
Best Regards Haibo
if (esdhc_read32(®s->vendorspec) &ESDHC_VENDORSPEC_VSELECT)
return 0;-- 2.17.1
-- andrey
-- andrey
-----Original Message----- From: ZHIZHIKIN Andrey [mailto:andrey.zhizhikin@leica-geosystems.com] Sent: 2021年3月3日 20:50 To: Bough Chen haibo.chen@nxp.com; Peng Fan peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: dl-uboot-imx uboot-imx@nxp.com; tharvey@gateworks.com; festevam@gmail.com; Ye Li ye.li@nxp.com Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
Hello Haibo,
-----Original Message----- From: Bough Chen haibo.chen@nxp.com Sent: Wednesday, March 3, 2021 12:27 PM To: ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com; Peng Fan peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: dl-uboot-imx uboot-imx@nxp.com; tharvey@gateworks.com; festevam@gmail.com; Ye Li ye.li@nxp.com Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
-----Original Message----- From: ZHIZHIKIN Andrey [mailto:andrey.zhizhikin@leica-geosystems.com] Sent: 2021年3月3日 19:00 To: Bough Chen haibo.chen@nxp.com; Peng Fan peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: dl-uboot-imx uboot-imx@nxp.com; tharvey@gateworks.com; festevam@gmail.com; Ye Li ye.li@nxp.com Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
-----Original Message----- From: haibo.chen@nxp.com haibo.chen@nxp.com Sent: Wednesday, March 3, 2021 10:06 AM To: peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: haibo.chen@nxp.com; uboot-imx@nxp.com;
tharvey@gateworks.com;
ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com; festevam@gmail.com; ye.li@nxp.com Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
From: Haibo Chen haibo.chen@nxp.com
Common code already handle the voltage switch sequence based on spec, so remove the redundant voltage switch code.
Signed-off-by: Haibo Chen haibo.chen@nxp.com
drivers/mmc/fsl_esdhc_imx.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index af36558b3c..a199cf3df6 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc, goto out; }
/* Switch voltage to 1.8V if CMD11 succeeded */if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {esdhc_setbits32(®s->vendorspec,ESDHC_VENDORSPEC_VSELECT);
printf("Run CMD11 1.8V switch\n");/* Sleep for 5 ms - max time for card to switch to 1.8V*/
udelay(5000);}/* Workaround for ESDHC errata ENGcm03648 */ if (!data && (cmd->resp_type & MMC_RSP_BUSY)) { int timeout = 50000; @@ -839,6 +830,7 @@ staticint esdhc_set_voltage(struct mmc *mmc) } #endif esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT);
mdelay(5);Why is this delay introduced here? It is not clear from the commit message whether and why it is required here.
If this is kept from the removed block - maybe it is better to move the corresponding comment here as well.
Hi Andrev,
Thanks for your careful review!
Thanks for the patch series on the first place! This allows to switch uSDHC into high-speed modes, which is quite valuable.
Without this 5ms delay, with patch1 and remove the upper redundant cmd11 related code, mmc_switch_voltage() will fail, due to the second mmc_wait_dat0() return timeout. Seems for usdhc, gate off clock for 10ms is not enough. If total delay 15ms, then the second mmc_wait_dat0() can return normal. So I add 5ms delay here. Yes, I should add a comment for this 5ms in the
code.
Exactly this information is missing with the patch, as later on it would be quite difficult to grasp on why this mdelay() was added on the first place.
You can also do the test on your side.
I've already did and reported with the boot log, mmc info, and my Tested-by: tag.
I spend some time to dig into the extra 5ms, find this is board design issue. I test three boards, Imx8mn-ddr4-evk and imx8mp-evk board do not need this 5ms delay, only imx8mm-evkb board need this extra 5ms delay. This is because on imx8mm-evkb board, the voltage switch circuit is controlled by our PMIC, and this LD0 output connect one 10uF capacitance, makes the voltage switch from 3.3v to 1,8v need around 18ms. For other imx8mn and imx8mp board, the capacitance connected is only 1uF/4.7uF, the voltage switch time is 400us/8ms, so software 10ms delay can cover these two boards.
Seems this is board specific issue, I will send v2 patch, only imx8mm-evkb board need this delay, and will add a comment in the code.
Thanks!
Best Regards Haibo
if (esdhc_read32(®s->vendorspec) &ESDHC_VENDORSPEC_VSELECT)
return 0;-- 2.17.1
-- andrey
-- andrey
-----Original Message----- From: Bough Chen Sent: 2021年3月8日 17:50 To: ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com; Peng Fan peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: dl-uboot-imx uboot-imx@nxp.com; tharvey@gateworks.com; festevam@gmail.com; Ye Li ye.li@nxp.com Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
-----Original Message----- From: ZHIZHIKIN Andrey [mailto:andrey.zhizhikin@leica-geosystems.com] Sent: 2021年3月3日 20:50 To: Bough Chen haibo.chen@nxp.com; Peng Fan peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: dl-uboot-imx uboot-imx@nxp.com; tharvey@gateworks.com; festevam@gmail.com; Ye Li ye.li@nxp.com Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
Hello Haibo,
-----Original Message----- From: Bough Chen haibo.chen@nxp.com Sent: Wednesday, March 3, 2021 12:27 PM To: ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com; Peng Fan peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: dl-uboot-imx uboot-imx@nxp.com; tharvey@gateworks.com; festevam@gmail.com; Ye Li ye.li@nxp.com Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
-----Original Message----- From: ZHIZHIKIN Andrey [mailto:andrey.zhizhikin@leica-geosystems.com] Sent: 2021年3月3日 19:00 To: Bough Chen haibo.chen@nxp.com; Peng Fan peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: dl-uboot-imx uboot-imx@nxp.com; tharvey@gateworks.com; festevam@gmail.com; Ye Li ye.li@nxp.com Subject: RE: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
-----Original Message----- From: haibo.chen@nxp.com haibo.chen@nxp.com Sent: Wednesday, March 3, 2021 10:06 AM To: peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: haibo.chen@nxp.com; uboot-imx@nxp.com;
tharvey@gateworks.com;
ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com; festevam@gmail.com; ye.li@nxp.com Subject: [PATCH 2/2] mmc: fsl_esdhc_imx: remove redundant cmd11 related code.
From: Haibo Chen haibo.chen@nxp.com
Common code already handle the voltage switch sequence based on spec, so remove the redundant voltage switch code.
Signed-off-by: Haibo Chen haibo.chen@nxp.com
drivers/mmc/fsl_esdhc_imx.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index af36558b3c..a199cf3df6 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -515,15 +515,6 @@ static int esdhc_send_cmd_common(struct fsl_esdhc_priv *priv, struct mmc *mmc, goto out; }
/* Switch voltage to 1.8V if CMD11 succeeded */if (cmd->cmdidx == SD_CMD_SWITCH_UHS18V) {esdhc_setbits32(®s->vendorspec,ESDHC_VENDORSPEC_VSELECT);
printf("Run CMD11 1.8V switch\n");/* Sleep for 5 ms - max time for card to switch to1.8V
*/
udelay(5000);}/* Workaround for ESDHC errata ENGcm03648 */ if (!data && (cmd->resp_type & MMC_RSP_BUSY)) { int timeout = 50000; @@ -839,6 +830,7 @@ staticint esdhc_set_voltage(struct mmc *mmc) } #endif esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT);
mdelay(5);Why is this delay introduced here? It is not clear from the commit message whether and why it is required here.
If this is kept from the removed block - maybe it is better to move the corresponding comment here as well.
Hi Andrev,
Thanks for your careful review!
Thanks for the patch series on the first place! This allows to switch uSDHC
into
high-speed modes, which is quite valuable.
Without this 5ms delay, with patch1 and remove the upper redundant cmd11 related code, mmc_switch_voltage() will fail, due to the second mmc_wait_dat0() return timeout. Seems for usdhc, gate off clock for 10ms is not enough. If total delay 15ms, then the second mmc_wait_dat0() can return normal. So I add 5ms delay here. Yes, I should add a comment for this 5ms in the
code.
Exactly this information is missing with the patch, as later on it would be
quite
difficult to grasp on why this mdelay() was added on the first place.
You can also do the test on your side.
I've already did and reported with the boot log, mmc info, and my Tested-by: tag.
I spend some time to dig into the extra 5ms, find this is board design issue. I test three boards, Imx8mn-ddr4-evk and imx8mp-evk board do not need this 5ms delay, only imx8mm-evkb board need this extra 5ms delay. This is because on imx8mm-evkb board, the voltage switch circuit is controlled by our PMIC, and this LD0 output connect one 10uF capacitance, makes the voltage switch from 3.3v to 1,8v need around 18ms. For other imx8mn and imx8mp board, the capacitance connected is only 1uF/4.7uF, the voltage switch time is 400us/8ms, so software 10ms delay can cover these two boards.
Seems this is board specific issue, I will send v2 patch, only imx8mm-evkb board need this delay, and will add a comment in the code.
Hi all,
Double confirm with our board design team, this is also related with the PMIC chip. We are asking our PMIC team, whether can config the LDO transition time from the PMIC side. So let's first hold this patch.
But for patch 1, there is no problem, Peng/Stefano, can you help pick that patch? Only apply patch 1 can also fix the previous issue.
Best Regards Haibo Chen
Thanks!
Best Regards Haibo
if (esdhc_read32(®s->vendorspec) &ESDHC_VENDORSPEC_VSELECT)
return 0;-- 2.17.1
-- andrey
-- andrey
Hello Haibo,
-----Original Message----- From: haibo.chen@nxp.com haibo.chen@nxp.com Sent: Wednesday, March 3, 2021 10:06 AM To: peng.fan@nxp.com; u-boot@lists.denx.de; sbabic@denx.de Cc: haibo.chen@nxp.com; uboot-imx@nxp.com; tharvey@gateworks.com; ZHIZHIKIN Andrey andrey.zhizhikin@leica-geosystems.com; festevam@gmail.com; ye.li@nxp.com Subject: [PATCH 1/2] mmc: fsl_esdhc_imx: use VENDORSPEC_FRC_SDCLK_ON to control card clock output
From: Haibo Chen haibo.chen@nxp.com
For FSL_USDHC, it do not implement VENDORSPEC_CKEN/PEREN/HCKEN/IPGEN, these are reserved bits. Instead, use VENDORSPEC_FRC_SDCLK_ON to gate on/off the card clock output.
After commit b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0() support"), we meet SD3.0 card can't work at UHS mode, mmc_switch_voltage() fail because the second mmc_wait_dat0 return -ETIMEDOUT. According to SD spec, during voltage switch, need to gate off/on the card clock. If not set the FRC_SDCLK_ON, after CMD11, hardware will gate off the card clock automatically, so card do not detect the clock off/on behavior, so will draw the data0 line low until next command.
I believe this patch is a RESEND of the previous one, right? I do recall I was trying to test the same patch, but was missing the second one from this series.
Maybe it make sense to deprecate the old submission then, so it's not dangling in Patchwork.
Fixes: b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0() support") Tested-by: Tim Harvey tharvey@gateworks.com Signed-off-by: Haibo Chen haibo.chen@nxp.com
drivers/mmc/fsl_esdhc_imx.c | 29 +++++++++++++++++++++-------- include/fsl_esdhc_imx.h | 2 ++ 2 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc_imx.c b/drivers/mmc/fsl_esdhc_imx.c index e0e132698e..af36558b3c 100644 --- a/drivers/mmc/fsl_esdhc_imx.c +++ b/drivers/mmc/fsl_esdhc_imx.c @@ -654,7 +654,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);
esdhc_clrbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);ret = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp, tmp &PRSSTAT_SDOFF, 100);
if (ret)pr_warn("fsl_esdhc_imx: Internal clock never gate- off.\n");
#else esdhc_clrbits32(®s->sysctl, SYSCTL_CKEN); #endif @@ -666,7 +669,7 @@ static void set_sysctl(struct fsl_esdhc_priv *priv, struct mmc *mmc, uint clock) pr_warn("fsl_esdhc_imx: Internal clock never stabilised.\n");
#ifdef CONFIG_FSL_USDHC
esdhc_setbits32(®s->vendorspec, VENDORSPEC_PEREN |VENDORSPEC_CKEN);
esdhc_setbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);#else esdhc_setbits32(®s->sysctl, SYSCTL_PEREN | SYSCTL_CKEN); #endif @@ - 721,8 +724,14 @@ static void esdhc_set_strobe_dll(struct mmc *mmc) struct fsl_esdhc_priv *priv = dev_get_priv(mmc->dev); struct fsl_esdhc *regs = priv->esdhc_regs; u32 val;
u32 tmp;int ret; if (priv->clock > ESDHC_STROBE_DLL_CLK_FREQ) {esdhc_clrbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);ret = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp, tmp &PRSSTAT_SDOFF, 100);
if (ret)pr_warn("fsl_esdhc_imx: Internal clock nevergate off.\n"); esdhc_write32(®s->strobe_dllctrl, ESDHC_STROBE_DLL_CTRL_RESET);
/*@@ -740,6 +749,7 @@ static void esdhc_set_strobe_dll(struct mmc *mmc) pr_warn("HS400 strobe DLL status REF not lock!\n"); if (!(val & ESDHC_STROBE_DLL_STS_SLV_LOCK)) pr_warn("HS400 strobe DLL status SLV not lock!\n");
esdhc_setbits32(®s->vendorspec,- VENDORSPEC_FRC_SDCLK_ON); }
}
@@ -963,14 +973,18 @@ static int esdhc_set_ios_common(struct fsl_esdhc_priv *priv, struct mmc *mmc) #ifdef MMC_SUPPORTS_TUNING if (mmc->clk_disable) { #ifdef CONFIG_FSL_USDHC
esdhc_clrbits32(®s->vendorspec, VENDORSPEC_CKEN);
u32 tmp;esdhc_clrbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);ret = readx_poll_timeout(esdhc_read32, ®s->prsstat, tmp, tmp &PRSSTAT_SDOFF, 100);
if (ret)pr_warn("fsl_esdhc_imx: Internal clock never- gate off.\n");
#else esdhc_clrbits32(®s->sysctl, SYSCTL_CKEN); #endif } else { #ifdef CONFIG_FSL_USDHC
esdhc_setbits32(®s->vendorspec, VENDORSPEC_PEREN |VENDORSPEC_CKEN);
esdhc_setbits32(®s->vendorspec,- VENDORSPEC_FRC_SDCLK_ON);
#else esdhc_setbits32(®s->sysctl, SYSCTL_PEREN | SYSCTL_CKEN); #endif @@ -1046,7 +1060,7 @@ static int esdhc_init_common(struct fsl_esdhc_priv *priv, struct mmc *mmc) #ifndef CONFIG_FSL_USDHC esdhc_setbits32(®s->sysctl, SYSCTL_HCKEN | SYSCTL_IPGEN); #else
esdhc_setbits32(®s->vendorspec, VENDORSPEC_HCKEN |VENDORSPEC_IPGEN);
esdhc_setbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);#endif
/* Set the initial clock speed */ @@ -1184,8 +1198,7 @@ static intfsl_esdhc_init(struct fsl_esdhc_priv *priv, esdhc_write32(®s->autoc12err, 0); esdhc_write32(®s->clktunectrlstatus, 0); #else
esdhc_setbits32(®s->vendorspec, VENDORSPEC_PEREN |VENDORSPEC_HCKEN | VENDORSPEC_IPGEN | VENDORSPEC_CKEN);
esdhc_setbits32(®s->vendorspec, VENDORSPEC_FRC_SDCLK_ON);#endif
if (priv->vs18_enable)diff --git a/include/fsl_esdhc_imx.h b/include/fsl_esdhc_imx.h index 45ed635a77..b092034464 100644 --- a/include/fsl_esdhc_imx.h +++ b/include/fsl_esdhc_imx.h @@ -39,6 +39,7 @@ #define VENDORSPEC_HCKEN 0x00001000 #define VENDORSPEC_IPGEN 0x00000800 #define VENDORSPEC_INIT 0x20007809 +#define VENDORSPEC_FRC_SDCLK_ON 0x00000100
#define IRQSTAT 0x0002e030 #define IRQSTAT_DMAE (0x10000000) @@ -96,6 +97,7 @@ #define PRSSTAT_CINS (0x00010000) #define PRSSTAT_BREN (0x00000800) #define PRSSTAT_BWEN (0x00000400) +#define PRSSTAT_SDOFF (0x00000080) #define PRSSTAT_SDSTB (0X00000008) #define PRSSTAT_DLA (0x00000004)
#define PRSSTAT_CICHB (0x00000002)
2.17.1
-- andrey
From: Haibo Chen haibo.chen@nxp.com For FSL_USDHC, it do not implement VENDORSPEC_CKEN/PEREN/HCKEN/IPGEN, these are reserved bits. Instead, use VENDORSPEC_FRC_SDCLK_ON to gate on/off the card clock output. After commit b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0() support"), we meet SD3.0 card can't work at UHS mode, mmc_switch_voltage() fail because the second mmc_wait_dat0 return -ETIMEDOUT. According to SD spec, during voltage switch, need to gate off/on the card clock. If not set the FRC_SDCLK_ON, after CMD11, hardware will gate off the card clock automatically, so card do not detect the clock off/on behavior, so will draw the data0 line low until next command. Fixes: b5874b552ffa ("mmc: fsl_esdhc_imx: add wait_dat0() support") Tested-by: Tim Harvey tharvey@gateworks.com Signed-off-by: Haibo Chen haibo.chen@nxp.com
Applied to u-boot-imx, master, thanks !
Best regards, Stefano Babic
participants (5)
-
Adam Ford -
Bough Chen -
haibo.chen＠nxp.com
-
sbabic＠denx.de -
ZHIZHIKIN Andrey