ARM: PXA: Zipit Z2: Fix oops in z2_power_off
Subsystems (3)
PATCH 1/1 ARM: PXA: Zipit Z2: Fix oops in z2_power_off
pxa27x_set_pwrmode is marked with __init, so it's not legitimate to call it in pm_power_off hook.
--- arch/arm/mach-pxa/z2.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
arch/arm/mach-pxa/z2.c +2 -2
@@ -33,6 +33,7 @@#include <asm/mach-types.h> #include <asm/mach/arch.h> +#include <asm/suspend.h> #include <mach/pxa27x.h> #include <mach/mfp-pxa27x.h>
@@ -739,8 +740,7 @@ static void z2_power_off(void)*/ PSPR =0x0 ;local_irq_disable (); -pxa27x_set_pwrmode (PWRMODE_DEEPSLEEP ); - pxa27x_cpu_pm_enter(PM_SUSPEND_MEM); + cpu_suspend(PWRMODE_DEEPSLEEP, pxa27x_finish_suspend); }#else #define z2_power_off NULL
Signature · 1 line
1.7.8.1
PATCH 1/1 ARM: PXA: Zipit Z2: Fix oops in z2_power_off
Quoted 21 lines
pxa27x_set_pwrmodeis marked with__init, so it's not legitimate to call it inpm_power_offhook.Signed-off-by Vasily Khoruzhick <[email protected]>--- arch/arm/mach-pxa/z2.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)arch/arm/mach-pxa/z2.c +1
-0@@ -33,6 +33,7 @@#include <asm/mach-types.h> #include <asm/mach/arch.h> +#include <asm/suspend.h> #include <mach/pxa27x.h> #include <mach/mfp-pxa27x.h>
arch/arm/mach-pxa/z2.c static void z2_power_off(void) · line 746
Quoted 9 lines
arch/arm/mach-pxa/z2.c +1
-2@@ -739,8 +740,7 @@ static void z2_power_off(void)*/ PSPR =0x0 ;local_irq_disable (); -pxa27x_set_pwrmode (PWRMODE_DEEPSLEEP ); - pxa27x_cpu_pm_enter(PM_SUSPEND_MEM); + cpu_suspend(PWRMODE_DEEPSLEEP, pxa27x_finish_suspend); }#else #define z2_power_off NULL
That's why I said this patch (suspend the device to make it feel like powerdown) is bullshit in the first place.
M
PATCH 1/1 ARM: PXA: Zipit Z2: Fix oops in z2_power_off
2012/1/8 Marek Vasut <[email protected]>:
Quoted 2 lines
That's why I said this patch (suspend the device to make it feel like powerdown) is bullshit in the first place.
Marek, 'deepsleep' suspend on Z2 is a way to save battery power for monthes. I blew the dust off my Z2 recently (did not touch it for ~2monthes), it was in fake poweroff (deepsleep), and battery is still charged (not full however). I believe it can't be achieved with 'sleep' suspend.
I agree that it looks weird, but it's hardware bizzarity. Btw, do you have suggestions how to make it clean?
Vasily.
PATCH 1/1 ARM: PXA: Zipit Z2: Fix oops in z2_power_off
Quoted 9 lines
2012/1/8 Marek Vasut <[email protected]>:
Quoted 2 lines
That's why I said this patch (suspend the device to make it feel like powerdown) is bullshit in the first place.
Marek, 'deepsleep' suspend on Z2 is a way to save battery power for monthes. I blew the dust off my Z2 recently (did not touch it for ~2monthes), it was in fake poweroff (deepsleep), and battery is still charged (not full however). I believe it can't be achieved with 'sleep' suspend.
But you can put the device into this deepsleep-suspend without faking shutdown, right?
Quoted 2 lines
I agree that it looks weird, but it's hardware bizzarity. Btw, do you have suggestions how to make it clean?
Yea ... why won't deepsleep suspend work via standard suspend interface?
Quoted 2 lines
Vasily.
PATCH 1/1 ARM: PXA: Zipit Z2: Fix oops in z2_power_off
Quoted 10 lines · Marek Vasut
Quoted 9 lines
2012/1/8 Marek Vasut <[email protected]>:
Quoted 2 lines
That's why I said this patch (suspend the device to make it feel like powerdown) is bullshit in the first place.
Marek, 'deepsleep' suspend on Z2 is a way to save battery power for monthes. I blew the dust off my Z2 recently (did not touch it for ~2monthes), it was in fake poweroff (deepsleep), and battery is still charged (not full however). I believe it can't be achieved with 'sleep' suspend.
Quoted 7 lines
But you can put the device into this deepsleep-suspend without faking shutdown, right?
Quoted 2 lines
I agree that it looks weird, but it's hardware bizzarity. Btw, do you have suggestions how to make it clean?
Yea ... why won't deepsleep suspend work via standard suspend interface?
Have you looked to see what pxa27x_cpu_pm_enter() does before calling the CPU suspend function?
It appears to do some SoC setup first, to ensure that things are correctly set before entering low power mode. I'd be willing to bet that some of that is still necessary (eg, ensuring that the voltage sequencer is not running and there are no edge detect events pending) so that the SoC really does hit the desired sleep mode.
I think the simplest, cheapest and easiest solution would be to remove the __init on the function.
As for using sleep modes to fake shutdown - I see nothing diabolically wrong with that. There's been devices doing that kind of stuff for years and years. iPAQs for example have no way to 'power off'.
However, I'm not saying it's a good idea: iPAQs ended up in that situation after lots of Compaq iPAQ customers complained about the little switch beneath the cover being 'too easy' to turn off!
The result of that clever complaint is that you have to keep iPAQs connected to their charger and plugged into the mains while you're not expecting to use them, otherwise you end up killing the battery. Even if you place them in so called 'off' mode, the battery will be drained in about a month, and if left like that the battery life suffers.
(I have two iPAQs with dead batteries because of this, which as a result are utterly useless. I have one other which is permanently connected to its charger for about 350 days a year to preserve its battery - just so it can be used for reasonable periods without external power and losing the programs and data loaded onto it.)
PATCH 1/1 ARM: PXA: Zipit Z2: Fix oops in z2_power_off
In reply to Marek Vasut2012/1/8 Marek Vasut <[email protected]>:
Quoted 4 lines
Quoted 4 lines
2012/1/8 Marek Vasut <[email protected]>:
Quoted 2 lines
That's why I said this patch (suspend the device to make it feel like powerdown) is bullshit in the first place.
Quoted 8 lines
Quoted 5 lines
Marek, 'deepsleep' suspend on Z2 is a way to save battery power for monthes. I blew the dust off my Z2 recently (did not touch it for ~2monthes), it was in fake poweroff (deepsleep), and battery is still charged (not full however). I believe it can't be achieved with 'sleep' suspend.
But you can put the device into this deepsleep-suspend without faking shutdown, right?
Nope, it does not work. Looks like device disables power on memory in deepsleep.
Quoted 4 lines
Quoted 2 lines
I agree that it looks weird, but it's hardware bizzarity. Btw, do you have suggestions how to make it clean?
Yea ... why won't deepsleep suspend work via standard suspend interface?
Just because it's not possible :)
Regards
Vasily
PATCH 1/1 ARM: PXA: Zipit Z2: Fix oops in z2_power_off
Quoted 11 lines
2012/1/8 Marek Vasut <[email protected]>:
Quoted 10 lines
Quoted 9 lines
2012/1/8 Marek Vasut <[email protected]>:
Quoted 2 lines
That's why I said this patch (suspend the device to make it feel like powerdown) is bullshit in the first place.
Marek, 'deepsleep' suspend on Z2 is a way to save battery power for monthes. I blew the dust off my Z2 recently (did not touch it for ~2monthes), it was in fake poweroff (deepsleep), and battery is still charged (not full however). I believe it can't be achieved with 'sleep' suspend.
Quoted 5 lines
Quoted 2 lines
But you can put the device into this deepsleep-suspend without faking shutdown, right?
Nope, it does not work. Looks like device disables power on memory in deepsleep.
Why?
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
I agree that it looks weird, but it's hardware bizzarity. Btw, do you have suggestions how to make it clean?
Yea ... why won't deepsleep suspend work via standard suspend interface?
Just because it's not possible :)
Why?
Quoted 3 lines
Regards
Vasily
PATCH 1/1 ARM: PXA: Zipit Z2: Fix oops in z2_power_off
2012/1/9 Marek Vasut <[email protected]>:
Quoted 4 lines
Quoted 2 lines
Nope, it does not work. Looks like device disables power on memory in deepsleep.
Why?
In deepsleep PXA27x deasserts SYS_EN pin, and looks like when it's deasserted there's no power on SDRAM (on Zipit). Just guess.
PATCH 1/1 ARM: PXA: Zipit Z2: Fix oops in z2_power_off
Quoted 8 lines
2012/1/9 Marek Vasut <[email protected]>:
Quoted 4 lines
Quoted 2 lines
Nope, it does not work. Looks like device disables power on memory in deepsleep.
Why?
In deepsleep PXA27x deasserts
SYS_ENpin, and looks like when it's deasserted there's no power on SDRAM (on Zipit). Just guess.
Then maybe implement suspend-to-disk ? There was a patch for omap on the list, maybe it's even merged.
M
PATCH 1/1 ARM: PXA: Zipit Z2: Fix oops in z2_power_off
Quoted 11 lines · Marek Vasut
Quoted 8 lines
2012/1/9 Marek Vasut <[email protected]>:
Quoted 4 lines
Quoted 2 lines
Nope, it does not work. Looks like device disables power on memory in deepsleep.
Why?
In deepsleep PXA27x deasserts
SYS_ENpin, and looks like when it's deasserted there's no power on SDRAM (on Zipit). Just guess.Then maybe implement suspend-to-disk ? There was a patch for omap on the list, maybe it's even merged.
What are hell you going on about? Why would someone want to even try implementing suspend-to-disk in the power off hook? Why would you even want to try to preserve the system state in a power off hook?
Nothing much you've said in this thread makes sense so far. What is the point you're trying to raise, other than a personal dislike for using the deep sleep mode to simulate an apparant 'power off' condition?
PATCH 1/1 ARM: PXA: Zipit Z2: Fix oops in z2_power_off
Quoted 7 lines · Marek Vasut
Quoted 6 lines
Quoted 6 lines
2012/1/9 Marek Vasut <[email protected]>:
Quoted 4 lines
Quoted 2 lines
Nope, it does not work. Looks like device disables power on memory in deepsleep.
Why?
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
In deepsleep PXA27x deasserts
SYS_ENpin, and looks like when it's deasserted there's no power on SDRAM (on Zipit). Just guess.Then maybe implement suspend-to-disk ? There was a patch for omap on the list, maybe it's even merged.
What are hell you going on about?
Cool your jets Russell.
Quoted 2 lines
Why would someone want to even try implementing suspend-to-disk in the power off hook?
I'm not talking about the powerdown hook anymore, but a way how to avoid it as much as possible via standard means.
Quoted 2 lines
Why would you even
want to try to preserve the system state in a power off hook?
Not in poweroff hook. Btw. I'm still convinced the bootloader should handle this -- the powerdown should just reset the thing and tell bootloader to deepsleep it.
Quoted 4 lines
Nothing much you've said in this thread makes sense so far. What is the point you're trying to raise, other than a personal dislike for using the deep sleep mode to simulate an apparant 'power off' condition?
See above.
PATCH 1/1 ARM: PXA: Zipit Z2: Fix oops in z2_power_off
Quoted 15 lines · Marek Vasut
Quoted 7 lines · Marek Vasut
Quoted 6 lines
Quoted 6 lines
2012/1/9 Marek Vasut <[email protected]>:
Quoted 4 lines
Quoted 2 lines
Nope, it does not work. Looks like device disables power on memory in deepsleep.
Why?
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
In deepsleep PXA27x deasserts
SYS_ENpin, and looks like when it's deasserted there's no power on SDRAM (on Zipit). Just guess.Then maybe implement suspend-to-disk ? There was a patch for omap on the list, maybe it's even merged.
What are hell you going on about?
Quoted 7 lines
Cool your jets Russell.
Quoted 2 lines
Why would someone want to even try implementing suspend-to-disk in the power off hook?
I'm not talking about the powerdown hook anymore, but a way how to avoid it as much as possible via standard means.
Maybe you should state that you are deviating from the subject of this email thread to aid your readers understanding?
Quoted 6 lines
Quoted 2 lines
Why would you even
want to try to preserve the system state in a power off hook?Not in poweroff hook. Btw. I'm still convinced the bootloader should handle this -- the powerdown should just reset the thing and tell bootloader to deepsleep it.
Why should the boot loader be involved? The boot loader is all about bringing the system _up_, not taking it down.
PATCH 1/1 ARM: PXA: Zipit Z2: Fix oops in z2_power_off
Quoted 18 lines · Marek Vasut
Quoted 17 lines
Quoted 7 lines · Marek Vasut
Quoted 6 lines
Quoted 6 lines
2012/1/9 Marek Vasut <[email protected]>:
Quoted 4 lines
Quoted 2 lines
Nope, it does not work. Looks like device disables power on memory in deepsleep.
Why?
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
In deepsleep PXA27x deasserts
SYS_ENpin, and looks like when it's deasserted there's no power on SDRAM (on Zipit). Just guess.Then maybe implement suspend-to-disk ? There was a patch for omap on the list, maybe it's even merged.
What are hell you going on about?
Cool your jets Russell.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Why would someone want to even try implementing suspend-to-disk in the power off hook?
I'm not talking about the powerdown hook anymore, but a way how to avoid it as much as possible via standard means.
Maybe you should state that you are deviating from the subject of this email thread to aid your readers understanding?
I think it was pretty clear already. OK
Quoted 10 lines
Quoted 7 lines
Quoted 3 lines
Why would you even
want to try to preserve the system state in a power off hook?Not in poweroff hook. Btw. I'm still convinced the bootloader should handle this -- the powerdown should just reset the thing and tell bootloader to deepsleep it.
Why should the boot loader be involved? The boot loader is all about bringing the system _up_, not taking it down.
Why would you bloat kernel with non-standard crap? The firmware should handle this if the hardware is broken (and even if this sounds acpi-ish, I still consider this particular thing should be handled by firmware). Especially since what the device does in the end is simply restart anyway.
M
PATCH 1/1 ARM: PXA: Zipit Z2: Fix oops in z2_power_off
Quoted 21 lines · Marek Vasut
Quoted 21 lines · Marek Vasut
Quoted 20 lines
Quoted 7 lines · Marek Vasut
Quoted 6 lines
Quoted 6 lines
2012/1/9 Marek Vasut <[email protected]>:
Quoted 4 lines
Quoted 2 lines
Nope, it does not work. Looks like device disables power on memory in deepsleep.
Why?
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
In deepsleep PXA27x deasserts
SYS_ENpin, and looks like when it's deasserted there's no power on SDRAM (on Zipit). Just guess.Then maybe implement suspend-to-disk ? There was a patch for omap on the list, maybe it's even merged.
What are hell you going on about?
Cool your jets Russell.
Quoted 2 lines
Why would someone want to even try implementing suspend-to-disk in the power off hook?
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
I'm not talking about the powerdown hook anymore, but a way how to avoid it as much as possible via standard means.
Maybe you should state that you are deviating from the subject of this email thread to aid your readers understanding?
I think it was pretty clear already. OK
Sigh, stop fucking arguing Marek. Plainly it wasn't, because YOUR READER (in other words me) did not notice your spontaneous context switch. It might have been clear to you - you know your line of thought - but you COMPLETELY FAILED to communicate it in effectively your email message.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 2 lines
Why would you even
want to try to preserve the system state in a power off hook?Not in poweroff hook. Btw. I'm still convinced the bootloader should handle this -- the powerdown should just reset the thing and tell bootloader to deepsleep it.
Quoted 7 lines
Quoted 2 lines
Why should the boot loader be involved? The boot loader is all about bringing the system _up_, not taking it down.
Why would you bloat kernel with non-standard crap? The firmware should handle this if the hardware is broken (and even if this sounds acpi-ish, I still consider this particular thing should be handled by firmware). Especially since what the device does in the end is simply restart anyway.
You talk about bloat, but I wonder if you've actually thought about what you're proposing - I don't think you have.
Let's see: if we want to force people to add stuff to their boot loaders to allow deep sleep power-off, then we need to have a way to tell the boot loader to do that.
We're going to have to _add_ code to the kernel to find some way to tell the boot loader that we want it to go to sleep. We're going to have to _add_ _more_ code to the kernel make a power off event cause a reboot. Code is also going to have to be added to the boot loader to allow it to understand that and implement the low power mode.
That's in addition to one simple function causing this problem, which is already in the kernel, but gets discarded after init time:
int __init pxa27x_set_pwrmode(unsigned int mode)
{
switch (mode) {
case PWRMODE_SLEEP:
case PWRMODE_DEEPSLEEP:
pwrmode = mode;
return 0;
}
return -EINVAL;
}and this function equates to _less_ code than the solution you're proposing - probably weighing in at around 8 instructions and probably one literal pool entry.
No way are you going to implement functionality to tell the boot loader what to do _and_ get the kernel to reboot in 8 instructions.
And you talk about bloat? You really need to stand back and look at the bigger picture, understand that what you're being completely unreasonable with your demands - and trying to justify your ideas with arguments which just don't stack up.
The z2 code stands. I'm sure Eric will take a patch to remove the __init attributation for it. I'll even take such a patch into my fixes branch for it. It's a kernel oops bug, and _fixing_ it is worthwhile _even_ if there isn't agreement with the method being used.