Re: [PATCH 3/5] clk: eyeq5: add controller

From: "Théo Lebrun" <theo.lebrun@bootlin.com>
To: "Théo Lebrun" <theo.lebrun@bootlin.com>,
	"Stephen Boyd" <sboyd@kernel.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Gregory CLEMENT" <gregory.clement@bootlin.com>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Michael Turquette" <mturquette@baylibre.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Thomas Bogendoerfer" <tsbogend@alpha.franken.de>
Cc: "Vladimir Kondratiev" <vladimir.kondratiev@mobileye.com>,
	<linux-mips@vger.kernel.org>, <linux-clk@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Tawfik Bayouk" <tawfik.bayouk@mobileye.com>
Subject: Re: [PATCH 3/5] clk: eyeq5: add controller
Date: Wed, 27 Dec 2023 17:30:20 +0100	[thread overview]
Message-ID: <CXZ9C7Y8QT0D.38ZNTXUY0HWUW@bootlin.com> (raw)
In-Reply-To: <CXUTPV1ZOSID.323RSEP4BL2AT@bootlin.com>

Hello,

On Fri Dec 22, 2023 at 12:25 PM CET, Théo Lebrun wrote:
> Hello,
>
> I've seen all your comments, thanks for that. I have a follow up about
> one:
>
> On Wed Dec 20, 2023 at 12:09 AM CET, Stephen Boyd wrote:
> > Quoting Théo Lebrun (2023-12-18 09:14:18)
> > > Add the Mobileye EyeQ5 clock controller driver. See the header comment
> > > for more information on how it works.
> >
> > "See the header" is like saying "Read the code" which is pretty obvious.
> > Remove this sentence and tell us why only the PLLs are supported at the
> > moment or something like that.
> >
> > > This driver is specific to this
> > > platform; it might grow to add later support of other platforms from
> > > Mobileye.
> > > 
> > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> > > ---
> > >  MAINTAINERS             |   1 +
> > >  drivers/clk/Kconfig     |  11 +++
> > >  drivers/clk/Makefile    |   1 +
> > >  drivers/clk/clk-eyeq5.c | 211 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 224 insertions(+)
> > > 
>
> [...]
>
> > > diff --git a/drivers/clk/clk-eyeq5.c b/drivers/clk/clk-eyeq5.c
> > > new file mode 100644
> > > index 000000000000..74bcb8cec5c1
> > > --- /dev/null
> > > +++ b/drivers/clk/clk-eyeq5.c
>
> [...]
>
> > > +       of_clk_add_hw_provider(np, of_clk_hw_onecell_get, data);
> > > +}
> > > +
> > > +CLK_OF_DECLARE_DRIVER(eq5c, "mobileye,eyeq5-clk", eq5c_init);
> >
> > Please use a platform driver.
>
> I've been trying to do that but I had a stall at boot. I initially
> associated it with the UART driver acquiring a clock too early but
> instead it is the CPU timer clocksource driver that consumes one of our
> clock way earlier than any platform driver initialisation.
>
> The clocksource driver we are talking about is this one for reference:
> https://elixir.bootlin.com/linux/v6.6.8/source/drivers/clocksource/mips-gic-timer.c
>
> Its usage of TIMER_OF_DECLARE means it gets probed by timer_probe ->
> plat_time_init -> time_init -> start_kernel. This is way before any
> initcalls. Our prior use of CLK_OF_DECLARE_DRIVER meant that we got
> probed by of_clk_init -> plat_time_init.
>
> I'm guessing we are not the first one in this situation; any advice on
> how to deal with it?

I went ahead with a V2, feeling it would be more productive to come up
with something and gather comments on concrete stuff. There were many
other things to address anyway.

I've addressed this point by declaring a dummy fixed-clock in the
devicetree that gets fed to the GIC timer. It is pretty much the same
thing as using `clock-frequency` which this specific clocksource uses
if `of_clk_get(node, 0)` fails. With the sent approach we have the
timer appear in the clock tree as a consumer.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com