Re: [flashrom] [PATCH] Don't ignore wait_82802ab() return value

[Stefan Reinauer]

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

[PATCH] Don't ignore wait_82802ab() return value

[Carl-Daniel Hailfinger]

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

[PATCH] Don't ignore wait_82802ab() return value

[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.

Uwe.

[PATCH] Print FWH chip status only if something is wrong

[Michael Karcher]

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 
---
 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");

[PATCH] Don't ignore wait_82802ab() return value

[Michael Karcher]

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