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@xxxxxxxxxx/
[...]
> > + 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
