Introduce pm_runtime_get() and pm_runtime_put_sync() on the
gdsc_toggle_logic().
This allows for the switching of the GDSC on/off to propagate to the parent
clock controller and consequently for any list of power-domains powering
that controller to be switched on/off.
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
drivers/clk/qcom/gdsc.c | 26 ++++++++++++++++++--------
drivers/clk/qcom/gdsc.h | 2 ++
2 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index fa5fe4c2a2ee7786c2e8858f3e41301f639e5d59..ff5df942147f87e0df24a70cf9ee53bb2df36e54 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>
@@ -141,10 +142,14 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status,
{
int ret;
- if (status == GDSC_ON && sc->rsupply) {
- ret = regulator_enable(sc->rsupply);
- if (ret < 0)
- return ret;
+ if (status == GDSC_ON) {
+ if (sc->rsupply) {
+ ret = regulator_enable(sc->rsupply);
+ if (ret < 0)
+ return ret;
+ }
+ if (pm_runtime_enabled(sc->dev))
+ pm_runtime_resume_and_get(sc->dev);
}
ret = gdsc_update_collapse_bit(sc, status == GDSC_OFF);
@@ -177,10 +182,14 @@ 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 < 0)
- return ret;
+ if (!ret && status == GDSC_OFF) {
+ if (pm_runtime_enabled(sc->dev))
+ pm_runtime_put_sync(sc->dev);
+ if (sc->rsupply) {
+ ret = regulator_disable(sc->rsupply);
+ if (ret < 0)
+ return ret;
+ }
}
return ret;
@@ -544,6 +553,7 @@ int gdsc_register(struct gdsc_desc *desc,
continue;
scs[i]->regmap = regmap;
scs[i]->rcdev = rcdev;
+ scs[i]->dev = dev;
ret = gdsc_init(scs[i]);
if (ret)
return ret;
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 1e2779b823d1c8ca077c9b4cd0a0dbdf5f9457ef..71ca02c78c5d089cdf96deadc417982ad6079255 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -30,6 +30,7 @@ struct reset_controller_dev;
* @resets: ids of resets associated with this gdsc
* @reset_count: number of @resets
* @rcdev: reset controller
+ * @dev: device associated with this gdsc
*/
struct gdsc {
struct generic_pm_domain pd;
@@ -74,6 +75,7 @@ struct gdsc {
const char *supply;
struct regulator *rsupply;
+ struct device *dev;
};
struct gdsc_desc {
--
2.45.2On Mon, Nov 18, 2024 at 02:24:33AM +0000, Bryan O'Donoghue wrote:
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
[..]
> @@ -177,10 +182,14 @@ 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 < 0)
> - return ret;
> + if (!ret && status == GDSC_OFF) {
> + if (pm_runtime_enabled(sc->dev))
> + pm_runtime_put_sync(sc->dev);
I already made this mistake, and 4cc47e8add63 ("clk: qcom: gdsc: Remove
direct runtime PM calls") covers the reason why it was a mistake.
What I think you want is two things:
1) When you're accessing the registers, you want the clock controller's
power-domain to be on.
2) When the client vote for a GDSC, you want to have the PM framework
also ensure that parent power-domains are kept on.
For the single case, this is handled by the pm_genpd_add_subdomain()
call below. This, or something along those lines, seems like the
appropriate solution.
Regards,
Bjorn
On 19/11/2024 15:34, Bjorn Andersson wrote: > What I think you want is two things: > 1) When you're accessing the registers, you want the clock controller's > power-domain to be on. > 2) When the client vote for a GDSC, you want to have the PM framework > also ensure that parent power-domains are kept on. > For the single case, this is handled by the pm_genpd_add_subdomain() > call below. This, or something along those lines, seems like the > appropriate solution. Yes. I'm finding with this patch reverted but, keeping the first patch that it pretty much works as you'd want with the caveat that gdsc_register -> gdsc_en -> gdsc_toggle fails the first time. After that I see the GDSCs on/off as excpected cat /sys/kernel/debug/pm_genpd/cam_cc_titan_top_gdsc/current_state off-0 cat /sys/kernel/debug/pm_genpd/cam_cc_ife_0_gdsc/current_state off-0 cam -c 1 --capture=10 --file cat /sys/kernel/debug/pm_genpd/cam_cc_titan_top_gdsc/current_state off-0 cat /sys/kernel/debug/pm_genpd/cam_cc_ife_0_gdsc/current_state off-0 Perhaps we just need to fix the probe path @ gdsc_register() --- bod
On Wed, Nov 20, 2024 at 05:09:08PM +0000, Bryan O'Donoghue wrote: > On 19/11/2024 15:34, Bjorn Andersson wrote: > > What I think you want is two things: > > 1) When you're accessing the registers, you want the clock controller's > > power-domain to be on. > > 2) When the client vote for a GDSC, you want to have the PM framework > > also ensure that parent power-domains are kept on. > > For the single case, this is handled by the pm_genpd_add_subdomain() > > call below. This, or something along those lines, seems like the > > appropriate solution. > > Yes. > > I'm finding with this patch reverted but, keeping the first patch that it > pretty much works as you'd want with the caveat that gdsc_register -> > gdsc_en -> gdsc_toggle fails the first time. > Can you clarify that call graph for me? The one case I can see where gdsc_register() leads to gdsc_enable() is if the sc is marked ALWAYS_ON and I don't think that is your case. What you describe sounds like we're trying to turn on the power-domain without first enabling the supplies, or perhaps there are clock dependencies that are not in order when this is being attempted? Regards, Bjorn > After that I see the GDSCs on/off as excpected > > cat /sys/kernel/debug/pm_genpd/cam_cc_titan_top_gdsc/current_state > off-0 > > cat /sys/kernel/debug/pm_genpd/cam_cc_ife_0_gdsc/current_state > off-0 > > cam -c 1 --capture=10 --file > > cat /sys/kernel/debug/pm_genpd/cam_cc_titan_top_gdsc/current_state > off-0 > > cat /sys/kernel/debug/pm_genpd/cam_cc_ife_0_gdsc/current_state > off-0 > > Perhaps we just need to fix the probe path @ gdsc_register() > > --- > bod
On 26/11/2024 17:23, Bjorn Andersson wrote: >> I'm finding with this patch reverted but, keeping the first patch that it >> pretty much works as you'd want with the caveat that gdsc_register -> >> gdsc_en -> gdsc_toggle fails the first time. >> > Can you clarify that call graph for me? Ah no my apologies, that wasn't the call graph I realised about 2 seconds after sending the mail and never corrected the record. Please see the v3 of this series instead. https://lore.kernel.org/lkml/20241126-b4-linux-next-24-11-18-clock-multiple-power-domains-v3-0-836dad33521a@linaro.org/ --- bod
On Mon, Nov 18, 2024 at 02:24:33AM +0000, Bryan O'Donoghue wrote:
> Introduce pm_runtime_get() and pm_runtime_put_sync() on the
> gdsc_toggle_logic().
>
> This allows for the switching of the GDSC on/off to propagate to the parent
> clock controller and consequently for any list of power-domains powering
> that controller to be switched on/off.
What is the end result of this patch? Does it bring up a single PM
domain or all of them? Or should it be a part of the driver's PM
callbacks? If the CC has multiple parent PM domains, shouldn't we also
use some of them as GDSC's parents?
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
> drivers/clk/qcom/gdsc.c | 26 ++++++++++++++++++--------
> drivers/clk/qcom/gdsc.h | 2 ++
> 2 files changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index fa5fe4c2a2ee7786c2e8858f3e41301f639e5d59..ff5df942147f87e0df24a70cf9ee53bb2df36e54 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>
> @@ -141,10 +142,14 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status,
> {
> int ret;
>
> - if (status == GDSC_ON && sc->rsupply) {
> - ret = regulator_enable(sc->rsupply);
> - if (ret < 0)
> - return ret;
> + if (status == GDSC_ON) {
> + if (sc->rsupply) {
> + ret = regulator_enable(sc->rsupply);
> + if (ret < 0)
> + return ret;
> + }
> + if (pm_runtime_enabled(sc->dev))
> + pm_runtime_resume_and_get(sc->dev);
> }
>
> ret = gdsc_update_collapse_bit(sc, status == GDSC_OFF);
> @@ -177,10 +182,14 @@ 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 < 0)
> - return ret;
> + if (!ret && status == GDSC_OFF) {
> + if (pm_runtime_enabled(sc->dev))
> + pm_runtime_put_sync(sc->dev);
> + if (sc->rsupply) {
> + ret = regulator_disable(sc->rsupply);
> + if (ret < 0)
> + return ret;
> + }
> }
>
> return ret;
> @@ -544,6 +553,7 @@ int gdsc_register(struct gdsc_desc *desc,
> continue;
> scs[i]->regmap = regmap;
> scs[i]->rcdev = rcdev;
> + scs[i]->dev = dev;
> ret = gdsc_init(scs[i]);
> if (ret)
> return ret;
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index 1e2779b823d1c8ca077c9b4cd0a0dbdf5f9457ef..71ca02c78c5d089cdf96deadc417982ad6079255 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -30,6 +30,7 @@ struct reset_controller_dev;
> * @resets: ids of resets associated with this gdsc
> * @reset_count: number of @resets
> * @rcdev: reset controller
> + * @dev: device associated with this gdsc
> */
> struct gdsc {
> struct generic_pm_domain pd;
> @@ -74,6 +75,7 @@ struct gdsc {
>
> const char *supply;
> struct regulator *rsupply;
> + struct device *dev;
> };
>
> struct gdsc_desc {
>
> --
> 2.45.2
>
--
With best wishes
Dmitry
On 18/11/2024 13:10, Dmitry Baryshkov wrote:
>> Introduce pm_runtime_get() and pm_runtime_put_sync() on the
>> gdsc_toggle_logic().
>>
>> This allows for the switching of the GDSC on/off to propagate to the parent
>> clock controller and consequently for any list of power-domains powering
>> that controller to be switched on/off.
> What is the end result of this patch? Does it bring up a single PM
> domain or all of them? Or should it be a part of the driver's PM
> callbacks? If the CC has multiple parent PM domains, shouldn't we also
> use some of them as GDSC's parents?
It brings up every PM domain in the list
clock_cc {
power-domains = <somedomain0>, <another-domain>;
};
No different to what the core code does for a single domain - except we
can actually turn the PDs off with the pm_runtime_put().
On Mon, Nov 18, 2024 at 01:19:49PM +0000, Bryan O'Donoghue wrote:
> On 18/11/2024 13:10, Dmitry Baryshkov wrote:
> > > Introduce pm_runtime_get() and pm_runtime_put_sync() on the
> > > gdsc_toggle_logic().
> > >
> > > This allows for the switching of the GDSC on/off to propagate to the parent
> > > clock controller and consequently for any list of power-domains powering
> > > that controller to be switched on/off.
> > What is the end result of this patch? Does it bring up a single PM
> > domain or all of them? Or should it be a part of the driver's PM
> > callbacks? If the CC has multiple parent PM domains, shouldn't we also
> > use some of them as GDSC's parents?
>
> It brings up every PM domain in the list
>
> clock_cc {
> power-domains = <somedomain0>, <another-domain>;
> };
>
> No different to what the core code does for a single domain - except we can
> actually turn the PDs off with the pm_runtime_put().
I see. I missed the device link part of the dev_pm_domain_attach_list().
Just to check, have you checked that this provides no splats in
lockdep-enabled kernels?
--
With best wishes
Dmitry
On 18/11/2024 13:39, Dmitry Baryshkov wrote:
>> It brings up every PM domain in the list
>>
>> clock_cc {
>> power-domains = <somedomain0>, <another-domain>;
>> };
>>
>> No different to what the core code does for a single domain - except we can
>> actually turn the PDs off with the pm_runtime_put().
> I see. I missed the device link part of the dev_pm_domain_attach_list().
>
> Just to check, have you checked that this provides no splats in
> lockdep-enabled kernels?
No, I haven't.
I'll have a look at that now. I did test on sc8280xp though.
I'll get back to you on lockdep.
---
bod
© 2016 - 2026 Red Hat, Inc.