On 09/12/21 23:45, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> [Sorry, my first email was accidentally had the patch appended as a MIME
> attachment.]
>
> On Thu, 2021-12-09 at 09:20 +0000, Ajay.Kathat@xxxxxxxxxxxxx wrote:
>> On 09/12/21 00:20, David Mosberger-Tang wrote:
>> The "chip_wakeup" & "chip_allow_sleep" API's are required to configure
>> the wake-up registers in wilc. These registers must be configured
>> correctly when the power save mode is enabled.
> I see.
>
>>> Here is what I see: on a system configured for minimum power consumption
>>> (all unnecessary daemons disabled, Ethernet unplugged) I measured 1,180 mW
>>> when the WILC chip is in RESET (the ENABLE pin is always high on our
>>> platform).
>>>
>>> With the wilc1000-spi/wilc1000 modules loaded and the WILC chip
>>> running but without being associated with a WLAN network, measured
>>> power consumption was 1,460 mW, regardless of whether chip sleep was
>>> enabled or disabled.
>> Did you test by enabling the power-save mode with "wpa_supplicant" or
>> using "iw" command. For power consumption measurement, please try by
>> disabling the PSM mode.
> No, I have not played with power-saving mode. There are some disconcerting
> messages when turning PSM on:
>
> [ 1764.070375] wilc1000_spi spi1.0: Failed cmd, cmd (ca), resp (00)
> [ 1764.076403] wilc1000_spi spi1.0: Failed cmd, read reg (000013f4)...
>
> The driver does still work, but occasionally, I get long streams of these
> messages, sometimes it gets to the point where there are so many that the
> watchdog timer ends up rebooting the system.
>
>> The chip-sleep code is needed to configure the registers properly for
>> wilc and not using these sequence would have impact on sending the data
>> to wilc, especially when the PS mode is enabled.
> How about something along the patch below then?
Thanks.
The changes looks okay to me.
> With this patch, I see the following iperf3 performance (120 second average):
>
> PSM on:
> [ 4] 0.00-120.00 sec 47.6 MBytes 3.33 Mbits/sec 0 sender
>
> PSM off:
> [ 4] 0.00-120.00 sec 201 MBytes 14.0 Mbits/sec 12 sender
>
> In terms of power consumption:
>
> baseline : 1.2 W
> PSM on, idle : 1.2 W
> PSM on, TX @ 3.33 Mbps : 1.7 W
> PSM off, idle : 1.5 W
> PSM off, TX @ 14.0 Mbps : 2.4 W
>
> --david
>
> diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c
> b/drivers/net/wireless/microchip/wilc1000/hif.c
> index 44e93cd5e3df..09aa5ced19d3 100644
> --- a/drivers/net/wireless/microchip/wilc1000/hif.c
> +++ b/drivers/net/wireless/microchip/wilc1000/hif.c
> @@ -1912,6 +1912,7 @@ int wilc_edit_station(struct wilc_vif *vif, const u8 *mac,
>
> int wilc_set_power_mgmt(struct wilc_vif *vif, bool enabled, u32 timeout)
> {
> + struct wilc *wilc = vif->wilc;
> struct wid wid;
> int result;
> s8 power_mode;
> @@ -1927,6 +1928,8 @@ int wilc_set_power_mgmt(struct wilc_vif *vif, bool
> enabled, u32 timeout)
> result = wilc_send_config_pkt(vif, WILC_SET_CFG, &wid, 1);
> if (result)
> netdev_err(vif->ndev, "Failed to send power management\n");
> + else
> + wilc->power_save_mode = enabled;
>
> return result;
> }
> diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h
> b/drivers/net/wireless/microchip/wilc1000/netdev.h
> index 152408232d51..db149abc5d0d 100644
> --- a/drivers/net/wireless/microchip/wilc1000/netdev.h
> +++ b/drivers/net/wireless/microchip/wilc1000/netdev.h
> @@ -267,6 +267,7 @@ struct wilc {
>
> int clients_count;
> struct workqueue_struct *hif_workqueue;
> + bool power_save_mode;
> enum chip_ps_states chip_ps_state;
> struct wilc_cfg cfg;
> void *bus_data;
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c
> b/drivers/net/wireless/microchip/wilc1000/wlan.c
> index ddd382996275..0e0e325d396e 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
> @@ -18,13 +18,13 @@ static inline bool is_wilc1000(u32 id)
> static inline void acquire_bus(struct wilc *wilc, enum bus_acquire acquire)
> {
> mutex_lock(&wilc->hif_cs);
> - if (acquire == WILC_BUS_ACQUIRE_AND_WAKEUP)
> + if (acquire == WILC_BUS_ACQUIRE_AND_WAKEUP && wilc->power_save_mode)
> chip_wakeup(wilc);
> }
>
> static inline void release_bus(struct wilc *wilc, enum bus_release release)
> {
> - if (release == WILC_BUS_RELEASE_ALLOW_SLEEP)
> + if (release == WILC_BUS_RELEASE_ALLOW_SLEEP && wilc->power_save_mode)
> chip_allow_sleep(wilc);
> mutex_unlock(&wilc->hif_cs);
> }
>
