[coreboot] Add support for SPI-Chips on ICH9 to flash rom / corrected

Peter Stuge peter at stuge.se
Tue May 13 15:14:15 CEST 2008


On Tue, May 13, 2008 at 01:56:38PM +0200, Uwe Hermann wrote:
> > +	{"ST",		"M25P40 at ICH9",		ST_ID,		ST_M25P40,		512,	64 * 1024,	TEST_UNTESTED,	probe_ichspi_stm25_sig,	erase_ichspi,		write_ichspi, read_ichspi},
> > +	{"ST",		"M25P80 at ICH9",		ST_ID,		ST_M25P80,		1024,	64 * 1024,	TEST_UNTESTED,	probe_ichspi_stm25_sig,	erase_ichspi,		write_ichspi, read_ichspi},
> > +	{"ST",		"M25P16 at ICH9",		ST_ID,		ST_M25P16,		2048,	64 * 1024,	TEST_UNTESTED,	probe_ichspi_stm25,	erase_ichspi,		write_ichspi, read_ichspi},
> > +	{"ST",		"M25P32 at ICH9",		ST_ID,		ST_M25P32,		4096,	64 * 1024,	TEST_UNTESTED,	probe_ichspi_stm25,		erase_ichspi,		write_ichspi, read_ichspi},
> > +	{"ST",		"M25P64 at ICH9",		ST_ID,		ST_M25P64,		8192,	64 * 1024,	TEST_UNTESTED,	probe_ichspi_stm25,		erase_ichspi,		write_ichspi, read_ichspi},
> 
> Why "@ICH9", does the chip behave differently depending on where
> you plug it in?

How to program an SPI flash chip depends on both the chip type, and
the SPI master type. (Ie which superio/southbridge it is connected
to.)

Currently, flashrom does not have infrastructure that can deal with a
non-transparent flash memory bus master, so the support for these
flash chips that Claus is proposing will only work on ICH9 systems.

This sucks of course, but it is a flaw of flashrom and I do think it
is OK to include Claus' changes because they do work for him.

It is too much to expect a brand new contributor to rewrite the SPI
infrastructure in flashrom. That will come, I know Carl-Daniel has
ideas.


> > Index: ichspi.c
..
> > + * Initially implemented by Stefan Wildemann <stefan.wildemann at kontron.com>
> > + * Adapted to flashrom by Claus Gindhart <claus.gindhart at kontron.com>
> 
> Please change these two lines to the usual Copyright lines, i.e.
> 
> Copyright (C) 200x Stefan Wildemann <stefan.wildemann at kontron.com>
> Copyright (C) 200y Claus Gindhart <claus.gindhart at kontron.com>
> 
> replacing the 200x/200y years appropriately.

Good point.


> > +#undef DEVELOPPERS_DEBUG
> 
> I'd drop this, see below.

Yes, I agree.


> > Index: ichspiacc.h
..
> Please add the same copyright header here as is the *.c files.
> Every file should have one.

Yep.


> > +// Bit defs
> > +#define BIT00                    0x0001
..
> > +#define BIT31                0x80000000
> 
> This is a bit elaborate. I'd drop it and just
> use (1 << 5) , (1 << 17) etc. as needed. Besides, they don't seem
> to be used currently anyway.

Hehe, nice one. I missed that.


//Peter




More information about the coreboot mailing list