[PATCHv6 0/2] Add ti qspi controller

[PATCHv6 0/2] Add ti qspi controller

[Sourav Poddar]

This patch series add support for ti qspi controller.
Adapted this series on top of Mark brown series[1]:

[1]: https://patchwork.kernel.org/patch/2834694/

And some other cleanups.

Sourav Poddar (2):
  drivers: spi: Add qspi flash controller
  driver: spi: Add quad spi read support

 Documentation/devicetree/bindings/spi/ti_qspi.txt |   22 +
 drivers/spi/Kconfig                               |    8 +
 drivers/spi/Makefile                              |    1 +
 drivers/spi/spi-ti-qspi.c                         |  532 +++++++++++++++++++++
 4 files changed, 563 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/ti_qspi.txt
 create mode 100644 drivers/spi/spi-ti-qspi.c

[PATCHv6 1/2] drivers: spi: Add qspi flash controller

[Sourav Poddar]

The patch add basic support for the quad spi controller.

QSPI is a kind of spi module that allows single,
dual and quad read access to external spi devices. The module
has a memory mapped interface which provide direct interface
for accessing data form external spi devices.

The patch will configure controller clocks, device control
register and for defining low level transfer apis which
will be used by the spi framework to transfer data to
the slave spi device(flash in this case).

Test details:
-------------
Tested this on dra7 board.
Test1: Ran mtd_stesstest for 40000 iterations.
   - All iterations went through without failure.
Test2: Use mtd utilities:
  - flash_erase to erase the flash device
  - nanddump to read data back.
  - nandwrite to write to the data flash.
 diff between the write and read data shows zero.
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
v5->v6
--------
- Modified read/write api
- Adapt the code to Mark Brown series of moving
runtime PM apis to core.
- Miscellaneous cleanup.

 Documentation/devicetree/bindings/spi/ti_qspi.txt |   22 +
 drivers/spi/Kconfig                               |    8 +
 drivers/spi/Makefile                              |    1 +
 drivers/spi/spi-ti-qspi.c                         |  520 +++++++++++++++++++++
 4 files changed, 551 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/ti_qspi.txt
 create mode 100644 drivers/spi/spi-ti-qspi.c

diff --git a/Documentation/devicetree/bindings/spi/ti_qspi.txt b/Documentation/devicetree/bindings/spi/ti_qspi.txt
new file mode 100644
index 0000000..398ef59
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/ti_qspi.txt
@@ -0,0 +1,22 @@
+TI QSPI controller.
+
+Required properties:
+- compatible : should be "ti,dra7xxx-qspi".
+- reg: Should contain QSPI registers location and length.
+- #address-cells, #size-cells : Must be present if the device has sub-nodes
+- ti,hwmods: Name of the hwmod associated to the QSPI
+
+Recommended properties:
+- spi-max-frequency: Definition as per
+                     Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Example:
+
+qspi: qspi@4b300000 {
+	compatible = "ti,dra7xxx-qspi";
+	reg = <0x4b300000 0x100>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	spi-max-frequency = <25000000>;
+	ti,hwmods = "qspi";
+};
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 92a9345..1c4e758 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -285,6 +285,14 @@ config SPI_OMAP24XX
 	  SPI master controller for OMAP24XX and later Multichannel SPI
 	  (McSPI) modules.
 
+config SPI_TI_QSPI
+	tristate "DRA7xxx QSPI controller support"
+	depends on ARCH_OMAP2PLUS || COMPILE_TEST
+	help
+	  QSPI master controller for DRA7xxx used for flash devices.
+	  This device supports single, dual and quad read support, while
+	  it only supports single write mode.
+
 config SPI_OMAP_100K
 	tristate "OMAP SPI 100K"
 	depends on ARCH_OMAP850 || ARCH_OMAP730
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 33f9c09..a174030 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_SPI_OCTEON)		+= spi-octeon.o
 obj-$(CONFIG_SPI_OMAP_UWIRE)		+= spi-omap-uwire.o
 obj-$(CONFIG_SPI_OMAP_100K)		+= spi-omap-100k.o
 obj-$(CONFIG_SPI_OMAP24XX)		+= spi-omap2-mcspi.o
+obj-$(CONFIG_SPI_TI_QSPI)		+= spi-ti-qspi.o
 obj-$(CONFIG_SPI_ORION)			+= spi-orion.o
 obj-$(CONFIG_SPI_PL022)			+= spi-pl022.o
 obj-$(CONFIG_SPI_PPC4xx)		+= spi-ppc4xx.o
diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
new file mode 100644
index 0000000..51fe95f
--- /dev/null
+++ b/drivers/spi/spi-ti-qspi.c
@@ -0,0 +1,520 @@
+/*
+ * TI QSPI driver
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
+ * Author: Sourav Poddar <sourav.poddar@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GPLv2.
+ *
+ * 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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/omap-dma.h>
+#include <linux/platform_device.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/pm_runtime.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/pinctrl/consumer.h>
+
+#include <linux/spi/spi.h>
+
+struct ti_qspi_regs {
+	u32 clkctrl;
+};
+
+struct ti_qspi {
+	spinlock_t              lock;           /* IRQ synchronization */
+	struct spi_master	*master;
+	void __iomem            *base;
+	struct device           *dev;
+	struct completion       transfer_complete;
+	struct clk *fclk;
+	struct ti_qspi_regs     ctx_reg;
+	int device_type;
+	u32 spi_max_frequency;
+	u32 cmd;
+	u32 dc;
+};
+
+#define QSPI_PID			(0x0)
+#define QSPI_SYSCONFIG			(0x10)
+#define QSPI_INTR_STATUS_RAW_SET	(0x20)
+#define QSPI_INTR_STATUS_ENABLED_CLEAR	(0x24)
+#define QSPI_INTR_ENABLE_SET_REG	(0x28)
+#define QSPI_INTR_ENABLE_CLEAR_REG	(0x2c)
+#define QSPI_SPI_CLOCK_CNTRL_REG	(0x40)
+#define QSPI_SPI_DC_REG			(0x44)
+#define QSPI_SPI_CMD_REG		(0x48)
+#define QSPI_SPI_STATUS_REG		(0x4c)
+#define QSPI_SPI_DATA_REG		(0x50)
+#define QSPI_SPI_SETUP0_REG		(0x54)
+#define QSPI_SPI_SWITCH_REG		(0x64)
+#define QSPI_SPI_SETUP1_REG		(0x58)
+#define QSPI_SPI_SETUP2_REG		(0x5c)
+#define QSPI_SPI_SETUP3_REG		(0x60)
+#define QSPI_SPI_DATA_REG_1		(0x68)
+#define QSPI_SPI_DATA_REG_2		(0x6c)
+#define QSPI_SPI_DATA_REG_3		(0x70)
+
+#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_RD_QUAD			(7 << 16)
+#define QSPI_INVAL			(4 << 16)
+#define QSPI_WC_CMD_INT_EN			(1 << 14)
+#define QSPI_FLEN(n)			((n-1) << 0)
+
+/* INTERRUPT REGISTER */
+#define QSPI_WC_INT_EN				(1 << 1)
+#define QSPI_WC_INT_DISABLE			(1 << 1)
+
+/* 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	QSPI_FRAME_MAX			0xfff
+
+#define SPI_AUTOSUSPEND_TIMEOUT         2000
+
+static inline unsigned long ti_qspi_read(struct ti_qspi *qspi,
+		unsigned long reg)
+{
+	return readl(qspi->base + reg);
+}
+
+static inline void ti_qspi_write(struct ti_qspi *qspi,
+		unsigned long val, unsigned long reg)
+{
+	writel(val, qspi->base + reg);
+}
+
+static inline void ti_qspi_read_data(struct ti_qspi *qspi,
+		unsigned long reg, int wlen, u8 **rxbuf)
+{
+	switch (wlen) {
+	case 8:
+		**rxbuf = readb(qspi->base + reg);
+		dev_dbg(qspi->dev, "rx done, read %02x\n", *(*rxbuf-1));
+		*rxbuf += 1;
+		break;
+	case 16:
+		**rxbuf = readw(qspi->base + reg);
+		dev_dbg(qspi->dev, "rx done, read %02x\n", *(*rxbuf-1));
+		*rxbuf += 2;
+		break;
+	case 32:
+		**rxbuf = readl(qspi->base + reg);
+		dev_dbg(qspi->dev, "rx done, read %02x\n", *(*rxbuf-1));
+		*rxbuf += 4;
+	default:
+		dev_dbg(qspi->dev, "word lenght out of range");
+		break;
+	}
+}
+
+static inline void ti_qspi_write_data(struct ti_qspi *qspi, unsigned long val,
+			unsigned long reg, int wlen, const u8 **txbuf)
+{
+	switch (wlen) {
+	case 8:
+		writeb(val, qspi->base + reg);
+		*txbuf += 1;
+		break;
+	case 16:
+		writew(val, qspi->base + reg);
+		*txbuf += 2;
+		break;
+	case 32:
+		writel(val, qspi->base + reg);
+		*txbuf += 4;
+		break;
+	default:
+		dev_dbg(qspi->dev, "word lenght out of range");
+		break;
+	}
+}
+
+static int ti_qspi_setup(struct spi_device *spi)
+{
+	struct ti_qspi	*qspi = spi_master_get_devdata(spi->master);
+	struct ti_qspi_regs *ctx_reg = &qspi->ctx_reg;
+	int clk_div = 0, ret;
+	u32 clk_ctrl_reg, clk_rate, clk_mask;
+
+	clk_rate = clk_get_rate(qspi->fclk);
+
+	if (!qspi->spi_max_frequency) {
+		dev_err(qspi->dev, "spi max frequency not defined\n");
+		return -EINVAL;
+	} else {
+		clk_div = DIV_ROUND_UP(clk_rate, qspi->spi_max_frequency) - 1;
+	}
+
+	dev_dbg(qspi->dev, "%s: hz: %d, clock divider %d\n", __func__,
+			qspi->spi_max_frequency, clk_div);
+
+	ret = pm_runtime_get_sync(qspi->dev);
+	if (ret) {
+		dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
+		return ret;
+	}
+
+	clk_ctrl_reg = ti_qspi_read(qspi, QSPI_SPI_CLOCK_CNTRL_REG);
+
+	clk_ctrl_reg &= ~QSPI_CLK_EN;
+
+	/* disable SCLK */
+	if (!spi->master->busy)
+		ti_qspi_write(qspi, clk_ctrl_reg, QSPI_SPI_CLOCK_CNTRL_REG);
+	else
+		dev_dbg(qspi->dev, "master busy doing other trasnfers\n");
+
+	if (clk_div < 0) {
+		dev_dbg(qspi->dev, "%s: clock divider < 0, using /1 divider\n",
+				__func__);
+		return -EINVAL;
+	}
+
+	if (clk_div > QSPI_CLK_DIV_MAX) {
+		dev_dbg(qspi->dev, "%s: clock divider >%d , using /%d divider\n",
+			__func__, QSPI_CLK_DIV_MAX, QSPI_CLK_DIV_MAX + 1);
+		return -EINVAL;
+	}
+
+	/* enable SCLK */
+	clk_mask = QSPI_CLK_EN | clk_div;
+	ti_qspi_write(qspi, clk_mask, QSPI_SPI_CLOCK_CNTRL_REG);
+	ctx_reg->clkctrl = clk_mask;
+
+	pm_runtime_mark_last_busy(qspi->dev);
+	ret = pm_runtime_put_autosuspend(qspi->dev);
+	if (ret < 0) {
+		dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void ti_qspi_restore_ctx(struct ti_qspi *qspi)
+{
+	struct ti_qspi_regs *ctx_reg = &qspi->ctx_reg;
+
+	ti_qspi_write(qspi, ctx_reg->clkctrl, QSPI_SPI_CLOCK_CNTRL_REG);
+}
+
+static void qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t)
+{
+	const u8 *txbuf;
+	int wlen, count;
+
+	count = t->len;
+	txbuf = t->tx_buf;
+	wlen = t->bits_per_word;
+
+	while (count--) {
+		dev_dbg(qspi->dev, "tx cmd %08x dc %08x data %02x\n",
+			qspi->cmd | QSPI_WR_SNGL, qspi->dc, *txbuf);
+		ti_qspi_write_data(qspi, *txbuf, QSPI_SPI_DATA_REG,
+					wlen, &txbuf);
+		ti_qspi_write(qspi, qspi->dc, QSPI_SPI_DC_REG);
+		ti_qspi_write(qspi, qspi->cmd | QSPI_WR_SNGL,
+			QSPI_SPI_CMD_REG);
+		ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
+		wait_for_completion(&qspi->transfer_complete);
+	}
+}
+
+static void qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t)
+{
+	u8 *rxbuf;
+	int wlen, count;
+
+	count = t->len;
+	rxbuf = t->rx_buf;
+	wlen = t->bits_per_word;
+
+	while (count--) {
+		dev_dbg(qspi->dev, "rx cmd %08x dc %08x\n",
+			qspi->cmd | QSPI_RD_SNGL, qspi->dc);
+		ti_qspi_write(qspi, qspi->dc, QSPI_SPI_DC_REG);
+		ti_qspi_write(qspi, qspi->cmd | QSPI_RD_SNGL,
+			QSPI_SPI_CMD_REG);
+		ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
+		wait_for_completion(&qspi->transfer_complete);
+		ti_qspi_read_data(qspi, QSPI_SPI_DATA_REG, wlen, &rxbuf);
+		dev_dbg(qspi->dev, "rx done, read %02x\n", *(rxbuf-1));
+	}
+}
+
+static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t)
+{
+	if (t->tx_buf)
+		qspi_write_msg(qspi, t);
+	if (t->rx_buf)
+		qspi_read_msg(qspi, t);
+
+	return 0;
+}
+
+static int ti_qspi_start_transfer_one(struct spi_master *master,
+		struct spi_message *m)
+{
+	struct ti_qspi *qspi = spi_master_get_devdata(master);
+	struct spi_device *spi = m->spi;
+	struct spi_transfer *t;
+	int status = 0, ret;
+	int frame_length;
+
+	/* setup device control reg */
+	qspi->dc = 0;
+
+	if (spi->mode & SPI_CPHA)
+		qspi->dc |= QSPI_CKPHA(spi->chip_select);
+	if (spi->mode & SPI_CPOL)
+		qspi->dc |= QSPI_CKPOL(spi->chip_select);
+	if (spi->mode & SPI_CS_HIGH)
+		qspi->dc |= QSPI_CSPOL(spi->chip_select);
+
+	frame_length = (m->frame_length << 3) / spi->bits_per_word;
+
+	frame_length = clamp(frame_length, 0, QSPI_FRAME_MAX);
+
+	/* setup command reg */
+	qspi->cmd = 0;
+	qspi->cmd |= QSPI_EN_CS(spi->chip_select);
+	qspi->cmd |= QSPI_FLEN(frame_length);
+	qspi->cmd |= QSPI_WC_CMD_INT_EN;
+
+	list_for_each_entry(t, &m->transfers, transfer_list) {
+		qspi->cmd |= QSPI_WLEN(t->bits_per_word);
+
+		ret = qspi_transfer_msg(qspi, t);
+		if (ret) {
+			dev_dbg(qspi->dev, "transfer message failed\n");
+			return -EINVAL;
+		}
+
+		m->actual_length += t->len;
+	}
+
+	m->status = status;
+	spi_finalize_current_message(master);
+
+	ti_qspi_write(qspi, qspi->cmd | QSPI_INVAL, QSPI_SPI_CMD_REG);
+
+	return status;
+}
+
+static irqreturn_t ti_qspi_isr(int irq, void *dev_id)
+{
+	struct ti_qspi *qspi = dev_id;
+	u16 mask, stat;
+
+	irqreturn_t ret = IRQ_HANDLED;
+
+	spin_lock(&qspi->lock);
+
+	stat = ti_qspi_read(qspi, QSPI_INTR_STATUS_ENABLED_CLEAR);
+	mask = ti_qspi_read(qspi, QSPI_INTR_ENABLE_SET_REG);
+
+	if (!(stat & mask)) {
+		dev_dbg(qspi->dev, "No IRQ triggered\n");
+		ret = IRQ_NONE;
+	}
+
+	ret = IRQ_WAKE_THREAD;
+
+	spin_unlock(&qspi->lock);
+
+	return ret;
+}
+
+static irqreturn_t ti_qspi_threaded_isr(int this_irq, void *dev_id)
+{
+	struct ti_qspi *qspi = dev_id;
+	unsigned long flags;
+
+	spin_lock_irqsave(&qspi->lock, flags);
+
+	ti_qspi_write(qspi, QSPI_WC_INT_DISABLE, QSPI_INTR_ENABLE_CLEAR_REG);
+	ti_qspi_write(qspi, QSPI_WC_INT_DISABLE,
+			QSPI_INTR_STATUS_ENABLED_CLEAR);
+
+	complete(&qspi->transfer_complete);
+
+	spin_unlock_irqrestore(&qspi->lock, flags);
+
+	return IRQ_HANDLED;
+}
+
+static int ti_qspi_runtime_resume(struct device *dev)
+{
+	struct ti_qspi      *qspi;
+	struct spi_master       *master;
+
+	master = dev_get_drvdata(dev);
+	qspi = spi_master_get_devdata(master);
+	ti_qspi_restore_ctx(qspi);
+
+	return 0;
+}
+
+static const struct of_device_id ti_qspi_match[] = {
+	{.compatible = "ti,dra7xxx-qspi" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, dra7xxx_qspi_match);
+
+static int ti_qspi_probe(struct platform_device *pdev)
+{
+	struct  ti_qspi *qspi;
+	struct spi_master *master;
+	struct resource         *r;
+	struct device_node *np = pdev->dev.of_node;
+	u32 max_freq;
+	int ret = 0, num_cs, irq;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(*qspi));
+	if (!master)
+		return -ENOMEM;
+
+	master->mode_bits = SPI_CPOL | SPI_CPHA;
+
+	master->num_chipselect = 1;
+	master->bus_num = -1;
+	master->flags = SPI_MASTER_HALF_DUPLEX;
+	master->setup = ti_qspi_setup;
+	master->auto_runtime_pm = true;
+	master->transfer_one_message = ti_qspi_start_transfer_one;
+	master->dev.of_node = pdev->dev.of_node;
+	master->bits_per_word_mask = BIT(32 - 1) | BIT(16 - 1) | BIT(8 - 1);
+
+	if (!of_property_read_u32(np, "num-cs", &num_cs))
+		master->num_chipselect = num_cs;
+
+	platform_set_drvdata(pdev, master);
+
+	qspi = spi_master_get_devdata(master);
+	qspi->master = master;
+	qspi->dev = &pdev->dev;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (r == NULL) {
+		ret = -ENODEV;
+		goto free_master;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "no irq resource?\n");
+		return irq;
+	}
+
+	spin_lock_init(&qspi->lock);
+
+	qspi->base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(qspi->base)) {
+		ret = PTR_ERR(qspi->base);
+		goto free_master;
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr,
+			ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT,
+			dev_name(&pdev->dev), qspi);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to register ISR for IRQ %d\n",
+				irq);
+		goto free_master;
+	}
+
+	qspi->fclk = devm_clk_get(&pdev->dev, "fck");
+	if (IS_ERR(qspi->fclk)) {
+		ret = PTR_ERR(qspi->fclk);
+		dev_err(&pdev->dev, "could not get clk: %d\n", ret);
+	}
+
+	init_completion(&qspi->transfer_complete);
+
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, SPI_AUTOSUSPEND_TIMEOUT);
+	pm_runtime_enable(&pdev->dev);
+
+	if (!of_property_read_u32(np, "spi-max-frequency", &max_freq))
+		qspi->spi_max_frequency = max_freq;
+
+	ret = spi_register_master(master);
+	if (ret)
+		goto free_master;
+
+	return ret;
+
+free_master:
+	spi_master_put(master);
+	return ret;
+}
+
+static int ti_qspi_remove(struct platform_device *pdev)
+{
+	struct	ti_qspi *qspi = platform_get_drvdata(pdev);
+
+	spi_unregister_master(qspi->master);
+
+	return 0;
+}
+
+static const struct dev_pm_ops ti_qspi_pm_ops = {
+	.runtime_resume = ti_qspi_runtime_resume,
+};
+
+static struct platform_driver ti_qspi_driver = {
+	.probe	= ti_qspi_probe,
+	.remove	= ti_qspi_remove,
+	.driver = {
+		.name	= "ti,dra7xxx-qspi",
+		.owner	= THIS_MODULE,
+		.pm =   &ti_qspi_pm_ops,
+		.of_match_table = ti_qspi_match,
+	}
+};
+
+module_platform_driver(ti_qspi_driver);
+
+MODULE_AUTHOR("Sourav Poddar <sourav.poddar@ti.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("TI QSPI controller driver");
-- 
1.7.1

Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 11309 bytes --]

Hi,

On Mon, Jul 29, 2013 at 12:52:29PM +0530, Sourav Poddar wrote:
> diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
> new file mode 100644
> index 0000000..51fe95f
> --- /dev/null
> +++ b/drivers/spi/spi-ti-qspi.c

[ snip ]

> +struct ti_qspi {
> +	spinlock_t              lock;           /* IRQ synchronization */
> +	struct spi_master	*master;
> +	void __iomem            *base;
> +	struct device           *dev;
> +	struct completion       transfer_complete;
> +	struct clk *fclk;
> +	struct ti_qspi_regs     ctx_reg;
> +	int device_type;

device_type isn't used

> +	u32 spi_max_frequency;
> +	u32 cmd;
> +	u32 dc;
> +};

you need to make a choice here ? Are you going to tabify the structure
or not ? You might also want to reorganize this structure to it looks
like below:

struct ti_qspi {
	struct completion	transfer_complete;

	/* IRQ synchronization */
	spinlock_t		lock;

	struct spi_master	*master;
	void __iomem		*base;
	struct clk		*fclk;
	struct device		*dev;

	struct ti_qspi_regs	ctx_reg;

	u32			spi_max_frequency;
	u32			cmd;
	u32			dc;
};

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

WC_BUSY isn't used in this file. It looks unnecessary too.

> +#define QSPI_XFER_DONE			QSPI_WC

so transfer done is word completion ? Why do you need this ? It's not
even used in this source file.

> +static inline void ti_qspi_read_data(struct ti_qspi *qspi,
> +		unsigned long reg, int wlen, u8 **rxbuf)
> +{
> +	switch (wlen) {
> +	case 8:
> +		**rxbuf = readb(qspi->base + reg);
> +		dev_dbg(qspi->dev, "rx done, read %02x\n", *(*rxbuf-1));

you're incrementing only after printing, do you really need the -1 here?

Also, I would change these into dev_vdbg(), it's quite unlikely someone
needs to track all reads to the data register and since you're not
printing the writes either, this looks even more like a case for
dev_vdbg()

> +		*rxbuf += 1;
> +		break;
> +	case 16:
> +		**rxbuf = readw(qspi->base + reg);
> +		dev_dbg(qspi->dev, "rx done, read %02x\n", *(*rxbuf-1));

%04x and no -1 (also, if you needed to decrement, it would have to be
%-2).

> +		*rxbuf += 2;
> +		break;
> +	case 32:
> +		**rxbuf = readl(qspi->base + reg);
> +		dev_dbg(qspi->dev, "rx done, read %02x\n", *(*rxbuf-1));

%04x

> +		*rxbuf += 4;
> +	default:
> +		dev_dbg(qspi->dev, "word lenght out of range");

s/lenght/length

> +static int ti_qspi_setup(struct spi_device *spi)
> +{
> +	struct ti_qspi	*qspi = spi_master_get_devdata(spi->master);
> +	struct ti_qspi_regs *ctx_reg = &qspi->ctx_reg;
> +	int clk_div = 0, ret;
> +	u32 clk_ctrl_reg, clk_rate, clk_mask;
> +
> +	clk_rate = clk_get_rate(qspi->fclk);
> +
> +	if (!qspi->spi_max_frequency) {
> +		dev_err(qspi->dev, "spi max frequency not defined\n");
> +		return -EINVAL;

if you're returning here...

> +	} else {

this else is unneeded

> +		clk_div = DIV_ROUND_UP(clk_rate, qspi->spi_max_frequency) - 1;
> +	}
> +
> +	dev_dbg(qspi->dev, "%s: hz: %d, clock divider %d\n", __func__,
> +			qspi->spi_max_frequency, clk_div);
> +
> +	ret = pm_runtime_get_sync(qspi->dev);
> +	if (ret) {
> +		dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
> +		return ret;
> +	}

after Mark's patch, is this really needed ?

> +	clk_ctrl_reg = ti_qspi_read(qspi, QSPI_SPI_CLOCK_CNTRL_REG);
> +
> +	clk_ctrl_reg &= ~QSPI_CLK_EN;
> +
> +	/* disable SCLK */
> +	if (!spi->master->busy)
> +		ti_qspi_write(qspi, clk_ctrl_reg, QSPI_SPI_CLOCK_CNTRL_REG);
> +	else
> +		dev_dbg(qspi->dev, "master busy doing other trasnfers\n");

if master is busy, shouldn't you return -EBUSY at this point ?

> +
> +	if (clk_div < 0) {
> +		dev_dbg(qspi->dev, "%s: clock divider < 0, using /1 divider\n",
> +				__func__);
> +		return -EINVAL;

you do a get_sync without a put_sync() here.

> +	}
> +
> +	if (clk_div > QSPI_CLK_DIV_MAX) {
> +		dev_dbg(qspi->dev, "%s: clock divider >%d , using /%d divider\n",
> +			__func__, QSPI_CLK_DIV_MAX, QSPI_CLK_DIV_MAX + 1);
> +		return -EINVAL;

no put_sync()

> +	}
> +
> +	/* enable SCLK */
> +	clk_mask = QSPI_CLK_EN | clk_div;
> +	ti_qspi_write(qspi, clk_mask, QSPI_SPI_CLOCK_CNTRL_REG);
> +	ctx_reg->clkctrl = clk_mask;
> +
> +	pm_runtime_mark_last_busy(qspi->dev);
> +	ret = pm_runtime_put_autosuspend(qspi->dev);
> +	if (ret < 0) {
> +		dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void ti_qspi_restore_ctx(struct ti_qspi *qspi)
> +{
> +	struct ti_qspi_regs *ctx_reg = &qspi->ctx_reg;
> +
> +	ti_qspi_write(qspi, ctx_reg->clkctrl, QSPI_SPI_CLOCK_CNTRL_REG);
> +}
> +
> +static void qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t)
> +{
> +	const u8 *txbuf;
> +	int wlen, count;
> +
> +	count = t->len;
> +	txbuf = t->tx_buf;
> +	wlen = t->bits_per_word;
> +
> +	while (count--) {
> +		dev_dbg(qspi->dev, "tx cmd %08x dc %08x data %02x\n",
> +			qspi->cmd | QSPI_WR_SNGL, qspi->dc, *txbuf);
> +		ti_qspi_write_data(qspi, *txbuf, QSPI_SPI_DATA_REG,
> +					wlen, &txbuf);
> +		ti_qspi_write(qspi, qspi->dc, QSPI_SPI_DC_REG);
> +		ti_qspi_write(qspi, qspi->cmd | QSPI_WR_SNGL,
> +			QSPI_SPI_CMD_REG);
> +		ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
> +		wait_for_completion(&qspi->transfer_complete);

you can't wait forever dude, wait_for_completion_timeout() and give it a
2 second timeout, then you can make this and qspi_read_msg() return
something so you can notify that the controller didn't transfer anything
in case your wait_for_completion_timeout() actually times out.

> +	}
> +}
> +
> +static void qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t)
> +{
> +	u8 *rxbuf;
> +	int wlen, count;
> +
> +	count = t->len;
> +	rxbuf = t->rx_buf;
> +	wlen = t->bits_per_word;
> +
> +	while (count--) {
> +		dev_dbg(qspi->dev, "rx cmd %08x dc %08x\n",
> +			qspi->cmd | QSPI_RD_SNGL, qspi->dc);
> +		ti_qspi_write(qspi, qspi->dc, QSPI_SPI_DC_REG);
> +		ti_qspi_write(qspi, qspi->cmd | QSPI_RD_SNGL,
> +			QSPI_SPI_CMD_REG);
> +		ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
> +		wait_for_completion(&qspi->transfer_complete);

wait_for_completion_timeout()

> +		ti_qspi_read_data(qspi, QSPI_SPI_DATA_REG, wlen, &rxbuf);
> +		dev_dbg(qspi->dev, "rx done, read %02x\n", *(rxbuf-1));
> +	}
> +}
> +
> +static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t)
> +{
> +	if (t->tx_buf)
> +		qspi_write_msg(qspi, t);
> +	if (t->rx_buf)
> +		qspi_read_msg(qspi, t);
> +
> +	return 0;
> +}
> +
> +static int ti_qspi_start_transfer_one(struct spi_master *master,
> +		struct spi_message *m)
> +{
> +	struct ti_qspi *qspi = spi_master_get_devdata(master);
> +	struct spi_device *spi = m->spi;
> +	struct spi_transfer *t;
> +	int status = 0, ret;
> +	int frame_length;
> +
> +	/* setup device control reg */
> +	qspi->dc = 0;
> +
> +	if (spi->mode & SPI_CPHA)
> +		qspi->dc |= QSPI_CKPHA(spi->chip_select);
> +	if (spi->mode & SPI_CPOL)
> +		qspi->dc |= QSPI_CKPOL(spi->chip_select);
> +	if (spi->mode & SPI_CS_HIGH)
> +		qspi->dc |= QSPI_CSPOL(spi->chip_select);
> +
> +	frame_length = (m->frame_length << 3) / spi->bits_per_word;

there's another way to optimize this. If you assume bits_per_word to
always be power of two you can:

	frame_length = (m->frame_length << 3) >> __ffs(spi->bits_per_word);

but that would need a comment stating that you're assuming bits_per_word
to always be a power of two. Not sure if this is a correct assumption.

> +	frame_length = clamp(frame_length, 0, QSPI_FRAME_MAX);
> +
> +	/* setup command reg */
> +	qspi->cmd = 0;
> +	qspi->cmd |= QSPI_EN_CS(spi->chip_select);
> +	qspi->cmd |= QSPI_FLEN(frame_length);
> +	qspi->cmd |= QSPI_WC_CMD_INT_EN;
> +
> +	list_for_each_entry(t, &m->transfers, transfer_list) {
> +		qspi->cmd |= QSPI_WLEN(t->bits_per_word);
> +
> +		ret = qspi_transfer_msg(qspi, t);
> +		if (ret) {
> +			dev_dbg(qspi->dev, "transfer message failed\n");
> +			return -EINVAL;
> +		}
> +
> +		m->actual_length += t->len;
> +	}
> +
> +	m->status = status;
> +	spi_finalize_current_message(master);
> +
> +	ti_qspi_write(qspi, qspi->cmd | QSPI_INVAL, QSPI_SPI_CMD_REG);
> +
> +	return status;
> +}
> +
> +static irqreturn_t ti_qspi_isr(int irq, void *dev_id)
> +{
> +	struct ti_qspi *qspi = dev_id;
> +	u16 mask, stat;
> +
> +	irqreturn_t ret = IRQ_HANDLED;
> +
> +	spin_lock(&qspi->lock);
> +
> +	stat = ti_qspi_read(qspi, QSPI_INTR_STATUS_ENABLED_CLEAR);

since you're not reading the _RAW register, you don't need the mask
below, right ?

> +	mask = ti_qspi_read(qspi, QSPI_INTR_ENABLE_SET_REG);
> +
> +	if (!(stat & mask)) {
> +		dev_dbg(qspi->dev, "No IRQ triggered\n");
> +		ret = IRQ_NONE;
> +	}
> +
> +	ret = IRQ_WAKE_THREAD;

look at this code and tell me when will you *EVER* return IRQ_NONE. Dude
you're waking up the IRQ thread even when there are no IRQs to be
handled.

> +	spin_unlock(&qspi->lock);

You should, before releasing the lock, mask your IRQs, so that you can
get rid of IRQF_ONESHOT. Unmask them before returning from the IRQ
thread.

> +static int ti_qspi_probe(struct platform_device *pdev)
> +{
> +	struct  ti_qspi *qspi;
> +	struct spi_master *master;
> +	struct resource         *r;
> +	struct device_node *np = pdev->dev.of_node;
> +	u32 max_freq;
> +	int ret = 0, num_cs, irq;
> +
> +	master = spi_alloc_master(&pdev->dev, sizeof(*qspi));
> +	if (!master)
> +		return -ENOMEM;
> +
> +	master->mode_bits = SPI_CPOL | SPI_CPHA;
> +
> +	master->num_chipselect = 1;

again with the num_chipselect = 1 ? IIRC this controller has 4
chipselects, just read 24.5.1 on your TRM.

> +	master->bus_num = -1;
> +	master->flags = SPI_MASTER_HALF_DUPLEX;
> +	master->setup = ti_qspi_setup;
> +	master->auto_runtime_pm = true;
> +	master->transfer_one_message = ti_qspi_start_transfer_one;
> +	master->dev.of_node = pdev->dev.of_node;
> +	master->bits_per_word_mask = BIT(32 - 1) | BIT(16 - 1) | BIT(8 - 1);
> +
> +	if (!of_property_read_u32(np, "num-cs", &num_cs))
> +		master->num_chipselect = num_cs;

so you reinitialize here num_chipselect, then why do you initialize it
above ?

> +	platform_set_drvdata(pdev, master);
> +
> +	qspi = spi_master_get_devdata(master);
> +	qspi->master = master;
> +	qspi->dev = &pdev->dev;
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (r == NULL) {
> +		ret = -ENODEV;
> +		goto free_master;
> +	}

you don't need to check the resource when using devm_ioremap_resource().

> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "no irq resource?\n");
> +		return irq;
> +	}
> +
> +	spin_lock_init(&qspi->lock);
> +
> +	qspi->base = devm_ioremap_resource(&pdev->dev, r);
> +	if (IS_ERR(qspi->base)) {
> +		ret = PTR_ERR(qspi->base);
> +		goto free_master;
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr,
> +			ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT,

why do you need IRQF_NO_SUSPEND ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller

[Sourav Poddar]

On Monday 29 July 2013 03:01 PM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Jul 29, 2013 at 12:52:29PM +0530, Sourav Poddar wrote:
>> diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
>> new file mode 100644
>> index 0000000..51fe95f
>> --- /dev/null
>> +++ b/drivers/spi/spi-ti-qspi.c
> [ snip ]
>
>> +struct ti_qspi {
>> +	spinlock_t              lock;           /* IRQ synchronization */
>> +	struct spi_master	*master;
>> +	void __iomem            *base;
>> +	struct device           *dev;
>> +	struct completion       transfer_complete;
>> +	struct clk *fclk;
>> +	struct ti_qspi_regs     ctx_reg;
>> +	int device_type;
> device_type isn't used
>
My bad. yes, will remove. Was added for some experiments.
>> +	u32 spi_max_frequency;
>> +	u32 cmd;
>> +	u32 dc;
>> +};
> you need to make a choice here ? Are you going to tabify the structure
> or not ? You might also want to reorganize this structure to it looks
> like below:
>
> struct ti_qspi {
> 	struct completion	transfer_complete;
>
> 	/* IRQ synchronization */
> 	spinlock_t		lock;
>
> 	struct spi_master	*master;
> 	void __iomem		*base;
> 	struct clk		*fclk;
> 	struct device		*dev;
>
> 	struct ti_qspi_regs	ctx_reg;
>
> 	u32			spi_max_frequency;
> 	u32			cmd;
> 	u32			dc;
> };
>
hmm..yes will do.
>> +/* Status */
>> +#define QSPI_WC				(1<<  1)
>> +#define QSPI_BUSY			(1<<  0)
>> +#define QSPI_WC_BUSY			(QSPI_WC | QSPI_BUSY)
> WC_BUSY isn't used in this file. It looks unnecessary too.
>
Yes.
>> +#define QSPI_XFER_DONE			QSPI_WC
> so transfer done is word completion ? Why do you need this ? It's not
> even used in this source file.
>
Yes, this was used in ealier versons for polled mode. Will remove.
>> +static inline void ti_qspi_read_data(struct ti_qspi *qspi,
>> +		unsigned long reg, int wlen, u8 **rxbuf)
>> +{
>> +	switch (wlen) {
>> +	case 8:
>> +		**rxbuf = readb(qspi->base + reg);
>> +		dev_dbg(qspi->dev, "rx done, read %02x\n", *(*rxbuf-1));
> you're incrementing only after printing, do you really need the -1 here?
>
> Also, I would change these into dev_vdbg(), it's quite unlikely someone
> needs to track all reads to the data register and since you're not
> printing the writes either, this looks even more like a case for
> dev_vdbg()
>
Ok.will change.
>> +		*rxbuf += 1;
>> +		break;
>> +	case 16:
>> +		**rxbuf = readw(qspi->base + reg);
>> +		dev_dbg(qspi->dev, "rx done, read %02x\n", *(*rxbuf-1));
> %04x and no -1 (also, if you needed to decrement, it would have to be
> %-2).
>
Ok.
>> +		*rxbuf += 2;
>> +		break;
>> +	case 32:
>> +		**rxbuf = readl(qspi->base + reg);
>> +		dev_dbg(qspi->dev, "rx done, read %02x\n", *(*rxbuf-1));
> %04x
>
OK
>> +		*rxbuf += 4;
>> +	default:
>> +		dev_dbg(qspi->dev, "word lenght out of range");
> s/lenght/length
>
hmm..will change.
>> +static int ti_qspi_setup(struct spi_device *spi)
>> +{
>> +	struct ti_qspi	*qspi = spi_master_get_devdata(spi->master);
>> +	struct ti_qspi_regs *ctx_reg =&qspi->ctx_reg;
>> +	int clk_div = 0, ret;
>> +	u32 clk_ctrl_reg, clk_rate, clk_mask;
>> +
>> +	clk_rate = clk_get_rate(qspi->fclk);
>> +
>> +	if (!qspi->spi_max_frequency) {
>> +		dev_err(qspi->dev, "spi max frequency not defined\n");
>> +		return -EINVAL;
> if you're returning here...
>
>> +	} else {
> this else is unneeded
>
hmm..true. Will change.
>> +		clk_div = DIV_ROUND_UP(clk_rate, qspi->spi_max_frequency) - 1;
>> +	}
>> +
>> +	dev_dbg(qspi->dev, "%s: hz: %d, clock divider %d\n", __func__,
>> +			qspi->spi_max_frequency, clk_div);
>> +
>> +	ret = pm_runtime_get_sync(qspi->dev);
>> +	if (ret) {
>> +		dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
>> +		return ret;
>> +	}
> after Mark's patch, is this really needed ?
>
>> +	clk_ctrl_reg = ti_qspi_read(qspi, QSPI_SPI_CLOCK_CNTRL_REG);
>> +
>> +	clk_ctrl_reg&= ~QSPI_CLK_EN;
>> +
>> +	/* disable SCLK */
>> +	if (!spi->master->busy)
>> +		ti_qspi_write(qspi, clk_ctrl_reg, QSPI_SPI_CLOCK_CNTRL_REG);
>> +	else
>> +		dev_dbg(qspi->dev, "master busy doing other trasnfers\n");
> if master is busy, shouldn't you return -EBUSY at this point ?
>
Yes. will add.
>> +
>> +	if (clk_div<  0) {
>> +		dev_dbg(qspi->dev, "%s: clock divider<  0, using /1 divider\n",
>> +				__func__);
>> +		return -EINVAL;
> you do a get_sync without a put_sync() here.
>
Hmm..will add put_sync in error path.
>> +	}
>> +
>> +	if (clk_div>  QSPI_CLK_DIV_MAX) {
>> +		dev_dbg(qspi->dev, "%s: clock divider>%d , using /%d divider\n",
>> +			__func__, QSPI_CLK_DIV_MAX, QSPI_CLK_DIV_MAX + 1);
>> +		return -EINVAL;
> no put_sync()
>
Same.
>> +	}
>> +
>> +	/* enable SCLK */
>> +	clk_mask = QSPI_CLK_EN | clk_div;
>> +	ti_qspi_write(qspi, clk_mask, QSPI_SPI_CLOCK_CNTRL_REG);
>> +	ctx_reg->clkctrl = clk_mask;
>> +
>> +	pm_runtime_mark_last_busy(qspi->dev);
>> +	ret = pm_runtime_put_autosuspend(qspi->dev);
>> +	if (ret<  0) {
>> +		dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void ti_qspi_restore_ctx(struct ti_qspi *qspi)
>> +{
>> +	struct ti_qspi_regs *ctx_reg =&qspi->ctx_reg;
>> +
>> +	ti_qspi_write(qspi, ctx_reg->clkctrl, QSPI_SPI_CLOCK_CNTRL_REG);
>> +}
>> +
>> +static void qspi_write_msg(struct ti_qspi *qspi, struct spi_transfer *t)
>> +{
>> +	const u8 *txbuf;
>> +	int wlen, count;
>> +
>> +	count = t->len;
>> +	txbuf = t->tx_buf;
>> +	wlen = t->bits_per_word;
>> +
>> +	while (count--) {
>> +		dev_dbg(qspi->dev, "tx cmd %08x dc %08x data %02x\n",
>> +			qspi->cmd | QSPI_WR_SNGL, qspi->dc, *txbuf);
>> +		ti_qspi_write_data(qspi, *txbuf, QSPI_SPI_DATA_REG,
>> +					wlen,&txbuf);
>> +		ti_qspi_write(qspi, qspi->dc, QSPI_SPI_DC_REG);
>> +		ti_qspi_write(qspi, qspi->cmd | QSPI_WR_SNGL,
>> +			QSPI_SPI_CMD_REG);
>> +		ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
>> +		wait_for_completion(&qspi->transfer_complete);
> you can't wait forever dude, wait_for_completion_timeout() and give it a
> 2 second timeout, then you can make this and qspi_read_msg() return
> something so you can notify that the controller didn't transfer anything
> in case your wait_for_completion_timeout() actually times out.
>
Ok.
>> +	}
>> +}
>> +
>> +static void qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t)
>> +{
>> +	u8 *rxbuf;
>> +	int wlen, count;
>> +
>> +	count = t->len;
>> +	rxbuf = t->rx_buf;
>> +	wlen = t->bits_per_word;
>> +
>> +	while (count--) {
>> +		dev_dbg(qspi->dev, "rx cmd %08x dc %08x\n",
>> +			qspi->cmd | QSPI_RD_SNGL, qspi->dc);
>> +		ti_qspi_write(qspi, qspi->dc, QSPI_SPI_DC_REG);
>> +		ti_qspi_write(qspi, qspi->cmd | QSPI_RD_SNGL,
>> +			QSPI_SPI_CMD_REG);
>> +		ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
>> +		wait_for_completion(&qspi->transfer_complete);
> wait_for_completion_timeout()
>
Ok.
>> +		ti_qspi_read_data(qspi, QSPI_SPI_DATA_REG, wlen,&rxbuf);
>> +		dev_dbg(qspi->dev, "rx done, read %02x\n", *(rxbuf-1));
>> +	}
>> +}
>> +
>> +static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t)
>> +{
>> +	if (t->tx_buf)
>> +		qspi_write_msg(qspi, t);
>> +	if (t->rx_buf)
>> +		qspi_read_msg(qspi, t);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ti_qspi_start_transfer_one(struct spi_master *master,
>> +		struct spi_message *m)
>> +{
>> +	struct ti_qspi *qspi = spi_master_get_devdata(master);
>> +	struct spi_device *spi = m->spi;
>> +	struct spi_transfer *t;
>> +	int status = 0, ret;
>> +	int frame_length;
>> +
>> +	/* setup device control reg */
>> +	qspi->dc = 0;
>> +
>> +	if (spi->mode&  SPI_CPHA)
>> +		qspi->dc |= QSPI_CKPHA(spi->chip_select);
>> +	if (spi->mode&  SPI_CPOL)
>> +		qspi->dc |= QSPI_CKPOL(spi->chip_select);
>> +	if (spi->mode&  SPI_CS_HIGH)
>> +		qspi->dc |= QSPI_CSPOL(spi->chip_select);
>> +
>> +	frame_length = (m->frame_length<<  3) / spi->bits_per_word;
> there's another way to optimize this. If you assume bits_per_word to
> always be power of two you can:
>
> 	frame_length = (m->frame_length<<  3)>>  __ffs(spi->bits_per_word);
>
> but that would need a comment stating that you're assuming bits_per_word
> to always be a power of two. Not sure if this is a correct assumption.
>
I dont think it will be a correct assumption, since our controller is 
flexible to
handle anything from 1 to 128 as bits_per_word.
>> +	frame_length = clamp(frame_length, 0, QSPI_FRAME_MAX);
>> +
>> +	/* setup command reg */
>> +	qspi->cmd = 0;
>> +	qspi->cmd |= QSPI_EN_CS(spi->chip_select);
>> +	qspi->cmd |= QSPI_FLEN(frame_length);
>> +	qspi->cmd |= QSPI_WC_CMD_INT_EN;
>> +
>> +	list_for_each_entry(t,&m->transfers, transfer_list) {
>> +		qspi->cmd |= QSPI_WLEN(t->bits_per_word);
>> +
>> +		ret = qspi_transfer_msg(qspi, t);
>> +		if (ret) {
>> +			dev_dbg(qspi->dev, "transfer message failed\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		m->actual_length += t->len;
>> +	}
>> +
>> +	m->status = status;
>> +	spi_finalize_current_message(master);
>> +
>> +	ti_qspi_write(qspi, qspi->cmd | QSPI_INVAL, QSPI_SPI_CMD_REG);
>> +
>> +	return status;
>> +}
>> +
>> +static irqreturn_t ti_qspi_isr(int irq, void *dev_id)
>> +{
>> +	struct ti_qspi *qspi = dev_id;
>> +	u16 mask, stat;
>> +
>> +	irqreturn_t ret = IRQ_HANDLED;
>> +
>> +	spin_lock(&qspi->lock);
>> +
>> +	stat = ti_qspi_read(qspi, QSPI_INTR_STATUS_ENABLED_CLEAR);
> since you're not reading the _RAW register, you don't need the mask
> below, right ?
>
I think yes, only status register should be sufficient.
>> +	mask = ti_qspi_read(qspi, QSPI_INTR_ENABLE_SET_REG);
>> +
>> +	if (!(stat&  mask)) {
>> +		dev_dbg(qspi->dev, "No IRQ triggered\n");
>> +		ret = IRQ_NONE;
>> +	}
>> +
>> +	ret = IRQ_WAKE_THREAD;
> look at this code and tell me when will you *EVER* return IRQ_NONE. Dude
> you're waking up the IRQ thread even when there are no IRQs to be
> handled.
>
My bad, should add a return ret before IRQ_NONE.
>> +	spin_unlock(&qspi->lock);
> You should, before releasing the lock, mask your IRQs, so that you can
> get rid of IRQF_ONESHOT. Unmask them before returning from the IRQ
> thread.
>
Sorry, did not get  you here?
  I am reading interrupt status register before and clearing them in the 
threaded irq.
>> +static int ti_qspi_probe(struct platform_device *pdev)
>> +{
>> +	struct  ti_qspi *qspi;
>> +	struct spi_master *master;
>> +	struct resource         *r;
>> +	struct device_node *np = pdev->dev.of_node;
>> +	u32 max_freq;
>> +	int ret = 0, num_cs, irq;
>> +
>> +	master = spi_alloc_master(&pdev->dev, sizeof(*qspi));
>> +	if (!master)
>> +		return -ENOMEM;
>> +
>> +	master->mode_bits = SPI_CPOL | SPI_CPHA;
>> +
>> +	master->num_chipselect = 1;
> again with the num_chipselect = 1 ? IIRC this controller has 4
> chipselects, just read 24.5.1 on your TRM.
>
this is unnecessary. I intended to configure chip selects through dt)as 
done below).
Will remove.
>> +	master->bus_num = -1;
>> +	master->flags = SPI_MASTER_HALF_DUPLEX;
>> +	master->setup = ti_qspi_setup;
>> +	master->auto_runtime_pm = true;
>> +	master->transfer_one_message = ti_qspi_start_transfer_one;
>> +	master->dev.of_node = pdev->dev.of_node;
>> +	master->bits_per_word_mask = BIT(32 - 1) | BIT(16 - 1) | BIT(8 - 1);
>> +
>> +	if (!of_property_read_u32(np, "num-cs",&num_cs))
>> +		master->num_chipselect = num_cs;
> so you reinitialize here num_chipselect, then why do you initialize it
> above ?
>
>> +	platform_set_drvdata(pdev, master);
>> +
>> +	qspi = spi_master_get_devdata(master);
>> +	qspi->master = master;
>> +	qspi->dev =&pdev->dev;
>> +
>> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (r == NULL) {
>> +		ret = -ENODEV;
>> +		goto free_master;
>> +	}
> you don't need to check the resource when using devm_ioremap_resource().
>
Ok.
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq<  0) {
>> +		dev_err(&pdev->dev, "no irq resource?\n");
>> +		return irq;
>> +	}
>> +
>> +	spin_lock_init(&qspi->lock);
>> +
>> +	qspi->base = devm_ioremap_resource(&pdev->dev, r);
>> +	if (IS_ERR(qspi->base)) {
>> +		ret = PTR_ERR(qspi->base);
>> +		goto free_master;
>> +	}
>> +
>> +	ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr,
>> +			ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT,
> why do you need IRQF_NO_SUSPEND ?
>
I should get away with this.

Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2621 bytes --]

Hi,

On Mon, Jul 29, 2013 at 03:42:03PM +0530, Sourav Poddar wrote:
> >>+	frame_length = (m->frame_length<<  3) / spi->bits_per_word;
> >there's another way to optimize this. If you assume bits_per_word to
> >always be power of two you can:
> >
> >	frame_length = (m->frame_length<<  3)>>  __ffs(spi->bits_per_word);
> >
> >but that would need a comment stating that you're assuming bits_per_word
> >to always be a power of two. Not sure if this is a correct assumption.
> >
> I dont think it will be a correct assumption, since our controller is
> flexible to
> handle anything from 1 to 128 as bits_per_word.

right, but can the framework handle non-power-of-two bits_per_word ?

> >>+	spin_unlock(&qspi->lock);
> >You should, before releasing the lock, mask your IRQs, so that you can
> >get rid of IRQF_ONESHOT. Unmask them before returning from the IRQ
> >thread.
> >
> Sorry, did not get  you here?
>  I am reading interrupt status register before and clearing them in
> the threaded irq.

you need to mask the IRQs, clear WC and FIRQ in IRQENABLE register...
set them back at the end of the thread handler.

> >>+static int ti_qspi_probe(struct platform_device *pdev)
> >>+{
> >>+	struct  ti_qspi *qspi;
> >>+	struct spi_master *master;
> >>+	struct resource         *r;
> >>+	struct device_node *np = pdev->dev.of_node;
> >>+	u32 max_freq;
> >>+	int ret = 0, num_cs, irq;
> >>+
> >>+	master = spi_alloc_master(&pdev->dev, sizeof(*qspi));
> >>+	if (!master)
> >>+		return -ENOMEM;
> >>+
> >>+	master->mode_bits = SPI_CPOL | SPI_CPHA;
> >>+
> >>+	master->num_chipselect = 1;
> >again with the num_chipselect = 1 ? IIRC this controller has 4
> >chipselects, just read 24.5.1 on your TRM.
> >
> this is unnecessary. I intended to configure chip selects through
> dt)as done below).
> Will remove.

which brings me to the point of:

Do you review your own code before sending it out ? Doesn't look like...

> >>+	irq = platform_get_irq(pdev, 0);
> >>+	if (irq<  0) {
> >>+		dev_err(&pdev->dev, "no irq resource?\n");
> >>+		return irq;
> >>+	}
> >>+
> >>+	spin_lock_init(&qspi->lock);
> >>+
> >>+	qspi->base = devm_ioremap_resource(&pdev->dev, r);
> >>+	if (IS_ERR(qspi->base)) {
> >>+		ret = PTR_ERR(qspi->base);
> >>+		goto free_master;
> >>+	}
> >>+
> >>+	ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr,
> >>+			ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT,
> >why do you need IRQF_NO_SUSPEND ?
> >
> I should get away with this.

why ? Do you need or do you *not* need it ? And in either case, why ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller

[Sourav Poddar]

On Monday 29 July 2013 03:50 PM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Jul 29, 2013 at 03:42:03PM +0530, Sourav Poddar wrote:
>>>> +	frame_length = (m->frame_length<<   3) / spi->bits_per_word;
>>> there's another way to optimize this. If you assume bits_per_word to
>>> always be power of two you can:
>>>
>>> 	frame_length = (m->frame_length<<   3)>>   __ffs(spi->bits_per_word);
>>>
>>> but that would need a comment stating that you're assuming bits_per_word
>>> to always be a power of two. Not sure if this is a correct assumption.
>>>
>> I dont think it will be a correct assumption, since our controller is
>> flexible to
>> handle anything from 1 to 128 as bits_per_word.
> right, but can the framework handle non-power-of-two bits_per_word ?
>
The only limitation I see is the max bits_per_word, which is 32.
Though,  I did set "master->bits_per_word_mask" as power of 2
in my probe.
>>>> +	spin_unlock(&qspi->lock);
>>> You should, before releasing the lock, mask your IRQs, so that you can
>>> get rid of IRQF_ONESHOT. Unmask them before returning from the IRQ
>>> thread.
>>>
>> Sorry, did not get  you here?
>>   I am reading interrupt status register before and clearing them in
>> the threaded irq.
> you need to mask the IRQs, clear WC and FIRQ in IRQENABLE register...
> set them back at the end of the thread handler.
>
Ok
>>>> +static int ti_qspi_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct  ti_qspi *qspi;
>>>> +	struct spi_master *master;
>>>> +	struct resource         *r;
>>>> +	struct device_node *np = pdev->dev.of_node;
>>>> +	u32 max_freq;
>>>> +	int ret = 0, num_cs, irq;
>>>> +
>>>> +	master = spi_alloc_master(&pdev->dev, sizeof(*qspi));
>>>> +	if (!master)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	master->mode_bits = SPI_CPOL | SPI_CPHA;
>>>> +
>>>> +	master->num_chipselect = 1;
>>> again with the num_chipselect = 1 ? IIRC this controller has 4
>>> chipselects, just read 24.5.1 on your TRM.
>>>
>> this is unnecessary. I intended to configure chip selects through
>> dt)as done below).
>> Will remove.
> which brings me to the point of:
>
> Do you review your own code before sending it out ? Doesn't look like...
>
>>>> +	irq = platform_get_irq(pdev, 0);
>>>> +	if (irq<   0) {
>>>> +		dev_err(&pdev->dev, "no irq resource?\n");
>>>> +		return irq;
>>>> +	}
>>>> +
>>>> +	spin_lock_init(&qspi->lock);
>>>> +
>>>> +	qspi->base = devm_ioremap_resource(&pdev->dev, r);
>>>> +	if (IS_ERR(qspi->base)) {
>>>> +		ret = PTR_ERR(qspi->base);
>>>> +		goto free_master;
>>>> +	}
>>>> +
>>>> +	ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr,
>>>> +			ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT,
>>> why do you need IRQF_NO_SUSPEND ?
>>>
>> I should get away with this.
> why ? Do you need or do you *not* need it ? And in either case, why ?
>
I was thinking, this will keep the irqs up even when we are
hitting suspend, we will not be prepared to handle it. ?

Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]

Hi,

On Mon, Jul 29, 2013 at 04:34:41PM +0530, Sourav Poddar wrote:
> >>>>+	irq = platform_get_irq(pdev, 0);
> >>>>+	if (irq<   0) {
> >>>>+		dev_err(&pdev->dev, "no irq resource?\n");
> >>>>+		return irq;
> >>>>+	}
> >>>>+
> >>>>+	spin_lock_init(&qspi->lock);
> >>>>+
> >>>>+	qspi->base = devm_ioremap_resource(&pdev->dev, r);
> >>>>+	if (IS_ERR(qspi->base)) {
> >>>>+		ret = PTR_ERR(qspi->base);
> >>>>+		goto free_master;
> >>>>+	}
> >>>>+
> >>>>+	ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr,
> >>>>+			ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT,
> >>>why do you need IRQF_NO_SUSPEND ?
> >>>
> >>I should get away with this.
> >why ? Do you need or do you *not* need it ? And in either case, why ?
> >
> I was thinking, this will keep the irqs up even when we are
> hitting suspend, we will not be prepared to handle it. ?

won't be prepared in what way ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller

[Sourav Poddar]

On Monday 29 July 2013 04:39 PM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Jul 29, 2013 at 04:34:41PM +0530, Sourav Poddar wrote:
>>>>>> +	irq = platform_get_irq(pdev, 0);
>>>>>> +	if (irq<    0) {
>>>>>> +		dev_err(&pdev->dev, "no irq resource?\n");
>>>>>> +		return irq;
>>>>>> +	}
>>>>>> +
>>>>>> +	spin_lock_init(&qspi->lock);
>>>>>> +
>>>>>> +	qspi->base = devm_ioremap_resource(&pdev->dev, r);
>>>>>> +	if (IS_ERR(qspi->base)) {
>>>>>> +		ret = PTR_ERR(qspi->base);
>>>>>> +		goto free_master;
>>>>>> +	}
>>>>>> +
>>>>>> +	ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr,
>>>>>> +			ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT,
>>>>> why do you need IRQF_NO_SUSPEND ?
>>>>>
>>>> I should get away with this.
>>> why ? Do you need or do you *not* need it ? And in either case, why ?
>>>
>> I was thinking, this will keep the irqs up even when we are
>> hitting suspend, we will not be prepared to handle it. ?
> won't be prepared in what way ?
>
Our driver will be down, so the irq might go un-serviced.


------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk

Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1960 bytes --]

Hi,

On Mon, Jul 29, 2013 at 04:45:02PM +0530, Sourav Poddar wrote:
> >>>>>>+	irq = platform_get_irq(pdev, 0);
> >>>>>>+	if (irq<    0) {
> >>>>>>+		dev_err(&pdev->dev, "no irq resource?\n");
> >>>>>>+		return irq;
> >>>>>>+	}
> >>>>>>+
> >>>>>>+	spin_lock_init(&qspi->lock);
> >>>>>>+
> >>>>>>+	qspi->base = devm_ioremap_resource(&pdev->dev, r);
> >>>>>>+	if (IS_ERR(qspi->base)) {
> >>>>>>+		ret = PTR_ERR(qspi->base);
> >>>>>>+		goto free_master;
> >>>>>>+	}
> >>>>>>+
> >>>>>>+	ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr,
> >>>>>>+			ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT,
> >>>>>why do you need IRQF_NO_SUSPEND ?
> >>>>>
> >>>>I should get away with this.
> >>>why ? Do you need or do you *not* need it ? And in either case, why ?
> >>>
> >>I was thinking, this will keep the irqs up even when we are
> >>hitting suspend, we will not be prepared to handle it. ?
> >won't be prepared in what way ?
> >
> Our driver will be down, so the irq might go un-serviced.

only if you wrote the driver that way. IRQ subsystem doesn't know the
state of the device, all it knows is that in case an IRQ fires, it needs
to call the registered IRQ handler.

Now the thing is:

Initially you had the flag setup, so I asked why you needed it. I
expected you to tell me why you think QSPI's IRQ shouldn't be disabled
during suspend and the implications of disabling it.

Instead you just changed your mind and decided to remove the flag.

Because you changed your mind with no explanation, that tells me you
haven't fully grasped how that flag works and what it means to set (or
not set) it.

My question now is simply: why don't you need that flag ? What are the
implications of setting that flag ? How would your driver behave if an
IRQ fired while your driver was suspended ? Unless you understand what
it does, how can you understand the behavior or the driver ?

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller

[Sourav Poddar]

Hi Felipe,
On Monday 29 July 2013 06:05 PM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Jul 29, 2013 at 04:45:02PM +0530, Sourav Poddar wrote:
>>>>>>>> +	irq = platform_get_irq(pdev, 0);
>>>>>>>> +	if (irq<     0) {
>>>>>>>> +		dev_err(&pdev->dev, "no irq resource?\n");
>>>>>>>> +		return irq;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	spin_lock_init(&qspi->lock);
>>>>>>>> +
>>>>>>>> +	qspi->base = devm_ioremap_resource(&pdev->dev, r);
>>>>>>>> +	if (IS_ERR(qspi->base)) {
>>>>>>>> +		ret = PTR_ERR(qspi->base);
>>>>>>>> +		goto free_master;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr,
>>>>>>>> +			ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT,
>>>>>>> why do you need IRQF_NO_SUSPEND ?
>>>>>>>
>>>>>> I should get away with this.
>>>>> why ? Do you need or do you *not* need it ? And in either case, why ?
>>>>>
>>>> I was thinking, this will keep the irqs up even when we are
>>>> hitting suspend, we will not be prepared to handle it. ?
>>> won't be prepared in what way ?
>>>
>> Our driver will be down, so the irq might go un-serviced.
> only if you wrote the driver that way. IRQ subsystem doesn't know the
> state of the device, all it knows is that in case an IRQ fires, it needs
> to call the registered IRQ handler.
>
> Now the thing is:
>
> Initially you had the flag setup, so I asked why you needed it. I
> expected you to tell me why you think QSPI's IRQ shouldn't be disabled
> during suspend and the implications of disabling it.
>
> Instead you just changed your mind and decided to remove the flag.
>
> Because you changed your mind with no explanation, that tells me you
> haven't fully grasped how that flag works and what it means to set (or
> not set) it.
>
> My question now is simply: why don't you need that flag ? What are the
> implications of setting that flag ? How would your driver behave if an
> IRQ fired while your driver was suspended ? Unless you understand what
> it does, how can you understand the behavior or the driver ?
>
> cheers
>
We dont need IRQF_NO_SUSPEND flag in our qspi controller.

Qspi driver need to be prevented from receiving interrupts during system 
wide suspend/hibernation.
"suspend_device_irqs" in kernel/irq/pm.c marks interrupt line in use and 
sets IRQS_SUSPENDED
for them.
If we use "IRQF_NO_SUSPEND", we will bypass setting this IRQS_SUSPENDED 
flag, which is not.
desired


For this, interrupt lines need to
  and this function is provided for this purpose.
  * It marks all interrupt lines in use, except for the timer ones, as 
disabled
  * and sets the IRQS_SUSPENDED flag for each of them.

This flag gets used in __disable_irq api(kernel/irq/manage.c) which

Re: [PATCHv6 1/2] drivers: spi: Add qspi flash controller

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3022 bytes --]

Hi,

On Tue, Jul 30, 2013 at 01:04:28PM +0530, Sourav Poddar wrote:
> Hi Felipe,
> On Monday 29 July 2013 06:05 PM, Felipe Balbi wrote:
> >Hi,
> >
> >On Mon, Jul 29, 2013 at 04:45:02PM +0530, Sourav Poddar wrote:
> >>>>>>>>+	irq = platform_get_irq(pdev, 0);
> >>>>>>>>+	if (irq<     0) {
> >>>>>>>>+		dev_err(&pdev->dev, "no irq resource?\n");
> >>>>>>>>+		return irq;
> >>>>>>>>+	}
> >>>>>>>>+
> >>>>>>>>+	spin_lock_init(&qspi->lock);
> >>>>>>>>+
> >>>>>>>>+	qspi->base = devm_ioremap_resource(&pdev->dev, r);
> >>>>>>>>+	if (IS_ERR(qspi->base)) {
> >>>>>>>>+		ret = PTR_ERR(qspi->base);
> >>>>>>>>+		goto free_master;
> >>>>>>>>+	}
> >>>>>>>>+
> >>>>>>>>+	ret = devm_request_threaded_irq(&pdev->dev, irq, ti_qspi_isr,
> >>>>>>>>+			ti_qspi_threaded_isr, IRQF_NO_SUSPEND | IRQF_ONESHOT,
> >>>>>>>why do you need IRQF_NO_SUSPEND ?
> >>>>>>>
> >>>>>>I should get away with this.
> >>>>>why ? Do you need or do you *not* need it ? And in either case, why ?
> >>>>>
> >>>>I was thinking, this will keep the irqs up even when we are
> >>>>hitting suspend, we will not be prepared to handle it. ?
> >>>won't be prepared in what way ?
> >>>
> >>Our driver will be down, so the irq might go un-serviced.
> >only if you wrote the driver that way. IRQ subsystem doesn't know the
> >state of the device, all it knows is that in case an IRQ fires, it needs
> >to call the registered IRQ handler.
> >
> >Now the thing is:
> >
> >Initially you had the flag setup, so I asked why you needed it. I
> >expected you to tell me why you think QSPI's IRQ shouldn't be disabled
> >during suspend and the implications of disabling it.
> >
> >Instead you just changed your mind and decided to remove the flag.
> >
> >Because you changed your mind with no explanation, that tells me you
> >haven't fully grasped how that flag works and what it means to set (or
> >not set) it.
> >
> >My question now is simply: why don't you need that flag ? What are the
> >implications of setting that flag ? How would your driver behave if an
> >IRQ fired while your driver was suspended ? Unless you understand what
> >it does, how can you understand the behavior or the driver ?
> >
> >cheers
> >
> We dont need IRQF_NO_SUSPEND flag in our qspi controller.
> 
> Qspi driver need to be prevented from receiving interrupts during
> system wide suspend/hibernation.
> "suspend_device_irqs" in kernel/irq/pm.c marks interrupt line in use
> and sets IRQS_SUSPENDED
> for them.
> If we use "IRQF_NO_SUSPEND", we will bypass setting this
> IRQS_SUSPENDED flag, which is not.
> desired
> 
> 
> For this, interrupt lines need to
>  and this function is provided for this purpose.
>  * It marks all interrupt lines in use, except for the timer ones, as
> disabled
>  * and sets the IRQS_SUSPENDED flag for each of them.
> 
> This flag gets used in __disable_irq api(kernel/irq/manage.c) which

some formatting issues in the mail, but good that you looked at the
code. Cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[RFC/PATCH 2/2] driver: spi: Add quad spi read support

[Sourav Poddar]

Since, qspi controller uses quad read.

Configuring the command register, if the transfer of data needs
dual or quad lines.

This patch has been done on top of the following patch[1], which is just the
basic idea of adding dual/quad support in spi framework.
$subject patch will undergo changes with the ongoing discussion in the
community.

This patch is posted to demonstrate how patch 1 of the series will support
quad read.

[1]: http://comments.gmane.org/gmane.linux.kernel.spi.devel/14047

Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
 drivers/spi/spi-ti-qspi.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
index 51fe95f..8a32b1c 100644
--- a/drivers/spi/spi-ti-qspi.c
+++ b/drivers/spi/spi-ti-qspi.c
@@ -86,6 +86,7 @@ struct ti_qspi {
 #define QSPI_3_PIN			(1 << 18)
 #define QSPI_RD_SNGL			(1 << 16)
 #define QSPI_WR_SNGL			(2 << 16)
+#define	QSPI_RD_DUAL			(3 << 16)
 #define QSPI_RD_QUAD			(7 << 16)
 #define QSPI_INVAL			(4 << 16)
 #define QSPI_WC_CMD_INT_EN			(1 << 14)
@@ -264,6 +265,7 @@ static void qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t)
 {
 	u8 *rxbuf;
 	int wlen, count;
+	unsigned cmd = qspi->cmd;
 
 	count = t->len;
 	rxbuf = t->rx_buf;
@@ -273,8 +275,18 @@ static void qspi_read_msg(struct ti_qspi *qspi, struct spi_transfer *t)
 		dev_dbg(qspi->dev, "rx cmd %08x dc %08x\n",
 			qspi->cmd | QSPI_RD_SNGL, qspi->dc);
 		ti_qspi_write(qspi, qspi->dc, QSPI_SPI_DC_REG);
-		ti_qspi_write(qspi, qspi->cmd | QSPI_RD_SNGL,
-			QSPI_SPI_CMD_REG);
+		switch (t->bitwidth) {
+		case SPI_BITWIDTH_QUAD:
+			cmd |= QSPI_RD_QUAD;
+			break;
+		case SPI_BITWIDTH_DUAL:
+			cmd |= QSPI_RD_DUAL;
+			break;
+		case SPI_BITWIDTH_SINGLE:
+		default:
+			cmd |= QSPI_RD_SNGL;
+		}
+		ti_qspi_writel(qspi, cmd, QSPI_SPI_CMD_REG);
 		ti_qspi_write(qspi, QSPI_WC_INT_EN, QSPI_INTR_ENABLE_SET_REG);
 		wait_for_completion(&qspi->transfer_complete);
 		ti_qspi_read_data(qspi, QSPI_SPI_DATA_REG, wlen, &rxbuf);
-- 
1.7.1

Re: [RFC/PATCH 2/2] driver: spi: Add quad spi read support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]

On Mon, Jul 29, 2013 at 12:52:30PM +0530, Sourav Poddar wrote:
> Since, qspi controller uses quad read.
> 
> Configuring the command register, if the transfer of data needs
> dual or quad lines.
> 
> This patch has been done on top of the following patch[1], which is just the
> basic idea of adding dual/quad support in spi framework.
> $subject patch will undergo changes with the ongoing discussion in the
> community.
> 
> This patch is posted to demonstrate how patch 1 of the series will support
> quad read.
> 
> [1]: http://comments.gmane.org/gmane.linux.kernel.spi.devel/14047
> 
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
>  drivers/spi/spi-ti-qspi.c |   16 ++++++++++++++--
>  1 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
> index 51fe95f..8a32b1c 100644
> --- a/drivers/spi/spi-ti-qspi.c
> +++ b/drivers/spi/spi-ti-qspi.c
> @@ -86,6 +86,7 @@ struct ti_qspi {
>  #define QSPI_3_PIN			(1 << 18)
>  #define QSPI_RD_SNGL			(1 << 16)
>  #define QSPI_WR_SNGL			(2 << 16)
> +#define	QSPI_RD_DUAL			(3 << 16)
>  #define QSPI_RD_QUAD			(7 << 16)

so RD_QUAD is defined in previous patch but not RD_DUAL ? What gives ?

IMHO, just merge this patch with previous and make the entire driver
depend on the other patch.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC/PATCH 2/2] driver: spi: Add quad spi read support

[Sourav Poddar]

On Monday 29 July 2013 03:02 PM, Felipe Balbi wrote:
> On Mon, Jul 29, 2013 at 12:52:30PM +0530, Sourav Poddar wrote:
>> Since, qspi controller uses quad read.
>>
>> Configuring the command register, if the transfer of data needs
>> dual or quad lines.
>>
>> This patch has been done on top of the following patch[1], which is just the
>> basic idea of adding dual/quad support in spi framework.
>> $subject patch will undergo changes with the ongoing discussion in the
>> community.
>>
>> This patch is posted to demonstrate how patch 1 of the series will support
>> quad read.
>>
>> [1]: http://comments.gmane.org/gmane.linux.kernel.spi.devel/14047
>>
>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
>> ---
>>   drivers/spi/spi-ti-qspi.c |   16 ++++++++++++++--
>>   1 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
>> index 51fe95f..8a32b1c 100644
>> --- a/drivers/spi/spi-ti-qspi.c
>> +++ b/drivers/spi/spi-ti-qspi.c
>> @@ -86,6 +86,7 @@ struct ti_qspi {
>>   #define QSPI_3_PIN			(1<<  18)
>>   #define QSPI_RD_SNGL			(1<<  16)
>>   #define QSPI_WR_SNGL			(2<<  16)
>> +#define	QSPI_RD_DUAL			(3<<  16)
>>   #define QSPI_RD_QUAD			(7<<  16)
> so RD_QUAD is defined in previous patch but not RD_DUAL ? What gives ?
>
I think I will define RD_DUAL in the previous patch...
> IMHO, just merge this patch with previous and make the entire driver
> depend on the other patch.
>
but I was thinking of keeping rest of the $subject patch seperate, since the
idea of implementing dual/quad read in spi framework is still under 
discussion.
and this patch will change.

So, it would be good if we can get the basic previous patch in, and then 
when the
other discussion is sorted out, we can get this in?

~Sourav

Re: [RFC/PATCH 2/2] driver: spi: Add quad spi read support

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2007 bytes --]

On Mon, Jul 29, 2013 at 03:13:07PM +0530, Sourav Poddar wrote:
> On Monday 29 July 2013 03:02 PM, Felipe Balbi wrote:
> >On Mon, Jul 29, 2013 at 12:52:30PM +0530, Sourav Poddar wrote:
> >>Since, qspi controller uses quad read.
> >>
> >>Configuring the command register, if the transfer of data needs
> >>dual or quad lines.
> >>
> >>This patch has been done on top of the following patch[1], which is just the
> >>basic idea of adding dual/quad support in spi framework.
> >>$subject patch will undergo changes with the ongoing discussion in the
> >>community.
> >>
> >>This patch is posted to demonstrate how patch 1 of the series will support
> >>quad read.
> >>
> >>[1]: http://comments.gmane.org/gmane.linux.kernel.spi.devel/14047
> >>
> >>Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
> >>---
> >>  drivers/spi/spi-ti-qspi.c |   16 ++++++++++++++--
> >>  1 files changed, 14 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
> >>index 51fe95f..8a32b1c 100644
> >>--- a/drivers/spi/spi-ti-qspi.c
> >>+++ b/drivers/spi/spi-ti-qspi.c
> >>@@ -86,6 +86,7 @@ struct ti_qspi {
> >>  #define QSPI_3_PIN			(1<<  18)
> >>  #define QSPI_RD_SNGL			(1<<  16)
> >>  #define QSPI_WR_SNGL			(2<<  16)
> >>+#define	QSPI_RD_DUAL			(3<<  16)
> >>  #define QSPI_RD_QUAD			(7<<  16)
> >so RD_QUAD is defined in previous patch but not RD_DUAL ? What gives ?
> >
> I think I will define RD_DUAL in the previous patch...
> >IMHO, just merge this patch with previous and make the entire driver
> >depend on the other patch.
> >
> but I was thinking of keeping rest of the $subject patch seperate, since the
> idea of implementing dual/quad read in spi framework is still under
> discussion.
> and this patch will change.
> 
> So, it would be good if we can get the basic previous patch in, and
> then when the
> other discussion is sorted out, we can get this in?

alright, in that case it makes sense.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]