[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 at nvidia.com>
> Cc: u-boot at lists.denx.de; jteki at openedev.com; Stephen Warren
> <swarren at 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 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? 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