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

[Santosh Shilimkar]

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.

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