[RFC PATCH] dt: add a binding review checklist

[RFC PATCH] dt: add a binding review checklist

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

I originally intended this to be a brief list of items to check when
reviewing a new DT binding. However, I appear to have become slightly
distracted and added in a bunch of background/justification verbiage
too!

Do people think this is going the right direction? Should I continue in
the current style and add more sections to the document, or strip it back
to be a much more plain list, perhaps something like:
https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
(which is where I got the idea for this)

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 .../bindings/binding-review-checklist.txt          | 179 +++++++++++++++++++++
 1 file changed, 179 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/binding-review-checklist.txt

diff --git a/Documentation/devicetree/bindings/binding-review-checklist.txt b/Documentation/devicetree/bindings/binding-review-checklist.txt
new file mode 100644
index 0000000..8c4b5bf
--- /dev/null
+++ b/Documentation/devicetree/bindings/binding-review-checklist.txt
@@ -0,0 +1,179 @@
+Reviewers' checklist for Device Tree bindings
+=============================================
+
+Introduction
+------------
+
+This document provides a set of guidelines for review of new Device Tree
+bindings. It is not (yet?) exhaustive. Good engineering judgement should be
+applied in addition to the requirements specified below.
+
+Much of this document is written from the perspective of defining bindings for
+individual chips, or hardware/IP blocks. However, many of the assertions here
+are valid when defining generic/super-class bindings for "subsystems" or
+device classes, such as "all I2C controllers", "all I2C clients", etc.
+
+What Device Tree Represents
+---------------------------
+
+Hardware Description
+~~~~~~~~~~~~~~~~~~~~
+
+Device Tree is a description of hardware, or firmware interfaces. It may
+describe device presence, type, location, associated configuration parameters
+that are purely derived from the hardware definition, and connectivity
+between components.
+
+The rest of this document uses the term "hardware" to refer to either a
+physical hardware device, or a firmware interface.
+
+For example, a DT might specify that a particular vendor's SHDCI controller is
+available, memory-mapped at address 0xc8000000, generates an interrupt that
+is routed to a specified active-low level-sensitive input on a specified IRQ
+controller, is fed by a specific clock generated by a specific clock source,
+and may detect card presence using a specific active-low GPIO hosted by a
+specific GPIO controller.
+
+No Representation of Policy
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Device tree is not a description of how to use the hardware, nor policy to
+be applied when using the hardware.
+
+For example, consider a system with two analog line-out audio jacks, where
+only one may be used at a time, with software control over which jack is
+active. The DT should describe the presence of both jacks, but should not
+specify the conditions under which to select when to use one jack or the
+other. That decision should be left up to the software running on the device.
+In this situation, however, it is reasonable for DT to identify which jack
+is which, so that software can usefully select between the two jacks.
+
+Equally, consider a system that is sold as two different products, one
+of which costs more because it allows the use of an SD card slot at a
+faster clock rate than the other, even though the hardware is actually
+identical. DT should not be used to impose the limit on the SD card
+clock rate purely for policy or differentiation reasons. However, if
+the PCB is physically different between the two products, and has been
+deliberately designed and qualified to support different maximum clock
+rates (e.g. differing trace width or impedance), it would be acceptable to
+represent this maximum supported clock rate in device tree.
+
+Operating System Agnostic
+~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Since Device tree is a pure description of hardware, the same bindings apply
+irrespective of the software running on the device. This implies that the same
+DT content should be passed to any software running on the device.
+
+Binding definitions should be self-contained, and not reference any particular
+operating system's documentation or code. Referencing OS-independent standards,
+such as USB or EHCI, is appropriate.
+
+Some exceptions are necessary, although these are generally limited to more
+system-level DT nodes such as /chosen. This is an area where engineering
+judgement may apply.
+
+Some bindings need to name or identify certain hardware properties. An example
+may be labels on keys in a matrix keyboard. Some numbering scheme is needed
+here, and in the absence of any OS-independent standard, it is reasonably to
+choose the numbering scheme of some particular operating system rather than
+inventing a new scheme. However, the full details of that scheme should be
+duplicated into the DT binding document (or associated header file) so that
+the binding definition remains stand-alone, and may be easily used by other
+operating systems.
+
+Vendor Prefix
+-------------
+
+A vendor prefix is simply a unique name for a (typically hardware-) vendor.
+This prefix is part of the entries in the compatible property, and names of
+binding-specific properties. The prefix serves to divide the name-space and
+avoid collisions between bindings from different vendors.
+
+All vendor prefix values must be documented in vendor-prefixes.txt.
+
+Valid values for a vendor prefix are:
+
+* A short, yet not obscure or obfuscated, representation of the vendor's name,
+  in lower-case. Stock tickers may be used in lower-case.
+* The vendor's stock ticker, in upper-case.
+
+Binding Structure
+-----------------
+
+A binding definition is currently a plain text file that contains:
+
+* A brief description of the hardware it describes, e.g. it's an I2C
+  controller, or an SPI controller, etc. Any unique or unusual features should
+  be explained in more depth.
+* Enough information to identify the hardware, and hence find the relevant
+  documentation for it to read alongside the binding definition (assuming such
+  documentation exists, and one has access to obtain it).
+* References any other generic/super-class bindings that this binding
+  "inherits" from, including their binding document filename.
+* For all properties the binding supports, defines:
+** The property name.
+** Whether the property is defined by, or is an instance of, some generic
+   binding, such as GPIO, IRQ, etc. If so, reference the binding document
+   filename that defines that binding.
+** Whether it is required or optional.
+** For optional properties, the implied value when the property is missing.
+** The type of the value.
+** The set or range of legal values for the property.
+** For properties that are lists, the order (if any) the entries must appear.
+* Typically contains a simple example to demonstrate the binding.
+
+Compatible Property
+-------------------
+
+A compatible value identifies a hardware module. It needs to identify the
+vendor (e.g. NVIDIA), type or name of device (e.g. I2C), and version of the
+device (e.g. an IP block version number or chip name). The following formats
+of compatible value are acceptable:
+
+* ${vendor},${device}-${version} (e.g. ti,omap4-i2c)
+* ${vendor},${version}-${device} (e.g. nvidia,tegra20-i2c)
+* ${vendor},${device}-${version} (e.g. synopsis,dwc3)
+
+Bindings for related hardware modules (e.g. all IP blocks in an SoC) should
+consistently use the same formatting option for their compatible values.
+
+A binding should enumerate all compatible values that it supports,
+both compatible values that imply a different software-visible programming interface, and
+compatible values that simply define device-specific instantiations of a
+common IP block. This provides a central database of all valid compatible
+values. This ensures that if a particular instance of an IP block is found
+to require a quirk to be activated, the binding document will authoritatively
+define the compatible value that software can use to identify it.
+
+Examples compatible values might be:
+
+* vendor,chip-i2c. First value defined.
+* vendor,chip2-i2c. 100% backwards-compatible with vendor,chip-i2c, yet
+  adds some optional new features.
+* vendor,chip3-i2c. Identical to vendor,chip2-i2c. This value exists only to
+  document the fact the IP block was instantiated into a different top-level
+  chip, to allow enabling any quirks required by that instantiation.
+
+Specifiers
+----------
+
+Many properties define a resource that is provided by one DT node and used by
+another. Any naming, formatting, and order of entry conventions for these
+properties should be defined by a "super-class" or "subsystem" binding.
+
+For example, some properties (such as reg, interrupts) are accessed by list
+index. Any reg-names or interrupt-names property is purely informational. The
+core definition of the regs or interrupts properties defines that these
+properties are accessed by index. Any binding that uses these properties must
+define how many entries these properties should contain (e.g. if multiple
+interrupts are generated by the hardware module or not), and which order
+the entries must appear.
+
+Other properties (such as clocks) have a mandatory associated names property
+(e.g. clock-names). In this case, the clock subsystem binding defines that
+clock names are first located in the clock-names property, then the index of
+the name is also used to index into the clocks property to find the actual
+clock definition. In this case, any binding that uses these properties must
+define the set of required and optional entries for the clock-names property.
+No specification of order of entries is possible.
-- 
1.8.1.5

Re: [RFC PATCH] dt: add a binding review checklist

[Mark Rutland]

Hi Stephen,

On Wed, Sep 04, 2013 at 11:42:12PM +0100, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> I originally intended this to be a brief list of items to check when
> reviewing a new DT binding. However, I appear to have become slightly
> distracted and added in a bunch of background/justification verbiage
> too!

Cheers for this!

I'd also had a go (and fell into the same verbiage trap). I could post
that in another reply for reference, some of it might be useful and
could probably be lifted directly.

> 
> Do people think this is going the right direction? Should I continue in
> the current style and add more sections to the document, or strip it back
> to be a much more plain list, perhaps something like:
> https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
> (which is where I got the idea for this)

I think this is going in the right direction, but I think it would make
sense to also have a basic checklist that people can go over
mechanically that detects big issues (e.g. unallocated vendor prefixes,
undocumented or ambiguous properties, undescribed HW inputs/outputs).

> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  .../bindings/binding-review-checklist.txt          | 179 +++++++++++++++++++++
>  1 file changed, 179 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/binding-review-checklist.txt
> 
> diff --git a/Documentation/devicetree/bindings/binding-review-checklist.txt b/Documentation/devicetree/bindings/binding-review-checklist.txt
> new file mode 100644
> index 0000000..8c4b5bf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/binding-review-checklist.txt
> @@ -0,0 +1,179 @@
> +Reviewers' checklist for Device Tree bindings
> +=============================================
> +
> +Introduction
> +------------
> +
> +This document provides a set of guidelines for review of new Device Tree
> +bindings. It is not (yet?) exhaustive. Good engineering judgement should be
> +applied in addition to the requirements specified below.
> +
> +Much of this document is written from the perspective of defining bindings for
> +individual chips, or hardware/IP blocks. However, many of the assertions here
> +are valid when defining generic/super-class bindings for "subsystems" or
> +device classes, such as "all I2C controllers", "all I2C clients", etc.
> +
> +What Device Tree Represents
> +---------------------------
> +
> +Hardware Description
> +~~~~~~~~~~~~~~~~~~~~
> +
> +Device Tree is a description of hardware, or firmware interfaces. It may
> +describe device presence, type, location, associated configuration parameters
> +that are purely derived from the hardware definition, and connectivity
> +between components.
> +
> +The rest of this document uses the term "hardware" to refer to either a
> +physical hardware device, or a firmware interface.
> +
> +For example, a DT might specify that a particular vendor's SHDCI controller is
> +available, memory-mapped at address 0xc8000000, generates an interrupt that
> +is routed to a specified active-low level-sensitive input on a specified IRQ
> +controller, is fed by a specific clock generated by a specific clock source,
> +and may detect card presence using a specific active-low GPIO hosted by a
> +specific GPIO controller.
> +
> +No Representation of Policy
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Device tree is not a description of how to use the hardware, nor policy to
> +be applied when using the hardware.
> +
> +For example, consider a system with two analog line-out audio jacks, where
> +only one may be used at a time, with software control over which jack is
> +active. The DT should describe the presence of both jacks, but should not
> +specify the conditions under which to select when to use one jack or the
> +other. That decision should be left up to the software running on the device.
> +In this situation, however, it is reasonable for DT to identify which jack
> +is which, so that software can usefully select between the two jacks.
> +
> +Equally, consider a system that is sold as two different products, one
> +of which costs more because it allows the use of an SD card slot at a
> +faster clock rate than the other, even though the hardware is actually
> +identical. DT should not be used to impose the limit on the SD card
> +clock rate purely for policy or differentiation reasons. However, if
> +the PCB is physically different between the two products, and has been
> +deliberately designed and qualified to support different maximum clock
> +rates (e.g. differing trace width or impedance), it would be acceptable to
> +represent this maximum supported clock rate in device tree.
> +
> +Operating System Agnostic
> +~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Since Device tree is a pure description of hardware, the same bindings apply
> +irrespective of the software running on the device. This implies that the same
> +DT content should be passed to any software running on the device.
> +
> +Binding definitions should be self-contained, and not reference any particular
> +operating system's documentation or code. Referencing OS-independent standards,
> +such as USB or EHCI, is appropriate.
> +
> +Some exceptions are necessary, although these are generally limited to more
> +system-level DT nodes such as /chosen. This is an area where engineering
> +judgement may apply.
> +
> +Some bindings need to name or identify certain hardware properties. An example
> +may be labels on keys in a matrix keyboard. Some numbering scheme is needed
> +here, and in the absence of any OS-independent standard, it is reasonably to
> +choose the numbering scheme of some particular operating system rather than
> +inventing a new scheme. However, the full details of that scheme should be
> +duplicated into the DT binding document (or associated header file) so that
> +the binding definition remains stand-alone, and may be easily used by other
> +operating systems.
> +
> +Vendor Prefix
> +-------------
> +
> +A vendor prefix is simply a unique name for a (typically hardware-) vendor.

s/hardware-/hardware/

> +This prefix is part of the entries in the compatible property, and names of
> +binding-specific properties. The prefix serves to divide the name-space and
> +avoid collisions between bindings from different vendors.
> +
> +All vendor prefix values must be documented in vendor-prefixes.txt.
> +
> +Valid values for a vendor prefix are:
> +
> +* A short, yet not obscure or obfuscated, representation of the vendor's name,
> +  in lower-case. Stock tickers may be used in lower-case.
> +* The vendor's stock ticker, in upper-case.

We've got plenty of stock ticker names in lower case. The only prefix I
can see documented with any upper case part is "GEFanuc" (though "SUNW"
is an obvious undocumented example...).

> +
> +Binding Structure
> +-----------------
> +
> +A binding definition is currently a plain text file that contains:
> +
> +* A brief description of the hardware it describes, e.g. it's an I2C
> +  controller, or an SPI controller, etc. Any unique or unusual features should
> +  be explained in more depth.
> +* Enough information to identify the hardware, and hence find the relevant
> +  documentation for it to read alongside the binding definition (assuming such
> +  documentation exists, and one has access to obtain it).
> +* References any other generic/super-class bindings that this binding
> +  "inherits" from, including their binding document filename.

s/References/References to/

> +* For all properties the binding supports, defines:
> +** The property name.
> +** Whether the property is defined by, or is an instance of, some generic
> +   binding, such as GPIO, IRQ, etc. If so, reference the binding document
> +   filename that defines that binding.
> +** Whether it is required or optional.
> +** For optional properties, the implied value when the property is missing.
> +** The type of the value.
> +** The set or range of legal values for the property.
> +** For properties that are lists, the order (if any) the entries must appear.

I'd note that this may be defined by reference to other properties. There
might be a ${X}-names style property defining each entry in the list, or
there might be other properties defining the list (e.g.
"#global-interrupts" in the "arm,smmu" binding).

> +* Typically contains a simple example to demonstrate the binding.
> +
> +Compatible Property
> +-------------------
> +
> +A compatible value identifies a hardware module. It needs to identify the
> +vendor (e.g. NVIDIA), type or name of device (e.g. I2C), and version of the
> +device (e.g. an IP block version number or chip name). The following formats
> +of compatible value are acceptable:
> +
> +* ${vendor},${device}-${version} (e.g. ti,omap4-i2c)
> +* ${vendor},${version}-${device} (e.g. nvidia,tegra20-i2c)
> +* ${vendor},${device}-${version} (e.g. synopsis,dwc3)

It would be nice to make it clear that the compatible string for a
device should (wherever possible) be the name of the specific IP block,
which isn't completely clear above (e.g. "arm,pl011" is preferred to
"arm,vexpress-v2m-serial"). It would be nice if we could avoid examples
with SoC names for this reason.

Obviously there will be SoC-specific devices that will have SoC names in
their bindings. But those bindings should be considered carefully.

> +
> +Bindings for related hardware modules (e.g. all IP blocks in an SoC) should
> +consistently use the same formatting option for their compatible values.
> +
> +A binding should enumerate all compatible values that it supports,
> +both compatible values that imply a different software-visible programming interface, and
> +compatible values that simply define device-specific instantiations of a
> +common IP block. This provides a central database of all valid compatible
> +values. This ensures that if a particular instance of an IP block is found
> +to require a quirk to be activated, the binding document will authoritatively
> +define the compatible value that software can use to identify it.
> +
> +Examples compatible values might be:
> +
> +* vendor,chip-i2c. First value defined.
> +* vendor,chip2-i2c. 100% backwards-compatible with vendor,chip-i2c, yet
> +  adds some optional new features.
> +* vendor,chip3-i2c. Identical to vendor,chip2-i2c. This value exists only to
> +  document the fact the IP block was instantiated into a different top-level
> +  chip, to allow enabling any quirks required by that instantiation.
> +
> +Specifiers
> +----------
> +
> +Many properties define a resource that is provided by one DT node and used by
> +another. Any naming, formatting, and order of entry conventions for these
> +properties should be defined by a "super-class" or "subsystem" binding.
> +
> +For example, some properties (such as reg, interrupts) are accessed by list
> +index. Any reg-names or interrupt-names property is purely informational. The
> +core definition of the regs or interrupts properties defines that these
> +properties are accessed by index. Any binding that uses these properties must
> +define how many entries these properties should contain (e.g. if multiple
> +interrupts are generated by the hardware module or not), and which order
> +the entries must appear.

I don't agree that any reg-names or interrupt-names property is purely
informational. I think this is certainly binding-specific, but we have
drivers grabbing values by name, and in some cases there is no defined
ordering and names are required for disambiguation and/or handling
missing entries.

> +
> +Other properties (such as clocks) have a mandatory associated names property
> +(e.g. clock-names). In this case, the clock subsystem binding defines that
> +clock names are first located in the clock-names property, then the index of
> +the name is also used to index into the clocks property to find the actual
> +clock definition. In this case, any binding that uses these properties must
> +define the set of required and optional entries for the clock-names property.
> +No specification of order of entries is possible.

The primecell bindings are in violation of the above (but the code is
not), and there may be other cases I'm not aware of. Ideally the above
would be true, but I'm not sure we can document it as such (though we
can enforce such rules for new bindings).

Thanks,
Mark.

Re: [RFC PATCH] dt: add a binding review checklist

[Stephen Warren]

On 09/05/2013 05:19 AM, Mark Rutland wrote:
> Hi Stephen,
> 
> On Wed, Sep 04, 2013 at 11:42:12PM +0100, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> I originally intended this to be a brief list of items to check when
>> reviewing a new DT binding. However, I appear to have become slightly
>> distracted and added in a bunch of background/justification verbiage
>> too!
>>
>> Do people think this is going the right direction? Should I continue in
>> the current style and add more sections to the document, or strip it back
>> to be a much more plain list, perhaps something like:
>> https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
>> (which is where I got the idea for this)
> 
> I think this is going in the right direction, but I think it would make
> sense to also have a basic checklist that people can go over
> mechanically that detects big issues (e.g. unallocated vendor prefixes,
> undocumented or ambiguous properties, undescribed HW inputs/outputs).

Perhaps we should do what Fedora does; rename this
binding-review-guidelines.txt, and have a simpler
binding-review-checklist.txt which is just a set of tick-boxes with
fairly short descriptions?

>> +Valid values for a vendor prefix are:
>> +
>> +* A short, yet not obscure or obfuscated, representation of the vendor's name,
>> +  in lower-case. Stock tickers may be used in lower-case.
>> +* The vendor's stock ticker, in upper-case.
> 
> We've got plenty of stock ticker names in lower case. The only prefix I
> can see documented with any upper case part is "GEFanuc" (though "SUNW"
> is an obvious undocumented example...).

Should we document what we hope to be standard procedure going forward,
or also write the document to encompass existing bindings? Perhaps we
just shouldn't mention upper-case stock tickers at all? I did because of
David's comments in:

http://www.spinics.net/lists/devicetree/msg00097.html

>> +* For all properties the binding supports, defines:
>> +** The property name.
>> +** Whether the property is defined by, or is an instance of, some generic
>> +   binding, such as GPIO, IRQ, etc. If so, reference the binding document
>> +   filename that defines that binding.
>> +** Whether it is required or optional.
>> +** For optional properties, the implied value when the property is missing.
>> +** The type of the value.
>> +** The set or range of legal values for the property.
>> +** For properties that are lists, the order (if any) the entries must appear.
> 
> I'd note that this may be defined by reference to other properties. There
> might be a ${X}-names style property defining each entry in the list, or
> there might be other properties defining the list (e.g.
> "#global-interrupts" in the "arm,smmu" binding).

Yes. I expand on this in the "specifiers" section below, but perhaps
it'd be a good idea to briefly mention that here too.

>> +* Typically contains a simple example to demonstrate the binding.
>> +
>> +Compatible Property
>> +-------------------
>> +
>> +A compatible value identifies a hardware module. It needs to identify the
>> +vendor (e.g. NVIDIA), type or name of device (e.g. I2C), and version of the
>> +device (e.g. an IP block version number or chip name). The following formats
>> +of compatible value are acceptable:
>> +
>> +* ${vendor},${device}-${version} (e.g. ti,omap4-i2c)
>> +* ${vendor},${version}-${device} (e.g. nvidia,tegra20-i2c)
>> +* ${vendor},${device}-${version} (e.g. synopsis,dwc3)
> 
> It would be nice to make it clear that the compatible string for a
> device should (wherever possible) be the name of the specific IP block,
> which isn't completely clear above (e.g. "arm,pl011" is preferred to
> "arm,vexpress-v2m-serial"). It would be nice if we could avoid examples
> with SoC names for this reason.

I guess I should expand upon the difference between the IP block
version, perhaps:

nvidia,i2c-v1 # NV I2C IP block version 1

and the "instance" version, perhaps:

nvidia,i2c-tegra20 # That same thing, but when it's part of Tegra20 SoC.

since I believe we're agreed that both should be present; the IP block
compatible value for a driver to bind against (typically), but the
"instance" value to allow quirks/WARs to be enabled if the SoC
integration causes some issue.

>> +Specifiers
>> +----------
>> +
>> +Many properties define a resource that is provided by one DT node and used by
>> +another. Any naming, formatting, and order of entry conventions for these
>> +properties should be defined by a "super-class" or "subsystem" binding.
>> +
>> +For example, some properties (such as reg, interrupts) are accessed by list
>> +index. Any reg-names or interrupt-names property is purely informational. The
>> +core definition of the regs or interrupts properties defines that these
>> +properties are accessed by index. Any binding that uses these properties must
>> +define how many entries these properties should contain (e.g. if multiple
>> +interrupts are generated by the hardware module or not), and which order
>> +the entries must appear.
> 
> I don't agree that any reg-names or interrupt-names property is purely
> informational. I think this is certainly binding-specific, but we have
> drivers grabbing values by name, and in some cases there is no defined
> ordering and names are required for disambiguation and/or handling
> missing entries.

Yes, part of posting this doc was to agree on the rules going forward:-)

I believe that Grant has asserted in the past that the ordering rules
are defined by the property itself, so reg/interrupt are always indexed,
whereas clocks is always looked up via clock-names. However, I know that
in the past we've discussed that individual bindings get to decide which
option(s) are allowed, irrespective of which property, so we could allow
reg/interrupts to be looked up by name, but clocks to be looked up by
index. Should we just decide to be flexible and allow whatever
individual bindings want here? Do we need to get an ack from Grant?

>> +Other properties (such as clocks) have a mandatory associated names property
>> +(e.g. clock-names). In this case, the clock subsystem binding defines that
>> +clock names are first located in the clock-names property, then the index of
>> +the name is also used to index into the clocks property to find the actual
>> +clock definition. In this case, any binding that uses these properties must
>> +define the set of required and optional entries for the clock-names property.
>> +No specification of order of entries is possible.
> 
> The primecell bindings are in violation of the above (but the code is
> not), and there may be other cases I'm not aware of. Ideally the above
> would be true, but I'm not sure we can document it as such (though we
> can enforce such rules for new bindings).

OK - this brings up the same question as above. Do we document new
practice, or attempt to encompass existing bindings? Documenting
required new practice would keep things simpler, although if we do that,
I should add a note at the top of the file to explain this.

Re: [RFC PATCH] dt: add a binding review checklist

[Thomas Petazzoni]

Mark, Stephen,

On Thu, 5 Sep 2013 12:19:18 +0100, Mark Rutland wrote:

> > +Compatible Property
> > +-------------------
> > +
> > +A compatible value identifies a hardware module. It needs to identify the
> > +vendor (e.g. NVIDIA), type or name of device (e.g. I2C), and version of the
> > +device (e.g. an IP block version number or chip name). The following formats
> > +of compatible value are acceptable:
> > +
> > +* ${vendor},${device}-${version} (e.g. ti,omap4-i2c)
> > +* ${vendor},${version}-${device} (e.g. nvidia,tegra20-i2c)
> > +* ${vendor},${device}-${version} (e.g. synopsis,dwc3)
> 
> It would be nice to make it clear that the compatible string for a
> device should (wherever possible) be the name of the specific IP block,
> which isn't completely clear above (e.g. "arm,pl011" is preferred to
> "arm,vexpress-v2m-serial"). It would be nice if we could avoid examples
> with SoC names for this reason.
> 
> Obviously there will be SoC-specific devices that will have SoC names in
> their bindings. But those bindings should be considered carefully.

I agree that it would be nice to make it clear that using the name of
an SoC family in the compatible string is not a good idea, and that
instead the name of the particular SoC that originally introduced the
IP block should be used. I.e nvidia,tegra20-i2c is fine, but
nvidia,tegra-i2c is not, because we have no idea what I2C controllers
will be used on future Tegra SoCs.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [RFC PATCH] dt: add a binding review checklist

[Sebastian Hesselbarth]

On 09/06/13 10:09, Thomas Petazzoni wrote:
> On Thu, 5 Sep 2013 12:19:18 +0100, Mark Rutland wrote:
 >> On Wed, Sep 04, 2013 at 11:42:12PM +0100, Stephen Warren wrote:
>>> +Compatible Property
>>> +-------------------
>>> +
>>> +A compatible value identifies a hardware module. It needs to identify the
>>> +vendor (e.g. NVIDIA), type or name of device (e.g. I2C), and version of the
>>> +device (e.g. an IP block version number or chip name). The following formats
>>> +of compatible value are acceptable:
>>> +
>>> +* ${vendor},${device}-${version} (e.g. ti,omap4-i2c)
>>> +* ${vendor},${version}-${device} (e.g. nvidia,tegra20-i2c)
>>> +* ${vendor},${device}-${version} (e.g. synopsis,dwc3)

BTW, the examples above look strange and rather should be

* ${vendor},${device}-${version} (e.g. ti,i2c-omap4)
* ${vendor},${version}-${device} (e.g. nvidia,tegra20-i2c)
* ${vendor},${device} (e.g. synopsis,dwc3)

>> It would be nice to make it clear that the compatible string for a
>> device should (wherever possible) be the name of the specific IP block,
>> which isn't completely clear above (e.g. "arm,pl011" is preferred to
>> "arm,vexpress-v2m-serial"). It would be nice if we could avoid examples
>> with SoC names for this reason.
>>
>> Obviously there will be SoC-specific devices that will have SoC names in
>> their bindings. But those bindings should be considered carefully.
>
> I agree that it would be nice to make it clear that using the name of
> an SoC family in the compatible string is not a good idea, and that
> instead the name of the particular SoC that originally introduced the
> IP block should be used. I.e nvidia,tegra20-i2c is fine, but
> nvidia,tegra-i2c is not, because we have no idea what I2C controllers
> will be used on future Tegra SoCs.

Speaking of "the particular SoC that originally introduced the IP
block", do you mean physically first or supported first?

Working on Armada 1500 support, there will be compatibles for
88de3100, while 88de3010 was (maybe) released before. It is just that
no one ever worked on that and I am not going to flip through hundreds
of LOC, just to find out which one was first.

Also, IP is reused so often and possibly over decades with minor
improvements. Just take mv643xx_eth as an example, it was introduced
as a up to 4 port ethernet ip in powerpc system controllers. Marvell
decided to reuse it as 1 port ethernet ip in its Orion SoCs.

Strictly speaking, you would have "marvell,kirkwood-eth", 
"marvell,mv64300-eth" (or was mv64360 first? ;-) ).
Or even "marvell,88f6281-eth", "marvell,mv64300-eth" because 88f6282
has fixed something you not know of, yet.
Or even "marvell,88f6281a0-eth" because A1 stepping... I think you get
the point.
Or should you rather drop "marvell,mv64300-eth" compatibility because
the ip has evolved enough to call it a different ip?

Is the compatibility just by the fact, that we look at the _current_
_linux_ driver and see no difference - just because the driver ignored
the differences for ages?

As a vendor it may be easy to find _the_ best compatible, for others
it's not. Unclear datasheets, weird numbering, hidden ip reuse, IMHO
this should be a really weak rule in any way.

Sebastian

Re: [RFC PATCH] dt: add a binding review checklist

[Mark Rutland]

On Thu, Sep 05, 2013 at 10:35:22PM +0100, Stephen Warren wrote:
> On 09/05/2013 05:19 AM, Mark Rutland wrote:
> > Hi Stephen,
> > 
> > On Wed, Sep 04, 2013 at 11:42:12PM +0100, Stephen Warren wrote:
> >> From: Stephen Warren <swarren@nvidia.com>
> >>
> >> I originally intended this to be a brief list of items to check when
> >> reviewing a new DT binding. However, I appear to have become slightly
> >> distracted and added in a bunch of background/justification verbiage
> >> too!
> >>
> >> Do people think this is going the right direction? Should I continue in
> >> the current style and add more sections to the document, or strip it back
> >> to be a much more plain list, perhaps something like:
> >> https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
> >> (which is where I got the idea for this)
> > 
> > I think this is going in the right direction, but I think it would make
> > sense to also have a basic checklist that people can go over
> > mechanically that detects big issues (e.g. unallocated vendor prefixes,
> > undocumented or ambiguous properties, undescribed HW inputs/outputs).
> 
> Perhaps we should do what Fedora does; rename this
> binding-review-guidelines.txt, and have a simpler
> binding-review-checklist.txt which is just a set of tick-boxes with
> fairly short descriptions?

That would make sense to me.

> 
> >> +Valid values for a vendor prefix are:
> >> +
> >> +* A short, yet not obscure or obfuscated, representation of the vendor's name,
> >> +  in lower-case. Stock tickers may be used in lower-case.
> >> +* The vendor's stock ticker, in upper-case.
> > 
> > We've got plenty of stock ticker names in lower case. The only prefix I
> > can see documented with any upper case part is "GEFanuc" (though "SUNW"
> > is an obvious undocumented example...).
> 
> Should we document what we hope to be standard procedure going forward,
> or also write the document to encompass existing bindings? Perhaps we
> just shouldn't mention upper-case stock tickers at all? I did because of
> David's comments in:
> 
> http://www.spinics.net/lists/devicetree/msg00097.html

I think the key point is that a string is stable once defined, I'm not
sure the distinction between upper/lower case adds anything.

> 
> >> +* For all properties the binding supports, defines:
> >> +** The property name.
> >> +** Whether the property is defined by, or is an instance of, some generic
> >> +   binding, such as GPIO, IRQ, etc. If so, reference the binding document
> >> +   filename that defines that binding.
> >> +** Whether it is required or optional.
> >> +** For optional properties, the implied value when the property is missing.
> >> +** The type of the value.
> >> +** The set or range of legal values for the property.
> >> +** For properties that are lists, the order (if any) the entries must appear.
> > 
> > I'd note that this may be defined by reference to other properties. There
> > might be a ${X}-names style property defining each entry in the list, or
> > there might be other properties defining the list (e.g.
> > "#global-interrupts" in the "arm,smmu" binding).
> 
> Yes. I expand on this in the "specifiers" section below, but perhaps
> it'd be a good idea to briefly mention that here too.
> 
> >> +* Typically contains a simple example to demonstrate the binding.
> >> +
> >> +Compatible Property
> >> +-------------------
> >> +
> >> +A compatible value identifies a hardware module. It needs to identify the
> >> +vendor (e.g. NVIDIA), type or name of device (e.g. I2C), and version of the
> >> +device (e.g. an IP block version number or chip name). The following formats
> >> +of compatible value are acceptable:
> >> +
> >> +* ${vendor},${device}-${version} (e.g. ti,omap4-i2c)
> >> +* ${vendor},${version}-${device} (e.g. nvidia,tegra20-i2c)
> >> +* ${vendor},${device}-${version} (e.g. synopsis,dwc3)
> > 
> > It would be nice to make it clear that the compatible string for a
> > device should (wherever possible) be the name of the specific IP block,
> > which isn't completely clear above (e.g. "arm,pl011" is preferred to
> > "arm,vexpress-v2m-serial"). It would be nice if we could avoid examples
> > with SoC names for this reason.
> 
> I guess I should expand upon the difference between the IP block
> version, perhaps:
> 
> nvidia,i2c-v1 # NV I2C IP block version 1
> 
> and the "instance" version, perhaps:
> 
> nvidia,i2c-tegra20 # That same thing, but when it's part of Tegra20 SoC.
> 
> since I believe we're agreed that both should be present; the IP block
> compatible value for a driver to bind against (typically), but the
> "instance" value to allow quirks/WARs to be enabled if the SoC
> integration causes some issue.

That sounds like a good idea. Ideally, a platform shouldn't require a
SoC-specific compatible entry (or we're just distributing board file
entries into each driver...).

What kind of quirks/differences are we handling currently with
"instance" compatible strings?

> 
> >> +Specifiers
> >> +----------
> >> +
> >> +Many properties define a resource that is provided by one DT node and used by
> >> +another. Any naming, formatting, and order of entry conventions for these
> >> +properties should be defined by a "super-class" or "subsystem" binding.
> >> +
> >> +For example, some properties (such as reg, interrupts) are accessed by list
> >> +index. Any reg-names or interrupt-names property is purely informational. The
> >> +core definition of the regs or interrupts properties defines that these
> >> +properties are accessed by index. Any binding that uses these properties must
> >> +define how many entries these properties should contain (e.g. if multiple
> >> +interrupts are generated by the hardware module or not), and which order
> >> +the entries must appear.
> > 
> > I don't agree that any reg-names or interrupt-names property is purely
> > informational. I think this is certainly binding-specific, but we have
> > drivers grabbing values by name, and in some cases there is no defined
> > ordering and names are required for disambiguation and/or handling
> > missing entries.
> 
> Yes, part of posting this doc was to agree on the rules going forward:-)
> 
> I believe that Grant has asserted in the past that the ordering rules
> are defined by the property itself, so reg/interrupt are always indexed,
> whereas clocks is always looked up via clock-names. However, I know that
> in the past we've discussed that individual bindings get to decide which
> option(s) are allowed, irrespective of which property, so we could allow
> reg/interrupts to be looked up by name, but clocks to be looked up by
> index. Should we just decide to be flexible and allow whatever
> individual bindings want here? Do we need to get an ack from Grant?

We should aim for consistency (e.g. all clocks *should* be referenced by
name), but inevitably there are going to be cases where that breaks down
(e.g. an IP block with a configurable number of clock inputs). Given
that and the existing legacy, I think this needs to be described in
particular binding documents.

At binding creation time we need to consider how a device may be
integrated. Interrupts often gets OR'd or thrown away by integrators, so
preferring interrupt-names to an ordered list makes sense (though there
will be cases with configurable IP blocks where another mechanism for
disambiguating interrupts entries may be necessary instead).

We also need to consider how future revisions may extend a device -- new
inputs and outputs may be added, but the device may be otherwise
backwards compatible. Using ${X}-names properties allows an accurate
description of the set of resources wired up, whether they are new and
unused or old and missing. That way we have a greater chance of a new
device working with an older kernel (using a driver for an older hw
revision).

For the above reasons, I think preferring the presence of ${X}-names in
bindings for devices with more than one entry in some property list
makes sense, but it needs to be dealt with on a case-by-case basis.
What I'm saying we need is a mechanism for disambiguating property lists
(which may or may not be a ${X}-names property).

> 
> >> +Other properties (such as clocks) have a mandatory associated names property
> >> +(e.g. clock-names). In this case, the clock subsystem binding defines that
> >> +clock names are first located in the clock-names property, then the index of
> >> +the name is also used to index into the clocks property to find the actual
> >> +clock definition. In this case, any binding that uses these properties must
> >> +define the set of required and optional entries for the clock-names property.
> >> +No specification of order of entries is possible.
> > 
> > The primecell bindings are in violation of the above (but the code is
> > not), and there may be other cases I'm not aware of. Ideally the above
> > would be true, but I'm not sure we can document it as such (though we
> > can enforce such rules for new bindings).
> 
> OK - this brings up the same question as above. Do we document new
> practice, or attempt to encompass existing bindings? Documenting
> required new practice would keep things simpler, although if we do that,
> I should add a note at the top of the file to explain this.
> 

We should definitely define best practices, even if they don't match
existing bindings. Whether or not they're required is a grey area, but
that can come from review -- these documents don't override a proper
review.

Thanks,
Mark.

Re: [RFC PATCH] dt: add a binding review checklist

[Stephen Warren]

On 09/06/2013 03:14 AM, Sebastian Hesselbarth wrote:
> On 09/06/13 10:09, Thomas Petazzoni wrote:
>> On Thu, 5 Sep 2013 12:19:18 +0100, Mark Rutland wrote:
>>> On Wed, Sep 04, 2013 at 11:42:12PM +0100, Stephen Warren wrote:
>>>> +Compatible Property
>>>> +-------------------
>>>> +
>>>> +A compatible value identifies a hardware module. It needs to
>>>> identify the
>>>> +vendor (e.g. NVIDIA), type or name of device (e.g. I2C), and
>>>> version of the
>>>> +device (e.g. an IP block version number or chip name). The
>>>> following formats
>>>> +of compatible value are acceptable:
>>>> +
>>>> +* ${vendor},${device}-${version} (e.g. ti,omap4-i2c)
>>>> +* ${vendor},${version}-${device} (e.g. nvidia,tegra20-i2c)
>>>> +* ${vendor},${device}-${version} (e.g. synopsis,dwc3)
> 
> BTW, the examples above look strange and rather should be
> 
> * ${vendor},${device}-${version} (e.g. ti,i2c-omap4)
> * ${vendor},${version}-${device} (e.g. nvidia,tegra20-i2c)
> * ${vendor},${device} (e.g. synopsis,dwc3)
> 
>>> It would be nice to make it clear that the compatible string for a
>>> device should (wherever possible) be the name of the specific IP block,
>>> which isn't completely clear above (e.g. "arm,pl011" is preferred to
>>> "arm,vexpress-v2m-serial"). It would be nice if we could avoid examples
>>> with SoC names for this reason.
>>>
>>> Obviously there will be SoC-specific devices that will have SoC names in
>>> their bindings. But those bindings should be considered carefully.
>>
>> I agree that it would be nice to make it clear that using the name of
>> an SoC family in the compatible string is not a good idea, and that
>> instead the name of the particular SoC that originally introduced the
>> IP block should be used. I.e nvidia,tegra20-i2c is fine, but
>> nvidia,tegra-i2c is not, because we have no idea what I2C controllers
>> will be used on future Tegra SoCs.
> 
> Speaking of "the particular SoC that originally introduced the IP
> block", do you mean physically first or supported first?

I think preferably whichever HW shipped (or was designed?) first that
contained the IP.

Certainly for HW where the community has developed the DT binding
without good visibility into the range of vendor HW, and without vendor
engagement during binding development, then picking the first chip that
a binding was written for would be about as good as we can do.

...
> Also, IP is reused so often and possibly over decades with minor
> improvements. Just take mv643xx_eth as an example, it was introduced
> as a up to 4 port ethernet ip in powerpc system controllers. Marvell
> decided to reuse it as 1 port ethernet ip in its Orion SoCs.

Port count (unless encoded into a separate DT property) sounds like
something that would require a separate compatible value, since the
driver needs some way to know how many ports are present.

...
> Is the compatibility just by the fact, that we look at the _current_
> _linux_ driver and see no difference - just because the driver ignored
> the differences for ages?

compatible should be based on the entire SW-visible interface to the HW.
The fact that one particular OS's driver didn't use some feature in the
HW and hence wasn't affected by a HW change between two HW versions does
not remove the need to give those two HW versions different compatible
values.

Put another way, if a (hypothetical) driver that used every feature of
HW version 1 to the full could work 100% successfully on HW version 2,
then those HW versions are compatible. HW version 2 can add new features
and still be compatible with HW version 1. HW version 2 cannot remove or
modify features (in a non-compatible and SW-/user-visible fashion) and
still be compatible.

When unsure, I suspect it's better to err on the side of adding new
compatible values rather than assuming compatibility of new HW versions
with old compatible values.

Re: [RFC PATCH] dt: add a binding review checklist

[Stephen Warren]

On 09/06/2013 04:50 AM, Mark Rutland wrote:
> On Thu, Sep 05, 2013 at 10:35:22PM +0100, Stephen Warren wrote:
>> On 09/05/2013 05:19 AM, Mark Rutland wrote:
>>> On Wed, Sep 04, 2013 at 11:42:12PM +0100, Stephen Warren wrote:

...
>>>> +Valid values for a vendor prefix are:
>>>> +
>>>> +* A short, yet not obscure or obfuscated, representation of the vendor's name,
>>>> +  in lower-case. Stock tickers may be used in lower-case.
>>>> +* The vendor's stock ticker, in upper-case.
>>>
>>> We've got plenty of stock ticker names in lower case. The only prefix I
>>> can see documented with any upper case part is "GEFanuc" (though "SUNW"
>>> is an obvious undocumented example...).
>>
>> Should we document what we hope to be standard procedure going forward,
>> or also write the document to encompass existing bindings? Perhaps we
>> just shouldn't mention upper-case stock tickers at all? I did because of
>> David's comments in:
>>
>> http://www.spinics.net/lists/devicetree/msg00097.html
> 
> I think the key point is that a string is stable once defined, I'm not
> sure the distinction between upper/lower case adds anything.

Should we just say that going forward, prefixes are lower-case then?

...
>>>> +Compatible Property
...
>> I guess I should expand upon the difference between the IP block
>> version, perhaps:
>>
>> nvidia,i2c-v1 # NV I2C IP block version 1
>>
>> and the "instance" version, perhaps:
>>
>> nvidia,i2c-tegra20 # That same thing, but when it's part of Tegra20 SoC.
>>
>> since I believe we're agreed that both should be present; the IP block
>> compatible value for a driver to bind against (typically), but the
>> "instance" value to allow quirks/WARs to be enabled if the SoC
>> integration causes some issue.
> 
> That sounds like a good idea. Ideally, a platform shouldn't require a
> SoC-specific compatible entry (or we're just distributing board file
> entries into each driver...).
> 
> What kind of quirks/differences are we handling currently with
> "instance" compatible strings?

I'm not actually aware of any, but I'm certainly not familiar enough
with every driver to guarantee that:-) This is just a rule that I've
seen discussed a few times.

Perhaps we should just drop the rule, and tell people to look at the
root node compatible value[1] if they need to quirk on some integration
issue.

[1] for on-SoC peripherals; for MFD components, presumably the root node
of the MFD device? I'm not sure how that works if a driver has no idea
whether the device is embedded into an SoC, or is in some external MMIO
peripheral, and hence has no idea which node to look at...

...
> For the above reasons, I think preferring the presence of ${X}-names in
> bindings for devices with more than one entry in some property list
> makes sense, but it needs to be dealt with on a case-by-case basis.

Well, given the future extensibility argument (that I didn't quote),
your statement above (that I did quote) implies we should always use
${X}-names going forward, since you never know that some future HW
revision won't add some extra potentially optional interrupts/..., and
hence all bindings should allow for multiple interrupts from the start.