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

[Théo Lebrun <theo.lebrun@xxxxxxxxxxx>]

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