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@xxxxxxxxxxxxx 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
From 61362f54c301275879539248d761dc77c9ee6271 Mon Sep 17 00:00:00 2001
From: Ajay Singh <ajay.kathat@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
---
.../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
