[RFC] flashrom: drop 82802ab.c or sharplhf00l04.c

[Carl-Daniel Hailfinger]

Hi,

flashrom/82802ab.c and flashrom/sharplhf00l04.c are basically identical
except for function names and pretty status printing. Can we please drop
one of them?
sharplhf00l04.c is slightly more verbose for erase, but that's it.

Regards,
Carl-Daniel

[Peter Stuge]

On Thu, Mar 13, 2008 at 12:49:49AM +0100, Carl-Daniel Hailfinger wrote:
> flashrom/82802ab.c and flashrom/sharplhf00l04.c are basically
> identical except for function names and pretty status printing.
> Can we please drop one of them?

Fine with me.

//Peter

[Carl-Daniel Hailfinger]

On 13.03.2008 02:39, Peter Stuge wrote:
> On Thu, Mar 13, 2008 at 12:49:49AM +0100, Carl-Daniel Hailfinger wrote:
> > flashrom/82802ab.c and flashrom/sharplhf00l04.c are basically
> > identical except for function names and pretty status printing.
> > Can we please drop one of them?
>
> Fine with me.

Drop 82802ab.c as it is identical to sharplhf00l04.c.
82802ab.c removal is done with "svn rm".

Signed-off-by: Carl-Daniel Hailfinger

Index: flashrom-fwh/82802ab.c
===================================================================
--- flashrom-fwh/82802ab.c	(Revision 3125)
+++ flashrom-fwh/82802ab.c	(Arbeitskopie)
@@ -1,186 +0,0 @@
-/*
- * This file is part of the flashrom project.
- *
- * Copyright (C) 2000 Silicon Integrated System Corporation
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
- */
-
-/*
- * Datasheet:
- *  - Name: Intel 82802AB/82802AC Firmware Hub (FWH)
- *  - URL: http://www.intel.com/design/chipsets/datashts/290658.htm
- *  - PDF: http://download.intel.com/design/chipsets/datashts/29065804.pdf
- *  - Order number: 290658-004
- */
-
-#include <stdio.h>
-#include <stdint.h>
-#include "flash.h"
-
-// I need that Berkeley bit-map printer
-void print_82802ab_status(uint8_t status)
-{
-	printf("%s", status & 0x80 ? "Ready:" : "Busy:");
-	printf("%s", status & 0x40 ? "BE SUSPEND:" : "BE RUN/FINISH:");
-	printf("%s", status & 0x20 ? "BE ERROR:" : "BE OK:");
-	printf("%s", status & 0x10 ? "PROG ERR:" : "PROG OK:");
-	printf("%s", status & 0x8 ? "VP ERR:" : "VPP OK:");
-	printf("%s", status & 0x4 ? "PROG SUSPEND:" : "PROG RUN/FINISH:");
-	printf("%s", status & 0x2 ? "WP|TBL#|WP#,ABORT:" : "UNLOCK:");
-}
-
-int probe_82802ab(struct flashchip *flash)
-{
-	volatile uint8_t *bios = flash->virtual_memory;
-	uint8_t id1, id2;
-
-#if 0
-	*(volatile uint8_t *)(bios + 0x5555) = 0xAA;
-	*(volatile uint8_t *)(bios + 0x2AAA) = 0x55;
-	*(volatile uint8_t *)(bios + 0x5555) = 0x90;
-#endif
-
-	*bios = 0xff;
-	myusec_delay(10);
-	*bios = 0x90;
-	myusec_delay(10);
-
-	id1 = *(volatile uint8_t *)bios;
-	id2 = *(volatile uint8_t *)(bios + 0x01);
-
-	/* Leave ID mode */
-	*(volatile uint8_t *)(bios + 0x5555) = 0xAA;
-	*(volatile uint8_t *)(bios + 0x2AAA) = 0x55;
-	*(volatile uint8_t *)(bios + 0x5555) = 0xF0;
-
-	myusec_delay(10);
-
-	printf_debug("%s: id1 0x%x, id2 0x%x\n", __FUNCTION__, id1, id2);
-
-	if (id1 != flash->manufacture_id || id2 != flash->model_id)
-		return 0;
-
-	map_flash_registers(flash);
-
-	return 1;
-}
-
-uint8_t wait_82802ab(volatile uint8_t *bios)
-{
-	uint8_t status;
-	uint8_t id1, id2;
-
-	*bios = 0x70;
-	if ((*bios & 0x80) == 0) {	// it's busy
-		while ((*bios & 0x80) == 0) ;
-	}
-
-	status = *bios;
-
-	// put another command to get out of status register mode
-
-	*bios = 0x90;
-	myusec_delay(10);
-
-	id1 = *(volatile uint8_t *)bios;
-	id2 = *(volatile uint8_t *)(bios + 0x01);
-
-	// this is needed to jam it out of "read id" mode
-	*(volatile uint8_t *)(bios + 0x5555) = 0xAA;
-	*(volatile uint8_t *)(bios + 0x2AAA) = 0x55;
-	*(volatile uint8_t *)(bios + 0x5555) = 0xF0;
-
-	return status;
-}
-
-int erase_82802ab_block(struct flashchip *flash, int offset)
-{
-	volatile uint8_t *bios = flash->virtual_memory + offset;
-	volatile uint8_t *wrprotect = flash->virtual_registers + offset + 2;
-	uint8_t status;
-
-	// clear status register
-	*bios = 0x50;
-	//printf("Erase at %p\n", bios);
-	// clear write protect
-	//printf("write protect is at %p\n", (wrprotect));
-	//printf("write protect is 0x%x\n", *(wrprotect));
-	*(wrprotect) = 0;
-	//printf("write protect is 0x%x\n", *(wrprotect));
-
-	// now start it
-	*(volatile uint8_t *)(bios) = 0x20;
-	*(volatile uint8_t *)(bios) = 0xd0;
-	myusec_delay(10);
-	// now let's see what the register is
-	status = wait_82802ab(flash->virtual_memory);
-	//print_82802ab_status(status);
-	printf("DONE BLOCK 0x%x\n", offset);
-
-	return 0;
-}
-
-int erase_82802ab(struct flashchip *flash)
-{
-	int i;
-	unsigned int total_size = flash->total_size * 1024;
-
-	printf("total_size is %d; flash->page_size is %d\n",
-	       total_size, flash->page_size);
-	for (i = 0; i < total_size; i += flash->page_size)
-		erase_82802ab_block(flash, i);
-	printf("DONE ERASE\n");
-
-	return 0;
-}
-
-void write_page_82802ab(volatile uint8_t *bios, uint8_t *src,
-			volatile uint8_t *dst, int page_size)
-{
-	int i;
-
-	for (i = 0; i < page_size; i++) {
-		/* transfer data from source to destination */
-		*dst = 0x40;
-		*dst++ = *src++;
-		wait_82802ab(bios);
-	}
-}
-
-int write_82802ab(struct flashchip *flash, uint8_t *buf)
-{
-	int i;
-	int total_size = flash->total_size * 1024;
-	int page_size = flash->page_size;
-	volatile uint8_t *bios = flash->virtual_memory;
-
-	erase_82802ab(flash);
-	if (*bios != 0xff) {
-		printf("ERASE FAILED\n");
-		return -1;
-	}
-	printf("Programming page: ");
-	for (i = 0; i < total_size / page_size; i++) {
-		printf("%04d at address: 0x%08x", i, i * page_size);
-		write_page_82802ab(bios, buf + i * page_size,
-				   bios + i * page_size, page_size);
-		printf("\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b\b");
-	}
-	printf("\n");
-	protect_jedec(bios);
-
-	return 0;
-}
Index: flashrom-fwh/Makefile
===================================================================
--- flashrom-fwh/Makefile	(Revision 3125)
+++ flashrom-fwh/Makefile	(Arbeitskopie)
@@ -22,7 +22,7 @@

OBJS = chipset_enable.o board_enable.o udelay.o jedec.o sst28sf040.o \
    am29f040b.o mx29f002.o sst39sf020.o m29f400bt.o w49f002u.o \
-	82802ab.o msys_doc.o pm49fl004.o sst49lf040.o sst49lfxxxc.o \
+	msys_doc.o pm49fl004.o sst49lf040.o sst49lfxxxc.o \
    sst_fwhub.o layout.o cbtable.o flashchips.o flashrom.o \
    sharplhf00l04.o w29ee011.o spi.o

Index: flashrom-fwh/flashchips.c
===================================================================
--- flashrom-fwh/flashchips.c	(Revision 3125)
+++ flashrom-fwh/flashchips.c	(Arbeitskopie)
@@ -192,10 +192,10 @@
     probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
    {"M25P128",	ST_ID,		ST_M25P128,	16384,	256,
     probe_spi,	generic_spi_chip_erase_c7,	generic_spi_chip_write, generic_spi_chip_read},
-	{"82802ab",	137,		173,		512, 64 * 1024,
-	 probe_82802ab, erase_82802ab,	write_82802ab},
-	{"82802ac",	137,		172,		1024, 64 * 1024,
-	 probe_82802ab, erase_82802ab,	write_82802ab},
+	{"82802ab",	INTEL_ID,	173,		512, 64 * 1024,
+	 probe_lhf00l04, erase_lhf00l04,	write_lhf00l04},
+	{"82802ac",	INTEL_ID,	172,		1024, 64 * 1024,
+	 probe_lhf00l04, erase_lhf00l04,	write_lhf00l04},
    {"F49B002UA",   EMST_ID,        EMST_F49B002UA, 256, 4096,
          probe_jedec,   erase_chip_jedec, write_49f002},
 #ifndef DISABLE_DOC

[Peter Stuge]

On Thu, Mar 13, 2008 at 01:22:40PM +0100, Carl-Daniel Hailfinger wrote:
> Drop 82802ab.c as it is identical to sharplhf00l04.c.
> 82802ab.c removal is done with "svn rm".
>
> Signed-off-by: Carl-Daniel Hailfinger

Acked-by: Peter Stuge

..with one caveat:
> --- flashrom-fwh/82802ab.c	(Revision 3125)
> -/*
> Datasheet:
> Name: Intel 82802AB/82802AC Firmware Hub (FWH)
> URL: http://www.intel.com/design/chipsets/datashts/290658.htm
> PDF: http://download.intel.com/design/chipsets/datashts/29065804.pdf
> Order number: 290658-004
> */

It would be nice to add this to the other file.

//Peter

[Carl-Daniel Hailfinger]

On 13.03.2008 13:26, Peter Stuge wrote:
> On Thu, Mar 13, 2008 at 01:22:40PM +0100, Carl-Daniel Hailfinger wrote:
> > Drop 82802ab.c as it is identical to sharplhf00l04.c.
> > 82802ab.c removal is done with "svn rm".
> >
> > Signed-off-by: Carl-Daniel Hailfinger
>
> Acked-by: Peter Stuge

Thanks!
> ..with one caveat:
> > --- flashrom-fwh/82802ab.c	(Revision 3125)
> > -/*
> > Datasheet:
> > Name: Intel 82802AB/82802AC Firmware Hub (FWH)
> > URL: http://www.intel.com/design/chipsets/datashts/290658.htm
> > PDF: http://download.intel.com/design/chipsets/datashts/29065804.pdf
> > Order number: 290658-004
> > */
>
> It would be nice to add this to the other file.

Actually, I did that, but patch generation was broken. Committed in
r3137 with the datasheet reference included.

Regards,
Carl-Daniel

[ron minnich]

you folks moved a tad fast. The sharp was an in-progress thing. It
never worked. Hence my more verbose comments.

I would rather have removed the sharp, if you were going to remove
something, but I would rather have not removed either of them.

The last time I had a board to test sharp on, it did not work.

Sorry about that.

ron

[Carl-Daniel Hailfinger]

On 13.03.2008 16:27, ron minnich wrote:
> you folks moved a tad fast. The sharp was an in-progress thing. It
> never worked. Hence my more verbose comments.

Now that explains it.
> I would rather have removed the sharp, if you were going to remove
> something, but I would rather have not removed either of them.

Should I revert or rename the sharp file to 82802ab.c?
> The last time I had a board to test sharp on, it did not work.

We really need to add another field to struct flashchip: "tested". Right
now, most chips are either completely untested or only detection was tested.

Regards,
Carl-Daniel

[Uwe Hermann]

On Thu, Mar 13, 2008 at 04:33:05PM +0100, Carl-Daniel Hailfinger wrote:
> We really need to add another field to struct flashchip: "tested". Right
> now, most chips are either completely untested or only detection was tested.

Yeah, that's probably a good idea. The field could be a string
containing one or more of P, R, W, E, for "probing was tested",
"reading was tested", "writing was tested", "erasing was tested".

Alternatively, we could track that information in the wiki table at
http://www.coreboot.org/Flashrom#Supported_devices
which might be easier/faster to update (no need for mail sending
and/or waiting for ACKs, and it can be updated by all users, not just
developers with commit access, etc).

Uwe.

> Alternatively, we could track that information in the wiki table at
> http://www.coreboot.org/Flashrom#Supported_devices
> which might be easier/faster to update (no need for mail sending
> and/or waiting for ACKs, and it can be updated by all users, not just
> developers with commit access, etc).

I like the wiki idea. This way people not on the mailing list (people  
interested but not yet commited) will still be able know the status of  
these chips:-)

Thanks - Joe

[Uwe Hermann]

On Thu, Mar 13, 2008 at 03:20:06PM -0400, [e-mail] wrote:
> > Alternatively, we could track that information in the wiki table at
> > http://www.coreboot.org/Flashrom#Supported_devices
> > which might be easier/faster to update (no need for mail sending
> > and/or waiting for ACKs, and it can be updated by all users, not just
> > developers with commit access, etc).
>
> I like the wiki idea. This way people not on the mailing list (people  
> interested but not yet commited) will still be able know the status of  
> these chips:-)

Yep. I've started reworking the wiki page so it tracks four status
fields, Probe status, Read status, Write status, Erase status.

Only chips verified to really work on actual hardware should be marked
as OK in that table. We should probably have a small paragraph about
proper testing procedures in the wiki page, too.

Uwe.

[ron minnich]

it would be interesting if the tool itself could generate output that
can be run through a filter to create the html.

That way keeping them in sync is simple.

Maybe not though ;-)

ron

Quoting ron minnich :
> it would be interesting if the tool itself could generate output that
> can be run through a filter to create the html.
>
> That way keeping them in sync is simple.
>
> Maybe not though ;-)
>
> ron

Wow I like this idea even better:-)

Thanks - Joe

[Carl-Daniel Hailfinger]

On 14.03.2008 00:35, [e-mail] wrote:
> Quoting ron minnich :
> > it would be interesting if the tool itself could generate output that
> > can be run through a filter to create the html.
> >
> > That way keeping them in sync is simple.
> >
> > Maybe not though ;-)
> >
> > ron
>
> Wow I like this idea even better:-)

Full Ack.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 13.03.2008 20:20, [e-mail] wrote:
> > Alternatively, we could track that information in the wiki table at
> > http://www.coreboot.org/Flashrom#Supported_devices
> > which might be easier/faster to update (no need for mail sending
> > and/or waiting for ACKs, and it can be updated by all users, not just
> > developers with commit access, etc).
>
> I like the wiki idea. This way people not on the mailing list (people
> interested but not yet commited) will still be able know the status of
> these chips:-)

But the wiki is almost always guaranteed to be wrong - unless the
flashrom version you use is the one referenced in the wiki.

Regards,
Carl-Daniel

[ron minnich]

On Thu, Mar 13, 2008 at 8:33 AM, Carl-Daniel Hailfinger
 wrote:
> > I would rather have removed the sharp, if you were going to remove
> > something, but I would rather have not removed either of them.
>
> Should I revert or rename the sharp file to 82802ab.c?

I think we should revert. The sharp file may not work for 802ab
> We really need to add another field to struct flashchip: "tested". Right
>  now, most chips are either completely untested or only detection was tested.

I like that, and then an option to flashrom:
./flashrom --flashchipstatus
or some such: and then you get:
chip (probe, read, write)
chip2 (probe)

and so on

ron

[Carl-Daniel Hailfinger]

On 13.03.2008 20:37, ron minnich wrote:
> On Thu, Mar 13, 2008 at 8:33 AM, Carl-Daniel Hailfinger
>  wrote:
> > > I would rather have removed the sharp, if you were going to remove
> > > something, but I would rather have not removed either of them.
> >
> > Should I revert or rename the sharp file to 82802ab.c?
>
> I think we should revert. The sharp file may not work for 802ab

Reverted with full history in r3140.
> > We really need to add another field to struct flashchip: "tested". Right
> >  now, most chips are either completely untested or only detection was tested.
>
> I like that, and then an option to flashrom:
> ./flashrom --flashchipstatus
> or some such: and then you get:
> chip (probe, read, write)
> chip2 (probe)
>
> and so on

How about a 32-bit bitfield for the status? This would allow not only
(probe, read, erase, write) but also stuff like (partial erase, partial
write) and maybe other status info as well for chips which are dual
FWH/LPC, but only one of the modes is supported.

Maybe an --autotest mode of flashrom would give people the opportunity
to check all possible functionality of the chip.

Regards,
Carl-Daniel

[Uwe Hermann]

On Thu, Mar 13, 2008 at 08:27:04AM -0700, ron minnich wrote:
> you folks moved a tad fast. The sharp was an in-progress thing. It
> never worked. Hence my more verbose comments.
>
> I would rather have removed the sharp, if you were going to remove
> something, but I would rather have not removed either of them.
>
> The last time I had a board to test sharp on, it did not work.

I think I had a 82802ab or sharp chip at some point, I'll check if I can
find it and test if the code (and which code) works. Can't promise
anything yet, though.

Uwe.