[v5,0/4] Add Mobileye EyeQ clock support

Message

Théo Lebrun Oct. 7, 2024, 1:49 p.m. UTC

This series adds a platform driver dealing with read-only PLLs derived
from the main crystal, and some divider clocks based on those PLLs. It
also acts at the one instantiating reset and pinctrl auxiliary devices.

One special feature is that some clocks are required before platform
bus infrastructure is available; we therefore register some clocks at
of_clk_init() stage.

We support EyeQ5, EyeQ6L and EyeQ6H SoCs. The last one is special in
that there are seven instances of this system-controller. All of those
handle clocks.

The clock driver instantiates reset and pinctrl auxiliary devices
present in the same system controller. Those drivers are upstream
already, as well as the devicetree patches to switch from static clocks
to using this driver:
 - 487b1b32e317 ("reset: eyeq: add platform driver") since v6.12-rc1
 - 41795aa1f56a ("pinctrl: eyeq5: add platform driver") since v6.12-rc1
 - bde4b22dc526 ("dt-bindings: soc: mobileye: add EyeQ OLB system
   controller") since v6.11-rc1

I had a pending question [0], asking for confirmation that the static
linked list to inherit cells from of_clk_init() stage to platform
device probe is indeed the right solution. As -rc1 got released I sent
the new revision anyway.

This new revision only touches the clk-eyeq driver. We officially
declare that we do not support unbinding (using .suppress_bind_attrs)
and remove all devres calls.

Have a nice day,
Thanks,
Théo

[0]: https://lore.kernel.org/lkml/[e-mail]/

Signed-off-by: Théo Lebrun <[e-mail]>
---
Changes in v5:
- Set `.suppress_bind_attrs = true`.
- Replace devm_platform_ioremap_resource()
  by platform_get_resource() + ioremap().
- Use non-devres version of devm_kzalloc().
- Use non-devres version of devm_of_clk_add_hw_provider().
- Remove the eqc_auxdev_unregister() devres action.
- Link to v4: https://lore.kernel.org/r/[e-mail]

Changes in v4:
- clk-divider:
  - Switch flags function arguments from u16 to unsigned long.
- clk-eyeq Kconfig:
  - Remove OF dependency that is not required.
- clk-eyeq driver:
  - eqc_pll_parse_registers():
    - Make clk accuracy computation more explicit using a comment.
    - Early return when not spread spectrum, to deindent code.
  - Rename eqc_init() to eqc_early_init().
  - Remove the early match table. Register a different function in each
    CLK_OF_DECLARE_DRIVER(), which calls eqc_early_init() with an
    additional match data argument.
  - Add __initconst to early match data structs.
  - Remove spinlock on static linked list.
- Link to v3: https://lore.kernel.org/r/[e-mail]

Changes in v3:
- Kconfig: add "depends on 64BIT" because we use readq(). This removes
  the ability to COMPILE_TEST the driver on 32bit, which is fine as
  this is a SoC clk platform driver used on 64bit SoCs.
- driver: avoid `of_match_node(...)->data` because, if !CONFIG_OF,
  of_match_node(...) is resolved by the preprocessor to NULL.
  There is still a warning "eqc_early_match_table declared but unused"
  if !CONFIG_OF. We fix the <linux/of.h> header in a separate patch:
  https://lore.kernel.org/lkml/[e-mail]/
- Link to v2: see [1]

Changes in v2:
- bindings: take Acked-by: Krzysztof Kozlowski.
- driver: eqc_auxdev_create(): cast the `void __iomem *base` variable to
  (void __force *) before putting it in platform_data, to avoid sparse
  warning.
- Link to v1: see [1]

Changes since OLB v3 [0]:
 - MAINTAINERS: Move changes into a separate commit to avoid merge
   conflicts. This commit is in the MIPS series [3].
 - dt-bindings: split include/dt-bindings/ changes into its own commit.
   It is part of this clk series.
 - dt-bindings: Take Reviewed-by: Rob Herring. The include/dt-bindings/
   new commit has NOT inherited from it, just to make sure.

---
Théo Lebrun (4):
      Revert "dt-bindings: clock: mobileye,eyeq5-clk: add bindings"
      dt-bindings: clock: add Mobileye EyeQ6L/EyeQ6H clock indexes
      clk: divider: Introduce CLK_DIVIDER_EVEN_INTEGERS flag
      clk: eyeq: add driver

 .../bindings/clock/mobileye,eyeq5-clk.yaml         |  51 --
 drivers/clk/Kconfig                                |  12 +
 drivers/clk/Makefile                               |   1 +
 drivers/clk/clk-divider.c                          |  16 +-
 drivers/clk/clk-eyeq.c                             | 779 +++++++++++++++++++++
 include/dt-bindings/clock/mobileye,eyeq5-clk.h     |  21 +
 include/linux/clk-provider.h                       |  15 +-
 7 files changed, 835 insertions(+), 60 deletions(-)
---
base-commit: 31d10adec91d1b3e51d7eaea7fdf294f1ebe83df
change-id: 20240628-mbly-clk-4c6ebc716347

Best regards,

Comments

Théo Lebrun Oct. 15, 2024, 8:50 a.m. UTC | #1

Hello Stephen,

On Mon Oct 7, 2024 at 3:49 PM CEST, Théo Lebrun wrote:
> This series adds a platform driver dealing with read-only PLLs derived
> from the main crystal, and some divider clocks based on those PLLs. It
> also acts at the one instantiating reset and pinctrl auxiliary devices.

I'd be curious to get feedback on this series?
Could it make it before the next merge window?

V4 fixed all your comments but one. You implied the linked list might be
useless, but I am not convinced:

> I had a pending question [0], asking for confirmation that the static
> linked list to inherit cells from of_clk_init() stage to platform
> device probe is indeed the right solution. As -rc1 got released I sent
> the new revision anyway.
>
> [0]: https://lore.kernel.org/lkml/[e-mail]/

Quoting here the original email for full context:

On Tue Sep 24, 2024 at 4:53 PM CEST, Théo Lebrun wrote:
> On Wed Sep 18, 2024 at 7:28 AM CEST, Stephen Boyd wrote:
> > Quoting Théo Lebrun (2024-07-30 09:04:46)
> > > +       list_add_tail(&priv->list, &eqc_list);
> >
> > The list is also kind of unnecessary. Set a bool in the match_data and
> > move on? We could have some sort of static_assert() check to make sure
> > if there's a CLK_OF_DECLARE_DRIVER() then the bool is set in the
> > match_data for the driver. Such a design is cheaper than taking a lock,
> > adding to a list.
>
> This list's main goal is not to know what was early-inited. Its only
> reason for existence is that we want to get, at eqc_probe(), the cells
> pointer allocated at eqc_init().
>
> struct eqc_priv {
> 	/* this field is why we store priv inside a linked list: */
> 	struct clk_hw_onecell_data	*cells;
> 	/* the rest, we don't care much: */
> 	const struct eqc_early_match_data *early_data;
> 	const struct eqc_match_data	*data;
> 	void __iomem			*base;
> 	struct device_node		*np;
> 	struct list_head		list;
> };
>
> I do not see how to do that with a bool. We could put the pointer into
> the match data, but that would mean we'd have to make them writable
> (currently static const data). We are talking about a linked list with
> two items in the worst case (EyeQ6H), accessed twice.
>
> The reason we store the whole of priv: simpler code and we avoid mapping
> registers twice (once at eqc_init() and once at eqc_probe()).
>
> Can you confirm the current static linked list approach (without any
> spinlock) will be good for next revision?

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Stephen Boyd Oct. 17, 2024, 6:42 p.m. UTC | #2

Quoting Théo Lebrun (2024-10-15 01:50:26)
> Hello Stephen,
> 
> On Mon Oct 7, 2024 at 3:49 PM CEST, Théo Lebrun wrote:
> > This series adds a platform driver dealing with read-only PLLs derived
> > from the main crystal, and some divider clocks based on those PLLs. It
> > also acts at the one instantiating reset and pinctrl auxiliary devices.
> 
> I'd be curious to get feedback on this series?
> Could it make it before the next merge window?
> 
> V4 fixed all your comments but one. You implied the linked list might be
> useless, but I am not convinced:
> 
> > I had a pending question [0], asking for confirmation that the static
> > linked list to inherit cells from of_clk_init() stage to platform
> > device probe is indeed the right solution. As -rc1 got released I sent
> > the new revision anyway.
> >
> > [0]: https://lore.kernel.org/lkml/[e-mail]/
> 
> Quoting here the original email for full context:

Thanks!

> 
> On Tue Sep 24, 2024 at 4:53 PM CEST, Théo Lebrun wrote:
> > On Wed Sep 18, 2024 at 7:28 AM CEST, Stephen Boyd wrote:
> > > Quoting Théo Lebrun (2024-07-30 09:04:46)
> > > > +       list_add_tail(&priv->list, &eqc_list);
> > >
> > > The list is also kind of unnecessary. Set a bool in the match_data and
> > > move on? We could have some sort of static_assert() check to make sure
> > > if there's a CLK_OF_DECLARE_DRIVER() then the bool is set in the
> > > match_data for the driver. Such a design is cheaper than taking a lock,
> > > adding to a list.
> >
> > This list's main goal is not to know what was early-inited. Its only
> > reason for existence is that we want to get, at eqc_probe(), the cells
> > pointer allocated at eqc_init().
> >
> > struct eqc_priv {
> >       /* this field is why we store priv inside a linked list: */
> >       struct clk_hw_onecell_data      *cells;
> >       /* the rest, we don't care much: */
> >       const struct eqc_early_match_data *early_data;

This is __initconst and won't be valid after init, which is possible if
the driver probe is delayed beyond the time that init memory is freed.

> >       const struct eqc_match_data     *data;
> >       void __iomem                    *base;
> >       struct device_node              *np;
> >       struct list_head                list;
> > };
> >
> > I do not see how to do that with a bool. We could put the pointer into
> > the match data, but that would mean we'd have to make them writable
> > (currently static const data). We are talking about a linked list with
> > two items in the worst case (EyeQ6H), accessed twice.

Ah I missed that you were trying to stash the onecell data away. You can
register a clk provider for the same node more than once. The first
"early" one can return an error pointer for the clk indices that will be
registered later. The second onecell data can be registered in the
regular driver probe and return the clks that are registered later. See
of_clk_get_hw_from_clkspec() and how it keeps trying to find a clk if
the provider that matches the node didn't return a valid pointer.

> >
> > The reason we store the whole of priv: simpler code and we avoid mapping
> > registers twice (once at eqc_init() and once at eqc_probe()).

The IO mapping code handles duplicate mappings internally by returning
the previous virtual address. It doesn't hurt to call it again.

> >
> > Can you confirm the current static linked list approach (without any
> > spinlock) will be good for next revision?
> 

Getting rid of the list will make the code simpler and avoid the driver
probe path from accessing the early data. Please do it!