| Message ID | 1550595636-23598-1-git-send-email-stefan.popa@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drivers: spi: core: Add optional stall delay between cs_change transfers | expand |
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
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)
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;