linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Linus Walleij <linus.walleij@linaro.org>
To: kettenis@openbsd.org
Cc: "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
	<devicetree@vger.kernel.org>, Hector Martin <marcan@marcan.st>,
	 Rob Herring <robh+dt@kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	 "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] dt-bindings: pinctrl: Add DT bindings for apple, pinctrl
Date: Sun, 9 May 2021 02:18:52 +0200	[thread overview]
Message-ID: <CACRpkdbexpwu9hDWFYe38-+issVw051YfJ-_hatZVmWzkh4M+w@mail.gmail.com> (raw)
In-Reply-To: <20210508142000.85116-2-kettenis@openbsd.org>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  parent reply	other threads:[~2021-05-09  0:22 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-08 14:19 [PATCH 0/2] Apple M1 pinctrl DT bindings Mark Kettenis
2021-05-08 14:19 ` [PATCH 1/2] dt-bindings: pinctrl: Add DT bindings for apple,pinctrl Mark Kettenis
2021-05-08 21:09   ` [PATCH 1/2] dt-bindings: pinctrl: Add DT bindings for apple, pinctrl Linus Walleij
2021-05-08 23:02     ` Mark Kettenis
2021-05-09  0:27       ` Linus Walleij
2021-05-09  9:50         ` Tomasz Figa
2021-05-10 13:09           ` [PATCH 1/2] dt-bindings: pinctrl: Add DT bindings for apple,pinctrl Krzysztof Kozlowski
2021-05-09  0:18   ` Linus Walleij [this message]
2021-05-09  9:46     ` [PATCH 1/2] dt-bindings: pinctrl: Add DT bindings for apple, pinctrl Mark Kettenis
2021-05-09 10:49       ` Linus Walleij
2021-05-10 13:03   ` [PATCH 1/2] dt-bindings: pinctrl: Add DT bindings for apple,pinctrl Krzysztof Kozlowski
2021-05-10 17:09     ` Mark Kettenis
2021-05-10 14:01   ` [PATCH 1/2] dt-bindings: pinctrl: Add DT bindings for apple, pinctrl Rob Herring
2021-05-10 18:18     ` Rob Herring
2021-05-10 14:19   ` [PATCH 1/2] dt-bindings: pinctrl: Add DT bindings for apple,pinctrl Rob Herring
2021-05-10 17:06     ` Mark Kettenis
2021-05-19 23:27       ` [PATCH 1/2] dt-bindings: pinctrl: Add DT bindings for apple, pinctrl Linus Walleij
2021-05-20 11:22         ` Mark Kettenis
2021-05-08 14:19 ` [PATCH 2/2] arm64: apple: Add pinctrl nodes Mark Kettenis

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CACRpkdbexpwu9hDWFYe38-+issVw051YfJ-_hatZVmWzkh4M+w@mail.gmail.com \
    --to=linus.walleij@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=kettenis@openbsd.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marcan@marcan.st \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).