| Message ID | 20171113012523.2328-1-andre.przywara@arm.com |
|---|---|
| Headers | show |
| Series | pinctrl: sunxi: Add DT-based generic pinctrl driver | expand |
On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote: > so far the pinctrl driver for supporting a particular Allwinner SoC requires > a hardcoded table in the kernel code. (...) > This series here moves the data encoded in the table so far into the DT itself, > removing the need for a hardcoded table in the kernel. (...) The DT maintainers have been pretty clear on that they don't like using the the DT as a generic fit-all information dump. They prefer to look up hardware data from per-soc compatible strings. I have been sceptic about it too, on the grounds that I think it make configuration and multiplatform kernels easy, while making debugging and reading code+device tree hard, also affecting maintenance cost. I'd like to have Maxime's buy-in before we progress and also some discussion on these themes in general. > The approach taken here is to parse the DT and generate the table with > the help of additional properties, then hand this table over to the existing > driver. This is covered by three basic extensions to the DT binding: I adressed this in the bindings patch. > The benefit of this series is two-fold: > - Future SoCs don't need an in-kernel table anymore. They can describe > everything in the DT, It can be debated whether that is really a good thing or actually a bad thing for the reasons stated above. Also an additional bad thing is inconsistency between different SoCs. What we have in the kernel for all-DT is pinctrl-single.c. This exists for the case where there is exactly one register per pin and all you have is strange macro files from the ASIC people that noone understands. OMAP and HiSilicon is using this. It's a compromise, I'm not super-happy with that driver at all times but it is for a very strongly specified case. Would it be possible for you guys to simply use/embrace/extend pinctrl-single.c if you want to go this route? Any higher order of complexity than "one register per pin" I think is better handled by open coding it than trying to push things into the device tree, because of readability, debuggability and maintenance issues. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 24/11/17 10:28, Linus Walleij wrote: > On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote: > >> so far the pinctrl driver for supporting a particular Allwinner SoC requires >> a hardcoded table in the kernel code. > (...) >> This series here moves the data encoded in the table so far into the DT itself, >> removing the need for a hardcoded table in the kernel. > (...) > > The DT maintainers have been pretty clear on that they don't like > using the the DT as a generic fit-all information dump. They > prefer to look up hardware data from per-soc compatible strings. > > I have been sceptic about it too, on the grounds that I think it > make configuration and multiplatform kernels easy, while making > debugging and reading code+device tree hard, also affecting > maintenance cost. > > I'd like to have Maxime's buy-in before we progress and also some > discussion on these themes in general. Indeed this was one of the goals of this series. I can understand were we came from and that we had those in-kernel tables in the past. I am just a bit worried that with Allwinner recently playing the SKU game we end up with tons of tables for only slightly different SoCs (see the H3 and H5, for instance). And with single image kernels we pile up quite some *data* in each kernel, which is of little interest for everyone else. Also my understanding is that the actual Allwinner pin controller IP (register map) is very much the same across all SoCs. Mostly the only difference is the mapping between pins and mux functions, which we express in the DT already anyway (in the subnodes). And this is really a poster book example of what DT should be doing: express the specific mappings of a particular implementation. I don't see why this would need to be per-board only, if we can pull this up to the SoC level. >> The approach taken here is to parse the DT and generate the table with >> the help of additional properties, then hand this table over to the existing >> driver. This is covered by three basic extensions to the DT binding: > > I adressed this in the bindings patch. > >> The benefit of this series is two-fold: >> - Future SoCs don't need an in-kernel table anymore. They can describe >> everything in the DT, > > It can be debated whether that is really a good thing or actually > a bad thing for the reasons stated above. The point is that we already rely on the DT anyway, just that we use a *string* to specify a certain mux function. The in-kernel table just maps this to an actual SoC-specific register value. I think the GPIO subsystem needs the name, hence I am just adding the pinmux property. > Also an additional bad thing is inconsistency between different > SoCs. Well, does that mean that we should never change anything? Consistency is nice, but should not be an excuse for not improving things. And I tried to embrace the existing driver by designing this on top of it. > What we have in the kernel for all-DT is pinctrl-single.c. > > This exists for the case where there is exactly one register per > pin and all you have is strange macro files from the ASIC people > that noone understands. OMAP and HiSilicon is using this. > It's a compromise, I'm not super-happy with that driver at all times > but it is for a very strongly specified case. > > Would it be possible for you guys to simply use/embrace/extend > pinctrl-single.c if you want to go this route? Thanks for the pointer, I will have a look. > Any higher order of complexity than "one register per pin" I think > is better handled by open coding it than trying to push things into > the device tree, because of readability, debuggability and maintenance > issues. I don't see how this binding would make this worse. Cheers, Andre. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 24, 2017 at 1:05 PM, Andre Przywara <andre.przywara@arm.com> wrote: > On 24/11/17 10:28, Linus Walleij wrote: >> The DT maintainers have been pretty clear on that they don't like >> using the the DT as a generic fit-all information dump. They >> prefer to look up hardware data from per-soc compatible strings. (...) > I am just a bit worried that with Allwinner recently playing the SKU > game we end up with tons of tables for only slightly different SoCs (see > the H3 and H5, for instance). And with single image kernels we pile up > quite some *data* in each kernel, which is of little interest for > everyone else. So what you are saying is that you want to use the DTS for data dumping and what I'm saying is that the DT maintainers do not like that stance. They will have to speak on the issue directly before we continue I think. I have been getting a *LOT* of pushback to putting large amounts of data and configuration in the DTS recently, so IIUC that is something they simply don't like, probably for good reasons. C.f: https://www.spinics.net/lists/dri-devel/msg150321.html > Also my understanding is that the actual Allwinner pin controller IP > (register map) is very much the same across all SoCs. Mostly the only > difference is the mapping between pins and mux functions, which we > express in the DT already anyway (in the subnodes). And this is really a > poster book example of what DT should be doing: express the specific > mappings of a particular implementation. I don't see why this would need > to be per-board only, if we can pull this up to the SoC level. It's not me you need to sell this point. You need to sell it to the DT maintainers. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 30/11/17 15:20, Linus Walleij wrote: > On Fri, Nov 24, 2017 at 1:05 PM, Andre Przywara <andre.przywara@arm.com> wrote: >> On 24/11/17 10:28, Linus Walleij wrote: > >>> The DT maintainers have been pretty clear on that they don't like >>> using the the DT as a generic fit-all information dump. They >>> prefer to look up hardware data from per-soc compatible strings. > (...) >> I am just a bit worried that with Allwinner recently playing the SKU >> game we end up with tons of tables for only slightly different SoCs (see >> the H3 and H5, for instance). And with single image kernels we pile up >> quite some *data* in each kernel, which is of little interest for >> everyone else. > > So what you are saying is that you want to use the DTS for > data dumping Well, that's what the DT is for, right? As opposed to dump the data in *every* kernel (Linux, U-Boot, BSD) for *every* SoC. And, following this argument, why do we have reg and interrupt properties if we could derive them from the compatible string as well? Those are SoC specific and immutable by a board as well. It seems the discussion went slightly heated and astray, but basically I am after two things, maybe we should separate them: 1) Put the actual mux value into the DT, next to the already existing function property. That is a small addition, but has the great effect of avoiding to hard code this information for *each* supported SoC in *every* kernel. Doing so just sounds like bonkers to me, sorry. If there is really so much resistance against also *using* this information from Linux, I would be happy to at least add the already existing and generic "pinmux" property to the binding. This way we could add those values to the .dtsi(s), and other OSes could use them. No need for a driver change in Linux, then. I am not buying this molly guard argument at all, but this way we could keep this protection in the kernel. This would immediately allow to get an easy DT based pinctrl driver on the road for U-Boot. 2) Add the (very few!) properties to the pinctrl root node that we need to size and enumerate the pins. This would allow generic pinctrl driver support, so one thing less to worry when bringing up a new SoC (variant). So my main goal is more about the binding and the DTs. Linux could just ignore them, though I don't see a good reason for us to not make use of those for good. I believe that both are not really a big issue and I don't really understand why there is so much opposition towards it. > and what I'm saying is that the DT maintainers > do not like that stance. > > They will have to speak on the issue directly before we continue > I think. Fair enough. > I have been getting a *LOT* of pushback to putting large amounts > of data and configuration in the DTS recently, so IIUC that is something > they simply don't like, probably for good reasons. That's a shame, maybe it's to limit the amount of review and maintenance needed? And it's a safe bet to have a simple binding, where nothing can go wrong? And since we can't predict the future? I don't know, but in this case we have quite a sample of already existing controllers and can make an much more educated guess for a better and compatible binding, kind of in hindsight. > C.f: > https://www.spinics.net/lists/dri-devel/msg150321.html > >> Also my understanding is that the actual Allwinner pin controller IP >> (register map) is very much the same across all SoCs. Mostly the only >> difference is the mapping between pins and mux functions, which we >> express in the DT already anyway (in the subnodes). And this is really a >> poster book example of what DT should be doing: express the specific >> mappings of a particular implementation. I don't see why this would need >> to be per-board only, if we can pull this up to the SoC level. > > It's not me you need to sell this point. > > You need to sell it to the DT maintainers. Well, so far I only got some push back from Thierry, Maxime and you. My understanding of the DT maintainers' position is to leave those details to the subsystem maintainers. Cheers, Andre. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, so far the pinctrl driver for supporting a particular Allwinner SoC requires a hardcoded table in the kernel code. This table (for instance [1]) lists all pins and puts names to each special function each pin can have, along with the respective mux value to put into the hardware registers. That looks like: SUNXI_PIN(SUNXI_PINCTRL_PIN(G, 1), SUNXI_FUNCTION(0x0, "gpio_in"), SUNXI_FUNCTION(0x1, "gpio_out"), SUNXI_FUNCTION(0x2, "mmc1"), /* CMD */ SUNXI_FUNCTION_IRQ_BANK(0x6, 1, 1)), /* EINT1 */ SUNXI_PIN(SUNXI_PINCTRL_PIN(G, 2), .... On top of that we have the DT, which groups the pins and refers to the function to use *by name*: mmc1_pins: mmc1-pins { pins = "PG0", "PG1", "PG2", "PG3", "PG4", "PG5"; function = "mmc1"; drive-strength = <30>; bias-pull-up; }; This series here moves the data encoded in the table so far into the DT itself, removing the need for a hardcoded table in the kernel. The approach taken here is to parse the DT and generate the table with the help of additional properties, then hand this table over to the existing driver. This is covered by three basic extensions to the DT binding: - allwinner,gpio-pins = <[nr of PA pins] [nr of PB pins] ...>; This tells the driver how many pins each port has (0 is possible as well). The sum of all of them is used to size the array. Also the pin names are deduced from it and generated. Each pin gets an entry for GPIO in and out, using mux 0 and 1, respectively. - allwinner,irq-pin-map = <[IRQ port] [1st IRQ pin] [GPIO port] [1st GPIO pin] [mux value] [length]> Maps IRQ pins to their associated GPIO pins, describing the pins that can actually trigger interrupts. There can be multiple of these maps, each consisting of those six values. Every IRQ capable pin in those ports gets assigned an additional function "irq" (see the SUNXI_FUNCTION_IRQ_BANK line above). - pinmux = <[mux value] ...>; This property (in the pin group subnodes) tells the driver which mux value to actually write into the hardware registers upon selecting this function. For almost every group this mux value is the same for every pin, so we fill remaining entries with the last entry in that property, if this property has less members than the number of pins in this group. For the A64, for instance, this looks like this: pio: pinctrl@1c20800 { compatible = "allwinner,sun50i-a64-pinctrl", "allwinner,sunxi-pinctrl"; reg = <0x01c20800 0x400>; ... /* No Port A, PB0..PB9, PC0..PC16, PD0..PD24, ... */ allwinner,gpio-pins = <0 10 17 25 18 7 14 12>; /* The three ports B, G and H can trigger interrupts. */ allwinner,irq-ports = <0 0 1 0 6 10>, <1 0 6 0 6 14>, <2 0 7 0 6 12>; ... mmc1_pins: mmc1-pins { pins = "PG0", "PG1", "PG2", "PG3", "PG4", "PG5"; function = "mmc1"; pinmux = <2>; drive-strength = <30>; bias-pull-up; }; ... The benefit of this series is two-fold: - Future SoCs don't need an in-kernel table anymore. They can describe everything in the DT, and use the generic compatible as a fallback: compatible = "allwinner,sun50i-h6-pinctrl", "allwinner,sunxi-pinctrl"; So this driver should be the last file added to this directory ever. Of course we can't remove the existing tables, to keep compatiblity with older DTs, but at least we don't need to add more tables in the future. The Allwinner H6 will be the first user of this driver. - New DT consumers (other than the Linux kernel) could force usage of the new binding, if that's feasible for them. They would not need to add any SoC specific data into their code, instead have a generic driver that reads all information from the DT. A prominent example is U-Boot, which at the moment has *some* pinctrl data hardcoded, but wants to move to at DT based driver. Instead of copying the kernel tables and blowing up the code, we can add the new properties to U-Boot's DT copy and keep the code clean. Please note that the new binding is indeed just an extension, the existing driver just ignores those new properties and uses the in-kernel table, thus working as before, without any restrictions. So we can as well add those new properties to the kernel DT copy, which makes it easier for other users to re-use the DT files. The extended DT would add the generic compatible as a fallback, but keep the existing compatible name in the first place. So existing kernel drivers would match on the first string and use the table. U-Boot on the other hand would not match on the specific string, but recognize the generic name and pull the information from the DT. This allows the very same DT to be used by both drivers (or both bindings), triggered by the generic compatible. Please let me know your opinion on this. The approach to generate the table from the DT and re-using the existing driver leaves some room for optimization. But I found it far easier to not touch the actual driver, since we need it to stay around to support the old binding anyway. If it seems worth the come up with a separate pinctrl driver which just supports the new binding and makes the DT a first class citizen, let me know - and give me some time to fully understand the inner workings of the pinctrl and GPIO subsystem then. Cheers, Andre. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/sunxi/pinctrl-sun50i-a64.c Andre Przywara (3): dt-bindings: pinctrl: sunxi: document new generic binding pinctrl: sunxi: introduce DT-based generic driver arm64: dts: allwinner: enhance A64 .dtsi with new pinctrl binding .../bindings/pinctrl/allwinner,sunxi-pinctrl.txt | 58 +++ arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 27 +- drivers/pinctrl/sunxi/Makefile | 1 + drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c | 412 +++++++++++++++++++++ 4 files changed, 496 insertions(+), 2 deletions(-) create mode 100644 drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c