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

[Andrew Davis <afd@ti.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