[PATCHv2 0/5] Serial Omap fixes and cleanups

[PATCHv2 0/5] Serial Omap fixes and cleanups

[Sourav Poddar]

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 (except drivers: serial: mpc52xx_uart: Remove "uart_console" defintion):
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

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 <santosh.shilimkar@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Rajendra nayak <rnayak@ti.com>


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

[PATCHv2 1/5] driver: tty: serial: Move "uart_console" def to core header file.

[Sourav Poddar]

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 <santosh.shilimkar@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Rajendra nayak <rnayak@ti.com>
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
 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

Re: [PATCHv2 1/5] driver: tty: serial: Move "uart_console" def to core header file.

[Felipe Balbi]

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

On Mon, Apr 22, 2013 at 07:13:53PM +0530, Sourav Poddar wrote:
> 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 <santosh.shilimkar@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Rajendra nayak <rnayak@ti.com>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>

looks alright

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCHv2 2/5] driver: serial: omap: prevent runtime PM for "no_console_suspend"

[Sourav Poddar]

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 <sourav.poddar@ti.com>
---
 drivers/tty/serial/omap-serial.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 08332f3..640b14e 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1582,7 +1582,7 @@ 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)
+	if (!up || (!console_suspend_enabled && uart_console(&up->port)))
 		return -EINVAL;
 
 	if (!pdata)
@@ -1614,6 +1614,9 @@ static int serial_omap_runtime_resume(struct device *dev)
 
 	int loss_cnt = serial_omap_get_context_loss_count(up);
 
+	if (!console_suspend_enabled && uart_console(&up->port))
+		return -EINVAL;
+
 	if (loss_cnt < 0) {
 		dev_err(dev, "serial_omap_get_context_loss_count failed : %d\n",
 			loss_cnt);
-- 
1.7.1

Re: [PATCHv2 2/5] driver: serial: omap: prevent runtime PM for "no_console_suspend"

[Felipe Balbi]

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

Hi,

On Mon, Apr 22, 2013 at 07:13:54PM +0530, Sourav Poddar wrote:
> 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 <sourav.poddar@ti.com>
> ---
>  drivers/tty/serial/omap-serial.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 08332f3..640b14e 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -1582,7 +1582,7 @@ 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)
> +	if (!up || (!console_suspend_enabled && uart_console(&up->port)))
>  		return -EINVAL;

-EBUSY would be a better value for uart_console case, so this check
should be splitted accordingly. Likewise on second hunk.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2 2/5] driver: serial: omap: prevent runtime PM for "no_console_suspend"

[Sourav Poddar]

Hi Felipe,
On Monday 22 April 2013 08:01 PM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Apr 22, 2013 at 07:13:54PM +0530, Sourav Poddar wrote:
>> 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<sourav.poddar@ti.com>
>> ---
>>   drivers/tty/serial/omap-serial.c |    5 ++++-
>>   1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index 08332f3..640b14e 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -1582,7 +1582,7 @@ 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)
>> +	if (!up || (!console_suspend_enabled&&  uart_console(&up->port)))
>>   		return -EINVAL;
> -EBUSY would be a better value for uart_console case, so this check
> should be splitted accordingly. Likewise on second hunk.
>
Ok. Will change in the next version.

Re: [PATCHv2 2/5] driver: serial: omap: prevent runtime PM for "no_console_suspend"

[Grygorii Strashko]

On 04/22/2013 04:43 PM, Sourav Poddar wrote:
> 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 <sourav.poddar@ti.com>
> ---
>   drivers/tty/serial/omap-serial.c |    5 ++++-
>   1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 08332f3..640b14e 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -1582,7 +1582,7 @@ 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)
> +	if (!up || (!console_suspend_enabled && uart_console(&up->port)))
>   		return -EINVAL;
Hi Sourav,
No ) You will block Runtime PM for console UART forever, but instead
it need to be blocked only during suspend - autosuspend should continue 
working.
But this will be not easy, again, -
because System suspend isn't synchronized with Runtime PM (I mean,
serial_omap_suspend/resume() may be called from one thread and
serial_omap_runtime_suspend/resume() from another at same time).
And now, serial_omap_suspend() callback is the only one place where you
can detect that system is going to sleep.

Personally, i don't believe in such approach (my experiences from K3.4 
said me
that there will be more problems than benefits).

And, I like combination of "no_console_suspend" in bootargs +
"ti,no_idle_on_suspend" for console UART in DT, because 1) it's debug 
option and 2) until
smth. will be decided about OMAP OCP Bus it can be used.

It's just my opinion.

Regards,
-grygorii
>   
>   	if (!pdata)
> @@ -1614,6 +1614,9 @@ static int serial_omap_runtime_resume(struct device *dev)
>   
>   	int loss_cnt = serial_omap_get_context_loss_count(up);
>   
> +	if (!console_suspend_enabled && uart_console(&up->port))
> +		return -EINVAL;
> +
>   	if (loss_cnt < 0) {
>   		dev_err(dev, "serial_omap_get_context_loss_count failed : %d\n",
>   			loss_cnt);

Re: [PATCHv2 2/5] driver: serial: omap: prevent runtime PM for "no_console_suspend"

[Kevin Hilman]

Grygorii Strashko <grygorii.strashko@ti.com> writes:

> On 04/22/2013 04:43 PM, Sourav Poddar wrote:
>> 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 <sourav.poddar@ti.com>
>> ---
>>   drivers/tty/serial/omap-serial.c |    5 ++++-
>>   1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index 08332f3..640b14e 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -1582,7 +1582,7 @@ 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)
>> +	if (!up || (!console_suspend_enabled && uart_console(&up->port)))
>>   		return -EINVAL;
> Hi Sourav,
> No ) You will block Runtime PM for console UART forever, but instead
> it need to be blocked only during suspend - autosuspend should
> continue working.

Correct.  

Sourav, as I mentioned when I suggested this approach, it should be done
*only* during suspend.

> But this will be not easy, again, -
> because System suspend isn't synchronized with Runtime PM (I mean,
> serial_omap_suspend/resume() may be called from one thread and
> serial_omap_runtime_suspend/resume() from another at same time).
> And now, serial_omap_suspend() callback is the only one place where you
> can detect that system is going to sleep.

So set an 'is_suspending' flag in ->suspend (it may need to be in
->prepare) and clear it in ->resume (->complete), and check the flag in
the ->runtime_supend() callback.  It's not uncommon for drivers to have
such a flag for various reasons.

> Personally, i don't believe in such approach (my experiences from K3.4
> said me that there will be more problems than benefits).
>
> And, I like combination of "no_console_suspend" in bootargs +
> "ti,no_idle_on_suspend" for console UART in DT, because 1) it's debug
> option and 2) until
> smth. will be decided about OMAP OCP Bus it can be used.
>
> It's just my opinion.

No, we need to get rid of ti,no_idle_on_suspend.  It's an ugly,
OMAP-specific hack (and I'm free to insult it because I introduced it.)

Kevin

Re: [PATCHv2 2/5] driver: serial: omap: prevent runtime PM for "no_console_suspend"

[Sourav Poddar]

Hi Kevin,
On Tuesday 23 April 2013 12:06 AM, Kevin Hilman wrote:
> Grygorii Strashko<grygorii.strashko@ti.com>  writes:
>
>> On 04/22/2013 04:43 PM, Sourav Poddar wrote:
>>> 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<sourav.poddar@ti.com>
>>> ---
>>>    drivers/tty/serial/omap-serial.c |    5 ++++-
>>>    1 files changed, 4 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>>> index 08332f3..640b14e 100644
>>> --- a/drivers/tty/serial/omap-serial.c
>>> +++ b/drivers/tty/serial/omap-serial.c
>>> @@ -1582,7 +1582,7 @@ 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)
>>> +	if (!up || (!console_suspend_enabled&&  uart_console(&up->port)))
>>>    		return -EINVAL;
>> Hi Sourav,
>> No ) You will block Runtime PM for console UART forever, but instead
>> it need to be blocked only during suspend - autosuspend should
>> continue working.
> Correct.
>
> Sourav, as I mentioned when I suggested this approach, it should be done
> *only* during suspend.
>
Yes, got the point.
>> But this will be not easy, again, -
>> because System suspend isn't synchronized with Runtime PM (I mean,
>> serial_omap_suspend/resume() may be called from one thread and
>> serial_omap_runtime_suspend/resume() from another at same time).
>> And now, serial_omap_suspend() callback is the only one place where you
>> can detect that system is going to sleep.
> So set an 'is_suspending' flag in ->suspend (it may need to be in
> ->prepare) and clear it in ->resume (->complete), and check the flag in
> the ->runtime_supend() callback.  It's not uncommon for drivers to have
> such a flag for various reasons.
>
Ok. Will adapt the next version inline with the above suggestions.
>> Personally, i don't believe in such approach (my experiences from K3.4
>> said me that there will be more problems than benefits).
>>
>> And, I like combination of "no_console_suspend" in bootargs +
>> "ti,no_idle_on_suspend" for console UART in DT, because 1) it's debug
>> option and 2) until
>> smth. will be decided about OMAP OCP Bus it can be used.
>>
>> It's just my opinion.
> No, we need to get rid of ti,no_idle_on_suspend.  It's an ugly,
> OMAP-specific hack (and I'm free to insult it because I introduced it.)
>
> Kevin
>

Re: [PATCHv2 2/5] driver: serial: omap: prevent runtime PM for "no_console_suspend"

[Sourav Poddar]

Hi Grygorii,
On Monday 22 April 2013 08:18 PM, Grygorii Strashko wrote:
> On 04/22/2013 04:43 PM, Sourav Poddar wrote:
>> 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 <sourav.poddar@ti.com>
>> ---
>>   drivers/tty/serial/omap-serial.c |    5 ++++-
>>   1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/tty/serial/omap-serial.c 
>> b/drivers/tty/serial/omap-serial.c
>> index 08332f3..640b14e 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -1582,7 +1582,7 @@ 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)
>> +    if (!up || (!console_suspend_enabled && uart_console(&up->port)))
>>           return -EINVAL;
> Hi Sourav,
> No ) You will block Runtime PM for console UART forever, but instead
> it need to be blocked only during suspend - autosuspend should 
> continue working.
> But this will be not easy, again, -
> because System suspend isn't synchronized with Runtime PM (I mean,
> serial_omap_suspend/resume() may be called from one thread and
> serial_omap_runtime_suspend/resume() from another at same time).
> And now, serial_omap_suspend() callback is the only one place where you
> can detect that system is going to sleep.
>
Yes, got this point.
As Kevin has already suggested a way to get around this, I will fix this 
portion
in the next version.
> Personally, i don't believe in such approach (my experiences from K3.4 
> said me
> that there will be more problems than benefits).
>
> And, I like combination of "no_console_suspend" in bootargs +
> "ti,no_idle_on_suspend" for console UART in DT, because 1) it's debug 
> option and 2) until
> smth. will be decided about OMAP OCP Bus it can be used.
>
> It's just my opinion.
>
> Regards,
> -grygorii
>>         if (!pdata)
>> @@ -1614,6 +1614,9 @@ static int serial_omap_runtime_resume(struct 
>> device *dev)
>>         int loss_cnt = serial_omap_get_context_loss_count(up);
>>   +    if (!console_suspend_enabled && uart_console(&up->port))
>> +        return -EINVAL;
>> +
>>       if (loss_cnt < 0) {
>>           dev_err(dev, "serial_omap_get_context_loss_count failed : 
>> %d\n",
>>               loss_cnt);
>

[PATCHv2 3/5] arm: omap2+: serial: remove no_console_suspend support

[Sourav Poddar]

"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 <santosh.shilimkar@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Rajendra nayak <rnayak@ti.com>
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
 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

Re: [PATCHv2 3/5] arm: omap2+: serial: remove no_console_suspend support

[Felipe Balbi]

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

On Mon, Apr 22, 2013 at 07:13:55PM +0530, Sourav Poddar wrote:
> "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 <santosh.shilimkar@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Rajendra nayak <rnayak@ti.com>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>

looks alright:

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCHv2 4/5] arm: dts: am33xx: Remove "ti,no_idle_on_suspend" property.

[Sourav Poddar]

The "ti,no_idle_on_suspend" property was required to keep ocmcram
clocks running during idle.

But the commit below[1], added in v3.6 should prevent the
any automaic 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.
[1]:
commit 72bb6f9b51c82c820ddef892455a85b115460904
Author: Kevin Hilman <khilman@ti.com>
Date:   Tue Jul 10 15:29:04 2012 -0700

    ARM: OMAP: omap_device: don't attempt late suspend if no driver bound

    Currently, the omap_device PM domain layer uses the late suspend and
    early resume callbacks to ensure devices are in their low power
    states.

    However, this is attempted even in cases where a driver probe has
    failed.  If a driver's ->probe() method fails, the driver is likely in
    a state where it is not expecting its runtime PM callbacks to be
    called, yet currently the omap_device PM domain code attempts to call
    the drivers callbacks.

    To fix, use the omap_device driver_status field to check whether a
    driver is bound to the omap_device before attempting to trigger driver
    callbacks.

Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Rajendra nayak <rnayak@ti.com>
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
 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

Re: [PATCHv2 4/5] arm: dts: am33xx: Remove "ti,no_idle_on_suspend" property.

[Felipe Balbi]

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

Hi,

On Mon, Apr 22, 2013 at 07:13:56PM +0530, Sourav Poddar wrote:
> The "ti,no_idle_on_suspend" property was required to keep ocmcram
> clocks running during idle.
> 
> But the commit below[1], added in v3.6 should prevent the
> any automaic 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.
> [1]:
> commit 72bb6f9b51c82c820ddef892455a85b115460904

your commit log can be improved a bit, it could look like below:

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.

other than that:

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2 4/5] arm: dts: am33xx: Remove "ti,no_idle_on_suspend" property.

[Sourav Poddar]

Hi Felipe,
On Monday 22 April 2013 08:05 PM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Apr 22, 2013 at 07:13:56PM +0530, Sourav Poddar wrote:
>> The "ti,no_idle_on_suspend" property was required to keep ocmcram
>> clocks running during idle.
>>
>> But the commit below[1], added in v3.6 should prevent the
>> any automaic 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.
>> [1]:
>> commit 72bb6f9b51c82c820ddef892455a85b115460904
> your commit log can be improved a bit, it could look like below:
>
> 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.
>
Looks better. Will make the necesary change in the next version.
> other than that:
>
> Reviewed-by: Felipe Balbi<balbi@ti.com>
>

[RFC/PATCHv2 5/5] arm: omap2+: omap_device: remove no_idle_on_suspend

[Sourav Poddar]

Remove the "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check, since 
driver should be able to prevent idling of an omap device
whenever required.

Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Rajendra nayak <rnayak@ti.com>
Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
Hi Kevin,

I have put this as an RFC, due to few comments on cover letter of
the previous version by Grygorii Strashko.
As, he has mentioned that there are Audio playback use cases which 
also requires "no_idle_on_suspend" and using them on mainline after
this series can cause regression.

What you think will be the right approach on this in relation to this patch? 
I mean every driver(if possible) should  prevent 
runtime PM for no_idle_on_suspend usecase and we get 
rid of this OMAP_DEVICE_NO_IDLE_ON_SUSPEND check? OR we should
drop this patch as of now? 

Hi Grygorii,

Is it possible to handle ABE no_idle_on_suspend uscase the way I am
trying to handle it for UART in the 2nd patch of this series? 


 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

Re: [RFC/PATCHv2 5/5] arm: omap2+: omap_device: remove no_idle_on_suspend

[Grygorii Strashko]

On 04/22/2013 04:43 PM, Sourav Poddar wrote:
> Remove the "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check, since
> driver should be able to prevent idling of an omap device
> whenever required.
>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Rajendra nayak <rnayak@ti.com>
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
> Hi Kevin,
>
> I have put this as an RFC, due to few comments on cover letter of
> the previous version by Grygorii Strashko.
> As, he has mentioned that there are Audio playback use cases which
> also requires "no_idle_on_suspend" and using them on mainline after
> this series can cause regression.
>
> What you think will be the right approach on this in relation to this patch?
> I mean every driver(if possible) should  prevent
> runtime PM for no_idle_on_suspend usecase and we get
> rid of this OMAP_DEVICE_NO_IDLE_ON_SUSPEND check? OR we should
> drop this patch as of now?
>
> Hi Grygorii,
>
> Is it possible to handle ABE no_idle_on_suspend uscase the way I am
> trying to handle it for UART in the 2nd patch of this series?
Unfortunately, I don't know ASOC details (my part is PM),  but from the 
first look it
will be not easy, because map4-dmic have no Runtime PM handlers at all, 
for example ((
>
>   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

Re: [RFC/PATCHv2 5/5] arm: omap2+: omap_device: remove no_idle_on_suspend

[Kevin Hilman]

Grygorii Strashko <grygorii.strashko@ti.com> writes:

> On 04/22/2013 04:43 PM, Sourav Poddar wrote:
>> Remove the "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check, since
>> driver should be able to prevent idling of an omap device
>> whenever required.
>>
>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Cc: Felipe Balbi <balbi@ti.com>
>> Cc: Rajendra nayak <rnayak@ti.com>
>> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
>> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
>> ---
>> Hi Kevin,
>>
>> I have put this as an RFC, due to few comments on cover letter of
>> the previous version by Grygorii Strashko.
>> As, he has mentioned that there are Audio playback use cases which
>> also requires "no_idle_on_suspend" and using them on mainline after
>> this series can cause regression.
>>
>> What you think will be the right approach on this in relation to this patch?
>> I mean every driver(if possible) should  prevent
>> runtime PM for no_idle_on_suspend usecase and we get
>> rid of this OMAP_DEVICE_NO_IDLE_ON_SUSPEND check? OR we should
>> drop this patch as of now?

This is the correct approach, and AFAICT you've fixed the *mainline*
users of this patch which is the important part.  If there are other
mainline users of this feature, we need to know about them.

Let me be clear: this OMAP_DEVICE_NO_IDLE_ON_SUSPEND feature is a hack
(it was introduced by me, but still a hack.)  We've found a way to
handle using the generic framework, and we should move to that.  There
are already a handful of complications when combining runtime PM and
system suspend, and this is just another one.  It makes the most sense
for this handling to be in the drivers themselves.  IOW: if the driver
wants to refuse to runtime suspend (during system suspend), it has the
choice.

>> Hi Grygorii,
>>
>> Is it possible to handle ABE no_idle_on_suspend uscase the way I am
>> trying to handle it for UART in the 2nd patch of this series?
> Unfortunately, I don't know ASOC details (my part is PM),  but from
> the first look it
> will be not easy, because map4-dmic have no Runtime PM handlers at
> all, for example ((

Are those drivers upstream?  If so, please point them out and show how
this feature is being used in *mainline* by those drivers.

For OMAP PM, we have been very clear for a long time all of our PM was
based on runtime PM.  Any drivers that are not runtime PM are broken and
need to be fixed.

As long as Sourav is fixing up all the mainline users of this feature, my
plan to merge/ack the changes unless there are some good arguemnts based
on *upstream* users of the feature.

Kevin

Re: [RFC/PATCHv2 5/5] arm: omap2+: omap_device: remove no_idle_on_suspend

[Sourav Poddar]

Hi Kevin,
On Tuesday 23 April 2013 12:11 AM, Kevin Hilman wrote:
> Grygorii Strashko<grygorii.strashko@ti.com>  writes:
>
>> On 04/22/2013 04:43 PM, Sourav Poddar wrote:
>>> Remove the "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check, since
>>> driver should be able to prevent idling of an omap device
>>> whenever required.
>>>
>>> Cc: Santosh Shilimkar<santosh.shilimkar@ti.com>
>>> Cc: Felipe Balbi<balbi@ti.com>
>>> Cc: Rajendra nayak<rnayak@ti.com>
>>> Cc: Grygorii Strashko<grygorii.strashko@ti.com>
>>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
>>> ---
>>> Hi Kevin,
>>>
>>> I have put this as an RFC, due to few comments on cover letter of
>>> the previous version by Grygorii Strashko.
>>> As, he has mentioned that there are Audio playback use cases which
>>> also requires "no_idle_on_suspend" and using them on mainline after
>>> this series can cause regression.
>>>
>>> What you think will be the right approach on this in relation to this patch?
>>> I mean every driver(if possible) should  prevent
>>> runtime PM for no_idle_on_suspend usecase and we get
>>> rid of this OMAP_DEVICE_NO_IDLE_ON_SUSPEND check? OR we should
>>> drop this patch as of now?
> This is the correct approach, and AFAICT you've fixed the *mainline*
> users of this patch which is the important part.  If there are other
> mainline users of this feature, we need to know about them.
>
> Let me be clear: this OMAP_DEVICE_NO_IDLE_ON_SUSPEND feature is a hack
> (it was introduced by me, but still a hack.)  We've found a way to
> handle using the generic framework, and we should move to that.  There
> are already a handful of complications when combining runtime PM and
> system suspend, and this is just another one.  It makes the most sense
> for this handling to be in the drivers themselves.  IOW: if the driver
> wants to refuse to runtime suspend (during system suspend), it has the
> choice.
>
Yes, I was also of the same view that the driver should take care of the
no_idle_on_suspend case and we should get rid of the hacks around this.
Modifying a respective driver will be a more generic solution which will 
work
irrespective of dt and non dt boot.

>>> Hi Grygorii,
>>>
>>> Is it possible to handle ABE no_idle_on_suspend uscase the way I am
>>> trying to handle it for UART in the 2nd patch of this series?
>> Unfortunately, I don't know ASOC details (my part is PM),  but from
>> the first look it
>> will be not easy, because map4-dmic have no Runtime PM handlers at
>> all, for example ((
> Are those drivers upstream?  If so, please point them out and show how
> this feature is being used in *mainline* by those drivers.
>
> For OMAP PM, we have been very clear for a long time all of our PM was
> based on runtime PM.  Any drivers that are not runtime PM are broken and
> need to be fixed.
>
> As long as Sourav is fixing up all the mainline users of this feature, my
> plan to merge/ack the changes unless there are some good arguemnts based
> on *upstream* users of the feature.
>
> Kevin
>

Re: [RFC/PATCHv2 5/5] arm: omap2+: omap_device: remove no_idle_on_suspend

[Grygorii Strashko]

On 04/23/2013 08:19 AM, Sourav Poddar wrote:
> Hi Kevin,
> On Tuesday 23 April 2013 12:11 AM, Kevin Hilman wrote:
>> Grygorii Strashko<grygorii.strashko@ti.com>  writes:
>>
>>> On 04/22/2013 04:43 PM, Sourav Poddar wrote:
>>>> Remove the "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check, since
>>>> driver should be able to prevent idling of an omap device
>>>> whenever required.
>>>>
>>>> Cc: Santosh Shilimkar<santosh.shilimkar@ti.com>
>>>> Cc: Felipe Balbi<balbi@ti.com>
>>>> Cc: Rajendra nayak<rnayak@ti.com>
>>>> Cc: Grygorii Strashko<grygorii.strashko@ti.com>
>>>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
>>>> ---
>>>> Hi Kevin,
>>>>
>>>> I have put this as an RFC, due to few comments on cover letter of
>>>> the previous version by Grygorii Strashko.
>>>> As, he has mentioned that there are Audio playback use cases which
>>>> also requires "no_idle_on_suspend" and using them on mainline after
>>>> this series can cause regression.
>>>>
>>>> What you think will be the right approach on this in relation to 
>>>> this patch?
>>>> I mean every driver(if possible) should  prevent
>>>> runtime PM for no_idle_on_suspend usecase and we get
>>>> rid of this OMAP_DEVICE_NO_IDLE_ON_SUSPEND check? OR we should
>>>> drop this patch as of now?
>> This is the correct approach, and AFAICT you've fixed the *mainline*
>> users of this patch which is the important part.  If there are other
>> mainline users of this feature, we need to know about them.
>>
>> Let me be clear: this OMAP_DEVICE_NO_IDLE_ON_SUSPEND feature is a hack
>> (it was introduced by me, but still a hack.)  We've found a way to
>> handle using the generic framework, and we should move to that. There
>> are already a handful of complications when combining runtime PM and
>> system suspend, and this is just another one.  It makes the most sense
>> for this handling to be in the drivers themselves.  IOW: if the driver
>> wants to refuse to runtime suspend (during system suspend), it has the
>> choice.
>>
> Yes, I was also of the same view that the driver should take care of the
> no_idle_on_suspend case and we should get rid of the hacks around this.
> Modifying a respective driver will be a more generic solution which 
> will work
> irrespective of dt and non dt boot.
Hi Sourav, Kevin,

Let it be, but could you update patch description with detailed explanation
of what drivers should do from now to be able to use such functionality
(make IP active while System is suspended).
So, people, who've used this hack before (even if these users are not in 
*mainline*)
will know what to do.

Regards
-grygorii

>>>> Hi Grygorii,
>>>>
>>>> Is it possible to handle ABE no_idle_on_suspend uscase the way I am
>>>> trying to handle it for UART in the 2nd patch of this series?
>>> Unfortunately, I don't know ASOC details (my part is PM),  but from
>>> the first look it
>>> will be not easy, because map4-dmic have no Runtime PM handlers at
>>> all, for example ((
>> Are those drivers upstream?  If so, please point them out and show how
>> this feature is being used in *mainline* by those drivers.
>>
>> For OMAP PM, we have been very clear for a long time all of our PM was
>> based on runtime PM.  Any drivers that are not runtime PM are broken and
>> need to be fixed.
>>
>> As long as Sourav is fixing up all the mainline users of this 
>> feature, my
>> plan to merge/ack the changes unless there are some good arguemnts based
>> on *upstream* users of the feature.
>>
>> Kevin
>>
>

Re: [RFC/PATCHv2 5/5] arm: omap2+: omap_device: remove no_idle_on_suspend

[Sourav Poddar]

On Tuesday 23 April 2013 02:49 PM, Grygorii Strashko wrote:
> On 04/23/2013 08:19 AM, Sourav Poddar wrote:
>> Hi Kevin,
>> On Tuesday 23 April 2013 12:11 AM, Kevin Hilman wrote:
>>> Grygorii Strashko<grygorii.strashko@ti.com>  writes:
>>>
>>>> On 04/22/2013 04:43 PM, Sourav Poddar wrote:
>>>>> Remove the "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check, since
>>>>> driver should be able to prevent idling of an omap device
>>>>> whenever required.
>>>>>
>>>>> Cc: Santosh Shilimkar<santosh.shilimkar@ti.com>
>>>>> Cc: Felipe Balbi<balbi@ti.com>
>>>>> Cc: Rajendra nayak<rnayak@ti.com>
>>>>> Cc: Grygorii Strashko<grygorii.strashko@ti.com>
>>>>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
>>>>> ---
>>>>> Hi Kevin,
>>>>>
>>>>> I have put this as an RFC, due to few comments on cover letter of
>>>>> the previous version by Grygorii Strashko.
>>>>> As, he has mentioned that there are Audio playback use cases which
>>>>> also requires "no_idle_on_suspend" and using them on mainline after
>>>>> this series can cause regression.
>>>>>
>>>>> What you think will be the right approach on this in relation to 
>>>>> this patch?
>>>>> I mean every driver(if possible) should  prevent
>>>>> runtime PM for no_idle_on_suspend usecase and we get
>>>>> rid of this OMAP_DEVICE_NO_IDLE_ON_SUSPEND check? OR we should
>>>>> drop this patch as of now?
>>> This is the correct approach, and AFAICT you've fixed the *mainline*
>>> users of this patch which is the important part.  If there are other
>>> mainline users of this feature, we need to know about them.
>>>
>>> Let me be clear: this OMAP_DEVICE_NO_IDLE_ON_SUSPEND feature is a hack
>>> (it was introduced by me, but still a hack.)  We've found a way to
>>> handle using the generic framework, and we should move to that. There
>>> are already a handful of complications when combining runtime PM and
>>> system suspend, and this is just another one.  It makes the most sense
>>> for this handling to be in the drivers themselves.  IOW: if the driver
>>> wants to refuse to runtime suspend (during system suspend), it has the
>>> choice.
>>>
>> Yes, I was also of the same view that the driver should take care of the
>> no_idle_on_suspend case and we should get rid of the hacks around this.
>> Modifying a respective driver will be a more generic solution which 
>> will work
>> irrespective of dt and non dt boot.
> Hi Sourav, Kevin,
>
> Let it be, but could you update patch description with detailed 
> explanation
> of what drivers should do from now to be able to use such functionality
> (make IP active while System is suspended).
> So, people, who've used this hack before (even if these users are not 
> in *mainline*)
> will know what to do.
>
Sure, will try to be more explicit in my change log in my
next version.
> Regards
> -grygorii
>
>>>>> Hi Grygorii,
>>>>>
>>>>> Is it possible to handle ABE no_idle_on_suspend uscase the way I am
>>>>> trying to handle it for UART in the 2nd patch of this series?
>>>> Unfortunately, I don't know ASOC details (my part is PM),  but from
>>>> the first look it
>>>> will be not easy, because map4-dmic have no Runtime PM handlers at
>>>> all, for example ((
>>> Are those drivers upstream?  If so, please point them out and show how
>>> this feature is being used in *mainline* by those drivers.
>>>
>>> For OMAP PM, we have been very clear for a long time all of our PM was
>>> based on runtime PM.  Any drivers that are not runtime PM are broken 
>>> and
>>> need to be fixed.
>>>
>>> As long as Sourav is fixing up all the mainline users of this 
>>> feature, my
>>> plan to merge/ack the changes unless there are some good arguemnts 
>>> based
>>> on *upstream* users of the feature.
>>>
>>> Kevin
>>>
>>
>

Re: [RFC/PATCHv2 5/5] arm: omap2+: omap_device: remove no_idle_on_suspend

[Felipe Balbi]

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

Hi,

On Mon, Apr 22, 2013 at 07:13:57PM +0530, Sourav Poddar wrote:
> Remove the "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check, since 
> driver should be able to prevent idling of an omap device
> whenever required.
> 
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Rajendra nayak <rnayak@ti.com>
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
> Hi Kevin,
> 
> I have put this as an RFC, due to few comments on cover letter of
> the previous version by Grygorii Strashko.
> As, he has mentioned that there are Audio playback use cases which 
> also requires "no_idle_on_suspend" and using them on mainline after
> this series can cause regression.
> 
> What you think will be the right approach on this in relation to this patch? 
> I mean every driver(if possible) should  prevent 
> runtime PM for no_idle_on_suspend usecase and we get 
> rid of this OMAP_DEVICE_NO_IDLE_ON_SUSPEND check? OR we should
> drop this patch as of now? 
> 
> Hi Grygorii,
> 
> Is it possible to handle ABE no_idle_on_suspend uscase the way I am
> trying to handle it for UART in the 2nd patch of this series? 

let's ask Péter.

Péter, OMAP_DEVICE_NO_IDLE_ON_SUSPEND should be removed as driver's can
get same behavior by just returning -EBUSY from their ->runtime_suspend
callbacks. Do you see any problems with patch below (left for
reference) ? Would it be too difficult to add
->runtime_suspend/->runtime_resume on ASoC layer ?

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

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC/PATCHv2 5/5] arm: omap2+: omap_device: remove no_idle_on_suspend

[Peter Ujfalusi]

Hi,

On 04/22/2013 04:38 PM, Felipe Balbi wrote:
>> Is it possible to handle ABE no_idle_on_suspend uscase the way I am
>> trying to handle it for UART in the 2nd patch of this series? 
> 
> let's ask Péter.
> 
> Péter, OMAP_DEVICE_NO_IDLE_ON_SUSPEND should be removed as driver's can
> get same behavior by just returning -EBUSY from their ->runtime_suspend
> callbacks. Do you see any problems with patch below (left for
> reference) ? Would it be too difficult to add
> ->runtime_suspend/->runtime_resume on ASoC layer ?

I don't see any issue with this. FWIW AESS/ABE does not use the
OMAP_DEVICE_NO_IDLE_ON_SUSPEND flag neither in upstream nor in my development
branch.

-- 
Péter

Re: [RFC/PATCHv2 5/5] arm: omap2+: omap_device: remove no_idle_on_suspend

[Felipe Balbi]

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

On Mon, Apr 22, 2013 at 07:13:57PM +0530, Sourav Poddar wrote:
> Remove the "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check, since 
> driver should be able to prevent idling of an omap device
> whenever required.
> 
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Rajendra nayak <rnayak@ti.com>
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>

Looks alright to me:

Reviewed-by: Felipe Balbi <balbi@ti.com>

but let's see what Péter Ujfalusi says before applying this one.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]