[U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver

[U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver

[Tom Warren]

This is the normal Tegra SPI driver modified to work with the
QSPI controller in Tegra210. It does not do 2x/4x transfers
or any other QSPI protocol.

Author: Yen Lin <yelin@nvidia.com>
Signed-off-by: Yen Lin <yelin@nvidia.com>
Signed-off-by: Tom Warren <twarren@nvidia.com>
---
Changes in v2:
- Drop defconfig and pinmux files, this is a driver-only patch.
- If/when pinmux tables have been updated for P2371/P2571, another
- patch will be sent to enable the QSPI driver on those boards.
Changes in v3:
- removed status reg write/clear in claim_bus(), done in xfer()

 drivers/spi/Kconfig         |   5 +
 drivers/spi/Makefile        |   1 +
 drivers/spi/tegra210_qspi.c | 400 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 406 insertions(+)
 create mode 100644 drivers/spi/tegra210_qspi.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 8e04fce..168f31d 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -115,6 +115,11 @@ config TEGRA20_SLINK
 	  be used to access the SPI NOR flash on platforms embedding this
 	  nVidia Tegra20/Tegra30 IP cores.
 
+config TEGRA210_QSPI
+	bool "nVidia Tegra210 QSPI driver"
+	help
+	  Enable the Tegra Quad-SPI (QSPI) driver for T210.
+
 config XILINX_SPI
 	bool "Xilinx SPI driver"
 	help
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index de241be..209a41e 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_SH_QSPI) += sh_qspi.o
 obj-$(CONFIG_TEGRA114_SPI) += tegra114_spi.o
 obj-$(CONFIG_TEGRA20_SFLASH) += tegra20_sflash.o
 obj-$(CONFIG_TEGRA20_SLINK) += tegra20_slink.o
+obj-$(CONFIG_TEGRA210_QSPI) += tegra210_qspi.o
 obj-$(CONFIG_TI_QSPI) += ti_qspi.o
 obj-$(CONFIG_XILINX_SPI) += xilinx_spi.o
 obj-$(CONFIG_ZYNQ_SPI) += zynq_spi.o
diff --git a/drivers/spi/tegra210_qspi.c b/drivers/spi/tegra210_qspi.c
new file mode 100644
index 0000000..6be37f3
--- /dev/null
+++ b/drivers/spi/tegra210_qspi.c
@@ -0,0 +1,400 @@
+/*
+ * NVIDIA Tegra210 QSPI controller driver
+ *  (C) Copyright 2015
+ *  NVIDIA Corporation <www.nvidia.com>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <asm/io.h>
+#include <asm/arch/clock.h>
+#include <asm/arch-tegra/clk_rst.h>
+#include <spi.h>
+#include <fdtdec.h>
+#include "tegra_spi.h"
+
+DECLARE_GLOBAL_DATA_PTR;
+
+/* COMMAND1 */
+#define QSPI_CMD1_GO			(1 << 31)
+#define QSPI_CMD1_M_S			(1 << 30)
+#define QSPI_CMD1_MODE_MASK		0x3
+#define QSPI_CMD1_MODE_SHIFT		28
+#define QSPI_CMD1_CS_SEL_MASK		0x3
+#define QSPI_CMD1_CS_SEL_SHIFT		26
+#define QSPI_CMD1_CS_POL_INACTIVE0	(1 << 22)
+#define QSPI_CMD1_CS_SW_HW		(1 << 21)
+#define QSPI_CMD1_CS_SW_VAL		(1 << 20)
+#define QSPI_CMD1_IDLE_SDA_MASK		0x3
+#define QSPI_CMD1_IDLE_SDA_SHIFT	18
+#define QSPI_CMD1_BIDIR			(1 << 17)
+#define QSPI_CMD1_LSBI_FE		(1 << 16)
+#define QSPI_CMD1_LSBY_FE		(1 << 15)
+#define QSPI_CMD1_BOTH_EN_BIT		(1 << 14)
+#define QSPI_CMD1_BOTH_EN_BYTE		(1 << 13)
+#define QSPI_CMD1_RX_EN			(1 << 12)
+#define QSPI_CMD1_TX_EN			(1 << 11)
+#define QSPI_CMD1_PACKED		(1 << 5)
+#define QSPI_CMD1_BITLEN_MASK		0x1F
+#define QSPI_CMD1_BITLEN_SHIFT		0
+
+/* COMMAND2 */
+#define QSPI_CMD2_TX_CLK_TAP_DELAY	(1 << 6)
+#define QSPI_CMD2_TX_CLK_TAP_DELAY_MASK	(0x3F << 6)
+#define QSPI_CMD2_RX_CLK_TAP_DELAY	(1 << 0)
+#define QSPI_CMD2_RX_CLK_TAP_DELAY_MASK	(0x3F << 0)
+
+/* TRANSFER STATUS */
+#define QSPI_XFER_STS_RDY		(1 << 30)
+
+/* FIFO STATUS */
+#define QSPI_FIFO_STS_CS_INACTIVE	(1 << 31)
+#define QSPI_FIFO_STS_FRAME_END		(1 << 30)
+#define QSPI_FIFO_STS_RX_FIFO_FLUSH	(1 << 15)
+#define QSPI_FIFO_STS_TX_FIFO_FLUSH	(1 << 14)
+#define QSPI_FIFO_STS_ERR		(1 << 8)
+#define QSPI_FIFO_STS_TX_FIFO_OVF	(1 << 7)
+#define QSPI_FIFO_STS_TX_FIFO_UNR	(1 << 6)
+#define QSPI_FIFO_STS_RX_FIFO_OVF	(1 << 5)
+#define QSPI_FIFO_STS_RX_FIFO_UNR	(1 << 4)
+#define QSPI_FIFO_STS_TX_FIFO_FULL	(1 << 3)
+#define QSPI_FIFO_STS_TX_FIFO_EMPTY	(1 << 2)
+#define QSPI_FIFO_STS_RX_FIFO_FULL	(1 << 1)
+#define QSPI_FIFO_STS_RX_FIFO_EMPTY	(1 << 0)
+
+#define QSPI_TIMEOUT		1000
+
+struct qspi_regs {
+	u32 command1;	/* 000:QSPI_COMMAND1 register */
+	u32 command2;	/* 004:QSPI_COMMAND2 register */
+	u32 timing1;	/* 008:QSPI_CS_TIM1 register */
+	u32 timing2;	/* 00c:QSPI_CS_TIM2 register */
+	u32 xfer_status;/* 010:QSPI_TRANS_STATUS register */
+	u32 fifo_status;/* 014:QSPI_FIFO_STATUS register */
+	u32 tx_data;	/* 018:QSPI_TX_DATA register */
+	u32 rx_data;	/* 01c:QSPI_RX_DATA register */
+	u32 dma_ctl;	/* 020:QSPI_DMA_CTL register */
+	u32 dma_blk;	/* 024:QSPI_DMA_BLK register */
+	u32 rsvd[56];	/* 028-107 reserved */
+	u32 tx_fifo;	/* 108:QSPI_FIFO1 register */
+	u32 rsvd2[31];	/* 10c-187 reserved */
+	u32 rx_fifo;	/* 188:QSPI_FIFO2 register */
+	u32 spare_ctl;	/* 18c:QSPI_SPARE_CTRL register */
+};
+
+struct tegra210_qspi_priv {
+	struct qspi_regs *regs;
+	unsigned int freq;
+	unsigned int mode;
+	int periph_id;
+	int valid;
+	int last_transaction_us;
+};
+
+static int tegra210_qspi_ofdata_to_platdata(struct udevice *bus)
+{
+	struct tegra_spi_platdata *plat = bus->platdata;
+	const void *blob = gd->fdt_blob;
+	int node = bus->of_offset;
+
+	plat->base = dev_get_addr(bus);
+	plat->periph_id = clock_decode_periph_id(blob, node);
+
+	if (plat->periph_id == PERIPH_ID_NONE) {
+		debug("%s: could not decode periph id %d\n", __func__,
+		      plat->periph_id);
+		return -FDT_ERR_NOTFOUND;
+	}
+
+	/* Use 500KHz as a suitable default */
+	plat->frequency = fdtdec_get_int(blob, node, "spi-max-frequency",
+					500000);
+	plat->deactivate_delay_us = fdtdec_get_int(blob, node,
+					"spi-deactivate-delay", 0);
+	debug("%s: base=%#08lx, periph_id=%d, max-frequency=%d, deactivate_delay=%d\n",
+	      __func__, plat->base, plat->periph_id, plat->frequency,
+	      plat->deactivate_delay_us);
+
+	return 0;
+}
+
+static int tegra210_qspi_probe(struct udevice *bus)
+{
+	struct tegra_spi_platdata *plat = dev_get_platdata(bus);
+	struct tegra210_qspi_priv *priv = dev_get_priv(bus);
+
+	priv->regs = (struct qspi_regs *)plat->base;
+
+	priv->last_transaction_us = timer_get_us();
+	priv->freq = plat->frequency;
+	priv->periph_id = plat->periph_id;
+
+	return 0;
+}
+
+static int tegra210_qspi_claim_bus(struct udevice *bus)
+{
+	struct tegra210_qspi_priv *priv = dev_get_priv(bus);
+	struct qspi_regs *regs = priv->regs;
+
+	/* Change SPI clock to correct frequency, PLLP_OUT0 source */
+	clock_start_periph_pll(priv->periph_id, CLOCK_ID_PERIPH, priv->freq);
+
+	debug("%s: FIFO STATUS = %08x\n", __func__, readl(&regs->fifo_status));
+
+	/* Set master mode and sw controlled CS */
+	setbits_le32(&regs->command1, QSPI_CMD1_M_S | QSPI_CMD1_CS_SW_HW |
+		     (priv->mode << QSPI_CMD1_MODE_SHIFT));
+	debug("%s: COMMAND1 = %08x\n", __func__, readl(&regs->command1));
+
+	return 0;
+}
+
+/**
+ * Activate the CS by driving it LOW
+ *
+ * @param slave	Pointer to spi_slave to which controller has to
+ *		communicate with
+ */
+static void spi_cs_activate(struct udevice *dev)
+{
+	struct udevice *bus = dev->parent;
+	struct tegra_spi_platdata *pdata = dev_get_platdata(bus);
+	struct tegra210_qspi_priv *priv = dev_get_priv(bus);
+
+	/* If it's too soon to do another transaction, wait */
+	if (pdata->deactivate_delay_us &&
+	    priv->last_transaction_us) {
+		ulong delay_us;		/* The delay completed so far */
+		delay_us = timer_get_us() - priv->last_transaction_us;
+		if (delay_us < pdata->deactivate_delay_us)
+			udelay(pdata->deactivate_delay_us - delay_us);
+	}
+
+	clrbits_le32(&priv->regs->command1, QSPI_CMD1_CS_SW_VAL);
+}
+
+/**
+ * Deactivate the CS by driving it HIGH
+ *
+ * @param slave	Pointer to spi_slave to which controller has to
+ *		communicate with
+ */
+static void spi_cs_deactivate(struct udevice *dev)
+{
+	struct udevice *bus = dev->parent;
+	struct tegra_spi_platdata *pdata = dev_get_platdata(bus);
+	struct tegra210_qspi_priv *priv = dev_get_priv(bus);
+
+	setbits_le32(&priv->regs->command1, QSPI_CMD1_CS_SW_VAL);
+
+	/* Remember time of this transaction so we can honour the bus delay */
+	if (pdata->deactivate_delay_us)
+		priv->last_transaction_us = timer_get_us();
+
+	debug("Deactivate CS, bus '%s'\n", bus->name);
+}
+
+static int tegra210_qspi_xfer(struct udevice *dev, unsigned int bitlen,
+			     const void *data_out, void *data_in,
+			     unsigned long flags)
+{
+	struct udevice *bus = dev->parent;
+	struct tegra210_qspi_priv *priv = dev_get_priv(bus);
+	struct qspi_regs *regs = priv->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__, bus->seq, spi_chip_select(dev), dout, din, bitlen);
+	if (bitlen % 8)
+		return -1;
+	num_bytes = bitlen / 8;
+
+	ret = 0;
+
+	/* clear all error status bits */
+	reg = readl(&regs->fifo_status);
+	writel(reg, &regs->fifo_status);
+
+	/* flush RX/TX FIFOs */
+	setbits_le32(&regs->fifo_status,
+		     (QSPI_FIFO_STS_RX_FIFO_FLUSH |
+		      QSPI_FIFO_STS_TX_FIFO_FLUSH));
+	while ((readl(&regs->fifo_status) &
+		      (QSPI_FIFO_STS_RX_FIFO_FLUSH |
+		       QSPI_FIFO_STS_TX_FIFO_FLUSH)))
+		;
+
+	/*
+	 * Notes:
+	 *   1. don't set LSBY_FE, so no need to swap bytes from/to TX/RX FIFOs;
+	 *   2. don't set RX_EN and TX_EN yet.
+	 *      (SW needs to make sure that while programming the blk_size,
+	 *       tx_en and rx_en bits must be zero)
+	 *      [TODO] I (Yen Lin) have problems when both RX/TX EN bits are set
+	 *	       i.e., both dout and din are not NULL.
+	 */
+	clrsetbits_le32(&regs->command1,
+			(QSPI_CMD1_LSBI_FE | QSPI_CMD1_LSBY_FE |
+			 QSPI_CMD1_RX_EN | QSPI_CMD1_TX_EN),
+			(spi_chip_select(dev) << QSPI_CMD1_CS_SEL_SHIFT));
+
+	/* set xfer size to 1 block (32 bits) */
+	writel(0, &regs->dma_blk);
+
+	if (flags & SPI_XFER_BEGIN)
+		spi_cs_activate(dev);
+
+	/* handle data in 32-bit chunks */
+	while (num_bytes > 0) {
+		int bytes;
+		int tm;
+
+		tmpdout = 0;
+		bytes = (num_bytes > 4) ?  4 : num_bytes;
+
+		if (dout != NULL) {
+			memcpy((void *)&tmpdout, (void *)dout, bytes);
+			dout += bytes;
+			num_bytes -= bytes;
+			writel(tmpdout, &regs->tx_fifo);
+			setbits_le32(&regs->command1, QSPI_CMD1_TX_EN);
+		}
+
+		if (din != NULL)
+			setbits_le32(&regs->command1, QSPI_CMD1_RX_EN);
+
+		/* clear ready bit */
+		setbits_le32(&regs->xfer_status, QSPI_XFER_STS_RDY);
+
+		clrsetbits_le32(&regs->command1,
+				QSPI_CMD1_BITLEN_MASK << QSPI_CMD1_BITLEN_SHIFT,
+				(bytes * 8 - 1) << QSPI_CMD1_BITLEN_SHIFT);
+		/* Need to stabilize other reg bit before GO bit set */
+		udelay(2);
+		setbits_le32(&regs->command1, QSPI_CMD1_GO);
+		udelay(1);
+
+		/*
+		 * Wait for SPI transmit FIFO to empty, or to time out.
+		 * The RX FIFO status will be read and cleared last
+		 */
+		for (tm = 0; tm < QSPI_TIMEOUT; ++tm) {
+			u32 fifo_status, xfer_status;
+
+			xfer_status = readl(&regs->xfer_status);
+			if (!(xfer_status & QSPI_XFER_STS_RDY))
+				continue;
+
+			fifo_status = readl(&regs->fifo_status);
+			if (fifo_status & QSPI_FIFO_STS_ERR) {
+				debug("%s: got a fifo error: ", __func__);
+				if (fifo_status & QSPI_FIFO_STS_TX_FIFO_OVF)
+					debug("tx FIFO overflow ");
+				if (fifo_status & QSPI_FIFO_STS_TX_FIFO_UNR)
+					debug("tx FIFO underrun ");
+				if (fifo_status & QSPI_FIFO_STS_RX_FIFO_OVF)
+					debug("rx FIFO overflow ");
+				if (fifo_status & QSPI_FIFO_STS_RX_FIFO_UNR)
+					debug("rx FIFO underrun ");
+				if (fifo_status & QSPI_FIFO_STS_TX_FIFO_FULL)
+					debug("tx FIFO full ");
+				if (fifo_status & QSPI_FIFO_STS_TX_FIFO_EMPTY)
+					debug("tx FIFO empty ");
+				if (fifo_status & QSPI_FIFO_STS_RX_FIFO_FULL)
+					debug("rx FIFO full ");
+				if (fifo_status & QSPI_FIFO_STS_RX_FIFO_EMPTY)
+					debug("rx FIFO empty ");
+				debug("\n");
+				break;
+			}
+
+			if (!(fifo_status & QSPI_FIFO_STS_RX_FIFO_EMPTY)) {
+				tmpdin = readl(&regs->rx_fifo);
+				if (din != NULL) {
+					memcpy(din, &tmpdin, bytes);
+					din += bytes;
+					num_bytes -= bytes;
+				}
+			}
+			break;
+		}
+
+		if (tm >= QSPI_TIMEOUT)
+			ret = tm;
+
+		/* clear ACK RDY, etc. bits */
+		writel(readl(&regs->fifo_status), &regs->fifo_status);
+	}
+
+	if (flags & SPI_XFER_END)
+		spi_cs_deactivate(dev);
+
+	debug("%s: transfer ended. Value=%08x, fifo_status = %08x\n",
+	      __func__, tmpdin, readl(&regs->fifo_status));
+
+	if (ret) {
+		printf("%s: timeout during SPI transfer, tm %d\n",
+		       __func__, ret);
+		return -1;
+	}
+
+	return ret;
+}
+
+static int tegra210_qspi_set_speed(struct udevice *bus, uint speed)
+{
+	struct tegra_spi_platdata *plat = bus->platdata;
+	struct tegra210_qspi_priv *priv = dev_get_priv(bus);
+
+	if (speed > plat->frequency)
+		speed = plat->frequency;
+	priv->freq = speed;
+	debug("%s: regs=%p, speed=%d\n", __func__, priv->regs, priv->freq);
+
+	return 0;
+}
+
+static int tegra210_qspi_set_mode(struct udevice *bus, uint mode)
+{
+	struct tegra210_qspi_priv *priv = dev_get_priv(bus);
+
+	priv->mode = mode;
+	debug("%s: regs=%p, mode=%d\n", __func__, priv->regs, priv->mode);
+
+	return 0;
+}
+
+static const struct dm_spi_ops tegra210_qspi_ops = {
+	.claim_bus	= tegra210_qspi_claim_bus,
+	.xfer		= tegra210_qspi_xfer,
+	.set_speed	= tegra210_qspi_set_speed,
+	.set_mode	= tegra210_qspi_set_mode,
+	/*
+	 * cs_info is not needed, since we require all chip selects to be
+	 * in the device tree explicitly
+	 */
+};
+
+static const struct udevice_id tegra210_qspi_ids[] = {
+	{ .compatible = "nvidia,tegra210-qspi" },
+	{ }
+};
+
+U_BOOT_DRIVER(tegra210_qspi) = {
+	.name = "tegra210-qspi",
+	.id = UCLASS_SPI,
+	.of_match = tegra210_qspi_ids,
+	.ops = &tegra210_qspi_ops,
+	.ofdata_to_platdata = tegra210_qspi_ofdata_to_platdata,
+	.platdata_auto_alloc_size = sizeof(struct tegra_spi_platdata),
+	.priv_auto_alloc_size = sizeof(struct tegra210_qspi_priv),
+	.per_child_auto_alloc_size = sizeof(struct spi_slave),
+	.probe = tegra210_qspi_probe,
+};
-- 
1.8.2.1.610.g562af5b

[U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver

[Stephen Warren]

On 10/20/2015 03:31 PM, Tom Warren wrote:
> This is the normal Tegra SPI driver modified to work with the
> QSPI controller in Tegra210. It does not do 2x/4x transfers
> or any other QSPI protocol.
>
> Author: Yen Lin <yelin@nvidia.com>
> Signed-off-by: Yen Lin <yelin@nvidia.com>
> Signed-off-by: Tom Warren <twarren@nvidia.com>
> ---
> Changes in v2:
> - Drop defconfig and pinmux files, this is a driver-only patch.
> - If/when pinmux tables have been updated for P2371/P2571, another
> - patch will be sent to enable the QSPI driver on those boards.
> Changes in v3:
> - removed status reg write/clear in claim_bus(), done in xfer()

The code looks OK, but we need the binding document written and approved 
before we commit to the driver, I think.

[U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver

[Tom Warren]

Stephen,

-----Original Message-----
From: Stephen Warren [mailto:swarren at wwwdotorg.org] 
Sent: Wednesday, October 21, 2015 9:52 AM
To: Tom Warren <TWarren@nvidia.com>
Cc: u-boot at lists.denx.de; jteki at openedev.com; Stephen Warren <swarren@nvidia.com>; tomcwarren3959 at gmail.com
Subject: Re: [U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver

On 10/20/2015 03:31 PM, Tom Warren wrote:
> This is the normal Tegra SPI driver modified to work with the QSPI 
> controller in Tegra210. It does not do 2x/4x transfers or any other 
> QSPI protocol.
>
> Author: Yen Lin <yelin@nvidia.com>
> Signed-off-by: Yen Lin <yelin@nvidia.com>
> Signed-off-by: Tom Warren <twarren@nvidia.com>
> ---
> Changes in v2:
> - Drop defconfig and pinmux files, this is a driver-only patch.
> - If/when pinmux tables have been updated for P2371/P2571, another
> - patch will be sent to enable the QSPI driver on those boards.
> Changes in v3:
> - removed status reg write/clear in claim_bus(), done in xfer()

The code looks OK, but we need the binding document written and approved before we commit to the driver, I think.
OK. As I asked in the V2 review comments, are you going to write the binding doc, or if not, who do you think should/could do it? I don't know enough to attempt it.

Thanks,

Tom
--
nvpublic

[U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver

[Stephen Warren]

On 10/21/2015 10:58 AM, Tom Warren wrote:
>Stephen Warren wrote atWednesday, October 21, 2015 9:52 AM:
>> On 10/20/2015 03:31 PM, Tom Warren wrote:
>>> This is the normal Tegra SPI driver modified to work with the QSPI
>>> controller in Tegra210. It does not do 2x/4x transfers or any other
>>> QSPI protocol.
>>>
>>> Author: Yen Lin <yelin@nvidia.com>
>>> Signed-off-by: Yen Lin <yelin@nvidia.com>
>>> Signed-off-by: Tom Warren <twarren@nvidia.com>
>>> ---
>>> Changes in v2:
>>> - Drop defconfig and pinmux files, this is a driver-only patch.
>>> - If/when pinmux tables have been updated for P2371/P2571, another
>>> - patch will be sent to enable the QSPI driver on those boards.
>>> Changes in v3:
>>> - removed status reg write/clear in claim_bus(), done in xfer()
>>
>> The code looks OK, but we need the binding document written and approved before we commit to the driver, I think.
 >
> OK. As I asked in the V2 review comments, are you going to write the binding doc, or if not, who do you think should/could do it? I don't know enough to attempt it.

Sorry, I didn't notice that comment in your email, since the quoting 
style wasn't standard (indent with >).

It's the responsibility of the person who implements the driver for the 
device. Writing binding docs is now unfortunately part and parcel of 
almost any driver work.

This device is probably similar enough to the other Tegra SPI 
controllers that you can likely cut/paste most of the doc, and simply 
adjust the names and resource definitions as appropriate for this HW module.

[U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver

[Jagan Teki]

On 21 October 2015 at 03:01, Tom Warren <twarren@nvidia.com> wrote:
> This is the normal Tegra SPI driver modified to work with the
> QSPI controller in Tegra210. It does not do 2x/4x transfers
> or any other QSPI protocol.

Is it totally different controller, can't we re use existing tegra20*
drivers in any way?

>
> Author: Yen Lin <yelin@nvidia.com>

Better to add this in driver license, unless if you have any specific
notation wrt. nVidia.

> Signed-off-by: Yen Lin <yelin@nvidia.com>
> Signed-off-by: Tom Warren <twarren@nvidia.com>
> ---
> Changes in v2:
> - Drop defconfig and pinmux files, this is a driver-only patch.
> - If/when pinmux tables have been updated for P2371/P2571, another
> - patch will be sent to enable the QSPI driver on those boards.
> Changes in v3:
> - removed status reg write/clear in claim_bus(), done in xfer()
>
>  drivers/spi/Kconfig         |   5 +
>  drivers/spi/Makefile        |   1 +
>  drivers/spi/tegra210_qspi.c | 400 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 406 insertions(+)
>  create mode 100644 drivers/spi/tegra210_qspi.c
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 8e04fce..168f31d 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -115,6 +115,11 @@ config TEGRA20_SLINK
>           be used to access the SPI NOR flash on platforms embedding this
>           nVidia Tegra20/Tegra30 IP cores.
>
> +config TEGRA210_QSPI
> +       bool "nVidia Tegra210 QSPI driver"
> +       help
> +         Enable the Tegra Quad-SPI (QSPI) driver for T210.

Add add some more test - at-least 3 lines.

> +
>  config XILINX_SPI
>         bool "Xilinx SPI driver"
>         help
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index de241be..209a41e 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_SH_QSPI) += sh_qspi.o
>  obj-$(CONFIG_TEGRA114_SPI) += tegra114_spi.o
>  obj-$(CONFIG_TEGRA20_SFLASH) += tegra20_sflash.o
>  obj-$(CONFIG_TEGRA20_SLINK) += tegra20_slink.o
> +obj-$(CONFIG_TEGRA210_QSPI) += tegra210_qspi.o
>  obj-$(CONFIG_TI_QSPI) += ti_qspi.o
>  obj-$(CONFIG_XILINX_SPI) += xilinx_spi.o
>  obj-$(CONFIG_ZYNQ_SPI) += zynq_spi.o
> diff --git a/drivers/spi/tegra210_qspi.c b/drivers/spi/tegra210_qspi.c
> new file mode 100644
> index 0000000..6be37f3
> --- /dev/null
> +++ b/drivers/spi/tegra210_qspi.c
> @@ -0,0 +1,400 @@
> +/*
> + * NVIDIA Tegra210 QSPI controller driver

Space here

> + *  (C) Copyright 2015
> + *  NVIDIA Corporation <www.nvidia.com>

Merge last two into one line.

> + *
> + * SPDX-License-Identifier:     GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <asm/io.h>
> +#include <asm/arch/clock.h>
> +#include <asm/arch-tegra/clk_rst.h>
> +#include <spi.h>
> +#include <fdtdec.h>
> +#include "tegra_spi.h"
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/* COMMAND1 */
> +#define QSPI_CMD1_GO                   (1 << 31)
> +#define QSPI_CMD1_M_S                  (1 << 30)
> +#define QSPI_CMD1_MODE_MASK            0x3
> +#define QSPI_CMD1_MODE_SHIFT           28
> +#define QSPI_CMD1_CS_SEL_MASK          0x3
> +#define QSPI_CMD1_CS_SEL_SHIFT         26
> +#define QSPI_CMD1_CS_POL_INACTIVE0     (1 << 22)
> +#define QSPI_CMD1_CS_SW_HW             (1 << 21)
> +#define QSPI_CMD1_CS_SW_VAL            (1 << 20)
> +#define QSPI_CMD1_IDLE_SDA_MASK                0x3
> +#define QSPI_CMD1_IDLE_SDA_SHIFT       18
> +#define QSPI_CMD1_BIDIR                        (1 << 17)
> +#define QSPI_CMD1_LSBI_FE              (1 << 16)
> +#define QSPI_CMD1_LSBY_FE              (1 << 15)
> +#define QSPI_CMD1_BOTH_EN_BIT          (1 << 14)
> +#define QSPI_CMD1_BOTH_EN_BYTE         (1 << 13)
> +#define QSPI_CMD1_RX_EN                        (1 << 12)
> +#define QSPI_CMD1_TX_EN                        (1 << 11)
> +#define QSPI_CMD1_PACKED               (1 << 5)
> +#define QSPI_CMD1_BITLEN_MASK          0x1F
> +#define QSPI_CMD1_BITLEN_SHIFT         0
> +
> +/* COMMAND2 */
> +#define QSPI_CMD2_TX_CLK_TAP_DELAY     (1 << 6)
> +#define QSPI_CMD2_TX_CLK_TAP_DELAY_MASK        (0x3F << 6)
> +#define QSPI_CMD2_RX_CLK_TAP_DELAY     (1 << 0)
> +#define QSPI_CMD2_RX_CLK_TAP_DELAY_MASK        (0x3F << 0)
> +
> +/* TRANSFER STATUS */
> +#define QSPI_XFER_STS_RDY              (1 << 30)
> +
> +/* FIFO STATUS */
> +#define QSPI_FIFO_STS_CS_INACTIVE      (1 << 31)
> +#define QSPI_FIFO_STS_FRAME_END                (1 << 30)
> +#define QSPI_FIFO_STS_RX_FIFO_FLUSH    (1 << 15)
> +#define QSPI_FIFO_STS_TX_FIFO_FLUSH    (1 << 14)
> +#define QSPI_FIFO_STS_ERR              (1 << 8)
> +#define QSPI_FIFO_STS_TX_FIFO_OVF      (1 << 7)
> +#define QSPI_FIFO_STS_TX_FIFO_UNR      (1 << 6)
> +#define QSPI_FIFO_STS_RX_FIFO_OVF      (1 << 5)
> +#define QSPI_FIFO_STS_RX_FIFO_UNR      (1 << 4)
> +#define QSPI_FIFO_STS_TX_FIFO_FULL     (1 << 3)
> +#define QSPI_FIFO_STS_TX_FIFO_EMPTY    (1 << 2)
> +#define QSPI_FIFO_STS_RX_FIFO_FULL     (1 << 1)
> +#define QSPI_FIFO_STS_RX_FIFO_EMPTY    (1 << 0)
> +
> +#define QSPI_TIMEOUT           1000
> +
> +struct qspi_regs {
> +       u32 command1;   /* 000:QSPI_COMMAND1 register */
> +       u32 command2;   /* 004:QSPI_COMMAND2 register */
> +       u32 timing1;    /* 008:QSPI_CS_TIM1 register */
> +       u32 timing2;    /* 00c:QSPI_CS_TIM2 register */
> +       u32 xfer_status;/* 010:QSPI_TRANS_STATUS register */
> +       u32 fifo_status;/* 014:QSPI_FIFO_STATUS register */
> +       u32 tx_data;    /* 018:QSPI_TX_DATA register */
> +       u32 rx_data;    /* 01c:QSPI_RX_DATA register */
> +       u32 dma_ctl;    /* 020:QSPI_DMA_CTL register */
> +       u32 dma_blk;    /* 024:QSPI_DMA_BLK register */
> +       u32 rsvd[56];   /* 028-107 reserved */
> +       u32 tx_fifo;    /* 108:QSPI_FIFO1 register */
> +       u32 rsvd2[31];  /* 10c-187 reserved */
> +       u32 rx_fifo;    /* 188:QSPI_FIFO2 register */
> +       u32 spare_ctl;  /* 18c:QSPI_SPARE_CTRL register */
> +};
> +
> +struct tegra210_qspi_priv {
> +       struct qspi_regs *regs;
> +       unsigned int freq;
> +       unsigned int mode;
> +       int periph_id;
> +       int valid;
> +       int last_transaction_us;
> +};
> +
> +static int tegra210_qspi_ofdata_to_platdata(struct udevice *bus)
> +{
> +       struct tegra_spi_platdata *plat = bus->platdata;
> +       const void *blob = gd->fdt_blob;
> +       int node = bus->of_offset;
> +
> +       plat->base = dev_get_addr(bus);
> +       plat->periph_id = clock_decode_periph_id(blob, node);
> +
> +       if (plat->periph_id == PERIPH_ID_NONE) {
> +               debug("%s: could not decode periph id %d\n", __func__,
> +                     plat->periph_id);
> +               return -FDT_ERR_NOTFOUND;
> +       }
> +
> +       /* Use 500KHz as a suitable default */
> +       plat->frequency = fdtdec_get_int(blob, node, "spi-max-frequency",
> +                                       500000);
> +       plat->deactivate_delay_us = fdtdec_get_int(blob, node,
> +                                       "spi-deactivate-delay", 0);
> +       debug("%s: base=%#08lx, periph_id=%d, max-frequency=%d, deactivate_delay=%d\n",
> +             __func__, plat->base, plat->periph_id, plat->frequency,
> +             plat->deactivate_delay_us);
> +
> +       return 0;
> +}
> +
> +static int tegra210_qspi_probe(struct udevice *bus)
> +{
> +       struct tegra_spi_platdata *plat = dev_get_platdata(bus);
> +       struct tegra210_qspi_priv *priv = dev_get_priv(bus);
> +
> +       priv->regs = (struct qspi_regs *)plat->base;
> +
> +       priv->last_transaction_us = timer_get_us();
> +       priv->freq = plat->frequency;
> +       priv->periph_id = plat->periph_id;
> +
> +       return 0;
> +}
> +
> +static int tegra210_qspi_claim_bus(struct udevice *bus)
> +{
> +       struct tegra210_qspi_priv *priv = dev_get_priv(bus);
> +       struct qspi_regs *regs = priv->regs;
> +
> +       /* Change SPI clock to correct frequency, PLLP_OUT0 source */
> +       clock_start_periph_pll(priv->periph_id, CLOCK_ID_PERIPH, priv->freq);
> +
> +       debug("%s: FIFO STATUS = %08x\n", __func__, readl(&regs->fifo_status));
> +
> +       /* Set master mode and sw controlled CS */
> +       setbits_le32(&regs->command1, QSPI_CMD1_M_S | QSPI_CMD1_CS_SW_HW |
> +                    (priv->mode << QSPI_CMD1_MODE_SHIFT));
> +       debug("%s: COMMAND1 = %08x\n", __func__, readl(&regs->command1));
> +
> +       return 0;
> +}
> +
> +/**
> + * Activate the CS by driving it LOW
> + *
> + * @param slave        Pointer to spi_slave to which controller has to
> + *             communicate with
> + */
> +static void spi_cs_activate(struct udevice *dev)
> +{
> +       struct udevice *bus = dev->parent;
> +       struct tegra_spi_platdata *pdata = dev_get_platdata(bus);
> +       struct tegra210_qspi_priv *priv = dev_get_priv(bus);
> +
> +       /* If it's too soon to do another transaction, wait */
> +       if (pdata->deactivate_delay_us &&
> +           priv->last_transaction_us) {
> +               ulong delay_us;         /* The delay completed so far */
> +               delay_us = timer_get_us() - priv->last_transaction_us;
> +               if (delay_us < pdata->deactivate_delay_us)
> +                       udelay(pdata->deactivate_delay_us - delay_us);
> +       }
> +
> +       clrbits_le32(&priv->regs->command1, QSPI_CMD1_CS_SW_VAL);
> +}
> +
> +/**
> + * Deactivate the CS by driving it HIGH
> + *
> + * @param slave        Pointer to spi_slave to which controller has to
> + *             communicate with
> + */
> +static void spi_cs_deactivate(struct udevice *dev)
> +{
> +       struct udevice *bus = dev->parent;
> +       struct tegra_spi_platdata *pdata = dev_get_platdata(bus);
> +       struct tegra210_qspi_priv *priv = dev_get_priv(bus);
> +
> +       setbits_le32(&priv->regs->command1, QSPI_CMD1_CS_SW_VAL);
> +
> +       /* Remember time of this transaction so we can honour the bus delay */
> +       if (pdata->deactivate_delay_us)
> +               priv->last_transaction_us = timer_get_us();
> +
> +       debug("Deactivate CS, bus '%s'\n", bus->name);
> +}
> +
> +static int tegra210_qspi_xfer(struct udevice *dev, unsigned int bitlen,
> +                            const void *data_out, void *data_in,
> +                            unsigned long flags)
> +{
> +       struct udevice *bus = dev->parent;
> +       struct tegra210_qspi_priv *priv = dev_get_priv(bus);
> +       struct qspi_regs *regs = priv->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__, bus->seq, spi_chip_select(dev), dout, din, bitlen);
> +       if (bitlen % 8)
> +               return -1;
> +       num_bytes = bitlen / 8;
> +
> +       ret = 0;
> +
> +       /* clear all error status bits */
> +       reg = readl(&regs->fifo_status);
> +       writel(reg, &regs->fifo_status);
> +
> +       /* flush RX/TX FIFOs */
> +       setbits_le32(&regs->fifo_status,
> +                    (QSPI_FIFO_STS_RX_FIFO_FLUSH |
> +                     QSPI_FIFO_STS_TX_FIFO_FLUSH));
> +       while ((readl(&regs->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.

> +
> +       /*
> +        * Notes:
> +        *   1. don't set LSBY_FE, so no need to swap bytes from/to TX/RX FIFOs;
> +        *   2. don't set RX_EN and TX_EN yet.
> +        *      (SW needs to make sure that while programming the blk_size,
> +        *       tx_en and rx_en bits must be zero)
> +        *      [TODO] I (Yen Lin) have problems when both RX/TX EN bits are set
> +        *             i.e., both dout and din are not NULL.
> +        */
> +       clrsetbits_le32(&regs->command1,
> +                       (QSPI_CMD1_LSBI_FE | QSPI_CMD1_LSBY_FE |
> +                        QSPI_CMD1_RX_EN | QSPI_CMD1_TX_EN),
> +                       (spi_chip_select(dev) << QSPI_CMD1_CS_SEL_SHIFT));
> +
> +       /* set xfer size to 1 block (32 bits) */
> +       writel(0, &regs->dma_blk);
> +
> +       if (flags & SPI_XFER_BEGIN)
> +               spi_cs_activate(dev);
> +
> +       /* handle data in 32-bit chunks */
> +       while (num_bytes > 0) {
> +               int bytes;
> +               int tm;
> +
> +               tmpdout = 0;
> +               bytes = (num_bytes > 4) ?  4 : num_bytes;
> +
> +               if (dout != NULL) {
> +                       memcpy((void *)&tmpdout, (void *)dout, bytes);
> +                       dout += bytes;
> +                       num_bytes -= bytes;
> +                       writel(tmpdout, &regs->tx_fifo);
> +                       setbits_le32(&regs->command1, QSPI_CMD1_TX_EN);
> +               }
> +
> +               if (din != NULL)
> +                       setbits_le32(&regs->command1, QSPI_CMD1_RX_EN);
> +
> +               /* clear ready bit */
> +               setbits_le32(&regs->xfer_status, QSPI_XFER_STS_RDY);
> +
> +               clrsetbits_le32(&regs->command1,
> +                               QSPI_CMD1_BITLEN_MASK << QSPI_CMD1_BITLEN_SHIFT,
> +                               (bytes * 8 - 1) << QSPI_CMD1_BITLEN_SHIFT);
> +               /* Need to stabilize other reg bit before GO bit set */
> +               udelay(2);
> +               setbits_le32(&regs->command1, QSPI_CMD1_GO);
> +               udelay(1);

Can we do any timeout check's instead of these numerical udelay's.

> +
> +               /*
> +                * Wait for SPI transmit FIFO to empty, or to time out.
> +                * The RX FIFO status will be read and cleared last
> +                */
> +               for (tm = 0; tm < QSPI_TIMEOUT; ++tm) {
> +                       u32 fifo_status, xfer_status;
> +
> +                       xfer_status = readl(&regs->xfer_status);
> +                       if (!(xfer_status & QSPI_XFER_STS_RDY))
> +                               continue;
> +
> +                       fifo_status = readl(&regs->fifo_status);
> +                       if (fifo_status & QSPI_FIFO_STS_ERR) {
> +                               debug("%s: got a fifo error: ", __func__);
> +                               if (fifo_status & QSPI_FIFO_STS_TX_FIFO_OVF)
> +                                       debug("tx FIFO overflow ");
> +                               if (fifo_status & QSPI_FIFO_STS_TX_FIFO_UNR)
> +                                       debug("tx FIFO underrun ");
> +                               if (fifo_status & QSPI_FIFO_STS_RX_FIFO_OVF)
> +                                       debug("rx FIFO overflow ");
> +                               if (fifo_status & QSPI_FIFO_STS_RX_FIFO_UNR)
> +                                       debug("rx FIFO underrun ");
> +                               if (fifo_status & QSPI_FIFO_STS_TX_FIFO_FULL)
> +                                       debug("tx FIFO full ");
> +                               if (fifo_status & QSPI_FIFO_STS_TX_FIFO_EMPTY)
> +                                       debug("tx FIFO empty ");
> +                               if (fifo_status & QSPI_FIFO_STS_RX_FIFO_FULL)
> +                                       debug("rx FIFO full ");
> +                               if (fifo_status & QSPI_FIFO_STS_RX_FIFO_EMPTY)
> +                                       debug("rx FIFO empty ");
> +                               debug("\n");
> +                               break;
> +                       }
> +
> +                       if (!(fifo_status & QSPI_FIFO_STS_RX_FIFO_EMPTY)) {
> +                               tmpdin = readl(&regs->rx_fifo);
> +                               if (din != NULL) {
> +                                       memcpy(din, &tmpdin, bytes);
> +                                       din += bytes;
> +                                       num_bytes -= bytes;
> +                               }
> +                       }
> +                       break;
> +               }
> +
> +               if (tm >= QSPI_TIMEOUT)
> +                       ret = tm;
> +
> +               /* clear ACK RDY, etc. bits */
> +               writel(readl(&regs->fifo_status), &regs->fifo_status);
> +       }

I think this rx logic looks similar to existing tegra20 drivers.

Honestly, this look quite uneasy for me, the code looks fine on the
implementation perspective no comment on that - but what if you
separate the code for  (drivers/spi/zynq_spi.c)
- checking TX fifo empty with timeout and
- then read the rx fifo (timeout couldn't need here)

> +
> +       if (flags & SPI_XFER_END)
> +               spi_cs_deactivate(dev);
> +
> +       debug("%s: transfer ended. Value=%08x, fifo_status = %08x\n",
> +             __func__, tmpdin, readl(&regs->fifo_status));
> +
> +       if (ret) {
> +               printf("%s: timeout during SPI transfer, tm %d\n",
> +                      __func__, ret);
> +               return -1;
> +       }
> +
> +       return ret;
> +}
> +
> +static int tegra210_qspi_set_speed(struct udevice *bus, uint speed)
> +{
> +       struct tegra_spi_platdata *plat = bus->platdata;
> +       struct tegra210_qspi_priv *priv = dev_get_priv(bus);
> +
> +       if (speed > plat->frequency)
> +               speed = plat->frequency;
> +       priv->freq = speed;
> +       debug("%s: regs=%p, speed=%d\n", __func__, priv->regs, priv->freq);
> +
> +       return 0;
> +}
> +
> +static int tegra210_qspi_set_mode(struct udevice *bus, uint mode)
> +{
> +       struct tegra210_qspi_priv *priv = dev_get_priv(bus);
> +
> +       priv->mode = mode;
> +       debug("%s: regs=%p, mode=%d\n", __func__, priv->regs, priv->mode);
> +
> +       return 0;
> +}
> +
> +static const struct dm_spi_ops tegra210_qspi_ops = {
> +       .claim_bus      = tegra210_qspi_claim_bus,
> +       .xfer           = tegra210_qspi_xfer,
> +       .set_speed      = tegra210_qspi_set_speed,
> +       .set_mode       = tegra210_qspi_set_mode,
> +       /*
> +        * cs_info is not needed, since we require all chip selects to be
> +        * in the device tree explicitly
> +        */
> +};
> +
> +static const struct udevice_id tegra210_qspi_ids[] = {
> +       { .compatible = "nvidia,tegra210-qspi" },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(tegra210_qspi) = {
> +       .name = "tegra210-qspi",
> +       .id = UCLASS_SPI,
> +       .of_match = tegra210_qspi_ids,
> +       .ops = &tegra210_qspi_ops,
> +       .ofdata_to_platdata = tegra210_qspi_ofdata_to_platdata,
> +       .platdata_auto_alloc_size = sizeof(struct tegra_spi_platdata),
> +       .priv_auto_alloc_size = sizeof(struct tegra210_qspi_priv),
> +       .per_child_auto_alloc_size = sizeof(struct spi_slave),
> +       .probe = tegra210_qspi_probe,
> +};

-- Jagan.

[U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver

[Tom Warren]

<Added Yen Lin back in CC - don't know how he keeps getting dropped. Yen - please comment where requested below>
Jagan,

> -----Original Message-----
> From: Jagan Teki [mailto:jteki at openedev.com]
> Sent: Thursday, October 22, 2015 5:23 AM
> To: Tom Warren <TWarren@nvidia.com>
> Cc: u-boot at lists.denx.de; Stephen Warren <swarren@nvidia.com>; Tom
> Warren <tomcwarren3959@gmail.com>
> Subject: Re: [U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver
> 
> On 21 October 2015 at 03:01, Tom Warren <twarren@nvidia.com> wrote:
> > This is the normal Tegra SPI driver modified to work with the QSPI
> > controller in Tegra210. It does not do 2x/4x transfers or any other
> > QSPI protocol.
> 
> Is it totally different controller, can't we re use existing tegra20* drivers in any
> way?
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.

> 
> >
> > Author: Yen Lin <yelin@nvidia.com>
> 
> Better to add this in driver license, unless if you have any specific notation wrt.
> nVidia.
> 
> > Signed-off-by: Yen Lin <yelin@nvidia.com>
> > Signed-off-by: Tom Warren <twarren@nvidia.com>
> > ---
> > Changes in v2:
> > - Drop defconfig and pinmux files, this is a driver-only patch.
> > - If/when pinmux tables have been updated for P2371/P2571, another
> > - patch will be sent to enable the QSPI driver on those boards.
> > Changes in v3:
> > - removed status reg write/clear in claim_bus(), done in xfer()
> >
> >  drivers/spi/Kconfig         |   5 +
> >  drivers/spi/Makefile        |   1 +
> >  drivers/spi/tegra210_qspi.c | 400
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 406 insertions(+)
> >  create mode 100644 drivers/spi/tegra210_qspi.c
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index
> > 8e04fce..168f31d 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -115,6 +115,11 @@ config TEGRA20_SLINK
> >           be used to access the SPI NOR flash on platforms embedding this
> >           nVidia Tegra20/Tegra30 IP cores.
> >
> > +config TEGRA210_QSPI
> > +       bool "nVidia Tegra210 QSPI driver"
> > +       help
> > +         Enable the Tegra Quad-SPI (QSPI) driver for T210.
> 
> Add add some more test - at-least 3 lines.
More test? Do you mean more 'text'? Will do.

> 
> > +
> >  config XILINX_SPI
> >         bool "Xilinx SPI driver"
> >         help
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile index
> > de241be..209a41e 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -46,6 +46,7 @@ obj-$(CONFIG_SH_QSPI) += sh_qspi.o
> >  obj-$(CONFIG_TEGRA114_SPI) += tegra114_spi.o
> >  obj-$(CONFIG_TEGRA20_SFLASH) += tegra20_sflash.o
> >  obj-$(CONFIG_TEGRA20_SLINK) += tegra20_slink.o
> > +obj-$(CONFIG_TEGRA210_QSPI) += tegra210_qspi.o
> >  obj-$(CONFIG_TI_QSPI) += ti_qspi.o
> >  obj-$(CONFIG_XILINX_SPI) += xilinx_spi.o
> >  obj-$(CONFIG_ZYNQ_SPI) += zynq_spi.o
> > diff --git a/drivers/spi/tegra210_qspi.c b/drivers/spi/tegra210_qspi.c
> > new file mode 100644 index 0000000..6be37f3
> > --- /dev/null
> > +++ b/drivers/spi/tegra210_qspi.c
> > @@ -0,0 +1,400 @@
> > +/*
> > + * NVIDIA Tegra210 QSPI controller driver
> 
> Space here
> 
> > + *  (C) Copyright 2015
> > + *  NVIDIA Corporation <www.nvidia.com>
> 
> Merge last two into one line.
Will do.

> 
> > + *
> > + * SPDX-License-Identifier:     GPL-2.0+
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <asm/io.h>
> > +#include <asm/arch/clock.h>
> > +#include <asm/arch-tegra/clk_rst.h>
> > +#include <spi.h>
> > +#include <fdtdec.h>
> > +#include "tegra_spi.h"
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +/* COMMAND1 */
> > +#define QSPI_CMD1_GO                   (1 << 31)
> > +#define QSPI_CMD1_M_S                  (1 << 30)
> > +#define QSPI_CMD1_MODE_MASK            0x3
> > +#define QSPI_CMD1_MODE_SHIFT           28
> > +#define QSPI_CMD1_CS_SEL_MASK          0x3
> > +#define QSPI_CMD1_CS_SEL_SHIFT         26
> > +#define QSPI_CMD1_CS_POL_INACTIVE0     (1 << 22)
> > +#define QSPI_CMD1_CS_SW_HW             (1 << 21)
> > +#define QSPI_CMD1_CS_SW_VAL            (1 << 20)
> > +#define QSPI_CMD1_IDLE_SDA_MASK                0x3
> > +#define QSPI_CMD1_IDLE_SDA_SHIFT       18
> > +#define QSPI_CMD1_BIDIR                        (1 << 17)
> > +#define QSPI_CMD1_LSBI_FE              (1 << 16)
> > +#define QSPI_CMD1_LSBY_FE              (1 << 15)
> > +#define QSPI_CMD1_BOTH_EN_BIT          (1 << 14)
> > +#define QSPI_CMD1_BOTH_EN_BYTE         (1 << 13)
> > +#define QSPI_CMD1_RX_EN                        (1 << 12)
> > +#define QSPI_CMD1_TX_EN                        (1 << 11)
> > +#define QSPI_CMD1_PACKED               (1 << 5)
> > +#define QSPI_CMD1_BITLEN_MASK          0x1F
> > +#define QSPI_CMD1_BITLEN_SHIFT         0
> > +
> > +/* COMMAND2 */
> > +#define QSPI_CMD2_TX_CLK_TAP_DELAY     (1 << 6)
> > +#define QSPI_CMD2_TX_CLK_TAP_DELAY_MASK        (0x3F << 6)
> > +#define QSPI_CMD2_RX_CLK_TAP_DELAY     (1 << 0)
> > +#define QSPI_CMD2_RX_CLK_TAP_DELAY_MASK        (0x3F << 0)
> > +
> > +/* TRANSFER STATUS */
> > +#define QSPI_XFER_STS_RDY              (1 << 30)
> > +
> > +/* FIFO STATUS */
> > +#define QSPI_FIFO_STS_CS_INACTIVE      (1 << 31)
> > +#define QSPI_FIFO_STS_FRAME_END                (1 << 30)
> > +#define QSPI_FIFO_STS_RX_FIFO_FLUSH    (1 << 15)
> > +#define QSPI_FIFO_STS_TX_FIFO_FLUSH    (1 << 14)
> > +#define QSPI_FIFO_STS_ERR              (1 << 8)
> > +#define QSPI_FIFO_STS_TX_FIFO_OVF      (1 << 7)
> > +#define QSPI_FIFO_STS_TX_FIFO_UNR      (1 << 6)
> > +#define QSPI_FIFO_STS_RX_FIFO_OVF      (1 << 5)
> > +#define QSPI_FIFO_STS_RX_FIFO_UNR      (1 << 4)
> > +#define QSPI_FIFO_STS_TX_FIFO_FULL     (1 << 3)
> > +#define QSPI_FIFO_STS_TX_FIFO_EMPTY    (1 << 2)
> > +#define QSPI_FIFO_STS_RX_FIFO_FULL     (1 << 1)
> > +#define QSPI_FIFO_STS_RX_FIFO_EMPTY    (1 << 0)
> > +
> > +#define QSPI_TIMEOUT           1000
> > +
> > +struct qspi_regs {
> > +       u32 command1;   /* 000:QSPI_COMMAND1 register */
> > +       u32 command2;   /* 004:QSPI_COMMAND2 register */
> > +       u32 timing1;    /* 008:QSPI_CS_TIM1 register */
> > +       u32 timing2;    /* 00c:QSPI_CS_TIM2 register */
> > +       u32 xfer_status;/* 010:QSPI_TRANS_STATUS register */
> > +       u32 fifo_status;/* 014:QSPI_FIFO_STATUS register */
> > +       u32 tx_data;    /* 018:QSPI_TX_DATA register */
> > +       u32 rx_data;    /* 01c:QSPI_RX_DATA register */
> > +       u32 dma_ctl;    /* 020:QSPI_DMA_CTL register */
> > +       u32 dma_blk;    /* 024:QSPI_DMA_BLK register */
> > +       u32 rsvd[56];   /* 028-107 reserved */
> > +       u32 tx_fifo;    /* 108:QSPI_FIFO1 register */
> > +       u32 rsvd2[31];  /* 10c-187 reserved */
> > +       u32 rx_fifo;    /* 188:QSPI_FIFO2 register */
> > +       u32 spare_ctl;  /* 18c:QSPI_SPARE_CTRL register */ };
> > +
> > +struct tegra210_qspi_priv {
> > +       struct qspi_regs *regs;
> > +       unsigned int freq;
> > +       unsigned int mode;
> > +       int periph_id;
> > +       int valid;
> > +       int last_transaction_us;
> > +};
> > +
> > +static int tegra210_qspi_ofdata_to_platdata(struct udevice *bus) {
> > +       struct tegra_spi_platdata *plat = bus->platdata;
> > +       const void *blob = gd->fdt_blob;
> > +       int node = bus->of_offset;
> > +
> > +       plat->base = dev_get_addr(bus);
> > +       plat->periph_id = clock_decode_periph_id(blob, node);
> > +
> > +       if (plat->periph_id == PERIPH_ID_NONE) {
> > +               debug("%s: could not decode periph id %d\n", __func__,
> > +                     plat->periph_id);
> > +               return -FDT_ERR_NOTFOUND;
> > +       }
> > +
> > +       /* Use 500KHz as a suitable default */
> > +       plat->frequency = fdtdec_get_int(blob, node, "spi-max-frequency",
> > +                                       500000);
> > +       plat->deactivate_delay_us = fdtdec_get_int(blob, node,
> > +                                       "spi-deactivate-delay", 0);
> > +       debug("%s: base=%#08lx, periph_id=%d, max-frequency=%d,
> deactivate_delay=%d\n",
> > +             __func__, plat->base, plat->periph_id, plat->frequency,
> > +             plat->deactivate_delay_us);
> > +
> > +       return 0;
> > +}
> > +
> > +static int tegra210_qspi_probe(struct udevice *bus) {
> > +       struct tegra_spi_platdata *plat = dev_get_platdata(bus);
> > +       struct tegra210_qspi_priv *priv = dev_get_priv(bus);
> > +
> > +       priv->regs = (struct qspi_regs *)plat->base;
> > +
> > +       priv->last_transaction_us = timer_get_us();
> > +       priv->freq = plat->frequency;
> > +       priv->periph_id = plat->periph_id;
> > +
> > +       return 0;
> > +}
> > +
> > +static int tegra210_qspi_claim_bus(struct udevice *bus) {
> > +       struct tegra210_qspi_priv *priv = dev_get_priv(bus);
> > +       struct qspi_regs *regs = priv->regs;
> > +
> > +       /* Change SPI clock to correct frequency, PLLP_OUT0 source */
> > +       clock_start_periph_pll(priv->periph_id, CLOCK_ID_PERIPH,
> > + priv->freq);
> > +
> > +       debug("%s: FIFO STATUS = %08x\n", __func__,
> > + readl(&regs->fifo_status));
> > +
> > +       /* Set master mode and sw controlled CS */
> > +       setbits_le32(&regs->command1, QSPI_CMD1_M_S |
> QSPI_CMD1_CS_SW_HW |
> > +                    (priv->mode << QSPI_CMD1_MODE_SHIFT));
> > +       debug("%s: COMMAND1 = %08x\n", __func__,
> > + readl(&regs->command1));
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * Activate the CS by driving it LOW
> > + *
> > + * @param slave        Pointer to spi_slave to which controller has to
> > + *             communicate with
> > + */
> > +static void spi_cs_activate(struct udevice *dev) {
> > +       struct udevice *bus = dev->parent;
> > +       struct tegra_spi_platdata *pdata = dev_get_platdata(bus);
> > +       struct tegra210_qspi_priv *priv = dev_get_priv(bus);
> > +
> > +       /* If it's too soon to do another transaction, wait */
> > +       if (pdata->deactivate_delay_us &&
> > +           priv->last_transaction_us) {
> > +               ulong delay_us;         /* The delay completed so far */
> > +               delay_us = timer_get_us() - priv->last_transaction_us;
> > +               if (delay_us < pdata->deactivate_delay_us)
> > +                       udelay(pdata->deactivate_delay_us - delay_us);
> > +       }
> > +
> > +       clrbits_le32(&priv->regs->command1, QSPI_CMD1_CS_SW_VAL); }
> > +
> > +/**
> > + * Deactivate the CS by driving it HIGH
> > + *
> > + * @param slave        Pointer to spi_slave to which controller has to
> > + *             communicate with
> > + */
> > +static void spi_cs_deactivate(struct udevice *dev) {
> > +       struct udevice *bus = dev->parent;
> > +       struct tegra_spi_platdata *pdata = dev_get_platdata(bus);
> > +       struct tegra210_qspi_priv *priv = dev_get_priv(bus);
> > +
> > +       setbits_le32(&priv->regs->command1, QSPI_CMD1_CS_SW_VAL);
> > +
> > +       /* Remember time of this transaction so we can honour the bus delay
> */
> > +       if (pdata->deactivate_delay_us)
> > +               priv->last_transaction_us = timer_get_us();
> > +
> > +       debug("Deactivate CS, bus '%s'\n", bus->name); }
> > +
> > +static int tegra210_qspi_xfer(struct udevice *dev, unsigned int bitlen,
> > +                            const void *data_out, void *data_in,
> > +                            unsigned long flags) {
> > +       struct udevice *bus = dev->parent;
> > +       struct tegra210_qspi_priv *priv = dev_get_priv(bus);
> > +       struct qspi_regs *regs = priv->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__, bus->seq, spi_chip_select(dev), dout, din, bitlen);
> > +       if (bitlen % 8)
> > +               return -1;
> > +       num_bytes = bitlen / 8;
> > +
> > +       ret = 0;
> > +
> > +       /* clear all error status bits */
> > +       reg = readl(&regs->fifo_status);
> > +       writel(reg, &regs->fifo_status);
> > +
> > +       /* flush RX/TX FIFOs */
> > +       setbits_le32(&regs->fifo_status,
> > +                    (QSPI_FIFO_STS_RX_FIFO_FLUSH |
> > +                     QSPI_FIFO_STS_TX_FIFO_FLUSH));
> > +       while ((readl(&regs->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.

> 
> > +
> > +       /*
> > +        * Notes:
> > +        *   1. don't set LSBY_FE, so no need to swap bytes from/to TX/RX FIFOs;
> > +        *   2. don't set RX_EN and TX_EN yet.
> > +        *      (SW needs to make sure that while programming the blk_size,
> > +        *       tx_en and rx_en bits must be zero)
> > +        *      [TODO] I (Yen Lin) have problems when both RX/TX EN bits are set
> > +        *             i.e., both dout and din are not NULL.
> > +        */
> > +       clrsetbits_le32(&regs->command1,
> > +                       (QSPI_CMD1_LSBI_FE | QSPI_CMD1_LSBY_FE |
> > +                        QSPI_CMD1_RX_EN | QSPI_CMD1_TX_EN),
> > +                       (spi_chip_select(dev) <<
> > + QSPI_CMD1_CS_SEL_SHIFT));
> > +
> > +       /* set xfer size to 1 block (32 bits) */
> > +       writel(0, &regs->dma_blk);
> > +
> > +       if (flags & SPI_XFER_BEGIN)
> > +               spi_cs_activate(dev);
> > +
> > +       /* handle data in 32-bit chunks */
> > +       while (num_bytes > 0) {
> > +               int bytes;
> > +               int tm;
> > +
> > +               tmpdout = 0;
> > +               bytes = (num_bytes > 4) ?  4 : num_bytes;
> > +
> > +               if (dout != NULL) {
> > +                       memcpy((void *)&tmpdout, (void *)dout, bytes);
> > +                       dout += bytes;
> > +                       num_bytes -= bytes;
> > +                       writel(tmpdout, &regs->tx_fifo);
> > +                       setbits_le32(&regs->command1, QSPI_CMD1_TX_EN);
> > +               }
> > +
> > +               if (din != NULL)
> > +                       setbits_le32(&regs->command1,
> > + QSPI_CMD1_RX_EN);
> > +
> > +               /* clear ready bit */
> > +               setbits_le32(&regs->xfer_status, QSPI_XFER_STS_RDY);
> > +
> > +               clrsetbits_le32(&regs->command1,
> > +                               QSPI_CMD1_BITLEN_MASK <<
> QSPI_CMD1_BITLEN_SHIFT,
> > +                               (bytes * 8 - 1) << QSPI_CMD1_BITLEN_SHIFT);
> > +               /* Need to stabilize other reg bit before GO bit set */
> > +               udelay(2);
> > +               setbits_le32(&regs->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.

> 
> > +
> > +               /*
> > +                * Wait for SPI transmit FIFO to empty, or to time out.
> > +                * The RX FIFO status will be read and cleared last
> > +                */
> > +               for (tm = 0; tm < QSPI_TIMEOUT; ++tm) {
> > +                       u32 fifo_status, xfer_status;
> > +
> > +                       xfer_status = readl(&regs->xfer_status);
> > +                       if (!(xfer_status & QSPI_XFER_STS_RDY))
> > +                               continue;
> > +
> > +                       fifo_status = readl(&regs->fifo_status);
> > +                       if (fifo_status & QSPI_FIFO_STS_ERR) {
> > +                               debug("%s: got a fifo error: ", __func__);
> > +                               if (fifo_status & QSPI_FIFO_STS_TX_FIFO_OVF)
> > +                                       debug("tx FIFO overflow ");
> > +                               if (fifo_status & QSPI_FIFO_STS_TX_FIFO_UNR)
> > +                                       debug("tx FIFO underrun ");
> > +                               if (fifo_status & QSPI_FIFO_STS_RX_FIFO_OVF)
> > +                                       debug("rx FIFO overflow ");
> > +                               if (fifo_status & QSPI_FIFO_STS_RX_FIFO_UNR)
> > +                                       debug("rx FIFO underrun ");
> > +                               if (fifo_status & QSPI_FIFO_STS_TX_FIFO_FULL)
> > +                                       debug("tx FIFO full ");
> > +                               if (fifo_status & QSPI_FIFO_STS_TX_FIFO_EMPTY)
> > +                                       debug("tx FIFO empty ");
> > +                               if (fifo_status & QSPI_FIFO_STS_RX_FIFO_FULL)
> > +                                       debug("rx FIFO full ");
> > +                               if (fifo_status & QSPI_FIFO_STS_RX_FIFO_EMPTY)
> > +                                       debug("rx FIFO empty ");
> > +                               debug("\n");
> > +                               break;
> > +                       }
> > +
> > +                       if (!(fifo_status & QSPI_FIFO_STS_RX_FIFO_EMPTY)) {
> > +                               tmpdin = readl(&regs->rx_fifo);
> > +                               if (din != NULL) {
> > +                                       memcpy(din, &tmpdin, bytes);
> > +                                       din += bytes;
> > +                                       num_bytes -= bytes;
> > +                               }
> > +                       }
> > +                       break;
> > +               }
> > +
> > +               if (tm >= QSPI_TIMEOUT)
> > +                       ret = tm;
> > +
> > +               /* clear ACK RDY, etc. bits */
> > +               writel(readl(&regs->fifo_status), &regs->fifo_status);
> > +       }
> 
> I think this rx logic looks similar to existing tegra20 drivers.
Yes, because it was ported from them. ;)

> 
> Honestly, this look quite uneasy for me, the code looks fine on the
> implementation perspective no comment on that - but what if you separate the
> code for  (drivers/spi/zynq_spi.c)
> - checking TX fifo empty with timeout and
> - then read the rx fifo (timeout couldn't need here)
I don't know that we want to rearchitect this driver after it's been working this way in the field for so long. This is just a port of a working SPI driver to QSPI, and is needed by a downstream customer fairly urgently. I don't have the means to rewrite and retest this on all of the HW it's needed on, nor does Yen, AFAIK.  Unless you feel really strongly about this change, I'll leave it as-is.

> 
> > +
> > +       if (flags & SPI_XFER_END)
> > +               spi_cs_deactivate(dev);
> > +
> > +       debug("%s: transfer ended. Value=%08x, fifo_status = %08x\n",
> > +             __func__, tmpdin, readl(&regs->fifo_status));
> > +
> > +       if (ret) {
> > +               printf("%s: timeout during SPI transfer, tm %d\n",
> > +                      __func__, ret);
> > +               return -1;
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> > +static int tegra210_qspi_set_speed(struct udevice *bus, uint speed) {
> > +       struct tegra_spi_platdata *plat = bus->platdata;
> > +       struct tegra210_qspi_priv *priv = dev_get_priv(bus);
> > +
> > +       if (speed > plat->frequency)
> > +               speed = plat->frequency;
> > +       priv->freq = speed;
> > +       debug("%s: regs=%p, speed=%d\n", __func__, priv->regs,
> > + priv->freq);
> > +
> > +       return 0;
> > +}
> > +
> > +static int tegra210_qspi_set_mode(struct udevice *bus, uint mode) {
> > +       struct tegra210_qspi_priv *priv = dev_get_priv(bus);
> > +
> > +       priv->mode = mode;
> > +       debug("%s: regs=%p, mode=%d\n", __func__, priv->regs,
> > + priv->mode);
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dm_spi_ops tegra210_qspi_ops = {
> > +       .claim_bus      = tegra210_qspi_claim_bus,
> > +       .xfer           = tegra210_qspi_xfer,
> > +       .set_speed      = tegra210_qspi_set_speed,
> > +       .set_mode       = tegra210_qspi_set_mode,
> > +       /*
> > +        * cs_info is not needed, since we require all chip selects to be
> > +        * in the device tree explicitly
> > +        */
> > +};
> > +
> > +static const struct udevice_id tegra210_qspi_ids[] = {
> > +       { .compatible = "nvidia,tegra210-qspi" },
> > +       { }
> > +};
> > +
> > +U_BOOT_DRIVER(tegra210_qspi) = {
> > +       .name = "tegra210-qspi",
> > +       .id = UCLASS_SPI,
> > +       .of_match = tegra210_qspi_ids,
> > +       .ops = &tegra210_qspi_ops,
> > +       .ofdata_to_platdata = tegra210_qspi_ofdata_to_platdata,
> > +       .platdata_auto_alloc_size = sizeof(struct tegra_spi_platdata),
> > +       .priv_auto_alloc_size = sizeof(struct tegra210_qspi_priv),
> > +       .per_child_auto_alloc_size = sizeof(struct spi_slave),
> > +       .probe = tegra210_qspi_probe,
> > +};
> 
> -- Jagan.
Thanks for the thorough review, Jagan!

Tom
--
nvpublic

[U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver

[Yen Lin]

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(&regs->fifo_status);
>  > > +       writel(reg, &regs->fifo_status);
>  > > +
>  > > +       /* flush RX/TX FIFOs */
>  > > +       setbits_le32(&regs->fifo_status,
>  > > +                    (QSPI_FIFO_STS_RX_FIFO_FLUSH |
>  > > +                     QSPI_FIFO_STS_TX_FIFO_FLUSH));
>  > > +       while ((readl(&regs->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.
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(&regs->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.


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

[U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver

[Jagan Teki]

Hi Yen,

On 23 October 2015 at 04:18, Yen Lin <yelin@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(&regs->fifo_status);
>>  > > +       writel(reg, &regs->fifo_status);
>>  > > +
>>  > > +       /* flush RX/TX FIFOs */
>>  > > +       setbits_le32(&regs->fifo_status,
>>  > > +                    (QSPI_FIFO_STS_RX_FIFO_FLUSH |
>>  > > +                     QSPI_FIFO_STS_TX_FIFO_FLUSH));
>>  > > +       while ((readl(&regs->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(&regs->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.

thanks!
-- 
Jagan | openedev.

[U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver

[Tom Warren]

Jagan,

> -----Original Message-----
> From: Jagan Teki [mailto:jteki at openedev.com]
> Sent: Saturday, October 24, 2015 8:15 PM
> To: Yen Lin <yelin@nvidia.com>
> Cc: Tom Warren <TWarren@nvidia.com>; u-boot at lists.denx.de; Stephen
> Warren <swarren@nvidia.com>; Tom Warren <tomcwarren3959@gmail.com>
> Subject: Re: [U-Boot] [PATCH v3] Tegra: T210: Add QSPI driver
> 
> Hi Yen,
> 
> On 23 October 2015 at 04:18, Yen Lin <yelin@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(&regs->fifo_status);
> >>  > > +       writel(reg, &regs->fifo_status);
> >>  > > +
> >>  > > +       /* flush RX/TX FIFOs */
> >>  > > +       setbits_le32(&regs->fifo_status,
> >>  > > +                    (QSPI_FIFO_STS_RX_FIFO_FLUSH |
> >>  > > +                     QSPI_FIFO_STS_TX_FIFO_FLUSH));
> >>  > > +       while ((readl(&regs->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(&regs->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.