[PATCH] ARM: PXA: Zipit Z2: Fix oops in z2_power_off

[PATCH] ARM: PXA: Zipit Z2: Fix oops in z2_power_off

[Vasily Khoruzhick]

pxa27x_set_pwrmode is marked with __init, so it's not legitimate
to call it in pm_power_off hook.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 arch/arm/mach-pxa/z2.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-pxa/z2.c b/arch/arm/mach-pxa/z2.c
index 0fb5617..8b0ce77 100644
--- a/arch/arm/mach-pxa/z2.c
+++ b/arch/arm/mach-pxa/z2.c
@@ -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
-- 
1.7.8.1

[PATCH] ARM: PXA: Zipit Z2: Fix oops in z2_power_off

[Marek Vasut]

> pxa27x_set_pwrmode is marked with __init, so it's not legitimate
> to call it in pm_power_off hook.
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  arch/arm/mach-pxa/z2.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/z2.c b/arch/arm/mach-pxa/z2.c
> index 0fb5617..8b0ce77 100644
> --- a/arch/arm/mach-pxa/z2.c
> +++ b/arch/arm/mach-pxa/z2.c
> @@ -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

That's why I said this patch (suspend the device to make it feel like powerdown) 
is bullshit in the first place.

M

[PATCH] ARM: PXA: Zipit Z2: Fix oops in z2_power_off

[Vasily Khoruzhick]

2012/1/8 Marek Vasut <marek.vasut@gmail.com>:

> 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] ARM: PXA: Zipit Z2: Fix oops in z2_power_off

[Marek Vasut]

> 2012/1/8 Marek Vasut <marek.vasut@gmail.com>:
> > 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?

> 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?
> 
> Vasily.

[PATCH] ARM: PXA: Zipit Z2: Fix oops in z2_power_off

[Russell King - ARM Linux]

On Sun, Jan 08, 2012 at 11:40:28AM +0100, Marek Vasut wrote:
> > 2012/1/8 Marek Vasut <marek.vasut@gmail.com>:
> > > 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?
> 
> > 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] ARM: PXA: Zipit Z2: Fix oops in z2_power_off

[Vasily Khoruzhick]

2012/1/8 Marek Vasut <marek.vasut@gmail.com>:
>> 2012/1/8 Marek Vasut <marek.vasut@gmail.com>:
>> > 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?

Nope, it does not work. Looks like device disables power on memory in deepsleep.

>> 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] ARM: PXA: Zipit Z2: Fix oops in z2_power_off

[Marek Vasut]

> 2012/1/8 Marek Vasut <marek.vasut@gmail.com>:
> >> 2012/1/8 Marek Vasut <marek.vasut@gmail.com>:
> >> > 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?
> 
> Nope, it does not work. Looks like device disables power on memory in
> deepsleep.

Why?
> 
> >> 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?

> 
> Regards
> Vasily

[PATCH] ARM: PXA: Zipit Z2: Fix oops in z2_power_off

[Vasily Khoruzhick]

2012/1/9 Marek Vasut <marek.vasut@gmail.com>:

>> 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] ARM: PXA: Zipit Z2: Fix oops in z2_power_off

[Marek Vasut]

> 2012/1/9 Marek Vasut <marek.vasut@gmail.com>:
> >> 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.

Then maybe implement suspend-to-disk ? There was a patch for omap on the list, 
maybe it's even merged.

M

[PATCH] ARM: PXA: Zipit Z2: Fix oops in z2_power_off

[Russell King - ARM Linux]

On Mon, Jan 09, 2012 at 01:13:31PM +0100, Marek Vasut wrote:
> > 2012/1/9 Marek Vasut <marek.vasut@gmail.com>:
> > >> 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.
> 
> 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] ARM: PXA: Zipit Z2: Fix oops in z2_power_off

[Marek Vasut]

> On Mon, Jan 09, 2012 at 01:13:31PM +0100, Marek Vasut wrote:
> > > 2012/1/9 Marek Vasut <marek.vasut@gmail.com>:
> > > >> 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.
> > 
> > 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.

> 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.

> 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.

> 
> 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] ARM: PXA: Zipit Z2: Fix oops in z2_power_off

[Russell King - ARM Linux]

On Mon, Jan 09, 2012 at 01:38:29PM +0100, Marek Vasut wrote:
> > On Mon, Jan 09, 2012 at 01:13:31PM +0100, Marek Vasut wrote:
> > > > 2012/1/9 Marek Vasut <marek.vasut@gmail.com>:
> > > > >> 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.
> > > 
> > > 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.
> 
> > 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?

> > 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] ARM: PXA: Zipit Z2: Fix oops in z2_power_off

[Marek Vasut]

> On Mon, Jan 09, 2012 at 01:38:29PM +0100, Marek Vasut wrote:
> > > On Mon, Jan 09, 2012 at 01:13:31PM +0100, Marek Vasut wrote:
> > > > > 2012/1/9 Marek Vasut <marek.vasut@gmail.com>:
> > > > > >> 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.
> > > > 
> > > > 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.
> > 
> > > 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

> 
> > > 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] ARM: PXA: Zipit Z2: Fix oops in z2_power_off

[Russell King - ARM Linux]

On Mon, Jan 09, 2012 at 05:25:04PM +0100, Marek Vasut wrote:
> > On Mon, Jan 09, 2012 at 01:38:29PM +0100, Marek Vasut wrote:
> > > > On Mon, Jan 09, 2012 at 01:13:31PM +0100, Marek Vasut wrote:
> > > > > > 2012/1/9 Marek Vasut <marek.vasut@gmail.com>:
> > > > > > >> 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.
> > > > > 
> > > > > 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.
> > > 
> > > > 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

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.

> > > > 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.

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.