diff mbox series

pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called

Message ID 20200429104651.63643-1-hdegoede@redhat.com (mailing list archive)
State Not Applicable, archived
Headers show
Series pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called | expand

Commit Message

Hans de Goede April 29, 2020, 10:46 a.m. UTC
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. This issue leads to the intel_vbtn driver
reporting the device always being in tablet-mode at boot, even if it
is in laptop mode. Which in turn causes userspace to ignore touchpad
events. So iow this issues causes the touchpad to not work at boot.

Since the bug in the DSDT stems from the confusion of having 2 different
OpRegion types for accessing GPIOs on Cherry Trail devices, I believe
that this is best fixed inside the cherryview pinctrl driver.

This commit adds a workaround to the cherryview pinctrl driver so
that the DSDT's expectations of _REG always getting called for the
GeneralPurposeIo OpRegion are met.

Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Andy Shevchenko April 29, 2020, 2:21 p.m. UTC | #1
On Wed, Apr 29, 2020 at 12:46:51PM +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. This issue leads to the intel_vbtn driver
> reporting the device always being in tablet-mode at boot, even if it
> is in laptop mode. Which in turn causes userspace to ignore touchpad
> events. So iow this issues causes the touchpad to not work at boot.
> 
> Since the bug in the DSDT stems from the confusion of having 2 different
> OpRegion types for accessing GPIOs on Cherry Trail devices, I believe
> that this is best fixed inside the cherryview pinctrl driver.
> 
> This commit adds a workaround to the cherryview pinctrl driver so
> that the DSDT's expectations of _REG always getting called for the
> GeneralPurposeIo OpRegion are met.

s/cherryview/Cherryview/g

...

> +	if (acpi_has_method(adev->handle, "_REG")) {

And this check si redundant, you may call it as is (you didn't check for error
anyway), see also below.

> +		struct acpi_object_list input;
> +		union acpi_object params[2];
> +
> +		input.count = 2;
> +		input.pointer = params;
> +		params[0].type = ACPI_TYPE_INTEGER;
> +		params[0].integer.value = ACPI_ADR_SPACE_GPIO;
> +		params[1].type = ACPI_TYPE_INTEGER;
> +		params[1].integer.value = 1;
> +		acpi_evaluate_object(adev->handle, "_REG", &input, NULL);
> +	}

Can you consider to unify this with one in drivers/pci/hotplug/acpiphp_glue.c,
so we will have some helper function at the end? (perhaps as separate changes
to make less burden on backporting this one)
Hans de Goede May 4, 2020, 2:55 p.m. UTC | #2
Hi,

On 4/29/20 4:21 PM, Andy Shevchenko wrote:
> On Wed, Apr 29, 2020 at 12:46:51PM +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. This issue leads to the intel_vbtn driver
>> reporting the device always being in tablet-mode at boot, even if it
>> is in laptop mode. Which in turn causes userspace to ignore touchpad
>> events. So iow this issues causes the touchpad to not work at boot.
>>
>> Since the bug in the DSDT stems from the confusion of having 2 different
>> OpRegion types for accessing GPIOs on Cherry Trail devices, I believe
>> that this is best fixed inside the cherryview pinctrl driver.
>>
>> This commit adds a workaround to the cherryview pinctrl driver so
>> that the DSDT's expectations of _REG always getting called for the
>> GeneralPurposeIo OpRegion are met.
> 
> s/cherryview/Cherryview/g

Fixed for v2.

> 
> ...
> 
>> +	if (acpi_has_method(adev->handle, "_REG")) {
> 
> And this check si redundant, you may call it as is (you didn't check for error
> anyway), see also below.

Good point, also dropped for v2.

>> +		struct acpi_object_list input;
>> +		union acpi_object params[2];
>> +
>> +		input.count = 2;
>> +		input.pointer = params;
>> +		params[0].type = ACPI_TYPE_INTEGER;
>> +		params[0].integer.value = ACPI_ADR_SPACE_GPIO;
>> +		params[1].type = ACPI_TYPE_INTEGER;
>> +		params[1].integer.value = 1;
>> +		acpi_evaluate_object(adev->handle, "_REG", &input, NULL);
>> +	}
> 
> Can you consider to unify this with one in drivers/pci/hotplug/acpiphp_glue.c,
> so we will have some helper function at the end? (perhaps as separate changes
> to make less burden on backporting this one)

I think that for backporting it is best to keep this patch as is
(with your other comments addresed). Also this way this can hopefully
be merged for a 5.7-rc# candidate.

I will do a follow up series adding a helper and moving both cases
over to the helper.

Regards,

Hans
diff mbox series

Patch

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 4c74fdde576d..e0f11f1f841f 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1755,6 +1755,27 @@  static int chv_pinctrl_probe(struct platform_device *pdev)
 	if (ACPI_FAILURE(status))
 		dev_err(&pdev->dev, "failed to install ACPI addr space handler\n");
 
+	/*
+	 * Some DSDT-s use the chv_pinctrl_mmio_access_handler while checking
+	 * for the regular GeneralPurposeIo OpRegion availability, mixed with
+	 * the DSDT not defining a GeneralPurposeIo OpRegion at all. In this
+	 * case the ACPICA code will not call _REG to signal availability of
+	 * the GeneralPurposeIo OpRegion. Manually call _REG here so that
+	 * the DSDT-s GeneralPurposeIo availability checks will succeed.
+	 */
+	if (acpi_has_method(adev->handle, "_REG")) {
+		struct acpi_object_list input;
+		union acpi_object params[2];
+
+		input.count = 2;
+		input.pointer = params;
+		params[0].type = ACPI_TYPE_INTEGER;
+		params[0].integer.value = ACPI_ADR_SPACE_GPIO;
+		params[1].type = ACPI_TYPE_INTEGER;
+		params[1].integer.value = 1;
+		acpi_evaluate_object(adev->handle, "_REG", &input, NULL);
+	}
+
 	platform_set_drvdata(pdev, pctrl);
 
 	return 0;