[coreboot] [PATCH] ASUS P2B ACPI sleep preparations

Idwer Vollering vidwer at gmail.com
Sun Nov 28 21:47:53 CET 2010


2010/11/28 Uwe Hermann <uwe at 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+coreboot at tdiedrich.de<ranma%2Bcoreboot 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
>

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20101128/6c294043/attachment.html>


More information about the coreboot mailing list