Re: [PATCH v8 03/10] clk: eyeq5: add platform driver, and init routine at of_clk_init()

["Théo Lebrun" <theo.lebrun@bootlin.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éo 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=n 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. Its
> > +	  registers live in a shared register region called OLB. It provides 10
> > +	  read-only PLLs derived from the main crystal clock which must be constant
> > +	  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 code.
> Perhaps you want to name this as reg64 (at least it will show that you have
> 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 = readl(base_plls + pll->reg);
	u32 r1 = readl(base_plls + pll->reg + sizeof(r0));

[...]

> > +		return -EINVAL;
>
> I didn't get. If eq5c_init() was finished successfully, why do you need to
> 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 = devm_platform_ioremap_resource_byname(pdev, "plls");
> > +	if (IS_ERR(base_plls))
> > +		return PTR_ERR(base_plls);
> > +
> > +	base_ospi = devm_platform_ioremap_resource_byname(pdev, "ospi");
> > +	if (IS_ERR(base_ospi))
> > +		return PTR_ERR(base_ospi);
> > +
> > +	for (i = 0; i < ARRAY_SIZE(eq5c_plls); i++) {
> > +		const struct eq5c_pll *pll = &eq5c_plls[i];
> > +		unsigned long mult, div, acc;
> > +		u32 r0, r1;
> > +		int ret;
> > +
> > +		r0 = readl(base_plls + pll->reg);
> > +		r1 = readl(base_plls + pll->reg + sizeof(r0));
> > +
> > +		ret = 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] = ERR_PTR(ret);
> > +			continue;
> > +		}
> > +
> > +		hw = clk_hw_register_fixed_factor_with_accuracy_fwname(dev, np,
> > +				pll->name, "ref", 0, mult, div, acc);
> > +		eq5c_clk_data->hws[pll->index] = 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 = clk_hw_register_fixed_factor_with_accuracy_fwname(NULL,
> > +				np, pll->name, "ref", 0, mult, div, acc);
> > +		eq5c_clk_data->hws[pll->index] = 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éo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com