Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220317152846.246281-1-ulf.hansson@linaro.org> <98472721-7f3c-3f2b-42a6-d50b51c6191f@foss.st.com>
In-Reply-To: <98472721-7f3c-3f2b-42a6-d50b51c6191f@foss.st.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Wed, 23 Mar 2022 09:15:12 +0100
Message-ID: <CAPDyKFpAmrdZQEAS8Vs98y5LrdYM4OM0srEo1XiNHy1bV6QT7g@mail.gmail.com>
Subject: Re: [PATCH] brcmfmac: Avoid keeping power to SDIO card unless WOWL is used
To:     Yann Gautier <yann.gautier@foss.st.com>
Cc:     Kalle Valo <kvalo@kernel.org>, linux-wireless@vger.kernel.org,
        Arend van Spriel <aspriel@gmail.com>,
        Franky Lin <franky.lin@broadcom.com>,
        Hante Meuleman <hante.meuleman@broadcom.com>,
        Gustavo Padovan <gustavo.padovan@collabora.com>,
        Adrian Ratiu <adrian.ratiu@collabora.com>,
        brcm80211-dev-list.pdl@broadcom.com, linux-kernel@vger.kernel.org,
        Christophe KERELLO - foss <christophe.kerello@foss.st.com>,
        Christophe ROULLIER-SCND-02 <christophe.roullier@foss.st.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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