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@xxxxxxxxxxx>
---
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@xxxxxxxxxx>
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@xxxxxxxxxx>
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
