diff mbox

[v3,2/3] spi: atmel: update DT bindings documentation

Message ID ce7c16faafaa24f5bb711115c0eea1f8992d08c3.1433850255.git.cyrille.pitchen@atmel.com
State Accepted, archived
Commit 2c01a3d6b3cf0aaf39b55318fd986b1bd0b98168
Headers show

Commit Message

Cyrille Pitchen June 9, 2015, 11:53 a.m. UTC
- add new property "atmel,fifo-size"
- change "cs-gpios" to optional for SPI controller version >= 2.

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
---
 Documentation/devicetree/bindings/spi/spi_atmel.txt | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Nicolas Ferre June 9, 2015, 12:15 p.m. UTC | #1
Le 09/06/2015 13:53, Cyrille Pitchen a écrit :
> - add new property "atmel,fifo-size"
> - change "cs-gpios" to optional for SPI controller version >= 2.
> 
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

> ---
>  Documentation/devicetree/bindings/spi/spi_atmel.txt | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi_atmel.txt b/Documentation/devicetree/bindings/spi/spi_atmel.txt
> index 4f8184d..fb588b3 100644
> --- a/Documentation/devicetree/bindings/spi/spi_atmel.txt
> +++ b/Documentation/devicetree/bindings/spi/spi_atmel.txt
> @@ -4,11 +4,16 @@ Required properties:
>  - compatible : should be "atmel,at91rm9200-spi".
>  - reg: Address and length of the register set for the device
>  - interrupts: Should contain spi interrupt
> -- cs-gpios: chipselects
> +- cs-gpios: chipselects (optional for SPI controller version >= 2 with the
> +  Chip Select Active After Transfer feature).
>  - clock-names: tuple listing input clock names.
>  	Required elements: "spi_clk"
>  - clocks: phandles to input clocks.
>  
> +Optional properties:
> +- atmel,fifo-size: maximum number of data the RX and TX FIFOs can store for FIFO
> +  capable SPI controllers.
> +
>  Example:
>  
>  spi1: spi@fffcc000 {
> @@ -20,6 +25,7 @@ spi1: spi@fffcc000 {
>  	clocks = <&spi1_clk>;
>  	clock-names = "spi_clk";
>  	cs-gpios = <&pioB 3 0>;
> +	atmel,fifo-size = <32>;
>  	status = "okay";
>  
>  	mmc-slot@0 {
>
Mark Brown June 9, 2015, 5:25 p.m. UTC | #2
On Tue, Jun 09, 2015 at 01:53:53PM +0200, Cyrille Pitchen wrote:
> - add new property "atmel,fifo-size"

Why is this a property and not something we know from the IP version?
Cyrille Pitchen June 11, 2015, 4:37 p.m. UTC | #3
Le 09/06/2015 19:25, Mark Brown a écrit :
> On Tue, Jun 09, 2015 at 01:53:53PM +0200, Cyrille Pitchen wrote:
>> - add new property "atmel,fifo-size"
> 
> Why is this a property and not something we know from the IP version?
>
Hi Mark,
 
Please be aware that the VERSION register can not be used to guess the
size of FIFOs. Indeed, for a given hardware version, the SPI controller
can be integrated on Atmel SoCs with different FIFO sizes. Also the
"atmel,fifo-size" property is optional as older SPI controllers don't
embed FIFO at all.

Besides, the FIFO size can not be read or guessed from other registers:
When designing the FIFO feature, no dedicated registers were added to
store this size. Unused spaces in the I/O register range are limited and
better reserved for future usages. Instead, the FIFO size of each
peripheral is documented in the programmer datasheet.

Finally, on a given SoC, there can be several instances of the SPI
controller with different FIFO sizes. This explain why we'd rather use a
dedicated DT property than use the "compatible" property.

I hope these pieces of information will help to clarify this point.
Of course, we are open to other suggestions.

Best Regards,

Cyrille
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown June 15, 2015, 3:49 p.m. UTC | #4
On Thu, Jun 11, 2015 at 06:37:51PM +0200, Cyrille Pitchen wrote:
> Le 09/06/2015 19:25, Mark Brown a écrit :
> > On Tue, Jun 09, 2015 at 01:53:53PM +0200, Cyrille Pitchen wrote:
> >> - add new property "atmel,fifo-size"

> > Why is this a property and not something we know from the IP version?

> Please be aware that the VERSION register can not be used to guess the
> size of FIFOs. Indeed, for a given hardware version, the SPI controller
> can be integrated on Atmel SoCs with different FIFO sizes. Also the
> "atmel,fifo-size" property is optional as older SPI controllers don't
> embed FIFO at all.

...

> Finally, on a given SoC, there can be several instances of the SPI
> controller with different FIFO sizes. This explain why we'd rather use a
> dedicated DT property than use the "compatible" property.

> I hope these pieces of information will help to clarify this point.
> Of course, we are open to other suggestions.

Ugh, what a wonderfully consistent hardware design :(  Please make it
clear in the documentation what is going on here, this looks like an
obvious bug in the DT binding - a very common pattern for bugs is to do
version quirks like this as properties.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/spi/spi_atmel.txt b/Documentation/devicetree/bindings/spi/spi_atmel.txt
index 4f8184d..fb588b3 100644
--- a/Documentation/devicetree/bindings/spi/spi_atmel.txt
+++ b/Documentation/devicetree/bindings/spi/spi_atmel.txt
@@ -4,11 +4,16 @@  Required properties:
 - compatible : should be "atmel,at91rm9200-spi".
 - reg: Address and length of the register set for the device
 - interrupts: Should contain spi interrupt
-- cs-gpios: chipselects
+- cs-gpios: chipselects (optional for SPI controller version >= 2 with the
+  Chip Select Active After Transfer feature).
 - clock-names: tuple listing input clock names.
 	Required elements: "spi_clk"
 - clocks: phandles to input clocks.
 
+Optional properties:
+- atmel,fifo-size: maximum number of data the RX and TX FIFOs can store for FIFO
+  capable SPI controllers.
+
 Example:
 
 spi1: spi@fffcc000 {
@@ -20,6 +25,7 @@  spi1: spi@fffcc000 {
 	clocks = <&spi1_clk>;
 	clock-names = "spi_clk";
 	cs-gpios = <&pioB 3 0>;
+	atmel,fifo-size = <32>;
 	status = "okay";
 
 	mmc-slot@0 {