Re: [PATCH 1/2] platform/x86: amd-pmc: Add alternative acpi id for PMC controller
From: Hans de Goede
Date: Mon Oct 11 2021 - 09:49:52 EST
Hi,
On 10/9/21 4:05 AM, Sachi King wrote:
> On Saturday, 9 October 2021 06:01:53 AEDT Limonciello, Mario wrote:
>> On 10/8/2021 10:57, Limonciello, Mario wrote:
>>> On 10/8/2021 07:19, Sachi King wrote:
>>>> On Friday, 8 October 2021 21:27:15 AEDT Shyam Sundar S K wrote:
>>>>>
>>>>> On 10/8/2021 1:30 AM, Limonciello, Mario wrote:
>>>>>>
>>>>>> On 10/5/2021 00:16, Shyam Sundar S K wrote:
>>>>>>>
>>>>>>> On 10/2/2021 9:48 AM, Sachi King wrote:
>>>>>>>> The Surface Laptop 4 AMD has used the AMD0005 to identify this
>>>>>>>> controller instead of using the appropriate ACPI ID AMDI0005.
>>>>>>>> Include
>>>>>>>> AMD0005 in the acpi id list.
>>>>>>>
>>>>>>> Can you provide an ACPI dump and output of 'cat /sys/power/mem_sleep'
>>>>>>
>>>>>> I had a look through the acpidump listed there and it seems like the
>>>>>> PEP
>>>>>> device is filled with a lot of NO-OP type of code. This means the LPS0
>>>>>> patch really isn't "needed", but still may be a good idea to include
>>>>>> for
>>>>>> completeness in case there ends up being a design based upon this that
>>>>>> does need it.
>>>>>>
>>>>>> As for this one (the amd-pmc patch) how are things working with it?
>>>>>> Have
>>>>>> you checked power consumption
>>>>
>>>> Using my rather limited plug-in power meter I measure 1w with this patch,
>>>> and I've never seen the meter go below this reading, so this may be over
>>>> reporting. Without this patch however the device bounces around
>>>> 2.2-2.5w.
>>>> The device consumes 6w with the display off.
>>>>
>>>> I have not left the device for long periods of time to see what the
>>>> battery
>>>> consumption is over a period of time, however this patch is being carried
>>>> in linux-surface in advance and one users suspend power consumption is
>>>> looking good. They have reported 2 hours of suspend without a noticable
>>>> power drop from the battery indicator.
>>>>
>>>>
>>>
>>> Thanks, in that case this is certainly part of what you'll need and it
>>> sounds like you're on the right train as it pertains to the wakeup sources.
>>>
>>> For both patches in this series:
>>>
>>> Reviewed-by: Mario Limonciello <mario.limonciello@xxxxxxx>
>>>
>>>>
>>>>>> and verified that the amd_pmc debugfs
>>>>>> statistics are increasing?
>>>>
>>>> s0ix_stats included following smu_fw_info below.
>>>>
>>>>>> Is the system able to resume from s2idle?
>>>>
>>>> It does, however additional patches are required to do so without an
>>>> external
>>>> device such as a keyboard. The power button, lid, and power plug trigger
>>>> events via pinctrl-amd. Keyboard and trackpad go via the Surface EC and
>>>> require the surface_* drivers, which do not have wakeup support.
>>>>
>>>> 1. The AMDI0031 pinctrl-amd device is setup on Interrupt 7, however
>>>> the APIC
>>>> table does not define an interrupt source override. Right now I'm not
>>>> sure
>>>> how approach producing a quirk for this. linux-surface is carrying
>>>> the hack
>>>> described in
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F87lf8ddjqx.ffs%40nanos.tec.linutronix.de%2F&data=04%7C01%7Cmario.limonciello%40amd.com%7Cb95422d699a2496a56f608d98a55e888%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637692923846585025%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=5dWwpgh%2FRIA%2F57UpY5h0l9Snzem%2BNpirgE6ujEHO7aY%3D&reserved=0
>>>>
>>>> Also available here:
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flinux-surface%2Fkernel%2Fcommit%2F25baf27d6d76f068ab8e7cb7a5be33218ac9bd6b&data=04%7C01%7Cmario.limonciello%40amd.com%7Cb95422d699a2496a56f608d98a55e888%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637692923846585025%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=HPZfqPoVUJT8w%2FRD7UaVjegT0iRLDlRkXfOwMx5HS8Q%3D&reserved=0
>>>>
>>>>
>>>> 2. pinctrl: amd: Handle wake-up interrupt
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Ftorvalds%2Fc%2Facd47b9f28e5&data=04%7C01%7Cmario.limonciello%40amd.com%7Cb95422d699a2496a56f608d98a55e888%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637692923846585025%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=gUtHcFKolVIZeHtIIJuT3BkruQbjq8NAOU5504%2F02Mg%3D&reserved=0
>>>>
>>>> Without this patch the device would suspend, but any interrupt via
>>>> pinctrl-amd would result in a failed resume, which is every wakeup
>>>> souce I know of on this device.
>>>
>>> Yes that was the same experience a number of us had on other AMD based
>>> platforms as well which led to this patch being submitted.
>>>
>>>>
>>>> 3. pinctrl: amd: disable and mask interrupts on probe
>>>> Once I worked out that I needed the patch in 2 above the device gets a
>>>> lot
>>>> of spurious wakeups, largely because Surface devices have a second
>>>> embedded
>>>> controller that wants to wake the device on all sorts of events. We
>>>> don't
>>>> have support for that, and there were a number of interrupts not
>>>> configured
>>>> by linux that were set enabled, unmasked, and wake in s0i3 on boot.
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-gpio%2F20211001161714.2053597-1-nakato%40nakato.io%2FT%2F%23t&data=04%7C01%7Cmario.limonciello%40amd.com%7Cb95422d699a2496a56f608d98a55e888%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637692923846585025%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=mwJgcXBY9zdlTG671KssViHdSwHfq6DCJ2fpeLbRbR4%3D&reserved=0
>>>>
>>>
>>> We'll have to take a look at this to make sure it's not causing a
>>> regression for the other platforms the original patch helped. If it
>>> does, then we'll need some sort of other messaging to accomplish this
>>> for the surface devices.
>>>
>>>>
>>>> These three are enough to be able to wake the device via a lid event,
>>>> or by
>>>> changing the state of the power cable.
>>>>
>>>> 4. The power button requires another pair of patches. These are only
>>>> in the
>>>> linux-surface kernel as qzed would like to run them there for a couple of
>>>> releases before we propose them upstream. These patches change the
>>>> method
>>>> used to determine if we should load surfacepro3-button or
>>>> soc-button-array.
>>>> The AMD variant Surface Laptops were loading surfacepro3-button instead
>>>> soc-button-array. They can be seen:
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flinux-surface%2Fkernel%2Fcommit%2F1927c0b30e5cd95a566a23b6926472bc2be54f42&data=04%7C01%7Cmario.limonciello%40amd.com%7Cb95422d699a2496a56f608d98a55e888%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637692923846585025%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=PGWON0kCpByJtsO1rS9wrYr7oH86V%2F8M%2FYLmUoFjBhM%3D&reserved=0
>>>>
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flinux-surface%2Fkernel%2Fcommit%2Fac1a977392880456f61e830a95e368cad7a0fa3f&data=04%7C01%7Cmario.limonciello%40amd.com%7Cb95422d699a2496a56f608d98a55e888%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637692923846585025%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=B%2BBW3M4L5TLCq3Fc6oB0KHaC9A%2FQp3uwkB2Jby%2FdDo8%3D&reserved=0
>>>>
>>>>
>>>>
>>>>> Echo-ing to what Mario said, I am also equally interested in knowing the
>>>>> the surface devices are able to reach S2Idle.
>>>>>
>>>>> Spefically can you check if your tree has this commit?
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fpdx86%2Fplatform-drivers-x86.git%2Fcommit%2F%3Fh%3Dfor-next%26id%3D9cfe02023cf67a36c2dfb05d1ea3eb79811a8720&data=04%7C01%7Cmario.limonciello%40amd.com%7Cb95422d699a2496a56f608d98a55e888%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637692923846585025%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=XdRCk8klBuDRCk7UWL%2Ft5wiupVVgdCWBqFmaYgGK%2BFU%3D&reserved=0
>>>>>
>>>>
>>>> My tree currently does not have that one. I've applied it.
>>>
>>> You should look through all the other amd-pmc patches that have happened
>>> as well in linux-next, it's very likely some others will make sense too
>>> for you to be using and testing with.
>>>
>>>>
>>>>> this would tell the last s0i3 status, whether it was successful or not.
>>>>>
>>>>> cat /sys/kernel/debug/amd_pmc/smu_fw_info
>>>>
>>>>
>>>> === SMU Statistics ===
>>>> Table Version: 3
>>>> Hint Count: 1
>>>> Last S0i3 Status: Success
>>>> Time (in us) to S0i3: 102543
>>>> Time (in us) in S0i3: 10790466
>>>>
>>>> === Active time (in us) ===
>>>> DISPLAY : 0
>>>> CPU : 39737
>>>> GFX : 0
>>>> VDD : 39732
>>>> ACP : 0
>>>> VCN : 0
>>>> DF : 18854
>>>> USB0 : 3790
>>>> USB1 : 2647
>>>>
>>>>>> /sys/kernel/debug/amd_pmc/s0ix_stats
>>>>
>>>> After two seperate suspends:
>>>>
>>>> === S0ix statistics ===
>>>> S0ix Entry Time: 19022953504
>>>> S0ix Exit Time: 19485830941
>>>> Residency Time: 9643279
>>>>
>>>> === S0ix statistics ===
>>>> S0ix Entry Time: 21091709805
>>>> S0ix Exit Time: 21586928064
>>>> Residency Time: 10317047
>>>>
>>>>
>>>
>>> Yeah these look good, thanks.
>>>
>>>>>> Does pinctrl-amd load on this system? It seems to me that the power
>>>>>> button GPIO doesn't get used like normally on "regular" UEFI based AMD
>>>>>> systems. I do see MSHW0040 so this is probably supported by
>>>>>> surfacepro3-button and that will probably service all the important
>>>>>> events.
>>>>
>>>> We require the first patch listed above to get pinctrl-amd to load on
>>>> this
>>>> system, and the two patches mentioned in 4 so we correctly choose
>>>> soc-button-array which is used by all recent Surface devices.
>>>>
>>>>
>>>>
>>>>
>>>
>>
>> Sachi,
>>
>> I was talking to some internal folks about this patch. We had one more
>> thought - can you please put into a Github gist (or somewhere
>> semi-permanent) the output of:
>>
>> # cat /sys/kernel/debug/dri/0/amdgpu_firmware_info
>>
>> That way we know more about the FW versions on your system in case of
>> any future regressions stemming from this.
>>
>> Hans,
>>
>> If you can pick up the tag:
>>
>> Link:
>> https://github.com/linux-surface/acpidumps/tree/master/surface_laptop_4_amd
>>
>> as well as that value for "Link: <url>" pointing to amdgpu_firmware_info
>> in the commit message. Or if you want Sachi to re-spin to do
>> themselves, then Sachi feel free to add my Reviewed-by tag in your v2.
>
> Hans,
>
> The requested amdgpu_firmware_info
> Link:
> https://gist.github.com/nakato/2a1a7df1a45fe680d7a08c583e1bf863
>
> If you want me to re-spin with with the these two links and Mario's
> Reviewed-by tag, let me know.
There is no need to resend this, I've just merged it with the following tags added:
Link: https://github.com/linux-surface/acpidumps/tree/master/surface_laptop_4_amd
Link: https://gist.github.com/nakato/2a1a7df1a45fe680d7a08c583e1bf863
Reviewed-by: Mario Limonciello <mario.limonciello@xxxxxxx>
Regards,
Hans