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

[Stefan Reinauer]

On 2/1/11 2:50 AM, Patrick Georgi wrote:
> The cmos.default code wasn't actually used so far, due to an oversight
> when forward-porting this feature from an old branch.
> Extend walkcbfs' use by factoring out the stage handling into C code.
> New sanitize_cmos() function that looks if CMOS data is invalid and
>  cmos.default exists and if so overwrites CMOS with cmos.default data.
> Use sanitize_cmos() in both bootblock implementations.
> Drop the need to reboot after writing CMOS: CMOS wasn't used so far,
>  so we can go on without a reboot.
> Remove the restriction that cmos.default only works on CAR boards.
> Always build in cmos.default support on boards that USE_OPTION_TABLE.
>
> Signed-off-by: Patrick Georgi

Acked-by: Stefan Reinauer

with some optional comments below
> 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 ?
> +/* 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?
> +#if CONFIG_USE_OPTION_TABLE
> +#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
> +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.
> diff --git a/src/pc80/mc146818rtc_early.c b/src/pc80/mc146818rtc_early.c
> index 920deda..d09d6b9 100644
> --- a/src/pc80/mc146818rtc_early.c
> +++ b/src/pc80/mc146818rtc_early.c
> @@ -11,15 +11,6 @@
>   #error "CONFIG_MAX_REBOOT_CNT too high"
>   #endif
>
> -#if CONFIG_USE_CMOS_RECOVERY
> -#include<cbfs.h>
> -#include<console/loglevel.h>
>
> -int do_printk(int msg_level, const char *fmt, ...) __attribute__((format(printf, 2, 3)));
> -#define printk_warning(fmt, arg...) do_printk(BIOS_WARNING ,fmt, ##arg)
> -#define printk_debug(fmt, arg...) do_printk(BIOS_DEBUG ,fmt, ##arg)
> -#endif
> static int cmos_error(void)
> {
>  unsigned char reg_d;
>
> @@ -63,25 +54,6 @@ static inline int do_normal_boot(void)
>    unsigned char byte;
>
> if (cmos_error() || !cmos_chksum_valid()) {
> -#if CONFIG_USE_CMOS_RECOVERY
> char *cmos_default = cbfs_find_file("cmos.default", CBFS_COMPONENT_CMOS_DEFAULT);
> if (cmos_default) {
> 	int i;
> 	printk_warning("WARNING - CMOS CORRUPTED. RESTORING DEFAULTS.\n");
> 	/* First 14 bytes are reserved for
> 	   RTC and ignored by nvramtool, too.
> 	   Only 128 bytes: 128+ requires cmos configuration and
> 	   contains only suspend-to-ram data, which isn't part
> 	   of the recovery procedure. */
> 	for (i = 14; i<  128; i++) {
> 		cmos_write(cmos_default[i], i);
> 	}
> 	/* Now reboot to run with default cmos. */
> 	outb(0x06, 0xcf9);
> 	for (;;) asm("hlt"); /* Wait for reset! */
> }
>
> -#endif

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