Re: [PATCH 02/11] dt-bindings: clock: mobileye,eyeq5-clk: add EyeQ6L and EyeQ6H

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

Hello Krzysztof,

On Thu Apr 11, 2024 at 8:14 AM CEST, Krzysztof Kozlowski wrote:
> On 10/04/2024 19:12, Théo Lebrun wrote:
> > Add bindings describing EyeQ6L and EyeQ6H clock controllers.
> > Add constants to index clocks.
> > 
> > Bindings are conditional for two reasons:
> >  - Some compatibles expose a single clock; they do not take clock cells.
> >  - All compatibles take a PLLs resource, not all take others (aimed at
> >    divider clocks). Those that only take a resource for PLLs do not
> >    require named resources.
> > 
> > Signed-off-by: Théo Lebrun <theo.lebrun@xxxxxxxxxxx>
> > ---
> >  .../bindings/clock/mobileye,eyeq5-clk.yaml         | 103 ++++++++++++++++++---
> >  MAINTAINERS                                        |   2 +
> >  include/dt-bindings/clock/mobileye,eyeq5-clk.h     |  21 +++++
> >  3 files changed, 113 insertions(+), 13 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml b/Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml

[...]

> >  properties:
> >    compatible:
> > -    const: mobileye,eyeq5-clk
> > +    enum:
> > +      - mobileye,eyeq5-clk
> > +      - mobileye,eyeq6l-clk
> > +      - mobileye,eyeq6h-central-clk
> > +      - mobileye,eyeq6h-west-clk
> > +      - mobileye,eyeq6h-east-clk
> > +      - mobileye,eyeq6h-south-clk
> > +      - mobileye,eyeq6h-ddr0-clk
> > +      - mobileye,eyeq6h-ddr1-clk
> > +      - mobileye,eyeq6h-acc-clk
> >  
> > -  reg:
> > -    maxItems: 2
> > +  reg: true
>
> No, you must leave widest constraints here.

Noted, will do.

> > -  reg-names:
> > -    items:
> > -      - const: plls
> > -      - const: ospi
> > +  reg-names: true
>
> No, you must leave widest constraints here.

Noted, will do.

> >    "#clock-cells":
> > -    const: 1
> > +    enum: [0, 1]
>
> Looks like you squash here quite different devices...

They are the same controllers but some only expose a single clock. It is
EyeQ6H that has 7 OLB instances, so some don't deal with many clocks.

I started with a more generic approach of #clock-cells = <1> and only
have index zero available for those that have a single clock.
I am not a fan of this however.

> >    clocks:
> >      maxItems: 1
> > @@ -43,9 +49,80 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > -  - reg-names
> >    - "#clock-cells"
> >    - clocks
> >    - clock-names
> >  
> > +allOf:
> > +  # "mobileye,eyeq5-clk" provides:
> > +  #  - PLLs and,
> > +  #  - One divider clock related to ospi.
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          const: mobileye,eyeq5-clk
> > +    then:
> > +      properties:
> > +        reg:
> > +          minItems: 2
> > +          maxItems: 2
> > +        reg-names:
> > +          minItems: 2
> > +          maxItems: 2
>
> So any name is now valid? Like "yellow-pony"?

I do not understand what implies this. Below "items: enum: [...]"
ensures only two allowed values. dtbs_check agrees:

⟩ git diff
diff --git a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
           b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
index 8d4f65ec912d..5031eb8b4270 100644
--- a/arch/mips/boot/dts/mobileye/eyeq5.dtsi
+++ b/arch/mips/boot/dts/mobileye/eyeq5.dtsi
@@ -126,7 +126,7 @@ reset: reset-controller@e00000 {
                        clocks: clock-controller@e0002c {
                                compatible = "mobileye,eyeq5-clk";
                                reg = <0x02c 0x50>, <0x11c 0x04>;
-                               reg-names = "plls", "ospi";
+                               reg-names = "plls", "yellow-pony";
                                #clock-cells = <1>;
                                clocks = <&xtal>;
                                clock-names = "ref";

⟩ make dtbs_check DT_SCHEMA_FILES=mobileye DT_CHECKER_FLAGS=-m
  UPD     include/config/kernel.release
  DTC_CHK arch/mips/boot/dts/mobileye/eyeq5-epm5.dtb
arch/mips/boot/dts/mobileye/eyeq5-epm5.dtb: system-controller@e00000:
  clock-controller@e0002c:reg-names:1:
  'yellow-pony' is not one of ['plls', 'ospi']
  from schema $id:
    http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml#

> > +          items:
> > +            enum: [ plls, ospi ]
> > +      required:
> > +        - reg-names
> > +
> > +  # "mobileye,eyeq6h-south-clk" provides:
> > +  #  - PLLs and,
> > +  #  - Four divider clocks related to emmc, ospi and tsu.
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          const: mobileye,eyeq6h-south-clk
> > +    then:
> > +      properties:
> > +        reg:
> > +          minItems: 4
> > +          maxItems: 4
> > +        reg-names:
> > +          minItems: 4
> > +          maxItems: 4
> > +          items:
> > +            enum: [ plls, emmc, ospi, tsu ]
> > +      required:
> > +        - reg-names
> > +
> > +  # Other compatibles only provide PLLs. Do not ask for named resources.
> > +  - if:
> > +      not:
> > +        required:
> > +          - reg-names
> > +    then:
> > +      properties:
> > +        reg:
> > +          minItems: 1
> > +          maxItems: 1
>
> No, just restrict properly reg per variant.

Noted, will do.

> > +        reg-names: false
>
> That's redundant. Drop entire if.

Ah, yes. Will fix that.

> > +
> > +  # Some compatibles provide a single clock; they do not take a clock cell.
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          enum:
> > +            - mobileye,eyeq6h-central-clk
> > +            - mobileye,eyeq6h-west-clk
> > +            - mobileye,eyeq6h-east-clk
> > +            - mobileye,eyeq6h-ddr0-clk
> > +            - mobileye,eyeq6h-ddr1-clk
> > +    then:
> > +      properties:
> > +        "#clock-cells":
> > +          const: 0
>
> Wait, so you define device-per-clock? That's a terrible idea. We also
> discussed it many times and it was rejected many times.
>
> You have one device, not 5.

Each region must be a syscon to make its various registers accessible to
drivers that'll need it. Following that, I have a hard time seeing what
would be the DT structure of 7 OLB system-controllers but a single
clock node?

Regards,

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