Hi,
On 4/18/23 15:16, Mark Pearson wrote:
>> Subject: [External] Re: [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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx
>>>>>>> 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@xxxxxxxxxxxxxxx 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.
I just remembered that making thinkpad_acpi fan-control
actually requires passing a module-parameter, because as
said generally speaking leaving the fan on auto mode is best.
I wonder if that parameter was set when testing on
the X380 ? I guess it was set since the actual
register was inspected and the changes were visible
there, right ?
Regards,
Hans
_______________________________________________
ibm-acpi-devel mailing list
ibm-acpi-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/ibm-acpi-devel
