[U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver

[Jagan Teki]

Hi Yen,

On 23 October 2015 at 04:18, Yen Lin <yelin at nvidia.com> wrote:
> Jagan,
>
>>  This is a port of the existing Tegra SPI driver (tegra114_spi.c). The Tegra 210
>>  QSPI controller is compatible with SPI, but had some quirks IIRC - Yen can
>>  comment on that, since he wrote this driver.
>>
> There are some minor differences between SPI and QSPI controllers.
>
>>  > > +       /* clear all error status bits */
>>  > > +       reg = readl(&regs->fifo_status);
>>  > > +       writel(reg, &regs->fifo_status);
>>  > > +
>>  > > +       /* flush RX/TX FIFOs */
>>  > > +       setbits_le32(&regs->fifo_status,
>>  > > +                    (QSPI_FIFO_STS_RX_FIFO_FLUSH |
>>  > > +                     QSPI_FIFO_STS_TX_FIFO_FLUSH));
>>  > > +       while ((readl(&regs->fifo_status) &
>>  > > +                     (QSPI_FIFO_STS_RX_FIFO_FLUSH |
>>  > > +                      QSPI_FIFO_STS_TX_FIFO_FLUSH)))
>>  > > +               ;
>>  >
>>  > May we can do this flush fifo check on claim_bus. if something goes in
>>  > while it may return there instead of xfer for each consecutive
>>  > transfers can lock. And also please use proper timeout check instead of
>>  while with semicolon.
>>  I can move the flush to claim_bus(), and add a true timeout in this while
>>  loop, if Yen approves. But this (and the other Tegra SPI drivers) have been
>>  architected this way and run fine for a long time, in Colibri and other
>>  production boards. I'm loathe to change it w/o thorough testing, which I'd
>>  need to find the BW for. Let's see what Yen says.
>>
> I prefer to put this here in spi_xfer().
> Caller will only call claim_bus() once, but it may call spi_xfer() several times. And, it is better to have a clean start for each xfer.

Make sense.

> But,  I agree with Jagan that having a timeout is safer.
>
>>  > > +               /* Need to stabilize other reg bit before GO bit set */
>>  > > +               udelay(2);
>>  > > +               setbits_le32(&regs->command1, QSPI_CMD1_GO);
>>  > > +               udelay(1);
>>  >
>>  > Can we do any timeout check's instead of these numerical udelay's.
>>  As I understand it, no, there's no bit that needs to be checked for status
>>  here, just waiting for I/O settle/register write propagation. These were
>>  added by Yen for the QSPI driver, I'll let him comment.
>>
> Our internal HW guide recommends:
>          * For successful operation at various freq combinations, min of 4-5
>          * spi_clk cycle delay might be required before enabling PIO or DMA bit.
>          * The worst case delay calculation can be done considering slowest
>          * qspi_clk as 1 MHz. based on that 1 us delay should be enough before
>          * enabling pio or dma.
> I padded another 1us.

So these udelay values are related to HW? that's what your saying is
it? if ie the case please add comments above on that saying why you
use only those specific numerical's on udelay.

thanks!
-- 
Jagan | openedev.