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

["Satya Priya Kakitapalli (Temp)" <quic_skakitap@quicinc.com>]

On 7/15/2024 8:31 PM, Bryan O'Donoghue wrote:
> On 15/07/2024 11:38, Dmitry Baryshkov wrote:
>>>> Does it apply to SM8150? For example, on SM8250 RCG2s are not parked.
>>>
>>> Yes, it applies to SM8150.
>> Should the same logic be applied to other chipsets supported upstream?
>> If this is the case, which chipsets?
>
> If you are representing the "top" GDSC inside of the CCF instead of 
> doing this
>
> +    /* Keep the critical clock always-on */
> +    qcom_branch_set_clk_en(regmap, 0xc1e4); /* cam_cc_gdsc_clk */
>
> then the clock should be parked else you'll find the GDSC doesn't come 
> out of reset.
>
> and... as I look at it now we have a logical conflict in 
> drivers/clk/qcom/camcc-sc8280xp.c
>
> static struct clk_branch camcc_gdsc_clk = {
>         .halt_reg = 0xc1e4,
>         .halt_check = BRANCH_HALT,
>         .clkr = {
>                 .enable_reg = 0xc1e4,
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "camcc_gdsc_clk",
>                         .parent_hws = (const struct clk_hw*[]){
>                                 &camcc_xo_clk_src.clkr.hw,
>                         },
>                         .num_parents = 1,
>                         .flags = CLK_SET_RATE_PARENT,
>                         .ops = &clk_branch2_ops,
>                 },
>         },
> };
>
> Patch sent.
>
> https://lore.kernel.org/linux-arm-msm/20240715-linux-next-24-07-13-sc8280xp-camcc-fixes-v1-1-fadb5d9445c1@linaro.org/T/#u 
>
>

If the clock is modelled, it can get disabled during the late init call, 
when the CCF disables the unused clocks. But, it is a PoR ON  clock and 
expectation from design team is to keep it always-on for GDSC functionality.

So, we should not model this, instead just keep it always on from probe.


> In the round I think we should avoid these horrific hard-coded 
> always-on writes where possible.
>

We have been suggested to make such clocks always ON from probe on 
previous upstream discussions. This is also recently acknowledged by 
maintainers, please see [1] and [2].


[1] 
https://lore.kernel.org/linux-clk/6286a410-6faa-4996-8a9e-dc334dd9421f@linaro.org/


[2] 
https://lore.kernel.org/linux-clk/664cca91-8615-d3f6-7525-15b9b6725cce@quicinc.com/


> Personally I think parking is better than always-on specifically 
> because you define the clock and "see" it in the clock tree.
>
> ---
> bod