[flashrom] [PATCH 1/3] Move Intel SPI initialisation to ichspi.c
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Thu Jul 22 17:09:52 CEST 2010
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 <c-d.hailfinger.devel.2006 at gmx.net>
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the flashrom
mailing list