[PATCH] cfi_flash: show progress while during erase

[PATCH] cfi_flash: show progress while during erase

[Franck Jullien]

Signed-off-by: Franck Jullien <franck.jullien@gmail.com>
---
 drivers/mtd/nor/cfi_flash.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nor/cfi_flash.c b/drivers/mtd/nor/cfi_flash.c
index 3d3d231..f2f52e1 100644
--- a/drivers/mtd/nor/cfi_flash.c
+++ b/drivers/mtd/nor/cfi_flash.c
@@ -470,7 +470,7 @@ flash_sect_t find_sector (struct flash_info *info, ulong addr)
 	return sector;
 }
 
-static int cfi_erase(struct flash_info *finfo, size_t count, loff_t offset)
+static int cfi_erase(struct flash_info *finfo, size_t count, loff_t offset, int verbose)
 {
         unsigned long start, end;
         int i, ret = 0;
@@ -481,17 +481,25 @@ static int cfi_erase(struct flash_info *finfo, size_t count, loff_t offset)
         end   = find_sector(finfo, (unsigned long)finfo->base + offset +
 			count - 1);
 
+	if (verbose)
+		init_progression_bar(end - start);
+
         for (i = start; i <= end; i++) {
                 ret = finfo->cfi_cmd_set->flash_erase_one(finfo, i);
                 if (ret)
                         goto out;
 
+		if (verbose)
+			show_progress(i - start);
+
 		if (ctrlc()) {
 			ret = -EINTR;
 			goto out;
 		}
         }
 out:
+	if (verbose)
+		putchar('\n');
         return ret;
 }
 
@@ -933,7 +941,7 @@ static int cfi_mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
 	struct flash_info *info = container_of(mtd, struct flash_info, mtd);
 	int ret;
 
-	ret = cfi_erase(info, instr->len, instr->addr);
+	ret = cfi_erase(info, instr->len, instr->addr, 1);
 
 	if (ret) {
 		instr->state = MTD_ERASE_FAILED;
-- 
1.7.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] cfi_flash: show progress while during erase

[Franck Jullien]

Sascha,

Any reason why you removed the progress bar here in this commit:
2749fbac48374b5f5ced ?

Franck.

2014-05-26 20:12 GMT+02:00 Franck Jullien <franck.jullien@gmail.com>:
> Signed-off-by: Franck Jullien <franck.jullien@gmail.com>
> ---
>  drivers/mtd/nor/cfi_flash.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nor/cfi_flash.c b/drivers/mtd/nor/cfi_flash.c
> index 3d3d231..f2f52e1 100644
> --- a/drivers/mtd/nor/cfi_flash.c
> +++ b/drivers/mtd/nor/cfi_flash.c
> @@ -470,7 +470,7 @@ flash_sect_t find_sector (struct flash_info *info, ulong addr)
>         return sector;
>  }
>
> -static int cfi_erase(struct flash_info *finfo, size_t count, loff_t offset)
> +static int cfi_erase(struct flash_info *finfo, size_t count, loff_t offset, int verbose)
>  {
>          unsigned long start, end;
>          int i, ret = 0;
> @@ -481,17 +481,25 @@ static int cfi_erase(struct flash_info *finfo, size_t count, loff_t offset)
>          end   = find_sector(finfo, (unsigned long)finfo->base + offset +
>                         count - 1);
>
> +       if (verbose)
> +               init_progression_bar(end - start);
> +
>          for (i = start; i <= end; i++) {
>                  ret = finfo->cfi_cmd_set->flash_erase_one(finfo, i);
>                  if (ret)
>                          goto out;
>
> +               if (verbose)
> +                       show_progress(i - start);
> +
>                 if (ctrlc()) {
>                         ret = -EINTR;
>                         goto out;
>                 }
>          }
>  out:
> +       if (verbose)
> +               putchar('\n');
>          return ret;
>  }
>
> @@ -933,7 +941,7 @@ static int cfi_mtd_erase(struct mtd_info *mtd, struct erase_info *instr)
>         struct flash_info *info = container_of(mtd, struct flash_info, mtd);
>         int ret;
>
> -       ret = cfi_erase(info, instr->len, instr->addr);
> +       ret = cfi_erase(info, instr->len, instr->addr, 1);
>
>         if (ret) {
>                 instr->state = MTD_ERASE_FAILED;
> --
> 1.7.1
>

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] cfi_flash: show progress while during erase

[Holger Schurig]

> -       ret = cfi_erase(info, instr->len, instr->addr);
> +       ret = cfi_erase(info, instr->len, instr->addr, 1);

Hmm, why this parameter?  Basically, three things would be sane:

* never output progress (no verbose parameter needed, obviously)
* always output progress (again no parameter needed)
* make it a command line option (parameter needed)

What you have here (in the current form) is a needless parameter to cfi_erase().

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] cfi_flash: show progress while during erase

[Franck Jullien]

2014-05-26 21:04 GMT+02:00 Holger Schurig <holgerschurig@gmail.com>:
>> -       ret = cfi_erase(info, instr->len, instr->addr);
>> +       ret = cfi_erase(info, instr->len, instr->addr, 1);
>
> Hmm, why this parameter?  Basically, three things would be sane:
>

I copied the code that was here before (I know that's not a good reason).
Well, this "1" means: if you want to make verbose an option, implement
this and replace "1" by whatever it needs.

> * never output progress (no verbose parameter needed, obviously)
> * always output progress (again no parameter needed)
> * make it a command line option (parameter needed)
>
> What you have here (in the current form) is a needless parameter to cfi_erase().

What would you think if we had a command line option to make erase quiet (-q) ?
By default we would have a progress bar displayed.

Franck.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] cfi_flash: show progress while during erase

[Sascha Hauer]

On Mon, May 26, 2014 at 08:20:52PM +0200, Franck Jullien wrote:
> Sascha,
> 
> Any reason why you removed the progress bar here in this commit:
> 2749fbac48374b5f5ced ?

The reason was that with the conversion of the cfi driver to mtd the
loop over the erase blocks was no longer in the cfi driver but in the
mtd layer. So if you see a progress bar for each erased block. You are
probably irritated because of this patch which came in later:

| commit 0d7ac7c3817e006cc4e258522a989642f1be1538
| Author: Sascha Hauer <s.hauer@pengutronix.de>
| Date:   Sat May 25 00:16:31 2013 +0200
| 
|     mtd: call mtd_erase with complete area if possible
|     
|     If a device does not have bad blocks loop over the eraseblocks
|     in the driver instead of the core. This allows the mtd_dataflash
|     driver to erase blocks instead of pages to gain more speed during
|     erasing. Also the mtd_dataflash driver modifies the erase_info
|     struct which causes the outer loop in the core to never end.
|     
|     Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
| 
| diff --git a/drivers/mtd/core.c b/drivers/mtd/core.c
| index 61744b6..f358098 100644
| --- a/drivers/mtd/core.c
| +++ b/drivers/mtd/core.c
| @@ -107,6 +107,12 @@ static int mtd_op_erase(struct cdev *cdev, size_t count, loff_t offset)
|  	memset(&erase, 0, sizeof(erase));
|  	erase.mtd = mtd;
|  	erase.addr = offset;
| +
| +	if (!mtd->block_isbad) {
| +		erase.len = count;
| +		return mtd_erase(mtd, &erase);
| +	}
| +
|  	erase.len = mtd->erasesize;
|  
|  	while (count > 0) {

This moves the eraseblock iteration back into the drivers when the
device does not have bad blocks, which is the case for cfi flashes.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] cfi_flash: show progress while during erase

[Franck Jullien]

2014-05-27 7:42 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>:
> On Mon, May 26, 2014 at 08:20:52PM +0200, Franck Jullien wrote:
>> Sascha,
>>
>> Any reason why you removed the progress bar here in this commit:
>> 2749fbac48374b5f5ced ?
>
> The reason was that with the conversion of the cfi driver to mtd the
> loop over the erase blocks was no longer in the cfi driver but in the
> mtd layer. So if you see a progress bar for each erased block. You are
> probably irritated because of this patch which came in later:
>
> | commit 0d7ac7c3817e006cc4e258522a989642f1be1538
> | Author: Sascha Hauer <s.hauer@pengutronix.de>
> | Date:   Sat May 25 00:16:31 2013 +0200
> |
> |     mtd: call mtd_erase with complete area if possible
> |
> |     If a device does not have bad blocks loop over the eraseblocks
> |     in the driver instead of the core. This allows the mtd_dataflash
> |     driver to erase blocks instead of pages to gain more speed during
> |     erasing. Also the mtd_dataflash driver modifies the erase_info
> |     struct which causes the outer loop in the core to never end.
> |
> |     Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> |
> | diff --git a/drivers/mtd/core.c b/drivers/mtd/core.c
> | index 61744b6..f358098 100644
> | --- a/drivers/mtd/core.c
> | +++ b/drivers/mtd/core.c
> | @@ -107,6 +107,12 @@ static int mtd_op_erase(struct cdev *cdev, size_t count, loff_t offset)
> |       memset(&erase, 0, sizeof(erase));
> |       erase.mtd = mtd;
> |       erase.addr = offset;
> | +
> | +     if (!mtd->block_isbad) {
> | +             erase.len = count;
> | +             return mtd_erase(mtd, &erase);
> | +     }
> | +
> |       erase.len = mtd->erasesize;
> |
> |       while (count > 0) {
>
> This moves the eraseblock iteration back into the drivers when the
> device does not have bad blocks, which is the case for cfi flashes.
>

Ok. So brings back the progress bar in the cfi_flash driver is good.

For NAND devices we could add a progress bar in the mtd core file.
However, I don't have a nand on my board so I can't test it.

Now, what would you think if we had a command line option to make
erase quiet (-q) ?
By default we would have a progress bar displayed.

Franck.

> Sascha
>
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] cfi_flash: show progress while during erase

[Sascha Hauer]

On Tue, May 27, 2014 at 08:50:08AM +0200, Franck Jullien wrote:
> 2014-05-27 7:42 GMT+02:00 Sascha Hauer <s.hauer@pengutronix.de>:
> > On Mon, May 26, 2014 at 08:20:52PM +0200, Franck Jullien wrote:
> >> Sascha,
> >>
> >> Any reason why you removed the progress bar here in this commit:
> >> 2749fbac48374b5f5ced ?
> >
> > The reason was that with the conversion of the cfi driver to mtd the
> > loop over the erase blocks was no longer in the cfi driver but in the
> > mtd layer. So if you see a progress bar for each erased block. You are
> > probably irritated because of this patch which came in later:
> >
> > | commit 0d7ac7c3817e006cc4e258522a989642f1be1538
> > | Author: Sascha Hauer <s.hauer@pengutronix.de>
> > | Date:   Sat May 25 00:16:31 2013 +0200
> > |
> > |     mtd: call mtd_erase with complete area if possible
> > |
> > |     If a device does not have bad blocks loop over the eraseblocks
> > |     in the driver instead of the core. This allows the mtd_dataflash
> > |     driver to erase blocks instead of pages to gain more speed during
> > |     erasing. Also the mtd_dataflash driver modifies the erase_info
> > |     struct which causes the outer loop in the core to never end.
> > |
> > |     Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > |
> > | diff --git a/drivers/mtd/core.c b/drivers/mtd/core.c
> > | index 61744b6..f358098 100644
> > | --- a/drivers/mtd/core.c
> > | +++ b/drivers/mtd/core.c
> > | @@ -107,6 +107,12 @@ static int mtd_op_erase(struct cdev *cdev, size_t count, loff_t offset)
> > |       memset(&erase, 0, sizeof(erase));
> > |       erase.mtd = mtd;
> > |       erase.addr = offset;
> > | +
> > | +     if (!mtd->block_isbad) {
> > | +             erase.len = count;
> > | +             return mtd_erase(mtd, &erase);
> > | +     }
> > | +
> > |       erase.len = mtd->erasesize;
> > |
> > |       while (count > 0) {
> >
> > This moves the eraseblock iteration back into the drivers when the
> > device does not have bad blocks, which is the case for cfi flashes.
> >
> 
> Ok. So brings back the progress bar in the cfi_flash driver is good.
> 
> For NAND devices we could add a progress bar in the mtd core file.
> However, I don't have a nand on my board so I can't test it.

For NAND we don't need a progress bar since the erase operation is very
fast. SPI NOR flashes could benefit from a progress bar though. However,
since the eraseblock looping is in the driver the progress bar must be
implemented there too.

> 
> Now, what would you think if we had a command line option to make
> erase quiet (-q) ?
> By default we would have a progress bar displayed.

I would prefer a -v (verbose) option instead. That would be in line with
the cp command.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox