RE: [PATCH] mmc: core: improve rationality of bus width setting for HS400es

["Fang Hongjie(方洪杰)" <hongjiefang@asrmicro.com>]

> On 10 July 2018 at 07:04, Fang Hongjie(方洪杰) <hongjiefang@asrmicro.com>
> wrote:
> >> On 9 July 2018 at 08:47, Hongjie Fang <hongjiefang@asrmicro.com> wrote:
> >> > mmc_select_hs400es() calls mmc_select_bus_width() which will try to
> >> > set 4bit transfer mode if fail to set 8bit mode. The problem is that
> >> > the bus width should not be set to 4bit in HS400es mode.
> >>
> >> I guess it fails because there is something wrong. Can you please
> >> elaborate under what circumstance the problem occurs?
> >>
> > This problem very occasionally occurred when system resume from
> > suspend state. The card stuck in the programming state after setting
> > 8bit mode. After that, mmc_select_bus_width() will continue to set
> > 4bit mode and return without errors. Of course, the following R/W
> > requests will be all failing.
> 
> I see. So it's the mmc_switch() operation that fails, when trying 8-bit. Hmm.
> 
> >
> > The problem scene is hard to reproduce. Maybe it happened
> > because there is something wrong about bus clock or core voltage
> > stability when system do resume.
> 
> Yeah, that needs further debugging.
> 
> Don't know what host driver you are using, but potentially you could
> play with a couple host specific mechanism. The ->host->card_busy()
> callback, host->max_busy_timeout and MMC_CAP_WAIT_WHILE_BUSY.
> Depending on these, __mmc_switch() changes its policy when polling for
> busy. Have a look at __mmc_switch() and mmc_poll_for_busy(), to see
> what goes on.
> 
Thanks.
I will do the further debugging according to your advice.

> >
> >> >
> >> > Signed-off-by: Hongjie Fang <hongjiefang@asrmicro.com>
> >> > ---
> >> >  drivers/mmc/core/mmc.c | 23 +++++++++++++++++++++--
> >> >  1 file changed, 21 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> >> > index 4466f5d..94c3cc5 100644
> >> > --- a/drivers/mmc/core/mmc.c
> >> > +++ b/drivers/mmc/core/mmc.c
> >> > @@ -1337,9 +1337,28 @@ static int mmc_select_hs400es(struct mmc_card
> *card)
> >> >         if (err)
> >> >                 goto out_err;
> >> >
> >> > -       err = mmc_select_bus_width(card);
> >> > -       if (err < 0)
> >> > +       /* Switch card to 8 bit mode */
> >> > +       err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> >> > +                        EXT_CSD_BUS_WIDTH,
> >> > +                        EXT_CSD_BUS_WIDTH_8,
> >> > +                        card->ext_csd.generic_cmd6_time);
> >> > +       if (err) {
> >> > +               pr_err("%s: switch to 8 bit mode for hs400es failed, err:%d\n",
> >> > +                       mmc_hostname(host), err);
> >> > +               goto out_err;
> >> > +       }
> >> > +
> >> > +       mmc_set_bus_width(host, MMC_BUS_WIDTH_8);
> >> > +
> >> > +       if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
> >> > +               err = mmc_compare_ext_csds(card, MMC_BUS_WIDTH_8);
> >> > +       else
> >> > +               err = mmc_bus_test(card, MMC_BUS_WIDTH_8);
> >> > +       if (err) {
> >> > +               pr_err("%s: test 8 bit mode for hs400es failed, err:%d\n",
> >> > +                       mmc_hostname(host), err);
> >> >                 goto out_err;
> >>
> >> Lot's open coding, no I don't like it.
> >>
> >> Wouldn't it instead be possible to check the return value from
> >> mmc_select_bus_width(), as it should tell what width it manage to
> >> select. Then if it isn't 8-bit, then we should bail out. Does that
> >> work?
> >>
> > After setting 4bit mode, it can't work because it cannot back to
> > HS200 mode like the HS400 mode.
> > For HS400es, if fail to set 8bit mode, maybe it is more reasonable
> > that return error directly.
> 
> Yeah. Care to send a patch for that?
OK. I will try to send a new patch that let mmc_select_bus_width()
return error directly when fail to set 8bit for HS400es.

> 
> [...]
> 
> Kind regards
> Uffe

B&R
Hongjie