| Message ID | 1355934463-24319-1-git-send-email-jagannadh.teki@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
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
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..."
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
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
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
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;
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(-)