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

[Sourav Poddar <sourav.poddar@ti.com>]

Hi,
On Tuesday 02 April 2013 04:14 PM, Michael Trimarchi wrote:
> Hi
>
> On 02/04/13 12:39, Sourav Poddar wrote:
>> Hi,
>> On Tuesday 02 April 2013 03:36 PM, Michael Trimarchi wrote:
>>> Hi
>>>
>>> On 02/04/13 11:50, Sourav Poddar wrote:
>>>> 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@ti.com>    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@ti.com>
>>>>>>>> Cc: Felipe Balbi<balbi@ti.com>
>>>>>>>> Cc: Rajendra nayak<rnayak@ti.com>
>>>>>>>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
>>>>>>> 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.
>>>>
>>>> Below is the patch (inlined) which further explains my point. The patch is "just for the
>>>> idea" I am trying to express.
>>>> I have used extern variables to exchange information between serial.c and omap_device.c.
>>>> Is there is a better way, we can do this "information exchange" without using extern variables?
>>>>
>>>>
>>>> -----
>>>> From: Sourav Poddar<sourav.poddar@ti.com>
>>>> Date: Wed, 13 Mar 2013 20:32:36 +0530
>>>> Subject: [RFC/PATCH] arm: mach-omap2: prevent UART console idle on suspend while using "no_console_suspend"
>>>>
>>>> With dt boot on omap5, 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. For non-dt case, this was taken care by platform data.
>>>>
>>>> Tested on omap5430evm, omap4430sdp.
>>>>
>>>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
>>>> ---
>>>> The problem is I need to use couple of "extern variables" which
>>>> can be used in the omap_device file, after the device is built from dt.
>>>>
>>>>    arch/arm/mach-omap2/omap_device.c |    7 ++++++-
>>>>    arch/arm/mach-omap2/serial.c      |    4 +++-
>>>>    2 files changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
>>>> index e065daa..f4ebf9f 100644
>>>> --- a/arch/arm/mach-omap2/omap_device.c
>>>> +++ b/arch/arm/mach-omap2/omap_device.c
>>>> @@ -96,6 +96,9 @@
>>>>    #define USE_WAKEUP_LAT            0
>>>>    #define IGNORE_WAKEUP_LAT        1
>>>>
>>>> +extern u8 no_console_suspend;
>>>> +extern char console_uart_name[];
>>>> +
>>>>    static int omap_early_device_register(struct platform_device *pdev);
>>>>
>>>>    static struct omap_device_pm_latency omap_default_latency[] = {
>>>> @@ -372,7 +375,9 @@ 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))
>>>> +    if (no_console_suspend&&   !strcmp(oh->name, console_uart_name))
>>>> +        omap_device_disable_idle_on_suspend(pdev);
>>>> +    else if (of_get_property(node, "ti,no_idle_on_suspend", NULL))
>>>>            omap_device_disable_idle_on_suspend(pdev);
>>> Why do not use some flags instead of external variable?
>>>
>>>
>> That flag also need to be set in serial.c and used in omap_device.c.
>> Moreover, I am doing a strcmp so I need a console uart name to be passed
>> from serial.c to omap_device.c
> Just an idea, I didn't check the code
>
> if (oh->flags&  OMAP_FORCE_DISABLE_IDLE_ON_SUSPEND || of_get_property(node, "ti,no_idle_on_suspend", NULL))
> 	omap_device_disable_idle_on_suspend(pdev);
>
> And I don't know if you can set such flags in serial.c
>
Yes, it should be possible.  I will try this out.
Thanks for the suggestion.

~Sourav
> Michael
>
>
>>>>        pdev->dev.pm_domain =&omap_device_pm_domain;
>>>> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
>>>> index 037e691..f841ab5 100644
>>>> --- a/arch/arm/mach-omap2/serial.c
>>>> +++ b/arch/arm/mach-omap2/serial.c
>>>> @@ -63,8 +63,9 @@ 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;
>>>> +u8 no_console_suspend;
>>>> +char console_uart_name[MAX_UART_HWMOD_NAME_LEN];
>>>>
>>>>    #define DEFAULT_RXDMA_POLLRATE        1    /* RX DMA polling rate (us) */
>>>>    #define DEFAULT_RXDMA_BUFSIZE        4096    /* RX DMA buffer size */
>>>> @@ -199,6 +200,7 @@ static int __init omap_serial_early_init(void)
>>>>                    "%s%d", OMAP_SERIAL_NAME, uart->num);
>>>>
>>>>            if (cmdline_find_option(uart_name)) {
>>>> +            strcpy(console_uart_name, oh_name);
>>>>                console_uart_id = uart->num;
>>>>
>>>>                if (console_loglevel>= 10) {
>>> Michael
>> ~Sourav