Recent changes to cdns_spi_irq introduced some issues.
Firstly, when writing the end of a longer transaction, the code in
cdns_spi_irq will write data into the TX FIFO, then immediately
fall into the if (!xspi->tx_bytes) path and attempt to read data
from the RX FIFO. However this required waiting for the TX FIFO to
empty before the RX data was ready.
Secondly, the variable trans_cnt is now rather inaccurately named
since in cases, where the watermark is set to 1, trans_cnt will be
1 but the count of bytes transferred would be much longer.
Finally, when setting up the transaction we set the watermark to 50%
of the FIFO if the transaction is great than 50% of the FIFO. However,
there is no need to split a tranaction that is smaller than the
whole FIFO, so anything up to the FIFO size can be done in a single
transaction.
Tidy up the code a little, to avoid repeatedly calling
cdns_spi_read_rx_fifo with a count of 1, and correct the three issues
noted above.
Fixes: b1b90514eaa3 ("spi: spi-cadence: Add support for Slave mode")
Signed-off-by: Charles Keepax <[email protected]>
---
drivers/spi/spi-cadence.c | 42 ++++++++++++++-------------------------
1 file changed, 15 insertions(+), 27 deletions(-)
diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c
index ac85d55622127..b0ccb138e3566 100644
--- a/drivers/spi/spi-cadence.c
+++ b/drivers/spi/spi-cadence.c
@@ -304,13 +304,11 @@ static int cdns_spi_setup_transfer(struct spi_device *spi,
* cdns_spi_fill_tx_fifo - Fills the TX FIFO with as many bytes as possible
* @xspi: Pointer to the cdns_spi structure
*/
-static void cdns_spi_fill_tx_fifo(struct cdns_spi *xspi)
+static void cdns_spi_fill_tx_fifo(struct cdns_spi *xspi, unsigned int avail)
{
unsigned long trans_cnt = 0;
- while ((trans_cnt < xspi->tx_fifo_depth) &&
- (xspi->tx_bytes > 0)) {
-
+ while ((trans_cnt < avail) && (xspi->tx_bytes > 0)) {
/* When xspi in busy condition, bytes may send failed,
* then spi control did't work thoroughly, add one byte delay
*/
@@ -381,33 +379,23 @@ static irqreturn_t cdns_spi_irq(int irq, void *dev_id)
spi_finalize_current_transfer(ctlr);
status = IRQ_HANDLED;
} else if (intr_status & CDNS_SPI_IXR_TXOW) {
- int trans_cnt = cdns_spi_read(xspi, CDNS_SPI_THLD);
+ int threshold = cdns_spi_read(xspi, CDNS_SPI_THLD);
+ int trans_cnt = xspi->rx_bytes - xspi->tx_bytes;
+
+ if (threshold > 1)
+ trans_cnt -= threshold;
+
/* Set threshold to one if number of pending are
* less than half fifo
*/
if (xspi->tx_bytes < xspi->tx_fifo_depth >> 1)
cdns_spi_write(xspi, CDNS_SPI_THLD, 1);
- while (trans_cnt) {
- cdns_spi_read_rx_fifo(xspi, 1);
-
- if (xspi->tx_bytes) {
- if (xspi->txbuf)
- cdns_spi_write(xspi, CDNS_SPI_TXD,
- *xspi->txbuf++);
- else
- cdns_spi_write(xspi, CDNS_SPI_TXD, 0);
- xspi->tx_bytes--;
- }
- trans_cnt--;
- }
- if (!xspi->tx_bytes) {
- /* Fixed delay due to controller limitation with
- * RX_NEMPTY incorrect status
- * Xilinx AR:65885 contains more details
- */
- udelay(10);
- cdns_spi_read_rx_fifo(xspi, xspi->rx_bytes);
+ cdns_spi_read_rx_fifo(xspi, trans_cnt);
+
+ if (xspi->tx_bytes) {
+ cdns_spi_fill_tx_fifo(xspi, trans_cnt);
+ } else {
cdns_spi_write(xspi, CDNS_SPI_IDR,
CDNS_SPI_IXR_DEFAULT);
spi_finalize_current_transfer(ctlr);
@@ -456,10 +444,10 @@ static int cdns_transfer_one(struct spi_controller *ctlr,
/* Set TX empty threshold to half of FIFO depth
* only if TX bytes are more than half FIFO depth.
*/
- if (xspi->tx_bytes > (xspi->tx_fifo_depth >> 1))
+ if (xspi->tx_bytes > xspi->tx_fifo_depth)
cdns_spi_write(xspi, CDNS_SPI_THLD, xspi->tx_fifo_depth >> 1);
- cdns_spi_fill_tx_fifo(xspi);
+ cdns_spi_fill_tx_fifo(xspi, xspi->tx_fifo_depth);
spi_transfer_delay_exec(transfer);
cdns_spi_write(xspi, CDNS_SPI_IER, CDNS_SPI_IXR_DEFAULT);
--
2.30.2
Commit b1b90514eaa3 ("spi: spi-cadence: Add support for Slave mode")
updated the code to trigger the IRQ when the FIFO was half empty,
overlapping filling more data into the FIFO and sending what is left.
This appears to cause regressions on the Zynq 7000, for transactions
longer than the FIFO size, below that no overlapping occurs.
It would appear from my testing that any attempt to put new data into
the FIFO whilst data is still transmitting causes data corruption
on both send and receive. If I am reading the commit message right
on commit 49530e641178 ("spi: cadence: Add usleep_range() for
cdns_spi_fill_tx_fifo()"), that would also seem to imply this is the
case.
On the assumption that this isn't an issue on the platform
the original slave mode support was added for, update the
cdns_transfer_one to only set the watermark to 50% of the FIFO size
when in slave mode. There by retaining the new behaviour for slave
mode but reverting to the older behaviour when the SPI is used a
master.
Fixes: b1b90514eaa3 ("spi: spi-cadence: Add support for Slave mode")
Signed-off-by: Charles Keepax <[email protected]>
---
drivers/spi/spi-cadence.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c
index b0ccb138e3566..ff02d81041319 100644
--- a/drivers/spi/spi-cadence.c
+++ b/drivers/spi/spi-cadence.c
@@ -438,14 +438,15 @@ static int cdns_transfer_one(struct spi_controller *ctlr,
xspi->tx_bytes = transfer->len;
xspi->rx_bytes = transfer->len;
- if (!spi_controller_is_slave(ctlr))
+ if (!spi_controller_is_slave(ctlr)) {
cdns_spi_setup_transfer(spi, transfer);
-
- /* Set TX empty threshold to half of FIFO depth
- * only if TX bytes are more than half FIFO depth.
- */
- if (xspi->tx_bytes > xspi->tx_fifo_depth)
- cdns_spi_write(xspi, CDNS_SPI_THLD, xspi->tx_fifo_depth >> 1);
+ } else {
+ /* Set TX empty threshold to half of FIFO depth
+ * only if TX bytes are more than half FIFO depth.
+ */
+ if (xspi->tx_bytes > xspi->tx_fifo_depth)
+ cdns_spi_write(xspi, CDNS_SPI_THLD, xspi->tx_fifo_depth >> 1);
+ }
cdns_spi_fill_tx_fifo(xspi, xspi->tx_fifo_depth);
spi_transfer_delay_exec(transfer);
--
2.30.2
On Tue, 09 May 2023 17:41:52 +0100, Charles Keepax wrote:
> Recent changes to cdns_spi_irq introduced some issues.
>
> Firstly, when writing the end of a longer transaction, the code in
> cdns_spi_irq will write data into the TX FIFO, then immediately
> fall into the if (!xspi->tx_bytes) path and attempt to read data
> from the RX FIFO. However this required waiting for the TX FIFO to
> empty before the RX data was ready.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
Thanks!
[1/2] spi: spi-cadence: Avoid read of RX FIFO before its ready
commit: a84c11e16dc2cc1faad2e688f8c12beeb369d80c
[2/2] spi: spi-cadence: Only overlap FIFO transactions in slave mode
commit: a0eb7be22c0f934d1fe7e1131f174ef5bc59d3f9
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
Hi,
>-----Original Message-----
>From: Charles Keepax <[email protected]>
>Sent: Tuesday, May 9, 2023 10:12 PM
>To: [email protected]
>Cc: Goud, Srinivas <[email protected]>; [email protected];
>[email protected]; [email protected]
>Subject: [PATCH 1/2] spi: spi-cadence: Avoid read of RX FIFO before its ready
>
>Recent changes to cdns_spi_irq introduced some issues.
>
>Firstly, when writing the end of a longer transaction, the code in cdns_spi_irq
>will write data into the TX FIFO, then immediately fall into the if (!xspi-
>>tx_bytes) path and attempt to read data from the RX FIFO. However this
>required waiting for the TX FIFO to empty before the RX data was ready.
>
>Secondly, the variable trans_cnt is now rather inaccurately named since in
>cases, where the watermark is set to 1, trans_cnt will be
>1 but the count of bytes transferred would be much longer.
>
>Finally, when setting up the transaction we set the watermark to 50% of the
>FIFO if the transaction is great than 50% of the FIFO. However, there is no need
>to split a tranaction that is smaller than the whole FIFO, so anything up to the
>FIFO size can be done in a single transaction.
>
>Tidy up the code a little, to avoid repeatedly calling cdns_spi_read_rx_fifo with
>a count of 1, and correct the three issues noted above.
>
>Fixes: b1b90514eaa3 ("spi: spi-cadence: Add support for Slave mode")
>Signed-off-by: Charles Keepax <[email protected]>
>---
> drivers/spi/spi-cadence.c | 42 ++++++++++++++-------------------------
> 1 file changed, 15 insertions(+), 27 deletions(-)
>
>diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c index
>ac85d55622127..b0ccb138e3566 100644
>--- a/drivers/spi/spi-cadence.c
>+++ b/drivers/spi/spi-cadence.c
>@@ -304,13 +304,11 @@ static int cdns_spi_setup_transfer(struct spi_device
>*spi,
> * cdns_spi_fill_tx_fifo - Fills the TX FIFO with as many bytes as possible
> * @xspi: Pointer to the cdns_spi structure
> */
>-static void cdns_spi_fill_tx_fifo(struct cdns_spi *xspi)
>+static void cdns_spi_fill_tx_fifo(struct cdns_spi *xspi, unsigned int
>+avail)
> {
> unsigned long trans_cnt = 0;
>
>- while ((trans_cnt < xspi->tx_fifo_depth) &&
>- (xspi->tx_bytes > 0)) {
>-
>+ while ((trans_cnt < avail) && (xspi->tx_bytes > 0)) {
> /* When xspi in busy condition, bytes may send failed,
> * then spi control did't work thoroughly, add one byte delay
> */
>@@ -381,33 +379,23 @@ static irqreturn_t cdns_spi_irq(int irq, void *dev_id)
> spi_finalize_current_transfer(ctlr);
> status = IRQ_HANDLED;
> } else if (intr_status & CDNS_SPI_IXR_TXOW) {
>- int trans_cnt = cdns_spi_read(xspi, CDNS_SPI_THLD);
>+ int threshold = cdns_spi_read(xspi, CDNS_SPI_THLD);
>+ int trans_cnt = xspi->rx_bytes - xspi->tx_bytes;
>+
>+ if (threshold > 1)
>+ trans_cnt -= threshold;
>+
> /* Set threshold to one if number of pending are
> * less than half fifo
> */
> if (xspi->tx_bytes < xspi->tx_fifo_depth >> 1)
> cdns_spi_write(xspi, CDNS_SPI_THLD, 1);
>
>- while (trans_cnt) {
>- cdns_spi_read_rx_fifo(xspi, 1);
>-
>- if (xspi->tx_bytes) {
>- if (xspi->txbuf)
>- cdns_spi_write(xspi, CDNS_SPI_TXD,
>- *xspi->txbuf++);
>- else
>- cdns_spi_write(xspi, CDNS_SPI_TXD,
>0);
>- xspi->tx_bytes--;
>- }
>- trans_cnt--;
>- }
>- if (!xspi->tx_bytes) {
>- /* Fixed delay due to controller limitation with
>- * RX_NEMPTY incorrect status
>- * Xilinx AR:65885 contains more details
>- */
>- udelay(10);
>- cdns_spi_read_rx_fifo(xspi, xspi->rx_bytes);
>+ cdns_spi_read_rx_fifo(xspi, trans_cnt);
Cadence SPI configured in Slave mode, when threshold is half of FIFO depth cdns_spi_read_rx_fifo() function continuously in read mode,
due to this we see incorrect data received on the Master side as Slave is failed to update the TX FIFO on time.
>+
>+ if (xspi->tx_bytes) {
>+ cdns_spi_fill_tx_fifo(xspi, trans_cnt);
>+ } else {
> cdns_spi_write(xspi, CDNS_SPI_IDR,
> CDNS_SPI_IXR_DEFAULT);
> spi_finalize_current_transfer(ctlr);
>@@ -456,10 +444,10 @@ static int cdns_transfer_one(struct spi_controller
>*ctlr,
> /* Set TX empty threshold to half of FIFO depth
> * only if TX bytes are more than half FIFO depth.
> */
>- if (xspi->tx_bytes > (xspi->tx_fifo_depth >> 1))
>+ if (xspi->tx_bytes > xspi->tx_fifo_depth)
> cdns_spi_write(xspi, CDNS_SPI_THLD, xspi->tx_fifo_depth >>
>1);
>
>- cdns_spi_fill_tx_fifo(xspi);
>+ cdns_spi_fill_tx_fifo(xspi, xspi->tx_fifo_depth);
> spi_transfer_delay_exec(transfer);
>
> cdns_spi_write(xspi, CDNS_SPI_IER, CDNS_SPI_IXR_DEFAULT);
>--
>2.30.2
Regards
Srinivas
On Mon, May 15, 2023 at 12:04:38PM +0000, Goud, Srinivas wrote:
> >-----Original Message-----
> >From: Charles Keepax <[email protected]>
> >Sent: Tuesday, May 9, 2023 10:12 PM
> >To: [email protected]
> >Cc: Goud, Srinivas <[email protected]>; [email protected];
> >[email protected]; [email protected]
> >Subject: [PATCH 1/2] spi: spi-cadence: Avoid read of RX FIFO before its ready
> >- while (trans_cnt) {
> >- cdns_spi_read_rx_fifo(xspi, 1);
> >-
> >- if (xspi->tx_bytes) {
> >- if (xspi->txbuf)
> >- cdns_spi_write(xspi, CDNS_SPI_TXD,
> >- *xspi->txbuf++);
> >- else
> >- cdns_spi_write(xspi, CDNS_SPI_TXD,
> >0);
> >- xspi->tx_bytes--;
> >- }
> >- trans_cnt--;
> >- }
> >- if (!xspi->tx_bytes) {
> >- /* Fixed delay due to controller limitation with
> >- * RX_NEMPTY incorrect status
> >- * Xilinx AR:65885 contains more details
> >- */
> >- udelay(10);
> >- cdns_spi_read_rx_fifo(xspi, xspi->rx_bytes);
> >+ cdns_spi_read_rx_fifo(xspi, trans_cnt);
> Cadence SPI configured in Slave mode, when threshold is half of FIFO depth cdns_spi_read_rx_fifo() function continuously in read mode,
> due to this we see incorrect data received on the Master side as Slave is failed to update the TX FIFO on time.
Apologies I am having a little trouble following this are you
saying this part of the patch cases issues for you running in
slave mode?
Thanks,
Charles
Hi,
>-----Original Message-----
>From: Charles Keepax <[email protected]>
>Sent: Monday, May 15, 2023 6:25 PM
>To: Goud, Srinivas <[email protected]>
>Cc: [email protected]; [email protected]; linux-
>[email protected]; [email protected]
>Subject: Re: [PATCH 1/2] spi: spi-cadence: Avoid read of RX FIFO before its
>ready
>
>On Mon, May 15, 2023 at 12:04:38PM +0000, Goud, Srinivas wrote:
>> >-----Original Message-----
>> >From: Charles Keepax <[email protected]>
>> >Sent: Tuesday, May 9, 2023 10:12 PM
>> >To: [email protected]
>> >Cc: Goud, Srinivas <[email protected]>;
>> >[email protected]; [email protected];
>> >[email protected]
>> >Subject: [PATCH 1/2] spi: spi-cadence: Avoid read of RX FIFO before its ready
>> >- while (trans_cnt) {
>> >- cdns_spi_read_rx_fifo(xspi, 1);
>> >-
>> >- if (xspi->tx_bytes) {
>> >- if (xspi->txbuf)
>> >- cdns_spi_write(xspi, CDNS_SPI_TXD,
>> >- *xspi->txbuf++);
>> >- else
>> >- cdns_spi_write(xspi, CDNS_SPI_TXD,
>> >0);
>> >- xspi->tx_bytes--;
>> >- }
>> >- trans_cnt--;
>> >- }
>> >- if (!xspi->tx_bytes) {
>> >- /* Fixed delay due to controller limitation with
>> >- * RX_NEMPTY incorrect status
>> >- * Xilinx AR:65885 contains more details
>> >- */
>> >- udelay(10);
>> >- cdns_spi_read_rx_fifo(xspi, xspi->rx_bytes);
>> >+ cdns_spi_read_rx_fifo(xspi, trans_cnt);
>> Cadence SPI configured in Slave mode, when threshold is half of FIFO
>> depth cdns_spi_read_rx_fifo() function continuously in read mode, due to
>this we see incorrect data received on the Master side as Slave is failed to
>update the TX FIFO on time.
>
>Apologies I am having a little trouble following this are you saying this part of
>the patch cases issues for you running in slave mode?
Yes, we see issue with this patch when we run in Slave mode.
When any master is in continuous read mode (anything > FIFO depth),
with updated logic cdns_spi_read_rx_fifo() function in cdns_spi_irq
continuously in read loop to read complete half FIFO data.
due to this Slave failed to write the TX FIFO on time and result in
incorrect data in Master receive.
Whereas in my previous patch, data read and write happening byte wise,
by which we are making sure data availability in TXFIFO on time.
>
>Thanks,
>Charles
Thanks,
Srinivas
On Wed, May 17, 2023 at 05:24:10AM +0000, Goud, Srinivas wrote:
> >On Mon, May 15, 2023 at 12:04:38PM +0000, Goud, Srinivas wrote:
> >> Cadence SPI configured in Slave mode, when threshold is half of FIFO
> >> depth cdns_spi_read_rx_fifo() function continuously in read mode, due to
> >this we see incorrect data received on the Master side as Slave is failed to
> >update the TX FIFO on time.
> >
> >Apologies I am having a little trouble following this are you saying this part of
> >the patch cases issues for you running in slave mode?
> Yes, we see issue with this patch when we run in Slave mode.
>
> When any master is in continuous read mode (anything > FIFO depth),
> with updated logic cdns_spi_read_rx_fifo() function in cdns_spi_irq
> continuously in read loop to read complete half FIFO data.
> due to this Slave failed to write the TX FIFO on time and result in
> incorrect data in Master receive.
> Whereas in my previous patch, data read and write happening byte wise,
> by which we are making sure data availability in TXFIFO on time.
That is a very tight system if reading 64 sequential memory locations
is the timing difference between success and failure, Linux is
not a real-time OS.
But I don't really mind moving back to a byte-wise operation. Although
we need to avoid the issues introduced by the first attempt at that. I
will have a look at doing a patch to put the byte-wise back in.
Thanks,
Charles