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
