Re: RFC: wilc1000 module parameter to disable chip sleep

From: <Ajay.Kathat@microchip.com>
Date: 2021-12-10 09:03:30

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@microchip.com wrote:

quoted

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.

quoted

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.

quoted

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.

quoted hunk ↗ jump to hunk

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);
  }

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help