| Message ID | 20240123-mbly-clk-v3-4-392b010b8281@bootlin.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add support for Mobileye EyeQ5 system controller | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On Tue, 23 Jan 2024 19:46:49 +0100, Théo Lebrun wrote: > Add documentation to describe the "Other Logic Block" syscon. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 77 ++++++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 78 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/clock/mobileye,eyeq5-clk.yaml /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: system-controller@e00000: clock-controller: False schema does not allow {'compatible': ['mobileye,eyeq5-clk'], '#clock-cells': [[1]], 'clocks': [[4294967295]], 'clock-names': ['ref']} from schema $id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: system-controller@e00000: reset-controller: False schema does not allow {'compatible': ['mobileye,eyeq5-reset'], '#reset-cells': [[2]]} from schema $id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: system-controller@e00000: pinctrl-a: False schema does not allow {'compatible': ['mobileye,eyeq5-a-pinctrl'], '#pinctrl-cells': [[1]]} from schema $id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: system-controller@e00000: pinctrl-b: False schema does not allow {'compatible': ['mobileye,eyeq5-b-pinctrl'], '#pinctrl-cells': [[1]]} from schema $id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml# Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: /example-0/system-controller@e00000/clock-controller: failed to match any schema with compatible: ['mobileye,eyeq5-clk'] Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: /example-0/system-controller@e00000/reset-controller: failed to match any schema with compatible: ['mobileye,eyeq5-reset'] Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: /example-0/system-controller@e00000/pinctrl-a: failed to match any schema with compatible: ['mobileye,eyeq5-a-pinctrl'] Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.example.dtb: /example-0/system-controller@e00000/pinctrl-b: failed to match any schema with compatible: ['mobileye,eyeq5-b-pinctrl'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240123-mbly-clk-v3-4-392b010b8281@bootlin.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Tue, Jan 23, 2024 at 07:46:49PM +0100, Théo Lebrun wrote: > Add documentation to describe the "Other Logic Block" syscon. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 77 ++++++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 78 insertions(+) > > diff --git a/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml > new file mode 100644 > index 000000000000..031ef6a532c1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml > @@ -0,0 +1,77 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Mobileye EyeQ5 SoC system controller > + > +maintainers: > + - Grégory Clement <gregory.clement@bootlin.com> > + - Théo Lebrun <theo.lebrun@bootlin.com> > + - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com> > + > +description: > + OLB ("Other Logic Block") is a hardware block grouping smaller blocks. Clocks, > + resets, pinctrl are being handled from here. > + > +properties: > + compatible: > + items: > + - const: mobileye,eyeq5-olb > + - const: syscon > + - const: simple-mfd > + > + reg: > + maxItems: 1 > + > + clock-controller: > + $ref: /schemas/clock/mobileye,eyeq5-clk.yaml# > + type: object > + > + reset-controller: > + $ref: /schemas/reset/mobileye,eyeq5-reset.yaml# > + type: object > + > + pinctrl-a: > + $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml# > + type: object > + > + pinctrl-b: > + $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml# > + type: object > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + system-controller@e00000 { > + compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd"; > + reg = <0xe00000 0x400>; > + > + clock-controller { > + compatible = "mobileye,eyeq5-clk"; > + #clock-cells = <1>; > + clocks = <&xtal>; > + clock-names = "ref"; > + }; > + > + reset-controller { > + compatible = "mobileye,eyeq5-reset"; > + #reset-cells = <2>; > + }; > + > + pinctrl-a { > + compatible = "mobileye,eyeq5-a-pinctrl"; > + #pinctrl-cells = <1>; Sure you need this? Generally only pinctrl-single uses this. > + }; > + > + pinctrl-b { > + compatible = "mobileye,eyeq5-b-pinctrl"; > + #pinctrl-cells = <1>; > + }; > + }; This can all be simplified to: system-controller@e00000 { compatible = "mobileye,eyeq5-olb", "syscon"; reg = <0xe00000 0x400>; #reset-cells = <2>; #clock-cells = <1>; clocks = <&xtal>; clock-names = "ref"; pins { ... }; }; There is no need for sub nodes unless you have reusable blocks or each block has its own resources in DT. Rob
Hello, On Wed Jan 24, 2024 at 4:14 PM CET, Rob Herring wrote: > On Tue, Jan 23, 2024 at 07:46:49PM +0100, Théo Lebrun wrote: > > Add documentation to describe the "Other Logic Block" syscon. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 77 ++++++++++++++++++++++ > > MAINTAINERS | 1 + > > 2 files changed, 78 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml > > new file mode 100644 > > index 000000000000..031ef6a532c1 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml > > @@ -0,0 +1,77 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Mobileye EyeQ5 SoC system controller > > + > > +maintainers: > > + - Grégory Clement <gregory.clement@bootlin.com> > > + - Théo Lebrun <theo.lebrun@bootlin.com> > > + - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com> > > + > > +description: > > + OLB ("Other Logic Block") is a hardware block grouping smaller blocks. Clocks, > > + resets, pinctrl are being handled from here. > > + > > +properties: > > + compatible: > > + items: > > + - const: mobileye,eyeq5-olb > > + - const: syscon > > + - const: simple-mfd > > + > > + reg: > > + maxItems: 1 > > + > > + clock-controller: > > + $ref: /schemas/clock/mobileye,eyeq5-clk.yaml# > > + type: object > > + > > + reset-controller: > > + $ref: /schemas/reset/mobileye,eyeq5-reset.yaml# > > + type: object > > + > > + pinctrl-a: > > + $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml# > > + type: object > > + > > + pinctrl-b: > > + $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml# > > + type: object > > + > > +required: > > + - compatible > > + - reg > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + system-controller@e00000 { > > + compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd"; > > + reg = <0xe00000 0x400>; > > + > > + clock-controller { > > + compatible = "mobileye,eyeq5-clk"; > > + #clock-cells = <1>; > > + clocks = <&xtal>; > > + clock-names = "ref"; > > + }; > > + > > + reset-controller { > > + compatible = "mobileye,eyeq5-reset"; > > + #reset-cells = <2>; > > + }; > > + > > + pinctrl-a { > > + compatible = "mobileye,eyeq5-a-pinctrl"; > > + #pinctrl-cells = <1>; > > Sure you need this? Generally only pinctrl-single uses this. You are completely right, it is useless. I naively expected it in the same vein as other subsystems. > > > + }; > > + > > + pinctrl-b { > > + compatible = "mobileye,eyeq5-b-pinctrl"; > > + #pinctrl-cells = <1>; > > + }; > > + }; > > This can all be simplified to: > > system-controller@e00000 { > compatible = "mobileye,eyeq5-olb", "syscon"; > reg = <0xe00000 0x400>; > #reset-cells = <2>; > #clock-cells = <1>; > clocks = <&xtal>; > clock-names = "ref"; > > pins { ... }; > }; > > There is no need for sub nodes unless you have reusable blocks or each > block has its own resources in DT. That is right, and it does simplify the devicetree as you have shown. However, the split nodes gives the following advantages: - Devicetree-wise, it allows for one alias per function. `clocks = <&clocks EQ5C_PLL_CPU>` is surely more intuitive than `clocks = <&olb EQ5C_PLL_CPU>;`. Same for reset. - It means an MFD driver must be implemented, adding between 100 to 200 lines of boilerplate code to the kernel. - It means one pinctrl device for the two banks. That addresses your comment on [PATCH v3 10/17]. This is often done and would be doable on this platform. However it means added logic to each individual function of pinctrl-eyeq5. Overall it makes for less readable code, for code that already looks more complex than it really is. My initial non-public version of pinctrl-eyeq5 was using this method (a device handling both banks) and I've leaned away from it. Those are all minor, but I don't have the feeling a few lines and nodes less in devicetree compensate for those. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Wed Jan 24, 2024 at 6:28 PM CET, Théo Lebrun wrote: > Hello, > > On Wed Jan 24, 2024 at 4:14 PM CET, Rob Herring wrote: > > On Tue, Jan 23, 2024 at 07:46:49PM +0100, Théo Lebrun wrote: > > > Add documentation to describe the "Other Logic Block" syscon. > > > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > > --- > > > .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 77 ++++++++++++++++++++++ > > > MAINTAINERS | 1 + > > > 2 files changed, 78 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml > > > new file mode 100644 > > > index 000000000000..031ef6a532c1 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml > > > @@ -0,0 +1,77 @@ > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Mobileye EyeQ5 SoC system controller > > > + > > > +maintainers: > > > + - Grégory Clement <gregory.clement@bootlin.com> > > > + - Théo Lebrun <theo.lebrun@bootlin.com> > > > + - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com> > > > + > > > +description: > > > + OLB ("Other Logic Block") is a hardware block grouping smaller blocks. Clocks, > > > + resets, pinctrl are being handled from here. > > > + > > > +properties: > > > + compatible: > > > + items: > > > + - const: mobileye,eyeq5-olb > > > + - const: syscon > > > + - const: simple-mfd > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + clock-controller: > > > + $ref: /schemas/clock/mobileye,eyeq5-clk.yaml# > > > + type: object > > > + > > > + reset-controller: > > > + $ref: /schemas/reset/mobileye,eyeq5-reset.yaml# > > > + type: object > > > + > > > + pinctrl-a: > > > + $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml# > > > + type: object > > > + > > > + pinctrl-b: > > > + $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml# > > > + type: object > > > + > > > +required: > > > + - compatible > > > + - reg > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + system-controller@e00000 { > > > + compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd"; > > > + reg = <0xe00000 0x400>; > > > + > > > + clock-controller { > > > + compatible = "mobileye,eyeq5-clk"; > > > + #clock-cells = <1>; > > > + clocks = <&xtal>; > > > + clock-names = "ref"; > > > + }; > > > + > > > + reset-controller { > > > + compatible = "mobileye,eyeq5-reset"; > > > + #reset-cells = <2>; > > > + }; > > > + > > > + pinctrl-a { > > > + compatible = "mobileye,eyeq5-a-pinctrl"; > > > + #pinctrl-cells = <1>; > > > > Sure you need this? Generally only pinctrl-single uses this. > > You are completely right, it is useless. I naively expected it in the > same vein as other subsystems. > > > > > > + }; > > > + > > > + pinctrl-b { > > > + compatible = "mobileye,eyeq5-b-pinctrl"; > > > + #pinctrl-cells = <1>; > > > + }; > > > + }; > > > > This can all be simplified to: > > > > system-controller@e00000 { > > compatible = "mobileye,eyeq5-olb", "syscon"; > > reg = <0xe00000 0x400>; > > #reset-cells = <2>; > > #clock-cells = <1>; > > clocks = <&xtal>; > > clock-names = "ref"; > > > > pins { ... }; > > }; > > > > There is no need for sub nodes unless you have reusable blocks or each > > block has its own resources in DT. > > That is right, and it does simplify the devicetree as you have shown. > However, the split nodes gives the following advantages: > > - Devicetree-wise, it allows for one alias per function. > `clocks = <&clocks EQ5C_PLL_CPU>` is surely more intuitive > than `clocks = <&olb EQ5C_PLL_CPU>;`. Same for reset. > > - It means an MFD driver must be implemented, adding between 100 to 200 > lines of boilerplate code to the kernel. > > - It means one pinctrl device for the two banks. That addresses your > comment on [PATCH v3 10/17]. This is often done and would be doable > on this platform. However it means added logic to each individual > function of pinctrl-eyeq5. > > Overall it makes for less readable code, for code that already looks > more complex than it really is. > > My initial non-public version of pinctrl-eyeq5 was using this method > (a device handling both banks) and I've leaned away from it. I had forgotten one other reason: - Reusability does count for something. Other Mobileye platforms exist, and the system controller stuff is more complex on those. Multiple different OLB blocks, etc. But my understanding is that per-peripheral logic is reused across versions. > > Those are all minor, but I don't have the feeling a few lines and nodes > less in devicetree compensate for those. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Wed, Jan 24, 2024 at 11:40 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > Hello, > > On Wed Jan 24, 2024 at 6:28 PM CET, Théo Lebrun wrote: > > Hello, > > > > On Wed Jan 24, 2024 at 4:14 PM CET, Rob Herring wrote: > > > On Tue, Jan 23, 2024 at 07:46:49PM +0100, Théo Lebrun wrote: > > > > Add documentation to describe the "Other Logic Block" syscon. > > > > > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > > > --- > > > > .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 77 ++++++++++++++++++++++ > > > > MAINTAINERS | 1 + > > > > 2 files changed, 78 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml > > > > new file mode 100644 > > > > index 000000000000..031ef6a532c1 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml > > > > @@ -0,0 +1,77 @@ > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Mobileye EyeQ5 SoC system controller > > > > + > > > > +maintainers: > > > > + - Grégory Clement <gregory.clement@bootlin.com> > > > > + - Théo Lebrun <theo.lebrun@bootlin.com> > > > > + - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com> > > > > + > > > > +description: > > > > + OLB ("Other Logic Block") is a hardware block grouping smaller blocks. Clocks, > > > > + resets, pinctrl are being handled from here. > > > > + > > > > +properties: > > > > + compatible: > > > > + items: > > > > + - const: mobileye,eyeq5-olb > > > > + - const: syscon > > > > + - const: simple-mfd > > > > + > > > > + reg: > > > > + maxItems: 1 > > > > + > > > > + clock-controller: > > > > + $ref: /schemas/clock/mobileye,eyeq5-clk.yaml# > > > > + type: object > > > > + > > > > + reset-controller: > > > > + $ref: /schemas/reset/mobileye,eyeq5-reset.yaml# > > > > + type: object > > > > + > > > > + pinctrl-a: > > > > + $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml# > > > > + type: object > > > > + > > > > + pinctrl-b: > > > > + $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml# > > > > + type: object > > > > + > > > > +required: > > > > + - compatible > > > > + - reg > > > > + > > > > +additionalProperties: false > > > > + > > > > +examples: > > > > + - | > > > > + system-controller@e00000 { > > > > + compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd"; > > > > + reg = <0xe00000 0x400>; > > > > + > > > > + clock-controller { > > > > + compatible = "mobileye,eyeq5-clk"; > > > > + #clock-cells = <1>; > > > > + clocks = <&xtal>; > > > > + clock-names = "ref"; > > > > + }; > > > > + > > > > + reset-controller { > > > > + compatible = "mobileye,eyeq5-reset"; > > > > + #reset-cells = <2>; > > > > + }; > > > > + > > > > + pinctrl-a { > > > > + compatible = "mobileye,eyeq5-a-pinctrl"; > > > > + #pinctrl-cells = <1>; > > > > > > Sure you need this? Generally only pinctrl-single uses this. > > > > You are completely right, it is useless. I naively expected it in the > > same vein as other subsystems. > > > > > > > > > + }; > > > > + > > > > + pinctrl-b { > > > > + compatible = "mobileye,eyeq5-b-pinctrl"; > > > > + #pinctrl-cells = <1>; > > > > + }; > > > > + }; > > > > > > This can all be simplified to: > > > > > > system-controller@e00000 { > > > compatible = "mobileye,eyeq5-olb", "syscon"; > > > reg = <0xe00000 0x400>; > > > #reset-cells = <2>; > > > #clock-cells = <1>; > > > clocks = <&xtal>; > > > clock-names = "ref"; > > > > > > pins { ... }; > > > }; > > > > > > There is no need for sub nodes unless you have reusable blocks or each > > > block has its own resources in DT. > > > > That is right, and it does simplify the devicetree as you have shown. > > However, the split nodes gives the following advantages: > > > > - Devicetree-wise, it allows for one alias per function. > > `clocks = <&clocks EQ5C_PLL_CPU>` is surely more intuitive > > than `clocks = <&olb EQ5C_PLL_CPU>;`. Same for reset. clocks: resets: pinctrl: system-controller@e00000 { > > > > - It means an MFD driver must be implemented, adding between 100 to 200 > > lines of boilerplate code to the kernel. From a binding perspective, not my problem... That's Linux details defining the binding. What about u-boot, BSD, future versions of Linux with different structure? I don't think an MFD is required here. A driver should be able to be both clock and reset provider. That's pretty common. pinctrl less so. > > - It means one pinctrl device for the two banks. That addresses your > > comment on [PATCH v3 10/17]. This is often done and would be doable > > on this platform. However it means added logic to each individual > > function of pinctrl-eyeq5. If it makes things easier, 2 'pins' sub-nodes is fine. That's just container nodes. > > Overall it makes for less readable code, for code that already looks > > more complex than it really is. > > > > My initial non-public version of pinctrl-eyeq5 was using this method > > (a device handling both banks) and I've leaned away from it. > > I had forgotten one other reason: > > - Reusability does count for something. Other Mobileye platforms exist, > and the system controller stuff is more complex on those. Multiple > different OLB blocks, etc. But my understanding is that > per-peripheral logic is reused across versions. IME, this stuff never stays exactly the same from chip to chip. Rob
On 24/01/2024 16:14, Rob Herring wrote: >> + >> + pinctrl-b { >> + compatible = "mobileye,eyeq5-b-pinctrl"; >> + #pinctrl-cells = <1>; >> + }; >> + }; > > This can all be simplified to: > > system-controller@e00000 { > compatible = "mobileye,eyeq5-olb", "syscon"; > reg = <0xe00000 0x400>; > #reset-cells = <2>; > #clock-cells = <1>; > clocks = <&xtal>; > clock-names = "ref"; > > pins { ... }; > }; > > There is no need for sub nodes unless you have reusable blocks or each > block has its own resources in DT. Yes, however I believe there should be resources here: each subnode should get its address space. This is a bit tied to implementation, which currently assumes "everyone can fiddle with everything" in this block. Theo, can you draw memory map? Best regards, Krzysztof
Hello, On Thu Jan 25, 2024 at 8:51 AM CET, Krzysztof Kozlowski wrote: > On 24/01/2024 16:14, Rob Herring wrote: > >> + > >> + pinctrl-b { > >> + compatible = "mobileye,eyeq5-b-pinctrl"; > >> + #pinctrl-cells = <1>; > >> + }; > >> + }; > > > > This can all be simplified to: > > > > system-controller@e00000 { > > compatible = "mobileye,eyeq5-olb", "syscon"; > > reg = <0xe00000 0x400>; > > #reset-cells = <2>; > > #clock-cells = <1>; > > clocks = <&xtal>; > > clock-names = "ref"; > > > > pins { ... }; > > }; > > > > There is no need for sub nodes unless you have reusable blocks or each > > block has its own resources in DT. > > Yes, however I believe there should be resources here: each subnode > should get its address space. This is a bit tied to implementation, > which currently assumes "everyone can fiddle with everything" in this block. > > Theo, can you draw memory map? It would be a mess. I've counted things up. The first 147 registers are used in this 0x400 block. There are 31 individual blocks, with 7 registers unused (holes to align next block). Functions are reset, clocks, LBIST, MBIST, DDR control, GPIO, accelerator control, CPU entrypoint, PDTrace, IRQs, chip info & ID stuff, control registers for PCIe / eMMC / Eth / SGMII / DMA / etc. Some will never get used from Linux, others might. Maybe a moderate approach would be to create ressources for major blocks and make it evolve organically, without imposing that all uses lead to a new ressource creation. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Wed Jan 24, 2024 at 8:22 PM CET, Rob Herring wrote: > On Wed, Jan 24, 2024 at 11:40 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > On Wed Jan 24, 2024 at 6:28 PM CET, Théo Lebrun wrote: > > > On Wed Jan 24, 2024 at 4:14 PM CET, Rob Herring wrote: > > > > On Tue, Jan 23, 2024 at 07:46:49PM +0100, Théo Lebrun wrote: [...] > > > > > + }; > > > > > + > > > > > + pinctrl-b { > > > > > + compatible = "mobileye,eyeq5-b-pinctrl"; > > > > > + #pinctrl-cells = <1>; > > > > > + }; > > > > > + }; > > > > > > > > This can all be simplified to: > > > > > > > > system-controller@e00000 { > > > > compatible = "mobileye,eyeq5-olb", "syscon"; > > > > reg = <0xe00000 0x400>; > > > > #reset-cells = <2>; > > > > #clock-cells = <1>; > > > > clocks = <&xtal>; > > > > clock-names = "ref"; > > > > > > > > pins { ... }; > > > > }; > > > > > > > > There is no need for sub nodes unless you have reusable blocks or each > > > > block has its own resources in DT. > > > > > > That is right, and it does simplify the devicetree as you have shown. > > > However, the split nodes gives the following advantages: > > > > > > - Devicetree-wise, it allows for one alias per function. > > > `clocks = <&clocks EQ5C_PLL_CPU>` is surely more intuitive > > > than `clocks = <&olb EQ5C_PLL_CPU>;`. Same for reset. > > clocks: resets: pinctrl: system-controller@e00000 { > > > > > > > - It means an MFD driver must be implemented, adding between 100 to 200 > > > lines of boilerplate code to the kernel. > > From a binding perspective, not my problem... That's Linux details > defining the binding. What about u-boot, BSD, future versions of Linux > with different structure? > > I don't think an MFD is required here. A driver should be able to be > both clock and reset provider. That's pretty common. pinctrl less so. @Rob & @Krzysztof: following Krzysztof's question about the memory map and adding ressources to the system-controller, I was wondering if the following approach would be more suitable: olb: system-controller@e00000 { compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd"; reg = <0 0xe00000 0x0 0x400>; #address-cells = <1>; #size-cells = <1>; clocks: clock-controller { compatible = "mobileye,eyeq5-clk"; reg = <0x02c 0x7C>; #clock-cells = <1>; clocks = <&xtal>; clock-names = "ref"; }; reset: reset-controller { compatible = "mobileye,eyeq5-reset"; reg = <0x004 0x08>, <0x120 0x04>, <0x200 0x34>; reg-names = "d0", "d2", "d1"; #reset-cells = <2>; }; pinctrl0: pinctrl-a { compatible = "mobileye,eyeq5-a-pinctrl"; reg = <0x0B0 0x30>; }; pinctrl1: pinctrl-b { compatible = "mobileye,eyeq5-b-pinctrl"; reg = <0x0B0 0x30>; }; }; It highlights that they are in fact separate controllers and not one device. The common thing between them is that they were custom-implemented by Mobileye and therefore all registers were put in a single block. Else we'll go with the driver that implements both the clock & reset providers. It'd live in drivers/clk/ I believe, as this is where other drivers of the sort live. > > > - It means one pinctrl device for the two banks. That addresses your > > > comment on [PATCH v3 10/17]. This is often done and would be doable > > > on this platform. However it means added logic to each individual > > > function of pinctrl-eyeq5. > > If it makes things easier, 2 'pins' sub-nodes is fine. That's just > container nodes. > > > > Overall it makes for less readable code, for code that already looks > > > more complex than it really is. > > > > > > My initial non-public version of pinctrl-eyeq5 was using this method > > > (a device handling both banks) and I've leaned away from it. > > > > I had forgotten one other reason: > > > > - Reusability does count for something. Other Mobileye platforms exist, > > and the system controller stuff is more complex on those. Multiple > > different OLB blocks, etc. But my understanding is that > > per-peripheral logic is reused across versions. > > IME, this stuff never stays exactly the same from chip to chip. If it helps, I have access to the downstream vendor kernel to see how things work there. It supports the next generation of Mobileye hardware. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 1/25/24 5:01 AM, Théo Lebrun wrote: > Hello, > > On Thu Jan 25, 2024 at 8:51 AM CET, Krzysztof Kozlowski wrote: >> On 24/01/2024 16:14, Rob Herring wrote: >>>> + >>>> + pinctrl-b { >>>> + compatible = "mobileye,eyeq5-b-pinctrl"; >>>> + #pinctrl-cells = <1>; >>>> + }; >>>> + }; >>> >>> This can all be simplified to: >>> >>> system-controller@e00000 { >>> compatible = "mobileye,eyeq5-olb", "syscon"; >>> reg = <0xe00000 0x400>; >>> #reset-cells = <2>; >>> #clock-cells = <1>; >>> clocks = <&xtal>; >>> clock-names = "ref"; >>> >>> pins { ... }; >>> }; >>> >>> There is no need for sub nodes unless you have reusable blocks or each >>> block has its own resources in DT. >> >> Yes, however I believe there should be resources here: each subnode >> should get its address space. This is a bit tied to implementation, >> which currently assumes "everyone can fiddle with everything" in this block. >> >> Theo, can you draw memory map? > > It would be a mess. I've counted things up. The first 147 registers are > used in this 0x400 block. There are 31 individual blocks, with 7 > registers unused (holes to align next block). > > Functions are reset, clocks, LBIST, MBIST, DDR control, GPIO, > accelerator control, CPU entrypoint, PDTrace, IRQs, chip info & ID > stuff, control registers for PCIe / eMMC / Eth / SGMII / DMA / etc. > > Some will never get used from Linux, others might. Maybe a moderate > approach would be to create ressources for major blocks and make it > evolve organically, without imposing that all uses lead to a new > ressource creation. > That is usually how nodes are added to DT. If you modeled this system-controller space as a "simple-bus" instead of a "syscon" device, you could add nodes as you implement them. Rather than all at once as you have to by treating this space as one large blob device. Andrew > Thanks, > > -- > Théo Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com >
Hello, On Thu Jan 25, 2024 at 3:33 PM CET, Andrew Davis wrote: > On 1/25/24 5:01 AM, Théo Lebrun wrote: > > Hello, > > > > On Thu Jan 25, 2024 at 8:51 AM CET, Krzysztof Kozlowski wrote: > >> On 24/01/2024 16:14, Rob Herring wrote: > >>>> + > >>>> + pinctrl-b { > >>>> + compatible = "mobileye,eyeq5-b-pinctrl"; > >>>> + #pinctrl-cells = <1>; > >>>> + }; > >>>> + }; > >>> > >>> This can all be simplified to: > >>> > >>> system-controller@e00000 { > >>> compatible = "mobileye,eyeq5-olb", "syscon"; > >>> reg = <0xe00000 0x400>; > >>> #reset-cells = <2>; > >>> #clock-cells = <1>; > >>> clocks = <&xtal>; > >>> clock-names = "ref"; > >>> > >>> pins { ... }; > >>> }; > >>> > >>> There is no need for sub nodes unless you have reusable blocks or each > >>> block has its own resources in DT. > >> > >> Yes, however I believe there should be resources here: each subnode > >> should get its address space. This is a bit tied to implementation, > >> which currently assumes "everyone can fiddle with everything" in this block. > >> > >> Theo, can you draw memory map? > > > > It would be a mess. I've counted things up. The first 147 registers are > > used in this 0x400 block. There are 31 individual blocks, with 7 > > registers unused (holes to align next block). > > > > Functions are reset, clocks, LBIST, MBIST, DDR control, GPIO, > > accelerator control, CPU entrypoint, PDTrace, IRQs, chip info & ID > > stuff, control registers for PCIe / eMMC / Eth / SGMII / DMA / etc. > > > > Some will never get used from Linux, others might. Maybe a moderate > > approach would be to create ressources for major blocks and make it > > evolve organically, without imposing that all uses lead to a new > > ressource creation. > > > > That is usually how nodes are added to DT. If you modeled this > system-controller space as a "simple-bus" instead of a "syscon" > device, you could add nodes as you implement them. Rather than > all at once as you have to by treating this space as one large > blob device. I see where you are coming from, but in our case modeling our DT node as a simple-bus would be lying about the hardware behind. There is no such underlying bus. Let's try to keep the devicetree an abstraction describing the hardware. Also, we are having conflicts because multiple such child nodes are being added at the same time as the base node. Once this initial series is out (meaning dt-bindings for the OLB will exist) we'll be able to add new nodes or ressources on a whim. Have you got an opinion on the approach described in this email? https://lore.kernel.org/lkml/CYNRCGYA1PJ2.FYENLB4SRJWH@bootlin.com/ Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 1/25/24 8:49 AM, Théo Lebrun wrote: > Hello, > > On Thu Jan 25, 2024 at 3:33 PM CET, Andrew Davis wrote: >> On 1/25/24 5:01 AM, Théo Lebrun wrote: >>> Hello, >>> >>> On Thu Jan 25, 2024 at 8:51 AM CET, Krzysztof Kozlowski wrote: >>>> On 24/01/2024 16:14, Rob Herring wrote: >>>>>> + >>>>>> + pinctrl-b { >>>>>> + compatible = "mobileye,eyeq5-b-pinctrl"; >>>>>> + #pinctrl-cells = <1>; >>>>>> + }; >>>>>> + }; >>>>> >>>>> This can all be simplified to: >>>>> >>>>> system-controller@e00000 { >>>>> compatible = "mobileye,eyeq5-olb", "syscon"; >>>>> reg = <0xe00000 0x400>; >>>>> #reset-cells = <2>; >>>>> #clock-cells = <1>; >>>>> clocks = <&xtal>; >>>>> clock-names = "ref"; >>>>> >>>>> pins { ... }; >>>>> }; >>>>> >>>>> There is no need for sub nodes unless you have reusable blocks or each >>>>> block has its own resources in DT. >>>> >>>> Yes, however I believe there should be resources here: each subnode >>>> should get its address space. This is a bit tied to implementation, >>>> which currently assumes "everyone can fiddle with everything" in this block. >>>> >>>> Theo, can you draw memory map? >>> >>> It would be a mess. I've counted things up. The first 147 registers are >>> used in this 0x400 block. There are 31 individual blocks, with 7 >>> registers unused (holes to align next block). >>> >>> Functions are reset, clocks, LBIST, MBIST, DDR control, GPIO, >>> accelerator control, CPU entrypoint, PDTrace, IRQs, chip info & ID >>> stuff, control registers for PCIe / eMMC / Eth / SGMII / DMA / etc. >>> >>> Some will never get used from Linux, others might. Maybe a moderate >>> approach would be to create ressources for major blocks and make it >>> evolve organically, without imposing that all uses lead to a new >>> ressource creation. >>> >> >> That is usually how nodes are added to DT. If you modeled this >> system-controller space as a "simple-bus" instead of a "syscon" >> device, you could add nodes as you implement them. Rather than >> all at once as you have to by treating this space as one large >> blob device. > > I see where you are coming from, but in our case modeling our DT node as > a simple-bus would be lying about the hardware behind. There is no such > underlying bus. Let's try to keep the devicetree an abstraction > describing the hardware. Sure there is a bus, every register is on a bus, all these registers are memory mapped aren't they? "simple-bus" is just a logical grouping, it doesn't have to imply the bus is physically separate from the rest of the system bus. If you don't want these misc registers logically grouped then add them all as subnodes directly on the main SoC bus node. Calling that group of miscellaneous registers a "simple-mfd" device is even more incorrectly modeled IMHO. We have the same problem on our SoCs (hardware folks just love making miscellaneous junk drawer register spaces :D). And we decided to model it as a "syscon", "simple-mfd" too, how simple to just have all the other nodes point to this space with phandles and pull out whatever register they need. But that was a mistake we are still working to unwind. > > Also, we are having conflicts because multiple such child nodes are > being added at the same time as the base node. Once this initial series > is out (meaning dt-bindings for the OLB will exist) we'll be able to > add new nodes or ressources on a whim. > Not to this "system-controller" space you won't. If you keep it as a "simple-mfd","syscon" you will need to update the binding every time you add a new node. > Have you got an opinion on the approach described in this email? > https://lore.kernel.org/lkml/CYNRCGYA1PJ2.FYENLB4SRJWH@bootlin.com/ > Looks better to me, the nodes contain the registers they use which means you could simply add a ranges property to the parent and not need to use special accessors and offsets in the drivers too. Andrew > Thanks, > > -- > Théo Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On 25/01/2024 12:01, Théo Lebrun wrote: > Hello, > > On Thu Jan 25, 2024 at 8:51 AM CET, Krzysztof Kozlowski wrote: >> On 24/01/2024 16:14, Rob Herring wrote: >>>> + >>>> + pinctrl-b { >>>> + compatible = "mobileye,eyeq5-b-pinctrl"; >>>> + #pinctrl-cells = <1>; >>>> + }; >>>> + }; >>> >>> This can all be simplified to: >>> >>> system-controller@e00000 { >>> compatible = "mobileye,eyeq5-olb", "syscon"; >>> reg = <0xe00000 0x400>; >>> #reset-cells = <2>; >>> #clock-cells = <1>; >>> clocks = <&xtal>; >>> clock-names = "ref"; >>> >>> pins { ... }; >>> }; >>> >>> There is no need for sub nodes unless you have reusable blocks or each >>> block has its own resources in DT. >> >> Yes, however I believe there should be resources here: each subnode >> should get its address space. This is a bit tied to implementation, >> which currently assumes "everyone can fiddle with everything" in this block. >> >> Theo, can you draw memory map? > > It would be a mess. I've counted things up. The first 147 registers are > used in this 0x400 block. There are 31 individual blocks, with 7 > registers unused (holes to align next block). Holes are not really a problem. > > Functions are reset, clocks, LBIST, MBIST, DDR control, GPIO, > accelerator control, CPU entrypoint, PDTrace, IRQs, chip info & ID > stuff, control registers for PCIe / eMMC / Eth / SGMII / DMA / etc. So they are within separate blocks or not? Best regards, Krzysztof
On 25/01/2024 12:40, Théo Lebrun wrote: > Hello, > > On Wed Jan 24, 2024 at 8:22 PM CET, Rob Herring wrote: >> On Wed, Jan 24, 2024 at 11:40 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote: >>> On Wed Jan 24, 2024 at 6:28 PM CET, Théo Lebrun wrote: >>>> On Wed Jan 24, 2024 at 4:14 PM CET, Rob Herring wrote: >>>>> On Tue, Jan 23, 2024 at 07:46:49PM +0100, Théo Lebrun wrote: > > [...] > >>>>>> + }; >>>>>> + >>>>>> + pinctrl-b { >>>>>> + compatible = "mobileye,eyeq5-b-pinctrl"; >>>>>> + #pinctrl-cells = <1>; >>>>>> + }; >>>>>> + }; >>>>> >>>>> This can all be simplified to: >>>>> >>>>> system-controller@e00000 { >>>>> compatible = "mobileye,eyeq5-olb", "syscon"; >>>>> reg = <0xe00000 0x400>; >>>>> #reset-cells = <2>; >>>>> #clock-cells = <1>; >>>>> clocks = <&xtal>; >>>>> clock-names = "ref"; >>>>> >>>>> pins { ... }; >>>>> }; >>>>> >>>>> There is no need for sub nodes unless you have reusable blocks or each >>>>> block has its own resources in DT. >>>> >>>> That is right, and it does simplify the devicetree as you have shown. >>>> However, the split nodes gives the following advantages: >>>> >>>> - Devicetree-wise, it allows for one alias per function. >>>> `clocks = <&clocks EQ5C_PLL_CPU>` is surely more intuitive >>>> than `clocks = <&olb EQ5C_PLL_CPU>;`. Same for reset. >> >> clocks: resets: pinctrl: system-controller@e00000 { >> >>>> >>>> - It means an MFD driver must be implemented, adding between 100 to 200 >>>> lines of boilerplate code to the kernel. >> >> From a binding perspective, not my problem... That's Linux details >> defining the binding. What about u-boot, BSD, future versions of Linux >> with different structure? >> >> I don't think an MFD is required here. A driver should be able to be >> both clock and reset provider. That's pretty common. pinctrl less so. > > @Rob & @Krzysztof: following Krzysztof's question about the memory map > and adding ressources to the system-controller, I was wondering if the > following approach would be more suitable: More or less (missing ranges, unit addresses, lower-case hex etc). > > olb: system-controller@e00000 { > compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd"; > reg = <0 0xe00000 0x0 0x400>; > #address-cells = <1>; > #size-cells = <1>; > > clocks: clock-controller { > compatible = "mobileye,eyeq5-clk"; > reg = <0x02c 0x7C>; > #clock-cells = <1>; > clocks = <&xtal>; > clock-names = "ref"; > }; > > reset: reset-controller { > compatible = "mobileye,eyeq5-reset"; > reg = <0x004 0x08>, <0x120 0x04>, <0x200 0x34>; > reg-names = "d0", "d2", "d1"; > #reset-cells = <2>; > }; > > pinctrl0: pinctrl-a { > compatible = "mobileye,eyeq5-a-pinctrl"; > reg = <0x0B0 0x30>; > }; > > pinctrl1: pinctrl-b { > compatible = "mobileye,eyeq5-b-pinctrl"; > reg = <0x0B0 0x30>; Duplicate reg? > }; > }; > > It highlights that they are in fact separate controllers and not one > device. The common thing between them is that they were > custom-implemented by Mobileye and therefore all registers were put in > a single block. > Best regards, Krzysztof
Hello, On Fri Jan 26, 2024 at 12:52 PM CET, Krzysztof Kozlowski wrote: > On 25/01/2024 12:40, Théo Lebrun wrote: > > Hello, > > > > On Wed Jan 24, 2024 at 8:22 PM CET, Rob Herring wrote: > >> On Wed, Jan 24, 2024 at 11:40 AM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > >>> On Wed Jan 24, 2024 at 6:28 PM CET, Théo Lebrun wrote: > >>>> On Wed Jan 24, 2024 at 4:14 PM CET, Rob Herring wrote: > >>>>> On Tue, Jan 23, 2024 at 07:46:49PM +0100, Théo Lebrun wrote: > > > > [...] > > > >>>>>> + }; > >>>>>> + > >>>>>> + pinctrl-b { > >>>>>> + compatible = "mobileye,eyeq5-b-pinctrl"; > >>>>>> + #pinctrl-cells = <1>; > >>>>>> + }; > >>>>>> + }; > >>>>> > >>>>> This can all be simplified to: > >>>>> > >>>>> system-controller@e00000 { > >>>>> compatible = "mobileye,eyeq5-olb", "syscon"; > >>>>> reg = <0xe00000 0x400>; > >>>>> #reset-cells = <2>; > >>>>> #clock-cells = <1>; > >>>>> clocks = <&xtal>; > >>>>> clock-names = "ref"; > >>>>> > >>>>> pins { ... }; > >>>>> }; > >>>>> > >>>>> There is no need for sub nodes unless you have reusable blocks or each > >>>>> block has its own resources in DT. > >>>> > >>>> That is right, and it does simplify the devicetree as you have shown. > >>>> However, the split nodes gives the following advantages: > >>>> > >>>> - Devicetree-wise, it allows for one alias per function. > >>>> `clocks = <&clocks EQ5C_PLL_CPU>` is surely more intuitive > >>>> than `clocks = <&olb EQ5C_PLL_CPU>;`. Same for reset. > >> > >> clocks: resets: pinctrl: system-controller@e00000 { > >> > >>>> > >>>> - It means an MFD driver must be implemented, adding between 100 to 200 > >>>> lines of boilerplate code to the kernel. > >> > >> From a binding perspective, not my problem... That's Linux details > >> defining the binding. What about u-boot, BSD, future versions of Linux > >> with different structure? > >> > >> I don't think an MFD is required here. A driver should be able to be > >> both clock and reset provider. That's pretty common. pinctrl less so. > > > > @Rob & @Krzysztof: following Krzysztof's question about the memory map > > and adding ressources to the system-controller, I was wondering if the > > following approach would be more suitable: > > More or less (missing ranges, unit addresses, lower-case hex etc). Yeah the details are not really on point, it was only a proposal highlighting a different way of dealing with the current situation. Looks like it is suitable to you. > > olb: system-controller@e00000 { > > compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd"; > > reg = <0 0xe00000 0x0 0x400>; > > #address-cells = <1>; > > #size-cells = <1>; > > > > clocks: clock-controller { > > compatible = "mobileye,eyeq5-clk"; > > reg = <0x02c 0x7C>; > > #clock-cells = <1>; > > clocks = <&xtal>; > > clock-names = "ref"; > > }; > > > > reset: reset-controller { > > compatible = "mobileye,eyeq5-reset"; > > reg = <0x004 0x08>, <0x120 0x04>, <0x200 0x34>; > > reg-names = "d0", "d2", "d1"; > > #reset-cells = <2>; > > }; > > > > pinctrl0: pinctrl-a { > > compatible = "mobileye,eyeq5-a-pinctrl"; > > reg = <0x0B0 0x30>; > > }; > > > > pinctrl1: pinctrl-b { > > compatible = "mobileye,eyeq5-b-pinctrl"; > > reg = <0x0B0 0x30>; > > Duplicate reg? Yes, the mapping is intertwined. Else it could be three ressources per pinctrl. Just really small ones. - 0xB0 mapping A - 0xB4 mapping B - 0xB8 - 0xBC - 0xC0 pull-down A - 0xC4 pull-up A - 0xC8 pull-down B - 0xCC pull-up B - 0xD0 drive-strength lo A - 0xD4 drive-strength hi A - 0xD8 drive-strength lo B - 0xDC drive-strength hi B 0xB8 is unrelated (I2C speed & SPI CS). 0xBC is a hole. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 26/01/2024 13:28, Théo Lebrun wrote: >>> >>> pinctrl0: pinctrl-a { >>> compatible = "mobileye,eyeq5-a-pinctrl"; >>> reg = <0x0B0 0x30>; >>> }; >>> >>> pinctrl1: pinctrl-b { >>> compatible = "mobileye,eyeq5-b-pinctrl"; >>> reg = <0x0B0 0x30>; >> >> Duplicate reg? > > Yes, the mapping is intertwined. Else it could be three ressources per > pinctrl. Just really small ones. > > - 0xB0 mapping A > - 0xB4 mapping B > - 0xB8 > - 0xBC > - 0xC0 pull-down A > - 0xC4 pull-up A > - 0xC8 pull-down B > - 0xCC pull-up B > - 0xD0 drive-strength lo A > - 0xD4 drive-strength hi A > - 0xD8 drive-strength lo B > - 0xDC drive-strength hi B > > 0xB8 is unrelated (I2C speed & SPI CS). 0xBC is a hole. Then maybe Rob's idea of one pinctrl device is better... Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml new file mode 100644 index 000000000000..031ef6a532c1 --- /dev/null +++ b/Documentation/devicetree/bindings/soc/mobileye/mobileye,eyeq5-olb.yaml @@ -0,0 +1,77 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/soc/mobileye/mobileye,eyeq5-olb.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Mobileye EyeQ5 SoC system controller + +maintainers: + - Grégory Clement <gregory.clement@bootlin.com> + - Théo Lebrun <theo.lebrun@bootlin.com> + - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com> + +description: + OLB ("Other Logic Block") is a hardware block grouping smaller blocks. Clocks, + resets, pinctrl are being handled from here. + +properties: + compatible: + items: + - const: mobileye,eyeq5-olb + - const: syscon + - const: simple-mfd + + reg: + maxItems: 1 + + clock-controller: + $ref: /schemas/clock/mobileye,eyeq5-clk.yaml# + type: object + + reset-controller: + $ref: /schemas/reset/mobileye,eyeq5-reset.yaml# + type: object + + pinctrl-a: + $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml# + type: object + + pinctrl-b: + $ref: /schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml# + type: object + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + system-controller@e00000 { + compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd"; + reg = <0xe00000 0x400>; + + clock-controller { + compatible = "mobileye,eyeq5-clk"; + #clock-cells = <1>; + clocks = <&xtal>; + clock-names = "ref"; + }; + + reset-controller { + compatible = "mobileye,eyeq5-reset"; + #reset-cells = <2>; + }; + + pinctrl-a { + compatible = "mobileye,eyeq5-a-pinctrl"; + #pinctrl-cells = <1>; + }; + + pinctrl-b { + compatible = "mobileye,eyeq5-b-pinctrl"; + #pinctrl-cells = <1>; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index cb431c79c7b8..fe1270e8c983 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14786,6 +14786,7 @@ M: Théo Lebrun <theo.lebrun@bootlin.com> L: linux-mips@vger.kernel.org S: Maintained 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
Add documentation to describe the "Other Logic Block" syscon. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- .../bindings/soc/mobileye/mobileye,eyeq5-olb.yaml | 77 ++++++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 78 insertions(+)