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@xxxxxxxxxxxxxxx> 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@xxxxxx> wrote: > > > > > > > Am 10.05.24 um 16:03 schrieb Rafael J. Wysocki: > > > > > > > > > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > > > > > > > > > > > > > > > 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
