Received-SPF: pass (google.com: domain of linux-kernel+bounces-171669-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: Tue, 07 May 2024 17:14:25 +0200
Message-Id: <D13IE06GUEJ9.UUNOU4QH2QN9@bootlin.com>
From: =?utf-8?q?Th=C3=A9o_Lebrun?= <theo.lebrun@bootlin.com>
Subject: Re: [PATCH v2 00/11] Add Mobileye EyeQ system controller support
 (clk, reset, pinctrl)
Cc: <linux-mips@vger.kernel.org>, <devicetree@vger.kernel.org>,
 <linux-kernel@vger.kernel.org>, <linux-clk@vger.kernel.org>,
 <linux-gpio@vger.kernel.org>, "Vladimir Kondratiev"
 <vladimir.kondratiev@mobileye.com>, "Gregory CLEMENT"
 <gregory.clement@bootlin.com>, "Thomas Petazzoni"
 <thomas.petazzoni@bootlin.com>, "Tawfik Bayouk"
 <tawfik.bayouk@mobileye.com>
To: =?utf-8?q?Th=C3=A9o_Lebrun?= <theo.lebrun@bootlin.com>, "Stephen Boyd"
 <sboyd@kernel.org>, "Conor Dooley" <conor+dt@kernel.org>, "Greg
 Kroah-Hartman" <gregkh@linuxfoundation.org>, "Krzysztof Kozlowski"
 <krzysztof.kozlowski+dt@linaro.org>, "Lee Jones" <lee@kernel.org>, "Linus
 Walleij" <linus.walleij@linaro.org>, "Michael Turquette"
 <mturquette@baylibre.com>, "Philipp Zabel" <p.zabel@pengutronix.de>,
 "Rafael J. Wysocki" <rafael@kernel.org>, "Rob Herring" <robh@kernel.org>,
 "Thomas Bogendoerfer" <tsbogend@alpha.franken.de>
References: <20240503-mbly-olb-v2-0-95ce5a1e18fe@bootlin.com>
 <8dcdb1422cd144128c1dc6fff1c273d3.sboyd@kernel.org>
 <D13HXGJGMS76.XIIIZLZBCZ09@bootlin.com>
In-Reply-To: <D13HXGJGMS76.XIIIZLZBCZ09@bootlin.com>

Hello,

On Tue May 7, 2024 at 4:52 PM CEST, Th=C3=A9o Lebrun wrote:
> On Sat May 4, 2024 at 4:34 AM CEST, Stephen Boyd wrote:
> > Quoting Th=C3=A9o Lebrun (2024-05-03 07:20:45)
> > > This builds on previous EyeQ5 system-controller revisions[0], support=
ing
> > > EyeQ5, EyeQ6L and EyeQ6H. We expose a few OLB system-controller
> > > features here:
> > >  - Clocks: some read-only PLLs derived from main crystal and some
> > >    divider clocks based on PLLs.
> > >  - Resets.
> > >  - Pin controller, only on EyeQ5 (rest will use generic pinctrl-singl=
e).
> > >=20
> > > EyeQ6H is special in that it has seven instances of this
> > > system-controller. Those are spread around and cannot be seen as a
> > > single device, hence are exposed as seven DT nodes and seven
> > > compatibles.
> > >=20
> > > This revision differs from previous in that it exposes all devices as=
 a
> > > single DT node. Driver-wise, a MFD registers multiple cells for each
> > > device. Each driver is still in isolation from one another, each in
> > > their respective subsystem.
> >
> > Why can't you use auxiliary device and driver APIs?
>
> Good question. Reasons I see:
>
>  - I didn't know about auxdev beforehand. I discussed the rework with a
>    few colleagues and none mentioned it either.
>
>  - It feels simpler to let each device access iomem resources. From my
>    understanding, an auxdev is supposed to make function calls to its
>    parent without inheriting iomem access. That sounds like it will put
>    the register logic/knowledge inside a single driver, which could or
>    could not be a better option.
>
>    Implementing a function like this feels like cheating:
>       int olb_read(struct device *dev, u32 offset, u32 *val);
>
>    With an MFD, we hand over a part of the iomem resource to each child
>    and they deal with it however they like.
>
>  - Syscon is what I picked to share parts of OLB to other devices that
>    need it. Currently that is only for I2C speed mode but other devices
>    have wrapping-related registers. MFD and syscon are deeply connected
>    so an MFD felt natural.
>
>  - That would require picking one device that is platform driver, the
>    rest being all aux devices. Clock driver appears to be the one, same
>    as two existing mpfs and starfive-jh7110 that use auxdev for clk and
>    reset.
>
> Main reason I see for picking auxdev is that it forces devices to
> interact with a defined internal API. That can lead to nicer
> abstractions rather than inheriting resources as is being done in MFD.
>
> Are there other reasons?

Self replying myself. I gave myself some time to think about that but I
still have more thought now that I've written the previous email, and
re-read almost all old revisions of this series.

I do like this auxdev proposal. More so than current MFD revision. One
really nice feature is that it centralises access to iomem. I've
noticed recently a register that has most its fields for reset but one
lost bit dealing with a clock mux. Logic to handle that would be in one
location.

Also, I just noticed you hinted at auxiliary devices in previous emails,
which I thought was a generic term. I did not see it as a specific
kernel infrastructure to be used. Sorry about that.

Regards,

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