Received-SPF: pass (google.com: domain of linux-kernel+bounces-176088-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Fri, 10 May 2024 21:17:21 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Mario Limonciello <mario.limonciello@amd.com>,
	Armin Wolf <W_Armin@gmx.de>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Linux ACPI <linux-acpi@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Hans de Goede <hdegoede@redhat.com>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>
Subject: Re: [PATCH v1 1/2] ACPI: EC: Install address space handler at the
 namespace root
Message-ID: <Zj5ksaCzhSGN2FpE@smile.fi.intel.com>
References: <5787281.DvuYhMxLoT@kreacher>
 <4926735.31r3eYUQgx@kreacher>
 <ac04c433-b0ac-4b82-b8eb-98ac16f872d8@gmx.de>
 <CAJZ5v0g_NjGHRvhm-N5vQFnOsqnxExSq99v8n_B_6ANoaCga0w@mail.gmail.com>
 <568291fc-fd79-4f08-9eb7-aed7f5a32345@gmx.de>
 <Zj5ZdcQeaTo9ImT4@smile.fi.intel.com>
 <0cdf0af6-851b-4781-83fe-99320c35544f@amd.com>
 <Zj5eZKgpg3LFpne8@smile.fi.intel.com>
 <CAJZ5v0geYUx6G_ZnTX7+BjEbkqdSovdwriMHBoBXMyvrPjqFmg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAJZ5v0geYUx6G_ZnTX7+BjEbkqdSovdwriMHBoBXMyvrPjqFmg@mail.gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Fri, May 10, 2024 at 08:06:11PM +0200, Rafael J. Wysocki wrote:
> On Fri, May 10, 2024 at 7:50 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, May 10, 2024 at 12:40:05PM -0500, Mario Limonciello wrote:
> > > On 5/10/2024 12:29, Andy Shevchenko wrote:
> > > > On Fri, May 10, 2024 at 06:52:41PM +0200, Armin Wolf wrote:
> > > > > Am 10.05.24 um 18:41 schrieb Rafael J. Wysocki:
> > > > > > On Fri, May 10, 2024 at 6:10 PM Armin Wolf <W_Armin@gmx.de> wrote:
> > > > > > > Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki:
> > > > > > >
> > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > > >
> > > > > > > > It is reported that _DSM evaluation fails in ucsi_acpi_dsm() on Lenovo
> > > > > > > > IdeaPad Pro 5 due to a missing address space handler for the EC address
> > > > > > > > space:
> > > > > > > >
> > > > > > > >     ACPI Error: No handler for Region [ECSI] (000000007b8176ee) [EmbeddedControl] (20230628/evregion-130)
> > > > > > > >
> > > > > > > > This happens because the EC driver only registers the EC address space
> > > > > > > > handler for operation regions defined in the EC device scope of the
> > > > > > > > ACPI namespace while the operation region being accessed by the _DSM
> > > > > > > > in question is located beyond that scope.
> > > > > > > >
> > > > > > > > To address this, modify the ACPI EC driver to install the EC address
> > > > > > > > space handler at the root of the ACPI namespace.
> > > > > > > >
> > > > > > > > Note that this change is consistent with some examples in the ACPI
> > > > > > > > specification in which EC operation regions located outside the EC
> > > > > > > > device scope are used (for example, see Section 9.17.15 in ACPI 6.5),
> > > > > > > > so the current behavior of the EC driver is arguably questionable.
> > > > > > > Hi,
> > > > > > >
> > > > > > > the patch itself looks good to me, but i wonder what happens if multiple
> > > > > > > ACPI EC devices are present. How would we handle such a situation?
> > > > > > I'm wondering if this is a theoretical question or do you have any
> > > > > > existing or planned systems in mind?
> > > > > >
> > > > > > ec_read(), ec_write() and ec_transaction() use only the first EC that
> > > > > > has been found anyway.
> > > > >
> > > > > Its a theoretical question, i do not know of any systems which have more than
> > > > > one ACPI EC device.
> > > >
> > > > The specification is clear about this case in the "ACPI Embedded Controller
> > > > Interface Specification":
> > > >
> > > >   "The ACPI standard supports multiple embedded controllers in a system,
> > > >    each with its own resources. Each embedded controller has a flat
> > > >    byte-addressable I/O space, currently defined as 256 bytes."
> > > >
> > > > However, I haven't checked deeper, so it might be a leftover in the documentation.
> > > >
> > > > The OperationRegion() has no reference to the EC (or in general, device) which
> > > > we need to speak to. The only possibility to declare OpRegion() for the second+
> > > > EC is to use vendor specific RegionSpace, AFAIU. So, even if ACPI specification
> > > > supports 2+ ECs, it doesn't support OpRegion():s for them under the same
> > > > RegionSpace.
> > > >
> > > > That said, the commit message might be extended to summarize this, but at
> > > > the same time I see no way how this series can break anything even in 2+ ECs
> > > > environments.
> > >
> > > It's deviating from the patch, but in practice /why/ would you even want to
> > > have a design with two ECs?  In general that is going to mean a much more
> > > complex state machine with synchronizing the interaction between both of
> > > them and the host.
> > >
> > > Understanding the benefit of such a design might make it easier to
> > > hypothesize impacts.
> >
> > First that comes to my mind (but hypothetical), is the separate CPU/EC add-on
> > cards. If the main firmware somehow supports all of these add-on platforms,
> > it might need to handle 2+ ECs.
> >
> > Again, it might be ACPI specification issue. For instance, the cited piece
> > doesn't tell about 16-bit EC accesses.
> 
> IMV this is a matter of what is testable.
> 
> We can only seriously say that we support 1 EC in the system, because
> that's what we can test.
> 
> Now, the specification allows (theoretically) multiple ECs to be
> supported which does not mean that it will ever be done in practice
> and it also does not mean that this is a good idea.

I briefly read the all mentions of the "Embedded Controller" in the
specification and like 98% implies that the only one is per system. I believe
the specification may be corrected to remove ambiguous plural forms in a couple
(or a few) places. In any case it's a question to ASWG.

-- 
With Best Regards,
Andy Shevchenko