| Message ID | 20241118-b4-linux-next-24-11-18-clock-multiple-power-domains-v1-1-b7a2bd82ba37@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | clk: qcom: Add support for multiple power-domains for a clock controller. | expand |
On Mon, Nov 18, 2024 at 02:24:32AM +0000, Bryan O'Donoghue wrote: > Right now we have a plethora of singleton power-domains which power clock > controllers. These singletons are switched on by core logic when we probe > the clocks. > > However when multiple power-domains are attached to a clock controller that > list of power-domains needs to be managed outside of core logic. > > Use dev_pm_domain_attach_list() to automatically hook the list of given > power-domains in the dtsi for the clock being registered in > qcom_cc_really_probe(). > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > drivers/clk/qcom/common.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c > index 33cc1f73c69d1f875a193aea0552902268dc8716..b4377fa09f7c0ec8d3c63dfc97d04fbb8cd6e10b 100644 > --- a/drivers/clk/qcom/common.c > +++ b/drivers/clk/qcom/common.c > @@ -22,6 +22,7 @@ struct qcom_cc { > struct qcom_reset_controller reset; > struct clk_regmap **rclks; > size_t num_rclks; > + struct dev_pm_domain_list *pd_list; > }; > > const > @@ -283,6 +284,25 @@ static int qcom_cc_icc_register(struct device *dev, > desc->num_icc_hws, icd); > } > > +static int qcom_cc_pds_attach(struct device *dev, struct qcom_cc *cc) > +{ > + struct dev_pm_domain_attach_data pd_data = { > + .pd_names = 0, > + .num_pd_names = 0, > + }; > + int ret; > + > + /* Only one power-domain platform framework will hook it up */ > + if (dev->pm_domain) > + return 0; > + > + ret = dev_pm_domain_attach_list(dev, &pd_data, &cc->pd_list); > + if (ret < 0) > + return ret; I don't think it is enough to just attach to power domains. We also need to power on some of them (MMCX) in order to be able to access clock controller registers. > + > + return 0; > +} > + > int qcom_cc_really_probe(struct device *dev, > const struct qcom_cc_desc *desc, struct regmap *regmap) > { > @@ -299,6 +319,10 @@ int qcom_cc_really_probe(struct device *dev, > if (!cc) > return -ENOMEM; > > + ret = qcom_cc_pds_attach(dev, cc); > + if (ret) > + return ret; > + > reset = &cc->reset; > reset->rcdev.of_node = dev->of_node; > reset->rcdev.ops = &qcom_reset_ops; > > -- > 2.45.2 >
On 18/11/2024 13:03, Dmitry Baryshkov wrote: > I don't think it is enough to just attach to power domains. We also need > to power on some of them (MMCX) in order to be able to access clock > controller registers. That the next patch.
On 11/18/24 04:24, Bryan O'Donoghue wrote: > Right now we have a plethora of singleton power-domains which power clock > controllers. These singletons are switched on by core logic when we probe > the clocks. > > However when multiple power-domains are attached to a clock controller that > list of power-domains needs to be managed outside of core logic. > > Use dev_pm_domain_attach_list() to automatically hook the list of given > power-domains in the dtsi for the clock being registered in > qcom_cc_really_probe(). > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > drivers/clk/qcom/common.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c > index 33cc1f73c69d1f875a193aea0552902268dc8716..b4377fa09f7c0ec8d3c63dfc97d04fbb8cd6e10b 100644 > --- a/drivers/clk/qcom/common.c > +++ b/drivers/clk/qcom/common.c > @@ -22,6 +22,7 @@ struct qcom_cc { > struct qcom_reset_controller reset; > struct clk_regmap **rclks; > size_t num_rclks; > + struct dev_pm_domain_list *pd_list; > }; > > const > @@ -283,6 +284,25 @@ static int qcom_cc_icc_register(struct device *dev, > desc->num_icc_hws, icd); > } > > +static int qcom_cc_pds_attach(struct device *dev, struct qcom_cc *cc) > +{ > + struct dev_pm_domain_attach_data pd_data = { > + .pd_names = 0, > + .num_pd_names = 0, > + }; > + int ret; > + > + /* Only one power-domain platform framework will hook it up */ > + if (dev->pm_domain) > + return 0; > + > + ret = dev_pm_domain_attach_list(dev, &pd_data, &cc->pd_list); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > int qcom_cc_really_probe(struct device *dev, > const struct qcom_cc_desc *desc, struct regmap *regmap) > { > @@ -299,6 +319,10 @@ int qcom_cc_really_probe(struct device *dev, > if (!cc) > return -ENOMEM; > > + ret = qcom_cc_pds_attach(dev, cc); > + if (ret) > + return ret; > + ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list); if (ret < 0 && ret != -EEXIST) return ret; That's it. No need to introduce a new function, not saying about 20 LoC off. Next, you have to call dev_pm_domain_detach_list() in your version of the change on the error paths etc., fortunately this can be easily avoided, if the resource management flavour of the same function is in use. > reset = &cc->reset; > reset->rcdev.of_node = dev->of_node; > reset->rcdev.ops = &qcom_reset_ops; > -- Best wishes, Vladimir
On 18/11/2024 22:17, Vladimir Zapolskiy wrote: > ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list); > if (ret < 0 && ret != -EEXIST) > return ret; > > That's it. No need to introduce a new function, not saying about 20 LoC > off. > > Next, you have to call dev_pm_domain_detach_list() in your version of the > change on the error paths etc., fortunately this can be easily avoided, > if the resource management flavour of the same function is in use. From memory I _thought_ I concluded this was necessary + /* Only one power-domain platform framework will hook it up */ + if (dev->pm_domain) + return 0; => for clocks which have a single power-domain the core framework will already have setup the linkage by the time we get to this point in the code. But, I'll check again to make sure. --- bod
On Mon, Nov 18, 2024 at 02:24:32AM +0000, Bryan O'Donoghue wrote: > Right now we have a plethora of singleton power-domains which power clock > controllers. These singletons are switched on by core logic when we probe > the clocks. > > However when multiple power-domains are attached to a clock controller that > list of power-domains needs to be managed outside of core logic. > I'd prefer if you rewrote this to make it clearer for the broader audience what exactly you mean with "singleton" and "core logic". > Use dev_pm_domain_attach_list() to automatically hook the list of given > power-domains in the dtsi for the clock being registered in > qcom_cc_really_probe(). > Do we need to power on/off all the associated power-domains every time we access registers in the clock controller etc, or only in relation to operating these GDSCs? Regards, Bjorn > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > drivers/clk/qcom/common.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c > index 33cc1f73c69d1f875a193aea0552902268dc8716..b4377fa09f7c0ec8d3c63dfc97d04fbb8cd6e10b 100644 > --- a/drivers/clk/qcom/common.c > +++ b/drivers/clk/qcom/common.c > @@ -22,6 +22,7 @@ struct qcom_cc { > struct qcom_reset_controller reset; > struct clk_regmap **rclks; > size_t num_rclks; > + struct dev_pm_domain_list *pd_list; > }; > > const > @@ -283,6 +284,25 @@ static int qcom_cc_icc_register(struct device *dev, > desc->num_icc_hws, icd); > } > > +static int qcom_cc_pds_attach(struct device *dev, struct qcom_cc *cc) > +{ > + struct dev_pm_domain_attach_data pd_data = { > + .pd_names = 0, > + .num_pd_names = 0, > + }; > + int ret; > + > + /* Only one power-domain platform framework will hook it up */ > + if (dev->pm_domain) > + return 0; > + > + ret = dev_pm_domain_attach_list(dev, &pd_data, &cc->pd_list); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > int qcom_cc_really_probe(struct device *dev, > const struct qcom_cc_desc *desc, struct regmap *regmap) > { > @@ -299,6 +319,10 @@ int qcom_cc_really_probe(struct device *dev, > if (!cc) > return -ENOMEM; > > + ret = qcom_cc_pds_attach(dev, cc); > + if (ret) > + return ret; > + > reset = &cc->reset; > reset->rcdev.of_node = dev->of_node; > reset->rcdev.ops = &qcom_reset_ops; > > -- > 2.45.2 >
diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c index 33cc1f73c69d1f875a193aea0552902268dc8716..b4377fa09f7c0ec8d3c63dfc97d04fbb8cd6e10b 100644 --- a/drivers/clk/qcom/common.c +++ b/drivers/clk/qcom/common.c @@ -22,6 +22,7 @@ struct qcom_cc { struct qcom_reset_controller reset; struct clk_regmap **rclks; size_t num_rclks; + struct dev_pm_domain_list *pd_list; }; const @@ -283,6 +284,25 @@ static int qcom_cc_icc_register(struct device *dev, desc->num_icc_hws, icd); } +static int qcom_cc_pds_attach(struct device *dev, struct qcom_cc *cc) +{ + struct dev_pm_domain_attach_data pd_data = { + .pd_names = 0, + .num_pd_names = 0, + }; + int ret; + + /* Only one power-domain platform framework will hook it up */ + if (dev->pm_domain) + return 0; + + ret = dev_pm_domain_attach_list(dev, &pd_data, &cc->pd_list); + if (ret < 0) + return ret; + + return 0; +} + int qcom_cc_really_probe(struct device *dev, const struct qcom_cc_desc *desc, struct regmap *regmap) { @@ -299,6 +319,10 @@ int qcom_cc_really_probe(struct device *dev, if (!cc) return -ENOMEM; + ret = qcom_cc_pds_attach(dev, cc); + if (ret) + return ret; + reset = &cc->reset; reset->rcdev.of_node = dev->of_node; reset->rcdev.ops = &qcom_reset_ops;
Right now we have a plethora of singleton power-domains which power clock controllers. These singletons are switched on by core logic when we probe the clocks. However when multiple power-domains are attached to a clock controller that list of power-domains needs to be managed outside of core logic. Use dev_pm_domain_attach_list() to automatically hook the list of given power-domains in the dtsi for the clock being registered in qcom_cc_really_probe(). Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/clk/qcom/common.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)