Messages in this thread | | | Date | Fri, 6 Oct 2023 12:05:45 +0200 | From | Maxime Ripard <> | Subject | Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes |
| |
On Wed, Oct 04, 2023 at 06:29:41PM +0200, 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.
If your clock ends up with a suboptimal set of parameters, you have a problem with determine_rate.
> ....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?
I guess I'm still confused about why you want to allow reparenting in the first place, but still don't want to reparent to the other PLL?
Anyway, it's not a big deal. Whatever works for you I guess :)
Maxime [unhandled content-type:application/pgp-signature] | |