[PATCH 0/7] arm64: dts: qcom: x1e80100: enable rtc

[PATCH 0/7] arm64: dts: qcom: x1e80100: enable rtc

[Johan Hovold]

This series adds support for utilising the UEFI firmware RTC offset to
the Qualcomm PMIC RTC driver and uses that to enable the RTC on all X
Elite machines.

Included is also a patch to switch the Lenovo ThinkPad X13s over to
using the UEFI offset.

The RTCs in many Qualcomm devices are effectively broken due to the time
registers being read-only. Instead some other non-volatile memory can be
used to store an offset which a driver can take into account. On Windows
on Arm laptops, the UEFI firmware (and Windows) use a UEFI variable for
storing such an offset.

When RTC support for the X13s was added two years ago we did not yet
have UEFI variable support for these machines in mainline and there were
also some concerns regarding flash wear. [1] As not all Qualcomm
platforms have UEFI firmware anyway, we instead opted to use a PMIC
scratch register for storing the offset. [2]

On the UEFI machines in question this is however arguable not correct
as it means that the RTC time can differ between the UEFI firmware (and
Windows) and Linux.

Now that the (reverse engineered) UEFI variable implementation has been
merged and thoroughly tested, let's switch to using that to store the
RTC offset also on Linux. The flash wear concerns can be mitigated by
deferring writes due to clock drift until shutdown.

Note that this also avoids having to wait for months for Qualcomm to
provide a free PMIC SDAM scratch register for X1E and future platforms,
and specifically allows us to enable the RTC on X1E laptops today.

Rob had some concerns about adding a DT property for indicating that a
machine uses UEFI for storing the offset and suggested that the driver
should probe for this instead. Unfortunately, this is easier said than
done given that UEFI variable support itself is probed for and may not
be available until after the RTC driver probes.

Hopefully this all goes away (for future platforms) once Qualcomm fix
their UEFI implementation so that the UEFI time (and variable) services
can be used directly.

Johan


Changes since UEFI offset RFC [1]:
 - clarify that UEFI variable format is not arbitrary (Alexandre)
 - add missing use_uefi kernel doc
 - use dev_dbg() instead of dev_err() (Alexandre)
 - rename epoch define RTC_TIMESTAMP_EPOCH_GPS (Alexandre)
 - mitigate flash wear by deferring writes due to clock drift until
   shutdown

Changes since Jonathan's X1E series v3 [3]:
 - tweak qcom,no-alarm binding update (and drop Krystzof's Reviewed-by tag)
 - drop no-alarm flag and restructure probe() to clear feature flag before
   registering RTC
 - use UEFI variable offset on X1E

[1] https://lore.kernel.org/lkml/20230126142057.25715-1-johan+linaro@kernel.org/
[2] https://lore.kernel.org/lkml/20230202155448.6715-1-johan+linaro@kernel.org/
[3] https://lore.kernel.org/lkml/20241015004945.3676-1-jonathan@marek.ca/


Johan Hovold (5):
  dt-bindings: rtc: qcom-pm8xxx: add uefi-variable offset
  rtc: pm8xxx: add support for uefi offset
  rtc: pm8xxx: mitigate flash wear
  arm64: dts: qcom: sc8280xp-x13s: switch to uefi rtc offset
  arm64: dts: qcom: x1e80100: enable rtc

Jonathan Marek (2):
  dt-bindings: rtc: qcom-pm8xxx: document qcom,no-alarm flag
  rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm

 .../bindings/rtc/qcom-pm8xxx-rtc.yaml         |  11 +
 .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts    |  11 +-
 arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi  |   4 +-
 drivers/rtc/rtc-pm8xxx.c                      | 194 +++++++++++++++---
 include/linux/rtc.h                           |   1 +
 5 files changed, 185 insertions(+), 36 deletions(-)

-- 
2.45.2

[PATCH 1/7] dt-bindings: rtc: qcom-pm8xxx: add uefi-variable offset

[Johan Hovold]

On many Qualcomm platforms the PMIC RTC control and time registers are
read-only so that the RTC time can not be updated. Instead an offset
needs be stored in some machine-specific non-volatile memory, which a
driver can take into account.

Add a 'qcom,uefi-rtc-info' boolean flag which indicates that the RTC
offset is stored in a Qualcomm specific UEFI variable so that the RTC
time can be updated on such platforms.

The UEFI variable is

	882f8c2b-9646-435f-8de5-f208ff80c1bd-RTCInfo

and holds a 12-byte structure where the first four bytes is a GPS time
offset in little-endian byte order.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
index d274bb7a534b..cdc56dfbfac3 100644
--- a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
@@ -50,6 +50,12 @@ properties:
     items:
       - const: offset
 
+  qcom,uefi-rtc-info:
+    type: boolean
+    description:
+      RTC offset is stored as a four-byte GPS time offset in a 12-byte UEFI
+      variable 882f8c2b-9646-435f-8de5-f208ff80c1bd-RTCInfo
+
   wakeup-source: true
 
 required:
-- 
2.45.2

[PATCH 2/7] dt-bindings: rtc: qcom-pm8xxx: document qcom,no-alarm flag

[Johan Hovold]

From: Jonathan Marek <jonathan@marek.ca>

Qualcomm x1e80100 firmware sets the ownership of the RTC alarm to ADSP.
Thus writing to RTC alarm registers and receiving alarm interrupts is not
possible.

Add a qcom,no-alarm flag to support RTC on this platform.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
Link: https://lore.kernel.org/r/20241015004945.3676-3-jonathan@marek.ca
[ johan: move vendor property; use boolean; reword description ]
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
index cdc56dfbfac3..7497dc3ac5b2 100644
--- a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
@@ -50,6 +50,11 @@ properties:
     items:
       - const: offset
 
+  qcom,no-alarm:
+    type: boolean
+    description:
+      RTC alarm is not owned by the OS
+
   qcom,uefi-rtc-info:
     type: boolean
     description:
-- 
2.45.2

[PATCH 3/7] rtc: pm8xxx: add support for uefi offset

[Johan Hovold]

On many Qualcomm platforms the PMIC RTC control and time registers are
read-only so that the RTC time can not be updated. Instead an offset
needs be stored in some machine-specific non-volatile memory, which the
driver can take into account.

Add support for storing a 32-bit offset from the GPS time epoch in a
UEFI variable so that the RTC time can be set on such platforms.

The UEFI variable is

            882f8c2b-9646-435f-8de5-f208ff80c1bd-RTCInfo

and holds a 12-byte structure where the first four bytes is a GPS time
offset in little-endian byte order.

Note that this format is not arbitrary as the variable is shared with
the UEFI firmware (and Windows).

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/rtc/rtc-pm8xxx.c | 125 +++++++++++++++++++++++++++++++++++++--
 include/linux/rtc.h      |   1 +
 2 files changed, 121 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 2f32187ecc8d..be1983bf35cd 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
  * Copyright (c) 2023, Linaro Limited
  */
+#include <linux/efi.h>
 #include <linux/of.h>
 #include <linux/module.h>
 #include <linux/nvmem-consumer.h>
@@ -16,9 +17,10 @@
 #include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
-
 #include <linux/unaligned.h>
 
+#include <asm/byteorder.h>
+
 /* RTC_CTRL register bit fields */
 #define PM8xxx_RTC_ENABLE		BIT(7)
 #define PM8xxx_RTC_ALARM_CLEAR		BIT(0)
@@ -46,14 +48,21 @@ struct pm8xxx_rtc_regs {
 	unsigned int alarm_en;
 };
 
+struct qcom_uefi_rtc_info {
+	__le32	offset_gps;
+	u8	reserved[8];
+} __packed;
+
 /**
  * struct pm8xxx_rtc -  RTC driver internal structure
  * @rtc:		RTC device
  * @regmap:		regmap used to access registers
  * @allow_set_time:	whether the time can be set
+ * @use_uefi:		use UEFI variable as fallback for offset
  * @alarm_irq:		alarm irq number
  * @regs:		register description
  * @dev:		device structure
+ * @rtc_info:		qcom uefi rtc-info structure
  * @nvmem_cell:		nvmem cell for offset
  * @offset:		offset from epoch in seconds
  */
@@ -61,13 +70,101 @@ struct pm8xxx_rtc {
 	struct rtc_device *rtc;
 	struct regmap *regmap;
 	bool allow_set_time;
+	bool use_uefi;
 	int alarm_irq;
 	const struct pm8xxx_rtc_regs *regs;
 	struct device *dev;
+	struct qcom_uefi_rtc_info rtc_info;
 	struct nvmem_cell *nvmem_cell;
 	u32 offset;
 };
 
+#ifdef CONFIG_EFI
+
+MODULE_IMPORT_NS("EFIVAR");
+
+#define QCOM_UEFI_NAME	L"RTCInfo"
+#define QCOM_UEFI_GUID	EFI_GUID(0x882f8c2b, 0x9646, 0x435f, \
+				 0x8d, 0xe5, 0xf2, 0x08, 0xff, 0x80, 0xc1, 0xbd)
+#define QCOM_UEFI_ATTRS	(EFI_VARIABLE_NON_VOLATILE | \
+			 EFI_VARIABLE_BOOTSERVICE_ACCESS | \
+			 EFI_VARIABLE_RUNTIME_ACCESS)
+
+static int pm8xxx_rtc_read_uefi_offset(struct pm8xxx_rtc *rtc_dd)
+{
+	struct qcom_uefi_rtc_info *rtc_info = &rtc_dd->rtc_info;
+	unsigned long size = sizeof(*rtc_info);
+	struct device *dev = rtc_dd->dev;
+	efi_status_t status;
+	u32 offset_gps;
+	int rc;
+
+	rc = efivar_lock();
+	if (rc)
+		return rc;
+
+	status = efivar_get_variable(QCOM_UEFI_NAME, &QCOM_UEFI_GUID, NULL,
+				     &size, rtc_info);
+	efivar_unlock();
+
+	if (status != EFI_SUCCESS) {
+		dev_dbg(dev, "failed to read UEFI offset: %lu\n", status);
+		return efi_status_to_err(status);
+	}
+
+	if (size != sizeof(*rtc_info)) {
+		dev_dbg(dev, "unexpected UEFI structure size %lu\n", size);
+		return -EINVAL;
+	}
+
+	dev_dbg(dev, "uefi_rtc_info = %*ph\n", (int)size, rtc_info);
+
+	/* Convert from GPS to Unix time offset */
+	offset_gps = le32_to_cpu(rtc_info->offset_gps);
+	rtc_dd->offset = offset_gps + (u32)RTC_TIMESTAMP_EPOCH_GPS;
+
+	return 0;
+}
+
+static int pm8xxx_rtc_write_uefi_offset(struct pm8xxx_rtc *rtc_dd, u32 offset)
+{
+	struct qcom_uefi_rtc_info *rtc_info = &rtc_dd->rtc_info;
+	unsigned long size = sizeof(*rtc_info);
+	struct device *dev = rtc_dd->dev;
+	efi_status_t status;
+	u32 offset_gps;
+
+	/* Convert from Unix to GPS time offset */
+	offset_gps = offset - (u32)RTC_TIMESTAMP_EPOCH_GPS;
+
+	rtc_info->offset_gps = cpu_to_le32(offset_gps);
+
+	dev_dbg(dev, "efi_rtc_info = %*ph\n", (int)size, rtc_info);
+
+	status = efivar_set_variable(QCOM_UEFI_NAME, &QCOM_UEFI_GUID,
+				     QCOM_UEFI_ATTRS, size, rtc_info);
+	if (status != EFI_SUCCESS) {
+		dev_dbg(dev, "failed to write UEFI offset: %lx\n", status);
+		return efi_status_to_err(status);
+	}
+
+	return 0;
+}
+
+#else	/* CONFIG_EFI */
+
+static int pm8xxx_rtc_read_uefi_offset(struct pm8xxx_rtc *rtc_dd)
+{
+	return -ENODEV;
+}
+
+static int pm8xxx_rtc_write_uefi_offset(struct pm8xxx_rtc *rtc_dd, u32 offset)
+{
+	return -ENODEV;
+}
+
+#endif	/* CONFIG_EFI */
+
 static int pm8xxx_rtc_read_nvmem_offset(struct pm8xxx_rtc *rtc_dd)
 {
 	size_t len;
@@ -112,10 +209,13 @@ static int pm8xxx_rtc_write_nvmem_offset(struct pm8xxx_rtc *rtc_dd, u32 offset)
 
 static int pm8xxx_rtc_read_offset(struct pm8xxx_rtc *rtc_dd)
 {
-	if (!rtc_dd->nvmem_cell)
+	if (!rtc_dd->nvmem_cell && !rtc_dd->use_uefi)
 		return 0;
 
-	return pm8xxx_rtc_read_nvmem_offset(rtc_dd);
+	if (rtc_dd->nvmem_cell)
+		return pm8xxx_rtc_read_nvmem_offset(rtc_dd);
+	else
+		return pm8xxx_rtc_read_uefi_offset(rtc_dd);
 }
 
 static int pm8xxx_rtc_read_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs)
@@ -155,7 +255,7 @@ static int pm8xxx_rtc_update_offset(struct pm8xxx_rtc *rtc_dd, u32 secs)
 	u32 offset;
 	int rc;
 
-	if (!rtc_dd->nvmem_cell)
+	if (!rtc_dd->nvmem_cell && !rtc_dd->use_uefi)
 		return -ENODEV;
 
 	rc = pm8xxx_rtc_read_raw(rtc_dd, &raw_secs);
@@ -167,7 +267,11 @@ static int pm8xxx_rtc_update_offset(struct pm8xxx_rtc *rtc_dd, u32 secs)
 	if (offset == rtc_dd->offset)
 		return 0;
 
-	rc = pm8xxx_rtc_write_nvmem_offset(rtc_dd, offset);
+	if (rtc_dd->nvmem_cell)
+		rc = pm8xxx_rtc_write_nvmem_offset(rtc_dd, offset);
+	else
+		rc = pm8xxx_rtc_write_uefi_offset(rtc_dd, offset);
+
 	if (rc)
 		return rc;
 
@@ -486,6 +590,17 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 		if (rc != -ENOENT)
 			return rc;
 		rtc_dd->nvmem_cell = NULL;
+
+		/* Use UEFI storage as fallback if available */
+		rtc_dd->use_uefi = of_property_read_bool(pdev->dev.of_node,
+							 "qcom,uefi-rtc-info");
+	}
+
+	if (rtc_dd->use_uefi && !efivar_is_available()) {
+		if (IS_ENABLED(CONFIG_EFI))
+			return -EPROBE_DEFER;
+		dev_warn(&pdev->dev, "efivars not available\n");
+		rtc_dd->use_uefi = false;
 	}
 
 	rtc_dd->regs = match->data;
diff --git a/include/linux/rtc.h b/include/linux/rtc.h
index 3f4d315aaec9..95da051fb155 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -170,6 +170,7 @@ struct rtc_device {
 /* useful timestamps */
 #define RTC_TIMESTAMP_BEGIN_0000	-62167219200ULL /* 0000-01-01 00:00:00 */
 #define RTC_TIMESTAMP_BEGIN_1900	-2208988800LL /* 1900-01-01 00:00:00 */
+#define RTC_TIMESTAMP_EPOCH_GPS		315964800LL /* 1980-01-06 00:00:00 */
 #define RTC_TIMESTAMP_BEGIN_2000	946684800LL /* 2000-01-01 00:00:00 */
 #define RTC_TIMESTAMP_END_2063		2966371199LL /* 2063-12-31 23:59:59 */
 #define RTC_TIMESTAMP_END_2079		3471292799LL /* 2079-12-31 23:59:59 */
-- 
2.45.2

[PATCH 4/7] rtc: pm8xxx: mitigate flash wear

[Johan Hovold]

On many Qualcomm platforms the PMIC RTC control and time registers are
read-only so that the RTC time can not be updated. Instead an offset
needs be stored in some machine-specific non-volatile memory, which the
driver can take into account.

On machines like the Lenovo ThinkPad X14s the PMIC RTC drifts about one
second every 3.5 hours, something which leads to repeated updates of the
offset when NTP synchronisation is enabled.

Reduce wear of the underlying flash storage (used for UEFI variables) by
deferring writes until shutdown in case they appear to be due to clock
drift.

As an example, allowing the on-flash offset to differ up to 30 s reduces
the number of writes on the X13s during a ten day session with the
machine not suspending for more than four days in a row from up to 68
writes (every 3.5 h) to at most two (boot and shutdown).

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/rtc/rtc-pm8xxx.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index be1983bf35cd..049daa39c417 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -65,6 +65,7 @@ struct qcom_uefi_rtc_info {
  * @rtc_info:		qcom uefi rtc-info structure
  * @nvmem_cell:		nvmem cell for offset
  * @offset:		offset from epoch in seconds
+ * @offset_dirty:	offset needs to be stored on shutdown
  */
 struct pm8xxx_rtc {
 	struct rtc_device *rtc;
@@ -77,6 +78,7 @@ struct pm8xxx_rtc {
 	struct qcom_uefi_rtc_info rtc_info;
 	struct nvmem_cell *nvmem_cell;
 	u32 offset;
+	bool offset_dirty;
 };
 
 #ifdef CONFIG_EFI
@@ -267,6 +269,15 @@ static int pm8xxx_rtc_update_offset(struct pm8xxx_rtc *rtc_dd, u32 secs)
 	if (offset == rtc_dd->offset)
 		return 0;
 
+	/*
+	 * Reduce flash wear by deferring updates due to clock drift until
+	 * shutdown.
+	 */
+	if (abs_diff(offset, rtc_dd->offset) < 30) {
+		rtc_dd->offset_dirty = true;
+		goto out;
+	}
+
 	if (rtc_dd->nvmem_cell)
 		rc = pm8xxx_rtc_write_nvmem_offset(rtc_dd, offset);
 	else
@@ -275,6 +286,8 @@ static int pm8xxx_rtc_update_offset(struct pm8xxx_rtc *rtc_dd, u32 secs)
 	if (rc)
 		return rc;
 
+	rtc_dd->offset_dirty = false;
+out:
 	rtc_dd->offset = offset;
 
 	return 0;
@@ -650,9 +663,22 @@ static void pm8xxx_remove(struct platform_device *pdev)
 	dev_pm_clear_wake_irq(&pdev->dev);
 }
 
+static void pm8xxx_shutdown(struct platform_device *pdev)
+{
+	struct pm8xxx_rtc *rtc_dd = platform_get_drvdata(pdev);
+
+	if (rtc_dd->offset_dirty) {
+		if (rtc_dd->nvmem_cell)
+			pm8xxx_rtc_write_nvmem_offset(rtc_dd, rtc_dd->offset);
+		else
+			pm8xxx_rtc_write_uefi_offset(rtc_dd, rtc_dd->offset);
+	}
+}
+
 static struct platform_driver pm8xxx_rtc_driver = {
 	.probe		= pm8xxx_rtc_probe,
 	.remove		= pm8xxx_remove,
+	.shutdown	= pm8xxx_shutdown,
 	.driver	= {
 		.name		= "rtc-pm8xxx",
 		.of_match_table	= pm8xxx_id_table,
-- 
2.45.2

[PATCH 5/7] rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm

[Johan Hovold]

From: Jonathan Marek <jonathan@marek.ca>

Qualcomm x1e80100 firmware sets the ownership of the RTC alarm to ADSP.
Thus writing to RTC alarm registers and receiving alarm interrupts is not
possible.

Add a qcom,no-alarm flag to support RTC on this platform.

Signed-off-by: Jonathan Marek <jonathan@marek.ca>
Link: https://lore.kernel.org/r/20241015004945.3676-2-jonathan@marek.ca
[ johan: drop no_alarm flag and restructure probe() ]
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/rtc/rtc-pm8xxx.c | 43 ++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 049daa39c417..f9755fb9eaa5 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -590,9 +590,11 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 	if (!rtc_dd->regmap)
 		return -ENXIO;
 
-	rtc_dd->alarm_irq = platform_get_irq(pdev, 0);
-	if (rtc_dd->alarm_irq < 0)
-		return -ENXIO;
+	if (!of_property_read_bool(pdev->dev.of_node, "qcom,no-alarm")) {
+		rtc_dd->alarm_irq = platform_get_irq(pdev, 0);
+		if (rtc_dd->alarm_irq < 0)
+			return -ENXIO;
+	}
 
 	rtc_dd->allow_set_time = of_property_read_bool(pdev->dev.of_node,
 						      "allow-set-time");
@@ -631,8 +633,6 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, rtc_dd);
 
-	device_init_wakeup(&pdev->dev, 1);
-
 	rtc_dd->rtc = devm_rtc_allocate_device(&pdev->dev);
 	if (IS_ERR(rtc_dd->rtc))
 		return PTR_ERR(rtc_dd->rtc);
@@ -640,27 +640,32 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 	rtc_dd->rtc->ops = &pm8xxx_rtc_ops;
 	rtc_dd->rtc->range_max = U32_MAX;
 
-	rc = devm_request_any_context_irq(&pdev->dev, rtc_dd->alarm_irq,
-					  pm8xxx_alarm_trigger,
-					  IRQF_TRIGGER_RISING,
-					  "pm8xxx_rtc_alarm", rtc_dd);
-	if (rc < 0)
-		return rc;
+	if (rtc_dd->alarm_irq) {
+		rc = devm_request_any_context_irq(&pdev->dev, rtc_dd->alarm_irq,
+						  pm8xxx_alarm_trigger,
+						  IRQF_TRIGGER_RISING,
+						  "pm8xxx_rtc_alarm", rtc_dd);
+		if (rc < 0)
+			return rc;
 
-	rc = devm_rtc_register_device(rtc_dd->rtc);
-	if (rc)
-		return rc;
+		rc = dev_pm_set_wake_irq(&pdev->dev, rtc_dd->alarm_irq);
+		if (rc)
+			return rc;
 
-	rc = dev_pm_set_wake_irq(&pdev->dev, rtc_dd->alarm_irq);
-	if (rc)
-		return rc;
+		device_init_wakeup(&pdev->dev, 1);
+	} else {
+		clear_bit(RTC_FEATURE_ALARM, rtc_dd->rtc->features);
+	}
 
-	return 0;
+	return devm_rtc_register_device(rtc_dd->rtc);
 }
 
 static void pm8xxx_remove(struct platform_device *pdev)
 {
-	dev_pm_clear_wake_irq(&pdev->dev);
+	struct pm8xxx_rtc *rtc_dd = platform_get_drvdata(pdev);
+
+	if (rtc_dd->alarm_irq)
+		dev_pm_clear_wake_irq(&pdev->dev);
 }
 
 static void pm8xxx_shutdown(struct platform_device *pdev)
-- 
2.45.2

[PATCH 6/7] arm64: dts: qcom: sc8280xp-x13s: switch to uefi rtc offset

[Johan Hovold]

On many Qualcomm platforms the PMIC RTC control and time registers are
read-only so that the RTC time can not be updated. Instead an offset
needs be stored in some machine-specific non-volatile memory, which a
driver can take into account.

Switch to using the Qualcomm specific UEFI variable that is used by the
UEFI firmware (and Windows) to store the RTC offset.

This specifically means that the RTC time will be synchronised between
the UEFI firmware setup (or UEFI shell), Windows and Linux.

Note however that Windows stores the RTC time in local time by default,
while Linux typically uses UTC (i.e. as on X86).

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts   | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
index 38d911992475..66c39765225f 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -1090,20 +1090,11 @@ &pmk8280_pon_resin {
 };
 
 &pmk8280_rtc {
-	nvmem-cells = <&rtc_offset>;
-	nvmem-cell-names = "offset";
+	qcom,uefi-rtc-info;
 
 	status = "okay";
 };
 
-&pmk8280_sdam_6 {
-	status = "okay";
-
-	rtc_offset: rtc-offset@bc {
-		reg = <0xbc 0x4>;
-	};
-};
-
 &pmk8280_vadc {
 	channel@144 {
 		reg = <PM8350_ADC7_AMUX_THM1_100K_PU(1)>;
-- 
2.45.2

[PATCH 7/7] arm64: dts: qcom: x1e80100: enable rtc

[Johan Hovold]

On many Qualcomm platforms the PMIC RTC control and time registers are
read-only so that the RTC time can not be updated. Instead an offset
needs be stored in some machine-specific non-volatile memory, which a
driver can take into account.

On X1E based Windows on Arm machines the offset is stored in a Qualcomm
specific UEFI variable.

Unlike on previous platforms the alarm registers are also unaccessible
on X1E as they are owned by the ADSP.

Assume all X1E machines use similar firmware and enable the RTC in the
PMIC dtsi for now.

Based on a patch by Jonathan Marek. [1]

Link: https://lore.kernel.org/r/20241015004945.3676-4-jonathan@marek.ca # [1]
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi b/arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi
index 5b54ee79f048..051fb3a304b9 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi
+++ b/arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi
@@ -223,8 +223,8 @@ pmk8550_rtc: rtc@6100 {
 			reg = <0x6100>, <0x6200>;
 			reg-names = "rtc", "alarm";
 			interrupts = <0x0 0x62 0x1 IRQ_TYPE_EDGE_RISING>;
-			/* Not yet sure what blocks access */
-			status = "reserved";
+			qcom,no-alarm; /* alarm owned by ADSP */
+			qcom,uefi-rtc-info;
 		};
 
 		pmk8550_sdam_2: nvram@7100 {
-- 
2.45.2

Re: [PATCH 3/7] rtc: pm8xxx: add support for uefi offset

[Sudeep Holla]

On Mon, Jan 20, 2025 at 03:41:48PM +0100, Johan Hovold wrote:
> On many Qualcomm platforms the PMIC RTC control and time registers are
> read-only so that the RTC time can not be updated. Instead an offset
> needs be stored in some machine-specific non-volatile memory, which the
> driver can take into account.
> 
> Add support for storing a 32-bit offset from the GPS time epoch in a
> UEFI variable so that the RTC time can be set on such platforms.
>

Why can't the UEFI runtime services be used here ?
In short, why is drivers/rtc/rtc-efi.c not working or no attempts are
made to check if that works just fine ?

-- 
Regards,
Sudeep

Re: [PATCH 3/7] rtc: pm8xxx: add support for uefi offset

[Johan Hovold]

On Mon, Jan 20, 2025 at 03:10:24PM +0000, Sudeep Holla wrote:
> On Mon, Jan 20, 2025 at 03:41:48PM +0100, Johan Hovold wrote:
> > On many Qualcomm platforms the PMIC RTC control and time registers are
> > read-only so that the RTC time can not be updated. Instead an offset
> > needs be stored in some machine-specific non-volatile memory, which the
> > driver can take into account.
> > 
> > Add support for storing a 32-bit offset from the GPS time epoch in a
> > UEFI variable so that the RTC time can be set on such platforms.
> >
> 
> Why can't the UEFI runtime services be used here ?
> In short, why is drivers/rtc/rtc-efi.c not working or no attempts are
> made to check if that works just fine ?

As I alluded to in the commit message, the Qualcomm UEFI implementation
is broken and the runtime services cannot be accessed during runtime...

That's why we can't use rtc-efi and why we have a dedicated efivars
implementation that talks directly to the firmware using a custom
protocol:

	https://lore.kernel.org/all/20220723224949.1089973-1-luzmaximilian@gmail.com/

Johan

Re: [PATCH 3/7] rtc: pm8xxx: add support for uefi offset

[Johan Hovold]

On Mon, Jan 20, 2025 at 04:15:54PM +0100, Johan Hovold wrote:
> On Mon, Jan 20, 2025 at 03:10:24PM +0000, Sudeep Holla wrote:
> > On Mon, Jan 20, 2025 at 03:41:48PM +0100, Johan Hovold wrote:
> > > On many Qualcomm platforms the PMIC RTC control and time registers are
> > > read-only so that the RTC time can not be updated. Instead an offset
> > > needs be stored in some machine-specific non-volatile memory, which the
> > > driver can take into account.
> > > 
> > > Add support for storing a 32-bit offset from the GPS time epoch in a
> > > UEFI variable so that the RTC time can be set on such platforms.
> > >
> > 
> > Why can't the UEFI runtime services be used here ?
> > In short, why is drivers/rtc/rtc-efi.c not working or no attempts are
> > made to check if that works just fine ?
> 
> As I alluded to in the commit message, the Qualcomm UEFI implementation

s/commit message/cover letter/

> is broken and the runtime services cannot be accessed during runtime...
> 
> That's why we can't use rtc-efi and why we have a dedicated efivars
> implementation that talks directly to the firmware using a custom
> protocol:
> 
> 	https://lore.kernel.org/all/20220723224949.1089973-1-luzmaximilian@gmail.com/

Johan

Re: [PATCH 3/7] rtc: pm8xxx: add support for uefi offset

[Sudeep Holla]

On Mon, Jan 20, 2025 at 04:17:36PM +0100, Johan Hovold wrote:
> On Mon, Jan 20, 2025 at 04:15:54PM +0100, Johan Hovold wrote:
> > On Mon, Jan 20, 2025 at 03:10:24PM +0000, Sudeep Holla wrote:
> > > On Mon, Jan 20, 2025 at 03:41:48PM +0100, Johan Hovold wrote:
> > > > On many Qualcomm platforms the PMIC RTC control and time registers are
> > > > read-only so that the RTC time can not be updated. Instead an offset
> > > > needs be stored in some machine-specific non-volatile memory, which the
> > > > driver can take into account.
> > > > 
> > > > Add support for storing a 32-bit offset from the GPS time epoch in a
> > > > UEFI variable so that the RTC time can be set on such platforms.
> > > >
> > > 
> > > Why can't the UEFI runtime services be used here ?
> > > In short, why is drivers/rtc/rtc-efi.c not working or no attempts are
> > > made to check if that works just fine ?
> > 
> > As I alluded to in the commit message, the Qualcomm UEFI implementation
> 
> s/commit message/cover letter/
>

Ah OK, Sorry for missing that and for you dealing with this mess 😄.
Never ending or very long list of quirks on Linux for the platform I imagine.

--
Regards,
Sudeep

Re: [PATCH 6/7] arm64: dts: qcom: sc8280xp-x13s: switch to uefi rtc offset

[Jens Glathe]

Hi Johan,

On 20.01.25 15:41, Johan Hovold wrote:
> On many Qualcomm platforms the PMIC RTC control and time registers are
> read-only so that the RTC time can not be updated. Instead an offset
> needs be stored in some machine-specific non-volatile memory, which a
> driver can take into account.
>
> Switch to using the Qualcomm specific UEFI variable that is used by the
> UEFI firmware (and Windows) to store the RTC offset.
>
> This specifically means that the RTC time will be synchronised between
> the UEFI firmware setup (or UEFI shell), Windows and Linux.
>
> Note however that Windows stores the RTC time in local time by default,
> while Linux typically uses UTC (i.e. as on X86).
>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   .../boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts   | 11 +----------
>   1 file changed, 1 insertion(+), 10 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> index 38d911992475..66c39765225f 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> @@ -1090,20 +1090,11 @@ &pmk8280_pon_resin {
>   };
>
>   &pmk8280_rtc {
> -	nvmem-cells = <&rtc_offset>;
> -	nvmem-cell-names = "offset";
> +	qcom,uefi-rtc-info;
>
>   	status = "okay";
>   };
>
> -&pmk8280_sdam_6 {
> -	status = "okay";
> -
> -	rtc_offset: rtc-offset@bc {
> -		reg = <0xbc 0x4>;
> -	};
> -};
> -
>   &pmk8280_vadc {
>   	channel@144 {
>   		reg = <PM8350_ADC7_AMUX_THM1_100K_PU(1)>;

Nice. I have it running on my X13s now, looks to work fine. Also ported
to Windows Dev Kit 2023, patch will follow soon after testing it. Thanks!

Tested-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>

with best regards

Jens

Re: [PATCH 7/7] arm64: dts: qcom: x1e80100: enable rtc

[Jens Glathe]

Hi Johan,

On 20.01.25 15:41, Johan Hovold wrote:
> On many Qualcomm platforms the PMIC RTC control and time registers are
> read-only so that the RTC time can not be updated. Instead an offset
> needs be stored in some machine-specific non-volatile memory, which a
> driver can take into account.
>
> On X1E based Windows on Arm machines the offset is stored in a Qualcomm
> specific UEFI variable.
>
> Unlike on previous platforms the alarm registers are also unaccessible
> on X1E as they are owned by the ADSP.
>
> Assume all X1E machines use similar firmware and enable the RTC in the
> PMIC dtsi for now.
>
> Based on a patch by Jonathan Marek. [1]
>
> Link: https://lore.kernel.org/r/20241015004945.3676-4-jonathan@marek.ca # [1]
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi b/arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi
> index 5b54ee79f048..051fb3a304b9 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi
> +++ b/arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi
> @@ -223,8 +223,8 @@ pmk8550_rtc: rtc@6100 {
>   			reg = <0x6100>, <0x6200>;
>   			reg-names = "rtc", "alarm";
>   			interrupts = <0x0 0x62 0x1 IRQ_TYPE_EDGE_RISING>;
> -			/* Not yet sure what blocks access */
> -			status = "reserved";
> +			qcom,no-alarm; /* alarm owned by ADSP */
> +			qcom,uefi-rtc-info;
>   		};
>
>   		pmk8550_sdam_2: nvram@7100 {

works nicely on SnapDragon Dev Kit X1E001DE. Thank you!

Tested-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>

with best regards

Jens Glathe

Re: [PATCH 0/7] arm64: dts: qcom: x1e80100: enable rtc

[Steev Klimaszewski]

Hi Johan,

On Mon, Jan 20, 2025 at 8:43 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> This series adds support for utilising the UEFI firmware RTC offset to
> the Qualcomm PMIC RTC driver and uses that to enable the RTC on all X
> Elite machines.
>
> Included is also a patch to switch the Lenovo ThinkPad X13s over to
> using the UEFI offset.
>
> The RTCs in many Qualcomm devices are effectively broken due to the time
> registers being read-only. Instead some other non-volatile memory can be
> used to store an offset which a driver can take into account. On Windows
> on Arm laptops, the UEFI firmware (and Windows) use a UEFI variable for
> storing such an offset.
>
> When RTC support for the X13s was added two years ago we did not yet
> have UEFI variable support for these machines in mainline and there were
> also some concerns regarding flash wear. [1] As not all Qualcomm
> platforms have UEFI firmware anyway, we instead opted to use a PMIC
> scratch register for storing the offset. [2]
>
> On the UEFI machines in question this is however arguable not correct
> as it means that the RTC time can differ between the UEFI firmware (and
> Windows) and Linux.
>
> Now that the (reverse engineered) UEFI variable implementation has been
> merged and thoroughly tested, let's switch to using that to store the
> RTC offset also on Linux. The flash wear concerns can be mitigated by
> deferring writes due to clock drift until shutdown.
>
> Note that this also avoids having to wait for months for Qualcomm to
> provide a free PMIC SDAM scratch register for X1E and future platforms,
> and specifically allows us to enable the RTC on X1E laptops today.
>
> Rob had some concerns about adding a DT property for indicating that a
> machine uses UEFI for storing the offset and suggested that the driver
> should probe for this instead. Unfortunately, this is easier said than
> done given that UEFI variable support itself is probed for and may not
> be available until after the RTC driver probes.
>
> Hopefully this all goes away (for future platforms) once Qualcomm fix
> their UEFI implementation so that the UEFI time (and variable) services
> can be used directly.
>
> Johan
>
>
> Changes since UEFI offset RFC [1]:
>  - clarify that UEFI variable format is not arbitrary (Alexandre)
>  - add missing use_uefi kernel doc
>  - use dev_dbg() instead of dev_err() (Alexandre)
>  - rename epoch define RTC_TIMESTAMP_EPOCH_GPS (Alexandre)
>  - mitigate flash wear by deferring writes due to clock drift until
>    shutdown
>
> Changes since Jonathan's X1E series v3 [3]:
>  - tweak qcom,no-alarm binding update (and drop Krystzof's Reviewed-by tag)
>  - drop no-alarm flag and restructure probe() to clear feature flag before
>    registering RTC
>  - use UEFI variable offset on X1E
>
> [1] https://lore.kernel.org/lkml/20230126142057.25715-1-johan+linaro@kernel.org/
> [2] https://lore.kernel.org/lkml/20230202155448.6715-1-johan+linaro@kernel.org/
> [3] https://lore.kernel.org/lkml/20241015004945.3676-1-jonathan@marek.ca/
>
>
> Johan Hovold (5):
>   dt-bindings: rtc: qcom-pm8xxx: add uefi-variable offset
>   rtc: pm8xxx: add support for uefi offset
>   rtc: pm8xxx: mitigate flash wear
>   arm64: dts: qcom: sc8280xp-x13s: switch to uefi rtc offset
>   arm64: dts: qcom: x1e80100: enable rtc
>
> Jonathan Marek (2):
>   dt-bindings: rtc: qcom-pm8xxx: document qcom,no-alarm flag
>   rtc: pm8xxx: implement qcom,no-alarm flag for non-HLOS owned alarm
>
>  .../bindings/rtc/qcom-pm8xxx-rtc.yaml         |  11 +
>  .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts    |  11 +-
>  arch/arm64/boot/dts/qcom/x1e80100-pmics.dtsi  |   4 +-
>  drivers/rtc/rtc-pm8xxx.c                      | 194 +++++++++++++++---
>  include/linux/rtc.h                           |   1 +
>  5 files changed, 185 insertions(+), 36 deletions(-)
>
> --
> 2.45.2
>
>

Tested this on the Thinkpad X13s, as well as booting it into Windows
and verifying that it has the correct clock both ways, which it does.
Thank you!

Tested-by: Steev Klimaszewski <steev@kali.org>

Re: [PATCH 0/7] arm64: dts: qcom: x1e80100: enable rtc

[Joel Stanley]

On Tue, 21 Jan 2025 at 01:14, Johan Hovold <johan+linaro@kernel.org> wrote:
>
> This series adds support for utilising the UEFI firmware RTC offset to
> the Qualcomm PMIC RTC driver and uses that to enable the RTC on all X
> Elite machines.

Looks good on a Surface Laptop 7 / romulus:

Tested-by: Joel Stanley <joel@jms.id.au>

[    0.407844] rtc-pm8xxx c42d000.spmi:pmic@0:rtc@6100: registered as rtc0
[    0.407876] rtc-pm8xxx c42d000.spmi:pmic@0:rtc@6100: setting system
clock to 2025-01-21T03:34:06 UTC (1737430446)

Cheers,

Joel

Re: [PATCH 7/7] arm64: dts: qcom: x1e80100: enable rtc

[Johan Hovold]

On Mon, Jan 20, 2025 at 07:12:05PM +0100, Jens Glathe wrote:

> works nicely on SnapDragon Dev Kit X1E001DE. Thank you!
> 
> Tested-by: Jens Glathe <jens.glathe@oldschoolsolutions.biz>

Thanks for testing.

Johan

Re: [PATCH 0/7] arm64: dts: qcom: x1e80100: enable rtc

[Johan Hovold]

On Mon, Jan 20, 2025 at 03:19:33PM -0600, Steev Klimaszewski wrote:

> Tested this on the Thinkpad X13s, as well as booting it into Windows
> and verifying that it has the correct clock both ways, which it does.
> Thank you!
> 
> Tested-by: Steev Klimaszewski <steev@kali.org>

Thanks for testing.

Johan

Re: [PATCH 0/7] arm64: dts: qcom: x1e80100: enable rtc

[Johan Hovold]

On Tue, Jan 21, 2025 at 02:18:21PM +1030, Joel Stanley wrote:
> On Tue, 21 Jan 2025 at 01:14, Johan Hovold <johan+linaro@kernel.org> wrote:
> >
> > This series adds support for utilising the UEFI firmware RTC offset to
> > the Qualcomm PMIC RTC driver and uses that to enable the RTC on all X
> > Elite machines.
> 
> Looks good on a Surface Laptop 7 / romulus:
> 
> Tested-by: Joel Stanley <joel@jms.id.au>
> 
> [    0.407844] rtc-pm8xxx c42d000.spmi:pmic@0:rtc@6100: registered as rtc0
> [    0.407876] rtc-pm8xxx c42d000.spmi:pmic@0:rtc@6100: setting system
> clock to 2025-01-21T03:34:06 UTC (1737430446)

Thanks for testing.

Johan

Re: [PATCH 4/7] rtc: pm8xxx: mitigate flash wear

[Johan Hovold]

On Mon, Jan 20, 2025 at 03:41:49PM +0100, Johan Hovold wrote:
> On many Qualcomm platforms the PMIC RTC control and time registers are
> read-only so that the RTC time can not be updated. Instead an offset
> needs be stored in some machine-specific non-volatile memory, which the
> driver can take into account.
> 
> On machines like the Lenovo ThinkPad X14s the PMIC RTC drifts about one

s/X14s/X13s/

> second every 3.5 hours, something which leads to repeated updates of the
> offset when NTP synchronisation is enabled.
> 
> Reduce wear of the underlying flash storage (used for UEFI variables) by
> deferring writes until shutdown in case they appear to be due to clock
> drift.
> 
> As an example, allowing the on-flash offset to differ up to 30 s reduces

And this was supposed to say:

	As an example, deferring writes when the new offset differs up
	to 30 s from the previous one reduces
	
> the number of writes on the X13s during a ten day session with the
> machine not suspending for more than four days in a row from up to 68
> writes (every 3.5 h) to at most two (boot and shutdown).

Johan

Re: [PATCH 7/7] arm64: dts: qcom: x1e80100: enable rtc

[Konrad Dybcio]

On 20.01.2025 3:41 PM, Johan Hovold wrote:
> On many Qualcomm platforms the PMIC RTC control and time registers are
> read-only so that the RTC time can not be updated. Instead an offset
> needs be stored in some machine-specific non-volatile memory, which a
> driver can take into account.
> 
> On X1E based Windows on Arm machines the offset is stored in a Qualcomm
> specific UEFI variable.
> 
> Unlike on previous platforms the alarm registers are also unaccessible
> on X1E as they are owned by the ADSP.
> 
> Assume all X1E machines use similar firmware and enable the RTC in the
> PMIC dtsi for now.
> 
> Based on a patch by Jonathan Marek. [1]
> 
> Link: https://lore.kernel.org/r/20241015004945.3676-4-jonathan@marek.ca # [1]
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH 6/7] arm64: dts: qcom: sc8280xp-x13s: switch to uefi rtc offset

[Konrad Dybcio]

On 20.01.2025 3:41 PM, Johan Hovold wrote:
> On many Qualcomm platforms the PMIC RTC control and time registers are
> read-only so that the RTC time can not be updated. Instead an offset
> needs be stored in some machine-specific non-volatile memory, which a
> driver can take into account.
> 
> Switch to using the Qualcomm specific UEFI variable that is used by the
> UEFI firmware (and Windows) to store the RTC offset.
> 
> This specifically means that the RTC time will be synchronised between
> the UEFI firmware setup (or UEFI shell), Windows and Linux.
> 
> Note however that Windows stores the RTC time in local time by default,
> while Linux typically uses UTC (i.e. as on X86).
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH 0/7] arm64: dts: qcom: x1e80100: enable rtc

[Konrad Dybcio]

On 20.01.2025 3:41 PM, Johan Hovold wrote:
> This series adds support for utilising the UEFI firmware RTC offset to
> the Qualcomm PMIC RTC driver and uses that to enable the RTC on all X
> Elite machines.
> 
> Included is also a patch to switch the Lenovo ThinkPad X13s over to
> using the UEFI offset.
> 
> The RTCs in many Qualcomm devices are effectively broken due to the time
> registers being read-only. Instead some other non-volatile memory can be
> used to store an offset which a driver can take into account. On Windows
> on Arm laptops, the UEFI firmware (and Windows) use a UEFI variable for
> storing such an offset.
> 
> When RTC support for the X13s was added two years ago we did not yet
> have UEFI variable support for these machines in mainline and there were
> also some concerns regarding flash wear. [1] As not all Qualcomm
> platforms have UEFI firmware anyway, we instead opted to use a PMIC
> scratch register for storing the offset. [2]
> 
> On the UEFI machines in question this is however arguable not correct
> as it means that the RTC time can differ between the UEFI firmware (and
> Windows) and Linux.
> 
> Now that the (reverse engineered) UEFI variable implementation has been
> merged and thoroughly tested, let's switch to using that to store the
> RTC offset also on Linux. The flash wear concerns can be mitigated by
> deferring writes due to clock drift until shutdown.
> 
> Note that this also avoids having to wait for months for Qualcomm to
> provide a free PMIC SDAM scratch register for X1E and future platforms,
> and specifically allows us to enable the RTC on X1E laptops today.
> 
> Rob had some concerns about adding a DT property for indicating that a
> machine uses UEFI for storing the offset and suggested that the driver
> should probe for this instead. Unfortunately, this is easier said than
> done given that UEFI variable support itself is probed for and may not
> be available until after the RTC driver probes.
> 
> Hopefully this all goes away (for future platforms) once Qualcomm fix
> their UEFI implementation so that the UEFI time (and variable) services
> can be used directly.
> 
> Johan

Thanks for working on this!

Konrad

Re: [PATCH 3/7] rtc: pm8xxx: add support for uefi offset

[Tobias Heider]

On Mon, Jan 20, 2025 at 03:41:48PM GMT, Johan Hovold wrote:
> On many Qualcomm platforms the PMIC RTC control and time registers are
> read-only so that the RTC time can not be updated. Instead an offset
> needs be stored in some machine-specific non-volatile memory, which the
> driver can take into account.
> 
> Add support for storing a 32-bit offset from the GPS time epoch in a
> UEFI variable so that the RTC time can be set on such platforms.
> 
> The UEFI variable is
> 
>             882f8c2b-9646-435f-8de5-f208ff80c1bd-RTCInfo
> 
> and holds a 12-byte structure where the first four bytes is a GPS time
> offset in little-endian byte order.
> 
> Note that this format is not arbitrary as the variable is shared with
> the UEFI firmware (and Windows).
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Thanks for the great work Johan, seems to work nicely here!

I think you might need to add MODULE_IMPORT_NS(EFIVAR) for the efivar_*
API calls you added.

> ---
>  drivers/rtc/rtc-pm8xxx.c | 125 +++++++++++++++++++++++++++++++++++++--
>  include/linux/rtc.h      |   1 +
>  2 files changed, 121 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> index 2f32187ecc8d..be1983bf35cd 100644
> --- a/drivers/rtc/rtc-pm8xxx.c
> +++ b/drivers/rtc/rtc-pm8xxx.c
> @@ -5,6 +5,7 @@
>   * Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
>   * Copyright (c) 2023, Linaro Limited
>   */
> +#include <linux/efi.h>
>  #include <linux/of.h>
>  #include <linux/module.h>
>  #include <linux/nvmem-consumer.h>
> @@ -16,9 +17,10 @@
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> -
>  #include <linux/unaligned.h>
>  
> +#include <asm/byteorder.h>
> +
>  /* RTC_CTRL register bit fields */
>  #define PM8xxx_RTC_ENABLE		BIT(7)
>  #define PM8xxx_RTC_ALARM_CLEAR		BIT(0)
> @@ -46,14 +48,21 @@ struct pm8xxx_rtc_regs {
>  	unsigned int alarm_en;
>  };
>  
> +struct qcom_uefi_rtc_info {
> +	__le32	offset_gps;
> +	u8	reserved[8];
> +} __packed;
> +
>  /**
>   * struct pm8xxx_rtc -  RTC driver internal structure
>   * @rtc:		RTC device
>   * @regmap:		regmap used to access registers
>   * @allow_set_time:	whether the time can be set
> + * @use_uefi:		use UEFI variable as fallback for offset
>   * @alarm_irq:		alarm irq number
>   * @regs:		register description
>   * @dev:		device structure
> + * @rtc_info:		qcom uefi rtc-info structure
>   * @nvmem_cell:		nvmem cell for offset
>   * @offset:		offset from epoch in seconds
>   */
> @@ -61,13 +70,101 @@ struct pm8xxx_rtc {
>  	struct rtc_device *rtc;
>  	struct regmap *regmap;
>  	bool allow_set_time;
> +	bool use_uefi;
>  	int alarm_irq;
>  	const struct pm8xxx_rtc_regs *regs;
>  	struct device *dev;
> +	struct qcom_uefi_rtc_info rtc_info;
>  	struct nvmem_cell *nvmem_cell;
>  	u32 offset;
>  };
>  
> +#ifdef CONFIG_EFI
> +
> +MODULE_IMPORT_NS("EFIVAR");
> +
> +#define QCOM_UEFI_NAME	L"RTCInfo"
> +#define QCOM_UEFI_GUID	EFI_GUID(0x882f8c2b, 0x9646, 0x435f, \
> +				 0x8d, 0xe5, 0xf2, 0x08, 0xff, 0x80, 0xc1, 0xbd)
> +#define QCOM_UEFI_ATTRS	(EFI_VARIABLE_NON_VOLATILE | \
> +			 EFI_VARIABLE_BOOTSERVICE_ACCESS | \
> +			 EFI_VARIABLE_RUNTIME_ACCESS)
> +
> +static int pm8xxx_rtc_read_uefi_offset(struct pm8xxx_rtc *rtc_dd)
> +{
> +	struct qcom_uefi_rtc_info *rtc_info = &rtc_dd->rtc_info;
> +	unsigned long size = sizeof(*rtc_info);
> +	struct device *dev = rtc_dd->dev;
> +	efi_status_t status;
> +	u32 offset_gps;
> +	int rc;
> +
> +	rc = efivar_lock();
> +	if (rc)
> +		return rc;
> +
> +	status = efivar_get_variable(QCOM_UEFI_NAME, &QCOM_UEFI_GUID, NULL,
> +				     &size, rtc_info);
> +	efivar_unlock();
> +
> +	if (status != EFI_SUCCESS) {
> +		dev_dbg(dev, "failed to read UEFI offset: %lu\n", status);
> +		return efi_status_to_err(status);
> +	}
> +
> +	if (size != sizeof(*rtc_info)) {
> +		dev_dbg(dev, "unexpected UEFI structure size %lu\n", size);
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(dev, "uefi_rtc_info = %*ph\n", (int)size, rtc_info);
> +
> +	/* Convert from GPS to Unix time offset */
> +	offset_gps = le32_to_cpu(rtc_info->offset_gps);
> +	rtc_dd->offset = offset_gps + (u32)RTC_TIMESTAMP_EPOCH_GPS;
> +
> +	return 0;
> +}
> +
> +static int pm8xxx_rtc_write_uefi_offset(struct pm8xxx_rtc *rtc_dd, u32 offset)
> +{
> +	struct qcom_uefi_rtc_info *rtc_info = &rtc_dd->rtc_info;
> +	unsigned long size = sizeof(*rtc_info);
> +	struct device *dev = rtc_dd->dev;
> +	efi_status_t status;
> +	u32 offset_gps;
> +
> +	/* Convert from Unix to GPS time offset */
> +	offset_gps = offset - (u32)RTC_TIMESTAMP_EPOCH_GPS;
> +
> +	rtc_info->offset_gps = cpu_to_le32(offset_gps);
> +
> +	dev_dbg(dev, "efi_rtc_info = %*ph\n", (int)size, rtc_info);
> +
> +	status = efivar_set_variable(QCOM_UEFI_NAME, &QCOM_UEFI_GUID,
> +				     QCOM_UEFI_ATTRS, size, rtc_info);
> +	if (status != EFI_SUCCESS) {
> +		dev_dbg(dev, "failed to write UEFI offset: %lx\n", status);
> +		return efi_status_to_err(status);
> +	}
> +
> +	return 0;
> +}
> +
> +#else	/* CONFIG_EFI */
> +
> +static int pm8xxx_rtc_read_uefi_offset(struct pm8xxx_rtc *rtc_dd)
> +{
> +	return -ENODEV;
> +}
> +
> +static int pm8xxx_rtc_write_uefi_offset(struct pm8xxx_rtc *rtc_dd, u32 offset)
> +{
> +	return -ENODEV;
> +}
> +
> +#endif	/* CONFIG_EFI */
> +
>  static int pm8xxx_rtc_read_nvmem_offset(struct pm8xxx_rtc *rtc_dd)
>  {
>  	size_t len;
> @@ -112,10 +209,13 @@ static int pm8xxx_rtc_write_nvmem_offset(struct pm8xxx_rtc *rtc_dd, u32 offset)
>  
>  static int pm8xxx_rtc_read_offset(struct pm8xxx_rtc *rtc_dd)
>  {
> -	if (!rtc_dd->nvmem_cell)
> +	if (!rtc_dd->nvmem_cell && !rtc_dd->use_uefi)
>  		return 0;
>  
> -	return pm8xxx_rtc_read_nvmem_offset(rtc_dd);
> +	if (rtc_dd->nvmem_cell)
> +		return pm8xxx_rtc_read_nvmem_offset(rtc_dd);
> +	else
> +		return pm8xxx_rtc_read_uefi_offset(rtc_dd);
>  }
>  
>  static int pm8xxx_rtc_read_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs)
> @@ -155,7 +255,7 @@ static int pm8xxx_rtc_update_offset(struct pm8xxx_rtc *rtc_dd, u32 secs)
>  	u32 offset;
>  	int rc;
>  
> -	if (!rtc_dd->nvmem_cell)
> +	if (!rtc_dd->nvmem_cell && !rtc_dd->use_uefi)
>  		return -ENODEV;
>  
>  	rc = pm8xxx_rtc_read_raw(rtc_dd, &raw_secs);
> @@ -167,7 +267,11 @@ static int pm8xxx_rtc_update_offset(struct pm8xxx_rtc *rtc_dd, u32 secs)
>  	if (offset == rtc_dd->offset)
>  		return 0;
>  
> -	rc = pm8xxx_rtc_write_nvmem_offset(rtc_dd, offset);
> +	if (rtc_dd->nvmem_cell)
> +		rc = pm8xxx_rtc_write_nvmem_offset(rtc_dd, offset);
> +	else
> +		rc = pm8xxx_rtc_write_uefi_offset(rtc_dd, offset);
> +
>  	if (rc)
>  		return rc;
>  
> @@ -486,6 +590,17 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
>  		if (rc != -ENOENT)
>  			return rc;
>  		rtc_dd->nvmem_cell = NULL;
> +
> +		/* Use UEFI storage as fallback if available */
> +		rtc_dd->use_uefi = of_property_read_bool(pdev->dev.of_node,
> +							 "qcom,uefi-rtc-info");
> +	}
> +
> +	if (rtc_dd->use_uefi && !efivar_is_available()) {
> +		if (IS_ENABLED(CONFIG_EFI))
> +			return -EPROBE_DEFER;
> +		dev_warn(&pdev->dev, "efivars not available\n");
> +		rtc_dd->use_uefi = false;
>  	}
>  
>  	rtc_dd->regs = match->data;
> diff --git a/include/linux/rtc.h b/include/linux/rtc.h
> index 3f4d315aaec9..95da051fb155 100644
> --- a/include/linux/rtc.h
> +++ b/include/linux/rtc.h
> @@ -170,6 +170,7 @@ struct rtc_device {
>  /* useful timestamps */
>  #define RTC_TIMESTAMP_BEGIN_0000	-62167219200ULL /* 0000-01-01 00:00:00 */
>  #define RTC_TIMESTAMP_BEGIN_1900	-2208988800LL /* 1900-01-01 00:00:00 */
> +#define RTC_TIMESTAMP_EPOCH_GPS		315964800LL /* 1980-01-06 00:00:00 */
>  #define RTC_TIMESTAMP_BEGIN_2000	946684800LL /* 2000-01-01 00:00:00 */
>  #define RTC_TIMESTAMP_END_2063		2966371199LL /* 2063-12-31 23:59:59 */
>  #define RTC_TIMESTAMP_END_2079		3471292799LL /* 2079-12-31 23:59:59 */
> -- 
> 2.45.2
> 
> 

Re: [PATCH 3/7] rtc: pm8xxx: add support for uefi offset

[Johan Hovold]

On Fri, Jan 24, 2025 at 01:56:56PM +0100, Tobias Heider wrote:
> On Mon, Jan 20, 2025 at 03:41:48PM GMT, Johan Hovold wrote:
> > On many Qualcomm platforms the PMIC RTC control and time registers are
> > read-only so that the RTC time can not be updated. Instead an offset
> > needs be stored in some machine-specific non-volatile memory, which the
> > driver can take into account.
> > 
> > Add support for storing a 32-bit offset from the GPS time epoch in a
> > UEFI variable so that the RTC time can be set on such platforms.

> Thanks for the great work Johan, seems to work nicely here!

Thanks for testing.

> I think you might need to add MODULE_IMPORT_NS(EFIVAR) for the efivar_*
> API calls you added.

It's already there (inside the ifdef):

> > +#ifdef CONFIG_EFI
> > +
> > +MODULE_IMPORT_NS("EFIVAR");

Johan

Re: [PATCH 0/7] arm64: dts: qcom: x1e80100: enable rtc

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

Hi,

On Mon, Jan 20, 2025 at 03:41:45PM +0100, Johan Hovold wrote:
> This series adds support for utilising the UEFI firmware RTC offset to
> the Qualcomm PMIC RTC driver and uses that to enable the RTC on all X
> Elite machines.
> 
> Included is also a patch to switch the Lenovo ThinkPad X13s over to
> using the UEFI offset.

I've been using this series for the last few days and it seems to
work well.

Tested-by: Sebastian Reichel <sre@kernel.org> # Lenovo T14s Gen6

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/7] arm64: dts: qcom: x1e80100: enable rtc

[Rob Herring]

On Mon, Jan 20, 2025 at 03:41:45PM +0100, Johan Hovold wrote:
> This series adds support for utilising the UEFI firmware RTC offset to
> the Qualcomm PMIC RTC driver and uses that to enable the RTC on all X
> Elite machines.
> 
> Included is also a patch to switch the Lenovo ThinkPad X13s over to
> using the UEFI offset.
> 
> The RTCs in many Qualcomm devices are effectively broken due to the time
> registers being read-only. Instead some other non-volatile memory can be
> used to store an offset which a driver can take into account. On Windows
> on Arm laptops, the UEFI firmware (and Windows) use a UEFI variable for
> storing such an offset.
> 
> When RTC support for the X13s was added two years ago we did not yet
> have UEFI variable support for these machines in mainline and there were
> also some concerns regarding flash wear. [1] As not all Qualcomm
> platforms have UEFI firmware anyway, we instead opted to use a PMIC
> scratch register for storing the offset. [2]
> 
> On the UEFI machines in question this is however arguable not correct
> as it means that the RTC time can differ between the UEFI firmware (and
> Windows) and Linux.
> 
> Now that the (reverse engineered) UEFI variable implementation has been
> merged and thoroughly tested, let's switch to using that to store the
> RTC offset also on Linux. The flash wear concerns can be mitigated by
> deferring writes due to clock drift until shutdown.
> 
> Note that this also avoids having to wait for months for Qualcomm to
> provide a free PMIC SDAM scratch register for X1E and future platforms,
> and specifically allows us to enable the RTC on X1E laptops today.
> 
> Rob had some concerns about adding a DT property for indicating that a
> machine uses UEFI for storing the offset and suggested that the driver
> should probe for this instead. Unfortunately, this is easier said than
> done given that UEFI variable support itself is probed for and may not
> be available until after the RTC driver probes.

This information would be useful in the binding commit...

Seems like something I would say, but this is v1 and I have no memory of 
discussing this. I would also say probe ordering is not a DT problem, 
but sounds like an OS problem. Aren't there other things needing EFI 
variables earlyish too? Do you really want to have to update the DT to 
enable this?

OTOH, it's one property, meh.

Rob

Re: [PATCH 2/7] dt-bindings: rtc: qcom-pm8xxx: document qcom,no-alarm flag

[Rob Herring (Arm)]

On Mon, 20 Jan 2025 15:41:47 +0100, Johan Hovold wrote:
> From: Jonathan Marek <jonathan@marek.ca>
> 
> Qualcomm x1e80100 firmware sets the ownership of the RTC alarm to ADSP.
> Thus writing to RTC alarm registers and receiving alarm interrupts is not
> possible.
> 
> Add a qcom,no-alarm flag to support RTC on this platform.
> 
> Signed-off-by: Jonathan Marek <jonathan@marek.ca>
> Link: https://lore.kernel.org/r/20241015004945.3676-3-jonathan@marek.ca
> [ johan: move vendor property; use boolean; reword description ]
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>

Re: [PATCH 0/7] arm64: dts: qcom: x1e80100: enable rtc

[Johan Hovold]

[ +CC: Ard and Maximilian ]

Hi Rob,

and sorry about the late follow up on this. I had to find some time to
think more about this so it ended up on the back burner.

On Sun, Jan 26, 2025 at 06:20:26PM -0600, Rob Herring wrote:
> On Mon, Jan 20, 2025 at 03:41:45PM +0100, Johan Hovold wrote:
> > This series adds support for utilising the UEFI firmware RTC offset to
> > the Qualcomm PMIC RTC driver and uses that to enable the RTC on all X
> > Elite machines.
> > 
> > Included is also a patch to switch the Lenovo ThinkPad X13s over to
> > using the UEFI offset.
> > 
> > The RTCs in many Qualcomm devices are effectively broken due to the time
> > registers being read-only. Instead some other non-volatile memory can be
> > used to store an offset which a driver can take into account. On Windows
> > on Arm laptops, the UEFI firmware (and Windows) use a UEFI variable for
> > storing such an offset.
> > 
> > When RTC support for the X13s was added two years ago we did not yet
> > have UEFI variable support for these machines in mainline and there were
> > also some concerns regarding flash wear. [1] As not all Qualcomm
> > platforms have UEFI firmware anyway, we instead opted to use a PMIC
> > scratch register for storing the offset. [2]
> > 
> > On the UEFI machines in question this is however arguable not correct
> > as it means that the RTC time can differ between the UEFI firmware (and
> > Windows) and Linux.
> > 
> > Now that the (reverse engineered) UEFI variable implementation has been
> > merged and thoroughly tested, let's switch to using that to store the
> > RTC offset also on Linux. The flash wear concerns can be mitigated by
> > deferring writes due to clock drift until shutdown.
> > 
> > Note that this also avoids having to wait for months for Qualcomm to
> > provide a free PMIC SDAM scratch register for X1E and future platforms,
> > and specifically allows us to enable the RTC on X1E laptops today.
> > 
> > Rob had some concerns about adding a DT property for indicating that a
> > machine uses UEFI for storing the offset and suggested that the driver
> > should probe for this instead. Unfortunately, this is easier said than
> > done given that UEFI variable support itself is probed for and may not
> > be available until after the RTC driver probes.
> 
> This information would be useful in the binding commit...
>
> Seems like something I would say, but this is v1 and I have no memory of 
> discussing this. 

You're right, I should have mentioned this in the commit message and
linked to the RFC discussion directly:

	https://lore.kernel.org/lkml/Y9qO0yQ7oLux2L9n@hovoldconsulting.com/

> I would also say probe ordering is not a DT problem, 
> but sounds like an OS problem. Aren't there other things needing EFI 
> variables earlyish too? Do you really want to have to update the DT to 
> enable this?

Yeah, there are more things that expect EFI variables during early boot,
including systemd. In fact, that is the reason why the Qualcomm efivars
implementation can currently only be built in:

	https://lore.kernel.org/lkml/ZJ11H8btBhvCx9gD@hovoldconsulting.com/

The variable service is supposed to be a runtime service that is
available when drivers probe (module init), so I think it's reasonably
to simple refuse further modular efivars implementation (we already have
another from Google).

Since allowing the Qualcomm implementation to be modular now would
regress user space it seems at least that one will need to stay built-in
indefinitely.

And again, hopefully all of this goes away (for future platforms) once
Qualcomm fix their UEFI implementation so that the UEFI time and
variable services can be used directly.

I've dropped the DT property for v2. 

Johan

Re: [PATCH 0/7] arm64: dts: qcom: x1e80100: enable rtc

[Rob Clark]

On Sun, Jan 26, 2025 at 4:20 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Jan 20, 2025 at 03:41:45PM +0100, Johan Hovold wrote:
> > This series adds support for utilising the UEFI firmware RTC offset to
> > the Qualcomm PMIC RTC driver and uses that to enable the RTC on all X
> > Elite machines.
> >
> > Included is also a patch to switch the Lenovo ThinkPad X13s over to
> > using the UEFI offset.
> >
> > The RTCs in many Qualcomm devices are effectively broken due to the time
> > registers being read-only. Instead some other non-volatile memory can be
> > used to store an offset which a driver can take into account. On Windows
> > on Arm laptops, the UEFI firmware (and Windows) use a UEFI variable for
> > storing such an offset.
> >
> > When RTC support for the X13s was added two years ago we did not yet
> > have UEFI variable support for these machines in mainline and there were
> > also some concerns regarding flash wear. [1] As not all Qualcomm
> > platforms have UEFI firmware anyway, we instead opted to use a PMIC
> > scratch register for storing the offset. [2]
> >
> > On the UEFI machines in question this is however arguable not correct
> > as it means that the RTC time can differ between the UEFI firmware (and
> > Windows) and Linux.
> >
> > Now that the (reverse engineered) UEFI variable implementation has been
> > merged and thoroughly tested, let's switch to using that to store the
> > RTC offset also on Linux. The flash wear concerns can be mitigated by
> > deferring writes due to clock drift until shutdown.
> >
> > Note that this also avoids having to wait for months for Qualcomm to
> > provide a free PMIC SDAM scratch register for X1E and future platforms,
> > and specifically allows us to enable the RTC on X1E laptops today.
> >
> > Rob had some concerns about adding a DT property for indicating that a
> > machine uses UEFI for storing the offset and suggested that the driver
> > should probe for this instead. Unfortunately, this is easier said than
> > done given that UEFI variable support itself is probed for and may not
> > be available until after the RTC driver probes.
>
> This information would be useful in the binding commit...
>
> Seems like something I would say, but this is v1 and I have no memory of
> discussing this. I would also say probe ordering is not a DT problem,
> but sounds like an OS problem. Aren't there other things needing EFI
> variables earlyish too? Do you really want to have to update the DT to
> enable this?

I was debugging why RTC offset was not working properly for me, and
eventually realized it was exactly this problem (efivars not avail
when rtc probes).

Hacking up rtc-pm8xxx to return -EPROBE_DEFER "fixes" it

BR,
-R

> OTOH, it's one property, meh.
>
> Rob
>

Re: [PATCH 0/7] arm64: dts: qcom: x1e80100: enable rtc

[Johan Hovold]

On Mon, Apr 21, 2025 at 07:36:28AM -0700, Rob Clark wrote:
> On Sun, Jan 26, 2025 at 4:20 PM Rob Herring <robh@kernel.org> wrote:
> > On Mon, Jan 20, 2025 at 03:41:45PM +0100, Johan Hovold wrote:
> > > This series adds support for utilising the UEFI firmware RTC offset to
> > > the Qualcomm PMIC RTC driver and uses that to enable the RTC on all X
> > > Elite machines.

> > > Rob had some concerns about adding a DT property for indicating that a
> > > machine uses UEFI for storing the offset and suggested that the driver
> > > should probe for this instead. Unfortunately, this is easier said than
> > > done given that UEFI variable support itself is probed for and may not
> > > be available until after the RTC driver probes.
> >
> > This information would be useful in the binding commit...
> >
> > Seems like something I would say, but this is v1 and I have no memory of
> > discussing this. I would also say probe ordering is not a DT problem,
> > but sounds like an OS problem. Aren't there other things needing EFI
> > variables earlyish too? Do you really want to have to update the DT to
> > enable this?
> 
> I was debugging why RTC offset was not working properly for me, and
> eventually realized it was exactly this problem (efivars not avail
> when rtc probes).

Hmm. It seems dropping that property for v2 under the assumption that
efivars would be available at module init time (since the driver can
only be built-in) was a mistake.

I see now that the current qcom efivars driver does not probe until
module init time itself, but even if we change that the scm driver
depends on an interconnect driver which can be built as a module...

> Hacking up rtc-pm8xxx to return -EPROBE_DEFER "fixes" it

> > OTOH, it's one property, meh.

I guess we need that property on these platforms as I had initially
concluded after all. As with the rest of this driver, hopefully all of
this goes away for future Qualcomm platforms once they fix their UEFI
implementation so that the time service can be used directly.

Johan