[PATCH v2] brcmfmac: Avoid keeping power to SDIO card unless WOWL is used

[PATCH v2] brcmfmac: Avoid keeping power to SDIO card unless WOWL is used

[Ulf Hansson]

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);
-- 
2.25.1

Re: [PATCH v2] brcmfmac: Avoid keeping power to SDIO card unless WOWL is used

[Yann Gautier]

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

Re: [PATCH v2] brcmfmac: Avoid keeping power to SDIO card unless WOWL is used

[Kalle Valo]

Yann Gautier <yann.gautier@foss.st.com> writes:

> 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>
>
> 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>

Acked-by is used by the driver maintainer, so I assume you mean
Reviewed-by?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2] brcmfmac: Avoid keeping power to SDIO card unless WOWL is used

[Yann Gautier]

On 3/23/22 17:04, Kalle Valo wrote:
> Yann Gautier <yann.gautier@foss.st.com> writes:
> 
>> 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>
>>
>> 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>
> 
> Acked-by is used by the driver maintainer, so I assume you mean
> Reviewed-by?
> 
Oops, sorry, yes I meant Reviewed-by.

Best regards,
Yann

Re: [v2] brcmfmac: Avoid keeping power to SDIO card unless WOWL is used

[Kalle Valo]

Ulf Hansson <ulf.hansson@linaro.org> 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>
> Tested-by: Christophe Roullier <christophe.roullier@foss.st.com>
> Reviewed-by: Yann Gautier <yann.gautier@foss.st.com>

Patch applied to wireless-next.git, thanks.

92cadedd9d5f brcmfmac: Avoid keeping power to SDIO card unless WOWL is used

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20220323083950.414783-1-ulf.hansson@linaro.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches