| Message ID | 20220317152846.246281-1-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | brcmfmac: Avoid keeping power to SDIO card unless WOWL is used | expand |
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 <ulf.hansson@linaro.org> > --- > > 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 <yann.gautier@foss.st.com> 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 <ulf.hansson@linaro.org> > > --- > > > > 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
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);
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 <ulf.hansson@linaro.org> --- 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(-)