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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 17/07/23 09:48, Maxime Ripard ha scritto:
> Hi,
> 
> On Fri, Jul 14, 2023 at 12:19:00PM +0800, Chen-Yu Tsai wrote:
>> On Thu, Jul 13, 2023 at 9:22 PM Alexandre Mergnat <amergnat@baylibre.com> wrote:
>>> On 13/07/2023 09:21, AngeloGioacchino Del Regno wrote:
>>>> The top_dp and top_edp muxes can be both parented to either TVDPLL1
>>>> or TVDPLL2, two identically specced PLLs for the specific purpose of
>>>> giving out pixel clock: this becomes a problem when the MediaTek
>>>> DisplayPort Interface (DPI) driver tries to set the pixel clock rate.
>>>>
>>>> In the usecase of two simultaneous outputs (using two controllers),
>>>> it was seen that one of the displays would sometimes display garbled
>>>> output (if any at all) and this was because:
>>>>    - top_edp was set to TVDPLL1, outputting X GHz
>>>>    - top_dp was set to TVDPLL2, outputting Y GHz
>>>>      - mtk_dpi calls clk_set_rate(top_edp, Z GHz)
>>>>        - top_dp is switched to TVDPLL1
>>>>        - TVDPLL1 changes its rate, top_edp outputs the wrong rate.
>>>>        - eDP display is garbled
>>>>
>>>> To solve this issue, remove all TVDPLL1 parents from `top_dp` and
>>>> all TVDPLL2 parents from `top_edp`, plus, necessarily switch both
>>>> clocks to use the new MUX_GATE_CLR_SET_UPD_INDEXED() macro to be
>>>> able to use the right bit index for the new parents list.
>>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>> ---
>>>>    drivers/clk/mediatek/clk-mt8195-topckgen.c | 22 ++++++++++++++--------
>>>>    1 file changed, 14 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/clk/mediatek/clk-mt8195-topckgen.c b/drivers/clk/mediatek/clk-mt8195-topckgen.c
>>>> index 81daa24cadde..abb3721f6e1b 100644
>>>> --- a/drivers/clk/mediatek/clk-mt8195-topckgen.c
>>>> +++ b/drivers/clk/mediatek/clk-mt8195-topckgen.c
>>>> @@ -417,15 +417,21 @@ static const char * const pwrmcu_parents[] = {
>>>>
>>>>    static const char * const dp_parents[] = {
>>>>        "clk26m",
>>>> -     "tvdpll1_d2",
>>>>        "tvdpll2_d2",
>>>> -     "tvdpll1_d4",
>>>>        "tvdpll2_d4",
>>>> -     "tvdpll1_d8",
>>>>        "tvdpll2_d8",
>>>> -     "tvdpll1_d16",
>>>>        "tvdpll2_d16"
>>>>    };
>>>> +static const u8 dp_parents_idx[] = { 0, 2, 4, 6, 8 };
>>>> +
>>>> +static const char * const edp_parents[] = {
>>>> +     "clk26m",
>>>> +     "tvdpll1_d2",
>>>> +     "tvdpll1_d4",
>>>> +     "tvdpll1_d8",
>>>> +     "tvdpll1_d16"
>>>> +};
>>>> +static const u8 edp_parents_idx[] = { 0, 1, 3, 5, 7 };
>>>
>>> AFAII your solution is to force a specific TVDPLLX for each display, and
>>> it isn't dynamic.
>>>
>>> Do you think it's possible to do that using the DTS ? I'm asking
>>> because, IMHO, this kind of setup is more friendly/readable/flexible in
>>> the DTS than hardcoded into the driver.
>>
>> (CC-ing Maxime, who has some experience in the matter.)
> 
> It's not clear to me what the context is, but I'll try my best :)
> 

I'll try to explain briefly.

On *some* MediaTek platforms, there are two *identical* PLLs for HDMI/(e)DP,
which are internal to the SoC; clocks for HDMI/eDP/DP controller*s* can be
parented either PLL (as you see from this commit).

The PLL's rate can be changed in runtime and you want to use PLL dividers to
get the final pixel clock (that's to obviously reduce the PLL jitter).

>> assigned-parents doesn't prevent your system from reparenting the clocks
>> back to a conflicting configuration.
> 
> Yep, it's very much a one-off thing. There's no guarantee at the moment,
> and semantics-wise we could change the whole thing at probe time and it
> would be fine.
> 

Would be fine... but more complicated I think?

>> 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!

Cheers,
Angelo

> Maxime