Fix sdio reinit card fail issue

Fix sdio reinit card fail issue

[Yong Mao]

Some UHS sdio devices can't enter to stable state after changing
the voltage from 1.8v to 3.3v in re-initialize flow. Need do some
error handle to recovery SDIO device.

[PATCH 1/3] mmc: core: need do mmc_power_cycle in mmc_sdio_resend_if_cond

[Yong Mao]

From: yong mao <yong.mao@mediatek.com>

When mmc_sdio_resned_if_cond is invoked, it indicates the SDIO
device is not in the right state. In this condition, the previous
implementation of mmc_sdio_resend_if_cond can't make sure SDIO
device be back to idle state. mmc_power_cycle can reset the SDIO
device by HW and also make sure SDIO device enter to idle state
correctly.

Signed-off-by: Yong Mao <yong.mao@mediatek.com>
---
 drivers/mmc/core/sdio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index ebb387a..ada0a80 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -546,6 +546,7 @@ static int mmc_sdio_init_uhs_card(struct mmc_card *card)
 static void mmc_sdio_resend_if_cond(struct mmc_host *host,
 				    struct mmc_card *card)
 {
+	mmc_power_cycle(host, host->card->ocr);
 	sdio_reset(host);
 	mmc_go_idle(host);
 	mmc_send_if_cond(host, host->ocr_avail);
-- 
1.9.1

[PATCH 2/3] mmc: core: rocr verification

[Yong Mao]

From: yong mao <yong.mao@mediatek.com>

Some UHS SDIO devices can't enter to stable state after changing the
voltage from 1.8v to 3.3v even after a power cycle.
Verifying the rocr and the result of mmc_set_signal_voltage, if it
is not expected, power cycle SDIO device and re-initialize it again.
Thus will re-initialize the SDIO device successfully.

Signed-off-by: Yong Mao <yong.mao@mediatek.com>
---
 drivers/mmc/core/sdio.c  | 14 +++++++++++++-
 include/linux/mmc/sdio.h |  2 ++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index ada0a80..f173cad 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -553,6 +553,12 @@ static void mmc_sdio_resend_if_cond(struct mmc_host *host,
 	mmc_remove_card(card);
 }
 
+static bool mmc_sdio_valid_rocr(u32 rocr)
+{
+	return (rocr & MMC_CARD_BUSY) && R4_OCR(rocr) &&
+		R4_FUNCTION_NUMBER(rocr);
+}
+
 /*
  * Handle the detection and initialisation of a card.
  *
@@ -605,6 +611,12 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 		goto err;
 	}
 
+	if (!mmc_sdio_valid_rocr(rocr)) {
+		mmc_sdio_resend_if_cond(host, card);
+		retries--;
+		goto try_again;
+	}
+
 	if ((rocr & R4_MEMORY_PRESENT) &&
 	    mmc_sd_get_cid(host, ocr & rocr, card->raw_cid, NULL) == 0) {
 		card->type = MMC_TYPE_SD_COMBO;
@@ -646,7 +658,7 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 	 */
 	if (rocr & ocr & R4_18V_PRESENT) {
 		err = mmc_set_uhs_voltage(host, ocr_card);
-		if (err == -EAGAIN) {
+		if (err == -EAGAIN || err == -EILSEQ) {
 			mmc_sdio_resend_if_cond(host, card);
 			retries--;
 			goto try_again;
diff --git a/include/linux/mmc/sdio.h b/include/linux/mmc/sdio.h
index e287699..03e23ec 100644
--- a/include/linux/mmc/sdio.h
+++ b/include/linux/mmc/sdio.h
@@ -36,6 +36,8 @@
 
 #define R4_18V_PRESENT (1<<24)
 #define R4_MEMORY_PRESENT (1 << 27)
+#define R4_OCR(x)		((x) & 0xFFFFFF)
+#define R4_FUNCTION_NUMBER(x)	(((x) & 0x70000000) >> 28)
 
 /*
   SDIO status in R5
-- 
1.9.1

[PATCH 3/3] mmc: core: fix mmc_sdio_reinit_card fail issue

[Yong Mao]

From: yong mao <yong.mao@mediatek.com>

If SDIO device is initialized by UHS mode, it will run with 1.8v power.
In this mode, mmc_go_idle may not make SDIO device go idle successfully
in some special SDIO device. And then it can't be re-initialized
successfully.
According to the logic in sdio_reset_comm and mmc_sdio_sw_reset,
invoking mmc_set_clock(host, host->f_min) before mmc_send_io_op_cond
can make this SDIO device back to right state.

Signed-off-by: Yong Mao <yong.mao@mediatek.com>
---
 drivers/mmc/core/sdio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index f173cad..dc4dc63 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -850,6 +850,7 @@ static int mmc_sdio_reinit_card(struct mmc_host *host)
 
 	sdio_reset(host);
 	mmc_go_idle(host);
+	mmc_set_clock(host, host->f_min);
 	mmc_send_if_cond(host, host->card->ocr);
 
 	ret = mmc_send_io_op_cond(host, 0, NULL);
-- 
1.9.1

Re: [PATCH 1/3] mmc: core: need do mmc_power_cycle in mmc_sdio_resend_if_cond

[Matthias Kaehlcke]

Hi,

On Tue, Apr 14, 2020 at 11:40:09AM +0800, Yong Mao wrote:
> From: yong mao <yong.mao@mediatek.com>
> 
> When mmc_sdio_resned_if_cond is invoked, it indicates the SDIO
> device is not in the right state. In this condition, the previous
> implementation of mmc_sdio_resend_if_cond can't make sure SDIO
> device be back to idle state. mmc_power_cycle can reset the SDIO
> device by HW and also make sure SDIO device enter to idle state
> correctly.
> 
> Signed-off-by: Yong Mao <yong.mao@mediatek.com>
> ---
>  drivers/mmc/core/sdio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index ebb387a..ada0a80 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -546,6 +546,7 @@ static int mmc_sdio_init_uhs_card(struct mmc_card *card)
>  static void mmc_sdio_resend_if_cond(struct mmc_host *host,
>  				    struct mmc_card *card)
>  {
> +	mmc_power_cycle(host, host->card->ocr);

My MMC/SDIO background is limited, but it seems this isn't needed for the
vast majority of SDIO devices, otherwise it probably would have been added
earlier. I wonder if it would make sense to make the power cycle
conditional through a quirk, to limit it to the devices that need it.

>  	sdio_reset(host);
>  	mmc_go_idle(host);
>  	mmc_send_if_cond(host, host->ocr_avail);
> -- 
> 1.9.1

Re: [PATCH 1/3] mmc: core: need do mmc_power_cycle in mmc_sdio_resend_if_cond

[yong.mao]

On Mon, 2020-04-20 at 12:15 -0700, Matthias Kaehlcke wrote:
> Hi,
> 
> On Tue, Apr 14, 2020 at 11:40:09AM +0800, Yong Mao wrote:
> > From: yong mao <yong.mao@mediatek.com>
> > 
> > When mmc_sdio_resned_if_cond is invoked, it indicates the SDIO
> > device is not in the right state. In this condition, the previous
> > implementation of mmc_sdio_resend_if_cond can't make sure SDIO
> > device be back to idle state. mmc_power_cycle can reset the SDIO
> > device by HW and also make sure SDIO device enter to idle state
> > correctly.
> > 
> > Signed-off-by: Yong Mao <yong.mao@mediatek.com>
> > ---
> >  drivers/mmc/core/sdio.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> > index ebb387a..ada0a80 100644
> > --- a/drivers/mmc/core/sdio.c
> > +++ b/drivers/mmc/core/sdio.c
> > @@ -546,6 +546,7 @@ static int mmc_sdio_init_uhs_card(struct mmc_card *card)
> >  static void mmc_sdio_resend_if_cond(struct mmc_host *host,
> >  				    struct mmc_card *card)
> >  {
> > +	mmc_power_cycle(host, host->card->ocr);
> 
> My MMC/SDIO background is limited, but it seems this isn't needed for the
> vast majority of SDIO devices, otherwise it probably would have been added
> earlier. I wonder if it would make sense to make the power cycle
> conditional through a quirk, to limit it to the devices that need it.
> 
	
Thanks for your comment.
mmc_sdio_resend_if_cond API is not for normal initialization flow, but
for error handle flow. If mmc_sdio_resend_if_cond is invoked, it
indicates there is something wrong with the SDIO device. HW power cycle
is the basic guarantee for device to back to idle state.
Therefore this patch will not affect the normal initialization for the 
vast majority of SDIO devices, but it is very helpful for error cases
for all SDIO devices.
In my opinion, we don't need that quirk.
Could Ulf help to give some advises on this?
Thanks.

> >  	sdio_reset(host);
> >  	mmc_go_idle(host);
> >  	mmc_send_if_cond(host, host->ocr_avail);
> > -- 
> > 1.9.1

Re: [PATCH 3/3] mmc: core: fix mmc_sdio_reinit_card fail issue

[Matthias Kaehlcke]

Hi Yong,

On Tue, Apr 14, 2020 at 11:40:11AM +0800, Yong Mao wrote:
> From: yong mao <yong.mao@mediatek.com>
> 
> If SDIO device is initialized by UHS mode, it will run with 1.8v power.
> In this mode, mmc_go_idle may not make SDIO device go idle successfully
> in some special SDIO device. And then it can't be re-initialized
> successfully.
> According to the logic in sdio_reset_comm and mmc_sdio_sw_reset,
> invoking mmc_set_clock(host, host->f_min) before mmc_send_io_op_cond
> can make this SDIO device back to right state.
>

The commit message isn't very concise. Suggestion for a better
structure:

mmc: core: reset clock to minimum speed during card reinit

Some buggy (?) SDIO devices don't (consistently?) enter idle mode
through mmc_go_idle() when running in UHS mode. [add rationale why
setting the clock to minimum speed fixes this]


Also the function sdio_reset_comm() mentioned in the commit message
doesn't exist in recent kernels. And mmc_sdio_sw_reset() does not invoke
mmc_send_io_op_cond(), as the commit message appears to claim.

> Signed-off-by: Yong Mao <yong.mao@mediatek.com>
> ---
>  drivers/mmc/core/sdio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index f173cad..dc4dc63 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -850,6 +850,7 @@ static int mmc_sdio_reinit_card(struct mmc_host *host)
>  
>  	sdio_reset(host);
>  	mmc_go_idle(host);
> +	mmc_set_clock(host, host->f_min);

mmc_sdio_sw_reset() - which is mentioned as reference in the commit
message - sets the clock speed before sdio_reset(). Should this order
be followed here too?

Re: [PATCH 1/3] mmc: core: need do mmc_power_cycle in mmc_sdio_resend_if_cond

[Ulf Hansson]

On Tue, 14 Apr 2020 at 05:40, Yong Mao <yong.mao@mediatek.com> wrote:
>
> From: yong mao <yong.mao@mediatek.com>
>
> When mmc_sdio_resned_if_cond is invoked, it indicates the SDIO
> device is not in the right state. In this condition, the previous
> implementation of mmc_sdio_resend_if_cond can't make sure SDIO
> device be back to idle state. mmc_power_cycle can reset the SDIO
> device by HW and also make sure SDIO device enter to idle state
> correctly.
>
> Signed-off-by: Yong Mao <yong.mao@mediatek.com>
> ---
>  drivers/mmc/core/sdio.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index ebb387a..ada0a80 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -546,6 +546,7 @@ static int mmc_sdio_init_uhs_card(struct mmc_card *card)
>  static void mmc_sdio_resend_if_cond(struct mmc_host *host,
>                                     struct mmc_card *card)
>  {
> +       mmc_power_cycle(host, host->card->ocr);

This looks wrong to me. mmc_sdio_resend_if_cond() is called from two places.

1. In the case when mmc_set_uhs_voltage() fails in
mmc_sdio_init_card(), which means a call to mmc_power_cycle() has
already been done.

2. Wen sdio_read_cccr() fails and when we decide to retry the UHS-I
voltage switch. Then perhaps it could make sense to run a power cycle.
But if so, we better do it only for that path.

I will continue to look a bit, as I think there are really more issues
that we may want to look into while looking at this piece of code.
However, allow me some more time before I can provide some more ideas
of how to move forward.

>         sdio_reset(host);
>         mmc_go_idle(host);
>         mmc_send_if_cond(host, host->ocr_avail);
> --
> 1.9.1

Kind regards
Uffe

Re: [PATCH 1/3] mmc: core: need do mmc_power_cycle in mmc_sdio_resend_if_cond

[yong.mao]

On Fri, 2020-04-24 at 12:09 +0200, Ulf Hansson wrote:
> On Tue, 14 Apr 2020 at 05:40, Yong Mao <yong.mao@mediatek.com> wrote:
> >
> > From: yong mao <yong.mao@mediatek.com>
> >
> > When mmc_sdio_resned_if_cond is invoked, it indicates the SDIO
> > device is not in the right state. In this condition, the previous
> > implementation of mmc_sdio_resend_if_cond can't make sure SDIO
> > device be back to idle state. mmc_power_cycle can reset the SDIO
> > device by HW and also make sure SDIO device enter to idle state
> > correctly.
> >
> > Signed-off-by: Yong Mao <yong.mao@mediatek.com>
> > ---
> >  drivers/mmc/core/sdio.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> > index ebb387a..ada0a80 100644
> > --- a/drivers/mmc/core/sdio.c
> > +++ b/drivers/mmc/core/sdio.c
> > @@ -546,6 +546,7 @@ static int mmc_sdio_init_uhs_card(struct mmc_card *card)
> >  static void mmc_sdio_resend_if_cond(struct mmc_host *host,
> >                                     struct mmc_card *card)
> >  {
> > +       mmc_power_cycle(host, host->card->ocr);
> 
> This looks wrong to me. mmc_sdio_resend_if_cond() is called from two places.
> 
> 1. In the case when mmc_set_uhs_voltage() fails in
> mmc_sdio_init_card(), which means a call to mmc_power_cycle() has
> already been done.
> 
  Thanks for your comment.
  Yes. It is right that mmc_power_cycle() has already been done when
  mmc_sdio_resend_if_cond() is called. In normal re-initialization case, 
  this mmc_power_cycle() (currently in 1.8v voltage and 208Mhz clock) 
  can make SDIO device really back to idle state. Unfortunately, in some
  special SDIO device, it will enter to unstable state.

  At this unstable state, device may keep data0 always low after receiving CMD11.
  And then every other SDIO CMD can't be sent to device any more due to card 
  is busy(data0 is low). Therefore, previous implementation can't save the 
  device. At this time, mmc_power_cycle() may be the final solution to make 
  sure SDIO device can back to idle state correctly.

> 2. Wen sdio_read_cccr() fails and when we decide to retry the UHS-I
> voltage switch. Then perhaps it could make sense to run a power cycle.
> But if so, we better do it only for that path.
> 
> I will continue to look a bit, as I think there are really more issues
> that we may want to look into while looking at this piece of code.
> However, allow me some more time before I can provide some more ideas
> of how to move forward.
  In the actual project, we do encounter many relative issues about re-initialized card.
  The following two categories are the most common issue we met before.
  A. the SDIO card is initialized by UHS-I mode at the first time, but will be 
     re-initialized by High Speed mode at the second time.
     ==> All this type of issues is relative with S18A in response of CMD5.
	 And most of the issues are related to the interval between powering off and 
         powering on card.
  B. If there is something wrong in the flow of voltage switch(after CMD11), card will
     always keep all data pins to low. And then it hangs up because data0 is always low.
  Hope this information will be helpful for you.
  
  Anyway, we will wait for your advises. 
> 
> >         sdio_reset(host);
> >         mmc_go_idle(host);
> >         mmc_send_if_cond(host, host->ocr_avail);
> > --
> > 1.9.1
> 
> Kind regards
> Uffe

Re: [PATCH 1/3] mmc: core: need do mmc_power_cycle in mmc_sdio_resend_if_cond

[Ulf Hansson]

On Tue, 28 Apr 2020 at 11:28, yong.mao@mediatek.com
<yong.mao@mediatek.com> wrote:
>
>
> On Fri, 2020-04-24 at 12:09 +0200, Ulf Hansson wrote:
> > On Tue, 14 Apr 2020 at 05:40, Yong Mao <yong.mao@mediatek.com> wrote:
> > >
> > > From: yong mao <yong.mao@mediatek.com>
> > >
> > > When mmc_sdio_resned_if_cond is invoked, it indicates the SDIO
> > > device is not in the right state. In this condition, the previous
> > > implementation of mmc_sdio_resend_if_cond can't make sure SDIO
> > > device be back to idle state. mmc_power_cycle can reset the SDIO
> > > device by HW and also make sure SDIO device enter to idle state
> > > correctly.
> > >
> > > Signed-off-by: Yong Mao <yong.mao@mediatek.com>
> > > ---
> > >  drivers/mmc/core/sdio.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> > > index ebb387a..ada0a80 100644
> > > --- a/drivers/mmc/core/sdio.c
> > > +++ b/drivers/mmc/core/sdio.c
> > > @@ -546,6 +546,7 @@ static int mmc_sdio_init_uhs_card(struct mmc_card *card)
> > >  static void mmc_sdio_resend_if_cond(struct mmc_host *host,
> > >                                     struct mmc_card *card)
> > >  {
> > > +       mmc_power_cycle(host, host->card->ocr);
> >
> > This looks wrong to me. mmc_sdio_resend_if_cond() is called from two places.
> >
> > 1. In the case when mmc_set_uhs_voltage() fails in
> > mmc_sdio_init_card(), which means a call to mmc_power_cycle() has
> > already been done.
> >
>   Thanks for your comment.
>   Yes. It is right that mmc_power_cycle() has already been done when
>   mmc_sdio_resend_if_cond() is called. In normal re-initialization case,
>   this mmc_power_cycle() (currently in 1.8v voltage and 208Mhz clock)
>   can make SDIO device really back to idle state. Unfortunately, in some
>   special SDIO device, it will enter to unstable state.
>
>   At this unstable state, device may keep data0 always low after receiving CMD11.
>   And then every other SDIO CMD can't be sent to device any more due to card
>   is busy(data0 is low). Therefore, previous implementation can't save the
>   device. At this time, mmc_power_cycle() may be the final solution to make
>   sure SDIO device can back to idle state correctly.

Well, this still sounds a bit vague to me. I need to understand more
exactly under what circumstances the problem occurs.

What platform are you testing with and what SDIO card is being used?

Is the problem happening during the system resume phase?

Are the SDIO func driver using runtime PM and then is the host capable
of MMC_CAP_POWER_OFF_CARD?

Is it easy to reproduce the problem for you?

>
> > 2. Wen sdio_read_cccr() fails and when we decide to retry the UHS-I
> > voltage switch. Then perhaps it could make sense to run a power cycle.
> > But if so, we better do it only for that path.
> >
> > I will continue to look a bit, as I think there are really more issues
> > that we may want to look into while looking at this piece of code.
> > However, allow me some more time before I can provide some more ideas
> > of how to move forward.
>   In the actual project, we do encounter many relative issues about re-initialized card.
>   The following two categories are the most common issue we met before.
>   A. the SDIO card is initialized by UHS-I mode at the first time, but will be
>      re-initialized by High Speed mode at the second time.
>      ==> All this type of issues is relative with S18A in response of CMD5.
>          And most of the issues are related to the interval between powering off and
>          powering on card.
>   B. If there is something wrong in the flow of voltage switch(after CMD11), card will
>      always keep all data pins to low. And then it hangs up because data0 is always low.
>   Hope this information will be helpful for you.

Thanks for sharing these details! I think we need to continue to debug
this issue, to fully understand.

In principle, it sounds to me that maybe mmc_power_cycle(), isn't
really successfully power-cycling the SDIO card. Perhaps insert a few
delays or so in that code to see how that would affect things?

Anyway, how is the power to the SDIO card controlled in this case? Are
you using a mmc-pwrseq?

>
>   Anyway, we will wait for your advises.
> >
> > >         sdio_reset(host);
> > >         mmc_go_idle(host);
> > >         mmc_send_if_cond(host, host->ocr_avail);
> > > --
> > > 1.9.1
> >
> > Kind regards
> > Uffe

I have a few patches in the pipe, which fixes some other problems in
mmc_sdio_init_card(). Possibly those can be related, but I need a day
or so to post them, let's see.

Kind regards
Uffe

Re: [PATCH 1/3] mmc: core: need do mmc_power_cycle in mmc_sdio_resend_if_cond

[yong.mao]

On Tue, 2020-04-28 at 14:13 +0200, Ulf Hansson wrote:
> On Tue, 28 Apr 2020 at 11:28, yong.mao@mediatek.com
> <yong.mao@mediatek.com> wrote:
> >
> >
> > On Fri, 2020-04-24 at 12:09 +0200, Ulf Hansson wrote:
> > > On Tue, 14 Apr 2020 at 05:40, Yong Mao <yong.mao@mediatek.com> wrote:
> > > >
> > > > From: yong mao <yong.mao@mediatek.com>
> > > >
> > > > When mmc_sdio_resned_if_cond is invoked, it indicates the SDIO
> > > > device is not in the right state. In this condition, the previous
> > > > implementation of mmc_sdio_resend_if_cond can't make sure SDIO
> > > > device be back to idle state. mmc_power_cycle can reset the SDIO
> > > > device by HW and also make sure SDIO device enter to idle state
> > > > correctly.
> > > >
> > > > Signed-off-by: Yong Mao <yong.mao@mediatek.com>
> > > > ---
> > > >  drivers/mmc/core/sdio.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> > > > index ebb387a..ada0a80 100644
> > > > --- a/drivers/mmc/core/sdio.c
> > > > +++ b/drivers/mmc/core/sdio.c
> > > > @@ -546,6 +546,7 @@ static int mmc_sdio_init_uhs_card(struct mmc_card *card)
> > > >  static void mmc_sdio_resend_if_cond(struct mmc_host *host,
> > > >                                     struct mmc_card *card)
> > > >  {
> > > > +       mmc_power_cycle(host, host->card->ocr);
> > >
> > > This looks wrong to me. mmc_sdio_resend_if_cond() is called from two places.
> > >
> > > 1. In the case when mmc_set_uhs_voltage() fails in
> > > mmc_sdio_init_card(), which means a call to mmc_power_cycle() has
> > > already been done.
> > >
> >   Thanks for your comment.
> >   Yes. It is right that mmc_power_cycle() has already been done when
> >   mmc_sdio_resend_if_cond() is called. In normal re-initialization case,
> >   this mmc_power_cycle() (currently in 1.8v voltage and 208Mhz clock)
> >   can make SDIO device really back to idle state. Unfortunately, in some
> >   special SDIO device, it will enter to unstable state.
> >
> >   At this unstable state, device may keep data0 always low after receiving CMD11.
> >   And then every other SDIO CMD can't be sent to device any more due to card
> >   is busy(data0 is low). Therefore, previous implementation can't save the
> >   device. At this time, mmc_power_cycle() may be the final solution to make
> >   sure SDIO device can back to idle state correctly.
> 
> Well, this still sounds a bit vague to me. I need to understand more
> exactly under what circumstances the problem occurs.
> 
> What platform are you testing with and what SDIO card is being used?
 The platform information is mt8173 + Marvell sdio device + kernel-3.18

> 
> Is the problem happening during the system resume phase?
  The problem happen when mmc_sdio_runtime_resume is invoked.
> 
> Are the SDIO func driver using runtime PM and then is the host capable
> of MMC_CAP_POWER_OFF_CARD?
> 
  Yes. SDIO func driver uses runtime PM and MMC_CAP_POWER_OFF_CARD is
enabled.

> Is it easy to reproduce the problem for you?
> 
 There are only two units out of many produced units that can always
reproduce this issue.
 
> >
> > > 2. Wen sdio_read_cccr() fails and when we decide to retry the UHS-I
> > > voltage switch. Then perhaps it could make sense to run a power cycle.
> > > But if so, we better do it only for that path.
> > >
> > > I will continue to look a bit, as I think there are really more issues
> > > that we may want to look into while looking at this piece of code.
> > > However, allow me some more time before I can provide some more ideas
> > > of how to move forward.
> >   In the actual project, we do encounter many relative issues about re-initialized card.
> >   The following two categories are the most common issue we met before.
> >   A. the SDIO card is initialized by UHS-I mode at the first time, but will be
> >      re-initialized by High Speed mode at the second time.
> >      ==> All this type of issues is relative with S18A in response of CMD5.
> >          And most of the issues are related to the interval between powering off and
> >          powering on card.
> >   B. If there is something wrong in the flow of voltage switch(after CMD11), card will
> >      always keep all data pins to low. And then it hangs up because data0 is always low.
> >   Hope this information will be helpful for you.
> 
> Thanks for sharing these details! I think we need to continue to debug
> this issue, to fully understand.
> 
> In principle, it sounds to me that maybe mmc_power_cycle(), isn't
> really successfully power-cycling the SDIO card. Perhaps insert a few
> delays or so in that code to see how that would affect things?
> 
> Anyway, how is the power to the SDIO card controlled in this case? Are
> you using a mmc-pwrseq?
> 
  vmmc is controlled through GPIO to supply 3.3v power.
  And the vqmmc is supplied from PMIC which is always 1.8v.
  (There is no 3.3v here. Perhaps this is one of the reasons to happen
this issues)

> >
> >   Anyway, we will wait for your advises.
> > >
> > > >         sdio_reset(host);
> > > >         mmc_go_idle(host);
> > > >         mmc_send_if_cond(host, host->ocr_avail);
> > > > --
> > > > 1.9.1
> > >
> > > Kind regards
> > > Uffe
> 
> I have a few patches in the pipe, which fixes some other problems in
> mmc_sdio_init_card(). Possibly those can be related, but I need a day
> or so to post them, let's see.
The codebase of this project is kernel-3.18. Maybe it is hard to apply 
these new patches. Anyway, We will try it when we get the patches.
Thanks. 


> 
> Kind regards
> Uffe

Re: [PATCH 1/3] mmc: core: need do mmc_power_cycle in mmc_sdio_resend_if_cond

[Ulf Hansson]

On Wed, 29 Apr 2020 at 10:21, yong.mao@mediatek.com
<yong.mao@mediatek.com> wrote:
>
> On Tue, 2020-04-28 at 14:13 +0200, Ulf Hansson wrote:
> > On Tue, 28 Apr 2020 at 11:28, yong.mao@mediatek.com
> > <yong.mao@mediatek.com> wrote:
> > >
> > >
> > > On Fri, 2020-04-24 at 12:09 +0200, Ulf Hansson wrote:
> > > > On Tue, 14 Apr 2020 at 05:40, Yong Mao <yong.mao@mediatek.com> wrote:
> > > > >
> > > > > From: yong mao <yong.mao@mediatek.com>
> > > > >
> > > > > When mmc_sdio_resned_if_cond is invoked, it indicates the SDIO
> > > > > device is not in the right state. In this condition, the previous
> > > > > implementation of mmc_sdio_resend_if_cond can't make sure SDIO
> > > > > device be back to idle state. mmc_power_cycle can reset the SDIO
> > > > > device by HW and also make sure SDIO device enter to idle state
> > > > > correctly.
> > > > >
> > > > > Signed-off-by: Yong Mao <yong.mao@mediatek.com>
> > > > > ---
> > > > >  drivers/mmc/core/sdio.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> > > > > index ebb387a..ada0a80 100644
> > > > > --- a/drivers/mmc/core/sdio.c
> > > > > +++ b/drivers/mmc/core/sdio.c
> > > > > @@ -546,6 +546,7 @@ static int mmc_sdio_init_uhs_card(struct mmc_card *card)
> > > > >  static void mmc_sdio_resend_if_cond(struct mmc_host *host,
> > > > >                                     struct mmc_card *card)
> > > > >  {
> > > > > +       mmc_power_cycle(host, host->card->ocr);
> > > >
> > > > This looks wrong to me. mmc_sdio_resend_if_cond() is called from two places.
> > > >
> > > > 1. In the case when mmc_set_uhs_voltage() fails in
> > > > mmc_sdio_init_card(), which means a call to mmc_power_cycle() has
> > > > already been done.
> > > >
> > >   Thanks for your comment.
> > >   Yes. It is right that mmc_power_cycle() has already been done when
> > >   mmc_sdio_resend_if_cond() is called. In normal re-initialization case,
> > >   this mmc_power_cycle() (currently in 1.8v voltage and 208Mhz clock)
> > >   can make SDIO device really back to idle state. Unfortunately, in some
> > >   special SDIO device, it will enter to unstable state.
> > >
> > >   At this unstable state, device may keep data0 always low after receiving CMD11.
> > >   And then every other SDIO CMD can't be sent to device any more due to card
> > >   is busy(data0 is low). Therefore, previous implementation can't save the
> > >   device. At this time, mmc_power_cycle() may be the final solution to make
> > >   sure SDIO device can back to idle state correctly.
> >
> > Well, this still sounds a bit vague to me. I need to understand more
> > exactly under what circumstances the problem occurs.
> >
> > What platform are you testing with and what SDIO card is being used?
>  The platform information is mt8173 + Marvell sdio device + kernel-3.18

I see, thanks for sharing this information. Forward/backporting
against 3.18 is hard, perhaps impossible when it comes to this, sorry.

A lot of SDIO core parts, especially related to re-initialization and
power management have been changed since v3.18.

"git log --oneline v3.18..v5.7-rc4 drivers/mmc/core/sdio*" will tell you.

Would it be possible to move to a later kernel and test instead? I
mean, the problem may already have been solved!? mt8173 should be
rather well supported upstream, but perhaps lots are missing for the
SDIO parts?

>
> >
> > Is the problem happening during the system resume phase?
>   The problem happen when mmc_sdio_runtime_resume is invoked.
> >
> > Are the SDIO func driver using runtime PM and then is the host capable
> > of MMC_CAP_POWER_OFF_CARD?
> >
>   Yes. SDIO func driver uses runtime PM and MMC_CAP_POWER_OFF_CARD is
> enabled.

Alright, that explains the use case, thanks!

>
> > Is it easy to reproduce the problem for you?
> >
>  There are only two units out of many produced units that can always
> reproduce this issue.

An idea to possibly help to narrow down the problem, could be to
implement an "test SDIO func driver" and use that rather than the
mwifiex driver (which I assume is the one you are using?). Then we
could run various tests from it, like calling pm_runtime_get|put() for
example.

We already have a similar thing to replace the mmc/sd block device
driver, so this could be useful for testing SDIO cards/funcs I think.

>
> > >
> > > > 2. Wen sdio_read_cccr() fails and when we decide to retry the UHS-I
> > > > voltage switch. Then perhaps it could make sense to run a power cycle.
> > > > But if so, we better do it only for that path.
> > > >
> > > > I will continue to look a bit, as I think there are really more issues
> > > > that we may want to look into while looking at this piece of code.
> > > > However, allow me some more time before I can provide some more ideas
> > > > of how to move forward.
> > >   In the actual project, we do encounter many relative issues about re-initialized card.
> > >   The following two categories are the most common issue we met before.
> > >   A. the SDIO card is initialized by UHS-I mode at the first time, but will be
> > >      re-initialized by High Speed mode at the second time.
> > >      ==> All this type of issues is relative with S18A in response of CMD5.
> > >          And most of the issues are related to the interval between powering off and
> > >          powering on card.

This sounds a bit like the card gets re-initialized without it first
being properly power cycled.

Perhaps you call mmc_sw_reset() for a "test SDIO func driver", which
re-initializes the card, but without doing a power cycle. Then that
should give you the similar problem?

> > >   B. If there is something wrong in the flow of voltage switch(after CMD11), card will
> > >      always keep all data pins to low. And then it hangs up because data0 is always low.
> > >   Hope this information will be helpful for you.

I keep repeating myself, but there seems to be a problem with the
power cycling of the SDIO card.

> >
> > Thanks for sharing these details! I think we need to continue to debug
> > this issue, to fully understand.
> >
> > In principle, it sounds to me that maybe mmc_power_cycle(), isn't
> > really successfully power-cycling the SDIO card. Perhaps insert a few
> > delays or so in that code to see how that would affect things?
> >
> > Anyway, how is the power to the SDIO card controlled in this case? Are
> > you using a mmc-pwrseq?
> >
>   vmmc is controlled through GPIO to supply 3.3v power.
>   And the vqmmc is supplied from PMIC which is always 1.8v.
>   (There is no 3.3v here. Perhaps this is one of the reasons to happen
> this issues)

If it's the Marvell 8787/8897/8997 SDIO module you are using, you most
likely need a mmc-pwrseq to properly control the power to the SDIO
module. Perhaps that is what is missing?

See Documentation/devicetree/bindings/net/wireless/marvell-8xxx.txt
and arch/arm/boot/dts/rk3288-veyron.dtsi for an example.

>
> > >
> > >   Anyway, we will wait for your advises.
> > > >
> > > > >         sdio_reset(host);
> > > > >         mmc_go_idle(host);
> > > > >         mmc_send_if_cond(host, host->ocr_avail);
> > > > > --
> > > > > 1.9.1
> > > >
> > > > Kind regards
> > > > Uffe
> >
> > I have a few patches in the pipe, which fixes some other problems in
> > mmc_sdio_init_card(). Possibly those can be related, but I need a day
> > or so to post them, let's see.
> The codebase of this project is kernel-3.18. Maybe it is hard to apply
> these new patches. Anyway, We will try it when we get the patches.
> Thanks.

As you are on a 3.18 kernel, the tests seem quite irrelevant, so I
wouldn't bother with the backports.

Kind regards
Uffe