Re: [U-Boot] [UBOOT][PATCHv4 4/6] spi: add TI QSPI driver

On Fri, Oct 4, 2013 at 8:21 PM, Sourav Poddar sourav.poddar@ti.com wrote:

From: Matt Porter matt.porter@linaro.org

Adds a SPI master driver for the TI QSPI peripheral.

Signed-off-by: Matt Porter matt.porter@linaro.org Signed-off-by: Sourav Poddar sourav.poddar@ti.com [Added quad read support and memory mapped support).

What is this comment, any specific?

You missed change log for all patches, i think you have summarized in 0/6. I feel it's better write it on individual patches.

drivers/spi/Makefile | 1 + drivers/spi/ti_qspi.c | 328 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 329 insertions(+), 0 deletions(-) create mode 100644 drivers/spi/ti_qspi.c

diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index 91d24ce..e5941b0 100644 --- a/drivers/spi/Makefile +++ b/drivers/spi/Makefile @@ -38,6 +38,7 @@ COBJS-$(CONFIG_FDT_SPI) += fdt_spi.o COBJS-$(CONFIG_TEGRA20_SFLASH) += tegra20_sflash.o COBJS-$(CONFIG_TEGRA20_SLINK) += tegra20_slink.o COBJS-$(CONFIG_TEGRA114_SPI) += tegra114_spi.o +COBJS-$(CONFIG_TI_QSPI) += ti_qspi.o COBJS-$(CONFIG_XILINX_SPI) += xilinx_spi.o COBJS-$(CONFIG_ZYNQ_SPI) += zynq_spi.o

diff --git a/drivers/spi/ti_qspi.c b/drivers/spi/ti_qspi.c new file mode 100644 index 0000000..d8a03a8 --- /dev/null +++ b/drivers/spi/ti_qspi.c @@ -0,0 +1,328 @@ +/*

• • TI QSPI driver
• • Copyright (C) 2013, Texas Instruments, Incorporated
• • SPDX-License-Identifier: GPL-2.0+

Got below format after apply this patch - please check *Â SPDX-License-Identifier:Â Â Â Â Â GPL-2.0+

• */

+#include <common.h> +#include <asm/io.h> +#include <asm/arch/omap.h> +#include <malloc.h> +#include <spi.h>

+struct qspi_regs { +u32 pid; +u32 pad0[3]; +u32 sysconfig; +u32 pad1[3]; +u32 intr_status_raw_set; +u32 intr_status_enabled_clear; +u32 intr_enable_set; +u32 intr_enable_clear; +u32 intc_eoi; +u32 pad2[3]; +u32 spi_clock_cntrl; +u32 spi_dc; +u32 spi_cmd; +u32 spi_status; +u32 spi_data; +u32 spi_setup0; +u32 spi_setup1; +u32 spi_setup2; +u32 spi_setup3; +u32 spi_switch; +u32 spi_data1; +u32 spi_data2; +u32 spi_data3;

Please add tab space.

+};

+struct qspi_slave {

•   struct spi_slave slave;
  

•   struct qspi_regs *base;
  

•   unsigned int mode;
  

•   u32 cmd;
  

•   u32 dc;
  

+};

-- TAG+

+#define QSPI_TIMEOUT 2000000

+#define QSPI_FCLK 192000000

+/* Clock Control */ +#define QSPI_CLK_EN (1 << 31) +#define QSPI_CLK_DIV_MAX 0xffff

+/* Command */ +#define QSPI_EN_CS(n) (n << 28) +#define QSPI_WLEN(n) ((n-1) << 19) +#define QSPI_3_PIN (1 << 18) +#define QSPI_RD_SNGL (1 << 16) +#define QSPI_WR_SNGL (2 << 16) +#define QSPI_INVAL (4 << 16) +#define QSPI_RD_QUAD (7 << 16)

+/* Device Control */ +#define QSPI_DD(m, n) (m << (3 + n*8)) +#define QSPI_CKPHA(n) (1 << (2 + n*8)) +#define QSPI_CSPOL(n) (1 << (1 + n*8)) +#define QSPI_CKPOL(n) (1 << (n*8))

+/* Status */ +#define QSPI_WC (1 << 1) +#define QSPI_BUSY (1 << 0) +#define QSPI_WC_BUSY (QSPI_WC | QSPI_BUSY) +#define QSPI_XFER_DONE QSPI_WC

+#define MM_SWITCH 0x01 +#define MEM_CS 0x100 +#define MEM_CS_UNSELECT 0xfffff0ff +#define MMAP_START_ADDR 0x5c000000 +#define CORE_CTRL_IO 0x4a002558

+#define QSPI_CMD_READ (0x3 << 0) +#define QSPI_CMD_READ_QUAD (0x6b << 0) +#define QSPI_CMD_READ_FAST (0x0b << 0) +#define QSPI_SETUP0_NUM_A_BYTES (0x2 << 8) +#define QSPI_SETUP0_NUM_D_BYTES_NO_BITS (0x0 << 10) +#define QSPI_SETUP0_NUM_D_BYTES_8_BITS (0x1 << 10) +#define QSPI_SETUP0_READ_NORMAL (0x0 << 12) +#define QSPI_SETUP0_READ_QUAD (0x3 << 12) +#define QSPI_CMD_WRITE (0x2 << 16) +#define QSPI_NUM_DUMMY_BITS (0x0 << 24)

--TAG-

TAG+ ... TAG- please move these macro definitions in below headers

+static inline struct qspi_slave *to_qspi_slave(struct spi_slave *slave) +{

•   return container_of(slave, struct qspi_slave, slave);
  

+} +static inline struct qspi_regs *get_qspi_bus(int dev) +{

•   if (!dev)
  

•           return (struct qspi_regs *)QSPI_BASE;
  

•   else
  

•           return NULL;
  

+}

Is this function really required, how many bus controller you have?

+int spi_cs_is_valid(unsigned int bus, unsigned int cs) +{

•   return 1;
  

+}

+void spi_cs_activate(struct spi_slave *slave) +{

•   /* CS handled in xfer */
  

•   return;
  

+}

+void spi_cs_deactivate(struct spi_slave *slave) +{

•   /* CS handled in xfer */
  

•   return;
  

+}

+void spi_init(void) +{

•   /* nothing to do */
  

+}

+void spi_set_up_spi_register(struct qspi_slave *qslave) +{

•   u32 memval = 0;
  

•   struct spi_slave *slave = &qslave->slave;
  

•   slave->memory_map = (void *)MMAP_START_ADDR;
  

•   memval |= (QSPI_CMD_READ | QSPI_SETUP0_NUM_A_BYTES |
  

•           QSPI_SETUP0_NUM_D_BYTES_NO_BITS | QSPI_SETUP0_READ_NORMAL |
  

•                   QSPI_CMD_WRITE | QSPI_NUM_DUMMY_BITS);
  

•   writel(memval, &qslave->base->spi_setup0);
  

+}

+void spi_set_speed(struct spi_slave *slave, uint hz) +{

•   struct qspi_slave *qslave = to_qspi_slave(slave);
  

•   uint clk_div;
  

•   if (!hz)
  

•           clk_div = 0;
  

•   else
  

•           clk_div = (QSPI_FCLK / hz) - 1;
  

•   debug("%s: hz: %d, clock divider %d\n", __func__, hz, clk_div);
  

•   /* disable SCLK */
  

•   writel(readl(&qslave->base->spi_clock_cntrl) & ~QSPI_CLK_EN,
  

•                   &qslave->base->spi_clock_cntrl);
  

•   if (clk_div < 0) {
  

•           debug("%s: clock divider < 0, using /1 divider\n", __func__);
  

•           clk_div = 0;
  

•   }
  

•   if (clk_div > QSPI_CLK_DIV_MAX) {
  

•           debug("%s: clock divider >%d , using /%d divider\n",
  

•                   __func__, QSPI_CLK_DIV_MAX, QSPI_CLK_DIV_MAX + 1);
  

•           clk_div = QSPI_CLK_DIV_MAX;
  

•   }
  

•   /* enable SCLK */
  

•   writel(QSPI_CLK_EN | clk_div, &qslave->base->spi_clock_cntrl);
  

•   debug("%s: spi_clock_cntrl %08x\n", __func__,
  

•           readl(&qslave->base->spi_clock_cntrl));
  

+}

+struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,

•                             unsigned int max_hz, unsigned int mode)
  

+{

•   struct qspi_slave *qslave;
  

•   qslave = spi_alloc_slave(struct qspi_slave, bus, cs);
  

•   if (!qslave)
  

•           return NULL;
  

•   qslave->base = get_qspi_bus(bus);
  

•   if (qslave->base)
  

•           debug("No Qspi device found\n");
  

•   spi_set_speed(&qslave->slave, max_hz);
  

•   qslave->mode = mode;
  

+#ifdef CONFIG_MMAP

•   spi_set_up_spi_register(qslave);
  

+#endif

•   debug("%s: bus:%i cs:%i mode:%i\n", __func__, bus, cs, mode);
  

•   return &qslave->slave;
  

+}

+void spi_free_slave(struct spi_slave *slave) +{

•   struct qspi_slave *qslave = to_qspi_slave(slave);
  

•   free(qslave);
  

+}

+int spi_claim_bus(struct spi_slave *slave) +{

•   struct qspi_slave *qslave = to_qspi_slave(slave);
  

•   debug("%s: bus:%i cs:%i\n", __func__, slave->bus, slave->cs);
  

•   writel(0, &qslave->base->spi_dc);
  

•   writel(0, &qslave->base->spi_cmd);
  

•   writel(0, &qslave->base->spi_data);
  

•   return 0;
  

+}

+void spi_release_bus(struct spi_slave *slave) +{

•   struct qspi_slave *qslave = to_qspi_slave(slave);
  

•   debug("%s: bus:%i cs:%i\n", __func__, slave->bus, slave->cs);
  

•   writel(0, &qslave->base->spi_dc);
  

•   writel(0, &qslave->base->spi_cmd);
  

•   writel(0, &qslave->base->spi_data);
  

+}

+int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout,

•        void *din, unsigned long flags)
  

+{

•   struct qspi_slave *qslave = to_qspi_slave(slave);
  

•   uint words = bitlen >> 3; /* fixed 8-bit word length */
  

•   const uchar *txp = dout;
  

•   uchar *rxp = din;
  

•   uint status;
  

•   int timeout, val;
  

•   debug("%s: bus:%i cs:%i bitlen:%i words:%i flags:%lx\n", __func__,
  

•           slave->bus, slave->cs, bitlen, words, flags);
  

•   qslave->dc = 0;
  

•   if (qslave->mode & SPI_CPHA)
  

•           qslave->dc |= QSPI_CKPHA(slave->cs);
  

•   if (qslave->mode & SPI_CPOL)
  

•           qslave->dc |= QSPI_CKPOL(slave->cs);
  

•   if (qslave->mode & SPI_CS_HIGH)
  

•           qslave->dc |= QSPI_CSPOL(slave->cs);
  

•   writel(qslave->dc, &qslave->base->spi_dc);
  

Adjust this code in spi_claim_bus()

•   if (flags & SPI_XFER_MEM_MAP) {
  

•           writel(MM_SWITCH, &qslave->base->spi_switch);
  

•           val = readl(CORE_CTRL_IO);
  

•           val |= MEM_CS;
  

•           writel(val, CORE_CTRL_IO);
  

•           return 0;
  

•   } else if (flags & SPI_XFER_MEM_MAP_END) {
  

•           writel(~MM_SWITCH, &qslave->base->spi_switch);
  

•           val = readl(CORE_CTRL_IO);
  

•           val &= MEM_CS_UNSELECT;
  

•           writel(val, CORE_CTRL_IO);
  

•           return 0;
  

•   }
  

What is this your are returning from here directly for memory_map? is it?

--TAG+

•   if (bitlen == 0)
  

•           goto out;
  

•   if (bitlen % 8) {
  

•           flags |= SPI_XFER_END;
  

•           goto out;
  

•   }
  

--TAG-

TAG+ .. TAG- please move this code on start of the xfer..ie below debug();

•   /* setup command reg */
  

•   qslave->cmd = 0;
  

•   qslave->cmd |= QSPI_WLEN(8);
  

•   qslave->cmd |= QSPI_EN_CS(slave->cs);
  

•   if (flags & SPI_3WIRE)
  

•           qslave->cmd |= QSPI_3_PIN;
  

•   qslave->cmd |= 0xfff;
  

•   while (words--) {
  

•           if (txp) {
  

•                   debug("tx cmd %08x dc %08x data %02x\n",
  

•                         qslave->cmd | QSPI_WR_SNGL, qslave->dc, *txp);
  

•                   writel(*txp++, &qslave->base->spi_data);
  

•                   writel(qslave->cmd | QSPI_WR_SNGL,
  

•                           &qslave->base->spi_cmd);
  

•                   status = readl(&qslave->base->spi_status);
  

•                   timeout = QSPI_TIMEOUT;
  

•                   while ((status & QSPI_WC_BUSY) != QSPI_XFER_DONE) {
  

•                           if (--timeout < 0) {
  

•                                   printf("QSPI tx timed out\n");
  

•                                   return -1;
  

•                           }
  

•                           status = readl(&qslave->base->spi_status);
  

•                   }
  

•                   debug("tx done, status %08x\n", status);
  

•           }
  

•           if (rxp) {
  

•                   qslave->cmd |= QSPI_RD_SNGL;
  

•                   debug("rx cmd %08x dc %08x\n",
  

•                           qslave->cmd, qslave->dc);
  

•                   writel(qslave->cmd, &qslave->base->spi_cmd);
  

•                   status = readl(&qslave->base->spi_status);
  

•                   timeout = QSPI_TIMEOUT;
  

•                   while ((status & QSPI_WC_BUSY) != QSPI_XFER_DONE) {
  

•                           if (--timeout < 0) {
  

•                                   printf("QSPI rx timed out\n");
  

•                                   return -1;
  

•                           }
  

•                           status = readl(&qslave->base->spi_status);
  

•                   }
  

•                   *rxp++ = readl(&qslave->base->spi_data);
  

•                   debug("rx done, status %08x, read %02x\n",
  

•                           status, *(rxp-1));
  

•           }
  

•   }
  

+out:

•   /* Terminate frame */
  

•   if (flags & SPI_XFER_END)
  

•           writel(qslave->cmd | QSPI_INVAL, &qslave->base->spi_cmd);
  

Please palce this code in spi_cs_deactivate()

I request you please follow the code structure in below thread. I feel better if you use same prints that used in below example driver. http://patchwork.ozlabs.org/patch/265683/