Re: [PATCH] AT91 Serial: update the powersave handler to match serial core

Re: [PATCH] AT91 Serial: update the powersave handler to match serial core

[Haavard Skinnemoen]

Anti Sullin <anti.sullin@artecdesign.ee> wrote:
> This problem seems to be unnoticed so far:
> 
> Patch http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=b3b708fa2780cd2b5d8266a8f0c3a1cab364d4d2
> has changed the serial core behavior to not to suspend the port if the device is enabled as a wakeup source. If the
> AT91 system goes to slow clock mode, the port should be suspended always and the clocks should be switched off. 
> The patch attached updates the atmel_serial driver to match the changes in serial core.

Right...I guess this might also help get rid of the warning about
unbalanced irqwake that I've been meaning to look at for a while now...

> Also, the interrupts are disabled when the clock is disabled. If we disable the clock with interrupts enabled, an interrupt
> may get stuck. If this is the DBGU interrupt, this blocks the OR logic at system controller and thus all other sysc interrupts.

Not good. I suspect we need to stop DMA as well though...or wait until
it's drained.

> The patch is against 2.6.25.3 + at91 patchset at maxim.org.za. 

It doesn't apply to latest mainline, but I'll try to cram it in somehow.

A few questions though...

> diff -purN linux-2.6.25.3/drivers/serial/atmel_serial.c linux-2.6.25.3_ok/drivers/serial/atmel_serial.c
> --- linux-2.6.25.3/drivers/serial/atmel_serial.c	2008-05-10 07:48:50.000000000 +0300
> +++ linux-2.6.25.3_ok/drivers/serial/atmel_serial.c	2008-09-08 12:35:35.000000000 +0300
> @@ -132,7 +132,8 @@ struct atmel_uart_char {
>  struct atmel_uart_port {
>  	struct uart_port	uart;		/* uart */
>  	struct clk		*clk;		/* uart clock */
> -	unsigned short		suspended;	/* is port suspended? */
> +	int			may_wakeup;		/* cached value of device_may_wakeup for times we need to disable it */
> +	int			backup_imr;		/* back up the IMR during suspend */

Since this is a hardware register, shouldn't it be u32?

> @@ -1264,6 +1273,8 @@ void __init atmel_register_uart_fns(stru
>  #ifdef CONFIG_SERIAL_ATMEL_CONSOLE
>  static void atmel_console_putchar(struct uart_port *port, int ch)
>  {
> +	if (port->suspended) return;
> +
>  	while (!(UART_GET_CSR(port) & ATMEL_US_TXRDY))
>  		cpu_relax();
>  	UART_PUT_CHAR(port, ch);
> @@ -1278,6 +1289,8 @@ static void atmel_console_write(struct c
>  	unsigned int status, imr;
>  	unsigned int pdc_tx;
>  
> +	if (port->suspended) return;
> +

Are both of these checks necessary? It doesn't look like
atmel_console_putchar() can be called from anywhere else than
atmel_console_write().

Also, does the no_console_suspend parameter still work after this?

Haavard

Re: [PATCH] AT91 Serial: update the powersave handler to match serial core

[Haavard Skinnemoen]

Anti Sullin <anti.sullin@artecdesign.ee> wrote:
> Haavard Skinnemoen wrote:
> > Not good. I suspect we need to stop DMA as well though...or wait until
> > it's drained.
> Right... I found independently the DMA problem with video controller DMA and suspend a week ago (but as I left some devices
> to do automated testing for the weekend, your patch had fixed it when I looked the kernel on monday morning... how ironic) and
> we wouldn't want another such bug.
> But the serial driver had already switched off the clocks for ages and so long I haven't found any crashes...

Maybe the tty layer ensures that the buffer is empty before
suspending...would be interesting to know. In any case, any such bug
would probably be insanely difficult to trigger, so I think we should
stop the PDC just in case.

> > 
> >> The patch is against 2.6.25.3 + at91 patchset at maxim.org.za. 
> > 
> > It doesn't apply to latest mainline, but I'll try to cram it in somehow.
> Sorry, my production devices are currently using 25.3...
> Use this e-mail just as bug report.

Ok, I think I managed to apply it correctly. I'm going to test it now;
you'll see the result when I send it upstream (this is 2.6.27 material,
I think.)

> After I fixed this and the LCDC DMA, i did 400k suspend cycles on parallel with multiple devices for testing and
> I did not get any crashes.

Nice.

> > A few questions though...
> > 
> >> diff -purN linux-2.6.25.3/drivers/serial/atmel_serial.c linux-2.6.25.3_ok/drivers/serial/atmel_serial.c
> >> --- linux-2.6.25.3/drivers/serial/atmel_serial.c	2008-05-10 07:48:50.000000000 +0300
> >> +++ linux-2.6.25.3_ok/drivers/serial/atmel_serial.c	2008-09-08 12:35:35.000000000 +0300
> >> @@ -132,7 +132,8 @@ struct atmel_uart_char {
> >>  struct atmel_uart_port {
> >>  	struct uart_port	uart;		/* uart */
> >>  	struct clk		*clk;		/* uart clock */
> >> -	unsigned short		suspended;	/* is port suspended? */
> >> +	int			may_wakeup;		/* cached value of device_may_wakeup for times we need to disable it */
> >> +	int			backup_imr;		/* back up the IMR during suspend */
> > 
> > Since this is a hardware register, shouldn't it be u32?
> Yep...

I'll change it.

> > 
> >> @@ -1264,6 +1273,8 @@ void __init atmel_register_uart_fns(stru
> >>  #ifdef CONFIG_SERIAL_ATMEL_CONSOLE
> >>  static void atmel_console_putchar(struct uart_port *port, int ch)
> >>  {
> >> +	if (port->suspended) return;
> >> +
> >>  	while (!(UART_GET_CSR(port) & ATMEL_US_TXRDY))
> >>  		cpu_relax();
> >>  	UART_PUT_CHAR(port, ch);
> >> @@ -1278,6 +1289,8 @@ static void atmel_console_write(struct c
> >>  	unsigned int status, imr;
> >>  	unsigned int pdc_tx;
> >>  
> >> +	if (port->suspended) return;
> >> +
> > 
> > Are both of these checks necessary? It doesn't look like
> > atmel_console_putchar() can be called from anywhere else than
> > atmel_console_write().
> Actually, not so much. I added them for protection so that we wouldn't run into
> a deadlock loop if something changes again. Those while() loops make me cautious.
> And I didn't have the time to look up if the suspend code is thread-safe without this.

I don't think it's much safer with that extra check if suspend can be
called in parallel with this. In that case, the only way to make
_absolutely_ sure would be to move the test inside the while loops.

And I only think this actually matters if console suspend is disabled,
which should probably never happen on a production system. So I'm
wondering if we really need any of those checks.

> I had once a bug in my RTT driver (before there was any RTT driver in mainline),
> that left the RTT interrupt flag set and this blocked the sysc interrupt. This was
> a pain to debug and I try to avoid such problems as much as I can.
> With RTT, even the WDT did not help as the WDT does not reset RTT and so if this
> happened, the device just ran into WDT resetting loop until the battery died.

Understand. 

> > 
> > Also, does the no_console_suspend parameter still work after this?
> Didn't try that one as I do only slow clock suspend, not standby.

Ok, I'll try it.

> Actually, I had an idea to implement the wakeup with GPIO interrupt. So that
> we could wake up even when we're running on slow clock.

Yes, that would be pretty cool.

Haavard

Re: [PATCH] AT91 Serial: update the powersave handler to match serial core

[Haavard Skinnemoen]

Haavard Skinnemoen <haavard.skinnemoen@atmel.com> wrote:
> > > Are both of these checks necessary? It doesn't look like
> > > atmel_console_putchar() can be called from anywhere else than
> > > atmel_console_write().  
> > Actually, not so much. I added them for protection so that we wouldn't run into
> > a deadlock loop if something changes again. Those while() loops make me cautious.
> > And I didn't have the time to look up if the suspend code is thread-safe without this.  
> 
> I don't think it's much safer with that extra check if suspend can be
> called in parallel with this. In that case, the only way to make
> _absolutely_ sure would be to move the test inside the while loops.
> 
> And I only think this actually matters if console suspend is disabled,
> which should probably never happen on a production system. So I'm
> wondering if we really need any of those checks.

After looking at the serial core code, I don't think they are needed.
The serial core shuts down the port completely before changing the pm
state, and it also calls console_stop() which ensures that
atmel_console_write() will never get called.

And I think this is by far the best way to deal with this. As long as
the console is disabled at a higher level, we don't need those
potentially racy checks in the low-level code. So I think I'm going to
remove them before submitting this patch upstream.

If no_console_suspend is set, none of this will happen, and the clock
will keep running until it eventually gets cut off when entering slow
clock mode. This could have potential issues, but I'm not sure if it's
worth dealing with them since no_console_suspend is purely a debugging
aid.

I also suspect that disabling interrupts is unnecessary since the
serial core calls ->shutdown() which will also disable interrupts. But
it's probably safest to keep that part since the pm state may get
changed by other things than suspend.

Haavard

Re: [PATCH] AT91 Serial: update the powersave handler to match serial core

[Alan Cox]

> Maybe the tty layer ensures that the buffer is empty before
> suspending...would be interesting to know. In any case, any such bug

It doesn't - but calling tty_wait_until_sent(tty, some_timeout) will do
that for you providing you call it in a sleep capable context.

Alan

Re: [PATCH] AT91 Serial: update the powersave handler to match serial core

[Haavard Skinnemoen]

Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > Maybe the tty layer ensures that the buffer is empty before
> > suspending...would be interesting to know. In any case, any such bug  
> 
> It doesn't - but calling tty_wait_until_sent(tty, some_timeout) will do
> that for you providing you call it in a sleep capable context.

Looks like it doesn't matter -- the serial core shuts the port down
completely, which will among other things disable the DMA controller.

Haavard