All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] sdio: skip initialization on powered resume
@ 2010-09-02  1:41 Bing Zhao
  2010-09-02 17:54 ` Nicolas Pitre
  0 siblings, 1 reply; 20+ messages in thread
From: Bing Zhao @ 2010-09-02  1:41 UTC (permalink / raw)
  To: linux-mmc
  Cc: Bing Zhao, Michal Miroslaw, Nicolas Pitre, Chris Ball, Andrew Morton

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


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] sdio: skip initialization on powered resume
  2010-09-02  1:41 [PATCH v2] sdio: skip initialization on powered resume Bing Zhao
@ 2010-09-02 17:54 ` Nicolas Pitre
  2010-09-02 22:58   ` Bing Zhao
  0 siblings, 1 reply; 20+ messages in thread
From: Nicolas Pitre @ 2010-09-02 17:54 UTC (permalink / raw)
  To: Bing Zhao; +Cc: linux-mmc, Michal Miroslaw, Chris Ball, Andrew Morton

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH v2] sdio: skip initialization on powered resume
  2010-09-02 17:54 ` Nicolas Pitre
@ 2010-09-02 22:58   ` Bing Zhao
  2010-09-02 23:30     ` Nicolas Pitre
  0 siblings, 1 reply; 20+ messages in thread
From: Bing Zhao @ 2010-09-02 22:58 UTC (permalink / raw)
  To: Nicolas Pitre; +Cc: linux-mmc, Michal Miroslaw, Chris Ball, Andrew Morton

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH v2] sdio: skip initialization on powered resume
  2010-09-02 22:58   ` Bing Zhao
@ 2010-09-02 23:30     ` Nicolas Pitre
  2010-09-08  1:03       ` Bing Zhao
  0 siblings, 1 reply; 20+ messages in thread
From: Nicolas Pitre @ 2010-09-02 23:30 UTC (permalink / raw)
  To: Bing Zhao; +Cc: linux-mmc, Michal Miroslaw, Chris Ball, Andrew Morton

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH v2] sdio: skip initialization on powered resume
  2010-09-02 23:30     ` Nicolas Pitre
@ 2010-09-08  1:03       ` Bing Zhao
  2010-09-08  1:28         ` Nicolas Pitre
  0 siblings, 1 reply; 20+ messages in thread
From: Bing Zhao @ 2010-09-08  1:03 UTC (permalink / raw)
  To: Nicolas Pitre; +Cc: linux-mmc, Michal Miroslaw, Chris Ball, Andrew Morton

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* RE: [PATCH v2] sdio: skip initialization on powered resume
  2010-09-08  1:03       ` Bing Zhao
@ 2010-09-08  1:28         ` Nicolas Pitre
  2010-09-08  2:10           ` Bing Zhao
  0 siblings, 1 reply; 20+ messages in thread
From: Nicolas Pitre @ 2010-09-08  1:28 UTC (permalink / raw)
  To: Bing Zhao; +Cc: linux-mmc, Michal Miroslaw, Chris Ball, Andrew Morton

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH v2] sdio: skip initialization on powered resume
  2010-09-08  1:28         ` Nicolas Pitre
@ 2010-09-08  2:10           ` Bing Zhao
  2010-09-14 10:15             ` Sahitya Tummala
  0 siblings, 1 reply; 20+ messages in thread
From: Bing Zhao @ 2010-09-08  2:10 UTC (permalink / raw)
  To: Nicolas Pitre; +Cc: linux-mmc, Michal Miroslaw, Chris Ball, Andrew Morton

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH v2] sdio: skip initialization on powered resume
  2010-09-08  2:10           ` Bing Zhao
@ 2010-09-14 10:15             ` Sahitya Tummala
  2010-09-16  0:27               ` Bing Zhao
  0 siblings, 1 reply; 20+ messages in thread
From: Sahitya Tummala @ 2010-09-14 10:15 UTC (permalink / raw)
  To: Bing Zhao
  Cc: Nicolas Pitre, linux-mmc, Michal Miroslaw, Chris Ball, Andrew Morton

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.



^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH v2] sdio: skip initialization on powered resume
  2010-09-14 10:15             ` Sahitya Tummala
@ 2010-09-16  0:27               ` Bing Zhao
  2010-09-16  2:26                 ` Nicolas Pitre
  0 siblings, 1 reply; 20+ messages in thread
From: Bing Zhao @ 2010-09-16  0:27 UTC (permalink / raw)
  To: Sahitya Tummala
  Cc: Nicolas Pitre, linux-mmc, Michal Miroslaw, Chris Ball,
	Andrew Morton, Maxim Levitsky

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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH v2] sdio: skip initialization on powered resume
  2010-09-16  0:27               ` Bing Zhao
@ 2010-09-16  2:26                 ` Nicolas Pitre
  2011-01-21  9:07                   ` zhangfei gao
  0 siblings, 1 reply; 20+ messages in thread
From: Nicolas Pitre @ 2010-09-16  2:26 UTC (permalink / raw)
  To: Bing Zhao
  Cc: Sahitya Tummala, linux-mmc, Michal Miroslaw, Chris Ball,
	Andrew Morton, Maxim Levitsky

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] sdio: skip initialization on powered resume
  2010-09-16  2:26                 ` Nicolas Pitre
@ 2011-01-21  9:07                   ` zhangfei gao
  2011-01-22  2:22                     ` Bing Zhao
  2011-01-22 22:01                     ` Ohad Ben-Cohen
  0 siblings, 2 replies; 20+ messages in thread
From: zhangfei gao @ 2011-01-21  9:07 UTC (permalink / raw)
  To: Nicolas Pitre, Bing Zhao
  Cc: Sahitya Tummala, linux-mmc, Michal Miroslaw, Chris Ball,
	Andrew Morton, Maxim Levitsky

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH v2] sdio: skip initialization on powered resume
  2011-01-21  9:07                   ` zhangfei gao
@ 2011-01-22  2:22                     ` Bing Zhao
  2011-01-22  2:55                       ` Nicolas Pitre
  2011-01-22 22:01                     ` Ohad Ben-Cohen
  1 sibling, 1 reply; 20+ messages in thread
From: Bing Zhao @ 2011-01-22  2:22 UTC (permalink / raw)
  To: zhangfei gao, Nicolas Pitre
  Cc: Sahitya Tummala, linux-mmc, Michal Miroslaw, Chris Ball,
	Andrew Morton, Maxim Levitsky

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH v2] sdio: skip initialization on powered resume
  2011-01-22  2:22                     ` Bing Zhao
@ 2011-01-22  2:55                       ` Nicolas Pitre
  2011-01-22  3:27                         ` Bing Zhao
  0 siblings, 1 reply; 20+ messages in thread
From: Nicolas Pitre @ 2011-01-22  2:55 UTC (permalink / raw)
  To: Bing Zhao
  Cc: zhangfei gao, Sahitya Tummala, linux-mmc, Michal Miroslaw,
	Chris Ball, Andrew Morton, Maxim Levitsky

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH v2] sdio: skip initialization on powered resume
  2011-01-22  2:55                       ` Nicolas Pitre
@ 2011-01-22  3:27                         ` Bing Zhao
  2011-01-22  3:38                           ` Chris Ball
  0 siblings, 1 reply; 20+ messages in thread
From: Bing Zhao @ 2011-01-22  3:27 UTC (permalink / raw)
  To: Nicolas Pitre, Chris Ball
  Cc: zhangfei gao, Sahitya Tummala, linux-mmc, Michal Miroslaw,
	Andrew Morton, Maxim Levitsky



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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] sdio: skip initialization on powered resume
  2011-01-22  3:27                         ` Bing Zhao
@ 2011-01-22  3:38                           ` Chris Ball
  0 siblings, 0 replies; 20+ messages in thread
From: Chris Ball @ 2011-01-22  3:38 UTC (permalink / raw)
  To: Bing Zhao
  Cc: Nicolas Pitre, Chris Ball, zhangfei gao, Sahitya Tummala,
	linux-mmc, Michal Miroslaw, Andrew Morton, Maxim Levitsky

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] sdio: skip initialization on powered resume
  2011-01-21  9:07                   ` zhangfei gao
  2011-01-22  2:22                     ` Bing Zhao
@ 2011-01-22 22:01                     ` Ohad Ben-Cohen
  2011-01-25  2:17                       ` Bing Zhao
  1 sibling, 1 reply; 20+ messages in thread
From: Ohad Ben-Cohen @ 2011-01-22 22:01 UTC (permalink / raw)
  To: zhangfei gao
  Cc: Nicolas Pitre, Bing Zhao, Sahitya Tummala, linux-mmc,
	Michal Miroslaw, Chris Ball, Andrew Morton, Maxim Levitsky

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* RE: [PATCH v2] sdio: skip initialization on powered resume
  2011-01-22 22:01                     ` Ohad Ben-Cohen
@ 2011-01-25  2:17                       ` Bing Zhao
  2011-01-25  3:10                         ` zhangfei gao
  0 siblings, 1 reply; 20+ messages in thread
From: Bing Zhao @ 2011-01-25  2:17 UTC (permalink / raw)
  To: Ohad Ben-Cohen, zhangfei gao
  Cc: Nicolas Pitre, Sahitya Tummala, linux-mmc, Michal Miroslaw,
	Chris Ball, Andrew Morton, Maxim Levitsky

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] sdio: skip initialization on powered resume
  2011-01-25  2:17                       ` Bing Zhao
@ 2011-01-25  3:10                         ` zhangfei gao
  2011-01-25  7:11                           ` Ohad Ben-Cohen
  0 siblings, 1 reply; 20+ messages in thread
From: zhangfei gao @ 2011-01-25  3:10 UTC (permalink / raw)
  To: Bing Zhao
  Cc: Ohad Ben-Cohen, Nicolas Pitre, Sahitya Tummala, linux-mmc,
	Michal Miroslaw, Chris Ball, Andrew Morton, Maxim Levitsky,
	Jun Nie

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
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] sdio: skip initialization on powered resume
  2011-01-25  3:10                         ` zhangfei gao
@ 2011-01-25  7:11                           ` Ohad Ben-Cohen
  2011-01-25  7:24                             ` Ohad Ben-Cohen
  0 siblings, 1 reply; 20+ messages in thread
From: Ohad Ben-Cohen @ 2011-01-25  7:11 UTC (permalink / raw)
  To: zhangfei gao
  Cc: Bing Zhao, Nicolas Pitre, Sahitya Tummala, linux-mmc,
	Michal Miroslaw, Chris Ball, Andrew Morton, Maxim Levitsky,
	Jun Nie

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] sdio: skip initialization on powered resume
  2011-01-25  7:11                           ` Ohad Ben-Cohen
@ 2011-01-25  7:24                             ` Ohad Ben-Cohen
  0 siblings, 0 replies; 20+ messages in thread
From: Ohad Ben-Cohen @ 2011-01-25  7:24 UTC (permalink / raw)
  To: zhangfei gao
  Cc: Bing Zhao, Nicolas Pitre, Sahitya Tummala, linux-mmc,
	Michal Miroslaw, Chris Ball, Andrew Morton, Maxim Levitsky,
	Jun Nie

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2011-01-25  7:25 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-02  1:41 [PATCH v2] sdio: skip initialization on powered resume Bing Zhao
2010-09-02 17:54 ` Nicolas Pitre
2010-09-02 22:58   ` Bing Zhao
2010-09-02 23:30     ` Nicolas Pitre
2010-09-08  1:03       ` Bing Zhao
2010-09-08  1:28         ` Nicolas Pitre
2010-09-08  2:10           ` Bing Zhao
2010-09-14 10:15             ` Sahitya Tummala
2010-09-16  0:27               ` Bing Zhao
2010-09-16  2:26                 ` Nicolas Pitre
2011-01-21  9:07                   ` zhangfei gao
2011-01-22  2:22                     ` Bing Zhao
2011-01-22  2:55                       ` Nicolas Pitre
2011-01-22  3:27                         ` Bing Zhao
2011-01-22  3:38                           ` Chris Ball
2011-01-22 22:01                     ` Ohad Ben-Cohen
2011-01-25  2:17                       ` Bing Zhao
2011-01-25  3:10                         ` zhangfei gao
2011-01-25  7:11                           ` Ohad Ben-Cohen
2011-01-25  7:24                             ` Ohad Ben-Cohen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.