Re: Suspend broken on 3.3?

Re: Suspend broken on 3.3?

[Paul Walmsley]

cc linux-serial 

Hi

On Mon, 9 Apr 2012, Kevin Hilman wrote:

> Presumably, if a user disables UART wakeups, it means either 1) that
> UART is not used

#1 seems easy enough to handle without the use of power/wakeup.  If there 
are no users of the TTY, then the driver can simply not configure hardware 
wakeups.

> or 2) performance is not critical.

Can you think of a use-case for #2?

> IMO, we simply need to ensure that the defaults are correct.  When UARTs
> are initialized/enabled wakeups should be enabled by default.  The user
> can then override this if desired, and will obviously see a performance
> impact.  But that's what happens with wakeups disabled.

I don't understand why a user would ever want to disable dynamic wakeups 
on an in-use serial port via the sysfs power/wakeup file.  (Disabling 
wakeups from suspend is a different matter, of course.)  The OMAP UART 
driver needs hardware wakeups to function for FIFO drain wakeups, etc.  
So to me it really doesn't make sense to disable those types of wakeup 
events, ever.  But maybe you know of some use-case that I don't?

If the user just wants a transmit-only serial port, they could use the 
termios CREAD flag as Neil mentioned a few months ago, and the driver 
could disable wakeups on incoming RXD (modulo any active flow control of 
course).


- Paul

Re: Suspend broken on 3.3?

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

[...]

> I don't understand why a user would ever want to disable dynamic wakeups 
> on an in-use serial port via the sysfs power/wakeup file.  (Disabling 
> wakeups from suspend is a different matter, of course.)  The OMAP UART 
> driver needs hardware wakeups to function for FIFO drain wakeups, etc.  
> So to me it really doesn't make sense to disable those types of wakeup 
> events, ever.  But maybe you know of some use-case that I don't?

No, I don't have a use-case in mind.

The more I try to remember why we added support to use the sysfs wakeup
attribute to manage idle, the more I think we can probably drop it now.
IIRC, it was added because on most boards we used to blindly initialize
all the UARTs, including default wakeup settings (we still do this on
many boards.)  

However, now that we have a real PM-aware driver for OMAP UARTs, this
should all be handled from the driver itself, so the sysfs wakeup
attribute should go back to only managing wakeups from suspend and
wakeups during idle should always be on for in-use UARTs.

Kevin

Re: Suspend broken on 3.3?

[Raja, Govindraj]

Hi Kevin,

On Mon, Apr 9, 2012 at 10:40 PM, Kevin Hilman <khilman@ti.com> wrote:
> Paul Walmsley <paul@pwsan.com> writes:
>
> [...]
>
>> I don't understand why a user would ever want to disable dynamic wakeups
>> on an in-use serial port via the sysfs power/wakeup file.  (Disabling
>> wakeups from suspend is a different matter, of course.)  The OMAP UART
>> driver needs hardware wakeups to function for FIFO drain wakeups, etc.
>> So to me it really doesn't make sense to disable those types of wakeup
>> events, ever.  But maybe you know of some use-case that I don't?
>
> No, I don't have a use-case in mind.
>
> The more I try to remember why we added support to use the sysfs wakeup
> attribute to manage idle, the more I think we can probably drop it now.
> IIRC, it was added because on most boards we used to blindly initialize
> all the UARTs, including default wakeup settings (we still do this on
> many boards.)
>
> However, now that we have a real PM-aware driver for OMAP UARTs, this
> should all be handled from the driver itself, so the sysfs wakeup
> attribute should go back to only managing wakeups from suspend and
> wakeups during idle should always be on for in-use UARTs.

Just to summarize on how the behavior should be IIUC if user disables uart
wakeup from sysfs and does system wide suspend it should _not_ wakeup
from uart.

And if the system is woken up from suspend due to keypad press and
uart resumes we have keep module level wakeup enabled from here.

We might need some minor changes in omap-serial to have this behavior
which I plan to do once we are aligned on this.

--
Thanks,
Govindraj.R
--
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: Suspend broken on 3.3?

[Kevin Hilman]

"Raja, Govindraj" <govindraj.raja@ti.com> writes:

> Hi Kevin,
>
> On Mon, Apr 9, 2012 at 10:40 PM, Kevin Hilman <khilman@ti.com> wrote:
>> Paul Walmsley <paul@pwsan.com> writes:
>>
>> [...]
>>
>>> I don't understand why a user would ever want to disable dynamic wakeups
>>> on an in-use serial port via the sysfs power/wakeup file.  (Disabling
>>> wakeups from suspend is a different matter, of course.)  The OMAP UART
>>> driver needs hardware wakeups to function for FIFO drain wakeups, etc.
>>> So to me it really doesn't make sense to disable those types of wakeup
>>> events, ever.  But maybe you know of some use-case that I don't?
>>
>> No, I don't have a use-case in mind.
>>
>> The more I try to remember why we added support to use the sysfs wakeup
>> attribute to manage idle, the more I think we can probably drop it now.
>> IIRC, it was added because on most boards we used to blindly initialize
>> all the UARTs, including default wakeup settings (we still do this on
>> many boards.)
>>
>> However, now that we have a real PM-aware driver for OMAP UARTs, this
>> should all be handled from the driver itself, so the sysfs wakeup
>> attribute should go back to only managing wakeups from suspend and
>> wakeups during idle should always be on for in-use UARTs.
>
> Just to summarize on how the behavior should be IIUC if user disables uart
> wakeup from sysfs and does system wide suspend it should _not_ wakeup
> from uart.

Correct.

> And if the system is woken up from suspend due to keypad press and
> uart resumes we have keep module level wakeup enabled from here.

Keypad press, or any other wakeup source, yes.

Basically, UART wakeups (module and IO) should be enabled all the time,
*except* when suspending and wakeups were disabled by sysfs control.

> We might need some minor changes in omap-serial to have this behavior
> which I plan to do once we are aligned on this.

Sure, this changes previous behavior based on assumptions that are no
longer true (namely, UARTs only disabled in idle path), so I would
expect some changes.

Kevin
--
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: Suspend broken on 3.3?

[Raja, Govindraj]

On Tue, Apr 10, 2012 at 11:33 PM, Kevin Hilman <khilman@ti.com> wrote:
> "Raja, Govindraj" <govindraj.raja@ti.com> writes:
>
>> Hi Kevin,
>>
>> On Mon, Apr 9, 2012 at 10:40 PM, Kevin Hilman <khilman@ti.com> wrote:
>>> Paul Walmsley <paul@pwsan.com> writes:

[...]

>>
>> Just to summarize on how the behavior should be IIUC if user disables uart
>> wakeup from sysfs and does system wide suspend it should _not_ wakeup
>> from uart.
>
> Correct.
>
>> And if the system is woken up from suspend due to keypad press and
>> uart resumes we have keep module level wakeup enabled from here.
>
> Keypad press, or any other wakeup source, yes.
>
> Basically, UART wakeups (module and IO) should be enabled all the time,
> *except* when suspending and wakeups were disabled by sysfs control.
>

Here is the patch [1] to do the same.

Tested on beagle-XM  with retention and off mode in suspend path and
idle path by disabling/enabling the uart wakeups from sysfs for the console.

--
Thanks,
Govindraj.R

[1]:

>From 4e2502015e8b69d3a5047ae9f92820e4833e6d74 Mon Sep 17 00:00:00 2001
From: "Govindraj.R" <govindraj.raja@ti.com>
Date: Tue, 27 Mar 2012 18:55:00 +0530
Subject: [PATCH] OMAP2+: UART: Correct the module level wakeup enable/disable
 mechanism

The commit (62f3ec5  ARM: OMAP2+: UART: Add wakeup mechanism for omap-uarts)
removed module level wakeup enable/disable mechanism and retained only
the pad wakeup handling.

On 24xx/34xx/36xx Module level wakeup events are enabled/disabled using
PM_WKEN1_CORE/PM_WKEN_PER regs. The module level wakeups are enabled by
default from bootloader, however the wakeups can be enabled/disabled
using sysfs entry
echo disabled > /sys/devices/platform/omap/omap_uart.X/power/wakeup
[X=0,1,2,3]

Since module level wakeups were left enabled from bootup and when
wakeups were disabled from sysfs uart module level wakeups were
still happening as they were not getting disabled.

The wakeup can be left enabled by default and should be disabled only
when disabled from sysfs and thus prevent system from uart wakeup in suspend
path. However in idle path the wakeup can be enabled and thus uart can wakeup
after gating of uart functional clocks.

Thanks to Kevin Hilman <khilman@ti.com> for suggesting this.
Discussion References:
http://www.spinics.net/lists/linux-omap/msg67764.html
http://www.spinics.net/lists/linux-omap/msg67838.html

Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
 arch/arm/mach-omap2/serial.c     |   88 +++++++++++++++++++++++++++++++++++++-
 drivers/tty/serial/omap-serial.c |   30 +++++--------
 2 files changed, 97 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 0cdd359..9312d6b 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -41,6 +41,7 @@
 #include "prm-regbits-34xx.h"
 #include "control.h"
 #include "mux.h"
+#include "iomap.h"

 /*
  * NOTE: By default the serial auto_suspend timeout is disabled as it causes
@@ -55,6 +56,10 @@
 struct omap_uart_state {
 	int num;

+	void __iomem *wk_st;
+	void __iomem *wk_en;
+	u32 wk_mask;
+
 	struct list_head node;
 	struct omap_hwmod *oh;
 };
@@ -80,17 +85,46 @@ static struct omap_uart_port_info
omap_serial_default_info[] __initdata = {
 };

 #ifdef CONFIG_PM
+
+static void omap_uart_disable_module_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);
+	}
+}
+
+static void omap_uart_enable_module_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);
+	}
+}
+
 static void omap_uart_enable_wakeup(struct platform_device *pdev, bool enable)
 {
 	struct omap_device *od = to_omap_device(pdev);
+	struct omap_uart_state *uart;

 	if (!od)
 		return;

-	if (enable)
+	list_for_each_entry(uart, &uart_list, node)
+		if (pdev->id == uart->num)
+			break;
+
+	if (enable) {
+		omap_uart_enable_module_wakeup(uart);
 		omap_hwmod_enable_wakeup(od->hwmods[0]);
-	else
+	} else {
+		omap_uart_disable_module_wakeup(uart);
 		omap_hwmod_disable_wakeup(od->hwmods[0]);
+	}
 }

 /*
@@ -112,7 +146,56 @@ static void omap_uart_set_smartidle(struct
platform_device *pdev)
 	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_SMART);
 }

+static void omap_uart_idle_init(struct omap_uart_state *uart)
+{
+	if (cpu_is_omap34xx() && !cpu_is_ti816x()) {
+		u32 mod = (uart->num == 2) ? OMAP3430_PER_MOD : CORE_MOD;
+
+		uart->wk_en = OMAP34XX_PRM_REGADDR(mod, PM_WKEN1);
+		uart->wk_st = OMAP34XX_PRM_REGADDR(mod, PM_WKST1);
+		switch (uart->num) {
+		case 0:
+			uart->wk_mask = OMAP3430_ST_UART1_MASK;
+			break;
+		case 1:
+			uart->wk_mask = OMAP3430_ST_UART2_MASK;
+			break;
+		case 2:
+			uart->wk_mask = OMAP3430_ST_UART3_MASK;
+			break;
+		case 3:
+			uart->wk_mask = OMAP3630_ST_UART4_MASK;
+			break;
+		}
+	} else if (cpu_is_omap24xx()) {
+
+		if (cpu_is_omap2430()) {
+			uart->wk_en = OMAP2430_PRM_REGADDR(CORE_MOD, PM_WKEN1);
+			uart->wk_st = OMAP2430_PRM_REGADDR(CORE_MOD, PM_WKST1);
+		} else if (cpu_is_omap2420()) {
+			uart->wk_en = OMAP2420_PRM_REGADDR(CORE_MOD, PM_WKEN1);
+			uart->wk_st = OMAP2420_PRM_REGADDR(CORE_MOD, PM_WKST1);
+		}
+		switch (uart->num) {
+		case 0:
+			uart->wk_mask = OMAP24XX_ST_UART1_MASK;
+			break;
+		case 1:
+			uart->wk_mask = OMAP24XX_ST_UART2_MASK;
+			break;
+		case 2:
+			uart->wk_mask = OMAP24XX_ST_UART3_MASK;
+			break;
+		}
+	} else {
+		uart->wk_en = 0;
+		uart->wk_st = 0;
+		uart->wk_mask = 0;
+	}
+}
+
 #else
+static void omap_uart_idle_init(struct omap_uart_state *uart) {}
 static void omap_uart_enable_wakeup(struct platform_device *pdev, bool enable)
 {}
 static void omap_uart_set_noidle(struct platform_device *pdev) {}
@@ -343,6 +426,7 @@ void __init omap_serial_init_port(struct
omap_board_data *bdata,
 	oh = uart->oh;
 	name = DRIVER_NAME;

+	omap_uart_idle_init(uart);
 	omap_up.dma_enabled = info->dma_enabled;
 	omap_up.uartclk = OMAP24XX_BASE_BAUD * 16;
 	omap_up.flags = UPF_BOOT_AUTOCONF;
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 0121486..3dec1cf 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -930,13 +930,6 @@ serial_omap_pm(struct uart_port *port, unsigned int state,
 	serial_out(up, UART_EFR, efr);
 	serial_out(up, UART_LCR, 0);

-	if (!device_may_wakeup(&up->pdev->dev)) {
-		if (!state)
-			pm_runtime_forbid(&up->pdev->dev);
-		else
-			pm_runtime_allow(&up->pdev->dev);
-	}
-
 	pm_runtime_put(&up->pdev->dev);
 }

@@ -1184,10 +1177,16 @@ static struct uart_driver serial_omap_reg = {
 static int serial_omap_suspend(struct device *dev)
 {
 	struct uart_omap_port *up = dev_get_drvdata(dev);
+	struct omap_uart_port_info *pdata = dev->platform_data;

 	if (up) {
 		uart_suspend_port(&serial_omap_reg, &up->port);
 		flush_work_sync(&up->qos_work);
+
+		if (!device_may_wakeup(dev)) {
+			pdata->enable_wakeup(up->pdev, false);
+			up->wakeups_enabled = false;
+		}
 	}

 	return 0;
@@ -1585,18 +1584,6 @@ static int serial_omap_runtime_suspend(struct
device *dev)
 	if (pdata->get_context_loss_count)
 		up->context_loss_cnt = pdata->get_context_loss_count(dev);

-	if (device_may_wakeup(dev)) {
-		if (!up->wakeups_enabled) {
-			pdata->enable_wakeup(up->pdev, true);
-			up->wakeups_enabled = true;
-		}
-	} else {
-		if (up->wakeups_enabled) {
-			pdata->enable_wakeup(up->pdev, false);
-			up->wakeups_enabled = false;
-		}
-	}
-
 	/* Errata i291 */
 	if (up->use_dma && pdata->set_forceidle &&
 			(up->errata & UART_ERRATA_i291_DMA_FORCEIDLE))
@@ -1621,6 +1608,11 @@ static int serial_omap_runtime_resume(struct device *dev)
 				serial_omap_restore_context(up);
 		}

+		if (!up->wakeups_enabled) {
+			pdata->enable_wakeup(up->pdev, true);
+			up->wakeups_enabled = true;
+		}
+
 		/* Errata i291 */
 		if (up->use_dma && pdata->set_noidle &&
 				(up->errata & UART_ERRATA_i291_DMA_FORCEIDLE))
-- 
1.7.9

Re: Suspend broken on 3.3?

[Paul Walmsley]

Hi

a few brief comments based on a quick scan:

On Wed, 11 Apr 2012, Raja, Govindraj wrote:

> Here is the patch [1] to do the same.

- I don't see where it affects I/O wakeups for the UART.  If I/O wakeups 
are still set on the UART pads, won't that still wake the chip up from 
suspend, even if module-level wakeups are disabled?

- The UART driver and integration code should not be reading from or 
writing to registers outside the UART IP block.  PRM register reads and 
writes belong in the PRM code, which should then export a higher-level 
interface to the calling code.  This is because, aside from making the 
code easier to read and debug, we're trying to move the PRM and CM code to 
drivers/.

- The code to change the PM_WKEN* and test the PM_WKST* bits should 
probably be called from omap_hwmod_{enable,disable}_wakeup(), not the UART 
code directly.  The UART code shouldn't need to care about the hardware 
details; it should just ask the integration layer to enable or disable 
module-level wakeups.

As you work on these changes, please split them up into several different 
topic series - one for the PRM changes, one for hwmod code/data changes, 
and one for the UART driver/integration changes.  Just note the 
dependencies in the series description E-mails.  That way, we can avoid 
merge conflicts.


- Paul

Re: Suspend broken on 3.3?

[Raja, Govindraj]

On Thu, Apr 12, 2012 at 1:04 AM, Paul Walmsley <paul@pwsan.com> wrote:
> Hi
>
> a few brief comments based on a quick scan:
>
> On Wed, 11 Apr 2012, Raja, Govindraj wrote:
>
>> Here is the patch [1] to do the same.
>
> - I don't see where it affects I/O wakeups for the UART.  If I/O wakeups
> are still set on the UART pads, won't that still wake the chip up from
> suspend, even if module-level wakeups are disabled?

pdata->enable_wakeup => omap_uart_enable_wakeup
was disabling both module level and pad wakeup.

omap_uart_enable_wakeup => has enabling/disabling both
module level and pad wakeup together.

>
> - The UART driver and integration code should not be reading from or
> writing to registers outside the UART IP block.  PRM register reads and
> writes belong in the PRM code, which should then export a higher-level
> interface to the calling code.  This is because, aside from making the
> code easier to read and debug, we're trying to move the PRM and CM code to
> drivers/.

okay.

>
> - The code to change the PM_WKEN* and test the PM_WKST* bits should
> probably be called from omap_hwmod_{enable,disable}_wakeup(), not the UART
> code directly.  The UART code shouldn't need to care about the hardware
> details; it should just ask the integration layer to enable or disable
> module-level wakeups.

To implement this I plan to extend the omap_hwmod_omap2_prcm structure
like this:

(unaligned tmp code snippet)

diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h
b/arch/arm/plat-omap/include/plat/omap_hwmod.h
index 8070145..5c7711b 100644
--- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
+++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
@@ -343,6 +343,8 @@ struct omap_hwmod_class_sysconfig {
  * @idlest_reg_id: IDLEST register ID (e.g., 3 for CM_IDLEST3)
  * @idlest_idle_bit: register bit shift for CM_IDLEST slave idle bit
  * @idlest_stdby_bit: register bit shift for CM_IDLEST master standby bit
+ * @module_wakeup_offs: PRCM register offset for PM_WKEN
+ * @module_wakeup_bit: regiter bit mask for PM_WKEN
  *
  * @prcm_reg_id and @module_bit are specific to the AUTOIDLE, WKST,
  * WKEN, GRPSEL registers.  In an ideal world, no extra information
@@ -357,6 +359,8 @@ struct omap_hwmod_omap2_prcm {
        u8 idlest_reg_id;
        u8 idlest_idle_bit;
        u8 idlest_stdby_bit;
+       s16 module_wakeup_offs;
+       u32 module_wakeup_bit;
 };

>
> As you work on these changes, please split them up into several different
> topic series - one for the PRM changes, one for hwmod code/data changes,
> and one for the UART driver/integration changes.  Just note the
> dependencies in the series description E-mails.  That way, we can avoid
> merge conflicts.
>

Yes fine. Since most changes will be on /mach-omap2/omap_hwmod*.c
Do you prefer the patches to be on any particular tree/branch where hwmod fixes
are already queued.

--
Thanks,
Govindraj.R
--
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