| Message ID | 20240227-mbly-clk-v8-3-c57fbda7664a@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for Mobileye EyeQ5 system controller | expand |
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. > > It handles 10 read-only PLLs derived from the main crystal on board. It If you wrap 'It' to the next line, overall text will look better. > exposes a table-based divider clock used for OSPI. Other platform > clocks are not configurable and therefore kept as fixed-factor > devicetree nodes. > > 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 Ditto for 'the' > UARTs. ... > +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? > + 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. ... > +#include <linux/array_size.h> > +#include <linux/bitfield.h> > +#include <linux/bits.h> > +#include <linux/clk-provider.h> > +#include <linux/device.h> > +#include <linux/err.h> + errno.h (yes, you need both) > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_address.h> + overflow.h > +#include <linux/platform_device.h> > +#include <linux/printk.h> > +#include <linux/slab.h> > +#include <linux/types.h> ... > +struct eq5c_pll { > + int index; Index can be negative? Any comment about this case? > + 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? > +}; ... > +static int eq5c_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; It's used only once. Why not just use dev->of_node there? > + void __iomem *base_plls, *base_ospi; > + struct clk_hw *hw; > + int i; > + > + /* Return potential error from eq5c_init(). */ > + if (IS_ERR(eq5c_clk_data)) > + return PTR_ERR(eq5c_clk_data); > + /* Return an error if eq5c_init() did not get called. */ > + else if (!eq5c_clk_data) Redundant 'else' > + 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? > + 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? > + } > + > + hw = clk_hw_register_divider_table_parent_hw(dev, EQ5C_OSPI_DIV_CLK_NAME, > + eq5c_clk_data->hws[EQ5C_PLL_PER], 0, > + base_ospi, 0, EQ5C_OSPI_DIV_WIDTH, 0, > + eq5c_ospi_div_table, NULL); > + eq5c_clk_data->hws[EQ5C_DIV_OSPI] = hw; > + if (IS_ERR(hw)) > + dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n", > + EQ5C_OSPI_DIV_CLK_NAME); Ditto. > + return 0; > +} > +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? > + eq5c_clk_data = kzalloc(struct_size(eq5c_clk_data, hws, EQ5C_NB_CLKS), > + GFP_KERNEL); > + if (!eq5c_clk_data) { > + ret = -ENOMEM; > + goto err; > + } > + > + eq5c_clk_data->num = EQ5C_NB_CLKS; > + > + /* > + * Mark all clocks as deferred. We register some now and others at > + * platform device probe. > + */ > + for (i = 0; i < EQ5C_NB_CLKS; i++) > + eq5c_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER); > + index_plls = of_property_match_string(np, "reg-names", "plls"); > + if (index_plls < 0) { > + ret = index_plls; > + goto err; > + } Better pattern is to avoid the output pollution in the error case. Hence ret = of_property_match_string(np, "reg-names", "plls"); if (ret < 0) goto err; index_plls = ret; > + index_ospi = of_property_match_string(np, "reg-names", "ospi"); > + if (index_ospi < 0) { > + ret = index_ospi; > + goto err; > + } Ditto. > + base_plls = of_iomap(np, index_plls); > + base_ospi = of_iomap(np, index_ospi); > + if (!base_plls || !base_ospi) { > + ret = -ENODEV; > + goto err; > + } > + for (i = 0; i < ARRAY_SIZE(eq5c_early_plls); i++) { > + const struct eq5c_pll *pll = &eq5c_early_plls[i]; > + unsigned long mult, div, acc; > + struct clk_hw *hw; > + u32 r0, r1; > + > + 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) { > + pr_warn("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(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? > + } > + > + ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, eq5c_clk_data); > + if (ret) { > + pr_err("failed registering clk provider: %d\n", ret); > + goto err; > + } > + > + return; > + > +err: > + kfree(eq5c_clk_data); > + /* Signal to platform driver probe that we failed init. */ > + eq5c_clk_data = ERR_PTR(ret); > +} > + > +CLK_OF_DECLARE_DRIVER(eq5c, "mobileye,eyeq5-clk", eq5c_init);
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
On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote: > 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: [...] > > > + 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? Yes! [...] > > > + 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)); Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h) can be used in this case? It will be much better overall and be aligned with reg64 name. [...] > > 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. Am I missing something or these two pieces are using the same IO resources? This looks like a lot of code duplication without clear benefit. Perhaps you can have a helper? > This has been approved by Stephen Boyd in this email: > https://lore.kernel.org/lkml/fa32e6fae168e10d42051b89197855e9.sboyd@kernel.org/ OK! [...] > > > + 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. But how critical these clocks are? I believe we should panic it we have no critical calls be available. Otherwise, why '_err_'? Shouldn't be dev_warn()?
Hello, On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote: > 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: [...] > > > > + 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)); > > Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h) > can be used in this case? It will be much better overall and be aligned with > reg64 name. The doc talks in terms of 32-bit registers. I do not see a reason to work in 64-bit. If we get a 64-bit value that we need to split we need to think about the endianness of our platform, which makes things more complex than just reading both values independently. > [...] > > > > 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. > > Am I missing something or these two pieces are using the same IO resources? > This looks like a lot of code duplication without clear benefit. Perhaps > you can have a helper? There are two subtle differences that make creating a helper difficult: - Logging, pr_*() vs dev_*(). Second option is preferred but only available once a device is created. - Behavior on error: we stop the world for early clocks but keep going for normal clocks. [...] > > > > + 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. > > But how critical these clocks are? I believe we should panic it we have no > critical calls be available. Otherwise, why '_err_'? Shouldn't be dev_warn()? Indeed printing should be dev_warn(), I missed that. Thanks Andy, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Thu, Feb 29, 2024 at 03:27:01PM +0100, Théo Lebrun wrote: > On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote: > > 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: [...] > > > > > + 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)); > > > > Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h) > > can be used in this case? It will be much better overall and be aligned with > > reg64 name. > > The doc talks in terms of 32-bit registers. I do not see a reason to > work in 64-bit. If we get a 64-bit value that we need to split we need > to think about the endianness of our platform, which makes things more > complex than just reading both values independently. 1) Would be nice to test on the real HW to confirm it doesn't accept 64-bit IO. 2) Still I see a benefit from using lo_hi_readq() and friends directly. [...] > > > > 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. > > > > Am I missing something or these two pieces are using the same IO resources? > > This looks like a lot of code duplication without clear benefit. Perhaps > > you can have a helper? > > There are two subtle differences that make creating a helper difficult: > > - Logging, pr_*() vs dev_*(). Second option is preferred but only > available once a device is created. Some code uses (yeah, arguable that it's better, but depends on how much the real deduplication takes) if (dev) dev_*(...); else pr_*(...); > - Behavior on error: we stop the world for early clocks but keep going > for normal clocks. ...(..., bool skip_errors) { ... } (with the same caveat)?
Hello, On Thu Feb 29, 2024 at 3:59 PM CET, Andy Shevchenko wrote: > On Thu, Feb 29, 2024 at 03:27:01PM +0100, Théo Lebrun wrote: > > On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote: > > > 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: > > [...] > > > > > > > + 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)); > > > > > > Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h) > > > can be used in this case? It will be much better overall and be aligned with > > > reg64 name. > > > > The doc talks in terms of 32-bit registers. I do not see a reason to > > work in 64-bit. If we get a 64-bit value that we need to split we need > > to think about the endianness of our platform, which makes things more > > complex than just reading both values independently. > > 1) Would be nice to test on the real HW to confirm it doesn't accept 64-bit IO. Just tested, it works. No error on the memory bus. And checked assembly generated was a single 64-bit instructions. It might not work on other hardware revisions though. I can't remember if memory bus is changing across them. > 2) Still I see a benefit from using lo_hi_readq() and friends directly. So it is: u32 r0 = readl(base_plls + pll->reg64); u32 r1 = readl(base_plls + pll->reg64 + sizeof(r0)); vs: u64 r = lo_hi_readq(base_plls + pll->regs64); u32 r0 = r; u32 r1 = r >> 32; One is straight forward, the other uses an obscure helper that code readers must understand and follows that with bit manipulation. > > [...] > > > > > > 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. > > > > > > Am I missing something or these two pieces are using the same IO resources? > > > This looks like a lot of code duplication without clear benefit. Perhaps > > > you can have a helper? > > > > There are two subtle differences that make creating a helper difficult: > > > > - Logging, pr_*() vs dev_*(). Second option is preferred but only > > available once a device is created. > > Some code uses (yeah, arguable that it's better, but depends on how much > the real deduplication takes) > > if (dev) > dev_*(...); > else > pr_*(...); > > > - Behavior on error: we stop the world for early clocks but keep going > > for normal clocks. > > ...(..., bool skip_errors) > { > ... > } > > (with the same caveat)? I started trying it out, but the combination of both flags means dealing with errors would look like: ret = foo(); if (ret) { if (!skip_errors) { if (dev) dev_err(dev, "..."); else pr_err("..."); return ret; } if (dev) dev_warn(dev, "..."); else pr_warn("..."); } There are two errors to handle, that makes a mess out of the code. Having a little bit of repetition but straight forward code is nicer in my opinion. At least we tried! Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Thu, Feb 29, 2024 at 04:40:25PM +0100, Théo Lebrun wrote: > On Thu Feb 29, 2024 at 3:59 PM CET, Andy Shevchenko wrote: > > On Thu, Feb 29, 2024 at 03:27:01PM +0100, Théo Lebrun wrote: > > > On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote: > > > > 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: [...] > > > > > > > + 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)); > > > > > > > > Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h) > > > > can be used in this case? It will be much better overall and be aligned with > > > > reg64 name. > > > > > > The doc talks in terms of 32-bit registers. I do not see a reason to > > > work in 64-bit. If we get a 64-bit value that we need to split we need > > > to think about the endianness of our platform, which makes things more > > > complex than just reading both values independently. > > > > 1) Would be nice to test on the real HW to confirm it doesn't accept 64-bit IO. > > Just tested, it works. No error on the memory bus. And checked assembly > generated was a single 64-bit instructions. > > It might not work on other hardware revisions though. I can't remember > if memory bus is changing across them. > > > 2) Still I see a benefit from using lo_hi_readq() and friends directly. > > So it is: > > u32 r0 = readl(base_plls + pll->reg64); > u32 r1 = readl(base_plls + pll->reg64 + sizeof(r0)); > > vs: > > u64 r = lo_hi_readq(base_plls + pll->regs64); > u32 r0 = r; > u32 r1 = r >> 32; It depends to the semantics of these two. How hard do they coupled to each other semantically? I.o.w. can they always be considered as 64-bit register with the respective bitfields? (And note FIELD_GET() here is your friend.) > One is straight forward, the other uses an obscure helper that code > readers must understand and follows that with bit manipulation. [...] > There are two errors to handle, that makes a mess out of the code. > Having a little bit of repetition but straight forward code is nicer in > my opinion. At least we tried! Yes! Perhaps you can add a couple of words into commit message to explain this detail of implementation (that code in two parts is not so identical to be easily deduplicated).
Hello, On Thu Feb 29, 2024 at 4:48 PM CET, Andy Shevchenko wrote: > On Thu, Feb 29, 2024 at 04:40:25PM +0100, Théo Lebrun wrote: > > On Thu Feb 29, 2024 at 3:59 PM CET, Andy Shevchenko wrote: > > > On Thu, Feb 29, 2024 at 03:27:01PM +0100, Théo Lebrun wrote: > > > > On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote: > > > > > 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: > > [...] > > > > > > > > > + 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)); > > > > > > > > > > Btw, why readq()/writeq() (with probably the inclusion of io-64-nonatomic-lo-hi.h) > > > > > can be used in this case? It will be much better overall and be aligned with > > > > > reg64 name. > > > > > > > > The doc talks in terms of 32-bit registers. I do not see a reason to > > > > work in 64-bit. If we get a 64-bit value that we need to split we need > > > > to think about the endianness of our platform, which makes things more > > > > complex than just reading both values independently. > > > > > > 1) Would be nice to test on the real HW to confirm it doesn't accept 64-bit IO. > > > > Just tested, it works. No error on the memory bus. And checked assembly > > generated was a single 64-bit instructions. > > > > It might not work on other hardware revisions though. I can't remember > > if memory bus is changing across them. > > > > > 2) Still I see a benefit from using lo_hi_readq() and friends directly. > > > > So it is: > > > > u32 r0 = readl(base_plls + pll->reg64); > > u32 r1 = readl(base_plls + pll->reg64 + sizeof(r0)); > > > > vs: > > > > u64 r = lo_hi_readq(base_plls + pll->regs64); > > > u32 r0 = r; > > u32 r1 = r >> 32; > > It depends to the semantics of these two. How hard do they coupled to each > other semantically? I.o.w. can they always be considered as 64-bit register > with the respective bitfields? (And note FIELD_GET() here is your friend.) OLB (the memory region) has always been described as a list of 32-bit registers. The semantics lean in the camp of two readl(). > > One is straight forward, the other uses an obscure helper that code > > readers must understand and follows that with bit manipulation. > > [...] > > > There are two errors to handle, that makes a mess out of the code. > > Having a little bit of repetition but straight forward code is nicer in > > my opinion. At least we tried! > > Yes! Perhaps you can add a couple of words into commit message to explain > this detail of implementation (that code in two parts is not so identical > to be easily deduplicated). Yes, will do. I get why from a reader's point-of-view it looks like duplicate code. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Quoting Théo Lebrun (2024-02-29 07:40:25) > Hello, > > On Thu Feb 29, 2024 at 3:59 PM CET, Andy Shevchenko wrote: > > On Thu, Feb 29, 2024 at 03:27:01PM +0100, Théo Lebrun wrote: > > > On Wed, Feb 28, 2024 at 03:33:29PM +0100, Théo Lebrun wrote: > > > > 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: > > > 2) Still I see a benefit from using lo_hi_readq() and friends directly. > > So it is: > > u32 r0 = readl(base_plls + pll->reg64); > u32 r1 = readl(base_plls + pll->reg64 + sizeof(r0)); > > vs: > > u64 r = lo_hi_readq(base_plls + pll->regs64); > u32 r0 = r; > u32 r1 = r >> 32; > > One is straight forward, the other uses an obscure helper that code > readers must understand and follows that with bit manipulation. > Just use readq() and include the correct header please. We know what readq() is in the kernel.
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 50af5fc7f570..c79b38f60b1b 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -218,6 +218,17 @@ config COMMON_CLK_EN7523 This driver provides the fixed clocks and gates present on Airoha ARM silicon. +config COMMON_CLK_EYEQ5 + bool "Clock driver for the Mobileye EyeQ5 platform" + depends on OF + 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. + config COMMON_CLK_FSL_FLEXSPI tristate "Clock driver for FlexSPI on Layerscape SoCs" depends on ARCH_LAYERSCAPE || COMPILE_TEST diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 14fa8d4ecc1f..81c4d11ca437 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -32,6 +32,7 @@ obj-$(CONFIG_ARCH_CLPS711X) += clk-clps711x.o obj-$(CONFIG_COMMON_CLK_CS2000_CP) += clk-cs2000-cp.o obj-$(CONFIG_ARCH_SPARX5) += clk-sparx5.o obj-$(CONFIG_COMMON_CLK_EN7523) += clk-en7523.o +obj-$(CONFIG_COMMON_CLK_EYEQ5) += clk-eyeq5.o obj-$(CONFIG_COMMON_CLK_FIXED_MMIO) += clk-fixed-mmio.o obj-$(CONFIG_COMMON_CLK_FSL_FLEXSPI) += clk-fsl-flexspi.o obj-$(CONFIG_COMMON_CLK_FSL_SAI) += clk-fsl-sai.o diff --git a/drivers/clk/clk-eyeq5.c b/drivers/clk/clk-eyeq5.c new file mode 100644 index 000000000000..85bf6f1c3fa3 --- /dev/null +++ b/drivers/clk/clk-eyeq5.c @@ -0,0 +1,306 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * PLL clock driver for the Mobileye EyeQ5 platform. + * + * This controller handles 10 read-only PLLs, all derived from the same main + * crystal clock. It also exposes one divider clock, a child of one of the + * PLLs. The parent clock is expected to be constant. This driver's registers + * live in a shared region called OLB. Two PLLs must be initialized by + * of_clk_init(). + * + * We use eq5c_ as prefix, as-in "EyeQ5 Clock", but way shorter. + * + * Copyright (C) 2024 Mobileye Vision Technologies Ltd. + */ + +#define pr_fmt(fmt) "clk-eyeq5: " fmt + +#include <linux/array_size.h> +#include <linux/bitfield.h> +#include <linux/bits.h> +#include <linux/clk-provider.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/printk.h> +#include <linux/slab.h> +#include <linux/types.h> + +#include <dt-bindings/clock/mobileye,eyeq5-clk.h> + +/* In frac mode, it enables fractional noise canceling DAC. Else, no function. */ +#define PCSR0_DAC_EN BIT(0) +/* Fractional or integer mode */ +#define PCSR0_DSM_EN BIT(1) +#define PCSR0_PLL_EN BIT(2) +/* All clocks output held at 0 */ +#define PCSR0_FOUTPOSTDIV_EN BIT(3) +#define PCSR0_POST_DIV1 GENMASK(6, 4) +#define PCSR0_POST_DIV2 GENMASK(9, 7) +#define PCSR0_REF_DIV GENMASK(15, 10) +#define PCSR0_INTIN GENMASK(27, 16) +#define PCSR0_BYPASS BIT(28) +/* Bits 30..29 are reserved */ +#define PCSR0_PLL_LOCKED BIT(31) + +#define PCSR1_RESET BIT(0) +#define PCSR1_SSGC_DIV GENMASK(4, 1) +/* Spread amplitude (% = 0.1 * SPREAD[4:0]) */ +#define PCSR1_SPREAD GENMASK(9, 5) +#define PCSR1_DIS_SSCG BIT(10) +/* Down-spread or center-spread */ +#define PCSR1_DOWN_SPREAD BIT(11) +#define PCSR1_FRAC_IN GENMASK(31, 12) + +static struct clk_hw_onecell_data *eq5c_clk_data; + +struct eq5c_pll { + int index; + const char *name; + u32 reg; /* next 8 bytes are r0 and r1 */ +}; + +/* Required early for the GIC timer (pll-cpu) and UARTs (pll-per). */ +static const struct eq5c_pll eq5c_early_plls[] = { + { .index = EQ5C_PLL_CPU, .name = "pll-cpu", .reg = 0x00, }, + { .index = EQ5C_PLL_PER, .name = "pll-per", .reg = 0x30, }, +}; + +static const struct eq5c_pll eq5c_plls[] = { + { .index = EQ5C_PLL_VMP, .name = "pll-vmp", .reg = 0x08, }, + { .index = EQ5C_PLL_PMA, .name = "pll-pma", .reg = 0x10, }, + { .index = EQ5C_PLL_VDI, .name = "pll-vdi", .reg = 0x18, }, + { .index = EQ5C_PLL_DDR0, .name = "pll-ddr0", .reg = 0x20, }, + { .index = EQ5C_PLL_PCI, .name = "pll-pci", .reg = 0x28, }, + { .index = EQ5C_PLL_PMAC, .name = "pll-pmac", .reg = 0x38, }, + { .index = EQ5C_PLL_MPC, .name = "pll-mpc", .reg = 0x40, }, + { .index = EQ5C_PLL_DDR1, .name = "pll-ddr1", .reg = 0x48, }, +}; + +#define EQ5C_OSPI_DIV_CLK_NAME "div-ospi" +#define EQ5C_OSPI_DIV_WIDTH 4 + +#define EQ5C_NB_CLKS (ARRAY_SIZE(eq5c_early_plls) + ARRAY_SIZE(eq5c_plls) + 1) + +static const struct clk_div_table eq5c_ospi_div_table[] = { + { .val = 0, .div = 2 }, + { .val = 1, .div = 4 }, + { .val = 2, .div = 6 }, + { .val = 3, .div = 8 }, + { .val = 4, .div = 10 }, + { .val = 5, .div = 12 }, + { .val = 6, .div = 14 }, + { .val = 7, .div = 16 }, + {} /* sentinel */ +}; + +static int eq5c_pll_parse_registers(u32 r0, u32 r1, unsigned long *mult, + unsigned long *div, unsigned long *acc) +{ + if (r0 & PCSR0_BYPASS) { + *mult = 1; + *div = 1; + *acc = 0; + return 0; + } + + if (!(r0 & PCSR0_PLL_LOCKED)) + return -EINVAL; + + *mult = FIELD_GET(PCSR0_INTIN, r0); + *div = FIELD_GET(PCSR0_REF_DIV, r0); + if (r0 & PCSR0_FOUTPOSTDIV_EN) + *div *= FIELD_GET(PCSR0_POST_DIV1, r0) * FIELD_GET(PCSR0_POST_DIV2, r0); + + /* Fractional mode, in 2^20 (0x100000) parts. */ + if (r0 & PCSR0_DSM_EN) { + *div *= 0x100000; + *mult = *mult * 0x100000 + FIELD_GET(PCSR1_FRAC_IN, r1); + } + + if (!*mult || !*div) + return -EINVAL; + + /* Spread spectrum. */ + if (!(r1 & (PCSR1_RESET | PCSR1_DIS_SSCG))) { + /* + * Spread is 1/1000 parts of frequency, accuracy is half of + * that. To get accuracy, convert to ppb (parts per billion). + */ + u32 spread = FIELD_GET(PCSR1_SPREAD, r1); + + *acc = spread * 500000; + if (r1 & PCSR1_DOWN_SPREAD) { + /* + * Downspreading: the central frequency is half a + * spread lower. + */ + *mult *= 2000 - spread; + *div *= 2000; + } + } else { + *acc = 0; + } + + return 0; +} + +static int eq5c_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + void __iomem *base_plls, *base_ospi; + struct clk_hw *hw; + int i; + + /* Return potential error from eq5c_init(). */ + if (IS_ERR(eq5c_clk_data)) + return PTR_ERR(eq5c_clk_data); + /* Return an error if eq5c_init() did not get called. */ + else if (!eq5c_clk_data) + return -EINVAL; + + 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); + } + + hw = clk_hw_register_divider_table_parent_hw(dev, EQ5C_OSPI_DIV_CLK_NAME, + eq5c_clk_data->hws[EQ5C_PLL_PER], 0, + base_ospi, 0, EQ5C_OSPI_DIV_WIDTH, 0, + eq5c_ospi_div_table, NULL); + eq5c_clk_data->hws[EQ5C_DIV_OSPI] = hw; + if (IS_ERR(hw)) + dev_err_probe(dev, PTR_ERR(hw), "failed registering %s\n", + EQ5C_OSPI_DIV_CLK_NAME); + + return 0; +} + +static const struct of_device_id eq5c_match_table[] = { + { .compatible = "mobileye,eyeq5-clk" }, + {} +}; +MODULE_DEVICE_TABLE(of, eq5c_match_table); + +static struct platform_driver eq5c_driver = { + .probe = eq5c_probe, + .driver = { + .name = "clk-eyeq5", + .of_match_table = eq5c_match_table, + }, +}; +builtin_platform_driver(eq5c_driver); + +static void __init eq5c_init(struct device_node *np) +{ + void __iomem *base_plls, *base_ospi; + int index_plls, index_ospi; + int i, ret; + + eq5c_clk_data = kzalloc(struct_size(eq5c_clk_data, hws, EQ5C_NB_CLKS), + GFP_KERNEL); + if (!eq5c_clk_data) { + ret = -ENOMEM; + goto err; + } + + eq5c_clk_data->num = EQ5C_NB_CLKS; + + /* + * Mark all clocks as deferred. We register some now and others at + * platform device probe. + */ + for (i = 0; i < EQ5C_NB_CLKS; i++) + eq5c_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER); + + index_plls = of_property_match_string(np, "reg-names", "plls"); + if (index_plls < 0) { + ret = index_plls; + goto err; + } + + index_ospi = of_property_match_string(np, "reg-names", "ospi"); + if (index_ospi < 0) { + ret = index_ospi; + goto err; + } + + base_plls = of_iomap(np, index_plls); + base_ospi = of_iomap(np, index_ospi); + if (!base_plls || !base_ospi) { + ret = -ENODEV; + goto err; + } + + for (i = 0; i < ARRAY_SIZE(eq5c_early_plls); i++) { + const struct eq5c_pll *pll = &eq5c_early_plls[i]; + unsigned long mult, div, acc; + struct clk_hw *hw; + u32 r0, r1; + + 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) { + pr_warn("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(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", + pll->name, PTR_ERR(hw)); + } + + ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, eq5c_clk_data); + if (ret) { + pr_err("failed registering clk provider: %d\n", ret); + goto err; + } + + return; + +err: + kfree(eq5c_clk_data); + /* Signal to platform driver probe that we failed init. */ + eq5c_clk_data = ERR_PTR(ret); +} + +CLK_OF_DECLARE_DRIVER(eq5c, "mobileye,eyeq5-clk", eq5c_init);
Add the Mobileye EyeQ5 clock controller driver. It might grow to add support for other platforms from Mobileye. It handles 10 read-only PLLs derived from the main crystal on board. It exposes a table-based divider clock used for OSPI. Other platform clocks are not configurable and therefore kept as fixed-factor devicetree nodes. 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. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- drivers/clk/Kconfig | 11 ++ drivers/clk/Makefile | 1 + drivers/clk/clk-eyeq5.c | 306 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 318 insertions(+)