[flashrom] [PATCH] ICH9/10: display FRAP/FREGx access controls

[Carl-Daniel Hailfinger]

Hi,

On 29.06.2010 17:34, Joshua Roys wrote:
> I'm glad it seems useful!  Some questions inline:
>
> On 06/29/2010 08:12 AM, Carl-Daniel Hailfinger wrote:
>   
>>> +static void ich_spi_frap(uint32_t frap, int i)
>>>   
>>>       
>> Can you change the name to print_ich9_spi_frap?
>>     
>
> I was considering making this function twiddle some bits eventually (if
> it could).  The BM{W,R}AG fields can apparently override the FRAP access
> controls to the "BIOS Descriptor" region.  Unfortunately on my board,
> the regions were labeled as a flash descriptor and a ME section...
> do_ich9_spi_frap?
>   

OK.


>>> +
>>> +	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 */
>>> +		return;
>>>   
>>>       
>> This is a bit irritating in the output. Can you print a message that the
>> FREG is disabled? OTOH, "disabled" implies that the region is
>> inaccessible or does not exist. Looking at your log, what does it mean
>> that the BIOS FREG is disabled? No access? Full access? Should the
>> message be "unknown location, full access", or is that incorrect?
>>
>>     
>
> If I understand the datasheet correctly it simply means that there is no
> region in the flash chip that is considered as this (ME, GbE, etc).
> Maybe something like msg_pdbg("%s region is unused.", region_names[i]);
>   

But your output says:

> 0x54: 0x00010000 (FREG0: Flash Descriptor)
> 0x00000000-0x00001fff is Read-only
> 0x58: 0x00001fff (FREG1: BIOS Descriptor)
> 0x5C: 0x007f0002 (FREG2: ME)
> 0x00002000-0x0007ffff is Locked
> 0x60: 0x00001fff (FREG3: GbE)
> 0x64: 0x00001fff (FREG4: Platform Data)
>   

That would imply no region of the flash chip is considered to be
BIOS/GbE/Platformdata.

Maybe we need to dump the flash descriptor region and analyze it. I
think Matthias Wenzel wrote a tool for that:
http://github.com/mazzoo/descriptormode
Such an analysis would be best done in a separate patch, though.


> Thanks for the review,
>   

You're welcome. I'm confident we can commit the next iteration of your
patch.

Regards,
Carl-Daniel

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