Re: [RFC/RFT PATCH 2/2] SERIAL: OMAP: Remove the idle handling from the driver

[a0131647 <sourav.poddar@ti.com>]

Hi,
On Friday 15 February 2013 07:13 PM, Santosh Shilimkar wrote:
> On Friday 15 February 2013 07:04 PM, Felipe Balbi wrote:
>> Hi,
>>
>> On Fri, Feb 15, 2013 at 06:49:20PM +0530, Santosh Shilimkar wrote:
>>>>> @@ -279,8 +259,6 @@ static void serial_omap_stop_tx(struct 
>>>>> uart_port *port)
>>>>>           serial_out(up, UART_IER, up->ier);
>>>>>       }
>>>>>
>>>>> -    serial_omap_set_forceidle(up);
>>>>> -
>>>>>       pm_runtime_mark_last_busy(up->dev);
>>>>>       pm_runtime_put_autosuspend(up->dev);
>>>>>   }
>>>>> @@ -348,7 +326,6 @@ static void serial_omap_start_tx(struct 
>>>>> uart_port *port)
>>>>>
>>>>>       pm_runtime_get_sync(up->dev);
>>>>>       serial_omap_enable_ier_thri(up);
>>>>> -    serial_omap_set_noidle(up);
>>>>
>>>> this patch is changing behavior. Currently driver has:
>>>>
>>>> start_tx()
>>>> get_sync()
>>>> set_noidle()
>>>> put_autosuspend()
>>>>
>>>> ....
>>>>
>>>> stop_tx()
>>>> get_sync()
>>>> set_force_idle()
>>>> put_autosuspend()
>>>>
>>>> with this change, you will have:
>>>>
>>>> start_tx()
>>>> get_sync()
>>>> set_noidle()
>>>> put_autosuspend()
>>>> set_force_idle()
>>>>
>>>> this in itself might be enough to go back to corrupted serial if
>>>> put_autosuspend is so quick that set_force_idle() is called before all
>>>> data has been shifted out of FIFO and through the UART lines.
>>>>
>>> Really. Infact the old sequence was buggy because you are setting
>>> force_idle even before suspending the device. And that force idle
>>
>> then that bug has to be fixed first and patch needs to be sent to stable
>> before we change that part of the code, wouldn't you agree ?
>>
> Agree. Will be good to get that fixed for stable. Probably Sourabh
> can fix that one.
>
Yes, will send a patch fix  for this.
>>> isn't really force idle but setting ip to smart idle. Just look at
>>> what serial.c
>>
>> indeed.
>>
>>>> Before doing this, you should at least test that removing
>>>> pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() from
>>>> start_tx() and removing pm_runtime_get_sync() from stop_tx() works 
>>>> fine.
>>>>
>>> Seems to work for me with above changes as well. Can you please
>>> try out and see if you see any issue. I doubt you will...
>>
>> what I'm saying is that current code, as you put it yourself, is buggy,
>> so we ought to fix the bugs first before changing behavior. If not for
>> anything else, at least to have a clean tree which we can bisect.
>>
>>>> Also, $SUBJECT isn't improving the situation regarding UART Wakeup,
>>>> there is still the regression of UART never being wakeup capable.
>>>>
>>> You are mixing the stuff here. The subject is correct.
>>
>> ->enable_wakeup() sets the IP to smart_idle_wakeup, which is done on
>> SYSC register. If $SUBJECT wants to fix SYSC usage, it ought to fix them
>> all.
>>
> But SYSC is already in smart_idle_wakeup() mode. I get your point
> though. The main purpose of that wakeup hook is to trigger io_ring
> and pad wakeup.
>
> BTW, Rajendra is looking at how to get rid of wakeup function pointer
> as well since that also comes in way for DT.
>
> Regards
> Santosh
>
>
~Sourav