Received-SPF: pass (google.com: domain of linux-kernel+bounces-171624-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Tue, 07 May 2024 16:52:49 +0200
Message-Id: <D13HXGJGMS76.XIIIZLZBCZ09@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: "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>
In-Reply-To: <8dcdb1422cd144128c1dc6fff1c273d3.sboyd@kernel.org>

Hello,

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], supportin=
g
> > 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-single)=
.
> >=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?

Thanks,

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