[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 <c-d.hailfinger.devel.2006 at gmx.net>

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/