Re: [ibm-acpi-devel] [PATCH] platform/x86: thinkpad_acpi: Fix Embedded Controller access on X380 Yoga

["Mark Pearson" <mpearson-lenovo@squebb.ca>]

> Subject: [External] Re: [ibm-acpi-devel] [PATCH] platform/x86: 
> thinkpad_acpi: Fix Embedded Controller access on X380 Yoga
>
> Hi,
>
> On 4/15/23 16:22, Daniel Bertalan wrote:
>> Hi,
>>
>> On Saturday, April 15th, 2023 at 15:30, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>>> Hi,
>>>
>>> On 4/15/23 15:24, Liav Albani wrote:
>>>
>>>> On 4/15/23 13:12, Hans de Goede wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> On 4/14/23 20:02, Daniel Bertalan wrote:
>>>>>
>>>>>> On the X380 Yoga, the `ECRD` and `ECWR` ACPI objects cannot be used for
>>>>>> accessing the Embedded Controller: instead of a method that reads from
>>>>>> the EC's memory, `ECRD` is the name of a location in high memory. This
>>>>>> meant that trying to call them would fail with the following message:
>>>>>>
>>>>>> ACPI: \_SB.PCI0.LPCB.EC.ECRD: 1 arguments were passed to a non-method
>>>>>> ACPI object (RegionField)
>>>>>>
>>>>>> With this commit, it is now possible to access the EC and read
>>>>>> temperature and fan speed information. Note that while writes to the
>>>>>> HFSP register do go through (as indicated by subsequent reads showing
>>>>>> the new value), the fan does not actually change its speed.
>>>>>>
>>>>>> Signed-off-by: Daniel Bertalan dani@danielbertalan.dev
>>>>>> Interestig, this looks like a pretty clean solution to me.
>>>>
>>>> Daniel and I have looked in the DSDT ASL code and found a bunch of registers in high physical memory location (which is an ACPI OpRegion),
>>>> and one of the registers had a bit called ECRD.
>>>> However, there were many other registers that might be interesting as well, the problem is the short names in the ASL code (so we only see abbreviations essentially).
>>>>
>>>> While I do agree that adding this code is indeed a clean solution, if you (people that are dealing with Thinkpad laptops) know about cleaner way to access the embedded controller, I think it's preferable, because this way Daniel might be able to trigger the fan on that laptop so it will actually spin and will dissipate-out heat from the system, without the relying on the embedded controller to get into some sort of thermal state and then to trigger the fan.
>>>
>>>
>>> Have you tried falling back to the ec_read() and ec_write() helpers
>>> exported by the ACPI EC code ?
>>>
>>> The "first_ec" pointer used by these functions is exported too,
>>> so you could try modifying thinkpad_acpi to use ec_read() and
>>> ec_write() as fallback when first_ec is set (and the quirk
>>> added by this patch triggers).
>>
>> This is basically what my patch does. If the ECRD/ECWR method handles
>> are NULL, thinkpad_acpi's acpi_ec_{read,write} functions fall back to
>> the regular ACPI EC helpers you mentioned.
>
> Ah I did not realize that. Ok that sounds good to me.
>
> I'll go and apply the patch then. To be on the save side I'm going
> to only add this to -next, so that it gets some testing before
> showing up in stable series. Once 6.4-rc1 is out you can then
> send it to stable@vger.kernel.org to get it backported.
>
>> Speaking of which, does anyone know why these private helper functions
>> exist in the first place? The code seems to use them interchangeably.
>> Even in the fan control code, there are places where the regular EC
>> helpers are called directly. Can we get away with always doing that?
>
> I assume that on some older models there is no standard ACPI EC device
> in the ACPI tables, so there only ECRD/ECWR work. I guess that code-paths
> which directly call ec_read() / ec_write() are only used on newer
> models. But this does indeed seem inconsistent.
>
>> Back to the issue at hand, is there someone we could ask if the X380Y
>> even supports manual fan control in the first place? My debugging
>> efforts are starting to look like a wild goose chase.
>>
>> The thermal sensors and fan PWM readings now work, which is better
>> than nothing, but it would be good to get to the bottom of this.
>
> Mark Pearson from Lenovo can hopefully help answer this, but I know
> that he is quite busy atm. Hopefully Mark will get back to you when
> he has some more bandwidth.
>
Sorry for the slow reply on this one.

I checked with the FW team and they confirmed on the x380 Yoga that the implementation is unique and not using the ECRD/WCWR ACPI methods. They didn't say why...but it's not expected to be something done again.

I had missed the question about fan control so didn't ask about that. Is there a reason you need to change the fans? It's generally not recommended as it can violate temperature specs and leads to all sorts of issues.

I don't know if the fact this is a one off makes this a better candidate as a quirk? To reduce the risk of breaking something on other platforms? But the code changes looked sensible to me.

I'll aim to do some builds with it in and test it on a few platforms - but I'm travelling next week so this week (and almost certainly the week after) are a bit hectic.

Thanks
Mark