On 2/22/2025 12:49 AM, Konrad Dybcio wrote: > On 18.02.2025 4:46 PM, 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() ? >> >> 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 ? > > I think this is a sane course of action: > > 1. associate pll config with the pll (treewide change) > 2. add a generic pll_configure call that parses the type > 3. store PLLs to be configured in an array of dt-bindings indices > 4. move PLL configuration to really_probe > 5. move RPM enablement to really_probe, make it conditional to limit resource > waste on e.g. gcc > 6. move attaching GDSCs to really_probe > > Konrad Thanks Konrad for your feedback and suggestion. I will check and work on this approach of moving PLL configure and RPM enablement to a common code. Thanks, Jagadeesh
