[PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Chuanhua Han]

Before we add this spi_transfer to the spi_message chain table, we need
bits_per_word_mask based on spi_control to set the bits_per_word of
this spi_transfer.

Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
---
 drivers/spi/spi-mem.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index eb72dba71d83..717e711c0952 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -175,6 +175,41 @@ bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
 }
 EXPORT_SYMBOL_GPL(spi_mem_supports_op);
 
+/**
+ * spi_set_xfer_bpw() - Set the bits_per_word for each transfer based on
+ *			the bits_per_word_mask of the spi controller
+ * @ctrl: the spi controller
+ * @xfer: the spi transfer
+ *
+ * This function sets the bits_per_word for each transfer based on the spi
+ * controller's bits_per_word_mask to improve the efficiency of spi transport.
+ *
+ * Return: 0 in case of success, a negative error code otherwise.
+ */
+int spi_set_xfer_bpw(struct spi_controller *ctlr, struct spi_transfer *xfer)
+{
+	if (!ctlr || !xfer) {
+		dev_err(&ctlr->dev,
+			"Fail to set bits_per_word for spi transfer\n");
+		return -EINVAL;
+	}
+
+	if (ctlr->bits_per_word_mask) {
+		if (!(xfer->len % 4)) {
+			if (ctlr->bits_per_word_mask & SPI_BPW_MASK(32))
+				xfer->bits_per_word = 32;
+		} else if (!(xfer->len % 2)) {
+			if (ctlr->bits_per_word_mask & SPI_BPW_MASK(16))
+				xfer->bits_per_word = 16;
+		} else {
+			xfer->bits_per_word = 8;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spi_set_xfer_bpw);
+
 /**
  * spi_mem_exec_op() - Execute a memory operation
  * @mem: the SPI memory
@@ -252,6 +287,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 	xfers[xferpos].tx_buf = tmpbuf;
 	xfers[xferpos].len = sizeof(op->cmd.opcode);
 	xfers[xferpos].tx_nbits = op->cmd.buswidth;
+	spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
 	spi_message_add_tail(&xfers[xferpos], &msg);
 	xferpos++;
 	totalxferlen++;
@@ -266,6 +302,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 		xfers[xferpos].tx_buf = tmpbuf + 1;
 		xfers[xferpos].len = op->addr.nbytes;
 		xfers[xferpos].tx_nbits = op->addr.buswidth;
+		spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
 		spi_message_add_tail(&xfers[xferpos], &msg);
 		xferpos++;
 		totalxferlen += op->addr.nbytes;
@@ -276,6 +313,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 		xfers[xferpos].tx_buf = tmpbuf + op->addr.nbytes + 1;
 		xfers[xferpos].len = op->dummy.nbytes;
 		xfers[xferpos].tx_nbits = op->dummy.buswidth;
+		spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
 		spi_message_add_tail(&xfers[xferpos], &msg);
 		xferpos++;
 		totalxferlen += op->dummy.nbytes;
@@ -291,6 +329,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 		}
 
 		xfers[xferpos].len = op->data.nbytes;
+		spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
 		spi_message_add_tail(&xfers[xferpos], &msg);
 		xferpos++;
 		totalxferlen += op->data.nbytes;
-- 
2.17.1

[PATCH 2/2] spi: spi-fsl-dspi: Fix support for XSPI transport mode

[Chuanhua Han]

This patch fixes the problem that the XSPI mode of the dspi controller
cannot transfer data properly.
In XSPI mode, cmd_fifo is written before tx_fifo, which transforms the
byte order of sending and receiving data.

Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
---
 drivers/spi/spi-fsl-dspi.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 3082e72e4f6c..44cc2bd0120e 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -220,9 +220,15 @@ static u32 dspi_pop_tx(struct fsl_dspi *dspi)
 		if (dspi->bytes_per_word == 1)
 			txdata = *(u8 *)dspi->tx;
 		else if (dspi->bytes_per_word == 2)
-			txdata = *(u16 *)dspi->tx;
+			if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
+				txdata =  cpu_to_le16(*(u16 *)dspi->tx);
+			else
+				txdata =  cpu_to_be16(*(u16 *)dspi->tx);
 		else  /* dspi->bytes_per_word == 4 */
-			txdata = *(u32 *)dspi->tx;
+			if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
+				txdata = cpu_to_le32(*(u32 *)dspi->tx);
+			else
+				txdata = cpu_to_be32(*(u32 *)dspi->tx);
 		dspi->tx += dspi->bytes_per_word;
 	}
 	dspi->len -= dspi->bytes_per_word;
@@ -243,15 +249,18 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
 	if (!dspi->rx)
 		return;
 
-	/* Mask of undefined bits */
-	rxdata &= (1 << dspi->bits_per_word) - 1;
-
 	if (dspi->bytes_per_word == 1)
 		*(u8 *)dspi->rx = rxdata;
 	else if (dspi->bytes_per_word == 2)
-		*(u16 *)dspi->rx = rxdata;
+		if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
+			*(u16 *)dspi->rx = be16_to_cpu(rxdata);
+		else
+			*(u16 *)dspi->rx = be16_to_cpu(rxdata);
 	else /* dspi->bytes_per_word == 4 */
-		*(u32 *)dspi->rx = rxdata;
+		if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
+			*(u32 *)dspi->rx = le32_to_cpu(rxdata);
+		else
+			*(u32 *)dspi->rx = be32_to_cpu(rxdata);
 	dspi->rx += dspi->bytes_per_word;
 }
 
@@ -593,16 +602,16 @@ static void dspi_tcfq_write(struct fsl_dspi *dspi)
 		 */
 		u32 data = dspi_pop_tx(dspi);
 
+		cmd_fifo_write(dspi);
 		if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1)) {
 			/* LSB */
-			tx_fifo_write(dspi, data & 0xFFFF);
 			tx_fifo_write(dspi, data >> 16);
+			tx_fifo_write(dspi, data & 0xFFFF);
 		} else {
 			/* MSB */
-			tx_fifo_write(dspi, data >> 16);
 			tx_fifo_write(dspi, data & 0xFFFF);
+			tx_fifo_write(dspi, data >> 16);
 		}
-		cmd_fifo_write(dspi);
 	} else {
 		/* Write one entry to both TX FIFO and CMD FIFO
 		 * simultaneously.
-- 
2.17.1

RE: [PATCH 2/2] spi: spi-fsl-dspi: Fix support for XSPI transport mode

[Chuanhua Han]

> -----Original Message-----
> From: Chuanhua Han <chuanhua.han@nxp.com>
> Sent: 2018年9月21日 15:06
> To: broonie@kernel.org
> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; eha@deif.com;
> boris.brezillon@bootlin.com; Chuanhua Han <chuanhua.han@nxp.com>
> Subject: [PATCH 2/2] spi: spi-fsl-dspi: Fix support for XSPI transport mode
> 
> This patch fixes the problem that the XSPI mode of the dspi controller cannot
> transfer data properly.
> In XSPI mode, cmd_fifo is written before tx_fifo, which transforms the byte
> order of sending and receiving data.
> 
> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> ---
>  drivers/spi/spi-fsl-dspi.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c index
> 3082e72e4f6c..44cc2bd0120e 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -220,9 +220,15 @@ static u32 dspi_pop_tx(struct fsl_dspi *dspi)
>  		if (dspi->bytes_per_word == 1)
>  			txdata = *(u8 *)dspi->tx;
>  		else if (dspi->bytes_per_word == 2)
> -			txdata = *(u16 *)dspi->tx;
> +			if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
> +				txdata =  cpu_to_le16(*(u16 *)dspi->tx);
> +			else
> +				txdata =  cpu_to_be16(*(u16 *)dspi->tx);
>  		else  /* dspi->bytes_per_word == 4 */
> -			txdata = *(u32 *)dspi->tx;
> +			if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
> +				txdata = cpu_to_le32(*(u32 *)dspi->tx);
> +			else
> +				txdata = cpu_to_be32(*(u32 *)dspi->tx);
>  		dspi->tx += dspi->bytes_per_word;
>  	}
>  	dspi->len -= dspi->bytes_per_word;
> @@ -243,15 +249,18 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32
> rxdata)
>  	if (!dspi->rx)
>  		return;
> 
> -	/* Mask of undefined bits */
> -	rxdata &= (1 << dspi->bits_per_word) - 1;
> -
>  	if (dspi->bytes_per_word == 1)
>  		*(u8 *)dspi->rx = rxdata;
>  	else if (dspi->bytes_per_word == 2)
> -		*(u16 *)dspi->rx = rxdata;
> +		if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
> +			*(u16 *)dspi->rx = be16_to_cpu(rxdata);
> +		else
> +			*(u16 *)dspi->rx = be16_to_cpu(rxdata);
>  	else /* dspi->bytes_per_word == 4 */
> -		*(u32 *)dspi->rx = rxdata;
> +		if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
> +			*(u32 *)dspi->rx = le32_to_cpu(rxdata);
> +		else
> +			*(u32 *)dspi->rx = be32_to_cpu(rxdata);
>  	dspi->rx += dspi->bytes_per_word;
>  }
> 
> @@ -593,16 +602,16 @@ static void dspi_tcfq_write(struct fsl_dspi *dspi)
>  		 */
>  		u32 data = dspi_pop_tx(dspi);
> 
> +		cmd_fifo_write(dspi);
>  		if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1)) {
>  			/* LSB */
> -			tx_fifo_write(dspi, data & 0xFFFF);
>  			tx_fifo_write(dspi, data >> 16);
> +			tx_fifo_write(dspi, data & 0xFFFF);
>  		} else {
>  			/* MSB */
> -			tx_fifo_write(dspi, data >> 16);
>  			tx_fifo_write(dspi, data & 0xFFFF);
> +			tx_fifo_write(dspi, data >> 16);
>  		}
> -		cmd_fifo_write(dspi);
>  	} else {
>  		/* Write one entry to both TX FIFO and CMD FIFO
>  		 * simultaneously.
> --
> 2.17.1
Hi,all
Could you please help me to see the fix of this patch? What changes need to be made? 
Looking forward to your valuable comments and criticism, thank you very much!!!
Thanks,
Chuanhua

Re: [PATCH 2/2] spi: spi-fsl-dspi: Fix support for XSPI transport mode

[Boris Brezillon]

Hi Chuanhua,

On Fri, 21 Sep 2018 15:06:27 +0800
Chuanhua Han <chuanhua.han@nxp.com> wrote:

> This patch fixes the problem that the XSPI mode of the dspi controller
> cannot transfer data properly.
> In XSPI mode, cmd_fifo is written before tx_fifo, which transforms the
> byte order of sending and receiving data.

Again, I have a hard time understanding what the problem is. It doesn't
seem related to the ->bits_per_word aspect, and I have the feeling
you're actually abusing this field to get your problem fixed.

Regards,

Boris

> 
> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> ---
>  drivers/spi/spi-fsl-dspi.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 3082e72e4f6c..44cc2bd0120e 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -220,9 +220,15 @@ static u32 dspi_pop_tx(struct fsl_dspi *dspi)
>  		if (dspi->bytes_per_word == 1)
>  			txdata = *(u8 *)dspi->tx;
>  		else if (dspi->bytes_per_word == 2)
> -			txdata = *(u16 *)dspi->tx;
> +			if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
> +				txdata =  cpu_to_le16(*(u16 *)dspi->tx);
> +			else
> +				txdata =  cpu_to_be16(*(u16 *)dspi->tx);
>  		else  /* dspi->bytes_per_word == 4 */
> -			txdata = *(u32 *)dspi->tx;
> +			if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
> +				txdata = cpu_to_le32(*(u32 *)dspi->tx);
> +			else
> +				txdata = cpu_to_be32(*(u32 *)dspi->tx);
>  		dspi->tx += dspi->bytes_per_word;
>  	}
>  	dspi->len -= dspi->bytes_per_word;
> @@ -243,15 +249,18 @@ static void dspi_push_rx(struct fsl_dspi *dspi, u32 rxdata)
>  	if (!dspi->rx)
>  		return;
>  
> -	/* Mask of undefined bits */
> -	rxdata &= (1 << dspi->bits_per_word) - 1;
> -
>  	if (dspi->bytes_per_word == 1)
>  		*(u8 *)dspi->rx = rxdata;
>  	else if (dspi->bytes_per_word == 2)
> -		*(u16 *)dspi->rx = rxdata;
> +		if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
> +			*(u16 *)dspi->rx = be16_to_cpu(rxdata);
> +		else
> +			*(u16 *)dspi->rx = be16_to_cpu(rxdata);
>  	else /* dspi->bytes_per_word == 4 */
> -		*(u32 *)dspi->rx = rxdata;
> +		if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1))
> +			*(u32 *)dspi->rx = le32_to_cpu(rxdata);
> +		else
> +			*(u32 *)dspi->rx = be32_to_cpu(rxdata);
>  	dspi->rx += dspi->bytes_per_word;
>  }
>  
> @@ -593,16 +602,16 @@ static void dspi_tcfq_write(struct fsl_dspi *dspi)
>  		 */
>  		u32 data = dspi_pop_tx(dspi);
>  
> +		cmd_fifo_write(dspi);
>  		if (dspi->cur_chip->ctar_val & SPI_CTAR_LSBFE(1)) {
>  			/* LSB */
> -			tx_fifo_write(dspi, data & 0xFFFF);
>  			tx_fifo_write(dspi, data >> 16);
> +			tx_fifo_write(dspi, data & 0xFFFF);
>  		} else {
>  			/* MSB */
> -			tx_fifo_write(dspi, data >> 16);
>  			tx_fifo_write(dspi, data & 0xFFFF);
> +			tx_fifo_write(dspi, data >> 16);
>  		}
> -		cmd_fifo_write(dspi);
>  	} else {
>  		/* Write one entry to both TX FIFO and CMD FIFO
>  		 * simultaneously.

Re: [PATCH 2/2] spi: spi-fsl-dspi: Fix support for XSPI transport mode

[Esben Haabendal]

Chuanhua Han <chuanhua.han@nxp.com> writes:

> This patch fixes the problem that the XSPI mode of the dspi controller
> cannot transfer data properly.
> In XSPI mode, cmd_fifo is written before tx_fifo, which transforms the
> byte order of sending and receiving data.

Did you find documentation on proper ordering of writes to related
TX FIFO and CMD FIFO entries?

I have failed to find such information, and thus opted for what I
believed would be the safe approach, writing to TX FIFO first, so that
when CMD FIFO is written, it will already have data in place.  And it
seems to work.

But, I now see that SPIx_SR[TFIWF] hints that it should be done the
other way around.

    Tranmit FIFO Invalid Write Flag - Indicates Data Write on TX FIFO
    while CMD FIFO is empty. Without a Command, the Data entries present
    in TXFIFO are invalid.

But I fail to see how that should be related to byte ordering.

So I believe this patch is doing two things.

1. Changing write ordering of TX FIFO and CMD FIFO.
2. Handling byte ordering based on SPIx_CTARn[LSBFE] flag.

It would be nice if we could get clarification from NXP on
what is the right way to do the TX FIFO and CMD FIFO write ordering.

But as for the byte ordering changes, I don't think it looks write.  The
meaning of SPIx_CTARn[LSBFE] is according to the documentaiton the bit
ordering on the wire, and should not be related to register byte
ordering.

You should probably split this patch in two, so they can be reviewed and
merged independently.

/Esben

Re: [PATCH 2/2] spi: spi-fsl-dspi: Fix support for XSPI transport mode

[Boris Brezillon]

Hi Esben,

On Sat, 29 Sep 2018 16:56:17 +0200
Esben Haabendal <esben.haabendal@gmail.com> wrote:

> Chuanhua Han <chuanhua.han@nxp.com> writes:
> 
> > This patch fixes the problem that the XSPI mode of the dspi controller
> > cannot transfer data properly.
> > In XSPI mode, cmd_fifo is written before tx_fifo, which transforms the
> > byte order of sending and receiving data.  
> 
> Did you find documentation on proper ordering of writes to related
> TX FIFO and CMD FIFO entries?
> 
> I have failed to find such information, and thus opted for what I
> believed would be the safe approach, writing to TX FIFO first, so that
> when CMD FIFO is written, it will already have data in place.  And it
> seems to work.
> 
> But, I now see that SPIx_SR[TFIWF] hints that it should be done the
> other way around.
> 
>     Tranmit FIFO Invalid Write Flag - Indicates Data Write on TX FIFO
>     while CMD FIFO is empty. Without a Command, the Data entries present
>     in TXFIFO are invalid.
> 
> But I fail to see how that should be related to byte ordering.
> 
> So I believe this patch is doing two things.
> 
> 1. Changing write ordering of TX FIFO and CMD FIFO.
> 2. Handling byte ordering based on SPIx_CTARn[LSBFE] flag.
> 
> It would be nice if we could get clarification from NXP on
> what is the right way to do the TX FIFO and CMD FIFO write ordering.
> 
> But as for the byte ordering changes, I don't think it looks write.  The
> meaning of SPIx_CTARn[LSBFE] is according to the documentaiton the bit
> ordering on the wire, and should not be related to register byte
> ordering.
> 
> You should probably split this patch in two, so they can be reviewed and
> merged independently.

Yes, I forgot to mention that, but this patch should definitely be
split in at least 2 patches.

Regards,

Boris 

RE: [PATCH 2/2] spi: spi-fsl-dspi: Fix support for XSPI transport mode

[Chuanhua Han]

> -----Original Message-----
> From: Esben Haabendal <esbenhaabendal@gmail.com> On Behalf Of Esben
> Haabendal
> Sent: 2018年9月29日 22:56
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: broonie@kernel.org; linux-spi@vger.kernel.org;
> linux-kernel@vger.kernel.org; boris.brezillon@bootlin.com
> Subject: Re: [PATCH 2/2] spi: spi-fsl-dspi: Fix support for XSPI transport mode
> 
> Chuanhua Han <chuanhua.han@nxp.com> writes:
> 
> > This patch fixes the problem that the XSPI mode of the dspi controller
> > cannot transfer data properly.
> > In XSPI mode, cmd_fifo is written before tx_fifo, which transforms the
> > byte order of sending and receiving data.
> 
> Did you find documentation on proper ordering of writes to related TX FIFO
> and CMD FIFO entries?
> 
> I have failed to find such information, and thus opted for what I believed would
> be the safe approach, writing to TX FIFO first, so that when CMD FIFO is
> written, it will already have data in place.  And it seems to work.
> 
> But, I now see that SPIx_SR[TFIWF] hints that it should be done the other way
> around.
> 
>     Tranmit FIFO Invalid Write Flag - Indicates Data Write on TX FIFO
>     while CMD FIFO is empty. Without a Command, the Data entries present
>     in TXFIFO are invalid.
> 
> But I fail to see how that should be related to byte ordering.
> 
> So I believe this patch is doing two things.
> 
> 1. Changing write ordering of TX FIFO and CMD FIFO.
> 2. Handling byte ordering based on SPIx_CTARn[LSBFE] flag.
> 
> It would be nice if we could get clarification from NXP on what is the right way
> to do the TX FIFO and CMD FIFO write ordering.
> 
> But as for the byte ordering changes, I don't think it looks write.  The meaning
> of SPIx_CTARn[LSBFE] is according to the documentaiton the bit ordering on
> the wire, and should not be related to register byte ordering.
> 
> You should probably split this patch in two, so they can be reviewed and
> merged independently.
> 
> /Esben
Hi, Esben
First of all, thank you for your valuable advice. I'm going to do two things:
1. Divide this patch into multiple patches.
2. Verify (if you can) from the relevant NXP documentation.

Second, let me mention the issues I found on fixing fsl_dspi's XSPI pattern not working:
1. When I executed TX FIFO first and then CMD FIFO for XSPI transmission, I found that SPIx_SR[TFIWF]=1 (Invalid Data present in TX FIFO since CMD FIFO is empty).
 This is the time when no Data can be read or written (all the Data obtained is equal to 0).
2. When I read and write data without converting data byte order, and read and write data directly,
 I tested spi-flash connected by fsl_dspi controller, and found that data byte order was reversed with the correct byte order. 
When I changed the byte order according to the SPIx_CTARn[LSBFE] flag, the correct data was obtained (not explained in this data manual).
3. In the dspi_push_rx function, if you add your "rxdata &= (1 << dspi->bits_per_word) -1" statement, the rxdata=0 is obtained when xspi is transmitted, 
and the correct data cannot be transmitted at this time (I don't quite understand why you added this statement).
Thanks,
Chuanhua

[PATCH] spi: spi-mem: Adjust op len based on message/transfer size limitations

[Chuanhua Han]

We need that to adjust the len of the 2nd transfer (called data in
spi-mem) if it's too long to fit in a SPI message or SPI transfer.

Fixes: c36ff266dc82 ("spi: Extend the core to ease integration of SPI memory controllers")
Cc: <stable@vger.kernel.org>
Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-mem.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 990770dfa5cf..ec0c24e873cd 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -328,10 +328,25 @@ EXPORT_SYMBOL_GPL(spi_mem_exec_op);
 int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
 {
 	struct spi_controller *ctlr = mem->spi->controller;
+	size_t len;
+
+	len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
 
 	if (ctlr->mem_ops && ctlr->mem_ops->adjust_op_size)
 		return ctlr->mem_ops->adjust_op_size(mem, op);
 
+	if (!ctlr->mem_ops || !ctlr->mem_ops->exec_op) {
+		if (len > spi_max_transfer_size(mem->spi))
+			return -EINVAL;
+
+		op->data.nbytes = min3((size_t)op->data.nbytes,
+				       spi_max_transfer_size(mem->spi),
+				       spi_max_message_size(mem->spi) -
+				       len);
+		if (!op->data.nbytes)
+			return -EINVAL;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(spi_mem_adjust_op_size);
-- 
2.17.1

RE: [PATCH] spi: spi-mem: Adjust op len based on message/transfer size limitations

[Chuanhua Han]

> -----Original Message-----
> From: Chuanhua Han <chuanhua.han@nxp.com>
> Sent: 2018年9月21日 15:06
> To: broonie@kernel.org
> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; eha@deif.com;
> boris.brezillon@bootlin.com; Chuanhua Han <chuanhua.han@nxp.com>;
> stable@vger.kernel.org
> Subject: [PATCH] spi: spi-mem: Adjust op len based on message/transfer size
> limitations
> 
> We need that to adjust the len of the 2nd transfer (called data in
> spi-mem) if it's too long to fit in a SPI message or SPI transfer.
> 
> Fixes: c36ff266dc82 ("spi: Extend the core to ease integration of SPI memory
> controllers")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  drivers/spi/spi-mem.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index
> 990770dfa5cf..ec0c24e873cd 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -328,10 +328,25 @@ EXPORT_SYMBOL_GPL(spi_mem_exec_op);
>  int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
> {
>  	struct spi_controller *ctlr = mem->spi->controller;
> +	size_t len;
> +
> +	len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
> 
>  	if (ctlr->mem_ops && ctlr->mem_ops->adjust_op_size)
>  		return ctlr->mem_ops->adjust_op_size(mem, op);
> 
> +	if (!ctlr->mem_ops || !ctlr->mem_ops->exec_op) {
> +		if (len > spi_max_transfer_size(mem->spi))
> +			return -EINVAL;
> +
> +		op->data.nbytes = min3((size_t)op->data.nbytes,
> +				       spi_max_transfer_size(mem->spi),
> +				       spi_max_message_size(mem->spi) -
> +				       len);
> +		if (!op->data.nbytes)
> +			return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(spi_mem_adjust_op_size);
> --
> 2.17.1
I'm really sorry for my error in operation, this has been applied to the latest kernel, please ignore this patch in email!!!

RE: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Chuanhua Han]

> -----Original Message-----
> From: Chuanhua Han <chuanhua.han@nxp.com>
> Sent: 2018年9月21日 15:06
> To: broonie@kernel.org
> Cc: linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; eha@deif.com;
> boris.brezillon@bootlin.com; Chuanhua Han <chuanhua.han@nxp.com>
> Subject: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function
> 
> Before we add this spi_transfer to the spi_message chain table, we need
> bits_per_word_mask based on spi_control to set the bits_per_word of this
> spi_transfer.
> 
> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> ---
>  drivers/spi/spi-mem.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index
> eb72dba71d83..717e711c0952 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -175,6 +175,41 @@ bool spi_mem_supports_op(struct spi_mem *mem,
> const struct spi_mem_op *op)  }
> EXPORT_SYMBOL_GPL(spi_mem_supports_op);
> 
> +/**
> + * spi_set_xfer_bpw() - Set the bits_per_word for each transfer based on
> + *			the bits_per_word_mask of the spi controller
> + * @ctrl: the spi controller
> + * @xfer: the spi transfer
> + *
> + * This function sets the bits_per_word for each transfer based on the
> +spi
> + * controller's bits_per_word_mask to improve the efficiency of spi transport.
> + *
> + * Return: 0 in case of success, a negative error code otherwise.
> + */
> +int spi_set_xfer_bpw(struct spi_controller *ctlr, struct spi_transfer
> +*xfer) {
> +	if (!ctlr || !xfer) {
> +		dev_err(&ctlr->dev,
> +			"Fail to set bits_per_word for spi transfer\n");
> +		return -EINVAL;
> +	}
> +
> +	if (ctlr->bits_per_word_mask) {
> +		if (!(xfer->len % 4)) {
> +			if (ctlr->bits_per_word_mask & SPI_BPW_MASK(32))
> +				xfer->bits_per_word = 32;
> +		} else if (!(xfer->len % 2)) {
> +			if (ctlr->bits_per_word_mask & SPI_BPW_MASK(16))
> +				xfer->bits_per_word = 16;
> +		} else {
> +			xfer->bits_per_word = 8;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(spi_set_xfer_bpw);
> +
>  /**
>   * spi_mem_exec_op() - Execute a memory operation
>   * @mem: the SPI memory
> @@ -252,6 +287,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const
> struct spi_mem_op *op)
>  	xfers[xferpos].tx_buf = tmpbuf;
>  	xfers[xferpos].len = sizeof(op->cmd.opcode);
>  	xfers[xferpos].tx_nbits = op->cmd.buswidth;
> +	spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
>  	spi_message_add_tail(&xfers[xferpos], &msg);
>  	xferpos++;
>  	totalxferlen++;
> @@ -266,6 +302,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const
> struct spi_mem_op *op)
>  		xfers[xferpos].tx_buf = tmpbuf + 1;
>  		xfers[xferpos].len = op->addr.nbytes;
>  		xfers[xferpos].tx_nbits = op->addr.buswidth;
> +		spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
>  		spi_message_add_tail(&xfers[xferpos], &msg);
>  		xferpos++;
>  		totalxferlen += op->addr.nbytes;
> @@ -276,6 +313,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const
> struct spi_mem_op *op)
>  		xfers[xferpos].tx_buf = tmpbuf + op->addr.nbytes + 1;
>  		xfers[xferpos].len = op->dummy.nbytes;
>  		xfers[xferpos].tx_nbits = op->dummy.buswidth;
> +		spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
>  		spi_message_add_tail(&xfers[xferpos], &msg);
>  		xferpos++;
>  		totalxferlen += op->dummy.nbytes;
> @@ -291,6 +329,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const
> struct spi_mem_op *op)
>  		}
> 
>  		xfers[xferpos].len = op->data.nbytes;
> +		spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
>  		spi_message_add_tail(&xfers[xferpos], &msg);
>  		xferpos++;
>  		totalxferlen += op->data.nbytes;
> --
> 2.17.1
Hi,all
Could you please help me to see the fix of this patch? What changes need to be made? 
Looking forward to your valuable comments and criticism, thank you very much!!!
Thanks,
Chuanhua

Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Boris Brezillon]

Hi Chuanhua,

On Fri, 21 Sep 2018 15:06:26 +0800
Chuanhua Han <chuanhua.han@nxp.com> wrote:

> Before we add this spi_transfer to the spi_message chain table, we need
> bits_per_word_mask based on spi_control to set the bits_per_word of
> this spi_transfer.

It's not clear to me what you're trying to fix/improve. Can you give
more details on what the problem is?

> 
> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> ---
>  drivers/spi/spi-mem.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index eb72dba71d83..717e711c0952 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -175,6 +175,41 @@ bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  }
>  EXPORT_SYMBOL_GPL(spi_mem_supports_op);
>  
> +/**
> + * spi_set_xfer_bpw() - Set the bits_per_word for each transfer based on
> + *			the bits_per_word_mask of the spi controller
> + * @ctrl: the spi controller
> + * @xfer: the spi transfer
> + *
> + * This function sets the bits_per_word for each transfer based on the spi
> + * controller's bits_per_word_mask to improve the efficiency of spi transport.
> + *
> + * Return: 0 in case of success, a negative error code otherwise.
> + */
> +int spi_set_xfer_bpw(struct spi_controller *ctlr, struct spi_transfer *xfer)
> +{
> +	if (!ctlr || !xfer) {
> +		dev_err(&ctlr->dev,
> +			"Fail to set bits_per_word for spi transfer\n");
> +		return -EINVAL;
> +	}
> +
> +	if (ctlr->bits_per_word_mask) {
> +		if (!(xfer->len % 4)) {
> +			if (ctlr->bits_per_word_mask & SPI_BPW_MASK(32))
> +				xfer->bits_per_word = 32;
> +		} else if (!(xfer->len % 2)) {
> +			if (ctlr->bits_per_word_mask & SPI_BPW_MASK(16))
> +				xfer->bits_per_word = 16;
> +		} else {
> +			xfer->bits_per_word = 8;
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(spi_set_xfer_bpw);

Why is this function placed in spi-mem.c, and more importantly, why is
it exported?

> +
>  /**
>   * spi_mem_exec_op() - Execute a memory operation
>   * @mem: the SPI memory
> @@ -252,6 +287,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  	xfers[xferpos].tx_buf = tmpbuf;
>  	xfers[xferpos].len = sizeof(op->cmd.opcode);
>  	xfers[xferpos].tx_nbits = op->cmd.buswidth;
> +	spi_set_xfer_bpw(ctlr, &xfers[xferpos]);

It's still unclear why you need to specify a bits_per_word value, but
if this is needed, it's probably something you want to add to spi.c,
when a message is queued.

>  	spi_message_add_tail(&xfers[xferpos], &msg);
>  	xferpos++;
>  	totalxferlen++;
> @@ -266,6 +302,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  		xfers[xferpos].tx_buf = tmpbuf + 1;
>  		xfers[xferpos].len = op->addr.nbytes;
>  		xfers[xferpos].tx_nbits = op->addr.buswidth;
> +		spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
>  		spi_message_add_tail(&xfers[xferpos], &msg);
>  		xferpos++;
>  		totalxferlen += op->addr.nbytes;
> @@ -276,6 +313,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  		xfers[xferpos].tx_buf = tmpbuf + op->addr.nbytes + 1;
>  		xfers[xferpos].len = op->dummy.nbytes;
>  		xfers[xferpos].tx_nbits = op->dummy.buswidth;
> +		spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
>  		spi_message_add_tail(&xfers[xferpos], &msg);
>  		xferpos++;
>  		totalxferlen += op->dummy.nbytes;
> @@ -291,6 +329,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>  		}
>  
>  		xfers[xferpos].len = op->data.nbytes;
> +		spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
>  		spi_message_add_tail(&xfers[xferpos], &msg);
>  		xferpos++;
>  		totalxferlen += op->data.nbytes;

RE: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Chuanhua Han]

HI,Boris,

> -----Original Message-----
> From: Boris Brezillon <boris.brezillon@bootlin.com>
> Sent: 2018年9月28日 14:45
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: broonie@kernel.org; linux-spi@vger.kernel.org;
> linux-kernel@vger.kernel.org; eha@deif.com
> Subject: Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function
> 
> Hi Chuanhua,
> 
> On Fri, 21 Sep 2018 15:06:26 +0800
> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> 
> > Before we add this spi_transfer to the spi_message chain table, we
> > need bits_per_word_mask based on spi_control to set the bits_per_word
> > of this spi_transfer.
> 
> It's not clear to me what you're trying to fix/improve. Can you give more
> details on what the problem is?
> 
> >
> > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > ---
> >  drivers/spi/spi-mem.c | 39
> +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >
> > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index
> > eb72dba71d83..717e711c0952 100644
> > --- a/drivers/spi/spi-mem.c
> > +++ b/drivers/spi/spi-mem.c
> > @@ -175,6 +175,41 @@ bool spi_mem_supports_op(struct spi_mem
> *mem,
> > const struct spi_mem_op *op)  }
> > EXPORT_SYMBOL_GPL(spi_mem_supports_op);
> >
> > +/**
> > + * spi_set_xfer_bpw() - Set the bits_per_word for each transfer based on
> > + *			the bits_per_word_mask of the spi controller
> > + * @ctrl: the spi controller
> > + * @xfer: the spi transfer
> > + *
> > + * This function sets the bits_per_word for each transfer based on
> > +the spi
> > + * controller's bits_per_word_mask to improve the efficiency of spi
> transport.
> > + *
> > + * Return: 0 in case of success, a negative error code otherwise.
> > + */
> > +int spi_set_xfer_bpw(struct spi_controller *ctlr, struct spi_transfer
> > +*xfer) {
> > +	if (!ctlr || !xfer) {
> > +		dev_err(&ctlr->dev,
> > +			"Fail to set bits_per_word for spi transfer\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (ctlr->bits_per_word_mask) {
> > +		if (!(xfer->len % 4)) {
> > +			if (ctlr->bits_per_word_mask & SPI_BPW_MASK(32))
> > +				xfer->bits_per_word = 32;
> > +		} else if (!(xfer->len % 2)) {
> > +			if (ctlr->bits_per_word_mask & SPI_BPW_MASK(16))
> > +				xfer->bits_per_word = 16;
> > +		} else {
> > +			xfer->bits_per_word = 8;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(spi_set_xfer_bpw);
> 
> Why is this function placed in spi-mem.c, and more importantly, why is it
> exported?
Since bits_per_word is not judged by spi every time it is transmitted, the code defaults to bits_per_word=8 so that bits_per_word cannot be implemented if it wants to transfer a specific spi controller, so it is flexible to assign bits_per_word according to the spi controller's bits_per_word_mask before each transfer spi. Ah, for which export, there is really no need to deliberately remove.
> 
> > +
> >  /**
> >   * spi_mem_exec_op() - Execute a memory operation
> >   * @mem: the SPI memory
> > @@ -252,6 +287,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const
> struct spi_mem_op *op)
> >  	xfers[xferpos].tx_buf = tmpbuf;
> >  	xfers[xferpos].len = sizeof(op->cmd.opcode);
> >  	xfers[xferpos].tx_nbits = op->cmd.buswidth;
> > +	spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
> 
> It's still unclear why you need to specify a bits_per_word value, but if this is
> needed, it's probably something you want to add to spi.c, when a message is
> queued.
To specify a specific bits_per_word to be able to use the xspi (32bit) mode of the fsl_dspi module to transfer data, you can look at my PATCH 2/2.
 Do not add a value in spis.c that takes into account that the value assigned to bits_per_word is decided before the transfer. Thanks for your check and reply!
> 
> >  	spi_message_add_tail(&xfers[xferpos], &msg);
> >  	xferpos++;
> >  	totalxferlen++;
> > @@ -266,6 +302,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const
> struct spi_mem_op *op)
> >  		xfers[xferpos].tx_buf = tmpbuf + 1;
> >  		xfers[xferpos].len = op->addr.nbytes;
> >  		xfers[xferpos].tx_nbits = op->addr.buswidth;
> > +		spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
> >  		spi_message_add_tail(&xfers[xferpos], &msg);
> >  		xferpos++;
> >  		totalxferlen += op->addr.nbytes;
> > @@ -276,6 +313,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const
> struct spi_mem_op *op)
> >  		xfers[xferpos].tx_buf = tmpbuf + op->addr.nbytes + 1;
> >  		xfers[xferpos].len = op->dummy.nbytes;
> >  		xfers[xferpos].tx_nbits = op->dummy.buswidth;
> > +		spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
> >  		spi_message_add_tail(&xfers[xferpos], &msg);
> >  		xferpos++;
> >  		totalxferlen += op->dummy.nbytes;
> > @@ -291,6 +329,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const
> struct spi_mem_op *op)
> >  		}
> >
> >  		xfers[xferpos].len = op->data.nbytes;
> > +		spi_set_xfer_bpw(ctlr, &xfers[xferpos]);
> >  		spi_message_add_tail(&xfers[xferpos], &msg);
> >  		xferpos++;
> >  		totalxferlen += op->data.nbytes;

Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Boris Brezillon]

On Fri, 28 Sep 2018 06:59:58 +0000
Chuanhua Han <chuanhua.han@nxp.com> wrote:

> > 
> > It's still unclear why you need to specify a bits_per_word value,
> > but if this is needed, it's probably something you want to add to
> > spi.c, when a message is queued.  
> To specify a specific bits_per_word to be able to use the xspi
> (32bit) mode of the fsl_dspi module to transfer data, you can look at
> my PATCH 2/2. Do not add a value in spis.c that takes into account
> that the value assigned to bits_per_word is decided before the
> transfer. Thanks for your check and reply!

I might be wrong, but that's not my understanding of ->bits_per_word.
To me, it's something that you can use when your *device* (not
controller) expects non-byte aligned words [1]. The spi-mem protocol is
definitely designed to work with 1byte large words, so, as I said, I
suspect you're abusing xfer->bits_per_word to address a controller
driver issue.

[1]https://elixir.bootlin.com/linux/latest/source/include/linux/spi/spi.h#L114

RE: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Chuanhua Han]

> -----Original Message-----
> From: Boris Brezillon <boris.brezillon@bootlin.com>
> Sent: 2018年9月28日 15:19
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: broonie@kernel.org; linux-spi@vger.kernel.org;
> linux-kernel@vger.kernel.org; eha@deif.com
> Subject: Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function
> 
> On Fri, 28 Sep 2018 06:59:58 +0000
> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> 
> > >
> > > It's still unclear why you need to specify a bits_per_word value,
> > > but if this is needed, it's probably something you want to add to
> > > spi.c, when a message is queued.
> > To specify a specific bits_per_word to be able to use the xspi
> > (32bit) mode of the fsl_dspi module to transfer data, you can look at
> > my PATCH 2/2. Do not add a value in spis.c that takes into account
> > that the value assigned to bits_per_word is decided before the
> > transfer. Thanks for your check and reply!
> 
> I might be wrong, but that's not my understanding of ->bits_per_word.
> To me, it's something that you can use when your *device* (not
> controller) expects non-byte aligned words [1]. The spi-mem protocol is
> definitely designed to work with 1byte large words, so, as I said, I suspect
> you're abusing xfer->bits_per_word to address a controller driver issue.
> 
> [1]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Feli
> xir.bootlin.com%2Flinux%2Flatest%2Fsource%2Finclude%2Flinux%2Fspi%2Fspi
> .h%23L114&amp;data=02%7C01%7Cchuanhua.han%40nxp.com%7C7ec61d6d
> 7ef741aba84408d62512a9e1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C636737159420820050&amp;sdata=iwFgXG3yFvH7Ruac7MGCoaP8h
> l2M916m9%2BZeV7nksTg%3D&amp;reserved=0
Ah, I'll study it. Thank you！

Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Mark Brown]

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

On Fri, Sep 28, 2018 at 09:18:33AM +0200, Boris Brezillon wrote:

> I might be wrong, but that's not my understanding of ->bits_per_word.
> To me, it's something that you can use when your *device* (not
> controller) expects non-byte aligned words [1]. The spi-mem protocol is

Yes, bits per word is a device setting.  SPI devices can have control
protocols that have words larger than a byte with a fixed byte order,
many controllers do the required byte swapping.

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

RE: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Chuanhua Han]

> -----Original Message-----
> From: Boris Brezillon <boris.brezillon@bootlin.com>
> Sent: 2018年9月28日 15:19
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: broonie@kernel.org; linux-spi@vger.kernel.org;
> linux-kernel@vger.kernel.org; eha@deif.com
> Subject: Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function
> 
> On Fri, 28 Sep 2018 06:59:58 +0000
> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> 
> > >
> > > It's still unclear why you need to specify a bits_per_word value,
> > > but if this is needed, it's probably something you want to add to
> > > spi.c, when a message is queued.
> > To specify a specific bits_per_word to be able to use the xspi
> > (32bit) mode of the fsl_dspi module to transfer data, you can look at
> > my PATCH 2/2. Do not add a value in spis.c that takes into account
> > that the value assigned to bits_per_word is decided before the
> > transfer. Thanks for your check and reply!
> 
> I might be wrong, but that's not my understanding of ->bits_per_word.
> To me, it's something that you can use when your *device* (not
> controller) expects non-byte aligned words [1]. The spi-mem protocol is
> definitely designed to work with 1byte large words, so, as I said, I suspect
> you're abusing xfer->bits_per_word to address a controller driver issue.
> 
> [1]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Feli
> xir.bootlin.com%2Flinux%2Flatest%2Fsource%2Finclude%2Flinux%2Fspi%2Fspi
> .h%23L114&amp;data=02%7C01%7Cchuanhua.han%40nxp.com%7C7ec61d6d
> 7ef741aba84408d62512a9e1%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C636737159420820050&amp;sdata=iwFgXG3yFvH7Ruac7MGCoaP8h
> l2M916m9%2BZeV7nksTg%3D&amp;reserved=0
1. In the dspi driver (spi controller), bits_per_word (dspi->bits_per_word = transfer->bits_per_word) passed from the upper layer (spi-mem.c) is used. 
In this way, I can only assign the appropriate value of transfer->bits_per_word before passing to the controller, that is, the controller driver does not
know the value of bits_per_word, and it will use this value when the upper level sets what value is passed.
2. As I understand, bits_per_word does not exist for non-byte alignment, but for the need to reserve non-byte transmission mode that meets the controller.
3. In addition, now the XSPI of dspi cannot transfer data normally, so this problem needs to be solved. As for the DMA transfer mode, some colleagues will study it.

Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Boris Brezillon]

On Tue, 9 Oct 2018 09:52:23 +0000
Chuanhua Han <chuanhua.han@nxp.com> wrote:

> 1. In the dspi driver (spi controller), bits_per_word
> (dspi->bits_per_word = transfer->bits_per_word) passed from the upper
> layer (spi-mem.c) is used. In this way, I can only assign the
> appropriate value of transfer->bits_per_word before passing to the
> controller, that is, the controller driver does not know the value of
> bits_per_word, and it will use this value when the upper level sets
> what value is passed.

I think you're missing my point: ->bits_per_word is not what you're
looking for if what you're trying to do is use 32-bits accesses when
things are properly aligned.

> 2. As I understand, bits_per_word does not
> exist for non-byte alignment, but for the need to reserve non-byte
> transmission mode that meets the controller.

Exactly. It's an optimization you have to take care of inside your
driver. The core cannot help you with that.

> 3. In addition, now the
> XSPI of dspi cannot transfer data normally, so this problem needs to
> be solved.

I still don't understand what the problem is.

> As for the DMA transfer mode, some colleagues will study
> it.

RE: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Chuanhua Han]

> -----Original Message-----
> From: Boris Brezillon <boris.brezillon@bootlin.com>
> Sent: 2018年10月9日 18:05
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: broonie@kernel.org; linux-spi@vger.kernel.org;
> linux-kernel@vger.kernel.org; eha@deif.com
> Subject: Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function
> 
> On Tue, 9 Oct 2018 09:52:23 +0000
> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> 
> > 1. In the dspi driver (spi controller), bits_per_word
> > (dspi->bits_per_word = transfer->bits_per_word) passed from the upper
> > layer (spi-mem.c) is used. In this way, I can only assign the
> > appropriate value of transfer->bits_per_word before passing to the
> > controller, that is, the controller driver does not know the value of
> > bits_per_word, and it will use this value when the upper level sets
> > what value is passed.
> 
> I think you're missing my point: ->bits_per_word is not what you're looking for
> if what you're trying to do is use 32-bits accesses when things are properly
> aligned.
> 
In the dspi driver (spi controller driver), it is based on whether ->bits_per_word is 
larger than 16 to decide whether to use the XSPI mode (32bit) to transfer data.
If ->bits_per_word is not set and the default bits_per_word =8 is passed to the 
dspi driver, the XSPI mode (32bit) is not used for data transfer in the dspi driver
> > 2. As I understand, bits_per_word does not exist for non-byte
> > alignment, but for the need to reserve non-byte transmission mode that
> > meets the controller.
> 
> Exactly. It's an optimization you have to take care of inside your driver. The core
> cannot help you with that.
> 
The core layer is the upper layer. If you don't set ->bits_per_word, bits_per_word
 will use the default value of 8, so that the controller's specific mode for transferring
 data cannot be used (eg: XSPI mode).
> > 3. In addition, now the
> > XSPI of dspi cannot transfer data normally, so this problem needs to
> > be solved.
> 
> I still don't understand what the problem is.
> 
The problem is that I tested the XSPI mode and could not work, that is, the data could
not be transmitted normally. I used spi flash connected on dspi to conduct the test. 
In any case, the controller is independent of connected slave devices, and the data should
be transmitted by spi-flash devices and other spi devices.

> > As for the DMA transfer mode, some colleagues will study it.

Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Esben Haabendal]

Chuanhua Han <chuanhua.han@nxp.com> writes:

>> -----Original Message-----
>> From: Boris Brezillon <boris.brezillon@bootlin.com>
>> Sent: 2018年10月9日 18:05
>> To: Chuanhua Han <chuanhua.han@nxp.com>
>> Cc: broonie@kernel.org; linux-spi@vger.kernel.org;
>> linux-kernel@vger.kernel.org; eha@deif.com
>> Subject: Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function
>> 
>> On Tue, 9 Oct 2018 09:52:23 +0000
>> Chuanhua Han <chuanhua.han@nxp.com> wrote:
>> 
>> > 1. In the dspi driver (spi controller), bits_per_word
>> > (dspi->bits_per_word = transfer->bits_per_word) passed from the upper
>> > layer (spi-mem.c) is used. In this way, I can only assign the
>> > appropriate value of transfer->bits_per_word before passing to the
>> > controller, that is, the controller driver does not know the value of
>> > bits_per_word, and it will use this value when the upper level sets
>> > what value is passed.
>> 
>> I think you're missing my point: ->bits_per_word is not what you're looking
>> for
>> if what you're trying to do is use 32-bits accesses when things are properly
>> aligned.
>> 
> In the dspi driver (spi controller driver), it is based on whether
> ->bits_per_word is
> larger than 16 to decide whether to use the XSPI mode (32bit) to transfer data.

Not completely true.  XSPI mode is enabled, also for words smaller than
or equal to 16 bits.  But TX FIFO and CMD FIFO is written together, just
as for non-XSPI mode.

> If ->bits_per_word is not set and the default bits_per_word =8 is passed to the 
> dspi driver, the XSPI mode (32bit) is not used for data transfer in the dspi
> driver

Not true.  XSPI mode is unconditionally enabled in dspi_init().  But
XSPI mode does not overrule the value of transfer->bits_per_word.
The meaning of XSPI mode is the following:

1. Frame (word) size is max 32 bits (with normal SPI mode, max is 16
bits).
2. For frames (words) with more than 16 bits per frame (word), each
frame transfer results in 2 TX FIFO pop operations.
3. Command cycling is possible, enabled when SPI_CTARE[DTCP] > 1.

Command cycling is currently not implemented.  If implemented, it would
be possible to send multiple frames (words) by writing one time to CMD
FIFO and multiple times to TX FIFO.  This could possibly improve performance

Another possibility would be to use EOQ mode, if supported by the DSPI
controller in the CPU.  It allows for filling TX FIFO, and getting IRQ
only after last TX FIFO entry is sent.  This is also a likely
performance improvement.

>> > 2. As I understand, bits_per_word does not exist for non-byte
>> > alignment, but for the need to reserve non-byte transmission mode that
>> > meets the controller.
>> 
>> Exactly. It's an optimization you have to take care of inside your
>> driver. The core
>> cannot help you with that.
>> 
> The core layer is the upper layer. If you don't set ->bits_per_word,
> bits_per_word will use the default value of 8, so that the
> controller's specific mode for transferring data cannot be used (eg:
> XSPI mode).

XSPI mode is independent of bits_per_word.  In XSPI mode, you can send
frames as small as 4 bits, and up to 32 bits.  In normal SPI mode, you
can send frames from 4 to 16 bits.

>> > 3. In addition, now the
>> > XSPI of dspi cannot transfer data normally, so this problem needs to
>> > be solved.
>> 
>> I still don't understand what the problem is.
>> 
> The problem is that I tested the XSPI mode and could not work, that
> is, the data could not be transmitted normally.

What does "could not be transmitted normally" mean?

I am using XSPI mode on LS1021A, talking to a lot of different SPI
devices.  And they all work, and I believe everything is quite "normal".

/Esben

RE: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Chuanhua Han]

> -----Original Message-----
> From: Esben Haabendal <esbenhaabendal@gmail.com> On Behalf Of Esben
> Haabendal
> Sent: 2018年10月9日 19:21
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: Boris Brezillon <boris.brezillon@bootlin.com>; broonie@kernel.org;
> linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function
> 
> Chuanhua Han <chuanhua.han@nxp.com> writes:
> 
> >> -----Original Message-----
> >> From: Boris Brezillon <boris.brezillon@bootlin.com>
> >> Sent: 2018年10月9日 18:05
> >> To: Chuanhua Han <chuanhua.han@nxp.com>
> >> Cc: broonie@kernel.org; linux-spi@vger.kernel.org;
> >> linux-kernel@vger.kernel.org; eha@deif.com
> >> Subject: Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw
> >> function
> >>
> >> On Tue, 9 Oct 2018 09:52:23 +0000
> >> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> >>
> >> > 1. In the dspi driver (spi controller), bits_per_word
> >> > (dspi->bits_per_word = transfer->bits_per_word) passed from the
> >> > upper layer (spi-mem.c) is used. In this way, I can only assign the
> >> > appropriate value of transfer->bits_per_word before passing to the
> >> > controller, that is, the controller driver does not know the value
> >> > of bits_per_word, and it will use this value when the upper level
> >> > sets what value is passed.
> >>
> >> I think you're missing my point: ->bits_per_word is not what you're
> >> looking for if what you're trying to do is use 32-bits accesses when
> >> things are properly aligned.
> >>
> > In the dspi driver (spi controller driver), it is based on whether
> > ->bits_per_word is
> > larger than 16 to decide whether to use the XSPI mode (32bit) to transfer
> data.
> 
> Not completely true.  XSPI mode is enabled, also for words smaller than or
> equal to 16 bits.  But TX FIFO and CMD FIFO is written together, just as for
> non-XSPI mode.
> 
> > If ->bits_per_word is not set and the default bits_per_word =8 is
> > passed to the dspi driver, the XSPI mode (32bit) is not used for data
> > transfer in the dspi driver
> 
> Not true.  XSPI mode is unconditionally enabled in dspi_init().  But XSPI
> mode does not overrule the value of transfer->bits_per_word.
> The meaning of XSPI mode is the following:
> 
> 1. Frame (word) size is max 32 bits (with normal SPI mode, max is 16 bits).
> 2. For frames (words) with more than 16 bits per frame (word), each frame
> transfer results in 2 TX FIFO pop operations.
> 3. Command cycling is possible, enabled when SPI_CTARE[DTCP] > 1.
> 
> Command cycling is currently not implemented.  If implemented, it would be
> possible to send multiple frames (words) by writing one time to CMD FIFO and
> multiple times to TX FIFO.  This could possibly improve performance
> 
> Another possibility would be to use EOQ mode, if supported by the DSPI
> controller in the CPU.  It allows for filling TX FIFO, and getting IRQ only after
> last TX FIFO entry is sent.  This is also a likely performance improvement.
> 
> >> > 2. As I understand, bits_per_word does not exist for non-byte
> >> > alignment, but for the need to reserve non-byte transmission mode
> >> > that meets the controller.
> >>
> >> Exactly. It's an optimization you have to take care of inside your
> >> driver. The core cannot help you with that.
> >>
> > The core layer is the upper layer. If you don't set ->bits_per_word,
> > bits_per_word will use the default value of 8, so that the
> > controller's specific mode for transferring data cannot be used (eg:
> > XSPI mode).
> 
> XSPI mode is independent of bits_per_word.  In XSPI mode, you can send
> frames as small as 4 bits, and up to 32 bits.  In normal SPI mode, you can
> send frames from 4 to 16 bits.
> 
> >> > 3. In addition, now the
> >> > XSPI of dspi cannot transfer data normally, so this problem needs
> >> > to be solved.
> >>
> >> I still don't understand what the problem is.
> >>
> > The problem is that I tested the XSPI mode and could not work, that
> > is, the data could not be transmitted normally.
> 
> What does "could not be transmitted normally" mean?
> 
> I am using XSPI mode on LS1021A, talking to a lot of different SPI devices.
> And they all work, and I believe everything is quite "normal".
> 
Since I don't have the board of LS1021, I can't test it. I use other boards with DSPI (such as LS1043, LS2088, etc.), 
and I test sp-flash connected on DSPI. If XSPI mode is used at this time, data cannot be transmitted normally.
> /Esben

Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Esben Haabendal]

Chuanhua Han <chuanhua.han@nxp.com> writes:

>> I am using XSPI mode on LS1021A, talking to a lot of different SPI devices.
>> And they all work, and I believe everything is quite "normal".
>> 
> Since I don't have the board of LS1021, I can't test it. I use other boards
> with DSPI (such as LS1043, LS2088, etc.),
> and I test sp-flash connected on DSPI. If XSPI mode is used at this time, data
> cannot be transmitted normally.

Please describe what happens in much more detailed terms that "cannot be
transmitted normally".  It is simply impossible to assist you when you
report problems in such general terms.

/Esben

Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Mark Brown]

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

On Tue, Oct 09, 2018 at 12:05:22PM +0200, Boris Brezillon wrote:
> On Tue, 9 Oct 2018 09:52:23 +0000
> Chuanhua Han <chuanhua.han@nxp.com> wrote:

> > 1. In the dspi driver (spi controller), bits_per_word
> > (dspi->bits_per_word = transfer->bits_per_word) passed from the upper
> > layer (spi-mem.c) is used. In this way, I can only assign the
> > appropriate value of transfer->bits_per_word before passing to the
> > controller, that is, the controller driver does not know the value of
> > bits_per_word, and it will use this value when the upper level sets
> > what value is passed.

> I think you're missing my point: ->bits_per_word is not what you're
> looking for if what you're trying to do is use 32-bits accesses when
> things are properly aligned.

To be clear: bits_per_word affects what goes out on the SPI bus (4 byte
words swapped to be in MSB first order), it needn't have any effect on
on what goes on inside the SoC - many controllers fill their FIFO in 32
bit blocks even when sending 8 bit SPI words.

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

Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Esben Haabendal]

Mark Brown <broonie@kernel.org> writes:

> On Tue, Oct 09, 2018 at 12:05:22PM +0200, Boris Brezillon wrote:
>> On Tue, 9 Oct 2018 09:52:23 +0000
>> Chuanhua Han <chuanhua.han@nxp.com> wrote:
>
>> > 1. In the dspi driver (spi controller), bits_per_word
>> > (dspi->bits_per_word = transfer->bits_per_word) passed from the upper
>> > layer (spi-mem.c) is used. In this way, I can only assign the
>> > appropriate value of transfer->bits_per_word before passing to the
>> > controller, that is, the controller driver does not know the value of
>> > bits_per_word, and it will use this value when the upper level sets
>> > what value is passed.
>
>> I think you're missing my point: ->bits_per_word is not what you're
>> looking for if what you're trying to do is use 32-bits accesses when
>> things are properly aligned.
>
> To be clear: bits_per_word affects what goes out on the SPI bus (4 byte
> words swapped to be in MSB first order), it needn't have any effect on
> on what goes on inside the SoC - many controllers fill their FIFO in 32
> bit blocks even when sending 8 bit SPI words.

Exactly.

I believe behind all the confusion here, is the fact that the current
spi-fsl-dspi.c driver does not utilize all the possible features of the
DSPI controller.

1. DMA support requires a nasty workaround before it can be enabled for
   LS1021A.
2. EOQ mode is not enabled (and not tested) for some platforms which
   might actually support it.
3. Cyclic command transfer is not implemented.

So in order to improve performance on LS1021A, I propose to do work on
above issues.  I believe the performance improvement that can be gained
is likely to be biggest for 1 (DMA support), and smallest for 3 (cyclic
command transfer).

I don't have access to any coldfire boards, so I haven't tested EOQ mode
(it is only enabled for coldfire currently), so I cannot really know if
there are some bugs hiding there.

/Esben

Re: [PATCH 1/2] spi: spi-mem: Add the spi_set_xfer_bpw function

[Esben Haabendal]

Chuanhua Han <chuanhua.han@nxp.com> writes:

> 1. In the dspi driver (spi controller), bits_per_word (dspi->bits_per_word =
> transfer->bits_per_word) passed from the upper layer (spi-mem.c) is used.
> In this way, I can only assign the appropriate value of
> transfer->bits_per_word before passing to the controller, that is, the
> controller driver does not
> know the value of bits_per_word, and it will use this value when the upper
> level sets what value is passed.

Yes, the upper layer (spi-mem.c) should set ->bits_per_word according to
how the SPI data is to be transfered on the wire.  In this case (I
haven't looked at spi-mem.c myself), it sounds like the desired value is
8.  So it is set to 8, and that should definitely not be changed by dspi
driver.

> 2. As I understand, bits_per_word does not exist for non-byte alignment, but
> for the need to reserve non-byte transmission mode that meets the
> controller.

Where did you get that understanding from?  The bits_per_word value
defines the word size in bits for all transfers.

> 3. In addition, now the XSPI of dspi cannot transfer data normally, so this
> problem needs to be solved. As for the DMA transfer mode, some colleagues will
> study it.

What do you mean that "XSPI of dspi cannot tranffer data normally"?
What specific problems do you see (with unpatched mainline kernel)?

/Esben