[PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.

[PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2749 bytes --]


Greetings.

 I've been chasing down a problem with the SDIO-attached wifi card in the
 GTA04 (www.gta04.org).
 The problem seems very similar to that described in 

    http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg04289.html

 as referenced in:

commit 08da834a24312157f512224691ad1fddd11c1073
Author: Daniel Drake <dsd@laptop.org>
Date:   Wed Jul 20 17:39:22 2011 +0100

    mmc: enable runtime PM by default


and if I revert that commit so that runtime PM is not enabled the problem
goes away.

(The wifi chip is a libertas_sdio supported chip connected to an mmc
interface on an omap3, and has a separate regulator for power supply, plus a
GPIO pin for reset, just like in the email thread.  The problem is
exemplified by:
[   64.643981] libertas_sdio: probe of mmc1:0001:1 failed with error -16
).

I finally managed to track down exactly what was happening - runtime PM was
putting the interface to sleep before the SDIO functions were probed so
initialising them failed.

This patch fixes it and seem to be 'correct'.

Please consider applying.

Thanks,
NeilBrown

From: NeilBrown <neilb@suse.de>
Date: Mon, 5 Dec 2011 11:34:47 +1100
Subject: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until
 probe is finished.

mmc_attach_sdio currently enables runtime power management on
the mmc interface before it completes the probe of functions.
This means that it might be asleep during the probe and the probe
will fail.

In particular, if 'drivers_autoprobe' is enabled on the mmc bus, then
device_attach() will be called by bus_probe_device() from device_add()
and it will pm_runtime_get_noresume()/pm_runtime_put_sync().

As runtime power management this the device is running
(pm_runtime_set_active() in mmc_attach_sdio()) and rtpm is enabled
(pm_runtime_enable()), the pm_runtime_put_sync() call puts the mmc
interface to sleep, so function probing fails.

So this patch moves the call to pm_runtime_enable to after all the
calls to sdio_add_func.

Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 3ab565e..c0c83f6 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -811,11 +811,6 @@ int mmc_attach_sdio(struct mmc_host *host)
 		err = pm_runtime_set_active(&card->dev);
 		if (err)
 			goto remove;
-
-		/*
-		 * Enable runtime PM for this card
-		 */
-		pm_runtime_enable(&card->dev);
 	}
 
 	/*
@@ -858,6 +853,12 @@ int mmc_attach_sdio(struct mmc_host *host)
 	}
 
 	mmc_claim_host(host);
+	if (host->caps & MMC_CAP_POWER_OFF_CARD) {
+		/*
+		 * Enable runtime PM for this card
+		 */
+		pm_runtime_enable(&card->dev);
+	}
 	return 0;
 
 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.

[Ohad Ben-Cohen]

Hi Neil,

On Mon, Dec 5, 2011 at 3:54 AM, NeilBrown <neilb@suse.de> wrote:
>  I've been chasing down a problem with the SDIO-attached wifi card in the
>  GTA04 (www.gta04.org).

8686, right ?

>  The problem seems very similar to that described in
>
>    http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg04289.html

Don't go that far back, Joe just reported this exact problem in :

http://comments.gmane.org/gmane.linux.kernel.mmc/11231

> commit 08da834a24312157f512224691ad1fddd11c1073
> Author: Daniel Drake <dsd@laptop.org>
> Date:   Wed Jul 20 17:39:22 2011 +0100
>
>    mmc: enable runtime PM by default
>
>
> and if I revert that commit so that runtime PM is not enabled the problem
> goes away.

And this is most likely what we're going to do, unless someone with
the 8686 can further look into the problem.

> (The wifi chip is a libertas_sdio supported chip connected to an mmc
> interface on an omap3, and has a separate regulator for power supply, plus a
> GPIO pin for reset, just like in the email thread.  The problem is
> exemplified by:
> [   64.643981] libertas_sdio: probe of mmc1:0001:1 failed with error -16
> ).

Yes, the same issue that Joe is seeing.

> I finally managed to track down exactly what was happening - runtime PM was
> putting the interface to sleep before the SDIO functions were probed so
> initialising them failed.

Yeah, but the question is why it fails.

It's perfectly fine to power the card down before the functions are
probed, because just before they are probed the bus is responsible to
power the card up again.

> From: NeilBrown <neilb@suse.de>
> Date: Mon, 5 Dec 2011 11:34:47 +1100
> Subject: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.

You can't tell when probe is finished. In fact, in can happen very far
in the future or even never at all (e.g. when the function driver
isn't loaded).

> mmc_attach_sdio currently enables runtime power management on
> the mmc interface before it completes the probe of functions.
> This means that it might be asleep during the probe and the probe
> will fail.

No - the sdio bus powers the card up before probing the drivers. See
sdio_bus_probe.

> In particular, if 'drivers_autoprobe' is enabled on the mmc bus, then
> device_attach() will be called by bus_probe_device() from device_add()
> and it will pm_runtime_get_noresume()/pm_runtime_put_sync().
>
> As runtime power management this the device is running
> (pm_runtime_set_active() in mmc_attach_sdio()) and rtpm is enabled
> (pm_runtime_enable()), the pm_runtime_put_sync() call puts the mmc
> interface to sleep,

This is fine

> so function probing fails.

The question is why. Again - sdio_bus_probe should power up the card.
For some reason, it (or something else) fails to do so with the 8686
on some setups. Other chips might have similar issues, too - we just
don't know (all we know is that it works great with the wl12xx, and
with the Daniel's 8686 setup).

> So this patch moves the call to pm_runtime_enable to after all the
> calls to sdio_add_func.

Looks like this will have the undesired side-effect of keeping the
chip powered up, even if it doesn't have any driver loaded for it.

And this doesn't really address the problem: we still don't know why
the 8686 fails to power up at this point.

Can you please tell us where exactly is the first failure coming from
? is this from libertas own probe function ? is this sdio_bus_probe
getting an error from calling pm_runtime_get_sync ?

Thanks,
Ohad

Re: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 5044 bytes --]

On Mon, 5 Dec 2011 12:25:42 +0200 Ohad Ben-Cohen <ohad@wizery.com> wrote:

> Hi Neil,
> 
> On Mon, Dec 5, 2011 at 3:54 AM, NeilBrown <neilb@suse.de> wrote:
> >  I've been chasing down a problem with the SDIO-attached wifi card in the
> >  GTA04 (www.gta04.org).
> 
> 8686, right ?

yep, that's the one!

> 
> >  The problem seems very similar to that described in
> >
> >    http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg04289.html
> 
> Don't go that far back, Joe just reported this exact problem in :
> 
> http://comments.gmane.org/gmane.linux.kernel.mmc/11231

Indeed.

> 
> > commit 08da834a24312157f512224691ad1fddd11c1073
> > Author: Daniel Drake <dsd@laptop.org>
> > Date:   Wed Jul 20 17:39:22 2011 +0100
> >
> >    mmc: enable runtime PM by default
> >
> >
> > and if I revert that commit so that runtime PM is not enabled the problem
> > goes away.
> 
> And this is most likely what we're going to do, unless someone with
> the 8686 can further look into the problem.

Challenge accepted.

Even if I don't find a better solution, this seems backwards.  Sure the
default should be that PM is enabled, but individual board can request no
PM on MMC interfaces where it is a problem.

> 
> > (The wifi chip is a libertas_sdio supported chip connected to an mmc
> > interface on an omap3, and has a separate regulator for power supply, plus a
> > GPIO pin for reset, just like in the email thread.  The problem is
> > exemplified by:
> > [   64.643981] libertas_sdio: probe of mmc1:0001:1 failed with error -16
> > ).
> 
> Yes, the same issue that Joe is seeing.
> 
> > I finally managed to track down exactly what was happening - runtime PM was
> > putting the interface to sleep before the SDIO functions were probed so
> > initialising them failed.
> 
> Yeah, but the question is why it fails.

The chip has a requirement that the reset line is held down during power-on,
and raised shortly after (I don't know exactly how short).  So if you just
remove power and give it back, the chip doesn't come up properly.

> 
> It's perfectly fine to power the card down before the functions are
> probed, because just before they are probed the bus is responsible to
> power the card up again.

"It *should be* perfectly fine ..." :-)

We just have to make sure the bug powers up the card properly.
Maybe I need to create a virtual regulator that powers on the real regulator,
then raises the reset line.  I wonder how hard that is.


> 
> > From: NeilBrown <neilb@suse.de>
> > Date: Mon, 5 Dec 2011 11:34:47 +1100
> > Subject: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.
> 
> You can't tell when probe is finished. In fact, in can happen very far
> in the future or even never at all (e.g. when the function driver
> isn't loaded).

Ahhh... I naively assumed that 

	/*
	 * ...then the SDIO functions.
	 */
	for (i = 0;i < funcs;i++) {
		err = sdio_add_func(host->card->sdio_func[i]);
		if (err)
			goto remove_added;
	}

would add all the functions.  but as you say: the drivers might not be loaded
yet.


> 
> > mmc_attach_sdio currently enables runtime power management on
> > the mmc interface before it completes the probe of functions.
> > This means that it might be asleep during the probe and the probe
> > will fail.
> 
> No - the sdio bus powers the card up before probing the drivers. See
> sdio_bus_probe.
> 
> > In particular, if 'drivers_autoprobe' is enabled on the mmc bus, then
> > device_attach() will be called by bus_probe_device() from device_add()
> > and it will pm_runtime_get_noresume()/pm_runtime_put_sync().
> >
> > As runtime power management this the device is running
> > (pm_runtime_set_active() in mmc_attach_sdio()) and rtpm is enabled
> > (pm_runtime_enable()), the pm_runtime_put_sync() call puts the mmc
> > interface to sleep,
> 
> This is fine
> 
> > so function probing fails.
> 
> The question is why. Again - sdio_bus_probe should power up the card.
> For some reason, it (or something else) fails to do so with the 8686
> on some setups. Other chips might have similar issues, too - we just
> don't know (all we know is that it works great with the wl12xx, and
> with the Daniel's 8686 setup).
> 
> > So this patch moves the call to pm_runtime_enable to after all the
> > calls to sdio_add_func.
> 
> Looks like this will have the undesired side-effect of keeping the
> chip powered up, even if it doesn't have any driver loaded for it.
> 
> And this doesn't really address the problem: we still don't know why
> the 8686 fails to power up at this point.
> 
> Can you please tell us where exactly is the first failure coming from
> ? is this from libertas own probe function ? is this sdio_bus_probe
> getting an error from calling pm_runtime_get_sync ?
> 
> Thanks,
> Ohad

I'll spend some more time on this and get back to you - probably next week.

Thanks for the hints and perspective.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.

[Daniel Drake]

On Mon, Dec 5, 2011 at 1:06 PM, NeilBrown <neilb@suse.de> wrote:
> The chip has a requirement that the reset line is held down during power-on,
> and raised shortly after (I don't know exactly how short).  So if you just
> remove power and give it back, the chip doesn't come up properly.

In another thread, Marvell confirmed that performing a SDIO reset is
sufficient, no reset line tweaking is needed in the face of a reset.

However, I would certainly advise playing with this reset line at the
point of failure - you could confirm if it is or isn't being tweaked
at this particular point, and if its not, you could try adjusting the
code to manipulate this line at this point to see if it helps at all.

Another area of experimentation is detailed here:
http://article.gmane.org/gmane.linux.kernel.mmc/11475
especially if you can confirm that mmc_send_if_cond is the source of
the failure.

Thanks,
Daniel

Re: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.

[Ohad Ben-Cohen]

On Mon, Dec 5, 2011 at 9:06 PM, NeilBrown <neilb@suse.de> wrote:
> Challenge accepted.

cool :)

> Even if I don't find a better solution, this seems backwards.  Sure the
> default should be that PM is enabled, but individual board can request no
> PM on MMC interfaces where it is a problem.

I somewhat tend to pick a white listing approach, so we don't keep on
inflicting further pain on random sdio users (at least until we learn
more about the issues at hand), but I wouldn't object black listing.

> The chip has a requirement that the reset line is held down during power-on,
> and raised shortly after (I don't know exactly how short).  So if you just
> remove power and give it back, the chip doesn't come up properly.

Well, not according to Bing Zhao from Marvell:

http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg08257.html

Though I agree that if we do find this reset line helping (and get it
to work cleanly with the help of some regulator code), the solution
might be much more robust.

> "It *should be* perfectly fine ..." :-)

heh, yeah :)

> We just have to make sure the bug powers up the card properly.
> Maybe I need to create a virtual regulator that powers on the real regulator,
> then raises the reset line.  I wonder how hard that is.

Btw did you try toggling the reset line manually and see if it really helps ?

> I'll spend some more time on this and get back to you - probably next week.

Very cool, thanks a lot.

Ohad.

Re: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 6496 bytes --]

On Mon, 5 Dec 2011 21:37:07 +0200 Ohad Ben-Cohen <ohad@wizery.com> wrote:

> On Mon, Dec 5, 2011 at 9:06 PM, NeilBrown <neilb@suse.de> wrote:
> > Challenge accepted.
> 
> cool :)
> 
> > Even if I don't find a better solution, this seems backwards.  Sure the
> > default should be that PM is enabled, but individual board can request no
> > PM on MMC interfaces where it is a problem.
> 
> I somewhat tend to pick a white listing approach, so we don't keep on
> inflicting further pain on random sdio users (at least until we learn
> more about the issues at hand), but I wouldn't object black listing.
> 
> > The chip has a requirement that the reset line is held down during power-on,
> > and raised shortly after (I don't know exactly how short).  So if you just
> > remove power and give it back, the chip doesn't come up properly.
> 
> Well, not according to Bing Zhao from Marvell:
> 
> http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg08257.html
> 
> Though I agree that if we do find this reset line helping (and get it
> to work cleanly with the help of some regulator code), the solution
> might be much more robust.
> 
> > "It *should be* perfectly fine ..." :-)
> 
> heh, yeah :)
> 
> > We just have to make sure the bug powers up the card properly.
> > Maybe I need to create a virtual regulator that powers on the real regulator,
> > then raises the reset line.  I wonder how hard that is.
> 
> Btw did you try toggling the reset line manually and see if it really helps ?
> 
> > I'll spend some more time on this and get back to you - probably next week.
> 
> Very cool, thanks a lot.
> 
> Ohad.

Well... I've got something to report.
 

Firstly, this has nothing to do with power-supply to the chip.  The regulator
stays on the whole time and I still need to reset as some point.  However
there is no difference between:
  - power on with reset high
  - power on with reset low, then raise reset.
So what I thought about reset before is wrong as you say.

Secondly, there are two ways that I can get the wifi to work.

1/ If I keep the device from suspending until the functions have been probed,
   it works.

 To expand on this a bit: when I allow it to sleep early I see messages like:

[  315.347930] bus: 'sdio': driver_probe_device: matched device mmc1:0001:1 with driver libertas_sdio
[  315.368713] bus: 'sdio': really_probe: probing driver libertas_sdio with device mmc1:0001:1
[  315.380859] mmc1: mmc_power_restore_host: powering up
[  315.478668] libertas_sdio: probe of mmc1:0001:1 failed with error -16

This error is ultimately caused by mmc_send_io_op_cond called from
mmc_sdio_power_restore returning -110.

When I force it to hold off suspend for a little while I see (starting at the
same place):

[  656.189697] bus: 'sdio': driver_probe_device: matched device mmc1:0001:1 with driver libertas_sdio
[  656.212768] bus: 'sdio': really_probe: probing driver libertas_sdio with device mmc1:0001:1
[  656.247741] libertas_sdio mmc1:0001:1: firmware: requesting libertas/sd8686_v9_helper.bin
[  656.257537] device: 'mmc1:0001:1': device_add
[  656.263580] PM: Adding info for No Bus:mmc1:0001:1
[  656.298187] device: 'mmc1:0001:1': device_unregister
[  656.303375] PM: Removing info for No Bus:mmc1:0001:1
[  656.322967] libertas_sdio mmc1:0001:1: firmware: requesting libertas/sd8686_v8_helper.bin
[  656.344207] device: 'mmc1:0001:1': device_add
[  656.357147] PM: Adding info for No Bus:mmc1:0001:1
[  656.395782] device: 'mmc1:0001:1': device_unregister
[  656.407409] PM: Removing info for No Bus:mmc1:0001:1
[  656.418579] libertas_sdio mmc1:0001:1: firmware: requesting sd8686_helper.bin
[  656.435699] device: 'mmc1:0001:1': device_add
[  656.446655] PM: Adding info for No Bus:mmc1:0001:1
[  656.504974] device: 'mmc1:0001:1': device_unregister
[  656.511749] PM: Removing info for No Bus:mmc1:0001:1
[  656.521148] libertas_sdio mmc1:0001:1: firmware: requesting sd8686.bin
[  656.529205] device: 'mmc1:0001:1': device_add
[  656.535095] PM: Adding info for No Bus:mmc1:0001:1
[  656.584625] device: 'mmc1:0001:1': device_unregister
[  656.591369] PM: Removing info for No Bus:mmc1:0001:1
[  657.384063] libertas_sdio mmc1:0001:1: (unregistered net_device): 00:19:88:3d:ff:f0, fw 9.70.3p24, cap 0x00000303
[  657.454467] libertas_sdio mmc1:0001:1: (unregistered net_device): PREP_CMD: command 0x00a3 failed: 2
[  657.464080] device: 'phy0': device_add


 But shortly there after the extra tracing I put in shows that mmc_power_off
 is called, then mmc_sdio_power_restore calls mmc_send_io_op_cond which again
 returns -110, but now it isn't a problem and the wifi chip keeps working.
 So maybe the fact that we error-out in the first case is a problem??


 I found that if I pull the reset line down and then let it back up then it
 all works.  mmc_send_io_op_cond never returns -110.
 Specifically I first put the reset (
	gpio_set_value_cansleep(212, 0);
	mmc_delay(10);
	gpio_set_value_cansleep(212, 1);
	mmc_delay(10);

  ) in mmc_power_up just before the final "mmc_host_clk_release(host);"
  That worked.

  I then tried to be clever and defined a 'remux' routine (called 
  from hsmmc23_before_set_reg
  from omap_hsmmc_235_set_power
  from omap_hsmmc_set_io
  from mmc_power_up), and that works too.

  So that allows me to reset the wifi chip at the right time with only
  changes to my 'board' file.

  However I don't think a reset was the intention of 'remux' so this solution
  feels a little dirty.

My thought is that something in the suspend/resume sequence is confusing the
wifi chip and it needs a reset to get out of it.
However if I just make mmc_power_off a no-op (insert an early 'return') it
still fails.  I tried poking around and commenting out other function calls
but only ever manage to break it more, never to fix it :-(

So I have run out of ideas.  I can make it work by reseting the chip during
mmc_power_up but I have no idea what is causing the chip to need a reset.
All I know is that it isn't the power-supply being removed.

If you or anyone would like to suggest anything else I could test, or
any interesting information that I could extract that might be helpful, then
I'm happy to give that a go (with the understanding that the turn-around time
could easily be a week or more).

However for now I think I'll go with my 'remux' hack.

Thanks,
NeilBrown



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.

[Ohad Ben-Cohen]

On Sat, Dec 10, 2011 at 9:15 AM, NeilBrown <neilb@suse.de> wrote:
> When I force it to hold off suspend for a little while I see (starting at the
> same place):
>
> [  656.189697] bus: 'sdio': driver_probe_device: matched device mmc1:0001:1 with driver libertas_sdio
> [  656.212768] bus: 'sdio': really_probe: probing driver libertas_sdio with device mmc1:0001:1
> [  656.247741] libertas_sdio mmc1:0001:1: firmware: requesting libertas/sd8686_v9_helper.bin
> [  656.257537] device: 'mmc1:0001:1': device_add
> [  656.263580] PM: Adding info for No Bus:mmc1:0001:1
> [  656.298187] device: 'mmc1:0001:1': device_unregister
> [  656.303375] PM: Removing info for No Bus:mmc1:0001:1
> [  656.322967] libertas_sdio mmc1:0001:1: firmware: requesting libertas/sd8686_v8_helper.bin
> [  656.344207] device: 'mmc1:0001:1': device_add
> [  656.357147] PM: Adding info for No Bus:mmc1:0001:1
> [  656.395782] device: 'mmc1:0001:1': device_unregister
> [  656.407409] PM: Removing info for No Bus:mmc1:0001:1
> [  656.418579] libertas_sdio mmc1:0001:1: firmware: requesting sd8686_helper.bin
> [  656.435699] device: 'mmc1:0001:1': device_add
> [  656.446655] PM: Adding info for No Bus:mmc1:0001:1
> [  656.504974] device: 'mmc1:0001:1': device_unregister
> [  656.511749] PM: Removing info for No Bus:mmc1:0001:1
> [  656.521148] libertas_sdio mmc1:0001:1: firmware: requesting sd8686.bin
> [  656.529205] device: 'mmc1:0001:1': device_add
> [  656.535095] PM: Adding info for No Bus:mmc1:0001:1
> [  656.584625] device: 'mmc1:0001:1': device_unregister
> [  656.591369] PM: Removing info for No Bus:mmc1:0001:1
> [  657.384063] libertas_sdio mmc1:0001:1: (unregistered net_device): 00:19:88:3d:ff:f0, fw 9.70.3p24, cap 0x00000303
> [  657.454467] libertas_sdio mmc1:0001:1: (unregistered net_device): PREP_CMD: command 0x00a3 failed: 2
> [  657.464080] device: 'phy0': device_add
>
>
>  But shortly there after the extra tracing I put in shows that mmc_power_off
>  is called,

Probably right after libertas' if_sdio_probe() returns ?

> then mmc_sdio_power_restore calls mmc_send_io_op_cond

Is that as a result of libertas' if_sdio_power_restore() being called
(i.e. someone/something called 'ifconfig up') ?

> which again
>  returns -110, but now it isn't a problem and the wifi chip keeps working.

if_sdio_power_restore doesn't check the return value of
pm_runtime_get_sync, so it won't error out, but I wonder how come the
chip still works.

>  So maybe the fact that we error-out in the first case is a problem??

It might be nice if Marvell could comment on this, though we can
probably empirically deduce this too.

>  I found that if I pull the reset line down and then let it back up then it
>  all works.

Nice. Joe, did this work out for you too ?

> So I have run out of ideas.  I can make it work by reseting the chip during
> mmc_power_up but I have no idea what is causing the chip to need a reset.

I wonder why the sdio reset command isn't helpful for you - it did
seem to resolve some issues for Daniel. Maybe you have two different
hardware revisions of the 8686 which behave differently in this
respect ?

> However for now I think I'll go with my 'remux' hack.

Feel free to post it so we can take a look.

Thanks,
Ohad.

RE: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.

[Bing Zhao]

Hi Ohad,

> >  But shortly there after the extra tracing I put in shows that mmc_power_off
> >  is called,
> 
> Probably right after libertas' if_sdio_probe() returns ?
> 
> > then mmc_sdio_power_restore calls mmc_send_io_op_cond
> 
> Is that as a result of libertas' if_sdio_power_restore() being called
> (i.e. someone/something called 'ifconfig up') ?
> 
> > which again
> >  returns -110, but now it isn't a problem and the wifi chip keeps working.
> 
> if_sdio_power_restore doesn't check the return value of
> pm_runtime_get_sync, so it won't error out, but I wonder how come the
> chip still works.
> 
> >  So maybe the fact that we error-out in the first case is a problem??
> 
> It might be nice if Marvell could comment on this, though we can
> probably empirically deduce this too.

We will get CMD5 R4 only after a PDn (power down line) or RESETn (reset line) toggling.

Was it timed out on CMD5 or CMD52? The 2nd enumeration will result CMD5 timeout if SD8686 has been initialized (with CMD5,5,3,7 sequence) already. The 2nd CMD5 timeout should not cause any side effect though.

> 
> >  I found that if I pull the reset line down and then let it back up then it
> >  all works.
> 
> Nice. Joe, did this work out for you too ?
> 
> > So I have run out of ideas.  I can make it work by reseting the chip during
> > mmc_power_up but I have no idea what is causing the chip to need a reset.
> 
> I wonder why the sdio reset command isn't helpful for you - it did
> seem to resolve some issues for Daniel. Maybe you have two different
> hardware revisions of the 8686 which behave differently in this
> respect ?

CMD52 Card Reset (writing bit3=1 to CCCR 0x06) may not reset the SD8686 properly for some systems. The reliable way to reset SD8686 is via RESETn toggling.

Regards,
Bing

> 
> > However for now I think I'll go with my 'remux' hack.
> 
> Feel free to post it so we can take a look.
> 
> Thanks,
> Ohad.

Re: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 8881 bytes --]

On Sun, 11 Dec 2011 09:30:48 +0200 Ohad Ben-Cohen <ohad@wizery.com> wrote:

> On Sat, Dec 10, 2011 at 9:15 AM, NeilBrown <neilb@suse.de> wrote:
> > When I force it to hold off suspend for a little while I see (starting at the
> > same place):
> >
> > [  656.189697] bus: 'sdio': driver_probe_device: matched device mmc1:0001:1 with driver libertas_sdio
> > [  656.212768] bus: 'sdio': really_probe: probing driver libertas_sdio with device mmc1:0001:1
> > [  656.247741] libertas_sdio mmc1:0001:1: firmware: requesting libertas/sd8686_v9_helper.bin
> > [  656.257537] device: 'mmc1:0001:1': device_add
> > [  656.263580] PM: Adding info for No Bus:mmc1:0001:1
> > [  656.298187] device: 'mmc1:0001:1': device_unregister
> > [  656.303375] PM: Removing info for No Bus:mmc1:0001:1
> > [  656.322967] libertas_sdio mmc1:0001:1: firmware: requesting libertas/sd8686_v8_helper.bin
> > [  656.344207] device: 'mmc1:0001:1': device_add
> > [  656.357147] PM: Adding info for No Bus:mmc1:0001:1
> > [  656.395782] device: 'mmc1:0001:1': device_unregister
> > [  656.407409] PM: Removing info for No Bus:mmc1:0001:1
> > [  656.418579] libertas_sdio mmc1:0001:1: firmware: requesting sd8686_helper.bin
> > [  656.435699] device: 'mmc1:0001:1': device_add
> > [  656.446655] PM: Adding info for No Bus:mmc1:0001:1
> > [  656.504974] device: 'mmc1:0001:1': device_unregister
> > [  656.511749] PM: Removing info for No Bus:mmc1:0001:1
> > [  656.521148] libertas_sdio mmc1:0001:1: firmware: requesting sd8686.bin
> > [  656.529205] device: 'mmc1:0001:1': device_add
> > [  656.535095] PM: Adding info for No Bus:mmc1:0001:1
> > [  656.584625] device: 'mmc1:0001:1': device_unregister
> > [  656.591369] PM: Removing info for No Bus:mmc1:0001:1
> > [  657.384063] libertas_sdio mmc1:0001:1: (unregistered net_device): 00:19:88:3d:ff:f0, fw 9.70.3p24, cap 0x00000303
> > [  657.454467] libertas_sdio mmc1:0001:1: (unregistered net_device): PREP_CMD: command 0x00a3 failed: 2
> > [  657.464080] device: 'phy0': device_add
> >
> >
> >  But shortly there after the extra tracing I put in shows that mmc_power_off
> >  is called,
> 
> Probably right after libertas' if_sdio_probe() returns ?

Sort-of.  It is actually called from pm_runtime_work so it is an async
pm_suspend.

It is triggered by the pm_runtime_put_sync() call in driver_probe_device()
just after really_probe() is called, and it was really_probe() that ultimately
called if_sdio_probe().  So depending on what you mean by "right after"
exactly, that might describe when it happened.

> 
> > then mmc_sdio_power_restore calls mmc_send_io_op_cond
> 
> Is that as a result of libertas' if_sdio_power_restore() being called
> (i.e. someone/something called 'ifconfig up') ?

Probably.  It is definitely if_sdio_power_restore() being called because
something did an ioctl, and that was probably wpa_supplicant trying to
configure the device.

> 
> > which again
> >  returns -110, but now it isn't a problem and the wifi chip keeps working.
> 
> if_sdio_power_restore doesn't check the return value of
> pm_runtime_get_sync, so it won't error out, but I wonder how come the
> chip still works.

That bit is still a mystery.  I don't think it always works, but certainly
does some times.

I put some tracing in mmc_wait_for_command and see:

[   69.939819] mmc_wait_for_cmd 52 -> -110
[   69.944274] mmc_wait_for_cmd 52 -> -110
[   69.949066] mmc_wait_for_cmd 0 -> 0
[   69.954254] mmc_wait_for_cmd 8 -> -110
[   69.958679] mmc_wait_for_cmd 5 -> 0
[   69.962707] mmc_wait_for_cmd 5 -> 0
[   69.966827] mmc_wait_for_cmd 3 -> 0
[   69.970886] mmc_wait_for_cmd 7 -> 0
[   69.974914] mmc_wait_for_cmd 52 -> 0
[   69.979064] mmc_wait_for_cmd 52 -> 0
[   69.983154] mmc_wait_for_cmd 52 -> 0
[   69.987274] mmc_wait_for_cmd 52 -> 0

When I first try to start the wifi (turn on the regulator and assert the
card-detect).

 	mmc_wait_for_req(host, &mrq);
+	if (host->index == 1)
+		printk("mmc_wait_for_cmd %d -> %d\n", cmd->opcode, cmd->error);

That last line repeats every few millisecs until

[   79.444793] mmc_wait_for_cmd 52 -> 0

about 10 seconds later (1777 repeats) when it stops.

Then when I start up wpa_supplicant I see


[  132.313293] mmc_wait_for_cmd 52 -> 0
[  132.317443] mmc_wait_for_cmd 52 -> 0
[  132.321990] mmc_wait_for_cmd 0 -> 0
[  132.327239] mmc_wait_for_cmd 8 -> -110
[  132.332824] mmc_wait_for_cmd 5 -> -110
[  132.337280] mmc_wait_for_cmd 52 -> 0
[  132.341400] mmc_wait_for_cmd 52 -> 0
[  132.345520] mmc_wait_for_cmd 52 -> 0
[  132.349639] mmc_wait_for_cmd 52 -> 0
[  132.353759] mmc_wait_for_cmd 52 -> 0

And the "52 -> 0" repeats indefinitely until I reboot.  But at this point
wifi is working - I get a dhcp reply and can ping hosts.


In the other case where I do let it suspend early (And it never recovers
without the reset line being toggled) I see:

[ 2170.100982] mmc_wait_for_cmd 52 -> -110
[ 2170.105407] mmc_wait_for_cmd 52 -> -110
[ 2170.110260] mmc_wait_for_cmd 0 -> 0
[ 2170.115509] mmc_wait_for_cmd 8 -> -110
[ 2170.119842] mmc_wait_for_cmd 5 -> 0
[ 2170.123901] mmc_wait_for_cmd 5 -> 0
[ 2170.127929] mmc_wait_for_cmd 3 -> 0
[ 2170.131958] mmc_wait_for_cmd 7 -> 0
[ 2170.135986] mmc_wait_for_cmd 52 -> 0
[ 2170.140136] mmc_wait_for_cmd 52 -> 0
[ 2170.144226] mmc_wait_for_cmd 52 -> 0
[ 2170.148376] mmc_wait_for_cmd 52 -> 0
[ 2170.152465] mmc_wait_for_cmd 52 -> 0


which is much the same but then one second later:

[ 2171.166656] mmc_wait_for_cmd 52 -> 0
[ 2171.170806] mmc_wait_for_cmd 52 -> 0
[ 2171.175384] mmc_wait_for_cmd 0 -> 0
[ 2171.180603] mmc_wait_for_cmd 8 -> -110
[ 2171.185943] mmc_wait_for_cmd 5 -> -110
[ 2171.190093] libertas_sdio: probe of mmc1:0001:1 failed with error -16


So this answers Bing Zhao's question - it was CMD5 that timed out and caused
the failure.

If I just get mmc_wait_for_cmd to hide error -110 for cmd 5 it complains

[   26.420440] mmc1: host doesn't support card's voltages

so that isn't a simple solution :-)


> 
> >  So maybe the fact that we error-out in the first case is a problem??
> 
> It might be nice if Marvell could comment on this, though we can
> probably empirically deduce this too.
> 
> >  I found that if I pull the reset line down and then let it back up then it
> >  all works.
> 
> Nice. Joe, did this work out for you too ?
> 
> > So I have run out of ideas.  I can make it work by reseting the chip during
> > mmc_power_up but I have no idea what is causing the chip to need a reset.
> 
> I wonder why the sdio reset command isn't helpful for you - it did
> seem to resolve some issues for Daniel. Maybe you have two different
> hardware revisions of the 8686 which behave differently in this
> respect ?
> 
> > However for now I think I'll go with my 'remux' hack.
> 
> Feel free to post it so we can take a look.
> 
> Thanks,
> Ohad.

It isn't very exciting but I include it below.

The change is entirely inside my 'board' file which isn't in mainline.

I define a 'remux' function  that lowers the reset line for 10msec and raises
it again. (I don't know if the 10msec is needed).
tca_setup() is a callback from the driver that enables the gpio line (it is
one pin on an LED driver chip).

NeilBrown



diff --git a/arch/arm/mach-omap2/board-omap3gta04.c b/arch/arm/mach-omap2/board-omap3gta04.c
index 8a09b98..6a13822 100644
--- a/arch/arm/mach-omap2/board-omap3gta04.c
+++ b/arch/arm/mach-omap2/board-omap3gta04.c
@@ -357,6 +357,8 @@ static struct regulator_consumer_supply gta04_vdvi_supply = {
 
 #include "sdram-micron-mt46h32m32lf-6.h"
 
+static void wlan_reset(struct device *dev, int slow, int power_on);
+
 static struct omap2_hsmmc_info mmc[] = {
 	{
 		.mmc		= 1,
@@ -370,6 +372,7 @@ static struct omap2_hsmmc_info mmc[] = {
 		.gpio_cd	= -EINVAL, // virtual card detect
 		.gpio_wp	= -EINVAL,	// no write protect
 		.transceiver	= true,	// external transceiver
+		.remux		= wlan_reset,
 // 		.ocr_mask	= 0x00100000,	/* fixed 3.3V */
 	},
 	{}	/* Terminator */
@@ -769,11 +772,22 @@ struct bmp085_platform_data bmp085_info = {
 /* "+2" because TWL4030 adds 2 LED drives as gpio outputs */
 #define GPIO_WIFI_RESET (OMAP_MAX_GPIO_LINES + TWL4030_GPIO_MAX + 2)
 
+static int reset_ready;
+static void wlan_reset(struct device *dev, int slow, int power_on)
+{
+	if (power_on && reset_ready) {
+		gpio_set_value(GPIO_WIFI_RESET, 0);
+		msleep(10);
+		gpio_set_value(GPIO_WIFI_RESET, 1);
+	}
+}
+
 void tca_setup(unsigned gpio_base, unsigned ngpio)
 {
 	gpio_request(GPIO_WIFI_RESET, "WIFI_RESET");
 	gpio_direction_output(GPIO_WIFI_RESET, true);
 	gpio_export(GPIO_WIFI_RESET, 0);
+	reset_ready = 1;
 }
 
 static struct led_info tca6507_leds[] = {


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

RE: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.

[Bing Zhao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1775 bytes --]

Hi Neil,

> In the other case where I do let it suspend early (And it never recovers
> without the reset line being toggled) I see:
> 
> [ 2170.100982] mmc_wait_for_cmd 52 -> -110
> [ 2170.105407] mmc_wait_for_cmd 52 -> -110
> [ 2170.110260] mmc_wait_for_cmd 0 -> 0
> [ 2170.115509] mmc_wait_for_cmd 8 -> -110
> [ 2170.119842] mmc_wait_for_cmd 5 -> 0
> [ 2170.123901] mmc_wait_for_cmd 5 -> 0
> [ 2170.127929] mmc_wait_for_cmd 3 -> 0
> [ 2170.131958] mmc_wait_for_cmd 7 -> 0

This works when reset line toggling was applied.

> [ 2170.135986] mmc_wait_for_cmd 52 -> 0
> [ 2170.140136] mmc_wait_for_cmd 52 -> 0
> [ 2170.144226] mmc_wait_for_cmd 52 -> 0
> [ 2170.148376] mmc_wait_for_cmd 52 -> 0
> [ 2170.152465] mmc_wait_for_cmd 52 -> 0
> 
> 
> which is much the same but then one second later:
> 
> [ 2171.166656] mmc_wait_for_cmd 52 -> 0
> [ 2171.170806] mmc_wait_for_cmd 52 -> 0
> [ 2171.175384] mmc_wait_for_cmd 0 -> 0
> [ 2171.180603] mmc_wait_for_cmd 8 -> -110
> [ 2171.185943] mmc_wait_for_cmd 5 -> -110

Here the CMD5 timeout is expected because SD8686 has already been initialized with CMD5,5,3,7.
If you are able to skip re-initialization at this point, like what "powered_resume" does, you will probably get SD8686 continue to run.

Regards,
Bing

> [ 2171.190093] libertas_sdio: probe of mmc1:0001:1 failed with error -16
> 
> 
> So this answers Bing Zhao's question - it was CMD5 that timed out and caused
> the failure.
> 
> If I just get mmc_wait_for_cmd to hide error -110 for cmd 5 it complains
> 
> [   26.420440] mmc1: host doesn't support card's voltages
> 
> so that isn't a simple solution :-)

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2900 bytes --]

On Thu, 15 Dec 2011 20:39:28 -0800 Bing Zhao <bzhao@marvell.com> wrote:

> Hi Neil,
> 
> > In the other case where I do let it suspend early (And it never recovers
> > without the reset line being toggled) I see:
> > 
> > [ 2170.100982] mmc_wait_for_cmd 52 -> -110
> > [ 2170.105407] mmc_wait_for_cmd 52 -> -110
> > [ 2170.110260] mmc_wait_for_cmd 0 -> 0
> > [ 2170.115509] mmc_wait_for_cmd 8 -> -110
> > [ 2170.119842] mmc_wait_for_cmd 5 -> 0
> > [ 2170.123901] mmc_wait_for_cmd 5 -> 0
> > [ 2170.127929] mmc_wait_for_cmd 3 -> 0
> > [ 2170.131958] mmc_wait_for_cmd 7 -> 0
> 
> This works when reset line toggling was applied.

Yep.

> 
> > [ 2170.135986] mmc_wait_for_cmd 52 -> 0
> > [ 2170.140136] mmc_wait_for_cmd 52 -> 0
> > [ 2170.144226] mmc_wait_for_cmd 52 -> 0
> > [ 2170.148376] mmc_wait_for_cmd 52 -> 0
> > [ 2170.152465] mmc_wait_for_cmd 52 -> 0
> > 
> > 
> > which is much the same but then one second later:
> > 
> > [ 2171.166656] mmc_wait_for_cmd 52 -> 0
> > [ 2171.170806] mmc_wait_for_cmd 52 -> 0
> > [ 2171.175384] mmc_wait_for_cmd 0 -> 0
> > [ 2171.180603] mmc_wait_for_cmd 8 -> -110
> > [ 2171.185943] mmc_wait_for_cmd 5 -> -110
> 
> Here the CMD5 timeout is expected because SD8686 has already been initialized with CMD5,5,3,7.
> If you are able to skip re-initialization at this point, like what "powered_resume" does, you will probably get SD8686 continue to run.
> 

OK... I think we are nearly there.

If I configure the regulator to turn off on suspend and back on for resume, I
don't need to toggle the reset line.  I just works.
Previously I had the regulator permanently on.
So it seems we need one of:
  - remove/restore power
  - toggle reset
  - don't sent the CMD5,5,3,7 sequence again

I can neither guarantee that the regulator will be powered off or
that it will stay on.  That regulator is shared with bluetooth (on the same
package and wired together) so if the bluetooth is inactive the regulator
will drop-out when the wifi is suspended, but if bluetooth is active it won't.

What I could do is create a 'gpio-regulator' which is feed by the real
regulator and give it the reset line as its gpio.
Then when the wifi enters pm_suspend it will reset the wifi chip and release
the regulator which will power down if bluetooth is inactive.  When it
pm_resumes the regulator will be powered up and the reset line released.
(at least, that is the  theory).

So that will work for me, but I suspect more people will fall into this trap.
Is there some way would could detect this particular situation from the
libertas driver and print a message about needing a hard reset or a power off,
and pointing to documentation.

Or maybe the mmc layer could somehow ask if a reset is needed and not bother
if the device doesn't seem to have been powered down.
Just guessing here.


Thanks,
NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] mmc/sdio: don't allow interface to runtime-suspend until probe is finished.

[Ohad Ben-Cohen]

Hello Bing,

On Thu, Dec 15, 2011 at 9:35 AM, Bing Zhao <bzhao@marvell.com> wrote:
>> I wonder why the sdio reset command isn't helpful for you - it did
>> seem to resolve some issues for Daniel. Maybe you have two different
>> hardware revisions of the 8686 which behave differently in this
>> respect ?
>
> CMD52 Card Reset (writing bit3=1 to CCCR 0x06) may not reset the SD8686 properly for some systems. The reliable way to reset SD8686 is via RESETn toggling.

Thanks a lot for confirming this.

Ohad.