Re: [PATCH 03/12] pinctrl: sunxi: add driver for Allwinner V853.

[Andre Przywara <andre.przywara@arm.com>]

On Wed, 15 Jan 2025 11:23:50 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

Hi Linus,

thanks for the reply, I was hoping to get some insight and discuss this!

> On Tue, Jan 14, 2025 at 3:20 PM Andre Przywara <andre.przywara@arm.com> wrote:
> > Andras Szemzo <szemzo.andras@gmail.com> wrote:  
> 
> > > The V853 family has multiple package variants, from BGA to QFN88.
> > > The latter has co-packaged DRAM and fewer pins, and less features (pin muxes).
> > > All family members can be supported by a single driver, as the available pins
> > > with allowed muxes is the same across the devices.  
> >
> > It depends a bit on the outcome of the discussion on the A523 pinctrl
> > driver [1], but I think we should use the same approach here (and for
> > every "new" Allwinner SoC coming up, really): put the pinmux value in the
> > DT, and get rid of this entire table altogether:
> > [1]
> >
> > The SoC specific pinctrl driver would then be very small ([2]), so this
> > pinctrl support patch here would actually become much smaller.
> >
> > Just feel a bit sorry for you having created this table, in a tedious and
> > eye-straining exercise - been there, done that ;-)  
> 
> It's pretty stressful for the pin control maintainer as well.
> 
> From the subsystems point of view, groups matches to functions by
> strings is the best. ("fun1") + ("group1", "group2"):
> 
> pio: pinctrl@1c20800 {
>                         compatible = "allwinner,sun8i-r40-pinctrl";
> (...)
>                         i2c0_pins: i2c0-pins {
>                                 pins = "PB0", "PB1";
>                                 function = "i2c0";
>                         };
> 
> abstract, strings, nice. The driver handles the particulars.

What bugs me about this it that this has quite some seemingly redundant
information (Who would have thought that the i2c0 pins use function
"i2c0"?), but misses out on the actual 4 bits(!) of information.
So the A523 version [1] just *adds* this one property:
		allwinner,pinmux = <2>;

[1]https://lore.kernel.org/linux-sunxi/20241111005750.13071-5-andre.przywara@arm.com/

And we keep all your beloved strings ;-)

> That is like so because we are designing for users which are
> let's say customization engineers. If these engineers jump from
> project to project matching function strings to group strings will
> be a common way to set up pins, and easy to understand and
> grasp, and it makes the DTS very readable.

That's an interesting view, and I see the point of it being easy to read,
but this is partly because it doesn't convey too much actual information,
does it, as it requires another lookup or two.
And the pinctrl group nodes are actually in the .dtsi file, which are
typically written once during the initial SoC enablement, and new board
.dts files normally just reference the existing pingroup nodes. So anyone
dealing with just a new board is not bothered by this.
Also in my experience most people have no problems in understanding the
concept of pinmuxing and that there is a selector number, also where to
find this.

> Then there are the engineers creating the pin control drivers,
> and they want everything to be convinient for *them*, and they
> think an opaque hex digit in the DTS is perfect at times, thus
> pinmux = <0xdeadbeef>;
> 
> Mediatek and STM32 made a compromise by using pinmux
> and adding some macros to define them so it looks more
> pleasant:
> 
>       i2c0_pins_a: i2c0-default {
>                 pins-i2c0 {
>                         pinmux = <MT7623_PIN_75_SDA0_FUNC_SDA0>,
>                                  <MT7623_PIN_76_SCL0_FUNC_SCL0>;

Well, I don't really get why they don't use the (MTK_PIN_NO(75) | 1)
definition directly, seems to be more telling to me?
So the plan for sunxi would be: <SUNXI_PINMUX(PORTC, 23, MUX_1)>, ...
And this would not be really "opaque", since it has a fixed known mapping:
	(port << 16) | (pin << 8) | (mux << 0))
I find this both technically elegant, because it combines all the
information into just one compact cell, but also readable by outsiders,
thanks to the macro.

But please note that using the generic pinmux is just an idea at this
point, last time I checked this would require significant rework in the
sunxi pinctrl driver.
Just adding the "allwinner,pinmux" property on the other hand is a
comparably easy addition, hence my patch, as a compromise.

>                         bias-disable;
>                 };
>         };
> 
> At least the bias control is using strings, this is nice.
> 
> So I'm mostly fine with that as well, but it can be pretty
> heavy on people coming from the outside, asking us questions
> like "on MT7689 how do you mux pin nnnn to function yyy"???
> Well I don't know? Some MT7689_PIN* macro I guess?

MTK_PIN_NO(nnn, yyy)?

> If it was just strings I would know what the
> expected behaviour and looks would be at least, then the driver
> could be buggy or missing things but that's clearly cut. That's
> why I prefer the strings.

My main arguments against the current (string-based) approach:
- They require the mapping table to be in every DT user, so not only the
  Linux kernel, but also U-Boot, FreeBSD, you name it...
https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pinctrl/sunxi/pinctrl-sunxi.c?ref_type=heads#L236-768
https://cgit.freebsd.org/src/tree/sys/arm/allwinner/h6/h6_padconf.c
- The tables are getting quite large, and they pollute the single image
  Linux kernel, with tons of very specific information for a number of very
  pitiful Allwinner SoCs. At the moment the tally is at 145KB of code+data
  for the existing arm64 SoCs, with the newer SoCs ever growing (H616 alone
  is 27KB, A523 would be quite larger even, I guess 40K). The new A523
  specific pinctrl support adds 872 Bytes.
- Most of the mappings are untested at pinctrl driver commit time, since we
  don't have the device drivers ready yet - by a margin. The new approach
  would add the pinmux values when we need them and can test them.
- The comments in the table give away that something is not quite right:
                  SUNXI_FUNCTION(0x2, "i2c0")),         /* SDA */
  This is just a comment, so has no relevance for the code, but it's not
  meant for humans either. Yet we try to make this correct and maintain
  it. Odd.

Cheers,
Andre