Re: [PATCH 02/11] dt-bindings: clock: mobileye,eyeq5-clk: add EyeQ6L and EyeQ6H
From: Théo Lebrun
Date: Thu Apr 11 2024 - 09:50:17 EST
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-clkyaml 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