altera_qspi: call callback even if the erase failed

PATCH New Thomas Chou 2 participants 30 messages Dec 24, 2015 – Dec 28, 2015 Memory technology devices (MTD)

Share & tools

Thomas Chou

author

2015-12-24 00:51 UTC #0 lore raw

PATCH 1/5 altera_qspi: call callback even if the erase failed

Erase is an asynchronous operation. Device drivers are supposed to call instr->callback() whenever the operation completes, even if it completes with a failure.

Signed-off-by Thomas Chou <[email protected]>

---
 drivers/mtd/altera_qspi.c | 1 +
 1 file changed, 1 insertion(+)

drivers/mtd/altera_qspi.c +1 -0

index c7e37ad..627a8cc 100644
@@ -146,6 +146,7 @@ static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr)
 			debug("erase %08x fail %x\n", sect, stat);
 			writel(stat, &regs->isr); /* clear isr */
 			instr->state = MTD_ERASE_FAILED;
+			mtd_erase_callback(instr);
 			return -EIO;
 		}
 		addr += mtd->erasesize;

Signature · 1 line

2.5.0

Thomas Chou

author

2015-12-28 01:34 UTC #1 lore raw

PATCH 1/5 altera_qspi: call callback even if the erase failed

Quoted 9 lines · Thomas Chou

Erase is an asynchronous operation. Device drivers are supposed to call instr->callback() whenever the operation completes, even if it completes with a failure.
Signed-off-by Thomas Chou <[email protected]>
---
  drivers/mtd/altera_qspi.c | 1 +
  1 file changed, 1 insertion(+)

Applied to u-boot-nios.

Thomas Chou

author

2015-12-24 00:51 UTC #2 lore raw

PATCH 2/5 altera_qspi: set fail_addr for erase ops

In reply to Thomas Chou

If the erase fails, fail_addr might indicate exactly which block failed. If fail_addr = MTD_FAIL_ADDR_UNKNOWN, the failure was not at the device level or was not specific to any particular block.

Signed-off-by Thomas Chou <[email protected]>

---
 drivers/mtd/altera_qspi.c | 1 +
 1 file changed, 1 insertion(+)

drivers/mtd/altera_qspi.c +1 -0

index 627a8cc..b0d4f2c 100644
@@ -145,6 +145,7 @@ static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr)
 			/* erase failed, sector might be protected */
 			debug("erase %08x fail %x\n", sect, stat);
 			writel(stat, &regs->isr); /* clear isr */
+			instr->fail_addr = addr;
 			instr->state = MTD_ERASE_FAILED;
 			mtd_erase_callback(instr);
 			return -EIO;

Signature · 1 line

2.5.0

Thomas Chou

author

2015-12-28 01:34 UTC #3 lore raw

PATCH 2/5 altera_qspi: set fail_addr for erase ops

Quoted 9 lines · Thomas Chou

If the erase fails, fail_addr might indicate exactly which block failed. If fail_addr = MTD_FAIL_ADDR_UNKNOWN, the failure was not at the device level or was not specific to any particular block.
Signed-off-by Thomas Chou <[email protected]>
---
  drivers/mtd/altera_qspi.c | 1 +
  1 file changed, 1 insertion(+)

Applied to u-boot-nios.

Thomas Chou

author

2015-12-24 00:51 UTC #4 lore raw

PATCH 3/5 altera_qspi: skip erase if the sector is blank

In reply to Thomas Chou

Skip erase if the sector is blank. The sector erase is slow, and may take 0.7 sec typically or up to 3 sec worst-case.

Signed-off-by Thomas Chou <[email protected]>

---
 drivers/mtd/altera_qspi.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

drivers/mtd/altera_qspi.c +25 ~~-14~~

index b0d4f2c..8a630a6 100644
@@ -131,24 +131,35 @@ static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr)
 	size_t end = addr + len;
 	u32 sect;
 	u32 stat;
+	u32 *flash, *last;
 
 	instr->state = MTD_ERASING;
 	addr &= ~(mtd->erasesize - 1); /* get lower aligned address */
 	while (addr < end) {
-		sect = addr / mtd->erasesize;
-		sect <<= 8;
-		sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
-		debug("erase %08x\n", sect);
-		writel(sect, &regs->mem_op);
-		stat = readl(&regs->isr);
-		if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
-			/* erase failed, sector might be protected */
-			debug("erase %08x fail %x\n", sect, stat);
-			writel(stat, &regs->isr); /* clear isr */
-			instr->fail_addr = addr;
-			instr->state = MTD_ERASE_FAILED;
-			mtd_erase_callback(instr);
-			return -EIO;
+		flash = pdata->base + addr;
+		last = pdata->base + addr + mtd->erasesize;
+		/* skip erase if sector is blank */
+		while (flash < last) {
+			if (readl(flash) != 0xffffffff)
+				break;
+			flash++;
+		}
+		if (flash < last) {
+			sect = addr / mtd->erasesize;
+			sect <<= 8;
+			sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
+			debug("erase %08x\n", sect);
+			writel(sect, &regs->mem_op);
+			stat = readl(&regs->isr);
+			if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
+				/* erase failed, sector might be protected */
+				debug("erase %08x fail %x\n", sect, stat);
+				writel(stat, &regs->isr); /* clear isr */
+				instr->fail_addr = addr;
+				instr->state = MTD_ERASE_FAILED;
+				mtd_erase_callback(instr);
+				return -EIO;
+			}
 		}
 		addr += mtd->erasesize;
 	}

Signature · 1 line

2.5.0

Marek Vasut

2015-12-24 01:28 UTC #5 lore raw

PATCH 3/5 altera_qspi: skip erase if the sector is blank

Quoted 32 lines · Thomas Chou

Skip erase if the sector is blank. The sector erase is slow, and may take 0.7 sec typically or up to 3 sec worst-case.

Signed-off-by Thomas Chou <[email protected]>

---
 drivers/mtd/altera_qspi.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

drivers/mtd/altera_qspi.c +1 ~~-11~~

index b0d4f2c..8a630a6 100644
@@ -131,24 +131,35 @@ static int altera_qspi_erase(struct mtd_info *mtd,
 struct erase_info *instr) size_t end = addr + len;
 	u32 sect;
 	u32 stat;
+	u32 *flash, *last;
 
 	instr->state = MTD_ERASING;
 	addr &= ~(mtd->erasesize - 1); /* get lower aligned address */
 	while (addr < end) {
-		sect = addr / mtd->erasesize;
-		sect <<= 8;
-		sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
-		debug("erase %08x\n", sect);
-		writel(sect, &regs->mem_op);
-		stat = readl(&regs->isr);
-		if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
-			/* erase failed, sector might be protected */
-			debug("erase %08x fail %x\n", sect, stat);
-			writel(stat, &regs->isr); /* clear isr */
-			instr->fail_addr = addr;

drivers/mtd/altera_qspi.c static int altera_qspi_erase(struct mtd_info *mtd, · line 145

Quoted 10 lines

drivers/mtd/altera_qspi.c +7 -3

-			instr->state = MTD_ERASE_FAILED;
-			mtd_erase_callback(instr);
-			return -EIO;
+		flash = pdata->base + addr;
+		last = pdata->base + addr + mtd->erasesize;
+		/* skip erase if sector is blank */
+		while (flash < last) {
+			if (readl(flash) != 0xffffffff)
+				break;
+			flash++;

Shouldn't $last be divided by 4 ? $flash is u32 * afterall .

Quoted 10 lines

drivers/mtd/altera_qspi.c +10 -0

+		}
+		if (flash < last) {
+			sect = addr / mtd->erasesize;
+			sect <<= 8;
+			sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
+			debug("erase %08x\n", sect);
+			writel(sect, &regs->mem_op);
+			stat = readl(&regs->isr);
+			if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
+				/* erase failed, sector might be protected */

drivers/mtd/altera_qspi.c static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr) · line 165

Quoted 10 lines

drivers/mtd/altera_qspi.c +7 -0

+				debug("erase %08x fail %x\n", sect, stat);
+				writel(stat, &regs->isr); /* clear isr */
+				instr->fail_addr = addr;
+				instr->state = MTD_ERASE_FAILED;
+				mtd_erase_callback(instr);
+				return -EIO;
+			}
 		}
 		addr += mtd->erasesize;
 	}

Best regards,
Marek Vasut

Thomas Chou

author

2015-12-24 02:23 UTC #6 lore raw

PATCH 3/5 altera_qspi: skip erase if the sector is blank

Hi Marek,

Quoted 37 lines · Marek Vasut

Quoted 36 lines · Thomas Chou

Skip erase if the sector is blank. The sector erase is slow, and may take 0.7 sec typically or up to 3 sec worst-case.

Signed-off-by Thomas Chou <[email protected]>

---
  drivers/mtd/altera_qspi.c | 39 +++++++++++++++++++++++++--------------
  1 file changed, 25 insertions(+), 14 deletions(-)

drivers/mtd/altera_qspi.c +2 ~~-14~~

index b0d4f2c..8a630a6 100644
@@ -131,24 +131,35 @@ static int altera_qspi_erase(struct mtd_info *mtd,
 struct erase_info *instr) size_t end = addr + len;
  	u32 sect;
  	u32 stat;
+	u32 *flash, *last;
 
  	instr->state = MTD_ERASING;
  	addr &= ~(mtd->erasesize - 1); /* get lower aligned address */
  	while (addr < end) {
-		sect = addr / mtd->erasesize;
-		sect <<= 8;
-		sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
-		debug("erase %08x\n", sect);
-		writel(sect, &regs->mem_op);
-		stat = readl(&regs->isr);
-		if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
-			/* erase failed, sector might be protected */
-			debug("erase %08x fail %x\n", sect, stat);
-			writel(stat, &regs->isr); /* clear isr */
-			instr->fail_addr = addr;
-			instr->state = MTD_ERASE_FAILED;
-			mtd_erase_callback(instr);
-			return -EIO;
+		flash = pdata->base + addr;

Quoted 8 lines

drivers/mtd/altera_qspi.c static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr) · line 145
Quoted 6 lines
drivers/mtd/altera_qspi.c +6 -0
+		last = pdata->base + addr + mtd->erasesize;
+		/* skip erase if sector is blank */
+		while (flash < last) {
+			if (readl(flash) != 0xffffffff)
+				break;
+			flash++;
Shouldn't $last be divided by 4 ? $flash is u32 * afterall .

No. Both flash and last are assigned with byte addressing from pdata->base, which is void *.

Best regards,
Thomas

Marek Vasut

2015-12-24 03:01 UTC #7 lore raw

PATCH 3/5 altera_qspi: skip erase if the sector is blank

Quoted 53 lines · Thomas Chou

Hi Marek,

Quoted 41 lines · Marek Vasut

Quoted 40 lines · Thomas Chou

Skip erase if the sector is blank. The sector erase is slow, and may take 0.7 sec typically or up to 3 sec worst-case.

Signed-off-by Thomas Chou <[email protected]>

---

  drivers/mtd/altera_qspi.c | 39 +++++++++++++++++++++++++--------------
  1 file changed, 25 insertions(+), 14 deletions(-)

drivers/mtd/altera_qspi.c +2 ~~-14~~

index b0d4f2c..8a630a6 100644
@@ -131,24 +131,35 @@ static int altera_qspi_erase(struct mtd_info *mtd,
 struct erase_info *instr) size_t end = addr + len;
 
  	u32 sect;
  	u32 stat;
 
+	u32 *flash, *last;
 
  	instr->state = MTD_ERASING;
  	addr &= ~(mtd->erasesize - 1); /* get lower aligned address */
  	while (addr < end) {
 
-		sect = addr / mtd->erasesize;
-		sect <<= 8;
-		sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
-		debug("erase %08x\n", sect);
-		writel(sect, &regs->mem_op);
-		stat = readl(&regs->isr);
-		if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
-			/* erase failed, sector might be protected */
-			debug("erase %08x fail %x\n", sect, stat);
-			writel(stat, &regs->isr); /* clear isr */
-			instr->fail_addr = addr;
-			instr->state = MTD_ERASE_FAILED;
-			mtd_erase_callback(instr);
-			return -EIO;
+		flash = pdata->base + addr;

Quoted 8 lines

drivers/mtd/altera_qspi.c static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr) · line 145
Quoted 6 lines
drivers/mtd/altera_qspi.c +6 -0
+		last = pdata->base + addr + mtd->erasesize;
+		/* skip erase if sector is blank */
+		while (flash < last) {
+			if (readl(flash) != 0xffffffff)
+				break;
+			flash++;
Shouldn't $last be divided by 4 ? $flash is u32 * afterall .

Quoted 2 lines

No. Both flash and last are assigned with byte addressing from pdata->base, which is void *.

The data type of both $flash and $last is u32 * though?

Best regards,
Marek Vasut

Thomas Chou

author

2015-12-24 03:08 UTC #8 lore raw

PATCH 3/5 altera_qspi: skip erase if the sector is blank

Hi Marek,

Quoted 54 lines · Marek Vasut

Quoted 53 lines · Thomas Chou

Hi Marek,

Quoted 49 lines · Marek Vasut

Quoted 36 lines · Thomas Chou

Skip erase if the sector is blank. The sector erase is slow, and may take 0.7 sec typically or up to 3 sec worst-case.

Signed-off-by Thomas Chou <[email protected]>

---

   drivers/mtd/altera_qspi.c | 39 +++++++++++++++++++++++++--------------
   1 file changed, 25 insertions(+), 14 deletions(-)

drivers/mtd/altera_qspi.c +1 ~~-11~~

index b0d4f2c..8a630a6 100644
@@ -131,24 +131,35 @@ static int altera_qspi_erase(struct mtd_info *mtd,
 struct erase_info *instr) size_t end = addr + len;
 
   	u32 sect;
   	u32 stat;
 
+	u32 *flash, *last;
 
   	instr->state = MTD_ERASING;
   	addr &= ~(mtd->erasesize - 1); /* get lower aligned address */
   	while (addr < end) {
 
-		sect = addr / mtd->erasesize;
-		sect <<= 8;
-		sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
-		debug("erase %08x\n", sect);
-		writel(sect, &regs->mem_op);
-		stat = readl(&regs->isr);
-		if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
-			/* erase failed, sector might be protected */
-			debug("erase %08x fail %x\n", sect, stat);
-			writel(stat, &regs->isr); /* clear isr */
-			instr->fail_addr = addr;

drivers/mtd/altera_qspi.c static int altera_qspi_erase(struct mtd_info *mtd, · line 148

Quoted 10 lines

drivers/mtd/altera_qspi.c +7 -3

-			instr->state = MTD_ERASE_FAILED;
-			mtd_erase_callback(instr);
-			return -EIO;
+		flash = pdata->base + addr;
+		last = pdata->base + addr + mtd->erasesize;
+		/* skip erase if sector is blank */
+		while (flash < last) {
+			if (readl(flash) != 0xffffffff)
+				break;
+			flash++;

Shouldn't $last be divided by 4 ? $flash is u32 * afterall .

Quoted 4 lines

Quoted 2 lines
No. Both flash and last are assigned with byte addressing from pdata->base, which is void *.
The data type of both $flash and $last is u32 * though?

Yes.

flash = pdata->base + addr;

will be the same as,

flash = pdata->base;
flash += addr / 4;

Best regards,
Thomas

Marek Vasut

2015-12-24 03:43 UTC #9 lore raw

PATCH 3/5 altera_qspi: skip erase if the sector is blank

Quoted 63 lines · Thomas Chou

Hi Marek,
Quoted 55 lines · Marek Vasut
Quoted 54 lines · Thomas Chou
Hi Marek,
Quoted 50 lines · Marek Vasut
Quoted 37 lines · Thomas Chou
Skip erase if the sector is blank. The sector erase is slow, and may take 0.7 sec typically or up to 3 sec worst-case.
Signed-off-by Thomas Chou <[email protected]>
---
   drivers/mtd/altera_qspi.c | 39
+++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-)
drivers/mtd/altera_qspi.c +1 ~~-11~~
index b0d4f2c..8a630a6 100644
@@ -131,24 +131,35 @@ static int altera_qspi_erase(struct mtd_info
 *mtd, struct erase_info *instr) size_t end = addr + len;
 
   	u32 sect;
   	u32 stat;
 
+	u32 *flash, *last;
 
   	instr->state = MTD_ERASING;
   	addr &= ~(mtd->erasesize - 1); /* get lower aligned address */
   	while (addr < end) {
 
-		sect = addr / mtd->erasesize;
-		sect <<= 8;
-		sect |= QUADSPI_MEM_OP_SECTOR_ERASE;
-		debug("erase %08x\n", sect);
-		writel(sect, &regs->mem_op);
-		stat = readl(&regs->isr);
-		if (stat & QUADSPI_ISR_ILLEGAL_ERASE) {
-			/* erase failed, sector might be protected */
-			debug("erase %08x fail %x\n", sect, stat);
-			writel(stat, &regs->isr); /* clear isr */
-			instr->fail_addr = addr;
drivers/mtd/altera_qspi.c static int altera_qspi_erase(struct mtd_info · line 148
Quoted 10 lines
drivers/mtd/altera_qspi.c +7 -3
-			instr->state = MTD_ERASE_FAILED;
-			mtd_erase_callback(instr);
-			return -EIO;
+		flash = pdata->base + addr;
+		last = pdata->base + addr + mtd->erasesize;
+		/* skip erase if sector is blank */
+		while (flash < last) {
+			if (readl(flash) != 0xffffffff)
+				break;
+			flash++;
Shouldn't $last be divided by 4 ? $flash is u32 * afterall .
Quoted 4 lines
Quoted 2 lines
No. Both flash and last are assigned with byte addressing from pdata->base, which is void *.
The data type of both $flash and $last is u32 * though?

Quoted 8 lines

Yes.
flash = pdata->base + addr;
will be the same as,
flash = pdata->base;
flash += addr / 4;

Please ignore what I said, sorry.

Thomas Chou

author

2015-12-28 01:34 UTC #10 lore raw

PATCH 3/5 altera_qspi: skip erase if the sector is blank

In reply to Thomas Chou

Quoted 8 lines · Thomas Chou

Skip erase if the sector is blank. The sector erase is slow, and may take 0.7 sec typically or up to 3 sec worst-case.
Signed-off-by Thomas Chou <[email protected]>
---
  drivers/mtd/altera_qspi.c | 39 +++++++++++++++++++++++++--------------
  1 file changed, 25 insertions(+), 14 deletions(-)

Applied to u-boot-nios.

Thomas Chou

author

2015-12-24 00:51 UTC #11 lore raw

PATCH 4/5 altera_qspi: show erase progress

In reply to Thomas Chou

Show sector erase progress with dot and comma.

Signed-off-by Thomas Chou <[email protected]>

---
 drivers/mtd/altera_qspi.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

drivers/mtd/altera_qspi.c +14 -0

index 8a630a6..0624ff4 100644
@@ -52,6 +52,7 @@ struct altera_qspi_platdata {
 	unsigned long size;
 };
 
+static uint flash_verbose;
 flash_info_t flash_info[CONFIG_SYS_MAX_FLASH_BANKS];	/* FLASH chips info */
 
 static void altera_qspi_get_locked_range(struct mtd_info *mtd, loff_t *ofs,

@@ -74,6 +75,11 @@ void flash_print_info(flash_info_t *info)
 	putc('\n');
 }
 
+void flash_set_verbose(uint v)
+{
+	flash_verbose = v;
+}
+
 int flash_erase(flash_info_t *info, int s_first, int s_last)
 {
 	struct mtd_info *mtd = info->mtd;

@@ -84,10 +90,13 @@ int flash_erase(flash_info_t *info, int s_first, int s_last)
 	instr.mtd = mtd;
 	instr.addr = mtd->erasesize * s_first;
 	instr.len = mtd->erasesize * (s_last + 1 - s_first);
+	flash_set_verbose(1);
 	ret = mtd_erase(mtd, &instr);
+	flash_set_verbose(0);
 	if (ret)
 		return ERR_PROTECTED;
 
+	puts(" done\n");
 	return 0;
 }

@@ -160,6 +169,11 @@ static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr)
 				mtd_erase_callback(instr);
 				return -EIO;
 			}
+			if (flash_verbose)
+				putc('.');
+		} else {
+			if (flash_verbose)
+				putc(',');
 		}
 		addr += mtd->erasesize;
 	}

Signature · 1 line

2.5.0

Marek Vasut

2015-12-24 01:29 UTC #12 lore raw

PATCH 4/5 altera_qspi: show erase progress

Quoted 4 lines · Thomas Chou

Show sector erase progress with dot and comma.
Signed-off-by Thomas Chou <[email protected]>
---

Shouldn't this go into common code ?

Best regards,
Marek Vasut

Thomas Chou

author

2015-12-24 02:50 UTC #13 lore raw

PATCH 4/5 altera_qspi: show erase progress

Hi Marek,

Quoted 8 lines · Marek Vasut

Quoted 4 lines · Thomas Chou
Show sector erase progress with dot and comma.
Signed-off-by Thomas Chou <[email protected]>
---
Shouldn't this go into common code ?

The code to print a dot is minimal. It will cost more if this go into common code.

Alternatively, we might set the flash_verbose directly and remove the flash_set_verbose().

Best regards,
Thomas

Marek Vasut

2015-12-24 03:02 UTC #14 lore raw

PATCH 4/5 altera_qspi: show erase progress

Quoted 6 lines · Thomas Chou

Hi Marek,
Quoted 3 lines · Marek Vasut
Quoted 2 lines · Thomas Chou
Show sector erase progress with dot and comma.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Signed-off-by Thomas Chou <[email protected]>
---
Shouldn't this go into common code ?
The code to print a dot is minimal. It will cost more if this go into common code.

But then this driver behaves in a non-standard manner AND noone benefits from this functionality but this driver.

Quoted 5 lines

Alternatively, we might set the flash_verbose directly and remove the flash_set_verbose().
Best regards,
Thomas

Best regards,
Marek Vasut

Thomas Chou

author

2015-12-24 03:26 UTC #15 lore raw

PATCH 4/5 altera_qspi: show erase progress

Hi Marek,

Quoted 10 lines · Marek Vasut

Quoted 9 lines · Thomas Chou
Hi Marek,
Quoted 6 lines · Marek Vasut
Quoted 4 lines · Thomas Chou
Show sector erase progress with dot and comma.
Signed-off-by Thomas Chou <[email protected]>
---

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
Shouldn't this go into common code ?
The code to print a dot is minimal. It will cost more if this go into common code.
But then this driver behaves in a non-standard manner AND noone benefits from this functionality but this driver.

The sector erase might take very long when there are a lot of sectors to erase, say 12 min to 50 min for 1024 sectors on the 10m50 board. Without the display of progress, it will look like the board hangs.

The dotting code comes from the old cfi_flash.c. Some other parallel flash have this, but not spi-flash. It is trivial to add though.

Best regards,
Thomas

Marek Vasut

2015-12-24 03:42 UTC #16 lore raw

PATCH 4/5 altera_qspi: show erase progress

Quoted 1 line · Thomas Chou

Hi Marek,

Hi Thomas,

Quoted 16 lines · Marek Vasut

Quoted 15 lines · Thomas Chou
Quoted 13 lines
Hi Marek,
Quoted 7 lines · Marek Vasut
Quoted 4 lines · Thomas Chou
Show sector erase progress with dot and comma.
Signed-off-by Thomas Chou <[email protected]>
---
Shouldn't this go into common code ?
The code to print a dot is minimal. It will cost more if this go into common code.

Quoted 6 lines

Quoted 2 lines
But then this driver behaves in a non-standard manner AND noone benefits from this functionality but this driver.
The sector erase might take very long when there are a lot of sectors to erase, say 12 min to 50 min for 1024 sectors on the 10m50 board. Without the display of progress, it will look like the board hangs.

Yeah, that I do understand and I agree this is a good idea :)

Quoted 2 lines

The dotting code comes from the old cfi_flash.c. Some other parallel flash have this, but not spi-flash. It is trivial to add though.

Urm, altera_qspi is CFI, right ? So can we stuff this into common code or not? Sorry if I am confused and off the mark.

Best regards,
Marek Vasut

Thomas Chou

author

2015-12-24 05:24 UTC #17 lore raw

PATCH 4/5 altera_qspi: show erase progress

Hi Marek,

Quoted 28 lines · Marek Vasut

Quoted 1 line · Thomas Chou
Hi Marek,
Hi Thomas,
Quoted 16 lines · Marek Vasut
Quoted 15 lines · Thomas Chou
Quoted 13 lines
Hi Marek,
Quoted 7 lines · Marek Vasut
Quoted 4 lines · Thomas Chou
Show sector erase progress with dot and comma.
Signed-off-by Thomas Chou <[email protected]>
---
Shouldn't this go into common code ?
The code to print a dot is minimal. It will cost more if this go into common code.
Quoted 6 lines
Quoted 2 lines
But then this driver behaves in a non-standard manner AND noone benefits from this functionality but this driver.
The sector erase might take very long when there are a lot of sectors to erase, say 12 min to 50 min for 1024 sectors on the 10m50 board. Without the display of progress, it will look like the board hangs.

Quoted 7 lines

Yeah, that I do understand and I agree this is a good idea :)
Quoted 2 lines
The dotting code comes from the old cfi_flash.c. Some other parallel flash have this, but not spi-flash. It is trivial to add though.
Urm, altera_qspi is CFI, right ? So can we stuff this into common code or not? Sorry if I am confused and off the mark.

No worries. I think the dotting is best done per driver. We can have common behavior. But there is little advantage to make it common code.

Merry Xmas.

Thomas Chou

Marek Vasut

2015-12-24 05:37 UTC #18 lore raw

PATCH 4/5 altera_qspi: show erase progress

Quoted 33 lines · Thomas Chou

Hi Marek,
Quoted 30 lines · Marek Vasut
Quoted 1 line · Thomas Chou
Hi Marek,
Hi Thomas,
Quoted 16 lines · Marek Vasut
Quoted 15 lines · Thomas Chou
Quoted 13 lines
Hi Marek,
Quoted 7 lines · Marek Vasut
Quoted 4 lines · Thomas Chou
Show sector erase progress with dot and comma.
Signed-off-by Thomas Chou <[email protected]>
---
Shouldn't this go into common code ?
The code to print a dot is minimal. It will cost more if this go into common code.
Quoted 6 lines
Quoted 2 lines
But then this driver behaves in a non-standard manner AND noone benefits from this functionality but this driver.
The sector erase might take very long when there are a lot of sectors to erase, say 12 min to 50 min for 1024 sectors on the 10m50 board. Without the display of progress, it will look like the board hangs.
Yeah, that I do understand and I agree this is a good idea :)

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
The dotting code comes from the old cfi_flash.c. Some other parallel flash have this, but not spi-flash. It is trivial to add though.
Urm, altera_qspi is CFI, right ? So can we stuff this into common code or not? Sorry if I am confused and off the mark.
No worries. I think the dotting is best done per driver.

Well why do you think so ?

Quoted 4 lines

We can have
common behavior. But there is little advantage to make it common code.
Merry Xmas.

Merry Xmas to you too!

Best regards,
Marek Vasut

Thomas Chou

author

2015-12-25 00:12 UTC #19 lore raw

PATCH 4/5 altera_qspi: show erase progress

Hi Marek,

Quoted 37 lines · Marek Vasut

Quoted 36 lines · Thomas Chou
Hi Marek,
Quoted 33 lines · Marek Vasut
Quoted 1 line · Thomas Chou
Hi Marek,
Hi Thomas,
Quoted 16 lines · Marek Vasut
Quoted 15 lines · Thomas Chou
Quoted 13 lines
Hi Marek,
Quoted 7 lines · Marek Vasut
Quoted 4 lines · Thomas Chou
Show sector erase progress with dot and comma.
Signed-off-by Thomas Chou <[email protected]>
---
Shouldn't this go into common code ?
The code to print a dot is minimal. It will cost more if this go into common code.
Quoted 6 lines
Quoted 2 lines
But then this driver behaves in a non-standard manner AND noone benefits from this functionality but this driver.
The sector erase might take very long when there are a lot of sectors to erase, say 12 min to 50 min for 1024 sectors on the 10m50 board. Without the display of progress, it will look like the board hangs.
Yeah, that I do understand and I agree this is a good idea :)
Quoted 2 lines
The dotting code comes from the old cfi_flash.c. Some other parallel flash have this, but not spi-flash. It is trivial to add though.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Urm, altera_qspi is CFI, right ? So can we stuff this into common code or not? Sorry if I am confused and off the mark.
No worries. I think the dotting is best done per driver.
Well why do you think so ?

Well, it is only a putc() which is plain and simple.

Best regards,
Thomas

Marek Vasut

2015-12-25 04:08 UTC #20 lore raw

PATCH 4/5 altera_qspi: show erase progress

Quoted 40 lines · Thomas Chou

Hi Marek,
Quoted 37 lines · Marek Vasut
Quoted 36 lines · Thomas Chou
Hi Marek,
Quoted 33 lines · Marek Vasut
Quoted 1 line · Thomas Chou
Hi Marek,
Hi Thomas,
Quoted 16 lines · Marek Vasut
Quoted 15 lines · Thomas Chou
Quoted 13 lines
Hi Marek,
Quoted 7 lines · Marek Vasut
Quoted 4 lines · Thomas Chou
Show sector erase progress with dot and comma.
Signed-off-by Thomas Chou <[email protected]>
---
Shouldn't this go into common code ?
The code to print a dot is minimal. It will cost more if this go into common code.
Quoted 6 lines
Quoted 2 lines
But then this driver behaves in a non-standard manner AND noone benefits from this functionality but this driver.
The sector erase might take very long when there are a lot of sectors to erase, say 12 min to 50 min for 1024 sectors on the 10m50 board. Without the display of progress, it will look like the board hangs.
Yeah, that I do understand and I agree this is a good idea :)
Quoted 2 lines
The dotting code comes from the old cfi_flash.c. Some other parallel flash have this, but not spi-flash. It is trivial to add though.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Urm, altera_qspi is CFI, right ? So can we stuff this into common code or not? Sorry if I am confused and off the mark.
No worries. I think the dotting is best done per driver.
Well why do you think so ?
Well, it is only a putc() which is plain and simple.

Sure, but then I still don't understand why this cannot be in the common code.

Best regards,
Marek Vasut

Thomas Chou

author

2015-12-25 08:33 UTC #21 lore raw

PATCH 4/5 altera_qspi: show erase progress

Hi Marek,

Quoted 3 lines · Marek Vasut

Quoted 1 line
Well, it is only a putc() which is plain and simple.
Sure, but then I still don't understand why this cannot be in the common code.

Yes, it can be in the common code. Do you have an idea how should it look like?

Best regards,
Thomas

Marek Vasut

2015-12-25 09:58 UTC #22 lore raw

PATCH 4/5 altera_qspi: show erase progress

Quoted 10 lines · Thomas Chou

Hi Marek,
Quoted 4 lines · Marek Vasut
Quoted 1 line
Well, it is only a putc() which is plain and simple.
Sure, but then I still don't understand why this cannot be in the common code.
Yes, it can be in the common code. Do you have an idea how should it look like?

No, I don't, I am not a CFI expert, sorry.

Best regards,
Marek Vasut

Thomas Chou

author

2015-12-28 01:17 UTC #23 lore raw

PATCH 4/5 altera_qspi: show erase progress

Hi Marek,

Quoted 6 lines · Marek Vasut

Quoted 5 lines · Thomas Chou
Hi Marek,
Quoted 2 lines · Marek Vasut
Quoted 1 line
Well, it is only a putc() which is plain and simple.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Sure, but then I still don't understand why this cannot be in the common code.
Yes, it can be in the common code. Do you have an idea how should it look like?
No, I don't, I am not a CFI expert, sorry.

Then I shall push these first and find way to put it to common code some time later. Thanks.

Best regards,
Thomas

Marek Vasut

2015-12-28 05:35 UTC #24 lore raw

PATCH 4/5 altera_qspi: show erase progress

Quoted 12 lines · Thomas Chou

Hi Marek,
Quoted 9 lines · Marek Vasut
Quoted 8 lines · Thomas Chou
Hi Marek,
Quoted 4 lines · Marek Vasut
Quoted 1 line
Well, it is only a putc() which is plain and simple.
Sure, but then I still don't understand why this cannot be in the common code.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Yes, it can be in the common code. Do you have an idea how should it look like?
No, I don't, I am not a CFI expert, sorry.
Then I shall push these first and find way to put it to common code some time later. Thanks.

Is this any way to deal with feedback ? Why don't you put it into flash_erase() for example, in drivers/mtd/cfi_flash.c ?

Best regards,
Marek Vasut

Thomas Chou

author

2015-12-28 08:04 UTC #25 lore raw

PATCH 4/5 altera_qspi: show erase progress

Hi Marek,

Quoted 16 lines · Marek Vasut

Quoted 15 lines · Thomas Chou
Hi Marek,
Quoted 12 lines · Marek Vasut
Quoted 10 lines · Thomas Chou
Hi Marek,
Quoted 4 lines · Marek Vasut
Quoted 1 line
Well, it is only a putc() which is plain and simple.
Sure, but then I still don't understand why this cannot be in the common code.
Yes, it can be in the common code. Do you have an idea how should it look like?

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
No, I don't, I am not a CFI expert, sorry.
Then I shall push these first and find way to put it to common code some time later. Thanks.
Is this any way to deal with feedback ? Why don't you put it into flash_erase() for example, in drivers/mtd/cfi_flash.c ?

Though altera-qspi works like CFI flash, it is not CFI flash as it does not have CFI info. It is a separate driver to cfi_flash.c.

Both drivers and many other parallel flash drivers under the board/arch directories are called by common/cmd_flash.c, which is very old and somewhat nasty. It could be the place for common feedback. But this is too far and deeply involved. I wanted to touch it a month ago, made some patches but withdrew then.

Best regards,
Thomas

Marek Vasut

2015-12-28 12:40 UTC #26 lore raw

PATCH 4/5 altera_qspi: show erase progress

Quoted 1 line · Thomas Chou

Hi Marek,

Hi!

Quoted 25 lines · Marek Vasut

Quoted 16 lines · Thomas Chou
Quoted 15 lines
Hi Marek,
Quoted 12 lines · Marek Vasut
Quoted 10 lines · Thomas Chou
Hi Marek,
Quoted 4 lines · Marek Vasut
Quoted 1 line
Well, it is only a putc() which is plain and simple.
Sure, but then I still don't understand why this cannot be in the common code.
Yes, it can be in the common code. Do you have an idea how should it look like?
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
No, I don't, I am not a CFI expert, sorry.
Then I shall push these first and find way to put it to common code some time later. Thanks.
Is this any way to deal with feedback ? Why don't you put it into flash_erase() for example, in drivers/mtd/cfi_flash.c ?

Quoted 8 lines

Though altera-qspi works like CFI flash, it is not CFI flash as it does not have CFI info. It is a separate driver to cfi_flash.c.
Both drivers and many other parallel flash drivers under the board/arch directories are called by common/cmd_flash.c, which is very old and somewhat nasty. It could be the place for common feedback. But this is too far and deeply involved. I wanted to touch it a month ago, made some patches but withdrew then.

I see, thanks for the clarification. OK then.

It'd be nice if you managed to clean that part up eventually :)

Best regards,
Marek Vasut

Thomas Chou

author

2015-12-28 01:35 UTC #27 lore raw

PATCH 4/5 altera_qspi: show erase progress

In reply to Thomas Chou

Quoted 7 lines · Thomas Chou

Show sector erase progress with dot and comma.
Signed-off-by Thomas Chou <[email protected]>
---
  drivers/mtd/altera_qspi.c | 14 ++++++++++++++
  1 file changed, 14 insertions(+)

Applied to u-boot-nios.

Thomas Chou

author

2015-12-24 00:51 UTC #28 lore raw

PATCH 5/5 altera_qspi: allow ctrl-c to abort the erase ops

In reply to Thomas Chou

Allow ctrl-c to abort the erase ops.

Signed-off-by Thomas Chou <[email protected]>

---
 drivers/mtd/altera_qspi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

drivers/mtd/altera_qspi.c +9 -0

index 0624ff4..a9148a7 100644
@@ -5,6 +5,7 @@
  */
 
 #include <common.h>
+#include <console.h>
 #include <dm.h>
 #include <errno.h>
 #include <fdt_support.h>

@@ -145,6 +146,14 @@ static int altera_qspi_erase(struct mtd_info *mtd, struct erase_info *instr)
 	instr->state = MTD_ERASING;
 	addr &= ~(mtd->erasesize - 1); /* get lower aligned address */
 	while (addr < end) {
+		if (ctrlc()) {
+			if (flash_verbose)
+				putc('\n');
+			instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
+			instr->state = MTD_ERASE_FAILED;
+			mtd_erase_callback(instr);
+			return -EIO;
+		}
 		flash = pdata->base + addr;
 		last = pdata->base + addr + mtd->erasesize;
 		/* skip erase if sector is blank */

Signature · 1 line

2.5.0

Thomas Chou

author

2015-12-28 01:35 UTC #29 lore raw

PATCH 5/5 altera_qspi: allow ctrl-c to abort the erase ops

Quoted 7 lines · Thomas Chou

Allow ctrl-c to abort the erase ops.
Signed-off-by Thomas Chou <[email protected]>
---
  drivers/mtd/altera_qspi.c | 9 +++++++++
  1 file changed, 9 insertions(+)

Applied to u-boot-nios.