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

From: Théo Lebrun
Date: Tue May 07 2024 - 11:14:37 EST


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