diff mbox

[v2] spi: davinci: add support for adding delay between word's transmission

Message ID 1410278851-10783-1-git-send-email-grygorii.strashko@ti.com
State Accepted, archived
Headers show

Commit Message

Grygorii Strashko Sept. 9, 2014, 4:07 p.m. UTC 
From: Murali Karicheri <m-karicheri2@ti.com>

This patch adds ability to configure delay between transmission of
words over SPI bus if it's required by SPI slave devices.

New optional SPI slave properties:
- ti,spi-word-delay : delay between transmission of words
	(SPIFMTn.WDELAY, SPIDAT1.WDEL)

- ti,spi-c2t-delay: Chip-select-active-to-transmit-start delay
	(SPIDELAY.C2TDELAY)

- ti,spi-t2c-delay: Transmit-end-to-chip-select-inactive delay
	(SPIDELAY.T2CDELAY)

- ti,spi-disable-cstimer: disable CS timer (SPIFMTn.DISCSTIMERS).

If WDELAY, C2TDELAY, T2CDELAY are configured then delay
between transmission of words will be added as following:

   wdelay = T2CDELAY + WDELAY + C2TDELAY.

If "ti,spi-disable-cstimer" is configured:

   wdelay = WDELAY.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---

Changes in v2:
- reduced number of new parameters
- bindings updated

Link on v1:
http://www.spinics.net/lists/linux-spi/msg01609.html

 .../devicetree/bindings/spi/spi-davinci.txt        | 50 +++++++++++++++++
 drivers/spi/spi-davinci.c                          | 62 +++++++++++++++++++---
 2 files changed, 105 insertions(+), 7 deletions(-)

Comments

Mark Brown Sept. 9, 2014, 4:55 p.m. UTC | #1 
On Tue, Sep 09, 2014 at 07:07:31PM +0300, Grygorii Strashko wrote:

> - ti,spi-c2t-delay: Chip-select-active-to-transmit-start delay
> 	(SPIDELAY.C2TDELAY)

> - ti,spi-t2c-delay: Transmit-end-to-chip-select-inactive delay
> 	(SPIDELAY.T2CDELAY)

Now I look at these they look very much like the standard delay feature
that the SPI subsystem has already - are they?
Murali Karicheri Sept. 9, 2014, 5:09 p.m. UTC | #2 
On 09/09/2014 12:55 PM, Mark Brown wrote:
> On Tue, Sep 09, 2014 at 07:07:31PM +0300, Grygorii Strashko wrote:
>
>> - ti,spi-c2t-delay: Chip-select-active-to-transmit-start delay
>> 	(SPIDELAY.C2TDELAY)
>
>> - ti,spi-t2c-delay: Transmit-end-to-chip-select-inactive delay
>> 	(SPIDELAY.T2CDELAY)
>
> Now I look at these they look very much like the standard delay feature
> that the SPI subsystem has already - are they?
Mark,

As Grygorii explained in previous postings (reproduced below), these 
delays are handled by the SPI hardware on Keystone and affect the delay 
between successive word tranmssion and has nothing to do with the delay 
you are talking about. Isn't the standard delay you mention here is 
between successive packets send down to the lower level driver (in this 
case spi-davinci.c) ?

Murali

Below is timing diagram which shows, in general, how these new 
parameters affect on words transmission over Keystone/Davinci SPI bus:

              +-+ +-+ +-+ +-+ +-+                           +-+ +-+ +-+
SPI_CLK      | | | | | | | | | |                           | | | | | |
   +----------+ +-+ +-+ +-+ +-+ +---------------------------+ +-+ +-+ +-

SPI_SOMI/SIMO+-----------------+                           +-----------
   +----------+ word1           +---------------------------+word2
              +-----------------+                           +-----------
                                           WDELAY
                                          <--------->
                                         +           +
SPI_CS                                  |           |
   +----+                                +-----------+
        |                                |           |
        +-----+-----------------+--------+           +-----+------------
        |     |                 |        |           |     |
        +     +                 +        |           +     +
         <--->                   <------>             <--->
        C2TDELAY                 T2CDELAY            C2TDELAY

Where:
         WDELAY - Delay in between transmissions
	C2TDELAY - Chip-select-active-to-transmit-start-delay
	T2CDELAY - Transmit-end-to-chip-select-inactive-delay
--
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 Sept. 9, 2014, 5:20 p.m. UTC | #3 
On Tue, Sep 09, 2014 at 01:09:27PM -0400, Murali Karicheri wrote:
> On 09/09/2014 12:55 PM, Mark Brown wrote:
> >On Tue, Sep 09, 2014 at 07:07:31PM +0300, Grygorii Strashko wrote:

> >>- ti,spi-c2t-delay: Chip-select-active-to-transmit-start delay
> >>	(SPIDELAY.C2TDELAY)

> >>- ti,spi-t2c-delay: Transmit-end-to-chip-select-inactive delay
> >>	(SPIDELAY.T2CDELAY)

> >Now I look at these they look very much like the standard delay feature
> >that the SPI subsystem has already - are they?

> As Grygorii explained in previous postings (reproduced below), these delays
> are handled by the SPI hardware on Keystone and affect the delay between
> successive word tranmssion and has nothing to do with the delay you are
> talking about. Isn't the standard delay you mention here is between
> successive packets send down to the lower level driver (in this case
> spi-davinci.c) ?

He talked about such delays between words (and there were some other
delays listed which seemed to meet that description) but the above don't
appear to refer to them, the above refer to delays around chip select
which most definitely are covered with the standard delays.

If these delays are not related to chip select then the documentation
needs to be fixed to not refer to chip select.
Murali Karicheri Sept. 9, 2014, 6:20 p.m. UTC | #4 
On 09/09/2014 01:20 PM, Mark Brown wrote:
> On Tue, Sep 09, 2014 at 01:09:27PM -0400, Murali Karicheri wrote:
>> On 09/09/2014 12:55 PM, Mark Brown wrote:
>>> On Tue, Sep 09, 2014 at 07:07:31PM +0300, Grygorii Strashko wrote:
>
>>>> - ti,spi-c2t-delay: Chip-select-active-to-transmit-start delay
>>>> 	(SPIDELAY.C2TDELAY)
>
>>>> - ti,spi-t2c-delay: Transmit-end-to-chip-select-inactive delay
>>>> 	(SPIDELAY.T2CDELAY)
>
>>> Now I look at these they look very much like the standard delay feature
>>> that the SPI subsystem has already - are they?
>
>> As Grygorii explained in previous postings (reproduced below), these delays
>> are handled by the SPI hardware on Keystone and affect the delay between
>> successive word tranmssion and has nothing to do with the delay you are
>> talking about. Isn't the standard delay you mention here is between
>> successive packets send down to the lower level driver (in this case
>> spi-davinci.c) ?
>
> He talked about such delays between words (and there were some other
> delays listed which seemed to meet that description) but the above don't
> appear to refer to them, the above refer to delays around chip select
> which most definitely are covered with the standard delays.
>
> If these delays are not related to chip select then the documentation
> needs to be fixed to not refer to chip select.

Ok. So what I understand is the issue is not having the right 
description to indicate that these parameters are delays associated with 
tranmission of successive words on the wire. Personally I like these 
description match with what is described in the device spec/ user guide 
and what is described above match with that. However we could add 
additional description as below to to make it more explicit.

spi-c2t-delay - delay after CS is asserted before output bits on wire
spi-t2c-delay - delay after tramission of bits and before deasserting CS
wdelay - delay between successive word transmission.

Do you think this will help?

Murali


--
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 Sept. 9, 2014, 7:58 p.m. UTC | #5 
On Tue, Sep 09, 2014 at 02:20:27PM -0400, Murali Karicheri wrote:
> On 09/09/2014 01:20 PM, Mark Brown wrote:

> >If these delays are not related to chip select then the documentation
> >needs to be fixed to not refer to chip select.

> Ok. So what I understand is the issue is not having the right description to
> indicate that these parameters are delays associated with tranmission of
> successive words on the wire. Personally I like these description match with
> what is described in the device spec/ user guide and what is described above
> match with that. However we could add additional description as below to to
> make it more explicit.

> spi-c2t-delay - delay after CS is asserted before output bits on wire
> spi-t2c-delay - delay after tramission of bits and before deasserting CS

> Do you think this will help?

No, that doesn't help at all.  You keep claiming that these parameters
are related to the intervals between words but then providing
descriptions of these parameters which relate to intervals around
changes in /CS which is patently not something connected with the gaps
between words.  Either your description of parameters which do not
relate to /CS should not need to mention /CS or the parameters are in
fact related to /CS, both can't simultaneously be true.
Grygorii Strashko Sept. 10, 2014, 5:02 p.m. UTC | #6 
Hi Mark,

On 09/09/2014 10:58 PM, Mark Brown wrote:
> On Tue, Sep 09, 2014 at 02:20:27PM -0400, Murali Karicheri wrote:
>> On 09/09/2014 01:20 PM, Mark Brown wrote:
> 
>>> If these delays are not related to chip select then the documentation
>>> needs to be fixed to not refer to chip select.
> 
>> Ok. So what I understand is the issue is not having the right description to
>> indicate that these parameters are delays associated with tranmission of
>> successive words on the wire. Personally I like these description match with
>> what is described in the device spec/ user guide and what is described above
>> match with that. However we could add additional description as below to to
>> make it more explicit.
> 
>> spi-c2t-delay - delay after CS is asserted before output bits on wire
>> spi-t2c-delay - delay after tramission of bits and before deasserting CS
> 
>> Do you think this will help?
> 
> No, that doesn't help at all.  You keep claiming that these parameters
> are related to the intervals between words but then providing
> descriptions of these parameters which relate to intervals around
> changes in /CS which is patently not something connected with the gaps
> between words.  Either your description of parameters which do not
> relate to /CS should not need to mention /CS or the parameters are in
> fact related to /CS, both can't simultaneously be true.
> 

This is what we have in DM (http://www.ti.com/lit/ug/sprugp2a/sprugp2a.pdf):
=========================================
2.5.1.3 Automatic Delay Between Transfers

The SPI master can automatically insert a delay of between 2 and 65 SPI module clock
cycles between transmissions. This delay is controlled by the WDELAY field in the SPI
data format register n (SPIFMTn) and is enabled by setting the WDEL bit in the SPI
transmit data register (SPIDAT1) to 1. The WDELAY period begins when the
T2EDELAY period terminates (if T2E delay period is enabled) or when
the T2CDELAY period terminates (if T2E delay period was disabled and T2C delay
period was enabled) or when the master deasserts SPISCS[n] (if T2E and T2C delay
periods are disabled). If a transfer is initiated by writing a 32-bit value to SPIDAT1,
then the new values of SPIDAT1.WDEL and SPIFMTn.WDELAY are used; otherwise,
the old values of SPIDAT1.WDEL and SPIFMTn.WDELAY are used. The WDELAY
delay period is specified by:

2.5.1.4 Chip Select Hold Option
There are slave devices available that require the chip select signal to be held
continuously active during several consecutive data word transfers. Other slave devices
require the chip select signal to be deactivated between consecutive data word transfers.
The SPI can support both types of slave devices. The CSHOLD bit in the SPI transmit
data register (SPIDAT1) selects between the two options.

If the chip select hold option is enabled, the chip select will not toggle between two
consecutive accesses. Therefore, the SPIDELAY.T2CDELAY of the first transfer and
the SPIDELAY.C2TDELAY of the second transfer will not be applied. However, the
wait delay could still be applied between the two transactions, if the WDEL bit in
SPIDAT1 is set to 1.

When the CSHOLD bit is 0, during the data transmission, the value of the chip select
number field (CSNR[n:0]) in the SPIDAT1 register is put on the chip select SPISCS[n]
to SPISCS[0] pins. When the transmission finishes, the chip select default pattern
(CSDEF[n:0]) is put on the SPISCS[n] to SPISCS[0] pins.
=========================================

And according to above the final delay between consecutive data word transfers can
be formed as sum of T2CDELAY  WDELAY, C2TDELAY and can include CS toggling
(exactly as shown on diagram).

I'm going to drop "spi-c2t-delay"/"spi-t2c-delay" properties, because CSHOLD is always
set now by Davinci SPI driver, so these delays will not be applied (this feature can be
enabled later). 
But WDELAY is important for us as we have issue with custom SPI device (FPGA based).

So, next version will only have "ti,spi-word-delay". Is it okay for you?

Best regards,
-grygorii

--
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
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/spi/spi-davinci.txt b/Documentation/devicetree/bindings/spi/spi-davinci.txt
index f80887b..1c336d4 100644
--- a/Documentation/devicetree/bindings/spi/spi-davinci.txt
+++ b/Documentation/devicetree/bindings/spi/spi-davinci.txt
@@ -24,6 +24,54 @@  Optional:
 	cs-gpios = <0>, <0>, <0>, <&gpio1 30 0>, <&gpio1 31 0>;
 	where first three are internal CS and last two are GPIO CS.
 
+Optional properties for slave devices:
+SPI slave nodes can contain the following properties.
+Not all SPI Peripherals from Texas Instruments support this.
+Please check SPI peripheral documentation for a device before using these.
+
+- ti,spi-word-delay : delay between transmission of words
+	(SPIFMTn.WDELAY, SPIDAT1.WDEL) must be specified in number of SPI module
+	clock periods.
+
+	delay = WDELAY * SPI_module_clock_period + 2 * SPI_module_clock_period
+
+- ti,spi-c2t-delay: Chip-select-active-to-transmit-start delay
+	(SPIDELAY.C2TDELAY) must be specified in number of SPI module
+	clock periods. if 0 - delay is not applied.
+
+	delay = (C2TDELAY + 2) * SPI_module_clock_period
+
+- ti,spi-t2c-delay: Transmit-end-to-chip-select-inactive delay
+	(SPIDELAY.T2CDELAY) must be specified in number of SPI module
+	clock periods. if 0 - delay is not applied.
+
+	delay = (T2CDELAY + 1) * SPI_module_clock_period
+
+- ti,spi-disable-cstimer: disable CS timer (SPIFMTn.DISCSTIMERS). Boolean
+	property that the C2TDELAY and T2CDELAY timers should be disabled.
+
+Below is timing diagram which shows functional meaning of
+"ti,spi-word-delay", "ti,spi-c2t-delay" and "ti,spi-t2c-delay" parameters.
+
+             +-+ +-+ +-+ +-+ +-+                           +-+ +-+ +-+
+SPI_CLK      | | | | | | | | | |                           | | | | | |
+  +----------+ +-+ +-+ +-+ +-+ +---------------------------+ +-+ +-+ +-
+
+SPI_SOMI/SIMO+-----------------+                           +-----------
+  +----------+ word1           +---------------------------+word2
+             +-----------------+                           +-----------
+                                          WDELAY
+                                         <--------->
+                                        +           +
+SPI_CS                                  |           |
+  +----+                                +-----------+
+       |                                |           |
+       +-----+-----------------+--------+           +-----+------------
+       |     |                 |        |           |     |
+       +     +                 +        |           +     +
+        <--->                   <------>             <--->
+       C2TDELAY                 T2CDELAY            C2TDELAY
+
 Example of a NOR flash slave device (n25q032) connected to DaVinci
 SPI controller device over the SPI bus.
 
@@ -43,6 +91,8 @@  spi0:spi@20BF0000 {
 		compatible = "st,m25p32";
 		spi-max-frequency = <25000000>;
 		reg = <0>;
+		ti,spi-c2t-delay = <8>;
+		ti,spi-t2c-delay = <8>;
 
 		partition@0 {
 			label = "u-boot-spl";
diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
index 48f1d26..ccadc95 100644
--- a/drivers/spi/spi-davinci.c
+++ b/drivers/spi/spi-davinci.c
@@ -65,6 +65,7 @@ 
 
 /* SPIDAT1 (upper 16 bit defines) */
 #define SPIDAT1_CSHOLD_MASK	BIT(12)
+#define SPIDAT1_WDEL		BIT(10)
 
 /* SPIGCR1 */
 #define SPIGCR1_CLKMOD_MASK	BIT(1)
@@ -209,6 +210,7 @@  static void davinci_spi_chipselect(struct spi_device *spi, int value)
 {
 	struct davinci_spi *dspi;
 	struct davinci_spi_platform_data *pdata;
+	struct davinci_spi_config *spicfg = spi->controller_data;
 	u8 chip_sel = spi->chip_select;
 	u16 spidat1 = CS_DEFAULT;
 	bool gpio_chipsel = false;
@@ -223,6 +225,10 @@  static void davinci_spi_chipselect(struct spi_device *spi, int value)
 		gpio = spi->cs_gpio;
 	}
 
+	/* program delay transfers if tx_delay is non zero */
+	if (spicfg->wdelay)
+		spidat1 |= SPIDAT1_WDEL;
+
 	/*
 	 * Board specific chip select logic decides the polarity and cs
 	 * line for the controller
@@ -237,9 +243,9 @@  static void davinci_spi_chipselect(struct spi_device *spi, int value)
 			spidat1 |= SPIDAT1_CSHOLD_MASK;
 			spidat1 &= ~(0x1 << chip_sel);
 		}
-
-		iowrite16(spidat1, dspi->base + SPIDAT1 + 2);
 	}
+
+	iowrite16(spidat1, dspi->base + SPIDAT1 + 2);
 }
 
 /**
@@ -285,7 +291,7 @@  static int davinci_spi_setup_transfer(struct spi_device *spi,
 	int prescale;
 
 	dspi = spi_master_get_devdata(spi->master);
-	spicfg = (struct davinci_spi_config *)spi->controller_data;
+	spicfg = spi->controller_data;
 	if (!spicfg)
 		spicfg = &davinci_spi_default_cfg;
 
@@ -333,6 +339,14 @@  static int davinci_spi_setup_transfer(struct spi_device *spi,
 		spifmt |= SPIFMT_PHASE_MASK;
 
 	/*
+	* Assume wdelay is used only on SPI peripherals that has this field
+	* in SPIFMTn register and when it's configured from board file or DT.
+	*/
+	if (spicfg->wdelay)
+		spifmt |= ((spicfg->wdelay << SPIFMT_WDELAY_SHIFT)
+				& SPIFMT_WDELAY_MASK);
+
+	/*
 	 * Version 1 hardware supports two basic SPI modes:
 	 *  - Standard SPI mode uses 4 pins, with chipselect
 	 *  - 3 pin SPI is a 4 pin variant without CS (SPI_NO_CS)
@@ -349,9 +363,6 @@  static int davinci_spi_setup_transfer(struct spi_device *spi,
 
 		u32 delay = 0;
 
-		spifmt |= ((spicfg->wdelay << SPIFMT_WDELAY_SHIFT)
-							& SPIFMT_WDELAY_MASK);
-
 		if (spicfg->odd_parity)
 			spifmt |= SPIFMT_ODD_PARITY_MASK;
 
@@ -383,6 +394,37 @@  static int davinci_spi_setup_transfer(struct spi_device *spi,
 	return 0;
 }
 
+static int davinci_spi_of_setup(struct spi_device *spi)
+{
+	struct davinci_spi_config *spicfg = spi->controller_data;
+	struct device_node *np = spi->dev.of_node;
+	u32 prop;
+	int len;
+
+	if (spicfg == NULL && np) {
+		spicfg = kzalloc(sizeof(*spicfg), GFP_KERNEL);
+		if (!spicfg)
+			return -ENOMEM;
+		*spicfg = davinci_spi_default_cfg;
+		/* override with dt configured values */
+		if (!of_property_read_u32(np,
+					  "ti,spi-word-delay", &prop))
+			spicfg->wdelay = (u8)prop;
+		if (of_find_property(np,
+				     "ti,spi-disable-cstimer", &len))
+			spicfg->timer_disable = 1;
+		if (!of_property_read_u32(np,
+					  "ti,spi-c2t-delay", &prop))
+			spicfg->c2tdelay = (u8)prop;
+		if (!of_property_read_u32(np,
+					  "ti,spi-t2c-delay", &prop))
+			spicfg->t2cdelay = (u8)prop;
+		spi->controller_data = spicfg;
+	}
+
+	return 0;
+}
+
 /**
  * davinci_spi_setup - This functions will set default transfer method
  * @spi: spi device on which data transfer to be done
@@ -436,11 +478,17 @@  static int davinci_spi_setup(struct spi_device *spi)
 	else
 		clear_io_bits(dspi->base + SPIGCR1, SPIGCR1_LOOPBACK_MASK);
 
-	return retval;
+	return davinci_spi_of_setup(spi);
 }
 
 static void davinci_spi_cleanup(struct spi_device *spi)
 {
+	struct davinci_spi_config *spicfg = spi->controller_data;
+
+	spi->controller_data = NULL;
+	if (spi->dev.of_node)
+		kfree(spicfg);
+
 	if (spi->cs_gpio >= 0)
 		gpio_free(spi->cs_gpio);
 }