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

[Théo Lebrun]

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