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

Message ID 20240227-mbly-clk-v8-3-c57fbda7664a@bootlin.com
State New
Headers
Series Add support for Mobileye EyeQ5 system controller |

Commit Message

Théo Lebrun Feb. 27, 2024, 2:55 p.m. UTC
  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

Andy Shevchenko Feb. 27, 2024, 5:11 p.m. UTC | #1
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);
  
Théo Lebrun Feb. 28, 2024, 2:33 p.m. UTC | #2
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
  
Andy Shevchenko Feb. 29, 2024, 11:15 a.m. UTC | #3
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()?
  
Théo Lebrun Feb. 29, 2024, 2:27 p.m. UTC | #4
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
  
Andy Shevchenko Feb. 29, 2024, 2:59 p.m. UTC | #5
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)?
  
Théo Lebrun Feb. 29, 2024, 3:40 p.m. UTC | #6
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
  
Andy Shevchenko Feb. 29, 2024, 3:48 p.m. UTC | #7
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).
  
Théo Lebrun Feb. 29, 2024, 3:57 p.m. UTC | #8
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
  
Stephen Boyd March 1, 2024, 1:33 a.m. UTC | #9
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.
  

Patch

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);