Received-SPF: pass (google.com: domain of linux-kernel+bounces-85188-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Wed, 28 Feb 2024 15:33:29 +0100
Message-Id: <CZGSB2O8P572.28HK6WFT43N6S@bootlin.com>
Subject: Re: [PATCH v8 03/10] clk: eyeq5: add platform driver, and init
 routine at of_clk_init()
Cc: "Gregory CLEMENT" <gregory.clement@bootlin.com>, "Michael Turquette"
 <mturquette@baylibre.com>, "Stephen Boyd" <sboyd@kernel.org>, "Rob Herring"
 <robh+dt@kernel.org>, "Krzysztof Kozlowski"
 <krzysztof.kozlowski+dt@linaro.org>, "Conor Dooley" <conor+dt@kernel.org>,
 "Thomas Bogendoerfer" <tsbogend@alpha.franken.de>, "Linus Walleij"
 <linus.walleij@linaro.org>, =?utf-8?q?Rafa=C5=82_Mi=C5=82ecki?=
 <rafal@milecki.pl>, "Philipp Zabel" <p.zabel@pengutronix.de>, "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>, <linux-gpio@vger.kernel.org>
To: "Andy Shevchenko" <andriy.shevchenko@intel.com>
From: =?utf-8?q?Th=C3=A9o_Lebrun?= <theo.lebrun@bootlin.com>
References: <20240227-mbly-clk-v8-0-c57fbda7664a@bootlin.com>
 <20240227-mbly-clk-v8-3-c57fbda7664a@bootlin.com>
 <Zd4X3NnBoEl0wu2H@smile.fi.intel.com>
In-Reply-To: <Zd4X3NnBoEl0wu2H@smile.fi.intel.com>

Hello Andy,

Thanks for the review! I'll be skipping straight forward comments.

On Tue Feb 27, 2024 at 6:11 PM CET, Andy Shevchenko wrote:
> On Tue, Feb 27, 2024 at 03:55:24PM +0100, Th=C3=A9o Lebrun wrote:
> > Add the Mobileye EyeQ5 clock controller driver. It might grow to add
> > support for other platforms from Mobileye.

[...]

> > +config COMMON_CLK_EYEQ5
> > +	bool "Clock driver for the Mobileye EyeQ5 platform"
>
> > +	depends on OF
>
> Since it's a functional dependency, why not allow compile test without OF=
 being
> enabled?

I'd do this then:

	depends on OF || COMPILE_TEST

Which is better than removing the depend line. I wouldn't want the
kernel to build fine with OF=3Dn even though we need it. OK for you?

>
> > +	depends on MACH_EYEQ5 || COMPILE_TEST
> > +	default MACH_EYEQ5
> > +	help
> > +	  This driver provides the clocks found on the Mobileye EyeQ5 SoC. It=
s
> > +	  registers live in a shared register region called OLB. It provides =
10
> > +	  read-only PLLs derived from the main crystal clock which must be co=
nstant
> > +	  and one divider clock based on one PLL.

[...]

> > +struct eq5c_pll {
> > +	int		index;
>
> Index can be negative? Any comment about this case?

No it cannot. I did not care much because structs of this type are only
defined in the following static const table, using constants from
dt-bindings header.

I'll change to unsigned int.

>
> > +	const char	*name;
> > +	u32		reg;	/* next 8 bytes are r0 and r1 */
>
> Not sure this comments gives any clarification to a mere reader of the co=
de.
> Perhaps you want to name this as reg64 (at least it will show that you ha=
ve
> 8 bytes, but I have no clue what is the semantic relationship between r0 =
and
> r1, it's quite cryptic to me). Or maybe it should be reg_0_1?

Clocks are defined by two 32-bit registers. We only store the first
register offset because they always follow each other.

I like the reg64 name and will remove the comment. This straight forward
code is found in the rest of the code, I don't think it is anything
hard to understand (ie does not need a comment):

	u32 r0 =3D readl(base_plls + pll->reg);
	u32 r1 =3D readl(base_plls + pll->reg + sizeof(r0));

[...]

> > +		return -EINVAL;
>
> I didn't get. If eq5c_init() was finished successfully, why do you need t=
o
> seems repeat what it already done? What did I miss?

The key here is that eq5c_init() iterates on eq5c_early_plls[] while
eq5c_probe() iterates on eq5c_plls[]. I've tried to hint at this in the
commit message:

> Two PLLs are required early on and are therefore registered at
> of_clk_init(). Those are pll-cpu for the GIC timer and pll-per for the
> UARTs.

Doing everything in eq5c_init() is not clean because we expect all new
clock provider drivers to be standard platform drivers. Doing
everything from a platform driver probe doesn't work because some
clocks are required earlier than platform bus init. We therefore do a
mix.

This has been approved by Stephen Boyd in this email:
https://lore.kernel.org/lkml/fa32e6fae168e10d42051b89197855e9.sboyd@kernel.=
org/

[...]

> > +	base_plls =3D devm_platform_ioremap_resource_byname(pdev, "plls");
> > +	if (IS_ERR(base_plls))
> > +		return PTR_ERR(base_plls);
> > +
> > +	base_ospi =3D devm_platform_ioremap_resource_byname(pdev, "ospi");
> > +	if (IS_ERR(base_ospi))
> > +		return PTR_ERR(base_ospi);
> > +
> > +	for (i =3D 0; i < ARRAY_SIZE(eq5c_plls); i++) {
> > +		const struct eq5c_pll *pll =3D &eq5c_plls[i];
> > +		unsigned long mult, div, acc;
> > +		u32 r0, r1;
> > +		int ret;
> > +
> > +		r0 =3D readl(base_plls + pll->reg);
> > +		r1 =3D readl(base_plls + pll->reg + sizeof(r0));
> > +
> > +		ret =3D eq5c_pll_parse_registers(r0, r1, &mult, &div, &acc);
> > +		if (ret) {
> > +			dev_warn(dev, "failed parsing state of %s\n", pll->name);
> > +			eq5c_clk_data->hws[pll->index] =3D ERR_PTR(ret);
> > +			continue;
> > +		}
> > +
> > +		hw =3D clk_hw_register_fixed_factor_with_accuracy_fwname(dev, np,
> > +				pll->name, "ref", 0, mult, div, acc);
> > +		eq5c_clk_data->hws[pll->index] =3D hw;
> > +		if (IS_ERR(hw))
>
> > +			dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n",
> > +				      pll->name);
>
> Missed return statement?

No, we still try to register all clocks even if one failed. I guess we
can call this being optimistic.

[...]

> > +static void __init eq5c_init(struct device_node *np)
> > +{
> > +	void __iomem *base_plls, *base_ospi;
> > +	int index_plls, index_ospi;
> > +	int i, ret;
>
> Why is i signed?

No reason, will be changed to unsigned int.

[...]

> > +		hw =3D clk_hw_register_fixed_factor_with_accuracy_fwname(NULL,
> > +				np, pll->name, "ref", 0, mult, div, acc);
> > +		eq5c_clk_data->hws[pll->index] =3D hw;
> > +		if (IS_ERR(hw))
> > +			pr_err("failed registering %s: %ld\n",
>
> %pe ?
>
> > +			       pll->name, PTR_ERR(hw));
>
> Is the error not critical? Is it fine? How is it supposed to work at such
> circumstances?

It is a critical error, the system will stop working in a few
milliseconds. :-) This is different from probe and it should indeed
return the error.

Thanks for the review Andy.

Have a nice day,

--
Th=C3=A9o Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com