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

[Tom Warren]

Jagan,

> -----Original Message-----
> From: Jagan Teki [mailto:jteki at openedev.com]
> Sent: Saturday, October 24, 2015 8:15 PM
> To: Yen Lin <yelin at nvidia.com>
> Cc: Tom Warren <TWarren at nvidia.com>; u-boot at lists.denx.de; Stephen
> Warren <swarren at nvidia.com>; Tom Warren <tomcwarren3959 at gmail.com>
> Subject: Re: [U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver
> 
> 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.
Done in V5 of this patch, sent to the list just now. Thanks.

Tom
--
nvpublic
> 
> thanks!
> --
> Jagan | openedev.