Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
From:   Benjamin Bara <bbara93@gmail.com>
To:     mripard@kernel.org
Cc:     abelvesa@kernel.org, bbara93@gmail.com, benjamin.bara@skidata.com,
        conor+dt@kernel.org, devicetree@vger.kernel.org,
        festevam@gmail.com, frank@oltmanns.dev, kernel@pengutronix.de,
        krzysztof.kozlowski+dt@linaro.org,
        linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org,
        linux-imx@nxp.com, linux-kernel@vger.kernel.org,
        linux@armlinux.org.uk, mturquette@baylibre.com, peng.fan@nxp.com,
        robh+dt@kernel.org, s.hauer@pengutronix.de, sboyd@kernel.org,
        shawnguo@kernel.org
Subject: Re: [PATCH 06/13] clk: keep track if a clock is explicitly configured
Date:   Wed, 20 Sep 2023 09:22:16 +0200
Message-Id: <20230920072216.1737599-1-bbara93@gmail.com>
In-Reply-To: <pgnlrokdqqqclqvp4h2zk7iyq2jfncnvvwavovydovdmj3d2gf@kszpslmeswbr>
References: <pgnlrokdqqqclqvp4h2zk7iyq2jfncnvvwavovydovdmj3d2gf@kszpslmeswbr>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Maxime!

thanks for taking the time to look through :)

On Tue, 19 Sept 2023 at 09:07, Maxime Ripard <mripard@kernel.org> wrote:
> On Mon, Sep 18, 2023 at 12:40:02AM +0200, Benjamin Bara wrote:
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > When we keep track if a clock has a given rate explicitly set by a
> > consumer, we can identify unintentional clock rate changes in an easy
> > way. This also helps during debugging, as one can see if a rate is set
> > by accident or due to a consumer-related change.
> >
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> >  drivers/clk/clk.c            | 25 +++++++++++++++++++++++++
> >  include/linux/clk-provider.h |  1 +
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 8f4f92547768..82c65ed432c5 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -70,6 +70,7 @@ struct clk_core {
> >       unsigned long           rate;
> >       unsigned long           req_rate;
> >       unsigned long           new_rate;
> > +     unsigned long           set_rate;
>
> This is pretty much what req_rate is supposed to be about. Why didn't it
> work in your case?

I picked this one to respond first because I think some of the
implemented stuff just workarounds the current req_rate behaviour.

Currently, I have two "problems" with it:
1. It's set during initialization[1]. In this phase, the *required* rate
   isn't known yet, so it should be 0 imo.
2. It's set during re-parenting[2,3]. Also here, just because we
   re-parent, the active consumer (which set the req_rate to a valid
   value) still requires the clock to have the same rate.

That is basically the reason why we have no info if the req_rate is
really "required" by a consumer or if it is just set because the parent
had it at some time. It's only usage is here[4], which IMO doesn't
really depends on the wrong behaviour I described above.

The respective sub-tree we talk about on the imx8mp looks like this (one
example for the the LVDS-only case):
video_pll1 (pll; 7x crtc rate - currently, rate is assigned via dt)
  video_pll1_bypass (mux; 7x crtc rate)
    video_pll1_out (gate; 7x crtc rate)
      media_ldb (divider; 7x crtc rate)
        media_ldb_root_clk (gate; 7x crtc rate)
      media_disp2_pix (divider; 1x crtc rate)
        media_disp2_pix_root_clk (gate; 1x crtc rate)
      media_disp1_pix (divider; unused for now)
        media_disp1_pix_root_clk (gate; unused for now)

The problem is that the panel driver sets media_disp1_pix_root_clk,
ldb-bridge driver sets media_ldb_root_clk. All the others have a
req_rate of the rate video_pll1 had when they got initialized or
re-parented.

My idea was, that when media_disp2_pix_root_clk is set to the CRTC rate,
IMO all clocks along the line (especially media_disp1_pix, which is
"seen" as child of the PLL, and the actual divider for
media_disp2_pix_root_clk) need to set their new rate as "required",
because the subtree below them relies on it. This might be a wrong
approach. It might be sufficient to have a req_rate only on the nodes
that actually require it. However, IMHO we need to make sure that *all*
required rates (especially the ones of leaves!) are respected after a
change. Meaning if we e.g. request video_pll1 to change again (this time
by media_ldb_root_clk), we have to ensure that media_disp2_pix_root_clk
has still the rate which has been set as req_rate before.

Ultimately, my trigger patch is also just a really bad workaround for a
new_rate != req_rate check, so I want to re-build the idea behind it
based on a differently defined req_rate. Need to take a deeper look on
that.

Thanks & regards
Benjamin

[1] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L3891
[2] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L2726
[3] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L2812
[4] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L2592