Re: [RFC 2/4] clk: qcom: gdsc: enable external switchable power domain

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

On Fri 11 Sep 08:09 CDT 2020, Dmitry Baryshkov wrote:

> Some GDSCs (SM8250's MDSS_GDSC for example) need switchable power domain
> to be on to be able to access hardware registers. Use dev_pm/opp to
                           ^
What you describe here ----+ sounds like the GDSC controller is part of
the power-domain specified and hence needs to be enabled in order to
control the GDSC.

But in contrast what the patch implements is a mechanism where the
GDSC power-domain is a child of some other power-domain. So the commit
message needs to better reflect what's implemented.

Then looking at the DT representation I think it says that the
controller sits in the specified power-domain, rather than the exposed
power-domain...

> enable corresponding power domain.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/gdsc.c | 56 ++++++++++++++++++++++++++++++++++++++---
>  drivers/clk/qcom/gdsc.h |  5 ++++
>  2 files changed, 57 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index bfc4ac02f9ea..a522e062a79a 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_opp.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/reset-controller.h>
> @@ -110,13 +111,31 @@ static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
>  	return -ETIMEDOUT;
>  }
>  
> +int gdsc_toggle_on(struct gdsc *sc)

This should be "static" and I think you should include "supply" in the
name to denote that it doesn't turn on the gdsc, but rather its supply.

> +{
> +	if (sc->rsupply)
> +		return regulator_enable(sc->rsupply);
> +	if (sc->pd_dev)
> +		return dev_pm_genpd_set_performance_state(sc->pd_dev, sc->pd_opp);
> +	return 0;
> +}
> +
> +int gdsc_toggle_off(struct gdsc *sc)
> +{
> +	if (sc->pd_dev)
> +		return dev_pm_genpd_set_performance_state(sc->pd_dev, 0);
> +	if (sc->rsupply)
> +		return regulator_disable(sc->rsupply);
> +	return 0;
> +}
> +
>  static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
>  {
>  	int ret;
>  	u32 val = (status == GDSC_ON) ? 0 : SW_COLLAPSE_MASK;
>  
> -	if (status == GDSC_ON && sc->rsupply) {
> -		ret = regulator_enable(sc->rsupply);
> +	if (status == GDSC_ON) {
> +		ret = gdsc_toggle_on(sc);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -153,8 +172,8 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
>  	ret = gdsc_poll_status(sc, status);
>  	WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n");
>  
> -	if (!ret && status == GDSC_OFF && sc->rsupply) {
> -		ret = regulator_disable(sc->rsupply);
> +	if (!ret && status == GDSC_OFF) {
> +		ret = gdsc_toggle_off(sc);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -407,6 +426,27 @@ int gdsc_register(struct gdsc_desc *desc,
>  			return PTR_ERR(scs[i]->rsupply);
>  	}
>  
> +	for (i = 0; i < num; i++) {
> +		if (!scs[i] || !scs[i]->domain)
> +			continue;
> +
> +		scs[i]->pd_opp = of_get_required_opp_performance_state(dev->of_node, scs[i]->perf_idx);
> +		if (scs[i]->pd_opp < 0)
> +			return scs[i]->pd_opp;
> +
> +		scs[i]->pd_dev = dev_pm_domain_attach_by_name(dev, scs[i]->domain);
> +		if (IS_ERR(scs[i]->pd_dev)) {
> +			ret = PTR_ERR(scs[i]->pd_dev);
> +			/* Single domain has been already attached, so reuse dev */
> +			if (ret == -EEXIST) {
> +				scs[i]->pd_dev = dev;
> +			} else {
> +				scs[i]->pd_dev = NULL;
> +				goto pm_detach;
> +			}
> +		}
> +	}
> +
>  	data->num_domains = num;
>  	for (i = 0; i < num; i++) {
>  		if (!scs[i])
> @@ -428,6 +468,12 @@ int gdsc_register(struct gdsc_desc *desc,
>  	}
>  
>  	return of_genpd_add_provider_onecell(dev->of_node, data);
> +
> +pm_detach:
> +	for (i = 0; i < num; i++)
> +		if (scs[i]->pd_dev)
> +			dev_pm_domain_detach(scs[i]->pd_dev, false);

I think that if dev_pm_domain_attach_by_name() returned -EEXIST you
will attempt to detach the main device's domain here.

> +	return ret;
>  }
>  
>  void gdsc_unregister(struct gdsc_desc *desc)
> @@ -443,6 +489,8 @@ void gdsc_unregister(struct gdsc_desc *desc)
>  			continue;
>  		if (scs[i]->parent)
>  			pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
> +		if (scs[i]->pd_dev && scs[i]->pd_dev != dev)
> +			dev_pm_domain_detach(scs[i]->pd_dev, true);

Ditto

Regards,
Bjorn

>  	}
>  	of_genpd_del_provider(dev->of_node);
>  }
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index bd537438c793..d58575f8f25f 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -57,6 +57,11 @@ struct gdsc {
>  
>  	const char 			*supply;
>  	struct regulator		*rsupply;
> +
> +	const char			*domain;
> +	unsigned int			perf_idx;
> +	struct device			*pd_dev;
> +	int				pd_opp;
>  };
>  
>  struct gdsc_desc {
> -- 
> 2.28.0
>