ARM: PXA: Zipit Z2: Fix oops in z2_power_off

Bootloader can clobber PWER value, so save it state on suspend.

Quoted 1 line

Bootloader can clobber PWER value, so save it state on suspend.

What boot loader is this, and why is it being so idiotic? ?Why can't it be fixed?

2012/2/26 Russell King - ARM Linux <[email protected]>:

Quoted 5 lines · Vasily Khoruzhick

Quoted 1 line

Bootloader can clobber PWER value, so save it state on suspend.

What boot loader is this, and why is it being so idiotic? ?Why can't it be fixed?

I'm using u-boot on this machine...
Hmm, I've looked throuhg its sources and did not find where it can clobber it. But PWER register value is not preserved across suspend for some reason (at least bit 0). Maybe it's some conflict with gpiolib? But gpio_set_wake() doesn't work
for me on PXA.

Quoted 1 line

Bootloader can clobber PWER value, so save it state on suspend.

What boot loader is this,

and why is it being so idiotic?

Why can't it
be fixed?

Quoted 4 lines · Vasily Khoruzhick

Quoted 1 line

Bootloader can clobber PWER value, so save it state on suspend.

What boot loader is this,

U-Boot, patched, not mainline.

Quoted 1 line

and why is it being so idiotic?

Mainline doesn't touch PWER, so please be careful about your accusations.

Quoted 2 lines

Why can't it
be fixed?

Yes, either by using mainline uboot (to rule out the bootloader problem), finding bug in linux kernel or reading the CPU manual.

Quoted 11 lines

Quoted 4 lines · Vasily Khoruzhick

Quoted 1 line

Bootloader can clobber PWER value, so save it state on suspend.

What boot loader is this,

U-Boot, patched, not mainline.

Quoted 1 line

and why is it being so idiotic?

Mainline doesn't touch PWER, so please be careful about your accusations.

Quoted 5 lines

Quoted 2 lines

Why can't it
be fixed?

Yes, either by using mainline uboot (to rule out the bootloader problem), finding bug in linux kernel or reading the CPU manual.

Was there supposed to be any useful information in your reply, or are you just trying to be your usual antagonistic self?

Quoted 18 lines · Marek Vasut

Quoted 16 lines

Quoted 4 lines · Vasily Khoruzhick

Quoted 1 line

Bootloader can clobber PWER value, so save it state on suspend.

What boot loader is this,

U-Boot, patched, not mainline.

Quoted 1 line

and why is it being so idiotic?

Mainline doesn't touch PWER, so please be careful about your accusations.

Quoted 2 lines

Why can't it
be fixed?

Yes, either by using mainline uboot (to rule out the bootloader problem), finding bug in linux kernel or reading the CPU manual.

Quoted 2 lines

Was there supposed to be any useful information in your reply, or are you just trying to be your usual antagonistic self?

Um ...

1) PWER isn't reconfigured by uboot (see above)

2) That you should try mainline uboot to avoid debugging problems with not- mainlined patches

3) That the bug with PWER is likely in kernel, since there's an issue with gpio_set_wake() on PXA

4) That the CPU might not preserve PWER throughout the suspend and that this information might be found in the CPU manual

Quoted 22 lines

Quoted 21 lines · Marek Vasut

Quoted 9 lines

Quoted 4 lines · Vasily Khoruzhick

Quoted 1 line

Bootloader can clobber PWER value, so save it state on suspend.

What boot loader is this,

U-Boot, patched, not mainline.

Quoted 1 line

and why is it being so idiotic?

Quoted 8 lines

Mainline doesn't touch PWER, so please be careful about your accusations.

Quoted 2 lines

Why can't it
be fixed?

Yes, either by using mainline uboot (to rule out the bootloader problem), finding bug in linux kernel or reading the CPU manual.

Was there supposed to be any useful information in your reply, or are you just trying to be your usual antagonistic self?

Quoted 3 lines

Um ...

1) PWER isn't reconfigured by uboot (see above)

Given that virtually everyone seems to use a modified version of uboot, how can you be so sure?

Quoted 3 lines

2) That you should try mainline uboot to avoid debugging problems with not- mainlined patches

Why should _I_ try anything with uboot? I'm not the one experiencing problems.

I'll point out that Vasily is reporting that _his_ boot loader, whatever it is, is clobbering the PWER register. That's *his* statement in the commit log, and I have no reason *not* to think that he's put in the research to confirm that. Otherwise, why make such a positive statement about where the problem lies?

Quoted 3 lines

3) That the bug with PWER is likely in kernel, since there's an issue with gpio_set_wake() on PXA

I've reviewed this function and its associated data, and can't see anything wrong with it (see below).

Quoted 2 lines

4) That the CPU might not preserve PWER throughout the suspend and that this information might be found in the CPU manual

No. The PXA270 manual says that this register is preserved on exit from deep sleep and sleep modes. It appends a note against each bit in the reset status for the PWER register which says "Exit from sleep or deep sleep mode does not clear or set this bit".

The setup for GPIO0 is:

        for (i = 0; i <= 15; i++) {
                /* skip GPIO2, 5, 6, 7, 8 */
                if (GPIO_bit(i) & 0x1e4)
                        continue;

                gpio_desc[i].can_wakeup = 1;
                gpio_desc[i].mask = GPIO_bit(i);
        }

So, gpio_desc[0].can_wakeup = 1 and gpio_desc[0].mask is the bitmask for GPIO0.

gpio_set_wake() does this:

        d = &gpio_desc[gpio];
        c = d->config;

        if (!d->valid)
                return -EINVAL;

well, that's already been set valid by pxa27x_mfp_init().

        if (d->keypad_gpio && (MFP_AF(d->config) == 0) &&

it's not a keypad gpio...

        mux_taken = (PWER & d->mux_mask) & (~d->mask);
        if (on && mux_taken)
                return -EBUSY;

well, d->mux_mask is zero, so this check fails.

        if (d->can_wakeup && (c & MFP_LPM_CAN_WAKEUP)) {
                if (on) {
                        PWER = (PWER & ~d->mux_mask) | d->mask;

Now, we know d->can_wakeup is true. However, what about the MFP configuration?

Well, that depends on the platform. I suspect that's where the problem with wakeup sources not working is located, because the MFP configuration hasn't been properly set, and that comes from platform code.

So please, stop blaming PXA code without full and proper analysis of bugs.

It's a waste of everyone's time, and it hurts proper kernel maintenance if these ill-researched 'workarounds' get merged instead of proper fixes.

We don't want machine to wake up on AC state change

Signed-off-by Vasily Khoruzhick <[email protected]>

---
 arch/arm/mach-pxa/z2.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

arch/arm/mach-pxa/z2.c +8 -1

index 0fb5617..bb672ee 100644
@@ -746,6 +746,13 @@ static void z2_power_off(void)
 #define z2_power_off   NULL
 #endif
 
+static void __init z2_map_io(void)
+{
+	pxa27x_map_io();
+
+	PWER &= ~(1 << 0);
+}
+
 /******************************************************************************
  * Machine init
  ******************************************************************************/

@@ -776,7 +783,7 @@ static void __init z2_init(void)
 
 MACHINE_START(ZIPIT2, "Zipit Z2")
 	.atag_offset	= 0x100,
-	.map_io		= pxa27x_map_io,
+	.map_io		= z2_map_io,
 	.init_irq	= pxa27x_init_irq,
 	.handle_irq	= pxa27x_handle_irq,
 	.timer		= &pxa_timer,

Signature · 7 lines

1.7.9.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 1 line

We don't want machine to wake up on AC state change

NAK. ?Stop polluting the machine callback methods. ?Use the right one.

2012/2/26 Russell King - ARM Linux <[email protected]>:

Quoted 4 lines · Vasily Khoruzhick

Quoted 1 line

We don't want machine to wake up on AC state change

NAK. ?Stop polluting the machine callback methods. ?Use the right one.

Ok, what's right one?

Quoted 7 lines

2012/2/26 Russell King - ARM Linux <[email protected]>:

Quoted 4 lines · Vasily Khoruzhick

Quoted 1 line

We don't want machine to wake up on AC state change

NAK. ?Stop polluting the machine callback methods. ?Use the right one.

Ok, what's right one?

? ? ? ?void ? ? ? ? ? ? ? ? ? ?(*fixup)(struct tag *, char **, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct meminfo *);
? ? ? ?void ? ? ? ? ? ? ? ? ? ?(*reserve)(void);/* reserve mem blocks ?*/ ? ? ? ?void ? ? ? ? ? ? ? ? ? ?(*map_io)(void);/* IO mapping function ?*/ ? ? ? ?void ? ? ? ? ? ? ? ? ? ?(*init_early)(void);
? ? ? ?void ? ? ? ? ? ? ? ? ? ?(*init_irq)(void);
? ? ? ?struct sys_timer ? ? ? ?*timer; ? ? ? ? /* system tick timer ? ?*/ ? ? ? ?void ? ? ? ? ? ? ? ? ? ?(*init_machine)(void);

and chose a more appropriate one from that? ?Maybe init_early() if it has to be done early, or preferably the init_machine() if it can wait until arch_initcall() time.

But stuffing it into fixup, reserve, map_io, init_irq or the timer initialization is pure abuse of those callbacks.

2012/2/26 Russell King - ARM Linux <[email protected]>:

Quoted 18 lines · Vasily Khoruzhick

Quoted 7 lines

2012/2/26 Russell King - ARM Linux <[email protected]>:

Quoted 4 lines · Vasily Khoruzhick

Quoted 1 line

We don't want machine to wake up on AC state change

NAK. Stop polluting the machine callback methods. Use the right one.

Ok, what's right one?

void (*fixup)(struct tag *, char **,
struct meminfo *);
void (*reserve)(void);/* reserve mem blocks */
void (*map_io)(void);/* IO mapping function */
void (*init_early)(void);
void (*init_irq)(void);
struct sys_timer *timer; /* system tick timer */
void (*init_machine)(void);

Quoted 6 lines

and chose a more appropriate one from that? Maybe init_early() if it has to be done early, or preferably the init_machine() if it can wait until arch_initcall() time.

But stuffing it into fixup, reserve, map_io, init_irq or the timer initialization is pure abuse of those callbacks.

Ok, thanks.

Quoted 23 lines

2012/2/26 Russell King - ARM Linux <[email protected]>:

Quoted 22 lines · Vasily Khoruzhick

Quoted 7 lines

2012/2/26 Russell King - ARM Linux <[email protected]>:

Quoted 4 lines · Vasily Khoruzhick

Quoted 1 line

We don't want machine to wake up on AC state change

NAK. Stop polluting the machine callback methods. Use the right one.

Ok, what's right one?

void (*fixup)(struct tag *, char **,
struct meminfo *);
void (*reserve)(void);/* reserve mem blocks */
void (*map_io)(void);/* IO mapping function */
void (*init_early)(void);
void (*init_irq)(void);
struct sys_timer *timer; /* system tick timer */
void (*init_machine)(void);

and chose a more appropriate one from that? Maybe init_early() if it has to be done early, or preferably the init_machine() if it can wait until arch_initcall() time.

Quoted 5 lines

Quoted 2 lines

But stuffing it into fixup, reserve, map_io, init_irq or the timer initialization is pure abuse of those callbacks.

Ok, thanks.

Or this should be patched in the bootloader too. Also, please dont

Quoted 29 lines

Quoted 29 lines

2012/2/26 Russell King - ARM Linux <[email protected]>:

Quoted 27 lines · Vasily Khoruzhick

Quoted 9 lines

2012/2/26 Russell King - ARM Linux <[email protected]>:

Quoted 5 lines · Vasily Khoruzhick

Quoted 1 line

We don't want machine to wake up on AC state change

NAK. Stop polluting the machine callback methods. Use the right one.

Ok, what's right one?

       void                    (*fixup)(struct tag *, char **,
       
                                        struct meminfo *);
       
       void                    (*reserve)(void);/* reserve mem blocks
        */ void                    (*map_io)(void);/* IO mapping
       function  */ void                    (*init_early)(void);
       void                    (*init_irq)(void);
       struct sys_timer        *timer;         /* system tick timer  
        */ void                    (*init_machine)(void);

and chose a more appropriate one from that? Maybe init_early() if it has to be done early, or preferably the init_machine() if it can wait until arch_initcall() time.

But stuffing it into fixup, reserve, map_io, init_irq or the timer initialization is pure abuse of those callbacks.

Quoted 3 lines

Quoted 1 line

Ok, thanks.

Or this should be patched in the bootloader too. Also, please dont

* introduce ad-hoc constants like "1 << 0", ever!

Fiddling with PWER should not be in the boot loader. The kernel handles PWER just fine.