[PATCH] SPI: Set SPI bits per words in an OF DeviceTree SPI node

[PATCH] SPI: Set SPI bits per words in an OF DeviceTree SPI node

[Christophe Leroy]

This patch allows to set up the bits per word together with all other SPI
parameters in the SPI Node of the OF device tree

In the node, you then have to include the 'spi-bits' property.

Exemple:
	fpga-loader@7 {
		compatible = "cs,fpga-loader";
		spi-max-frequency = <10000000>;
		reg = <7>;
		spi-cs-high;
		spi-bits = <16>;
	};

Signed-off-by: Christophe Leroy <christophe.leroy-GgN8y9CXRhA@public.gmane.org>

--- linux-3.8.13/drivers/spi/spi.c	2013-05-11 22:57:46.000000000 +0200
+++ linux/drivers/spi/spi.c	2013-08-06 18:19:30.000000000 +0200
@@ -870,6 +870,11 @@
 		if (of_find_property(nc, "spi-3wire", NULL))
 			spi->mode |= SPI_3WIRE;
 
+		/* Bits per word */
+		prop = of_get_property(nc, "spi-bits", &len);
+		if (prop && len >= sizeof(*prop))
+			spi->bits_per_word = prop[0];
+
 		/* Device speed */
 		prop = of_get_property(nc, "spi-max-frequency", &len);
 		if (!prop || len < sizeof(*prop)) {

------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead. 
Download for free and get started troubleshooting in minutes. 
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk

Fwd: [PATCH] SPI: Set SPI bits per words in an OF DeviceTree SPI node

[leroy christophe]

Sujet: 	[PATCH] SPI: Set SPI bits per words in an OF DeviceTree SPI node
Date : 	Wed, 7 Aug 2013 17:44:45 +0200
De : 	Christophe Leroy <christophe.leroy@c-s.fr>
Pour : 	Grant Likely <grant.likely@secretlab.ca>, Rob Herring 
<rob.herring@calxeda.com>
Copie � : 	spi-devel-general@lists.sourceforge.net, 
linux-kernel@vger.kernel.org, devicetree-discuss@lists.ozlabs.org, 
stigge@antcom.de



This patch allows to set up the bits per word together with all other SPI
parameters in the SPI Node of the OF device tree

In the node, you then have to include the 'spi-bits' property.

Exemple:
	fpga-loader@7 {
		compatible = "cs,fpga-loader";
		spi-max-frequency = <10000000>;
		reg = <7>;
		spi-cs-high;
		spi-bits = <16>;
	};

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>

--- linux-3.8.13/drivers/spi/spi.c	2013-05-11 22:57:46.000000000 +0200
+++ linux/drivers/spi/spi.c	2013-08-06 18:19:30.000000000 +0200
@@ -870,6 +870,11 @@
   		if (of_find_property(nc, "spi-3wire", NULL))
   			spi->mode |= SPI_3WIRE;
   +		/* Bits per word */
+		prop = of_get_property(nc, "spi-bits", &len);
+		if (prop && len >= sizeof(*prop))
+			spi->bits_per_word = prop[0];
+
   		/* Device speed */
   		prop = of_get_property(nc, "spi-max-frequency", &len);
   		if (!prop || len < sizeof(*prop)) {

Re: [PATCH] SPI: Set SPI bits per words in an OF DeviceTree SPI node

[Kumar Gala]

On Aug 7, 2013, at 10:54 AM, leroy christophe wrote:

> Sujet: 	[PATCH] SPI: Set SPI bits per words in an OF DeviceTree SPI node
> Date : 	Wed, 7 Aug 2013 17:44:45 +0200
> De : 	Christophe Leroy <christophe.leroy@c-s.fr>
> Pour : 	Grant Likely <grant.likely@secretlab.ca>, Rob Herring <rob.herring@calxeda.com>
> Copie � : 	spi-devel-general@lists.sourceforge.net, linux-kernel@vger.kernel.org, devicetree-discuss@lists.ozlabs.org, stigge@antcom.de
> 
> 
> 
> This patch allows to set up the bits per word together with all other SPI
> parameters in the SPI Node of the OF device tree
> 
> In the node, you then have to include the 'spi-bits' property.
> 
> Exemple:
> 	fpga-loader@7 {
> 		compatible = "cs,fpga-loader";
> 		spi-max-frequency = <10000000>;
> 		reg = <7>;
> 		spi-cs-high;
> 		spi-bits = <16>;
> 	};
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>

Is there an associated device tree binding spec update?  If not there should be.

- k

> 
> --- linux-3.8.13/drivers/spi/spi.c	2013-05-11 22:57:46.000000000 +0200
> +++ linux/drivers/spi/spi.c	2013-08-06 18:19:30.000000000 +0200
> @@ -870,6 +870,11 @@
>  		if (of_find_property(nc, "spi-3wire", NULL))
>  			spi->mode |= SPI_3WIRE;
>  +		/* Bits per word */
> +		prop = of_get_property(nc, "spi-bits", &len);
> +		if (prop && len >= sizeof(*prop))
> +			spi->bits_per_word = prop[0];
> +
>  		/* Device speed */
>  		prop = of_get_property(nc, "spi-max-frequency", &len);
>  		if (!prop || len < sizeof(*prop)) {
> 
> 
> 
> 
> --
> 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

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: Fwd: [PATCH] SPI: Set SPI bits per words in an OF DeviceTree SPI node

[Stephen Warren]

CCing devicetree and SPI maintainers.

On 08/07/2013 09:54 AM, leroy christophe wrote:
> This patch allows to set up the bits per word together with all other SPI
> parameters in the SPI Node of the OF device tree

The new property needs to be documented somewhere in
Documentation/devicetree/bindings.

> In the node, you then have to include the 'spi-bits' property.
> 
> Exemple:
>     fpga-loader@7 {
>         compatible = "cs,fpga-loader";
>         spi-max-frequency = <10000000>;
>         reg = <7>;
>         spi-cs-high;
>         spi-bits = <16>;
>     };

Shouldn't the driver for cs,fpga-loader be defining how many bit the SPI
transactions should use? If the binding covers HW that can operate at
various bits-per-word, it's reasonable for those individual bindings to
define a property that sets that width, but I'm not convinced it's
reasonable for the SPI core to allow any DT node to specify that, and
presumably override the SPI device's driver's selection.

> --- linux-3.8.13/drivers/spi/spi.c    2013-05-11 22:57:46.000000000 +0200
> +++ linux/drivers/spi/spi.c    2013-08-06 18:19:30.000000000 +0200
> @@ -870,6 +870,11 @@
>           if (of_find_property(nc, "spi-3wire", NULL))
>               spi->mode |= SPI_3WIRE;
>   +        /* Bits per word */
> +        prop = of_get_property(nc, "spi-bits", &len);
> +        if (prop && len >= sizeof(*prop))
> +            spi->bits_per_word = prop[0];
> +

Re: Fwd: [PATCH] SPI: Set SPI bits per words in an OF DeviceTree SPI node

[leroy christophe]

>> In the node, you then have to include the 'spi-bits' property.
>>
>> Exemple:
>>      fpga-loader@7 {
>>          compatible = "cs,fpga-loader";
>>          spi-max-frequency = <10000000>;
>>          reg = <7>;
>>          spi-cs-high;
>>          spi-bits = <16>;
>>      };
> Shouldn't the driver for cs,fpga-loader be defining how many bit the SPI
> transactions should use? If the binding covers HW that can operate at
> various bits-per-word, it's reasonable for those individual bindings to
> define a property that sets that width, but I'm not convinced it's
> reasonable for the SPI core to allow any DT node to specify that, and
> presumably override the SPI device's driver's selection.
>
This is a tricky question.

The issue in the CPM (communication co-processor) on the MPC 8xx CPU is 
that, when you use the 16 bits mode, the CPM swaps the bytes, so the 
driver must unswap them to use them properly.
It looks like their is the same issue with other CPUs, like the MPCs 
having the Quick Engine. In the QE driver, it is done simple: the 
transfert is forced at 8bits at all time regardless on what the driver 
requests. This is known to work well, but I don't want to do that for 
the CPM driver, because the 16 bits mode is a lot less CPU consuming 
than the 8 bits mode, and my own drivers for my custom components do 
work in 16 bits mode (byte swap is included in those drivers).

That's the reason why I was proposing a way, through the Device Tree, to 
select the speed instead of forcing it inside the driver.
Because the MAX7301 driver for instance, which used to force 16 bits 
mode, does work perfectly well on the MPC8xx in 8 bits mode, but doesn't 
work in 16 bits because it doesn't unswap the bytes.
If I modify that driver to swap the bytes, it will not work anymore on 
platforms that don't require bytes to be swapped.

So, really, I don't know what to do here. Do you have any suggestion ?

Re: Fwd: [PATCH] SPI: Set SPI bits per words in an OF DeviceTree SPI node

[Stephen Warren]

On 08/14/2013 07:21 AM, leroy christophe wrote:
> 
>>> In the node, you then have to include the 'spi-bits' property.
>>>
>>> Exemple:
>>>      fpga-loader@7 {
>>>          compatible = "cs,fpga-loader";
>>>          spi-max-frequency = <10000000>;
>>>          reg = <7>;
>>>          spi-cs-high;
>>>          spi-bits = <16>;
>>>      };
>> Shouldn't the driver for cs,fpga-loader be defining how many bit the SPI
>> transactions should use? If the binding covers HW that can operate at
>> various bits-per-word, it's reasonable for those individual bindings to
>> define a property that sets that width, but I'm not convinced it's
>> reasonable for the SPI core to allow any DT node to specify that, and
>> presumably override the SPI device's driver's selection.
>
> This is a tricky question.
> 
> The issue in the CPM (communication co-processor) on the MPC 8xx CPU is
> that, when you use the 16 bits mode, the CPM swaps the bytes, so the
> driver must unswap them to use them properly.
> It looks like their is the same issue with other CPUs, like the MPCs
> having the Quick Engine. In the QE driver, it is done simple: the
> transfert is forced at 8bits at all time regardless on what the driver
> requests. This is known to work well, but I don't want to do that for
> the CPM driver, because the 16 bits mode is a lot less CPU consuming
> than the 8 bits mode, and my own drivers for my custom components do
> work in 16 bits mode (byte swap is included in those drivers).

If there's a byte-swap bug in the CPM HW/driver, it should be solved in
the CPM driver, not in the drivers that happen to communicate over the
CPM driver. As you say below, fixing it in the client drivers is the
wrong thing to do, since then they won't work with other SPI controllers.

Either way, this doesn't seem like something that should be represented
in DT at all; it should be down to the CPM driver to indicate to the SPI
sub-system that it doesn't support 16-bit transfers.

An in general, your answer in no way addresses why this property should
apply to all SPI devices.

> That's the reason why I was proposing a way, through the Device Tree, to
> select the speed instead of forcing it inside the driver.
> Because the MAX7301 driver for instance, which used to force 16 bits
> mode, does work perfectly well on the MPC8xx in 8 bits mode, but doesn't
> work in 16 bits because it doesn't unswap the bytes.
> If I modify that driver to swap the bytes, it will not work anymore on
> platforms that don't require bytes to be swapped.
> 
> So, really, I don't know what to do here. Do you have any suggestion ?

Drop 16-bit support from the CPM driver if it doesn't work.

Re: Fwd: [PATCH] SPI: Set SPI bits per words in an OF DeviceTree SPI node

[leroy christophe]

Le 14/08/2013 18:19, Stephen Warren a �crit :
> On 08/14/2013 07:21 AM, leroy christophe wrote:
>>>> In the node, you then have to include the 'spi-bits' property.
>>>>
>>>> Exemple:
>>>>       fpga-loader@7 {
>>>>           compatible = "cs,fpga-loader";
>>>>           spi-max-frequency = <10000000>;
>>>>           reg = <7>;
>>>>           spi-cs-high;
>>>>           spi-bits = <16>;
>>>>       };
>>> Shouldn't the driver for cs,fpga-loader be defining how many bit the SPI
>>> transactions should use? If the binding covers HW that can operate at
>>> various bits-per-word, it's reasonable for those individual bindings to
>>> define a property that sets that width, but I'm not convinced it's
>>> reasonable for the SPI core to allow any DT node to specify that, and
>>> presumably override the SPI device's driver's selection.
>> This is a tricky question.
>>
>> The issue in the CPM (communication co-processor) on the MPC 8xx CPU is
>> that, when you use the 16 bits mode, the CPM swaps the bytes, so the
>> driver must unswap them to use them properly.
>> It looks like their is the same issue with other CPUs, like the MPCs
>> having the Quick Engine. In the QE driver, it is done simple: the
>> transfert is forced at 8bits at all time regardless on what the driver
>> requests. This is known to work well, but I don't want to do that for
>> the CPM driver, because the 16 bits mode is a lot less CPU consuming
>> than the 8 bits mode, and my own drivers for my custom components do
>> work in 16 bits mode (byte swap is included in those drivers).
> If there's a byte-swap bug in the CPM HW/driver, it should be solved in
> the CPM driver, not in the drivers that happen to communicate over the
> CPM driver. As you say below, fixing it in the client drivers is the
> wrong thing to do, since then they won't work with other SPI controllers.
>
> Either way, this doesn't seem like something that should be represented
> in DT at all; it should be down to the CPM driver to indicate to the SPI
> sub-system that it doesn't support 16-bit transfers.
>
> An in general, your answer in no way addresses why this property should
> apply to all SPI devices.
>
>> That's the reason why I was proposing a way, through the Device Tree, to
>> select the speed instead of forcing it inside the driver.
>> Because the MAX7301 driver for instance, which used to force 16 bits
>> mode, does work perfectly well on the MPC8xx in 8 bits mode, but doesn't
>> work in 16 bits because it doesn't unswap the bytes.
>> If I modify that driver to swap the bytes, it will not work anymore on
>> platforms that don't require bytes to be swapped.
>>
>> So, really, I don't know what to do here. Do you have any suggestion ?
> Drop 16-bit support from the CPM driver if it doesn't work.
>
But as I said, I don't want to drop this support from the driver because 
it does work as far as the driver is able to swap the bytes prior.
My own drivers are swapping the bytes and it works perfectly well.
I need to keep this capability because it allows a higher transfert 
speed than the 8 bits mode on the CPM (about 6 times), and this is 
important when I have to load a big FPGA on my board.
But on the other hand I also need drivers like the MAX7301 to work on my 
board.
This driver works very well when I modify it to force 8 bits mode. But 
in standard, the driver forces itself to 16 bits mode which makes it fail.

So I need another solution than droping the 16 bits mode from the driver.

Re: Fwd: [PATCH] SPI: Set SPI bits per words in an OF DeviceTree SPI node

[Stephen Warren]

On 08/14/2013 10:29 AM, leroy christophe wrote:
> 
> Le 14/08/2013 18:19, Stephen Warren a �crit :
>> On 08/14/2013 07:21 AM, leroy christophe wrote:
>>>>> In the node, you then have to include the 'spi-bits' property.
>>>>>
>>>>> Exemple:
>>>>>       fpga-loader@7 {
>>>>>           compatible = "cs,fpga-loader";
>>>>>           spi-max-frequency = <10000000>;
>>>>>           reg = <7>;
>>>>>           spi-cs-high;
>>>>>           spi-bits = <16>;
>>>>>       };
>>>> Shouldn't the driver for cs,fpga-loader be defining how many bit the
>>>> SPI
>>>> transactions should use? If the binding covers HW that can operate at
>>>> various bits-per-word, it's reasonable for those individual bindings to
>>>> define a property that sets that width, but I'm not convinced it's
>>>> reasonable for the SPI core to allow any DT node to specify that, and
>>>> presumably override the SPI device's driver's selection.
>>> This is a tricky question.
>>>
>>> The issue in the CPM (communication co-processor) on the MPC 8xx CPU is
>>> that, when you use the 16 bits mode, the CPM swaps the bytes, so the
>>> driver must unswap them to use them properly.
>>> It looks like their is the same issue with other CPUs, like the MPCs
>>> having the Quick Engine. In the QE driver, it is done simple: the
>>> transfert is forced at 8bits at all time regardless on what the driver
>>> requests. This is known to work well, but I don't want to do that for
>>> the CPM driver, because the 16 bits mode is a lot less CPU consuming
>>> than the 8 bits mode, and my own drivers for my custom components do
>>> work in 16 bits mode (byte swap is included in those drivers).
>> If there's a byte-swap bug in the CPM HW/driver, it should be solved in
>> the CPM driver, not in the drivers that happen to communicate over the
>> CPM driver. As you say below, fixing it in the client drivers is the
>> wrong thing to do, since then they won't work with other SPI controllers.
>>
>> Either way, this doesn't seem like something that should be represented
>> in DT at all; it should be down to the CPM driver to indicate to the SPI
>> sub-system that it doesn't support 16-bit transfers.
>>
>> An in general, your answer in no way addresses why this property should
>> apply to all SPI devices.
>>
>>> That's the reason why I was proposing a way, through the Device Tree, to
>>> select the speed instead of forcing it inside the driver.
>>> Because the MAX7301 driver for instance, which used to force 16 bits
>>> mode, does work perfectly well on the MPC8xx in 8 bits mode, but doesn't
>>> work in 16 bits because it doesn't unswap the bytes.
>>> If I modify that driver to swap the bytes, it will not work anymore on
>>> platforms that don't require bytes to be swapped.
>>>
>>> So, really, I don't know what to do here. Do you have any suggestion ?
>> Drop 16-bit support from the CPM driver if it doesn't work.
>>
> But as I said, I don't want to drop this support from the driver because
> it does work as far as the driver is able to swap the bytes prior.
> My own drivers are swapping the bytes and it works perfectly well.
> I need to keep this capability because it allows a higher transfert
> speed than the 8 bits mode on the CPM (about 6 times), and this is
> important when I have to load a big FPGA on my board.
> But on the other hand I also need drivers like the MAX7301 to work on my
> board.
> This driver works very well when I modify it to force 8 bits mode. But
> in standard, the driver forces itself to 16 bits mode which makes it fail.
> 
> So I need another solution than droping the 16 bits mode from the driver.

Perhaps enhance the SPI subsystem with a new flag/quirk/... that
indicates the host-controller supports byte-swapped 16-bit transactions.