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

[Théo Lebrun]

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

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com