On 3/17/22 16:28, Ulf Hansson wrote:
> Keeping the power to the SDIO card during system wide suspend, consumes
> energy. Especially on battery driven embedded systems, this can be a
> problem. Therefore, let's change the behaviour into allowing the SDIO card
> to be powered off, unless WOWL is supported and enabled.
>
> Note that, the downside from this change, is that at system resume the SDIO
> card needs to be re-initialized and the FW must re-programmed. Even it that
> may take some time to complete, it should we worth it, rather than draining
> a battery.
>
> Signed-off-by: Ulf Hansson <[email protected]>
> ---
>
> Please note that, I have only compile-tested this patch, so I am relying on help
> from Yann and others to run tests on real HW.
>
> Kind regards
> Ulf Hansson
>
> ---
> .../broadcom/brcm80211/brcmfmac/bcmsdh.c | 33 +++++++++++--------
> 1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index ac02244a6fdf..351886c9d68e 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -1119,9 +1119,21 @@ void brcmf_sdio_wowl_config(struct device *dev, bool enabled)
> {
> struct brcmf_bus *bus_if = dev_get_drvdata(dev);
> struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
> + mmc_pm_flag_t pm_caps = sdio_get_host_pm_caps(sdiodev->func1);
>
> - brcmf_dbg(SDIO, "Configuring WOWL, enabled=%d\n", enabled);
> - sdiodev->wowl_enabled = enabled;
> + /* Power must be preserved to be able to support WOWL. */
> + if (!(pm_caps & MMC_PM_KEEP_POWER))
> + goto notsup;
> +
> + if (sdiodev->settings->bus.sdio.oob_irq_supported ||
> + pm_caps & MMC_PM_WAKE_SDIO_IRQ) {
> + sdiodev->wowl_enabled = enabled;
> + brcmf_dbg(SDIO, "Configuring WOWL, enabled=%d\n", enabled);
> + return;
> + }
> +
> +notsup:
> + brcmf_dbg(SDIO, "WOWL not supported\n");
> }
>
> #ifdef CONFIG_PM_SLEEP
> @@ -1130,7 +1142,7 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
> struct sdio_func *func;
> struct brcmf_bus *bus_if;
> struct brcmf_sdio_dev *sdiodev;
> - mmc_pm_flag_t pm_caps, sdio_flags;
> + mmc_pm_flag_t sdio_flags;
> int ret = 0;
>
> func = container_of(dev, struct sdio_func, dev);
> @@ -1142,20 +1154,15 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
> bus_if = dev_get_drvdata(dev);
> sdiodev = bus_if->bus_priv.sdio;
>
> - pm_caps = sdio_get_host_pm_caps(func);
> -
> - if (pm_caps & MMC_PM_KEEP_POWER) {
> - /* preserve card power during suspend */
> + if (sdiodev->wowl_enabled) {
> brcmf_sdiod_freezer_on(sdiodev);
> brcmf_sdio_wd_timer(sdiodev->bus, 0);
>
> sdio_flags = MMC_PM_KEEP_POWER;
> - if (sdiodev->wowl_enabled) {
> - if (sdiodev->settings->bus.sdio.oob_irq_supported)
> - enable_irq_wake(sdiodev->settings->bus.sdio.oob_irq_nr);
> - else
> - sdio_flags |= MMC_PM_WAKE_SDIO_IRQ;
> - }
> + if (sdiodev->settings->bus.sdio.oob_irq_supported)
> + enable_irq_wake(sdiodev->settings->bus.sdio.oob_irq_nr);
> + else
> + sdio_flags |= MMC_PM_WAKE_SDIO_IRQ;
>
> if (sdio_set_host_pm_flags(sdiodev->func1, sdio_flags))
> brcmf_err("Failed to set pm_flags %x\n", sdio_flags);
Hi Ulf,
You are missing the counter part in brcmf_ops_sdio_resume:
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -1190,14 +1190,13 @@ static int brcmf_ops_sdio_resume(struct device *dev)
if (func->num != 2)
return 0;
- if (!(pm_caps & MMC_PM_KEEP_POWER)) {
+ if (!sdiodev->wowl_enabled) {
/* bus was powered off and device removed, probe again */
ret = brcmf_sdiod_probe(sdiodev);
if (ret)
brcmf_err("Failed to probe device on resume\n");
} else {
- if (sdiodev->wowl_enabled &&
- sdiodev->settings->bus.sdio.oob_irq_supported)
+ if (sdiodev->settings->bus.sdio.oob_irq_supported)
disable_irq_wake(sdiodev->settings->bus.sdio.oob_irq_nr);
Else, we get an oops when calling brcmf_sdio_sleep.
Best regards,
Yann
On Fri, 18 Mar 2022 at 11:47, Yann Gautier <[email protected]> wrote:
>
> On 3/17/22 16:28, Ulf Hansson wrote:
> > Keeping the power to the SDIO card during system wide suspend, consumes
> > energy. Especially on battery driven embedded systems, this can be a
> > problem. Therefore, let's change the behaviour into allowing the SDIO card
> > to be powered off, unless WOWL is supported and enabled.
> >
> > Note that, the downside from this change, is that at system resume the SDIO
> > card needs to be re-initialized and the FW must re-programmed. Even it that
> > may take some time to complete, it should we worth it, rather than draining
> > a battery.
> >
> > Signed-off-by: Ulf Hansson <[email protected]>
> > ---
> >
> > Please note that, I have only compile-tested this patch, so I am relying on help
> > from Yann and others to run tests on real HW.
> >
> > Kind regards
> > Ulf Hansson
> >
> > ---
> > .../broadcom/brcm80211/brcmfmac/bcmsdh.c | 33 +++++++++++--------
> > 1 file changed, 20 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> > index ac02244a6fdf..351886c9d68e 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> > @@ -1119,9 +1119,21 @@ void brcmf_sdio_wowl_config(struct device *dev, bool enabled)
> > {
> > struct brcmf_bus *bus_if = dev_get_drvdata(dev);
> > struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
> > + mmc_pm_flag_t pm_caps = sdio_get_host_pm_caps(sdiodev->func1);
> >
> > - brcmf_dbg(SDIO, "Configuring WOWL, enabled=%d\n", enabled);
> > - sdiodev->wowl_enabled = enabled;
> > + /* Power must be preserved to be able to support WOWL. */
> > + if (!(pm_caps & MMC_PM_KEEP_POWER))
> > + goto notsup;
> > +
> > + if (sdiodev->settings->bus.sdio.oob_irq_supported ||
> > + pm_caps & MMC_PM_WAKE_SDIO_IRQ) {
> > + sdiodev->wowl_enabled = enabled;
> > + brcmf_dbg(SDIO, "Configuring WOWL, enabled=%d\n", enabled);
> > + return;
> > + }
> > +
> > +notsup:
> > + brcmf_dbg(SDIO, "WOWL not supported\n");
> > }
> >
> > #ifdef CONFIG_PM_SLEEP
> > @@ -1130,7 +1142,7 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
> > struct sdio_func *func;
> > struct brcmf_bus *bus_if;
> > struct brcmf_sdio_dev *sdiodev;
> > - mmc_pm_flag_t pm_caps, sdio_flags;
> > + mmc_pm_flag_t sdio_flags;
> > int ret = 0;
> >
> > func = container_of(dev, struct sdio_func, dev);
> > @@ -1142,20 +1154,15 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
> > bus_if = dev_get_drvdata(dev);
> > sdiodev = bus_if->bus_priv.sdio;
> >
> > - pm_caps = sdio_get_host_pm_caps(func);
> > -
> > - if (pm_caps & MMC_PM_KEEP_POWER) {
> > - /* preserve card power during suspend */
> > + if (sdiodev->wowl_enabled) {
> > brcmf_sdiod_freezer_on(sdiodev);
> > brcmf_sdio_wd_timer(sdiodev->bus, 0);
> >
> > sdio_flags = MMC_PM_KEEP_POWER;
> > - if (sdiodev->wowl_enabled) {
> > - if (sdiodev->settings->bus.sdio.oob_irq_supported)
> > - enable_irq_wake(sdiodev->settings->bus.sdio.oob_irq_nr);
> > - else
> > - sdio_flags |= MMC_PM_WAKE_SDIO_IRQ;
> > - }
> > + if (sdiodev->settings->bus.sdio.oob_irq_supported)
> > + enable_irq_wake(sdiodev->settings->bus.sdio.oob_irq_nr);
> > + else
> > + sdio_flags |= MMC_PM_WAKE_SDIO_IRQ;
> >
> > if (sdio_set_host_pm_flags(sdiodev->func1, sdio_flags))
> > brcmf_err("Failed to set pm_flags %x\n", sdio_flags);
>
> Hi Ulf,
>
> You are missing the counter part in brcmf_ops_sdio_resume:
>
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -1190,14 +1190,13 @@ static int brcmf_ops_sdio_resume(struct device *dev)
> if (func->num != 2)
> return 0;
>
> - if (!(pm_caps & MMC_PM_KEEP_POWER)) {
> + if (!sdiodev->wowl_enabled) {
> /* bus was powered off and device removed, probe again */
> ret = brcmf_sdiod_probe(sdiodev);
> if (ret)
> brcmf_err("Failed to probe device on resume\n");
> } else {
> - if (sdiodev->wowl_enabled &&
> - sdiodev->settings->bus.sdio.oob_irq_supported)
> + if (sdiodev->settings->bus.sdio.oob_irq_supported)
>
> disable_irq_wake(sdiodev->settings->bus.sdio.oob_irq_nr);
>
>
> Else, we get an oops when calling brcmf_sdio_sleep.
Yes, of course - thanks for reviewing and testing. A v2 is on the way out.
>
>
> Best regards,
> Yann
Kind regards
Uffe