| Message ID | 20250218-videocc-pll-multi-pd-voting-v1-0-cfe6289ea29b@quicinc.com |
|---|---|
| Headers | show |
| Series | clk: qcom: Add support to attach multiple power domains in cc probe | expand |
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@quicinc.com> >>> --- >>> 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@linaro.org> 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@linaro.org> 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. New clock controllers would then use this callback mechanism and potentially all of the controllers to have uniformity. --- bod
On 2/18/2025 10:49 PM, 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@quicinc.com> >>> --- >>> 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. > I earlier tried moving the attach PDs call to qcom_cc_map(), but I faced the below issues 1. desc passed to qcom_cc_map() has const qualifier, so updating desc->pd_list inside qcom_cc_map() is leading to a warning. 2. If we attach the PDs after calling get_sync() on device, I observed that PDS are not getting enabled during probe. Currently qcom_cc_map() is called after get_sync() is already called on device. Probably, we can add a new function qcom_cc_attach_pds_map() where we can attach PDs and call qcom_cc_map() inside it. We can then invoke this new function at the start of probe before get_sync(). I will post this change in next version if this aligns with your thoughts. Thanks, Jagadeesh
On Wed, Feb 19, 2025 at 05:08:52PM +0530, Jagadeesh Kona wrote: > > > On 2/18/2025 10:49 PM, 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@quicinc.com> > >>> --- > >>> 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. > > > > I earlier tried moving the attach PDs call to qcom_cc_map(), but I faced the below issues > 1. desc passed to qcom_cc_map() has const qualifier, so updating desc->pd_list > inside qcom_cc_map() is leading to a warning. And? Can you fix the warning? > 2. If we attach the PDs after calling get_sync() on device, I observed > that PDS are not getting enabled during probe. Currently qcom_cc_map() > is called after get_sync() is already called on device. Move PM handling into qcom_cc_map(). Then together with the Bryan's proposal most of the probe() functions can just call qcom_cc_probe() > > Probably, we can add a new function qcom_cc_attach_pds_map() where we can > attach PDs and call qcom_cc_map() inside it. We can then invoke this new > function at the start of probe before get_sync(). I will post this change > in next version if this aligns with your thoughts. > > Thanks, > Jagadeesh
On 2/18/2025 11:02 PM, Dmitry Baryshkov wrote: > On Tue, Feb 18, 2025 at 07:56:50PM +0530, Jagadeesh Kona wrote: >> Videocc requires both MMCX and MXC rails to be powered ON >> to configure the video PLLs on SM8450, SM8550 and SM8650 >> platforms. Hence add MXC power domain to videocc node on >> these platforms. >> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 ++- >> arch/arm64/boot/dts/qcom/sm8550.dtsi | 3 ++- >> arch/arm64/boot/dts/qcom/sm8650.dtsi | 3 ++- >> 3 files changed, 6 insertions(+), 3 deletions(-) > > Three separate patches, please. With that in mind: > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Sure, will split this into separate patches. Thanks, Jagadeesh >> >> diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi >> index 9c809fc5fa45a98ff5441a0b6809931588897243..4f8dca8fc64212191780067c5d8815e3a2bb137f 100644 >> --- a/arch/arm64/boot/dts/qcom/sm8450.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi >> @@ -3136,7 +3136,8 @@ videocc: clock-controller@aaf0000 { >> reg = <0 0x0aaf0000 0 0x10000>; >> clocks = <&rpmhcc RPMH_CXO_CLK>, >> <&gcc GCC_VIDEO_AHB_CLK>; >> - power-domains = <&rpmhpd RPMHPD_MMCX>; >> + power-domains = <&rpmhpd RPMHPD_MMCX>, >> + <&rpmhpd RPMHPD_MXC>; >> required-opps = <&rpmhpd_opp_low_svs>; >> #clock-cells = <1>; >> #reset-cells = <1>; >> diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi >> index eac8de4005d82f246bc50f64f09515631d895c99..a039ae71e1b7bba8124128d19de5e00c65217770 100644 >> --- a/arch/arm64/boot/dts/qcom/sm8550.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi >> @@ -2889,7 +2889,8 @@ videocc: clock-controller@aaf0000 { >> reg = <0 0x0aaf0000 0 0x10000>; >> clocks = <&bi_tcxo_div2>, >> <&gcc GCC_VIDEO_AHB_CLK>; >> - power-domains = <&rpmhpd RPMHPD_MMCX>; >> + power-domains = <&rpmhpd RPMHPD_MMCX>, >> + <&rpmhpd RPMHPD_MXC>; >> required-opps = <&rpmhpd_opp_low_svs>; >> #clock-cells = <1>; >> #reset-cells = <1>; >> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi >> index 86684cb9a9325618ddb74458621cf4bbdc1cc0d1..32af2a0f7a0030f155b7d8c93faeffa384a42768 100644 >> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi >> @@ -3524,7 +3524,8 @@ videocc: clock-controller@aaf0000 { >> reg = <0 0x0aaf0000 0 0x10000>; >> clocks = <&bi_tcxo_div2>, >> <&gcc GCC_VIDEO_AHB_CLK>; >> - power-domains = <&rpmhpd RPMHPD_MMCX>; >> + power-domains = <&rpmhpd RPMHPD_MMCX>, >> + <&rpmhpd RPMHPD_MXC>; >> #clock-cells = <1>; >> #reset-cells = <1>; >> #power-domain-cells = <1>; >> >> -- >> 2.34.1 >> >
On 20/02/2025 22:31, Bryan O'Donoghue wrote: > It seems to me anyway we should try to push as much of this into core > logic to be reused as possible. But there's no valid use-case for doing ret = devm_pm_domain_attach_list(dev, NULL, &cc->pd_list); in the driver and then conditionally doing it again in really_probe(). Its an either/or. --- bod
On 21/02/2025 00:10, Dmitry Baryshkov wrote: >> +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? That second point is pretty interesting - I think at this stage ~ all of them do this boilerplate stuff over and over again .. --- bod
On 2/21/2025 4:01 AM, Bryan O'Donoghue wrote: > On 20/02/2025 07:15, Jagadeesh Kona wrote: >> >> >> On 2/19/2025 5:37 PM, Bryan O'Donoghue wrote: >>> On 19/02/2025 11:59, Dmitry Baryshkov wrote: >>>> On Wed, Feb 19, 2025 at 05:11:03PM +0530, Jagadeesh Kona wrote: >>>>> >>>>> >>>>> 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@quicinc.com> >>>>>>>>> --- >>>>>>>>> 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@linaro.org> >>>>>> 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@linaro.org> >>>>>> >>>>>> 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. >>>> >>>> But with his patch domains are attached before configuring the PLLs, are >>>> they not? >>> >>> Yes, its logically the same just done in core code. >>> >> >> Yes, this code attaches domains before configuring the PLLs, but it attaches PDs after get_sync() >> is called on device. As I mentioned in other patch earlier, if we attach PDS after get_sync() is >> already called on device, then power domains are not getting enabled during the probe, leading to >> the same improper PLL configuration issue. But the current patch series posted will fix this issue >> >>>>> >>>>>> 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. >>>> >>>> Bryan's proposal moves us towards having a common code, so it's better. >>>> >>> >>> I can take the time to do the whole sweep and publish a RFC. >>> >> >> Yes, but moving the PLL configuration to callback will not solve the actual PLL configuration >> issue being discussed here. >> >> Thanks, >> Jagadeesh >> > > Right what you are really saying is that the power-rails for the clock controller need to remain always on at the moment. > > 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@linaro.org> > 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@linaro.org> > > 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; > + > + 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. > As per the issue I pointer earlier, I see now you moved the get_sync() call to after the attach_pds(). But this PLL callback approach also requires changes in each individual clock driver, and adding a callback in each clock controller driver to configure the PLLs doesn't reduce any boiler plate code in my opinion. Infact I feel this is harder to maintain as the code in callback is not constant and vary from one driver to another. Instead the current approach to explicitly attach pds if we have multiple power domains is much easier to maintain since it is the same function call in every driver. Kindly review the discussion in another thread[PATCH 3/5] as that will avoid these extra callbacks in each driver and the code will be uniform across all the clock drivers Thanks, Jagadeesh > --- > bod
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. The support to attach multiple power domains to clock controllers was recently added in Bryan's series[1] but it currently doesn't enable all clock controller power domains during probe which are needed for PLL configuration. This series adds required support to enable the multiple power domains during clock controllers probe and adds support to enable both MMCX & MXC rails during probe for videocc on SM8450, SM8475, SM8550 and SM8650 platforms to configure the video PLLs properly. This fixes the below warning reported in SM8550 venus testing due to video_cc_pll0 not properly getting configured during videocc probe [ 46.535132] Lucid PLL latch failed. Output may be unstable! [1]: https://lore.kernel.org/all/20250117-b4-linux-next-24-11-18-clock-multiple-power-domains-v10-0-13f2bb656dad@linaro.org/ Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> --- Jagadeesh Kona (4): dt-bindings: clock: qcom,sm8450-videocc: Add MXC power domain clk: qcom: common: Attach clock power domains conditionally clk: qcom: videocc: Add support to attach multiple power domains arm64: dts: qcom: Add MXC power domain to videocc nodes Taniya Das (1): clk: qcom: common: Add support to attach multiple power domains .../bindings/clock/qcom,sm8450-videocc.yaml | 9 ++++++--- arch/arm64/boot/dts/qcom/sm8450.dtsi | 3 ++- arch/arm64/boot/dts/qcom/sm8550.dtsi | 3 ++- arch/arm64/boot/dts/qcom/sm8650.dtsi | 3 ++- drivers/clk/qcom/common.c | 21 ++++++++++++++++++--- drivers/clk/qcom/common.h | 2 ++ drivers/clk/qcom/videocc-sm8450.c | 4 ++++ drivers/clk/qcom/videocc-sm8550.c | 4 ++++ 8 files changed, 40 insertions(+), 9 deletions(-) --- base-commit: e5d3fd687aac5eceb1721fa92b9f49afcf4c3717 change-id: 20250218-videocc-pll-multi-pd-voting-d614dce910e7 Best regards,