Suspend broken on 3.3?

Suspend broken on 3.3?

[Joe Woodward]

Is system suspend broken on stock 3.3?

I have a working stock 3.2 (with patches to fix runtime_pm for DSS2), and system suspend works just fine!

This is running on a variety of GUMSTIX boards (both OMAP3530 and AM3703-based).

I've just updated to stock 3.3 and suspend returns immediately when entered:

# echo mem > /sys/power/state
[   72.391693] PM: Syncing filesystems ... done.
[   72.397003] Freezing user space processes ... (elapsed 0.02 seconds) done.
[   72.425201] Freezing remaining freezable tasks ... (elapsed 0.02 seconds) done.
[   72.456451] Suspending console(s) (use no_console_suspend to debug)
[   72.581695] PM: suspend of devices complete after 115.977 msecs
[   72.584594] PM: late suspend of devices complete after 2.868 msecs
[   72.612060] Successfully put all powerdomains to target state
[   72.613983] PM: early resume of devices complete after 1.770 msecs
[   72.941894] PM: resume of devices complete after 326.630 msecs
[   72.975677] Restarting tasks ... done.

I've looked in the debugfs and see:
# cat suspend_stats
success: 1
fail: 0
failed_freeze: 0
failed_prepare: 0
failed_suspend: 0
failed_suspend_noirq: 0
failed_resume: 0
failed_resume_noirq: 0
failures:
  last_failed_dev:

  last_failed_errno:    0
                        0
  last_failed_step:

# cat wakeup_sources
name            active_count    event_count     hit_count       active_since    total_time      max_time        last_change
gpio-keys       0               0               0               0               0               0               0
(null)          0               0               0               0               0               0               0
(null)          0               0               0               0               0               0               0
1-0048          0               0               0               0               0               0               0
omap_uart.3     0               0               0               0               0               0               0
omap_uart.2     0               0               0               0               0               0               0

So have no idea what is causing the wakeup.

How should I go about debugging this further?

Has anyone else tried suspend on 3.3?

I've tried this both with my custom defconfig and omap2plus_defconfig.

I do have some modifications to the GUMSTIX overo board file (minor changes like defining some extra buttons, and setting the correct LCD parameters), but I'm fairly certain these shouldn't be causing any problems...

Cheers,
Joe

Re: Suspend broken on 3.3?

[Kevin Hilman]

"Joe Woodward" <jw@terrafix.co.uk> writes:

> Is system suspend broken on stock 3.3?

I hope not. :)

It *should* work, I'm using it regularily here, and "it works for me"
(I'm sure that's just what you want to hear.)  :)

> I have a working stock 3.2 (with patches to fix runtime_pm for DSS2), and system suspend works just fine!
>
> This is running on a variety of GUMSTIX boards (both OMAP3530 and AM3703-based).

I currently only have a 3530-based Gumstix Overo (although a
AM3xxx-based one is on the way, thanks Gumstix!), but it's working fine
for me on my Overo.

Stock v3.3 won't boot on Overo because of the smsc911x regulator issues
recently discusssed, so if you're using Overo, you also need the patch
in Tony's fix-smsc911x-regulator branch.

After that, suspend/resume is working fine for me using
omap2plus_defconfig.  I tried both with initramfs and with MMC rootfs.

Can you try without your board file changes, using vanilla v3.3 +
smsc911x fix above and using omap2plus_defconfig?

Also, please share the kernel command-line you're using.

Thanks,

Kevin

Re: Suspend broken on 3.3?

[Joe Woodward]

-----Original Message-----
From: Kevin Hilman <khilman@ti.com>
To: "Joe Woodward" <jw@terrafix.co.uk>
Cc: "linux-omap\@vger.kernel.org" <linux-omap@vger.kernel.org>
Date: Thu, 22 Mar 2012 10:33:56 -0700
Subject: Re: Suspend broken on 3.3?

> "Joe Woodward" <jw@terrafix.co.uk> writes:
> 
> > Is system suspend broken on stock 3.3?
> 
> I hope not. :)
> 
> It *should* work, I'm using it regularily here, and "it works for me"
> (I'm sure that's just what you want to hear.)  :)
> 
> > I have a working stock 3.2 (with patches to fix runtime_pm for DSS2),
> and system suspend works just fine!
> >
> > This is running on a variety of GUMSTIX boards (both OMAP3530 and
> AM3703-based).
> 
> I currently only have a 3530-based Gumstix Overo (although a
> AM3xxx-based one is on the way, thanks Gumstix!), but it's working fine
> for me on my Overo.
> 
> Stock v3.3 won't boot on Overo because of the smsc911x regulator issues
> recently discusssed, so if you're using Overo, you also need the patch
> in Tony's fix-smsc911x-regulator branch.
> 
> After that, suspend/resume is working fine for me using
> omap2plus_defconfig.  I tried both with initramfs and with MMC rootfs.
> 
> Can you try without your board file changes, using vanilla v3.3 +
> smsc911x fix above and using omap2plus_defconfig?
> 
> Also, please share the kernel command-line you're using.

Right, I've stepped back a bit and dug out a GUSMTIX Palo43 carrier board on which to test the Overo OMAP3530 COM and I've found:
 - Running a stock 3.3 (with absolutely no changes) does indeed suspend correctly.
 - Running the 3.3 kernel with my (minor) board modifications (basically defining some buttons) suspends correctly as well.

Then I went back to my original board and the 3.3 still wakes up from suspend immediately. So I had a think, and the only real differences 
between my board the the GUMSTIX Palo43 board is that I am using multiple UARTs.

Up to this point I've only wanted to wake on the console (ttyO2), and not any other UARTs so I've stopped them waking with:
  echo disabled > /sys/devices/platform/omap/omap_uart.0/power/wakeup
  echo disabled > /sys/devices/platform/omap/omap_uart.1/power/wakeup

I wanted to check that this still worked, so tried disabling wakeup on the console (ttyO2):
  echo disabled > /sys/devices/platform/omap/omap_uart.2/power/wakeup

And if I do "echo mem > /sys/power/state" I was expecting to stay in suspend when I typed on my keyboard... However, the kernel still 
woke from suspend, which leads me to believe that the UART wakeup hasn't been disabled?

Could you test if this is also the case your end?

Cheers,
Joe

> Thanks,
> 
> 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?

[Kevin Hilman]

+Govindraj,

"Joe Woodward" <jw@terrafix.co.uk> writes:

> Right, I've stepped back a bit and dug out a GUSMTIX Palo43 carrier
> board on which to test the Overo OMAP3530 COM and I've found:
>  - Running a stock 3.3 (with absolutely no changes) does indeed suspend correctly.
>  - Running the 3.3 kernel with my (minor) board modifications
>  (basically defining some buttons) suspends correctly as well.
>
> Then I went back to my original board and the 3.3 still wakes up from
> suspend immediately. So I had a think, and the only real differences
> between my board the the GUMSTIX Palo43 board is that I am using
> multiple UARTs.
>
> Up to this point I've only wanted to wake on the console (ttyO2), and
> not any other UARTs so I've stopped them waking with:
>   echo disabled > /sys/devices/platform/omap/omap_uart.0/power/wakeup
>   echo disabled > /sys/devices/platform/omap/omap_uart.1/power/wakeup
>
> I wanted to check that this still worked, so tried disabling wakeup on
> the console (ttyO2):
>   echo disabled > /sys/devices/platform/omap/omap_uart.2/power/wakeup
>
> And if I do "echo mem > /sys/power/state" I was expecting to stay in
> suspend when I typed on my keyboard... However, the kernel still woke
> from suspend, which leads me to believe that the UART wakeup hasn't
> been disabled?

Just to confirm: did the above work for you before v3.3?

> Could you test if this is also the case your end?

Yes, I get the same behavior, which is indeed broken.

Govindraj, can you look into this?

A quick glance suggests that disabling wakeups via the sysfs file is
only disabling runtime PM, but not actually disabling wakups at the
module-level or at the IO ring.

Kevin

Re: Suspend broken on 3.3?

[Raja, Govindraj]

Hi Kevin,

On Tue, Mar 27, 2012 at 6:04 AM, Kevin Hilman <khilman@ti.com> wrote:
> +Govindraj,
>
> "Joe Woodward" <jw@terrafix.co.uk> writes:
>
>> Right, I've stepped back a bit and dug out a GUSMTIX Palo43 carrier
>> board on which to test the Overo OMAP3530 COM and I've found:
>>  - Running a stock 3.3 (with absolutely no changes) does indeed suspend correctly.
>>  - Running the 3.3 kernel with my (minor) board modifications
>>  (basically defining some buttons) suspends correctly as well.
>>
>> Then I went back to my original board and the 3.3 still wakes up from
>> suspend immediately. So I had a think, and the only real differences
>> between my board the the GUMSTIX Palo43 board is that I am using
>> multiple UARTs.
>>
>> Up to this point I've only wanted to wake on the console (ttyO2), and
>> not any other UARTs so I've stopped them waking with:
>>   echo disabled > /sys/devices/platform/omap/omap_uart.0/power/wakeup
>>   echo disabled > /sys/devices/platform/omap/omap_uart.1/power/wakeup
>>
>> I wanted to check that this still worked, so tried disabling wakeup on
>> the console (ttyO2):
>>   echo disabled > /sys/devices/platform/omap/omap_uart.2/power/wakeup
>>
>> And if I do "echo mem > /sys/power/state" I was expecting to stay in
>> suspend when I typed on my keyboard... However, the kernel still woke
>> from suspend, which leads me to believe that the UART wakeup hasn't
>> been disabled?
>
> Just to confirm: did the above work for you before v3.3?
>
>> Could you test if this is also the case your end?
>
> Yes, I get the same behavior, which is indeed broken.
>
> Govindraj, can you look into this?
>
> A quick glance suggests that disabling wakeups via the sysfs file is
> only disabling runtime PM, but not actually disabling wakups at the
> module-level or at the IO ring.
>

I have started looking into this, disabling and enabling of wake-ups
from .runtime_suspend needs some changes as in here[1] with that I see pad
wakeup getting disabled and it doesn't wake up after enabling off mode
and suspending.

If clocks left enabled form uart driver during system wide suspend
-> _od_suspend_noirq
    -> .runtime_suspend from uart driver (with [1])
         -> omap_hwmod_disable_wakeup
          -> omap_device_idle

But module level wakeup from sysc reg also needs to be disabled,
with which I see some strange behavior, even though _disable_wakeup
updates sysc reg it seem to wakeup, but removing SYSC_HAS_ENAWAKEUP
flag from .sysc flags from hmwod data I see module level wakeup failing after
disabling wakeup. Still checking on this.

--
Thanks,
Govindraj.R

[1]:

From 7f92f73006a82fdd7328fe7906fbf094b503cd13 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 wakeup enable mechanism

The module level wakeups are enabled by default during bootup
init from hmwod framework and pad wakeup will be disabled.

Correct the condition check in uart runtime suspend path to
enable/disable wakeups.

Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
 arch/arm/plat-omap/include/plat/omap-serial.h |    3 ++-
 drivers/tty/serial/omap-serial.c              |   16 ++++++++++++----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h
b/arch/arm/plat-omap/include/plat/omap-serial.h
index 9ff4444..386a25b 100644
--- a/arch/arm/plat-omap/include/plat/omap-serial.h
+++ b/arch/arm/plat-omap/include/plat/omap-serial.h
@@ -130,7 +130,8 @@ struct uart_omap_port {
 	unsigned long		port_activity;
 	u32			context_loss_cnt;
 	u32			errata;
-	u8			wakeups_enabled;
+	u8			pad_wakeups_enabled;
+	u8			module_wakeups_enabled;

 	struct pm_qos_request	pm_qos_request;
 	u32			latency;
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 0121486..0a35b7e 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1458,6 +1458,12 @@ static int serial_omap_probe(struct
platform_device *pdev)
 		up->uart_dma.rx_dma_channel = OMAP_UART_DMA_CH_FREE;
 	}

+	/* Module level wakeup from sysc(BIT[2])
+	 * will be enabled during boot
+	 * from hwmod framework.
+	 */
+	up->module_wakeups_enabled = true;
+
 	up->latency = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE;
 	up->calc_latency = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE;
 	pm_qos_add_request(&up->pm_qos_request,
@@ -1586,14 +1592,16 @@ static int serial_omap_runtime_suspend(struct
device *dev)
 		up->context_loss_cnt = pdata->get_context_loss_count(dev);

 	if (device_may_wakeup(dev)) {
-		if (!up->wakeups_enabled) {
+		if (!up->pad_wakeups_enabled || !up->module_wakeups_enabled) {
 			pdata->enable_wakeup(up->pdev, true);
-			up->wakeups_enabled = true;
+			up->module_wakeups_enabled = true;
+			up->pad_wakeups_enabled = true;
 		}
 	} else {
-		if (up->wakeups_enabled) {
+		if (up->pad_wakeups_enabled || up->module_wakeups_enabled) {
 			pdata->enable_wakeup(up->pdev, false);
-			up->wakeups_enabled = false;
+			up->module_wakeups_enabled = false;
+			up->pad_wakeups_enabled = false;
 		}
 	}

-- 
1.7.5.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

Re: Suspend broken on 3.3?

[Joe Woodward]

...snip...

> Just to confirm: did the above work for you before v3.3?
> 

I've checked and v3.2 works correctly:

echo "enabled" > /sys/devices/platform/omap/omap_uart.2/power/wakeup => device wakes from console presses
echo "disabled" > /sys/devices/platform/omap/omap_uart.2/power/wakeup => device does not wake from console presses

Thanks for looking in to this!

Cheers,
Joe 

> > Could you test if this is also the case your end?
> 
> Yes, I get the same behavior, which is indeed broken.
> 
> Govindraj, can you look into this?
> 
> A quick glance suggests that disabling wakeups via the sysfs file is
> only disabling runtime PM, but not actually disabling wakups at the
> module-level or at the IO ring.
> 
> 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?

[Kevin Hilman]

"Joe Woodward" <jw@terrafix.co.uk> writes:

> ...snip...
>
>> Just to confirm: did the above work for you before v3.3?
>> 
>
> I've checked and v3.2 works correctly:
>
> echo "enabled" > /sys/devices/platform/omap/omap_uart.2/power/wakeup => device wakes from console presses
> echo "disabled" > /sys/devices/platform/omap/omap_uart.2/power/wakeup => device does not wake from console presses

OK, that's what I suspected.  Thanks for confirming.

Kevin

Re: Suspend broken on 3.3?

[Kevin Hilman]

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

> Hi Kevin,
>
> On Tue, Mar 27, 2012 at 6:04 AM, Kevin Hilman <khilman@ti.com> wrote:
>> +Govindraj,
>>
>> "Joe Woodward" <jw@terrafix.co.uk> writes:
>>
>>> Right, I've stepped back a bit and dug out a GUSMTIX Palo43 carrier
>>> board on which to test the Overo OMAP3530 COM and I've found:
>>>  - Running a stock 3.3 (with absolutely no changes) does indeed suspend correctly.
>>>  - Running the 3.3 kernel with my (minor) board modifications
>>>  (basically defining some buttons) suspends correctly as well.
>>>
>>> Then I went back to my original board and the 3.3 still wakes up from
>>> suspend immediately. So I had a think, and the only real differences
>>> between my board the the GUMSTIX Palo43 board is that I am using
>>> multiple UARTs.
>>>
>>> Up to this point I've only wanted to wake on the console (ttyO2), and
>>> not any other UARTs so I've stopped them waking with:
>>>   echo disabled > /sys/devices/platform/omap/omap_uart.0/power/wakeup
>>>   echo disabled > /sys/devices/platform/omap/omap_uart.1/power/wakeup
>>>
>>> I wanted to check that this still worked, so tried disabling wakeup on
>>> the console (ttyO2):
>>>   echo disabled > /sys/devices/platform/omap/omap_uart.2/power/wakeup
>>>
>>> And if I do "echo mem > /sys/power/state" I was expecting to stay in
>>> suspend when I typed on my keyboard... However, the kernel still woke
>>> from suspend, which leads me to believe that the UART wakeup hasn't
>>> been disabled?
>>
>> Just to confirm: did the above work for you before v3.3?
>>
>>> Could you test if this is also the case your end?
>>
>> Yes, I get the same behavior, which is indeed broken.
>>
>> Govindraj, can you look into this?
>>
>> A quick glance suggests that disabling wakeups via the sysfs file is
>> only disabling runtime PM, but not actually disabling wakups at the
>> module-level or at the IO ring.
>>
>
> I have started looking into this, disabling and enabling of wake-ups
> from .runtime_suspend needs some changes as in here[1] with that I see pad
> wakeup getting disabled and it doesn't wake up after enabling off mode
> and suspending.

Thanks for looking into this.

> If clocks left enabled form uart driver during system wide suspend
> -> _od_suspend_noirq
>     -> .runtime_suspend from uart driver (with [1])
>          -> omap_hwmod_disable_wakeup
>           -> omap_device_idle
>
> But module level wakeup from sysc reg also needs to be disabled,
> with which I see some strange behavior, even though _disable_wakeup
> updates sysc reg it seem to wakeup, but removing SYSC_HAS_ENAWAKEUP
> flag from .sysc flags from hmwod data I see module level wakeup failing after
> disabling wakeup. Still checking on this.
>
> --
> Thanks,
> Govindraj.R
>
> [1]:
>
> From 7f92f73006a82fdd7328fe7906fbf094b503cd13 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 wakeup enable mechanism
>
> The module level wakeups are enabled by default during bootup
> init from hmwod framework and pad wakeup will be disabled.
>
> Correct the condition check in uart runtime suspend path to
> enable/disable wakeups.
>
> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
> ---
>  arch/arm/plat-omap/include/plat/omap-serial.h |    3 ++-
>  drivers/tty/serial/omap-serial.c              |   16 ++++++++++++----
>  2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h
> b/arch/arm/plat-omap/include/plat/omap-serial.h
> index 9ff4444..386a25b 100644
> --- a/arch/arm/plat-omap/include/plat/omap-serial.h
> +++ b/arch/arm/plat-omap/include/plat/omap-serial.h
> @@ -130,7 +130,8 @@ struct uart_omap_port {
>  	unsigned long		port_activity;
>  	u32			context_loss_cnt;
>  	u32			errata;
> -	u8			wakeups_enabled;
> +	u8			pad_wakeups_enabled;
> +	u8			module_wakeups_enabled;

Why do you need 2 flags when they are always managed together.

Kevin

>  	struct pm_qos_request	pm_qos_request;
>  	u32			latency;
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 0121486..0a35b7e 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -1458,6 +1458,12 @@ static int serial_omap_probe(struct
> platform_device *pdev)
>  		up->uart_dma.rx_dma_channel = OMAP_UART_DMA_CH_FREE;
>  	}
>
> +	/* Module level wakeup from sysc(BIT[2])
> +	 * will be enabled during boot
> +	 * from hwmod framework.
> +	 */
> +	up->module_wakeups_enabled = true;
> +
>  	up->latency = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE;
>  	up->calc_latency = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE;
>  	pm_qos_add_request(&up->pm_qos_request,
> @@ -1586,14 +1592,16 @@ static int serial_omap_runtime_suspend(struct
> device *dev)
>  		up->context_loss_cnt = pdata->get_context_loss_count(dev);
>
>  	if (device_may_wakeup(dev)) {
> -		if (!up->wakeups_enabled) {
> +		if (!up->pad_wakeups_enabled || !up->module_wakeups_enabled) {
>  			pdata->enable_wakeup(up->pdev, true);
> -			up->wakeups_enabled = true;
> +			up->module_wakeups_enabled = true;
> +			up->pad_wakeups_enabled = true;
>  		}
>  	} else {
> -		if (up->wakeups_enabled) {
> +		if (up->pad_wakeups_enabled || up->module_wakeups_enabled) {
>  			pdata->enable_wakeup(up->pdev, false);
> -			up->wakeups_enabled = false;
> +			up->module_wakeups_enabled = false;
> +			up->pad_wakeups_enabled = false;
>  		}
>  	}
--
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 Wed, Mar 28, 2012 at 3:07 AM, Kevin Hilman <khilman@ti.com> wrote:
> "Raja, Govindraj" <govindraj.raja@ti.com> writes:
>
>> Hi Kevin,
>>
>> On Tue, Mar 27, 2012 at 6:04 AM, Kevin Hilman <khilman@ti.com> wrote:
>>> +Govindraj,
>>>
>>> "Joe Woodward" <jw@terrafix.co.uk> writes:
>>>
>>>> Right, I've stepped back a bit and dug out a GUSMTIX Palo43 carrier
>>>> board on which to test the Overo OMAP3530 COM and I've found:
>>>>  - Running a stock 3.3 (with absolutely no changes) does indeed suspend correctly.
>>>>  - Running the 3.3 kernel with my (minor) board modifications
>>>>  (basically defining some buttons) suspends correctly as well.
>>>>
>>>> Then I went back to my original board and the 3.3 still wakes up from
>>>> suspend immediately. So I had a think, and the only real differences
>>>> between my board the the GUMSTIX Palo43 board is that I am using
>>>> multiple UARTs.
>>>>
>>>> Up to this point I've only wanted to wake on the console (ttyO2), and
>>>> not any other UARTs so I've stopped them waking with:
>>>>   echo disabled > /sys/devices/platform/omap/omap_uart.0/power/wakeup
>>>>   echo disabled > /sys/devices/platform/omap/omap_uart.1/power/wakeup
>>>>
>>>> I wanted to check that this still worked, so tried disabling wakeup on
>>>> the console (ttyO2):
>>>>   echo disabled > /sys/devices/platform/omap/omap_uart.2/power/wakeup
>>>>
>>>> And if I do "echo mem > /sys/power/state" I was expecting to stay in
>>>> suspend when I typed on my keyboard... However, the kernel still woke
>>>> from suspend, which leads me to believe that the UART wakeup hasn't
>>>> been disabled?
>>>
>>> Just to confirm: did the above work for you before v3.3?
>>>
>>>> Could you test if this is also the case your end?
>>>
>>> Yes, I get the same behavior, which is indeed broken.
>>>
>>> Govindraj, can you look into this?
>>>
>>> A quick glance suggests that disabling wakeups via the sysfs file is
>>> only disabling runtime PM, but not actually disabling wakups at the
>>> module-level or at the IO ring.
>>>
>>
>> I have started looking into this, disabling and enabling of wake-ups
>> from .runtime_suspend needs some changes as in here[1] with that I see pad
>> wakeup getting disabled and it doesn't wake up after enabling off mode
>> and suspending.
>
> Thanks for looking into this.
>

Looks like the module wakeup event handling left to default
value during runtime clean up is causing the wakeup to happen

Here is the patch[1] to fix the same tested on 3430SDP.

--
Thanks,
Govindraj.R

[1]:


From 5a5126750d527811547a45de9546c3e0f0fac77d 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.

Adding the support to handle module level wakeups for omap2/3 socs.

Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
 arch/arm/mach-omap2/serial.c |   95 +++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 93 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index f590afc..92ff94c 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -56,6 +56,10 @@ struct omap_uart_state {
 	int num;
 	int can_sleep;

+	void __iomem *wk_st;
+	void __iomem *wk_en;
+	u32 wk_mask;
+
 	struct list_head node;
 	struct omap_hwmod *oh;
 	struct platform_device *pdev;
@@ -82,17 +86,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]);
+	}
 }

 /*
@@ -114,7 +147,64 @@ 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;
+	}
+
+	/* Module level wakeups might be left enabled from
+	 * bootloader disable it. Module level wakeup/pad_wakeup
+	 * will be enabled if port is wakeup capaable after gating uart
+	 * port clocks from omap-serial driver.
+	 * Wakeup capability can be enabled or disbaled using sysfs entry.
+	 */
+	omap_uart_disable_module_wakeup(uart);
+}
+
 #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) {}
@@ -345,6 +435,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;
-- 
1.7.5.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

Re: Suspend broken on 3.3?

[Joe Woodward]

-----Original Message-----
From: "Raja, Govindraj" <govindraj.raja@ti.com>
To: Kevin Hilman <khilman@ti.com>
Cc: Joe Woodward <jw@terrafix.co.uk>, "linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>, Felipe Balbi <balbi@ti.com>
Date: Wed, 28 Mar 2012 16:29:53 +0530
Subject: Re: Suspend broken on 3.3?

> On Wed, Mar 28, 2012 at 3:07 AM, Kevin Hilman <khilman@ti.com> wrote:
> > "Raja, Govindraj" <govindraj.raja@ti.com> writes:
> >
> >> Hi Kevin,
> >>
> >> On Tue, Mar 27, 2012 at 6:04 AM, Kevin Hilman <khilman@ti.com>
> wrote:
> >>> +Govindraj,
> >>>
> >>> "Joe Woodward" <jw@terrafix.co.uk> writes:
> >>>
> >>>> Right, I've stepped back a bit and dug out a GUSMTIX Palo43
> carrier
> >>>> board on which to test the Overo OMAP3530 COM and I've found:
> >>>>  - Running a stock 3.3 (with absolutely no changes) does indeed
> suspend correctly.
> >>>>  - Running the 3.3 kernel with my (minor) board modifications
> >>>>  (basically defining some buttons) suspends correctly as well.
> >>>>
> >>>> Then I went back to my original board and the 3.3 still wakes up
> from
> >>>> suspend immediately. So I had a think, and the only real
> differences
> >>>> between my board the the GUMSTIX Palo43 board is that I am using
> >>>> multiple UARTs.
> >>>>
> >>>> Up to this point I've only wanted to wake on the console (ttyO2),
> and
> >>>> not any other UARTs so I've stopped them waking with:
> >>>>   echo disabled >
> /sys/devices/platform/omap/omap_uart.0/power/wakeup
> >>>>   echo disabled >
> /sys/devices/platform/omap/omap_uart.1/power/wakeup
> >>>>
> >>>> I wanted to check that this still worked, so tried disabling
> wakeup on
> >>>> the console (ttyO2):
> >>>>   echo disabled >
> /sys/devices/platform/omap/omap_uart.2/power/wakeup
> >>>>
> >>>> And if I do "echo mem > /sys/power/state" I was expecting to stay
> in
> >>>> suspend when I typed on my keyboard... However, the kernel still
> woke
> >>>> from suspend, which leads me to believe that the UART wakeup
> hasn't
> >>>> been disabled?
> >>>
> >>> Just to confirm: did the above work for you before v3.3?
> >>>
> >>>> Could you test if this is also the case your end?
> >>>
> >>> Yes, I get the same behavior, which is indeed broken.
> >>>
> >>> Govindraj, can you look into this?
> >>>
> >>> A quick glance suggests that disabling wakeups via the sysfs file
> is
> >>> only disabling runtime PM, but not actually disabling wakups at the
> >>> module-level or at the IO ring.
> >>>
> >>
> >> I have started looking into this, disabling and enabling of wake-ups
> >> from .runtime_suspend needs some changes as in here[1] with that I
> see pad
> >> wakeup getting disabled and it doesn't wake up after enabling off
> mode
> >> and suspending.
> >
> > Thanks for looking into this.
> >
> 
> Looks like the module wakeup event handling left to default
> value during runtime clean up is causing the wakeup to happen
> 
> Here is the patch[1] to fix the same tested on 3430SDP.
> 
> --
> Thanks,

Thanks,

This patch fixes the suspend problem for me, but there is another UART issue...

Basically I've got a fairly high speed data source (in UART terms, >900KBaud) pumping data to the OMAP (such as GPS positions).

I don't want this to wake me when suspended (which this patch fixes).

However, it seems on 3.3 that I get a lot of corruption/lost characters, which I'm assuming is because the UART is implementing runtime-PM.

So my next question is: How do I disable runtime-PM/force-always-on for a given UART? Can this be done via the sysfs?

Or where are the runtime-PM constraints set for the UART? I'm guessing they'll work for 115200Baud, but my high speed UART fowls these?

Cheers,
Joe


> Govindraj.R
> 
> [1]:
> 
> 
> From 5a5126750d527811547a45de9546c3e0f0fac77d 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.
> 
> Adding the support to handle module level wakeups for omap2/3 socs.
> 
> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
> ---
>  arch/arm/mach-omap2/serial.c |   95
> +++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 93 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/serial.c
> b/arch/arm/mach-omap2/serial.c
> index f590afc..92ff94c 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -56,6 +56,10 @@ struct omap_uart_state {
>  	int num;
>  	int can_sleep;
> 
> +	void __iomem *wk_st;
> +	void __iomem *wk_en;
> +	u32 wk_mask;
> +
>  	struct list_head node;
>  	struct omap_hwmod *oh;
>  	struct platform_device *pdev;
> @@ -82,17 +86,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]);
> +	}
>  }
> 
>  /*
> @@ -114,7 +147,64 @@ 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;
> +	}
> +
> +	/* Module level wakeups might be left enabled from
> +	 * bootloader disable it. Module level wakeup/pad_wakeup
> +	 * will be enabled if port is wakeup capaable after gating uart
> +	 * port clocks from omap-serial driver.
> +	 * Wakeup capability can be enabled or disbaled using sysfs entry.
> +	 */
> +	omap_uart_disable_module_wakeup(uart);
> +}
> +
>  #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) {}
> @@ -345,6 +435,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;
> -- 
> 1.7.5.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


--
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]

+Paul, NeilBrown who both have worked on/around recent UART breakage
 since v3.2

"Joe Woodward" <jw@terrafix.co.uk> writes:

[...]

> This patch fixes the suspend problem for me, but there is another UART issue...
>
> Basically I've got a fairly high speed data source (in UART terms,
> >900KBaud) pumping data to the OMAP (such as GPS positions).
>
> I don't want this to wake me when suspended (which this patch fixes).
>
> However, it seems on 3.3 that I get a lot of corruption/lost
> characters, which I'm assuming is because the UART is implementing
> runtime-PM.
>
> So my next question is: How do I disable runtime-PM/force-always-on
> for a given UART? Can this be done via the sysfs?

Yes, but the boot-time default for this is that the UARTs have runtime
PM disabled since the default autosuspend timeout is set to -1.

You must be setting an autosuspend timeout >0 if you're seeing runtime
PM kick in.

That being said, even with an autosuspend timeout enabled, you can
disable runtime PM by setting the /sys/devices/.../power/control knob to
'on' (instead of auto, which means it's controle by runtime PM):

  echo on > /sys/devices/platform/omap/omap_uart.2/power/control

That will disable runtime PM and leave the UARTs always clocked.

> Or where are the runtime-PM constraints set for the UART? 

Look for this in the driver:

	/* calculate wakeup latency constraint */
	up->calc_latency = (USEC_PER_SEC * up->port.fifosize) / (baud / 8);

> I'm guessing they'll work for 115200Baud, but my high speed UART fowls
> these?

The constraint calculations take into account baud rate, but are known
to be somewhat broken currently.

You might want to experiment with Paul's work on fixing up the QoS
contstraint calculation[1] to see if it helps as well.  That is available here

Kevin

[1] git://git.pwsan.com/linux-2.6 omap_serial_fix_pm_qos_formula_devel_3.4

Re: Suspend broken on 3.3?

[Joe Woodward]

-----Original Message-----
From: Kevin Hilman <khilman@ti.com>
To: "Joe Woodward" <jw@terrafix.co.uk>
Cc: "Raja\, Govindraj" <govindraj.raja@ti.com>, linux-omap@vger.kernel.org, "Felipe Balbi" <balbi@ti.com>, Paul Walmsley <paul@pwsan.com>, neilb@suse.de
Date: Wed, 28 Mar 2012 10:46:23 -0700
Subject: Re: Suspend broken on 3.3?

> +Paul, NeilBrown who both have worked on/around recent UART breakage
>  since v3.2
> 
> "Joe Woodward" <jw@terrafix.co.uk> writes:
> 
> [...]
> 
> > This patch fixes the suspend problem for me, but there is another
> UART issue...
> >
> > Basically I've got a fairly high speed data source (in UART terms,
> > >900KBaud) pumping data to the OMAP (such as GPS positions).
> >
> > I don't want this to wake me when suspended (which this patch fixes).
> >
> > However, it seems on 3.3 that I get a lot of corruption/lost
> > characters, which I'm assuming is because the UART is implementing
> > runtime-PM.
> >
> > So my next question is: How do I disable runtime-PM/force-always-on
> > for a given UART? Can this be done via the sysfs?
> 
> Yes, but the boot-time default for this is that the UARTs have runtime
> PM disabled since the default autosuspend timeout is set to -1.
> 
> You must be setting an autosuspend timeout >0 if you're seeing runtime
> PM kick in.
> 
> That being said, even with an autosuspend timeout enabled, you can
> disable runtime PM by setting the /sys/devices/.../power/control knob
> to
> 'on' (instead of auto, which means it's controle by runtime PM):
> 
>   echo on > /sys/devices/platform/omap/omap_uart.2/power/control
> 

Right,

First an apology... After checking  /sys/devices/platform/omap/omap_uart.2/power/control I can see that runtime-PM is indeed disabled.

After digging a bit further I found that the problem isn't lost characters or character corruption at all...

The UART is actually at 430KBaud (not 900KBaud as I mentioned earlier). The data received is very bursty (i.e. sets of messages every second or so), containing a 
sync sequence to indicate a start of packet.

The received bytes should be: 0x01, 0x52, 0x41 ....rest of packet.

This works 100% of the time on 3.2, but on 3.3 I sometimes (but not always) get: 0x01, 0x00, 0x52, 0x41.

i.e. there is a NULL/0x00 inserted after the first character.

All this is tested using a very simple userspace application thats reads data from ttyO1.

Any ideas? Should I kick open a new thread as it's not really to do with suspend anymore?

Thanks,
Joe

> That will disable runtime PM and leave the UARTs always clocked.
> 
> > Or where are the runtime-PM constraints set for the UART? 
> 
> Look for this in the driver:
> 
> 	/* calculate wakeup latency constraint */
> 	up->calc_latency = (USEC_PER_SEC * up->port.fifosize) / (baud / 8);
> 
> > I'm guessing they'll work for 115200Baud, but my high speed UART
> fowls
> > these?
> 
> The constraint calculations take into account baud rate, but are known
> to be somewhat broken currently.
> 
> You might want to experiment with Paul's work on fixing up the QoS
> contstraint calculation[1] to see if it helps as well.  That is
> available here
> 
> Kevin
> 
> [1] git://git.pwsan.com/linux-2.6
> omap_serial_fix_pm_qos_formula_devel_3.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

Re: Suspend broken on 3.3?

[Shubhrajyoti Datta]

Hi Joe,

> After digging a bit further I found that the problem isn't lost characters or character corruption at all...
>
> The UART is actually at 430KBaud (not 900KBaud as I mentioned earlier).
How did you verify that register read?


The data received is very bursty (i.e. sets of messages every second
or so), containing a
> sync sequence to indicate a start of packet.
>
> The received bytes should be: 0x01, 0x52, 0x41 ....rest of packet.
>
> This works 100% of the time on 3.2, but on 3.3 I sometimes (but not always) get: 0x01, 0x00, 0x52, 0x41.
>
> i.e. there is a NULL/0x00 inserted after the first character.
>
> All this is tested using a very simple userspace application thats reads data from ttyO1.
>
> Any ideas? Should I kick open a new thread as it's not really to do with suspend anymore?
Is there any flow control you are using?

>

Re: Suspend broken on 3.3?

[Joe Woodward]

> Hi Joe,
> 
> > After digging a bit further I found that the problem isn't lost
> characters or character corruption at all...
> >
> > The UART is actually at 460KBaud (not 900KBaud as I mentioned
> earlier).
> How did you verify that register read?
> 

I actually looked at the setting applied in my code (which I should have done earlier, oops :p).

...
  newtio.c_iflag = (IGNPAR);
  newtio.c_oflag = 0;
  newtio.c_cflag = B460800 | CS8 | CLOCAL | CREAD;
  newtio.c_lflag = 0;

  newtio.c_cc[VINTR]    = 0;     /* Ctrl-c */ 
  newtio.c_cc[VQUIT]    = 0;     /* Ctrl-\ */
  newtio.c_cc[VERASE]   = 0;     /* del */
  newtio.c_cc[VKILL]    = 0;     /* @ */
  newtio.c_cc[VEOF]     = 0;     /* Ctrl-d */
  newtio.c_cc[VTIME]    = 0;     /* inter-character timer unused */
  newtio.c_cc[VMIN]     = 1;     /* blocking read until 1 character arrives */
  newtio.c_cc[VSWTC]    = 0;     /* '\0' */
  newtio.c_cc[VSTART]   = 0;     /* Ctrl-q */ 
  newtio.c_cc[VSTOP]    = 0;     /* Ctrl-s */
  newtio.c_cc[VSUSP]    = 0;     /* Ctrl-z */
  newtio.c_cc[VEOL]     = 0;     /* '\0' */
  newtio.c_cc[VREPRINT] = 0;     /* Ctrl-r */
  newtio.c_cc[VDISCARD] = 0;     /* Ctrl-u */
  newtio.c_cc[VWERASE]  = 0;     /* Ctrl-w */
  newtio.c_cc[VLNEXT]   = 0;     /* Ctrl-v */
  newtio.c_cc[VEOL2]    = 0;     /* '\0' */

  /* Clean the modem line and activate the settings for the port
  */
  tcflush (handle->serialFD, TCIFLUSH);
  tcsetattr (handle->serialFD, TCSANOW, &newtio);
...

I then loop read from a file descriptor to see the received bytes:

  serialFD = open ("/dev/ttyO0", O_RDWR | O_NOCTTY); 
...
  while (1) {
    count = read (serialFD, buffer, MAXIMUM_LINE_LENGTH);
    ...
    debug here...
    ...
  }

And it's by inspecting the bytes read that I noticed the inserted 0x00 on 3.3.

> 
> The data received is very bursty (i.e. sets of messages every second
> or so), containing a
> > sync sequence to indicate a start of packet.
> >
> > The received bytes should be: 0x01, 0x52, 0x41 ....rest of packet.
> >
> > This works 100% of the time on 3.2, but on 3.3 I sometimes (but not
> always) get: 0x01, 0x00, 0x52, 0x41.
> >
> > i.e. there is a NULL/0x00 inserted after the first character.
> >
> > All this is tested using a very simple userspace application thats
> reads data from ttyO1.
> >
> > Any ideas? Should I kick open a new thread as it's not really to do
> with suspend anymore?
> Is there any flow control you are using?

No flow control, but lack of flow control hasn't caused problems up to 3.2.

Cheers,
Joe

> 
> >
> --
> 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?

[Paul Walmsley]

Hello Joe,

thanks for reporting this.  Some thoughts -- really just pure speculation 
-- but I hope some of it might be useful for you...

On Thu, 29 Mar 2012, Joe Woodward wrote:

> After digging a bit further I found that the problem isn't lost characters or character corruption at all...
> 
> The UART is actually at 430KBaud (not 900KBaud as I mentioned earlier).

430Kbps?  Could you confirm this?  OMAP UARTs don't support that rate as 
far as I know - the closest is 460Kbps (OMAP34xx TRM vZR Table 17-1 "UART 
Mode Baud Rates, Divisor Values, and Error Rates).  If one was desperate, 
it might be possible to get 430Kbps by tweaking other parts of the clock 
tree though...

> The data received is very bursty (i.e. sets of messages every second or 
> so), containing a sync sequence to indicate a start of packet.
> 
> The received bytes should be: 0x01, 0x52, 0x41 ....rest of packet.
> 
> This works 100% of the time on 3.2, but on 3.3 I sometimes (but not always) get: 0x01, 0x00, 0x52, 0x41.
> 
> i.e. there is a NULL/0x00 inserted after the first character.

Is this on the serial console, or on a non-console serial port?

Looking at drivers/tty/serial/omap-serial.c:receive_chars(), I wonder if 
the driver is somehow reading bytes from the RX FIFO when it's empty.  
It's unclear to me how this could happen.  But you might want to try doing 
an LSR read right before entering the loop in 
drivers/tty/serial/omap-serial.c:receive_chars().  In stock v3.3, this 
would mean adding

		lsr = serial_in(up, UART_LSR);

at line 190 of drivers/tty/serial/omap-serial.c.


You might also try the DMA path as an experiment.  This will totally wedge 
power management due to an insanely low timer expiration in that path, but 
at least might help narrow the problem down.  To do so with a quick hack, 
you could set omap_serial_default_info.dma_enabled to true instead of 
false at arch/arm/mach-omap2/serial.c:76.


And one other thing to try: does the behavior change if you set uart_debug 
to true at arch/arm/mach-omap2/serial.c:278 ?


- Paul

Re: Suspend broken on 3.3?

[Joe Woodward]

> Hello Joe,
> 
> thanks for reporting this.  Some thoughts -- really just pure
> speculation 
> -- but I hope some of it might be useful for you...
> 
> On Thu, 29 Mar 2012, Joe Woodward wrote:
> 
> > After digging a bit further I found that the problem isn't lost
> characters or character corruption at all...
> > 
> > The UART is actually at 430KBaud (not 900KBaud as I mentioned
> earlier).
> 
> 430Kbps?  Could you confirm this?  OMAP UARTs don't support that rate
> as 
> far as I know - the closest is 460Kbps (OMAP34xx TRM vZR Table 17-1
> "UART 
> Mode Baud Rates, Divisor Values, and Error Rates).  If one was
> desperate, 
> it might be possible to get 430Kbps by tweaking other parts of the
> clock 
> tree though...

Sorry for the confusion... It's 460KBaud - the 430 was just a typo in my previous mail...

> 
> > The data received is very bursty (i.e. sets of messages every second
> or 
> > so), containing a sync sequence to indicate a start of packet.
> > 
> > The received bytes should be: 0x01, 0x52, 0x41 ....rest of packet.
> > 
> > This works 100% of the time on 3.2, but on 3.3 I sometimes (but not
> always) get: 0x01, 0x00, 0x52, 0x41.
> > 
> > i.e. there is a NULL/0x00 inserted after the first character.
> 
> Is this on the serial console, or on a non-console serial port?
> 
> Looking at drivers/tty/serial/omap-serial.c:receive_chars(), I wonder
> if 
> the driver is somehow reading bytes from the RX FIFO when it's empty.  
> It's unclear to me how this could happen.  But you might want to try
> doing 
> an LSR read right before entering the loop in 
> drivers/tty/serial/omap-serial.c:receive_chars().  In stock v3.3, this 
> would mean adding
> 
> 		lsr = serial_in(up, UART_LSR);
> 
> at line 190 of drivers/tty/serial/omap-serial.c.
> 

Adding this is made no obvious difference.

> 
> You might also try the DMA path as an experiment.  This will totally
> wedge 
> power management due to an insanely low timer expiration in that path,
> but 
> at least might help narrow the problem down.  To do so with a quick
> hack, 
> you could set omap_serial_default_info.dma_enabled to true instead of 
> false at arch/arm/mach-omap2/serial.c:76.
> 

This did the trick (I've added it in addition to the LSR read above, i'll back out the LSR read and see if it still works).
When DMA is enabled the behaviour (as seen from my application in userspace) is the same as on the stock 3.2 kernel (i.e. for me it works :) ).

Cheers,
Joe

> 
> And one other thing to try: does the behavior change if you set
> uart_debug 
> to true at arch/arm/mach-omap2/serial.c:278 ?
> 
> 
> - Paul
> --
> 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?

[Joe Woodward]

-----Original Message-----
From: "Joe Woodward" <jw@terrafix.co.uk>
To: "Paul Walmsley" <paul@pwsan.com>
Cc: "Kevin Hilman" <khilman@ti.com>, "Raja\\, Govindraj" <govindraj.raja@ti.com>, linux-omap@vger.kernel.org, "Felipe Balbi" <balbi@ti.com>, neilb@suse.de
Date: Thu, 29 Mar 2012 12:27:55 +0100
Subject: Re: Suspend broken on 3.3?

> > Hello Joe,
> > 
> > thanks for reporting this.  Some thoughts -- really just pure
> > speculation 
> > -- but I hope some of it might be useful for you...
> > 
> > On Thu, 29 Mar 2012, Joe Woodward wrote:
> > 
> > > After digging a bit further I found that the problem isn't lost
> > characters or character corruption at all...
> > > 
> > > The UART is actually at 430KBaud (not 900KBaud as I mentioned
> > earlier).
> > 
> > 430Kbps?  Could you confirm this?  OMAP UARTs don't support that rate
> > as 
> > far as I know - the closest is 460Kbps (OMAP34xx TRM vZR Table 17-1
> > "UART 
> > Mode Baud Rates, Divisor Values, and Error Rates).  If one was
> > desperate, 
> > it might be possible to get 430Kbps by tweaking other parts of the
> > clock 
> > tree though...
> 
> Sorry for the confusion... It's 460KBaud - the 430 was just a typo in
> my previous mail...
> 
> > 
> > > The data received is very bursty (i.e. sets of messages every
> second
> > or 
> > > so), containing a sync sequence to indicate a start of packet.
> > > 
> > > The received bytes should be: 0x01, 0x52, 0x41 ....rest of packet.
> > > 
> > > This works 100% of the time on 3.2, but on 3.3 I sometimes (but not
> > always) get: 0x01, 0x00, 0x52, 0x41.
> > > 
> > > i.e. there is a NULL/0x00 inserted after the first character.
> > 
> > Is this on the serial console, or on a non-console serial port?
> > 
> > Looking at drivers/tty/serial/omap-serial.c:receive_chars(), I wonder
> > if 
> > the driver is somehow reading bytes from the RX FIFO when it's empty.
>  
> > It's unclear to me how this could happen.  But you might want to try
> > doing 
> > an LSR read right before entering the loop in 
> > drivers/tty/serial/omap-serial.c:receive_chars().  In stock v3.3,
> this 
> > would mean adding
> > 
> > 		lsr = serial_in(up, UART_LSR);
> > 
> > at line 190 of drivers/tty/serial/omap-serial.c.
> > 
> 
> Adding this is made no obvious difference.
> 
> > 
> > You might also try the DMA path as an experiment.  This will totally
> > wedge 
> > power management due to an insanely low timer expiration in that
> path,
> > but 
> > at least might help narrow the problem down.  To do so with a quick
> > hack, 
> > you could set omap_serial_default_info.dma_enabled to true instead of
> > false at arch/arm/mach-omap2/serial.c:76.
> > 
> 
> This did the trick (I've added it in addition to the LSR read above,
> i'll back out the LSR read and see if it still works).
> When DMA is enabled the behaviour (as seen from my application in
> userspace) is the same as on the stock 3.2 kernel (i.e. for me it works
> :) ).
> 

I've just realised that if anyone has joined this thread late, then I'm running in a state with Govindraj's patch in a previous mail in this thread applied to serial.c to 
fix the suspend issues due to the UART wakeup's not being correctly changed from userspace via sysfs.

It may actually by this patch that is causing the interrupt-enabled serial driver to have broken? The tty that is causing me a problem does have wake-from-suspend 
disabled for it from userspace...

Cheers,
Joe


> Cheers,
> Joe
> 
> > 
> > And one other thing to try: does the behavior change if you set
> > uart_debug 
> > to true at arch/arm/mach-omap2/serial.c:278 ?
> > 
> > 
> > - Paul
> > --
> > 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
> 
> 
> --
> 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 Thu, Mar 29, 2012 at 5:10 PM, Joe Woodward <jw@terrafix.co.uk> wrote:
>
>
> -----Original Message-----
> From: "Joe Woodward" <jw@terrafix.co.uk>
> To: "Paul Walmsley" <paul@pwsan.com>
> Cc: "Kevin Hilman" <khilman@ti.com>, "Raja\\, Govindraj" <govindraj.raja@ti.com>, linux-omap@vger.kernel.org, "Felipe Balbi" <balbi@ti.com>, neilb@suse.de
> Date: Thu, 29 Mar 2012 12:27:55 +0100
> Subject: Re: Suspend broken on 3.3?
>
>> > Hello Joe,
>> >
>> > thanks for reporting this.  Some thoughts -- really just pure
>> > speculation
>> > -- but I hope some of it might be useful for you...
>> >
>> > On Thu, 29 Mar 2012, Joe Woodward wrote:
>> >
>> > > After digging a bit further I found that the problem isn't lost
>> > characters or character corruption at all...
>> > >
>> > > The UART is actually at 430KBaud (not 900KBaud as I mentioned
>> > earlier).
>> >
>> > 430Kbps?  Could you confirm this?  OMAP UARTs don't support that rate
>> > as
>> > far as I know - the closest is 460Kbps (OMAP34xx TRM vZR Table 17-1
>> > "UART
>> > Mode Baud Rates, Divisor Values, and Error Rates).  If one was
>> > desperate,
>> > it might be possible to get 430Kbps by tweaking other parts of the
>> > clock
>> > tree though...
>>
>> Sorry for the confusion... It's 460KBaud - the 430 was just a typo in
>> my previous mail...
>>
>> >
>> > > The data received is very bursty (i.e. sets of messages every
>> second
>> > or
>> > > so), containing a sync sequence to indicate a start of packet.
>> > >
>> > > The received bytes should be: 0x01, 0x52, 0x41 ....rest of packet.
>> > >
>> > > This works 100% of the time on 3.2, but on 3.3 I sometimes (but not
>> > always) get: 0x01, 0x00, 0x52, 0x41.
>> > >
>> > > i.e. there is a NULL/0x00 inserted after the first character.
>> >
>> > Is this on the serial console, or on a non-console serial port?
>> >
>> > Looking at drivers/tty/serial/omap-serial.c:receive_chars(), I wonder
>> > if
>> > the driver is somehow reading bytes from the RX FIFO when it's empty.
>>
>> > It's unclear to me how this could happen.  But you might want to try
>> > doing
>> > an LSR read right before entering the loop in
>> > drivers/tty/serial/omap-serial.c:receive_chars().  In stock v3.3,
>> this
>> > would mean adding
>> >
>> >             lsr = serial_in(up, UART_LSR);
>> >
>> > at line 190 of drivers/tty/serial/omap-serial.c.
>> >
>>
>> Adding this is made no obvious difference.
>>
>> >
>> > You might also try the DMA path as an experiment.  This will totally
>> > wedge
>> > power management due to an insanely low timer expiration in that
>> path,
>> > but
>> > at least might help narrow the problem down.  To do so with a quick
>> > hack,
>> > you could set omap_serial_default_info.dma_enabled to true instead of
>> > false at arch/arm/mach-omap2/serial.c:76.
>> >
>>
>> This did the trick (I've added it in addition to the LSR read above,
>> i'll back out the LSR read and see if it still works).
>> When DMA is enabled the behaviour (as seen from my application in
>> userspace) is the same as on the stock 3.2 kernel (i.e. for me it works
>> :) ).
>>
>
> I've just realised that if anyone has joined this thread late, then I'm running in a state with Govindraj's patch in a previous mail in this thread applied to serial.c to
> fix the suspend issues due to the UART wakeup's not being correctly changed from userspace via sysfs.
>
> It may actually by this patch that is causing the interrupt-enabled serial driver to have broken? The tty that is causing me a problem does have wake-from-suspend
> disabled for it from userspace...

Is the patch shared earlier causing this issue of getting 0x00 from rx
randomly ?

--
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?

[Joe Woodward]

-----Original Message-----
From: "Raja, Govindraj" <govindraj.raja@ti.com>
To: Joe Woodward <jw@terrafix.co.uk>
Cc: Paul Walmsley <paul@pwsan.com>, Kevin Hilman <khilman@ti.com>, linux-omap@vger.kernel.org, Felipe Balbi <balbi@ti.com>, neilb@suse.de
Date: Thu, 29 Mar 2012 19:59:54 +0530
Subject: Re: Suspend broken on 3.3?

> On Thu, Mar 29, 2012 at 5:10 PM, Joe Woodward <jw@terrafix.co.uk>
> wrote:
> >
> >
> > -----Original Message-----
> > From: "Joe Woodward" <jw@terrafix.co.uk>
> > To: "Paul Walmsley" <paul@pwsan.com>
> > Cc: "Kevin Hilman" <khilman@ti.com>, "Raja\\, Govindraj"
> <govindraj.raja@ti.com>, linux-omap@vger.kernel.org, "Felipe Balbi"
> <balbi@ti.com>, neilb@suse.de
> > Date: Thu, 29 Mar 2012 12:27:55 +0100
> > Subject: Re: Suspend broken on 3.3?
> >
> >> > Hello Joe,
> >> >
> >> > thanks for reporting this.  Some thoughts -- really just pure
> >> > speculation
> >> > -- but I hope some of it might be useful for you...
> >> >
> >> > On Thu, 29 Mar 2012, Joe Woodward wrote:
> >> >
> >> > > After digging a bit further I found that the problem isn't lost
> >> > characters or character corruption at all...
> >> > >
> >> > > The UART is actually at 430KBaud (not 900KBaud as I mentioned
> >> > earlier).
> >> >
> >> > 430Kbps?  Could you confirm this?  OMAP UARTs don't support that
> rate
> >> > as
> >> > far as I know - the closest is 460Kbps (OMAP34xx TRM vZR Table
> 17-1
> >> > "UART
> >> > Mode Baud Rates, Divisor Values, and Error Rates).  If one was
> >> > desperate,
> >> > it might be possible to get 430Kbps by tweaking other parts of the
> >> > clock
> >> > tree though...
> >>
> >> Sorry for the confusion... It's 460KBaud - the 430 was just a typo
> in
> >> my previous mail...
> >>
> >> >
> >> > > The data received is very bursty (i.e. sets of messages every
> >> second
> >> > or
> >> > > so), containing a sync sequence to indicate a start of packet.
> >> > >
> >> > > The received bytes should be: 0x01, 0x52, 0x41 ....rest of
> packet.
> >> > >
> >> > > This works 100% of the time on 3.2, but on 3.3 I sometimes (but
> not
> >> > always) get: 0x01, 0x00, 0x52, 0x41.
> >> > >
> >> > > i.e. there is a NULL/0x00 inserted after the first character.
> >> >
> >> > Is this on the serial console, or on a non-console serial port?
> >> >
> >> > Looking at drivers/tty/serial/omap-serial.c:receive_chars(), I
> wonder
> >> > if
> >> > the driver is somehow reading bytes from the RX FIFO when it's
> empty.
> >>
> >> > It's unclear to me how this could happen.  But you might want to
> try
> >> > doing
> >> > an LSR read right before entering the loop in
> >> > drivers/tty/serial/omap-serial.c:receive_chars().  In stock v3.3,
> >> this
> >> > would mean adding
> >> >
> >> >             lsr = serial_in(up, UART_LSR);
> >> >
> >> > at line 190 of drivers/tty/serial/omap-serial.c.
> >> >
> >>
> >> Adding this is made no obvious difference.
> >>
> >> >
> >> > You might also try the DMA path as an experiment.  This will
> totally
> >> > wedge
> >> > power management due to an insanely low timer expiration in that
> >> path,
> >> > but
> >> > at least might help narrow the problem down.  To do so with a
> quick
> >> > hack,
> >> > you could set omap_serial_default_info.dma_enabled to true instead
> of
> >> > false at arch/arm/mach-omap2/serial.c:76.
> >> >
> >>
> >> This did the trick (I've added it in addition to the LSR read above,
> >> i'll back out the LSR read and see if it still works).
> >> When DMA is enabled the behaviour (as seen from my application in
> >> userspace) is the same as on the stock 3.2 kernel (i.e. for me it
> works
> >> :) ).
> >>
> >
> > I've just realised that if anyone has joined this thread late, then
> I'm running in a state with Govindraj's patch in a previous mail in
> this thread applied to serial.c to
> > fix the suspend issues due to the UART wakeup's not being correctly
> changed from userspace via sysfs.
> >
> > It may actually by this patch that is causing the interrupt-enabled
> serial driver to have broken? The tty that is causing me a problem does
> have wake-from-suspend
> > disabled for it from userspace...
> 
> Is the patch shared earlier causing this issue of getting 0x00 from rx
> randomly ?
> 

In short, yes.

I've gone back to a stock 3.3 kernel and the serial ports give the correct data, but suspend fails (due to not being able to disable wake-from-serial).

I then applied your patch and could disable wakeup on the serial ports and suspend correctly, but the (non-console) serial port starts to misbehave.

Forcing the driver to be DMA-enabled caused everything to work again. So something in the patch is causing the (default) interrupt-enabled serial driver to 
occassionally fail.

Sorry for the goose chase yesterday. I didn't even think to check if the patch caused the issue as it seemed a bit unrelated.

Cheers,
Joe


> --
> 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


--
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]

[-- Attachment #1: Type: text/plain, Size: 5498 bytes --]

On Fri, Mar 30, 2012 at 1:23 PM, Joe Woodward <jw@terrafix.co.uk> wrote:
> -----Original Message-----
> From: "Raja, Govindraj" <govindraj.raja@ti.com>
> To: Joe Woodward <jw@terrafix.co.uk>
> Cc: Paul Walmsley <paul@pwsan.com>, Kevin Hilman <khilman@ti.com>, linux-omap@vger.kernel.org, Felipe Balbi <balbi@ti.com>, neilb@suse.de
> Date: Thu, 29 Mar 2012 19:59:54 +0530
> Subject: Re: Suspend broken on 3.3?
>
>> On Thu, Mar 29, 2012 at 5:10 PM, Joe Woodward <jw@terrafix.co.uk>
>> wrote:
>> >
>> >
>> > -----Original Message-----
>> > From: "Joe Woodward" <jw@terrafix.co.uk>
>> > To: "Paul Walmsley" <paul@pwsan.com>
>> > Cc: "Kevin Hilman" <khilman@ti.com>, "Raja\\, Govindraj"
>> <govindraj.raja@ti.com>, linux-omap@vger.kernel.org, "Felipe Balbi"
>> <balbi@ti.com>, neilb@suse.de
>> > Date: Thu, 29 Mar 2012 12:27:55 +0100
>> > Subject: Re: Suspend broken on 3.3?
>> >
>> >> > Hello Joe,
>> >> >
>> >> > thanks for reporting this.  Some thoughts -- really just pure
>> >> > speculation
>> >> > -- but I hope some of it might be useful for you...
>> >> >
>> >> > On Thu, 29 Mar 2012, Joe Woodward wrote:
>> >> >
>> >> > > After digging a bit further I found that the problem isn't lost
>> >> > characters or character corruption at all...
>> >> > >
>> >> > > The UART is actually at 430KBaud (not 900KBaud as I mentioned
>> >> > earlier).
>> >> >
>> >> > 430Kbps?  Could you confirm this?  OMAP UARTs don't support that
>> rate
>> >> > as
>> >> > far as I know - the closest is 460Kbps (OMAP34xx TRM vZR Table
>> 17-1
>> >> > "UART
>> >> > Mode Baud Rates, Divisor Values, and Error Rates).  If one was
>> >> > desperate,
>> >> > it might be possible to get 430Kbps by tweaking other parts of the
>> >> > clock
>> >> > tree though...
>> >>
>> >> Sorry for the confusion... It's 460KBaud - the 430 was just a typo
>> in
>> >> my previous mail...
>> >>
>> >> >
>> >> > > The data received is very bursty (i.e. sets of messages every
>> >> second
>> >> > or
>> >> > > so), containing a sync sequence to indicate a start of packet.
>> >> > >
>> >> > > The received bytes should be: 0x01, 0x52, 0x41 ....rest of
>> packet.
>> >> > >
>> >> > > This works 100% of the time on 3.2, but on 3.3 I sometimes (but
>> not
>> >> > always) get: 0x01, 0x00, 0x52, 0x41.
>> >> > >
>> >> > > i.e. there is a NULL/0x00 inserted after the first character.
>> >> >
>> >> > Is this on the serial console, or on a non-console serial port?
>> >> >
>> >> > Looking at drivers/tty/serial/omap-serial.c:receive_chars(), I
>> wonder
>> >> > if
>> >> > the driver is somehow reading bytes from the RX FIFO when it's
>> empty.
>> >>
>> >> > It's unclear to me how this could happen.  But you might want to
>> try
>> >> > doing
>> >> > an LSR read right before entering the loop in
>> >> > drivers/tty/serial/omap-serial.c:receive_chars().  In stock v3.3,
>> >> this
>> >> > would mean adding
>> >> >
>> >> >             lsr = serial_in(up, UART_LSR);
>> >> >
>> >> > at line 190 of drivers/tty/serial/omap-serial.c.
>> >> >
>> >>
>> >> Adding this is made no obvious difference.
>> >>
>> >> >
>> >> > You might also try the DMA path as an experiment.  This will
>> totally
>> >> > wedge
>> >> > power management due to an insanely low timer expiration in that
>> >> path,
>> >> > but
>> >> > at least might help narrow the problem down.  To do so with a
>> quick
>> >> > hack,
>> >> > you could set omap_serial_default_info.dma_enabled to true instead
>> of
>> >> > false at arch/arm/mach-omap2/serial.c:76.
>> >> >
>> >>
>> >> This did the trick (I've added it in addition to the LSR read above,
>> >> i'll back out the LSR read and see if it still works).
>> >> When DMA is enabled the behaviour (as seen from my application in
>> >> userspace) is the same as on the stock 3.2 kernel (i.e. for me it
>> works
>> >> :) ).
>> >>
>> >
>> > I've just realised that if anyone has joined this thread late, then
>> I'm running in a state with Govindraj's patch in a previous mail in
>> this thread applied to serial.c to
>> > fix the suspend issues due to the UART wakeup's not being correctly
>> changed from userspace via sysfs.
>> >
>> > It may actually by this patch that is causing the interrupt-enabled
>> serial driver to have broken? The tty that is causing me a problem does
>> have wake-from-suspend
>> > disabled for it from userspace...
>>
>> Is the patch shared earlier causing this issue of getting 0x00 from rx
>> randomly ?
>>
>
> In short, yes.
>
> I've gone back to a stock 3.3 kernel and the serial ports give the correct data, but suspend fails (due to not being able to disable wake-from-serial).
>
> I then applied your patch and could disable wakeup on the serial ports and suspend correctly, but the (non-console) serial port starts to misbehave.
>
> Forcing the driver to be DMA-enabled caused everything to work again. So something in the patch is causing the (default) interrupt-enabled serial driver to
> occassionally fail.
>

Disabling module level wakeup by default on bootup in previous shared patch
in serial.c in omap_uart_idle_init => omap_uart_disable_module_wakeup
might be causing this issue and probably this should be left enabled by default
and be disabled only when requested from sysfs on suspend.

Could you please try attached patch and let me know if this solves the
rx issue as well,
without using dma mode.

--
Thanks,
Govindraj.R

[-- Attachment #2: 0001-OMAP2-UART-Correct-the-module-level-wakeup-enable-di.patch --]
[-- Type: application/octet-stream, Size: 6654 bytes --]

From 6a5143110405ce397b5941b756041ecc6e1f0561 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.

Adding the support to handle module level wakeups for omap2/3 socs.

Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
 arch/arm/mach-omap2/serial.c                  |   88 ++++++++++++++++++++++++-
 arch/arm/plat-omap/include/plat/omap-serial.h |    3 +-
 drivers/tty/serial/omap-serial.c              |   15 +++-
 3 files changed, 99 insertions(+), 7 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/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
index 9ff4444..386a25b 100644
--- a/arch/arm/plat-omap/include/plat/omap-serial.h
+++ b/arch/arm/plat-omap/include/plat/omap-serial.h
@@ -130,7 +130,8 @@ struct uart_omap_port {
 	unsigned long		port_activity;
 	u32			context_loss_cnt;
 	u32			errata;
-	u8			wakeups_enabled;
+	u8			pad_wakeups_enabled;
+	u8			module_wakeups_enabled;
 
 	struct pm_qos_request	pm_qos_request;
 	u32			latency;
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 0121486..bb0a74d 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1458,6 +1458,11 @@ static int serial_omap_probe(struct platform_device *pdev)
 		up->uart_dma.rx_dma_channel = OMAP_UART_DMA_CH_FREE;
 	}
 
+	/* Module level wakeup will be left enabled
+	 * from bootloader.
+	 */
+	up->module_wakeups_enabled = true;
+
 	up->latency = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE;
 	up->calc_latency = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE;
 	pm_qos_add_request(&up->pm_qos_request,
@@ -1586,14 +1591,16 @@ static int serial_omap_runtime_suspend(struct device *dev)
 		up->context_loss_cnt = pdata->get_context_loss_count(dev);
 
 	if (device_may_wakeup(dev)) {
-		if (!up->wakeups_enabled) {
+		if (!up->pad_wakeups_enabled || !up->module_wakeups_enabled) {
 			pdata->enable_wakeup(up->pdev, true);
-			up->wakeups_enabled = true;
+			up->module_wakeups_enabled = true;
+			up->pad_wakeups_enabled = true;
 		}
 	} else {
-		if (up->wakeups_enabled) {
+		if (up->pad_wakeups_enabled || up->module_wakeups_enabled) {
 			pdata->enable_wakeup(up->pdev, false);
-			up->wakeups_enabled = false;
+			up->module_wakeups_enabled = false;
+			up->pad_wakeups_enabled = false;
 		}
 	}
 
-- 
1.7.5.4

Re: Suspend broken on 3.3?

[Joe Woodward]

...[snip]...
> 
> Could you please try attached patch and let me know if this solves the
> rx issue as well,
> without using dma mode.
> 

Right,

I think we've getting closer, but still not quite there...

Firstly, the patch adds an include to "iomap.h" - but this doesn't exist in stock 3.3. Simply removing this include seemed to allow the compile to complete 
successfully.

With this patch applied (and not in DMA mode) I now get the following:
  - If I leave wake-from-suspend enabled for the serial port then it works correctly (i.e. no lost/extra 0x00 characters).
  - If I disable wake-from-suspend for the serial port and go through a suspend cycle (i.e. suspend and then wake), then the serial port starts to misbehave as 
before.
  - If I then re-enable wake-from-suspend and go through a suspend cycle it starts to work correctly again.

So the problem is still that if wake-from-suspend is disabled for a serial port, and a suspend cycle is performed then when woken the serial port will not function 
correctly if operating in interrupt-mode.

Cheers,
Joe

> --
> Thanks,
> Govindraj.R

Re: Suspend broken on 3.3?

[Raja, Govindraj]

[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]

On Fri, Mar 30, 2012 at 2:56 PM, Joe Woodward <jw@terrafix.co.uk> wrote:
> ...[snip]...
>>
>> Could you please try attached patch and let me know if this solves the
>> rx issue as well,
>> without using dma mode.
>>
>
> Right,
>
> I think we've getting closer, but still not quite there...
>
> Firstly, the patch adds an include to "iomap.h" - but this doesn't exist in stock 3.3. Simply removing this include seemed to allow the compile to complete
> successfully.

Sorry I forgot to specify this is needed for latest mainline.

>
> With this patch applied (and not in DMA mode) I now get the following:
>  - If I leave wake-from-suspend enabled for the serial port then it works correctly (i.e. no lost/extra 0x00 characters).
>  - If I disable wake-from-suspend for the serial port and go through a suspend cycle (i.e. suspend and then wake), then the serial port starts to misbehave as
> before.
>  - If I then re-enable wake-from-suspend and go through a suspend cycle it starts to work correctly again.
>
> So the problem is still that if wake-from-suspend is disabled for a serial port, and a suspend cycle is performed then when woken the serial port will not function
> correctly if operating in interrupt-mode.

I tried it on my 3430sdp but strangely I don't see a 0x00 char read
after disabling uart wakeups
and waking up by keypad press.

Probably as a quick try we can try doing uart_insert_char only if data
was read from rx fifo
(Attached patch)

--
Thanks,
Govindraj.R

[-- Attachment #2: rx_fifo_check.patch --]
[-- Type: application/octet-stream, Size: 971 bytes --]

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 05620fb..13a15c6 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -206,10 +206,14 @@ static inline void receive_chars(struct uart_omap_port *up,
 	unsigned int flag, lsr = *status;
 	unsigned char ch = 0;
 	int max_count = 256;
+	u8 recd_ch = false;
 
 	do {
-		if (likely(lsr & UART_LSR_DR))
+		if (likely(lsr & UART_LSR_DR)) {
 			ch = serial_in(up, UART_RX);
+			recd_ch = true;
+		}
+
 		flag = TTY_NORMAL;
 		up->port.icount.rx++;
 
@@ -258,7 +262,9 @@ static inline void receive_chars(struct uart_omap_port *up,
 
 		if (uart_handle_sysrq_char(&up->port, ch))
 			goto ignore_char;
-		uart_insert_char(&up->port, lsr, UART_LSR_OE, ch, flag);
+
+		if (recd_ch)
+			uart_insert_char(&up->port, lsr, UART_LSR_OE, ch, flag);
 ignore_char:
 		lsr = serial_in(up, UART_LSR);
 	} while ((lsr & (UART_LSR_DR | UART_LSR_BI)) && (max_count-- > 0));

Re: Suspend broken on 3.3?

[Joe Woodward]

-----Original Message-----
From: "Raja, Govindraj" <govindraj.raja@ti.com>
To: Joe Woodward <jw@terrafix.co.uk>
Cc: Paul Walmsley <paul@pwsan.com>, Kevin Hilman <khilman@ti.com>, linux-omap@vger.kernel.org, Felipe Balbi <balbi@ti.com>, neilb@suse.de
Date: Fri, 30 Mar 2012 15:45:19 +0530
Subject: Re: Suspend broken on 3.3?

> On Fri, Mar 30, 2012 at 2:56 PM, Joe Woodward <jw@terrafix.co.uk>
> wrote:
> > ...[snip]...
> >>
> >> Could you please try attached patch and let me know if this solves
> the
> >> rx issue as well,
> >> without using dma mode.
> >>
> >
> > Right,
> >
> > I think we've getting closer, but still not quite there...
> >
> > Firstly, the patch adds an include to "iomap.h" - but this doesn't
> exist in stock 3.3. Simply removing this include seemed to allow the
> compile to complete
> > successfully.
> 
> Sorry I forgot to specify this is needed for latest mainline.
> 
> >
> > With this patch applied (and not in DMA mode) I now get the
> following:
> >  - If I leave wake-from-suspend enabled for the serial port then it
> works correctly (i.e. no lost/extra 0x00 characters).
> >  - If I disable wake-from-suspend for the serial port and go through
> a suspend cycle (i.e. suspend and then wake), then the serial port
> starts to misbehave as
> > before.
> >  - If I then re-enable wake-from-suspend and go through a suspend
> cycle it starts to work correctly again.
> >
> > So the problem is still that if wake-from-suspend is disabled for a
> serial port, and a suspend cycle is performed then when woken the
> serial port will not function
> > correctly if operating in interrupt-mode.
> 
> I tried it on my 3430sdp but strangely I don't see a 0x00 char read
> after disabling uart wakeups
> and waking up by keypad press.
> 
> Probably as a quick try we can try doing uart_insert_char only if data
> was read from rx fifo
> (Attached patch)
> 

Sadly the patch makes no difference.

I'm suprised you aren't seeing similar effects.

I've done more testing, and a simplified test case is as follows:
- take a stock 3.3 kernel and apply your patch to allow disabling of wake-from-suspend for the serial ports.
- disable wake-from-suspend for the console tty:
  echo disable > /sys/devices/platform/omap/omap_uart.2/power/wakeup
- perform a suspend (you'll need a button or something to wake you now that the console wont).
 echo mem > /sys/power/state

At this point the console is noticable/painfully slow. No characters are lost, but it's obvious something isn't right!

Cheers,
Joe

> --
> 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?

[Raja, Govindraj]

On Fri, Mar 30, 2012 at 4:34 PM, Joe Woodward <jw@terrafix.co.uk> wrote:
>
>
> -----Original Message-----
> From: "Raja, Govindraj" <govindraj.raja@ti.com>
> To: Joe Woodward <jw@terrafix.co.uk>
> Cc: Paul Walmsley <paul@pwsan.com>, Kevin Hilman <khilman@ti.com>, linux-omap@vger.kernel.org, Felipe Balbi <balbi@ti.com>, neilb@suse.de
> Date: Fri, 30 Mar 2012 15:45:19 +0530
> Subject: Re: Suspend broken on 3.3?
>
>> On Fri, Mar 30, 2012 at 2:56 PM, Joe Woodward <jw@terrafix.co.uk>
>> wrote:
>> > ...[snip]...
>> >>
>> >> Could you please try attached patch and let me know if this solves
>> the
>> >> rx issue as well,
>> >> without using dma mode.
>> >>
>> >
>> > Right,
>> >
>> > I think we've getting closer, but still not quite there...
>> >
>> > Firstly, the patch adds an include to "iomap.h" - but this doesn't
>> exist in stock 3.3. Simply removing this include seemed to allow the
>> compile to complete
>> > successfully.
>>
>> Sorry I forgot to specify this is needed for latest mainline.
>>
>> >
>> > With this patch applied (and not in DMA mode) I now get the
>> following:
>> >  - If I leave wake-from-suspend enabled for the serial port then it
>> works correctly (i.e. no lost/extra 0x00 characters).
>> >  - If I disable wake-from-suspend for the serial port and go through
>> a suspend cycle (i.e. suspend and then wake), then the serial port
>> starts to misbehave as
>> > before.
>> >  - If I then re-enable wake-from-suspend and go through a suspend
>> cycle it starts to work correctly again.
>> >
>> > So the problem is still that if wake-from-suspend is disabled for a
>> serial port, and a suspend cycle is performed then when woken the
>> serial port will not function
>> > correctly if operating in interrupt-mode.
>>
>> I tried it on my 3430sdp but strangely I don't see a 0x00 char read
>> after disabling uart wakeups
>> and waking up by keypad press.
>>
>> Probably as a quick try we can try doing uart_insert_char only if data
>> was read from rx fifo
>> (Attached patch)
>>
>
> Sadly the patch makes no difference.
>
> I'm suprised you aren't seeing similar effects.
>
> I've done more testing, and a simplified test case is as follows:
> - take a stock 3.3 kernel and apply your patch to allow disabling of wake-from-suspend for the serial ports.
> - disable wake-from-suspend for the console tty:
>  echo disable > /sys/devices/platform/omap/omap_uart.2/power/wakeup
> - perform a suspend (you'll need a button or something to wake you now that the console wont).
>  echo mem > /sys/power/state
>
> At this point the console is noticable/painfully slow. No characters are lost, but it's obvious something isn't right!


Yes I see this behavior with console now it becomes very slow after we
disable module level wakeups.

One difference to note is :

in 3.2 => full system PM is prevented in idle path if uart is active
i.e, MPU will not hit retention

in 3.3 => MPU will hit retention independently of uart is active or not.
             I think the way to get MPU qos for uart port activity
while in irq mode is to use PM_WKEN1
             reg settings, meaning enabling module level wakeup event
generation. Disabling it is causing this
             slow response.

Checking if we can workaround this scenario.

--
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?

[Raja, Govindraj]

On Fri, Mar 30, 2012 at 5:54 PM, Raja, Govindraj <govindraj.raja@ti.com> wrote:
> On Fri, Mar 30, 2012 at 4:34 PM, Joe Woodward <jw@terrafix.co.uk> wrote:
>>
>>
>> -----Original Message-----
>> From: "Raja, Govindraj" <govindraj.raja@ti.com>
>> To: Joe Woodward <jw@terrafix.co.uk>
>> Cc: Paul Walmsley <paul@pwsan.com>, Kevin Hilman <khilman@ti.com>, linux-omap@vger.kernel.org, Felipe Balbi <balbi@ti.com>, neilb@suse.de
>> Date: Fri, 30 Mar 2012 15:45:19 +0530
>> Subject: Re: Suspend broken on 3.3?
>>
>>> On Fri, Mar 30, 2012 at 2:56 PM, Joe Woodward <jw@terrafix.co.uk>
>>> wrote:
>>> > ...[snip]...
>>> >>
>>> >> Could you please try attached patch and let me know if this solves
>>> the
>>> >> rx issue as well,
>>> >> without using dma mode.
>>> >>
>>> >
>>> > Right,
>>> >
>>> > I think we've getting closer, but still not quite there...
>>> >
>>> > Firstly, the patch adds an include to "iomap.h" - but this doesn't
>>> exist in stock 3.3. Simply removing this include seemed to allow the
>>> compile to complete
>>> > successfully.
>>>
>>> Sorry I forgot to specify this is needed for latest mainline.
>>>
>>> >
>>> > With this patch applied (and not in DMA mode) I now get the
>>> following:
>>> >  - If I leave wake-from-suspend enabled for the serial port then it
>>> works correctly (i.e. no lost/extra 0x00 characters).
>>> >  - If I disable wake-from-suspend for the serial port and go through
>>> a suspend cycle (i.e. suspend and then wake), then the serial port
>>> starts to misbehave as
>>> > before.
>>> >  - If I then re-enable wake-from-suspend and go through a suspend
>>> cycle it starts to work correctly again.
>>> >
>>> > So the problem is still that if wake-from-suspend is disabled for a
>>> serial port, and a suspend cycle is performed then when woken the
>>> serial port will not function
>>> > correctly if operating in interrupt-mode.
>>>
>>> I tried it on my 3430sdp but strangely I don't see a 0x00 char read
>>> after disabling uart wakeups
>>> and waking up by keypad press.
>>>
>>> Probably as a quick try we can try doing uart_insert_char only if data
>>> was read from rx fifo
>>> (Attached patch)
>>>
>>
>> Sadly the patch makes no difference.
>>
>> I'm suprised you aren't seeing similar effects.
>>
>> I've done more testing, and a simplified test case is as follows:
>> - take a stock 3.3 kernel and apply your patch to allow disabling of wake-from-suspend for the serial ports.
>> - disable wake-from-suspend for the console tty:
>>  echo disable > /sys/devices/platform/omap/omap_uart.2/power/wakeup
>> - perform a suspend (you'll need a button or something to wake you now that the console wont).
>>  echo mem > /sys/power/state
>>
>> At this point the console is noticable/painfully slow. No characters are lost, but it's obvious something isn't right!
>
>
> Yes I see this behavior with console now it becomes very slow after we
> disable module level wakeups.
>
> One difference to note is :
>
> in 3.2 => full system PM is prevented in idle path if uart is active
> i.e, MPU will not hit retention
>
> in 3.3 => MPU will hit retention independently of uart is active or not.
>             I think the way to get MPU qos for uart port activity
> while in irq mode is to use PM_WKEN1
>             reg settings, meaning enabling module level wakeup event
> generation. Disabling it is causing this
>             slow response.
>
> Checking if we can workaround this scenario.

As of now what I can think of is to update qos reqest to prevent MPU
from transitioning in case of port activity if wakeup capability for port
is disabled.

--
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?

[Joe Woodward]

-----Original Message-----
From: "Raja, Govindraj" <govindraj.raja@ti.com>
To: Joe Woodward <jw@terrafix.co.uk>, Kevin Hilman <khilman@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>, linux-omap@vger.kernel.org, Felipe Balbi <balbi@ti.com>, neilb@suse.de
Date: Mon, 2 Apr 2012 16:13:13 +0530
Subject: Re: Suspend broken on 3.3?

> On Fri, Mar 30, 2012 at 5:54 PM, Raja, Govindraj
> <govindraj.raja@ti.com> wrote:
> > On Fri, Mar 30, 2012 at 4:34 PM, Joe Woodward <jw@terrafix.co.uk>
> wrote:
> >>
> >>
> >> -----Original Message-----
> >> From: "Raja, Govindraj" <govindraj.raja@ti.com>
> >> To: Joe Woodward <jw@terrafix.co.uk>
> >> Cc: Paul Walmsley <paul@pwsan.com>, Kevin Hilman <khilman@ti.com>,
> linux-omap@vger.kernel.org, Felipe Balbi <balbi@ti.com>, neilb@suse.de
> >> Date: Fri, 30 Mar 2012 15:45:19 +0530
> >> Subject: Re: Suspend broken on 3.3?
> >>
> >>> On Fri, Mar 30, 2012 at 2:56 PM, Joe Woodward <jw@terrafix.co.uk>
> >>> wrote:
> >>> > ...[snip]...
> >>> >>
> >>> >> Could you please try attached patch and let me know if this
> solves
> >>> the
> >>> >> rx issue as well,
> >>> >> without using dma mode.
> >>> >>
> >>> >
> >>> > Right,
> >>> >
> >>> > I think we've getting closer, but still not quite there...
> >>> >
> >>> > Firstly, the patch adds an include to "iomap.h" - but this
> doesn't
> >>> exist in stock 3.3. Simply removing this include seemed to allow
> the
> >>> compile to complete
> >>> > successfully.
> >>>
> >>> Sorry I forgot to specify this is needed for latest mainline.
> >>>
> >>> >
> >>> > With this patch applied (and not in DMA mode) I now get the
> >>> following:
> >>> >  - If I leave wake-from-suspend enabled for the serial port then
> it
> >>> works correctly (i.e. no lost/extra 0x00 characters).
> >>> >  - If I disable wake-from-suspend for the serial port and go
> through
> >>> a suspend cycle (i.e. suspend and then wake), then the serial port
> >>> starts to misbehave as
> >>> > before.
> >>> >  - If I then re-enable wake-from-suspend and go through a
> suspend
> >>> cycle it starts to work correctly again.
> >>> >
> >>> > So the problem is still that if wake-from-suspend is disabled for
> a
> >>> serial port, and a suspend cycle is performed then when woken the
> >>> serial port will not function
> >>> > correctly if operating in interrupt-mode.
> >>>
> >>> I tried it on my 3430sdp but strangely I don't see a 0x00 char read
> >>> after disabling uart wakeups
> >>> and waking up by keypad press.
> >>>
> >>> Probably as a quick try we can try doing uart_insert_char only if
> data
> >>> was read from rx fifo
> >>> (Attached patch)
> >>>
> >>
> >> Sadly the patch makes no difference.
> >>
> >> I'm suprised you aren't seeing similar effects.
> >>
> >> I've done more testing, and a simplified test case is as follows:
> >> - take a stock 3.3 kernel and apply your patch to allow disabling of
> wake-from-suspend for the serial ports.
> >> - disable wake-from-suspend for the console tty:
> >>  echo disable > /sys/devices/platform/omap/omap_uart.2/power/wakeup
> >> - perform a suspend (you'll need a button or something to wake you
> now that the console wont).
> >>  echo mem > /sys/power/state
> >>
> >> At this point the console is noticable/painfully slow. No characters
> are lost, but it's obvious something isn't right!
> >
> >
> > Yes I see this behavior with console now it becomes very slow after
> we
> > disable module level wakeups.
> >
> > One difference to note is :
> >
> > in 3.2 => full system PM is prevented in idle path if uart is active
> > i.e, MPU will not hit retention
> >
> > in 3.3 => MPU will hit retention independently of uart is active or
> not.
> >             I think the way to get MPU qos for uart port
> activity
> > while in irq mode is to use PM_WKEN1
> >             reg settings, meaning enabling module level wakeup
> event
> > generation. Disabling it is causing this
> >             slow response.
> >
> > Checking if we can workaround this scenario.
> 
> As of now what I can think of is to update qos reqest to prevent MPU
> from transitioning in case of port activity if wakeup capability for
> port
> is disabled.
> 

Does that get us back to the same behaviour as in 3.2? If thats the best we can do, then I guess that'll have to do.

Will similar problems exist when in DMA-mode (as we I set the DMA flag it seemed to work ok)?

It does seem a little strange from my naive point of view: surely what peripherals/pins are used to wake the device from suspend should not affect how the device 
chooses to enable/disable clocks/power-domains to save power when running?

Cheers,
Joe

> --
> 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


--
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?

[Govindraj]

On Mon, Apr 2, 2012 at 6:07 PM, Joe Woodward <jw@terrafix.co.uk> wrote:
>
>
> -----Original Message-----
> From: "Raja, Govindraj" <govindraj.raja@ti.com>
> To: Joe Woodward <jw@terrafix.co.uk>, Kevin Hilman <khilman@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>, linux-omap@vger.kernel.org, Felipe Balbi <balbi@ti.com>, neilb@suse.de
> Date: Mon, 2 Apr 2012 16:13:13 +0530
> Subject: Re: Suspend broken on 3.3?
>
>> On Fri, Mar 30, 2012 at 5:54 PM, Raja, Govindraj
>> <govindraj.raja@ti.com> wrote:
>> > On Fri, Mar 30, 2012 at 4:34 PM, Joe Woodward <jw@terrafix.co.uk>
>> wrote:
>> >>
>> >>
>> >> -----Original Message-----
>> >> From: "Raja, Govindraj" <govindraj.raja@ti.com>
>> >> To: Joe Woodward <jw@terrafix.co.uk>
>> >> Cc: Paul Walmsley <paul@pwsan.com>, Kevin Hilman <khilman@ti.com>,
>> linux-omap@vger.kernel.org, Felipe Balbi <balbi@ti.com>, neilb@suse.de
>> >> Date: Fri, 30 Mar 2012 15:45:19 +0530
>> >> Subject: Re: Suspend broken on 3.3?
>> >>
>> >>> On Fri, Mar 30, 2012 at 2:56 PM, Joe Woodward <jw@terrafix.co.uk>
>> >>> wrote:
>> >>> > ...[snip]...
>> >>> >>
>> >>> >> Could you please try attached patch and let me know if this
>> solves
>> >>> the
>> >>> >> rx issue as well,
>> >>> >> without using dma mode.
>> >>> >>
>> >>> >
>> >>> > Right,
>> >>> >
>> >>> > I think we've getting closer, but still not quite there...
>> >>> >
>> >>> > Firstly, the patch adds an include to "iomap.h" - but this
>> doesn't
>> >>> exist in stock 3.3. Simply removing this include seemed to allow
>> the
>> >>> compile to complete
>> >>> > successfully.
>> >>>
>> >>> Sorry I forgot to specify this is needed for latest mainline.
>> >>>
>> >>> >
>> >>> > With this patch applied (and not in DMA mode) I now get the
>> >>> following:
>> >>> >  - If I leave wake-from-suspend enabled for the serial port then
>> it
>> >>> works correctly (i.e. no lost/extra 0x00 characters).
>> >>> >  - If I disable wake-from-suspend for the serial port and go
>> through
>> >>> a suspend cycle (i.e. suspend and then wake), then the serial port
>> >>> starts to misbehave as
>> >>> > before.
>> >>> >  - If I then re-enable wake-from-suspend and go through a
>> suspend
>> >>> cycle it starts to work correctly again.
>> >>> >
>> >>> > So the problem is still that if wake-from-suspend is disabled for
>> a
>> >>> serial port, and a suspend cycle is performed then when woken the
>> >>> serial port will not function
>> >>> > correctly if operating in interrupt-mode.
>> >>>
>> >>> I tried it on my 3430sdp but strangely I don't see a 0x00 char read
>> >>> after disabling uart wakeups
>> >>> and waking up by keypad press.
>> >>>
>> >>> Probably as a quick try we can try doing uart_insert_char only if
>> data
>> >>> was read from rx fifo
>> >>> (Attached patch)
>> >>>
>> >>
>> >> Sadly the patch makes no difference.
>> >>
>> >> I'm suprised you aren't seeing similar effects.
>> >>
>> >> I've done more testing, and a simplified test case is as follows:
>> >> - take a stock 3.3 kernel and apply your patch to allow disabling of
>> wake-from-suspend for the serial ports.
>> >> - disable wake-from-suspend for the console tty:
>> >>  echo disable > /sys/devices/platform/omap/omap_uart.2/power/wakeup
>> >> - perform a suspend (you'll need a button or something to wake you
>> now that the console wont).
>> >>  echo mem > /sys/power/state
>> >>
>> >> At this point the console is noticable/painfully slow. No characters
>> are lost, but it's obvious something isn't right!
>> >
>> >
>> > Yes I see this behavior with console now it becomes very slow after
>> we
>> > disable module level wakeups.
>> >
>> > One difference to note is :
>> >
>> > in 3.2 => full system PM is prevented in idle path if uart is active
>> > i.e, MPU will not hit retention
>> >
>> > in 3.3 => MPU will hit retention independently of uart is active or
>> not.
>> >             I think the way to get MPU qos for uart port
>> activity
>> > while in irq mode is to use PM_WKEN1
>> >             reg settings, meaning enabling module level wakeup
>> event
>> > generation. Disabling it is causing this
>> >             slow response.
>> >
>> > Checking if we can workaround this scenario.
>>
>> As of now what I can think of is to update qos reqest to prevent MPU
>> from transitioning in case of port activity if wakeup capability for
>> port
>> is disabled.
>>
>
> Does that get us back to the same behaviour as in 3.2? If thats the best we can do, then I guess that'll have to do.
>

yes, only if we are in non-dma mode with wakeup disabled.


> Will similar problems exist when in DMA-mode (as we I set the DMA flag it seemed to work ok)?

In dma mode we might not need MPU for uart fifo ops and while in irq
mode we need MPU
for fifo ops, while mpu is hitting low power states way to get mpu qos
for uart ops
is by generating module level wakeup events, if module level wakeups
are disabled
and if uart does relies on mpu for fifo ops we have to prevent MPU
from hitting low power
states.

>
> It does seem a little strange from my naive point of view: surely what peripherals/pins are used to wake the device from suspend should not affect how the device
> chooses to enable/disable clocks/power-domains to save power when running?
>
--
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?

[Paul Walmsley]

Hello Govindraj

On Mon, 2 Apr 2012, Raja, Govindraj wrote:

> As of now what I can think of is to update qos reqest to prevent MPU
> from transitioning in case of port activity if wakeup capability for port
> is disabled.

Haven't been following this thread closely, but this doesn't seem right.  
Why would changing the UART driver's ability to wake from suspend via the 
sysfs file prevent the driver from using hardware wakeups to wake from 
dynamic idle?


- Paul

Re: Suspend broken on 3.3?

[Raja, Govindraj]

On Wed, Apr 4, 2012 at 8:26 PM, Paul Walmsley <paul@pwsan.com> wrote:
> Hello Govindraj
>
> On Mon, 2 Apr 2012, Raja, Govindraj wrote:
>
>> As of now what I can think of is to update qos reqest to prevent MPU
>> from transitioning in case of port activity if wakeup capability for port
>> is disabled.
>
> Haven't been following this thread closely, but this doesn't seem right.
> Why would changing the UART driver's ability to wake from suspend via the
> sysfs file prevent the driver from using hardware wakeups to wake from
> dynamic idle?


IIUC wakeup capability is needed in suspend path or in cpu idle path.

once uart wakeup capability is disabled from sysfs the Module level wakeup
from PM_WKEN1 reg on omap3 and pad wakeup from pin mux data given
will be disabled..

So after we enter suspend and wakeup from suspend using keypad press,
now through pm workqueue the MPU enters retention and uart module level
wakeups disabled at this point console becomes slower to respond.

Now enabling wakeups from sysfs enter suspend/resume to enable wakeups
and once we come back from wakeup console response is better.

So disabling uart module level wake up and allowing MPU to enter retention
is making console slow.

--
Thanks,
Govindraj.R

Re: Suspend broken on 3.3?

[Paul Walmsley]

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

> On Wed, Apr 4, 2012 at 8:26 PM, Paul Walmsley <paul@pwsan.com> wrote:
> > On Mon, 2 Apr 2012, Raja, Govindraj wrote:
> >
> >> As of now what I can think of is to update qos reqest to prevent MPU
> >> from transitioning in case of port activity if wakeup capability for port
> >> is disabled.
> >
> > Haven't been following this thread closely, but this doesn't seem right.
> > Why would changing the UART driver's ability to wake from suspend via the
> > sysfs file prevent the driver from using hardware wakeups to wake from
> > dynamic idle?
> 
> 
> IIUC wakeup capability is needed in suspend path or in cpu idle path.
> 
> once uart wakeup capability is disabled from sysfs the Module level 
> wakeup from PM_WKEN1 reg on omap3 and pad wakeup from pin mux data given 
> will be disabled..

As far as I know, that sysfs wakeup file is not meant to disable 
all module-level wakeup.  Kevin can correct me if I'm wrong, but I think
it's only supposed to control wakeup from suspend.

So in my opinion, that sysfs file should not affect dynamic 
module-level, or I/O ring wakeup at all.  If it is intended to affect 
dynamic idle wakeup, then we also will need code to prevent the UART from 
disabling its clocks when the sysfs wakeup file is set to disabled.

> So after we enter suspend and wakeup from suspend using keypad press,
> now through pm workqueue the MPU enters retention and uart module level
> wakeups disabled at this point console becomes slower to respond.
> 
> Now enabling wakeups from sysfs enter suspend/resume to enable wakeups
> and once we come back from wakeup console response is better.
> 
> So disabling uart module level wake up and allowing MPU to enter retention
> is making console slow.


- Paul

Re: Suspend broken on 3.3?

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> On Wed, 4 Apr 2012, Raja, Govindraj wrote:
>
>> On Wed, Apr 4, 2012 at 8:26 PM, Paul Walmsley <paul@pwsan.com> wrote:
>> > On Mon, 2 Apr 2012, Raja, Govindraj wrote:
>> >
>> >> As of now what I can think of is to update qos reqest to prevent MPU
>> >> from transitioning in case of port activity if wakeup capability for port
>> >> is disabled.
>> >
>> > Haven't been following this thread closely, but this doesn't seem right.
>> > Why would changing the UART driver's ability to wake from suspend via the
>> > sysfs file prevent the driver from using hardware wakeups to wake from
>> > dynamic idle?
>> 
>> 
>> IIUC wakeup capability is needed in suspend path or in cpu idle path.
>> 
>> once uart wakeup capability is disabled from sysfs the Module level 
>> wakeup from PM_WKEN1 reg on omap3 and pad wakeup from pin mux data given 
>> will be disabled..
>
> As far as I know, that sysfs wakeup file is not meant to disable 
> all module-level wakeup.  Kevin can correct me if I'm wrong, but I think
> it's only supposed to control wakeup from suspend.

In theory, sysfs control is meant for static suspend.  ISTR though that
we were using it for idle as well so there weren't unncessary UART
wakeups from idle on UART activity that was not serial console.

Kevin

Re: Suspend broken on 3.3?

[Raja, Govindraj]

Hi Kevin / Paul,

On Fri, Apr 6, 2012 at 7:51 PM, Kevin Hilman <khilman@ti.com> wrote:
> Paul Walmsley <paul@pwsan.com> writes:
>
>> On Wed, 4 Apr 2012, Raja, Govindraj wrote:
>>
>>> On Wed, Apr 4, 2012 at 8:26 PM, Paul Walmsley <paul@pwsan.com> wrote:
>>> > On Mon, 2 Apr 2012, Raja, Govindraj wrote:
>>> >
>>> >> As of now what I can think of is to update qos reqest to prevent MPU
>>> >> from transitioning in case of port activity if wakeup capability for port
>>> >> is disabled.
>>> >
>>> > Haven't been following this thread closely, but this doesn't seem right.
>>> > Why would changing the UART driver's ability to wake from suspend via the
>>> > sysfs file prevent the driver from using hardware wakeups to wake from
>>> > dynamic idle?
>>>
>>>
>>> IIUC wakeup capability is needed in suspend path or in cpu idle path.
>>>
>>> once uart wakeup capability is disabled from sysfs the Module level
>>> wakeup from PM_WKEN1 reg on omap3 and pad wakeup from pin mux data given
>>> will be disabled..
>>
>> As far as I know, that sysfs wakeup file is not meant to disable
>> all module-level wakeup.  Kevin can correct me if I'm wrong, but I think
>> it's only supposed to control wakeup from suspend.
>
> In theory, sysfs control is meant for static suspend.  ISTR though that
> we were using it for idle as well so there weren't unncessary UART
> wakeups from idle on UART activity that was not serial console.

So incase of uart wakeups are disabled and uart tx/rx is requested
can we prevent MPU from low power state.

--
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:

[...]

> So incase of uart wakeups are disabled and uart tx/rx is requested
> can we prevent MPU from low power state.

I think that would be a mistake.

The main point of disabling UART wakeups is to save power by preventing
unwanted wakups on UARTs that don't need/want them.  If we then leave
MPU enabled because UART wakeups are disabled, that would defeat the
power-saving goals of disabling wakeups in the first place.

Presumably, if a user disables UART wakeups, it means either 1) that
UART is not used or 2) performance is not critical.

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.

Kevin

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