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:   Fri, 26 Aug 2022 06:05:19 -0700
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Johan Hovold <johan@kernel.org>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>, linux-arm-msm@vger.kernel.org,
        Johan Hovold <johan+linaro@kernel.org>,
        Douglas Anderson <dianders@chromium.org>,
        Krishna Kurapati <quic_kriskura@quicinc.com>,
        linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org,
        Bjorn Andersson <bjorn.andersson@linaro.org>
Subject: Re: [PATCH 2/2] clk: qcom: gcc-sc7280: Keep USB GDSC power domains
 on when USB wakeup is enabled
Message-ID: <YwjFD9uHnSxoZHvT@google.com>
References: <20220822115246.1.I45235b7c40997bc2abf813e4722b4dcdd6aecf6b@changeid>
 <20220822115246.2.If09027f73daa6e1ed95f5eab02326b543c67132e@changeid>
 <YwS3FCOqIeajMEgz@hovoldconsulting.com>
 <YwUDjaG6n95Ddij2@google.com>
 <YwXlsK3pjK/q1xwO@hovoldconsulting.com>
 <Ywf02oIXEL8G/Heo@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Ywf02oIXEL8G/Heo@google.com>
Precedence: bulk

On Thu, Aug 25, 2022 at 03:16:58PM -0700, Matthias Kaehlcke wrote:
> On Wed, Aug 24, 2022 at 10:47:44AM +0200, Johan Hovold wrote:
> > On Tue, Aug 23, 2022 at 09:42:53AM -0700, Matthias Kaehlcke wrote:
> > > On Tue, Aug 23, 2022 at 01:16:36PM +0200, Johan Hovold wrote:
> > > > On Mon, Aug 22, 2022 at 11:53:11AM -0700, Matthias Kaehlcke wrote:
> > > > > Set GENPD_FLAG_ACTIVE_WAKEUP for the USB GDSC power domains of SC7280.
> > > > > 
> > > > > Suggested-by: Johan Hovold <johan+linaro@kernel.org>
> > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > > ---
> > > > > 
> > > > >  drivers/clk/qcom/gcc-sc7280.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
> > > > > index 7ff64d4d5920..4ff855269467 100644
> > > > > --- a/drivers/clk/qcom/gcc-sc7280.c
> > > > > +++ b/drivers/clk/qcom/gcc-sc7280.c
> > > > > @@ -3125,6 +3125,7 @@ static struct gdsc gcc_usb30_prim_gdsc = {
> > > > >  	.gdscr = 0xf004,
> > > > >  	.pd = {
> > > > >  		.name = "gcc_usb30_prim_gdsc",
> > > > > +		.flags = GENPD_FLAG_ACTIVE_WAKEUP,
> > > > 
> > > > Have you verified that the power-domain doesn't need to remain on also
> > > > when USB isn't used for wakeup?
> > > 
> > > So far I haven't observed issues with this on sc7180 and sc7280 when USB
> > > wakeup is disabled.
> > 
> > Ok, good.
> > 
> > I would have assumed that it needed to stay always-on before the
> > s/device_can_wakeup/device_may_wakeup/ change as before that the PHYs
> > would be left on regardless of the (sysfs) wakeup setting.
> 
> That make sense, not sure why GENPD_FLAG_ACTIVE_WAKEUP works with wakeup
> disabled.
> 
> In any case setting the genpd flags directly in the gdsc descriptor doesn't
> seem to be the right thing to do. With GENPD_FLAG_ALWAYS_ON my sc7280 system
> stalls at boot. It boots when ALWAYS_ON is set in the gdsc flags, which ends
> up setting GENPD_FLAG_ALWAYS_ON of the genpd. I'll send a new version of this
> series which sets the correct flag.
> 
> > > > This is the case for sc8280xp and indicates that there are further
> > > > missing pieces here (at least for that platform).
> > > 
> > > What are you observing on sc8280xp when wakeup is disabled?
> > 
> > The wakeup setting doesn't seem to have anything to do with the genpd
> > issues on sc8280xp and the controller doesn't resume properly regardless
> > of whether the PHYs have been disabled or not during suspend unless the
> > PD is left on.
> 
> I'm essentially seeing the same. USB is hosed after resume unless the PD
> is left on.
> 
> On Chrome OS we currently work around that with a version of commit
> d9be8d5c5b03 ("usb: dwc3: qcom: Keep power domain on to retain controller
> status") which was reverted upstream. I'm not sure whether USB worked after
> resume before we enabled wakeup support. I would have sworn it did, but we
> landed an old version of the wakeup patches a long time ago, so my
> memory might be failing me.

I need to remind myself that keeping the GDSC on is only needed when the PHYs
are kept on. The PHYs were always off before wakeup support was added, which
is why USB wasn't broken after suspend in the pre-wakeup days.

> I wonder what the status on other QC platforms is, from a quick grep it
> seems only msm8953 sets the USB gdsc to ALWAYS_ON.