Messages in this thread | | | Date | Thu, 5 Oct 2023 09:49:52 +0200 | Subject | Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes | From | Alexandre Mergnat <> |
| |
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
| |