An extremely simplified pinctrl bindings proposal

[Stephen Warren]

Sorry, I haven't had a chance to read any of the pincrl emails from
Friday onwards. However, I thought a bit more about this, and decided
to propose someting much simpler:

Thoughts:

* Defining all the pins, groups, functions, ... takes a lot of space,
  whether it's in static data in pinctrl drivers or in the device tree.
  The lists must also be stored in RAM at runtime.

* It's been very difficult to come up with a generic description of all
  pin controller's capabilities. This is true even irrespective of device
  tree; think pin config where we've agonized over whether we can create
  a standardized list of pin config properties, or need to allow each
  pinctrl driver to define its own set of properties, etc.

* The only real use of the lists is for debugfs. Drivers shouldn't expect
  to directly request specific pinctrl settings, since that would encode
  knowledge of an individual SoC's pin controller. This should be
  abstracted from drivers.

* The data in debugfs could easily be replaced by a raw register dump
  coupled with a SoC-specific script to print out what each register
  means.

My proposal below is to radically simplify the pinctrl subsystem, and
make it little more than a system to execute a list of arbitrary register
writes.

Advantages:

* No need to define any kind of data model for pinctrl.

* No need to store any list of pins/groups/function/parameters/... either
  in static data, in device tree, or at run-time.

* No need to store the the selected board-specific settings anywhere either.

* Completely generic; can handle anything that exists now or in the future.

* Handles pin mux and pin config identically.

* Extremely simple to implement and understand; probably just a few K
  of code and no data. I assume this will be very appealing to those
  interested in using the binding within U-Boot too.

* The lists of register writes can just as easily be provided by board
  files as device tree, so operation should be identical.

Disadvantages:

* Creating the list of register values/masks may be a little painful. If
  we move forward with this, we'd want to strongly push dtc towards
  providing named constants, expressesion, macros, etc. Those dtc features
  would be very useful anyway.

* Lack of high-level semantic meaning to the data; but I assert there's
  little benefit to having that anyway.

  To solve this, write a script to parse each pinctrl driver's regcache
  file and print out all the register meanings.

* This scheme wouldn't represent which device "owns" which pins/groups,
  nor be able to validate that different devices' pinmux settings don't
  conflict.

  I don't think this is a huge issue though, since that's mainly error-
  checking. Any problems should be just as obvious during testing whether
  they cause the HW to fail to operate correctly, or whether they cause
  pinctrl to throw an error. Equally, there are many error conditions
  that pinctrl core wasn't checking for anyway.

Precedent:

* There was previous discussion on using this exact technique for pinmux
  at a previous Linaro Connect:

  http://www.spinics.net/lists/linux-tegra/msg01943.html

* There is an existing binding that works very similarly, for the Mavell
  PHY:

  http://www.gossamer-threads.com/lists/linux/kernel/1305624
  drivers/net/phy/marvell.c

  Explicit mention was made here that a simple list of register writes
  avoiding the complexity of representing a huge number of combinations
  in the device tree.

tegra20.dtsi:

pinmux: pinmux@70000000 {
        compatible = "nvidia,tegra20-pinmux";
        reg = <0x70000014 0x10>  /* Tri-state registers */
              <0x70000080 0x20>  /* Mux registers */
              <0x700000a0 0x14>  /* Pull-up/down registers */
              <0x70000868 0xa8>; /* Pad control registers */

        pinmux_i2c1_spdio_active: pinmux-i2c1-spdio-active {
                reg-init =
                    /* Set pingroup SPDO to function I2C1 */
                    <
                        1          /* Register "bank" in controller */
                        0x8c       /* Register offset */
                        0x000000c0 /* Write bitmask, 0 for whole register */
                        0x00000080 /* Write value */
                    >
                    /* Set pingroup SPDI to function I2C1 */
                    <
                        1          /* Register "bank" in controller */
                        0x8c       /* Register offset */
                        0x00000300 /* Write bitmask, 0 for whole register */
                        0x00000200 /* Write value */
                    >;
        };
        pinmux_i2c1_spdio_suspend: pinmux-i2c1-spdio-suspend {
                ...
        };
};

tegra-seaboard.dts:

i2c@7000c000 {
        /*
         * These are phandles to nodes that containthe reg-init list. Can
         * we have phandles to properties instead? Then we wouldn't need
         * all those nodes which each just contains 1 property.
         */
        pinctrl = <&pinmux_i2c1_spdio_active &pinmux_i2c1_spdio_suspend>;
        pinctrl-names = "active", "suspend";
};

In order to support programming more than one pin controller at once, we
could include the phandle of the pin controller in the "reg-init" property:

somewhere-other-than-under-a-pin-controller {
        pinmux-foo {
                reg-init =
                    <
                        &tegra_pmx /* Pin controller */
                        2          /* Number of entries for this controller */
                    >
                    <
                        1          /* Register "bank" in controller */
                        0x8c       /* Register offset */
                        0x000000c0 /* Write bitmask, 0 for whole register */
                        0x00000080 /* Write value */
                    >
                    <
                        1          /* Register "bank" in controller */
                        0x8c       /* Register offset */
                        0x00000300 /* Write bitmask, 0 for whole register */
                        0x00000200 /* Write value */
                    >
                    <
                        &other_pmx /* Pin controller */
                        1          /* Number of entries for this controller */
                    >
                    <
                        0          /* Register "bank" in controller */
                        0x04       /* Register offset */
                        0x000000ff /* Write bitmask, 0 for whole register */
                        0x0000005a /* Write value */
                    >;
        }
};

When parsing a reg-init property, we know if it contains phandles by:
* If node containing property is a child of a pin controller, the property
  doesn't containt pin controller phandles.
* Otherwise, it does.

What are people's thoughts. Thinking about all the issues involved, this
is certainly the approach I like most right now.

-- 
nvpublic

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/