Re: Possible suspend/resume regression in .32-rc?

Re: Possible suspend/resume regression in .32-rc?

[Pavel Machek]

Hi!

> > Is anyone using suspend/resume with a recent git mainline kernel on PXA
> > or other ARM embedded boards? My platform used to suspend and resume
> > just fine on 2.6.31 but now as I rebased it, it fails the resume part.
> > 
> > Unfortunately, I can't bisect it as the platform is not mainline yet and
> > so I always have mandatory patches (without my platform won't do
> > anything) on top of the git repository. Which breaks the bisect logic.
> > 
> > What puzzles me is that I see the current raising at wakeup time, so at
> > least the processor seems to resume, but I can't see any serial console
> > output, just like if the kernel crashed very early after wakeup.
> > 'no_console_suspend' didn't help either.
> 
> Ok, got it. The culprit is commit d2c37068 ("[ARM] pxa: initialize
> default interrupt priority and use ICHP for IRQ handling"). Reverting it
> make suspend/resume work again on my board.
> 
> Haojian, Eric, could you have a look at this?

Okay, patch is this one: I'll test reverting it shortly.

commit d2c37068429b29d6549cf3486fc84b836689e122
Author: Haojian Zhuang <haojian.zhuang@marvell.com>
Date:   Wed Aug 19 19:49:31 2009 +0800

    [ARM] pxa: initialize default interrupt priority and use ICHP for IRQ handling
    
    Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
    Signed-off-by: Eric Miao <eric.y.miao@gmail.com>

diff --git a/arch/arm/mach-pxa/include/mach/entry-macro.S b/arch/arm/mach-pxa/include/mach/entry-macro.S
index f6b4bf3..2418806 100644
--- a/arch/arm/mach-pxa/include/mach/entry-macro.S
+++ b/arch/arm/mach-pxa/include/mach/entry-macro.S
@@ -24,34 +24,27 @@
 		mov	\tmp, \tmp, lsr #13
 		and	\tmp, \tmp, #0x7		@ Core G
 		cmp	\tmp, #1
-		bhi	1004f
+		bhi	1002f
 
+		@ Core Generation 1 (PXA25x)
 		mov	\base, #io_p2v(0x40000000)	@ IIR Ctl = 0x40d00000
 		add	\base, \base, #0x00d00000
 		ldr	\irqstat, [\base, #0]		@ ICIP
 		ldr	\irqnr, [\base, #4]		@ ICMR
-		b	1002f
 
-1004:
-		mrc	p6, 0, \irqstat, c6, c0, 0	@ ICIP2
-		mrc	p6, 0, \irqnr, c7, c0, 0	@ ICMR2
 		ands	\irqnr, \irqstat, \irqnr
-		beq	1003f
+		beq	1001f
 		rsb	\irqstat, \irqnr, #0
 		and	\irqstat, \irqstat, \irqnr
 		clz	\irqnr, \irqstat
-		rsb	\irqnr, \irqnr, #31
-		add	\irqnr, \irqnr, #(32 + PXA_IRQ(0))
+		rsb	\irqnr, \irqnr, #(31 + PXA_IRQ(0))
 		b	1001f
-1003:
-		mrc	p6, 0, \irqstat, c0, c0, 0	@ ICIP
-		mrc	p6, 0, \irqnr, c1, c0, 0	@ ICMR
 1002:
-		ands	\irqnr, \irqstat, \irqnr
+		@ Core Generation 2 (PXA27x) or Core Generation 3 (PXA3xx)
+		mrc	p6, 0, \irqstat, c5, c0, 0	@ ICHP
+		tst	\irqstat, #0x80000000
 		beq	1001f
-		rsb	\irqstat, \irqnr, #0
-		and	\irqstat, \irqstat, \irqnr
-		clz	\irqnr, \irqstat
-		rsb	\irqnr, \irqnr, #(31 + PXA_IRQ(0))
+		bic	\irqstat, \irqstat, #0x80000000
+		mov	\irqnr, \irqstat, lsr #16
 1001:
 		.endm
diff --git a/arch/arm/mach-pxa/irq.c b/arch/arm/mach-pxa/irq.c
index f6e0300..d694ce2 100644
--- a/arch/arm/mach-pxa/irq.c
+++ b/arch/arm/mach-pxa/irq.c
@@ -120,7 +120,7 @@ static void __init pxa_init_low_gpio_irq(set_wake_t fn)
 
 void __init pxa_init_irq(int irq_nr, set_wake_t fn)
 {
-	int irq;
+	int irq, i;
 
 	pxa_internal_irq_nr = irq_nr;
 
@@ -129,6 +129,12 @@ void __init pxa_init_irq(int irq_nr, set_wake_t fn)
 		_ICLR(irq) = 0;	/* all IRQs are IRQ, not FIQ */
 	}
 
+	/* initialize interrupt priority */
+	if (cpu_is_pxa27x() || cpu_is_pxa3xx()) {
+		for (i = 0; i < irq_nr; i++)
+			IPR(i) = i | (1 << 31);
+	}
+
 	/* only unmasked interrupts kick us out of idle */
 	ICCR = 1;
 


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Possible suspend/resume regression in .32-rc?

[Pavel Machek]

> Hi!
> 
> > > Is anyone using suspend/resume with a recent git mainline kernel on PXA
> > > or other ARM embedded boards? My platform used to suspend and resume
> > > just fine on 2.6.31 but now as I rebased it, it fails the resume part.
> > > 
> > > Unfortunately, I can't bisect it as the platform is not mainline yet and
> > > so I always have mandatory patches (without my platform won't do
> > > anything) on top of the git repository. Which breaks the bisect logic.
> > > 
> > > What puzzles me is that I see the current raising at wakeup time, so at
> > > least the processor seems to resume, but I can't see any serial console
> > > output, just like if the kernel crashed very early after wakeup.
> > > 'no_console_suspend' didn't help either.
> > 
> > Ok, got it. The culprit is commit d2c37068 ("[ARM] pxa: initialize
> > default interrupt priority and use ICHP for IRQ handling"). Reverting it
> > make suspend/resume work again on my board.
> > 
> > Haojian, Eric, could you have a look at this?
> 
> Okay, patch is this one: I'll test reverting it shortly.
> 
> commit d2c37068429b29d6549cf3486fc84b836689e122
> Author: Haojian Zhuang <haojian.zhuang@marvell.com>
> Date:   Wed Aug 19 19:49:31 2009 +0800
> 
>     [ARM] pxa: initialize default interrupt priority and use ICHP for IRQ handling
>     
>     Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
>     Signed-off-by: Eric Miao <eric.y.miao@gmail.com>

And yes, reverting it _does_ fix suspend on spitz.

									Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Possible suspend/resume regression in .32-rc?

[Haojian Zhuang]

On Sun, Nov 1, 2009 at 6:03 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> Hi!
>>
>> > > Is anyone using suspend/resume with a recent git mainline kernel on PXA
>> > > or other ARM embedded boards? My platform used to suspend and resume
>> > > just fine on 2.6.31 but now as I rebased it, it fails the resume part.
>> > >
>> > > Unfortunately, I can't bisect it as the platform is not mainline yet and
>> > > so I always have mandatory patches (without my platform won't do
>> > > anything) on top of the git repository. Which breaks the bisect logic.
>> > >
>> > > What puzzles me is that I see the current raising at wakeup time, so at
>> > > least the processor seems to resume, but I can't see any serial console
>> > > output, just like if the kernel crashed very early after wakeup.
>> > > 'no_console_suspend' didn't help either.
>> >
>> > Ok, got it. The culprit is commit d2c37068 ("[ARM] pxa: initialize
>> > default interrupt priority and use ICHP for IRQ handling"). Reverting it
>> > make suspend/resume work again on my board.
>> >
>> > Haojian, Eric, could you have a look at this?
>>
>> Okay, patch is this one: I'll test reverting it shortly.
>>
>> commit d2c37068429b29d6549cf3486fc84b836689e122
>> Author: Haojian Zhuang <haojian.zhuang@marvell.com>
>> Date:   Wed Aug 19 19:49:31 2009 +0800
>>
>>     [ARM] pxa: initialize default interrupt priority and use ICHP for IRQ handling
>>
>>     Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
>>     Signed-off-by: Eric Miao <eric.y.miao@gmail.com>
>
> And yes, reverting it _does_ fix suspend on spitz.
>
>                                                                        Pavel

Em, it's not caused by the IRQ patch.

The kernel is blocked in resume path. When console is resumed, IRQ is
already disabled and system is blocked. Actually, IRQ shouldn't be
disabled at here. Up to now, I only find which patch will cause this
issue. But I can't find the best solution on it. The patch with issue
is pasted in below.

So this issue is only occused when console suspend is enabled. If you
enable no_console_suspend in command, you won't meet this issue. It
seems that it's caused by removing termios setting in
uart_resume_port() in the below patch. If I add these code back, the
issue doesn't occur any more.

Deepak,
Could you help to investigate this issue also?

By the way, the suggested quick test command is in below. If we tried
these commands for 20 times, the issue would occur. I only test it in
PXA silicon. I don't know the symptom on other silicons.
$ echo devices > /sys/power/pm_test
$ echo mem > /sys/power/state

Thanks
Haojian

>From ba15ab0e8de0d4439a91342ad52d55ca9e313f3d Mon Sep 17 00:00:00 2001
From: Deepak Saxena <dsaxena@laptop.org>
Date: Sat, 19 Sep 2009 13:13:33 -0700
Subject: [PATCH] Set proper console speed on resume if console suspend
is disabled

Commit b5b82df6, from May 2007, breaks no_console_suspend on the OLPC
XO laptop. Basically what happens is that upon returning from resume,
serial8250_resume_port() will reconfigure the port for high speed
mode and all console output will be garbled, making debug of the
resume path painful. This patch modifies uart_resume_port() to
reset the port to the state it was in before we suspended.

Original patch by Marcelo Tosatti

Second patch by Deepak then reworked by Alan to fit with the tty changes
before it got submitted. Also fixed the console path to set c_i/ospeed as
some drivers require the termios fields are valid

Signed-off-by: Deepak Saxena <dsaxena@laptop.org>
Signed-off-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/serial/serial_core.c |   32 ++++++++++++++++++--------------
 1 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index 3fd0134..2514d00 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -2068,11 +2068,29 @@ int uart_resume_port(struct uart_driver *drv,
struct uart_port *uport)
 	struct tty_port *port = &state->port;
 	struct device *tty_dev;
 	struct uart_match match = {uport, drv};
+	struct ktermios termios;

 	mutex_lock(&port->mutex);

 	if (!console_suspend_enabled && uart_console(uport)) {
 		/* no need to resume serial console, it wasn't suspended */
+		/*
+		 * First try to use the console cflag setting.
+		 */
+		memset(&termios, 0, sizeof(struct ktermios));
+		termios.c_cflag = uport->cons->cflag;
+		/*
+		 * If that's unset, use the tty termios setting.
+		 */
+		if (termios.c_cflag == 0)
+			termios = *state->port.tty->termios;
+		else {
+			termios.c_ispeed = termios.c_ospeed =
+				tty_termios_input_baud_rate(&termios);
+			termios.c_ispeed = termios.c_ospeed =
+				tty_termios_baud_rate(&termios);
+		}
+		uport->ops->set_termios(uport, &termios, NULL);
 		mutex_unlock(&port->mutex);
 		return 0;
 	}
@@ -2089,20 +2107,6 @@ int uart_resume_port(struct uart_driver *drv,
struct uart_port *uport)
 	 * Re-enable the console device after suspending.
 	 */
 	if (uart_console(uport)) {
-		struct ktermios termios;
-
-		/*
-		 * First try to use the console cflag setting.
-		 */
-		memset(&termios, 0, sizeof(struct ktermios));
-		termios.c_cflag = uport->cons->cflag;
-
-		/*
-		 * If that's unset, use the tty termios setting.
-		 */
-		if (port->tty && termios.c_cflag == 0)
-			termios = *port->tty->termios;
-
 		uart_change_pm(state, 0);
 		uport->ops->set_termios(uport, &termios, NULL);
 		console_start(uport->cons);
-- 
1.5.6.5

Re: Possible suspend/resume regression in .32-rc?

[Daniel Mack]

On Mon, Nov 02, 2009 at 05:22:30AM -0400, Haojian Zhuang wrote:
> >> > Ok, got it. The culprit is commit d2c37068 ("[ARM] pxa: initialize
> >> > default interrupt priority and use ICHP for IRQ handling"). Reverting it
> >> > make suspend/resume work again on my board.
> >> >
> >> > Haojian, Eric, could you have a look at this?
> >>
> >> Okay, patch is this one: I'll test reverting it shortly.
> >>
> >> commit d2c37068429b29d6549cf3486fc84b836689e122
> >> Author: Haojian Zhuang <haojian.zhuang@marvell.com>
> >> Date:   Wed Aug 19 19:49:31 2009 +0800
> >>
> >>     [ARM] pxa: initialize default interrupt priority and use ICHP for IRQ handling
> >>
> >>     Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
> >>     Signed-off-by: Eric Miao <eric.y.miao@gmail.com>
> >
> > And yes, reverting it _does_ fix suspend on spitz.
> >
> >                                                                        Pavel
> 
> Em, it's not caused by the IRQ patch.
> 
> The kernel is blocked in resume path. When console is resumed, IRQ is
> already disabled and system is blocked. Actually, IRQ shouldn't be
> disabled at here. Up to now, I only find which patch will cause this
> issue. But I can't find the best solution on it. The patch with issue
> is pasted in below.
> 
> So this issue is only occused when console suspend is enabled. If you
> enable no_console_suspend in command, you won't meet this issue. It
> seems that it's caused by removing termios setting in
> uart_resume_port() in the below patch. If I add these code back, the
> issue doesn't occur any more.

Well no, not confirmed. I tested with no_console_suspend, and it still
hit me. And without too, btw.

Daniel

Re: Possible suspend/resume regression in .32-rc?

[Pavel Machek]

On Mon 2009-11-02 05:22:30, Haojian Zhuang wrote:
> On Sun, Nov 1, 2009 at 6:03 PM, Pavel Machek <pavel@ucw.cz> wrote:
> >> Hi!
> >>
> >> > > Is anyone using suspend/resume with a recent git mainline kernel on PXA
> >> > > or other ARM embedded boards? My platform used to suspend and resume
> >> > > just fine on 2.6.31 but now as I rebased it, it fails the resume part.
> >> > >
> >> > > Unfortunately, I can't bisect it as the platform is not mainline yet and
> >> > > so I always have mandatory patches (without my platform won't do
> >> > > anything) on top of the git repository. Which breaks the bisect logic.
> >> > >
> >> > > What puzzles me is that I see the current raising at wakeup time, so at
> >> > > least the processor seems to resume, but I can't see any serial console
> >> > > output, just like if the kernel crashed very early after wakeup.
> >> > > 'no_console_suspend' didn't help either.
> >> >
> >> > Ok, got it. The culprit is commit d2c37068 ("[ARM] pxa: initialize
> >> > default interrupt priority and use ICHP for IRQ handling"). Reverting it
> >> > make suspend/resume work again on my board.
> >> >
> >> > Haojian, Eric, could you have a look at this?
> >>
> >> Okay, patch is this one: I'll test reverting it shortly.
> >>
> >> commit d2c37068429b29d6549cf3486fc84b836689e122
> >> Author: Haojian Zhuang <haojian.zhuang@marvell.com>
> >> Date:   Wed Aug 19 19:49:31 2009 +0800
> >>
> >>     [ARM] pxa: initialize default interrupt priority and use ICHP for IRQ handling
> >>
> >>     Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
> >>     Signed-off-by: Eric Miao <eric.y.miao@gmail.com>
> >
> > And yes, reverting it _does_ fix suspend on spitz.
> 
> Em, it's not caused by the IRQ patch.
> 
> The kernel is blocked in resume path. When console is resumed, IRQ is
> already disabled and system is blocked. Actually, IRQ shouldn't be
> disabled at here. Up to now, I only find which patch will cause this
> issue. But I can't find the best solution on it. The patch with issue
> is pasted in below.
> 
> So this issue is only occused when console suspend is enabled. If you
> enable no_console_suspend in command, you won't meet this issue. It
> seems that it's caused by removing termios setting in
> uart_resume_port() in the below patch. If I add these code back, the
> issue doesn't occur any more.

Given that it hangs very early, in arch_suspend_enable_irqs() (see my
other mail), I don't trust your analysis.

> Deepak,
> Could you help to investigate this issue also?
> 
> By the way, the suggested quick test command is in below. If we tried
> these commands for 20 times, the issue would occur. I only test it in
> PXA silicon. I don't know the symptom on other silicons.
> $ echo devices > /sys/power/pm_test
> $ echo mem > /sys/power/state

I'm not using serial console on spitz, and I have never had successful
resume with the patch applied.

The original patch is also poorly changelogged. What is the advantage
of ICHP and why do we want default priorities?

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Possible suspend/resume regression in .32-rc?

[Eric Miao]

> The original patch is also poorly changelogged. What is the advantage
> of ICHP and why do we want default priorities?
>

That's going to simplify the "get_irqnr_and_base" macro significantly
and make it much easier to adopt pxa950 which has more than 64
IRQs. And besides, it's priority based, so we do want to handle IRQ
numbers from high priority to low from software either.

Re: Possible suspend/resume regression in .32-rc?

[Haojian Zhuang]

On Mon, Nov 2, 2009 at 5:38 AM, Pavel Machek <pavel@ucw.cz> wrote:
> On Mon 2009-11-02 05:22:30, Haojian Zhuang wrote:
>> On Sun, Nov 1, 2009 at 6:03 PM, Pavel Machek <pavel@ucw.cz> wrote:
>>
>> Em, it's not caused by the IRQ patch.
>>
>> The kernel is blocked in resume path. When console is resumed, IRQ is
>> already disabled and system is blocked. Actually, IRQ shouldn't be
>> disabled at here. Up to now, I only find which patch will cause this
>> issue. But I can't find the best solution on it. The patch with issue
>> is pasted in below.
>>
>> So this issue is only occused when console suspend is enabled. If you
>> enable no_console_suspend in command, you won't meet this issue. It
>> seems that it's caused by removing termios setting in
>> uart_resume_port() in the below patch. If I add these code back, the
>> issue doesn't occur any more.
>
> Given that it hangs very early, in arch_suspend_enable_irqs() (see my
> other mail), I don't trust your analysis.
>
> I'm not using serial console on spitz, and I have never had successful
> resume with the patch applied.

It seems that we're talking on different issue with similar symptom.
Please check my test method. While I'm testing suspend with devices
level, kernel is blocked in console resume. In this level, it won't
call arch_suspend_enable_irqs(). This function call is only invoked in
processor level or below.

Up to now, I can't reproduce the issue you're talking on my platform
yet. I'll check this issue continuously. I also want to know your
hardware information.

Thanks
Haojian

Re: Possible suspend/resume regression in .32-rc?

[Pavel Machek]

On Mon 2009-11-02 06:48:28, Haojian Zhuang wrote:
> On Mon, Nov 2, 2009 at 5:38 AM, Pavel Machek <pavel@ucw.cz> wrote:
> > On Mon 2009-11-02 05:22:30, Haojian Zhuang wrote:
> >> On Sun, Nov 1, 2009 at 6:03 PM, Pavel Machek <pavel@ucw.cz> wrote:
> >>
> >> Em, it's not caused by the IRQ patch.
> >>
> >> The kernel is blocked in resume path. When console is resumed, IRQ is
> >> already disabled and system is blocked. Actually, IRQ shouldn't be
> >> disabled at here. Up to now, I only find which patch will cause this
> >> issue. But I can't find the best solution on it. The patch with issue
> >> is pasted in below.
> >>
> >> So this issue is only occused when console suspend is enabled. If you
> >> enable no_console_suspend in command, you won't meet this issue. It
> >> seems that it's caused by removing termios setting in
> >> uart_resume_port() in the below patch. If I add these code back, the
> >> issue doesn't occur any more.
> >
> > Given that it hangs very early, in arch_suspend_enable_irqs() (see my
> > other mail), I don't trust your analysis.
> >
> > I'm not using serial console on spitz, and I have never had successful
> > resume with the patch applied.
> 
> It seems that we're talking on different issue with similar symptom.
> Please check my test method. While I'm testing suspend with devices
> level, kernel is blocked in console resume. In this level, it won't
> call arch_suspend_enable_irqs(). This function call is only invoked in
> processor level or below.

For me, everything but real suspend works. I do _not_ have serial
console for spitz.

> Up to now, I can't reproduce the issue you're talking on my platform
> yet. I'll check this issue continuously. I also want to know your
> hardware information.

Spitz, aka Sharp Zaurus c3000.

Can we get the patch reverted till its fixed, so other development can
continue?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Possible suspend/resume regression in .32-rc?

[Haojian Zhuang]

On Mon, Nov 2, 2009 at 6:51 AM, Pavel Machek <pavel@ucw.cz> wrote:
> On Mon 2009-11-02 06:48:28, Haojian Zhuang wrote:
>> On Mon, Nov 2, 2009 at 5:38 AM, Pavel Machek <pavel@ucw.cz> wrote:
>> > On Mon 2009-11-02 05:22:30, Haojian Zhuang wrote:
>> >> On Sun, Nov 1, 2009 at 6:03 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> >>
>> >> Em, it's not caused by the IRQ patch.
>> >>
>> >> The kernel is blocked in resume path. When console is resumed, IRQ is
>> >> already disabled and system is blocked. Actually, IRQ shouldn't be
>> >> disabled at here. Up to now, I only find which patch will cause this
>> >> issue. But I can't find the best solution on it. The patch with issue
>> >> is pasted in below.
>> >>
>> >> So this issue is only occused when console suspend is enabled. If you
>> >> enable no_console_suspend in command, you won't meet this issue. It
>> >> seems that it's caused by removing termios setting in
>> >> uart_resume_port() in the below patch. If I add these code back, the
>> >> issue doesn't occur any more.
>> >
>> > Given that it hangs very early, in arch_suspend_enable_irqs() (see my
>> > other mail), I don't trust your analysis.
>> >
>> > I'm not using serial console on spitz, and I have never had successful
>> > resume with the patch applied.
>>
>> It seems that we're talking on different issue with similar symptom.
>> Please check my test method. While I'm testing suspend with devices
>> level, kernel is blocked in console resume. In this level, it won't
>> call arch_suspend_enable_irqs(). This function call is only invoked in
>> processor level or below.
>
> For me, everything but real suspend works. I do _not_ have serial
> console for spitz.
>
>> Up to now, I can't reproduce the issue you're talking on my platform
>> yet. I'll check this issue continuously. I also want to know your
>> hardware information.
>
> Spitz, aka Sharp Zaurus c3000.
>

I see. You can chech my patch in below. I need to save IPRs in suspend
resume routine.

Thanks
Haojian

>From 1764e836424d42a0654b8b73c402a2dddb118dc4 Mon Sep 17 00:00:00 2001
From: Haojian Zhuang <haojian.zhuang@marvell.com>
Date: Mon, 2 Nov 2009 14:02:21 -0500
Subject: [PATCH] pxa: fix system resume issue on pxa27x and pxa3xx

Since interrupt handler is changed to use interrupt priority, we also need to
save and restore these interrupt controller registers in suspend/resume
routine.

Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
---
 arch/arm/mach-pxa/irq.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-pxa/irq.c b/arch/arm/mach-pxa/irq.c
index d694ce2..cdc272b 100644
--- a/arch/arm/mach-pxa/irq.c
+++ b/arch/arm/mach-pxa/irq.c
@@ -150,6 +150,7 @@ void __init pxa_init_irq(int irq_nr, set_wake_t fn)

 #ifdef CONFIG_PM
 static unsigned long saved_icmr[2];
+static unsigned long saved_ipr[128];

 static int pxa_irq_suspend(struct sys_device *dev, pm_message_t state)
 {
@@ -159,6 +160,10 @@ static int pxa_irq_suspend(struct sys_device
*dev, pm_message_t state)
 		saved_icmr[i] = _ICMR(irq);
 		_ICMR(irq) = 0;
 	}
+	if (pxa_internal_irq_nr > 128)
+		BUG();
+	for (i = 0; i < pxa_internal_irq_nr; i++)
+		saved_ipr[i] = IPR(i);

 	return 0;
 }
@@ -171,6 +176,8 @@ static int pxa_irq_resume(struct sys_device *dev)
 		_ICMR(irq) = saved_icmr[i];
 		_ICLR(irq) = 0;
 	}
+	for (i = 0; i < pxa_internal_irq_nr; i++)
+		IPR(i) = saved_ipr[i];

 	ICCR = 1;
 	return 0;
-- 
1.5.6.5

Re: Possible suspend/resume regression in .32-rc?

[Daniel Mack]

On Mon, Nov 02, 2009 at 07:18:02AM -0400, Haojian Zhuang wrote:
> On Mon, Nov 2, 2009 at 6:51 AM, Pavel Machek <pavel@ucw.cz> wrote:
> >>
> >> It seems that we're talking on different issue with similar symptom.
> >> Please check my test method. While I'm testing suspend with devices
> >> level, kernel is blocked in console resume. In this level, it won't
> >> call arch_suspend_enable_irqs(). This function call is only invoked in
> >> processor level or below.
> >
> > For me, everything but real suspend works. I do _not_ have serial
> > console for spitz.
> >
> >> Up to now, I can't reproduce the issue you're talking on my platform
> >> yet. I'll check this issue continuously. I also want to know your
> >> hardware information.
> >
> > Spitz, aka Sharp Zaurus c3000.
> >
> 
> I see. You can chech my patch in below. I need to save IPRs in suspend
> resume routine.

Yep - that fixes it on my platform. Great. Add my 'Tested-by' if you
like.

Thanks,
Daniel

> From 1764e836424d42a0654b8b73c402a2dddb118dc4 Mon Sep 17 00:00:00 2001
> From: Haojian Zhuang <haojian.zhuang@marvell.com>
> Date: Mon, 2 Nov 2009 14:02:21 -0500
> Subject: [PATCH] pxa: fix system resume issue on pxa27x and pxa3xx
> 
> Since interrupt handler is changed to use interrupt priority, we also need to
> save and restore these interrupt controller registers in suspend/resume
> routine.
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>
> ---
>  arch/arm/mach-pxa/irq.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-pxa/irq.c b/arch/arm/mach-pxa/irq.c
> index d694ce2..cdc272b 100644
> --- a/arch/arm/mach-pxa/irq.c
> +++ b/arch/arm/mach-pxa/irq.c
> @@ -150,6 +150,7 @@ void __init pxa_init_irq(int irq_nr, set_wake_t fn)
> 
>  #ifdef CONFIG_PM
>  static unsigned long saved_icmr[2];
> +static unsigned long saved_ipr[128];
> 
>  static int pxa_irq_suspend(struct sys_device *dev, pm_message_t state)
>  {
> @@ -159,6 +160,10 @@ static int pxa_irq_suspend(struct sys_device
> *dev, pm_message_t state)
>  		saved_icmr[i] = _ICMR(irq);
>  		_ICMR(irq) = 0;
>  	}
> +	if (pxa_internal_irq_nr > 128)
> +		BUG();
> +	for (i = 0; i < pxa_internal_irq_nr; i++)
> +		saved_ipr[i] = IPR(i);
> 
>  	return 0;
>  }
> @@ -171,6 +176,8 @@ static int pxa_irq_resume(struct sys_device *dev)
>  		_ICMR(irq) = saved_icmr[i];
>  		_ICLR(irq) = 0;
>  	}
> +	for (i = 0; i < pxa_internal_irq_nr; i++)
> +		IPR(i) = saved_ipr[i];
> 
>  	ICCR = 1;
>  	return 0;
> -- 
> 1.5.6.5

Re: Possible suspend/resume regression in .32-rc?

[Pavel Machek]

> >> > Given that it hangs very early, in arch_suspend_enable_irqs() (see my
> >> > other mail), I don't trust your analysis.
> >> >
> >> > I'm not using serial console on spitz, and I have never had successful
> >> > resume with the patch applied.
...
> I see. You can chech my patch in below. I need to save IPRs in suspend
> resume routine.
...
> >From 1764e836424d42a0654b8b73c402a2dddb118dc4 Mon Sep 17 00:00:00 2001
> From: Haojian Zhuang <haojian.zhuang@marvell.com>
> Date: Mon, 2 Nov 2009 14:02:21 -0500
> Subject: [PATCH] pxa: fix system resume issue on pxa27x and pxa3xx
> 
> Since interrupt handler is changed to use interrupt priority, we also need to
> save and restore these interrupt controller registers in suspend/resume
> routine.
> 
> Signed-off-by: Haojian Zhuang <haojian.zhuang@marvell.com>

Acked-by: Pavel Machek <pavel@ucw.cz>
Tested-by: Pavel Machek <pavel@ucw.cz>

> @@ -150,6 +150,7 @@ void __init pxa_init_irq(int irq_nr, set_wake_t fn)
> 
>  #ifdef CONFIG_PM
>  static unsigned long saved_icmr[2];
> +static unsigned long saved_ipr[128];
> 
>  static int pxa_irq_suspend(struct sys_device *dev, pm_message_t state)
>  {
> @@ -159,6 +160,10 @@ static int pxa_irq_suspend(struct sys_device
> *dev, pm_message_t state)

Note: Your mail client is word-wrapping.

>  		saved_icmr[i] = _ICMR(irq);
>  		_ICMR(irq) = 0;
>  	}
> +	if (pxa_internal_irq_nr > 128)
> +		BUG();

BUG_ON()? WARN_ON() then irq_nr = 128? User is very unlikely to read
the BUG() message at this point...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Possible suspend/resume regression in .32-rc?

[Stanislav Brabec]

Haojian Zhuang wrote:

> Commit b5b82df6, from May 2007, breaks no_console_suspend on the OLPC
> XO laptop. Basically what happens is that upon returning from resume,
> serial8250_resume_port() will reconfigure the port for high speed
> mode and all console output will be garbled, making debug of the
> resume path painful. This patch modifies uart_resume_port() to
> reset the port to the state it was in before we suspended.

See my patch waiting for approval in LKML thread
"serial-core: resume serial hardware with no_console_suspend".

It attempts to fix it, but I was not yet able to test it due to spitz
resume breakage.


________________________________________________________________________
Stanislav Brabec
http://www.penguin.cz/~utx

Re: Possible suspend/resume regression in .32-rc?

[Haojian Zhuang]

On Mon, Nov 2, 2009 at 5:57 AM, Stanislav Brabec <utx@penguin.cz> wrote:
> Haojian Zhuang wrote:
>
>> Commit b5b82df6, from May 2007, breaks no_console_suspend on the OLPC
>> XO laptop. Basically what happens is that upon returning from resume,
>> serial8250_resume_port() will reconfigure the port for high speed
>> mode and all console output will be garbled, making debug of the
>> resume path painful. This patch modifies uart_resume_port() to
>> reset the port to the state it was in before we suspended.
>
> See my patch waiting for approval in LKML thread
> "serial-core: resume serial hardware with no_console_suspend".
>
> It attempts to fix it, but I was not yet able to test it due to spitz
> resume breakage.
>

Hi Stanislav,

At first, your patch can't be applied into my latest codebase. I have
to update the patch. But the console resume is still blocked after
applied your patch. It works only after I merge some old code back. My
modified patch is in below.

diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index 1689bda..73f41cc 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -2005,12 +2005,6 @@ int uart_suspend_port(struct uart_driver *drv,
struct uart_port *uport)

 	mutex_lock(&port->mutex);

-	if (!console_suspend_enabled && uart_console(uport)) {
-		/* we're going to avoid suspending serial console */
-		mutex_unlock(&port->mutex);
-		return 0;
-	}
-
 	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
 	if (device_may_wakeup(tty_dev)) {
 		enable_irq_wake(uport->irq);
@@ -2018,20 +2012,22 @@ int uart_suspend_port(struct uart_driver *drv,
struct uart_port *uport)
 		mutex_unlock(&port->mutex);
 		return 0;
 	}
-	uport->suspended = 1;
+	if (console_suspend_enabled || !uart_console(uport))
+		uport->suspended = 1;

 	if (port->flags & ASYNC_INITIALIZED) {
 		const struct uart_ops *ops = uport->ops;
 		int tries;

-		set_bit(ASYNCB_SUSPENDED, &port->flags);
-		clear_bit(ASYNCB_INITIALIZED, &port->flags);
-
-		spin_lock_irq(&uport->lock);
-		ops->stop_tx(uport);
-		ops->set_mctrl(uport, 0);
-		ops->stop_rx(uport);
-		spin_unlock_irq(&uport->lock);
+		if (console_suspend_enabled || !uart_console(uport)) {
+			set_bit(ASYNCB_SUSPENDED, &port->flags);
+			clear_bit(ASYNCB_INITIALIZED, &port->flags);
+			spin_lock_irq(&uport->lock);
+			ops->stop_tx(uport);
+			ops->set_mctrl(uport, 0);
+			ops->stop_rx(uport);
+			spin_unlock_irq(&uport->lock);
+		}

 		/*
 		 * Wait for the transmitter to empty.
@@ -2046,16 +2042,18 @@ int uart_suspend_port(struct uart_driver *drv,
struct uart_port *uport)
 			       drv->dev_name,
 			       drv->tty_driver->name_base + uport->line);

-		ops->shutdown(uport);
+		if (console_suspend_enabled || !uart_console(uport))
+			ops->shutdown(uport);
 	}

 	/*
 	 * Disable the console device before suspending.
 	 */
-	if (uart_console(uport))
+	if (console_suspend_enabled && uart_console(uport))
 		console_stop(uport->cons);

-	uart_change_pm(state, 3);
+	if (console_suspend_enabled || !uart_console(uport))
+		uart_change_pm(state, 3);

 	mutex_unlock(&port->mutex);

@@ -2072,8 +2070,18 @@ int uart_resume_port(struct uart_driver *drv,
struct uart_port *uport)

 	mutex_lock(&port->mutex);

-	if (!console_suspend_enabled && uart_console(uport)) {
-		/* no need to resume serial console, it wasn't suspended */
+	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
+	if (!uport->suspended && device_may_wakeup(tty_dev)) {
+		disable_irq_wake(uport->irq);
+		mutex_unlock(&port->mutex);
+		return 0;
+	}
+	uport->suspended = 0;
+
+	/*
+	 * Re-enable the console device after suspending.
+	 */
+	if (uart_console(uport)) {
 		/*
 		 * First try to use the console cflag setting.
 		 */
@@ -2090,23 +2098,6 @@ int uart_resume_port(struct uart_driver *drv,
struct uart_port *uport)
 			termios.c_ispeed = termios.c_ospeed =
 				tty_termios_baud_rate(&termios);
 		}
-		uport->ops->set_termios(uport, &termios, NULL);
-		mutex_unlock(&port->mutex);
-		return 0;
-	}
-
-	tty_dev = device_find_child(uport->dev, &match, serial_match_port);
-	if (!uport->suspended && device_may_wakeup(tty_dev)) {
-		disable_irq_wake(uport->irq);
-		mutex_unlock(&port->mutex);
-		return 0;
-	}
-	uport->suspended = 0;
-
-	/*
-	 * Re-enable the console device after suspending.
-	 */
-	if (uart_console(uport)) {
 		uart_change_pm(state, 0);
 		uport->ops->set_termios(uport, &termios, NULL);
 		console_start(uport->cons);
@@ -2120,21 +2111,23 @@ int uart_resume_port(struct uart_driver *drv,
struct uart_port *uport)
 		spin_lock_irq(&uport->lock);
 		ops->set_mctrl(uport, 0);
 		spin_unlock_irq(&uport->lock);
-		ret = ops->startup(uport);
-		if (ret == 0) {
-			uart_change_speed(state, NULL);
-			spin_lock_irq(&uport->lock);
-			ops->set_mctrl(uport, uport->mctrl);
-			ops->start_tx(uport);
-			spin_unlock_irq(&uport->lock);
-			set_bit(ASYNCB_INITIALIZED, &port->flags);
-		} else {
-			/*
-			 * Failed to resume - maybe hardware went away?
-			 * Clear the "initialized" flag so we won't try
-			 * to call the low level drivers shutdown method.
-			 */
-			uart_shutdown(state);
+		if (console_suspend_enabled || !uart_console(uport)) {
+			ret = ops->startup(uport);
+			if (ret == 0) {
+				uart_change_speed(state, NULL);
+				spin_lock_irq(&uport->lock);
+				ops->set_mctrl(uport, uport->mctrl);
+				ops->start_tx(uport);
+				spin_unlock_irq(&uport->lock);
+				set_bit(ASYNCB_INITIALIZED, &port->flags);
+			} else {
+				/*
+				 * Failed to resume - maybe hardware went away?
+				 * Clear the "initialized" flag so we won't try
+				 * to call the low level drivers shutdown method.
+				 */
+				uart_shutdown(state);
+			}
 		}

 		clear_bit(ASYNCB_SUSPENDED, &port->flags);

Re: Possible suspend/resume regression in .32-rc?

[Stanislav Brabec]

Haojian Zhuang wrote:
> On Mon, Nov 2, 2009 at 5:57 AM, Stanislav Brabec <utx@penguin.cz> wrote:
> > Haojian Zhuang wrote:
> >
> >> Commit b5b82df6, from May 2007, breaks no_console_suspend on the OLPC
> >> XO laptop. Basically what happens is that upon returning from resume,
> >> serial8250_resume_port() will reconfigure the port for high speed
> >> mode and all console output will be garbled, making debug of the
> >> resume path painful. This patch modifies uart_resume_port() to
> >> reset the port to the state it was in before we suspended.
> >
> > See my patch waiting for approval in LKML thread
> > "serial-core: resume serial hardware with no_console_suspend".
> >
> > It attempts to fix it, but I was not yet able to test it due to spitz
> > resume breakage.
> >
> 
> Hi Stanislav,
> 
> At first, your patch can't be applied into my latest codebase. I have
> to update the patch. But the console resume is still blocked after
> applied your patch. It works only after I merge some old code back. My
> modified patch is in below.

Yes, code changed there a bit. I sent rebased patch later in the thread.
Did you try this one? (Well I did not test it yet - in time of sending
resume did not work at all on my Zaurus.)

Your patch is a bit different. I'll test both as soon as possible.


________________________________________________________________________
Stanislav Brabec
http://www.penguin.cz/~utx

Re: Possible suspend/resume regression in .32-rc?

[Haojian Zhuang]

On Tue, Nov 3, 2009 at 4:50 AM, Stanislav Brabec <utx@penguin.cz> wrote:
> Haojian Zhuang wrote:
>> On Mon, Nov 2, 2009 at 5:57 AM, Stanislav Brabec <utx@penguin.cz> wrote:
>> > Haojian Zhuang wrote:
>> >
>> >> Commit b5b82df6, from May 2007, breaks no_console_suspend on the OLPC
>> >> XO laptop. Basically what happens is that upon returning from resume,
>> >> serial8250_resume_port() will reconfigure the port for high speed
>> >> mode and all console output will be garbled, making debug of the
>> >> resume path painful. This patch modifies uart_resume_port() to
>> >> reset the port to the state it was in before we suspended.
>> >
>> > See my patch waiting for approval in LKML thread
>> > "serial-core: resume serial hardware with no_console_suspend".
>> >
>> > It attempts to fix it, but I was not yet able to test it due to spitz
>> > resume breakage.
>> >
>>
>> Hi Stanislav,
>>
>> At first, your patch can't be applied into my latest codebase. I have
>> to update the patch. But the console resume is still blocked after
>> applied your patch. It works only after I merge some old code back. My
>> modified patch is in below.
>
> Yes, code changed there a bit. I sent rebased patch later in the thread.
> Did you try this one? (Well I did not test it yet - in time of sending
> resume did not work at all on my Zaurus.)
>
> Your patch is a bit different. I'll test both as soon as possible.
>
>

Your latest patch works well in my platform. Thanks a lot.

Re: Possible suspend/resume regression in .32-rc?

[Daniel Mack]

On Thu, Nov 05, 2009 at 12:06:13AM -0500, Haojian Zhuang wrote:
> On Tue, Nov 3, 2009 at 4:50 AM, Stanislav Brabec <utx@penguin.cz> wrote:
> > Haojian Zhuang wrote:
> >> On Mon, Nov 2, 2009 at 5:57 AM, Stanislav Brabec <utx@penguin.cz> wrote:
> >> > Haojian Zhuang wrote:
> >> >
> >> >> Commit b5b82df6, from May 2007, breaks no_console_suspend on the OLPC
> >> >> XO laptop. Basically what happens is that upon returning from resume,
> >> >> serial8250_resume_port() will reconfigure the port for high speed
> >> >> mode and all console output will be garbled, making debug of the
> >> >> resume path painful. This patch modifies uart_resume_port() to
> >> >> reset the port to the state it was in before we suspended.
> >> >
> >> > See my patch waiting for approval in LKML thread
> >> > "serial-core: resume serial hardware with no_console_suspend".
> >> >
> >> > It attempts to fix it, but I was not yet able to test it due to spitz
> >> > resume breakage.
> >> >
> >>
> >> Hi Stanislav,
> >>
> >> At first, your patch can't be applied into my latest codebase. I have
> >> to update the patch. But the console resume is still blocked after
> >> applied your patch. It works only after I merge some old code back. My
> >> modified patch is in below.
> >
> > Yes, code changed there a bit. I sent rebased patch later in the thread.
> > Did you try this one? (Well I did not test it yet - in time of sending
> > resume did not work at all on my Zaurus.)
> >
> > Your patch is a bit different. I'll test both as soon as possible.
> >
> >
> 
> Your latest patch works well in my platform. Thanks a lot.

It seems Stanislav's patch is still unmerged in the current git HEAD.
Any idea why it is held back? I have it applied locally since quite a
while here, and it works well. So if it helps, take my Tested-by: for
it.

Daniel

Re: Possible suspend/resume regression in .32-rc?

[Stanislav Brabec]

Daniel Mack wrote:

> It seems Stanislav's patch is still unmerged in the current git HEAD.
> Any idea why it is held back? I have it applied locally since quite a
> while here, and it works well. So if it helps, take my Tested-by: for
> it.

It seems to be on the right way:

From: gregkh@suse.de
Subject: patch serial-core-resume-serial-hardware-with-no_console_suspend.patch added to gregkh-2.6 tree
Date: Tue, 22 Dec 2009 12:22:35 -0800

This is a note to let you know that I've just added the patch titled

    Subject: serial-core: resume serial hardware with no_console_suspend

to my gregkh-2.6 tree.  Its filename is

    serial-core-resume-serial-hardware-with-no_console_suspend.patch

This tree can be found at 

http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/


-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                          e-mail: sbrabec@suse.cz
Lihovarská 1060/12           tel: +420 284 028 966, +49 911 740538747
190 00 Praha 9                                  fax: +420 284 028 951
Czech Republic                                    http://www.suse.cz/

Re: Possible suspend/resume regression in .32-rc?

[Greg KH]

On Wed, Jan 13, 2010 at 02:42:41PM +0100, Stanislav Brabec wrote:
> Daniel Mack wrote:
> 
> > It seems Stanislav's patch is still unmerged in the current git HEAD.
> > Any idea why it is held back? I have it applied locally since quite a
> > while here, and it works well. So if it helps, take my Tested-by: for
> > it.
> 
> It seems to be on the right way:
> 
> From: gregkh@suse.de
> Subject: patch serial-core-resume-serial-hardware-with-no_console_suspend.patch added to gregkh-2.6 tree
> Date: Tue, 22 Dec 2009 12:22:35 -0800
> 
> This is a note to let you know that I've just added the patch titled
> 
>     Subject: serial-core: resume serial hardware with no_console_suspend
> 
> to my gregkh-2.6 tree.  Its filename is
> 
>     serial-core-resume-serial-hardware-with-no_console_suspend.patch
> 
> This tree can be found at 
> 
> http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/patches/

Ah, I missed that this was fixing a regression, so I'll move it to my
"apply before .32 is out" queue.  Sorry about that.

greg k-h