Re: [PATCH/Resend 2/2] arm: mach-omap2: prevent UART console idleon suspend while using "no_console_suspend"

[Santosh Shilimkar]

On Friday 05 April 2013 12:38 PM, Sourav Poddar wrote:
> Hi Kevin,
> On Wednesday 03 April 2013 11:18 PM, Kevin Hilman wrote:
>> Sourav Poddar<sourav.poddar@xxxxxx>  writes:
>>
>>> Hi Kevin,
>>> On Wednesday 20 March 2013 05:36 PM, Sourav Poddar wrote:
>>>> Realised the list  to whom the patch was send got dropped. Ccing
>>>> them all..
>>>> On Wednesday 20 March 2013 05:18 PM, Sourav Poddar wrote:
>>>>> Hi Kevin,
>>>>> On Tuesday 19 March 2013 12:24 AM, Kevin Hilman wrote:
>>>>>> Sourav Poddar<sourav.poddar@xxxxxx>   writes:
>>>>>>
>>>>>>> With dt boot, uart wakeup after suspend is non functional on
>>>>>>> omap4/5 while using
>>>>>>> "no_console_suspend" in the bootargs. With "no_console_suspend"
>>>>>>> used, od->flags
>>>>>>> should be ORed with "OMAP_DEVICE_NO_IDLE_ON_SUSPEND", thereby not
>>>>>>> allowing the console
>>>>>>> to idle in the suspend path. For non-dt case, this was taken care
>>>>>>> by platform data.
>>>>>>>
>>>>>>> Tested on omap5430evm, omap4430sdp.
>>>>>>>
>>>>>>> Cc: Santosh Shilimkar<santosh.shilimkar@xxxxxx>
>>>>>>> Cc: Felipe Balbi<balbi@xxxxxx>
>>>>>>> Cc: Rajendra nayak<rnayak@xxxxxx>
>>>>>>> Signed-off-by: Sourav Poddar<sourav.poddar@xxxxxx>
>>>>>> This patch creates a dependency between omap_device (generic,
>>>>>> device-independent code) and a specific driver (UART.)
>>>>>>
>>>>>> If you need to do something like this that's DT boot specific, then
>>>>>> we probably need some late initcall in serial.c to handle this.
>>>>>> It does
>>>>>> not belong in omap_device.
>>>>>>
>>>>> The following function "omap_device_disable_idle_on_suspend(pdev)"
>>>>> should only
>>>>> be called once the omap device has been build, which in the case of
>>>>> device tree is
>>>>> done in omap_device.c file. Moreover, the above call should be
>>>>> executed conditionally
>>>>> and should depend on the following two parameter.
>>>>>
>>>>> [1]  a. Whether "no_console_suspend" is set and
>>>>>       b.  the device build is a console uart.
>>>>>
>>>>> When I look closely into the serial.c file, I realised that
>>>>> "core_initcall(omap_serial_early_init)" gets called irrespective
>>>>> of dt/non dt boot and will take care of most of the stuff(checking
>>>>> whether
>>>>> "no_console_suspend" is used and which uart is used as a console
>>>>> uart) which the
>>>>> $subject patch is proposing.
>>>>>
>>>>> But the problem is that we need to exchange the parsed information
>>>>> from serial.c to the omap_device file for the condtional execution of
>>>>> "omap_device_disable_idle_on_suspend"
>>>>>
>>>>> In this case,
>>>>> from "serial.c" we need
>>>>>      1. no_console_suspend = true
>>>>>      2. strcpy(console_name, oh_name), where oh_name corresponds to
>>>>> the console uart.
>>>>>
>>>>> then in "omap_device.c" do
>>>>>      if (no_console_suspend&&  !strcmp(oh->name, console_name))
>>>>>                  omap_device_disable_idle_on_suspend(pdev);
>>>>>
>>>>> Please correct if I am understanding it incorrectly.
>>>>>
>>>>> If the above understanding looks good to you, is there a way we can
>>>>> make this
>>>>> exchange of information happen between serial.c and omap_device.c file?
>>> Any input on this?
>>> As I explained earlier, that there is a need to parse information in
>>> serial.c and use that in
>>> omap_device.c only after the device is build.
>> As I explained earlier, any device specific hacks inside omap_device
>> should be a red flag that something has gone wrong.
>>
>> How about fixing the UART driver/core to not runtime suspend if
>> no_console_suspend is given?
>>
>> Then we can get rid of this no_idle_on_suspend hack all together since
>> UART is the only remaining user.
>>
> 
> Yes, that can be done.
> 
> I cooked up an experimental patch, based on your above suggestion. The patch is
> inlined along with  the mail. I tested the patch and it resolves
> the issue.
> 
Would be good to start a new thread.

> Depending on whether no_console_suspend is used or not, we can set a variable
> which can be used as a condition in runtime api's in serial driver.
> 
> The information whether "no_console_suspend" is used or not is parsed in
> serial.c file, and this need to be communicated to the driver layer.
> 
WHY ?
Serial core already knows about 'no_console_suspend' and thats used
to manage console lock.

> In my experimental patch below, I have used an extern variable
> "force_console_suspend_disable".
> 
> Do you see a better way of handling this without using extern?
> 
> ---------
> From: Sourav Poddar <sourav.poddar@xxxxxx>
> Date: Wed, 13 Mar 2013 20:32:36 +0530
> Subject: [PATCH] arm: mach-omap2: prevent UART console idle on suspend while using "no_console_suspend"
> 
> With dt boot, uart wakeup after suspend is non functional while using
> "no_console_suspend" in the bootargs. With "no_console_suspend" used, od->flags
> should be ORed with "OMAP_DEVICE_NO_IDLE_ON_SUSPEND", thereby not allowing the console
> to idle in the suspend path.
> 
> Tested on omap5430evm, omap4430sdp.
> 
> Signed-off-by: Sourav Poddar <sourav.poddar@xxxxxx>
> ---
>  arch/arm/mach-omap2/serial.c     |    6 ++++--
>  drivers/tty/serial/omap-serial.c |    6 +++++-
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index f660156..427c407 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -51,7 +51,7 @@
>  #define DEFAULT_AUTOSUSPEND_DELAY    -1
> 
>  #define MAX_UART_HWMOD_NAME_LEN        16
> -
> +int force_console_suspend_disable;
>  struct omap_uart_state {
>      int num;
> 
> @@ -207,8 +207,10 @@ static int __init omap_serial_early_init(void)
>                      uart_name, uart->num);
>              }
> 
> -            if (cmdline_find_option("no_console_suspend"))
> +            if (cmdline_find_option("no_console_suspend")) {
> +                force_console_suspend_disable = 1;
>                  no_console_suspend = true;
> +            }
> 
>              /*
>               * omap-uart can be used for earlyprintk logs
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 08332f3..395ade8 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -43,6 +43,7 @@
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_data/serial-omap.h>
> 
> +extern int force_console_suspend_disable;
This is no good. You need to use the information from
core serial layer

>  #define OMAP_MAX_HSUART_PORTS    6
> 
>  #define UART_BUILD_REVISION(x, y)    (((x) << 8) | (y))
> @@ -1582,7 +1583,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 || force_console_suspend_disable)
>          return -EINVAL;
> 
>      if (!pdata)
> @@ -1614,6 +1615,9 @@ static int serial_omap_runtime_resume(struct device *dev)
> 
>      int loss_cnt = serial_omap_get_context_loss_count(up);
> 
> +    if (force_console_suspend_disable)
> +        return -EINVAL;
> +
>      if (loss_cnt < 0) {
>          dev_err(dev, "serial_omap_get_context_loss_count failed : %d\n",
>              loss_cnt);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/