diff mbox series

spi: xilinx: Inhibit transmitter while filling TX FIFO

Message ID 20210507215319.3882138-1-jonathan.lemon@gmail.com
State New
Headers show
Series spi: xilinx: Inhibit transmitter while filling TX FIFO | expand

Commit Message

Jonathan Lemon May 7, 2021, 9:53 p.m. UTC 
Contrary to the comment in xilinx_spi_txrx_bufs(), the transmitter
was not disabled on entry.  On my particular board, when sending a PP
sequence, the first address byte was clocked out 3 times, writing data
into the wrong location, and generally locking up the chip.

Fix this by inhibiting the transmitter at initialization time, and
then enabling/disabling it appropriately.

With this patch, I can successfully read/erase/program the flash.

Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
---
 drivers/spi/spi-xilinx.c | 54 +++++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 12 deletions(-)

Comments

Ricardo Ribalda Delgado May 8, 2021, 10:01 a.m. UTC | #1 
Hi Jonathan

On Sat, May 8, 2021 at 2:47 AM Jonathan Lemon <jonathan.lemon@gmail.com> wrote:
>
> On 7 May 2021, at 16:02, Ricardo Ribalda Delgado wrote:
>
> Hi Jonathan
>
> Thanks for your patch.
>
> On Fri, May 7, 2021 at 11:53 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote:
>
> Contrary to the comment in xilinx_spi_txrx_bufs(), the transmitter
> was not disabled on entry. On my particular board, when sending a PP
> sequence, the first address byte was clocked out 3 times, writing data
> into the wrong location, and generally locking up the chip.
>
> Sorry, what do you mean by PP sequence?
>
> By clocked out 3 times you mean that the sequence is sent like
> B0........B1.........B2
> instead of:
> B0B1B2
>
> PP: Page program. When I’m trying to write to the flash, the sequence
> [opcode 02][A1 A2 A3][D1 D2 ..] is sent. The result is a flash write
> at location [A1 A1 A1] = [A2 A3 D1 D2 ...]
>
> In other words, the first byte of the address appears to have been
> sent to the chip 3x.

Any chance you could connect chipscope to the spi pins? Or a real
logic analyser?
>
> If your hardware supports IRQ. can you try forcing use_irq to true?
>
> Will try this in a bit. The hw does have an irq registered, but it
> it isn't always set, as it depends on how many how many bytes the
> spi_transfer sets. So sometimes it will set use_irq, and sometimes not.
>
> Fix this by inhibiting the transmitter at initialization time, and
> then enabling/disabling it appropriately.
>
> With this patch, I can successfully read/erase/program the flash.
>
> Can you provide a bit more details about your setup? What core version
> are you using? C_PARAMS? Flash?
>
> Flash: https://media-www.micron.com/-/media/client/global/documents/products/data-sheet/nor-flash/serial-nor/n25q/n25q_128mb_3v_65nm.pdf
>
> xlinx-spi: https://www.xilinx.com/support/documentation/ip_documentation/axi_quad_spi/v3_2/pg153-axi-quad-spi.pdf

What are your C_PARAMS?

>
> In general, I think it makes more sense to title your patch as:
> Inhibit transfer while idle. Because the current code already inhibits
> before sending data in irq mode.
>
> But irq mode is entered/exited based on t->len:
>
>     if (xspi->irq >= 0 &&  remaining_words > xspi->buffer_size) {
>
> So sometimes it will set use_irq, and sometimes not.
>
> The current design tries to limit as much as possible the register
> access and only enable inhibit in irq mode.
>
> In principle, I believe both approaches shall be equally valid, but if
> you have a use case that does not work with the current approach your
> patch is very welcome.
>
> My use case is I have a FPGA with a flash chip on a PCI board, my goal
> is to reflash the firmware through the PCI driver. The existing code
> works for flash reads, but for whatever reason, fails miserably when
> sending the 'page program' write sequence without this patch.

I have been using this for a couple of years now :S




> --
> Jonathan
diff mbox series

Patch

diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c
index 523edfdf5dcd..10eccfb09e20 100644
--- a/drivers/spi/spi-xilinx.c
+++ b/drivers/spi/spi-xilinx.c
@@ -179,8 +179,8 @@  static void xspi_init_hw(struct xilinx_spi *xspi)
 	/* Disable the transmitter, enable Manual Slave Select Assertion,
 	 * put SPI controller into master mode, and enable it */
 	xspi->write_fn(XSPI_CR_MANUAL_SSELECT |	XSPI_CR_MASTER_MODE |
-		XSPI_CR_ENABLE | XSPI_CR_TXFIFO_RESET |	XSPI_CR_RXFIFO_RESET,
-		regs_base + XSPI_CR_OFFSET);
+		XSPI_CR_ENABLE | XSPI_CR_TXFIFO_RESET |	XSPI_CR_RXFIFO_RESET |
+		XSPI_CR_TRANS_INHIBIT, regs_base + XSPI_CR_OFFSET);
 }
 
 static void xilinx_spi_chipselect(struct spi_device *spi, int is_on)
@@ -235,14 +235,46 @@  static int xilinx_spi_setup_transfer(struct spi_device *spi,
 	return 0;
 }
 
+static void
+xilinx_spi_inhibit_master(struct xilinx_spi *xspi, bool inhibit)
+{
+	u16 cr;
+
+	cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET);
+	if (inhibit)
+		cr |= XSPI_CR_TRANS_INHIBIT;
+	else
+		cr &= ~XSPI_CR_TRANS_INHIBIT;
+	xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
+}
+
+static void
+xilinx_spi_enable_transfer(struct xilinx_spi *xspi)
+{
+	xilinx_spi_inhibit_master(xspi, false);
+}
+
+static void
+xilinx_spi_disable_transfer(struct xilinx_spi *xspi)
+{
+	xilinx_spi_inhibit_master(xspi, true);
+}
+
+static bool
+xilinx_spi_is_transfer_disabled(struct xilinx_spi *xspi)
+{
+	u16 cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET);
+	return !!(cr & XSPI_CR_TRANS_INHIBIT);
+}
+
 static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 {
 	struct xilinx_spi *xspi = spi_master_get_devdata(spi->master);
 	int remaining_words;	/* the number of words left to transfer */
 	bool use_irq = false;
-	u16 cr = 0;
 
 	/* We get here with transmitter inhibited */
+	WARN_ON_ONCE(!xilinx_spi_is_transfer_disabled(xspi));
 
 	xspi->tx_ptr = t->tx_buf;
 	xspi->rx_ptr = t->rx_buf;
@@ -252,14 +284,13 @@  static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 		u32 isr;
 		use_irq = true;
 		/* Inhibit irq to avoid spurious irqs on tx_empty*/
-		cr = xspi->read_fn(xspi->regs + XSPI_CR_OFFSET);
-		xspi->write_fn(cr | XSPI_CR_TRANS_INHIBIT,
-			       xspi->regs + XSPI_CR_OFFSET);
+
 		/* ACK old irqs (if any) */
 		isr = xspi->read_fn(xspi->regs + XIPIF_V123B_IISR_OFFSET);
 		if (isr)
 			xspi->write_fn(isr,
 				       xspi->regs + XIPIF_V123B_IISR_OFFSET);
+
 		/* Enable the global IPIF interrupt */
 		xspi->write_fn(XIPIF_V123B_GINTR_ENABLE,
 				xspi->regs + XIPIF_V123B_DGIER_OFFSET);
@@ -280,9 +311,9 @@  static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 		/* Start the transfer by not inhibiting the transmitter any
 		 * longer
 		 */
+		xilinx_spi_enable_transfer(xspi);
 
 		if (use_irq) {
-			xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
 			wait_for_completion(&xspi->done);
 			/* A transmit has just completed. Process received data
 			 * and check for more data to transmit. Always inhibit
@@ -290,8 +321,7 @@  static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 			 * register/FIFO, or make sure it is stopped if we're
 			 * done.
 			 */
-			xspi->write_fn(cr | XSPI_CR_TRANS_INHIBIT,
-				       xspi->regs + XSPI_CR_OFFSET);
+			xilinx_spi_disable_transfer(xspi);
 			sr = XSPI_SR_TX_EMPTY_MASK;
 		} else
 			sr = xspi->read_fn(xspi->regs + XSPI_SR_OFFSET);
@@ -325,10 +355,10 @@  static int xilinx_spi_txrx_bufs(struct spi_device *spi, struct spi_transfer *t)
 		remaining_words -= n_words;
 	}
 
-	if (use_irq) {
+	if (use_irq)
 		xspi->write_fn(0, xspi->regs + XIPIF_V123B_DGIER_OFFSET);
-		xspi->write_fn(cr, xspi->regs + XSPI_CR_OFFSET);
-	}
+
+	xilinx_spi_disable_transfer(xspi);
 
 	return t->len;
 }