[PATCH 00/24] rtc: pm8xxx: add support for setting time using nvmem

[PATCH 00/24] rtc: pm8xxx: add support for setting time using nvmem

[Johan Hovold]

This series adds support for setting the RTC time on Qualcomm platforms
where the PMIC RTC time registers are read-only by instead storing an
offset in some other non-volatile memory. This is used to enable the RTC
in the SC8280XP Compute Reference Design (CRD) and Lenovo Thinkpad X13s
laptop.

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 and offset which a driver can take into account. On
machines like the X13s, the UEFI firmware (and Windows) use a UEFI
variable for storing such an offset, but not all Qualcomm systems use
UEFI.

The Qualcomm firmware also does not support any UEFI runtime services,
but Maximilian Luz recently posted a driver for talking to the secure
world directly through the SCM interface and this can be used to access
the UEFI variables:

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

I was initially told that the PMICs in the X13s did not have any spare
battery-backed registers which could have been used to store an RTC
offset so there seemed to be no alternative to try using the UEFI
offset. In the processes however, I learnt that there are in fact some
registers in PMIC that could be used, at least on the SC8280XP CRD and
the X13s. 

This was especially fortunate as it turned out that the firmware on the
CRD does not allow updating the UEFI RTC offset even if this works on
the X13s.

As the benefit of sharing the RTC offset with the UEFI firmware (and
Windows) is rather small (e.g. to make sure they never get out sync), I
instead opted for using the PMIC registers on both machines. This also
avoids relying on a fairly complex reverse-engineered firmware driver,
as well as potential issues like flash wear due to RTC drift. Let's keep
it simple.

But as there could be older Qualcomm UEFI machines out there where we
don't have any other non-volatile storage I included the UEFI patches
here as an RFC for reference. In case it turns out there are systems out
there were this could be used, those two patches could be merged as
well. An alternative could be to see if Maximilian's work could be
extended to access the time services directly.

This series first fixes a few issues with the current Qualcomm PMIC RTC
driver before cleaning it up a bit so that support for setting the time
using an offset stored in an nvmem cell can be added.

The two RFC patches on top, add support for the Qualcomm UEFI RTC offset
and are not intended to be merged just yet. Note that these two also
depend on an efi core patch that has been merged for 6.3:

	https://lore.kernel.org/all/20230119164255.28091-1-johan+linaro@kernel.org/

The final patches enables the RTC on the SC8280XP CRD and X13s and can
be merged by Bjorn once the (non-UEFI) RTC patches are in.

Note that for the SDAM nvmem driver to be autoloaded when built as a
module, you also need this fix:

	https://lore.kernel.org/lkml/20230126133034.27491-1-johan+linaro@kernel.org/

Johan


Johan Hovold (24):
  rtc: pm8xxx: fix set-alarm race
  rtc: pm8xxx: drop spmi error messages
  rtc: pm8xxx: use regmap_update_bits()
  rtc: pm8xxx: drop bogus locking
  rtc: pm8xxx: return IRQ_NONE on errors
  rtc: pm8xxx: drop unused register defines
  rtc: pm8xxx: use unaligned le32 helpers
  rtc: pm8xxx: clean up time and alarm debugging
  rtc: pm8xxx: rename struct device pointer
  rtc: pm8xxx: rename alarm irq variable
  rtc: pm8xxx: clean up comments
  rtc: pm8xxx: use u32 for timestamps
  rtc: pm8xxx: refactor read_time()
  rtc: pm8xxx: clean up local declarations
  dt-bindings: rtc: qcom-pm8xxx: add nvmem-cell offset
  rtc: pm8xxx: add support for nvmem offset
  rtc: pm8xxx: add copyright notice
  dt-bindings: rtc: qcom-pm8xxx: add uefi-variable offset
  rtc: pm8xxx: add support for uefi offset
  arm64: defconfig: enable Qualcomm SDAM nvmem driver
  arm64: dts: qcom: sc8280xp-pmics: add pmk8280 rtc
  arm64: dts: qcom: sc8280xp-pmics: add pmk8280 sdam nvram
  arm64: dts: qcom: sc8280xp-crd: enable rtc
  arm64: dts: qcom: sc8280xp-x13s: enable rtc

 .../bindings/rtc/qcom-pm8xxx-rtc.yaml         |  18 +
 arch/arm64/boot/dts/qcom/sc8280xp-crd.dts     |  15 +
 .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts    |  15 +
 arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi  |  18 +
 arch/arm64/configs/defconfig                  |   1 +
 drivers/rtc/rtc-pm8xxx.c                      | 639 ++++++++++--------
 include/linux/rtc.h                           |   1 +
 7 files changed, 443 insertions(+), 264 deletions(-)

-- 
2.39.1

[PATCH 01/24] rtc: pm8xxx: fix set-alarm race

[Johan Hovold]

Make sure to disable the alarm before updating the four alarm time
registers to avoid spurious alarms during the update.

Note that the disable needs to be done outside of the ctrl_reg_lock
section to prevent a racing alarm interrupt from disabling the newly set
alarm when the lock is released.

Fixes: 9a9a54ad7aa2 ("drivers/rtc: add support for Qualcomm PMIC8xxx RTC")
Cc: stable@vger.kernel.org      # 3.1
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/rtc/rtc-pm8xxx.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 716e5d9ad74d..d114f0da537d 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -221,7 +221,6 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 {
 	int rc, i;
 	u8 value[NUM_8_BIT_RTC_REGS];
-	unsigned int ctrl_reg;
 	unsigned long secs, irq_flags;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
@@ -233,6 +232,11 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 		secs >>= 8;
 	}
 
+	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
+				regs->alarm_en, 0);
+	if (rc)
+		return rc;
+
 	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
 
 	rc = regmap_bulk_write(rtc_dd->regmap, regs->alarm_rw, value,
@@ -242,19 +246,11 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 		goto rtc_rw_fail;
 	}
 
-	rc = regmap_read(rtc_dd->regmap, regs->alarm_ctrl, &ctrl_reg);
-	if (rc)
-		goto rtc_rw_fail;
-
-	if (alarm->enabled)
-		ctrl_reg |= regs->alarm_en;
-	else
-		ctrl_reg &= ~regs->alarm_en;
-
-	rc = regmap_write(rtc_dd->regmap, regs->alarm_ctrl, ctrl_reg);
-	if (rc) {
-		dev_err(dev, "Write to RTC alarm control register failed\n");
-		goto rtc_rw_fail;
+	if (alarm->enabled) {
+		rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
+					regs->alarm_en, regs->alarm_en);
+		if (rc)
+			goto rtc_rw_fail;
 	}
 
 	dev_dbg(dev, "Alarm Set for h:m:s=%ptRt, y-m-d=%ptRdr\n",
-- 
2.39.1

[PATCH 02/24] rtc: pm8xxx: drop spmi error messages

[Johan Hovold]

Drop the unnecessary error messages after every spmi regmap access,
which are not expected to fail.

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index d114f0da537d..f49bda999e02 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -105,10 +105,8 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 		alarm_enabled = 1;
 		ctrl_reg &= ~regs->alarm_en;
 		rc = regmap_write(rtc_dd->regmap, regs->alarm_ctrl, ctrl_reg);
-		if (rc) {
-			dev_err(dev, "Write to RTC Alarm control register failed\n");
+		if (rc)
 			goto rtc_rw_fail;
-		}
 	}
 
 	/* Disable RTC H/w before writing on RTC register */
@@ -120,51 +118,39 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 		rtc_disabled = 1;
 		rtc_ctrl_reg &= ~PM8xxx_RTC_ENABLE;
 		rc = regmap_write(rtc_dd->regmap, regs->ctrl, rtc_ctrl_reg);
-		if (rc) {
-			dev_err(dev, "Write to RTC control register failed\n");
+		if (rc)
 			goto rtc_rw_fail;
-		}
 	}
 
 	/* Write 0 to Byte[0] */
 	rc = regmap_write(rtc_dd->regmap, regs->write, 0);
-	if (rc) {
-		dev_err(dev, "Write to RTC write data register failed\n");
+	if (rc)
 		goto rtc_rw_fail;
-	}
 
 	/* Write Byte[1], Byte[2], Byte[3] */
 	rc = regmap_bulk_write(rtc_dd->regmap, regs->write + 1,
 			       &value[1], sizeof(value) - 1);
-	if (rc) {
-		dev_err(dev, "Write to RTC write data register failed\n");
+	if (rc)
 		goto rtc_rw_fail;
-	}
 
 	/* Write Byte[0] */
 	rc = regmap_write(rtc_dd->regmap, regs->write, value[0]);
-	if (rc) {
-		dev_err(dev, "Write to RTC write data register failed\n");
+	if (rc)
 		goto rtc_rw_fail;
-	}
 
 	/* Enable RTC H/w after writing on RTC register */
 	if (rtc_disabled) {
 		rtc_ctrl_reg |= PM8xxx_RTC_ENABLE;
 		rc = regmap_write(rtc_dd->regmap, regs->ctrl, rtc_ctrl_reg);
-		if (rc) {
-			dev_err(dev, "Write to RTC control register failed\n");
+		if (rc)
 			goto rtc_rw_fail;
-		}
 	}
 
 	if (alarm_enabled) {
 		ctrl_reg |= regs->alarm_en;
 		rc = regmap_write(rtc_dd->regmap, regs->alarm_ctrl, ctrl_reg);
-		if (rc) {
-			dev_err(dev, "Write to RTC Alarm control register failed\n");
+		if (rc)
 			goto rtc_rw_fail;
-		}
 	}
 
 rtc_rw_fail:
@@ -183,28 +169,22 @@ static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
 
 	rc = regmap_bulk_read(rtc_dd->regmap, regs->read, value, sizeof(value));
-	if (rc) {
-		dev_err(dev, "RTC read data register failed\n");
+	if (rc)
 		return rc;
-	}
 
 	/*
 	 * Read the LSB again and check if there has been a carry over.
 	 * If there is, redo the read operation.
 	 */
 	rc = regmap_read(rtc_dd->regmap, regs->read, &reg);
-	if (rc < 0) {
-		dev_err(dev, "RTC read data register failed\n");
+	if (rc < 0)
 		return rc;
-	}
 
 	if (unlikely(reg < value[0])) {
 		rc = regmap_bulk_read(rtc_dd->regmap, regs->read,
 				      value, sizeof(value));
-		if (rc) {
-			dev_err(dev, "RTC read data register failed\n");
+		if (rc)
 			return rc;
-		}
 	}
 
 	secs = value[0] | (value[1] << 8) | (value[2] << 16) |
@@ -241,10 +221,8 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 
 	rc = regmap_bulk_write(rtc_dd->regmap, regs->alarm_rw, value,
 			       sizeof(value));
-	if (rc) {
-		dev_err(dev, "Write to RTC ALARM register failed\n");
+	if (rc)
 		goto rtc_rw_fail;
-	}
 
 	if (alarm->enabled) {
 		rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
@@ -271,10 +249,8 @@ static int pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 
 	rc = regmap_bulk_read(rtc_dd->regmap, regs->alarm_rw, value,
 			      sizeof(value));
-	if (rc) {
-		dev_err(dev, "RTC alarm time read failed\n");
+	if (rc)
 		return rc;
-	}
 
 	secs = value[0] | (value[1] << 8) | (value[2] << 16) |
 	       ((unsigned long)value[3] << 24);
@@ -282,10 +258,9 @@ static int pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 	rtc_time64_to_tm(secs, &alarm->time);
 
 	rc = regmap_read(rtc_dd->regmap, regs->alarm_ctrl, &ctrl_reg);
-	if (rc) {
-		dev_err(dev, "Read from RTC alarm control register failed\n");
+	if (rc)
 		return rc;
-	}
+
 	alarm->enabled = !!(ctrl_reg & PM8xxx_RTC_ALARM_ENABLE);
 
 	dev_dbg(dev, "Alarm set for - h:m:s=%ptRt, y-m-d=%ptRdr\n",
@@ -315,19 +290,15 @@ static int pm8xxx_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
 		ctrl_reg &= ~regs->alarm_en;
 
 	rc = regmap_write(rtc_dd->regmap, regs->alarm_ctrl, ctrl_reg);
-	if (rc) {
-		dev_err(dev, "Write to RTC control register failed\n");
+	if (rc)
 		goto rtc_rw_fail;
-	}
 
 	/* Clear Alarm register */
 	if (!enable) {
 		rc = regmap_bulk_write(rtc_dd->regmap, regs->alarm_rw, value,
 				       sizeof(value));
-		if (rc) {
-			dev_err(dev, "Clear RTC ALARM register failed\n");
+		if (rc)
 			goto rtc_rw_fail;
-		}
 	}
 
 rtc_rw_fail:
@@ -366,8 +337,6 @@ static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
 	rc = regmap_write(rtc_dd->regmap, regs->alarm_ctrl, ctrl_reg);
 	if (rc) {
 		spin_unlock(&rtc_dd->ctrl_reg_lock);
-		dev_err(rtc_dd->rtc_dev,
-			"Write to alarm control register failed\n");
 		goto rtc_alarm_handled;
 	}
 
@@ -375,17 +344,11 @@ static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
 
 	/* Clear RTC alarm register */
 	rc = regmap_read(rtc_dd->regmap, regs->alarm_ctrl2, &ctrl_reg);
-	if (rc) {
-		dev_err(rtc_dd->rtc_dev,
-			"RTC Alarm control2 register read failed\n");
+	if (rc)
 		goto rtc_alarm_handled;
-	}
 
 	ctrl_reg |= PM8xxx_RTC_ALARM_CLEAR;
 	rc = regmap_write(rtc_dd->regmap, regs->alarm_ctrl2, ctrl_reg);
-	if (rc)
-		dev_err(rtc_dd->rtc_dev,
-			"Write to RTC Alarm control2 register failed\n");
 
 rtc_alarm_handled:
 	return IRQ_HANDLED;
-- 
2.39.1

[PATCH 03/24] rtc: pm8xxx: use regmap_update_bits()

[Johan Hovold]

Switch to using regmap_update_bits() instead of open coding
read-modify-write accesses.

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index f49bda999e02..8c2847ac64f4 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -78,10 +78,10 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	int rc, i;
 	unsigned long secs, irq_flags;
-	u8 value[NUM_8_BIT_RTC_REGS], alarm_enabled = 0, rtc_disabled = 0;
-	unsigned int ctrl_reg, rtc_ctrl_reg;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
+	u8 value[NUM_8_BIT_RTC_REGS];
+	bool alarm_enabled;
 
 	if (!rtc_dd->allow_set_time)
 		return -ENODEV;
@@ -97,31 +97,16 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 
 	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
 
-	rc = regmap_read(rtc_dd->regmap, regs->alarm_ctrl, &ctrl_reg);
+	rc = regmap_update_bits_check(rtc_dd->regmap, regs->alarm_ctrl,
+				      regs->alarm_en, 0, &alarm_enabled);
 	if (rc)
 		goto rtc_rw_fail;
 
-	if (ctrl_reg & regs->alarm_en) {
-		alarm_enabled = 1;
-		ctrl_reg &= ~regs->alarm_en;
-		rc = regmap_write(rtc_dd->regmap, regs->alarm_ctrl, ctrl_reg);
-		if (rc)
-			goto rtc_rw_fail;
-	}
-
 	/* Disable RTC H/w before writing on RTC register */
-	rc = regmap_read(rtc_dd->regmap, regs->ctrl, &rtc_ctrl_reg);
+	rc = regmap_update_bits(rtc_dd->regmap, regs->ctrl, PM8xxx_RTC_ENABLE, 0);
 	if (rc)
 		goto rtc_rw_fail;
 
-	if (rtc_ctrl_reg & PM8xxx_RTC_ENABLE) {
-		rtc_disabled = 1;
-		rtc_ctrl_reg &= ~PM8xxx_RTC_ENABLE;
-		rc = regmap_write(rtc_dd->regmap, regs->ctrl, rtc_ctrl_reg);
-		if (rc)
-			goto rtc_rw_fail;
-	}
-
 	/* Write 0 to Byte[0] */
 	rc = regmap_write(rtc_dd->regmap, regs->write, 0);
 	if (rc)
@@ -139,16 +124,14 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 		goto rtc_rw_fail;
 
 	/* Enable RTC H/w after writing on RTC register */
-	if (rtc_disabled) {
-		rtc_ctrl_reg |= PM8xxx_RTC_ENABLE;
-		rc = regmap_write(rtc_dd->regmap, regs->ctrl, rtc_ctrl_reg);
-		if (rc)
-			goto rtc_rw_fail;
-	}
+	rc = regmap_update_bits(rtc_dd->regmap, regs->ctrl, PM8xxx_RTC_ENABLE,
+				PM8xxx_RTC_ENABLE);
+	if (rc)
+		goto rtc_rw_fail;
 
 	if (alarm_enabled) {
-		ctrl_reg |= regs->alarm_en;
-		rc = regmap_write(rtc_dd->regmap, regs->alarm_ctrl, ctrl_reg);
+		rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
+					regs->alarm_en, regs->alarm_en);
 		if (rc)
 			goto rtc_rw_fail;
 	}
@@ -275,21 +258,18 @@ static int pm8xxx_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
 	unsigned long irq_flags;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
-	unsigned int ctrl_reg;
 	u8 value[NUM_8_BIT_RTC_REGS] = {0};
+	unsigned int val;
 
 	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
 
-	rc = regmap_read(rtc_dd->regmap, regs->alarm_ctrl, &ctrl_reg);
-	if (rc)
-		goto rtc_rw_fail;
-
 	if (enable)
-		ctrl_reg |= regs->alarm_en;
+		val = regs->alarm_en;
 	else
-		ctrl_reg &= ~regs->alarm_en;
+		val = 0;
 
-	rc = regmap_write(rtc_dd->regmap, regs->alarm_ctrl, ctrl_reg);
+	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
+				regs->alarm_en, val);
 	if (rc)
 		goto rtc_rw_fail;
 
@@ -318,7 +298,6 @@ static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
 {
 	struct pm8xxx_rtc *rtc_dd = dev_id;
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
-	unsigned int ctrl_reg;
 	int rc;
 
 	rtc_update_irq(rtc_dd->rtc, 1, RTC_IRQF | RTC_AF);
@@ -326,15 +305,8 @@ static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
 	spin_lock(&rtc_dd->ctrl_reg_lock);
 
 	/* Clear the alarm enable bit */
-	rc = regmap_read(rtc_dd->regmap, regs->alarm_ctrl, &ctrl_reg);
-	if (rc) {
-		spin_unlock(&rtc_dd->ctrl_reg_lock);
-		goto rtc_alarm_handled;
-	}
-
-	ctrl_reg &= ~regs->alarm_en;
-
-	rc = regmap_write(rtc_dd->regmap, regs->alarm_ctrl, ctrl_reg);
+	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
+				regs->alarm_en, 0);
 	if (rc) {
 		spin_unlock(&rtc_dd->ctrl_reg_lock);
 		goto rtc_alarm_handled;
@@ -343,13 +315,11 @@ static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
 	spin_unlock(&rtc_dd->ctrl_reg_lock);
 
 	/* Clear RTC alarm register */
-	rc = regmap_read(rtc_dd->regmap, regs->alarm_ctrl2, &ctrl_reg);
+	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl2,
+				PM8xxx_RTC_ALARM_CLEAR, 0);
 	if (rc)
 		goto rtc_alarm_handled;
 
-	ctrl_reg |= PM8xxx_RTC_ALARM_CLEAR;
-	rc = regmap_write(rtc_dd->regmap, regs->alarm_ctrl2, ctrl_reg);
-
 rtc_alarm_handled:
 	return IRQ_HANDLED;
 }
@@ -357,22 +327,9 @@ static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
 static int pm8xxx_rtc_enable(struct pm8xxx_rtc *rtc_dd)
 {
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
-	unsigned int ctrl_reg;
-	int rc;
-
-	/* Check if the RTC is on, else turn it on */
-	rc = regmap_read(rtc_dd->regmap, regs->ctrl, &ctrl_reg);
-	if (rc)
-		return rc;
 
-	if (!(ctrl_reg & PM8xxx_RTC_ENABLE)) {
-		ctrl_reg |= PM8xxx_RTC_ENABLE;
-		rc = regmap_write(rtc_dd->regmap, regs->ctrl, ctrl_reg);
-		if (rc)
-			return rc;
-	}
-
-	return 0;
+	return regmap_update_bits(rtc_dd->regmap, regs->ctrl, PM8xxx_RTC_ENABLE,
+				  PM8xxx_RTC_ENABLE);
 }
 
 static const struct pm8xxx_rtc_regs pm8921_regs = {
-- 
2.39.1

[PATCH 04/24] rtc: pm8xxx: drop bogus locking

[Johan Hovold]

Since commit c8d523a4b053 ("drivers/rtc/rtc-pm8xxx.c: rework to support
pm8941 rtc") which removed the shadow control register there is no need
for a driver lock.

Specifically, the rtc ops are serialised by rtc core and the interrupt
handler only unconditionally disables the alarm using the alarm_ctrl
register.

Note that since only the alarm enable bit of alarm_ctrl is used after
enabling the RTC at probe, the locking was not needed when doing open
coded read-modify-write cycles either.

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 8c2847ac64f4..053a04f74a91 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -53,7 +53,6 @@ struct pm8xxx_rtc_regs {
  * @rtc_alarm_irq:	rtc alarm irq number.
  * @regs:		rtc registers description.
  * @rtc_dev:		device structure.
- * @ctrl_reg_lock:	spinlock protecting access to ctrl_reg.
  */
 struct pm8xxx_rtc {
 	struct rtc_device *rtc;
@@ -62,7 +61,6 @@ struct pm8xxx_rtc {
 	int rtc_alarm_irq;
 	const struct pm8xxx_rtc_regs *regs;
 	struct device *rtc_dev;
-	spinlock_t ctrl_reg_lock;
 };
 
 /*
@@ -77,11 +75,11 @@ struct pm8xxx_rtc {
 static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	int rc, i;
-	unsigned long secs, irq_flags;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
 	u8 value[NUM_8_BIT_RTC_REGS];
 	bool alarm_enabled;
+	unsigned long secs;
 
 	if (!rtc_dd->allow_set_time)
 		return -ENODEV;
@@ -95,51 +93,46 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 		secs >>= 8;
 	}
 
-	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
-
 	rc = regmap_update_bits_check(rtc_dd->regmap, regs->alarm_ctrl,
 				      regs->alarm_en, 0, &alarm_enabled);
 	if (rc)
-		goto rtc_rw_fail;
+		return rc;
 
 	/* Disable RTC H/w before writing on RTC register */
 	rc = regmap_update_bits(rtc_dd->regmap, regs->ctrl, PM8xxx_RTC_ENABLE, 0);
 	if (rc)
-		goto rtc_rw_fail;
+		return rc;
 
 	/* Write 0 to Byte[0] */
 	rc = regmap_write(rtc_dd->regmap, regs->write, 0);
 	if (rc)
-		goto rtc_rw_fail;
+		return rc;
 
 	/* Write Byte[1], Byte[2], Byte[3] */
 	rc = regmap_bulk_write(rtc_dd->regmap, regs->write + 1,
 			       &value[1], sizeof(value) - 1);
 	if (rc)
-		goto rtc_rw_fail;
+		return rc;
 
 	/* Write Byte[0] */
 	rc = regmap_write(rtc_dd->regmap, regs->write, value[0]);
 	if (rc)
-		goto rtc_rw_fail;
+		return rc;
 
 	/* Enable RTC H/w after writing on RTC register */
 	rc = regmap_update_bits(rtc_dd->regmap, regs->ctrl, PM8xxx_RTC_ENABLE,
 				PM8xxx_RTC_ENABLE);
 	if (rc)
-		goto rtc_rw_fail;
+		return rc;
 
 	if (alarm_enabled) {
 		rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
 					regs->alarm_en, regs->alarm_en);
 		if (rc)
-			goto rtc_rw_fail;
+			return rc;
 	}
 
-rtc_rw_fail:
-	spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
-
-	return rc;
+	return 0;
 }
 
 static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
@@ -184,9 +177,9 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 {
 	int rc, i;
 	u8 value[NUM_8_BIT_RTC_REGS];
-	unsigned long secs, irq_flags;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
+	unsigned long secs;
 
 	secs = rtc_tm_to_time64(&alarm->time);
 
@@ -200,25 +193,22 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 	if (rc)
 		return rc;
 
-	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
-
 	rc = regmap_bulk_write(rtc_dd->regmap, regs->alarm_rw, value,
 			       sizeof(value));
 	if (rc)
-		goto rtc_rw_fail;
+		return rc;
 
 	if (alarm->enabled) {
 		rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
 					regs->alarm_en, regs->alarm_en);
 		if (rc)
-			goto rtc_rw_fail;
+			return rc;
 	}
 
 	dev_dbg(dev, "Alarm Set for h:m:s=%ptRt, y-m-d=%ptRdr\n",
 		&alarm->time, &alarm->time);
-rtc_rw_fail:
-	spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
-	return rc;
+
+	return 0;
 }
 
 static int pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
@@ -255,14 +245,11 @@ static int pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 static int pm8xxx_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
 {
 	int rc;
-	unsigned long irq_flags;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
 	u8 value[NUM_8_BIT_RTC_REGS] = {0};
 	unsigned int val;
 
-	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
-
 	if (enable)
 		val = regs->alarm_en;
 	else
@@ -271,19 +258,17 @@ static int pm8xxx_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
 	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
 				regs->alarm_en, val);
 	if (rc)
-		goto rtc_rw_fail;
+		return rc;
 
 	/* Clear Alarm register */
 	if (!enable) {
 		rc = regmap_bulk_write(rtc_dd->regmap, regs->alarm_rw, value,
 				       sizeof(value));
 		if (rc)
-			goto rtc_rw_fail;
+			return rc;
 	}
 
-rtc_rw_fail:
-	spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
-	return rc;
+	return 0;
 }
 
 static const struct rtc_class_ops pm8xxx_rtc_ops = {
@@ -302,25 +287,18 @@ static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
 
 	rtc_update_irq(rtc_dd->rtc, 1, RTC_IRQF | RTC_AF);
 
-	spin_lock(&rtc_dd->ctrl_reg_lock);
-
 	/* Clear the alarm enable bit */
 	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
 				regs->alarm_en, 0);
-	if (rc) {
-		spin_unlock(&rtc_dd->ctrl_reg_lock);
-		goto rtc_alarm_handled;
-	}
-
-	spin_unlock(&rtc_dd->ctrl_reg_lock);
+	if (rc)
+		goto out;
 
 	/* Clear RTC alarm register */
 	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl2,
 				PM8xxx_RTC_ALARM_CLEAR, 0);
 	if (rc)
-		goto rtc_alarm_handled;
-
-rtc_alarm_handled:
+		goto out;
+out:
 	return IRQ_HANDLED;
 }
 
@@ -398,9 +376,6 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 	if (rtc_dd == NULL)
 		return -ENOMEM;
 
-	/* Initialise spinlock to protect RTC control register */
-	spin_lock_init(&rtc_dd->ctrl_reg_lock);
-
 	rtc_dd->regmap = dev_get_regmap(pdev->dev.parent, NULL);
 	if (!rtc_dd->regmap) {
 		dev_err(&pdev->dev, "Parent regmap unavailable.\n");
-- 
2.39.1

[PATCH 05/24] rtc: pm8xxx: return IRQ_NONE on errors

[Johan Hovold]

In the unlikely event that disabling the alarm and clearing the status
ever fails, return IRQ_NONE instead of IRQ_HANDLED.

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 053a04f74a91..dc7e659cbb2a 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -291,14 +291,14 @@ static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
 	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
 				regs->alarm_en, 0);
 	if (rc)
-		goto out;
+		return IRQ_NONE;
 
 	/* Clear RTC alarm register */
 	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl2,
 				PM8xxx_RTC_ALARM_CLEAR, 0);
 	if (rc)
-		goto out;
-out:
+		return IRQ_NONE;
+
 	return IRQ_HANDLED;
 }
 
-- 
2.39.1

[PATCH 06/24] rtc: pm8xxx: drop unused register defines

[Johan Hovold]

Drop the original register defines which have been used since commit
c8d523a4b053 ("drivers/rtc/rtc-pm8xxx.c: rework to support pm8941 rtc").

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index dc7e659cbb2a..90027a7cfb12 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -12,12 +12,6 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
-/* RTC Register offsets from RTC CTRL REG */
-#define PM8XXX_ALARM_CTRL_OFFSET	0x01
-#define PM8XXX_RTC_WRITE_OFFSET		0x02
-#define PM8XXX_RTC_READ_OFFSET		0x06
-#define PM8XXX_ALARM_RW_OFFSET		0x0A
-
 /* RTC_CTRL register bit fields */
 #define PM8xxx_RTC_ENABLE		BIT(7)
 #define PM8xxx_RTC_ALARM_CLEAR		BIT(0)
-- 
2.39.1

[PATCH 07/24] rtc: pm8xxx: use unaligned le32 helpers

[Johan Hovold]

Use the unaligned le32 helpers instead of open coding when accessing the
time and alarm registers.

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 90027a7cfb12..5ff6898bcace 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -12,6 +12,8 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
+#include <asm/unaligned.h>
+
 /* RTC_CTRL register bit fields */
 #define PM8xxx_RTC_ENABLE		BIT(7)
 #define PM8xxx_RTC_ALARM_CLEAR		BIT(0)
@@ -68,25 +70,21 @@ struct pm8xxx_rtc {
  */
 static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
-	int rc, i;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
 	u8 value[NUM_8_BIT_RTC_REGS];
 	bool alarm_enabled;
 	unsigned long secs;
+	int rc;
 
 	if (!rtc_dd->allow_set_time)
 		return -ENODEV;
 
 	secs = rtc_tm_to_time64(tm);
+	put_unaligned_le32(secs, value);
 
 	dev_dbg(dev, "Seconds value to be written to RTC = %lu\n", secs);
 
-	for (i = 0; i < NUM_8_BIT_RTC_REGS; i++) {
-		value[i] = secs & 0xFF;
-		secs >>= 8;
-	}
-
 	rc = regmap_update_bits_check(rtc_dd->regmap, regs->alarm_ctrl,
 				      regs->alarm_en, 0, &alarm_enabled);
 	if (rc)
@@ -157,9 +155,7 @@ static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 			return rc;
 	}
 
-	secs = value[0] | (value[1] << 8) | (value[2] << 16) |
-	       ((unsigned long)value[3] << 24);
-
+	secs = get_unaligned_le32(value);
 	rtc_time64_to_tm(secs, tm);
 
 	dev_dbg(dev, "secs = %lu, h:m:s == %ptRt, y-m-d = %ptRdr\n", secs, tm, tm);
@@ -169,18 +165,14 @@ static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 
 static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 {
-	int rc, i;
 	u8 value[NUM_8_BIT_RTC_REGS];
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
 	unsigned long secs;
+	int rc;
 
 	secs = rtc_tm_to_time64(&alarm->time);
-
-	for (i = 0; i < NUM_8_BIT_RTC_REGS; i++) {
-		value[i] = secs & 0xFF;
-		secs >>= 8;
-	}
+	put_unaligned_le32(secs, value);
 
 	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
 				regs->alarm_en, 0);
@@ -219,9 +211,7 @@ static int pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 	if (rc)
 		return rc;
 
-	secs = value[0] | (value[1] << 8) | (value[2] << 16) |
-	       ((unsigned long)value[3] << 24);
-
+	secs = get_unaligned_le32(value);
 	rtc_time64_to_tm(secs, &alarm->time);
 
 	rc = regmap_read(rtc_dd->regmap, regs->alarm_ctrl, &ctrl_reg);
-- 
2.39.1

[PATCH 08/24] rtc: pm8xxx: clean up time and alarm debugging

[Johan Hovold]

Clean up the time and alarm callback debugging by using a consistent and
succinct human-readable (i.e. non-raw) format.

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 5ff6898bcace..8c364e5d3b57 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -83,7 +83,7 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	secs = rtc_tm_to_time64(tm);
 	put_unaligned_le32(secs, value);
 
-	dev_dbg(dev, "Seconds value to be written to RTC = %lu\n", secs);
+	dev_dbg(dev, "set time: %ptRd %ptRt (%lu)\n", tm, tm, secs);
 
 	rc = regmap_update_bits_check(rtc_dd->regmap, regs->alarm_ctrl,
 				      regs->alarm_en, 0, &alarm_enabled);
@@ -158,7 +158,7 @@ static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	secs = get_unaligned_le32(value);
 	rtc_time64_to_tm(secs, tm);
 
-	dev_dbg(dev, "secs = %lu, h:m:s == %ptRt, y-m-d = %ptRdr\n", secs, tm, tm);
+	dev_dbg(dev, "read time: %ptRd %ptRt (%lu)\n", tm, tm, secs);
 
 	return 0;
 }
@@ -191,8 +191,7 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 			return rc;
 	}
 
-	dev_dbg(dev, "Alarm Set for h:m:s=%ptRt, y-m-d=%ptRdr\n",
-		&alarm->time, &alarm->time);
+	dev_dbg(dev, "set alarm: %ptRd %ptRt\n", &alarm->time, &alarm->time);
 
 	return 0;
 }
@@ -220,8 +219,7 @@ static int pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 
 	alarm->enabled = !!(ctrl_reg & PM8xxx_RTC_ALARM_ENABLE);
 
-	dev_dbg(dev, "Alarm set for - h:m:s=%ptRt, y-m-d=%ptRdr\n",
-		&alarm->time, &alarm->time);
+	dev_dbg(dev, "read alarm: %ptRd %ptRt\n", &alarm->time, &alarm->time);
 
 	return 0;
 }
-- 
2.39.1

[PATCH 09/24] rtc: pm8xxx: rename struct device pointer

[Johan Hovold]

Rename the driver-data struct device pointer by dropping the "rtc"
prefix which is both redundant and misleading (as this is a pointer to
the platform device and not the rtc class device).

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 8c364e5d3b57..0fdbd233b10e 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -48,7 +48,7 @@ struct pm8xxx_rtc_regs {
  * @allow_set_time:	indicates whether writing to the RTC is allowed
  * @rtc_alarm_irq:	rtc alarm irq number.
  * @regs:		rtc registers description.
- * @rtc_dev:		device structure.
+ * @dev:		device structure
  */
 struct pm8xxx_rtc {
 	struct rtc_device *rtc;
@@ -56,7 +56,7 @@ struct pm8xxx_rtc {
 	bool allow_set_time;
 	int rtc_alarm_irq;
 	const struct pm8xxx_rtc_regs *regs;
-	struct device *rtc_dev;
+	struct device *dev;
 };
 
 /*
@@ -372,7 +372,7 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 						      "allow-set-time");
 
 	rtc_dd->regs = match->data;
-	rtc_dd->rtc_dev = &pdev->dev;
+	rtc_dd->dev = &pdev->dev;
 
 	rc = pm8xxx_rtc_enable(rtc_dd);
 	if (rc)
-- 
2.39.1

[PATCH 10/24] rtc: pm8xxx: rename alarm irq variable

[Johan Hovold]

Clean up the driver somewhat by renaming the driver-data alarm irq
variable by dropping the redundant "rtc" prefix.

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 0fdbd233b10e..ea867b20573a 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -46,7 +46,7 @@ struct pm8xxx_rtc_regs {
  * @rtc:		rtc device for this driver.
  * @regmap:		regmap used to access RTC registers
  * @allow_set_time:	indicates whether writing to the RTC is allowed
- * @rtc_alarm_irq:	rtc alarm irq number.
+ * @alarm_irq:		alarm irq number
  * @regs:		rtc registers description.
  * @dev:		device structure
  */
@@ -54,7 +54,7 @@ struct pm8xxx_rtc {
 	struct rtc_device *rtc;
 	struct regmap *regmap;
 	bool allow_set_time;
-	int rtc_alarm_irq;
+	int alarm_irq;
 	const struct pm8xxx_rtc_regs *regs;
 	struct device *dev;
 };
@@ -364,8 +364,8 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
-	rtc_dd->rtc_alarm_irq = platform_get_irq(pdev, 0);
-	if (rtc_dd->rtc_alarm_irq < 0)
+	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,
@@ -391,7 +391,7 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 	rtc_dd->rtc->range_max = U32_MAX;
 
 	/* Request the alarm IRQ */
-	rc = devm_request_any_context_irq(&pdev->dev, rtc_dd->rtc_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);
@@ -404,7 +404,7 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 	if (rc)
 		return rc;
 
-	rc = dev_pm_set_wake_irq(&pdev->dev, rtc_dd->rtc_alarm_irq);
+	rc = dev_pm_set_wake_irq(&pdev->dev, rtc_dd->alarm_irq);
 	if (rc)
 		return rc;
 
-- 
2.39.1

[PATCH 11/24] rtc: pm8xxx: clean up comments

[Johan Hovold]

Clean up the driver comments somewhat and remove obsolete, incorrect or
redundant ones.

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index ea867b20573a..8a94a19e0d14 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -23,13 +23,13 @@
 
 /**
  * struct pm8xxx_rtc_regs - describe RTC registers per PMIC versions
- * @ctrl: base address of control register
- * @write: base address of write register
- * @read: base address of read register
- * @alarm_ctrl: base address of alarm control register
- * @alarm_ctrl2: base address of alarm control2 register
- * @alarm_rw: base address of alarm read-write register
- * @alarm_en: alarm enable mask
+ * @ctrl:		address of control register
+ * @write:		base address of write registers
+ * @read:		base address of read registers
+ * @alarm_ctrl:		address of alarm control register
+ * @alarm_ctrl2:	address of alarm control2 register
+ * @alarm_rw:		base address of alarm read-write registers
+ * @alarm_en:		alarm enable mask
  */
 struct pm8xxx_rtc_regs {
 	unsigned int ctrl;
@@ -42,12 +42,12 @@ struct pm8xxx_rtc_regs {
 };
 
 /**
- * struct pm8xxx_rtc -  rtc driver internal structure
- * @rtc:		rtc device for this driver.
- * @regmap:		regmap used to access RTC registers
- * @allow_set_time:	indicates whether writing to the RTC is allowed
+ * 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
  * @alarm_irq:		alarm irq number
- * @regs:		rtc registers description.
+ * @regs:		register description
  * @dev:		device structure
  */
 struct pm8xxx_rtc {
@@ -90,7 +90,7 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	if (rc)
 		return rc;
 
-	/* Disable RTC H/w before writing on RTC register */
+	/* Disable RTC */
 	rc = regmap_update_bits(rtc_dd->regmap, regs->ctrl, PM8xxx_RTC_ENABLE, 0);
 	if (rc)
 		return rc;
@@ -111,7 +111,7 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	if (rc)
 		return rc;
 
-	/* Enable RTC H/w after writing on RTC register */
+	/* Enable RTC */
 	rc = regmap_update_bits(rtc_dd->regmap, regs->ctrl, PM8xxx_RTC_ENABLE,
 				PM8xxx_RTC_ENABLE);
 	if (rc)
@@ -242,7 +242,7 @@ static int pm8xxx_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
 	if (rc)
 		return rc;
 
-	/* Clear Alarm register */
+	/* Clear alarm register */
 	if (!enable) {
 		rc = regmap_bulk_write(rtc_dd->regmap, regs->alarm_rw, value,
 				       sizeof(value));
@@ -269,13 +269,13 @@ static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
 
 	rtc_update_irq(rtc_dd->rtc, 1, RTC_IRQF | RTC_AF);
 
-	/* Clear the alarm enable bit */
+	/* Disable alarm */
 	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
 				regs->alarm_en, 0);
 	if (rc)
 		return IRQ_NONE;
 
-	/* Clear RTC alarm register */
+	/* Clear alarm status */
 	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl2,
 				PM8xxx_RTC_ALARM_CLEAR, 0);
 	if (rc)
@@ -332,9 +332,6 @@ static const struct pm8xxx_rtc_regs pmk8350_regs = {
 	.alarm_en	= BIT(7),
 };
 
-/*
- * Hardcoded RTC bases until IORESOURCE_REG mapping is figured out
- */
 static const struct of_device_id pm8xxx_id_table[] = {
 	{ .compatible = "qcom,pm8921-rtc", .data = &pm8921_regs },
 	{ .compatible = "qcom,pm8058-rtc", .data = &pm8058_regs },
@@ -382,7 +379,6 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, 1);
 
-	/* Register the RTC device */
 	rtc_dd->rtc = devm_rtc_allocate_device(&pdev->dev);
 	if (IS_ERR(rtc_dd->rtc))
 		return PTR_ERR(rtc_dd->rtc);
@@ -390,7 +386,6 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 	rtc_dd->rtc->ops = &pm8xxx_rtc_ops;
 	rtc_dd->rtc->range_max = U32_MAX;
 
-	/* Request the alarm IRQ */
 	rc = devm_request_any_context_irq(&pdev->dev, rtc_dd->alarm_irq,
 					  pm8xxx_alarm_trigger,
 					  IRQF_TRIGGER_RISING,
-- 
2.39.1

[PATCH 12/24] rtc: pm8xxx: use u32 for timestamps

[Johan Hovold]

The PMIC RTC registers are 32-bit so explicitly use u32 rather than
unsigned long for timestamps to reflect the hardware.

This will also help avoid unintentional range extensions when adding
support for managing an external offset.

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 8a94a19e0d14..b1ce246c501a 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -74,7 +74,7 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
 	u8 value[NUM_8_BIT_RTC_REGS];
 	bool alarm_enabled;
-	unsigned long secs;
+	u32 secs;
 	int rc;
 
 	if (!rtc_dd->allow_set_time)
@@ -83,7 +83,7 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	secs = rtc_tm_to_time64(tm);
 	put_unaligned_le32(secs, value);
 
-	dev_dbg(dev, "set time: %ptRd %ptRt (%lu)\n", tm, tm, secs);
+	dev_dbg(dev, "set time: %ptRd %ptRt (%u)\n", tm, tm, secs);
 
 	rc = regmap_update_bits_check(rtc_dd->regmap, regs->alarm_ctrl,
 				      regs->alarm_en, 0, &alarm_enabled);
@@ -131,10 +131,10 @@ static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	int rc;
 	u8 value[NUM_8_BIT_RTC_REGS];
-	unsigned long secs;
 	unsigned int reg;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
+	u32 secs;
 
 	rc = regmap_bulk_read(rtc_dd->regmap, regs->read, value, sizeof(value));
 	if (rc)
@@ -158,7 +158,7 @@ static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	secs = get_unaligned_le32(value);
 	rtc_time64_to_tm(secs, tm);
 
-	dev_dbg(dev, "read time: %ptRd %ptRt (%lu)\n", tm, tm, secs);
+	dev_dbg(dev, "read time: %ptRd %ptRt (%u)\n", tm, tm, secs);
 
 	return 0;
 }
@@ -168,7 +168,7 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 	u8 value[NUM_8_BIT_RTC_REGS];
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
-	unsigned long secs;
+	u32 secs;
 	int rc;
 
 	secs = rtc_tm_to_time64(&alarm->time);
@@ -201,9 +201,9 @@ static int pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 	int rc;
 	unsigned int ctrl_reg;
 	u8 value[NUM_8_BIT_RTC_REGS];
-	unsigned long secs;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
+	u32 secs;
 
 	rc = regmap_bulk_read(rtc_dd->regmap, regs->alarm_rw, value,
 			      sizeof(value));
-- 
2.39.1

[PATCH 13/24] rtc: pm8xxx: refactor read_time()

[Johan Hovold]

In preparation for adding support for setting the time by means of an
externally stored offset, refactor read_time() by adding a new helper
that can be used to retrieve the raw time as stored in the RTC.

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index b1ce246c501a..2f96a178595c 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -59,6 +59,37 @@ struct pm8xxx_rtc {
 	struct device *dev;
 };
 
+static int pm8xxx_rtc_read_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs)
+{
+	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
+	u8 value[NUM_8_BIT_RTC_REGS];
+	unsigned int reg;
+	int rc;
+
+	rc = regmap_bulk_read(rtc_dd->regmap, regs->read, value, sizeof(value));
+	if (rc)
+		return rc;
+
+	/*
+	 * Read the LSB again and check if there has been a carry over.
+	 * If there has, redo the read operation.
+	 */
+	rc = regmap_read(rtc_dd->regmap, regs->read, &reg);
+	if (rc < 0)
+		return rc;
+
+	if (reg < value[0]) {
+		rc = regmap_bulk_read(rtc_dd->regmap, regs->read, value,
+				      sizeof(value));
+		if (rc)
+			return rc;
+	}
+
+	*secs = get_unaligned_le32(value);
+
+	return 0;
+}
+
 /*
  * Steps to write the RTC registers.
  * 1. Disable alarm if enabled.
@@ -129,33 +160,14 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 
 static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
-	int rc;
-	u8 value[NUM_8_BIT_RTC_REGS];
-	unsigned int reg;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
-	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
 	u32 secs;
+	int rc;
 
-	rc = regmap_bulk_read(rtc_dd->regmap, regs->read, value, sizeof(value));
+	rc = pm8xxx_rtc_read_raw(rtc_dd, &secs);
 	if (rc)
 		return rc;
 
-	/*
-	 * Read the LSB again and check if there has been a carry over.
-	 * If there is, redo the read operation.
-	 */
-	rc = regmap_read(rtc_dd->regmap, regs->read, &reg);
-	if (rc < 0)
-		return rc;
-
-	if (unlikely(reg < value[0])) {
-		rc = regmap_bulk_read(rtc_dd->regmap, regs->read,
-				      value, sizeof(value));
-		if (rc)
-			return rc;
-	}
-
-	secs = get_unaligned_le32(value);
 	rtc_time64_to_tm(secs, tm);
 
 	dev_dbg(dev, "read time: %ptRd %ptRt (%u)\n", tm, tm, secs);
-- 
2.39.1

[PATCH 14/24] rtc: pm8xxx: clean up local declarations

[Johan Hovold]

Clean up local declarations somewhat by using the reverse xmas style
consistently throughout.

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 2f96a178595c..922aef0f0241 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -177,9 +177,9 @@ static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 
 static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 {
-	u8 value[NUM_8_BIT_RTC_REGS];
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
+	u8 value[NUM_8_BIT_RTC_REGS];
 	u32 secs;
 	int rc;
 
@@ -210,12 +210,12 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 
 static int pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 {
-	int rc;
-	unsigned int ctrl_reg;
-	u8 value[NUM_8_BIT_RTC_REGS];
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
+	u8 value[NUM_8_BIT_RTC_REGS];
+	unsigned int ctrl_reg;
 	u32 secs;
+	int rc;
 
 	rc = regmap_bulk_read(rtc_dd->regmap, regs->alarm_rw, value,
 			      sizeof(value));
@@ -238,11 +238,11 @@ static int pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 
 static int pm8xxx_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
 {
-	int rc;
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
 	u8 value[NUM_8_BIT_RTC_REGS] = {0};
 	unsigned int val;
+	int rc;
 
 	if (enable)
 		val = regs->alarm_en;
@@ -355,9 +355,9 @@ MODULE_DEVICE_TABLE(of, pm8xxx_id_table);
 
 static int pm8xxx_rtc_probe(struct platform_device *pdev)
 {
-	int rc;
-	struct pm8xxx_rtc *rtc_dd;
 	const struct of_device_id *match;
+	struct pm8xxx_rtc *rtc_dd;
+	int rc;
 
 	match = of_match_node(pm8xxx_id_table, pdev->dev.of_node);
 	if (!match)
-- 
2.39.1

[PATCH 15/24] dt-bindings: rtc: qcom-pm8xxx: add nvmem-cell 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 an 'offset' nvmem cell which can be used to store a 32-bit offset
from the Unix epoch so that the RTC time can be updated on such
platforms.

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

diff --git a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
index 21c8ea08ff0a..b95a69cc9ae0 100644
--- a/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
+++ b/Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml
@@ -40,6 +40,16 @@ properties:
     description:
       Indicates that the setting of RTC time is allowed by the host CPU.
 
+  nvmem-cells:
+    items:
+      - description:
+          four-byte nvmem cell holding a little-endian offset from the Unix
+          epoch representing the time when the RTC timer was last reset
+
+  nvmem-cell-names:
+    items:
+      - const: offset
+
   wakeup-source: true
 
 required:
@@ -69,6 +79,8 @@ examples:
           compatible = "qcom,pm8921-rtc";
           reg = <0x11d>;
           interrupts = <0x27 0>;
+          nvmem-cells = <&rtc_offset>;
+          nvmem-cell-names = "offset";
         };
       };
     };
-- 
2.39.1

[PATCH 16/24] rtc: pm8xxx: add support for nvmem 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 Epoch in an nvmem cell
so that the RTC time can be set on such platforms.

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 922aef0f0241..09816b9f6282 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -3,6 +3,7 @@
  */
 #include <linux/of.h>
 #include <linux/module.h>
+#include <linux/nvmem-consumer.h>
 #include <linux/init.h>
 #include <linux/rtc.h>
 #include <linux/platform_device.h>
@@ -49,6 +50,8 @@ struct pm8xxx_rtc_regs {
  * @alarm_irq:		alarm irq number
  * @regs:		register description
  * @dev:		device structure
+ * @nvmem_cell:		nvmem cell for offset
+ * @offset:		offset from epoch in seconds
  */
 struct pm8xxx_rtc {
 	struct rtc_device *rtc;
@@ -57,8 +60,60 @@ struct pm8xxx_rtc {
 	int alarm_irq;
 	const struct pm8xxx_rtc_regs *regs;
 	struct device *dev;
+	struct nvmem_cell *nvmem_cell;
+	u32 offset;
 };
 
+static int pm8xxx_rtc_read_nvmem_offset(struct pm8xxx_rtc *rtc_dd)
+{
+	size_t len;
+	void *buf;
+	int rc;
+
+	buf = nvmem_cell_read(rtc_dd->nvmem_cell, &len);
+	if (IS_ERR(buf)) {
+		rc = PTR_ERR(buf);
+		dev_err(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc);
+		return rc;
+	}
+
+	if (len != sizeof(u32)) {
+		dev_err(rtc_dd->dev, "unexpected nvmem cell size %zu\n", len);
+		kfree(buf);
+		return -EINVAL;
+	}
+
+	rtc_dd->offset = get_unaligned_le32(buf);
+
+	kfree(buf);
+
+	return 0;
+}
+
+static int pm8xxx_rtc_write_nvmem_offset(struct pm8xxx_rtc *rtc_dd, u32 offset)
+{
+	u8 buf[sizeof(u32)];
+	int rc;
+
+	put_unaligned_le32(offset, buf);
+
+	rc = nvmem_cell_write(rtc_dd->nvmem_cell, buf, sizeof(buf));
+	if (rc < 0) {
+		dev_err(rtc_dd->dev, "failed to write nvmem offset: %d\n", rc);
+		return rc;
+	}
+
+	return 0;
+}
+
+static int pm8xxx_rtc_read_offset(struct pm8xxx_rtc *rtc_dd)
+{
+	if (!rtc_dd->nvmem_cell)
+		return 0;
+
+	return pm8xxx_rtc_read_nvmem_offset(rtc_dd);
+}
+
 static int pm8xxx_rtc_read_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs)
 {
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
@@ -90,6 +145,33 @@ static int pm8xxx_rtc_read_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs)
 	return 0;
 }
 
+static int pm8xxx_rtc_update_offset(struct pm8xxx_rtc *rtc_dd, u32 secs)
+{
+	u32 raw_secs;
+	u32 offset;
+	int rc;
+
+	if (!rtc_dd->nvmem_cell)
+		return -ENODEV;
+
+	rc = pm8xxx_rtc_read_raw(rtc_dd, &raw_secs);
+	if (rc)
+		return rc;
+
+	offset = secs - raw_secs;
+
+	if (offset == rtc_dd->offset)
+		return 0;
+
+	rc = pm8xxx_rtc_write_nvmem_offset(rtc_dd, offset);
+	if (rc)
+		return rc;
+
+	rtc_dd->offset = offset;
+
+	return 0;
+}
+
 /*
  * Steps to write the RTC registers.
  * 1. Disable alarm if enabled.
@@ -99,23 +181,15 @@ static int pm8xxx_rtc_read_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs)
  * 5. Enable rtc if disabled in step 2.
  * 6. Enable alarm if disabled in step 1.
  */
-static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
+static int __pm8xxx_rtc_set_time(struct pm8xxx_rtc *rtc_dd, u32 secs)
 {
-	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
 	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
 	u8 value[NUM_8_BIT_RTC_REGS];
 	bool alarm_enabled;
-	u32 secs;
 	int rc;
 
-	if (!rtc_dd->allow_set_time)
-		return -ENODEV;
-
-	secs = rtc_tm_to_time64(tm);
 	put_unaligned_le32(secs, value);
 
-	dev_dbg(dev, "set time: %ptRd %ptRt (%u)\n", tm, tm, secs);
-
 	rc = regmap_update_bits_check(rtc_dd->regmap, regs->alarm_ctrl,
 				      regs->alarm_en, 0, &alarm_enabled);
 	if (rc)
@@ -158,6 +232,27 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
 	return 0;
 }
 
+static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
+	u32 secs;
+	int rc;
+
+	secs = rtc_tm_to_time64(tm);
+
+	if (rtc_dd->allow_set_time)
+		rc = __pm8xxx_rtc_set_time(rtc_dd, secs);
+	else
+		rc = pm8xxx_rtc_update_offset(rtc_dd, secs);
+
+	if (rc)
+		return rc;
+
+	dev_dbg(dev, "set time: %ptRd %ptRt (%u + %u)\n", tm, tm,
+			secs - rtc_dd->offset, rtc_dd->offset);
+	return 0;
+}
+
 static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
@@ -168,10 +263,11 @@ static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
 	if (rc)
 		return rc;
 
+	secs += rtc_dd->offset;
 	rtc_time64_to_tm(secs, tm);
 
-	dev_dbg(dev, "read time: %ptRd %ptRt (%u)\n", tm, tm, secs);
-
+	dev_dbg(dev, "read time: %ptRd %ptRt (%u + %u)\n", tm, tm,
+			secs - rtc_dd->offset, rtc_dd->offset);
 	return 0;
 }
 
@@ -184,6 +280,7 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 	int rc;
 
 	secs = rtc_tm_to_time64(&alarm->time);
+	secs -= rtc_dd->offset;
 	put_unaligned_le32(secs, value);
 
 	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
@@ -223,6 +320,7 @@ static int pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
 		return rc;
 
 	secs = get_unaligned_le32(value);
+	secs += rtc_dd->offset;
 	rtc_time64_to_tm(secs, &alarm->time);
 
 	rc = regmap_read(rtc_dd->regmap, regs->alarm_ctrl, &ctrl_reg);
@@ -380,9 +478,23 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 	rtc_dd->allow_set_time = of_property_read_bool(pdev->dev.of_node,
 						      "allow-set-time");
 
+	rtc_dd->nvmem_cell = devm_nvmem_cell_get(&pdev->dev, "offset");
+	if (IS_ERR(rtc_dd->nvmem_cell)) {
+		rc = PTR_ERR(rtc_dd->nvmem_cell);
+		if (rc != -ENOENT)
+			return rc;
+		rtc_dd->nvmem_cell = NULL;
+	}
+
 	rtc_dd->regs = match->data;
 	rtc_dd->dev = &pdev->dev;
 
+	if (!rtc_dd->allow_set_time) {
+		rc = pm8xxx_rtc_read_offset(rtc_dd);
+		if (rc)
+			return rc;
+	}
+
 	rc = pm8xxx_rtc_enable(rtc_dd);
 	if (rc)
 		return rc;
-- 
2.39.1

[PATCH 17/24] rtc: pm8xxx: add copyright notice

[Johan Hovold]

Add a copyright notice for Linaro and add myself as a (primary) author
of this driver.

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 09816b9f6282..25bdd804b4d2 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -1,5 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0-only
-/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
+/*
+ * pm8xxx RTC driver
+ *
+ * Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
+ * Copyright (c) 2023, Linaro Limited
  */
 #include <linux/of.h>
 #include <linux/module.h>
@@ -551,3 +555,4 @@ MODULE_ALIAS("platform:rtc-pm8xxx");
 MODULE_DESCRIPTION("PMIC8xxx RTC driver");
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Anirudh Ghayal <aghayal@codeaurora.org>");
+MODULE_AUTHOR("Johan Hovold <johan@kernel.org>");
-- 
2.39.1

[RFC 18/24] 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 b95a69cc9ae0..774c34c3f8f6 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.39.1

[PATCH 19/24] 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.

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 25bdd804b4d2..6c2324baeec6 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>
@@ -17,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
+#include <asm/byteorder.h>
 #include <asm/unaligned.h>
 
 /* RTC_CTRL register bit fields */
@@ -46,6 +48,11 @@ 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
@@ -54,6 +61,7 @@ struct pm8xxx_rtc_regs {
  * @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 +69,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_err(dev, "failed to read UEFI offset: %lu\n", status);
+		return efi_status_to_err(status);
+	}
+
+	if (size != sizeof(*rtc_info)) {
+		dev_err(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_BEGIN_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_BEGIN_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_err(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 +208,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 +254,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 +266,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;
 
@@ -488,6 +591,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 1fd9c6a21ebe..1ecee2fe4214 100644
--- a/include/linux/rtc.h
+++ b/include/linux/rtc.h
@@ -169,6 +169,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_BEGIN_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.39.1

[PATCH 20/24] arm64: defconfig: enable Qualcomm SDAM nvmem driver

[Johan Hovold]

The SDAM nvmem driver can be used to access the Shared Direct Access
Memory Module registers in some Qualcomm PMICs.

These registers can specifically be used to store a time offset on
platforms where the PMIC RTC time registers are read-only in order to
allow the RTC time to be updated.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index a17cbdac1904..b3f714ff4006 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1308,6 +1308,7 @@ CONFIG_NVMEM_MTK_EFUSE=y
 CONFIG_NVMEM_QCOM_QFPROM=y
 CONFIG_NVMEM_ROCKCHIP_EFUSE=y
 CONFIG_NVMEM_SNVS_LPGPR=y
+CONFIG_NVMEM_SPMI_SDAM=m
 CONFIG_NVMEM_SUNXI_SID=y
 CONFIG_NVMEM_UNIPHIER_EFUSE=y
 CONFIG_NVMEM_MESON_EFUSE=m
-- 
2.39.1

[PATCH 21/24] arm64: dts: qcom: sc8280xp-pmics: add pmk8280 rtc

[Johan Hovold]

The PMK8280 has an RTC which can also be used as a wakeup source.

Note that the RTC can not be disabled and updating the time is not
permitted either. Instead an offset can be stored in some other machine-
specific non-volatile memory which a driver can take into account.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
index f2c0b71b5d8e..c62262b88810 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
@@ -95,6 +95,15 @@ pmk8280_adc_tm: adc-tm@3400 {
 			#thermal-sensor-cells = <1>;
 			status = "disabled";
 		};
+
+		pmk8280_rtc: rtc@6100 {
+			compatible = "qcom,pmk8350-rtc";
+			reg = <0x6100>, <0x6200>;
+			reg-names = "rtc", "alarm";
+			interrupts = <0x0 0x62 0x1 IRQ_TYPE_EDGE_RISING>;
+			wakeup-source;
+			status = "disabled";
+		};
 	};
 
 	pmc8280_1: pmic@1 {
-- 
2.39.1

[PATCH 22/24] arm64: dts: qcom: sc8280xp-pmics: add pmk8280 sdam nvram

[Johan Hovold]

Add one of the PMK8280 SDAM blocks which can be used to store an RTC
offset.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
index c62262b88810..7220d5b5ab7b 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi
@@ -104,6 +104,15 @@ pmk8280_rtc: rtc@6100 {
 			wakeup-source;
 			status = "disabled";
 		};
+
+		pmk8280_sdam_6: nvram@8500 {
+			compatible = "qcom,spmi-sdam";
+			reg = <0x8500 0x100>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges = <0 0x8500 0x100>;
+			status = "disabled";
+		};
 	};
 
 	pmc8280_1: pmic@1 {
-- 
2.39.1

[PATCH 23/24] arm64: dts: qcom: sc8280xp-crd: enable rtc

[Johan Hovold]

The SC8280XP CRD firmware does not implement the UEFI time runtime
services so the RTC in the PM8280K PMIC needs to be accessed directly.

To complicate things further, the RTC control and time registers are
read-only on this platform so an offset must be stored in some other
machine-specific non-volatile memory which an RTC driver can take into
account when reading or updating the time.

The UEFI firmware (and Windows) use a UEFI variable for this:

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

but the offset can only be accessed via the Qualcomm UEFI Secure
Application residing in the TEE as the firmware does not implement the
variable runtime services either.

Unfortunately setting variables using this interface does not work on
the CRD so updating the time would not be possible. Instead, reserve
four bytes in one of the PMIC SDAM blocks to hold the RTC offset.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
index c780b82e498d..192e076345d2 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
@@ -500,6 +500,21 @@ &pmk8280_pon_pwrkey {
 	status = "okay";
 };
 
+&pmk8280_rtc {
+	nvmem-cells = <&rtc_offset>;
+	nvmem-cell-names = "offset";
+
+	status = "okay";
+};
+
+&pmk8280_sdam_6 {
+	status = "okay";
+
+	rtc_offset: rtc-offset@bc {
+		reg = <0xbc 0x4>;
+	};
+};
+
 &qup0 {
 	status = "okay";
 };
-- 
2.39.1

[PATCH 24/24] arm64: dts: qcom: sc8280xp-x13s: enable rtc

[Johan Hovold]

The Lenovo X13s firmware does not implement the UEFI time runtime
services so the RTC in the PM8280K PMIC needs to be accessed directly.

To complicate things further, the RTC control and time registers are
read-only on this platform so an offset must be stored in some other
machine-specific non-volatile memory which an RTC driver can take into
account when reading or updating the time.

The UEFI firmware (and Windows) use a UEFI variable for this:

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

but the offset can only be accessed via the Qualcomm UEFI Secure
Application residing in the TEE as the firmware does not implement the
variable runtime services either.

While it is possible to access this UEFI variable from Linux on the
X13s, this requires using a fairly complex and reverse-engineered
firmware interface. As the only benefit of doing so is to make sure that
the UEFI (Windows) and Linux time never gets out of sync, it seems
preferable to use the PMIC scratch registers for storing an offset
instead. This also avoids flash wear in case of RTC drift, etc.

So instead of using the UEFI RTC offset, reserve four bytes in one of
the PMIC SDAM blocks to hold the RTC offset.

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

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 db406f7774de..6e88e0bb6871 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -683,6 +683,21 @@ &pmk8280_pon_resin {
 	status = "okay";
 };
 
+&pmk8280_rtc {
+	nvmem-cells = <&rtc_offset>;
+	nvmem-cell-names = "offset";
+
+	status = "okay";
+};
+
+&pmk8280_sdam_6 {
+	status = "okay";
+
+	rtc_offset: rtc-offset@bc {
+		reg = <0xbc 0x4>;
+	};
+};
+
 &pmk8280_vadc {
 	status = "okay";
 
-- 
2.39.1

Re: [PATCH 19/24] rtc: pm8xxx: add support for uefi offset

[Johan Hovold]

On Thu, Jan 26, 2023 at 03:20:52PM +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.
> 
> 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>

The Subject of this one was supposed to say "RFC" as it, like the
previous patch, is not intended to be merged just yet.

Johan

Re: [PATCH 15/24] dt-bindings: rtc: qcom-pm8xxx: add nvmem-cell offset

[Krzysztof Kozlowski]

On 26/01/2023 15:20, 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.
> 
> Add an 'offset' nvmem cell which can be used to store a 32-bit offset
> from the Unix epoch so that the RTC time can be updated on such
> platforms.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 17/24] rtc: pm8xxx: add copyright notice

[Alexandre Belloni]

On 26/01/2023 15:20:50+0100, Johan Hovold wrote:
> Add a copyright notice for Linaro and add myself as a (primary) author
> of this driver.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/rtc/rtc-pm8xxx.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> index 09816b9f6282..25bdd804b4d2 100644
> --- a/drivers/rtc/rtc-pm8xxx.c
> +++ b/drivers/rtc/rtc-pm8xxx.c
> @@ -1,5 +1,9 @@
>  // SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
> +/*
> + * pm8xxx RTC driver
> + *
> + * Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
> + * Copyright (c) 2023, Linaro Limited

Is this really useful? The authoritative source is going to be git
anyway.

>   */
>  #include <linux/of.h>
>  #include <linux/module.h>
> @@ -551,3 +555,4 @@ MODULE_ALIAS("platform:rtc-pm8xxx");
>  MODULE_DESCRIPTION("PMIC8xxx RTC driver");
>  MODULE_LICENSE("GPL v2");
>  MODULE_AUTHOR("Anirudh Ghayal <aghayal@codeaurora.org>");
> +MODULE_AUTHOR("Johan Hovold <johan@kernel.org>");
> -- 
> 2.39.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 17/24] rtc: pm8xxx: add copyright notice

[Johan Hovold]

On Thu, Jan 26, 2023 at 05:06:20PM +0100, Alexandre Belloni wrote:
> On 26/01/2023 15:20:50+0100, Johan Hovold wrote:
> > Add a copyright notice for Linaro and add myself as a (primary) author
> > of this driver.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/rtc/rtc-pm8xxx.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> > index 09816b9f6282..25bdd804b4d2 100644
> > --- a/drivers/rtc/rtc-pm8xxx.c
> > +++ b/drivers/rtc/rtc-pm8xxx.c
> > @@ -1,5 +1,9 @@
> >  // SPDX-License-Identifier: GPL-2.0-only
> > -/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
> > +/*
> > + * pm8xxx RTC driver
> > + *
> > + * Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
> > + * Copyright (c) 2023, Linaro Limited
> 
> Is this really useful? The authoritative source is going to be git
> anyway.

Sure, but in this case the driver ended up being almost completely
reworked so I think it is warranted.
 
> >   */
> >  #include <linux/of.h>
> >  #include <linux/module.h>
> > @@ -551,3 +555,4 @@ MODULE_ALIAS("platform:rtc-pm8xxx");
> >  MODULE_DESCRIPTION("PMIC8xxx RTC driver");
> >  MODULE_LICENSE("GPL v2");
> >  MODULE_AUTHOR("Anirudh Ghayal <aghayal@codeaurora.org>");
> > +MODULE_AUTHOR("Johan Hovold <johan@kernel.org>");
> > -- 
> > 2.39.1

Johan

Re: [PATCH 16/24] rtc: pm8xxx: add support for nvmem offset

[Srinivas Kandagatla]

On 26/01/2023 14:20, 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 Epoch in an nvmem cell
> so that the RTC time can be set on such platforms.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/rtc/rtc-pm8xxx.c | 134 +++++++++++++++++++++++++++++++++++----
>   1 file changed, 123 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> index 922aef0f0241..09816b9f6282 100644
> --- a/drivers/rtc/rtc-pm8xxx.c
> +++ b/drivers/rtc/rtc-pm8xxx.c
> @@ -3,6 +3,7 @@
>    */
> +static int pm8xxx_rtc_read_nvmem_offset(struct pm8xxx_rtc *rtc_dd)
> +{
> +	size_t len;
> +	void *buf;
> +	int rc;
> +
> +	buf = nvmem_cell_read(rtc_dd->nvmem_cell, &len);
> +	if (IS_ERR(buf)) {
> +		rc = PTR_ERR(buf);
> +		dev_err(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc);
> +		return rc;
> +	}
> +
> +	if (len != sizeof(u32)) {
> +		dev_err(rtc_dd->dev, "unexpected nvmem cell size %zu\n", len);
> +		kfree(buf);
> +		return -EINVAL;
> +	}
how about us nvmem_cell_read_u32()

> +
> +	rtc_dd->offset = get_unaligned_le32(buf);
> +
> +	kfree(buf);
> +
> +	return 0;
> +}
> +

Re: [PATCH 17/24] rtc: pm8xxx: add copyright notice

[Krzysztof Kozlowski]

On 27/01/2023 14:04, Johan Hovold wrote:
> On Thu, Jan 26, 2023 at 05:06:20PM +0100, Alexandre Belloni wrote:
>> On 26/01/2023 15:20:50+0100, Johan Hovold wrote:
>>> Add a copyright notice for Linaro and add myself as a (primary) author
>>> of this driver.
>>>
>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>>> ---
>>>  drivers/rtc/rtc-pm8xxx.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
>>> index 09816b9f6282..25bdd804b4d2 100644
>>> --- a/drivers/rtc/rtc-pm8xxx.c
>>> +++ b/drivers/rtc/rtc-pm8xxx.c
>>> @@ -1,5 +1,9 @@
>>>  // SPDX-License-Identifier: GPL-2.0-only
>>> -/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
>>> +/*
>>> + * pm8xxx RTC driver
>>> + *
>>> + * Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
>>> + * Copyright (c) 2023, Linaro Limited
>>
>> Is this really useful? The authoritative source is going to be git
>> anyway.
> 
> Sure, but in this case the driver ended up being almost completely
> reworked so I think it is warranted.

Copyright update is an effect of rework, not independent, thus it should
be squashed with the code doing the rework. Copyright updates on their
own do not make sense.

Best regards,
Krzysztof

Re: [PATCH 16/24] rtc: pm8xxx: add support for nvmem offset

[Alexandre Belloni]

On 26/01/2023 15:20:49+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 Epoch in an nvmem cell
> so that the RTC time can be set on such platforms.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/rtc/rtc-pm8xxx.c | 134 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 123 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> index 922aef0f0241..09816b9f6282 100644
> --- a/drivers/rtc/rtc-pm8xxx.c
> +++ b/drivers/rtc/rtc-pm8xxx.c
> @@ -3,6 +3,7 @@
>   */
>  #include <linux/of.h>
>  #include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
>  #include <linux/init.h>
>  #include <linux/rtc.h>
>  #include <linux/platform_device.h>
> @@ -49,6 +50,8 @@ struct pm8xxx_rtc_regs {
>   * @alarm_irq:		alarm irq number
>   * @regs:		register description
>   * @dev:		device structure
> + * @nvmem_cell:		nvmem cell for offset
> + * @offset:		offset from epoch in seconds
>   */
>  struct pm8xxx_rtc {
>  	struct rtc_device *rtc;
> @@ -57,8 +60,60 @@ struct pm8xxx_rtc {
>  	int alarm_irq;
>  	const struct pm8xxx_rtc_regs *regs;
>  	struct device *dev;
> +	struct nvmem_cell *nvmem_cell;
> +	u32 offset;
>  };
>  
> +static int pm8xxx_rtc_read_nvmem_offset(struct pm8xxx_rtc *rtc_dd)
> +{
> +	size_t len;
> +	void *buf;
> +	int rc;
> +
> +	buf = nvmem_cell_read(rtc_dd->nvmem_cell, &len);
> +	if (IS_ERR(buf)) {
> +		rc = PTR_ERR(buf);
> +		dev_err(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc);

You removed many dev_err strings in your previous patch and now this is
verbose. Honestly, there is not much to do apart from reying the
operation so I don't think the strings are worth it.

> +		return rc;
> +	}
> +
> +	if (len != sizeof(u32)) {
> +		dev_err(rtc_dd->dev, "unexpected nvmem cell size %zu\n", len);
> +		kfree(buf);
> +		return -EINVAL;
> +	}
> +
> +	rtc_dd->offset = get_unaligned_le32(buf);
> +
> +	kfree(buf);
> +
> +	return 0;
> +}
> +
> +static int pm8xxx_rtc_write_nvmem_offset(struct pm8xxx_rtc *rtc_dd, u32 offset)
> +{
> +	u8 buf[sizeof(u32)];
> +	int rc;
> +
> +	put_unaligned_le32(offset, buf);
> +
> +	rc = nvmem_cell_write(rtc_dd->nvmem_cell, buf, sizeof(buf));
> +	if (rc < 0) {
> +		dev_err(rtc_dd->dev, "failed to write nvmem offset: %d\n", rc);
> +		return rc;
> +	}
> +
> +	return 0;
> +}
> +
> +static int pm8xxx_rtc_read_offset(struct pm8xxx_rtc *rtc_dd)
> +{
> +	if (!rtc_dd->nvmem_cell)
> +		return 0;
> +
> +	return pm8xxx_rtc_read_nvmem_offset(rtc_dd);
> +}
> +
>  static int pm8xxx_rtc_read_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs)
>  {
>  	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
> @@ -90,6 +145,33 @@ static int pm8xxx_rtc_read_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs)
>  	return 0;
>  }
>  
> +static int pm8xxx_rtc_update_offset(struct pm8xxx_rtc *rtc_dd, u32 secs)
> +{
> +	u32 raw_secs;
> +	u32 offset;
> +	int rc;
> +
> +	if (!rtc_dd->nvmem_cell)
> +		return -ENODEV;
> +
> +	rc = pm8xxx_rtc_read_raw(rtc_dd, &raw_secs);
> +	if (rc)
> +		return rc;
> +
> +	offset = secs - raw_secs;
> +
> +	if (offset == rtc_dd->offset)
> +		return 0;
> +
> +	rc = pm8xxx_rtc_write_nvmem_offset(rtc_dd, offset);
> +	if (rc)
> +		return rc;
> +
> +	rtc_dd->offset = offset;
> +
> +	return 0;
> +}
> +
>  /*
>   * Steps to write the RTC registers.
>   * 1. Disable alarm if enabled.
> @@ -99,23 +181,15 @@ static int pm8xxx_rtc_read_raw(struct pm8xxx_rtc *rtc_dd, u32 *secs)
>   * 5. Enable rtc if disabled in step 2.
>   * 6. Enable alarm if disabled in step 1.
>   */
> -static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +static int __pm8xxx_rtc_set_time(struct pm8xxx_rtc *rtc_dd, u32 secs)
>  {
> -	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
>  	const struct pm8xxx_rtc_regs *regs = rtc_dd->regs;
>  	u8 value[NUM_8_BIT_RTC_REGS];
>  	bool alarm_enabled;
> -	u32 secs;
>  	int rc;
>  
> -	if (!rtc_dd->allow_set_time)
> -		return -ENODEV;
> -
> -	secs = rtc_tm_to_time64(tm);
>  	put_unaligned_le32(secs, value);
>  
> -	dev_dbg(dev, "set time: %ptRd %ptRt (%u)\n", tm, tm, secs);
> -
>  	rc = regmap_update_bits_check(rtc_dd->regmap, regs->alarm_ctrl,
>  				      regs->alarm_en, 0, &alarm_enabled);
>  	if (rc)
> @@ -158,6 +232,27 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  	return 0;
>  }
>  
> +static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
> +	u32 secs;
> +	int rc;
> +
> +	secs = rtc_tm_to_time64(tm);
> +
> +	if (rtc_dd->allow_set_time)
> +		rc = __pm8xxx_rtc_set_time(rtc_dd, secs);
> +	else
> +		rc = pm8xxx_rtc_update_offset(rtc_dd, secs);
> +
> +	if (rc)
> +		return rc;
> +
> +	dev_dbg(dev, "set time: %ptRd %ptRt (%u + %u)\n", tm, tm,
> +			secs - rtc_dd->offset, rtc_dd->offset);
> +	return 0;
> +}
> +
>  static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  {
>  	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
> @@ -168,10 +263,11 @@ static int pm8xxx_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  	if (rc)
>  		return rc;
>  
> +	secs += rtc_dd->offset;
>  	rtc_time64_to_tm(secs, tm);
>  
> -	dev_dbg(dev, "read time: %ptRd %ptRt (%u)\n", tm, tm, secs);
> -
> +	dev_dbg(dev, "read time: %ptRd %ptRt (%u + %u)\n", tm, tm,
> +			secs - rtc_dd->offset, rtc_dd->offset);
>  	return 0;
>  }
>  
> @@ -184,6 +280,7 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
>  	int rc;
>  
>  	secs = rtc_tm_to_time64(&alarm->time);
> +	secs -= rtc_dd->offset;
>  	put_unaligned_le32(secs, value);
>  
>  	rc = regmap_update_bits(rtc_dd->regmap, regs->alarm_ctrl,
> @@ -223,6 +320,7 @@ static int pm8xxx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
>  		return rc;
>  
>  	secs = get_unaligned_le32(value);
> +	secs += rtc_dd->offset;
>  	rtc_time64_to_tm(secs, &alarm->time);
>  
>  	rc = regmap_read(rtc_dd->regmap, regs->alarm_ctrl, &ctrl_reg);
> @@ -380,9 +478,23 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
>  	rtc_dd->allow_set_time = of_property_read_bool(pdev->dev.of_node,
>  						      "allow-set-time");
>  
> +	rtc_dd->nvmem_cell = devm_nvmem_cell_get(&pdev->dev, "offset");

Maybe we should get something more specific than just "offset" so this
could be parsed in the RTC core at some point (this is the second RTC to
behave like this)

> +	if (IS_ERR(rtc_dd->nvmem_cell)) {
> +		rc = PTR_ERR(rtc_dd->nvmem_cell);
> +		if (rc != -ENOENT)
> +			return rc;
> +		rtc_dd->nvmem_cell = NULL;
> +	}
> +
>  	rtc_dd->regs = match->data;
>  	rtc_dd->dev = &pdev->dev;
>  
> +	if (!rtc_dd->allow_set_time) {
> +		rc = pm8xxx_rtc_read_offset(rtc_dd);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	rc = pm8xxx_rtc_enable(rtc_dd);
>  	if (rc)
>  		return rc;
> -- 
> 2.39.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 19/24] rtc: pm8xxx: add support for uefi offset

[Alexandre Belloni]

On 26/01/2023 15:20:52+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 are you using the GPS epoch? This seems pretty random.

> 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>
> ---
>  drivers/rtc/rtc-pm8xxx.c | 122 +++++++++++++++++++++++++++++++++++++--
>  include/linux/rtc.h      |   1 +
>  2 files changed, 119 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> index 25bdd804b4d2..6c2324baeec6 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>
> @@ -17,6 +18,7 @@
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  
> +#include <asm/byteorder.h>
>  #include <asm/unaligned.h>
>  
>  /* RTC_CTRL register bit fields */
> @@ -46,6 +48,11 @@ 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
> @@ -54,6 +61,7 @@ struct pm8xxx_rtc_regs {
>   * @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 +69,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_err(dev, "failed to read UEFI offset: %lu\n", status);
> +		return efi_status_to_err(status);
> +	}
> +
> +	if (size != sizeof(*rtc_info)) {
> +		dev_err(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_BEGIN_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_BEGIN_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_err(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 +208,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 +254,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 +266,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;
>  
> @@ -488,6 +591,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 1fd9c6a21ebe..1ecee2fe4214 100644
> --- a/include/linux/rtc.h
> +++ b/include/linux/rtc.h
> @@ -169,6 +169,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_BEGIN_GPS		315964800LL /* 1980-01-06 00:00:00 */

I'd use RTC_TIMESTAMP_EPOCH_GPS but really, I would prefer the UNIX epoch
to be used

>  #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.39.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 19/24] rtc: pm8xxx: add support for uefi offset

[Johan Hovold]

On Fri, Jan 27, 2023 at 04:19:18PM +0100, Alexandre Belloni wrote:
> On 26/01/2023 15:20:52+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 are you using the GPS epoch? This seems pretty random.

Tell that to the Qualcomm firmware team. ;)

Perhaps I could have made it more clear, but this is the format that the
firmware uses so Linux is not free to pick a different base here (or
time would differ ten years between UEFI/Windows and Linux).

> > 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.

Johan

Re: [PATCH 16/24] rtc: pm8xxx: add support for nvmem offset

[Johan Hovold]

On Fri, Jan 27, 2023 at 02:13:21PM +0000, Srinivas Kandagatla wrote:
> On 26/01/2023 14:20, 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 Epoch in an nvmem cell
> > so that the RTC time can be set on such platforms.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >   drivers/rtc/rtc-pm8xxx.c | 134 +++++++++++++++++++++++++++++++++++----
> >   1 file changed, 123 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> > index 922aef0f0241..09816b9f6282 100644
> > --- a/drivers/rtc/rtc-pm8xxx.c
> > +++ b/drivers/rtc/rtc-pm8xxx.c
> > @@ -3,6 +3,7 @@
> >    */
> > +static int pm8xxx_rtc_read_nvmem_offset(struct pm8xxx_rtc *rtc_dd)
> > +{
> > +	size_t len;
> > +	void *buf;
> > +	int rc;
> > +
> > +	buf = nvmem_cell_read(rtc_dd->nvmem_cell, &len);
> > +	if (IS_ERR(buf)) {
> > +		rc = PTR_ERR(buf);
> > +		dev_err(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc);
> > +		return rc;
> > +	}
> > +
> > +	if (len != sizeof(u32)) {
> > +		dev_err(rtc_dd->dev, "unexpected nvmem cell size %zu\n", len);
> > +		kfree(buf);
> > +		return -EINVAL;
> > +	}

> how about us nvmem_cell_read_u32()

I considered that, but did not like the asymmetry of the interface.

Specifically, nvmem_cell_read_u32() would go out and look up the nvmem
cell again even though I already have and need a reference for
nvmem_cell_write().

nvmem_cell_read_u32() seems to be a better fit as a convenience wrapper
for drivers that only need to do a single read at probe.

Johan

Re: [PATCH 16/24] rtc: pm8xxx: add support for nvmem offset

[Johan Hovold]

On Fri, Jan 27, 2023 at 04:09:54PM +0100, Alexandre Belloni wrote:
> On 26/01/2023 15:20:49+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 Epoch in an nvmem cell
> > so that the RTC time can be set on such platforms.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/rtc/rtc-pm8xxx.c | 134 +++++++++++++++++++++++++++++++++++----
> >  1 file changed, 123 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> > index 922aef0f0241..09816b9f6282 100644
> > --- a/drivers/rtc/rtc-pm8xxx.c
> > +++ b/drivers/rtc/rtc-pm8xxx.c
> > @@ -3,6 +3,7 @@
> >   */
> >  #include <linux/of.h>
> >  #include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> >  #include <linux/init.h>
> >  #include <linux/rtc.h>
> >  #include <linux/platform_device.h>
> > @@ -49,6 +50,8 @@ struct pm8xxx_rtc_regs {
> >   * @alarm_irq:		alarm irq number
> >   * @regs:		register description
> >   * @dev:		device structure
> > + * @nvmem_cell:		nvmem cell for offset
> > + * @offset:		offset from epoch in seconds
> >   */
> >  struct pm8xxx_rtc {
> >  	struct rtc_device *rtc;
> > @@ -57,8 +60,60 @@ struct pm8xxx_rtc {
> >  	int alarm_irq;
> >  	const struct pm8xxx_rtc_regs *regs;
> >  	struct device *dev;
> > +	struct nvmem_cell *nvmem_cell;
> > +	u32 offset;
> >  };
> >  
> > +static int pm8xxx_rtc_read_nvmem_offset(struct pm8xxx_rtc *rtc_dd)
> > +{
> > +	size_t len;
> > +	void *buf;
> > +	int rc;
> > +
> > +	buf = nvmem_cell_read(rtc_dd->nvmem_cell, &len);
> > +	if (IS_ERR(buf)) {
> > +		rc = PTR_ERR(buf);
> > +		dev_err(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc);
> 
> You removed many dev_err strings in your previous patch and now this is
> verbose. Honestly, there is not much to do apart from reying the
> operation so I don't think the strings are worth it.

There's a difference. The SPMI ones are basically equivalent to mmio
reads, which we also don't expect to fail (and other spmi drivers also
ignore them).

These nvmem error paths I actually hit during development and it could
help someone trying to enable this feature on a new platform.
 
> > +		return rc;
> > +	}
> > +
> > +	if (len != sizeof(u32)) {
> > +		dev_err(rtc_dd->dev, "unexpected nvmem cell size %zu\n", len);
> > +		kfree(buf);
> > +		return -EINVAL;
> > +	}
> > +
> > +	rtc_dd->offset = get_unaligned_le32(buf);
> > +
> > +	kfree(buf);
> > +
> > +	return 0;
> > +}

> > @@ -380,9 +478,23 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
> >  	rtc_dd->allow_set_time = of_property_read_bool(pdev->dev.of_node,
> >  						      "allow-set-time");
> >  
> > +	rtc_dd->nvmem_cell = devm_nvmem_cell_get(&pdev->dev, "offset");
> 
> Maybe we should get something more specific than just "offset" so this
> could be parsed in the RTC core at some point (this is the second RTC to
> behave like this)

Yes, that thought crossed my mind, but it's an nvmem cell name (label)
and not a generic devicetree property. If you look at the binding
document I think the name makes sense given the current description, and
I'm not sure changing to something like 'base' would be much of an
improvement.

I also don't expect there to be more broken RTCs out there like these
ones. Hopefully Qualcomm will even get this fixed at some point
themselves.

And I assume you were think of the old Atmel driver which uses a timer
counter and a scratch register as a base? That one is also a bit
different in that the timer can be reset, just not set.

> > +	if (IS_ERR(rtc_dd->nvmem_cell)) {
> > +		rc = PTR_ERR(rtc_dd->nvmem_cell);
> > +		if (rc != -ENOENT)
> > +			return rc;
> > +		rtc_dd->nvmem_cell = NULL;
> > +	}

Johan

Re: [PATCH 19/24] rtc: pm8xxx: add support for uefi offset

[Alexandre Belloni]

On 27/01/2023 16:26:26+0100, Johan Hovold wrote:
> On Fri, Jan 27, 2023 at 04:19:18PM +0100, Alexandre Belloni wrote:
> > On 26/01/2023 15:20:52+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 are you using the GPS epoch? This seems pretty random.
> 
> Tell that to the Qualcomm firmware team. ;)
> 
> Perhaps I could have made it more clear, but this is the format that the
> firmware uses so Linux is not free to pick a different base here (or
> time would differ ten years between UEFI/Windows and Linux).

I expected this answer so please add this to the commit message, this
will be the fifth epoch we ave to handle then...

> 
> > > 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.
> 
> Johan

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 16/24] rtc: pm8xxx: add support for nvmem offset

[Alexandre Belloni]

On 27/01/2023 16:51:27+0100, Johan Hovold wrote:
> > > +static int pm8xxx_rtc_read_nvmem_offset(struct pm8xxx_rtc *rtc_dd)
> > > +{
> > > +	size_t len;
> > > +	void *buf;
> > > +	int rc;
> > > +
> > > +	buf = nvmem_cell_read(rtc_dd->nvmem_cell, &len);
> > > +	if (IS_ERR(buf)) {
> > > +		rc = PTR_ERR(buf);
> > > +		dev_err(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc);
> > 
> > You removed many dev_err strings in your previous patch and now this is
> > verbose. Honestly, there is not much to do apart from reying the
> > operation so I don't think the strings are worth it.
> 
> There's a difference. The SPMI ones are basically equivalent to mmio
> reads, which we also don't expect to fail (and other spmi drivers also
> ignore them).
> 
> These nvmem error paths I actually hit during development and it could
> help someone trying to enable this feature on a new platform.
>  

then consider using dev_dbg, the end user will never see or act on those
error messages anyway. I'm on a quest to cut down the number of strings
in the kernel, at least in the RTC subsystem ;)

> > > +		return rc;
> > > +	}
> > > +
> > > +	if (len != sizeof(u32)) {
> > > +		dev_err(rtc_dd->dev, "unexpected nvmem cell size %zu\n", len);
> > > +		kfree(buf);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	rtc_dd->offset = get_unaligned_le32(buf);
> > > +
> > > +	kfree(buf);
> > > +
> > > +	return 0;
> > > +}
> 
> > > @@ -380,9 +478,23 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
> > >  	rtc_dd->allow_set_time = of_property_read_bool(pdev->dev.of_node,
> > >  						      "allow-set-time");
> > >  
> > > +	rtc_dd->nvmem_cell = devm_nvmem_cell_get(&pdev->dev, "offset");
> > 
> > Maybe we should get something more specific than just "offset" so this
> > could be parsed in the RTC core at some point (this is the second RTC to
> > behave like this)
> 
> Yes, that thought crossed my mind, but it's an nvmem cell name (label)
> and not a generic devicetree property. If you look at the binding
> document I think the name makes sense given the current description, and
> I'm not sure changing to something like 'base' would be much of an
> improvement.
> 
> I also don't expect there to be more broken RTCs out there like these
> ones. Hopefully Qualcomm will even get this fixed at some point
> themselves.
> 
> And I assume you were think of the old Atmel driver which uses a timer
> counter and a scratch register as a base? That one is also a bit
> different in that the timer can be reset, just not set.

Nope, I'm thinking about the gamecube one and probably the nintendo
switch one which seems to behave similarly (no driver in the kernel
though).

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC 18/24] dt-bindings: rtc: qcom-pm8xxx: add uefi-variable offset

[Rob Herring]

On Thu, Jan 26, 2023 at 03:20:51PM +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 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.

Can't you just try to read the UEFI variable and use it if that 
succeeds?

I don't like this in DT because what if lots of devices start storing 
lots of things in vendor specific UEFI variables. It doesn't scale.

Rob

Re: [RFC 18/24] dt-bindings: rtc: qcom-pm8xxx: add uefi-variable offset

[Johan Hovold]

[ +CC: Ard ]

On Mon, Jan 30, 2023 at 12:49:44PM -0600, Rob Herring wrote:
> On Thu, Jan 26, 2023 at 03:20:51PM +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 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.
> 
> Can't you just try to read the UEFI variable and use it if that 
> succeeds?

Generally, yes. The problem here is that this UEFI variable is not used
on all devices using these PMICs and I need a way to determine whether
to wait for the UEFI variables to become available or not (e.g. when
efivars support is built as module, yes, that's a thing now...).

> I don't like this in DT because what if lots of devices start storing 
> lots of things in vendor specific UEFI variables. It doesn't scale.

I hope we won't see that even if we already have some devices for x86
platforms storing MAC addresses and such in UEFI variables. They
currently access the UEFI firmware directly (i.e. not using the efivars
abstraction) and simply assume UEFI is always there.

With the Google SMI efivars implementation or the new Qualcomm SMC-based
one, we need a way to determine whether to wait for efivars to become
registered. For drivers where efivars is always needed we can just probe
defer, but in this case we should not wait unless the DT indicates that
the RTC offset is stored in UEFI on this particular machine.

Just as the nvmem-cell property indicates that the offset is stored in
some abstract nvmem, it seems reasonable to describe the offset being
stored in UEFI when that is the case (even if it is indeed generally
possible to probe for the latter).

An alternative might be to describe the efivars fw dependency in DT too
(e.g. for device links), but I believe you have already expressed some
concerns over that:

	https://lore.kernel.org/lkml/20230130210530.GA3339716-robh@kernel.org/

Johan

Re: [PATCH 17/24] rtc: pm8xxx: add copyright notice

[Johan Hovold]

On Fri, Jan 27, 2023 at 03:14:48PM +0100, Krzysztof Kozlowski wrote:
> On 27/01/2023 14:04, Johan Hovold wrote:
> > On Thu, Jan 26, 2023 at 05:06:20PM +0100, Alexandre Belloni wrote:
> >> On 26/01/2023 15:20:50+0100, Johan Hovold wrote:
> >>> Add a copyright notice for Linaro and add myself as a (primary) author
> >>> of this driver.
> >>>
> >>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> >>> ---
> >>>  drivers/rtc/rtc-pm8xxx.c | 7 ++++++-
> >>>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> >>> index 09816b9f6282..25bdd804b4d2 100644
> >>> --- a/drivers/rtc/rtc-pm8xxx.c
> >>> +++ b/drivers/rtc/rtc-pm8xxx.c
> >>> @@ -1,5 +1,9 @@
> >>>  // SPDX-License-Identifier: GPL-2.0-only
> >>> -/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
> >>> +/*
> >>> + * pm8xxx RTC driver
> >>> + *
> >>> + * Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
> >>> + * Copyright (c) 2023, Linaro Limited
> >>
> >> Is this really useful? The authoritative source is going to be git
> >> anyway.
> > 
> > Sure, but in this case the driver ended up being almost completely
> > reworked so I think it is warranted.
> 
> Copyright update is an effect of rework, not independent, thus it should
> be squashed with the code doing the rework. Copyright updates on their
> own do not make sense.

I tend to agree, but there can be reasons for adding it separately as in
this case where I wanted to highlight that I was adding a new entry. And
it is still added along with the changes (i.e. as part of the series).

But I'll squash it with the previous patch for v2.

Johan

Re: [PATCH 16/24] rtc: pm8xxx: add support for nvmem offset

[Johan Hovold]

On Fri, Jan 27, 2023 at 05:05:03PM +0100, Alexandre Belloni wrote:
> On 27/01/2023 16:51:27+0100, Johan Hovold wrote:

> > > > @@ -380,9 +478,23 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
> > > >  	rtc_dd->allow_set_time = of_property_read_bool(pdev->dev.of_node,
> > > >  						      "allow-set-time");
> > > >  
> > > > +	rtc_dd->nvmem_cell = devm_nvmem_cell_get(&pdev->dev, "offset");
> > > 
> > > Maybe we should get something more specific than just "offset" so this
> > > could be parsed in the RTC core at some point (this is the second RTC to
> > > behave like this)
> > 
> > Yes, that thought crossed my mind, but it's an nvmem cell name (label)
> > and not a generic devicetree property. If you look at the binding
> > document I think the name makes sense given the current description, and
> > I'm not sure changing to something like 'base' would be much of an
> > improvement.
> > 
> > I also don't expect there to be more broken RTCs out there like these
> > ones. Hopefully Qualcomm will even get this fixed at some point
> > themselves.
> > 
> > And I assume you were think of the old Atmel driver which uses a timer
> > counter and a scratch register as a base? That one is also a bit
> > different in that the timer can be reset, just not set.
> 
> Nope, I'm thinking about the gamecube one and probably the nintendo
> switch one which seems to behave similarly (no driver in the kernel
> though).

Found the gamecube one now (misread you comment above to imply that it
was also out of tree).

That one is also different in that the timer in that RTC can also be
set (e.g. like the atmel one), but for consistency with some firmware an
offset also needs to be read from SRAM (not NVRAM) and applied. That
offset is also never updated by Linux.

Johan

Re: [PATCH 16/24] rtc: pm8xxx: add support for nvmem offset

[Alexandre Belloni]

On 02/02/2023 16:12:33+0100, Johan Hovold wrote:
> On Fri, Jan 27, 2023 at 05:05:03PM +0100, Alexandre Belloni wrote:
> > On 27/01/2023 16:51:27+0100, Johan Hovold wrote:
> 
> > > > > @@ -380,9 +478,23 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
> > > > >  	rtc_dd->allow_set_time = of_property_read_bool(pdev->dev.of_node,
> > > > >  						      "allow-set-time");
> > > > >  
> > > > > +	rtc_dd->nvmem_cell = devm_nvmem_cell_get(&pdev->dev, "offset");
> > > > 
> > > > Maybe we should get something more specific than just "offset" so this
> > > > could be parsed in the RTC core at some point (this is the second RTC to
> > > > behave like this)
> > > 
> > > Yes, that thought crossed my mind, but it's an nvmem cell name (label)
> > > and not a generic devicetree property. If you look at the binding
> > > document I think the name makes sense given the current description, and
> > > I'm not sure changing to something like 'base' would be much of an
> > > improvement.
> > > 
> > > I also don't expect there to be more broken RTCs out there like these
> > > ones. Hopefully Qualcomm will even get this fixed at some point
> > > themselves.
> > > 
> > > And I assume you were think of the old Atmel driver which uses a timer
> > > counter and a scratch register as a base? That one is also a bit
> > > different in that the timer can be reset, just not set.
> > 
> > Nope, I'm thinking about the gamecube one and probably the nintendo
> > switch one which seems to behave similarly (no driver in the kernel
> > though).
> 
> Found the gamecube one now (misread you comment above to imply that it
> was also out of tree).
> 
> That one is also different in that the timer in that RTC can also be
> set (e.g. like the atmel one), but for consistency with some firmware an
> offset also needs to be read from SRAM (not NVRAM) and applied. That
> offset is also never updated by Linux.

Yeah, I deally, the gamecube counter shouldn't be updated, the switch
one doesn't seem to be updatable. I guess the idea being that games need
to be able to know if you are messing with the RTC to get an advantage.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RFC 18/24] dt-bindings: rtc: qcom-pm8xxx: add uefi-variable offset

[Ard Biesheuvel]

On Wed, 1 Feb 2023 at 17:09, Johan Hovold <johan@kernel.org> wrote:
>
> [ +CC: Ard ]
>
> On Mon, Jan 30, 2023 at 12:49:44PM -0600, Rob Herring wrote:
> > On Thu, Jan 26, 2023 at 03:20:51PM +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 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.
> >
> > Can't you just try to read the UEFI variable and use it if that
> > succeeds?
>
> Generally, yes. The problem here is that this UEFI variable is not used
> on all devices using these PMICs and I need a way to determine whether
> to wait for the UEFI variables to become available or not (e.g. when
> efivars support is built as module, yes, that's a thing now...).
>

Could we read this variable at boot and pass it to the kernel in a
different way? That way, we only have to defer the ability to set the
RTC, right?

> > I don't like this in DT because what if lots of devices start storing
> > lots of things in vendor specific UEFI variables. It doesn't scale.
>
> I hope we won't see that even if we already have some devices for x86
> platforms storing MAC addresses and such in UEFI variables. They
> currently access the UEFI firmware directly (i.e. not using the efivars
> abstraction) and simply assume UEFI is always there.
>
> With the Google SMI efivars implementation or the new Qualcomm SMC-based
> one, we need a way to determine whether to wait for efivars to become
> registered. For drivers where efivars is always needed we can just probe
> defer, but in this case we should not wait unless the DT indicates that
> the RTC offset is stored in UEFI on this particular machine.
>
> Just as the nvmem-cell property indicates that the offset is stored in
> some abstract nvmem, it seems reasonable to describe the offset being
> stored in UEFI when that is the case (even if it is indeed generally
> possible to probe for the latter).
>
> An alternative might be to describe the efivars fw dependency in DT too
> (e.g. for device links), but I believe you have already expressed some
> concerns over that:
>
>         https://lore.kernel.org/lkml/20230130210530.GA3339716-robh@kernel.org/
>
> Johan