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