On 15.03.2012 10:56, Stefano Babic wrote:
On 08/03/2012 13:36, Dirk Behme wrote:
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.
Signed-off-by: Dirk Behme <dirk.be...@de.bosch.com>
CC: Andy Fleming <aflem...@freescale.com>
CC: Fabio Estevam <fabio.este...@freescale.com>
---
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
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
and the third
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
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
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
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.
All together makes your
patch not so easy to read.
include/fsl_esdhc.h | 4 ++-
2 files changed, 56 insertions(+), 9 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
index 30db030..694d6fd 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc.c
@@ -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);
-
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.
/* 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
Yes, ack.
+ 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 ?
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?
/* 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);
+ }
+
Ok, you reset the DAT lines (according to the manual) before returning
the error.
if (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...
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
