Hi Maxime!
thanks for taking the time to look through :)
On Tue, 19 Sept 2023 at 09:07, Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> On Mon, Sep 18, 2023 at 12:40:02AM +0200, Benjamin Bara wrote:
> > From: Benjamin Bara <benjamin.bara@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > ---
> > 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
