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).
>
> 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
