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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

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
?

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@xxxxxxxxx>
> ---
>  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
>
[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux