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 ?
Yes, we need this clock to be always on.
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.
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.
Defining the gdsc clk allows consumers to control it, we do not want
this clock to be disabled/controlled from consumers. Hence it is better
to not model this clock and just keep it always on from probe.
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.
Sure I'll do this.
---
bod
