[PATCH] spi: xilinx: Inhibit transmitter while filling TX FIFO

[PATCH] spi: xilinx: Inhibit transmitter while filling TX FIFO

[Jonathan Lemon]

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(-)

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;
 }
-- 
2.27.0

Re: [PATCH] spi: xilinx: Inhibit transmitter while filling TX FIFO

[Ricardo Ribalda Delgado]

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
?

If your hardware supports IRQ. can you try forcing use_irq to true?

>
> 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?
>

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.

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.

> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
> ---
>  drivers/spi/spi-xilinx.c | 54 +++++++++++++++++++++++++++++++---------
>  1 file changed, 42 insertions(+), 12 deletions(-)
>
> 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);
> +}
> +
Although these helper functions make very clear what you want to
achieve, they run a lot of extra register access.
In some platforms register access is VERY slow.
Please embed them into txrx_bufs (use the cr variable as before)

>  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*/
You shall remove this comment also
> -               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.
This comment is not valid anymore., the ISR does not refill the FIFO.
Can you send a following patch fixing this?
Thanks!
>                          */
> -                       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);
I believe this shall be moved to after:
remaining_words -= n_words;

and be something like:
if (!use_irq)
  xspi->write_fn(cr | XSPI_CR_TRANS_INHIBIT , xspi->regs + XSPI_CR_OFFSET);

>
>         return t->len;
>  }
> --
> 2.27.0
>

Re: [PATCH] spi: xilinx: Inhibit transmitter while filling TX FIFO

[Jonathan Lemon]

On 7 May 2021, at 17:46, Jonathan Lemon 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.
>
>
>> 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.

So I tried the following change:

-       if (xspi->irq >= 0 &&  remaining_words > xspi->buffer_size) {
+       if (xspi->irq >= 0) {

And that also allows writes to to through successfully.   Perhaps
this is a simpler change that would work?
-- 
Jonathan

Re: [PATCH] spi: xilinx: Inhibit transmitter while filling TX FIFO

[Ricardo Ribalda Delgado]

Hi Jonathan

On Sat, May 8, 2021 at 4:45 AM Jonathan Lemon <jonathan.lemon@gmail.com> wrote:
>
> On 7 May 2021, at 17:46, Jonathan Lemon 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.
> >
> >
> >> 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.
>
> So I tried the following change:
>
> -       if (xspi->irq >= 0 &&  remaining_words > xspi->buffer_size) {
> +       if (xspi->irq >= 0) {
>
> And that also allows writes to to through successfully.   Perhaps
> this is a simpler change that would work?

It was just a test... we do not want to disable the non-irq mode,
because for small operations it is much faster.

I think this probes that it might be a timming issue. I would really
like to see the output from your logic analyser/chipscope/

Thanks :)

> --
> Jonathan

Re: [PATCH] spi: xilinx: Inhibit transmitter while filling TX FIFO

[Ricardo Ribalda Delgado]

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