Message-ID: <20251009-amendable-trimming-da31551d730b@spud>
Date: Thu, 9 Oct 2025 16:55:50 +0100
From: Conor Dooley <conor@...nel.org>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Conor Dooley <conor.dooley@...rochip.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [RFC 0/5] microchip mpfs/pic64gx pinctrl questions

On Wed, Oct 01, 2025 at 05:15:07PM +0100, Conor Dooley wrote:
> On Wed, Oct 01, 2025 at 01:29:01PM +0200, Linus Walleij wrote:
> > Hi Conor,
> > 
> > thanks for your patches!
> > 
> > looking at the drivers it appears to be trying extensively to make use
> > of the pinmux = <>; property to mux entire groups of pins.
> > 
> > pinmux = <nn>; is supposed to mux *one* pin per group, not entire
> > groups of pins from one property. See
> > Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml:
> > 
> >   The pinmux property accepts an array of pinmux groups, each of them describing
> >   a single pin multiplexing configuration.
> > 
> >   pincontroller {
> >     state_0_node_a {
> >       pinmux = <PINMUX_GROUP>, <PINMUX_GROUP>, ...;
> >     };
> >   };
> > 
> > So e.g. when you do this:
> > 
> >        spi0_mssio: spi0-mssio-pins {
> >          pinmux = <MPFS_PINFUNC(0, 0)>;
> >        };
> > 
> > We all know SPI uses more than one pin so this is clearly abusing
> > the pinmux property.
> > 
> > It is unfortunate that so many drivers now use this "mux one pin
> > individually" concept that we cannot see the diversity of pin
> > controllers.
> > 
> > I cannot recommend using the pinmux property for this SoC.
> > 
> > What you need to do is to define the actual pins and groups
> > that you have.
> > 
> > Look for example at
> > Documentation/devicetree/bindings/pinctrl/cortina,gemini-pinctrl.txt
> > drivers/pinctrl/pinctrl-gemini.c
> > arch/arm/boot/dts/gemini/gemini.dtsi
> > 
> > This is another SoC that muxes pins in groups, not in single per-pin
> > settings.
> 
> This looks like something that the "gpio2" stuff could definitely go to,
> since it covers multiple functions trying to access the same pin. Do you
> have an "approved" example for a more demultiplexer case, where the
> contention is about which of multiple possible pins (or pin analogues)
> an IO from a particular block must be routed to?
> 
> > Notice that the driver in this case enumerates and registers all 323
> > pins on the package! This is done because some of the groups
> > are mutually exclusive and this way the pin control framework
> > will do its job to detect collisions between pin groups and disallow
> > this, and that is what pin control is supposed to be doing.
> 
> In that case, the mutual exclusion would be that a function can only be
> routed to one "pin", but there's no concern about multiple functions
> being routed to any given "pin".

So, what I ended up doing is moving the "gpio2" stuff to use
functions/groups as your gemini stuff does, so each function contains
one group containing all the pins it needs - except for the gpio
function which contains analogues for each of the function's groups.
I'll send a patchset next week when the merge window is closed, but for
now that driver is here:
https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=pinctrl&id=8c0fdf0c76fe99549d293894121d64300dc4057f
Unlike "gpio2", where each pin supports two mutually exclusive
functions and so fits nicely into the pins/groups/functions hierarchy,
for iomux0 each function has two mutually exclusive routings but there's
no contention over the "pins", the corresponding function is either
routed there or it is entirely unused. I implemented this by co-opting
the pin structure to really contain functions, with each appearing in
two groups, one per routing. Each function then contains those two
groups - I think that then takes advantage of the framework's collision
detection like you requested? It's here in case you care enough to take
a look before I send a proper patchset next week:
https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=pinctrl&id=5aec232c11bd45262fb2cfaf7994e3030fd6d947

Cheers,
Conor.

> 
> > 
> > I.e. do not orient your design around which registers and settings
> > you have, and do not model your driver around that, instead
> > model the driver around which actual pins exist on the physical
> > component, how these are sorted into groups, how the groups
> > are related to function (such as the group of SPI pins being
> > related to the spi function) and define these pins, groups
> > and functions in your driver.
> > 
> > Yours,
> > Linus Walleij



Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists