| Message ID | 20220825182152.v2.1.I45235b7c40997bc2abf813e4722b4dcdd6aecf6b@changeid (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/2] clk: qcom: gcc-sc7180: Keep the USB GDSC always on | expand |
On Thu, Aug 25, 2022 at 06:21:58PM -0700, Matthias Kaehlcke wrote: > When the GDSC is disabled during system suspend USB is broken on > sc7180 when the system resumes. Mark the GDSC as always on to > make sure USB still works after system suspend. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> Rajendra, where you able to find some time to look into take the GDSC into retention state? Do you suggest that I merge these two patches for now? Thanks, Bjorn > --- > I'm not entirely sure that this is the correct solution. What makes > me doubt is that only msm8953 sets ALWAYS_ON for the USB GDSC. Is USB > broken after suspend on all the other QC platforms? > > Changes in v2: > - set the flags of the GDSC not of the GDSC power domain > - updated commit message > > 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..c0d7509a782e 100644 > --- a/drivers/clk/qcom/gcc-sc7180.c > +++ b/drivers/clk/qcom/gcc-sc7180.c > @@ -2225,6 +2225,7 @@ static struct gdsc usb30_prim_gdsc = { > .name = "usb30_prim_gdsc", > }, > .pwrsts = PWRSTS_OFF_ON, > + .flags = ALWAYS_ON, > }; > > static struct gdsc hlos1_vote_mmnoc_mmu_tbu_hf0_gdsc = { > -- > 2.37.2.672.g94769d06f0-goog >
On 8/26/2022 8:10 AM, Bjorn Andersson wrote: > On Thu, Aug 25, 2022 at 06:21:58PM -0700, Matthias Kaehlcke wrote: >> When the GDSC is disabled during system suspend USB is broken on >> sc7180 when the system resumes. Mark the GDSC as always on to >> make sure USB still works after system suspend. >> >> Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > Rajendra, where you able to find some time to look into take the GDSC > into retention state? Do you suggest that I merge these two patches for > now? > Hi Bjorn, based on my experiments to support retention on sc7280 these are some of my findings On Platforms which support CX retention (for example sc7180/sc7280) instead of CX PowerCollapse (PC), We can leave the GDSC turned ON. When CX transitions to RET state the GDSC goes into retention too (some controller state is retained) and USB wakeups work. On platforms which support CX PC, just leaving the GDSC turned ON will not help since the GDSC will also transition to OFF state when we enter CX PC, hence wake-ups from USB won't work. For such platforms we need to make sure gdsc_force_mem_on() is called and cxcs (* @cxcs: offsets of branch registers to toggle mem/periph bits in) are populated correctly, while leaving the GDSC turned ON. This will make sure usb gdsc transitions from being powered by CX to MX when CX hits PC and we still get USB wakeups to work. So in short we could do the same thing that this patch does on those platforms too with additionally populating the right cxcs entries and it should just work fine. Now the problem that I see with this approach is not with getting USB wakeups to work in suspend, but with supporting performance state voting when USB is active. The last conclusion we had on that [1] was to model usb_gdsc as a subdomain of CX, so if we do that and we model usb_gdsc as something that supports ALWAYS_ON, we would _never_ drop the CX vote and prevent CX from going down (either to ret or pc) The only way I think we can solve both the USB wakeups and performance state needs (with usb_gdsc as a subdomain of CX) is if we can model a RET state for gdsc which sets the mem/periph bits while leaving the GDSC ON (Today the RET state sets the mem/periph bits but turns the GDSC OFF) That would mean a change in gdsc.c like this --- diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c index d3244006c661..0fe017ba901b 100644 --- a/drivers/clk/qcom/gdsc.c +++ b/drivers/clk/qcom/gdsc.c @@ -368,6 +368,10 @@ static int _gdsc_disable(struct gdsc *sc) if (sc->pwrsts & PWRSTS_OFF) gdsc_clear_mem_on(sc); + /* If the GDSC supports RET, do not explicitly power it off */ + if (sc->pwrsts & PWRSTS_RET) + return 0; + ret = gdsc_toggle_logic(sc, GDSC_OFF); if (ret) return ret; So with that change, we would then not need the ALWAYS_ON flag set for usb gdsc, instead we would update the .pwrsts to PWRSTS_RET_ON instead of PWRSTS_OFF_ON, and that should make both usb wake-ups to work and we can still have the usb_gdsc as a subdomain of CX for performance state voting. Does that sounds like a reasonable solution? Thanks, Rajendra [1] https://patchwork.kernel.org/project/linux-arm-msm/patch/1630346073-7099-2-git-send-email-sanm@codeaurora.org/ > Thanks, > Bjorn > >> --- >> I'm not entirely sure that this is the correct solution. What makes >> me doubt is that only msm8953 sets ALWAYS_ON for the USB GDSC. Is USB >> broken after suspend on all the other QC platforms? >> >> Changes in v2: >> - set the flags of the GDSC not of the GDSC power domain >> - updated commit message >> >> 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..c0d7509a782e 100644 >> --- a/drivers/clk/qcom/gcc-sc7180.c >> +++ b/drivers/clk/qcom/gcc-sc7180.c >> @@ -2225,6 +2225,7 @@ static struct gdsc usb30_prim_gdsc = { >> .name = "usb30_prim_gdsc", >> }, >> .pwrsts = PWRSTS_OFF_ON, >> + .flags = ALWAYS_ON, >> }; >> >> static struct gdsc hlos1_vote_mmnoc_mmu_tbu_hf0_gdsc = { >> -- >> 2.37.2.672.g94769d06f0-goog >>
On Mon, Aug 29, 2022 at 01:42:02PM +0530, Rajendra Nayak wrote: > > On 8/26/2022 8:10 AM, Bjorn Andersson wrote: > > On Thu, Aug 25, 2022 at 06:21:58PM -0700, Matthias Kaehlcke wrote: > > > When the GDSC is disabled during system suspend USB is broken on > > > sc7180 when the system resumes. Mark the GDSC as always on to > > > make sure USB still works after system suspend. > > > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > > > Rajendra, where you able to find some time to look into take the GDSC > > into retention state? Do you suggest that I merge these two patches for > > now? > > > > Hi Bjorn, based on my experiments to support retention on sc7280 these are > some of my findings > > On Platforms which support CX retention (for example sc7180/sc7280) instead of > CX PowerCollapse (PC), We can leave the GDSC turned ON. When CX transitions to RET state > the GDSC goes into retention too (some controller state is retained) and USB wakeups work. > > On platforms which support CX PC, just leaving the GDSC > turned ON will not help since the GDSC will also transition to OFF state > when we enter CX PC, hence wake-ups from USB won't work. > For such platforms we need to make sure gdsc_force_mem_on() is called > and cxcs (* @cxcs: offsets of branch registers to toggle mem/periph bits in) > are populated correctly, while leaving the GDSC turned ON. > This will make sure usb gdsc transitions from being powered by CX to MX > when CX hits PC and we still get USB wakeups to work. > So in short we could do the same thing that this patch does on those > platforms too with additionally populating the right cxcs entries and it > should just work fine. > > Now the problem that I see with this approach is not with getting USB wakeups > to work in suspend, but with supporting performance state voting when > USB is active. > The last conclusion we had on that [1] was to model usb_gdsc as a subdomain of CX, > so if we do that and we model usb_gdsc as something that supports ALWAYS_ON, > we would _never_ drop the CX vote and prevent CX from going down (either to ret > or pc) > > The only way I think we can solve both the USB wakeups and performance state > needs (with usb_gdsc as a subdomain of CX) is if we can model a RET state for gdsc > which sets the mem/periph bits while leaving the GDSC ON (Today the RET state sets > the mem/periph bits but turns the GDSC OFF) > > That would mean a change in gdsc.c like this > --- > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index d3244006c661..0fe017ba901b 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -368,6 +368,10 @@ static int _gdsc_disable(struct gdsc *sc) > if (sc->pwrsts & PWRSTS_OFF) > gdsc_clear_mem_on(sc); > > + /* If the GDSC supports RET, do not explicitly power it off */ > + if (sc->pwrsts & PWRSTS_RET) > + return 0; > + > ret = gdsc_toggle_logic(sc, GDSC_OFF); > if (ret) > return ret; > > > So with that change, we would then not need the ALWAYS_ON flag set for usb gdsc, > instead we would update the .pwrsts to PWRSTS_RET_ON instead of PWRSTS_OFF_ON, > and that should make both usb wake-ups to work and we can still have the usb_gdsc as > a subdomain of CX for performance state voting. Krishna and I confirmed that this works from the USB side for sc7180 and sc7280.
Quoting Rajendra Nayak (2022-08-29 01:12:02) > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index d3244006c661..0fe017ba901b 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -368,6 +368,10 @@ static int _gdsc_disable(struct gdsc *sc) > if (sc->pwrsts & PWRSTS_OFF) > gdsc_clear_mem_on(sc); > > + /* If the GDSC supports RET, do not explicitly power it off */ > + if (sc->pwrsts & PWRSTS_RET) > + return 0; > + > ret = gdsc_toggle_logic(sc, GDSC_OFF); > if (ret) > return ret; > > > So with that change, we would then not need the ALWAYS_ON flag set for usb gdsc, > instead we would update the .pwrsts to PWRSTS_RET_ON instead of PWRSTS_OFF_ON, > and that should make both usb wake-ups to work and we can still have the usb_gdsc as > a subdomain of CX for performance state voting. To clarify, usb_gdsc is not setup as a subdomain of CX so far, right? Just that eventually we'll make usb_gdsc a subdomain of CX so that dev_pm_opp_set_rate() can target the CX domain instead of the usb one? > Does that sounds like a reasonable solution? It sounds good to me. What about the existing users of PWRSTS_RET though? If I understand correctly this flag means the domain will never be turned off, instead it will hit retention during low power modes. While you're crafting this patch can you also document the PWRSTS_* defines so that we know what they mean? I can guess that PWRSTS_RET means "retention" but I don't know what it really means. I guess it means "Deepest power off state is retention of memory cells".
On 8/31/2022 3:05 AM, Stephen Boyd wrote: > Quoting Rajendra Nayak (2022-08-29 01:12:02) >> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c >> index d3244006c661..0fe017ba901b 100644 >> --- a/drivers/clk/qcom/gdsc.c >> +++ b/drivers/clk/qcom/gdsc.c >> @@ -368,6 +368,10 @@ static int _gdsc_disable(struct gdsc *sc) >> if (sc->pwrsts & PWRSTS_OFF) >> gdsc_clear_mem_on(sc); >> >> + /* If the GDSC supports RET, do not explicitly power it off */ >> + if (sc->pwrsts & PWRSTS_RET) >> + return 0; >> + >> ret = gdsc_toggle_logic(sc, GDSC_OFF); >> if (ret) >> return ret; >> >> >> So with that change, we would then not need the ALWAYS_ON flag set for usb gdsc, >> instead we would update the .pwrsts to PWRSTS_RET_ON instead of PWRSTS_OFF_ON, >> and that should make both usb wake-ups to work and we can still have the usb_gdsc as >> a subdomain of CX for performance state voting. > > To clarify, usb_gdsc is not setup as a subdomain of CX so far, right? > Just that eventually we'll make usb_gdsc a subdomain of CX so that > dev_pm_opp_set_rate() can target the CX domain instead of the usb one? Thats right, > >> Does that sounds like a reasonable solution? > > It sounds good to me. What about the existing users of PWRSTS_RET hmm, I thought no ones been actually using PWRSTS_RET but looks like there is *one* user on msm8974, I'll need to dig up how this change would affect it, > though? If I understand correctly this flag means the domain will never > be turned off, instead it will hit retention during low power modes. right, > > While you're crafting this patch can you also document the PWRSTS_* > defines so that we know what they mean? I can guess that PWRSTS_RET > means "retention" but I don't know what it really means. I guess it > means "Deepest power off state is retention of memory cells". Sure I will update the PWRSTS_RET description. The definition of 'retention' here I think remains the same, it means memory is retained (if RETAIN_MEM is set) or memory *and* some part of logic is retained (if both RETAIN_MEM and RETAIN_PERIPH are set) in Deepest power off state, however there is a disconnected in terms of 'how to enter the retention state' for a given domain. There is no SW control to enter the retention state and this happens in HW when the parent domain/rail transitions to low power. Either the parents lowest power state is also retention (like is the case with sc7180/sc7280 for usb with cx as parent) or the parent makes sure it hands the domain over to another parent (CX to MX in some cases) to support retention.
diff --git a/drivers/clk/qcom/gcc-sc7180.c b/drivers/clk/qcom/gcc-sc7180.c index c2ea09945c47..c0d7509a782e 100644 --- a/drivers/clk/qcom/gcc-sc7180.c +++ b/drivers/clk/qcom/gcc-sc7180.c @@ -2225,6 +2225,7 @@ static struct gdsc usb30_prim_gdsc = { .name = "usb30_prim_gdsc", }, .pwrsts = PWRSTS_OFF_ON, + .flags = ALWAYS_ON, }; static struct gdsc hlos1_vote_mmnoc_mmu_tbu_hf0_gdsc = {
When the GDSC is disabled during system suspend USB is broken on sc7180 when the system resumes. Mark the GDSC as always on to make sure USB still works after system suspend. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- I'm not entirely sure that this is the correct solution. What makes me doubt is that only msm8953 sets ALWAYS_ON for the USB GDSC. Is USB broken after suspend on all the other QC platforms? Changes in v2: - set the flags of the GDSC not of the GDSC power domain - updated commit message drivers/clk/qcom/gcc-sc7180.c | 1 + 1 file changed, 1 insertion(+)