diff mbox

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

Message ID 1357981631-21245-9-git-send-email-amartin@nvidia.com
State Superseded
Delegated to: Tom Warren
Headers show

Commit Message

Allen Martin Jan. 12, 2013, 9:07 a.m. UTC
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

Signed-off-by: Allen Martin <amartin@nvidia.com>
---
 arch/arm/include/asm/arch-tegra/tegra_slink.h |   84 +++++++
 board/nvidia/common/board.c                   |    3 +-
 drivers/spi/Makefile                          |    1 +
 drivers/spi/tegra_slink.c                     |  335 +++++++++++++++++++++++++
 include/fdtdec.h                              |    1 +
 lib/fdtdec.c                                  |    1 +
 6 files changed, 424 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/include/asm/arch-tegra/tegra_slink.h
 create mode 100644 drivers/spi/tegra_slink.c

Comments

Simon Glass Jan. 12, 2013, 4:56 p.m. UTC | #1
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.

>
> Signed-off-by: Allen Martin <amartin@nvidia.com>
> ---
>  arch/arm/include/asm/arch-tegra/tegra_slink.h |   84 +++++++
>  board/nvidia/common/board.c                   |    3 +-
>  drivers/spi/Makefile                          |    1 +
>  drivers/spi/tegra_slink.c                     |  335 +++++++++++++++++++++++++
>  include/fdtdec.h                              |    1 +
>  lib/fdtdec.c                                  |    1 +
>  6 files changed, 424 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/include/asm/arch-tegra/tegra_slink.h
>  create mode 100644 drivers/spi/tegra_slink.c
>
> diff --git a/arch/arm/include/asm/arch-tegra/tegra_slink.h b/arch/arm/include/asm/arch-tegra/tegra_slink.h
> new file mode 100644
> index 0000000..74804b5
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-tegra/tegra_slink.h
> @@ -0,0 +1,84 @@
> +/*
> + * NVIDIA Tegra SPI-SLINK controller
> + *
> + * Copyright 2010-2013 NVIDIA Corporation
> + *
> + * This software may be used and distributed according to the
> + * terms of the GNU Public License, Version 2, incorporated
> + * herein by reference.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * Version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#ifndef _TEGRA_SLINK_H_
> +#define _TEGRA_SLINK_H_
> +
> +#include <asm/types.h>
> +
> +struct slink_tegra {
> +       u32 command;    /* SLINK_COMMAND_0 register  */
> +       u32 command2;   /* SLINK_COMMAND2_0 reg */
> +       u32 status;     /* SLINK_STATUS_0 register */
> +       u32 reserved;   /* Reserved offset 0C */
> +       u32 mas_data;   /* SLINK_MAS_DATA_0 reg */
> +       u32 slav_data;  /* SLINK_SLAVE_DATA_0 reg */
> +       u32 dma_ctl;    /* SLINK_DMA_CTL_0 register */
> +       u32 status2;    /* SLINK_STATUS2_0 reg */
> +       u32 rsvd[56];   /* 0x20 to 0xFF reserved */
> +       u32 tx_fifo;    /* SLINK_TX_FIFO_0 reg off 100h */
> +       u32 rsvd2[31];  /* 0x104 to 0x17F reserved */
> +       u32 rx_fifo;    /* SLINK_RX_FIFO_0 reg off 180h */
> +};
> +
> +/* COMMAND */
> +#define SLINK_CMD_ENB                  (1 << 31)
> +#define SLINK_CMD_GO                   (1 << 30)
> +#define SLINK_CMD_M_S                  (1 << 28)
> +#define SLINK_CMD_CK_SDA               (1 << 21)
> +#define SLINK_CMD_CS_POL               (1 << 13)
> +#define SLINK_CMD_CS_VAL               (1 << 12)
> +#define SLINK_CMD_CS_SOFT              (1 << 11)
> +#define SLINK_CMD_BIT_LENGTH           (1 << 4)
> +#define SLINK_CMD_BIT_LENGTH_MASK      0x0000001F
> +/* COMMAND2 */
> +#define SLINK_CMD2_TXEN                        (1 << 30)
> +#define SLINK_CMD2_RXEN                        (1 << 31)
> +#define SLINK_CMD2_SS_EN               (1 << 18)
> +#define SLINK_CMD2_SS_EN_SHIFT         18
> +#define SLINK_CMD2_SS_EN_MASK          0x000C0000
> +#define SLINK_CMD2_CS_ACTIVE_BETWEEN   (1 << 17)
> +/* STATUS */
> +#define SLINK_STAT_BSY                 (1 << 31)
> +#define SLINK_STAT_RDY                 (1 << 30)
> +#define SLINK_STAT_ERR                 (1 << 29)
> +#define SLINK_STAT_RXF_FLUSH           (1 << 27)
> +#define SLINK_STAT_TXF_FLUSH           (1 << 26)
> +#define SLINK_STAT_RXF_OVF             (1 << 25)
> +#define SLINK_STAT_TXF_UNR             (1 << 24)
> +#define SLINK_STAT_RXF_EMPTY           (1 << 23)
> +#define SLINK_STAT_RXF_FULL            (1 << 22)
> +#define SLINK_STAT_TXF_EMPTY           (1 << 21)
> +#define SLINK_STAT_TXF_FULL            (1 << 20)
> +#define SLINK_STAT_TXF_OVF             (1 << 19)
> +#define SLINK_STAT_RXF_UNR             (1 << 18)
> +#define SLINK_STAT_CUR_BLKCNT          (1 << 15)
> +/* STATUS2 */
> +#define SLINK_STAT2_RXF_FULL_CNT       (1 << 16)
> +#define SLINK_STAT2_TXF_FULL_CNT       (1 << 0)
> +
> +#define SPI_TIMEOUT            1000
> +#define TEGRA_SPI_MAX_FREQ     52000000
> +
> +#endif /* _TEGRA_SLINK_H_ */
> diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c
> index a4af539..63a7fcb 100644
> --- a/board/nvidia/common/board.c
> +++ b/board/nvidia/common/board.c
> @@ -131,10 +131,11 @@ int board_init(void)
>  #ifdef CONFIG_SPI_UART_SWITCH
>         gpio_config_uart();
>  #endif
> -#ifdef CONFIG_TEGRA_SPI
> +#if defined(CONFIG_TEGRA_SPI) || defined(CONFIG_TEGRA_SLINK)
>         pin_mux_spi();
>         spi_init();
>  #endif
> +
>  #ifdef CONFIG_PWM_TEGRA
>         if (pwm_init(gd->fdt_blob))
>                 debug("%s: Failed to init pwm\n", __func__);
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 824d357..83abcbd 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -46,6 +46,7 @@ COBJS-$(CONFIG_SOFT_SPI) += soft_spi.o
>  COBJS-$(CONFIG_SH_SPI) += sh_spi.o
>  COBJS-$(CONFIG_FSL_ESPI) += fsl_espi.o
>  COBJS-$(CONFIG_TEGRA_SPI) += tegra_spi.o
> +COBJS-$(CONFIG_TEGRA_SLINK) += tegra_slink.o
>  COBJS-$(CONFIG_XILINX_SPI) += xilinx_spi.o
>
>  COBJS  := $(COBJS-y)
> diff --git a/drivers/spi/tegra_slink.c b/drivers/spi/tegra_slink.c
> new file mode 100644
> index 0000000..6e1c436
> --- /dev/null
> +++ b/drivers/spi/tegra_slink.c
> @@ -0,0 +1,335 @@
> +/*
> + * NVIDIA Tegra SPI-SLINK controller
> + *
> + * Copyright (c) 2010-2013 NVIDIA Corporation
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h>
> +#include <malloc.h>
> +#include <asm/io.h>
> +#include <asm/gpio.h>
> +#include <asm/arch/clock.h>
> +#include <asm/arch-tegra/clk_rst.h>
> +#include <asm/arch-tegra/tegra_slink.h>
> +#include <spi.h>
> +#ifdef CONFIG_OF_CONTROL

You probably don't need this ifdef

> +#include <fdtdec.h>
> +#endif
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +struct tegra_spi_ctrl {
> +       struct slink_tegra *regs;
> +       unsigned int freq;
> +       unsigned int mode;
> +       int periph_id;
> +       int valid;
> +};
> +
> +struct tegra_spi_slave {
> +       struct spi_slave slave;
> +       struct tegra_spi_ctrl *ctrl;
> +};
> +
> +static struct tegra_spi_ctrl spi_ctrls[CONFIG_TEGRA_SLINK_CTRLS];
> +
> +static inline struct tegra_spi_slave *to_tegra_spi(struct spi_slave *slave)
> +{
> +       return container_of(slave, struct tegra_spi_slave, slave);
> +}
> +
> +int spi_cs_is_valid(unsigned int bus, unsigned int cs)
> +{
> +       if (bus >= CONFIG_TEGRA_SLINK_CTRLS || cs > 3 || !spi_ctrls[bus].valid)
> +               return 0;
> +       else
> +               return 1;
> +}
> +
> +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
> +               unsigned int max_hz, unsigned int mode)
> +{
> +       struct tegra_spi_slave *spi;
> +
> +       debug("%s: bus: %u, cs: %u, max_hz: %u, mode: %u\n", __func__,
> +               bus, cs, max_hz, mode);
> +
> +       if (!spi_cs_is_valid(bus, cs)) {
> +               printf("SPI error: unsupported bus %d / chip select %d\n",
> +                      bus, cs);
> +               return NULL;
> +       }
> +
> +       if (max_hz > TEGRA_SPI_MAX_FREQ) {
> +               printf("SPI error: unsupported frequency %d Hz. Max frequency"
> +                       " is %d Hz\n", max_hz, TEGRA_SPI_MAX_FREQ);
> +               return NULL;
> +       }
> +
> +       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/

> +       if (!spi) {
> +               printf("SPI error: malloc of SPI structure failed\n");
> +               return NULL;
> +       }
> +       spi->slave.bus = bus;
> +       spi->slave.cs = cs;
> +       spi->ctrl = &spi_ctrls[bus];
> +       if (!spi->ctrl) {
> +               printf("SPI error: could not find controller for bus %d\n",
> +                      bus);
> +               return NULL;
> +       }
> +
> +       if (max_hz < spi->ctrl->freq) {
> +               debug("%s: limiting frequency from %u to %u\n", __func__,
> +                     spi->ctrl->freq, max_hz);
> +               spi->ctrl->freq = max_hz;
> +       }
> +       spi->ctrl->mode = mode;
> +
> +       return &spi->slave;
> +}
> +
> +void spi_free_slave(struct spi_slave *slave)
> +{
> +       struct tegra_spi_slave *spi = to_tegra_spi(slave);
> +
> +       free(spi);
> +}
> +
> +void spi_init(void)
> +{
> +       int node = 0, i;
> +       struct tegra_spi_ctrl *ctrl;

blank line here

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

> +               ctrl->regs = (struct slink_tegra *)fdtdec_get_addr(gd->fdt_blob,
> +                                                                  node, "reg");
> +               if ((fdt_addr_t)ctrl->regs == FDT_ADDR_T_NONE) {
> +                       debug("%s: no slink register found\n", __func__);
> +                       continue;
> +               }
> +               ctrl->freq = fdtdec_get_int(gd->fdt_blob, node,
> +                                           "spi-max-frequency", 0);
> +               if (!ctrl->freq) {
> +                       debug("%s: no slink max frequency found\n", __func__);
> +                       continue;
> +               }
> +
> +               ctrl->periph_id = clock_decode_periph_id(gd->fdt_blob, node);
> +               if (ctrl->periph_id == PERIPH_ID_NONE) {
> +                       debug("%s: could not decode periph id\n", __func__);
> +                       continue;
> +               }
> +               ctrl->valid = 1;
> +#else
> +               u32 base_regs[] = {
> +                       NV_PA_SLINK1_BASE,
> +                       NV_PA_SLINK2_BASE,
> +                       NV_PA_SLINK3_BASE,
> +                       NV_PA_SLINK4_BASE,
> +                       NV_PA_SLINK5_BASE,
> +                       NV_PA_SLINK6_BASE,
> +               };
> +               int periph_ids[] = {
> +                       PERIPH_ID_SBC1,
> +                       PERIPH_ID_SBC2,
> +                       PERIPH_ID_SBC3,
> +                       PERIPH_ID_SBC4,
> +                       PERIPH_ID_SBC5,
> +                       PERIPH_ID_SBC6,
> +               }
> +               ctrl->regs = (struct slink_tegra *)base_regs[i];
> +               ctrl->freq = TEGRA_SPI_MAX_FREQ;
> +               ctrl->periph_id = periph_ids[i];
> +               ctrl->valid = 1;
> +#endif
> +
> +               debug("%s: found controller at %p, freq = %u, periph_id = %d\n",
> +                     __func__, ctrl->regs, ctrl->freq, ctrl->periph_id);
> +       }
> +}
> +
> +int spi_claim_bus(struct spi_slave *slave)
> +{
> +       struct tegra_spi_slave *spi = to_tegra_spi(slave);
> +       struct slink_tegra *regs = spi->ctrl->regs;
> +       u32 reg;
> +
> +       /* Change SPI clock to correct frequency, PLLP_OUT0 source */
> +       clock_start_periph_pll(spi->ctrl->periph_id, CLOCK_ID_PERIPH,
> +                              spi->ctrl->freq);
> +
> +       /* Clear stale status here */
> +       reg = SLINK_STAT_RDY | SLINK_STAT_RXF_FLUSH | SLINK_STAT_TXF_FLUSH | \
> +               SLINK_STAT_RXF_UNR | SLINK_STAT_TXF_OVF;
> +       writel(reg, &regs->status);
> +       debug("%s: STATUS = %08x\n", __func__, readl(&regs->status));
> +
> +       /* Set master mode and sw controlled CS */
> +       reg = readl(&regs->command);
> +       reg |= SLINK_CMD_M_S | SLINK_CMD_CS_SOFT;
> +       writel(reg, &regs->command);
> +       debug("%s: COMMAND = %08x\n", __func__, readl(&regs->command));
> +
> +       return 0;
> +}
> +
> +void spi_release_bus(struct spi_slave *slave)
> +{
> +}
> +
> +void spi_cs_activate(struct spi_slave *slave)
> +{
> +       struct tegra_spi_slave *spi = to_tegra_spi(slave);
> +       struct slink_tegra *regs = spi->ctrl->regs;
> +
> +       /* CS is negated on Tegra, so drive a 1 to get a 0 */
> +       setbits_le32(&regs->command, SLINK_CMD_CS_VAL);
> +}
> +
> +void spi_cs_deactivate(struct spi_slave *slave)
> +{
> +       struct tegra_spi_slave *spi = to_tegra_spi(slave);
> +       struct slink_tegra *regs = spi->ctrl->regs;
> +
> +       /* CS is negated on Tegra, so drive a 0 to get a 1 */
> +       clrbits_le32(&regs->command, SLINK_CMD_CS_VAL);
> +}
> +
> +int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
> +               const void *data_out, void *data_in, unsigned long flags)
> +{
> +       struct tegra_spi_slave *spi = to_tegra_spi(slave);
> +       struct slink_tegra *regs = spi->ctrl->regs;
> +       u32 reg, tmpdout, tmpdin = 0;
> +       const u8 *dout = data_out;
> +       u8 *din = data_in;
> +       int num_bytes;
> +       int ret;
> +
> +       debug("%s: slave %u:%u dout %p din %p bitlen %u\n",
> +             __func__, slave->bus, slave->cs, dout, din, bitlen);
> +       if (bitlen % 8)
debug()

> +               return -1;
> +       num_bytes = bitlen / 8;
> +
> +       ret = 0;
> +
> +       reg = readl(&regs->status);
> +       writel(reg, &regs->status);     /* Clear all SPI events via R/W */
> +       debug("%s entry: STATUS = %08x\n", __func__, reg);
> +
> +       reg = readl(&regs->status2);
> +       writel(reg, &regs->status2);    /* Clear all STATUS2 events via R/W */
> +       debug("%s entry: STATUS2 = %08x\n", __func__, reg);
> +
> +       debug("%s entry: COMMAND = %08x\n", __func__, readl(&regs->command));
> +
> +       reg = readl(&regs->command2);
> +       reg |= SLINK_CMD2_TXEN | SLINK_CMD2_RXEN |
> +               (slave->cs << SLINK_CMD2_SS_EN_SHIFT);
> +       writel(reg, &regs->command2);

Could use clrsetbits_le32() if you like

> +       debug("%s entry: COMMAND2 = %08x\n", __func__, reg);
> +
> +       if (flags & SPI_XFER_BEGIN)
> +               spi_cs_activate(slave);
> +
> +       /* handle data in 32-bit chunks */
> +       while (num_bytes > 0) {
> +               int bytes;
> +               int is_read = 0;
> +               int tm, i;
> +
> +               tmpdout = 0;
> +               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?

> +
> +               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?

> +                       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
Stephen Warren Jan. 14, 2013, 6:49 p.m. UTC | #2
On 01/12/2013 09:56 AM, 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

>> diff --git a/drivers/spi/tegra_slink.c b/drivers/spi/tegra_slink.c

>> +#ifdef CONFIG_OF_CONTROL
> 
> You probably don't need this ifdef

If we did assume CONFIG_OF_CONTROL was on, then we could get rid of the
previous patch; all the device addresses would come from device tree, so
there would be no need to add them to any header file:-)

>> +void spi_init(void)
>> +{
>> +       int node = 0, i;
>> +       struct tegra_spi_ctrl *ctrl;
> 
> blank line here
> 
>> +       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 strongly believe we shouldn't be using aliases for enumeration, which
is what fdtdec_find_aliases_for_id() does, IIRC. Instead, we should
enumerate all the devices in the correct fashion, and then apply aliases
as a separate step if they exists. Hence, I believe it's not correct to
use fdtdec_find_aliases_for_id() anywhere.
Allen Martin Jan. 15, 2013, 4:27 a.m. UTC | #3
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
Allen Martin Jan. 15, 2013, 4:33 a.m. UTC | #4
On Mon, Jan 14, 2013 at 10:49:53AM -0800, Stephen Warren wrote:
> On 01/12/2013 09:56 AM, 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
> 
> >> diff --git a/drivers/spi/tegra_slink.c b/drivers/spi/tegra_slink.c
> 
> >> +#ifdef CONFIG_OF_CONTROL
> > 
> > You probably don't need this ifdef
> 
> If we did assume CONFIG_OF_CONTROL was on, then we could get rid of the
> previous patch; all the device addresses would come from device tree, so
> there would be no need to add them to any header file:-)

I took Simon's comment to mean it's safe to include fdtdec.h even if
the fdt code is ifdef'ed out.

> 
> >> +void spi_init(void)
> >> +{
> >> +       int node = 0, i;
> >> +       struct tegra_spi_ctrl *ctrl;
> > 
> > blank line here
> > 
> >> +       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 strongly believe we shouldn't be using aliases for enumeration, which
> is what fdtdec_find_aliases_for_id() does, IIRC. Instead, we should
> enumerate all the devices in the correct fashion, and then apply aliases
> as a separate step if they exists. Hence, I believe it's not correct to
> use fdtdec_find_aliases_for_id() anywhere.

I'm a bit confused about the usage of fdt aliases in u-boot.  My
understanding of aliases is that they are intended to give drivers
hints about device naming.  Since u-boot doesn't really name devices,
what are aliases supposed to do?

-Allen
Stephen Warren Jan. 15, 2013, 4:36 p.m. UTC | #5
On 01/14/2013 09:33 PM, Allen Martin wrote:
> On Mon, Jan 14, 2013 at 10:49:53AM -0800, Stephen Warren wrote:
>> On 01/12/2013 09:56 AM, 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

>>>> +void spi_init(void)
>>>> +{
>>>> +       int node = 0, i;
>>>> +       struct tegra_spi_ctrl *ctrl;
>>>
>>> blank line here
>>>
>>>> +       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 strongly believe we shouldn't be using aliases for enumeration, which
>> is what fdtdec_find_aliases_for_id() does, IIRC. Instead, we should
>> enumerate all the devices in the correct fashion, and then apply aliases
>> as a separate step if they exists. Hence, I believe it's not correct to
>> use fdtdec_find_aliases_for_id() anywhere.
> 
> I'm a bit confused about the usage of fdt aliases in u-boot.  My
> understanding of aliases is that they are intended to give drivers
> hints about device naming.  Since u-boot doesn't really name devices,
> what are aliases supposed to do?

Most U-Boot command use IDs to identify devices/interfaces, e.g. in
"ext2load mmc 0:1", the "0" means MMC instance #0. The instance number
is parsed out from the end of the alias name.
Simon Glass Jan. 22, 2013, 1:56 p.m. UTC | #6
Hi Allen,

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

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
Simon Glass Jan. 22, 2013, 2:06 p.m. UTC | #7
Hi Stephen,

On Mon, Jan 14, 2013 at 10:49 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 01/12/2013 09:56 AM, 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
>
>>> diff --git a/drivers/spi/tegra_slink.c b/drivers/spi/tegra_slink.c
>
>>> +#ifdef CONFIG_OF_CONTROL
>>
>> You probably don't need this ifdef
>
> If we did assume CONFIG_OF_CONTROL was on, then we could get rid of the
> previous patch; all the device addresses would come from device tree, so
> there would be no need to add them to any header file:-)

As Allen said I just mean we shouldn't need to put #ifdefs around
#include <fdtdec.h>

>
>>> +void spi_init(void)
>>> +{
>>> +       int node = 0, i;
>>> +       struct tegra_spi_ctrl *ctrl;
>>
>> blank line here
>>
>>> +       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 strongly believe we shouldn't be using aliases for enumeration, which
> is what fdtdec_find_aliases_for_id() does, IIRC. Instead, we should
> enumerate all the devices in the correct fashion, and then apply aliases
> as a separate step if they exists. Hence, I believe it's not correct to
> use fdtdec_find_aliases_for_id() anywhere.

We might be saying the same thing, but I can't tell. We had quite a
long discussion about this when the function was written and I thought
it was then considered correct.

The function does not ignore nodes without an alias. It simply returns
a list of nodes that your driver should init, in the order in which
U-Boot expects to find them. So I think it is safe to call, and in
fact should be called in any driver that has more than one device and
wants to provide the ability to select device ordering. Please can you
take another look at the function and see what you think?

We should not enumerate devices that are not supposed to be used, so
checking fdtdec_get_is_enabled() might be good too.

Regards,
Simon
diff mbox

Patch

diff --git a/arch/arm/include/asm/arch-tegra/tegra_slink.h b/arch/arm/include/asm/arch-tegra/tegra_slink.h
new file mode 100644
index 0000000..74804b5
--- /dev/null
+++ b/arch/arm/include/asm/arch-tegra/tegra_slink.h
@@ -0,0 +1,84 @@ 
+/*
+ * NVIDIA Tegra SPI-SLINK controller
+ *
+ * Copyright 2010-2013 NVIDIA Corporation
+ *
+ * This software may be used and distributed according to the
+ * terms of the GNU Public License, Version 2, incorporated
+ * herein by reference.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * Version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#ifndef _TEGRA_SLINK_H_
+#define _TEGRA_SLINK_H_
+
+#include <asm/types.h>
+
+struct slink_tegra {
+	u32 command;	/* SLINK_COMMAND_0 register  */
+	u32 command2;	/* SLINK_COMMAND2_0 reg */
+	u32 status;	/* SLINK_STATUS_0 register */
+	u32 reserved;	/* Reserved offset 0C */
+	u32 mas_data;	/* SLINK_MAS_DATA_0 reg */
+	u32 slav_data;	/* SLINK_SLAVE_DATA_0 reg */
+	u32 dma_ctl;	/* SLINK_DMA_CTL_0 register */
+	u32 status2;	/* SLINK_STATUS2_0 reg */
+	u32 rsvd[56];	/* 0x20 to 0xFF reserved */
+	u32 tx_fifo;	/* SLINK_TX_FIFO_0 reg off 100h */
+	u32 rsvd2[31];	/* 0x104 to 0x17F reserved */
+	u32 rx_fifo;	/* SLINK_RX_FIFO_0 reg off 180h */
+};
+
+/* COMMAND */
+#define SLINK_CMD_ENB			(1 << 31)
+#define SLINK_CMD_GO			(1 << 30)
+#define SLINK_CMD_M_S			(1 << 28)
+#define SLINK_CMD_CK_SDA		(1 << 21)
+#define SLINK_CMD_CS_POL		(1 << 13)
+#define SLINK_CMD_CS_VAL		(1 << 12)
+#define SLINK_CMD_CS_SOFT		(1 << 11)
+#define SLINK_CMD_BIT_LENGTH		(1 << 4)
+#define SLINK_CMD_BIT_LENGTH_MASK	0x0000001F
+/* COMMAND2 */
+#define SLINK_CMD2_TXEN			(1 << 30)
+#define SLINK_CMD2_RXEN			(1 << 31)
+#define SLINK_CMD2_SS_EN		(1 << 18)
+#define SLINK_CMD2_SS_EN_SHIFT		18
+#define SLINK_CMD2_SS_EN_MASK		0x000C0000
+#define SLINK_CMD2_CS_ACTIVE_BETWEEN	(1 << 17)
+/* STATUS */
+#define SLINK_STAT_BSY			(1 << 31)
+#define SLINK_STAT_RDY			(1 << 30)
+#define SLINK_STAT_ERR			(1 << 29)
+#define SLINK_STAT_RXF_FLUSH		(1 << 27)
+#define SLINK_STAT_TXF_FLUSH		(1 << 26)
+#define SLINK_STAT_RXF_OVF		(1 << 25)
+#define SLINK_STAT_TXF_UNR		(1 << 24)
+#define SLINK_STAT_RXF_EMPTY		(1 << 23)
+#define SLINK_STAT_RXF_FULL		(1 << 22)
+#define SLINK_STAT_TXF_EMPTY		(1 << 21)
+#define SLINK_STAT_TXF_FULL		(1 << 20)
+#define SLINK_STAT_TXF_OVF		(1 << 19)
+#define SLINK_STAT_RXF_UNR		(1 << 18)
+#define SLINK_STAT_CUR_BLKCNT		(1 << 15)
+/* STATUS2 */
+#define SLINK_STAT2_RXF_FULL_CNT	(1 << 16)
+#define SLINK_STAT2_TXF_FULL_CNT	(1 << 0)
+
+#define SPI_TIMEOUT		1000
+#define TEGRA_SPI_MAX_FREQ	52000000
+
+#endif	/* _TEGRA_SLINK_H_ */
diff --git a/board/nvidia/common/board.c b/board/nvidia/common/board.c
index a4af539..63a7fcb 100644
--- a/board/nvidia/common/board.c
+++ b/board/nvidia/common/board.c
@@ -131,10 +131,11 @@  int board_init(void)
 #ifdef CONFIG_SPI_UART_SWITCH
 	gpio_config_uart();
 #endif
-#ifdef CONFIG_TEGRA_SPI
+#if defined(CONFIG_TEGRA_SPI) || defined(CONFIG_TEGRA_SLINK)
 	pin_mux_spi();
 	spi_init();
 #endif
+
 #ifdef CONFIG_PWM_TEGRA
 	if (pwm_init(gd->fdt_blob))
 		debug("%s: Failed to init pwm\n", __func__);
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 824d357..83abcbd 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -46,6 +46,7 @@  COBJS-$(CONFIG_SOFT_SPI) += soft_spi.o
 COBJS-$(CONFIG_SH_SPI) += sh_spi.o
 COBJS-$(CONFIG_FSL_ESPI) += fsl_espi.o
 COBJS-$(CONFIG_TEGRA_SPI) += tegra_spi.o
+COBJS-$(CONFIG_TEGRA_SLINK) += tegra_slink.o
 COBJS-$(CONFIG_XILINX_SPI) += xilinx_spi.o
 
 COBJS	:= $(COBJS-y)
diff --git a/drivers/spi/tegra_slink.c b/drivers/spi/tegra_slink.c
new file mode 100644
index 0000000..6e1c436
--- /dev/null
+++ b/drivers/spi/tegra_slink.c
@@ -0,0 +1,335 @@ 
+/*
+ * NVIDIA Tegra SPI-SLINK controller
+ *
+ * Copyright (c) 2010-2013 NVIDIA Corporation
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <asm/io.h>
+#include <asm/gpio.h>
+#include <asm/arch/clock.h>
+#include <asm/arch-tegra/clk_rst.h>
+#include <asm/arch-tegra/tegra_slink.h>
+#include <spi.h>
+#ifdef CONFIG_OF_CONTROL
+#include <fdtdec.h>
+#endif
+
+DECLARE_GLOBAL_DATA_PTR;
+
+struct tegra_spi_ctrl {
+	struct slink_tegra *regs;
+	unsigned int freq;
+	unsigned int mode;
+	int periph_id;
+	int valid;
+};
+
+struct tegra_spi_slave {
+	struct spi_slave slave;
+	struct tegra_spi_ctrl *ctrl;
+};
+
+static struct tegra_spi_ctrl spi_ctrls[CONFIG_TEGRA_SLINK_CTRLS];
+
+static inline struct tegra_spi_slave *to_tegra_spi(struct spi_slave *slave)
+{
+	return container_of(slave, struct tegra_spi_slave, slave);
+}
+
+int spi_cs_is_valid(unsigned int bus, unsigned int cs)
+{
+	if (bus >= CONFIG_TEGRA_SLINK_CTRLS || cs > 3 || !spi_ctrls[bus].valid)
+		return 0;
+	else
+		return 1;
+}
+
+struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
+		unsigned int max_hz, unsigned int mode)
+{
+	struct tegra_spi_slave *spi;
+
+	debug("%s: bus: %u, cs: %u, max_hz: %u, mode: %u\n", __func__,
+		bus, cs, max_hz, mode);
+
+	if (!spi_cs_is_valid(bus, cs)) {
+		printf("SPI error: unsupported bus %d / chip select %d\n",
+		       bus, cs);
+		return NULL;
+	}
+
+	if (max_hz > TEGRA_SPI_MAX_FREQ) {
+		printf("SPI error: unsupported frequency %d Hz. Max frequency"
+			" is %d Hz\n", max_hz, TEGRA_SPI_MAX_FREQ);
+		return NULL;
+	}
+
+	spi = malloc(sizeof(struct tegra_spi_slave));
+	if (!spi) {
+		printf("SPI error: malloc of SPI structure failed\n");
+		return NULL;
+	}
+	spi->slave.bus = bus;
+	spi->slave.cs = cs;
+	spi->ctrl = &spi_ctrls[bus];
+	if (!spi->ctrl) {
+		printf("SPI error: could not find controller for bus %d\n",
+		       bus);
+		return NULL;
+	}
+
+	if (max_hz < spi->ctrl->freq) {
+		debug("%s: limiting frequency from %u to %u\n", __func__,
+		      spi->ctrl->freq, max_hz);
+		spi->ctrl->freq = max_hz;
+	}
+	spi->ctrl->mode = mode;
+
+	return &spi->slave;
+}
+
+void spi_free_slave(struct spi_slave *slave)
+{
+	struct tegra_spi_slave *spi = to_tegra_spi(slave);
+
+	free(spi);
+}
+
+void spi_init(void)
+{
+	int node = 0, i;
+	struct tegra_spi_ctrl *ctrl;
+	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;
+		ctrl->regs = (struct slink_tegra *)fdtdec_get_addr(gd->fdt_blob,
+								   node, "reg");
+		if ((fdt_addr_t)ctrl->regs == FDT_ADDR_T_NONE) {
+			debug("%s: no slink register found\n", __func__);
+			continue;
+		}
+		ctrl->freq = fdtdec_get_int(gd->fdt_blob, node,
+					    "spi-max-frequency", 0);
+		if (!ctrl->freq) {
+			debug("%s: no slink max frequency found\n", __func__);
+			continue;
+		}
+
+		ctrl->periph_id = clock_decode_periph_id(gd->fdt_blob, node);
+		if (ctrl->periph_id == PERIPH_ID_NONE) {
+			debug("%s: could not decode periph id\n", __func__);
+			continue;
+		}
+		ctrl->valid = 1;
+#else
+		u32 base_regs[] = {
+			NV_PA_SLINK1_BASE,
+			NV_PA_SLINK2_BASE,
+			NV_PA_SLINK3_BASE,
+			NV_PA_SLINK4_BASE,
+			NV_PA_SLINK5_BASE,
+			NV_PA_SLINK6_BASE,
+		};
+		int periph_ids[] = {
+			PERIPH_ID_SBC1,
+			PERIPH_ID_SBC2,
+			PERIPH_ID_SBC3,
+			PERIPH_ID_SBC4,
+			PERIPH_ID_SBC5,
+			PERIPH_ID_SBC6,
+		}
+		ctrl->regs = (struct slink_tegra *)base_regs[i];
+		ctrl->freq = TEGRA_SPI_MAX_FREQ;
+		ctrl->periph_id = periph_ids[i];
+		ctrl->valid = 1;
+#endif
+
+		debug("%s: found controller at %p, freq = %u, periph_id = %d\n",
+		      __func__, ctrl->regs, ctrl->freq, ctrl->periph_id);
+	}
+}
+
+int spi_claim_bus(struct spi_slave *slave)
+{
+	struct tegra_spi_slave *spi = to_tegra_spi(slave);
+	struct slink_tegra *regs = spi->ctrl->regs;
+	u32 reg;
+
+	/* Change SPI clock to correct frequency, PLLP_OUT0 source */
+	clock_start_periph_pll(spi->ctrl->periph_id, CLOCK_ID_PERIPH,
+			       spi->ctrl->freq);
+
+	/* Clear stale status here */
+	reg = SLINK_STAT_RDY | SLINK_STAT_RXF_FLUSH | SLINK_STAT_TXF_FLUSH | \
+		SLINK_STAT_RXF_UNR | SLINK_STAT_TXF_OVF;
+	writel(reg, &regs->status);
+	debug("%s: STATUS = %08x\n", __func__, readl(&regs->status));
+
+	/* Set master mode and sw controlled CS */
+	reg = readl(&regs->command);
+	reg |= SLINK_CMD_M_S | SLINK_CMD_CS_SOFT;
+	writel(reg, &regs->command);
+	debug("%s: COMMAND = %08x\n", __func__, readl(&regs->command));
+
+	return 0;
+}
+
+void spi_release_bus(struct spi_slave *slave)
+{
+}
+
+void spi_cs_activate(struct spi_slave *slave)
+{
+	struct tegra_spi_slave *spi = to_tegra_spi(slave);
+	struct slink_tegra *regs = spi->ctrl->regs;
+
+	/* CS is negated on Tegra, so drive a 1 to get a 0 */
+	setbits_le32(&regs->command, SLINK_CMD_CS_VAL);
+}
+
+void spi_cs_deactivate(struct spi_slave *slave)
+{
+	struct tegra_spi_slave *spi = to_tegra_spi(slave);
+	struct slink_tegra *regs = spi->ctrl->regs;
+
+	/* CS is negated on Tegra, so drive a 0 to get a 1 */
+	clrbits_le32(&regs->command, SLINK_CMD_CS_VAL);
+}
+
+int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
+		const void *data_out, void *data_in, unsigned long flags)
+{
+	struct tegra_spi_slave *spi = to_tegra_spi(slave);
+	struct slink_tegra *regs = spi->ctrl->regs;
+	u32 reg, tmpdout, tmpdin = 0;
+	const u8 *dout = data_out;
+	u8 *din = data_in;
+	int num_bytes;
+	int ret;
+
+	debug("%s: slave %u:%u dout %p din %p bitlen %u\n",
+	      __func__, slave->bus, slave->cs, dout, din, bitlen);
+	if (bitlen % 8)
+		return -1;
+	num_bytes = bitlen / 8;
+
+	ret = 0;
+
+	reg = readl(&regs->status);
+	writel(reg, &regs->status);	/* Clear all SPI events via R/W */
+	debug("%s entry: STATUS = %08x\n", __func__, reg);
+
+	reg = readl(&regs->status2);
+	writel(reg, &regs->status2);	/* Clear all STATUS2 events via R/W */
+	debug("%s entry: STATUS2 = %08x\n", __func__, reg);
+
+	debug("%s entry: COMMAND = %08x\n", __func__, readl(&regs->command));
+
+	reg = readl(&regs->command2);
+	reg |= SLINK_CMD2_TXEN | SLINK_CMD2_RXEN |
+		(slave->cs << SLINK_CMD2_SS_EN_SHIFT);
+	writel(reg, &regs->command2);
+	debug("%s entry: COMMAND2 = %08x\n", __func__, reg);
+
+	if (flags & SPI_XFER_BEGIN)
+		spi_cs_activate(slave);
+
+	/* handle data in 32-bit chunks */
+	while (num_bytes > 0) {
+		int bytes;
+		int is_read = 0;
+		int tm, i;
+
+		tmpdout = 0;
+		bytes = (num_bytes > 4) ?  4 : num_bytes;
+
+		if (dout != NULL) {
+			for (i = 0; i < bytes; ++i)
+				tmpdout = (tmpdout << 8) | dout[i];
+		}
+
+		num_bytes -= bytes;
+		if (dout)
+			dout += bytes;
+
+		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;
+
+			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)