diff mbox series

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

Message ID 20240410-mbly-olb-v1-2-335e496d7be3@bootlin.com (mailing list archive)
State Superseded
Headers show
Series Add Mobileye EyeQ system controller support (clk, reset, pinctrl) | expand

Commit Message

Théo Lebrun April 10, 2024, 5:12 p.m. UTC
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@bootlin.com>
---
 .../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(-)

Comments

Krzysztof Kozlowski April 11, 2024, 6:14 a.m. UTC | #1
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@bootlin.com>
> ---
>  .../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
> index 2d4f2cde1e58..a1651fcce258 100644
> --- a/Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml
> +++ b/Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml
> @@ -4,12 +4,13 @@
>  $id: http://devicetree.org/schemas/clock/mobileye,eyeq5-clk.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: Mobileye EyeQ5 clock controller
> +title: Mobileye EyeQ clock controller
>  
>  description:
> -  The EyeQ5 clock controller handles 10 read-only PLLs derived from the main
> -  crystal clock. It also exposes one divider clock, a child of one of the PLLs.
> -  Its registers live in a shared region called OLB.
> +  EyeQ clock controllers expose read-only PLLs derived from main crystal clock.
> +  Some also expose divider clocks, children of specific PLLs. Its registers
> +  live in a shared region called OLB. EyeQ5 and EyeQ6L have a single OLB
> +  instance while EyeQ6H have seven, leading to seven clock controllers.
>  
>  maintainers:
>    - Grégory Clement <gregory.clement@bootlin.com>
> @@ -18,18 +19,23 @@ maintainers:
>  
>  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.

>  
> -  reg-names:
> -    items:
> -      - const: plls
> -      - const: ospi
> +  reg-names: true

No, you must leave widest constraints here.


>  
>    "#clock-cells":
> -    const: 1
> +    enum: [0, 1]

Looks like you squash here quite different devices...

>  
>    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"?

> +          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.


> +        reg-names: false

That's redundant. Drop entire if.


> +
> +  # 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.



Best regards,
Krzysztof
Théo Lebrun April 11, 2024, 1:49 p.m. UTC | #2
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@bootlin.com>
> > ---
> >  .../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
Krzysztof Kozlowski April 11, 2024, 3:02 p.m. UTC | #3
On 11/04/2024 15:49, Théo Lebrun wrote:
>>> +    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#

Ah, so you defined the items but made them an random order? No, please
keep same syntax which is what we always recommend anyway:

https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L132

...

>>> +
>>> +  # 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?

I assumed all these are in one syscon. Lack of DTS (example is quite
limited, which is expected) does not help. Please link full DTS so we
can see what you want to achieve.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml b/Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml
index 2d4f2cde1e58..a1651fcce258 100644
--- a/Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml
+++ b/Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml
@@ -4,12 +4,13 @@ 
 $id: http://devicetree.org/schemas/clock/mobileye,eyeq5-clk.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Mobileye EyeQ5 clock controller
+title: Mobileye EyeQ clock controller
 
 description:
-  The EyeQ5 clock controller handles 10 read-only PLLs derived from the main
-  crystal clock. It also exposes one divider clock, a child of one of the PLLs.
-  Its registers live in a shared region called OLB.
+  EyeQ clock controllers expose read-only PLLs derived from main crystal clock.
+  Some also expose divider clocks, children of specific PLLs. Its registers
+  live in a shared region called OLB. EyeQ5 and EyeQ6L have a single OLB
+  instance while EyeQ6H have seven, leading to seven clock controllers.
 
 maintainers:
   - Grégory Clement <gregory.clement@bootlin.com>
@@ -18,18 +19,23 @@  maintainers:
 
 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
 
-  reg-names:
-    items:
-      - const: plls
-      - const: ospi
+  reg-names: true
 
   "#clock-cells":
-    const: 1
+    enum: [0, 1]
 
   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
+          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
+        reg-names: false
+
+  # 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
+    else:
+      properties:
+        "#clock-cells":
+          const: 1
+
 additionalProperties: false
diff --git a/MAINTAINERS b/MAINTAINERS
index 30dfbee84007..f5a488331b38 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14925,11 +14925,13 @@  M:	Gregory CLEMENT <gregory.clement@bootlin.com>
 M:	Théo Lebrun <theo.lebrun@bootlin.com>
 L:	linux-mips@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/clock/mobileye,eyeq5-clk.yaml
 F:	Documentation/devicetree/bindings/mips/mobileye.yaml
 F:	Documentation/devicetree/bindings/soc/mobileye/
 F:	arch/mips/boot/dts/mobileye/
 F:	arch/mips/configs/eyeq5_defconfig
 F:	arch/mips/mobileye/board-epm5.its.S
+F:	include/dt-bindings/clock/mobileye,eyeq5-clk.h
 
 MODULE SUPPORT
 M:	Luis Chamberlain <mcgrof@kernel.org>
diff --git a/include/dt-bindings/clock/mobileye,eyeq5-clk.h b/include/dt-bindings/clock/mobileye,eyeq5-clk.h
index 26d8930335e4..b433c1772c28 100644
--- a/include/dt-bindings/clock/mobileye,eyeq5-clk.h
+++ b/include/dt-bindings/clock/mobileye,eyeq5-clk.h
@@ -19,4 +19,25 @@ 
 
 #define EQ5C_DIV_OSPI	10
 
+#define EQ6LC_PLL_DDR		0
+#define EQ6LC_PLL_CPU		1
+#define EQ6LC_PLL_PER		2
+#define EQ6LC_PLL_VDI		3
+
+#define EQ6HC_SOUTH_PLL_VDI		0
+#define EQ6HC_SOUTH_PLL_PCIE		1
+#define EQ6HC_SOUTH_PLL_PER		2
+#define EQ6HC_SOUTH_PLL_ISP		3
+
+#define EQ6HC_SOUTH_DIV_EMMC		4
+#define EQ6HC_SOUTH_DIV_OSPI_REF	5
+#define EQ6HC_SOUTH_DIV_OSPI_SYS	6
+#define EQ6HC_SOUTH_DIV_TSU		7
+
+#define EQ6HC_ACC_PLL_XNN		0
+#define EQ6HC_ACC_PLL_VMP		1
+#define EQ6HC_ACC_PLL_PMA		2
+#define EQ6HC_ACC_PLL_MPC		3
+#define EQ6HC_ACC_PLL_NOC		4
+
 #endif