diff mbox series

[2/2] clk: qcom: gcc-sc7280: Keep USB GDSC power domains on when USB wakeup is enabled

Message ID 20220822115246.2.If09027f73daa6e1ed95f5eab02326b543c67132e@changeid
State New
Headers show
Series [1/2] clk: qcom: gcc-sc7180: Keep USB GDSC power domain on when USB wakeup is enabled | expand

Commit Message

Matthias Kaehlcke Aug. 22, 2022, 6:53 p.m. UTC
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(+)

Comments

Johan Hovold Aug. 23, 2022, 11:16 a.m. UTC | #1
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?

This is the case for sc8280xp and indicates that there are further
missing pieces here (at least for that platform).

Johan
Matthias Kaehlcke Aug. 23, 2022, 4:42 p.m. UTC | #2
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.

> 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?
Johan Hovold Aug. 24, 2022, 8:47 a.m. UTC | #3
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.

> > 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.

Wakeup itself notably works either way.

Johan
Matthias Kaehlcke Aug. 25, 2022, 10:16 p.m. UTC | #4
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 wonder what the status on other QC platforms is, from a quick grep it
seems only msm8953 sets the USB gdsc to ALWAYS_ON.
Johan Hovold Aug. 26, 2022, 7:12 a.m. UTC | #5
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.

Yes, GENPD_FLAG_ALWAYS_ON needs to be set via the gdsc ALWAYS_ON flag as
genpd expects the PD to be on when registered or else it silently errors
out...

	https://lore.kernel.org/all/20220805121250.10347-3-johan+linaro@kernel.org/

> > > > 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.

Thanks for confirming.
 
> 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 wonder what the status on other QC platforms is, from a quick grep it
> seems only msm8953 sets the USB gdsc to ALWAYS_ON.

Yes, this is likely needed on other platforms as well.

Johan
Matthias Kaehlcke Aug. 26, 2022, 1:05 p.m. UTC | #6
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.
Johan Hovold Aug. 26, 2022, 1:12 p.m. UTC | #7
On Fri, Aug 26, 2022 at 06:05:19AM -0700, Matthias Kaehlcke wrote:
> 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:

> > > 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.

Ok, so to be clear: if you disable wakeup with my patches applied so
that the PD is actually turned off, USB still resumes with only
GENPD_FLAG_ACTIVE_WAKEUP set?

That is, you don't need to set ALWAYS_ON as I do with sc8280xp?

Johan
Matthias Kaehlcke Aug. 26, 2022, 1:19 p.m. UTC | #8
On Fri, Aug 26, 2022 at 03:12:05PM +0200, Johan Hovold wrote:
> On Fri, Aug 26, 2022 at 06:05:19AM -0700, Matthias Kaehlcke wrote:
> > 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:
> 
> > > > 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.
> 
> Ok, so to be clear: if you disable wakeup with my patches applied so
> that the PD is actually turned off, USB still resumes with only
> GENPD_FLAG_ACTIVE_WAKEUP set?
> 
> That is, you don't need to set ALWAYS_ON as I do with sc8280xp?

Correct

The USB controller isn't entirely happy:

[   23.062936] xhci-hcd xhci-hcd.13.auto: xHC error in resume, USBSTS 0x411, Reinit

but USB still works.
Matthias Kaehlcke Aug. 26, 2022, 1:24 p.m. UTC | #9
On Fri, Aug 26, 2022 at 06:19:37AM -0700, Matthias Kaehlcke wrote:
> On Fri, Aug 26, 2022 at 03:12:05PM +0200, Johan Hovold wrote:
> > On Fri, Aug 26, 2022 at 06:05:19AM -0700, Matthias Kaehlcke wrote:
> > > 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:
> > 
> > > > > 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.
> > 
> > Ok, so to be clear: if you disable wakeup with my patches applied so
> > that the PD is actually turned off, USB still resumes with only
> > GENPD_FLAG_ACTIVE_WAKEUP set?

Actually without GENPD_FLAG_ACTIVE_WAKEUP

> > That is, you don't need to set ALWAYS_ON as I do with sc8280xp?
> 
> Correct
> 
> The USB controller isn't entirely happy:
> 
> [   23.062936] xhci-hcd xhci-hcd.13.auto: xHC error in resume, USBSTS 0x411, Reinit
> 
> but USB still works.
Johan Hovold Aug. 26, 2022, 1:26 p.m. UTC | #10
On Fri, Aug 26, 2022 at 06:24:37AM -0700, Matthias Kaehlcke wrote:
> On Fri, Aug 26, 2022 at 06:19:37AM -0700, Matthias Kaehlcke wrote:
> > On Fri, Aug 26, 2022 at 03:12:05PM +0200, Johan Hovold wrote:
> > > On Fri, Aug 26, 2022 at 06:05:19AM -0700, Matthias Kaehlcke wrote:
> > > > 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:
> > > 
> > > > > > 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.
> > > 
> > > Ok, so to be clear: if you disable wakeup with my patches applied so
> > > that the PD is actually turned off, USB still resumes with only
> > > GENPD_FLAG_ACTIVE_WAKEUP set?
> 
> Actually without GENPD_FLAG_ACTIVE_WAKEUP

Sure, that only comes into play with wakeup enabled. I assume you still
need it for USB wakeup to work?

Johan
Matthias Kaehlcke Aug. 26, 2022, 1:33 p.m. UTC | #11
On Fri, Aug 26, 2022 at 03:26:51PM +0200, Johan Hovold wrote:
> On Fri, Aug 26, 2022 at 06:24:37AM -0700, Matthias Kaehlcke wrote:
> > On Fri, Aug 26, 2022 at 06:19:37AM -0700, Matthias Kaehlcke wrote:
> > > On Fri, Aug 26, 2022 at 03:12:05PM +0200, Johan Hovold wrote:
> > > > On Fri, Aug 26, 2022 at 06:05:19AM -0700, Matthias Kaehlcke wrote:
> > > > > 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:
> > > > 
> > > > > > > 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.
> > > > 
> > > > Ok, so to be clear: if you disable wakeup with my patches applied so
> > > > that the PD is actually turned off, USB still resumes with only
> > > > GENPD_FLAG_ACTIVE_WAKEUP set?
> > 
> > Actually without GENPD_FLAG_ACTIVE_WAKEUP
> 
> Sure, that only comes into play with wakeup enabled. I assume you still
> need it for USB wakeup to work?

Yes, for USB wakeup to work either setting the above genpd flag or (better)
the ALWAYS_ON flag of the GDSC is needed.
diff mbox series

Patch

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,
 	},
 	.pwrsts = PWRSTS_OFF_ON,
 	.flags = VOTABLE,
@@ -3134,6 +3135,7 @@  static struct gdsc gcc_usb30_sec_gdsc = {
 	.gdscr = 0x9e004,
 	.pd = {
 		.name = "gcc_usb30_sec_gdsc",
+		.flags = GENPD_FLAG_ACTIVE_WAKEUP,
 	},
 	.pwrsts = PWRSTS_OFF_ON,
 	.flags = VOTABLE,