| Message ID | 20240410-mbly-olb-v1-2-335e496d7be3@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add Mobileye EyeQ system controller support (clk, reset, pinctrl) | expand |
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
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
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 --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
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(-)