| Message ID | 1485771118-13748-3-git-send-email-rk@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 30 January 2017 at 11:11, Ravikumar Kattekola <rk@ti.com> wrote: > From: Kishon Vijay Abraham I <kishon@ti.com> > > commit e2bf08d643a244ccb ("omap_hsmmc: set a large data timeout for > commands with busy signal") sets an arbitrary timeout value (100ms) for > commands like CMD6 (MMC SWITCH). However extended CSD register defined > in the eMMC standard has a field for GENERIC_CMD6_TIME which indicates > the default maximum timeout for a SWITCH command. > Use busy_timeout of cmd structure (populated with GENERIC_CMD6_TIME > in the case of SWITCH command) to program the data timeout value in > omap_hsmmc driver. > SWITCH command to turn the cache on took more than 100ms to complete > with MICRON eMMC card present in AM572x IDK REV 1.3A resulting in > timeout and failed enumeration. It is fixed here by programming the > timeout with the value advertised in GENERIC_CMD6_TIME. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > Signed-off-by: Sekhar Nori <nsekhar@ti.com> > Signed-off-by: Ravikumar Kattekola <rk@ti.com> > --- > drivers/mmc/host/omap_hsmmc.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c > index 0ee5650..51ca3d7 100644 > --- a/drivers/mmc/host/omap_hsmmc.c > +++ b/drivers/mmc/host/omap_hsmmc.c > @@ -1527,16 +1527,24 @@ static void omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host) > omap_hsmmc_prepare_data(struct omap_hsmmc_host *host, struct mmc_request *req) > { > int ret; > + unsigned int timeout; > + > host->data = req->data; > > if (req->data == NULL) { > OMAP_HSMMC_WRITE(host->base, BLK, 0); > - /* > - * Set an arbitrary 100ms data timeout for commands with > - * busy signal. > - */ > - if (req->cmd->flags & MMC_RSP_BUSY) > - set_data_timeout(host, 100000000U, 0); > + if (req->cmd->flags & MMC_RSP_BUSY) { > + timeout = req->cmd->busy_timeout * NSEC_PER_MSEC; > + > + /* > + * Set an arbitrary 100ms data timeout for commands with > + * busy signal and no indication of busy_timeout. > + */ > + if (!timeout) This is a bug in the mmc core if this ever happen. Therefore I am particularly interested to find out if this is really needed or it's just playing safe? > + timeout = 100000000U; > + > + set_data_timeout(host, timeout, 0); > + } > return 0; > } > > -- > 1.9.1 > Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 31 January 2017 03:42 PM, Ulf Hansson wrote: > On 30 January 2017 at 11:11, Ravikumar Kattekola <rk@ti.com> wrote: >> From: Kishon Vijay Abraham I <kishon@ti.com> >> >> commit e2bf08d643a244ccb ("omap_hsmmc: set a large data timeout for >> commands with busy signal") sets an arbitrary timeout value (100ms) for >> commands like CMD6 (MMC SWITCH). However extended CSD register defined >> in the eMMC standard has a field for GENERIC_CMD6_TIME which indicates >> the default maximum timeout for a SWITCH command. >> Use busy_timeout of cmd structure (populated with GENERIC_CMD6_TIME >> in the case of SWITCH command) to program the data timeout value in >> omap_hsmmc driver. >> SWITCH command to turn the cache on took more than 100ms to complete >> with MICRON eMMC card present in AM572x IDK REV 1.3A resulting in >> timeout and failed enumeration. It is fixed here by programming the >> timeout with the value advertised in GENERIC_CMD6_TIME. >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> Signed-off-by: Sekhar Nori <nsekhar@ti.com> >> Signed-off-by: Ravikumar Kattekola <rk@ti.com> >> --- >> drivers/mmc/host/omap_hsmmc.c | 20 ++++++++++++++------ >> 1 file changed, 14 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c >> index 0ee5650..51ca3d7 100644 >> --- a/drivers/mmc/host/omap_hsmmc.c >> +++ b/drivers/mmc/host/omap_hsmmc.c >> @@ -1527,16 +1527,24 @@ static void omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host) >> omap_hsmmc_prepare_data(struct omap_hsmmc_host *host, struct mmc_request *req) >> { >> int ret; >> + unsigned int timeout; >> + >> host->data = req->data; >> >> if (req->data == NULL) { >> OMAP_HSMMC_WRITE(host->base, BLK, 0); >> - /* >> - * Set an arbitrary 100ms data timeout for commands with >> - * busy signal. >> - */ >> - if (req->cmd->flags & MMC_RSP_BUSY) >> - set_data_timeout(host, 100000000U, 0); >> + if (req->cmd->flags & MMC_RSP_BUSY) { >> + timeout = req->cmd->busy_timeout * NSEC_PER_MSEC; >> + >> + /* >> + * Set an arbitrary 100ms data timeout for commands with >> + * busy signal and no indication of busy_timeout. >> + */ >> + if (!timeout) > This is a bug in the mmc core if this ever happen. > > Therefore I am particularly interested to find out if this is really > needed or it's just playing safe? You could call it playing safe. We haven't hit any case where it was set to zero but per mmc_switch() description you are allowed to set it to zero to let the host decide what it wants to use. >> + timeout = 100000000U; >> + >> + set_data_timeout(host, timeout, 0); >> + } >> return 0; >> } >> >> -- >> 1.9.1 >> > Kind regards > Uffe Regards, RK -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] >>> + /* >>> + * Set an arbitrary 100ms data timeout for >>> commands with >>> + * busy signal and no indication of busy_timeout. >>> + */ >>> + if (!timeout) >> >> This is a bug in the mmc core if this ever happen. >> >> Therefore I am particularly interested to find out if this is really >> needed or it's just playing safe? > > You could call it playing safe. > We haven't hit any case where it was set to zero but per mmc_switch() > description you are allowed to set it to zero to let the host decide what it > wants to use. I check the code in the core. Apparently there are some cases when INAND_CMD38_ARG* is used, but also some cases where I think the timeout value becomes picked from the EXT_CSD without validating its value. Let's keep $subject patch as is, then allow me to submit a few changes for core to deal with this properly. > >>> + timeout = 100000000U; >>> + >>> + set_data_timeout(host, timeout, 0); >>> + } >>> return 0; >>> } Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c index 0ee5650..51ca3d7 100644 --- a/drivers/mmc/host/omap_hsmmc.c +++ b/drivers/mmc/host/omap_hsmmc.c @@ -1527,16 +1527,24 @@ static void omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host) omap_hsmmc_prepare_data(struct omap_hsmmc_host *host, struct mmc_request *req) { int ret; + unsigned int timeout; + host->data = req->data; if (req->data == NULL) { OMAP_HSMMC_WRITE(host->base, BLK, 0); - /* - * Set an arbitrary 100ms data timeout for commands with - * busy signal. - */ - if (req->cmd->flags & MMC_RSP_BUSY) - set_data_timeout(host, 100000000U, 0); + if (req->cmd->flags & MMC_RSP_BUSY) { + timeout = req->cmd->busy_timeout * NSEC_PER_MSEC; + + /* + * Set an arbitrary 100ms data timeout for commands with + * busy signal and no indication of busy_timeout. + */ + if (!timeout) + timeout = 100000000U; + + set_data_timeout(host, timeout, 0); + } return 0; }