[PATCH 1/3] mtd: spi-nor: add id/id_len for flash_info{}

[PATCH 1/3] mtd: spi-nor: add id/id_len for flash_info{}

[Huang Shijie]

This patch adds the id/id_len fields for flash_info{}, and rewrite the
INFO to fill them. And at last, we read out 6 bytes in the spi_nor_read_id(),
and we use these new fields to parse out the correct flash_info.

Signed-off-by: Huang Shijie <shijie.huang@intel.com>
---
 drivers/mtd/spi-nor/spi-nor.c |   39 +++++++++++++++++++++++++--------------
 1 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index b5ad6be..0b69072 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -422,6 +422,8 @@ err:
 	return ret;
 }
 
+#define SPI_NOR_MAX_ID_LEN	6
+
 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,
@@ -430,6 +432,14 @@ struct flash_info {
 	u32		jedec_id;
 	u16             ext_id;
 
+	/*
+	 * This array stores the ID bytes.
+	 * The first three bytes are the JEDIC id.
+	 * JEDEC id zero means "no ID" (most older chips);
+	 */
+	u8		id[SPI_NOR_MAX_ID_LEN];
+	u8		id_len;
+
 	/* The size listed here is what works with SPINOR_OP_SE, which isn't
 	 * necessarily called a "sector" by the vendor.
 	 */
@@ -450,10 +460,19 @@ struct flash_info {
 #define	USE_FSR			0x80	/* use flag status register */
 };
 
+/* Used when the "_ext_id" is two bytes at most */
 #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),					\
+		.id = {							\
+			((_jedec_id) >> 16) & 0xff,			\
+			((_jedec_id) >> 8) & 0xff,			\
+			(_jedec_id) & 0xff,				\
+			((_ext_id) >> 8) & 0xff,			\
+			(_ext_id) & 0xff,				\
+			},						\
+		.id_len = (!(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0))),	\
 		.sector_size = (_sector_size),				\
 		.n_sectors = (_n_sectors),				\
 		.page_size = 256,					\
@@ -642,32 +661,24 @@ EXPORT_SYMBOL_GPL(spi_nor_ids);
 static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
 {
 	int			tmp;
-	u8			id[5];
-	u32			jedec;
-	u16                     ext_jedec;
+	u8			id[SPI_NOR_MAX_ID_LEN];
 	struct flash_info	*info;
 
-	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 5);
+	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, SPI_NOR_MAX_ID_LEN);
 	if (tmp < 0) {
 		dev_dbg(nor->dev, " 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)
+		if (info->id_len) {
+			if (!strncmp(info->id, id, info->id_len))
 				return &spi_nor_ids[tmp];
 		}
 	}
-	dev_err(nor->dev, "unrecognized JEDEC id %06x\n", jedec);
+	dev_err(nor->dev, "unrecognized JEDEC id bytes: %02x, %2x, %2x\n",
+		id[0], id[1], id[2]);
 	return ERR_PTR(-ENODEV);
 }
 
-- 
1.7.7.6

[PATCH 2/3] mtd: spi-nor: remove the jedec_id/ext_id

[Huang Shijie]

The "id" array contains all the information about the JEDEC and the
manufacturer ID info, this patch remove the jedec_id/ext_id from the
flash_info.

Signed-off-by: Huang Shijie <shijie.huang@intel.com>
---
 drivers/mtd/spi-nor/spi-nor.c |   98 +++++++++++++++++++----------------------
 1 files changed, 45 insertions(+), 53 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 0b69072..a0923b0 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -26,7 +26,38 @@
 /* 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)
+#define SPI_NOR_MAX_ID_LEN	6
+
+struct flash_info {
+	/*
+	 * This array stores the ID bytes.
+	 * The first three bytes are the JEDIC id.
+	 * JEDEC id zero means "no ID" (most older chips);
+	 */
+	u8		id[SPI_NOR_MAX_ID_LEN];
+	u8		id_len;
+
+	/* The size listed here is what works with SPINOR_OP_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	/* SPINOR_OP_BE_4K works uniformly */
+#define	SPI_NOR_NO_ERASE	0x02	/* No erase command needed */
+#define	SST_WRITE		0x04	/* use SST byte programming */
+#define	SPI_NOR_NO_FR		0x08	/* Can't do fastread */
+#define	SECT_4K_PMC		0x10	/* SPINOR_OP_BE_4K_PMC works uniformly */
+#define	SPI_NOR_DUAL_READ	0x20    /* Flash supports Dual Read */
+#define	SPI_NOR_QUAD_READ	0x40    /* Flash supports Quad Read */
+#define	USE_FSR			0x80	/* use flag status register */
+};
+
+#define JEDEC_MFR(info)	((info)->id[0])
 
 /*
  * Read the status register, returning its value in the location
@@ -136,13 +167,14 @@ static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
 }
 
 /* Enable/disable 4-byte addressing mode. */
-static inline int set_4byte(struct spi_nor *nor, u32 jedec_id, int enable)
+static inline int
+set_4byte(struct spi_nor *nor, struct flash_info *info, int enable)
 {
 	int status;
 	bool need_wren = false;
 	u8 cmd;
 
-	switch (JEDEC_MFR(jedec_id)) {
+	switch (JEDEC_MFR(info)) {
 	case CFI_MFR_ST: /* Micron, actually */
 		/* Some Micron need WREN command; all will accept it */
 		need_wren = true;
@@ -422,49 +454,9 @@ err:
 	return ret;
 }
 
-#define SPI_NOR_MAX_ID_LEN	6
-
-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;
-
-	/*
-	 * This array stores the ID bytes.
-	 * The first three bytes are the JEDIC id.
-	 * JEDEC id zero means "no ID" (most older chips);
-	 */
-	u8		id[SPI_NOR_MAX_ID_LEN];
-	u8		id_len;
-
-	/* The size listed here is what works with SPINOR_OP_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	/* SPINOR_OP_BE_4K works uniformly */
-#define	SPI_NOR_NO_ERASE	0x02	/* No erase command needed */
-#define	SST_WRITE		0x04	/* use SST byte programming */
-#define	SPI_NOR_NO_FR		0x08	/* Can't do fastread */
-#define	SECT_4K_PMC		0x10	/* SPINOR_OP_BE_4K_PMC works uniformly */
-#define	SPI_NOR_DUAL_READ	0x20    /* Flash supports Dual Read */
-#define	SPI_NOR_QUAD_READ	0x40    /* Flash supports Quad Read */
-#define	USE_FSR			0x80	/* use flag status register */
-};
-
 /* Used when the "_ext_id" is two bytes at most */
 #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),					\
 		.id = {							\
 			((_jedec_id) >> 16) & 0xff,			\
 			((_jedec_id) >> 8) & 0xff,			\
@@ -889,11 +881,11 @@ static int spansion_quad_enable(struct spi_nor *nor)
 	return 0;
 }
 
-static int set_quad_mode(struct spi_nor *nor, u32 jedec_id)
+static int set_quad_mode(struct spi_nor *nor, struct flash_info *info)
 {
 	int status;
 
-	switch (JEDEC_MFR(jedec_id)) {
+	switch (JEDEC_MFR(info)) {
 	case CFI_MFR_MACRONIX:
 		status = macronix_quad_enable(nor);
 		if (status) {
@@ -966,7 +958,7 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
 
 	info = (void *)id->driver_data;
 
-	if (info->jedec_id) {
+	if (info->id_len) {
 		const struct spi_device_id *jid;
 
 		jid = jedec_probe(nor);
@@ -994,9 +986,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
 	 * 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) {
+	if (JEDEC_MFR(info) == CFI_MFR_ATMEL ||
+	    JEDEC_MFR(info) == CFI_MFR_INTEL ||
+	    JEDEC_MFR(info) == CFI_MFR_SST) {
 		write_enable(nor);
 		write_sr(nor, 0);
 	}
@@ -1014,7 +1006,7 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
 	mtd->_read = spi_nor_read;
 
 	/* nor protection support for STmicro chips */
-	if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) {
+	if (JEDEC_MFR(info) == CFI_MFR_ST) {
 		mtd->_lock = spi_nor_lock;
 		mtd->_unlock = spi_nor_unlock;
 	}
@@ -1065,7 +1057,7 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
 
 	/* Quad/Dual-read mode takes precedence over fast/normal */
 	if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) {
-		ret = set_quad_mode(nor, info->jedec_id);
+		ret = set_quad_mode(nor, info);
 		if (ret) {
 			dev_err(dev, "quad mode not supported\n");
 			return ret;
@@ -1101,7 +1093,7 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
 	else if (mtd->size > 0x1000000) {
 		/* enable 4-byte addressing if the device exceeds 16MiB */
 		nor->addr_width = 4;
-		if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) {
+		if (JEDEC_MFR(info) == CFI_MFR_AMD) {
 			/* Dedicated 4-byte command set */
 			switch (nor->flash_read) {
 			case SPI_NOR_QUAD:
@@ -1122,7 +1114,7 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
 			nor->erase_opcode = SPINOR_OP_SE_4B;
 			mtd->erasesize = info->sector_size;
 		} else
-			set_4byte(nor, info->jedec_id, 1);
+			set_4byte(nor, info, 1);
 	} else {
 		nor->addr_width = 3;
 	}
-- 
1.7.7.6

[PATCH 3/3] mtd: spi-nor: add support for s25fl128s

[Huang Shijie]

We need to store the six bytes ID for s25fl128s, since it shares the same
five bytes with s25fl129p1.

This patch adds a macro INFO6 which is used for the six bytes ID flash, and adds
a new item for the s25fl128s.

Signed-off-by: Huang Shijie <shijie.huang@intel.com>
---
 drivers/mtd/spi-nor/spi-nor.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index a0923b0..c130bf7 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -471,6 +471,23 @@ err:
 		.flags = (_flags),					\
 	})
 
+#define INFO6(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\
+	((kernel_ulong_t)&(struct flash_info) {				\
+		.id = {							\
+			((_jedec_id) >> 16) & 0xff,			\
+			((_jedec_id) >> 8) & 0xff,			\
+			(_jedec_id) & 0xff,				\
+			((_ext_id) >> 16) & 0xff,			\
+			((_ext_id) >> 8) & 0xff,			\
+			(_ext_id) & 0xff,				\
+			},						\
+		.id_len = 6,						\
+		.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),				\
@@ -565,6 +582,7 @@ const struct spi_device_id spi_nor_ids[] = {
 	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
 	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
 	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
+	{ "s25fl128s",	INFO6(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_QUAD_READ) },
 	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, 0) },
 	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
 	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
-- 
1.7.7.6

Re: [PATCH 3/3] mtd: spi-nor: add support for s25fl128s

[Fabio Estevam]

Hi Brian,

On Mon, Aug 11, 2014 at 9:54 PM, Huang Shijie <shijie.huang@intel.com> wrote:
> We need to store the six bytes ID for s25fl128s, since it shares the same
> five bytes with s25fl129p1.
>
> This patch adds a macro INFO6 which is used for the six bytes ID flash, and adds
> a new item for the s25fl128s.
>
> Signed-off-by: Huang Shijie <shijie.huang@intel.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index a0923b0..c130bf7 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -471,6 +471,23 @@ err:
>                 .flags = (_flags),                                      \
>         })
>
> +#define INFO6(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)    \
> +       ((kernel_ulong_t)&(struct flash_info) {                         \
> +               .id = {                                                 \
> +                       ((_jedec_id) >> 16) & 0xff,                     \
> +                       ((_jedec_id) >> 8) & 0xff,                      \
> +                       (_jedec_id) & 0xff,                             \
> +                       ((_ext_id) >> 16) & 0xff,                       \
> +                       ((_ext_id) >> 8) & 0xff,                        \
> +                       (_ext_id) & 0xff,                               \
> +                       },                                              \
> +               .id_len = 6,                                            \
> +               .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),                          \
> @@ -565,6 +582,7 @@ const struct spi_device_id spi_nor_ids[] = {
>         { "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
>         { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>         { "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
> +       { "s25fl128s",  INFO6(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_QUAD_READ) },
>         { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, 0) },
>         { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
>         { "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },

We have a s25fl128s flash on imx6sx-sdb board and this patch would be helpful.

Do you see any issues with it?

Thanks,

Fabio Estevam

Re: [PATCH 3/3] mtd: spi-nor: add support for s25fl128s

[Fabio Estevam]

Hi Huang/Brian,

On Fri, Oct 17, 2014 at 3:42 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Brian,
>
> On Mon, Aug 11, 2014 at 9:54 PM, Huang Shijie <shijie.huang@intel.com> wrote:
>> We need to store the six bytes ID for s25fl128s, since it shares the same
>> five bytes with s25fl129p1.
>>
>> This patch adds a macro INFO6 which is used for the six bytes ID flash, and adds
>> a new item for the s25fl128s.
>>
>> Signed-off-by: Huang Shijie <shijie.huang@intel.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c |   18 ++++++++++++++++++
>>  1 files changed, 18 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index a0923b0..c130bf7 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -471,6 +471,23 @@ err:
>>                 .flags = (_flags),                                      \
>>         })
>>
>> +#define INFO6(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)    \
>> +       ((kernel_ulong_t)&(struct flash_info) {                         \
>> +               .id = {                                                 \
>> +                       ((_jedec_id) >> 16) & 0xff,                     \
>> +                       ((_jedec_id) >> 8) & 0xff,                      \
>> +                       (_jedec_id) & 0xff,                             \
>> +                       ((_ext_id) >> 16) & 0xff,                       \
>> +                       ((_ext_id) >> 8) & 0xff,                        \
>> +                       (_ext_id) & 0xff,                               \
>> +                       },                                              \
>> +               .id_len = 6,                                            \
>> +               .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),                          \
>> @@ -565,6 +582,7 @@ const struct spi_device_id spi_nor_ids[] = {
>>         { "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
>>         { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
>>         { "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
>> +       { "s25fl128s",  INFO6(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_QUAD_READ) },
>>         { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, 0) },
>>         { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
>>         { "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
>
> We have a s25fl128s flash on imx6sx-sdb board and this patch would be helpful.
>
> Do you see any issues with it?

Is there still issues preventing this patch to be merged?

I would like to make use of it on a mx6sx-sdb board.

Regards,

Fabio Estevam

Re: [PATCH 3/3] mtd: spi-nor: add support for s25fl128s

[Brian Norris]

On Tue, Nov 04, 2014 at 05:39:07PM -0200, Fabio Estevam wrote:
> Hi Huang/Brian,
> 
> On Fri, Oct 17, 2014 at 3:42 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > Hi Brian,
> >
> > On Mon, Aug 11, 2014 at 9:54 PM, Huang Shijie <shijie.huang@intel.com> wrote:
> >> We need to store the six bytes ID for s25fl128s, since it shares the same
> >> five bytes with s25fl129p1.
> >>
> >> This patch adds a macro INFO6 which is used for the six bytes ID flash, and adds
> >> a new item for the s25fl128s.
> >>
> >> Signed-off-by: Huang Shijie <shijie.huang@intel.com>
> >> ---
> >>  drivers/mtd/spi-nor/spi-nor.c |   18 ++++++++++++++++++
> >>  1 files changed, 18 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> >> index a0923b0..c130bf7 100644
> >> --- a/drivers/mtd/spi-nor/spi-nor.c
> >> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >> @@ -471,6 +471,23 @@ err:
> >>                 .flags = (_flags),                                      \
> >>         })
> >>
> >> +#define INFO6(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)    \
> >> +       ((kernel_ulong_t)&(struct flash_info) {                         \
> >> +               .id = {                                                 \
> >> +                       ((_jedec_id) >> 16) & 0xff,                     \
> >> +                       ((_jedec_id) >> 8) & 0xff,                      \
> >> +                       (_jedec_id) & 0xff,                             \
> >> +                       ((_ext_id) >> 16) & 0xff,                       \
> >> +                       ((_ext_id) >> 8) & 0xff,                        \
> >> +                       (_ext_id) & 0xff,                               \
> >> +                       },                                              \
> >> +               .id_len = 6,                                            \
> >> +               .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),                          \
> >> @@ -565,6 +582,7 @@ const struct spi_device_id spi_nor_ids[] = {
> >>         { "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
> >>         { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
> >>         { "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
> >> +       { "s25fl128s",  INFO6(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_QUAD_READ) },
> >>         { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, 0) },
> >>         { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
> >>         { "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },
> >
> > We have a s25fl128s flash on imx6sx-sdb board and this patch would be helpful.
> >
> > Do you see any issues with it?
> 
> Is there still issues preventing this patch to be merged?

Two things:

(1) My time (I'll get to it)

(2) The spi_nor_ids[] table is still not in great shape. We haven't
    fully agreed on the fallout of this change, and how people should be
    binding against the m25p80 driver in the future:

      commit a5b7616c55e188fe3d6ef686bef402d4703ecb62
      Author: Ben Hutchings <ben@decadent.org.uk>
      Date:   Tue Sep 30 03:14:55 2014 +0100

          mtd: m25p80,spi-nor: Fix module aliases for m25p80

    In the meantime, I'm not too keen on modifying this table. But I'll
    probably merge patches soon anyway.

Brian

Re: [PATCH 2/3] mtd: spi-nor: remove the jedec_id/ext_id

[Brian Norris]

On Tue, Aug 12, 2014 at 08:54:55AM +0800, Huang Shijie wrote:
> The "id" array contains all the information about the JEDEC and the
> manufacturer ID info, this patch remove the jedec_id/ext_id from the
> flash_info.
> 
> Signed-off-by: Huang Shijie <shijie.huang@intel.com>

This patch does not apply any more. Can you rebase?

Brian

Re: [PATCH 2/3] mtd: spi-nor: remove the jedec_id/ext_id

[Rafał Miłecki]

On 5 November 2014 12:16, Brian Norris <computersforpeace@gmail.com> wrote:
> On Tue, Aug 12, 2014 at 08:54:55AM +0800, Huang Shijie wrote:
>> The "id" array contains all the information about the JEDEC and the
>> manufacturer ID info, this patch remove the jedec_id/ext_id from the
>> flash_info.
>>
>> Signed-off-by: Huang Shijie <shijie.huang@intel.com>
>
> This patch does not apply any more. Can you rebase?

Hm, I've just noticed that my patch:
"prefer more specific entries from chips database"
http://patchwork.ozlabs.org/patch/401907/
conflicts with this one.

I think the final solution (after somehow applying both patches)
should be something like:
for (len = 6; i >= 4; len--) {
    for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
        if (tmp->id_len == len && !strncmp(info->id, id, info->id_len)
            return &spi_nor_ids[tmp];
    }
}

Does it make sense to you?

Huang: I think it'll be the most optimal to me rebase my patch on top
on your ones.

Btw. I have two comments regarding 1/3 (can't find it to reply properly):

1) Do we need the if (info->id_len) check?
2) Shouldn't we use memcmp in the if (!strncmp(info->id, id, info->id_len))

-- 
Rafał

Re: [PATCH 2/3] mtd: spi-nor: remove the jedec_id/ext_id

[Huang Shijie]

On Wed, Nov 05, 2014 at 03:16:31AM -0800, Brian Norris wrote:
> On Tue, Aug 12, 2014 at 08:54:55AM +0800, Huang Shijie wrote:
> > The "id" array contains all the information about the JEDEC and the
> > manufacturer ID info, this patch remove the jedec_id/ext_id from the
> > flash_info.
> > 
> > Signed-off-by: Huang Shijie <shijie.huang@intel.com>
> 
> This patch does not apply any more. Can you rebase?
ok, no preblem.

thanks
Huang Shijie

Re: [PATCH 2/3] mtd: spi-nor: remove the jedec_id/ext_id

[Huang Shijie]

On Wed, Nov 05, 2014 at 10:35:09PM +0100, Rafał Miłecki wrote:
> On 5 November 2014 12:16, Brian Norris <computersforpeace@gmail.com> wrote:
> > On Tue, Aug 12, 2014 at 08:54:55AM +0800, Huang Shijie wrote:
> >> The "id" array contains all the information about the JEDEC and the
> >> manufacturer ID info, this patch remove the jedec_id/ext_id from the
> >> flash_info.
> >>
> >> Signed-off-by: Huang Shijie <shijie.huang@intel.com>
> >
> > This patch does not apply any more. Can you rebase?
> 
> Hm, I've just noticed that my patch:
> "prefer more specific entries from chips database"
> http://patchwork.ozlabs.org/patch/401907/
> conflicts with this one.
> 
> I think the final solution (after somehow applying both patches)
> should be something like:
> for (len = 6; i >= 4; len--) {
>     for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
>         if (tmp->id_len == len && !strncmp(info->id, id, info->id_len)
>             return &spi_nor_ids[tmp];
>     }
> }
> 
> Does it make sense to you?
sorry, I can not understand this code. could you please give me a full
patch for your idea? thanks

> 
> Huang: I think it'll be the most optimal to me rebase my patch on top
> on your ones.
It is okay. I will CC to you with the new patch.
> 
> Btw. I have two comments regarding 1/3 (can't find it to reply properly):
> 
> 1) Do we need the if (info->id_len) check?
yes. some chips may have short info->id_len, some chips may have long
info->id_len.

> 2) Shouldn't we use memcmp in the if (!strncmp(info->id, id, info->id_len))
I prefer to memcmp too. But i misused with strncmp.
You can send a patch to fix it. I can give you my Ack. :)

thanks
Huang Shiji

[PATCH] mtd: spi-nor: remove the jedec_id/ext_id

[Huang Shijie]

The "id" array contains all the information about the JEDEC and the
manufacturer ID info, this patch remove the jedec_id/ext_id from the
flash_info.

Signed-off-by: Huang Shijie <shijie.huang@intel.com>
---
  rebase this patch on the latest l2-mtd
---
 drivers/mtd/spi-nor/spi-nor.c |   98 +++++++++++++++++++----------------------
 1 files changed, 45 insertions(+), 53 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 8d6b832..182dd42 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -26,7 +26,38 @@
 /* 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)
+#define SPI_NOR_MAX_ID_LEN	6
+
+struct flash_info {
+	/*
+	 * This array stores the ID bytes.
+	 * The first three bytes are the JEDIC id.
+	 * JEDEC id zero means "no ID" (most older chips);
+	 */
+	u8		id[SPI_NOR_MAX_ID_LEN];
+	u8		id_len;
+
+	/* The size listed here is what works with SPINOR_OP_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	/* SPINOR_OP_BE_4K works uniformly */
+#define	SPI_NOR_NO_ERASE	0x02	/* No erase command needed */
+#define	SST_WRITE		0x04	/* use SST byte programming */
+#define	SPI_NOR_NO_FR		0x08	/* Can't do fastread */
+#define	SECT_4K_PMC		0x10	/* SPINOR_OP_BE_4K_PMC works uniformly */
+#define	SPI_NOR_DUAL_READ	0x20    /* Flash supports Dual Read */
+#define	SPI_NOR_QUAD_READ	0x40    /* Flash supports Quad Read */
+#define	USE_FSR			0x80	/* use flag status register */
+};
+
+#define JEDEC_MFR(info)	((info)->id[0])
 
 static const struct spi_device_id *spi_nor_match_id(const char *name);
 
@@ -138,13 +169,14 @@ static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
 }
 
 /* Enable/disable 4-byte addressing mode. */
-static inline int set_4byte(struct spi_nor *nor, u32 jedec_id, int enable)
+static inline int
+set_4byte(struct spi_nor *nor, struct flash_info *info, int enable)
 {
 	int status;
 	bool need_wren = false;
 	u8 cmd;
 
-	switch (JEDEC_MFR(jedec_id)) {
+	switch (JEDEC_MFR(info)) {
 	case CFI_MFR_ST: /* Micron, actually */
 		/* Some Micron need WREN command; all will accept it */
 		need_wren = true;
@@ -413,49 +445,9 @@ err:
 	return ret;
 }
 
-#define SPI_NOR_MAX_ID_LEN	6
-
-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;
-
-	/*
-	 * This array stores the ID bytes.
-	 * The first three bytes are the JEDIC id.
-	 * JEDEC id zero means "no ID" (most older chips);
-	 */
-	u8		id[SPI_NOR_MAX_ID_LEN];
-	u8		id_len;
-
-	/* The size listed here is what works with SPINOR_OP_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	/* SPINOR_OP_BE_4K works uniformly */
-#define	SPI_NOR_NO_ERASE	0x02	/* No erase command needed */
-#define	SST_WRITE		0x04	/* use SST byte programming */
-#define	SPI_NOR_NO_FR		0x08	/* Can't do fastread */
-#define	SECT_4K_PMC		0x10	/* SPINOR_OP_BE_4K_PMC works uniformly */
-#define	SPI_NOR_DUAL_READ	0x20    /* Flash supports Dual Read */
-#define	SPI_NOR_QUAD_READ	0x40    /* Flash supports Quad Read */
-#define	USE_FSR			0x80	/* use flag status register */
-};
-
 /* Used when the "_ext_id" is two bytes at most */
 #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),					\
 		.id = {							\
 			((_jedec_id) >> 16) & 0xff,			\
 			((_jedec_id) >> 8) & 0xff,			\
@@ -872,11 +864,11 @@ static int spansion_quad_enable(struct spi_nor *nor)
 	return 0;
 }
 
-static int set_quad_mode(struct spi_nor *nor, u32 jedec_id)
+static int set_quad_mode(struct spi_nor *nor, struct flash_info *info)
 {
 	int status;
 
-	switch (JEDEC_MFR(jedec_id)) {
+	switch (JEDEC_MFR(info)) {
 	case CFI_MFR_MACRONIX:
 		status = macronix_quad_enable(nor);
 		if (status) {
@@ -925,7 +917,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 
 	info = (void *)id->driver_data;
 
-	if (info->jedec_id) {
+	if (info->id_len) {
 		const struct spi_device_id *jid;
 
 		jid = spi_nor_read_id(nor);
@@ -953,9 +945,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	 * 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) {
+	if (JEDEC_MFR(info) == CFI_MFR_ATMEL ||
+	    JEDEC_MFR(info) == CFI_MFR_INTEL ||
+	    JEDEC_MFR(info) == CFI_MFR_SST) {
 		write_enable(nor);
 		write_sr(nor, 0);
 	}
@@ -970,7 +962,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	mtd->_read = spi_nor_read;
 
 	/* nor protection support for STmicro chips */
-	if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) {
+	if (JEDEC_MFR(info) == CFI_MFR_ST) {
 		mtd->_lock = spi_nor_lock;
 		mtd->_unlock = spi_nor_unlock;
 	}
@@ -1023,7 +1015,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 
 	/* Quad/Dual-read mode takes precedence over fast/normal */
 	if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) {
-		ret = set_quad_mode(nor, info->jedec_id);
+		ret = set_quad_mode(nor, info);
 		if (ret) {
 			dev_err(dev, "quad mode not supported\n");
 			return ret;
@@ -1059,7 +1051,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	else if (mtd->size > 0x1000000) {
 		/* enable 4-byte addressing if the device exceeds 16MiB */
 		nor->addr_width = 4;
-		if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) {
+		if (JEDEC_MFR(info) == CFI_MFR_AMD) {
 			/* Dedicated 4-byte command set */
 			switch (nor->flash_read) {
 			case SPI_NOR_QUAD:
@@ -1080,7 +1072,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 			nor->erase_opcode = SPINOR_OP_SE_4B;
 			mtd->erasesize = info->sector_size;
 		} else
-			set_4byte(nor, info->jedec_id, 1);
+			set_4byte(nor, info, 1);
 	} else {
 		nor->addr_width = 3;
 	}
-- 
1.7.7.6

Re: [PATCH 2/3] mtd: spi-nor: remove the jedec_id/ext_id

[Rafał Miłecki]

On 6 November 2014 02:09, Huang Shijie <shijie.huang@intel.com> wrote:
>> 1) Do we need the if (info->id_len) check?
> yes. some chips may have short info->id_len, some chips may have long
> info->id_len.

Right, thanks.


>> 2) Shouldn't we use memcmp in the if (!strncmp(info->id, id, info->id_len))
> I prefer to memcmp too. But i misused with strncmp.
> You can send a patch to fix it. I can give you my Ack. :)

Well, you were sending V2 2 hours later anyway... you could fix that in it :|

-- 
Rafał

Re: [PATCH 2/3] mtd: spi-nor: remove the jedec_id/ext_id

[Huang Shijie]

On Thu, Nov 06, 2014 at 07:21:22AM +0100, Rafał Miłecki wrote:
> 
> Well, you were sending V2 2 hours later anyway... you could fix that in it :|
okay.
I will fix it, and send out a new version.

thanks
Huang Shijie

[PATCH V2 1/3] mtd: spi-nor: add id/id_len for flash_info{}

[Rafał Miłecki]

From: Huang Shijie <shijie.huang@intel.com>

This patch adds the id/id_len fields for flash_info{}, and rewrite the
INFO to fill them. And at last, we read out 6 bytes in the spi_nor_read_id(),
and we use these new fields to parse out the correct flash_info.

Signed-off-by: Huang Shijie <shijie.huang@intel.com>
Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
V2: s/strncmp/memcmp/ as we don't compare strings at all
---
 drivers/mtd/spi-nor/spi-nor.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index eafaeeb..b49efee 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -413,6 +413,8 @@ err:
 	return ret;
 }
 
+#define SPI_NOR_MAX_ID_LEN	6
+
 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,
@@ -421,6 +423,14 @@ struct flash_info {
 	u32		jedec_id;
 	u16             ext_id;
 
+	/*
+	 * This array stores the ID bytes.
+	 * The first three bytes are the JEDIC id.
+	 * JEDEC id zero means "no ID" (most older chips);
+	 */
+	u8		id[SPI_NOR_MAX_ID_LEN];
+	u8		id_len;
+
 	/* The size listed here is what works with SPINOR_OP_SE, which isn't
 	 * necessarily called a "sector" by the vendor.
 	 */
@@ -441,10 +451,19 @@ struct flash_info {
 #define	USE_FSR			0x80	/* use flag status register */
 };
 
+/* Used when the "_ext_id" is two bytes at most */
 #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),					\
+		.id = {							\
+			((_jedec_id) >> 16) & 0xff,			\
+			((_jedec_id) >> 8) & 0xff,			\
+			(_jedec_id) & 0xff,				\
+			((_ext_id) >> 8) & 0xff,			\
+			(_ext_id) & 0xff,				\
+			},						\
+		.id_len = (!(_jedec_id) ? 0 : (3 + ((_ext_id) ? 2 : 0))),	\
 		.sector_size = (_sector_size),				\
 		.n_sectors = (_n_sectors),				\
 		.page_size = 256,					\
@@ -636,32 +655,24 @@ static const struct spi_device_id spi_nor_ids[] = {
 static const struct spi_device_id *spi_nor_read_id(struct spi_nor *nor)
 {
 	int			tmp;
-	u8			id[5];
-	u32			jedec;
-	u16                     ext_jedec;
+	u8			id[SPI_NOR_MAX_ID_LEN];
 	struct flash_info	*info;
 
-	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, 5);
+	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, SPI_NOR_MAX_ID_LEN);
 	if (tmp < 0) {
 		dev_dbg(nor->dev, " 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)
+		if (info->id_len) {
+			if (!memcmp(info->id, id, info->id_len))
 				return &spi_nor_ids[tmp];
 		}
 	}
-	dev_err(nor->dev, "unrecognized JEDEC id %06x\n", jedec);
+	dev_err(nor->dev, "unrecognized JEDEC id bytes: %02x, %2x, %2x\n",
+		id[0], id[1], id[2]);
 	return ERR_PTR(-ENODEV);
 }
 
-- 
1.8.4.5

Re: [PATCH V2 1/3] mtd: spi-nor: add id/id_len for flash_info{}

[Huang Shijie]

On Thu, Nov 06, 2014 at 07:34:01AM +0100, Rafał Miłecki wrote:
> From: Huang Shijie <shijie.huang@intel.com>
> 
> This patch adds the id/id_len fields for flash_info{}, and rewrite the
> INFO to fill them. And at last, we read out 6 bytes in the spi_nor_read_id(),
> and we use these new fields to parse out the correct flash_info.
> 
> Signed-off-by: Huang Shijie <shijie.huang@intel.com>
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
thanks a lot.

I was just about to send out the new patch, then i saw this patch. :)

thanks
Huang Shijie

Re: [PATCH V2 1/3] mtd: spi-nor: add id/id_len for flash_info{}

[Brian Norris]

On Thu, Nov 06, 2014 at 07:34:01AM +0100, Rafał Miłecki wrote:
> From: Huang Shijie <shijie.huang@intel.com>
> 
> This patch adds the id/id_len fields for flash_info{}, and rewrite the
> INFO to fill them. And at last, we read out 6 bytes in the spi_nor_read_id(),
> and we use these new fields to parse out the correct flash_info.
> 
> Signed-off-by: Huang Shijie <shijie.huang@intel.com>
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
> V2: s/strncmp/memcmp/ as we don't compare strings at all

Applied to l2-mtd.git. Thanks!

Brian

Re: [PATCH] mtd: spi-nor: remove the jedec_id/ext_id

[Brian Norris]

On Thu, Nov 06, 2014 at 11:24:33AM +0800, Huang Shijie wrote:
> The "id" array contains all the information about the JEDEC and the
> manufacturer ID info, this patch remove the jedec_id/ext_id from the
> flash_info.
> 
> Signed-off-by: Huang Shijie <shijie.huang@intel.com>
> ---
>   rebase this patch on the latest l2-mtd

Applied to l2-mtd.git. Thanks!

Brian

Re: [PATCH 3/3] mtd: spi-nor: add support for s25fl128s

[Brian Norris]

On Tue, Aug 12, 2014 at 08:54:56AM +0800, Huang Shijie wrote:
> We need to store the six bytes ID for s25fl128s, since it shares the same
> five bytes with s25fl129p1.
> 
> This patch adds a macro INFO6 which is used for the six bytes ID flash, and adds
> a new item for the s25fl128s.
> 
> Signed-off-by: Huang Shijie <shijie.huang@intel.com>

Applied to l2-mtd.git. Thanks.

Brian