Received-SPF: pass (google.com: domain of linux-kernel+bounces-40093-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Fri, 26 Jan 2024 13:28:29 +0100
Message-Id: <CYOMZE0XIEIR.7Q1BDZCKX1E@bootlin.com>
To: "Krzysztof Kozlowski" <krzysztof.kozlowski@linaro.org>, "Rob Herring"
 <robh@kernel.org>
From: =?utf-8?q?Th=C3=A9o_Lebrun?= <theo.lebrun@bootlin.com>
Subject: Re: [PATCH v3 04/17] dt-bindings: soc: mobileye: add EyeQ5 OLB
 system controller
Cc: "Gregory CLEMENT" <gregory.clement@bootlin.com>, "Michael Turquette"
 <mturquette@baylibre.com>, "Stephen Boyd" <sboyd@kernel.org>, "Krzysztof
 Kozlowski" <krzysztof.kozlowski+dt@linaro.org>, "Conor Dooley"
 <conor+dt@kernel.org>, "Thomas Bogendoerfer" <tsbogend@alpha.franken.de>,
 "Linus Walleij" <linus.walleij@linaro.org>,
 =?utf-8?q?Rafa=C5=82_Mi=C5=82ecki?= <rafal@milecki.pl>, "Philipp Zabel"
 <p.zabel@pengutronix.de>, "Vladimir Kondratiev"
 <vladimir.kondratiev@mobileye.com>, <linux-mips@vger.kernel.org>,
 <linux-clk@vger.kernel.org>, <devicetree@vger.kernel.org>,
 <linux-kernel@vger.kernel.org>, "Thomas Petazzoni"
 <thomas.petazzoni@bootlin.com>, "Tawfik Bayouk"
 <tawfik.bayouk@mobileye.com>, <linux-gpio@vger.kernel.org>
References: <20240123-mbly-clk-v3-0-392b010b8281@bootlin.com>
 <20240123-mbly-clk-v3-4-392b010b8281@bootlin.com>
 <20240124151405.GA930997-robh@kernel.org>
 <CYN43TSPPPZ5.1VUA1CH95D8KJ@bootlin.com>
 <CYN4D0Z6600X.20W9VWX4BGNXX@bootlin.com>
 <CAL_JsqKHPdmafDvKCHZTNNzRAzq2Y34b2dqUXQD6WpE7z2k-jA@mail.gmail.com>
 <CYNRCGYA1PJ2.FYENLB4SRJWH@bootlin.com>
 <8054e01d-0a1e-45b6-b62a-25303e8f4593@linaro.org>
In-Reply-To: <8054e01d-0a1e-45b6-b62a-25303e8f4593@linaro.org>

Hello,

On Fri Jan 26, 2024 at 12:52 PM CET, Krzysztof Kozlowski wrote:
> On 25/01/2024 12:40, Th=C3=A9o Lebrun wrote:
> > Hello,
> >=20
> > On Wed Jan 24, 2024 at 8:22 PM CET, Rob Herring wrote:
> >> On Wed, Jan 24, 2024 at 11:40=E2=80=AFAM Th=C3=A9o Lebrun <theo.lebrun=
@bootlin.com> wrote:
> >>> On Wed Jan 24, 2024 at 6:28 PM CET, Th=C3=A9o 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=C3=A9o Lebrun wrote:
> >=20
> > [...]
> >=20
> >>>>>> +      };
> >>>>>> +
> >>>>>> +      pinctrl-b {
> >>>>>> +        compatible =3D "mobileye,eyeq5-b-pinctrl";
> >>>>>> +        #pinctrl-cells =3D <1>;
> >>>>>> +      };
> >>>>>> +    };
> >>>>>
> >>>>> This can all be simplified to:
> >>>>>
> >>>>> system-controller@e00000 {
> >>>>>     compatible =3D "mobileye,eyeq5-olb", "syscon";
> >>>>>     reg =3D <0xe00000 0x400>;
> >>>>>     #reset-cells =3D <2>;
> >>>>>     #clock-cells =3D <1>;
> >>>>>     clocks =3D <&xtal>;
> >>>>>     clock-names =3D "ref";
> >>>>>
> >>>>>     pins { ... };
> >>>>> };
> >>>>>
> >>>>> There is no need for sub nodes unless you have reusable blocks or e=
ach
> >>>>> 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 =3D <&clocks EQ5C_PLL_CPU>` is surely more intuitive
> >>>>    than `clocks =3D <&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.
> >=20
> > @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).

Yeah the details are not really on point, it was only a proposal
highlighting a different way of dealing with the current situation.
Looks like it is suitable to you.

> > 	olb: system-controller@e00000 {
> > 		compatible =3D "mobileye,eyeq5-olb", "syscon", "simple-mfd";
> > 		reg =3D <0 0xe00000 0x0 0x400>;
> > 		#address-cells =3D <1>;
> > 		#size-cells =3D <1>;
> >=20
> > 		clocks: clock-controller {
> > 			compatible =3D "mobileye,eyeq5-clk";
> > 			reg =3D <0x02c 0x7C>;
> > 			#clock-cells =3D <1>;
> > 			clocks =3D <&xtal>;
> > 			clock-names =3D "ref";
> > 		};
> >=20
> > 		reset: reset-controller {
> > 			compatible =3D "mobileye,eyeq5-reset";
> > 			reg =3D <0x004 0x08>, <0x120 0x04>, <0x200 0x34>;
> > 			reg-names =3D "d0", "d2", "d1";
> > 			#reset-cells =3D <2>;
> > 		};
> >=20
> > 		pinctrl0: pinctrl-a {
> > 			compatible =3D "mobileye,eyeq5-a-pinctrl";
> > 			reg =3D <0x0B0 0x30>;
> > 		};
> >=20
> > 		pinctrl1: pinctrl-b {
> > 			compatible =3D "mobileye,eyeq5-b-pinctrl";
> > 			reg =3D <0x0B0 0x30>;
>
> Duplicate reg?

Yes, the mapping is intertwined. Else it could be three ressources per
pinctrl. Just really small ones.

 - 0xB0 mapping   A
 - 0xB4 mapping   B
 - 0xB8
 - 0xBC
 - 0xC0 pull-down A
 - 0xC4 pull-up   A
 - 0xC8 pull-down B
 - 0xCC pull-up   B
 - 0xD0 drive-strength lo A
 - 0xD4 drive-strength hi A
 - 0xD8 drive-strength lo B
 - 0xDC drive-strength hi B

0xB8 is unrelated (I2C speed & SPI CS). 0xBC is a hole.

Thanks,

--
Th=C3=A9o Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com