| Message ID | 20240123-mbly-clk-v3-4-392b010b8281@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Mobileye EyeQ5 system controller | expand |
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 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 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
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
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(+)