Re: RFC: wilc1000 module parameter to disable chip sleep

[<Ajay.Kathat@microchip.com>]

On 09/12/21 00:20, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> At least on our SAMA5-based platform, the chip-sleep in the wilc1000
> driver degrades WILC1000 transmit throughput by more than three times,
> without providing significant power-savings.  Because of that, I have
> been considering adding a module parameter that would make the chip
> sleep optional.  Something along these lines:
>
> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
> index 757bd4471fd4..421672488100 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
> @@ -10,6 +10,12 @@
>   #include "cfg80211.h"
>   #include "wlan_cfg.h"
>
> +static bool enable_sleep;
> +module_param(enable_sleep, bool, 0644);
> +MODULE_PARM_DESC(enable_sleep,
> +                "Enable chip sleep whenever the host is done communicating\n"
> +                "\t\t\twith the WILC1000 chip.");
> +
>   static inline bool is_wilc1000(u32 id)
>   {
>          return (id & (~WILC_CHIP_REV_FIELD)) == WILC_1000_BASE_ID;
> @@ -18,13 +24,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 (enable_sleep && acquire == WILC_BUS_ACQUIRE_AND_WAKEUP)
>                  chip_wakeup(wilc);
>   }
>
>   static inline void release_bus(struct wilc *wilc, enum bus_release release)
>   {
> -       if (release == WILC_BUS_RELEASE_ALLOW_SLEEP)
> +       if (enable_sleep && release == WILC_BUS_RELEASE_ALLOW_SLEEP)
>                  chip_allow_sleep(wilc);
>          mutex_unlock(&wilc->hif_cs);
>   }
>
> However, based on the numbers below, I'm now wondering if it wouldn't
> make more sense to remove the chip-sleep code altogether.


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. These API's get used for 
both SPI/SDIO bus.


> 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.


Below command can be used to enable/disable PSM mode.

iw dev wlan0 set power_save on/off


There is a document(link below) which contain details about the power 
consumption measured for WILC1000. Please check, it may be useful.

https://ww1.microchip.com/downloads/en/Appnotes/ATWILC1000-Power-Measurement-for-Wi-Fi-Link-Controller-00002797A.pdf


> On the other hand, chip-sleep makes a huge difference for TX
> throughput and also reduces RX throughput, but to a smaller
> extent.  Specifically, I used ttcp to measure throughput on the
> test system 5 times in a row, both in TX and RX direction
> (TX meaning "ttcp -t" is run from the test system to a desktop
> machine):
> TX throughput ("./ttcp -t DESKTOPIPADDR" on the DUT):
> enable_sleep=1: 16777216 bytes in 41.22 real seconds = 397.50 KB/sec +++
> enable_sleep=1: 16777216 bytes in 40.67 real seconds = 402.81 KB/sec +++
> enable_sleep=1: 16777216 bytes in 41.08 real seconds = 398.83 KB/sec +++
> enable_sleep=1: 16777216 bytes in 41.35 real seconds = 396.25 KB/sec +++
>
> enable_sleep=0: 16777216 bytes in 11.12 real seconds = 1472.78 KB/sec +++
> enable_sleep=0: 16777216 bytes in 10.76 real seconds = 1523.10 KB/sec +++
> enable_sleep=0: 16777216 bytes in 11.83 real seconds = 1385.21 KB/sec +++
> enable_sleep=0: 16777216 bytes in 10.94 real seconds = 1497.66 KB/sec +++
> enable_sleep=0: 16777216 bytes in 10.13 real seconds = 1617.21 KB/sec +++
>
> RX throughput ("./ttcp -r" on the DUT):
>
> enable_sleep=1: 16777216 bytes in 8.44 real seconds = 1941.97 KB/sec +++
> enable_sleep=1: wilc1000, w/ sleep: 16777216 bytes in 12.69 real seconds = 1290.94 KB/sec +++
> enable_sleep=1: 16777216 bytes in 12.79 real seconds = 1280.93 KB/sec +++
> enable_sleep=1: 16777216 bytes in 12.39 real seconds = 1322.33 KB/sec +++
>
> enable_sleep=0: 16777216 bytes in 5.83 real seconds = 2811.94 KB/sec +++
> enable_sleep=0: 16777216 bytes in 5.75 real seconds = 2848.09 KB/sec +++
> enable_sleep=0: 16777216 bytes in 5.97 real seconds = 2744.44 KB/sec +++
> enable_sleep=0: 16777216 bytes in 6.11 real seconds = 2681.96 KB/sec +++
> enable_sleep=0: 16777216 bytes in 6.01 real seconds = 2724.09 KB/sec +++


These throughput number are low in your setup though I am not sure about 
the reason. Actually, the throughput depends on many factors.

Below are the number, I got using iPerf on my setup in open environment 
with/without PS mode. The iPerf test duration was 100sec and the SPI 
max_speed_hz at 48Mhz.

The test is executed with chip-sleep code.


*power-save enabled*

TCP Tx :  [ 4] 0.0-100.1 sec 47.8 MBytes 4.01 Mbits/sec

TCP Rx : [ 4] 0.0-100.3 sec 115 MBytes 9.65 Mbits/sec


*power-save disabled*

TCP Tx :  [ 4] 0.0-100.1 sec 61.9 MBytes 5.19 Mbits/sec

TCP Rx : [ 4] 0.0-100.0 sec 122 MBytes 10.2 Mbits/sec


As I observed, there is an improvement of around ~1Mbps when the PS mode 
is disabled. These numbers would be higher when the test is performed in 
the shield room.

For SDIO bus, the throughput numbers are higher and may cross 20Mbps+ 
for both Tx/Rx path.


>  From what I can tell, the chip-sleep code either isn't working or the chip
> sleep just isn't making any real difference in power consumption.
>
> Given this, is there any reason to keep the chip-sleep code around?


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.


Regards,

Ajay