Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 31 May 2023 21:07:34 -0700
From:   Bjorn Andersson <andersson@kernel.org>
To:     "Mehul Raninga (Temp) (QUIC)" <quic_mraninga@quicinc.com>
Cc:     "agross@kernel.org" <agross@kernel.org>,
        "konrad.dybcio@linaro.org" <konrad.dybcio@linaro.org>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "jirislaby@kernel.org" <jirislaby@kernel.org>,
        "linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>,
        "linux-serial@vger.kernel.org" <linux-serial@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Viken Dadhaniya (QUIC)" <quic_vdadhani@quicinc.com>,
        "Visweswara Tanuku (QUIC)" <quic_vtanuku@quicinc.com>,
        "Vijaya Krishna Nivarthi (Temp) (QUIC)" <quic_vnivarth@quicinc.com>
Subject: Re: [PATCH] serial: qcom_geni_serial: Setup serial port after Deep
 sleep
Message-ID: <20230601040734.qiy3ibrpznrqptui@ripper>
References: <20230530111557.10944-1-quic_mraninga@quicinc.com>
 <20230530150631.7eg6nkre6sva5dcz@ripper>
 <SJ0PR02MB7742A7386189692ACACC0A3FE3489@SJ0PR02MB7742.namprd02.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <SJ0PR02MB7742A7386189692ACACC0A3FE3489@SJ0PR02MB7742.namprd02.prod.outlook.com>
Precedence: bulk

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 <andersson@kernel.org>
> > Sent: Tuesday, May 30, 2023 8:37 PM
> > To: Mehul Raninga (Temp) (QUIC) <quic_mraninga@quicinc.com>
> > Cc: agross@kernel.org; konrad.dybcio@linaro.org; gregkh@linuxfoundation.org;
> > jirislaby@kernel.org; linux-arm-msm@vger.kernel.org; linux-
> > serial@vger.kernel.org; linux-kernel@vger.kernel.org; Viken Dadhaniya (QUIC)
> > <quic_vdadhani@quicinc.com>; Visweswara Tanuku (QUIC)
> > <quic_vtanuku@quicinc.com>; Vijaya Krishna Nivarthi (Temp) (QUIC)
> > <quic_vnivarth@quicinc.com>
> > 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 <quic_mraninga@quicinc.com>
> > > ---
> > >  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
> > >