Re: [PATCH 2/3] mmc: host: omap_hsmmc: use generic_cmd6_time to program timeout value for CMD6

[Ravikumar]

On Tuesday 31 January 2017 03:42 PM, Ulf Hansson wrote:
> On 30 January 2017 at 11:11, Ravikumar Kattekola <rk@xxxxxx> wrote:
> > From: Kishon Vijay Abraham I <kishon@xxxxxx>
> >
> > 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@xxxxxx>
> > Signed-off-by: Sekhar Nori <nsekhar@xxxxxx>
> > Signed-off-by: Ravikumar Kattekola <rk@xxxxxx>
> > ---
> >   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