Ticket #3355 (closed enhancement: fixed)

Opened 11 months ago

Last modified 2 months ago

Wireless shutdown on Laptop Lid Close

Reported by: kimquirk Owned by: dsaxena
Priority: high Milestone: 8.2.0 (was Update.2)
Component: kernel Version:
Keywords: update.1? Cc: jg, mbletsas, dwmw2, ashish, wad, rsmith, ramyac@…, rchokshi@…, cjb
Action Needed: Verified: no
Blocked By: Blocking:

Description

For Trial-3 and FRS, when the laptop's lid is closed we need to go into suspend mode, including shut down of wireless.

Attachments

q204v.rom (1.0 MB) - added by rsmith 8 months ago.
Test system firmware with wlan reset assert

Change History

  Changed 11 months ago by cjb

  • owner changed from hughsient@… to cjb

I can do this. Are we sure, though, given that it's unstable?

  Changed 11 months ago by kimquirk

  • milestone changed from Trial-3 to First Deployment, V1.0

Moving to FRS, when hopefully things will be more stable with suspend/resume.

  Changed 10 months ago by kimquirk

  • keywords killjoy? added

  Changed 10 months ago by kimquirk

  • keywords update.1? added; killjoy? removed

  Changed 10 months ago by kimquirk

  • milestone changed from Update.2 to Update.1

  Changed 10 months ago by kimquirk

  • owner changed from cjb to carrano

Ricardo, can you please tell if the mesh beacons are turned off in the 623? Please respond and then you can re-assign this bug to cjb.

  Changed 10 months ago by carrano

  • owner changed from carrano to cjb

Confirming that beacons can indeed be turned off in 623.

iwpriv eth0 bcn_control 0

It's also possible to change beacon interval

iwpriv ethX bcn_control 1 $INTERVAL

$INTERVAL in ms (20 to 1000)

  Changed 10 months ago by cjb

  • cc mbletsas added

This is waiting on #4665. Michailis?

  Changed 10 months ago by mbletsas

This doesn't really depend on #4665.

Remember the real discussion here is whether we wakeup on wireless events or not, not whether we turn the wireless off completely (that's a control panel issue).

Nevertheless we should do the measurements on tinderbox.

M.

  Changed 10 months ago by cjb

Michail,

the real discussion here is whether we wakeup on wireless events or not, not whether we turn the wireless off completely (that's a control panel issue).

Jim and Kim had decided that we should *always* turn off the wireless completely on "sleep" (which is lid close or power button), because otherwise keeping it on depletes the battery too quickly, at 900mW.

I hope that disabling mesh beacons will halve that number, which should be more acceptable. That is why I'm asking you whether we can turn off mesh beacons now; if we can, and tinderbox shows it to save as much power as we hope it will, I will not implement a complete wireless shutdown on lid close straight-away.

I will still implement "go back to sleep after a wireless wakeup after lid close", which is a different bug.

  Changed 10 months ago by mbletsas

Chris,

Fine with me, however we need to do the tinderbox testing with the beacon frequency. I don't think that just that is going to take the consumption down by 40%, but boy, I would be very excited to be completely wrong on this one!

M.

  Changed 10 months ago by jg

Why is the default for mesh beacons on, rather than off?

  Changed 8 months ago by dwmw2

  • cc dwmw2, ashish added

There is a 'Deep Sleep' mode for the wireless, from which it is woken only by a GPIO.

Is this implemented in our firmware, and is the GPIO hooked up?

follow-ups: ↓ 15 ↓ 16   Changed 8 months ago by mbletsas

I don:t think we ever hooked up such a GPIO. We have one the reset the wireless module. The schematics will tell for sure, but I don't remember such a GPIO to the wlan module.

M

in reply to: ↑ 14   Changed 8 months ago by cjb

Replying to mbletsas:

I don:t think we ever hooked up such a GPIO. We have one the reset the wireless module. The schematics will tell for sure, but I don't remember such a GPIO to the wlan module.

Thanks. This bug calls for a wireless shutdown on lid close -- how should we achieve the shutdown?

in reply to: ↑ 14   Changed 8 months ago by dwmw2

Replying to mbletsas:

I don:t think we ever hooked up such a GPIO. We have one the reset the wireless module. The schematics will tell for sure, but I don't remember such a GPIO to the wlan module.

I think we have some GPIOs which we're not really using at the moment -- originally intended for battery level indication, I believe. Although there's also a GPIO which we use for wakeup from HOST_SLEEP. Perhaps we can also use that for wakeup for DEEP_SLEEP?

  Changed 8 months ago by mbletsas

I have to look at the schematics, however we never intended to do that out-of-band (via a GPIO), the plan was to use USB commands to put the wlan module into sleep mode. The only relevant GPIO is the one that wakes up the host when traffic arrives.

M.

follow-up: ↓ 21   Changed 8 months ago by cjb

Any further ideas on how I could implement this in the very near future?

  Changed 8 months ago by mbletsas

As a normal USB peripheral sleep process.

M.

  Changed 8 months ago by cjb

Hi Michail,

As a normal USB peripheral sleep process.

I'm confused -- the point of this bug is to stop the 8388 from drawing power while we are in sleep mode. If we could tolerate the 8388 drawing power while we are asleep, there wouldn't be a bug here. Will the "normal USB peripheral sleep process" stop the 8388 from drawing power?

If it won't, what should we do for Update.1?

in reply to: ↑ 18   Changed 8 months ago by dwmw2

Replying to cjb:

Any further ideas on how I could implement this in the very near future?

By using Deep Sleep mode would be my guess. Ashish, is Deep Sleep mode supported in our firmware, and how do we wake up from it?

Failing that, perhaps we could assert the RESET line on going to sleep and release it on wake.

  Changed 8 months ago by ashish

Deep Sleep mode is not supported in the firmware.

follow-up: ↓ 26   Changed 8 months ago by dwmw2

Ashish, thank you. Can you recommend the best way to quiesce the device while we sleep?

Would it suffice to assert (and hold) the RESET line, then release it when we awake? Or should the (CMD_802_11_RESET, CMD_ACT_HALT) command do it? We don't busy-wait in boot2 code?

The RESET line would be easier, because then the device will disappear from the USB bus and reappear as a 'different' device, and the driver will cope gracefully. If we issue the RESET/HALT command, I believe we then have to cope with reloading the firmware at runtime on the _same_ logical instance of the device. Which is something we'd like to do, but wasn't necessarily at the top of my priority list for this week.

What would give the lowest power consumption?

  Changed 8 months ago by dwmw2

  • cc wad, richard added

  Changed 8 months ago by dwmw2

  • cc rsmith added; richard removed

in reply to: ↑ 23 ; follow-up: ↓ 27   Changed 8 months ago by ashish

  • cc ramyac@…, rchokshi@… added

Replying to dwmw2:

Ashish, thank you. Can you recommend the best way to quiesce the device while we sleep? Would it suffice to assert (and hold) the RESET line, then release it when we awake? Or should the (CMD_802_11_RESET, CMD_ACT_HALT) command do it? We don't busy-wait in boot2 code? The RESET line would be easier, because then the device will disappear from the USB bus and reappear as a 'different' device, and the driver will cope gracefully. If we issue the RESET/HALT command, I believe we then have to cope with reloading the firmware at runtime on the _same_ logical instance of the device. Which is something we'd like to do, but wasn't necessarily at the top of my priority list for this week. What would give the lowest power consumption?

I guess you are talking of hardware RESET line, which I believe should be easier, and since device is out of USB bus this approach should consume minimum power. However, device state will be lost and a fresh device --including firmware download from host-- will appear on wakeup. I think RESET/HALT command may not be feasible at this point. I will check this with our hardware folks, all the possibilities, and will update you soon.

in reply to: ↑ 26   Changed 8 months ago by dwmw2

Replying to ashish:

I guess you are talking of hardware RESET line, which I believe should be easier, and since device is out of USB bus this approach should consume minimum power. However, device state will be lost and a fresh device --including firmware download from host-- will appear on wakeup.

Yes. That's actually a good feature; we know the driver copes with that situation well, because it sees a completely new instance of the hardware and doesn't expect any state to be preserved.

I think RESET/HALT command may not be feasible at this point.

RESET/HALT works for resetting the active antennae so that we can reload the firmware on them, at least most of the time. The active antennae actually come out of reset after 5 seconds and run their internal firmware, but that shouldn't happen on the XO. This approach would require more work in the driver, because I believe it doesn't cause the device to detach and reappear on the USB bus.

I will check this with our hardware folks, all the possibilities, and will update you soon.

Thank you.

  Changed 8 months ago by rchokshi

For the goal of this trac ticket, asserting the RESET line is the best option.

  Changed 8 months ago by dwmw2

OK, thank you. Richard, please can we have a way to do that?

follow-up: ↓ 31   Changed 8 months ago by cjb

  • owner changed from cjb to rsmith
  • component changed from power manager (OHM) to embedded controller

Reassigning to Richard; once the EC supports this reset, the kernel needs to expose it to OHM, and then OHM needs to call it.

Since that's a lot of interaction, this bug is in danger of slipping past Update.1.

in reply to: ↑ 30 ; follow-up: ↓ 32   Changed 8 months ago by rsmith

Replying to cjb:

Since that's a lot of interaction, this bug is in danger of slipping past Update.1.

I believe I _finally_ have things fixed so I'll get @#$%@@# CC mail from trac. My apologies for not commenting on this sooner. I'll know if i have it fixed when I submit this note.

The EC side of this is trivial. Its 2 extra EC commands. I'll do it right after I finish this. The issue here will be the fact that we still sometimes have problems communicating with the EC. If you put the WLAN in reset and then have EC issues then you are just out of luck until you power cycle/reset the EC.

Changed 8 months ago by rsmith

Test system firmware with wlan reset assert

in reply to: ↑ 31   Changed 8 months ago by rsmith

  • type changed from defect to enhancement

Replying to rsmith:

The EC side of this is trivial. Its 2 extra EC commands. I'll do it right after I finish this.

Done. 'q204v.rom' Attached. "Works for me" athough once I bring it out of reset the kernel never re-inits the device.

Rather than 2 extra commands I just did one. EC command 0x35 will assert the WLAN reset line. To clear it just do a normal wlan reset via EC 0x25. 0x25 will try to assert the already asserted line and then de-assert 1 ms later.

  Changed 8 months ago by cjb

  • cc cjb added

  Changed 8 months ago by cjb

  • owner changed from rsmith to dwmw2

Back to Dave, I guess?

We'll need a way to trigger this reset from userspace/OHM -- sysfs would be fine with me.

  Changed 7 months ago by rsmith

  • component changed from embedded controller to kernel

  Changed 7 months ago by dilinger

Make sure this commands ends up documented in http://wiki.laptop.org/go/Ec_specification , please.

  Changed 2 months ago by dsaxena

  • owner changed from dwmw2 to dsaxena
  • status changed from new to assigned

  Changed 2 months ago by cjb

Here's a patch for the kernel interface to the EC commands:

[PATCH #3355] Add sysfs support for powering down the OLPC 88W8838 wireless chip.

This uses the OLPC EC 0x35/0x25 interface.  

Question:  is simple_strtoul() safe here?

Signed-off-by: Chris Ball <cjb at laptop.org>
---
 arch/x86/kernel/olpc-pm.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/olpc-pm.c b/arch/x86/kernel/olpc-pm.c
index e99a464..9f0a565 100644
--- a/arch/x86/kernel/olpc-pm.c
+++ b/arch/x86/kernel/olpc-pm.c
@@ -43,6 +43,8 @@
 /* These, and the battery EC commands, should be in an olpc.h. */
 #define EC_WRITE_SCI_MASK 0x1b
 #define EC_READ_SCI_MASK  0x1c
+#define EC_WLAN_ENTER_RESET 0x35
+#define EC_WLAN_LEAVE_RESET 0x25
 
 extern void do_olpc_suspend_lowlevel(void);
 
@@ -661,6 +663,19 @@ static ssize_t wackup_show(struct kobject *s, struct kobj_attribute *attr,
 	return sprintf(buf, "%s\n", wackup_source ? wackup_source : "none");
 }
 
+static ssize_t wlanreset_execute(struct kobject *s, struct kobj_attribute *attr,
+		const char *buf, size_t n)
+{
+	unsigned int val = simple_strtoul(buf, NULL, 0);
+	if (val == 1) {
+		olpc_ec_cmd(EC_WLAN_ENTER_RESET, NULL, 0, NULL, 0);
+	}
+	else if (val == 0) {
+		olpc_ec_cmd(EC_WLAN_LEAVE_RESET, NULL, 0, NULL, 0);
+	}
+	return n;
+}
+
 static struct kobj_attribute control_attr =
 	__ATTR(olpc-pm, 0644, control_show, control_store);
 
@@ -670,10 +685,14 @@ static struct kobj_attribute test_attr =
 static struct kobj_attribute wackup_attr =
 	__ATTR(wakeup-source, 0400, wackup_show, NULL);
 
+static struct kobj_attribute wlanreset_attr =
+	__ATTR(wlan-reset, 0644, NULL, wlanreset_execute);
+
 static struct attribute * olpc_attributes[] = {
 	&control_attr.attr,
 	&test_attr.attr,
 	&wackup_attr.attr,
+	&wlanreset_attr.attr,
 	NULL
 };

]}}

  Changed 2 months ago by dsaxena

Code has been checked into master and testing branches of kernel.

  Changed 2 months ago by dsaxena

  • status changed from assigned to closed
  • resolution set to fixed

Fixed.

Note: See TracTickets for help on using tickets.