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

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

[Tom Warren]

This patch adds the device tree binding doc for the Tegra114
SPI controller and the Tegra210 QSPI controller.

Signed-off-by: Tom Warren <twarren@nvidia.com>
---
 doc/device-tree-bindings/spi/spi-tegra.txt | 47 ++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 doc/device-tree-bindings/spi/spi-tegra.txt

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.
+
+Required properties:
+- compatible : should be "nvidia,tegra114-spi".
+- reg: Should contain SPI registers location and length.
+- interrupts: Should contain SPI interrupts.
+- clocks : Should contain an entry for SPI clock.
+
+Recommended properties:
+- spi-max-frequency: Definition as per
+                     doc/device-tree-bindings/spi/spi-bus.txt
+Example:
+
+spi at 7000d600 {
+	compatible = "nvidia,tegra114-spi";
+	reg = <0x7000d600 0x200>;
+	interrupts = <0 82 0x04>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	clocks = <&tegra_car 44>;
+	status = "disabled";
+	spi-max-frequency = <25000000>;
+};
+
+NVIDIA Tegra210 QSPI controller.
+
+Required properties:
+- compatible : should be "nvidia,tegra210-qspi".
+- reg: Should contain QSPI registers location and length.
+- interrupts: Should contain QSPI interrupts.
+- clocks : Should contain an entry for QSPI clock.
+
+Recommended properties:
+- spi-max-frequency: Definition as per
+                     doc/device-tree-bindings/spi/spi-bus.txt
+Example:
+
+spi at 70410000 {
+	compatible = "nvidia,tegra210-qspi";
+	reg = <0x0 0x70410000 0x0 0x1000>;
+	interrupts = <0 10 0x04>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	clocks = <&tegra_car 211>;
+	status = "disabled";
+	spi-max-frequency = <24000000>;
+};
-- 
1.8.2.1.610.g562af5b

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

[Stephen Warren]

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.

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@kernel.org>
M:      Pawel Moll <pawel.moll@arm.com>
M:      Mark Rutland <mark.rutland@arm.com>
M:      Ian Campbell <ijc+devicetree@hellion.org.uk>
M:      Kumar Gala <galak@codeaurora.org>
L:      devicetree at vger.kernel.org

> 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?

> +Required properties:
> +- compatible : should be "nvidia,tegra114-spi".

This should be "qspi" not "spi", assuming the HW really is different. 
This is a separate HW module, right?

> +- reg: Should contain SPI registers location and length.
> +- interrupts: Should contain SPI interrupts.
> +- clocks : Should contain an entry for SPI clock.

Reset- and DMA-related properties are missing here.

You could mark the DMA properties optional, and leave it up to drivers 
to support PIO mode if the DMA properties are missing.

> +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.

> +NVIDIA Tegra210 QSPI controller.

The binding for Tegra114 and Tegra210 should be (and appears to be) 
identical. There's no need to duplicate the text. Instead, simply say 
something like the following for the compatible value in the one copy of 
the text:

- compatible : should be one of the following:
  "nvidia,tegra114-qspi" (for Tegra114)
  "nvidia,tegra210-qspi" (for Tegra210)

However, if a driver that supports Tegra114 could operate correctly on 
Tegra210 without knowledge that it was running on different HW, the 
following compatible values area appropriate:

- compatible : should be one of the following:
   "nvidia,tegra114-qspi" (for Tegra114)
   "nvidia,tegra210-qspi", "nvidia,tegra114-qspi" (for Tegra210)

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

[Tom Warren]

Stephen,

> -----Original Message-----
> From: Stephen Warren [mailto:swarren at wwwdotorg.org]
> Sent: Friday, October 23, 2015 10:26 AM
> To: Tom Warren <TWarren@nvidia.com>
> Cc: u-boot at lists.denx.de; jteki at openedev.com; Stephen Warren
> <swarren@nvidia.com>; tomcwarren3959 at gmail.com
> Subject: Re: [U-Boot] [PATCH] spi: Tegra: add device tree binding doc for SPI
> and QSPI
> 
> 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.

> 
> 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@kernel.org>
> M:      Pawel Moll <pawel.moll@arm.com>
> M:      Mark Rutland <mark.rutland@arm.com>
> M:      Ian Campbell <ijc+devicetree@hellion.org.uk>
> M:      Kumar Gala <galak@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? Jagan - you have a few of these in the SPI bindings - did you have them reviewed by kernel/DT folk first?

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

> 
> > 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.

> 
> > +Required properties:
> > +- compatible : should be "nvidia,tegra114-spi".
> 
> This should be "qspi" not "spi", assuming the HW really is different.
> This is a separate HW module, right?
Again, this is for the SPI controller, not QSPI. QSPI binding follows @ line 25 below.

> 
> > +- reg: Should contain SPI registers location and length.
> > +- interrupts: Should contain SPI interrupts.
> > +- clocks : Should contain an entry for SPI clock.
> 
> Reset- and DMA-related properties are missing here.
> 
> You could mark the DMA properties optional, and leave it up to drivers to
> support PIO mode if the DMA properties are missing.
Will do.

> 
> > +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.

> 
> > +NVIDIA Tegra210 QSPI controller.
> 
> The binding for Tegra114 and Tegra210 should be (and appears to be)
> identical. There's no need to duplicate the text. Instead, simply say something
> like the following for the compatible value in the one copy of the text:
> 
> - compatible : should be one of the following:
>   "nvidia,tegra114-qspi" (for Tegra114)
>   "nvidia,tegra210-qspi" (for Tegra210)
> 
Will do.

> However, if a driver that supports Tegra114 could operate correctly on
> Tegra210 without knowledge that it was running on different HW, the following
> compatible values area appropriate:
> 
> - compatible : should be one of the following:
>    "nvidia,tegra114-qspi" (for Tegra114)
>    "nvidia,tegra210-qspi", "nvidia,tegra114-qspi" (for Tegra210)

Thanks,

Tom
--
nvpublic

[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@kernel.org>
>> M:      Pawel Moll <pawel.moll@arm.com>
>> M:      Mark Rutland <mark.rutland@arm.com>
>> M:      Ian Campbell <ijc+devicetree@hellion.org.uk>
>> M:      Kumar Gala <galak@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@wwwdotorg.org>
M:      Thierry Reding <thierry.reding@gmail.com>
M:      Alexandre Courbot <gnurou@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.