| Message ID | 20241118-b4-linux-next-24-11-18-clock-multiple-power-domains-v1-2-b7a2bd82ba37@linaro.org (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | clk: qcom: Add support for multiple power-domains for a clock controller. | expand |
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 >
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?
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
On 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
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 {
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(-)