Sorry for the delay... > On April 13, 2018 at 7:27 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote: > > On Fri, Apr 13, 2018 at 7:00 PM, Ulrich Hecht > <ulrich.hecht+renesas@xxxxxxxxx> wrote: > > These patches make sure that the device is up while the suspend/resume code > > is executed. Up-port from the BSP, tested not to break stuff. > > > > Hien Dang (2): > > serial: sh-sci: Use pm_runtime_get_sync()/put() on suspend > > serial: sh-sci: Use pm_runtime_get_sync()/put() on resume > > I don't think it makes much sense to split this in two parts... > > Furthermore, shouldn't this be handled by the serial core, calling > uart_change_pm()? > > It looks like uart_resume_port() already changes the port's state to > enabled, while uart_suspend_port() assumes the port is already enabled, > and disables it. > > Perhaps handling is not correct for some code paths? The way I understand it, the problem this intends to fix is not the state the device ends up in, but that it needs to be powered while registers are read or written. It seems to me that that the current "resume" code should work in that respect, because it changes the PM state to "on" in uart_resume_port() before any access to hardware registers takes place, so there is nothing that needs to be fixed. That may be different for the "suspend" part, though, because it assumes that the PM state is "on", and I think that is what the patch asserts to not be a valid assumption anymore. It's hard to tell if that is true, though, because I cannot reproduce the issue here; it just works either way... CU Uli
