[PATCHv6] OMAP3: Serial: Improved sleep logic

[PATCHv6] OMAP3: Serial: Improved sleep logic

[Tero Kristo]

From: Tero Kristo <tero.kristo@nokia.com>

This patch contains following improvements:
- Only RX interrupt will now kick the sleep prevent timer
- TX fifo status is checked before disabling clocks, this will prevent
  on-going transmission to be cut
- Smartidle is now enabled/disabled only while switching clocks, as having
  smartidle enabled while RX/TX prevents any wakeups from being received
  from UART module
- Added workqueue for wakeup checks, as jiffy timer access within the
  idle loop results into skewed timers as jiffy timers are stopped
- Added garbage_timer for ignoring the first character received during
  the first tick after clock enable, this prevents garbage characters to be
  received in low sleep states
- omap_uart_enable_irqs() changed to use enable_irq / disable_irq instead
  of request / free. Using request/free changes the behavior after first
  suspend due to reversed interrupt handler ordering

Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
---
 arch/arm/mach-omap2/serial.c |   67 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 5f3035e..06d18f5 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -23,6 +23,7 @@
 #include <linux/serial_reg.h>
 #include <linux/clk.h>
 #include <linux/io.h>
+#include <linux/workqueue.h>
 
 #include <plat/common.h>
 #include <plat/board.h>
@@ -48,7 +49,10 @@ struct omap_uart_state {
 	int num;
 	int can_sleep;
 	struct timer_list timer;
+	struct timer_list garbage_timer;
+	struct work_struct wakeup_work;
 	u32 timeout;
+	u8 garbage_ignore;
 
 	void __iomem *wk_st;
 	void __iomem *wk_en;
@@ -243,6 +247,11 @@ static inline void omap_uart_save_context(struct omap_uart_state *uart) {}
 static inline void omap_uart_restore_context(struct omap_uart_state *uart) {}
 #endif /* CONFIG_PM && CONFIG_ARCH_OMAP3 */
 
+#ifdef CONFIG_PM
+static void omap_uart_smart_idle_enable(struct omap_uart_state *uart,
+		int enable);
+#endif
+
 static inline void omap_uart_enable_clocks(struct omap_uart_state *uart)
 {
 	if (uart->clocked)
@@ -252,6 +261,15 @@ static inline void omap_uart_enable_clocks(struct omap_uart_state *uart)
 	clk_enable(uart->fck);
 	uart->clocked = 1;
 	omap_uart_restore_context(uart);
+#ifdef CONFIG_PM
+	omap_uart_smart_idle_enable(uart, 0);
+#endif
+
+	/* Set up garbage timer to ignore RX during first jiffy */
+	if (uart->timeout) {
+		mod_timer(&uart->garbage_timer, jiffies + 1);
+		uart->garbage_ignore = 1;
+	}
 }
 
 #ifdef CONFIG_PM
@@ -263,6 +281,7 @@ static inline void omap_uart_disable_clocks(struct omap_uart_state *uart)
 
 	omap_uart_save_context(uart);
 	uart->clocked = 0;
+	omap_uart_smart_idle_enable(uart, 1);
 	clk_disable(uart->ick);
 	clk_disable(uart->fck);
 }
@@ -320,7 +339,6 @@ static void omap_uart_block_sleep(struct omap_uart_state *uart)
 {
 	omap_uart_enable_clocks(uart);
 
-	omap_uart_smart_idle_enable(uart, 0);
 	uart->can_sleep = 0;
 	if (uart->timeout)
 		mod_timer(&uart->timer, jiffies + uart->timeout);
@@ -338,7 +356,6 @@ static void omap_uart_allow_sleep(struct omap_uart_state *uart)
 	if (!uart->clocked)
 		return;
 
-	omap_uart_smart_idle_enable(uart, 1);
 	uart->can_sleep = 1;
 	del_timer(&uart->timer);
 }
@@ -350,13 +367,30 @@ static void omap_uart_idle_timer(unsigned long data)
 	omap_uart_allow_sleep(uart);
 }
 
+static void omap_uart_garbage_timer(unsigned long data)
+{
+	struct omap_uart_state *uart = (struct omap_uart_state *)data;
+
+	uart->garbage_ignore = 0;
+}
+
+static void omap_uart_wakeup_work(struct work_struct *work)
+{
+	struct omap_uart_state *uart =
+		container_of(work, struct omap_uart_state, wakeup_work);
+
+	omap_uart_block_sleep(uart);
+}
+
 void omap_uart_prepare_idle(int num)
 {
 	struct omap_uart_state *uart;
 
 	list_for_each_entry(uart, &uart_list, node) {
 		if (num == uart->num && uart->can_sleep) {
-			omap_uart_disable_clocks(uart);
+			if (serial_read_reg(uart->p, UART_LSR) &
+					UART_LSR_TEMT)
+				omap_uart_disable_clocks(uart);
 			return;
 		}
 	}
@@ -375,12 +409,12 @@ void omap_uart_resume_idle(int num)
 				u16 p = omap_ctrl_readw(uart->padconf);
 
 				if (p & OMAP3_PADCONF_WAKEUPEVENT0)
-					omap_uart_block_sleep(uart);
+					schedule_work(&uart->wakeup_work);
 			}
 
 			/* Check for normal UART wakeup */
 			if (__raw_readl(uart->wk_st) & uart->wk_mask)
-				omap_uart_block_sleep(uart);
+				schedule_work(&uart->wakeup_work);
 			return;
 		}
 	}
@@ -428,8 +462,18 @@ int omap_uart_can_sleep(void)
 static irqreturn_t omap_uart_interrupt(int irq, void *dev_id)
 {
 	struct omap_uart_state *uart = dev_id;
+	u8 lsr;
 
-	omap_uart_block_sleep(uart);
+	lsr = serial_read_reg(uart->p, UART_LSR);
+	/* Check for receive interrupt */
+	if (lsr & UART_LSR_DR) {
+		omap_uart_block_sleep(uart);
+		if (uart->garbage_ignore) {
+			del_timer(&uart->garbage_timer);
+			uart->garbage_ignore = 0;
+			serial_read_reg(uart->p, UART_RX);
+		}
+	}
 
 	return IRQ_NONE;
 }
@@ -443,6 +487,9 @@ static void omap_uart_idle_init(struct omap_uart_state *uart)
 	uart->timeout = DEFAULT_TIMEOUT;
 	setup_timer(&uart->timer, omap_uart_idle_timer,
 		    (unsigned long) uart);
+	setup_timer(&uart->garbage_timer, omap_uart_garbage_timer,
+		    (unsigned long) uart);
+	INIT_WORK(&uart->wakeup_work, omap_uart_wakeup_work);
 	if (uart->timeout)
 		mod_timer(&uart->timer, jiffies + uart->timeout);
 	omap_uart_smart_idle_enable(uart, 0);
@@ -507,15 +554,13 @@ static void omap_uart_idle_init(struct omap_uart_state *uart)
 
 void omap_uart_enable_irqs(int enable)
 {
-	int ret;
 	struct omap_uart_state *uart;
 
 	list_for_each_entry(uart, &uart_list, node) {
 		if (enable)
-			ret = request_irq(uart->p->irq, omap_uart_interrupt,
-				IRQF_SHARED, "serial idle", (void *)uart);
+			enable_irq(uart->p->irq);
 		else
-			free_irq(uart->p->irq, (void *)uart);
+			disable_irq(uart->p->irq);
 	}
 }
 
-- 
1.5.4.3

Re: [PATCHv6] OMAP3: Serial: Improved sleep logic

[Kevin Hilman]

Tero Kristo <tero.kristo@nokia.com> writes:

> From: Tero Kristo <tero.kristo@nokia.com>
>
> This patch contains following improvements:
> - Only RX interrupt will now kick the sleep prevent timer
> - TX fifo status is checked before disabling clocks, this will prevent
>   on-going transmission to be cut
> - Smartidle is now enabled/disabled only while switching clocks, as having
>   smartidle enabled while RX/TX prevents any wakeups from being received
>   from UART module
> - Added workqueue for wakeup checks, as jiffy timer access within the
>   idle loop results into skewed timers as jiffy timers are stopped
> - Added garbage_timer for ignoring the first character received during
>   the first tick after clock enable, this prevents garbage characters to be
>   received in low sleep states
> - omap_uart_enable_irqs() changed to use enable_irq / disable_irq instead
>   of request / free. Using request/free changes the behavior after first
>   suspend due to reversed interrupt handler ordering
>
> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>

Thanks Tero.  This version looks good.

Adding to pm-fixes queue for 2.6.34-rcX after minor change below...

> ---
>  arch/arm/mach-omap2/serial.c |   67 +++++++++++++++++++++++++++++++++++-------
>  1 files changed, 56 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 5f3035e..06d18f5 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -23,6 +23,7 @@
>  #include <linux/serial_reg.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
> +#include <linux/workqueue.h>
>  
>  #include <plat/common.h>
>  #include <plat/board.h>
> @@ -48,7 +49,10 @@ struct omap_uart_state {
>  	int num;
>  	int can_sleep;
>  	struct timer_list timer;
> +	struct timer_list garbage_timer;
> +	struct work_struct wakeup_work;
>  	u32 timeout;
> +	u8 garbage_ignore;
>  
>  	void __iomem *wk_st;
>  	void __iomem *wk_en;
> @@ -243,6 +247,11 @@ static inline void omap_uart_save_context(struct omap_uart_state *uart) {}
>  static inline void omap_uart_restore_context(struct omap_uart_state *uart) {}
>  #endif /* CONFIG_PM && CONFIG_ARCH_OMAP3 */
>  
> +#ifdef CONFIG_PM
> +static void omap_uart_smart_idle_enable(struct omap_uart_state *uart,
> +		int enable);
> +#endif

I moved this up into the #ifdef block just above and added a dummy function
into the #else clause...

>  static inline void omap_uart_enable_clocks(struct omap_uart_state *uart)
>  {
>  	if (uart->clocked)
> @@ -252,6 +261,15 @@ static inline void omap_uart_enable_clocks(struct omap_uart_state *uart)
>  	clk_enable(uart->fck);
>  	uart->clocked = 1;
>  	omap_uart_restore_context(uart);
> +#ifdef CONFIG_PM
> +	omap_uart_smart_idle_enable(uart, 0);
> +#endif

and then dropped this #ifdef.

> +	/* Set up garbage timer to ignore RX during first jiffy */
> +	if (uart->timeout) {
> +		mod_timer(&uart->garbage_timer, jiffies + 1);
> +		uart->garbage_ignore = 1;
> +	}
>  }

Kevin

Re: [PATCHv6] OMAP3: Serial: Improved sleep logic

[Kevin Hilman]

Kevin Hilman <khilman@deeprootsystems.com> writes:

> Tero Kristo <tero.kristo@nokia.com> writes:
>
>> From: Tero Kristo <tero.kristo@nokia.com>
>>
>> This patch contains following improvements:
>> - Only RX interrupt will now kick the sleep prevent timer
>> - TX fifo status is checked before disabling clocks, this will prevent
>>   on-going transmission to be cut
>> - Smartidle is now enabled/disabled only while switching clocks, as having
>>   smartidle enabled while RX/TX prevents any wakeups from being received
>>   from UART module
>> - Added workqueue for wakeup checks, as jiffy timer access within the
>>   idle loop results into skewed timers as jiffy timers are stopped
>> - Added garbage_timer for ignoring the first character received during
>>   the first tick after clock enable, this prevents garbage characters to be
>>   received in low sleep states
>> - omap_uart_enable_irqs() changed to use enable_irq / disable_irq instead
>>   of request / free. Using request/free changes the behavior after first
>>   suspend due to reversed interrupt handler ordering
>>
>> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
>
> Thanks Tero.  This version looks good.
>
> Adding to pm-fixes queue for 2.6.34-rcX after minor change below...
>

There's still something slightly strange going on here...

I noticed via powertop that the garbage timer is now one of the top
reasons for wakeup in an idle system.  Seems like the garbage timer
is firing when it shouldn't be, and triggering unnecessary wakeups

Based on powertop stats, the garbage timer is firing about 3x more
often than the GPtimer that should be waking the system.

I haven't dug any deeper, but this needs to be fixed before merging
upstream.

Kevin

RE: [PATCHv6] OMAP3: Serial: Improved sleep logic

[Tero.Kristo]

 

>-----Original Message-----
>From: ext Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>Sent: 10 March, 2010 20:21
>To: Kristo Tero (Nokia-D/Tampere)
>Cc: linux-omap@vger.kernel.org
>Subject: Re: [PATCHv6] OMAP3: Serial: Improved sleep logic
>
>Kevin Hilman <khilman@deeprootsystems.com> writes:
>
>> Tero Kristo <tero.kristo@nokia.com> writes:
>>
>>> From: Tero Kristo <tero.kristo@nokia.com>
>>>
>>> This patch contains following improvements:
>>> - Only RX interrupt will now kick the sleep prevent timer
>>> - TX fifo status is checked before disabling clocks, this 
>will prevent
>>>   on-going transmission to be cut
>>> - Smartidle is now enabled/disabled only while switching 
>clocks, as having
>>>   smartidle enabled while RX/TX prevents any wakeups from 
>being received
>>>   from UART module
>>> - Added workqueue for wakeup checks, as jiffy timer access 
>within the
>>>   idle loop results into skewed timers as jiffy timers are stopped
>>> - Added garbage_timer for ignoring the first character 
>received during
>>>   the first tick after clock enable, this prevents garbage 
>characters to be
>>>   received in low sleep states
>>> - omap_uart_enable_irqs() changed to use enable_irq / 
>disable_irq instead
>>>   of request / free. Using request/free changes the 
>behavior after first
>>>   suspend due to reversed interrupt handler ordering
>>>
>>> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
>>
>> Thanks Tero.  This version looks good.
>>
>> Adding to pm-fixes queue for 2.6.34-rcX after minor change below...
>>
>
>There's still something slightly strange going on here...
>
>I noticed via powertop that the garbage timer is now one of the top
>reasons for wakeup in an idle system.  Seems like the garbage timer
>is firing when it shouldn't be, and triggering unnecessary wakeups
>
>Based on powertop stats, the garbage timer is firing about 3x more
>often than the GPtimer that should be waking the system.

You get one timer expire each wakeup cycle, but the system actually fires its own timer for each UART, thus you get 3x. It is possible to optimize the timer a bit by only firing it if we have a wakeup from UART, but this probably causes occasional garbage to the console, if a wakeup from some other source than UART and UART RX happen at the same time. Might be the lesser of two evils though.

I can experiment with this change a bit and see how it behaves.

>
>I haven't dug any deeper, but this needs to be fixed before merging
>upstream.
>
>Kevin
>

Re: [PATCHv6] OMAP3: Serial: Improved sleep logic

[Kevin Hilman]

<Tero.Kristo@nokia.com> writes:

>  
>
>>-----Original Message-----
>>From: ext Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>>Sent: 10 March, 2010 20:21
>>To: Kristo Tero (Nokia-D/Tampere)
>>Cc: linux-omap@vger.kernel.org
>>Subject: Re: [PATCHv6] OMAP3: Serial: Improved sleep logic
>>
>>Kevin Hilman <khilman@deeprootsystems.com> writes:
>>
>>> Tero Kristo <tero.kristo@nokia.com> writes:
>>>
>>>> From: Tero Kristo <tero.kristo@nokia.com>
>>>>
>>>> This patch contains following improvements:
>>>> - Only RX interrupt will now kick the sleep prevent timer
>>>> - TX fifo status is checked before disabling clocks, this 
>>will prevent
>>>>   on-going transmission to be cut
>>>> - Smartidle is now enabled/disabled only while switching 
>>clocks, as having
>>>>   smartidle enabled while RX/TX prevents any wakeups from 
>>being received
>>>>   from UART module
>>>> - Added workqueue for wakeup checks, as jiffy timer access 
>>within the
>>>>   idle loop results into skewed timers as jiffy timers are stopped
>>>> - Added garbage_timer for ignoring the first character 
>>received during
>>>>   the first tick after clock enable, this prevents garbage 
>>characters to be
>>>>   received in low sleep states
>>>> - omap_uart_enable_irqs() changed to use enable_irq / 
>>disable_irq instead
>>>>   of request / free. Using request/free changes the 
>>behavior after first
>>>>   suspend due to reversed interrupt handler ordering
>>>>
>>>> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
>>>
>>> Thanks Tero.  This version looks good.
>>>
>>> Adding to pm-fixes queue for 2.6.34-rcX after minor change below...
>>>
>>
>>There's still something slightly strange going on here...
>>
>>I noticed via powertop that the garbage timer is now one of the top
>>reasons for wakeup in an idle system.  Seems like the garbage timer
>>is firing when it shouldn't be, and triggering unnecessary wakeups
>>
>>Based on powertop stats, the garbage timer is firing about 3x more
>>often than the GPtimer that should be waking the system.
>
> You get one timer expire each wakeup cycle, but the system actually fires its own timer for each UART, thus you get 3x. It is possible to optimize the timer a bit by only firing it if we have a wakeup from UART, but this probably causes occasional garbage to the console, if a wakeup from some other source than UART and UART RX happen at the same time. Might be the lesser of two evils though.
>
> I can experiment with this change a bit and see how it behaves.
>

OK, the 3x makes sense, but the garbage timer should never be the
cause of a wakeup.

Maybe you also need to be sure that the garbage timer is disabled
before clocks are disabled so it doesn't fire and cause a wakeup.

Kevin

RE: [PATCHv6] OMAP3: Serial: Improved sleep logic

[Tero.Kristo]

 

>-----Original Message-----
>From: ext Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>Sent: 11 March, 2010 18:30
>To: Kristo Tero (Nokia-D/Tampere)
>Cc: linux-omap@vger.kernel.org
>Subject: Re: [PATCHv6] OMAP3: Serial: Improved sleep logic
>
><Tero.Kristo@nokia.com> writes:
>
>>  
>>
>>>-----Original Message-----
>>>From: ext Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>>>Sent: 10 March, 2010 20:21
>>>To: Kristo Tero (Nokia-D/Tampere)
>>>Cc: linux-omap@vger.kernel.org
>>>Subject: Re: [PATCHv6] OMAP3: Serial: Improved sleep logic
>>>
>>>Kevin Hilman <khilman@deeprootsystems.com> writes:
>>>
>>>> Tero Kristo <tero.kristo@nokia.com> writes:
>>>>
>>>>> From: Tero Kristo <tero.kristo@nokia.com>
>>>>>
>>>>> This patch contains following improvements:
>>>>> - Only RX interrupt will now kick the sleep prevent timer
>>>>> - TX fifo status is checked before disabling clocks, this 
>>>will prevent
>>>>>   on-going transmission to be cut
>>>>> - Smartidle is now enabled/disabled only while switching 
>>>clocks, as having
>>>>>   smartidle enabled while RX/TX prevents any wakeups from 
>>>being received
>>>>>   from UART module
>>>>> - Added workqueue for wakeup checks, as jiffy timer access 
>>>within the
>>>>>   idle loop results into skewed timers as jiffy timers are stopped
>>>>> - Added garbage_timer for ignoring the first character 
>>>received during
>>>>>   the first tick after clock enable, this prevents garbage 
>>>characters to be
>>>>>   received in low sleep states
>>>>> - omap_uart_enable_irqs() changed to use enable_irq / 
>>>disable_irq instead
>>>>>   of request / free. Using request/free changes the 
>>>behavior after first
>>>>>   suspend due to reversed interrupt handler ordering
>>>>>
>>>>> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
>>>>
>>>> Thanks Tero.  This version looks good.
>>>>
>>>> Adding to pm-fixes queue for 2.6.34-rcX after minor change below...
>>>>
>>>
>>>There's still something slightly strange going on here...
>>>
>>>I noticed via powertop that the garbage timer is now one of the top
>>>reasons for wakeup in an idle system.  Seems like the garbage timer
>>>is firing when it shouldn't be, and triggering unnecessary wakeups
>>>
>>>Based on powertop stats, the garbage timer is firing about 3x more
>>>often than the GPtimer that should be waking the system.
>>
>> You get one timer expire each wakeup cycle, but the system 
>actually fires its own timer for each UART, thus you get 3x. 
>It is possible to optimize the timer a bit by only firing it 
>if we have a wakeup from UART, but this probably causes 
>occasional garbage to the console, if a wakeup from some other 
>source than UART and UART RX happen at the same time. Might be 
>the lesser of two evils though.
>>
>> I can experiment with this change a bit and see how it behaves.
>>
>
>OK, the 3x makes sense, but the garbage timer should never be the
>cause of a wakeup.
>
>Maybe you also need to be sure that the garbage timer is disabled
>before clocks are disabled so it doesn't fire and cause a wakeup.

Yeah, that's another addition that I can do, but I don't think the system is actually woken up even though powertop claims so. Powertop is not too reliable wakeup source indicator, it only shows the number of expired timers / interrupts in most cases, but it does not know whether the device was sleeping when the interrupt was raised or not. The number of wakeups can be reliably seen from the number of PRCM interrupts that have occurred, but powertop does not parse the results too well for OMAP3 currently. And, there is no wakeup source indicator anyway available that would be exported from the kernel currently. I have fiddled at some point with a patch that does this, but I never posted it anywhere.

-Tero