[coreboot] [flashrom] [PATCH] superiotool: libsuperiodetect

[Carl-Daniel Hailfinger]

On 30.06.2010 16:36, Stefan Reinauer wrote:
> On 6/30/10 4:16 PM, Carl-Daniel Hailfinger wrote:
>   
>>> 15kb lzma compressed is nothing.
>>>       
>>>       
>> 8 kB more or less in a recovery payload of a coreboot image are not
>> negligible IMHO.
>>
>> In normal OS environments people don't use compressed binaries, and
>> there a 2 MB size increase for flashrom (98% of it for dead code which
>> can't be optimized away by the compiler/linker) is definitely something
>> people will object to.
>>   
>>     
> Well...
>
> - once we have a recovery payload (which will have to load a 1MB rom
> image from _somewhere_ as we speak of size) and
>   

If it is recovery over serial at 115200 bit/s (14 kByte/s), 2 MB more or
less make a difference. However, if that recovery loader also has
decompression support, that's different.


> - once we have people complaining
>
> we should definitely think about such optimizations. My bet is by then
> anything below 8mbit flash parts has died out. :)
>   

I hope the same, but I'm not as optimistic as you are.


>> IMHO the {EOT} markers should be killed in libsuperiodetect, at least
>>     
>>>> for the LDNs. Such an operation would have increased the size of the
>>>> patch and would have reduced readability as well. Due to that, I skipped
>>>> it, but I plan to do that in a later operation.
>>>>       
>>>>         
>>> What would you use instead? The array members all have different sizes
>>> as far as I can tell, so some kind of end marker will be needed.
>>>   
>>>     
>>>       
>> The libsuperiodetect case doesn't care about LDNs at all. Having exactly
>> one LDN definition per Super I/O which is always EOT looks like dead
>> code to me, especially if nothing is looking at the LDN.
>>   
>>     
> Maybe using the tables from superiotool is not the best way to solve the
> problem you are trying to solve at all?
>
> What is that problem anyways? Since flashrom has mechanisms to figure
> out the board, and the super io is a hard component on the board, will
> you need anything more but the numeric ID of the chip?
>   

Good point.
Many mainboards use Super I/O chips to translate flash buses, usually
LPC->SPI for newer boards and LPC->Parallel for older boards.
A few months ago, we had to handle every single board with such a
feature with a board enable, and the board enable table grew fast, even
for boards which didn't need any GPIO twiddling for write enables. Since
then, flashrom has evolved to autodetect ITE IT87* LPC->SPI translation,
and this allowed us to support a few dozen boards out of the box without
any board enables. flashrom has a few pending patches which add
autodetection for ITE and Winbond LPC->Parallel translation, and those
patches will allow us to kill a dozen board enables, and hundreds of
boards will start working automatically.

For automatic Super I/O probing to work well, I'd like to reuse some
proven code instead of rewriting the whole probing stuff from scratch.
Two possible sources of code exist for this:
- superiotool
- Linux kernel
Superiotool focuses on more recent (less than 12 years old) chips,
whereas the Linux kernel focuses on chips from the 90s. Both codebases
get some testing, and superiotool is regularly extended to support new
chips.

So flashrom needs the probing, and it also needs a function in each chip
definition which takes care of handling any translation functionality.
With that in mind, I picked the #ifdef solution because it would have
allowed me to simply run unifdef over the superiotool source code, then
import the generated files into the flashrom source without manual editing.

OTOH, I could write a Coccinelle semantic patch which eliminates the
need to touch any chip definitions in the superiotool source, and simply
run the semantic patch locally before copying over the patched files.

Printing the name (instead of a cryptic ID) of the detected Super I/O
chip is just for user friendliness.


>>>>  {
>>>>  	uint8_t id, rev;
>>>> +#ifndef LIBSUPERIODETECT
>>>>  	uint16_t runtime_base;
>>>> +#endif /* ! LIBSUPERIODETECT */
>>>>   
>>>>     
>>>>       
>>>>         
>>> @@ -765,7 +803,9 @@
>>> Maybe this could be moved downwards to where it is used, to save an ifndef?
>>>   
>>>     
>>>       
>> IIRC this would break on some compilers which don't allow variable
>> declarations in the middle of some code.
>>     
>
> Ok, which ones of them are supported for compiling flashrom?
>   

Well, it breaks if we specify -std=c90 or -ansi to gcc, and superiotool
has -ansi in the Makefile.


> The way superiotool defines functions also breaks K&R style.
>   

I won't comment on coding style in superiotool because I haven't touched
it for quite some time, and coding style discussions usually lead to
frustration.

I hope my mail helped to clarify the goals I want to achieve.

Regards,
Carl-Daniel

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