mmc: fsl_esdhc: Add workaround for auto-clock gate errata ENGcm03648
PATCH 1/1 mmc: fsl_esdhc: Add workaround for auto-clock gate errata ENGcm03648
This patch imports three patches from the Freescale U-Boot with the following commit messages:
ENGR00156405 ESDHC: Add workaround for auto-clock gate errata ENGcm03648
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=e436525a70fe47623d346bc7d9f08f12ff8ad787
The errata, not applicable to USDHC, causes ESDHC to shut off clock to the card when auto-clock gating is enabled for commands with busy signalling and no data phase. The card might require the clock to exit the busy state, so the workaround is to disable the auto-clock gate bits in SYSCTL register for such commands. The workaround also entails polling on DAT0 bit in the PRSSTAT register to learn when busy state is complete. Auto-clock gating is re-enabled at the end of busy state.
ENGR00156670-1 ESDHC/USDHC: Remove delay before each cmd and some bug fixes
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=a77c6fec8596891be96b2cdbc742c9824844b92a
Removed delay of 10 ms before each command. There should not be a need to have this delay after the ENGR00156405 patch that polls until card is not busy anymore before proceeding to next cmd.
ENGR00161852: remove u-boot build warnings for mx6q
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=97efee177f82b082db9d2019ed641c5b99b3f54b
Remove u-boot build warnings for mx6q.
SYSCTL_RSTA was defined twice. Remove one definition.
--- drivers/mmc/fsl_esdhc.c | 61 ++++++++++++++++++++++++++++++++++++++++------ include/fsl_esdhc.h | 4 ++- 2 files changed, 56 insertions(+), 9 deletions(-)
drivers/mmc/fsl_esdhc.c +53 -8
@@ -259,6 +259,7 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) {uint xfertyp;uint irqstat; +uint sysctl_restore =0 ;struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv;volatile struct fsl_esdhc *regs = (struct fsl_esdhc *)cfg->esdhc_base;
@@ -279,13 +280,6 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)while (esdhc_read32 (®s->prsstat) &PRSSTAT_DLA ) ; -/* Wait at least 8 SD clock cycles before the next command */ -/* -* Note: This is way more than 8 cycles, but 1ms seems to -* resolve timing issues with some cards -*/ -udelay (1000 ); -/* Set up for a data transfer if we have one */ if (data) {int err;
@@ -298,6 +292,14 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)/* Figure out the transfer arguments */ xfertyp =esdhc_xfertyp (cmd, data); +/* ESDHC errata ENGcm03648: Turn off auto-clock gate for commands +* with busy signaling and no data +*/ +if (!data && (cmd->resp_type &MMC_RSP_BUSY )) { + sysctl_restore =esdhc_read32 (®s->sysctl); +esdhc_write32 (®s->sysctl, sysctl_restore |0xF ); + } +/* Send the command */ esdhc_write32 (®s->cmdarg, cmd->cmdarg);#if defined(CONFIG_FSL_USDHC )
@@ -307,19 +309,62 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)#else esdhc_write32 (®s->xfertyp, xfertyp);#endif + +/* Mask all irqs */ +esdhc_write32 (®s->irqsigen,0 ); +/* Wait for the command to complete */ -while (!(esdhc_read32 (®s->irqstat) &IRQSTAT_CC )) +while (!(esdhc_read32 (®s->irqstat) & (IRQSTAT_CC |IRQSTAT_CTOE ))) ; irqstat =esdhc_read32 (®s->irqstat);esdhc_write32 (®s->irqstat, irqstat); +/* Reset CMD and DATA portions on error */ +if (irqstat & (CMD_ERR |IRQSTAT_CTOE )) { +esdhc_write32 (®s->sysctl,esdhc_read32 (®s->sysctl) | +SYSCTL_RSTC ); +while (esdhc_read32 (®s->sysctl) &SYSCTL_RSTC ) + ; + +if (data) { +esdhc_write32 (®s->sysctl, +esdhc_read32 (®s->sysctl) | +SYSCTL_RSTD ); +while ((esdhc_read32 (®s->sysctl) &SYSCTL_RSTD )) + ; + } + +/* Restore auto-clock gate if error */ +if (!data && (cmd->resp_type &MMC_RSP_BUSY )) +esdhc_write32 (®s->sysctl, sysctl_restore); + } +if (irqstat &CMD_ERR )return COMM_ERR ;if (irqstat &IRQSTAT_CTOE )return TIMEOUT ; +/* Workaround for ESDHC errata ENGcm03648 */ +if (!data && (cmd->resp_type &MMC_RSP_BUSY )) { +int timeout =2500 ; + +/* Poll on DATA0 line for cmd with busy signal for 250 ms */ +while (timeout >0 && !(esdhc_read32 (®s->prsstat) & +PRSSTAT_DAT0 )) { +udelay (100 ); + timeout--; + } + +esdhc_write32 (®s->sysctl, sysctl_restore); + +if (timeout <=0 ) { +printf ("Timeout waiting for DAT0 to go high!\n" ); +return TIMEOUT ; + } + } +/* Copy the response to the response buffer */ if (cmd->resp_type &MMC_RSP_136 ) {u32 cmdrsp3, cmdrsp2, cmdrsp1, cmdrsp0;
include/fsl_esdhc.h +3 -1
@@ -34,12 +34,13 @@#define SYSCTL_INITA 0x08000000#define SYSCTL_TIMEOUT_MASK 0x000f0000#define SYSCTL_CLOCK_MASK 0x0000fff0 -#define SYSCTL_RSTA 0x01000000#define SYSCTL_CKEN 0x00000008#define SYSCTL_PEREN 0x00000004#define SYSCTL_HCKEN 0x00000002#define SYSCTL_IPGEN 0x00000001#define SYSCTL_RSTA 0x01000000 +#define SYSCTL_RSTC 0x02000000 +#define SYSCTL_RSTD 0x04000000#define IRQSTAT 0x0002e030#define IRQSTAT_DMAE (0x10000000)
@@ -85,6 +86,7 @@#define IRQSTATEN_CC (0x00000001)#define PRSSTAT 0x0002e024 +#define PRSSTAT_DAT0 (0x01000000)#define PRSSTAT_CLSL (0x00800000)#define PRSSTAT_WPSPL (0x00080000)#define PRSSTAT_CDPL (0x00040000)
Signature · 1 line
1.7.0.4
PATCH 1/1 mmc: fsl_esdhc: Add workaround for auto-clock gate errata ENGcm03648
Quoted 22 lines · Dirk Behme
This patch imports three patches from the Freescale U-Boot with the following commit messages:
ENGR00156405 ESDHC: Add workaround for auto-clock gate errata ENGcm03648
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=e436525a70fe47623d346bc7d9f08f12ff8ad787
The errata, not applicable to USDHC, causes ESDHC to shut off clock to the card when auto-clock gating is enabled for commands with busy signalling and no data phase. The card might require the clock to exit the busy state, so the workaround is to disable the auto-clock gate bits in SYSCTL register for such commands. The workaround also entails polling on DAT0 bit in the PRSSTAT register to learn when busy state is complete. Auto-clock gating is re-enabled at the end of busy state.ENGR00156670-1 ESDHC/USDHC: Remove delay before each cmd and some bug fixes
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=a77c6fec8596891be96b2cdbc742c9824844b92a
Removed delay of 10 ms before each command. There should not be a need to have this delay after the ENGR00156405 patch that polls until card is not busy anymore before proceeding to next cmd.ENGR00161852: remove u-boot build warnings for mx6q
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=97efee177f82b082db9d2019ed641c5b99b3f54b
Remove u-boot build warnings for mx6q.
Quoted 7 lines
SYSCTL_RSTAwas defined twice. Remove one definition.Signed-off-by Dirk Behme <[email protected]>CC Andy Fleming <[email protected]>CC Fabio Estevam <[email protected]>--- drivers/mmc/fsl_esdhc.c | 61 ++++++++++++++++++++++++++++++++++++++++------
This is not a blocking point for me, but is it maybe better to split this into three patches as it was originally ? All together makes your patch not so easy to read.
Quoted 16 lines
include/fsl_esdhc.h | 4 ++- 2 files changed, 56 insertions(+), 9 deletions(-)drivers/mmc/fsl_esdhc.c +1
-0@@ -259,6 +259,7 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) {uint xfertyp;uint irqstat; +uint sysctl_restore =0 ;struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv;volatile struct fsl_esdhc *regs = (struct fsl_esdhc *)cfg->esdhc_base;
drivers/mmc/fsl_esdhc.c esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) · line 288
Quoted 10 lines
drivers/mmc/fsl_esdhc.c +0
-7@@ -279,13 +280,6 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)while (esdhc_read32 (®s->prsstat) &PRSSTAT_DLA ) ; -/* Wait at least 8 SD clock cycles before the next command */ -/* -* Note: This is way more than 8 cycles, but 1ms seems to -* resolve timing issues with some cards -*/ -udelay (1000 ); -
You drop a delay - is it ok for PowerPC, too ? Maybe some PowerPC guys can answer to this.
drivers/mmc/fsl_esdhc.c esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) · line 297
Quoted 10 lines
drivers/mmc/fsl_esdhc.c +3
-0/* Set up for a data transfer if we have one */ if (data) {int err;@@ -298,6 +292,14 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)/* Figure out the transfer arguments */ xfertyp =esdhc_xfertyp (cmd, data); +/* ESDHC errata ENGcm03648: Turn off auto-clock gate for commands +* with busy signaling and no data +*/
Wrong multiline comment
drivers/mmc/fsl_esdhc.c esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) · line 302
Quoted 5 lines
drivers/mmc/fsl_esdhc.c +5
-0+if (!data && (cmd->resp_type &MMC_RSP_BUSY )) { + sysctl_restore =esdhc_read32 (®s->sysctl); +esdhc_write32 (®s->sysctl, sysctl_restore |0xF ); + } +
I see two issue. There is not a SDCLKEN bit in the PowerPC ESDHC, as far as I can see (for example, in MPC8536). Should we not use the HOSTVER register to handle these cases ?
The comment says you are disabling auto-clock. However, in the register you are enabling all clocks (PEREN / SDCLKEN /..). Can you explain better the reason ? Do you want really disabling clocks ?
Quoted 29 lines
drivers/mmc/fsl_esdhc.c +15
-1/* Send the command */ esdhc_write32 (®s->cmdarg, cmd->cmdarg);#if defined(CONFIG_FSL_USDHC )@@ -307,19 +309,62 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)#else esdhc_write32 (®s->xfertyp, xfertyp);#endif + +/* Mask all irqs */ +esdhc_write32 (®s->irqsigen,0 ); +/* Wait for the command to complete */ -while (!(esdhc_read32 (®s->irqstat) &IRQSTAT_CC )) +while (!(esdhc_read32 (®s->irqstat) & (IRQSTAT_CC |IRQSTAT_CTOE ))) ; irqstat =esdhc_read32 (®s->irqstat);esdhc_write32 (®s->irqstat, irqstat); +/* Reset CMD and DATA portions on error */ +if (irqstat & (CMD_ERR |IRQSTAT_CTOE )) { +esdhc_write32 (®s->sysctl,esdhc_read32 (®s->sysctl) | +SYSCTL_RSTC ); +while (esdhc_read32 (®s->sysctl) &SYSCTL_RSTC ) + ; + +if (data) { + esdhc_write32(®s->sysctl, + esdhc_read32(®s->sysctl) |
drivers/mmc/fsl_esdhc.c esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) · line 342
Quoted 10 lines
drivers/mmc/fsl_esdhc.c +10
-0+ SYSCTL_RSTD); +while ((esdhc_read32 (®s->sysctl) &SYSCTL_RSTD )) + ; + } + +/* Restore auto-clock gate if error */ +if (!data && (cmd->resp_type &MMC_RSP_BUSY )) +esdhc_write32 (®s->sysctl, sysctl_restore); + } +
Ok, you reset the DAT lines (according to the manual) before returning the error.
drivers/mmc/fsl_esdhc.c esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) · line 349
Quoted 7 lines
drivers/mmc/fsl_esdhc.c +1
-0if (irqstat &CMD_ERR )return COMM_ERR ;if (irqstat &IRQSTAT_CTOE )return TIMEOUT ; +/* Workaround for ESDHC errata ENGcm03648 */
This ENGcm03648 is not mentioned in the commit message, it seems another issue...
Best regards,
Stefano Babic
Signature · 5 lines
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de =====================================================================
PATCH 1/1 mmc: fsl_esdhc: Add workaround for auto-clock gate errata ENGcm03648
Quoted 25 lines · Stefano Babic
Quoted 24 lines · Dirk Behme
This patch imports three patches from the Freescale U-Boot with the following commit messages:
ENGR00156405 ESDHC: Add workaround for auto-clock gate errata ENGcm03648
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=e436525a70fe47623d346bc7d9f08f12ff8ad787
The errata, not applicable to USDHC, causes ESDHC to shut off clock to the card when auto-clock gating is enabled for commands with busy signalling and no data phase. The card might require the clock to exit the busy state, so the workaround is to disable the auto-clock gate bits in SYSCTL register for such commands. The workaround also entails polling on DAT0 bit in the PRSSTAT register to learn when busy state is complete. Auto-clock gating is re-enabled at the end of busy state.ENGR00156670-1 ESDHC/USDHC: Remove delay before each cmd and some bug fixes
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=a77c6fec8596891be96b2cdbc742c9824844b92a
Removed delay of 10 ms before each command. There should not be a need to have this delay after the ENGR00156405 patch that polls until card is not busy anymore before proceeding to next cmd.ENGR00161852: remove u-boot build warnings for mx6q
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=97efee177f82b082db9d2019ed641c5b99b3f54b
Remove u-boot build warnings for mx6q.
SYSCTL_RSTAwas defined twice. Remove one definition.
Quoted 8 lines
Quoted 5 lines
Signed-off-by Dirk Behme <[email protected]>CC Andy Fleming <[email protected]>CC Fabio Estevam <[email protected]>--- drivers/mmc/fsl_esdhc.c | 61 ++++++++++++++++++++++++++++++++++++++++------This is not a blocking point for me, but is it maybe better to split this into three patches as it was originally ?
Hmm, I'm unsure about this. Looking at the 3 referenced Freescale patches and at the way the Freescale commits look, I was under the impression that the first patch was improved by Freescale two times to the then 'final' state.
At least the first patch
and the third
definitely should be one patch. The third one wouldn't be there if there would have been a proper review of the first one ;)
And the commit message of the second patch
mentions that the delay can be removed due to the first patch. So I was under the impression that this could have been done in the first patch, already, too.
But anyway, as you said, this shouldn't be a blocker.
Quoted 23 lines
All together makes your
patch not so easy to read.Quoted 19 lines
include/fsl_esdhc.h | 4 ++- 2 files changed, 56 insertions(+), 9 deletions(-)drivers/mmc/fsl_esdhc.c +1
-0@@ -259,6 +259,7 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) {uint xfertyp;uint irqstat; +uint sysctl_restore =0 ;struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv;volatile struct fsl_esdhc *regs = (struct fsl_esdhc *)cfg->esdhc_base;@@ -279,13 +280,6 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)while (esdhc_read32 (®s->prsstat) &PRSSTAT_DLA ) ;
Quoted 10 lines
drivers/mmc/fsl_esdhc.c esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) · line 288
Quoted 7 lines
drivers/mmc/fsl_esdhc.c +0
-7-/* Wait at least 8 SD clock cycles before the next command */ -/* -* Note: This is way more than 8 cycles, but 1ms seems to -* resolve timing issues with some cards -*/ -udelay (1000 ); -You drop a delay - is it ok for PowerPC, too ? Maybe some PowerPC guys can answer to this.
Regarding PowerPC I have no idea. Yes, it would be nice if the PowerPC guys could comment.
Quoted 7 lines
Quoted 7 lines
drivers/mmc/fsl_esdhc.c +0
-0/* Set up for a data transfer if we have one */ if (data) {int err;@@ -298,6 +292,14 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)/* Figure out the transfer arguments */ xfertyp =esdhc_xfertyp (cmd, data);
Quoted 5 lines
drivers/mmc/fsl_esdhc.c esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) · line 297
Quoted 3 lines
drivers/mmc/fsl_esdhc.c +3
-0+/* ESDHC errata ENGcm03648: Turn off auto-clock gate for commands +* with busy signaling and no data +*/ Wrong multiline comment
Yes, ack.
Quoted 10 lines
drivers/mmc/fsl_esdhc.c esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) · line 302
Quoted 5 lines
drivers/mmc/fsl_esdhc.c +5
-0+if (!data && (cmd->resp_type &MMC_RSP_BUSY )) { + sysctl_restore =esdhc_read32 (®s->sysctl); +esdhc_write32 (®s->sysctl, sysctl_restore |0xF ); + } +I see two issue. There is not a SDCLKEN bit in the PowerPC ESDHC, as far as I can see (for example, in MPC8536). Should we not use the HOSTVER register to handle these cases ?
Quoted 3 lines
The comment says you are disabling auto-clock. However, in the register you are enabling all clocks (PEREN / SDCLKEN /..). Can you explain better the reason ? Do you want really disabling clocks ?
I have to admit that I have no idea :(
I'm no SD/MMC expert. This patch was created doing a functional diff. On a custom board we found that the imx_esdhc.c driver in Freescale's old U-Boot works fine, while the mainline fsl_esdhc.c we talk about here doesn't. Then, we traced it down to the changes we talk about here and ported them to the mainline fsl_esdhc.c. This fixes our issue. Unfortunately, there is no real understanding what these changes do.
Could any SD expert help with this?
Quoted 46 lines
Quoted 29 lines
drivers/mmc/fsl_esdhc.c +15
-1/* Send the command */ esdhc_write32 (®s->cmdarg, cmd->cmdarg);#if defined(CONFIG_FSL_USDHC )@@ -307,19 +309,62 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)#else esdhc_write32 (®s->xfertyp, xfertyp);#endif + +/* Mask all irqs */ +esdhc_write32 (®s->irqsigen,0 ); +/* Wait for the command to complete */ -while (!(esdhc_read32 (®s->irqstat) &IRQSTAT_CC )) +while (!(esdhc_read32 (®s->irqstat) & (IRQSTAT_CC |IRQSTAT_CTOE ))) ; irqstat =esdhc_read32 (®s->irqstat);esdhc_write32 (®s->irqstat, irqstat); +/* Reset CMD and DATA portions on error */ +if (irqstat & (CMD_ERR |IRQSTAT_CTOE )) { +esdhc_write32 (®s->sysctl,esdhc_read32 (®s->sysctl) | +SYSCTL_RSTC ); +while (esdhc_read32 (®s->sysctl) &SYSCTL_RSTC ) + ; + +if (data) { + esdhc_write32(®s->sysctl, + esdhc_read32(®s->sysctl) |drivers/mmc/fsl_esdhc.c esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) · line 342
Quoted 10 lines
drivers/mmc/fsl_esdhc.c +10
-0+ SYSCTL_RSTD); +while ((esdhc_read32 (®s->sysctl) &SYSCTL_RSTD )) + ; + } + +/* Restore auto-clock gate if error */ +if (!data && (cmd->resp_type &MMC_RSP_BUSY )) +esdhc_write32 (®s->sysctl, sysctl_restore); + } +Ok, you reset the DAT lines (according to the manual) before returning the error.
Quoted 3 lines
drivers/mmc/fsl_esdhc.c +0
-0if (irqstat &CMD_ERR )return COMM_ERR ;
Quoted 7 lines
drivers/mmc/fsl_esdhc.c esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) · line 349
Quoted 4 lines
drivers/mmc/fsl_esdhc.c +1
-0if (irqstat &IRQSTAT_CTOE )return TIMEOUT ; +/* Workaround for ESDHC errata ENGcm03648 */ This ENGcm03648 is not mentioned in the commit message, it seems another issue...
Hmm? The subject is about "errata ENGcm03648" And the commit messages refers to
...
ENGR00156405 ESDHC: Add workaround for auto-clock gate errata ENGcm03648
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=e436525a70fe47623d346bc7d9f08f12ff8ad787
...
Sorry if I overlooked anything.
Best regards
Dirk
PATCH 1/1 mmc: fsl_esdhc: Add workaround for auto-clock gate errata ENGcm03648
Quoted 20 lines · Dirk Behme
Hmm, I'm unsure about this. Looking at the 3 referenced Freescale patches and at the way the Freescale commits look, I was under the impression that the first patch was improved by Freescale two times to the then 'final' state.
At least the first patch
and the third
definitely should be one patch. The third one wouldn't be there if there would have been a proper review of the first one ;)
And the commit message of the second patch
Quoted 8 lines
mentions that the delay can be removed due to the first patch. So I was under the impression that this could have been done in the first patch, already, too.
But anyway, as you said, this shouldn't be a blocker.
No, it is not. I am fine with your explanation.
Quoted 14 lines
Quoted 6 lines
drivers/mmc/fsl_esdhc.c esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) · line 302
Quoted 5 lines
drivers/mmc/fsl_esdhc.c +5
-0+if (!data && (cmd->resp_type &MMC_RSP_BUSY )) { + sysctl_restore =esdhc_read32 (®s->sysctl); +esdhc_write32 (®s->sysctl, sysctl_restore |0xF ); + } +Quoted 7 lines
I see two issue. There is not a SDCLKEN bit in the PowerPC ESDHC, as far as I can see (for example, in MPC8536). Should we not use the HOSTVER register to handle these cases ?
The comment says you are disabling auto-clock. However, in the register you are enabling all clocks (PEREN / SDCLKEN /..). Can you explain better the reason ? Do you want really disabling clocks ?
Quoted 8 lines
I have to admit that I have no idea :(
I'm no SD/MMC expert. This patch was created doing a functional diff. On a custom board we found that the
imx_esdhc.cdriver in Freescale's old U-Boot works fine, while the mainlinefsl_esdhc.cwe talk about here doesn't. Then, we traced it down to the changes we talk about here and ported them to the mainlinefsl_esdhc.c. This fixes our issue. Unfortunately, there is no real understanding what these changes do.
At least the comment seems wrong : code is enabling cloks in the sysctl register. You should change or drop the comment.
Quoted 11 lines
Could any SD expert help with this?
Quoted 2 lines
This ENGcm03648 is not mentioned in the commit message, it seems another issue...
Hmm? The subject is about "errata ENGcm03648" And the commit messages refers to
...
ENGR00156405 ESDHC: Add workaround for auto-clock gate errata ENGcm03648
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=e436525a70fe47623d346bc7d9f08f12ff8ad787
Ah, I see, I was confused with the ENG numbers...
Best regards,
Stefano Babic
Signature · 5 lines
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de =====================================================================
PATCH 1/1 mmc: fsl_esdhc: Add workaround for auto-clock gate errata ENGcm03648
In reply to Stefano BabicQuoted 55 lines · Stefano Babic
Quoted 22 lines · Dirk Behme
This patch imports three patches from the Freescale U-Boot with the following commit messages:
ENGR00156405 ESDHC: Add workaround for auto-clock gate errata ENGcm03648
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=e436525a70fe47623d346bc7d9f08f12ff8ad787
The errata, not applicable to USDHC, causes ESDHC to shut off clock to the card when auto-clock gating is enabled for commands with busy signalling and no data phase. The card might require the clock to exit the busy state, so the workaround is to disable the auto-clock gate bits in SYSCTL register for such commands. The workaround also entails polling on DAT0 bit in the PRSSTAT register to learn when busy state is complete. Auto-clock gating is re-enabled at the end of busy state.ENGR00156670-1 ESDHC/USDHC: Remove delay before each cmd and some bug fixes
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=a77c6fec8596891be96b2cdbc742c9824844b92a
Removed delay of 10 ms before each command. There should not be a need to have this delay after the ENGR00156405 patch that polls until card is not busy anymore before proceeding to next cmd.ENGR00161852: remove u-boot build warnings for mx6q
http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/drivers/mmc/imx_esdhc.c?h=imx_v2009.08_12.01.01&id=97efee177f82b082db9d2019ed641c5b99b3f54b
Remove u-boot build warnings for mx6q.Quoted 7 lines
SYSCTL_RSTAwas defined twice. Remove one definition.Signed-off-by Dirk Behme <[email protected]>CC Andy Fleming <[email protected]>CC Fabio Estevam <[email protected]>--- drivers/mmc/fsl_esdhc.c | 61 ++++++++++++++++++++++++++++++++++++++++------This is not a blocking point for me, but is it maybe better to split this into three patches as it was originally ? All together makes your patch not so easy to read.
Quoted 19 lines
include/fsl_esdhc.h | 4 ++- 2 files changed, 56 insertions(+), 9 deletions(-)drivers/mmc/fsl_esdhc.c +1
-0@@ -259,6 +259,7 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) {uint xfertyp;uint irqstat; +uint sysctl_restore =0 ;struct fsl_esdhc_cfg *cfg = (struct fsl_esdhc_cfg *)mmc->priv;volatile struct fsl_esdhc *regs = (struct fsl_esdhc *)cfg->esdhc_base;@@ -279,13 +280,6 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)while (esdhc_read32 (®s->prsstat) &PRSSTAT_DLA ) ;
Quoted 10 lines
drivers/mmc/fsl_esdhc.c esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) · line 288
Quoted 7 lines
drivers/mmc/fsl_esdhc.c +0
-7-/* Wait at least 8 SD clock cycles before the next command */ -/* -* Note: This is way more than 8 cycles, but 1ms seems to -* resolve timing issues with some cards -*/ -udelay (1000 ); -You drop a delay - is it ok for PowerPC, too ? Maybe some PowerPC guys can answer to this.
There seems to be no comment from the PowerPC guys regarding this, yet. For safety, I'll drop this udelay() removal in a v2 of this patch, then.
Quoted 23 lines
drivers/mmc/fsl_esdhc.c esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) · line 297
Quoted 10 lines
drivers/mmc/fsl_esdhc.c +3
-0/* Set up for a data transfer if we have one */ if (data) {int err;@@ -298,6 +292,14 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)/* Figure out the transfer arguments */ xfertyp =esdhc_xfertyp (cmd, data); +/* ESDHC errata ENGcm03648: Turn off auto-clock gate for commands +* with busy signaling and no data +*/ Wrong multiline comment
drivers/mmc/fsl_esdhc.c esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) · line 302
Quoted 5 lines
drivers/mmc/fsl_esdhc.c +5
-0+if (!data && (cmd->resp_type &MMC_RSP_BUSY )) { + sysctl_restore =esdhc_read32 (®s->sysctl); +esdhc_write32 (®s->sysctl, sysctl_restore |0xF ); + } +I see two issue. There is not a SDCLKEN bit in the PowerPC ESDHC, as far as I can see (for example, in MPC8536). Should we not use the HOSTVER register to handle these cases ?
Quoted 3 lines
The comment says you are disabling auto-clock. However, in the register you are enabling all clocks (PEREN / SDCLKEN /..). Can you explain better the reason ? Do you want really disabling clocks ?
Thanks for letting me look more closely on this. The 4 bits which are touched with 0xF above seem to be marked as "Not implemented" in the i.MX6 spec. Testing the patch without any touching of sysctl still works. I.e. I will remove the sysctl handling from the v2, too.
Many thanks and best regards
Dirk