Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <51b4917b-823d-263a-2412-a4b17cb38420@foss.st.com>
Date:   Wed, 23 Mar 2022 14:20:36 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v2] brcmfmac: Avoid keeping power to SDIO card unless WOWL
 is used
Content-Language: en-US
To:     Ulf Hansson <ulf.hansson@linaro.org>,
        Kalle Valo <kvalo@kernel.org>,
        <linux-wireless@vger.kernel.org>,
        Christophe ROULLIER-SCND-02 <christophe.roullier@foss.st.com>
CC:     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>
References: <20220323083950.414783-1-ulf.hansson@linaro.org>
From:   Yann Gautier <yann.gautier@foss.st.com>
In-Reply-To: <20220323083950.414783-1-ulf.hansson@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/23/22 09:39, 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 during system resume the
> SDIO card needs to be re-initialized and the FW must be re-programmed. Even
> if this may take some time to complete, it should we worth it, rather than
> draining the battery.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> 
> Changes in v2:
> 	- As pointed out by Yann, the changes for the resume path was missing,
> 	so I have added that too.
> 
> Again, 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      | 39 +++++++++++--------
>   1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index ac02244a6fdf..9c598ea97499 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);
> @@ -1176,21 +1183,19 @@ static int brcmf_ops_sdio_resume(struct device *dev)
>   	struct brcmf_bus *bus_if = dev_get_drvdata(dev);
>   	struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
>   	struct sdio_func *func = container_of(dev, struct sdio_func, dev);
> -	mmc_pm_flag_t pm_caps = sdio_get_host_pm_caps(func);
>   	int ret = 0;
>   
>   	brcmf_dbg(SDIO, "Enter: F%d\n", func->num);
>   	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);
>   
>   		brcmf_sdiod_freezer_off(sdiodev);

Hi Ulf,

Thanks for the patch, it is OK, and tested by Christophe (R.).
So you can add:
Tested-by: Christophe Roullier <christophe.roullier@foss.st.com>
Acked-by: Yann Gautier <yann.gautier@foss.st.com>


Best regards,
Yann