Message-ID: <jxgy2pvns4ri2aj5nmdhb4zbluseuzdejbplh2avwz63df2cfx@grrrdm6ujzi4>
Date:   Mon, 17 Jul 2023 13:24:19 +0200
From:   Maxime Ripard <mripard@...nel.org>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
Cc:     Chen-Yu Tsai <wenst@...omium.org>,
        Alexandre Mergnat <amergnat@...libre.com>, sboyd@...nel.org,
        mturquette@...libre.com, matthias.bgg@...il.com, msp@...libre.com,
        yangyingliang@...wei.com, u.kleine-koenig@...gutronix.de,
        miles.chen@...iatek.com, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, kernel@...labora.com
Subject: Re: [PATCH 2/2] clk: mediatek: mt8195-topckgen: Refactor parents for
 top_dp/edp muxes

On Mon, Jul 17, 2023 at 11:13:26AM +0200, AngeloGioacchino Del Regno wrote:
> 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@...libre.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@...labora.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)

So the HDMI controller can be parented either to the first or second PLL
(and same thing for the (e)DP controllers)?

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

My point wasn't that you should do it, but that you can't rely on the
parent or rate sticking around.

> > > 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 ? :)

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists