Re: [PATCH v4 02/12] clk: qcom: Use qcom_branch_set_clk_en()

[Konrad Dybcio]

On 2.01.2024 11:35, Johan Hovold wrote:
> On Sat, Dec 30, 2023 at 02:04:04PM +0100, Konrad Dybcio wrote:
>> Instead of magically poking at the bit0 of branch clocks' CBCR, use
>> the newly introduced helper.
>>
>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
>> ---
> 
>> diff --git a/drivers/clk/qcom/camcc-sc8280xp.c b/drivers/clk/qcom/camcc-sc8280xp.c
>> index 3dcd79b01515..94db130b85e2 100644
>> --- a/drivers/clk/qcom/camcc-sc8280xp.c
>> +++ b/drivers/clk/qcom/camcc-sc8280xp.c
>> @@ -3010,10 +3010,8 @@ static int camcc_sc8280xp_probe(struct platform_device *pdev)
>>  	clk_lucid_pll_configure(&camcc_pll6, regmap, &camcc_pll6_config);
>>  	clk_lucid_pll_configure(&camcc_pll7, regmap, &camcc_pll7_config);
>>  
>> -	/*
>> -	 * Keep camcc_gdsc_clk always enabled:
>> -	 */
>> -	regmap_update_bits(regmap, 0xc1e4, BIT(0), 1);
>> +	/* Keep the critical clocks always-on */
>> +	qcom_branch_set_clk_en(regmap, 0xc1e4); /* CAMCC_GDSC_CLK */
> 
> I still think something along the lines of
> 
> 	/* Keep some clocks always on */
> 
> is preferred as it is far from obvious why a camera clock would be
> considered "critical".
> 
> Or perhaps you can come up with a better description of why we've
> decided not to model these clocks and just leave them ungated.
Technically they're not really super critical if the hardware is
not in use.. It's just that at one point Qualcomm decided to take
the lazy decision to keep them always-on downstream and we seem to
have agreed on going with that, instead of pm_clk (remember my old
attempt at getting rid of this on dispcc-sc8280xp?)..

For now, I was just trying to clean this up a bit before looking
into a better solution for this (probably a whole lot of pm_clks
with some clever handle-getting due to different ways of grabbing
clock sources.. by-name vs by-index vs global lookup that we've
accumulated over the years).

Some clock drivers do expose clocks that are related to the CPU
subsystem access and keep them always-on for obvious reasons,
but on newer socs they're not even controlled from Linux, so
perhaps we can just unregister these (read: delete from the driver)

Konrad