Message-ID: <20250117145228.2fc8a64e@donnerap.manchester.arm.com>
Date: Fri, 17 Jan 2025 14:52:28 +0000
From: Andre Przywara <andre.przywara@....com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Andras Szemzo <szemzo.andras@...il.com>, Michael Turquette
 <mturquette@...libre.com>, Stephen Boyd <sboyd@...nel.org>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>, Jernej Skrabec
 <jernej.skrabec@...il.com>, Samuel Holland <samuel@...lland.org>, Philipp
 Zabel <p.zabel@...gutronix.de>, Maxime Ripard <mripard@...nel.org>, Vinod
 Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>, Ulf
 Hansson <ulf.hansson@...aro.org>, Paul Walmsley <paul.walmsley@...ive.com>,
 Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, Uwe
 Kleine-König <u.kleine-koenig@...libre.com>, Florian
 Fainelli <florian.fainelli@...adcom.com>, linux-clk@...r.kernel.org,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org,
 linux-phy@...ts.infradead.org, linux-gpio@...r.kernel.org,
 linux-pm@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH 03/12] pinctrl: sunxi: add driver for Allwinner V853.

On Thu, 16 Jan 2025 10:34:26 +0100
Linus Walleij <linus.walleij@...aro.org> wrote:

Hi Linus,

> some nice talk here, actually the following is just opinions, I will
> be likely happy with whatever approach is taken eventually.

it's indeed an intriguing discussion, made me think a bit ...
And thanks for describing your maintainer's view, that's definitely
helpful.

> On Wed, Jan 15, 2025 at 4:26 PM Andre Przywara <andre.przywara@....com> wrote:
> 
> > > pio: pinctrl@...0800 {
> > >                         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.  
> 
> the pins in this example are called PB0 and PB1 though. The designation
> on the package. And often pins actually named "i2c0_1" "i2c0_2" are
> for that primary function, but muxable to a few other functions,
> at least GPIO in most cases. So it's just some name for the pin
> really.

Sure, there is information in the form that marks those two pins for
being usable as I2C pins. But I feel like the function parameter is not
living up to its promise (see below).

> > > 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.  
> 
> You have a point, and when working with a system the application
> engineer often finds bugs in the pin control driver, and has to go
> and fix the actual driver and then all the information hiding and
> simplification is moot.
> 
> This can become an expensive lesson for the current attempts
> to push pin control into firmware where the configuration is
> mostly "dead simple" (and just using strings) - the bugs will be
> in the firmware instead, and impossible or really hard to fix.

... in that old embedded world where firmware is an opaque closed source
unfixable blob. Mainline Allwinner is all "organic": we have upstreamed
U-Boot and TF-A support, actively maintained, plus an Open Source
management firmware. Updating the firmware is a matter of "dd" or
"flashcp" - at least in theory. And I'd rather think of the kernel being
not easy to change - think Debian installer image, or getting it upstream
first, then backported, then picked up by a distro.
But I think this discussion slightly digresses here - at least following it
would double the length of this email ;-)

> > 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.  
> 
> Yeah the ambition with the strings was to avoid forcing application
> engineers to know all about that. If they do, they are then
> developing the driver, not just using it.

I'd argue that an application engineer just has to choose the right
pingroup DT node:
	pinctrl-0 = <&i2c2_pb_pins>;
In the worst case they go ahead and ask their hardware buddy, and create
a new pingroup node. In my experience the typical Allwinner users are more
on the hardware side, so are no strangers to looking up data sheets.

> > > 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?  
> 
> That's what STM32 does as well and it's usable.
> 
> But of course it drives a truck through the initial ambition that pins
> on all systems be configured the same way, with strings. So now
> there are some families of drivers all "necessarily different" which
> is not so nice for people jumping between different SoCs, but
> very compelling for people focusing on just one SoC.

Ah, I think I get your point.

> Well, unless this way of doing things becomes so prevalent that
> it's the new black.

That is actually a good argument: At the moment I am happy with my
proposal (the allwinner,pinmux = <number>; property), but that seems like
standard #15 then.
So would biting the bullet and adopting the Apple/STM32 way then be more
sustainable?
On the other hand: the allwinner,pinmux solution has the advantage of being
already written and proven working, also it stays very close to the
existing description/binding - so implementations like U-Boot could just
keep on using the "function" string.

I am a bit torn here... I don't think I will find the solitude to
implement this "Apple" approach in the next few weeks.

> > 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.  
> 
> And a new standard, to add to the other standards, so that
> is my problem as maintainer. It makes sense on its own, and it
> complicates the bigger picture.

I see your point - xkcd 927.

> > 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...  
> 
> That's true.
> 
> This comes from the DT ambition to describe hardware and config,
> but not *define* hardware, i.e. to stop device tree to turn into
> Verilog or SystemC, which is what will happen if we take the
> 1:1 reflection of hardware to device tree too far.
> 
> I don't think anyone really knows where to cut the line.

That's true, and I learned to step back from this ambition - it's
tempting, but indeed out of scope.
One thing to consider, though: the function names are effectively part of
the binding, since they must match exactly between the driver and DT - it's
not just a name. But they are documented nowhere. Names like "i2c0" or
"uart2" seem obvious, but there are ambiguities like "twi" vs "i2c" (the
former used in the vendor BSP), or "owa" vs "spdif" (same situation).
Also, is it "emac" or "emac0"? For some SoCs we discover a second
interface later: A523 just exposes one EMAC, but the A527 package (same
die) provides the pins for the second MAC as well. So I am afraid strings
are not as unique or straight-forward as we hope for.

So for creating a new pingroup, an application engineer would need to
check the Linux driver (or U-Boot? Or FreeBSD?) for the name to use - that
sounds somewhat wrong to me.
In the new model they just create a unique function name and enter the
pinmux from the datasheet - I think that's not too much to ask?

> > - 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.  
> 
> This is a generic problem though, look at GPU drivers.

Yes, but for GPUs there is good rationale (because they are big
complicated pieces of hardware), and they are easily put into modules.
Pinctrl on the other hand is essential for *any* device to come up
(UART!), so having them in the image makes things much easier. Also
Allwinner pinctrl devices are not nearly as sexy as a Radeon or Geforce ;-)

> The community (especially Android) seem set on fixing this by using
> modules.
> 
> > - 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.  
> 
> I like this argument the best.
> 
> However this also reads "upfront firmware to handle pin control is a
> dead end" yet there are people dedicatedly working on exactly that.
> (Not that its' the Allwinner developers' problem...)

I see, but I feel it's an orthogonal problem, and relies on the a
pessimistic firmware view (see above).
Having those gory SoC specific details in firmware (which is by
definition device specific) solves one big problem, though: there is a
chance that existing kernels would already run on new SoCs/boards.
But again: different discussion thread, I guess, and not what I ask for.

> > - 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.  
> 
> So i2c0 is SDA and i2c1 is SCL or something?

No, sorry, I cut out too much, it looks like this:

	SUNXI_PIN(SUNXI_PINCTRL_PIN(I, 5),
		  SUNXI_FUNCTION(0x5, "i2c0"),		/* SCK */
		...
        SUNXI_PIN(SUNXI_PINCTRL_PIN(I, 6),
		  SUNXI_FUNCTION(0x5, "i2c0"),		/* SDA */
		...
        SUNXI_PIN(SUNXI_PINCTRL_PIN(I, 7),
		  SUNXI_FUNCTION(0x5, "i2c1"),		/* SCK */
		...
        SUNXI_PIN(SUNXI_PINCTRL_PIN(I, 8),
		  SUNXI_FUNCTION(0x5, "i2c1"),		/* SDA */

So the function just selects the group, and the actual pin name is
completely irrelevant to the software side - hence the comment.
But this "put something not meant for the human reader into a comment"
typically points to something not being quite right.

Cheers,
Andre

Powered by blists - more mailing lists