[PATCH v3 0/2] spi: support inter-word delays

[PATCH v3 0/2] spi: support inter-word delays

[Jonas Bonn]

Changed in v3:
* Drop setting of inter-word delay via device tree

Changed in v2:
* Fix atmel-spi driver to not unconditionally set minimal delay if no
  delay is required (erroneous clamping)

This short series adds support for SPI inter-word delays and configures
the spi-atmel driver to honour the setting.

Some SPI slaves are so slow that they are unable to keep up even at the
SPI controller's lowest available clock frequency.  I have such a
configuration where an AVR-based SPI slave is unable to feed the SPI bus
fast enough even the SPI master runs at the lowest possible clock speed.

Jonas Bonn (2):
  spi: support inter-word delay requirement for devices
  spi-atmel: support inter-word delay

 drivers/spi/spi-atmel.c | 18 +++++++++++++-----
 include/linux/spi/spi.h |  3 +++
 2 files changed, 16 insertions(+), 5 deletions(-)

-- 
2.19.1

[PATCH v3 1/2] spi: support inter-word delay requirement for devices

[Jonas Bonn]

Some devices are slow and cannot keep up with the SPI bus and therefore
require a short delay between words of the SPI transfer.

The example of this that I'm looking at is a SAMA5D2 with a minimum SPI
clock of 400kHz talking to an AVR-based SPI slave.  The AVR cannot put
bytes on the bus fast enough to keep up with the SoC's SPI controller
even at the lowest bus speed.

This patch introduces the ability to specify a required inter-word
delay for SPI devices.  It is up to the controller driver to configure
itself accordingly in order to introduce the requested delay.

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
CC: Mark Brown <broonie@kernel.org>
CC: Rob Herring <robh+dt@kernel.org>
CC: Mark Rutland <mark.rutland@arm.com>
CC: linux-spi@vger.kernel.org
CC: devicetree@vger.kernel.org
---
 include/linux/spi/spi.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 314d922ca607..8e410de02af4 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -118,6 +118,8 @@ void spi_statistics_add_transfer_stats(struct spi_statistics *stats,
  *	for driver coldplugging, and in uevents used for hotplugging
  * @cs_gpio: gpio number of the chipselect line (optional, -ENOENT when
  *	not using a GPIO line)
+ * @word_delay: microsecond delay to be inserted between consecutive words
+ *      of a transfer
  *
  * @statistics: statistics for the spi_device
  *
@@ -164,6 +166,7 @@ struct spi_device {
 	char			modalias[SPI_NAME_SIZE];
 	const char		*driver_override;
 	int			cs_gpio;	/* chip select gpio */
+	uint16_t		word_delay;	/* inter-word delay (us) */
 
 	/* the statistics */
 	struct spi_statistics	statistics;
-- 
2.19.1

[PATCH v3 2/2] spi-atmel: support inter-word delay

[Jonas Bonn]

If the SPI slave requires an inter-word delay, configure the DLYBCT
register accordingly.

Tested on a SAMA5D2 board (derived from SAMA5D2-Xplained reference
board).

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
CC: Nicolas Ferre <nicolas.ferre@microchip.com>
CC: Mark Brown <broonie@kernel.org>
CC: Alexandre Belloni <alexandre.belloni@bootlin.com>
CC: Ludovic Desroches <ludovic.desroches@microchip.com>
CC: linux-spi@vger.kernel.org
CC: linux-arm-kernel@lists.infradead.org
---
 drivers/spi/spi-atmel.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index 74fddcd3282b..24445bfbd74e 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -1209,13 +1209,21 @@ static int atmel_spi_setup(struct spi_device *spi)
 		csr |= SPI_BIT(CSAAT);
 
 	/* DLYBS is mostly irrelevant since we manage chipselect using GPIOs.
-	 *
-	 * DLYBCT would add delays between words, slowing down transfers.
-	 * It could potentially be useful to cope with DMA bottlenecks, but
-	 * in those cases it's probably best to just use a lower bitrate.
 	 */
 	csr |= SPI_BF(DLYBS, 0);
-	csr |= SPI_BF(DLYBCT, 0);
+
+	/* DLYBCT adds delays between words.  This is useful for slow devices
+	 * that need a bit of time to setup the next transfer.
+	 */
+	if (spi->word_delay) {
+		csr |= SPI_BF(DLYBCT,
+			clamp_t(u8,
+				(as->spi_clk/1000000*spi->word_delay)>>5,
+				1, 255));
+	} else {
+		csr |= SPI_BF(DLYBCT, 0);
+	}
+
 
 	/* chipselect must have been muxed as GPIO (e.g. in board setup) */
 	npcs_pin = (unsigned long)spi->controller_data;
-- 
2.19.1

[PATCH v3 2/2] spi-atmel: support inter-word delay

[Jonas Bonn]

If the SPI slave requires an inter-word delay, configure the DLYBCT
register accordingly.

Tested on a SAMA5D2 board (derived from SAMA5D2-Xplained reference
board).

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
CC: Nicolas Ferre <nicolas.ferre@microchip.com>
CC: Mark Brown <broonie@kernel.org>
CC: Alexandre Belloni <alexandre.belloni@bootlin.com>
CC: Ludovic Desroches <ludovic.desroches@microchip.com>
CC: linux-spi@vger.kernel.org
CC: linux-arm-kernel@lists.infradead.org
---
 drivers/spi/spi-atmel.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index 74fddcd3282b..24445bfbd74e 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -1209,13 +1209,21 @@ static int atmel_spi_setup(struct spi_device *spi)
 		csr |= SPI_BIT(CSAAT);
 
 	/* DLYBS is mostly irrelevant since we manage chipselect using GPIOs.
-	 *
-	 * DLYBCT would add delays between words, slowing down transfers.
-	 * It could potentially be useful to cope with DMA bottlenecks, but
-	 * in those cases it's probably best to just use a lower bitrate.
 	 */
 	csr |= SPI_BF(DLYBS, 0);
-	csr |= SPI_BF(DLYBCT, 0);
+
+	/* DLYBCT adds delays between words.  This is useful for slow devices
+	 * that need a bit of time to setup the next transfer.
+	 */
+	if (spi->word_delay) {
+		csr |= SPI_BF(DLYBCT,
+			clamp_t(u8,
+				(as->spi_clk/1000000*spi->word_delay)>>5,
+				1, 255));
+	} else {
+		csr |= SPI_BF(DLYBCT, 0);
+	}
+
 
 	/* chipselect must have been muxed as GPIO (e.g. in board setup) */
 	npcs_pin = (unsigned long)spi->controller_data;
-- 
2.19.1

[PATCH v3 2/2] spi-atmel: support inter-word delay

[Jonas Bonn]

If the SPI slave requires an inter-word delay, configure the DLYBCT
register accordingly.

Tested on a SAMA5D2 board (derived from SAMA5D2-Xplained reference
board).

Signed-off-by: Jonas Bonn <jonas@norrbonn.se>
CC: Nicolas Ferre <nicolas.ferre@microchip.com>
CC: Mark Brown <broonie@kernel.org>
CC: Alexandre Belloni <alexandre.belloni@bootlin.com>
CC: Ludovic Desroches <ludovic.desroches@microchip.com>
CC: linux-spi@vger.kernel.org
CC: linux-arm-kernel@lists.infradead.org
---
 drivers/spi/spi-atmel.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index 74fddcd3282b..24445bfbd74e 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -1209,13 +1209,21 @@ static int atmel_spi_setup(struct spi_device *spi)
 		csr |= SPI_BIT(CSAAT);
 
 	/* DLYBS is mostly irrelevant since we manage chipselect using GPIOs.
-	 *
-	 * DLYBCT would add delays between words, slowing down transfers.
-	 * It could potentially be useful to cope with DMA bottlenecks, but
-	 * in those cases it's probably best to just use a lower bitrate.
 	 */
 	csr |= SPI_BF(DLYBS, 0);
-	csr |= SPI_BF(DLYBCT, 0);
+
+	/* DLYBCT adds delays between words.  This is useful for slow devices
+	 * that need a bit of time to setup the next transfer.
+	 */
+	if (spi->word_delay) {
+		csr |= SPI_BF(DLYBCT,
+			clamp_t(u8,
+				(as->spi_clk/1000000*spi->word_delay)>>5,
+				1, 255));
+	} else {
+		csr |= SPI_BF(DLYBCT, 0);
+	}
+
 
 	/* chipselect must have been muxed as GPIO (e.g. in board setup) */
 	npcs_pin = (unsigned long)spi->controller_data;
-- 
2.19.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/2] spi: support inter-word delay requirement for devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

On Sat, Jan 26, 2019 at 05:32:19PM +0100, Jonas Bonn wrote:

> @@ -164,6 +166,7 @@ struct spi_device {
>  	char			modalias[SPI_NAME_SIZE];
>  	const char		*driver_override;
>  	int			cs_gpio;	/* chip select gpio */
> +	uint16_t		word_delay;	/* inter-word delay (us) */

This needs some code in the core joining it up with the per-transfer
word delay similar to what we have for speed_hz and bits_per_word in
__spi_validate().  Then the controller drivers can just look at the
per-transfer value and support both without having to duplicate logic.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] spi: support inter-word delay requirement for devices

[Jonas Bonn]

Hi,

On 28/01/2019 19:10, Mark Brown wrote:
> On Sat, Jan 26, 2019 at 05:32:19PM +0100, Jonas Bonn wrote:
> 
>> @@ -164,6 +166,7 @@ struct spi_device {
>>   	char			modalias[SPI_NAME_SIZE];
>>   	const char		*driver_override;
>>   	int			cs_gpio;	/* chip select gpio */
>> +	uint16_t		word_delay;	/* inter-word delay (us) */
> 
> This needs some code in the core joining it up with the per-transfer
> word delay similar to what we have for speed_hz and bits_per_word in
> __spi_validate().  Then the controller drivers can just look at the
> per-transfer value and support both without having to duplicate logic.
> 

So spi_transfer already has a field word_delay and it's defined as 
_clock cycles_ to delay between words.  I defined word_delay in 
spi_device as _microseconds_ to delay along the lines of delay_usecs.

Given that the inter-word delay is a function of the slave device speed 
and not of the SPI bus speed, I'm inclined to say that a time-based 
delay is what we want (to be independent of bus speed).  As such, I want 
to know if I should add word_delay_usecs to _both_ spi_transfer and 
spi_device?

There's only one user of word_delay from spi_transfer.  Just looking at 
it quickly, it looks like it wants the word_delay in 
SPI-controller-clock cycles and not SCK cycles which seems pretty broken 
to me.  Adding Baolin and Lanqing to CC: for comment.  Could we rework 
that to be microseconds and do the calculation in the driver?

Thanks,
Jonas

Re: [PATCH v3 1/2] spi: support inter-word delay requirement for devices

[Baolin Wang]

Hi Jonas,
On Tue, 29 Jan 2019 at 05:28, Jonas Bonn <jonas@norrbonn.se> wrote:
>
> Hi,
>
> On 28/01/2019 19:10, Mark Brown wrote:
> > On Sat, Jan 26, 2019 at 05:32:19PM +0100, Jonas Bonn wrote:
> >
> >> @@ -164,6 +166,7 @@ struct spi_device {
> >>      char                    modalias[SPI_NAME_SIZE];
> >>      const char              *driver_override;
> >>      int                     cs_gpio;        /* chip select gpio */
> >> +    uint16_t                word_delay;     /* inter-word delay (us) */
> >
> > This needs some code in the core joining it up with the per-transfer
> > word delay similar to what we have for speed_hz and bits_per_word in
> > __spi_validate().  Then the controller drivers can just look at the
> > per-transfer value and support both without having to duplicate logic.
> >
>
> So spi_transfer already has a field word_delay and it's defined as
> _clock cycles_ to delay between words.  I defined word_delay in
> spi_device as _microseconds_ to delay along the lines of delay_usecs.
>
> Given that the inter-word delay is a function of the slave device speed
> and not of the SPI bus speed, I'm inclined to say that a time-based
> delay is what we want (to be independent of bus speed).  As such, I want
> to know if I should add word_delay_usecs to _both_ spi_transfer and
> spi_device?
>
> There's only one user of word_delay from spi_transfer.  Just looking at
> it quickly, it looks like it wants the word_delay in
> SPI-controller-clock cycles and not SCK cycles which seems pretty broken
> to me.  Adding Baolin and Lanqing to CC: for comment.  Could we rework
> that to be microseconds and do the calculation in the driver?

The Spreadtrum SPI controller's word delay unit is clock cycle of the
SPI clock, since the SPI source clock can be changed, we can not force
user to know the real microseconds. But can we change it to a union
structure? not sure if this is a good way.

union {
     int word_delay_us;
     int word_delay_cycle;
} w;

-- 
Baolin Wang
Best Regards

Re: [PATCH v3 1/2] spi: support inter-word delay requirement for devices

[Jonas Bonn]

On 29/01/2019 10:04, Baolin Wang wrote:
> Hi Jonas,
> On Tue, 29 Jan 2019 at 05:28, Jonas Bonn <jonas@norrbonn.se> wrote:
>>
>> Hi,
>>
>> On 28/01/2019 19:10, Mark Brown wrote:
>>> On Sat, Jan 26, 2019 at 05:32:19PM +0100, Jonas Bonn wrote:
>>>
>>>> @@ -164,6 +166,7 @@ struct spi_device {
>>>>       char                    modalias[SPI_NAME_SIZE];
>>>>       const char              *driver_override;
>>>>       int                     cs_gpio;        /* chip select gpio */
>>>> +    uint16_t                word_delay;     /* inter-word delay (us) */
>>>
>>> This needs some code in the core joining it up with the per-transfer
>>> word delay similar to what we have for speed_hz and bits_per_word in
>>> __spi_validate().  Then the controller drivers can just look at the
>>> per-transfer value and support both without having to duplicate logic.
>>>
>>
>> So spi_transfer already has a field word_delay and it's defined as
>> _clock cycles_ to delay between words.  I defined word_delay in
>> spi_device as _microseconds_ to delay along the lines of delay_usecs.
>>
>> Given that the inter-word delay is a function of the slave device speed
>> and not of the SPI bus speed, I'm inclined to say that a time-based
>> delay is what we want (to be independent of bus speed).  As such, I want
>> to know if I should add word_delay_usecs to _both_ spi_transfer and
>> spi_device?
>>
>> There's only one user of word_delay from spi_transfer.  Just looking at
>> it quickly, it looks like it wants the word_delay in
>> SPI-controller-clock cycles and not SCK cycles which seems pretty broken
>> to me.  Adding Baolin and Lanqing to CC: for comment.  Could we rework
>> that to be microseconds and do the calculation in the driver?
> 
> The Spreadtrum SPI controller's word delay unit is clock cycle of the
> SPI clock, since the SPI source clock can be changed, we can not force
> user to know the real microseconds. But can we change it to a union
> structure? not sure if this is a good way.

OK, so it is the SPI clock.  That's good.  There's a comment in the 
driver that makes it look like it should be the source clock.

The problem with a delay in clock cycles is that the faster the clock, 
the shorter the delay.  The delay is a property of the slave and the 
slave has a fixed internal clock.  This means that if we increase SCK we 
also need to increase the word_delay (in cycles) in order to give the 
slave the same amount of breathing room.

> 
> union {
>       int word_delay_us;
>       int word_delay_cycle;
> } w;
> 

I don't think that's a practical solution.

The register setting in the spi-sprd driver is what... SCK cycles?  So 
you'd want word_delay_us * max_speed_hz?

The register setting on my Atmel board is in SPI-clock cycles 
(effectively).  So I want word_delay_us*clk_get_rate(spi-clk).

/Jonas

Re: [PATCH v3 1/2] spi: support inter-word delay requirement for devices

[Baolin Wang]

On Tue, 29 Jan 2019 at 17:14, Jonas Bonn <jonas@norrbonn.se> wrote:
>
>
>
> On 29/01/2019 10:04, Baolin Wang wrote:
> > Hi Jonas,
> > On Tue, 29 Jan 2019 at 05:28, Jonas Bonn <jonas@norrbonn.se> wrote:
> >>
> >> Hi,
> >>
> >> On 28/01/2019 19:10, Mark Brown wrote:
> >>> On Sat, Jan 26, 2019 at 05:32:19PM +0100, Jonas Bonn wrote:
> >>>
> >>>> @@ -164,6 +166,7 @@ struct spi_device {
> >>>>       char                    modalias[SPI_NAME_SIZE];
> >>>>       const char              *driver_override;
> >>>>       int                     cs_gpio;        /* chip select gpio */
> >>>> +    uint16_t                word_delay;     /* inter-word delay (us) */
> >>>
> >>> This needs some code in the core joining it up with the per-transfer
> >>> word delay similar to what we have for speed_hz and bits_per_word in
> >>> __spi_validate().  Then the controller drivers can just look at the
> >>> per-transfer value and support both without having to duplicate logic.
> >>>
> >>
> >> So spi_transfer already has a field word_delay and it's defined as
> >> _clock cycles_ to delay between words.  I defined word_delay in
> >> spi_device as _microseconds_ to delay along the lines of delay_usecs.
> >>
> >> Given that the inter-word delay is a function of the slave device speed
> >> and not of the SPI bus speed, I'm inclined to say that a time-based
> >> delay is what we want (to be independent of bus speed).  As such, I want
> >> to know if I should add word_delay_usecs to _both_ spi_transfer and
> >> spi_device?
> >>
> >> There's only one user of word_delay from spi_transfer.  Just looking at
> >> it quickly, it looks like it wants the word_delay in
> >> SPI-controller-clock cycles and not SCK cycles which seems pretty broken
> >> to me.  Adding Baolin and Lanqing to CC: for comment.  Could we rework
> >> that to be microseconds and do the calculation in the driver?
> >
> > The Spreadtrum SPI controller's word delay unit is clock cycle of the
> > SPI clock, since the SPI source clock can be changed, we can not force
> > user to know the real microseconds. But can we change it to a union
> > structure? not sure if this is a good way.
>
> OK, so it is the SPI clock.  That's good.  There's a comment in the
> driver that makes it look like it should be the source clock.

Sorry for my unclear description, what I mean is that it is the SPI
source clock cycles.

> The problem with a delay in clock cycles is that the faster the clock,
> the shorter the delay.  The delay is a property of the slave and the
> slave has a fixed internal clock.  This means that if we increase SCK we
> also need to increase the word_delay (in cycles) in order to give the
> slave the same amount of breathing room.

Sorry for my confusing description, our case requires source clock
cycles for word delay.

>
> >
> > union {
> >       int word_delay_us;
> >       int word_delay_cycle;
> > } w;
> >
>
> I don't think that's a practical solution.
>
> The register setting in the spi-sprd driver is what... SCK cycles?  So
> you'd want word_delay_us * max_speed_hz?
>
> The register setting on my Atmel board is in SPI-clock cycles
> (effectively).  So I want word_delay_us*clk_get_rate(spi-clk).

Okay, so your case is different with Spreadtrum SPI.

-- 
Baolin Wang
Best Regards

Re: [PATCH v3 1/2] spi: support inter-word delay requirement for devices

[Jonas Bonn]

On 29/01/2019 10:35, Baolin Wang wrote:
> On Tue, 29 Jan 2019 at 17:14, Jonas Bonn <jonas@norrbonn.se> wrote:
>>
>>
>>
>> On 29/01/2019 10:04, Baolin Wang wrote:
>>> Hi Jonas,
>>> On Tue, 29 Jan 2019 at 05:28, Jonas Bonn <jonas@norrbonn.se> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 28/01/2019 19:10, Mark Brown wrote:
>>>>> On Sat, Jan 26, 2019 at 05:32:19PM +0100, Jonas Bonn wrote:
>>>>>
>>>>>> @@ -164,6 +166,7 @@ struct spi_device {
>>>>>>        char                    modalias[SPI_NAME_SIZE];
>>>>>>        const char              *driver_override;
>>>>>>        int                     cs_gpio;        /* chip select gpio */
>>>>>> +    uint16_t                word_delay;     /* inter-word delay (us) */
>>>>>
>>>>> This needs some code in the core joining it up with the per-transfer
>>>>> word delay similar to what we have for speed_hz and bits_per_word in
>>>>> __spi_validate().  Then the controller drivers can just look at the
>>>>> per-transfer value and support both without having to duplicate logic.
>>>>>
>>>>
>>>> So spi_transfer already has a field word_delay and it's defined as
>>>> _clock cycles_ to delay between words.  I defined word_delay in
>>>> spi_device as _microseconds_ to delay along the lines of delay_usecs.
>>>>
>>>> Given that the inter-word delay is a function of the slave device speed
>>>> and not of the SPI bus speed, I'm inclined to say that a time-based
>>>> delay is what we want (to be independent of bus speed).  As such, I want
>>>> to know if I should add word_delay_usecs to _both_ spi_transfer and
>>>> spi_device?
>>>>
>>>> There's only one user of word_delay from spi_transfer.  Just looking at
>>>> it quickly, it looks like it wants the word_delay in
>>>> SPI-controller-clock cycles and not SCK cycles which seems pretty broken
>>>> to me.  Adding Baolin and Lanqing to CC: for comment.  Could we rework
>>>> that to be microseconds and do the calculation in the driver?
>>>
>>> The Spreadtrum SPI controller's word delay unit is clock cycle of the
>>> SPI clock, since the SPI source clock can be changed, we can not force
>>> user to know the real microseconds. But can we change it to a union
>>> structure? not sure if this is a good way.
>>
>> OK, so it is the SPI clock.  That's good.  There's a comment in the
>> driver that makes it look like it should be the source clock.
> 
> Sorry for my unclear description, what I mean is that it is the SPI
> source clock cycles.
> 
>> The problem with a delay in clock cycles is that the faster the clock,
>> the shorter the delay.  The delay is a property of the slave and the
>> slave has a fixed internal clock.  This means that if we increase SCK we
>> also need to increase the word_delay (in cycles) in order to give the
>> slave the same amount of breathing room.
> 
> Sorry for my confusing description, our case requires source clock
> cycles for word delay.

OK.  So the user (perhaps in userspace using spidev) has to know the 
rate of the IO clock that the SPI controller sits behind and then has to 
match this to the required delay of the slave device...  Doesn't sound 
very portable.

> 
>>
>>>
>>> union {
>>>        int word_delay_us;
>>>        int word_delay_cycle;
>>> } w;
>>>
>>
>> I don't think that's a practical solution.
>>
>> The register setting in the spi-sprd driver is what... SCK cycles?  So
>> you'd want word_delay_us * max_speed_hz?
>>
>> The register setting on my Atmel board is in SPI-clock cycles
>> (effectively).  So I want word_delay_us*clk_get_rate(spi-clk).
> 
> Okay, so your case is different with Spreadtrum SPI.
> 

No, if your register is source clock cycles then it's the same.  On my 
board, the register setting is source clock cycles, too.  It's a 
straightforward conversion from delay to clock cycles... rough, unscaled 
formula above.

/Jonas

Re: [PATCH v3 1/2] spi: support inter-word delay requirement for devices

[Geert Uytterhoeven]

Hi Jonas, Baolin,

On Tue, Jan 29, 2019 at 10:50 AM Jonas Bonn <jonas@norrbonn.se> wrote:
> On 29/01/2019 10:35, Baolin Wang wrote:
> > On Tue, 29 Jan 2019 at 17:14, Jonas Bonn <jonas@norrbonn.se> wrote:
> >> On 29/01/2019 10:04, Baolin Wang wrote:
> >>> On Tue, 29 Jan 2019 at 05:28, Jonas Bonn <jonas@norrbonn.se> wrote:
> >>>> On 28/01/2019 19:10, Mark Brown wrote:
> >>>>> On Sat, Jan 26, 2019 at 05:32:19PM +0100, Jonas Bonn wrote:
> >>>>>
> >>>>>> @@ -164,6 +166,7 @@ struct spi_device {
> >>>>>>        char                    modalias[SPI_NAME_SIZE];
> >>>>>>        const char              *driver_override;
> >>>>>>        int                     cs_gpio;        /* chip select gpio */
> >>>>>> +    uint16_t                word_delay;     /* inter-word delay (us) */
> >>>>>
> >>>>> This needs some code in the core joining it up with the per-transfer
> >>>>> word delay similar to what we have for speed_hz and bits_per_word in
> >>>>> __spi_validate().  Then the controller drivers can just look at the
> >>>>> per-transfer value and support both without having to duplicate logic.
> >>>>>
> >>>>
> >>>> So spi_transfer already has a field word_delay and it's defined as
> >>>> _clock cycles_ to delay between words.  I defined word_delay in
> >>>> spi_device as _microseconds_ to delay along the lines of delay_usecs.
> >>>>
> >>>> Given that the inter-word delay is a function of the slave device speed
> >>>> and not of the SPI bus speed, I'm inclined to say that a time-based
> >>>> delay is what we want (to be independent of bus speed).  As such, I want
> >>>> to know if I should add word_delay_usecs to _both_ spi_transfer and
> >>>> spi_device?
> >>>>
> >>>> There's only one user of word_delay from spi_transfer.  Just looking at
> >>>> it quickly, it looks like it wants the word_delay in
> >>>> SPI-controller-clock cycles and not SCK cycles which seems pretty broken
> >>>> to me.  Adding Baolin and Lanqing to CC: for comment.  Could we rework
> >>>> that to be microseconds and do the calculation in the driver?
> >>>
> >>> The Spreadtrum SPI controller's word delay unit is clock cycle of the
> >>> SPI clock, since the SPI source clock can be changed, we can not force
> >>> user to know the real microseconds. But can we change it to a union
> >>> structure? not sure if this is a good way.
> >>
> >> OK, so it is the SPI clock.  That's good.  There's a comment in the
> >> driver that makes it look like it should be the source clock.
> >
> > Sorry for my unclear description, what I mean is that it is the SPI
> > source clock cycles.
> >
> >> The problem with a delay in clock cycles is that the faster the clock,
> >> the shorter the delay.  The delay is a property of the slave and the
> >> slave has a fixed internal clock.  This means that if we increase SCK we
> >> also need to increase the word_delay (in cycles) in order to give the
> >> slave the same amount of breathing room.
> >
> > Sorry for my confusing description, our case requires source clock
> > cycles for word delay.
>
> OK.  So the user (perhaps in userspace using spidev) has to know the
> rate of the IO clock that the SPI controller sits behind and then has to
> match this to the required delay of the slave device...  Doesn't sound
> very portable.

I can see the value of having both:
On some slaves, the delay may depend on a fixed internal or
external clock[1] on the SPI slave, so it should be specified in time units.
Some slaves may be clocked by the SPI clock[2], so the delay should be
specified in SPI clock cycles.

[1] For an external clock, the SPI slave driver may need to obtain a clock
    reference from DT, get its rate, and calculate the needed delay.
[2] I've seen hardware designs where the SPI clock had to be kept running all
    the time because of this.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 1/2] spi: support inter-word delay requirement for devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 882 bytes --]

On Tue, Jan 29, 2019 at 11:07:50AM +0100, Geert Uytterhoeven wrote:
> On Tue, Jan 29, 2019 at 10:50 AM Jonas Bonn <jonas@norrbonn.se> wrote:

> > OK.  So the user (perhaps in userspace using spidev) has to know the
> > rate of the IO clock that the SPI controller sits behind and then has to
> > match this to the required delay of the slave device...  Doesn't sound
> > very portable.

I think if we're doing translation we should just do it in the core and
either say that clients should only set one or the other or pick what
looks like the higher value.

> I can see the value of having both:
> On some slaves, the delay may depend on a fixed internal or
> external clock[1] on the SPI slave, so it should be specified in time units.
> Some slaves may be clocked by the SPI clock[2], so the delay should be
> specified in SPI clock cycles.

Yes, they're definitely both useful.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]