[coreboot] [PATCH] ASUS P2B ACPI sleep preparations

Uwe Hermann uwe at hermann-uwe.de
Sun Nov 28 21:36:47 CET 2010


On Sat, Nov 27, 2010 at 12:02:46PM +0100, Tobias Diedrich wrote:
> Add acpi_is_wakeup_early to i82371eb and P2B.
> Build fix for src/arch/i386/boot/acpi.c if !CONFIG_SMP
> Also check for acpi_slp_type 2 in acpi_is_wakeup, since S2
> uses the same acpi wakeup vector as S3.
> Other chipsets so far only ever set acpi_slp_type to 0 and 3.
> 
> Abuild-tested.
> 
> Signed-off-by: Tobias Diedrich <ranma+coreboot at tdiedrich.de>

Hm, not sure I feel confident enough to ack this, esp. as it might
affect non-P2B boards, but for now here's a quick review below.


> Index: coreboot-svn-p2b/src/arch/i386/boot/acpi.c
> ===================================================================
> --- coreboot-svn-p2b.orig/src/arch/i386/boot/acpi.c	2010-11-27 11:48:28.000000000 +0100
> +++ coreboot-svn-p2b/src/arch/i386/boot/acpi.c	2010-11-27 11:48:41.000000000 +0100
> @@ -481,7 +481,7 @@
>  
>  static int acpi_is_wakeup(void)
>  {
> -	return (acpi_slp_type == 3);
> +	return (acpi_slp_type == 3 || acpi_slp_type == 2);

Can this have negative effects for other ACPI-enabled chipsets/boards?


>  static acpi_rsdp_t *valid_rsdp(acpi_rsdp_t *rsdp)
> @@ -567,9 +567,11 @@
>  	return wake_vec;
>  }
>  
> +#if CONFIG_SMP == 1

I think you can write most of the kconfig CONFIG_* variable checks as:

#if CONFIG_SMP

The "== 1" part is _usally_ not needed (there may be exceptions).


> +#if CONFIG_HAVE_ACPI_RESUME == 1
> +	reg = (inw(DEFAULT_PMBASE + PMCNTRL) >> 10) & 7;
> +	switch (reg) {
> +	case 1:
> +		acpi_slp_type = 3;
> +		break;
> +	case 2:
> +	case 3:
> +		acpi_slp_type = 2;
> +		break;
> +	default:
> +		acpi_slp_type = 5;
> +		break;
> +	}
> +	printk(BIOS_INFO,
> +	       "%s: acpi_slp_type=%d!\n", __func__, acpi_slp_type);

I'd drop __func__ and make this a bit more userfriendly by wording it
something like "ACPI sleep type = %d" or similar.


> +/*
> + * Intel i82371eb (piix4e) datasheet, section 7.2.3, page 142:

Intel 82371EB (PIIX4E)

(cosmetics)


> + *
> + * 000b / 0x0: soft off/suspend to disk (soff/std)               S5
> + * 001b / 0x1: suspend to ram (str)                              S3
> + * 010b / 0x2: powered on suspend, context lost (poscl)          S2
> + * Note: 'context lost' menas the cpu restarts at the reset vector
> + * 011b / 0x3: powered on suspend, cpu context lost (posccl)     S1
> + * Note: Looks like 'cpu context lost' does _not_ mean the cpu

cpu -> CPU


> +int acpi_is_wakeup_early(void)
> +{
> +	u16 tmp, result;
> +
> +	print_debug(__func__);
> +	print_debug("\n");

Please use printk() in general, unless there's a technical reason to resort
to print_debug().


Uwe.
-- 
http://hermann-uwe.de     | http://sigrok.org
http://randomprojects.org | http://unmaintained-free-software.org




More information about the coreboot mailing list