While exiting deep sleep, serial port loses its configuration
hence it prints garbage characters on console.
Set serial port configuration while resume from deep sleep.
Signed-off-by: Mehul Raninga <[email protected]>
---
drivers/tty/serial/qcom_geni_serial.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 8582479f0211..c04b8fec30ba 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -20,6 +20,7 @@
#include <linux/serial.h>
#include <linux/serial_core.h>
#include <linux/slab.h>
+#include <linux/suspend.h>
#include <linux/tty.h>
#include <linux/tty_flip.h>
#include <dt-bindings/interconnect/qcom,icc.h>
@@ -1737,6 +1738,8 @@ static int qcom_geni_serial_sys_resume(struct device *dev)
if (uart_console(uport)) {
geni_icc_set_tag(&port->se, QCOM_ICC_TAG_ALWAYS);
geni_icc_set_bw(&port->se);
+ if (pm_suspend_via_firmware())
+ qcom_geni_serial_port_setup(uport);
}
return ret;
}
--
2.17.1
On Tue, May 30, 2023 at 04:45:57PM +0530, Mehul Raninga wrote:
> While exiting deep sleep, serial port loses its configuration
> hence it prints garbage characters on console.
Presumably it lost its configuration sometime after suspend, rather than
while resuming the system?
>
> Set serial port configuration while resume from deep sleep.
>
What happens if you do this unconditionally?
> Signed-off-by: Mehul Raninga <[email protected]>
> ---
> drivers/tty/serial/qcom_geni_serial.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 8582479f0211..c04b8fec30ba 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -20,6 +20,7 @@
> #include <linux/serial.h>
> #include <linux/serial_core.h>
> #include <linux/slab.h>
> +#include <linux/suspend.h>
> #include <linux/tty.h>
> #include <linux/tty_flip.h>
> #include <dt-bindings/interconnect/qcom,icc.h>
> @@ -1737,6 +1738,8 @@ static int qcom_geni_serial_sys_resume(struct device *dev)
> if (uart_console(uport)) {
> geni_icc_set_tag(&port->se, QCOM_ICC_TAG_ALWAYS);
> geni_icc_set_bw(&port->se);
> + if (pm_suspend_via_firmware())
I'm not familiar with this api, but aren't all our systems implementing
firmware-assisted suspend?
Regards,
Bjorn
> + qcom_geni_serial_port_setup(uport);
> }
> return ret;
> }
> --
> 2.17.1
>
Hello Andersson,
Thanks for the review. Kindly find my reply inline below
> -----Original Message-----
> From: Bjorn Andersson <[email protected]>
> Sent: Tuesday, May 30, 2023 8:37 PM
> To: Mehul Raninga (Temp) (QUIC) <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Viken Dadhaniya (QUIC)
> <[email protected]>; Visweswara Tanuku (QUIC)
> <[email protected]>; Vijaya Krishna Nivarthi (Temp) (QUIC)
> <[email protected]>
> Subject: Re: [PATCH] serial: qcom_geni_serial: Setup serial port after Deep sleep
>
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
>
> On Tue, May 30, 2023 at 04:45:57PM +0530, Mehul Raninga wrote:
> > While exiting deep sleep, serial port loses its configuration hence it
> > prints garbage characters on console.
>
> Presumably it lost its configuration sometime after suspend, rather than while
> resuming the system?
I will reword commit message in V2 as below:
Serial port lost its configuration sometime after suspend hence it prints garbage characters on console after resuming.
>
> >
> > Set serial port configuration while resume from deep sleep.
> >
>
> What happens if you do this unconditionally?
pm_suspend_via_firmware returns true indicating system is resuming from deepsleep. In case we are not resuming from deepsleep, this serial port setup is not required.
>
> > Signed-off-by: Mehul Raninga <[email protected]>
> > ---
> > drivers/tty/serial/qcom_geni_serial.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c
> > b/drivers/tty/serial/qcom_geni_serial.c
> > index 8582479f0211..c04b8fec30ba 100644
> > --- a/drivers/tty/serial/qcom_geni_serial.c
> > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > @@ -20,6 +20,7 @@
> > #include <linux/serial.h>
> > #include <linux/serial_core.h>
> > #include <linux/slab.h>
> > +#include <linux/suspend.h>
> > #include <linux/tty.h>
> > #include <linux/tty_flip.h>
> > #include <dt-bindings/interconnect/qcom,icc.h>
> > @@ -1737,6 +1738,8 @@ static int qcom_geni_serial_sys_resume(struct
> device *dev)
> > if (uart_console(uport)) {
> > geni_icc_set_tag(&port->se, QCOM_ICC_TAG_ALWAYS);
> > geni_icc_set_bw(&port->se);
> > + if (pm_suspend_via_firmware())
>
> I'm not familiar with this api, but aren't all our systems implementing firmware-
> assisted suspend?
Not all the platform supports deep sleep hence to differentiate if resume is from deep sleep suspend or normal suspend, this api is required.
>
> Regards,
> Bjorn
>
> > + qcom_geni_serial_port_setup(uport);
> > }
> > return ret;
> > }
> > --
> > 2.17.1
> >
On Wed, May 31, 2023 at 01:06:22PM +0000, Mehul Raninga (Temp) (QUIC) wrote:
> Hello Andersson,
> Thanks for the review. Kindly find my reply inline below
>
> > -----Original Message-----
> > From: Bjorn Andersson <[email protected]>
> > Sent: Tuesday, May 30, 2023 8:37 PM
> > To: Mehul Raninga (Temp) (QUIC) <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; linux-
> > [email protected]; [email protected]; Viken Dadhaniya (QUIC)
> > <[email protected]>; Visweswara Tanuku (QUIC)
> > <[email protected]>; Vijaya Krishna Nivarthi (Temp) (QUIC)
> > <[email protected]>
> > Subject: Re: [PATCH] serial: qcom_geni_serial: Setup serial port after Deep sleep
> >
> > WARNING: This email originated from outside of Qualcomm. Please be wary of
> > any links or attachments, and do not enable macros.
> >
> > On Tue, May 30, 2023 at 04:45:57PM +0530, Mehul Raninga wrote:
> > > While exiting deep sleep, serial port loses its configuration hence it
> > > prints garbage characters on console.
> >
> > Presumably it lost its configuration sometime after suspend, rather than while
> > resuming the system?
>
> I will reword commit message in V2 as below:
> Serial port lost its configuration sometime after suspend hence it
> prints garbage characters on console after resuming.
Please wrap your replies to 72 chars wide
You can probably be more specific than "sometime after suspend" :)
> >
> > >
> > > Set serial port configuration while resume from deep sleep.
> > >
> >
> > What happens if you do this unconditionally?
>
> pm_suspend_via_firmware returns true indicating system is resuming
> from deepsleep. In case we are not resuming from deepsleep, this
> serial port setup is not required.
> >
> > > Signed-off-by: Mehul Raninga <[email protected]>
> > > ---
> > > drivers/tty/serial/qcom_geni_serial.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/qcom_geni_serial.c
> > > b/drivers/tty/serial/qcom_geni_serial.c
> > > index 8582479f0211..c04b8fec30ba 100644
> > > --- a/drivers/tty/serial/qcom_geni_serial.c
> > > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > > @@ -20,6 +20,7 @@
> > > #include <linux/serial.h>
> > > #include <linux/serial_core.h>
> > > #include <linux/slab.h>
> > > +#include <linux/suspend.h>
> > > #include <linux/tty.h>
> > > #include <linux/tty_flip.h>
> > > #include <dt-bindings/interconnect/qcom,icc.h>
> > > @@ -1737,6 +1738,8 @@ static int qcom_geni_serial_sys_resume(struct
> > device *dev)
> > > if (uart_console(uport)) {
> > > geni_icc_set_tag(&port->se, QCOM_ICC_TAG_ALWAYS);
> > > geni_icc_set_bw(&port->se);
> > > + if (pm_suspend_via_firmware())
> >
> > I'm not familiar with this api, but aren't all our systems implementing firmware-
> > assisted suspend?
>
> Not all the platform supports deep sleep hence to differentiate if
> resume is from deep sleep suspend or normal suspend, this api is
> required.
Can you point me to where this difference in flags is coming from in the
upstream kernel?
Thanks,
Bjorn
> >
> > Regards,
> > Bjorn
> >
> > > + qcom_geni_serial_port_setup(uport);
> > > }
> > > return ret;
> > > }
> > > --
> > > 2.17.1
> > >
Hello Andersson,
Thanks for the review. Kindly find my reply inline below.
> -----Original Message-----
> From: Bjorn Andersson <[email protected]>
> Sent: Thursday, June 1, 2023 9:38 AM
> To: Mehul Raninga (Temp) (QUIC) <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; linux-arm-
> [email protected]; [email protected]; linux-
> [email protected]; Viken Dadhaniya (QUIC)
> <[email protected]>; Visweswara Tanuku (QUIC)
> <[email protected]>; Vijaya Krishna Nivarthi (Temp) (QUIC)
> <[email protected]>
> Subject: Re: [PATCH] serial: qcom_geni_serial: Setup serial port after Deep
> sleep
>
> WARNING: This email originated from outside of Qualcomm. Please be
> wary of any links or attachments, and do not enable macros.
>
> On Wed, May 31, 2023 at 01:06:22PM +0000, Mehul Raninga (Temp)
> (QUIC) wrote:
> > Hello Andersson,
> > Thanks for the review. Kindly find my reply inline below
> >
> > > -----Original Message-----
> > > From: Bjorn Andersson <[email protected]>
> > > Sent: Tuesday, May 30, 2023 8:37 PM
> > > To: Mehul Raninga (Temp) (QUIC) <[email protected]>
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; linux- [email protected];
> > > [email protected]; Viken Dadhaniya (QUIC)
> > > <[email protected]>; Visweswara Tanuku (QUIC)
> > > <[email protected]>; Vijaya Krishna Nivarthi (Temp) (QUIC)
> > > <[email protected]>
> > > Subject: Re: [PATCH] serial: qcom_geni_serial: Setup serial port
> > > after Deep sleep
> > >
> > > WARNING: This email originated from outside of Qualcomm. Please be
> > > wary of any links or attachments, and do not enable macros.
> > >
> > > On Tue, May 30, 2023 at 04:45:57PM +0530, Mehul Raninga wrote:
> > > > While exiting deep sleep, serial port loses its configuration
> > > > hence it prints garbage characters on console.
> > >
> > > Presumably it lost its configuration sometime after suspend, rather
> > > than while resuming the system?
> >
> > I will reword commit message in V2 as below:
> > Serial port lost its configuration sometime after suspend hence it
> > prints garbage characters on console after resuming.
>
> Please wrap your replies to 72 chars wide
Noted
>
> You can probably be more specific than "sometime after suspend" :)
During deepsleep HW configurations are lost, hence it prints garbage
characters on console after resuming.
>
> > >
> > > >
> > > > Set serial port configuration while resume from deep sleep.
> > > >
> > >
> > > What happens if you do this unconditionally?
> >
> > pm_suspend_via_firmware returns true indicating system is resuming
> > from deepsleep. In case we are not resuming from deepsleep, this
> > serial port setup is not required.
> > >
> > > > Signed-off-by: Mehul Raninga <[email protected]>
> > > > ---
> > > > drivers/tty/serial/qcom_geni_serial.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/tty/serial/qcom_geni_serial.c
> > > > b/drivers/tty/serial/qcom_geni_serial.c
> > > > index 8582479f0211..c04b8fec30ba 100644
> > > > --- a/drivers/tty/serial/qcom_geni_serial.c
> > > > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > > > @@ -20,6 +20,7 @@
> > > > #include <linux/serial.h>
> > > > #include <linux/serial_core.h>
> > > > #include <linux/slab.h>
> > > > +#include <linux/suspend.h>
> > > > #include <linux/tty.h>
> > > > #include <linux/tty_flip.h>
> > > > #include <dt-bindings/interconnect/qcom,icc.h>
> > > > @@ -1737,6 +1738,8 @@ static int
> > > > qcom_geni_serial_sys_resume(struct
> > > device *dev)
> > > > if (uart_console(uport)) {
> > > > geni_icc_set_tag(&port->se, QCOM_ICC_TAG_ALWAYS);
> > > > geni_icc_set_bw(&port->se);
> > > > + if (pm_suspend_via_firmware())
> > >
> > > I'm not familiar with this api, but aren't all our systems
> > > implementing firmware- assisted suspend?
> >
> > Not all the platform supports deep sleep hence to differentiate if
> > resume is from deep sleep suspend or normal suspend, this api is
> > required.
>
> Can you point me to where this difference in flags is coming from in the
> upstream kernel?
In upstream kernel by default the flag is false, and for deepsleep
support vendor kernel sets this flag true.
>
> Thanks,
> Bjorn
>
> > >
> > > Regards,
> > > Bjorn
> > >
> > > > + qcom_geni_serial_port_setup(uport);
> > > > }
> > > > return ret;
> > > > }
> > > > --
> > > > 2.17.1
> > > >
On Fri, Jun 02, 2023 at 06:39:02AM +0000, Mehul Raninga (Temp) (QUIC) wrote:
[..]
> > On Wed, May 31, 2023 at 01:06:22PM +0000, Mehul Raninga (Temp)
> > (QUIC) wrote:
[..]
> > > > > + if (pm_suspend_via_firmware())
> > > >
> > > > I'm not familiar with this api, but aren't all our systems
> > > > implementing firmware- assisted suspend?
> > >
> > > Not all the platform supports deep sleep hence to differentiate if
> > > resume is from deep sleep suspend or normal suspend, this api is
> > > required.
> >
> > Can you point me to where this difference in flags is coming from in the
> > upstream kernel?
>
> In upstream kernel by default the flag is false, and for deepsleep
> support vendor kernel sets this flag true.
I am questioning whether this is correct, given that we indeed "suspend
with the help of firmware". But I might be misinterpreting what that
statement means.
Regards,
Bjorn