| 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: <d4c4ecf0-9094-4341-8711-78a48e5d1344@linaro.org>
Date: Wed, 19 Feb 2025 01:21:53 +0000
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: Jagadeesh Kona <quic_jkona@...cinc.com>,
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 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.
New clock controllers would then use this callback mechanism and
potentially all of the controllers to have uniformity.
---
bod
Powered by blists - more mailing lists