Message-ID: <f97cb190debc849b598e6675f8780470.sboyd@kernel.org>
Date: Thu, 11 Apr 2024 22:46:38 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Conor Dooley <conor+dt@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Linus Walleij <linus.walleij@...aro.org>, Michael Turquette <mturquette@...libre.com>, Philipp Zabel <p.zabel@...gutronix.de>, Rob Herring <robh@...nel.org>, Théo Lebrun <theo.lebrun@...tlin.com>
Cc: linux-mips@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org, linux-gpio@...r.kernel.org, Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>, Gregory CLEMENT <gregory.clement@...tlin.com>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>, Tawfik Bayouk <tawfik.bayouk@...ileye.com>
Subject: Re: [PATCH 05/11] clk: eyeq: add driver

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@...000 {
>                 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@...000 {
>                         compatible = "mobileye,eyeq5-reset";
>                         reg = <0x000 0x0c>, <0x200 0x34>, <0x120 0x04>;
>                         reg-names = "d0", "d1", "d2";
>                         #reset-cells = <2>;
>                 };
> 
>                 clocks: clock-controller@...02c {
>                         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@...0b0 {
>                         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@...000 {
>                 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.

Powered by blists - more mailing lists