[PATCH] wlcore: sdio: Fixup power on/off sequence

[PATCH] wlcore: sdio: Fixup power on/off sequence

[Ulf Hansson]

During "wlan-up", we are programming the FW into the WiFi-chip. However,
re-programming the FW doesn't work, unless a power cycle of the WiFi-chip
is made in-between the programmings.

To conform to this requirement and to fix the regression in a simple way,
let's start by allowing that the SDIO card (WiFi-chip) may stay powered on
(runtime resumed) when wl12xx_sdio_power_off() returns. The intent with the
current code is to treat this scenario as an error, but unfortunate this
doesn't work as expected, so let's fix this.

The other part is to guarantee that a power cycle of the SDIO card has been
completed when wl12xx_sdio_power_on() returns, as to allow the FW
programming to succeed. However, relying solely on runtime PM to deal with
this isn't sufficient. For example, userspace may prevent runtime suspend
via sysfs for the device that represents the SDIO card, leading to that the
mmc core also keeps it powered on. For this reason, let's instead do a
brute force power cycle in wl12xx_sdio_power_on().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/net/wireless/ti/wlcore/sdio.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c
index bd10165d7eec..bc73085fe806 100644
--- a/drivers/net/wireless/ti/wlcore/sdio.c
+++ b/drivers/net/wireless/ti/wlcore/sdio.c
@@ -163,6 +163,12 @@ static int wl12xx_sdio_power_on(struct wl12xx_sdio_glue *glue)
 		return ret;
 	}
 
+	/*
+	 * To guarantee that the SDIO card is power cycled, as required to make
+	 * the FW programming to succeed, let's do a brute force HW reset.
+	 */
+	mmc_hw_reset(card->host);
+
 	sdio_claim_host(func);
 	sdio_enable_func(func);
 	sdio_release_host(func);
@@ -174,20 +180,13 @@ static int wl12xx_sdio_power_off(struct wl12xx_sdio_glue *glue)
 {
 	struct sdio_func *func = dev_to_sdio_func(glue->dev);
 	struct mmc_card *card = func->card;
-	int error;
 
 	sdio_claim_host(func);
 	sdio_disable_func(func);
 	sdio_release_host(func);
 
 	/* Let runtime PM know the card is powered off */
-	error = pm_runtime_put(&card->dev);
-	if (error < 0 && error != -EBUSY) {
-		dev_err(&card->dev, "%s failed: %i\n", __func__, error);
-
-		return error;
-	}
-
+	pm_runtime_put(&card->dev);
 	return 0;
 }
 
-- 
2.17.1

Re: [PATCH] wlcore: sdio: Fixup power on/off sequence

[Tony Lindgren]

Hi,

* Ulf Hansson <ulf.hansson@linaro.org> [190115 15:04]:
> During "wlan-up", we are programming the FW into the WiFi-chip. However,
> re-programming the FW doesn't work, unless a power cycle of the WiFi-chip
> is made in-between the programmings.
> 
> To conform to this requirement and to fix the regression in a simple way,
> let's start by allowing that the SDIO card (WiFi-chip) may stay powered on
> (runtime resumed) when wl12xx_sdio_power_off() returns. The intent with the
> current code is to treat this scenario as an error, but unfortunate this
> doesn't work as expected, so let's fix this.
> 
> The other part is to guarantee that a power cycle of the SDIO card has been
> completed when wl12xx_sdio_power_on() returns, as to allow the FW
> programming to succeed. However, relying solely on runtime PM to deal with
> this isn't sufficient. For example, userspace may prevent runtime suspend
> via sysfs for the device that represents the SDIO card, leading to that the
> mmc core also keeps it powered on. For this reason, let's instead do a
> brute force power cycle in wl12xx_sdio_power_on().

Thanks this looks good.. But looks like udelay 20000
is needed with:

# while [ 1 ]; do ifconfig wlan0 down; usleep 20000; \
  ifconfig wlan0 up; done

Otherwise I get the following on warning pandaboard-es:

WARNING: CPU: 0 PID: 1770 at drivers/bus/omap_l3_noc.c:147
l3_interrupt_handler+0x2f8/0x388
44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4PER2 (Read):
Data Access in User mode during Functional access

Meaning that we access registers while not clocked
somewhere. I think that warning is different from the
earlier errors though :) And we could add a msleep(50)
to the end to mostly avoid it until we have a better
fix available?

Regards,

Tony

Re: [PATCH] wlcore: sdio: Fixup power on/off sequence

[Ulf Hansson]

On Tue, 15 Jan 2019 at 19:55, Tony Lindgren <tony@atomide.com> wrote:
>
> Hi,
>
> * Ulf Hansson <ulf.hansson@linaro.org> [190115 15:04]:
> > During "wlan-up", we are programming the FW into the WiFi-chip. However,
> > re-programming the FW doesn't work, unless a power cycle of the WiFi-chip
> > is made in-between the programmings.
> >
> > To conform to this requirement and to fix the regression in a simple way,
> > let's start by allowing that the SDIO card (WiFi-chip) may stay powered on
> > (runtime resumed) when wl12xx_sdio_power_off() returns. The intent with the
> > current code is to treat this scenario as an error, but unfortunate this
> > doesn't work as expected, so let's fix this.
> >
> > The other part is to guarantee that a power cycle of the SDIO card has been
> > completed when wl12xx_sdio_power_on() returns, as to allow the FW
> > programming to succeed. However, relying solely on runtime PM to deal with
> > this isn't sufficient. For example, userspace may prevent runtime suspend
> > via sysfs for the device that represents the SDIO card, leading to that the
> > mmc core also keeps it powered on. For this reason, let's instead do a
> > brute force power cycle in wl12xx_sdio_power_on().
>
> Thanks this looks good.. But looks like udelay 20000
> is needed with:

Thanks for testing!

>
> # while [ 1 ]; do ifconfig wlan0 down; usleep 20000; \
>   ifconfig wlan0 up; done
>
> Otherwise I get the following on warning pandaboard-es:
>
> WARNING: CPU: 0 PID: 1770 at drivers/bus/omap_l3_noc.c:147
> l3_interrupt_handler+0x2f8/0x388
> 44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4PER2 (Read):
> Data Access in User mode during Functional access
>
> Meaning that we access registers while not clocked
> somewhere. I think that warning is different from the
> earlier errors though :) And we could add a msleep(50)
> to the end to mostly avoid it until we have a better
> fix available?

Well, that would paper over the problem, let's see if we can avoid it.

I realized that I have not invoked mmc_hw_reset() with the sdio host
claimed. This could be the reason and is fixed in a v2, please try and
see if that solves the "new" problem. If id doesn't, you could also
convert the pm_runtime_put() to a pm_runtime_put_sync() in
wl12xx_sdio_power_off() and see if that makes a difference.

Kind regards
Uffe

Re: [PATCH] wlcore: sdio: Fixup power on/off sequence

[Tony Lindgren]

* Ulf Hansson <ulf.hansson@linaro.org> [190116 11:44]:
> On Tue, 15 Jan 2019 at 19:55, Tony Lindgren <tony@atomide.com> wrote:
> > # while [ 1 ]; do ifconfig wlan0 down; usleep 20000; \
> >   ifconfig wlan0 up; done
> >
> > Otherwise I get the following on warning pandaboard-es:
> >
> > WARNING: CPU: 0 PID: 1770 at drivers/bus/omap_l3_noc.c:147
> > l3_interrupt_handler+0x2f8/0x388
> > 44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4PER2 (Read):
> > Data Access in User mode during Functional access
> >
> > Meaning that we access registers while not clocked
> > somewhere. I think that warning is different from the
> > earlier errors though :) And we could add a msleep(50)
> > to the end to mostly avoid it until we have a better
> > fix available?
> 
> Well, that would paper over the problem, let's see if we can avoid it.
> 
> I realized that I have not invoked mmc_hw_reset() with the sdio host
> claimed. This could be the reason and is fixed in a v2, please try and
> see if that solves the "new" problem. If id doesn't, you could also
> convert the pm_runtime_put() to a pm_runtime_put_sync() in
> wl12xx_sdio_power_off() and see if that makes a difference.

Yes you're right v2 works for me no problem. I'll go ack
it there.

Tony