[RFC,0/2] pinctrl: sunxi: Introduce DT-based pinctrl builder

  Hi,

since the dawn of time every Allwinner SoC dumped a rather large table
of data into the kernel, to describe the mapping between the pinctrl
function name and its mux value, for each pin.

This series introduces code that avoids that (for new SoCs), by instead
reading that information directly from the devicetree. We have per-pin
group nodes there anyway, and were just missing the mux value.

Compared to my previous effort almost exactly five years ago [1], this
new version drops the idea of describing the pinctrl data entirely in
the DT, instead it still relies on driver provided information for that.
That is more flexible, since it allows to introduce quirks and special
handling more cleanly, at the cost of still requiring a separate driver
file for each SoC. However this file is now very small, and can be
easily written and reviewed. All that is needed is the number of pins
per bank, plus information about each bank's IRQ capability.
Patch 2/2 shows an example, for the yet unsupported Allwinner V5 SoC.

On the DT side all that would be needed is *one* extra property per
pin group to announce the mux value:

	uart0_pb_pins: uart0-pb-pins {
		pins = "PB9", "PB10";
		function = "uart0";
		pinmux = <2>;
	};

The new code works by providing a function that builds the former
mapping table *at runtime*, by using both the driver provided
information, plus traversing all children of the pinctrl DT node, to
find all pin groups needed. This table looks the same as what we
hardcoded so far, so can easily be digested by the existing sunxi
pinctrl driver.

Please have a look and tell me whether this new approach has a better
future than my previous attempt.

Cheers,
Andre

[1] https://patchwork.ozlabs.org/project/linux-gpio/cover/20171113012523.2328-1-andre.przywara@arm.com/


Andre Przywara (2):
  pinctrl: sunxi: allow reading mux values from DT
  pinctrl: sunxi: Add support for the Allwinner V5 pin controller

 drivers/pinctrl/sunxi/Kconfig            |   5 +
 drivers/pinctrl/sunxi/Makefile           |   2 +
 drivers/pinctrl/sunxi/pinctrl-sun8i-v5.c |  52 ++++
 drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c | 355 +++++++++++++++++++++++
 drivers/pinctrl/sunxi/pinctrl-sunxi.h    |   8 +
 5 files changed, 422 insertions(+)
 create mode 100644 drivers/pinctrl/sunxi/pinctrl-sun8i-v5.c
 create mode 100644 drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c
  

On Thu, Nov 10, 2022 at 2:44 AM Andre Przywara <andre.przywara@arm.com> wrote:

> Compared to my previous effort almost exactly five years ago [1], this
> new version drops the idea of describing the pinctrl data entirely in
> the DT, instead it still relies on driver provided information for that.
(...)
> On the DT side all that would be needed is *one* extra property per
> pin group to announce the mux value:
>
>         uart0_pb_pins: uart0-pb-pins {
>                 pins = "PB9", "PB10";
>                 function = "uart0";
>                 pinmux = <2>;
>         };

So what you need to do is to convince the device tree people that this
is a good idea.

For me as linux maintainer it's no big deal, it's fine either way. The new
code looks elegant.

But from a DT point of view this needs to make sense also for Windows
and BSD, so that is who you have to convince. If it is possible to derive
the same information from the compatible string (like today) that will
need an extended argument why all operating systems will benefit from
this.

Yours,
Linus Walleij
  

On Thu, 10 Nov 2022 11:21:02 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

Hi Linus,

thanks for having a look!

> On Thu, Nov 10, 2022 at 2:44 AM Andre Przywara <andre.przywara@arm.com> wrote:
> 
> > Compared to my previous effort almost exactly five years ago [1], this
> > new version drops the idea of describing the pinctrl data entirely in
> > the DT, instead it still relies on driver provided information for that.  
> (...)
> > On the DT side all that would be needed is *one* extra property per
> > pin group to announce the mux value:
> >
> >         uart0_pb_pins: uart0-pb-pins {
> >                 pins = "PB9", "PB10";
> >                 function = "uart0";
> >                 pinmux = <2>;
> >         };  
> 
> So what you need to do is to convince the device tree people that this
> is a good idea.
> 
> For me as linux maintainer it's no big deal, it's fine either way. The new
> code looks elegant.
> 
> But from a DT point of view this needs to make sense also for Windows
> and BSD, so that is who you have to convince. If it is possible to derive
> the same information from the compatible string (like today) that will
> need an extended argument why all operating systems will benefit from
> this.

This is actually an argument in favour of it: at the moment *every* OS
(or DT user) has to carry some form of this table[1]. For U-Boot this is a
major pain, for instance, and we came up with some minimal and
simplified version of that (assuming one pinmux per function name,
ignoring different mappings in different ports: [2]), but we are already
touching its limits.
And I don't think this DT argument counts anyway: we already store a much
bigger chunk of "information" in the DT, namely the function name. This has
no technical meaning, really, other than to map this to a 4-bit value
internally. I don't know why we have an information like "UART0 is using
the 'uart0' pin group" in the DT, but refuse to put the actual
hardware information in there. We could possibly even get rid of the
string, and derive this from the node name, if we need some human readable
identifier.

And just to make sure: I don't propose to change this for existing DTs,
it's just for new SoCs going forward. Allwinner at the moment spins out
many SoCs with only little differences, but all require this largish
table, since the pin assignments are the ones that differ.

Cheers,
Andre

[1]
https://github.com/freebsd/freebsd-src/blob/main/sys/arm/allwinner/a64/a64_padconf.c
[2]
https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pinctrl/sunxi/pinctrl-sunxi.c#L587-605
  

On Thu, Nov 10, 2022 at 12:33 PM Andre Przywara <andre.przywara@arm.com> wrote:

> This is actually an argument in favour of it: at the moment *every* OS
> (or DT user) has to carry some form of this table[1]. For U-Boot this is a
> major pain, for instance, and we came up with some minimal and
> simplified version of that (assuming one pinmux per function name,
> ignoring different mappings in different ports: [2]), but we are already
> touching its limits.

That's a compelling argument.

I don't know about reusing the existing pinmux property in a new way
which differs from the standard one:

  pinmux:
    description:
      The list of numeric pin ids and their mux settings that properties in the
      node apply to (either this, "pins" or "groups" have to be specified)
    $ref: /schemas/types.yaml#/definitions/uint32-array

You should rather invent something like "sunxi,pin-mux-val" or so.

That makes me happy to merge it at least, I don't see any problem
with it.

> And I don't think this DT argument counts anyway: we already store a much
> bigger chunk of "information" in the DT, namely the function name. This has
> no technical meaning, really, other than to map this to a 4-bit value
> internally. I don't know why we have an information like "UART0 is using
> the 'uart0' pin group" in the DT, but refuse to put the actual
> hardware information in there. We could possibly even get rid of the
> string, and derive this from the node name, if we need some human readable
> identifier.

These exist for consistency and maintenance, they are established
standard bindings:
Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml

Associating a function "uart0" with something like
[ "uart0-txrx", uart0-rtscts" ] by a line such as:

groups = "uart0-txrx", "uart0-rtscts";

is simple to understand, and makes it easier for maintainers who have
to look at a lot of different platforms with different muxes. So these
are there for human readability.

I.e. the goal of standard properties is not to minimize amount of
information (which is your goal here) but to structure things in a way that
makes maintenance easier by being similar on several platforms.

Some systems deviate from standard properties, and admittedly
this way of structuring things with strings is maybe not the ultimate.
The most deviating one is:
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
which is used by OMAP and HiSilicon.
Some feel this should never have been merged.
But it was merged.
By me.

There is also the pinmux property above which some systems use for
putting in enumerated magic. That's part of the standard but
makes the standard somewhat incoherent.

This difference in pin control DT patterns is because we could
not come up with something that was acceptable for all and the
result of some diplomacy when the subsystem was created around
2011-2012.

The thing is that I am not a very consistent and stubborn person
and I think more along the lines of the IETF motto "rough consensus
and running code". The DT people were different back in 2011,
and also softer around the edges, not as strict and not insisting on
things being done one way for all systems. Their ambitions have
stepped up since. So these are the people you need to convince.

I suggest you propose the bindings with the patch set by Cc
devicetree@vger.kernel.org using the custom "sunxi,pin" or whatever
name you want and see if the DT reviewers agree with this solution.

If they say nothing in like 2 weeks I usually merge things that I
find non-objectionable anyways.

Yours,
Linus Walleij