[PATCH v4 0/3] Mediatek SPI-NOR flash driver

[PATCH v4 0/3] Mediatek SPI-NOR flash driver

[Bayi Cheng]

This series is based on v4.3-rc1 and l2-mtd.git
(git://git.infradead.org/l2-mtd.git)

Change in v4:
1: delete the parament "write_enable" for mt8173_nor_write_reg
2: fix the build warning for calling mt8173_nor_write_single_byte

Change in v3:
1: use switch() to replace some if-else statement
2: use shifts to replace endianness statement
3: delete some unused macros
4: use auto-increment mechanism for single write
5: write address added to 32bytes

Change in v2:
1. Rebase to 4.3-rc1
2. propagate error code
3. delete mux clock and axi clock in dts file
4. descripts more exactly for binding file
5. change file names from mtk-nor.c to mtk_quadspi.c
6. delete some functions witch were used once time

Bayi Cheng (3):
  doc: dt: add documentation for Mediatek spi-nor controller
  mtd: mtk-nor: mtk serial flash controller driver
  arm64: dts: mt8173: Add nor flash node

 .../devicetree/bindings/mtd/mtk-quadspi.txt        |  30 ++
 arch/arm64/boot/dts/mediatek/mt8173.dtsi           |  15 +
 drivers/mtd/spi-nor/Kconfig                        |   7 +
 drivers/mtd/spi-nor/Makefile                       |   1 +
 drivers/mtd/spi-nor/mtk-quadspi.c                  | 486 +++++++++++++++++++++
 5 files changed, 539 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
 create mode 100644 drivers/mtd/spi-nor/mtk-quadspi.c

--
1.8.1.1.dirty

[PATCH v4 1/3] doc: dt: add documentation for Mediatek spi-nor controller

[Bayi Cheng]

Add device tree binding documentation for serial flash with
Mediatek serial flash controller

Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
---
 .../devicetree/bindings/mtd/mtk-quadspi.txt        | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/mtk-quadspi.txt

diff --git a/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt b/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
new file mode 100644
index 0000000..8fff0ed
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
@@ -0,0 +1,30 @@
+* MTD SPI nor driver for MTK MT81xx (and similar) serial flash controller
+
+Required properties:
+- compatible: 	  should be "mediatek,mt8173-nor";
+- reg: 		  physical base address and length of the controller's register
+- clocks: 	  the phandle of the clock needed by the nor controller
+- clock-names: 	  the name of the clocks
+		  the clocks needed "spi" and "sf". "spi" is used for spi bus,
+		  and "sf" is used for controller, these are the clocks witch
+		  hardware needs to enabling nor flash and nor flash controller.
+		  See Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
+- #address-cells: should be <1>
+- #size-cells:	  should be <0>
+
+Example:
+
+nor_flash: spi@1100d000 {
+	compatible = "mediatek,mt8173-nor";
+	reg = <0 0x1100d000 0 0xe0>;
+	clocks = <&pericfg CLK_PERI_SPI>,
+		 <&topckgen CLK_TOP_SPINFI_IFR_SEL>;
+	clock-names = "spi", "sf";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	flash@0 {
+		....
+	};
+};
+
-- 
1.8.1.1.dirty

Re: [PATCH v4 1/3] doc: dt: add documentation for Mediatek spi-nor controller

[Brian Norris]

Hi Bayi,

On Tue, Oct 13, 2015 at 05:39:18PM +0800, Bayi Cheng wrote:
> Add device tree binding documentation for serial flash with
> Mediatek serial flash controller
> 
> Signed-off-by: Bayi Cheng <bayi.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  .../devicetree/bindings/mtd/mtk-quadspi.txt        | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
> 
> diff --git a/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt b/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
> new file mode 100644
> index 0000000..8fff0ed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/mtk-quadspi.txt
> @@ -0,0 +1,30 @@
> +* MTD SPI nor driver for MTK MT81xx (and similar) serial flash controller
> +
> +Required properties:
> +- compatible: 	  should be "mediatek,mt8173-nor";
> +- reg: 		  physical base address and length of the controller's register
> +- clocks: 	  the phandle of the clock needed by the nor controller
> +- clock-names: 	  the name of the clocks
> +		  the clocks needed "spi" and "sf". "spi" is used for spi bus,
> +		  and "sf" is used for controller, these are the clocks witch
> +		  hardware needs to enabling nor flash and nor flash controller.
> +		  See Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
> +- #address-cells: should be <1>
> +- #size-cells:	  should be <0>
> +
> +Example:
> +
> +nor_flash: spi@1100d000 {
> +	compatible = "mediatek,mt8173-nor";
> +	reg = <0 0x1100d000 0 0xe0>;
> +	clocks = <&pericfg CLK_PERI_SPI>,
> +		 <&topckgen CLK_TOP_SPINFI_IFR_SEL>;
> +	clock-names = "spi", "sf";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	flash@0 {

I see you've added the 'flash' node as I requested, but you didn't
document it above, and your driver doesn't use it.

Brian

> +		....
> +	};
> +};
> +
> -- 
> 1.8.1.1.dirty
> 
--
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 v4 2/3] mtd: mtk-nor: mtk serial flash controller driver

[Bayi Cheng]

add spi nor flash driver for mediatek controller

Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
---
 drivers/mtd/spi-nor/Kconfig       |   7 +
 drivers/mtd/spi-nor/Makefile      |   1 +
 drivers/mtd/spi-nor/mtk-quadspi.c | 486 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 494 insertions(+)
 create mode 100644 drivers/mtd/spi-nor/mtk-quadspi.c

diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
index 89bf4c1..f433890 100644
--- a/drivers/mtd/spi-nor/Kconfig
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -7,6 +7,13 @@ menuconfig MTD_SPI_NOR
 
 if MTD_SPI_NOR
 
+config MTD_MT81xx_NOR
+	tristate "Support SPI flash Controller MTD_MT81xx_NOR"
+	help
+	  This enables access to SPI Nor flash, using MTD_MT81XX_NOR controller.
+	  This controller does nor support generic SPI BUS, It only supports
+	  SPI NOR Flash.
+
 config MTD_SPI_NOR_USE_4K_SECTORS
 	bool "Use small 4096 B erase sectors"
 	default y
diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index e53333e..37c020a 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -1,3 +1,4 @@
+obj-$(CONFIG_MTD_MT81xx_NOR)	+= mtk-quadspi.o
 obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
 obj-$(CONFIG_SPI_FSL_QUADSPI)	+= fsl-quadspi.o
 obj-$(CONFIG_SPI_NXP_SPIFI)	+= nxp-spifi.o
diff --git a/drivers/mtd/spi-nor/mtk-quadspi.c b/drivers/mtd/spi-nor/mtk-quadspi.c
new file mode 100644
index 0000000..c6ac366
--- /dev/null
+++ b/drivers/mtd/spi-nor/mtk-quadspi.c
@@ -0,0 +1,486 @@
+/*
+ * Copyright (c) 2015 MediaTek Inc.
+ * Author: Bayi Cheng <bayi.cheng@mediatek.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/ioport.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <linux/mtd/spi-nor.h>
+
+#define MTK_NOR_CMD_REG			0x00
+#define MTK_NOR_CNT_REG			0x04
+#define MTK_NOR_RDSR_REG		0x08
+#define MTK_NOR_RDATA_REG		0x0c
+#define MTK_NOR_RADR0_REG		0x10
+#define MTK_NOR_RADR1_REG		0x14
+#define MTK_NOR_RADR2_REG		0x18
+#define MTK_NOR_WDATA_REG		0x1c
+#define MTK_NOR_PRGDATA0_REG		0x20
+#define MTK_NOR_PRGDATA1_REG		0x24
+#define MTK_NOR_PRGDATA2_REG		0x28
+#define MTK_NOR_PRGDATA3_REG		0x2c
+#define MTK_NOR_PRGDATA4_REG		0x30
+#define MTK_NOR_PRGDATA5_REG		0x34
+#define MTK_NOR_SHREG0_REG		0x38
+#define MTK_NOR_SHREG1_REG		0x3c
+#define MTK_NOR_SHREG2_REG		0x40
+#define MTK_NOR_SHREG3_REG		0x44
+#define MTK_NOR_SHREG4_REG		0x48
+#define MTK_NOR_SHREG5_REG		0x4c
+#define MTK_NOR_SHREG6_REG		0x50
+#define MTK_NOR_SHREG7_REG		0x54
+#define MTK_NOR_SHREG8_REG		0x58
+#define MTK_NOR_SHREG9_REG		0x5c
+#define MTK_NOR_CFG1_REG		0x60
+#define MTK_NOR_CFG2_REG		0x64
+#define MTK_NOR_CFG3_REG		0x68
+#define MTK_NOR_STATUS0_REG		0x70
+#define MTK_NOR_STATUS1_REG		0x74
+#define MTK_NOR_STATUS2_REG		0x78
+#define MTK_NOR_STATUS3_REG		0x7c
+#define MTK_NOR_FLHCFG_REG		0x84
+#define MTK_NOR_TIME_REG		0x94
+#define MTK_NOR_PP_DATA_REG		0x98
+#define MTK_NOR_PREBUF_STUS_REG		0x9c
+#define MTK_NOR_DELSEL0_REG		0xa0
+#define MTK_NOR_DELSEL1_REG		0xa4
+#define MTK_NOR_INTRSTUS_REG		0xa8
+#define MTK_NOR_INTREN_REG		0xac
+#define MTK_NOR_CHKSUM_CTL_REG		0xb8
+#define MTK_NOR_CHKSUM_REG		0xbc
+#define MTK_NOR_CMD2_REG		0xc0
+#define MTK_NOR_WRPROT_REG		0xc4
+#define MTK_NOR_RADR3_REG		0xc8
+#define MTK_NOR_DUAL_REG		0xcc
+#define MTK_NOR_DELSEL2_REG		0xd0
+#define MTK_NOR_DELSEL3_REG		0xd4
+#define MTK_NOR_DELSEL4_REG		0xd8
+
+/* commands for mtk nor controller */
+#define MTK_NOR_READ_CMD		0x0
+#define MTK_NOR_RDSR_CMD		0x2
+#define MTK_NOR_PRG_CMD			0x4
+#define MTK_NOR_WR_CMD			0x10
+#define MTK_NOR_PIO_WR_CMD		0x90
+#define MTK_NOR_WRSR_CMD		0x20
+#define MTK_NOR_PIO_READ_CMD		0x81
+#define MTK_NOR_WR_BUF_ENABLE		0x1
+#define MTK_NOR_WR_BUF_DISABLE		0x0
+#define MTK_NOR_ENABLE_SF_CMD		0x30
+#define MTK_NOR_DUAD_ADDR_EN		0x8
+#define MTK_NOR_QUAD_READ_EN		0x4
+#define MTK_NOR_DUAL_ADDR_EN		0x2
+#define MTK_NOR_DUAL_READ_EN		0x1
+#define MTK_NOR_DUAL_DISABLE		0x0
+#define MTK_NOR_FAST_READ		0x1
+
+#define SFLASH_WRBUF_SIZE		128
+
+struct mt8173_nor {
+	struct mtd_info mtd;
+	struct spi_nor nor;
+	struct device *dev;
+	void __iomem *base;	/* nor flash base address */
+	struct clk *spi_clk;
+	struct clk *nor_clk;
+};
+
+static void mt8173_nor_set_read_mode(struct mt8173_nor *mt8173_nor)
+{
+	struct spi_nor *nor = &mt8173_nor->nor;
+
+	switch (nor->flash_read) {
+	case SPI_NOR_FAST:
+		writeb(SPINOR_OP_READ_FAST, mt8173_nor->base +
+		       MTK_NOR_PRGDATA3_REG);
+		writeb(MTK_NOR_FAST_READ, mt8173_nor->base +
+		       MTK_NOR_CFG1_REG);
+		break;
+	case SPI_NOR_DUAL:
+		writeb(SPINOR_OP_READ_1_1_2, mt8173_nor->base +
+		       MTK_NOR_PRGDATA3_REG);
+		writeb(MTK_NOR_DUAL_READ_EN, mt8173_nor->base +
+		       MTK_NOR_DUAL_REG);
+		break;
+	case SPI_NOR_QUAD:
+		writeb(SPINOR_OP_READ_1_1_4, mt8173_nor->base +
+		       MTK_NOR_PRGDATA3_REG);
+		writeb(MTK_NOR_QUAD_READ_EN, mt8173_nor->base +
+		       MTK_NOR_DUAL_REG);
+		break;
+	default:
+		writeb(SPINOR_OP_READ, mt8173_nor->base +
+		       MTK_NOR_PRGDATA3_REG);
+		writeb(MTK_NOR_DUAL_DISABLE, mt8173_nor->base +
+		       MTK_NOR_DUAL_REG);
+		break;
+	}
+}
+
+static int mt8173_nor_execute_cmd(struct mt8173_nor *mt8173_nor, u8 cmdval)
+{
+	int reg;
+	u8 val = cmdval & 0x1f;
+
+	writeb(cmdval, mt8173_nor->base + MTK_NOR_CMD_REG);
+	return readl_poll_timeout(mt8173_nor->base + MTK_NOR_CMD_REG, reg,
+				  !(reg & val), 100, 10000);
+}
+
+static int mt8173_nor_set_cmd(struct mt8173_nor *mt8173_nor, int addr, u8 len,
+			      u8 op)
+{
+	writeb(op, mt8173_nor->base + MTK_NOR_PRGDATA5_REG);
+	/*  send the address to nor flash
+	 *  MTK_NOR_PRGDATA5_REG is shifted first
+	 */
+	if (len > 8) {
+		writeb((addr >> 16), mt8173_nor->base + MTK_NOR_PRGDATA4_REG);
+		writeb((addr >> 8), mt8173_nor->base + MTK_NOR_PRGDATA3_REG);
+		writeb((addr), mt8173_nor->base + MTK_NOR_PRGDATA2_REG);
+	}
+	writeb(len, mt8173_nor->base + MTK_NOR_CNT_REG);
+	return mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_PRG_CMD);
+}
+
+/* cmd1 sent to nor flash, cmd2 write to nor controller */
+static int mt8173_nor_set_para(struct mt8173_nor *mt8173_nor, int cmd1,
+			       int cmd2)
+{
+	int ret;
+
+	ret = mt8173_nor_set_cmd(mt8173_nor, 0, 8, SPINOR_OP_WREN);
+	if (ret < 0)
+		return ret;
+
+	writeb(cmd1, mt8173_nor->base + MTK_NOR_PRGDATA5_REG);
+	writeb(8, mt8173_nor->base + MTK_NOR_CNT_REG);
+	return mt8173_nor_execute_cmd(mt8173_nor, cmd2);
+}
+
+static int mt8173_nor_write_buffer_enable(struct mt8173_nor *mt8173_nor)
+{
+	u8 reg;
+
+	/* the bit0 of MTK_NOR_CFG2_REG is pre-fetch buffer
+	 * 0: pre-fetch buffer use for read
+	 * 1: pre-fetch buffer use for page program
+	 */
+	writel(MTK_NOR_WR_BUF_ENABLE, mt8173_nor->base + MTK_NOR_CFG2_REG);
+	return readb_poll_timeout(mt8173_nor->base + MTK_NOR_CFG2_REG, reg,
+				  0x01 == (reg & 0x01), 100, 10000);
+}
+
+static int mt8173_nor_write_buffer_disable(struct mt8173_nor *mt8173_nor)
+{
+	u8 reg;
+
+	writel(MTK_NOR_WR_BUF_DISABLE, mt8173_nor->base + MTK_NOR_CFG2_REG);
+	return readb_poll_timeout(mt8173_nor->base + MTK_NOR_CFG2_REG, reg,
+				  MTK_NOR_WR_BUF_DISABLE == (reg & 0x1), 100,
+				  10000);
+}
+
+static int mt8173_nor_erase_sector(struct spi_nor *nor, loff_t offset)
+{
+	int ret;
+	struct mt8173_nor *mt8173_nor = nor->priv;
+
+	ret = mt8173_nor_set_cmd(mt8173_nor, 0, 8, SPINOR_OP_WREN);
+	if (ret < 0)
+		return ret;
+
+	return mt8173_nor_set_cmd(mt8173_nor, (int)offset, 32, SPINOR_OP_BE_4K);
+}
+
+static int mt8173_nor_read(struct spi_nor *nor, loff_t from, size_t length,
+			   size_t *retlen, u_char *buffer)
+{
+	int i, ret;
+	int addr = (int)from;
+	u8 *buf = (u8 *)buffer;
+	struct mt8173_nor *mt8173_nor = nor->priv;
+
+	/* set mode for fast read mode ,dual mode or quad mode */
+	mt8173_nor_set_read_mode(mt8173_nor);
+	writeb((addr >> 24), mt8173_nor->base + MTK_NOR_RADR3_REG);
+	writeb((addr >> 16), mt8173_nor->base + MTK_NOR_RADR2_REG);
+	writeb((addr >> 8), mt8173_nor->base + MTK_NOR_RADR1_REG);
+	writeb(addr, mt8173_nor->base + MTK_NOR_RADR0_REG);
+
+	for (i = 0; i < length; i++, (*retlen)++) {
+		ret = mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_PIO_READ_CMD);
+		if (ret < 0)
+			return ret;
+		buf[i] = readb(mt8173_nor->base + MTK_NOR_RDATA_REG);
+	}
+	return 0;
+}
+
+static int mt8173_nor_write_single_byte(struct mt8173_nor *mt8173_nor,
+					int addr, int length, u8 *data)
+{
+	int i, ret;
+
+	writeb((addr >> 24), mt8173_nor->base + MTK_NOR_RADR3_REG);
+	writeb((addr >> 16), mt8173_nor->base + MTK_NOR_RADR2_REG);
+	writeb((addr >> 8), mt8173_nor->base + MTK_NOR_RADR1_REG);
+	writeb(addr, mt8173_nor->base + MTK_NOR_RADR0_REG);
+
+	for (i = 0; i < length; i++) {
+		ret = mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_PIO_WR_CMD);
+		if (ret < 0)
+			return ret;
+		writeb(*data++, mt8173_nor->base + MTK_NOR_WDATA_REG);
+	}
+	return 0;
+}
+
+static int mt8173_nor_write_buffer(struct mt8173_nor *mt8173_nor, int addr,
+				   const u8 *buf)
+{
+	int i, bufidx, data;
+
+	writeb((addr >> 24), mt8173_nor->base + MTK_NOR_RADR3_REG);
+	writeb((addr >> 16), mt8173_nor->base + MTK_NOR_RADR2_REG);
+	writeb((addr >> 8), mt8173_nor->base + MTK_NOR_RADR1_REG);
+	writeb(addr, mt8173_nor->base + MTK_NOR_RADR0_REG);
+
+	bufidx = 0;
+	for (i = 0; i < SFLASH_WRBUF_SIZE; i += 4) {
+		data = buf[bufidx + 3]<<24 | buf[bufidx + 2]<<16 |
+		       buf[bufidx + 1]<<8 | buf[bufidx];
+		bufidx += 4;
+		writel(data, mt8173_nor->base + MTK_NOR_PP_DATA_REG);
+	}
+	return mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_WR_CMD);
+}
+
+static void mt8173_nor_write(struct spi_nor *nor, loff_t to, size_t len,
+			     size_t *retlen, const u_char *buf)
+{
+	int ret;
+	struct mt8173_nor *mt8173_nor = nor->priv;
+
+	ret = mt8173_nor_write_buffer_enable(mt8173_nor);
+	if (ret < 0)
+		dev_warn(mt8173_nor->dev, "write buffer enable failed!\n");
+
+	while (len >= SFLASH_WRBUF_SIZE) {
+		ret = mt8173_nor_write_buffer(mt8173_nor, to, buf);
+		if (ret < 0)
+			dev_warn(mt8173_nor->dev, "write buffer failed!\n");
+		len -= SFLASH_WRBUF_SIZE;
+		to += SFLASH_WRBUF_SIZE;
+		buf += SFLASH_WRBUF_SIZE;
+		(*retlen) += SFLASH_WRBUF_SIZE;
+	}
+	ret = mt8173_nor_write_buffer_disable(mt8173_nor);
+	if (ret < 0)
+		dev_warn(mt8173_nor->dev, "write buffer disable failed!\n");
+
+	if (len) {
+		ret = mt8173_nor_write_single_byte(mt8173_nor, to, (int)len,
+						   (u8 *)buf);
+		if (ret < 0)
+			dev_warn(mt8173_nor->dev, "write single byte failed!\n");
+		(*retlen) += len;
+	}
+}
+
+static int mt8173_nor_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
+{
+	int ret;
+	struct mt8173_nor *mt8173_nor = nor->priv;
+
+	/* mtk nor controller doesn't supoort SPINOR_OP_RDCR */
+	switch (opcode) {
+	case SPINOR_OP_RDID:
+		/* read JEDEC ID need 4 bytes commands */
+		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 32, SPINOR_OP_RDID);
+		if (ret < 0)
+			return ret;
+
+		/* mtk nor flash controller only support 3 bytes IDs */
+		buf[2] = readb(mt8173_nor->base + MTK_NOR_SHREG0_REG);
+		buf[1] = readb(mt8173_nor->base + MTK_NOR_SHREG1_REG);
+		buf[0] = readb(mt8173_nor->base + MTK_NOR_SHREG2_REG);
+		break;
+	case SPINOR_OP_RDSR:
+		ret = mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_RDSR_CMD);
+		if (ret < 0)
+			return ret;
+		*buf = readb(mt8173_nor->base + MTK_NOR_RDSR_REG);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+static int mt8173_nor_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
+				int len)
+{
+	int ret;
+	struct mt8173_nor *mt8173_nor = nor->priv;
+
+	switch (opcode) {
+	case SPINOR_OP_WRSR:
+		ret = mt8173_nor_set_para(mt8173_nor, *buf,
+					  MTK_NOR_WRSR_CMD);
+		break;
+	case SPINOR_OP_CHIP_ERASE:
+		ret = mt8173_nor_set_para(mt8173_nor, opcode,
+					  MTK_NOR_PRG_CMD);
+		break;
+	case SPINOR_OP_WREN:
+		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 8, opcode);
+		if (ret)
+			dev_warn(mt8173_nor->dev, "set write enable fail!\n");
+		break;
+	case SPINOR_OP_WRDI:
+		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 8, opcode);
+		if (ret)
+			dev_warn(mt8173_nor->dev, "set write disable fail!\n");
+		break;
+	default:
+		dev_warn(mt8173_nor->dev, "doesn't support cmd %d\n", opcode);
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+static int __init mtk_nor_init(struct mt8173_nor *mt8173_nor,
+			       struct mtd_part_parser_data *ppdata)
+{
+	int ret;
+	struct spi_nor *nor;
+	struct mtd_info *mtd;
+
+	writel(MTK_NOR_ENABLE_SF_CMD, mt8173_nor->base + MTK_NOR_WRPROT_REG);
+	nor = &mt8173_nor->nor;
+	mtd = &mt8173_nor->mtd;
+	nor->mtd = *mtd;
+	nor->dev = mt8173_nor->dev;
+	nor->priv = mt8173_nor;
+	mtd->priv = nor;
+
+	/* fill the hooks to spi nor */
+	nor->read = mt8173_nor_read;
+	nor->read_reg = mt8173_nor_read_reg;
+	nor->write = mt8173_nor_write;
+	nor->write_reg = mt8173_nor_write_reg;
+	nor->erase = mt8173_nor_erase_sector;
+	nor->mtd.owner = THIS_MODULE;
+	nor->mtd.name = "mtk_nor";
+	/* initialized with NULL */
+	ret = spi_nor_scan(nor, NULL, SPI_NOR_DUAL);
+	if (ret)
+		return ret;
+
+	dev_dbg(mt8173_nor->dev, "mtd->size :0x%llx!\n", mtd->size);
+	return  mtd_device_parse_register(&nor->mtd, NULL, ppdata, NULL, 0);
+}
+
+static int mtk_nor_drv_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct mtd_part_parser_data ppdata;
+	struct resource *res;
+	int ret;
+	struct mt8173_nor *mt8173_nor;
+
+	if (!pdev->dev.of_node) {
+		dev_err(&pdev->dev, "No DT found\n");
+		return -EINVAL;
+	}
+
+	mt8173_nor = devm_kzalloc(&pdev->dev, sizeof(*mt8173_nor), GFP_KERNEL);
+	if (!mt8173_nor)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, mt8173_nor);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mt8173_nor->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(mt8173_nor->base))
+		return PTR_ERR(mt8173_nor->base);
+
+	mt8173_nor->spi_clk = devm_clk_get(&pdev->dev, "spi");
+	if (IS_ERR(mt8173_nor->spi_clk)) {
+		ret = PTR_ERR(mt8173_nor->spi_clk);
+		goto nor_free;
+	}
+
+	mt8173_nor->nor_clk = devm_clk_get(&pdev->dev, "sf");
+	if (IS_ERR(mt8173_nor->nor_clk)) {
+		ret = PTR_ERR(mt8173_nor->nor_clk);
+		goto nor_free;
+	}
+
+	mt8173_nor->dev = &pdev->dev;
+	clk_prepare_enable(mt8173_nor->spi_clk);
+	clk_prepare_enable(mt8173_nor->nor_clk);
+
+	ppdata.of_node = np;
+	ret = mtk_nor_init(mt8173_nor, &ppdata);
+
+nor_free:
+	return ret;
+}
+
+static int mtk_nor_drv_remove(struct platform_device *pdev)
+{
+	struct mt8173_nor *mt8173_nor = platform_get_drvdata(pdev);
+
+	mtd_device_unregister(&mt8173_nor->mtd);
+	clk_disable_unprepare(mt8173_nor->spi_clk);
+	clk_disable_unprepare(mt8173_nor->nor_clk);
+	return 0;
+}
+
+static const struct of_device_id mtk_nor_of_ids[] = {
+	{ .compatible = "mediatek,mt8173-nor"},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mtk_nor_of_ids);
+
+static struct platform_driver mtk_nor_driver = {
+	.probe = mtk_nor_drv_probe,
+	.remove = mtk_nor_drv_remove,
+	.driver = {
+		.name = "mtk-nor",
+		.of_match_table = mtk_nor_of_ids,
+	},
+};
+
+module_platform_driver(mtk_nor_driver);
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MediaTek SPI NOR Flash Driver");
-- 
1.8.1.1.dirty

Re: [PATCH v4 2/3] mtd: mtk-nor: mtk serial flash controller driver

[kbuild test robot]

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

Hi Bayi,

[auto build test ERROR on mtd/master -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Bayi-Cheng/Mediatek-SPI-NOR-flash-driver/20151013-174438
config: x86_64-allmodconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   drivers/mtd/spi-nor/mtk-quadspi.c:391:18: sparse: incorrect type in assignment (different base types)
   drivers/mtd/spi-nor/mtk-quadspi.c:391:18:    expected struct mtd_info *mtd
   drivers/mtd/spi-nor/mtk-quadspi.c:391:18:    got struct mtd_info <noident>
   drivers/mtd/spi-nor/mtk-quadspi.c:400:24: sparse: incorrect type in assignment (different argument counts)
   drivers/mtd/spi-nor/mtk-quadspi.c:400:24:    expected int ( *write_reg )( ... )
   drivers/mtd/spi-nor/mtk-quadspi.c:400:24:    got int ( static [toplevel] *<noident> )( ... )
   drivers/mtd/spi-nor/mtk-quadspi.c:402:17: sparse: expected structure or union
   drivers/mtd/spi-nor/mtk-quadspi.c:403:17: sparse: expected structure or union
   drivers/mtd/spi-nor/mtk-quadspi.c: In function 'mtk_nor_init':
>> drivers/mtd/spi-nor/mtk-quadspi.c:391:11: error: incompatible types when assigning to type 'struct mtd_info *' from type 'struct mtd_info'
     nor->mtd = *mtd;
              ^
>> drivers/mtd/spi-nor/mtk-quadspi.c:400:17: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types]
     nor->write_reg = mt8173_nor_write_reg;
                    ^
>> drivers/mtd/spi-nor/mtk-quadspi.c:402:10: error: request for member 'owner' in something not a structure or union
     nor->mtd.owner = THIS_MODULE;
             ^
>> drivers/mtd/spi-nor/mtk-quadspi.c:403:10: error: request for member 'name' in something not a structure or union
     nor->mtd.name = "mtk_nor";
             ^
   drivers/mtd/spi-nor/mtk-quadspi.c:410:36: warning: passing argument 1 of 'mtd_device_parse_register' from incompatible pointer type [-Wincompatible-pointer-types]
     return  mtd_device_parse_register(&nor->mtd, NULL, ppdata, NULL, 0);
                                       ^
   In file included from drivers/mtd/spi-nor/mtk-quadspi.c:24:0:
   include/linux/mtd/mtd.h:372:12: note: expected 'struct mtd_info *' but argument is of type 'struct mtd_info **'
    extern int mtd_device_parse_register(struct mtd_info *mtd,
               ^

sparse warnings: (new ones prefixed by >>)

   drivers/mtd/spi-nor/mtk-quadspi.c:391:18: sparse: incorrect type in assignment (different base types)
   drivers/mtd/spi-nor/mtk-quadspi.c:391:18:    expected struct mtd_info *mtd
   drivers/mtd/spi-nor/mtk-quadspi.c:391:18:    got struct mtd_info <noident>
>> drivers/mtd/spi-nor/mtk-quadspi.c:400:24: sparse: incorrect type in assignment (different argument counts)
   drivers/mtd/spi-nor/mtk-quadspi.c:400:24:    expected int ( *write_reg )( ... )
   drivers/mtd/spi-nor/mtk-quadspi.c:400:24:    got int ( static [toplevel] *<noident> )( ... )
   drivers/mtd/spi-nor/mtk-quadspi.c:402:17: sparse: expected structure or union
   drivers/mtd/spi-nor/mtk-quadspi.c:403:17: sparse: expected structure or union
   drivers/mtd/spi-nor/mtk-quadspi.c: In function 'mtk_nor_init':
   drivers/mtd/spi-nor/mtk-quadspi.c:391:11: error: incompatible types when assigning to type 'struct mtd_info *' from type 'struct mtd_info'
     nor->mtd = *mtd;
              ^
   drivers/mtd/spi-nor/mtk-quadspi.c:400:17: warning: assignment from incompatible pointer type [-Wincompatible-pointer-types]
     nor->write_reg = mt8173_nor_write_reg;
                    ^
   drivers/mtd/spi-nor/mtk-quadspi.c:402:10: error: request for member 'owner' in something not a structure or union
     nor->mtd.owner = THIS_MODULE;
             ^
   drivers/mtd/spi-nor/mtk-quadspi.c:403:10: error: request for member 'name' in something not a structure or union
     nor->mtd.name = "mtk_nor";
             ^
   drivers/mtd/spi-nor/mtk-quadspi.c:410:36: warning: passing argument 1 of 'mtd_device_parse_register' from incompatible pointer type [-Wincompatible-pointer-types]
     return  mtd_device_parse_register(&nor->mtd, NULL, ppdata, NULL, 0);
                                       ^
   In file included from drivers/mtd/spi-nor/mtk-quadspi.c:24:0:
   include/linux/mtd/mtd.h:372:12: note: expected 'struct mtd_info *' but argument is of type 'struct mtd_info **'
    extern int mtd_device_parse_register(struct mtd_info *mtd,
               ^

vim +391 drivers/mtd/spi-nor/mtk-quadspi.c

   385		struct spi_nor *nor;
   386		struct mtd_info *mtd;
   387	
   388		writel(MTK_NOR_ENABLE_SF_CMD, mt8173_nor->base + MTK_NOR_WRPROT_REG);
   389		nor = &mt8173_nor->nor;
   390		mtd = &mt8173_nor->mtd;
 > 391		nor->mtd = *mtd;
   392		nor->dev = mt8173_nor->dev;
   393		nor->priv = mt8173_nor;
   394		mtd->priv = nor;
   395	
   396		/* fill the hooks to spi nor */
   397		nor->read = mt8173_nor_read;
   398		nor->read_reg = mt8173_nor_read_reg;
   399		nor->write = mt8173_nor_write;
 > 400		nor->write_reg = mt8173_nor_write_reg;
   401		nor->erase = mt8173_nor_erase_sector;
 > 402		nor->mtd.owner = THIS_MODULE;
 > 403		nor->mtd.name = "mtk_nor";
   404		/* initialized with NULL */
   405		ret = spi_nor_scan(nor, NULL, SPI_NOR_DUAL);
   406		if (ret)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 49968 bytes --]

Re: [PATCH v4 2/3] mtd: mtk-nor: mtk serial flash controller driver

[Brian Norris]

Hi Bayi,

On Tue, Oct 13, 2015 at 05:39:19PM +0800, Bayi Cheng wrote:
> add spi nor flash driver for mediatek controller
> 
> Signed-off-by: Bayi Cheng <bayi.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/mtd/spi-nor/Kconfig       |   7 +
>  drivers/mtd/spi-nor/Makefile      |   1 +
>  drivers/mtd/spi-nor/mtk-quadspi.c | 486 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 494 insertions(+)
>  create mode 100644 drivers/mtd/spi-nor/mtk-quadspi.c
> 
> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> index 89bf4c1..f433890 100644
> --- a/drivers/mtd/spi-nor/Kconfig
> +++ b/drivers/mtd/spi-nor/Kconfig
> @@ -7,6 +7,13 @@ menuconfig MTD_SPI_NOR
>  
>  if MTD_SPI_NOR
>  
> +config MTD_MT81xx_NOR
> +	tristate "Support SPI flash Controller MTD_MT81xx_NOR"

You don't need to repeat the exact symbol name in the short description.
But you probably should include the name "MediaTek". How about this?

	tristate "MediaTek MT81xx SPI NOR flash controller"

> +	help
> +	  This enables access to SPI Nor flash, using MTD_MT81XX_NOR controller.

s/Nor/NOR/

And again, don't name the controller "MTD_MT81XX_NOR."

> +	  This controller does nor support generic SPI BUS, It only supports

s/nor/not/

> +	  SPI NOR Flash.
> +
>  config MTD_SPI_NOR_USE_4K_SECTORS
>  	bool "Use small 4096 B erase sectors"
>  	default y
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index e53333e..37c020a 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_MTD_MT81xx_NOR)	+= mtk-quadspi.o

Please include this between 'fsl-quadspi' and 'nxp-spifi' (i.e.,
alphabetical order).

>  obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
>  obj-$(CONFIG_SPI_FSL_QUADSPI)	+= fsl-quadspi.o
>  obj-$(CONFIG_SPI_NXP_SPIFI)	+= nxp-spifi.o
> diff --git a/drivers/mtd/spi-nor/mtk-quadspi.c b/drivers/mtd/spi-nor/mtk-quadspi.c
> new file mode 100644
> index 0000000..c6ac366
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/mtk-quadspi.c
> @@ -0,0 +1,486 @@
> +/*
> + * Copyright (c) 2015 MediaTek Inc.
> + * Author: Bayi Cheng <bayi.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/ioport.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/mtd/spi-nor.h>
> +
> +#define MTK_NOR_CMD_REG			0x00
> +#define MTK_NOR_CNT_REG			0x04
> +#define MTK_NOR_RDSR_REG		0x08
> +#define MTK_NOR_RDATA_REG		0x0c
> +#define MTK_NOR_RADR0_REG		0x10
> +#define MTK_NOR_RADR1_REG		0x14
> +#define MTK_NOR_RADR2_REG		0x18
> +#define MTK_NOR_WDATA_REG		0x1c
> +#define MTK_NOR_PRGDATA0_REG		0x20
> +#define MTK_NOR_PRGDATA1_REG		0x24
> +#define MTK_NOR_PRGDATA2_REG		0x28
> +#define MTK_NOR_PRGDATA3_REG		0x2c
> +#define MTK_NOR_PRGDATA4_REG		0x30
> +#define MTK_NOR_PRGDATA5_REG		0x34
> +#define MTK_NOR_SHREG0_REG		0x38
> +#define MTK_NOR_SHREG1_REG		0x3c
> +#define MTK_NOR_SHREG2_REG		0x40
> +#define MTK_NOR_SHREG3_REG		0x44
> +#define MTK_NOR_SHREG4_REG		0x48
> +#define MTK_NOR_SHREG5_REG		0x4c
> +#define MTK_NOR_SHREG6_REG		0x50
> +#define MTK_NOR_SHREG7_REG		0x54
> +#define MTK_NOR_SHREG8_REG		0x58
> +#define MTK_NOR_SHREG9_REG		0x5c
> +#define MTK_NOR_CFG1_REG		0x60
> +#define MTK_NOR_CFG2_REG		0x64
> +#define MTK_NOR_CFG3_REG		0x68
> +#define MTK_NOR_STATUS0_REG		0x70
> +#define MTK_NOR_STATUS1_REG		0x74
> +#define MTK_NOR_STATUS2_REG		0x78
> +#define MTK_NOR_STATUS3_REG		0x7c
> +#define MTK_NOR_FLHCFG_REG		0x84
> +#define MTK_NOR_TIME_REG		0x94
> +#define MTK_NOR_PP_DATA_REG		0x98
> +#define MTK_NOR_PREBUF_STUS_REG		0x9c
> +#define MTK_NOR_DELSEL0_REG		0xa0
> +#define MTK_NOR_DELSEL1_REG		0xa4
> +#define MTK_NOR_INTRSTUS_REG		0xa8
> +#define MTK_NOR_INTREN_REG		0xac
> +#define MTK_NOR_CHKSUM_CTL_REG		0xb8
> +#define MTK_NOR_CHKSUM_REG		0xbc
> +#define MTK_NOR_CMD2_REG		0xc0
> +#define MTK_NOR_WRPROT_REG		0xc4
> +#define MTK_NOR_RADR3_REG		0xc8
> +#define MTK_NOR_DUAL_REG		0xcc
> +#define MTK_NOR_DELSEL2_REG		0xd0
> +#define MTK_NOR_DELSEL3_REG		0xd4
> +#define MTK_NOR_DELSEL4_REG		0xd8
> +
> +/* commands for mtk nor controller */
> +#define MTK_NOR_READ_CMD		0x0
> +#define MTK_NOR_RDSR_CMD		0x2
> +#define MTK_NOR_PRG_CMD			0x4
> +#define MTK_NOR_WR_CMD			0x10
> +#define MTK_NOR_PIO_WR_CMD		0x90
> +#define MTK_NOR_WRSR_CMD		0x20
> +#define MTK_NOR_PIO_READ_CMD		0x81
> +#define MTK_NOR_WR_BUF_ENABLE		0x1
> +#define MTK_NOR_WR_BUF_DISABLE		0x0
> +#define MTK_NOR_ENABLE_SF_CMD		0x30
> +#define MTK_NOR_DUAD_ADDR_EN		0x8
> +#define MTK_NOR_QUAD_READ_EN		0x4
> +#define MTK_NOR_DUAL_ADDR_EN		0x2
> +#define MTK_NOR_DUAL_READ_EN		0x1
> +#define MTK_NOR_DUAL_DISABLE		0x0
> +#define MTK_NOR_FAST_READ		0x1
> +
> +#define SFLASH_WRBUF_SIZE		128
> +
> +struct mt8173_nor {
> +	struct mtd_info mtd;
> +	struct spi_nor nor;
> +	struct device *dev;
> +	void __iomem *base;	/* nor flash base address */
> +	struct clk *spi_clk;
> +	struct clk *nor_clk;
> +};
> +
> +static void mt8173_nor_set_read_mode(struct mt8173_nor *mt8173_nor)
> +{
> +	struct spi_nor *nor = &mt8173_nor->nor;
> +
> +	switch (nor->flash_read) {
> +	case SPI_NOR_FAST:
> +		writeb(SPINOR_OP_READ_FAST, mt8173_nor->base +

The SPINOR_OP_READ_xxx is just nor->read_opcode. Same for all the other
cases. So you can pull the writeb() out of the switch block.

> +		       MTK_NOR_PRGDATA3_REG);
> +		writeb(MTK_NOR_FAST_READ, mt8173_nor->base +
> +		       MTK_NOR_CFG1_REG);
> +		break;
> +	case SPI_NOR_DUAL:
> +		writeb(SPINOR_OP_READ_1_1_2, mt8173_nor->base +
> +		       MTK_NOR_PRGDATA3_REG);
> +		writeb(MTK_NOR_DUAL_READ_EN, mt8173_nor->base +
> +		       MTK_NOR_DUAL_REG);
> +		break;
> +	case SPI_NOR_QUAD:
> +		writeb(SPINOR_OP_READ_1_1_4, mt8173_nor->base +
> +		       MTK_NOR_PRGDATA3_REG);
> +		writeb(MTK_NOR_QUAD_READ_EN, mt8173_nor->base +
> +		       MTK_NOR_DUAL_REG);
> +		break;
> +	default:
> +		writeb(SPINOR_OP_READ, mt8173_nor->base +
> +		       MTK_NOR_PRGDATA3_REG);
> +		writeb(MTK_NOR_DUAL_DISABLE, mt8173_nor->base +
> +		       MTK_NOR_DUAL_REG);
> +		break;
> +	}
> +}
> +
> +static int mt8173_nor_execute_cmd(struct mt8173_nor *mt8173_nor, u8 cmdval)
> +{
> +	int reg;
> +	u8 val = cmdval & 0x1f;
> +
> +	writeb(cmdval, mt8173_nor->base + MTK_NOR_CMD_REG);
> +	return readl_poll_timeout(mt8173_nor->base + MTK_NOR_CMD_REG, reg,
> +				  !(reg & val), 100, 10000);
> +}
> +
> +static int mt8173_nor_set_cmd(struct mt8173_nor *mt8173_nor, int addr, u8 len,
> +			      u8 op)

This function protype looks all wrong, and it seems you're not using it
to its full potential either.

What you have is the ability to write an opcode and up to 5 bytes
following it. For several cases, 3 of those following it are just
address bytes. But what about 4-byte addressing (needed on larger
flash)? And what about opcodes you didn't prepare for?

So, I believe you can rewrite this function to be more like:

static int mt8173_nor_do_tx(struct mt8173_nor *mt8173_nor, u8 op, u8 *buf,
			    int len)
{
	int i;

	if (len > 5)
		return -EINVAL;

	writeb(op, mt8173_nor->base + MTK_NOR_PRGDATA5_REG);

	for (i = 0; i < len; i++)
		writeb(buf[i], mt8173_nor->base +
			       MTK_NOR_PRGDATA0_REG + 4 * (4 - i));
	writeb((len + 1) * 8, mt8173_nor->base + MTK_NOR_CNT_REG);
	return mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_PRG_CMD);
}

So existing uses like this:

		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 8, opcode);

can instead look like this:

		ret = mt8173_nor_do_tx(mt8173_nor, opcode, NULL, 0);

But you can also do more general write_reg() transfers with:

		ret = mt8173_nor_do_tx(mt8173_nor, opcode, buf, len);

I make similar comments below. I believe I've made these comments
previoulsy. If you have questions, please ask instead of just submitting
another version that doesn't really address my comment.

> +{
> +	writeb(op, mt8173_nor->base + MTK_NOR_PRGDATA5_REG);
> +	/*  send the address to nor flash
> +	 *  MTK_NOR_PRGDATA5_REG is shifted first
> +	 */
> +	if (len > 8) {
> +		writeb((addr >> 16), mt8173_nor->base + MTK_NOR_PRGDATA4_REG);
> +		writeb((addr >> 8), mt8173_nor->base + MTK_NOR_PRGDATA3_REG);
> +		writeb((addr), mt8173_nor->base + MTK_NOR_PRGDATA2_REG);
> +	}
> +	writeb(len, mt8173_nor->base + MTK_NOR_CNT_REG);
> +	return mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_PRG_CMD);
> +}
> +
> +/* cmd1 sent to nor flash, cmd2 write to nor controller */
> +static int mt8173_nor_set_para(struct mt8173_nor *mt8173_nor, int cmd1,
> +			       int cmd2)
> +{
> +	int ret;
> +
> +	ret = mt8173_nor_set_cmd(mt8173_nor, 0, 8, SPINOR_OP_WREN);
> +	if (ret < 0)
> +		return ret;

Why do you need this? You're just reimplementing write_enable() from
spi-nor.c, which should already get called in the right places. If it's
not, then fix that instead of hacking it in here.

Same applies in your erase function.

> +
> +	writeb(cmd1, mt8173_nor->base + MTK_NOR_PRGDATA5_REG);
> +	writeb(8, mt8173_nor->base + MTK_NOR_CNT_REG);
> +	return mt8173_nor_execute_cmd(mt8173_nor, cmd2);
> +}
> +
> +static int mt8173_nor_write_buffer_enable(struct mt8173_nor *mt8173_nor)
> +{
> +	u8 reg;
> +
> +	/* the bit0 of MTK_NOR_CFG2_REG is pre-fetch buffer
> +	 * 0: pre-fetch buffer use for read
> +	 * 1: pre-fetch buffer use for page program
> +	 */
> +	writel(MTK_NOR_WR_BUF_ENABLE, mt8173_nor->base + MTK_NOR_CFG2_REG);
> +	return readb_poll_timeout(mt8173_nor->base + MTK_NOR_CFG2_REG, reg,
> +				  0x01 == (reg & 0x01), 100, 10000);
> +}
> +
> +static int mt8173_nor_write_buffer_disable(struct mt8173_nor *mt8173_nor)
> +{
> +	u8 reg;
> +
> +	writel(MTK_NOR_WR_BUF_DISABLE, mt8173_nor->base + MTK_NOR_CFG2_REG);
> +	return readb_poll_timeout(mt8173_nor->base + MTK_NOR_CFG2_REG, reg,
> +				  MTK_NOR_WR_BUF_DISABLE == (reg & 0x1), 100,
> +				  10000);
> +}
> +
> +static int mt8173_nor_erase_sector(struct spi_nor *nor, loff_t offset)
> +{
> +	int ret;
> +	struct mt8173_nor *mt8173_nor = nor->priv;
> +
> +	ret = mt8173_nor_set_cmd(mt8173_nor, 0, 8, SPINOR_OP_WREN);
> +	if (ret < 0)
> +		return ret;

Same comment here. You shouldn't need the WREN.

> +
> +	return mt8173_nor_set_cmd(mt8173_nor, (int)offset, 32, SPINOR_OP_BE_4K);
> +}
> +
> +static int mt8173_nor_read(struct spi_nor *nor, loff_t from, size_t length,
> +			   size_t *retlen, u_char *buffer)
> +{
> +	int i, ret;
> +	int addr = (int)from;
> +	u8 *buf = (u8 *)buffer;
> +	struct mt8173_nor *mt8173_nor = nor->priv;
> +
> +	/* set mode for fast read mode ,dual mode or quad mode */
> +	mt8173_nor_set_read_mode(mt8173_nor);
> +	writeb((addr >> 24), mt8173_nor->base + MTK_NOR_RADR3_REG);
> +	writeb((addr >> 16), mt8173_nor->base + MTK_NOR_RADR2_REG);
> +	writeb((addr >> 8), mt8173_nor->base + MTK_NOR_RADR1_REG);
> +	writeb(addr, mt8173_nor->base + MTK_NOR_RADR0_REG);
> +
> +	for (i = 0; i < length; i++, (*retlen)++) {
> +		ret = mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_PIO_READ_CMD);
> +		if (ret < 0)
> +			return ret;
> +		buf[i] = readb(mt8173_nor->base + MTK_NOR_RDATA_REG);
> +	}
> +	return 0;
> +}
> +
> +static int mt8173_nor_write_single_byte(struct mt8173_nor *mt8173_nor,
> +					int addr, int length, u8 *data)
> +{
> +	int i, ret;
> +
> +	writeb((addr >> 24), mt8173_nor->base + MTK_NOR_RADR3_REG);
> +	writeb((addr >> 16), mt8173_nor->base + MTK_NOR_RADR2_REG);
> +	writeb((addr >> 8), mt8173_nor->base + MTK_NOR_RADR1_REG);
> +	writeb(addr, mt8173_nor->base + MTK_NOR_RADR0_REG);
> +
> +	for (i = 0; i < length; i++) {
> +		ret = mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_PIO_WR_CMD);
> +		if (ret < 0)
> +			return ret;
> +		writeb(*data++, mt8173_nor->base + MTK_NOR_WDATA_REG);
> +	}
> +	return 0;
> +}
> +
> +static int mt8173_nor_write_buffer(struct mt8173_nor *mt8173_nor, int addr,
> +				   const u8 *buf)
> +{
> +	int i, bufidx, data;
> +
> +	writeb((addr >> 24), mt8173_nor->base + MTK_NOR_RADR3_REG);
> +	writeb((addr >> 16), mt8173_nor->base + MTK_NOR_RADR2_REG);
> +	writeb((addr >> 8), mt8173_nor->base + MTK_NOR_RADR1_REG);
> +	writeb(addr, mt8173_nor->base + MTK_NOR_RADR0_REG);
> +
> +	bufidx = 0;
> +	for (i = 0; i < SFLASH_WRBUF_SIZE; i += 4) {
> +		data = buf[bufidx + 3]<<24 | buf[bufidx + 2]<<16 |
> +		       buf[bufidx + 1]<<8 | buf[bufidx];
> +		bufidx += 4;
> +		writel(data, mt8173_nor->base + MTK_NOR_PP_DATA_REG);
> +	}
> +	return mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_WR_CMD);
> +}
> +
> +static void mt8173_nor_write(struct spi_nor *nor, loff_t to, size_t len,
> +			     size_t *retlen, const u_char *buf)
> +{
> +	int ret;
> +	struct mt8173_nor *mt8173_nor = nor->priv;
> +
> +	ret = mt8173_nor_write_buffer_enable(mt8173_nor);
> +	if (ret < 0)
> +		dev_warn(mt8173_nor->dev, "write buffer enable failed!\n");
> +
> +	while (len >= SFLASH_WRBUF_SIZE) {
> +		ret = mt8173_nor_write_buffer(mt8173_nor, to, buf);
> +		if (ret < 0)
> +			dev_warn(mt8173_nor->dev, "write buffer failed!\n");
> +		len -= SFLASH_WRBUF_SIZE;
> +		to += SFLASH_WRBUF_SIZE;
> +		buf += SFLASH_WRBUF_SIZE;
> +		(*retlen) += SFLASH_WRBUF_SIZE;
> +	}
> +	ret = mt8173_nor_write_buffer_disable(mt8173_nor);
> +	if (ret < 0)
> +		dev_warn(mt8173_nor->dev, "write buffer disable failed!\n");
> +
> +	if (len) {
> +		ret = mt8173_nor_write_single_byte(mt8173_nor, to, (int)len,
> +						   (u8 *)buf);
> +		if (ret < 0)
> +			dev_warn(mt8173_nor->dev, "write single byte failed!\n");
> +		(*retlen) += len;
> +	}
> +}
> +
> +static int mt8173_nor_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
> +{
> +	int ret;
> +	struct mt8173_nor *mt8173_nor = nor->priv;
> +
> +	/* mtk nor controller doesn't supoort SPINOR_OP_RDCR */
> +	switch (opcode) {
> +	case SPINOR_OP_RDID:
> +		/* read JEDEC ID need 4 bytes commands */
> +		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 32, SPINOR_OP_RDID);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* mtk nor flash controller only support 3 bytes IDs */

Are you absolutely sure of this? That would be highly unfortunate, but
I also don't believe it's true.

> +		buf[2] = readb(mt8173_nor->base + MTK_NOR_SHREG0_REG);
> +		buf[1] = readb(mt8173_nor->base + MTK_NOR_SHREG1_REG);
> +		buf[0] = readb(mt8173_nor->base + MTK_NOR_SHREG2_REG);
> +		break;
> +	case SPINOR_OP_RDSR:
> +		ret = mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_RDSR_CMD);
> +		if (ret < 0)
> +			return ret;
> +		*buf = readb(mt8173_nor->base + MTK_NOR_RDSR_REG);
> +		break;
> +	default:
> +		ret = -EINVAL;

Why have you only implemented RDSR and RDID? What stops you from
supporting other opcodes here? It's a generic programming pattern, so
make this function generic. If you have limitations (e.g., you can only
read up to 6 bytes), then just make them explicit, with something like:

	/* Can only read out xxx bytes per command */
	if (len > xxx)
		return -EINVAL;

Really, I think you can just make a little modification to my sample
mt8173_nor_do_tx() function and make a mt8173_nor_do_rx() function that
will handle a few bytes of register read.

> +		break;
> +	}
> +	return ret;
> +}
> +
> +static int mt8173_nor_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
> +				int len)
> +{
> +	int ret;
> +	struct mt8173_nor *mt8173_nor = nor->priv;
> +
> +	switch (opcode) {
> +	case SPINOR_OP_WRSR:
> +		ret = mt8173_nor_set_para(mt8173_nor, *buf,
> +					  MTK_NOR_WRSR_CMD);
> +		break;
> +	case SPINOR_OP_CHIP_ERASE:
> +		ret = mt8173_nor_set_para(mt8173_nor, opcode,
> +					  MTK_NOR_PRG_CMD);
> +		break;
> +	case SPINOR_OP_WREN:

This case is the same as...

> +		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 8, opcode);
> +		if (ret)
> +			dev_warn(mt8173_nor->dev, "set write enable fail!\n");
> +		break;
> +	case SPINOR_OP_WRDI:

... this one.

So why duplicate them?

> +		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 8, opcode);
> +		if (ret)
> +			dev_warn(mt8173_nor->dev, "set write disable fail!\n");
> +		break;
> +	default:
> +		dev_warn(mt8173_nor->dev, "doesn't support cmd %d\n", opcode);
> +		ret = -EINVAL;

Again, why are you restricting support here? It looks like
mt8173_nor_set_cmd() can handle generic opcodes.

> +		break;
> +	}
> +	return ret;
> +}
> +
> +static int __init mtk_nor_init(struct mt8173_nor *mt8173_nor,
> +			       struct mtd_part_parser_data *ppdata)
> +{
> +	int ret;
> +	struct spi_nor *nor;
> +	struct mtd_info *mtd;
> +
> +	writel(MTK_NOR_ENABLE_SF_CMD, mt8173_nor->base + MTK_NOR_WRPROT_REG);
> +	nor = &mt8173_nor->nor;
> +	mtd = &mt8173_nor->mtd;
> +	nor->mtd = *mtd;

Yikes! I don't think you want to copy the entire struct here. Just drop
the 'mtd' field in struct mt8173_nor, since we now have the mtd_info
struct embedded inside struct spi_nor.

> +	nor->dev = mt8173_nor->dev;
> +	nor->priv = mt8173_nor;
> +	mtd->priv = nor;
> +
> +	/* fill the hooks to spi nor */
> +	nor->read = mt8173_nor_read;
> +	nor->read_reg = mt8173_nor_read_reg;
> +	nor->write = mt8173_nor_write;
> +	nor->write_reg = mt8173_nor_write_reg;
> +	nor->erase = mt8173_nor_erase_sector;
> +	nor->mtd.owner = THIS_MODULE;
> +	nor->mtd.name = "mtk_nor";
> +	/* initialized with NULL */
> +	ret = spi_nor_scan(nor, NULL, SPI_NOR_DUAL);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(mt8173_nor->dev, "mtd->size :0x%llx!\n", mtd->size);

Please remove this. This information is readily available in plenty of
other ways.

> +	return  mtd_device_parse_register(&nor->mtd, NULL, ppdata, NULL, 0);
> +}
> +
> +static int mtk_nor_drv_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct mtd_part_parser_data ppdata;
> +	struct resource *res;
> +	int ret;
> +	struct mt8173_nor *mt8173_nor;
> +
> +	if (!pdev->dev.of_node) {
> +		dev_err(&pdev->dev, "No DT found\n");
> +		return -EINVAL;
> +	}
> +
> +	mt8173_nor = devm_kzalloc(&pdev->dev, sizeof(*mt8173_nor), GFP_KERNEL);
> +	if (!mt8173_nor)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, mt8173_nor);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	mt8173_nor->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(mt8173_nor->base))
> +		return PTR_ERR(mt8173_nor->base);
> +
> +	mt8173_nor->spi_clk = devm_clk_get(&pdev->dev, "spi");
> +	if (IS_ERR(mt8173_nor->spi_clk)) {
> +		ret = PTR_ERR(mt8173_nor->spi_clk);
> +		goto nor_free;
> +	}
> +
> +	mt8173_nor->nor_clk = devm_clk_get(&pdev->dev, "sf");
> +	if (IS_ERR(mt8173_nor->nor_clk)) {
> +		ret = PTR_ERR(mt8173_nor->nor_clk);
> +		goto nor_free;
> +	}
> +
> +	mt8173_nor->dev = &pdev->dev;
> +	clk_prepare_enable(mt8173_nor->spi_clk);
> +	clk_prepare_enable(mt8173_nor->nor_clk);
> +
> +	ppdata.of_node = np;

On an earlier version, I suggested that you use a subnode to represent
the flash(es), even if you're only planning to ever use one flash. I
don't see that represented here though.

Also, please assign nor->flash_node somewhere in this init routine.

> +	ret = mtk_nor_init(mt8173_nor, &ppdata);
> +
> +nor_free:
> +	return ret;
> +}
> +
> +static int mtk_nor_drv_remove(struct platform_device *pdev)
> +{
> +	struct mt8173_nor *mt8173_nor = platform_get_drvdata(pdev);
> +
> +	mtd_device_unregister(&mt8173_nor->mtd);
> +	clk_disable_unprepare(mt8173_nor->spi_clk);
> +	clk_disable_unprepare(mt8173_nor->nor_clk);
> +	return 0;
> +}
> +
> +static const struct of_device_id mtk_nor_of_ids[] = {
> +	{ .compatible = "mediatek,mt8173-nor"},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mtk_nor_of_ids);
> +
> +static struct platform_driver mtk_nor_driver = {
> +	.probe = mtk_nor_drv_probe,
> +	.remove = mtk_nor_drv_remove,
> +	.driver = {
> +		.name = "mtk-nor",
> +		.of_match_table = mtk_nor_of_ids,
> +	},
> +};
> +
> +module_platform_driver(mtk_nor_driver);
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("MediaTek SPI NOR Flash Driver");

Brian
--
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 v4 2/3] mtd: mtk-nor: mtk serial flash controller driver

[bayi.cheng]

On Fri, 2015-10-16 at 00:39 -0700, Brian Norris wrote:
> Hi Bayi,
> 
> On Tue, Oct 13, 2015 at 05:39:19PM +0800, Bayi Cheng wrote:
> > add spi nor flash driver for mediatek controller
> > 
> > Signed-off-by: Bayi Cheng <bayi.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > ---
> >  drivers/mtd/spi-nor/Kconfig       |   7 +
> >  drivers/mtd/spi-nor/Makefile      |   1 +
> >  drivers/mtd/spi-nor/mtk-quadspi.c | 486 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 494 insertions(+)
> >  create mode 100644 drivers/mtd/spi-nor/mtk-quadspi.c
> > 
> > diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
> > index 89bf4c1..f433890 100644
> > --- a/drivers/mtd/spi-nor/Kconfig
> > +++ b/drivers/mtd/spi-nor/Kconfig
> > @@ -7,6 +7,13 @@ menuconfig MTD_SPI_NOR
> >  
> >  if MTD_SPI_NOR
> >  
> > +config MTD_MT81xx_NOR
> > +	tristate "Support SPI flash Controller MTD_MT81xx_NOR"
> 
> You don't need to repeat the exact symbol name in the short description.
> But you probably should include the name "MediaTek". How about this?
> 
> 	tristate "MediaTek MT81xx SPI NOR flash controller"
Hi, Brian, Sorry for later reply! 
and thanks for your advise!
> 
> > +	help
> > +	  This enables access to SPI Nor flash, using MTD_MT81XX_NOR controller.
> 
> s/Nor/NOR/
> 
> And again, don't name the controller "MTD_MT81XX_NOR."
OK, I will fix it.
> 
> > +	  This controller does nor support generic SPI BUS, It only supports
> 
> s/nor/not/

OK, I will fix it! Thanks a lot!
> 
> > +	  SPI NOR Flash.
> > +
> >  config MTD_SPI_NOR_USE_4K_SECTORS
> >  	bool "Use small 4096 B erase sectors"
> >  	default y
> > diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> > index e53333e..37c020a 100644
> > --- a/drivers/mtd/spi-nor/Makefile
> > +++ b/drivers/mtd/spi-nor/Makefile
> > @@ -1,3 +1,4 @@
> > +obj-$(CONFIG_MTD_MT81xx_NOR)	+= mtk-quadspi.o
> 
> Please include this between 'fsl-quadspi' and 'nxp-spifi' (i.e.,
> alphabetical order).

OK, I will fix it !
> 
> >  obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor.o
> >  obj-$(CONFIG_SPI_FSL_QUADSPI)	+= fsl-quadspi.o
> >  obj-$(CONFIG_SPI_NXP_SPIFI)	+= nxp-spifi.o
> > diff --git a/drivers/mtd/spi-nor/mtk-quadspi.c b/drivers/mtd/spi-nor/mtk-quadspi.c
> > new file mode 100644
> > index 0000000..c6ac366
> > --- /dev/null
> > +++ b/drivers/mtd/spi-nor/mtk-quadspi.c
> > @@ -0,0 +1,486 @@
> > +/*
> > + * Copyright (c) 2015 MediaTek Inc.
> > + * Author: Bayi Cheng <bayi.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/ioport.h>
> > +#include <linux/math64.h>
> > +#include <linux/module.h>
> > +#include <linux/mtd/mtd.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/mtd/mtd.h>
> > +#include <linux/mtd/partitions.h>
> > +#include <linux/mtd/spi-nor.h>
> > +
> > +#define MTK_NOR_CMD_REG			0x00
> > +#define MTK_NOR_CNT_REG			0x04
> > +#define MTK_NOR_RDSR_REG		0x08
> > +#define MTK_NOR_RDATA_REG		0x0c
> > +#define MTK_NOR_RADR0_REG		0x10
> > +#define MTK_NOR_RADR1_REG		0x14
> > +#define MTK_NOR_RADR2_REG		0x18
> > +#define MTK_NOR_WDATA_REG		0x1c
> > +#define MTK_NOR_PRGDATA0_REG		0x20
> > +#define MTK_NOR_PRGDATA1_REG		0x24
> > +#define MTK_NOR_PRGDATA2_REG		0x28
> > +#define MTK_NOR_PRGDATA3_REG		0x2c
> > +#define MTK_NOR_PRGDATA4_REG		0x30
> > +#define MTK_NOR_PRGDATA5_REG		0x34
> > +#define MTK_NOR_SHREG0_REG		0x38
> > +#define MTK_NOR_SHREG1_REG		0x3c
> > +#define MTK_NOR_SHREG2_REG		0x40
> > +#define MTK_NOR_SHREG3_REG		0x44
> > +#define MTK_NOR_SHREG4_REG		0x48
> > +#define MTK_NOR_SHREG5_REG		0x4c
> > +#define MTK_NOR_SHREG6_REG		0x50
> > +#define MTK_NOR_SHREG7_REG		0x54
> > +#define MTK_NOR_SHREG8_REG		0x58
> > +#define MTK_NOR_SHREG9_REG		0x5c
> > +#define MTK_NOR_CFG1_REG		0x60
> > +#define MTK_NOR_CFG2_REG		0x64
> > +#define MTK_NOR_CFG3_REG		0x68
> > +#define MTK_NOR_STATUS0_REG		0x70
> > +#define MTK_NOR_STATUS1_REG		0x74
> > +#define MTK_NOR_STATUS2_REG		0x78
> > +#define MTK_NOR_STATUS3_REG		0x7c
> > +#define MTK_NOR_FLHCFG_REG		0x84
> > +#define MTK_NOR_TIME_REG		0x94
> > +#define MTK_NOR_PP_DATA_REG		0x98
> > +#define MTK_NOR_PREBUF_STUS_REG		0x9c
> > +#define MTK_NOR_DELSEL0_REG		0xa0
> > +#define MTK_NOR_DELSEL1_REG		0xa4
> > +#define MTK_NOR_INTRSTUS_REG		0xa8
> > +#define MTK_NOR_INTREN_REG		0xac
> > +#define MTK_NOR_CHKSUM_CTL_REG		0xb8
> > +#define MTK_NOR_CHKSUM_REG		0xbc
> > +#define MTK_NOR_CMD2_REG		0xc0
> > +#define MTK_NOR_WRPROT_REG		0xc4
> > +#define MTK_NOR_RADR3_REG		0xc8
> > +#define MTK_NOR_DUAL_REG		0xcc
> > +#define MTK_NOR_DELSEL2_REG		0xd0
> > +#define MTK_NOR_DELSEL3_REG		0xd4
> > +#define MTK_NOR_DELSEL4_REG		0xd8
> > +
> > +/* commands for mtk nor controller */
> > +#define MTK_NOR_READ_CMD		0x0
> > +#define MTK_NOR_RDSR_CMD		0x2
> > +#define MTK_NOR_PRG_CMD			0x4
> > +#define MTK_NOR_WR_CMD			0x10
> > +#define MTK_NOR_PIO_WR_CMD		0x90
> > +#define MTK_NOR_WRSR_CMD		0x20
> > +#define MTK_NOR_PIO_READ_CMD		0x81
> > +#define MTK_NOR_WR_BUF_ENABLE		0x1
> > +#define MTK_NOR_WR_BUF_DISABLE		0x0
> > +#define MTK_NOR_ENABLE_SF_CMD		0x30
> > +#define MTK_NOR_DUAD_ADDR_EN		0x8
> > +#define MTK_NOR_QUAD_READ_EN		0x4
> > +#define MTK_NOR_DUAL_ADDR_EN		0x2
> > +#define MTK_NOR_DUAL_READ_EN		0x1
> > +#define MTK_NOR_DUAL_DISABLE		0x0
> > +#define MTK_NOR_FAST_READ		0x1
> > +
> > +#define SFLASH_WRBUF_SIZE		128
> > +
> > +struct mt8173_nor {
> > +	struct mtd_info mtd;
> > +	struct spi_nor nor;
> > +	struct device *dev;
> > +	void __iomem *base;	/* nor flash base address */
> > +	struct clk *spi_clk;
> > +	struct clk *nor_clk;
> > +};
> > +
> > +static void mt8173_nor_set_read_mode(struct mt8173_nor *mt8173_nor)
> > +{
> > +	struct spi_nor *nor = &mt8173_nor->nor;
> > +
> > +	switch (nor->flash_read) {
> > +	case SPI_NOR_FAST:
> > +		writeb(SPINOR_OP_READ_FAST, mt8173_nor->base +
> 
> The SPINOR_OP_READ_xxx is just nor->read_opcode. Same for all the other
> cases. So you can pull the writeb() out of the switch block.

OK, you are right, I will fix it !
> 
> > +		       MTK_NOR_PRGDATA3_REG);
> > +		writeb(MTK_NOR_FAST_READ, mt8173_nor->base +
> > +		       MTK_NOR_CFG1_REG);
> > +		break;
> > +	case SPI_NOR_DUAL:
> > +		writeb(SPINOR_OP_READ_1_1_2, mt8173_nor->base +
> > +		       MTK_NOR_PRGDATA3_REG);
> > +		writeb(MTK_NOR_DUAL_READ_EN, mt8173_nor->base +
> > +		       MTK_NOR_DUAL_REG);
> > +		break;
> > +	case SPI_NOR_QUAD:
> > +		writeb(SPINOR_OP_READ_1_1_4, mt8173_nor->base +
> > +		       MTK_NOR_PRGDATA3_REG);
> > +		writeb(MTK_NOR_QUAD_READ_EN, mt8173_nor->base +
> > +		       MTK_NOR_DUAL_REG);
> > +		break;
> > +	default:
> > +		writeb(SPINOR_OP_READ, mt8173_nor->base +
> > +		       MTK_NOR_PRGDATA3_REG);
> > +		writeb(MTK_NOR_DUAL_DISABLE, mt8173_nor->base +
> > +		       MTK_NOR_DUAL_REG);
> > +		break;
> > +	}
> > +}
> > +
> > +static int mt8173_nor_execute_cmd(struct mt8173_nor *mt8173_nor, u8 cmdval)
> > +{
> > +	int reg;
> > +	u8 val = cmdval & 0x1f;
> > +
> > +	writeb(cmdval, mt8173_nor->base + MTK_NOR_CMD_REG);
> > +	return readl_poll_timeout(mt8173_nor->base + MTK_NOR_CMD_REG, reg,
> > +				  !(reg & val), 100, 10000);
> > +}
> > +
> > +static int mt8173_nor_set_cmd(struct mt8173_nor *mt8173_nor, int addr, u8 len,
> > +			      u8 op)
> 
> This function protype looks all wrong, and it seems you're not using it
> to its full potential either.
> 
> What you have is the ability to write an opcode and up to 5 bytes
> following it. For several cases, 3 of those following it are just
> address bytes. But what about 4-byte addressing (needed on larger
> flash)? And what about opcodes you didn't prepare for?
> 
> So, I believe you can rewrite this function to be more like:
> 
> static int mt8173_nor_do_tx(struct mt8173_nor *mt8173_nor, u8 op, u8 *buf,
> 			    int len)
> {
> 	int i;
> 
> 	if (len > 5)
> 		return -EINVAL;
> 
> 	writeb(op, mt8173_nor->base + MTK_NOR_PRGDATA5_REG);
> 
> 	for (i = 0; i < len; i++)
> 		writeb(buf[i], mt8173_nor->base +
> 			       MTK_NOR_PRGDATA0_REG + 4 * (4 - i));
> 	writeb((len + 1) * 8, mt8173_nor->base + MTK_NOR_CNT_REG);
> 	return mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_PRG_CMD);
> }
> 
> So existing uses like this:
> 
> 		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 8, opcode);
> 
> can instead look like this:
> 
> 		ret = mt8173_nor_do_tx(mt8173_nor, opcode, NULL, 0);
> 
> But you can also do more general write_reg() transfers with:
> 
> 		ret = mt8173_nor_do_tx(mt8173_nor, opcode, buf, len);
> 
> I make similar comments below. I believe I've made these comments
> previoulsy. If you have questions, please ask instead of just submitting
> another version that doesn't really address my comment.
> 

Thank you very much for your detailed guidance, and i will fix it in
next patch!

> > +{
> > +	writeb(op, mt8173_nor->base + MTK_NOR_PRGDATA5_REG);
> > +	/*  send the address to nor flash
> > +	 *  MTK_NOR_PRGDATA5_REG is shifted first
> > +	 */
> > +	if (len > 8) {
> > +		writeb((addr >> 16), mt8173_nor->base + MTK_NOR_PRGDATA4_REG);
> > +		writeb((addr >> 8), mt8173_nor->base + MTK_NOR_PRGDATA3_REG);
> > +		writeb((addr), mt8173_nor->base + MTK_NOR_PRGDATA2_REG);
> > +	}
> > +	writeb(len, mt8173_nor->base + MTK_NOR_CNT_REG);
> > +	return mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_PRG_CMD);
> > +}
> > +
> > +/* cmd1 sent to nor flash, cmd2 write to nor controller */
> > +static int mt8173_nor_set_para(struct mt8173_nor *mt8173_nor, int cmd1,
> > +			       int cmd2)
> > +{
> > +	int ret;
> > +
> > +	ret = mt8173_nor_set_cmd(mt8173_nor, 0, 8, SPINOR_OP_WREN);
> > +	if (ret < 0)
> > +		return ret;
> 
> Why do you need this? You're just reimplementing write_enable() from
> spi-nor.c, which should already get called in the right places. If it's
> not, then fix that instead of hacking it in here.
> 
> Same applies in your erase function.
> 
Ok, I will remove this in next patch, Thanks for your advise!

> > +
> > +	writeb(cmd1, mt8173_nor->base + MTK_NOR_PRGDATA5_REG);
> > +	writeb(8, mt8173_nor->base + MTK_NOR_CNT_REG);
> > +	return mt8173_nor_execute_cmd(mt8173_nor, cmd2);
> > +}
> > +
> > +static int mt8173_nor_write_buffer_enable(struct mt8173_nor *mt8173_nor)
> > +{
> > +	u8 reg;
> > +
> > +	/* the bit0 of MTK_NOR_CFG2_REG is pre-fetch buffer
> > +	 * 0: pre-fetch buffer use for read
> > +	 * 1: pre-fetch buffer use for page program
> > +	 */
> > +	writel(MTK_NOR_WR_BUF_ENABLE, mt8173_nor->base + MTK_NOR_CFG2_REG);
> > +	return readb_poll_timeout(mt8173_nor->base + MTK_NOR_CFG2_REG, reg,
> > +				  0x01 == (reg & 0x01), 100, 10000);
> > +}
> > +
> > +static int mt8173_nor_write_buffer_disable(struct mt8173_nor *mt8173_nor)
> > +{
> > +	u8 reg;
> > +
> > +	writel(MTK_NOR_WR_BUF_DISABLE, mt8173_nor->base + MTK_NOR_CFG2_REG);
> > +	return readb_poll_timeout(mt8173_nor->base + MTK_NOR_CFG2_REG, reg,
> > +				  MTK_NOR_WR_BUF_DISABLE == (reg & 0x1), 100,
> > +				  10000);
> > +}
> > +
> > +static int mt8173_nor_erase_sector(struct spi_nor *nor, loff_t offset)
> > +{
> > +	int ret;
> > +	struct mt8173_nor *mt8173_nor = nor->priv;
> > +
> > +	ret = mt8173_nor_set_cmd(mt8173_nor, 0, 8, SPINOR_OP_WREN);
> > +	if (ret < 0)
> > +		return ret;
> 
> Same comment here. You shouldn't need the WREN.

OK, I will remove it.

> 
> > +
> > +	return mt8173_nor_set_cmd(mt8173_nor, (int)offset, 32, SPINOR_OP_BE_4K);
> > +}
> > +
> > +static int mt8173_nor_read(struct spi_nor *nor, loff_t from, size_t length,
> > +			   size_t *retlen, u_char *buffer)
> > +{
> > +	int i, ret;
> > +	int addr = (int)from;
> > +	u8 *buf = (u8 *)buffer;
> > +	struct mt8173_nor *mt8173_nor = nor->priv;
> > +
> > +	/* set mode for fast read mode ,dual mode or quad mode */
> > +	mt8173_nor_set_read_mode(mt8173_nor);
> > +	writeb((addr >> 24), mt8173_nor->base + MTK_NOR_RADR3_REG);
> > +	writeb((addr >> 16), mt8173_nor->base + MTK_NOR_RADR2_REG);
> > +	writeb((addr >> 8), mt8173_nor->base + MTK_NOR_RADR1_REG);
> > +	writeb(addr, mt8173_nor->base + MTK_NOR_RADR0_REG);
> > +
> > +	for (i = 0; i < length; i++, (*retlen)++) {
> > +		ret = mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_PIO_READ_CMD);
> > +		if (ret < 0)
> > +			return ret;
> > +		buf[i] = readb(mt8173_nor->base + MTK_NOR_RDATA_REG);
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int mt8173_nor_write_single_byte(struct mt8173_nor *mt8173_nor,
> > +					int addr, int length, u8 *data)
> > +{
> > +	int i, ret;
> > +
> > +	writeb((addr >> 24), mt8173_nor->base + MTK_NOR_RADR3_REG);
> > +	writeb((addr >> 16), mt8173_nor->base + MTK_NOR_RADR2_REG);
> > +	writeb((addr >> 8), mt8173_nor->base + MTK_NOR_RADR1_REG);
> > +	writeb(addr, mt8173_nor->base + MTK_NOR_RADR0_REG);
> > +
> > +	for (i = 0; i < length; i++) {
> > +		ret = mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_PIO_WR_CMD);
> > +		if (ret < 0)
> > +			return ret;
> > +		writeb(*data++, mt8173_nor->base + MTK_NOR_WDATA_REG);
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int mt8173_nor_write_buffer(struct mt8173_nor *mt8173_nor, int addr,
> > +				   const u8 *buf)
> > +{
> > +	int i, bufidx, data;
> > +
> > +	writeb((addr >> 24), mt8173_nor->base + MTK_NOR_RADR3_REG);
> > +	writeb((addr >> 16), mt8173_nor->base + MTK_NOR_RADR2_REG);
> > +	writeb((addr >> 8), mt8173_nor->base + MTK_NOR_RADR1_REG);
> > +	writeb(addr, mt8173_nor->base + MTK_NOR_RADR0_REG);
> > +
> > +	bufidx = 0;
> > +	for (i = 0; i < SFLASH_WRBUF_SIZE; i += 4) {
> > +		data = buf[bufidx + 3]<<24 | buf[bufidx + 2]<<16 |
> > +		       buf[bufidx + 1]<<8 | buf[bufidx];
> > +		bufidx += 4;
> > +		writel(data, mt8173_nor->base + MTK_NOR_PP_DATA_REG);
> > +	}
> > +	return mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_WR_CMD);
> > +}
> > +
> > +static void mt8173_nor_write(struct spi_nor *nor, loff_t to, size_t len,
> > +			     size_t *retlen, const u_char *buf)
> > +{
> > +	int ret;
> > +	struct mt8173_nor *mt8173_nor = nor->priv;
> > +
> > +	ret = mt8173_nor_write_buffer_enable(mt8173_nor);
> > +	if (ret < 0)
> > +		dev_warn(mt8173_nor->dev, "write buffer enable failed!\n");
> > +
> > +	while (len >= SFLASH_WRBUF_SIZE) {
> > +		ret = mt8173_nor_write_buffer(mt8173_nor, to, buf);
> > +		if (ret < 0)
> > +			dev_warn(mt8173_nor->dev, "write buffer failed!\n");
> > +		len -= SFLASH_WRBUF_SIZE;
> > +		to += SFLASH_WRBUF_SIZE;
> > +		buf += SFLASH_WRBUF_SIZE;
> > +		(*retlen) += SFLASH_WRBUF_SIZE;
> > +	}
> > +	ret = mt8173_nor_write_buffer_disable(mt8173_nor);
> > +	if (ret < 0)
> > +		dev_warn(mt8173_nor->dev, "write buffer disable failed!\n");
> > +
> > +	if (len) {
> > +		ret = mt8173_nor_write_single_byte(mt8173_nor, to, (int)len,
> > +						   (u8 *)buf);
> > +		if (ret < 0)
> > +			dev_warn(mt8173_nor->dev, "write single byte failed!\n");
> > +		(*retlen) += len;
> > +	}
> > +}
> > +
> > +static int mt8173_nor_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
> > +{
> > +	int ret;
> > +	struct mt8173_nor *mt8173_nor = nor->priv;
> > +
> > +	/* mtk nor controller doesn't supoort SPINOR_OP_RDCR */
> > +	switch (opcode) {
> > +	case SPINOR_OP_RDID:
> > +		/* read JEDEC ID need 4 bytes commands */
> > +		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 32, SPINOR_OP_RDID);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		/* mtk nor flash controller only support 3 bytes IDs */
> 
> Are you absolutely sure of this? That would be highly unfortunate, but
> I also don't believe it's true.
> 
Yes, for this issue I have asked our designer of nor flash controller,
unfortunately, it is true, and I have tried to read more IDs, but our
controller just accept 3 IDs from nor flash, and our next generation IC
may solve this problem.


> > +		buf[2] = readb(mt8173_nor->base + MTK_NOR_SHREG0_REG);
> > +		buf[1] = readb(mt8173_nor->base + MTK_NOR_SHREG1_REG);
> > +		buf[0] = readb(mt8173_nor->base + MTK_NOR_SHREG2_REG);
> > +		break;
> > +	case SPINOR_OP_RDSR:
> > +		ret = mt8173_nor_execute_cmd(mt8173_nor, MTK_NOR_RDSR_CMD);
> > +		if (ret < 0)
> > +			return ret;
> > +		*buf = readb(mt8173_nor->base + MTK_NOR_RDSR_REG);
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> 
> Why have you only implemented RDSR and RDID? What stops you from
> supporting other opcodes here? It's a generic programming pattern, so
> make this function generic. If you have limitations (e.g., you can only
> read up to 6 bytes), then just make them explicit, with something like:
> 
> 	/* Can only read out xxx bytes per command */
> 	if (len > xxx)
> 		return -EINVAL;
> 
> Really, I think you can just make a little modification to my sample
> mt8173_nor_do_tx() function and make a mt8173_nor_do_rx() function that
> will handle a few bytes of register read.

OK, Thanks for your detailed guidance! I will fix it in the next patch
> 
> > +		break;
> > +	}
> > +	return ret;
> > +}
> > +
> > +static int mt8173_nor_write_reg(struct spi_nor *nor, u8 opcode, u8 *buf,
> > +				int len)
> > +{
> > +	int ret;
> > +	struct mt8173_nor *mt8173_nor = nor->priv;
> > +
> > +	switch (opcode) {
> > +	case SPINOR_OP_WRSR:
> > +		ret = mt8173_nor_set_para(mt8173_nor, *buf,
> > +					  MTK_NOR_WRSR_CMD);
> > +		break;
> > +	case SPINOR_OP_CHIP_ERASE:
> > +		ret = mt8173_nor_set_para(mt8173_nor, opcode,
> > +					  MTK_NOR_PRG_CMD);
> > +		break;
> > +	case SPINOR_OP_WREN:
> 
> This case is the same as...
> 
> > +		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 8, opcode);
> > +		if (ret)
> > +			dev_warn(mt8173_nor->dev, "set write enable fail!\n");
> > +		break;
> > +	case SPINOR_OP_WRDI:
> 
> ... this one.
> 
> So why duplicate them?

Sorry, It is my fault, I will remove one of them.
> 
> > +		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 8, opcode);
> > +		if (ret)
> > +			dev_warn(mt8173_nor->dev, "set write disable fail!\n");
> > +		break;
> > +	default:
> > +		dev_warn(mt8173_nor->dev, "doesn't support cmd %d\n", opcode);
> > +		ret = -EINVAL;
> 
> Again, why are you restricting support here? It looks like
> mt8173_nor_set_cmd() can handle generic opcodes.

OK, I will fix it in the next patch.
> 
> > +		break;
> > +	}
> > +	return ret;
> > +}
> > +
> > +static int __init mtk_nor_init(struct mt8173_nor *mt8173_nor,
> > +			       struct mtd_part_parser_data *ppdata)
> > +{
> > +	int ret;
> > +	struct spi_nor *nor;
> > +	struct mtd_info *mtd;
> > +
> > +	writel(MTK_NOR_ENABLE_SF_CMD, mt8173_nor->base + MTK_NOR_WRPROT_REG);
> > +	nor = &mt8173_nor->nor;
> > +	mtd = &mt8173_nor->mtd;
> > +	nor->mtd = *mtd;
> 
> Yikes! I don't think you want to copy the entire struct here. Just drop
> the 'mtd' field in struct mt8173_nor, since we now have the mtd_info
> struct embedded inside struct spi_nor.

OK, I will fix it, and thanks for your advise! 
> 
> > +	nor->dev = mt8173_nor->dev;
> > +	nor->priv = mt8173_nor;
> > +	mtd->priv = nor;
> > +
> > +	/* fill the hooks to spi nor */
> > +	nor->read = mt8173_nor_read;
> > +	nor->read_reg = mt8173_nor_read_reg;
> > +	nor->write = mt8173_nor_write;
> > +	nor->write_reg = mt8173_nor_write_reg;
> > +	nor->erase = mt8173_nor_erase_sector;
> > +	nor->mtd.owner = THIS_MODULE;
> > +	nor->mtd.name = "mtk_nor";
> > +	/* initialized with NULL */
> > +	ret = spi_nor_scan(nor, NULL, SPI_NOR_DUAL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dev_dbg(mt8173_nor->dev, "mtd->size :0x%llx!\n", mtd->size);
> 
> Please remove this. This information is readily available in plenty of
> other ways.

OK, I will it in the next patch!
> 
> > +	return  mtd_device_parse_register(&nor->mtd, NULL, ppdata, NULL, 0);
> > +}
> > +
> > +static int mtk_nor_drv_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct mtd_part_parser_data ppdata;
> > +	struct resource *res;
> > +	int ret;
> > +	struct mt8173_nor *mt8173_nor;
> > +
> > +	if (!pdev->dev.of_node) {
> > +		dev_err(&pdev->dev, "No DT found\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	mt8173_nor = devm_kzalloc(&pdev->dev, sizeof(*mt8173_nor), GFP_KERNEL);
> > +	if (!mt8173_nor)
> > +		return -ENOMEM;
> > +	platform_set_drvdata(pdev, mt8173_nor);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	mt8173_nor->base = devm_ioremap_resource(&pdev->dev, res);
> > +	if (IS_ERR(mt8173_nor->base))
> > +		return PTR_ERR(mt8173_nor->base);
> > +
> > +	mt8173_nor->spi_clk = devm_clk_get(&pdev->dev, "spi");
> > +	if (IS_ERR(mt8173_nor->spi_clk)) {
> > +		ret = PTR_ERR(mt8173_nor->spi_clk);
> > +		goto nor_free;
> > +	}
> > +
> > +	mt8173_nor->nor_clk = devm_clk_get(&pdev->dev, "sf");
> > +	if (IS_ERR(mt8173_nor->nor_clk)) {
> > +		ret = PTR_ERR(mt8173_nor->nor_clk);
> > +		goto nor_free;
> > +	}
> > +
> > +	mt8173_nor->dev = &pdev->dev;
> > +	clk_prepare_enable(mt8173_nor->spi_clk);
> > +	clk_prepare_enable(mt8173_nor->nor_clk);
> > +
> > +	ppdata.of_node = np;
> 
> On an earlier version, I suggested that you use a subnode to represent
> the flash(es), even if you're only planning to ever use one flash. I
> don't see that represented here though.
> 
> Also, please assign nor->flash_node somewhere in this init routine.

OK, I will fix it in the next patch! Thank you very much for your
detailed advise !
> 
> > +	ret = mtk_nor_init(mt8173_nor, &ppdata);
> > +
> > +nor_free:
> > +	return ret;
> > +}
> > +
> > +static int mtk_nor_drv_remove(struct platform_device *pdev)
> > +{
> > +	struct mt8173_nor *mt8173_nor = platform_get_drvdata(pdev);
> > +
> > +	mtd_device_unregister(&mt8173_nor->mtd);
> > +	clk_disable_unprepare(mt8173_nor->spi_clk);
> > +	clk_disable_unprepare(mt8173_nor->nor_clk);
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id mtk_nor_of_ids[] = {
> > +	{ .compatible = "mediatek,mt8173-nor"},
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_nor_of_ids);
> > +
> > +static struct platform_driver mtk_nor_driver = {
> > +	.probe = mtk_nor_drv_probe,
> > +	.remove = mtk_nor_drv_remove,
> > +	.driver = {
> > +		.name = "mtk-nor",
> > +		.of_match_table = mtk_nor_of_ids,
> > +	},
> > +};
> > +
> > +module_platform_driver(mtk_nor_driver);
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("MediaTek SPI NOR Flash Driver");
> 
> Brian

Re: [PATCH v4 2/3] mtd: mtk-nor: mtk serial flash controller driver

[Brian Norris]

Hi Bayi,

I'm looking over your v5, and I still have a lot of comments. I'll send
those soon. But I still question one of your responses here:

On Sun, Oct 18, 2015 at 10:20:35PM +0800, bayi.cheng wrote:
> On Fri, 2015-10-16 at 00:39 -0700, Brian Norris wrote:
> > On Tue, Oct 13, 2015 at 05:39:19PM +0800, Bayi Cheng wrote:

> > > diff --git a/drivers/mtd/spi-nor/mtk-quadspi.c b/drivers/mtd/spi-nor/mtk-quadspi.c
> > > new file mode 100644
> > > index 0000000..c6ac366
> > > --- /dev/null
> > > +++ b/drivers/mtd/spi-nor/mtk-quadspi.c
> > > @@ -0,0 +1,486 @@
[...]
> > > +#define MTK_NOR_CMD_REG			0x00
> > > +#define MTK_NOR_CNT_REG			0x04
> > > +#define MTK_NOR_RDSR_REG		0x08
> > > +#define MTK_NOR_RDATA_REG		0x0c
> > > +#define MTK_NOR_RADR0_REG		0x10
> > > +#define MTK_NOR_RADR1_REG		0x14
> > > +#define MTK_NOR_RADR2_REG		0x18
> > > +#define MTK_NOR_WDATA_REG		0x1c
> > > +#define MTK_NOR_PRGDATA0_REG		0x20
> > > +#define MTK_NOR_PRGDATA1_REG		0x24
> > > +#define MTK_NOR_PRGDATA2_REG		0x28
> > > +#define MTK_NOR_PRGDATA3_REG		0x2c
> > > +#define MTK_NOR_PRGDATA4_REG		0x30
> > > +#define MTK_NOR_PRGDATA5_REG		0x34

^^ so you have 6 "TX" registers.

> > > +#define MTK_NOR_SHREG0_REG		0x38
> > > +#define MTK_NOR_SHREG1_REG		0x3c
> > > +#define MTK_NOR_SHREG2_REG		0x40
> > > +#define MTK_NOR_SHREG3_REG		0x44
> > > +#define MTK_NOR_SHREG4_REG		0x48
> > > +#define MTK_NOR_SHREG5_REG		0x4c

^^ and you have at least 6 "RX" registers (looking at the mt8173 manual,
I'm not sure what the SHREG{6..9} are for). So I don't see why you can't
program MTK_NOR_CNT_REG to 48 (6 bytes * 8 bits/byte) to get 6 bytes
TX/RX, or IOW, 1 byte of opcode and 5 bytes of either RX or TX data. So
I'm still not convinced about your claim below.

I was able to rewrite your driver to do the above, and I can read out 5
bytes no problem, but unfortunately, the flash device I have has only 3
bytes of ID, so the last two bytes are zero, which doesn't tell me too
much if this is working right.

> > > +#define MTK_NOR_SHREG6_REG		0x50
> > > +#define MTK_NOR_SHREG7_REG		0x54
> > > +#define MTK_NOR_SHREG8_REG		0x58
> > > +#define MTK_NOR_SHREG9_REG		0x5c
> > > +#define MTK_NOR_CFG1_REG		0x60
> > > +#define MTK_NOR_CFG2_REG		0x64
> > > +#define MTK_NOR_CFG3_REG		0x68
> > > +#define MTK_NOR_STATUS0_REG		0x70
> > > +#define MTK_NOR_STATUS1_REG		0x74
> > > +#define MTK_NOR_STATUS2_REG		0x78
> > > +#define MTK_NOR_STATUS3_REG		0x7c
> > > +#define MTK_NOR_FLHCFG_REG		0x84
> > > +#define MTK_NOR_TIME_REG		0x94
> > > +#define MTK_NOR_PP_DATA_REG		0x98
> > > +#define MTK_NOR_PREBUF_STUS_REG		0x9c
> > > +#define MTK_NOR_DELSEL0_REG		0xa0
> > > +#define MTK_NOR_DELSEL1_REG		0xa4
> > > +#define MTK_NOR_INTRSTUS_REG		0xa8
> > > +#define MTK_NOR_INTREN_REG		0xac
> > > +#define MTK_NOR_CHKSUM_CTL_REG		0xb8
> > > +#define MTK_NOR_CHKSUM_REG		0xbc
> > > +#define MTK_NOR_CMD2_REG		0xc0
> > > +#define MTK_NOR_WRPROT_REG		0xc4
> > > +#define MTK_NOR_RADR3_REG		0xc8
> > > +#define MTK_NOR_DUAL_REG		0xcc
> > > +#define MTK_NOR_DELSEL2_REG		0xd0
> > > +#define MTK_NOR_DELSEL3_REG		0xd4
> > > +#define MTK_NOR_DELSEL4_REG		0xd8

...

> > > +static int mt8173_nor_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
> > > +{
> > > +	int ret;
> > > +	struct mt8173_nor *mt8173_nor = nor->priv;
> > > +
> > > +	/* mtk nor controller doesn't supoort SPINOR_OP_RDCR */
> > > +	switch (opcode) {
> > > +	case SPINOR_OP_RDID:
> > > +		/* read JEDEC ID need 4 bytes commands */
> > > +		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 32, SPINOR_OP_RDID);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		/* mtk nor flash controller only support 3 bytes IDs */
> > 
> > Are you absolutely sure of this? That would be highly unfortunate, but
> > I also don't believe it's true.
> > 
> Yes, for this issue I have asked our designer of nor flash controller,
> unfortunately, it is true, and I have tried to read more IDs, but our
> controller just accept 3 IDs from nor flash, and our next generation IC
> may solve this problem.

How exactly did you try? Did you do what I suggested above? Are the
"shift registers" not all actually functional?

> > > +		buf[2] = readb(mt8173_nor->base + MTK_NOR_SHREG0_REG);
> > > +		buf[1] = readb(mt8173_nor->base + MTK_NOR_SHREG1_REG);
> > > +		buf[0] = readb(mt8173_nor->base + MTK_NOR_SHREG2_REG);
> > > +		break;

[...]

Brian

Re: [PATCH v4 2/3] mtd: mtk-nor: mtk serial flash controller driver

[bayi.cheng]

On Wed, 2015-10-28 at 18:52 -0700, Brian Norris wrote:
> Hi Bayi,
> 
> I'm looking over your v5, and I still have a lot of comments. I'll send
> those soon. But I still question one of your responses here:
> 
> On Sun, Oct 18, 2015 at 10:20:35PM +0800, bayi.cheng wrote:
> > On Fri, 2015-10-16 at 00:39 -0700, Brian Norris wrote:
> > > On Tue, Oct 13, 2015 at 05:39:19PM +0800, Bayi Cheng wrote:
> 
> > > > diff --git a/drivers/mtd/spi-nor/mtk-quadspi.c b/drivers/mtd/spi-nor/mtk-quadspi.c
> > > > new file mode 100644
> > > > index 0000000..c6ac366
> > > > --- /dev/null
> > > > +++ b/drivers/mtd/spi-nor/mtk-quadspi.c
> > > > @@ -0,0 +1,486 @@
> [...]
> > > > +#define MTK_NOR_CMD_REG			0x00
> > > > +#define MTK_NOR_CNT_REG			0x04
> > > > +#define MTK_NOR_RDSR_REG		0x08
> > > > +#define MTK_NOR_RDATA_REG		0x0c
> > > > +#define MTK_NOR_RADR0_REG		0x10
> > > > +#define MTK_NOR_RADR1_REG		0x14
> > > > +#define MTK_NOR_RADR2_REG		0x18
> > > > +#define MTK_NOR_WDATA_REG		0x1c
> > > > +#define MTK_NOR_PRGDATA0_REG		0x20
> > > > +#define MTK_NOR_PRGDATA1_REG		0x24
> > > > +#define MTK_NOR_PRGDATA2_REG		0x28
> > > > +#define MTK_NOR_PRGDATA3_REG		0x2c
> > > > +#define MTK_NOR_PRGDATA4_REG		0x30
> > > > +#define MTK_NOR_PRGDATA5_REG		0x34
> 
> ^^ so you have 6 "TX" registers.
> 
> > > > +#define MTK_NOR_SHREG0_REG		0x38
> > > > +#define MTK_NOR_SHREG1_REG		0x3c
> > > > +#define MTK_NOR_SHREG2_REG		0x40
> > > > +#define MTK_NOR_SHREG3_REG		0x44
> > > > +#define MTK_NOR_SHREG4_REG		0x48
> > > > +#define MTK_NOR_SHREG5_REG		0x4c
> 
> ^^ and you have at least 6 "RX" registers (looking at the mt8173 manual,
> I'm not sure what the SHREG{6..9} are for). So I don't see why you can't
> program MTK_NOR_CNT_REG to 48 (6 bytes * 8 bits/byte) to get 6 bytes
> TX/RX, or IOW, 1 byte of opcode and 5 bytes of either RX or TX data. So
> I'm still not convinced about your claim below.
> 
> I was able to rewrite your driver to do the above, and I can read out 5
> bytes no problem, but unfortunately, the flash device I have has only 3
> bytes of ID, so the last two bytes are zero, which doesn't tell me too
> much if this is working right.
> 
> > > > +#define MTK_NOR_SHREG6_REG		0x50
> > > > +#define MTK_NOR_SHREG7_REG		0x54
> > > > +#define MTK_NOR_SHREG8_REG		0x58
> > > > +#define MTK_NOR_SHREG9_REG		0x5c
> > > > +#define MTK_NOR_CFG1_REG		0x60
> > > > +#define MTK_NOR_CFG2_REG		0x64
> > > > +#define MTK_NOR_CFG3_REG		0x68
> > > > +#define MTK_NOR_STATUS0_REG		0x70
> > > > +#define MTK_NOR_STATUS1_REG		0x74
> > > > +#define MTK_NOR_STATUS2_REG		0x78
> > > > +#define MTK_NOR_STATUS3_REG		0x7c
> > > > +#define MTK_NOR_FLHCFG_REG		0x84
> > > > +#define MTK_NOR_TIME_REG		0x94
> > > > +#define MTK_NOR_PP_DATA_REG		0x98
> > > > +#define MTK_NOR_PREBUF_STUS_REG		0x9c
> > > > +#define MTK_NOR_DELSEL0_REG		0xa0
> > > > +#define MTK_NOR_DELSEL1_REG		0xa4
> > > > +#define MTK_NOR_INTRSTUS_REG		0xa8
> > > > +#define MTK_NOR_INTREN_REG		0xac
> > > > +#define MTK_NOR_CHKSUM_CTL_REG		0xb8
> > > > +#define MTK_NOR_CHKSUM_REG		0xbc
> > > > +#define MTK_NOR_CMD2_REG		0xc0
> > > > +#define MTK_NOR_WRPROT_REG		0xc4
> > > > +#define MTK_NOR_RADR3_REG		0xc8
> > > > +#define MTK_NOR_DUAL_REG		0xcc
> > > > +#define MTK_NOR_DELSEL2_REG		0xd0
> > > > +#define MTK_NOR_DELSEL3_REG		0xd4
> > > > +#define MTK_NOR_DELSEL4_REG		0xd8
> 
> ...
> 
> > > > +static int mt8173_nor_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
> > > > +{
> > > > +	int ret;
> > > > +	struct mt8173_nor *mt8173_nor = nor->priv;
> > > > +
> > > > +	/* mtk nor controller doesn't supoort SPINOR_OP_RDCR */
> > > > +	switch (opcode) {
> > > > +	case SPINOR_OP_RDID:
> > > > +		/* read JEDEC ID need 4 bytes commands */
> > > > +		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 32, SPINOR_OP_RDID);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +
> > > > +		/* mtk nor flash controller only support 3 bytes IDs */
> > > 
> > > Are you absolutely sure of this? That would be highly unfortunate, but
> > > I also don't believe it's true.
> > > 
> > Yes, for this issue I have asked our designer of nor flash controller,
> > unfortunately, it is true, and I have tried to read more IDs, but our
> > controller just accept 3 IDs from nor flash, and our next generation IC
> > may solve this problem.
> 
> How exactly did you try? Did you do what I suggested above? Are the
> "shift registers" not all actually functional?
> 
Hi Brian, For this problem, I have asked our nor controller designer to
double confirm again, and he promise to do some simulation testes, On
the other hand, I have got some Spansion nor flash which has 5 IDs, and
after our designer make sure our controller can support 5 or 6 IDs, then
I will rework a special platform with S25FL256SA. If we have any
progress, I will inform you immediately!  Thanks!!

> > > > +		buf[2] = readb(mt8173_nor->base + MTK_NOR_SHREG0_REG);
> > > > +		buf[1] = readb(mt8173_nor->base + MTK_NOR_SHREG1_REG);
> > > > +		buf[0] = readb(mt8173_nor->base + MTK_NOR_SHREG2_REG);
> > > > +		break;
> 
> [...]
> 
> Brian

Re: [PATCH v4 2/3] mtd: mtk-nor: mtk serial flash controller driver

[bayi.cheng]

On Thu, 2015-10-29 at 11:28 +0800, bayi.cheng wrote:
> On Wed, 2015-10-28 at 18:52 -0700, Brian Norris wrote:
> > Hi Bayi,
> > 
> > I'm looking over your v5, and I still have a lot of comments. I'll send
> > those soon. But I still question one of your responses here:
> > 
> > On Sun, Oct 18, 2015 at 10:20:35PM +0800, bayi.cheng wrote:
> > > On Fri, 2015-10-16 at 00:39 -0700, Brian Norris wrote:
> > > > On Tue, Oct 13, 2015 at 05:39:19PM +0800, Bayi Cheng wrote:
> > 
> > > > > diff --git a/drivers/mtd/spi-nor/mtk-quadspi.c b/drivers/mtd/spi-nor/mtk-quadspi.c
> > > > > new file mode 100644
> > > > > index 0000000..c6ac366
> > > > > --- /dev/null
> > > > > +++ b/drivers/mtd/spi-nor/mtk-quadspi.c
> > > > > @@ -0,0 +1,486 @@
> > [...]
> > > > > +#define MTK_NOR_CMD_REG			0x00
> > > > > +#define MTK_NOR_CNT_REG			0x04
> > > > > +#define MTK_NOR_RDSR_REG		0x08
> > > > > +#define MTK_NOR_RDATA_REG		0x0c
> > > > > +#define MTK_NOR_RADR0_REG		0x10
> > > > > +#define MTK_NOR_RADR1_REG		0x14
> > > > > +#define MTK_NOR_RADR2_REG		0x18
> > > > > +#define MTK_NOR_WDATA_REG		0x1c
> > > > > +#define MTK_NOR_PRGDATA0_REG		0x20
> > > > > +#define MTK_NOR_PRGDATA1_REG		0x24
> > > > > +#define MTK_NOR_PRGDATA2_REG		0x28
> > > > > +#define MTK_NOR_PRGDATA3_REG		0x2c
> > > > > +#define MTK_NOR_PRGDATA4_REG		0x30
> > > > > +#define MTK_NOR_PRGDATA5_REG		0x34
> > 
> > ^^ so you have 6 "TX" registers.
> > 
> > > > > +#define MTK_NOR_SHREG0_REG		0x38
> > > > > +#define MTK_NOR_SHREG1_REG		0x3c
> > > > > +#define MTK_NOR_SHREG2_REG		0x40
> > > > > +#define MTK_NOR_SHREG3_REG		0x44
> > > > > +#define MTK_NOR_SHREG4_REG		0x48
> > > > > +#define MTK_NOR_SHREG5_REG		0x4c
> > 
> > ^^ and you have at least 6 "RX" registers (looking at the mt8173 manual,
> > I'm not sure what the SHREG{6..9} are for). So I don't see why you can't
> > program MTK_NOR_CNT_REG to 48 (6 bytes * 8 bits/byte) to get 6 bytes
> > TX/RX, or IOW, 1 byte of opcode and 5 bytes of either RX or TX data. So
> > I'm still not convinced about your claim below.
> > 
> > I was able to rewrite your driver to do the above, and I can read out 5
> > bytes no problem, but unfortunately, the flash device I have has only 3
> > bytes of ID, so the last two bytes are zero, which doesn't tell me too
> > much if this is working right.
> > 
> > > > > +#define MTK_NOR_SHREG6_REG		0x50
> > > > > +#define MTK_NOR_SHREG7_REG		0x54
> > > > > +#define MTK_NOR_SHREG8_REG		0x58
> > > > > +#define MTK_NOR_SHREG9_REG		0x5c
> > > > > +#define MTK_NOR_CFG1_REG		0x60
> > > > > +#define MTK_NOR_CFG2_REG		0x64
> > > > > +#define MTK_NOR_CFG3_REG		0x68
> > > > > +#define MTK_NOR_STATUS0_REG		0x70
> > > > > +#define MTK_NOR_STATUS1_REG		0x74
> > > > > +#define MTK_NOR_STATUS2_REG		0x78
> > > > > +#define MTK_NOR_STATUS3_REG		0x7c
> > > > > +#define MTK_NOR_FLHCFG_REG		0x84
> > > > > +#define MTK_NOR_TIME_REG		0x94
> > > > > +#define MTK_NOR_PP_DATA_REG		0x98
> > > > > +#define MTK_NOR_PREBUF_STUS_REG		0x9c
> > > > > +#define MTK_NOR_DELSEL0_REG		0xa0
> > > > > +#define MTK_NOR_DELSEL1_REG		0xa4
> > > > > +#define MTK_NOR_INTRSTUS_REG		0xa8
> > > > > +#define MTK_NOR_INTREN_REG		0xac
> > > > > +#define MTK_NOR_CHKSUM_CTL_REG		0xb8
> > > > > +#define MTK_NOR_CHKSUM_REG		0xbc
> > > > > +#define MTK_NOR_CMD2_REG		0xc0
> > > > > +#define MTK_NOR_WRPROT_REG		0xc4
> > > > > +#define MTK_NOR_RADR3_REG		0xc8
> > > > > +#define MTK_NOR_DUAL_REG		0xcc
> > > > > +#define MTK_NOR_DELSEL2_REG		0xd0
> > > > > +#define MTK_NOR_DELSEL3_REG		0xd4
> > > > > +#define MTK_NOR_DELSEL4_REG		0xd8
> > 
> > ...
> > 
> > > > > +static int mt8173_nor_read_reg(struct spi_nor *nor, u8 opcode, u8 *buf, int len)
> > > > > +{
> > > > > +	int ret;
> > > > > +	struct mt8173_nor *mt8173_nor = nor->priv;
> > > > > +
> > > > > +	/* mtk nor controller doesn't supoort SPINOR_OP_RDCR */
> > > > > +	switch (opcode) {
> > > > > +	case SPINOR_OP_RDID:
> > > > > +		/* read JEDEC ID need 4 bytes commands */
> > > > > +		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 32, SPINOR_OP_RDID);
> > > > > +		if (ret < 0)
> > > > > +			return ret;
> > > > > +
> > > > > +		/* mtk nor flash controller only support 3 bytes IDs */
> > > > 
> > > > Are you absolutely sure of this? That would be highly unfortunate, but
> > > > I also don't believe it's true.
> > > > 
> > > Yes, for this issue I have asked our designer of nor flash controller,
> > > unfortunately, it is true, and I have tried to read more IDs, but our
> > > controller just accept 3 IDs from nor flash, and our next generation IC
> > > may solve this problem.
> > 
> > How exactly did you try? Did you do what I suggested above? Are the
> > "shift registers" not all actually functional?
> > 
> Hi Brian, For this problem, I have asked our nor controller designer to
> double confirm again, and he promise to do some simulation testes, On
> the other hand, I have got some Spansion nor flash which has 5 IDs, and
> after our designer make sure our controller can support 5 or 6 IDs, then
> I will rework a special platform with S25FL256SA. If we have any
> progress, I will inform you immediately!  Thanks!!
> 
Hi Brian, Thanks very much for your perseverance!!
Actually, You idea is correct, and our designer has completed some
simulation test on spansion nor flash witch has more than 3 IDs, and the
results has prove that we can support to read 5 IDs. I also double
confirmed with spansion nor flash. The only pity is that our nor
controller can't support 6 IDs, we just support 5 IDs.
 
I will submit my changes to V6 version. Thanks again!!


> > > > > +		buf[2] = readb(mt8173_nor->base + MTK_NOR_SHREG0_REG);
> > > > > +		buf[1] = readb(mt8173_nor->base + MTK_NOR_SHREG1_REG);
> > > > > +		buf[0] = readb(mt8173_nor->base + MTK_NOR_SHREG2_REG);
> > > > > +		break;
> > 
> > [...]
> > 
> > Brian
> 

Re: [PATCH v4 2/3] mtd: mtk-nor: mtk serial flash controller driver

[Brian Norris]

On Thu, Oct 29, 2015 at 09:56:31PM +0800, bayi.cheng wrote:
> On Thu, 2015-10-29 at 11:28 +0800, bayi.cheng wrote:
> > On Wed, 2015-10-28 at 18:52 -0700, Brian Norris wrote:
> > > On Sun, Oct 18, 2015 at 10:20:35PM +0800, bayi.cheng wrote:
> > > > On Fri, 2015-10-16 at 00:39 -0700, Brian Norris wrote:
> > > > > On Tue, Oct 13, 2015 at 05:39:19PM +0800, Bayi Cheng wrote:

> > > > > > +	/* mtk nor controller doesn't supoort SPINOR_OP_RDCR */
> > > > > > +	switch (opcode) {
> > > > > > +	case SPINOR_OP_RDID:
> > > > > > +		/* read JEDEC ID need 4 bytes commands */
> > > > > > +		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 32, SPINOR_OP_RDID);
> > > > > > +		if (ret < 0)
> > > > > > +			return ret;
> > > > > > +
> > > > > > +		/* mtk nor flash controller only support 3 bytes IDs */
> > > > > 
> > > > > Are you absolutely sure of this? That would be highly unfortunate, but
> > > > > I also don't believe it's true.
> > > > > 
> > > > Yes, for this issue I have asked our designer of nor flash controller,
> > > > unfortunately, it is true, and I have tried to read more IDs, but our
> > > > controller just accept 3 IDs from nor flash, and our next generation IC
> > > > may solve this problem.
> > > 
> > > How exactly did you try? Did you do what I suggested above? Are the
> > > "shift registers" not all actually functional?
> > > 
> > Hi Brian, For this problem, I have asked our nor controller designer to
> > double confirm again, and he promise to do some simulation testes, On
> > the other hand, I have got some Spansion nor flash which has 5 IDs, and
> > after our designer make sure our controller can support 5 or 6 IDs, then
> > I will rework a special platform with S25FL256SA. If we have any
> > progress, I will inform you immediately!  Thanks!!
> > 
> Hi Brian, Thanks very much for your perseverance!!
> Actually, You idea is correct, and our designer has completed some
> simulation test on spansion nor flash witch has more than 3 IDs, and the
> results has prove that we can support to read 5 IDs. I also double

Awesome!

> confirmed with spansion nor flash. The only pity is that our nor
> controller can't support 6 IDs, we just support 5 IDs.

That's not quite as awesome. But that's still much more workable than
only 3 bytes. Perhaps we'll want a way to communicate that to the
spi-nor layer, so it doesn't think it can match a full 6 bytes?

> I will submit my changes to V6 version. Thanks again!!

OK, but let me take another look at v5 and send you comments on that one
first.

Regards,
Brian

Re: [PATCH v4 2/3] mtd: mtk-nor: mtk serial flash controller driver

[bayi.cheng]

On Thu, 2015-10-29 at 09:03 -0700, Brian Norris wrote:
> On Thu, Oct 29, 2015 at 09:56:31PM +0800, bayi.cheng wrote:
> > On Thu, 2015-10-29 at 11:28 +0800, bayi.cheng wrote:
> > > On Wed, 2015-10-28 at 18:52 -0700, Brian Norris wrote:
> > > > On Sun, Oct 18, 2015 at 10:20:35PM +0800, bayi.cheng wrote:
> > > > > On Fri, 2015-10-16 at 00:39 -0700, Brian Norris wrote:
> > > > > > On Tue, Oct 13, 2015 at 05:39:19PM +0800, Bayi Cheng wrote:
> 
> > > > > > > +	/* mtk nor controller doesn't supoort SPINOR_OP_RDCR */
> > > > > > > +	switch (opcode) {
> > > > > > > +	case SPINOR_OP_RDID:
> > > > > > > +		/* read JEDEC ID need 4 bytes commands */
> > > > > > > +		ret = mt8173_nor_set_cmd(mt8173_nor, 0, 32, SPINOR_OP_RDID);
> > > > > > > +		if (ret < 0)
> > > > > > > +			return ret;
> > > > > > > +
> > > > > > > +		/* mtk nor flash controller only support 3 bytes IDs */
> > > > > > 
> > > > > > Are you absolutely sure of this? That would be highly unfortunate, but
> > > > > > I also don't believe it's true.
> > > > > > 
> > > > > Yes, for this issue I have asked our designer of nor flash controller,
> > > > > unfortunately, it is true, and I have tried to read more IDs, but our
> > > > > controller just accept 3 IDs from nor flash, and our next generation IC
> > > > > may solve this problem.
> > > > 
> > > > How exactly did you try? Did you do what I suggested above? Are the
> > > > "shift registers" not all actually functional?
> > > > 
> > > Hi Brian, For this problem, I have asked our nor controller designer to
> > > double confirm again, and he promise to do some simulation testes, On
> > > the other hand, I have got some Spansion nor flash which has 5 IDs, and
> > > after our designer make sure our controller can support 5 or 6 IDs, then
> > > I will rework a special platform with S25FL256SA. If we have any
> > > progress, I will inform you immediately!  Thanks!!
> > > 
> > Hi Brian, Thanks very much for your perseverance!!
> > Actually, You idea is correct, and our designer has completed some
> > simulation test on spansion nor flash witch has more than 3 IDs, and the
> > results has prove that we can support to read 5 IDs. I also double
> 
> Awesome!
> 
> > confirmed with spansion nor flash. The only pity is that our nor
> > controller can't support 6 IDs, we just support 5 IDs.
> 
> That's not quite as awesome. But that's still much more workable than
> only 3 bytes. Perhaps we'll want a way to communicate that to the
> spi-nor layer, so it doesn't think it can match a full 6 bytes?
> 
Hi Brian,  The current station is as follows.

1: put 0x9F to MTK_NOR_PRGDATA5_REG, and five 0x0 to
MTK_NOR_PRGDATA4_REG ~ MTK_NOR_PRGDATA0_REG, then set (1 + 5) * 8 
to MTK_NOR_CNT_REG, for this way, we can read five IDs.

2: put 0x9F to MTK_NOR_PRGDATA5_REG, and five 0x0 to
MTK_NOR_PRGDATA4_REG ~ MTK_NOR_PRGDATA0_REG, then set (1 + 5 + 1) * 8
to MTK_NOR_CNT_REG, for this way, we can read six IDs.
In this case, nor flash IDs can be read from MTK_NOR_SHREG5_REG to
MTK_NOR_SHREG0_REG . Thanks!

> > I will submit my changes to V6 version. Thanks again!!
> 
> OK, but let me take another look at v5 and send you comments on that one
> first.
> 
> Regards,
> Brian
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v4 2/3] mtd: mtk-nor: mtk serial flash controller driver

[Brian Norris]

Hi Bayi,

On Fri, Oct 30, 2015 at 10:12:39AM +0800, bayi.cheng wrote:
> Hi Brian,  The current station is as follows.
> 
> 1: put 0x9F to MTK_NOR_PRGDATA5_REG, and five 0x0 to
> MTK_NOR_PRGDATA4_REG ~ MTK_NOR_PRGDATA0_REG, then set (1 + 5) * 8 
> to MTK_NOR_CNT_REG, for this way, we can read five IDs.
> 
> 2: put 0x9F to MTK_NOR_PRGDATA5_REG, and five 0x0 to
> MTK_NOR_PRGDATA4_REG ~ MTK_NOR_PRGDATA0_REG, then set (1 + 5 + 1) * 8
> to MTK_NOR_CNT_REG, for this way, we can read six IDs.
> In this case, nor flash IDs can be read from MTK_NOR_SHREG5_REG to
> MTK_NOR_SHREG0_REG . Thanks!

Thanks for the update. Glad to hear you can read more bytes there; the
extra number of Shift Registers *looked* to me like you should be able
to read even more than 5...

So in my other email, I showed you how I generalized the TX/RX function,
so it can handle most of both the write_reg() and read_reg() functions.
I'm not 100% sure now that it has all of the RX path correct; it worked
for the cases I could test, but it's confusing when reading the manual
to figure out which SHREG register I should start from. But anyway, I
think it should only take some small adjustments to my patch to make it
handle things properly.

I'd really appreciate it if you could incorporate my feedback and
review/improve the ..._do_tx_rx() function I wrote, to make sure it
handles reading any arbitrary number of bytes (at least up to 6). So,
you might, for example, run some tests where you have spi-nor.c call

	nor->read_reg(nor, SPINOR_OP_RDID, id, idlen)

with varying values for 'idlen', and make sure they all work properly.

Let me know if you have any questions about comments either here, or on
v5.

Regards,
Brian
--
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 v4 3/3] arm64: dts: mt8173: Add nor flash node

[Bayi Cheng]

Add Mediatek nor flash node

Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
---
 arch/arm64/boot/dts/mediatek/mt8173.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index d18ee42..385c2e4 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -365,6 +365,21 @@
 			status = "disabled";
 		};
 
+		nor_flash: spi@1100d000 {
+			compatible = "mediatek,mt8173-nor";
+			reg = <0 0x1100d000 0 0xe0>;
+			clocks = <&pericfg CLK_PERI_SPI>,
+				 <&topckgen CLK_TOP_SPINFI_IFR_SEL>;
+			clock-names = "spi", "sf";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			flash@0 {
+				compatible = "jedec,spi-nor";
+				reg = <0>;
+			};
+		};
+
 		i2c3: i2c3@11010000 {
 			compatible = "mediatek,mt8173-i2c";
 			reg = <0 0x11010000 0 0x70>,
-- 
1.8.1.1.dirty

Re: [PATCH v4 3/3] arm64: dts: mt8173: Add nor flash node

[Sergei Shtylyov]

Hello.

On 10/13/2015 12:39 PM, Bayi Cheng wrote:

> Add Mediatek nor flash node
>
> Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
> ---
>   arch/arm64/boot/dts/mediatek/mt8173.dtsi | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index d18ee42..385c2e4 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -365,6 +365,21 @@
>   			status = "disabled";
>   		};
>
> +		nor_flash: spi@1100d000 {

    I don't think this is a good label name for a SPI controller.

> +			compatible = "mediatek,mt8173-nor";
> +			reg = <0 0x1100d000 0 0xe0>;
> +			clocks = <&pericfg CLK_PERI_SPI>,
> +				 <&topckgen CLK_TOP_SPINFI_IFR_SEL>;
> +			clock-names = "spi", "sf";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			flash@0 {
> +				compatible = "jedec,spi-nor";
> +				reg = <0>;
> +			};
> +		};
> +
>   		i2c3: i2c3@11010000 {

    Grr, the name should be "i2c@11010000".

[...]

MBR, Sergei

Re: [PATCH v4 3/3] arm64: dts: mt8173: Add nor flash node

[Daniel Kurtz]

Sergei,

On Tue, Oct 13, 2015 at 10:33 PM, Sergei Shtylyov
<sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org> wrote:
> Hello.
>
> On 10/13/2015 12:39 PM, Bayi Cheng wrote:
>
>> Add Mediatek nor flash node
>>
>> Signed-off-by: Bayi Cheng <bayi.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>> ---
>>   arch/arm64/boot/dts/mediatek/mt8173.dtsi | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> index d18ee42..385c2e4 100644
>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> @@ -365,6 +365,21 @@
>>                         status = "disabled";
>>                 };
>>
>> +               nor_flash: spi@1100d000 {
>
>
>    I don't think this is a good label name for a SPI controller.

You think "spi" is not a good name for SPI controller?
Or you mean the label "nor_flash"?

Would you prefer:

  spi_nor_flash: spi@1100d000


>
>> +                       compatible = "mediatek,mt8173-nor";
>> +                       reg = <0 0x1100d000 0 0xe0>;
>> +                       clocks = <&pericfg CLK_PERI_SPI>,
>> +                                <&topckgen CLK_TOP_SPINFI_IFR_SEL>;
>> +                       clock-names = "spi", "sf";
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +
>> +                       flash@0 {
>> +                               compatible = "jedec,spi-nor";
>> +                               reg = <0>;
>> +                       };
>> +               };
>> +
>>                 i2c3: i2c3@11010000 {
>
>
>    Grr, the name should be "i2c@11010000".

Don't worry.  This is already fixed in Matthias' tree:
  https://github.com/mbgg/linux-mediatek/commit/1ee35c05d9c67771d649f687e6f76e61e76eb779

-Dan

>
> [...]
>
> MBR, Sergei
>
--
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 v4 3/3] arm64: dts: mt8173: Add nor flash node

[Sergei Shtylyov]

Hello.

On 10/14/2015 4:07 AM, Daniel Kurtz wrote:

>>> Add Mediatek nor flash node
>>>
>>> Signed-off-by: Bayi Cheng <bayi.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>>> ---
>>>    arch/arm64/boot/dts/mediatek/mt8173.dtsi | 15 +++++++++++++++
>>>    1 file changed, 15 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>>> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>>> index d18ee42..385c2e4 100644
>>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>>> @@ -365,6 +365,21 @@
>>>                          status = "disabled";
>>>                  };
>>>
>>> +               nor_flash: spi@1100d000 {
>>
>>     I don't think this is a good label name for a SPI controller.
>
> You think "spi" is not a good name for SPI controller?
> Or you mean the label "nor_flash"?

    I think I said clearly: "label". :-)

> Would you prefer:
>
>    spi_nor_flash: spi@1100d000

    I'd prefer the "_nor_flash" part to be dropped, to be used for the real 
flash device (if needed) which is a sub-node of this node.

[...]

> -Dan

MBR, Sergei

--
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 v4 3/3] arm64: dts: mt8173: Add nor flash node

[Daniel Kurtz]

Sergei,

On Wed, Oct 14, 2015 at 7:05 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Hello.
>
> On 10/14/2015 4:07 AM, Daniel Kurtz wrote:
>
>>>> Add Mediatek nor flash node
>>>>
>>>> Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
>>>> ---
>>>>    arch/arm64/boot/dts/mediatek/mt8173.dtsi | 15 +++++++++++++++
>>>>    1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>>>> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>>>> index d18ee42..385c2e4 100644
>>>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>>>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>>>> @@ -365,6 +365,21 @@
>>>>                          status = "disabled";
>>>>                  };
>>>>
>>>> +               nor_flash: spi@1100d000 {
>>>
>>>
>>>     I don't think this is a good label name for a SPI controller.
>>
>>
>> You think "spi" is not a good name for SPI controller?
>> Or you mean the label "nor_flash"?
>
>
>    I think I said clearly: "label". :-)
>
>> Would you prefer:
>>
>>    spi_nor_flash: spi@1100d000
>
>
>    I'd prefer the "_nor_flash" part to be dropped, to be used for the real
> flash device (if needed) which is a sub-node of this node.

The dedicated nor_flash spi bus cannot just be labeled 'spi', as that
would conflict with the general purpose 'spi' bus.  Instead, it is
given a unique label that can be used as a phandle elsewhere (e.g., in
a board file, to enable the bus).  The name "spi_nor_flash" makes it
clear that this spi bus is only for use for accessing the nor_flash.

Bayi: Actually, this reminds me.  I'd prefer if the bus node was
disabled by default, and only enabled by boards as required.  So, the
node should include:
   status = "disabled";

-Dan

Re: [PATCH v4 3/3] arm64: dts: mt8173: Add nor flash node

[bayi.cheng]

On Wed, 2015-10-14 at 19:26 +0800, Daniel Kurtz wrote:
> Sergei,
> 
> On Wed, Oct 14, 2015 at 7:05 PM, Sergei Shtylyov
> <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org> wrote:
> > Hello.
> >
> > On 10/14/2015 4:07 AM, Daniel Kurtz wrote:
> >
> >>>> Add Mediatek nor flash node
> >>>>
> >>>> Signed-off-by: Bayi Cheng <bayi.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> >>>> ---
> >>>>    arch/arm64/boot/dts/mediatek/mt8173.dtsi | 15 +++++++++++++++
> >>>>    1 file changed, 15 insertions(+)
> >>>>
> >>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>>> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>>> index d18ee42..385c2e4 100644
> >>>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>>> @@ -365,6 +365,21 @@
> >>>>                          status = "disabled";
> >>>>                  };
> >>>>
> >>>> +               nor_flash: spi@1100d000 {
> >>>
> >>>
> >>>     I don't think this is a good label name for a SPI controller.
> >>
> >>
> >> You think "spi" is not a good name for SPI controller?
> >> Or you mean the label "nor_flash"?
> >
> >
> >    I think I said clearly: "label". :-)
> >
> >> Would you prefer:
> >>
> >>    spi_nor_flash: spi@1100d000
> >
> >
> >    I'd prefer the "_nor_flash" part to be dropped, to be used for the real
> > flash device (if needed) which is a sub-node of this node.
> 
> The dedicated nor_flash spi bus cannot just be labeled 'spi', as that
> would conflict with the general purpose 'spi' bus.  Instead, it is
> given a unique label that can be used as a phandle elsewhere (e.g., in
> a board file, to enable the bus).  The name "spi_nor_flash" makes it
> clear that this spi bus is only for use for accessing the nor_flash.
> 
> Bayi: Actually, this reminds me.  I'd prefer if the bus node was
> disabled by default, and only enabled by boards as required.  So, the
> node should include:
>    status = "disabled";
> 
> -Dan

Hi, Dan & Sergei, Thanks for your advice, and I will add "status =
"disable"" to the node in the next patch.Thanks~

Bayi