drivers: spi: core: Add optional stall delay between cs_change transfers
diff mbox series

Message ID 1550595636-23598-1-git-send-email-stefan.popa@analog.com
State New, archived
Headers show
Series
  • drivers: spi: core: Add optional stall delay between cs_change transfers
Related show

Commit Message

Stefan Popa Feb. 19, 2019, 5 p.m. UTC
From: Michael Hennerich <michael.hennerich@analog.com>

Some devices like the ADIS16460 IMU require a stall period between
transfers. The default value of 10us are not enough. Introduce a per
transfer configurable delay.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Stefan Popa <stefan.popa@analog.com>
---
 drivers/spi/spi.c       | 3 ++-
 include/linux/spi/spi.h | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Geert Uytterhoeven Feb. 19, 2019, 7:58 p.m. UTC | #1
Hi Stefan,

Thanks for your patch!

On Tue, Feb 19, 2019 at 6:02 PM Stefan Popa <stefan.popa@analog.com> wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
>
> Some devices like the ADIS16460 IMU require a stall period between
> transfers.

This is not just about the period between transfers (which is handled by
delay_usecs), but about the period in between chipselect pulses if
cs_change is set.

> The default value of 10us are not enough. Introduce a per

... is not enough.

> transfer configurable delay.

> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1144,7 +1144,8 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
>                                 keep_cs = true;
>                         } else {
>                                 spi_set_cs(msg->spi, false);
> -                               udelay(10);
> +                               udelay(xfer->cs_change_stall_delay_us ?
> +                                      xfer->cs_change_stall_delay_us : 10);
>                                 spi_set_cs(msg->spi, true);
>                         }
>                 }
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 314d922..273774c 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -702,6 +702,8 @@ extern void spi_res_release(struct spi_controller *ctlr,
>   *      transfer. If 0 the default (from @spi_device) is used.
>   * @bits_per_word: select a bits_per_word other than the device default
>   *      for this transfer. If 0 the default (from @spi_device) is used.
> + * @cs_change_stall_delay_us: microseconds to delay between cs_change
> + *     transfers.

I think this needs some better wording, too.
Else it is hard to understand how this is different from delay_usecs,
without looking at the implementation.

>   * @cs_change: affects chipselect after this transfer completes
>   * @delay_usecs: microseconds to delay after this transfer before
>   *     (optionally) changing the chipselect status, then starting
> @@ -788,6 +790,7 @@ struct spi_transfer {
>  #define        SPI_NBITS_DUAL          0x02 /* 2bits transfer */
>  #define        SPI_NBITS_QUAD          0x04 /* 4bits transfer */
>         u8              bits_per_word;
> +       u8              cs_change_stall_delay_us;
>         u16             delay_usecs;
>         u32             speed_hz;
>         u16             word_delay;

Gr{oetje,eeting}s,

                        Geert
Martin Sperl Feb. 19, 2019, 8:42 p.m. UTC | #2
Hi Stefan!

I have had a similar patch in the pipeline...

But as my use case is for a device with speed_hz = 20MHz mine was with
nano seconds precision as even at only 20 MHz spi speed the lowest
resolution which is permissible by your patch wastes the BW of more than
2 bytes that could have gone over the wire while waiting for the minimum
of 1us which this patch provides.

Faster SPI-devices often require a 1 spi clock cycle where CS is de-asserted.
We should at least have a means to define a “as short” as possible, where
no delay is done at all and only the overhead of the gpio framework left.

> On 19.02.2019, at 18:00, Stefan Popa <stefan.popa@analog.com> wrote:
> 
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1144,7 +1144,8 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
> 				keep_cs = true;
> 			} else {
> 				spi_set_cs(msg->spi, false);
> -				udelay(10);
> +				udelay(xfer->cs_change_stall_delay_us ?
> +				       xfer->cs_change_stall_delay_us : 10);

Similar to the code implementing delay_usecs you should also use
usleep_range() for longer delays.

Martin

P.s: Maybe it could look something like this:

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9a7def7c3237..189f4ba7c5f9 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1071,6 +1071,20 @@ static int spi_transfer_wait(struct spi_controller *ctlr,
 	return 0;
 }
 
+static void _spi_transfer_delay_ns(u32 ns)
+{
+	if (ns <= 1000) {
+		ndelay(ns);
+	} else {
+		u32 us = DIV_ROUND_UP(ns, 1000);
+
+		if (us <= 10)
+			udelay(us);
+		else
+			usleep_range(us, us + DIV_ROUND_UP(us, 10));
+	}
+}
+
 /*
  * spi_transfer_one_message - Default implementation of transfer_one_message()
  *
@@ -1129,22 +1143,26 @@ static int spi_transfer_one_message(struct spi_controller *ctlr,
 		if (msg->status != -EINPROGRESS)
 			goto out;
 
-		if (xfer->delay_usecs) {
-			u16 us = xfer->delay_usecs;
-
-			if (us <= 10)
-				udelay(us);
-			else
-				usleep_range(us, us + DIV_ROUND_UP(us, 10));
-		}
+		if (xfer->delay_usecs)
+			_spi_transfer_delay_ns(xfer->delay_usecs * 1000);
 
 		if (xfer->cs_change) {
 			if (list_is_last(&xfer->transfer_list,
 					 &msg->transfers)) {
 				keep_cs = true;
 			} else {
+				u32 delay = xfer->cs_change_delay;
+
 				spi_set_cs(msg->spi, false);
-				udelay(10);
+				switch (xfer->cs_change_delay_unit) {
+				case SPI_DELAY_UNIT_USECS:
+					_spi_transfer_delay_ns(delay ? delay :
+							       10000);
+					break;
+				case SPI_DELAY_UNIT_NSECS:
+					_spi_transfer_delay_ns(delay);
+					break;
+				}
 				spi_set_cs(msg->spi, true);
 			}
 		}
@@ -3642,4 +3660,3 @@ static int __init spi_init(void)
  * include needing to have boardinfo data structures be much more public.
  */
 postcore_initcall(spi_init);
-
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 314d922ca607..f2ce1fb403ef 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -703,6 +703,9 @@ extern void spi_res_release(struct spi_controller *ctlr,
  * @bits_per_word: select a bits_per_word other than the device default
  *      for this transfer. If 0 the default (from @spi_device) is used.
  * @cs_change: affects chipselect after this transfer completes
+ * @cs_change_delay: delay between cs deassert and assert when
+ *      @cs_change is set and @spi_transfer is not the last in @spi_message
+ * @cs_change_delay_unit: unit of cs_change_delay
  * @delay_usecs: microseconds to delay after this transfer before
  *	(optionally) changing the chipselect status, then starting
  *	the next transfer or completing this @spi_message.
@@ -789,6 +792,10 @@ struct spi_transfer {
 #define	SPI_NBITS_QUAD		0x04 /* 4bits transfer */
 	u8		bits_per_word;
 	u16		delay_usecs;
+	u16		cs_change_delay;
+	u8		cs_change_delay_unit;
+#define SPI_DELAY_UNIT_USECS	0
+#define SPI_DELAY_UNIT_NSECS	1
 	u32		speed_hz;
 	u16		word_delay;
 

When we have a means to access the exact SPI clock speed that the
controller is really using (speed_hz is just an upper limit here!),
then we may even have a patch that "#define SPI_DELAY_UNIT_SCK 2"
and implements the corresponding computations to convert to us.
(some controllers with HW CS support may also implement it in HW)

Patch
diff mbox series

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9a7def7..717b92a 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1144,7 +1144,8 @@  static int spi_transfer_one_message(struct spi_controller *ctlr,
 				keep_cs = true;
 			} else {
 				spi_set_cs(msg->spi, false);
-				udelay(10);
+				udelay(xfer->cs_change_stall_delay_us ?
+				       xfer->cs_change_stall_delay_us : 10);
 				spi_set_cs(msg->spi, true);
 			}
 		}
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 314d922..273774c 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -702,6 +702,8 @@  extern void spi_res_release(struct spi_controller *ctlr,
  *      transfer. If 0 the default (from @spi_device) is used.
  * @bits_per_word: select a bits_per_word other than the device default
  *      for this transfer. If 0 the default (from @spi_device) is used.
+ * @cs_change_stall_delay_us: microseconds to delay between cs_change
+ *	transfers.
  * @cs_change: affects chipselect after this transfer completes
  * @delay_usecs: microseconds to delay after this transfer before
  *	(optionally) changing the chipselect status, then starting
@@ -788,6 +790,7 @@  struct spi_transfer {
 #define	SPI_NBITS_DUAL		0x02 /* 2bits transfer */
 #define	SPI_NBITS_QUAD		0x04 /* 4bits transfer */
 	u8		bits_per_word;
+	u8		cs_change_stall_delay_us;
 	u16		delay_usecs;
 	u32		speed_hz;
 	u16		word_delay;