Hi, On Fri, Feb 15, 2013 at 19:13:42, Shilimkar, Santosh 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. > > >> 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. > With these changes the async wakeup mechanism for UART which depends on SIDLE bits being set to 0x3 and ENWAKEUP being set to 0x1 breaks. I noticed this while testing these changes on top of the AM335x suspend-resume support. How about leveraging the generic wakeup flag for all devices to get the required functionality for wakeup? A call to device_init_wakeup() in probe, followed by a check for device_may_wakeup() in the driver's suspend routine can be used to have omap_device_idle_hwmods() configure the SIDLE bits appropriately. This should help configure SIDLE to FORCE/NO_IDLE during active mode along with the appropriate SYSC configuration during suspend. The device_may_wakeup() check could even be used to enable IO Daisy chaining feature for the IOs. Regards, Vaibhav -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
