[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <29ece6c8-ddf4-4dcd-b5b4-1cad8bc858d3@linaro.org>
Date: Thu, 11 Apr 2024 08:14:01 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Théo Lebrun <theo.lebrun@...tlin.com>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Michael Turquette <mturquette@...libre.com>, Stephen Boyd
<sboyd@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>,
Linus Walleij <linus.walleij@...aro.org>
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 02/11] dt-bindings: clock: mobileye,eyeq5-clk: add EyeQ6L
and EyeQ6H
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@...tlin.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@...tlin.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
Powered by blists - more mailing lists