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@xxxxxxxxxxx> 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