2010/11/28 Uwe Hermann <span dir="ltr"><<a 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 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> </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: Looks like 'cpu context lost' does _not_ mean the cpu</div></blockquote><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">
<br>
</div>cpu -> CPU<br>
<div class="im"><br>
<br>
> +int acpi_is_wakeup_early(void)<br>
> +{<br>
> + u16 tmp, result;<br>
> +<br>
> + print_debug(__func__);<br>
> + print_debug("\n");<br>
<br>
</div>Please use printk() in general, unless there's a technical reason to resort<br>
to print_debug().<br>
<br>
<br>
Uwe.<br>
<font color="#888888">--<br>
<a href="http://hermann-uwe.de" target="_blank">http://hermann-uwe.de</a> | <a href="http://sigrok.org" target="_blank">http://sigrok.org</a><br>
<a href="http://randomprojects.org" target="_blank">http://randomprojects.org</a> | <a href="http://unmaintained-free-software.org" target="_blank">http://unmaintained-free-software.org</a><br>
</font></blockquote></div><br>