| Message ID | 20241126-b4-linux-next-24-11-18-clock-multiple-power-domains-v3-0-836dad33521a@linaro.org |
|---|---|
| Headers | show |
| Series | clk: qcom: Add support for multiple power-domains for a clock controller. | expand |
On Tue, Nov 26, 2024 at 11:44:27PM +0000, Bryan O'Donoghue wrote: > Adding a new clause to this if/else I noticed the existing usage of > pm_genpd_add_subdomain() wasn't capturing and returning the result code. > > pm_genpd_add_subdomain() returns and int and can fail. Capture that result (note to myself?) Drop the 'd' in "an int". > code and throw it up the call stack if something goes wrong. > > Fixes: 1b771839de05 ("clk: qcom: gdsc: enable optional power domain support") > Cc: stable@vger.kernel.org > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Reviewed-by: Bjorn Andersson <andersson@kernel.org> Regards, Bjorn > --- > drivers/clk/qcom/gdsc.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index fa5fe4c2a2ee7786c2e8858f3e41301f639e5d59..4fc6f957d0b846cc90e50ef243f23a7a27e66899 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -555,9 +555,11 @@ int gdsc_register(struct gdsc_desc *desc, > if (!scs[i]) > continue; > if (scs[i]->parent) > - pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd); > + ret = pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd); > else if (!IS_ERR_OR_NULL(dev->pm_domain)) > - pm_genpd_add_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd); > + ret = pm_genpd_add_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd); > + if (ret) > + return ret; > } > > return of_genpd_add_provider_onecell(dev->of_node, data); > > -- > 2.45.2 >
On Tue, Nov 26, 2024 at 11:44:29PM +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. > This is a bit sparse, in particular it would be nice to capture the open questions about whether every GDSC always should be parented by all defined power-domains, and if performance-state should be applied equally across all those power-domains (and/or if this actually happens). PS. Please drop "drivers: " and s/subdomain list/GDSC/ in subject. Regards, Bjorn > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > 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 7727295c57c8f6672d46d2380e1ff5ec2ac68d42..58a8397eefe51da237a4285d4e7cee967e19948f 100644 > --- a/drivers/clk/qcom/common.c > +++ b/drivers/clk/qcom/common.c > @@ -338,6 +338,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 >
v3: - Fixes commit log "per which" - Bryan - Link to v2: https://lore.kernel.org/r/20241125-b4-linux-next-24-11-18-clock-multiple-power-domains-v2-0-a5e7554d7e45@linaro.org v2: The main change in this version is Bjorn's pointing out that pm_runtime_* inside of the gdsc_enable/gdsc_disable path would be recursive and cause a lockdep splat. Dmitry alluded to this too. Bjorn pointed to stuff being done lower in the gdsc_register() routine that might be a starting point. I iterated around that idea and came up with patch #3. When a gdsc has no parent and the pd_list is non-NULL then attach that orphan GDSC to the clock controller power-domain list. Existing subdomain code in gdsc_register() will connect the parent GDSCs in the clock-controller to the clock-controller subdomain, the new code here does that same job for a list of power-domains the clock controller depends on. To Dmitry's point about MMCX and MCX dependencies for the registers inside of the clock controller, I have switched off all references in a test dtsi and confirmed that accessing the clock-controller regs themselves isn't required. On the second point I also verified my test branch with lockdep on which was a concern with the pm_domain version of this solution but I wanted to cover it anyway with the new approach for completeness sake. Here's the item-by-item list of changes: - Adds a patch to capture pm_genpd_add_subdomain() result code - Bryan - Changes changelog of second patch to remove singleton and generally to make the commit log easier to understand - Bjorn - Uses demv_pm_domain_attach_list - Vlad - Changes error check to if (ret < 0 && ret != -EEXIST) - Vlad - Retains passing &pd_data instead of NULL - because NULL doesn't do the same thing - Bryan/Vlad - Retains standalone function qcom_cc_pds_attach() because the pd_data enumeration looks neater in a standalone function - Bryan/Vlad - Drops pm_runtime in favour of gdsc_add_subdomain_list() for each power-domain in the pd_list. The pd_list will be whatever is pointed to by power-domains = <> in the dtsi - Bjorn - Link to v1: https://lore.kernel.org/r/20241118-b4-linux-next-24-11-18-clock-multiple-power-domains-v1-0-b7a2bd82ba37@linaro.org v1: On x1e80100 and it's SKUs the Camera Clock Controller - CAMCC has multiple power-domains which power it. Usually with a single power-domain the core platform code will automatically switch on the singleton power-domain for you. If you have multiple power-domains for a device, in this case the clock controller, you need to switch those power-domains on/off yourself. The clock controllers can also contain Global Distributed Switch Controllers - GDSCs which themselves can be referenced from dtsi nodes ultimately triggering a gdsc_en() in drivers/clk/qcom/gdsc.c. As an example: cci0: cci@ac4a000 { power-domains = <&camcc TITAN_TOP_GDSC>; }; This series adds the support to attach a power-domain list to the clock-controllers and the GDSCs those controllers provide so that in the case of the above example gdsc_toggle_logic() will trigger the power-domain list with pm_runtime_resume_and_get() and pm_runtime_put_sync() respectively. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- Bryan O'Donoghue (3): clk: qcom: gdsc: Capture pm_genpd_add_subdomain result code clk: qcom: common: Add support for power-domain attachment driver: clk: qcom: Support attaching subdomain list to multiple parents drivers/clk/qcom/common.c | 21 +++++++++++++++++++++ drivers/clk/qcom/gdsc.c | 41 +++++++++++++++++++++++++++++++++++++++-- drivers/clk/qcom/gdsc.h | 1 + 3 files changed, 61 insertions(+), 2 deletions(-) --- base-commit: 744cf71b8bdfcdd77aaf58395e068b7457634b2c change-id: 20241118-b4-linux-next-24-11-18-clock-multiple-power-domains-a5f994dc452a Best regards,