linux-kernel - Re: [PATCH v2 00/11] Add Mobileye EyeQ system controller support (clk, reset, pinctrl)

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]

Message-Id: <D13IE06GUEJ9.UUNOU4QH2QN9@bootlin.com>
Date: Tue, 07 May 2024 17:14:25 +0200
From: Théo Lebrun <theo.lebrun@...tlin.com>
To: Théo Lebrun <theo.lebrun@...tlin.com>, "Stephen Boyd"
 <sboyd@...nel.org>, "Conor Dooley" <conor+dt@...nel.org>, "Greg
 Kroah-Hartman" <gregkh@...uxfoundation.org>, "Krzysztof Kozlowski"
 <krzysztof.kozlowski+dt@...aro.org>, "Lee Jones" <lee@...nel.org>, "Linus
 Walleij" <linus.walleij@...aro.org>, "Michael Turquette"
 <mturquette@...libre.com>, "Philipp Zabel" <p.zabel@...gutronix.de>,
 "Rafael J. Wysocki" <rafael@...nel.org>, "Rob Herring" <robh@...nel.org>,
 "Thomas Bogendoerfer" <tsbogend@...ha.franken.de>
Cc: <linux-mips@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <linux-clk@...r.kernel.org>,
 <linux-gpio@...r.kernel.org>, "Vladimir Kondratiev"
 <vladimir.kondratiev@...ileye.com>, "Gregory CLEMENT"
 <gregory.clement@...tlin.com>, "Thomas Petazzoni"
 <thomas.petazzoni@...tlin.com>, "Tawfik Bayouk"
 <tawfik.bayouk@...ileye.com>
Subject: Re: [PATCH v2 00/11] Add Mobileye EyeQ system controller support
 (clk, reset, pinctrl)

Hello,

On Tue May 7, 2024 at 4:52 PM CEST, Théo Lebrun wrote:
> On Sat May 4, 2024 at 4:34 AM CEST, Stephen Boyd wrote:
> > Quoting Théo Lebrun (2024-05-03 07:20:45)
> > > This builds on previous EyeQ5 system-controller revisions[0], supporting
> > > 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).
> > > 
> > > 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.
> > > 
> > > 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éo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com