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

[PATCH v4 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..6389a228d2f5 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_us) {
+		csr |= SPI_BF(DLYBCT,
+			clamp_t(u8,
+				(as->spi_clk/1000000*spi->word_delay_us)>>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 v4 2/2] spi-atmel: support inter-word delay

[Nicolas.Ferre]

On 29/01/2019 at 09:38, Jonas Bonn wrote:
> 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..6389a228d2f5 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_us) {

Well...

> +		csr |= SPI_BF(DLYBCT,
> +			clamp_t(u8,
> +				(as->spi_clk/1000000*spi->word_delay_us)>>5,
> +				1, 255));

... why not simplifying to:
+				0, 255));
and remove the test altogether, after all?

> +	} 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;
> 


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

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

[Jonas Bonn]

Hi,

On 29/01/2019 15:27, Nicolas.Ferre@microchip.com wrote:
> On 29/01/2019 at 09:38, Jonas Bonn wrote:
>> 
>> +	/* 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_us) {
> 
> Well...
> 
>> +		csr |= SPI_BF(DLYBCT,
>> +			clamp_t(u8,
>> +				(as->spi_clk/1000000*spi->word_delay_us)>>5,
>> +				1, 255));
> 
> ... why not simplifying to:
> +				0, 255));
> and remove the test altogether, after all?

Hmm... that seemed too easy!  This started out as something else and 
looking at it now I think even the clamp_t() is unnecessary.  The value 
is already 0-255 and the way SPI_BF works any overflow is already 
truncated...  I'll rework this and resubmit once I get some feedback on 
the word_delay_us bits.

Thanks,
/Jonas

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

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

[Alexandre Belloni]

Hi,

On 29/01/2019 15:56:31+0100, Jonas Bonn wrote:
> On 29/01/2019 15:27, Nicolas.Ferre@microchip.com wrote:
> > On 29/01/2019 at 09:38, Jonas Bonn wrote:
> > > 
> > > +	/* 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_us) {
> > 
> > Well...
> > 
> > > +		csr |= SPI_BF(DLYBCT,
> > > +			clamp_t(u8,
> > > +				(as->spi_clk/1000000*spi->word_delay_us)>>5,
> > > +				1, 255));
> > 
> > ... why not simplifying to:
> > +				0, 255));
> > and remove the test altogether, after all?
> 
> Hmm... that seemed too easy!  This started out as something else and looking
> at it now I think even the clamp_t() is unnecessary.  The value is already
> 0-255 and the way SPI_BF works any overflow is already truncated...  I'll
> rework this and resubmit once I get some feedback on the word_delay_us bits.
> 

While at it, note that you need to add spaces around the operators.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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