[U-Boot] [PATCH v2 8/9] tegra: add SPI SLINK driver

[Allen Martin <amartin@nvidia.com>]

On Sat, Jan 12, 2013 at 08:56:23AM -0800, Simon Glass wrote:
> Hi,
> 
> On Sat, Jan 12, 2013 at 1:07 AM, Allen Martin <amartin@nvidia.com> wrote:
> > Add driver for tegra SPI "SLINK" style driver.  This controller is
> > similar to the tegra20 SPI "SFLASH" controller.  The difference is
> > that the SLINK controller is a genernal purpose SPI controller and the
> > SFLASH controller is special purpose and can only talk to FLASH
> > devices.  In addition there are potentially many instances of an SLINK
> > controller on tegra and only a single instance of SFLASH.  Tegra20 is
> > currently ths only version of tegra that instantiates an SFLASH
> > controller.
> >
> > This driver supports basic PIO mode of operation and is configurable
> > (CONFIG_OF_CONTROL) to be driven off devicetree bindings.  Up to 4
> > devices per controller may be attached, although typically only a
> > single chip select line is exposed from tegra per controller so in
> > reality this is usually limited to 1.
> >
> > To enable this driver, use CONFIG_TEGRA_SLINK
> 
> A few comments - note I am on holiday next week so please don't wait
> for my response on the next version.
> 
> > ...
> > +#include <spi.h>
> > +#ifdef CONFIG_OF_CONTROL
> 
> You probably don't need this ifdef
> 

ok

> > +
> > +       spi = malloc(sizeof(struct tegra_spi_slave));
> 
> Please also look at my SPI series where I added an allocate function for this.
> 
> http://patchwork.ozlabs.org/patch/208226/
> http://patchwork.ozlabs.org/patch/208229/
> 

Nice, thanks.  I propose I should wait for that to land in
u-boot/master and trick back down to u-boot-arm and u-boot-tegra, and
then add it in a separate patch.  That way I don't have to add a
cross repo dependency.



> > +{
> > +       int node = 0, i;
> > +       struct tegra_spi_ctrl *ctrl;
> 
> blank line here

ok

> 
> > +       for (i = 0; i < CONFIG_TEGRA_SLINK_CTRLS; i++) {
> > +               ctrl = &spi_ctrls[i];
> > +#ifdef CONFIG_OF_CONTROL
> > +               node = fdtdec_next_compatible(gd->fdt_blob, node,
> > +                                             COMPAT_NVIDIA_TEGRA20_SLINK);
> > +               if (!node)
> > +                       break;
> 
> I think you should be using fdtdec_find_aliases_for_id() so that aliases work.

I'll reply in Stephen's follow-up on this.


> > +               (slave->cs << SLINK_CMD2_SS_EN_SHIFT);
> > +       writel(reg, &regs->command2);
> 
> Could use clrsetbits_le32() if you like

Ok


> > +               bytes = (num_bytes > 4) ?  4 : num_bytes;
> > +
> > +               if (dout != NULL) {
> > +                       for (i = 0; i < bytes; ++i)
> > +                               tmpdout = (tmpdout << 8) | dout[i];
> 
> dout += bytes here...
> 
> > +               }
> > +
> > +               num_bytes -= bytes;
> > +               if (dout)
> > +                       dout += bytes;
> 
> instead of here?

ok

> 
> > +
> > +               clrsetbits_le32(&regs->command, SLINK_CMD_BIT_LENGTH_MASK,
> > +                               bytes * 8 - 1);
> > +               writel(tmpdout, &regs->tx_fifo);
> > +               setbits_le32(&regs->command, SLINK_CMD_GO);
> > +
> > +               /*
> > +                * Wait for SPI transmit FIFO to empty, or to time out.
> > +                * The RX FIFO status will be read and cleared last
> > +                */
> > +               for (tm = 0, is_read = 0; tm < SPI_TIMEOUT; ++tm) {
> > +                       u32 status;
> > +
> 
> This says timeout but doesn't seem to actually check get_timer(). Also
> is it possible to separate the code that waits for completion from the
> code below?

You're right about get_timer(), I'll fix that.

I pulled this loop directly from the tegra20 tegra_spi driver, so I'm
not the original author, but I believe the reason it's written this
way is so there's a single timeout loop around the wait for STAT_BSY
to drop, RXF_EMPTY to drop, and TXF_EMPTY to go high.  It *should* be
ok to move the TXF_EMPTY wait to a separate wait loop, but I'm a
little hesitant to touch the code since it's beeen well tested in the
tegra20 driver, and this part of the driver is identical.

> 
> > +                       status = readl(&regs->status);
> > +
> > +                       /* We can exit when we've had both RX and TX activity */
> > +                       if (is_read && (status & SLINK_STAT_TXF_EMPTY))
> > +                               break;
> > +
> > +                       if ((status & (SLINK_STAT_BSY | SLINK_STAT_RDY)) !=
> > +                                       SLINK_STAT_RDY)
> > +                               tm++;
> > +
> > +                       else if (!(status & SLINK_STAT_RXF_EMPTY)) {
> > +                               tmpdin = readl(&regs->rx_fifo);
> > +                               is_read = 1;
> > +
> > +                               /* swap bytes read in */
> > +                               if (din != NULL) {
> > +                                       for (i = bytes - 1; i >= 0; --i) {
> > +                                               din[i] = tmpdin & 0xff;
> > +                                               tmpdin >>= 8;
> > +                                       }
> > +                                       din += bytes;
> > +                               }
> > +                       }
> > +               }
> > +
> > +               if (tm >= SPI_TIMEOUT)
> > +                       ret = tm;
> > +
> > +               /* clear ACK RDY, etc. bits */
> > +               writel(readl(&regs->status), &regs->status);
> > +       }
> > +
> > +       if (flags & SPI_XFER_END)
> > +               spi_cs_deactivate(slave);
> > +
> > +       debug("%s: transfer ended. Value=%08x, status = %08x\n",
> > +             __func__, tmpdin, readl(&regs->status));
> > +
> > +       if (ret) {
> > +               printf("%s: timeout during SPI transfer, tm %d\n",
> > +                      __func__, ret);
> > +               return -1;
> > +       }
> > +
> > +       return 0;
> > +}
> > diff --git a/include/fdtdec.h b/include/fdtdec.h
> > index 1504336..14aa308 100644
> > --- a/include/fdtdec.h
> > +++ b/include/fdtdec.h
> > @@ -71,6 +71,7 @@ enum fdt_compat_id {
> >         COMPAT_NVIDIA_TEGRA20_PWM,      /* Tegra 2 PWM controller */
> >         COMPAT_NVIDIA_TEGRA20_DC,       /* Tegra 2 Display controller */
> >         COMPAT_NVIDIA_TEGRA20_SFLASH,   /* Tegra 2 SPI flash controller */
> > +       COMPAT_NVIDIA_TEGRA20_SLINK,    /* Tegra 2 SPI SLINK controller */
> >
> >         COMPAT_COUNT,
> >  };
> > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > index 6779278..4fef428 100644
> > --- a/lib/fdtdec.c
> > +++ b/lib/fdtdec.c
> > @@ -46,6 +46,7 @@ static const char * const compat_names[COMPAT_COUNT] = {
> >         COMPAT(NVIDIA_TEGRA20_PWM, "nvidia,tegra20-pwm"),
> >         COMPAT(NVIDIA_TEGRA20_DC, "nvidia,tegra20-dc"),
> >         COMPAT(NVIDIA_TEGRA20_SFLASH, "nvidia,tegra20-sflash"),
> > +       COMPAT(NVIDIA_TEGRA20_SLINK, "nvidia,tegra20-slink"),
> >  };
> >
> >  const char *fdtdec_get_compatible(enum fdt_compat_id id)
> > --
> > 1.7.10.4
> >
> 
> Regards,
> Simon

-Allen
-- 
nvpublic