| Message ID | 20230808072625.2109564-1-nick.hu@sifive.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] serial: sifive: Add suspend and resume operations | expand |
On Tue, Aug 08, 2023 at 03:26:25PM +0800, Nick Hu wrote: > If the Sifive Uart is not used as the wake up source, suspend the uart > before the system enter the suspend state to prevent it woken up by > unexpected uart interrupt. Resume the uart once the system woken up. > > Signed-off-by: Nick Hu <nick.hu@sifive.com> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> Where is patch 2/2?
On 08/08/2023 08:26, Nick Hu wrote: > If the Sifive Uart is not used as the wake up source, suspend the uart > before the system enter the suspend state to prevent it woken up by > unexpected uart interrupt. Resume the uart once the system woken up. > > Signed-off-by: Nick Hu <nick.hu@sifive.com> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> ^ This should be Reviewed-by, as I did review on this earlier. > --- > drivers/tty/serial/sifive.c | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c > index a19db49327e2..87994cb69007 100644 > --- a/drivers/tty/serial/sifive.c > +++ b/drivers/tty/serial/sifive.c > @@ -1022,6 +1022,31 @@ static int sifive_serial_remove(struct platform_device *dev) > return 0; > } > > +static int sifive_serial_suspend(struct device *dev) > +{ > + int ret = 0; > + struct sifive_serial_port *ssp = dev_get_drvdata(dev); Minor annyonance is ordering of ssp and ret, I think the showrter one last is the nicest looking. > + if (ssp && ssp->port.type != PORT_UNKNOWN) > + ret = uart_suspend_port(&sifive_serial_uart_driver, &ssp->port); Do we really need a test for ssp being valid if the device is bound. Not sure if the port.type is also useful? > + return ret; > +} > + > +static int sifive_serial_resume(struct device *dev) > +{ > + int ret = 0; > + struct sifive_serial_port *ssp = dev_get_drvdata(dev); > + > + if (ssp && ssp->port.type != PORT_UNKNOWN) > + ret = uart_resume_port(&sifive_serial_uart_driver, &ssp->port); > + > + return ret; > +} > + > +DEFINE_SIMPLE_DEV_PM_OPS(sifive_uart_pm_ops, sifive_serial_suspend, > + sifive_serial_resume); > + > static const struct of_device_id sifive_serial_of_match[] = { > { .compatible = "sifive,fu540-c000-uart0" }, > { .compatible = "sifive,uart0" }, > @@ -1034,6 +1059,7 @@ static struct platform_driver sifive_serial_platform_driver = { > .remove = sifive_serial_remove, > .driver = { > .name = SIFIVE_SERIAL_NAME, > + .pm = pm_sleep_ptr(&sifive_uart_pm_ops), > .of_match_table = of_match_ptr(sifive_serial_of_match), > }, > };
Hi Conor On Tue, Aug 8, 2023 at 4:44 PM Conor Dooley <conor@kernel.org> wrote: > > On Tue, Aug 08, 2023 at 03:26:25PM +0800, Nick Hu wrote: > > If the Sifive Uart is not used as the wake up source, suspend the uart > > before the system enter the suspend state to prevent it woken up by > > unexpected uart interrupt. Resume the uart once the system woken up. > > > > Signed-off-by: Nick Hu <nick.hu@sifive.com> > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > > Where is patch 2/2? This was my bad. There is no patch 2/2. I'll correct it in V2. Nick
Hi Ben On Tue, Aug 8, 2023 at 4:47 PM Ben Dooks <ben.dooks@codethink.co.uk> wrote: > > On 08/08/2023 08:26, Nick Hu wrote: > > If the Sifive Uart is not used as the wake up source, suspend the uart > > before the system enter the suspend state to prevent it woken up by > > unexpected uart interrupt. Resume the uart once the system woken up. > > > > Signed-off-by: Nick Hu <nick.hu@sifive.com> > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > > ^ This should be Reviewed-by, as I did review on this earlier. > I'll correct it in V2 > > --- > > drivers/tty/serial/sifive.c | 26 ++++++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c > > index a19db49327e2..87994cb69007 100644 > > --- a/drivers/tty/serial/sifive.c > > +++ b/drivers/tty/serial/sifive.c > > @@ -1022,6 +1022,31 @@ static int sifive_serial_remove(struct platform_device *dev) > > return 0; > > } > > > > +static int sifive_serial_suspend(struct device *dev) > > +{ > > + int ret = 0; > > + struct sifive_serial_port *ssp = dev_get_drvdata(dev); > > Minor annyonance is ordering of ssp and ret, I think the showrter one > last is the nicest looking. > > > + if (ssp && ssp->port.type != PORT_UNKNOWN) > > + ret = uart_suspend_port(&sifive_serial_uart_driver, &ssp->port); > > Do we really need a test for ssp being valid if the device is bound. > Not sure if the port.type is also useful? > You are right. This might be an unnecessary check. The ssp is bound in the probe function and the PORT_UNKNOWN only be set when we do the sifive_serial_remove(). And for the ordering of ret, We don't need the ret if we move the check. We can just return uart_suspend_port(&sifive_serial_uart_driver, &ssp->port); Will correct it in V2. > > + return ret; > > +} > > + > > +static int sifive_serial_resume(struct device *dev) > > +{ > > + int ret = 0; > > + struct sifive_serial_port *ssp = dev_get_drvdata(dev); > > + > > + if (ssp && ssp->port.type != PORT_UNKNOWN) > > + ret = uart_resume_port(&sifive_serial_uart_driver, &ssp->port); > > + > > + return ret; > > +} > > + > > +DEFINE_SIMPLE_DEV_PM_OPS(sifive_uart_pm_ops, sifive_serial_suspend, > > + sifive_serial_resume); > > + > > static const struct of_device_id sifive_serial_of_match[] = { > > { .compatible = "sifive,fu540-c000-uart0" }, > > { .compatible = "sifive,uart0" }, > > @@ -1034,6 +1059,7 @@ static struct platform_driver sifive_serial_platform_driver = { > > .remove = sifive_serial_remove, > > .driver = { > > .name = SIFIVE_SERIAL_NAME, > > + .pm = pm_sleep_ptr(&sifive_uart_pm_ops), > > .of_match_table = of_match_ptr(sifive_serial_of_match), > > }, > > }; > > -- > Ben Dooks http://www.codethink.co.uk/ > Senior Engineer Codethink - Providing Genius > > https://www.codethink.co.uk/privacy.html >
diff --git a/drivers/tty/serial/sifive.c b/drivers/tty/serial/sifive.c index a19db49327e2..87994cb69007 100644 --- a/drivers/tty/serial/sifive.c +++ b/drivers/tty/serial/sifive.c @@ -1022,6 +1022,31 @@ static int sifive_serial_remove(struct platform_device *dev) return 0; } +static int sifive_serial_suspend(struct device *dev) +{ + int ret = 0; + struct sifive_serial_port *ssp = dev_get_drvdata(dev); + + if (ssp && ssp->port.type != PORT_UNKNOWN) + ret = uart_suspend_port(&sifive_serial_uart_driver, &ssp->port); + + return ret; +} + +static int sifive_serial_resume(struct device *dev) +{ + int ret = 0; + struct sifive_serial_port *ssp = dev_get_drvdata(dev); + + if (ssp && ssp->port.type != PORT_UNKNOWN) + ret = uart_resume_port(&sifive_serial_uart_driver, &ssp->port); + + return ret; +} + +DEFINE_SIMPLE_DEV_PM_OPS(sifive_uart_pm_ops, sifive_serial_suspend, + sifive_serial_resume); + static const struct of_device_id sifive_serial_of_match[] = { { .compatible = "sifive,fu540-c000-uart0" }, { .compatible = "sifive,uart0" }, @@ -1034,6 +1059,7 @@ static struct platform_driver sifive_serial_platform_driver = { .remove = sifive_serial_remove, .driver = { .name = SIFIVE_SERIAL_NAME, + .pm = pm_sleep_ptr(&sifive_uart_pm_ops), .of_match_table = of_match_ptr(sifive_serial_of_match), }, };