| Message ID | 20211002041840.2058647-1-nakato@nakato.io (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [1/2] platform/x86: amd-pmc: Add alternative acpi id for PMC controller | expand |
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' Thanks, Shyam
On Tuesday, 5 October 2021 16:16:18 AEDT 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 The ACPI dump for this device is available here: https://github.com/linux-surface/acpidumps/tree/master/surface_laptop_4_amd > output of 'cat /sys/power/mem_sleep' [s2idle] Thanks, Sachi
+Sanket Goswami 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' > > Thanks, > Shyam > 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 and verified that the amd_pmc debugfs statistics are increasing? Is the system able to resume from s2idle? 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.
On 10/8/2021 1:30 AM, Limonciello, Mario wrote: > +Sanket Goswami > > 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' >> >> Thanks, >> Shyam >> > > 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 and verified that the amd_pmc debugfs > statistics are increasing? Is the system able to resume from s2idle? 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://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next&id=9cfe02023cf67a36c2dfb05d1ea3eb79811a8720 this would tell the last s0i3 status, whether it was successful or not. cat /sys/kernel/debug/amd_pmc/smu_fw_info > > 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.
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. https://github.com/linux-surface/linux-surface/issues/591#issuecomment-936891479 > > 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://lore.kernel.org/lkml/87lf8ddjqx.ffs@nanos.tec.linutronix.de/ Also available here: https://github.com/linux-surface/kernel/commit/25baf27d6d76f068ab8e7cb7a5be33218ac9bd6b 2. pinctrl: amd: Handle wake-up interrupt https://git.kernel.org/torvalds/c/acd47b9f28e5 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. 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://lore.kernel.org/linux-gpio/20211001161714.2053597-1-nakato@nakato.io/T/#t 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://github.com/linux-surface/kernel/commit/1927c0b30e5cd95a566a23b6926472bc2be54f42 https://github.com/linux-surface/kernel/commit/ac1a977392880456f61e830a95e368cad7a0fa3f > 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://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next&id=9cfe02023cf67a36c2dfb05d1ea3eb79811a8720 My tree currently does not have that one. I've applied it. > 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 > > 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.
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. > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flinux-surface%2Flinux-surface%2Fissues%2F591%23issuecomment-936891479&data=04%7C01%7Cmario.limonciello%40amd.com%7Cb95422d699a2496a56f608d98a55e888%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637692923846585025%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=MND10b0iIblTgywFsxoLNx7D1bZuLZOmbqbhQJiezxM%3D&reserved=0 > 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@amd.com> > >>> 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. > > > >
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. >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flinux-surface%2Flinux-surface%2Fissues%2F591%23issuecomment-936891479&data=04%7C01%7Cmario.limonciello%40amd.com%7Cb95422d699a2496a56f608d98a55e888%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637692923846585025%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=MND10b0iIblTgywFsxoLNx7D1bZuLZOmbqbhQJiezxM%3D&reserved=0 >> >> > > 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@amd.com> > >> >>>> 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. Thanks,
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@amd.com> > > > >> > >>>> 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. Thanks
Hi, On 10/2/21 6:18 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. > > Cc: <stable@vger.kernel.org> # 5.14+ > Signed-off-by: Sachi King <nakato@nakato.io> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. I will also include this in my upcoming pdx86-fixes pull-req for 5.15 . Regards, Hans > --- > drivers/platform/x86/amd-pmc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c > index d6a7c896ac86..fc95620101e8 100644 > --- a/drivers/platform/x86/amd-pmc.c > +++ b/drivers/platform/x86/amd-pmc.c > @@ -476,6 +476,7 @@ static const struct acpi_device_id amd_pmc_acpi_ids[] = { > {"AMDI0006", 0}, > {"AMDI0007", 0}, > {"AMD0004", 0}, > + {"AMD0005", 0}, > { } > }; > MODULE_DEVICE_TABLE(acpi, amd_pmc_acpi_ids); >
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@amd.com> >>> >>>> >>>>>> 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@amd.com> Regards, Hans
diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c index d6a7c896ac86..fc95620101e8 100644 --- a/drivers/platform/x86/amd-pmc.c +++ b/drivers/platform/x86/amd-pmc.c @@ -476,6 +476,7 @@ static const struct acpi_device_id amd_pmc_acpi_ids[] = { {"AMDI0006", 0}, {"AMDI0007", 0}, {"AMD0004", 0}, + {"AMD0005", 0}, { } }; MODULE_DEVICE_TABLE(acpi, amd_pmc_acpi_ids);
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. Cc: <stable@vger.kernel.org> # 5.14+ Signed-off-by: Sachi King <nakato@nakato.io> --- drivers/platform/x86/amd-pmc.c | 1 + 1 file changed, 1 insertion(+)