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

From: Alexandre Mergnat
Date: Thu Oct 05 2023 - 09:57:48 EST



On 04/10/2023 18:29, AngeloGioacchino Del Regno wrote:
Il 18/07/23 11:03, Maxime Ripard ha scritto:
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

Sorry for resurrecting a very old thread, I was able to come back to this issue
right now: there's an issue that I can't really think about how to solve with
just the usage of clk_set_rate_exclusive().

Remembering that the clock tree is as following:
TVDPLL(x) -> PLL Divider (fixed) ->
-> MUX (can choose any of TVDPLL(1/2)_d(2/4/6/8/16)) -> Gate -> Controller

The DPI driver is doing:
1. Check the best factor for setting rate of a TVDPLL
2. Set rate of one TVDPLL (specified in DT): clk_set_rate(dpi->tvd_clk, rate);
   2a. Read the rate of that PLL again to know the precise clock output
3. Set rate on the Gate clock (forwards to MUX, selecting TVDPLL(x)_d(y)):
   clk_set_rate(dpi->pixel_clk, rate);


Now, the issue is: if I change the final pixel_clk rate setting to _exclusive(),
nothing still guarantees that we will be selecting the TVDPLL that we have
manipulated in step 2, look at the following example.

tvd_clk == TVDPLL1
pixel_clk == TOP_DP (can be muxed to any tvdpll1/2 dividers!)

clk_set_rate(tvdpll1, something); new_rate = clk_get_rate(tvdpll1)

...calculations... new_rate = pixclk * factor;
...more calculations....

clk_set_rate(pixel_clk, calculated_something)
       ^^^^^^

There is still no guarantee that pixel_clk is getting parented to one of the
TVDPLL1 dividers, as it could still get parented to a TVDPLL2 divider instead
if the other controller has set TVDPLL2 to "an acceptable rate": it's true that
this would work - yes but suboptimally! - because we want to set a specific
factor to reduce jitter on the final pixel clock.


....And I came back to this commit being again the best solution for me because....

1. You also seem to agree with me that a notifier would be troublesome and would
   probably introduce glitches; and
2. clk_set_rate_exclusive() doesn't give me any guarantee about selecting the same
   PLL that the driver was manipulating before.


Am I underestimating and/or ignoring anything else in all of that?

Thanks for the detailed explanation. I've no solution for you.
You still have my ReviewBy.

--
Regards,
Alexandre