Re: [flashrom] [PATCH] Don't ignore wait_82802ab() return value
On 5/14/10 3:06 PM, Michael Karcher wrote:
Am Freitag, den 14.05.2010, 14:48 +0200 schrieb Stefan Reinauer:
On 4/16/10 6:21 PM, Stefan Reinauer wrote:
Look at the return value of wait_82802ab() Index: sst49lfxxxc.c =================================================================== --- sst49lfxxxc.c (revision 993) +++ sst49lfxxxc.c (working copy) @@ -84,6 +84,7 @@ chip_writeb(0xD0, bios + address);
status = wait_82802ab(bios); + print_status_82802ab(status);
[...] basically a good idea, but printing one status dump per erased block seems a bit excessive, even in verbose mode, as there are some chips with 4K sector size. Could you limit status printing to "status != 0x80", as this is "ready, no error bits"?
That's what the 82802ab and sharp lhf00l04 do, too. So if you think the behavior should be changed, you should change it in all three places. Stefan
On 14.05.2010 15:51, Stefan Reinauer wrote:
On 5/14/10 3:06 PM, Michael Karcher wrote:
Am Freitag, den 14.05.2010, 14:48 +0200 schrieb Stefan Reinauer:
On 4/16/10 6:21 PM, Stefan Reinauer wrote:
Look at the return value of wait_82802ab() Index: sst49lfxxxc.c =================================================================== --- sst49lfxxxc.c (revision 993) +++ sst49lfxxxc.c (working copy) @@ -84,6 +84,7 @@ chip_writeb(0xD0, bios + address);
status = wait_82802ab(bios); + print_status_82802ab(status);
[...] basically a good idea, but printing one status dump per erased block seems a bit excessive, even in verbose mode, as there are some chips with 4K sector size. Could you limit status printing to "status != 0x80", as this is "ready, no error bits"?
That's what the 82802ab and sharp lhf00l04 do, too. So if you think the behavior should be changed, you should change it in all three places.
That's the plan. Regards, Carl-Daniel -- http://www.hailfinger.org/
On Sat, May 15, 2010 at 12:29:18AM +0200, Carl-Daniel Hailfinger wrote:
Look at the return value of wait_82802ab() Index: sst49lfxxxc.c =================================================================== --- sst49lfxxxc.c (revision 993) +++ sst49lfxxxc.c (working copy) @@ -84,6 +84,7 @@ chip_writeb(0xD0, bios + address);
status = wait_82802ab(bios); + print_status_82802ab(status);
[...] basically a good idea, but printing one status dump per erased block seems a bit excessive, even in verbose mode, as there are some chips with 4K sector size. Could you limit status printing to "status != 0x80", as this is "ready, no error bits"?
That's what the 82802ab and sharp lhf00l04 do, too. So if you think the behavior should be changed, you should change it in all three places.
That's the plan.
In the mean-time I guess we can get this in (r1347), if only to reduce the patch queue size. Uwe. -- http://hermann-uwe.de | http://sigrok.org http://randomprojects.org | http://unmaintained-free-software.org
This is especially important for the SST FWH compatible chips with 4K sector size, where status printing for that small size was omitted up to r1347. Signed-off-by: Michael Karcher <flashrom@mkarcher.dialup.fu-berlin.de> --- 82802ab.c | 3 ++- sharplhf00l04.c | 6 ++++-- sst49lfxxxc.c | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/82802ab.c b/82802ab.c index 09b95e6..9c82c35 100644 --- a/82802ab.c +++ b/82802ab.c @@ -134,7 +134,8 @@ int erase_block_82802ab(struct flashchip *flash, unsigned int page, unsigned int // now let's see what the register is status = wait_82802ab(flash); - print_status_82802ab(status); + if (status != 0x80) /* Ready, no error bits */ + print_status_82802ab(status); if (check_erased_range(flash, page, pagesize)) { msg_cerr("ERASE FAILED!\n"); diff --git a/sharplhf00l04.c b/sharplhf00l04.c index f21950a..584f0b3 100644 --- a/sharplhf00l04.c +++ b/sharplhf00l04.c @@ -35,7 +35,8 @@ int erase_lhf00l04_block(struct flashchip *flash, unsigned int blockaddr, unsign // clear status register chip_writeb(0x50, bios); status = wait_82802ab(flash); - print_status_82802ab(status); + if (status != 0x80) /* Ready, no error bits */ + print_status_82802ab(status); // clear write protect msg_cspew("write protect is at 0x%lx\n", (wrprotect)); msg_cspew("write protect is 0x%x\n", chip_readb(wrprotect)); @@ -48,7 +49,8 @@ int erase_lhf00l04_block(struct flashchip *flash, unsigned int blockaddr, unsign programmer_delay(10); // now let's see what the register is status = wait_82802ab(flash); - print_status_82802ab(status); + if (status != 0x80) /* Ready, no error bits */ + print_status_82802ab(status); if (check_erased_range(flash, blockaddr, blocklen)) { msg_cerr("ERASE FAILED!\n"); diff --git a/sst49lfxxxc.c b/sst49lfxxxc.c index 392d7a8..bca49cd 100644 --- a/sst49lfxxxc.c +++ b/sst49lfxxxc.c @@ -68,7 +68,8 @@ int erase_sector_49lfxxxc(struct flashchip *flash, unsigned int address, unsigne chip_writeb(0xD0, bios + address); status = wait_82802ab(flash); - print_status_82802ab(status); + if (status != 0x80) /* Ready, no error bits */ + print_status_82802ab(status); if (check_erased_range(flash, address, sector_size)) { msg_cerr("ERASE FAILED!\n"); -- 1.7.5.3
Am Sonntag, den 19.06.2011, 19:11 +0200 schrieb Uwe Hermann:
On Sat, May 15, 2010 at 12:29:18AM +0200, Carl-Daniel Hailfinger wrote:
Look at the return value of wait_82802ab() Index: sst49lfxxxc.c =================================================================== --- sst49lfxxxc.c (revision 993) +++ sst49lfxxxc.c (working copy) @@ -84,6 +84,7 @@ chip_writeb(0xD0, bios + address);
status = wait_82802ab(bios); + print_status_82802ab(status);
[...] basically a good idea, but printing one status dump per erased block seems a bit excessive, even in verbose mode, as there are some chips with 4K sector size. Could you limit status printing to "status != 0x80", as this is "ready, no error bits"?
That's what the 82802ab and sharp lhf00l04 do, too. So if you think the behavior should be changed, you should change it in all three places.
That's the plan.
In the mean-time I guess we can get this in (r1347), if only to reduce the patch queue size.
Getting this in is a good idea, as IIRC this is the point gcc 4.6 disliked (for not using the status variable). Not printing the chip status 512 times also is a good ides, so I sent a patch that implements "print status only if abnormal". Sorry for working against getting the patch queue small, but we shouldn't forget it. BTW: the 82802ab and lhf00l04 *block* erase procedures operate on blocks around 64k, while the sst49 *sector* erase procedure that just was changed in this commit is for 4k sectors, so dumping the status here is much more excessive as in the other cases - probably that's the reason why the status printing was not in. Regards, Michael Karcher
participants (4)
-
Carl-Daniel Hailfinger -
Michael Karcher -
Stefan Reinauer -
Uwe Hermann