[PATCH 0/3] Introduce die erase command

[PATCH 0/3] Introduce die erase command

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

This series introduce die erase command and new MT25Q00 Micron devices.

Marcin Krzeminski (3):
  mtd: spi-nor: Add die_cnt field and flags
  mtd: spi-nor: Implement die erase command logic
  mtd: spi-nor: Enable die erase for Micron 1GiB

 drivers/mtd/spi-nor/spi-nor.c | 128 ++++++++++++++++++++++++++++++++++++++----
 include/linux/mtd/spi-nor.h   |   5 ++
 2 files changed, 122 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH 1/3] mtd: spi-nor: Add die_cnt field and flags

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

This commit adds new field and flags that could
be used to signalize that chip support die erase
command.

If DIE_ERASE flag will be selected but die_cnt field
will be 0 chip will not use chip erase command.

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 18 ++++++++++++++++++
 include/linux/mtd/spi-nor.h   |  5 +++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d0fc165..3afe207 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -75,6 +75,7 @@ struct flash_info {
 					 * bit. Must be used with
 					 * SPI_NOR_HAS_LOCK.
 					 */
+#define DIE_ERASE			BIT(10)	/* Support for die erase cmd */
 };
 
 #define JEDEC_MFR(info)	((info)->id[0])
@@ -295,6 +296,23 @@ static int erase_chip(struct spi_nor *nor)
 	return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0);
 }
 
+static int spi_nor_die_erase(struct spi_nor *nor, u32 addr)
+{
+	u8 buf[SPI_NOR_MAX_ADDR_WIDTH];
+	int i;
+
+	dev_dbg(nor->dev, "erase @ 0x%X\n", addr);
+
+	write_enable(nor);
+
+	for (i = nor->addr_width - 1; i >= 0; i--) {
+		buf[i] = addr & 0xff;
+		addr >>= 8;
+	}
+
+	return nor->write_reg(nor, SPINOR_OP_DIE_ERASE, buf, nor->addr_width);
+}
+
 static int spi_nor_lock_and_prep(struct spi_nor *nor, enum spi_nor_ops ops)
 {
 	int ret = 0;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index c425c7b..80154b2 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -50,6 +50,7 @@
 #define SPINOR_OP_BE_4K_PMC	0xd7	/* Erase 4KiB block on PMC chips */
 #define SPINOR_OP_BE_32K	0x52	/* Erase 32KiB block */
 #define SPINOR_OP_CHIP_ERASE	0xc7	/* Erase whole flash chip */
+#define SPINOR_OP_DIE_ERASE	0xc4	/* Erase whole die within chip */
 #define SPINOR_OP_SE		0xd8	/* Sector erase (usually 64KiB) */
 #define SPINOR_OP_RDID		0x9f	/* Read JEDEC ID */
 #define SPINOR_OP_RDCR		0x35	/* Read configuration register */
@@ -119,6 +120,7 @@ enum spi_nor_ops {
 enum spi_nor_option_flags {
 	SNOR_F_USE_FSR		= BIT(0),
 	SNOR_F_HAS_SR_TB	= BIT(1),
+	SNOR_F_DIE_ERASE	= BIT(2),
 };
 
 /**
@@ -136,6 +138,8 @@ enum spi_nor_option_flags {
  * @sst_write_second:	used by the SST write operation
  * @flags:		flag options for the current SPI-NOR (SNOR_F_*)
  * @cmd_buf:		used by the write_reg
+ * @die_cnt:		number of dies in chip, if and SNOR_F_DIE_ERASE
+ * 			flasg is enabled CE command will be disabled
  * @prepare:		[OPTIONAL] do some preparations for the
  *			read/write/erase/lock/unlock operations
  * @unprepare:		[OPTIONAL] do some post work after the
@@ -167,6 +171,7 @@ struct spi_nor {
 	bool			sst_write_second;
 	u32			flags;
 	u8			cmd_buf[SPI_NOR_MAX_CMD_SIZE];
+	u32			die_cnt;
 
 	int (*prepare)(struct spi_nor *nor, enum spi_nor_ops ops);
 	void (*unprepare)(struct spi_nor *nor, enum spi_nor_ops ops);
-- 
2.7.4

[PATCH 2/3] mtd: spi-nor: Implement die erase command logic

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

This commit implements die erase logic.
Sector at a time procedure is moved to function,
then erase algorithm will try to use die erase cmd
if size and address cover one or more dies.

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 90 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 78 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 3afe207..17bbec0 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -360,6 +360,29 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
 	return nor->write_reg(nor, nor->erase_opcode, buf, nor->addr_width);
 }
 
+static inline int spi_nor_sector_at_time_erase(struct mtd_info *mtd, u32 addr, u32 len)
+{
+	struct spi_nor *nor = mtd_to_spi_nor(mtd);
+	int ret = 0;
+
+	while (len) {
+		write_enable(nor);
+
+		ret = spi_nor_erase_sector(nor, addr);
+		if (ret)
+			return ret;
+
+		addr += mtd->erasesize;
+		len -= mtd->erasesize;
+
+		ret = spi_nor_wait_till_ready(nor);
+		if (ret)
+			return ret;
+	}
+
+	return ret;
+}
+
 /*
  * Erase an address range on the nor chip.  The address range may extend
  * one or more erase sectors.  Return an error is there is a problem erasing.
@@ -367,9 +390,11 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
 static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 {
 	struct spi_nor *nor = mtd_to_spi_nor(mtd);
-	u32 addr, len;
+	u32 addr, len, die_no, die_size;
 	uint32_t rem;
 	int ret;
+	unsigned long timeout;
+	u8 die_erase = nor->die_cnt && SNOR_F_DIE_ERASE;
 
 	dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr->addr,
 			(long long)instr->len);
@@ -386,7 +411,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 		return ret;
 
 	/* whole-chip erase? */
-	if (len == mtd->size) {
+	if (len == mtd->size && !die_erase) {
 		unsigned long timeout;
 
 		write_enable(nor);
@@ -416,17 +441,58 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 
 	/* "sector"-at-a-time erase */
 	} else {
-		while (len) {
-			write_enable(nor);
-
-			ret = spi_nor_erase_sector(nor, addr);
-			if (ret)
+		if (die_erase) {
+			die_size = div_u64_rem(mtd->size, nor->die_cnt, &rem);
+			if (rem) {
+				ret = -EINVAL;
 				goto erase_err;
-
-			addr += mtd->erasesize;
-			len -= mtd->erasesize;
-
-			ret = spi_nor_wait_till_ready(nor);
+			}
+			while (len) {
+				die_no = div_u64_rem(addr, die_size, &rem);
+
+				/* Check if address is aligned to die begin*/
+				if (!rem) {
+					/* die erase? */
+					if (len >= die_size) {
+						ret = spi_nor_die_erase(nor, addr);
+						if (ret)
+							goto erase_err;
+
+						len -= die_size;
+						addr += die_size;
+
+						timeout = max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
+								CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
+								(unsigned long)(die_size / SZ_2M));
+						ret = spi_nor_wait_till_ready_with_timeout(nor, timeout);
+						if (ret)
+							goto erase_err;
+					} else {
+						ret = spi_nor_sector_at_time_erase(mtd, addr, len);
+						if (ret)
+							goto erase_err;
+						len = 0;
+					}
+				} else {
+					/* check if end address cover at least one die */
+					if (div64_ul(addr + len, die_size) > ++die_no) {
+						/* align to next die */
+						rem = die_size - rem;
+						ret = spi_nor_sector_at_time_erase(mtd, addr, rem);
+						if (ret)
+							goto erase_err;
+						len -= rem;
+						addr += rem;
+					} else {
+						ret = spi_nor_sector_at_time_erase(mtd, addr, len);
+						if (ret)
+							goto erase_err;
+						len = 0;
+					}
+				}
+			}
+		} else {
+			ret = spi_nor_sector_at_time_erase(mtd, addr, len);
 			if (ret)
 				goto erase_err;
 		}
-- 
2.7.4

[PATCH 3/3] mtd: spi-nor: Enable die erase for Micron 1GiB

[marcin.krzeminski]

From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Micron N25Q00 and MT25Q00 share same JEDEC Id,
but it seem can be properly recognized by second
ext_jedec id byte.

This commits extends n25q00 ids by adding ext
bytes and also adds mt25q00 family.
For MT25Q00 family, the number of dies is two, N25Q00
has it four. Logic to support that is added.

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 17bbec0..b33ead6 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -218,6 +218,28 @@ static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
 		return nor->write_reg(nor, SPINOR_OP_BRWR, nor->cmd_buf, 1);
 	}
 }
+
+static void spi_nor_die_cnt(struct spi_nor *nor,
+		const struct flash_info *info)
+{
+	switch (JEDEC_MFR(info)) {
+	case SNOR_MFR_MICRON:
+		/* 1GiB devices */
+		if (info->id[2] == 0x21) {
+			/* MT25Q00 has 2 dies N25Q00 has 4 */
+			if (info->id[4] & BIT(6))
+				nor->die_cnt = 2;
+			else
+				nor->die_cnt = 4;
+		} else
+			nor->die_cnt = 0;
+	break;
+	default:
+		nor->die_cnt = 0;
+		break;
+	}
+}
+
 static inline int spi_nor_sr_ready(struct spi_nor *nor)
 {
 	int sr = read_sr(nor);
@@ -970,8 +992,14 @@ static const struct flash_info spi_nor_ids[] = {
 	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K | SPI_NOR_QUAD_READ) },
 	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
 	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
-	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
-	{ "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
+	{ "n25q00",      INFO(0x20ba21, 0x1000, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
+	{ "n25q00a",     INFO(0x20bb21, 0x1000, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
+	{ "n25q00",      INFO(0x20ba21, 0x1004, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
+	{ "n25q00a",     INFO(0x20bb21, 0x1004, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
+	{ "mt25ql01g",   INFO(0x20ba21, 0x1044, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
+	{ "mt25qu01g",   INFO(0x20bb21, 0x1044, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
+	{ "mt25ql01g",   INFO(0x20ba21, 0x1040, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
+	{ "mt25qu01g",   INFO(0x20bb21, 0x1040, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
 
 	/* PMC */
 	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
@@ -1479,6 +1507,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 		nor->flags |= SNOR_F_USE_FSR;
 	if (info->flags & SPI_NOR_HAS_TB)
 		nor->flags |= SNOR_F_HAS_SR_TB;
+	if (info->flags & DIE_ERASE) {
+		nor->flags |= SNOR_F_DIE_ERASE;
+		spi_nor_die_cnt(nor, info);
+	}
 
 #ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
 	/* prefer "small sector" erase if possible */
-- 
2.7.4

Re: [PATCH 1/3] mtd: spi-nor: Add die_cnt field and flags

[Cyrille Pitchen]

Hi Marcin,

Le 24/10/2016 à 15:03, marcin.krzeminski@nokia.com a écrit :
> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> 
> This commit adds new field and flags that could
> be used to signalize that chip support die erase
> command.
> 
> If DIE_ERASE flag will be selected but die_cnt field
> will be 0 chip will not use chip erase command.
> 
> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 18 ++++++++++++++++++
>  include/linux/mtd/spi-nor.h   |  5 +++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index d0fc165..3afe207 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -75,6 +75,7 @@ struct flash_info {
>  					 * bit. Must be used with
>  					 * SPI_NOR_HAS_LOCK.
>  					 */
> +#define DIE_ERASE			BIT(10)	/* Support for die erase cmd */
>  };
>  
>  #define JEDEC_MFR(info)	((info)->id[0])
> @@ -295,6 +296,23 @@ static int erase_chip(struct spi_nor *nor)
>  	return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0);
>  }
>  
> +static int spi_nor_die_erase(struct spi_nor *nor, u32 addr)
> +{
> +	u8 buf[SPI_NOR_MAX_ADDR_WIDTH];
> +	int i;
> +
> +	dev_dbg(nor->dev, "erase @ 0x%X\n", addr);
> +
> +	write_enable(nor);
> +
> +	for (i = nor->addr_width - 1; i >= 0; i--) {
> +		buf[i] = addr & 0xff;
> +		addr >>= 8;
> +	}
> +
> +	return nor->write_reg(nor, SPINOR_OP_DIE_ERASE, buf, nor->addr_width);

If think you should first check whether nor->erase() is implemented by the SPI
controller driver and call it instead of nor->write_reg().

For instance, some SPI controllers are mapped to the system memory to speed
up Fast Read operations. In which case, they could also enable data cache to
fasten even more the read operations.

So when the spi-nor framework calls nor->erase() or nor->write(), those
handlers might need to invalidate the cache before performing the next read.

This is just an example but generally speaking, don't forget the nor->erase()
handler! :)


> +}
> +
>  static int spi_nor_lock_and_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>  {
>  	int ret = 0;
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index c425c7b..80154b2 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -50,6 +50,7 @@
>  #define SPINOR_OP_BE_4K_PMC	0xd7	/* Erase 4KiB block on PMC chips */
>  #define SPINOR_OP_BE_32K	0x52	/* Erase 32KiB block */
>  #define SPINOR_OP_CHIP_ERASE	0xc7	/* Erase whole flash chip */
> +#define SPINOR_OP_DIE_ERASE	0xc4	/* Erase whole die within chip */
>  #define SPINOR_OP_SE		0xd8	/* Sector erase (usually 64KiB) */
>  #define SPINOR_OP_RDID		0x9f	/* Read JEDEC ID */
>  #define SPINOR_OP_RDCR		0x35	/* Read configuration register */
> @@ -119,6 +120,7 @@ enum spi_nor_ops {
>  enum spi_nor_option_flags {
>  	SNOR_F_USE_FSR		= BIT(0),
>  	SNOR_F_HAS_SR_TB	= BIT(1),
> +	SNOR_F_DIE_ERASE	= BIT(2),
>  };
>  
>  /**
> @@ -136,6 +138,8 @@ enum spi_nor_option_flags {
>   * @sst_write_second:	used by the SST write operation
>   * @flags:		flag options for the current SPI-NOR (SNOR_F_*)
>   * @cmd_buf:		used by the write_reg
> + * @die_cnt:		number of dies in chip, if and SNOR_F_DIE_ERASE

The 1st flag is missing between "if" and "and", isn't it?
> + * 			flasg is enabled CE command will be disabled

typo: s/flasg/flags/
>   * @prepare:		[OPTIONAL] do some preparations for the
>   *			read/write/erase/lock/unlock operations
>   * @unprepare:		[OPTIONAL] do some post work after the
> @@ -167,6 +171,7 @@ struct spi_nor {
>  	bool			sst_write_second;
>  	u32			flags;
>  	u8			cmd_buf[SPI_NOR_MAX_CMD_SIZE];
> +	u32			die_cnt;
>  
>  	int (*prepare)(struct spi_nor *nor, enum spi_nor_ops ops);
>  	void (*unprepare)(struct spi_nor *nor, enum spi_nor_ops ops);
> 

Best regards,

Cyrille

RE: [PATCH 1/3] mtd: spi-nor: Add die_cnt field and flags

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

Hi Cyrille,

> -----Original Message-----
> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> Sent: Monday, October 24, 2016 3:55 PM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> computersforpeace@gmail.com; marek.vasut@gmail.com
> Subject: Re: [PATCH 1/3] mtd: spi-nor: Add die_cnt field and flags
> 
> Hi Marcin,
> 
> Le 24/10/2016 à 15:03, marcin.krzeminski@nokia.com a écrit :
> > From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> >
> > This commit adds new field and flags that could be used to signalize
> > that chip support die erase command.
> >
> > If DIE_ERASE flag will be selected but die_cnt field will be 0 chip
> > will not use chip erase command.
> >
> > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 18 ++++++++++++++++++
> >  include/linux/mtd/spi-nor.h   |  5 +++++
> >  2 files changed, 23 insertions(+)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > b/drivers/mtd/spi-nor/spi-nor.c index d0fc165..3afe207 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -75,6 +75,7 @@ struct flash_info {
> >  					 * bit. Must be used with
> >  					 * SPI_NOR_HAS_LOCK.
> >  					 */
> > +#define DIE_ERASE			BIT(10)	/* Support for die erase cmd
> */
> >  };
> >
> >  #define JEDEC_MFR(info)	((info)->id[0])
> > @@ -295,6 +296,23 @@ static int erase_chip(struct spi_nor *nor)
> >  	return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0);  }
> >
> > +static int spi_nor_die_erase(struct spi_nor *nor, u32 addr) {
> > +	u8 buf[SPI_NOR_MAX_ADDR_WIDTH];
> > +	int i;
> > +
> > +	dev_dbg(nor->dev, "erase @ 0x%X\n", addr);
> > +
> > +	write_enable(nor);
> > +
> > +	for (i = nor->addr_width - 1; i >= 0; i--) {
> > +		buf[i] = addr & 0xff;
> > +		addr >>= 8;
> > +	}
> > +
> > +	return nor->write_reg(nor, SPINOR_OP_DIE_ERASE, buf,
> > +nor->addr_width);
> 
> If think you should first check whether nor->erase() is implemented by the
> SPI controller driver and call it instead of nor->write_reg().
> 
> For instance, some SPI controllers are mapped to the system memory to
> speed up Fast Read operations. In which case, they could also enable data
> cache to fasten even more the read operations.
> 
> So when the spi-nor framework calls nor->erase() or nor->write(), those
> handlers might need to invalidate the cache before performing the next
> read.
> 
> This is just an example but generally speaking, don't forget the nor->erase()
> handler! :)
> 
Thanks for extensive clarification, I will include this in v2.

Regards,
Marcin

> 
> > +}
> > +
> >  static int spi_nor_lock_and_prep(struct spi_nor *nor, enum
> > spi_nor_ops ops)  {
> >  	int ret = 0;
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> > index c425c7b..80154b2 100644
> > --- a/include/linux/mtd/spi-nor.h
> > +++ b/include/linux/mtd/spi-nor.h
> > @@ -50,6 +50,7 @@
> >  #define SPINOR_OP_BE_4K_PMC	0xd7	/* Erase 4KiB block on PMC
> chips */
> >  #define SPINOR_OP_BE_32K	0x52	/* Erase 32KiB block */
> >  #define SPINOR_OP_CHIP_ERASE	0xc7	/* Erase whole flash chip */
> > +#define SPINOR_OP_DIE_ERASE	0xc4	/* Erase whole die within chip
> */
> >  #define SPINOR_OP_SE		0xd8	/* Sector erase (usually
> 64KiB) */
> >  #define SPINOR_OP_RDID		0x9f	/* Read JEDEC ID */
> >  #define SPINOR_OP_RDCR		0x35	/* Read configuration register
> */
> > @@ -119,6 +120,7 @@ enum spi_nor_ops {  enum spi_nor_option_flags {
> >  	SNOR_F_USE_FSR		= BIT(0),
> >  	SNOR_F_HAS_SR_TB	= BIT(1),
> > +	SNOR_F_DIE_ERASE	= BIT(2),
> >  };
> >
> >  /**
> > @@ -136,6 +138,8 @@ enum spi_nor_option_flags {
> >   * @sst_write_second:	used by the SST write operation
> >   * @flags:		flag options for the current SPI-NOR (SNOR_F_*)
> >   * @cmd_buf:		used by the write_reg
> > + * @die_cnt:		number of dies in chip, if and
> SNOR_F_DIE_ERASE
> 
> The 1st flag is missing between "if" and "and", isn't it?
> > + * 			flasg is enabled CE command will be disabled
> 
> typo: s/flasg/flags/
> >   * @prepare:		[OPTIONAL] do some preparations for the
> >   *			read/write/erase/lock/unlock operations
> >   * @unprepare:		[OPTIONAL] do some post work after the
> > @@ -167,6 +171,7 @@ struct spi_nor {
> >  	bool			sst_write_second;
> >  	u32			flags;
> >  	u8			cmd_buf[SPI_NOR_MAX_CMD_SIZE];
> > +	u32			die_cnt;
> >
> >  	int (*prepare)(struct spi_nor *nor, enum spi_nor_ops ops);
> >  	void (*unprepare)(struct spi_nor *nor, enum spi_nor_ops ops);
> >
> 
> Best regards,
> 
> Cyrille

RE: [PATCH 1/3] mtd: spi-nor: Add die_cnt field and flags

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

Cyrille,

....
> >
> > If think you should first check whether nor->erase() is implemented by
> > the SPI controller driver and call it instead of nor->write_reg().
> >
> > For instance, some SPI controllers are mapped to the system memory to
> > speed up Fast Read operations. In which case, they could also enable
> > data cache to fasten even more the read operations.
> >
> > So when the spi-nor framework calls nor->erase() or nor->write(),
> > those handlers might need to invalidate the cache before performing
> > the next read.
> >
> > This is just an example but generally speaking, don't forget the
> > nor->erase() handler! :)
> >
> Thanks for extensive clarification, I will include this in v2.
> 
> Regards,
> Marcin
> 
I've just sent v2 with added check for nor->erase() handler.
As we talked on IRC, with the example you gave above there is
a bug in case of chip erase command, because the driver will not
know that whole flash has been erased.
Maybe there is a need to have a handler in spi-nor framework
for memory invalidation?

Thanks,
Marcin

Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic

[Cyrille Pitchen]

Hi Marcin,

I know you have already answered some of my questions of IRC but I ask
them again here on the mailing list so everybody can benefit from your
answers.

Le 24/10/2016 à 15:03, marcin.krzeminski@nokia.com a écrit :
> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> 
> This commit implements die erase logic.
> Sector at a time procedure is moved to function,
> then erase algorithm will try to use die erase cmd
> if size and address cover one or more dies.
> 

I'm reading your v2 series but indeed I try to understand the purpose of the
series simply because I don't know what does die erase do as compared to chip
erase of sector erase.
Is you series a bug fix or an optmization, maybe both?

Best regards,

Cyrille

> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 90 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 78 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 3afe207..17bbec0 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -360,6 +360,29 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>  	return nor->write_reg(nor, nor->erase_opcode, buf, nor->addr_width);
>  }
>  
> +static inline int spi_nor_sector_at_time_erase(struct mtd_info *mtd, u32 addr, u32 len)
> +{
> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> +	int ret = 0;
> +
> +	while (len) {
> +		write_enable(nor);
> +
> +		ret = spi_nor_erase_sector(nor, addr);
> +		if (ret)
> +			return ret;
> +
> +		addr += mtd->erasesize;
> +		len -= mtd->erasesize;
> +
> +		ret = spi_nor_wait_till_ready(nor);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return ret;
> +}
> +
>  /*
>   * Erase an address range on the nor chip.  The address range may extend
>   * one or more erase sectors.  Return an error is there is a problem erasing.
> @@ -367,9 +390,11 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>  static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
>  {
>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> -	u32 addr, len;
> +	u32 addr, len, die_no, die_size;
>  	uint32_t rem;
>  	int ret;
> +	unsigned long timeout;
> +	u8 die_erase = nor->die_cnt && SNOR_F_DIE_ERASE;
>  
>  	dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr->addr,
>  			(long long)instr->len);
> @@ -386,7 +411,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
>  		return ret;
>  
>  	/* whole-chip erase? */
> -	if (len == mtd->size) {
> +	if (len == mtd->size && !die_erase) {
>  		unsigned long timeout;
>  
>  		write_enable(nor);
> @@ -416,17 +441,58 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
>  
>  	/* "sector"-at-a-time erase */
>  	} else {
> -		while (len) {
> -			write_enable(nor);
> -
> -			ret = spi_nor_erase_sector(nor, addr);
> -			if (ret)
> +		if (die_erase) {
> +			die_size = div_u64_rem(mtd->size, nor->die_cnt, &rem);
> +			if (rem) {
> +				ret = -EINVAL;
>  				goto erase_err;
> -
> -			addr += mtd->erasesize;
> -			len -= mtd->erasesize;
> -
> -			ret = spi_nor_wait_till_ready(nor);
> +			}
> +			while (len) {
> +				die_no = div_u64_rem(addr, die_size, &rem);
> +
> +				/* Check if address is aligned to die begin*/
> +				if (!rem) {
> +					/* die erase? */
> +					if (len >= die_size) {
> +						ret = spi_nor_die_erase(nor, addr);
> +						if (ret)
> +							goto erase_err;
> +
> +						len -= die_size;
> +						addr += die_size;
> +
> +						timeout = max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
> +								CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
> +								(unsigned long)(die_size / SZ_2M));
> +						ret = spi_nor_wait_till_ready_with_timeout(nor, timeout);
> +						if (ret)
> +							goto erase_err;
> +					} else {
> +						ret = spi_nor_sector_at_time_erase(mtd, addr, len);
> +						if (ret)
> +							goto erase_err;
> +						len = 0;
> +					}
> +				} else {
> +					/* check if end address cover at least one die */
> +					if (div64_ul(addr + len, die_size) > ++die_no) {
> +						/* align to next die */
> +						rem = die_size - rem;
> +						ret = spi_nor_sector_at_time_erase(mtd, addr, rem);
> +						if (ret)
> +							goto erase_err;
> +						len -= rem;
> +						addr += rem;
> +					} else {
> +						ret = spi_nor_sector_at_time_erase(mtd, addr, len);
> +						if (ret)
> +							goto erase_err;
> +						len = 0;
> +					}
> +				}
> +			}
> +		} else {
> +			ret = spi_nor_sector_at_time_erase(mtd, addr, len);
>  			if (ret)
>  				goto erase_err;
>  		}
> 

Re: [PATCH 3/3] mtd: spi-nor: Enable die erase for Micron 1GiB

[Cyrille Pitchen]

Hi Marcin,

Le 24/10/2016 à 15:03, marcin.krzeminski@nokia.com a écrit :
> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> 
> Micron N25Q00 and MT25Q00 share same JEDEC Id,
> but it seem can be properly recognized by second
> ext_jedec id byte.
> 
> This commits extends n25q00 ids by adding ext
> bytes and also adds mt25q00 family.
> For MT25Q00 family, the number of dies is two, N25Q00
> has it four. Logic to support that is added.
> 
> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 36 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 17bbec0..b33ead6 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -218,6 +218,28 @@ static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>  		return nor->write_reg(nor, SPINOR_OP_BRWR, nor->cmd_buf, 1);
>  	}
>  }
> +
> +static void spi_nor_die_cnt(struct spi_nor *nor,
> +		const struct flash_info *info)
> +{
> +	switch (JEDEC_MFR(info)) {
> +	case SNOR_MFR_MICRON:
> +		/* 1GiB devices */
> +		if (info->id[2] == 0x21) {
> +			/* MT25Q00 has 2 dies N25Q00 has 4 */
> +			if (info->id[4] & BIT(6))
> +				nor->die_cnt = 2;
> +			else
> +				nor->die_cnt = 4;
> +		} else
> +			nor->die_cnt = 0;

You set nor->die_cnt according to the JEDED ID but what is BIT(6) in the 5th
byte of the JEDEC ID?
How do you know whether this "rule of thumb" also applies to other Micron
memories?

I think we should find another solution to set nor->die_cnt properly.
Maybe adding some info in the relevant entries of the spi_nor_ids[] array?

Best regards,

Cyrille

> +	break;
> +	default:
> +		nor->die_cnt = 0;
> +		break;
> +	}
> +}
> +
>  static inline int spi_nor_sr_ready(struct spi_nor *nor)
>  {
>  	int sr = read_sr(nor);
> @@ -970,8 +992,14 @@ static const struct flash_info spi_nor_ids[] = {
>  	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K | SPI_NOR_QUAD_READ) },
>  	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
>  	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
> -	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
> -	{ "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
> +	{ "n25q00",      INFO(0x20ba21, 0x1000, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
> +	{ "n25q00a",     INFO(0x20bb21, 0x1000, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
> +	{ "n25q00",      INFO(0x20ba21, 0x1004, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
> +	{ "n25q00a",     INFO(0x20bb21, 0x1004, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
> +	{ "mt25ql01g",   INFO(0x20ba21, 0x1044, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
> +	{ "mt25qu01g",   INFO(0x20bb21, 0x1044, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
> +	{ "mt25ql01g",   INFO(0x20ba21, 0x1040, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
> +	{ "mt25qu01g",   INFO(0x20bb21, 0x1040, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
>  
>  	/* PMC */
>  	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
> @@ -1479,6 +1507,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  		nor->flags |= SNOR_F_USE_FSR;
>  	if (info->flags & SPI_NOR_HAS_TB)
>  		nor->flags |= SNOR_F_HAS_SR_TB;
> +	if (info->flags & DIE_ERASE) {
> +		nor->flags |= SNOR_F_DIE_ERASE;
> +		spi_nor_die_cnt(nor, info);
> +	}
>  
>  #ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
>  	/* prefer "small sector" erase if possible */
> 

RE: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

> -----Original Message-----
> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> Sent: Tuesday, November 29, 2016 5:47 PM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> computersforpeace@gmail.com; marek.vasut@gmail.com
> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic
> 
> Hi Marcin,
> 
> I know you have already answered some of my questions of IRC but I ask
> them again here on the mailing list so everybody can benefit from your
> answers.
> 
> Le 24/10/2016 à 15:03, marcin.krzeminski@nokia.com a écrit :
> > From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> >
> > This commit implements die erase logic.
> > Sector at a time procedure is moved to function, then erase algorithm
> > will try to use die erase cmd if size and address cover one or more
> > dies.
> >
> 
> I'm reading your v2 series but indeed I try to understand the purpose of the
> series simply because I don't know what does die erase do as compared to
> chip erase of sector erase.
> Is you series a bug fix or an optmization, maybe both?

In current spi-nor framework when user want to erase whole flash
(eg. mtd_debug erase /dev/mtdX 0 0x8000000) framework issue chip erase cmd.
N25Q00 does not support chip erase, instead there was introduced
die erase command that can erase whole die (N25Q00 is a mulit-die NOR chip).
So for this case this patch is a bug fix.
Since I have die erase command implemented it was tempting to use die erase
also in case where erase area cover one or more dies to speed up erase process,
especially when user has enabled 4KiB sector. That is a small optimization.

I will rewrite commit message and add those informations.

Thanks,
Marcin

> 
> Best regards,
> 
> Cyrille
> 
> > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 90
> > +++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 78 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > b/drivers/mtd/spi-nor/spi-nor.c index 3afe207..17bbec0 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -360,6 +360,29 @@ static int spi_nor_erase_sector(struct spi_nor
> *nor, u32 addr)
> >  	return nor->write_reg(nor, nor->erase_opcode, buf, nor-
> >addr_width);
> > }
> >
> > +static inline int spi_nor_sector_at_time_erase(struct mtd_info *mtd,
> > +u32 addr, u32 len) {
> > +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> > +	int ret = 0;
> > +
> > +	while (len) {
> > +		write_enable(nor);
> > +
> > +		ret = spi_nor_erase_sector(nor, addr);
> > +		if (ret)
> > +			return ret;
> > +
> > +		addr += mtd->erasesize;
> > +		len -= mtd->erasesize;
> > +
> > +		ret = spi_nor_wait_till_ready(nor);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  /*
> >   * Erase an address range on the nor chip.  The address range may extend
> >   * one or more erase sectors.  Return an error is there is a problem erasing.
> > @@ -367,9 +390,11 @@ static int spi_nor_erase_sector(struct spi_nor
> > *nor, u32 addr)  static int spi_nor_erase(struct mtd_info *mtd, struct
> > erase_info *instr)  {
> >  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> > -	u32 addr, len;
> > +	u32 addr, len, die_no, die_size;
> >  	uint32_t rem;
> >  	int ret;
> > +	unsigned long timeout;
> > +	u8 die_erase = nor->die_cnt && SNOR_F_DIE_ERASE;
> >
> >  	dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr->addr,
> >  			(long long)instr->len);
> > @@ -386,7 +411,7 @@ static int spi_nor_erase(struct mtd_info *mtd,
> struct erase_info *instr)
> >  		return ret;
> >
> >  	/* whole-chip erase? */
> > -	if (len == mtd->size) {
> > +	if (len == mtd->size && !die_erase) {
> >  		unsigned long timeout;
> >
> >  		write_enable(nor);
> > @@ -416,17 +441,58 @@ static int spi_nor_erase(struct mtd_info *mtd,
> > struct erase_info *instr)
> >
> >  	/* "sector"-at-a-time erase */
> >  	} else {
> > -		while (len) {
> > -			write_enable(nor);
> > -
> > -			ret = spi_nor_erase_sector(nor, addr);
> > -			if (ret)
> > +		if (die_erase) {
> > +			die_size = div_u64_rem(mtd->size, nor->die_cnt,
> &rem);
> > +			if (rem) {
> > +				ret = -EINVAL;
> >  				goto erase_err;
> > -
> > -			addr += mtd->erasesize;
> > -			len -= mtd->erasesize;
> > -
> > -			ret = spi_nor_wait_till_ready(nor);
> > +			}
> > +			while (len) {
> > +				die_no = div_u64_rem(addr, die_size,
> &rem);
> > +
> > +				/* Check if address is aligned to die begin*/
> > +				if (!rem) {
> > +					/* die erase? */
> > +					if (len >= die_size) {
> > +						ret = spi_nor_die_erase(nor,
> addr);
> > +						if (ret)
> > +							goto erase_err;
> > +
> > +						len -= die_size;
> > +						addr += die_size;
> > +
> > +						timeout =
> max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
> > +
> 	CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
> > +								(unsigned
> long)(die_size / SZ_2M));
> > +						ret =
> spi_nor_wait_till_ready_with_timeout(nor, timeout);
> > +						if (ret)
> > +							goto erase_err;
> > +					} else {
> > +						ret =
> spi_nor_sector_at_time_erase(mtd, addr, len);
> > +						if (ret)
> > +							goto erase_err;
> > +						len = 0;
> > +					}
> > +				} else {
> > +					/* check if end address cover at least
> one die */
> > +					if (div64_ul(addr + len, die_size) >
> ++die_no) {
> > +						/* align to next die */
> > +						rem = die_size - rem;
> > +						ret =
> spi_nor_sector_at_time_erase(mtd, addr, rem);
> > +						if (ret)
> > +							goto erase_err;
> > +						len -= rem;
> > +						addr += rem;
> > +					} else {
> > +						ret =
> spi_nor_sector_at_time_erase(mtd, addr, len);
> > +						if (ret)
> > +							goto erase_err;
> > +						len = 0;
> > +					}
> > +				}
> > +			}
> > +		} else {
> > +			ret = spi_nor_sector_at_time_erase(mtd, addr, len);
> >  			if (ret)
> >  				goto erase_err;
> >  		}
> >

RE: [PATCH 3/3] mtd: spi-nor: Enable die erase for Micron 1GiB

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

> -----Original Message-----
> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> Sent: Tuesday, November 29, 2016 5:54 PM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> computersforpeace@gmail.com; marek.vasut@gmail.com
> Subject: Re: [PATCH 3/3] mtd: spi-nor: Enable die erase for Micron 1GiB
> 
> Hi Marcin,
> 
> Le 24/10/2016 à 15:03, marcin.krzeminski@nokia.com a écrit :
> > From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> >
> > Micron N25Q00 and MT25Q00 share same JEDEC Id, but it seem can be
> > properly recognized by second ext_jedec id byte.
> >
> > This commits extends n25q00 ids by adding ext bytes and also adds
> > mt25q00 family.
> > For MT25Q00 family, the number of dies is two, N25Q00 has it four.
> > Logic to support that is added.
> >
> > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> > ---
> >  drivers/mtd/spi-nor/spi-nor.c | 36
> > ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > b/drivers/mtd/spi-nor/spi-nor.c index 17bbec0..b33ead6 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -218,6 +218,28 @@ static inline int set_4byte(struct spi_nor *nor,
> const struct flash_info *info,
> >  		return nor->write_reg(nor, SPINOR_OP_BRWR, nor-
> >cmd_buf, 1);
> >  	}
> >  }
> > +
> > +static void spi_nor_die_cnt(struct spi_nor *nor,
> > +		const struct flash_info *info)
> > +{
> > +	switch (JEDEC_MFR(info)) {
> > +	case SNOR_MFR_MICRON:
> > +		/* 1GiB devices */
> > +		if (info->id[2] == 0x21) {
> > +			/* MT25Q00 has 2 dies N25Q00 has 4 */
> > +			if (info->id[4] & BIT(6))
> > +				nor->die_cnt = 2;
> > +			else
> > +				nor->die_cnt = 4;
> > +		} else
> > +			nor->die_cnt = 0;
> 
> You set nor->die_cnt according to the JEDED ID but what is BIT(6) in the 5th
> byte of the JEDEC ID?
Unfortunately 1Gb MT25Q and N25Q00 has the same JEDEDC ID.
Datasheet for MT25Q specified this bit as revision, N25Q00 as reserved but set to 0,
so my guess was that it can be used to distinguish them.
> How do you know whether this "rule of thumb" also applies to other Micron
> memories?
No idea, at least it also should work for 2Gb chips.
> 
> I think we should find another solution to set nor->die_cnt properly.
> Maybe adding some info in the relevant entries of the spi_nor_ids[] array?
I wanted to have this solve in "automatic" way by updating this function if
there will be a need, but you are right - more generic way is better.
I'll take a look how it could be done using spi_nor_ids.
Maybe there are some other idea how to make this  work?
Unfortunately JEDEC216B will not help in this case.

Thanks,
Marcin

> 
> Best regards,
> 
> Cyrille
> 
> > +	break;
> > +	default:
> > +		nor->die_cnt = 0;
> > +		break;
> > +	}
> > +}
> > +
> >  static inline int spi_nor_sr_ready(struct spi_nor *nor)  {
> >  	int sr = read_sr(nor);
> > @@ -970,8 +992,14 @@ static const struct flash_info spi_nor_ids[] = {
> >  	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K |
> SPI_NOR_QUAD_READ) },
> >  	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K |
> USE_FSR | SPI_NOR_QUAD_READ) },
> >  	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K |
> USE_FSR | SPI_NOR_QUAD_READ) },
> > -	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR
> | SPI_NOR_QUAD_READ) },
> > -	{ "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR
> | SPI_NOR_QUAD_READ) },
> > +	{ "n25q00",      INFO(0x20ba21, 0x1000, 64 * 1024, 2048, SECT_4K |
> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
> > +	{ "n25q00a",     INFO(0x20bb21, 0x1000, 64 * 1024, 2048, SECT_4K |
> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
> > +	{ "n25q00",      INFO(0x20ba21, 0x1004, 64 * 1024, 2048, SECT_4K |
> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
> > +	{ "n25q00a",     INFO(0x20bb21, 0x1004, 64 * 1024, 2048, SECT_4K |
> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
> > +	{ "mt25ql01g",   INFO(0x20ba21, 0x1044, 64 * 1024, 2048, SECT_4K |
> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
> > +	{ "mt25qu01g",   INFO(0x20bb21, 0x1044, 64 * 1024, 2048, SECT_4K |
> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
> > +	{ "mt25ql01g",   INFO(0x20ba21, 0x1040, 64 * 1024, 2048, SECT_4K |
> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
> > +	{ "mt25qu01g",   INFO(0x20bb21, 0x1040, 64 * 1024, 2048, SECT_4K |
> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
> >
> >  	/* PMC */
> >  	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
> > @@ -1479,6 +1507,10 @@ int spi_nor_scan(struct spi_nor *nor, const char
> *name, enum read_mode mode)
> >  		nor->flags |= SNOR_F_USE_FSR;
> >  	if (info->flags & SPI_NOR_HAS_TB)
> >  		nor->flags |= SNOR_F_HAS_SR_TB;
> > +	if (info->flags & DIE_ERASE) {
> > +		nor->flags |= SNOR_F_DIE_ERASE;
> > +		spi_nor_die_cnt(nor, info);
> > +	}
> >
> >  #ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
> >  	/* prefer "small sector" erase if possible */
> >

Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic

[Cyrille Pitchen]

Le 30/11/2016 à 17:44, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> 
> 
>> -----Original Message-----
>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
>> Sent: Tuesday, November 29, 2016 5:47 PM
>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
>> computersforpeace@gmail.com; marek.vasut@gmail.com
>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic
>>
>> Hi Marcin,
>>
>> I know you have already answered some of my questions of IRC but I ask
>> them again here on the mailing list so everybody can benefit from your
>> answers.
>>
>> Le 24/10/2016 à 15:03, marcin.krzeminski@nokia.com a écrit :
>>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>>>
>>> This commit implements die erase logic.
>>> Sector at a time procedure is moved to function, then erase algorithm
>>> will try to use die erase cmd if size and address cover one or more
>>> dies.
>>>
>>
>> I'm reading your v2 series but indeed I try to understand the purpose of the
>> series simply because I don't know what does die erase do as compared to
>> chip erase of sector erase.
>> Is you series a bug fix or an optmization, maybe both?
> 
> In current spi-nor framework when user want to erase whole flash
> (eg. mtd_debug erase /dev/mtdX 0 0x8000000) framework issue chip erase cmd.
> N25Q00 does not support chip erase, instead there was introduced
> die erase command that can erase whole die (N25Q00 is a mulit-die NOR chip).
> So for this case this patch is a bug fix.
> Since I have die erase command implemented it was tempting to use die erase
> also in case where erase area cover one or more dies to speed up erase process,
> especially when user has enabled 4KiB sector. That is a small optimization.
> 
> I will rewrite commit message and add those informations.
> 
> Thanks,
> Marcin
>

OK, then I think your patch should be split into two different patches.

1 - bug fix:

The first patch introducing an explicit flag such as SPI_NOR_NO_CHIP_ERASE,
which would be set in the relevant entries of the spi_nor_ids[] (see already
existing flags such as SPI_NOR_NO_FR, SPI_NOR_NO_ERASE, ...).

In spi_nor_scan():

 	if (info->flags & USE_FSR)
 		nor->flags |= SNOR_F_USE_FSR;
 	if (info->flags & SPI_NOR_HAS_TB)
 		nor->flags |= SNOR_F_HAS_SR_TB;
+	if (info->flags & SPI_NOR_NO_CHIP_ERASE)
+		nor->flags |= SNOR_F_NO_CHIP_ERASE;

Then in spi_nor_erase():

 	/* whole-chip erase? */
-	if (len == mtd->size) {
+	if (len == mtd->size && !(nor->flags & SNOR_F_NO_CHIP_ERASE)) {
 		unsigned long timeout;

So you fall into the sector erase case, no optimization yet.

2 - optimization of spi_nor_erase()

In the "sector"-at-a-time erase branch, try to optimize when possible using
die erase instead of sector erase.

Each commit message describing the purpose of the patch.

I would like to also handle the cases where the memory supports:
- both chip and die erases: if so, no reason not to use chip erase on such
  multi-die memory.
- neither chip nor die erases.

Maybe I'm wrong but the assumption (multi-die => chip erase not supported)
might be false.


Best regards,

Cyrille

 
>>
>> Best regards,
>>
>> Cyrille
>>
>>> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>>> ---
>>>  drivers/mtd/spi-nor/spi-nor.c | 90
>>> +++++++++++++++++++++++++++++++++++++------
>>>  1 file changed, 78 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c
>>> b/drivers/mtd/spi-nor/spi-nor.c index 3afe207..17bbec0 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -360,6 +360,29 @@ static int spi_nor_erase_sector(struct spi_nor
>> *nor, u32 addr)
>>>  	return nor->write_reg(nor, nor->erase_opcode, buf, nor-
>>> addr_width);
>>> }
>>>
>>> +static inline int spi_nor_sector_at_time_erase(struct mtd_info *mtd,
>>> +u32 addr, u32 len) {
>>> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>>> +	int ret = 0;
>>> +
>>> +	while (len) {
>>> +		write_enable(nor);
>>> +
>>> +		ret = spi_nor_erase_sector(nor, addr);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		addr += mtd->erasesize;
>>> +		len -= mtd->erasesize;
>>> +
>>> +		ret = spi_nor_wait_till_ready(nor);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>  /*
>>>   * Erase an address range on the nor chip.  The address range may extend
>>>   * one or more erase sectors.  Return an error is there is a problem erasing.
>>> @@ -367,9 +390,11 @@ static int spi_nor_erase_sector(struct spi_nor
>>> *nor, u32 addr)  static int spi_nor_erase(struct mtd_info *mtd, struct
>>> erase_info *instr)  {
>>>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>>> -	u32 addr, len;
>>> +	u32 addr, len, die_no, die_size;
>>>  	uint32_t rem;
>>>  	int ret;
>>> +	unsigned long timeout;
>>> +	u8 die_erase = nor->die_cnt && SNOR_F_DIE_ERASE;
>>>
>>>  	dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr->addr,
>>>  			(long long)instr->len);
>>> @@ -386,7 +411,7 @@ static int spi_nor_erase(struct mtd_info *mtd,
>> struct erase_info *instr)
>>>  		return ret;
>>>
>>>  	/* whole-chip erase? */
>>> -	if (len == mtd->size) {
>>> +	if (len == mtd->size && !die_erase) {
>>>  		unsigned long timeout;
>>>
>>>  		write_enable(nor);
>>> @@ -416,17 +441,58 @@ static int spi_nor_erase(struct mtd_info *mtd,
>>> struct erase_info *instr)
>>>
>>>  	/* "sector"-at-a-time erase */
>>>  	} else {
>>> -		while (len) {
>>> -			write_enable(nor);
>>> -
>>> -			ret = spi_nor_erase_sector(nor, addr);
>>> -			if (ret)
>>> +		if (die_erase) {
>>> +			die_size = div_u64_rem(mtd->size, nor->die_cnt,
>> &rem);
>>> +			if (rem) {
>>> +				ret = -EINVAL;
>>>  				goto erase_err;
>>> -
>>> -			addr += mtd->erasesize;
>>> -			len -= mtd->erasesize;
>>> -
>>> -			ret = spi_nor_wait_till_ready(nor);
>>> +			}
>>> +			while (len) {
>>> +				die_no = div_u64_rem(addr, die_size,
>> &rem);
>>> +
>>> +				/* Check if address is aligned to die begin*/
>>> +				if (!rem) {
>>> +					/* die erase? */
>>> +					if (len >= die_size) {
>>> +						ret = spi_nor_die_erase(nor,
>> addr);
>>> +						if (ret)
>>> +							goto erase_err;
>>> +
>>> +						len -= die_size;
>>> +						addr += die_size;
>>> +
>>> +						timeout =
>> max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
>>> +
>> 	CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
>>> +								(unsigned
>> long)(die_size / SZ_2M));
>>> +						ret =
>> spi_nor_wait_till_ready_with_timeout(nor, timeout);
>>> +						if (ret)
>>> +							goto erase_err;
>>> +					} else {
>>> +						ret =
>> spi_nor_sector_at_time_erase(mtd, addr, len);
>>> +						if (ret)
>>> +							goto erase_err;
>>> +						len = 0;
>>> +					}
>>> +				} else {
>>> +					/* check if end address cover at least
>> one die */
>>> +					if (div64_ul(addr + len, die_size) >
>> ++die_no) {
>>> +						/* align to next die */
>>> +						rem = die_size - rem;
>>> +						ret =
>> spi_nor_sector_at_time_erase(mtd, addr, rem);
>>> +						if (ret)
>>> +							goto erase_err;
>>> +						len -= rem;
>>> +						addr += rem;
>>> +					} else {
>>> +						ret =
>> spi_nor_sector_at_time_erase(mtd, addr, len);
>>> +						if (ret)
>>> +							goto erase_err;
>>> +						len = 0;
>>> +					}
>>> +				}
>>> +			}
>>> +		} else {
>>> +			ret = spi_nor_sector_at_time_erase(mtd, addr, len);
>>>  			if (ret)
>>>  				goto erase_err;
>>>  		}
>>>
> 
> 

Re: [PATCH 3/3] mtd: spi-nor: Enable die erase for Micron 1GiB

[Cyrille Pitchen]

Le 30/11/2016 à 18:00, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> 
> 
>> -----Original Message-----
>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
>> Sent: Tuesday, November 29, 2016 5:54 PM
>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
>> computersforpeace@gmail.com; marek.vasut@gmail.com
>> Subject: Re: [PATCH 3/3] mtd: spi-nor: Enable die erase for Micron 1GiB
>>
>> Hi Marcin,
>>
>> Le 24/10/2016 à 15:03, marcin.krzeminski@nokia.com a écrit :
>>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>>>
>>> Micron N25Q00 and MT25Q00 share same JEDEC Id, but it seem can be
>>> properly recognized by second ext_jedec id byte.
>>>
>>> This commits extends n25q00 ids by adding ext bytes and also adds
>>> mt25q00 family.
>>> For MT25Q00 family, the number of dies is two, N25Q00 has it four.
>>> Logic to support that is added.
>>>
>>> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>>> ---
>>>  drivers/mtd/spi-nor/spi-nor.c | 36
>>> ++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 34 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c
>>> b/drivers/mtd/spi-nor/spi-nor.c index 17bbec0..b33ead6 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -218,6 +218,28 @@ static inline int set_4byte(struct spi_nor *nor,
>> const struct flash_info *info,
>>>  		return nor->write_reg(nor, SPINOR_OP_BRWR, nor-
>>> cmd_buf, 1);
>>>  	}
>>>  }
>>> +
>>> +static void spi_nor_die_cnt(struct spi_nor *nor,
>>> +		const struct flash_info *info)
>>> +{
>>> +	switch (JEDEC_MFR(info)) {
>>> +	case SNOR_MFR_MICRON:
>>> +		/* 1GiB devices */
>>> +		if (info->id[2] == 0x21) {
>>> +			/* MT25Q00 has 2 dies N25Q00 has 4 */
>>> +			if (info->id[4] & BIT(6))
>>> +				nor->die_cnt = 2;
>>> +			else
>>> +				nor->die_cnt = 4;
>>> +		} else
>>> +			nor->die_cnt = 0;
>>
>> You set nor->die_cnt according to the JEDED ID but what is BIT(6) in the 5th
>> byte of the JEDEC ID?
> Unfortunately 1Gb MT25Q and N25Q00 has the same JEDEDC ID.
> Datasheet for MT25Q specified this bit as revision, N25Q00 as reserved but set to 0,
> so my guess was that it can be used to distinguish them.
>> How do you know whether this "rule of thumb" also applies to other Micron
>> memories?
> No idea, at least it also should work for 2Gb chips.
>>
>> I think we should find another solution to set nor->die_cnt properly.
>> Maybe adding some info in the relevant entries of the spi_nor_ids[] array?
> I wanted to have this solve in "automatic" way by updating this function if
> there will be a need, but you are right - more generic way is better.
> I'll take a look how it could be done using spi_nor_ids.
> Maybe there are some other idea how to make this  work?
> Unfortunately JEDEC216B will not help in this case.
> 
> Thanks,
> Marcin
>

Just two proposals but they still need to be discussed of course:

1 - we could add extra fields directly in the struct flash_info

However most entries of the spi_nor_ids[] array won't use those extra fields
and the memory footprint of this array will grow anyway...

2 - adding just a single pointer in the struct flash_info

This new member would point to an optional structure containing the extra
info you need. For most entries of the spi_nor_ids[], this new pointer would
be NULL, hence limiting the memory footprint.

As I said, this is just two proposals and I would be pleased to study other
proposals :)

Best regards,

Cyrille
 
>>
>> Best regards,
>>
>> Cyrille
>>
>>> +	break;
>>> +	default:
>>> +		nor->die_cnt = 0;
>>> +		break;
>>> +	}
>>> +}
>>> +
>>>  static inline int spi_nor_sr_ready(struct spi_nor *nor)  {
>>>  	int sr = read_sr(nor);
>>> @@ -970,8 +992,14 @@ static const struct flash_info spi_nor_ids[] = {
>>>  	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K |
>> SPI_NOR_QUAD_READ) },
>>>  	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K |
>> USE_FSR | SPI_NOR_QUAD_READ) },
>>>  	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K |
>> USE_FSR | SPI_NOR_QUAD_READ) },
>>> -	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR
>> | SPI_NOR_QUAD_READ) },
>>> -	{ "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR
>> | SPI_NOR_QUAD_READ) },
>>> +	{ "n25q00",      INFO(0x20ba21, 0x1000, 64 * 1024, 2048, SECT_4K |
>> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
>>> +	{ "n25q00a",     INFO(0x20bb21, 0x1000, 64 * 1024, 2048, SECT_4K |
>> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
>>> +	{ "n25q00",      INFO(0x20ba21, 0x1004, 64 * 1024, 2048, SECT_4K |
>> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
>>> +	{ "n25q00a",     INFO(0x20bb21, 0x1004, 64 * 1024, 2048, SECT_4K |
>> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
>>> +	{ "mt25ql01g",   INFO(0x20ba21, 0x1044, 64 * 1024, 2048, SECT_4K |
>> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
>>> +	{ "mt25qu01g",   INFO(0x20bb21, 0x1044, 64 * 1024, 2048, SECT_4K |
>> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
>>> +	{ "mt25ql01g",   INFO(0x20ba21, 0x1040, 64 * 1024, 2048, SECT_4K |
>> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
>>> +	{ "mt25qu01g",   INFO(0x20bb21, 0x1040, 64 * 1024, 2048, SECT_4K |
>> USE_FSR | SPI_NOR_QUAD_READ | DIE_ERASE) },
>>>
>>>  	/* PMC */
>>>  	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },
>>> @@ -1479,6 +1507,10 @@ int spi_nor_scan(struct spi_nor *nor, const char
>> *name, enum read_mode mode)
>>>  		nor->flags |= SNOR_F_USE_FSR;
>>>  	if (info->flags & SPI_NOR_HAS_TB)
>>>  		nor->flags |= SNOR_F_HAS_SR_TB;
>>> +	if (info->flags & DIE_ERASE) {
>>> +		nor->flags |= SNOR_F_DIE_ERASE;
>>> +		spi_nor_die_cnt(nor, info);
>>> +	}
>>>
>>>  #ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
>>>  	/* prefer "small sector" erase if possible */
>>>
> 
> 

RE: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

> -----Original Message-----
> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> Sent: Wednesday, November 30, 2016 6:20 PM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> computersforpeace@gmail.com; marek.vasut@gmail.com
> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic
> 
> Le 30/11/2016 à 17:44, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> >
> >
> >> -----Original Message-----
> >> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> >> Sent: Tuesday, November 29, 2016 5:47 PM
> >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> >> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> >> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> >> computersforpeace@gmail.com; marek.vasut@gmail.com
> >> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command
> >> logic
> >>
> >> Hi Marcin,
> >>
> >> I know you have already answered some of my questions of IRC but I
> >> ask them again here on the mailing list so everybody can benefit from
> >> your answers.
> >>
> >> Le 24/10/2016 à 15:03, marcin.krzeminski@nokia.com a écrit :
> >>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> >>>
> >>> This commit implements die erase logic.
> >>> Sector at a time procedure is moved to function, then erase
> >>> algorithm will try to use die erase cmd if size and address cover
> >>> one or more dies.
> >>>
> >>
> >> I'm reading your v2 series but indeed I try to understand the purpose
> >> of the series simply because I don't know what does die erase do as
> >> compared to chip erase of sector erase.
> >> Is you series a bug fix or an optmization, maybe both?
> >
> > In current spi-nor framework when user want to erase whole flash (eg.
> > mtd_debug erase /dev/mtdX 0 0x8000000) framework issue chip erase
> cmd.
> > N25Q00 does not support chip erase, instead there was introduced die
> > erase command that can erase whole die (N25Q00 is a mulit-die NOR chip).
> > So for this case this patch is a bug fix.
> > Since I have die erase command implemented it was tempting to use die
> > erase also in case where erase area cover one or more dies to speed up
> > erase process, especially when user has enabled 4KiB sector. That is a small
> optimization.
> >
> > I will rewrite commit message and add those informations.
> >
> > Thanks,
> > Marcin
> >
> 
> OK, then I think your patch should be split into two different patches.
> 
> 1 - bug fix:
> 
> The first patch introducing an explicit flag such as SPI_NOR_NO_CHIP_ERASE,
> which would be set in the relevant entries of the spi_nor_ids[] (see already
> existing flags such as SPI_NOR_NO_FR, SPI_NOR_NO_ERASE, ...).
> 
> In spi_nor_scan():
> 
>  	if (info->flags & USE_FSR)
>  		nor->flags |= SNOR_F_USE_FSR;
>  	if (info->flags & SPI_NOR_HAS_TB)
>  		nor->flags |= SNOR_F_HAS_SR_TB;
> +	if (info->flags & SPI_NOR_NO_CHIP_ERASE)
> +		nor->flags |= SNOR_F_NO_CHIP_ERASE;
> 
> Then in spi_nor_erase():
> 
>  	/* whole-chip erase? */
> -	if (len == mtd->size) {
> +	if (len == mtd->size && !(nor->flags & SNOR_F_NO_CHIP_ERASE)) {
>  		unsigned long timeout;
> 
> So you fall into the sector erase case, no optimization yet.
> 
> 2 - optimization of spi_nor_erase()
> 
> In the "sector"-at-a-time erase branch, try to optimize when possible using
> die erase instead of sector erase.
> 
> Each commit message describing the purpose of the patch.


OK, I will split patches in this way.

> 
> I would like to also handle the cases where the memory supports:
> - both chip and die erases: if so, no reason not to use chip erase on such
>   multi-die memory.
I do not think that there are chips supporting both.
In this case two flags are needed one for disable chip erase
and another one for enabling die erase. Is that OK?
> - neither chip nor die erases.
This is supported DIE_ERASE flasg set and die_cnt == 0
> 
> Maybe I'm wrong but the assumption (multi-die => chip erase not
> supported) might be false.
It is false, but all multi-die chip I know support die erase or chip erase.
Never both.

Thanks,
Marcin

> 
> 
> Best regards,
> 
> Cyrille
> 
> 
> >>
> >> Best regards,
> >>
> >> Cyrille
> >>
> >>> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> >>> ---
> >>>  drivers/mtd/spi-nor/spi-nor.c | 90
> >>> +++++++++++++++++++++++++++++++++++++------
> >>>  1 file changed, 78 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/mtd/spi-nor/spi-nor.c
> >>> b/drivers/mtd/spi-nor/spi-nor.c index 3afe207..17bbec0 100644
> >>> --- a/drivers/mtd/spi-nor/spi-nor.c
> >>> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >>> @@ -360,6 +360,29 @@ static int spi_nor_erase_sector(struct spi_nor
> >> *nor, u32 addr)
> >>>  	return nor->write_reg(nor, nor->erase_opcode, buf, nor-
> >>> addr_width); }
> >>>
> >>> +static inline int spi_nor_sector_at_time_erase(struct mtd_info
> >>> +*mtd,
> >>> +u32 addr, u32 len) {
> >>> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> >>> +	int ret = 0;
> >>> +
> >>> +	while (len) {
> >>> +		write_enable(nor);
> >>> +
> >>> +		ret = spi_nor_erase_sector(nor, addr);
> >>> +		if (ret)
> >>> +			return ret;
> >>> +
> >>> +		addr += mtd->erasesize;
> >>> +		len -= mtd->erasesize;
> >>> +
> >>> +		ret = spi_nor_wait_till_ready(nor);
> >>> +		if (ret)
> >>> +			return ret;
> >>> +	}
> >>> +
> >>> +	return ret;
> >>> +}
> >>> +
> >>>  /*
> >>>   * Erase an address range on the nor chip.  The address range may
> extend
> >>>   * one or more erase sectors.  Return an error is there is a problem
> erasing.
> >>> @@ -367,9 +390,11 @@ static int spi_nor_erase_sector(struct spi_nor
> >>> *nor, u32 addr)  static int spi_nor_erase(struct mtd_info *mtd,
> >>> struct erase_info *instr)  {
> >>>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> >>> -	u32 addr, len;
> >>> +	u32 addr, len, die_no, die_size;
> >>>  	uint32_t rem;
> >>>  	int ret;
> >>> +	unsigned long timeout;
> >>> +	u8 die_erase = nor->die_cnt && SNOR_F_DIE_ERASE;
> >>>
> >>>  	dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr->addr,
> >>>  			(long long)instr->len);
> >>> @@ -386,7 +411,7 @@ static int spi_nor_erase(struct mtd_info *mtd,
> >> struct erase_info *instr)
> >>>  		return ret;
> >>>
> >>>  	/* whole-chip erase? */
> >>> -	if (len == mtd->size) {
> >>> +	if (len == mtd->size && !die_erase) {
> >>>  		unsigned long timeout;
> >>>
> >>>  		write_enable(nor);
> >>> @@ -416,17 +441,58 @@ static int spi_nor_erase(struct mtd_info *mtd,
> >>> struct erase_info *instr)
> >>>
> >>>  	/* "sector"-at-a-time erase */
> >>>  	} else {
> >>> -		while (len) {
> >>> -			write_enable(nor);
> >>> -
> >>> -			ret = spi_nor_erase_sector(nor, addr);
> >>> -			if (ret)
> >>> +		if (die_erase) {
> >>> +			die_size = div_u64_rem(mtd->size, nor->die_cnt,
> >> &rem);
> >>> +			if (rem) {
> >>> +				ret = -EINVAL;
> >>>  				goto erase_err;
> >>> -
> >>> -			addr += mtd->erasesize;
> >>> -			len -= mtd->erasesize;
> >>> -
> >>> -			ret = spi_nor_wait_till_ready(nor);
> >>> +			}
> >>> +			while (len) {
> >>> +				die_no = div_u64_rem(addr, die_size,
> >> &rem);
> >>> +
> >>> +				/* Check if address is aligned to die begin*/
> >>> +				if (!rem) {
> >>> +					/* die erase? */
> >>> +					if (len >= die_size) {
> >>> +						ret = spi_nor_die_erase(nor,
> >> addr);
> >>> +						if (ret)
> >>> +							goto erase_err;
> >>> +
> >>> +						len -= die_size;
> >>> +						addr += die_size;
> >>> +
> >>> +						timeout =
> >> max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
> >>> +
> >> 	CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
> >>> +								(unsigned
> >> long)(die_size / SZ_2M));
> >>> +						ret =
> >> spi_nor_wait_till_ready_with_timeout(nor, timeout);
> >>> +						if (ret)
> >>> +							goto erase_err;
> >>> +					} else {
> >>> +						ret =
> >> spi_nor_sector_at_time_erase(mtd, addr, len);
> >>> +						if (ret)
> >>> +							goto erase_err;
> >>> +						len = 0;
> >>> +					}
> >>> +				} else {
> >>> +					/* check if end address cover at least
> >> one die */
> >>> +					if (div64_ul(addr + len, die_size) >
> >> ++die_no) {
> >>> +						/* align to next die */
> >>> +						rem = die_size - rem;
> >>> +						ret =
> >> spi_nor_sector_at_time_erase(mtd, addr, rem);
> >>> +						if (ret)
> >>> +							goto erase_err;
> >>> +						len -= rem;
> >>> +						addr += rem;
> >>> +					} else {
> >>> +						ret =
> >> spi_nor_sector_at_time_erase(mtd, addr, len);
> >>> +						if (ret)
> >>> +							goto erase_err;
> >>> +						len = 0;
> >>> +					}
> >>> +				}
> >>> +			}
> >>> +		} else {
> >>> +			ret = spi_nor_sector_at_time_erase(mtd, addr, len);
> >>>  			if (ret)
> >>>  				goto erase_err;
> >>>  		}
> >>>
> >
> >

Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic

[Cyrille Pitchen]

Le 30/11/2016 à 19:18, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> 
> 
>> -----Original Message-----
>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
>> Sent: Wednesday, November 30, 2016 6:20 PM
>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
>> computersforpeace@gmail.com; marek.vasut@gmail.com
>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic
>>
>> Le 30/11/2016 à 17:44, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
>>>
>>>
>>>> -----Original Message-----
>>>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
>>>> Sent: Tuesday, November 29, 2016 5:47 PM
>>>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
>>>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
>>>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
>>>> computersforpeace@gmail.com; marek.vasut@gmail.com
>>>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command
>>>> logic
>>>>
>>>> Hi Marcin,
>>>>
>>>> I know you have already answered some of my questions of IRC but I
>>>> ask them again here on the mailing list so everybody can benefit from
>>>> your answers.
>>>>
>>>> Le 24/10/2016 à 15:03, marcin.krzeminski@nokia.com a écrit :
>>>>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>>>>>
>>>>> This commit implements die erase logic.
>>>>> Sector at a time procedure is moved to function, then erase
>>>>> algorithm will try to use die erase cmd if size and address cover
>>>>> one or more dies.
>>>>>
>>>>
>>>> I'm reading your v2 series but indeed I try to understand the purpose
>>>> of the series simply because I don't know what does die erase do as
>>>> compared to chip erase of sector erase.
>>>> Is you series a bug fix or an optmization, maybe both?
>>>
>>> In current spi-nor framework when user want to erase whole flash (eg.
>>> mtd_debug erase /dev/mtdX 0 0x8000000) framework issue chip erase
>> cmd.
>>> N25Q00 does not support chip erase, instead there was introduced die
>>> erase command that can erase whole die (N25Q00 is a mulit-die NOR chip).
>>> So for this case this patch is a bug fix.
>>> Since I have die erase command implemented it was tempting to use die
>>> erase also in case where erase area cover one or more dies to speed up
>>> erase process, especially when user has enabled 4KiB sector. That is a small
>> optimization.
>>>
>>> I will rewrite commit message and add those informations.
>>>
>>> Thanks,
>>> Marcin
>>>
>>
>> OK, then I think your patch should be split into two different patches.
>>
>> 1 - bug fix:
>>
>> The first patch introducing an explicit flag such as SPI_NOR_NO_CHIP_ERASE,
>> which would be set in the relevant entries of the spi_nor_ids[] (see already
>> existing flags such as SPI_NOR_NO_FR, SPI_NOR_NO_ERASE, ...).
>>
>> In spi_nor_scan():
>>
>>  	if (info->flags & USE_FSR)
>>  		nor->flags |= SNOR_F_USE_FSR;
>>  	if (info->flags & SPI_NOR_HAS_TB)
>>  		nor->flags |= SNOR_F_HAS_SR_TB;
>> +	if (info->flags & SPI_NOR_NO_CHIP_ERASE)
>> +		nor->flags |= SNOR_F_NO_CHIP_ERASE;
>>
>> Then in spi_nor_erase():
>>
>>  	/* whole-chip erase? */
>> -	if (len == mtd->size) {
>> +	if (len == mtd->size && !(nor->flags & SNOR_F_NO_CHIP_ERASE)) {
>>  		unsigned long timeout;
>>
>> So you fall into the sector erase case, no optimization yet.
>>
>> 2 - optimization of spi_nor_erase()
>>
>> In the "sector"-at-a-time erase branch, try to optimize when possible using
>> die erase instead of sector erase.
>>
>> Each commit message describing the purpose of the patch.
> 
> 
> OK, I will split patches in this way.
> 
>>
>> I would like to also handle the cases where the memory supports:
>> - both chip and die erases: if so, no reason not to use chip erase on such
>>   multi-die memory.
> I do not think that there are chips supporting both.
> In this case two flags are needed one for disable chip erase
> and another one for enabling die erase. Is that OK?

I think it might be better to have 2 separated flags, at least for semantic
reason: "the memory supports Die-Erase hence we don't try to use
Chip-Erase" sounds a little bit odd to me.

This is what I understand when I read:
 	/* whole-chip erase? */
-	if (len == mtd->size) {
+	if (len == mtd->size && !die_erase) {

Why do we try Chip-Erase only if Die-Erase is not supported?

Reading something like "we try Chip-Erase unless Chip-Erase is not
supported" is more logic, isn't it?

I want to avoid the implicit assumption "Die Erase supported => Chip Erase
not supported", even if it *might* be true, because it is strange when we
read the code.

>> - neither chip nor die erases.
> This is supported DIE_ERASE flasg set and die_cnt == 0

DIE_ERASE flag set and die_cnt == 0 meaning Chip-Erase is not supported?
IHMO, semantically, it's not logical.

Also discussing another topic on #mtd, it seems that some memories don't
have a 4-byte address version of the Die Erase op code.

If so, it's preferred to use a 4-byte address Sector/Block Erase op code
than to enter/exit the statefull 4-byte address mode then use the Die Erase
op code: this statefull mode should be avoided as much as possible because
many bootloaders expect the SPI memory to be in 3byte address mode and if a
spurious reset occurs on the CPU side (but not on the memory side) those
bootloaders will fail to read data from the SPI memory.

>>
>> Maybe I'm wrong but the assumption (multi-die => chip erase not
>> supported) might be false.
> It is false, but all multi-die chip I know support die erase or chip erase.
> Never both.
> 
> Thanks,
> Marcin
> 
>>
>>
>> Best regards,
>>
>> Cyrille
>>
>>
>>>>
>>>> Best regards,
>>>>
>>>> Cyrille
>>>>
>>>>> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>>>>> ---
>>>>>  drivers/mtd/spi-nor/spi-nor.c | 90
>>>>> +++++++++++++++++++++++++++++++++++++------
>>>>>  1 file changed, 78 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c
>>>>> b/drivers/mtd/spi-nor/spi-nor.c index 3afe207..17bbec0 100644
>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>>> @@ -360,6 +360,29 @@ static int spi_nor_erase_sector(struct spi_nor
>>>> *nor, u32 addr)
>>>>>  	return nor->write_reg(nor, nor->erase_opcode, buf, nor-
>>>>> addr_width); }
>>>>>
>>>>> +static inline int spi_nor_sector_at_time_erase(struct mtd_info
>>>>> +*mtd,
>>>>> +u32 addr, u32 len) {
>>>>> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>>>>> +	int ret = 0;
>>>>> +
>>>>> +	while (len) {
>>>>> +		write_enable(nor);
>>>>> +
>>>>> +		ret = spi_nor_erase_sector(nor, addr);
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>> +
>>>>> +		addr += mtd->erasesize;
>>>>> +		len -= mtd->erasesize;
>>>>> +
>>>>> +		ret = spi_nor_wait_till_ready(nor);
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>> +	}
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>>  /*
>>>>>   * Erase an address range on the nor chip.  The address range may
>> extend
>>>>>   * one or more erase sectors.  Return an error is there is a problem
>> erasing.
>>>>> @@ -367,9 +390,11 @@ static int spi_nor_erase_sector(struct spi_nor
>>>>> *nor, u32 addr)  static int spi_nor_erase(struct mtd_info *mtd,
>>>>> struct erase_info *instr)  {
>>>>>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>>>>> -	u32 addr, len;
>>>>> +	u32 addr, len, die_no, die_size;
>>>>>  	uint32_t rem;
>>>>>  	int ret;
>>>>> +	unsigned long timeout;
>>>>> +	u8 die_erase = nor->die_cnt && SNOR_F_DIE_ERASE;
>>>>>
>>>>>  	dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr->addr,
>>>>>  			(long long)instr->len);
>>>>> @@ -386,7 +411,7 @@ static int spi_nor_erase(struct mtd_info *mtd,
>>>> struct erase_info *instr)
>>>>>  		return ret;
>>>>>
>>>>>  	/* whole-chip erase? */
>>>>> -	if (len == mtd->size) {
>>>>> +	if (len == mtd->size && !die_erase) {
>>>>>  		unsigned long timeout;
>>>>>
>>>>>  		write_enable(nor);
>>>>> @@ -416,17 +441,58 @@ static int spi_nor_erase(struct mtd_info *mtd,
>>>>> struct erase_info *instr)
>>>>>
>>>>>  	/* "sector"-at-a-time erase */
>>>>>  	} else {
>>>>> -		while (len) {
>>>>> -			write_enable(nor);
>>>>> -
>>>>> -			ret = spi_nor_erase_sector(nor, addr);
>>>>> -			if (ret)
>>>>> +		if (die_erase) {
>>>>> +			die_size = div_u64_rem(mtd->size, nor->die_cnt,
>>>> &rem);
>>>>> +			if (rem) {
>>>>> +				ret = -EINVAL;
>>>>>  				goto erase_err;
>>>>> -
>>>>> -			addr += mtd->erasesize;
>>>>> -			len -= mtd->erasesize;
>>>>> -
>>>>> -			ret = spi_nor_wait_till_ready(nor);
>>>>> +			}
>>>>> +			while (len) {
>>>>> +				die_no = div_u64_rem(addr, die_size,
>>>> &rem);
>>>>> +
>>>>> +				/* Check if address is aligned to die begin*/
>>>>> +				if (!rem) {
>>>>> +					/* die erase? */
>>>>> +					if (len >= die_size) {
>>>>> +						ret = spi_nor_die_erase(nor,
>>>> addr);
>>>>> +						if (ret)
>>>>> +							goto erase_err;
>>>>> +
>>>>> +						len -= die_size;
>>>>> +						addr += die_size;
>>>>> +
>>>>> +						timeout =
>>>> max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
>>>>> +
>>>> 	CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
>>>>> +								(unsigned
>>>> long)(die_size / SZ_2M));
>>>>> +						ret =
>>>> spi_nor_wait_till_ready_with_timeout(nor, timeout);
>>>>> +						if (ret)
>>>>> +							goto erase_err;
>>>>> +					} else {
>>>>> +						ret =
>>>> spi_nor_sector_at_time_erase(mtd, addr, len);
>>>>> +						if (ret)
>>>>> +							goto erase_err;
>>>>> +						len = 0;
>>>>> +					}
>>>>> +				} else {
>>>>> +					/* check if end address cover at least
>>>> one die */
>>>>> +					if (div64_ul(addr + len, die_size) >
>>>> ++die_no) {
>>>>> +						/* align to next die */
>>>>> +						rem = die_size - rem;
>>>>> +						ret =
>>>> spi_nor_sector_at_time_erase(mtd, addr, rem);
>>>>> +						if (ret)
>>>>> +							goto erase_err;
>>>>> +						len -= rem;
>>>>> +						addr += rem;
>>>>> +					} else {
>>>>> +						ret =
>>>> spi_nor_sector_at_time_erase(mtd, addr, len);
>>>>> +						if (ret)
>>>>> +							goto erase_err;
>>>>> +						len = 0;
>>>>> +					}
>>>>> +				}
>>>>> +			}
>>>>> +		} else {
>>>>> +			ret = spi_nor_sector_at_time_erase(mtd, addr, len);
>>>>>  			if (ret)
>>>>>  				goto erase_err;
>>>>>  		}
>>>>>
>>>
>>>
> 
> 

RE: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

Cyrille,

Sorry for late answer.

> -----Original Message-----
> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> Sent: Friday, December 09, 2016 6:23 PM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> computersforpeace@gmail.com; marek.vasut@gmail.com
> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic
> 
> Le 30/11/2016 à 19:18, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> >
> >
> >> -----Original Message-----
> >> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> >> Sent: Wednesday, November 30, 2016 6:20 PM
> >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> >> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> >> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> >> computersforpeace@gmail.com; marek.vasut@gmail.com
> >> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command
> >> logic
> >>
> >> Le 30/11/2016 à 17:44, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> >>>> Sent: Tuesday, November 29, 2016 5:47 PM
> >>>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> >>>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> >>>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> >>>> computersforpeace@gmail.com; marek.vasut@gmail.com
> >>>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command
> >>>> logic
> >>>>
> >>>> Hi Marcin,
> >>>>
> >>>> I know you have already answered some of my questions of IRC but I
> >>>> ask them again here on the mailing list so everybody can benefit
> >>>> from your answers.
> >>>>
> >>>> Le 24/10/2016 à 15:03, marcin.krzeminski@nokia.com a écrit :
> >>>>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> >>>>>
> >>>>> This commit implements die erase logic.
> >>>>> Sector at a time procedure is moved to function, then erase
> >>>>> algorithm will try to use die erase cmd if size and address cover
> >>>>> one or more dies.
> >>>>>
> >>>>
> >>>> I'm reading your v2 series but indeed I try to understand the
> >>>> purpose of the series simply because I don't know what does die
> >>>> erase do as compared to chip erase of sector erase.
> >>>> Is you series a bug fix or an optmization, maybe both?
> >>>
> >>> In current spi-nor framework when user want to erase whole flash (eg.
> >>> mtd_debug erase /dev/mtdX 0 0x8000000) framework issue chip erase
> >> cmd.
> >>> N25Q00 does not support chip erase, instead there was introduced die
> >>> erase command that can erase whole die (N25Q00 is a mulit-die NOR
> chip).
> >>> So for this case this patch is a bug fix.
> >>> Since I have die erase command implemented it was tempting to use
> >>> die erase also in case where erase area cover one or more dies to
> >>> speed up erase process, especially when user has enabled 4KiB
> >>> sector. That is a small
> >> optimization.
> >>>
> >>> I will rewrite commit message and add those informations.
> >>>
> >>> Thanks,
> >>> Marcin
> >>>
> >>
> >> OK, then I think your patch should be split into two different patches.
> >>
> >> 1 - bug fix:
> >>
> >> The first patch introducing an explicit flag such as
> >> SPI_NOR_NO_CHIP_ERASE, which would be set in the relevant entries of
> >> the spi_nor_ids[] (see already existing flags such as SPI_NOR_NO_FR,
> SPI_NOR_NO_ERASE, ...).
> >>
> >> In spi_nor_scan():
> >>
> >>  	if (info->flags & USE_FSR)
> >>  		nor->flags |= SNOR_F_USE_FSR;
> >>  	if (info->flags & SPI_NOR_HAS_TB)
> >>  		nor->flags |= SNOR_F_HAS_SR_TB;
> >> +	if (info->flags & SPI_NOR_NO_CHIP_ERASE)
> >> +		nor->flags |= SNOR_F_NO_CHIP_ERASE;
> >>
> >> Then in spi_nor_erase():
> >>
> >>  	/* whole-chip erase? */
> >> -	if (len == mtd->size) {
> >> +	if (len == mtd->size && !(nor->flags & SNOR_F_NO_CHIP_ERASE)) {
> >>  		unsigned long timeout;
> >>
> >> So you fall into the sector erase case, no optimization yet.
> >>
> >> 2 - optimization of spi_nor_erase()
> >>
> >> In the "sector"-at-a-time erase branch, try to optimize when possible
> >> using die erase instead of sector erase.
> >>
> >> Each commit message describing the purpose of the patch.
> >
> >
> > OK, I will split patches in this way.
> >
> >>
> >> I would like to also handle the cases where the memory supports:
> >> - both chip and die erases: if so, no reason not to use chip erase on such
> >>   multi-die memory.
> > I do not think that there are chips supporting both.
> > In this case two flags are needed one for disable chip erase and
> > another one for enabling die erase. Is that OK?
> 
> I think it might be better to have 2 separated flags, at least for semantic
> reason: "the memory supports Die-Erase hence we don't try to use Chip-
> Erase" sounds a little bit odd to me.

Let's do it with two flags then.

> 
> This is what I understand when I read:
>  	/* whole-chip erase? */
> -	if (len == mtd->size) {
> +	if (len == mtd->size && !die_erase) {
> 
> Why do we try Chip-Erase only if Die-Erase is not supported?

I assumed that there is no flash that support both.
Do you know one that has chip erase and die erase?

> 
> Reading something like "we try Chip-Erase unless Chip-Erase is not
> supported" is more logic, isn't it?
> 
> I want to avoid the implicit assumption "Die Erase supported => Chip Erase
> not supported", even if it *might* be true, because it is strange when we
> read the code.

Ok.
> 
> >> - neither chip nor die erases.
> > This is supported DIE_ERASE flasg set and die_cnt == 0
> 
> DIE_ERASE flag set and die_cnt == 0 meaning Chip-Erase is not supported?
> IHMO, semantically, it's not logical.

Just to save yet another flag. Having so many flags make codes complicated,
but I agree the it I at least logical.
> 
> Also discussing another topic on #mtd, it seems that some memories don't
> have a 4-byte address version of the Die Erase op code.

Yes. Could be workarounded by enabling 4byte address mode just before
sending die erase and then disable it back.
The more important question is if you really one to have in upstream die erase?
It is about 30% faster than  sector at time erase on some flash
(especially in case 4KiB sectors). From the other hand I see chips that it was
not faster at all (it also applies on chip erase).
My first version of fixing this problem was to add only the flag of disable chip erase.
See here:
http://lists.infradead.org/pipermail/linux-mtd/2016-October/069888.html
Maybe for now it is better to take above?

> 
> If so, it's preferred to use a 4-byte address Sector/Block Erase op code than
> to enter/exit the statefull 4-byte address mode then use the Die Erase op
> code: this statefull mode should be avoided as much as possible because
> many bootloaders expect the SPI memory to be in 3byte address mode and
> if a spurious reset occurs on the CPU side (but not on the memory side)
> those bootloaders will fail to read data from the SPI memory.

Yeap, moreover similar problem you will face when you will try to
add QPi modes (444) to framework.

Thanks,
Marcin

> 
> >>
> >> Maybe I'm wrong but the assumption (multi-die => chip erase not
> >> supported) might be false.
> > It is false, but all multi-die chip I know support die erase or chip erase.
> > Never both.
> >
> > Thanks,
> > Marcin
> >
> >>
> >>
> >> Best regards,
> >>
> >> Cyrille
> >>
> >>
> >>>>
> >>>> Best regards,
> >>>>
> >>>> Cyrille
> >>>>
> >>>>> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> >>>>> ---
> >>>>>  drivers/mtd/spi-nor/spi-nor.c | 90
> >>>>> +++++++++++++++++++++++++++++++++++++------
> >>>>>  1 file changed, 78 insertions(+), 12 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c
> >>>>> b/drivers/mtd/spi-nor/spi-nor.c index 3afe207..17bbec0 100644
> >>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
> >>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >>>>> @@ -360,6 +360,29 @@ static int spi_nor_erase_sector(struct
> >>>>> spi_nor
> >>>> *nor, u32 addr)
> >>>>>  	return nor->write_reg(nor, nor->erase_opcode, buf, nor-
> >>>>> addr_width); }
> >>>>>
> >>>>> +static inline int spi_nor_sector_at_time_erase(struct mtd_info
> >>>>> +*mtd,
> >>>>> +u32 addr, u32 len) {
> >>>>> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> >>>>> +	int ret = 0;
> >>>>> +
> >>>>> +	while (len) {
> >>>>> +		write_enable(nor);
> >>>>> +
> >>>>> +		ret = spi_nor_erase_sector(nor, addr);
> >>>>> +		if (ret)
> >>>>> +			return ret;
> >>>>> +
> >>>>> +		addr += mtd->erasesize;
> >>>>> +		len -= mtd->erasesize;
> >>>>> +
> >>>>> +		ret = spi_nor_wait_till_ready(nor);
> >>>>> +		if (ret)
> >>>>> +			return ret;
> >>>>> +	}
> >>>>> +
> >>>>> +	return ret;
> >>>>> +}
> >>>>> +
> >>>>>  /*
> >>>>>   * Erase an address range on the nor chip.  The address range may
> >> extend
> >>>>>   * one or more erase sectors.  Return an error is there is a
> >>>>> problem
> >> erasing.
> >>>>> @@ -367,9 +390,11 @@ static int spi_nor_erase_sector(struct
> >>>>> spi_nor *nor, u32 addr)  static int spi_nor_erase(struct mtd_info
> >>>>> *mtd, struct erase_info *instr)  {
> >>>>>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> >>>>> -	u32 addr, len;
> >>>>> +	u32 addr, len, die_no, die_size;
> >>>>>  	uint32_t rem;
> >>>>>  	int ret;
> >>>>> +	unsigned long timeout;
> >>>>> +	u8 die_erase = nor->die_cnt && SNOR_F_DIE_ERASE;
> >>>>>
> >>>>>  	dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr->addr,
> >>>>>  			(long long)instr->len);
> >>>>> @@ -386,7 +411,7 @@ static int spi_nor_erase(struct mtd_info *mtd,
> >>>> struct erase_info *instr)
> >>>>>  		return ret;
> >>>>>
> >>>>>  	/* whole-chip erase? */
> >>>>> -	if (len == mtd->size) {
> >>>>> +	if (len == mtd->size && !die_erase) {
> >>>>>  		unsigned long timeout;
> >>>>>
> >>>>>  		write_enable(nor);
> >>>>> @@ -416,17 +441,58 @@ static int spi_nor_erase(struct mtd_info
> >>>>> *mtd, struct erase_info *instr)
> >>>>>
> >>>>>  	/* "sector"-at-a-time erase */
> >>>>>  	} else {
> >>>>> -		while (len) {
> >>>>> -			write_enable(nor);
> >>>>> -
> >>>>> -			ret = spi_nor_erase_sector(nor, addr);
> >>>>> -			if (ret)
> >>>>> +		if (die_erase) {
> >>>>> +			die_size = div_u64_rem(mtd->size, nor-
> >die_cnt,
> >>>> &rem);
> >>>>> +			if (rem) {
> >>>>> +				ret = -EINVAL;
> >>>>>  				goto erase_err;
> >>>>> -
> >>>>> -			addr += mtd->erasesize;
> >>>>> -			len -= mtd->erasesize;
> >>>>> -
> >>>>> -			ret = spi_nor_wait_till_ready(nor);
> >>>>> +			}
> >>>>> +			while (len) {
> >>>>> +				die_no = div_u64_rem(addr,
> die_size,
> >>>> &rem);
> >>>>> +
> >>>>> +				/* Check if address is aligned to die
> begin*/
> >>>>> +				if (!rem) {
> >>>>> +					/* die erase? */
> >>>>> +					if (len >= die_size) {
> >>>>> +						ret =
> spi_nor_die_erase(nor,
> >>>> addr);
> >>>>> +						if (ret)
> >>>>> +							goto
> erase_err;
> >>>>> +
> >>>>> +						len -= die_size;
> >>>>> +						addr += die_size;
> >>>>> +
> >>>>> +						timeout =
> >>>> max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
> >>>>> +
> >>>> 	CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
> >>>>> +
> 	(unsigned
> >>>> long)(die_size / SZ_2M));
> >>>>> +						ret =
> >>>> spi_nor_wait_till_ready_with_timeout(nor, timeout);
> >>>>> +						if (ret)
> >>>>> +							goto
> erase_err;
> >>>>> +					} else {
> >>>>> +						ret =
> >>>> spi_nor_sector_at_time_erase(mtd, addr, len);
> >>>>> +						if (ret)
> >>>>> +							goto
> erase_err;
> >>>>> +						len = 0;
> >>>>> +					}
> >>>>> +				} else {
> >>>>> +					/* check if end address cover
> at least
> >>>> one die */
> >>>>> +					if (div64_ul(addr + len,
> die_size) >
> >>>> ++die_no) {
> >>>>> +						/* align to next die */
> >>>>> +						rem = die_size - rem;
> >>>>> +						ret =
> >>>> spi_nor_sector_at_time_erase(mtd, addr, rem);
> >>>>> +						if (ret)
> >>>>> +							goto
> erase_err;
> >>>>> +						len -= rem;
> >>>>> +						addr += rem;
> >>>>> +					} else {
> >>>>> +						ret =
> >>>> spi_nor_sector_at_time_erase(mtd, addr, len);
> >>>>> +						if (ret)
> >>>>> +							goto
> erase_err;
> >>>>> +						len = 0;
> >>>>> +					}
> >>>>> +				}
> >>>>> +			}
> >>>>> +		} else {
> >>>>> +			ret = spi_nor_sector_at_time_erase(mtd,
> addr, len);
> >>>>>  			if (ret)
> >>>>>  				goto erase_err;
> >>>>>  		}
> >>>>>
> >>>
> >>>
> >
> >

Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic

[Cyrille Pitchen]

Hi Marcin,

Le 16/12/2016 à 07:38, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> Cyrille,
> 
> Sorry for late answer.
> 
>> -----Original Message-----
>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
>> Sent: Friday, December 09, 2016 6:23 PM
>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
>> computersforpeace@gmail.com; marek.vasut@gmail.com
>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic
>>
>> Le 30/11/2016 à 19:18, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
>>>
>>>
>>>> -----Original Message-----
>>>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
>>>> Sent: Wednesday, November 30, 2016 6:20 PM
>>>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
>>>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
>>>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
>>>> computersforpeace@gmail.com; marek.vasut@gmail.com
>>>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command
>>>> logic
>>>>
>>>> Le 30/11/2016 à 17:44, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
>>>>>> Sent: Tuesday, November 29, 2016 5:47 PM
>>>>>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
>>>>>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
>>>>>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
>>>>>> computersforpeace@gmail.com; marek.vasut@gmail.com
>>>>>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command
>>>>>> logic
>>>>>>
>>>>>> Hi Marcin,
>>>>>>
>>>>>> I know you have already answered some of my questions of IRC but I
>>>>>> ask them again here on the mailing list so everybody can benefit
>>>>>> from your answers.
>>>>>>
>>>>>> Le 24/10/2016 à 15:03, marcin.krzeminski@nokia.com a écrit :
>>>>>>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>>>>>>>
>>>>>>> This commit implements die erase logic.
>>>>>>> Sector at a time procedure is moved to function, then erase
>>>>>>> algorithm will try to use die erase cmd if size and address cover
>>>>>>> one or more dies.
>>>>>>>
>>>>>>
>>>>>> I'm reading your v2 series but indeed I try to understand the
>>>>>> purpose of the series simply because I don't know what does die
>>>>>> erase do as compared to chip erase of sector erase.
>>>>>> Is you series a bug fix or an optmization, maybe both?
>>>>>
>>>>> In current spi-nor framework when user want to erase whole flash (eg.
>>>>> mtd_debug erase /dev/mtdX 0 0x8000000) framework issue chip erase
>>>> cmd.
>>>>> N25Q00 does not support chip erase, instead there was introduced die
>>>>> erase command that can erase whole die (N25Q00 is a mulit-die NOR
>> chip).
>>>>> So for this case this patch is a bug fix.
>>>>> Since I have die erase command implemented it was tempting to use
>>>>> die erase also in case where erase area cover one or more dies to
>>>>> speed up erase process, especially when user has enabled 4KiB
>>>>> sector. That is a small
>>>> optimization.
>>>>>
>>>>> I will rewrite commit message and add those informations.
>>>>>
>>>>> Thanks,
>>>>> Marcin
>>>>>
>>>>
>>>> OK, then I think your patch should be split into two different patches.
>>>>
>>>> 1 - bug fix:
>>>>
>>>> The first patch introducing an explicit flag such as
>>>> SPI_NOR_NO_CHIP_ERASE, which would be set in the relevant entries of
>>>> the spi_nor_ids[] (see already existing flags such as SPI_NOR_NO_FR,
>> SPI_NOR_NO_ERASE, ...).
>>>>
>>>> In spi_nor_scan():
>>>>
>>>>  	if (info->flags & USE_FSR)
>>>>  		nor->flags |= SNOR_F_USE_FSR;
>>>>  	if (info->flags & SPI_NOR_HAS_TB)
>>>>  		nor->flags |= SNOR_F_HAS_SR_TB;
>>>> +	if (info->flags & SPI_NOR_NO_CHIP_ERASE)
>>>> +		nor->flags |= SNOR_F_NO_CHIP_ERASE;
>>>>
>>>> Then in spi_nor_erase():
>>>>
>>>>  	/* whole-chip erase? */
>>>> -	if (len == mtd->size) {
>>>> +	if (len == mtd->size && !(nor->flags & SNOR_F_NO_CHIP_ERASE)) {
>>>>  		unsigned long timeout;
>>>>
>>>> So you fall into the sector erase case, no optimization yet.
>>>>
>>>> 2 - optimization of spi_nor_erase()
>>>>
>>>> In the "sector"-at-a-time erase branch, try to optimize when possible
>>>> using die erase instead of sector erase.
>>>>
>>>> Each commit message describing the purpose of the patch.
>>>
>>>
>>> OK, I will split patches in this way.
>>>
>>>>
>>>> I would like to also handle the cases where the memory supports:
>>>> - both chip and die erases: if so, no reason not to use chip erase on such
>>>>   multi-die memory.
>>> I do not think that there are chips supporting both.
>>> In this case two flags are needed one for disable chip erase and
>>> another one for enabling die erase. Is that OK?
>>
>> I think it might be better to have 2 separated flags, at least for semantic
>> reason: "the memory supports Die-Erase hence we don't try to use Chip-
>> Erase" sounds a little bit odd to me.
> 
> Let's do it with two flags then.
> 
>>
>> This is what I understand when I read:
>>  	/* whole-chip erase? */
>> -	if (len == mtd->size) {
>> +	if (len == mtd->size && !die_erase) {
>>
>> Why do we try Chip-Erase only if Die-Erase is not supported?
> 
> I assumed that there is no flash that support both.
> Do you know one that has chip erase and die erase?
>

No, I don't but I didn't focus on erase operations so maybe some already
exist or might exit in the future, who knows? :)

Also, please have a look at Ricardo's patch:
[v9] mtd: spi-nor: Add supported for S3AN spi-nor devices
http://patchwork.ozlabs.org/patch/701935/

This patch has recently been merged into git://github.com/spi-nor/linux.git
so you will have to rebase your work on that tree.

Ricardo's patch already introduces a SNOR_F_NO_OP_CHIP_ERASE flag used from
spi_nor_erase(). The only missing part is to set this flag in nor->flag
when another dedicated flag is set in info->flags.
Currently this SNOR_F_NO_OP_CHIP_ERASE is only set from s3an_nor_scan()
but it should be also set from spi_nor_scan() for the relevant memories.

>>
>> Reading something like "we try Chip-Erase unless Chip-Erase is not
>> supported" is more logic, isn't it?
>>
>> I want to avoid the implicit assumption "Die Erase supported => Chip Erase
>> not supported", even if it *might* be true, because it is strange when we
>> read the code.
> 
> Ok.
>>
>>>> - neither chip nor die erases.
>>> This is supported DIE_ERASE flasg set and die_cnt == 0
>>
>> DIE_ERASE flag set and die_cnt == 0 meaning Chip-Erase is not supported?
>> IHMO, semantically, it's not logical.
> 
> Just to save yet another flag. Having so many flags make codes complicated,
> but I agree the it I at least logical.
>>
>> Also discussing another topic on #mtd, it seems that some memories don't
>> have a 4-byte address version of the Die Erase op code.
> 
> Yes. Could be workarounded by enabling 4byte address mode just before
> sending die erase and then disable it back.
> The more important question is if you really one to have in upstream die erase?
> It is about 30% faster than  sector at time erase on some flash
> (especially in case 4KiB sectors). From the other hand I see chips that it was
> not faster at all (it also applies on chip erase).
> My first version of fixing this problem was to add only the flag of disable chip erase.
> See here:
> http://lists.infradead.org/pipermail/linux-mtd/2016-October/069888.html
> Maybe for now it is better to take above?
> 
>>
>> If so, it's preferred to use a 4-byte address Sector/Block Erase op code than
>> to enter/exit the statefull 4-byte address mode then use the Die Erase op
>> code: this statefull mode should be avoided as much as possible because
>> many bootloaders expect the SPI memory to be in 3byte address mode and
>> if a spurious reset occurs on the CPU side (but not on the memory side)
>> those bootloaders will fail to read data from the SPI memory.
> 
> Yeap, moreover similar problem you will face when you will try to
> add QPi modes (444) to framework.
>

Entering the 4-byte address mode might be accepted as a workaround to allow
using the Die Erase op code even on > 16MiB memory as long as the user can
choose to disable this feature and the feature is not enabled by default.
This way we don't prevent end users from using Die Erase if they want but
we don't force them to use it if they don't want to.

Indeed, Die Erase is an optimization but it is not mandatory to erase the
memory: the slower Sector/Block Erase commands can still be used.
However entering the 4-byte address mode to allow using the Die Erase
command has side effects, as explained above, that might introduce regression.

Let's imagine the case of a 2-die memory used to store 2 firmware images.
The firmware update process could alternate between dies when writing a new
firmware image so the current/older firmware image is still available if
the update process fails to write the new image.

Then, if you make the whole memory enter its 4-byte address mode to erase
one single die and a spurious reboot occurs, many bootloaders would fail to
read the old firmware from the other die just because the whole memory
expects 4-byte addresses.

About the QPI mode (SPI 4-4-4), I agree with you, it has the very same
annoying side effect. Hence I've removed support of the SPI 2-2-2 and SPI
4-4-4 protocols at the memory side from my SFDP series for the moment.
Besides, on the controller side, by changing the 3rd argument parameter of
spi_nor_scan(), one of my patch allows the SPI controller driver to claim
"I can't/don't want to use SPI 4-4-4 but I support SPI 1-1-4 and SPI 1-4-4
protocols". So the SPI 4-4-4 protocol can be disabled if needed.

Best regards,

Cyrille


> Thanks,
> Marcin
> 
>>
>>>>
>>>> Maybe I'm wrong but the assumption (multi-die => chip erase not
>>>> supported) might be false.
>>> It is false, but all multi-die chip I know support die erase or chip erase.
>>> Never both.
>>>
>>> Thanks,
>>> Marcin
>>>
>>>>
>>>>
>>>> Best regards,
>>>>
>>>> Cyrille
>>>>
>>>>
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>> Cyrille
>>>>>>
>>>>>>> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>>>>>>> ---
>>>>>>>  drivers/mtd/spi-nor/spi-nor.c | 90
>>>>>>> +++++++++++++++++++++++++++++++++++++------
>>>>>>>  1 file changed, 78 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c
>>>>>>> b/drivers/mtd/spi-nor/spi-nor.c index 3afe207..17bbec0 100644
>>>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>>>>> @@ -360,6 +360,29 @@ static int spi_nor_erase_sector(struct
>>>>>>> spi_nor
>>>>>> *nor, u32 addr)
>>>>>>>  	return nor->write_reg(nor, nor->erase_opcode, buf, nor-
>>>>>>> addr_width); }
>>>>>>>
>>>>>>> +static inline int spi_nor_sector_at_time_erase(struct mtd_info
>>>>>>> +*mtd,
>>>>>>> +u32 addr, u32 len) {
>>>>>>> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>>>>>>> +	int ret = 0;
>>>>>>> +
>>>>>>> +	while (len) {
>>>>>>> +		write_enable(nor);
>>>>>>> +
>>>>>>> +		ret = spi_nor_erase_sector(nor, addr);
>>>>>>> +		if (ret)
>>>>>>> +			return ret;
>>>>>>> +
>>>>>>> +		addr += mtd->erasesize;
>>>>>>> +		len -= mtd->erasesize;
>>>>>>> +
>>>>>>> +		ret = spi_nor_wait_till_ready(nor);
>>>>>>> +		if (ret)
>>>>>>> +			return ret;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	return ret;
>>>>>>> +}
>>>>>>> +
>>>>>>>  /*
>>>>>>>   * Erase an address range on the nor chip.  The address range may
>>>> extend
>>>>>>>   * one or more erase sectors.  Return an error is there is a
>>>>>>> problem
>>>> erasing.
>>>>>>> @@ -367,9 +390,11 @@ static int spi_nor_erase_sector(struct
>>>>>>> spi_nor *nor, u32 addr)  static int spi_nor_erase(struct mtd_info
>>>>>>> *mtd, struct erase_info *instr)  {
>>>>>>>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>>>>>>> -	u32 addr, len;
>>>>>>> +	u32 addr, len, die_no, die_size;
>>>>>>>  	uint32_t rem;
>>>>>>>  	int ret;
>>>>>>> +	unsigned long timeout;
>>>>>>> +	u8 die_erase = nor->die_cnt && SNOR_F_DIE_ERASE;
>>>>>>>
>>>>>>>  	dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr->addr,
>>>>>>>  			(long long)instr->len);
>>>>>>> @@ -386,7 +411,7 @@ static int spi_nor_erase(struct mtd_info *mtd,
>>>>>> struct erase_info *instr)
>>>>>>>  		return ret;
>>>>>>>
>>>>>>>  	/* whole-chip erase? */
>>>>>>> -	if (len == mtd->size) {
>>>>>>> +	if (len == mtd->size && !die_erase) {
>>>>>>>  		unsigned long timeout;
>>>>>>>
>>>>>>>  		write_enable(nor);
>>>>>>> @@ -416,17 +441,58 @@ static int spi_nor_erase(struct mtd_info
>>>>>>> *mtd, struct erase_info *instr)
>>>>>>>
>>>>>>>  	/* "sector"-at-a-time erase */
>>>>>>>  	} else {
>>>>>>> -		while (len) {
>>>>>>> -			write_enable(nor);
>>>>>>> -
>>>>>>> -			ret = spi_nor_erase_sector(nor, addr);
>>>>>>> -			if (ret)
>>>>>>> +		if (die_erase) {
>>>>>>> +			die_size = div_u64_rem(mtd->size, nor-
>>> die_cnt,
>>>>>> &rem);
>>>>>>> +			if (rem) {
>>>>>>> +				ret = -EINVAL;
>>>>>>>  				goto erase_err;
>>>>>>> -
>>>>>>> -			addr += mtd->erasesize;
>>>>>>> -			len -= mtd->erasesize;
>>>>>>> -
>>>>>>> -			ret = spi_nor_wait_till_ready(nor);
>>>>>>> +			}
>>>>>>> +			while (len) {
>>>>>>> +				die_no = div_u64_rem(addr,
>> die_size,
>>>>>> &rem);
>>>>>>> +
>>>>>>> +				/* Check if address is aligned to die
>> begin*/
>>>>>>> +				if (!rem) {
>>>>>>> +					/* die erase? */
>>>>>>> +					if (len >= die_size) {
>>>>>>> +						ret =
>> spi_nor_die_erase(nor,
>>>>>> addr);
>>>>>>> +						if (ret)
>>>>>>> +							goto
>> erase_err;
>>>>>>> +
>>>>>>> +						len -= die_size;
>>>>>>> +						addr += die_size;
>>>>>>> +
>>>>>>> +						timeout =
>>>>>> max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
>>>>>>> +
>>>>>> 	CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
>>>>>>> +
>> 	(unsigned
>>>>>> long)(die_size / SZ_2M));
>>>>>>> +						ret =
>>>>>> spi_nor_wait_till_ready_with_timeout(nor, timeout);
>>>>>>> +						if (ret)
>>>>>>> +							goto
>> erase_err;
>>>>>>> +					} else {
>>>>>>> +						ret =
>>>>>> spi_nor_sector_at_time_erase(mtd, addr, len);
>>>>>>> +						if (ret)
>>>>>>> +							goto
>> erase_err;
>>>>>>> +						len = 0;
>>>>>>> +					}
>>>>>>> +				} else {
>>>>>>> +					/* check if end address cover
>> at least
>>>>>> one die */
>>>>>>> +					if (div64_ul(addr + len,
>> die_size) >
>>>>>> ++die_no) {
>>>>>>> +						/* align to next die */
>>>>>>> +						rem = die_size - rem;
>>>>>>> +						ret =
>>>>>> spi_nor_sector_at_time_erase(mtd, addr, rem);
>>>>>>> +						if (ret)
>>>>>>> +							goto
>> erase_err;
>>>>>>> +						len -= rem;
>>>>>>> +						addr += rem;
>>>>>>> +					} else {
>>>>>>> +						ret =
>>>>>> spi_nor_sector_at_time_erase(mtd, addr, len);
>>>>>>> +						if (ret)
>>>>>>> +							goto
>> erase_err;
>>>>>>> +						len = 0;
>>>>>>> +					}
>>>>>>> +				}
>>>>>>> +			}
>>>>>>> +		} else {
>>>>>>> +			ret = spi_nor_sector_at_time_erase(mtd,
>> addr, len);
>>>>>>>  			if (ret)
>>>>>>>  				goto erase_err;
>>>>>>>  		}
>>>>>>>
>>>>>
>>>>>
>>>
>>>
> 
> 

RE: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

> -----Original Message-----
> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> Sent: Friday, December 16, 2016 11:52 AM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> computersforpeace@gmail.com; marek.vasut@gmail.com
> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic
> 
> Hi Marcin,
> 
> Le 16/12/2016 à 07:38, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> > Cyrille,
> >
> > Sorry for late answer.
> >
> >> -----Original Message-----
> >> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> >> Sent: Friday, December 09, 2016 6:23 PM
> >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> >> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> >> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> >> computersforpeace@gmail.com; marek.vasut@gmail.com
> >> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command
> >> logic
> >>
> >> Le 30/11/2016 à 19:18, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> >>>> Sent: Wednesday, November 30, 2016 6:20 PM
> >>>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> >>>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> >>>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> >>>> computersforpeace@gmail.com; marek.vasut@gmail.com
> >>>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command
> >>>> logic
> >>>>
> >>>> Le 30/11/2016 à 17:44, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> >>>>>> Sent: Tuesday, November 29, 2016 5:47 PM
> >>>>>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> >>>>>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> >>>>>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> >>>>>> computersforpeace@gmail.com; marek.vasut@gmail.com
> >>>>>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase
> >>>>>> command logic
> >>>>>>
> >>>>>> Hi Marcin,
> >>>>>>
> >>>>>> I know you have already answered some of my questions of IRC but
> >>>>>> I ask them again here on the mailing list so everybody can
> >>>>>> benefit from your answers.
> >>>>>>
> >>>>>> Le 24/10/2016 à 15:03, marcin.krzeminski@nokia.com a écrit :
> >>>>>>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> >>>>>>>
> >>>>>>> This commit implements die erase logic.
> >>>>>>> Sector at a time procedure is moved to function, then erase
> >>>>>>> algorithm will try to use die erase cmd if size and address
> >>>>>>> cover one or more dies.
> >>>>>>>
> >>>>>>
> >>>>>> I'm reading your v2 series but indeed I try to understand the
> >>>>>> purpose of the series simply because I don't know what does die
> >>>>>> erase do as compared to chip erase of sector erase.
> >>>>>> Is you series a bug fix or an optmization, maybe both?
> >>>>>
> >>>>> In current spi-nor framework when user want to erase whole flash
> (eg.
> >>>>> mtd_debug erase /dev/mtdX 0 0x8000000) framework issue chip
> erase
> >>>> cmd.
> >>>>> N25Q00 does not support chip erase, instead there was introduced
> >>>>> die erase command that can erase whole die (N25Q00 is a mulit-die
> >>>>> NOR
> >> chip).
> >>>>> So for this case this patch is a bug fix.
> >>>>> Since I have die erase command implemented it was tempting to use
> >>>>> die erase also in case where erase area cover one or more dies to
> >>>>> speed up erase process, especially when user has enabled 4KiB
> >>>>> sector. That is a small
> >>>> optimization.
> >>>>>
> >>>>> I will rewrite commit message and add those informations.
> >>>>>
> >>>>> Thanks,
> >>>>> Marcin
> >>>>>
> >>>>
> >>>> OK, then I think your patch should be split into two different patches.
> >>>>
> >>>> 1 - bug fix:
> >>>>
> >>>> The first patch introducing an explicit flag such as
> >>>> SPI_NOR_NO_CHIP_ERASE, which would be set in the relevant entries
> >>>> of the spi_nor_ids[] (see already existing flags such as
> >>>> SPI_NOR_NO_FR,
> >> SPI_NOR_NO_ERASE, ...).
> >>>>
> >>>> In spi_nor_scan():
> >>>>
> >>>>  	if (info->flags & USE_FSR)
> >>>>  		nor->flags |= SNOR_F_USE_FSR;
> >>>>  	if (info->flags & SPI_NOR_HAS_TB)
> >>>>  		nor->flags |= SNOR_F_HAS_SR_TB;
> >>>> +	if (info->flags & SPI_NOR_NO_CHIP_ERASE)
> >>>> +		nor->flags |= SNOR_F_NO_CHIP_ERASE;
> >>>>
> >>>> Then in spi_nor_erase():
> >>>>
> >>>>  	/* whole-chip erase? */
> >>>> -	if (len == mtd->size) {
> >>>> +	if (len == mtd->size && !(nor->flags & SNOR_F_NO_CHIP_ERASE)) {
> >>>>  		unsigned long timeout;
> >>>>
> >>>> So you fall into the sector erase case, no optimization yet.
> >>>>
> >>>> 2 - optimization of spi_nor_erase()
> >>>>
> >>>> In the "sector"-at-a-time erase branch, try to optimize when
> >>>> possible using die erase instead of sector erase.
> >>>>
> >>>> Each commit message describing the purpose of the patch.
> >>>
> >>>
> >>> OK, I will split patches in this way.
> >>>
> >>>>
> >>>> I would like to also handle the cases where the memory supports:
> >>>> - both chip and die erases: if so, no reason not to use chip erase on
> such
> >>>>   multi-die memory.
> >>> I do not think that there are chips supporting both.
> >>> In this case two flags are needed one for disable chip erase and
> >>> another one for enabling die erase. Is that OK?
> >>
> >> I think it might be better to have 2 separated flags, at least for
> >> semantic
> >> reason: "the memory supports Die-Erase hence we don't try to use
> >> Chip- Erase" sounds a little bit odd to me.
> >
> > Let's do it with two flags then.
> >
> >>
> >> This is what I understand when I read:
> >>  	/* whole-chip erase? */
> >> -	if (len == mtd->size) {
> >> +	if (len == mtd->size && !die_erase) {
> >>
> >> Why do we try Chip-Erase only if Die-Erase is not supported?
> >
> > I assumed that there is no flash that support both.
> > Do you know one that has chip erase and die erase?
> >
> 
> No, I don't but I didn't focus on erase operations so maybe some already
> exist or might exit in the future, who knows? :)
> 
> Also, please have a look at Ricardo's patch:
> [v9] mtd: spi-nor: Add supported for S3AN spi-nor devices
> http://patchwork.ozlabs.org/patch/701935/
> 
> This patch has recently been merged into git://github.com/spi-nor/linux.git
> so you will have to rebase your work on that tree.

Thanks for information, so git://github.com/spi-nor/linux.git should be used 
As base to send spi-nor patches?
> 
> Ricardo's patch already introduces a SNOR_F_NO_OP_CHIP_ERASE flag used
> from spi_nor_erase(). The only missing part is to set this flag in nor->flag
> when another dedicated flag is set in info->flags.
> Currently this SNOR_F_NO_OP_CHIP_ERASE is only set from
> s3an_nor_scan() but it should be also set from spi_nor_scan() for the
> relevant memories.

Ok.
> 
> >>
> >> Reading something like "we try Chip-Erase unless Chip-Erase is not
> >> supported" is more logic, isn't it?
> >>
> >> I want to avoid the implicit assumption "Die Erase supported => Chip
> >> Erase not supported", even if it *might* be true, because it is
> >> strange when we read the code.
> >
> > Ok.
> >>
> >>>> - neither chip nor die erases.
> >>> This is supported DIE_ERASE flasg set and die_cnt == 0
> >>
> >> DIE_ERASE flag set and die_cnt == 0 meaning Chip-Erase is not supported?
> >> IHMO, semantically, it's not logical.
> >
> > Just to save yet another flag. Having so many flags make codes
> > complicated, but I agree the it I at least logical.
> >>
> >> Also discussing another topic on #mtd, it seems that some memories
> >> don't have a 4-byte address version of the Die Erase op code.
> >
> > Yes. Could be workarounded by enabling 4byte address mode just before
> > sending die erase and then disable it back.
> > The more important question is if you really one to have in upstream die
> erase?
> > It is about 30% faster than  sector at time erase on some flash
> > (especially in case 4KiB sectors). From the other hand I see chips
> > that it was not faster at all (it also applies on chip erase).
> > My first version of fixing this problem was to add only the flag of disable
> chip erase.
> > See here:
> > http://lists.infradead.org/pipermail/linux-mtd/2016-October/069888.htm
> > l Maybe for now it is better to take above?
> >
> >>
> >> If so, it's preferred to use a 4-byte address Sector/Block Erase op
> >> code than to enter/exit the statefull 4-byte address mode then use
> >> the Die Erase op
> >> code: this statefull mode should be avoided as much as possible
> >> because many bootloaders expect the SPI memory to be in 3byte address
> >> mode and if a spurious reset occurs on the CPU side (but not on the
> >> memory side) those bootloaders will fail to read data from the SPI
> memory.
> >
> > Yeap, moreover similar problem you will face when you will try to add
> > QPi modes (444) to framework.
> >
> 
> Entering the 4-byte address mode might be accepted as a workaround to
> allow using the Die Erase op code even on > 16MiB memory as long as the
> user can choose to disable this feature and the feature is not enabled by
> default.
> This way we don't prevent end users from using Die Erase if they want but
> we don't force them to use it if they don't want to.
> 
> Indeed, Die Erase is an optimization but it is not mandatory to erase the
> memory: the slower Sector/Block Erase commands can still be used.
> However entering the 4-byte address mode to allow using the Die Erase
> command has side effects, as explained above, that might introduce
> regression.
> 
> Let's imagine the case of a 2-die memory used to store 2 firmware images.
> The firmware update process could alternate between dies when writing a
> new firmware image so the current/older firmware image is still available if
> the update process fails to write the new image.

At least for Micron it is possible to do such sequence:
1. Enter 4 byte address mode.
2. Send die erase.
3. Exit 4 byte address mode.
4. Start to poll if erase has been done.

Anyway this prove that upstreaming die erase impl. Is not good idea for now.
Let's wait for your JESD218 work to be merged.

> 
> Then, if you make the whole memory enter its 4-byte address mode to erase
> one single die and a spurious reboot occurs, many bootloaders would fail to
> read the old firmware from the other die just because the whole memory
> expects 4-byte addresses.
> 
> About the QPI mode (SPI 4-4-4), I agree with you, it has the very same
> annoying side effect. Hence I've removed support of the SPI 2-2-2 and SPI
> 4-4-4 protocols at the memory side from my SFDP series for the moment.

And that's remained me I have a comment about that :)

Thanks,
Marcin

> Besides, on the controller side, by changing the 3rd argument parameter of
> spi_nor_scan(), one of my patch allows the SPI controller driver to claim "I
> can't/don't want to use SPI 4-4-4 but I support SPI 1-1-4 and SPI 1-4-4
> protocols". So the SPI 4-4-4 protocol can be disabled if needed.
> 
> Best regards,
> 
> Cyrille
> 
> 
> > Thanks,
> > Marcin
> >
> >>
> >>>>
> >>>> Maybe I'm wrong but the assumption (multi-die => chip erase not
> >>>> supported) might be false.
> >>> It is false, but all multi-die chip I know support die erase or chip erase.
> >>> Never both.
> >>>
> >>> Thanks,
> >>> Marcin
> >>>
> >>>>
> >>>>
> >>>> Best regards,
> >>>>
> >>>> Cyrille
> >>>>
> >>>>
> >>>>>>
> >>>>>> Best regards,
> >>>>>>
> >>>>>> Cyrille
> >>>>>>
> >>>>>>> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> >>>>>>> ---
> >>>>>>>  drivers/mtd/spi-nor/spi-nor.c | 90
> >>>>>>> +++++++++++++++++++++++++++++++++++++------
> >>>>>>>  1 file changed, 78 insertions(+), 12 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c
> >>>>>>> b/drivers/mtd/spi-nor/spi-nor.c index 3afe207..17bbec0 100644
> >>>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
> >>>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >>>>>>> @@ -360,6 +360,29 @@ static int spi_nor_erase_sector(struct
> >>>>>>> spi_nor
> >>>>>> *nor, u32 addr)
> >>>>>>>  	return nor->write_reg(nor, nor->erase_opcode, buf, nor-
> >>>>>>> addr_width); }
> >>>>>>>
> >>>>>>> +static inline int spi_nor_sector_at_time_erase(struct mtd_info
> >>>>>>> +*mtd,
> >>>>>>> +u32 addr, u32 len) {
> >>>>>>> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> >>>>>>> +	int ret = 0;
> >>>>>>> +
> >>>>>>> +	while (len) {
> >>>>>>> +		write_enable(nor);
> >>>>>>> +
> >>>>>>> +		ret = spi_nor_erase_sector(nor, addr);
> >>>>>>> +		if (ret)
> >>>>>>> +			return ret;
> >>>>>>> +
> >>>>>>> +		addr += mtd->erasesize;
> >>>>>>> +		len -= mtd->erasesize;
> >>>>>>> +
> >>>>>>> +		ret = spi_nor_wait_till_ready(nor);
> >>>>>>> +		if (ret)
> >>>>>>> +			return ret;
> >>>>>>> +	}
> >>>>>>> +
> >>>>>>> +	return ret;
> >>>>>>> +}
> >>>>>>> +
> >>>>>>>  /*
> >>>>>>>   * Erase an address range on the nor chip.  The address range
> >>>>>>> may
> >>>> extend
> >>>>>>>   * one or more erase sectors.  Return an error is there is a
> >>>>>>> problem
> >>>> erasing.
> >>>>>>> @@ -367,9 +390,11 @@ static int spi_nor_erase_sector(struct
> >>>>>>> spi_nor *nor, u32 addr)  static int spi_nor_erase(struct
> >>>>>>> mtd_info *mtd, struct erase_info *instr)  {
> >>>>>>>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> >>>>>>> -	u32 addr, len;
> >>>>>>> +	u32 addr, len, die_no, die_size;
> >>>>>>>  	uint32_t rem;
> >>>>>>>  	int ret;
> >>>>>>> +	unsigned long timeout;
> >>>>>>> +	u8 die_erase = nor->die_cnt && SNOR_F_DIE_ERASE;
> >>>>>>>
> >>>>>>>  	dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr-
> >addr,
> >>>>>>>  			(long long)instr->len);
> >>>>>>> @@ -386,7 +411,7 @@ static int spi_nor_erase(struct mtd_info
> >>>>>>> *mtd,
> >>>>>> struct erase_info *instr)
> >>>>>>>  		return ret;
> >>>>>>>
> >>>>>>>  	/* whole-chip erase? */
> >>>>>>> -	if (len == mtd->size) {
> >>>>>>> +	if (len == mtd->size && !die_erase) {
> >>>>>>>  		unsigned long timeout;
> >>>>>>>
> >>>>>>>  		write_enable(nor);
> >>>>>>> @@ -416,17 +441,58 @@ static int spi_nor_erase(struct mtd_info
> >>>>>>> *mtd, struct erase_info *instr)
> >>>>>>>
> >>>>>>>  	/* "sector"-at-a-time erase */
> >>>>>>>  	} else {
> >>>>>>> -		while (len) {
> >>>>>>> -			write_enable(nor);
> >>>>>>> -
> >>>>>>> -			ret = spi_nor_erase_sector(nor, addr);
> >>>>>>> -			if (ret)
> >>>>>>> +		if (die_erase) {
> >>>>>>> +			die_size = div_u64_rem(mtd->size, nor-
> >>> die_cnt,
> >>>>>> &rem);
> >>>>>>> +			if (rem) {
> >>>>>>> +				ret = -EINVAL;
> >>>>>>>  				goto erase_err;
> >>>>>>> -
> >>>>>>> -			addr += mtd->erasesize;
> >>>>>>> -			len -= mtd->erasesize;
> >>>>>>> -
> >>>>>>> -			ret = spi_nor_wait_till_ready(nor);
> >>>>>>> +			}
> >>>>>>> +			while (len) {
> >>>>>>> +				die_no = div_u64_rem(addr,
> >> die_size,
> >>>>>> &rem);
> >>>>>>> +
> >>>>>>> +				/* Check if address is aligned to die
> >> begin*/
> >>>>>>> +				if (!rem) {
> >>>>>>> +					/* die erase? */
> >>>>>>> +					if (len >= die_size) {
> >>>>>>> +						ret =
> >> spi_nor_die_erase(nor,
> >>>>>> addr);
> >>>>>>> +						if (ret)
> >>>>>>> +							goto
> >> erase_err;
> >>>>>>> +
> >>>>>>> +						len -= die_size;
> >>>>>>> +						addr += die_size;
> >>>>>>> +
> >>>>>>> +						timeout =
> >>>>>> max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
> >>>>>>> +
> >>>>>> 	CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
> >>>>>>> +
> >> 	(unsigned
> >>>>>> long)(die_size / SZ_2M));
> >>>>>>> +						ret =
> >>>>>> spi_nor_wait_till_ready_with_timeout(nor, timeout);
> >>>>>>> +						if (ret)
> >>>>>>> +							goto
> >> erase_err;
> >>>>>>> +					} else {
> >>>>>>> +						ret =
> >>>>>> spi_nor_sector_at_time_erase(mtd, addr, len);
> >>>>>>> +						if (ret)
> >>>>>>> +							goto
> >> erase_err;
> >>>>>>> +						len = 0;
> >>>>>>> +					}
> >>>>>>> +				} else {
> >>>>>>> +					/* check if end address cover
> >> at least
> >>>>>> one die */
> >>>>>>> +					if (div64_ul(addr + len,
> >> die_size) >
> >>>>>> ++die_no) {
> >>>>>>> +						/* align to next die */
> >>>>>>> +						rem = die_size - rem;
> >>>>>>> +						ret =
> >>>>>> spi_nor_sector_at_time_erase(mtd, addr, rem);
> >>>>>>> +						if (ret)
> >>>>>>> +							goto
> >> erase_err;
> >>>>>>> +						len -= rem;
> >>>>>>> +						addr += rem;
> >>>>>>> +					} else {
> >>>>>>> +						ret =
> >>>>>> spi_nor_sector_at_time_erase(mtd, addr, len);
> >>>>>>> +						if (ret)
> >>>>>>> +							goto
> >> erase_err;
> >>>>>>> +						len = 0;
> >>>>>>> +					}
> >>>>>>> +				}
> >>>>>>> +			}
> >>>>>>> +		} else {
> >>>>>>> +			ret = spi_nor_sector_at_time_erase(mtd,
> >> addr, len);
> >>>>>>>  			if (ret)
> >>>>>>>  				goto erase_err;
> >>>>>>>  		}
> >>>>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >
> >

Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic

[Cyrille Pitchen]

Le 16/12/2016 à 14:36, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> 
> 
>> -----Original Message-----
>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
>> Sent: Friday, December 16, 2016 11:52 AM
>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
>> computersforpeace@gmail.com; marek.vasut@gmail.com
>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic
>>
>> Hi Marcin,
>>
>> Le 16/12/2016 à 07:38, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
>>> Cyrille,
>>>
>>> Sorry for late answer.
>>>
>>>> -----Original Message-----
>>>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
>>>> Sent: Friday, December 09, 2016 6:23 PM
>>>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
>>>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
>>>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
>>>> computersforpeace@gmail.com; marek.vasut@gmail.com
>>>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command
>>>> logic
>>>>
>>>> Le 30/11/2016 à 19:18, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
>>>>>> Sent: Wednesday, November 30, 2016 6:20 PM
>>>>>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
>>>>>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
>>>>>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
>>>>>> computersforpeace@gmail.com; marek.vasut@gmail.com
>>>>>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command
>>>>>> logic
>>>>>>
>>>>>> Le 30/11/2016 à 17:44, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
>>>>>>>> Sent: Tuesday, November 29, 2016 5:47 PM
>>>>>>>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
>>>>>>>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
>>>>>>>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
>>>>>>>> computersforpeace@gmail.com; marek.vasut@gmail.com
>>>>>>>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase
>>>>>>>> command logic
>>>>>>>>
>>>>>>>> Hi Marcin,
>>>>>>>>
>>>>>>>> I know you have already answered some of my questions of IRC but
>>>>>>>> I ask them again here on the mailing list so everybody can
>>>>>>>> benefit from your answers.
>>>>>>>>
>>>>>>>> Le 24/10/2016 à 15:03, marcin.krzeminski@nokia.com a écrit :
>>>>>>>>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>>>>>>>>>
>>>>>>>>> This commit implements die erase logic.
>>>>>>>>> Sector at a time procedure is moved to function, then erase
>>>>>>>>> algorithm will try to use die erase cmd if size and address
>>>>>>>>> cover one or more dies.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I'm reading your v2 series but indeed I try to understand the
>>>>>>>> purpose of the series simply because I don't know what does die
>>>>>>>> erase do as compared to chip erase of sector erase.
>>>>>>>> Is you series a bug fix or an optmization, maybe both?
>>>>>>>
>>>>>>> In current spi-nor framework when user want to erase whole flash
>> (eg.
>>>>>>> mtd_debug erase /dev/mtdX 0 0x8000000) framework issue chip
>> erase
>>>>>> cmd.
>>>>>>> N25Q00 does not support chip erase, instead there was introduced
>>>>>>> die erase command that can erase whole die (N25Q00 is a mulit-die
>>>>>>> NOR
>>>> chip).
>>>>>>> So for this case this patch is a bug fix.
>>>>>>> Since I have die erase command implemented it was tempting to use
>>>>>>> die erase also in case where erase area cover one or more dies to
>>>>>>> speed up erase process, especially when user has enabled 4KiB
>>>>>>> sector. That is a small
>>>>>> optimization.
>>>>>>>
>>>>>>> I will rewrite commit message and add those informations.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Marcin
>>>>>>>
>>>>>>
>>>>>> OK, then I think your patch should be split into two different patches.
>>>>>>
>>>>>> 1 - bug fix:
>>>>>>
>>>>>> The first patch introducing an explicit flag such as
>>>>>> SPI_NOR_NO_CHIP_ERASE, which would be set in the relevant entries
>>>>>> of the spi_nor_ids[] (see already existing flags such as
>>>>>> SPI_NOR_NO_FR,
>>>> SPI_NOR_NO_ERASE, ...).
>>>>>>
>>>>>> In spi_nor_scan():
>>>>>>
>>>>>>  	if (info->flags & USE_FSR)
>>>>>>  		nor->flags |= SNOR_F_USE_FSR;
>>>>>>  	if (info->flags & SPI_NOR_HAS_TB)
>>>>>>  		nor->flags |= SNOR_F_HAS_SR_TB;
>>>>>> +	if (info->flags & SPI_NOR_NO_CHIP_ERASE)
>>>>>> +		nor->flags |= SNOR_F_NO_CHIP_ERASE;
>>>>>>
>>>>>> Then in spi_nor_erase():
>>>>>>
>>>>>>  	/* whole-chip erase? */
>>>>>> -	if (len == mtd->size) {
>>>>>> +	if (len == mtd->size && !(nor->flags & SNOR_F_NO_CHIP_ERASE)) {
>>>>>>  		unsigned long timeout;
>>>>>>
>>>>>> So you fall into the sector erase case, no optimization yet.
>>>>>>
>>>>>> 2 - optimization of spi_nor_erase()
>>>>>>
>>>>>> In the "sector"-at-a-time erase branch, try to optimize when
>>>>>> possible using die erase instead of sector erase.
>>>>>>
>>>>>> Each commit message describing the purpose of the patch.
>>>>>
>>>>>
>>>>> OK, I will split patches in this way.
>>>>>
>>>>>>
>>>>>> I would like to also handle the cases where the memory supports:
>>>>>> - both chip and die erases: if so, no reason not to use chip erase on
>> such
>>>>>>   multi-die memory.
>>>>> I do not think that there are chips supporting both.
>>>>> In this case two flags are needed one for disable chip erase and
>>>>> another one for enabling die erase. Is that OK?
>>>>
>>>> I think it might be better to have 2 separated flags, at least for
>>>> semantic
>>>> reason: "the memory supports Die-Erase hence we don't try to use
>>>> Chip- Erase" sounds a little bit odd to me.
>>>
>>> Let's do it with two flags then.
>>>
>>>>
>>>> This is what I understand when I read:
>>>>  	/* whole-chip erase? */
>>>> -	if (len == mtd->size) {
>>>> +	if (len == mtd->size && !die_erase) {
>>>>
>>>> Why do we try Chip-Erase only if Die-Erase is not supported?
>>>
>>> I assumed that there is no flash that support both.
>>> Do you know one that has chip erase and die erase?
>>>
>>
>> No, I don't but I didn't focus on erase operations so maybe some already
>> exist or might exit in the future, who knows? :)
>>
>> Also, please have a look at Ricardo's patch:
>> [v9] mtd: spi-nor: Add supported for S3AN spi-nor devices
>> http://patchwork.ozlabs.org/patch/701935/
>>
>> This patch has recently been merged into git://github.com/spi-nor/linux.git
>> so you will have to rebase your work on that tree.
> 
> Thanks for information, so git://github.com/spi-nor/linux.git should be used 
> As base to send spi-nor patches?

Indeed yes, as described in MAINTAINERS this is currently the official git
tree for the spi-nor subsystem. However, we will move soon to infradead.org
and so the nand subsystem is likely to do. Hence all MTD trees will be
hosted by infradead.org.

>>
>> Ricardo's patch already introduces a SNOR_F_NO_OP_CHIP_ERASE flag used
>> from spi_nor_erase(). The only missing part is to set this flag in nor->flag
>> when another dedicated flag is set in info->flags.
>> Currently this SNOR_F_NO_OP_CHIP_ERASE is only set from
>> s3an_nor_scan() but it should be also set from spi_nor_scan() for the
>> relevant memories.
> 
> Ok.
>>
>>>>
>>>> Reading something like "we try Chip-Erase unless Chip-Erase is not
>>>> supported" is more logic, isn't it?
>>>>
>>>> I want to avoid the implicit assumption "Die Erase supported => Chip
>>>> Erase not supported", even if it *might* be true, because it is
>>>> strange when we read the code.
>>>
>>> Ok.
>>>>
>>>>>> - neither chip nor die erases.
>>>>> This is supported DIE_ERASE flasg set and die_cnt == 0
>>>>
>>>> DIE_ERASE flag set and die_cnt == 0 meaning Chip-Erase is not supported?
>>>> IHMO, semantically, it's not logical.
>>>
>>> Just to save yet another flag. Having so many flags make codes
>>> complicated, but I agree the it I at least logical.
>>>>
>>>> Also discussing another topic on #mtd, it seems that some memories
>>>> don't have a 4-byte address version of the Die Erase op code.
>>>
>>> Yes. Could be workarounded by enabling 4byte address mode just before
>>> sending die erase and then disable it back.
>>> The more important question is if you really one to have in upstream die
>> erase?
>>> It is about 30% faster than  sector at time erase on some flash
>>> (especially in case 4KiB sectors). From the other hand I see chips
>>> that it was not faster at all (it also applies on chip erase).
>>> My first version of fixing this problem was to add only the flag of disable
>> chip erase.
>>> See here:
>>> http://lists.infradead.org/pipermail/linux-mtd/2016-October/069888.htm
>>> l Maybe for now it is better to take above?
>>>
>>>>
>>>> If so, it's preferred to use a 4-byte address Sector/Block Erase op
>>>> code than to enter/exit the statefull 4-byte address mode then use
>>>> the Die Erase op
>>>> code: this statefull mode should be avoided as much as possible
>>>> because many bootloaders expect the SPI memory to be in 3byte address
>>>> mode and if a spurious reset occurs on the CPU side (but not on the
>>>> memory side) those bootloaders will fail to read data from the SPI
>> memory.
>>>
>>> Yeap, moreover similar problem you will face when you will try to add
>>> QPi modes (444) to framework.
>>>
>>
>> Entering the 4-byte address mode might be accepted as a workaround to
>> allow using the Die Erase op code even on > 16MiB memory as long as the
>> user can choose to disable this feature and the feature is not enabled by
>> default.
>> This way we don't prevent end users from using Die Erase if they want but
>> we don't force them to use it if they don't want to.
>>
>> Indeed, Die Erase is an optimization but it is not mandatory to erase the
>> memory: the slower Sector/Block Erase commands can still be used.
>> However entering the 4-byte address mode to allow using the Die Erase
>> command has side effects, as explained above, that might introduce
>> regression.
>>
>> Let's imagine the case of a 2-die memory used to store 2 firmware images.
>> The firmware update process could alternate between dies when writing a
>> new firmware image so the current/older firmware image is still available if
>> the update process fails to write the new image.
> 
> At least for Micron it is possible to do such sequence:
> 1. Enter 4 byte address mode.
> 2. Send die erase.
> 3. Exit 4 byte address mode.
> 4. Start to poll if erase has been done.
>

If a spurious CPU reset occurs between the end of step 1 and the beginning
of step 3, the whole memory would be stuck in its 4byte address mode and
the bootloader would fail to read data. This is precisely what some
products want to avoid.

> Anyway this prove that upstreaming die erase impl. Is not good idea for now.
> Let's wait for your JESD218 work to be merged.
>

Your use case is interesting, I don't say we should not add it, I just say
a mechanism is needed to disable the feature and let it disabled by
default. On some products, it might be risky hence unacceptable to enter
the 4 byte mode but on other products it could be safe then the
optimization you propose is interesting.


>>
>> Then, if you make the whole memory enter its 4-byte address mode to erase
>> one single die and a spurious reboot occurs, many bootloaders would fail to
>> read the old firmware from the other die just because the whole memory
>> expects 4-byte addresses.
>>
>> About the QPI mode (SPI 4-4-4), I agree with you, it has the very same
>> annoying side effect. Hence I've removed support of the SPI 2-2-2 and SPI
>> 4-4-4 protocols at the memory side from my SFDP series for the moment.
> 
> And that's remained me I have a comment about that :)
> 
> Thanks,
> Marcin
> 
>> Besides, on the controller side, by changing the 3rd argument parameter of
>> spi_nor_scan(), one of my patch allows the SPI controller driver to claim "I
>> can't/don't want to use SPI 4-4-4 but I support SPI 1-1-4 and SPI 1-4-4
>> protocols". So the SPI 4-4-4 protocol can be disabled if needed.
>>
>> Best regards,
>>
>> Cyrille
>>
>>
>>> Thanks,
>>> Marcin
>>>
>>>>
>>>>>>
>>>>>> Maybe I'm wrong but the assumption (multi-die => chip erase not
>>>>>> supported) might be false.
>>>>> It is false, but all multi-die chip I know support die erase or chip erase.
>>>>> Never both.
>>>>>
>>>>> Thanks,
>>>>> Marcin
>>>>>
>>>>>>
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>> Cyrille
>>>>>>
>>>>>>
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>>
>>>>>>>> Cyrille
>>>>>>>>
>>>>>>>>> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>>>>>>>>> ---
>>>>>>>>>  drivers/mtd/spi-nor/spi-nor.c | 90
>>>>>>>>> +++++++++++++++++++++++++++++++++++++------
>>>>>>>>>  1 file changed, 78 insertions(+), 12 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c
>>>>>>>>> b/drivers/mtd/spi-nor/spi-nor.c index 3afe207..17bbec0 100644
>>>>>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>>>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>>>>>>> @@ -360,6 +360,29 @@ static int spi_nor_erase_sector(struct
>>>>>>>>> spi_nor
>>>>>>>> *nor, u32 addr)
>>>>>>>>>  	return nor->write_reg(nor, nor->erase_opcode, buf, nor-
>>>>>>>>> addr_width); }
>>>>>>>>>
>>>>>>>>> +static inline int spi_nor_sector_at_time_erase(struct mtd_info
>>>>>>>>> +*mtd,
>>>>>>>>> +u32 addr, u32 len) {
>>>>>>>>> +	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>>>>>>>>> +	int ret = 0;
>>>>>>>>> +
>>>>>>>>> +	while (len) {
>>>>>>>>> +		write_enable(nor);
>>>>>>>>> +
>>>>>>>>> +		ret = spi_nor_erase_sector(nor, addr);
>>>>>>>>> +		if (ret)
>>>>>>>>> +			return ret;
>>>>>>>>> +
>>>>>>>>> +		addr += mtd->erasesize;
>>>>>>>>> +		len -= mtd->erasesize;
>>>>>>>>> +
>>>>>>>>> +		ret = spi_nor_wait_till_ready(nor);
>>>>>>>>> +		if (ret)
>>>>>>>>> +			return ret;
>>>>>>>>> +	}
>>>>>>>>> +
>>>>>>>>> +	return ret;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>  /*
>>>>>>>>>   * Erase an address range on the nor chip.  The address range
>>>>>>>>> may
>>>>>> extend
>>>>>>>>>   * one or more erase sectors.  Return an error is there is a
>>>>>>>>> problem
>>>>>> erasing.
>>>>>>>>> @@ -367,9 +390,11 @@ static int spi_nor_erase_sector(struct
>>>>>>>>> spi_nor *nor, u32 addr)  static int spi_nor_erase(struct
>>>>>>>>> mtd_info *mtd, struct erase_info *instr)  {
>>>>>>>>>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>>>>>>>>> -	u32 addr, len;
>>>>>>>>> +	u32 addr, len, die_no, die_size;
>>>>>>>>>  	uint32_t rem;
>>>>>>>>>  	int ret;
>>>>>>>>> +	unsigned long timeout;
>>>>>>>>> +	u8 die_erase = nor->die_cnt && SNOR_F_DIE_ERASE;
>>>>>>>>>
>>>>>>>>>  	dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr-
>>> addr,
>>>>>>>>>  			(long long)instr->len);
>>>>>>>>> @@ -386,7 +411,7 @@ static int spi_nor_erase(struct mtd_info
>>>>>>>>> *mtd,
>>>>>>>> struct erase_info *instr)
>>>>>>>>>  		return ret;
>>>>>>>>>
>>>>>>>>>  	/* whole-chip erase? */
>>>>>>>>> -	if (len == mtd->size) {
>>>>>>>>> +	if (len == mtd->size && !die_erase) {
>>>>>>>>>  		unsigned long timeout;
>>>>>>>>>
>>>>>>>>>  		write_enable(nor);
>>>>>>>>> @@ -416,17 +441,58 @@ static int spi_nor_erase(struct mtd_info
>>>>>>>>> *mtd, struct erase_info *instr)
>>>>>>>>>
>>>>>>>>>  	/* "sector"-at-a-time erase */
>>>>>>>>>  	} else {
>>>>>>>>> -		while (len) {
>>>>>>>>> -			write_enable(nor);
>>>>>>>>> -
>>>>>>>>> -			ret = spi_nor_erase_sector(nor, addr);
>>>>>>>>> -			if (ret)
>>>>>>>>> +		if (die_erase) {
>>>>>>>>> +			die_size = div_u64_rem(mtd->size, nor-
>>>>> die_cnt,
>>>>>>>> &rem);
>>>>>>>>> +			if (rem) {
>>>>>>>>> +				ret = -EINVAL;
>>>>>>>>>  				goto erase_err;
>>>>>>>>> -
>>>>>>>>> -			addr += mtd->erasesize;
>>>>>>>>> -			len -= mtd->erasesize;
>>>>>>>>> -
>>>>>>>>> -			ret = spi_nor_wait_till_ready(nor);
>>>>>>>>> +			}
>>>>>>>>> +			while (len) {
>>>>>>>>> +				die_no = div_u64_rem(addr,
>>>> die_size,
>>>>>>>> &rem);
>>>>>>>>> +
>>>>>>>>> +				/* Check if address is aligned to die
>>>> begin*/
>>>>>>>>> +				if (!rem) {
>>>>>>>>> +					/* die erase? */
>>>>>>>>> +					if (len >= die_size) {
>>>>>>>>> +						ret =
>>>> spi_nor_die_erase(nor,
>>>>>>>> addr);
>>>>>>>>> +						if (ret)
>>>>>>>>> +							goto
>>>> erase_err;
>>>>>>>>> +
>>>>>>>>> +						len -= die_size;
>>>>>>>>> +						addr += die_size;
>>>>>>>>> +
>>>>>>>>> +						timeout =
>>>>>>>> max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
>>>>>>>>> +
>>>>>>>> 	CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
>>>>>>>>> +
>>>> 	(unsigned
>>>>>>>> long)(die_size / SZ_2M));
>>>>>>>>> +						ret =
>>>>>>>> spi_nor_wait_till_ready_with_timeout(nor, timeout);
>>>>>>>>> +						if (ret)
>>>>>>>>> +							goto
>>>> erase_err;
>>>>>>>>> +					} else {
>>>>>>>>> +						ret =
>>>>>>>> spi_nor_sector_at_time_erase(mtd, addr, len);
>>>>>>>>> +						if (ret)
>>>>>>>>> +							goto
>>>> erase_err;
>>>>>>>>> +						len = 0;
>>>>>>>>> +					}
>>>>>>>>> +				} else {
>>>>>>>>> +					/* check if end address cover
>>>> at least
>>>>>>>> one die */
>>>>>>>>> +					if (div64_ul(addr + len,
>>>> die_size) >
>>>>>>>> ++die_no) {
>>>>>>>>> +						/* align to next die */
>>>>>>>>> +						rem = die_size - rem;
>>>>>>>>> +						ret =
>>>>>>>> spi_nor_sector_at_time_erase(mtd, addr, rem);
>>>>>>>>> +						if (ret)
>>>>>>>>> +							goto
>>>> erase_err;
>>>>>>>>> +						len -= rem;
>>>>>>>>> +						addr += rem;
>>>>>>>>> +					} else {
>>>>>>>>> +						ret =
>>>>>>>> spi_nor_sector_at_time_erase(mtd, addr, len);
>>>>>>>>> +						if (ret)
>>>>>>>>> +							goto
>>>> erase_err;
>>>>>>>>> +						len = 0;
>>>>>>>>> +					}
>>>>>>>>> +				}
>>>>>>>>> +			}
>>>>>>>>> +		} else {
>>>>>>>>> +			ret = spi_nor_sector_at_time_erase(mtd,
>>>> addr, len);
>>>>>>>>>  			if (ret)
>>>>>>>>>  				goto erase_err;
>>>>>>>>>  		}
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
> 
> 

Odp.: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

>  
> Le 16/12/2016 à 14:36, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> > 
> > 
> >> -----Original Message-----
> >> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> >> Sent: Friday, December 16, 2016 11:52 AM
> >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> >> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> >> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> >> computersforpeace@gmail.com; marek.vasut@gmail.com
> >> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command logic
> >>
> >> Hi Marcin,
> >>
> >> Le 16/12/2016 à 07:38, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> >>> Cyrille,
> >>>
> >>> Sorry for late answer.
> >>>
> >>>> -----Original Message-----
> >>>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> >>>> Sent: Friday, December 09, 2016 6:23 PM
> >>>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> >>>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> >>>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> >>>> computersforpeace@gmail.com; marek.vasut@gmail.com
> >>>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command
> >>>> logic
> >>>>
> >>>> Le 30/11/2016 à 19:18, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> >>>>>> Sent: Wednesday, November 30, 2016 6:20 PM
> >>>>>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> >>>>>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> >>>>>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> >>>>>> computersforpeace@gmail.com; marek.vasut@gmail.com
> >>>>>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase command
> >>>>>> logic
> >>>>>>
> >>>>>> Le 30/11/2016 à 17:44, Krzeminski, Marcin (Nokia - PL/Wroclaw) a écrit :
> >>>>>>>
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Cyrille Pitchen [mailto:cyrille.pitchen@atmel.com]
> >>>>>>>> Sent: Tuesday, November 29, 2016 5:47 PM
> >>>>>>>> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> >>>>>>>> <marcin.krzeminski@nokia.com>; linux-mtd@lists.infradead.org
> >>>>>>>> Cc: rfsw-patches@mlist.nokia.com; dwmw2@infradead.org;
> >>>>>>>> computersforpeace@gmail.com; marek.vasut@gmail.com
> >>>>>>>> Subject: Re: [PATCH 2/3] mtd: spi-nor: Implement die erase
> >>>>>>>> command logic
> >>>>>>>>
> >>>>>>>> Hi Marcin,
> >>>>>>>>
> >>>>>>>> I know you have already answered some of my questions of IRC but
> >>>>>>>> I ask them again here on the mailing list so everybody can
> >>>>>>>> benefit from your answers.
> >>>>>>>>
> >>>>>>>> Le 24/10/2016 à 15:03, marcin.krzeminski@nokia.com a écrit :
> >>>>>>>>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> >>>>>>>>>
> >>>>>>>>> This commit implements die erase logic.
> >>>>>>>>> Sector at a time procedure is moved to function, then erase
> >>>>>>>>> algorithm will try to use die erase cmd if size and address
> >>>>>>>>> cover one or more dies.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> I'm reading your v2 series but indeed I try to understand the
> >>>>>>>> purpose of the series simply because I don't know what does die
> >>>>>>>> erase do as compared to chip erase of sector erase.
> >>>>>>>> Is you series a bug fix or an optmization, maybe both?
> >>>>>>>
> >>>>>>> In current spi-nor framework when user want to erase whole flash
> >> (eg.
> >>>>>>> mtd_debug erase /dev/mtdX 0 0x8000000) framework issue chip
> >> erase
> >>>>>> cmd.
> >>>>>>> N25Q00 does not support chip erase, instead there was introduced
> >>>>>>> die erase command that can erase whole die (N25Q00 is a mulit-die
> >>>>>>> NOR
> >>>> chip).
> >>>>>>> So for this case this patch is a bug fix.
> >>>>>>> Since I have die erase command implemented it was tempting to use
> >>>>>>> die erase also in case where erase area cover one or more dies to
> >>>>>>> speed up erase process, especially when user has enabled 4KiB
> >>>>>>> sector. That is a small
> >>>>>> optimization.
> >>>>>>>
> >>>>>>> I will rewrite commit message and add those informations.
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Marcin
> >>>>>>>
> >>>>>>
> >>>>>> OK, then I think your patch should be split into two different patches.
> >>>>>>
> >>>>>> 1 - bug fix:
> >>>>>>
> >>>>>> The first patch introducing an explicit flag such as
> >>>>>> SPI_NOR_NO_CHIP_ERASE, which would be set in the relevant entries
> >>>>>> of the spi_nor_ids[] (see already existing flags such as
> >>>>>> SPI_NOR_NO_FR,
> >>>> SPI_NOR_NO_ERASE, ...).
> >>>>>>
> >>>>>> In spi_nor_scan():
> >>>>>>
> >>>>>>   if (info->flags & USE_FSR)
> >>>>>>           nor->flags |= SNOR_F_USE_FSR;
> >>>>>>   if (info->flags & SPI_NOR_HAS_TB)
> >>>>>>           nor->flags |= SNOR_F_HAS_SR_TB;
> >>>>>> +        if (info->flags & SPI_NOR_NO_CHIP_ERASE)
> >>>>>> +                nor->flags |= SNOR_F_NO_CHIP_ERASE;
> >>>>>>
> >>>>>> Then in spi_nor_erase():
> >>>>>>
> >>>>>>   /* whole-chip erase? */
> >>>>>> -        if (len == mtd->size) {
> >>>>>> +        if (len == mtd->size && !(nor->flags & SNOR_F_NO_CHIP_ERASE)) {
> >>>>>>           unsigned long timeout;
> >>>>>>
> >>>>>> So you fall into the sector erase case, no optimization yet.
> >>>>>>
> >>>>>> 2 - optimization of spi_nor_erase()
> >>>>>>
> >>>>>> In the "sector"-at-a-time erase branch, try to optimize when
> >>>>>> possible using die erase instead of sector erase.
> >>>>>>
> >>>>>> Each commit message describing the purpose of the patch.
> >>>>>
> >>>>>
> >>>>> OK, I will split patches in this way.
> >>>>>
> >>>>>>
> >>>>>> I would like to also handle the cases where the memory supports:
> >>>>>> - both chip and die erases: if so, no reason not to use chip erase on
> >> such
> >>>>>>   multi-die memory.
> >>>>> I do not think that there are chips supporting both.
> >>>>> In this case two flags are needed one for disable chip erase and
> >>>>> another one for enabling die erase. Is that OK?
> >>>>
> >>>> I think it might be better to have 2 separated flags, at least for
> >>>> semantic
> >>>> reason: "the memory supports Die-Erase hence we don't try to use
> >>>> Chip- Erase" sounds a little bit odd to me.
> >>>
> >>> Let's do it with two flags then.
> >>>
> >>>>
> >>>> This is what I understand when I read:
> >>>>     /* whole-chip erase? */
> >>>> -  if (len == mtd->size) {
> >>>> +  if (len == mtd->size && !die_erase) {
> >>>>
> >>>> Why do we try Chip-Erase only if Die-Erase is not supported?
> >>>
> >>> I assumed that there is no flash that support both.
> >>> Do you know one that has chip erase and die erase?
> >>>
> >>
> >> No, I don't but I didn't focus on erase operations so maybe some already
> >> exist or might exit in the future, who knows? :)
> >>
> >> Also, please have a look at Ricardo's patch:
> >> [v9] mtd: spi-nor: Add supported for S3AN spi-nor devices
> >> http://patchwork.ozlabs.org/patch/701935/
> >>
> >> This patch has recently been merged into git://github.com/spi-nor/linux.git
> >> so you will have to rebase your work on that tree.
> > 
> > Thanks for information, so git://github.com/spi-nor/linux.git should be used 
> > As base to send spi-nor patches?
>
> Indeed yes, as described in MAINTAINERS this is currently the official git
> tree for the spi-nor subsystem. However, we will move soon to infradead.org
> and so the nand subsystem is likely to do. Hence all MTD trees will be
> hosted by infradead.org.
>
> >>
> >> Ricardo's patch already introduces a SNOR_F_NO_OP_CHIP_ERASE flag used
> >> from spi_nor_erase(). The only missing part is to set this flag in nor->flag
> >> when another dedicated flag is set in info->flags.
> >> Currently this SNOR_F_NO_OP_CHIP_ERASE is only set from
> >> s3an_nor_scan() but it should be also set from spi_nor_scan() for the
> >> relevant memories.
> > 
> > Ok.
> >>
> >>>>
> >>>> Reading something like "we try Chip-Erase unless Chip-Erase is not
> >>>> supported" is more logic, isn't it?
> >>>>
> >>>> I want to avoid the implicit assumption "Die Erase supported => Chip
> >>>> Erase not supported", even if it *might* be true, because it is
> >>>> strange when we read the code.
> >>>
> >>> Ok.
> >>>>
> >>>>>> - neither chip nor die erases.
> >>>>> This is supported DIE_ERASE flasg set and die_cnt == 0
> >>>>
> >>>> DIE_ERASE flag set and die_cnt == 0 meaning Chip-Erase is not supported?
> >>>> IHMO, semantically, it's not logical.
> >>>
> >>> Just to save yet another flag. Having so many flags make codes
> >>> complicated, but I agree the it I at least logical.
> >>>>
> >>>> Also discussing another topic on #mtd, it seems that some memories
> >>>> don't have a 4-byte address version of the Die Erase op code.
> >>>
> >>> Yes. Could be workarounded by enabling 4byte address mode just before
> >>> sending die erase and then disable it back.
> >>> The more important question is if you really one to have in upstream die
> >> erase?
> >>> It is about 30% faster than  sector at time erase on some flash
> >>> (especially in case 4KiB sectors). From the other hand I see chips
> >>> that it was not faster at all (it also applies on chip erase).
> >>> My first version of fixing this problem was to add only the flag of disable
> >> chip erase.
> >>> See here:
> >>> http://lists.infradead.org/pipermail/linux-mtd/2016-October/069888.htm
> >>> l Maybe for now it is better to take above?
> >>>
> >>>>
> >>>> If so, it's preferred to use a 4-byte address Sector/Block Erase op
> >>>> code than to enter/exit the statefull 4-byte address mode then use
> >>>> the Die Erase op
> >>>> code: this statefull mode should be avoided as much as possible
> >>>> because many bootloaders expect the SPI memory to be in 3byte address
> >>>> mode and if a spurious reset occurs on the CPU side (but not on the
> >>>> memory side) those bootloaders will fail to read data from the SPI
> >> memory.
> >>>
> >>> Yeap, moreover similar problem you will face when you will try to add
> >>> QPi modes (444) to framework.
> >>>
> >>
> >> Entering the 4-byte address mode might be accepted as a workaround to
> >> allow using the Die Erase op code even on > 16MiB memory as long as the
> >> user can choose to disable this feature and the feature is not enabled by
> >> default.
> >> This way we don't prevent end users from using Die Erase if they want but
> >> we don't force them to use it if they don't want to.
> >>
> >> Indeed, Die Erase is an optimization but it is not mandatory to erase the
> >> memory: the slower Sector/Block Erase commands can still be used.
> >> However entering the 4-byte address mode to allow using the Die Erase
> >> command has side effects, as explained above, that might introduce
> >> regression.
> >>
> >> Let's imagine the case of a 2-die memory used to store 2 firmware images.
> >> The firmware update process could alternate between dies when writing a
> >> new firmware image so the current/older firmware image is still available if
> >> the update process fails to write the new image.
> > 
> > At least for Micron it is possible to do such sequence:
> > 1. Enter 4 byte address mode.
> > 2. Send die erase.
> > 3. Exit 4 byte address mode.
> > 4. Start to poll if erase has been done.
> >
>
> If a spurious CPU reset occurs between the end of step 1 and the beginning
> of step 3, the whole memory would be stuck in its 4byte address mode and
> the bootloader would fail to read data. This is precisely what some
> products want to avoid.

Yes, I just had impression that you are thinking this could take whole erase.
Unfortunately I do not have better solution than this. Any ideas are welcome :)
>
> > Anyway this prove that upstreaming die erase impl. Is not good idea for now.
> > Let's wait for your JESD218 work to be merged.
> >
>
> Your use case is interesting, I don't say we should not add it, I just say
> a mechanism is needed to disable the feature and let it disabled by
> default. On some products, it might be risky hence unacceptable to enter
> the 4 byte mode but on other products it could be safe then the
> optimization you propose is interesting.

I am giving up this series for now. I'll wait for JESD216B merge since it will 
completely brake this functionality. Instead I am going to re-spin removal
of chip erase as it is a clear bug in spi-nor framework at lest for n25q00.

Thanks,
Marcin
>
> >>
> >> Then, if you make the whole memory enter its 4-byte address mode to erase
> >> one single die and a spurious reboot occurs, many bootloaders would fail to
> >> read the old firmware from the other die just because the whole memory
> >> expects 4-byte addresses.
> >>
> >> About the QPI mode (SPI 4-4-4), I agree with you, it has the very same
> >> annoying side effect. Hence I've removed support of the SPI 2-2-2 and SPI
> >> 4-4-4 protocols at the memory side from my SFDP series for the moment.
> > 
> > And that's remained me I have a comment about that :)
> > 
> > Thanks,
> > Marcin
> > 
> >> Besides, on the controller side, by changing the 3rd argument parameter of
> >> spi_nor_scan(), one of my patch allows the SPI controller driver to claim "I
> >> can't/don't want to use SPI 4-4-4 but I support SPI 1-1-4 and SPI 1-4-4
> >> protocols". So the SPI 4-4-4 protocol can be disabled if needed.
> >>
> >> Best regards,
> >>
> >> Cyrille
> >>
> >>
> >>> Thanks,
> >>> Marcin
> >>>
> >>>>
> >>>>>>
> >>>>>> Maybe I'm wrong but the assumption (multi-die => chip erase not
> >>>>>> supported) might be false.
> >>>>> It is false, but all multi-die chip I know support die erase or chip erase.
> >>>>> Never both.
> >>>>>
> >>>>> Thanks,
> >>>>> Marcin
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>> Best regards,
> >>>>>>
> >>>>>> Cyrille
> >>>>>>
> >>>>>>
> >>>>>>>>
> >>>>>>>> Best regards,
> >>>>>>>>
> >>>>>>>> Cyrille
> >>>>>>>>
> >>>>>>>>> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> >>>>>>>>> ---
> >>>>>>>>>  drivers/mtd/spi-nor/spi-nor.c | 90
> >>>>>>>>> +++++++++++++++++++++++++++++++++++++------
> >>>>>>>>>  1 file changed, 78 insertions(+), 12 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c
> >>>>>>>>> b/drivers/mtd/spi-nor/spi-nor.c index 3afe207..17bbec0 100644
> >>>>>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
> >>>>>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >>>>>>>>> @@ -360,6 +360,29 @@ static int spi_nor_erase_sector(struct
> >>>>>>>>> spi_nor
> >>>>>>>> *nor, u32 addr)
> >>>>>>>>>        return nor->write_reg(nor, nor->erase_opcode, buf, nor-
> >>>>>>>>> addr_width); }
> >>>>>>>>>
> >>>>>>>>> +static inline int spi_nor_sector_at_time_erase(struct mtd_info
> >>>>>>>>> +*mtd,
> >>>>>>>>> +u32 addr, u32 len) {
> >>>>>>>>> +     struct spi_nor *nor = mtd_to_spi_nor(mtd);
> >>>>>>>>> +     int ret = 0;
> >>>>>>>>> +
> >>>>>>>>> +     while (len) {
> >>>>>>>>> +             write_enable(nor);
> >>>>>>>>> +
> >>>>>>>>> +             ret = spi_nor_erase_sector(nor, addr);
> >>>>>>>>> +             if (ret)
> >>>>>>>>> +                     return ret;
> >>>>>>>>> +
> >>>>>>>>> +             addr += mtd->erasesize;
> >>>>>>>>> +             len -= mtd->erasesize;
> >>>>>>>>> +
> >>>>>>>>> +             ret = spi_nor_wait_till_ready(nor);
> >>>>>>>>> +             if (ret)
> >>>>>>>>> +                     return ret;
> >>>>>>>>> +     }
> >>>>>>>>> +
> >>>>>>>>> +     return ret;
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>>  /*
> >>>>>>>>>   * Erase an address range on the nor chip.  The address range
> >>>>>>>>> may
> >>>>>> extend
> >>>>>>>>>   * one or more erase sectors.  Return an error is there is a
> >>>>>>>>> problem
> >>>>>> erasing.
> >>>>>>>>> @@ -367,9 +390,11 @@ static int spi_nor_erase_sector(struct
> >>>>>>>>> spi_nor *nor, u32 addr)  static int spi_nor_erase(struct
> >>>>>>>>> mtd_info *mtd, struct erase_info *instr)  {
> >>>>>>>>>        struct spi_nor *nor = mtd_to_spi_nor(mtd);
> >>>>>>>>> -     u32 addr, len;
> >>>>>>>>> +     u32 addr, len, die_no, die_size;
> >>>>>>>>>        uint32_t rem;
> >>>>>>>>>        int ret;
> >>>>>>>>> +     unsigned long timeout;
> >>>>>>>>> +     u8 die_erase = nor->die_cnt && SNOR_F_DIE_ERASE;
> >>>>>>>>>
> >>>>>>>>>        dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr-
> >>> addr,
> >>>>>>>>>                        (long long)instr->len);
> >>>>>>>>> @@ -386,7 +411,7 @@ static int spi_nor_erase(struct mtd_info
> >>>>>>>>> *mtd,
> >>>>>>>> struct erase_info *instr)
> >>>>>>>>>                return ret;
> >>>>>>>>>
> >>>>>>>>>        /* whole-chip erase? */
> >>>>>>>>> -     if (len == mtd->size) {
> >>>>>>>>> +     if (len == mtd->size && !die_erase) {
> >>>>>>>>>                unsigned long timeout;
> >>>>>>>>>
> >>>>>>>>>                write_enable(nor);
> >>>>>>>>> @@ -416,17 +441,58 @@ static int spi_nor_erase(struct mtd_info
> >>>>>>>>> *mtd, struct erase_info *instr)
> >>>>>>>>>
> >>>>>>>>>        /* "sector"-at-a-time erase */
> >>>>>>>>>        } else {
> >>>>>>>>> -             while (len) {
> >>>>>>>>> -                     write_enable(nor);
> >>>>>>>>> -
> >>>>>>>>> -                     ret = spi_nor_erase_sector(nor, addr);
> >>>>>>>>> -                     if (ret)
> >>>>>>>>> +             if (die_erase) {
> >>>>>>>>> +                     die_size = div_u64_rem(mtd->size, nor-
> >>>>> die_cnt,
> >>>>>>>> &rem);
> >>>>>>>>> +                     if (rem) {
> >>>>>>>>> +                             ret = -EINVAL;
> >>>>>>>>>                                goto erase_err;
> >>>>>>>>> -
> >>>>>>>>> -                     addr += mtd->erasesize;
> >>>>>>>>> -                     len -= mtd->erasesize;
> >>>>>>>>> -
> >>>>>>>>> -                     ret = spi_nor_wait_till_ready(nor);
> >>>>>>>>> +                     }
> >>>>>>>>> +                     while (len) {
> >>>>>>>>> +                             die_no = div_u64_rem(addr,
> >>>> die_size,
> >>>>>>>> &rem);
> >>>>>>>>> +
> >>>>>>>>> +                             /* Check if address is aligned to die
> >>>> begin*/
> >>>>>>>>> +                             if (!rem) {
> >>>>>>>>> +                                     /* die erase? */
> >>>>>>>>> +                                     if (len >= die_size) {
> >>>>>>>>> +                                             ret =
> >>>> spi_nor_die_erase(nor,
> >>>>>>>> addr);
> >>>>>>>>> +                                             if (ret)
> >>>>>>>>> +                                                     goto
> >>>> erase_err;
> >>>>>>>>> +
> >>>>>>>>> +                                             len -= die_size;
> >>>>>>>>> +                                             addr += die_size;
> >>>>>>>>> +
> >>>>>>>>> +                                             timeout =
> >>>>>>>> max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
> >>>>>>>>> +
> >>>>>>>>         CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
> >>>>>>>>> +
> >>>>     (unsigned
> >>>>>>>> long)(die_size / SZ_2M));
> >>>>>>>>> +                                             ret =
> >>>>>>>> spi_nor_wait_till_ready_with_timeout(nor, timeout);
> >>>>>>>>> +                                             if (ret)
> >>>>>>>>> +                                                     goto
> >>>> erase_err;
> >>>>>>>>> +                                     } else {
> >>>>>>>>> +                                             ret =
> >>>>>>>> spi_nor_sector_at_time_erase(mtd, addr, len);
> >>>>>>>>> +                                             if (ret)
> >>>>>>>>> +                                                     goto
> >>>> erase_err;
> >>>>>>>>> +                                             len = 0;
> >>>>>>>>> +                                     }
> >>>>>>>>> +                             } else {
> >>>>>>>>> +                                     /* check if end address cover
> >>>> at least
> >>>>>>>> one die */
> >>>>>>>>> +                                     if (div64_ul(addr + len,
> >>>> die_size) >
> >>>>>>>> ++die_no) {
> >>>>>>>>> +                                             /* align to next die */
> >>>>>>>>> +                                             rem = die_size - rem;
> >>>>>>>>> +                                             ret =
> >>>>>>>> spi_nor_sector_at_time_erase(mtd, addr, rem);
> >>>>>>>>> +                                             if (ret)
> >>>>>>>>> +                                                     goto
> >>>> erase_err;
> >>>>>>>>> +                                             len -= rem;
> >>>>>>>>> +                                             addr += rem;
> >>>>>>>>> +                                     } else {
> >>>>>>>>> +                                             ret =
> >>>>>>>> spi_nor_sector_at_time_erase(mtd, addr, len);
> >>>>>>>>> +                                             if (ret)
> >>>>>>>>> +                                                     goto
> >>>> erase_err;
> >>>>>>>>> +                                             len = 0;
> >>>>>>>>> +                                     }
> >>>>>>>>> +                             }
> >>>>>>>>> +                     }
> >>>>>>>>> +             } else {
> >>>>>>>>> +                     ret = spi_nor_sector_at_time_erase(mtd,
> >>>> addr, len);
> >>>>>>>>>                        if (ret)
> >>>>>>>>>                                goto erase_err;
> >>>>>>>>>                }
> >>>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> > 
> > 
>