| Message ID | 20241111005750.13071-6-andre.przywara@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: sunxi: Add Allwinner A523 support | expand |
On Mon, Nov 11, 2024 at 12:57:48AM +0000, Andre Przywara wrote: > The A523 contains a pin controller similar to previous SoCs, although > using 10 GPIO banks (PortB-PortK), all of them being IRQ capable. > This introduces a new style of binding, where the pinmux values for each > pin group is stored in the new "allwinner,pinmux" property in the DT > node, instead of requiring every driver to store a mapping between the > function names and the required pinmux. > > Add the new name to the list of compatible strings, and required it to > have 10 interrupts described. Also add the new pinmux property. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > .../pinctrl/allwinner,sun4i-a10-pinctrl.yaml | 23 +++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > index 4502405703145..6fc18e92e1e94 100644 > --- a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > +++ b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > @@ -56,6 +56,8 @@ properties: > - allwinner,sun50i-h6-r-pinctrl > - allwinner,sun50i-h616-pinctrl > - allwinner,sun50i-h616-r-pinctrl > + - allwinner,sun55i-a523-pinctrl > + - allwinner,sun55i-a523-r-pinctrl > - allwinner,suniv-f1c100s-pinctrl > - nextthing,gr8-pinctrl > > @@ -64,7 +66,7 @@ properties: > > interrupts: > minItems: 1 > - maxItems: 8 > + maxItems: 10 > description: > One interrupt per external interrupt bank supported on the > controller, sorted by bank number ascending order. > @@ -119,13 +121,17 @@ patternProperties: > $ref: /schemas/types.yaml#/definitions/uint32 > enum: [10, 20, 30, 40] > > + allwinner,pinmux: > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: pinmux selector for each pin Please add some constraints and/or description about what goes in the array. > + > required: > - pins > - function > > additionalProperties: false > > - "^vcc-p[a-ilm]-supply$": > + "^vcc-p[a-klm]-supply$": > description: > Power supplies for pin banks. > > @@ -156,6 +162,17 @@ allOf: > - interrupts > - interrupt-controller > > + - if: > + properties: > + compatible: > + enum: > + - allwinner,sun55i-a523-pinctrl > + > + then: > + properties: > + interrupts: > + minItems: 10 > + > - if: > properties: > compatible: > @@ -166,6 +183,7 @@ allOf: > properties: > interrupts: > minItems: 8 > + maxItems: 8 > > - if: > properties: > @@ -244,6 +262,7 @@ allOf: > - allwinner,sun8i-v3s-pinctrl > - allwinner,sun9i-a80-r-pinctrl > - allwinner,sun50i-h6-r-pinctrl > + - allwinner,sun55i-a523-r-pinctrl > > then: > properties: > -- > 2.46.2 >
On Mon, Nov 11, 2024 at 8:58 AM Andre Przywara <andre.przywara@arm.com> wrote: > > The A523 contains a pin controller similar to previous SoCs, although > using 10 GPIO banks (PortB-PortK), all of them being IRQ capable. > This introduces a new style of binding, where the pinmux values for each > pin group is stored in the new "allwinner,pinmux" property in the DT > node, instead of requiring every driver to store a mapping between the > function names and the required pinmux. > > Add the new name to the list of compatible strings, and required it to > have 10 interrupts described. Also add the new pinmux property. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > .../pinctrl/allwinner,sun4i-a10-pinctrl.yaml | 23 +++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > index 4502405703145..6fc18e92e1e94 100644 > --- a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > +++ b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > @@ -56,6 +56,8 @@ properties: > - allwinner,sun50i-h6-r-pinctrl > - allwinner,sun50i-h616-pinctrl > - allwinner,sun50i-h616-r-pinctrl > + - allwinner,sun55i-a523-pinctrl > + - allwinner,sun55i-a523-r-pinctrl > - allwinner,suniv-f1c100s-pinctrl > - nextthing,gr8-pinctrl > > @@ -64,7 +66,7 @@ properties: > > interrupts: > minItems: 1 > - maxItems: 8 > + maxItems: 10 > description: > One interrupt per external interrupt bank supported on the > controller, sorted by bank number ascending order. > @@ -119,13 +121,17 @@ patternProperties: > $ref: /schemas/types.yaml#/definitions/uint32 > enum: [10, 20, 30, 40] > > + allwinner,pinmux: > + $ref: /schemas/types.yaml#/definitions/uint32-array > + description: pinmux selector for each pin > + Why not just the standard "pinmux" property, as given in Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml > required: > - pins > - function This section should be made to apply only to the existing compatibles? Maybe we could just split the files and have a clean slate for sun55i? ChenYu > additionalProperties: false > > - "^vcc-p[a-ilm]-supply$": > + "^vcc-p[a-klm]-supply$": > description: > Power supplies for pin banks. > > @@ -156,6 +162,17 @@ allOf: > - interrupts > - interrupt-controller > > + - if: > + properties: > + compatible: > + enum: > + - allwinner,sun55i-a523-pinctrl > + > + then: > + properties: > + interrupts: > + minItems: 10 > + > - if: > properties: > compatible: > @@ -166,6 +183,7 @@ allOf: > properties: > interrupts: > minItems: 8 > + maxItems: 8 > > - if: > properties: > @@ -244,6 +262,7 @@ allOf: > - allwinner,sun8i-v3s-pinctrl > - allwinner,sun9i-a80-r-pinctrl > - allwinner,sun50i-h6-r-pinctrl > + - allwinner,sun55i-a523-r-pinctrl > > then: > properties: > -- > 2.46.2 >
On Wed, 13 Nov 2024 16:50:19 +0800 Chen-Yu Tsai <wens@csie.org> wrote: Hi Chen-Yu, sorry for the late reply, I was away for a week. > On Mon, Nov 11, 2024 at 8:58 AM Andre Przywara <andre.przywara@arm.com> wrote: > > > > The A523 contains a pin controller similar to previous SoCs, although > > using 10 GPIO banks (PortB-PortK), all of them being IRQ capable. > > This introduces a new style of binding, where the pinmux values for each > > pin group is stored in the new "allwinner,pinmux" property in the DT > > node, instead of requiring every driver to store a mapping between the > > function names and the required pinmux. > > > > Add the new name to the list of compatible strings, and required it to > > have 10 interrupts described. Also add the new pinmux property. > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > .../pinctrl/allwinner,sun4i-a10-pinctrl.yaml | 23 +++++++++++++++++-- > > 1 file changed, 21 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > > index 4502405703145..6fc18e92e1e94 100644 > > --- a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > > +++ b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > > @@ -56,6 +56,8 @@ properties: > > - allwinner,sun50i-h6-r-pinctrl > > - allwinner,sun50i-h616-pinctrl > > - allwinner,sun50i-h616-r-pinctrl > > + - allwinner,sun55i-a523-pinctrl > > + - allwinner,sun55i-a523-r-pinctrl > > - allwinner,suniv-f1c100s-pinctrl > > - nextthing,gr8-pinctrl > > > > @@ -64,7 +66,7 @@ properties: > > > > interrupts: > > minItems: 1 > > - maxItems: 8 > > + maxItems: 10 > > description: > > One interrupt per external interrupt bank supported on the > > controller, sorted by bank number ascending order. > > @@ -119,13 +121,17 @@ patternProperties: > > $ref: /schemas/types.yaml#/definitions/uint32 > > enum: [10, 20, 30, 40] > > > > + allwinner,pinmux: > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > + description: pinmux selector for each pin > > + > > Why not just the standard "pinmux" property, as given in > Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml I had it like this in my last post two years ago, but learned from LinusW [1] that the generic pinmux property has a slightly different meaning, and abusing it for just the pinmux index values would not match the generic definition. We *could* use the generic definition, but then this would include what's in the "pins" property, like I sketched out in the cover letter, as an alternative to this approach: pinmux = <SUNXI_PIN(PB, 9, 2)>, <SUNXI_PIN(PB, 10, 2)>; Where the SUNXI_PIN macro would combine the pin number and the pinmux into one 32-bit cell. See the Apple GPIO DT nodes for an example. This looks indeed nicer, but requires quite some rewrite of the existing pinctrl driver, AFAICS. [1] Previous reply from LinusW: https://lore.kernel.org/linux-sunxi/CACRpkdbMc-Q6wjgsiddu6-tWC1dt2uFk+4LyerMdgFk2KRGK4w@mail.gmail.com/ > > > required: > > - pins > > - function > > This section should be made to apply only to the existing > compatibles? Maybe we could just split the files and have > a clean slate for sun55i? Yeah, I couldn't find a good example how to make it *required* for one compatible and *not allowed* for all the others. But creating a whole new file is actually a good idea, as this also avoids adding another case to the already quite indented if-else cascade. Cheers, Andre > ChenYu > > > additionalProperties: false > > > > - "^vcc-p[a-ilm]-supply$": > > + "^vcc-p[a-klm]-supply$": > > description: > > Power supplies for pin banks. > > > > @@ -156,6 +162,17 @@ allOf: > > - interrupts > > - interrupt-controller > > > > + - if: > > + properties: > > + compatible: > > + enum: > > + - allwinner,sun55i-a523-pinctrl > > + > > + then: > > + properties: > > + interrupts: > > + minItems: 10 > > + > > - if: > > properties: > > compatible: > > @@ -166,6 +183,7 @@ allOf: > > properties: > > interrupts: > > minItems: 8 > > + maxItems: 8 > > > > - if: > > properties: > > @@ -244,6 +262,7 @@ allOf: > > - allwinner,sun8i-v3s-pinctrl > > - allwinner,sun9i-a80-r-pinctrl > > - allwinner,sun50i-h6-r-pinctrl > > + - allwinner,sun55i-a523-r-pinctrl > > > > then: > > properties: > > -- > > 2.46.2 > >
On Wed, Nov 20, 2024 at 6:12 PM Andre Przywara <andre.przywara@arm.com> wrote: > > On Wed, 13 Nov 2024 16:50:19 +0800 > Chen-Yu Tsai <wens@csie.org> wrote: > > Hi Chen-Yu, > > sorry for the late reply, I was away for a week. > > > On Mon, Nov 11, 2024 at 8:58 AM Andre Przywara <andre.przywara@arm.com> wrote: > > > > > > The A523 contains a pin controller similar to previous SoCs, although > > > using 10 GPIO banks (PortB-PortK), all of them being IRQ capable. > > > This introduces a new style of binding, where the pinmux values for each > > > pin group is stored in the new "allwinner,pinmux" property in the DT > > > node, instead of requiring every driver to store a mapping between the > > > function names and the required pinmux. > > > > > > Add the new name to the list of compatible strings, and required it to > > > have 10 interrupts described. Also add the new pinmux property. > > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > --- > > > .../pinctrl/allwinner,sun4i-a10-pinctrl.yaml | 23 +++++++++++++++++-- > > > 1 file changed, 21 insertions(+), 2 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > > > index 4502405703145..6fc18e92e1e94 100644 > > > --- a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > > > +++ b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > > > @@ -56,6 +56,8 @@ properties: > > > - allwinner,sun50i-h6-r-pinctrl > > > - allwinner,sun50i-h616-pinctrl > > > - allwinner,sun50i-h616-r-pinctrl > > > + - allwinner,sun55i-a523-pinctrl > > > + - allwinner,sun55i-a523-r-pinctrl > > > - allwinner,suniv-f1c100s-pinctrl > > > - nextthing,gr8-pinctrl > > > > > > @@ -64,7 +66,7 @@ properties: > > > > > > interrupts: > > > minItems: 1 > > > - maxItems: 8 > > > + maxItems: 10 > > > description: > > > One interrupt per external interrupt bank supported on the > > > controller, sorted by bank number ascending order. > > > @@ -119,13 +121,17 @@ patternProperties: > > > $ref: /schemas/types.yaml#/definitions/uint32 > > > enum: [10, 20, 30, 40] > > > > > > + allwinner,pinmux: > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > + description: pinmux selector for each pin > > > + > > > > Why not just the standard "pinmux" property, as given in > > Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml > > I had it like this in my last post two years ago, but learned from > LinusW [1] that the generic pinmux property has a slightly different > meaning, and abusing it for just the pinmux index values would not match > the generic definition. > We *could* use the generic definition, but then this would include what's > in the "pins" property, like I sketched out in the cover letter, as an > alternative to this approach: > pinmux = <SUNXI_PIN(PB, 9, 2)>, <SUNXI_PIN(PB, 10, 2)>; > Where the SUNXI_PIN macro would combine the pin number and the pinmux into > one 32-bit cell. See the Apple GPIO DT nodes for an example. > This looks indeed nicer, but requires quite some rewrite of the existing > pinctrl driver, AFAICS. Sorry for taking so long to get back to this. Could we maybe add a generic replacement of the existing "function" property, which takes a string? Like "function-id" or "function-selector" that takes u32 (or u8). Then it could be one or the other. Not sure if the binding maintainers would accept this or not. I understand that we probably don't want the mux value combined with the pin number. ChenYu > [1] Previous reply from LinusW: > https://lore.kernel.org/linux-sunxi/CACRpkdbMc-Q6wjgsiddu6-tWC1dt2uFk+4LyerMdgFk2KRGK4w@mail.gmail.com/ > > > > > > required: > > > - pins > > > - function > > > > This section should be made to apply only to the existing > > compatibles? Maybe we could just split the files and have > > a clean slate for sun55i? > > Yeah, I couldn't find a good example how to make it *required* for one > compatible and *not allowed* for all the others. But creating a whole new > file is actually a good idea, as this also avoids adding another case to > the already quite indented if-else cascade. > > Cheers, > Andre > > > ChenYu > > > > > additionalProperties: false > > > > > > - "^vcc-p[a-ilm]-supply$": > > > + "^vcc-p[a-klm]-supply$": > > > description: > > > Power supplies for pin banks. > > > > > > @@ -156,6 +162,17 @@ allOf: > > > - interrupts > > > - interrupt-controller > > > > > > + - if: > > > + properties: > > > + compatible: > > > + enum: > > > + - allwinner,sun55i-a523-pinctrl > > > + > > > + then: > > > + properties: > > > + interrupts: > > > + minItems: 10 > > > + > > > - if: > > > properties: > > > compatible: > > > @@ -166,6 +183,7 @@ allOf: > > > properties: > > > interrupts: > > > minItems: 8 > > > + maxItems: 8 > > > > > > - if: > > > properties: > > > @@ -244,6 +262,7 @@ allOf: > > > - allwinner,sun8i-v3s-pinctrl > > > - allwinner,sun9i-a80-r-pinctrl > > > - allwinner,sun50i-h6-r-pinctrl > > > + - allwinner,sun55i-a523-r-pinctrl > > > > > > then: > > > properties: > > > -- > > > 2.46.2 > > > >
On Tue, 14 Jan 2025 15:01:31 +0800 Chen-Yu Tsai <wens@csie.org> wrote: Hi Chen-Yu, before I get to your specific question below: what do you think in general of the idea of getting rid of that table based approach we use so far? Is that something worthwhile? I definitely think yes, but wanted to hear the maintainers' opinion about this. Happy to present some arguments if need be. ... > On Wed, Nov 20, 2024 at 6:12 PM Andre Przywara <andre.przywara@arm.com> wrote: > > > > On Wed, 13 Nov 2024 16:50:19 +0800 > > Chen-Yu Tsai <wens@csie.org> wrote: > > > > Hi Chen-Yu, > > > > sorry for the late reply, I was away for a week. > > > > > On Mon, Nov 11, 2024 at 8:58 AM Andre Przywara <andre.przywara@arm.com> wrote: > > > > > > > > The A523 contains a pin controller similar to previous SoCs, although > > > > using 10 GPIO banks (PortB-PortK), all of them being IRQ capable. > > > > This introduces a new style of binding, where the pinmux values for each > > > > pin group is stored in the new "allwinner,pinmux" property in the DT > > > > node, instead of requiring every driver to store a mapping between the > > > > function names and the required pinmux. > > > > > > > > Add the new name to the list of compatible strings, and required it to > > > > have 10 interrupts described. Also add the new pinmux property. > > > > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > --- > > > > .../pinctrl/allwinner,sun4i-a10-pinctrl.yaml | 23 +++++++++++++++++-- > > > > 1 file changed, 21 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > > > > index 4502405703145..6fc18e92e1e94 100644 > > > > --- a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > > > > +++ b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > > > > @@ -56,6 +56,8 @@ properties: > > > > - allwinner,sun50i-h6-r-pinctrl > > > > - allwinner,sun50i-h616-pinctrl > > > > - allwinner,sun50i-h616-r-pinctrl > > > > + - allwinner,sun55i-a523-pinctrl > > > > + - allwinner,sun55i-a523-r-pinctrl > > > > - allwinner,suniv-f1c100s-pinctrl > > > > - nextthing,gr8-pinctrl > > > > > > > > @@ -64,7 +66,7 @@ properties: > > > > > > > > interrupts: > > > > minItems: 1 > > > > - maxItems: 8 > > > > + maxItems: 10 > > > > description: > > > > One interrupt per external interrupt bank supported on the > > > > controller, sorted by bank number ascending order. > > > > @@ -119,13 +121,17 @@ patternProperties: > > > > $ref: /schemas/types.yaml#/definitions/uint32 > > > > enum: [10, 20, 30, 40] > > > > > > > > + allwinner,pinmux: > > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > > + description: pinmux selector for each pin > > > > + > > > > > > Why not just the standard "pinmux" property, as given in > > > Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml > > > > I had it like this in my last post two years ago, but learned from > > LinusW [1] that the generic pinmux property has a slightly different > > meaning, and abusing it for just the pinmux index values would not match > > the generic definition. > > We *could* use the generic definition, but then this would include what's > > in the "pins" property, like I sketched out in the cover letter, as an > > alternative to this approach: > > pinmux = <SUNXI_PIN(PB, 9, 2)>, <SUNXI_PIN(PB, 10, 2)>; > > Where the SUNXI_PIN macro would combine the pin number and the pinmux into > > one 32-bit cell. See the Apple GPIO DT nodes for an example. > > This looks indeed nicer, but requires quite some rewrite of the existing > > pinctrl driver, AFAICS. > > Sorry for taking so long to get back to this. > > Could we maybe add a generic replacement of the existing "function" > property, which takes a string? Like "function-id" or "function-selector" > that takes u32 (or u8). Then it could be one or the other. Not sure > if the binding maintainers would accept this or not. Do you mean specifically a *generic* property, as opposed to something prefixed with a vendor string, and coded up in just the sunxi driver? Because otherwise "allwinner,pinmux" is that numeric equivalent to "function". I kept "function", as a string, because the GPIO framework still needs a string at places, for instance for sysfs. We could create those strings, based on the node name, by sprintf'ing something, but I figured we might as well keep "function". In my U-Boot patches [1] I actually ignore the new pinmux property, and still use the function name, as it was easier to integrate into the existing code. U-Boot uses a very limited subset of our current table, so each new SoC doesn't add much to the code. [1] https://github.com/apritzel/u-boot/commit/ab4f7ed0879022357646 Code-wise I think we would still need our own driver code, so whether we use "function-id" or "allwinner,pinmux" in there doesn't make much of a difference, I think. > I understand that we probably don't want the mux value combined with > the pin number. You mean like the Apple GPIO does? I wonder why not, actually? I find this actually quite clever and compact. Again the "pins" property atm is quite string-heavy, so the code has to translate this back into a bank and pin number, then lookup the function string in our table to get the pinmux value. We could fit all of this information easily into this new generic "pinmux" property, and the code just needs to mask and shift that number. Each pin occupies a cell, I don't think we can get much better than that? Cheers, Andre > ChenYu > > > [1] Previous reply from LinusW: > > https://lore.kernel.org/linux-sunxi/CACRpkdbMc-Q6wjgsiddu6-tWC1dt2uFk+4LyerMdgFk2KRGK4w@mail.gmail.com/ > > > > > > > > > required: > > > > - pins > > > > - function > > > > > > This section should be made to apply only to the existing > > > compatibles? Maybe we could just split the files and have > > > a clean slate for sun55i? > > > > Yeah, I couldn't find a good example how to make it *required* for one > > compatible and *not allowed* for all the others. But creating a whole new > > file is actually a good idea, as this also avoids adding another case to > > the already quite indented if-else cascade. > > > > Cheers, > > Andre > > > > > ChenYu > > > > > > > additionalProperties: false > > > > > > > > - "^vcc-p[a-ilm]-supply$": > > > > + "^vcc-p[a-klm]-supply$": > > > > description: > > > > Power supplies for pin banks. > > > > > > > > @@ -156,6 +162,17 @@ allOf: > > > > - interrupts > > > > - interrupt-controller > > > > > > > > + - if: > > > > + properties: > > > > + compatible: > > > > + enum: > > > > + - allwinner,sun55i-a523-pinctrl > > > > + > > > > + then: > > > > + properties: > > > > + interrupts: > > > > + minItems: 10 > > > > + > > > > - if: > > > > properties: > > > > compatible: > > > > @@ -166,6 +183,7 @@ allOf: > > > > properties: > > > > interrupts: > > > > minItems: 8 > > > > + maxItems: 8 > > > > > > > > - if: > > > > properties: > > > > @@ -244,6 +262,7 @@ allOf: > > > > - allwinner,sun8i-v3s-pinctrl > > > > - allwinner,sun9i-a80-r-pinctrl > > > > - allwinner,sun50i-h6-r-pinctrl > > > > + - allwinner,sun55i-a523-r-pinctrl > > > > > > > > then: > > > > properties: > > > > -- > > > > 2.46.2 > > > > > >
On Tue, Jan 14, 2025 at 7:21 PM Andre Przywara <andre.przywara@arm.com> wrote: > > On Tue, 14 Jan 2025 15:01:31 +0800 > Chen-Yu Tsai <wens@csie.org> wrote: > > Hi Chen-Yu, > > before I get to your specific question below: what do you think in > general of the idea of getting rid of that table based approach we use so > far? Is that something worthwhile? I definitely think yes, but wanted to > hear the maintainers' opinion about this. Happy to present some arguments > if need be. Seems OK to me. The other platforms I've worked on don't have the tables. OOTH, would we have headers alongside the dts files for macros that give the function IDs a bit more meaning? I mean sure we can always go back to the datasheets, it's just a bit of a hassle. > ... > > > On Wed, Nov 20, 2024 at 6:12 PM Andre Przywara <andre.przywara@arm.com> wrote: > > > > > > On Wed, 13 Nov 2024 16:50:19 +0800 > > > Chen-Yu Tsai <wens@csie.org> wrote: > > > > > > Hi Chen-Yu, > > > > > > sorry for the late reply, I was away for a week. > > > > > > > On Mon, Nov 11, 2024 at 8:58 AM Andre Przywara <andre.przywara@arm.com> wrote: > > > > > > > > > > The A523 contains a pin controller similar to previous SoCs, although > > > > > using 10 GPIO banks (PortB-PortK), all of them being IRQ capable. > > > > > This introduces a new style of binding, where the pinmux values for each > > > > > pin group is stored in the new "allwinner,pinmux" property in the DT > > > > > node, instead of requiring every driver to store a mapping between the > > > > > function names and the required pinmux. > > > > > > > > > > Add the new name to the list of compatible strings, and required it to > > > > > have 10 interrupts described. Also add the new pinmux property. > > > > > > > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > > --- > > > > > .../pinctrl/allwinner,sun4i-a10-pinctrl.yaml | 23 +++++++++++++++++-- > > > > > 1 file changed, 21 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > > > > > index 4502405703145..6fc18e92e1e94 100644 > > > > > --- a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > > > > > +++ b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml > > > > > @@ -56,6 +56,8 @@ properties: > > > > > - allwinner,sun50i-h6-r-pinctrl > > > > > - allwinner,sun50i-h616-pinctrl > > > > > - allwinner,sun50i-h616-r-pinctrl > > > > > + - allwinner,sun55i-a523-pinctrl > > > > > + - allwinner,sun55i-a523-r-pinctrl > > > > > - allwinner,suniv-f1c100s-pinctrl > > > > > - nextthing,gr8-pinctrl > > > > > > > > > > @@ -64,7 +66,7 @@ properties: > > > > > > > > > > interrupts: > > > > > minItems: 1 > > > > > - maxItems: 8 > > > > > + maxItems: 10 > > > > > description: > > > > > One interrupt per external interrupt bank supported on the > > > > > controller, sorted by bank number ascending order. > > > > > @@ -119,13 +121,17 @@ patternProperties: > > > > > $ref: /schemas/types.yaml#/definitions/uint32 > > > > > enum: [10, 20, 30, 40] > > > > > > > > > > + allwinner,pinmux: > > > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > > > + description: pinmux selector for each pin > > > > > + > > > > > > > > Why not just the standard "pinmux" property, as given in > > > > Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml > > > > > > I had it like this in my last post two years ago, but learned from > > > LinusW [1] that the generic pinmux property has a slightly different > > > meaning, and abusing it for just the pinmux index values would not match > > > the generic definition. > > > We *could* use the generic definition, but then this would include what's > > > in the "pins" property, like I sketched out in the cover letter, as an > > > alternative to this approach: > > > pinmux = <SUNXI_PIN(PB, 9, 2)>, <SUNXI_PIN(PB, 10, 2)>; > > > Where the SUNXI_PIN macro would combine the pin number and the pinmux into > > > one 32-bit cell. See the Apple GPIO DT nodes for an example. > > > This looks indeed nicer, but requires quite some rewrite of the existing > > > pinctrl driver, AFAICS. > > > > Sorry for taking so long to get back to this. > > > > Could we maybe add a generic replacement of the existing "function" > > property, which takes a string? Like "function-id" or "function-selector" > > that takes u32 (or u8). Then it could be one or the other. Not sure > > if the binding maintainers would accept this or not. > > Do you mean specifically a *generic* property, as opposed to something > prefixed with a vendor string, and coded up in just the sunxi driver? > > Because otherwise "allwinner,pinmux" is that numeric equivalent to > "function". I kept "function", as a string, because the GPIO framework > still needs a string at places, for instance for sysfs. We could create > those strings, based on the node name, by sprintf'ing something, but I > figured we might as well keep "function". That seems to be what other drivers are doing. > In my U-Boot patches [1] I actually ignore the new pinmux property, and > still use the function name, as it was easier to integrate into the > existing code. U-Boot uses a very limited subset of our current table, > so each new SoC doesn't add much to the code. > > [1] https://github.com/apritzel/u-boot/commit/ab4f7ed0879022357646 > > Code-wise I think we would still need our own driver code, so whether we > use "function-id" or "allwinner,pinmux" in there doesn't make much of a > difference, I think. I guess it simply affects whether the bindings and subsequent helper code can be generalized or not. > > I understand that we probably don't want the mux value combined with > > the pin number. > > You mean like the Apple GPIO does? I wonder why not, actually? I find this > actually quite clever and compact. Again the "pins" property atm is quite > string-heavy, so the code has to translate this back into a bank and pin > number, then lookup the function string in our table to get the pinmux > value. We could fit all of this information easily into this new > generic "pinmux" property, and the code just needs to mask and shift that > number. Each pin occupies a cell, I don't think we can get much better > than that? Mostly because it prevents one from having pin config options without muxing, for example set drive strength without setting the mux to GPIO, the latter on sunxi is done exclusively using GPIO bindings. I talked about this at my talk at ELCE last year, and afterwards was talking to Heiko and Quentin, and it seems at least Rockchip can't separate this. Rockchip goes even further with their "rockchip,pins" property by also including a reference to the pin config node phandle. I believe MediaTek, which uses "pinmux" as you described, can't either. That's my main concern. Also, the "pins" property can be integers. The question then becomes how do you want to handle the numbering, i.e. what to do with the holes in between the different banks. ChenYu > Cheers, > Andre > > > ChenYu > > > > > [1] Previous reply from LinusW: > > > https://lore.kernel.org/linux-sunxi/CACRpkdbMc-Q6wjgsiddu6-tWC1dt2uFk+4LyerMdgFk2KRGK4w@mail.gmail.com/ > > > > > > > > > > > > required: > > > > > - pins > > > > > - function > > > > > > > > This section should be made to apply only to the existing > > > > compatibles? Maybe we could just split the files and have > > > > a clean slate for sun55i? > > > > > > Yeah, I couldn't find a good example how to make it *required* for one > > > compatible and *not allowed* for all the others. But creating a whole new > > > file is actually a good idea, as this also avoids adding another case to > > > the already quite indented if-else cascade. > > > > > > Cheers, > > > Andre > > > > > > > ChenYu > > > > > > > > > additionalProperties: false > > > > > > > > > > - "^vcc-p[a-ilm]-supply$": > > > > > + "^vcc-p[a-klm]-supply$": > > > > > description: > > > > > Power supplies for pin banks. > > > > > > > > > > @@ -156,6 +162,17 @@ allOf: > > > > > - interrupts > > > > > - interrupt-controller > > > > > > > > > > + - if: > > > > > + properties: > > > > > + compatible: > > > > > + enum: > > > > > + - allwinner,sun55i-a523-pinctrl > > > > > + > > > > > + then: > > > > > + properties: > > > > > + interrupts: > > > > > + minItems: 10 > > > > > + > > > > > - if: > > > > > properties: > > > > > compatible: > > > > > @@ -166,6 +183,7 @@ allOf: > > > > > properties: > > > > > interrupts: > > > > > minItems: 8 > > > > > + maxItems: 8 > > > > > > > > > > - if: > > > > > properties: > > > > > @@ -244,6 +262,7 @@ allOf: > > > > > - allwinner,sun8i-v3s-pinctrl > > > > > - allwinner,sun9i-a80-r-pinctrl > > > > > - allwinner,sun50i-h6-r-pinctrl > > > > > + - allwinner,sun55i-a523-r-pinctrl > > > > > > > > > > then: > > > > > properties: > > > > > -- > > > > > 2.46.2 > > > > > > > > > >
diff --git a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml index 4502405703145..6fc18e92e1e94 100644 --- a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml +++ b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml @@ -56,6 +56,8 @@ properties: - allwinner,sun50i-h6-r-pinctrl - allwinner,sun50i-h616-pinctrl - allwinner,sun50i-h616-r-pinctrl + - allwinner,sun55i-a523-pinctrl + - allwinner,sun55i-a523-r-pinctrl - allwinner,suniv-f1c100s-pinctrl - nextthing,gr8-pinctrl @@ -64,7 +66,7 @@ properties: interrupts: minItems: 1 - maxItems: 8 + maxItems: 10 description: One interrupt per external interrupt bank supported on the controller, sorted by bank number ascending order. @@ -119,13 +121,17 @@ patternProperties: $ref: /schemas/types.yaml#/definitions/uint32 enum: [10, 20, 30, 40] + allwinner,pinmux: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: pinmux selector for each pin + required: - pins - function additionalProperties: false - "^vcc-p[a-ilm]-supply$": + "^vcc-p[a-klm]-supply$": description: Power supplies for pin banks. @@ -156,6 +162,17 @@ allOf: - interrupts - interrupt-controller + - if: + properties: + compatible: + enum: + - allwinner,sun55i-a523-pinctrl + + then: + properties: + interrupts: + minItems: 10 + - if: properties: compatible: @@ -166,6 +183,7 @@ allOf: properties: interrupts: minItems: 8 + maxItems: 8 - if: properties: @@ -244,6 +262,7 @@ allOf: - allwinner,sun8i-v3s-pinctrl - allwinner,sun9i-a80-r-pinctrl - allwinner,sun50i-h6-r-pinctrl + - allwinner,sun55i-a523-r-pinctrl then: properties:
The A523 contains a pin controller similar to previous SoCs, although using 10 GPIO banks (PortB-PortK), all of them being IRQ capable. This introduces a new style of binding, where the pinmux values for each pin group is stored in the new "allwinner,pinmux" property in the DT node, instead of requiring every driver to store a mapping between the function names and the required pinmux. Add the new name to the list of compatible strings, and required it to have 10 interrupts described. Also add the new pinmux property. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- .../pinctrl/allwinner,sun4i-a10-pinctrl.yaml | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)