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>