On Thu, Feb 20, 2025 at 10:31:44PM +0000, Bryan O'Donoghue wrote:
>
> Where we can zap the GDSCs the power-rails for the block should be always on
> because the initial PLL configuration we typically do in probe() would be
> negated as soon as the power rail for the block is switched off.
>
> True.
>
> In my opinion:
>
> - We should only do the pd list addition in one place
> Either that or push it into each driver.
>
> I don't favour doing it in each driver since it is boilerplate
> code that we basically just end up copy/pasting again and again.
>
> - We can start off by only including a configure_pll callback
> for the 2-3 blocks where we know we have multiple rails
>
> This here works well for me on x1e:
>
> Author: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> Date: Tue Feb 18 19:46:55 2025 +0000
>
> clk: qcom: common: Add configure_plls callback prototype
>
> Add a configure_plls() callback so that we can stage
> qcom_cc_attach_pds()
> before configuring PLLs and ensure that the power-domain rail list is
> switched on prior to configuring PLLs.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
>
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 9e3380fd71819..4aa00ad51c2f6 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -304,6 +304,12 @@ int qcom_cc_really_probe(struct device *dev,
> if (ret < 0 && ret != -EEXIST)
> return ret;
>
> + if (desc->configure_plls) {
> + ret = desc->configure_plls(dev, desc, regmap);
> + if (ret)
> + return ret;
> + }
> +
> reset = &cc->reset;
> reset->rcdev.of_node = dev->of_node;
> reset->rcdev.ops = &qcom_reset_ops;
> diff --git a/drivers/clk/qcom/common.h b/drivers/clk/qcom/common.h
> index 7ace5d7f5836a..77002e39337d7 100644
> --- a/drivers/clk/qcom/common.h
> +++ b/drivers/clk/qcom/common.h
> @@ -38,6 +38,9 @@ struct qcom_cc_desc {
> const struct qcom_icc_hws_data *icc_hws;
> size_t num_icc_hws;
> unsigned int icc_first_node_id;
> + int (*configure_plls)(struct device *dev,
> + const struct qcom_cc_desc *desc,
> + struct regmap *regmap);
> };
>
> +static int cam_cc_x1e80100_configure_plls(struct device *dev,
> + const struct qcom_cc_desc *desc,
> + struct regmap *regmap)
> +{
> + int ret;
> +
> + ret = devm_pm_runtime_enable(dev);
> + if (ret)
> + return ret;
> +
> + ret = pm_runtime_resume_and_get(dev);
> + if (ret)
> + return ret;
I think, it's better to add desc->use_rpm. Then these two calls and
pm_runtime_put() can go to a generic code.
Or maybe we can enable RPM for all clock controllers?
> +
> + clk_lucid_ole_pll_configure(&cam_cc_pll0, regmap,
> &cam_cc_pll0_config);
> + clk_lucid_ole_pll_configure(&cam_cc_pll1, regmap,
> &cam_cc_pll1_config);
> + clk_rivian_evo_pll_configure(&cam_cc_pll2, regmap,
> &cam_cc_pll2_config);
> + clk_lucid_ole_pll_configure(&cam_cc_pll3, regmap,
> &cam_cc_pll3_config);
> + clk_lucid_ole_pll_configure(&cam_cc_pll4, regmap,
> &cam_cc_pll4_config);
> + clk_lucid_ole_pll_configure(&cam_cc_pll6, regmap,
> &cam_cc_pll6_config);
> + clk_lucid_ole_pll_configure(&cam_cc_pll8, regmap,
> &cam_cc_pll8_config);
> +
> + /* Keep clocks always enabled */
> + qcom_branch_set_clk_en(regmap, 0x13a9c); /* CAM_CC_GDSC_CLK */
> + qcom_branch_set_clk_en(regmap, 0x13ab8); /* CAM_CC_SLEEP_CLK */
> +
> + pm_runtime_put(dev);
> +
> + return 0;
> +}
> +
> static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
> .config = &cam_cc_x1e80100_regmap_config,
> .clks = cam_cc_x1e80100_clocks,
> @@ -2434,6 +2465,7 @@ static const struct qcom_cc_desc cam_cc_x1e80100_desc
> = {
> .num_resets = ARRAY_SIZE(cam_cc_x1e80100_resets),
> .gdscs = cam_cc_x1e80100_gdscs,
> .num_gdscs = ARRAY_SIZE(cam_cc_x1e80100_gdscs),
> + .configure_plls = cam_cc_x1e80100_configure_plls,
> };
>
> This has the same effect as you were alluding to and in fact we could
> probably even move the pm_runtime_enable/resume_and_get and pm_runtime_put
> into really_probe().
>
> It seems to me anyway we should try to push as much of this into core logic
> to be reused as possible.
>
> ---
> bod
--
With best wishes
Dmitry
