Patchwork [U-Boot,U-Boot,v2,1/6] cmd_sf: Add print messages on flash erase command

login
register
mail settings
Submitter Jagannadha Sutradharudu Teki
Date Dec. 19, 2012, 4:27 p.m.
Message ID <1355934463-24319-1-git-send-email-jagannadh.teki@gmail.com>
Download mbox | patch
Permalink /patch/207452/
State Superseded
Delegated to: Jagannadha Sutradharudu Teki
Headers show

Comments

Jagannadha Sutradharudu Teki - Dec. 19, 2012, 4:27 p.m.
This patch adds a print messages while using 'sf erase' command
to make sure that how many bytes erased in flash device.

Signed-off-by: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
---
Changes in v2:
	Move print messages from spi_flash.c into cmd_sf.c
	
 common/cmd_sf.c             |   11 ++++++-----
 drivers/mtd/spi/spi_flash.c |    2 --
 2 files changed, 6 insertions(+), 7 deletions(-)
Wolfgang Denk - Dec. 19, 2012, 11:16 p.m.
Dear Jagannadha Sutradharudu Teki,

In message <1355934463-24319-1-git-send-email-jagannadh.teki@gmail.com> you wrote:
> This patch adds a print messages while using 'sf erase' command
> to make sure that how many bytes erased in flash device.
> 
> Signed-off-by: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
> ---
> Changes in v2:
> 	Move print messages from spi_flash.c into cmd_sf.c

This makes little sense to me.  Please see all comments here:

http://article.gmane.org/gmane.comp.boot-loaders.u-boot/149663

and here:

http://article.gmane.org/gmane.comp.boot-loaders.u-boot/149665



Best regards,

Wolfgang Denk
Jagannadha Sutradharudu Teki - Dec. 20, 2012, 7:46 a.m.
Hi Wolfgang Denk,

On Thu, Dec 20, 2012 at 4:46 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Jagannadha Sutradharudu Teki,
>
> In message <1355934463-24319-1-git-send-email-jagannadh.teki@gmail.com> you wrote:
>> This patch adds a print messages while using 'sf erase' command
>> to make sure that how many bytes erased in flash device.
>>
>> Signed-off-by: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
>> ---
>> Changes in v2:
>>       Move print messages from spi_flash.c into cmd_sf.c
>
> This makes little sense to me.  Please see all comments here:
>
> http://article.gmane.org/gmane.comp.boot-loaders.u-boot/149663
>
> and here:
>
> http://article.gmane.org/gmane.comp.boot-loaders.u-boot/149665

I thought these are useful verbose prints for sf read/write/erase commands as
there is no verbose support before.

Like NOR, NAND and SD have some verbose prints while executing respective
commands
This is the only way to know the user whether these commands are
- executed properly
- what is the status after executing (success/fail, how many bytes
read/write/erase. etc)

Due to the above reasons I have added these verbose support.

Apart from this sometimes (very rare) due to the slowness of UART or SPI flash
even if we run the sf commands it will not execute the actual code
just terminate with showing
u-boot prompt, so the user assumes that this command is happen
properly. [but actually command is not done]

I thought these verbose prints will help such wild scenarios.

Correct me if am wrong with my thinking.

Thanks,
Jagan.

>
>
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> Programmer's Lament: (Shakespeare, Macbeth, Act I, Scene vii)
>         "That we but teach bloody instructions,
>         which, being taught, return to plague the inventor..."
Wolfgang Denk - Dec. 20, 2012, 12:49 p.m.
Dear Jagan Teki,

In message <CAD6G_RRw=YgbYtkwaZMC=KaS_5Lbfbm+fR-=LV96aOY72W383w@mail.gmail.com> you wrote:
>
> Apart from this sometimes (very rare) due to the slowness of UART or SPI flash
> even if we run the sf commands it will not execute the actual code
> just terminate with showing
> u-boot prompt, so the user assumes that this command is happen
> properly. [but actually command is not done]

But that would be a different thing - if there are errors without
clear error messages, this is a bug that needs fixing.  [But I do not
see which part of your patch would address such an issue.  Am I
missing something?]

Adding verbose progress messages is a different thing, though.

Best regards,

Wolfgang Denk
Jagannadha Sutradharudu Teki - Dec. 21, 2012, 5:50 a.m.
Hi Wolfgang Denk,

On Thu, Dec 20, 2012 at 6:19 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Jagan Teki,
>
> In message <CAD6G_RRw=YgbYtkwaZMC=KaS_5Lbfbm+fR-=LV96aOY72W383w@mail.gmail.com> you wrote:
>>
>> Apart from this sometimes (very rare) due to the slowness of UART or SPI flash
>> even if we run the sf commands it will not execute the actual code
>> just terminate with showing
>> u-boot prompt, so the user assumes that this command is happen
>> properly. [but actually command is not done]
>
> But that would be a different thing - if there are errors without
> clear error messages, this is a bug that needs fixing.  [But I do not
> see which part of your patch would address such an issue.  Am I
> missing something?]

Basically if there is an error while executing these commands,
then this print will show an :ERROR based on the return value from
spi_flash_erase.
---- of-course there is a verbose print already if spi_flash_erase
returns 1 (error case).

OK, I agree my patch will show only verbose prints for both in Success
and Failure cases.

I thought it could be a good progress prints for sf read/write/erase
commands as we didn't have
these verbose prints before.

Do you think these are useful/required messages?, please take my above concerns.

Thanks,
Jagan.

>
> Adding verbose progress messages is a different thing, though.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> It is much easier to suggest solutions when you know nothing
Wolfgang Denk - Dec. 21, 2012, 6:26 a.m.
Dear Jagan Teki,

In message <CAD6G_RQtZZinzcx0haLqR-JD4cjM8WyjZZGgarZ-+DwaZJnR2g@mail.gmail.com> you wrote:
> 
> I thought it could be a good progress prints for sf read/write/erase
> commands as we didn't have
> these verbose prints before.
> 
> Do you think these are useful/required messages?, please take my above concerns.

Some people will like such verbocity, others will dislike it.  Also,
some people will dislike th memory footprint that comes with the added
code.  So in any case this should be configurable.

But most of all: SF is just one storage device out of a much larger
list.  If we add such a feature, it should be done in common code and
in a generic way that all similar devices can use as well.

Please see again the other thread, and Simon's response.  I think it
makes sense to handle these things together, as a common set of
patches.


Best regards,

Wolfgang Denk

Patch

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index 5ac1d0c..ddb1a65 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -266,13 +266,14 @@  static int do_spi_flash_erase(int argc, char * const argv[])
 		return 1;
 	}
 
+	printf("SF: Erasing flash... ");
+
 	ret = spi_flash_erase(flash, offset, len);
-	if (ret) {
-		printf("SPI flash %s failed\n", argv[0]);
-		return 1;
-	}
 
-	return 0;
+	printf("%zu bytes @ %#x erased: %s\n", (size_t)len, (u32)offset,
+			ret ? "ERROR" : "OK");
+
+	return ret == 0 ? 0 : 1;
 }
 
 static int do_spi_flash(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 00aece9..43e0334 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -235,8 +235,6 @@  int spi_flash_cmd_erase(struct spi_flash *flash, u32 offset, size_t len)
 			goto out;
 	}
 
-	debug("SF: Successfully erased %zu bytes @ %#x\n", len, start);
-
  out:
 	spi_release_bus(flash->spi);
 	return ret;