Re: [PATCH v3 4/7] clk: qcom: gdsc: call runtime PM functions for the provider device

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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@linaro.org>
> > > ---
> > >   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