[flashrom] [PATCH 1/3] Move Intel SPI initialisation to ichspi.c

[Michael Karcher]

Am Sonntag, den 18.07.2010, 17:24 +0200 schrieb Carl-Daniel Hailfinger:
> On 18.07.2010 16:19, Michael Karcher wrote:
> > +	ich_init_spi(dev, tmp, rcrb, ich_generation);
> check for the return value?
And do what in case of error? ich_init_spi should set CHIP_BUSTYPE_SPI
only if it worked, and ich_init_spi also knows better what failed if
initializing didn't work. Of course one can pass the error code to make
the chipset enable fail, but is it really fatal? (let me remind you:
this code path is also taken for FWH chips)
  
> > +#define ICH_BMWAG(x) ((x >> 24) & 0xff)
> > +#define ICH_BMRAG(x) ((x >> 16) & 0xff)
> > +#define ICH_BRWA(x)  ((x >>  8) & 0xff)
> > +#define ICH_BRRA(x)  ((x >>  0) & 0xff)
> > +
> > +#define ICH_FREG_BASE(x)  ((x >>  0) & 0x1fff)
> > +#define ICH_FREG_LIMIT(x) ((x >> 16) & 0x1fff)
> > +
> > +static void do_ich9_spi_frap(uint32_t frap, int i)
> > +{
> > +	const char *access_names[4] = {
> > +		"locked", "read-only", "write-only", "read-write"
> > +	};
> > +	const char *region_names[5] = {
> > +		"Flash Descriptor", "BIOS", "Management Engine",
> > +		"Gigabit Ethernet", "Platform Data"
> > +	};
> > +	uint32_t base, limit;
> > +	int rwperms = (((ICH_BRWA(frap) >> i) & 1) << 1) |
> > +		      (((ICH_BRRA(frap) >> i) & 1) << 0);
> > +	int offset = 0x54 + i * 4;
> > +	uint32_t freg = mmio_readl(ich_spibar + offset);
> > +
> > +	msg_pdbg("0x%02X: 0x%08x (FREG%i: %s)\n",
> > +		     offset, freg, i, region_names[i]);
> > +
> > +	base  = ICH_FREG_BASE(freg);
> > +	limit = ICH_FREG_LIMIT(freg);
> > +	if (base == 0x1fff && limit == 0) {
> > +		/* this FREG is disabled */
> > +		msg_pdbg("%s region is unused.\n", region_names[i]);
> > +		return;
> > +	}
> > +
> > +	msg_pdbg("0x%08x-0x%08x is %s\n",
> > +		    (base << 12), (limit << 12) | 0x0fff,
> > +		    access_names[rwperms]);
> > +}   
> Is the FRAP/FREG stuff really SPI specific? If not, it should stay in
> chipset_enable.c AFAICS.

The whole region stuff (and flash descriptors at all) are SPI specific,
as far as I can tell.

> > +void ich_init_spi(struct pci_dev *dev, uint32_t base, void * rcrb, 
> whitespace after * and at end of line
> Can you make this function return an error value?
Did so, but always returns success for now.

> Is adding BUSTYPE_FWH to buses_supported a good idea here in ichspi.c?
not really. Moved the FWH/no FWH handling back to chipset_enable.c

Thanks for the review, new version attached.

Regards,
  Michael Karcher
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Move-Intel-SPI-initialisation-to-ichspi.c.patch
Type: text/x-patch
Size: 15981 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20100718/9083bf35/attachment.patch>