[PATCHv5] OMAP3: Serial: Improved sleep logic

[PATCHv5] 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 interrupts from being
  received from UART module
- Sleep prevent timer is changed to use timespec instead of a jiffy timer
  as jiffy timers are not valid within idle loop (tick scheduler is stopped)
- Added RX ignore timer for ignoring the first character received during
  first millisecond of wakeup, this prevents garbage character to be receive
  in low sleep states

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

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 5f3035e..f49c465 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -29,6 +29,8 @@
 #include <plat/clock.h>
 #include <plat/control.h>
 
+#include <asm/div64.h>
+
 #include "prm.h"
 #include "pm.h"
 #include "prm-regbits-34xx.h"
@@ -42,13 +44,14 @@
  * disabled via sysfs. This also causes that any deeper omap sleep states are
  * blocked. 
  */
-#define DEFAULT_TIMEOUT 0
+#define DEFAULT_TIMEOUT (0LL * NSEC_PER_SEC)
 
 struct omap_uart_state {
 	int num;
 	int can_sleep;
-	struct timer_list timer;
-	u32 timeout;
+	struct timespec expire_time;
+	struct timespec garbage_time;
+	u64 timeout;
 
 	void __iomem *wk_st;
 	void __iomem *wk_en;
@@ -243,6 +246,9 @@ 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 */
 
+static void omap_uart_smart_idle_enable(struct omap_uart_state *uart,
+		int enable);
+
 static inline void omap_uart_enable_clocks(struct omap_uart_state *uart)
 {
 	if (uart->clocked)
@@ -250,8 +256,13 @@ static inline void omap_uart_enable_clocks(struct omap_uart_state *uart)
 
 	clk_enable(uart->ick);
 	clk_enable(uart->fck);
+	omap_uart_smart_idle_enable(uart, 0);
 	uart->clocked = 1;
 	omap_uart_restore_context(uart);
+
+	/* Set up garbage timer to ignore RX during first 1ms */
+	getrawmonotonic(&uart->garbage_time);
+	timespec_add_ns(&uart->garbage_time, NSEC_PER_MSEC);
 }
 
 #ifdef CONFIG_PM
@@ -263,6 +274,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,12 +332,11 @@ 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);
-	else
-		del_timer(&uart->timer);
+	if (uart->timeout) {
+		getrawmonotonic(&uart->expire_time);
+		timespec_add_ns(&uart->expire_time, uart->timeout);
+	}
 }
 
 static void omap_uart_allow_sleep(struct omap_uart_state *uart)
@@ -338,25 +349,24 @@ 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);
-}
-
-static void omap_uart_idle_timer(unsigned long data)
-{
-	struct omap_uart_state *uart = (struct omap_uart_state *)data;
-
-	omap_uart_allow_sleep(uart);
 }
 
 void omap_uart_prepare_idle(int num)
 {
 	struct omap_uart_state *uart;
+	struct timespec t;
 
 	list_for_each_entry(uart, &uart_list, node) {
+		if (num == uart->num && !uart->can_sleep && uart->timeout) {
+			getrawmonotonic(&t);
+			if (timespec_compare(&t, &uart->expire_time) > 0)
+				uart->can_sleep = 1;
+		}
 		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;
 		}
 	}
@@ -381,6 +391,7 @@ void omap_uart_resume_idle(int num)
 			/* Check for normal UART wakeup */
 			if (__raw_readl(uart->wk_st) & uart->wk_mask)
 				omap_uart_block_sleep(uart);
+
 			return;
 		}
 	}
@@ -399,11 +410,18 @@ int omap_uart_can_sleep(void)
 {
 	struct omap_uart_state *uart;
 	int can_sleep = 1;
+	struct timespec t;
 
 	list_for_each_entry(uart, &uart_list, node) {
 		if (!uart->clocked)
 			continue;
 
+		if (!uart->can_sleep && uart->timeout) {
+			getrawmonotonic(&t);
+			if (timespec_compare(&t, &uart->expire_time) > 0)
+				uart->can_sleep = 1;
+		}
+
 		if (!uart->can_sleep) {
 			can_sleep = 0;
 			continue;
@@ -428,10 +446,25 @@ 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;
+	int ret = IRQ_NONE;
+	struct timespec t;
 
-	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_time.tv_sec) {
+			getrawmonotonic(&t);
+			if (timespec_compare(&t, &uart->garbage_time) < 0) {
+				serial_read_reg(uart->p, UART_RX);
+				uart->garbage_time.tv_sec = 0;
+				ret = IRQ_HANDLED;
+			}
+		}
+	}
 
-	return IRQ_NONE;
+	return ret;
 }
 
 static void omap_uart_idle_init(struct omap_uart_state *uart)
@@ -441,10 +474,12 @@ static void omap_uart_idle_init(struct omap_uart_state *uart)
 
 	uart->can_sleep = 0;
 	uart->timeout = DEFAULT_TIMEOUT;
-	setup_timer(&uart->timer, omap_uart_idle_timer,
-		    (unsigned long) uart);
-	if (uart->timeout)
-		mod_timer(&uart->timer, jiffies + uart->timeout);
+
+	if (uart->timeout) {
+		getrawmonotonic(&uart->expire_time);
+		timespec_add_ns(&uart->expire_time, uart->timeout);
+	}
+
 	omap_uart_smart_idle_enable(uart, 0);
 
 	if (cpu_is_omap34xx()) {
@@ -527,8 +562,12 @@ static ssize_t sleep_timeout_show(struct device *dev,
 					struct platform_device, dev);
 	struct omap_uart_state *uart = container_of(pdev,
 					struct omap_uart_state, pdev);
+	u64 val;
+
+	val = uart->timeout;
 
-	return sprintf(buf, "%u\n", uart->timeout / HZ);
+	do_div(val, NSEC_PER_SEC);
+	return sprintf(buf, "%llu\n", val);
 }
 
 static ssize_t sleep_timeout_store(struct device *dev,
@@ -546,10 +585,11 @@ static ssize_t sleep_timeout_store(struct device *dev,
 		return -EINVAL;
 	}
 
-	uart->timeout = value * HZ;
-	if (uart->timeout)
-		mod_timer(&uart->timer, jiffies + uart->timeout);
-	else
+	uart->timeout = (u64)value * NSEC_PER_SEC;
+	if (uart->timeout) {
+		getrawmonotonic(&uart->expire_time);
+		timespec_add_ns(&uart->expire_time, uart->timeout);
+	} else
 		/* A zero value means disable timeout feature */
 		omap_uart_block_sleep(uart);
 
-- 
1.5.4.3

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

[Tony Lindgren]

* Tero Kristo <tero.kristo@nokia.com> [100217 06:01]:
> 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 interrupts from being
>   received from UART module
> - Sleep prevent timer is changed to use timespec instead of a jiffy timer
>   as jiffy timers are not valid within idle loop (tick scheduler is stopped)
> - Added RX ignore timer for ignoring the first character received during
>   first millisecond of wakeup, this prevents garbage character to be receive
>   in low sleep states
> 
> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>

Kevin, do you have any comments on this?

Regards,

Tony


> ---
>  arch/arm/mach-omap2/serial.c |   98 +++++++++++++++++++++++++++++------------
>  1 files changed, 69 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 5f3035e..f49c465 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -29,6 +29,8 @@
>  #include <plat/clock.h>
>  #include <plat/control.h>
>  
> +#include <asm/div64.h>
> +
>  #include "prm.h"
>  #include "pm.h"
>  #include "prm-regbits-34xx.h"
> @@ -42,13 +44,14 @@
>   * disabled via sysfs. This also causes that any deeper omap sleep states are
>   * blocked. 
>   */
> -#define DEFAULT_TIMEOUT 0
> +#define DEFAULT_TIMEOUT (0LL * NSEC_PER_SEC)
>  
>  struct omap_uart_state {
>  	int num;
>  	int can_sleep;
> -	struct timer_list timer;
> -	u32 timeout;
> +	struct timespec expire_time;
> +	struct timespec garbage_time;
> +	u64 timeout;
>  
>  	void __iomem *wk_st;
>  	void __iomem *wk_en;
> @@ -243,6 +246,9 @@ 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 */
>  
> +static void omap_uart_smart_idle_enable(struct omap_uart_state *uart,
> +		int enable);
> +
>  static inline void omap_uart_enable_clocks(struct omap_uart_state *uart)
>  {
>  	if (uart->clocked)
> @@ -250,8 +256,13 @@ static inline void omap_uart_enable_clocks(struct omap_uart_state *uart)
>  
>  	clk_enable(uart->ick);
>  	clk_enable(uart->fck);
> +	omap_uart_smart_idle_enable(uart, 0);
>  	uart->clocked = 1;
>  	omap_uart_restore_context(uart);
> +
> +	/* Set up garbage timer to ignore RX during first 1ms */
> +	getrawmonotonic(&uart->garbage_time);
> +	timespec_add_ns(&uart->garbage_time, NSEC_PER_MSEC);
>  }
>  
>  #ifdef CONFIG_PM
> @@ -263,6 +274,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,12 +332,11 @@ 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);
> -	else
> -		del_timer(&uart->timer);
> +	if (uart->timeout) {
> +		getrawmonotonic(&uart->expire_time);
> +		timespec_add_ns(&uart->expire_time, uart->timeout);
> +	}
>  }
>  
>  static void omap_uart_allow_sleep(struct omap_uart_state *uart)
> @@ -338,25 +349,24 @@ 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);
> -}
> -
> -static void omap_uart_idle_timer(unsigned long data)
> -{
> -	struct omap_uart_state *uart = (struct omap_uart_state *)data;
> -
> -	omap_uart_allow_sleep(uart);
>  }
>  
>  void omap_uart_prepare_idle(int num)
>  {
>  	struct omap_uart_state *uart;
> +	struct timespec t;
>  
>  	list_for_each_entry(uart, &uart_list, node) {
> +		if (num == uart->num && !uart->can_sleep && uart->timeout) {
> +			getrawmonotonic(&t);
> +			if (timespec_compare(&t, &uart->expire_time) > 0)
> +				uart->can_sleep = 1;
> +		}
>  		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;
>  		}
>  	}
> @@ -381,6 +391,7 @@ void omap_uart_resume_idle(int num)
>  			/* Check for normal UART wakeup */
>  			if (__raw_readl(uart->wk_st) & uart->wk_mask)
>  				omap_uart_block_sleep(uart);
> +
>  			return;
>  		}
>  	}
> @@ -399,11 +410,18 @@ int omap_uart_can_sleep(void)
>  {
>  	struct omap_uart_state *uart;
>  	int can_sleep = 1;
> +	struct timespec t;
>  
>  	list_for_each_entry(uart, &uart_list, node) {
>  		if (!uart->clocked)
>  			continue;
>  
> +		if (!uart->can_sleep && uart->timeout) {
> +			getrawmonotonic(&t);
> +			if (timespec_compare(&t, &uart->expire_time) > 0)
> +				uart->can_sleep = 1;
> +		}
> +
>  		if (!uart->can_sleep) {
>  			can_sleep = 0;
>  			continue;
> @@ -428,10 +446,25 @@ 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;
> +	int ret = IRQ_NONE;
> +	struct timespec t;
>  
> -	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_time.tv_sec) {
> +			getrawmonotonic(&t);
> +			if (timespec_compare(&t, &uart->garbage_time) < 0) {
> +				serial_read_reg(uart->p, UART_RX);
> +				uart->garbage_time.tv_sec = 0;
> +				ret = IRQ_HANDLED;
> +			}
> +		}
> +	}
>  
> -	return IRQ_NONE;
> +	return ret;
>  }
>  
>  static void omap_uart_idle_init(struct omap_uart_state *uart)
> @@ -441,10 +474,12 @@ static void omap_uart_idle_init(struct omap_uart_state *uart)
>  
>  	uart->can_sleep = 0;
>  	uart->timeout = DEFAULT_TIMEOUT;
> -	setup_timer(&uart->timer, omap_uart_idle_timer,
> -		    (unsigned long) uart);
> -	if (uart->timeout)
> -		mod_timer(&uart->timer, jiffies + uart->timeout);
> +
> +	if (uart->timeout) {
> +		getrawmonotonic(&uart->expire_time);
> +		timespec_add_ns(&uart->expire_time, uart->timeout);
> +	}
> +
>  	omap_uart_smart_idle_enable(uart, 0);
>  
>  	if (cpu_is_omap34xx()) {
> @@ -527,8 +562,12 @@ static ssize_t sleep_timeout_show(struct device *dev,
>  					struct platform_device, dev);
>  	struct omap_uart_state *uart = container_of(pdev,
>  					struct omap_uart_state, pdev);
> +	u64 val;
> +
> +	val = uart->timeout;
>  
> -	return sprintf(buf, "%u\n", uart->timeout / HZ);
> +	do_div(val, NSEC_PER_SEC);
> +	return sprintf(buf, "%llu\n", val);
>  }
>  
>  static ssize_t sleep_timeout_store(struct device *dev,
> @@ -546,10 +585,11 @@ static ssize_t sleep_timeout_store(struct device *dev,
>  		return -EINVAL;
>  	}
>  
> -	uart->timeout = value * HZ;
> -	if (uart->timeout)
> -		mod_timer(&uart->timer, jiffies + uart->timeout);
> -	else
> +	uart->timeout = (u64)value * NSEC_PER_SEC;
> +	if (uart->timeout) {
> +		getrawmonotonic(&uart->expire_time);
> +		timespec_add_ns(&uart->expire_time, uart->timeout);
> +	} else
>  		/* A zero value means disable timeout feature */
>  		omap_uart_block_sleep(uart);
>  
> -- 
> 1.5.4.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv5] 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 interrupts from being
>   received from UART module
> - Sleep prevent timer is changed to use timespec instead of a jiffy timer
>   as jiffy timers are not valid within idle loop (tick scheduler is stopped)

Could also probably use hrtimers for this.

That being said, I'm not sure what is the problem you're trying to
solve with this change.  I don't see any timings that are timing events
inside the idle loop.

> - Added RX ignore timer for ignoring the first character received during
>   first millisecond of wakeup, this prevents garbage character to be receive
>   in low sleep states
>
> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>

Something is still not quite right here.

This doesn't work on my n900 here when testing this patch on top of
the PM branch.  The default is now a default timeout of zero.  When I
enable a 5 sec. timeout for UART2 on RX-51, when the timer expires, I
no longer have response on the console.

To test, I booted my n900 with init=/bin/sh to avoid all the setup
done by /sbin/preinit.  Then I enabled a timeout for UART2 only, and
then the console hangs.

Here's my hunch as to what's happening:

I think the problem is a deadlock in getrawmonotonic().  Nested calls
here will deadlock due to the xtime_lock being held.

When updading the timeout, sleep_timeout_store() does a
getrawmonotonic() to update the expiry time.  While this happening,
the UART interrupt could fire, causing an omap_uart_block_sleep()
which would also getrawmonotonic() and deadlock in interrupt mode.

Kevin

> ---
>  arch/arm/mach-omap2/serial.c |   98 +++++++++++++++++++++++++++++------------
>  1 files changed, 69 insertions(+), 29 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 5f3035e..f49c465 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -29,6 +29,8 @@
>  #include <plat/clock.h>
>  #include <plat/control.h>
>  
> +#include <asm/div64.h>
> +
>  #include "prm.h"
>  #include "pm.h"
>  #include "prm-regbits-34xx.h"
> @@ -42,13 +44,14 @@
>   * disabled via sysfs. This also causes that any deeper omap sleep states are
>   * blocked. 
>   */
> -#define DEFAULT_TIMEOUT 0
> +#define DEFAULT_TIMEOUT (0LL * NSEC_PER_SEC)
>  
>  struct omap_uart_state {
>  	int num;
>  	int can_sleep;
> -	struct timer_list timer;
> -	u32 timeout;
> +	struct timespec expire_time;
> +	struct timespec garbage_time;
> +	u64 timeout;
>  
>  	void __iomem *wk_st;
>  	void __iomem *wk_en;
> @@ -243,6 +246,9 @@ 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 */
>  
> +static void omap_uart_smart_idle_enable(struct omap_uart_state *uart,
> +		int enable);
> +
>  static inline void omap_uart_enable_clocks(struct omap_uart_state *uart)
>  {
>  	if (uart->clocked)
> @@ -250,8 +256,13 @@ static inline void omap_uart_enable_clocks(struct omap_uart_state *uart)
>  
>  	clk_enable(uart->ick);
>  	clk_enable(uart->fck);
> +	omap_uart_smart_idle_enable(uart, 0);
>  	uart->clocked = 1;
>  	omap_uart_restore_context(uart);
> +
> +	/* Set up garbage timer to ignore RX during first 1ms */
> +	getrawmonotonic(&uart->garbage_time);
> +	timespec_add_ns(&uart->garbage_time, NSEC_PER_MSEC);
>  }
>  
>  #ifdef CONFIG_PM
> @@ -263,6 +274,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,12 +332,11 @@ 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);
> -	else
> -		del_timer(&uart->timer);
> +	if (uart->timeout) {
> +		getrawmonotonic(&uart->expire_time);
> +		timespec_add_ns(&uart->expire_time, uart->timeout);
> +	}
>  }
>  
>  static void omap_uart_allow_sleep(struct omap_uart_state *uart)
> @@ -338,25 +349,24 @@ 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);
> -}
> -
> -static void omap_uart_idle_timer(unsigned long data)
> -{
> -	struct omap_uart_state *uart = (struct omap_uart_state *)data;
> -
> -	omap_uart_allow_sleep(uart);
>  }
>  
>  void omap_uart_prepare_idle(int num)
>  {
>  	struct omap_uart_state *uart;
> +	struct timespec t;
>  
>  	list_for_each_entry(uart, &uart_list, node) {
> +		if (num == uart->num && !uart->can_sleep && uart->timeout) {
> +			getrawmonotonic(&t);
> +			if (timespec_compare(&t, &uart->expire_time) > 0)
> +				uart->can_sleep = 1;
> +		}
>  		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;
>  		}
>  	}
> @@ -381,6 +391,7 @@ void omap_uart_resume_idle(int num)
>  			/* Check for normal UART wakeup */
>  			if (__raw_readl(uart->wk_st) & uart->wk_mask)
>  				omap_uart_block_sleep(uart);
> +
>  			return;
>  		}
>  	}
> @@ -399,11 +410,18 @@ int omap_uart_can_sleep(void)
>  {
>  	struct omap_uart_state *uart;
>  	int can_sleep = 1;
> +	struct timespec t;
>  
>  	list_for_each_entry(uart, &uart_list, node) {
>  		if (!uart->clocked)
>  			continue;
>  
> +		if (!uart->can_sleep && uart->timeout) {
> +			getrawmonotonic(&t);
> +			if (timespec_compare(&t, &uart->expire_time) > 0)
> +				uart->can_sleep = 1;
> +		}
> +
>  		if (!uart->can_sleep) {
>  			can_sleep = 0;
>  			continue;
> @@ -428,10 +446,25 @@ 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;
> +	int ret = IRQ_NONE;
> +	struct timespec t;
>  
> -	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_time.tv_sec) {
> +			getrawmonotonic(&t);
> +			if (timespec_compare(&t, &uart->garbage_time) < 0) {
> +				serial_read_reg(uart->p, UART_RX);
> +				uart->garbage_time.tv_sec = 0;
> +				ret = IRQ_HANDLED;
> +			}
> +		}
> +	}
>  
> -	return IRQ_NONE;
> +	return ret;
>  }
>  
>  static void omap_uart_idle_init(struct omap_uart_state *uart)
> @@ -441,10 +474,12 @@ static void omap_uart_idle_init(struct omap_uart_state *uart)
>  
>  	uart->can_sleep = 0;
>  	uart->timeout = DEFAULT_TIMEOUT;
> -	setup_timer(&uart->timer, omap_uart_idle_timer,
> -		    (unsigned long) uart);
> -	if (uart->timeout)
> -		mod_timer(&uart->timer, jiffies + uart->timeout);
> +
> +	if (uart->timeout) {
> +		getrawmonotonic(&uart->expire_time);
> +		timespec_add_ns(&uart->expire_time, uart->timeout);
> +	}
> +
>  	omap_uart_smart_idle_enable(uart, 0);
>  
>  	if (cpu_is_omap34xx()) {
> @@ -527,8 +562,12 @@ static ssize_t sleep_timeout_show(struct device *dev,
>  					struct platform_device, dev);
>  	struct omap_uart_state *uart = container_of(pdev,
>  					struct omap_uart_state, pdev);
> +	u64 val;
> +
> +	val = uart->timeout;
>  
> -	return sprintf(buf, "%u\n", uart->timeout / HZ);
> +	do_div(val, NSEC_PER_SEC);
> +	return sprintf(buf, "%llu\n", val);
>  }
>  
>  static ssize_t sleep_timeout_store(struct device *dev,
> @@ -546,10 +585,11 @@ static ssize_t sleep_timeout_store(struct device *dev,
>  		return -EINVAL;
>  	}
>  
> -	uart->timeout = value * HZ;
> -	if (uart->timeout)
> -		mod_timer(&uart->timer, jiffies + uart->timeout);
> -	else
> +	uart->timeout = (u64)value * NSEC_PER_SEC;
> +	if (uart->timeout) {
> +		getrawmonotonic(&uart->expire_time);
> +		timespec_add_ns(&uart->expire_time, uart->timeout);
> +	} else
>  		/* A zero value means disable timeout feature */
>  		omap_uart_block_sleep(uart);
>  
> -- 
> 1.5.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tero.Kristo]

 

>-----Original Message-----
>From: ext Kevin Hilman [mailto:khilman@deeprootsystems.com] 
>Sent: 24 February, 2010 18:05
>To: Kristo Tero (Nokia-D/Tampere)
>Cc: linux-omap@vger.kernel.org
>Subject: Re: [PATCHv5] OMAP3: Serial: Improved sleep logic
>
>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 interrupts from being
>>   received from UART module
>> - Sleep prevent timer is changed to use timespec instead of 
>a jiffy timer
>>   as jiffy timers are not valid within idle loop (tick 
>scheduler is stopped)
>
>Could also probably use hrtimers for this.

Maybe, I didn't try this out though. It is possible that there are some issues as hrtimers use ktime() as far as I understand and it is not accessible during suspend. There is a separate issue with this patch and suspend though, I am currently working on a fix for that. Suspend skewes the timebase even more, and we get expire timers that point far into the future after suspend.

>
>That being said, I'm not sure what is the problem you're trying to
>solve with this change.  I don't see any timings that are timing events
>inside the idle loop.

Here is the call-chain that currently accesses jiffies count incorrectly:

cpu_idle:
   tick_nohz_stop_sched_tick(1); /* jiffy timer stops here */
   omap_sram_idle();
     omap_uart_prepare_idle();
     asm("wfi"); /* sleep here for N seconds */
     omap_uart_resume_idle(); /* This call uses incorrect jiffy timer now */
   tick_nohz_restart_sched_tick(); /* jiffy timer restarted here, and jiffy timer is refreshed also to correct value */

>
>> - Added RX ignore timer for ignoring the first character 
>received during
>>   first millisecond of wakeup, this prevents garbage 
>character to be receive
>>   in low sleep states
>>
>> Signed-off-by: Tero Kristo <tero.kristo@nokia.com>
>
>Something is still not quite right here.
>
>This doesn't work on my n900 here when testing this patch on top of
>the PM branch.  The default is now a default timeout of zero.  When I
>enable a 5 sec. timeout for UART2 on RX-51, when the timer expires, I
>no longer have response on the console.
>
>To test, I booted my n900 with init=/bin/sh to avoid all the setup
>done by /sbin/preinit.  Then I enabled a timeout for UART2 only, and
>then the console hangs.
>
>Here's my hunch as to what's happening:
>
>I think the problem is a deadlock in getrawmonotonic().  Nested calls
>here will deadlock due to the xtime_lock being held.

Looking at the seqlock code, I think a seqlock reader can "hang" only in a case where someone is constantly writing the seqlock. And, as we are inside interrupt, this should not be possible.

>When updading the timeout, sleep_timeout_store() does a
>getrawmonotonic() to update the expiry time.  While this happening,
>the UART interrupt could fire, causing an omap_uart_block_sleep()
>which would also getrawmonotonic() and deadlock in interrupt mode.

It does not really explain why it hangs after the 5 second period though, as the device has called getrawmonotonic several times by this already. I have not seen this kind of behavior in my testing, even while fiddling with the sleep_timeout_store().

Anyway, I'll attempt to re-run my test on the latest PM / master branches and see what happens.

>
>Kevin
>
>> ---
>>  arch/arm/mach-omap2/serial.c |   98 
>+++++++++++++++++++++++++++++------------
>>  1 files changed, 69 insertions(+), 29 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/serial.c 
>b/arch/arm/mach-omap2/serial.c
>> index 5f3035e..f49c465 100644
>> --- a/arch/arm/mach-omap2/serial.c
>> +++ b/arch/arm/mach-omap2/serial.c
>> @@ -29,6 +29,8 @@
>>  #include <plat/clock.h>
>>  #include <plat/control.h>
>>  
>> +#include <asm/div64.h>
>> +
>>  #include "prm.h"
>>  #include "pm.h"
>>  #include "prm-regbits-34xx.h"
>> @@ -42,13 +44,14 @@
>>   * disabled via sysfs. This also causes that any deeper 
>omap sleep states are
>>   * blocked. 
>>   */
>> -#define DEFAULT_TIMEOUT 0
>> +#define DEFAULT_TIMEOUT (0LL * NSEC_PER_SEC)
>>  
>>  struct omap_uart_state {
>>  	int num;
>>  	int can_sleep;
>> -	struct timer_list timer;
>> -	u32 timeout;
>> +	struct timespec expire_time;
>> +	struct timespec garbage_time;
>> +	u64 timeout;
>>  
>>  	void __iomem *wk_st;
>>  	void __iomem *wk_en;
>> @@ -243,6 +246,9 @@ 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 */
>>  
>> +static void omap_uart_smart_idle_enable(struct 
>omap_uart_state *uart,
>> +		int enable);
>> +
>>  static inline void omap_uart_enable_clocks(struct 
>omap_uart_state *uart)
>>  {
>>  	if (uart->clocked)
>> @@ -250,8 +256,13 @@ static inline void 
>omap_uart_enable_clocks(struct omap_uart_state *uart)
>>  
>>  	clk_enable(uart->ick);
>>  	clk_enable(uart->fck);
>> +	omap_uart_smart_idle_enable(uart, 0);
>>  	uart->clocked = 1;
>>  	omap_uart_restore_context(uart);
>> +
>> +	/* Set up garbage timer to ignore RX during first 1ms */
>> +	getrawmonotonic(&uart->garbage_time);
>> +	timespec_add_ns(&uart->garbage_time, NSEC_PER_MSEC);
>>  }
>>  
>>  #ifdef CONFIG_PM
>> @@ -263,6 +274,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,12 +332,11 @@ 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);
>> -	else
>> -		del_timer(&uart->timer);
>> +	if (uart->timeout) {
>> +		getrawmonotonic(&uart->expire_time);
>> +		timespec_add_ns(&uart->expire_time, uart->timeout);
>> +	}
>>  }
>>  
>>  static void omap_uart_allow_sleep(struct omap_uart_state *uart)
>> @@ -338,25 +349,24 @@ 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);
>> -}
>> -
>> -static void omap_uart_idle_timer(unsigned long data)
>> -{
>> -	struct omap_uart_state *uart = (struct omap_uart_state *)data;
>> -
>> -	omap_uart_allow_sleep(uart);
>>  }
>>  
>>  void omap_uart_prepare_idle(int num)
>>  {
>>  	struct omap_uart_state *uart;
>> +	struct timespec t;
>>  
>>  	list_for_each_entry(uart, &uart_list, node) {
>> +		if (num == uart->num && !uart->can_sleep && 
>uart->timeout) {
>> +			getrawmonotonic(&t);
>> +			if (timespec_compare(&t, 
>&uart->expire_time) > 0)
>> +				uart->can_sleep = 1;
>> +		}
>>  		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;
>>  		}
>>  	}
>> @@ -381,6 +391,7 @@ void omap_uart_resume_idle(int num)
>>  			/* Check for normal UART wakeup */
>>  			if (__raw_readl(uart->wk_st) & uart->wk_mask)
>>  				omap_uart_block_sleep(uart);
>> +
>>  			return;
>>  		}
>>  	}
>> @@ -399,11 +410,18 @@ int omap_uart_can_sleep(void)
>>  {
>>  	struct omap_uart_state *uart;
>>  	int can_sleep = 1;
>> +	struct timespec t;
>>  
>>  	list_for_each_entry(uart, &uart_list, node) {
>>  		if (!uart->clocked)
>>  			continue;
>>  
>> +		if (!uart->can_sleep && uart->timeout) {
>> +			getrawmonotonic(&t);
>> +			if (timespec_compare(&t, 
>&uart->expire_time) > 0)
>> +				uart->can_sleep = 1;
>> +		}
>> +
>>  		if (!uart->can_sleep) {
>>  			can_sleep = 0;
>>  			continue;
>> @@ -428,10 +446,25 @@ 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;
>> +	int ret = IRQ_NONE;
>> +	struct timespec t;
>>  
>> -	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_time.tv_sec) {
>> +			getrawmonotonic(&t);
>> +			if (timespec_compare(&t, 
>&uart->garbage_time) < 0) {
>> +				serial_read_reg(uart->p, UART_RX);
>> +				uart->garbage_time.tv_sec = 0;
>> +				ret = IRQ_HANDLED;
>> +			}
>> +		}
>> +	}
>>  
>> -	return IRQ_NONE;
>> +	return ret;
>>  }
>>  
>>  static void omap_uart_idle_init(struct omap_uart_state *uart)
>> @@ -441,10 +474,12 @@ static void omap_uart_idle_init(struct 
>omap_uart_state *uart)
>>  
>>  	uart->can_sleep = 0;
>>  	uart->timeout = DEFAULT_TIMEOUT;
>> -	setup_timer(&uart->timer, omap_uart_idle_timer,
>> -		    (unsigned long) uart);
>> -	if (uart->timeout)
>> -		mod_timer(&uart->timer, jiffies + uart->timeout);
>> +
>> +	if (uart->timeout) {
>> +		getrawmonotonic(&uart->expire_time);
>> +		timespec_add_ns(&uart->expire_time, uart->timeout);
>> +	}
>> +
>>  	omap_uart_smart_idle_enable(uart, 0);
>>  
>>  	if (cpu_is_omap34xx()) {
>> @@ -527,8 +562,12 @@ static ssize_t 
>sleep_timeout_show(struct device *dev,
>>  					struct platform_device, dev);
>>  	struct omap_uart_state *uart = container_of(pdev,
>>  					struct omap_uart_state, pdev);
>> +	u64 val;
>> +
>> +	val = uart->timeout;
>>  
>> -	return sprintf(buf, "%u\n", uart->timeout / HZ);
>> +	do_div(val, NSEC_PER_SEC);
>> +	return sprintf(buf, "%llu\n", val);
>>  }
>>  
>>  static ssize_t sleep_timeout_store(struct device *dev,
>> @@ -546,10 +585,11 @@ static ssize_t 
>sleep_timeout_store(struct device *dev,
>>  		return -EINVAL;
>>  	}
>>  
>> -	uart->timeout = value * HZ;
>> -	if (uart->timeout)
>> -		mod_timer(&uart->timer, jiffies + uart->timeout);
>> -	else
>> +	uart->timeout = (u64)value * NSEC_PER_SEC;
>> +	if (uart->timeout) {
>> +		getrawmonotonic(&uart->expire_time);
>> +		timespec_add_ns(&uart->expire_time, uart->timeout);
>> +	} else
>>  		/* A zero value means disable timeout feature */
>>  		omap_uart_block_sleep(uart);
>>  
>> -- 
>> 1.5.4.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe 
>linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Kevin Hilman]

<Tero.Kristo@nokia.com> writes:

[...]

>>Here's my hunch as to what's happening:
>>
>>I think the problem is a deadlock in getrawmonotonic().  Nested calls
>>here will deadlock due to the xtime_lock being held.
>
> Looking at the seqlock code, I think a seqlock reader can "hang" only in a case where someone is constantly writing the seqlock. And, as we are inside interrupt, this should not be possible.
>
>>When updading the timeout, sleep_timeout_store() does a
>>getrawmonotonic() to update the expiry time.  While this happening,
>>the UART interrupt could fire, causing an omap_uart_block_sleep()
>>which would also getrawmonotonic() and deadlock in interrupt mode.
>
> It does not really explain why it hangs after the 5 second period though, as the device has called getrawmonotonic several times by this already. I have not seen this kind of behavior in my testing, even while fiddling with the sleep_timeout_store().

minor clarification...

For me the hang is not after the 5 second timeout.  For me it happens
as soon as I write a non-zero value to
/sys/devices/.../serial8250.2/sleep_timeout.

Kevin

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

[Tero.Kristo]

 

>-----Original Message-----
>From: linux-omap-owner@vger.kernel.org 
>[mailto:linux-omap-owner@vger.kernel.org] On Behalf Of ext Kevin Hilman
>Sent: 25 February, 2010 17:21
>To: Kristo Tero (Nokia-D/Tampere)
>Cc: linux-omap@vger.kernel.org
>Subject: Re: [PATCHv5] OMAP3: Serial: Improved sleep logic
>
><Tero.Kristo@nokia.com> writes:
>
>[...]
>
>>>Here's my hunch as to what's happening:
>>>
>>>I think the problem is a deadlock in getrawmonotonic().  Nested calls
>>>here will deadlock due to the xtime_lock being held.
>>
>> Looking at the seqlock code, I think a seqlock reader can 
>"hang" only in a case where someone is constantly writing the 
>seqlock. And, as we are inside interrupt, this should not be possible.
>>
>>>When updading the timeout, sleep_timeout_store() does a
>>>getrawmonotonic() to update the expiry time.  While this happening,
>>>the UART interrupt could fire, causing an omap_uart_block_sleep()
>>>which would also getrawmonotonic() and deadlock in interrupt mode.
>>
>> It does not really explain why it hangs after the 5 second 
>period though, as the device has called getrawmonotonic 
>several times by this already. I have not seen this kind of 
>behavior in my testing, even while fiddling with the 
>sleep_timeout_store().
>
>minor clarification...
>
>For me the hang is not after the 5 second timeout.  For me it happens
>as soon as I write a non-zero value to
>/sys/devices/.../serial8250.2/sleep_timeout.

It looks somewhat weird... I did some debugging on this today and I am seeing the same hang here, and it actually seems to be caused by the getrawmonotonic() somehow, as the system did not hang anymore after I removed the calls. Anyway, I figured another way to solve the issues I am seeing. I reverted back to using jiffy timers, but added a workqueue for wakeups; now wakeup timers are kicked into action after the timebase is restored. I'll post the new patch right away.

There is still one issue with the UART though; some TX is still lost because PM code does not currently prevent CORE off while PER is active, basically resetting the PER domain when CORE goes off. This would be fixed by the cpuidle set I posted a bit back.

Another note, I am seeing really strange behavior for my n900 with current linux-omap tree. PM branch boots up nicely, but it does not go into dynamic idle until I force it to suspend once.... Also, master branch crashes mid-boot to an ubifs caused DMA failure.

-Tero