On Mon, Dec 11, 2017 at 3:46 PM, Rob Herring <robh@xxxxxxxxxx> wrote: > On Mon, Dec 11, 2017 at 3:07 AM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: >> On Thu, Dec 7, 2017 at 9:51 PM, Rob Herring <robh@xxxxxxxxxx> wrote: >> >> I don't really know the context but I guess devicetree.org standards >> document so I need to read it close :) >> >>> +Linus W >>> >>> On Thu, Dec 7, 2017 at 11:10 AM, Grant Likely <grant.likely@xxxxxxxxxxxx> wrote: >>>> Add the GPIOs binding as described in the Linux kernel. All of the >>>> binding except pinmux has been included. Pinmux is omitted until the >>>> pinmux binding is similar transferred. >> >> Two words: pin control - pin multiplexing is just half of the things >> pin control does. It also does electrical configuration of pins >> (pull ups etc) and that is not pinmux. >> >> But I get what you mean. >> >>>> The source binding in the Linux kernel tree can be found here: >>>> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/gpio/gpio.txt >>>> >>>> This patch is provided for review, but also as an exploration of >>>> bringing a full binding file over from the Linux freeform text into the >>>> specification document, and it shows the weakness of the current >>>> approach. All of the work done on this patch gives the text more >>>> structure and more formatting options, but it doesn't do anything to >>>> make binding files machine readable. I intend to iterate this patch to >>>> split the binding content off into a machine parsable file that can be >>>> parsed to emit Sphinx markup. This patch can be considered as the >>>> baseline. >> >> Ambitious! I like. >> >>>> +General Purpose IO (GPIO) >>>> +------------------------- >>>> + >>>> +GPIO signals are modelled in |spec| as a point to point connection between >>>> +a GPIO controller node which provides the GPIO signal, >>>> +and a GPIO consumer node. >>>> +A connection is described with a ``single-gpio`` which is placed in the >>>> +consumer node, and identifies a specific GPIO controller and signal. >> >> This reads like a textual form of the BNF-form specification that we have >> in the kernel. I assume other resources such as interrupts, clocks, regulators >> etc are described in similar wording so OK. >> >>>> +In this model, the purpose of a GPIO signal is determined by the GPIO consumer >>>> +node, which is entirely dependant on what device the consumer node represents. >>>> +The GPIO controller does not make any assumptions about how GPIOs will be used. >> >> Good. That is a way of saying that when we say something is general purpose, >> it is actually general purpose. Should be evident from the name, but given how >> some electronics people call everything GPIO it needs to be pointed out >> I guess. >> >>>> +For example, an MMC controller may use a GPIO connection to communicate the RW/RO pin state. >>>> +In this case the MMC controller node would identify the specific GPIO signal >>>> +used to detect RW/RO state, >>>> +and the MMC controller driver would know to configure it as an input. >>>> +The GPIO controller node has no knowledge of how the pin should be used and >>>> +merely provides a pin control interface to the MMC driver. >> >> This kind of mandates that the OS implementing GPIO must provide accessors >> for setting pin direction and reading/writing lines. I guess mandating what an >> OS driver "must do" is not the scope of the spec but it is kind of hard to avoid >> it creeping in. >> >>>> +To conform to the generic GPIO binding, both the GPIO controller and consumer >>>> +nodes must conform to this binding as detailed below. >>>> + >>>> +GPIO Definitions >>>> +^^^^^^^^^^^^^^^^ >>>> + >>>> +.. tabularcolumns:: | l l J | >>>> +.. table:: GPIO Binding Datatype Definitions >>>> + >>>> + =================== ============================== ================================================ >>>> + Type Definition Description >>>> + =================== ============================== ================================================ >>>> + ``gpio-list`` ``single-gpio [gpio-list]`` Array of one or more ``gpio-single``. >> >> Should it be "single-gpio" in the end there? >> >> As it is named here: >> >>>> + ``single-gpio`` ``<phandle> <gpio-specifier>`` Reference to a single GPIO signal specifying >>>> + both GPIO controller (``phandle``) and GPIO >>>> + signal from that controller (``gpio-specifier``) >>>> + ``gpio-specifier`` ``<u32>[0..#gpio-cells]`` Array of ``cells``. Array size defined by >>>> + value of *#gpio-cells* in GPIO controller node. >>>> + In other words, the size of a ``gpio-specifier`` >>>> + is dependent on the GPIO controller. >>>> + =================== ============================== ================================================ >>>> + >>>> +A ``gpio-specifier`` is an array of 1 or more ``cells`` indicating the specific GPIO signal and configuration of that signal. >> >> I would prefer to hammer down the gpio-specifier to 2 cells where the >> first cell specifies the line/signal and the second specifier electric >> properties, such as active low, open drain etc. >> >>>> +The GPIO controller binding must describe what information is encoded into its gpio-specifier, >>>> +which could include bank number, pin position, driver configuration, or signal inversion. >>>> +Typically a GPIO controller will set ``#gpio-cells=<2>``, with the first cell encoding the signal number, >>>> +and the second encoding flags for pin configuration. >> >> I don't see why we need to be vague and say "typically" here. It's >> better if we just >> mandate it. I think the few existing GPIO drivers using one cell are >> an historical >> artifact we should not standardize. We could just write "some older bindings use >> one cell" or so, and say that this is deprecated. >> >> (I see the flags are specified under best practices, good!) >> >>>> +GPIO Consumer Nodes >> >> Mostly looks good! >> >>>> +GPIO Consumer Example >>>> +~~~~~~~~~~~~~~~~~~~~~ >>>> + >>>> +.. code-block:: dts >>>> + >>>> + gpio1: gpio1 { >>>> + gpio-controller; >>>> + #gpio-cells = <2>; >>>> + }; >>>> + >>>> + gpio2: gpio2 { >>>> + gpio-controller; >>>> + #gpio-cells = <1>; >>>> + }; >> >> I think maybe we should cut that #gpio-cells = <1> example. >> Deprecate it IMO. >> >>>> + gpio-consumer { >>>> + enable-gpios = <&gpio2 2>; >> >> Ditto. Use a twocell example instead. >> >>>> + data-gpios = <&gpio1 12 0>, >>>> + <&gpio1 13 0>, >>>> + <&gpio1 14 0>, >>>> + <&gpio1 15 0>; >> >> This is good. >> >>>> +GPIO Controller Nodes >>>> +^^^^^^^^^^^^^^^^^^^^^ >>>> + >>>> +Every GPIO controller node must contain an empty *gpio-controller* property, >>>> +and a *gpio-cells* integer property, which indicates the number of cells in a >>>> +``single-gpio``. >>>> + >>>> +.. tabularcolumns:: | l c l J | >>>> +.. table:: GPIO Controller Properties >>>> + >>>> + =================== ===== ================= ================================================ >>>> + Property Name Usage Value Type Definition >>>> + =================== ===== ================= ================================================ >>>> + ``#gpio-cells`` R ``<u32>`` Specifies the number of ``<u32>`` cells to >>>> + represent the address in the ``reg`` property in >>>> + children of root. >> >> I would strongly recommend that this be <2>. >> >>>> + ``gpio-controller`` R ``<empty>`` Declares the device as a GPIO controller that >>>> + can be referenced using a ``single-gpio`` >>>> + ``ngpios`` OR ``<u32>`` Optional property describing the number >>>> + of GPIO signals provided by the controller. >>>> + This property may be used when the controller >>>> + provides fewer signals than the driver binding >>>> + supports. For example, a MMIO GPIO controller >>>> + with 32-bit registers might theoretically >>>> + support 32 GPIO signals, but only numbers 0 >>>> + through 11 wired up. In this case >>>> + "ngpios = <12>;" should be set. >> >> Good that you include this! >> >>>> + ``gpio-line-names`` O ``<stringlist>`` An array of names for each of the GPIO signals >>>> + provided by the controller. This name should be >>>> + the most meaningful producer name for the >>>> + system, such as a rail name indicating the >>>> + usage. Package names such as pin names are >>>> + discouraged. For example, "MMC-CD", >>>> + "Red LED Vdd" and "Ethernet RST" are reasonable >>>> + line names as they describe what the line is >>>> + used for. Names like "GPIO0" are discouraged. >>>> + Additionally, placeholder names are discouraged. >>>> + If a signal does not have a name defined, then >>>> + use "" (blank string) as the placeholder. >> >> Very nice. >> >>>> +GPIO Specifier Best Practices >>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>> + >>>> +How a ``gpio-specifier`` is encoded is defined by the GPIO controller binding, >>>> +and so it allows quite a bit of flexibility to encode extra information required >>>> +by the interrupt controller into the ``gpio-specifier`` itself. >>>> +However, most GPIO controllers encode the same information so it is strongly recommended >>>> +to follow established convention for ``gpio-specifier`` format. >>>> + >>>> +Established convention for ``gpio-specifier`` is to use either 2 or 3 cells to describe >>>> +a GPIO signal depending on whether or not the GPIO controller provides multiple "banks" >>>> +of GPIO registers. >> >> I don't understand this 3 cell idea here. >> >>>> +If the controller does use banks, then 3 cells should be used with the bank encoded >>>> +into the first cell, the pin within the bank in the second, and the flags in the third >>>> +cell. >> >> I would prefer not to encourage the use of bank specifiers. >> >> The best practice IMO is: >> >> (A) model the banks as one device node per bank with n (typically 32) >> lines per device/bank and reference with two cells (handle and flags). >> >> (B) represent all the GPIOs in the device as one long range, so if it is >> 6 banks of 32 lines, just expose a GPIO device, with two cells and >> GPIO numbers 0..191. >> >> This is the most common in the kernel and I don't see why we should bring >> in the three-cell pattern at all. It should be deprecated if present in bindings >> IMO. > > We have to allow for oddball h/w in the spec even if 3 cells usually > means you are doing it wrong. > >>>> +If the controller does not use banks, then 2 cells are sufficient; the first cell >>>> +encoding the signal number and the second encoding the flags. >> >> I disagree per above. >> >>>> +Standard encoding for the flags cell are as follows >> >> Awesome, thanks for wrapping this up in the spec. >> >>>> +Example >> >> Example is nice and to the point. >> >>>> +GPIO Hogging >>>> +~~~~~~~~~~~~ >>>> + >>>> +A GPIO controller may provide automatic configuration information for one or more GPIO >>>> +signals by adding ``GPIO hog`` child nodes. >>>> +GPIO hogging informs the GPIO controller driver that some pins must be configured in a >>>> +particular way at driver initialization time, without requiring a reference from a GPIO >>>> +consumer node. >> >> They even *MUST NOT* be referenced from consumer nodes. >> >> Hogs are hoggy hogs that hog around. Very greedily. >> >> So we need to say that hogs exclude a GPIO line from any use by >> any consumer node. >> >> There has been a lot of back-and-forth of another type of self-reference >> specifying initial values and/or directions to a GPIO line, later to be >> taken (or not) by some consumer. >> >> These discussions have stalled. It didn't lead anywhere. > > I would suggest we leave hogs out for now. I think the above needs to > be resolved first. I'm not a fan of the gpio hogs binding so much, but > I don't really have a better suggestion on how to handle it. > > Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree-spec" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
