Hi,
This patch series contains fixes and cleanups around the issue that
the console UART should not idled on suspend while using "no_console_suspend"
in bootargs.
The approach thought of is to modify the serial core/serial driver to bypass
runtime PM if the UART in contention is a console and we are using "no_console_suspend"
in our bootargs.
While fixing the above issue, there are other cleanups also done as part of
this series which are no longer required. This cleanups mainly include getting
rid of using "omap_device_disable_idle_on_suspend" api for both dt and non dt case
as the serial driver will be self sufficient to handle the "no_idle_on_suspend" issue.
Serial was the only one making use of "omap_device_disable_idle_on_suspend"
There were discussions about how to handle "no_idle_on_suspend" issue and all the
discussions are as follows:
https://lkml.org/lkml/2013/4/5/239
https://lkml.org/lkml/2013/4/2/350
https://lkml.org/lkml/2013/3/18/199
https://lkml.org/lkml/2013/3/18/295
Due to the amount of change in approach and other cleanups coming around it, I am posting
this as a new series.
v3->v4
1. check for console in runtime api.
2. Prevent setting od->flags to OMAP_DEVICE_SUSPENDED
v2->v3
1. Use "-EBUSY" for no_console_suspend case
2. Bypass runtime PM only during suspend
3. Improve the commit log based on community suggestion.
v1->v2
1. Remove the prepare/complete callback.
2. Adapt runtime PM callback to deal with the issue.
3. Fold patch(1,2) of previous series into 1.
4. Reordered the patch.
5. Change $subject and chage log for few patches.
Cc: Santosh Shilimkar <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Rajendra nayak <[email protected]>
Test info
Omap4430sdp:
- Tested wakeup from UART after suspend for dt and non dt case.
Omap5430evm:
- Tested wakeup from UART after suspend for dt case.
This patches are based on 3.9-rc8
The following changes since commit 824282ca7d250bd7c301f221c3cd902ce906d731:
Merge branch 'upstream' of git://git.linux-mips.org/pub/scm/ralf/upstream-linus (2013-04-22 15:00:59 -0700)
are available in the git repository at:
git://gitorious.org/linux-connectivity/linux-connectivity.git serial_omap_fix_cleanup
Sourav Poddar (5):
driver: tty: serial: Move "uart_console" def to core header file.
driver: serial: omap: prevent runtime PM for "no_console_suspend"
arm: omap2+: serial: remove no_console_suspend support
arm: dts: am33xx: Remove "ti,no_idle_on_suspend" property.
arm: omap2+: omap_device: remove no_idle_on_suspend
arch/arm/boot/dts/am33xx.dtsi | 1 -
arch/arm/mach-omap2/omap_device.c | 9 ++-------
arch/arm/mach-omap2/omap_device.h | 10 ----------
arch/arm/mach-omap2/serial.c | 7 -------
drivers/tty/serial/mpc52xx_uart.c | 10 ----------
drivers/tty/serial/omap-serial.c | 28 +++++++++++++++++++++++++++-
drivers/tty/serial/serial_core.c | 6 ------
include/linux/serial_core.h | 7 +++++++
8 files changed, 36 insertions(+), 42 deletions(-)
Move "uart_console" definition to serial core header file, so that it can be
used by serial drivers.
Get rid of the uart_console defintion from mpc52xx_uart driver.
Cc: Santosh Shilimkar <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Rajendra nayak <[email protected]>
Signed-off-by: Sourav Poddar <[email protected]>
Reviewed-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/mpc52xx_uart.c | 10 ----------
drivers/tty/serial/serial_core.c | 6 ------
include/linux/serial_core.h | 7 +++++++
3 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/drivers/tty/serial/mpc52xx_uart.c b/drivers/tty/serial/mpc52xx_uart.c
index 018bad9..d74ac06 100644
--- a/drivers/tty/serial/mpc52xx_uart.c
+++ b/drivers/tty/serial/mpc52xx_uart.c
@@ -84,16 +84,6 @@ static void mpc52xx_uart_of_enumerate(void);
static irqreturn_t mpc52xx_uart_int(int irq, void *dev_id);
static irqreturn_t mpc5xxx_uart_process_int(struct uart_port *port);
-
-/* Simple macro to test if a port is console or not. This one is taken
- * for serial_core.c and maybe should be moved to serial_core.h ? */
-#ifdef CONFIG_SERIAL_CORE_CONSOLE
-#define uart_console(port) \
- ((port)->cons && (port)->cons->index == (port)->line)
-#else
-#define uart_console(port) (0)
-#endif
-
/* ======================================================================== */
/* PSC fifo operations for isolating differences between 52xx and 512x */
/* ======================================================================== */
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index a400002..b5f74bf 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -50,12 +50,6 @@ static struct lock_class_key port_lock_key;
#define HIGH_BITS_OFFSET ((sizeof(long)-sizeof(int))*8)
-#ifdef CONFIG_SERIAL_CORE_CONSOLE
-#define uart_console(port) ((port)->cons && (port)->cons->index == (port)->line)
-#else
-#define uart_console(port) (0)
-#endif
-
static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
struct ktermios *old_termios);
static void uart_wait_until_sent(struct tty_struct *tty, int timeout);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 87d4bbc..b98291a 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -31,6 +31,13 @@
#include <linux/sysrq.h>
#include <uapi/linux/serial_core.h>
+#ifdef CONFIG_SERIAL_CORE_CONSOLE
+#define uart_console(port) \
+ ((port)->cons && (port)->cons->index == (port)->line)
+#else
+#define uart_console(port) (0)
+#endif
+
struct uart_port;
struct serial_struct;
struct device;
--
1.7.1
"no_console_suspend" is no longer handled in platform file,
Since the omap serial driver is now adapted to prevent
console UART idleing during suspend.
Cc: Santosh Shilimkar <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Rajendra nayak <[email protected]>
Signed-off-by: Sourav Poddar <[email protected]>
Reviewed-by: Felipe Balbi <[email protected]>
---
arch/arm/mach-omap2/serial.c | 7 -------
1 files changed, 0 insertions(+), 7 deletions(-)
diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 8396b5b..25fb6e9 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -63,7 +63,6 @@ struct omap_uart_state {
static LIST_HEAD(uart_list);
static u8 num_uarts;
static u8 console_uart_id = -1;
-static u8 no_console_suspend;
static u8 uart_debug;
#define DEFAULT_RXDMA_POLLRATE 1 /* RX DMA polling rate (us) */
@@ -236,9 +235,6 @@ static int __init omap_serial_early_init(void)
uart_name, uart->num);
}
- if (cmdline_find_option("no_console_suspend"))
- no_console_suspend = true;
-
/*
* omap-uart can be used for earlyprintk logs
* So if omap-uart is used as console then prevent
@@ -323,9 +319,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
return;
}
- if ((console_uart_id == bdata->id) && no_console_suspend)
- omap_device_disable_idle_on_suspend(pdev);
-
oh->mux = omap_hwmod_mux_init(bdata->pads, bdata->pads_cnt);
if (console_uart_id == bdata->id) {
--
1.7.1
The "ti,no_idle_on_suspend" property was required to keep ocmcram clocks
running during idle.
But commit 72bb6f9 (ARM: OMAP: omap_device: don't attempt late suspend
if no driver bound), added in v3.6 should prevent any automatic clock
gating for devices without drivers bound. Since there is no driver for
the OCM RAM block, we are not affected by the automatic idle on suspend
anyways which means "ti,no_idle_on_suspend" can be safely removed since
there are no users for it.
Cc: Benoit Cousson <[email protected]>
Cc: Santosh Shilimkar <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Rajendra nayak <[email protected]>
Signed-off-by: Sourav Poddar <[email protected]>
Reviewed-by: Felipe Balbi <[email protected]>
---
arch/arm/boot/dts/am33xx.dtsi | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 0957645..729ca24 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -390,7 +390,6 @@
compatible = "ti,am3352-ocmcram";
reg = <0x40300000 0x10000>;
ti,hwmods = "ocmcram";
- ti,no_idle_on_suspend;
};
wkup_m3: wkup_m3@44d00000 {
--
1.7.1
The driver manages "no_console_suspend" by preventing runtime PM
during the suspend path, which forces the console UART to stay awake.
Signed-off-by: Sourav Poddar <[email protected]>
Reviewed-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 28 +++++++++++++++++++++++++++-
1 files changed, 27 insertions(+), 1 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 30d4f7a..eddff3c 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -161,6 +161,7 @@ struct uart_omap_port {
u32 calc_latency;
struct work_struct qos_work;
struct pinctrl *pins;
+ bool is_suspending;
};
#define to_uart_omap_port(p) ((container_of((p), struct uart_omap_port, port)))
@@ -1312,6 +1313,22 @@ static struct uart_driver serial_omap_reg = {
};
#ifdef CONFIG_PM_SLEEP
+static int serial_omap_prepare(struct device *dev)
+{
+ struct uart_omap_port *up = dev_get_drvdata(dev);
+
+ up->is_suspending = true;
+
+ return 0;
+}
+
+static void serial_omap_complete(struct device *dev)
+{
+ struct uart_omap_port *up = dev_get_drvdata(dev);
+
+ up->is_suspending = false;
+}
+
static int serial_omap_suspend(struct device *dev)
{
struct uart_omap_port *up = dev_get_drvdata(dev);
@@ -1330,7 +1347,10 @@ static int serial_omap_resume(struct device *dev)
return 0;
}
-#endif
+#else
+#define serial_omap_prepare NULL
+#define serial_omap_prepare NULL
+#endif /* CONFIG_PM_SLEEP */
static void omap_serial_fill_features_erratas(struct uart_omap_port *up)
{
@@ -1616,6 +1636,10 @@ static int serial_omap_runtime_suspend(struct device *dev)
struct uart_omap_port *up = dev_get_drvdata(dev);
struct omap_uart_port_info *pdata = dev->platform_data;
+ if (up->is_suspending && !console_suspend_enabled
+ && uart_console(&up->port))
+ return -EBUSY;
+
if (!up)
return -EINVAL;
@@ -1666,6 +1690,8 @@ static const struct dev_pm_ops serial_omap_dev_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(serial_omap_suspend, serial_omap_resume)
SET_RUNTIME_PM_OPS(serial_omap_runtime_suspend,
serial_omap_runtime_resume, NULL)
+ .prepare = serial_omap_prepare,
+ .complete = serial_omap_complete,
};
#if defined(CONFIG_OF)
--
1.7.1
Remove "no_idle_on_suspend" check, since respective
driver should be able to prevent idling of an
omap device whenever required.
Driver's can get same behavior by just returning -EBUSY
from their ->runtime_suspend only during suspend.
Cc: Santosh Shilimkar <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Rajendra nayak <[email protected]>
Cc: Grygorii Strashko <[email protected]>
Signed-off-by: Sourav Poddar <[email protected]>
Reviewed-by: Felipe Balbi <[email protected]>
---
arch/arm/mach-omap2/omap_device.c | 9 ++-------
arch/arm/mach-omap2/omap_device.h | 10 ----------
2 files changed, 2 insertions(+), 17 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index 381be7a..7dc8551 100644
--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -170,9 +170,6 @@ static int omap_device_build_from_dt(struct platform_device *pdev)
r->name = dev_name(&pdev->dev);
}
- if (of_get_property(node, "ti,no_idle_on_suspend", NULL))
- omap_device_disable_idle_on_suspend(pdev);
-
pdev->dev.pm_domain = &omap_device_pm_domain;
odbfd_exit1:
@@ -621,8 +618,7 @@ static int _od_suspend_noirq(struct device *dev)
if (!ret && !pm_runtime_status_suspended(dev)) {
if (pm_generic_runtime_suspend(dev) == 0) {
- if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
- omap_device_idle(pdev);
+ omap_device_idle(pdev);
od->flags |= OMAP_DEVICE_SUSPENDED;
}
}
@@ -638,8 +634,7 @@ static int _od_resume_noirq(struct device *dev)
if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
!pm_runtime_status_suspended(dev)) {
od->flags &= ~OMAP_DEVICE_SUSPENDED;
- if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
- omap_device_enable(pdev);
+ omap_device_enable(pdev);
pm_generic_runtime_resume(dev);
}
diff --git a/arch/arm/mach-omap2/omap_device.h b/arch/arm/mach-omap2/omap_device.h
index 044c31d..17ca1ae 100644
--- a/arch/arm/mach-omap2/omap_device.h
+++ b/arch/arm/mach-omap2/omap_device.h
@@ -38,7 +38,6 @@ extern struct dev_pm_domain omap_device_pm_domain;
/* omap_device.flags values */
#define OMAP_DEVICE_SUSPENDED BIT(0)
-#define OMAP_DEVICE_NO_IDLE_ON_SUSPEND BIT(1)
/**
* struct omap_device - omap_device wrapper for platform_devices
@@ -101,13 +100,4 @@ static inline struct omap_device *to_omap_device(struct platform_device *pdev)
{
return pdev ? pdev->archdata.od : NULL;
}
-
-static inline
-void omap_device_disable_idle_on_suspend(struct platform_device *pdev)
-{
- struct omap_device *od = to_omap_device(pdev);
-
- od->flags |= OMAP_DEVICE_NO_IDLE_ON_SUSPEND;
-}
-
#endif
--
1.7.1
Sourav Poddar <[email protected]> writes:
> The driver manages "no_console_suspend" by preventing runtime PM
> during the suspend path, which forces the console UART to stay awake.
>
> Signed-off-by: Sourav Poddar <[email protected]>
> Reviewed-by: Felipe Balbi <[email protected]>
> ---
> drivers/tty/serial/omap-serial.c | 28 +++++++++++++++++++++++++++-
> 1 files changed, 27 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 30d4f7a..eddff3c 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -161,6 +161,7 @@ struct uart_omap_port {
> u32 calc_latency;
> struct work_struct qos_work;
> struct pinctrl *pins;
> + bool is_suspending;
> };
>
> #define to_uart_omap_port(p) ((container_of((p), struct uart_omap_port, port)))
> @@ -1312,6 +1313,22 @@ static struct uart_driver serial_omap_reg = {
> };
>
> #ifdef CONFIG_PM_SLEEP
> +static int serial_omap_prepare(struct device *dev)
> +{
> + struct uart_omap_port *up = dev_get_drvdata(dev);
> +
> + up->is_suspending = true;
> +
> + return 0;
> +}
> +
> +static void serial_omap_complete(struct device *dev)
> +{
> + struct uart_omap_port *up = dev_get_drvdata(dev);
> +
> + up->is_suspending = false;
> +}
> +
> static int serial_omap_suspend(struct device *dev)
> {
> struct uart_omap_port *up = dev_get_drvdata(dev);
> @@ -1330,7 +1347,10 @@ static int serial_omap_resume(struct device *dev)
>
> return 0;
> }
> -#endif
> +#else
> +#define serial_omap_prepare NULL
> +#define serial_omap_prepare NULL
> +#endif /* CONFIG_PM_SLEEP */
>
> static void omap_serial_fill_features_erratas(struct uart_omap_port *up)
> {
> @@ -1616,6 +1636,10 @@ static int serial_omap_runtime_suspend(struct device *dev)
> struct uart_omap_port *up = dev_get_drvdata(dev);
> struct omap_uart_port_info *pdata = dev->platform_data;
>
I'd like to see a comment here about why we return error. It's written
in the changelog, but it deserves a comment here as well. Something like:
When using 'no_console_suspend', the console UART must not be suspended.
Since driver suspend is managed by runtime suspend, preventing runtime
suspend (by returning error) will keep device active during suspend.
> + if (up->is_suspending && !console_suspend_enabled
> + && uart_console(&up->port))
nit: can you put the '&&' at the end of the first line, and have the
2nd line line up with the start of the first line?
See other examples of similiar multi-line checks elsewhere in the
driver.
Other than those minor things, this patch is good.
Then, can you repost and break this into 2 series since they can go
independently.
Patches 1-2 need to go through the serial tree (Greg), and I will queue
up the OMAP specific changes once Greg has queued the serial core changes.
Kevin
Hi Kevin,
On Friday 26 April 2013 11:58 PM, Kevin Hilman wrote:
> Sourav Poddar<[email protected]> writes:
>
>> The driver manages "no_console_suspend" by preventing runtime PM
>> during the suspend path, which forces the console UART to stay awake.
>>
>> Signed-off-by: Sourav Poddar<[email protected]>
>> Reviewed-by: Felipe Balbi<[email protected]>
>> ---
>> drivers/tty/serial/omap-serial.c | 28 +++++++++++++++++++++++++++-
>> 1 files changed, 27 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index 30d4f7a..eddff3c 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -161,6 +161,7 @@ struct uart_omap_port {
>> u32 calc_latency;
>> struct work_struct qos_work;
>> struct pinctrl *pins;
>> + bool is_suspending;
>> };
>>
>> #define to_uart_omap_port(p) ((container_of((p), struct uart_omap_port, port)))
>> @@ -1312,6 +1313,22 @@ static struct uart_driver serial_omap_reg = {
>> };
>>
>> #ifdef CONFIG_PM_SLEEP
>> +static int serial_omap_prepare(struct device *dev)
>> +{
>> + struct uart_omap_port *up = dev_get_drvdata(dev);
>> +
>> + up->is_suspending = true;
>> +
>> + return 0;
>> +}
>> +
>> +static void serial_omap_complete(struct device *dev)
>> +{
>> + struct uart_omap_port *up = dev_get_drvdata(dev);
>> +
>> + up->is_suspending = false;
>> +}
>> +
>> static int serial_omap_suspend(struct device *dev)
>> {
>> struct uart_omap_port *up = dev_get_drvdata(dev);
>> @@ -1330,7 +1347,10 @@ static int serial_omap_resume(struct device *dev)
>>
>> return 0;
>> }
>> -#endif
>> +#else
>> +#define serial_omap_prepare NULL
>> +#define serial_omap_prepare NULL
>> +#endif /* CONFIG_PM_SLEEP */
>>
>> static void omap_serial_fill_features_erratas(struct uart_omap_port *up)
>> {
>> @@ -1616,6 +1636,10 @@ static int serial_omap_runtime_suspend(struct device *dev)
>> struct uart_omap_port *up = dev_get_drvdata(dev);
>> struct omap_uart_port_info *pdata = dev->platform_data;
>>
> I'd like to see a comment here about why we return error. It's written
> in the changelog, but it deserves a comment here as well. Something like:
>
> When using 'no_console_suspend', the console UART must not be suspended.
> Since driver suspend is managed by runtime suspend, preventing runtime
> suspend (by returning error) will keep device active during suspend.
>
>
Ok. Will add this.
>> + if (up->is_suspending&& !console_suspend_enabled
>> + && uart_console(&up->port))
> nit: can you put the '&&' at the end of the first line, and have the
> 2nd line line up with the start of the first line?
>
Ok. Will modify this part.
> See other examples of similiar multi-line checks elsewhere in the
> driver.
>
> Other than those minor things, this patch is good.
>
> Then, can you repost and break this into 2 series since they can go
> independently.
>
> Patches 1-2 need to go through the serial tree (Greg), and I will queue
> up the OMAP specific changes once Greg has queued the serial core changes.
>
Yes, sounds good. Will break it in the next version into two series.
> Kevin