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

[Ricardo Ribalda Delgado <ribalda@xxxxxxxxxx>]

Hi Jonathan

On Sat, May 8, 2021 at 2:47 AM Jonathan Lemon <jonathan.lemon@xxxxxxxxx> 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@xxxxxxxxx> 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