[PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Marek Vasut]

The WILC3000 can suspend and enter low power state. According to local
measurements, the WILC3000 consumes the same amount of power if the slot
is powered up and WILC3000 is suspended, and if the WILC3000 is powered
off. Use the former option, keep the WILC3000 powered up as that allows
for things like WoWlan to work.

Note that this is tested on WILC3000 only, not on WILC1000 .

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Ajay Singh <ajay.kathat@microchip.com>
Cc: Alexis Lothoré <alexis.lothore@bootlin.com>
Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
Cc: Kalle Valo <kvalo@kernel.org>
Cc: Marek Vasut <marex@denx.de>
Cc: linux-wireless@vger.kernel.org
---
 drivers/net/wireless/microchip/wilc1000/sdio.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
index 683a35c682a8f..41122199d51eb 100644
--- a/drivers/net/wireless/microchip/wilc1000/sdio.c
+++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
@@ -973,7 +973,6 @@ static int wilc_sdio_suspend(struct device *dev)
 {
 	struct sdio_func *func = dev_to_sdio_func(dev);
 	struct wilc *wilc = sdio_get_drvdata(func);
-	int ret;
 
 	dev_info(dev, "sdio suspend\n");
 
@@ -987,13 +986,7 @@ static int wilc_sdio_suspend(struct device *dev)
 
 	wilc_sdio_disable_interrupt(wilc);
 
-	ret = wilc_sdio_reset(wilc);
-	if (ret) {
-		dev_err(&func->dev, "Fail reset sdio\n");
-		return ret;
-	}
-
-	return 0;
+	return sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
 }
 
 static int wilc_sdio_resume(struct device *dev)
-- 
2.43.0

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Alexis Lothoré]

On 8/21/24 20:37, Marek Vasut wrote:
> The WILC3000 can suspend and enter low power state. According to local
> measurements, the WILC3000 consumes the same amount of power if the slot
> is powered up and WILC3000 is suspended, and if the WILC3000 is powered
> off. Use the former option, keep the WILC3000 powered up as that allows
> for things like WoWlan to work.
> 
> Note that this is tested on WILC3000 only, not on WILC1000 .
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Ajay Singh <ajay.kathat@microchip.com>
> Cc: Alexis Lothoré <alexis.lothore@bootlin.com>
> Cc: Claudiu Beznea <claudiu.beznea@tuxon.dev>
> Cc: Kalle Valo <kvalo@kernel.org>
> Cc: Marek Vasut <marex@denx.de>
> Cc: linux-wireless@vger.kernel.org
> ---
>  drivers/net/wireless/microchip/wilc1000/sdio.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
> index 683a35c682a8f..41122199d51eb 100644
> --- a/drivers/net/wireless/microchip/wilc1000/sdio.c
> +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
> @@ -973,7 +973,6 @@ static int wilc_sdio_suspend(struct device *dev)
>  {
>  	struct sdio_func *func = dev_to_sdio_func(dev);
>  	struct wilc *wilc = sdio_get_drvdata(func);
> -	int ret;
>  
>  	dev_info(dev, "sdio suspend\n");
>  
> @@ -987,13 +986,7 @@ static int wilc_sdio_suspend(struct device *dev)
>  
>  	wilc_sdio_disable_interrupt(wilc);
>  
> -	ret = wilc_sdio_reset(wilc);
> -	if (ret) {
> -		dev_err(&func->dev, "Fail reset sdio\n");
> -		return ret;
> -	}
> -
> -	return 0;
> +	return sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
>  }

This change breaks suspend/resume on my wilc1000 setup (sama5d2 wlsom evk +
wilc1000 sd):

# echo mem > /sys/power/state
PM: suspend entry (deep)
Filesystems sync: 0.055 seconds
Freezing user space processes
Freezing user space processes completed (elapsed 0.018 seconds)
OOM killer disabled.
Freezing remaining freezable tasks
Freezing remaining freezable tasks completed (elapsed 0.006 seconds)
wilc1000_sdio mmc0:0001:1: sdio suspend
wilc1000_sdio mmc0:0001:1: PM: dpm_run_callback(): pm_generic_suspend returns -22
wilc1000_sdio mmc0:0001:1: PM: failed to suspend async: error -22
PM: Some devices failed to suspend, or early wake event detected
OOM killer enabled.
Restarting tasks ... done.
random: crng reseeded on system resumption
PM: suspend exit
sh: write error: Invalid argument

But I have to dig more to really understand the root cause.

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Marek Vasut]

On 8/27/24 11:53 AM, Alexis Lothoré wrote:

Hi,

>> diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
>> index 683a35c682a8f..41122199d51eb 100644
>> --- a/drivers/net/wireless/microchip/wilc1000/sdio.c
>> +++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
>> @@ -973,7 +973,6 @@ static int wilc_sdio_suspend(struct device *dev)
>>   {
>>   	struct sdio_func *func = dev_to_sdio_func(dev);
>>   	struct wilc *wilc = sdio_get_drvdata(func);
>> -	int ret;
>>   
>>   	dev_info(dev, "sdio suspend\n");
>>   
>> @@ -987,13 +986,7 @@ static int wilc_sdio_suspend(struct device *dev)
>>   
>>   	wilc_sdio_disable_interrupt(wilc);
>>   
>> -	ret = wilc_sdio_reset(wilc);
>> -	if (ret) {
>> -		dev_err(&func->dev, "Fail reset sdio\n");
>> -		return ret;
>> -	}
>> -
>> -	return 0;
>> +	return sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
>>   }
> 
> This change breaks suspend/resume on my wilc1000 setup (sama5d2 wlsom evk +
> wilc1000 sd):
> 
> # echo mem > /sys/power/state
> PM: suspend entry (deep)
> Filesystems sync: 0.055 seconds
> Freezing user space processes
> Freezing user space processes completed (elapsed 0.018 seconds)
> OOM killer disabled.
> Freezing remaining freezable tasks
> Freezing remaining freezable tasks completed (elapsed 0.006 seconds)
> wilc1000_sdio mmc0:0001:1: sdio suspend
> wilc1000_sdio mmc0:0001:1: PM: dpm_run_callback(): pm_generic_suspend returns -22
> wilc1000_sdio mmc0:0001:1: PM: failed to suspend async: error -22
> PM: Some devices failed to suspend, or early wake event detected
> OOM killer enabled.
> Restarting tasks ... done.
> random: crng reseeded on system resumption
> PM: suspend exit
> sh: write error: Invalid argument
> 
> But I have to dig more to really understand the root cause.

Does your MMC controller struct mmc_host set .pm_caps |= 
MMC_PM_KEEP_POWER ? Maybe that's the problem, that the controller does 
not set these PM caps ?

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Alexis Lothoré]

On 8/27/24 17:20, Marek Vasut wrote:
> On 8/27/24 11:53 AM, Alexis Lothoré wrote:

[...]

>>> -    return 0;
>>> +    return sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
>>>   }
>>
>> This change breaks suspend/resume on my wilc1000 setup (sama5d2 wlsom evk +
>> wilc1000 sd):
>>
>> # echo mem > /sys/power/state
>> PM: suspend entry (deep)
>> Filesystems sync: 0.055 seconds
>> Freezing user space processes
>> Freezing user space processes completed (elapsed 0.018 seconds)
>> OOM killer disabled.
>> Freezing remaining freezable tasks
>> Freezing remaining freezable tasks completed (elapsed 0.006 seconds)
>> wilc1000_sdio mmc0:0001:1: sdio suspend
>> wilc1000_sdio mmc0:0001:1: PM: dpm_run_callback(): pm_generic_suspend returns -22
>> wilc1000_sdio mmc0:0001:1: PM: failed to suspend async: error -22
>> PM: Some devices failed to suspend, or early wake event detected
>> OOM killer enabled.
>> Restarting tasks ... done.
>> random: crng reseeded on system resumption
>> PM: suspend exit
>> sh: write error: Invalid argument
>>
>> But I have to dig more to really understand the root cause.
> 
> Does your MMC controller struct mmc_host set .pm_caps |= MMC_PM_KEEP_POWER ?
> Maybe that's the problem, that the controller does not set these PM caps ?

It looks like you are right, my sdmmc controller was missing some
keep-power-in-suspend flag in my device tree, preventing your change to work on
my platform. So it behaves correctly for me with both wilc1000/wilc3000, thanks.

Looks ok for me, but others may have a more informed opinion than me about the
impact of this change.

Thanks,

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Ajay.Kathat]

Hi,

On 8/28/24 00:42, Alexis Lothoré wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 8/27/24 17:20, Marek Vasut wrote:
>> On 8/27/24 11:53 AM, Alexis Lothoré wrote:
> 
> [...]
> 
>>>> -    return 0;
>>>> +    return sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
>>>>   }
>>>
>>> This change breaks suspend/resume on my wilc1000 setup (sama5d2 wlsom evk +
>>> wilc1000 sd):
>>>
>>> # echo mem > /sys/power/state
>>> PM: suspend entry (deep)
>>> Filesystems sync: 0.055 seconds
>>> Freezing user space processes
>>> Freezing user space processes completed (elapsed 0.018 seconds)
>>> OOM killer disabled.
>>> Freezing remaining freezable tasks
>>> Freezing remaining freezable tasks completed (elapsed 0.006 seconds)
>>> wilc1000_sdio mmc0:0001:1: sdio suspend
>>> wilc1000_sdio mmc0:0001:1: PM: dpm_run_callback(): pm_generic_suspend returns -22
>>> wilc1000_sdio mmc0:0001:1: PM: failed to suspend async: error -22
>>> PM: Some devices failed to suspend, or early wake event detected
>>> OOM killer enabled.
>>> Restarting tasks ... done.
>>> random: crng reseeded on system resumption
>>> PM: suspend exit
>>> sh: write error: Invalid argument
>>>
>>> But I have to dig more to really understand the root cause.
>>
>> Does your MMC controller struct mmc_host set .pm_caps |= MMC_PM_KEEP_POWER ?
>> Maybe that's the problem, that the controller does not set these PM caps ?
> 
> It looks like you are right, my sdmmc controller was missing some
> keep-power-in-suspend flag in my device tree, preventing your change to work on
> my platform. So it behaves correctly for me with both wilc1000/wilc3000, thanks.
> 
> Looks ok for me, but others may have a more informed opinion than me about the
> impact of this change.
> 

When the host suspend is triggered, the WILC power consumption should be
reduced since it is controlled via chip_allow_sleep() sequence which is
irrespective of MMC_PM_KEEP_POWER flag state of the host. So I'm not sure why
there was no difference observed in Marek's setup. It may be that when the
power consumption was measured,the WILC suspend state is not enabled because
of MMC controller pm_caps in the test setup.

I think it is better to have a generic patch for any host which has
MMC_PM_KEEP_POWER capabilities defined or not. With proposed patch, driver
will not allow the host to go into the suspend state when MMC_PM_KEEP_POWER is
not set in PM caps. I think, sdio_set_host_pm_flags() should only be called if
MMC_PM_KEEP_POWER is defined in the host.

Actually, WILC can support suspend mode with or without this host
capabilities. For SDIO, the host can be wake-up using the external IRQ GPIO
(uses out-of-band, instead of in-band interrupt) on WILC.

To test wake-on-wlan(wowlan) in suspend mode, the IRQ pin from WILC should be
connected with the host that will help to interrupt/wake the host when any
WiFi packet arrives. Without 'wowlan' enabled in suspend mode, the host should
be allowed to go into suspend mode but it can't be wake-up by WiFi packets.
All the packets will be dropped in the firmware in suspend state.

WILC supports only ANY option for wowlan. So, after connecting the IRQ line
with host, the below commands can be used to test "wowlan" in suspend state.


#  iw phy <phyname> wowlan enable any
#  echo mem > /sys/power/state


Regards,
Ajay

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Marek Vasut]

On 8/29/24 3:45 AM, Ajay.Kathat@microchip.com wrote:
> Hi,

Hi,

>>>> This change breaks suspend/resume on my wilc1000 setup (sama5d2 wlsom evk +
>>>> wilc1000 sd):
>>>>
>>>> # echo mem > /sys/power/state
>>>> PM: suspend entry (deep)
>>>> Filesystems sync: 0.055 seconds
>>>> Freezing user space processes
>>>> Freezing user space processes completed (elapsed 0.018 seconds)
>>>> OOM killer disabled.
>>>> Freezing remaining freezable tasks
>>>> Freezing remaining freezable tasks completed (elapsed 0.006 seconds)
>>>> wilc1000_sdio mmc0:0001:1: sdio suspend
>>>> wilc1000_sdio mmc0:0001:1: PM: dpm_run_callback(): pm_generic_suspend returns -22
>>>> wilc1000_sdio mmc0:0001:1: PM: failed to suspend async: error -22
>>>> PM: Some devices failed to suspend, or early wake event detected
>>>> OOM killer enabled.
>>>> Restarting tasks ... done.
>>>> random: crng reseeded on system resumption
>>>> PM: suspend exit
>>>> sh: write error: Invalid argument
>>>>
>>>> But I have to dig more to really understand the root cause.
>>>
>>> Does your MMC controller struct mmc_host set .pm_caps |= MMC_PM_KEEP_POWER ?
>>> Maybe that's the problem, that the controller does not set these PM caps ?
>>
>> It looks like you are right, my sdmmc controller was missing some
>> keep-power-in-suspend flag in my device tree, preventing your change to work on
>> my platform. So it behaves correctly for me with both wilc1000/wilc3000, thanks.
>>
>> Looks ok for me, but others may have a more informed opinion than me about the
>> impact of this change.
>>
> 
> When the host suspend is triggered, the WILC power consumption should be
> reduced since it is controlled via chip_allow_sleep() sequence which is
> irrespective of MMC_PM_KEEP_POWER flag state of the host. So I'm not sure why
> there was no difference observed in Marek's setup.

I think you misunderstood the patch description, there is no measurable 
power consumption difference with/without this patch. The chip does 
either get powered off (without this patch) or enter some sort of low 
power state (with this patch), which I can see on the drop in power 
consumption during suspend/resume.

This patch assures the chip does not get power-cycled during 
suspend/resume cycle, which makes it lose state (and firmware), so the 
chip is unusable after resume without this patch.

> It may be that when the
> power consumption was measured,the WILC suspend state is not enabled because
> of MMC controller pm_caps in the test setup.
> 
> I think it is better to have a generic patch for any host which has
> MMC_PM_KEEP_POWER capabilities defined or not. With proposed patch, driver
> will not allow the host to go into the suspend state when MMC_PM_KEEP_POWER is
> not set in PM caps. I think, sdio_set_host_pm_flags() should only be called if
> MMC_PM_KEEP_POWER is defined in the host.

To retain firmware in the chip, the chip must not be powered off during 
suspend/resume, which is what this patch assures. Without this patch, 
the controller may power off the slot during suspend/resume and the WILC 
will lose firmware and be unusable after resume.

> Actually, WILC can support suspend mode with or without this host
> capabilities. For SDIO, the host can be wake-up using the external IRQ GPIO
> (uses out-of-band, instead of in-band interrupt) on WILC.
> 
> To test wake-on-wlan(wowlan) in suspend mode, the IRQ pin from WILC should be
> connected with the host that will help to interrupt/wake the host when any
> WiFi packet arrives. Without 'wowlan' enabled in suspend mode, the host should
> be allowed to go into suspend mode but it can't be wake-up by WiFi packets.
> All the packets will be dropped in the firmware in suspend state.
> 
> WILC supports only ANY option for wowlan. So, after connecting the IRQ line
> with host, the below commands can be used to test "wowlan" in suspend state.
> 
> 
> #  iw phy <phyname> wowlan enable any
> #  echo mem > /sys/power/state

If the WILC is powered off because the slot is powered off, WILC cannot 
resume anything.

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Ajay.Kathat]

Hi Marek,

On 8/28/24 19:45, Marek Vasut wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On 8/29/24 3:45 AM, Ajay.Kathat@microchip.com wrote:
>> Hi,
> 
> Hi,
> 
>>>>> This change breaks suspend/resume on my wilc1000 setup (sama5d2 wlsom evk +
>>>>> wilc1000 sd):
>>>>>
>>>>> # echo mem > /sys/power/state
>>>>> PM: suspend entry (deep)
>>>>> Filesystems sync: 0.055 seconds
>>>>> Freezing user space processes
>>>>> Freezing user space processes completed (elapsed 0.018 seconds)
>>>>> OOM killer disabled.
>>>>> Freezing remaining freezable tasks
>>>>> Freezing remaining freezable tasks completed (elapsed 0.006 seconds)
>>>>> wilc1000_sdio mmc0:0001:1: sdio suspend
>>>>> wilc1000_sdio mmc0:0001:1: PM: dpm_run_callback(): pm_generic_suspend
>>>>> returns -22
>>>>> wilc1000_sdio mmc0:0001:1: PM: failed to suspend async: error -22
>>>>> PM: Some devices failed to suspend, or early wake event detected
>>>>> OOM killer enabled.
>>>>> Restarting tasks ... done.
>>>>> random: crng reseeded on system resumption
>>>>> PM: suspend exit
>>>>> sh: write error: Invalid argument
>>>>>
>>>>> But I have to dig more to really understand the root cause.
>>>>
>>>> Does your MMC controller struct mmc_host set .pm_caps |= MMC_PM_KEEP_POWER ?
>>>> Maybe that's the problem, that the controller does not set these PM caps ?
>>>
>>> It looks like you are right, my sdmmc controller was missing some
>>> keep-power-in-suspend flag in my device tree, preventing your change to
>>> work on
>>> my platform. So it behaves correctly for me with both wilc1000/wilc3000,
>>> thanks.
>>>
>>> Looks ok for me, but others may have a more informed opinion than me about the
>>> impact of this change.
>>>
>>
>> When the host suspend is triggered, the WILC power consumption should be
>> reduced since it is controlled via chip_allow_sleep() sequence which is
>> irrespective of MMC_PM_KEEP_POWER flag state of the host. So I'm not sure why
>> there was no difference observed in Marek's setup.
> 
> I think you misunderstood the patch description, there is no measurable
> power consumption difference with/without this patch. The chip does
> either get powered off (without this patch) or enter some sort of low
> power state (with this patch), which I can see on the drop in power
> consumption during suspend/resume.

Thanks for the clarification.

> 
> This patch assures the chip does not get power-cycled during
> suspend/resume cycle, which makes it lose state (and firmware), so the
> chip is unusable after resume without this patch.
> 

During host suspend, the firmware doesn't get power-cycled and also doesn't
forward the frames to the host.

1. Without this patch, is the station getting disconnected from the AP during
the suspend state? Does a rescan and reconnect help to resume the connection
with the AP?

2. With this patch, does the ping to the station work during the suspend state?

AFAIR, during host suspend, the firmware continues to run without passing
frames to the host unless 'wowlan' is enabled.

There is another scenario. Let's assume a host that wants to go to suspend
(power save mode) without caring about the WiFi status, i.e., it is okay to
reconnect with the AP if required (anyway, the AP may disconnect the station
based on inactivity timeout) or have to re-trigger the DHCP request again. But
with this change, the driver would block the host from entering suspend mode.

How about adding an 'if' check for host pm_caps before calling
sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER)? In that case, it will only
request when configured by the host platform.


>> It may be that when the
>> power consumption was measured,the WILC suspend state is not enabled because
>> of MMC controller pm_caps in the test setup.
>>
>> I think it is better to have a generic patch for any host which has
>> MMC_PM_KEEP_POWER capabilities defined or not. With proposed patch, driver
>> will not allow the host to go into the suspend state when MMC_PM_KEEP_POWER is
>> not set in PM caps. I think, sdio_set_host_pm_flags() should only be called if
>> MMC_PM_KEEP_POWER is defined in the host.
> 
> To retain firmware in the chip, the chip must not be powered off during
> suspend/resume, which is what this patch assures. Without this patch,
> the controller may power off the slot during suspend/resume and the WILC
> will lose firmware and be unusable after resume.
> 
>> Actually, WILC can support suspend mode with or without this host
>> capabilities. For SDIO, the host can be wake-up using the external IRQ GPIO
>> (uses out-of-band, instead of in-band interrupt) on WILC.
>>
>> To test wake-on-wlan(wowlan) in suspend mode, the IRQ pin from WILC should be
>> connected with the host that will help to interrupt/wake the host when any
>> WiFi packet arrives. Without 'wowlan' enabled in suspend mode, the host should
>> be allowed to go into suspend mode but it can't be wake-up by WiFi packets.
>> All the packets will be dropped in the firmware in suspend state.
>>
>> WILC supports only ANY option for wowlan. So, after connecting the IRQ line
>> with host, the below commands can be used to test "wowlan" in suspend state.
>>
>>
>> #  iw phy <phyname> wowlan enable any
>> #  echo mem > /sys/power/state
> 
> If the WILC is powered off because the slot is powered off, WILC cannot
> resume anything.

I think, the wilc firmware should resume but the connection with AP may get
closed. Additional commands to scan and reconnect with AP may be required that
should work without downloading the firmware to wilc chip again.


Regard,
Ajay

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Marek Vasut]

On 8/29/24 7:51 AM, Ajay.Kathat@microchip.com wrote:

Hi,

>> This patch assures the chip does not get power-cycled during
>> suspend/resume cycle, which makes it lose state (and firmware), so the
>> chip is unusable after resume without this patch.
>>
> 
> During host suspend, the firmware doesn't get power-cycled and also doesn't
> forward the frames to the host.

If the slot is powered OFF, which it currently can be, then the entire 
WILC gets power-cycled.

> 1. Without this patch, is the station getting disconnected from the AP during
> the suspend state? Does a rescan and reconnect help to resume the connection
> with the AP?

Rescan doesn't work because there is no firmware in the WILC.

> 2. With this patch, does the ping to the station work during the suspend state?

I haven't tested this, but that's unlikely because the host is 
suspended. Still, that's not really the point here, the point is that 
the whole WILC gets powered off during suspend/resume without this 
patch. At least on STM32MP15xx it is, maybe on the Atmel controller it 
is not, but we cannot depend on that.

> AFAIR, during host suspend, the firmware continues to run without passing
> frames to the host unless 'wowlan' is enabled.
> 
> There is another scenario. Let's assume a host that wants to go to suspend
> (power save mode) without caring about the WiFi status, i.e., it is okay to
> reconnect with the AP if required (anyway, the AP may disconnect the station
> based on inactivity timeout) or have to re-trigger the DHCP request again. But
> with this change, the driver would block the host from entering suspend mode.
> 
> How about adding an 'if' check for host pm_caps before calling
> sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER)? In that case, it will only
> request when configured by the host platform.

Since this driver does not reload the firmware into the card on resume, 
the card has to be kept powered on during suspend/resume cycle. The card 
can NOT be powered off during suspend/resume cycle, otherwise the 
firmware is lost.

Without this flag, the card may be powered off during suspend/resume 
cycle. It possibly does not happen on the Atmel controller, but it does 
on the STM32MP15xx ARM MMCI one.

Now, since the card does consume about the same amount of power whether 
it is powered OFF or whether it is powered ON but suspended, I opt for 
the later option -- keep the card powered ON, suspend it, and that's 
what this patch does. This also allows us to support WoWlan then.

>>> It may be that when the
>>> power consumption was measured,the WILC suspend state is not enabled because
>>> of MMC controller pm_caps in the test setup.
>>>
>>> I think it is better to have a generic patch for any host which has
>>> MMC_PM_KEEP_POWER capabilities defined or not. With proposed patch, driver
>>> will not allow the host to go into the suspend state when MMC_PM_KEEP_POWER is
>>> not set in PM caps. I think, sdio_set_host_pm_flags() should only be called if
>>> MMC_PM_KEEP_POWER is defined in the host.
>>
>> To retain firmware in the chip, the chip must not be powered off during
>> suspend/resume, which is what this patch assures. Without this patch,
>> the controller may power off the slot during suspend/resume and the WILC
>> will lose firmware and be unusable after resume.
>>
>>> Actually, WILC can support suspend mode with or without this host
>>> capabilities. For SDIO, the host can be wake-up using the external IRQ GPIO
>>> (uses out-of-band, instead of in-band interrupt) on WILC.
>>>
>>> To test wake-on-wlan(wowlan) in suspend mode, the IRQ pin from WILC should be
>>> connected with the host that will help to interrupt/wake the host when any
>>> WiFi packet arrives. Without 'wowlan' enabled in suspend mode, the host should
>>> be allowed to go into suspend mode but it can't be wake-up by WiFi packets.
>>> All the packets will be dropped in the firmware in suspend state.
>>>
>>> WILC supports only ANY option for wowlan. So, after connecting the IRQ line
>>> with host, the below commands can be used to test "wowlan" in suspend state.
>>>
>>>
>>> #  iw phy <phyname> wowlan enable any
>>> #  echo mem > /sys/power/state
>>
>> If the WILC is powered off because the slot is powered off, WILC cannot
>> resume anything.
> 
> I think, the wilc firmware should resume but the connection with AP may get
> closed. Additional commands to scan and reconnect with AP may be required that
> should work without downloading the firmware to wilc chip again.

Currently, the slot may get powered off and then there is no firmware.

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Kalle Valo]

Marek Vasut <marex@denx.de> writes:

> Since this driver does not reload the firmware into the card on
> resume, the card has to be kept powered on during suspend/resume
> cycle. The card can NOT be powered off during suspend/resume cycle,
> otherwise the firmware is lost.
>
> Without this flag, the card may be powered off during suspend/resume
> cycle. It possibly does not happen on the Atmel controller, but it
> does on the STM32MP15xx ARM MMCI one.
>
> Now, since the card does consume about the same amount of power
> whether it is powered OFF or whether it is powered ON but suspended, I
> opt for the later option -- keep the card powered ON, suspend it, and
> that's what this patch does. This also allows us to support WoWlan
> then.

Are you also taking into account hibernation? During hibernation the
device will be powered off. I can't remember the details right now but
wanted to mention this.

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

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

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Marek Vasut]

On 8/29/24 6:32 PM, Kalle Valo wrote:
> Marek Vasut <marex@denx.de> writes:
> 
>> Since this driver does not reload the firmware into the card on
>> resume, the card has to be kept powered on during suspend/resume
>> cycle. The card can NOT be powered off during suspend/resume cycle,
>> otherwise the firmware is lost.
>>
>> Without this flag, the card may be powered off during suspend/resume
>> cycle. It possibly does not happen on the Atmel controller, but it
>> does on the STM32MP15xx ARM MMCI one.
>>
>> Now, since the card does consume about the same amount of power
>> whether it is powered OFF or whether it is powered ON but suspended, I
>> opt for the later option -- keep the card powered ON, suspend it, and
>> that's what this patch does. This also allows us to support WoWlan
>> then.
> 
> Are you also taking into account hibernation? During hibernation the
> device will be powered off. I can't remember the details right now but
> wanted to mention this.

I don't think I am. Isn't hibernation actually a full shutdown, so the 
hardware does get reinitialized ?

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Kalle Valo]

Marek Vasut <marex@denx.de> writes:

> On 8/29/24 6:32 PM, Kalle Valo wrote:
>> Marek Vasut <marex@denx.de> writes:
>> 
>>> Since this driver does not reload the firmware into the card on
>>> resume, the card has to be kept powered on during suspend/resume
>>> cycle. The card can NOT be powered off during suspend/resume cycle,
>>> otherwise the firmware is lost.
>>>
>>> Without this flag, the card may be powered off during suspend/resume
>>> cycle. It possibly does not happen on the Atmel controller, but it
>>> does on the STM32MP15xx ARM MMCI one.
>>>
>>> Now, since the card does consume about the same amount of power
>>> whether it is powered OFF or whether it is powered ON but suspended, I
>>> opt for the later option -- keep the card powered ON, suspend it, and
>>> that's what this patch does. This also allows us to support WoWlan
>>> then.
>> Are you also taking into account hibernation? During hibernation the
>> device will be powered off. I can't remember the details right now but
>> wanted to mention this.
>
> I don't think I am. Isn't hibernation actually a full shutdown, so the
> hardware does get reinitialized ?

I don't know how it works exactly nor what you exactly mean with
reinitalized. But at least with ath11k hibernation didn't work when it
left the firmware running during suspend.

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

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
https://docs.kernel.org/process/submitting-patches.html

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Marek Vasut]

On 9/2/24 6:32 PM, Kalle Valo wrote:

Hi,

>>>> Since this driver does not reload the firmware into the card on
>>>> resume, the card has to be kept powered on during suspend/resume
>>>> cycle. The card can NOT be powered off during suspend/resume cycle,
>>>> otherwise the firmware is lost.
>>>>
>>>> Without this flag, the card may be powered off during suspend/resume
>>>> cycle. It possibly does not happen on the Atmel controller, but it
>>>> does on the STM32MP15xx ARM MMCI one.
>>>>
>>>> Now, since the card does consume about the same amount of power
>>>> whether it is powered OFF or whether it is powered ON but suspended, I
>>>> opt for the later option -- keep the card powered ON, suspend it, and
>>>> that's what this patch does. This also allows us to support WoWlan
>>>> then.
>>> Are you also taking into account hibernation? During hibernation the
>>> device will be powered off. I can't remember the details right now but
>>> wanted to mention this.
>>
>> I don't think I am. Isn't hibernation actually a full shutdown, so the
>> hardware does get reinitialized ?
> 
> I don't know how it works exactly nor what you exactly mean with
> reinitalized.

My understanding is that hibernation is suspend-to-disk, isn't it ?
(that's not something that is even available on my hardware)

Doesn't the hardware get completely turned OFF during suspend-to-disk 
and then turned ON (and therefore initialized again) on resume-from-disk?

> But at least with ath11k hibernation didn't work when it
> left the firmware running during suspend.

Is there a thread on lore or some details of this you could point me to?

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Kalle Valo]

Marek Vasut <marex@denx.de> writes:

>>>> Are you also taking into account hibernation? During hibernation the
>>>> device will be powered off. I can't remember the details right now but
>>>> wanted to mention this.
>>>
>>> I don't think I am. Isn't hibernation actually a full shutdown, so the
>>> hardware does get reinitialized ?
>> I don't know how it works exactly nor what you exactly mean with
>> reinitalized.
>
> My understanding is that hibernation is suspend-to-disk, isn't it ?

Yes, that's what I mean.

> (that's not something that is even available on my hardware)

Yeah, I'm guessing wilc1000 is used more in embedded enviroments where
hibernation is not really an important, it's used more in laptops. So I
guess it won't be a big problem if wilc1000 doesn't support hibernation
but please try to keep it in mind still.

> Doesn't the hardware get completely turned OFF during suspend-to-disk
> and then turned ON (and therefore initialized again) on
> resume-from-disk?

I'm not sure how it works exactly but my experience is that for
hibernation a driver cannot assume that the firmware is running during
resume.

>> But at least with ath11k hibernation didn't work when it
>> left the firmware running during suspend.
>
> Is there a thread on lore or some details of this you could point me to?

See commit 166a490f59ac ("wifi: ath11k: support hibernation") and the
commits before that. I'm sure that there's more info about hibernation
but don't have any pointers.

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

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

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Marek Vasut]

On 9/5/24 9:27 AM, Kalle Valo wrote:
> Marek Vasut <marex@denx.de> writes:
> 
>>>>> Are you also taking into account hibernation? During hibernation the
>>>>> device will be powered off. I can't remember the details right now but
>>>>> wanted to mention this.
>>>>
>>>> I don't think I am. Isn't hibernation actually a full shutdown, so the
>>>> hardware does get reinitialized ?
>>> I don't know how it works exactly nor what you exactly mean with
>>> reinitalized.
>>
>> My understanding is that hibernation is suspend-to-disk, isn't it ?
> 
> Yes, that's what I mean.
> 
>> (that's not something that is even available on my hardware)
> 
> Yeah, I'm guessing wilc1000 is used more in embedded enviroments where
> hibernation is not really an important, it's used more in laptops. So I
> guess it won't be a big problem if wilc1000 doesn't support hibernation
> but please try to keep it in mind still.
> 
>> Doesn't the hardware get completely turned OFF during suspend-to-disk
>> and then turned ON (and therefore initialized again) on
>> resume-from-disk?
> 
> I'm not sure how it works exactly but my experience is that for
> hibernation a driver cannot assume that the firmware is running during
> resume.
> 
>>> But at least with ath11k hibernation didn't work when it
>>> left the firmware running during suspend.
>>
>> Is there a thread on lore or some details of this you could point me to?
> 
> See commit 166a490f59ac ("wifi: ath11k: support hibernation") and the
> commits before that. I'm sure that there's more info about hibernation
> but don't have any pointers.

Ah, thank you.

The firmware reloading support should be addressed by some sort of 
future variant of patch Ajay sent as attachment in [1], that should then 
also deal with the hibernation part. I asked them to post the patch 
properly to the ML.

What about this patch, it fixes suspend/resume for me and with a fix to 
the atmel controller I suggested to Alexis in this thread, it should not 
have any adverse effects .

[1] 
https://lore.kernel.org/all/2bbdc690-aec5-4a11-893e-01270c6d5b84@microchip.com/

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Ajay.Kathat]

[-- Attachment #1: Type: text/plain, Size: 5908 bytes --]

Hi Marek,

On 8/29/24 08:38, Marek Vasut wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On 8/29/24 7:51 AM, Ajay.Kathat@microchip.com wrote:
> 
> Hi,
> 
>>> This patch assures the chip does not get power-cycled during
>>> suspend/resume cycle, which makes it lose state (and firmware), so the
>>> chip is unusable after resume without this patch.
>>>
>>
>> During host suspend, the firmware doesn't get power-cycled and also doesn't
>> forward the frames to the host.
> 
> If the slot is powered OFF, which it currently can be, then the entire
> WILC gets power-cycled.
> 
>> 1. Without this patch, is the station getting disconnected from the AP during
>> the suspend state? Does a rescan and reconnect help to resume the connection
>> with the AP?
> 
> Rescan doesn't work because there is no firmware in the WILC.
> 
>> 2. With this patch, does the ping to the station work during the suspend state?
> 
> I haven't tested this, but that's unlikely because the host is
> suspended. Still, that's not really the point here, the point is that
> the whole WILC gets powered off during suspend/resume without this
> patch. At least on STM32MP15xx it is, maybe on the Atmel controller it
> is not, but we cannot depend on that.

Indeed, you are right. It seems, the test results have dependency on the host
controller power management behavior. In my setup, the card keep the power,
when the host is suspended.

> 
>> AFAIR, during host suspend, the firmware continues to run without passing
>> frames to the host unless 'wowlan' is enabled.
>>
>> There is another scenario. Let's assume a host that wants to go to suspend
>> (power save mode) without caring about the WiFi status, i.e., it is okay to
>> reconnect with the AP if required (anyway, the AP may disconnect the station
>> based on inactivity timeout) or have to re-trigger the DHCP request again. But
>> with this change, the driver would block the host from entering suspend mode.
>>
>> How about adding an 'if' check for host pm_caps before calling
>> sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER)? In that case, it will only
>> request when configured by the host platform.
> 
> Since this driver does not reload the firmware into the card on resume,
> the card has to be kept powered on during suspend/resume cycle. The card
> can NOT be powered off during suspend/resume cycle, otherwise the
> firmware is lost.
> 
> Without this flag, the card may be powered off during suspend/resume
> cycle. It possibly does not happen on the Atmel controller, but it does
> on the STM32MP15xx ARM MMCI one.
> 
yes, in the Atmel controller, it is not necessary to re-program the firmware
on resume.

> Now, since the card does consume about the same amount of power whether
> it is powered OFF or whether it is powered ON but suspended, I opt for
> the later option -- keep the card powered ON, suspend it, and that's
> what this patch does. This also allows us to support WoWlan then.
> >>>> It may be that when the
>>>> power consumption was measured,the WILC suspend state is not enabled because
>>>> of MMC controller pm_caps in the test setup.
>>>>
>>>> I think it is better to have a generic patch for any host which has
>>>> MMC_PM_KEEP_POWER capabilities defined or not. With proposed patch, driver
>>>> will not allow the host to go into the suspend state when
>>>> MMC_PM_KEEP_POWER is
>>>> not set in PM caps. I think, sdio_set_host_pm_flags() should only be
>>>> called if
>>>> MMC_PM_KEEP_POWER is defined in the host.
>>>
>>> To retain firmware in the chip, the chip must not be powered off during
>>> suspend/resume, which is what this patch assures. Without this patch,
>>> the controller may power off the slot during suspend/resume and the WILC
>>> will lose firmware and be unusable after resume.
>>>
>>>> Actually, WILC can support suspend mode with or without this host
>>>> capabilities. For SDIO, the host can be wake-up using the external IRQ GPIO
>>>> (uses out-of-band, instead of in-band interrupt) on WILC.
>>>>
>>>> To test wake-on-wlan(wowlan) in suspend mode, the IRQ pin from WILC should be
>>>> connected with the host that will help to interrupt/wake the host when any
>>>> WiFi packet arrives. Without 'wowlan' enabled in suspend mode, the host
>>>> should
>>>> be allowed to go into suspend mode but it can't be wake-up by WiFi packets.
>>>> All the packets will be dropped in the firmware in suspend state.
>>>>
>>>> WILC supports only ANY option for wowlan. So, after connecting the IRQ line
>>>> with host, the below commands can be used to test "wowlan" in suspend state.
>>>>
>>>>
>>>> #  iw phy <phyname> wowlan enable any
>>>> #  echo mem > /sys/power/state
>>>
>>> If the WILC is powered off because the slot is powered off, WILC cannot
>>> resume anything.
>>
>> I think, the wilc firmware should resume but the connection with AP may get
>> closed. Additional commands to scan and reconnect with AP may be required that
>> should work without downloading the firmware to wilc chip again.
> 
> Currently, the slot may get powered off and then there is no firmware.

Currently, driver doesn't support re-programming of the chip when the host
resumes. Having that support would allow both types of hosts to suspend/resume.
Added changes to download the firmware asynchronously on resume and it is
included in the attached patch. It is based on the latest 'for-next' branch. I
was not able to test it for the suspend scenario without powering the card. If
possible, could you please try this patch in your setup.

Incase, there is no improvement with the attached patch then IMO, your patch
looks safe to keep the MMC_PM_KEEP_POWER state since host resume may not work
without the card powered-on anyway.


Regards,
Ajay


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-wifi-wilc1000-download-firmware-on-resume-when-host-.patch --]
[-- Type: text/x-patch; name="0001-wifi-wilc1000-download-firmware-on-resume-when-host-.patch", Size: 6716 bytes --]

From 61362f54c301275879539248d761dc77c9ee6271 Mon Sep 17 00:00:00 2001
From: Ajay Singh <ajay.kathat@microchip.com>
Date: Fri, 30 Aug 2024 18:21:23 -0700
Subject: [PATCH] wifi: wilc1000: download firmware on resume when host suspend
 without power to SDIO card

The host can enter the power-save mode using command [1], with or without
keeping the power to the SDIO card. The host controller driver maintains this
information in the power management capabilities(pm_caps).
Based on the defined capabilities or supported mode, the host keeps the power
state of the card.
wilc can resume successfully when the host keeps the power to card in suspend state.
However, it fails to resume when power is not applied to SDIO card.
This change will again download the firmware asynchronously, only when the card is
suspended with powering the SDIO cardi;otherwise, it will resume using the
existing firmware.

[1]. echo mem > /sys/power/state
Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 .../net/wireless/microchip/wilc1000/netdev.c  | 82 +++++++++++++++++--
 .../net/wireless/microchip/wilc1000/netdev.h  |  9 ++
 .../net/wireless/microchip/wilc1000/sdio.c    | 11 ++-
 3 files changed, 96 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 9ecf3fb29b55..11b219b7a87e 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -191,12 +191,12 @@ static int wilc_txq_task(void *vp)
 	return 0;
 }
 
-static int wilc_wlan_get_firmware(struct net_device *dev)
+static int wilc_wlan_get_firmware(struct net_device *dev, int nowait)
 {
 	struct wilc_vif *vif = netdev_priv(dev);
 	struct wilc *wilc = vif->wilc;
 	int chip_id;
-	const struct firmware *wilc_fw;
+	const struct firmware *wilc_fw = NULL;
 	int ret;
 
 	chip_id = wilc_get_chipid(wilc, false);
@@ -204,8 +204,15 @@ static int wilc_wlan_get_firmware(struct net_device *dev)
 	netdev_info(dev, "ChipID [%x] loading firmware [%s]\n", chip_id,
 		    WILC1000_FW(WILC1000_API_VER));
 
-	ret = request_firmware(&wilc_fw, WILC1000_FW(WILC1000_API_VER),
-			       wilc->dev);
+	if (!nowait)
+		ret = request_firmware(&wilc_fw, WILC1000_FW(WILC1000_API_VER),
+				       wilc->dev);
+	else
+		ret = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,
+					      WILC1000_FW(WILC1000_API_VER),
+					      wilc->dev, GFP_KERNEL,
+					      wilc, wilc_load_firmware);
+
 	if (ret != 0) {
 		netdev_err(dev, "%s - firmware not available\n",
 			   WILC1000_FW(WILC1000_API_VER));
@@ -495,6 +502,71 @@ static int wlan_initialize_threads(struct net_device *dev)
 	return 0;
 }
 
+void wilc_load_firmware(const struct firmware *firmware, void *context)
+{
+	struct wilc *wl = (struct wilc *)context;
+	struct wilc_vif *vif;
+	struct net_device *dev;
+	int ret;
+	int index = 0;
+
+	vif = wl->ctx.vif[index];
+	dev = vif->ndev;
+	wl->firmware = firmware;
+	ret = wilc1000_firmware_download(dev);
+	if (ret)
+		return;
+
+	ret = wilc_start_firmware(dev);
+	if (ret)
+		return;
+
+	ret = wilc_init_fw_config(dev, vif);
+	if (ret)
+		return;
+
+	wl->initialized = true;
+
+	for (; index < WILC_NUM_CONCURRENT_IFC; index++) {
+		vif = wl->ctx.vif[index];
+		if (!vif)
+			break;
+
+		dev = vif->ndev;
+		wilc_set_mac_address(vif, dev->dev_addr);
+		wilc_set_operation_mode(vif, wilc_get_vif_idx(vif),
+					vif->iftype, vif->idx);
+	}
+	memset(&wl->ctx, 0x0, sizeof(wl->ctx));
+}
+
+int wilc_reflash_firmware(struct wilc *wl)
+{
+	int ret = -EINVAL;
+	int index = 0;
+	int srcu_idx;
+	struct wilc_vif *vif;
+
+	srcu_idx = srcu_read_lock(&wl->srcu);
+	memset(&wl->ctx, 0x0, sizeof(wl->ctx));
+	wilc_for_each_vif(wl, vif) {
+		wl->ctx.vif[index] = vif;
+		index++;
+	}
+	if (!index)
+		goto out;
+
+	ret = wilc_wlan_get_firmware(wl->ctx.vif[0]->ndev, true);
+	if (ret)
+		goto out;
+	wl->mac_status = WILC_MAC_STATUS_INIT;
+
+out:
+	srcu_read_unlock(&wl->srcu, srcu_idx);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(wilc_reflash_firmware);
+
 static int wilc_wlan_initialize(struct net_device *dev, struct wilc_vif *vif)
 {
 	int ret = 0;
@@ -524,7 +596,7 @@ static int wilc_wlan_initialize(struct net_device *dev, struct wilc_vif *vif)
 			goto fail_irq_init;
 		}
 
-		ret = wilc_wlan_get_firmware(dev);
+		ret = wilc_wlan_get_firmware(dev, false);
 		if (ret)
 			goto fail_irq_enable;
 
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h b/drivers/net/wireless/microchip/wilc1000/netdev.h
index 95bc8b8fe65a..77285597baec 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.h
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.h
@@ -205,6 +205,11 @@ struct wilc_tx_queue_status {
 	bool initialized;
 };
 
+struct wilc_reflash_context {
+	u8 fw_reflash;
+	struct wilc_vif *vif[WILC_NUM_CONCURRENT_IFC];
+};
+
 struct wilc {
 	struct wiphy *wiphy;
 	const struct wilc_hif_func *hif_func;
@@ -287,6 +292,7 @@ struct wilc {
 	struct ieee80211_supported_band band;
 	u32 cipher_suites[ARRAY_SIZE(wilc_cipher_suites)];
 	u8 nv_mac_address[ETH_ALEN];
+	struct wilc_reflash_context ctx;
 };
 
 struct wilc_wfi_mon_priv {
@@ -302,4 +308,7 @@ void wilc_wlan_set_bssid(struct net_device *wilc_netdev, const u8 *bssid,
 struct wilc_vif *wilc_netdev_ifc_init(struct wilc *wl, const char *name,
 				      int vif_type, enum nl80211_iftype type,
 				      bool rtnl_locked);
+void wilc_load_firmware(const struct firmware *firmware, void *context);
+int wilc_reflash_firmware(struct wilc *wl);
+
 #endif
diff --git a/drivers/net/wireless/microchip/wilc1000/sdio.c b/drivers/net/wireless/microchip/wilc1000/sdio.c
index 0043f7a0fdf9..8e1b179897c7 100644
--- a/drivers/net/wireless/microchip/wilc1000/sdio.c
+++ b/drivers/net/wireless/microchip/wilc1000/sdio.c
@@ -973,6 +973,7 @@ static int wilc_sdio_suspend(struct device *dev)
 {
 	struct sdio_func *func = dev_to_sdio_func(dev);
 	struct wilc *wilc = sdio_get_drvdata(func);
+	mmc_pm_flag_t flags = sdio_get_host_pm_caps(func);
 	int ret;
 
 	dev_info(dev, "sdio suspend\n");
@@ -989,8 +990,12 @@ static int wilc_sdio_suspend(struct device *dev)
 		dev_err(&func->dev, "Fail reset sdio\n");
 		return ret;
 	}
+	if (flags & MMC_PM_KEEP_POWER)
+		ret = sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
+	else
+		wilc->ctx.fw_reflash = 1;
 
-	return 0;
+	return ret;
 }
 
 static int wilc_sdio_resume(struct device *dev)
@@ -1002,6 +1007,10 @@ static int wilc_sdio_resume(struct device *dev)
 	wilc_sdio_init(wilc, true);
 	wilc_sdio_enable_interrupt(wilc);
 
+	/* reprogram firmware to chip, if host suspended with no power to card */
+	if (wilc->ctx.fw_reflash)
+		wilc_reflash_firmware(wilc);
+
 	host_wakeup_notify(wilc);
 
 	return 0;
-- 
2.34.1

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Marek Vasut]

On 8/31/24 7:22 AM, Ajay.Kathat@microchip.com wrote:
> Hi Marek,

Hi,

>>> 2. With this patch, does the ping to the station work during the suspend state?
>>
>> I haven't tested this, but that's unlikely because the host is
>> suspended. Still, that's not really the point here, the point is that
>> the whole WILC gets powered off during suspend/resume without this
>> patch. At least on STM32MP15xx it is, maybe on the Atmel controller it
>> is not, but we cannot depend on that.
> 
> Indeed, you are right. It seems, the test results have dependency on the host
> controller power management behavior. In my setup, the card keep the power,
> when the host is suspended.

Right, currently the driver depends on the controller behavior and it 
works by accident with the atmel controller. This patch fixes that and 
assures this will work on other controllers too.

>>> AFAIR, during host suspend, the firmware continues to run without passing
>>> frames to the host unless 'wowlan' is enabled.
>>>
>>> There is another scenario. Let's assume a host that wants to go to suspend
>>> (power save mode) without caring about the WiFi status, i.e., it is okay to
>>> reconnect with the AP if required (anyway, the AP may disconnect the station
>>> based on inactivity timeout) or have to re-trigger the DHCP request again. But
>>> with this change, the driver would block the host from entering suspend mode.
>>>
>>> How about adding an 'if' check for host pm_caps before calling
>>> sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER)? In that case, it will only
>>> request when configured by the host platform.
>>
>> Since this driver does not reload the firmware into the card on resume,
>> the card has to be kept powered on during suspend/resume cycle. The card
>> can NOT be powered off during suspend/resume cycle, otherwise the
>> firmware is lost.
>>
>> Without this flag, the card may be powered off during suspend/resume
>> cycle. It possibly does not happen on the Atmel controller, but it does
>> on the STM32MP15xx ARM MMCI one.
>>
> yes, in the Atmel controller, it is not necessary to re-program the firmware
> on resume.

It is if the slot gets powered off across suspend/resume . It could be 
that this does not happen on the hardware you use for testing, but the 
driver cannot depend on that.

[...]

>>> I think, the wilc firmware should resume but the connection with AP may get
>>> closed. Additional commands to scan and reconnect with AP may be required that
>>> should work without downloading the firmware to wilc chip again.
>>
>> Currently, the slot may get powered off and then there is no firmware.
> 
> Currently, driver doesn't support re-programming of the chip when the host
> resumes.

Therefore, the slot MUST stay powered on across suspend/resume.

> Having that support would allow both types of hosts to suspend/resume.
> Added changes to download the firmware asynchronously on resume and it is
> included in the attached patch. It is based on the latest 'for-next' branch. I
> was not able to test it for the suspend scenario without powering the card. If
> possible, could you please try this patch in your setup.
> 
> Incase, there is no improvement with the attached patch then IMO, your patch
> looks safe to keep the MMC_PM_KEEP_POWER state since host resume may not work
> without the card powered-on anyway.

I think the attached patch is a new feature, this patch is a fix for 
existing bug. Please submit the attached patch properly, so it can get 
proper review on the ML.

Thank you

Re: [PATCH] wifi: wilc1000: Keep slot powered on during suspend/resume

[Ajay.Kathat]

On 8/31/24 14:18, Marek Vasut wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On 8/31/24 7:22 AM, Ajay.Kathat@microchip.com wrote:
>> Hi Marek,
> 
> Hi,
> 
>>>> 2. With this patch, does the ping to the station work during the suspend
>>>> state?
>>>
>>> I haven't tested this, but that's unlikely because the host is
>>> suspended. Still, that's not really the point here, the point is that
>>> the whole WILC gets powered off during suspend/resume without this
>>> patch. At least on STM32MP15xx it is, maybe on the Atmel controller it
>>> is not, but we cannot depend on that.
>>
>> Indeed, you are right. It seems, the test results have dependency on the host
>> controller power management behavior. In my setup, the card keep the power,
>> when the host is suspended.
> 
> Right, currently the driver depends on the controller behavior and it
> works by accident with the atmel controller. This patch fixes that and
> assures this will work on other controllers too.
> 
>>>> AFAIR, during host suspend, the firmware continues to run without passing
>>>> frames to the host unless 'wowlan' is enabled.
>>>>
>>>> There is another scenario. Let's assume a host that wants to go to suspend
>>>> (power save mode) without caring about the WiFi status, i.e., it is okay to
>>>> reconnect with the AP if required (anyway, the AP may disconnect the station
>>>> based on inactivity timeout) or have to re-trigger the DHCP request again.
>>>> But
>>>> with this change, the driver would block the host from entering suspend mode.
>>>>
>>>> How about adding an 'if' check for host pm_caps before calling
>>>> sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER)? In that case, it will only
>>>> request when configured by the host platform.
>>>
>>> Since this driver does not reload the firmware into the card on resume,
>>> the card has to be kept powered on during suspend/resume cycle. The card
>>> can NOT be powered off during suspend/resume cycle, otherwise the
>>> firmware is lost.
>>>
>>> Without this flag, the card may be powered off during suspend/resume
>>> cycle. It possibly does not happen on the Atmel controller, but it does
>>> on the STM32MP15xx ARM MMCI one.
>>>
>> yes, in the Atmel controller, it is not necessary to re-program the firmware
>> on resume.
> 
> It is if the slot gets powered off across suspend/resume . It could be
> that this does not happen on the hardware you use for testing, but the
> driver cannot depend on that.
> 
> [...]
> 
>>>> I think, the wilc firmware should resume but the connection with AP may get
>>>> closed. Additional commands to scan and reconnect with AP may be required
>>>> that
>>>> should work without downloading the firmware to wilc chip again.
>>>
>>> Currently, the slot may get powered off and then there is no firmware.
>>
>> Currently, driver doesn't support re-programming of the chip when the host
>> resumes.
> 
> Therefore, the slot MUST stay powered on across suspend/resume.

>> Having that support would allow both types of hosts to suspend/resume.
>> Added changes to download the firmware asynchronously on resume and it is
>> included in the attached patch. It is based on the latest 'for-next' branch. I
>> was not able to test it for the suspend scenario without powering the card. If
>> possible, could you please try this patch in your setup.
>>
>> Incase, there is no improvement with the attached patch then IMO, your patch
>> looks safe to keep the MMC_PM_KEEP_POWER state since host resume may not work
>> without the card powered-on anyway.
> 
> I think the attached patch is a new feature, this patch is a fix for
> existing bug. Please submit the attached patch properly, so it can get
> proper review on the ML.

I believe your patch addresses the host suspend/resume scenario until the
driver includes support to re-download the firmware.

Before submitting the new patch to ML for re-download on resume, I must test
it with the exact setup to confirm if it really resolves the issue. I tested
the patch by forcing the re-download in resume, but it was not an exact
scenario because the wilc chip was not entirely powered down during host
suspend in my setup. I will check if a similar setup can be arranged to test
this scenario before submitting it to ML.

Regards,
Ajay