[coreboot] [PATCH] Move cmos.default handling to bootblock

Patrick Georgi patrick at georgi-clan.de
Fri Mar 4 21:07:30 CET 2011

Am 04.03.2011 09:32, schrieb Stefan Reinauer:
>> index 895a185..a808cec 100644
>> --- a/src/arch/x86/include/bootblock_common.h
>> +++ b/src/arch/x86/include/bootblock_common.h
>> @@ -17,17 +17,45 @@ static void bootblock_northbridge_init(void) { }
>>  static void bootblock_southbridge_init(void) { }
>>  #endif
>> -static unsigned long findstage(char* target)
>> +static void *walkcbfs(char *target)
>>  {
>> -	unsigned long entry;
>> +	void *entry;
>>  	asm volatile (
>>  		"mov $1f, %%esp\n\t"
>> -		"jmp walkcbfs\n\t"
>> +		"jmp walkcbfs_asm\n\t"
> maybe just call it _walkcbfs ?
I thought about it, but I wanted to avoid confusion in case some
compiler uses _ prefixes (in which case that would be __walkcbfs).

>> +/* just enough to support findstage. copied because the original version doesn't easily pass through romcc */
>> +struct cbfs_stage {
>> +	unsigned long compression;
>> +	unsigned long entry; // this is really 64bit, but properly endianized
> Would it make sense to add an unsigned long entry_high after this, in
> this case? Or use a union or uint64_t for entry?
unsigned long entry_high; is better (romcc and all that)

>> +#include <pc80/mc146818rtc.h>
> Since you start using cmos in the bootblock, you might have to consider
> enabling RCBA and the upper 128 bytes of CMOS in some Intel
> southbridges' bootblock.c
The recovery code explicitely covers the first 128 bytes only - so far
the upper 128 bytes are used for suspend-to-ram data only, so this isn't
urgent yet.

RCBA, yes.. I'll take a look, but I think it worked when I tested it.

>> +static void sanitize_cmos(void)
>> +{
>> +	if (cmos_error() || !cmos_chksum_valid()) {
> Is this reliably working on hardware? I remember cmos_error being flaky
> on ICH7 early on at some point.
And I vaguely remember that we managed to stabilize this by tweaking

> How does cmos recovery behave on non-CAR systems if this is removed? It
> would be nice to if we could make sure it works everywhere
The only caller of this function is bootblock_normal - so by default,
this code wasn't used at all.

The patch works on qemu, which is a "non-CAR" system, right?


More information about the coreboot mailing list