| Message ID | 20230509164153.3907694-1-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a84c11e16dc2cc1faad2e688f8c12beeb369d80c |
| Headers | show |
| Series | [1/2] spi: spi-cadence: Avoid read of RX FIFO before its ready | expand |
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 <ckeepax@opensource.cirrus.com> >Sent: Tuesday, May 9, 2023 10:12 PM >To: broonie@kernel.org >Cc: Goud, Srinivas <srinivas.goud@amd.com>; linux-spi@vger.kernel.org; >linux-kernel@vger.kernel.org; patches@opensource.cirrus.com >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 <ckeepax@opensource.cirrus.com> >--- > 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 <ckeepax@opensource.cirrus.com> > >Sent: Tuesday, May 9, 2023 10:12 PM > >To: broonie@kernel.org > >Cc: Goud, Srinivas <srinivas.goud@amd.com>; linux-spi@vger.kernel.org; > >linux-kernel@vger.kernel.org; patches@opensource.cirrus.com > >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 <ckeepax@opensource.cirrus.com> >Sent: Monday, May 15, 2023 6:25 PM >To: Goud, Srinivas <srinivas.goud@amd.com> >Cc: broonie@kernel.org; linux-spi@vger.kernel.org; linux- >kernel@vger.kernel.org; patches@opensource.cirrus.com >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 <ckeepax@opensource.cirrus.com> >> >Sent: Tuesday, May 9, 2023 10:12 PM >> >To: broonie@kernel.org >> >Cc: Goud, Srinivas <srinivas.goud@amd.com>; >> >linux-spi@vger.kernel.org; linux-kernel@vger.kernel.org; >> >patches@opensource.cirrus.com >> >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
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);
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 <ckeepax@opensource.cirrus.com> --- drivers/spi/spi-cadence.c | 42 ++++++++++++++------------------------- 1 file changed, 15 insertions(+), 27 deletions(-)