David Mosberger-Tang <davidm@xxxxxxxxxx> writes:
> 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.
>
> 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.
>
> 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 +++
>
>>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?
If it doesn't work I would just remove it to keep things simple. It can
always added back if somebody fixes it.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
