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