Re: [RFC] Allow disabling wakeup for serial ports, including during off mode.

[Kevin Hilman <khilman@deeprootsystems.com>]

Russ Dill <russ.dill@gmail.com> writes:

> This patch causes the OMAP uarts to honor the sysfs power/wakeup file for
> IOPADs. Before the OMAP was always woken up from off mode on a rs232 signal
> change.
>
> This patch also creates a different platform device for each serial
> port so that the wakeup properties can be control per port.
>
> The patch is not in a complete state, but for my testing it was necessary
> to disable rs232 wakeup as allowing the signals to float in off mode by
> powering off the level converters was causing a wakeup event.

Hi Russ,

Thanks for this feature.  This is a nice improvement for UART wakeups.
I have a few comments below...

Again, can you update the description here to show how you're using
the various sysfs files for your testing.  I want to make it easy
for others to be able to test and use these sysfs features.

> ---
>  arch/arm/mach-omap2/serial.c |  165 ++++++++++++++++++++++++++++++++----------
>  1 files changed, 126 insertions(+), 39 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 0762165..95b047a 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -49,6 +49,7 @@ struct omap_uart_state {
>  
>  	struct plat_serial8250_port *p;
>  	struct list_head node;
> +	struct platform_device pdev;
>  
>  #if defined(CONFIG_ARCH_OMAP3) && defined(CONFIG_PM)
>  	int context_valid;
> @@ -63,10 +64,9 @@ struct omap_uart_state {
>  #endif
>  };
>  
> -static struct omap_uart_state omap_uart[OMAP_MAX_NR_PORTS];
>  static LIST_HEAD(uart_list);
>  
> -static struct plat_serial8250_port serial_platform_data[] = {
> +static struct plat_serial8250_port serial_platform_data0[] = {
>  	{
>  		.membase	= IO_ADDRESS(OMAP_UART1_BASE),
>  		.mapbase	= OMAP_UART1_BASE,
> @@ -76,6 +76,12 @@ static struct plat_serial8250_port serial_platform_data[] = {
>  		.regshift	= 2,
>  		.uartclk	= OMAP24XX_BASE_BAUD * 16,
>  	}, {
> +		.flags		= 0
> +	}
> +};
> +
> +static struct plat_serial8250_port serial_platform_data1[] = {
> +	{
>  		.membase	= IO_ADDRESS(OMAP_UART2_BASE),
>  		.mapbase	= OMAP_UART2_BASE,
>  		.irq		= 73,
> @@ -84,6 +90,12 @@ static struct plat_serial8250_port serial_platform_data[] = {
>  		.regshift	= 2,
>  		.uartclk	= OMAP24XX_BASE_BAUD * 16,
>  	}, {
> +		.flags		= 0
> +	}
> +};
> +
> +static struct plat_serial8250_port serial_platform_data2[] = {
> +	{
>  		.membase	= IO_ADDRESS(OMAP_UART3_BASE),
>  		.mapbase	= OMAP_UART3_BASE,
>  		.irq		= 74,

Can you break the platform_data splitup out into a separate patch?

> @@ -197,6 +209,40 @@ 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_ARCH_OMAP3 */
>  
> +static void omap_uart_enable_wakeup(struct omap_uart_state *uart)
> +{
> +	/* Set wake-enable bit */
> +	if (uart->wk_en && uart->wk_mask) {
> +		u32 v = __raw_readl(uart->wk_en);
> +		v |= uart->wk_mask;
> +		__raw_writel(v, uart->wk_en);
> +	}
> +
> +	/* Ensure IOPAD wake-enables are set */
> +	if (cpu_is_omap34xx() && uart->padconf) {
> +		u16 v = omap_ctrl_readw(uart->padconf);
> +		v |= OMAP3_PADCONF_WAKEUPENABLE0;
> +		omap_ctrl_writew(v, uart->padconf);
> +	}
> +}
> +
> +static void omap_uart_disable_wakeup(struct omap_uart_state *uart)
> +{
> +	/* Clear wake-enable bit */
> +	if (uart->wk_en && uart->wk_mask) {
> +		u32 v = __raw_readl(uart->wk_en);
> +		v &= ~uart->wk_mask;
> +		__raw_writel(v, uart->wk_en);
> +	}
> +
> +	/* Ensure IOPAD wake-enables are cleared */
> +	if (cpu_is_omap34xx() && uart->padconf) {
> +		u16 v = omap_ctrl_readw(uart->padconf);
> +		v &= ~OMAP3_PADCONF_WAKEUPENABLE0;
> +		omap_ctrl_writew(v, uart->padconf);
> +	}
> +}
> +
>  static void omap_uart_smart_idle_enable(struct omap_uart_state *uart,
>  					  int enable)
>  {
> @@ -220,6 +266,11 @@ static inline void omap_uart_restore(struct omap_uart_state *uart)
>  
>  static inline void omap_uart_disable_clocks(struct omap_uart_state *uart)
>  {
> +	if (device_may_wakeup(&uart->pdev.dev))
> +		omap_uart_enable_wakeup(uart);
> +	else
> +		omap_uart_disable_wakeup(uart);
> +

When UART clocks are disabled, you always either disable or enable the
wakeup feature...

>  	if (!uart->clocked)
>  		return;
>  
> @@ -290,6 +341,7 @@ void omap_uart_resume_idle(int num)
>  			if (__raw_readl(uart->wk_st) & uart->wk_mask)
>  				omap_uart_block_sleep(uart);
>  
> +			omap_uart_enable_wakeup(uart);

so is this 'enable' needed? ...

>  			return;
>  		}
>  	}
> @@ -300,6 +352,7 @@ void omap_uart_prepare_suspend(void)
>  	struct omap_uart_state *uart;
>  
>  	list_for_each_entry(uart, &uart_list, node) {
> +		omap_uart_enable_wakeup(uart);

or this one?

>  		omap_uart_allow_sleep(uart);
>  	}
>  }
> @@ -343,16 +396,13 @@ static irqreturn_t omap_uart_interrupt(int irq, void *dev_id)
>  	return IRQ_NONE;
>  }
>  
> -static u32 sleep_timeout = DEFAULT_TIMEOUT;
> -
>  static void omap_uart_idle_init(struct omap_uart_state *uart)
>  {
> -	u32 v;
>  	struct plat_serial8250_port *p = uart->p;
>  	int ret;
>  
>  	uart->can_sleep = 0;
> -	uart->timeout = sleep_timeout;
> +	uart->timeout = DEFAULT_TIMEOUT;
>  	setup_timer(&uart->timer, omap_uart_idle_timer,
>  		    (unsigned long) uart);
>  	mod_timer(&uart->timer, jiffies + uart->timeout);
> @@ -410,22 +460,6 @@ static void omap_uart_idle_init(struct omap_uart_state *uart)
>  		uart->padconf = 0;
>  	}
>  
> -	/* Set wake-enable bit */
> -	if (uart->wk_en && uart->wk_mask) {
> -		v = __raw_readl(uart->wk_en);
> -		v |= uart->wk_mask;
> -		__raw_writel(v, uart->wk_en);
> -	}
> -
> -	/* Ensure IOPAD wake-enables are set */
> -	if (cpu_is_omap34xx() && uart->padconf) {
> -		u16 v;
> -
> -		v = omap_ctrl_readw(uart->padconf);
> -		v |= OMAP3_PADCONF_WAKEUPENABLE0;
> -		omap_ctrl_writew(v, uart->padconf);
> -	}
> -
>  	p->flags |= UPF_SHARE_IRQ;
>  	ret = request_irq(p->irq, omap_uart_interrupt, IRQF_SHARED,
>  			  "serial idle", (void *)uart);
> @@ -450,23 +484,30 @@ static ssize_t sleep_timeout_show(struct kobject *kobj,
>  				  struct kobj_attribute *attr,
>  				  char *buf)
>  {
> -	return sprintf(buf, "%u\n", sleep_timeout / HZ);
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct platform_device *pdev = container_of(dev,
> +					struct platform_device, dev);
> +	struct omap_uart_state *uart = container_of(pdev,
> +					struct omap_uart_state, pdev);
> +	return sprintf(buf, "%u\n", uart->timeout / HZ);
>  }
>  
>  static ssize_t sleep_timeout_store(struct kobject *kobj,
>  				   struct kobj_attribute *attr,
>  				   const char *buf, size_t n)
>  {
> -	struct omap_uart_state *uart;
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct platform_device *pdev = container_of(dev,
> +					struct platform_device, dev);
> +	struct omap_uart_state *uart = container_of(pdev,
> +					struct omap_uart_state, pdev);
>  	unsigned int value;
>  
>  	if (sscanf(buf, "%u", &value) != 1) {
>  		printk(KERN_ERR "sleep_timeout_store: Invalid value\n");
>  		return -EINVAL;
>  	}
> -	sleep_timeout = value * HZ;
> -	list_for_each_entry(uart, &uart_list, node)
> -		uart->timeout = sleep_timeout;
> +	uart->timeout = value * HZ;
>  	return n;
>  }
>  
> @@ -477,6 +518,34 @@ static struct kobj_attribute sleep_timeout_attr =
>  static inline void omap_uart_idle_init(struct omap_uart_state *uart) {}
>  #endif /* CONFIG_PM */
>  
> +static struct omap_uart_state omap_uart[OMAP_MAX_NR_PORTS] = {
> +	{
> +		.pdev = {
> +			.name			= "serial8250",
> +			.id			= PLAT8250_DEV_PLATFORM,
> +			.dev			= {
> +				.platform_data	= serial_platform_data0,
> +			},
> +		},
> +	}, {
> +		.pdev = {
> +			.name			= "serial8250",
> +			.id			= PLAT8250_DEV_PLATFORM1,
> +			.dev			= {
> +				.platform_data	= serial_platform_data1,
> +			},
> +		},
> +	}, {
> +		.pdev = {
> +			.name			= "serial8250",
> +			.id			= PLAT8250_DEV_PLATFORM2,
> +			.dev			= {
> +				.platform_data	= serial_platform_data2,
> +			},
> +		},
> +	},
> +};
> +
>  void __init omap_serial_init(void)
>  {
>  	int i;
> @@ -495,8 +564,8 @@ void __init omap_serial_init(void)
>  		return;
>  
>  	for (i = 0; i < OMAP_MAX_NR_PORTS; i++) {
> -		struct plat_serial8250_port *p = serial_platform_data + i;
>  		struct omap_uart_state *uart = &omap_uart[i];
> +		struct plat_serial8250_port *p = uart->pdev.dev.platform_data;
>  
>  		if (!(info->enabled_uarts & (1 << i))) {
>  			p->membase = NULL;
> @@ -532,25 +601,43 @@ void __init omap_serial_init(void)
>  	}
>  }
>  
> -static struct platform_device serial_device = {
> -	.name			= "serial8250",
> -	.id			= PLAT8250_DEV_PLATFORM,
> -	.dev			= {
> -		.platform_data	= serial_platform_data,
> -	},
> -};
> -
>  static int __init omap_init(void)
>  {
>  	int ret;
> +	int i;
>  
> -	ret = platform_device_register(&serial_device);
> +	for (i = 0; i < ARRAY_SIZE(omap_uart); i++) {
> +		ret = platform_device_register(&omap_uart[i].pdev);
> +		if (ret < 0)
> +			goto device_err;
> +		if ((cpu_is_omap34xx() && omap_uart[i].padconf) ||
> +		    (omap_uart[i].wk_en && omap_uart[i].wk_mask)) {
> +			printk(KERN_ERR "%s: Setting init_wakeup\n", __func__);

This looks like a debug printk that can be dropped for the final version.

> +			device_init_wakeup(&omap_uart[i].pdev.dev, true);
> +		}
> +	}
>  
>  #ifdef CONFIG_PM
> -	if (!ret)
> -		ret = sysfs_create_file(&serial_device.dev.kobj,
> +	for (i = 0; i < ARRAY_SIZE(omap_uart); i++) {
> +		ret = sysfs_create_file(&omap_uart[i].pdev.dev.kobj,
>  					&sleep_timeout_attr.attr);
> +		if (ret < 0)
> +			goto sysfs_err;
> +	}
> +#endif
> +	return ret;
> +
> +#ifdef CONFIG_PM
> +sysfs_err:
> +	for (i--; i >= 0; i--)
> +		sysfs_remove_file(&omap_uart[i].pdev.dev.kobj,
> +				&sleep_timeout_attr.attr);
> +	i = ARRAY_SIZE(omap_uart);
>  #endif
> +
> +device_err:
> +	for (i--; i >= 0; i--)
> +		platform_device_unregister(&omap_uart[i].pdev);
>  	return ret;
>  }
>  arch_initcall(omap_init);
> -- 
> 1.6.0.4
>
> --
> 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