On 10/26/2021 09:51, Alexandre Belloni wrote:
Hello Mario,
On 20/10/2021 11:29:46-0500, Mario Limonciello wrote:
RTC based wakeup from s0i3 doesn't work properly on some Green Sardine
platforms. Because of this, a newer SMU for Green Sardine has the ability
to pass wakeup time as argument of the upper 16 bits of OS_HINT message.
With older firmware setting the timer value in OS_HINT will cause firmware
to reject the hint, so only run this path on:
1) Green Sardine
2) Minimum SMU FW
3) RTC alarm armed during s0i3 entry
Using this method has some limitations that the s0i3 wakeup will need to
be between 4 seconds and 18 hours, so check those boundary conditions as
well and abort the suspend if RTC is armed for too short or too long of a
duration.
Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
---
drivers/platform/x86/amd-pmc.c | 60 +++++++++++++++++++++++++++++++++-
1 file changed, 59 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index 99ac50616bc3..678bf6874c63 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -17,9 +17,11 @@
#include <linux/delay.h>
#include <linux/io.h>
#include <linux/iopoll.h>
+#include <linux/limits.h>
#include <linux/module.h>
#include <linux/pci.h>
#include <linux/platform_device.h>
+#include <linux/rtc.h>
#include <linux/suspend.h>
#include <linux/seq_file.h>
#include <linux/uaccess.h>
@@ -412,20 +414,76 @@ static int amd_pmc_get_os_hint(struct amd_pmc_dev *dev)
return -EINVAL;
}
+static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
+{
+ struct rtc_device *rtc_device;
+ time64_t then, now, duration;
+ struct rtc_wkalrm alarm;
+ struct rtc_time tm;
+ int rc;
+
+ if (pdev->major < 64 || (pdev->major == 64 && pdev->minor < 53))
+ return 0;
+
+ rtc_device = rtc_class_open(CONFIG_RTC_SYSTOHC_DEVICE);
No this will not work as you can see in your and Arnd's patch adding
ifdefery to handle RTC_SYSTOHC_DEVICE not being set. You have no
guarantee that RTC_SYSTOHC_DEVICE is the device you are looking for. You
need to rework that part.
I'm intending to send a new one back up to the ML to just hardcode it to
"rtc0" per Hans' suggestion.
+ if (!rtc_device)
+ return 0;
+ rc = rtc_read_alarm(rtc_device, &alarm);
+ if (rc)
+ return rc;
+ if (!alarm.enabled) {
+ dev_dbg(pdev->dev, "alarm not enabled\n");
+ return 0;
+ }
+ rc = rtc_valid_tm(&alarm.time);
+ if (rc)
+ return rc;
Why do you think the RTC core give you an invalid alarm time?
It doesn't matter "why" to me. It's defensive programming.
The function has a return code to check for problems. If an invalid
alarm time was somehow programmed, we shouldn't be using it.
+ rc = rtc_read_time(rtc_device, &tm);
+ if (rc)
+ return rc;
+ then = rtc_tm_to_time64(&alarm.time);
+ now = rtc_tm_to_time64(&tm);
+ duration = then-now;
+
+ /* in the past */
+ if (then < now)
+ return 0;
+
+ /* will be stored in upper 16 bits of s0i3 hint argument,
+ * so timer wakeup from s0i3 is limited to ~18 hours or less
+ */
+ if (duration <= 4 || duration > U16_MAX)
+ return -EINVAL;
+
+ *arg |= (duration << 16);
+ rc = rtc_alarm_irq_enable(rtc_device, 0);
What if userspace is waiting for the alarm to happen?
They won't receive it. Note; this is no different than before the patch
on these affected systems.
+ dev_info(pdev->dev, "wakeup timer programmed for %lld seconds\n", duration);
+
Do you actually need this message, looks like leftover debug to me?
Yes it was definitely used in debugging to produce this patch in the
first place.
I left it for two reasons:
1) It makes it clear if someone is using this functionality when they
report an error.
2) It shows if they were operating really close to the boundary
conditions (and thus it may be required to adjust).
I am amenable to downgrading it to dev_dbg and on reported issues around
this asking people to +p dynamic debugging for amd_pmc.
+ return rc;
+}
+
static int __maybe_unused amd_pmc_suspend(struct device *dev)
{
struct amd_pmc_dev *pdev = dev_get_drvdata(dev);
int rc;
u8 msg;
+ u32 arg = 1;
/* Reset and Start SMU logging - to monitor the s0i3 stats */
amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_RESET, 0);
amd_pmc_send_cmd(pdev, 0, NULL, SMU_MSG_LOG_START, 0);
+ /* Activate CZN specific RTC functionality */
+ if (pdev->cpu_id == AMD_CPU_ID_CZN) {
+ rc = amd_pmc_verify_czn_rtc(pdev, &arg);
+ if (rc < 0)
+ return rc;
+ }
+
/* Dump the IdleMask before we send hint to SMU */
amd_pmc_idlemask_read(pdev, dev, NULL);
msg = amd_pmc_get_os_hint(pdev);
- rc = amd_pmc_send_cmd(pdev, 1, NULL, msg, 0);
+ rc = amd_pmc_send_cmd(pdev, arg, NULL, msg, 0);
if (rc)
dev_err(pdev->dev, "suspend failed\n");
--
2.25.1
