Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes

[Maxime Ripard]

On Mon, Jul 17, 2023 at 04:30:48PM +0200, AngeloGioacchino Del Regno wrote:
> > > > > AFAIK the recommended way to deal with this is to use
> > > > > clk_set_rate_exclusive() and co. in whatever consumer driver that
> > > > > needs exclusive control on the clock rate.
> > > > 
> > > > I guess it works, but it looks to me like the issue here is that the
> > > > provider should disable it entirely? My expectation for
> > > > clk_set_rate_exclusive() is that one user needs to lock the clock rate
> > > > to operate properly.
> > > > 
> > > > If the provider expectation is that the rate or parent should never
> > > > changed, then that needs to be dealt with at the provider level, ie
> > > > through the clk_ops.
> > > > 
> > > > > However I'm not sure if that works for parents. It should, given the
> > > > > original use case was for the sunxi platforms, which like the MediaTek
> > > > > platform here has 2 PLLs for video related consumers, but I couldn't
> > > > > find code verifying it.
> > > > 
> > > > If you want to prevent clocks from ever being reparented, you can use
> > > > the new clk_hw_determine_rate_no_reparent() determine_rate
> > > > implementation.
> > > > 
> > > 
> > > We want the clocks to be reparented, as we need them to switch parents as
> > > explained before... that's more or less how the tree looks:
> > > 
> > > TVDPLL(x) -> PLL Divider (fixed) -> MUX -> Gate -> Controller
> > > 
> > > Besides, I think that forcing *one* parent to the dp/edp mux would produce a
> > > loss of the flexibility that the clock framework provides.
> > > 
> > > I again want to emphasize on the fact that TVDPLL1 and TVDPLL2 are *identical*
> > > in specs, and on that there will never be a MT8195 SoC that has only one of
> > > the two PLLs, for obvious reasons...
> > > 
> > > P.S.: If you need more context, I'll be glad to answer to any other question!
> > 
> > Then I have no idea what the question is :)
> > 
> > What are you trying to achieve / fix, and how can I help you ? :)
>
> Chen-Yu, Alexandre had/have questions about if there was any other solution instead
> of using the solution of *this* commit, so, if there's any other better solution
> than the one that I've sent as this commit.
> 
> I'm the one saying that this commit is the best solution :-P

I went back to the original patch, and my understanding is that, when
running two output in parallel, the modeset of one can affect the second
one, and that's bad, right?

If so, then you usually have multiple ways to fix this:

 - This patch
 - Using clk_set_rate_exclusive like Chen-Yu suggested
 - Using a notifier to react to a rate change and adjust

I'm not aware of any "official" guidelines at the clock framework level
regarding which to pick and all are fine.

My opinion though would be to use clk_set_rate_exclusive(), for multiple
reasons.

The first one is that it models correctly what you consumer expects:
that the rate is left untouched. This can happen in virtually any
situation where you have one clock in the same subtree changing rate,
while the patch above will only fix that particular interference.

The second one is that, especially with DP, you only have a handful of
rates you'll need to reach. 148MHz, 297MHz, 594MHz, and possibly a bunch
of others for eDP panels. It's thus likely to have both controllers
having the same frequency requirement, and thus it makes it possible to
run from only one PLL and shut the other down.

This patch will introduce orphan clocks issues that are always a bit
bothersome. A notifier would be troublesome to use and will probably
introduce glitches plus some weird interaction with scrambling if you
ever support it.

So, yeah, using clk_set_rate_exclusive() seems like the best option to me :)

Maxime

Attachment: signature.asc
Description: PGP signature