Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Sachi King <nakato@nakato.io>
To:     "Limonciello, Mario" <mario.limonciello@amd.com>,
        hdegoede@redhat.com, mgross@linux.intel.com, rafael@kernel.org,
        lenb@kernel.org, Sanket.Goswami@amd.com,
        Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Cc:     platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-acpi@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH 1/2] platform/x86: amd-pmc: Add alternative acpi id for PMC controller
Date:   Fri, 08 Oct 2021 23:19:35 +1100
Message-ID: <1837953.FDaK0lLtFO@youmu>
In-Reply-To: <609f5254-4527-38b8-3d1d-5cb06791e103@amd.com>
References: <20211002041840.2058647-1-nakato@nakato.io> <909f28e9-245a-df90-52f1-98b0f63a2b3a@amd.com> <609f5254-4527-38b8-3d1d-5cb06791e103@amd.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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.