| Message ID | 20210625225414.1318338-1-bjorn.andersson@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: qcom: gdsc: Ensure regulator init state matches GDSC state | expand |
On 25-06-21, 15:54, Bjorn Andersson wrote: > As GDSCs are registered and found to be already enabled > gdsc_toggle_logic() will be invoked for votable GDSCs and ensure that > the vote is matching the hardware state. Part of this the related > regulator will be enabled. > > But for non-votable GDSCs the regulator and GDSC status will be out of > sync and as the GDSC is later disabled regulator_disable() will face an > unbalanced enable-count, or something might turn off the supply under > the feet of the GDSC. > > So ensure that the regulator is enabled even for non-votable GDSCs. Reviewed-by: Vinod Koul <vkoul@kernel.org>
Quoting Bjorn Andersson (2021-06-25 15:54:14) > As GDSCs are registered and found to be already enabled > gdsc_toggle_logic() will be invoked for votable GDSCs and ensure that > the vote is matching the hardware state. Part of this the related > regulator will be enabled. > > But for non-votable GDSCs the regulator and GDSC status will be out of > sync and as the GDSC is later disabled regulator_disable() will face an > unbalanced enable-count, or something might turn off the supply under > the feet of the GDSC. > > So ensure that the regulator is enabled even for non-votable GDSCs. > > Cc: stable@vger.kernel.org > Fixes: 37416e554961 ("clk: qcom: gdsc: Handle GDSC regulator supplies") > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/clk/qcom/gdsc.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index 51ed640e527b..f7e7759cdb90 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -359,10 +359,17 @@ static int gdsc_init(struct gdsc *sc) > > /* > * Votable GDSCs can be ON due to Vote from other masters. > - * If a Votable GDSC is ON, make sure we have a Vote. > + * If a Votable GDSC is ON, make sure we have a Vote. If > + * non-votable, ensure that the supply is kept enabled (as > + * is done by gdsc_enable). > */ > - if ((sc->flags & VOTABLE) && on) > + if ((sc->flags & VOTABLE) && on) { > gdsc_enable(&sc->pd); > + } else if (on) { > + ret = regulator_enable(sc->rsupply); > + if (ret < 0) > + return ret; Looking at this makes me think we've messed something up with gdsc_enable() being called or cherry-picking the regulator enable (and other stuff in this gdsc_init()) out of the enable path. Maybe we should have a followup patch that replaces the gdsc_enable() with gdsc_toggle_logic(sc, GDSC_ON) so that we know it isn't doing anything else during init like asserting a reset when presumably all we want to do is toggle the enable bit to assert our vote. And I notice that we already call gdsc_toggle_logic() in gdsc_init(), so then we'll have a double regulator_enable() in the case of PWRSTS_ON? And then if the flag is ALWAYS_ON we'll call regulator_enable() yet again, but luckily only if it isn't on initially, phew! This code is quite twisted. It would be super nice to make it more like if (on) { /* It was on in hardware, sync kernel state */ regulator_enable(); if (votable) write bit, why do any wait? if (retain ff) write bit } else if (always_on) { /* Force on */ gdsc_enable(); on = true; } if (on || ...) > + } > > /* > * Make sure the retain bit is set if the GDSC is already on, otherwise
On Fri 25 Jun 17:54 CDT 2021, Bjorn Andersson wrote: > As GDSCs are registered and found to be already enabled > gdsc_toggle_logic() will be invoked for votable GDSCs and ensure that > the vote is matching the hardware state. Part of this the related > regulator will be enabled. > > But for non-votable GDSCs the regulator and GDSC status will be out of > sync and as the GDSC is later disabled regulator_disable() will face an > unbalanced enable-count, or something might turn off the supply under > the feet of the GDSC. > > So ensure that the regulator is enabled even for non-votable GDSCs. > > Cc: stable@vger.kernel.org > Fixes: 37416e554961 ("clk: qcom: gdsc: Handle GDSC regulator supplies") > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/clk/qcom/gdsc.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index 51ed640e527b..f7e7759cdb90 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -359,10 +359,17 @@ static int gdsc_init(struct gdsc *sc) > > /* > * Votable GDSCs can be ON due to Vote from other masters. > - * If a Votable GDSC is ON, make sure we have a Vote. > + * If a Votable GDSC is ON, make sure we have a Vote. If > + * non-votable, ensure that the supply is kept enabled (as > + * is done by gdsc_enable). > */ > - if ((sc->flags & VOTABLE) && on) > + if ((sc->flags & VOTABLE) && on) { > gdsc_enable(&sc->pd); > + } else if (on) { I somehow lost the check for sc->rsupply != NULL while trying to make this readable. In other words, this is broken. Regards, Bjorn > + ret = regulator_enable(sc->rsupply); > + if (ret < 0) > + return ret; > + } > > /* > * Make sure the retain bit is set if the GDSC is already on, otherwise > -- > 2.29.2 >
On Tue 29 Jun 01:34 CDT 2021, Stephen Boyd wrote: > Quoting Bjorn Andersson (2021-06-25 15:54:14) > > As GDSCs are registered and found to be already enabled > > gdsc_toggle_logic() will be invoked for votable GDSCs and ensure that > > the vote is matching the hardware state. Part of this the related > > regulator will be enabled. > > > > But for non-votable GDSCs the regulator and GDSC status will be out of > > sync and as the GDSC is later disabled regulator_disable() will face an > > unbalanced enable-count, or something might turn off the supply under > > the feet of the GDSC. > > > > So ensure that the regulator is enabled even for non-votable GDSCs. > > > > Cc: stable@vger.kernel.org > > Fixes: 37416e554961 ("clk: qcom: gdsc: Handle GDSC regulator supplies") > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > drivers/clk/qcom/gdsc.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > > index 51ed640e527b..f7e7759cdb90 100644 > > --- a/drivers/clk/qcom/gdsc.c > > +++ b/drivers/clk/qcom/gdsc.c > > @@ -359,10 +359,17 @@ static int gdsc_init(struct gdsc *sc) > > > > /* > > * Votable GDSCs can be ON due to Vote from other masters. > > - * If a Votable GDSC is ON, make sure we have a Vote. > > + * If a Votable GDSC is ON, make sure we have a Vote. If > > + * non-votable, ensure that the supply is kept enabled (as > > + * is done by gdsc_enable). > > */ > > - if ((sc->flags & VOTABLE) && on) > > + if ((sc->flags & VOTABLE) && on) { > > gdsc_enable(&sc->pd); > > + } else if (on) { > > + ret = regulator_enable(sc->rsupply); > > + if (ret < 0) > > + return ret; > > Looking at this makes me think we've messed something up with > gdsc_enable() being called or cherry-picking the regulator enable (and > other stuff in this gdsc_init()) out of the enable path. Maybe we should > have a followup patch that replaces the gdsc_enable() with > gdsc_toggle_logic(sc, GDSC_ON) so that we know it isn't doing anything > else during init like asserting a reset when presumably all we want to > do is toggle the enable bit to assert our vote. > > And I notice that we already call gdsc_toggle_logic() in gdsc_init(), so > then we'll have a double regulator_enable() in the case of PWRSTS_ON? > And then if the flag is ALWAYS_ON we'll call regulator_enable() yet > again, but luckily only if it isn't on initially, phew! This code is > quite twisted. > > It would be super nice to make it more like > > if (on) { > /* It was on in hardware, sync kernel state */ > regulator_enable(); > > if (votable) > write bit, why do any wait? > > if (retain ff) > write bit > } else if (always_on) { > /* Force on */ > gdsc_enable(); > on = true; > } > > if (on || ...) > This does look cleaner, I will dig through the logic here once more before figuring out what to do for v2. Thanks, Bjorn > > + } > > > > /* > > * Make sure the retain bit is set if the GDSC is already on, otherwise
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c index 51ed640e527b..f7e7759cdb90 100644 --- a/drivers/clk/qcom/gdsc.c +++ b/drivers/clk/qcom/gdsc.c @@ -359,10 +359,17 @@ static int gdsc_init(struct gdsc *sc) /* * Votable GDSCs can be ON due to Vote from other masters. - * If a Votable GDSC is ON, make sure we have a Vote. + * If a Votable GDSC is ON, make sure we have a Vote. If + * non-votable, ensure that the supply is kept enabled (as + * is done by gdsc_enable). */ - if ((sc->flags & VOTABLE) && on) + if ((sc->flags & VOTABLE) && on) { gdsc_enable(&sc->pd); + } else if (on) { + ret = regulator_enable(sc->rsupply); + if (ret < 0) + return ret; + } /* * Make sure the retain bit is set if the GDSC is already on, otherwise
As GDSCs are registered and found to be already enabled gdsc_toggle_logic() will be invoked for votable GDSCs and ensure that the vote is matching the hardware state. Part of this the related regulator will be enabled. But for non-votable GDSCs the regulator and GDSC status will be out of sync and as the GDSC is later disabled regulator_disable() will face an unbalanced enable-count, or something might turn off the supply under the feet of the GDSC. So ensure that the regulator is enabled even for non-votable GDSCs. Cc: stable@vger.kernel.org Fixes: 37416e554961 ("clk: qcom: gdsc: Handle GDSC regulator supplies") Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- drivers/clk/qcom/gdsc.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)