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

[Carl-Daniel Hailfinger]

On 18.07.2010 18:29, Michael Karcher wrote:
> 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)

In my latest Nvidia MCP SPI patch, I'm solving this with an additional
boolean parameter want_spi. If the chipset is strapped to SPI, I set
want_spi=1 and return an error code if SPI init fails. If the chipset is
not strapped to SPI, want_spi is set to 0 and the code tries SPI init
but will never return an error.
> > > +#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.

OK, thanks.
> > > +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.

Looks good. The want_spi stuff mentioned above may be desirable, but I
don't want to force this on you.

One additional comment: Could you move do_ich9_spi_frap down a bit so it
is in the same region as the init functions?
With that addressed, your patch is
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel