[PATCH v9 0/4] clk: qcom: Add support for multiple power-domains for a clock controller.
Changes in v9: - Added patch to unwind pm subdomains in reverse order. It would also be possible to squash this patch into patch#2 but, my own preference is for more granular patches like this instead of "slipping in" functional changes in larger patches like #2. - bod - Unwinding pm subdomain on error in patch #2. To facilitate this change patch #1 was created - Vlad - Drops Bjorn's RB on patch #2. There is a small churn in this patch but enough that a reviewer might reasonably expect RB to be given again. - Amends commit log for patch #3 further. v8 added a lot to the commit log to provide further information but, it is clear from the comments I received on the commit log that the added verbiage was occlusive not elucidative. Reduce down the commit log of patch #3 - especially Q&A item #1. Sometimes less is more. - Link to v8: https://lore.kernel.org/r/20241211-b4-linux-next-24-11-18-clock-multiple-pow...
Changes in v8: - Picks up change I agreed with Vlad but failed to cherry-pick into my b4 tree - Vlad/Bod - Rewords the commit log for patch #3. As I read it I decided I might translate bits of it from thought-stream into English - Bod - Link to v7: https://lore.kernel.org/r/20241211-b4-linux-next-24-11-18-clock-multiple-pow...
Changes in v7: - Expand commit log in patch #3 I've discussed with Bjorn on IRC and video what to put into the log here and captured most of what we discussed.
Mostly the point here is voting for voltages in the power-domain list is up to the drivers to do with performance states/opp-tables not for the GDSC code. - Bjorn/Bryan - Link to v6: https://lore.kernel.org/r/20241129-b4-linux-next-24-11-18-clock-multiple-pow...
Changes in v6: - Passes NULL to second parameter of devm_pm_domain_attach_list - Vlad - Link to v5: https://lore.kernel.org/r/20241128-b4-linux-next-24-11-18-clock-multiple-pow...
Changes in v5: - In-lines devm_pm_domain_attach_list() in probe() directly - Vlad - Link to v4: https://lore.kernel.org/r/20241127-b4-linux-next-24-11-18-clock-multiple-pow...
v4: - Adds Bjorn's RB to first patch - Bjorn - Drops the 'd' in "and int" - Bjorn - Amends commit log of patch 3 to capture a number of open questions - Bjorn - Link to v3: https://lore.kernel.org/r/20241126-b4-linux-next-24-11-18-clock-multiple-pow...
v3: - Fixes commit log "per which" - Bryan - Link to v2: https://lore.kernel.org/r/20241125-b4-linux-next-24-11-18-clock-multiple-pow...
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-pow...
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 (4): clk: qcom: gdsc: Release pm subdomains in reverse add order clk: qcom: gdsc: Capture pm_genpd_add_subdomain result code clk: qcom: common: Add support for power-domain attachment clk: qcom: Support attaching GDSCs to multiple parents
drivers/clk/qcom/common.c | 6 ++++ drivers/clk/qcom/gdsc.c | 75 +++++++++++++++++++++++++++++++++++++++-------- drivers/clk/qcom/gdsc.h | 1 + 3 files changed, 69 insertions(+), 13 deletions(-) --- base-commit: 8155b4ef3466f0e289e8fcc9e6e62f3f4dceeac2 change-id: 20241118-b4-linux-next-24-11-18-clock-multiple-power-domains-a5f994dc452a
Best regards,
gdsc_unregister() should release subdomains in the reverse order to the order in which those subdomains were added.
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 --- drivers/clk/qcom/gdsc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c index fa5fe4c2a2ee7786c2e8858f3e41301f639e5d59..bc1b1e37bf4222017c172b77603f8dedba961ed5 100644 --- a/drivers/clk/qcom/gdsc.c +++ b/drivers/clk/qcom/gdsc.c @@ -571,7 +571,7 @@ void gdsc_unregister(struct gdsc_desc *desc) size_t num = desc->num;
/* Remove subdomains */ - for (i = 0; i < num; i++) { + for (i = num - 1; i >= 0; i--) { if (!scs[i]) continue; if (scs[i]->parent)
On Mon, Dec 30, 2024 at 01:30:18PM +0000, Bryan O'Donoghue wrote:
gdsc_unregister() should release subdomains in the reverse order to the order in which those subdomains were added.
This sounds very reasonable to me, but what's the actual reason?
Fixes: 1b771839de05 ("clk: qcom: gdsc: enable optional power domain support") Cc: stable@vger.kernel.org
Without a reason it's hard to see why this needs to be backported.
Regards, Bjorn
Signed-off-by: Bryan O'Donoghue bryan.odonoghue@linaro.org
drivers/clk/qcom/gdsc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c index fa5fe4c2a2ee7786c2e8858f3e41301f639e5d59..bc1b1e37bf4222017c172b77603f8dedba961ed5 100644 --- a/drivers/clk/qcom/gdsc.c +++ b/drivers/clk/qcom/gdsc.c @@ -571,7 +571,7 @@ void gdsc_unregister(struct gdsc_desc *desc) size_t num = desc->num; /* Remove subdomains */
- for (i = 0; i < num; i++) {
- for (i = num - 1; i >= 0; i--) { if (!scs[i]) continue; if (scs[i]->parent)
-- 2.45.2
On 06/01/2025 16:53, Bjorn Andersson wrote:
This sounds very reasonable to me, but what's the actual reason?
Fixes: 1b771839de05 ("clk: qcom: gdsc: enable optional power domain support") Cc:stable@vger.kernel.org
Without a reason it's hard to see why this needs to be backported.
Regards, Bjorn
The reason is it makes the next patch much cleaner and makes backporting the Fixes in the next patch cleaner too.
I could squash the two patches together as another option..
On Mon, Jan 06, 2025 at 04:55:18PM +0000, Bryan O'Donoghue wrote:
On 06/01/2025 16:53, Bjorn Andersson wrote:
This sounds very reasonable to me, but what's the actual reason?
Fixes: 1b771839de05 ("clk: qcom: gdsc: enable optional power domain support") Cc:stable@vger.kernel.org
Without a reason it's hard to see why this needs to be backported.
Regards, Bjorn
The reason is it makes the next patch much cleaner and makes backporting the Fixes in the next patch cleaner too.
That makes sense, but let's state that in the commit message then.
I could squash the two patches together as another option..
That would work too. Although in the unexpected case that the order has any impact on outcome it would still be nice to have a comment about why it was done - and why it was flagged for stable backporting.
Regards, Bjorn
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 an int and can fail. Capture that result 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 --- drivers/clk/qcom/gdsc.c | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-)
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c index bc1b1e37bf4222017c172b77603f8dedba961ed5..fdedf6dfe7b90c074b200353fc0c2b897863c79f 100644 --- a/drivers/clk/qcom/gdsc.c +++ b/drivers/clk/qcom/gdsc.c @@ -506,6 +506,23 @@ static int gdsc_init(struct gdsc *sc) return ret; }
+static void gdsc_pm_subdomain_remove(struct gdsc_desc *desc, size_t num) +{ + struct device *dev = desc->dev; + struct gdsc **scs = desc->scs; + int i; + + /* Remove subdomains */ + for (i = num - 1; i >= 0; i--) { + if (!scs[i]) + continue; + if (scs[i]->parent) + 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); + } +} + int gdsc_register(struct gdsc_desc *desc, struct reset_controller_dev *rcdev, struct regmap *regmap) { @@ -555,30 +572,27 @@ 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) + goto err_pm_subdomain_remove; }
return of_genpd_add_provider_onecell(dev->of_node, data); + +err_pm_subdomain_remove: + gdsc_pm_subdomain_remove(desc, i); + + return ret; }
void gdsc_unregister(struct gdsc_desc *desc) { - int i; struct device *dev = desc->dev; - struct gdsc **scs = desc->scs; size_t num = desc->num;
- /* Remove subdomains */ - for (i = num - 1; i >= 0; i--) { - if (!scs[i]) - continue; - if (scs[i]->parent) - 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); - } + gdsc_pm_subdomain_remove(desc, num); of_genpd_del_provider(dev->of_node); }
linux-stable-mirror@lists.linaro.org
-
Bjorn Andersson -
Bryan O'Donoghue