From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dirk Behme Date: Sat, 17 Mar 2012 10:33:37 +0100 Subject: [U-Boot] [PATCH] mmc: fsl_esdhc: Add workaround for auto-clock gate errata ENGcm03648 In-Reply-To: <4F61BCCF.9060306@denx.de> References: <1331210198-11818-1-git-send-email-dirk.behme@de.bosch.com> <4F61BCCF.9060306@denx.de> Message-ID: <4F645A71.3020205@de.bosch.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> CC: Andy Fleming >> CC: Fabio Estevam >> --- >> 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