| Message ID | 20210507215319.3882138-1-jonathan.lemon@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: xilinx: Inhibit transmitter while filling TX FIFO | expand |
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 >
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?
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
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
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; }
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(-)