[05/11] clk: eyeq: add driver

Add Mobileye EyeQ5, EyeQ6L and EyeQ6H clock controller driver. It is
both a platform driver and a hook onto of_clk_init() used for clocks
required early (GIC timer, UARTs).

For some compatible, it is both at the same time. eqc_init() initialises
early PLLs and stores clock array in a static linked list. It marks
other clocks as deferred. eqc_probe() retrieves the clock array and
adds all remaining clocks.

It exposes read-only PLLs derived from the main crystal on board. It
also exposes another type of clocks: divider clocks. They always have
even divisors and have one PLL as parent.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 MAINTAINERS            |   1 +
 drivers/clk/Kconfig    |  11 +
 drivers/clk/Makefile   |   1 +
 drivers/clk/clk-eyeq.c | 644 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 657 insertions(+)

Quoting Théo Lebrun (2024-04-10 10:12:34)
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 50af5fc7f570..1eb6e70977a3 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_EYEQ
> +       bool "Clock driver for the Mobileye EyeQ platform"
> +       depends on OF || COMPILE_TEST

The OF build dependency looks useless as we have the MACH_ dependency
below.

> +       depends on MACH_EYEQ5 || MACH_EYEQ6H || COMPILE_TEST
> +       default MACH_EYEQ5 || MACH_EYEQ6H
> +       help
> +         This driver provides clocks found on Mobileye EyeQ5, EyeQ6L and Eye6H
> +         SoCs. Controllers live in shared register regions called OLB. Driver
> +         provides read-only PLLs, derived from the main crystal clock (which
> +         must be constant). It also exposes some divider clocks.
> +
>  config COMMON_CLK_FSL_FLEXSPI
>         tristate "Clock driver for FlexSPI on Layerscape SoCs"
>         depends on ARCH_LAYERSCAPE || COMPILE_TEST
> diff --git a/drivers/clk/clk-eyeq.c b/drivers/clk/clk-eyeq.c
> new file mode 100644
> index 000000000000..bb2535010ae6
> --- /dev/null
> +++ b/drivers/clk/clk-eyeq.c
> @@ -0,0 +1,644 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * PLL clock driver for the Mobileye EyeQ5, EyeQ6L and EyeQ6H platforms.
> + *
> + * This controller handles read-only PLLs, all derived from the same main
> + * crystal clock. It also exposes divider clocks, those are children to PLLs.
> + * Parent clock is expected to be constant. This driver's registers live in
> + * a shared region called OLB. Some PLLs are initialised early by of_clk_init().

Is OLB a different DT node? It sounds like maybe this is trying to jam a
driver into DT when the OLB node should be a #clock-cells node.

> + *
> + * We use eqc_ as prefix, as-in "EyeQ Clock", but way shorter.
> + *
> + * Copyright (C) 2024 Mobileye Vision Technologies Ltd.
> + */
> +
> +#define pr_fmt(fmt) "clk-eyeq: " 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/errno.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/overflow.h>
> +#include <linux/platform_device.h>
> +#include <linux/printk.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
> +#include <dt-bindings/clock/mobileye,eyeq5-clk.h>
> +
> +#define EQC_MAX_DIV_COUNT              4
> +
> +/* 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)
> +
> +/*
> + * Driver might register clock provider from eqc_init() if PLLs are required
> + * early (before platform bus is ready). Store struct eqc_priv inside linked
> + * list to pass clock provider from eqc_init() to eqc_probe() and register
> + * remaining clocks from platform device probe.
> + *
> + * Clock provider is NOT created by eqc_init() if no early clock is required.
> + * Store as linked list because EyeQ6H has multiple clock controller instances.
> + * Matching is done based on devicetree node pointer.
> + */
> +static DEFINE_SPINLOCK(eqc_list_slock);
> +static LIST_HEAD(eqc_list);
> +
> +struct eqc_pll {
> +       unsigned int    index;
> +       const char      *name;
> +       u32             reg64;
> +};
> +
> +/*
> + * Divider clock. Divider is 2*(v+1), with v the register value.
> + * Min divider is 2, max is 2*(2^width).
> + */
> +struct eqc_div {
> +       unsigned int    index;
> +       const char      *name;
> +       unsigned int    parent;
> +       const char      *resource_name;
> +       u8              shift;
> +       u8              width;
> +};
> +
> +struct eqc_match_data {
> +       unsigned int            early_pll_count;
> +       const struct eqc_pll    *early_plls;
> +
> +       unsigned int            pll_count;
> +       const struct eqc_pll    *plls;
> +
> +       unsigned int            div_count;
> +       const struct eqc_div    *divs;
> +};
> +
> +struct eqc_priv {
> +       struct clk_hw_onecell_data      *cells;
> +       const struct eqc_match_data     *data;
> +       void __iomem                    *base_plls;
> +       struct device_node              *np;
> +       struct list_head                list;
> +};
> +
> +static int eqc_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 unsigned int eqc_compute_clock_count(const struct eqc_match_data *data)
> +{
> +       unsigned int i, nb_clks = 0;
> +
> +       for (i = 0; i < data->early_pll_count; i++)
> +               if (data->early_plls[i].index >= nb_clks)
> +                       nb_clks = data->early_plls[i].index + 1;
> +       for (i = 0; i < data->pll_count; i++)
> +               if (data->plls[i].index >= nb_clks)
> +                       nb_clks = data->plls[i].index + 1;
> +       for (i = 0; i < data->div_count; i++)
> +               if (data->divs[i].index >= nb_clks)
> +                       nb_clks = data->divs[i].index + 1;
> +
> +       /* We expect the biggest clock index to be 1 below the clock count. */
> +       WARN_ON(nb_clks != data->early_pll_count + data->pll_count + data->div_count);
> +
> +       return nb_clks;
> +}
> +
> +static int eqc_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       void __iomem *div_resources[EQC_MAX_DIV_COUNT];
> +       struct device_node *np = dev->of_node;
> +       const struct eqc_match_data *data;
> +       struct eqc_priv *priv = NULL;
> +       struct clk_hw *hw;
> +       unsigned int i;
> +
> +       data = device_get_match_data(dev);
> +       if (!data)
> +               return -ENODEV;
> +
> +       if (data->early_pll_count) {
> +               /* Device got inited early. Retrieve clock provider from list. */
> +               struct eqc_priv *entry;
> +
> +               spin_lock(&eqc_list_slock);
> +               list_for_each_entry(entry, &eqc_list, list) {
> +                       if (entry->np == np) {
> +                               priv = entry;
> +                               break;
> +                       }
> +               }
> +               spin_unlock(&eqc_list_slock);
> +
> +               if (!priv)
> +                       return -ENODEV;

This can be a sub-function.

> +       } else {
> +               /* Device did NOT get init early. Do it now. */
> +               unsigned int nb_clks;
> +
> +               priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +               if (!priv)
> +                       return -ENOMEM;
> +
> +               priv->np = np;
> +               priv->data = data;
> +
> +               nb_clks = eqc_compute_clock_count(data);
> +               priv->cells = devm_kzalloc(dev, struct_size(priv->cells, hws, nb_clks),
> +                                          GFP_KERNEL);
> +               if (!priv->cells)
> +                       return -ENOMEM;
> +
> +               priv->cells->num = nb_clks;
> +
> +               /*
> +                * We expect named resources if divider clocks are present.
> +                * Else, we only expect one resource.
> +                */
> +               if (data->div_count)
> +                       priv->base_plls = devm_platform_ioremap_resource_byname(pdev, "plls");
> +               else
> +                       priv->base_plls = devm_platform_ioremap_resource(pdev, 0);
> +               if (IS_ERR(priv->base_plls))
> +                       return PTR_ERR(priv->base_plls);
> +       }
> +
> +       for (i = 0; i < data->pll_count; i++) {
> +               const struct eqc_pll *pll = &data->plls[i];
> +               unsigned long mult, div, acc;
> +               u32 r0, r1;
> +               u64 val;
> +               int ret;

All variables should be declared at the start of the function. Once it
becomes "too heavy" you can split it up into smaller functions, that
again have all variables declared at the start of the function.

> +
> +               val = readq(priv->base_plls + pll->reg64);
> +               r0 = val;
> +               r1 = val >> 32;
> +
> +               ret = eqc_pll_parse_registers(r0, r1, &mult, &div, &acc);
> +               if (ret) {
> +                       dev_warn(dev, "failed parsing state of %s\n", pll->name);
> +                       priv->cells->hws[pll->index] = ERR_PTR(ret);
> +                       continue;
> +               }
> +
> +               hw = clk_hw_register_fixed_factor_with_accuracy_fwname(dev,
> +                               dev->of_node, pll->name, "ref", 0, mult, div, acc);
> +               priv->cells->hws[pll->index] = hw;
> +               if (IS_ERR(hw))
> +                       dev_warn(dev, "failed registering %s: %pe\n", pll->name, hw);
> +       }
> +
> +       BUG_ON(ARRAY_SIZE(div_resources) < data->div_count);

Can this be a static assert instead on the arrays these are based on?
Put some static_assert() near the match data macros.

> +
> +       for (i = 0; i < data->div_count; i++) {
> +               const struct eqc_div *div = &data->divs[i];
> +               void __iomem *base = NULL;
> +               struct clk_hw *parent;
> +               unsigned int j;
> +
> +               /*
> +                * Multiple divider clocks can request the same resource. Store
> +                * resource pointers during probe(). For each divider clock,
> +                * check if previous clocks referenced the same resource name.
> +                *
> +                * See EQ6HC_SOUTH_DIV_OSPI_REF and EQ6HC_SOUTH_DIV_OSPI_SYS.
> +                */
> +               for (j = 0; j < i; j++) {
> +                       if (strcmp(data->divs[j].resource_name, div->resource_name) == 0) {
> +                               base = div_resources[j];
> +                               break;
> +                       }
> +               }
> +
> +               /* Resource is first encountered. */
> +               if (!base) {
> +                       base = devm_platform_ioremap_resource_byname(pdev, div->resource_name);
> +                       if (IS_ERR(base)) {
> +                               dev_warn(dev, "failed to iomap resource for %s\n", div->name);
> +                               priv->cells->hws[div->index] = base;
> +                               continue;
> +                       }
> +               }

I don't get this code at all. The driver should simply map the
resources because it knows that there's an io resource. I'll look at the
binding which is probably wrong and causing the driver to be written
this way.

> +
> +               div_resources[i] = base;
> +
> +               parent = priv->cells->hws[div->parent];
> +               hw = clk_hw_register_divider_table_parent_hw(dev, div->name,
> +                               parent, 0, base, div->shift, div->width,
> +                               CLK_DIVIDER_EVEN_INTEGERS, NULL, NULL);
> +               priv->cells->hws[div->index] = hw;
> +               if (IS_ERR(hw))
> +                       dev_warn(dev, "failed registering %s: %pe\n",
> +                                div->name, hw);
> +       }
> +
> +       /* Clock provider has not been registered by eqc_init(). Do it now. */
> +       if (data->early_pll_count == 0) {
> +               /* When providing a single clock, require no cell. */
> +               if (priv->cells->num == 1)
> +                       return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
> +                                                          priv->cells->hws);
> +               else
> +                       return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +                                                          priv->cells);
> +       }
> +
> +       return 0;
> +}
> +
> +/* Required early for GIC timer (pll-cpu) and UARTs (pll-per). */
> +static const struct eqc_pll eqc_eyeq5_early_plls[] = {
> +       { .index = EQ5C_PLL_CPU, .name = "pll-cpu",  .reg64 = 0x00, },
> +       { .index = EQ5C_PLL_PER, .name = "pll-per",  .reg64 = 0x30, },
> +};
> +
> +static const struct eqc_pll eqc_eyeq5_plls[] = {
> +       { .index = EQ5C_PLL_VMP,  .name = "pll-vmp",  .reg64 = 0x08, },
> +       { .index = EQ5C_PLL_PMA,  .name = "pll-pma",  .reg64 = 0x10, },
> +       { .index = EQ5C_PLL_VDI,  .name = "pll-vdi",  .reg64 = 0x18, },
> +       { .index = EQ5C_PLL_DDR0, .name = "pll-ddr0", .reg64 = 0x20, },
> +       { .index = EQ5C_PLL_PCI,  .name = "pll-pci",  .reg64 = 0x28, },
> +       { .index = EQ5C_PLL_PMAC, .name = "pll-pmac", .reg64 = 0x38, },
> +       { .index = EQ5C_PLL_MPC,  .name = "pll-mpc",  .reg64 = 0x40, },
> +       { .index = EQ5C_PLL_DDR1, .name = "pll-ddr1", .reg64 = 0x48, },
> +};
> +
> +static const struct eqc_div eqc_eyeq5_divs[] = {
> +       {
> +               .index = EQ5C_DIV_OSPI,
> +               .name = "div-ospi",
> +               .parent = EQ5C_PLL_PER,
> +               .resource_name = "ospi",
> +               .shift = 0,
> +               .width = 4,
> +       },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq5_match_data = {
> +       .early_pll_count        = ARRAY_SIZE(eqc_eyeq5_early_plls),
> +       .early_plls             = eqc_eyeq5_early_plls,
> +
> +       .pll_count      = ARRAY_SIZE(eqc_eyeq5_plls),
> +       .plls           = eqc_eyeq5_plls,
> +
> +       .div_count      = ARRAY_SIZE(eqc_eyeq5_divs),
> +       .divs           = eqc_eyeq5_divs,
> +};
> +
> +static const struct eqc_pll eqc_eyeq6l_plls[] = {
> +       { .index = EQ6LC_PLL_DDR, .name = "pll-ddr", .reg64 = 0x2C },
> +       { .index = EQ6LC_PLL_CPU, .name = "pll-cpu", .reg64 = 0x34 }, /* also acc */
> +       { .index = EQ6LC_PLL_PER, .name = "pll-per", .reg64 = 0x3C },
> +       { .index = EQ6LC_PLL_VDI, .name = "pll-vdi", .reg64 = 0x44 },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6l_match_data = {
> +       .pll_count      = ARRAY_SIZE(eqc_eyeq6l_plls),
> +       .plls           = eqc_eyeq6l_plls,
> +};
> +
> +/* Required early for GIC timer. */
> +static const struct eqc_pll eqc_eyeq6h_central_early_plls[] = {
> +       { .index = 0, .name = "pll-cpu", .reg64 = 0x00 },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6h_central_match_data = {
> +       .early_pll_count        = ARRAY_SIZE(eqc_eyeq6h_central_early_plls),
> +       .early_plls             = eqc_eyeq6h_central_early_plls,
> +};
> +
> +/* Required early for UART. */
> +static const struct eqc_pll eqc_eyeq6h_west_early_plls[] = {
> +       { .index = 0, .name = "pll-west", .reg64 = 0x00 },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6h_west_match_data = {
> +       .early_pll_count        = ARRAY_SIZE(eqc_eyeq6h_west_early_plls),
> +       .early_plls             = eqc_eyeq6h_west_early_plls,
> +};
> +
> +static const struct eqc_pll eqc_eyeq6h_east_plls[] = {
> +       { .index = 0, .name = "pll-east", .reg64 = 0x00 },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6h_east_match_data = {
> +       .pll_count      = ARRAY_SIZE(eqc_eyeq6h_east_plls),
> +       .plls           = eqc_eyeq6h_east_plls,
> +};
> +
> +static const struct eqc_pll eqc_eyeq6h_south_plls[] = {
> +       { .index = EQ6HC_SOUTH_PLL_VDI,  .name = "pll-vdi",  .reg64 = 0x00 },
> +       { .index = EQ6HC_SOUTH_PLL_PCIE, .name = "pll-pcie", .reg64 = 0x08 },
> +       { .index = EQ6HC_SOUTH_PLL_PER,  .name = "pll-per",  .reg64 = 0x10 },
> +       { .index = EQ6HC_SOUTH_PLL_ISP,  .name = "pll-isp",  .reg64 = 0x18 },
> +};
> +
> +static const struct eqc_div eqc_eyeq6h_south_divs[] = {
> +       {
> +               .index = EQ6HC_SOUTH_DIV_EMMC,
> +               .name = "div-emmc",
> +               .parent = EQ6HC_SOUTH_PLL_PER,
> +               .resource_name = "emmc",
> +               .shift = 4,
> +               .width = 4,
> +       },
> +       {
> +               .index = EQ6HC_SOUTH_DIV_OSPI_REF,
> +               .name = "div-ospi-ref",
> +               .parent = EQ6HC_SOUTH_PLL_PER,
> +               .resource_name = "ospi",
> +               .shift = 4,
> +               .width = 4,
> +       },
> +       {
> +               .index = EQ6HC_SOUTH_DIV_OSPI_SYS,
> +               .name = "div-ospi-sys",
> +               .parent = EQ6HC_SOUTH_PLL_PER,
> +               .resource_name = "ospi",
> +               .shift = 8,
> +               .width = 1,
> +       },
> +       {
> +               .index = EQ6HC_SOUTH_DIV_TSU,
> +               .name = "div-tsu",
> +               .parent = EQ6HC_SOUTH_PLL_PCIE,
> +               .resource_name = "tsu",
> +               .shift = 4,
> +               .width = 8,
> +       },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6h_south_match_data = {
> +       .pll_count      = ARRAY_SIZE(eqc_eyeq6h_south_plls),
> +       .plls           = eqc_eyeq6h_south_plls,
> +
> +       .div_count      = ARRAY_SIZE(eqc_eyeq6h_south_divs),
> +       .divs           = eqc_eyeq6h_south_divs,
> +};
> +
> +static const struct eqc_pll eqc_eyeq6h_ddr0_plls[] = {
> +       { .index = 0, .name = "pll-ddr0", .reg64 = 0x00 },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6h_ddr0_match_data = {
> +       .pll_count      = ARRAY_SIZE(eqc_eyeq6h_ddr0_plls),
> +       .plls           = eqc_eyeq6h_ddr0_plls,
> +};
> +
> +static const struct eqc_pll eqc_eyeq6h_ddr1_plls[] = {
> +       { .index = 0, .name = "pll-ddr1", .reg64 = 0x00 },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6h_ddr1_match_data = {
> +       .pll_count      = ARRAY_SIZE(eqc_eyeq6h_ddr1_plls),
> +       .plls           = eqc_eyeq6h_ddr1_plls,
> +};
> +
> +static const struct eqc_pll eqc_eyeq6h_acc_plls[] = {
> +       { .index = EQ6HC_ACC_PLL_XNN, .name = "pll-xnn", .reg64 = 0x00 },
> +       { .index = EQ6HC_ACC_PLL_VMP, .name = "pll-vmp", .reg64 = 0x10 },
> +       { .index = EQ6HC_ACC_PLL_PMA, .name = "pll-pma", .reg64 = 0x1C },
> +       { .index = EQ6HC_ACC_PLL_MPC, .name = "pll-mpc", .reg64 = 0x28 },
> +       { .index = EQ6HC_ACC_PLL_NOC, .name = "pll-noc", .reg64 = 0x30 },
> +};
> +
> +static const struct eqc_match_data eqc_eyeq6h_acc_match_data = {
> +       .pll_count      = ARRAY_SIZE(eqc_eyeq6h_acc_plls),
> +       .plls           = eqc_eyeq6h_acc_plls,
> +};
> +
> +static const struct of_device_id eqc_match_table[] = {
> +       { .compatible = "mobileye,eyeq5-clk", .data = &eqc_eyeq5_match_data },
> +       { .compatible = "mobileye,eyeq6l-clk", .data = &eqc_eyeq6l_match_data },
> +       { .compatible = "mobileye,eyeq6h-central-clk", .data = &eqc_eyeq6h_central_match_data },
> +       { .compatible = "mobileye,eyeq6h-west-clk", .data = &eqc_eyeq6h_west_match_data },
> +       { .compatible = "mobileye,eyeq6h-east-clk", .data = &eqc_eyeq6h_east_match_data },
> +       { .compatible = "mobileye,eyeq6h-south-clk", .data = &eqc_eyeq6h_south_match_data },
> +       { .compatible = "mobileye,eyeq6h-ddr0-clk", .data = &eqc_eyeq6h_ddr0_match_data },
> +       { .compatible = "mobileye,eyeq6h-ddr1-clk", .data = &eqc_eyeq6h_ddr1_match_data },
> +       { .compatible = "mobileye,eyeq6h-acc-clk", .data = &eqc_eyeq6h_acc_match_data },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, eqc_match_table);
> +
> +static struct platform_driver eqc_driver = {
> +       .probe = eqc_probe,
> +       .driver = {
> +               .name = "clk-eyeq",
> +               .of_match_table = eqc_match_table,
> +       },
> +};
> +builtin_platform_driver(eqc_driver);
> +
> +static void __init eqc_init(struct device_node *np)
> +{
> +       const struct eqc_match_data *data;
> +       unsigned int nb_clks = 0;
> +       struct eqc_priv *priv;
> +       unsigned int i;
> +       int ret;
> +
> +       data = of_match_node(eqc_match_table, np)->data;
> +
> +       /* No reason to early init this clock provider. Do it at probe. */
> +       if (data->early_pll_count == 0)

You can have a different match table for this function then.

> +               return;
> +
> +       priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +       if (!priv) {
> +               ret = -ENOMEM;
> +               goto err;
> +       }
> +
> +       priv->np = np;
> +       priv->data = data;
> +
> +       nb_clks = eqc_compute_clock_count(data);
> +       priv->cells = kzalloc(struct_size(priv->cells, hws, nb_clks), GFP_KERNEL);
> +       if (!priv->cells) {
> +               ret = -ENOMEM;
> +               goto err;
> +       }
> +
> +       priv->cells->num = nb_clks;
> +
> +       /*
> +        * Mark non-early clocks as deferred; they'll be registered at platform
> +        * device probe.
> +        */
> +       for (i = 0; i < data->pll_count; i++)
> +               priv->cells->hws[data->plls[i].index] = ERR_PTR(-EPROBE_DEFER);
> +       for (i = 0; i < data->div_count; i++)
> +               priv->cells->hws[data->divs[i].index] = ERR_PTR(-EPROBE_DEFER);
> +
> +       /*
> +        * We expect named resources if divider clocks are present.
> +        * Else, we only expect one resource.
> +        */

Please avoid named resources. They give the false sense of hope that the
binding can re-order the reg property when that can't be done. Instead,
just index and know which index to use in the driver.

Hello,

On Thu Apr 11, 2024 at 5:22 AM CEST, Stephen Boyd wrote:
> Quoting Théo Lebrun (2024-04-10 10:12:34)
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index 50af5fc7f570..1eb6e70977a3 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_EYEQ
> > +       bool "Clock driver for the Mobileye EyeQ platform"
> > +       depends on OF || COMPILE_TEST
>
> The OF build dependency looks useless as we have the MACH_ dependency
> below.

Indeed. I thought explicit dependency could be useful. Will remove.

> > +       depends on MACH_EYEQ5 || MACH_EYEQ6H || COMPILE_TEST
> > +       default MACH_EYEQ5 || MACH_EYEQ6H
> > +       help
> > +         This driver provides clocks found on Mobileye EyeQ5, EyeQ6L and Eye6H
> > +         SoCs. Controllers live in shared register regions called OLB. Driver
> > +         provides read-only PLLs, derived from the main crystal clock (which
> > +         must be constant). It also exposes some divider clocks.
> > +
> >  config COMMON_CLK_FSL_FLEXSPI
> >         tristate "Clock driver for FlexSPI on Layerscape SoCs"
> >         depends on ARCH_LAYERSCAPE || COMPILE_TEST
> > diff --git a/drivers/clk/clk-eyeq.c b/drivers/clk/clk-eyeq.c
> > new file mode 100644
> > index 000000000000..bb2535010ae6
> > --- /dev/null
> > +++ b/drivers/clk/clk-eyeq.c
> > @@ -0,0 +1,644 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * PLL clock driver for the Mobileye EyeQ5, EyeQ6L and EyeQ6H platforms.
> > + *
> > + * This controller handles read-only PLLs, all derived from the same main
> > + * crystal clock. It also exposes divider clocks, those are children to PLLs.
> > + * Parent clock is expected to be constant. This driver's registers live in
> > + * a shared region called OLB. Some PLLs are initialised early by of_clk_init().
>
> Is OLB a different DT node? It sounds like maybe this is trying to jam a
> driver into DT when the OLB node should be a #clock-cells node.

Yes OLB is a different DT node. It looks like on EyeQ5:

	olb: system-controller@e00000 {
		compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
		reg = <0 0xe00000 0x0 0x400>;
		ranges = <0x0 0x0 0xe00000 0x400>;
		#address-cells = <1>;
		#size-cells = <1>;

		reset: reset-controller@e00000 {
			compatible = "mobileye,eyeq5-reset";
			reg = <0x000 0x0c>, <0x200 0x34>, <0x120 0x04>;
			reg-names = "d0", "d1", "d2";
			#reset-cells = <2>;
		};

		clocks: clock-controller@e0002c {
			compatible = "mobileye,eyeq5-clk";
			reg = <0x02c 0x50>, <0x11c 0x04>;
			reg-names = "plls", "ospi";
			#clock-cells = <1>;
			clocks = <&xtal>;
			clock-names = "ref";
		};

		pinctrl: pinctrl@e000b0 {
			compatible = "mobileye,eyeq5-pinctrl";
			reg = <0x0b0 0x30>;
		};
	};

Keep in mind OLB is a complex beast. On EyeQ5, it hosts something like
150 registers, describing 20ish various hardware features. We have to
expose registers to drivers for one-off reads/writes. One example found
upstream: I2C speed mode register. Others will be Ethernet, eMMC DMA
config, etc. A syscon makes sense.

I2C looks like like this for example, look at mobileye,olb.

	i2c@300000 {
		compatible = "mobileye,eyeq5-i2c", "arm,primecell";
		reg = <0x300000 0x1000>;
		interrupt-parent = <&gic>;
		interrupts = <GIC_SHARED 1 IRQ_TYPE_LEVEL_HIGH>;
		clock-frequency = <400000>;
		#address-cells = <1>;
		#size-cells = <0>;
		clocks = <&i2c_ser_clk>, <&i2c_clk>;
		clock-names = "i2cclk", "apb_pclk";
		mobileye,olb = <&olb 0>;
	};

See commits 7d4c57abb928 and 1b9a8e8af0d9:
  i2c: nomadik: support Mobileye EyeQ5 I2C controller
  dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

> > +
> > +static int eqc_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       void __iomem *div_resources[EQC_MAX_DIV_COUNT];
> > +       struct device_node *np = dev->of_node;
> > +       const struct eqc_match_data *data;
> > +       struct eqc_priv *priv = NULL;
> > +       struct clk_hw *hw;
> > +       unsigned int i;
> > +
> > +       data = device_get_match_data(dev);
> > +       if (!data)
> > +               return -ENODEV;
> > +
> > +       if (data->early_pll_count) {
> > +               /* Device got inited early. Retrieve clock provider from list. */
> > +               struct eqc_priv *entry;
> > +
> > +               spin_lock(&eqc_list_slock);
> > +               list_for_each_entry(entry, &eqc_list, list) {
> > +                       if (entry->np == np) {
> > +                               priv = entry;
> > +                               break;
> > +                       }
> > +               }
> > +               spin_unlock(&eqc_list_slock);
> > +
> > +               if (!priv)
> > +                       return -ENODEV;
>
> This can be a sub-function.

Will do.

[...]

> > +       for (i = 0; i < data->pll_count; i++) {
> > +               const struct eqc_pll *pll = &data->plls[i];
> > +               unsigned long mult, div, acc;
> > +               u32 r0, r1;
> > +               u64 val;
> > +               int ret;
>
> All variables should be declared at the start of the function. Once it
> becomes "too heavy" you can split it up into smaller functions, that
> again have all variables declared at the start of the function.

Will avoid variables declarations at start of loops.

> > +
> > +               val = readq(priv->base_plls + pll->reg64);
> > +               r0 = val;
> > +               r1 = val >> 32;
> > +
> > +               ret = eqc_pll_parse_registers(r0, r1, &mult, &div, &acc);
> > +               if (ret) {
> > +                       dev_warn(dev, "failed parsing state of %s\n", pll->name);
> > +                       priv->cells->hws[pll->index] = ERR_PTR(ret);
> > +                       continue;
> > +               }
> > +
> > +               hw = clk_hw_register_fixed_factor_with_accuracy_fwname(dev,
> > +                               dev->of_node, pll->name, "ref", 0, mult, div, acc);
> > +               priv->cells->hws[pll->index] = hw;
> > +               if (IS_ERR(hw))
> > +                       dev_warn(dev, "failed registering %s: %pe\n", pll->name, hw);
> > +       }
> > +
> > +       BUG_ON(ARRAY_SIZE(div_resources) < data->div_count);
>
> Can this be a static assert instead on the arrays these are based on?
> Put some static_assert() near the match data macros.

I hesitated before sending. Will update.

> > +
> > +       for (i = 0; i < data->div_count; i++) {
> > +               const struct eqc_div *div = &data->divs[i];
> > +               void __iomem *base = NULL;
> > +               struct clk_hw *parent;
> > +               unsigned int j;
> > +
> > +               /*
> > +                * Multiple divider clocks can request the same resource. Store
> > +                * resource pointers during probe(). For each divider clock,
> > +                * check if previous clocks referenced the same resource name.
> > +                *
> > +                * See EQ6HC_SOUTH_DIV_OSPI_REF and EQ6HC_SOUTH_DIV_OSPI_SYS.
> > +                */
> > +               for (j = 0; j < i; j++) {
> > +                       if (strcmp(data->divs[j].resource_name, div->resource_name) == 0) {
> > +                               base = div_resources[j];
> > +                               break;
> > +                       }
> > +               }
> > +
> > +               /* Resource is first encountered. */
> > +               if (!base) {
> > +                       base = devm_platform_ioremap_resource_byname(pdev, div->resource_name);
> > +                       if (IS_ERR(base)) {
> > +                               dev_warn(dev, "failed to iomap resource for %s\n", div->name);
> > +                               priv->cells->hws[div->index] = base;
> > +                               continue;
> > +                       }
> > +               }
>
> I don't get this code at all. The driver should simply map the
> resources because it knows that there's an io resource. I'll look at the
> binding which is probably wrong and causing the driver to be written
> this way.

This is here for a single reason: EyeQ6H south OLB has two clocks that
live in the same register:

 - div-ospi-ref, reg offset 0x90, mask GENMASK(9,  8) == 0x300.
 - div-ospi-sys, reg offset 0x90, mask GENMASK(12, 4) == 0x1FF0.

Calling twice devm_platform_ioremap_resource_byname() with the same
resource name gives an error. So we need to buffer resources already
requested.

If there is a simpler & better solution I'd be happy to take it.

[...]

> > +static void __init eqc_init(struct device_node *np)
> > +{
> > +       const struct eqc_match_data *data;
> > +       unsigned int nb_clks = 0;
> > +       struct eqc_priv *priv;
> > +       unsigned int i;
> > +       int ret;
> > +
> > +       data = of_match_node(eqc_match_table, np)->data;
> > +
> > +       /* No reason to early init this clock provider. Do it at probe. */
> > +       if (data->early_pll_count == 0)
>
> You can have a different match table for this function then.

Ah, clever. Will do.

[...]

> > +       /*
> > +        * We expect named resources if divider clocks are present.
> > +        * Else, we only expect one resource.
> > +        */
>
> Please avoid named resources. They give the false sense of hope that the
> binding can re-order the reg property when that can't be done. Instead,
> just index and know which index to use in the driver.

It is unclear what you mean by not being able to re-order reg property?
Are you talking about reg-names being most often defined as items const
list and therefore cannot be reordered? Here binding declare things
using minItems/maxItems/enum so it can be reordered, looking like:

  properties:
    reg:
      minItems: 2
      maxItems: 2
    reg-names:
      minItems: 2
      maxItems: 2
      items:
        enum: [ plls, ospi ]

If this is not what you are talking about then I rambled about garbage
and I'll use indexed resources.

Thanks,

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

Quoting Théo Lebrun (2024-04-11 03:46:04)
> Hello,
> 
> On Thu Apr 11, 2024 at 5:22 AM CEST, Stephen Boyd wrote:
> > Quoting Théo Lebrun (2024-04-10 10:12:34)
> > > diff --git a/drivers/clk/clk-eyeq.c b/drivers/clk/clk-eyeq.c
> > > new file mode 100644
> > > index 000000000000..bb2535010ae6
> > > --- /dev/null
> > > +++ b/drivers/clk/clk-eyeq.c
> > > @@ -0,0 +1,644 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * PLL clock driver for the Mobileye EyeQ5, EyeQ6L and EyeQ6H platforms.
> > > + *
> > > + * This controller handles read-only PLLs, all derived from the same main
> > > + * crystal clock. It also exposes divider clocks, those are children to PLLs.
> > > + * Parent clock is expected to be constant. This driver's registers live in
> > > + * a shared region called OLB. Some PLLs are initialised early by of_clk_init().
> >
> > Is OLB a different DT node? It sounds like maybe this is trying to jam a
> > driver into DT when the OLB node should be a #clock-cells node.
> 
> Yes OLB is a different DT node. It looks like on EyeQ5:
> 
>         olb: system-controller@e00000 {
>                 compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
>                 reg = <0 0xe00000 0x0 0x400>;
>                 ranges = <0x0 0x0 0xe00000 0x400>;
>                 #address-cells = <1>;
>                 #size-cells = <1>;
> 
>                 reset: reset-controller@e00000 {
>                         compatible = "mobileye,eyeq5-reset";
>                         reg = <0x000 0x0c>, <0x200 0x34>, <0x120 0x04>;
>                         reg-names = "d0", "d1", "d2";
>                         #reset-cells = <2>;
>                 };
> 
>                 clocks: clock-controller@e0002c {
>                         compatible = "mobileye,eyeq5-clk";
>                         reg = <0x02c 0x50>, <0x11c 0x04>;

Is this reg property always the same value '0x2c'?

>                         reg-names = "plls", "ospi";
>                         #clock-cells = <1>;
>                         clocks = <&xtal>;
>                         clock-names = "ref";
>                 };
> 
>                 pinctrl: pinctrl@e000b0 {
>                         compatible = "mobileye,eyeq5-pinctrl";
>                         reg = <0x0b0 0x30>;
>                 };
>         };


> 
> Keep in mind OLB is a complex beast. On EyeQ5, it hosts something like
> 150 registers, describing 20ish various hardware features. We have to
> expose registers to drivers for one-off reads/writes. One example found
> upstream: I2C speed mode register. Others will be Ethernet, eMMC DMA
> config, etc. A syscon makes sense.

Syscons are a slippery slope. It makes it easy to give up abstracting
the 20ish hardware features and makes the resulting drivers which use
the syscon highly platform specific.

Regardless of having a syscon or not, the binding should collapse the
sub-nodes into the olb node. If that requires making a different
compatible for different olb nodes, then that's actually better because
there may be some quirk for one of the olbs and not the other and we
won't be able to fix that without a compatible string update. It would
also make the reg-names property go away, because the sub-functionality
drivers would have the register offsets hard-coded as some offset from
the base of olb, instead of encoding that in DT.

> 
> I2C looks like like this for example, look at mobileye,olb.
> 
>         i2c@300000 {
>                 compatible = "mobileye,eyeq5-i2c", "arm,primecell";
>                 reg = <0x300000 0x1000>;
>                 interrupt-parent = <&gic>;
>                 interrupts = <GIC_SHARED 1 IRQ_TYPE_LEVEL_HIGH>;
>                 clock-frequency = <400000>;
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>                 clocks = <&i2c_ser_clk>, <&i2c_clk>;
>                 clock-names = "i2cclk", "apb_pclk";
>                 mobileye,olb = <&olb 0>;
>         };
> 
> See commits 7d4c57abb928 and 1b9a8e8af0d9:
>   i2c: nomadik: support Mobileye EyeQ5 I2C controller
>   dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

Why isn't i2c speed mode another clk exposed by OLB that rounds to the
different rates?

> 
> > > +
> > > +       for (i = 0; i < data->div_count; i++) {
> > > +               const struct eqc_div *div = &data->divs[i];
> > > +               void __iomem *base = NULL;
> > > +               struct clk_hw *parent;
> > > +               unsigned int j;
> > > +
> > > +               /*
> > > +                * Multiple divider clocks can request the same resource. Store
> > > +                * resource pointers during probe(). For each divider clock,
> > > +                * check if previous clocks referenced the same resource name.
> > > +                *
> > > +                * See EQ6HC_SOUTH_DIV_OSPI_REF and EQ6HC_SOUTH_DIV_OSPI_SYS.
> > > +                */
> > > +               for (j = 0; j < i; j++) {
> > > +                       if (strcmp(data->divs[j].resource_name, div->resource_name) == 0) {
> > > +                               base = div_resources[j];
> > > +                               break;
> > > +                       }
> > > +               }
> > > +
> > > +               /* Resource is first encountered. */
> > > +               if (!base) {
> > > +                       base = devm_platform_ioremap_resource_byname(pdev, div->resource_name);
> > > +                       if (IS_ERR(base)) {
> > > +                               dev_warn(dev, "failed to iomap resource for %s\n", div->name);
> > > +                               priv->cells->hws[div->index] = base;
> > > +                               continue;
> > > +                       }
> > > +               }
> >
> > I don't get this code at all. The driver should simply map the
> > resources because it knows that there's an io resource. I'll look at the
> > binding which is probably wrong and causing the driver to be written
> > this way.
> 
> This is here for a single reason: EyeQ6H south OLB has two clocks that
> live in the same register:
> 
>  - div-ospi-ref, reg offset 0x90, mask GENMASK(9,  8) == 0x300.
>  - div-ospi-sys, reg offset 0x90, mask GENMASK(12, 4) == 0x1FF0.
> 
> Calling twice devm_platform_ioremap_resource_byname() with the same
> resource name gives an error. So we need to buffer resources already
> requested.
> 
> If there is a simpler & better solution I'd be happy to take it.

Sure, don't call platform_ioremap_resource() and friends more than once
per index. But why is the code written in a way that that is happening?
Maybe the driver can ioremap resources, and then register clks for those
resources. I suspect the only way of getting here is that the driver is
focused on registering clks, and ioremapping resources while registering
clks. Don't do that, because then you have to write code to track
resources.

> 
> 
> [...]
> 
> > > +       /*
> > > +        * We expect named resources if divider clocks are present.
> > > +        * Else, we only expect one resource.
> > > +        */
> >
> > Please avoid named resources. They give the false sense of hope that the
> > binding can re-order the reg property when that can't be done. Instead,
> > just index and know which index to use in the driver.
> 
> It is unclear what you mean by not being able to re-order reg property?
> Are you talking about reg-names being most often defined as items const
> list and therefore cannot be reordered? Here binding declare things
> using minItems/maxItems/enum so it can be reordered, looking like:

Yes, that's wrong.

> 
>   properties:
>     reg:
>       minItems: 2
>       maxItems: 2
>     reg-names:
>       minItems: 2
>       maxItems: 2
>       items:
>         enum: [ plls, ospi ]
> 
> If this is not what you are talking about then I rambled about garbage
> and I'll use indexed resources.
> 

You cannot reorder strings in a DT binding property after the fact.
While the code will keep working if the reg-names elements are
re-ordered, the binding will be backwards incompatible, because the
reg-names property must have the same order. It can be convenient to use
reg-names if you have a long list of reg properties to map, but having
two or one elements isn't a very strong argument.

Hello,

On Fri Apr 12, 2024 at 7:46 AM CEST, Stephen Boyd wrote:
> Quoting Théo Lebrun (2024-04-11 03:46:04)
> > On Thu Apr 11, 2024 at 5:22 AM CEST, Stephen Boyd wrote:
> > > Quoting Théo Lebrun (2024-04-10 10:12:34)
> > > > diff --git a/drivers/clk/clk-eyeq.c b/drivers/clk/clk-eyeq.c
> > > > new file mode 100644
> > > > index 000000000000..bb2535010ae6
> > > > --- /dev/null
> > > > +++ b/drivers/clk/clk-eyeq.c
> > > > @@ -0,0 +1,644 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * PLL clock driver for the Mobileye EyeQ5, EyeQ6L and EyeQ6H platforms.
> > > > + *
> > > > + * This controller handles read-only PLLs, all derived from the same main
> > > > + * crystal clock. It also exposes divider clocks, those are children to PLLs.
> > > > + * Parent clock is expected to be constant. This driver's registers live in
> > > > + * a shared region called OLB. Some PLLs are initialised early by of_clk_init().
> > >
> > > Is OLB a different DT node? It sounds like maybe this is trying to jam a
> > > driver into DT when the OLB node should be a #clock-cells node.
> > 
> > Yes OLB is a different DT node. It looks like on EyeQ5:
> > 
> >         olb: system-controller@e00000 {
> >                 compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
> >                 reg = <0 0xe00000 0x0 0x400>;
> >                 ranges = <0x0 0x0 0xe00000 0x400>;
> >                 #address-cells = <1>;
> >                 #size-cells = <1>;
> > 
> >                 reset: reset-controller@e00000 {
> >                         compatible = "mobileye,eyeq5-reset";
> >                         reg = <0x000 0x0c>, <0x200 0x34>, <0x120 0x04>;
> >                         reg-names = "d0", "d1", "d2";
> >                         #reset-cells = <2>;
> >                 };
> > 
> >                 clocks: clock-controller@e0002c {
> >                         compatible = "mobileye,eyeq5-clk";
> >                         reg = <0x02c 0x50>, <0x11c 0x04>;
>
> Is this reg property always the same value '0x2c'?

On EyeQ5 yes.
On EyeQ6L and EyeQ6H (next revisions, different compatible), no.

> >                         reg-names = "plls", "ospi";
> >                         #clock-cells = <1>;
> >                         clocks = <&xtal>;
> >                         clock-names = "ref";
> >                 };
> > 
> >                 pinctrl: pinctrl@e000b0 {
> >                         compatible = "mobileye,eyeq5-pinctrl";
> >                         reg = <0x0b0 0x30>;
> >                 };
> >         };
>
>
> > 
> > Keep in mind OLB is a complex beast. On EyeQ5, it hosts something like
> > 150 registers, describing 20ish various hardware features. We have to
> > expose registers to drivers for one-off reads/writes. One example found
> > upstream: I2C speed mode register. Others will be Ethernet, eMMC DMA
> > config, etc. A syscon makes sense.
>
> Syscons are a slippery slope. It makes it easy to give up abstracting
> the 20ish hardware features and makes the resulting drivers which use
> the syscon highly platform specific.

I see where you are coming from. In my mind syscons make sense here
because most features in the block are only small additions/tweaks to
existing blocks. A few examples:

 - eMMC DMA integration config
 - eMMC high impedance register
 - OSPI high impedance register

Those cannot be abstracted. The main node is elsewhere and it needs
access to those registers.

Some other registers however will be able to be abstracted away, as
custom clocks for example (eg I'm seeing SGMII PLL control register).
That is not the case for all however.

> Regardless of having a syscon or not, the binding should collapse the
> sub-nodes into the olb node. If that requires making a different
> compatible for different olb nodes, then that's actually better because
> there may be some quirk for one of the olbs and not the other and we
> won't be able to fix that without a compatible string update. It would
> also make the reg-names property go away, because the sub-functionality
> drivers would have the register offsets hard-coded as some offset from
> the base of olb, instead of encoding that in DT.

Do you have examples of existing nodes that both are syscons and expose
multiple resources (clocks, resets, etc)? I did not envision this as
possible which is why I didn't go this route. Also, this would require
big changes to dt-bindings currently in v6.9-rc.

To be honest, I do not comprehend what aggregating sub-nodes into a
single one brings. Here we have one node per feature, each exposing
their small feature set, with a single driver handling each. Devicetree
is longer but more straight forward, with small resource provider nodes
versus a big behemoth handling all of it. Simple and easy: my brain
has an easy time grasping it all.

What you are proposing sounds more complex, for no clear benefit to my
eyes. Can you expand on what that brings? I guess answer has been given
elsewhere in depth, so answer might just be a link to the right
resource. This is an open question, not reserved to Stephen. Thanks!

> > I2C looks like like this for example, look at mobileye,olb.
> > 
> >         i2c@300000 {
> >                 compatible = "mobileye,eyeq5-i2c", "arm,primecell";
> >                 reg = <0x300000 0x1000>;
> >                 interrupt-parent = <&gic>;
> >                 interrupts = <GIC_SHARED 1 IRQ_TYPE_LEVEL_HIGH>;
> >                 clock-frequency = <400000>;
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> >                 clocks = <&i2c_ser_clk>, <&i2c_clk>;
> >                 clock-names = "i2cclk", "apb_pclk";
> >                 mobileye,olb = <&olb 0>;
> >         };
> > 
> > See commits 7d4c57abb928 and 1b9a8e8af0d9:
> >   i2c: nomadik: support Mobileye EyeQ5 I2C controller
> >   dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example
>
> Why isn't i2c speed mode another clk exposed by OLB that rounds to the
> different rates?

It would be a new clock referenced from DT on which we would only do
clk_set_rate(priv->clk_freq). How would it fit in the clock tree? It
wouldn't have any parent, even though in practice is it parented to an
internal Nomadik I2C clock that is not being exposed.

I thought it would be an issue to have a clock modeled as having no
parent (because its true parent is not exposed) which is a blatant lie.

> > > > +       for (i = 0; i < data->div_count; i++) {
> > > > +               const struct eqc_div *div = &data->divs[i];
> > > > +               void __iomem *base = NULL;
> > > > +               struct clk_hw *parent;
> > > > +               unsigned int j;
> > > > +
> > > > +               /*
> > > > +                * Multiple divider clocks can request the same resource. Store
> > > > +                * resource pointers during probe(). For each divider clock,
> > > > +                * check if previous clocks referenced the same resource name.
> > > > +                *
> > > > +                * See EQ6HC_SOUTH_DIV_OSPI_REF and EQ6HC_SOUTH_DIV_OSPI_SYS.
> > > > +                */
> > > > +               for (j = 0; j < i; j++) {
> > > > +                       if (strcmp(data->divs[j].resource_name, div->resource_name) == 0) {
> > > > +                               base = div_resources[j];
> > > > +                               break;
> > > > +                       }
> > > > +               }
> > > > +
> > > > +               /* Resource is first encountered. */
> > > > +               if (!base) {
> > > > +                       base = devm_platform_ioremap_resource_byname(pdev, div->resource_name);
> > > > +                       if (IS_ERR(base)) {
> > > > +                               dev_warn(dev, "failed to iomap resource for %s\n", div->name);
> > > > +                               priv->cells->hws[div->index] = base;
> > > > +                               continue;
> > > > +                       }
> > > > +               }
> > >
> > > I don't get this code at all. The driver should simply map the
> > > resources because it knows that there's an io resource. I'll look at the
> > > binding which is probably wrong and causing the driver to be written
> > > this way.
> > 
> > This is here for a single reason: EyeQ6H south OLB has two clocks that
> > live in the same register:
> > 
> >  - div-ospi-ref, reg offset 0x90, mask GENMASK(9,  8) == 0x300.
> >  - div-ospi-sys, reg offset 0x90, mask GENMASK(12, 4) == 0x1FF0.
> > 
> > Calling twice devm_platform_ioremap_resource_byname() with the same
> > resource name gives an error. So we need to buffer resources already
> > requested.
> > 
> > If there is a simpler & better solution I'd be happy to take it.
>
> Sure, don't call platform_ioremap_resource() and friends more than once
> per index. But why is the code written in a way that that is happening?
> Maybe the driver can ioremap resources, and then register clks for those
> resources. I suspect the only way of getting here is that the driver is
> focused on registering clks, and ioremapping resources while registering
> clks. Don't do that, because then you have to write code to track
> resources.

So code would look like (removing reg-names as it brings nothing, and
error checking for brevity):

void __iomem *div_resources[EQC_MAX_DIV_COUNT];
unsigned int max_div_resource_index = 0;
const struct eqc_div *div;
struct clk_hw *parent;
void __iomem *base;
struct clk_hw *hw;

// Learn what resources should be acquired
for (i = 0; i < data->div_count; i++) {
	div = &data->divs[i];

	if (div->resource_index > max_div_resource_index)
		max_div_resource_index = div->resource_index;
}

// Grab resources (starting at 1 because 0 is for PLLs)
for (i = 1; i < max_div_resource_index; i++) {
	div_resources[i] = devm_platform_ioremap_resource(pdev, i);
	// TODO: error checking
}

// Register clocks
for (i = 0; i < data->div_count; i++) {
	div = &data->divs[i];
	base = div_resources[div->resource_index];

	parent = priv->cells->hws[div->parent];
	hw = clk_hw_register_divider_table_parent_hw(dev, div->name,
			parent, 0, base, div->shift, div->width,
			CLK_DIVIDER_EVEN_INTEGERS, NULL, NULL);
	priv->cells->hws[div->index] = hw;
	// TODO: error checking
}

>
> > 
> > 
> > [...]
> > 
> > > > +       /*
> > > > +        * We expect named resources if divider clocks are present.
> > > > +        * Else, we only expect one resource.
> > > > +        */
> > >
> > > Please avoid named resources. They give the false sense of hope that the
> > > binding can re-order the reg property when that can't be done. Instead,
> > > just index and know which index to use in the driver.
> > 
> > It is unclear what you mean by not being able to re-order reg property?
> > Are you talking about reg-names being most often defined as items const
> > list and therefore cannot be reordered? Here binding declare things
> > using minItems/maxItems/enum so it can be reordered, looking like:
>
> Yes, that's wrong.
>
> > 
> >   properties:
> >     reg:
> >       minItems: 2
> >       maxItems: 2
> >     reg-names:
> >       minItems: 2
> >       maxItems: 2
> >       items:
> >         enum: [ plls, ospi ]
> > 
> > If this is not what you are talking about then I rambled about garbage
> > and I'll use indexed resources.
> > 
>
> You cannot reorder strings in a DT binding property after the fact.
> While the code will keep working if the reg-names elements are
> re-ordered, the binding will be backwards incompatible, because the
> reg-names property must have the same order. It can be convenient to use
> reg-names if you have a long list of reg properties to map, but having
> two or one elements isn't a very strong argument.

I didn't know that beforehands. I completely get what you mean now.
Why it is that way is unclear to me but whatever. It is even documented:
https://elixir.bootlin.com/linux/v6.8.6/source/Documentation/devicetree/bindings/writing-bindings.rst#L64

Thanks Stephen,

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