* [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called @ 2020-05-04 14:59 Hans de Goede 2020-05-06 6:40 ` Mika Westerberg 2020-05-06 23:42 ` Sasha Levin 0 siblings, 2 replies; 12+ messages in thread From: Hans de Goede @ 2020-05-04 14:59 UTC (permalink / raw) To: Mika Westerberg, Andy Shevchenko, Linus Walleij Cc: Hans de Goede, linux-acpi, linux-gpio, stable 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> --- Changes in v2: - Drop unnecessary if (acpi_has_method(adev->handle, "_REG")) check - Fix Cherryview spelling in the commit message --- drivers/pinctrl/intel/pinctrl-cherryview.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c index 4c74fdde576d..4817aec114d6 100644 --- a/drivers/pinctrl/intel/pinctrl-cherryview.c +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c @@ -1693,6 +1693,8 @@ static acpi_status chv_pinctrl_mmio_access_handler(u32 function, static int chv_pinctrl_probe(struct platform_device *pdev) { + struct acpi_object_list input; + union acpi_object params[2]; struct chv_pinctrl *pctrl; struct acpi_device *adev; acpi_status status; @@ -1755,6 +1757,22 @@ 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. + */ + 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; + input.count = 2; + input.pointer = params; + acpi_evaluate_object(adev->handle, "_REG", &input, NULL); + platform_set_drvdata(pdev, pctrl); return 0; -- 2.26.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called 2020-05-04 14:59 [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called Hans de Goede @ 2020-05-06 6:40 ` Mika Westerberg 2020-05-07 10:15 ` Hans de Goede 2020-05-06 23:42 ` Sasha Levin 1 sibling, 1 reply; 12+ messages in thread From: Mika Westerberg @ 2020-05-06 6:40 UTC (permalink / raw) To: Hans de Goede Cc: Andy Shevchenko, Linus Walleij, linux-acpi, linux-gpio, stable, Rafael J. Wysocki, Bob Moore, Erik Kaneda +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. Are the ACPI tables from this system available somewhere? > 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. I would like to understand the issue bit better before we do this. > Cc: stable@vger.kernel.org > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > - Drop unnecessary if (acpi_has_method(adev->handle, "_REG")) check > - Fix Cherryview spelling in the commit message > --- > drivers/pinctrl/intel/pinctrl-cherryview.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c > index 4c74fdde576d..4817aec114d6 100644 > --- a/drivers/pinctrl/intel/pinctrl-cherryview.c > +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c > @@ -1693,6 +1693,8 @@ static acpi_status chv_pinctrl_mmio_access_handler(u32 function, > > static int chv_pinctrl_probe(struct platform_device *pdev) > { > + struct acpi_object_list input; > + union acpi_object params[2]; > struct chv_pinctrl *pctrl; > struct acpi_device *adev; > acpi_status status; > @@ -1755,6 +1757,22 @@ 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. > + */ > + 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; > + input.count = 2; > + input.pointer = params; > + acpi_evaluate_object(adev->handle, "_REG", &input, NULL); > + > platform_set_drvdata(pdev, pctrl); > > return 0; > -- > 2.26.0 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called 2020-05-06 6:40 ` Mika Westerberg @ 2020-05-07 10:15 ` Hans de Goede 2020-05-07 12:30 ` Mika Westerberg 0 siblings, 1 reply; 12+ messages in thread From: Hans de Goede @ 2020-05-07 10:15 UTC (permalink / raw) To: Mika Westerberg Cc: Andy Shevchenko, Linus Walleij, linux-acpi, linux-gpio, stable, Rafael J. Wysocki, Bob Moore, Erik Kaneda 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. We could move the manual _REG call I'm adding to pinctrl-cherry-view.c but that has the same issue of calling _REG twice in many cases. Most (all?) _REG implementations are fine with that, as they just set a variable to 1 (to the Arg1 value). Still calling _REG twice is something which we might want to avoid. As a compromise I've chosen to add the extra unconditional _REG call to pinctrl-cherryview.c because: 1. The problem in the DSDT in question stems from there being 2 different OpRegions for accessing GPIOs which AFAIK is unique to cherryview 2. I've seen many many cherryview DSDT-s and as such I'm confident that calling _REG twice is not an issue on cherryview. > Are the ACPI tables from this system available somewhere? Here you go: https://fedorapeople.org/~jwrdegoede/medion-e1239t-dsdt.dsl The problem is that on line 12624 there is a GPO2.AVBL == One check, before GPO2.DCDT is used. If you then look at line 17688 you see that _REG for the GPO2 device checkes for a space-id of 8 (ACPI_ADR_SPACE_GPIO) to set AVBL But the only OpRegion defined for the GPO2 device, and the OpRegion to which GPO2.DCDT is mapped is the cherryview UserDefined 0x93 GPIO access OpRegion, see line 17760. Since there is no OpRegion for the ACPI_ADR_SPACE_GPIO space-id, ACPICA never calls _REG with Arg0 == 8. So as already mentioned the problem stems from the confusion of there being 2 different OpRegions for accessing GPIOs on cherryview. Regards, Hans >> 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. > > I would like to understand the issue bit better before we do this. > >> Cc: stable@vger.kernel.org >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v2: >> - Drop unnecessary if (acpi_has_method(adev->handle, "_REG")) check >> - Fix Cherryview spelling in the commit message >> --- >> drivers/pinctrl/intel/pinctrl-cherryview.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c >> index 4c74fdde576d..4817aec114d6 100644 >> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c >> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c >> @@ -1693,6 +1693,8 @@ static acpi_status chv_pinctrl_mmio_access_handler(u32 function, >> >> static int chv_pinctrl_probe(struct platform_device *pdev) >> { >> + struct acpi_object_list input; >> + union acpi_object params[2]; >> struct chv_pinctrl *pctrl; >> struct acpi_device *adev; >> acpi_status status; >> @@ -1755,6 +1757,22 @@ 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. >> + */ >> + 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; >> + input.count = 2; >> + input.pointer = params; >> + acpi_evaluate_object(adev->handle, "_REG", &input, NULL); >> + >> platform_set_drvdata(pdev, pctrl); >> >> return 0; >> -- >> 2.26.0 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called 2020-05-07 10:15 ` Hans de Goede @ 2020-05-07 12:30 ` Mika Westerberg 2020-05-07 12:39 ` Hans de Goede 2020-10-08 9:31 ` Hans de Goede 0 siblings, 2 replies; 12+ messages in thread From: Mika Westerberg @ 2020-05-07 12:30 UTC (permalink / raw) To: Hans de Goede Cc: Andy Shevchenko, Linus Walleij, linux-acpi, linux-gpio, stable, Rafael J. Wysocki, Bob Moore, Erik Kaneda 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? > We could move the manual _REG call I'm adding to pinctrl-cherry-view.c > but that has the same issue of calling _REG twice in many cases. > > Most (all?) _REG implementations are fine with that, as they just set a > variable to 1 (to the Arg1 value). Still calling _REG twice is something > which we might want to avoid. > > As a compromise I've chosen to add the extra unconditional _REG call > to pinctrl-cherryview.c because: > > 1. The problem in the DSDT in question stems from there being 2 > different OpRegions for accessing GPIOs which AFAIK is unique to > cherryview > > 2. I've seen many many cherryview DSDT-s and as such I'm confident > that calling _REG twice is not an issue on cherryview. > > > Are the ACPI tables from this system available somewhere? > > Here you go: > https://fedorapeople.org/~jwrdegoede/medion-e1239t-dsdt.dsl Thanks for sharing! > The problem is that on line 12624 there is a GPO2.AVBL == One > check, before GPO2.DCDT is used. If you then look at line > 17688 you see that _REG for the GPO2 device checkes for a > space-id of 8 (ACPI_ADR_SPACE_GPIO) to set AVBL > > But the only OpRegion defined for the GPO2 device, and the > OpRegion to which GPO2.DCDT is mapped is the cherryview > UserDefined 0x93 GPIO access OpRegion, see line 17760. > Since there is no OpRegion for the ACPI_ADR_SPACE_GPIO > space-id, ACPICA never calls _REG with Arg0 == 8. Indeed, I see the issue now. I guess calling _REG always when there is handler installed would solve this as well? > So as already mentioned the problem stems from the confusion > of there being 2 different OpRegions for accessing GPIOs > on cherryview. > > Regards, > > Hans > > > > > > 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. > > > > I would like to understand the issue bit better before we do this. > > > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > --- > > > Changes in v2: > > > - Drop unnecessary if (acpi_has_method(adev->handle, "_REG")) check > > > - Fix Cherryview spelling in the commit message > > > --- > > > drivers/pinctrl/intel/pinctrl-cherryview.c | 18 ++++++++++++++++++ > > > 1 file changed, 18 insertions(+) > > > > > > diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c > > > index 4c74fdde576d..4817aec114d6 100644 > > > --- a/drivers/pinctrl/intel/pinctrl-cherryview.c > > > +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c > > > @@ -1693,6 +1693,8 @@ static acpi_status chv_pinctrl_mmio_access_handler(u32 function, > > > static int chv_pinctrl_probe(struct platform_device *pdev) > > > { > > > + struct acpi_object_list input; > > > + union acpi_object params[2]; > > > struct chv_pinctrl *pctrl; > > > struct acpi_device *adev; > > > acpi_status status; > > > @@ -1755,6 +1757,22 @@ 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. > > > + */ > > > + 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; > > > + input.count = 2; > > > + input.pointer = params; > > > + acpi_evaluate_object(adev->handle, "_REG", &input, NULL); > > > + > > > platform_set_drvdata(pdev, pctrl); > > > return 0; > > > -- > > > 2.26.0 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called 2020-05-07 12:30 ` Mika Westerberg @ 2020-05-07 12:39 ` Hans de Goede 2020-05-07 13:39 ` Andy Shevchenko 2020-10-08 9:31 ` Hans de Goede 1 sibling, 1 reply; 12+ messages in thread From: Hans de Goede @ 2020-05-07 12:39 UTC (permalink / raw) To: Mika Westerberg Cc: Andy Shevchenko, Linus Walleij, linux-acpi, linux-gpio, stable, Rafael J. Wysocki, Bob Moore, Erik Kaneda 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? > >> We could move the manual _REG call I'm adding to pinctrl-cherry-view.c >> but that has the same issue of calling _REG twice in many cases. >> >> Most (all?) _REG implementations are fine with that, as they just set a >> variable to 1 (to the Arg1 value). Still calling _REG twice is something >> which we might want to avoid. >> >> As a compromise I've chosen to add the extra unconditional _REG call >> to pinctrl-cherryview.c because: >> >> 1. The problem in the DSDT in question stems from there being 2 >> different OpRegions for accessing GPIOs which AFAIK is unique to >> cherryview >> >> 2. I've seen many many cherryview DSDT-s and as such I'm confident >> that calling _REG twice is not an issue on cherryview. >> >>> Are the ACPI tables from this system available somewhere? >> >> Here you go: >> https://fedorapeople.org/~jwrdegoede/medion-e1239t-dsdt.dsl > > Thanks for sharing! > >> The problem is that on line 12624 there is a GPO2.AVBL == One >> check, before GPO2.DCDT is used. If you then look at line >> 17688 you see that _REG for the GPO2 device checkes for a >> space-id of 8 (ACPI_ADR_SPACE_GPIO) to set AVBL >> >> But the only OpRegion defined for the GPO2 device, and the >> OpRegion to which GPO2.DCDT is mapped is the cherryview >> UserDefined 0x93 GPIO access OpRegion, see line 17760. >> Since there is no OpRegion for the ACPI_ADR_SPACE_GPIO >> space-id, ACPICA never calls _REG with Arg0 == 8. > > Indeed, I see the issue now. I guess calling _REG always when there is > handler installed would solve this as well? Yes that should solve the issue, that is actualy more or less what my patch does, but my patch only does it for the pinctrl-cherryview.c case. Regards, Hans >>>> 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. >>> >>> I would like to understand the issue bit better before we do this. >>> >>>> Cc: stable@vger.kernel.org >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> Changes in v2: >>>> - Drop unnecessary if (acpi_has_method(adev->handle, "_REG")) check >>>> - Fix Cherryview spelling in the commit message >>>> --- >>>> drivers/pinctrl/intel/pinctrl-cherryview.c | 18 ++++++++++++++++++ >>>> 1 file changed, 18 insertions(+) >>>> >>>> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c >>>> index 4c74fdde576d..4817aec114d6 100644 >>>> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c >>>> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c >>>> @@ -1693,6 +1693,8 @@ static acpi_status chv_pinctrl_mmio_access_handler(u32 function, >>>> static int chv_pinctrl_probe(struct platform_device *pdev) >>>> { >>>> + struct acpi_object_list input; >>>> + union acpi_object params[2]; >>>> struct chv_pinctrl *pctrl; >>>> struct acpi_device *adev; >>>> acpi_status status; >>>> @@ -1755,6 +1757,22 @@ 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. >>>> + */ >>>> + 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; >>>> + input.count = 2; >>>> + input.pointer = params; >>>> + acpi_evaluate_object(adev->handle, "_REG", &input, NULL); >>>> + >>>> platform_set_drvdata(pdev, pctrl); >>>> return 0; >>>> -- >>>> 2.26.0 >>> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called 2020-05-07 12:39 ` Hans de Goede @ 2020-05-07 13:39 ` Andy Shevchenko 0 siblings, 0 replies; 12+ messages in thread From: Andy Shevchenko @ 2020-05-07 13:39 UTC (permalink / raw) To: Hans de Goede Cc: Mika Westerberg, Linus Walleij, linux-acpi, linux-gpio, stable, Rafael J. Wysocki, Bob Moore, Erik Kaneda On Thu, May 07, 2020 at 02:39:21PM +0200, Hans de Goede wrote: > On 5/7/20 2:30 PM, Mika Westerberg wrote: > > On Thu, May 07, 2020 at 12:15:09PM +0200, Hans de Goede wrote: > > > On 5/6/20 8:40 AM, Mika Westerberg wrote: +Rafael and ACPICA folks. ... > > 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? > > > > > We could move the manual _REG call I'm adding to pinctrl-cherry-view.c > > > but that has the same issue of calling _REG twice in many cases. > > > > > > Most (all?) _REG implementations are fine with that, as they just set a > > > variable to 1 (to the Arg1 value). Still calling _REG twice is something > > > which we might want to avoid. > > > > > > As a compromise I've chosen to add the extra unconditional _REG call > > > to pinctrl-cherryview.c because: > > > > > > 1. The problem in the DSDT in question stems from there being 2 > > > different OpRegions for accessing GPIOs which AFAIK is unique to > > > cherryview > > > > > > 2. I've seen many many cherryview DSDT-s and as such I'm confident > > > that calling _REG twice is not an issue on cherryview. > > > > > > > Are the ACPI tables from this system available somewhere? > > > > > > Here you go: > > > https://fedorapeople.org/~jwrdegoede/medion-e1239t-dsdt.dsl > > > > Thanks for sharing! > > > > > The problem is that on line 12624 there is a GPO2.AVBL == One > > > check, before GPO2.DCDT is used. If you then look at line > > > 17688 you see that _REG for the GPO2 device checkes for a > > > space-id of 8 (ACPI_ADR_SPACE_GPIO) to set AVBL > > > > > > But the only OpRegion defined for the GPO2 device, and the > > > OpRegion to which GPO2.DCDT is mapped is the cherryview > > > UserDefined 0x93 GPIO access OpRegion, see line 17760. > > > Since there is no OpRegion for the ACPI_ADR_SPACE_GPIO > > > space-id, ACPICA never calls _REG with Arg0 == 8. > > > > Indeed, I see the issue now. I guess calling _REG always when there is > > handler installed would solve this as well? > > Yes that should solve the issue, that is actualy more or less > what my patch does, but my patch only does it for the > pinctrl-cherryview.c case. And ACPICA guys, in case of thinking about generic solution there, can also have a look into ACPI hotplug code. Something tells me that there may be the very same root cause. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called 2020-05-07 12:30 ` Mika Westerberg 2020-05-07 12:39 ` Hans de Goede @ 2020-10-08 9:31 ` Hans de Goede 2020-10-08 14:44 ` Mika Westerberg 1 sibling, 1 reply; 12+ messages in thread From: Hans de Goede @ 2020-10-08 9:31 UTC (permalink / raw) To: Mika Westerberg Cc: Andy Shevchenko, Linus Walleij, linux-acpi, linux-gpio, stable, Rafael J. Wysocki, Bob Moore, Erik Kaneda [-- Attachment #1: Type: text/plain, Size: 4070 bytes --] 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 [-- Attachment #2: 0001-ACPICA-Also-handle-orphan-_REG-methods-for-GPIO-OpRe.patch --] [-- Type: text/x-patch, Size: 7471 bytes --] From a2729247cb69707c0244e84cfa4316cffc63b35f Mon Sep 17 00:00:00 2001 From: Hans de Goede <hdegoede@redhat.com> Date: Wed, 22 Jul 2020 22:22:13 +0200 Subject: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO OpRegions Before this commit acpi_ev_execute_reg_methods() had special handling to handle "orphan" (no matching OpRegion declared) _REG methods for EC nodes. On Intel 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. Except for the "orphan" EC handling, 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. This commit fixes this by extending the "orphan" _REG method handling to also apply to GPIO address-space handlers. Note it seems that Windows always calls "orphan" _REG methods so me may want to consider dropping the space-id check and always do "orphan" _REG method handling. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/acpi/acpica/evregion.c | 54 +++++++++++++++++----------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c index 738d4b231f34..21ff341e34a4 100644 --- a/drivers/acpi/acpica/evregion.c +++ b/drivers/acpi/acpica/evregion.c @@ -21,7 +21,8 @@ extern u8 acpi_gbl_default_address_spaces[]; /* Local prototypes */ static void -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node); +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *device_node, + acpi_adr_space_type space_id); static acpi_status acpi_ev_reg_run(acpi_handle obj_handle, @@ -684,10 +685,12 @@ acpi_ev_execute_reg_methods(struct acpi_namespace_node *node, ACPI_NS_WALK_UNLOCK, acpi_ev_reg_run, NULL, &info, NULL); - /* Special case for EC: handle "orphan" _REG methods with no region */ - - if (space_id == ACPI_ADR_SPACE_EC) { - acpi_ev_orphan_ec_reg_method(node); + /* + * Special case for EC and GPIO: handle "orphan" _REG methods with + * no region. + */ + if (space_id == ACPI_ADR_SPACE_EC || space_id == ACPI_ADR_SPACE_GPIO) { + acpi_ev_execute_orphan_reg_method(node, space_id); } ACPI_DEBUG_PRINT_RAW((ACPI_DB_NAMES, @@ -760,31 +763,28 @@ acpi_ev_reg_run(acpi_handle obj_handle, /******************************************************************************* * - * FUNCTION: acpi_ev_orphan_ec_reg_method + * FUNCTION: acpi_ev_execute_orphan_reg_method * - * PARAMETERS: ec_device_node - Namespace node for an EC device + * PARAMETERS: device_node - Namespace node for an ACPI device + * space_id - The address space ID * * RETURN: None * - * DESCRIPTION: Execute an "orphan" _REG method that appears under the EC + * DESCRIPTION: Execute an "orphan" _REG method that appears under an ACPI * device. This is a _REG method that has no corresponding region - * within the EC device scope. The orphan _REG method appears to - * have been enabled by the description of the ECDT in the ACPI - * specification: "The availability of the region space can be - * detected by providing a _REG method object underneath the - * Embedded Controller device." - * - * To quickly access the EC device, we use the ec_device_node used - * during EC handler installation. Otherwise, we would need to - * perform a time consuming namespace walk, executing _HID - * methods to find the EC device. + * within the device's scope. ACPI tables depending on these + * "orphan" _REG methods have been seen for both EC and GPIO + * Operation Regions. Presumably the Windows ACPI implementation + * always calls the _REG method independent of the presence of + * an actual Operation Region with the correct address space ID. * * MUTEX: Assumes the namespace is locked * ******************************************************************************/ static void -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *device_node, + acpi_adr_space_type space_id) { acpi_handle reg_method; struct acpi_namespace_node *next_node; @@ -792,9 +792,9 @@ acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) struct acpi_object_list args; union acpi_object objects[2]; - ACPI_FUNCTION_TRACE(ev_orphan_ec_reg_method); + ACPI_FUNCTION_TRACE(ev_execute_orphan_reg_method); - if (!ec_device_node) { + if (!device_node) { return_VOID; } @@ -804,7 +804,7 @@ acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) /* Get a handle to a _REG method immediately under the EC device */ - status = acpi_get_handle(ec_device_node, METHOD_NAME__REG, ®_method); + status = acpi_get_handle(device_node, METHOD_NAME__REG, ®_method); if (ACPI_FAILURE(status)) { goto exit; /* There is no _REG method present */ } @@ -816,23 +816,23 @@ acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) * with other space IDs to be present; but the code below will then * execute the _REG method with the embedded_control space_ID argument. */ - next_node = acpi_ns_get_next_node(ec_device_node, NULL); + next_node = acpi_ns_get_next_node(device_node, NULL); while (next_node) { if ((next_node->type == ACPI_TYPE_REGION) && (next_node->object) && - (next_node->object->region.space_id == ACPI_ADR_SPACE_EC)) { + (next_node->object->region.space_id == space_id)) { goto exit; /* Do not execute the _REG */ } - next_node = acpi_ns_get_next_node(ec_device_node, next_node); + next_node = acpi_ns_get_next_node(device_node, next_node); } - /* Evaluate the _REG(embedded_control,Connect) method */ + /* Evaluate the _REG(space_id, Connect) method */ args.count = 2; args.pointer = objects; objects[0].type = ACPI_TYPE_INTEGER; - objects[0].integer.value = ACPI_ADR_SPACE_EC; + objects[0].integer.value = space_id; objects[1].type = ACPI_TYPE_INTEGER; objects[1].integer.value = ACPI_REG_CONNECT; -- 2.28.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called 2020-10-08 9:31 ` Hans de Goede @ 2020-10-08 14:44 ` Mika Westerberg 2020-10-08 15:37 ` Hans de Goede 0 siblings, 1 reply; 12+ messages in thread From: Mika Westerberg @ 2020-10-08 14:44 UTC (permalink / raw) To: Hans de Goede Cc: Andy Shevchenko, Linus Walleij, linux-acpi, linux-gpio, stable, Rafael J. Wysocki, Bob Moore, Erik Kaneda Hi, On Thu, Oct 08, 2020 at 11:31:50AM +0200, Hans de Goede wrote: > 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 ? IIRC we sometimes take the ACPICA related patches first to Linux and then it gets picked up by the ACPICA maintainers. I think Erik Kaneda (who is Cc'd on this thread) has been doing some of that work. Erik, Rafael, can you help us out here? What is the best way for Hans to get the below patch to the upstream ACPICA? > Regards, > > Hans > > > > >From a2729247cb69707c0244e84cfa4316cffc63b35f Mon Sep 17 00:00:00 2001 > From: Hans de Goede <hdegoede@redhat.com> > Date: Wed, 22 Jul 2020 22:22:13 +0200 > Subject: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO OpRegions > > Before this commit acpi_ev_execute_reg_methods() had special handling > to handle "orphan" (no matching OpRegion declared) _REG methods for EC > nodes. > > On Intel 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. > > Except for the "orphan" EC handling, 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. > > This commit fixes this by extending the "orphan" _REG method handling > to also apply to GPIO address-space handlers. > > Note it seems that Windows always calls "orphan" _REG methods so me > may want to consider dropping the space-id check and always do > "orphan" _REG method handling. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/acpi/acpica/evregion.c | 54 +++++++++++++++++----------------- > 1 file changed, 27 insertions(+), 27 deletions(-) > > diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c > index 738d4b231f34..21ff341e34a4 100644 > --- a/drivers/acpi/acpica/evregion.c > +++ b/drivers/acpi/acpica/evregion.c > @@ -21,7 +21,8 @@ extern u8 acpi_gbl_default_address_spaces[]; > /* Local prototypes */ > > static void > -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node); > +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *device_node, > + acpi_adr_space_type space_id); > > static acpi_status > acpi_ev_reg_run(acpi_handle obj_handle, > @@ -684,10 +685,12 @@ acpi_ev_execute_reg_methods(struct acpi_namespace_node *node, > ACPI_NS_WALK_UNLOCK, acpi_ev_reg_run, NULL, > &info, NULL); > > - /* Special case for EC: handle "orphan" _REG methods with no region */ > - > - if (space_id == ACPI_ADR_SPACE_EC) { > - acpi_ev_orphan_ec_reg_method(node); > + /* > + * Special case for EC and GPIO: handle "orphan" _REG methods with > + * no region. > + */ > + if (space_id == ACPI_ADR_SPACE_EC || space_id == ACPI_ADR_SPACE_GPIO) { > + acpi_ev_execute_orphan_reg_method(node, space_id); > } > > ACPI_DEBUG_PRINT_RAW((ACPI_DB_NAMES, > @@ -760,31 +763,28 @@ acpi_ev_reg_run(acpi_handle obj_handle, > > /******************************************************************************* > * > - * FUNCTION: acpi_ev_orphan_ec_reg_method > + * FUNCTION: acpi_ev_execute_orphan_reg_method > * > - * PARAMETERS: ec_device_node - Namespace node for an EC device > + * PARAMETERS: device_node - Namespace node for an ACPI device > + * space_id - The address space ID > * > * RETURN: None > * > - * DESCRIPTION: Execute an "orphan" _REG method that appears under the EC > + * DESCRIPTION: Execute an "orphan" _REG method that appears under an ACPI > * device. This is a _REG method that has no corresponding region > - * within the EC device scope. The orphan _REG method appears to > - * have been enabled by the description of the ECDT in the ACPI > - * specification: "The availability of the region space can be > - * detected by providing a _REG method object underneath the > - * Embedded Controller device." > - * > - * To quickly access the EC device, we use the ec_device_node used > - * during EC handler installation. Otherwise, we would need to > - * perform a time consuming namespace walk, executing _HID > - * methods to find the EC device. > + * within the device's scope. ACPI tables depending on these > + * "orphan" _REG methods have been seen for both EC and GPIO > + * Operation Regions. Presumably the Windows ACPI implementation > + * always calls the _REG method independent of the presence of > + * an actual Operation Region with the correct address space ID. > * > * MUTEX: Assumes the namespace is locked > * > ******************************************************************************/ > > static void > -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) > +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *device_node, > + acpi_adr_space_type space_id) > { > acpi_handle reg_method; > struct acpi_namespace_node *next_node; > @@ -792,9 +792,9 @@ acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) > struct acpi_object_list args; > union acpi_object objects[2]; > > - ACPI_FUNCTION_TRACE(ev_orphan_ec_reg_method); > + ACPI_FUNCTION_TRACE(ev_execute_orphan_reg_method); > > - if (!ec_device_node) { > + if (!device_node) { > return_VOID; > } > > @@ -804,7 +804,7 @@ acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) > > /* Get a handle to a _REG method immediately under the EC device */ > > - status = acpi_get_handle(ec_device_node, METHOD_NAME__REG, ®_method); > + status = acpi_get_handle(device_node, METHOD_NAME__REG, ®_method); > if (ACPI_FAILURE(status)) { > goto exit; /* There is no _REG method present */ > } > @@ -816,23 +816,23 @@ acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) > * with other space IDs to be present; but the code below will then > * execute the _REG method with the embedded_control space_ID argument. > */ > - next_node = acpi_ns_get_next_node(ec_device_node, NULL); > + next_node = acpi_ns_get_next_node(device_node, NULL); > while (next_node) { > if ((next_node->type == ACPI_TYPE_REGION) && > (next_node->object) && > - (next_node->object->region.space_id == ACPI_ADR_SPACE_EC)) { > + (next_node->object->region.space_id == space_id)) { > goto exit; /* Do not execute the _REG */ > } > > - next_node = acpi_ns_get_next_node(ec_device_node, next_node); > + next_node = acpi_ns_get_next_node(device_node, next_node); > } > > - /* Evaluate the _REG(embedded_control,Connect) method */ > + /* Evaluate the _REG(space_id, Connect) method */ > > args.count = 2; > args.pointer = objects; > objects[0].type = ACPI_TYPE_INTEGER; > - objects[0].integer.value = ACPI_ADR_SPACE_EC; > + objects[0].integer.value = space_id; > objects[1].type = ACPI_TYPE_INTEGER; > objects[1].integer.value = ACPI_REG_CONNECT; > > -- > 2.28.0 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called 2020-10-08 14:44 ` Mika Westerberg @ 2020-10-08 15:37 ` Hans de Goede 2020-10-08 15:52 ` Mika Westerberg 0 siblings, 1 reply; 12+ messages in thread From: Hans de Goede @ 2020-10-08 15:37 UTC (permalink / raw) To: Mika Westerberg Cc: Andy Shevchenko, Linus Walleij, linux-acpi, linux-gpio, stable, Rafael J. Wysocki, Bob Moore, Erik Kaneda Hi, On 10/8/20 4:44 PM, Mika Westerberg wrote: > Hi, > > On Thu, Oct 08, 2020 at 11:31:50AM +0200, Hans de Goede wrote: >> 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 ? > > IIRC we sometimes take the ACPICA related patches first to Linux and > then it gets picked up by the ACPICA maintainers. I think Erik Kaneda > (who is Cc'd on this thread) has been doing some of that work. > > Erik, Rafael, can you help us out here? What is the best way for Hans to > get the below patch to the upstream ACPICA? Thanks Mika, I did not know that getting it into Linux directly was an option. Mika, do you have input wrt always calling _REG for just the GpioIoOpRegion type (on top of the existing EC exception) vs just simply always calling it for all all/more OpRegion types ? Regards, Hans >> >From a2729247cb69707c0244e84cfa4316cffc63b35f Mon Sep 17 00:00:00 2001 >> From: Hans de Goede <hdegoede@redhat.com> >> Date: Wed, 22 Jul 2020 22:22:13 +0200 >> Subject: [PATCH] ACPICA: Also handle "orphan" _REG methods for GPIO OpRegions >> >> Before this commit acpi_ev_execute_reg_methods() had special handling >> to handle "orphan" (no matching OpRegion declared) _REG methods for EC >> nodes. >> >> On Intel 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. >> >> Except for the "orphan" EC handling, 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. >> >> This commit fixes this by extending the "orphan" _REG method handling >> to also apply to GPIO address-space handlers. >> >> Note it seems that Windows always calls "orphan" _REG methods so me >> may want to consider dropping the space-id check and always do >> "orphan" _REG method handling. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/acpi/acpica/evregion.c | 54 +++++++++++++++++----------------- >> 1 file changed, 27 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c >> index 738d4b231f34..21ff341e34a4 100644 >> --- a/drivers/acpi/acpica/evregion.c >> +++ b/drivers/acpi/acpica/evregion.c >> @@ -21,7 +21,8 @@ extern u8 acpi_gbl_default_address_spaces[]; >> /* Local prototypes */ >> >> static void >> -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node); >> +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *device_node, >> + acpi_adr_space_type space_id); >> >> static acpi_status >> acpi_ev_reg_run(acpi_handle obj_handle, >> @@ -684,10 +685,12 @@ acpi_ev_execute_reg_methods(struct acpi_namespace_node *node, >> ACPI_NS_WALK_UNLOCK, acpi_ev_reg_run, NULL, >> &info, NULL); >> >> - /* Special case for EC: handle "orphan" _REG methods with no region */ >> - >> - if (space_id == ACPI_ADR_SPACE_EC) { >> - acpi_ev_orphan_ec_reg_method(node); >> + /* >> + * Special case for EC and GPIO: handle "orphan" _REG methods with >> + * no region. >> + */ >> + if (space_id == ACPI_ADR_SPACE_EC || space_id == ACPI_ADR_SPACE_GPIO) { >> + acpi_ev_execute_orphan_reg_method(node, space_id); >> } >> >> ACPI_DEBUG_PRINT_RAW((ACPI_DB_NAMES, >> @@ -760,31 +763,28 @@ acpi_ev_reg_run(acpi_handle obj_handle, >> >> /******************************************************************************* >> * >> - * FUNCTION: acpi_ev_orphan_ec_reg_method >> + * FUNCTION: acpi_ev_execute_orphan_reg_method >> * >> - * PARAMETERS: ec_device_node - Namespace node for an EC device >> + * PARAMETERS: device_node - Namespace node for an ACPI device >> + * space_id - The address space ID >> * >> * RETURN: None >> * >> - * DESCRIPTION: Execute an "orphan" _REG method that appears under the EC >> + * DESCRIPTION: Execute an "orphan" _REG method that appears under an ACPI >> * device. This is a _REG method that has no corresponding region >> - * within the EC device scope. The orphan _REG method appears to >> - * have been enabled by the description of the ECDT in the ACPI >> - * specification: "The availability of the region space can be >> - * detected by providing a _REG method object underneath the >> - * Embedded Controller device." >> - * >> - * To quickly access the EC device, we use the ec_device_node used >> - * during EC handler installation. Otherwise, we would need to >> - * perform a time consuming namespace walk, executing _HID >> - * methods to find the EC device. >> + * within the device's scope. ACPI tables depending on these >> + * "orphan" _REG methods have been seen for both EC and GPIO >> + * Operation Regions. Presumably the Windows ACPI implementation >> + * always calls the _REG method independent of the presence of >> + * an actual Operation Region with the correct address space ID. >> * >> * MUTEX: Assumes the namespace is locked >> * >> ******************************************************************************/ >> >> static void >> -acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) >> +acpi_ev_execute_orphan_reg_method(struct acpi_namespace_node *device_node, >> + acpi_adr_space_type space_id) >> { >> acpi_handle reg_method; >> struct acpi_namespace_node *next_node; >> @@ -792,9 +792,9 @@ acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) >> struct acpi_object_list args; >> union acpi_object objects[2]; >> >> - ACPI_FUNCTION_TRACE(ev_orphan_ec_reg_method); >> + ACPI_FUNCTION_TRACE(ev_execute_orphan_reg_method); >> >> - if (!ec_device_node) { >> + if (!device_node) { >> return_VOID; >> } >> >> @@ -804,7 +804,7 @@ acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) >> >> /* Get a handle to a _REG method immediately under the EC device */ >> >> - status = acpi_get_handle(ec_device_node, METHOD_NAME__REG, ®_method); >> + status = acpi_get_handle(device_node, METHOD_NAME__REG, ®_method); >> if (ACPI_FAILURE(status)) { >> goto exit; /* There is no _REG method present */ >> } >> @@ -816,23 +816,23 @@ acpi_ev_orphan_ec_reg_method(struct acpi_namespace_node *ec_device_node) >> * with other space IDs to be present; but the code below will then >> * execute the _REG method with the embedded_control space_ID argument. >> */ >> - next_node = acpi_ns_get_next_node(ec_device_node, NULL); >> + next_node = acpi_ns_get_next_node(device_node, NULL); >> while (next_node) { >> if ((next_node->type == ACPI_TYPE_REGION) && >> (next_node->object) && >> - (next_node->object->region.space_id == ACPI_ADR_SPACE_EC)) { >> + (next_node->object->region.space_id == space_id)) { >> goto exit; /* Do not execute the _REG */ >> } >> >> - next_node = acpi_ns_get_next_node(ec_device_node, next_node); >> + next_node = acpi_ns_get_next_node(device_node, next_node); >> } >> >> - /* Evaluate the _REG(embedded_control,Connect) method */ >> + /* Evaluate the _REG(space_id, Connect) method */ >> >> args.count = 2; >> args.pointer = objects; >> objects[0].type = ACPI_TYPE_INTEGER; >> - objects[0].integer.value = ACPI_ADR_SPACE_EC; >> + objects[0].integer.value = space_id; >> objects[1].type = ACPI_TYPE_INTEGER; >> objects[1].integer.value = ACPI_REG_CONNECT; >> >> -- >> 2.28.0 >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called 2020-10-08 15:37 ` Hans de Goede @ 2020-10-08 15:52 ` Mika Westerberg 2020-10-08 17:39 ` Hans de Goede 0 siblings, 1 reply; 12+ messages in thread From: Mika Westerberg @ 2020-10-08 15:52 UTC (permalink / raw) To: Hans de Goede Cc: Andy Shevchenko, Linus Walleij, linux-acpi, linux-gpio, stable, Rafael J. Wysocki, Bob Moore, Erik Kaneda On Thu, Oct 08, 2020 at 05:37:10PM +0200, Hans de Goede wrote: > Mika, do you have input wrt always calling _REG for just the > GpioIoOpRegion type (on top of the existing EC exception) vs > just simply always calling it for all all/more OpRegion types ? IMO it is safer to call it only for GPIO (GpioIoOpRegion) now. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called 2020-10-08 15:52 ` Mika Westerberg @ 2020-10-08 17:39 ` Hans de Goede 0 siblings, 0 replies; 12+ messages in thread From: Hans de Goede @ 2020-10-08 17:39 UTC (permalink / raw) To: Mika Westerberg Cc: Andy Shevchenko, Linus Walleij, linux-acpi, linux-gpio, stable, Rafael J. Wysocki, Bob Moore, Erik Kaneda Hi, On 10/8/20 5:52 PM, Mika Westerberg wrote: > On Thu, Oct 08, 2020 at 05:37:10PM +0200, Hans de Goede wrote: >> Mika, do you have input wrt always calling _REG for just the >> GpioIoOpRegion type (on top of the existing EC exception) vs >> just simply always calling it for all all/more OpRegion types ? > > IMO it is safer to call it only for GPIO (GpioIoOpRegion) now. That was my thought too, and is what my current patch does. Thank you for your input on this. Regards, Hans ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called 2020-05-04 14:59 [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called Hans de Goede 2020-05-06 6:40 ` Mika Westerberg @ 2020-05-06 23:42 ` Sasha Levin 1 sibling, 0 replies; 12+ messages in thread From: Sasha Levin @ 2020-05-06 23:42 UTC (permalink / raw) To: Sasha Levin, Hans de Goede, Mika Westerberg Cc: Hans de Goede, linux-acpi, stable, stable Hi [This is an automated email] This commit has been processed because it contains a -stable tag. The stable tag indicates that it's relevant for the following trees: all The bot has tested the following trees: v5.6.10, v5.4.38, v4.19.120, v4.14.178, v4.9.221, v4.4.221. v5.6.10: Build OK! v5.4.38: Build OK! v4.19.120: Build OK! v4.14.178: Build OK! v4.9.221: Failed to apply! Possible dependencies: a0b028597d59 ("pinctrl: cherryview: Add support for GMMR GPIO opregion") v4.4.221: Build OK! NOTE: The patch will not be queued to stable trees until it is upstream. How should we proceed with this patch? -- Thanks Sasha ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-10-08 17:39 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-04 14:59 [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called Hans de Goede 2020-05-06 6:40 ` Mika Westerberg 2020-05-07 10:15 ` Hans de Goede 2020-05-07 12:30 ` Mika Westerberg 2020-05-07 12:39 ` Hans de Goede 2020-05-07 13:39 ` Andy Shevchenko 2020-10-08 9:31 ` Hans de Goede 2020-10-08 14:44 ` Mika Westerberg 2020-10-08 15:37 ` Hans de Goede 2020-10-08 15:52 ` Mika Westerberg 2020-10-08 17:39 ` Hans de Goede 2020-05-06 23:42 ` Sasha Levin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).