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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Tero Kristo <tero.kristo@xxxxxxxxx> writes:

> From: Tero Kristo <tero.kristo@xxxxxxxxx>
>
> 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@xxxxxxxxx>

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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux