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

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

[Hongjie Fang]

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.

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;
+	}
 
 	/* Switch card to HS mode */
 	err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
-- 
1.9.1

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

[Ulf Hansson]

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?

>
> 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?

> +       }
>
>         /* Switch card to HS mode */
>         err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> --
> 1.9.1
>

Kind regards
Uffe

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

[Fang Hongjie(方洪杰)]

> 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.

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.

> >
> > 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.

> > +       }
> >
> >         /* Switch card to HS mode */
> >         err = __mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> > --
> > 1.9.1
> >
> 
> Kind regards
> Uffe


B&R
Hongjie

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

[Ulf Hansson]

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.

>
>> >
>> > 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?

[...]

Kind regards
Uffe

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

[Fang Hongjie(方洪杰)]

> 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