| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <D0HCAV6APTSD.WKGPESJ29D8A@bootlin.com>
Date: Thu, 11 Apr 2024 15:49:34 +0200
From: Théo Lebrun <theo.lebrun@...tlin.com>
To: "Krzysztof Kozlowski" <krzysztof.kozlowski@...aro.org>, "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
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@...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-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@...000 {
clocks: clock-controller@...02c {
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@...000:
clock-controller@...02c: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
Powered by blists - more mailing lists