[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