On 2/19/2025 6:51 AM, Bryan O'Donoghue wrote:
> On 18/02/2025 17:19, Dmitry Baryshkov wrote:
>> On Tue, Feb 18, 2025 at 03:46:15PM +0000, Bryan O'Donoghue wrote:
>>> On 18/02/2025 14:26, Jagadeesh Kona wrote:
>>>> During boot-up, the PLL configuration might be missed even after
>>>> calling pll_configure() from the clock controller probe. This can
>>>> happen because the PLL is connected to one or more rails that are
>>>> turned off, and the current clock controller code cannot enable
>>>> multiple rails during probe. Consequently, the PLL may be activated
>>>> with suboptimal settings, causing functional issues.
>>>>
>>>> To properly configure the video PLLs in the probe on SM8450, SM8475,
>>>> SM8550, and SM8650 platforms, the MXC rail must be ON along with MMCX.
>>>> Therefore, add support to attach multiple power domains to videocc on
>>>> these platforms.
>>>>
>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@xxxxxxxxxxx>
>>>> ---
>>>> drivers/clk/qcom/videocc-sm8450.c | 4 ++++
>>>> drivers/clk/qcom/videocc-sm8550.c | 4 ++++
>>>> 2 files changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/qcom/videocc-sm8450.c b/drivers/clk/qcom/videocc-sm8450.c
>>>> index f26c7eccb62e7eb8dbd022e2f01fa496eb570b3f..b50a14547336580de88a741f1d33b126e9daa848 100644
>>>> --- a/drivers/clk/qcom/videocc-sm8450.c
>>>> +++ b/drivers/clk/qcom/videocc-sm8450.c
>>>> @@ -437,6 +437,10 @@ static int video_cc_sm8450_probe(struct platform_device *pdev)
>>>> struct regmap *regmap;
>>>> int ret;
>>>> + ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8450_desc);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> ret = devm_pm_runtime_enable(&pdev->dev);
>>>> if (ret)
>>>> return ret;
>>>> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c
>>>> index 7c25a50cfa970dff55d701cb24bc3aa5924ca12d..d4b223d1392f0721afd1b582ed35d5061294079e 100644
>>>> --- a/drivers/clk/qcom/videocc-sm8550.c
>>>> +++ b/drivers/clk/qcom/videocc-sm8550.c
>>>> @@ -542,6 +542,10 @@ static int video_cc_sm8550_probe(struct platform_device *pdev)
>>>> int ret;
>>>> u32 sleep_clk_offset = 0x8140;
>>>> + ret = qcom_cc_attach_pds(&pdev->dev, &video_cc_sm8550_desc);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> ret = devm_pm_runtime_enable(&pdev->dev);
>>>> if (ret)
>>>> return ret;
>>>>
>>>
>>> What's the difference between doing the attach here or doing it in
>>> really_probe() ?
>>
>> I'd second this. If the domains are to be attached before calling any
>> other functions, move the call to the qcom_cc_map(), so that all drivers
>> get all domains attached before configuring PLLs instead of manually
>> calling the function.
>>
>>> There doesn't seem to be any difference except that we will have an
>>> additional delay introduced.
>>>
>>> Are you describing a race condition ?
>>>
>>> I don't see _logic_ here to moving the call into the controller's higher
>>> level probe.
>>>
>>> Can you describe some more ?
>>>
>>> ---
>>> bod
>>
>
> Here's one way this could work
>
> 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..1924130814600 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -304,6 +304,9 @@ int qcom_cc_really_probe(struct device *dev,
> if (ret < 0 && ret != -EEXIST)
> return ret;
>
> + if (desc->configure_plls)
> + desc->configure_plls(regmap);
> +
> 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..4955085ff8669 100644
> --- a/drivers/clk/qcom/common.h
> +++ b/drivers/clk/qcom/common.h
> @@ -38,6 +38,7 @@ struct qcom_cc_desc {
> const struct qcom_icc_hws_data *icc_hws;
> size_t num_icc_hws;
> unsigned int icc_first_node_id;
> + void (*configure_plls)(struct regmap *regmap);
> };
>
> and
>
> % git diff drivers/clk/qcom/camcc-x1e80100.c
> diff --git a/drivers/clk/qcom/camcc-x1e80100.c b/drivers/clk/qcom/camcc-x1e80100.c
> index b73524ae64b1b..c9748d1f8a15b 100644
> --- a/drivers/clk/qcom/camcc-x1e80100.c
> +++ b/drivers/clk/qcom/camcc-x1e80100.c
> @@ -2426,6 +2426,21 @@ static const struct regmap_config cam_cc_x1e80100_regmap_config = {
> .fast_io = true,
> };
>
> +static void cam_cc_x1e80100_configure_plls(struct regmap *regmap)
> +{
> + 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 */
> +}
> +
> static const struct qcom_cc_desc cam_cc_x1e80100_desc = {
> .config = &cam_cc_x1e80100_regmap_config,
> .clks = cam_cc_x1e80100_clocks,
> @@ -2434,6 +2449,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,
> };
>
> static const struct of_device_id cam_cc_x1e80100_match_table[] = {
> @@ -2461,18 +2477,6 @@ static int cam_cc_x1e80100_probe(struct platform_device *pdev)
> return PTR_ERR(regmap);
> }
>
> - 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 */
> -
> ret = qcom_cc_really_probe(&pdev->dev, &cam_cc_x1e80100_desc, regmap);
>
> pm_runtime_put(&pdev->dev);
>
> Or a least it works for me.
>
This patch will not work in all cases, maybe in your case required power domains might be ON
from bootloaders so it might be working.
> New clock controllers would then use this callback mechanism and potentially all of the controllers to have uniformity.
>
No, above approach also requires changes in each individual clock driver to define the callback. So I don't see any advantage
with this than the current approach.
Thanks,
Jagadeesh
