Re: [RFC PATCH 06/11] dt-bindings: soc: microchip: document the two simple-mfd syscons on PolarFire SoC

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3869 bytes --]

On Thu, Aug 15, 2024 at 02:00:03PM -0600, Rob Herring wrote:
> On Thu, Aug 15, 2024 at 03:01:09PM +0100, Conor Dooley wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> > 
> > There are two syscons on PolarFire SoC that provide various functionality of
> > use to the OS.
> > 
> > The first of these is the "control-scb" region, that contains the "tvs"
> > temperature and voltage sensors and the control/status registers for the
> > system controller's mailbox. The mailbox has a dedicated node, so
> > there's no need for a child node describing it, looking the syscon up by
> > compatible is sufficient.
> > 
> > The second, "mss-top-sysreg", contains clocks, pinctrl, resets, and
> > interrupt controller and more. For this RFC, only the reset controller
> > child is described as that's all that is described by the existing
> > bindings. The clock controller already has a dedicated node, and will
> > retain it as there are other clock regions, so like the mailbox,
> > a compatible-based lookup of the syscon is sufficient to keep the clock
> > driver working as before so no child is needed.
> 
> I'm confused. The reset controller is reused from somewhere else?

There's already a driver for it on this device, but probed via the
auxiliary bus, and the #reset-cells property is in the clock controller
node. The only devices that use this driver are the various different
logic element SKUs (which all share a compatible, they're identical as
far as an OS is concerned) and an upcoming SoC that is effectively a
zero logic element SKU.

> I 
> thought you didn't expect any reuse of the IP happening.

> If a child node 
> makes it possible to enable the h/w without any s/w changes, then that 
> is a compelling argument for having a child node.

No, in both cases there'd be software changes - they're just simpler
with a child node.

> 
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> > (I'll split this in two later, it's just easier when I have the same
> > questions about both...)
> > 
> > Are these things entitled to have child nodes for the reset and sensor
> > nodes, or should the properties be in the parent and the OS probe the
> > drivers for the functions? That's something that, despite supposedly
> > being a maintainer, I do not understand the rules (of thumb?) for.
> 
> Besides the is it an independent, reusable IP block test, my test 
> generally is do the child nodes have their own DT resources? Say 
> you have phy registers mixed in some syscon and clocks which only go to 
> the phy. Then a child node with "clocks" makes sense. If your only 
> property is #phy-cells, then a child node doesn't make sense. Of course 
> you could reach different conclusions based on the completeness of the 
> binding.

AFAIK, none of these things are consumers of resources like that, other
than the interrupt controller, which has an interrupts property. I think
that could justify a child node (and I think a dedicated binding,
because it is a confusing irq mux that that kernel doesn't appear to
have anything else similar to).

> > Secondly, is it okay to make the "pragmatic" decision to not have a
> > child clock node and keep routing the clocks via the existing & retained
> > clock node (and therefore not update the various clocks nodes in the
> > consumers)? Doing so would require a lot more hocus pocus with the clock
> > driver than this series does, as the same driver would no longer be
> > suitable for the before/after bindings.
> 
> In the 2 cases here, I don't think you need child nodes. I would expect 
> pinctrl to have one though if only as a container for all the pinctrl 
> child nodes.

Good to know for when that gets written. Hopefully not by me, I have
enough messes to sort out as is!

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]