Re: [PATCH 0/5] clk: qcom: sm8[56]50: Drop the Disp AHB clock from Display Clock Controller

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 1/25/24 11:44, Abel Vesa wrote:
> On 24-01-25 10:49:23, Konrad Dybcio wrote:
>>
>>
>> On 1/25/24 10:27, Abel Vesa wrote:
>>> The Disp AHB clock is provided by the GCC but never registered. It is
>>> instead enabled on probe as it is expected to be always-on. So it should
>>> be dropped from Disp CC entirely.
>>>
>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>>> ---
>>
>> Abel, you just raised some concerns over my series doing this and now
>> you're doing the same, plus breaking backwards compatibility for no
>> good reason, instead of solving the problem.
> 
> Sorry but, during the off-list discussion, you convinced me that it is OK to drop
> their registration as long as we enable them on probe.
> 
> I've not seen the following reply in time before sending current series:
> https://lore.kernel.org/all/6aa58497-9727-4601-b6eb-264c478997c3@linaro.org/
> 
> Since this is blocking the patches for dispcc and dts for X1E80100, I
> thought I'd just drop the clock as required from DT point of view.
> But yeah, you're right, it breaks bindings ABI and that's wrong.
> 
>>
>> The correct solution here is to register the AHB clock with GCC and
>> pm_clk_add() it from dispcc's .probe (and enable runtime PM on dispcc
>> if it's already not the case). Then the AHB clock will be gated when
>> no display hardware (= no dispcc consumer) is in use.
> 
> I agree.
> 
>>
>> 8[56]50 are in a good position for this, as they already have the
>> required DTS reference. Unfortunately, I still haven't fully dug
>> into this for platforms without one, but that's on me.
> 
> Since I need to do this for the X1E80100, I'll probably do it for the
> other two as well.

Thanks!

> 
> Sorry for the misunderstanding.

The story is confusing as per usual, perhaps I could have explained
it better in the first place..

Konrad