rtc: pm8xxx: add support for setting time using nvmem

---
 drivers/rtc/rtc-pm8xxx.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

drivers/rtc/rtc-pm8xxx.c +10 ~~-14~~

index 716e5d9ad74d..d114f0da537d 100644
@@ -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",

Signature · 1 line

2.39.1

David Collins

Reviewed-by on v2 1/22

2023-02-07 03:13 UTC #2 lore raw

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

Quoted 2 lines · Johan Hovold

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

Quoted 3 lines

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?

Quoted 6 lines

Fixes 9a9a54ad7aa2 ("drivers/rtc: add support for Qualcomm PMIC8xxx RTC")
Cc [email protected] # 3.1
Signed-off-by Johan Hovold <[email protected]>
---
 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.

Thanks,
David

Johan Hovold

2023-02-07 15:13 UTC #3 lore raw

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

Quoted 6 lines · David Collins

Quoted 2 lines · Johan Hovold
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.

Quoted 6 lines

Quoted 3 lines
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.

Quoted 9 lines

Quoted 6 lines
Fixes 9a9a54ad7aa2 ("drivers/rtc: add support for Qualcomm PMIC8xxx RTC")
Cc [email protected] # 3.1
Signed-off-by Johan Hovold <[email protected]>
---
 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

Johan Hovold

2023-02-07 15:55 UTC #4 lore raw

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

Quoted 10 lines · Johan Hovold

Quoted 6 lines · David Collins
Quoted 2 lines · Johan Hovold
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

Johan Hovold

author

2023-02-02 15:55 UTC #5 lore raw

PATCH v2 2/22 rtc: pm8xxx: drop spmi error messages

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

---
 drivers/rtc/rtc-pm8xxx.c | 71 ++++++++++------------------------------
 1 file changed, 17 insertions(+), 54 deletions(-)

drivers/rtc/rtc-pm8xxx.c +17 ~~-54~~

index d114f0da537d..f49bda999e02 100644
@@ -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;

Signature · 1 line

2.39.1

Johan Hovold

author

2023-02-02 15:56 UTC #6 lore raw

PATCH v2 3/22 rtc: pm8xxx: use regmap_update_bits()

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

---
 drivers/rtc/rtc-pm8xxx.c | 87 ++++++++++------------------------------
 1 file changed, 22 insertions(+), 65 deletions(-)

drivers/rtc/rtc-pm8xxx.c +22 ~~-65~~

index f49bda999e02..8c2847ac64f4 100644
@@ -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 = {

Signature · 1 line

2.39.1

David Collins

Reviewed-by on v2 3/22

2023-02-07 03:14 UTC #7 lore raw

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

Quoted 7 lines · Johan Hovold

Switch to using regmap_update_bits() instead of open coding read-modify-write accesses.
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/rtc/rtc-pm8xxx.c | 87 ++++++++++------------------------------
 1 file changed, 22 insertions(+), 65 deletions(-)

Quoted 25 lines

drivers/rtc/rtc-pm8xxx.c +4 -3

index f49bda999e02..8c2847ac64f4 100644
@@ -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)

drivers/rtc/rtc-pm8xxx.c static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm) · line 111

Quoted 10 lines

drivers/rtc/rtc-pm8xxx.c +0 -8

 		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

Johan Hovold

author

2023-02-02 15:55 UTC #8 lore raw

PATCH v2 4/22 rtc: pm8xxx: drop bogus locking

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.

---
 drivers/rtc/rtc-pm8xxx.c | 67 +++++++++++++---------------------------
 1 file changed, 21 insertions(+), 46 deletions(-)

drivers/rtc/rtc-pm8xxx.c +21 ~~-46~~

index 8c2847ac64f4..053a04f74a91 100644
@@ -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");

Signature · 1 line

2.39.1

David Collins

Reviewed-by on v2 4/22

2023-02-07 03:15 UTC #9 lore raw

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

Quoted 8 lines · 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.

Quoted 8 lines

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 <[email protected]>
---
 drivers/rtc/rtc-pm8xxx.c | 67 +++++++++++++---------------------------
 1 file changed, 21 insertions(+), 46 deletions(-)

Take care,
David

Johan Hovold

author

2023-02-02 15:56 UTC #10 lore raw

PATCH v2 5/22 rtc: pm8xxx: return IRQ_NONE on errors

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

---
 drivers/rtc/rtc-pm8xxx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/rtc/rtc-pm8xxx.c +3 -3

index 053a04f74a91..dc7e659cbb2a 100644
@@ -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;
 }

Signature · 1 line

2.39.1

Johan Hovold

author

2023-02-02 15:55 UTC #11 lore raw

PATCH v2 6/22 rtc: pm8xxx: drop unused register defines

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

---
 drivers/rtc/rtc-pm8xxx.c | 6 ------
 1 file changed, 6 deletions(-)

drivers/rtc/rtc-pm8xxx.c +0 -6

index dc7e659cbb2a..90027a7cfb12 100644
@@ -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)

Signature · 1 line

2.39.1

David Collins

Reviewed-by on v2 6/22

2023-02-07 03:15 UTC #12 lore raw

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

Quoted 1 line · Johan Hovold

Drop the original register defines which have been used since commit

s/used/unused/

Quoted 6 lines

c8d523a4b053 ("drivers/rtc/rtc-pm8xxx.c: rework to support pm8941 rtc").
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/rtc/rtc-pm8xxx.c | 6 ------
 1 file changed, 6 deletions(-)

Assuming that the minor commit text comment above is addressed:

Take care,
David

Johan Hovold

2023-02-07 15:15 UTC #13 lore raw

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

Quoted 4 lines · David Collins

Quoted 1 line · Johan Hovold
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.

Quoted 10 lines

Quoted 6 lines
c8d523a4b053 ("drivers/rtc/rtc-pm8xxx.c: rework to support pm8941 rtc").
Signed-off-by Johan Hovold <[email protected]>
---
 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 <[email protected]>

Johan

Johan Hovold

author

2023-02-02 15:55 UTC #14 lore raw

PATCH v2 7/22 rtc: pm8xxx: use unaligned le32 helpers

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

---
 drivers/rtc/rtc-pm8xxx.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

drivers/rtc/rtc-pm8xxx.c +8 ~~-18~~

index 90027a7cfb12..5ff6898bcace 100644
@@ -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);

Signature · 1 line

2.39.1

David Collins

Reviewed-by on v2 7/22

2023-02-07 03:15 UTC #15 lore raw

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

Quoted 7 lines · Johan Hovold

Use the unaligned le32 helpers instead of open coding when accessing the time and alarm registers.
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/rtc/rtc-pm8xxx.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

Take care,
David

Johan Hovold

author

2023-02-02 15:55 UTC #16 lore raw

PATCH v2 8/22 rtc: pm8xxx: clean up time and alarm debugging

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

---
 drivers/rtc/rtc-pm8xxx.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

drivers/rtc/rtc-pm8xxx.c +4 -6

index 5ff6898bcace..8c364e5d3b57 100644
@@ -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;
 }

Signature · 1 line

2.39.1

David Collins

Reviewed-by on v2 8/22

2023-02-07 03:15 UTC #17 lore raw

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

Quoted 7 lines · 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 <[email protected]>
---
 drivers/rtc/rtc-pm8xxx.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Take care,
David

Johan Hovold

author

2023-02-02 15:55 UTC #18 lore raw

PATCH v2 9/22 rtc: pm8xxx: rename struct device pointer

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

---
 drivers/rtc/rtc-pm8xxx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/rtc/rtc-pm8xxx.c +3 -3

index 8c364e5d3b57..0fdbd233b10e 100644
@@ -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)

Signature · 1 line

2.39.1

David Collins

Reviewed-by on v2 9/22

2023-02-07 03:15 UTC #19 lore raw

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

Quoted 8 lines · 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 <[email protected]>
---
 drivers/rtc/rtc-pm8xxx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Take care,
David

Johan Hovold

author

2023-02-02 15:55 UTC #20 lore raw

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

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

---
 drivers/rtc/rtc-pm8xxx.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/rtc/rtc-pm8xxx.c +6 -6

index 0fdbd233b10e..ea867b20573a 100644
@@ -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;

Signature · 1 line

2.39.1

David Collins

Reviewed-by on v2 10/22

2023-02-07 03:16 UTC #21 lore raw

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

Quoted 7 lines · 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 <[email protected]>
---
 drivers/rtc/rtc-pm8xxx.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Take care,
David

Johan Hovold

author

2023-02-02 15:55 UTC #22 lore raw

PATCH v2 11/22 rtc: pm8xxx: clean up comments

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

---
 drivers/rtc/rtc-pm8xxx.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

drivers/rtc/rtc-pm8xxx.c +17 ~~-22~~

index ea867b20573a..8a94a19e0d14 100644
@@ -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,

Signature · 1 line

2.39.1

David Collins

Reviewed-by on v2 11/22

2023-02-07 03:16 UTC #23 lore raw

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

Quoted 7 lines · Johan Hovold

Clean up the driver comments somewhat and remove obsolete, incorrect or redundant ones.
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/rtc/rtc-pm8xxx.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

Take care,
David

Johan Hovold

author

2023-02-02 15:55 UTC #24 lore raw

PATCH v2 12/22 rtc: pm8xxx: use u32 for timestamps

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.

---
 drivers/rtc/rtc-pm8xxx.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/rtc/rtc-pm8xxx.c +6 -6

index 8a94a19e0d14..b1ce246c501a 100644
@@ -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));

Signature · 1 line

2.39.1

Johan Hovold

author

2023-02-02 15:55 UTC #25 lore raw

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

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.

---
 drivers/rtc/rtc-pm8xxx.c | 54 ++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 21 deletions(-)

drivers/rtc/rtc-pm8xxx.c +33 ~~-21~~

index b1ce246c501a..2f96a178595c 100644
@@ -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);

Signature · 1 line

2.39.1

David Collins

2023-02-07 03:16 UTC #26 lore raw

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

Quoted 14 lines · 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 <[email protected]>
---
 drivers/rtc/rtc-pm8xxx.c | 54 ++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 21 deletions(-)
drivers/rtc/rtc-pm8xxx.c +0 -0
index b1ce246c501a..2f96a178595c 100644

drivers/rtc/rtc-pm8xxx.c struct pm8xxx_rtc { · line 62

Quoted 4 lines

drivers/rtc/rtc-pm8xxx.c +1 -0

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

Johan Hovold

2023-02-07 15:19 UTC #27 lore raw

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

Quoted 18 lines · David Collins

Quoted 17 lines · 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 <[email protected]>
---
 drivers/rtc/rtc-pm8xxx.c | 54 ++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 21 deletions(-)
drivers/rtc/rtc-pm8xxx.c +0 -0
index b1ce246c501a..2f96a178595c 100644
@@ -59,6 +59,37 @@ struct pm8xxx_rtc {
 	struct device *dev;
 };
 

Quoted 5 lines

drivers/rtc/rtc-pm8xxx.c struct pm8xxx_rtc { · line 62
Quoted 1 line
drivers/rtc/rtc-pm8xxx.c +1 -0
+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.

Quoted 1 line

The patch looks good to me otherwise.

Johan

Johan Hovold

author

2023-02-02 15:55 UTC #28 lore raw

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

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

---
 drivers/rtc/rtc-pm8xxx.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/rtc/rtc-pm8xxx.c +7 -7

index 2f96a178595c..922aef0f0241 100644
@@ -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)

Signature · 1 line

2.39.1

David Collins

Reviewed-by on v2 14/22

2023-02-07 03:17 UTC #29 lore raw

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

Quoted 7 lines · Johan Hovold

Clean up local declarations somewhat by using the reverse xmas style consistently throughout.
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/rtc/rtc-pm8xxx.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Take care,
David

Johan Hovold

author

2023-02-02 15:55 UTC #30 lore raw

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

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

---
 drivers/rtc/rtc-pm8xxx.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

drivers/rtc/rtc-pm8xxx.c +2 -6

index 922aef0f0241..eff2782beeed 100644
@@ -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)

Signature · 1 line

2.39.1

Johan Hovold

author

2023-02-02 15:55 UTC #31 lore raw

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

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 <[email protected]>

---
 .../devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml     | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml +12 -0

index 21c8ea08ff0a..b95a69cc9ae0 100644
@@ -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";
         };
       };
     };

Signature · 1 line

2.39.1

David Collins

Reviewed-by on v2 16/22

2023-02-07 03:17 UTC #32 lore raw

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

Quoted 2 lines · 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

s/can not/cannot/

Quoted 7 lines

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.

Quoted 5 lines

Acked-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 .../devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml     | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Assuming that the minor commit text comment above is addressed:

Take care,
David

Johan Hovold

2023-02-07 15:24 UTC #33 lore raw

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

Quoted 5 lines · David Collins

Quoted 2 lines · 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
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'.

Quoted 7 lines

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

Quoted 7 lines

Quoted 5 lines
Acked-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 .../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.

Quoted 1 line

Reviewed-by David Collins <[email protected]>

Johan

Alexandre Belloni

maintainer

2023-02-10 22:48 UTC #34 lore raw

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

Quoted 24 lines · 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 <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 .../devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml     | 12 ++++++++++++
 1 file changed, 12 insertions(+)
Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml +1 -0
index 21c8ea08ff0a..b95a69cc9ae0 100644
@@ -40,6 +40,16 @@ properties:
     description:
       Indicates that the setting of RTC time is allowed by the host CPU.
 
+  nvmem-cells:

Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml properties: · line 53

Quoted 10 lines

Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml +9 -0

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

Quoted 14 lines

Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml +2 -0

 required:

@@ -69,6 +79,8 @@ examples:
           compatible = "qcom,pm8921-rtc";
           reg = <0x11d>;
           interrupts = <0x27 0>;
+          nvmem-cells = <&rtc_offset>;
+          nvmem-cell-names = "offset";
         };
       };
     };

Signature · 1 line

2.39.1

Signature · 3 lines

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

Johan Hovold

2023-02-11 08:22 UTC #35 lore raw

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

Quoted 24 lines · Alexandre Belloni

Quoted 23 lines · 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 <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 .../devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml     | 12 ++++++++++++
 1 file changed, 12 insertions(+)
Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml +0 -0
index 21c8ea08ff0a..b95a69cc9ae0 100644
@@ -40,6 +40,16 @@ properties:
     description:
       Indicates that the setting of RTC time is allowed by the host CPU.
 

Quoted 14 lines

Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml properties: · line 53
Quoted 11 lines
Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml +10 -0
+  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

Alexandre Belloni

maintainer

2023-02-11 16:44 UTC #36 lore raw

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

Quoted 40 lines · Johan Hovold

Quoted 24 lines · Alexandre Belloni
Quoted 23 lines · 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 <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 .../devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml     | 12 ++++++++++++
 1 file changed, 12 insertions(+)
Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml +0 -0
index 21c8ea08ff0a..b95a69cc9ae0 100644
@@ -40,6 +40,16 @@ properties:
     description:
       Indicates that the setting of RTC time is allowed by the host CPU.
 
Quoted 14 lines
Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml properties: · line 53
Quoted 11 lines
Documentation/devicetree/bindings/rtc/qcom-pm8xxx-rtc.yaml +10 -0
+  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?

Quoted 6 lines

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

Signature · 3 lines

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

Johan Hovold

2023-02-15 16:50 UTC #37 lore raw

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

Quoted 18 lines · Alexandre Belloni

Quoted 17 lines · Johan Hovold
Quoted 15 lines · Alexandre Belloni
Quoted 11 lines · 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 <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
The patch doesn't apply because this part of the context is not upstream. Can you rebase?

Quoted 8 lines

Quoted 6 lines
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

Johan Hovold

2023-02-24 08:21 UTC #38 lore raw

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

Hi Alexandre,

Quoted 23 lines · Johan Hovold

Quoted 22 lines · Alexandre Belloni
Quoted 21 lines · Johan Hovold
Quoted 10 lines · Alexandre Belloni
Quoted 9 lines · 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.
Quoted 5 lines
Quoted 2 lines
Acked-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
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).

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
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

Alexandre Belloni

maintainer

2023-02-24 09:19 UTC #39 lore raw

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

Quoted 31 lines · Johan Hovold

Hi Alexandre,
Quoted 28 lines · Johan Hovold
Quoted 26 lines · Alexandre Belloni
Quoted 17 lines · Johan Hovold
Quoted 10 lines · Alexandre Belloni
Quoted 9 lines · 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.
Quoted 5 lines
Quoted 2 lines
Acked-by Krzysztof Kozlowski <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
The patch doesn't apply because this part of the context is not upstream. Can you rebase?
Quoted 6 lines
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

Quoted 7 lines

Quoted 2 lines
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

Signature · 3 lines

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

Johan Hovold

2023-02-24 09:24 UTC #40 lore raw

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

Quoted 9 lines · Alexandre Belloni

Quoted 4 lines · Johan Hovold
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

Johan Hovold

author

2023-02-02 15:55 UTC #41 lore raw

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

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.

---
 drivers/rtc/rtc-pm8xxx.c | 141 +++++++++++++++++++++++++++++++++++----
 1 file changed, 129 insertions(+), 12 deletions(-)

drivers/rtc/rtc-pm8xxx.c +129 ~~-12~~

index eff2782beeed..372494e82f40 100644
@@ -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 <[email protected]>");
+MODULE_AUTHOR("Johan Hovold <[email protected]>");

Signature · 1 line

2.39.1

Konrad Dybcio

2023-02-03 03:32 UTC #42 lore raw

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

Quoted 10 lines · 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 <[email protected]>
---

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

Quoted 238 lines

 drivers/rtc/rtc-pm8xxx.c | 141 +++++++++++++++++++++++++++++++++++----
 1 file changed, 129 insertions(+), 12 deletions(-)

drivers/rtc/rtc-pm8xxx.c +129 ~~-12~~

index eff2782beeed..372494e82f40 100644
@@ -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 <[email protected]>");
+MODULE_AUTHOR("Johan Hovold <[email protected]>");

Johan Hovold

2023-02-03 07:22 UTC #43 lore raw

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

Quoted 11 lines · Konrad Dybcio

Quoted 8 lines · 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.

Quoted 7 lines

Quoted 2 lines
Signed-off-by Johan Hovold <[email protected]>
---
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

David Collins

2023-02-07 03:17 UTC #44 lore raw

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

Quoted 2 lines · 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

s/can not/cannot/

Quoted 47 lines

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.

---
 drivers/rtc/rtc-pm8xxx.c | 141 +++++++++++++++++++++++++++++++++++----
 1 file changed, 129 insertions(+), 12 deletions(-)

drivers/rtc/rtc-pm8xxx.c +10 -1

index eff2782beeed..372494e82f40 100644
@@ -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;
 };

drivers/rtc/rtc-pm8xxx.c struct pm8xxx_rtc { · line 80

Quoted 10 lines

drivers/rtc/rtc-pm8xxx.c +10 -0

+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

Johan Hovold

2023-02-07 15:36 UTC #45 lore raw

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

Quoted 16 lines · David Collins

drivers/rtc/rtc-pm8xxx.c struct pm8xxx_rtc { · line 80
Quoted 10 lines · Johan Hovold
drivers/rtc/rtc-pm8xxx.c +10 -0
+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

Johan Hovold

author

2023-02-02 15:55 UTC #46 lore raw

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

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.

---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

arch/arm64/configs/defconfig +1 -0

index a17cbdac1904..b3f714ff4006 100644
@@ -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

Signature · 1 line

2.39.1

Krzysztof Kozlowski

Reviewed-by on v2 18/22

2023-02-03 09:32 UTC #47 lore raw

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

Quoted 6 lines · 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.

Reviewed-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

Johan Hovold

author

2023-02-02 15:55 UTC #48 lore raw

PATCH v2 19/22 arm64: dts: qcom: sc8280xp-pmics: add pmk8280 rtc

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.

---
 arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi +9 -0

index f2c0b71b5d8e..c62262b88810 100644
@@ -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 {

Signature · 1 line

2.39.1

Johan Hovold

author

2023-02-02 15:55 UTC #49 lore raw

PATCH v2 20/22 arm64: dts: qcom: sc8280xp-pmics: add pmk8280 sdam nvram

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

---
 arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi +9 -0

index c62262b88810..7220d5b5ab7b 100644
@@ -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 {

Signature · 1 line

2.39.1

Johan Hovold

author

2023-02-02 15:56 UTC #50 lore raw

PATCH v2 21/22 arm64: dts: qcom: sc8280xp-crd: enable rtc

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.

---
 arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

arch/arm64/boot/dts/qcom/sc8280xp-crd.dts +15 -0

index 3e0cbacb0641..73b7507b956a 100644
@@ -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";
 };

Signature · 1 line

2.39.1

Johan Hovold

author

2023-02-02 15:55 UTC #51 lore raw

PATCH v2 22/22 arm64: dts: qcom: sc8280xp-x13s: enable rtc

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

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.

---
 .../dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts    | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts +15 -0

index db406f7774de..6e88e0bb6871 100644
@@ -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";

Signature · 1 line

2.39.1

Bjorn Andersson

maintainer Applied

2023-02-09 04:41 UTC #52 lore raw

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

Quoted 6 lines · 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.

Quoted 8 lines

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,

Signature · 1 line

Bjorn Andersson <[email protected]>

Alexandre Belloni

maintainer Applied

2023-02-09 22:25 UTC #53 lore raw

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