2010/11/28 Uwe Hermann <u...@hermann-uwe.de> > 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+coreb...@tdiedrich.de<ranma%2bcoreb...@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 >
soff/std --> Soff/STD > + * 001b / 0x1: suspend to ram (str) S3 > str --> STR > + * 010b / 0x2: powered on suspend, context lost (poscl) S2 > poscl --> POSCL > > + * Note: 'context lost' menas the cpu restarts at the reset vector > menas --> means > > + * 011b / 0x3: powered on suspend, cpu context lost (posccl) S1 > posccl --> POSCCL Should we drop the binary notation and just keep the 0x.. values ? > > + * 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 >
-- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot