<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
On 11/28/10 12:47 PM, Idwer Vollering wrote:
<blockquote
cite="mid:AANLkTi=wLB+C-O0nQijquSQ1eGGJ7+m4w9DfeBQZ6T9+@mail.gmail.com"
type="cite">2010/11/28 Uwe Hermann <span dir="ltr"><<a
moz-do-not-send="true" href="mailto:uwe@hermann-uwe.de">uwe@hermann-uwe.de</a>></span><br>
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
<div class="im">On Sat, Nov 27, 2010 at 12:02:46PM +0100,
Tobias Diedrich wrote:<br>
> Add acpi_is_wakeup_early to i82371eb and P2B.<br>
> Build fix for src/arch/i386/boot/acpi.c if !CONFIG_SMP<br>
> Also check for acpi_slp_type 2 in acpi_is_wakeup, since
S2<br>
> uses the same acpi wakeup vector as S3.<br>
> Other chipsets so far only ever set acpi_slp_type to 0
and 3.<br>
><br>
> Abuild-tested.<br>
><br>
> Signed-off-by: Tobias Diedrich <<a
moz-do-not-send="true"
href="mailto:ranma%2Bcoreboot@tdiedrich.de">ranma+coreboot@tdiedrich.de</a>><br>
<br>
</div>
Hm, not sure I feel confident enough to ack this, esp. as it
might<br>
affect non-P2B boards, but for now here's a quick review
below.<br>
<div class="im"><br>
<br>
> Index: coreboot-svn-p2b/src/arch/i386/boot/acpi.c<br>
>
===================================================================<br>
> --- coreboot-svn-p2b.orig/src/arch/i386/boot/acpi.c
2010-11-27 11:48:28.000000000 +0100<br>
> +++ coreboot-svn-p2b/src/arch/i386/boot/acpi.c
2010-11-27 11:48:41.000000000 +0100<br>
> @@ -481,7 +481,7 @@<br>
><br>
> static int acpi_is_wakeup(void)<br>
> {<br>
> - return (acpi_slp_type == 3);<br>
> + return (acpi_slp_type == 3 || acpi_slp_type ==
2);<br>
<br>
</div>
Can this have negative effects for other ACPI-enabled
chipsets/boards?<br>
<div class="im"><br>
<br>
> static acpi_rsdp_t *valid_rsdp(acpi_rsdp_t *rsdp)<br>
> @@ -567,9 +567,11 @@<br>
> return wake_vec;<br>
> }<br>
><br>
> +#if CONFIG_SMP == 1<br>
<br>
</div>
I think you can write most of the kconfig CONFIG_* variable
checks as:<br>
<br>
#if CONFIG_SMP<br>
<br>
The "== 1" part is _usally_ not needed (there may be
exceptions).<br>
<div class="im"><br>
<br>
> +#if CONFIG_HAVE_ACPI_RESUME == 1<br>
> + reg = (inw(DEFAULT_PMBASE + PMCNTRL) >> 10)
& 7;<br>
> + switch (reg) {<br>
> + case 1:<br>
> + acpi_slp_type = 3;<br>
> + break;<br>
> + case 2:<br>
> + case 3:<br>
> + acpi_slp_type = 2;<br>
> + break;<br>
> + default:<br>
> + acpi_slp_type = 5;<br>
> + break;<br>
> + }<br>
> + printk(BIOS_INFO,<br>
> + "%s: acpi_slp_type=%d!\n", __func__,
acpi_slp_type);<br>
<br>
</div>
I'd drop __func__ and make this a bit more userfriendly by
wording it<br>
something like "ACPI sleep type = %d" or similar.<br>
<div class="im"><br>
<br>
> +/*<br>
> + * Intel i82371eb (piix4e) datasheet, section 7.2.3,
page 142:<br>
<br>
</div>
Intel 82371EB (PIIX4E)<br>
<br>
(cosmetics)<br>
<div class="im"><br>
<br>
> + *<br>
> + * 000b / 0x0: soft off/suspend to disk (soff/std)
S5<br>
</div>
</blockquote>
<div><br>
soff/std --> Soff/STD<br>
<br>
</div>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
<div class="im">
> + * 001b / 0x1: suspend to ram (str)
S3<br>
</div>
</blockquote>
<div><br>
str --> STR<br>
<br>
</div>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
<div class="im">
> + * 010b / 0x2: powered on suspend, context lost
(poscl) S2<br>
</div>
</blockquote>
<div><br>
poscl --> POSCL<br>
</div>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
<div class="im">
> + * Note: 'context lost' menas the cpu restarts at the
reset vector<br>
</div>
</blockquote>
<div><br>
menas --> means<br>
<br>
</div>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt
0.8ex; border-left: 1px solid rgb(204, 204, 204);
padding-left: 1ex;">
<div class="im">
> + * 011b / 0x3: powered on suspend, cpu context lost
(posccl) S1<br>
</div>
</blockquote>
<div><br>
posccl --> POSCCL<br>
<br>
Should we drop the binary notation and just keep the 0x..
values ? <br>
<br>
</div>
</div>
</blockquote>
Yes, I think so, too. Referring to non-standard names such as POSCCL
and POSCL is not useful.<br>
<br>
Stefan<br>
<br>
</body>
</html>