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

Krzysztof Kozlowski
2024-04-11T06:14:05Z

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