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

["András Szemző" <szemzo.andras@gmail.com>]

I’ve actually already converted the pinctrl driver to the new dt based pinmux, as Andre
suggested. It’s simple, get rid of those huge pinmux tables, and easy to understand to
add the mux settings to the dtsi.


> On 16 Jan 2025, at 10:34, Linus Walleij <linus.walleij@linaro.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@arm.com> wrote:
> 
>>> 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.
> 
> 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.
> 
>>> 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.
> 
> Yours,
> Linus Walleij