| 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: <6b0684a0-a519-463f-b7be-176a4752a786@linaro.org>
Date: Thu, 20 Feb 2025 22:31:44 +0000
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Jagadeesh Kona <quic_jkona@...cinc.com>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: 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 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@...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?
>>
>> 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@...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..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
Powered by blists - more mailing lists