Re: [PATCH 4/5] clk: qcom: Add camera clock controller driver for SM8150

From: Satya Priya Kakitapalli (Temp)
Date: Fri Apr 05 2024 - 02:28:25 EST



On 3/2/2024 9:43 PM, Bryan O'Donoghue wrote:
On 29/02/2024 5:38 a.m., Satya Priya Kakitapalli wrote:
Add support for the camera clock controller for camera clients
to be able to request for camcc clocks on SM8150 platform.

Signed-off-by: Satya Priya Kakitapalli <quic_skakitap@xxxxxxxxxxx>
---

+static int cam_cc_sm8150_probe(struct platform_device *pdev)
+{
+    struct regmap *regmap;
+    int ret;
+
+    ret = devm_pm_runtime_enable(&pdev->dev);
+    if (ret)
+        return ret;
+
+    ret = pm_runtime_resume_and_get(&pdev->dev);
+    if (ret)
+        return ret;
+
+    regmap = qcom_cc_map(pdev, &cam_cc_sm8150_desc);
+    if (IS_ERR(regmap)) {
+        pm_runtime_put(&pdev->dev);
+        return PTR_ERR(regmap);
+    }
+
+    clk_trion_pll_configure(&cam_cc_pll0, regmap, &cam_cc_pll0_config);
+    clk_trion_pll_configure(&cam_cc_pll1, regmap, &cam_cc_pll1_config);
+    clk_regera_pll_configure(&cam_cc_pll2, regmap, &cam_cc_pll2_config);
+    clk_trion_pll_configure(&cam_cc_pll3, regmap, &cam_cc_pll3_config);
+    clk_trion_pll_configure(&cam_cc_pll4, regmap, &cam_cc_pll4_config);
+
+    /* Keep the critical clock always-on */
+    qcom_branch_set_clk_en(regmap, 0xc1e4); /* cam_cc_gdsc_clk */

Does this clock need to be specified this way ?

drivers/clk/qcom/camcc-sc8280xp.c::camcc_gdsc_clk specifies the gdsc clock as a shared op clock.

Actually it looks to be register compatible, please try defining titan_top_gdsc as per the example in 8280xp.

+
+    ret = qcom_cc_really_probe(pdev, &cam_cc_sm8150_desc, regmap);
+
+    pm_runtime_put(&pdev->dev);
+
+    return ret;
+}

So this is a pattern we keep repeating in the clock probe() functions which I am writing a series to address. There's no need to continue to replicate the bug in new code though.

Only switch on always-on clocks if probe succeeds.

    ret = qcom_cc_really_probe(pdev, &cam_cc_sm8150_desc, regmap);
    if (ret)
        goto probe_err;

    qcom_branch_set_clk_en(regmap, 0xc1e4); /* cam_cc_gdsc_clk */

    pm_runtime_put(&pdev->dev);

    return 0;

probe_err:
    pm_runtime_put_sync(&pdev->dev);

Alternatively switch on the always-on clocks before the really_probe() but then roll back in a probe_err: goto

probe_err:
    remap_bits_update(regmap, 0xc1e4, BIT(0), 0);
    pm_runtime_put_sync(&pdev->dev);

There may be corner cases where always-on has to happen before really_probe() I suppose but as a general pattern the above should be how we go.


I have rechecked this and see that this clock is PoR ON (i.e BIT(0) is set upon power ON) and it should be kept always ON as per HW recommendation. So even if the probe fails we shouldn't be clearing it against the hw recommendation. We are setting the bit here again to make sure it is set when the driver probes.


Anyway I suspect the right thing to do is to define a titan_top_gdsc_clk with shared ops to "park" the GDSC clock to 19.2 MHz instead of turning it off.

You can get rid of the hard-coded always-on and indeed represent the clock in /sysfs - which is preferable IMO to just whacking registers to keep clocks always-on in probe anyway.

Please try to define the titan_top_gdsc_clk as a shared_ops clock instead of hard coding to always on.

If that doesn't work for some reason, then please fix your always-on logic in probe() to only make the clock fixed on, if really_probe() succeeds.

---
bod