Message-ID: <CAGb2v64sNfytUPbyBFjiZCU+jLkXOH1r9iH=Z_E0PjwVxkMCWg@mail.gmail.com>
Date: Fri, 17 Jan 2025 23:43:05 +0800
From: Chen-Yu Tsai <wens@...e.org>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Andre Przywara <andre.przywara@....com>, 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>, 
	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, Jan 16, 2025 at 5:34 PM Linus Walleij <linus.walleij@...aro.org> wrote:
>
> Hi Andre,
>
> some nice talk here, actually the following is just opinions, I will
> be likely happy with whatever approach is taken eventually.
>
> 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.

No. Allwinner actually names their pins like this, kind of like Rockchip
which provides standardized names such as "GPIO0_B2", but unlike MediaTek
which just names the pins after their designated primary function such as
"EINT22" or "TDM_BCLK". The ball names are a separate thing.

Even though the pin names seem a bit random, there's actually a grouping
logic underneath:

- PC pins are always for the bootable internal storage (NAND, SPI-NOR, eMMC)
- PF pins are always the external SD card / debug UART / JTAG

> > > 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.
>
> > 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.
>
> > > 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.
>
> Well, unless this way of doing things becomes so prevalent that
> it's the new black.
>
> > 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.
>
> > 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.
>
> > - 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.
>
> 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...)
>
> > - 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?
> It seems common, but yeah it can be confusing.

No. i2c0 is the actual peripheral that gets muxed in. The SDA / SCL
comments are simply denoting which signal from the peripheral this
pin & mux value carry.

Powered by blists - more mailing lists