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

[Satya Priya Kakitapalli (Temp)]

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