[PATCH 1/2] clk: qcom: gcc-sc7180: Keep USB GDSC power domain on when USB wakeup is enabled

[PATCH 1/2] clk: qcom: gcc-sc7180: Keep USB GDSC power domain on when USB wakeup is enabled

[Matthias Kaehlcke]

Set GENPD_FLAG_ACTIVE_WAKEUP for the USB GDSC power domain of SC7180.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---

 drivers/clk/qcom/gcc-sc7180.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c
index c2ea09945c47..2b6b66d01833 100644
--- a/drivers/clk/qcom/gcc-sc7180.c
+++ b/drivers/clk/qcom/gcc-sc7180.c
@@ -2223,6 +2223,7 @@ static struct gdsc usb30_prim_gdsc = {
 	.gdscr = 0x0f004,
 	.pd = {
 		.name = "usb30_prim_gdsc",
+		.flags = GENPD_FLAG_ACTIVE_WAKEUP,
 	},
 	.pwrsts = PWRSTS_OFF_ON,
 };
-- 
2.37.1.595.g718a3a8f04-goog

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

[Matthias Kaehlcke]

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,
 	},
 	.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,
-- 
2.37.1.595.g718a3a8f04-goog

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

[Johan Hovold]

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

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

[Matthias Kaehlcke]

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?

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

[Johan Hovold]

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

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

[Matthias Kaehlcke]

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.

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

[Johan Hovold]

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

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

[Matthias Kaehlcke]

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.

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

[Johan Hovold]

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

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

[Matthias Kaehlcke]

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.

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

[Matthias Kaehlcke]

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.

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

[Johan Hovold]

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

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

[Matthias Kaehlcke]

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.

Re: [PATCH 1/2] clk: qcom: gcc-sc7180: Keep USB GDSC power domain on when USB wakeup is enabled

[Stephen Boyd]

Quoting Matthias Kaehlcke (2022-08-22 11:53:10)
> Set GENPD_FLAG_ACTIVE_WAKEUP for the USB GDSC power domain of SC7180.

Why? Does it fix something? Please add details to the commit text so we
know how urgent of a patch it is and why it is important.

Re: [PATCH 1/2] clk: qcom: gcc-sc7180: Keep USB GDSC power domain on when USB wakeup is enabled

[Matthias Kaehlcke]

Hi Stephen,

On Mon, Aug 22, 2022 at 04:52:05PM -0700, Stephen Boyd wrote:
> Quoting Matthias Kaehlcke (2022-08-22 11:53:10)
> > Set GENPD_FLAG_ACTIVE_WAKEUP for the USB GDSC power domain of SC7180.
> 
> Why? Does it fix something? Please add details to the commit text so we
> know how urgent of a patch it is and why it is important.

Without it USB is broken after resume when the PHYs remain enabled
during suspend.

Johan just sent a patch [1] that always disables the PHYs (again)
during system suspend, with that this series shouldn't be needed.
It is possible that in the future we add a mechanism to keep the
PHYs on depending on some config option (quick, kconfig, ...). If
that happens we'll have to revisit this series, for now it
shouldn't be needed assuming Johan's patch gets landed.

[1] https://patchwork.kernel.org/project/linux-usb/patch/20220823124047.14634-1-johan+linaro@kernel.org/