[PATCH v2] sdio: skip initialization on powered resume

[PATCH v2] sdio: skip initialization on powered resume

[Bing Zhao]

Quoted Michal Miroslaw's comment:

Simplified SDIO spec v.2.00 (section 6.14 - Bus State Diagram)
suggests, that initialization commands (CMD5, CMD3) are not accepted
in CMD state. As the card stays in that state on powered suspend (no
resetting CMD52 nor power cycle is issued) then reinitialization
should be entirely skipped on resume unless the power was lost between
suspend and resume (or card was temporarily removed from the slot).

Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
Changes since v1:
	* Subject changed (was "add MMC_PM_SKIP_RESUME_PROBE to...")
	* No need to introduce new flag MMC_PM_SKIP_RESUME_PROBE
	* Add Michal Miroslaw's comment as patch description

 drivers/mmc/core/sdio.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index f332c52..64d2471 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -605,15 +605,23 @@ static int mmc_sdio_suspend(struct mmc_host *host)
 
 static int mmc_sdio_resume(struct mmc_host *host)
 {
-	int i, err;
+	int i, err = 0;
 
 	BUG_ON(!host);
 	BUG_ON(!host->card);
 
 	/* Basic card reinitialization. */
 	mmc_claim_host(host);
-	err = mmc_sdio_init_card(host, host->ocr, host->card,
-				 (host->pm_flags & MMC_PM_KEEP_POWER));
+
+	/*
+	 * Simplified SDIO spec v2.00 (section 6.14 - Bus State Diagram)
+	 * suggests that initialization should be skipped on powered resume.
+	 */
+	if (!(host->pm_flags & MMC_PM_KEEP_POWER)) {
+		err = mmc_sdio_init_card(host, host->ocr, host->card,
+					host->pm_flags & MMC_PM_KEEP_POWER);
+	}
+
 	if (!err) {
 		/* We may have switched to 1-bit mode during suspend. */
 		err = sdio_enable_4bit_bus(host->card);
-- 
1.5.3.6

Re: [PATCH v2] sdio: skip initialization on powered resume

[Nicolas Pitre]

On Wed, 1 Sep 2010, Bing Zhao wrote:

> Quoted Michal Miroslaw's comment:
> 
> Simplified SDIO spec v.2.00 (section 6.14 - Bus State Diagram)
> suggests, that initialization commands (CMD5, CMD3) are not accepted
> in CMD state. As the card stays in that state on powered suspend (no
> resetting CMD52 nor power cycle is issued) then reinitialization
> should be entirely skipped on resume unless the power was lost between
> suspend and resume (or card was temporarily removed from the slot).
> 
> Signed-off-by: Bing Zhao <bzhao@marvell.com>

Comments below.

> +	/*
> +	 * Simplified SDIO spec v2.00 (section 6.14 - Bus State Diagram)
> +	 * suggests that initialization should be skipped on powered resume.
> +	 */
> +	if (!(host->pm_flags & MMC_PM_KEEP_POWER)) {
> +		err = mmc_sdio_init_card(host, host->ocr, host->card,
> +					host->pm_flags & MMC_PM_KEEP_POWER);
> +	}

Please look at the if() condition, and at the last argument to 
mmc_sdio_init_card(), then ponder.

I think the proper fix goes _inside_ mmc_sdio_init_card() as there are 
certainly still validation checks which are appropriate to perform.


Nicolas

RE: [PATCH v2] sdio: skip initialization on powered resume

[Bing Zhao]

Hi Nicolas,

Thanks for your comments.

> -----Original Message-----
> From: Nicolas Pitre [mailto:nico@fluxnic.net]
> Sent: Thursday, September 02, 2010 10:54 AM
> To: Bing Zhao
> Cc: linux-mmc@vger.kernel.org; Michal Miroslaw; Chris Ball; Andrew Morton
> Subject: Re: [PATCH v2] sdio: skip initialization on powered resume
> 
> On Wed, 1 Sep 2010, Bing Zhao wrote:
> 
> > Quoted Michal Miroslaw's comment:
> >
> > Simplified SDIO spec v.2.00 (section 6.14 - Bus State Diagram)
> > suggests, that initialization commands (CMD5, CMD3) are not accepted
> > in CMD state. As the card stays in that state on powered suspend (no
> > resetting CMD52 nor power cycle is issued) then reinitialization
> > should be entirely skipped on resume unless the power was lost between
> > suspend and resume (or card was temporarily removed from the slot).
> >
> > Signed-off-by: Bing Zhao <bzhao@marvell.com>
> 
> Comments below.
> 
> > +	/*
> > +	 * Simplified SDIO spec v2.00 (section 6.14 - Bus State Diagram)
> > +	 * suggests that initialization should be skipped on powered resume.
> > +	 */
> > +	if (!(host->pm_flags & MMC_PM_KEEP_POWER)) {
> > +		err = mmc_sdio_init_card(host, host->ocr, host->card,
> > +					host->pm_flags & MMC_PM_KEEP_POWER);
> > +	}
> 
> Please look at the if() condition, and at the last argument to
> mmc_sdio_init_card(), then ponder.

You are right. The last argument passed to mmc_sdio_init_card() is zero actually.

		err = mmc_sdio_init_card(host, host->ocr, host->card, 0);

> 
> I think the proper fix goes _inside_ mmc_sdio_init_card() as there are
> certainly still validation checks which are appropriate to perform.

When you have a thought for the fix, I can do the testing on my system.

Thanks,

Bing

> 
> 
> Nicolas

RE: [PATCH v2] sdio: skip initialization on powered resume

[Nicolas Pitre]

On Thu, 2 Sep 2010, Bing Zhao wrote:

> > Please look at the if() condition, and at the last argument to
> > mmc_sdio_init_card(), then ponder.
> 
> You are right. The last argument passed to mmc_sdio_init_card() is zero actually.
> 
> 		err = mmc_sdio_init_card(host, host->ocr, host->card, 0);
> 
> > 
> > I think the proper fix goes _inside_ mmc_sdio_init_card() as there are
> > certainly still validation checks which are appropriate to perform.
> 
> When you have a thought for the fix, I can do the testing on my system.

I'm telling you that you should use the powered_resume argument of 
mmc_sdio_init_card() to skip problematic initializations inside 
mmc_sdio_init_card() when powered_resume is not zero.  Looking at the 
existing code should give you examples of how powered_resume is used and 
why.


Nicolas

RE: [PATCH v2] sdio: skip initialization on powered resume

[Bing Zhao]

Hi Nicolas,

> -----Original Message-----
> From: Nicolas Pitre [mailto:nico@fluxnic.net]
> Sent: Thursday, September 02, 2010 4:30 PM
> To: Bing Zhao
> Cc: linux-mmc@vger.kernel.org; Michal Miroslaw; Chris Ball; Andrew Morton
> Subject: RE: [PATCH v2] sdio: skip initialization on powered resume
> 
> On Thu, 2 Sep 2010, Bing Zhao wrote:
> 
> > > Please look at the if() condition, and at the last argument to
> > > mmc_sdio_init_card(), then ponder.
> >
> > You are right. The last argument passed to mmc_sdio_init_card() is zero actually.
> >
> > 		err = mmc_sdio_init_card(host, host->ocr, host->card, 0);
> >
> > >
> > > I think the proper fix goes _inside_ mmc_sdio_init_card() as there are
> > > certainly still validation checks which are appropriate to perform.
> >
> > When you have a thought for the fix, I can do the testing on my system.
> 
> I'm telling you that you should use the powered_resume argument of
> mmc_sdio_init_card() to skip problematic initializations inside
> mmc_sdio_init_card() when powered_resume is not zero.  Looking at the
> existing code should give you examples of how powered_resume is used and
> why.

Thanks for the hint.

The new patch skips reading CCCR, common CIS, and validation of vendor/device IDs inside mmc_sdio_init_card() when powered_resume is not zero.

If it looks okay for you I will resend it as V3.


drivers/mmc/core/sdio.c |   38 +++++++++++++++++++++-----------------
 1 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index f332c52..37f64d6 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -434,28 +434,32 @@ static int mmc_sdio_init_card(struct mmc_host *host, u32 ocr,
 		goto finish;
 	}
 
-	/*
-	 * Read the common registers.
-	 */
-	err = sdio_read_cccr(card);
-	if (err)
-		goto remove;
+	if (!powered_resume) {
+		/*
+		 * Read the common registers.
+		 */
+		err = sdio_read_cccr(card);
+		if (err)
+			goto remove;
 
-	/*
-	 * Read the common CIS tuples.
-	 */
-	err = sdio_read_common_cis(card);
-	if (err)
-		goto remove;
+		/*
+		 * Read the common CIS tuples.
+		 */
+		err = sdio_read_common_cis(card);
+		if (err)
+			goto remove;
+	}
 
 	if (oldcard) {
-		int same = (card->cis.vendor == oldcard->cis.vendor &&
-			    card->cis.device == oldcard->cis.device);
 		mmc_remove_card(card);
-		if (!same)
-			return -ENOENT;
 
-		card = oldcard;
+		if (!powered_resume) {
+			int same = (card->cis.vendor == oldcard->cis.vendor &&
+				    card->cis.device == oldcard->cis.device);
+			if (!same)
+				return -ENOENT;
+		}
+
 		return 0;
 	}


Regards,

Bing

> 
> 
> Nicolas

RE: [PATCH v2] sdio: skip initialization on powered resume

[Nicolas Pitre]

On Tue, 7 Sep 2010, Bing Zhao wrote:

> Thanks for the hint.
> 
> The new patch skips reading CCCR, common CIS, and validation of 
> vendor/device IDs inside mmc_sdio_init_card() when powered_resume is 
> not zero.

Why do you skip the reading of the CIS and IDs validation?  That's 
basically the main reason for still calling mmc_sdio_init_card().  And 
that only requires CMD 52 so that should be fine.


Nicolas

RE: [PATCH v2] sdio: skip initialization on powered resume

[Bing Zhao]

Hi Nicolas,

> -----Original Message-----
> From: Nicolas Pitre [mailto:nico@fluxnic.net]
> Sent: Tuesday, September 07, 2010 6:29 PM
> To: Bing Zhao
> Cc: linux-mmc@vger.kernel.org; Michal Miroslaw; Chris Ball; Andrew Morton
> Subject: RE: [PATCH v2] sdio: skip initialization on powered resume
> 
> On Tue, 7 Sep 2010, Bing Zhao wrote:
> 
> > Thanks for the hint.
> >
> > The new patch skips reading CCCR, common CIS, and validation of
> > vendor/device IDs inside mmc_sdio_init_card() when powered_resume is
> > not zero.
> 
> Why do you skip the reading of the CIS and IDs validation?  That's
> basically the main reason for still calling mmc_sdio_init_card().  And
> that only requires CMD 52 so that should be fine.

While the system is suspended, the SDIO card could be in sleep mode (deep sleep or IEEE Power Save) as well. Reading CIS or any other CMD52 will fail if the card happens to be in sleep at this moment. If we skip re-initialization (including CCCR/CIS and IDs validation), mmc_sdio_init_card() returns success. Then the client driver's resume() handler will be called and the card can be woken up by client driver.

Regards,

Bing

> 
> 
> Nicolas

RE: [PATCH v2] sdio: skip initialization on powered resume

[Sahitya Tummala]

Hi Bing, Chris,

On Tue, 2010-09-07 at 19:10 -0700, Bing Zhao wrote:
> Hi Nicolas,
> 
> > -----Original Message-----
> > From: Nicolas Pitre [mailto:nico@fluxnic.net]
> > Sent: Tuesday, September 07, 2010 6:29 PM
> > To: Bing Zhao
> > Cc: linux-mmc@vger.kernel.org; Michal Miroslaw; Chris Ball; Andrew Morton
> > Subject: RE: [PATCH v2] sdio: skip initialization on powered resume
> > 
> > On Tue, 7 Sep 2010, Bing Zhao wrote:
> > 
> > > Thanks for the hint.
> > >
> > > The new patch skips reading CCCR, common CIS, and validation of
> > > vendor/device IDs inside mmc_sdio_init_card() when powered_resume is
> > > not zero.
> > 
> > Why do you skip the reading of the CIS and IDs validation?  That's
> > basically the main reason for still calling mmc_sdio_init_card().  And
> > that only requires CMD 52 so that should be fine.
> 
> While the system is suspended, the SDIO card could be in sleep mode (deep sleep or IEEE Power Save) as well. Reading CIS or any other CMD52 will fail if the card happens to be in sleep at this moment. If we skip re-initialization (including CCCR/CIS and IDs validation), mmc_sdio_init_card() returns success. Then the client driver's resume() handler will be called and the card can be woken up by client driver.

In one of your previous patches, "sdio: don't use CMD[357] as part of a
powered SDIO resume", its mentioned that there is an issue with CMD7 in
mmc_sdio_init_card() that is called during SDIO resume on Marvell 8686.

But in mmc_resume_host(), there is still a call to mmc_detect_change()
which in turn calls mmc_sdio_detect(), thus sending CMD7 to the card.
Does CMD7 to your card succeed after your client driver resume is
called?

Thanks,
Sahitya.

--
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

RE: [PATCH v2] sdio: skip initialization on powered resume

[Bing Zhao]

Hi Sahitya,

> -----Original Message-----
> From: Sahitya Tummala [mailto:stummala@codeaurora.org]
> Sent: Tuesday, September 14, 2010 3:16 AM
> To: Bing Zhao
> Cc: Nicolas Pitre; linux-mmc@vger.kernel.org; Michal Miroslaw; Chris Ball; Andrew Morton
> Subject: RE: [PATCH v2] sdio: skip initialization on powered resume
> 
> Hi Bing, Chris,
> 
> On Tue, 2010-09-07 at 19:10 -0700, Bing Zhao wrote:
> > Hi Nicolas,
> >
> > > -----Original Message-----
> > > From: Nicolas Pitre [mailto:nico@fluxnic.net]
> > > Sent: Tuesday, September 07, 2010 6:29 PM
> > > To: Bing Zhao
> > > Cc: linux-mmc@vger.kernel.org; Michal Miroslaw; Chris Ball; Andrew Morton
> > > Subject: RE: [PATCH v2] sdio: skip initialization on powered resume
> > >
> > > On Tue, 7 Sep 2010, Bing Zhao wrote:
> > >
> > > > Thanks for the hint.
> > > >
> > > > The new patch skips reading CCCR, common CIS, and validation of
> > > > vendor/device IDs inside mmc_sdio_init_card() when powered_resume is
> > > > not zero.
> > >
> > > Why do you skip the reading of the CIS and IDs validation?  That's
> > > basically the main reason for still calling mmc_sdio_init_card().  And
> > > that only requires CMD 52 so that should be fine.
> >
> > While the system is suspended, the SDIO card could be in sleep mode (deep sleep or IEEE Power Save)
> as well. Reading CIS or any other CMD52 will fail if the card happens to be in sleep at this moment.
> If we skip re-initialization (including CCCR/CIS and IDs validation), mmc_sdio_init_card() returns
> success. Then the client driver's resume() handler will be called and the card can be woken up by
> client driver.
> 
> In one of your previous patches, "sdio: don't use CMD[357] as part of a
> powered SDIO resume", its mentioned that there is an issue with CMD7 in
> mmc_sdio_init_card() that is called during SDIO resume on Marvell 8686.
> 
> But in mmc_resume_host(), there is still a call to mmc_detect_change()
> which in turn calls mmc_sdio_detect(), thus sending CMD7 to the card.
> Does CMD7 to your card succeed after your client driver resume is
> called?

If CMD7 is sent _before_ client driver's resume handler is called, while 8686 card is in sleep mode, it will fail. If CMD7 is sent _after_ client driver's resume handler is called, it should succeed.

By the way, a patch "mmc: fix all hangs related to mmc/sd card insert/removal during suspend/resume" (4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e by Maxim Levitsky) has removed the call to mmc_detect_change() in mmc_resume_host().

Regards,

Bing

> 
> Thanks,
> Sahitya.
> 
> --
> Sent by a consultant of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum.
> 

RE: [PATCH v2] sdio: skip initialization on powered resume

[Nicolas Pitre]

On Wed, 15 Sep 2010, Bing Zhao wrote:

> If CMD7 is sent _before_ client driver's resume handler is called, 
> while 8686 card is in sleep mode, it will fail. If CMD7 is sent 
> _after_ client driver's resume handler is called, it should succeed.

Maybe that's what we should do in the powered suspend case then.

> By the way, a patch "mmc: fix all hangs related to mmc/sd card 
> insert/removal during suspend/resume" 
> (4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e by Maxim Levitsky) has 
> removed the call to mmc_detect_change() in mmc_resume_host().

If a card is removed while the host is suspended, then this should be 
detected.


Nicolas

Re: [PATCH v2] sdio: skip initialization on powered resume

[zhangfei gao]

On Wed, Sep 15, 2010 at 10:26 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Wed, 15 Sep 2010, Bing Zhao wrote:
>
>> If CMD7 is sent _before_ client driver's resume handler is called,
>> while 8686 card is in sleep mode, it will fail. If CMD7 is sent
>> _after_ client driver's resume handler is called, it should succeed.
>
> Maybe that's what we should do in the powered suspend case then.
>
>> By the way, a patch "mmc: fix all hangs related to mmc/sd card
>> insert/removal during suspend/resume"
>> (4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e by Maxim Levitsky) has
>> removed the call to mmc_detect_change() in mmc_resume_host().
>
> If a card is removed while the host is suspended, then this should be
> detected.
>
>
> Nicolas
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Hi, Bing

Do you have any updated patch to skip mmc_sdio_init_card in resume back.
We need such patch in enable host sleep feature for mrvl8787.

Thanks a lot.

RE: [PATCH v2] sdio: skip initialization on powered resume

[Bing Zhao]

Hi Zhangfei,

> -----Original Message-----
> From: zhangfei gao [mailto:zhangfei.gao@gmail.com]
> Sent: Friday, January 21, 2011 1:07 AM
> To: Nicolas Pitre; Bing Zhao
> Cc: Sahitya Tummala; linux-mmc@vger.kernel.org; Michal Miroslaw; Chris Ball; Andrew Morton; Maxim
> Levitsky
> Subject: Re: [PATCH v2] sdio: skip initialization on powered resume
> 
> On Wed, Sep 15, 2010 at 10:26 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> > On Wed, 15 Sep 2010, Bing Zhao wrote:
> >
> >> If CMD7 is sent _before_ client driver's resume handler is called,
> >> while 8686 card is in sleep mode, it will fail. If CMD7 is sent
> >> _after_ client driver's resume handler is called, it should succeed.
> >
> > Maybe that's what we should do in the powered suspend case then.
> >
> >> By the way, a patch "mmc: fix all hangs related to mmc/sd card
> >> insert/removal during suspend/resume"
> >> (4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e by Maxim Levitsky) has
> >> removed the call to mmc_detect_change() in mmc_resume_host().
> >
> > If a card is removed while the host is suspended, then this should be
> > detected.
> >
> >
> > Nicolas
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> 
> Hi, Bing
> 
> Do you have any updated patch to skip mmc_sdio_init_card in resume back.
> We need such patch in enable host sleep feature for mrvl8787.

I posted a patch that skips mmc_sdio_init_card() with MMC_PM_SKIP_RESUME_PROBE flag earlier:

[PATCH v1] sdio: add MMC_PM_SKIP_RESUME_PROBE to workaround powered resume
http://marc.info/?l=linux-mmc&m=128294262424567&w=2

Nicolas commented that it's too hackish with this approach.
http://marc.info/?l=linux-mmc&m=128294738230151&w=2

Other than that, I couldn't think of a better way to solve the issue here:

In mmc_sdio_init_card() CIS device/vendor IDs are read to check if the card has been replaced or not when the system was suspended. But reading these IDs will cause CMD52 timeout if the card is in sleep state. The function driver can wake up the card by writing to certain card specific register, so that the followed SDIO commands (CMD52, CMD53, etc.) can go through. But the resume handler of the function driver won't be invoked until the IDs get validated.

Regards,

Bing

> 
> Thanks a lot.

RE: [PATCH v2] sdio: skip initialization on powered resume

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2821 bytes --]

On Fri, 21 Jan 2011, Bing Zhao wrote:

> Hi Zhangfei,
> 
> > -----Original Message-----
> > From: zhangfei gao [mailto:zhangfei.gao@gmail.com]
> > Sent: Friday, January 21, 2011 1:07 AM
> > To: Nicolas Pitre; Bing Zhao
> > Cc: Sahitya Tummala; linux-mmc@vger.kernel.org; Michal Miroslaw; Chris Ball; Andrew Morton; Maxim
> > Levitsky
> > Subject: Re: [PATCH v2] sdio: skip initialization on powered resume
> > 
> > On Wed, Sep 15, 2010 at 10:26 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> > > On Wed, 15 Sep 2010, Bing Zhao wrote:
> > >
> > >> If CMD7 is sent _before_ client driver's resume handler is called,
> > >> while 8686 card is in sleep mode, it will fail. If CMD7 is sent
> > >> _after_ client driver's resume handler is called, it should succeed.
> > >
> > > Maybe that's what we should do in the powered suspend case then.
> > >
> > >> By the way, a patch "mmc: fix all hangs related to mmc/sd card
> > >> insert/removal during suspend/resume"
> > >> (4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e by Maxim Levitsky) has
> > >> removed the call to mmc_detect_change() in mmc_resume_host().
> > >
> > > If a card is removed while the host is suspended, then this should be
> > > detected.
> > >
> > >
> > > Nicolas
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > >
> > 
> > Hi, Bing
> > 
> > Do you have any updated patch to skip mmc_sdio_init_card in resume back.
> > We need such patch in enable host sleep feature for mrvl8787.
> 
> I posted a patch that skips mmc_sdio_init_card() with MMC_PM_SKIP_RESUME_PROBE flag earlier:
> 
> [PATCH v1] sdio: add MMC_PM_SKIP_RESUME_PROBE to workaround powered resume
> http://marc.info/?l=linux-mmc&m=128294262424567&w=2
> 
> Nicolas commented that it's too hackish with this approach.
> http://marc.info/?l=linux-mmc&m=128294738230151&w=2
> 
> Other than that, I couldn't think of a better way to solve the issue here:
> 
> In mmc_sdio_init_card() CIS device/vendor IDs are read to check if the 
> card has been replaced or not when the system was suspended. But 
> reading these IDs will cause CMD52 timeout if the card is in sleep 
> state. The function driver can wake up the card by writing to certain 
> card specific register, so that the followed SDIO commands (CMD52, 
> CMD53, etc.) can go through. But the resume handler of the function 
> driver won't be invoked until the IDs get validated.

Please add the extra explanation above to the commit log so that 
the context is not lost.  And then

Acked-by: Nicolas Pitre <nico@fluxnic.net>

Then this could be revisited eventually when more devices are supported 
and a better abstraction to cover their needs could be created.


Nicolas

RE: [PATCH v2] sdio: skip initialization on powered resume

[Bing Zhao]

> -----Original Message-----
> From: Nicolas Pitre [mailto:nico@fluxnic.net]
> Sent: Friday, January 21, 2011 6:56 PM
> To: Bing Zhao
> Cc: zhangfei gao; Sahitya Tummala; linux-mmc@vger.kernel.org; Michal Miroslaw; Chris Ball; Andrew
> Morton; Maxim Levitsky
> Subject: RE: [PATCH v2] sdio: skip initialization on powered resume
> 
> On Fri, 21 Jan 2011, Bing Zhao wrote:
> 
> > Hi Zhangfei,
> >
> > > -----Original Message-----
> > > From: zhangfei gao [mailto:zhangfei.gao@gmail.com]
> > > Sent: Friday, January 21, 2011 1:07 AM
> > > To: Nicolas Pitre; Bing Zhao
> > > Cc: Sahitya Tummala; linux-mmc@vger.kernel.org; Michal Miroslaw; Chris Ball; Andrew Morton; Maxim
> > > Levitsky
> > > Subject: Re: [PATCH v2] sdio: skip initialization on powered resume
> > >
> > > On Wed, Sep 15, 2010 at 10:26 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> > > > On Wed, 15 Sep 2010, Bing Zhao wrote:
> > > >
> > > >> If CMD7 is sent _before_ client driver's resume handler is called,
> > > >> while 8686 card is in sleep mode, it will fail. If CMD7 is sent
> > > >> _after_ client driver's resume handler is called, it should succeed.
> > > >
> > > > Maybe that's what we should do in the powered suspend case then.
> > > >
> > > >> By the way, a patch "mmc: fix all hangs related to mmc/sd card
> > > >> insert/removal during suspend/resume"
> > > >> (4c2ef25fe0b847d2ae818f74758ddb0be1c27d8e by Maxim Levitsky) has
> > > >> removed the call to mmc_detect_change() in mmc_resume_host().
> > > >
> > > > If a card is removed while the host is suspended, then this should be
> > > > detected.
> > > >
> > > >
> > > > Nicolas
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > >
> > >
> > > Hi, Bing
> > >
> > > Do you have any updated patch to skip mmc_sdio_init_card in resume back.
> > > We need such patch in enable host sleep feature for mrvl8787.
> >
> > I posted a patch that skips mmc_sdio_init_card() with MMC_PM_SKIP_RESUME_PROBE flag earlier:
> >
> > [PATCH v1] sdio: add MMC_PM_SKIP_RESUME_PROBE to workaround powered resume
> > http://marc.info/?l=linux-mmc&m=128294262424567&w=2
> >
> > Nicolas commented that it's too hackish with this approach.
> > http://marc.info/?l=linux-mmc&m=128294738230151&w=2
> >
> > Other than that, I couldn't think of a better way to solve the issue here:
> >
> > In mmc_sdio_init_card() CIS device/vendor IDs are read to check if the
> > card has been replaced or not when the system was suspended. But
> > reading these IDs will cause CMD52 timeout if the card is in sleep
> > state. The function driver can wake up the card by writing to certain
> > card specific register, so that the followed SDIO commands (CMD52,
> > CMD53, etc.) can go through. But the resume handler of the function
> > driver won't be invoked until the IDs get validated.
> 
> Please add the extra explanation above to the commit log so that
> the context is not lost.  And then
> 
> Acked-by: Nicolas Pitre <nico@fluxnic.net>
> 
> Then this could be revisited eventually when more devices are supported
> and a better abstraction to cover their needs could be created.
> 
> 
> Nicolas

Hi Nicolas,

Thanks for ack. I'll resend the patch with extra explanation in commit log.


Hi Chris,

I'll rebase and test the patch based on git://git.kernel.org/pub/scm/linux/kernel/git/cjb/mmc.git tree. Should I use "master" or "mmc-next" branch?

Thanks,

Bing

Re: [PATCH v2] sdio: skip initialization on powered resume

[Chris Ball]

Hi Bing,

On Fri, Jan 21, 2011 at 07:27:06PM -0800, Bing Zhao wrote:
> I'll rebase and test the patch based on git://git.kernel.org/pub/scm/linux/kernel/git/cjb/mmc.git tree. Should I use "master" or "mmc-next" branch?

mmc-next, please.  Thanks!

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v2] sdio: skip initialization on powered resume

[Ohad Ben-Cohen]

On Fri, Jan 21, 2011 at 11:07 AM, zhangfei gao <zhangfei.gao@gmail.com> wrote:
> Do you have any updated patch to skip mmc_sdio_init_card in resume back.
> We need such patch in enable host sleep feature for mrvl8787.

Is mrvl8787 a removable card ?

I'm asking because we already skip mmc_sdio_init_card() for
powered-resumed nonremovable cards (check out commit 3cfc33a "mmc:
sdio: don't reinitialize nonremovable powered-resumed cards").

I'm not familiar with marvell's cards, but I do remember a thread
mentioning they have dedicated reset GPIOs, and that may suggest they
are nonremovables. If that's the case, simply setting
MMC_CAP_NONREMOVABLE on the relevant slot should do the trick for you.

RE: [PATCH v2] sdio: skip initialization on powered resume

[Bing Zhao]

Hi Ohad,

> -----Original Message-----
> From: Ohad Ben-Cohen [mailto:ohad@wizery.com]
> Sent: Saturday, January 22, 2011 2:01 PM
> To: zhangfei gao
> Cc: Nicolas Pitre; Bing Zhao; Sahitya Tummala; linux-mmc@vger.kernel.org; Michal Miroslaw; Chris Ball;
> Andrew Morton; Maxim Levitsky
> Subject: Re: [PATCH v2] sdio: skip initialization on powered resume
> 
> On Fri, Jan 21, 2011 at 11:07 AM, zhangfei gao <zhangfei.gao@gmail.com> wrote:
> > Do you have any updated patch to skip mmc_sdio_init_card in resume back.
> > We need such patch in enable host sleep feature for mrvl8787.
> 
> Is mrvl8787 a removable card ?

It can be either a removable or non-removable card, depending on what platform is used.

> 
> I'm asking because we already skip mmc_sdio_init_card() for
> powered-resumed nonremovable cards (check out commit 3cfc33a "mmc:
> sdio: don't reinitialize nonremovable powered-resumed cards").

Thanks for the info.

> 
> I'm not familiar with marvell's cards, but I do remember a thread
> mentioning they have dedicated reset GPIOs, and that may suggest they
> are nonremovables. If that's the case, simply setting
> MMC_CAP_NONREMOVABLE on the relevant slot should do the trick for you.

I think this approach works for Zhangfei on his embedded platform on which the 8787 card is non-removable.

Regards,

Bing

Re: [PATCH v2] sdio: skip initialization on powered resume

[zhangfei gao]

On Mon, Jan 24, 2011 at 9:17 PM, Bing Zhao <bzhao@marvell.com> wrote:
> Hi Ohad,
>
>> -----Original Message-----
>> From: Ohad Ben-Cohen [mailto:ohad@wizery.com]
>> Sent: Saturday, January 22, 2011 2:01 PM
>> To: zhangfei gao
>> Cc: Nicolas Pitre; Bing Zhao; Sahitya Tummala; linux-mmc@vger.kernel.org; Michal Miroslaw; Chris Ball;
>> Andrew Morton; Maxim Levitsky
>> Subject: Re: [PATCH v2] sdio: skip initialization on powered resume
>>
>> On Fri, Jan 21, 2011 at 11:07 AM, zhangfei gao <zhangfei.gao@gmail.com> wrote:
>> > Do you have any updated patch to skip mmc_sdio_init_card in resume back.
>> > We need such patch in enable host sleep feature for mrvl8787.
>>
>> Is mrvl8787 a removable card ?
>
> It can be either a removable or non-removable card, depending on what platform is used.
>
>>
>> I'm asking because we already skip mmc_sdio_init_card() for
>> powered-resumed nonremovable cards (check out commit 3cfc33a "mmc:
>> sdio: don't reinitialize nonremovable powered-resumed cards").
>
> Thanks for the info.
>
>>
>> I'm not familiar with marvell's cards, but I do remember a thread
>> mentioning they have dedicated reset GPIOs, and that may suggest they
>> are nonremovables. If that's the case, simply setting
>> MMC_CAP_NONREMOVABLE on the relevant slot should do the trick for you.
>
> I think this approach works for Zhangfei on his embedded platform on which the 8787 card is non-removable.

Hi, Ohad,

Thanks for your patch, it is workable on the platform where mrvl8787
is non-removable.
However, we still have platform mrvl8787 is removable,
SDHCI_PRESENT_STATE can be read out value.
Could we remove mmc_card_is_removable(host) condition, the skip is not
related with whether card is removable or not, do you think so?

Thanks a lot.

>
> Regards,
>
> Bing
>

Re: [PATCH v2] sdio: skip initialization on powered resume

[Ohad Ben-Cohen]

Hello Zhangfei,

On Tue, Jan 25, 2011 at 5:10 AM, zhangfei gao <zhangfei.gao@gmail.com> wrote:
> Could we remove mmc_card_is_removable(host) condition, the skip is not
> related with whether card is removable or not, do you think so?

But, as Nicolas pointed out before, what if the card is removed (or
replaced) while the system was suspended ?

It seems that this won't be detected, and your driver, once resumed,
will assume the card is still in place and will just fail. Are you ok
with it ?

Can you please explain what's required to wake up mrvl8787 from this
deep sleep ?

One thing we can do is letting drivers register a wake-up function
within the SDIO core, just before suspending. Then, upon resume, if
such a wake-up function was registered, SDIO core would invoke it just
before it tries to read the CIS.

This way your card stays in deep sleep while the system is suspended,
and when the system wakes up, SDIO core has a chance to make sure the
card was not removed/replaced.

If the card is replaced, the wake-up function will probably just fail,
and then SDIO core will have to reinitialize that new card. I'm not
sure that's ideal, but those errors would happen anyway if we just
skip the reinitialization and let the driver assume the card wasn't
replaced.

What do you think ? Would that work for you ?

Btw we have the same thing with the wl1271. Since it's mostly
nonremovable, we're ok with 3cfc33a, but that may change.

Thanks,
Ohad.

Re: [PATCH v2] sdio: skip initialization on powered resume

[Ohad Ben-Cohen]

On Tue, Jan 25, 2011 at 9:11 AM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> One thing we can do is letting drivers register a wake-up function
> within the SDIO core, just before suspending. Then, upon resume, if
> such a wake-up function was registered, SDIO core would invoke it just
> before it tries to read the CIS.

I guess that would be non-trivial, since at that point the driver is
not resumed yet, and you might need its facilities to receive some
wake-up ACK from the chip before continuing.