[v8,03/10] clk: eyeq5: add platform driver, and init routine at of_clk_init()
Commit Message
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(+)
Comments
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.
@@ -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
@@ -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
new file mode 100644
@@ -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);