| Message ID | 20210508142000.85116-2-kettenis@openbsd.org |
|---|---|
| State | Accepted |
| Commit | 710071479cf8c6127791427561a6ba4ee916c07f |
| Headers | show |
| Series | Apple M1 pinctrl DT bindings | expand |
Hi Mark, here is a second note on pin mux layout: On Sat, May 8, 2021 at 4:20 PM Mark Kettenis <kettenis@openbsd.org> wrote: > + pcie_pins: pcie-pins { > + pinmux = <APPLE_PINMUX(150, 1)>, > + <APPLE_PINMUX(151, 1)>, > + <APPLE_PINMUX(32, 1)>; (...) > +#define APPLE_PINMUX(pin, func) ((pin) | ((func) << 16)) > +#define APPLE_PIN(pinmux) ((pinmux) & 0xffff) > +#define APPLE_FUNC(pinmux) ((pinmux) >> 16) Since the word altfunction is used, I suppose that this is one of those pin controllers where each pin can be muxed individually (usually with one register or one group of bits per pin). So this is one way to do it, which Another way is what Qualcomm is doing and looks for example like this: pinctrl@800000 { /* eMMMC pins, all 8 data lines connected */ dragon_sdcc1_pins: sdcc1 { mux { pins = "gpio159", "gpio160", "gpio161", "gpio162", "gpio163", "gpio164", "gpio165", "gpio166", "gpio167", "gpio168"; function = "sdc1"; }; (...) Here all pins have a name and they get assigned as a group to a function. Each pin is referenced by name. Some people don't like this because they like bitstuffing and bitfiddling and are worried that the DTB file strings will take up too much memory, and they have to include all these strings in their operating system driver. However there are clear upsides to it, when you later on come to set up the electrical pin config: cmd { pins = "gpio168"; /* SDC1 CMD */ drive-strength = <12>; bias-pull-up; }; data { /* SDC1 D0 to D7 */ pins = "gpio159", "gpio160", "gpio161", "gpio162", "gpio163", "gpio164", "gpio165", "gpio166"; drive-strength = <8>; bias-pull-none; }; As you can see this becomes quite readable. It is clear and crisp which pins are set up for pull-up and not, and what drive strength is used on each pin. But notice first and foremost this: the muxing is done in one node, and the electrical config is done in two separate nodes, breaking muxing and config into two different categories in the device tree. The problem with the magic number approach to muxing is that the magic numbers will fall through to the electrical pin config later and indeed it looks like in the STM32 device trees: sdmmc1_b4_od_pins_a: sdmmc1-b4-od-0 { pins1 { pinmux = <STM32_PINMUX('C', 8, AF12)>, /* SDMMC1_D0 */ <STM32_PINMUX('C', 9, AF12)>, /* SDMMC1_D1 */ <STM32_PINMUX('C', 10, AF12)>, /* SDMMC1_D2 */ <STM32_PINMUX('C', 11, AF12)>, /* SDMMC1_D3 */ <STM32_PINMUX('C', 12, AF12)>; /* SDMMC1_CK */ slew-rate = <3>; drive-push-pull; bias-disable; }; pins2 { pinmux = <STM32_PINMUX('D', 2, AF12)>; /* SDMMC1_CMD */ slew-rate = <3>; drive-open-drain; bias-disable; }; }; Notice here how the pins need to be separated into two subnodes in order to set different electrical configuration on them, and how muxing and configuration are mixed up. This is a side effect of using the "pinmux" attribute rather than "pins" and "function". So make sure you really like this rather than the other approach in your device trees. I will definately insist that you electrical config be done similar to how STM32 does it when you implement that later, for example any magic numbers for electrical config is not acceptable, you will have to find a way to use drive-open-drain; and such flags in the device tree. Sadly we have something like three different ways to do pin control device tree, as a result of failure to find consensus. Yours, Linus Walleij
On Sat, May 08, 2021 at 04:19:55PM +0200, Mark Kettenis wrote: > The Apple GPIO controller is a simple combined pin and GPIO conroller > present on Apple ARM SoC platforms, including various iPhone and iPad > devices and the "Apple Silicon" Macs. > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > --- > .../bindings/pinctrl/apple,pinctrl.yaml | 103 ++++++++++++++++++ > MAINTAINERS | 2 + > include/dt-bindings/pinctrl/apple.h | 13 +++ > 3 files changed, 118 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml > create mode 100644 include/dt-bindings/pinctrl/apple.h > > diff --git a/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml > new file mode 100644 > index 000000000000..cc7805ca6ba1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml > @@ -0,0 +1,103 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/apple,pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Apple GPIO controller > + > +maintainers: > + - Mark Kettenis <kettenis@openbsd.org> > + > +description: | > + The Apple GPIO controller is a simple combined pin and GPIO conroller > + present on Apple ARM SoC platforms, including various iPhone and iPad > + devices and the "Apple Silicon" Macs. > + > +properties: > + compatible: > + items: > + - const: apple,t8103-pinctrl > + - const: apple,pinctrl A genericish fallback is maybe questionable for pinctrl. That's not often the same from one SoC to the next. > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + gpio-controller: true > + > + '#gpio-cells': > + const: 2 > + > + gpio-ranges: > + maxItems: 1 > + > + interrupts: > + minItems: 1 > + maxItems: 7 > + > + interrupt-controller: true > + > +patternProperties: > + '-pins$': > + type: object > + $ref: pinmux-node.yaml# > + > + properties: > + pinmux: > + description: > + Values are constructed from pin number and alternate function > + configuration number using the APPLE_PINMUX() helper macro > + defined in include/dt-bindings/pinctrl/apple.h. > + > + required: > + - pinmux > + > + additionalProperties: false > + > +required: > + - compatible > + - reg > + - gpio-controller > + - '#gpio-cells' > + - gpio-ranges > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/apple-aic.h> > + #include <dt-bindings/pinctrl/apple.h> > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + > + pinctrl: pinctrl@23c100000 { > + compatible = "apple,t8103-pinctrl", "apple,pinctrl"; > + reg = <0x2 0x3c100000 0x0 0x100000>; > + clocks = <&gpio_clk>; > + > + gpio-controller; > + #gpio-cells = <2>; > + gpio-ranges = <&pinctrl 0 0 212>; > + > + interrupt-controller; > + interrupt-parent = <&aic>; > + interrupts = <AIC_IRQ 16 IRQ_TYPE_LEVEL_HIGH>, > + <AIC_IRQ 17 IRQ_TYPE_LEVEL_HIGH>, > + <AIC_IRQ 18 IRQ_TYPE_LEVEL_HIGH>, > + <AIC_IRQ 19 IRQ_TYPE_LEVEL_HIGH>, > + <AIC_IRQ 20 IRQ_TYPE_LEVEL_HIGH>, > + <AIC_IRQ 21 IRQ_TYPE_LEVEL_HIGH>, > + <AIC_IRQ 22 IRQ_TYPE_LEVEL_HIGH>; > + > + pcie_pins: pcie-pins { > + pinmux = <APPLE_PINMUX(150, 1)>, > + <APPLE_PINMUX(151, 1)>, > + <APPLE_PINMUX(32, 1)>; > + }; > + }; > + }; > diff --git a/MAINTAINERS b/MAINTAINERS > index ad0e9be66885..7327c9b778f1 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1654,9 +1654,11 @@ C: irc://chat.freenode.net/asahi-dev > T: git https://github.com/AsahiLinux/linux.git > F: Documentation/devicetree/bindings/arm/apple.yaml > F: Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml > +F: Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml > F: arch/arm64/boot/dts/apple/ > F: drivers/irqchip/irq-apple-aic.c > F: include/dt-bindings/interrupt-controller/apple-aic.h > +F: include/dt-bindings/pinctrl/apple.h > > ARM/ARTPEC MACHINE SUPPORT > M: Jesper Nilsson <jesper.nilsson@axis.com> > diff --git a/include/dt-bindings/pinctrl/apple.h b/include/dt-bindings/pinctrl/apple.h > new file mode 100644 > index 000000000000..ea0a6f466592 > --- /dev/null > +++ b/include/dt-bindings/pinctrl/apple.h > @@ -0,0 +1,13 @@ > +/* SPDX-License-Identifier: GPL-2.0+ OR MIT */ > +/* > + * This header provides constants for Apple pinctrl bindings. > + */ > + > +#ifndef _DT_BINDINGS_PINCTRL_APPLE_H > +#define _DT_BINDINGS_PINCTRL_APPLE_H > + > +#define APPLE_PINMUX(pin, func) ((pin) | ((func) << 16)) > +#define APPLE_PIN(pinmux) ((pinmux) & 0xffff) > +#define APPLE_FUNC(pinmux) ((pinmux) >> 16) > + > +#endif /* _DT_BINDINGS_PINCTRL_APPLE_H */ > -- > 2.31.1 >
> From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > Date: Mon, 10 May 2021 09:03:13 -0400 > > On 08/05/2021 10:19, Mark Kettenis wrote: > > The Apple GPIO controller is a simple combined pin and GPIO conroller > > present on Apple ARM SoC platforms, including various iPhone and iPad > > devices and the "Apple Silicon" Macs. > > > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > > --- > > .../bindings/pinctrl/apple,pinctrl.yaml | 103 ++++++++++++++++++ > > MAINTAINERS | 2 + > > include/dt-bindings/pinctrl/apple.h | 13 +++ > > 3 files changed, 118 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml > > create mode 100644 include/dt-bindings/pinctrl/apple.h > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml > > new file mode 100644 > > index 000000000000..cc7805ca6ba1 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml > > @@ -0,0 +1,103 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pinctrl/apple,pinctrl.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Apple GPIO controller > > + > > +maintainers: > > + - Mark Kettenis <kettenis@openbsd.org> > > + > > +description: | > > + The Apple GPIO controller is a simple combined pin and GPIO conroller > > + present on Apple ARM SoC platforms, including various iPhone and iPad > > + devices and the "Apple Silicon" Macs. > > + > > +properties: > > + compatible: > > + items: > > + - const: apple,t8103-pinctrl > > + - const: apple,pinctrl > > What is the point of having very generic final compatible in the binding > which does not relate to actual hardware? > > Let's say next SoC will be > apple,x-abcd-foo-2323-whatever-nothing-in-common and you still have to > use generic "apple,pinctrl" even though HW is not at all compatible? > This looks like wildcard, not HW description. Hi Krzysztof, See my reply to Rob's mail. We have some confidence that Apple isn't changing their GPIO block very often. If they were to change it in an incompatible way in a future SoC, we'd drop the "apple,pinctrl" compatible of course. Thanks, Mark
On Mon, May 10, 2021 at 9:01 AM Rob Herring <robh@kernel.org> wrote: > > On Sat, 08 May 2021 16:19:55 +0200, Mark Kettenis wrote: > > The Apple GPIO controller is a simple combined pin and GPIO conroller > > present on Apple ARM SoC platforms, including various iPhone and iPad > > devices and the "Apple Silicon" Macs. > > > > Signed-off-by: Mark Kettenis <kettenis@openbsd.org> > > --- > > .../bindings/pinctrl/apple,pinctrl.yaml | 103 ++++++++++++++++++ > > MAINTAINERS | 2 + > > include/dt-bindings/pinctrl/apple.h | 13 +++ > > 3 files changed, 118 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml > > create mode 100644 include/dt-bindings/pinctrl/apple.h > > > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > Documentation/devicetree/bindings/pinctrl/apple,pinctrl.example.dts:19:18: fatal error: dt-bindings/interrupt-controller/apple-aic.h: No such file or directory > 19 | #include <dt-bindings/interrupt-controller/apple-aic.h> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > compilation terminated. > make[1]: *** [scripts/Makefile.lib:377: Documentation/devicetree/bindings/pinctrl/apple,pinctrl.example.dt.yaml] Error 1 > make[1]: *** Waiting for unfinished jobs.... > make: *** [Makefile:1414: dt_binding_check] Error 2 Ignore this. I've now updated the base to 5.13-rc1. Rob
On Mon, May 10, 2021 at 7:06 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > From: Rob Herring <robh@kernel.org> > > > +properties: > > > + compatible: > > > + items: > > > + - const: apple,t8103-pinctrl > > > + - const: apple,pinctrl > > > > A genericish fallback is maybe questionable for pinctrl. That's not > > often the same from one SoC to the next. > > Krzysztof raised a similar point. It seems that Apple isn't in the > habit of changing this aspect of their SoCs. Rob what's your stance on this? Does it need to be changed? Else I'll apply the patch. Yours, Linus Walleij
> From: Linus Walleij <linus.walleij@linaro.org> > Date: Thu, 20 May 2021 01:27:37 +0200 > > On Mon, May 10, 2021 at 7:06 PM Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Rob Herring <robh@kernel.org> > > > > > +properties: > > > > + compatible: > > > > + items: > > > > + - const: apple,t8103-pinctrl > > > > + - const: apple,pinctrl > > > > > > A genericish fallback is maybe questionable for pinctrl. That's not > > > often the same from one SoC to the next. > > > > Krzysztof raised a similar point. It seems that Apple isn't in the > > habit of changing this aspect of their SoCs. > > Rob what's your stance on this? Does it need to be changed? > Else I'll apply the patch. Hi Linus, Rob asked me to provide a description for the interrupts in response to the v2 I sent a few days ago: http://patchwork.ozlabs.org/project/devicetree-bindings/patch/20210516183221.93686-2-mark.kettenis@xs4all.nl/ I'll roll a v3 later today for that. Cheers, Mark
diff --git a/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml new file mode 100644 index 000000000000..cc7805ca6ba1 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml @@ -0,0 +1,103 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pinctrl/apple,pinctrl.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Apple GPIO controller + +maintainers: + - Mark Kettenis <kettenis@openbsd.org> + +description: | + The Apple GPIO controller is a simple combined pin and GPIO conroller + present on Apple ARM SoC platforms, including various iPhone and iPad + devices and the "Apple Silicon" Macs. + +properties: + compatible: + items: + - const: apple,t8103-pinctrl + - const: apple,pinctrl + + reg: + maxItems: 1 + + clocks: + maxItems: 1 + + gpio-controller: true + + '#gpio-cells': + const: 2 + + gpio-ranges: + maxItems: 1 + + interrupts: + minItems: 1 + maxItems: 7 + + interrupt-controller: true + +patternProperties: + '-pins$': + type: object + $ref: pinmux-node.yaml# + + properties: + pinmux: + description: + Values are constructed from pin number and alternate function + configuration number using the APPLE_PINMUX() helper macro + defined in include/dt-bindings/pinctrl/apple.h. + + required: + - pinmux + + additionalProperties: false + +required: + - compatible + - reg + - gpio-controller + - '#gpio-cells' + - gpio-ranges + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/apple-aic.h> + #include <dt-bindings/pinctrl/apple.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + pinctrl: pinctrl@23c100000 { + compatible = "apple,t8103-pinctrl", "apple,pinctrl"; + reg = <0x2 0x3c100000 0x0 0x100000>; + clocks = <&gpio_clk>; + + gpio-controller; + #gpio-cells = <2>; + gpio-ranges = <&pinctrl 0 0 212>; + + interrupt-controller; + interrupt-parent = <&aic>; + interrupts = <AIC_IRQ 16 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 17 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 18 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 19 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 20 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 21 IRQ_TYPE_LEVEL_HIGH>, + <AIC_IRQ 22 IRQ_TYPE_LEVEL_HIGH>; + + pcie_pins: pcie-pins { + pinmux = <APPLE_PINMUX(150, 1)>, + <APPLE_PINMUX(151, 1)>, + <APPLE_PINMUX(32, 1)>; + }; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index ad0e9be66885..7327c9b778f1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1654,9 +1654,11 @@ C: irc://chat.freenode.net/asahi-dev T: git https://github.com/AsahiLinux/linux.git F: Documentation/devicetree/bindings/arm/apple.yaml F: Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml +F: Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml F: arch/arm64/boot/dts/apple/ F: drivers/irqchip/irq-apple-aic.c F: include/dt-bindings/interrupt-controller/apple-aic.h +F: include/dt-bindings/pinctrl/apple.h ARM/ARTPEC MACHINE SUPPORT M: Jesper Nilsson <jesper.nilsson@axis.com> diff --git a/include/dt-bindings/pinctrl/apple.h b/include/dt-bindings/pinctrl/apple.h new file mode 100644 index 000000000000..ea0a6f466592 --- /dev/null +++ b/include/dt-bindings/pinctrl/apple.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0+ OR MIT */ +/* + * This header provides constants for Apple pinctrl bindings. + */ + +#ifndef _DT_BINDINGS_PINCTRL_APPLE_H +#define _DT_BINDINGS_PINCTRL_APPLE_H + +#define APPLE_PINMUX(pin, func) ((pin) | ((func) << 16)) +#define APPLE_PIN(pinmux) ((pinmux) & 0xffff) +#define APPLE_FUNC(pinmux) ((pinmux) >> 16) + +#endif /* _DT_BINDINGS_PINCTRL_APPLE_H */
The Apple GPIO controller is a simple combined pin and GPIO conroller present on Apple ARM SoC platforms, including various iPhone and iPad devices and the "Apple Silicon" Macs. Signed-off-by: Mark Kettenis <kettenis@openbsd.org> --- .../bindings/pinctrl/apple,pinctrl.yaml | 103 ++++++++++++++++++ MAINTAINERS | 2 + include/dt-bindings/pinctrl/apple.h | 13 +++ 3 files changed, 118 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/apple,pinctrl.yaml create mode 100644 include/dt-bindings/pinctrl/apple.h