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

[Krzysztof Kozlowski]

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