[PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Marek Vasut]

From: Graham Moore <grmoore@opensource.altera.com>

Add binding document for the Cadence QSPI controller.

Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alan Tull <atull@opensource.altera.com>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Graham Moore <grmoore@opensource.altera.com>
Cc: "R, Vignesh" <vigneshr@ti.com>
Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
Cc: devicetree@vger.kernel.org
---
 .../devicetree/bindings/mtd/cadence-quadspi.txt    | 56 ++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/cadence-quadspi.txt

V2: Add cdns prefix to driver-specific bindings.
V3: Use existing property "is-decoded-cs" instead of creating a
    duplicate, "ext-decoder". Timing parameters are in nanoseconds,
    not master reference clocks. Remove bus-num completely.
V4: Add new properties fifo-width and trigger-address
V7: - Prefix all of the Cadence-specific properties with cdns prefix,
      those are in particular "cdns,is-decoded-cs", "cdns,fifo-depth",
      "cdns,fifo-width", "cdns,trigger-address".
    - Drop bogus properties which were not used and were incorrect.
V8: Align lines to 80 chars.

diff --git a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
new file mode 100644
index 0000000..f248056
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
@@ -0,0 +1,56 @@
+* Cadence Quad SPI controller
+
+Required properties:
+- compatible : Should be "cdns,qspi-nor".
+- reg : Contains two entries, each of which is a tuple consisting of a
+	physical address and length. The first entry is the address and
+	length of the controller register set. The second entry is the
+	address and length of the QSPI Controller data area.
+- interrupts : Unit interrupt specifier for the controller interrupt.
+- clocks : phandle to the Quad SPI clock.
+- cdns,fifo-depth : Size of the data FIFO in words.
+- cdns,fifo-width : Bus width of the data FIFO in bytes.
+- cdns,trigger-address : 32-bit indirect AHB trigger address.
+
+Optional properties:
+- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not.
+
+Optional subnodes:
+Subnodes of the Cadence Quad SPI controller are spi slave nodes with additional
+custom properties:
+- cdns,read-delay : Delay for read capture logic, in clock cycles
+- cdns,tshsl-ns : Delay in nanoseconds for the length that the master
+                  mode chip select outputs are de-asserted between
+		  transactions.
+- cdns,tsd2d-ns : Delay in nanoseconds between one chip select being
+                  de-activated and the activation of another.
+- cdns,tchsh-ns : Delay in nanoseconds between last bit of current
+                  transaction and deasserting the device chip select
+		  (qspi_n_ss_out).
+- cdns,tslch-ns : Delay in nanoseconds between setting qspi_n_ss_out low
+                  and first bit transfer.
+
+Example:
+
+	qspi: spi@ff705000 {
+		compatible = "cdns,qspi-nor";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0xff705000 0x1000>,
+		      <0xffa00000 0x1000>;
+		interrupts = <0 151 4>;
+		clocks = <&qspi_clk>;
+		cdns,is-decoded-cs;
+		cdns,fifo-depth = <128>;
+		cdns,fifo-width = <4>;
+		cdns,trigger-address = <0x00000000>;
+
+		flash0: n25q00@0 {
+			...
+			cdns,read-delay = <4>;
+			cdns,tshsl-ns = <50>;
+			cdns,tsd2d-ns = <50>;
+			cdns,tchsh-ns = <4>;
+			cdns,tslch-ns = <4>;
+		};
+	};
-- 
2.1.4

[PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Marek Vasut]

From: Graham Moore <grmoore@opensource.altera.com>

Add support for the Cadence QSPI controller. This controller is
present in the Altera SoCFPGA SoCs and this driver has been tested
on the Cyclone V SoC.

Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alan Tull <atull@opensource.altera.com>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Graham Moore <grmoore@opensource.altera.com>
Cc: "R, Vignesh" <vigneshr@ti.com>
Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
Cc: devicetree@vger.kernel.org
---
 drivers/mtd/spi-nor/Kconfig           |   11 +
 drivers/mtd/spi-nor/Makefile          |    1 +
 drivers/mtd/spi-nor/cadence-quadspi.c | 1280 +++++++++++++++++++++++++++++++++
 3 files changed, 1292 insertions(+)
 create mode 100644 drivers/mtd/spi-nor/cadence-quadspi.c

V2: use NULL instead of modalias in spi_nor_scan call
V3: Use existing property is-decoded-cs instead of creating duplicate.
V4: Support Micron quad mode by snooping command stream for EVCR command
    and subsequently configuring Cadence controller for quad mode.
V5: Clean up sparse and smatch complaints.  Remove snooping of Micron
    quad mode.  Add comment on XIP mode bit and dummy clock cycles.  Set
    up SRAM partition at 1:1 during init.
V6: Remove dts patch that was included by mistake.  Incorporate Vikas's
    comments regarding fifo width, SRAM partition setting, and trigger
    address.  Trigger address was added as an unsigned int, as it is not
    an IO resource per se, and does not need to be mapped. Also add
    Marek Vasut's workaround for picking up OF properties on subnodes.
V7: - Perform coding-style cleanup and type fixes. Remove ugly QSPI_*()
      macros and replace them with functions. Get rid of unused variables.
    - Implement support for nor->set_protocol() to handle Quad-command,
      this patch now depends on the following patch:
      mtd: spi-nor: notify (Q)SPI controller about protocol change
    - Replace that cqspi_fifo_read() disaster with plain old readsl()
      and cqspi_fifo_write() tentacle horror with pretty writesl().
    - Remove CQSPI_SUPPORT_XIP_CHIPS, which is broken.
    - Get rid of cqspi_find_chipselect() mess, instead just place the
      struct cqspi_st and chipselect number into struct cqspi_flash_pdata
      and set nor->priv to the struct cqspi_flash_pdata of that particular
      chip.
    - Replace the odd math in calculate_ticks_for_ns() with DIV_ROUND_UP().
    - Make variables const where applicable.
V8: - Implement a function to wait for bit being set/unset for a given
      period of time and use it to replace the ad-hoc bits of code.
    - Configure the write underflow watermark to be 1/8 if FIFO size.
    - Extract out the SPI NOR flash probing code into separate function
      to clearly mark what will soon be considered a boilerplate code.
    - Repair the handling of mode bits, which caused instability in V7.
    - Clean up the interrupt handling
    - Fix Kconfig help text and make the patch depend on OF and COMPILE_TEST.
V9: - Rename CQSPI_REG_IRQ_IND_RD_OVERFLOW to CQSPI_REG_IRQ_IND_SRAM_FULL
    - Merge cqspi_controller_disable() into cqspi_controller_enable() and
      make the mode selectable via parameter.
V10: - Update against Cyrille's new patchset and changes to linux-mtd.
     - Repair problem with multiple QSPI NOR devices having the same mtd->name,
       they are now named devname.cs , where cs is the chipselect ID.

diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 43bafde..7e29050 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -50,4 +50,15 @@ config SPI_NXP_SPIFI
 	  Flash. Enable this option if you have a device with a SPIFI
 	  controller and want to access the Flash as a mtd device.
 
+config SPI_CADENCE_QUADSPI
+	tristate "Cadence Quad SPI controller"
+	depends on OF && (ARCH_SOCFPGA || COMPILE_TEST)
+	help
+	  Enable support for the Cadence Quad SPI Flash controller.
+
+	  Cadence QSPI is a specialized controller for connecting an SPI
+	  Flash over 1/2/4-bit wide bus. Enable this option if you have a
+	  device with a Cadence QSPI controller and want to access the
+	  Flash as an MTD device.
+
 endif # MTD_SPI_NOR
diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 5d80c9d..8ced75e 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
+obj-$(CONFIG_SPI_CADENCE_QUADSPI)	+= cadence-quadspi.o
 obj-$(CONFIG_SPI_FSL_QUADSPI)	+= fsl-quadspi.o
 obj-$(CONFIG_SPI_NXP_SPIFI)	+= nxp-spifi.o
 obj-$(CONFIG_SPI_ATMEL_QUADSPI)	+= atmel-quadspi.o
diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
new file mode 100644
index 0000000..d9a7a67
--- /dev/null
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -0,0 +1,1280 @@
+/*
+ * Driver for Cadence QSPI Controller
+ *
+ * Copyright Altera Corporation (C) 2012-2014. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <linux/mtd/spi-nor.h>
+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/spi/spi.h>
+#include <linux/timer.h>
+
+#define CQSPI_NAME			"cadence-qspi"
+#define CQSPI_MAX_CHIPSELECT		16
+
+struct cqspi_st;
+
+struct cqspi_flash_pdata {
+	struct spi_nor	nor;
+	struct cqspi_st	*cqspi;
+	u32		clk_rate;
+	u32		read_delay;
+	u32		tshsl_ns;
+	u32		tsd2d_ns;
+	u32		tchsh_ns;
+	u32		tslch_ns;
+	u8		inst_width;
+	u8		addr_width;
+	u8		cs;
+};
+
+struct cqspi_st {
+	struct platform_device	*pdev;
+
+	struct clk		*clk;
+	unsigned int		sclk;
+
+	void __iomem		*iobase;
+	void __iomem		*ahb_base;
+	struct completion	transfer_complete;
+
+	int			current_cs;
+	unsigned long		master_ref_clk_hz;
+	bool			is_decoded_cs;
+	u32			fifo_depth;
+	u32			fifo_width;
+	u32			trigger_address;
+	struct cqspi_flash_pdata f_pdata[CQSPI_MAX_CHIPSELECT];
+};
+
+/* Operation timeout value */
+#define CQSPI_TIMEOUT_MS			500
+#define CQSPI_READ_TIMEOUT_MS			10
+
+/* Instruction type */
+#define CQSPI_INST_TYPE_SINGLE			0
+#define CQSPI_INST_TYPE_DUAL			1
+#define CQSPI_INST_TYPE_QUAD			2
+
+#define CQSPI_DUMMY_CLKS_PER_BYTE		8
+#define CQSPI_DUMMY_BYTES_MAX			4
+#define CQSPI_DUMMY_CLKS_MAX			31
+
+#define CQSPI_STIG_DATA_LEN_MAX			8
+
+/* Register map */
+#define CQSPI_REG_CONFIG			0x00
+#define CQSPI_REG_CONFIG_ENABLE_MASK		BIT(0)
+#define CQSPI_REG_CONFIG_DECODE_MASK		BIT(9)
+#define CQSPI_REG_CONFIG_CHIPSELECT_LSB		10
+#define CQSPI_REG_CONFIG_DMA_MASK		BIT(15)
+#define CQSPI_REG_CONFIG_BAUD_LSB		19
+#define CQSPI_REG_CONFIG_IDLE_LSB		31
+#define CQSPI_REG_CONFIG_CHIPSELECT_MASK	0xF
+#define CQSPI_REG_CONFIG_BAUD_MASK		0xF
+
+#define CQSPI_REG_RD_INSTR			0x04
+#define CQSPI_REG_RD_INSTR_OPCODE_LSB		0
+#define CQSPI_REG_RD_INSTR_TYPE_INSTR_LSB	8
+#define CQSPI_REG_RD_INSTR_TYPE_ADDR_LSB	12
+#define CQSPI_REG_RD_INSTR_TYPE_DATA_LSB	16
+#define CQSPI_REG_RD_INSTR_MODE_EN_LSB		20
+#define CQSPI_REG_RD_INSTR_DUMMY_LSB		24
+#define CQSPI_REG_RD_INSTR_TYPE_INSTR_MASK	0x3
+#define CQSPI_REG_RD_INSTR_TYPE_ADDR_MASK	0x3
+#define CQSPI_REG_RD_INSTR_TYPE_DATA_MASK	0x3
+#define CQSPI_REG_RD_INSTR_DUMMY_MASK		0x1F
+
+#define CQSPI_REG_WR_INSTR			0x08
+#define CQSPI_REG_WR_INSTR_OPCODE_LSB		0
+#define CQSPI_REG_WR_INSTR_TYPE_ADDR_LSB	12
+#define CQSPI_REG_WR_INSTR_TYPE_DATA_LSB	16
+
+#define CQSPI_REG_DELAY				0x0C
+#define CQSPI_REG_DELAY_TSLCH_LSB		0
+#define CQSPI_REG_DELAY_TCHSH_LSB		8
+#define CQSPI_REG_DELAY_TSD2D_LSB		16
+#define CQSPI_REG_DELAY_TSHSL_LSB		24
+#define CQSPI_REG_DELAY_TSLCH_MASK		0xFF
+#define CQSPI_REG_DELAY_TCHSH_MASK		0xFF
+#define CQSPI_REG_DELAY_TSD2D_MASK		0xFF
+#define CQSPI_REG_DELAY_TSHSL_MASK		0xFF
+
+#define CQSPI_REG_READCAPTURE			0x10
+#define CQSPI_REG_READCAPTURE_BYPASS_LSB	0
+#define CQSPI_REG_READCAPTURE_DELAY_LSB		1
+#define CQSPI_REG_READCAPTURE_DELAY_MASK	0xF
+
+#define CQSPI_REG_SIZE				0x14
+#define CQSPI_REG_SIZE_ADDRESS_LSB		0
+#define CQSPI_REG_SIZE_PAGE_LSB			4
+#define CQSPI_REG_SIZE_BLOCK_LSB		16
+#define CQSPI_REG_SIZE_ADDRESS_MASK		0xF
+#define CQSPI_REG_SIZE_PAGE_MASK		0xFFF
+#define CQSPI_REG_SIZE_BLOCK_MASK		0x3F
+
+#define CQSPI_REG_SRAMPARTITION			0x18
+#define CQSPI_REG_INDIRECTTRIGGER		0x1C
+
+#define CQSPI_REG_DMA				0x20
+#define CQSPI_REG_DMA_SINGLE_LSB		0
+#define CQSPI_REG_DMA_BURST_LSB			8
+#define CQSPI_REG_DMA_SINGLE_MASK		0xFF
+#define CQSPI_REG_DMA_BURST_MASK		0xFF
+
+#define CQSPI_REG_REMAP				0x24
+#define CQSPI_REG_MODE_BIT			0x28
+
+#define CQSPI_REG_SDRAMLEVEL			0x2C
+#define CQSPI_REG_SDRAMLEVEL_RD_LSB		0
+#define CQSPI_REG_SDRAMLEVEL_WR_LSB		16
+#define CQSPI_REG_SDRAMLEVEL_RD_MASK		0xFFFF
+#define CQSPI_REG_SDRAMLEVEL_WR_MASK		0xFFFF
+
+#define CQSPI_REG_IRQSTATUS			0x40
+#define CQSPI_REG_IRQMASK			0x44
+
+#define CQSPI_REG_INDIRECTRD			0x60
+#define CQSPI_REG_INDIRECTRD_START_MASK		BIT(0)
+#define CQSPI_REG_INDIRECTRD_CANCEL_MASK	BIT(1)
+#define CQSPI_REG_INDIRECTRD_DONE_MASK		BIT(5)
+
+#define CQSPI_REG_INDIRECTRDWATERMARK		0x64
+#define CQSPI_REG_INDIRECTRDSTARTADDR		0x68
+#define CQSPI_REG_INDIRECTRDBYTES		0x6C
+
+#define CQSPI_REG_CMDCTRL			0x90
+#define CQSPI_REG_CMDCTRL_EXECUTE_MASK		BIT(0)
+#define CQSPI_REG_CMDCTRL_INPROGRESS_MASK	BIT(1)
+#define CQSPI_REG_CMDCTRL_WR_BYTES_LSB		12
+#define CQSPI_REG_CMDCTRL_WR_EN_LSB		15
+#define CQSPI_REG_CMDCTRL_ADD_BYTES_LSB		16
+#define CQSPI_REG_CMDCTRL_ADDR_EN_LSB		19
+#define CQSPI_REG_CMDCTRL_RD_BYTES_LSB		20
+#define CQSPI_REG_CMDCTRL_RD_EN_LSB		23
+#define CQSPI_REG_CMDCTRL_OPCODE_LSB		24
+#define CQSPI_REG_CMDCTRL_WR_BYTES_MASK		0x7
+#define CQSPI_REG_CMDCTRL_ADD_BYTES_MASK	0x3
+#define CQSPI_REG_CMDCTRL_RD_BYTES_MASK		0x7
+
+#define CQSPI_REG_INDIRECTWR			0x70
+#define CQSPI_REG_INDIRECTWR_START_MASK		BIT(0)
+#define CQSPI_REG_INDIRECTWR_CANCEL_MASK	BIT(1)
+#define CQSPI_REG_INDIRECTWR_DONE_MASK		BIT(5)
+
+#define CQSPI_REG_INDIRECTWRWATERMARK		0x74
+#define CQSPI_REG_INDIRECTWRSTARTADDR		0x78
+#define CQSPI_REG_INDIRECTWRBYTES		0x7C
+
+#define CQSPI_REG_CMDADDRESS			0x94
+#define CQSPI_REG_CMDREADDATALOWER		0xA0
+#define CQSPI_REG_CMDREADDATAUPPER		0xA4
+#define CQSPI_REG_CMDWRITEDATALOWER		0xA8
+#define CQSPI_REG_CMDWRITEDATAUPPER		0xAC
+
+/* Interrupt status bits */
+#define CQSPI_REG_IRQ_MODE_ERR			BIT(0)
+#define CQSPI_REG_IRQ_UNDERFLOW			BIT(1)
+#define CQSPI_REG_IRQ_IND_COMP			BIT(2)
+#define CQSPI_REG_IRQ_IND_RD_REJECT		BIT(3)
+#define CQSPI_REG_IRQ_WR_PROTECTED_ERR		BIT(4)
+#define CQSPI_REG_IRQ_ILLEGAL_AHB_ERR		BIT(5)
+#define CQSPI_REG_IRQ_WATERMARK			BIT(6)
+#define CQSPI_REG_IRQ_IND_SRAM_FULL		BIT(12)
+
+#define CQSPI_IRQ_MASK_RD		(CQSPI_REG_IRQ_WATERMARK	| \
+					 CQSPI_REG_IRQ_IND_SRAM_FULL	| \
+					 CQSPI_REG_IRQ_IND_COMP)
+
+#define CQSPI_IRQ_MASK_WR		(CQSPI_REG_IRQ_IND_COMP		| \
+					 CQSPI_REG_IRQ_WATERMARK	| \
+					 CQSPI_REG_IRQ_UNDERFLOW)
+
+#define CQSPI_IRQ_STATUS_MASK		0x1FFFF
+
+static int cqspi_wait_for_bit(void __iomem *reg, const u32 mask, bool clear)
+{
+	unsigned long end = jiffies + msecs_to_jiffies(CQSPI_TIMEOUT_MS);
+	u32 val;
+
+	while (1) {
+		val = readl(reg);
+		if (clear)
+			val = ~val;
+		val &= mask;
+
+		if (val == mask)
+			return 0;
+
+		if (time_after(jiffies, end))
+			return -ETIMEDOUT;
+	}
+}
+
+static bool cqspi_is_idle(struct cqspi_st *cqspi)
+{
+	u32 reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
+
+	return reg & (1 << CQSPI_REG_CONFIG_IDLE_LSB);
+}
+
+static u32 cqspi_get_rd_sram_level(struct cqspi_st *cqspi)
+{
+	u32 reg = readl(cqspi->iobase + CQSPI_REG_SDRAMLEVEL);
+
+	reg >>= CQSPI_REG_SDRAMLEVEL_RD_LSB;
+	return reg & CQSPI_REG_SDRAMLEVEL_RD_MASK;
+}
+
+static irqreturn_t cqspi_irq_handler(int this_irq, void *dev)
+{
+	struct cqspi_st *cqspi = dev;
+	unsigned int irq_status;
+
+	/* Read interrupt status */
+	irq_status = readl(cqspi->iobase + CQSPI_REG_IRQSTATUS);
+
+	/* Clear interrupt */
+	writel(irq_status, cqspi->iobase + CQSPI_REG_IRQSTATUS);
+
+	irq_status &= CQSPI_IRQ_MASK_RD | CQSPI_IRQ_MASK_WR;
+
+	if (irq_status)
+		complete(&cqspi->transfer_complete);
+
+	return IRQ_HANDLED;
+}
+
+static unsigned int cqspi_calc_rdreg(struct spi_nor *nor, const u8 opcode)
+{
+	unsigned int rdreg = 0;
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+
+	rdreg |= f_pdata->inst_width << CQSPI_REG_RD_INSTR_TYPE_INSTR_LSB;
+	rdreg |= f_pdata->addr_width << CQSPI_REG_RD_INSTR_TYPE_ADDR_LSB;
+
+	if (nor->flash_read == SPI_NOR_QUAD)
+		rdreg |= CQSPI_INST_TYPE_QUAD
+			 << CQSPI_REG_RD_INSTR_TYPE_DATA_LSB;
+	return rdreg;
+}
+
+static int cqspi_wait_idle(struct cqspi_st *cqspi)
+{
+	const unsigned int poll_idle_retry = 3;
+	unsigned int count = 0;
+	unsigned long timeout;
+
+	timeout = jiffies + msecs_to_jiffies(CQSPI_TIMEOUT_MS);
+	while (1) {
+		/*
+		 * Read few times in succession to ensure the controller
+		 * is indeed idle, that is, the bit does not transition
+		 * low again.
+		 */
+		if (cqspi_is_idle(cqspi))
+			count++;
+		else
+			count = 0;
+
+		if (count >= poll_idle_retry)
+			return 0;
+
+		if (time_after(jiffies, timeout)) {
+			/* Timeout, in busy mode. */
+			dev_err(&cqspi->pdev->dev,
+				"QSPI is still busy after %dms timeout.\n",
+				CQSPI_TIMEOUT_MS);
+			return -ETIMEDOUT;
+		}
+
+		cpu_relax();
+	}
+}
+
+static int cqspi_exec_flash_cmd(struct cqspi_st *cqspi, unsigned int reg)
+{
+	void __iomem *reg_base = cqspi->iobase;
+	int ret;
+
+	/* Write the CMDCTRL without start execution. */
+	writel(reg, reg_base + CQSPI_REG_CMDCTRL);
+	/* Start execute */
+	reg |= CQSPI_REG_CMDCTRL_EXECUTE_MASK;
+	writel(reg, reg_base + CQSPI_REG_CMDCTRL);
+
+	/* Polling for completion. */
+	ret = cqspi_wait_for_bit(reg_base + CQSPI_REG_CMDCTRL,
+				 CQSPI_REG_CMDCTRL_INPROGRESS_MASK, 1);
+	if (ret) {
+		dev_err(&cqspi->pdev->dev,
+			"Flash command execution timed out.\n");
+		return ret;
+	}
+
+	/* Polling QSPI idle status. */
+	return cqspi_wait_idle(cqspi);
+}
+
+static int cqspi_command_read(struct spi_nor *nor,
+			      const u8 *txbuf, const unsigned n_tx,
+			      u8 *rxbuf, const unsigned n_rx)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	void __iomem *reg_base = cqspi->iobase;
+	unsigned int rdreg;
+	unsigned int reg;
+	unsigned int read_len;
+	int status;
+
+	if (!n_rx || n_rx > CQSPI_STIG_DATA_LEN_MAX || !rxbuf) {
+		dev_err(nor->dev, "Invalid input argument, len %d rxbuf 0x%p\n",
+			n_rx, rxbuf);
+		return -EINVAL;
+	}
+
+	reg = txbuf[0] << CQSPI_REG_CMDCTRL_OPCODE_LSB;
+
+	rdreg = cqspi_calc_rdreg(nor, txbuf[0]);
+	writel(rdreg, reg_base + CQSPI_REG_RD_INSTR);
+
+	reg |= (0x1 << CQSPI_REG_CMDCTRL_RD_EN_LSB);
+
+	/* 0 means 1 byte. */
+	reg |= (((n_rx - 1) & CQSPI_REG_CMDCTRL_RD_BYTES_MASK)
+		<< CQSPI_REG_CMDCTRL_RD_BYTES_LSB);
+	status = cqspi_exec_flash_cmd(cqspi, reg);
+	if (status)
+		return status;
+
+	reg = readl(reg_base + CQSPI_REG_CMDREADDATALOWER);
+
+	/* Put the read value into rx_buf */
+	read_len = (n_rx > 4) ? 4 : n_rx;
+	memcpy(rxbuf, &reg, read_len);
+	rxbuf += read_len;
+
+	if (n_rx > 4) {
+		reg = readl(reg_base + CQSPI_REG_CMDREADDATAUPPER);
+
+		read_len = n_rx - read_len;
+		memcpy(rxbuf, &reg, read_len);
+	}
+
+	return 0;
+}
+
+static int cqspi_command_write(struct spi_nor *nor, const u8 opcode,
+			       const u8 *txbuf, const unsigned n_tx)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	void __iomem *reg_base = cqspi->iobase;
+	unsigned int reg;
+	unsigned int data;
+	int ret;
+
+	if (n_tx > 4 || (n_tx && !txbuf)) {
+		dev_err(nor->dev,
+			"Invalid input argument, cmdlen %d txbuf 0x%p\n",
+			n_tx, txbuf);
+		return -EINVAL;
+	}
+
+	reg = opcode << CQSPI_REG_CMDCTRL_OPCODE_LSB;
+	if (n_tx) {
+		reg |= (0x1 << CQSPI_REG_CMDCTRL_WR_EN_LSB);
+		reg |= ((n_tx - 1) & CQSPI_REG_CMDCTRL_WR_BYTES_MASK)
+			<< CQSPI_REG_CMDCTRL_WR_BYTES_LSB;
+		data = 0;
+		memcpy(&data, txbuf, n_tx);
+		writel(data, reg_base + CQSPI_REG_CMDWRITEDATALOWER);
+	}
+
+	ret = cqspi_exec_flash_cmd(cqspi, reg);
+	return ret;
+}
+
+static int cqspi_command_write_addr(struct spi_nor *nor,
+				    const u8 opcode, const unsigned int addr)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	void __iomem *reg_base = cqspi->iobase;
+	unsigned int reg;
+
+	reg = opcode << CQSPI_REG_CMDCTRL_OPCODE_LSB;
+	reg |= (0x1 << CQSPI_REG_CMDCTRL_ADDR_EN_LSB);
+	reg |= ((nor->addr_width - 1) & CQSPI_REG_CMDCTRL_ADD_BYTES_MASK)
+		<< CQSPI_REG_CMDCTRL_ADD_BYTES_LSB;
+
+	writel(addr, reg_base + CQSPI_REG_CMDADDRESS);
+
+	return cqspi_exec_flash_cmd(cqspi, reg);
+}
+
+static int cqspi_indirect_read_setup(struct spi_nor *nor,
+				     const unsigned int from_addr)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	void __iomem *reg_base = cqspi->iobase;
+	unsigned int dummy_clk = 0;
+	unsigned int reg;
+
+	writel(from_addr, reg_base + CQSPI_REG_INDIRECTRDSTARTADDR);
+
+	reg = nor->read_opcode << CQSPI_REG_RD_INSTR_OPCODE_LSB;
+	reg |= cqspi_calc_rdreg(nor, nor->read_opcode);
+
+	/* Setup dummy clock cycles */
+	dummy_clk = nor->read_dummy;
+	if (dummy_clk > CQSPI_DUMMY_CLKS_MAX)
+		dummy_clk = CQSPI_DUMMY_CLKS_MAX;
+
+	if (dummy_clk / 8) {
+		reg |= (1 << CQSPI_REG_RD_INSTR_MODE_EN_LSB);
+		/* Set mode bits high to ensure chip doesn't enter XIP */
+		writel(0xFF, reg_base + CQSPI_REG_MODE_BIT);
+
+		/* Need to subtract the mode byte (8 clocks). */
+		if (f_pdata->inst_width != CQSPI_INST_TYPE_QUAD)
+			dummy_clk -= 8;
+
+		if (dummy_clk)
+			reg |= (dummy_clk & CQSPI_REG_RD_INSTR_DUMMY_MASK)
+			       << CQSPI_REG_RD_INSTR_DUMMY_LSB;
+	}
+
+	writel(reg, reg_base + CQSPI_REG_RD_INSTR);
+
+	/* Set address width */
+	reg = readl(reg_base + CQSPI_REG_SIZE);
+	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
+	reg |= (nor->addr_width - 1);
+	writel(reg, reg_base + CQSPI_REG_SIZE);
+	return 0;
+}
+
+static int cqspi_indirect_read_execute(struct spi_nor *nor,
+				       u8 *rxbuf, const unsigned n_rx)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	void __iomem *reg_base = cqspi->iobase;
+	void __iomem *ahb_base = cqspi->ahb_base;
+	unsigned int remaining = n_rx;
+	unsigned int bytes_to_read = 0;
+	int ret = 0;
+
+	writel(remaining, reg_base + CQSPI_REG_INDIRECTRDBYTES);
+
+	/* Clear all interrupts. */
+	writel(CQSPI_IRQ_STATUS_MASK, reg_base + CQSPI_REG_IRQSTATUS);
+
+	writel(CQSPI_IRQ_MASK_RD, reg_base + CQSPI_REG_IRQMASK);
+
+	reinit_completion(&cqspi->transfer_complete);
+	writel(CQSPI_REG_INDIRECTRD_START_MASK,
+	       reg_base + CQSPI_REG_INDIRECTRD);
+
+	while (remaining > 0) {
+		ret = wait_for_completion_timeout(&cqspi->transfer_complete,
+						  msecs_to_jiffies
+						  (CQSPI_READ_TIMEOUT_MS));
+
+		bytes_to_read = cqspi_get_rd_sram_level(cqspi);
+
+		if (!ret && bytes_to_read == 0) {
+			dev_err(nor->dev, "Indirect read timeout, no bytes\n");
+			ret = -ETIMEDOUT;
+			goto failrd;
+		}
+
+		while (bytes_to_read != 0) {
+			bytes_to_read *= cqspi->fifo_width;
+			bytes_to_read = bytes_to_read > remaining ?
+					remaining : bytes_to_read;
+			readsl(ahb_base, rxbuf, DIV_ROUND_UP(bytes_to_read, 4));
+			rxbuf += bytes_to_read;
+			remaining -= bytes_to_read;
+			bytes_to_read = cqspi_get_rd_sram_level(cqspi);
+		}
+	}
+
+	/* Check indirect done status */
+	ret = cqspi_wait_for_bit(reg_base + CQSPI_REG_INDIRECTRD,
+				 CQSPI_REG_INDIRECTRD_DONE_MASK, 0);
+	if (ret) {
+		dev_err(nor->dev,
+			"Indirect read completion error (%i)\n", ret);
+		goto failrd;
+	}
+
+	/* Disable interrupt */
+	writel(0, reg_base + CQSPI_REG_IRQMASK);
+
+	/* Clear indirect completion status */
+	writel(CQSPI_REG_INDIRECTRD_DONE_MASK, reg_base + CQSPI_REG_INDIRECTRD);
+
+	return 0;
+
+failrd:
+	/* Disable interrupt */
+	writel(0, reg_base + CQSPI_REG_IRQMASK);
+
+	/* Cancel the indirect read */
+	writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
+	       reg_base + CQSPI_REG_INDIRECTRD);
+	return ret;
+}
+
+static int cqspi_indirect_write_setup(struct spi_nor *nor,
+				      const unsigned int to_addr)
+{
+	unsigned int reg;
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	void __iomem *reg_base = cqspi->iobase;
+
+	/* Set opcode. */
+	reg = nor->program_opcode << CQSPI_REG_WR_INSTR_OPCODE_LSB;
+	writel(reg, reg_base + CQSPI_REG_WR_INSTR);
+	reg = cqspi_calc_rdreg(nor, nor->program_opcode);
+	writel(reg, reg_base + CQSPI_REG_RD_INSTR);
+
+	writel(to_addr, reg_base + CQSPI_REG_INDIRECTWRSTARTADDR);
+
+	reg = readl(reg_base + CQSPI_REG_SIZE);
+	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
+	reg |= (nor->addr_width - 1);
+	writel(reg, reg_base + CQSPI_REG_SIZE);
+	return 0;
+}
+
+static int cqspi_indirect_write_execute(struct spi_nor *nor,
+					const u8 *txbuf, const unsigned n_tx)
+{
+	const unsigned int page_size = nor->page_size;
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	void __iomem *reg_base = cqspi->iobase;
+	unsigned int remaining = n_tx;
+	unsigned int write_bytes;
+	int ret;
+
+	writel(remaining, reg_base + CQSPI_REG_INDIRECTWRBYTES);
+
+	/* Clear all interrupts. */
+	writel(CQSPI_IRQ_STATUS_MASK, reg_base + CQSPI_REG_IRQSTATUS);
+
+	writel(CQSPI_IRQ_MASK_WR, reg_base + CQSPI_REG_IRQMASK);
+
+	reinit_completion(&cqspi->transfer_complete);
+	writel(CQSPI_REG_INDIRECTWR_START_MASK,
+	       reg_base + CQSPI_REG_INDIRECTWR);
+
+	while (remaining > 0) {
+		write_bytes = remaining > page_size ? page_size : remaining;
+		writesl(cqspi->ahb_base, txbuf, DIV_ROUND_UP(write_bytes, 4));
+
+		ret = wait_for_completion_timeout(&cqspi->transfer_complete,
+						  msecs_to_jiffies
+						  (CQSPI_TIMEOUT_MS));
+		if (!ret) {
+			dev_err(nor->dev, "Indirect write timeout\n");
+			ret = -ETIMEDOUT;
+			goto failwr;
+		}
+
+		txbuf += write_bytes;
+		remaining -= write_bytes;
+	}
+
+	/* Check indirect done status */
+	ret = cqspi_wait_for_bit(reg_base + CQSPI_REG_INDIRECTWR,
+				 CQSPI_REG_INDIRECTWR_DONE_MASK, 0);
+	if (ret) {
+		dev_err(nor->dev,
+			"Indirect write completion error (%i)\n", ret);
+		goto failwr;
+	}
+
+	/* Disable interrupt. */
+	writel(0, reg_base + CQSPI_REG_IRQMASK);
+
+	/* Clear indirect completion status */
+	writel(CQSPI_REG_INDIRECTWR_DONE_MASK, reg_base + CQSPI_REG_INDIRECTWR);
+
+	cqspi_wait_idle(cqspi);
+
+	return 0;
+
+failwr:
+	/* Disable interrupt. */
+	writel(0, reg_base + CQSPI_REG_IRQMASK);
+
+	/* Cancel the indirect write */
+	writel(CQSPI_REG_INDIRECTWR_CANCEL_MASK,
+	       reg_base + CQSPI_REG_INDIRECTWR);
+	return ret;
+}
+
+static int cqspi_set_protocol(struct spi_nor *nor, enum spi_nor_protocol proto)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+
+	switch (proto) {
+	case SNOR_PROTO_1_1_1:
+	case SNOR_PROTO_1_1_2:
+	case SNOR_PROTO_1_1_4:
+	case SNOR_PROTO_1_2_2:
+	case SNOR_PROTO_1_4_4:
+		f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE;
+		break;
+	case SNOR_PROTO_2_2_2:
+		f_pdata->inst_width = CQSPI_INST_TYPE_DUAL;
+		break;
+	case SNOR_PROTO_4_4_4:
+		f_pdata->inst_width = CQSPI_INST_TYPE_QUAD;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	switch (proto) {
+	case SNOR_PROTO_1_1_1:
+	case SNOR_PROTO_1_1_2:
+	case SNOR_PROTO_1_1_4:
+		f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE;
+		break;
+	case SNOR_PROTO_1_2_2:
+	case SNOR_PROTO_2_2_2:
+		f_pdata->addr_width = CQSPI_INST_TYPE_DUAL;
+		break;
+	case SNOR_PROTO_1_4_4:
+	case SNOR_PROTO_4_4_4:
+		f_pdata->addr_width = CQSPI_INST_TYPE_QUAD;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void cqspi_write(struct spi_nor *nor, loff_t to,
+			size_t len, size_t *retlen, const u_char *buf)
+{
+	int ret;
+
+	ret = cqspi_set_protocol(nor, nor->write_proto);
+	if (ret)
+		return;
+
+	ret = cqspi_indirect_write_setup(nor, to);
+	if (ret)
+		return;
+
+	ret = cqspi_indirect_write_execute(nor, buf, len);
+	if (ret)
+		return;
+
+	*retlen += len;
+}
+
+static int cqspi_read(struct spi_nor *nor, loff_t from,
+		      size_t len, size_t *retlen, u_char *buf)
+{
+	int ret;
+
+	ret = cqspi_set_protocol(nor, nor->read_proto);
+	if (ret)
+		return ret;
+
+	ret = cqspi_indirect_read_setup(nor, from);
+	if (ret)
+		return ret;
+
+	ret = cqspi_indirect_read_execute(nor, buf, len);
+	if (ret)
+		return ret;
+
+	*retlen += len;
+	return ret;
+}
+
+static int cqspi_erase(struct spi_nor *nor, loff_t offs)
+{
+	int ret;
+
+	ret = cqspi_set_protocol(nor, nor->erase_proto);
+	if (ret)
+		return ret;
+
+	/* Send write enable, then erase commands. */
+	ret = nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0);
+	if (ret)
+		return ret;
+
+	/* Set up command buffer. */
+	ret = cqspi_command_write_addr(nor, nor->erase_opcode, offs);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static unsigned int calculate_ticks_for_ns(const unsigned int ref_clk_hz,
+					   const unsigned int ns_val)
+{
+	unsigned int ticks;
+
+	ticks = ref_clk_hz / 1000;	/* kHz */
+	ticks = DIV_ROUND_UP(ticks * ns_val, 1000000);
+
+	return ticks;
+}
+
+static void cqspi_delay(struct spi_nor *nor, const unsigned int sclk_hz)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	void __iomem *iobase = cqspi->iobase;
+	const unsigned int ref_clk_hz = cqspi->master_ref_clk_hz;
+	unsigned int tshsl, tchsh, tslch, tsd2d;
+	unsigned int reg;
+	unsigned int tsclk;
+
+	/* calculate the number of ref ticks for one sclk tick */
+	tsclk = (ref_clk_hz + sclk_hz - 1) / sclk_hz;
+
+	tshsl = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tshsl_ns);
+	/* this particular value must be at least one sclk */
+	if (tshsl < tsclk)
+		tshsl = tsclk;
+
+	tchsh = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tchsh_ns);
+	tslch = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tslch_ns);
+	tsd2d = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tsd2d_ns);
+
+	reg = (tshsl & CQSPI_REG_DELAY_TSHSL_MASK)
+	       << CQSPI_REG_DELAY_TSHSL_LSB;
+	reg |= (tchsh & CQSPI_REG_DELAY_TCHSH_MASK)
+		<< CQSPI_REG_DELAY_TCHSH_LSB;
+	reg |= (tslch & CQSPI_REG_DELAY_TSLCH_MASK)
+		<< CQSPI_REG_DELAY_TSLCH_LSB;
+	reg |= (tsd2d & CQSPI_REG_DELAY_TSD2D_MASK)
+		<< CQSPI_REG_DELAY_TSD2D_LSB;
+	writel(reg, iobase + CQSPI_REG_DELAY);
+}
+
+static void cqspi_config_baudrate_div(struct cqspi_st *cqspi,
+				      const unsigned int sclk_hz)
+{
+	const unsigned int ref_clk_hz = cqspi->master_ref_clk_hz;
+	void __iomem *reg_base = cqspi->iobase;
+	unsigned int reg;
+	unsigned int div;
+
+	reg = readl(reg_base + CQSPI_REG_CONFIG);
+	reg &= ~(CQSPI_REG_CONFIG_BAUD_MASK << CQSPI_REG_CONFIG_BAUD_LSB);
+
+	div = ref_clk_hz / sclk_hz;
+
+	/* Recalculate the baudrate divisor based on QSPI specification. */
+	if (div > 32)
+		div = 32;
+
+	/* Check if even number. */
+	if (div & 1)
+		div = (div / 2);
+	else
+		div = (div / 2) - 1;
+
+	div = (div & CQSPI_REG_CONFIG_BAUD_MASK) << CQSPI_REG_CONFIG_BAUD_LSB;
+	reg |= div;
+	writel(reg, reg_base + CQSPI_REG_CONFIG);
+}
+
+static void cqspi_readdata_capture(struct cqspi_st *cqspi,
+				   const unsigned int bypass,
+				   const unsigned int delay)
+{
+	void __iomem *reg_base = cqspi->iobase;
+	unsigned int reg;
+
+	reg = readl(reg_base + CQSPI_REG_READCAPTURE);
+
+	if (bypass)
+		reg |= (1 << CQSPI_REG_READCAPTURE_BYPASS_LSB);
+	else
+		reg &= ~(1 << CQSPI_REG_READCAPTURE_BYPASS_LSB);
+
+	reg &= ~(CQSPI_REG_READCAPTURE_DELAY_MASK
+		 << CQSPI_REG_READCAPTURE_DELAY_LSB);
+
+	reg |= (delay & CQSPI_REG_READCAPTURE_DELAY_MASK)
+		<< CQSPI_REG_READCAPTURE_DELAY_LSB;
+
+	writel(reg, reg_base + CQSPI_REG_READCAPTURE);
+}
+
+static void cqspi_chipselect(struct spi_nor *nor)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	void __iomem *reg_base = cqspi->iobase;
+	unsigned int chip_select = f_pdata->cs;
+	unsigned int reg;
+
+	reg = readl(reg_base + CQSPI_REG_CONFIG);
+	if (cqspi->is_decoded_cs) {
+		reg |= CQSPI_REG_CONFIG_DECODE_MASK;
+	} else {
+		reg &= ~CQSPI_REG_CONFIG_DECODE_MASK;
+
+		/* Convert CS if without decoder.
+		 * CS0 to 4b'1110
+		 * CS1 to 4b'1101
+		 * CS2 to 4b'1011
+		 * CS3 to 4b'0111
+		 */
+		chip_select = 0xF & ~(1 << chip_select);
+	}
+
+	reg &= ~(CQSPI_REG_CONFIG_CHIPSELECT_MASK
+		 << CQSPI_REG_CONFIG_CHIPSELECT_LSB);
+	reg |= (chip_select & CQSPI_REG_CONFIG_CHIPSELECT_MASK)
+	    << CQSPI_REG_CONFIG_CHIPSELECT_LSB;
+	writel(reg, reg_base + CQSPI_REG_CONFIG);
+}
+
+static void cqspi_controller_enable(struct cqspi_st *cqspi, bool enable)
+{
+	void __iomem *reg_base = cqspi->iobase;
+	unsigned int reg;
+
+	reg = readl(reg_base + CQSPI_REG_CONFIG);
+
+	if (enable)
+		reg |= CQSPI_REG_CONFIG_ENABLE_MASK;
+	else
+		reg &= ~CQSPI_REG_CONFIG_ENABLE_MASK;
+
+	writel(reg, reg_base + CQSPI_REG_CONFIG);
+}
+
+static void cqspi_switch_cs(struct spi_nor *nor)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	void __iomem *iobase = cqspi->iobase;
+	unsigned int reg;
+
+	cqspi_controller_enable(cqspi, 0);
+
+	/* configure page size and block size. */
+	reg = readl(iobase + CQSPI_REG_SIZE);
+	reg &= ~(CQSPI_REG_SIZE_PAGE_MASK << CQSPI_REG_SIZE_PAGE_LSB);
+	reg &= ~(CQSPI_REG_SIZE_BLOCK_MASK << CQSPI_REG_SIZE_BLOCK_LSB);
+	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
+	reg |= (nor->page_size << CQSPI_REG_SIZE_PAGE_LSB);
+	reg |= (ilog2(nor->mtd.erasesize) << CQSPI_REG_SIZE_BLOCK_LSB);
+	reg |= (nor->addr_width - 1);
+	writel(reg, iobase + CQSPI_REG_SIZE);
+
+	/* configure the chip select */
+	cqspi_chipselect(nor);
+
+	cqspi_controller_enable(cqspi, 1);
+}
+
+static int cqspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	const unsigned int sclk = f_pdata->clk_rate;
+
+	/* Switch chip select. */
+	if (cqspi->current_cs != f_pdata->cs) {
+		cqspi->current_cs = f_pdata->cs;
+		cqspi_switch_cs(nor);
+	}
+
+	/* Setup baudrate divisor and delays */
+	if (cqspi->sclk != sclk) {
+		cqspi->sclk = sclk;
+		cqspi_controller_enable(cqspi, 0);
+		cqspi_config_baudrate_div(cqspi, sclk);
+		cqspi_delay(nor, sclk);
+		cqspi_readdata_capture(cqspi, 1, f_pdata->read_delay);
+		cqspi_controller_enable(cqspi, 1);
+	}
+
+	return 0;
+}
+
+static int cqspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
+{
+	int ret;
+
+	ret = cqspi_set_protocol(nor, nor->reg_proto);
+	if (ret)
+		return ret;
+
+	cqspi_prep(nor, SPI_NOR_OPS_READ);
+
+	ret = cqspi_command_read(nor, &opcode, 1, buf, len);
+	return ret;
+}
+
+static int cqspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
+{
+	int ret = 0;
+
+	ret = cqspi_set_protocol(nor, nor->reg_proto);
+	if (ret)
+		return ret;
+
+	cqspi_prep(nor, SPI_NOR_OPS_WRITE);
+
+	ret = cqspi_command_write(nor, opcode, buf, len);
+	return ret;
+}
+
+static int cqspi_of_get_flash_pdata(struct platform_device *pdev,
+				    struct cqspi_flash_pdata *f_pdata,
+				    struct device_node *np)
+{
+	if (of_property_read_u32(np, "cdns,read-delay", &f_pdata->read_delay)) {
+		dev_err(&pdev->dev, "couldn't determine read-delay\n");
+		return -ENXIO;
+	}
+
+	if (of_property_read_u32(np, "cdns,tshsl-ns", &f_pdata->tshsl_ns)) {
+		dev_err(&pdev->dev, "couldn't determine tshsl-ns\n");
+		return -ENXIO;
+	}
+
+	if (of_property_read_u32(np, "cdns,tsd2d-ns", &f_pdata->tsd2d_ns)) {
+		dev_err(&pdev->dev, "couldn't determine tsd2d-ns\n");
+		return -ENXIO;
+	}
+
+	if (of_property_read_u32(np, "cdns,tchsh-ns", &f_pdata->tchsh_ns)) {
+		dev_err(&pdev->dev, "couldn't determine tchsh-ns\n");
+		return -ENXIO;
+	}
+
+	if (of_property_read_u32(np, "cdns,tslch-ns", &f_pdata->tslch_ns)) {
+		dev_err(&pdev->dev, "couldn't determine tslch-ns\n");
+		return -ENXIO;
+	}
+
+	if (of_property_read_u32(np, "spi-max-frequency", &f_pdata->clk_rate)) {
+		dev_err(&pdev->dev, "couldn't determine spi-max-frequency\n");
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
+static int cqspi_of_get_pdata(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct cqspi_st *cqspi = platform_get_drvdata(pdev);
+
+	cqspi->is_decoded_cs = of_property_read_bool(np, "cdns,is-decoded-cs");
+
+	if (of_property_read_u32(np, "cdns,fifo-depth", &cqspi->fifo_depth)) {
+		dev_err(&pdev->dev, "couldn't determine fifo-depth\n");
+		return -ENXIO;
+	}
+
+	if (of_property_read_u32(np, "cdns,fifo-width", &cqspi->fifo_width)) {
+		dev_err(&pdev->dev, "couldn't determine fifo-width\n");
+		return -ENXIO;
+	}
+
+	if (of_property_read_u32(np, "cdns,trigger-address",
+				 &cqspi->trigger_address)) {
+		dev_err(&pdev->dev, "couldn't determine trigger-address\n");
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
+static void cqspi_controller_init(struct cqspi_st *cqspi)
+{
+	cqspi_controller_enable(cqspi, 0);
+
+	/* Configure the remap address register, no remap */
+	writel(0, cqspi->iobase + CQSPI_REG_REMAP);
+
+	/* Disable all interrupts. */
+	writel(0, cqspi->iobase + CQSPI_REG_IRQMASK);
+
+	/* Configure the SRAM split to 1:1 . */
+	writel(cqspi->fifo_depth / 2, cqspi->iobase + CQSPI_REG_SRAMPARTITION);
+
+	/* Load indirect trigger address. */
+	writel(cqspi->trigger_address,
+	       cqspi->iobase + CQSPI_REG_INDIRECTTRIGGER);
+
+	/* Program read watermark -- 1/2 of the FIFO. */
+	writel(cqspi->fifo_depth * cqspi->fifo_width / 2,
+	       cqspi->iobase + CQSPI_REG_INDIRECTRDWATERMARK);
+	/* Program write watermark -- 1/8 of the FIFO. */
+	writel(cqspi->fifo_depth * cqspi->fifo_width / 8,
+	       cqspi->iobase + CQSPI_REG_INDIRECTWRWATERMARK);
+
+	cqspi_controller_enable(cqspi, 1);
+}
+
+static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
+{
+	struct platform_device *pdev = cqspi->pdev;
+	struct device *dev = &pdev->dev;
+	struct cqspi_flash_pdata *f_pdata;
+	struct spi_nor *nor;
+	struct mtd_info *mtd;
+	unsigned int cs;
+	int i, ret;
+
+	/* Get flash device data */
+	for_each_available_child_of_node(dev->of_node, np) {
+		if (of_property_read_u32(np, "reg", &cs)) {
+			dev_err(dev, "Couldn't determine chip select.\n");
+			goto err;
+		}
+
+		if (cs > CQSPI_MAX_CHIPSELECT) {
+			dev_err(dev, "Chip select %d out of range.\n", cs);
+			goto err;
+		}
+
+		f_pdata = &cqspi->f_pdata[cs];
+		f_pdata->cqspi = cqspi;
+		f_pdata->cs = cs;
+
+		ret = cqspi_of_get_flash_pdata(pdev, f_pdata, np);
+		if (ret)
+			goto err;
+
+		nor = &f_pdata->nor;
+		mtd = &nor->mtd;
+
+		mtd->priv = nor;
+
+		nor->dev = dev;
+		spi_nor_set_flash_node(nor, np);
+		nor->priv = f_pdata;
+
+		nor->read_reg = cqspi_read_reg;
+		nor->write_reg = cqspi_write_reg;
+		nor->read = cqspi_read;
+		nor->write = cqspi_write;
+		nor->erase = cqspi_erase;
+		nor->prepare = cqspi_prep;
+
+		mtd->name = kasprintf(GFP_KERNEL, "%s.%d", dev_name(dev), cs);
+		if (!mtd->name) {
+			ret = -ENOMEM;
+			goto err;
+		}
+
+		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
+		if (ret)
+			goto err;
+
+		ret = mtd_device_register(mtd, NULL, 0);
+		if (ret)
+			goto err;
+	}
+
+	return 0;
+
+err:
+	for (i = 0; i < CQSPI_MAX_CHIPSELECT; i++)
+		if (cqspi->f_pdata[i].nor.mtd.name) {
+			mtd_device_unregister(&cqspi->f_pdata[i].nor.mtd);
+			kfree(cqspi->f_pdata[i].nor.mtd.name);
+		}
+	return ret;
+}
+
+static int cqspi_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct cqspi_st *cqspi;
+	struct resource *res;
+	struct resource *res_ahb;
+	int ret;
+	int irq;
+
+	cqspi = devm_kzalloc(dev, sizeof(*cqspi), GFP_KERNEL);
+	if (!cqspi)
+		return -ENOMEM;
+
+	cqspi->pdev = pdev;
+	platform_set_drvdata(pdev, cqspi);
+
+	/* Obtain configuration from OF. */
+	ret = cqspi_of_get_pdata(pdev);
+	if (ret) {
+		dev_err(dev, "Cannot get mandatory OF data.\n");
+		return -ENODEV;
+	}
+
+	/* Obtain QSPI clock. */
+	cqspi->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(cqspi->clk)) {
+		dev_err(dev, "Cannot claim QSPI clock.\n");
+		return PTR_ERR(cqspi->clk);
+	}
+
+	cqspi->master_ref_clk_hz = clk_get_rate(cqspi->clk);
+
+	/* Obtain and remap controller address. */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	cqspi->iobase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(cqspi->iobase)) {
+		dev_err(dev, "Cannot remap controller address.\n");
+		return PTR_ERR(cqspi->iobase);
+	}
+
+	/* Obtain and remap AHB address. */
+	res_ahb = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	cqspi->ahb_base = devm_ioremap_resource(dev, res_ahb);
+	if (IS_ERR(cqspi->ahb_base)) {
+		dev_err(dev, "Cannot remap AHB address.\n");
+		return PTR_ERR(cqspi->ahb_base);
+	}
+
+	init_completion(&cqspi->transfer_complete);
+
+	/* Obtain IRQ line. */
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "Cannot obtain IRQ.\n");
+		return -ENXIO;
+	}
+
+	ret = devm_request_irq(dev, irq, cqspi_irq_handler, 0,
+			       pdev->name, cqspi);
+	if (ret) {
+		dev_err(dev, "Cannot request IRQ.\n");
+		return ret;
+	}
+
+	cqspi_wait_idle(cqspi);
+	cqspi_controller_init(cqspi);
+	cqspi->current_cs = -1;
+	cqspi->sclk = 0;
+
+	ret = cqspi_setup_flash(cqspi, np);
+	if (ret) {
+		dev_err(dev, "Cadence QSPI NOR probe failed %d\n", ret);
+		cqspi_controller_enable(cqspi, 0);
+	}
+
+	return ret;
+}
+
+static int cqspi_remove(struct platform_device *pdev)
+{
+	struct cqspi_st *cqspi = platform_get_drvdata(pdev);
+	int i;
+
+	cqspi_controller_enable(cqspi, 0);
+
+	for (i = 0; i < CQSPI_MAX_CHIPSELECT; i++)
+		if (cqspi->f_pdata[i].nor.mtd.name) {
+			mtd_device_unregister(&cqspi->f_pdata[i].nor.mtd);
+			kfree(cqspi->f_pdata[i].nor.mtd.name);
+		}
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int cqspi_suspend(struct device *dev)
+{
+	struct cqspi_st *cqspi = dev_get_drvdata(dev);
+
+	cqspi_controller_enable(cqspi, 0);
+	return 0;
+}
+
+static int cqspi_resume(struct device *dev)
+{
+	struct cqspi_st *cqspi = dev_get_drvdata(dev);
+
+	cqspi_controller_enable(cqspi, 1);
+	return 0;
+}
+
+static const struct dev_pm_ops cqspi__dev_pm_ops = {
+	.suspend = cqspi_suspend,
+	.resume = cqspi_resume,
+};
+
+#define CQSPI_DEV_PM_OPS	(&cqspi__dev_pm_ops)
+#else
+#define CQSPI_DEV_PM_OPS	NULL
+#endif
+
+static struct of_device_id const cqspi_dt_ids[] = {
+	{.compatible = "cdns,qspi-nor",},
+	{ /* end of table */ }
+};
+
+MODULE_DEVICE_TABLE(of, cqspi_dt_ids);
+
+static struct platform_driver cqspi_platform_driver = {
+	.probe = cqspi_probe,
+	.remove = cqspi_remove,
+	.driver = {
+		.name = CQSPI_NAME,
+		.pm = CQSPI_DEV_PM_OPS,
+		.of_match_table = cqspi_dt_ids,
+	},
+};
+
+module_platform_driver(cqspi_platform_driver);
+
+MODULE_DESCRIPTION("Cadence QSPI Controller Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" CQSPI_NAME);
+MODULE_AUTHOR("Ley Foon Tan <lftan@altera.com>");
+MODULE_AUTHOR("Graham Moore <grmoore@opensource.altera.com>");
-- 
2.1.4

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Dinh Nguyen]

On 01/10/2016 10:34 PM, Marek Vasut wrote:
> From: Graham Moore <grmoore@opensource.altera.com>
> 
> Add support for the Cadence QSPI controller. This controller is
> present in the Altera SoCFPGA SoCs and this driver has been tested
> on the Cyclone V SoC.
> 
> Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alan Tull <atull@opensource.altera.com>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Graham Moore <grmoore@opensource.altera.com>
> Cc: "R, Vignesh" <vigneshr@ti.com>
> Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
> Cc: devicetree@vger.kernel.org
> ---
>  drivers/mtd/spi-nor/Kconfig           |   11 +
>  drivers/mtd/spi-nor/Makefile          |    1 +
>  drivers/mtd/spi-nor/cadence-quadspi.c | 1280 +++++++++++++++++++++++++++++++++
>  3 files changed, 1292 insertions(+)
>  create mode 100644 drivers/mtd/spi-nor/cadence-quadspi.c
> 
> V2: use NULL instead of modalias in spi_nor_scan call
> V3: Use existing property is-decoded-cs instead of creating duplicate.
> V4: Support Micron quad mode by snooping command stream for EVCR command
>     and subsequently configuring Cadence controller for quad mode.
> V5: Clean up sparse and smatch complaints.  Remove snooping of Micron
>     quad mode.  Add comment on XIP mode bit and dummy clock cycles.  Set
>     up SRAM partition at 1:1 during init.
> V6: Remove dts patch that was included by mistake.  Incorporate Vikas's
>     comments regarding fifo width, SRAM partition setting, and trigger
>     address.  Trigger address was added as an unsigned int, as it is not
>     an IO resource per se, and does not need to be mapped. Also add
>     Marek Vasut's workaround for picking up OF properties on subnodes.
> V7: - Perform coding-style cleanup and type fixes. Remove ugly QSPI_*()
>       macros and replace them with functions. Get rid of unused variables.
>     - Implement support for nor->set_protocol() to handle Quad-command,
>       this patch now depends on the following patch:
>       mtd: spi-nor: notify (Q)SPI controller about protocol change
>     - Replace that cqspi_fifo_read() disaster with plain old readsl()
>       and cqspi_fifo_write() tentacle horror with pretty writesl().
>     - Remove CQSPI_SUPPORT_XIP_CHIPS, which is broken.
>     - Get rid of cqspi_find_chipselect() mess, instead just place the
>       struct cqspi_st and chipselect number into struct cqspi_flash_pdata
>       and set nor->priv to the struct cqspi_flash_pdata of that particular
>       chip.
>     - Replace the odd math in calculate_ticks_for_ns() with DIV_ROUND_UP().
>     - Make variables const where applicable.
> V8: - Implement a function to wait for bit being set/unset for a given
>       period of time and use it to replace the ad-hoc bits of code.
>     - Configure the write underflow watermark to be 1/8 if FIFO size.
>     - Extract out the SPI NOR flash probing code into separate function
>       to clearly mark what will soon be considered a boilerplate code.
>     - Repair the handling of mode bits, which caused instability in V7.
>     - Clean up the interrupt handling
>     - Fix Kconfig help text and make the patch depend on OF and COMPILE_TEST.
> V9: - Rename CQSPI_REG_IRQ_IND_RD_OVERFLOW to CQSPI_REG_IRQ_IND_SRAM_FULL
>     - Merge cqspi_controller_disable() into cqspi_controller_enable() and
>       make the mode selectable via parameter.
> V10: - Update against Cyrille's new patchset and changes to linux-mtd.
>      - Repair problem with multiple QSPI NOR devices having the same mtd->name,
>        they are now named devname.cs , where cs is the chipselect ID.
> 
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 43bafde..7e29050 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -50,4 +50,15 @@ config SPI_NXP_SPIFI
>  	  Flash. Enable this option if you have a device with a SPIFI
>  	  controller and want to access the Flash as a mtd device.
>  
> +config SPI_CADENCE_QUADSPI
> +	tristate "Cadence Quad SPI controller"
> +	depends on OF && (ARCH_SOCFPGA || COMPILE_TEST)

I think you can remove the ARCH_SOCFPGA dependency as this driver is
used on a TI EVM as well.

Dinh

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Marek Vasut]

On Monday, January 11, 2016 at 05:09:24 PM, Dinh Nguyen wrote:
> On 01/10/2016 10:34 PM, Marek Vasut wrote:
> > From: Graham Moore <grmoore@opensource.altera.com>
> > 
> > Add support for the Cadence QSPI controller. This controller is
> > present in the Altera SoCFPGA SoCs and this driver has been tested
> > on the Cyclone V SoC.
> > 
> > Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Alan Tull <atull@opensource.altera.com>
> > Cc: Brian Norris <computersforpeace@gmail.com>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Graham Moore <grmoore@opensource.altera.com>
> > Cc: "R, Vignesh" <vigneshr@ti.com>
> > Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
> > Cc: devicetree@vger.kernel.org
> > ---
> > 
> >  drivers/mtd/spi-nor/Kconfig           |   11 +
> >  drivers/mtd/spi-nor/Makefile          |    1 +
> >  drivers/mtd/spi-nor/cadence-quadspi.c | 1280
> >  +++++++++++++++++++++++++++++++++ 3 files changed, 1292 insertions(+)
> >  create mode 100644 drivers/mtd/spi-nor/cadence-quadspi.c
> > 
> > V2: use NULL instead of modalias in spi_nor_scan call
> > V3: Use existing property is-decoded-cs instead of creating duplicate.
> > V4: Support Micron quad mode by snooping command stream for EVCR command
> > 
> >     and subsequently configuring Cadence controller for quad mode.
> > 
> > V5: Clean up sparse and smatch complaints.  Remove snooping of Micron
> > 
> >     quad mode.  Add comment on XIP mode bit and dummy clock cycles.  Set
> >     up SRAM partition at 1:1 during init.
> > 
> > V6: Remove dts patch that was included by mistake.  Incorporate Vikas's
> > 
> >     comments regarding fifo width, SRAM partition setting, and trigger
> >     address.  Trigger address was added as an unsigned int, as it is not
> >     an IO resource per se, and does not need to be mapped. Also add
> >     Marek Vasut's workaround for picking up OF properties on subnodes.
> > 
> > V7: - Perform coding-style cleanup and type fixes. Remove ugly QSPI_*()
> > 
> >       macros and replace them with functions. Get rid of unused
> >       variables.
> >     
> >     - Implement support for nor->set_protocol() to handle Quad-command,
> >     
> >       this patch now depends on the following patch:
> >       mtd: spi-nor: notify (Q)SPI controller about protocol change
> >     
> >     - Replace that cqspi_fifo_read() disaster with plain old readsl()
> >     
> >       and cqspi_fifo_write() tentacle horror with pretty writesl().
> >     
> >     - Remove CQSPI_SUPPORT_XIP_CHIPS, which is broken.
> >     - Get rid of cqspi_find_chipselect() mess, instead just place the
> >     
> >       struct cqspi_st and chipselect number into struct cqspi_flash_pdata
> >       and set nor->priv to the struct cqspi_flash_pdata of that
> >       particular chip.
> >     
> >     - Replace the odd math in calculate_ticks_for_ns() with
> >     DIV_ROUND_UP(). - Make variables const where applicable.
> > 
> > V8: - Implement a function to wait for bit being set/unset for a given
> > 
> >       period of time and use it to replace the ad-hoc bits of code.
> >     
> >     - Configure the write underflow watermark to be 1/8 if FIFO size.
> >     - Extract out the SPI NOR flash probing code into separate function
> >     
> >       to clearly mark what will soon be considered a boilerplate code.
> >     
> >     - Repair the handling of mode bits, which caused instability in V7.
> >     - Clean up the interrupt handling
> >     - Fix Kconfig help text and make the patch depend on OF and
> >     COMPILE_TEST.
> > 
> > V9: - Rename CQSPI_REG_IRQ_IND_RD_OVERFLOW to CQSPI_REG_IRQ_IND_SRAM_FULL
> > 
> >     - Merge cqspi_controller_disable() into cqspi_controller_enable() and
> >     
> >       make the mode selectable via parameter.
> > 
> > V10: - Update against Cyrille's new patchset and changes to linux-mtd.
> > 
> >      - Repair problem with multiple QSPI NOR devices having the same
> >      mtd->name,
> >      
> >        they are now named devname.cs , where cs is the chipselect ID.
> > 
> > diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> > index 43bafde..7e29050 100644
> > --- a/drivers/mtd/spi-nor/Kconfig
> > +++ b/drivers/mtd/spi-nor/Kconfig
> > @@ -50,4 +50,15 @@ config SPI_NXP_SPIFI
> > 
> >  	  Flash. Enable this option if you have a device with a SPIFI
> >  	  controller and want to access the Flash as a mtd device.
> > 
> > +config SPI_CADENCE_QUADSPI
> > +	tristate "Cadence Quad SPI controller"
> > +	depends on OF && (ARCH_SOCFPGA || COMPILE_TEST)
> 
> I think you can remove the ARCH_SOCFPGA dependency as this driver is
> used on a TI EVM as well.

Yeah, that's a good point, thanks!

Best regards,
Marek Vasut

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Vignesh R]

On 01/11/2016 09:39 PM, Dinh Nguyen wrote:
> On 01/10/2016 10:34 PM, Marek Vasut wrote:
>> From: Graham Moore <grmoore@opensource.altera.com>
>>
>> Add support for the Cadence QSPI controller. This controller is
>> present in the Altera SoCFPGA SoCs and this driver has been tested
>> on the Cyclone V SoC.
>>

[...]

>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 43bafde..7e29050 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -50,4 +50,15 @@ config SPI_NXP_SPIFI
>>  	  Flash. Enable this option if you have a device with a SPIFI
>>  	  controller and want to access the Flash as a mtd device.
>>  
>> +config SPI_CADENCE_QUADSPI
>> +	tristate "Cadence Quad SPI controller"
>> +	depends on OF && (ARCH_SOCFPGA || COMPILE_TEST)
> 
> I think you can remove the ARCH_SOCFPGA dependency as this driver is
> used on a TI EVM as well.

I think above usage is correct. This will enable SPI_CADENCE_QUADSPI to
be selected when ARCH_SOCFPGA=y without having to enable COMPILE_TEST.
AFAIK, COMPILE_TEST is used usually to build test the driver w/o having
to enable ARCH specific configs (usually by maintainers).
For TI EVM, I would patch this file to add my architecture later(i.e
along with DT patch to my board).

-- 
Regards
Vignesh

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Marek Vasut]

On Tuesday, January 12, 2016 at 05:41:02 AM, Vignesh R wrote:
> On 01/11/2016 09:39 PM, Dinh Nguyen wrote:
> > On 01/10/2016 10:34 PM, Marek Vasut wrote:
> >> From: Graham Moore <grmoore@opensource.altera.com>
> >> 
> >> Add support for the Cadence QSPI controller. This controller is
> >> present in the Altera SoCFPGA SoCs and this driver has been tested
> >> on the Cyclone V SoC.
> 
> [...]
> 
> >> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> >> index 43bafde..7e29050 100644
> >> --- a/drivers/mtd/spi-nor/Kconfig
> >> +++ b/drivers/mtd/spi-nor/Kconfig
> >> @@ -50,4 +50,15 @@ config SPI_NXP_SPIFI
> >> 
> >>  	  Flash. Enable this option if you have a device with a SPIFI
> >>  	  controller and want to access the Flash as a mtd device.
> >> 
> >> +config SPI_CADENCE_QUADSPI
> >> +	tristate "Cadence Quad SPI controller"
> >> +	depends on OF && (ARCH_SOCFPGA || COMPILE_TEST)
> > 
> > I think you can remove the ARCH_SOCFPGA dependency as this driver is
> > used on a TI EVM as well.
> 
> I think above usage is correct. This will enable SPI_CADENCE_QUADSPI to
> be selected when ARCH_SOCFPGA=y without having to enable COMPILE_TEST.
> AFAIK, COMPILE_TEST is used usually to build test the driver w/o having
> to enable ARCH specific configs (usually by maintainers).
> For TI EVM, I would patch this file to add my architecture later(i.e
> along with DT patch to my board).

I changed this to OF && (ARM || COMPILE_TEST) , so that should be enough.

Best regards,
Marek Vasut

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[R, Vignesh]

Hi Marek,

I encountered a issue with this driver while testing.

On 1/11/2016 10:04 AM, Marek Vasut wrote:
> From: Graham Moore <grmoore@opensource.altera.com>
> 
> Add support for the Cadence QSPI controller. This controller is
> present in the Altera SoCFPGA SoCs and this driver has been tested
> on the Cyclone V SoC.


> +static void cqspi_switch_cs(struct spi_nor *nor)
> +{
> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
> +	struct cqspi_st *cqspi = f_pdata->cqspi;
> +	void __iomem *iobase = cqspi->iobase;
> +	unsigned int reg;
> +
> +	cqspi_controller_enable(cqspi, 0);
> +
> +	/* configure page size and block size. */
> +	reg = readl(iobase + CQSPI_REG_SIZE);
> +	reg &= ~(CQSPI_REG_SIZE_PAGE_MASK << CQSPI_REG_SIZE_PAGE_LSB);
> +	reg &= ~(CQSPI_REG_SIZE_BLOCK_MASK << CQSPI_REG_SIZE_BLOCK_LSB);
> +	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
> +	reg |= (nor->page_size << CQSPI_REG_SIZE_PAGE_LSB);
> +	reg |= (ilog2(nor->mtd.erasesize) << CQSPI_REG_SIZE_BLOCK_LSB);
> +	reg |= (nor->addr_width - 1);
> +	writel(reg, iobase + CQSPI_REG_SIZE);
> +

Page size and block size are configured here...

> +	/* configure the chip select */
> +	cqspi_chipselect(nor);
> +
> +	cqspi_controller_enable(cqspi, 1);
> +}
> +
> +static int cqspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
> +{
> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
> +	struct cqspi_st *cqspi = f_pdata->cqspi;
> +	const unsigned int sclk = f_pdata->clk_rate;
> +
> +	/* Switch chip select. */
> +	if (cqspi->current_cs != f_pdata->cs) {
> +		cqspi->current_cs = f_pdata->cs;
> +		cqspi_switch_cs(nor);

cqspi_switch_cs(nor) is called from cqspi_prep(). And is called only
once for a given slave (assuming only one slave on the QSPI bus)

> +	}
> +
> +	/* Setup baudrate divisor and delays */
> +	if (cqspi->sclk != sclk) {
> +		cqspi->sclk = sclk;
> +		cqspi_controller_enable(cqspi, 0);
> +		cqspi_config_baudrate_div(cqspi, sclk);
> +		cqspi_delay(nor, sclk);
> +		cqspi_readdata_capture(cqspi, 1, f_pdata->read_delay);
> +		cqspi_controller_enable(cqspi, 1);
> +	}
> +
> +	return 0;
> +}
> +
> +static int cqspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
> +{
> +	int ret;
> +
> +	ret = cqspi_set_protocol(nor, nor->reg_proto);
> +	if (ret)
> +		return ret;
> +
> +	cqspi_prep(nor, SPI_NOR_OPS_READ);

cqspi_read_reg() is first called to read JEDEC ID, this calls
cqspi_prep() for first time. But nor->page_size, nor->mtd.erasesize are
not yet populated. Therefore cqspi_switch_cs() will not populate
CQSPI_REG_SIZE with correct values.
I think its better to configure this register each time during
read/write ops.

Regards
Vignesh

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Marek Vasut]

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

On 04/06/2016 06:55 PM, R, Vignesh wrote:
> Hi Marek,

Hi!

> I encountered a issue with this driver while testing.

Try with the attached patches, I am planning to use them for V11
submission. I think you're hitting the problem with missing buslock.

> On 1/11/2016 10:04 AM, Marek Vasut wrote:
>> From: Graham Moore <grmoore@opensource.altera.com>
>>
>> Add support for the Cadence QSPI controller. This controller is
>> present in the Altera SoCFPGA SoCs and this driver has been tested
>> on the Cyclone V SoC.
> 
> 
>> +static void cqspi_switch_cs(struct spi_nor *nor)
>> +{
>> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
>> +	struct cqspi_st *cqspi = f_pdata->cqspi;
>> +	void __iomem *iobase = cqspi->iobase;
>> +	unsigned int reg;
>> +
>> +	cqspi_controller_enable(cqspi, 0);
>> +
>> +	/* configure page size and block size. */
>> +	reg = readl(iobase + CQSPI_REG_SIZE);
>> +	reg &= ~(CQSPI_REG_SIZE_PAGE_MASK << CQSPI_REG_SIZE_PAGE_LSB);
>> +	reg &= ~(CQSPI_REG_SIZE_BLOCK_MASK << CQSPI_REG_SIZE_BLOCK_LSB);
>> +	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
>> +	reg |= (nor->page_size << CQSPI_REG_SIZE_PAGE_LSB);
>> +	reg |= (ilog2(nor->mtd.erasesize) << CQSPI_REG_SIZE_BLOCK_LSB);
>> +	reg |= (nor->addr_width - 1);
>> +	writel(reg, iobase + CQSPI_REG_SIZE);
>> +
> 
> Page size and block size are configured here...
> 
>> +	/* configure the chip select */
>> +	cqspi_chipselect(nor);
>> +
>> +	cqspi_controller_enable(cqspi, 1);
>> +}
>> +
>> +static int cqspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>> +{
>> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
>> +	struct cqspi_st *cqspi = f_pdata->cqspi;
>> +	const unsigned int sclk = f_pdata->clk_rate;
>> +
>> +	/* Switch chip select. */
>> +	if (cqspi->current_cs != f_pdata->cs) {
>> +		cqspi->current_cs = f_pdata->cs;
>> +		cqspi_switch_cs(nor);
> 
> cqspi_switch_cs(nor) is called from cqspi_prep(). And is called only
> once for a given slave (assuming only one slave on the QSPI bus)
> 
>> +	}
>> +
>> +	/* Setup baudrate divisor and delays */
>> +	if (cqspi->sclk != sclk) {
>> +		cqspi->sclk = sclk;
>> +		cqspi_controller_enable(cqspi, 0);
>> +		cqspi_config_baudrate_div(cqspi, sclk);
>> +		cqspi_delay(nor, sclk);
>> +		cqspi_readdata_capture(cqspi, 1, f_pdata->read_delay);
>> +		cqspi_controller_enable(cqspi, 1);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int cqspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
>> +{
>> +	int ret;
>> +
>> +	ret = cqspi_set_protocol(nor, nor->reg_proto);
>> +	if (ret)
>> +		return ret;
>> +
>> +	cqspi_prep(nor, SPI_NOR_OPS_READ);
> 
> cqspi_read_reg() is first called to read JEDEC ID, this calls
> cqspi_prep() for first time. But nor->page_size, nor->mtd.erasesize are
> not yet populated. Therefore cqspi_switch_cs() will not populate
> CQSPI_REG_SIZE with correct values.
> I think its better to configure this register each time during
> read/write ops.
> 
> Regards
> Vignesh
> 


-- 
Best regards,
Marek Vasut

[-- Attachment #2: 0001-mtd-spi-nor-cqspi-Reinit-completion-during-indirect-.patch --]
[-- Type: text/x-patch, Size: 1270 bytes --]

>From 6a649c8263149b06d29a3acc91b63fb0c1728deb Mon Sep 17 00:00:00 2001
From: Marek Vasut <marex@denx.de>
Date: Wed, 23 Mar 2016 08:26:46 +0100
Subject: [PATCH 1/3] mtd: spi-nor: cqspi: Reinit completion during indirect
 read and write

Reinit the completion structures when performing indirect I/O. This
does not manifest as a bug, but is necessary to make the code fully
correct.

Signed-off-by: Marek Vasut <marex@denx.de>
---
 drivers/mtd/spi-nor/cadence-quadspi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index d9a7a67..a4d246c 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -527,6 +527,9 @@ static int cqspi_indirect_read_execute(struct spi_nor *nor,
 			remaining -= bytes_to_read;
 			bytes_to_read = cqspi_get_rd_sram_level(cqspi);
 		}
+
+		if (remaining > 0)
+			reinit_completion(&cqspi->transfer_complete);
 	}
 
 	/* Check indirect done status */
@@ -616,6 +619,9 @@ static int cqspi_indirect_write_execute(struct spi_nor *nor,
 
 		txbuf += write_bytes;
 		remaining -= write_bytes;
+
+		if (remaining > 0)
+			reinit_completion(&cqspi->transfer_complete);
 	}
 
 	/* Check indirect done status */
-- 
2.7.0


[-- Attachment #3: 0002-mtd-spi-nor-cqspi-Optimize-the-control-reconfigurati.patch --]
[-- Type: text/x-patch, Size: 2261 bytes --]

>From 30391427f34193d9229e70bf62794cbcb733b047 Mon Sep 17 00:00:00 2001
From: Marek Vasut <marex@denx.de>
Date: Wed, 23 Mar 2016 08:27:50 +0100
Subject: [PATCH 2/3] mtd: spi-nor: cqspi: Optimize the control reconfiguration

Always disable and re-enable the controller only once when switching
the chipselect and bus speed instead of doing so twice.

Signed-off-by: Marek Vasut <marex@denx.de>
---
 drivers/mtd/spi-nor/cadence-quadspi.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index a4d246c..ee309cb 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -905,8 +905,6 @@ static void cqspi_switch_cs(struct spi_nor *nor)
 	void __iomem *iobase = cqspi->iobase;
 	unsigned int reg;
 
-	cqspi_controller_enable(cqspi, 0);
-
 	/* configure page size and block size. */
 	reg = readl(iobase + CQSPI_REG_SIZE);
 	reg &= ~(CQSPI_REG_SIZE_PAGE_MASK << CQSPI_REG_SIZE_PAGE_LSB);
@@ -919,8 +917,6 @@ static void cqspi_switch_cs(struct spi_nor *nor)
 
 	/* configure the chip select */
 	cqspi_chipselect(nor);
-
-	cqspi_controller_enable(cqspi, 1);
 }
 
 static int cqspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
@@ -928,23 +924,29 @@ static int cqspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
 	struct cqspi_st *cqspi = f_pdata->cqspi;
 	const unsigned int sclk = f_pdata->clk_rate;
+	const int switch_cs = (cqspi->current_cs != f_pdata->cs);
+	const int switch_ck = (cqspi->sclk != sclk);
+
+	if (switch_cs || switch_ck)
+		cqspi_controller_enable(cqspi, 0);
 
 	/* Switch chip select. */
-	if (cqspi->current_cs != f_pdata->cs) {
+	if (switch_cs) {
 		cqspi->current_cs = f_pdata->cs;
 		cqspi_switch_cs(nor);
 	}
 
 	/* Setup baudrate divisor and delays */
-	if (cqspi->sclk != sclk) {
+	if (switch_ck) {
 		cqspi->sclk = sclk;
-		cqspi_controller_enable(cqspi, 0);
 		cqspi_config_baudrate_div(cqspi, sclk);
 		cqspi_delay(nor, sclk);
 		cqspi_readdata_capture(cqspi, 1, f_pdata->read_delay);
-		cqspi_controller_enable(cqspi, 1);
 	}
 
+	if (switch_cs || switch_ck)
+		cqspi_controller_enable(cqspi, 1);
+
 	return 0;
 }
 
-- 
2.7.0


[-- Attachment #4: 0003-mtd-spi-nor-cqspi-Add-bus-lock.patch --]
[-- Type: text/x-patch, Size: 3372 bytes --]

>From e680fa497cf0e2f41fe1b56c7d6b868596d8e420 Mon Sep 17 00:00:00 2001
From: Marek Vasut <marex@denx.de>
Date: Wed, 23 Mar 2016 08:30:47 +0100
Subject: [PATCH 3/3] mtd: spi-nor: cqspi: Add bus lock

Lock the whole bus in .prepare callback and unlock the whole bus
in the .unprepare callback. This is necessary to prevent a race
condition when accessing two flashes, each of which has it's own
instance of struct spi_nor, in parallel. The SPI NOR framework
only locks the mutex in struct spi_nor and therefore the locking
happens with per-flash granularity, which is not enough to prevent
concurrent access to the SPI NOR controller registers.

Signed-off-by: Marek Vasut <marex@denx.de>
---
 drivers/mtd/spi-nor/cadence-quadspi.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index ee309cb..b086609 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -63,6 +63,7 @@ struct cqspi_st {
 	void __iomem		*iobase;
 	void __iomem		*ahb_base;
 	struct completion	transfer_complete;
+	struct mutex		bus_mutex;
 
 	int			current_cs;
 	unsigned long		master_ref_clk_hz;
@@ -919,7 +920,7 @@ static void cqspi_switch_cs(struct spi_nor *nor)
 	cqspi_chipselect(nor);
 }
 
-static int cqspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
+static int cqspi_prep_unlocked(struct spi_nor *nor, enum spi_nor_ops ops)
 {
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
 	struct cqspi_st *cqspi = f_pdata->cqspi;
@@ -950,31 +951,51 @@ static int cqspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
 	return 0;
 }
 
+static int cqspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+
+	mutex_lock(&cqspi->bus_mutex);
+
+	return cqspi_prep_unlocked(nor, ops);
+}
+
+static void cqspi_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+
+	mutex_unlock(&cqspi->bus_mutex);
+}
+
 static int cqspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 {
 	int ret;
 
 	ret = cqspi_set_protocol(nor, nor->reg_proto);
 	if (ret)
-		return ret;
+		goto exit;
 
-	cqspi_prep(nor, SPI_NOR_OPS_READ);
+	cqspi_prep_unlocked(nor, SPI_NOR_OPS_READ);
 
 	ret = cqspi_command_read(nor, &opcode, 1, buf, len);
+exit:
 	return ret;
 }
 
 static int cqspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 {
-	int ret = 0;
+	int ret;
 
 	ret = cqspi_set_protocol(nor, nor->reg_proto);
 	if (ret)
-		return ret;
+		goto exit;
 
-	cqspi_prep(nor, SPI_NOR_OPS_WRITE);
+	cqspi_prep_unlocked(nor, SPI_NOR_OPS_WRITE);
 
 	ret = cqspi_command_write(nor, opcode, buf, len);
+exit:
 	return ret;
 }
 
@@ -1113,6 +1134,7 @@ static int cqspi_setup_flash(struct cqspi_st *cqspi, struct device_node *np)
 		nor->write = cqspi_write;
 		nor->erase = cqspi_erase;
 		nor->prepare = cqspi_prep;
+		nor->unprepare = cqspi_unprep;
 
 		mtd->name = kasprintf(GFP_KERNEL, "%s.%d", dev_name(dev), cs);
 		if (!mtd->name) {
@@ -1154,6 +1176,7 @@ static int cqspi_probe(struct platform_device *pdev)
 	if (!cqspi)
 		return -ENOMEM;
 
+	mutex_init(&cqspi->bus_mutex);
 	cqspi->pdev = pdev;
 	platform_set_drvdata(pdev, cqspi);
 
-- 
2.7.0

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Vignesh R]

On 04/07/2016 01:00 AM, Marek Vasut wrote:
> On 04/06/2016 06:55 PM, R, Vignesh wrote:
>> Hi Marek,
> 
> Hi!
> 
>> I encountered a issue with this driver while testing.
> 
> Try with the attached patches, I am planning to use them for V11
> submission. I think you're hitting the problem with missing buslock.
> 

Thanks for the patches.
But I am pretty sure that's not the problem at my end, because I have
only one flash device on QSPI bus.

The problem is cqspi_switch_cs() is called only once ie when JEDEC ID is
being read(during autodetect of chip), but at that instance,
nor->page_size and nor->mtd.erasesize are not yet initialized (They are
initialized only after JEDEC ID is looked up in the table and page_size
and erasesize are known).
Therefore if nor->page_size is printed during cqspi_switch_cs() then its
zero. But nor->page_size reports 256 when printed in cqspi_flash_setup()
after spi_nor_scan(). Therefore CQSPI_REG_SIZE register has to be
configured only after spi_nor struct is fully populated (i.e after
spi_nor_scan() has recognized the slave after JEDEC ID read).


-- 
Regards
Vignesh

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Marek Vasut]

On 04/07/2016 06:55 AM, Vignesh R wrote:
> 
> 
> On 04/07/2016 01:00 AM, Marek Vasut wrote:
>> On 04/06/2016 06:55 PM, R, Vignesh wrote:
>>> Hi Marek,
>>
>> Hi!
>>
>>> I encountered a issue with this driver while testing.
>>
>> Try with the attached patches, I am planning to use them for V11
>> submission. I think you're hitting the problem with missing buslock.
>>
> 
> Thanks for the patches.
> But I am pretty sure that's not the problem at my end, because I have
> only one flash device on QSPI bus.

OK, sorry for the delayed reply. I will go through the rest of the QSPI
mail later, probably this week.

> The problem is cqspi_switch_cs() is called only once ie when JEDEC ID is
> being read(during autodetect of chip), but at that instance,
> nor->page_size and nor->mtd.erasesize are not yet initialized (They are
> initialized only after JEDEC ID is looked up in the table and page_size
> and erasesize are known).
> Therefore if nor->page_size is printed during cqspi_switch_cs() then its
> zero. But nor->page_size reports 256 when printed in cqspi_flash_setup()
> after spi_nor_scan(). Therefore CQSPI_REG_SIZE register has to be
> configured only after spi_nor struct is fully populated (i.e after
> spi_nor_scan() has recognized the slave after JEDEC ID read).
> 
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Marek Vasut]

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

On 04/07/2016 06:55 AM, Vignesh R wrote:
> 
> 
> On 04/07/2016 01:00 AM, Marek Vasut wrote:
>> On 04/06/2016 06:55 PM, R, Vignesh wrote:
>>> Hi Marek,
>>
>> Hi!
>>
>>> I encountered a issue with this driver while testing.
>>
>> Try with the attached patches, I am planning to use them for V11
>> submission. I think you're hitting the problem with missing buslock.
>>
> 
> Thanks for the patches.
> But I am pretty sure that's not the problem at my end, because I have
> only one flash device on QSPI bus.
> 
> The problem is cqspi_switch_cs() is called only once ie when JEDEC ID is
> being read(during autodetect of chip), but at that instance,
> nor->page_size and nor->mtd.erasesize are not yet initialized (They are
> initialized only after JEDEC ID is looked up in the table and page_size
> and erasesize are known).
> Therefore if nor->page_size is printed during cqspi_switch_cs() then its
> zero. But nor->page_size reports 256 when printed in cqspi_flash_setup()
> after spi_nor_scan(). Therefore CQSPI_REG_SIZE register has to be
> configured only after spi_nor struct is fully populated (i.e after
> spi_nor_scan() has recognized the slave after JEDEC ID read).

Got it and I have a patch for this. Nice find, thanks! It gave me 60%
read performance boost on my machine :-)

I am now caching the page_size, erasesize, addr_width values, so I can
avoid reconfiguring the controller if there is no need for it, but
reconfigure it if there is a need. The patch is attached, but it's quite
big, so I also pushed a git branch with this driver for your convenience
(based on linux-next, expect rebases):

https://git.kernel.org/cgit/linux/kernel/git/marex/linux-2.6.git/log/?h=next/cadence-qspi

-- 
Best regards,
Marek Vasut

[-- Attachment #2: qspi.patch --]
[-- Type: text/x-patch, Size: 10714 bytes --]

diff --git a/drivers/mtd/spi-nor/cadence-quadspi.c b/drivers/mtd/spi-nor/cadence-quadspi.c
index 7e61fba..331e7d1 100644
--- a/drivers/mtd/spi-nor/cadence-quadspi.c
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -66,6 +66,9 @@ struct cqspi_st {
 	struct mutex		bus_mutex;
 
 	int			current_cs;
+	int			current_page_size;
+	int			current_erase_size;
+	int			current_addr_width;
 	unsigned long		master_ref_clk_hz;
 	bool			is_decoded_cs;
 	u32			fifo_depth;
@@ -654,109 +657,60 @@ failwr:
 	return ret;
 }
 
-static int cqspi_set_protocol(struct spi_nor *nor, enum spi_nor_protocol proto)
+static void cqspi_chipselect(struct spi_nor *nor)
 {
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	void __iomem *reg_base = cqspi->iobase;
+	unsigned int chip_select = f_pdata->cs;
+	unsigned int reg;
 
-	switch (proto) {
-	case SNOR_PROTO_1_1_1:
-	case SNOR_PROTO_1_1_2:
-	case SNOR_PROTO_1_1_4:
-	case SNOR_PROTO_1_2_2:
-	case SNOR_PROTO_1_4_4:
-		f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE;
-		break;
-	case SNOR_PROTO_2_2_2:
-		f_pdata->inst_width = CQSPI_INST_TYPE_DUAL;
-		break;
-	case SNOR_PROTO_4_4_4:
-		f_pdata->inst_width = CQSPI_INST_TYPE_QUAD;
-		break;
-	default:
-		return -EINVAL;
-	}
+	reg = readl(reg_base + CQSPI_REG_CONFIG);
+	if (cqspi->is_decoded_cs) {
+		reg |= CQSPI_REG_CONFIG_DECODE_MASK;
+	} else {
+		reg &= ~CQSPI_REG_CONFIG_DECODE_MASK;
 
-	switch (proto) {
-	case SNOR_PROTO_1_1_1:
-	case SNOR_PROTO_1_1_2:
-	case SNOR_PROTO_1_1_4:
-		f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE;
-		break;
-	case SNOR_PROTO_1_2_2:
-	case SNOR_PROTO_2_2_2:
-		f_pdata->addr_width = CQSPI_INST_TYPE_DUAL;
-		break;
-	case SNOR_PROTO_1_4_4:
-	case SNOR_PROTO_4_4_4:
-		f_pdata->addr_width = CQSPI_INST_TYPE_QUAD;
-		break;
-	default:
-		return -EINVAL;
+		/* Convert CS if without decoder.
+		 * CS0 to 4b'1110
+		 * CS1 to 4b'1101
+		 * CS2 to 4b'1011
+		 * CS3 to 4b'0111
+		 */
+		chip_select = 0xF & ~(1 << chip_select);
 	}
 
-	return 0;
-}
-
-static void cqspi_write(struct spi_nor *nor, loff_t to,
-			size_t len, size_t *retlen, const u_char *buf)
-{
-	int ret;
-
-	ret = cqspi_set_protocol(nor, nor->write_proto);
-	if (ret)
-		return;
-
-	ret = cqspi_indirect_write_setup(nor, to);
-	if (ret)
-		return;
-
-	ret = cqspi_indirect_write_execute(nor, buf, len);
-	if (ret)
-		return;
-
-	*retlen += len;
-}
-
-static int cqspi_read(struct spi_nor *nor, loff_t from,
-		      size_t len, size_t *retlen, u_char *buf)
-{
-	int ret;
-
-	ret = cqspi_set_protocol(nor, nor->read_proto);
-	if (ret)
-		return ret;
-
-	ret = cqspi_indirect_read_setup(nor, from);
-	if (ret)
-		return ret;
-
-	ret = cqspi_indirect_read_execute(nor, buf, len);
-	if (ret)
-		return ret;
-
-	*retlen += len;
-	return ret;
+	reg &= ~(CQSPI_REG_CONFIG_CHIPSELECT_MASK
+		 << CQSPI_REG_CONFIG_CHIPSELECT_LSB);
+	reg |= (chip_select & CQSPI_REG_CONFIG_CHIPSELECT_MASK)
+	    << CQSPI_REG_CONFIG_CHIPSELECT_LSB;
+	writel(reg, reg_base + CQSPI_REG_CONFIG);
 }
 
-static int cqspi_erase(struct spi_nor *nor, loff_t offs)
+static void cqspi_configure_cs_and_sizes(struct spi_nor *nor)
 {
-	int ret;
-
-	ret = cqspi_set_protocol(nor, nor->erase_proto);
-	if (ret)
-		return ret;
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+	struct cqspi_st *cqspi = f_pdata->cqspi;
+	void __iomem *iobase = cqspi->iobase;
+	unsigned int reg;
 
-	/* Send write enable, then erase commands. */
-	ret = nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0);
-	if (ret)
-		return ret;
+	/* configure page size and block size. */
+	reg = readl(iobase + CQSPI_REG_SIZE);
+	reg &= ~(CQSPI_REG_SIZE_PAGE_MASK << CQSPI_REG_SIZE_PAGE_LSB);
+	reg &= ~(CQSPI_REG_SIZE_BLOCK_MASK << CQSPI_REG_SIZE_BLOCK_LSB);
+	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
+	reg |= (nor->page_size << CQSPI_REG_SIZE_PAGE_LSB);
+	reg |= (ilog2(nor->mtd.erasesize) << CQSPI_REG_SIZE_BLOCK_LSB);
+	reg |= (nor->addr_width - 1);
+	writel(reg, iobase + CQSPI_REG_SIZE);
 
-	/* Set up command buffer. */
-	ret = cqspi_command_write_addr(nor, nor->erase_opcode, offs);
-	if (ret)
-		return ret;
+	/* configure the chip select */
+	cqspi_chipselect(nor);
 
-	return 0;
+	/* Store the new configuration of the controller */
+	cqspi->current_page_size = nor->page_size;
+	cqspi->current_erase_size = nor->mtd.erasesize;
+	cqspi->current_addr_width = nor->addr_width;
 }
 
 static unsigned int calculate_ticks_for_ns(const unsigned int ref_clk_hz,
@@ -854,36 +808,6 @@ static void cqspi_readdata_capture(struct cqspi_st *cqspi,
 	writel(reg, reg_base + CQSPI_REG_READCAPTURE);
 }
 
-static void cqspi_chipselect(struct spi_nor *nor)
-{
-	struct cqspi_flash_pdata *f_pdata = nor->priv;
-	struct cqspi_st *cqspi = f_pdata->cqspi;
-	void __iomem *reg_base = cqspi->iobase;
-	unsigned int chip_select = f_pdata->cs;
-	unsigned int reg;
-
-	reg = readl(reg_base + CQSPI_REG_CONFIG);
-	if (cqspi->is_decoded_cs) {
-		reg |= CQSPI_REG_CONFIG_DECODE_MASK;
-	} else {
-		reg &= ~CQSPI_REG_CONFIG_DECODE_MASK;
-
-		/* Convert CS if without decoder.
-		 * CS0 to 4b'1110
-		 * CS1 to 4b'1101
-		 * CS2 to 4b'1011
-		 * CS3 to 4b'0111
-		 */
-		chip_select = 0xF & ~(1 << chip_select);
-	}
-
-	reg &= ~(CQSPI_REG_CONFIG_CHIPSELECT_MASK
-		 << CQSPI_REG_CONFIG_CHIPSELECT_LSB);
-	reg |= (chip_select & CQSPI_REG_CONFIG_CHIPSELECT_MASK)
-	    << CQSPI_REG_CONFIG_CHIPSELECT_LSB;
-	writel(reg, reg_base + CQSPI_REG_CONFIG);
-}
-
 static void cqspi_controller_enable(struct cqspi_st *cqspi, bool enable)
 {
 	void __iomem *reg_base = cqspi->iobase;
@@ -899,34 +823,18 @@ static void cqspi_controller_enable(struct cqspi_st *cqspi, bool enable)
 	writel(reg, reg_base + CQSPI_REG_CONFIG);
 }
 
-static void cqspi_switch_cs(struct spi_nor *nor)
-{
-	struct cqspi_flash_pdata *f_pdata = nor->priv;
-	struct cqspi_st *cqspi = f_pdata->cqspi;
-	void __iomem *iobase = cqspi->iobase;
-	unsigned int reg;
-
-	/* configure page size and block size. */
-	reg = readl(iobase + CQSPI_REG_SIZE);
-	reg &= ~(CQSPI_REG_SIZE_PAGE_MASK << CQSPI_REG_SIZE_PAGE_LSB);
-	reg &= ~(CQSPI_REG_SIZE_BLOCK_MASK << CQSPI_REG_SIZE_BLOCK_LSB);
-	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
-	reg |= (nor->page_size << CQSPI_REG_SIZE_PAGE_LSB);
-	reg |= (ilog2(nor->mtd.erasesize) << CQSPI_REG_SIZE_BLOCK_LSB);
-	reg |= (nor->addr_width - 1);
-	writel(reg, iobase + CQSPI_REG_SIZE);
-
-	/* configure the chip select */
-	cqspi_chipselect(nor);
-}
-
-static int cqspi_prep_unlocked(struct spi_nor *nor, enum spi_nor_ops ops)
+static void cqspi_configure(struct spi_nor *nor)
 {
 	struct cqspi_flash_pdata *f_pdata = nor->priv;
 	struct cqspi_st *cqspi = f_pdata->cqspi;
 	const unsigned int sclk = f_pdata->clk_rate;
-	const int switch_cs = (cqspi->current_cs != f_pdata->cs);
-	const int switch_ck = (cqspi->sclk != sclk);
+	int switch_cs = (cqspi->current_cs != f_pdata->cs);
+	int switch_ck = (cqspi->sclk != sclk);
+
+	if ((cqspi->current_page_size != nor->page_size) ||
+	    (cqspi->current_erase_size != nor->mtd.erasesize) ||
+	    (cqspi->current_addr_width != nor->addr_width))
+		switch_cs = 1;
 
 	if (switch_cs || switch_ck)
 		cqspi_controller_enable(cqspi, 0);
@@ -934,7 +842,7 @@ static int cqspi_prep_unlocked(struct spi_nor *nor, enum spi_nor_ops ops)
 	/* Switch chip select. */
 	if (switch_cs) {
 		cqspi->current_cs = f_pdata->cs;
-		cqspi_switch_cs(nor);
+		cqspi_configure_cs_and_sizes(nor);
 	}
 
 	/* Setup baudrate divisor and delays */
@@ -947,6 +855,111 @@ static int cqspi_prep_unlocked(struct spi_nor *nor, enum spi_nor_ops ops)
 
 	if (switch_cs || switch_ck)
 		cqspi_controller_enable(cqspi, 1);
+}
+
+static int cqspi_set_protocol(struct spi_nor *nor, enum spi_nor_protocol proto)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+
+	switch (proto) {
+	case SNOR_PROTO_1_1_1:
+	case SNOR_PROTO_1_1_2:
+	case SNOR_PROTO_1_1_4:
+	case SNOR_PROTO_1_2_2:
+	case SNOR_PROTO_1_4_4:
+		f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE;
+		break;
+	case SNOR_PROTO_2_2_2:
+		f_pdata->inst_width = CQSPI_INST_TYPE_DUAL;
+		break;
+	case SNOR_PROTO_4_4_4:
+		f_pdata->inst_width = CQSPI_INST_TYPE_QUAD;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	switch (proto) {
+	case SNOR_PROTO_1_1_1:
+	case SNOR_PROTO_1_1_2:
+	case SNOR_PROTO_1_1_4:
+		f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE;
+		break;
+	case SNOR_PROTO_1_2_2:
+	case SNOR_PROTO_2_2_2:
+		f_pdata->addr_width = CQSPI_INST_TYPE_DUAL;
+		break;
+	case SNOR_PROTO_1_4_4:
+	case SNOR_PROTO_4_4_4:
+		f_pdata->addr_width = CQSPI_INST_TYPE_QUAD;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	cqspi_configure(nor);
+
+	return 0;
+}
+
+static void cqspi_write(struct spi_nor *nor, loff_t to,
+			size_t len, size_t *retlen, const u_char *buf)
+{
+	int ret;
+
+	ret = cqspi_set_protocol(nor, nor->write_proto);
+	if (ret)
+		return;
+
+	ret = cqspi_indirect_write_setup(nor, to);
+	if (ret)
+		return;
+
+	ret = cqspi_indirect_write_execute(nor, buf, len);
+	if (ret)
+		return;
+
+	*retlen += len;
+}
+
+static int cqspi_read(struct spi_nor *nor, loff_t from,
+		      size_t len, size_t *retlen, u_char *buf)
+{
+	int ret;
+
+	ret = cqspi_set_protocol(nor, nor->read_proto);
+	if (ret)
+		return ret;
+
+	ret = cqspi_indirect_read_setup(nor, from);
+	if (ret)
+		return ret;
+
+	ret = cqspi_indirect_read_execute(nor, buf, len);
+	if (ret)
+		return ret;
+
+	*retlen += len;
+	return ret;
+}
+
+static int cqspi_erase(struct spi_nor *nor, loff_t offs)
+{
+	int ret;
+
+	ret = cqspi_set_protocol(nor, nor->erase_proto);
+	if (ret)
+		return ret;
+
+	/* Send write enable, then erase commands. */
+	ret = nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0);
+	if (ret)
+		return ret;
+
+	/* Set up command buffer. */
+	ret = cqspi_command_write_addr(nor, nor->erase_opcode, offs);
+	if (ret)
+		return ret;
 
 	return 0;
 }
@@ -958,7 +971,7 @@ static int cqspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
 
 	mutex_lock(&cqspi->bus_mutex);
 
-	return cqspi_prep_unlocked(nor, ops);
+	return 0;
 }
 
 static void cqspi_unprep(struct spi_nor *nor, enum spi_nor_ops ops)
@@ -974,13 +987,9 @@ static int cqspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 	int ret;
 
 	ret = cqspi_set_protocol(nor, nor->reg_proto);
-	if (ret)
-		goto exit;
-
-	cqspi_prep_unlocked(nor, SPI_NOR_OPS_READ);
+	if (!ret)
+		ret = cqspi_command_read(nor, &opcode, 1, buf, len);
 
-	ret = cqspi_command_read(nor, &opcode, 1, buf, len);
-exit:
 	return ret;
 }
 
@@ -989,13 +998,9 @@ static int cqspi_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
 	int ret;
 
 	ret = cqspi_set_protocol(nor, nor->reg_proto);
-	if (ret)
-		goto exit;
-
-	cqspi_prep_unlocked(nor, SPI_NOR_OPS_WRITE);
+	if (!ret)
+		ret = cqspi_command_write(nor, opcode, buf, len);
 
-	ret = cqspi_command_write(nor, opcode, buf, len);
-exit:
 	return ret;
 }
 

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[R, Vignesh]

On 04/13/2016 08:36 PM, Marek Vasut wrote:
> On 04/07/2016 06:55 AM, Vignesh R wrote:
>>
>>
>> On 04/07/2016 01:00 AM, Marek Vasut wrote:
>>> On 04/06/2016 06:55 PM, R, Vignesh wrote:
>>>> Hi Marek,
>>>
>>> Hi!
>>>
>>>> I encountered a issue with this driver while testing.
>>>
>>> Try with the attached patches, I am planning to use them for V11
>>> submission. I think you're hitting the problem with missing buslock.
>>>
>>
>> Thanks for the patches.
>> But I am pretty sure that's not the problem at my end, because I have
>> only one flash device on QSPI bus.
>>
>> The problem is cqspi_switch_cs() is called only once ie when JEDEC ID is
>> being read(during autodetect of chip), but at that instance,
>> nor->page_size and nor->mtd.erasesize are not yet initialized (They are
>> initialized only after JEDEC ID is looked up in the table and page_size
>> and erasesize are known).
>> Therefore if nor->page_size is printed during cqspi_switch_cs() then its
>> zero. But nor->page_size reports 256 when printed in cqspi_flash_setup()
>> after spi_nor_scan(). Therefore CQSPI_REG_SIZE register has to be
>> configured only after spi_nor struct is fully populated (i.e after
>> spi_nor_scan() has recognized the slave after JEDEC ID read).
> 
> Got it and I have a patch for this. Nice find, thanks! It gave me 60%
> read performance boost on my machine :-)
>

Ah, hope I will see similar improvement at my end :)

> I am now caching the page_size, erasesize, addr_width values, so I can
> avoid reconfiguring the controller if there is no need for it, but
> reconfigure it if there is a need. The patch is attached, but it's quite
> big, so I also pushed a git branch with this driver for your convenience
> (based on linux-next, expect rebases):
> 
> https://git.kernel.org/cgit/linux/kernel/git/marex/linux-2.6.git/log/?h=next/cadence-qspi
> 

Ok, I will test this on my board sometime soon, thanks.

-- 
Regards
Vignesh

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Marek Vasut]

On 04/14/2016 06:41 PM, R, Vignesh wrote:
> 
> 
> On 04/13/2016 08:36 PM, Marek Vasut wrote:
>> On 04/07/2016 06:55 AM, Vignesh R wrote:
>>>
>>>
>>> On 04/07/2016 01:00 AM, Marek Vasut wrote:
>>>> On 04/06/2016 06:55 PM, R, Vignesh wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi!
>>>>
>>>>> I encountered a issue with this driver while testing.
>>>>
>>>> Try with the attached patches, I am planning to use them for V11
>>>> submission. I think you're hitting the problem with missing buslock.
>>>>
>>>
>>> Thanks for the patches.
>>> But I am pretty sure that's not the problem at my end, because I have
>>> only one flash device on QSPI bus.
>>>
>>> The problem is cqspi_switch_cs() is called only once ie when JEDEC ID is
>>> being read(during autodetect of chip), but at that instance,
>>> nor->page_size and nor->mtd.erasesize are not yet initialized (They are
>>> initialized only after JEDEC ID is looked up in the table and page_size
>>> and erasesize are known).
>>> Therefore if nor->page_size is printed during cqspi_switch_cs() then its
>>> zero. But nor->page_size reports 256 when printed in cqspi_flash_setup()
>>> after spi_nor_scan(). Therefore CQSPI_REG_SIZE register has to be
>>> configured only after spi_nor struct is fully populated (i.e after
>>> spi_nor_scan() has recognized the slave after JEDEC ID read).
>>
>> Got it and I have a patch for this. Nice find, thanks! It gave me 60%
>> read performance boost on my machine :-)
>>
> 
> Ah, hope I will see similar improvement at my end :)
> 
>> I am now caching the page_size, erasesize, addr_width values, so I can
>> avoid reconfiguring the controller if there is no need for it, but
>> reconfigure it if there is a need. The patch is attached, but it's quite
>> big, so I also pushed a git branch with this driver for your convenience
>> (based on linux-next, expect rebases):
>>
>> https://git.kernel.org/cgit/linux/kernel/git/marex/linux-2.6.git/log/?h=next/cadence-qspi
>>
> 
> Ok, I will test this on my board sometime soon, thanks.
> 
Thanks! Let me know how it went :)

There are now new patches from Cyrille, so I will rebase the driver on
top of those and push when ready.

-- 
Best regards,
Marek Vasut

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Trent Piepho]

On Mon, 2016-01-11 at 05:34 +0100, Marek Vasut wrote:
> From: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> 
> Add support for the Cadence QSPI controller. This controller is
> present in the Altera SoCFPGA SoCs and this driver has been tested
> on the Cyclone V SoC.

I'm trying to use this driver the Alaric Devkit for Altera's Arria10
SoC.  It's not working so far.  In the course of trying to debug it,
I've found a few things with the driver in the socfpga-4.1-ltsi branch.
However most of them are in code that's not present in this driver,
which is the newest post of the code I could find to reply to.

> +					 CQSPI_REG_IRQ_WATERMARK	| \
> +					 CQSPI_REG_IRQ_UNDERFLOW)
> +
> +#define CQSPI_IRQ_STATUS_MASK		0x1FFFF
> +

Perhaps a comment.
/* waits for all bits set in mask to be zero (clear==false) or one
(clear==true) */

> +static int cqspi_wait_for_bit(void __iomem *reg, const u32 mask, bool clear)
> +{
> +	unsigned long end = jiffies + msecs_to_jiffies(CQSPI_TIMEOUT_MS);
> +	u32 val;
> +
> +	while (1) {
> +		val = readl(reg);
> +		if (clear)
> +			val = ~val;
> +		val &= mask;
> +
> +		if (val == mask)
> +			return 0;

Somewhat simpler.

if ((readl(reg) & mask) == (clear ? 0 : mask))
        return 0;

> +
> +		if (time_after(jiffies, end))
> +			return -ETIMEDOUT;

Note that there is a hypervisor/vm/long hardirq etc. bug that can happen
without timeouts like this.  What happens is after the check of the bits
fails, there is a very long delay before this task runs again.  This can
be easy if one is running under virtualization.  The the time_after call
reports the timeout expired.  But the last time it the bit was checked,
before the long delay, was well before the timeout expired.  The way to
avoid this is to always be sure to check the condition once after the
timeout expired.  This is sure to give the full timeout.


> +	}
> +}
> +

> +
> +static unsigned int calculate_ticks_for_ns(const unsigned int ref_clk_hz,
> +					   const unsigned int ns_val)
> +{
> +	unsigned int ticks;
> +
> +	ticks = ref_clk_hz / 1000;	/* kHz */
This division doesn't round up.

> +	ticks = DIV_ROUND_UP(ticks * ns_val, 1000000);
But this one does.

> +
> +	return ticks;
> +}
> +
> +static void cqspi_delay(struct spi_nor *nor, const unsigned int sclk_hz)
> +{
> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
> +	struct cqspi_st *cqspi = f_pdata->cqspi;

Isn't the sclk_hz parameter of this function already available here as
cqspi->sclk?

> +	void __iomem *iobase = cqspi->iobase;
> +	const unsigned int ref_clk_hz = cqspi->master_ref_clk_hz;
> +	unsigned int tshsl, tchsh, tslch, tsd2d;
> +	unsigned int reg;
> +	unsigned int tsclk;
> +
> +	/* calculate the number of ref ticks for one sclk tick */
> +	tsclk = (ref_clk_hz + sclk_hz - 1) / sclk_hz;

DIV_ROUND_UP(ref_clk_hz, sclk_hz);

> +
> +	tshsl = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tshsl_ns);
> +	/* this particular value must be at least one sclk */
> +	if (tshsl < tsclk)
> +		tshsl = tsclk;
> +
> +	tchsh = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tchsh_ns);
> +	tslch = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tslch_ns);
> +	tsd2d = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tsd2d_ns);
> +
> +	reg = (tshsl & CQSPI_REG_DELAY_TSHSL_MASK)
> +	       << CQSPI_REG_DELAY_TSHSL_LSB;
> +	reg |= (tchsh & CQSPI_REG_DELAY_TCHSH_MASK)
> +		<< CQSPI_REG_DELAY_TCHSH_LSB;
> +	reg |= (tslch & CQSPI_REG_DELAY_TSLCH_MASK)
> +		<< CQSPI_REG_DELAY_TSLCH_LSB;
> +	reg |= (tsd2d & CQSPI_REG_DELAY_TSD2D_MASK)
> +		<< CQSPI_REG_DELAY_TSD2D_LSB;
> +	writel(reg, iobase + CQSPI_REG_DELAY);
> +}
> +
> +static void cqspi_config_baudrate_div(struct cqspi_st *cqspi,
> +				      const unsigned int sclk_hz)
> +{

sclk_hz is available as cqspi->sclk.

> +	const unsigned int ref_clk_hz = cqspi->master_ref_clk_hz;
> +	void __iomem *reg_base = cqspi->iobase;
> +	unsigned int reg;
> +	unsigned int div;
> +
> +	reg = readl(reg_base + CQSPI_REG_CONFIG);
> +	reg &= ~(CQSPI_REG_CONFIG_BAUD_MASK << CQSPI_REG_CONFIG_BAUD_LSB);
> +
> +	div = ref_clk_hz / sclk_hz;

Should this round up too?

> +
> +	/* Recalculate the baudrate divisor based on QSPI specification. */
> +	if (div > 32)
> +		div = 32;
> +
> +	/* Check if even number. */
> +	if (div & 1)
> +		div = (div / 2);
> +	else
> +		div = (div / 2) - 1;

Wouldn't this be the same as div = DIV_ROUND_UP(div, 2) - 1;

The entire div calculation could be done with:

   /* Register programmed with divider minus 1 */
   div = DIV_ROUND_UP(ref_clk_hz, s_clk_hz * 2) - 1;
   if (div > 15)
        div = 15;


> +
> +	div = (div & CQSPI_REG_CONFIG_BAUD_MASK) << CQSPI_REG_CONFIG_BAUD_LSB;
> +	reg |= div;
> +	writel(reg, reg_base + CQSPI_REG_CONFIG);
> +}
> +

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Marek Vasut]

On 05/13/2016 02:00 AM, Trent Piepho wrote:
> On Mon, 2016-01-11 at 05:34 +0100, Marek Vasut wrote:
>> From: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>
>> Add support for the Cadence QSPI controller. This controller is
>> present in the Altera SoCFPGA SoCs and this driver has been tested
>> on the Cyclone V SoC.
> 
> I'm trying to use this driver the Alaric Devkit for Altera's Arria10
> SoC.  It's not working so far.  In the course of trying to debug it,
> I've found a few things with the driver in the socfpga-4.1-ltsi branch.

So are you trying to debug this driver or some other out-of-tree driver?
btw. I pushed the latest version here:

https://git.kernel.org/cgit/linux/kernel/git/marex/linux-2.6.git/log/?h=next/cadence-qspi

> However most of them are in code that's not present in this driver,
> which is the newest post of the code I could find to reply to.

I will check the rest later, thanks!

>> +					 CQSPI_REG_IRQ_WATERMARK	| \
>> +					 CQSPI_REG_IRQ_UNDERFLOW)
>> +
>> +#define CQSPI_IRQ_STATUS_MASK		0x1FFFF
>> +
> 
> Perhaps a comment.
> /* waits for all bits set in mask to be zero (clear==false) or one
> (clear==true) */
> 
>> +static int cqspi_wait_for_bit(void __iomem *reg, const u32 mask, bool clear)
>> +{
>> +	unsigned long end = jiffies + msecs_to_jiffies(CQSPI_TIMEOUT_MS);
>> +	u32 val;
>> +
>> +	while (1) {
>> +		val = readl(reg);
>> +		if (clear)
>> +			val = ~val;
>> +		val &= mask;
>> +
>> +		if (val == mask)
>> +			return 0;
> 
> Somewhat simpler.
> 
> if ((readl(reg) & mask) == (clear ? 0 : mask))
>         return 0;
> 
>> +
>> +		if (time_after(jiffies, end))
>> +			return -ETIMEDOUT;
> 
> Note that there is a hypervisor/vm/long hardirq etc. bug that can happen
> without timeouts like this.  What happens is after the check of the bits
> fails, there is a very long delay before this task runs again.  This can
> be easy if one is running under virtualization.  The the time_after call
> reports the timeout expired.  But the last time it the bit was checked,
> before the long delay, was well before the timeout expired.  The way to
> avoid this is to always be sure to check the condition once after the
> timeout expired.  This is sure to give the full timeout.
> 
> 
>> +	}
>> +}
>> +
> 
>> +
>> +static unsigned int calculate_ticks_for_ns(const unsigned int ref_clk_hz,
>> +					   const unsigned int ns_val)
>> +{
>> +	unsigned int ticks;
>> +
>> +	ticks = ref_clk_hz / 1000;	/* kHz */
> This division doesn't round up.
> 
>> +	ticks = DIV_ROUND_UP(ticks * ns_val, 1000000);
> But this one does.
> 
>> +
>> +	return ticks;
>> +}
>> +
>> +static void cqspi_delay(struct spi_nor *nor, const unsigned int sclk_hz)
>> +{
>> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
>> +	struct cqspi_st *cqspi = f_pdata->cqspi;
> 
> Isn't the sclk_hz parameter of this function already available here as
> cqspi->sclk?
> 
>> +	void __iomem *iobase = cqspi->iobase;
>> +	const unsigned int ref_clk_hz = cqspi->master_ref_clk_hz;
>> +	unsigned int tshsl, tchsh, tslch, tsd2d;
>> +	unsigned int reg;
>> +	unsigned int tsclk;
>> +
>> +	/* calculate the number of ref ticks for one sclk tick */
>> +	tsclk = (ref_clk_hz + sclk_hz - 1) / sclk_hz;
> 
> DIV_ROUND_UP(ref_clk_hz, sclk_hz);
> 
>> +
>> +	tshsl = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tshsl_ns);
>> +	/* this particular value must be at least one sclk */
>> +	if (tshsl < tsclk)
>> +		tshsl = tsclk;
>> +
>> +	tchsh = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tchsh_ns);
>> +	tslch = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tslch_ns);
>> +	tsd2d = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tsd2d_ns);
>> +
>> +	reg = (tshsl & CQSPI_REG_DELAY_TSHSL_MASK)
>> +	       << CQSPI_REG_DELAY_TSHSL_LSB;
>> +	reg |= (tchsh & CQSPI_REG_DELAY_TCHSH_MASK)
>> +		<< CQSPI_REG_DELAY_TCHSH_LSB;
>> +	reg |= (tslch & CQSPI_REG_DELAY_TSLCH_MASK)
>> +		<< CQSPI_REG_DELAY_TSLCH_LSB;
>> +	reg |= (tsd2d & CQSPI_REG_DELAY_TSD2D_MASK)
>> +		<< CQSPI_REG_DELAY_TSD2D_LSB;
>> +	writel(reg, iobase + CQSPI_REG_DELAY);
>> +}
>> +
>> +static void cqspi_config_baudrate_div(struct cqspi_st *cqspi,
>> +				      const unsigned int sclk_hz)
>> +{
> 
> sclk_hz is available as cqspi->sclk.
> 
>> +	const unsigned int ref_clk_hz = cqspi->master_ref_clk_hz;
>> +	void __iomem *reg_base = cqspi->iobase;
>> +	unsigned int reg;
>> +	unsigned int div;
>> +
>> +	reg = readl(reg_base + CQSPI_REG_CONFIG);
>> +	reg &= ~(CQSPI_REG_CONFIG_BAUD_MASK << CQSPI_REG_CONFIG_BAUD_LSB);
>> +
>> +	div = ref_clk_hz / sclk_hz;
> 
> Should this round up too?
> 
>> +
>> +	/* Recalculate the baudrate divisor based on QSPI specification. */
>> +	if (div > 32)
>> +		div = 32;
>> +
>> +	/* Check if even number. */
>> +	if (div & 1)
>> +		div = (div / 2);
>> +	else
>> +		div = (div / 2) - 1;
> 
> Wouldn't this be the same as div = DIV_ROUND_UP(div, 2) - 1;
> 
> The entire div calculation could be done with:
> 
>    /* Register programmed with divider minus 1 */
>    div = DIV_ROUND_UP(ref_clk_hz, s_clk_hz * 2) - 1;
>    if (div > 15)
>         div = 15;
> 
> 
>> +
>> +	div = (div & CQSPI_REG_CONFIG_BAUD_MASK) << CQSPI_REG_CONFIG_BAUD_LSB;
>> +	reg |= div;
>> +	writel(reg, reg_base + CQSPI_REG_CONFIG);
>> +}
>> +
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Trent Piepho]

On Fri, 2016-05-13 at 02:24 +0200, Marek Vasut wrote:
> On 05/13/2016 02:00 AM, Trent Piepho wrote:
> > On Mon, 2016-01-11 at 05:34 +0100, Marek Vasut wrote:
> >> From: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> >>
> >> Add support for the Cadence QSPI controller. This controller is
> >> present in the Altera SoCFPGA SoCs and this driver has been tested
> >> on the Cyclone V SoC.
> > 
> > I'm trying to use this driver the Alaric Devkit for Altera's Arria10
> > SoC.  It's not working so far.  In the course of trying to debug it,
> > I've found a few things with the driver in the socfpga-4.1-ltsi branch.
> 
> So are you trying to debug this driver or some other out-of-tree driver?
> btw. I pushed the latest version here:
> 
> https://git.kernel.org/cgit/linux/kernel/git/marex/linux-2.6.git/log/?h=next/cadence-qspi

I'm trying to use the out of tree driver from Altera's latest kernel,
from here:
https://github.com/altera-opensource/linux-socfpga/tree/socfpga-4.4

But it's not a totally different driver, it's just an older version of
this driver.  I'm thinking of porting in this version since I think it's
much improved (after V11 one would hope so).  Really, Altera should
update their supported kernel with this code.

Comments about what's on your branch.

Commit "ARM: socfpga: Add Candence QSPI controller DT node" only
adds the node for Cyclone 5.  This same DT node should also be added to
the socfpga_arria10.dtsi file.

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Marek Vasut]

On 05/13/2016 10:43 PM, Trent Piepho wrote:
> On Fri, 2016-05-13 at 02:24 +0200, Marek Vasut wrote:
>> On 05/13/2016 02:00 AM, Trent Piepho wrote:
>>> On Mon, 2016-01-11 at 05:34 +0100, Marek Vasut wrote:
>>>> From: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>>>
>>>> Add support for the Cadence QSPI controller. This controller is
>>>> present in the Altera SoCFPGA SoCs and this driver has been tested
>>>> on the Cyclone V SoC.
>>>
>>> I'm trying to use this driver the Alaric Devkit for Altera's Arria10
>>> SoC.  It's not working so far.  In the course of trying to debug it,
>>> I've found a few things with the driver in the socfpga-4.1-ltsi branch.
>>
>> So are you trying to debug this driver or some other out-of-tree driver?
>> btw. I pushed the latest version here:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/marex/linux-2.6.git/log/?h=next/cadence-qspi
> 
> I'm trying to use the out of tree driver from Altera's latest kernel,
> from here:
> https://github.com/altera-opensource/linux-socfpga/tree/socfpga-4.4
> 
> But it's not a totally different driver, it's just an older version of
> this driver.  I'm thinking of porting in this version since I think it's
> much improved (after V11 one would hope so).  Really, Altera should
> update their supported kernel with this code.

I'm quite sure they're doing their best and really, they _are_ better
than most vendors.

> Comments about what's on your branch.
> 
> Commit "ARM: socfpga: Add Candence QSPI controller DT node" only
> adds the node for Cyclone 5.  This same DT node should also be added to
> the socfpga_arria10.dtsi file.
> 
I don't have A10 devkit (yet), but feel free to send a patch once this
hits mainline if you can test it on A10. I updated the commit message
until then.

The latest stuff is pushed here:
http://git.kernel.org/cgit/linux/kernel/git/marex/linux-2.6.git/log/?h=korg/next/cadence-qspi-v12

-- 
Best regards,
Marek Vasut

Re: [PATCH V10 2/2] mtd: spi-nor: Add driver for Cadence Quad SPI Flash Controller.

[Marek Vasut]

On 05/13/2016 02:00 AM, Trent Piepho wrote:
> On Mon, 2016-01-11 at 05:34 +0100, Marek Vasut wrote:
>> From: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>
>> Add support for the Cadence QSPI controller. This controller is
>> present in the Altera SoCFPGA SoCs and this driver has been tested
>> on the Cyclone V SoC.
> 
> I'm trying to use this driver the Alaric Devkit for Altera's Arria10
> SoC.  It's not working so far.  In the course of trying to debug it,
> I've found a few things with the driver in the socfpga-4.1-ltsi branch.
> However most of them are in code that's not present in this driver,
> which is the newest post of the code I could find to reply to.
> 
>> +					 CQSPI_REG_IRQ_WATERMARK	| \
>> +					 CQSPI_REG_IRQ_UNDERFLOW)
>> +
>> +#define CQSPI_IRQ_STATUS_MASK		0x1FFFF
>> +
> 
> Perhaps a comment.
> /* waits for all bits set in mask to be zero (clear==false) or one
> (clear==true) */
> 
>> +static int cqspi_wait_for_bit(void __iomem *reg, const u32 mask, bool clear)
>> +{
>> +	unsigned long end = jiffies + msecs_to_jiffies(CQSPI_TIMEOUT_MS);
>> +	u32 val;
>> +
>> +	while (1) {
>> +		val = readl(reg);
>> +		if (clear)
>> +			val = ~val;
>> +		val &= mask;
>> +
>> +		if (val == mask)
>> +			return 0;
> 
> Somewhat simpler.
> 
> if ((readl(reg) & mask) == (clear ? 0 : mask))
>         return 0;

This looks far more cryptic though, so I will skip this one.

>> +
>> +		if (time_after(jiffies, end))
>> +			return -ETIMEDOUT;
> 
> Note that there is a hypervisor/vm/long hardirq etc. bug that can happen
> without timeouts like this.  What happens is after the check of the bits
> fails, there is a very long delay before this task runs again.  This can
> be easy if one is running under virtualization.  The the time_after call
> reports the timeout expired.  But the last time it the bit was checked,
> before the long delay, was well before the timeout expired.  The way to
> avoid this is to always be sure to check the condition once after the
> timeout expired.  This is sure to give the full timeout.

The CQSPI timeout is 500 mS, which is pretty generous considering the
completion will happen in the first 10 mS tops. Do you think this is
of concern here and it's worth complicating the code further ?

>> +	}
>> +}
>> +
> 
>> +
>> +static unsigned int calculate_ticks_for_ns(const unsigned int ref_clk_hz,
>> +					   const unsigned int ns_val)
>> +{
>> +	unsigned int ticks;
>> +
>> +	ticks = ref_clk_hz / 1000;	/* kHz */
> This division doesn't round up.
> 
>> +	ticks = DIV_ROUND_UP(ticks * ns_val, 1000000);
> But this one does.
> 
>> +
>> +	return ticks;
>> +}
>> +
>> +static void cqspi_delay(struct spi_nor *nor, const unsigned int sclk_hz)
>> +{
>> +	struct cqspi_flash_pdata *f_pdata = nor->priv;
>> +	struct cqspi_st *cqspi = f_pdata->cqspi;
> 
> Isn't the sclk_hz parameter of this function already available here as
> cqspi->sclk?

It is , good catch, thanks.

>> +	void __iomem *iobase = cqspi->iobase;
>> +	const unsigned int ref_clk_hz = cqspi->master_ref_clk_hz;
>> +	unsigned int tshsl, tchsh, tslch, tsd2d;
>> +	unsigned int reg;
>> +	unsigned int tsclk;
>> +
>> +	/* calculate the number of ref ticks for one sclk tick */
>> +	tsclk = (ref_clk_hz + sclk_hz - 1) / sclk_hz;
> 
> DIV_ROUND_UP(ref_clk_hz, sclk_hz);

OK

>> +
>> +	tshsl = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tshsl_ns);
>> +	/* this particular value must be at least one sclk */
>> +	if (tshsl < tsclk)
>> +		tshsl = tsclk;
>> +
>> +	tchsh = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tchsh_ns);
>> +	tslch = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tslch_ns);
>> +	tsd2d = calculate_ticks_for_ns(ref_clk_hz, f_pdata->tsd2d_ns);
>> +
>> +	reg = (tshsl & CQSPI_REG_DELAY_TSHSL_MASK)
>> +	       << CQSPI_REG_DELAY_TSHSL_LSB;
>> +	reg |= (tchsh & CQSPI_REG_DELAY_TCHSH_MASK)
>> +		<< CQSPI_REG_DELAY_TCHSH_LSB;
>> +	reg |= (tslch & CQSPI_REG_DELAY_TSLCH_MASK)
>> +		<< CQSPI_REG_DELAY_TSLCH_LSB;
>> +	reg |= (tsd2d & CQSPI_REG_DELAY_TSD2D_MASK)
>> +		<< CQSPI_REG_DELAY_TSD2D_LSB;
>> +	writel(reg, iobase + CQSPI_REG_DELAY);
>> +}
>> +
>> +static void cqspi_config_baudrate_div(struct cqspi_st *cqspi,
>> +				      const unsigned int sclk_hz)
>> +{
> 
> sclk_hz is available as cqspi->sclk.

OK

>> +	const unsigned int ref_clk_hz = cqspi->master_ref_clk_hz;
>> +	void __iomem *reg_base = cqspi->iobase;
>> +	unsigned int reg;
>> +	unsigned int div;
>> +
>> +	reg = readl(reg_base + CQSPI_REG_CONFIG);
>> +	reg &= ~(CQSPI_REG_CONFIG_BAUD_MASK << CQSPI_REG_CONFIG_BAUD_LSB);
>> +
>> +	div = ref_clk_hz / sclk_hz;
> 
> Should this round up too?
> 
>> +
>> +	/* Recalculate the baudrate divisor based on QSPI specification. */
>> +	if (div > 32)
>> +		div = 32;
>> +
>> +	/* Check if even number. */
>> +	if (div & 1)
>> +		div = (div / 2);
>> +	else
>> +		div = (div / 2) - 1;
> 
> Wouldn't this be the same as div = DIV_ROUND_UP(div, 2) - 1;
> 
> The entire div calculation could be done with:
> 
>    /* Register programmed with divider minus 1 */
>    div = DIV_ROUND_UP(ref_clk_hz, s_clk_hz * 2) - 1;

Good point, thanks.

>    if (div > 15)
>         div = 15;

You can even drop this, CQSPI_REG_CONFIG_BAUD_MASK is 0xf.

> 
>> +
>> +	div = (div & CQSPI_REG_CONFIG_BAUD_MASK) << CQSPI_REG_CONFIG_BAUD_LSB;
>> +	reg |= div;
>> +	writel(reg, reg_base + CQSPI_REG_CONFIG);
>> +}
>> +
> 

I pushed V12 btw:

http://git.kernel.org/cgit/linux/kernel/git/marex/linux-2.6.git/log/?h=korg/next/cadence-qspi-v12


-- 
Best regards,
Marek Vasut

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Dinh Nguyen]

Hi Marek,

On 01/10/2016 10:34 PM, Marek Vasut wrote:
> From: Graham Moore <grmoore@opensource.altera.com>
> 
> Add binding document for the Cadence QSPI controller.
> 
> Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alan Tull <atull@opensource.altera.com>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Graham Moore <grmoore@opensource.altera.com>
> Cc: "R, Vignesh" <vigneshr@ti.com>
> Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
> Cc: devicetree@vger.kernel.org
> ---

I think you also need to CC the following people for proper review of
these bindings:

Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED
DEVICE TREE BINDINGS)
Pawel Moll <pawel.moll@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED
DEVICE TREE BINDINGS)
Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND
FLATTENED DEVICE TREE BINDINGS)
Ian Campbell <ijc+devicetree@hellion.org.uk> (maintainer:OPEN FIRMWARE
AND FLATTENED DEVICE TREE BINDINGS)
Kumar Gala <galak@codeaurora.org> (maintainer:OPEN FIRMWARE AND
FLATTENED DEVICE TREE BINDINGS)

Dinh

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Marek Vasut]

On Monday, January 11, 2016 at 05:06:30 PM, Dinh Nguyen wrote:
> Hi Marek,

Hi!

> On 01/10/2016 10:34 PM, Marek Vasut wrote:
> > From: Graham Moore <grmoore@opensource.altera.com>
> > 
> > Add binding document for the Cadence QSPI controller.
> > 
> > Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Alan Tull <atull@opensource.altera.com>
> > Cc: Brian Norris <computersforpeace@gmail.com>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Graham Moore <grmoore@opensource.altera.com>
> > Cc: "R, Vignesh" <vigneshr@ti.com>
> > Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
> > Cc: devicetree@vger.kernel.org
> > ---
> 
> I think you also need to CC the following people for proper review of
> these bindings:
> 
> Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED
> DEVICE TREE BINDINGS)
> Pawel Moll <pawel.moll@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED
> DEVICE TREE BINDINGS)
> Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND
> FLATTENED DEVICE TREE BINDINGS)
> Ian Campbell <ijc+devicetree@hellion.org.uk> (maintainer:OPEN FIRMWARE
> AND FLATTENED DEVICE TREE BINDINGS)
> Kumar Gala <galak@codeaurora.org> (maintainer:OPEN FIRMWARE AND
> FLATTENED DEVICE TREE BINDINGS)

Isn't it enough to submit them to devicetree@vger ?

Best regards,
Marek Vasut

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Dinh Nguyen]

On 01/11/2016 10:32 AM, Marek Vasut wrote:
> On Monday, January 11, 2016 at 05:06:30 PM, Dinh Nguyen wrote:
>> Hi Marek,
> 
> Hi!
> 
>> On 01/10/2016 10:34 PM, Marek Vasut wrote:
>>> From: Graham Moore <grmoore@opensource.altera.com>
>>>
>>> Add binding document for the Cadence QSPI controller.
>>>
>>> Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Alan Tull <atull@opensource.altera.com>
>>> Cc: Brian Norris <computersforpeace@gmail.com>
>>> Cc: David Woodhouse <dwmw2@infradead.org>
>>> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
>>> Cc: Graham Moore <grmoore@opensource.altera.com>
>>> Cc: "R, Vignesh" <vigneshr@ti.com>
>>> Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
>>> Cc: devicetree@vger.kernel.org
>>> ---
>>
>> I think you also need to CC the following people for proper review of
>> these bindings:
>>
>> Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED
>> DEVICE TREE BINDINGS)
>> Pawel Moll <pawel.moll@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED
>> DEVICE TREE BINDINGS)
>> Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND
>> FLATTENED DEVICE TREE BINDINGS)
>> Ian Campbell <ijc+devicetree@hellion.org.uk> (maintainer:OPEN FIRMWARE
>> AND FLATTENED DEVICE TREE BINDINGS)
>> Kumar Gala <galak@codeaurora.org> (maintainer:OPEN FIRMWARE AND
>> FLATTENED DEVICE TREE BINDINGS)
> 
> Isn't it enough to submit them to devicetree@vger ?
> 

Not sure, but I always CC those names explicitly for DTS bindings since
they are listed in the MAINTAINERS file.

Dinh

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Marek Vasut]

On Monday, January 11, 2016 at 06:03:53 PM, Dinh Nguyen wrote:
> On 01/11/2016 10:32 AM, Marek Vasut wrote:
> > On Monday, January 11, 2016 at 05:06:30 PM, Dinh Nguyen wrote:
> >> Hi Marek,
> > 
> > Hi!
> > 
> >> On 01/10/2016 10:34 PM, Marek Vasut wrote:
> >>> From: Graham Moore <grmoore@opensource.altera.com>
> >>> 
> >>> Add binding document for the Cadence QSPI controller.
> >>> 
> >>> Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
> >>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>> Cc: Alan Tull <atull@opensource.altera.com>
> >>> Cc: Brian Norris <computersforpeace@gmail.com>
> >>> Cc: David Woodhouse <dwmw2@infradead.org>
> >>> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> >>> Cc: Graham Moore <grmoore@opensource.altera.com>
> >>> Cc: "R, Vignesh" <vigneshr@ti.com>
> >>> Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
> >>> Cc: devicetree@vger.kernel.org
> >>> ---
> >> 
> >> I think you also need to CC the following people for proper review of
> >> these bindings:
> >> 
> >> Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED
> >> DEVICE TREE BINDINGS)
> >> Pawel Moll <pawel.moll@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED
> >> DEVICE TREE BINDINGS)
> >> Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND
> >> FLATTENED DEVICE TREE BINDINGS)
> >> Ian Campbell <ijc+devicetree@hellion.org.uk> (maintainer:OPEN FIRMWARE
> >> AND FLATTENED DEVICE TREE BINDINGS)
> >> Kumar Gala <galak@codeaurora.org> (maintainer:OPEN FIRMWARE AND
> >> FLATTENED DEVICE TREE BINDINGS)
> > 
> > Isn't it enough to submit them to devicetree@vger ?
> 
> Not sure, but I always CC those names explicitly for DTS bindings since
> they are listed in the MAINTAINERS file.

OK, will do in the next revision (if needed). Thanks!

Best regards,
Marek Vasut

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Rob Herring]

On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote:
> From: Graham Moore <grmoore@opensource.altera.com>
> 
> Add binding document for the Cadence QSPI controller.
> 
> Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alan Tull <atull@opensource.altera.com>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Graham Moore <grmoore@opensource.altera.com>
> Cc: "R, Vignesh" <vigneshr@ti.com>
> Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
> Cc: devicetree@vger.kernel.org
> ---
>  .../devicetree/bindings/mtd/cadence-quadspi.txt    | 56 ++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> 
> V2: Add cdns prefix to driver-specific bindings.
> V3: Use existing property "is-decoded-cs" instead of creating a
>     duplicate, "ext-decoder". Timing parameters are in nanoseconds,
>     not master reference clocks. Remove bus-num completely.
> V4: Add new properties fifo-width and trigger-address
> V7: - Prefix all of the Cadence-specific properties with cdns prefix,
>       those are in particular "cdns,is-decoded-cs", "cdns,fifo-depth",
>       "cdns,fifo-width", "cdns,trigger-address".
>     - Drop bogus properties which were not used and were incorrect.
> V8: Align lines to 80 chars.
> 
> diff --git a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> new file mode 100644
> index 0000000..f248056
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> @@ -0,0 +1,56 @@
> +* Cadence Quad SPI controller
> +
> +Required properties:
> +- compatible : Should be "cdns,qspi-nor".

Fine, but I expect to see SOCs using this block add their own compatible 
strings. It wouldn't surprise me that we already have some using this 
block.

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Marek Vasut]

On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote:
> On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote:
> > From: Graham Moore <grmoore@opensource.altera.com>
> > 
> > Add binding document for the Cadence QSPI controller.
> > 
> > Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Alan Tull <atull@opensource.altera.com>
> > Cc: Brian Norris <computersforpeace@gmail.com>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Graham Moore <grmoore@opensource.altera.com>
> > Cc: "R, Vignesh" <vigneshr@ti.com>
> > Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
> > Cc: devicetree@vger.kernel.org
> > ---
> > 
> >  .../devicetree/bindings/mtd/cadence-quadspi.txt    | 56
> >  ++++++++++++++++++++++ 1 file changed, 56 insertions(+)
> >  create mode 100644
> >  Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> > 
> > V2: Add cdns prefix to driver-specific bindings.
> > V3: Use existing property "is-decoded-cs" instead of creating a
> > 
> >     duplicate, "ext-decoder". Timing parameters are in nanoseconds,
> >     not master reference clocks. Remove bus-num completely.
> > 
> > V4: Add new properties fifo-width and trigger-address
> > V7: - Prefix all of the Cadence-specific properties with cdns prefix,
> > 
> >       those are in particular "cdns,is-decoded-cs", "cdns,fifo-depth",
> >       "cdns,fifo-width", "cdns,trigger-address".
> >     
> >     - Drop bogus properties which were not used and were incorrect.
> > 
> > V8: Align lines to 80 chars.
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> > b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt new file
> > mode 100644
> > index 0000000..f248056
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> > @@ -0,0 +1,56 @@
> > +* Cadence Quad SPI controller
> > +
> > +Required properties:
> > +- compatible : Should be "cdns,qspi-nor".
> 
> Fine, but I expect to see SOCs using this block add their own compatible
> strings. It wouldn't surprise me that we already have some using this
> block.
> 
> Acked-by: Rob Herring <robh@kernel.org>

I finally got an Ack on this, I am so happy :-)

As for the SoCs, there is Altera SoCFPGA Gen 5 and Gen 10 which uses this.
Then there is some TI SoC, but I don't know the model. Vignesh (on CC) would.
Then there is some ST SoC, but I have no idea what that's all about, sorry.

All these SoCs should be capable of tweaking the block to fit their needs
by just the DT properties. I believe they differ only in the FIFO depth and
sometimes someone is greedy and uses 4:16 CS multiplexer, which is an external
passive component, but that's all.

Would we need soc-specific compatible strings if this is the case?

Best regards,
Marek Vasut

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Brian Norris]

On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote:
> On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote:
> > On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote:
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> > > @@ -0,0 +1,56 @@
> > > +* Cadence Quad SPI controller
> > > +
> > > +Required properties:
> > > +- compatible : Should be "cdns,qspi-nor".
> > 
> > Fine, but I expect to see SOCs using this block add their own compatible
> > strings. It wouldn't surprise me that we already have some using this
> > block.
> > 
> > Acked-by: Rob Herring <robh@kernel.org>
> 
> I finally got an Ack on this, I am so happy :-)
> 
> As for the SoCs, there is Altera SoCFPGA Gen 5 and Gen 10 which uses this.
> Then there is some TI SoC, but I don't know the model. Vignesh (on CC) would.
> Then there is some ST SoC, but I have no idea what that's all about, sorry.
> 
> All these SoCs should be capable of tweaking the block to fit their needs
> by just the DT properties. I believe they differ only in the FIFO depth and
> sometimes someone is greedy and uses 4:16 CS multiplexer, which is an external
> passive component, but that's all.
> 
> Would we need soc-specific compatible strings if this is the case?

It's nice when most things can be supported with a small set of DT
properties, as you've done. But IUIC, I think it's usually good practice
to define and use SoC-specific (or maybe SoC family) compatible strings
in the docs and DTS files, in addition to the generic one, in case there
are future quirks that need to be handled. Note that you don't actually
have to use these in the driver yet, but it's good to have a definition.
So you can, today, have:

	foo@xxxx {
		compatible = "ti,baz-12345", "cdns,qspi-nor";
		...
	};

And we have the option to pick up "ti,baz-12345" in the Linux driver *if
needed.*

Brian

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Marek Vasut]

On Monday, February 01, 2016 at 10:03:35 PM, Brian Norris wrote:
> On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote:
> > On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote:
> > > On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote:
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> > > > @@ -0,0 +1,56 @@
> > > > +* Cadence Quad SPI controller
> > > > +
> > > > +Required properties:
> > > > +- compatible : Should be "cdns,qspi-nor".
> > > 
> > > Fine, but I expect to see SOCs using this block add their own
> > > compatible strings. It wouldn't surprise me that we already have some
> > > using this block.
> > > 
> > > Acked-by: Rob Herring <robh@kernel.org>
> > 
> > I finally got an Ack on this, I am so happy :-)
> > 
> > As for the SoCs, there is Altera SoCFPGA Gen 5 and Gen 10 which uses
> > this. Then there is some TI SoC, but I don't know the model. Vignesh (on
> > CC) would. Then there is some ST SoC, but I have no idea what that's all
> > about, sorry.
> > 
> > All these SoCs should be capable of tweaking the block to fit their needs
> > by just the DT properties. I believe they differ only in the FIFO depth
> > and sometimes someone is greedy and uses 4:16 CS multiplexer, which is
> > an external passive component, but that's all.
> > 
> > Would we need soc-specific compatible strings if this is the case?
> 
> It's nice when most things can be supported with a small set of DT
> properties, as you've done. But IUIC, I think it's usually good practice
> to define and use SoC-specific (or maybe SoC family) compatible strings
> in the docs and DTS files, in addition to the generic one, in case there
> are future quirks that need to be handled. Note that you don't actually
> have to use these in the driver yet, but it's good to have a definition.
> So you can, today, have:
> 
> 	foo@xxxx {
> 		compatible = "ti,baz-12345", "cdns,qspi-nor";
> 		...
> 	};
> 
> And we have the option to pick up "ti,baz-12345" in the Linux driver *if
> needed.*

Ah, got it, thanks!

Best regards,
Marek Vasut

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Vignesh R]

On 02/02/2016 02:43 AM, Marek Vasut wrote:
> On Monday, February 01, 2016 at 10:03:35 PM, Brian Norris wrote:
>> On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote:
>>> On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote:
>>>> On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote:

[...]

>>>
>>> All these SoCs should be capable of tweaking the block to fit their needs
>>> by just the DT properties. I believe they differ only in the FIFO depth
>>> and sometimes someone is greedy and uses 4:16 CS multiplexer, which is
>>> an external passive component, but that's all.
>>>
>>> Would we need soc-specific compatible strings if this is the case?
>>
>> It's nice when most things can be supported with a small set of DT
>> properties, as you've done. But IUIC, I think it's usually good practice
>> to define and use SoC-specific (or maybe SoC family) compatible strings
>> in the docs and DTS files, in addition to the generic one, in case there
>> are future quirks that need to be handled. Note that you don't actually
>> have to use these in the driver yet, but it's good to have a definition.
>> So you can, today, have:
>>
>> 	foo@xxxx {
>> 		compatible = "ti,baz-12345", "cdns,qspi-nor";
>> 		...
>> 	};
>>
>> And we have the option to pick up "ti,baz-12345" in the Linux driver *if
>> needed.*

The support for TI SoC that has this IP is not in upstream yet. I will
add TI-specific compatible later. It will be:

	foo@xxxx {
		compatible = "ti,k2g-qspi", "cdns,qspi-nor";
 		...
 	};

-- 
Regards
Vignesh

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Marek Vasut]

On Thursday, February 04, 2016 at 08:38:47 AM, Vignesh R wrote:
> On 02/02/2016 02:43 AM, Marek Vasut wrote:
> > On Monday, February 01, 2016 at 10:03:35 PM, Brian Norris wrote:
> >> On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote:
> >>> On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote:
> >>>> On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote:
> [...]
> 
> >>> All these SoCs should be capable of tweaking the block to fit their
> >>> needs by just the DT properties. I believe they differ only in the
> >>> FIFO depth and sometimes someone is greedy and uses 4:16 CS
> >>> multiplexer, which is an external passive component, but that's all.
> >>> 
> >>> Would we need soc-specific compatible strings if this is the case?
> >> 
> >> It's nice when most things can be supported with a small set of DT
> >> properties, as you've done. But IUIC, I think it's usually good practice
> >> to define and use SoC-specific (or maybe SoC family) compatible strings
> >> in the docs and DTS files, in addition to the generic one, in case there
> >> are future quirks that need to be handled. Note that you don't actually
> >> have to use these in the driver yet, but it's good to have a definition.
> >> 
> >> So you can, today, have:
> >> 	foo@xxxx {
> >> 	
> >> 		compatible = "ti,baz-12345", "cdns,qspi-nor";
> >> 		...
> >> 	
> >> 	};
> >> 
> >> And we have the option to pick up "ti,baz-12345" in the Linux driver *if
> >> needed.*
> 
> The support for TI SoC that has this IP is not in upstream yet. I will
> add TI-specific compatible later. It will be:
> 
> 	foo@xxxx {
> 		compatible = "ti,k2g-qspi", "cdns,qspi-nor";
>  		...
>  	};

Do you expect any specifics which cannot be handled by the current bindings btw?
In my socfpga case, the compatible strings will be probably:

"altr,socfpga-gen5-qspi"

Dinh, Graham, do you agree with this or should we use something else ?

Thanks!

Best regards,
Marek Vasut

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Dinh Nguyen]

On 02/04/2016 05:25 AM, Marek Vasut wrote:
> On Thursday, February 04, 2016 at 08:38:47 AM, Vignesh R wrote:
>> On 02/02/2016 02:43 AM, Marek Vasut wrote:
>>> On Monday, February 01, 2016 at 10:03:35 PM, Brian Norris wrote:
>>>> On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote:
>>>>> On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote:
>>>>>> On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote:
>> [...]
>>
>>>>> All these SoCs should be capable of tweaking the block to fit their
>>>>> needs by just the DT properties. I believe they differ only in the
>>>>> FIFO depth and sometimes someone is greedy and uses 4:16 CS
>>>>> multiplexer, which is an external passive component, but that's all.
>>>>>
>>>>> Would we need soc-specific compatible strings if this is the case?
>>>>
>>>> It's nice when most things can be supported with a small set of DT
>>>> properties, as you've done. But IUIC, I think it's usually good practice
>>>> to define and use SoC-specific (or maybe SoC family) compatible strings
>>>> in the docs and DTS files, in addition to the generic one, in case there
>>>> are future quirks that need to be handled. Note that you don't actually
>>>> have to use these in the driver yet, but it's good to have a definition.
>>>>
>>>> So you can, today, have:
>>>> 	foo@xxxx {
>>>> 	
>>>> 		compatible = "ti,baz-12345", "cdns,qspi-nor";
>>>> 		...
>>>> 	
>>>> 	};
>>>>
>>>> And we have the option to pick up "ti,baz-12345" in the Linux driver *if
>>>> needed.*
>>
>> The support for TI SoC that has this IP is not in upstream yet. I will
>> add TI-specific compatible later. It will be:
>>
>> 	foo@xxxx {
>> 		compatible = "ti,k2g-qspi", "cdns,qspi-nor";
>>  		...
>>  	};
> 
> Do you expect any specifics which cannot be handled by the current bindings btw?
> In my socfpga case, the compatible strings will be probably:
> 
> "altr,socfpga-gen5-qspi"
> 
> Dinh, Graham, do you agree with this or should we use something else ?
> 

I agree.

Dinh

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Marek Vasut]

On Thursday, February 04, 2016 at 06:04:08 PM, Dinh Nguyen wrote:
> On 02/04/2016 05:25 AM, Marek Vasut wrote:
> > On Thursday, February 04, 2016 at 08:38:47 AM, Vignesh R wrote:
> >> On 02/02/2016 02:43 AM, Marek Vasut wrote:
> >>> On Monday, February 01, 2016 at 10:03:35 PM, Brian Norris wrote:
> >>>> On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote:
> >>>>> On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote:
> >>>>>> On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote:
> >> [...]
> >> 
> >>>>> All these SoCs should be capable of tweaking the block to fit their
> >>>>> needs by just the DT properties. I believe they differ only in the
> >>>>> FIFO depth and sometimes someone is greedy and uses 4:16 CS
> >>>>> multiplexer, which is an external passive component, but that's all.
> >>>>> 
> >>>>> Would we need soc-specific compatible strings if this is the case?
> >>>> 
> >>>> It's nice when most things can be supported with a small set of DT
> >>>> properties, as you've done. But IUIC, I think it's usually good
> >>>> practice to define and use SoC-specific (or maybe SoC family)
> >>>> compatible strings in the docs and DTS files, in addition to the
> >>>> generic one, in case there are future quirks that need to be handled.
> >>>> Note that you don't actually have to use these in the driver yet, but
> >>>> it's good to have a definition.
> >>>> 
> >>>> So you can, today, have:
> >>>> 	foo@xxxx {
> >>>> 	
> >>>> 		compatible = "ti,baz-12345", "cdns,qspi-nor";
> >>>> 		...
> >>>> 	
> >>>> 	};
> >>>> 
> >>>> And we have the option to pick up "ti,baz-12345" in the Linux driver
> >>>> *if needed.*
> >> 
> >> The support for TI SoC that has this IP is not in upstream yet. I will
> >> 
> >> add TI-specific compatible later. It will be:
> >> 	foo@xxxx {
> >> 	
> >> 		compatible = "ti,k2g-qspi", "cdns,qspi-nor";
> >> 		
> >>  		...
> >>  	
> >>  	};
> > 
> > Do you expect any specifics which cannot be handled by the current
> > bindings btw? In my socfpga case, the compatible strings will be
> > probably:
> > 
> > "altr,socfpga-gen5-qspi"
> > 
> > Dinh, Graham, do you agree with this or should we use something else ?
> 
> I agree.

Thanks

Best regards,
Marek Vasut

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[R, Vignesh]

On 2/4/2016 4:55 PM, Marek Vasut wrote:
> On Thursday, February 04, 2016 at 08:38:47 AM, Vignesh R wrote:
>> On 02/02/2016 02:43 AM, Marek Vasut wrote:
>>> On Monday, February 01, 2016 at 10:03:35 PM, Brian Norris wrote:
>>>> On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote:
>>>>> On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote:
>>>>>> On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote:
>> [...]
>>
>>>>> All these SoCs should be capable of tweaking the block to fit their
>>>>> needs by just the DT properties. I believe they differ only in the
>>>>> FIFO depth and sometimes someone is greedy and uses 4:16 CS
>>>>> multiplexer, which is an external passive component, but that's all.
>>>>>
>>>>> Would we need soc-specific compatible strings if this is the case?
>>>>
>>>> It's nice when most things can be supported with a small set of DT
>>>> properties, as you've done. But IUIC, I think it's usually good practice
>>>> to define and use SoC-specific (or maybe SoC family) compatible strings
>>>> in the docs and DTS files, in addition to the generic one, in case there
>>>> are future quirks that need to be handled. Note that you don't actually
>>>> have to use these in the driver yet, but it's good to have a definition.
>>>>
>>>> So you can, today, have:
>>>> 	foo@xxxx {
>>>> 	
>>>> 		compatible = "ti,baz-12345", "cdns,qspi-nor";
>>>> 		...
>>>> 	
>>>> 	};
>>>>
>>>> And we have the option to pick up "ti,baz-12345" in the Linux driver *if
>>>> needed.*
>>
>> The support for TI SoC that has this IP is not in upstream yet. I will
>> add TI-specific compatible later. It will be:
>>
>> 	foo@xxxx {
>> 		compatible = "ti,k2g-qspi", "cdns,qspi-nor";
>>  		...
>>  	};
> 
> Do you expect any specifics which cannot be handled by the current bindings btw?
> In my socfpga case, the compatible strings will be probably:
> 

Yeah, there is delay(of few ns) required between writing to
INDIRECTWR_START bit and actually writing data to flash(i.e writesl()
call). This is specific to TI K2G SoC and needs to be tied to the new
binding.

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Marek Vasut]

On Thursday, February 04, 2016 at 06:30:27 PM, R, Vignesh wrote:
> On 2/4/2016 4:55 PM, Marek Vasut wrote:
> > On Thursday, February 04, 2016 at 08:38:47 AM, Vignesh R wrote:
> >> On 02/02/2016 02:43 AM, Marek Vasut wrote:
> >>> On Monday, February 01, 2016 at 10:03:35 PM, Brian Norris wrote:
> >>>> On Wed, Jan 13, 2016 at 03:39:17AM +0100, Marek Vasut wrote:
> >>>>> On Wednesday, January 13, 2016 at 03:26:08 AM, Rob Herring wrote:
> >>>>>> On Mon, Jan 11, 2016 at 05:34:45AM +0100, Marek Vasut wrote:
> >> [...]
> >> 
> >>>>> All these SoCs should be capable of tweaking the block to fit their
> >>>>> needs by just the DT properties. I believe they differ only in the
> >>>>> FIFO depth and sometimes someone is greedy and uses 4:16 CS
> >>>>> multiplexer, which is an external passive component, but that's all.
> >>>>> 
> >>>>> Would we need soc-specific compatible strings if this is the case?
> >>>> 
> >>>> It's nice when most things can be supported with a small set of DT
> >>>> properties, as you've done. But IUIC, I think it's usually good
> >>>> practice to define and use SoC-specific (or maybe SoC family)
> >>>> compatible strings in the docs and DTS files, in addition to the
> >>>> generic one, in case there are future quirks that need to be handled.
> >>>> Note that you don't actually have to use these in the driver yet, but
> >>>> it's good to have a definition.
> >>>> 
> >>>> So you can, today, have:
> >>>> 	foo@xxxx {
> >>>> 	
> >>>> 		compatible = "ti,baz-12345", "cdns,qspi-nor";
> >>>> 		...
> >>>> 	
> >>>> 	};
> >>>> 
> >>>> And we have the option to pick up "ti,baz-12345" in the Linux driver
> >>>> *if needed.*
> >> 
> >> The support for TI SoC that has this IP is not in upstream yet. I will
> >> 
> >> add TI-specific compatible later. It will be:
> >> 	foo@xxxx {
> >> 	
> >> 		compatible = "ti,k2g-qspi", "cdns,qspi-nor";
> >> 		
> >>  		...
> >>  	
> >>  	};
> > 
> > Do you expect any specifics which cannot be handled by the current
> > bindings btw?
> 
> > In my socfpga case, the compatible strings will be probably:
> Yeah, there is delay(of few ns) required between writing to
> INDIRECTWR_START bit and actually writing data to flash(i.e writesl()
> call). This is specific to TI K2G SoC and needs to be tied to the new
> binding.

Can't you somehow poll the hardware to check whether or not it's ready instead
of adding some random delay ?

Best regards,
Marek Vasut

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Vignesh R]

On 02/06/2016 01:12 PM, Marek Vasut wrote:
> On Thursday, February 04, 2016 at 06:30:27 PM, R, Vignesh wrote:
>> On 2/4/2016 4:55 PM, Marek Vasut wrote:

[...]

>> Yeah, there is delay(of few ns) required between writing to
>> INDIRECTWR_START bit and actually writing data to flash(i.e writesl()
>> call). This is specific to TI K2G SoC and needs to be tied to the new
>> binding.
> 
> Can't you somehow poll the hardware to check whether or not it's ready instead
> of adding some random delay ?

There is no dedicated register to poll as such.

According to TRM:
"Wait for couple of cycles of QSPI_REF_CLK(functional clock of QSPI
@384MHz) until CQSPI_REG_INDIRECTWR[0] bit is internally synchronized by
the QSPI module before writing to flash".

So, a few ns(~6ns @384MHz) delay is needed (or accessing a QSPI module
register should be sufficient as it will take more than 2 clock cycles).
I believe this delay is specific to TI K2G SoC and maybe needs to be
tied to the binding.


-- 
Regards
Vignesh

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Marek Vasut]

On Monday, February 08, 2016 at 12:19:25 PM, Vignesh R wrote:
> On 02/06/2016 01:12 PM, Marek Vasut wrote:
> > On Thursday, February 04, 2016 at 06:30:27 PM, R, Vignesh wrote:
> >> On 2/4/2016 4:55 PM, Marek Vasut wrote:
> [...]
> 
> >> Yeah, there is delay(of few ns) required between writing to
> >> INDIRECTWR_START bit and actually writing data to flash(i.e writesl()
> >> call). This is specific to TI K2G SoC and needs to be tied to the new
> >> binding.
> > 
> > Can't you somehow poll the hardware to check whether or not it's ready
> > instead of adding some random delay ?
> 
> There is no dedicated register to poll as such.
> 
> According to TRM:
> "Wait for couple of cycles of QSPI_REF_CLK(functional clock of QSPI
> @384MHz) until CQSPI_REG_INDIRECTWR[0] bit is internally synchronized by
> the QSPI module before writing to flash".
> 
> So, a few ns(~6ns @384MHz) delay is needed (or accessing a QSPI module
> register should be sufficient as it will take more than 2 clock cycles).
> I believe this delay is specific to TI K2G SoC and maybe needs to be
> tied to the binding.

OK, got it. Dinh/Graham, can you check if this might be needed on SoCFPGA too 
please?

Best regards,
Marek Vasut

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Graham Moore]

On 02/08/2016 09:27 AM, Marek Vasut wrote:
> On Monday, February 08, 2016 at 12:19:25 PM, Vignesh R wrote:
>> On 02/06/2016 01:12 PM, Marek Vasut wrote:
>>> On Thursday, February 04, 2016 at 06:30:27 PM, R, Vignesh wrote:
>>>> On 2/4/2016 4:55 PM, Marek Vasut wrote:
>> [...]
>>
>>>> Yeah, there is delay(of few ns) required between writing to
>>>> INDIRECTWR_START bit and actually writing data to flash(i.e writesl()
>>>> call). This is specific to TI K2G SoC and needs to be tied to the new
>>>> binding.
>>>
>>> Can't you somehow poll the hardware to check whether or not it's ready
>>> instead of adding some random delay ?
>>
>> There is no dedicated register to poll as such.
>>
>> According to TRM:
>> "Wait for couple of cycles of QSPI_REF_CLK(functional clock of QSPI
>> @384MHz) until CQSPI_REG_INDIRECTWR[0] bit is internally synchronized by
>> the QSPI module before writing to flash".
>>
>> So, a few ns(~6ns @384MHz) delay is needed (or accessing a QSPI module
>> register should be sufficient as it will take more than 2 clock cycles).
>> I believe this delay is specific to TI K2G SoC and maybe needs to be
>> tied to the binding.
>
> OK, got it. Dinh/Graham, can you check if this might be needed on SoCFPGA too
> please?
>

I don't see any such requirement in the data sheet.  It's working 
without it.  So I think it's not needed on SoCFPGA

-Graham

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Marek Vasut]

On 02/10/2016 05:10 PM, Graham Moore wrote:
> On 02/08/2016 09:27 AM, Marek Vasut wrote:
>> On Monday, February 08, 2016 at 12:19:25 PM, Vignesh R wrote:
>>> On 02/06/2016 01:12 PM, Marek Vasut wrote:
>>>> On Thursday, February 04, 2016 at 06:30:27 PM, R, Vignesh wrote:
>>>>> On 2/4/2016 4:55 PM, Marek Vasut wrote:
>>> [...]
>>>
>>>>> Yeah, there is delay(of few ns) required between writing to
>>>>> INDIRECTWR_START bit and actually writing data to flash(i.e writesl()
>>>>> call). This is specific to TI K2G SoC and needs to be tied to the new
>>>>> binding.
>>>>
>>>> Can't you somehow poll the hardware to check whether or not it's ready
>>>> instead of adding some random delay ?
>>>
>>> There is no dedicated register to poll as such.
>>>
>>> According to TRM:
>>> "Wait for couple of cycles of QSPI_REF_CLK(functional clock of QSPI
>>> @384MHz) until CQSPI_REG_INDIRECTWR[0] bit is internally synchronized by
>>> the QSPI module before writing to flash".
>>>
>>> So, a few ns(~6ns @384MHz) delay is needed (or accessing a QSPI module
>>> register should be sufficient as it will take more than 2 clock cycles).
>>> I believe this delay is specific to TI K2G SoC and maybe needs to be
>>> tied to the binding.
>>
>> OK, got it. Dinh/Graham, can you check if this might be needed on
>> SoCFPGA too
>> please?
>>
> 
> I don't see any such requirement in the data sheet.  It's working
> without it.  So I think it's not needed on SoCFPGA

All right, so we will just add a special property or compat string for
the TI SoC. But we need to get this driver mainlined first :)

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Graham Moore]

On 02/10/2016 10:17 AM, Marek Vasut wrote:

[...]

> All right, so we will just add a special property or compat string for
> the TI SoC. But we need to get this driver mainlined first :)
>

Hi Marek,

How's that mainlining going?  You probably noticed this patch needed 
some refactoring for 4.4.  In the course of testing, we realized this 
driver needs to enable its clock.

Do you have a 4.4 or later version in your tree?  I'd like to add the 
clock enabling.

-Graham

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Marek Vasut]

On 03/10/2016 09:55 PM, Graham Moore wrote:
> On 02/10/2016 10:17 AM, Marek Vasut wrote:
> 
> [...]
> 
>> All right, so we will just add a special property or compat string for
>> the TI SoC. But we need to get this driver mainlined first :)
>>
> 
> Hi Marek,

Hi Graham,

> How's that mainlining going?  You probably noticed this patch needed
> some refactoring for 4.4.  In the course of testing, we realized this
> driver needs to enable its clock.
> 
> Do you have a 4.4 or later version in your tree?  I'd like to add the
> clock enabling.

Still waiting for the patches from Cyrille to go in, so this patch is
stuck. I just checked next and some of them made it, but there are more
which didn't. I have a V10 of this patch , it is in the ML already [1],
so try playing around with that.

I have linux 4.4 branch with all the necessary patches backported and/or
applied if you'd be interested in that, but I do my development on
linux-next .

What sort of clock patch are you missing ?

[1] https://patchwork.ozlabs.org/patch/565599/

> -Graham

-- 
Best regards,
Marek Vasut

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Graham Moore]

On 03/10/2016 03:10 PM, Marek Vasut wrote:
> On 03/10/2016 09:55 PM, Graham Moore wrote:
>> On 02/10/2016 10:17 AM, Marek Vasut wrote:
>>
>> [...]
>>
>>> All right, so we will just add a special property or compat string for
>>> the TI SoC. But we need to get this driver mainlined first :)
>>>
>>
>> Hi Marek,
>
> Hi Graham,
>
>> How's that mainlining going?  You probably noticed this patch needed
>> some refactoring for 4.4.  In the course of testing, we realized this
>> driver needs to enable its clock.
>>
>> Do you have a 4.4 or later version in your tree?  I'd like to add the
>> clock enabling.
>
> Still waiting for the patches from Cyrille to go in, so this patch is
> stuck. I just checked next and some of them made it, but there are more
> which didn't. I have a V10 of this patch , it is in the ML already [1],
> so try playing around with that.
>
> I have linux 4.4 branch with all the necessary patches backported and/or
> applied if you'd be interested in that, but I do my development on
> linux-next .
>
> What sort of clock patch are you missing ?
>

The clock needs to be enabled, after the devm_clk_get().  I've added a 
clk_prepare_enable() there, and also clk_disable_unprepare() in the 
remove function.

That ML patch won't apply to any tree I have, 4.4, 4.5, linux-next, etc. 
  Maybe I'll just wait until it's in linux-next.

-Graham

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Marek Vasut]

On 03/14/2016 07:17 PM, Graham Moore wrote:
> On 03/10/2016 03:10 PM, Marek Vasut wrote:
>> On 03/10/2016 09:55 PM, Graham Moore wrote:
>>> On 02/10/2016 10:17 AM, Marek Vasut wrote:
>>>
>>> [...]
>>>
>>>> All right, so we will just add a special property or compat string for
>>>> the TI SoC. But we need to get this driver mainlined first :)
>>>>
>>>
>>> Hi Marek,
>>
>> Hi Graham,
>>
>>> How's that mainlining going?  You probably noticed this patch needed
>>> some refactoring for 4.4.  In the course of testing, we realized this
>>> driver needs to enable its clock.
>>>
>>> Do you have a 4.4 or later version in your tree?  I'd like to add the
>>> clock enabling.
>>
>> Still waiting for the patches from Cyrille to go in, so this patch is
>> stuck. I just checked next and some of them made it, but there are more
>> which didn't. I have a V10 of this patch , it is in the ML already [1],
>> so try playing around with that.
>>
>> I have linux 4.4 branch with all the necessary patches backported and/or
>> applied if you'd be interested in that, but I do my development on
>> linux-next .
>>
>> What sort of clock patch are you missing ?
>>
> 
> The clock needs to be enabled, after the devm_clk_get().  I've added a
> clk_prepare_enable() there, and also clk_disable_unprepare() in the
> remove function.

Yeah, please send me a patch and I will add it into the driver, add your
SoB line and repost.

> That ML patch won't apply to any tree I have, 4.4, 4.5, linux-next, etc.
>  Maybe I'll just wait until it's in linux-next.

I'll send you the backport to 4.4 off-list, so I don't bother people here.

> -Graham
> 


-- 
Best regards,
Marek Vasut

[PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver

[Marek Vasut]

From: Graham Moore <grmoore@opensource.altera.com>

Add binding document for the Cadence QSPI controller.

Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alan Tull <atull@opensource.altera.com>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Graham Moore <grmoore@opensource.altera.com>
Cc: Vignesh R <vigneshr@ti.com>
Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
Cc: devicetree@vger.kernel.org
---
V2: Add cdns prefix to driver-specific bindings.
V3: Use existing property "is-decoded-cs" instead of creating a
    duplicate, "ext-decoder". Timing parameters are in nanoseconds,
    not master reference clocks. Remove bus-num completely.
V4: Add new properties fifo-width and trigger-address
V7: - Prefix all of the Cadence-specific properties with cdns prefix,
      those are in particular "cdns,is-decoded-cs", "cdns,fifo-depth",
      "cdns,fifo-width", "cdns,trigger-address".
    - Drop bogus properties which were not used and were incorrect.
V8: Align lines to 80 chars.
---
 .../devicetree/bindings/mtd/cadence-quadspi.txt    | 56 ++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/cadence-quadspi.txt

diff --git a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
new file mode 100644
index 0000000..f248056
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
@@ -0,0 +1,56 @@
+* Cadence Quad SPI controller
+
+Required properties:
+- compatible : Should be "cdns,qspi-nor".
+- reg : Contains two entries, each of which is a tuple consisting of a
+	physical address and length. The first entry is the address and
+	length of the controller register set. The second entry is the
+	address and length of the QSPI Controller data area.
+- interrupts : Unit interrupt specifier for the controller interrupt.
+- clocks : phandle to the Quad SPI clock.
+- cdns,fifo-depth : Size of the data FIFO in words.
+- cdns,fifo-width : Bus width of the data FIFO in bytes.
+- cdns,trigger-address : 32-bit indirect AHB trigger address.
+
+Optional properties:
+- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not.
+
+Optional subnodes:
+Subnodes of the Cadence Quad SPI controller are spi slave nodes with additional
+custom properties:
+- cdns,read-delay : Delay for read capture logic, in clock cycles
+- cdns,tshsl-ns : Delay in nanoseconds for the length that the master
+                  mode chip select outputs are de-asserted between
+		  transactions.
+- cdns,tsd2d-ns : Delay in nanoseconds between one chip select being
+                  de-activated and the activation of another.
+- cdns,tchsh-ns : Delay in nanoseconds between last bit of current
+                  transaction and deasserting the device chip select
+		  (qspi_n_ss_out).
+- cdns,tslch-ns : Delay in nanoseconds between setting qspi_n_ss_out low
+                  and first bit transfer.
+
+Example:
+
+	qspi: spi@ff705000 {
+		compatible = "cdns,qspi-nor";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0xff705000 0x1000>,
+		      <0xffa00000 0x1000>;
+		interrupts = <0 151 4>;
+		clocks = <&qspi_clk>;
+		cdns,is-decoded-cs;
+		cdns,fifo-depth = <128>;
+		cdns,fifo-width = <4>;
+		cdns,trigger-address = <0x00000000>;
+
+		flash0: n25q00@0 {
+			...
+			cdns,read-delay = <4>;
+			cdns,tshsl-ns = <50>;
+			cdns,tsd2d-ns = <50>;
+			cdns,tchsh-ns = <4>;
+			cdns,tslch-ns = <4>;
+		};
+	};
-- 
2.7.0

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver

[Rob Herring]

On Sat, Jun 04, 2016 at 02:39:33AM +0200, Marek Vasut wrote:
> From: Graham Moore <grmoore@opensource.altera.com>
> 
> Add binding document for the Cadence QSPI controller.
> 
> Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alan Tull <atull@opensource.altera.com>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Graham Moore <grmoore@opensource.altera.com>
> Cc: Vignesh R <vigneshr@ti.com>
> Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
> Cc: devicetree@vger.kernel.org
> ---
> V2: Add cdns prefix to driver-specific bindings.
> V3: Use existing property "is-decoded-cs" instead of creating a
>     duplicate, "ext-decoder". Timing parameters are in nanoseconds,
>     not master reference clocks. Remove bus-num completely.
> V4: Add new properties fifo-width and trigger-address
> V7: - Prefix all of the Cadence-specific properties with cdns prefix,
>       those are in particular "cdns,is-decoded-cs", "cdns,fifo-depth",
>       "cdns,fifo-width", "cdns,trigger-address".
>     - Drop bogus properties which were not used and were incorrect.
> V8: Align lines to 80 chars.
> ---
>  .../devicetree/bindings/mtd/cadence-quadspi.txt    | 56 ++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/cadence-quadspi.txt

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver

[Brian Norris]

On Tue, Jun 07, 2016 at 09:00:57AM -0500, Rob Herring wrote:
> On Sat, Jun 04, 2016 at 02:39:33AM +0200, Marek Vasut wrote:
> > From: Graham Moore <grmoore@opensource.altera.com>
> > 
> > Add binding document for the Cadence QSPI controller.
> > 
> > Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Alan Tull <atull@opensource.altera.com>
> > Cc: Brian Norris <computersforpeace@gmail.com>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Graham Moore <grmoore@opensource.altera.com>
> > Cc: Vignesh R <vigneshr@ti.com>
> > Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
> > Cc: devicetree@vger.kernel.org
> > ---
> > V2: Add cdns prefix to driver-specific bindings.
> > V3: Use existing property "is-decoded-cs" instead of creating a
> >     duplicate, "ext-decoder". Timing parameters are in nanoseconds,
> >     not master reference clocks. Remove bus-num completely.
> > V4: Add new properties fifo-width and trigger-address
> > V7: - Prefix all of the Cadence-specific properties with cdns prefix,
> >       those are in particular "cdns,is-decoded-cs", "cdns,fifo-depth",
> >       "cdns,fifo-width", "cdns,trigger-address".
> >     - Drop bogus properties which were not used and were incorrect.
> > V8: Align lines to 80 chars.
> > ---
> >  .../devicetree/bindings/mtd/cadence-quadspi.txt    | 56 ++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> 
> Acked-by: Rob Herring <robh@kernel.org>

Applied to l2-mtd.git

[PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Marek Vasut]

From: Graham Moore <grmoore@opensource.altera.com>

Add binding document for the Cadence QSPI controller.

Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alan Tull <atull@opensource.altera.com>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Graham Moore <grmoore@opensource.altera.com>
Cc: Vikas MANOCHA <vikas.manocha@st.com>
Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
Cc: devicetree@vger.kernel.org
---
 .../devicetree/bindings/mtd/cadence-quadspi.txt    | 56 ++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/cadence-quadspi.txt

V2: Add cdns prefix to driver-specific bindings.
V3: Use existing property "is-decoded-cs" instead of creating a
    duplicate, "ext-decoder". Timing parameters are in nanoseconds,
    not master reference clocks. Remove bus-num completely.
V4: Add new properties fifo-width and trigger-address
V7: - Prefix all of the Cadence-specific properties with cdns prefix,
      those are in particular "cdns,is-decoded-cs", "cdns,fifo-depth",
      "cdns,fifo-width", "cdns,trigger-address".
    - Drop bogus properties which were not used and were incorrect.
V8: Align lines to 80 chars.

diff --git a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
new file mode 100644
index 0000000..f248056
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
@@ -0,0 +1,56 @@
+* Cadence Quad SPI controller
+
+Required properties:
+- compatible : Should be "cdns,qspi-nor".
+- reg : Contains two entries, each of which is a tuple consisting of a
+	physical address and length. The first entry is the address and
+	length of the controller register set. The second entry is the
+	address and length of the QSPI Controller data area.
+- interrupts : Unit interrupt specifier for the controller interrupt.
+- clocks : phandle to the Quad SPI clock.
+- cdns,fifo-depth : Size of the data FIFO in words.
+- cdns,fifo-width : Bus width of the data FIFO in bytes.
+- cdns,trigger-address : 32-bit indirect AHB trigger address.
+
+Optional properties:
+- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not.
+
+Optional subnodes:
+Subnodes of the Cadence Quad SPI controller are spi slave nodes with additional
+custom properties:
+- cdns,read-delay : Delay for read capture logic, in clock cycles
+- cdns,tshsl-ns : Delay in nanoseconds for the length that the master
+                  mode chip select outputs are de-asserted between
+		  transactions.
+- cdns,tsd2d-ns : Delay in nanoseconds between one chip select being
+                  de-activated and the activation of another.
+- cdns,tchsh-ns : Delay in nanoseconds between last bit of current
+                  transaction and deasserting the device chip select
+		  (qspi_n_ss_out).
+- cdns,tslch-ns : Delay in nanoseconds between setting qspi_n_ss_out low
+                  and first bit transfer.
+
+Example:
+
+	qspi: spi@ff705000 {
+		compatible = "cdns,qspi-nor";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0xff705000 0x1000>,
+		      <0xffa00000 0x1000>;
+		interrupts = <0 151 4>;
+		clocks = <&qspi_clk>;
+		cdns,is-decoded-cs;
+		cdns,fifo-depth = <128>;
+		cdns,fifo-width = <4>;
+		cdns,trigger-address = <0x00000000>;
+
+		flash0: n25q00@0 {
+			...
+			cdns,read-delay = <4>;
+			cdns,tshsl-ns = <50>;
+			cdns,tsd2d-ns = <50>;
+			cdns,tchsh-ns = <4>;
+			cdns,tslch-ns = <4>;
+		};
+	};
-- 
2.1.4

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[vikas]

Hi,

On 08/21/2015 02:20 AM, Marek Vasut wrote:
> From: Graham Moore <grmoore@opensource.altera.com>
> 
> Add binding document for the Cadence QSPI controller.
> 
> Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alan Tull <atull@opensource.altera.com>
> Cc: Brian Norris <computersforpeace@gmail.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Graham Moore <grmoore@opensource.altera.com>
> Cc: Vikas MANOCHA <vikas.manocha@st.com>
> Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
> Cc: devicetree@vger.kernel.org
> ---
>  .../devicetree/bindings/mtd/cadence-quadspi.txt    | 56 ++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> 
> V2: Add cdns prefix to driver-specific bindings.
> V3: Use existing property "is-decoded-cs" instead of creating a
>     duplicate, "ext-decoder". Timing parameters are in nanoseconds,
>     not master reference clocks. Remove bus-num completely.
> V4: Add new properties fifo-width and trigger-address
> V7: - Prefix all of the Cadence-specific properties with cdns prefix,
>       those are in particular "cdns,is-decoded-cs", "cdns,fifo-depth",
>       "cdns,fifo-width", "cdns,trigger-address".
>     - Drop bogus properties which were not used and were incorrect.
> V8: Align lines to 80 chars.
> 
> diff --git a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> new file mode 100644
> index 0000000..f248056
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> @@ -0,0 +1,56 @@
> +* Cadence Quad SPI controller
> +
> +Required properties:
> +- compatible : Should be "cdns,qspi-nor".
> +- reg : Contains two entries, each of which is a tuple consisting of a
> +	physical address and length. The first entry is the address and
> +	length of the controller register set. The second entry is the
> +	address and length of the QSPI Controller data area.

still hooked up with  "Controller data area", it is ambiguous.
Use something which is more clear: Nor Flash memory mapped address.

> +- interrupts : Unit interrupt specifier for the controller interrupt.
> +- clocks : phandle to the Quad SPI clock.
> +- cdns,fifo-depth : Size of the data FIFO in words.
> +- cdns,fifo-width : Bus width of the data FIFO in bytes.
> +- cdns,trigger-address : 32-bit indirect AHB trigger address.
> +
> +Optional properties:

again, is it optional ? can the driver be used without these properties ?

> +- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not.

again, add info what the decoder is for ? 

> +
> +Optional subnodes:
> +Subnodes of the Cadence Quad SPI controller are spi slave nodes with additional
> +custom properties:
> +- cdns,read-delay : Delay for read capture logic, in clock cycles
> +- cdns,tshsl-ns : Delay in nanoseconds for the length that the master
> +                  mode chip select outputs are de-asserted between
> +		  transactions.
> +- cdns,tsd2d-ns : Delay in nanoseconds between one chip select being
> +                  de-activated and the activation of another.
> +- cdns,tchsh-ns : Delay in nanoseconds between last bit of current
> +                  transaction and deasserting the device chip select
> +		  (qspi_n_ss_out).
> +- cdns,tslch-ns : Delay in nanoseconds between setting qspi_n_ss_out low
> +                  and first bit transfer.
> +
> +Example:
> +
> +	qspi: spi@ff705000 {
> +		compatible = "cdns,qspi-nor";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		reg = <0xff705000 0x1000>,
> +		      <0xffa00000 0x1000>;
> +		interrupts = <0 151 4>;
> +		clocks = <&qspi_clk>;
> +		cdns,is-decoded-cs;

flag value ?

Cheers,
Vikas

> +		cdns,fifo-depth = <128>;
> +		cdns,fifo-width = <4>;
> +		cdns,trigger-address = <0x00000000>;
> +
> +		flash0: n25q00@0 {
> +			...
> +			cdns,read-delay = <4>;
> +			cdns,tshsl-ns = <50>;
> +			cdns,tsd2d-ns = <50>;
> +			cdns,tchsh-ns = <4>;
> +			cdns,tslch-ns = <4>;
> +		};
> +	};
> 

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[Marek Vasut]

On Thursday, August 27, 2015 at 07:44:34 PM, vikas wrote:
> Hi,
> 
> On 08/21/2015 02:20 AM, Marek Vasut wrote:
> > From: Graham Moore <grmoore@opensource.altera.com>
> > 
> > Add binding document for the Cadence QSPI controller.
> > 
> > Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Alan Tull <atull@opensource.altera.com>
> > Cc: Brian Norris <computersforpeace@gmail.com>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > Cc: Graham Moore <grmoore@opensource.altera.com>
> > Cc: Vikas MANOCHA <vikas.manocha@st.com>
> > Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
> > Cc: devicetree@vger.kernel.org
> > ---
> > 
> >  .../devicetree/bindings/mtd/cadence-quadspi.txt    | 56
> >  ++++++++++++++++++++++ 1 file changed, 56 insertions(+)
> >  create mode 100644
> >  Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> > 
> > V2: Add cdns prefix to driver-specific bindings.
> > V3: Use existing property "is-decoded-cs" instead of creating a
> > 
> >     duplicate, "ext-decoder". Timing parameters are in nanoseconds,
> >     not master reference clocks. Remove bus-num completely.
> > 
> > V4: Add new properties fifo-width and trigger-address
> > V7: - Prefix all of the Cadence-specific properties with cdns prefix,
> > 
> >       those are in particular "cdns,is-decoded-cs", "cdns,fifo-depth",
> >       "cdns,fifo-width", "cdns,trigger-address".
> >     
> >     - Drop bogus properties which were not used and were incorrect.
> > 
> > V8: Align lines to 80 chars.
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> > b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt new file
> > mode 100644
> > index 0000000..f248056
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
> > @@ -0,0 +1,56 @@
> > +* Cadence Quad SPI controller
> > +
> > +Required properties:
> > +- compatible : Should be "cdns,qspi-nor".
> > +- reg : Contains two entries, each of which is a tuple consisting of a
> > +	physical address and length. The first entry is the address and
> > +	length of the controller register set. The second entry is the
> > +	address and length of the QSPI Controller data area.
> 
> still hooked up with  "Controller data area", it is ambiguous.
> Use something which is more clear: Nor Flash memory mapped address.

I have to disagree, I will call it whatever it is called in the datasheet
and it is called "controller data area".

> > +- interrupts : Unit interrupt specifier for the controller interrupt.
> > +- clocks : phandle to the Quad SPI clock.
> > +- cdns,fifo-depth : Size of the data FIFO in words.
> > +- cdns,fifo-width : Bus width of the data FIFO in bytes.
> > +- cdns,trigger-address : 32-bit indirect AHB trigger address.
> > +
> 
> > +Optional properties:
> again, is it optional ? can the driver be used without these properties ?

Why wouldn't it be possible to use the driver with no SPI NOR attached to
it? It's a cornercase, but still a valid one.

> > +- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not.
> 
> again, add info what the decoder is for ?

This is something Graham has to clarify. Based on the code (I'm sure you did
check the code), it's a 4:16 demuxer.

> > +
> > +Optional subnodes:
> > +Subnodes of the Cadence Quad SPI controller are spi slave nodes with
> > additional +custom properties:
> > +- cdns,read-delay : Delay for read capture logic, in clock cycles
> > +- cdns,tshsl-ns : Delay in nanoseconds for the length that the master
> > +                  mode chip select outputs are de-asserted between
> > +		  transactions.
> > +- cdns,tsd2d-ns : Delay in nanoseconds between one chip select being
> > +                  de-activated and the activation of another.
> > +- cdns,tchsh-ns : Delay in nanoseconds between last bit of current
> > +                  transaction and deasserting the device chip select
> > +		  (qspi_n_ss_out).
> > +- cdns,tslch-ns : Delay in nanoseconds between setting qspi_n_ss_out low
> > +                  and first bit transfer.
> > +
> > +Example:
> > +
> > +	qspi: spi@ff705000 {
> > +		compatible = "cdns,qspi-nor";
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		reg = <0xff705000 0x1000>,
> > +		      <0xffa00000 0x1000>;
> > +		interrupts = <0 151 4>;
> > +		clocks = <&qspi_clk>;
> > +		cdns,is-decoded-cs;
> 
> flag value ?

Sorry, I don't quite understand the question. If you mean why there is no
value, it's because this is a boolean OF node, which just does't need to
have a value ; it's either present or not.

Best regards,
Marek Vasut

Re: [PATCH V8 1/2] mtd: spi-nor: Bindings for Cadence Quad SPI Flash Controller driver.

[vikas]

Hi,

On 08/27/2015 11:12 AM, Marek Vasut wrote:
> On Thursday, August 27, 2015 at 07:44:34 PM, vikas wrote:
>> Hi,
>>
>> On 08/21/2015 02:20 AM, Marek Vasut wrote:
>>> From: Graham Moore <grmoore@opensource.altera.com>
>>>
>>> Add binding document for the Cadence QSPI controller.
>>>
>>> Signed-off-by: Graham Moore <grmoore@opensource.altera.com>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Alan Tull <atull@opensource.altera.com>
>>> Cc: Brian Norris <computersforpeace@gmail.com>
>>> Cc: David Woodhouse <dwmw2@infradead.org>
>>> Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
>>> Cc: Graham Moore <grmoore@opensource.altera.com>
>>> Cc: Vikas MANOCHA <vikas.manocha@st.com>
>>> Cc: Yves Vandervennet <yvanderv@opensource.altera.com>
>>> Cc: devicetree@vger.kernel.org
>>> ---
>>>
>>>  .../devicetree/bindings/mtd/cadence-quadspi.txt    | 56
>>>  ++++++++++++++++++++++ 1 file changed, 56 insertions(+)
>>>  create mode 100644
>>>  Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
>>>
>>> V2: Add cdns prefix to driver-specific bindings.
>>> V3: Use existing property "is-decoded-cs" instead of creating a
>>>
>>>     duplicate, "ext-decoder". Timing parameters are in nanoseconds,
>>>     not master reference clocks. Remove bus-num completely.
>>>
>>> V4: Add new properties fifo-width and trigger-address
>>> V7: - Prefix all of the Cadence-specific properties with cdns prefix,
>>>
>>>       those are in particular "cdns,is-decoded-cs", "cdns,fifo-depth",
>>>       "cdns,fifo-width", "cdns,trigger-address".
>>>     
>>>     - Drop bogus properties which were not used and were incorrect.
>>>
>>> V8: Align lines to 80 chars.
>>>
>>> diff --git a/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
>>> b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt new file
>>> mode 100644
>>> index 0000000..f248056
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mtd/cadence-quadspi.txt
>>> @@ -0,0 +1,56 @@
>>> +* Cadence Quad SPI controller
>>> +
>>> +Required properties:
>>> +- compatible : Should be "cdns,qspi-nor".
>>> +- reg : Contains two entries, each of which is a tuple consisting of a
>>> +	physical address and length. The first entry is the address and
>>> +	length of the controller register set. The second entry is the
>>> +	address and length of the QSPI Controller data area.
>>
>> still hooked up with  "Controller data area", it is ambiguous.
>> Use something which is more clear: Nor Flash memory mapped address.
> 
> I have to disagree, I will call it whatever it is called in the datasheet
> and it is called "controller data area".

It is preferable to use terminology which readers understand & that is the purpose
of explaining it here otherwise we could have just pasted the doc link.
I have to stop here for this point.

> 
>>> +- interrupts : Unit interrupt specifier for the controller interrupt.
>>> +- clocks : phandle to the Quad SPI clock.
>>> +- cdns,fifo-depth : Size of the data FIFO in words.
>>> +- cdns,fifo-width : Bus width of the data FIFO in bytes.
>>> +- cdns,trigger-address : 32-bit indirect AHB trigger address.
>>> +
>>
>>> +Optional properties:
>> again, is it optional ? can the driver be used without these properties ?
> 
> Why wouldn't it be possible to use the driver with no SPI NOR attached to
> it? It's a cornercase, but still a valid one.

that's not right, this controller is only spi flash controller.

> 
>>> +- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not.
>>
>> again, add info what the decoder is for ?
> 
> This is something Graham has to clarify. Based on the code (I'm sure you did
> check the code), it's a 4:16 demuxer.

Please clarify if possible & add the info for others benefit. This part is not common in other spi/nor controllers.

> 
>>> +
>>> +Optional subnodes:
>>> +Subnodes of the Cadence Quad SPI controller are spi slave nodes with
>>> additional +custom properties:
>>> +- cdns,read-delay : Delay for read capture logic, in clock cycles
>>> +- cdns,tshsl-ns : Delay in nanoseconds for the length that the master
>>> +                  mode chip select outputs are de-asserted between
>>> +		  transactions.
>>> +- cdns,tsd2d-ns : Delay in nanoseconds between one chip select being
>>> +                  de-activated and the activation of another.
>>> +- cdns,tchsh-ns : Delay in nanoseconds between last bit of current
>>> +                  transaction and deasserting the device chip select
>>> +		  (qspi_n_ss_out).
>>> +- cdns,tslch-ns : Delay in nanoseconds between setting qspi_n_ss_out low
>>> +                  and first bit transfer.
>>> +
>>> +Example:
>>> +
>>> +	qspi: spi@ff705000 {
>>> +		compatible = "cdns,qspi-nor";
>>> +		#address-cells = <1>;
>>> +		#size-cells = <0>;
>>> +		reg = <0xff705000 0x1000>,
>>> +		      <0xffa00000 0x1000>;
>>> +		interrupts = <0 151 4>;
>>> +		clocks = <&qspi_clk>;
>>> +		cdns,is-decoded-cs;
>>
>> flag value ?
> 
> Sorry, I don't quite understand the question. If you mean why there is no
> value, it's because this is a boolean OF node, which just does't need to
> have a value ; it's either present or not.

you are right, thanks.

Cheers,
Vikas

> 
> Best regards,
> Marek Vasut
> .
>