[PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[Tony Lindgren]

At least HiKey board can fail to bring up wireless interface again
if the interface is brought down and up with no delay inbetween.

This can be done tested with:

# while true; do ifconfig wlan0 down; ifconfig wlan0 up; done

According to Ricardo Salveti <rsalveti@rsalveti.net>, we need to
wait between 30 - 40 ms on HiKey. This seems to happen when we
get -EBUSY returned by pm_runtime_put() basedon the check in
rpm_check_suspend_allowed(). But as it still unclear if part of
the delay needed is because of capacitance, let's always do a
at least 50 ms msleep even if no -EBUSY is returned.

Cc: Anders Roxell <anders.roxell@linaro.org>
Cc: Eyal Reizer <eyalr@ti.com>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Ricardo Salveti <rsalveti@rsalveti.net>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Reported-by: Ricardo Salveti <rsalveti@rsalveti.net>
Fixes: 60f36637bbbd ("wlcore: sdio: allow pm to handle sdio power")
Signed-off-by: Tony Lindgren <tony@atomide.com>
---

Uffe, do you have some better ideas on how to fix this issue?

---
 drivers/net/wireless/ti/wlcore/sdio.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c
--- a/drivers/net/wireless/ti/wlcore/sdio.c
+++ b/drivers/net/wireless/ti/wlcore/sdio.c
@@ -174,7 +174,7 @@ 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;
+	int error, retries = 5;
 
 	sdio_claim_host(func);
 	sdio_disable_func(func);
@@ -188,6 +188,17 @@ static int wl12xx_sdio_power_off(struct wl12xx_sdio_glue *glue)
 		return error;
 	}
 
+	/* Wait a bit to ensure the card gets power off. Otherwise
+	 * bringing interface down and up again may not power off the
+	 * card inbetween. And then firmware load will fail on trying
+	 * to bring the card up again. We need to wait between 30 - 40
+	 * ms for this based on measurements on HiKey board.
+	 */
+	do {
+		msleep(50);
+	} while (error == -EBUSY && !pm_runtime_suspended(&card->dev) &&
+		 retries--);
+
 	return 0;
 }
 
-- 
2.19.2

Re: [PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[Ricardo Salveti]

Hi,

On Mon, Dec 17, 2018 at 2:42 PM Tony Lindgren <tony@atomide.com> wrote:
>
> At least HiKey board can fail to bring up wireless interface again
> if the interface is brought down and up with no delay inbetween.
>
> This can be done tested with:
>
> # while true; do ifconfig wlan0 down; ifconfig wlan0 up; done
>
> According to Ricardo Salveti <rsalveti@rsalveti.net>, we need to
> wait between 30 - 40 ms on HiKey. This seems to happen when we
> get -EBUSY returned by pm_runtime_put() basedon the check in
> rpm_check_suspend_allowed(). But as it still unclear if part of
> the delay needed is because of capacitance, let's always do a
> at least 50 ms msleep even if no -EBUSY is returned.
>
> Cc: Anders Roxell <anders.roxell@linaro.org>
> Cc: Eyal Reizer <eyalr@ti.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Ricardo Salveti <rsalveti@rsalveti.net>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Reported-by: Ricardo Salveti <rsalveti@rsalveti.net>
> Fixes: 60f36637bbbd ("wlcore: sdio: allow pm to handle sdio power")
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>
> Uffe, do you have some better ideas on how to fix this issue?
>
> ---
>  drivers/net/wireless/ti/wlcore/sdio.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c
> --- a/drivers/net/wireless/ti/wlcore/sdio.c
> +++ b/drivers/net/wireless/ti/wlcore/sdio.c
> @@ -174,7 +174,7 @@ 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;
> +       int error, retries = 5;
>
>         sdio_claim_host(func);
>         sdio_disable_func(func);
> @@ -188,6 +188,17 @@ static int wl12xx_sdio_power_off(struct wl12xx_sdio_glue *glue)
>                 return error;
>         }
>
> +       /* Wait a bit to ensure the card gets power off. Otherwise
> +        * bringing interface down and up again may not power off the
> +        * card inbetween. And then firmware load will fail on trying
> +        * to bring the card up again. We need to wait between 30 - 40
> +        * ms for this based on measurements on HiKey board.
> +        */
> +       do {
> +               msleep(50);
> +       } while (error == -EBUSY && !pm_runtime_suspended(&card->dev) &&
> +                retries--);
> +
>         return 0;
>  }
>
> --
> 2.19.2

Tested with 'while true; do ifconfig wlan0 down; ifconfig wlan0 up;
done' for 3 hours on HiKey, HiKey960 and BeagleBone Black Wireless, no
issues.

Tested-by: Ricardo Salveti <rsalveti@rsalveti.net>

Thanks,
-- 
Ricardo Salveti de Araujo

Re: [PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[Ulf Hansson]

On Mon, 17 Dec 2018 at 17:42, Tony Lindgren <tony@atomide.com> wrote:
>
> At least HiKey board can fail to bring up wireless interface again
> if the interface is brought down and up with no delay inbetween.
>
> This can be done tested with:
>
> # while true; do ifconfig wlan0 down; ifconfig wlan0 up; done
>
> According to Ricardo Salveti <rsalveti@rsalveti.net>, we need to
> wait between 30 - 40 ms on HiKey. This seems to happen when we
> get -EBUSY returned by pm_runtime_put() basedon the check in
> rpm_check_suspend_allowed(). But as it still unclear if part of
> the delay needed is because of capacitance, let's always do a
> at least 50 ms msleep even if no -EBUSY is returned.
>
> Cc: Anders Roxell <anders.roxell@linaro.org>
> Cc: Eyal Reizer <eyalr@ti.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Ricardo Salveti <rsalveti@rsalveti.net>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Reported-by: Ricardo Salveti <rsalveti@rsalveti.net>
> Fixes: 60f36637bbbd ("wlcore: sdio: allow pm to handle sdio power")
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>
> Uffe, do you have some better ideas on how to fix this issue?
>
> ---
>  drivers/net/wireless/ti/wlcore/sdio.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c
> --- a/drivers/net/wireless/ti/wlcore/sdio.c
> +++ b/drivers/net/wireless/ti/wlcore/sdio.c
> @@ -174,7 +174,7 @@ 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;
> +       int error, retries = 5;
>
>         sdio_claim_host(func);
>         sdio_disable_func(func);
> @@ -188,6 +188,17 @@ static int wl12xx_sdio_power_off(struct wl12xx_sdio_glue *glue)
>                 return error;
>         }
>
> +       /* Wait a bit to ensure the card gets power off. Otherwise
> +        * bringing interface down and up again may not power off the
> +        * card inbetween. And then firmware load will fail on trying
> +        * to bring the card up again. We need to wait between 30 - 40
> +        * ms for this based on measurements on HiKey board.
> +        */
> +       do {
> +               msleep(50);
> +       } while (error == -EBUSY && !pm_runtime_suspended(&card->dev) &&
> +                retries--);
> +

To me, this looks wrong, let me explain why.

I assume wl12xx_sdio_power_off() is being called, at "ifconfig wlan0
down". Although, could wl12xx_sdio_power_off() also be called during
system suspend, or you have another function dealing with that path?

Anyway, the call to pm_runtime_put*() a few lines above the new code
added in $subject patch, doesn't guarantee that the device ever
becomes runtime suspended. During system suspend, that is for sure,
but even when the platform is up an running, as userspace may prevent
it via the device's sysfs nobs.

That said, checking the state of the device with
pm_runtime_suspended() here, doesn't really play well.

Instead, it looks like what you need, is a way to keep track of
whether the SDIO card, became power cycled or if it stayed powered on,
when "ifconfig wlan0 up" is done. In case of a power cycle, you need
to re-program the firmware, right?

Would it be possible to re-program the firmware, even if the SDIO card
stayed powered-on?

In regards to delays needed due to a capacitance. If that really is
the case, that shall be the managed by the mmc core, as it's there the
power cycle sequence is being done. As a matter of fact, you should be
able to use drivers/mmc/core/pwrseq_simple.c and extend
"power-off-delay-us" in the DTB for Hikey if that is needed.

Kind regards
Uffe

Re: [PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[Tony Lindgren]

Hi,

* Ulf Hansson <ulf.hansson@linaro.org> [181218 12:34]:
> Instead, it looks like what you need, is a way to keep track of
> whether the SDIO card, became power cycled or if it stayed powered on,
> when "ifconfig wlan0 up" is done. In case of a power cycle, you need
> to re-program the firmware, right?

Yeah mostly. But we also need to ensure things do get powered down
properly after ifconfig wlan0 down :) IMO after ifconfig wlan0 down
returns, there should be no waiting needed.

> Would it be possible to re-program the firmware, even if the SDIO card
> stayed powered-on?

That might help for some cases, but the problem of how to ensure the
card is powered down after ifconfig wlan0 down returns still exists.

> In regards to delays needed due to a capacitance. If that really is
> the case, that shall be the managed by the mmc core, as it's there the
> power cycle sequence is being done. As a matter of fact, you should be
> able to use drivers/mmc/core/pwrseq_simple.c and extend
> "power-off-delay-us" in the DTB for Hikey if that is needed.

Hmm I do have a mostly done wlcore pwrseq driver here somewhere that
I have not had a chance to finish again. But that's not going to work
for a fix and needs updating of the related dts files too.

Regards,

Tony

Re: [PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[Ulf Hansson]

On Tue, 18 Dec 2018 at 16:54, Tony Lindgren <tony@atomide.com> wrote:
>
> Hi,
>
> * Ulf Hansson <ulf.hansson@linaro.org> [181218 12:34]:
> > Instead, it looks like what you need, is a way to keep track of
> > whether the SDIO card, became power cycled or if it stayed powered on,
> > when "ifconfig wlan0 up" is done. In case of a power cycle, you need
> > to re-program the firmware, right?
>
> Yeah mostly. But we also need to ensure things do get powered down
> properly after ifconfig wlan0 down :) IMO after ifconfig wlan0 down
> returns, there should be no waiting needed.
>
> > Would it be possible to re-program the firmware, even if the SDIO card
> > stayed powered-on?
>
> That might help for some cases, but the problem of how to ensure the
> card is powered down after ifconfig wlan0 down returns still exists.

Well, does the SDIO card really have to be powered down before
"ifconfig wlan0 down" returns? If so, why?

An option would be to call pm_runtime_get_sync() at "ifconfig wlan0
up" (I assume you already do that) and then re-program the firmware,
even if the card hasn't been power cycled. Wouldn't that work?

Or perhaps this is the problem you are encountering, that the FW can't
be reprogrammed, unless the card has been power cycled? You can easily
test that, by simply bumping the runtime usage count for the card
device via sysfs, before doing the "ifconfig wlan0 up/down" thingy.

>
> > In regards to delays needed due to a capacitance. If that really is
> > the case, that shall be the managed by the mmc core, as it's there the
> > power cycle sequence is being done. As a matter of fact, you should be
> > able to use drivers/mmc/core/pwrseq_simple.c and extend
> > "power-off-delay-us" in the DTB for Hikey if that is needed.
>
> Hmm I do have a mostly done wlcore pwrseq driver here somewhere that
> I have not had a chance to finish again. But that's not going to work
> for a fix and needs updating of the related dts files too.

Right. Let's see what we can figure out.

Kind regards
Uffe

Re: [PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[Tony Lindgren]

* Ulf Hansson <ulf.hansson@linaro.org> [181220 10:15]:
> On Tue, 18 Dec 2018 at 16:54, Tony Lindgren <tony@atomide.com> wrote:
> >
> > Hi,
> >
> > * Ulf Hansson <ulf.hansson@linaro.org> [181218 12:34]:
> > > Instead, it looks like what you need, is a way to keep track of
> > > whether the SDIO card, became power cycled or if it stayed powered on,
> > > when "ifconfig wlan0 up" is done. In case of a power cycle, you need
> > > to re-program the firmware, right?
> >
> > Yeah mostly. But we also need to ensure things do get powered down
> > properly after ifconfig wlan0 down :) IMO after ifconfig wlan0 down
> > returns, there should be no waiting needed.
> >
> > > Would it be possible to re-program the firmware, even if the SDIO card
> > > stayed powered-on?
> >
> > That might help for some cases, but the problem of how to ensure the
> > card is powered down after ifconfig wlan0 down returns still exists.
> 
> Well, does the SDIO card really have to be powered down before
> "ifconfig wlan0 down" returns? If so, why?

Good question. Eyal, any comments what should happen here from
the wlcore hardware point of view?

> An option would be to call pm_runtime_get_sync() at "ifconfig wlan0
> up" (I assume you already do that) and then re-program the firmware,
> even if the card hasn't been power cycled. Wouldn't that work?

Ricardo, care to test and see if the problem comes back if you
keep PM runtime enabled?

> Or perhaps this is the problem you are encountering, that the FW can't
> be reprogrammed, unless the card has been power cycled? You can easily
> test that, by simply bumping the runtime usage count for the card
> device via sysfs, before doing the "ifconfig wlan0 up/down" thingy.

Ricardo, care to play with this too?

Regards,

Tony

RE: [EXTERNAL] Re: [PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[Reizer, Eyal]

> > > > Instead, it looks like what you need, is a way to keep track of
> > > > whether the SDIO card, became power cycled or if it stayed powered
> on,
> > > > when "ifconfig wlan0 up" is done. In case of a power cycle, you need
> > > > to re-program the firmware, right?
> > >
> > > Yeah mostly. But we also need to ensure things do get powered down
> > > properly after ifconfig wlan0 down :) IMO after ifconfig wlan0 down
> > > returns, there should be no waiting needed.
> > >
> > > > Would it be possible to re-program the firmware, even if the SDIO card
> > > > stayed powered-on?
> > >
> > > That might help for some cases, but the problem of how to ensure the
> > > card is powered down after ifconfig wlan0 down returns still exists.
> >
> > Well, does the SDIO card really have to be powered down before
> > "ifconfig wlan0 down" returns? If so, why?
> 
> Good question. Eyal, any comments what should happen here from
> the wlcore hardware point of view?
> 

You are ok as long as the wlan_enable pin Does go down for a sufficient amount of time 
turning the wl18xx device off.
The firmware can only be downloaded once after power on.
In between down/up you have to make sure the wlan_enable is fully going through on->off->on 
and the wl18xx device is fully reset. 
On power on the firmware is loaded by the driver and this will fail in case the reset didn't happen
.
> > An option would be to call pm_runtime_get_sync() at "ifconfig wlan0
> > up" (I assume you already do that) and then re-program the firmware,
> > even if the card hasn't been power cycled. Wouldn't that work?
> 
No, this wouldn't work in case the wlan_enable pin didn't reset the chip as part of the 
"ifconfig wlan0 down" Command.

> Ricardo, care to test and see if the problem comes back if you
> keep PM runtime enabled?
> 
> > Or perhaps this is the problem you are encountering, that the FW can't
> > be reprogrammed, unless the card has been power cycled? You can easily
> > test that, by simply bumping the runtime usage count for the card
> > device via sysfs, before doing the "ifconfig wlan0 up/down" thingy.
> 

Correct. The firmware can't be reprogrammed , unless the card has been power cycled.

> Ricardo, care to play with this too?
> 
Best Regards,
Eyal

Re: [EXTERNAL] Re: [PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[Tony Lindgren]

Hi,

* Reizer, Eyal <eyalr@ti.com> [181223 07:38]:
> You are ok as long as the wlan_enable pin Does go down for a sufficient amount of time 
> turning the wl18xx device off.
> The firmware can only be downloaded once after power on.
> In between down/up you have to make sure the wlan_enable is fully going through on->off->on 
> and the wl18xx device is fully reset. 
> On power on the firmware is loaded by the driver and this will fail in case the reset didn't happen

OK thanks for confirming that it's the enable pin that needs to toggle.

Sounds like I need to get the wlcore pwrseq changes done and posted as
the long term solution.

And for a short term fix, we should just add msleep(50) for now with
updated patch description.

Does that that sound OK to everybody?

Regards,

Tony

Re: [EXTERNAL] Re: [PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[Ulf Hansson]

On Sun, 23 Dec 2018 at 17:02, Tony Lindgren <tony@atomide.com> wrote:
>
> Hi,
>
> * Reizer, Eyal <eyalr@ti.com> [181223 07:38]:
> > You are ok as long as the wlan_enable pin Does go down for a sufficient amount of time
> > turning the wl18xx device off.
> > The firmware can only be downloaded once after power on.
> > In between down/up you have to make sure the wlan_enable is fully going through on->off->on
> > and the wl18xx device is fully reset.
> > On power on the firmware is loaded by the driver and this will fail in case the reset didn't happen
>
> OK thanks for confirming that it's the enable pin that needs to toggle.
>
> Sounds like I need to get the wlcore pwrseq changes done and posted as
> the long term solution.

Just to make sure we are talking about the same things here. The
pwrseq thingy, is already implemented in the mmc core. When it comes
to Hikey, there is already a pwrseq node deployed in the DTB. So, we
should be fine.

Here are the MMC pwrseq bindings:
Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
Documentation/devicetree/bindings/mmc/mmc.txt

Here is the Hikey DTS file:
arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts

>
> And for a short term fix, we should just add msleep(50) for now with
> updated patch description.
>
> Does that that sound OK to everybody?

Unfortunate, that doesn't work. Because, if user space via sysfs has
prevented runtime suspend, the SDIO card never becomes power off with
a call to pm_runtime_put*(), not even after waiting 50 ms.

If we need a short term solution, I think there are two options.

1) Call pm_runtime_force_suspend() instead of pm_runtime_put*() at
"down". This makes sure the card becomes powered off. At "up", call
pm_runtime_force_resume() instead of pm_runtime_get_sync(). Because
the runtime PM usage count it > 1, at "up", pm_runtime_force_resume()
will power up the card, rather than deferring it.

The problem with 1), is if there is an ACPI PM domain attached to the
SDIO card device. Then this doesn't work. I can't tell if that is ever
the case here.

2) At "up", after the call to pm_runtime_get_sync(), add a call to
mmc_hw_reset(). This forces the mmc core to power cycle and re-init
the SDIO card. This may in some cases be a waste, as the card may
already have been power cycled, but at least we should now always be
able to re-program the firmware.

If there is no rush, I think we may consider to move away from using
runtime PM to control power for SDIO cards. Because, what we seem to
need, is a simple interface (reference counted) to power-on/off SDIO
cards.

Kind regards
Uffe

Re: [EXTERNAL] Re: [PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[Tony Lindgren]

Hi,

* Ulf Hansson <ulf.hansson@linaro.org> [181228 11:01]:
> On Sun, 23 Dec 2018 at 17:02, Tony Lindgren <tony@atomide.com> wrote:
> >
> > Hi,
> >
> > * Reizer, Eyal <eyalr@ti.com> [181223 07:38]:
> > > You are ok as long as the wlan_enable pin Does go down for a sufficient amount of time
> > > turning the wl18xx device off.
> > > The firmware can only be downloaded once after power on.
> > > In between down/up you have to make sure the wlan_enable is fully going through on->off->on
> > > and the wl18xx device is fully reset.
> > > On power on the firmware is loaded by the driver and this will fail in case the reset didn't happen
> >
> > OK thanks for confirming that it's the enable pin that needs to toggle.
> >
> > Sounds like I need to get the wlcore pwrseq changes done and posted as
> > the long term solution.
> 
> Just to make sure we are talking about the same things here. The
> pwrseq thingy, is already implemented in the mmc core. When it comes
> to Hikey, there is already a pwrseq node deployed in the DTB. So, we
> should be fine.
> 
> Here are the MMC pwrseq bindings:
> Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> Documentation/devicetree/bindings/mmc/mmc.txt
> 
> Here is the Hikey DTS file:
> arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts

Oh so it seems. And looking at my earlier pwrseq_wlcore.c hacks, I only
added pinctrl handling to work around a GPIO glitch on some SoCs for
deeper idle states. So I don't think we need a pwrseq_wlcore.c as
I've found a better way to deal with the GPIO glitch by implementing
gpiochip for the pinctrl driver.

> > And for a short term fix, we should just add msleep(50) for now with
> > updated patch description.
> >
> > Does that that sound OK to everybody?
> 
> Unfortunate, that doesn't work. Because, if user space via sysfs has
> prevented runtime suspend, the SDIO card never becomes power off with
> a call to pm_runtime_put*(), not even after waiting 50 ms.

Yes you're right.

> If we need a short term solution, I think there are two options.
> 
> 1) Call pm_runtime_force_suspend() instead of pm_runtime_put*() at
> "down". This makes sure the card becomes powered off. At "up", call
> pm_runtime_force_resume() instead of pm_runtime_get_sync(). Because
> the runtime PM usage count it > 1, at "up", pm_runtime_force_resume()
> will power up the card, rather than deferring it.
> 
> The problem with 1), is if there is an ACPI PM domain attached to the
> SDIO card device. Then this doesn't work. I can't tell if that is ever
> the case here.

Hmm..

> 2) At "up", after the call to pm_runtime_get_sync(), add a call to
> mmc_hw_reset(). This forces the mmc core to power cycle and re-init
> the SDIO card. This may in some cases be a waste, as the card may
> already have been power cycled, but at least we should now always be
> able to re-program the firmware.

Option 2 sounds more generic to me. Do you have some test patch for
this?

Sounds like you're thinking about adding this to the MMC framework?

If the only issue is if the card must be powered off when ifconfig
wlan0 returns, I think that's a cosmetic issue. For example,
switching to wlcore test mode after ifconfig wlan0 down might need
the card powered down, but then again the test mode resets things
anyway. Eyal?

> If there is no rush, I think we may consider to move away from using
> runtime PM to control power for SDIO cards. Because, what we seem to
> need, is a simple interface (reference counted) to power-on/off SDIO
> cards.

Well we should fix the regression in a minimal way first though.
And to me it sounds like option 2 above should do the trick, not
sure if we need something beyond that.

Regards,

Tony

Re: [EXTERNAL] Re: [PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[Ulf Hansson]

On Fri, 28 Dec 2018 at 20:59, Tony Lindgren <tony@atomide.com> wrote:
>
> Hi,
>
> * Ulf Hansson <ulf.hansson@linaro.org> [181228 11:01]:
> > On Sun, 23 Dec 2018 at 17:02, Tony Lindgren <tony@atomide.com> wrote:
> > >
> > > Hi,
> > >
> > > * Reizer, Eyal <eyalr@ti.com> [181223 07:38]:
> > > > You are ok as long as the wlan_enable pin Does go down for a sufficient amount of time
> > > > turning the wl18xx device off.
> > > > The firmware can only be downloaded once after power on.
> > > > In between down/up you have to make sure the wlan_enable is fully going through on->off->on
> > > > and the wl18xx device is fully reset.
> > > > On power on the firmware is loaded by the driver and this will fail in case the reset didn't happen
> > >
> > > OK thanks for confirming that it's the enable pin that needs to toggle.
> > >
> > > Sounds like I need to get the wlcore pwrseq changes done and posted as
> > > the long term solution.
> >
> > Just to make sure we are talking about the same things here. The
> > pwrseq thingy, is already implemented in the mmc core. When it comes
> > to Hikey, there is already a pwrseq node deployed in the DTB. So, we
> > should be fine.
> >
> > Here are the MMC pwrseq bindings:
> > Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> > Documentation/devicetree/bindings/mmc/mmc.txt
> >
> > Here is the Hikey DTS file:
> > arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts
>
> Oh so it seems. And looking at my earlier pwrseq_wlcore.c hacks, I only
> added pinctrl handling to work around a GPIO glitch on some SoCs for
> deeper idle states. So I don't think we need a pwrseq_wlcore.c as
> I've found a better way to deal with the GPIO glitch by implementing
> gpiochip for the pinctrl driver.
>

If you have a "default" pinctrl state defined in the mmc-pwrseq node
that state will be set during probe. That's because we have made each
pwrseq provider being a standalone driver, thus pinctrl_bind_pins()
get called by driver core. I realize that we should update the DT
bindings for mmc pwrseq to reflect that, I will send a patch for that.

If you need some additional pinctrl states for the mmc pwrseq_simple
driver, for example, we can easily add that. I assume putting the pins
in a sleep state once powering off the wifi chip could make sense.

However, if this is about an out-of-band IRQ line, instead of using
the SDIO IRQs on DAT1, I think management of that, belongs in the wlan
(or gpiochip) layers.

> > > And for a short term fix, we should just add msleep(50) for now with
> > > updated patch description.
> > >
> > > Does that that sound OK to everybody?
> >
> > Unfortunate, that doesn't work. Because, if user space via sysfs has
> > prevented runtime suspend, the SDIO card never becomes power off with
> > a call to pm_runtime_put*(), not even after waiting 50 ms.
>
> Yes you're right.
>
> > If we need a short term solution, I think there are two options.
> >
> > 1) Call pm_runtime_force_suspend() instead of pm_runtime_put*() at
> > "down". This makes sure the card becomes powered off. At "up", call
> > pm_runtime_force_resume() instead of pm_runtime_get_sync(). Because
> > the runtime PM usage count it > 1, at "up", pm_runtime_force_resume()
> > will power up the card, rather than deferring it.
> >
> > The problem with 1), is if there is an ACPI PM domain attached to the
> > SDIO card device. Then this doesn't work. I can't tell if that is ever
> > the case here.
>
> Hmm..
>
> > 2) At "up", after the call to pm_runtime_get_sync(), add a call to
> > mmc_hw_reset(). This forces the mmc core to power cycle and re-init
> > the SDIO card. This may in some cases be a waste, as the card may
> > already have been power cycled, but at least we should now always be
> > able to re-program the firmware.
>
> Option 2 sounds more generic to me. Do you have some test patch for
> this?

I can send a formal patch, but not sure which of the option to pick
yet, (if any).

>
> Sounds like you're thinking about adding this to the MMC framework?

Both 1) and 2) is doable already, without having to change the MMC framework.

>
> If the only issue is if the card must be powered off when ifconfig
> wlan0 returns, I think that's a cosmetic issue. For example,
> switching to wlcore test mode after ifconfig wlan0 down might need
> the card powered down, but then again the test mode resets things
> anyway. Eyal?

I am not convinced, sorry.

We have to distinguish if "down" has a strict requirement to power off
the wlan-chip.

For example, in "flight mode", is it okay to leave the wlan chip
powered on and thus potentially also having its radio part active?

>
> > If there is no rush, I think we may consider to move away from using
> > runtime PM to control power for SDIO cards. Because, what we seem to
> > need, is a simple interface (reference counted) to power-on/off SDIO
> > cards.
>
> Well we should fix the regression in a minimal way first though.
> And to me it sounds like option 2 above should do the trick, not
> sure if we need something beyond that.

Okay, let's focus on fixing the regression first, then we can look
into more long term improvements.

Kind regards
Uffe

Re: [EXTERNAL] Re: [PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[Tony Lindgren]

* Ulf Hansson <ulf.hansson@linaro.org> [190102 12:02]:
>
> However, if this is about an out-of-band IRQ line, instead of using
> the SDIO IRQs on DAT1, I think management of that, belongs in the wlan
> (or gpiochip) layers.

Yes this can be handled at the gpiochip layer.

> We have to distinguish if "down" has a strict requirement to power off
> the wlan-chip.
> 
> For example, in "flight mode", is it okay to leave the wlan chip
> powered on and thus potentially also having its radio part active?

I don't know. If unsure, we should at least have that possibility
available for future versions if needed. It might also help a bit
with PM measurements :)

Regards,

Tony

Re: [EXTERNAL] Re: [PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[Kalle Valo]

Tony Lindgren <tony@atomide.com> writes:

> * Ulf Hansson <ulf.hansson@linaro.org> [190102 12:02]:
>>
>> However, if this is about an out-of-band IRQ line, instead of using
>> the SDIO IRQs on DAT1, I think management of that, belongs in the wlan
>> (or gpiochip) layers.
>
> Yes this can be handled at the gpiochip layer.
>
>> We have to distinguish if "down" has a strict requirement to power off
>> the wlan-chip.
>> 
>> For example, in "flight mode", is it okay to leave the wlan chip
>> powered on and thus potentially also having its radio part active?
>
> I don't know. If unsure, we should at least have that possibility
> available for future versions if needed. It might also help a bit
> with PM measurements :)

I don't know if there are any written rules but I have preferred that in
wireless drivers "ifconfig down" means that all power is turned off on
the device, at least this way user space can achive maximum power
savings. And there's also a simple way to start the firmware from clean
state, we all know how buggy firmwares are so I think that's important
as well.

-- 
Kalle Valo

Re: [EXTERNAL] Re: [PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[Ulf Hansson]

On Mon, 7 Jan 2019 at 17:12, Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Tony Lindgren <tony@atomide.com> writes:
>
> > * Ulf Hansson <ulf.hansson@linaro.org> [190102 12:02]:
> >>
> >> However, if this is about an out-of-band IRQ line, instead of using
> >> the SDIO IRQs on DAT1, I think management of that, belongs in the wlan
> >> (or gpiochip) layers.
> >
> > Yes this can be handled at the gpiochip layer.
> >
> >> We have to distinguish if "down" has a strict requirement to power off
> >> the wlan-chip.
> >>
> >> For example, in "flight mode", is it okay to leave the wlan chip
> >> powered on and thus potentially also having its radio part active?
> >
> > I don't know. If unsure, we should at least have that possibility
> > available for future versions if needed. It might also help a bit
> > with PM measurements :)
>
> I don't know if there are any written rules but I have preferred that in
> wireless drivers "ifconfig down" means that all power is turned off on
> the device, at least this way user space can achive maximum power
> savings. And there's also a simple way to start the firmware from clean
> state, we all know how buggy firmwares are so I think that's important
> as well.

Alright, I buy this! I am going to have a look to see how we can
accomplish this in the best way, but I need some more time before I
can post something.

In the meantime, let's go with Tony's suggestion to fix the
regression. I am about to post a patch.

Kind regards
Uffe

Re: [EXTERNAL] Re: [PATCH] wlcore: Fix bringing up wlan0 again if powered down briefly

[Ulf Hansson]

On Sun, 23 Dec 2018 at 08:38, Reizer, Eyal <eyalr@ti.com> wrote:
>
> > > > > Instead, it looks like what you need, is a way to keep track of
> > > > > whether the SDIO card, became power cycled or if it stayed powered
> > on,
> > > > > when "ifconfig wlan0 up" is done. In case of a power cycle, you need
> > > > > to re-program the firmware, right?
> > > >
> > > > Yeah mostly. But we also need to ensure things do get powered down
> > > > properly after ifconfig wlan0 down :) IMO after ifconfig wlan0 down
> > > > returns, there should be no waiting needed.
> > > >
> > > > > Would it be possible to re-program the firmware, even if the SDIO card
> > > > > stayed powered-on?
> > > >
> > > > That might help for some cases, but the problem of how to ensure the
> > > > card is powered down after ifconfig wlan0 down returns still exists.
> > >
> > > Well, does the SDIO card really have to be powered down before
> > > "ifconfig wlan0 down" returns? If so, why?
> >
> > Good question. Eyal, any comments what should happen here from
> > the wlcore hardware point of view?
> >
>
> You are ok as long as the wlan_enable pin Does go down for a sufficient amount of time
> turning the wl18xx device off.
> The firmware can only be downloaded once after power on.
> In between down/up you have to make sure the wlan_enable is fully going through on->off->on
> and the wl18xx device is fully reset.
> On power on the firmware is loaded by the driver and this will fail in case the reset didn't happen

This clearly answers one of my question, in regards to re-programming
the FW, thanks!

However, I am still lacking an answer to if there is hard requirement
to actually power off the SDIO card at "down". So far, the indications
I have got, gives that answer "yes". Can you or anybody else confirm
that?

> .
> > > An option would be to call pm_runtime_get_sync() at "ifconfig wlan0
> > > up" (I assume you already do that) and then re-program the firmware,
> > > even if the card hasn't been power cycled. Wouldn't that work?
> >
> No, this wouldn't work in case the wlan_enable pin didn't reset the chip as part of the
> "ifconfig wlan0 down" Command.
>
> > Ricardo, care to test and see if the problem comes back if you
> > keep PM runtime enabled?
> >
> > > Or perhaps this is the problem you are encountering, that the FW can't
> > > be reprogrammed, unless the card has been power cycled? You can easily
> > > test that, by simply bumping the runtime usage count for the card
> > > device via sysfs, before doing the "ifconfig wlan0 up/down" thingy.
> >
>
> Correct. The firmware can't be reprogrammed , unless the card has been power cycled.

Again, thanks for clarifying this.

[...]

Kind regards
Uffe