Hi,
On 5/7/20 2:30 PM, Mika Westerberg wrote:
On Thu, May 07, 2020 at 12:15:09PM +0200, Hans de Goede wrote:
Hi,
On 5/6/20 8:40 AM, Mika Westerberg wrote:
+Rafael and ACPICA folks.
On Mon, May 04, 2020 at 04:59:57PM +0200, Hans de Goede wrote:
On Cherry Trail devices there are 2 possible ACPI OpRegions for accessing GPIOs. The standard GeneralPurposeIo OpRegion and the Cherry Trail specific UserDefined 0x9X OpRegions.
Having 2 different types of OpRegions leads to potential issues with checks for OpRegion availability, or in other words checks if _REG has been called for the OpRegion which the ACPI code wants to use.
The ACPICA core does not call _REG on an ACPI node which does not define an OpRegion matching the type being registered; and the reference design DSDT, from which most Cherry Trail DSDTs are derived, does not define GeneralPurposeIo, nor UserDefined(0x93) OpRegions for the GPO2 (UID 3) device, because no pins were assigned ACPI controlled functions in the reference design.
Together this leads to the perfect storm, at least on the Cherry Trail based Medion Akayo E1239T. This design does use a GPO2 pin from its ACPI code and has added the Cherry Trail specific UserDefined(0x93) opregion to its GPO2 ACPI node to access this pin.
But it uses a has _REG been called availability check for the standard GeneralPurposeIo OpRegion. This clearly is a bug in the DSDT, but this does work under Windows.
Do we know why this works under Windows? I mean if possible we should do the same and I kind of suspect that they forcibly call _REG in their GPIO driver.
Windows has its own ACPI implementation, so it could also be that their equivalent of the:
status = acpi_install_address_space_handler(handle, ACPI_ADR_SPACE_GPIO, acpi_gpio_adr_space_handler, NULL, achip);Call from drivers/gpio/gpiolib-acpi.c indeed always calls _REG on the handle without checking that there is an actual OpRegion with a space-id of ACPI_ADR_SPACE_GPIO defined, as the ACPICA code does. Note that the current ACPICA code would require significant rework to allow this, or it would need to add a _REG call at the end of acpi_install_address_space_handler(), potentially calling _REG twice in many cases.
I actually think this is the correct solution. Reading ACPI spec it say this:
Once _REG has been executed for a particular operation region, indicating that the operation region handler is ready, a control method can access fields in the operation region
You can interpret it so that _REG gets called when operation region handler is ready. It does not say that there needs to be an actual operation region even though the examples following all have operation region.
I wonder what our ACPICA gurus think about this? Rafael, Bob, Erik?
I realize that this thread has gone a bit stale (sorry about that) but I have been working on an ACPICA solution on that, and this works nicely. It turns out that ACPICA already had code to run the _REG method unconditionally in some cases, so I've simply extended that to also apply to GpioOpRegions.
One thing which is open for discussion is if we want to extend this to more then just GpioOpRegions. For now I've chosen to just extend the current special handling for EC OpRegions to also apply to GpioOpRegions which fixes the issue at hand.
I've attached a patch directly against the Linux kernel acpica copy which fixes this.
Mika (or anyone else reading along who wants to help), I know that ACPICA patches go upstream through the ACPICA repo, but I'm not really familiar with there workflow and I'm a bit swamped with work atm. So I was wondering if you could perhaps convert this patch to an upstream ACPICA patch and submit it there for me ?
Regards,
Hans