Re: [PATCH 05/11] clk: eyeq: add driver

[Théo Lebrun]

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