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.
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-rc3 custom tree which has
Santosh Shilimkar serial patch[1]
[1]: http://permalink.gmane.org/gmane.linux.ports.arm.omap/95828
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]>
Sourav Poddar (5):
driver: tty: serial: Move "uart_console" def to core header file.
driver: serial: omap: prevent runtime PM for "no_console_suspend"
case
arm: omap2+: serial: remove no_console_suspend support
arm: dts: am33xx: Remove "ti,no_idle_on_suspend" property.
arm: omap2+: device: remove "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check
arch/arm/boot/dts/am33xx.dtsi | 1 -
arch/arm/mach-omap2/omap_device.c | 12 +++---------
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 | 5 ++++-
drivers/tty/serial/serial_core.c | 6 ------
include/linux/serial_core.h | 7 +++++++
8 files changed, 14 insertions(+), 44 deletions(-)
"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 f660156..58d5b56 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) */
@@ -207,9 +206,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
@@ -292,9 +288,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
Remove "no_idle_on_suspend" check, since respective
driver should be able to prevent idling of a
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 | 12 +++---------
arch/arm/mach-omap2/omap_device.h | 10 ----------
2 files changed, 3 insertions(+), 19 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index 381be7a..2043d71 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:
@@ -620,11 +617,9 @@ static int _od_suspend_noirq(struct device *dev)
ret = pm_generic_suspend_noirq(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);
+ if (pm_generic_runtime_suspend(dev) == 0)
+ omap_device_idle(pdev);
od->flags |= OMAP_DEVICE_SUSPENDED;
- }
}
return ret;
@@ -638,8 +633,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
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]>
---
drivers/tty/serial/omap-serial.c | 29 ++++++++++++++++++++++++++++-
1 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 08332f3..468e7ad 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)))
@@ -1278,6 +1279,24 @@ 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);
+
+ if (!console_suspend_enabled && uart_console(&up->port))
+ up->is_suspending = true;
+
+ return 0;
+}
+
+static void serial_omap_complete(struct device *dev)
+{
+ struct uart_omap_port *up = dev_get_drvdata(dev);
+
+ if (!console_suspend_enabled && uart_console(&up->port))
+ up->is_suspending = false;
+}
+
static int serial_omap_suspend(struct device *dev)
{
struct uart_omap_port *up = dev_get_drvdata(dev);
@@ -1296,7 +1315,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)
{
@@ -1582,6 +1604,9 @@ 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)
+ return -EBUSY;
+
if (!up)
return -EINVAL;
@@ -1632,6 +1657,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
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 74a8125..49a050e 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -394,7 +394,6 @@
compatible = "ti,am3352-ocmcram";
reg = <0x40300000 0x10000>;
ti,hwmods = "ocmcram";
- ti,no_idle_on_suspend;
};
wkup_m3: wkup_m3@44d00000 {
--
1.7.1
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
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]>
> ---
> drivers/tty/serial/omap-serial.c | 29 ++++++++++++++++++++++++++++-
> 1 files changed, 28 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 08332f3..468e7ad 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)))
> @@ -1278,6 +1279,24 @@ 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);
> +
> + if (!console_suspend_enabled && uart_console(&up->port))
> + up->is_suspending = true;
This flag should be set unconditionally. IOW, the system is suspending
independently of the values of those flags. And then...
> + return 0;
> +}
> +
> +static void serial_omap_complete(struct device *dev)
> +{
> + struct uart_omap_port *up = dev_get_drvdata(dev);
> +
> + if (!console_suspend_enabled && uart_console(&up->port))
> + up->is_suspending = false;
> +}
> +
> static int serial_omap_suspend(struct device *dev)
> {
> struct uart_omap_port *up = dev_get_drvdata(dev);
> @@ -1296,7 +1315,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)
> {
> @@ -1582,6 +1604,9 @@ 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)
> + return -EBUSY;
... the console checks should be here.
> if (!up)
> return -EINVAL;
>
> @@ -1632,6 +1657,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)
Kevin
Sourav Poddar <[email protected]> writes:
> Remove "no_idle_on_suspend" check, since respective
> driver should be able to prevent idling of a
> 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 | 12 +++---------
> arch/arm/mach-omap2/omap_device.h | 10 ----------
> 2 files changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
> index 381be7a..2043d71 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:
> @@ -620,11 +617,9 @@ static int _od_suspend_noirq(struct device *dev)
> ret = pm_generic_suspend_noirq(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);
> + if (pm_generic_runtime_suspend(dev) == 0)
> + omap_device_idle(pdev);
> od->flags |= OMAP_DEVICE_SUSPENDED;
> - }
Look closely at the if statement, followed by more than one line, and
the braces removed.
That means that od->flag will be set, even when omap_device_idle() was
not called. Which in turn means that upon resume, omap_device_enable()
will be called on a device that has not had omap_device_idle() called,
which means there would be WARNs coming from omap_device due to the
mismatch.
Hmm... this does not instill confidence that this code has been tested.
Kevin
> }
>
> return ret;
> @@ -638,8 +633,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
Hi Kevin,
On Thursday 25 April 2013 03:04 AM, 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]>
>> ---
>> drivers/tty/serial/omap-serial.c | 29 ++++++++++++++++++++++++++++-
>> 1 files changed, 28 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index 08332f3..468e7ad 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)))
>> @@ -1278,6 +1279,24 @@ 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);
>> +
>> + if (!console_suspend_enabled&& uart_console(&up->port))
>> + up->is_suspending = true;
> This flag should be set unconditionally. IOW, the system is suspending
> independently of the values of those flags. And then...
>
True. Will change.
>> + return 0;
>> +}
>> +
>> +static void serial_omap_complete(struct device *dev)
>> +{
>> + struct uart_omap_port *up = dev_get_drvdata(dev);
>> +
>> + if (!console_suspend_enabled&& uart_console(&up->port))
>> + up->is_suspending = false;
>> +}
>> +
>> static int serial_omap_suspend(struct device *dev)
>> {
>> struct uart_omap_port *up = dev_get_drvdata(dev);
>> @@ -1296,7 +1315,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)
>> {
>> @@ -1582,6 +1604,9 @@ 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)
>> + return -EBUSY;
> ... the console checks should be here.
>
Ok. will change.
>> if (!up)
>> return -EINVAL;
>>
>> @@ -1632,6 +1657,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)
> Kevin
Hi Kevin,
On Thursday 25 April 2013 03:45 AM, Kevin Hilman wrote:
> Sourav Poddar<[email protected]> writes:
>
>> Remove "no_idle_on_suspend" check, since respective
>> driver should be able to prevent idling of a
>> 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 | 12 +++---------
>> arch/arm/mach-omap2/omap_device.h | 10 ----------
>> 2 files changed, 3 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
>> index 381be7a..2043d71 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:
>> @@ -620,11 +617,9 @@ static int _od_suspend_noirq(struct device *dev)
>> ret = pm_generic_suspend_noirq(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);
>> + if (pm_generic_runtime_suspend(dev) == 0)
>> + omap_device_idle(pdev);
>> od->flags |= OMAP_DEVICE_SUSPENDED;
>> - }
> Look closely at the if statement, followed by more than one line, and
> the braces removed.
>
> That means that od->flag will be set, even when omap_device_idle() was
> not called. Which in turn means that upon resume, omap_device_enable()
> will be called on a device that has not had omap_device_idle() called,
> which means there would be WARNs coming from omap_device due to the
> mismatch.
>
True, my bad. Will modify the code to bypass
"od->flags |= OMAP_DEVICE_SUSPENDED"
> Hmm... this does not instill confidence that this code has been tested.
>
Sorry for the above glitch, I tested this code. I checked that the
system is able to resume from suspend state, but completely missed the
following statement in the log..
"[ 47.922424] omap_uart omap_uart.2: omap_device: omap_device_enable()
called from invalid state 1"
> Kevin
>
>
>> }
>>
>> return ret;
>> @@ -638,8 +633,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
Hi Sourav,
Sourav Poddar <[email protected]> writes:
> Hi Kevin,
> On Thursday 25 April 2013 03:45 AM, Kevin Hilman wrote:
>> Sourav Poddar<[email protected]> writes:
>>
>>> Remove "no_idle_on_suspend" check, since respective
>>> driver should be able to prevent idling of a
>>> 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 | 12 +++---------
>>> arch/arm/mach-omap2/omap_device.h | 10 ----------
>>> 2 files changed, 3 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
>>> index 381be7a..2043d71 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:
>>> @@ -620,11 +617,9 @@ static int _od_suspend_noirq(struct device *dev)
>>> ret = pm_generic_suspend_noirq(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);
>>> + if (pm_generic_runtime_suspend(dev) == 0)
>>> + omap_device_idle(pdev);
>>> od->flags |= OMAP_DEVICE_SUSPENDED;
>>> - }
>> Look closely at the if statement, followed by more than one line, and
>> the braces removed.
>>
>> That means that od->flag will be set, even when omap_device_idle() was
>> not called. Which in turn means that upon resume, omap_device_enable()
>> will be called on a device that has not had omap_device_idle() called,
>> which means there would be WARNs coming from omap_device due to the
>> mismatch.
>>
> True, my bad. Will modify the code to bypass
> "od->flags |= OMAP_DEVICE_SUSPENDED"
>> Hmm... this does not instill confidence that this code has been tested.
>>
> Sorry for the above glitch, I tested this code. I checked that the
> system is able to resume from suspend state, but completely missed the
> following statement in the log..
> "[ 47.922424] omap_uart omap_uart.2: omap_device:
> omap_device_enable() called from invalid state 1"
No worries.
Thanks a lot for your responsiveness on this on this series. We've gone
through several ideas/approaches to fix this problem and you've been
very quick to address concerns and adapt the solution.
Thanks for your persistence, we're almost there...
Kevin