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

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

[Marek Vasut]

From: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>

Add binding document for the Cadence QSPI controller.

Signed-off-by: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
Cc: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Cc: Dinh Nguyen <dinguyen-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
Cc: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
Cc: Vikas MANOCHA <vikas.manocha-qxv4g6HH51o@public.gmane.org>
Cc: Yves Vandervennet <yvanderv-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 .../devicetree/bindings/mtd/cadence_quadspi.txt    | 50 ++++++++++++++++++++++
 1 file changed, 50 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.

diff --git a/Documentation/devicetree/bindings/mtd/cadence_quadspi.txt b/Documentation/devicetree/bindings/mtd/cadence_quadspi.txt
new file mode 100644
index 0000000..ebaf1fd
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/cadence_quadspi.txt
@@ -0,0 +1,50 @@
+* 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Marek Vasut]

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.

Signed-off-by: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
Cc: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
Cc: Dinh Nguyen <dinguyen-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
Cc: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
Cc: Vikas MANOCHA <vikas.manocha-qxv4g6HH51o@public.gmane.org>
Cc: Yves Vandervennet <yvanderv-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 drivers/mtd/spi-nor/Kconfig           |    6 +
 drivers/mtd/spi-nor/Makefile          |    1 +
 drivers/mtd/spi-nor/cadence-quadspi.c | 1266 +++++++++++++++++++++++++++++++++
 3 files changed, 1273 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.

diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 64a4f0e..9485481 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -28,4 +28,10 @@ config SPI_FSL_QUADSPI
 	  This enables support for the Quad SPI controller in master mode.
 	  We only connect the NOR to this controller now.
 
+config SPI_CADENCE_QUADSPI
+	tristate "Cadence Quad SPI controller"
+	depends on ARCH_SOCFPGA
+	help
+	  This enables support for the Cadence Quad SPI controller and NOR flash.
+
 endif # MTD_SPI_NOR
diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 6a7ce14..372628c 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
+obj-$(CONFIG_SPI_CADENCE_QUADSPI)	+= cadence-quadspi.o
 obj-$(CONFIG_SPI_FSL_QUADSPI)	+= fsl-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..f59e286
--- /dev/null
+++ b/drivers/mtd/spi-nor/cadence-quadspi.c
@@ -0,0 +1,1266 @@
+/*
+ * 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 mtd_info	mtd;
+	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;
+
+	u32			irq_mask;
+	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
+#define CQSPI_POLL_IDLE_RETRY			3
+
+#define CQSPI_REG_SRAM_RESV_WORDS		2
+#define CQSPI_REG_SRAM_PARTITION_WR		1
+#define CQSPI_REG_SRAM_THRESHOLD_BYTES		50
+
+/* Instruction type */
+#define CQSPI_INST_TYPE_SINGLE			0
+#define CQSPI_INST_TYPE_DUAL			1
+#define CQSPI_INST_TYPE_QUAD			2
+
+#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_RD_OVERFLOW		BIT(12)
+
+#define CQSPI_IRQ_STATUS_ERR		(CQSPI_REG_IRQ_MODE_ERR		| \
+					 CQSPI_REG_IRQ_IND_RD_REJECT	| \
+					 CQSPI_REG_IRQ_WR_PROTECTED_ERR	| \
+					 CQSPI_REG_IRQ_ILLEGAL_AHB_ERR)
+
+#define CQSPI_IRQ_MASK_RD		(CQSPI_REG_IRQ_WATERMARK	| \
+					 CQSPI_REG_IRQ_IND_RD_OVERFLOW	| \
+					 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 unsigned int cqspi_init_timeout(const unsigned long timeout_in_ms)
+{
+	return jiffies + msecs_to_jiffies(timeout_in_ms);
+}
+
+static unsigned int cqspi_check_timeout(const unsigned long timeout)
+{
+	return time_before(jiffies, timeout);
+}
+
+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);
+
+	if (irq_status & cqspi->irq_mask)
+		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 unsigned int cqspi_wait_idle(struct cqspi_st *cqspi)
+{
+	unsigned int count = 0;
+	unsigned timeout;
+
+	timeout = cqspi_init_timeout(CQSPI_TIMEOUT_MS);
+	while (cqspi_check_timeout(timeout)) {
+		if (cqspi_is_idle(cqspi)) {
+			/*
+			 * Read few times in succession to ensure it does
+			 * not transition low again
+			 */
+			count++;
+			if (count >= CQSPI_POLL_IDLE_RETRY)
+				return 1;
+		} else {
+			count = 0;
+		}
+		cpu_relax();
+	}
+
+	/* Timeout, in busy mode. */
+	dev_err(&cqspi->pdev->dev, "QSPI is still busy after %dms timeout.\n",
+		CQSPI_TIMEOUT_MS);
+	return 0;
+}
+
+static int cqspi_exec_flash_cmd(struct cqspi_st *cqspi, unsigned int reg)
+{
+	void __iomem *reg_base = cqspi->iobase;
+	unsigned int timeout;
+
+	/* 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. */
+	timeout = cqspi_init_timeout(CQSPI_TIMEOUT_MS);
+	while (cqspi_check_timeout(timeout)) {
+		reg = readl(reg_base + CQSPI_REG_CMDCTRL) &
+			    CQSPI_REG_CMDCTRL_INPROGRESS_MASK;
+		if (!reg)
+			break;
+	}
+
+	if (reg != 0) {
+		dev_err(&cqspi->pdev->dev, "flash cmd execute time out\n");
+		return -EIO;
+	}
+
+	/* Polling QSPI idle status. */
+	if (!cqspi_wait_idle(cqspi))
+		return -EIO;
+
+	return 0;
+}
+
+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 != 0)
+		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;
+
+	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 reg = 0;
+	unsigned int bytes_to_read = 0;
+	unsigned int timeout;
+	int ret = 0;
+
+	writel(remaining, reg_base + CQSPI_REG_INDIRECTRDBYTES);
+
+	/* Clear all interrupts. */
+	writel(CQSPI_IRQ_STATUS_MASK, reg_base + CQSPI_REG_IRQSTATUS);
+
+	cqspi->irq_mask = CQSPI_IRQ_MASK_RD;
+	writel(cqspi->irq_mask, 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 */
+	timeout = cqspi_init_timeout(CQSPI_TIMEOUT_MS);
+	while (cqspi_check_timeout(timeout)) {
+		reg = readl(reg_base + CQSPI_REG_INDIRECTRD);
+		if (reg & CQSPI_REG_INDIRECTRD_DONE_MASK)
+			break;
+	}
+
+	if (!(reg & CQSPI_REG_INDIRECTRD_DONE_MASK)) {
+		dev_err(nor->dev,
+			"Indirect read completion error 0x%08x\n", reg);
+		ret = -ETIMEDOUT;
+		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 timeout;
+	unsigned int reg = 0;
+	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);
+
+	cqspi->irq_mask = CQSPI_IRQ_MASK_WR;
+	writel(cqspi->irq_mask, 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;
+	}
+
+	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;
+	}
+
+	/* Check indirect done status */
+	timeout = cqspi_init_timeout(CQSPI_TIMEOUT_MS);
+	while (cqspi_check_timeout(timeout)) {
+		reg = readl(reg_base + CQSPI_REG_INDIRECTWR);
+		if (reg & CQSPI_REG_INDIRECTWR_DONE_MASK)
+			break;
+	}
+
+	if (!(reg & CQSPI_REG_INDIRECTWR_DONE_MASK)) {
+		dev_err(nor->dev,
+			"Indirect write completion error 0x%08x\n", reg);
+		ret = -ETIMEDOUT;
+		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 void cqspi_write(struct spi_nor *nor, loff_t to,
+			size_t len, size_t *retlen, const u_char *buf)
+{
+	int ret;
+
+	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_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;
+
+	/* Send write enable, then erase commands. */
+	ret = nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0, 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 int cqspi_set_protocol(struct spi_nor *nor, enum spi_protocol proto)
+{
+	struct cqspi_flash_pdata *f_pdata = nor->priv;
+
+	switch (proto) {
+	case SPI_PROTO_1_1_1:
+	case SPI_PROTO_1_1_2:
+	case SPI_PROTO_1_1_4:
+	case SPI_PROTO_1_2_2:
+	case SPI_PROTO_1_4_4:
+		f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE;
+		break;
+	case SPI_PROTO_2_2_2:
+		f_pdata->inst_width = CQSPI_INST_TYPE_DUAL;
+		break;
+	case SPI_PROTO_4_4_4:
+		f_pdata->inst_width = CQSPI_INST_TYPE_QUAD;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	switch (proto) {
+	case SPI_PROTO_1_1_1:
+	case SPI_PROTO_1_1_2:
+	case SPI_PROTO_1_1_4:
+		f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE;
+		break;
+	case SPI_PROTO_1_2_2:
+	case SPI_PROTO_2_2_2:
+		f_pdata->addr_width = CQSPI_INST_TYPE_DUAL;
+		break;
+	case SPI_PROTO_1_4_4:
+	case SPI_PROTO_4_4_4:
+		f_pdata->addr_width = CQSPI_INST_TYPE_QUAD;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	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)
+{
+	void __iomem *reg_base = cqspi->iobase;
+	unsigned int reg;
+
+	reg = readl(reg_base + CQSPI_REG_CONFIG);
+	reg |= CQSPI_REG_CONFIG_ENABLE_MASK;
+	writel(reg, reg_base + CQSPI_REG_CONFIG);
+}
+
+static void cqspi_controller_disable(struct cqspi_st *cqspi)
+{
+	void __iomem *reg_base = cqspi->iobase;
+	unsigned int reg;
+
+	reg = readl(reg_base + CQSPI_REG_CONFIG);
+	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_disable(cqspi);
+
+	/* 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 |= (nor->page_size << CQSPI_REG_SIZE_PAGE_LSB);
+	reg |= (ilog2(nor->mtd->erasesize) << CQSPI_REG_SIZE_BLOCK_LSB);
+	reg &= ~CQSPI_REG_SIZE_ADDRESS_MASK;
+	reg |= (nor->addr_width - 1);
+	writel(reg, iobase + CQSPI_REG_SIZE);
+
+	/* configure the chip select */
+	cqspi_chipselect(nor);
+
+	cqspi_controller_enable(cqspi);
+}
+
+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_disable(cqspi);
+		cqspi_config_baudrate_div(cqspi, sclk);
+		cqspi_delay(nor, sclk);
+		cqspi_readdata_capture(cqspi, 1, f_pdata->read_delay);
+		cqspi_controller_enable(cqspi);
+	}
+	return 0;
+}
+
+static int cqspi_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
+{
+	int 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 write_enable)
+{
+	int ret = 0;
+
+	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_disable(cqspi);
+
+	/* 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 and write watermark. */
+	writel(cqspi->fifo_depth * cqspi->fifo_width / 2,
+	       cqspi->iobase + CQSPI_REG_INDIRECTRDWATERMARK);
+	writel(CQSPI_REG_SRAM_THRESHOLD_BYTES,
+	       cqspi->iobase + CQSPI_REG_INDIRECTWRWATERMARK);
+
+	cqspi_controller_enable(cqspi);
+}
+
+static int cqspi_remove(struct platform_device *pdev)
+{
+	struct cqspi_st *cqspi = platform_get_drvdata(pdev);
+	int i;
+
+	cqspi_controller_disable(cqspi);
+
+	for (i = 0; i < CQSPI_MAX_CHIPSELECT; i++)
+		if (cqspi->f_pdata[i].mtd.name)
+			mtd_device_unregister(&cqspi->f_pdata[i].mtd);
+
+	return 0;
+}
+
+static int cqspi_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct mtd_part_parser_data ppdata;
+	struct device *dev = &pdev->dev;
+	struct cqspi_st *cqspi;
+	struct spi_nor *nor;
+	struct mtd_info *mtd;
+	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;
+
+	/* Get flash device data */
+	for_each_available_child_of_node(dev->of_node, np) {
+		unsigned int cs;
+		struct cqspi_flash_pdata *f_pdata;
+
+		if (of_property_read_u32(np, "reg", &cs)) {
+			dev_err(dev, "couldn't determine chip select\n");
+			return -ENXIO;
+		}
+
+		if (cs >= CQSPI_MAX_CHIPSELECT) {
+			dev_err(dev, "chip select %d out of range\n", cs);
+			return -ENXIO;
+		}
+
+		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 probe_failed;
+
+		nor = &f_pdata->nor;
+		mtd = &f_pdata->mtd;
+
+		nor->mtd = mtd;
+		nor->dev = dev;
+		nor->priv = f_pdata;
+		mtd->priv = nor;
+
+		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->set_protocol = cqspi_set_protocol;
+
+		nor->prepare = cqspi_prep;
+
+		/*
+		 * Here is a 'nasty hack' from Marek Vasut to pick
+		 * up OF properties from flash device subnode.
+		 */
+		nor->dev->of_node = np;
+
+		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
+		if (ret)
+			goto probe_failed;
+
+		if (nor->read_dummy > CQSPI_DUMMY_CLKS_MAX)
+			nor->read_dummy = CQSPI_DUMMY_CLKS_MAX;
+
+		ppdata.of_node = np;
+		ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
+		if (ret)
+			goto probe_failed;
+	}
+
+	dev_info(dev, "Cadence QSPI NOR flash driver\n");
+	return 0;
+
+probe_failed:
+	dev_err(dev, "Cadence QSPI NOR probe failed %d\n", ret);
+	cqspi_remove(pdev);
+	return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int cqspi_suspend(struct device *dev)
+{
+	struct cqspi_st *cqspi = dev_get_drvdata(dev);
+
+	cqspi_controller_disable(cqspi);
+	return 0;
+}
+
+static int cqspi_resume(struct device *dev)
+{
+	struct cqspi_st *cqspi = dev_get_drvdata(dev);
+
+	cqspi_controller_enable(cqspi);
+	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-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>");
+MODULE_AUTHOR("Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>");
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Marek Vasut]

On Friday, August 14, 2015 at 05:28:12 AM, 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.
> 
> Signed-off-by: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> Cc: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> Cc: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> Cc: Dinh Nguyen <dinguyen-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> Cc: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> Cc: Vikas MANOCHA <vikas.manocha-qxv4g6HH51o@public.gmane.org>
> Cc: Yves Vandervennet <yvanderv-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

[...]

> +	/* Get flash device data */
> +	for_each_available_child_of_node(dev->of_node, np) {
> +		unsigned int cs;
> +		struct cqspi_flash_pdata *f_pdata;
> +
> +		if (of_property_read_u32(np, "reg", &cs)) {
> +			dev_err(dev, "couldn't determine chip select\n");
> +			return -ENXIO;
> +		}
> +
> +		if (cs >= CQSPI_MAX_CHIPSELECT) {
> +			dev_err(dev, "chip select %d out of range\n", cs);
> +			return -ENXIO;
> +		}
> +
> +		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 probe_failed;
> +
> +		nor = &f_pdata->nor;
> +		mtd = &f_pdata->mtd;
> +
> +		nor->mtd = mtd;
> +		nor->dev = dev;
> +		nor->priv = f_pdata;
> +		mtd->priv = nor;
> +
> +		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->set_protocol = cqspi_set_protocol;
> +
> +		nor->prepare = cqspi_prep;
> +

--->8---

> +		/*
> +		 * Here is a 'nasty hack' from Marek Vasut to pick
> +		 * up OF properties from flash device subnode.
> +		 */
> +		nor->dev->of_node = np;
> +
> +		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
> +		if (ret)
> +			goto probe_failed;

The only bizzare thing is this stuff above ^ . If I want to pass for example 
"m25p,fast-read" to the SPI NOR connected to this controller, I have to set
up the nor->dev->of_node, otherwise the of_node would point to the controller.
I am positive this is wrong, but I'm not quite sure how to repair this.
Brian, can you please comment on this one bit ?

[...]
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Brian Norris]

On Fri, Aug 14, 2015 at 05:32:34AM +0200, Marek Vasut wrote:
> On Friday, August 14, 2015 at 05:28:12 AM, Marek Vasut wrote:
> 
> > +	/* Get flash device data */
> > +	for_each_available_child_of_node(dev->of_node, np) {
...
> --->8---
> 
> > +		/*
> > +		 * Here is a 'nasty hack' from Marek Vasut to pick
> > +		 * up OF properties from flash device subnode.
> > +		 */
> > +		nor->dev->of_node = np;
> > +
> > +		ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
> > +		if (ret)
> > +			goto probe_failed;
> 
> The only bizzare thing is this stuff above ^ . If I want to pass for example 
> "m25p,fast-read" to the SPI NOR connected to this controller, I have to set

Do we really want to extend m25p80 properties like 'm25p,fast-read' to
all SPI NOR controllers? Are they necessary? It seems that there has
been some attempt to do so, but I'm not sure if that's by good design or
just by accident.

> up the nor->dev->of_node, otherwise the of_node would point to the controller.
> I am positive this is wrong, but I'm not quite sure how to repair this.
> Brian, can you please comment on this one bit ?

The problem is that spi_nor_scan() is assuming that nor->dev represents
a flash device, not a flash controller (to which we might connect
multiple flash). So we need to provide a way for spi_nor_scan() to find
the flash device_node, not the controller device_node. Easy option:

 (a) add a field to struct spi_nor, like I did to nand_chip [1]; e.g.,
     spi_nor::dn.

In doing that, we might then reevaluate what spi_nor::dev is supposed to
mean (and clarify the doc comments in include/linux/mtd/spi-nor.h
accordingly). Currently, it's used to shoe-horn in DT support (badly),
as well as to provide mostly auxiliary info, like naming, debug info
(dev_{dbg,info,err,etc...}()), and simliar. The latter half can actually
be problematic too, since they start to become less useful once you have
more than one flash connected to a single controller. e.g., you'll get
colliding MTD names (via dev_name(nor->dev)) and debug info is suddenly
less obvious (which flash chip is the log message from?).

So, we might want to do something in the long run to avoid the mixing of
layers that looks more like:

 (b) remove 'dev' from struct spi_nor entirely

We can do debug prints and such with spi-nor-specific printk()
formatters (e.g., snor_{info,dbg,err,etc...}()) and stop assuming that
dev_name(nor->dev) is actually a good name for an MTD.

While we're at it... we may also want a larger rework to allow spi-nor.c
to better support a notion of controllers (using the existing platform
device) and flash devices (mostly without the use of struct device, and
mostly using struct spi_nor as-is). You'll notice that controllers that
want to support multiple flash are starting to develop much of the same
initialization boiler-plate code.

Anyway, that's my rambling thoughts for now. I think (a) is pretty
straightforward, correct, and quickly attainable, so you can ignore the
remaining bits in the context of upstreaming this driver.

Brian

[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=5844feeaa4154d1c46d3462c7a4653d22356d8b4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Marek Vasut]

On Tuesday, August 18, 2015 at 04:47:35 AM, Brian Norris wrote:

Hi!

[...]

> > The only bizzare thing is this stuff above ^ . If I want to pass for
> > example "m25p,fast-read" to the SPI NOR connected to this controller, I
> > have to set
> 
> Do we really want to extend m25p80 properties like 'm25p,fast-read' to
> all SPI NOR controllers? Are they necessary? It seems that there has
> been some attempt to do so, but I'm not sure if that's by good design or
> just by accident.

I guess we might want to support at least the m25p,fast-read prop.
I think it might be a good idea to let the SPI NOR framework parse
the common props, while also let the SPI NOR controller drivers parse
whatever props they need.

> > up the nor->dev->of_node, otherwise the of_node would point to the
> > controller. I am positive this is wrong, but I'm not quite sure how to
> > repair this. Brian, can you please comment on this one bit ?
> 
> The problem is that spi_nor_scan() is assuming that nor->dev represents
> a flash device, not a flash controller (to which we might connect
> multiple flash). So we need to provide a way for spi_nor_scan() to find
> the flash device_node, not the controller device_node. Easy option:

Right.

>  (a) add a field to struct spi_nor, like I did to nand_chip [1]; e.g.,
>      spi_nor::dn.
> 
> In doing that, we might then reevaluate what spi_nor::dev is supposed to
> mean (and clarify the doc comments in include/linux/mtd/spi-nor.h
> accordingly). Currently, it's used to shoe-horn in DT support (badly),
> as well as to provide mostly auxiliary info, like naming, debug info
> (dev_{dbg,info,err,etc...}()), and simliar. The latter half can actually
> be problematic too, since they start to become less useful once you have
> more than one flash connected to a single controller. e.g., you'll get
> colliding MTD names (via dev_name(nor->dev)) and debug info is suddenly
> less obvious (which flash chip is the log message from?).

Right. Would it make sense to have one device per one SPI NOR flash and
then another one for the controller ?

> So, we might want to do something in the long run to avoid the mixing of
> layers that looks more like:
> 
>  (b) remove 'dev' from struct spi_nor entirely
> 
> We can do debug prints and such with spi-nor-specific printk()
> formatters (e.g., snor_{info,dbg,err,etc...}()) and stop assuming that
> dev_name(nor->dev) is actually a good name for an MTD.

I cannot say I'm very fond of introducing new ad-hoc kernel printing
facility just for the sake of dealing with this.

> While we're at it... we may also want a larger rework to allow spi-nor.c
> to better support a notion of controllers (using the existing platform
> device) and flash devices (mostly without the use of struct device, and
> mostly using struct spi_nor as-is).

See my question about the devices above please, I'm not quite sure here.

> You'll notice that controllers that
> want to support multiple flash are starting to develop much of the same
> initialization boiler-plate code.

Yep, I tweaked the Cadence driver such, that the boilerplate code is nicely
isolated in a separate function now, so this would also be visible :)

> Anyway, that's my rambling thoughts for now. I think (a) is pretty
> straightforward, correct, and quickly attainable

I just did that, it was a couple lines of code. I think I need to write a
better commit message for it before I send it out.

> so you can ignore the
> remaining bits in the context of upstreaming this driver.
> 
> Brian
> 
> [1]
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id
> =5844feeaa4154d1c46d3462c7a4653d22356d8b4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[vikas]

Hi Marek,

On 08/13/2015 08:28 PM, 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.
> 
> Signed-off-by: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> Cc: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> Cc: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> Cc: Dinh Nguyen <dinguyen-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> Cc: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> Cc: Vikas MANOCHA <vikas.manocha-qxv4g6HH51o@public.gmane.org>
> Cc: Yves Vandervennet <yvanderv-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  drivers/mtd/spi-nor/Kconfig           |    6 +
>  drivers/mtd/spi-nor/Makefile          |    1 +
>  drivers/mtd/spi-nor/cadence-quadspi.c | 1266 +++++++++++++++++++++++++++++++++
>  3 files changed, 1273 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.
> 
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 64a4f0e..9485481 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -28,4 +28,10 @@ config SPI_FSL_QUADSPI
>           This enables support for the Quad SPI controller in master mode.
>           We only connect the NOR to this controller now.
> 
> +config SPI_CADENCE_QUADSPI
> +       tristate "Cadence Quad SPI controller"
> +       depends on ARCH_SOCFPGA

Remove the dependency on SOCFPGA.

> +       help
> +         This enables support for the Cadence Quad SPI controller and NOR flash.
> +
>  endif # MTD_SPI_NOR
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 6a7ce14..372628c 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -1,2 +1,3 @@
>  obj-$(CONFIG_MTD_SPI_NOR)      += spi-nor.o
> +obj-$(CONFIG_SPI_CADENCE_QUADSPI)      += cadence-quadspi.o
>  obj-$(CONFIG_SPI_FSL_QUADSPI)  += fsl-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..f59e286
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/cadence-quadspi.c
> @@ -0,0 +1,1266 @@
> +/*
> + * 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 mtd_info mtd;
> +       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;
> +
> +       u32                     irq_mask;
> +       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

please add some comment about the timeouts value selection.

> +#define CQSPI_POLL_IDLE_RETRY                  3
> +
> +#define CQSPI_REG_SRAM_RESV_WORDS              2
> +#define CQSPI_REG_SRAM_PARTITION_WR            1

remove unused macros.

> +#define CQSPI_REG_SRAM_THRESHOLD_BYTES         50

the macro is used only for write sram watermark, something like ...WR_THRESH_BYTES would be better.
Infact instead of magic number like 50, it should be based on sram_depth similar to read watermark configuration.

> +
> +/* Instruction type */
> +#define CQSPI_INST_TYPE_SINGLE                 0
> +#define CQSPI_INST_TYPE_DUAL                   1
> +#define CQSPI_INST_TYPE_QUAD                   2
> +
> +#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

To be consistent, it would be good to use BIT(nr) for all bit positions.

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

I am not sure if there is any recommended way but instread of register macros with offset, isn't it better
to use something like struct cdns_qspi { 
					u32 config,
					u32 rd_instn,
					....
				};
& then configuring the pointer to this structure to the peripheral's (qspi controller in this case) base address.

It helps in debugging also as you can have all registers under one structure, easy/clean access of register like cdsn_qspi_ptr->config
instead of adding offset for every access & also clean picture of all peripheral registers.

> +
> +/* 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_RD_OVERFLOW          BIT(12)
> +
> +#define CQSPI_IRQ_STATUS_ERR           (CQSPI_REG_IRQ_MODE_ERR         | \
> +                                        CQSPI_REG_IRQ_IND_RD_REJECT    | \
> +                                        CQSPI_REG_IRQ_WR_PROTECTED_ERR | \
> +                                        CQSPI_REG_IRQ_ILLEGAL_AHB_ERR)
> +
> +#define CQSPI_IRQ_MASK_RD              (CQSPI_REG_IRQ_WATERMARK        | \
> +                                        CQSPI_REG_IRQ_IND_RD_OVERFLOW  | \
> +                                        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 unsigned int cqspi_init_timeout(const unsigned long timeout_in_ms)
> +{
> +       return jiffies + msecs_to_jiffies(timeout_in_ms);
> +}

unnecessary function, msecs_to_jiffies can be used directly in cqspi_check_timeout function.

> +
> +static unsigned int cqspi_check_timeout(const unsigned long timeout)
> +{
> +       return time_before(jiffies, timeout);
> +}

try to replace using blocking jiffies check using kernel timeout function.

> +
> +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);
> +
> +       if (irq_status & cqspi->irq_mask)
> +               complete(&cqspi->transfer_complete);
> +
> +       return IRQ_HANDLED;
> +}
> +

[...]

> +
> +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 reg = 0;
> +       unsigned int bytes_to_read = 0;
> +       unsigned int timeout;
> +       int ret = 0;
> +
> +       writel(remaining, reg_base + CQSPI_REG_INDIRECTRDBYTES);
> +
> +       /* Clear all interrupts. */
> +       writel(CQSPI_IRQ_STATUS_MASK, reg_base + CQSPI_REG_IRQSTATUS);
> +
> +       cqspi->irq_mask = CQSPI_IRQ_MASK_RD;
> +       writel(cqspi->irq_mask, reg_base + CQSPI_REG_IRQMASK);

here interrupt mask is being configured for every read, better would be to move it in init.

> +
> +       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 */
> +       timeout = cqspi_init_timeout(CQSPI_TIMEOUT_MS);
> +       while (cqspi_check_timeout(timeout)) {
> +               reg = readl(reg_base + CQSPI_REG_INDIRECTRD);
> +               if (reg & CQSPI_REG_INDIRECTRD_DONE_MASK)
> +                       break;
> +       }
> +
> +       if (!(reg & CQSPI_REG_INDIRECTRD_DONE_MASK)) {
> +               dev_err(nor->dev,
> +                       "Indirect read completion error 0x%08x\n", reg);
> +               ret = -ETIMEDOUT;
> +               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 timeout;
> +       unsigned int reg = 0;
> +       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);
> +
> +       cqspi->irq_mask = CQSPI_IRQ_MASK_WR;
> +       writel(cqspi->irq_mask, reg_base + CQSPI_REG_IRQMASK);

same like read, it should be moved to init.

> +
> +       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;
> +       }
> +
> +       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;
> +       }
> +
> +       /* Check indirect done status */
> +       timeout = cqspi_init_timeout(CQSPI_TIMEOUT_MS);
> +       while (cqspi_check_timeout(timeout)) {
> +               reg = readl(reg_base + CQSPI_REG_INDIRECTWR);
> +               if (reg & CQSPI_REG_INDIRECTWR_DONE_MASK)
> +                       break;
> +       }
> +
> +       if (!(reg & CQSPI_REG_INDIRECTWR_DONE_MASK)) {
> +               dev_err(nor->dev,
> +                       "Indirect write completion error 0x%08x\n", reg);
> +               ret = -ETIMEDOUT;
> +               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 void cqspi_controller_init(struct cqspi_st *cqspi)
> +{
> +       cqspi_controller_disable(cqspi);
> +
> +       /* 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. */

almost all comments of this function seems unnecessary.

> +       writel(cqspi->trigger_address,
> +              cqspi->iobase + CQSPI_REG_INDIRECTTRIGGER);
> +
> +       /* Program read and write watermark. */
> +       writel(cqspi->fifo_depth * cqspi->fifo_width / 2,
> +              cqspi->iobase + CQSPI_REG_INDIRECTRDWATERMARK);
> +       writel(CQSPI_REG_SRAM_THRESHOLD_BYTES,
> +              cqspi->iobase + CQSPI_REG_INDIRECTWRWATERMARK);

better to configure indirect write watermark based on sram depth.

Rgds,
Vikas

> +
> +       cqspi_controller_enable(cqspi);
> +}
> +
> +static int cqspi_remove(struct platform_device *pdev)
> +{
> +       struct cqspi_st *cqspi = platform_get_drvdata(pdev);
> +       int i;
> +
> +       cqspi_controller_disable(cqspi);
> +
> +       for (i = 0; i < CQSPI_MAX_CHIPSELECT; i++)
> +               if (cqspi->f_pdata[i].mtd.name)
> +                       mtd_device_unregister(&cqspi->f_pdata[i].mtd);
> +
> +       return 0;
> +}
> +
> +static int cqspi_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct mtd_part_parser_data ppdata;
> +       struct device *dev = &pdev->dev;
> +       struct cqspi_st *cqspi;
> +       struct spi_nor *nor;
> +       struct mtd_info *mtd;
> +       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;
> +
> +       /* Get flash device data */
> +       for_each_available_child_of_node(dev->of_node, np) {
> +               unsigned int cs;
> +               struct cqspi_flash_pdata *f_pdata;
> +
> +               if (of_property_read_u32(np, "reg", &cs)) {
> +                       dev_err(dev, "couldn't determine chip select\n");
> +                       return -ENXIO;
> +               }
> +
> +               if (cs >= CQSPI_MAX_CHIPSELECT) {
> +                       dev_err(dev, "chip select %d out of range\n", cs);
> +                       return -ENXIO;
> +               }
> +
> +               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 probe_failed;
> +
> +               nor = &f_pdata->nor;
> +               mtd = &f_pdata->mtd;
> +
> +               nor->mtd = mtd;
> +               nor->dev = dev;
> +               nor->priv = f_pdata;
> +               mtd->priv = nor;
> +
> +               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->set_protocol = cqspi_set_protocol;
> +
> +               nor->prepare = cqspi_prep;
> +
> +               /*
> +                * Here is a 'nasty hack' from Marek Vasut to pick
> +                * up OF properties from flash device subnode.
> +                */
> +               nor->dev->of_node = np;
> +
> +               ret = spi_nor_scan(nor, NULL, SPI_NOR_QUAD);
> +               if (ret)
> +                       goto probe_failed;
> +
> +               if (nor->read_dummy > CQSPI_DUMMY_CLKS_MAX)
> +                       nor->read_dummy = CQSPI_DUMMY_CLKS_MAX;
> +
> +               ppdata.of_node = np;
> +               ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
> +               if (ret)
> +                       goto probe_failed;
> +       }
> +
> +       dev_info(dev, "Cadence QSPI NOR flash driver\n");
> +       return 0;
> +
> +probe_failed:
> +       dev_err(dev, "Cadence QSPI NOR probe failed %d\n", ret);
> +       cqspi_remove(pdev);
> +       return ret;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int cqspi_suspend(struct device *dev)
> +{
> +       struct cqspi_st *cqspi = dev_get_drvdata(dev);
> +
> +       cqspi_controller_disable(cqspi);
> +       return 0;
> +}
> +
> +static int cqspi_resume(struct device *dev)
> +{
> +       struct cqspi_st *cqspi = dev_get_drvdata(dev);
> +
> +       cqspi_controller_enable(cqspi);
> +       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-EIB2kfCEclfQT0dZR+AlfA@public.gmane.org>");
> +MODULE_AUTHOR("Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>");
> --
> 2.1.4
> 
> .
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Graham Moore]

Hi Vikas,

On 08/17/2015 09:34 PM, vikas wrote:
 > Hi Marek,
 >

[...]

 >> +
>> +/* Operation timeout value */
>> +#define CQSPI_TIMEOUT_MS                       500
>> +#define CQSPI_READ_TIMEOUT_MS                  10
>
> please add some comment about the timeouts value selection.
>

I wish I could comment, but I don't know the origin of these values. 
The 500 ms value is probably just "a very long time".

[...]

>> +
>> +       cqspi->irq_mask = CQSPI_IRQ_MASK_RD;
>> +       writel(cqspi->irq_mask, reg_base + CQSPI_REG_IRQMASK);
>
> here interrupt mask is being configured for every read, better would be to move it in init.
>

[...]

>> +
>> +       cqspi->irq_mask = CQSPI_IRQ_MASK_WR;
>> +       writel(cqspi->irq_mask, reg_base + CQSPI_REG_IRQMASK);
>
> same like read, it should be moved to init.
>

It uses different masks for read and write

[...]

BR,
Graham


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[vikas]

Hi,


On 08/18/2015 12:17 PM, Graham Moore wrote:
> Hi Vikas,
> 
> On 08/17/2015 09:34 PM, vikas wrote:
>  > Hi Marek,
>  >
> 
> [...]
> 
>  >> +
>>> +/* Operation timeout value */
>>> +#define CQSPI_TIMEOUT_MS                       500
>>> +#define CQSPI_READ_TIMEOUT_MS                  10
>>
>> please add some comment about the timeouts value selection.
>>
> 
> I wish I could comment, but I don't know the origin of these values. 
> The 500 ms value is probably just "a very long time".

In my opinion we should have some logical value based on some worst timing like read/write sector.
I let you decide on this point.

> 
> [...]
> 
>>> +
>>> +       cqspi->irq_mask = CQSPI_IRQ_MASK_RD;
>>> +       writel(cqspi->irq_mask, reg_base + CQSPI_REG_IRQMASK);
>>
>> here interrupt mask is being configured for every read, better would be to move it in init.
>>
> 
> [...]
> 
>>> +
>>> +       cqspi->irq_mask = CQSPI_IRQ_MASK_WR;
>>> +       writel(cqspi->irq_mask, reg_base + CQSPI_REG_IRQMASK);
>>
>> same like read, it should be moved to init.
>>
> 
> It uses different masks for read and write

Yeah i saw it but why not to OR these values & configure for once in init. 
After that in ISR, check for the interrupt source & take action accordingly. I think other drivers also use it this way.

Rgds,
Vikas

> 
> [...]
> 
> BR,
> Graham
> 
> .
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Marek Vasut]

On Tuesday, August 18, 2015 at 04:34:53 AM, vikas wrote:
> Hi Marek,

Hi,

[...]

> > +#define CQSPI_POLL_IDLE_RETRY                  3
> > +
> > +#define CQSPI_REG_SRAM_RESV_WORDS              2
> > +#define CQSPI_REG_SRAM_PARTITION_WR            1
> 
> remove unused macros.
> 
> > +#define CQSPI_REG_SRAM_THRESHOLD_BYTES         50
> 
> the macro is used only for write sram watermark, something like
> ...WR_THRESH_BYTES would be better. Infact instead of magic number like
> 50, it should be based on sram_depth similar to read watermark
> configuration.

I suppose if the fifo falls below 1/8, that might be a sensible time to
trigger an underflow interrupt.

> > +
> > +/* Instruction type */
> > +#define CQSPI_INST_TYPE_SINGLE                 0
> > +#define CQSPI_INST_TYPE_DUAL                   1
> > +#define CQSPI_INST_TYPE_QUAD                   2
> > +
> > +#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
> 
> To be consistent, it would be good to use BIT(nr) for all bit positions.

You don't use BIT() macro for bitfields and the above are bitfields.
Thus, this is not applicable.

> > +#define CQSPI_REG_CONFIG_CHIPSELECT_MASK       0xF
> > +#define CQSPI_REG_CONFIG_BAUD_MASK             0xF

[...]

> > +#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
> 
> I am not sure if there is any recommended way but instread of register
> macros with offset, isn't it better to use something like struct cdns_qspi
> {
> 					u32 config,
> 					u32 rd_instn,
> 					....
> 				};
> & then configuring the pointer to this structure to the peripheral's (qspi
> controller in this case) base address.

U-Boot is slowly abolishing this practice as it turned out to be a horrible
mistake, which is annoying to deal with . No, I will not do this.

> It helps in debugging also as you can have all registers under one
> structure, easy/clean access of register like cdsn_qspi_ptr->config
> instead of adding offset for every access & also clean picture of all
> peripheral registers.

Once you have three similar controllers with only one register mapped
elsewhere, you'd need three such structures. This approach does look
nice at first, but certainly does not scale.

> > +
> > +/* 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_RD_OVERFLOW          BIT(12)

[...]

> > +
> > +static unsigned int cqspi_check_timeout(const unsigned long timeout)
> > +{
> > +       return time_before(jiffies, timeout);
> > +}
> 
> try to replace using blocking jiffies check using kernel timeout function.
> 

What function do you refer to ?

[...]

> > +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 reg = 0;
> > +       unsigned int bytes_to_read = 0;
> > +       unsigned int timeout;
> > +       int ret = 0;
> > +
> > +       writel(remaining, reg_base + CQSPI_REG_INDIRECTRDBYTES);
> > +
> > +       /* Clear all interrupts. */
> > +       writel(CQSPI_IRQ_STATUS_MASK, reg_base + CQSPI_REG_IRQSTATUS);
> > +
> > +       cqspi->irq_mask = CQSPI_IRQ_MASK_RD;
> > +       writel(cqspi->irq_mask, reg_base + CQSPI_REG_IRQMASK);
> 
> here interrupt mask is being configured for every read, better would be to
> move it in init.

It certainly would not, it is configured differently for READ and WRITE.

> > +
> > +       reinit_completion(&cqspi->transfer_complete);
> > +       writel(CQSPI_REG_INDIRECTRD_START_MASK,
> > +              reg_base + CQSPI_REG_INDIRECTRD);
> > +
> > +       while (remaining > 0) {
> > +               ret =

[...]

> > +static void cqspi_controller_init(struct cqspi_st *cqspi)
> > +{
> > +       cqspi_controller_disable(cqspi);
> > +
> > +       /* 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. */
> 
> almost all comments of this function seems unnecessary.

I disagree, having sensible comments in code makes my life easier after
I have to revisit the code a few years later.

> > +       writel(cqspi->trigger_address,
> > +              cqspi->iobase + CQSPI_REG_INDIRECTTRIGGER);
> > +
> > +       /* Program read and write watermark. */
> > +       writel(cqspi->fifo_depth * cqspi->fifo_width / 2,
> > +              cqspi->iobase + CQSPI_REG_INDIRECTRDWATERMARK);
> > +       writel(CQSPI_REG_SRAM_THRESHOLD_BYTES,
> > +              cqspi->iobase + CQSPI_REG_INDIRECTWRWATERMARK);

[...]

Best regards,
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Vikas MANOCHA]

Hi,



> On Aug 20, 2015, at 10:20 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> 
>> On Tuesday, August 18, 2015 at 04:34:53 AM, vikas wrote:
>> Hi Marek,
> 
> Hi,
> 
> [...]
> 
>>> +#define CQSPI_POLL_IDLE_RETRY                  3
>>> +
>>> +#define CQSPI_REG_SRAM_RESV_WORDS              2
>>> +#define CQSPI_REG_SRAM_PARTITION_WR            1
>> 
>> remove unused macros.
>> 
>>> +#define CQSPI_REG_SRAM_THRESHOLD_BYTES         50
>> 
>> the macro is used only for write sram watermark, something like
>> ...WR_THRESH_BYTES would be better. Infact instead of magic number like
>> 50, it should be based on sram_depth similar to read watermark
>> configuration.
> 
> I suppose if the fifo falls below 1/8, that might be a sensible time to
> trigger an underflow interrupt.
> 
>>> +
>>> +/* Instruction type */
>>> +#define CQSPI_INST_TYPE_SINGLE                 0
>>> +#define CQSPI_INST_TYPE_DUAL                   1
>>> +#define CQSPI_INST_TYPE_QUAD                   2
>>> +
>>> +#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
>> 
>> To be consistent, it would be good to use BIT(nr) for all bit positions.
> 
> You don't use BIT() macro for bitfields and the above are bitfields.
> Thus, this is not applicable.
> 
>>> +#define CQSPI_REG_CONFIG_CHIPSELECT_MASK       0xF
>>> +#define CQSPI_REG_CONFIG_BAUD_MASK             0xF
> 
> [...]
> 
>>> +#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
>> 
>> I am not sure if there is any recommended way but instread of register
>> macros with offset, isn't it better to use something like struct cdns_qspi
>> {
>>                    u32 config,
>>                    u32 rd_instn,
>>                    ....
>>                };
>> & then configuring the pointer to this structure to the peripheral's (qspi
>> controller in this case) base address.
> 
> U-Boot is slowly abolishing this practice as it turned out to be a horrible
> mistake, which is annoying to deal with . No, I will not do this.
> 
>> It helps in debugging also as you can have all registers under one
>> structure, easy/clean access of register like cdsn_qspi_ptr->config
>> instead of adding offset for every access & also clean picture of all
>> peripheral registers.
> 
> Once you have three similar controllers with only one register mapped
> elsewhere, you'd need three such structures. This approach does look
> nice at first, but certainly does not scale.
> 
>>> +
>>> +/* 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_RD_OVERFLOW          BIT(12)
> 
> [...]
> 
>>> +
>>> +static unsigned int cqspi_check_timeout(const unsigned long timeout)
>>> +{
>>> +       return time_before(jiffies, timeout);
>>> +}
>> 
>> try to replace using blocking jiffies check using kernel timeout function.
> 
> What function do you refer to ?
> 
> [...]
> 
>>> +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 reg = 0;
>>> +       unsigned int bytes_to_read = 0;
>>> +       unsigned int timeout;
>>> +       int ret = 0;
>>> +
>>> +       writel(remaining, reg_base + CQSPI_REG_INDIRECTRDBYTES);
>>> +
>>> +       /* Clear all interrupts. */
>>> +       writel(CQSPI_IRQ_STATUS_MASK, reg_base + CQSPI_REG_IRQSTATUS);
>>> +
>>> +       cqspi->irq_mask = CQSPI_IRQ_MASK_RD;
>>> +       writel(cqspi->irq_mask, reg_base + CQSPI_REG_IRQMASK);
>> 
>> here interrupt mask is being configured for every read, better would be to
>> move it in init.
> 
> It certainly would not, it is configured differently for READ and WRITE.

And what is blocking to configure read and write mask together at one time in the register ?

> 
>>> +
>>> +       reinit_completion(&cqspi->transfer_complete);
>>> +       writel(CQSPI_REG_INDIRECTRD_START_MASK,
>>> +              reg_base + CQSPI_REG_INDIRECTRD);
>>> +
>>> +       while (remaining > 0) {
>>> +               ret =
> 
> [...]
> 
>>> +static void cqspi_controller_init(struct cqspi_st *cqspi)
>>> +{
>>> +       cqspi_controller_disable(cqspi);
>>> +
>>> +       /* 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. */
>> 
>> almost all comments of this function seems unnecessary.
> 
> I disagree, having sensible comments in code makes my life easier after
> I have to revisit the code a few years later.

Yes for sensible comments , you are loading value in one register and commenting it.

Cheers,
Vikas 

> 
>>> +       writel(cqspi->trigger_address,
>>> +              cqspi->iobase + CQSPI_REG_INDIRECTTRIGGER);
>>> +
>>> +       /* Program read and write watermark. */
>>> +       writel(cqspi->fifo_depth * cqspi->fifo_width / 2,
>>> +              cqspi->iobase + CQSPI_REG_INDIRECTRDWATERMARK);
>>> +       writel(CQSPI_REG_SRAM_THRESHOLD_BYTES,
>>> +              cqspi->iobase + CQSPI_REG_INDIRECTWRWATERMARK);
> 
> [...]
> 
> Best regards,
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[vikas]

Hi Marek,

On 08/13/2015 08:28 PM, Marek Vasut wrote:
> From: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> 
> Add binding document for the Cadence QSPI controller.
> 
> Signed-off-by: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> Cc: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> Cc: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> Cc: Dinh Nguyen <dinguyen-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> Cc: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> Cc: Vikas MANOCHA <vikas.manocha-qxv4g6HH51o@public.gmane.org>
> Cc: Yves Vandervennet <yvanderv-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  .../devicetree/bindings/mtd/cadence_quadspi.txt    | 50 ++++++++++++++++++++++
>  1 file changed, 50 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.
> 
> diff --git a/Documentation/devicetree/bindings/mtd/cadence_quadspi.txt b/Documentation/devicetree/bindings/mtd/cadence_quadspi.txt
> new file mode 100644
> index 0000000..ebaf1fd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/cadence_quadspi.txt
> @@ -0,0 +1,50 @@
> +* 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.

"Controller data area", i think it means mapped NOR Flash address ?
If yes, it would be more clear with "Physical base address & size of NOR Flash".

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

It actually is sram-depth, better would be "sram-depth" to avoid confusion.

> +- cdns,fifo-width: Bus width of the data FIFO in bytes.
> +- cdns,trigger-address : 32-bit indirect AHB trigger address.

we might make trigger-address also part of property "reg".

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

Not clear about the usage of decoder, if it does not make sense we might remove it from driver.

> +
> +Optional subnodes:

I think flash subnode is mandatory.

Rgds,
Vikas

> +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>;
> +		};
> +	};
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Marek Vasut]

On Tuesday, August 18, 2015 at 04:35:55 AM, vikas wrote:
> Hi Marek,
> 
> On 08/13/2015 08:28 PM, Marek Vasut wrote:
> > From: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> > 
> > Add binding document for the Cadence QSPI controller.
> > 
> > Signed-off-by: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> > Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > Cc: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> > Cc: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Cc: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> > Cc: Dinh Nguyen <dinguyen-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> > Cc: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> > Cc: Vikas MANOCHA <vikas.manocha-qxv4g6HH51o@public.gmane.org>
> > Cc: Yves Vandervennet <yvanderv-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> > Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > ---
> > 
> >  .../devicetree/bindings/mtd/cadence_quadspi.txt    | 50
> >  ++++++++++++++++++++++ 1 file changed, 50 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.
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/cadence_quadspi.txt
> > b/Documentation/devicetree/bindings/mtd/cadence_quadspi.txt new file
> > mode 100644
> > index 0000000..ebaf1fd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/cadence_quadspi.txt
> > @@ -0,0 +1,50 @@
> > +* 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.
> 
> "Controller data area", i think it means mapped NOR Flash address ?

Probably ; Graham ?

> If yes, it would be more clear with "Physical base address & size of NOR
> Flash".

This is the Direct mode thing, correct ? We don't support this, so I think
we should drop this bit altogether and keep only one single address in this
field.

> > +- 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.
> 
> It actually is sram-depth, better would be "sram-depth" to avoid confusion.

Is there any documentation for this piece to which you can point me ?

> > +- cdns,fifo-width: Bus width of the data FIFO in bytes.
> > +- cdns,trigger-address : 32-bit indirect AHB trigger address.
> 
> we might make trigger-address also part of property "reg".

No, we cannot, it's not part of the controller's base address, is it ?

> > +
> > +Optional properties:
> > +- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not.
> 
> Not clear about the usage of decoder, if it does not make sense we might
> remove it from driver.

Graham ? SoCFPGA uses this decoded-cs thing, so we absolutely can not remove
it from the driver.

> > +Optional subnodes:
> I think flash subnode is mandatory.

You can have a controller with no SPI NORs on it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Vikas MANOCHA]

Ho Marek,



> On Aug 17, 2015, at 9:48 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> 
>> On Tuesday, August 18, 2015 at 04:35:55 AM, vikas wrote:
>> Hi Marek,
>> 
>>> On 08/13/2015 08:28 PM, Marek Vasut wrote:
>>> From: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>> 
>>> Add binding document for the Cadence QSPI controller.
>>> 
>>> Signed-off-by: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>> Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
>>> Cc: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>> Cc: Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> Cc: David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
>>> Cc: Dinh Nguyen <dinguyen-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>> Cc: Graham Moore <grmoore-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>> Cc: Vikas MANOCHA <vikas.manocha-qxv4g6HH51o@public.gmane.org>
>>> Cc: Yves Vandervennet <yvanderv-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> ---
>>> 
>>> .../devicetree/bindings/mtd/cadence_quadspi.txt    | 50
>>> ++++++++++++++++++++++ 1 file changed, 50 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.
>>> 
>>> diff --git a/Documentation/devicetree/bindings/mtd/cadence_quadspi.txt
>>> b/Documentation/devicetree/bindings/mtd/cadence_quadspi.txt new file
>>> mode 100644
>>> index 0000000..ebaf1fd
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mtd/cadence_quadspi.txt
>>> @@ -0,0 +1,50 @@
>>> +* 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.
>> 
>> "Controller data area", i think it means mapped NOR Flash address ?
> 
> Probably ; Graham ?
> 
>> If yes, it would be more clear with "Physical base address & size of NOR
>> Flash".
> 
> This is the Direct mode thing, correct ? We don't support this, so I think
> we should drop this bit altogether and keep only one single address in this
> field.

No it's not.

> 
>>> +- 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.
>> 
>> It actually is sram-depth, better would be "sram-depth" to avoid confusion.
> 
> Is there any documentation for this piece to which you can point me ?

It's on the controller data sheet , should also be part of socfpga doc, let me know the document you have access to, I will point to the relevant part.

> 
>>> +- cdns,fifo-width: Bus width of the data FIFO in bytes.
>>> +- cdns,trigger-address : 32-bit indirect AHB trigger address.
>> 
>> we might make trigger-address also part of property "reg".
> 
> No, we cannot, it's not part of the controller's base address, is it ?

Ok.

> 
>>> +
>>> +Optional properties:
>>> +- cdns,is-decoded-cs : Flag to indicate whether decoder is used or not.
>> 
>> Not clear about the usage of decoder, if it does not make sense we might
>> remove it from driver.
> 
> Graham ? SoCFPGA uses this decoded-cs thing, so we absolutely can not remove
> it from the driver.
> 
>>> +Optional subnodes:
>> I think flash subnode is mandatory.
> 
> You can have a controller with no SPI NORs on it.

Without spi nor this controller/driver can't be used for anything.

Rgds,
Vikas --
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Graham Moore]

Hi all,

On 08/18/2015 12:48 AM, Vikas MANOCHA wrote:

[...]

>>>> +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.
>>>
>>> "Controller data area", i think it means mapped NOR Flash address ?
>>
>> Probably ; Graham ?
>>
>>> If yes, it would be more clear with "Physical base address & size of NOR
>>> Flash".
>>
>> This is the Direct mode thing, correct ? We don't support this, so I think
>> we should drop this bit altogether and keep only one single address in this
>> field.
>
> No it's not.
>

It's the location of the SRAM fifo.  Also direct mode location I think, 
if that were ever used.

The size is determined by a configuration parameter during system 
design.  On Altera Cyclone5 the size is really big compared to SRAM 
fifo.  I don't know why, maybe some hw engineer thought it would be 
better to have a large size in case direct mode was used.

BR,
Graham

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[vikas]

Hi,

On 08/18/2015 12:03 PM, Graham Moore wrote:
> Hi all,
> 
> On 08/18/2015 12:48 AM, Vikas MANOCHA wrote:
> 
> [...]
> 
>>>>> +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.
>>>>
>>>> "Controller data area", i think it means mapped NOR Flash address ?
>>>
>>> Probably ; Graham ?
>>>
>>>> If yes, it would be more clear with "Physical base address & size of NOR
>>>> Flash".
>>>
>>> This is the Direct mode thing, correct ? We don't support this, so I think
>>> we should drop this bit altogether and keep only one single address in this
>>> field.
>>
>> No it's not.
>>
> 
> It's the location of the SRAM fifo.  Also direct mode location I think, 
> if that were ever used.

Hmm...It is the base address of NOR flash. SRAM is not memory mapped.

> 
> The size is determined by a configuration parameter during system 
> design.  On Altera Cyclone5 the size is really big compared to SRAM 
> fifo.  I don't know why, maybe some hw engineer thought it would be 
> better to have a large size in case direct mode was used.

my comment is about second parameter of property "reg" which is NOR flash address, so above explanation does not
make sense for it.
Also in direct mode, sram does not come into play.

Rgds,
Vikas

> 
> BR,
> Graham
> .
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Marek Vasut]

On Tuesday, August 18, 2015 at 10:18:33 PM, vikas wrote:
> Hi,
> 
> On 08/18/2015 12:03 PM, Graham Moore wrote:
> > Hi all,
> > 
> > On 08/18/2015 12:48 AM, Vikas MANOCHA wrote:
> > 
> > [...]
> > 
> >>>>> +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.
> >>>> 
> >>>> "Controller data area", i think it means mapped NOR Flash address ?
> >>> 
> >>> Probably ; Graham ?
> >>> 
> >>>> If yes, it would be more clear with "Physical base address & size of
> >>>> NOR Flash".
> >>> 
> >>> This is the Direct mode thing, correct ? We don't support this, so I
> >>> think we should drop this bit altogether and keep only one single
> >>> address in this field.
> >> 
> >> No it's not.
> > 
> > It's the location of the SRAM fifo.  Also direct mode location I think,
> > if that were ever used.
> 
> Hmm...It is the base address of NOR flash. SRAM is not memory mapped.

Huh ? I am inclined to trust Graham more -- I have seen memory mapped SRAM,
but I have yet to see memory mapped SPI NOR. Also, the driver code clearly
uses that area in a way one would use a memory mapped SRAM with FIFO on the
other side. I think the above description is pretty much OK.

> > The size is determined by a configuration parameter during system
> > design.  On Altera Cyclone5 the size is really big compared to SRAM
> > fifo.  I don't know why, maybe some hw engineer thought it would be
> > better to have a large size in case direct mode was used.
> 
> my comment is about second parameter of property "reg" which is NOR flash
> address, so above explanation does not make sense for it.
> Also in direct mode, sram does not come into play.

This is absolutelly not a SPI NOR address.

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[vikas]

Hi,

On 08/19/2015 09:03 PM, Marek Vasut wrote:
> On Tuesday, August 18, 2015 at 10:18:33 PM, vikas wrote:
>> Hi,
>>
>> On 08/18/2015 12:03 PM, Graham Moore wrote:
>>> Hi all,
>>>
>>> On 08/18/2015 12:48 AM, Vikas MANOCHA wrote:
>>>
>>> [...]
>>>
>>>>>>> +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.
>>>>>>
>>>>>> "Controller data area", i think it means mapped NOR Flash address ?
>>>>>
>>>>> Probably ; Graham ?
>>>>>
>>>>>> If yes, it would be more clear with "Physical base address & size of
>>>>>> NOR Flash".
>>>>>
>>>>> This is the Direct mode thing, correct ? We don't support this, so I
>>>>> think we should drop this bit altogether and keep only one single
>>>>> address in this field.
>>>>
>>>> No it's not.
>>>
>>> It's the location of the SRAM fifo.  Also direct mode location I think,
>>> if that were ever used.
>>
>> Hmm...It is the base address of NOR flash. SRAM is not memory mapped.
> 
> Huh ? I am inclined to trust Graham more -- I have seen memory mapped SRAM,
> but I have yet to see memory mapped SPI NOR.

Well, SPI NOR flash in SOCs normally is memory mapped.
To give some mainline examples, all Spear family SOCs (spear300, 320, 1310, 1340).

> Also, the driver code clearly
> uses that area in a way one would use a memory mapped SRAM with FIFO on the
> other side. I think the above description is pretty much OK.

that is the purpose of making NOR flash memory mapped.

> 
>>> The size is determined by a configuration parameter during system
>>> design.  On Altera Cyclone5 the size is really big compared to SRAM
>>> fifo.  I don't know why, maybe some hw engineer thought it would be
>>> better to have a large size in case direct mode was used.
>>
>> my comment is about second parameter of property "reg" which is NOR flash
>> address, so above explanation does not make sense for it.
>> Also in direct mode, sram does not come into play.
> 
> This is absolutelly not a SPI NOR address.

Then i can only suggest to check out the controller literature.

Think like this : what is the purpose of SRAM in all this flash access business, It can work without SRAM also,
the only purpose of sram (& in fact indirect mode) is to access data from flash memory without AHB access to trigger it.
Once the data is available in SRAM(in case of read), AHB Master can access it with low letency. Same is true for writes.
SRAM is integral internal part of state machine in case of indirect mode, there is no need for it to memory mapped. If the controller
does not support indirect mode, there is no need of sram in the system.

Hope it is little bit more clear.

Cheers,
Vikas

> 
> Best regards,
> Marek Vasut
> .
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Marek Vasut]

On Thursday, August 20, 2015 at 06:06:49 PM, vikas wrote:
> Hi,

Hi!

[...]

> >>> It's the location of the SRAM fifo.  Also direct mode location I think,
> >>> if that were ever used.
> >> 
> >> Hmm...It is the base address of NOR flash. SRAM is not memory mapped.
> > 
> > Huh ? I am inclined to trust Graham more -- I have seen memory mapped
> > SRAM, but I have yet to see memory mapped SPI NOR.
> 
> Well, SPI NOR flash in SOCs normally is memory mapped.

I'm afraid you have some very disturbed views of how SPI NORs are operated
most of the time. No, SPI NOR is most often hanging off of some sort of SPI
controller just like any other SPI device. It is definitelly not common for
a SPI NOR to be memory mapped.

> To give some mainline examples, all Spear family SOCs (spear300, 320, 1310,
> 1340).

That's an exception, not a rule :)

> > Also, the driver code clearly
> > uses that area in a way one would use a memory mapped SRAM with FIFO on
> > the other side. I think the above description is pretty much OK.
> 
> that is the purpose of making NOR flash memory mapped.

I'm not sure if you agree with me or not anymore. I am pretty sure the 
discussion went quite far for such a trivial matter though.

> >>> The size is determined by a configuration parameter during system
> >>> design.  On Altera Cyclone5 the size is really big compared to SRAM
> >>> fifo.  I don't know why, maybe some hw engineer thought it would be
> >>> better to have a large size in case direct mode was used.
> >> 
> >> my comment is about second parameter of property "reg" which is NOR
> >> flash address, so above explanation does not make sense for it.
> >> Also in direct mode, sram does not come into play.
> > 
> > This is absolutelly not a SPI NOR address.
> 
> Then i can only suggest to check out the controller literature.
> 
> Think like this : what is the purpose of SRAM in all this flash access
> business, It can work without SRAM also, the only purpose of sram (& in
> fact indirect mode) is to access data from flash memory without AHB access
> to trigger it. Once the data is available in SRAM(in case of read), AHB
> Master can access it with low letency. Same is true for writes. SRAM is
> integral internal part of state machine in case of indirect mode, there is
> no need for it to memory mapped. If the controller does not support
> indirect mode, there is no need of sram in the system.
> 
> Hope it is little bit more clear.

It is, I understand what you're trying to convey now.

Yes, this register area can be used in certain modes of the controller in
such a way that it presents a window of the SPI NOR, which can be directly
accessed.

This register area can also be used as a FIFO, which is the way we used this
area in the driver . We read/write the first four bytes when communicating
with the flash.

So I don't really see a problem with calling the second register a "Controller
data area", that's what the IP documentation also calls it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Vikas MANOCHA]

Hi,



> On Aug 20, 2015, at 10:20 PM, Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> wrote:
> 
>> On Thursday, August 20, 2015 at 06:06:49 PM, vikas wrote:
>> Hi,
> 
> Hi!
> 
> [...]
> 
>>>>> It's the location of the SRAM fifo.  Also direct mode location I think,
>>>>> if that were ever used.
>>>> 
>>>> Hmm...It is the base address of NOR flash. SRAM is not memory mapped.
>>> 
>>> Huh ? I am inclined to trust Graham more -- I have seen memory mapped
>>> SRAM, but I have yet to see memory mapped SPI NOR.
>> 
>> Well, SPI NOR flash in SOCs normally is memory mapped.
> 
> I'm afraid you have some very disturbed views of how SPI NORs are operated
> most of the time.

Just wondering what should I say, hoping you will sit back and try to correct your views.

> No, SPI NOR is most often hanging off of some sort of SPI
> controller just like any other SPI device. It is definitelly not common for
> a SPI NOR to be memory mapped.

Definitely ? And what made you think like this ? Do you even know the spi peripheral you are sending patch for has memory mapped nor flash ?

> 
>> To give some mainline examples, all Spear family SOCs (spear300, 320, 1310,
>> 1340).
> 
> That's an exception, not a rule :)

And there are many such exceptions starting from this cadence qspi :-)

> 
>>> Also, the driver code clearly
>>> uses that area in a way one would use a memory mapped SRAM with FIFO on
>>> the other side. I think the above description is pretty much OK.
>> 
>> that is the purpose of making NOR flash memory mapped.
> 
> I'm not sure if you agree with me or not anymore. I am pretty sure the 
> discussion went quite far for such a trivial matter though.

I definitely disagree with you.
I stop here now.

Cheers,
Vikas

> 
>>>>> The size is determined by a configuration parameter during system
>>>>> design.  On Altera Cyclone5 the size is really big compared to SRAM
>>>>> fifo.  I don't know why, maybe some hw engineer thought it would be
>>>>> better to have a large size in case direct mode was used.
>>>> 
>>>> my comment is about second parameter of property "reg" which is NOR
>>>> flash address, so above explanation does not make sense for it.
>>>> Also in direct mode, sram does not come into play.
>>> 
>>> This is absolutelly not a SPI NOR address.
>> 
>> Then i can only suggest to check out the controller literature.
>> 
>> Think like this : what is the purpose of SRAM in all this flash access
>> business, It can work without SRAM also, the only purpose of sram (& in
>> fact indirect mode) is to access data from flash memory without AHB access
>> to trigger it. Once the data is available in SRAM(in case of read), AHB
>> Master can access it with low letency. Same is true for writes. SRAM is
>> integral internal part of state machine in case of indirect mode, there is
>> no need for it to memory mapped. If the controller does not support
>> indirect mode, there is no need of sram in the system.
>> 
>> Hope it is little bit more clear.
> 
> It is, I understand what you're trying to convey now.
> 
> Yes, this register area can be used in certain modes of the controller in
> such a way that it presents a window of the SPI NOR, which can be directly
> accessed.
> 
> This register area can also be used as a FIFO, which is the way we used this
> area in the driver . We read/write the first four bytes when communicating
> with the flash.
> 
> So I don't really see a problem with calling the second register a "Controller
> data area", that's what the IP documentation also calls it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html