| Message ID | 20230803-samsung_tty_pm_ops-v1-1-1ea7be72194d@axis.com |
|---|---|
| State | New |
| Headers | show |
| Series | tty: serial: samsung: Set missing PM ops for hibernation support | expand |
Hi Anton, On Thu, Aug 03, 2023 at 01:26:42PM +0200, Anton Eliasson wrote: > At least freeze, restore and thaw need to be set in order for the driver > to support system hibernation. The existing suspend/resume functions can > be reused since those functions don't touch the device's power state or > wakeup capability. Use the helper macros SET_SYSTEM_SLEEP_PM_OPS and > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS for symmetry with similar drivers. and why do we need hibernation in this device? Andi
On 05/08/2023 23.38, Andi Shyti wrote: > Hi Anton, > > On Thu, Aug 03, 2023 at 01:26:42PM +0200, Anton Eliasson wrote: >> At least freeze, restore and thaw need to be set in order for the driver >> to support system hibernation. The existing suspend/resume functions can >> be reused since those functions don't touch the device's power state or >> wakeup capability. Use the helper macros SET_SYSTEM_SLEEP_PM_OPS and >> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS for symmetry with similar drivers. > and why do we need hibernation in this device? > > Andi Hi! I wanted to test whether hibernation is possible on our SoC, even though it is not a common feature on embedded ARM systems. This is the only mainline driver that I found that needed modification, for my proof-of-concept anyway, and I couldn't see any harm in the change. Anton Eliasson
On 03/08/2023 13:26, Anton Eliasson wrote: > At least freeze, restore and thaw need to be set in order for the driver > to support system hibernation. The existing suspend/resume functions can > be reused since those functions don't touch the device's power state or > wakeup capability. Use the helper macros SET_SYSTEM_SLEEP_PM_OPS and > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS for symmetry with similar drivers. > Looks sensible, although you should also test the other sleep methods, e.g. suspend to idle. Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
Hi Anton, On Mon, Aug 07, 2023 at 11:57:04AM +0200, Anton Eliasson wrote: > On 05/08/2023 23.38, Andi Shyti wrote: > > Hi Anton, > > > > On Thu, Aug 03, 2023 at 01:26:42PM +0200, Anton Eliasson wrote: > > > At least freeze, restore and thaw need to be set in order for the driver > > > to support system hibernation. The existing suspend/resume functions can > > > be reused since those functions don't touch the device's power state or > > > wakeup capability. Use the helper macros SET_SYSTEM_SLEEP_PM_OPS and > > > SET_NOIRQ_SYSTEM_SLEEP_PM_OPS for symmetry with similar drivers. > > and why do we need hibernation in this device? > > > > Andi > > Hi! > > I wanted to test whether hibernation is possible on our SoC, even though it > is not a common feature on embedded ARM systems. This is the only mainline > driver that I found that needed modification, for my proof-of-concept > anyway, and I couldn't see any harm in the change. Thanks, makes sense, mine was just curiosity, can I know which SoC you are testing that is using the samsung serial device? You can add my r-b, anyway: Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
On 07/08/2023 15.50, Krzysztof Kozlowski wrote: > On 03/08/2023 13:26, Anton Eliasson wrote: >> At least freeze, restore and thaw need to be set in order for the driver >> to support system hibernation. The existing suspend/resume functions can >> be reused since those functions don't touch the device's power state or >> wakeup capability. Use the helper macros SET_SYSTEM_SLEEP_PM_OPS and >> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS for symmetry with similar drivers. >> > Looks sensible, although you should also test the other sleep methods, > e.g. suspend to idle. > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Best regards, > Krzysztof > Yes, s2idle still works. Standby / Power-On Suspend and s2ram we don't have support for so I can't test it. Thanks for the review! Anton Eliasson
On 07/08/2023 23.34, Andi Shyti wrote: > Hi Anton, > > On Mon, Aug 07, 2023 at 11:57:04AM +0200, Anton Eliasson wrote: >> On 05/08/2023 23.38, Andi Shyti wrote: >>> Hi Anton, >>> >>> On Thu, Aug 03, 2023 at 01:26:42PM +0200, Anton Eliasson wrote: >>>> At least freeze, restore and thaw need to be set in order for the driver >>>> to support system hibernation. The existing suspend/resume functions can >>>> be reused since those functions don't touch the device's power state or >>>> wakeup capability. Use the helper macros SET_SYSTEM_SLEEP_PM_OPS and >>>> SET_NOIRQ_SYSTEM_SLEEP_PM_OPS for symmetry with similar drivers. >>> and why do we need hibernation in this device? >>> >>> Andi >> Hi! >> >> I wanted to test whether hibernation is possible on our SoC, even though it >> is not a common feature on embedded ARM systems. This is the only mainline >> driver that I found that needed modification, for my proof-of-concept >> anyway, and I couldn't see any harm in the change. > Thanks, makes sense, mine was just curiosity, can I know which > SoC you are testing that is using the samsung serial device? > > You can add my r-b, anyway: > > Reviewed-by: Andi Shyti <andi.shyti@kernel.org> > > Thanks, > Andi It's the Axis Communications ARTPEC-8, an SoC for surveillance cameras: https://www.axis.com/solutions/system-on-chip Thanks for the review! Anton Eliasson
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c index b29e9dfd81a6..e2247c11067d 100644 --- a/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c @@ -2273,9 +2273,8 @@ static int s3c24xx_serial_resume_noirq(struct device *dev) } static const struct dev_pm_ops s3c24xx_serial_pm_ops = { - .suspend = s3c24xx_serial_suspend, - .resume = s3c24xx_serial_resume, - .resume_noirq = s3c24xx_serial_resume_noirq, + SET_SYSTEM_SLEEP_PM_OPS(s3c24xx_serial_suspend, s3c24xx_serial_resume) + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(NULL, s3c24xx_serial_resume_noirq) }; #define SERIAL_SAMSUNG_PM_OPS (&s3c24xx_serial_pm_ops)
At least freeze, restore and thaw need to be set in order for the driver to support system hibernation. The existing suspend/resume functions can be reused since those functions don't touch the device's power state or wakeup capability. Use the helper macros SET_SYSTEM_SLEEP_PM_OPS and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS for symmetry with similar drivers. Signed-off-by: Anton Eliasson <anton.eliasson@axis.com> --- I have not investigated the impact of adding the additional noirq handler functions. The hardware that I tested on (Axis ARTPEC-8) appears to work both with and without them assigned. Other similar drivers that use noirq handlers assign them to both resume, thaw and restore so I follow that style also. --- drivers/tty/serial/samsung_tty.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) --- base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5 change-id: 20230731-samsung_tty_pm_ops-7c98f309a4e9 Best regards,