| Message ID | 20250306-videocc-pll-multi-pd-voting-v2-3-0cd00612bc0e@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | clk: qcom: Add support to attach multiple power domains in cc probe | expand |
On 6.03.2025 9:55 AM, Jagadeesh Kona wrote: > Add support for runtime power management, PLL configuration and enabling > critical clocks in qcom_cc_really_probe() to commonize the clock > controller probe. > > The runtime power management is not required for all clock controllers, > hence handle the rpm based on use_rpm flag in clock controller descriptor. > Also the power domains need to be kept enabled during pll configuration, > hence attach all required power domains prior to calling get_sync() on the > device. > > Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > --- [...] > + * struct qcom_clk_cfg - To maintain list of clocks that needs to be > + * kept ON or misc clock register settings I have some reservations about this name, particularly since 'clk_cfg' has already been used in the msmbus/interconnect space.. Perhaps qcom_clk_reg_settings? > + * @offset: address offset for clock register > + * @mask: bit mask to indicate the bits to update > + */ > +struct qcom_clk_cfg { > + unsigned int offset; > + unsigned int mask; "u32" also, to expand it, we probably want "field" and "val" to replace the calls to regmap_update_bits in some drivers I think we may keep this /\ struct for things like: /* Enable clock gating for MDP clocks */ regmap_update_bits(regmap, DISP_CC_MISC_CMD, 0x10, 0x10); while keeping a separate u32 array of branch clocks to call qcom_branch_set_clk_en() on - we would then use 3x less memory > +}; > + > struct qcom_cc_desc { > const struct regmap_config *config; > struct clk_regmap **clks; > @@ -39,6 +50,11 @@ struct qcom_cc_desc { > const struct qcom_icc_hws_data *icc_hws; > size_t num_icc_hws; > unsigned int icc_first_node_id; > + struct qcom_clk_cfg *clks_cfg; > + size_t num_clks_cfg; > + struct clk_alpha_pll **plls; Some ancient or "non-standard" SoCs have non-alpha PLLs, please rename this to something like alpha_plls Konrad
On Thu, Mar 06, 2025 at 02:25:35PM +0530, Jagadeesh Kona wrote: > Add support for runtime power management, PLL configuration and enabling > critical clocks in qcom_cc_really_probe() to commonize the clock > controller probe. Please split this into two commits: one for the runtime PM and another one for clock configuration, because ... > > The runtime power management is not required for all clock controllers, > hence handle the rpm based on use_rpm flag in clock controller descriptor. > Also the power domains need to be kept enabled during pll configuration, > hence attach all required power domains prior to calling get_sync() on the > device. > > Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > --- > drivers/clk/qcom/common.c | 45 ++++++++++++++++++++++++++++++++++++--------- > drivers/clk/qcom/common.h | 16 ++++++++++++++++ > 2 files changed, 52 insertions(+), 9 deletions(-) [...] > + > + for (i = 0; i < desc->num_plls; i++) > + qcom_cc_clk_pll_configure(desc->plls[i], regmap); > + > + for (i = 0 ; i < desc->num_clks_cfg; i++) > + regmap_update_bits(regmap, clks_cfg[i].offset, > + clks_cfg[i].mask, clks_cfg[i].mask); > + ... just calling regmap_update_bits() looks like a step backwards. In the past several years we got several sensible wrappers and helpers. I suggest having a callback instead of a fixed 'update bits' table. > reset = &cc->reset; > reset->rcdev.of_node = dev->of_node; > reset->rcdev.ops = &qcom_reset_ops; The RPM part is fine with me.
On 3/6/2025 6:15 PM, Konrad Dybcio wrote: > On 6.03.2025 9:55 AM, Jagadeesh Kona wrote: >> Add support for runtime power management, PLL configuration and enabling >> critical clocks in qcom_cc_really_probe() to commonize the clock >> controller probe. >> >> The runtime power management is not required for all clock controllers, >> hence handle the rpm based on use_rpm flag in clock controller descriptor. >> Also the power domains need to be kept enabled during pll configuration, >> hence attach all required power domains prior to calling get_sync() on the >> device. >> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> --- > > [...] > >> + * struct qcom_clk_cfg - To maintain list of clocks that needs to be >> + * kept ON or misc clock register settings > > I have some reservations about this name, particularly since 'clk_cfg' > has already been used in the msmbus/interconnect space.. > > Perhaps qcom_clk_reg_settings? > Sure, will update. >> + * @offset: address offset for clock register >> + * @mask: bit mask to indicate the bits to update >> + */ >> +struct qcom_clk_cfg { >> + unsigned int offset; >> + unsigned int mask; > > "u32" > > also, to expand it, we probably want "field" and "val" to replace the > calls to regmap_update_bits in some drivers > > I think we may keep this /\ struct for things like: > > /* Enable clock gating for MDP clocks */ > regmap_update_bits(regmap, DISP_CC_MISC_CMD, 0x10, 0x10); > > while keeping a separate u32 array of branch clocks to call > qcom_branch_set_clk_en() on - we would then use 3x less memory > > Sure, will take care of these in next series. >> +}; > >> + >> struct qcom_cc_desc { >> const struct regmap_config *config; >> struct clk_regmap **clks; >> @@ -39,6 +50,11 @@ struct qcom_cc_desc { >> const struct qcom_icc_hws_data *icc_hws; >> size_t num_icc_hws; >> unsigned int icc_first_node_id; >> + struct qcom_clk_cfg *clks_cfg; >> + size_t num_clks_cfg; >> + struct clk_alpha_pll **plls; > > Some ancient or "non-standard" SoCs have non-alpha PLLs, please rename > this to something like alpha_plls > Sure, will update it. Thanks, Jagadeesh > > Konrad
On 3/7/2025 2:17 PM, Dmitry Baryshkov wrote: > On Thu, Mar 06, 2025 at 02:25:35PM +0530, Jagadeesh Kona wrote: >> Add support for runtime power management, PLL configuration and enabling >> critical clocks in qcom_cc_really_probe() to commonize the clock >> controller probe. > > Please split this into two commits: one for the runtime PM and another > one for clock configuration, because ... > Sure, will split this in the next series. >> >> The runtime power management is not required for all clock controllers, >> hence handle the rpm based on use_rpm flag in clock controller descriptor. >> Also the power domains need to be kept enabled during pll configuration, >> hence attach all required power domains prior to calling get_sync() on the >> device. >> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> --- >> drivers/clk/qcom/common.c | 45 ++++++++++++++++++++++++++++++++++++--------- >> drivers/clk/qcom/common.h | 16 ++++++++++++++++ >> 2 files changed, 52 insertions(+), 9 deletions(-) > > [...] > >> + >> + for (i = 0; i < desc->num_plls; i++) >> + qcom_cc_clk_pll_configure(desc->plls[i], regmap); >> + >> + for (i = 0 ; i < desc->num_clks_cfg; i++) >> + regmap_update_bits(regmap, clks_cfg[i].offset, >> + clks_cfg[i].mask, clks_cfg[i].mask); >> + > > ... just calling regmap_update_bits() looks like a step backwards. In > the past several years we got several sensible wrappers and helpers. I > suggest having a callback instead of a fixed 'update bits' table. > Sure, will check and add a callback to handle all these clock config settings. Thanks, Jagadeesh >> reset = &cc->reset; >> reset->rcdev.of_node = dev->of_node; >> reset->rcdev.ops = &qcom_reset_ops; > > The RPM part is fine with me. >
diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c index 74d062b5da0647f7f2bd8fd7a004ffdb1116c1ea..ce87f74fa51639ed270a0c56fff1cd2845885647 100644 --- a/drivers/clk/qcom/common.c +++ b/drivers/clk/qcom/common.c @@ -9,6 +9,7 @@ #include <linux/platform_device.h> #include <linux/clk-provider.h> #include <linux/interconnect-clk.h> +#include <linux/pm_runtime.h> #include <linux/reset-controller.h> #include <linux/of.h> @@ -350,6 +351,7 @@ int qcom_cc_really_probe(struct device *dev, struct clk_regmap **rclks = desc->clks; size_t num_clk_hws = desc->num_clk_hws; struct clk_hw **clk_hws = desc->clk_hws; + struct qcom_clk_cfg *clks_cfg = desc->clks_cfg; cc = devm_kzalloc(dev, sizeof(*cc), GFP_KERNEL); if (!cc) @@ -359,6 +361,23 @@ int qcom_cc_really_probe(struct device *dev, if (ret < 0 && ret != -EEXIST) return ret; + if (desc->use_rpm) { + ret = devm_pm_runtime_enable(dev); + if (ret) + return ret; + + ret = pm_runtime_resume_and_get(dev); + if (ret) + return ret; + } + + for (i = 0; i < desc->num_plls; i++) + qcom_cc_clk_pll_configure(desc->plls[i], regmap); + + for (i = 0 ; i < desc->num_clks_cfg; i++) + regmap_update_bits(regmap, clks_cfg[i].offset, + clks_cfg[i].mask, clks_cfg[i].mask); + reset = &cc->reset; reset->rcdev.of_node = dev->of_node; reset->rcdev.ops = &qcom_reset_ops; @@ -369,23 +388,25 @@ int qcom_cc_really_probe(struct device *dev, ret = devm_reset_controller_register(dev, &reset->rcdev); if (ret) - return ret; + goto put_rpm; if (desc->gdscs && desc->num_gdscs) { scd = devm_kzalloc(dev, sizeof(*scd), GFP_KERNEL); - if (!scd) - return -ENOMEM; + if (!scd) { + ret = -ENOMEM; + goto put_rpm; + } 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; + goto put_rpm; ret = devm_add_action_or_reset(dev, qcom_cc_gdsc_unregister, scd); if (ret) - return ret; + goto put_rpm; } cc->rclks = rclks; @@ -396,7 +417,7 @@ int qcom_cc_really_probe(struct device *dev, for (i = 0; i < num_clk_hws; i++) { ret = devm_clk_hw_register(dev, clk_hws[i]); if (ret) - return ret; + goto put_rpm; } for (i = 0; i < num_clks; i++) { @@ -405,14 +426,20 @@ int qcom_cc_really_probe(struct device *dev, ret = devm_clk_register_regmap(dev, rclks[i]); if (ret) - return ret; + goto put_rpm; } ret = devm_of_clk_add_hw_provider(dev, qcom_cc_clk_hw_get, cc); if (ret) - return ret; + goto put_rpm; + + ret = qcom_cc_icc_register(dev, desc); + +put_rpm: + if (desc->use_rpm) + pm_runtime_put(dev); - return qcom_cc_icc_register(dev, desc); + return ret; } EXPORT_SYMBOL_GPL(qcom_cc_really_probe); diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h index 2066c8937936235d7bd03ab3225d4b3f4fb08dd0..da27290b7da3ddc6d8ba1b064619e294bfcb686c 100644 --- a/drivers/clk/qcom/common.h +++ b/drivers/clk/qcom/common.h @@ -26,6 +26,17 @@ struct qcom_icc_hws_data { int clk_id; }; +/** + * struct qcom_clk_cfg - To maintain list of clocks that needs to be + * kept ON or misc clock register settings + * @offset: address offset for clock register + * @mask: bit mask to indicate the bits to update + */ +struct qcom_clk_cfg { + unsigned int offset; + unsigned int mask; +}; + struct qcom_cc_desc { const struct regmap_config *config; struct clk_regmap **clks; @@ -39,6 +50,11 @@ struct qcom_cc_desc { const struct qcom_icc_hws_data *icc_hws; size_t num_icc_hws; unsigned int icc_first_node_id; + struct qcom_clk_cfg *clks_cfg; + size_t num_clks_cfg; + struct clk_alpha_pll **plls; + size_t num_plls; + bool use_rpm; }; /**
Add support for runtime power management, PLL configuration and enabling critical clocks in qcom_cc_really_probe() to commonize the clock controller probe. The runtime power management is not required for all clock controllers, hence handle the rpm based on use_rpm flag in clock controller descriptor. Also the power domains need to be kept enabled during pll configuration, hence attach all required power domains prior to calling get_sync() on the device. Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> --- drivers/clk/qcom/common.c | 45 ++++++++++++++++++++++++++++++++++++--------- drivers/clk/qcom/common.h | 16 ++++++++++++++++ 2 files changed, 52 insertions(+), 9 deletions(-)