| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <gzjyyl2kzv52zsewn5zf6ei65fymyi4pspvsmsjaqj5sklfxvc@bkg46saulni5>
Date: Wed, 19 Feb 2025 13:59:11 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Jagadeesh Kona <quic_jkona@...cinc.com>
Cc: Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
Bjorn Andersson <andersson@...nel.org>, Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>, Ajit Pandey <quic_ajipan@...cinc.com>,
Imran Shaik <quic_imrashai@...cinc.com>, Taniya Das <quic_tdas@...cinc.com>,
Satya Priya Kakitapalli <quic_skakitap@...cinc.com>, linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] clk: qcom: videocc: Add support to attach multiple
power domains
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@...cinc.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@...aro.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@...aro.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.
--
With best wishes
Dmitry
Powered by blists - more mailing lists