* [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() @ 2018-04-13 17:00 Ulrich Hecht 2018-04-13 17:00 ` [PATCH 1/2] serial: sh-sci: Use pm_runtime_get_sync()/put() on suspend Ulrich Hecht ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Ulrich Hecht @ 2018-04-13 17:00 UTC (permalink / raw) To: linux-renesas-soc; +Cc: wsa, geert, linux-serial, magnus.damm, Ulrich Hecht Hi! 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. CU Uli 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 drivers/tty/serial/sh-sci.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] serial: sh-sci: Use pm_runtime_get_sync()/put() on suspend 2018-04-13 17:00 [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() Ulrich Hecht @ 2018-04-13 17:00 ` Ulrich Hecht 2018-04-13 17:00 ` [PATCH 2/2] serial: sh-sci: Use pm_runtime_get_sync()/put() on resume Ulrich Hecht 2018-04-13 17:27 ` [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() Geert Uytterhoeven 2 siblings, 0 replies; 8+ messages in thread From: Ulrich Hecht @ 2018-04-13 17:00 UTC (permalink / raw) To: linux-renesas-soc Cc: wsa, geert, linux-serial, magnus.damm, Hien Dang, Hiromitsu Yamasaki, Ulrich Hecht From: Hien Dang <hien.dang.eb@renesas.com> Since commit '39dd0f234fc37d ("PM / Domains: Allow runtime PM during system PM phases")', runtime PM may be in suspended state when the module registers are backed up. It is therefore necessary to ensure the device is on during suspend by using pm_runtime_get_sync()/pm_runtime_put(). Signed-off-by: Hien Dang <hien.dang.eb@renesas.com> Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com> [uli: edited description for clarity] Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com> --- drivers/tty/serial/sh-sci.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index fdbbff5..22d7a78 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -3246,8 +3246,11 @@ static __maybe_unused int sci_suspend(struct device *dev) { struct sci_port *sport = dev_get_drvdata(dev); - if (sport) + if (sport) { + pm_runtime_get_sync(sport->port.dev); uart_suspend_port(&sci_uart_driver, &sport->port); + pm_runtime_put(sport->port.dev); + } return 0; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] serial: sh-sci: Use pm_runtime_get_sync()/put() on resume 2018-04-13 17:00 [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() Ulrich Hecht 2018-04-13 17:00 ` [PATCH 1/2] serial: sh-sci: Use pm_runtime_get_sync()/put() on suspend Ulrich Hecht @ 2018-04-13 17:00 ` Ulrich Hecht 2018-04-13 17:27 ` [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() Geert Uytterhoeven 2 siblings, 0 replies; 8+ messages in thread From: Ulrich Hecht @ 2018-04-13 17:00 UTC (permalink / raw) To: linux-renesas-soc Cc: wsa, geert, linux-serial, magnus.damm, Hien Dang, Hiromitsu Yamasaki, Ulrich Hecht From: Hien Dang <hien.dang.eb@renesas.com> Since commit '39dd0f234fc37d ("PM / Domains: Allow runtime PM during system PM phases")', runtime PM may be in suspended state during the system suspend phase. It is therefore necessary to call pm_runtime_get_sync()/ pm_runtime_put() when accessing the hardware. This modification is the counterpart for the resume case. It ensures stability of the system, should the kernel allow the devices's runtime suspend state to change during the system resume phase as well. Signed-off-by: Hien Dang <hien.dang.eb@renesas.com> Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com> [uli: edited description for clarity] Signed-off-by: Ulrich Hecht <ulrich.hecht+renesas@gmail.com> --- drivers/tty/serial/sh-sci.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index 22d7a78..d5a1acb 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -3259,8 +3259,11 @@ static __maybe_unused int sci_resume(struct device *dev) { struct sci_port *sport = dev_get_drvdata(dev); - if (sport) + if (sport) { + pm_runtime_get_sync(sport->port.dev); uart_resume_port(&sci_uart_driver, &sport->port); + pm_runtime_put(sport->port.dev); + } return 0; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() 2018-04-13 17:00 [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() Ulrich Hecht 2018-04-13 17:00 ` [PATCH 1/2] serial: sh-sci: Use pm_runtime_get_sync()/put() on suspend Ulrich Hecht 2018-04-13 17:00 ` [PATCH 2/2] serial: sh-sci: Use pm_runtime_get_sync()/put() on resume Ulrich Hecht @ 2018-04-13 17:27 ` Geert Uytterhoeven 2018-06-20 5:52 ` Wolfram Sang ` (2 more replies) 2 siblings, 3 replies; 8+ messages in thread From: Geert Uytterhoeven @ 2018-04-13 17:27 UTC (permalink / raw) To: Ulrich Hecht; +Cc: Linux-Renesas, Wolfram Sang, linux-serial, Magnus Damm Hi Ulrich, On Fri, Apr 13, 2018 at 7:00 PM, Ulrich Hecht <ulrich.hecht+renesas@gmail.com> 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? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() 2018-04-13 17:27 ` [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() Geert Uytterhoeven @ 2018-06-20 5:52 ` Wolfram Sang 2018-07-12 9:00 ` Wolfram Sang 2018-09-20 6:32 ` Ulrich Hecht 2 siblings, 0 replies; 8+ messages in thread From: Wolfram Sang @ 2018-06-20 5:52 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: Ulrich Hecht, Linux-Renesas, linux-serial, Magnus Damm [-- Attachment #1: Type: text/plain, Size: 760 bytes --] > > 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? Ulrich, do you have an answer to Geert's question? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() 2018-04-13 17:27 ` [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() Geert Uytterhoeven 2018-06-20 5:52 ` Wolfram Sang @ 2018-07-12 9:00 ` Wolfram Sang 2018-09-20 6:32 ` Ulrich Hecht 2 siblings, 0 replies; 8+ messages in thread From: Wolfram Sang @ 2018-07-12 9:00 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: Ulrich Hecht, Linux-Renesas, linux-serial, Magnus Damm [-- Attachment #1: Type: text/plain, Size: 921 bytes --] On Fri, Apr 13, 2018 at 07:27:59PM +0200, Geert Uytterhoeven wrote: > Hi Ulrich, > > On Fri, Apr 13, 2018 at 7:00 PM, Ulrich Hecht > <ulrich.hecht+renesas@gmail.com> 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? Ulrich, what is your take on this? [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() 2018-04-13 17:27 ` [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() Geert Uytterhoeven 2018-06-20 5:52 ` Wolfram Sang 2018-07-12 9:00 ` Wolfram Sang @ 2018-09-20 6:32 ` Ulrich Hecht 2018-10-03 11:50 ` Wolfram Sang 2 siblings, 1 reply; 8+ messages in thread From: Ulrich Hecht @ 2018-09-20 6:32 UTC (permalink / raw) To: Geert Uytterhoeven, Ulrich Hecht Cc: Linux-Renesas, Wolfram Sang, linux-serial, Magnus Damm Sorry for the delay... > On April 13, 2018 at 7:27 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Fri, Apr 13, 2018 at 7:00 PM, Ulrich Hecht > <ulrich.hecht+renesas@gmail.com> 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() 2018-09-20 6:32 ` Ulrich Hecht @ 2018-10-03 11:50 ` Wolfram Sang 0 siblings, 0 replies; 8+ messages in thread From: Wolfram Sang @ 2018-10-03 11:50 UTC (permalink / raw) To: Ulrich Hecht Cc: Geert Uytterhoeven, Ulrich Hecht, Linux-Renesas, linux-serial, Magnus Damm [-- Attachment #1: Type: text/plain, Size: 1386 bytes --] Hey guys, I stumbled over this one again and want to make sure we have a conclusion here. > 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. I agree. > 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. Ack. > 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... So, do we agree then that *if* there needs to be a fix for that, it should be in uart_suspend_port() by adding some 'uart_change_pm' shortly before accessing the ops-callbacks? I am not super-fit with the uart layer, but can't we assume because of the "Nothing to do if the console is not suspending"-if-block that (for SCIF at least) it means the port is on because it _is_ the console? (I wonder, though, if the OMAPs won't need such a fix because they seem to use runtime_autosuspend, so their state might be off actually?) Regards, Wolfram [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-10-03 18:38 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-13 17:00 [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() Ulrich Hecht 2018-04-13 17:00 ` [PATCH 1/2] serial: sh-sci: Use pm_runtime_get_sync()/put() on suspend Ulrich Hecht 2018-04-13 17:00 ` [PATCH 2/2] serial: sh-sci: Use pm_runtime_get_sync()/put() on resume Ulrich Hecht 2018-04-13 17:27 ` [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put() Geert Uytterhoeven 2018-06-20 5:52 ` Wolfram Sang 2018-07-12 9:00 ` Wolfram Sang 2018-09-20 6:32 ` Ulrich Hecht 2018-10-03 11:50 ` Wolfram Sang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.