Jagan, > -----Original Message----- > From: Jagan Teki [mailto:jt...@openedev.com] > Sent: Saturday, October 24, 2015 8:15 PM > To: Yen Lin <ye...@nvidia.com> > Cc: Tom Warren <twar...@nvidia.com>; u-boot@lists.denx.de; Stephen > Warren <swar...@nvidia.com>; Tom Warren <tomcwarren3...@gmail.com> > Subject: Re: [U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver > > Hi Yen, > > On 23 October 2015 at 04:18, Yen Lin <ye...@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(®s->fifo_status); > >> > > + writel(reg, ®s->fifo_status); > >> > > + > >> > > + /* flush RX/TX FIFOs */ > >> > > + setbits_le32(®s->fifo_status, > >> > > + (QSPI_FIFO_STS_RX_FIFO_FLUSH | > >> > > + QSPI_FIFO_STS_TX_FIFO_FLUSH)); > >> > > + while ((readl(®s->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(®s->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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
