On Fri, Nov 29, 2024 at 01:06:49PM +0000, Bryan O'Donoghue wrote:
> When a clock-controller has multiple power-domains we need to attach
> parent GDSCs in that clock-controller as subdomains of each of the
> power-domains.
I envision a fair number of current and future readers will wonder why
this is... Why do we _need_ to perform this attachment?
>
> Testing on the x1e80100 reference shows that both power-domains need to be
> switched on for the GDSCs in the clock-controller to work.
You're making a completely generic change, but are referring here to
some specific (probably camera) case.
> Some open
> questions remain.
>
> 1. Should there be a hirearchy of power-domains in the clock-controller.
Your TITAN_TOP patches is already an example of such hierarchy, so I
don't think that's an open question.
> 2. If there should be no hirearchy should the parent GDSC inside the
> clock-controller attach to each power-domain in the clock-controller.
I suspect that the TITAN_TOP gives you this impression that there is a
"parent GDSC"; that's generally not the case - but the mechanism
introduced by the patch is still needed.
> 3. If there are multiple parent GDSCs in a clock-controller do we attach
> those top-level GDSCs to each controller power-domain.
> 4. Finally should performance-states be applied equally across those
> power-domains.
>
> It may be if we see more clock-controllers with multiple power-domains that
> some mixture of these questions will need to be implemented for specific
> hardware.
GPUCC has always been an example of this and there are several other
examples in multimedia, which we've just ignored. And even for GCC you
have a mix of voltage requirements cast across CX and MX.
> Right now the approach taken here is to attach the
> clock-controller GDSC parent to each clock-controller power-domain.
>
What is "the clock-controller GDSC parent"? Perhaps I'm just parsing
this incorrectly?
Perhaps we can use the naming from the API and say "each GDSC is put in
the subdomain of all power-domains of the clock-controller"?
I'm not convinced that the propagation of set_performance_state has been
adequately been understood.
But, in general, I'm not against the idea of starting off by voting on
all rails, mentioning that it's likely that in some cases more effective
propagation of votes can be made and then leave this as a future
exercise.
I would like to see 1-2 use cases beyond camcc being exposed to this
though.
Regards,
Bjorn
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> ---
> drivers/clk/qcom/common.c | 1 +
> drivers/clk/qcom/gdsc.c | 35 +++++++++++++++++++++++++++++++++++
> drivers/clk/qcom/gdsc.h | 1 +
> 3 files changed, 37 insertions(+)
>
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index b79e6a73b53a4113ca324d102d7be5504a9fe85e..9e3380fd718198c9fe63d7361615a91c3ecb3d60 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -323,6 +323,7 @@ int qcom_cc_really_probe(struct device *dev,
> scd->dev = dev;
> scd->scs = desc->gdscs;
> scd->num = desc->num_gdscs;
> + scd->pd_list = cc->pd_list;
> ret = gdsc_register(scd, &reset->rcdev, regmap);
> if (ret)
> return ret;
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 4fc6f957d0b846cc90e50ef243f23a7a27e66899..cb4afa6d584899f3dafa380d5e01be6de9711737 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -506,6 +506,36 @@ static int gdsc_init(struct gdsc *sc)
> return ret;
> }
>
> +static int gdsc_add_subdomain_list(struct dev_pm_domain_list *pd_list,
> + struct generic_pm_domain *subdomain)
> +{
> + int i, ret;
> +
> + for (i = 0; i < pd_list->num_pds; i++) {
> + struct device *dev = pd_list->pd_devs[i];
> + struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +
> + ret = pm_genpd_add_subdomain(genpd, subdomain);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void gdsc_remove_subdomain_list(struct dev_pm_domain_list *pd_list,
> + struct generic_pm_domain *subdomain)
> +{
> + int i;
> +
> + for (i = 0; i < pd_list->num_pds; i++) {
> + struct device *dev = pd_list->pd_devs[i];
> + struct generic_pm_domain *genpd = pd_to_genpd(dev->pm_domain);
> +
> + pm_genpd_remove_subdomain(genpd, subdomain);
> + }
> +}
> +
> int gdsc_register(struct gdsc_desc *desc,
> struct reset_controller_dev *rcdev, struct regmap *regmap)
> {
> @@ -558,6 +588,9 @@ int gdsc_register(struct gdsc_desc *desc,
> ret = pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
> else if (!IS_ERR_OR_NULL(dev->pm_domain))
> ret = pm_genpd_add_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
> + else if (desc->pd_list)
> + ret = gdsc_add_subdomain_list(desc->pd_list, &scs[i]->pd);
> +
> if (ret)
> return ret;
> }
> @@ -580,6 +613,8 @@ void gdsc_unregister(struct gdsc_desc *desc)
> pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
> else if (!IS_ERR_OR_NULL(dev->pm_domain))
> pm_genpd_remove_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
> + else if (desc->pd_list)
> + gdsc_remove_subdomain_list(desc->pd_list, &scs[i]->pd);
> }
> of_genpd_del_provider(dev->of_node);
> }
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index 1e2779b823d1c8ca077c9b4cd0a0dbdf5f9457ef..dd843e86c05b2f30e6d9e978681580016333839d 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -80,6 +80,7 @@ struct gdsc_desc {
> struct device *dev;
> struct gdsc **scs;
> size_t num;
> + struct dev_pm_domain_list *pd_list;
> };
>
> #ifdef CONFIG_QCOM_GDSC
>
> --
> 2.45.2
>
