| Message ID | 20221110014255.20711-1-andre.przywara@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | pinctrl: sunxi: Introduce DT-based pinctrl builder | expand |
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
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