diff mbox series

[v3,04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system controller

Message ID 20240123-mbly-clk-v3-4-392b010b8281@bootlin.com
State New
Headers show
Series Add support for Mobileye EyeQ5 system controller | expand

Commit Message

Théo Lebrun Jan. 23, 2024, 6:46 p.m. UTC
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(+)

Comments

Rob Herring (Arm) Jan. 24, 2024, 7:22 p.m. UTC | #1
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.
Andrew Davis Jan. 25, 2024, 2:33 p.m. UTC | #2
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
>
Théo Lebrun Jan. 25, 2024, 2:49 p.m. UTC | #3
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
Andrew Davis Jan. 25, 2024, 3:11 p.m. UTC | #4
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
Krzysztof Kozlowski Jan. 26, 2024, 11:51 a.m. UTC | #5
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
Krzysztof Kozlowski Jan. 26, 2024, 11:52 a.m. UTC | #6
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
Théo Lebrun Jan. 26, 2024, 12:28 p.m. UTC | #7
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
Krzysztof Kozlowski Jan. 26, 2024, 2:54 p.m. UTC | #8
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 mbox series

Patch

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