Re: [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called

Andy Shevchenko
2020-05-07T13:39:29Z

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