| Message ID | 20230518093927.711358-1-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6afe2ae8dc48e643cb9f52e86494b96942440bc6 |
| Headers | show |
| Series | [v2,1/2] spi: spi-cadence: Interleave write of TX and read of RX FIFO | expand |
On Thu, 18 May 2023 10:39:26 +0100, Charles Keepax wrote: > When working in slave mode it seems the timing is exceedingly tight. > The TX FIFO can never empty, because the master is driving the clock so > zeros would be sent for those bytes where the FIFO is empty. > > Return to interleaving the writing of the TX FIFO and the reading > of the RX FIFO to try to ensure the data is available when required. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/2] spi: spi-cadence: Interleave write of TX and read of RX FIFO commit: 6afe2ae8dc48e643cb9f52e86494b96942440bc6 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 Charles, >-----Original Message----- >From: Charles Keepax <ckeepax@opensource.cirrus.com> >Sent: Thursday, May 18, 2023 3:09 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 v2 1/2] spi: spi-cadence: Interleave write of TX and read of RX >FIFO > >When working in slave mode it seems the timing is exceedingly tight. >The TX FIFO can never empty, because the master is driving the clock so zeros >would be sent for those bytes where the FIFO is empty. > >Return to interleaving the writing of the TX FIFO and the reading of the RX FIFO >to try to ensure the data is available when required. > >Fixes: a84c11e16dc2 ("spi: spi-cadence: Avoid read of RX FIFO before its >ready") >Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> >--- > >Updates since v1: > - Update the kernel doc to match the changes > >Thanks, >Charles > > drivers/spi/spi-cadence.c | 64 ++++++++++++++++++--------------------- > 1 file changed, 30 insertions(+), 34 deletions(-) > >diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c index >ff02d81041319..26e6633693196 100644 >--- a/drivers/spi/spi-cadence.c >+++ b/drivers/spi/spi-cadence.c >@@ -12,6 +12,7 @@ > #include <linux/gpio/consumer.h> > #include <linux/interrupt.h> > #include <linux/io.h> >+#include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of_irq.h> > #include <linux/of_address.h> >@@ -301,47 +302,43 @@ 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 >+ * cdns_spi_process_fifo - Fills the TX FIFO, and drain the RX FIFO > * @xspi: Pointer to the cdns_spi structure >+ * @ntx: Number of bytes to pack into the TX FIFO >+ * @nrx: Number of bytes to drain from the RX FIFO > */ >-static void cdns_spi_fill_tx_fifo(struct cdns_spi *xspi, unsigned int avail) >+static void cdns_spi_process_fifo(struct cdns_spi *xspi, int ntx, int >+nrx) > { >- unsigned long trans_cnt = 0; >+ ntx = clamp(ntx, 0, xspi->tx_bytes); >+ nrx = clamp(nrx, 0, xspi->rx_bytes); > >- while ((trans_cnt < avail) && (xspi->tx_bytes > 0)) { >+ xspi->tx_bytes -= ntx; >+ xspi->rx_bytes -= nrx; >+ >+ while (ntx || nrx) { > /* When xspi in busy condition, bytes may send failed, > * then spi control did't work thoroughly, add one byte delay > */ >- if (cdns_spi_read(xspi, CDNS_SPI_ISR) & >- CDNS_SPI_IXR_TXFULL) >+ if (cdns_spi_read(xspi, CDNS_SPI_ISR) & >CDNS_SPI_IXR_TXFULL) > udelay(10); Linux driver configured as Slave, due to this above delay we see data corruption issue on Master side. when Master is continuously reading the data, Slave is failed to prepare the date on time due to above delay. > >- if (xspi->txbuf) >- cdns_spi_write(xspi, CDNS_SPI_TXD, *xspi->txbuf++); >- else >- cdns_spi_write(xspi, CDNS_SPI_TXD, 0); >+ if (ntx) { >+ 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++; >- } >-} >+ ntx--; >+ } > >-/** >- * cdns_spi_read_rx_fifo - Reads the RX FIFO with as many bytes as possible >- * @xspi: Pointer to the cdns_spi structure >- * @count: Read byte count >- */ >-static void cdns_spi_read_rx_fifo(struct cdns_spi *xspi, unsigned long count) -{ >- u8 data; >- >- /* Read out the data from the RX FIFO */ >- while (count > 0) { >- data = cdns_spi_read(xspi, CDNS_SPI_RXD); >- if (xspi->rxbuf) >- *xspi->rxbuf++ = data; >- xspi->rx_bytes--; >- count--; >+ if (nrx) { >+ u8 data = cdns_spi_read(xspi, CDNS_SPI_RXD); >+ >+ if (xspi->rxbuf) >+ *xspi->rxbuf++ = data; >+ >+ nrx--; >+ } > } > } > >@@ -391,11 +388,10 @@ static irqreturn_t cdns_spi_irq(int irq, void *dev_id) > if (xspi->tx_bytes < xspi->tx_fifo_depth >> 1) > cdns_spi_write(xspi, CDNS_SPI_THLD, 1); > >- cdns_spi_read_rx_fifo(xspi, trans_cnt); >- > if (xspi->tx_bytes) { >- cdns_spi_fill_tx_fifo(xspi, trans_cnt); >+ cdns_spi_process_fifo(xspi, trans_cnt, trans_cnt); > } else { >+ cdns_spi_process_fifo(xspi, 0, trans_cnt); When Linux driver configured as Slave, we observed data corruption issue with Slave mode read when data length is 400 bytes. As TX empty doesn't guaranties valid data in RX FIFO, hence we added one byte delay(10us) before RX FIFO read to overcome above issue. Created patch with above changes, find patch as attachment. Can you please test and let me know your observations. Thanks, Srinivas > cdns_spi_write(xspi, CDNS_SPI_IDR, > CDNS_SPI_IXR_DEFAULT); > spi_finalize_current_transfer(ctlr); >@@ -448,7 +444,7 @@ static int cdns_transfer_one(struct spi_controller *ctlr, > cdns_spi_write(xspi, CDNS_SPI_THLD, xspi- >>tx_fifo_depth >> 1); > } > >- cdns_spi_fill_tx_fifo(xspi, xspi->tx_fifo_depth); >+ cdns_spi_process_fifo(xspi, xspi->tx_fifo_depth, 0); > spi_transfer_delay_exec(transfer); > > cdns_spi_write(xspi, CDNS_SPI_IER, CDNS_SPI_IXR_DEFAULT); >-- >2.30.2
On Wed, Aug 09, 2023 at 11:31:24AM +0000, Goud, Srinivas wrote: > >+ while (ntx || nrx) { > > /* When xspi in busy condition, bytes may send failed, > > * then spi control did't work thoroughly, add one byte delay > > */ > >- if (cdns_spi_read(xspi, CDNS_SPI_ISR) & > >- CDNS_SPI_IXR_TXFULL) > >+ if (cdns_spi_read(xspi, CDNS_SPI_ISR) & > >CDNS_SPI_IXR_TXFULL) > > udelay(10); > Linux driver configured as Slave, due to this above delay we see data corruption issue on Master side. > when Master is continuously reading the data, Slave is failed to prepare the date on time due to above delay. > > >@@ -391,11 +388,10 @@ static irqreturn_t cdns_spi_irq(int irq, void *dev_id) > > if (xspi->tx_bytes < xspi->tx_fifo_depth >> 1) > > cdns_spi_write(xspi, CDNS_SPI_THLD, 1); > > > >- cdns_spi_read_rx_fifo(xspi, trans_cnt); > >- > > if (xspi->tx_bytes) { > >- cdns_spi_fill_tx_fifo(xspi, trans_cnt); > >+ cdns_spi_process_fifo(xspi, trans_cnt, trans_cnt); > > } else { > >+ cdns_spi_process_fifo(xspi, 0, trans_cnt); > When Linux driver configured as Slave, we observed data corruption issue with Slave mode read when data length is 400 bytes. > As TX empty doesn't guaranties valid data in RX FIFO, hence we added one byte delay(10us) before RX FIFO read to overcome above issue. > Created patch with above changes, find patch as attachment. > Can you please test and let me know your observations. > Yeah I can test the patch, I am on holiday this week so don't have access to the hardware, but will do so next week. > From 40154932ac7486c99e339bbc0b85b3cfe382286c Mon Sep 17 00:00:00 2001 > From: Srinivas Goud <srinivas.goud@amd.com> > Date: Tue, 1 Aug 2023 21:21:09 +0530 > Subject: [PATCH] spi: spi-cadence: Fix data corruption issues in slave mode > > Remove 10us delay in cdns_spi_process_fifo() (called from cdns_spi_irq()) > to fix data corruption issue on Master side when this driver > configured in Slave mode, as Slave is failed to prepare the date > on time due to above delay. > > Add 10us delay before processing the RX FIFO as TX empty doesn't > guaranty valid data in RX FIFO. guarantee > > Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> > --- > drivers/spi/spi-cadence.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c > index 42f101d..07a593c 100644 > --- a/drivers/spi/spi-cadence.c > +++ b/drivers/spi/spi-cadence.c > @@ -317,12 +317,6 @@ static void cdns_spi_process_fifo(struct cdns_spi *xspi, int ntx, int nrx) > xspi->rx_bytes -= nrx; > > while (ntx || nrx) { > - /* When xspi in busy condition, bytes may send failed, > - * then spi control did't work thoroughly, add one byte delay > - */ > - if (cdns_spi_read(xspi, CDNS_SPI_ISR) & CDNS_SPI_IXR_TXFULL) > - udelay(10); > - > if (ntx) { > if (xspi->txbuf) > cdns_spi_write(xspi, CDNS_SPI_TXD, *xspi->txbuf++); > @@ -392,6 +386,11 @@ static irqreturn_t cdns_spi_irq(int irq, void *dev_id) > if (xspi->tx_bytes) { > cdns_spi_process_fifo(xspi, trans_cnt, trans_cnt); > } else { > + /* Fixed delay due to controller limitation with > + * RX_NEMPTY incorrect status > + * Xilinx AR:65885 contains more details Do you have a web link to this ticket? Would be good to get some more background. > + */ > + udelay(10); > cdns_spi_process_fifo(xspi, 0, trans_cnt); > cdns_spi_write(xspi, CDNS_SPI_IDR, > CDNS_SPI_IXR_DEFAULT); > @@ -439,12 +438,18 @@ static int cdns_transfer_one(struct spi_controller *ctlr, > cdns_spi_setup_transfer(spi, transfer); > } else { > /* Set TX empty threshold to half of FIFO depth > - * only if TX bytes are more than half FIFO depth. > + * only if TX bytes are more than FIFO depth. > */ > if (xspi->tx_bytes > xspi->tx_fifo_depth) > cdns_spi_write(xspi, CDNS_SPI_THLD, xspi->tx_fifo_depth >> 1); > } > > + /* When xspi in busy condition, bytes may send failed, > + * then spi control did't work thoroughly, add one byte delay Just noticing there is an n missing in didn't might as well add that whilst moving the comment. > + */ > + if (cdns_spi_read(xspi, CDNS_SPI_ISR) & CDNS_SPI_IXR_TXFULL) > + udelay(10); > + > cdns_spi_process_fifo(xspi, xspi->tx_fifo_depth, 0); > spi_transfer_delay_exec(transfer); > > -- > 2.1.1 Thanks, Charles
Hi Charles, >-----Original Message----- >From: Charles Keepax <ckeepax@opensource.cirrus.com> >Sent: Thursday, August 10, 2023 3:40 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 v2 1/2] spi: spi-cadence: Interleave write of TX and read of >RX FIFO > >On Wed, Aug 09, 2023 at 11:31:24AM +0000, Goud, Srinivas wrote: >> >+ while (ntx || nrx) { >> > /* When xspi in busy condition, bytes may send failed, >> > * then spi control did't work thoroughly, add one byte delay >> > */ >> >- if (cdns_spi_read(xspi, CDNS_SPI_ISR) & >> >- CDNS_SPI_IXR_TXFULL) >> >+ if (cdns_spi_read(xspi, CDNS_SPI_ISR) & >> >CDNS_SPI_IXR_TXFULL) >> > udelay(10); >> Linux driver configured as Slave, due to this above delay we see data >corruption issue on Master side. >> when Master is continuously reading the data, Slave is failed to prepare the >date on time due to above delay. >> >> >@@ -391,11 +388,10 @@ static irqreturn_t cdns_spi_irq(int irq, void >*dev_id) >> > if (xspi->tx_bytes < xspi->tx_fifo_depth >> 1) >> > cdns_spi_write(xspi, CDNS_SPI_THLD, 1); >> > >> >- cdns_spi_read_rx_fifo(xspi, trans_cnt); >> >- >> > if (xspi->tx_bytes) { >> >- cdns_spi_fill_tx_fifo(xspi, trans_cnt); >> >+ cdns_spi_process_fifo(xspi, trans_cnt, trans_cnt); >> > } else { >> >+ cdns_spi_process_fifo(xspi, 0, trans_cnt); >> When Linux driver configured as Slave, we observed data corruption issue >with Slave mode read when data length is 400 bytes. >> As TX empty doesn't guaranties valid data in RX FIFO, hence we added one >byte delay(10us) before RX FIFO read to overcome above issue. >> Created patch with above changes, find patch as attachment. >> Can you please test and let me know your observations. >> > >Yeah I can test the patch, I am on holiday this week so don't have access to the >hardware, but will do so next week. > >> From 40154932ac7486c99e339bbc0b85b3cfe382286c Mon Sep 17 00:00:00 >2001 >> From: Srinivas Goud <srinivas.goud@amd.com> >> Date: Tue, 1 Aug 2023 21:21:09 +0530 >> Subject: [PATCH] spi: spi-cadence: Fix data corruption issues in slave >> mode >> >> Remove 10us delay in cdns_spi_process_fifo() (called from >> cdns_spi_irq()) to fix data corruption issue on Master side when this >> driver configured in Slave mode, as Slave is failed to prepare the >> date on time due to above delay. >> >> Add 10us delay before processing the RX FIFO as TX empty doesn't >> guaranty valid data in RX FIFO. > >guarantee > >> >> Signed-off-by: Srinivas Goud <srinivas.goud@amd.com> >> --- >> drivers/spi/spi-cadence.c | 19 ++++++++++++------- >> 1 file changed, 12 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c >> index 42f101d..07a593c 100644 >> --- a/drivers/spi/spi-cadence.c >> +++ b/drivers/spi/spi-cadence.c >> @@ -317,12 +317,6 @@ static void cdns_spi_process_fifo(struct cdns_spi >*xspi, int ntx, int nrx) >> xspi->rx_bytes -= nrx; >> >> while (ntx || nrx) { >> - /* When xspi in busy condition, bytes may send failed, >> - * then spi control did't work thoroughly, add one byte delay >> - */ >> - if (cdns_spi_read(xspi, CDNS_SPI_ISR) & >CDNS_SPI_IXR_TXFULL) >> - udelay(10); >> - >> if (ntx) { >> if (xspi->txbuf) >> cdns_spi_write(xspi, CDNS_SPI_TXD, *xspi- >>txbuf++); @@ -392,6 >> +386,11 @@ static irqreturn_t cdns_spi_irq(int irq, void *dev_id) >> if (xspi->tx_bytes) { >> cdns_spi_process_fifo(xspi, trans_cnt, trans_cnt); >> } else { >> + /* Fixed delay due to controller limitation with >> + * RX_NEMPTY incorrect status >> + * Xilinx AR:65885 contains more details > >Do you have a web link to this ticket? Would be good to get some more >background. Here AR link which contains the issue description https://support.xilinx.com/s/article/65885?language=en_US Thanks, Srinivas > >> + */ >> + udelay(10); >> cdns_spi_process_fifo(xspi, 0, trans_cnt); >> cdns_spi_write(xspi, CDNS_SPI_IDR, >> CDNS_SPI_IXR_DEFAULT); >> @@ -439,12 +438,18 @@ static int cdns_transfer_one(struct spi_controller >*ctlr, >> cdns_spi_setup_transfer(spi, transfer); >> } else { >> /* Set TX empty threshold to half of FIFO depth >> - * only if TX bytes are more than half FIFO depth. >> + * only if TX bytes are more than FIFO depth. >> */ >> if (xspi->tx_bytes > xspi->tx_fifo_depth) >> cdns_spi_write(xspi, CDNS_SPI_THLD, xspi- >>tx_fifo_depth >> 1); >> } >> >> + /* When xspi in busy condition, bytes may send failed, >> + * then spi control did't work thoroughly, add one byte delay > >Just noticing there is an n missing in didn't might as well add that whilst moving >the comment. > >> + */ >> + if (cdns_spi_read(xspi, CDNS_SPI_ISR) & CDNS_SPI_IXR_TXFULL) >> + udelay(10); >> + >> cdns_spi_process_fifo(xspi, xspi->tx_fifo_depth, 0); >> spi_transfer_delay_exec(transfer); >> >> -- >> 2.1.1 > >Thanks, >Charles
On Mon, Aug 21, 2023 at 07:26:57AM +0000, Goud, Srinivas wrote: > > > >Do you have a web link to this ticket? Would be good to get some more > >background. > Here AR link which contains the issue description > https://support.xilinx.com/s/article/65885?language=en_US > > Thanks, > Srinivas Apologies for the delay, the patch tests out fine for me. You probably want to send it properly as a patch rather than just an attachment. But when you do feel free to add my: Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com> Tested-by: Charles Keepax <ckeepax@opensource.cirrus.com> Thanks, Charles
diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c index ff02d81041319..26e6633693196 100644 --- a/drivers/spi/spi-cadence.c +++ b/drivers/spi/spi-cadence.c @@ -12,6 +12,7 @@ #include <linux/gpio/consumer.h> #include <linux/interrupt.h> #include <linux/io.h> +#include <linux/kernel.h> #include <linux/module.h> #include <linux/of_irq.h> #include <linux/of_address.h> @@ -301,47 +302,43 @@ 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 + * cdns_spi_process_fifo - Fills the TX FIFO, and drain the RX FIFO * @xspi: Pointer to the cdns_spi structure + * @ntx: Number of bytes to pack into the TX FIFO + * @nrx: Number of bytes to drain from the RX FIFO */ -static void cdns_spi_fill_tx_fifo(struct cdns_spi *xspi, unsigned int avail) +static void cdns_spi_process_fifo(struct cdns_spi *xspi, int ntx, int nrx) { - unsigned long trans_cnt = 0; + ntx = clamp(ntx, 0, xspi->tx_bytes); + nrx = clamp(nrx, 0, xspi->rx_bytes); - while ((trans_cnt < avail) && (xspi->tx_bytes > 0)) { + xspi->tx_bytes -= ntx; + xspi->rx_bytes -= nrx; + + while (ntx || nrx) { /* When xspi in busy condition, bytes may send failed, * then spi control did't work thoroughly, add one byte delay */ - if (cdns_spi_read(xspi, CDNS_SPI_ISR) & - CDNS_SPI_IXR_TXFULL) + if (cdns_spi_read(xspi, CDNS_SPI_ISR) & CDNS_SPI_IXR_TXFULL) udelay(10); - if (xspi->txbuf) - cdns_spi_write(xspi, CDNS_SPI_TXD, *xspi->txbuf++); - else - cdns_spi_write(xspi, CDNS_SPI_TXD, 0); + if (ntx) { + 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++; - } -} + ntx--; + } -/** - * cdns_spi_read_rx_fifo - Reads the RX FIFO with as many bytes as possible - * @xspi: Pointer to the cdns_spi structure - * @count: Read byte count - */ -static void cdns_spi_read_rx_fifo(struct cdns_spi *xspi, unsigned long count) -{ - u8 data; - - /* Read out the data from the RX FIFO */ - while (count > 0) { - data = cdns_spi_read(xspi, CDNS_SPI_RXD); - if (xspi->rxbuf) - *xspi->rxbuf++ = data; - xspi->rx_bytes--; - count--; + if (nrx) { + u8 data = cdns_spi_read(xspi, CDNS_SPI_RXD); + + if (xspi->rxbuf) + *xspi->rxbuf++ = data; + + nrx--; + } } } @@ -391,11 +388,10 @@ static irqreturn_t cdns_spi_irq(int irq, void *dev_id) if (xspi->tx_bytes < xspi->tx_fifo_depth >> 1) cdns_spi_write(xspi, CDNS_SPI_THLD, 1); - cdns_spi_read_rx_fifo(xspi, trans_cnt); - if (xspi->tx_bytes) { - cdns_spi_fill_tx_fifo(xspi, trans_cnt); + cdns_spi_process_fifo(xspi, trans_cnt, trans_cnt); } else { + cdns_spi_process_fifo(xspi, 0, trans_cnt); cdns_spi_write(xspi, CDNS_SPI_IDR, CDNS_SPI_IXR_DEFAULT); spi_finalize_current_transfer(ctlr); @@ -448,7 +444,7 @@ static int cdns_transfer_one(struct spi_controller *ctlr, cdns_spi_write(xspi, CDNS_SPI_THLD, xspi->tx_fifo_depth >> 1); } - cdns_spi_fill_tx_fifo(xspi, xspi->tx_fifo_depth); + cdns_spi_process_fifo(xspi, xspi->tx_fifo_depth, 0); spi_transfer_delay_exec(transfer); cdns_spi_write(xspi, CDNS_SPI_IER, CDNS_SPI_IXR_DEFAULT);
When working in slave mode it seems the timing is exceedingly tight. The TX FIFO can never empty, because the master is driving the clock so zeros would be sent for those bytes where the FIFO is empty. Return to interleaving the writing of the TX FIFO and the reading of the RX FIFO to try to ensure the data is available when required. Fixes: a84c11e16dc2 ("spi: spi-cadence: Avoid read of RX FIFO before its ready") Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com> --- Updates since v1: - Update the kernel doc to match the changes Thanks, Charles drivers/spi/spi-cadence.c | 64 ++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 34 deletions(-)