| Message ID | 20240821183823.163268-1-marex@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | wifi: wilc1000: Keep slot powered on during suspend/resume | expand |
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.
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 ?
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
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
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.
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
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.
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.
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 ?
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
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
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.
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?
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
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.
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/
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)
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(-)