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

[Konrad Dybcio]

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@xxxxxxxxxx>
> > > ---
> >
> > 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@xxxxxxxxxx/
>
> 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