| Message ID | 20250218-videocc-pll-multi-pd-voting-v1-4-cfe6289ea29b@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | clk: qcom: Add support to attach multiple power domains in cc probe | expand |
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() ? 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
On 18/02/2025 15:46, Bryan O'Donoghue wrote: >> + 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() ? > > 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. I see you're saying do this before waking up the local PLLs prior to really_probe. hmm.. the existing code works for me on the CRD but not on the Dell Insprion14 so I missed this on my series. Instead of pushing qcom_cc_attach_pds() back up one level and having to do that over and over again for each clock controller that has multiple power domains, we could just move the configure pll logic to a callback. Add a new callback to qcom_cc_desc()->configure_plls() Then qcom_cc_really_probe() would look like: ret = devm_pm_domain_attach_list(); if (ret < 0 && ret != -EEXIST); return ret; desc->configure_plls(); There's no point in having devm_pm_domain_attach_list() twice within < 20 LOC we should just force the sequencing in the right order once and IMO do it in really_probe() so that we don't keep adding custom logic to multi-pd controllers. Its a generic problem we can solve closer to the core logic. --- bod
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
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 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. > 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
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? > > > 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.
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 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. >> >>> 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. --- bod
On 2/19/2025 5:32 PM, Dmitry Baryshkov wrote: > 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? > I can remove the const qualifier in qcom_cc_map() prototype to fix this, but that requires changes in many other clock drivers also since they are currently passing const descriptor to qcom_cc_map(). So would like to keep the qcom_cc_map() unchanged. >> 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() > I agree with this approach to move entire PM handling to qcom_cc_map() but one concern is const qualifier mentioned above and it also enables runtime PM for clock controllers that doesn't need any runtime PM(e.g:- GCC/GPUCC). That may not cause any issue but we also need to see from where we need to call pm_runtime_put(). We may have to add pm_runtime_put() at the end of both qcom_cc_probe() and qcom_cc_really_probe() to move the device back to suspend after probe. But ideally runtime PM is not required for most clock controllers except MMCC's that have MMCX dependency. Please let me know your thoughts on this. Thanks, Jagadeesh >> >> 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/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 > --- > bod
On 20/02/2025 07:15, Jagadeesh Kona wrote: >> 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 That's not what I see. The PLLs start and the GDSCs which depend also start. Perhaps you could give the code a try and comment ? --- bod
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. --- bod
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 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@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; 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
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
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;
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(+)