[PATCH 0/2] spi-orion: one fix and one improvement

[PATCH 0/2] spi-orion: one fix and one improvement

[Thomas Petazzoni]

Hello,

This is a small patch series agains the spi-orion driver. The first
patch fixes a problem with the handling of the cell-index DT property,
and should be merged for 3.16 as well as backported to -stable.

The second patch adds the support for multiple chip selects, but there
might be some discussion needed on what is the best approach to
describe that in the DT. Suggestions welcome.

Best regards,

Thomas

Thomas Petazzoni (2):
  spi: orion: fix incorrect handling of cell-index DT property
  spi: orion: add support for multiple chip selects

 .../devicetree/bindings/spi/spi-orion.txt          |  3 +++
 drivers/spi/spi-orion.c                            | 26 +++++++++++++++-------
 2 files changed, 21 insertions(+), 8 deletions(-)

-- 
2.0.0

[PATCH 1/2] spi: orion: fix incorrect handling of cell-index DT property

[Thomas Petazzoni]

In commit f814f9ac5a81 ("spi/orion: add device tree binding"), Device
Tree support was added to the spi-orion driver. However, this commit
reads the "cell-index" property, without taking into account the fact
that DT properties are big-endian encoded.

Since most of the platforms using spi-orion with DT have apparently
not used anything but cell-index = <0>, the problem was not
visible. But as soon as one starts using cell-index = <1>, the problem
becomes clearly visible, as the master->bus_num gets a wrong value
(actually it gets the value 0, which conflicts with the first bus that
has cell-index = <0>).

This commit fixes that by using of_property_read_u32() to read the
property value, which does the appropriate endianness conversion when
needed.

Fixes: f814f9ac5a81 ("spi/orion: add device tree binding")
Cc: <stable@vger.kernel.org> # v3.6+
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/spi/spi-orion.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
index d018a4a..c206a4a 100644
--- a/drivers/spi/spi-orion.c
+++ b/drivers/spi/spi-orion.c
@@ -346,8 +346,6 @@ static int orion_spi_probe(struct platform_device *pdev)
 	struct resource *r;
 	unsigned long tclk_hz;
 	int status = 0;
-	const u32 *iprop;
-	int size;
 
 	master = spi_alloc_master(&pdev->dev, sizeof(*spi));
 	if (master == NULL) {
@@ -358,10 +356,10 @@ static int orion_spi_probe(struct platform_device *pdev)
 	if (pdev->id != -1)
 		master->bus_num = pdev->id;
 	if (pdev->dev.of_node) {
-		iprop = of_get_property(pdev->dev.of_node, "cell-index",
-					&size);
-		if (iprop && size == sizeof(*iprop))
-			master->bus_num = *iprop;
+		u32 cell_index;
+		if (!of_property_read_u32(pdev->dev.of_node, "cell-index",
+					  &cell_index))
+			master->bus_num = cell_index;
 	}
 
 	/* we support only mode 0, and no options */
-- 
2.0.0

[PATCH 1/2] spi: orion: fix incorrect handling of cell-index DT property

[Sebastian Hesselbarth]

On 07/27/2014 11:53 PM, Thomas Petazzoni wrote:
> In commit f814f9ac5a81 ("spi/orion: add device tree binding"), Device
> Tree support was added to the spi-orion driver. However, this commit
> reads the "cell-index" property, without taking into account the fact
> that DT properties are big-endian encoded.
> 
> Since most of the platforms using spi-orion with DT have apparently
> not used anything but cell-index = <0>, the problem was not
> visible. But as soon as one starts using cell-index = <1>, the problem
> becomes clearly visible, as the master->bus_num gets a wrong value
> (actually it gets the value 0, which conflicts with the first bus that
> has cell-index = <0>).
> 
> This commit fixes that by using of_property_read_u32() to read the
> property value, which does the appropriate endianness conversion when
> needed.
> 
> Fixes: f814f9ac5a81 ("spi/orion: add device tree binding")
> Cc: <stable@vger.kernel.org> # v3.6+
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

For MVEBU,

Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

> ---
>  drivers/spi/spi-orion.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
> index d018a4a..c206a4a 100644
> --- a/drivers/spi/spi-orion.c
> +++ b/drivers/spi/spi-orion.c
> @@ -346,8 +346,6 @@ static int orion_spi_probe(struct platform_device *pdev)
>  	struct resource *r;
>  	unsigned long tclk_hz;
>  	int status = 0;
> -	const u32 *iprop;
> -	int size;
>  
>  	master = spi_alloc_master(&pdev->dev, sizeof(*spi));
>  	if (master == NULL) {
> @@ -358,10 +356,10 @@ static int orion_spi_probe(struct platform_device *pdev)
>  	if (pdev->id != -1)
>  		master->bus_num = pdev->id;
>  	if (pdev->dev.of_node) {
> -		iprop = of_get_property(pdev->dev.of_node, "cell-index",
> -					&size);
> -		if (iprop && size == sizeof(*iprop))
> -			master->bus_num = *iprop;
> +		u32 cell_index;
> +		if (!of_property_read_u32(pdev->dev.of_node, "cell-index",
> +					  &cell_index))
> +			master->bus_num = cell_index;
>  	}
>  
>  	/* we support only mode 0, and no options */
> 

[PATCH 1/2] spi: orion: fix incorrect handling of cell-index DT property

[Mark Brown]

On Sun, Jul 27, 2014 at 11:53:19PM +0200, Thomas Petazzoni wrote:
> In commit f814f9ac5a81 ("spi/orion: add device tree binding"), Device
> Tree support was added to the spi-orion driver. However, this commit
> reads the "cell-index" property, without taking into account the fact
> that DT properties are big-endian encoded.

Applied, thanks.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140728/2c1dd110/attachment.sig>

[PATCH 2/2] spi: orion: add support for multiple chip selects

[Thomas Petazzoni]

Until now, the spi-orion driver was limited to supporting only one
internal chip select (i.e not the GPIO ones, but the ones directly
handled by the SPI controller). However, recent Marvell platforms
potentially have more than one chip select, so this commit adds
support for a new 'ncs' DT property, which indicates how many chip
selects are available on this platform.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
We could certainly discuss:

 * Whether 'ncs' should be the number of *connected* chip selects, or
   the number of *usable* chip selects.

 * If it's the number of *usable* chip selects, then maybe it
   shouldn't be a DT property, but rather something that the driver
   "knows" thanks to the compatible string (which then, would be
   different from one SoC to the other, since Armada XP and Armada 375
   for example don't have the same number of chip selects).

Suggestions welcome.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 Documentation/devicetree/bindings/spi/spi-orion.txt |  3 +++
 drivers/spi/spi-orion.c                             | 16 ++++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-orion.txt b/Documentation/devicetree/bindings/spi/spi-orion.txt
index a3ff50f..0388c48 100644
--- a/Documentation/devicetree/bindings/spi/spi-orion.txt
+++ b/Documentation/devicetree/bindings/spi/spi-orion.txt
@@ -6,6 +6,8 @@ Required properties:
 - cell-index : Which of multiple SPI controllers is this.
 Optional properties:
 - interrupts : Is currently not used.
+- ncs : Number of chip selects used on the platform. Defaults to 1 when
+  unspecified.
 
 Example:
        spi at 10600 {
@@ -15,5 +17,6 @@ Example:
 	       cell-index = <0>;
 	       reg = <0x10600 0x28>;
 	       interrupts = <23>;
+	       ncs = <2>;
 	       status = "disabled";
        };
diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
index c206a4a..8145855 100644
--- a/drivers/spi/spi-orion.c
+++ b/drivers/spi/spi-orion.c
@@ -23,7 +23,6 @@
 
 #define DRIVER_NAME			"orion_spi"
 
-#define ORION_NUM_CHIPSELECTS		1 /* only one slave is supported*/
 #define ORION_SPI_WAIT_RDY_MAX_LOOP	2000 /* in usec */
 
 #define ORION_SPI_IF_CTRL_REG		0x00
@@ -38,6 +37,8 @@
 #define ORION_SPI_CLK_PRESCALE_MASK	0x1F
 #define ORION_SPI_MODE_MASK		(ORION_SPI_MODE_CPOL | \
 					 ORION_SPI_MODE_CPHA)
+#define ORION_SPI_CS(cs)		((cs) << 2)
+#define ORION_SPI_CS_MASK		GENMASK(4, 2)
 
 struct orion_spi {
 	struct spi_master	*master;
@@ -150,6 +151,11 @@ orion_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
 	if (rc)
 		return rc;
 
+	orion_spi_clrbits(orion_spi, ORION_SPI_IF_CTRL_REG,
+			  ORION_SPI_CS_MASK);
+	orion_spi_setbits(orion_spi, ORION_SPI_IF_CTRL_REG,
+			  ORION_SPI_CS(spi->chip_select));
+
 	if (bits_per_word == 16)
 		orion_spi_setbits(orion_spi, ORION_SPI_IF_CONFIG_REG,
 				  ORION_SPI_IF_8_16_BIT_MODE);
@@ -346,6 +352,7 @@ static int orion_spi_probe(struct platform_device *pdev)
 	struct resource *r;
 	unsigned long tclk_hz;
 	int status = 0;
+	u32 ncs;
 
 	master = spi_alloc_master(&pdev->dev, sizeof(*spi));
 	if (master == NULL) {
@@ -366,7 +373,12 @@ static int orion_spi_probe(struct platform_device *pdev)
 	master->mode_bits = SPI_CPHA | SPI_CPOL;
 
 	master->transfer_one_message = orion_spi_transfer_one_message;
-	master->num_chipselect = ORION_NUM_CHIPSELECTS;
+
+	if (of_property_read_u32(pdev->dev.of_node, "ncs", &ncs))
+		master->num_chipselect = 1;
+	else
+		master->num_chipselect = ncs;
+
 	master->bits_per_word_mask = SPI_BPW_MASK(8) | SPI_BPW_MASK(16);
 
 	platform_set_drvdata(pdev, master);
-- 
2.0.0

[PATCH 2/2] spi: orion: add support for multiple chip selects

[Sebastian Hesselbarth]

On 07/27/2014 11:53 PM, Thomas Petazzoni wrote:
> Until now, the spi-orion driver was limited to supporting only one
> internal chip select (i.e not the GPIO ones, but the ones directly
> handled by the SPI controller). However, recent Marvell platforms
> potentially have more than one chip select, so this commit adds
> support for a new 'ncs' DT property, which indicates how many chip
> selects are available on this platform.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
> We could certainly discuss:
> 
>  * Whether 'ncs' should be the number of *connected* chip selects, or
>    the number of *usable* chip selects.
> 
>  * If it's the number of *usable* chip selects, then maybe it
>    shouldn't be a DT property, but rather something that the driver
>    "knows" thanks to the compatible string (which then, would be
>    different from one SoC to the other, since Armada XP and Armada 375
>    for example don't have the same number of chip selects).
> 
> Suggestions welcome.

Thomas,

I guess Documentation/devicetree/bindings/spi/spi-bus.txt is quite clear
about it:

"""
The SPI master node requires the following properties:
...
Optional property:
- num-cs : total number of chipselects

If cs-gpios is used the number of chip select will automatically increased
with max(cs-gpios > hw cs)

So if for example the controller has 2 CS lines, and the cs-gpios
property looks like this:

cs-gpios = <&gpio1 0 0> <0> <&gpio1 1 0> <&gpio1 2 0>;

Then it should be configured so that num_chipselect = 4 with the
following mapping:

cs0 : &gpio1 0 0
cs1 : native
cs2 : &gpio1 1 0
cs3 : &gpio1 2 0
"""

So, "num-cs" describes the number of *available* chip select signals. As
they can comprise extra cs lines that are GPIO and not dedicated
controller signals (native above), it cannot be something the driver
knows.

As long as you only use native cs lines, omit the cs-gpios property, if
there is any GPIO cs lines, insert <0> where you want native ones.
Although, I wonder how the driver learns about reg = <1> is actually the
controller's cs0 in the example above?

The actual number of *used* chip selects is then determined by
corresponding child node's reg property.

Sebastian

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  Documentation/devicetree/bindings/spi/spi-orion.txt |  3 +++
>  drivers/spi/spi-orion.c                             | 16 ++++++++++++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-orion.txt b/Documentation/devicetree/bindings/spi/spi-orion.txt
> index a3ff50f..0388c48 100644
> --- a/Documentation/devicetree/bindings/spi/spi-orion.txt
> +++ b/Documentation/devicetree/bindings/spi/spi-orion.txt
> @@ -6,6 +6,8 @@ Required properties:
>  - cell-index : Which of multiple SPI controllers is this.
>  Optional properties:
>  - interrupts : Is currently not used.
> +- ncs : Number of chip selects used on the platform. Defaults to 1 when
> +  unspecified.
>  
>  Example:
>         spi at 10600 {
> @@ -15,5 +17,6 @@ Example:
>  	       cell-index = <0>;
>  	       reg = <0x10600 0x28>;
>  	       interrupts = <23>;
> +	       ncs = <2>;
>  	       status = "disabled";
>         };
> diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
> index c206a4a..8145855 100644
> --- a/drivers/spi/spi-orion.c
> +++ b/drivers/spi/spi-orion.c
> @@ -23,7 +23,6 @@
>  
>  #define DRIVER_NAME			"orion_spi"
>  
> -#define ORION_NUM_CHIPSELECTS		1 /* only one slave is supported*/
>  #define ORION_SPI_WAIT_RDY_MAX_LOOP	2000 /* in usec */
>  
>  #define ORION_SPI_IF_CTRL_REG		0x00
> @@ -38,6 +37,8 @@
>  #define ORION_SPI_CLK_PRESCALE_MASK	0x1F
>  #define ORION_SPI_MODE_MASK		(ORION_SPI_MODE_CPOL | \
>  					 ORION_SPI_MODE_CPHA)
> +#define ORION_SPI_CS(cs)		((cs) << 2)
> +#define ORION_SPI_CS_MASK		GENMASK(4, 2)
>  
>  struct orion_spi {
>  	struct spi_master	*master;
> @@ -150,6 +151,11 @@ orion_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
>  	if (rc)
>  		return rc;
>  
> +	orion_spi_clrbits(orion_spi, ORION_SPI_IF_CTRL_REG,
> +			  ORION_SPI_CS_MASK);
> +	orion_spi_setbits(orion_spi, ORION_SPI_IF_CTRL_REG,
> +			  ORION_SPI_CS(spi->chip_select));
> +
>  	if (bits_per_word == 16)
>  		orion_spi_setbits(orion_spi, ORION_SPI_IF_CONFIG_REG,
>  				  ORION_SPI_IF_8_16_BIT_MODE);
> @@ -346,6 +352,7 @@ static int orion_spi_probe(struct platform_device *pdev)
>  	struct resource *r;
>  	unsigned long tclk_hz;
>  	int status = 0;
> +	u32 ncs;
>  
>  	master = spi_alloc_master(&pdev->dev, sizeof(*spi));
>  	if (master == NULL) {
> @@ -366,7 +373,12 @@ static int orion_spi_probe(struct platform_device *pdev)
>  	master->mode_bits = SPI_CPHA | SPI_CPOL;
>  
>  	master->transfer_one_message = orion_spi_transfer_one_message;
> -	master->num_chipselect = ORION_NUM_CHIPSELECTS;
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "ncs", &ncs))
> +		master->num_chipselect = 1;
> +	else
> +		master->num_chipselect = ncs;
> +
>  	master->bits_per_word_mask = SPI_BPW_MASK(8) | SPI_BPW_MASK(16);
>  
>  	platform_set_drvdata(pdev, master);
> 

[PATCH 2/2] spi: orion: add support for multiple chip selects

[Mark Brown]

On Mon, Jul 28, 2014 at 08:25:14AM +0200, Sebastian Hesselbarth wrote:

> So, "num-cs" describes the number of *available* chip select signals. As
> they can comprise extra cs lines that are GPIO and not dedicated
> controller signals (native above), it cannot be something the driver
> knows.

Yes, indeed.  Though given that we also need a way to pass in the GPIOs
for the chip selects which isn't standardised at the minute it's all a
bit of a mess right now.

> As long as you only use native cs lines, omit the cs-gpios property, if
> there is any GPIO cs lines, insert <0> where you want native ones.

Right, something like that should be usable.  We also need a way to say
which native chip select to set when using a GPIO chip select if the
GPIO chip selects go beyond the native ones and none of the native ones
are masked by GPIOs.

> Although, I wonder how the driver learns about reg = <1> is actually the
> controller's cs0 in the example above?

Interesting question...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140728/d9928469/attachment.sig>

[PATCH 2/2] spi: orion: add support for multiple chip selects

[Mark Brown]

On Sun, Jul 27, 2014 at 11:53:20PM +0200, Thomas Petazzoni wrote:

> @@ -150,6 +151,11 @@ orion_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
>  	if (rc)
>  		return rc;
>  
> +	orion_spi_clrbits(orion_spi, ORION_SPI_IF_CTRL_REG,
> +			  ORION_SPI_CS_MASK);
> +	orion_spi_setbits(orion_spi, ORION_SPI_IF_CTRL_REG,
> +			  ORION_SPI_CS(spi->chip_select));
> +
>  	if (bits_per_word == 16)
>  		orion_spi_setbits(orion_spi, ORION_SPI_IF_CONFIG_REG,
>  				  ORION_SPI_IF_8_16_BIT_MODE);

I would expect to see this chip select manipulation to be being done in
set_cs not in setup_transfer() otherwise we'll probably get surprised
when the driver is converted to use transfer_one() and pull the chip
select handling out of the driver (among other things).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140728/cf427c91/attachment-0001.sig>