Re: [PATCH] Add ACPI translator and shutdown RPC that turns off pc

[Kalle Olavi Niemitalo]

Damien Zammit <damien@zamaudio.com> writes:

> +  /* Enable ACPI */
> +  outb(acpi_en, smi_cmd);
> +  for (i = 0; i < 300; i++)
> +  {
> +    if ( (inw(pm1a_ctl) & SCI_EN) == SCI_EN)
> +      break;
> +  }

Perhaps this code should first check whether the SCI_EN bit is
already set, as described in ACPI 5.0 section 4.8.2.5
"Legacy/ACPI Select and the SCI Interrupt", and skip the outb if
so.  Section 5.2.9 "Fixed ACPI Description Table (FADT)" says the
SMI_CMD field of FADT can be zero.

> +  /* try sleep state 5 first */
> +  outw(SLP_TYP5 | SLP_EN, pm1a_ctl);
> +
> +  /* if we reach here then above did not work */
> +  outw(SLP_TYP0 | SLP_EN, pm1a_ctl);

More about these below.

> +#define SLP_TYP0     (0x0 << 10)
> +#define SLP_TYP5     (0x5 << 10)

This hardcodes QEMU-specific values.  ACPI 5.0 section 7.3.4 says
the values for PM1a_CNT.SLP_TYP must be read from the \S0-\S5
objects.  The DSDT of my laptop has:

    Name (_S5, Package (0x04)  // _S5_: S5 System State
    {
        0x07, 
        Zero, 
        Zero, 
        Zero
    })

so PM1a_CNT.SLP_TYP on this system should be 7, not 5.  However,
implementing this properly would require an AML interpreter; the
DSDT of my laptop also has:

    If (LEqual (S3DS, One))
    {
        Name (_S3, Package (0x04)  // _S3_: S3 System State
        {
            0x05, 
            Zero, 
            Zero, 
            Zero
        })
    }

and defines S3DS as a field in an OperationRegion, so OSPM
would have to understand those concepts in order to know
whether \_S3 exists.  I assume a similar construct is possible
for \_S5.

Maybe it's good enough if S5 works on QEMU.  In that case though,
I would like to see a comment warning about this limitation.