[PATCH RESEND 0/5] spi: imx: Improve non 8-bit aligned words and dynamic bursts

[PATCH RESEND 0/5] spi: imx: Improve non 8-bit aligned words and dynamic bursts

[Maxime Chevallier]

[Resend with i.MX platform maintainers and reviewers in Cc: ]

This series aims to improve PIO transfers on the SPI imx driver for several
use-cases, where bits_per_words isn't a multiple of 8, or when using
dynamic_burst mode.

The first patch is just a cosmetic cleanup of extra variable assignments

The second patch enforces the use of the dynamic_burst mode only when we
can pack words into the 32 bits FIFO entries. This avoid having to mask out the
remaining parts of the words, and avoid shifting extra clock ticks.

The 3rd and 4th patches fixes the way we compute the number of bytes per words,
by using 4 bytes to transfer 24 bits words as expected by the core.

Finally, the 5th patch reworks the way dynamic bursts are emitted, by shifting
out the non 4-bytes aligned parts first as expected by the imx SPI controller.

This avoid splitting out transfer when not necessary.

This was tested on imx6s and imx6q, with and without DMA, in single and full
duplex with a wide range of transfer sizes and bit_per_words values.

I however couldn't test the slave mode with these patches, so some review is
very welcomed, especially on the last patch.

Thanks,

Maxime

Maxime Chevallier (5):
  spi: imx: Remove duplicate variable assignments
  spi: imx: Use dynamic bursts only when bits_per_word is 8, 16 or 32
  spi: imx: Use correct number of bytes per words
  spi: imx: remove unnecessary check in spi_imx_can_dma
  spi: imx: Use the longuest possible burst size when in dynamic_burst

 drivers/spi/spi-imx.c | 162 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 101 insertions(+), 61 deletions(-)

-- 
2.11.0

[PATCH RESEND 1/5] spi: imx: Remove duplicate variable assignments

[Maxime Chevallier]

Some fields in struct spi_imx_data are assigned a different value twice
in a row, in spi_imx_setupxfer.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/spi/spi-imx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index d3b21faf6b1f..d75153c995af 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1106,8 +1106,6 @@ static int spi_imx_setupxfer(struct spi_device *spi,
 	if (spi_imx->devtype_data->dynamic_burst && !spi_imx->slave_mode) {
 		u32 mask;
 
-		spi_imx->dynamic_burst = 0;
-		spi_imx->remainder = 0;
 		spi_imx->read_u32  = 1;
 
 		mask = (1 << spi_imx->bits_per_word) - 1;
-- 
2.11.0

[PATCH RESEND 2/5] spi: imx: Use dynamic bursts only when bits_per_word is 8, 16 or 32

[Maxime Chevallier]

The dynamic bursts mode allows to group together multiple words into a
single burst. To do so, it's necessary that words can be packed into the
32-bits FIFO entries, so we can't allow using this mode with bit_per_words
different to 8, 16 or 32.

This prevents shitfing out extra clock ticks for transfers with
bit_per_word values not aligned on 8 bits.

With that , we are sure that only the correct number of bits is
shifted out at each transfer, so we don't need to mask out the remaining
parts of the words.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/spi/spi-imx.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index d75153c995af..ecafbda5ec94 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -95,7 +95,6 @@ struct spi_imx_data {
 	const void *tx_buf;
 	unsigned int txfifo; /* number of words pushed in tx FIFO */
 	unsigned int dynamic_burst, read_u32;
-	unsigned int word_mask;
 
 	/* Slave mode */
 	bool slave_mode;
@@ -291,7 +290,6 @@ static void spi_imx_buf_rx_swap_u32(struct spi_imx_data *spi_imx)
 		else if (bytes_per_word == 2)
 			val = (val << 16) | (val >> 16);
 #endif
-		val &= spi_imx->word_mask;
 		*(u32 *)spi_imx->rx_buf = val;
 		spi_imx->rx_buf += sizeof(u32);
 	}
@@ -322,7 +320,6 @@ static void spi_imx_buf_tx_swap_u32(struct spi_imx_data *spi_imx)
 
 	if (spi_imx->tx_buf) {
 		val = *(u32 *)spi_imx->tx_buf;
-		val &= spi_imx->word_mask;
 		spi_imx->tx_buf += sizeof(u32);
 	}
 
@@ -1102,25 +1099,23 @@ static int spi_imx_setupxfer(struct spi_device *spi,
 	spi_imx->bits_per_word = t->bits_per_word;
 	spi_imx->speed_hz  = t->speed_hz;
 
-	/* Initialize the functions for transfer */
-	if (spi_imx->devtype_data->dynamic_burst && !spi_imx->slave_mode) {
-		u32 mask;
+	/*
+	 * Initialize the functions for transfer. To transfer non byte-aligned
+	 * words, we have to use multiple word-size bursts, we can't use
+	 * dynamic_burst in that case.
+	 */
+	if (spi_imx->devtype_data->dynamic_burst && !spi_imx->slave_mode &&
+	    (spi_imx->bits_per_word == 8 ||
+	    spi_imx->bits_per_word == 16 ||
+	    spi_imx->bits_per_word == 32)) {
 
 		spi_imx->read_u32  = 1;
 
-		mask = (1 << spi_imx->bits_per_word) - 1;
 		spi_imx->rx = spi_imx_buf_rx_swap;
 		spi_imx->tx = spi_imx_buf_tx_swap;
 		spi_imx->dynamic_burst = 1;
 		spi_imx->remainder = t->len;
 
-		if (spi_imx->bits_per_word <= 8)
-			spi_imx->word_mask = mask << 24 | mask << 16
-					     | mask << 8 | mask;
-		else if (spi_imx->bits_per_word <= 16)
-			spi_imx->word_mask = mask << 16 | mask;
-		else
-			spi_imx->word_mask = mask;
 	} else {
 		if (spi_imx->bits_per_word <= 8) {
 			spi_imx->rx = spi_imx_buf_rx_u8;
-- 
2.11.0

[PATCH RESEND 3/5] spi: imx: Use correct number of bytes per words

[Maxime Chevallier]

The SPI core enforces that we always use the next power-of-two number of
bytes to store words. As a result, a 24 bits word will be stored in 4
bytes.

This commit fixes the spi_imx_bytes_per_word function to return the
correct number of bytes.

This also allows to get rid of unnecessary checks in the can_dma
function, since the SPI core validates that we always have a transfer
length that is a multiple of the number of bytes per word.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/spi/spi-imx.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index ecafbda5ec94..3ae706dac660 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -202,7 +202,12 @@ static unsigned int spi_imx_clkdiv_2(unsigned int fin,
 
 static int spi_imx_bytes_per_word(const int bits_per_word)
 {
-	return DIV_ROUND_UP(bits_per_word, BITS_PER_BYTE);
+	if (bits_per_word <= 8)
+		return 1;
+	else if (bits_per_word <= 16)
+		return 2;
+	else
+		return 4;
 }
 
 static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
@@ -219,9 +224,6 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
 
 	bytes_per_word = spi_imx_bytes_per_word(transfer->bits_per_word);
 
-	if (bytes_per_word != 1 && bytes_per_word != 2 && bytes_per_word != 4)
-		return false;
-
 	for (i = spi_imx->devtype_data->fifo_size / 2; i > 0; i--) {
 		if (!(transfer->len % (i * bytes_per_word)))
 			break;
-- 
2.11.0

[PATCH RESEND 4/5] spi: imx: remove unnecessary check in spi_imx_can_dma

[Maxime Chevallier]

The spi_imx_can_dma function computes the watermark level so that
the transfer will fit in exactly N bursts (without a remainder).

The smallest watermark level possible being one FIFO entry per burst, we
can't never have a case where the transfer size isn't divsiible by 1.

Remove the extra check for the wml being different than 0.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/spi/spi-imx.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 3ae706dac660..ef6d3648396a 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -229,9 +229,6 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
 			break;
 	}
 
-	if (i == 0)
-		return false;
-
 	spi_imx->wml = i;
 	spi_imx->dynamic_burst = 0;
 
-- 
2.11.0

[PATCH RESEND 5/5] spi: imx: Use the longuest possible burst size when in dynamic_burst

[Maxime Chevallier]

Dynamic burst mode allows to group together multiple words and send them
in one continuous burst. When the number of bytes to be sent is not a
strict multiple of the FIFO entry size (32 bits), the controller expects
the non aligned bits to be sent first.

This commit adds support for this particular constraint, avoiding the
need to send the non-aligned bytes one by one at the end of the
transfer, speeding-up transfer speed in that case.

With this method, a transfer is divided into multiple bursts, limited in
size by the maximum amount of data that the controller can transfer in
one continuous burst (which is 512 bytes).

The non-512 byte part of the transfer is sent first. The remaining bytes
to be transferred in the current burst is stored in the 'remainder'
field.

With this method, the read_u32 field is no longer necessary, and is
removed.

This was tested on imx6 solo and imx6 quad.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/spi/spi-imx.c | 122 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 85 insertions(+), 37 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index ef6d3648396a..08dd3a31a3e5 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -94,7 +94,7 @@ struct spi_imx_data {
 	void *rx_buf;
 	const void *tx_buf;
 	unsigned int txfifo; /* number of words pushed in tx FIFO */
-	unsigned int dynamic_burst, read_u32;
+	unsigned int dynamic_burst;
 
 	/* Slave mode */
 	bool slave_mode;
@@ -139,6 +139,8 @@ static void spi_imx_buf_rx_##type(struct spi_imx_data *spi_imx)		\
 		*(type *)spi_imx->rx_buf = val;				\
 		spi_imx->rx_buf += sizeof(type);			\
 	}								\
+									\
+	spi_imx->remainder -= sizeof(type);				\
 }
 
 #define MXC_SPI_BUF_TX(type)						\
@@ -292,22 +294,36 @@ static void spi_imx_buf_rx_swap_u32(struct spi_imx_data *spi_imx)
 		*(u32 *)spi_imx->rx_buf = val;
 		spi_imx->rx_buf += sizeof(u32);
 	}
+
+	spi_imx->remainder -= sizeof(u32);
 }
 
 static void spi_imx_buf_rx_swap(struct spi_imx_data *spi_imx)
 {
-	unsigned int bytes_per_word;
+	int unaligned;
+	u32 val;
 
-	bytes_per_word = spi_imx_bytes_per_word(spi_imx->bits_per_word);
-	if (spi_imx->read_u32) {
+	unaligned = spi_imx->remainder % 4;
+
+	if (!unaligned) {
 		spi_imx_buf_rx_swap_u32(spi_imx);
 		return;
 	}
 
-	if (bytes_per_word == 1)
-		spi_imx_buf_rx_u8(spi_imx);
-	else if (bytes_per_word == 2)
+	if (spi_imx_bytes_per_word(spi_imx->bits_per_word) == 2) {
 		spi_imx_buf_rx_u16(spi_imx);
+		return;
+	}
+
+	val = readl(spi_imx->base + MXC_CSPIRXDATA);
+
+	while (unaligned--) {
+		if (spi_imx->rx_buf) {
+			*(u8 *)spi_imx->rx_buf = (val >> (8 * unaligned)) & 0xff;
+			spi_imx->rx_buf++;
+		}
+		spi_imx->remainder--;
+	}
 }
 
 static void spi_imx_buf_tx_swap_u32(struct spi_imx_data *spi_imx)
@@ -336,40 +352,30 @@ static void spi_imx_buf_tx_swap_u32(struct spi_imx_data *spi_imx)
 
 static void spi_imx_buf_tx_swap(struct spi_imx_data *spi_imx)
 {
-	u32 ctrl, val;
-	unsigned int bytes_per_word;
+	int unaligned;
+	u32 val = 0;
 
-	if (spi_imx->count == spi_imx->remainder) {
-		ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
-		ctrl &= ~MX51_ECSPI_CTRL_BL_MASK;
-		if (spi_imx->count > MX51_ECSPI_CTRL_MAX_BURST) {
-			spi_imx->remainder = spi_imx->count %
-					     MX51_ECSPI_CTRL_MAX_BURST;
-			val = MX51_ECSPI_CTRL_MAX_BURST * 8 - 1;
-		} else if (spi_imx->count >= sizeof(u32)) {
-			spi_imx->remainder = spi_imx->count % sizeof(u32);
-			val = (spi_imx->count - spi_imx->remainder) * 8 - 1;
-		} else {
-			spi_imx->remainder = 0;
-			val = spi_imx->bits_per_word - 1;
-			spi_imx->read_u32 = 0;
-		}
+	unaligned = spi_imx->count % 4;
 
-		ctrl |= (val << MX51_ECSPI_CTRL_BL_OFFSET);
-		writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
+	if (!unaligned) {
+		spi_imx_buf_tx_swap_u32(spi_imx);
+		return;
 	}
 
-	if (spi_imx->count >= sizeof(u32)) {
-		spi_imx_buf_tx_swap_u32(spi_imx);
+	if (spi_imx_bytes_per_word(spi_imx->bits_per_word) == 2) {
+		spi_imx_buf_tx_u16(spi_imx);
 		return;
 	}
 
-	bytes_per_word = spi_imx_bytes_per_word(spi_imx->bits_per_word);
+	while (unaligned--) {
+		if (spi_imx->tx_buf) {
+			val |= *(u8 *)spi_imx->tx_buf << (8 * unaligned);
+			spi_imx->tx_buf++;
+		}
+		spi_imx->count--;
+	}
 
-	if (bytes_per_word == 1)
-		spi_imx_buf_tx_u8(spi_imx);
-	else if (bytes_per_word == 2)
-		spi_imx_buf_tx_u16(spi_imx);
+	writel(val, spi_imx->base + MXC_CSPITXDATA);
 }
 
 static void mx53_ecspi_rx_slave(struct spi_imx_data *spi_imx)
@@ -388,6 +394,8 @@ static void mx53_ecspi_rx_slave(struct spi_imx_data *spi_imx)
 		spi_imx->rx_buf += n_bytes;
 		spi_imx->slave_burst -= n_bytes;
 	}
+
+	spi_imx->remainder -= sizeof(u32);
 }
 
 static void mx53_ecspi_tx_slave(struct spi_imx_data *spi_imx)
@@ -997,12 +1005,52 @@ static void spi_imx_chipselect(struct spi_device *spi, int is_active)
 	gpio_set_value(spi->cs_gpio, dev_is_lowactive ^ active);
 }
 
+static void spi_imx_set_burst_len(struct spi_imx_data *spi_imx, int n_bits)
+{
+	u32 ctrl;
+
+	ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
+	ctrl &= ~MX51_ECSPI_CTRL_BL_MASK;
+	ctrl |= ((n_bits - 1) << MX51_ECSPI_CTRL_BL_OFFSET);
+	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
+}
+
 static void spi_imx_push(struct spi_imx_data *spi_imx)
 {
+	unsigned int burst_len, fifo_words;
+
+	if (spi_imx->dynamic_burst)
+		fifo_words = 4;
+	else
+		fifo_words = spi_imx_bytes_per_word(spi_imx->bits_per_word);
+	/*
+	 * Reload the FIFO when the remaining bytes to be transferred in the
+	 * current burst is 0. This only applies when bits_per_word is a
+	 * multiple of 8.
+	 */
+	if (!spi_imx->remainder) {
+		if (spi_imx->dynamic_burst) {
+
+			/* We need to deal unaligned data first */
+			burst_len = spi_imx->count % MX51_ECSPI_CTRL_MAX_BURST;
+
+			if (!burst_len)
+				burst_len = MX51_ECSPI_CTRL_MAX_BURST;
+
+			spi_imx_set_burst_len(spi_imx, burst_len * 8);
+
+			spi_imx->remainder = burst_len;
+		} else {
+			spi_imx->remainder = fifo_words;
+		}
+	}
+
 	while (spi_imx->txfifo < spi_imx->devtype_data->fifo_size) {
 		if (!spi_imx->count)
 			break;
-		if (spi_imx->txfifo && (spi_imx->count == spi_imx->remainder))
+		if (spi_imx->dynamic_burst &&
+		    spi_imx->txfifo >=  DIV_ROUND_UP(spi_imx->remainder,
+						     fifo_words))
 			break;
 		spi_imx->tx(spi_imx);
 		spi_imx->txfifo++;
@@ -1108,12 +1156,9 @@ static int spi_imx_setupxfer(struct spi_device *spi,
 	    spi_imx->bits_per_word == 16 ||
 	    spi_imx->bits_per_word == 32)) {
 
-		spi_imx->read_u32  = 1;
-
 		spi_imx->rx = spi_imx_buf_rx_swap;
 		spi_imx->tx = spi_imx_buf_tx_swap;
 		spi_imx->dynamic_burst = 1;
-		spi_imx->remainder = t->len;
 
 	} else {
 		if (spi_imx->bits_per_word <= 8) {
@@ -1126,6 +1171,7 @@ static int spi_imx_setupxfer(struct spi_device *spi,
 			spi_imx->rx = spi_imx_buf_rx_u32;
 			spi_imx->tx = spi_imx_buf_tx_u32;
 		}
+		spi_imx->dynamic_burst = 0;
 	}
 
 	if (spi_imx_can_dma(spi_imx->bitbang.master, spi, t))
@@ -1309,6 +1355,7 @@ static int spi_imx_pio_transfer(struct spi_device *spi,
 	spi_imx->rx_buf = transfer->rx_buf;
 	spi_imx->count = transfer->len;
 	spi_imx->txfifo = 0;
+	spi_imx->remainder = 0;
 
 	reinit_completion(&spi_imx->xfer_done);
 
@@ -1346,6 +1393,7 @@ static int spi_imx_pio_transfer_slave(struct spi_device *spi,
 	spi_imx->rx_buf = transfer->rx_buf;
 	spi_imx->count = transfer->len;
 	spi_imx->txfifo = 0;
+	spi_imx->remainder = 0;
 
 	reinit_completion(&spi_imx->xfer_done);
 	spi_imx->slave_aborted = false;
-- 
2.11.0

Re: [PATCH RESEND 1/5] spi: imx: Remove duplicate variable assignments

[Sascha Hauer]

On Tue, Jul 17, 2018 at 04:31:50PM +0200, Maxime Chevallier wrote:
> Some fields in struct spi_imx_data are assigned a different value twice
> in a row, in spi_imx_setupxfer.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  drivers/spi/spi-imx.c | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>

Sascha

> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index d3b21faf6b1f..d75153c995af 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -1106,8 +1106,6 @@ static int spi_imx_setupxfer(struct spi_device *spi,
>  	if (spi_imx->devtype_data->dynamic_burst && !spi_imx->slave_mode) {
>  		u32 mask;
>  
> -		spi_imx->dynamic_burst = 0;
> -		spi_imx->remainder = 0;
>  		spi_imx->read_u32  = 1;
>  
>  		mask = (1 << spi_imx->bits_per_word) - 1;
> -- 
> 2.11.0
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH RESEND 4/5] spi: imx: remove unnecessary check in spi_imx_can_dma

[Sascha Hauer]

On Tue, Jul 17, 2018 at 04:31:53PM +0200, Maxime Chevallier wrote:
> The spi_imx_can_dma function computes the watermark level so that
> the transfer will fit in exactly N bursts (without a remainder).
> 
> The smallest watermark level possible being one FIFO entry per burst, we
> can't never have a case where the transfer size isn't divsiible by 1.

s/can't never/can never/
s/divsiible/divisible/

Otherwise:

Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>

Sascha

> 
> Remove the extra check for the wml being different than 0.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>  drivers/spi/spi-imx.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 3ae706dac660..ef6d3648396a 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -229,9 +229,6 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
>  			break;
>  	}
>  
> -	if (i == 0)
> -		return false;
> -
>  	spi_imx->wml = i;
>  	spi_imx->dynamic_burst = 0;
>  
> -- 
> 2.11.0
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH RESEND 3/5] spi: imx: Use correct number of bytes per words

[Sascha Hauer]

On Tue, Jul 17, 2018 at 04:31:52PM +0200, Maxime Chevallier wrote:
> The SPI core enforces that we always use the next power-of-two number of
> bytes to store words. As a result, a 24 bits word will be stored in 4
> bytes.
> 
> This commit fixes the spi_imx_bytes_per_word function to return the
> correct number of bytes.
> 
> This also allows to get rid of unnecessary checks in the can_dma
> function, since the SPI core validates that we always have a transfer
> length that is a multiple of the number of bytes per word.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>

Sascha

> ---
>  drivers/spi/spi-imx.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index ecafbda5ec94..3ae706dac660 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -202,7 +202,12 @@ static unsigned int spi_imx_clkdiv_2(unsigned int fin,
>  
>  static int spi_imx_bytes_per_word(const int bits_per_word)
>  {
> -	return DIV_ROUND_UP(bits_per_word, BITS_PER_BYTE);
> +	if (bits_per_word <= 8)
> +		return 1;
> +	else if (bits_per_word <= 16)
> +		return 2;
> +	else
> +		return 4;
>  }
>  
>  static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
> @@ -219,9 +224,6 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
>  
>  	bytes_per_word = spi_imx_bytes_per_word(transfer->bits_per_word);
>  
> -	if (bytes_per_word != 1 && bytes_per_word != 2 && bytes_per_word != 4)
> -		return false;
> -
>  	for (i = spi_imx->devtype_data->fifo_size / 2; i > 0; i--) {
>  		if (!(transfer->len % (i * bytes_per_word)))
>  			break;
> -- 
> 2.11.0
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH RESEND 2/5] spi: imx: Use dynamic bursts only when bits_per_word is 8, 16 or 32

[Sascha Hauer]

On Tue, Jul 17, 2018 at 04:31:51PM +0200, Maxime Chevallier wrote:
> The dynamic bursts mode allows to group together multiple words into a
> single burst. To do so, it's necessary that words can be packed into the
> 32-bits FIFO entries, so we can't allow using this mode with bit_per_words
> different to 8, 16 or 32.
> 
> This prevents shitfing out extra clock ticks for transfers with

s/shitfing/shifting/

> bit_per_word values not aligned on 8 bits.
> 
> With that , we are sure that only the correct number of bits is
> shifted out at each transfer, so we don't need to mask out the remaining
> parts of the words.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Sascha Hauer <s.hauer@pengutronix.de>

Sascha

> ---
>  drivers/spi/spi-imx.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index d75153c995af..ecafbda5ec94 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -95,7 +95,6 @@ struct spi_imx_data {
>  	const void *tx_buf;
>  	unsigned int txfifo; /* number of words pushed in tx FIFO */
>  	unsigned int dynamic_burst, read_u32;
> -	unsigned int word_mask;
>  
>  	/* Slave mode */
>  	bool slave_mode;
> @@ -291,7 +290,6 @@ static void spi_imx_buf_rx_swap_u32(struct spi_imx_data *spi_imx)
>  		else if (bytes_per_word == 2)
>  			val = (val << 16) | (val >> 16);
>  #endif
> -		val &= spi_imx->word_mask;
>  		*(u32 *)spi_imx->rx_buf = val;
>  		spi_imx->rx_buf += sizeof(u32);
>  	}
> @@ -322,7 +320,6 @@ static void spi_imx_buf_tx_swap_u32(struct spi_imx_data *spi_imx)
>  
>  	if (spi_imx->tx_buf) {
>  		val = *(u32 *)spi_imx->tx_buf;
> -		val &= spi_imx->word_mask;
>  		spi_imx->tx_buf += sizeof(u32);
>  	}
>  
> @@ -1102,25 +1099,23 @@ static int spi_imx_setupxfer(struct spi_device *spi,
>  	spi_imx->bits_per_word = t->bits_per_word;
>  	spi_imx->speed_hz  = t->speed_hz;
>  
> -	/* Initialize the functions for transfer */
> -	if (spi_imx->devtype_data->dynamic_burst && !spi_imx->slave_mode) {
> -		u32 mask;
> +	/*
> +	 * Initialize the functions for transfer. To transfer non byte-aligned
> +	 * words, we have to use multiple word-size bursts, we can't use
> +	 * dynamic_burst in that case.
> +	 */
> +	if (spi_imx->devtype_data->dynamic_burst && !spi_imx->slave_mode &&
> +	    (spi_imx->bits_per_word == 8 ||
> +	    spi_imx->bits_per_word == 16 ||
> +	    spi_imx->bits_per_word == 32)) {
>  
>  		spi_imx->read_u32  = 1;
>  
> -		mask = (1 << spi_imx->bits_per_word) - 1;
>  		spi_imx->rx = spi_imx_buf_rx_swap;
>  		spi_imx->tx = spi_imx_buf_tx_swap;
>  		spi_imx->dynamic_burst = 1;
>  		spi_imx->remainder = t->len;
>  
> -		if (spi_imx->bits_per_word <= 8)
> -			spi_imx->word_mask = mask << 24 | mask << 16
> -					     | mask << 8 | mask;
> -		else if (spi_imx->bits_per_word <= 16)
> -			spi_imx->word_mask = mask << 16 | mask;
> -		else
> -			spi_imx->word_mask = mask;
>  	} else {
>  		if (spi_imx->bits_per_word <= 8) {
>  			spi_imx->rx = spi_imx_buf_rx_u8;
> -- 
> 2.11.0
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |