Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-acpi@vger.kernel.org, linux-gpio@vger.kernel.org,
	stable@vger.kernel.org,
	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
	Bob Moore <robert.moore@intel.com>,
	Erik Kaneda <erik.kaneda@intel.com>
Subject: Re: [PATCH v2] pinctrl: cherryview: Ensure _REG(ACPI_ADR_SPACE_GPIO, 1) gets called
Date: Thu, 7 May 2020 16:39:28 +0300
Message-ID: <20200507133928.GW185537@smile.fi.intel.com> (raw)
In-Reply-To: <3c509026-7d4f-9915-1c5e-dd6002042d92@redhat.com>

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



  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-04 14:59 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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200507133928.GW185537@smile.fi.intel.com \
    --to=andriy.shevchenko@linux.intel.com \
    --cc=erik.kaneda@intel.com \
    --cc=hdegoede@redhat.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=mika.westerberg@linux.intel.com \
    --cc=rafael.j.wysocki@intel.com \
    --cc=robert.moore@intel.com \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git