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

[PATCH v2 00/22] 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 I have does not allow updating the UEFI RTC offset even if this
works on the X13s (and on a second CRD with more recent firmware I
learned last week).

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
as an RFC in v1 of this series for reference. In case it turns out there
are systems out there were this could be used (e.g. the Lenovo Yoga C630
laptop) 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 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


Changes in v2
 - replace nvme operation dev_err() with dev_dbg() (Alexandre)
 - squash patch adding copyright entry with patch adding nvme support
   (Krzysztof)
 - drop last two dev_err() (new patch)
 - amend dts commit message as setting variables appears to work on some
   CRD
 - drop the two UEFI RFC patches for now due to one EFI dependency not
   yet in mainline and on-going binding discussion

Johan Hovold (22):
  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
  rtc: pm8xxx: drop error messages
  dt-bindings: rtc: qcom-pm8xxx: add nvmem-cell offset
  rtc: pm8xxx: add support for nvmem 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         |  12 +
 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                      | 533 +++++++++---------
 6 files changed, 324 insertions(+), 270 deletions(-)

-- 
2.39.1

[PATCH v2 01/22] 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

Re: [PATCH v2 01/22] rtc: pm8xxx: fix set-alarm race

[David Collins]

On 2/2/23 07:54, Johan Hovold wrote:
> Make sure to disable the alarm before updating the four alarm time
> registers to avoid spurious alarms during the update.

What scenario can encounter a spurious alarm triggering upon writing the
new alarm time inside of pm8xxx_rtc_set_alarm()?

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

What scenario shows the IRQ race issue that you mentioned?  How does not
protecting this register write with a lock avoid the race condition?

> 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(-)

Note that since locking is removed later in the patch series, my
questions above are mainly for the sake of curiosity.


Reviewed-by: David Collins <quic_collinsd@quicinc.com>

Thanks,
David

Re: [PATCH v2 01/22] rtc: pm8xxx: fix set-alarm race

[Johan Hovold]

On Mon, Feb 06, 2023 at 07:12:43PM -0800, David Collins wrote:
> On 2/2/23 07:54, Johan Hovold wrote:
> > Make sure to disable the alarm before updating the four alarm time
> > registers to avoid spurious alarms during the update.
> 
> What scenario can encounter a spurious alarm triggering upon writing the
> new alarm time inside of pm8xxx_rtc_set_alarm()?

The alarm is stored in four bytes in little-endian order. Consider
having had an alarm set and expired at:

	00 01 00 00

and now you want to set an alarm at

	01 02 00 00

Unless the alarm is disabled before the update the alarm could go off at

	01 01 00 00

after updating the first byte.
 
> > 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.
> 
> What scenario shows the IRQ race issue that you mentioned?  How does not
> protecting this register write with a lock avoid the race condition?

If a previously set alarm goes off after disabling interrupts but before
disabling the alarm inside the critical section, then that interrupt
could be serviced as soon as interrupts are re-enabled and the handler
would disable the newly set alarm.

> > 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(-)
> 
> Note that since locking is removed later in the patch series, my
> questions above are mainly for the sake of curiosity.

Johan

Re: [PATCH v2 01/22] rtc: pm8xxx: fix set-alarm race

[Johan Hovold]

On Tue, Feb 07, 2023 at 04:13:36PM +0100, Johan Hovold wrote:
> On Mon, Feb 06, 2023 at 07:12:43PM -0800, David Collins wrote:
> > On 2/2/23 07:54, Johan Hovold wrote:
> > > Make sure to disable the alarm before updating the four alarm time
> > > registers to avoid spurious alarms during the update.
> > 
> > What scenario can encounter a spurious alarm triggering upon writing the
> > new alarm time inside of pm8xxx_rtc_set_alarm()?
> 
> The alarm is stored in four bytes in little-endian order. Consider
> having had an alarm set and expired at:

This was just supposed to say "Consider having an alarm set at:" as the
alarm must still be enabled. Let me update the example I gave:

Consider having an alarm set at
 
 	10 01 00 00

and now you want to set an alarm at

 	01 02 00 00
 
Unless the alarm is disabled before the update the alarm could go off at
 
 	01 01 00 00
 
after updating the first byte.

Johan

[PATCH v2 02/22] 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 v2 03/22] 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

Re: [PATCH v2 03/22] rtc: pm8xxx: use regmap_update_bits()

[David Collins]

On 2/2/23 07:54, Johan Hovold wrote:
> 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(-)

Reviewed-by: David Collins <quic_collinsd@quicinc.com>


> 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;
> -	}
> -

I like this usage of regmap_update_bits_check().  It is a great helper
to use in this situation.  It really helps to simplify the logic.

Take care,
David

[PATCH v2 04/22] 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

Re: [PATCH v2 04/22] rtc: pm8xxx: drop bogus locking

[David Collins]

On 2/2/23 07:54, Johan Hovold wrote:
> 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(-)

Reviewed-by: David Collins <quic_collinsd@quicinc.com>

Take care,
David

[PATCH v2 05/22] 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 v2 06/22] 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

Re: [PATCH v2 06/22] rtc: pm8xxx: drop unused register defines

[David Collins]

On 2/2/23 07:54, Johan Hovold wrote:
> Drop the original register defines which have been used since commit

s/used/unused/

> 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(-)

Assuming that the minor commit text comment above is addressed:

Reviewed-by: David Collins <quic_collinsd@quicinc.com>

Take care,
David

Re: [PATCH v2 06/22] rtc: pm8xxx: drop unused register defines

[Johan Hovold]

On Mon, Feb 06, 2023 at 07:15:02PM -0800, David Collins wrote:
> On 2/2/23 07:54, Johan Hovold wrote:
> > Drop the original register defines which have been used since commit
> 
> s/used/unused/

Good catch. Perhaps Alexandre can amend the commit message when
applying.

It should be clear from Subject and the diff that this was a typo so I
don't think this alone is worth resending the series for.

> > 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(-)
> 
> Assuming that the minor commit text comment above is addressed:
> 
> Reviewed-by: David Collins <quic_collinsd@quicinc.com>

Johan

[PATCH v2 07/22] 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

Re: [PATCH v2 07/22] rtc: pm8xxx: use unaligned le32 helpers

[David Collins]

On 2/2/23 07:54, Johan Hovold wrote:
> 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(-)

Reviewed-by: David Collins <quic_collinsd@quicinc.com>

Take care,
David

[PATCH v2 08/22] 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

Re: [PATCH v2 08/22] rtc: pm8xxx: clean up time and alarm debugging

[David Collins]

On 2/2/23 07:54, Johan Hovold wrote:
> 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(-)

Reviewed-by: David Collins <quic_collinsd@quicinc.com>

Take care,
David

[PATCH v2 09/22] 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

Re: [PATCH v2 09/22] rtc: pm8xxx: rename struct device pointer

[David Collins]

On 2/2/23 07:54, Johan Hovold wrote:
> 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(-)

Reviewed-by: David Collins <quic_collinsd@quicinc.com>

Take care,
David

[PATCH v2 10/22] 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

Re: [PATCH v2 10/22] rtc: pm8xxx: rename alarm irq variable

[David Collins]

On 2/2/23 07:54, Johan Hovold wrote:
> 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(-)

Reviewed-by: David Collins <quic_collinsd@quicinc.com>

Take care,
David

[PATCH v2 11/22] 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

Re: [PATCH v2 11/22] rtc: pm8xxx: clean up comments

[David Collins]

On 2/2/23 07:54, Johan Hovold wrote:
> 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(-)

Reviewed-by: David Collins <quic_collinsd@quicinc.com>

Take care,
David

[PATCH v2 12/22] 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 v2 13/22] 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

Re: [PATCH v2 13/22] rtc: pm8xxx: refactor read_time()

[David Collins]

On 2/2/23 07:54, Johan Hovold wrote:
> 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)

I think that pm8xxx_rtc_read_time_raw() might be a better name for this
function to avoid any possible confusion if it is being used to read the
RTC time or the alarm time.

The patch looks good to me otherwise.

Take care,
David

Re: [PATCH v2 13/22] rtc: pm8xxx: refactor read_time()

[Johan Hovold]

On Mon, Feb 06, 2023 at 07:16:09PM -0800, David Collins wrote:
> On 2/2/23 07:54, Johan Hovold wrote:
> > 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)
> 
> I think that pm8xxx_rtc_read_time_raw() might be a better name for this
> function to avoid any possible confusion if it is being used to read the
> RTC time or the alarm time.

Thanks, that's a good suggestion. I don't think there's any real risk
for confusion here, but if I'm sending a v3 I can rename this one.

> The patch looks good to me otherwise.

Johan

[PATCH v2 14/22] 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

Re: [PATCH v2 14/22] rtc: pm8xxx: clean up local declarations

[David Collins]

On 2/2/23 07:54, Johan Hovold wrote:
> 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(-)

Reviewed-by: David Collins <quic_collinsd@quicinc.com>

Take care,
David

[PATCH v2 15/22] rtc: pm8xxx: drop error messages

[Johan Hovold]

For consistency with the rest of the driver, drop the last two error
messages for conditions that should only occur during development, if
ever.

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

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 922aef0f0241..eff2782beeed 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -368,10 +368,8 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	rtc_dd->regmap = dev_get_regmap(pdev->dev.parent, NULL);
-	if (!rtc_dd->regmap) {
-		dev_err(&pdev->dev, "Parent regmap unavailable.\n");
+	if (!rtc_dd->regmap)
 		return -ENXIO;
-	}
 
 	rtc_dd->alarm_irq = platform_get_irq(pdev, 0);
 	if (rtc_dd->alarm_irq < 0)
@@ -402,10 +400,8 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
 					  pm8xxx_alarm_trigger,
 					  IRQF_TRIGGER_RISING,
 					  "pm8xxx_rtc_alarm", rtc_dd);
-	if (rc < 0) {
-		dev_err(&pdev->dev, "Request IRQ failed (%d)\n", rc);
+	if (rc < 0)
 		return rc;
-	}
 
 	rc = devm_rtc_register_device(rtc_dd->rtc);
 	if (rc)
-- 
2.39.1

[PATCH v2 16/22] 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.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
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

Re: [PATCH v2 16/22] dt-bindings: rtc: qcom-pm8xxx: add nvmem-cell offset

[David Collins]

On 2/2/23 07:54, 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

s/can not/cannot/

> 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.
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  .../devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml     | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

Assuming that the minor commit text comment above is addressed:

Reviewed-by: David Collins <quic_collinsd@quicinc.com>

Take care,
David

Re: [PATCH v2 16/22] dt-bindings: rtc: qcom-pm8xxx: add nvmem-cell offset

[Johan Hovold]

On Mon, Feb 06, 2023 at 07:16:37PM -0800, David Collins wrote:
> On 2/2/23 07:54, 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
> 
> s/can not/cannot/

As far as I can tell, 'can not' is still correct even if it's less
commonly used this way compared to 'cannot'.

> > 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.
> > 
> > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  .../devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml     | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> 
> Assuming that the minor commit text comment above is addressed:

I'm definitely not resending because of this, but I'll keep it in mind
for future patches. Thanks.

> Reviewed-by: David Collins <quic_collinsd@quicinc.com>

Johan

Re: [PATCH v2 16/22] dt-bindings: rtc: qcom-pm8xxx: add nvmem-cell offset

[Alexandre Belloni]

On 02/02/2023 16:54:42+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 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.
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 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

The patch doesn't apply because this part of the context is not
upstream. Can you rebase?

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

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

Re: [PATCH v2 16/22] dt-bindings: rtc: qcom-pm8xxx: add nvmem-cell offset

[Johan Hovold]

On Fri, Feb 10, 2023 at 11:48:08PM +0100, Alexandre Belloni wrote:
> On 02/02/2023 16:54:42+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 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.
> > 
> > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > 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
> 
> The patch doesn't apply because this part of the context is not
> upstream. Can you rebase?

Ah, sorry about that. That's because of commit 51b3802e7960
("dt-bindings: rtc: qcom-pm8xxx: allow 'wakeup-source' property") which
is now in Linus's tree (and your rtc-fixes branch).

Do you still want me to rebase or do you prefer to handle the conflict
some other way?

Johan

Re: [PATCH v2 16/22] dt-bindings: rtc: qcom-pm8xxx: add nvmem-cell offset

[Alexandre Belloni]

On 11/02/2023 09:22:54+0100, Johan Hovold wrote:
> On Fri, Feb 10, 2023 at 11:48:08PM +0100, Alexandre Belloni wrote:
> > On 02/02/2023 16:54:42+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 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.
> > > 
> > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > 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
> > 
> > The patch doesn't apply because this part of the context is not
> > upstream. Can you rebase?
> 
> Ah, sorry about that. That's because of commit 51b3802e7960
> ("dt-bindings: rtc: qcom-pm8xxx: allow 'wakeup-source' property") which
> is now in Linus's tree (and your rtc-fixes branch).
> 
> Do you still want me to rebase or do you prefer to handle the conflict
> some other way?

Ah yes, my bad, I'll merge rtc-fixes in rtc-next before applying


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

Re: [PATCH v2 16/22] dt-bindings: rtc: qcom-pm8xxx: add nvmem-cell offset

[Johan Hovold]

On Sat, Feb 11, 2023 at 05:44:39PM +0100, Alexandre Belloni wrote:
> On 11/02/2023 09:22:54+0100, Johan Hovold wrote:
> > On Fri, Feb 10, 2023 at 11:48:08PM +0100, Alexandre Belloni wrote:
> > > On 02/02/2023 16:54:42+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 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.
> > > > 
> > > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

> > > The patch doesn't apply because this part of the context is not
> > > upstream. Can you rebase?
> > 
> > Ah, sorry about that. That's because of commit 51b3802e7960
> > ("dt-bindings: rtc: qcom-pm8xxx: allow 'wakeup-source' property") which
> > is now in Linus's tree (and your rtc-fixes branch).
> > 
> > Do you still want me to rebase or do you prefer to handle the conflict
> > some other way?
> 
> Ah yes, my bad, I'll merge rtc-fixes in rtc-next before applying

Sorry about reminding so soon, but with the merge window approaching
fast, will you be able to get this merged for 6.3?

Johan

Re: [PATCH v2 16/22] dt-bindings: rtc: qcom-pm8xxx: add nvmem-cell offset

[Johan Hovold]

Hi Alexandre,

On Wed, Feb 15, 2023 at 05:51:20PM +0100, Johan Hovold wrote:
> On Sat, Feb 11, 2023 at 05:44:39PM +0100, Alexandre Belloni wrote:
> > On 11/02/2023 09:22:54+0100, Johan Hovold wrote:
> > > On Fri, Feb 10, 2023 at 11:48:08PM +0100, Alexandre Belloni wrote:
> > > > On 02/02/2023 16:54:42+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 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.
> > > > > 
> > > > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> > > > The patch doesn't apply because this part of the context is not
> > > > upstream. Can you rebase?
> > > 
> > > Ah, sorry about that. That's because of commit 51b3802e7960
> > > ("dt-bindings: rtc: qcom-pm8xxx: allow 'wakeup-source' property") which
> > > is now in Linus's tree (and your rtc-fixes branch).
> > > 
> > > Do you still want me to rebase or do you prefer to handle the conflict
> > > some other way?
> > 
> > Ah yes, my bad, I'll merge rtc-fixes in rtc-next before applying
> 
> Sorry about reminding so soon, but with the merge window approaching
> fast, will you be able to get this merged for 6.3?

Looks like these last two patches adding support for the nvmem offset
has not been applied yet. Still hoping you can get them merged for 6.3
even if this one does not apply cleanly unless you first merge your
rtc-fixes branch.

Johan

Re: [PATCH v2 16/22] dt-bindings: rtc: qcom-pm8xxx: add nvmem-cell offset

[Alexandre Belloni]

On 24/02/2023 09:21:07+0100, Johan Hovold wrote:
> Hi Alexandre,
> 
> On Wed, Feb 15, 2023 at 05:51:20PM +0100, Johan Hovold wrote:
> > On Sat, Feb 11, 2023 at 05:44:39PM +0100, Alexandre Belloni wrote:
> > > On 11/02/2023 09:22:54+0100, Johan Hovold wrote:
> > > > On Fri, Feb 10, 2023 at 11:48:08PM +0100, Alexandre Belloni wrote:
> > > > > On 02/02/2023 16:54:42+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 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.
> > > > > > 
> > > > > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > > > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > 
> > > > > The patch doesn't apply because this part of the context is not
> > > > > upstream. Can you rebase?
> > > > 
> > > > Ah, sorry about that. That's because of commit 51b3802e7960
> > > > ("dt-bindings: rtc: qcom-pm8xxx: allow 'wakeup-source' property") which
> > > > is now in Linus's tree (and your rtc-fixes branch).
> > > > 
> > > > Do you still want me to rebase or do you prefer to handle the conflict
> > > > some other way?
> > > 
> > > Ah yes, my bad, I'll merge rtc-fixes in rtc-next before applying
> > 
> > Sorry about reminding so soon, but with the merge window approaching
> > fast, will you be able to get this merged for 6.3?
> 
> Looks like these last two patches adding support for the nvmem offset
> has not been applied yet. Still hoping you can get them merged for 6.3
> even if this one does not apply cleanly unless you first merge your
> rtc-fixes branch.

This is still my plan, I'm travelling right now but they will be sent for
6.3

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

Re: [PATCH v2 16/22] dt-bindings: rtc: qcom-pm8xxx: add nvmem-cell offset

[Johan Hovold]

On Fri, Feb 24, 2023 at 10:18:54AM +0100, Alexandre Belloni wrote:
> On 24/02/2023 09:21:07+0100, Johan Hovold wrote:

> > Looks like these last two patches adding support for the nvmem offset
> > has not been applied yet. Still hoping you can get them merged for 6.3
> > even if this one does not apply cleanly unless you first merge your
> > rtc-fixes branch.
> 
> This is still my plan, I'm travelling right now but they will be sent for
> 6.3

Perfect, thanks!

Johan

[PATCH v2 17/22] 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 | 141 +++++++++++++++++++++++++++++++++++----
 1 file changed, 129 insertions(+), 12 deletions(-)

diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index eff2782beeed..372494e82f40 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -1,8 +1,13 @@
 // 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>
+#include <linux/nvmem-consumer.h>
 #include <linux/init.h>
 #include <linux/rtc.h>
 #include <linux/platform_device.h>
@@ -49,6 +54,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 +64,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_dbg(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc);
+		return rc;
+	}
+
+	if (len != sizeof(u32)) {
+		dev_dbg(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_dbg(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 +149,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 +185,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 +236,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 +267,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 +284,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 +324,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);
@@ -378,9 +480,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;
@@ -435,3 +551,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

Re: [PATCH v2 17/22] rtc: pm8xxx: add support for nvmem offset

[Konrad Dybcio]

On 2.02.2023 16:54, 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>
> ---
That's gonna be a stupid question, but just to make sure..

SDAM is rewritable, right? So that when somebody sets the time to
year 2077 by mistake, they won't have to put up with it for the next
50 years? :D

Konrad
>  drivers/rtc/rtc-pm8xxx.c | 141 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 129 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> index eff2782beeed..372494e82f40 100644
> --- a/drivers/rtc/rtc-pm8xxx.c
> +++ b/drivers/rtc/rtc-pm8xxx.c
> @@ -1,8 +1,13 @@
>  // 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>
> +#include <linux/nvmem-consumer.h>
>  #include <linux/init.h>
>  #include <linux/rtc.h>
>  #include <linux/platform_device.h>
> @@ -49,6 +54,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 +64,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_dbg(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc);
> +		return rc;
> +	}
> +
> +	if (len != sizeof(u32)) {
> +		dev_dbg(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_dbg(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 +149,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 +185,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 +236,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 +267,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 +284,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 +324,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);
> @@ -378,9 +480,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;
> @@ -435,3 +551,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>");

Re: [PATCH v2 17/22] rtc: pm8xxx: add support for nvmem offset

[Johan Hovold]

On Fri, Feb 03, 2023 at 04:31:54AM +0100, Konrad Dybcio wrote:
> 
> 
> On 2.02.2023 16:54, 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>
> > ---
> That's gonna be a stupid question, but just to make sure..
> 
> SDAM is rewritable, right? So that when somebody sets the time to
> year 2077 by mistake, they won't have to put up with it for the next
> 50 years? :D

Heh, yes, it is re-writeable. Otherwise, using SDAM wouldn't really have
been an alternative to using the UEFI offset. :)

These registers are reset if you lose battery power so you'd be back at
1970 as expected if that ever happens too.

Johan

Re: [PATCH v2 17/22] rtc: pm8xxx: add support for nvmem offset

[David Collins]

On 2/2/23 07:54, 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

s/can not/cannot/

> 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 | 141 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 129 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> index eff2782beeed..372494e82f40 100644
> --- a/drivers/rtc/rtc-pm8xxx.c
> +++ b/drivers/rtc/rtc-pm8xxx.c
> @@ -1,8 +1,13 @@
>  // 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>
> +#include <linux/nvmem-consumer.h>
>  #include <linux/init.h>
>  #include <linux/rtc.h>
>  #include <linux/platform_device.h>
> @@ -49,6 +54,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 +64,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_dbg(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc);

Why is dev_dbg() used instead of dev_err() for newly added error
messages?  Also, why do these conditions warrant error logging when some
of the previous patches in this series removed older error logging?

Thanks,
David

Re: [PATCH v2 17/22] rtc: pm8xxx: add support for nvmem offset

[Johan Hovold]

On Mon, Feb 06, 2023 at 07:16:54PM -0800, David Collins wrote:
> On 2/2/23 07:54, 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_dbg(rtc_dd->dev, "failed to read nvmem offset: %d\n", rc);
> 
> Why is dev_dbg() used instead of dev_err() for newly added error
> messages?  Also, why do these conditions warrant error logging when some
> of the previous patches in this series removed older error logging?

I would have used dev_err() here and did so for v1, but Alexandre
prefers dev_dbg() for errors that are unlikely to be seen by regular
users but that can still be useful to developers (e.g. when enabling the
rtc on a new platform).

One or two of the spmi errors I removed falls in the same category in so
far that the control and time registers may write-protected on some
platforms, but such errors are currently logged by the spmi controller
driver.

Johan

[PATCH v2 18/22] 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

Re: [PATCH v2 18/22] arm64: defconfig: enable Qualcomm SDAM nvmem driver

[Krzysztof Kozlowski]

On 02/02/2023 16:54, Johan Hovold wrote:
> 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.


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

Best regards,
Krzysztof

[PATCH v2 19/22] 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 v2 20/22] 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 v2 21/22] 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.

While it is possible to access this UEFI variable from Linux on the CRD,
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.

Also note that setting variables using this interface does not work on
at least one CRD for reasons not yet known.

So instead of using the UEFI RTC offset, reserve four bytes in one of
the PMIC SDAM scratch-register 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 3e0cbacb0641..73b7507b956a 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
@@ -499,6 +499,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 v2 22/22] 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 scratch-register 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: (subset) [PATCH v2 00/22] rtc: pm8xxx: add support for setting time using nvmem

[Bjorn Andersson]

On Thu, 2 Feb 2023 16:54:26 +0100, Johan Hovold wrote:
> 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.
> 
> [...]

Applied, thanks!

[18/22] arm64: defconfig: enable Qualcomm SDAM nvmem driver
        commit: 480ba14b9a95641647a6561d5b246de661589514

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>

Re: (subset) [PATCH v2 00/22] rtc: pm8xxx: add support for setting time using nvmem

[Alexandre Belloni]

On Thu, 02 Feb 2023 16:54:26 +0100, Johan Hovold wrote:
> 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.
> 
> [...]

Applied, thanks!

[01/22] rtc: pm8xxx: fix set-alarm race
        commit: c88db0eff9722fc2b6c4d172a50471d20e08ecc6
[02/22] rtc: pm8xxx: drop spmi error messages
        commit: eb245631836b4843199d7176d1597759dda4ee9e
[03/22] rtc: pm8xxx: use regmap_update_bits()
        commit: 182c23bbfea3713206b0da3fbbb7350e197a92dd
[04/22] rtc: pm8xxx: drop bogus locking
        commit: 8d273f33fd090a2c270c67b6ac7fa03f5a7eee3f
[05/22] rtc: pm8xxx: return IRQ_NONE on errors
        commit: cb9bb7b2364bb5f4f51226ce1f9ec6ffda618f0a
[06/22] rtc: pm8xxx: drop unused register defines
        commit: f081b74c1c748a7da972c782c2f974f239a9b51f
[07/22] rtc: pm8xxx: use unaligned le32 helpers
        commit: 79dd75661e4284169768859012a4bf6898cef758
[08/22] rtc: pm8xxx: clean up time and alarm debugging
        commit: c996956fcc5b7756eb04615cc36618acaa85caa9
[09/22] rtc: pm8xxx: rename struct device pointer
        commit: a375510efeda0dfbad205cd1de8b57f63d0779c9
[10/22] rtc: pm8xxx: rename alarm irq variable
        commit: 4727b58fc84daf6d7097ac3528a6517456a5e110
[11/22] rtc: pm8xxx: clean up comments
        commit: 3c3326394ba420608d0665aef846b2268c9c9629
[12/22] rtc: pm8xxx: use u32 for timestamps
        commit: 35d9c472925748a1cb1f5b6cc8ae71cf8138e30f
[13/22] rtc: pm8xxx: refactor read_time()
        commit: da862c3df6add928e2f51d6cadec128a9a1940f3
[14/22] rtc: pm8xxx: clean up local declarations
        commit: 9e5a799138042ac8276e6744c548b0411f371600
[15/22] rtc: pm8xxx: drop error messages
        commit: c94fb939e65155bc889e62396f83ef4317d643ac

Best regards,

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

Re: (subset) [PATCH v2 00/22] rtc: pm8xxx: add support for setting time using nvmem

[Johan Hovold]

On Thu, Feb 09, 2023 at 11:25:34PM +0100, Alexandre Belloni wrote:
> 
> On Thu, 02 Feb 2023 16:54:26 +0100, Johan Hovold wrote:
> > 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.
> > 
> > [...]
> 
> Applied, thanks!
> 
> [01/22] rtc: pm8xxx: fix set-alarm race
>         commit: c88db0eff9722fc2b6c4d172a50471d20e08ecc6

...

> [15/22] rtc: pm8xxx: drop error messages
>         commit: c94fb939e65155bc889e62396f83ef4317d643ac

I noticed that you did not apply patches 16 and 17 that add support for
the nvmem offset. Was that on purpose or a mistake?

Johan

Re: (subset) [PATCH v2 00/22] rtc: pm8xxx: add support for setting time using nvmem

[Alexandre Belloni]

On 10/02/2023 08:53:52+0100, Johan Hovold wrote:
> On Thu, Feb 09, 2023 at 11:25:34PM +0100, Alexandre Belloni wrote:
> > 
> > On Thu, 02 Feb 2023 16:54:26 +0100, Johan Hovold wrote:
> > > 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.
> > > 
> > > [...]
> > 
> > Applied, thanks!
> > 
> > [01/22] rtc: pm8xxx: fix set-alarm race
> >         commit: c88db0eff9722fc2b6c4d172a50471d20e08ecc6
> 
> ...
> 
> > [15/22] rtc: pm8xxx: drop error messages
> >         commit: c94fb939e65155bc889e62396f83ef4317d643ac
> 
> I noticed that you did not apply patches 16 and 17 that add support for
> the nvmem offset. Was that on purpose or a mistake?

This was on purpose, I'll handle them tonight.

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

Re: (subset) [PATCH v2 00/22] rtc: pm8xxx: add support for setting time using nvmem

[Johan Hovold]

On Fri, Feb 10, 2023 at 10:04:03AM +0100, Alexandre Belloni wrote:
> On 10/02/2023 08:53:52+0100, Johan Hovold wrote:
> > On Thu, Feb 09, 2023 at 11:25:34PM +0100, Alexandre Belloni wrote:
> > > 
> > > On Thu, 02 Feb 2023 16:54:26 +0100, Johan Hovold wrote:
> > > > 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.
> > > > 
> > > > [...]
> > > 
> > > Applied, thanks!
> > > 
> > > [01/22] rtc: pm8xxx: fix set-alarm race
> > >         commit: c88db0eff9722fc2b6c4d172a50471d20e08ecc6
> > 
> > ...
> > 
> > > [15/22] rtc: pm8xxx: drop error messages
> > >         commit: c94fb939e65155bc889e62396f83ef4317d643ac
> > 
> > I noticed that you did not apply patches 16 and 17 that add support for
> > the nvmem offset. Was that on purpose or a mistake?
> 
> This was on purpose, I'll handle them tonight.

Ok, thanks.

Johan