GPIO: Add ACPI _AEI support

[cperciva]

Summary

Changes to acpi_gpiobus.c handle discovering and parsing the _AEI
objects and storing necessary data in device ivars. A new gpioaei.c
file implements the device, which simply requests an interrupt when
the pin is triggered and invokes the appropriate _Exx or _Lxx ACPI
method.

This makes the GPIO "power button" work on arm64 Graviton systems,
allowing EC2 "Stop"/"Reboot" instance calls to be handled cleanly.
(Prior to this change, those requests would time out after 4 minutes
and the instance would be forcibly killed.)

Sponsored by: Amazon

[cperciva]

sys/dev/gpio/acpi_gpiobus.c

183

This flag is added in D47239.

[andrew]

sys/dev/gpio/gpioaei.c

88–89

Can we check this object exists? I have a board that uses an _EVT method so will likely fail trying to call a method that doesn't exist.

[cperciva]

sys/dev/gpio/gpioaei.c

88–89

Ah, I didn't have access to any hardware using the newer _EVT option. We should implement support for that (necessary for the pins > 255 case I flagged a few lines earlier as well) but without hardware I can't do that right now. Can you add that support?

Until we implement support for _EVT methods it's probably better to print an error when we fail to invoke a nonexistent _Exx method than to simply do nothing though?

I'd like to get this driver into the tree -- with or without _EVT method support -- in time for 14.2 because it's a major pain point for EC2.

[ahmadkhalifa570_gmail.com]

sys/dev/gpio/acpi_gpiobus.c

50

This is accessible through sc->sc_busdev.

163

There should be a check here to make sure we have the correct controller, like the one in acpi_gpiobus_enumerate_res.
e.g gpiobus0 should only interact with pins on GPO0.

This becomes more of a problem when you have multiple controllers with the same pin numbers then gpiobus0 tries to take ownership of both.

188

Sometimes ACPI provides us with flags that aren't accepted by the GPIO controller's driver. e.g bytgpio only accepts GPIO_PIN_INPUT or GPIO_PIN_OUTPUT. ACPI may give us either one of those along with GPIO_PIN_PULLUP for example. This makes GPIOBUS_PIN_SETFLAGS fail.

Maybe we can use GPIO_PIN_SETFLAGS with GPIO_PIN_GETCAPS to weed out the flags we don't need.
We should probably also be doing this in acpi_gpiobus_enumerate_res instead of passing the flags directly.

334

This should be updated since we also check for _AEI now. Preferably to something more generic.

[ahmadkhalifa570_gmail.com]

sys/dev/gpio/acpi_gpiobus.c

334

Actually, "GPIO resources" works for both. Not sure why I thought it didn't :p
Nevermind.

[cperciva]

sys/dev/gpio/acpi_gpiobus.c

50

Thanks!

163

I don't understand why this is necessary -- we reach here from AcpiWalkResources(handle, "_AEI", acpi_gpiobus_enumerate_aei, context); which is looking at resources under the provided handle, right?

188

If ACPI tells us to use flags which the GPIO controller's driver doesn't accept, it seems that failing to attach a driver is the right solution here. (Unless it's a case like in EC2 where we need a "the ACPI tables are buggy" quirk.)

[ahmadkhalifa570_gmail.com]

sys/dev/gpio/acpi_gpiobus.c

163

Yes, but acpi_gpiobus_enumerate itself gets called once for every ACPI device we have, which can include multiple controllers. This is necessary for _CRS since it can appear anywhere.

Taking a look at the spec, it looks like _AEI can only appear under the GPIO controller it's referencing, so the AcpiWalkResources call can actually be moved to the attach function and use that handle directly. Note that nothing stops buggy ACPI implementations from having this outside of the controller's scope.

This object appears within the scope of the GPIO controller device whose pins are to be used as GPIO-signaled events.

ACPI Specification Release 6.5 - §5.6.5.2

188

Hmm, I assumed it was failing because changing the pin to either pull-up or pull-down wasn't supported (even if the pin is already a pull-up one).
Though, looking at the acpidump of this machine, it seems to be inconsistent when it comes to setting the pin config. And the example I gave (bytgpio) doesn't seem to have pins with varying pin configs either.

it seems that failing to attach a driver is the right solution here.

I'm not sure how common this inconsistency is, but I think failing is too harsh. Maybe we could output a warning instead?

[cperciva]

Use sc->sc_busdev rather than stashing another copy of dev.

[cperciva]

Enumerate _AEI from acpi_gpiobus_attach

[cperciva]

@ahmadkhalifa570_gmail.com I think I've addressed all the issues here; can you take a look and tell me if it looks ok to you? I'm eager to commit this so it's available in 14.2. :-)

sys/dev/gpio/acpi_gpiobus.c

163

Aha, I didn't realize that _CRS could appear anywhere. I've fixed the _AEI enumeration to run from acpi_gpiobus_attach.

188

If this turns out to be a problem later we can always change it later. I'd prefer to "fail safe" by not attaching the device rather than enabling something which might not do what is intended. (For example, in EC2 the _AEI is used to signal a control plane shutdown request; if that got accidentally asserted we could end up with instances shutting down immediately after booting, a situation which would be rather difficult for users to fix.)

[andrew]

sys/dev/gpio/acpi_gpiobus.c

188

I have a server where the _AEI data has the GPIOs a pull-up, however this is a pl061 that doesn't support that so we fail adding the child here.

374

Could we have this return a list of pins, and have an ivar with the size? I would like to rework the driver to support that in a single gpio_aei device, but am unlikely to have time for 14.2 so would like to at least have that as a future option.

[ahmadkhalifa570_gmail.com]

This looks good. My only worry is that there is a theoretical maximum of 65535 interrupts that can be specified in _AEI (currently we limit it to 255, but with _EVT support it grows to 65535), and having even 255 gpiobus children seems ... not good. Maybe we could make it so one instance of gpioaei can handle multiple pins, or even merge the driver into acpi_gpiobus and handle it there.

Anyways, this doesn't look like it's a problem for now.

sys/dev/gpio/acpi_gpiobus.c

188

That's a good point. I can look more into this when I get the time.

[imp]

sys/dev/gpio/gpioaei.c

55

I don't think this is a useful test. It will always be gpio_aei because we've set the devclass to do the probe.
You have to rely on the bus driver to only add this when it's valid.

80

How do we know pin isn't NULL?

[cperciva]

sys/dev/gpio/gpioaei.c

55

We specified that we're a gpio_aei device hanging off a gpiobus, but a future gpiobus might add children with a devclass of NULL in which case the matching logic would still call our probe routine, yes? So I wanted to make sure we only matched when the parent bus said explicitly that the child device is a gpio_aei.

80

Because any parent bus which creates a gpio_aei child guarantees that pin won't be NULL. (Mechanically: Because pin is a pointer into the ivars structure.)

[cperciva]

sys/dev/gpio/acpi_gpiobus.c

188

Ah, that's the same problem as Amazon has. You can set debug.acpi.quirks="8" to work around the ACPI bug.

374

Oh, I thought it was cleaner to have a device per _AEI. In EC2 there's two, but they invoke different ACPI "buttons"; we have acpi_button0 and acpi_button1 so I figured it made sense to have gpio_aei0 and gpio_aei1 which invoked them.

[imp]

sys/dev/gpio/gpioaei.c

55

But the devclass is set just before probe is called. So this will never do anything.

[cperciva]

Fixes to probing:

• Don't check device_get_name; the probing logic sets the devclass before

calling our probe function so this is useless.

• Return BUS_PROBE_NOWILDCARD so we won't match unless the bus explicitly

requested gpio_aei.

• Call device_set_desc from device_attach, not device_probe.

[cperciva]

@imp I think I've fixed the newbus logic... let me know if I'm still screwing up somehow.

sys/dev/gpio/gpioaei.c

55

Aha, now I understand. Fixed.

[andrew]

sys/dev/gpio/acpi_gpiobus.c

374

Having a look again I don't think this should exist as gpiobus.c already stores the pin information.

We could add gpio_pin_get_by_idx to get the pin similar to how we already do it in ofw_gpiobus.c

[cperciva]

sys/dev/gpio/acpi_gpiobus.c

374

gpiobus.c stores the pin number but not the flags. And we can't even use gpio_pin_get_by_child_index to get a gpio_pin_t structure without the flags, because using that results in gpiobus_acquire_pin being called twice (which fails).

[andrew]

sys/dev/gpio/acpi_gpiobus.c

168

Why M_NOWAIT? Can we not sleep here?

374

In ofw_gpiobus.c we populate the pin list directly in ofw_gpiobus_setup_devinfo so don't see the acquire issue. I'm not sure why @avg added GPIOBUS_IVAR_PINS in D20459, id doesn't seem to be used.

Could gpio_pin_get_by_bus_pinnum call GPIO_PIN_GETFLAGS to set the flags?

[cperciva]

sys/dev/gpio/acpi_gpiobus.c

168

That's pure cargo culting; gpiobus_add_child uses M_NOWAIT and I wasn't sure why so I copied it.

That said, boot-time device probing happens early enough in the boot process that if we're already failing to allocate memory we're probably pretty much screwed; we can't swap and I'm not sure if we even have other threads running yet.

374

Yeah, we could set the pin information directly. I figured that since there was an API for doing it we were supposed to use it. ;-)

And no we can't use GPIO_PIN_GETFLAGS; that gets passed down to the driver which reports back *some* of the flags but not all of them.

[andrew]

sys/dev/gpio/acpi_gpiobus.c

374

I've pushed a change on top of this to add and use gpio_pin_get_by_acpi_index to be consistent with other gpiobus code to https://github.com/zxombie/freebsd/commit/8c3f8aba9c616dd1f0e1e3aaf76fe9630b616e00.

[cperciva]

sys/dev/gpio/acpi_gpiobus.c

374

That looks fine to me aside from possibly moving a declaration into acpi_gpiobusvar.h (https://github.com/zxombie/freebsd/commit/8c3f8aba9c616dd1f0e1e3aaf76fe9630b616e00#r148595677). I assume the for (i = 0; i < npins; i++) loops with npins hard-coded to 1 are because it might not be fixed at 1 in the future.

Are you ok with pushing this into the src tree as two commits?

[andrew]

sys/dev/gpio/acpi_gpiobus.c

374

There could be more than one pin per GpioInt, but as gpio_aei only supports a single event I limited it to 1 for now.

[avg]

sys/dev/gpio/acpi_gpiobus.c

374

Regarding GPIOBUS_IVAR_PINS, it's been a long time but my recollection is that it was for a kind of a "platform" driver.
The machine was x86 and ACPI based, but there was no advertisement for its GPIO capabilities in ACPI.
And, if I recall correctly, ACPI GPIO support was kind of very new at the time.
So, the idea was that the platform driver would configure pins for child devices on a gpio bus based on the external knowledge (hardcoded).

Then later, a new version of firmware / BIOS for that machine came out and it had ACPIO GPIO information for the pins of interest.
So, there was no need for the "platform driver".

So, that ivar is indeed unused, but theoretically it can be useful for cases where we know that there is something on a GPIO, but the platform does not advertise it and it is not easy to add an overlay (i.e., it's not FDT or alike).

[avg]

sys/dev/gpio/acpi_gpiobus.c

374

FWIW, D26311 is what I came up when the firmware started advertising GPIO.

I think that gpioled_acpi should be much easier and cleaner to implement now than it was back then.