[U-Boot] [PATCH] spi: Tegra: add device tree binding doc for SPI and QSPI

[Stephen Warren]

On 10/23/2015 11:52 AM, Tom Warren wrote:
> Stephen,
>
> Stephen Warren wrote at Friday, October 23, 2015 10:26 AM:
>> On 10/23/2015 11:11 AM, Tom Warren wrote:
>>> This patch adds the device tree binding doc for the Tegra114 SPI
>>> controller and the Tegra210 QSPI controller.
>>
>> Initially, this should be sent as a Linux kernel patch, since the kernel currently
>> holds the definitive repository for DT bindings.
>>
>> The binding should be based on the Tegra SPI binding present there, not on the
>> non-standard binding for Tegra114 SPI that's evidently in the U-Boot tree.
 >
> This is a copy of the 'nvidia,tegra114-spi.txt' binding in the kernel
 > (Documentation/devicetree/bindings/spi/). I removed the dma and reset
> fields, since they aren't required (or used) in U-Boot. I then added
 > QSPI for T210, and named the file spi-tegra.txt. There wasn't a U-Boot
 > SPI binding doc in U-Boot to start with.

Which kernel release did you use? The content in kernel git tag 
next-20151002, has been cleaned up quite a bit I suspect.

(Note that in that tag, the existing nvidia,tegra114-spi binding has one 
layout issue; I'd expect the clocks property to be documented 
immediately after the clock-names property. I'm not sure why it's not 
right now. This is just an FYI if you copy that file.)

Re "since they aren't required (or used) in U-Boot". A binding should be 
a description of the HW, and not influence by any particular OS or SW 
stack's design or implementation. If some feature exists in HW, it 
should be described in the binding, even if some particular SW stack 
isn't going to immediately make use of it. There's some leeway for 
optional features; if any arbitrary SW stack could make useful use of 
the HW without a particular feature, a simple binding could be submitted 
first without describing that feature, then enhanced later to add it. 
This avoids bike-shedding re: the design of some esoteric feature from 
blocking a simple binding being accepted. However, basic features that 
are simple, common, well-understood, and widely used (e.g. resets, DMA), 
should invariably be described right from the start.

>> That would imply sending the patch to the people/lists listed in the following
>> Linux kernel MAINTAINERS entry for DT bindings, plus at least the Tegra
>> mailing list and maintainers too:
>>
>> OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
>> M:      Rob Herring <robh+dt at kernel.org>
>> M:      Pawel Moll <pawel.moll at arm.com>
>> M:      Mark Rutland <mark.rutland at arm.com>
>> M:      Ian Campbell <ijc+devicetree at hellion.org.uk>
>> M:      Kumar Gala <galak at codeaurora.org>
>> L:      devicetree at vger.kernel.org
 >
> Since this is basically a copy of a kernel binding doc, I didn't know that
 > was necessary. Is that policy for U-Boot binding docs?

All bindings need to be reviewed by the DT binding review process. That 
currently involves submitting a Linux kernel patch to add the 
documentation of the binding to the kernel source tree. There have been 
discussions about moving the DT binding docs into a separate standalone 
repo with a seprate review process, but that hasn't happened yet. This 
is all policy for DT, so nothing to do with U-Boot or Linux or other SW.

> Jagan - you have a few of these in the SPI bindings - did you have
 > them reviewed by kernel/DT folk first?

I don't know about those specifically, but historical usage of DT in 
U-Boot has been extremely lax about standards, and has tended to diverge 
from the official set of bindings, currently stored in the kernel git tree.

> Regardless, I'll resend with those people/lists in CC. Which Tegra ML/maintainers
 > did you also want in there?

Generally, Linux kernel patches should be sent to whatever 
./scripts/get_maintainers.pl says, although in some cases you'd want to 
strip out some irrelevant people/lists so as not to make your CC too 
large. Here's the entry for Tegra:

TEGRA ARCHITECTURE SUPPORT
M:      Stephen Warren <swarren at wwwdotorg.org>
M:      Thierry Reding <thierry.reding at gmail.com>
M:      Alexandre Courbot <gnurou at gmail.com>
L:      linux-tegra at vger.kernel.org

You should probably add in the ARM linux kernel mailing list but not 
bother with the main Linux kernel mailing list or general documentation 
people (the DT-binding-specific reviewers should be enough for this).

>>> diff --git a/doc/device-tree-bindings/spi/spi-tegra.txt
>>> b/doc/device-tree-bindings/spi/spi-tegra.txt
>>> new file mode 100644
>>> index 0000000..e215efe
>>> --- /dev/null
>>> +++ b/doc/device-tree-bindings/spi/spi-tegra.txt
>>> @@ -0,0 +1,47 @@
>>> +NVIDIA Tegra114 SPI controller.
>>
>> Isn't this intended to be a binding for the QSPI controller?
 >
> Since there was no SPI binding, I included the Tegra114 SPI binding here.
 > Note that other QSPI bindings exist here, for instance
> spi-cadence.txt.If you'd like two separate binding docs, I can do that,
 > but this seemed more efficient.

Any DT binding docs that get imported into the U-Boot source tree should 
be exactly identical to the official docs that are currently located in 
the kernel git tree. This means all of content, directory layout, and 
filenames.

For the existing SPI bindings, you could just copy the existing file 
from the kernel tree to the U-Boot tree. However, where the kernel and 
U-Boot use identical bindings (which should be everywhere but currently 
isn't), I don't see the point of duplicating the doc in the U-Boot tree 
at all, since it just leaves two separate copies that likely won't be 
kept in sync.

Generally, there is a separate DT binding doc per type of incompatible 
HW module. So, if the Tegra SPI and QSPI HW modules need to be 
programmed differently, there would be two bindings. However, if the 
programming for e.g. Tegra114 and Tegra124 (regular) SPI HW modules are 
identical or backwards-compatible or a superset, then those two HW 
modules can share a binding definition, e.g. with a note in the 
compatible value definition describing a different value for each 
version of that HW module.

>>> +Recommended properties:
>>> +- spi-max-frequency: Definition as per
>>> +                     doc/device-tree-bindings/spi/spi-bus.txt
>>
>> That should use a relative path ("spi-bus.txt"), so that the same text applies
>> irrespective of whether the file is contained within the Linux kernel or U-Boot
>> source trees.
 >
> Linux binding uses an absolute path (Documentation/devicetree/bindings/spi/spi-bus.txt),
 > so I copied their usage to point to our spi-bus.txt. I can change to 
a relative path.

Unfortunately, there are plenty of bad examples that were written before 
we realized some things didn't work well:-( The more bindings get fixed 
when first submitted, the lower the chance of randomly picking the wrong 
example:-)

Sorry there's a lot of tribal knowledge re: DT bindings. The first is 
the hardest; the next hopefully much simpler.