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

Simon Glass Tue, 22 Jan 2013 05:57:17 -0800

Hi Allen,

On Mon, Jan 14, 2013 at 8:27 PM, Allen Martin <amar...@nvidia.com> wrote:
> 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 <amar...@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.


Yes, sounds good.

>
>
>
>> > +{
>> > +       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.

OK, well it's up to you - just a suggestion and what you have works.

>
>>
>> > +                       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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

Reply via email to