| Message ID | 20211210114222.26581-2-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dt-bindings: pinctrl: pins, groups & functions | expand |
On Fri, Dec 10, 2021 at 12:42 PM Rafał Miłecki <zajec5@gmail.com> wrote: > This binding change is meant to introduce a generic way of describing > pinctrl blocks details. Every pinmux block is expected to have: > 1. Named pins > 2. Named groups containing one or more pins > 3. Named functions referencing one or more groups > > It doesn't describe how hw should be programmed. That remains binding > and driver specific. So what this does is to take a large chunk of data that we known to be associated with the compatible string (names of pins, groups and functions, etc) and put it into the device tree instead of the alternative, which is what most drivers do, and that is to compile in the data into the operating system and just look it up by using a compatible string. The DT maintainers have already indicated that this is not desirable and I don't see it getting merged before it has a Reviewed-by tag from one of the DT binding maintainers. I think we need to know what the USP (unique selling point) is? Would it be something like not having to duplicate work across some boot loaders and operating systems? (Well they all need to parse this type of description but that can be put into a library.) Or something else? Yours, Linus Walleij
Rob: please kindly comment on this idea of storing pins/groups/functions in DT. For a sample Linux implementation you can check (incomplete): [PATCH V2 4/6] pinctrl: support reading pins, groups & functions from DT https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-5-zajec5@gmail.com/ For a real life DT usage you can check: [PATCH V2 6/6] ARM: dts: BCM5301X: add pinctrl pins, groups & functions https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-7-zajec5@gmail.com/ Also see below inline comments. On 11.12.2021 00:26, Linus Walleij wrote: > On Fri, Dec 10, 2021 at 12:42 PM Rafał Miłecki <zajec5@gmail.com> wrote: > >> This binding change is meant to introduce a generic way of describing >> pinctrl blocks details. Every pinmux block is expected to have: >> 1. Named pins >> 2. Named groups containing one or more pins >> 3. Named functions referencing one or more groups >> >> It doesn't describe how hw should be programmed. That remains binding >> and driver specific. > > So what this does is to take a large chunk of data that we known to be > associated with the compatible string (names of pins, groups and functions, > etc) and put it into the device tree instead of the alternative, which is > what most drivers do, and that is to compile in the data into the > operating system and just look it up by using a compatible > string. Correct. It changes the place of storing platform specific data. > The DT maintainers have already indicated that this is not desirable > and I don't see it getting merged before it has a Reviewed-by > tag from one of the DT binding maintainers. Tony pointed out that it was back in 2011. It's worth reconsidering. https://patchwork.ozlabs.org/comment/2786915/ Rob said it depends on whether "data be static (complete) and correct" https://patchwork.ozlabs.org/comment/2786688/ I find it absolutely required to get Rob's Reviewed-by before we merge it. I hope we can get Rob's opinion on this. > I think we need to know what the USP (unique selling point) is? > > Would it be something like not having to duplicate work across some > boot loaders and operating systems? (Well they all need to parse this > type of description but that can be put into a library.) > > Or something else? There are two reasons for me to work on this binding: 1. I think it's cleaner to keep pinctrl details in DT DT seems more natural (than C code) for: a) Translating info from datasheets b) Storing pin/group/function custom values c) Defining relations (phandles) d) Handling chip differences (adding new pins in newer revisions) Last time I learnt that pins don't always/usually have numbers (in datasheets) but are rather named. Still in the "pinctrl_pin_desc" we have "unsigned number" just to enumerate them and reference in groups. Adding or removing pins/groups/functions in DT is as simple as adding/deleting nodes. That also means less logic in C code. 2. It avoids data duplication It allows me to keep pins/groups/functions data in one place (DT) and use it in both: Linux and U-Boot. No duplication & easier maintenance.
On Sat, Dec 11, 2021 at 12:16:25PM +0100, Rafał Miłecki wrote: > Rob: please kindly comment on this idea of storing pins/groups/functions > in DT. I was never a fan of stuffing pin mux/ctrl into DT for what's mostly a one time stuffing of register values. And given how many things run before getting to the kernel, doing proper pin configuration in the kernel is much too late (or redundant because it was actually already done). > > For a sample Linux implementation you can check (incomplete): > [PATCH V2 4/6] pinctrl: support reading pins, groups & functions from DT > https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-5-zajec5@gmail.com/ > > For a real life DT usage you can check: > [PATCH V2 6/6] ARM: dts: BCM5301X: add pinctrl pins, groups & functions > https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-7-zajec5@gmail.com/ What about h/w with no concept of 'groups'? > Also see below inline comments. > > > On 11.12.2021 00:26, Linus Walleij wrote: > > On Fri, Dec 10, 2021 at 12:42 PM Rafał Miłecki <zajec5@gmail.com> wrote: > > > > > This binding change is meant to introduce a generic way of describing > > > pinctrl blocks details. Every pinmux block is expected to have: > > > 1. Named pins > > > 2. Named groups containing one or more pins > > > 3. Named functions referencing one or more groups > > > > > > It doesn't describe how hw should be programmed. That remains binding > > > and driver specific. > > > > So what this does is to take a large chunk of data that we known to be > > associated with the compatible string (names of pins, groups and functions, > > etc) and put it into the device tree instead of the alternative, which is > > what most drivers do, and that is to compile in the data into the > > operating system and just look it up by using a compatible > > string. > > Correct. It changes the place of storing platform specific data. > > > > The DT maintainers have already indicated that this is not desirable > > and I don't see it getting merged before it has a Reviewed-by > > tag from one of the DT binding maintainers. > > Tony pointed out that it was back in 2011. It's worth reconsidering. > https://patchwork.ozlabs.org/comment/2786915/ > > Rob said it depends on whether "data be static (complete) and correct" > https://patchwork.ozlabs.org/comment/2786688/ I haven't seen an answer for that question... That and working for multiple platforms (from different vendors) are the main things that matter to me. > I find it absolutely required to get Rob's Reviewed-by before we merge > it. I hope we can get Rob's opinion on this. > > > > I think we need to know what the USP (unique selling point) is? > > > > Would it be something like not having to duplicate work across some > > boot loaders and operating systems? (Well they all need to parse this > > type of description but that can be put into a library.) > > > > Or something else? > > There are two reasons for me to work on this binding: > > > 1. I think it's cleaner to keep pinctrl details in DT > > DT seems more natural (than C code) for: > a) Translating info from datasheets > b) Storing pin/group/function custom values > c) Defining relations (phandles) > d) Handling chip differences (adding new pins in newer revisions) > > Last time I learnt that pins don't always/usually have numbers (in > datasheets) but are rather named. Still in the "pinctrl_pin_desc" we > have "unsigned number" just to enumerate them and reference in groups. > > Adding or removing pins/groups/functions in DT is as simple as > adding/deleting nodes. That also means less logic in C code. > > > 2. It avoids data duplication > > It allows me to keep pins/groups/functions data in one place (DT) and > use it in both: Linux and U-Boot. No duplication & easier maintenance. >
On 14.12.2021 20:59, Rob Herring wrote: > On Sat, Dec 11, 2021 at 12:16:25PM +0100, Rafał Miłecki wrote: >> Rob: please kindly comment on this idea of storing pins/groups/functions >> in DT. > > I was never a fan of stuffing pin mux/ctrl into DT for what's mostly a > one time stuffing of register values. And given how many things run > before getting to the kernel, doing proper pin configuration in the > kernel is much too late (or redundant because it was actually already > done). OK, thanks for sharing that. Given a pretty limited optimism on this approach I'll simply drop it and do things the old good way. I thought it's a better desing but I probably was wrong. It was still worth a try :) Thanks to everyone involved in this discussion. >> For a sample Linux implementation you can check (incomplete): >> [PATCH V2 4/6] pinctrl: support reading pins, groups & functions from DT >> https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-5-zajec5@gmail.com/ >> >> For a real life DT usage you can check: >> [PATCH V2 6/6] ARM: dts: BCM5301X: add pinctrl pins, groups & functions >> https://patchwork.ozlabs.org/project/linux-gpio/patch/20211124230439.17531-7-zajec5@gmail.com/ > > What about h/w with no concept of 'groups'? It could probably be handled with sth like functions { bar { pins = <&foo>; } } but my binding didn't cover that indeed. >> Also see below inline comments. >> >> >> On 11.12.2021 00:26, Linus Walleij wrote: >>> On Fri, Dec 10, 2021 at 12:42 PM Rafał Miłecki <zajec5@gmail.com> wrote: >>> >>>> This binding change is meant to introduce a generic way of describing >>>> pinctrl blocks details. Every pinmux block is expected to have: >>>> 1. Named pins >>>> 2. Named groups containing one or more pins >>>> 3. Named functions referencing one or more groups >>>> >>>> It doesn't describe how hw should be programmed. That remains binding >>>> and driver specific. >>> >>> So what this does is to take a large chunk of data that we known to be >>> associated with the compatible string (names of pins, groups and functions, >>> etc) and put it into the device tree instead of the alternative, which is >>> what most drivers do, and that is to compile in the data into the >>> operating system and just look it up by using a compatible >>> string. >> >> Correct. It changes the place of storing platform specific data. >> >> >>> The DT maintainers have already indicated that this is not desirable >>> and I don't see it getting merged before it has a Reviewed-by >>> tag from one of the DT binding maintainers. >> >> Tony pointed out that it was back in 2011. It's worth reconsidering. >> https://patchwork.ozlabs.org/comment/2786915/ >> >> Rob said it depends on whether "data be static (complete) and correct" >> https://patchwork.ozlabs.org/comment/2786688/ > > I haven't seen an answer for that question... > > That and working for multiple platforms (from different vendors) are the > main things that matter to me. I thought my design description & BCM5301X DTS patch may be a proof of that but apparently it wasn't enough ;)
On 14.12.2021 21:10, Rafał Miłecki wrote: > On 14.12.2021 20:59, Rob Herring wrote: >> On Sat, Dec 11, 2021 at 12:16:25PM +0100, Rafał Miłecki wrote: >>> Rob: please kindly comment on this idea of storing pins/groups/functions >>> in DT. >> >> I was never a fan of stuffing pin mux/ctrl into DT for what's mostly a >> one time stuffing of register values. And given how many things run >> before getting to the kernel, doing proper pin configuration in the >> kernel is much too late (or redundant because it was actually already >> done). > > OK, thanks for sharing that. Given a pretty limited optimism on this > approach I'll simply drop it and do things the old good way. I feel I need to post one more comment though. *** What I find a really clean DT code for defining some BCM4908 groups: groups { led_0_grp { pins = <&pin0 3>; }; led_1_grp { pins = <&pin1 3>; }; nand_grp { pins = <&pin32 0>, <&pin33 0>, <&pin34 0>, <&pin43 0>, <&pin44 0>, <&pin45 0>, <&pin56 1>; }; }; *** Gets a bit cumbersome (for me) when using ANSI C structs. I remain unconvinced about ANSI C being a good place for storing such data. Maybe I'm just getting too old & grumpy ;) struct bcm4908_pinctrl_pin_setup { unsigned number; unsigned function; }; static const struct bcm4908_pinctrl_pin_setup led_0_pins[] = { { 0, 3 }, }; static const struct bcm4908_pinctrl_pin_setup led_1_pins[] = { { 0, 3 }, }; static const struct bcm4908_pinctrl_pin_setup nand_pins[] = { { 32, 0 }, { 33, 0 }, { 34, 0 }, { 43, 0 }, { 44, 0 }, { 45, 0 }, { 56, 1 }, }; struct bcm4908_pinctrl_grp { const char *name; const struct bcm4908_pinctrl_pin_setup *pins; const unsigned int num_pins; }; static const struct bcm4908_pinctrl_grp bcm4908_pinctrl_grps[] = { { "led_0_grp", led_0_pins, ARRAY_SIZE(led_0_pins) }, { "led_1_grp", led_1_pins, ARRAY_SIZE(led_1_pins) }, { "nand_grp", nand_pins, ARRAY_SIZE(nand_pins) }, };
diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml index d471563119a9..e36662cb1f3b 100644 --- a/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl.yaml @@ -42,4 +42,38 @@ properties: This property can be set either globally for the pin controller or in child nodes for individual pin group control. + pins: + type: object + + patternProperties: + "^.*$": + type: object + description: Pin named by node name + + groups: + type: object + + patternProperties: + "^.*$": + type: object + description: Group named by node name + + properties: + pins: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: Array of pins belonging to this group + + functions: + type: object + + patternProperties: + "^.*$": + type: object + description: Function named by node name + + properties: + groups: + $ref: /schemas/types.yaml#/definitions/phandle-array + description: Array of groups used by this function + additionalProperties: true