diff mbox series

[1/2] dt-bindings: pinctrl: Add DT bindings for apple,pinctrl

Message ID 20210508142000.85116-2-kettenis@openbsd.org
State Accepted
Commit 710071479cf8c6127791427561a6ba4ee916c07f
Headers show
Series Apple M1 pinctrl DT bindings | expand

Commit Message

Mark Kettenis May 8, 2021, 2:19 p.m. UTC 
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

Comments

Linus Walleij May 9, 2021, 12:18 a.m. UTC | #1 
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
Krzysztof Kozlowski May 10, 2021, 1:03 p.m. UTC | #2 
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.


Best regards,
Krzysztof
Rob Herring (Arm) May 10, 2021, 2:01 p.m. UTC | #3 
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

See https://patchwork.ozlabs.org/patch/1475875

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
Rob Herring (Arm) May 10, 2021, 2:19 p.m. UTC | #4 
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

>
Mark Kettenis May 10, 2021, 5:06 p.m. UTC | #5 
> Date: Mon, 10 May 2021 09:19:55 -0500

> From: Rob Herring <robh@kernel.org>


Hi Rob,

> 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.


Krzysztof raised a similar point.  It seems that Apple isn't in the
habit of changing this aspect of their SoCs.  Judging from

  https://github.com/corellium/linux-sandcastle/blob/sandcastle-5.4/drivers/pinctrl/pinctrl-hx-gpio.c

Apple has been using a controller with the same register layout and
the same known bits in those registers since the T8010 SoC (the A10
used in the iPhone 7).  So there is some confidence that a single
driver will work for many generations of the SoC.  Of course we don't
have any authoritative documentaton on these SoCs so I can't be 100%
sure that there aren't any subtle differences in the behaviour of the
hardware.  That's why I also include a more specific compatible for
the T8103 SoC (the M1 used in the current Macs).

> > +

> > +  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

> > 

>
Mark Kettenis May 10, 2021, 5:09 p.m. UTC | #6 
> 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
Rob Herring May 10, 2021, 6:18 p.m. UTC | #7 
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
Linus Walleij May 19, 2021, 11:27 p.m. UTC | #8 
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
Mark Kettenis May 20, 2021, 11:22 a.m. UTC | #9 
> 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 mbox series

Patch

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 */