[RFC] wl1251: Add support for idle mode

[RFC] wl1251: Add support for idle mode

[Jarkko Nikula]

On Nokia N900 the wl1251 consumes the most power when the interface is up
but not associated to access point (that supports PSM). In terms of battery
current consumtion, the consumtion is ~180 mA higher when the interface is
up but not associated and only ~5 mA higher when associated compared to
interface down and driver not loaded cases.

This patch adds support for the mac80211 idle notifications. Chip is put into
idle very much the same way when entering into PSM by utilizing the Extreme
Low Power (ELP) mode. I.e. idle is entered by first setting necessary
conditions with psm flag set and then via calling the wl1251_ps_elp_sleep.

It seems it is just enough the authorize ELP mode followed by
CMD_DISCONNECT (thanks to Kalle Valo about the idea to use it).
Without disconnect command the chip remains somewhat active and stays
consuming ~20 mA. Idle mode is left by same way than PSM. The wl1251_join
call is used to revert the CMD_DISCONNECT. Without it association to AP
doesn't work when trying second time.

With this patch the interface up but not associated case the battery current
consumtion is less than 1 mA higher compared to interface down case.

Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
Cc: Kalle Valo <kvalo@adurom.com>
---
- Developed on top of vanilla commit 89078d572eb9ce8d4c04264b8b0ba86de0d74c8f.
- I don't have specs for the chip and this patch is developed heavily by
  trial and error method. Anyway, I believe the chip enters into idle properly
  as the consumtion is very near to interface down case.
- I have verified the patch by making sure that the driver can scan the APs
  and connect/disconnect multiple times after the idle mode was entered.
- There is still problem that the consumtion jumps again ~+180 mA after
  disconnection happens. Reason is the BSS_LOSE_EVENT which puts the chip
  again into active mode and driver doesn't receive an idle mode request
  after that. For me it looks the driver doesn't notify the mac80211
  properly and thus no idle mode request is coming. As this patch doesn't
  affect that I see that is better to handle separately.
---
 drivers/net/wireless/wl1251/cmd.h  |    3 ++-
 drivers/net/wireless/wl1251/main.c |   16 ++++++++++++++++
 drivers/net/wireless/wl1251/ps.c   |   13 +++++++++++++
 3 files changed, 31 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/wl1251/cmd.h b/drivers/net/wireless/wl1251/cmd.h
index e5c74c6..7ca0768 100644
--- a/drivers/net/wireless/wl1251/cmd.h
+++ b/drivers/net/wireless/wl1251/cmd.h
@@ -314,7 +314,8 @@ struct wl1251_cmd_vbm_update {
 
 enum wl1251_cmd_ps_mode {
 	STATION_ACTIVE_MODE,
-	STATION_POWER_SAVE_MODE
+	STATION_POWER_SAVE_MODE,
+	STATION_IDLE,
 };
 
 struct wl1251_cmd_ps_params {
diff --git a/drivers/net/wireless/wl1251/main.c b/drivers/net/wireless/wl1251/main.c
index 12c9e63..86c0b0e 100644
--- a/drivers/net/wireless/wl1251/main.c
+++ b/drivers/net/wireless/wl1251/main.c
@@ -639,6 +639,22 @@ static int wl1251_op_config(struct ieee80211_hw *hw, u32 changed)
 		}
 	}
 
+	if (changed & IEEE80211_CONF_CHANGE_IDLE) {
+		if (conf->flags & IEEE80211_CONF_IDLE) {
+			ret = wl1251_ps_set_mode(wl, STATION_IDLE);
+			if (ret < 0)
+				goto out_sleep;
+		} else {
+			ret = wl1251_ps_set_mode(wl, STATION_ACTIVE_MODE);
+			if (ret < 0)
+				goto out_sleep;
+			ret = wl1251_join(wl, wl->bss_type, wl->channel,
+					  wl->beacon_int, wl->dtim_period);
+			if (ret < 0)
+				goto out_sleep;
+		}
+	}
+
 	if (conf->power_level != wl->power_level) {
 		ret = wl1251_acx_tx_power(wl, conf->power_level);
 		if (ret < 0)
diff --git a/drivers/net/wireless/wl1251/ps.c b/drivers/net/wireless/wl1251/ps.c
index 9cc5147..2b035e0 100644
--- a/drivers/net/wireless/wl1251/ps.c
+++ b/drivers/net/wireless/wl1251/ps.c
@@ -138,6 +138,19 @@ int wl1251_ps_set_mode(struct wl1251 *wl, enum wl1251_cmd_ps_mode mode)
 
 		wl->psm = 1;
 		break;
+	case STATION_IDLE:
+		wl1251_debug(DEBUG_PSM, "entering idle");
+
+		ret = wl1251_acx_sleep_auth(wl, WL1251_PSM_ELP);
+		if (ret < 0)
+			return ret;
+
+		ret = wl1251_cmd_template_set(wl, CMD_DISCONNECT, NULL, 0);
+		if (ret < 0)
+			return ret;
+
+		wl->psm = 1;
+		break;
 	case STATION_ACTIVE_MODE:
 	default:
 		wl1251_debug(DEBUG_PSM, "leaving psm");
-- 
1.7.0.4

Re: [RFC] wl1251: Add support for idle mode

[Kalle Valo]

Moro Jarkko,

Jarkko Nikula <jhnikula@gmail.com> writes:

> This patch adds support for the mac80211 idle notifications. Chip is put into
> idle very much the same way when entering into PSM by utilizing the Extreme
> Low Power (ELP) mode. I.e. idle is entered by first setting necessary
> conditions with psm flag set and then via calling the wl1251_ps_elp_sleep.

Good stuff! Thank you very much for working on this.

> It seems it is just enough the authorize ELP mode followed by
> CMD_DISCONNECT (thanks to Kalle Valo about the idea to use it).

Actually I was expecting that you would first issue the disconnect
command and only then enable elp. The thing is that for sending
commands we need to wakeup firmware from elp, so enabling elp last
makes more sense. Was there a reason why you did it in this order?

> - There is still problem that the consumtion jumps again ~+180 mA after
>   disconnection happens. Reason is the BSS_LOSE_EVENT which puts the chip
>   again into active mode and driver doesn't receive an idle mode request
>   after that. For me it looks the driver doesn't notify the mac80211
>   properly and thus no idle mode request is coming.

Yes, that's an existing bug in wl1251. It should call either
mac80211_beacon_loss() or mac80211_connection_loss() after the event.

> As this patch doesn't affect that I see that is better to handle
> separately.

Agreed.

> --- a/drivers/net/wireless/wl1251/cmd.h
> +++ b/drivers/net/wireless/wl1251/cmd.h
> @@ -314,7 +314,8 @@ struct wl1251_cmd_vbm_update {
>  
>  enum wl1251_cmd_ps_mode {
>  	STATION_ACTIVE_MODE,
> -	STATION_POWER_SAVE_MODE
> +	STATION_POWER_SAVE_MODE,
> +	STATION_IDLE,
>  };

My idea was that cmd and acx files contain only the firmware
interface, nothing more. I assume that firmware knows nothing this new
value, right?

Another way to do this would be that you create a new state enum to
wl1251.h with the three states above and then change
wl1251_ps_set_mode(), and it's callers, to use the new enum. That way
you can keep cmd.h intact.

Also I'm guessing that in the future we need to store the new state
enum to struct wl1251.

-- 
Kalle Valo

Re: [RFC] wl1251: Add support for idle mode

[Jarkko Nikula]

Hi

On Wed, 30 Mar 2011 09:42:13 +0300
Kalle Valo <kvalo@adurom.com> wrote:

> > It seems it is just enough the authorize ELP mode followed by
> > CMD_DISCONNECT (thanks to Kalle Valo about the idea to use it).
> 
> Actually I was expecting that you would first issue the disconnect
> command and only then enable elp. The thing is that for sending
> commands we need to wakeup firmware from elp, so enabling elp last
> makes more sense. Was there a reason why you did it in this order?
> 
Actually order is this. Both PSM and idle are activated via
wl1251_op_config so those precommands etc are executed first and the ELP
activation after that. See

wl1251_op_config
-> wl1251_ps_elp_wakeup
...
-> wl1251_ps_set_mode (those precommands for PSM and idle)
-> wl1251_ps_elp_sleep (+ called from other functions as well)
  -> ieee80211_queue_delayed_work(wl->hw, &wl->elp_work, delay);
    -> wl1251_elp_work (ELP activated here)

> > --- a/drivers/net/wireless/wl1251/cmd.h
> > +++ b/drivers/net/wireless/wl1251/cmd.h
> > @@ -314,7 +314,8 @@ struct wl1251_cmd_vbm_update {
> >  
> >  enum wl1251_cmd_ps_mode {
> >  	STATION_ACTIVE_MODE,
> > -	STATION_POWER_SAVE_MODE
> > +	STATION_POWER_SAVE_MODE,
> > +	STATION_IDLE,
> >  };
> 
> My idea was that cmd and acx files contain only the firmware
> interface, nothing more. I assume that firmware knows nothing this new
> value, right?
> 
Ok, this is good to know.

> Another way to do this would be that you create a new state enum to
> wl1251.h with the three states above and then change
> wl1251_ps_set_mode(), and it's callers, to use the new enum. That way
> you can keep cmd.h intact.
> 
> Also I'm guessing that in the future we need to store the new state
> enum to struct wl1251.
> 
This sounds much better idea. E.g. now the code is a bit unclear as the
flag psm is used also when in idle. I think the flag psm is better to
replace by some ps_mode state variable since then it can be used to
distinguish the PSM and idle if there's ever need to do so and no risk
that psm and new variable goes out of sync.

-- 
Jarkko

Re: [RFC] wl1251: Add support for idle mode

[Kalle Valo]

Jarkko Nikula <jhnikula@gmail.com> writes:

> On Wed, 30 Mar 2011 09:42:13 +0300
> Kalle Valo <kvalo@adurom.com> wrote:
>
>> Actually I was expecting that you would first issue the disconnect
>> command and only then enable elp. The thing is that for sending
>> commands we need to wakeup firmware from elp, so enabling elp last
>> makes more sense. Was there a reason why you did it in this order?
>> 
> Actually order is this. Both PSM and idle are activated via
> wl1251_op_config so those precommands etc are executed first and the ELP
> activation after that. See
>
> wl1251_op_config
> -> wl1251_ps_elp_wakeup
> ...
> -> wl1251_ps_set_mode (those precommands for PSM and idle)
> -> wl1251_ps_elp_sleep (+ called from other functions as well)
>   -> ieee80211_queue_delayed_work(wl->hw, &wl->elp_work, delay);
>     -> wl1251_elp_work (ELP activated here)

Ah, of course. Forgot that we delay elp.

>> My idea was that cmd and acx files contain only the firmware
>> interface, nothing more. I assume that firmware knows nothing this new
>> value, right?
>> 
> Ok, this is good to know.

My bad, I should document this properly in the headers.

>> Another way to do this would be that you create a new state enum to
>> wl1251.h with the three states above and then change
>> wl1251_ps_set_mode(), and it's callers, to use the new enum. That way
>> you can keep cmd.h intact.
>> 
>> Also I'm guessing that in the future we need to store the new state
>> enum to struct wl1251.
>> 
> This sounds much better idea. E.g. now the code is a bit unclear as the
> flag psm is used also when in idle. I think the flag psm is better to
> replace by some ps_mode state variable since then it can be used to
> distinguish the PSM and idle if there's ever need to do so and no risk
> that psm and new variable goes out of sync.

Good idea, this way it is more robust.

-- 
Kalle Valo