[PATCH] wilc1000: Allow setting power_save before driver is initialized

[PATCH] wilc1000: Allow setting power_save before driver is initialized

[David Mosberger-Tang]

Without this patch, trying to use:

	iw dev wlan0 set power_save on

before the driver is initialized results in an EIO error.  It is more
useful to simply remember the desired setting and establish it when
the driver is initialized.

Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
---
 drivers/net/wireless/microchip/wilc1000/cfg80211.c | 3 ---
 drivers/net/wireless/microchip/wilc1000/hif.c      | 8 ++++++++
 drivers/net/wireless/microchip/wilc1000/netdev.c   | 3 ++-
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
index dc4bfe7be378..01d607fa2ded 100644
--- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
+++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
@@ -1280,9 +1280,6 @@ static int set_power_mgmt(struct wiphy *wiphy, struct net_device *dev,
 	struct wilc_vif *vif = netdev_priv(dev);
 	struct wilc_priv *priv = &vif->priv;
 
-	if (!priv->hif_drv)
-		return -EIO;
-
 	wilc_set_power_mgmt(vif, enabled, timeout);
 
 	return 0;
diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c b/drivers/net/wireless/microchip/wilc1000/hif.c
index 29a42bc47017..66fd77c816f7 100644
--- a/drivers/net/wireless/microchip/wilc1000/hif.c
+++ b/drivers/net/wireless/microchip/wilc1000/hif.c
@@ -1934,6 +1934,14 @@ int wilc_set_power_mgmt(struct wilc_vif *vif, bool enabled, u32 timeout)
 	int result;
 	s8 power_mode;
 
+	if (!wilc->initialized) {
+		/* Simply remember the desired setting for now; will be
+		 * established by wilc_init_fw_config().
+		 */
+		wilc->power_save_mode = enabled;
+		return 0;
+	}
+
 	if (enabled)
 		power_mode = WILC_FW_MIN_FAST_PS;
 	else
diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
index 4712cd7dff9f..082bed26a981 100644
--- a/drivers/net/wireless/microchip/wilc1000/netdev.c
+++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
@@ -244,6 +244,7 @@ static int wilc1000_firmware_download(struct net_device *dev)
 static int wilc_init_fw_config(struct net_device *dev, struct wilc_vif *vif)
 {
 	struct wilc_priv *priv = &vif->priv;
+	struct wilc *wilc = vif->wilc;
 	struct host_if_drv *hif_drv;
 	u8 b;
 	u16 hw;
@@ -305,7 +306,7 @@ static int wilc_init_fw_config(struct net_device *dev, struct wilc_vif *vif)
 	if (!wilc_wlan_cfg_set(vif, 0, WID_QOS_ENABLE, &b, 1, 0, 0))
 		goto fail;
 
-	b = WILC_FW_NO_POWERSAVE;
+	b = wilc->power_save_mode ? WILC_FW_MIN_FAST_PS : WILC_FW_NO_POWERSAVE;
 	if (!wilc_wlan_cfg_set(vif, 0, WID_POWER_MANAGEMENT, &b, 1, 0, 0))
 		goto fail;
 
-- 
2.25.1

Re: [PATCH] wilc1000: Allow setting power_save before driver is initialized

[David Mosberger-Tang]

Unfortunately, this patch doesn't seem to be sufficient.  From what I
can tell, if power-save mode is turned on before a station is
associated with an access-point, there is no actual power savings.  If
I issue the command after the station is associated, it works perfectly
fine.

Ajay, does this make sense to you?

Best regards,

  --david

On Sun, 2021-12-12 at 01:18 +0000, David Mosberger-Tang wrote:
> Without this patch, trying to use:
> 
> 	iw dev wlan0 set power_save on
> 
> before the driver is initialized results in an EIO error.  It is more
> useful to simply remember the desired setting and establish it when
> the driver is initialized.
> 
> Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
> ---
>  drivers/net/wireless/microchip/wilc1000/cfg80211.c | 3 ---
>  drivers/net/wireless/microchip/wilc1000/hif.c      | 8 ++++++++
>  drivers/net/wireless/microchip/wilc1000/netdev.c   | 3 ++-
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> index dc4bfe7be378..01d607fa2ded 100644
> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c
> @@ -1280,9 +1280,6 @@ static int set_power_mgmt(struct wiphy *wiphy, struct net_device *dev,
>  	struct wilc_vif *vif = netdev_priv(dev);
>  	struct wilc_priv *priv = &vif->priv;
>  
> -	if (!priv->hif_drv)
> -		return -EIO;
> -
>  	wilc_set_power_mgmt(vif, enabled, timeout);
>  
>  	return 0;
> diff --git a/drivers/net/wireless/microchip/wilc1000/hif.c b/drivers/net/wireless/microchip/wilc1000/hif.c
> index 29a42bc47017..66fd77c816f7 100644
> --- a/drivers/net/wireless/microchip/wilc1000/hif.c
> +++ b/drivers/net/wireless/microchip/wilc1000/hif.c
> @@ -1934,6 +1934,14 @@ int wilc_set_power_mgmt(struct wilc_vif *vif, bool enabled, u32 timeout)
>  	int result;
>  	s8 power_mode;
>  
> +	if (!wilc->initialized) {
> +		/* Simply remember the desired setting for now; will be
> +		 * established by wilc_init_fw_config().
> +		 */
> +		wilc->power_save_mode = enabled;
> +		return 0;
> +	}
> +
>  	if (enabled)
>  		power_mode = WILC_FW_MIN_FAST_PS;
>  	else
> diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.c b/drivers/net/wireless/microchip/wilc1000/netdev.c
> index 4712cd7dff9f..082bed26a981 100644
> --- a/drivers/net/wireless/microchip/wilc1000/netdev.c
> +++ b/drivers/net/wireless/microchip/wilc1000/netdev.c
> @@ -244,6 +244,7 @@ static int wilc1000_firmware_download(struct net_device *dev)
>  static int wilc_init_fw_config(struct net_device *dev, struct wilc_vif *vif)
>  {
>  	struct wilc_priv *priv = &vif->priv;
> +	struct wilc *wilc = vif->wilc;
>  	struct host_if_drv *hif_drv;
>  	u8 b;
>  	u16 hw;
> @@ -305,7 +306,7 @@ static int wilc_init_fw_config(struct net_device *dev, struct wilc_vif *vif)
>  	if (!wilc_wlan_cfg_set(vif, 0, WID_QOS_ENABLE, &b, 1, 0, 0))
>  		goto fail;
>  
> -	b = WILC_FW_NO_POWERSAVE;
> +	b = wilc->power_save_mode ? WILC_FW_MIN_FAST_PS : WILC_FW_NO_POWERSAVE;
>  	if (!wilc_wlan_cfg_set(vif, 0, WID_POWER_MANAGEMENT, &b, 1, 0, 0))
>  		goto fail;
>  

Re: [PATCH] wilc1000: Allow setting power_save before driver is initialized

[Ajay.Kathat]

On 13/12/21 02:50, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Unfortunately, this patch doesn't seem to be sufficient.  From what I
> can tell, if power-save mode is turned on before a station is
> associated with an access-point, there is no actual power savings.  If
> I issue the command after the station is associated, it works perfectly
> fine.
>
> Ajay, does this make sense to you?


I think the patch has no effect because wilc_init_fw_config() gets 
called before wilc_set_power_mgmt().

Also, you had mentioned that to enable the PSM(power-save mode), the 
toggling of PS mode is required that means the previous set_power_mgmt() 
was successful. I believe ".set_power_mgmt" cfg80211_ops doesn't get 
called when there is no change from the previous state.

Power-save mode is allowed to be enabled irrespective of station 
association state. Before association, the power consumption should be 
less with PSM enabled compared to PSM disabled. The WLAN automatic power 
save delivery gets enabled after the association with AP.

To check the power measurement before association,  test without 
wpa_supplicant.


Steps:
- load the module
- ifconfig wlan0 up
- iw dev wlan0 set power_save off (check the pwr measurement after PS 
mode disabled)
- iw dev wlan0 set power_save on (check the pwr measurement after PS 
mode enable)


Regards,
Ajay

Re: [PATCH] wilc1000: Allow setting power_save before driver is initialized

[David Mosberger-Tang]

On Wed, 2021-12-15 at 13:01 +0000, Ajay.Kathat@microchip.com wrote:
> On 13/12/21 02:50, David Mosberger-Tang wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Unfortunately, this patch doesn't seem to be sufficient.  From what I
> > can tell, if power-save mode is turned on before a station is
> > associated with an access-point, there is no actual power savings.  If
> > I issue the command after the station is associated, it works perfectly
> > fine.
> > 
> > Ajay, does this make sense to you?
>   <snip>
> Power-save mode is allowed to be enabled irrespective of station 
> association state. Before association, the power consumption should be 
> less with PSM enabled compared to PSM disabled. The WLAN automatic power 
> save delivery gets enabled after the association with AP.
> 
> To check the power measurement before association,  test without 
> wpa_supplicant.
> 
> 
> Steps:
> - load the module
> - ifconfig wlan0 up
> - iw dev wlan0 set power_save off (check the pwr measurement after PS 
> mode disabled)
> - iw dev wlan0 set power_save on (check the pwr measurement after PS 
> mode enable)

It appears wpa_supplicant consistently renders PSM ineffective:

                                (current draw, 1 min avg):
------------------------------  --------------------------
- base case (no module loaded): 16.8 mA
- module loaded & PSM on      : 16.8 mA
- wpa_supplicant started      : 19.6 mA
- PSM on                      : 19.6 mA (no change)
- PSM off                     : 19.6 mA (no change)
- PSM on                      : 15.4 mA

What's strange is when I try this sequence a couple of times in a row,
the device gets into a state where after starting wpa_supplicant, no
amount of PSM on/off commands will get it to enter power-savings mode
any more.  When in that state, only removing wilc1000-spi.ko and adding
it back gets it out of that state.  A power-cycle does not.  Very
confusing.

  --david

Re: [PATCH] wilc1000: Allow setting power_save before driver is initialized

[David Mosberger-Tang]

On Thu, 2021-12-16 at 13:01 +0000, Ajay.Kathat@microchip.com wrote:
> On 16/12/21 11:07, David Mosberger-Tang wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Wed, 2021-12-15 at 13:01 +0000, Ajay.Kathat@microchip.com wrote:
> > > On 13/12/21 02:50, David Mosberger-Tang wrote:
> > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > > > 
> > > > Unfortunately, this patch doesn't seem to be sufficient.  From what I
> > > > can tell, if power-save mode is turned on before a station is
> > > > associated with an access-point, there is no actual power savings.  If
> > > > I issue the command after the station is associated, it works perfectly
> > > > fine.
> > > > 
> > > > Ajay, does this make sense to you?
> > > 
> > >   <snip>
> > > Power-save mode is allowed to be enabled irrespective of station
> > > association state. Before association, the power consumption should be
> > > less with PSM enabled compared to PSM disabled. The WLAN automatic power
> > > save delivery gets enabled after the association with AP.
> > > 
> > > To check the power measurement before association,  test without
> > > wpa_supplicant.
> > > 
> > > 
> > > Steps:
> > > - load the module
> > > - ifconfig wlan0 up
> > > - iw dev wlan0 set power_save off (check the pwr measurement after PS
> > > mode disabled)
> > > - iw dev wlan0 set power_save on (check the pwr measurement after PS
> > > mode enable)
> > 
> > It appears wpa_supplicant consistently renders PSM ineffective:
> > 
> >                                 (current draw, 1 min avg):
> > ------------------------------  --------------------------
> > - base case (no module loaded): 16.8 mA
> > - module loaded & PSM on      : 16.8 mA
> > - wpa_supplicant started      : 19.6 mA
> > - PSM on                      : 19.6 mA (no change)
> > - PSM off                     : 19.6 mA (no change)
> > - PSM on                      : 15.4 mA
>  
> From the above data, it looks like there is no difference with or without PSM
> in your setup. I am not sure if the values are captured correctly. Did you use
> power measurement ports in WILC extension for the current measurements.

Oh, no, not at all!  There is a nice power savings when PSM actually takes hold.
 Current drops from 19.6mA to 15.4mA as shown by the last two lines.

This is average current draw at 120V for the entire board, as my board is not
set up to measure chip current draw alone.

> 
> > What's strange is when I try this sequence a couple of times in a row,
> > the device gets into a state where after starting wpa_supplicant, no
> > amount of PSM on/off commands will get it to enter power-savings mode
> > any more.  When in that state, only removing wilc1000-spi.ko and adding
> > it back gets it out of that state.  A power-cycle does not.  Very
> > confusing.
> 
> Btw, I did a quick test to verify current measurement with PS mode off/on and observed numbers like below
> 
> Tested by making the interface up(ifconfig wlan0 up) then issued 'iw' command to enable/disable PS mode. 
> 
>                               (current draw)
> ------------------------------------------------------
> - PSM off                    : 75.5 mA
> - PSM on                     : 1.28 mA
> 
> 
> I have verified for SPI module with the setup mentioned in link[1] and used power debugger[2]
> 
> 1. https://ww1.microchip.com/downloads/en/Appnotes/ATWILC1000-Power-Measurement-for-Wi-Fi-Link-Controller-00002797A.pdf
> 2. https://www.microchip.com/en-us/development-tool/ATPOWERDEBUGGER

Sure, I assume your measurements are at 3.3V for the chip alone.

But the question is: what happens once you start wpa_supplicant?

  --david

Re: [PATCH] wilc1000: Allow setting power_save before driver is initialized

[Ajay.Kathat]

On 16/12/21 21:00, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Thu, 2021-12-16 at 13:01 +0000, Ajay.Kathat@microchip.com wrote:
>> On 16/12/21 11:07, David Mosberger-Tang wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Wed, 2021-12-15 at 13:01 +0000, Ajay.Kathat@microchip.com wrote:
>>>> On 13/12/21 02:50, David Mosberger-Tang wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> Unfortunately, this patch doesn't seem to be sufficient.  From what I
>>>>> can tell, if power-save mode is turned on before a station is
>>>>> associated with an access-point, there is no actual power savings.  If
>>>>> I issue the command after the station is associated, it works perfectly
>>>>> fine.
>>>>>
>>>>> Ajay, does this make sense to you?
>>>>    <snip>
>>>> Power-save mode is allowed to be enabled irrespective of station
>>>> association state. Before association, the power consumption should be
>>>> less with PSM enabled compared to PSM disabled. The WLAN automatic power
>>>> save delivery gets enabled after the association with AP.
>>>>
>>>> To check the power measurement before association,  test without
>>>> wpa_supplicant.
>>>>
>>>>
>>>> Steps:
>>>> - load the module
>>>> - ifconfig wlan0 up
>>>> - iw dev wlan0 set power_save off (check the pwr measurement after PS
>>>> mode disabled)
>>>> - iw dev wlan0 set power_save on (check the pwr measurement after PS
>>>> mode enable)
>>> It appears wpa_supplicant consistently renders PSM ineffective:
>>>
>>>                                  (current draw, 1 min avg):
>>> ------------------------------  --------------------------
>>> - base case (no module loaded): 16.8 mA
>>> - module loaded & PSM on      : 16.8 mA
>>> - wpa_supplicant started      : 19.6 mA
>>> - PSM on                      : 19.6 mA (no change)
>>> - PSM off                     : 19.6 mA (no change)
>>> - PSM on                      : 15.4 mA
>>  From the above data, it looks like there is no difference with or without PSM
>> in your setup. I am not sure if the values are captured correctly. Did you use
>> power measurement ports in WILC extension for the current measurements.
> Oh, no, not at all!  There is a nice power savings when PSM actually takes hold.
>   Current drops from 19.6mA to 15.4mA as shown by the last two lines.
>
> This is average current draw at 120V for the entire board, as my board is not
> set up to measure chip current draw alone.
>
>>> What's strange is when I try this sequence a couple of times in a row,
>>> the device gets into a state where after starting wpa_supplicant, no
>>> amount of PSM on/off commands will get it to enter power-savings mode
>>> any more.  When in that state, only removing wilc1000-spi.ko and adding
>>> it back gets it out of that state.  A power-cycle does not.  Very
>>> confusing.
>> Btw, I did a quick test to verify current measurement with PS mode off/on and observed numbers like below
>>
>> Tested by making the interface up(ifconfig wlan0 up) then issued 'iw' command to enable/disable PS mode.
>>
>>                                (current draw)
>> ------------------------------------------------------
>> - PSM off                    : 75.5 mA
>> - PSM on                     : 1.28 mA
>>
>>
>> I have verified for SPI module with the setup mentioned in link[1] and used power debugger[2]
>>
>> 1. https://ww1.microchip.com/downloads/en/Appnotes/ATWILC1000-Power-Measurement-for-Wi-Fi-Link-Controller-00002797A.pdf
>> 2. https://www.microchip.com/en-us/development-tool/ATPOWERDEBUGGER
> Sure, I assume your measurements are at 3.3V for the chip alone.
>
> But the question is: what happens once you start wpa_supplicant?


I verified with wpa_supplicant and it seems the power save mode is 
working fine. Tested multiple times with wpa_supplicant running. I 
didn't observe any issue in entering or exiting the power-save mode with 
wpa_supplicant.

Try to verify without wpa_supplicant in your setup to observe if we are 
seeing this same results in that case.

With wpa_supplicant, the current consumption is less when PS mode is 
enabled but it would be more compared to without wpa_supplicant. Because 
wpa_supplicant may have to perform continuous scan till association. And 
after association, the device has to wake up periodically to receive the 
beacon and broadcast packets when PS mode is enabled.

Please refer to the 
"ATWILC1000-Power-Measurement-for-Wi-Fi-Link-Controller" document 
section 4.4 which contains details about the power save scenario when 
connected with AP.

Regards,
Ajay

Re: [PATCH] wilc1000: Allow setting power_save before driver is initialized

[David Mosberger-Tang]

On Thu, 2021-12-23 at 14:02 +0000, Ajay.Kathat@microchip.com wrote:
> 
> I verified with wpa_supplicant and it seems the power save mode is 
> working fine. Tested multiple times with wpa_supplicant running. I 
> didn't observe any issue in entering or exiting the power-save mode with 
> wpa_supplicant.
> 
> Try to verify without wpa_supplicant in your setup to observe if we are 
> seeing this same results in that case.

It doesn't help me if it works without wpa_supplicant.  I need a
reliable way to have power-savings mode in effect when using
wpa_supplicant.

> With wpa_supplicant, the current consumption is less when PS mode is 
> enabled but it would be more compared to without wpa_supplicant.

That's not what I'm talking about though.  The problem is that it seems
to be rather erratic whether issuing the iw power_save command makes a
difference in power-consumption.

I fixed my setup so I can directly measure power consumed rather than
just current (power factor matters).  Again, this is for the entire
device (not just WILC1000).

What I find that when power-saving mode is working as expected, the
device uses an average of 1.1W.  When power-saving mode is not working,
power consumption is about 1.4W, or about 300mW higher.

I tried again *without* the patch applied and, as expected, the patch
doesn't really affect this behavior.

After playing with this for a while, I think I found two sequences that
reliably reproduce the difference.

First, on a freshly booted system and with wilc1000-spi autoloaded by
the kernel, try this sequence (copy & paste the commands):

   /usr/sbin/wpa_supplicant -Bs -iwlan0 -c/etc/wpa_supplicant.conf
   sleep 10
   iw dev wlan0 set power_save on

The above yields a power consumption of 1.4W reliably.  The "sleep 10"
doesn't matter here; the behavior is the same with or without it.  I
tried waiting up to 120 seconds with no difference.

Second, on a freshly booted system and with wilc1000-spi autoloaded by
the kernel, try this sequence (copy & paste the commands):

   /usr/sbin/wpa_supplicant -Bs -iwlan0 -c/etc/wpa_supplicant.conf
   sleep 10
   rmmod wilc1000-spi
   modprobe wilc1000-spi
   sleep 10
   iw dev wlan0 set power_save on

The above yields a power consumption of 1.1W reliably.

Can you reproduce this, or, if not, share the power consumption you see
for the two cases?

  --david

Re: [PATCH] wilc1000: Allow setting power_save before driver is initialized

[Ajay.Kathat]

On 23/12/21 22:38, David Mosberger-Tang wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Thu, 2021-12-23 at 14:02 +0000, Ajay.Kathat@microchip.com wrote:
>> I verified with wpa_supplicant and it seems the power save mode is
>> working fine. Tested multiple times with wpa_supplicant running. I
>> didn't observe any issue in entering or exiting the power-save mode with
>> wpa_supplicant.
>>
>> Try to verify without wpa_supplicant in your setup to observe if we are
>> seeing this same results in that case.
> It doesn't help me if it works without wpa_supplicant.  I need a
> reliable way to have power-savings mode in effect when using
> wpa_supplicant.


Okay. In my setup, I observed that PS mode works with and without 
wpa_supplicant. Now tested by following your command sequence.


>> With wpa_supplicant, the current consumption is less when PS mode is
>> enabled but it would be more compared to without wpa_supplicant.
> That's not what I'm talking about though.  The problem is that it seems
> to be rather erratic whether issuing the iw power_save command makes a
> difference in power-consumption.
>
> I fixed my setup so I can directly measure power consumed rather than
> just current (power factor matters).  Again, this is for the entire
> device (not just WILC1000).
>
> What I find that when power-saving mode is working as expected, the
> device uses an average of 1.1W.  When power-saving mode is not working,
> power consumption is about 1.4W, or about 300mW higher.
>
> I tried again *without* the patch applied and, as expected, the patch
> doesn't really affect this behavior.
>
> After playing with this for a while, I think I found two sequences that
> reliably reproduce the difference.
>
> First, on a freshly booted system and with wilc1000-spi autoloaded by
> the kernel, try this sequence (copy & paste the commands):
>
>     /usr/sbin/wpa_supplicant -Bs -iwlan0 -c/etc/wpa_supplicant.conf
>     sleep 10
>     iw dev wlan0 set power_save on
>
> The above yields a power consumption of 1.4W reliably.  The "sleep 10"
> doesn't matter here; the behavior is the same with or without it.  I
> tried waiting up to 120 seconds with no difference.


I have tested by making the WILC as build-in module to insert driver 
automatically at boot-up. I hope it should be fine. Because I have 
already tested as loadable module earlier.

Below are the number observed
------------------------------ --------------------------
- before starting wpa_supplicant             : ~16.3 mA
- wpa_supplicant started                         : ~40 mA
- PSM on                                                  :  ~6 mA


The 'sleep 10' would have no impact in my setup because I have measured 
the current consumption for wilc1000 chip.

I have shared the screenshot at https://postimg.cc/67S41dkb


> Second, on a freshly booted system and with wilc1000-spi autoloaded by
> the kernel, try this sequence (copy & paste the commands):
>
>     /usr/sbin/wpa_supplicant -Bs -iwlan0 -c/etc/wpa_supplicant.conf
>     sleep 10
>     rmmod wilc1000-spi
>     modprobe wilc1000-spi
>     sleep 10
>     iw dev wlan0 set power_save on
>
> The above yields a power consumption of 1.1W reliably.
>
> Can you reproduce this, or, if not, share the power consumption you see
> for the two cases?


Second case was verified earlier and also tested by toggle power_save 
mode on/off many times and I observe the numbers are in same range for 
both cases.

Regards,
Ajay

Re: [PATCH] wilc1000: Allow setting power_save before driver is initialized

[David Mosberger-Tang]

Ajay,

On Fri, 2021-12-24 at 16:20 +0000, Ajay.Kathat@microchip.com wrote:
> On 23/12/21 22:38, David Mosberger-Tang wrote:
> > First, on a freshly booted system and with wilc1000-spi autoloaded by
> > the kernel, try this sequence (copy & paste the commands):
> > 
> >     /usr/sbin/wpa_supplicant -Bs -iwlan0 -c/etc/wpa_supplicant.conf
> >     sleep 10
> >     iw dev wlan0 set power_save on
> > 
> > The above yields a power consumption of 1.4W reliably.  The "sleep 10"
> > doesn't matter here; the behavior is the same with or without it.  I
> > tried waiting up to 120 seconds with no difference.
> 
> I have tested by making the WILC as build-in module to insert driver 
> automatically at boot-up. I hope it should be fine. Because I have 
> already tested as loadable module earlier.
> 
> Below are the number observed
> ------------------------------ --------------------------
> - before starting wpa_supplicant             : ~16.3 mA
> - wpa_supplicant started                         : ~40 mA
> - PSM on                                                  :  ~6 mA
> 
> 
> The 'sleep 10' would have no impact in my setup because I have measured 
> the current consumption for wilc1000 chip.
> 
> I have shared the screenshot at https://postimg.cc/67S41dkb

Huh, that's curious.  I definitely cannot reproduce this.  To match
your setup as closely as possibly, I also built wilc1000-spi into the
kernel, but that makes no difference (as expected).

What kernel version are you on?  I switched to wireless-drivers-next as
of today (latest commit d430dffbe9dd30759f3c64b65bf85b0245c8d8ab).

With this kernel, the numbers are about 100mW lower than reported
before, but the relative behavior is the same: about 300mW higher
power-consumption when PSM is not taking effect properly.

To recap, back with wilc1000-spi being a module again, after freshly
booting the system and issuing this commands:

   /usr/sbin/wpa_supplicant -Bs -iwlan0 -c/etc/wpa_supplicant.conf
   /usr/sbin/iw dev wlan0 set power_save on

I see a power-consumption of about 1.25W.  PSM on/off makes no
difference in this state.  Then, if I issue the commands:

rmmod wilc1000-spi
modprobe wilc1000-spi
sleep 10
iw dev wlan0 set power_save on

power-consumption drops to about 0.9W.

Here is a screenshot that shows the annotated power-measurements:

   https://postimg.cc/3dbKSGht

Apart from kernel version, the only things that I can think of that'd
be different is that we don't have the ENABLE pin wired to a GPIO.
 Instead, the chip is always enabled.  I doubt this would explain the
difference (~RESET is wired to a GPIO).


  --david

Re: [PATCH] wilc1000: Allow setting power_save before driver is initialized

[David Mosberger-Tang]

Ajay,

On Thu, 2021-12-30 at 16:24 +0000, Ajay.Kathat@microchip.com wrote:
> On 24/12/21 23:08, David Mosberger-Tang wrote:
> > On Fri, 2021-12-24 at 16:20 +0000, Ajay.Kathat@microchip.com wrote:
> > > On 23/12/21 22:38, David Mosberger-Tang wrote:
> > > > First, on a freshly booted system and with wilc1000-spi autoloaded by
> > > > the kernel, try this sequence (copy & paste the commands):
> > > > 
> > > >     /usr/sbin/wpa_supplicant -Bs -iwlan0 -c/etc/wpa_supplicant.conf
> > > >     sleep 10
> > > >     iw dev wlan0 set power_save on
> > > > 
> > > > The above yields a power consumption of 1.4W reliably.  The "sleep 10"
> > > > doesn't matter here; the behavior is the same with or without it.  I
> > > > tried waiting up to 120 seconds with no difference.
> > > 
> > > I have tested by making the WILC as build-in module to insert driver
> > > automatically at boot-up. I hope it should be fine. Because I have
> > > already tested as loadable module earlier.
> > > 
> > > Below are the number observed
> > > ------------------------------ --------------------------
> > > - before starting wpa_supplicant             : ~16.3 mA
> > > - wpa_supplicant started                         : ~40 mA
> > > - PSM on                                                  :  ~6 mA
> > > 
> > > 
> > > The 'sleep 10' would have no impact in my setup because I have measured
> > > the current consumption for wilc1000 chip.
> > > 
> > > I have shared the screenshot at https://postimg.cc/67S41dkb
> > 
> > Huh, that's curious.  I definitely cannot reproduce this.  To match
> > your setup as closely as possibly, I also built wilc1000-spi into the
> > kernel, but that makes no difference (as expected).
> > 
> > What kernel version are you on?  I switched to wireless-drivers-next as
> > of today (latest commit d430dffbe9dd30759f3c64b65bf85b0245c8d8ab).
> 
> I have tested with 5.10 as well as with 5.16-rc1(commit#
> fa55b7dcdc43). I don't think WILC1000 chip PS gets affected because
> of a specific kernel version. 
> 
> > With this kernel, the numbers are about 100mW lower than reported
> > before, but the relative behavior is the same: about 300mW higher
> > power-consumption when PSM is not taking effect properly.
> > 
> > To recap, back with wilc1000-spi being a module again, after freshly
> > booting the system and issuing this commands:
> > 
> >    /usr/sbin/wpa_supplicant -Bs -iwlan0 -c/etc/wpa_supplicant.conf
> >    /usr/sbin/iw dev wlan0 set power_save on
> > 
> > I see a power-consumption of about 1.25W.  PSM on/off makes no
> > difference in this state.  Then, if I issue the commands:
> > 
> > rmmod wilc1000-spi
> > modprobe wilc1000-spi
> > sleep 10
> > iw dev wlan0 set power_save on
> > 
> > power-consumption drops to about 0.9W.
> > 
> > Here is a screenshot that shows the annotated power-measurements:
> > 
> >    https://postimg.cc/3dbKSGht
> > 
> > Apart from kernel version, the only things that I can think of that'd
> > be different is that we don't have the ENABLE pin wired to a GPIO.
> >  Instead, the chip is always enabled.  I doubt this would explain the
> > difference (~RESET is wired to a GPIO).
> 
> When PS mode is enabled, the host can wake-up the chip, if there is
> some data to transfer. Just check, if there is any network
> application trying to access WILC (send continuous data) in your
> setup.

That shouldn't be an issue.  To confirm, in the output below, I
included the ifconfig stats so you can see that there is virtually no
traffic during the measurement periods.

>  Also, you have mentioned that removing the module(without killing
> the wpa_supplicant) worked so I guess, it might be possible that the
> application got interrupted when the module was removed and didn't
> start when the module was inserted again. 

There are no applications running on the system at all.  I tested the
two command sequences in the exact same setup, with everything that
normally runs on our system disabled.  CPU utilization is basically 0%.

> Try disabling CONFIG_CFG80211_DEFAULT_PS config to check if it has any impact.

I already had CONFIG_CFG80211_DEFAULT_PS disabled.  It makes no
difference, except that with CONFIG_CFG80211_DEFAULT_PS enabled, you
have to issue "power_save off" before "power_save on" to get PSM to
take effect since cfg80211 thinks power-save is already enabled when
it's not.

To mirror your setup more closely, I installed a 1 ohm shunt resistor
on the 3.3V supply to the WILC1000 so I can measure current draw of the
WILC1000 alone.  This is just for paranoia's sake to make sure there is
nothing wonky going on outside of the WILC1000 (there is not).  So the
current draws mentioned below are at 3.3V and DC average measures
across 1 minute.

First test command sequence (power saving fails to take effect):

   ------------------------------------------------------------------
   REBOOT SYSTEM
   # ifconfig wlan0 | tail -5
             RX packets:0 errors:0 dropped:0 overruns:0 frame:0
             TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
             collisions:0 txqueuelen:1000
             RX bytes:0 (0.0 B)  TX bytes:0 (0.0 B)
   # /usr/sbin/wpa_supplicant -Bs -iwlan0 -c/etc/wpa_supplicant.conf
   # iw dev wlan0 set power_save on
   # sleep 60
   # ifconfig wlan0 | tail -5
             RX packets:51 errors:0 dropped:0 overruns:0 frame:0
             TX packets:37 errors:0 dropped:0 overruns:0 carrier:0
             collisions:0 txqueuelen:1000
             RX bytes:5980 (5.8 KiB)  TX bytes:5425 (5.2 KiB)
   ------------------------------------------------------------------

with the above command sequence, current-draw remains pretty much
constant at 76mA.


Second test command sequence (power saving successfully takes effect):

   ------------------------------------------------------------------
   REBOOT SYSTEM
   # ifconfig wlan0 | tail -5
             RX packets:0 errors:0 dropped:0 overruns:0 frame:0
             TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
             collisions:0 txqueuelen:1000
             RX bytes:0 (0.0 B)  TX bytes:0 (0.0 B)
   # /usr/sbin/wpa_supplicant -Bs -iwlan0 -c/etc/wpa_supplicant.conf
   # sleep 15
   # iw dev wlan0 set power_save on
   # sleep 60
   # ifconfig wlan0 | tail -5
             RX packets:41 errors:0 dropped:0 overruns:0 frame:0
             TX packets:34 errors:0 dropped:0 overruns:0 carrier:0
             collisions:0 txqueuelen:1000
             RX bytes:5007 (4.8 KiB)  TX bytes:5436 (5.3 KiB)
   ------------------------------------------------------------------

with the above command sequence, current-draw fluctuates quite a bit,
but averages to somewhere around 15mA.

The exact amount of time needed between starting wpa_supplicant and
issuing the PSM on command fluctuates a bit.  Sometimes 9 seconds is
enough, but oftentimes a pause of 10-15 seconds is required.

A screenshot of the current draw for this command sequence is graphed
here:

  https://postimg.cc/bZc1v0jR

Oh, and just at the off-chance that the association details matters:

      # wpa_cli status
      Selected interface 'wlan0'
      bssid=d8:07:b6:af:4b:e4
      freq=2442
      ssid=MOSTANG
      id=0
      mode=station
      pairwise_cipher=CCMP
      group_cipher=CCMP
      key_mgmt=WPA2-PSK
      wpa_state=COMPLETED
      p2p_device_address=f8:f0:05:62:76:48
      address=f8:f0:05:62:76:48
      uuid=22b21bb3-ec5a-5038-aa7f-911a2b012173

  --david