On Fri 09 Jul 17:10 CDT 2021, Dmitry Baryshkov wrote:
> On 09/07/2021 21:54, Bjorn Andersson wrote:
> > On Fri 09 Jul 12:31 CDT 2021, Dmitry Baryshkov wrote:
> >
> > > In order to properly handle runtime PM status of the provider device,
> > > call pm_runtime_get/pm_runtime_put on the clock controller device.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > ---
> > > drivers/clk/qcom/gdsc.c | 66 ++++++++++++++++++++++++++++++++++++++---
> > > drivers/clk/qcom/gdsc.h | 2 ++
> > > 2 files changed, 64 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > index ccd36617d067..6bec31fccb09 100644
> > > --- a/drivers/clk/qcom/gdsc.c
> > > +++ b/drivers/clk/qcom/gdsc.c
> > > @@ -11,6 +11,7 @@
> > > #include <linux/kernel.h>
> > > #include <linux/ktime.h>
> > > #include <linux/pm_domain.h>
> > > +#include <linux/pm_runtime.h>
> > > #include <linux/regmap.h>
> > > #include <linux/regulator/consumer.h>
> > > #include <linux/reset-controller.h>
> > > @@ -50,6 +51,30 @@ enum gdsc_status {
> > > GDSC_ON
> > > };
> > > +static int gdsc_pm_runtime_get(struct gdsc *sc)
> > > +{
> > > + int ret;
> > > +
> > > + if (!sc->rpm_dev)
> > > + return 0;
> > > +
> > > + ret = pm_runtime_get_sync(sc->rpm_dev);
> > > + if (ret < 0) {
> > > + pm_runtime_put_noidle(sc->rpm_dev);
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int gdsc_pm_runtime_put(struct gdsc *sc)
> > > +{
> > > + if (!sc->rpm_dev)
> > > + return 0;
> > > +
> > > + return pm_runtime_put_sync(sc->rpm_dev);
> > > +}
> > > +
> > > /* Returns 1 if GDSC status is status, 0 if not, and < 0 on error */
> > > static int gdsc_check_status(struct gdsc *sc, enum gdsc_status status)
> > > {
> > > @@ -232,9 +257,8 @@ static void gdsc_retain_ff_on(struct gdsc *sc)
> > > regmap_update_bits(sc->regmap, sc->gdscr, mask, mask);
> > > }
> > > -static int gdsc_enable(struct generic_pm_domain *domain)
> > > +static int _gdsc_enable(struct gdsc *sc)
> > > {
> > > - struct gdsc *sc = domain_to_gdsc(domain);
> > > int ret;
> > > if (sc->pwrsts == PWRSTS_ON)
> > > @@ -290,11 +314,28 @@ static int gdsc_enable(struct generic_pm_domain *domain)
> > > return 0;
> > > }
> > > -static int gdsc_disable(struct generic_pm_domain *domain)
> > > +static int gdsc_enable(struct generic_pm_domain *domain)
> > > {
> > > struct gdsc *sc = domain_to_gdsc(domain);
> > > int ret;
> > > + ret = gdsc_pm_runtime_get(sc);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = _gdsc_enable(sc);
> > > + if (ret) {
> > > + gdsc_pm_runtime_put(sc);
> >
> > I presume what you do here is to leave the pm_runtime state of dispcc
> > active if we succeeded in enabling the gdsc. But the gdsc is a subdomain
> > of the parent domain, so the framework should take case of its
> > dependency.
> >
> > So the reason for gdsc_pm_runtime_get()/put() in this code path is so
> > that you can access the dispcc registers, i.e. I think you should
> > get()/put() regardless of the return value.
>
> pm domain code will handle enabling MMCX, so this code is not required
> strictly speaking. Ulf suggested adding it back, so I followed the
> suggestion. Maybe I misunderstood his suggestion.
>
> putting pm_runtime after gdsc_enable does not sound like a logical case.
> However it would simplify code a bit. Let me try...
>
If you consider this device's and the individual gdsc's power state as
separate consumers of MMCX, then it perhaps makes more sense?
Similar to how a regulator driver would rely on the regulator framework
to deal with parent regulators...
Regards,
Bjorn
> >
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int _gdsc_disable(struct gdsc *sc)
> > > +{
> > > + int ret;
> > > +
> > > if (sc->pwrsts == PWRSTS_ON)
> > > return gdsc_assert_reset(sc);
> > > @@ -329,6 +370,18 @@ static int gdsc_disable(struct generic_pm_domain *domain)
> > > return 0;
> > > }
> > > +static int gdsc_disable(struct generic_pm_domain *domain)
> > > +{
> > > + struct gdsc *sc = domain_to_gdsc(domain);
> > > + int ret;
> > > +
> >
> > If the gdsc is found to be on at initialization, the next operation that
> > will happen is gdsc_disable() and as you didn't activate the pm_runtime
> > state in gdsc_init() you would in theory get here with registers
> > unaccessible.
> >
> > In practice though, the active gdsc should through the being a subdomain
> > of the parent domain keep power on for you, so you won't notice this
> > issue.
>
> Nice catch.
>
> >
> > But as above, I think you should wrap _gdsc_disable() in a get()/put()
> > pair.
> >
> > > + ret = _gdsc_disable(sc);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return gdsc_pm_runtime_put(sc);
> > > +}
> > > +
> > > static int gdsc_init(struct gdsc *sc)
> > > {
> > > u32 mask, val;
> > > @@ -425,6 +478,8 @@ int gdsc_register(struct gdsc_desc *desc,
> > > for (i = 0; i < num; i++) {
> > > if (!scs[i])
> > > continue;
> > > + if (pm_runtime_enabled(dev))
> > > + scs[i]->rpm_dev = dev;
> > > scs[i]->regmap = regmap;
> > > scs[i]->rcdev = rcdev;
> > > ret = gdsc_init(scs[i]);
> > > @@ -486,7 +541,10 @@ void gdsc_unregister(struct gdsc_desc *desc)
> > > */
> > > int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain)
> > > {
> > > + struct gdsc *sc = domain_to_gdsc(domain);
> > > +
> > > /* Do nothing but give genpd the impression that we were successful */
> > > - return 0;
> > > + /* Get the runtime PM device only */
> > > + return gdsc_pm_runtime_get(sc);
> >
> > Per above, if you let the framework deal with the gdsc's dependencies on
> > the parent domain and you only get()/put() for the sake of dispcc then
> > you don't need you don't need to do this to keep the subsequent
> > gdsc_disable() in balance.
> >
> > > }
> > > EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable);
> > > diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> > > index 5bb396b344d1..a82982df0a55 100644
> > > --- a/drivers/clk/qcom/gdsc.h
> > > +++ b/drivers/clk/qcom/gdsc.h
> > > @@ -25,6 +25,7 @@ struct reset_controller_dev;
> > > * @resets: ids of resets associated with this gdsc
> > > * @reset_count: number of @resets
> > > * @rcdev: reset controller
> > > + * @rpm_dev: runtime PM device
> > > */
> > > struct gdsc {
> > > struct generic_pm_domain pd;
> > > @@ -58,6 +59,7 @@ struct gdsc {
> > > const char *supply;
> > > struct regulator *rsupply;
> > > + struct device *rpm_dev;
> >
> > This isn't just the "runtime pm device", it's the device this gdsc is
> > associated with. So "dev" sounds sufficient to me, but that requires
> > that you have a separate bool rpm_enabled to remember if
> > pm_runtime_enabled() was true during probe.
> >
> > So unless we need "dev" for something else this might be sufficient.
> >
> > Regards,
> > Bjorn
> >
> > > };
> > > struct gdsc_desc {
> > > --
> > > 2.30.2
> > >
>
>
> --
> With best wishes
> Dmitry
