[PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

1.) Why add a new framework for SPI NOR?
  The SPI-NOR controller such as Freescale's Quadspi controller is working
  in a different way from the SPI bus. It should knows the NOR commands to
  find the right LUT sequence. Unfortunately, the current code can not meet
  this requirement.

2.) How does this patch set do?
   This patch set adds a new spi-nor layer.
   Before this patch, the layer is like:

                   MTD
         ------------------------
                  m25p80
         ------------------------
	       spi bus driver
         ------------------------
	        SPI NOR chip

   After this patch, the layer is like:
                   MTD
         ------------------------
                  spi-nor
         ------------------------
                  m25p80
         ------------------------
	       spi bus driver
         ------------------------
	       SPI NOR chip

  With the spi-nor controller driver(Freescale Quadspi), it looks like:
                   MTD
         ------------------------
                  spi-nor
         ------------------------
                fsl-quadspi
         ------------------------
	       SPI NOR chip

3.) more details
   This patch set adds a new data structrue spi_nor{}, clones most the common
  code to spi-nor.c. Make the m25p80.c use the new APIs.

4.) TODO list:
   3.1) add the new spi_nor_device{} for the spi-nor controller's device.
   3.2) add the Freescale Quadspi driver.

	       
Huang Shijie (4):
  mtd: spi-nor: move the SPI NOR commands to a new header file
  mtd: spi-nor: add a new data structrue spi_nor{}
  mtd: spi-nor: add the framework for SPI NOR
  mtd: m25p80: use the new spi-nor APIs

 drivers/mtd/Kconfig           |    2 +
 drivers/mtd/Makefile          |    1 +
 drivers/mtd/devices/Kconfig   |    2 +-
 drivers/mtd/devices/m25p80.c  | 1249 +++--------------------------------------
 drivers/mtd/spi-nor/Kconfig   |    6 +
 drivers/mtd/spi-nor/Makefile  |    1 +
 drivers/mtd/spi-nor/spi-nor.c | 1057 ++++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h   |  101 ++++
 8 files changed, 1239 insertions(+), 1180 deletions(-)
 create mode 100644 drivers/mtd/spi-nor/Kconfig
 create mode 100644 drivers/mtd/spi-nor/Makefile
 create mode 100644 drivers/mtd/spi-nor/spi-nor.c
 create mode 100644 include/linux/mtd/spi-nor.h

-- 
1.7.2.rc3

[PATCH 1/4] mtd: spi-nor: move the SPI NOR commands to a new header file

[Huang Shijie]

This patch adds a new header :spi-nor.h,
and moves all the SPI NOR commands and relative macros into this new header.

This hearder can be used by the m25p80.c and other spi-nor controller,
such as Freescale's Quadspi.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/devices/m25p80.c |   50 +--------------------------------------
 include/linux/mtd/spi-nor.h  |   53 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 49 deletions(-)
 create mode 100644 include/linux/mtd/spi-nor.h

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 7dc2c14..4703aa4 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -34,55 +34,7 @@
 
 #include <linux/spi/spi.h>
 #include <linux/spi/flash.h>
-
-/* Flash opcodes. */
-#define	OPCODE_WREN		0x06	/* Write enable */
-#define	OPCODE_RDSR		0x05	/* Read status register */
-#define	OPCODE_WRSR		0x01	/* Write status register 1 byte */
-#define	OPCODE_NORM_READ	0x03	/* Read data bytes (low frequency) */
-#define	OPCODE_FAST_READ	0x0b	/* Read data bytes (high frequency) */
-#define	OPCODE_QUAD_READ        0x6b    /* Read data bytes */
-#define	OPCODE_PP		0x02	/* Page program (up to 256 bytes) */
-#define	OPCODE_BE_4K		0x20	/* Erase 4KiB block */
-#define	OPCODE_BE_4K_PMC	0xd7	/* Erase 4KiB block on PMC chips */
-#define	OPCODE_BE_32K		0x52	/* Erase 32KiB block */
-#define	OPCODE_CHIP_ERASE	0xc7	/* Erase whole flash chip */
-#define	OPCODE_SE		0xd8	/* Sector erase (usually 64KiB) */
-#define	OPCODE_RDID		0x9f	/* Read JEDEC ID */
-#define	OPCODE_RDCR             0x35    /* Read configuration register */
-
-/* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
-#define	OPCODE_NORM_READ_4B	0x13	/* Read data bytes (low frequency) */
-#define	OPCODE_FAST_READ_4B	0x0c	/* Read data bytes (high frequency) */
-#define	OPCODE_QUAD_READ_4B	0x6c    /* Read data bytes */
-#define	OPCODE_PP_4B		0x12	/* Page program (up to 256 bytes) */
-#define	OPCODE_SE_4B		0xdc	/* Sector erase (usually 64KiB) */
-
-/* Used for SST flashes only. */
-#define	OPCODE_BP		0x02	/* Byte program */
-#define	OPCODE_WRDI		0x04	/* Write disable */
-#define	OPCODE_AAI_WP		0xad	/* Auto address increment word program */
-
-/* Used for Macronix and Winbond flashes. */
-#define	OPCODE_EN4B		0xb7	/* Enter 4-byte mode */
-#define	OPCODE_EX4B		0xe9	/* Exit 4-byte mode */
-
-/* Used for Spansion flashes only. */
-#define	OPCODE_BRWR		0x17	/* Bank register write */
-
-/* Status Register bits. */
-#define	SR_WIP			1	/* Write in progress */
-#define	SR_WEL			2	/* Write enable latch */
-/* meaning of other SR_* bits may differ between vendors */
-#define	SR_BP0			4	/* Block protect 0 */
-#define	SR_BP1			8	/* Block protect 1 */
-#define	SR_BP2			0x10	/* Block protect 2 */
-#define	SR_SRWD			0x80	/* SR write protect */
-
-#define SR_QUAD_EN_MX           0x40    /* Macronix Quad I/O */
-
-/* Configuration Register bits. */
-#define CR_QUAD_EN_SPAN		0x2     /* Spansion Quad I/O */
+#include <linux/mtd/spi-nor.h>
 
 /* Define max times to check status register before we give up. */
 #define	MAX_READY_WAIT_JIFFIES	(40 * HZ)	/* M25P16 specs 40s max chip erase */
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
new file mode 100644
index 0000000..ab2ea1e
--- /dev/null
+++ b/include/linux/mtd/spi-nor.h
@@ -0,0 +1,53 @@
+#ifndef __LINUX_MTD_SPI_NOR_H
+#define __LINUX_MTD_SPI_NOR_H
+
+/* Flash opcodes. */
+#define	OPCODE_WREN		0x06	/* Write enable */
+#define	OPCODE_RDSR		0x05	/* Read status register */
+#define	OPCODE_WRSR		0x01	/* Write status register 1 byte */
+#define	OPCODE_NORM_READ	0x03	/* Read data bytes (low frequency) */
+#define	OPCODE_FAST_READ	0x0b	/* Read data bytes (high frequency) */
+#define	OPCODE_QUAD_READ        0x6b    /* Read data bytes */
+#define	OPCODE_PP		0x02	/* Page program (up to 256 bytes) */
+#define	OPCODE_BE_4K		0x20	/* Erase 4KiB block */
+#define	OPCODE_BE_4K_PMC	0xd7	/* Erase 4KiB block on PMC chips */
+#define	OPCODE_BE_32K		0x52	/* Erase 32KiB block */
+#define	OPCODE_CHIP_ERASE	0xc7	/* Erase whole flash chip */
+#define	OPCODE_SE		0xd8	/* Sector erase (usually 64KiB) */
+#define	OPCODE_RDID		0x9f	/* Read JEDEC ID */
+#define	OPCODE_RDCR             0x35    /* Read configuration register */
+
+/* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
+#define	OPCODE_NORM_READ_4B	0x13	/* Read data bytes (low frequency) */
+#define	OPCODE_FAST_READ_4B	0x0c	/* Read data bytes (high frequency) */
+#define	OPCODE_QUAD_READ_4B	0x6c    /* Read data bytes */
+#define	OPCODE_PP_4B		0x12	/* Page program (up to 256 bytes) */
+#define	OPCODE_SE_4B		0xdc	/* Sector erase (usually 64KiB) */
+
+/* Used for SST flashes only. */
+#define	OPCODE_BP		0x02	/* Byte program */
+#define	OPCODE_WRDI		0x04	/* Write disable */
+#define	OPCODE_AAI_WP		0xad	/* Auto address increment word program */
+
+/* Used for Macronix and Winbond flashes. */
+#define	OPCODE_EN4B		0xb7	/* Enter 4-byte mode */
+#define	OPCODE_EX4B		0xe9	/* Exit 4-byte mode */
+
+/* Used for Spansion flashes only. */
+#define	OPCODE_BRWR		0x17	/* Bank register write */
+
+/* Status Register bits. */
+#define	SR_WIP			1	/* Write in progress */
+#define	SR_WEL			2	/* Write enable latch */
+/* meaning of other SR_* bits may differ between vendors */
+#define	SR_BP0			4	/* Block protect 0 */
+#define	SR_BP1			8	/* Block protect 1 */
+#define	SR_BP2			0x10	/* Block protect 2 */
+#define	SR_SRWD			0x80	/* SR write protect */
+
+#define SR_QUAD_EN_MX           0x40    /* Macronix Quad I/O */
+
+/* Configuration Register bits. */
+#define CR_QUAD_EN_SPAN		0x2     /* Spansion Quad I/O */
+
+#endif
-- 
1.7.2.rc3

RE: [PATCH 1/4] mtd: spi-nor: move the SPI NOR commands to a new header file

[Gupta, Pekon]

Hi Shijie,

Few feedbacks inline.. 

> From: Huang Shijie [mailto:b32955@freescale.com]
> This patch adds a new header :spi-nor.h,
> and moves all the SPI NOR commands and relative macros into this new
> header.
> 
(1)
It's better to take all Flash vendor specific opcodes from DT.. 
With new framework we should do away with macros for each vendor,
instead let each board file provide this information based on flash device
which is mounted on it.

(2)
And also, I suggest not to touch m25p80 in anyway, bcoz it stable driver
used by many. So unless spi-nor framework is accepted and is stable we
m25p80 should continue working as it is.
Thus your patches should be independent of any existing driver or other
frameworks. This way it might get accepted fast. Then later you can provide
a way to attach m25p80 to spi-nor framework.
Thoughts ?


with regards, pekon

Re: [PATCH 1/4] mtd: spi-nor: move the SPI NOR commands to a new header file

[Huang Shijie]

Hi Pekon:
> (1)
> It's better to take all Flash vendor specific opcodes from DT..
firstly, some arch may does not support the DT, such as x86.

second, it makes people crazy if we put the opcodes in the DT :)
> With new framework we should do away with macros for each vendor,
> instead let each board file provide this information based on flash device
> which is mounted on it.
>
> (2)
> And also, I suggest not to touch m25p80 in anyway, bcoz it stable driver
> used by many. So unless spi-nor framework is accepted and is stable we
> m25p80 should continue working as it is.
> Thus your patches should be independent of any existing driver or other
> frameworks. This way it might get accepted fast. Then later you can provide
> a way to attach m25p80 to spi-nor framework.
> Thoughts ?
the merge window is closed not long ago, and we have rc1 now.

Isn't it the BEST time to change the m25p80?

thanks
Huang Shijie

Re: [PATCH 1/4] mtd: spi-nor: move the SPI NOR commands to a new header file

[Angus Clark]

On 11/26/2013 08:53 AM, Huang Shijie wrote:
> Hi Pekon:
>> (1)
>> It's better to take all Flash vendor specific opcodes from DT..
> firstly, some arch may does not support the DT, such as x86.

Indeed, although the 'flash_platform_data' structure could be extended to
provide similar functionality, if that were deemed the right way to go.  However...

> 
> second, it makes people crazy if we put the opcodes in the DT :)

I would agree.  Where possible, I think it's preferable for the driver to probe
device properties at runtime.  One benefit, often overlooked, is that it allows
board manufacturers to second-source parts without having to rebuild, and
possibly re-certify, binaries.

>> With new framework we should do away with macros for each vendor,
>> instead let each board file provide this information based on flash
>> device
>> which is mounted on it.
>>
>> (2)
>> And also, I suggest not to touch m25p80 in anyway, bcoz it stable driver
>> used by many. So unless spi-nor framework is accepted and is stable we
>> m25p80 should continue working as it is.
>> Thus your patches should be independent of any existing driver or other
>> frameworks. This way it might get accepted fast. Then later you can
>> provide
>> a way to attach m25p80 to spi-nor framework.
>> Thoughts ?
> the merge window is closed not long ago, and we have rc1 now.
> 
> Isn't it the BEST time to change the m25p80?
> 

The spi-nor framework has great potential, but I think you should allow some
time for other driver developers to comment, and the framework to mature.
Without greater support, it risks becoming another single-use framework.

Cheers,

Angus

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

-- 
-------------------------------------
Angus Clark
ST Microelectronics (R&D) Ltd.
1000 Aztec West, Bristol, BS32 4SQ
email: angus.clark@st.com
tel: +44 (0) 1454 462389
st-tina: 065 2389
-------------------------------------

[PATCH 2/4] mtd: spi-nor: add a new data structrue spi_nor{}

[Huang Shijie]

The spi_nor{} is cloned from the m25p{}.
The spi_nor{} can be used by both the m25p80 and spi-nor controller.

 1) Add four hooks:
    @read_reg: used to read the registers, such as read status register,
             read ID, read configure register.

    @write_reg: used to write the registers, such as write enable,
             erase sector.

    @read: use the proper read opcode to read out the data from the NOR.

    @write: use the proper write opcode to write data to the NOR.

 2) Add a new field sst_write_second for the SST NOR write.

 3) change the @command field from pointer to array which makes the code
    more simple.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/devices/m25p80.c |    6 -----
 include/linux/mtd/spi-nor.h  |   43 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 4703aa4..13d9864 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -44,12 +44,6 @@
 
 /****************************************************************************/
 
-enum read_type {
-	M25P80_NORMAL = 0,
-	M25P80_FAST,
-	M25P80_QUAD,
-};
-
 struct m25p {
 	struct spi_device	*spi;
 	struct mutex		lock;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index ab2ea1e..8da1f69 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -50,4 +50,47 @@
 /* Configuration Register bits. */
 #define CR_QUAD_EN_SPAN		0x2     /* Spansion Quad I/O */
 
+#define	MAX_CMD_SIZE		6
+
+enum read_type {
+	M25P80_NORMAL = 0,
+	M25P80_FAST,
+	M25P80_QUAD,
+};
+
+struct spi_nor {
+	struct mutex		lock;
+	struct mtd_info		mtd;
+	struct device		*dev;
+	u16			page_size;
+	u16			addr_width;
+	u8			erase_opcode;
+	u8			read_opcode;
+	u8			program_opcode;
+	u8			command[MAX_CMD_SIZE];
+	enum read_type		flash_read;
+	bool			sst_write_second;
+
+	/*
+	 * Read the register:
+	 *  Read `len` length data from the register specified by the `opcode`,
+	 *  and store the data to the `buf`.
+	 */
+	int (*read_reg)(struct spi_nor *flash, u8 opcode, u8 *buf, int len);
+
+	/*
+	 * Write the register:
+	 *  Write the `cmd_len` length data stored in the @command to the NOR,
+	 *  the command[0] stores the write opcode. `offset` is only used for
+	 *  erase operation, it should set to zero for other NOR commands.
+	 */
+	int (*write_reg)(struct spi_nor *flash, int cmd_len, u32 offset);
+
+	/* write data */
+	void (*write)(struct spi_nor *flash, loff_t to, size_t len,
+			size_t *retlen, const u_char *buf);
+	/* read data */
+	int (*read)(struct spi_nor *flash, loff_t from, size_t len,
+			size_t *retlen, u_char *buf);
+};
 #endif
-- 
1.7.2.rc3

RE: [PATCH 2/4] mtd: spi-nor: add a new data structrue spi_nor{}

[Gupta, Pekon]

> From: Huang Shijie [mailto:b32955@freescale.com]
[...]
> 
> +#define	MAX_CMD_SIZE		6
> +
> +enum read_type {
> +	M25P80_NORMAL = 0,
> +	M25P80_FAST,
> +	M25P80_QUAD,
> +};
Sorry. no 'M25P80' suffix here this is spi-nor.h :-)


> +
> +struct spi_nor {
> +	struct mutex		lock;
> +	struct mtd_info		mtd;
> 
mtd_info should not be present here. Rather it should be other way round
'mtd_info->priv = (struct spi_nor *) spi_nor;


> +	struct device		*dev;
Again, spi_nor would be a MTD device, not a new type of device on its own.
Thus you should use, mtd_info->dev.


> +	u16			page_size;
> +	u16			addr_width;
> +	u8			erase_opcode;
> +	u8			read_opcode;
s/read_opcode/read_flash_opcode
How about '+  u8  read_reg_opcode' ??


> +	u8			program_opcode;
+ How about '+  u8  write_reg_opcode' ??


> +	u8			command[MAX_CMD_SIZE];
> +	enum read_type		flash_read;
s/read_type/read_mode
(agree .. there is nothing in the name, but it matches SPI generic framework)

Other missing fields I can think of are.. 
+ u8 read_dummy_cycles;
+ u8 write_dummy_cycles;


> +	bool			sst_write_second;
> +
> +	/*
> +	 * Read the register:
> +	 *  Read `len` length data from the register specified by the `opcode`,
> +	 *  and store the data to the `buf`.
> +	 */
> +	int (*read_reg)(struct spi_nor *flash, u8 opcode, u8 *buf, int len);
> 
Do you need 'opcode' passed in argument here ?
Can you add it as 'read_reg_opcode' field in struct spi_nor ?
'read_reg_opcode' should be fixed for a device like a 'read_flash_opcode'.


> +
> +	/*
> +	 * Write the register:
> +	 *  Write the `cmd_len` length data stored in the @command to the
> NOR,
> +	 *  the command[0] stores the write opcode. `offset` is only used for
> +	 *  erase operation, it should set to zero for other NOR commands.
> +	 */
> +	int (*write_reg)(struct spi_nor *flash, int cmd_len, u32 offset);
> 
Instead of having actual 'command[]' array in struct spi_nor, and pass its
valid length here.. shouldn't you pass the command as u8[] here..
int (*write_reg)(struct spi_nor *flash, u8 *cmd, u32 cmd_len);
where
	cmd[0] == command_opcode
	cmd[1] == command argument 1 (like offset for erase)
	cmd[2] == command argument 2 
	...

> +
> +	/* write data */
> +	void (*write)(struct spi_nor *flash, loff_t to, size_t len,
> +			size_t *retlen, const u_char *buf);
> +	/* read data */
> +	int (*read)(struct spi_nor *flash, loff_t from, size_t len,
> +			size_t *retlen, u_char *buf);
> +};
>  #endif
> --
> 1.7.2.rc3
> 

Sorry for bit intrusive feedback. But I think it would help you to
refine it better. Also some reference below
http://lists.infradead.org/pipermail/linux-mtd/2013-October/049307.html


with regards, pekon

Re: [PATCH 2/4] mtd: spi-nor: add a new data structrue spi_nor{}

[Huang Shijie]

于 2013年11月26日 19:42, Gupta, Pekon 写道:
>> From: Huang Shijie [mailto:b32955@freescale.com]
> [...]
>> +#define	MAX_CMD_SIZE		6
>> +
>> +enum read_type {
>> +	M25P80_NORMAL = 0,
>> +	M25P80_FAST,
>> +	M25P80_QUAD,
>> +};
> Sorry. no 'M25P80' suffix here this is spi-nor.h :-)
>
>
ok. thanks. :)
>> +
>> +struct spi_nor {
>> +	struct mutex		lock;
>> +	struct mtd_info		mtd;
>>
> mtd_info should not be present here. Rather it should be other way round
> 'mtd_info->priv = (struct spi_nor *) spi_nor;
>
>
put the mtd here can make code simple,

do David/Brian have any comment about this?
If all object to put the mtd here, i will change it.


>> +	struct device		*dev;
> Again, spi_nor would be a MTD device, not a new type of device on its own.
> Thus you should use, mtd_info->dev.
>
>
this dev pointer is not from the mtd_info->dev, it from the spi_device 
or other spi nor device .
>> +	u16			page_size;
>> +	u16			addr_width;
>> +	u8			erase_opcode;
>> +	u8			read_opcode;
> s/read_opcode/read_flash_opcode
why not keep the legacy name?
should we also rename the erase_opcode to erase_flash_opcode? :)
> How about '+  u8  read_reg_opcode' ??
>
>
>> +	u8			program_opcode;
> + How about '+  u8  write_reg_opcode' ??
>
>
>> +	u8			command[MAX_CMD_SIZE];
>> +	enum read_type		flash_read;
> s/read_type/read_mode
> (agree .. there is nothing in the name, but it matches SPI generic framework)
>
> Other missing fields I can think of are..
> + u8 read_dummy_cycles;
I was wondering if other SPI NOR driver needs this field.
current dummy is 8 clock cycles for quad-read/fast-read, but for DDR 
QUAD read, the dummy is not 8 clock cycles.
so i did not add this field to spi-nor{}.

I do not know how the m25p80 handle the non-8 clock cycles cases...

But it's okay to me to add this field to spi-nor{}.




> + u8 write_dummy_cycles;
>
>
do the write need a dummy?

I check the s25fl128s's quad page program, it does not need a dummy.
>> +	bool			sst_write_second;
>> +
>> +	/*
>> +	 * Read the register:
>> +	 *  Read `len` length data from the register specified by the `opcode`,
>> +	 *  and store the data to the `buf`.
>> +	 */
>> +	int (*read_reg)(struct spi_nor *flash, u8 opcode, u8 *buf, int len);
>>
> Do you need 'opcode' passed in argument here ?
> Can you add it as 'read_reg_opcode' field in struct spi_nor ?
> 'read_reg_opcode' should be fixed for a device like a 'read_flash_opcode'.
>
>
the @read_reg can be used to read id, read status and so on.
so the opcode here is not a fixed value.

but i do not object to add the read_reg_opcode/write_reg_opcode to 
spi_nor{}.



>> +
>> +	/*
>> +	 * Write the register:
>> +	 *  Write the `cmd_len` length data stored in the @command to the
>> NOR,
>> +	 *  the command[0] stores the write opcode. `offset` is only used for
>> +	 *  erase operation, it should set to zero for other NOR commands.
>> +	 */
>> +	int (*write_reg)(struct spi_nor *flash, int cmd_len, u32 offset);
>>
> Instead of having actual 'command[]' array in struct spi_nor, and pass its
> valid length here.. shouldn't you pass the command as u8[] here..
> int (*write_reg)(struct spi_nor *flash, u8 *cmd, u32 cmd_len);
> where
> 	cmd[0] == command_opcode
> 	cmd[1] == command argument 1 (like offset for erase)
> 	cmd[2] == command argument 2
> 	...
>
is there any benefit of your code?
you will use a local array in the stack.

why not use the spi_nor->command which has used for a long time.


>> +
>> +	/* write data */
>> +	void (*write)(struct spi_nor *flash, loff_t to, size_t len,
>> +			size_t *retlen, const u_char *buf);
>> +	/* read data */
>> +	int (*read)(struct spi_nor *flash, loff_t from, size_t len,
>> +			size_t *retlen, u_char *buf);
>> +};
>>   #endif
>> --
>> 1.7.2.rc3
>>
> Sorry for bit intrusive feedback. But I think it would help you to
> refine it better. Also some reference below
> http://lists.infradead.org/pipermail/linux-mtd/2013-October/049307.html
>
thanks. I referenced it when i wrote this patch.

thanks
Huang Shijie

Re: [PATCH 2/4] mtd: spi-nor: add a new data structrue spi_nor{}

[Marek Vasut]

Dear Huang Shijie,

> 于 2013年11月26日 19:42, Gupta, Pekon 写道:
> >> From: Huang Shijie [mailto:b32955@freescale.com]
> > 
> > [...]
> > 
> >> +#define	MAX_CMD_SIZE		6
> >> +
> >> +enum read_type {
> >> +	M25P80_NORMAL = 0,
> >> +	M25P80_FAST,
> >> +	M25P80_QUAD,
> >> +};
> > 
> > Sorry. no 'M25P80' suffix here this is spi-nor.h :-)
> 
> ok. thanks. :)
> 
> >> +
> >> +struct spi_nor {
> >> +	struct mutex		lock;
> >> +	struct mtd_info		mtd;
> > 
> > mtd_info should not be present here. Rather it should be other way round
> > 'mtd_info->priv = (struct spi_nor *) spi_nor;
> 
> put the mtd here can make code simple,

The MTD API functions will pass you the struct mtd_info anyway, so you will need 
to implement mtdinfo_to_yourdriverdata() function, no need for duplication.

> do David/Brian have any comment about this?
> If all object to put the mtd here, i will change it.
> 
> >> +	struct device		*dev;
> > 
> > Again, spi_nor would be a MTD device, not a new type of device on its
> > own. Thus you should use, mtd_info->dev.
> 
> this dev pointer is not from the mtd_info->dev, it from the spi_device
> or other spi nor device .

So this is your own device pointer or ... what kind of device pointer?
[...]

Best regards,
Marek Vasut

Re: [PATCH 2/4] mtd: spi-nor: add a new data structrue spi_nor{}

[Huang Shijie]

于 2013年11月27日 17:32, Marek Vasut 写道:
> So this is your own device pointer or ... what kind of device pointer?
the pointer from spi_device{}

Huang Shijie

Re: [PATCH 2/4] mtd: spi-nor: add a new data structrue spi_nor{}

[Marek Vasut]

Dear Huang Shijie,

> 于 2013年11月27日 17:32, Marek Vasut 写道:
> > So this is your own device pointer or ... what kind of device pointer?
> 
> the pointer from spi_device{}

OK, I am 200% positive you need to add comments into the structure about what 
exactly does each member of it mean AND it would be nice if you could rename 
them to have less generic names (like spi_dev instead of dev).

Thanks ! :)

Best regards,
Marek Vasut

[PATCH 3/4] mtd: spi-nor: add the framework for SPI NOR

[Huang Shijie]

This patch cloned most of the m25p80.c. In theory, it adds a new spi-nor layer.

Before this patch, the layer is like:

                   MTD
         ------------------------
                  m25p80
         ------------------------
	       spi bus driver
         ------------------------
	        SPI NOR chip

After this patch, the layer is like:
                   MTD
         ------------------------
                  spi-nor
         ------------------------
                  m25p80
         ------------------------
	       spi bus driver
         ------------------------
	       SPI NOR chip

With the spi-nor controller driver(Freescale Quadspi), it looks like:
                   MTD
         ------------------------
                  spi-nor
         ------------------------
                fsl-quadspi
         ------------------------
	       SPI NOR chip

New APIs:
   spi_nor_register: used to register a spi-nor flash.
   spi_nor_unregister: used to unregister a spi-nor flash.

The m25p80 and spi-nor controller driver should implement the hooks
   @read_reg, @write_reg, @write, @read.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/Kconfig           |    2 +
 drivers/mtd/Makefile          |    1 +
 drivers/mtd/devices/Kconfig   |    2 +-
 drivers/mtd/spi-nor/Kconfig   |    6 +
 drivers/mtd/spi-nor/Makefile  |    1 +
 drivers/mtd/spi-nor/spi-nor.c | 1057 +++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h   |    5 +
 7 files changed, 1073 insertions(+), 1 deletions(-)
 create mode 100644 drivers/mtd/spi-nor/Kconfig
 create mode 100644 drivers/mtd/spi-nor/Makefile
 create mode 100644 drivers/mtd/spi-nor/spi-nor.c

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index 5fab4e6e..8adb5af 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -320,6 +320,8 @@ source "drivers/mtd/onenand/Kconfig"
 
 source "drivers/mtd/lpddr/Kconfig"
 
+source "drivers/mtd/spi-nor/Kconfig"
+
 source "drivers/mtd/ubi/Kconfig"
 
 endif # MTD
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index 4cfb31e..40fd153 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -32,4 +32,5 @@ inftl-objs		:= inftlcore.o inftlmount.o
 
 obj-y		+= chips/ lpddr/ maps/ devices/ nand/ onenand/ tests/
 
+obj-$(CONFIG_MTD_SPI_NOR_BASE)	+= spi-nor/
 obj-$(CONFIG_MTD_UBI)		+= ubi/
diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
index 0128138..004b17b 100644
--- a/drivers/mtd/devices/Kconfig
+++ b/drivers/mtd/devices/Kconfig
@@ -80,7 +80,7 @@ config MTD_DATAFLASH_OTP
 
 config MTD_M25P80
 	tristate "Support most SPI Flash chips (AT26DF, M25P, W25X, ...)"
-	depends on SPI_MASTER
+	depends on SPI_MASTER && MTD_SPI_NOR_BASE
 	help
 	  This enables access to most modern SPI flash chips, used for
 	  program and data storage.   Series supported include Atmel AT26DF,
diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
new file mode 100644
index 0000000..41591af
--- /dev/null
+++ b/drivers/mtd/spi-nor/Kconfig
@@ -0,0 +1,6 @@
+config MTD_SPI_NOR_BASE
+	bool "the framework for SPI-NOR support"
+	depends on MTD
+	help
+	  This is the framework for the SPI NOR which can be used by the SPI
+	  device drivers and the SPI-NOR device driver.
diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
new file mode 100644
index 0000000..7dfe1f9
--- /dev/null
+++ b/drivers/mtd/spi-nor/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_MTD_SPI_NOR_BASE)	+= spi-nor.o
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
new file mode 100644
index 0000000..0e84c730
--- /dev/null
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -0,0 +1,1057 @@
+/*
+ * Cloned most of the code from the m25p80.c
+ *
+ * This code 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.
+ */
+
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/mutex.h>
+#include <linux/math64.h>
+#include <linux/slab.h>
+#include <linux/sched.h>
+#include <linux/mod_devicetable.h>
+
+#include <linux/mtd/cfi.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <linux/of_platform.h>
+#include <linux/spi/flash.h>
+#include <linux/mtd/spi-nor.h>
+
+/* Define max times to check status register before we give up. */
+#define	MAX_READY_WAIT_JIFFIES	(40 * HZ)	/* M25P16 specs 40s max chip erase */
+
+#define JEDEC_MFR(_jedec_id)	((_jedec_id) >> 16)
+
+/*
+ * Read the status register, returning its value in the location
+ * Return the status register value.
+ * Returns negative if error occurred.
+ */
+static int read_sr(struct spi_nor *flash)
+{
+	int ret;
+	u8 val;
+
+	ret = flash->read_reg(flash, OPCODE_RDSR, &val, 1);
+	if (ret < 0) {
+		pr_err("error %d reading SR\n", (int) ret);
+		return ret;
+	}
+
+	return val;
+}
+
+/*
+ * Read configuration register, returning its value in the
+ * location. Return the configuration register value.
+ * Returns negative if error occured.
+ */
+static int read_cr(struct spi_nor *flash)
+{
+	int ret;
+	u8 val;
+
+	ret = flash->read_reg(flash, OPCODE_RDCR, &val, 1);
+	if (ret < 0) {
+		dev_err(flash->dev, "error %d reading CR\n", ret);
+		return ret;
+	}
+
+	return val;
+}
+
+/*
+ * Write status register 1 byte
+ * Returns negative if error occurred.
+ */
+static inline int write_sr(struct spi_nor *flash, u8 val)
+{
+	flash->command[0] = OPCODE_WRSR;
+	flash->command[1] = val;
+	return flash->write_reg(flash, 2, 0);
+}
+
+/*
+ * Set write enable latch with Write Enable command.
+ * Returns negative if error occurred.
+ */
+static inline int write_enable(struct spi_nor *flash)
+{
+	flash->command[0] = OPCODE_WREN;
+	return flash->write_reg(flash, 1, 0);
+}
+
+/*
+ * Write status Register and configuration register with 2 bytes
+ * The first byte will be written to the status register, while the
+ * second byte will be written to the configuration register.
+ * Return negative if error occured.
+ */
+static int write_sr_cr(struct spi_nor *flash, u16 val)
+{
+	flash->command[0] = OPCODE_WRSR;
+	flash->command[1] = val & 0xff;
+	flash->command[2] = (val >> 8);
+
+	return flash->write_reg(flash, 3, 0);
+}
+
+/*
+ * Send write disble instruction to the chip.
+ */
+static inline int write_disable(struct spi_nor *flash)
+{
+	flash->command[0] = OPCODE_WRDI;
+	return flash->write_reg(flash, 1, 0);
+}
+
+static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
+{
+	return container_of(mtd, struct spi_nor, mtd);
+}
+
+/* Enable/disable 4-byte addressing mode. */
+static inline int set_4byte(struct spi_nor *flash, u32 jedec_id, int enable)
+{
+	int status;
+	bool need_wren = false;
+
+	switch (JEDEC_MFR(jedec_id)) {
+	case CFI_MFR_ST: /* Micron, actually */
+		/* Some Micron need WREN command; all will accept it */
+		need_wren = true;
+	case CFI_MFR_MACRONIX:
+	case 0xEF /* winbond */:
+		if (need_wren)
+			write_enable(flash);
+
+		flash->command[0] = enable ? OPCODE_EN4B : OPCODE_EX4B;
+		status = flash->write_reg(flash, 1, 0);
+		if (need_wren)
+			write_disable(flash);
+
+		return status;
+	default:
+		/* Spansion style */
+		flash->command[0] = OPCODE_BRWR;
+		flash->command[1] = enable << 7;
+		return flash->write_reg(flash, 2, 0);
+	}
+}
+
+/*
+ * Service routine to read status register until ready, or timeout occurs.
+ * Returns non-zero if error.
+ */
+static int wait_till_ready(struct spi_nor *flash)
+{
+	unsigned long deadline;
+	int sr;
+
+	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
+
+	do {
+		if ((sr = read_sr(flash)) < 0)
+			break;
+		else if (!(sr & SR_WIP))
+			return 0;
+
+		cond_resched();
+
+	} while (!time_after_eq(jiffies, deadline));
+
+	return 1;
+}
+
+/*
+ * Erase the whole flash memory
+ *
+ * Returns 0 if successful, non-zero otherwise.
+ */
+static int erase_chip(struct spi_nor *flash)
+{
+	pr_debug("%s: %s %lldKiB\n", dev_name(flash->dev), __func__,
+			(long long)(flash->mtd.size >> 10));
+
+	/* Wait until finished previous write command. */
+	if (wait_till_ready(flash))
+		return 1;
+
+	/* Send write enable, then erase commands. */
+	write_enable(flash);
+
+	/* Set up command buffer. */
+	flash->command[0] = OPCODE_CHIP_ERASE;
+
+	return flash->write_reg(flash, 1, 0);
+}
+
+/*
+ * Erase one sector of flash memory at offset ``offset'' which is any
+ * address within the sector which should be erased.
+ *
+ * Returns 0 if successful, non-zero otherwise.
+ */
+static int erase_sector(struct spi_nor *flash, u32 offset)
+{
+	pr_debug("%s: %s %dKiB at 0x%08x\n", dev_name(flash->dev),
+			__func__, flash->mtd.erasesize / 1024, offset);
+
+	/* Wait until finished previous write command. */
+	if (wait_till_ready(flash))
+		return 1;
+
+	/* Send write enable, then erase commands. */
+	write_enable(flash);
+
+	/* Set up command buffer. */
+	flash->command[0] = flash->erase_opcode;
+	return flash->write_reg(flash, 1, offset);
+}
+
+/*
+ * Erase an address range on the flash chip.  The address range may extend
+ * one or more erase sectors.  Return an error is there is a problem erasing.
+ */
+static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
+{
+	struct spi_nor *flash = mtd_to_spi_nor(mtd);
+	u32 addr,len;
+	uint32_t rem;
+
+	pr_debug("%s: %s at 0x%llx, len %lld\n", dev_name(flash->dev),
+			__func__, (long long)instr->addr,
+			(long long)instr->len);
+
+	div_u64_rem(instr->len, mtd->erasesize, &rem);
+	if (rem)
+		return -EINVAL;
+
+	addr = instr->addr;
+	len = instr->len;
+
+	mutex_lock(&flash->lock);
+
+	/* whole-chip erase? */
+	if (len == mtd->size) {
+		if (erase_chip(flash)) {
+			instr->state = MTD_ERASE_FAILED;
+			mutex_unlock(&flash->lock);
+			return -EIO;
+		}
+
+	/* REVISIT in some cases we could speed up erasing large regions
+	 * by using OPCODE_SE instead of OPCODE_BE_4K.  We may have set up
+	 * to use "small sector erase", but that's not always optimal.
+	 */
+
+	/* "sector"-at-a-time erase */
+	} else {
+		while (len) {
+			if (erase_sector(flash, addr)) {
+				instr->state = MTD_ERASE_FAILED;
+				mutex_unlock(&flash->lock);
+				return -EIO;
+			}
+
+			addr += mtd->erasesize;
+			len -= mtd->erasesize;
+		}
+	}
+
+	mutex_unlock(&flash->lock);
+
+	instr->state = MTD_ERASE_DONE;
+	mtd_erase_callback(instr);
+
+	return 0;
+}
+
+
+static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
+{
+	struct spi_nor *flash = mtd_to_spi_nor(mtd);
+	uint32_t offset = ofs;
+	uint8_t status_old, status_new;
+	int res = 0;
+
+	mutex_lock(&flash->lock);
+	/* Wait until finished previous command */
+	if (wait_till_ready(flash)) {
+		res = 1;
+		goto err;
+	}
+
+	status_old = read_sr(flash);
+
+	if (offset < mtd->size - (mtd->size / 2))
+		status_new = status_old | SR_BP2 | SR_BP1 | SR_BP0;
+	else if (offset < mtd->size - (mtd->size / 4))
+		status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
+	else if (offset < mtd->size - (mtd->size / 8))
+		status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
+	else if (offset < mtd->size - (mtd->size / 16))
+		status_new = (status_old & ~(SR_BP0 | SR_BP1)) | SR_BP2;
+	else if (offset < mtd->size - (mtd->size / 32))
+		status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
+	else if (offset < mtd->size - (mtd->size / 64))
+		status_new = (status_old & ~(SR_BP2 | SR_BP0)) | SR_BP1;
+	else
+		status_new = (status_old & ~(SR_BP2 | SR_BP1)) | SR_BP0;
+
+	/* Only modify protection if it will not unlock other areas */
+	if ((status_new & (SR_BP2 | SR_BP1 | SR_BP0)) >
+				(status_old & (SR_BP2 | SR_BP1 | SR_BP0))) {
+		write_enable(flash);
+		if (write_sr(flash, status_new) < 0) {
+			res = 1;
+			goto err;
+		}
+	}
+
+err:	mutex_unlock(&flash->lock);
+	return res;
+}
+
+static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
+{
+	struct spi_nor *flash = mtd_to_spi_nor(mtd);
+	uint32_t offset = ofs;
+	uint8_t status_old, status_new;
+	int res = 0;
+
+	mutex_lock(&flash->lock);
+	/* Wait until finished previous command */
+	if (wait_till_ready(flash)) {
+		res = 1;
+		goto err;
+	}
+
+	status_old = read_sr(flash);
+
+	if (offset+len > mtd->size - (mtd->size / 64))
+		status_new = status_old & ~(SR_BP2 | SR_BP1 | SR_BP0);
+	else if (offset+len > mtd->size - (mtd->size / 32))
+		status_new = (status_old & ~(SR_BP2 | SR_BP1)) | SR_BP0;
+	else if (offset+len > mtd->size - (mtd->size / 16))
+		status_new = (status_old & ~(SR_BP2 | SR_BP0)) | SR_BP1;
+	else if (offset+len > mtd->size - (mtd->size / 8))
+		status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
+	else if (offset+len > mtd->size - (mtd->size / 4))
+		status_new = (status_old & ~(SR_BP0 | SR_BP1)) | SR_BP2;
+	else if (offset+len > mtd->size - (mtd->size / 2))
+		status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
+	else
+		status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
+
+	/* Only modify protection if it will not lock other areas */
+	if ((status_new & (SR_BP2 | SR_BP1 | SR_BP0)) <
+				(status_old & (SR_BP2 | SR_BP1 | SR_BP0))) {
+		write_enable(flash);
+		if (write_sr(flash, status_new) < 0) {
+			res = 1;
+			goto err;
+		}
+	}
+
+err:	mutex_unlock(&flash->lock);
+	return res;
+}
+
+struct flash_info {
+	/* JEDEC id zero means "no ID" (most older chips); otherwise it has
+	 * a high byte of zero plus three data bytes: the manufacturer id,
+	 * then a two byte device id.
+	 */
+	u32		jedec_id;
+	u16             ext_id;
+
+	/* The size listed here is what works with OPCODE_SE, which isn't
+	 * necessarily called a "sector" by the vendor.
+	 */
+	unsigned	sector_size;
+	u16		n_sectors;
+
+	u16		page_size;
+	u16		addr_width;
+
+	u16		flags;
+#define	SECT_4K		0x01		/* OPCODE_BE_4K works uniformly */
+#define	M25P_NO_ERASE	0x02		/* No erase command needed */
+#define	SST_WRITE	0x04		/* use SST byte programming */
+#define	M25P_NO_FR	0x08		/* Can't do fastread */
+#define	SECT_4K_PMC	0x10		/* OPCODE_BE_4K_PMC works uniformly */
+#define	M25P80_QUAD_READ	0x20    /* Flash supports Quad Read */
+};
+
+#define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\
+	((kernel_ulong_t)&(struct flash_info) {				\
+		.jedec_id = (_jedec_id),				\
+		.ext_id = (_ext_id),					\
+		.sector_size = (_sector_size),				\
+		.n_sectors = (_n_sectors),				\
+		.page_size = 256,					\
+		.flags = (_flags),					\
+	})
+
+#define CAT25_INFO(_sector_size, _n_sectors, _page_size, _addr_width, _flags)	\
+	((kernel_ulong_t)&(struct flash_info) {				\
+		.sector_size = (_sector_size),				\
+		.n_sectors = (_n_sectors),				\
+		.page_size = (_page_size),				\
+		.addr_width = (_addr_width),				\
+		.flags = (_flags),					\
+	})
+
+/* NOTE: double check command sets and memory organization when you add
+ * more flash chips.  This current list focusses on newer chips, which
+ * have been converging on command sets which including JEDEC ID.
+ */
+const struct spi_device_id spi_nor_ids[] = {
+	/* Atmel -- some are (confusingly) marketed as "DataFlash" */
+	{ "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K) },
+	{ "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K) },
+
+	{ "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K) },
+	{ "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K) },
+	{ "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K) },
+
+	{ "at26f004",   INFO(0x1f0400, 0, 64 * 1024,  8, SECT_4K) },
+	{ "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16, SECT_4K) },
+	{ "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32, SECT_4K) },
+	{ "at26df321",  INFO(0x1f4700, 0, 64 * 1024, 64, SECT_4K) },
+
+	{ "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) },
+
+	/* EON -- en25xxx */
+	{ "en25f32",    INFO(0x1c3116, 0, 64 * 1024,   64, SECT_4K) },
+	{ "en25p32",    INFO(0x1c2016, 0, 64 * 1024,   64, 0) },
+	{ "en25q32b",   INFO(0x1c3016, 0, 64 * 1024,   64, 0) },
+	{ "en25p64",    INFO(0x1c2017, 0, 64 * 1024,  128, 0) },
+	{ "en25q64",    INFO(0x1c3017, 0, 64 * 1024,  128, SECT_4K) },
+	{ "en25qh256",  INFO(0x1c7019, 0, 64 * 1024,  512, 0) },
+
+	/* ESMT */
+	{ "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64, SECT_4K) },
+
+	/* Everspin */
+	{ "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, M25P_NO_ERASE | M25P_NO_FR) },
+	{ "mr25h10",  CAT25_INFO(128 * 1024, 1, 256, 3, M25P_NO_ERASE | M25P_NO_FR) },
+
+	/* GigaDevice */
+	{ "gd25q32", INFO(0xc84016, 0, 64 * 1024,  64, SECT_4K) },
+	{ "gd25q64", INFO(0xc84017, 0, 64 * 1024, 128, SECT_4K) },
+
+	/* Intel/Numonyx -- xxxs33b */
+	{ "160s33b",  INFO(0x898911, 0, 64 * 1024,  32, 0) },
+	{ "320s33b",  INFO(0x898912, 0, 64 * 1024,  64, 0) },
+	{ "640s33b",  INFO(0x898913, 0, 64 * 1024, 128, 0) },
+
+	/* Macronix */
+	{ "mx25l2005a",  INFO(0xc22012, 0, 64 * 1024,   4, SECT_4K) },
+	{ "mx25l4005a",  INFO(0xc22013, 0, 64 * 1024,   8, SECT_4K) },
+	{ "mx25l8005",   INFO(0xc22014, 0, 64 * 1024,  16, 0) },
+	{ "mx25l1606e",  INFO(0xc22015, 0, 64 * 1024,  32, SECT_4K) },
+	{ "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, 0) },
+	{ "mx25l3255e",  INFO(0xc29e16, 0, 64 * 1024,  64, SECT_4K) },
+	{ "mx25l6405d",  INFO(0xc22017, 0, 64 * 1024, 128, 0) },
+	{ "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, 0) },
+	{ "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
+	{ "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) },
+	{ "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
+	{ "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, M25P80_QUAD_READ) },
+
+	/* Micron */
+	{ "n25q064",     INFO(0x20ba17, 0, 64 * 1024,  128, 0) },
+	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, 0) },
+	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
+	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
+	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
+
+	/* PMC */
+	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
+	{ "pm25lv010",   INFO(0,        0, 32 * 1024,    4, SECT_4K_PMC) },
+	{ "pm25lq032",   INFO(0x7f9d46, 0, 64 * 1024,   64, SECT_4K) },
+
+	/* Spansion -- single (large) sector size only, at least
+	 * for the chips listed here (without boot sectors).
+	 */
+	{ "s25sl032p",  INFO(0x010215, 0x4d00,  64 * 1024,  64, 0) },
+	{ "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, 0) },
+	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, 0) },
+	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, M25P80_QUAD_READ) },
+	{ "s25fl512s",  INFO(0x010220, 0x4d00, 256 * 1024, 256, 0) },
+	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
+	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
+	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
+	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, 0) },
+	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
+	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
+	{ "s25sl008a",  INFO(0x010213,      0,  64 * 1024,  16, 0) },
+	{ "s25sl016a",  INFO(0x010214,      0,  64 * 1024,  32, 0) },
+	{ "s25sl032a",  INFO(0x010215,      0,  64 * 1024,  64, 0) },
+	{ "s25sl064a",  INFO(0x010216,      0,  64 * 1024, 128, 0) },
+	{ "s25fl016k",  INFO(0xef4015,      0,  64 * 1024,  32, SECT_4K) },
+	{ "s25fl064k",  INFO(0xef4017,      0,  64 * 1024, 128, SECT_4K) },
+
+	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
+	{ "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8, SECT_4K | SST_WRITE) },
+	{ "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | SST_WRITE) },
+	{ "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | SST_WRITE) },
+	{ "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | SST_WRITE) },
+	{ "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K) },
+	{ "sst25wf512",  INFO(0xbf2501, 0, 64 * 1024,  1, SECT_4K | SST_WRITE) },
+	{ "sst25wf010",  INFO(0xbf2502, 0, 64 * 1024,  2, SECT_4K | SST_WRITE) },
+	{ "sst25wf020",  INFO(0xbf2503, 0, 64 * 1024,  4, SECT_4K | SST_WRITE) },
+	{ "sst25wf040",  INFO(0xbf2504, 0, 64 * 1024,  8, SECT_4K | SST_WRITE) },
+
+	/* ST Microelectronics -- newer production may have feature updates */
+	{ "m25p05",  INFO(0x202010,  0,  32 * 1024,   2, 0) },
+	{ "m25p10",  INFO(0x202011,  0,  32 * 1024,   4, 0) },
+	{ "m25p20",  INFO(0x202012,  0,  64 * 1024,   4, 0) },
+	{ "m25p40",  INFO(0x202013,  0,  64 * 1024,   8, 0) },
+	{ "m25p80",  INFO(0x202014,  0,  64 * 1024,  16, 0) },
+	{ "m25p16",  INFO(0x202015,  0,  64 * 1024,  32, 0) },
+	{ "m25p32",  INFO(0x202016,  0,  64 * 1024,  64, 0) },
+	{ "m25p64",  INFO(0x202017,  0,  64 * 1024, 128, 0) },
+	{ "m25p128", INFO(0x202018,  0, 256 * 1024,  64, 0) },
+	{ "n25q032", INFO(0x20ba16,  0,  64 * 1024,  64, 0) },
+
+	{ "m25p05-nonjedec",  INFO(0, 0,  32 * 1024,   2, 0) },
+	{ "m25p10-nonjedec",  INFO(0, 0,  32 * 1024,   4, 0) },
+	{ "m25p20-nonjedec",  INFO(0, 0,  64 * 1024,   4, 0) },
+	{ "m25p40-nonjedec",  INFO(0, 0,  64 * 1024,   8, 0) },
+	{ "m25p80-nonjedec",  INFO(0, 0,  64 * 1024,  16, 0) },
+	{ "m25p16-nonjedec",  INFO(0, 0,  64 * 1024,  32, 0) },
+	{ "m25p32-nonjedec",  INFO(0, 0,  64 * 1024,  64, 0) },
+	{ "m25p64-nonjedec",  INFO(0, 0,  64 * 1024, 128, 0) },
+	{ "m25p128-nonjedec", INFO(0, 0, 256 * 1024,  64, 0) },
+
+	{ "m45pe10", INFO(0x204011,  0, 64 * 1024,    2, 0) },
+	{ "m45pe80", INFO(0x204014,  0, 64 * 1024,   16, 0) },
+	{ "m45pe16", INFO(0x204015,  0, 64 * 1024,   32, 0) },
+
+	{ "m25pe20", INFO(0x208012,  0, 64 * 1024,  4,       0) },
+	{ "m25pe80", INFO(0x208014,  0, 64 * 1024, 16,       0) },
+	{ "m25pe16", INFO(0x208015,  0, 64 * 1024, 32, SECT_4K) },
+
+	{ "m25px32",    INFO(0x207116,  0, 64 * 1024, 64, SECT_4K) },
+	{ "m25px32-s0", INFO(0x207316,  0, 64 * 1024, 64, SECT_4K) },
+	{ "m25px32-s1", INFO(0x206316,  0, 64 * 1024, 64, SECT_4K) },
+	{ "m25px64",    INFO(0x207117,  0, 64 * 1024, 128, 0) },
+
+	/* Winbond -- w25x "blocks" are 64K, "sectors" are 4KiB */
+	{ "w25x10", INFO(0xef3011, 0, 64 * 1024,  2,  SECT_4K) },
+	{ "w25x20", INFO(0xef3012, 0, 64 * 1024,  4,  SECT_4K) },
+	{ "w25x40", INFO(0xef3013, 0, 64 * 1024,  8,  SECT_4K) },
+	{ "w25x80", INFO(0xef3014, 0, 64 * 1024,  16, SECT_4K) },
+	{ "w25x16", INFO(0xef3015, 0, 64 * 1024,  32, SECT_4K) },
+	{ "w25x32", INFO(0xef3016, 0, 64 * 1024,  64, SECT_4K) },
+	{ "w25q32", INFO(0xef4016, 0, 64 * 1024,  64, SECT_4K) },
+	{ "w25q32dw", INFO(0xef6016, 0, 64 * 1024,  64, SECT_4K) },
+	{ "w25x64", INFO(0xef3017, 0, 64 * 1024, 128, SECT_4K) },
+	{ "w25q64", INFO(0xef4017, 0, 64 * 1024, 128, SECT_4K) },
+	{ "w25q128", INFO(0xef4018, 0, 64 * 1024, 256, SECT_4K) },
+	{ "w25q80", INFO(0xef5014, 0, 64 * 1024,  16, SECT_4K) },
+	{ "w25q80bl", INFO(0xef4014, 0, 64 * 1024,  16, SECT_4K) },
+	{ "w25q128", INFO(0xef4018, 0, 64 * 1024, 256, SECT_4K) },
+	{ "w25q256", INFO(0xef4019, 0, 64 * 1024, 512, SECT_4K) },
+
+	/* Catalyst / On Semiconductor -- non-JEDEC */
+	{ "cat25c11", CAT25_INFO(  16, 8, 16, 1, M25P_NO_ERASE | M25P_NO_FR) },
+	{ "cat25c03", CAT25_INFO(  32, 8, 16, 2, M25P_NO_ERASE | M25P_NO_FR) },
+	{ "cat25c09", CAT25_INFO( 128, 8, 32, 2, M25P_NO_ERASE | M25P_NO_FR) },
+	{ "cat25c17", CAT25_INFO( 256, 8, 32, 2, M25P_NO_ERASE | M25P_NO_FR) },
+	{ "cat25128", CAT25_INFO(2048, 8, 64, 2, M25P_NO_ERASE | M25P_NO_FR) },
+	{ },
+};
+
+static const struct spi_device_id *jedec_probe(struct spi_nor *flash)
+{
+	int			tmp;
+	u8			id[5];
+	u32			jedec;
+	u16                     ext_jedec;
+	struct flash_info	*info;
+
+	tmp = flash->read_reg(flash, OPCODE_RDID, id, 5);
+	if (tmp < 0) {
+		pr_debug(" error %d reading JEDEC ID\n", tmp);
+		return ERR_PTR(tmp);
+	}
+	jedec = id[0];
+	jedec = jedec << 8;
+	jedec |= id[1];
+	jedec = jedec << 8;
+	jedec |= id[2];
+
+	ext_jedec = id[3] << 8 | id[4];
+
+	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
+		info = (void *)spi_nor_ids[tmp].driver_data;
+		if (info->jedec_id == jedec) {
+			if (info->ext_id != 0 && info->ext_id != ext_jedec)
+				continue;
+			return &spi_nor_ids[tmp];
+		}
+	}
+	pr_err("unrecognized JEDEC id %06x\n", jedec);
+	return ERR_PTR(-ENODEV);
+}
+
+static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
+			size_t *retlen, u_char *buf)
+{
+	struct spi_nor *flash = mtd_to_spi_nor(mtd);
+	int ret;
+
+	pr_debug("%s: %s from 0x%08x, len %zd\n", dev_name(flash->dev),
+			__func__, (u32)from, len);
+
+	mutex_lock(&flash->lock);
+
+	/* Wait till previous write/erase is done. */
+	if (wait_till_ready(flash)) {
+		/* REVISIT status return?? */
+		mutex_unlock(&flash->lock);
+		return 1;
+	}
+
+	ret = flash->read(flash, from, len, retlen, buf);
+
+	mutex_unlock(&flash->lock);
+	return ret;
+}
+
+static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
+		size_t *retlen, const u_char *buf)
+{
+	struct spi_nor *flash = mtd_to_spi_nor(mtd);
+	size_t actual;
+	int ret;
+
+	pr_debug("%s: %s to 0x%08x, len %zd\n", dev_name(flash->dev),
+			__func__, (u32)to, len);
+
+	mutex_lock(&flash->lock);
+
+	/* Wait until finished previous write command. */
+	ret = wait_till_ready(flash);
+	if (ret)
+		goto time_out;
+
+	write_enable(flash);
+
+	flash->sst_write_second = false;
+
+	actual = to % 2;
+	/* Start write from odd address. */
+	if (actual) {
+		flash->program_opcode = OPCODE_BP;
+
+		/* write one byte. */
+		flash->write(flash, to, 1, retlen, buf);
+		ret = wait_till_ready(flash);
+		if (ret)
+			goto time_out;
+	}
+	to += actual;
+
+	/* Write out most of the data here. */
+	for (; actual < len - 1; actual += 2) {
+		flash->program_opcode = OPCODE_AAI_WP;
+
+		/* write two bytes. */
+		flash->write(flash, to, 2, retlen, buf + actual);
+		ret = wait_till_ready(flash);
+		if (ret)
+			goto time_out;
+		to += 2;
+		flash->sst_write_second = true;
+	}
+	flash->sst_write_second = false;
+
+	write_disable(flash);
+	ret = wait_till_ready(flash);
+	if (ret)
+		goto time_out;
+
+	/* Write out trailing byte if it exists. */
+	if (actual != len) {
+		write_enable(flash);
+
+		flash->program_opcode = OPCODE_BP;
+		flash->write(flash, to, 1, retlen, buf + actual);
+
+		ret = wait_till_ready(flash);
+		if (ret)
+			goto time_out;
+		write_disable(flash);
+	}
+time_out:
+	mutex_unlock(&flash->lock);
+	return ret;
+}
+
+/*
+ * Write an address range to the flash chip.  Data must be written in
+ * FLASH_PAGESIZE chunks.  The address range may be any size provided
+ * it is within the physical boundaries.
+ */
+static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
+	size_t *retlen, const u_char *buf)
+{
+	struct spi_nor *flash = mtd_to_spi_nor(mtd);
+	u32 page_offset, page_size, i;
+
+	pr_debug("%s: %s to 0x%08x, len %zd\n", dev_name(flash->dev),
+			__func__, (u32)to, len);
+
+	mutex_lock(&flash->lock);
+
+	/* Wait until finished previous write command. */
+	if (wait_till_ready(flash)) {
+		mutex_unlock(&flash->lock);
+		return 1;
+	}
+	write_enable(flash);
+
+	page_offset = to & (flash->page_size - 1);
+
+	/* do all the bytes fit onto one page? */
+	if (page_offset + len <= flash->page_size) {
+		flash->write(flash, to, len, retlen, buf);
+	} else {
+		/* the size of data remaining on the first page */
+		page_size = flash->page_size - page_offset;
+		flash->write(flash, to, page_size, retlen, buf);
+
+		/* write everything in flash->page_size chunks */
+		for (i = page_size; i < len; i += page_size) {
+			page_size = len - i;
+			if (page_size > flash->page_size)
+				page_size = flash->page_size;
+
+			wait_till_ready(flash);
+			write_enable(flash);
+
+			flash->write(flash, to + i, page_size, retlen, buf + i);
+		}
+	}
+
+	mutex_unlock(&flash->lock);
+	return 0;
+}
+
+static int macronix_quad_enable(struct spi_nor *flash)
+{
+	int ret, val;
+
+	val = read_sr(flash);
+	write_enable(flash);
+
+	flash->command[0] = OPCODE_WRSR;
+	flash->command[1] = val | SR_QUAD_EN_MX;
+	flash->write_reg(flash, 2, 0);
+
+	if (wait_till_ready(flash))
+		return 1;
+
+	ret = read_sr(flash);
+	if (!(ret > 0 && (ret & SR_QUAD_EN_MX))) {
+		dev_err(flash->dev, "Macronix Quad bit not set\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int spansion_quad_enable(struct spi_nor *flash)
+{
+	int ret;
+	int quad_en = CR_QUAD_EN_SPAN << 8;
+
+	write_enable(flash);
+
+	ret = write_sr_cr(flash, quad_en);
+	if (ret < 0) {
+		dev_err(flash->dev,
+			"error while writing configuration register\n");
+		return -EINVAL;
+	}
+
+	/* read back and check it */
+	ret = read_cr(flash);
+	if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) {
+		dev_err(flash->dev, "Spansion Quad bit not set\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int set_quad_mode(struct spi_nor *flash, u32 jedec_id)
+{
+	int status;
+
+	switch (JEDEC_MFR(jedec_id)) {
+	case CFI_MFR_MACRONIX:
+		status = macronix_quad_enable(flash);
+		if (status) {
+			dev_err(flash->dev, "Macronix quad-read not enabled\n");
+			return -EINVAL;
+		}
+		return status;
+	default:
+		status = spansion_quad_enable(flash);
+		if (status) {
+			dev_err(flash->dev, "Spansion quad-read not enabled\n");
+			return -EINVAL;
+		}
+		return status;
+	}
+}
+
+static int spi_nor_check(struct spi_nor *flash)
+{
+	if (!flash->dev || !flash->read || !flash->write ||
+		!flash->read_reg || !flash->write_reg) {
+		pr_err("spi-nor: please fill all the necessary fields!\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+
+int spi_nor_register(struct spi_nor *flash, const struct spi_device_id *id,
+			bool quad_read)
+{
+	struct mtd_part_parser_data	ppdata;
+	struct flash_info		*info;
+	struct flash_platform_data	*data;
+	struct device *dev = flash->dev;
+	struct mtd_info *mtd = &flash->mtd;
+	struct device_node *np = dev->of_node;
+	int ret;
+	int i;
+
+	ret = spi_nor_check(flash);
+	if (ret)
+		return ret;
+
+	/* Platform data helps sort out which chip type we have, as
+	 * well as how this board partitions it.  If we don't have
+	 * a chip ID, try the JEDEC id commands; they'll work for most
+	 * newer chips, even if we don't recognize the particular chip.
+	 */
+	data = dev_get_platdata(dev);
+	if (data && data->type) {
+		const struct spi_device_id *plat_id;
+
+		for (i = 0; i < ARRAY_SIZE(spi_nor_ids) - 1; i++) {
+			plat_id = &spi_nor_ids[i];
+			if (strcmp(data->type, plat_id->name))
+				continue;
+			break;
+		}
+
+		if (i < ARRAY_SIZE(spi_nor_ids) - 1)
+			id = plat_id;
+		else
+			dev_warn(dev, "unrecognized id %s\n", data->type);
+	}
+
+	info = (void *)id->driver_data;
+
+	if (info->jedec_id) {
+		const struct spi_device_id *jid;
+
+		jid = jedec_probe(flash);
+		if (IS_ERR(jid)) {
+			return PTR_ERR(jid);
+		} else if (jid != id) {
+			/*
+			 * JEDEC knows better, so overwrite platform ID. We
+			 * can't trust partitions any longer, but we'll let
+			 * mtd apply them anyway, since some partitions may be
+			 * marked read-only, and we don't want to lose that
+			 * information, even if it's not 100% accurate.
+			 */
+			dev_warn(dev, "found %s, expected %s\n",
+				 jid->name, id->name);
+			id = jid;
+			info = (void *)jid->driver_data;
+		}
+	}
+
+	mutex_init(&flash->lock);
+
+	/*
+	 * Atmel, SST and Intel/Numonyx serial flash tend to power
+	 * up with the software protection bits set
+	 */
+
+	if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ATMEL ||
+	    JEDEC_MFR(info->jedec_id) == CFI_MFR_INTEL ||
+	    JEDEC_MFR(info->jedec_id) == CFI_MFR_SST) {
+		write_enable(flash);
+		write_sr(flash, 0);
+	}
+
+	if (data && data->name)
+		flash->mtd.name = data->name;
+	else
+		flash->mtd.name = dev_name(dev);
+
+	flash->mtd.type = MTD_NORFLASH;
+	flash->mtd.writesize = 1;
+	flash->mtd.flags = MTD_CAP_NORFLASH;
+	flash->mtd.size = info->sector_size * info->n_sectors;
+	flash->mtd._erase = spi_nor_erase;
+	flash->mtd._read = spi_nor_read;
+
+	/* flash protection support for STmicro chips */
+	if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) {
+		flash->mtd._lock = spi_nor_lock;
+		flash->mtd._unlock = spi_nor_unlock;
+	}
+
+	/* sst flash chips use AAI word program */
+	if (info->flags & SST_WRITE)
+		flash->mtd._write = sst_write;
+	else
+		flash->mtd._write = spi_nor_write;
+
+	/* prefer "small sector" erase if possible */
+	if (info->flags & SECT_4K) {
+		flash->erase_opcode = OPCODE_BE_4K;
+		flash->mtd.erasesize = 4096;
+	} else if (info->flags & SECT_4K_PMC) {
+		flash->erase_opcode = OPCODE_BE_4K_PMC;
+		flash->mtd.erasesize = 4096;
+	} else {
+		flash->erase_opcode = OPCODE_SE;
+		flash->mtd.erasesize = info->sector_size;
+	}
+
+	if (info->flags & M25P_NO_ERASE)
+		flash->mtd.flags |= MTD_NO_ERASE;
+
+	ppdata.of_node = dev->of_node;
+	flash->mtd.dev.parent = dev;
+	flash->page_size = info->page_size;
+	flash->mtd.writebufsize = flash->page_size;
+
+	if (np) {
+		/* If we were instantiated by DT, use it */
+		if (of_property_read_bool(np, "m25p,fast-read"))
+			flash->flash_read = M25P80_FAST;
+	} else {
+		/* If we weren't instantiated by DT, default to fast-read */
+		flash->flash_read = M25P80_FAST;
+	}
+
+	/* Some devices cannot do fast-read, no matter what DT tells us */
+	if (info->flags & M25P_NO_FR)
+		flash->flash_read = M25P80_NORMAL;
+
+	/* Quad-read mode takes precedence over fast/normal */
+	if (quad_read && info->flags & M25P80_QUAD_READ) {
+		ret = set_quad_mode(flash, info->jedec_id);
+		if (ret) {
+			dev_err(dev, "quad mode not supported\n");
+			return ret;
+		}
+		flash->flash_read = M25P80_QUAD;
+	}
+
+	/* Default commands */
+	switch (flash->flash_read) {
+	case M25P80_QUAD:
+		flash->read_opcode = OPCODE_QUAD_READ;
+		break;
+	case M25P80_FAST:
+		flash->read_opcode = OPCODE_FAST_READ;
+		break;
+	case M25P80_NORMAL:
+		flash->read_opcode = OPCODE_NORM_READ;
+		break;
+	default:
+		dev_err(dev, "No Read opcode defined\n");
+		return -EINVAL;
+	}
+
+	flash->program_opcode = OPCODE_PP;
+
+	if (info->addr_width)
+		flash->addr_width = info->addr_width;
+	else if (flash->mtd.size > 0x1000000) {
+		/* enable 4-byte addressing if the device exceeds 16MiB */
+		flash->addr_width = 4;
+		if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) {
+			/* Dedicated 4-byte command set */
+			switch (flash->flash_read) {
+			case M25P80_QUAD:
+				flash->read_opcode = OPCODE_QUAD_READ;
+				break;
+			case M25P80_FAST:
+				flash->read_opcode = OPCODE_FAST_READ_4B;
+				break;
+			case M25P80_NORMAL:
+				flash->read_opcode = OPCODE_NORM_READ_4B;
+				break;
+			}
+			flash->program_opcode = OPCODE_PP_4B;
+			/* No small sector erase for 4-byte command set */
+			flash->erase_opcode = OPCODE_SE_4B;
+			flash->mtd.erasesize = info->sector_size;
+		} else
+			set_4byte(flash, info->jedec_id, 1);
+	} else {
+		flash->addr_width = 3;
+	}
+
+	dev_info(dev, "%s (%lld Kbytes)\n", id->name,
+			(long long)flash->mtd.size >> 10);
+
+	pr_debug("mtd .name = %s, .size = 0x%llx (%lldMiB) "
+			".erasesize = 0x%.8x (%uKiB) .numeraseregions = %d\n",
+		flash->mtd.name,
+		(long long)flash->mtd.size, (long long)(flash->mtd.size >> 20),
+		flash->mtd.erasesize, flash->mtd.erasesize / 1024,
+		flash->mtd.numeraseregions);
+
+	if (flash->mtd.numeraseregions)
+		for (i = 0; i < flash->mtd.numeraseregions; i++)
+			pr_debug("mtd.eraseregions[%d] = { .offset = 0x%llx, "
+				".erasesize = 0x%.8x (%uKiB), "
+				".numblocks = %d }\n",
+				i, (long long)flash->mtd.eraseregions[i].offset,
+				flash->mtd.eraseregions[i].erasesize,
+				flash->mtd.eraseregions[i].erasesize / 1024,
+				flash->mtd.eraseregions[i].numblocks);
+
+
+	/* partitions should match sector boundaries; and it may be good to
+	 * use readonly partitions for writeprotected sectors (BP2..BP0).
+	 */
+	return mtd_device_parse_register(mtd, NULL, &ppdata,
+			data ? data->parts : NULL,
+			data ? data->nr_parts : 0);
+}
+
+int spi_nor_unregister(struct spi_nor *flash)
+{
+	return mtd_device_unregister(&flash->mtd);
+}
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Freescale Semiconductor Inc.");
+MODULE_DESCRIPTION("framework for SPI NOR flash");
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 8da1f69..2d6dc56 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -93,4 +93,9 @@ struct spi_nor {
 	int (*read)(struct spi_nor *flash, loff_t from, size_t len,
 			size_t *retlen, u_char *buf);
 };
+
+int spi_nor_register(struct spi_nor *flash, const struct spi_device_id *id,
+			bool quad_read);
+int spi_nor_unregister(struct spi_nor *flash);
+extern const struct spi_device_id spi_nor_ids[];
 #endif
-- 
1.7.2.rc3

RE: [PATCH 3/4] mtd: spi-nor: add the framework for SPI NOR

[Gupta, Pekon]

> From: Huang Shijie [mailto:b32955@freescale.com]
> This patch cloned most of the m25p80.c. In theory, it adds a new spi-nor
> layer.
[...]
> Signed-off-by: Huang Shijie <b32955@freescale.com>
>
Is there a way we can also original m25p80 contributors' sign-off also?
David/Brian, can you suggest ?


With regards, pekon

Re: [PATCH 3/4] mtd: spi-nor: add the framework for SPI NOR

[Marek Vasut]

Dear Huang Shijie,

> This patch cloned most of the m25p80.c. In theory, it adds a new spi-nor
> layer.
> 
> Before this patch, the layer is like:
> 
>                    MTD
>          ------------------------
>                   m25p80
>          ------------------------
> 	       spi bus driver
>          ------------------------
> 	        SPI NOR chip
> 
> After this patch, the layer is like:
>                    MTD
>          ------------------------
>                   spi-nor
>          ------------------------
>                   m25p80
>          ------------------------
> 	       spi bus driver
>          ------------------------
> 	       SPI NOR chip
> 
> With the spi-nor controller driver(Freescale Quadspi), it looks like:
>                    MTD
>          ------------------------
>                   spi-nor
>          ------------------------
>                 fsl-quadspi
>          ------------------------
> 	       SPI NOR chip
> 
> New APIs:
>    spi_nor_register: used to register a spi-nor flash.
>    spi_nor_unregister: used to unregister a spi-nor flash.
> 
> The m25p80 and spi-nor controller driver should implement the hooks
>    @read_reg, @write_reg, @write, @read.

I would approach this the other way around -- first I'd split the common code 
out from m25p80 driver and then I'd add my own driver which uses the common 
code. Then we won't have to review the common code (or such a large chunk of it 
anyway) again.

But I am still not too sure we need this new framework at all, I will wait for 
your reply on 0/4 .

Cheers!

Best regards,
Marek Vasut

[PATCH 4/4] mtd: m25p80: use the new spi-nor APIs

[Huang Shijie]

This patch removes all the common code(including the m25p_ids) which has
already been cloned to spi-nor.c, and implements the necessary hooks for
spi_nor{}.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/devices/m25p80.c | 1193 +++---------------------------------------
 1 files changed, 69 insertions(+), 1124 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 13d9864..9984e37 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -36,282 +36,43 @@
 #include <linux/spi/flash.h>
 #include <linux/mtd/spi-nor.h>
 
-/* Define max times to check status register before we give up. */
-#define	MAX_READY_WAIT_JIFFIES	(40 * HZ)	/* M25P16 specs 40s max chip erase */
-#define	MAX_CMD_SIZE		6
-
-#define JEDEC_MFR(_jedec_id)	((_jedec_id) >> 16)
-
-/****************************************************************************/
-
-struct m25p {
-	struct spi_device	*spi;
-	struct mutex		lock;
-	struct mtd_info		mtd;
-	u16			page_size;
-	u16			addr_width;
-	u8			erase_opcode;
-	u8			read_opcode;
-	u8			program_opcode;
-	u8			*command;
-	enum read_type		flash_read;
-};
-
-static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
-{
-	return container_of(mtd, struct m25p, mtd);
-}
-
-/****************************************************************************/
-
-/*
- * Internal helper functions
- */
-
-/*
- * Read the status register, returning its value in the location
- * Return the status register value.
- * Returns negative if error occurred.
- */
-static int read_sr(struct m25p *flash)
-{
-	ssize_t retval;
-	u8 code = OPCODE_RDSR;
-	u8 val;
-
-	retval = spi_write_then_read(flash->spi, &code, 1, &val, 1);
-
-	if (retval < 0) {
-		dev_err(&flash->spi->dev, "error %d reading SR\n",
-				(int) retval);
-		return retval;
-	}
-
-	return val;
-}
-
-/*
- * Read configuration register, returning its value in the
- * location. Return the configuration register value.
- * Returns negative if error occured.
- */
-static int read_cr(struct m25p *flash)
-{
-	u8 code = OPCODE_RDCR;
-	int ret;
-	u8 val;
-
-	ret = spi_write_then_read(flash->spi, &code, 1, &val, 1);
-	if (ret < 0) {
-		dev_err(&flash->spi->dev, "error %d reading CR\n", ret);
-		return ret;
-	}
-
-	return val;
-}
-
 /*
- * Write status register 1 byte
- * Returns negative if error occurred.
- */
-static int write_sr(struct m25p *flash, u8 val)
-{
-	flash->command[0] = OPCODE_WRSR;
-	flash->command[1] = val;
-
-	return spi_write(flash->spi, flash->command, 2);
-}
-
-/*
- * Set write enable latch with Write Enable command.
- * Returns negative if error occurred.
- */
-static inline int write_enable(struct m25p *flash)
-{
-	u8	code = OPCODE_WREN;
-
-	return spi_write_then_read(flash->spi, &code, 1, NULL, 0);
-}
-
-/*
- * Send write disble instruction to the chip.
- */
-static inline int write_disable(struct m25p *flash)
-{
-	u8	code = OPCODE_WRDI;
-
-	return spi_write_then_read(flash->spi, &code, 1, NULL, 0);
-}
-
-/*
- * Enable/disable 4-byte addressing mode.
+ * Dummy Cycle calculation for different type of read.
+ * It can be used to support more commands with
+ * different dummy cycle requirements.
  */
-static inline int set_4byte(struct m25p *flash, u32 jedec_id, int enable)
+static inline int m25p80_dummy_cycles_read(struct spi_nor *flash)
 {
-	int status;
-	bool need_wren = false;
-
-	switch (JEDEC_MFR(jedec_id)) {
-	case CFI_MFR_ST: /* Micron, actually */
-		/* Some Micron need WREN command; all will accept it */
-		need_wren = true;
-	case CFI_MFR_MACRONIX:
-	case 0xEF /* winbond */:
-		if (need_wren)
-			write_enable(flash);
-
-		flash->command[0] = enable ? OPCODE_EN4B : OPCODE_EX4B;
-		status = spi_write(flash->spi, flash->command, 1);
-
-		if (need_wren)
-			write_disable(flash);
-
-		return status;
+	switch (flash->flash_read) {
+	case M25P80_FAST:
+	case M25P80_QUAD:
+		return 1;
+	case M25P80_NORMAL:
+		return 0;
 	default:
-		/* Spansion style */
-		flash->command[0] = OPCODE_BRWR;
-		flash->command[1] = enable << 7;
-		return spi_write(flash->spi, flash->command, 2);
+		dev_err(flash->dev, "No valid read type supported\n");
+		return -1;
 	}
 }
 
-/*
- * Service routine to read status register until ready, or timeout occurs.
- * Returns non-zero if error.
- */
-static int wait_till_ready(struct m25p *flash)
+static inline struct spi_device *m25p_get_spi(struct spi_nor *flash)
 {
-	unsigned long deadline;
-	int sr;
-
-	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
-
-	do {
-		if ((sr = read_sr(flash)) < 0)
-			break;
-		else if (!(sr & SR_WIP))
-			return 0;
-
-		cond_resched();
-
-	} while (!time_after_eq(jiffies, deadline));
-
-	return 1;
+	return container_of(flash->dev, struct spi_device, dev);
 }
 
-/*
- * Write status Register and configuration register with 2 bytes
- * The first byte will be written to the status register, while the
- * second byte will be written to the configuration register.
- * Return negative if error occured.
- */
-static int write_sr_cr(struct m25p *flash, u16 val)
-{
-	flash->command[0] = OPCODE_WRSR;
-	flash->command[1] = val & 0xff;
-	flash->command[2] = (val >> 8);
-
-	return spi_write(flash->spi, flash->command, 3);
-}
-
-static int macronix_quad_enable(struct m25p *flash)
-{
-	int ret, val;
-	u8 cmd[2];
-	cmd[0] = OPCODE_WRSR;
-
-	val = read_sr(flash);
-	cmd[1] = val | SR_QUAD_EN_MX;
-	write_enable(flash);
-
-	spi_write(flash->spi, &cmd, 2);
-
-	if (wait_till_ready(flash))
-		return 1;
-
-	ret = read_sr(flash);
-	if (!(ret > 0 && (ret & SR_QUAD_EN_MX))) {
-		dev_err(&flash->spi->dev, "Macronix Quad bit not set\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-static int spansion_quad_enable(struct m25p *flash)
+static int m25p_read_reg(struct spi_nor *flash, u8 code, u8 *val, int len)
 {
 	int ret;
-	int quad_en = CR_QUAD_EN_SPAN << 8;
-
-	write_enable(flash);
-
-	ret = write_sr_cr(flash, quad_en);
-	if (ret < 0) {
-		dev_err(&flash->spi->dev,
-			"error while writing configuration register\n");
-		return -EINVAL;
-	}
+	struct spi_device *spi = m25p_get_spi(flash);
 
-	/* read back and check it */
-	ret = read_cr(flash);
-	if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) {
-		dev_err(&flash->spi->dev, "Spansion Quad bit not set\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-static int set_quad_mode(struct m25p *flash, u32 jedec_id)
-{
-	int status;
+	ret = spi_write_then_read(spi, &code, 1, val, len);
+	if (ret < 0)
+		dev_err(&spi->dev, "error %d reading %x\n", ret, code);
 
-	switch (JEDEC_MFR(jedec_id)) {
-	case CFI_MFR_MACRONIX:
-		status = macronix_quad_enable(flash);
-		if (status) {
-			dev_err(&flash->spi->dev,
-				"Macronix quad-read not enabled\n");
-			return -EINVAL;
-		}
-		return status;
-	default:
-		status = spansion_quad_enable(flash);
-		if (status) {
-			dev_err(&flash->spi->dev,
-				"Spansion quad-read not enabled\n");
-			return -EINVAL;
-		}
-		return status;
-	}
+	return ret ;
 }
 
-/*
- * Erase the whole flash memory
- *
- * Returns 0 if successful, non-zero otherwise.
- */
-static int erase_chip(struct m25p *flash)
-{
-	pr_debug("%s: %s %lldKiB\n", dev_name(&flash->spi->dev), __func__,
-			(long long)(flash->mtd.size >> 10));
-
-	/* Wait until finished previous write command. */
-	if (wait_till_ready(flash))
-		return 1;
-
-	/* Send write enable, then erase commands. */
-	write_enable(flash);
-
-	/* Set up command buffer. */
-	flash->command[0] = OPCODE_CHIP_ERASE;
-
-	spi_write(flash->spi, flash->command, 1);
-
-	return 0;
-}
-
-static void m25p_addr2cmd(struct m25p *flash, unsigned int addr, u8 *cmd)
+static void m25p_addr2cmd(struct spi_nor *flash, unsigned int addr, u8 *cmd)
 {
 	/* opcode is in cmd[0] */
 	cmd[1] = addr >> (flash->addr_width * 8 -  8);
@@ -320,143 +81,72 @@ static void m25p_addr2cmd(struct m25p *flash, unsigned int addr, u8 *cmd)
 	cmd[4] = addr >> (flash->addr_width * 8 - 32);
 }
 
-static int m25p_cmdsz(struct m25p *flash)
+static int m25p_cmdsz(struct spi_nor *flash)
 {
 	return 1 + flash->addr_width;
 }
 
-/*
- * Erase one sector of flash memory at offset ``offset'' which is any
- * address within the sector which should be erased.
- *
- * Returns 0 if successful, non-zero otherwise.
- */
-static int erase_sector(struct m25p *flash, u32 offset)
+static int m25p_write_reg(struct spi_nor *flash, int cmd_len, unsigned int addr)
 {
-	pr_debug("%s: %s %dKiB at 0x%08x\n", dev_name(&flash->spi->dev),
-			__func__, flash->mtd.erasesize / 1024, offset);
-
-	/* Wait until finished previous write command. */
-	if (wait_till_ready(flash))
-		return 1;
-
-	/* Send write enable, then erase commands. */
-	write_enable(flash);
-
-	/* Set up command buffer. */
-	flash->command[0] = flash->erase_opcode;
-	m25p_addr2cmd(flash, offset, flash->command);
-
-	spi_write(flash->spi, flash->command, m25p_cmdsz(flash));
+	/* set the address for erase. */
+	if (flash->command[0] == flash->erase_opcode) {
+		m25p_addr2cmd(flash, addr, flash->command);
+		cmd_len = m25p_cmdsz(flash);
+	}
 
-	return 0;
+	return spi_write(m25p_get_spi(flash), flash->command, cmd_len);
 }
 
-/****************************************************************************/
-
-/*
- * MTD implementation
- */
-
-/*
- * Erase an address range on the flash chip.  The address range may extend
- * one or more erase sectors.  Return an error is there is a problem erasing.
- */
-static int m25p80_erase(struct mtd_info *mtd, struct erase_info *instr)
+static void m25p_write(struct spi_nor *flash, loff_t to, size_t len,
+			size_t *retlen, const u_char *buf)
 {
-	struct m25p *flash = mtd_to_m25p(mtd);
-	u32 addr,len;
-	uint32_t rem;
-
-	pr_debug("%s: %s at 0x%llx, len %lld\n", dev_name(&flash->spi->dev),
-			__func__, (long long)instr->addr,
-			(long long)instr->len);
-
-	div_u64_rem(instr->len, mtd->erasesize, &rem);
-	if (rem)
-		return -EINVAL;
-
-	addr = instr->addr;
-	len = instr->len;
-
-	mutex_lock(&flash->lock);
-
-	/* whole-chip erase? */
-	if (len == flash->mtd.size) {
-		if (erase_chip(flash)) {
-			instr->state = MTD_ERASE_FAILED;
-			mutex_unlock(&flash->lock);
-			return -EIO;
-		}
+	struct spi_device *spi = m25p_get_spi(flash);
+	struct spi_transfer t[2] = {};
+	struct spi_message m;
+	int cmd_sz = m25p_cmdsz(flash);
 
-	/* REVISIT in some cases we could speed up erasing large regions
-	 * by using OPCODE_SE instead of OPCODE_BE_4K.  We may have set up
-	 * to use "small sector erase", but that's not always optimal.
-	 */
+	spi_message_init(&m);
 
-	/* "sector"-at-a-time erase */
-	} else {
-		while (len) {
-			if (erase_sector(flash, addr)) {
-				instr->state = MTD_ERASE_FAILED;
-				mutex_unlock(&flash->lock);
-				return -EIO;
-			}
+	if (flash->program_opcode == OPCODE_AAI_WP && flash->sst_write_second)
+		cmd_sz = 1;
 
-			addr += mtd->erasesize;
-			len -= mtd->erasesize;
-		}
-	}
+	flash->command[0] = flash->program_opcode;
+	m25p_addr2cmd(flash, to, flash->command);
 
-	mutex_unlock(&flash->lock);
+	t[0].tx_buf = flash->command;
+	t[0].len = cmd_sz;
+	spi_message_add_tail(&t[0], &m);
 
-	instr->state = MTD_ERASE_DONE;
-	mtd_erase_callback(instr);
+	t[1].tx_buf = buf;
+	t[1].len = len;
+	spi_message_add_tail(&t[1], &m);
 
-	return 0;
-}
+	spi_sync(spi, &m);
 
-/*
- * Dummy Cycle calculation for different type of read.
- * It can be used to support more commands with
- * different dummy cycle requirements.
- */
-static inline int m25p80_dummy_cycles_read(struct m25p *flash)
-{
-	switch (flash->flash_read) {
-	case M25P80_FAST:
-	case M25P80_QUAD:
-		return 1;
-	case M25P80_NORMAL:
-		return 0;
-	default:
-		dev_err(&flash->spi->dev, "No valid read type supported\n");
-		return -1;
-	}
+	*retlen += m.actual_length - cmd_sz;
 }
 
 /*
  * Read an address range from the flash chip.  The address range
  * may be any size provided it is within the physical boundaries.
  */
-static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
-	size_t *retlen, u_char *buf)
+static int m25p_read(struct spi_nor *flash, loff_t from, size_t len,
+			size_t *retlen, u_char *buf)
 {
-	struct m25p *flash = mtd_to_m25p(mtd);
+	struct spi_device *spi = m25p_get_spi(flash);
 	struct spi_transfer t[2];
 	struct spi_message m;
-	uint8_t opcode;
 	int dummy;
 
-	pr_debug("%s: %s from 0x%08x, len %zd\n", dev_name(&flash->spi->dev),
-			__func__, (u32)from, len);
-
 	spi_message_init(&m);
 	memset(t, 0, (sizeof t));
 
+	flash->command[0] = flash->read_opcode;
+	m25p_addr2cmd(flash, from, flash->command);
+
 	dummy =  m25p80_dummy_cycles_read(flash);
 	if (dummy < 0) {
-		dev_err(&flash->spi->dev, "No valid read command supported\n");
+		dev_err(flash->dev, "No valid read command supported\n");
 		return -EINVAL;
 	}
 
@@ -468,797 +158,52 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from, size_t len,
 	t[1].len = len;
 	spi_message_add_tail(&t[1], &m);
 
-	mutex_lock(&flash->lock);
-
-	/* Wait till previous write/erase is done. */
-	if (wait_till_ready(flash)) {
-		/* REVISIT status return?? */
-		mutex_unlock(&flash->lock);
-		return 1;
-	}
-
-	/* Set up the write data buffer. */
-	opcode = flash->read_opcode;
-	flash->command[0] = opcode;
-	m25p_addr2cmd(flash, from, flash->command);
-
-	spi_sync(flash->spi, &m);
+	spi_sync(spi, &m);
 
 	*retlen = m.actual_length - m25p_cmdsz(flash) - dummy;
-
-	mutex_unlock(&flash->lock);
-
 	return 0;
 }
 
 /*
- * Write an address range to the flash chip.  Data must be written in
- * FLASH_PAGESIZE chunks.  The address range may be any size provided
- * it is within the physical boundaries.
- */
-static int m25p80_write(struct mtd_info *mtd, loff_t to, size_t len,
-	size_t *retlen, const u_char *buf)
-{
-	struct m25p *flash = mtd_to_m25p(mtd);
-	u32 page_offset, page_size;
-	struct spi_transfer t[2];
-	struct spi_message m;
-
-	pr_debug("%s: %s to 0x%08x, len %zd\n", dev_name(&flash->spi->dev),
-			__func__, (u32)to, len);
-
-	spi_message_init(&m);
-	memset(t, 0, (sizeof t));
-
-	t[0].tx_buf = flash->command;
-	t[0].len = m25p_cmdsz(flash);
-	spi_message_add_tail(&t[0], &m);
-
-	t[1].tx_buf = buf;
-	spi_message_add_tail(&t[1], &m);
-
-	mutex_lock(&flash->lock);
-
-	/* Wait until finished previous write command. */
-	if (wait_till_ready(flash)) {
-		mutex_unlock(&flash->lock);
-		return 1;
-	}
-
-	write_enable(flash);
-
-	/* Set up the opcode in the write buffer. */
-	flash->command[0] = flash->program_opcode;
-	m25p_addr2cmd(flash, to, flash->command);
-
-	page_offset = to & (flash->page_size - 1);
-
-	/* do all the bytes fit onto one page? */
-	if (page_offset + len <= flash->page_size) {
-		t[1].len = len;
-
-		spi_sync(flash->spi, &m);
-
-		*retlen = m.actual_length - m25p_cmdsz(flash);
-	} else {
-		u32 i;
-
-		/* the size of data remaining on the first page */
-		page_size = flash->page_size - page_offset;
-
-		t[1].len = page_size;
-		spi_sync(flash->spi, &m);
-
-		*retlen = m.actual_length - m25p_cmdsz(flash);
-
-		/* write everything in flash->page_size chunks */
-		for (i = page_size; i < len; i += page_size) {
-			page_size = len - i;
-			if (page_size > flash->page_size)
-				page_size = flash->page_size;
-
-			/* write the next page to flash */
-			m25p_addr2cmd(flash, to + i, flash->command);
-
-			t[1].tx_buf = buf + i;
-			t[1].len = page_size;
-
-			wait_till_ready(flash);
-
-			write_enable(flash);
-
-			spi_sync(flash->spi, &m);
-
-			*retlen += m.actual_length - m25p_cmdsz(flash);
-		}
-	}
-
-	mutex_unlock(&flash->lock);
-
-	return 0;
-}
-
-static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
-		size_t *retlen, const u_char *buf)
-{
-	struct m25p *flash = mtd_to_m25p(mtd);
-	struct spi_transfer t[2];
-	struct spi_message m;
-	size_t actual;
-	int cmd_sz, ret;
-
-	pr_debug("%s: %s to 0x%08x, len %zd\n", dev_name(&flash->spi->dev),
-			__func__, (u32)to, len);
-
-	spi_message_init(&m);
-	memset(t, 0, (sizeof t));
-
-	t[0].tx_buf = flash->command;
-	t[0].len = m25p_cmdsz(flash);
-	spi_message_add_tail(&t[0], &m);
-
-	t[1].tx_buf = buf;
-	spi_message_add_tail(&t[1], &m);
-
-	mutex_lock(&flash->lock);
-
-	/* Wait until finished previous write command. */
-	ret = wait_till_ready(flash);
-	if (ret)
-		goto time_out;
-
-	write_enable(flash);
-
-	actual = to % 2;
-	/* Start write from odd address. */
-	if (actual) {
-		flash->command[0] = OPCODE_BP;
-		m25p_addr2cmd(flash, to, flash->command);
-
-		/* write one byte. */
-		t[1].len = 1;
-		spi_sync(flash->spi, &m);
-		ret = wait_till_ready(flash);
-		if (ret)
-			goto time_out;
-		*retlen += m.actual_length - m25p_cmdsz(flash);
-	}
-	to += actual;
-
-	flash->command[0] = OPCODE_AAI_WP;
-	m25p_addr2cmd(flash, to, flash->command);
-
-	/* Write out most of the data here. */
-	cmd_sz = m25p_cmdsz(flash);
-	for (; actual < len - 1; actual += 2) {
-		t[0].len = cmd_sz;
-		/* write two bytes. */
-		t[1].len = 2;
-		t[1].tx_buf = buf + actual;
-
-		spi_sync(flash->spi, &m);
-		ret = wait_till_ready(flash);
-		if (ret)
-			goto time_out;
-		*retlen += m.actual_length - cmd_sz;
-		cmd_sz = 1;
-		to += 2;
-	}
-	write_disable(flash);
-	ret = wait_till_ready(flash);
-	if (ret)
-		goto time_out;
-
-	/* Write out trailing byte if it exists. */
-	if (actual != len) {
-		write_enable(flash);
-		flash->command[0] = OPCODE_BP;
-		m25p_addr2cmd(flash, to, flash->command);
-		t[0].len = m25p_cmdsz(flash);
-		t[1].len = 1;
-		t[1].tx_buf = buf + actual;
-
-		spi_sync(flash->spi, &m);
-		ret = wait_till_ready(flash);
-		if (ret)
-			goto time_out;
-		*retlen += m.actual_length - m25p_cmdsz(flash);
-		write_disable(flash);
-	}
-
-time_out:
-	mutex_unlock(&flash->lock);
-	return ret;
-}
-
-static int m25p80_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
-{
-	struct m25p *flash = mtd_to_m25p(mtd);
-	uint32_t offset = ofs;
-	uint8_t status_old, status_new;
-	int res = 0;
-
-	mutex_lock(&flash->lock);
-	/* Wait until finished previous command */
-	if (wait_till_ready(flash)) {
-		res = 1;
-		goto err;
-	}
-
-	status_old = read_sr(flash);
-
-	if (offset < flash->mtd.size-(flash->mtd.size/2))
-		status_new = status_old | SR_BP2 | SR_BP1 | SR_BP0;
-	else if (offset < flash->mtd.size-(flash->mtd.size/4))
-		status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
-	else if (offset < flash->mtd.size-(flash->mtd.size/8))
-		status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
-	else if (offset < flash->mtd.size-(flash->mtd.size/16))
-		status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2;
-	else if (offset < flash->mtd.size-(flash->mtd.size/32))
-		status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
-	else if (offset < flash->mtd.size-(flash->mtd.size/64))
-		status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1;
-	else
-		status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0;
-
-	/* Only modify protection if it will not unlock other areas */
-	if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) >
-					(status_old&(SR_BP2|SR_BP1|SR_BP0))) {
-		write_enable(flash);
-		if (write_sr(flash, status_new) < 0) {
-			res = 1;
-			goto err;
-		}
-	}
-
-err:	mutex_unlock(&flash->lock);
-	return res;
-}
-
-static int m25p80_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
-{
-	struct m25p *flash = mtd_to_m25p(mtd);
-	uint32_t offset = ofs;
-	uint8_t status_old, status_new;
-	int res = 0;
-
-	mutex_lock(&flash->lock);
-	/* Wait until finished previous command */
-	if (wait_till_ready(flash)) {
-		res = 1;
-		goto err;
-	}
-
-	status_old = read_sr(flash);
-
-	if (offset+len > flash->mtd.size-(flash->mtd.size/64))
-		status_new = status_old & ~(SR_BP2|SR_BP1|SR_BP0);
-	else if (offset+len > flash->mtd.size-(flash->mtd.size/32))
-		status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0;
-	else if (offset+len > flash->mtd.size-(flash->mtd.size/16))
-		status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1;
-	else if (offset+len > flash->mtd.size-(flash->mtd.size/8))
-		status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
-	else if (offset+len > flash->mtd.size-(flash->mtd.size/4))
-		status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2;
-	else if (offset+len > flash->mtd.size-(flash->mtd.size/2))
-		status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
-	else
-		status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
-
-	/* Only modify protection if it will not lock other areas */
-	if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) <
-					(status_old&(SR_BP2|SR_BP1|SR_BP0))) {
-		write_enable(flash);
-		if (write_sr(flash, status_new) < 0) {
-			res = 1;
-			goto err;
-		}
-	}
-
-err:	mutex_unlock(&flash->lock);
-	return res;
-}
-
-/****************************************************************************/
-
-/*
- * SPI device driver setup and teardown
- */
-
-struct flash_info {
-	/* JEDEC id zero means "no ID" (most older chips); otherwise it has
-	 * a high byte of zero plus three data bytes: the manufacturer id,
-	 * then a two byte device id.
-	 */
-	u32		jedec_id;
-	u16             ext_id;
-
-	/* The size listed here is what works with OPCODE_SE, which isn't
-	 * necessarily called a "sector" by the vendor.
-	 */
-	unsigned	sector_size;
-	u16		n_sectors;
-
-	u16		page_size;
-	u16		addr_width;
-
-	u16		flags;
-#define	SECT_4K		0x01		/* OPCODE_BE_4K works uniformly */
-#define	M25P_NO_ERASE	0x02		/* No erase command needed */
-#define	SST_WRITE	0x04		/* use SST byte programming */
-#define	M25P_NO_FR	0x08		/* Can't do fastread */
-#define	SECT_4K_PMC	0x10		/* OPCODE_BE_4K_PMC works uniformly */
-#define	M25P80_QUAD_READ	0x20    /* Flash supports Quad Read */
-};
-
-#define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\
-	((kernel_ulong_t)&(struct flash_info) {				\
-		.jedec_id = (_jedec_id),				\
-		.ext_id = (_ext_id),					\
-		.sector_size = (_sector_size),				\
-		.n_sectors = (_n_sectors),				\
-		.page_size = 256,					\
-		.flags = (_flags),					\
-	})
-
-#define CAT25_INFO(_sector_size, _n_sectors, _page_size, _addr_width, _flags)	\
-	((kernel_ulong_t)&(struct flash_info) {				\
-		.sector_size = (_sector_size),				\
-		.n_sectors = (_n_sectors),				\
-		.page_size = (_page_size),				\
-		.addr_width = (_addr_width),				\
-		.flags = (_flags),					\
-	})
-
-/* NOTE: double check command sets and memory organization when you add
- * more flash chips.  This current list focusses on newer chips, which
- * have been converging on command sets which including JEDEC ID.
- */
-static const struct spi_device_id m25p_ids[] = {
-	/* Atmel -- some are (confusingly) marketed as "DataFlash" */
-	{ "at25fs010",  INFO(0x1f6601, 0, 32 * 1024,   4, SECT_4K) },
-	{ "at25fs040",  INFO(0x1f6604, 0, 64 * 1024,   8, SECT_4K) },
-
-	{ "at25df041a", INFO(0x1f4401, 0, 64 * 1024,   8, SECT_4K) },
-	{ "at25df321a", INFO(0x1f4701, 0, 64 * 1024,  64, SECT_4K) },
-	{ "at25df641",  INFO(0x1f4800, 0, 64 * 1024, 128, SECT_4K) },
-
-	{ "at26f004",   INFO(0x1f0400, 0, 64 * 1024,  8, SECT_4K) },
-	{ "at26df081a", INFO(0x1f4501, 0, 64 * 1024, 16, SECT_4K) },
-	{ "at26df161a", INFO(0x1f4601, 0, 64 * 1024, 32, SECT_4K) },
-	{ "at26df321",  INFO(0x1f4700, 0, 64 * 1024, 64, SECT_4K) },
-
-	{ "at45db081d", INFO(0x1f2500, 0, 64 * 1024, 16, SECT_4K) },
-
-	/* EON -- en25xxx */
-	{ "en25f32",    INFO(0x1c3116, 0, 64 * 1024,   64, SECT_4K) },
-	{ "en25p32",    INFO(0x1c2016, 0, 64 * 1024,   64, 0) },
-	{ "en25q32b",   INFO(0x1c3016, 0, 64 * 1024,   64, 0) },
-	{ "en25p64",    INFO(0x1c2017, 0, 64 * 1024,  128, 0) },
-	{ "en25q64",    INFO(0x1c3017, 0, 64 * 1024,  128, SECT_4K) },
-	{ "en25qh256",  INFO(0x1c7019, 0, 64 * 1024,  512, 0) },
-
-	/* ESMT */
-	{ "f25l32pa", INFO(0x8c2016, 0, 64 * 1024, 64, SECT_4K) },
-
-	/* Everspin */
-	{ "mr25h256", CAT25_INFO( 32 * 1024, 1, 256, 2, M25P_NO_ERASE | M25P_NO_FR) },
-	{ "mr25h10",  CAT25_INFO(128 * 1024, 1, 256, 3, M25P_NO_ERASE | M25P_NO_FR) },
-
-	/* GigaDevice */
-	{ "gd25q32", INFO(0xc84016, 0, 64 * 1024,  64, SECT_4K) },
-	{ "gd25q64", INFO(0xc84017, 0, 64 * 1024, 128, SECT_4K) },
-
-	/* Intel/Numonyx -- xxxs33b */
-	{ "160s33b",  INFO(0x898911, 0, 64 * 1024,  32, 0) },
-	{ "320s33b",  INFO(0x898912, 0, 64 * 1024,  64, 0) },
-	{ "640s33b",  INFO(0x898913, 0, 64 * 1024, 128, 0) },
-
-	/* Macronix */
-	{ "mx25l2005a",  INFO(0xc22012, 0, 64 * 1024,   4, SECT_4K) },
-	{ "mx25l4005a",  INFO(0xc22013, 0, 64 * 1024,   8, SECT_4K) },
-	{ "mx25l8005",   INFO(0xc22014, 0, 64 * 1024,  16, 0) },
-	{ "mx25l1606e",  INFO(0xc22015, 0, 64 * 1024,  32, SECT_4K) },
-	{ "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, 0) },
-	{ "mx25l3255e",  INFO(0xc29e16, 0, 64 * 1024,  64, SECT_4K) },
-	{ "mx25l6405d",  INFO(0xc22017, 0, 64 * 1024, 128, 0) },
-	{ "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, 0) },
-	{ "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
-	{ "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, 0) },
-	{ "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
-	{ "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, M25P80_QUAD_READ) },
-
-	/* Micron */
-	{ "n25q064",     INFO(0x20ba17, 0, 64 * 1024,  128, 0) },
-	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, 0) },
-	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
-	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
-	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
-
-	/* PMC */
-	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
-	{ "pm25lv010",   INFO(0,        0, 32 * 1024,    4, SECT_4K_PMC) },
-	{ "pm25lq032",   INFO(0x7f9d46, 0, 64 * 1024,   64, SECT_4K) },
-
-	/* Spansion -- single (large) sector size only, at least
-	 * for the chips listed here (without boot sectors).
-	 */
-	{ "s25sl032p",  INFO(0x010215, 0x4d00,  64 * 1024,  64, 0) },
-	{ "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128, 0) },
-	{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, 0) },
-	{ "s25fl256s1", INFO(0x010219, 0x4d01,  64 * 1024, 512, M25P80_QUAD_READ) },
-	{ "s25fl512s",  INFO(0x010220, 0x4d00, 256 * 1024, 256, 0) },
-	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
-	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
-	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
-	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, 0) },
-	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
-	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
-	{ "s25sl008a",  INFO(0x010213,      0,  64 * 1024,  16, 0) },
-	{ "s25sl016a",  INFO(0x010214,      0,  64 * 1024,  32, 0) },
-	{ "s25sl032a",  INFO(0x010215,      0,  64 * 1024,  64, 0) },
-	{ "s25sl064a",  INFO(0x010216,      0,  64 * 1024, 128, 0) },
-	{ "s25fl016k",  INFO(0xef4015,      0,  64 * 1024,  32, SECT_4K) },
-	{ "s25fl064k",  INFO(0xef4017,      0,  64 * 1024, 128, SECT_4K) },
-
-	/* SST -- large erase sizes are "overlays", "sectors" are 4K */
-	{ "sst25vf040b", INFO(0xbf258d, 0, 64 * 1024,  8, SECT_4K | SST_WRITE) },
-	{ "sst25vf080b", INFO(0xbf258e, 0, 64 * 1024, 16, SECT_4K | SST_WRITE) },
-	{ "sst25vf016b", INFO(0xbf2541, 0, 64 * 1024, 32, SECT_4K | SST_WRITE) },
-	{ "sst25vf032b", INFO(0xbf254a, 0, 64 * 1024, 64, SECT_4K | SST_WRITE) },
-	{ "sst25vf064c", INFO(0xbf254b, 0, 64 * 1024, 128, SECT_4K) },
-	{ "sst25wf512",  INFO(0xbf2501, 0, 64 * 1024,  1, SECT_4K | SST_WRITE) },
-	{ "sst25wf010",  INFO(0xbf2502, 0, 64 * 1024,  2, SECT_4K | SST_WRITE) },
-	{ "sst25wf020",  INFO(0xbf2503, 0, 64 * 1024,  4, SECT_4K | SST_WRITE) },
-	{ "sst25wf040",  INFO(0xbf2504, 0, 64 * 1024,  8, SECT_4K | SST_WRITE) },
-
-	/* ST Microelectronics -- newer production may have feature updates */
-	{ "m25p05",  INFO(0x202010,  0,  32 * 1024,   2, 0) },
-	{ "m25p10",  INFO(0x202011,  0,  32 * 1024,   4, 0) },
-	{ "m25p20",  INFO(0x202012,  0,  64 * 1024,   4, 0) },
-	{ "m25p40",  INFO(0x202013,  0,  64 * 1024,   8, 0) },
-	{ "m25p80",  INFO(0x202014,  0,  64 * 1024,  16, 0) },
-	{ "m25p16",  INFO(0x202015,  0,  64 * 1024,  32, 0) },
-	{ "m25p32",  INFO(0x202016,  0,  64 * 1024,  64, 0) },
-	{ "m25p64",  INFO(0x202017,  0,  64 * 1024, 128, 0) },
-	{ "m25p128", INFO(0x202018,  0, 256 * 1024,  64, 0) },
-	{ "n25q032", INFO(0x20ba16,  0,  64 * 1024,  64, 0) },
-
-	{ "m25p05-nonjedec",  INFO(0, 0,  32 * 1024,   2, 0) },
-	{ "m25p10-nonjedec",  INFO(0, 0,  32 * 1024,   4, 0) },
-	{ "m25p20-nonjedec",  INFO(0, 0,  64 * 1024,   4, 0) },
-	{ "m25p40-nonjedec",  INFO(0, 0,  64 * 1024,   8, 0) },
-	{ "m25p80-nonjedec",  INFO(0, 0,  64 * 1024,  16, 0) },
-	{ "m25p16-nonjedec",  INFO(0, 0,  64 * 1024,  32, 0) },
-	{ "m25p32-nonjedec",  INFO(0, 0,  64 * 1024,  64, 0) },
-	{ "m25p64-nonjedec",  INFO(0, 0,  64 * 1024, 128, 0) },
-	{ "m25p128-nonjedec", INFO(0, 0, 256 * 1024,  64, 0) },
-
-	{ "m45pe10", INFO(0x204011,  0, 64 * 1024,    2, 0) },
-	{ "m45pe80", INFO(0x204014,  0, 64 * 1024,   16, 0) },
-	{ "m45pe16", INFO(0x204015,  0, 64 * 1024,   32, 0) },
-
-	{ "m25pe20", INFO(0x208012,  0, 64 * 1024,  4,       0) },
-	{ "m25pe80", INFO(0x208014,  0, 64 * 1024, 16,       0) },
-	{ "m25pe16", INFO(0x208015,  0, 64 * 1024, 32, SECT_4K) },
-
-	{ "m25px32",    INFO(0x207116,  0, 64 * 1024, 64, SECT_4K) },
-	{ "m25px32-s0", INFO(0x207316,  0, 64 * 1024, 64, SECT_4K) },
-	{ "m25px32-s1", INFO(0x206316,  0, 64 * 1024, 64, SECT_4K) },
-	{ "m25px64",    INFO(0x207117,  0, 64 * 1024, 128, 0) },
-
-	/* Winbond -- w25x "blocks" are 64K, "sectors" are 4KiB */
-	{ "w25x10", INFO(0xef3011, 0, 64 * 1024,  2,  SECT_4K) },
-	{ "w25x20", INFO(0xef3012, 0, 64 * 1024,  4,  SECT_4K) },
-	{ "w25x40", INFO(0xef3013, 0, 64 * 1024,  8,  SECT_4K) },
-	{ "w25x80", INFO(0xef3014, 0, 64 * 1024,  16, SECT_4K) },
-	{ "w25x16", INFO(0xef3015, 0, 64 * 1024,  32, SECT_4K) },
-	{ "w25x32", INFO(0xef3016, 0, 64 * 1024,  64, SECT_4K) },
-	{ "w25q32", INFO(0xef4016, 0, 64 * 1024,  64, SECT_4K) },
-	{ "w25q32dw", INFO(0xef6016, 0, 64 * 1024,  64, SECT_4K) },
-	{ "w25x64", INFO(0xef3017, 0, 64 * 1024, 128, SECT_4K) },
-	{ "w25q64", INFO(0xef4017, 0, 64 * 1024, 128, SECT_4K) },
-	{ "w25q128", INFO(0xef4018, 0, 64 * 1024, 256, SECT_4K) },
-	{ "w25q80", INFO(0xef5014, 0, 64 * 1024,  16, SECT_4K) },
-	{ "w25q80bl", INFO(0xef4014, 0, 64 * 1024,  16, SECT_4K) },
-	{ "w25q128", INFO(0xef4018, 0, 64 * 1024, 256, SECT_4K) },
-	{ "w25q256", INFO(0xef4019, 0, 64 * 1024, 512, SECT_4K) },
-
-	/* Catalyst / On Semiconductor -- non-JEDEC */
-	{ "cat25c11", CAT25_INFO(  16, 8, 16, 1, M25P_NO_ERASE | M25P_NO_FR) },
-	{ "cat25c03", CAT25_INFO(  32, 8, 16, 2, M25P_NO_ERASE | M25P_NO_FR) },
-	{ "cat25c09", CAT25_INFO( 128, 8, 32, 2, M25P_NO_ERASE | M25P_NO_FR) },
-	{ "cat25c17", CAT25_INFO( 256, 8, 32, 2, M25P_NO_ERASE | M25P_NO_FR) },
-	{ "cat25128", CAT25_INFO(2048, 8, 64, 2, M25P_NO_ERASE | M25P_NO_FR) },
-	{ },
-};
-MODULE_DEVICE_TABLE(spi, m25p_ids);
-
-static const struct spi_device_id *jedec_probe(struct spi_device *spi)
-{
-	int			tmp;
-	u8			code = OPCODE_RDID;
-	u8			id[5];
-	u32			jedec;
-	u16                     ext_jedec;
-	struct flash_info	*info;
-
-	/* JEDEC also defines an optional "extended device information"
-	 * string for after vendor-specific data, after the three bytes
-	 * we use here.  Supporting some chips might require using it.
-	 */
-	tmp = spi_write_then_read(spi, &code, 1, id, 5);
-	if (tmp < 0) {
-		pr_debug("%s: error %d reading JEDEC ID\n",
-				dev_name(&spi->dev), tmp);
-		return ERR_PTR(tmp);
-	}
-	jedec = id[0];
-	jedec = jedec << 8;
-	jedec |= id[1];
-	jedec = jedec << 8;
-	jedec |= id[2];
-
-	ext_jedec = id[3] << 8 | id[4];
-
-	for (tmp = 0; tmp < ARRAY_SIZE(m25p_ids) - 1; tmp++) {
-		info = (void *)m25p_ids[tmp].driver_data;
-		if (info->jedec_id == jedec) {
-			if (info->ext_id != 0 && info->ext_id != ext_jedec)
-				continue;
-			return &m25p_ids[tmp];
-		}
-	}
-	dev_err(&spi->dev, "unrecognized JEDEC id %06x\n", jedec);
-	return ERR_PTR(-ENODEV);
-}
-
-
-/*
  * board specific setup should have ensured the SPI clock used here
  * matches what the READ command supports, at least until this driver
  * understands FAST_READ (for clocks over 25 MHz).
  */
 static int m25p_probe(struct spi_device *spi)
 {
-	const struct spi_device_id	*id = spi_get_device_id(spi);
-	struct flash_platform_data	*data;
-	struct m25p			*flash;
-	struct flash_info		*info;
-	unsigned			i;
-	struct mtd_part_parser_data	ppdata;
-	struct device_node *np = spi->dev.of_node;
-	int ret;
-
-	/* Platform data helps sort out which chip type we have, as
-	 * well as how this board partitions it.  If we don't have
-	 * a chip ID, try the JEDEC id commands; they'll work for most
-	 * newer chips, even if we don't recognize the particular chip.
-	 */
-	data = dev_get_platdata(&spi->dev);
-	if (data && data->type) {
-		const struct spi_device_id *plat_id;
-
-		for (i = 0; i < ARRAY_SIZE(m25p_ids) - 1; i++) {
-			plat_id = &m25p_ids[i];
-			if (strcmp(data->type, plat_id->name))
-				continue;
-			break;
-		}
-
-		if (i < ARRAY_SIZE(m25p_ids) - 1)
-			id = plat_id;
-		else
-			dev_warn(&spi->dev, "unrecognized id %s\n", data->type);
-	}
-
-	info = (void *)id->driver_data;
-
-	if (info->jedec_id) {
-		const struct spi_device_id *jid;
-
-		jid = jedec_probe(spi);
-		if (IS_ERR(jid)) {
-			return PTR_ERR(jid);
-		} else if (jid != id) {
-			/*
-			 * JEDEC knows better, so overwrite platform ID. We
-			 * can't trust partitions any longer, but we'll let
-			 * mtd apply them anyway, since some partitions may be
-			 * marked read-only, and we don't want to lose that
-			 * information, even if it's not 100% accurate.
-			 */
-			dev_warn(&spi->dev, "found %s, expected %s\n",
-				 jid->name, id->name);
-			id = jid;
-			info = (void *)jid->driver_data;
-		}
-	}
+	struct spi_nor *flash;
 
 	flash = devm_kzalloc(&spi->dev, sizeof(*flash), GFP_KERNEL);
 	if (!flash)
 		return -ENOMEM;
 
-	flash->command = devm_kzalloc(&spi->dev, MAX_CMD_SIZE, GFP_KERNEL);
-	if (!flash->command)
-		return -ENOMEM;
+	/* install the hooks */
+	flash->read = m25p_read;
+	flash->write = m25p_write;
+	flash->write_reg = m25p_write_reg;
+	flash->read_reg = m25p_read_reg;
 
-	flash->spi = spi;
-	mutex_init(&flash->lock);
+	flash->dev = &spi->dev;
 	spi_set_drvdata(spi, flash);
 
-	/*
-	 * Atmel, SST and Intel/Numonyx serial flash tend to power
-	 * up with the software protection bits set
-	 */
-
-	if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ATMEL ||
-	    JEDEC_MFR(info->jedec_id) == CFI_MFR_INTEL ||
-	    JEDEC_MFR(info->jedec_id) == CFI_MFR_SST) {
-		write_enable(flash);
-		write_sr(flash, 0);
-	}
-
-	if (data && data->name)
-		flash->mtd.name = data->name;
-	else
-		flash->mtd.name = dev_name(&spi->dev);
-
-	flash->mtd.type = MTD_NORFLASH;
-	flash->mtd.writesize = 1;
-	flash->mtd.flags = MTD_CAP_NORFLASH;
-	flash->mtd.size = info->sector_size * info->n_sectors;
-	flash->mtd._erase = m25p80_erase;
-	flash->mtd._read = m25p80_read;
-
-	/* flash protection support for STmicro chips */
-	if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) {
-		flash->mtd._lock = m25p80_lock;
-		flash->mtd._unlock = m25p80_unlock;
-	}
-
-	/* sst flash chips use AAI word program */
-	if (info->flags & SST_WRITE)
-		flash->mtd._write = sst_write;
-	else
-		flash->mtd._write = m25p80_write;
-
-	/* prefer "small sector" erase if possible */
-	if (info->flags & SECT_4K) {
-		flash->erase_opcode = OPCODE_BE_4K;
-		flash->mtd.erasesize = 4096;
-	} else if (info->flags & SECT_4K_PMC) {
-		flash->erase_opcode = OPCODE_BE_4K_PMC;
-		flash->mtd.erasesize = 4096;
-	} else {
-		flash->erase_opcode = OPCODE_SE;
-		flash->mtd.erasesize = info->sector_size;
-	}
-
-	if (info->flags & M25P_NO_ERASE)
-		flash->mtd.flags |= MTD_NO_ERASE;
-
-	ppdata.of_node = spi->dev.of_node;
-	flash->mtd.dev.parent = &spi->dev;
-	flash->page_size = info->page_size;
-	flash->mtd.writebufsize = flash->page_size;
-
-	if (np) {
-		/* If we were instantiated by DT, use it */
-		if (of_property_read_bool(np, "m25p,fast-read"))
-			flash->flash_read = M25P80_FAST;
-	} else {
-		/* If we weren't instantiated by DT, default to fast-read */
-		flash->flash_read = M25P80_FAST;
-	}
-
-	/* Some devices cannot do fast-read, no matter what DT tells us */
-	if (info->flags & M25P_NO_FR)
-		flash->flash_read = M25P80_NORMAL;
-
-	/* Quad-read mode takes precedence over fast/normal */
-	if (spi->mode & SPI_RX_QUAD && info->flags & M25P80_QUAD_READ) {
-		ret = set_quad_mode(flash, info->jedec_id);
-		if (ret) {
-			dev_err(&flash->spi->dev, "quad mode not supported\n");
-			return ret;
-		}
-		flash->flash_read = M25P80_QUAD;
-	}
-
-	/* Default commands */
-	switch (flash->flash_read) {
-	case M25P80_QUAD:
-		flash->read_opcode = OPCODE_QUAD_READ;
-		break;
-	case M25P80_FAST:
-		flash->read_opcode = OPCODE_FAST_READ;
-		break;
-	case M25P80_NORMAL:
-		flash->read_opcode = OPCODE_NORM_READ;
-		break;
-	default:
-		dev_err(&flash->spi->dev, "No Read opcode defined\n");
-		return -EINVAL;
-	}
-
-	flash->program_opcode = OPCODE_PP;
-
-	if (info->addr_width)
-		flash->addr_width = info->addr_width;
-	else if (flash->mtd.size > 0x1000000) {
-		/* enable 4-byte addressing if the device exceeds 16MiB */
-		flash->addr_width = 4;
-		if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) {
-			/* Dedicated 4-byte command set */
-			switch (flash->flash_read) {
-			case M25P80_QUAD:
-				flash->read_opcode = OPCODE_QUAD_READ;
-				break;
-			case M25P80_FAST:
-				flash->read_opcode = OPCODE_FAST_READ_4B;
-				break;
-			case M25P80_NORMAL:
-				flash->read_opcode = OPCODE_NORM_READ_4B;
-				break;
-			}
-			flash->program_opcode = OPCODE_PP_4B;
-			/* No small sector erase for 4-byte command set */
-			flash->erase_opcode = OPCODE_SE_4B;
-			flash->mtd.erasesize = info->sector_size;
-		} else
-			set_4byte(flash, info->jedec_id, 1);
-	} else {
-		flash->addr_width = 3;
-	}
-
-	dev_info(&spi->dev, "%s (%lld Kbytes)\n", id->name,
-			(long long)flash->mtd.size >> 10);
-
-	pr_debug("mtd .name = %s, .size = 0x%llx (%lldMiB) "
-			".erasesize = 0x%.8x (%uKiB) .numeraseregions = %d\n",
-		flash->mtd.name,
-		(long long)flash->mtd.size, (long long)(flash->mtd.size >> 20),
-		flash->mtd.erasesize, flash->mtd.erasesize / 1024,
-		flash->mtd.numeraseregions);
-
-	if (flash->mtd.numeraseregions)
-		for (i = 0; i < flash->mtd.numeraseregions; i++)
-			pr_debug("mtd.eraseregions[%d] = { .offset = 0x%llx, "
-				".erasesize = 0x%.8x (%uKiB), "
-				".numblocks = %d }\n",
-				i, (long long)flash->mtd.eraseregions[i].offset,
-				flash->mtd.eraseregions[i].erasesize,
-				flash->mtd.eraseregions[i].erasesize / 1024,
-				flash->mtd.eraseregions[i].numblocks);
-
-
-	/* partitions should match sector boundaries; and it may be good to
-	 * use readonly partitions for writeprotected sectors (BP2..BP0).
-	 */
-	return mtd_device_parse_register(&flash->mtd, NULL, &ppdata,
-			data ? data->parts : NULL,
-			data ? data->nr_parts : 0);
+	/* do the scan */
+	return spi_nor_register(flash, spi_get_device_id(spi),
+			spi->mode & SPI_RX_QUAD);
 }
 
-
 static int m25p_remove(struct spi_device *spi)
 {
-	struct m25p	*flash = spi_get_drvdata(spi);
+	struct spi_nor *flash = spi_get_drvdata(spi);
 
-	/* Clean up MTD stuff. */
-	return mtd_device_unregister(&flash->mtd);
+	return spi_nor_unregister(flash);
 }
 
-
 static struct spi_driver m25p80_driver = {
 	.driver = {
 		.name	= "m25p80",
 		.owner	= THIS_MODULE,
 	},
-	.id_table	= m25p_ids,
+	.id_table	= spi_nor_ids,
 	.probe	= m25p_probe,
 	.remove	= m25p_remove,
 
-- 
1.7.2.rc3

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Angus Clark]

Hi Huang Shijie,

I have not gone through your proposal in detail yet, but I just wanted to say I
fully support the motivation behind an update to the way Serial Flash is handled
in the kernel.  Looking at the various drivers supporting Serial Flash, it is
obvious that the current framework is not adequate.  Some have chosen to
implement standalone drivers, in driver/mtd/devices, duplicating much of
m25p80.c, while others have attempted to squeeze dedicated Serial Flash
Controllers within the generic SPI framework, often requiring fragile heuristics
to re-create the semantics of m25p80 which are lost over the SPI framework.

I will try to go through the patches in the next couple of days -- I have many
pending tasks at present!

Cheers,

Angus


On 11/26/2013 06:32 AM, Huang Shijie wrote:
> 1.) Why add a new framework for SPI NOR?
>   The SPI-NOR controller such as Freescale's Quadspi controller is working
>   in a different way from the SPI bus. It should knows the NOR commands to
>   find the right LUT sequence. Unfortunately, the current code can not meet
>   this requirement.
> 
> 2.) How does this patch set do?
>    This patch set adds a new spi-nor layer.
>    Before this patch, the layer is like:
> 
>                    MTD
>          ------------------------
>                   m25p80
>          ------------------------
> 	       spi bus driver
>          ------------------------
> 	        SPI NOR chip
> 
>    After this patch, the layer is like:
>                    MTD
>          ------------------------
>                   spi-nor
>          ------------------------
>                   m25p80
>          ------------------------
> 	       spi bus driver
>          ------------------------
> 	       SPI NOR chip
> 
>   With the spi-nor controller driver(Freescale Quadspi), it looks like:
>                    MTD
>          ------------------------
>                   spi-nor
>          ------------------------
>                 fsl-quadspi
>          ------------------------
> 	       SPI NOR chip
> 
> 3.) more details
>    This patch set adds a new data structrue spi_nor{}, clones most the common
>   code to spi-nor.c. Make the m25p80.c use the new APIs.
> 
> 4.) TODO list:
>    3.1) add the new spi_nor_device{} for the spi-nor controller's device.
>    3.2) add the Freescale Quadspi driver.
> 
> 	       
> Huang Shijie (4):
>   mtd: spi-nor: move the SPI NOR commands to a new header file
>   mtd: spi-nor: add a new data structrue spi_nor{}
>   mtd: spi-nor: add the framework for SPI NOR
>   mtd: m25p80: use the new spi-nor APIs
> 
>  drivers/mtd/Kconfig           |    2 +
>  drivers/mtd/Makefile          |    1 +
>  drivers/mtd/devices/Kconfig   |    2 +-
>  drivers/mtd/devices/m25p80.c  | 1249 +++--------------------------------------
>  drivers/mtd/spi-nor/Kconfig   |    6 +
>  drivers/mtd/spi-nor/Makefile  |    1 +
>  drivers/mtd/spi-nor/spi-nor.c | 1057 ++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h   |  101 ++++
>  8 files changed, 1239 insertions(+), 1180 deletions(-)
>  create mode 100644 drivers/mtd/spi-nor/Kconfig
>  create mode 100644 drivers/mtd/spi-nor/Makefile
>  create mode 100644 drivers/mtd/spi-nor/spi-nor.c
>  create mode 100644 include/linux/mtd/spi-nor.h
> 

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Brian Norris]

+ Lee Jones, others

I'd like to keep a similar CC list for the various conversations going
on about SPI NOR frameworks.

On Tue, Nov 26, 2013 at 02:32:51PM +0800, Huang Shijie wrote:
> 1.) Why add a new framework for SPI NOR?
>   The SPI-NOR controller such as Freescale's Quadspi controller is working
>   in a different way from the SPI bus. It should knows the NOR commands to
>   find the right LUT sequence. Unfortunately, the current code can not meet
>   this requirement.
> 
> 2.) How does this patch set do?
>    This patch set adds a new spi-nor layer.
>    Before this patch, the layer is like:
> 
>                    MTD
>          ------------------------
>                   m25p80
>          ------------------------
> 	       spi bus driver
>          ------------------------
> 	        SPI NOR chip
> 
>    After this patch, the layer is like:
>                    MTD
>          ------------------------
>                   spi-nor
>          ------------------------
>                   m25p80
>          ------------------------
> 	       spi bus driver
>          ------------------------
> 	       SPI NOR chip
> 
>   With the spi-nor controller driver(Freescale Quadspi), it looks like:
>                    MTD
>          ------------------------
>                   spi-nor
>          ------------------------
>                 fsl-quadspi
>          ------------------------
> 	       SPI NOR chip
> 
> 3.) more details
>    This patch set adds a new data structrue spi_nor{}, clones most the common
>   code to spi-nor.c. Make the m25p80.c use the new APIs.
> 
> 4.) TODO list:
>    3.1) add the new spi_nor_device{} for the spi-nor controller's device.
>    3.2) add the Freescale Quadspi driver.
> 
> 	       
> Huang Shijie (4):
>   mtd: spi-nor: move the SPI NOR commands to a new header file
>   mtd: spi-nor: add a new data structrue spi_nor{}
>   mtd: spi-nor: add the framework for SPI NOR
>   mtd: m25p80: use the new spi-nor APIs
> 
>  drivers/mtd/Kconfig           |    2 +
>  drivers/mtd/Makefile          |    1 +
>  drivers/mtd/devices/Kconfig   |    2 +-
>  drivers/mtd/devices/m25p80.c  | 1249 +++--------------------------------------
>  drivers/mtd/spi-nor/Kconfig   |    6 +
>  drivers/mtd/spi-nor/Makefile  |    1 +
>  drivers/mtd/spi-nor/spi-nor.c | 1057 ++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h   |  101 ++++
>  8 files changed, 1239 insertions(+), 1180 deletions(-)
>  create mode 100644 drivers/mtd/spi-nor/Kconfig
>  create mode 100644 drivers/mtd/spi-nor/Makefile
>  create mode 100644 drivers/mtd/spi-nor/spi-nor.c
>  create mode 100644 include/linux/mtd/spi-nor.h
> 

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

于 2013年11月27日 12:32, Brian Norris 写道:
> 'd like to keep a similar CC list for the various conversations going
> on about SPI NOR frameworks.
it seems i missed to CC linux-SPI maillist.

thanks
Huang Shijie

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Angus Clark]

Hi Huang Shijie,

On 11/27/2013 04:32 AM, Brian Norris wrote:
> + Lee Jones, others
> 
> I'd like to keep a similar CC list for the various conversations going
> on about SPI NOR frameworks.
> 
> On Tue, Nov 26, 2013 at 02:32:51PM +0800, Huang Shijie wrote:
>> 1.) Why add a new framework for SPI NOR?
>>   The SPI-NOR controller such as Freescale's Quadspi controller is working
>>   in a different way from the SPI bus. It should knows the NOR commands to
>>   find the right LUT sequence. Unfortunately, the current code can not meet
>>   this requirement.

I fully support your argument for introducing a new framework to support Serial
Flash controllers.  Reiterating my previous comment:

"The kernel offers many examples of others who have struggled with the same
problem.  Some have chosen to write self-contained drivers (e.g.
drivers/mtd/devices/spear_smi.c and drivers/mtd/devices/sst251.c) duplicating
much of m25p80.c, while others have attempted to squeeze into the SPI framework
(e.g. drivers/spi/spi-tegra20-sflash.c and drivers/spi/spi-falcon.c), relying on
fragile heuristics to recreate the semantics of m25p80 that is lost over the ,
current circumstances SPI framework ."

However, having now been through your proposed framework, it would not seem to
provide a generally applicable solution.  Indeed, other than making the Serial
Flash commands and the device table available, it is not obvious how it improves
the situation for your own specific controller either; the
read/write/read_reg/write_reg callbacks do not offer a great deal more than
spi_read_then_write().  (Perhaps all will become clear when you submit the
fsl-quadspi driver.)

As I see it, the main problem with the current support is that dedicated Serial
Flash Controllers require a level of semantics that cannot be conveyed over the
generic SPI framework.  With this in mind, I would start by defining something
along the lines of a "Serial Flash transfer".  Some initial ramblings of my mind:

/**
 * struct spi_nor_xfer_cfg - Structure for defining a Serial Flash transfer
 * @wren:		command for "Write Enable", or 0x00 for not required
 * @cmd:		command for operation
 * @cmd_pins:		number of pins to send @cmd (1, 2, 4)
 * @addr:		address for operation
 * @addr_pins:		number of pins to send @addr (1, 2, 4)
 * @addr_width: 	number of address bytes (3,4, or 0 for address not required)
 * @mode:		mode data
 * @mode_pins:		number of pins to send @mode (1, 2, 4)
 * @mode_cycles:	number of mode cycles (0 for mode not required)
 * @dummy_cycles:	number of dummy cycles (0 for dummy not required)
 */
struct spi_nor_xfer_cfg {
	uint8_t		wren;
	uint8_t		cmd;
	int		cmd_pins;
	uint32_t	addr;
	int		addr_pins;
	int		addr_width;
	uint32_t	mode;
	int		mode_pins;
	int		mode_cycles;
	int		dummy_cycles;
};

We could then build up layers of functionality, based on two fundamental primitives:

	int (*read_xfer)(struct spi_nor_info *info,
			 struct spi_nor_xfer_cfg *cfg,
			 uint8_t *buf, size_t len);
	
	int (*write_xfer)(struct spi_nor_info *info,
			  struct spi_nor_xfer_cfg *cfg,
			  uint8_t *buf, size_t len);
	
Default implementations for standard operations could follow:

	int (*read_reg)(struct spi_nor_info *info,
			uint8_t cmd, uint8_t *reg, int len);
	int (*write_reg)(struct spi_nor_info *info,
			 uint8_t cmd, uint8_t *reg, int len,
			 int wren, int wtr);

	int (*readid)(struct spi_nor_info *info, uint8_t *readid);
	int (*wait_till_ready)(struct spi_nor_info *info);
	
	int (*read)(struct spi_nor_info *info, uint8_t buf, off_t offs, size_t len);
	int (*write)(struct spi_nor_info *info, off_t offs, size_t len);
	int (*erase_sector)(struct spi_nor_info *info, off_t offs);

Each driver would be required to implement read_xfer() and write_xfer(), and
then free to either use the default implementations, or override with
driver-optimised implementations.

In the case of a pure SPI Controller, read_xfer() and write_xfer() would simply
flatten to spi_messages.  The key benefit is that dedicated Serial Flash
Controllers would be able to take advantage of the richer semantics offered by
the 'spi_nor_xfer_cfg' data.

I would also expect the spi-nor framework to provide the following services,
applicable to all Serial Flash drivers:

	- determine devices properties at runtime, based on the READID data (and
perhaps SFDP, if and when it matures!).  This should include supported opcodes
(e.g. READ_1_1_4, READ_1_4_4, READ4B_1_1_4...), operating frequencies, mode and
dummy requirements, means of setting Quad Enable, entering/exiting 4-byte
addressing etc.

	- provide optimal read, write and erase configurations, based on the combined
capabilities of the Serial Flash device and the H/W Controller.

	- device specific configuration (e.g. setting QE, unlock BPx bits, DYB unlocking).

Getting back to reality, I realise undertaking such a task would be a huge
commitment.  I would be keen to put forward my own proposal, but current
circumstances mean that that is unlikely to happen any time soon.

I guess in summary, while I am pleased that this area is being looked at, my own
feeling is that proposed framework needs to be reworked for it to be generally
applicable to other Serial Flash controllers.  Of course, another option would
be to stick with what is currently offered, and make do.

Cheers,

Angus

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

Hi Angus:
   thanks for the long suggestion. it's very useful.

Let's push the spi nor framework.:)
> Hi Huang Shijie,
>
> On 11/27/2013 04:32 AM, Brian Norris wrote:
>> + Lee Jones, others
>>
>> I'd like to keep a similar CC list for the various conversations going
>> on about SPI NOR frameworks.
>>
>> On Tue, Nov 26, 2013 at 02:32:51PM +0800, Huang Shijie wrote:
>>> 1.) Why add a new framework for SPI NOR?
>>>    The SPI-NOR controller such as Freescale's Quadspi controller is working
>>>    in a different way from the SPI bus. It should knows the NOR commands to
>>>    find the right LUT sequence. Unfortunately, the current code can not meet
>>>    this requirement.
> I fully support your argument for introducing a new framework to support Serial
> Flash controllers.  Reiterating my previous comment:
>
> "The kernel offers many examples of others who have struggled with the same
> problem.  Some have chosen to write self-contained drivers (e.g.
> drivers/mtd/devices/spear_smi.c and drivers/mtd/devices/sst251.c) duplicating
> much of m25p80.c, while others have attempted to squeeze into the SPI framework
> (e.g. drivers/spi/spi-tegra20-sflash.c and drivers/spi/spi-falcon.c), relying on
> fragile heuristics to recreate the semantics of m25p80 that is lost over the ,
> current circumstances SPI framework ."
>
> However, having now been through your proposed framework, it would not seem to
> provide a generally applicable solution.  Indeed, other than making the Serial
> Flash commands and the device table available, it is not obvious how it improves
> the situation for your own specific controller either; the
> read/write/read_reg/write_reg callbacks do not offer a great deal more than
> spi_read_then_write().  (Perhaps all will become clear when you submit the
> fsl-quadspi driver.)

> As I see it, the main problem with the current support is that dedicated Serial
> Flash Controllers require a level of semantics that cannot be conveyed over the
> generic SPI framework.  With this in mind, I would start by defining something
> along the lines of a "Serial Flash transfer".  Some initial ramblings of my mind:
>
> /**
>   * struct spi_nor_xfer_cfg - Structure for defining a Serial Flash transfer
>   * @wren:		command for "Write Enable", or 0x00 for not required
why this wren is needed?
>   * @cmd:		command for operation
>   * @cmd_pins:		number of pins to send @cmd (1, 2, 4)
>   * @addr:		address for operation
>   * @addr_pins:		number of pins to send @addr (1, 2, 4)
>   * @addr_width: 	number of address bytes (3,4, or 0 for address not required)
this field has been in the spi_nor{} , it should be a fixed value.
so i think we do not need this argument.
>   * @mode:		mode data
>   * @mode_pins:		number of pins to send @mode (1, 2, 4)
>   * @mode_cycles:	number of mode cycles (0 for mode not required)
>   * @dummy_cycles:	number of dummy cycles (0 for dummy not required)
>   */
> struct spi_nor_xfer_cfg {
> 	uint8_t		wren;
> 	uint8_t		cmd;
> 	int		cmd_pins;
> 	uint32_t	addr;
> 	int		addr_pins;
> 	int		addr_width;
> 	uint32_t	mode;
> 	int		mode_pins;
> 	int		mode_cycles;
> 	int		dummy_cycles;
> };
>
> We could then build up layers of functionality, based on two fundamental primitives:
>
> 	int (*read_xfer)(struct spi_nor_info *info,
> 			 struct spi_nor_xfer_cfg *cfg,
> 			 uint8_t *buf, size_t len);
> 	
> 	int (*write_xfer)(struct spi_nor_info *info,
> 			  struct spi_nor_xfer_cfg *cfg,
> 			  uint8_t *buf, size_t len);
> 	
> Default implementations for standard operations could follow:
>
> 	int (*read_reg)(struct spi_nor_info *info,
> 			uint8_t cmd, uint8_t *reg, int len);
> 	int (*write_reg)(struct spi_nor_info *info,
> 			 uint8_t cmd, uint8_t *reg, int len,
> 			 int wren, int wtr);
>
> 	int (*readid)(struct spi_nor_info *info, uint8_t *readid);
> 	int (*wait_till_ready)(struct spi_nor_info *info);

currently, we poll the status register.
why this hook is needed?

> 	
> 	int (*read)(struct spi_nor_info *info, uint8_t buf, off_t offs, size_t len);
> 	int (*write)(struct spi_nor_info *info, off_t offs, size_t len);
> 	int (*erase_sector)(struct spi_nor_info *info, off_t offs);
I also confused at this hook.

if we need erase_sector, do we still need the erase_chip()?

> Each driver would be required to implement read_xfer() and write_xfer(), and
> then free to either use the default implementations, or override with
> driver-optimised implementations.
>
> In the case of a pure SPI Controller, read_xfer() and write_xfer() would simply
> flatten to spi_messages.  The key benefit is that dedicated Serial Flash
> Controllers would be able to take advantage of the richer semantics offered by
> the 'spi_nor_xfer_cfg' data.
>
> I would also expect the spi-nor framework to provide the following services,
> applicable to all Serial Flash drivers:
>
> 	- determine devices properties at runtime, based on the READID data (and
> perhaps SFDP, if and when it matures!).  This should include supported opcodes
> (e.g. READ_1_1_4, READ_1_4_4, READ4B_1_1_4...), operating frequencies, mode and
> dummy requirements, means of setting Quad Enable, entering/exiting 4-byte
> addressing etc.
>
> 	- provide optimal read, write and erase configurations, based on the combined
> capabilities of the Serial Flash device and the H/W Controller.
>
> 	- device specific configuration (e.g. setting QE, unlock BPx bits, DYB unlocking).
>
Does Jones have any opinion about this?

thanks
Huang Shijie

RE: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Gupta, Pekon]

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

>From: Huang Shijie [mailto:b32955@freescale.com]
>Hi Angus:
>   thanks for the long suggestion. it's very useful.
>
>Let's push the spi nor framework.:)
> 
Yes, indeed nicely explained ..

[...]

>> 	int (*wait_till_ready)(struct spi_nor_info *info);
>
>currently, we poll the status register.
>why this hook is needed?
>
Currently there is no uniformity between different vendors to R/W Status
and Config registers. Also the layout and functionality of various fields in
Status and Config register vary from vendor to vendor. So, either you can
define this for-each vendor.. Or let each driver handle it separately.

[...]

>> 	int (*erase_sector)(struct spi_nor_info *info, off_t offs);
>I also confused at this hook.
>
>if we need erase_sector, do we still need the erase_chip()?
>
Yes, there are multiple types of erase commands..
(1) sector erase.
(2) block erase.
(3) bulk erase or chip erase.
Refer attachment ..


with regards, pekon

[-- Attachment #2: Serial_Flash_commands.png --]
[-- Type: image/png, Size: 59763 bytes --]

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Angus Clark]

Hi Huang Shijie

On 12/02/2013 10:06 AM, Huang Shijie wrote:
> Hi Angus:
>   thanks for the long suggestion. it's very useful.
> 
> Let's push the spi nor framework.:)
[...]
>>
>> /**
>>   * struct spi_nor_xfer_cfg - Structure for defining a Serial Flash
>> transfer
>>   * @wren:        command for "Write Enable", or 0x00 for not required
> why this wren is needed?

Issuing WREN is needed for certain sequences of operations.  I guess your
question relates to why have I included it as part of the 'spi_nor_xfer_cfg'
structure, when it could be controlled from the spi_nor layer with a separate
spi_nor_xfer.

Well, firstly, I am aware of at least one H/W controller that can be optimised
to issue the WREN command as part of a longer erase or write sequence, thereby
avoiding the overheads of issuing a separate spi_nor_xfer.

However, the main reason was to try and come up with a generic representation
for a typical Serial Flash transfer, capturing, where possible, the semantics of
the intended operation, just in case that information can be used by the
underlying H/W in some way.  In this instance, the WREN cycles just represents
another part of the transfer, in the same way as the command, address, mode, and
dummy cycles.

>>   * @cmd:        command for operation
>>   * @cmd_pins:        number of pins to send @cmd (1, 2, 4)
>>   * @addr:        address for operation
>>   * @addr_pins:        number of pins to send @addr (1, 2, 4)
>>   * @addr_width:     number of address bytes (3,4, or 0 for address
>> not required)
> this field has been in the spi_nor{} , it should be a fixed value.
> so i think we do not need this argument.

The aim of the 'spi_nor_xfer_cfg' is to provide an interface between the spi-nor
layer and the underlying H/W driver.   In some cases, the H/W may need to know
the address width (e.g. issuing a new command sequence, with an incremented
address, following an arbitration stall).  You could argue that the H/W driver
should be able query the spi_nor structure, or perhaps store the addr_width
field during an initialisation process, but I feel having it as part of the
'spi_nor_xfer_cfg' provides a cleaner interface between the two layers.

>>   * @mode:        mode data
>>   * @mode_pins:        number of pins to send @mode (1, 2, 4)
>>   * @mode_cycles:    number of mode cycles (0 for mode not required)
>>   * @dummy_cycles:    number of dummy cycles (0 for dummy not required)
>>   */
>> struct spi_nor_xfer_cfg {
>>     uint8_t        wren;
>>     uint8_t        cmd;
>>     int        cmd_pins;
>>     uint32_t    addr;
>>     int        addr_pins;
>>     int        addr_width;
>>     uint32_t    mode;
>>     int        mode_pins;
>>     int        mode_cycles;
>>     int        dummy_cycles;
>> };
>>
>> We could then build up layers of functionality, based on two
>> fundamental primitives:
>>
>>     int (*read_xfer)(struct spi_nor_info *info,
>>              struct spi_nor_xfer_cfg *cfg,
>>              uint8_t *buf, size_t len);
>>     
>>     int (*write_xfer)(struct spi_nor_info *info,
>>               struct spi_nor_xfer_cfg *cfg,
>>               uint8_t *buf, size_t len);
>>     
>> Default implementations for standard operations could follow:
>>
>>     int (*read_reg)(struct spi_nor_info *info,
>>             uint8_t cmd, uint8_t *reg, int len);
>>     int (*write_reg)(struct spi_nor_info *info,
>>              uint8_t cmd, uint8_t *reg, int len,
>>              int wren, int wtr);
>>
>>     int (*readid)(struct spi_nor_info *info, uint8_t *readid);
>>     int (*wait_till_ready)(struct spi_nor_info *info);
> 
> currently, we poll the status register.
> why this hook is needed?

The default implementation would do just as you suggest, and I would expect most
H/W drivers to use the default implementation.  However, some H/W Controllers
can automate the polling of status registers, so having a hook allows the driver
override the default behaviour.

>>     int (*read)(struct spi_nor_info *info, uint8_t buf, off_t offs,
>> size_t len);
>>     int (*write)(struct spi_nor_info *info, off_t offs, size_t len);
>>     int (*erase_sector)(struct spi_nor_info *info, off_t offs);
> I also confused at this hook.
> 
> if we need erase_sector, do we still need the erase_chip()?

Serial Flash devices support various "Erase Sector" commands, and an "Erase
Chip" command.  The erase_sector() and erase_chip() hooks would just form the
appropriate command sequences (e.g. Erase Chip does not require address cycles).

I have to admit, the circumstances that provoke an erase_chip() are rather
limited.  As far as I can see, the MTD tool 'mtd_debug' is the only component
that can lead to an Erase Chip operation, and even then, only when the device is
not partitioned.  However, m25p80 supports Erase Chip, so I thought I would
maintain that support here.

As I said before, my proposal was just some initial ramblings of my mind, and I
am definitely open to discussion and other suggestions.

To be honest, I am just glad that someone is taking a look at this area.
However, whether or not is it possible to come up with something that solves all
our problems remains to be seen.  It might turn out that the various H/W
Controllers are just too different to fit into a single unified framework.

Cheers,

Angus

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

于 2013年12月02日 19:19, Angus Clark 写道:
> To be honest, I am just glad that someone is taking a look at this area.
> However, whether or not is it possible to come up with something that solves all
> our problems remains to be seen.  It might turn out that the various H/W
> Controllers are just too different to fit into a single unified framework.
>
Hi Angus & Pekon:
thanks for the explanations.

I am not sure that after we add more hooks, if it will meet Jones's new 
driver.

i will wait for Jones' comment for several days, and send out the next 
version if he does not reply this thread.


thanks
Huang Shijie

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Lee Jones]

> >To be honest, I am just glad that someone is taking a look at this area.
> >However, whether or not is it possible to come up with something that solves all
> >our problems remains to be seen.  It might turn out that the various H/W
> >Controllers are just too different to fit into a single unified framework.
> >
> Hi Angus & Pekon:
> thanks for the explanations.
> 
> I am not sure that after we add more hooks, if it will meet Jones's
> new driver.

Angus is the author of "Lee's" driver and knows more about it that I
ever will, so whatever he says goes in that regard.

I would like to make a suggestion to Brian though. Even if the new
framework is written within the next couple of months and the
semantics do suit the FSM Controller driver, I'd still like the
implementation that's currently on the list to be applied. That way
we'd have a known good version of the driver which is almost identical
to how ST's internal driver does now. The one that is present out in
the wild (i.e. _real_ products).

I will subsequently volunteer to provide my utmost best efforts to
port the driver over to the new framework as a new task once it has
landed.

> i will wait for Jones' comment for several days, and send out the
> next version if he does not reply this thread.

/me waves \o/

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Brian Norris]

On Tue, Dec 03, 2013 at 08:23:43AM +0000, Lee Jones wrote:
> I would like to make a suggestion to Brian though. Even if the new
> framework is written within the next couple of months and the
> semantics do suit the FSM Controller driver, I'd still like the
> implementation that's currently on the list to be applied. That way
> we'd have a known good version of the driver which is almost identical
> to how ST's internal driver does now. The one that is present out in
> the wild (i.e. _real_ products).

I think this suggestion is feasible: that we might merge this driver
even if it doesn't try to fit to some (still vague) idea of what a "SPI
NOR framework" should be. However, I don't know if the latter argument
is valid; just because it's in real products doesn't mean we really want
it as-is. But I think that in this case, the perfect may be the enemy of
the good -- the "good" driver now is better than the "perfect" driver
that never comes.

> I will subsequently volunteer to provide my utmost best efforts to
> port the driver over to the new framework as a new task once it has
> landed.

I'll hold you to that! Do we have any collateral for that guarantee? ;)

Brian

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Lee Jones]

On Tue, 10 Dec 2013, Brian Norris wrote:

> On Tue, Dec 03, 2013 at 08:23:43AM +0000, Lee Jones wrote:
> > I would like to make a suggestion to Brian though. Even if the new
> > framework is written within the next couple of months and the
> > semantics do suit the FSM Controller driver, I'd still like the
> > implementation that's currently on the list to be applied. That way
> > we'd have a known good version of the driver which is almost identical
> > to how ST's internal driver does now. The one that is present out in
> > the wild (i.e. _real_ products).
> 
> I think this suggestion is feasible: that we might merge this driver
> even if it doesn't try to fit to some (still vague) idea of what a "SPI
> NOR framework" should be. However, I don't know if the latter argument
> is valid; just because it's in real products doesn't mean we really want
> it as-is.

True, it just makes things easier for us internally when we are due
for a kernel version up-lift.

> But I think that in this case, the perfect may be the enemy of
> the good -- the "good" driver now is better than the "perfect" driver
> that never comes.

+1

> > I will subsequently volunteer to provide my utmost best efforts to
> > port the driver over to the new framework as a new task once it has
> > landed.
> 
> I'll hold you to that! Do we have any collateral for that guarantee? ;)

My word is my bond Brian. Failing that, beer? :)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Marek Vasut]

Hi Angus,

_nicely_ explained indeed :)

> Hi Huang Shijie,
> 
> On 11/27/2013 04:32 AM, Brian Norris wrote:
> > + Lee Jones, others
> > 
> > I'd like to keep a similar CC list for the various conversations going
> > on about SPI NOR frameworks.
> > 
> > On Tue, Nov 26, 2013 at 02:32:51PM +0800, Huang Shijie wrote:
> >> 1.) Why add a new framework for SPI NOR?
> >> 
> >>   The SPI-NOR controller such as Freescale's Quadspi controller is
> >>   working in a different way from the SPI bus. It should knows the NOR
> >>   commands to find the right LUT sequence. Unfortunately, the current
> >>   code can not meet this requirement.
> 
> I fully support your argument for introducing a new framework to support
> Serial Flash controllers.

So do I, now I finally get the problem.

> Reiterating my previous comment:
> 
> "The kernel offers many examples of others who have struggled with the same
> problem.  Some have chosen to write self-contained drivers (e.g.
> drivers/mtd/devices/spear_smi.c and drivers/mtd/devices/sst251.c)
> duplicating much of m25p80.c, while others have attempted to squeeze into
> the SPI framework (e.g. drivers/spi/spi-tegra20-sflash.c and
> drivers/spi/spi-falcon.c), relying on fragile heuristics to recreate the
> semantics of m25p80 that is lost over the , current circumstances SPI
> framework ."
> 
> However, having now been through your proposed framework, it would not seem
> to provide a generally applicable solution.  Indeed, other than making the
> Serial Flash commands and the device table available, it is not obvious
> how it improves the situation for your own specific controller either; the
> read/write/read_reg/write_reg callbacks do not offer a great deal more than
> spi_read_then_write().  (Perhaps all will become clear when you submit the
> fsl-quadspi driver.)
> 
> As I see it, the main problem with the current support is that dedicated
> Serial Flash Controllers require a level of semantics that cannot be
> conveyed over the generic SPI framework.  With this in mind, I would start
> by defining something along the lines of a "Serial Flash transfer".  Some
> initial ramblings of my mind:
> 
> /**
>  * struct spi_nor_xfer_cfg - Structure for defining a Serial Flash transfer
>  * @wren:		command for "Write Enable", or 0x00 for not required
>  * @cmd:		command for operation
>  * @cmd_pins:		number of pins to send @cmd (1, 2, 4)
>  * @addr:		address for operation
>  * @addr_pins:		number of pins to send @addr (1, 2, 4)
>  * @addr_width: 	number of address bytes (3,4, or 0 for address not
> required) * @mode:		mode data
>  * @mode_pins:		number of pins to send @mode (1, 2, 4)
>  * @mode_cycles:	number of mode cycles (0 for mode not required)
>  * @dummy_cycles:	number of dummy cycles (0 for dummy not required)
>  */
> struct spi_nor_xfer_cfg {
> 	uint8_t		wren;
> 	uint8_t		cmd;
> 	int		cmd_pins;
> 	uint32_t	addr;
> 	int		addr_pins;
> 	int		addr_width;
> 	uint32_t	mode;
> 	int		mode_pins;
> 	int		mode_cycles;
> 	int		dummy_cycles;
> };
> 
> We could then build up layers of functionality, based on two fundamental
> primitives:
> 
> 	int (*read_xfer)(struct spi_nor_info *info,
> 			 struct spi_nor_xfer_cfg *cfg,
> 			 uint8_t *buf, size_t len);
> 
> 	int (*write_xfer)(struct spi_nor_info *info,
> 			  struct spi_nor_xfer_cfg *cfg,
> 			  uint8_t *buf, size_t len);
> 
> Default implementations for standard operations could follow:
> 
> 	int (*read_reg)(struct spi_nor_info *info,
> 			uint8_t cmd, uint8_t *reg, int len);
> 	int (*write_reg)(struct spi_nor_info *info,
> 			 uint8_t cmd, uint8_t *reg, int len,
> 			 int wren, int wtr);
> 
> 	int (*readid)(struct spi_nor_info *info, uint8_t *readid);
> 	int (*wait_till_ready)(struct spi_nor_info *info);
> 
> 	int (*read)(struct spi_nor_info *info, uint8_t buf, off_t offs, size_t
> len); int (*write)(struct spi_nor_info *info, off_t offs, size_t len);
> 	int (*erase_sector)(struct spi_nor_info *info, off_t offs);
> 
> Each driver would be required to implement read_xfer() and write_xfer(),
> and then free to either use the default implementations, or override with
> driver-optimised implementations.

I checked the VYBRIDRM.pdf datasheet. I grok down that the thing Vybrid does is 
it has such a lookup table. The table has 16 slots , each can contain up to 8 
SPI NOR instructions . These instructions are programmed when the driver 
probe()s I suppose. To do a communication with the SPI NOR, you give the 
controller an index in this lookup table and it will execute this pre-programmed 
sequence of commands on the SPI NOR.

So yes, your API would work nicely with Vybrid. I agree about read_reg() and 
write_reg() functions, they will be needed for reading the standard CR/SR etc 
registers on the SPI NOR and also SPI NOR specific cruft if need be. As for 
readid(), I wonder if we cannot implement it via some generalized read_reg() 
call, I suspect we can for starters. Same goes for wait_till_read(), let's 
implement it in the spi-nor framework first and only if a piece of hardware can 
speed this up significantly by re-implementing it itself, they will change the 
framework.

The read() and write() calls are nice, something like mmap() might be 
interesting here as well, since based on what Poddar explained, his controller 
can somehow map the SPI flash into the CPU address space. I don't know how this 
should work precisely, but again, this can be left until such controller driver 
hits mainline and the benefits of such implementation would become obvious.

Finally, the erase_sector() call would probably need to be extended. I would 
rename it to simple erase(), since we can erase from sub-sectors (4K) to whole 
chip.

btw. I really like the idea of the new SPI NOR framework. The M25P80 driver 
could then be rewritten and plugged into the SPI NOR framework just like any 
other SPI NOR controller driver with "advanced capabilities".

> In the case of a pure SPI Controller, read_xfer() and write_xfer() would
> simply flatten to spi_messages.  The key benefit is that dedicated Serial
> Flash Controllers would be able to take advantage of the richer semantics
> offered by the 'spi_nor_xfer_cfg' data.
> 
> I would also expect the spi-nor framework to provide the following
> services, applicable to all Serial Flash drivers:
> 
> 	- determine devices properties at runtime, based on the READID data (and
> perhaps SFDP, if and when it matures!).  This should include supported
> opcodes (e.g. READ_1_1_4, READ_1_4_4, READ4B_1_1_4...), operating
> frequencies, mode and dummy requirements, means of setting Quad Enable,
> entering/exiting 4-byte addressing etc.
> 
> 	- provide optimal read, write and erase configurations, based on the
> combined capabilities of the Serial Flash device and the H/W Controller.

Talking about read-write-erase, why don't we -- instead of passing buffer -- 
pass a scatterlist ? I suppose that might make it easier for these "advanced" 
controller to do DMA on it.

> 	- device specific configuration (e.g. setting QE, unlock BPx bits, DYB
> unlocking).
> 
> Getting back to reality, I realise undertaking such a task would be a huge
> commitment.  I would be keen to put forward my own proposal, but current
> circumstances mean that that is unlikely to happen any time soon.

I'd love to take this on, but I need to flush my pipeline first ;-( Besides, it 
seems we have an abundance of good engineers here already, so maybe someone will 
actually write it ;-)

> I guess in summary, while I am pleased that this area is being looked at,
> my own feeling is that proposed framework needs to be reworked for it to
> be generally applicable to other Serial Flash controllers.  Of course,
> another option would be to stick with what is currently offered, and make
> do.

Full ACK.

> Cheers,
> 
> Angus

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

于 2013年12月03日 08:32, Marek Vasut 写道:
> Finally, the erase_sector() call would probably need to be extended. I would
> rename it to simple erase(), since we can erase from sub-sectors (4K) to whole
> chip.
yes. this is what i am coding now. I rename it to erase().

thanks
Huang Shijie

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[David Woodhouse]

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

On Fri, 2013-11-29 at 14:52 +0000, Angus Clark wrote:
> 
> As I see it, the main problem with the current support is that dedicated Serial
> Flash Controllers require a level of semantics that cannot be conveyed over the
> generic SPI framework.  With this in mind, I would start by defining something
> along the lines of a "Serial Flash transfer".  

I think this is the best way to describe the issue, and approach the
solution.

We should be looking to extend the generic SPI framework to support what
we need, rather than hacking around it purely on the MTD side.

As such, I'd be very interested in Mark's thoughts on it...

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Brian Norris]

On Tue, Dec 03, 2013 at 02:51:13PM +0000, David Woodhouse wrote:
> On Fri, 2013-11-29 at 14:52 +0000, Angus Clark wrote:
> > 
> > As I see it, the main problem with the current support is that dedicated Serial
> > Flash Controllers require a level of semantics that cannot be conveyed over the
> > generic SPI framework.  With this in mind, I would start by defining something
> > along the lines of a "Serial Flash transfer".  
> 
> I think this is the best way to describe the issue, and approach the
> solution.
> 
> We should be looking to extend the generic SPI framework to support what
> we need, rather than hacking around it purely on the MTD side.
> 
> As such, I'd be very interested in Mark's thoughts on it...

Hmm, I thought we already had (some form of) this explanation
weeks/months ago, yet the conclusion was that Mark doesn't want this in
the SPI framework at all, since a dedicated Serial Flash Controller is
not truly SPI. But perhaps I'm wrong.

On this topic, I have one more stick to throw on this fire; I have some
Broadcom hardware that consists of two pieces:

  (1) a true SPI controller, with single-line-I/O and
  (2) a Serial Flash controller that can accelerate flash read, using
      dual/quad I/O.

The latter hardware cannot handle flash writes, only uses quad I/O for
data (not addresses), has some other peculiarities specific to SPI
flash, and is highly dependent on the former for providing complete SPI
flash support. My current driver is not really upstream-ready, since
(like Huang's original driver), it pretends to be a SPI driver yet
requires "incestuous knowledge" of m25p80 / MTD. But I hope that any new
framework can support hardware like mine.

I don't have public documentation for this hardware, but I can provide
more detail if desired.

I mostly bring this up, though, because it is an example of hardware
that

   (a) can operate as a true SPI device (hardware (1) can handle generic
       SPI transfers), but
   (b) requires knowledge of the details of SPI flash in order to get
       optimal usage out of the acceleration from (2).

In my book, point (a) and (b) hold some bearing over the question of
"where should this SPI NOR framework live", because it is reasonable
that a single driver for this hardware should be able to handle either
true SPI devices or accelerated SPI-NOR flash.

Sorry for my late-ish addition to this discussion. There's been a lot of
noise on this topic, and SPI flash is not the highest priority on my
list for now (although it's certainly on the list).

Brian

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

On Wed, Dec 04, 2013 at 10:44:05AM -0800, Brian Norris wrote:
> 
> I mostly bring this up, though, because it is an example of hardware
> that
> 
>    (a) can operate as a true SPI device (hardware (1) can handle generic
>        SPI transfers), but
>    (b) requires knowledge of the details of SPI flash in order to get
>        optimal usage out of the acceleration from (2).
> 
> In my book, point (a) and (b) hold some bearing over the question of
> "where should this SPI NOR framework live", because it is reasonable
> that a single driver for this hardware should be able to handle either
> true SPI devices or accelerated SPI-NOR flash.

We have add a @read_id hook for the spi-nor layer, you can use it to read out
more info (such as the Device Geometry Definition) for your spi-nor controller driver.

Is it okay?

thanks
Huang Shijie

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Brian Norris]

On Thu, Dec 05, 2013 at 03:12:25PM +0800, Huang Shijie wrote:
> On Wed, Dec 04, 2013 at 10:44:05AM -0800, Brian Norris wrote:
> > 
> > I mostly bring this up, though, because it is an example of hardware
> > that
> > 
> >    (a) can operate as a true SPI device (hardware (1) can handle generic
> >        SPI transfers), but
> >    (b) requires knowledge of the details of SPI flash in order to get
> >        optimal usage out of the acceleration from (2).
> > 
> > In my book, point (a) and (b) hold some bearing over the question of
> > "where should this SPI NOR framework live", because it is reasonable
> > that a single driver for this hardware should be able to handle either
> > true SPI devices or accelerated SPI-NOR flash.
> 
> We have add a @read_id hook for the spi-nor layer, you can use it to read out
> more info (such as the Device Geometry Definition) for your spi-nor controller driver.
> 
> Is it okay?

I'm not sure which @read_id hook you're talking about, as I've only
skimmed the most recent thread, and it's not in the last patch series I
have from you, I don't think. But that is not a response at all to the
points I brought up. My statements have nothing to do with device
geometry detection.

What I was actually addressing:

Your framework aligns with Mark's original suggestion: that the "SPI
NOR" framework should be a separate layer held entirely within the MTD
subsystem, and that any given driver is *either* a "SPI NOR" driver *or*
a "true SPI" driver, not both.

I'm simply describing how my hardware is both, and perhaps (as David
seems to be doing) we should reconsider the premise that this framework
will exist solely and entirely within MTD.

Brian

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

On Thu, Dec 05, 2013 at 12:11:01AM -0800, Brian Norris wrote:
> On Thu, Dec 05, 2013 at 03:12:25PM +0800, Huang Shijie wrote:
> > On Wed, Dec 04, 2013 at 10:44:05AM -0800, Brian Norris wrote:
> > > 
> > > I mostly bring this up, though, because it is an example of hardware
> > > that
> > > 
> > >    (a) can operate as a true SPI device (hardware (1) can handle generic
> > >        SPI transfers), but
> > >    (b) requires knowledge of the details of SPI flash in order to get
> > >        optimal usage out of the acceleration from (2).
> > > 
> > > In my book, point (a) and (b) hold some bearing over the question of
> > > "where should this SPI NOR framework live", because it is reasonable
> > > that a single driver for this hardware should be able to handle either
> > > true SPI devices or accelerated SPI-NOR flash.
> > 
> > We have add a @read_id hook for the spi-nor layer, you can use it to read out
> > more info (such as the Device Geometry Definition) for your spi-nor controller driver.
> > 
> > Is it okay?
> 
> I'm not sure which @read_id hook you're talking about, as I've only
> skimmed the most recent thread, and it's not in the last patch series I
> have from you, I don't think. But that is not a response at all to the
> points I brought up. My statements have nothing to do with device
> geometry detection.

My meaning is You can write a new driver for the (b) with the new spi-nor layer,

In the new driver you can implement the @read_id yourself, and read out the
the necessary info, and implement other hooks for the so-called peculiarities.

> 
> What I was actually addressing:
> 
> Your framework aligns with Mark's original suggestion: that the "SPI
> NOR" framework should be a separate layer held entirely within the MTD
> subsystem, and that any given driver is *either* a "SPI NOR" driver *or*
> a "true SPI" driver, not both.
You can only use a SPI-NOR driver, and you can abandon the "true SPI" driver.

Is there some limits that prevent we just code a SPI-NOR driver for your hardware?

thanks
Huang Shijie

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Brian Norris]

On Thu, Dec 05, 2013 at 03:59:02PM +0800, Huang Shijie wrote:
> On Thu, Dec 05, 2013 at 12:11:01AM -0800, Brian Norris wrote:
> > On Thu, Dec 05, 2013 at 03:12:25PM +0800, Huang Shijie wrote:
> > > On Wed, Dec 04, 2013 at 10:44:05AM -0800, Brian Norris wrote:
> > > > 
> > > > I mostly bring this up, though, because it is an example of hardware
> > > > that
> > > > 
> > > >    (a) can operate as a true SPI device (hardware (1) can handle generic
> > > >        SPI transfers), but
> > > >    (b) requires knowledge of the details of SPI flash in order to get
> > > >        optimal usage out of the acceleration from (2).
> > > > 
> > > > In my book, point (a) and (b) hold some bearing over the question of
> > > > "where should this SPI NOR framework live", because it is reasonable
> > > > that a single driver for this hardware should be able to handle either
> > > > true SPI devices or accelerated SPI-NOR flash.
> > > 
> > > We have add a @read_id hook for the spi-nor layer, you can use it to read out
> > > more info (such as the Device Geometry Definition) for your spi-nor controller driver.
> > > 
> > > Is it okay?
> > 
> > I'm not sure which @read_id hook you're talking about, as I've only
> > skimmed the most recent thread, and it's not in the last patch series I
> > have from you, I don't think. But that is not a response at all to the
> > points I brought up. My statements have nothing to do with device
> > geometry detection.
> 
> My meaning is You can write a new driver for the (b) with the new spi-nor layer,

Yes, I can do that.

> In the new driver you can implement the @read_id yourself, and read out the
> the necessary info, and implement other hooks for the so-called peculiarities.

Without giving a full judgment on the framework yet (I haven't followed
as closely as I'd like), I expect that the SPI NOR API will cover the
needs of my SPI NOR hardware.

> > 
> > What I was actually addressing:
> > 
> > Your framework aligns with Mark's original suggestion: that the "SPI
> > NOR" framework should be a separate layer held entirely within the MTD
> > subsystem, and that any given driver is *either* a "SPI NOR" driver *or*
> > a "true SPI" driver, not both.
> You can only use a SPI-NOR driver, and you can abandon the "true SPI" driver.
> 
> Is there some limits that prevent we just code a SPI-NOR driver for your hardware?

SPI is not used only for SPI NOR; it is useful for other things, like
communicating with off-chip peripherals or microcontrollers, supporting
touchscreens, LEDs, media controllers, etc. So with a SPI NOR framework
living only in the MTD subsystem, I have to write two drivers: one
(under drivers/spi/) to use only-(a) for controlling non-flash SPI
devices, and one (under drivers/mtd/) to manage (a)+(b) on SPI NOR
flash.

If, however, we can define a spi_nor_transfer like Angus suggested, and
if we take that a step further and make it a first-class (but optional)
citizen of the SPI framework, then we could have drivers that support
SPI-only, SPI-NOR-only, or both.

Now, I'm not sure how exactly that sort of proposal would work yet, but
I wanted to test the waters to see
  - if this is a complete distortion of what Angus or David may have
    suggested;
  - if Mark thinks this is reasonable, or if he thinks I'm spouting
    nonsense; and
  - if there is some obvious alternate way to support what I describe
    without moving the SPI-NOR layer into the SPI subsystem.

Brian

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

On Thu, Dec 05, 2013 at 01:20:48AM -0800, Brian Norris wrote:
> SPI is not used only for SPI NOR; it is useful for other things, like
> communicating with off-chip peripherals or microcontrollers, supporting
> touchscreens, LEDs, media controllers, etc. So with a SPI NOR framework
> living only in the MTD subsystem, I have to write two drivers: one
> (under drivers/spi/) to use only-(a) for controlling non-flash SPI
> devices, and one (under drivers/mtd/) to manage (a)+(b) on SPI NOR
> flash.

If the NOR is connected to a SPI bus controller which can attaches other SPI
devices too, I think you use a pure SPI driver to access it;

If the NOR is connected to a SPI-NOR controller such as your spi-nor controller
, you can use a SPI-NOR driver to access it.


Does your NOR flash connect to both the SPI bus controller and SPI-NOR
controller at the same time? :) If it does connect to both of the controllers,
I do not know how to handle it too.	   


> 
> If, however, we can define a spi_nor_transfer like Angus suggested, and
> if we take that a step further and make it a first-class (but optional)
> citizen of the SPI framework, then we could have drivers that support
> SPI-only, SPI-NOR-only, or both.
> 
I guess Mark will not agree with this. :)


thanks
Huang Shijie


> Now, I'm not sure how exactly that sort of proposal would work yet, but
> I wanted to test the waters to see
>   - if this is a complete distortion of what Angus or David may have
>     suggested;
>   - if Mark thinks this is reasonable, or if he thinks I'm spouting
>     nonsense; and
>   - if there is some obvious alternate way to support what I describe
>     without moving the SPI-NOR layer into the SPI subsystem.
> 
> Brian
> 

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Angus Clark]

Hi Brian,

On 12/04/2013 06:44 PM, Brian Norris wrote:
> On this topic, I have one more stick to throw on this fire; I have some
> Broadcom hardware that consists of two pieces:
> 
>   (1) a true SPI controller, with single-line-I/O and
>   (2) a Serial Flash controller that can accelerate flash read, using
>       dual/quad I/O.

We support one platform with properties similar to the hardware you describe,
and such a hybrid configuration was in mind when I was thinking about the
spi-nor framework.  I am not sure how well this will come across over email, but
I envisage something like this:

                             spi-nor
                                |
                                v
                          [spi-nor-xfer]
                                |
         ------------------------------------------------
        |                       |                        |
        v                       v                        v
   spi-wrapper (<------)  hybrid-driver          serial-flash-driver
        |                       |                        |
      [SPI]                     |                        |
        |                       |                        |
        v                       |                        |
generic-spi-driver              |                        |
        |                       |                        |
        v                       v                        v
[generic-SPI-Cntrl]   [SPI-Flash-Read Cntrl]   [SPI-Flash-Controller]

The 'spi-nor' layer is responsible for Serial Flash domain knowledge, and how to
generate 'spi-nor-xfer' messages.  The 'spi-wrapper' would provide an interface
to the generic SPI framework, flattening the 'spi-nor-xfer' message to standard
SPI calls.  The combination of 'spi-nor' and 'spi-wrapper' should achieve what
we have today for the m25p80 driver.  The 'serial-flash-driver' in the example
provides an implementation for a dedicated Serial Flash Controller, making use
of the spi-nor-xfer semantics where required.  The 'hybrid-driver' could use the
'spi-wrapper' implementation for write and erase hooks, and optimised
implementations for read hooks.

[...]
> I mostly bring this up, though, because it is an example of hardware
> that
> 
>    (a) can operate as a true SPI device (hardware (1) can handle generic
>        SPI transfers), but
>    (b) requires knowledge of the details of SPI flash in order to get
>        optimal usage out of the acceleration from (2).
> 
> In my book, point (a) and (b) hold some bearing over the question of
> "where should this SPI NOR framework live", because it is reasonable
> that a single driver for this hardware should be able to handle either
> true SPI devices or accelerated SPI-NOR flash.
> 

Yes, I agree.  At present, within the m25p80 driver, the knowledge of how to
drive Serial Flash devices is entwined with an implementation that assumes a
pure SPI Controller.  Creating an additional layer of abstraction allows us to
separate the two issues, and write drivers that can make us of generic SPI
controllers, or take advantage of accelerated Serial Flash Controllers, or a
combination of both.

I realise it is going to take a significant effort to get this off the ground
and the framework will require some tuning to make sure it can accommodate the
eccentric range of hardware out there.  Unfortunately I am not in a position to
commit much time myself, although I will help advise where I can.  Ironically,
once the ST Bristol site closes next year, I will have lots of time available,
but no access to equipment!

Cheers,

Angus

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

On Thu, Dec 05, 2013 at 02:35:47PM +0000, Angus Clark wrote:
> Hi Brian,
> 
> On 12/04/2013 06:44 PM, Brian Norris wrote:
> > On this topic, I have one more stick to throw on this fire; I have some
> > Broadcom hardware that consists of two pieces:
> > 
> >   (1) a true SPI controller, with single-line-I/O and
> >   (2) a Serial Flash controller that can accelerate flash read, using
> >       dual/quad I/O.
> 
> We support one platform with properties similar to the hardware you describe,
> and such a hybrid configuration was in mind when I was thinking about the
> spi-nor framework.  I am not sure how well this will come across over email, but
> I envisage something like this:
> 
>                              spi-nor
>                                 |
>                                 v
>                           [spi-nor-xfer]
>                                 |
>          ------------------------------------------------
>         |                       |                        |
>         v                       v                        v
>    spi-wrapper (<------)  hybrid-driver          serial-flash-driver
>         |                       |                        |
>       [SPI]                     |                        |
>         |                       |                        |
>         v                       |                        |
> generic-spi-driver              |                        |
>         |                       |                        |
>         v                       v                        v
> [generic-SPI-Cntrl]   [SPI-Flash-Read Cntrl]   [SPI-Flash-Controller]
> 
> The 'spi-nor' layer is responsible for Serial Flash domain knowledge, and how to
> generate 'spi-nor-xfer' messages.  The 'spi-wrapper' would provide an interface
> to the generic SPI framework, flattening the 'spi-nor-xfer' message to standard
> SPI calls.  The combination of 'spi-nor' and 'spi-wrapper' should achieve what
> we have today for the m25p80 driver.  The 'serial-flash-driver' in the example
> provides an implementation for a dedicated Serial Flash Controller, making use
> of the spi-nor-xfer semantics where required.  The 'hybrid-driver' could use the
> 'spi-wrapper' implementation for write and erase hooks, and optimised
> implementations for read hooks.
> 
For the diagram above, I think we can regard the hybrid driver as a kind of
serial-flash-driver too.

thanks
Huang Shijie

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Brian Norris]

On Thu, Dec 05, 2013 at 02:35:47PM +0000, Angus Clark wrote:
> On 12/04/2013 06:44 PM, Brian Norris wrote:
> > On this topic, I have one more stick to throw on this fire; I have some
> > Broadcom hardware that consists of two pieces:
> > 
> >   (1) a true SPI controller, with single-line-I/O and
> >   (2) a Serial Flash controller that can accelerate flash read, using
> >       dual/quad I/O.
> 
> We support one platform with properties similar to the hardware you describe,
> and such a hybrid configuration was in mind when I was thinking about the
> spi-nor framework.  I am not sure how well this will come across over email, but
> I envisage something like this:
> 
>                              spi-nor
>                                 |
>                                 v
>                           [spi-nor-xfer]
>                                 |
>          ------------------------------------------------
>         |                       |                        |
>         v                       v                        v
>    spi-wrapper (<------)  hybrid-driver          serial-flash-driver
>         |                       |                        |
>       [SPI]                     |                        |
>         |                       |                        |
>         v                       |                        |
> generic-spi-driver              |                        |
>         |                       |                        |
>         v                       v                        v
> [generic-SPI-Cntrl]   [SPI-Flash-Read Cntrl]   [SPI-Flash-Controller]
> 
> The 'spi-nor' layer is responsible for Serial Flash domain knowledge, and how to
> generate 'spi-nor-xfer' messages.  The 'spi-wrapper' would provide an interface
> to the generic SPI framework, flattening the 'spi-nor-xfer' message to standard
> SPI calls.  The combination of 'spi-nor' and 'spi-wrapper' should achieve what
> we have today for the m25p80 driver.  The 'serial-flash-driver' in the example
> provides an implementation for a dedicated Serial Flash Controller, making use
> of the spi-nor-xfer semantics where required.  The 'hybrid-driver' could use the
> 'spi-wrapper' implementation for write and erase hooks, and optimised
> implementations for read hooks.

OK, that sounds reasonable. But how do I bind my "generic SPI driver"
with my "SPI NOR accelerated read driver" to represent this? Does the
hybrid driver (my accelerated-read driver) have to register itself as a
SPI protocol driver (struct spi_driver)? The "controller" boundaries
seem to get a little fuzzy under this design.

Brian

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

于 2013年11月29日 22:52, Angus Clark 写道:
> 	int (*write_reg)(struct spi_nor_info *info,
> 			 uint8_t cmd, uint8_t *reg, int len,
> 			 int wren, int wtr);
I guess you add the 'wren' for the issuing write-enable before issuing 
the 'cmd'.

but what's 'wtr' for? i do not know what's the meaning of the 'wtr'.

thanks
Huang Shijie

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Angus Clark]

On 12/04/2013 02:46 AM, Huang Shijie wrote:
> 于 2013年11月29日 22:52, Angus Clark 写道:
>>     int (*write_reg)(struct spi_nor_info *info,
>>              uint8_t cmd, uint8_t *reg, int len,
>>              int wren, int wtr);
> I guess you add the 'wren' for the issuing write-enable before issuing
> the 'cmd'.
> 
> but what's 'wtr' for? i do not know what's the meaning of the 'wtr'.
> 

'wtr' is for "Wait 'til Ready".  Some register writes are instant, while others
require polling of the "Write In Progress" bit.

Cheers,

Angus

RE: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Gupta, Pekon]

>From: Angus Clark [mailto:angus.clark@st.com]
>>On 12/04/2013 02:46 AM, Huang Shijie wrote:
>> 于 2013年11月29日 22:52, Angus Clark 写道:
>>>     int (*write_reg)(struct spi_nor_info *info,
>>>              uint8_t cmd, uint8_t *reg, int len,
>>>              int wren, int wtr);
>> I guess you add the 'wren' for the issuing write-enable before issuing
>> the 'cmd'.
>>
>> but what's 'wtr' for? i do not know what's the meaning of the 'wtr'.
>>
>
>'wtr' is for "Wait 'til Ready".  Some register writes are instant, while others
>require polling of the "Write In Progress" bit.
>
Unless a register is controlling a status of internal state-machine, or something
It should be instantaneously writable.

Also polling should not be part of this particular *(write_reg), if a register
needs to be polled then it should be part of *(write) or *(read) .. Like 
WIP: Write-in-progress bit of flash
generic_flash_write(...) {
	while ((read_reg(STATUS_REG) & WIP) || ~timeout) {
		timeout--;
	};


With this it just came to my mind, that you also need a 'timeout' field
in 'struct spinor-cfg'. This is because different controllers would be
running at different clock speeds (and different SPI_CLK frequencies).
So they need to specify a 'timeout' value for all polling accesses.


with regards, pekon

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Angus Clark]

Hi Pekon,

On 12/04/2013 07:19 AM, Gupta, Pekon wrote:
> Unless a register is controlling a status of internal state-machine, or something
> It should be instantaneously writable.

Yes, that is my point.  Some register writes are instant, other are not,
particularly those that involve non-volatile status or configuration bits.

> Also polling should not be part of this particular *(write_reg), if a register
> needs to be polled then it should be part of *(write) or *(read) .. Like 
> WIP: Write-in-progress bit of flash
> generic_flash_write(...) {
> 	while ((read_reg(STATUS_REG) & WIP) || ~timeout) {
> 		timeout--;
> 	};

I am in two minds about where the WTR loop should live.  On the one hand, moving
it to the "generic_flash_write()" call offers H/W controllers the opportunity to
optimise the polling, if they support such a feature (I know of some that will
in the future).

However, requiring the H/W driver to perform the polling, including knowledge of
the STATUS CMD, and the WIP bit-field, just seems like moving too much
"intelligence" away from the spi-nor layer.  What concerns me is when we get on
to reading/clearing error flags (as now mandated on some Serial Flash devices,
e.g. n25p512 and n25q00a), this will also need to be part of the generic write.

> With this it just came to my mind, that you also need a 'timeout' field
> in 'struct spinor-cfg'. 

Yes, that is a good point.

I would propose that the spi-nor layer retains responsibility for determining
whether or not 'WTR' is required for a particular operation, and a suitable
timeout.  At some point, during initialisation, the H/W driver will need to
inform the spi-nor layer about its capabilities (e.g. support for DUAL or QUAD
mode, any warm-reset requirements, etc), and native support of WIP polling could
be part of this set.  The spi-nor layer could then decide whether to add the WTR
flag and timeout to the spinor-cfg transfer, or to take responsibility itself,
and execute its own polling loop.  This gives greatest flexibility, which seems
to be an important factor, while retaining the knowledge in the spi-nor layer.

Cheers,

Angus

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Marek Vasut]

On Wednesday, December 04, 2013 at 09:21:05 AM, Angus Clark wrote:
> Hi Pekon,
> 
> On 12/04/2013 07:19 AM, Gupta, Pekon wrote:
> > Unless a register is controlling a status of internal state-machine, or
> > something It should be instantaneously writable.
> 
> Yes, that is my point.  Some register writes are instant, other are not,
> particularly those that involve non-volatile status or configuration bits.
> 
> > Also polling should not be part of this particular *(write_reg), if a
> > register needs to be polled then it should be part of *(write) or
> > *(read) .. Like WIP: Write-in-progress bit of flash
> > generic_flash_write(...) {
> > 
> > 	while ((read_reg(STATUS_REG) & WIP) || ~timeout) {
> > 	
> > 		timeout--;
> > 	
> > 	};
> 
> I am in two minds about where the WTR loop should live.  On the one hand,
> moving it to the "generic_flash_write()" call offers H/W controllers the
> opportunity to optimise the polling, if they support such a feature (I
> know of some that will in the future).

I disagree we should bloat the API with features, that will be used by "possible 
future controllers" or "possible single controller", right from the start. The 
real question here is, can the API be designed in such a way, that the SR 
polling will happen in software NOW and only when a controller appears that can 
do polling in HW will the API be extended to support it ?

> However, requiring the H/W driver to perform the polling, including
> knowledge of the STATUS CMD, and the WIP bit-field, just seems like moving
> too much "intelligence" away from the spi-nor layer.  What concerns me is
> when we get on to reading/clearing error flags (as now mandated on some
> Serial Flash devices, e.g. n25p512 and n25q00a), this will also need to be
> part of the generic write.
> 
> > With this it just came to my mind, that you also need a 'timeout' field
> > in 'struct spinor-cfg'.
> 
> Yes, that is a good point.
> 
> I would propose that the spi-nor layer retains responsibility for
> determining whether or not 'WTR' is required for a particular operation,
> and a suitable timeout.  At some point, during initialisation, the H/W
> driver will need to inform the spi-nor layer about its capabilities (e.g.
> support for DUAL or QUAD mode, any warm-reset requirements, etc), and
> native support of WIP polling could be part of this set.  The spi-nor
> layer could then decide whether to add the WTR flag and timeout to the
> spinor-cfg transfer, or to take responsibility itself, and execute its own
> polling loop.  This gives greatest flexibility, which seems to be an
> important factor, while retaining the knowledge in the spi-nor layer.

Sounds good!

> Cheers,
> 
> Angus

Best regards,
Marek Vasut

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

于 2013年12月04日 23:36, Marek Vasut 写道:
> I disagree we should bloat the API with features, that will be used by "possible
> future controllers" or "possible single controller", right from the start. The
> real question here is, can the API be designed in such a way, that the SR
> polling will happen in software NOW and only when a controller appears that can
> do polling in HW will the API be extended to support it ?
ok. Let's add this feature in the future when a real case occurs.

thanks
Huang Shijie

RE: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Gupta, Pekon]

>From: Huang Shijie [mailto:b32955@freescale.com]
>
>>于 2013年12月04日 23:36, Marek Vasut 写道:
>> I disagree we should bloat the API with features, that will be used by "possible
>> future controllers" or "possible single controller", right from the start. The
>> real question here is, can the API be designed in such a way, that the SR
>> polling will happen in software NOW and only when a controller appears that can
>> do polling in HW will the API be extended to support it ?
>ok. Let's add this feature in the future when a real case occurs.
>
Sorry got lost.. Can you please summarize following plans for your next patch:

(1) Polling of status registers should be 
  (a) done in S/W (generic driver)
  (b) done controller driver, which can be optimized for specific hardware

(2) How do you plan to have timeout value, (as serial flash can be pretty slow
   to access) ?
  (a) independent for each access
       - read timeouts per sector, which can be multiplied with read_len/sector_size
       - write timeout per sector, which can be multiplied with write_len/sector_size
       - erase timeout per block, which can be multiplied with erase_len/block_size
  (b) Any other method, or leave it to controller driver ?

In my view, before proceeding to writing down a patch, I think
we should come-up with a doc/Readme, which defines all API and interface
structs (taking Angus' proposal as baseline). And then refine that spec first.
Otherwise there may be too many revisions spent on patch RFC's .. 

Any thoughts ?
And is there a space in kernel_docs for such specs ?

With regards, pekon

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

On Thu, Dec 05, 2013 at 05:43:05AM +0000, Gupta, Pekon wrote:
> >From: Huang Shijie [mailto:b32955@freescale.com]
> >
> >>于 2013年12月04日 23:36, Marek Vasut 写道:
> >> I disagree we should bloat the API with features, that will be used by "possible
> >> future controllers" or "possible single controller", right from the start. The
> >> real question here is, can the API be designed in such a way, that the SR
> >> polling will happen in software NOW and only when a controller appears that can
> >> do polling in HW will the API be extended to support it ?
> >ok. Let's add this feature in the future when a real case occurs.
> >
> Sorry got lost.. Can you please summarize following plans for your next patch:
> 
> (1) Polling of status registers should be 
>   (a) done in S/W (generic driver)
>   (b) done controller driver, which can be optimized for specific hardware
> 
For (b), we can implement the @wait_till_ready hook for the controller driver.

For (a), we use the default hook for @wait_till_ready.



> (2) How do you plan to have timeout value, (as serial flash can be pretty slow
>    to access) ?
>   (a) independent for each access
>        - read timeouts per sector, which can be multiplied with read_len/sector_size
>        - write timeout per sector, which can be multiplied with write_len/sector_size
>        - erase timeout per block, which can be multiplied with erase_len/block_size
>   (b) Any other method, or leave it to controller driver ?
> 

Since i have no idea about this kind of spi-nor controller, so I prefer the (b). 

> In my view, before proceeding to writing down a patch, I think
> we should come-up with a doc/Readme, which defines all API and interface
> structs (taking Angus' proposal as baseline). And then refine that spec first.
> Otherwise there may be too many revisions spent on patch RFC's .. 
> 
> Any thoughts ?
> And is there a space in kernel_docs for such specs ?

maybe the best solution is I send out the new version as soon as possible.

thanks
Huang Shijie

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Marek Vasut]

Dear Huang Shijie,

> 1.) Why add a new framework for SPI NOR?
>   The SPI-NOR controller such as Freescale's Quadspi controller is working
>   in a different way from the SPI bus. It should knows the NOR commands to
>   find the right LUT sequence. Unfortunately, the current code can not meet
>   this requirement.

Is there any kind of documentation for this controller available? I cannot quite 
understand how this controller works and why can it not be used with our current 
infrastructure.

Best regards,
Marek Vasut

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Sourav Poddar]

Dear Marek Vasut, Huang,
On Wednesday 27 November 2013 02:57 PM, Marek Vasut wrote:
> Dear Huang Shijie,
>
>> 1.) Why add a new framework for SPI NOR?
>>    The SPI-NOR controller such as Freescale's Quadspi controller is working
>>    in a different way from the SPI bus. It should knows the NOR commands to
>>    find the right LUT sequence. Unfortunately, the current code can not meet
>>    this requirement.
> Is there any kind of documentation for this controller available? I cannot quite
> understand how this controller works and why can it not be used with our current
> infrastructure.
>
I do have a similar requirement where my controller need to be configured
from slave info. I have submiited a series in the mtd list adding that 
portion
of handling such cases. Here, is the patch which specific to m25p80 part.
http://patchwork.ozlabs.org/patch/294285/

The whole series can be found here:
https://www.mail-archive.com/linux-omap@vger.kernel.org/msg98691.html
> Best regards,
> Marek Vasut

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Marek Vasut]

Dear Sourav Poddar,

> Dear Marek Vasut, Huang,
> 
> On Wednesday 27 November 2013 02:57 PM, Marek Vasut wrote:
> > Dear Huang Shijie,
> > 
> >> 1.) Why add a new framework for SPI NOR?
> >> 
> >>    The SPI-NOR controller such as Freescale's Quadspi controller is
> >>    working in a different way from the SPI bus. It should knows the NOR
> >>    commands to find the right LUT sequence. Unfortunately, the current
> >>    code can not meet this requirement.
> > 
> > Is there any kind of documentation for this controller available? I
> > cannot quite understand how this controller works and why can it not be
> > used with our current infrastructure.
> 
> I do have a similar requirement where my controller need to be configured
> from slave info. I have submiited a series in the mtd list adding that
> portion
> of handling such cases. Here, is the patch which specific to m25p80 part.
> http://patchwork.ozlabs.org/patch/294285/
> 
> The whole series can be found here:
> https://www.mail-archive.com/linux-omap@vger.kernel.org/msg98691.html

Is this TI QSPI the same thing as the Altera QSPI controller please ?

Otherwise, I seriously believe you and Huang should work on a common 
infrastructure. I would first like to understand how is the controller in DRA7xx 
different from regular SPI controller though. Is there any kind of documentation 
I could study please?

Best regards,
Marek Vasut

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Sourav Poddar]

Dear Marek Vasut,
On Wednesday 27 November 2013 03:36 PM, Marek Vasut wrote:
> Dear Sourav Poddar,
>
>> Dear Marek Vasut, Huang,
>>
>> On Wednesday 27 November 2013 02:57 PM, Marek Vasut wrote:
>>> Dear Huang Shijie,
>>>
>>>> 1.) Why add a new framework for SPI NOR?
>>>>
>>>>     The SPI-NOR controller such as Freescale's Quadspi controller is
>>>>     working in a different way from the SPI bus. It should knows the NOR
>>>>     commands to find the right LUT sequence. Unfortunately, the current
>>>>     code can not meet this requirement.
>>> Is there any kind of documentation for this controller available? I
>>> cannot quite understand how this controller works and why can it not be
>>> used with our current infrastructure.
>> I do have a similar requirement where my controller need to be configured
>> from slave info. I have submiited a series in the mtd list adding that
>> portion
>> of handling such cases. Here, is the patch which specific to m25p80 part.
>> http://patchwork.ozlabs.org/patch/294285/
>>
>> The whole series can be found here:
>> https://www.mail-archive.com/linux-omap@vger.kernel.org/msg98691.html
> Is this TI QSPI the same thing as the Altera QSPI controller please ?
>
No, its differenet.
> Otherwise, I seriously believe you and Huang should work on a common
> infrastructure. I would first like to understand how is the controller in DRA7xx
> different from regular SPI controller though. Is there any kind of documentation
> I could study please?
>
Sorry, we dont have a public document yet.

Though, this is what ti qspi contoller has

It supports two modes of operation, one is SPI mode(normal), other is
the memory mapped read mode.

For SPI mode, the state machine remains the same as it is with other spi 
controller
available.

For memory mapped, there is something more which we need to do around ..

1. There is a qspi "set up" register available, which needs to be filled 
with
      information like flash opcode, dummy bytes etc. In short, flash 
specific
      details.
2   if the above register is configured with the required opcodes, then 
whenever
      we need to use memory mapped operations, we need to do is to 
switch our
      qspi controller to memory mapped mode.
     Switching of this mode to memory mapped needs
      a )  write to a particular qspi register
      b)   write to control module(optional based on SOC).

3. Once the above steps are configured, then the flash data will be 
mapped to a
     particular memory address(SOC specific) from where the flash data  
can be read.

The series 
https://www.mail-archive.com/linux-omap@vger.kernel.org/msg98691.html
tries to work on the above features, and tries to add a support for the 
same in the
spi framework and m25p80 code.

As you see in my patches, once we take care of the above points and add 
support
for memory mapped in m25p80 and qspi, then while doing a read in m25p80 
we can
do memcpy at the beginning of m25p80_read and can bypass the entire SPI
framework for memory mapped read operation. Throughput almost gets 
doubles with this,
as compared to normal SPI operations.

Please get back, if you need more info on this concept.

> Best regards,
> Marek Vasut

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Marek Vasut]

Dear Sourav Poddar,

> Dear Marek Vasut,
> 
> On Wednesday 27 November 2013 03:36 PM, Marek Vasut wrote:
> > Dear Sourav Poddar,
> > 
> >> Dear Marek Vasut, Huang,
> >> 
> >> On Wednesday 27 November 2013 02:57 PM, Marek Vasut wrote:
> >>> Dear Huang Shijie,
> >>> 
> >>>> 1.) Why add a new framework for SPI NOR?
> >>>> 
> >>>>     The SPI-NOR controller such as Freescale's Quadspi controller is
> >>>>     working in a different way from the SPI bus. It should knows the
> >>>>     NOR commands to find the right LUT sequence. Unfortunately, the
> >>>>     current code can not meet this requirement.
> >>> 
> >>> Is there any kind of documentation for this controller available? I
> >>> cannot quite understand how this controller works and why can it not be
> >>> used with our current infrastructure.
> >> 
> >> I do have a similar requirement where my controller need to be
> >> configured from slave info. I have submiited a series in the mtd list
> >> adding that portion
> >> of handling such cases. Here, is the patch which specific to m25p80
> >> part. http://patchwork.ozlabs.org/patch/294285/
> >> 
> >> The whole series can be found here:
> >> https://www.mail-archive.com/linux-omap@vger.kernel.org/msg98691.html
> > 
> > Is this TI QSPI the same thing as the Altera QSPI controller please ?
> 
> No, its differenet.
> 
> > Otherwise, I seriously believe you and Huang should work on a common
> > infrastructure. I would first like to understand how is the controller in
> > DRA7xx different from regular SPI controller though. Is there any kind
> > of documentation I could study please?
> 
> Sorry, we dont have a public document yet.

Sorry for the delayed reply. I am processing the input on the QSPI and I'm 
finally starting to understand what's going on in here.

> Though, this is what ti qspi contoller has
> 
> It supports two modes of operation, one is SPI mode(normal), other is
> the memory mapped read mode.
> 
> For SPI mode, the state machine remains the same as it is with other spi
> controller
> available.
> 
> For memory mapped, there is something more which we need to do around ..
> 
> 1. There is a qspi "set up" register available, which needs to be filled
> with
>       information like flash opcode, dummy bytes etc. In short, flash
> specific
>       details.
> 2   if the above register is configured with the required opcodes, then
> whenever
>       we need to use memory mapped operations, we need to do is to
> switch our
>       qspi controller to memory mapped mode.
>      Switching of this mode to memory mapped needs
>       a )  write to a particular qspi register
>       b)   write to control module(optional based on SOC).
> 
> 3. Once the above steps are configured, then the flash data will be
> mapped to a
>      particular memory address(SOC specific) from where the flash data
> can be read.

OK, but is the memory mapped mode of any use (but for booting I suppose) ? How 
does it handle large SPI NOR flashes (we have spansion devices as big as 
128MiB), does it really hog a _large_ amount of address space from the CPU 
address space ? Or is the operation somehow indexed ? Why is it better than 
using DMA?

> The series
> https://www.mail-archive.com/linux-omap@vger.kernel.org/msg98691.html
> tries to work on the above features, and tries to add a support for the
> same in the
> spi framework and m25p80 code.
> 
> As you see in my patches, once we take care of the above points and add
> support
> for memory mapped in m25p80 and qspi, then while doing a read in m25p80
> we can
> do memcpy at the beginning of m25p80_read and can bypass the entire SPI
> framework for memory mapped read operation. Throughput almost gets
> doubles with this,
> as compared to normal SPI operations.
> 
> Please get back, if you need more info on this concept.

I am getting there, I'm just trying to wrap my head around these news here. 
Thanks for explaining in so much detail, it's very helpful!

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Sourav Poddar]

Dear Marek Vasut,
On Tuesday 03 December 2013 05:29 AM, Marek Vasut wrote:
> Dear Sourav Poddar,
>
>> Dear Marek Vasut,
>>
>> On Wednesday 27 November 2013 03:36 PM, Marek Vasut wrote:
>>> Dear Sourav Poddar,
>>>
>>>> Dear Marek Vasut, Huang,
>>>>
>>>> On Wednesday 27 November 2013 02:57 PM, Marek Vasut wrote:
>>>>> Dear Huang Shijie,
>>>>>
>>>>>> 1.) Why add a new framework for SPI NOR?
>>>>>>
>>>>>>      The SPI-NOR controller such as Freescale's Quadspi controller is
>>>>>>      working in a different way from the SPI bus. It should knows the
>>>>>>      NOR commands to find the right LUT sequence. Unfortunately, the
>>>>>>      current code can not meet this requirement.
>>>>> Is there any kind of documentation for this controller available? I
>>>>> cannot quite understand how this controller works and why can it not be
>>>>> used with our current infrastructure.
>>>> I do have a similar requirement where my controller need to be
>>>> configured from slave info. I have submiited a series in the mtd list
>>>> adding that portion
>>>> of handling such cases. Here, is the patch which specific to m25p80
>>>> part. http://patchwork.ozlabs.org/patch/294285/
>>>>
>>>> The whole series can be found here:
>>>> https://www.mail-archive.com/linux-omap@vger.kernel.org/msg98691.html
>>> Is this TI QSPI the same thing as the Altera QSPI controller please ?
>> No, its differenet.
>>
>>> Otherwise, I seriously believe you and Huang should work on a common
>>> infrastructure. I would first like to understand how is the controller in
>>> DRA7xx different from regular SPI controller though. Is there any kind
>>> of documentation I could study please?
>> Sorry, we dont have a public document yet.
> Sorry for the delayed reply. I am processing the input on the QSPI and I'm
> finally starting to understand what's going on in here.
>
Thanks for the response.
>> Though, this is what ti qspi contoller has
>>
>> It supports two modes of operation, one is SPI mode(normal), other is
>> the memory mapped read mode.
>>
>> For SPI mode, the state machine remains the same as it is with other spi
>> controller
>> available.
>>
>> For memory mapped, there is something more which we need to do around ..
>>
>> 1. There is a qspi "set up" register available, which needs to be filled
>> with
>>        information like flash opcode, dummy bytes etc. In short, flash
>> specific
>>        details.
>> 2   if the above register is configured with the required opcodes, then
>> whenever
>>        we need to use memory mapped operations, we need to do is to
>> switch our
>>        qspi controller to memory mapped mode.
>>       Switching of this mode to memory mapped needs
>>        a )  write to a particular qspi register
>>        b)   write to control module(optional based on SOC).
>>
>> 3. Once the above steps are configured, then the flash data will be
>> mapped to a
>>       particular memory address(SOC specific) from where the flash data
>> can be read.
> OK, but is the memory mapped mode of any use (but for booting I suppose) ? How
> does it handle large SPI NOR flashes (we have spansion devices as big as
> 128MiB), does it really hog a _large_ amount of address space from the CPU
> address space ? Or is the operation somehow indexed ? Why is it better than
> using DMA?
>
Memory mapped will be of use whenever we try to read the flash content.
Instead of going through the entire SPI framework, and raising 
interrupts, we can
memcpy the flash contents. I am using it for mounting a jffs2 filesystem.

For me the memory mapped regions are like in the range 5c000000 - 
5fffffff, so
I can handle flash as large as 64MB.

As far as its comparison with DMA is concerned, I cant comment much 
about it.
My Qspi controller does not support DMA :(:(. So, memory mapped becomes the
best option option for me.
>> The series
>> https://www.mail-archive.com/linux-omap@vger.kernel.org/msg98691.html
>> tries to work on the above features, and tries to add a support for the
>> same in the
>> spi framework and m25p80 code.
>>
>> As you see in my patches, once we take care of the above points and add
>> support
>> for memory mapped in m25p80 and qspi, then while doing a read in m25p80
>> we can
>> do memcpy at the beginning of m25p80_read and can bypass the entire SPI
>> framework for memory mapped read operation. Throughput almost gets
>> doubles with this,
>> as compared to normal SPI operations.
>>
>> Please get back, if you need more info on this concept.
> I am getting there, I'm just trying to wrap my head around these news here.
> Thanks for explaining in so much detail, it's very helpful!
Thanks!

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Marek Vasut]

Dear Sourav Poddar,

> Dear Marek Vasut,
> 
> On Tuesday 03 December 2013 05:29 AM, Marek Vasut wrote:
> > Dear Sourav Poddar,
> > 
> >> Dear Marek Vasut,
> >> 
> >> On Wednesday 27 November 2013 03:36 PM, Marek Vasut wrote:
> >>> Dear Sourav Poddar,
> >>> 
> >>>> Dear Marek Vasut, Huang,
> >>>> 
> >>>> On Wednesday 27 November 2013 02:57 PM, Marek Vasut wrote:
> >>>>> Dear Huang Shijie,
> >>>>> 
> >>>>>> 1.) Why add a new framework for SPI NOR?
> >>>>>> 
> >>>>>>      The SPI-NOR controller such as Freescale's Quadspi controller
> >>>>>>      is working in a different way from the SPI bus. It should
> >>>>>>      knows the NOR commands to find the right LUT sequence.
> >>>>>>      Unfortunately, the current code can not meet this requirement.
> >>>>> 
> >>>>> Is there any kind of documentation for this controller available? I
> >>>>> cannot quite understand how this controller works and why can it not
> >>>>> be used with our current infrastructure.
> >>>> 
> >>>> I do have a similar requirement where my controller need to be
> >>>> configured from slave info. I have submiited a series in the mtd list
> >>>> adding that portion
> >>>> of handling such cases. Here, is the patch which specific to m25p80
> >>>> part. http://patchwork.ozlabs.org/patch/294285/
> >>>> 
> >>>> The whole series can be found here:
> >>>> https://www.mail-archive.com/linux-omap@vger.kernel.org/msg98691.html
> >>> 
> >>> Is this TI QSPI the same thing as the Altera QSPI controller please ?
> >> 
> >> No, its differenet.
> >> 
> >>> Otherwise, I seriously believe you and Huang should work on a common
> >>> infrastructure. I would first like to understand how is the controller
> >>> in DRA7xx different from regular SPI controller though. Is there any
> >>> kind of documentation I could study please?
> >> 
> >> Sorry, we dont have a public document yet.
> > 
> > Sorry for the delayed reply. I am processing the input on the QSPI and
> > I'm finally starting to understand what's going on in here.
> 
> Thanks for the response.
> 
> >> Though, this is what ti qspi contoller has
> >> 
> >> It supports two modes of operation, one is SPI mode(normal), other is
> >> the memory mapped read mode.
> >> 
> >> For SPI mode, the state machine remains the same as it is with other spi
> >> controller
> >> available.
> >> 
> >> For memory mapped, there is something more which we need to do around ..
> >> 
> >> 1. There is a qspi "set up" register available, which needs to be filled
> >> with
> >> 
> >>        information like flash opcode, dummy bytes etc. In short, flash
> >> 
> >> specific
> >> 
> >>        details.
> >> 
> >> 2   if the above register is configured with the required opcodes, then
> >> whenever
> >> 
> >>        we need to use memory mapped operations, we need to do is to
> >> 
> >> switch our
> >> 
> >>        qspi controller to memory mapped mode.
> >>       
> >>       Switching of this mode to memory mapped needs
> >>       
> >>        a )  write to a particular qspi register
> >>        b)   write to control module(optional based on SOC).
> >> 
> >> 3. Once the above steps are configured, then the flash data will be
> >> mapped to a
> >> 
> >>       particular memory address(SOC specific) from where the flash data
> >> 
> >> can be read.
> > 
> > OK, but is the memory mapped mode of any use (but for booting I suppose)
> > ? How does it handle large SPI NOR flashes (we have spansion devices as
> > big as 128MiB), does it really hog a _large_ amount of address space
> > from the CPU address space ? Or is the operation somehow indexed ? Why
> > is it better than using DMA?
> 
> Memory mapped will be of use whenever we try to read the flash content.
> Instead of going through the entire SPI framework, and raising
> interrupts, we can
> memcpy the flash contents. I am using it for mounting a jffs2 filesystem.
> 
> For me the memory mapped regions are like in the range 5c000000 -
> 5fffffff, so
> I can handle flash as large as 64MB.
> 
> As far as its comparison with DMA is concerned, I cant comment much
> about it.
> My Qspi controller does not support DMA :(:(. So, memory mapped becomes the
> best option option for me.

OK, understood. So to sling large chunks of memory from SPI NOR to your DRAM, 
you need to issue these two steps in a loop:

1) write into the controller register the starting address of the SPI flash 
which you want to have available via the mmap interface
2) memcpy() from this mmaped area to DRAM

correct? Won't the second step be pretty CPU-intensive?

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Sourav Poddar]

Dear Marek Vasut,
On Tuesday 03 December 2013 07:12 PM, Marek Vasut wrote:
> Dear Sourav Poddar,
>
>> Dear Marek Vasut,
>>
>> On Tuesday 03 December 2013 05:29 AM, Marek Vasut wrote:
>>> Dear Sourav Poddar,
>>>
>>>> Dear Marek Vasut,
>>>>
>>>> On Wednesday 27 November 2013 03:36 PM, Marek Vasut wrote:
>>>>> Dear Sourav Poddar,
>>>>>
>>>>>> Dear Marek Vasut, Huang,
>>>>>>
>>>>>> On Wednesday 27 November 2013 02:57 PM, Marek Vasut wrote:
>>>>>>> Dear Huang Shijie,
>>>>>>>
>>>>>>>> 1.) Why add a new framework for SPI NOR?
>>>>>>>>
>>>>>>>>       The SPI-NOR controller such as Freescale's Quadspi controller
>>>>>>>>       is working in a different way from the SPI bus. It should
>>>>>>>>       knows the NOR commands to find the right LUT sequence.
>>>>>>>>       Unfortunately, the current code can not meet this requirement.
>>>>>>> Is there any kind of documentation for this controller available? I
>>>>>>> cannot quite understand how this controller works and why can it not
>>>>>>> be used with our current infrastructure.
>>>>>> I do have a similar requirement where my controller need to be
>>>>>> configured from slave info. I have submiited a series in the mtd list
>>>>>> adding that portion
>>>>>> of handling such cases. Here, is the patch which specific to m25p80
>>>>>> part. http://patchwork.ozlabs.org/patch/294285/
>>>>>>
>>>>>> The whole series can be found here:
>>>>>> https://www.mail-archive.com/linux-omap@vger.kernel.org/msg98691.html
>>>>> Is this TI QSPI the same thing as the Altera QSPI controller please ?
>>>> No, its differenet.
>>>>
>>>>> Otherwise, I seriously believe you and Huang should work on a common
>>>>> infrastructure. I would first like to understand how is the controller
>>>>> in DRA7xx different from regular SPI controller though. Is there any
>>>>> kind of documentation I could study please?
>>>> Sorry, we dont have a public document yet.
>>> Sorry for the delayed reply. I am processing the input on the QSPI and
>>> I'm finally starting to understand what's going on in here.
>> Thanks for the response.
>>
>>>> Though, this is what ti qspi contoller has
>>>>
>>>> It supports two modes of operation, one is SPI mode(normal), other is
>>>> the memory mapped read mode.
>>>>
>>>> For SPI mode, the state machine remains the same as it is with other spi
>>>> controller
>>>> available.
>>>>
>>>> For memory mapped, there is something more which we need to do around ..
>>>>
>>>> 1. There is a qspi "set up" register available, which needs to be filled
>>>> with
>>>>
>>>>         information like flash opcode, dummy bytes etc. In short, flash
>>>>
>>>> specific
>>>>
>>>>         details.
>>>>
>>>> 2   if the above register is configured with the required opcodes, then
>>>> whenever
>>>>
>>>>         we need to use memory mapped operations, we need to do is to
>>>>
>>>> switch our
>>>>
>>>>         qspi controller to memory mapped mode.
>>>>
>>>>        Switching of this mode to memory mapped needs
>>>>
>>>>         a )  write to a particular qspi register
>>>>         b)   write to control module(optional based on SOC).
>>>>
>>>> 3. Once the above steps are configured, then the flash data will be
>>>> mapped to a
>>>>
>>>>        particular memory address(SOC specific) from where the flash data
>>>>
>>>> can be read.
>>> OK, but is the memory mapped mode of any use (but for booting I suppose)
>>> ? How does it handle large SPI NOR flashes (we have spansion devices as
>>> big as 128MiB), does it really hog a _large_ amount of address space
>>> from the CPU address space ? Or is the operation somehow indexed ? Why
>>> is it better than using DMA?
>> Memory mapped will be of use whenever we try to read the flash content.
>> Instead of going through the entire SPI framework, and raising
>> interrupts, we can
>> memcpy the flash contents. I am using it for mounting a jffs2 filesystem.
>>
>> For me the memory mapped regions are like in the range 5c000000 -
>> 5fffffff, so
>> I can handle flash as large as 64MB.
>>
>> As far as its comparison with DMA is concerned, I cant comment much
>> about it.
>> My Qspi controller does not support DMA :(:(. So, memory mapped becomes the
>> best option option for me.
> OK, understood. So to sling large chunks of memory from SPI NOR to your DRAM,
> you need to issue these two steps in a loop:
>
> 1) write into the controller register the starting address of the SPI flash
> which you want to have available via the mmap interface
> 2) memcpy() from this mmaped area to DRAM
>
> correct? Won't the second step be pretty CPU-intensive

No, we dont need to write the starting address in any register.

1. we need to write opcodes(flash specific) in a qspi set up register.
2. Switch to mmap mode using qspi SWITCH register.

Memory mapped address need to be avilable though to m25p80_read api to
do memcpy, which is currently done by get_buf api.

We dont need to do the steps in a loop.
Point1 above is one time configurable.
point2 above need to be done whenever we want to use mmap operations.

memcpy(buf, base_addr + from, len) , where   len <= min(FLASH_SIZE, MMAP 
region)

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Marek Vasut]

Dear Sourav Poddar,

> Dear Marek Vasut,
> 
> On Tuesday 03 December 2013 07:12 PM, Marek Vasut wrote:
> > Dear Sourav Poddar,
> > 
> >> Dear Marek Vasut,
> >> 
> >> On Tuesday 03 December 2013 05:29 AM, Marek Vasut wrote:
> >>> Dear Sourav Poddar,
> >>> 
> >>>> Dear Marek Vasut,
> >>>> 
> >>>> On Wednesday 27 November 2013 03:36 PM, Marek Vasut wrote:
> >>>>> Dear Sourav Poddar,
> >>>>> 
> >>>>>> Dear Marek Vasut, Huang,
> >>>>>> 
> >>>>>> On Wednesday 27 November 2013 02:57 PM, Marek Vasut wrote:
> >>>>>>> Dear Huang Shijie,
> >>>>>>> 
> >>>>>>>> 1.) Why add a new framework for SPI NOR?
> >>>>>>>> 
> >>>>>>>>       The SPI-NOR controller such as Freescale's Quadspi
> >>>>>>>>       controller is working in a different way from the SPI bus.
> >>>>>>>>       It should knows the NOR commands to find the right LUT
> >>>>>>>>       sequence. Unfortunately, the current code can not meet this
> >>>>>>>>       requirement.
> >>>>>>> 
> >>>>>>> Is there any kind of documentation for this controller available? I
> >>>>>>> cannot quite understand how this controller works and why can it
> >>>>>>> not be used with our current infrastructure.
> >>>>>> 
> >>>>>> I do have a similar requirement where my controller need to be
> >>>>>> configured from slave info. I have submiited a series in the mtd
> >>>>>> list adding that portion
> >>>>>> of handling such cases. Here, is the patch which specific to m25p80
> >>>>>> part. http://patchwork.ozlabs.org/patch/294285/
> >>>>>> 
> >>>>>> The whole series can be found here:
> >>>>>> https://www.mail-archive.com/linux-omap@vger.kernel.org/msg98691.htm
> >>>>>> l
> >>>>> 
> >>>>> Is this TI QSPI the same thing as the Altera QSPI controller please ?
> >>>> 
> >>>> No, its differenet.
> >>>> 
> >>>>> Otherwise, I seriously believe you and Huang should work on a common
> >>>>> infrastructure. I would first like to understand how is the
> >>>>> controller in DRA7xx different from regular SPI controller though.
> >>>>> Is there any kind of documentation I could study please?
> >>>> 
> >>>> Sorry, we dont have a public document yet.
> >>> 
> >>> Sorry for the delayed reply. I am processing the input on the QSPI and
> >>> I'm finally starting to understand what's going on in here.
> >> 
> >> Thanks for the response.
> >> 
> >>>> Though, this is what ti qspi contoller has
> >>>> 
> >>>> It supports two modes of operation, one is SPI mode(normal), other is
> >>>> the memory mapped read mode.
> >>>> 
> >>>> For SPI mode, the state machine remains the same as it is with other
> >>>> spi controller
> >>>> available.
> >>>> 
> >>>> For memory mapped, there is something more which we need to do around
> >>>> ..
> >>>> 
> >>>> 1. There is a qspi "set up" register available, which needs to be
> >>>> filled with
> >>>> 
> >>>>         information like flash opcode, dummy bytes etc. In short,
> >>>>         flash
> >>>> 
> >>>> specific
> >>>> 
> >>>>         details.
> >>>> 
> >>>> 2   if the above register is configured with the required opcodes,
> >>>> then whenever
> >>>> 
> >>>>         we need to use memory mapped operations, we need to do is to
> >>>> 
> >>>> switch our
> >>>> 
> >>>>         qspi controller to memory mapped mode.
> >>>>        
> >>>>        Switching of this mode to memory mapped needs
> >>>>        
> >>>>         a )  write to a particular qspi register
> >>>>         b)   write to control module(optional based on SOC).
> >>>> 
> >>>> 3. Once the above steps are configured, then the flash data will be
> >>>> mapped to a
> >>>> 
> >>>>        particular memory address(SOC specific) from where the flash
> >>>>        data
> >>>> 
> >>>> can be read.
> >>> 
> >>> OK, but is the memory mapped mode of any use (but for booting I
> >>> suppose) ? How does it handle large SPI NOR flashes (we have spansion
> >>> devices as big as 128MiB), does it really hog a _large_ amount of
> >>> address space from the CPU address space ? Or is the operation somehow
> >>> indexed ? Why is it better than using DMA?
> >> 
> >> Memory mapped will be of use whenever we try to read the flash content.
> >> Instead of going through the entire SPI framework, and raising
> >> interrupts, we can
> >> memcpy the flash contents. I am using it for mounting a jffs2
> >> filesystem.
> >> 
> >> For me the memory mapped regions are like in the range 5c000000 -
> >> 5fffffff, so
> >> I can handle flash as large as 64MB.
> >> 
> >> As far as its comparison with DMA is concerned, I cant comment much
> >> about it.
> >> My Qspi controller does not support DMA :(:(. So, memory mapped becomes
> >> the best option option for me.
> > 
> > OK, understood. So to sling large chunks of memory from SPI NOR to your
> > DRAM, you need to issue these two steps in a loop:
> > 
> > 1) write into the controller register the starting address of the SPI
> > flash which you want to have available via the mmap interface
> > 2) memcpy() from this mmaped area to DRAM
> > 
> > correct? Won't the second step be pretty CPU-intensive
> 
> No, we dont need to write the starting address in any register.

OK, but how does this handle for example Spansion S70FL01GS , which is 1 GBit 
SPI NOR (128MB) if your memory map window is only 64MB?
 
> 1. we need to write opcodes(flash specific) in a qspi set up register.
> 2. Switch to mmap mode using qspi SWITCH register.
>
> Memory mapped address need to be avilable though to m25p80_read api to
> do memcpy, which is currently done by get_buf api.

OK, got it.
 
> We dont need to do the steps in a loop.
> Point1 above is one time configurable.
> point2 above need to be done whenever we want to use mmap operations.

OK, but copying the SPI NOR will be pretty CPU intensive, correct? You'd need to 
do memcpy() on a full 64MB of stuff on the CPU. I mean, that's what we had DMA 
for thus far, so the CPU can do more useful things.

> memcpy(buf, base_addr + from, len) , where   len <= min(FLASH_SIZE, MMAP
> region)

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Sourav Poddar]

Dear Marek Vasut,
On Tuesday 03 December 2013 07:49 PM, Marek Vasut wrote:
> Dear Sourav Poddar,
>
>> Dear Marek Vasut,
>>
>> On Tuesday 03 December 2013 07:12 PM, Marek Vasut wrote:
>>> Dear Sourav Poddar,
>>>
>>>> Dear Marek Vasut,
>>>>
>>>> On Tuesday 03 December 2013 05:29 AM, Marek Vasut wrote:
>>>>> Dear Sourav Poddar,
>>>>>
>>>>>> Dear Marek Vasut,
>>>>>>
>>>>>> On Wednesday 27 November 2013 03:36 PM, Marek Vasut wrote:
>>>>>>> Dear Sourav Poddar,
>>>>>>>
>>>>>>>> Dear Marek Vasut, Huang,
>>>>>>>>
>>>>>>>> On Wednesday 27 November 2013 02:57 PM, Marek Vasut wrote:
>>>>>>>>> Dear Huang Shijie,
>>>>>>>>>
>>>>>>>>>> 1.) Why add a new framework for SPI NOR?
>>>>>>>>>>
>>>>>>>>>>        The SPI-NOR controller such as Freescale's Quadspi
>>>>>>>>>>        controller is working in a different way from the SPI bus.
>>>>>>>>>>        It should knows the NOR commands to find the right LUT
>>>>>>>>>>        sequence. Unfortunately, the current code can not meet this
>>>>>>>>>>        requirement.
>>>>>>>>> Is there any kind of documentation for this controller available? I
>>>>>>>>> cannot quite understand how this controller works and why can it
>>>>>>>>> not be used with our current infrastructure.
>>>>>>>> I do have a similar requirement where my controller need to be
>>>>>>>> configured from slave info. I have submiited a series in the mtd
>>>>>>>> list adding that portion
>>>>>>>> of handling such cases. Here, is the patch which specific to m25p80
>>>>>>>> part. http://patchwork.ozlabs.org/patch/294285/
>>>>>>>>
>>>>>>>> The whole series can be found here:
>>>>>>>> https://www.mail-archive.com/linux-omap@vger.kernel.org/msg98691.htm
>>>>>>>> l
>>>>>>> Is this TI QSPI the same thing as the Altera QSPI controller please ?
>>>>>> No, its differenet.
>>>>>>
>>>>>>> Otherwise, I seriously believe you and Huang should work on a common
>>>>>>> infrastructure. I would first like to understand how is the
>>>>>>> controller in DRA7xx different from regular SPI controller though.
>>>>>>> Is there any kind of documentation I could study please?
>>>>>> Sorry, we dont have a public document yet.
>>>>> Sorry for the delayed reply. I am processing the input on the QSPI and
>>>>> I'm finally starting to understand what's going on in here.
>>>> Thanks for the response.
>>>>
>>>>>> Though, this is what ti qspi contoller has
>>>>>>
>>>>>> It supports two modes of operation, one is SPI mode(normal), other is
>>>>>> the memory mapped read mode.
>>>>>>
>>>>>> For SPI mode, the state machine remains the same as it is with other
>>>>>> spi controller
>>>>>> available.
>>>>>>
>>>>>> For memory mapped, there is something more which we need to do around
>>>>>> ..
>>>>>>
>>>>>> 1. There is a qspi "set up" register available, which needs to be
>>>>>> filled with
>>>>>>
>>>>>>          information like flash opcode, dummy bytes etc. In short,
>>>>>>          flash
>>>>>>
>>>>>> specific
>>>>>>
>>>>>>          details.
>>>>>>
>>>>>> 2   if the above register is configured with the required opcodes,
>>>>>> then whenever
>>>>>>
>>>>>>          we need to use memory mapped operations, we need to do is to
>>>>>>
>>>>>> switch our
>>>>>>
>>>>>>          qspi controller to memory mapped mode.
>>>>>>
>>>>>>         Switching of this mode to memory mapped needs
>>>>>>
>>>>>>          a )  write to a particular qspi register
>>>>>>          b)   write to control module(optional based on SOC).
>>>>>>
>>>>>> 3. Once the above steps are configured, then the flash data will be
>>>>>> mapped to a
>>>>>>
>>>>>>         particular memory address(SOC specific) from where the flash
>>>>>>         data
>>>>>>
>>>>>> can be read.
>>>>> OK, but is the memory mapped mode of any use (but for booting I
>>>>> suppose) ? How does it handle large SPI NOR flashes (we have spansion
>>>>> devices as big as 128MiB), does it really hog a _large_ amount of
>>>>> address space from the CPU address space ? Or is the operation somehow
>>>>> indexed ? Why is it better than using DMA?
>>>> Memory mapped will be of use whenever we try to read the flash content.
>>>> Instead of going through the entire SPI framework, and raising
>>>> interrupts, we can
>>>> memcpy the flash contents. I am using it for mounting a jffs2
>>>> filesystem.
>>>>
>>>> For me the memory mapped regions are like in the range 5c000000 -
>>>> 5fffffff, so
>>>> I can handle flash as large as 64MB.
>>>>
>>>> As far as its comparison with DMA is concerned, I cant comment much
>>>> about it.
>>>> My Qspi controller does not support DMA :(:(. So, memory mapped becomes
>>>> the best option option for me.
>>> OK, understood. So to sling large chunks of memory from SPI NOR to your
>>> DRAM, you need to issue these two steps in a loop:
>>>
>>> 1) write into the controller register the starting address of the SPI
>>> flash which you want to have available via the mmap interface
>>> 2) memcpy() from this mmaped area to DRAM
>>>
>>> correct? Won't the second step be pretty CPU-intensive
>> No, we dont need to write the starting address in any register.
> OK, but how does this handle for example Spansion S70FL01GS , which is 1 GBit
> SPI NOR (128MB) if your memory map window is only 64MB?
So, the code added in m25p80 does not care about the flash size. It 
works for me
for 64MB flash spansion (S25fl256s) and Macronix( mx66l51235l, 128 MB) 
flash.
That memory mapped region info will from device tree. You can see 
patch16/17) of my
series.
so in m25p80_read,
  memcpy(buf, base + from, len)
where,
base= memmaped base region
          ex: ioremap(0x5c000000)

len =  can be anything, (64mb, 128 mb etc..). Just we need to
make sure that we have ioremapped the required region through dt.

For me, SOC takes care of the memory mapped region required.

DRA7xx board with 64mb spansion flash has mmap region (5c000000 - 5fffffff)
AM43x board with 128mb macronix flash has mmap region (30000000 - 33fffffff)
>
>> 1. we need to write opcodes(flash specific) in a qspi set up register.
>> 2. Switch to mmap mode using qspi SWITCH register.
>>
>> Memory mapped address need to be avilable though to m25p80_read api to
>> do memcpy, which is currently done by get_buf api.
> OK, got it.
>
>> We dont need to do the steps in a loop.
>> Point1 above is one time configurable.
>> point2 above need to be done whenever we want to use mmap operations.
> OK, but copying the SPI NOR will be pretty CPU intensive, correct? You'd need to
> do memcpy() on a full 64MB of stuff on the CPU. I mean, that's what we had DMA
> for thus far, so the CPU can do more useful things.
Yes, but there is no DMA available for my controller.
>> memcpy(buf, base_addr + from, len) , where   len<= min(FLASH_SIZE, MMAP
>> region)

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Marek Vasut]

Dear Sourav Poddar,

> Dear Marek Vasut,
> 
> On Tuesday 03 December 2013 07:49 PM, Marek Vasut wrote:
> > Dear Sourav Poddar,
> > 
> >> Dear Marek Vasut,
> >> 
> >> On Tuesday 03 December 2013 07:12 PM, Marek Vasut wrote:
> >>> Dear Sourav Poddar,
> >>> 
> >>>> Dear Marek Vasut,
> >>>> 
> >>>> On Tuesday 03 December 2013 05:29 AM, Marek Vasut wrote:
> >>>>> Dear Sourav Poddar,
> >>>>> 
> >>>>>> Dear Marek Vasut,
> >>>>>> 
> >>>>>> On Wednesday 27 November 2013 03:36 PM, Marek Vasut wrote:
> >>>>>>> Dear Sourav Poddar,
> >>>>>>> 
> >>>>>>>> Dear Marek Vasut, Huang,
> >>>>>>>> 
> >>>>>>>> On Wednesday 27 November 2013 02:57 PM, Marek Vasut wrote:
> >>>>>>>>> Dear Huang Shijie,
> >>>>>>>>> 
> >>>>>>>>>> 1.) Why add a new framework for SPI NOR?
> >>>>>>>>>> 
> >>>>>>>>>>        The SPI-NOR controller such as Freescale's Quadspi
> >>>>>>>>>>        controller is working in a different way from the SPI
> >>>>>>>>>>        bus. It should knows the NOR commands to find the right
> >>>>>>>>>>        LUT sequence. Unfortunately, the current code can not
> >>>>>>>>>>        meet this requirement.
> >>>>>>>>> 
> >>>>>>>>> Is there any kind of documentation for this controller available?
> >>>>>>>>> I cannot quite understand how this controller works and why can
> >>>>>>>>> it not be used with our current infrastructure.
> >>>>>>>> 
> >>>>>>>> I do have a similar requirement where my controller need to be
> >>>>>>>> configured from slave info. I have submiited a series in the mtd
> >>>>>>>> list adding that portion
> >>>>>>>> of handling such cases. Here, is the patch which specific to
> >>>>>>>> m25p80 part. http://patchwork.ozlabs.org/patch/294285/
> >>>>>>>> 
> >>>>>>>> The whole series can be found here:
> >>>>>>>> https://www.mail-archive.com/linux-omap@vger.kernel.org/msg98691.h
> >>>>>>>> tm l
> >>>>>>> 
> >>>>>>> Is this TI QSPI the same thing as the Altera QSPI controller please
> >>>>>>> ?
> >>>>>> 
> >>>>>> No, its differenet.
> >>>>>> 
> >>>>>>> Otherwise, I seriously believe you and Huang should work on a
> >>>>>>> common infrastructure. I would first like to understand how is the
> >>>>>>> controller in DRA7xx different from regular SPI controller though.
> >>>>>>> Is there any kind of documentation I could study please?
> >>>>>> 
> >>>>>> Sorry, we dont have a public document yet.
> >>>>> 
> >>>>> Sorry for the delayed reply. I am processing the input on the QSPI
> >>>>> and I'm finally starting to understand what's going on in here.
> >>>> 
> >>>> Thanks for the response.
> >>>> 
> >>>>>> Though, this is what ti qspi contoller has
> >>>>>> 
> >>>>>> It supports two modes of operation, one is SPI mode(normal), other
> >>>>>> is the memory mapped read mode.
> >>>>>> 
> >>>>>> For SPI mode, the state machine remains the same as it is with other
> >>>>>> spi controller
> >>>>>> available.
> >>>>>> 
> >>>>>> For memory mapped, there is something more which we need to do
> >>>>>> around ..
> >>>>>> 
> >>>>>> 1. There is a qspi "set up" register available, which needs to be
> >>>>>> filled with
> >>>>>> 
> >>>>>>          information like flash opcode, dummy bytes etc. In short,
> >>>>>>          flash
> >>>>>> 
> >>>>>> specific
> >>>>>> 
> >>>>>>          details.
> >>>>>> 
> >>>>>> 2   if the above register is configured with the required opcodes,
> >>>>>> then whenever
> >>>>>> 
> >>>>>>          we need to use memory mapped operations, we need to do is
> >>>>>>          to
> >>>>>> 
> >>>>>> switch our
> >>>>>> 
> >>>>>>          qspi controller to memory mapped mode.
> >>>>>>         
> >>>>>>         Switching of this mode to memory mapped needs
> >>>>>>         
> >>>>>>          a )  write to a particular qspi register
> >>>>>>          b)   write to control module(optional based on SOC).
> >>>>>> 
> >>>>>> 3. Once the above steps are configured, then the flash data will be
> >>>>>> mapped to a
> >>>>>> 
> >>>>>>         particular memory address(SOC specific) from where the flash
> >>>>>>         data
> >>>>>> 
> >>>>>> can be read.
> >>>>> 
> >>>>> OK, but is the memory mapped mode of any use (but for booting I
> >>>>> suppose) ? How does it handle large SPI NOR flashes (we have spansion
> >>>>> devices as big as 128MiB), does it really hog a _large_ amount of
> >>>>> address space from the CPU address space ? Or is the operation
> >>>>> somehow indexed ? Why is it better than using DMA?
> >>>> 
> >>>> Memory mapped will be of use whenever we try to read the flash
> >>>> content. Instead of going through the entire SPI framework, and
> >>>> raising interrupts, we can
> >>>> memcpy the flash contents. I am using it for mounting a jffs2
> >>>> filesystem.
> >>>> 
> >>>> For me the memory mapped regions are like in the range 5c000000 -
> >>>> 5fffffff, so
> >>>> I can handle flash as large as 64MB.
> >>>> 
> >>>> As far as its comparison with DMA is concerned, I cant comment much
> >>>> about it.
> >>>> My Qspi controller does not support DMA :(:(. So, memory mapped
> >>>> becomes the best option option for me.
> >>> 
> >>> OK, understood. So to sling large chunks of memory from SPI NOR to your
> >>> DRAM, you need to issue these two steps in a loop:
> >>> 
> >>> 1) write into the controller register the starting address of the SPI
> >>> flash which you want to have available via the mmap interface
> >>> 2) memcpy() from this mmaped area to DRAM
> >>> 
> >>> correct? Won't the second step be pretty CPU-intensive
> >> 
> >> No, we dont need to write the starting address in any register.
> > 
> > OK, but how does this handle for example Spansion S70FL01GS , which is 1
> > GBit SPI NOR (128MB) if your memory map window is only 64MB?
> 
> So, the code added in m25p80 does not care about the flash size. It
> works for me
> for 64MB flash spansion (S25fl256s) and Macronix( mx66l51235l, 128 MB)
> flash.
> That memory mapped region info will from device tree. You can see
> patch16/17) of my
> series.
> so in m25p80_read,
>   memcpy(buf, base + from, len)
> where,
> base= memmaped base region
>           ex: ioremap(0x5c000000)
> 
> len =  can be anything, (64mb, 128 mb etc..). Just we need to
> make sure that we have ioremapped the required region through dt.
> 
> For me, SOC takes care of the memory mapped region required.
> 
> DRA7xx board with 64mb spansion flash has mmap region (5c000000 - 5fffffff)
> AM43x board with 128mb macronix flash has mmap region (30000000 -
> 33fffffff)

You mean 0x3000_0000 - 0x33ff_ffff (with one less 'f'), or am I wrong? But 
0x0400_0000 is only 64MiB, how can this map 128MiB SPI NOR? I am still not 
connecting with you here, I am sorry.

Does the QSPI controller simply chomp away N MiB of CPU address space? How big 
is the maximum N here ? I was under the impression that N=64 , but now I am a 
bit confused by your claim that you can use 128 MiB SPI NOR.

> 
> >> 1. we need to write opcodes(flash specific) in a qspi set up register.
> >> 2. Switch to mmap mode using qspi SWITCH register.
> >> 
> >> Memory mapped address need to be avilable though to m25p80_read api to
> >> do memcpy, which is currently done by get_buf api.
> > 
> > OK, got it.
> > 
> >> We dont need to do the steps in a loop.
> >> Point1 above is one time configurable.
> >> point2 above need to be done whenever we want to use mmap operations.
> > 
> > OK, but copying the SPI NOR will be pretty CPU intensive, correct? You'd
> > need to do memcpy() on a full 64MB of stuff on the CPU. I mean, that's
> > what we had DMA for thus far, so the CPU can do more useful things.
> 
> Yes, but there is no DMA available for my controller.

OK, got it, thanks for explaining!

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Sourav Poddar]

Dear Marek Vasut,
On Tuesday 03 December 2013 08:39 PM, Marek Vasut wrote:
> Dear Sourav Poddar,
>
>> Dear Marek Vasut,
>>
>> On Tuesday 03 December 2013 07:49 PM, Marek Vasut wrote:
>>> Dear Sourav Poddar,
>>>
>>>> Dear Marek Vasut,
>>>>
>>>> On Tuesday 03 December 2013 07:12 PM, Marek Vasut wrote:
>>>>> Dear Sourav Poddar,
>>>>>
>>>>>> Dear Marek Vasut,
>>>>>>
>>>>>> On Tuesday 03 December 2013 05:29 AM, Marek Vasut wrote:
>>>>>>> Dear Sourav Poddar,
>>>>>>>
>>>>>>>> Dear Marek Vasut,
>>>>>>>>
>>>>>>>> On Wednesday 27 November 2013 03:36 PM, Marek Vasut wrote:
>>>>>>>>> Dear Sourav Poddar,
>>>>>>>>>
>>>>>>>>>> Dear Marek Vasut, Huang,
>>>>>>>>>>
>>>>>>>>>> On Wednesday 27 November 2013 02:57 PM, Marek Vasut wrote:
>>>>>>>>>>> Dear Huang Shijie,
>>>>>>>>>>>
>>>>>>>>>>>> 1.) Why add a new framework for SPI NOR?
>>>>>>>>>>>>
>>>>>>>>>>>>         The SPI-NOR controller such as Freescale's Quadspi
>>>>>>>>>>>>         controller is working in a different way from the SPI
>>>>>>>>>>>>         bus. It should knows the NOR commands to find the right
>>>>>>>>>>>>         LUT sequence. Unfortunately, the current code can not
>>>>>>>>>>>>         meet this requirement.
>>>>>>>>>>> Is there any kind of documentation for this controller available?
>>>>>>>>>>> I cannot quite understand how this controller works and why can
>>>>>>>>>>> it not be used with our current infrastructure.
>>>>>>>>>> I do have a similar requirement where my controller need to be
>>>>>>>>>> configured from slave info. I have submiited a series in the mtd
>>>>>>>>>> list adding that portion
>>>>>>>>>> of handling such cases. Here, is the patch which specific to
>>>>>>>>>> m25p80 part. http://patchwork.ozlabs.org/patch/294285/
>>>>>>>>>>
>>>>>>>>>> The whole series can be found here:
>>>>>>>>>> https://www.mail-archive.com/linux-omap@vger.kernel.org/msg98691.h
>>>>>>>>>> tm l
>>>>>>>>> Is this TI QSPI the same thing as the Altera QSPI controller please
>>>>>>>>> ?
>>>>>>>> No, its differenet.
>>>>>>>>
>>>>>>>>> Otherwise, I seriously believe you and Huang should work on a
>>>>>>>>> common infrastructure. I would first like to understand how is the
>>>>>>>>> controller in DRA7xx different from regular SPI controller though.
>>>>>>>>> Is there any kind of documentation I could study please?
>>>>>>>> Sorry, we dont have a public document yet.
>>>>>>> Sorry for the delayed reply. I am processing the input on the QSPI
>>>>>>> and I'm finally starting to understand what's going on in here.
>>>>>> Thanks for the response.
>>>>>>
>>>>>>>> Though, this is what ti qspi contoller has
>>>>>>>>
>>>>>>>> It supports two modes of operation, one is SPI mode(normal), other
>>>>>>>> is the memory mapped read mode.
>>>>>>>>
>>>>>>>> For SPI mode, the state machine remains the same as it is with other
>>>>>>>> spi controller
>>>>>>>> available.
>>>>>>>>
>>>>>>>> For memory mapped, there is something more which we need to do
>>>>>>>> around ..
>>>>>>>>
>>>>>>>> 1. There is a qspi "set up" register available, which needs to be
>>>>>>>> filled with
>>>>>>>>
>>>>>>>>           information like flash opcode, dummy bytes etc. In short,
>>>>>>>>           flash
>>>>>>>>
>>>>>>>> specific
>>>>>>>>
>>>>>>>>           details.
>>>>>>>>
>>>>>>>> 2   if the above register is configured with the required opcodes,
>>>>>>>> then whenever
>>>>>>>>
>>>>>>>>           we need to use memory mapped operations, we need to do is
>>>>>>>>           to
>>>>>>>>
>>>>>>>> switch our
>>>>>>>>
>>>>>>>>           qspi controller to memory mapped mode.
>>>>>>>>
>>>>>>>>          Switching of this mode to memory mapped needs
>>>>>>>>
>>>>>>>>           a )  write to a particular qspi register
>>>>>>>>           b)   write to control module(optional based on SOC).
>>>>>>>>
>>>>>>>> 3. Once the above steps are configured, then the flash data will be
>>>>>>>> mapped to a
>>>>>>>>
>>>>>>>>          particular memory address(SOC specific) from where the flash
>>>>>>>>          data
>>>>>>>>
>>>>>>>> can be read.
>>>>>>> OK, but is the memory mapped mode of any use (but for booting I
>>>>>>> suppose) ? How does it handle large SPI NOR flashes (we have spansion
>>>>>>> devices as big as 128MiB), does it really hog a _large_ amount of
>>>>>>> address space from the CPU address space ? Or is the operation
>>>>>>> somehow indexed ? Why is it better than using DMA?
>>>>>> Memory mapped will be of use whenever we try to read the flash
>>>>>> content. Instead of going through the entire SPI framework, and
>>>>>> raising interrupts, we can
>>>>>> memcpy the flash contents. I am using it for mounting a jffs2
>>>>>> filesystem.
>>>>>>
>>>>>> For me the memory mapped regions are like in the range 5c000000 -
>>>>>> 5fffffff, so
>>>>>> I can handle flash as large as 64MB.
>>>>>>
>>>>>> As far as its comparison with DMA is concerned, I cant comment much
>>>>>> about it.
>>>>>> My Qspi controller does not support DMA :(:(. So, memory mapped
>>>>>> becomes the best option option for me.
>>>>> OK, understood. So to sling large chunks of memory from SPI NOR to your
>>>>> DRAM, you need to issue these two steps in a loop:
>>>>>
>>>>> 1) write into the controller register the starting address of the SPI
>>>>> flash which you want to have available via the mmap interface
>>>>> 2) memcpy() from this mmaped area to DRAM
>>>>>
>>>>> correct? Won't the second step be pretty CPU-intensive
>>>> No, we dont need to write the starting address in any register.
>>> OK, but how does this handle for example Spansion S70FL01GS , which is 1
>>> GBit SPI NOR (128MB) if your memory map window is only 64MB?
>> So, the code added in m25p80 does not care about the flash size. It
>> works for me
>> for 64MB flash spansion (S25fl256s) and Macronix( mx66l51235l, 128 MB)
>> flash.
>> That memory mapped region info will from device tree. You can see
>> patch16/17) of my
>> series.
>> so in m25p80_read,
>>    memcpy(buf, base + from, len)
>> where,
>> base= memmaped base region
>>            ex: ioremap(0x5c000000)
>>
>> len =  can be anything, (64mb, 128 mb etc..). Just we need to
>> make sure that we have ioremapped the required region through dt.
>>
>> For me, SOC takes care of the memory mapped region required.
>>
>> DRA7xx board with 64mb spansion flash has mmap region (5c000000 - 5fffffff)
>> AM43x board with 128mb macronix flash has mmap region (30000000 -
>> 33fffffff)
> You mean 0x3000_0000 - 0x33ff_ffff (with one less 'f'), or am I wrong? But
> 0x0400_0000 is only 64MiB, how can this map 128MiB SPI NOR? I am still not
> connecting with you here, I am sorry.
>
Sorry on my side for confusing you. I got confused with macronix
flash sheet. Actually, here also the flash is 64MB, for which my SOC has
the required memory map address space reserved.
> Does the QSPI controller simply chomp away N MiB of CPU address space? How big
> is the maximum N here ? I was under the impression that N=64 , but now I am a
> bit confused by your claim that you can use 128 MiB SPI NOR.
>
Yes, and for my SOC(DRA7x and am43x), its 64MB.
>>>> 1. we need to write opcodes(flash specific) in a qspi set up register.
>>>> 2. Switch to mmap mode using qspi SWITCH register.
>>>>
>>>> Memory mapped address need to be avilable though to m25p80_read api to
>>>> do memcpy, which is currently done by get_buf api.
>>> OK, got it.
>>>
>>>> We dont need to do the steps in a loop.
>>>> Point1 above is one time configurable.
>>>> point2 above need to be done whenever we want to use mmap operations.
>>> OK, but copying the SPI NOR will be pretty CPU intensive, correct? You'd
>>> need to do memcpy() on a full 64MB of stuff on the CPU. I mean, that's
>>> what we had DMA for thus far, so the CPU can do more useful things.
>> Yes, but there is no DMA available for my controller.
> OK, got it, thanks for explaining!

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Marek Vasut]

Dear Sourav Poddar,

> Dear Marek Vasut,
> 
> On Tuesday 03 December 2013 08:39 PM, Marek Vasut wrote:
> > Dear Sourav Poddar,
> > 
> >> Dear Marek Vasut,
> >> 
> >> On Tuesday 03 December 2013 07:49 PM, Marek Vasut wrote:
> >>> Dear Sourav Poddar,
> >>> 
> >>>> Dear Marek Vasut,
> >>>> 
> >>>> On Tuesday 03 December 2013 07:12 PM, Marek Vasut wrote:
> >>>>> Dear Sourav Poddar,
> >>>>> 
> >>>>>> Dear Marek Vasut,
> >>>>>> 
> >>>>>> On Tuesday 03 December 2013 05:29 AM, Marek Vasut wrote:
> >>>>>>> Dear Sourav Poddar,
> >>>>>>> 
> >>>>>>>> Dear Marek Vasut,
> >>>>>>>> 
> >>>>>>>> On Wednesday 27 November 2013 03:36 PM, Marek Vasut wrote:
> >>>>>>>>> Dear Sourav Poddar,
> >>>>>>>>> 
> >>>>>>>>>> Dear Marek Vasut, Huang,
> >>>>>>>>>> 
> >>>>>>>>>> On Wednesday 27 November 2013 02:57 PM, Marek Vasut wrote:
> >>>>>>>>>>> Dear Huang Shijie,
> >>>>>>>>>>> 
> >>>>>>>>>>>> 1.) Why add a new framework for SPI NOR?
> >>>>>>>>>>>> 
> >>>>>>>>>>>>         The SPI-NOR controller such as Freescale's Quadspi
> >>>>>>>>>>>>         controller is working in a different way from the SPI
> >>>>>>>>>>>>         bus. It should knows the NOR commands to find the
> >>>>>>>>>>>>         right LUT sequence. Unfortunately, the current code
> >>>>>>>>>>>>         can not meet this requirement.
> >>>>>>>>>>> 
> >>>>>>>>>>> Is there any kind of documentation for this controller
> >>>>>>>>>>> available? I cannot quite understand how this controller works
> >>>>>>>>>>> and why can it not be used with our current infrastructure.
> >>>>>>>>>> 
> >>>>>>>>>> I do have a similar requirement where my controller need to be
> >>>>>>>>>> configured from slave info. I have submiited a series in the mtd
> >>>>>>>>>> list adding that portion
> >>>>>>>>>> of handling such cases. Here, is the patch which specific to
> >>>>>>>>>> m25p80 part. http://patchwork.ozlabs.org/patch/294285/
> >>>>>>>>>> 
> >>>>>>>>>> The whole series can be found here:
> >>>>>>>>>> https://www.mail-archive.com/linux-omap@vger.kernel.org/msg98691
> >>>>>>>>>> .h tm l
> >>>>>>>>> 
> >>>>>>>>> Is this TI QSPI the same thing as the Altera QSPI controller
> >>>>>>>>> please ?
> >>>>>>>> 
> >>>>>>>> No, its differenet.
> >>>>>>>> 
> >>>>>>>>> Otherwise, I seriously believe you and Huang should work on a
> >>>>>>>>> common infrastructure. I would first like to understand how is
> >>>>>>>>> the controller in DRA7xx different from regular SPI controller
> >>>>>>>>> though. Is there any kind of documentation I could study please?
> >>>>>>>> 
> >>>>>>>> Sorry, we dont have a public document yet.
> >>>>>>> 
> >>>>>>> Sorry for the delayed reply. I am processing the input on the QSPI
> >>>>>>> and I'm finally starting to understand what's going on in here.
> >>>>>> 
> >>>>>> Thanks for the response.
> >>>>>> 
> >>>>>>>> Though, this is what ti qspi contoller has
> >>>>>>>> 
> >>>>>>>> It supports two modes of operation, one is SPI mode(normal), other
> >>>>>>>> is the memory mapped read mode.
> >>>>>>>> 
> >>>>>>>> For SPI mode, the state machine remains the same as it is with
> >>>>>>>> other spi controller
> >>>>>>>> available.
> >>>>>>>> 
> >>>>>>>> For memory mapped, there is something more which we need to do
> >>>>>>>> around ..
> >>>>>>>> 
> >>>>>>>> 1. There is a qspi "set up" register available, which needs to be
> >>>>>>>> filled with
> >>>>>>>> 
> >>>>>>>>           information like flash opcode, dummy bytes etc. In
> >>>>>>>>           short, flash
> >>>>>>>> 
> >>>>>>>> specific
> >>>>>>>> 
> >>>>>>>>           details.
> >>>>>>>> 
> >>>>>>>> 2   if the above register is configured with the required opcodes,
> >>>>>>>> then whenever
> >>>>>>>> 
> >>>>>>>>           we need to use memory mapped operations, we need to do
> >>>>>>>>           is to
> >>>>>>>> 
> >>>>>>>> switch our
> >>>>>>>> 
> >>>>>>>>           qspi controller to memory mapped mode.
> >>>>>>>>          
> >>>>>>>>          Switching of this mode to memory mapped needs
> >>>>>>>>          
> >>>>>>>>           a )  write to a particular qspi register
> >>>>>>>>           b)   write to control module(optional based on SOC).
> >>>>>>>> 
> >>>>>>>> 3. Once the above steps are configured, then the flash data will
> >>>>>>>> be mapped to a
> >>>>>>>> 
> >>>>>>>>          particular memory address(SOC specific) from where the
> >>>>>>>>          flash data
> >>>>>>>> 
> >>>>>>>> can be read.
> >>>>>>> 
> >>>>>>> OK, but is the memory mapped mode of any use (but for booting I
> >>>>>>> suppose) ? How does it handle large SPI NOR flashes (we have
> >>>>>>> spansion devices as big as 128MiB), does it really hog a _large_
> >>>>>>> amount of address space from the CPU address space ? Or is the
> >>>>>>> operation somehow indexed ? Why is it better than using DMA?
> >>>>>> 
> >>>>>> Memory mapped will be of use whenever we try to read the flash
> >>>>>> content. Instead of going through the entire SPI framework, and
> >>>>>> raising interrupts, we can
> >>>>>> memcpy the flash contents. I am using it for mounting a jffs2
> >>>>>> filesystem.
> >>>>>> 
> >>>>>> For me the memory mapped regions are like in the range 5c000000 -
> >>>>>> 5fffffff, so
> >>>>>> I can handle flash as large as 64MB.
> >>>>>> 
> >>>>>> As far as its comparison with DMA is concerned, I cant comment much
> >>>>>> about it.
> >>>>>> My Qspi controller does not support DMA :(:(. So, memory mapped
> >>>>>> becomes the best option option for me.
> >>>>> 
> >>>>> OK, understood. So to sling large chunks of memory from SPI NOR to
> >>>>> your DRAM, you need to issue these two steps in a loop:
> >>>>> 
> >>>>> 1) write into the controller register the starting address of the SPI
> >>>>> flash which you want to have available via the mmap interface
> >>>>> 2) memcpy() from this mmaped area to DRAM
> >>>>> 
> >>>>> correct? Won't the second step be pretty CPU-intensive
> >>>> 
> >>>> No, we dont need to write the starting address in any register.
> >>> 
> >>> OK, but how does this handle for example Spansion S70FL01GS , which is
> >>> 1 GBit SPI NOR (128MB) if your memory map window is only 64MB?
> >> 
> >> So, the code added in m25p80 does not care about the flash size. It
> >> works for me
> >> for 64MB flash spansion (S25fl256s) and Macronix( mx66l51235l, 128 MB)
> >> flash.
> >> That memory mapped region info will from device tree. You can see
> >> patch16/17) of my
> >> series.
> >> so in m25p80_read,
> >> 
> >>    memcpy(buf, base + from, len)
> >> 
> >> where,
> >> base= memmaped base region
> >> 
> >>            ex: ioremap(0x5c000000)
> >> 
> >> len =  can be anything, (64mb, 128 mb etc..). Just we need to
> >> make sure that we have ioremapped the required region through dt.
> >> 
> >> For me, SOC takes care of the memory mapped region required.
> >> 
> >> DRA7xx board with 64mb spansion flash has mmap region (5c000000 -
> >> 5fffffff) AM43x board with 128mb macronix flash has mmap region
> >> (30000000 - 33fffffff)
> > 
> > You mean 0x3000_0000 - 0x33ff_ffff (with one less 'f'), or am I wrong?
> > But 0x0400_0000 is only 64MiB, how can this map 128MiB SPI NOR? I am
> > still not connecting with you here, I am sorry.
> 
> Sorry on my side for confusing you. I got confused with macronix
> flash sheet. Actually, here also the flash is 64MB, for which my SOC has
> the required memory map address space reserved.
> 
> > Does the QSPI controller simply chomp away N MiB of CPU address space?
> > How big is the maximum N here ? I was under the impression that N=64 ,
> > but now I am a bit confused by your claim that you can use 128 MiB SPI
> > NOR.
> 
> Yes, and for my SOC(DRA7x and am43x), its 64MB.

OK, I think I now clearly understand the requirement for your controller. Thank 
you!

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[David Woodhouse]

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

On Tue, 2013-12-03 at 16:09 +0100, Marek Vasut wrote:
> Dear Sourav Poddar,
> 
> > Dear Marek Vasut,
> > 
> > On Tuesday 03 December 2013 07:49 PM, Marek Vasut wrote:
> > > Dear Sourav Poddar,
> > > 
> > >> Dear Marek Vasut,
> > >> 
> > >> On Tuesday 03 December 2013 07:12 PM, Marek Vasut wrote:
> > >>> Dear Sourav Poddar,
> > >>> 
> > >>>> Dear Marek Vasut,
> > >>>> 
> > >>>> On Tuesday 03 December 2013 05:29 AM, Marek Vasut wrote:
> > >>>>> Dear Sourav Poddar,
> > >>>>> 
> > >>>>>> Dear Marek Vasut,
> > >>>>>> 
> > >>>>>> On Wednesday 27 November 2013 03:36 PM, Marek Vasut wrote:
> > >>>>>>> Dear Sourav Poddar,
> > >>>>>>> 
> > >>>>>>>> Dear Marek Vasut, Huang,
> > >>>>>>>> 
> > >>>>>>>> On Wednesday 27 November 2013 02:57 PM, Marek Vasut wrote:
> > >>>>>>>>> Dear Huang Shijie,
> > >>>>>>>>> 

Seriously, guys, can you please trim your citations? Include only what's
absolutely needed for what you're saying right now!

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Brian Norris]

On Tue, Dec 03, 2013 at 03:23:00PM +0000, David Woodhouse wrote:
> Seriously, guys, can you please trim your citations? Include only what's
> absolutely needed for what you're saying right now!

+1

Additionally, when trimming replies, can you please leave a proper

  On <date / time>, <sende> wrote:

line before the quote? It helps to identify who said what in a quote,
and it serves a much better purpose than leaving the 'Dear So-and-so'
intact.

Seeing the following tells me who you were speaking to, not who was
actually speaking:

> > Dear Sourav Poddar,
> > 
> > > Dear Marek Vasut,
> > > 

Particularly, it seems like Marek's and Huang's mailers tend to leave
the attribution line off.

Thanks,
Brian

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Marek Vasut]

On Tuesday, December 03, 2013 at 07:28:08 PM, Brian Norris wrote:
> On Tue, Dec 03, 2013 at 03:23:00PM +0000, David Woodhouse wrote:
> > Seriously, guys, can you please trim your citations? Include only what's
> > absolutely needed for what you're saying right now!
> 
> +1
> 
> Additionally, when trimming replies, can you please leave a proper

Sorry, I was so into the discussion I was carried away. I humbly apologize for 
my misstep.

>   On <date / time>, <sende> wrote:
> 
> line before the quote? It helps to identify who said what in a quote,
> and it serves a much better purpose than leaving the 'Dear So-and-so'
> intact.

Yeah, I was thinking about changing this for a while now.

> Seeing the following tells me who you were speaking to, not who was
> 
> actually speaking:
> > > Dear Sourav Poddar,
> > > 
> > > > Dear Marek Vasut,
> 
> Particularly, it seems like Marek's and Huang's mailers tend to leave
> the attribution line off.

Fixed now I hope.

Best regards,
Marek Vasut

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Huang Shijie]

于 2013年11月27日 17:27, Marek Vasut 写道:
> Is there any kind of documentation for this controller available? I cannot quite
> understand how this controller works and why can it not be used with our current
> infrastructure.
http://cache.freescale.com/files/32bit/doc/ref_manual/VYBRIDRM.pdf

thanks
Huang Shijie

Re: [PATCH 0/4] mtd: spi-nor: add a new framework for SPI NOR

[Marek Vasut]

Dear Huang Shijie,

> 于 2013年11月27日 17:27, Marek Vasut 写道:
> > Is there any kind of documentation for this controller available? I
> > cannot quite understand how this controller works and why can it not be
> > used with our current infrastructure.
> 
> http://cache.freescale.com/files/32bit/doc/ref_manual/VYBRIDRM.pdf

It's a nice bedside reading, I'm making my way through, thanks ;-)

Best regards,
Marek Vasut