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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 26 Jan 2024 12:52:47 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Théo Lebrun <theo.lebrun@...tlin.com>,
 Rob Herring <robh@...nel.org>
Cc: Gregory CLEMENT <gregory.clement@...tlin.com>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
 Linus Walleij <linus.walleij@...aro.org>, Rafał Miłecki
 <rafal@...ecki.pl>, Philipp Zabel <p.zabel@...gutronix.de>,
 Vladimir Kondratiev <vladimir.kondratiev@...ileye.com>,
 linux-mips@...r.kernel.org, linux-clk@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 Tawfik Bayouk <tawfik.bayouk@...ileye.com>, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB system
 controller

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@...tlin.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@...000 {
>>>>>     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@...000 {
>>
>>>>
>>>>  - 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@...000 {
> 		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