Re: [coreboot] [proposal] EC-mainboard interaction at ASL level

[Duncan Laurie]

On Tue, Apr 8, 2014 at 2:08 AM, Peter Stuge  wrote:
> mrnuke wrote:
> > Define a set of common ACPI method names which the mainboard code should
> > define, and the EC code can always use.
>
> Thanks for starting this! It is exactly what is needed.
> > MB_TOGGLE_WLAN() or MB_TOGGLE_WIRELESS()
> > MB_INCREASE_BRIGHTNESS() and MB_DECREASE_BRIGHTNESS()
> > MB_SWITCH_DISPLAY()
> > MB_NOTIFY_POWER_EVENT()
>
> I additionally have lock, battery, sleep, network, eject and suspend
> hotkeys.

There are so many special cases with EC that it could be useful to
have a generic mainboard "EC event" method that any EC _Qxx could call
and pass the event ID to.

The event could be the Qxx number itself (for all the special cases)
or a standard defined event ID like those listed above which could be
> 255 so as not to conflict with possible Qxx IDs.  This limits the

permutations of mainboard EC event handlers and gives the flexibility
to pass all the various EC events to the mainboard for handling.

#define EC_EVENT_LID_CLOSED 0x100

External (_SB.MBEC)

// lid closed
Method (_Q01) {
  If (CondRefOf (_SB.MBEC)) {
    _SB.MBEC (EC_EVENT_LID_CLOSED)
  }
}

// special button handler
Method (_Q14) {
  If (CondRefOf (_SB.MBEC)) {
    _SB.MBEC (0x14)
  }
}

etc..

In the mainboard:

Scope (_SB)
{
  // mainboard event handler
  Method (MBEC, 1, Serialized) {
    Switch (ToInteger (Arg0)) {
      Case (EC_EVENT_LID_CLOSED) { ... }
      Case (0x14) { ... }
    }
  }
}

If we do go the route of having lots of mainboard handlers I think we
should keep the method names consistent with the ACPI spec and use a
mainboard namespace to separate them.  Conditional references are
already a headache in ASL and I think relying on the preprocessor
would make things worse.

Scope (_SB.MB)
  // Lid open
  Method (LIDO) { ... }
  // Lid closed
  Method (LIDC) { ... }
  // Increase brightness
  Method (BRTI) { ... }
}

But even in this form there needs to be a generic method for all the
special cases...
> > MB_LID_STATE
> > LIDS
> > PWRS
>
> Battery state(s) too?

These are actually problematic shadow variables, especially around
suspend/resume.

In order to update the shadow variable state on resume we typically
have the _WAK handler update them from the EC (or a GPIO) but the _WAK
handler is actually executed by the OS asynchronously so there is no
guarantee that the shadow variables are correct for any early methods
(or Notify() handlers) that are executed.

This has bitten me a number of times because it does not always show
up (there are not many dependencies currently) and depending on the
kernel behavior it can be hard to reproduce.  Reading this state
directly from the EC may require a few IO operations, but when dealing
with EC state that can affect the OS it is more important to be
correct than to save a few IO cycles.  (especially since the OS is
already dealing with the overhead of parsing and executing AML)

Rather than defining shadow variables in the NVS it would be safer to
have a (mainboard defined) method that can go and read the state
directly.  This is especially useful if the EC is not the source of
the data -- for example in some mainboards we have a direct gpio for
some state that is typically read from the EC.  This provides the
abstraction of system state without the concerns of stale data.

Method (PWRS) { Return (_SB.PCI.LPC.EC.ACEX) }
Method (LIDS) { Return (_SB.PCI.LPC.EC.LIDS) }

or like Parrot:
Method (LIDS) { Return (GP15) } // GPIO15

-duncan