Skip to contentSkip to conversation

spi: Tegra: add device tree binding doc for SPI and QSPI

PATCH New Tom Warren 2 participants 4 messages Oct 23, 2015 Tegra architecture support

lore
Share & tools
lore

Tom Warren

author
#0 lore raw

PATCH 1/1 spi: Tegra: add device tree binding doc for SPI and QSPI

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

---
 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
doc/device-tree-bindings/spi/spi-tegra.txt new file +47 -0
new file mode 100644
index 0000000..e215efe
@@ -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>;
+};
Signature · 1 line
1.8.2.1.610.g562af5b
#1 lore raw

PATCH 1/1 spi: Tegra: add device tree binding doc for SPI and QSPI

Quoted 2 lines · Tom Warren

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 <[email protected]>
M:      Pawel Moll <[email protected]>
M:      Mark Rutland <[email protected]>
M:      Ian Campbell <[email protected]>
M:      Kumar Gala <[email protected]>
L:      devicetree at vger.kernel.org

doc/device-tree-bindings/spi/spi-tegra.txt line 1

Quoted 7 lines
doc/device-tree-bindings/spi/spi-tegra.txt new file +1 -0
new file mode 100644
index 0000000..e215efe
@@ -0,0 +1,47 @@
+NVIDIA Tegra114 SPI controller.

Isn't this intended to be a binding for the QSPI controller?

doc/device-tree-bindings/spi/spi-tegra.txt line 4

Quoted 2 lines
doc/device-tree-bindings/spi/spi-tegra.txt new file +2 -0
+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?

doc/device-tree-bindings/spi/spi-tegra.txt line 7

Quoted 3 lines
doc/device-tree-bindings/spi/spi-tegra.txt new file +3 -0
+- 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.

doc/device-tree-bindings/spi/spi-tegra.txt line 11

Quoted 3 lines
doc/device-tree-bindings/spi/spi-tegra.txt new file +3 -0
+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.

doc/device-tree-bindings/spi/spi-tegra.txt line 25

Quoted 1 line
doc/device-tree-bindings/spi/spi-tegra.txt new file +1 -0
+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)

author
#2 lore raw

PATCH 1/1 spi: Tegra: add device tree binding doc for SPI and QSPI

Stephen,

Quoted 10 lines

-----Original Message-----
From: Stephen Warren [mailto:swarren at wwwdotorg.org]
Sent: Friday, October 23, 2015 10:26 AM
To: Tom Warren <[email protected]>
Cc: u-boot at lists.denx.de; jteki at openedev.com; Stephen Warren <[email protected]>; 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:

Quoted 8 lines
Quoted 2 lines

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.

Quoted 5 lines

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:

Quoted 7 lines

OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

M:      Rob Herring <[email protected]>
M:      Pawel Moll <[email protected]>
M:      Mark Rutland <[email protected]>
M:      Ian Campbell <[email protected]>
M:      Kumar Gala <[email protected]>
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?

Quoted 11 lines

doc/device-tree-bindings/spi/spi-tegra.txt line 1

Quoted 9 lines
doc/device-tree-bindings/spi/spi-tegra.txt new file +1 -0
new file mode 100644
index 0000000..e215efe
@@ -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.

Quoted 6 lines

doc/device-tree-bindings/spi/spi-tegra.txt line 4

Quoted 3 lines
doc/device-tree-bindings/spi/spi-tegra.txt new file +2 -0
+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.

Quoted 9 lines

doc/device-tree-bindings/spi/spi-tegra.txt line 7

Quoted 4 lines
doc/device-tree-bindings/spi/spi-tegra.txt new file +3 -0
+- 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.

Quoted 8 lines

doc/device-tree-bindings/spi/spi-tegra.txt line 11

Quoted 4 lines
doc/device-tree-bindings/spi/spi-tegra.txt new file +3 -0
+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.

Quoted 11 lines

doc/device-tree-bindings/spi/spi-tegra.txt line 25

Quoted 2 lines
doc/device-tree-bindings/spi/spi-tegra.txt new file +1 -0
+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.

Quoted 7 lines

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

#3 lore raw

PATCH 1/1 spi: Tegra: add device tree binding doc for SPI and QSPI

Quoted 10 lines · Tom Warren

Stephen,

Stephen Warren wrote at Friday, October 23, 2015 10:26 AM:

Quoted 7 lines · Tom Warren
Quoted 2 lines

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.

Quoted 8 lines
Quoted 2 lines

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.

Quoted 12 lines
Quoted 11 lines

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 <[email protected]>
M:      Pawel Moll <[email protected]>
M:      Mark Rutland <[email protected]>
M:      Ian Campbell <[email protected]>
M:      Kumar Gala <[email protected]>
L:      devicetree at vger.kernel.org
Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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 <[email protected]>
M:      Thierry Reding <[email protected]>
M:      Alexandre Courbot <[email protected]>
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).

Quoted 11 lines
Quoted 10 lines

doc/device-tree-bindings/spi/spi-tegra.txt line 1

Quoted 8 lines
doc/device-tree-bindings/spi/spi-tegra.txt new file +1 -0
new file mode 100644
index 0000000..e215efe
@@ -0,0 +1,47 @@
+NVIDIA Tegra114 SPI controller.

Isn't this intended to be a binding for the QSPI controller?

Quoted 4 lines

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.

Quoted 10 lines
Quoted 7 lines

doc/device-tree-bindings/spi/spi-tegra.txt line 11

Quoted 3 lines
doc/device-tree-bindings/spi/spi-tegra.txt new file +3 -0
+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.