lkml.org 
[lkml]   [2023]   [Oct]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for top_dp/edp muxes
From


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

\
 
 \ /
  Last update: 2023-10-05 15:59    [W:0.079 / U:2.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site