[PATCH v3 0/4] rtc: pcf85063: Update interrupt support

[PATCH v3 0/4] rtc: pcf85063: Update interrupt support

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Add hardware-backed UIE (1 Hz update interrupt) support to the pcf85063
driver. The PCF85063A/RV8263 have no dedicated update-interrupt source,
so chrony and similar tools that rely on RTC_UIE_ON fall back to the
kernel's software polling emulation. This series repurposes the chip's
countdown timer (TCF = 1 Hz, T = 1) to generate a periodic interrupt on
the INT pin, serving UIE entirely in hardware.

Patches 2-3 are preparatory: they fix an AIE race in the interrupt
handler and a lost-alarm-flag race in the clkout helpers.

Patch 1 fixes a pre-existing use-after-free in the clkout clock provider
on unbind, found during review by Sashiko.

Changelog:
v3:
- split into 3 preparatory patches and the final new feature patch
- replaced racy regmap_write() with regmap_update_bits() in IRQ handler
- the timer flag (TF) is write-0-to-clear, so every read-modify-write of
  CTRL2 preserves it by writing it back as 1
v2:
- https://lore.kernel.org/all/20260820112437.3715237-1-alexander.sverdlin@siemens.com/

Alexander Sverdlin (4):
  rtc: pcf85063: use devm_of_clk_add_hw_provider() for clkout
  rtc: pcf85063: do not clear AIE in the interrupt handler
  rtc: pcf85063: preserve the alarm flag in clkout register updates
  rtc: pcf85063: add 1 Hz update interrupt (UIE) support

 drivers/rtc/rtc-pcf85063.c | 139 +++++++++++++++++++++++++++++++------
 1 file changed, 118 insertions(+), 21 deletions(-)

-- 
2.55.0

[PATCH v3 1/4] rtc: pcf85063: use devm_of_clk_add_hw_provider() for clkout

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

pcf85063_clkout_register_clk() registered the OF clock provider with the
deprecated of_clk_add_provider() and never removed it, so the provider
kept pointing at freed data after unbind, leading to a use-after-free the
next time the device-tree clock was resolved.

Switch to devm_clk_hw_register() and devm_of_clk_add_hw_provider(): both
the clock and its OF provider are managed resources now, and the provider
is torn down before the clock on unbind.

Register the clock on the parent i2c device instead of the rtc device.
devm_of_clk_add_hw_provider() must use the parent, which owns the
of_node, and the clkout_hw it points at lives in the driver data
allocated on the parent. Tying both to the parent releases them together
on unbind, before the driver data is freed, and keeps their teardown
ordering guaranteed by a single devres list. Registering the clock on the
rtc device instead could defer its unregistration past unbind (e.g. while
a /dev/rtcN fd is open), after clkout_hw has already been freed.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
Changelog:
v3:
- new patch (pre-exising issue found by Sashiko)

 drivers/rtc/rtc-pcf85063.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
index 8cb9ffc73f6d9..ac9ab1c376c0b 100644
--- a/drivers/rtc/rtc-pcf85063.c
+++ b/drivers/rtc/rtc-pcf85063.c
@@ -495,12 +495,13 @@ static const struct clk_ops pcf85063_clkout_ops = {
 	.set_rate = pcf85063_clkout_set_rate,
 };
 
-static struct clk *pcf85063_clkout_register_clk(struct pcf85063 *pcf85063)
+static int pcf85063_clkout_register_clk(struct pcf85063 *pcf85063)
 {
-	struct clk *clk;
+	struct device *dev = pcf85063->rtc->dev.parent;
 	struct clk_init_data init = {};
-	struct device_node *node = pcf85063->rtc->dev.parent->of_node;
+	struct device_node *node = dev->of_node;
 	struct device_node *fixed_clock;
+	int ret;
 
 	fixed_clock = of_get_child_by_name(node, "clock");
 	if (fixed_clock) {
@@ -510,7 +511,7 @@ static struct clk *pcf85063_clkout_register_clk(struct pcf85063 *pcf85063)
 		 * registered automatically when being referenced.
 		 */
 		of_node_put(fixed_clock);
-		return NULL;
+		return 0;
 	}
 
 	init.name = "pcf85063-clkout";
@@ -524,12 +525,12 @@ static struct clk *pcf85063_clkout_register_clk(struct pcf85063 *pcf85063)
 	of_property_read_string(node, "clock-output-names", &init.name);
 
 	/* register the clock */
-	clk = devm_clk_register(&pcf85063->rtc->dev, &pcf85063->clkout_hw);
-
-	if (!IS_ERR(clk))
-		of_clk_add_provider(node, of_clk_src_simple_get, clk);
+	ret = devm_clk_hw_register(dev, &pcf85063->clkout_hw);
+	if (ret)
+		return ret;
 
-	return clk;
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
+					   &pcf85063->clkout_hw);
 }
 #endif
 
-- 
2.55.0

Re: [PATCH v3 1/4] rtc: pcf85063: use devm_of_clk_add_hw_provider() for clkout

[sashiko-bot]

> From: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> pcf85063_clkout_register_clk() registered the OF clock provider with the
> deprecated of_clk_add_provider() and never removed it, so the provider
> kept pointing at freed data after unbind, leading to a use-after-free the
> next time the device-tree clock was resolved.
> 
> Switch to devm_clk_hw_register() and devm_of_clk_add_hw_provider(): both
> the clock and its OF provider are managed resources now, and the provider
> is torn down before the clock on unbind.
> 
> Register the clock on the parent i2c device instead of the rtc device.
> devm_of_clk_add_hw_provider() must use the parent, which owns the
> of_node, and the clkout_hw it points at lives in the driver data
> allocated on the parent. Tying both to the parent releases them together
> on unbind, before the driver data is freed, and keeps their teardown
> ordering guaranteed by a single devres list. Registering the clock on the
> rtc device instead could defer its unregistration past unbind (e.g. while
> a /dev/rtcN fd is open), after clkout_hw has already been freed.
> 
> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828164445.3907839-1-alexander.sverdlin@siemens.com?part=1

[PATCH v3 2/4] rtc: pcf85063: do not clear AIE in the interrupt handler

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

The interrupt handler cleared the alarm interrupt enable (AIE) bit while
acknowledging the alarm flag (AF). This races with process-context updates
of AIE in pcf85063_rtc_alarm_irq_enable() and pcf85063_rtc_set_alarm():
a handler running concurrently can clobber an AIE change.

The RTC core already disables the alarm interrupt after a one-shot alarm
via the .alarm_irq_enable callback, so the handler does not need to touch
AIE at all. Acknowledge AF only and leave AIE management to the core.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
Changelog:
v3:
- new patch (pre-existing issue found by Sashiko)

 drivers/rtc/rtc-pcf85063.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
index ac9ab1c376c0b..af900fe8ae15e 100644
--- a/drivers/rtc/rtc-pcf85063.c
+++ b/drivers/rtc/rtc-pcf85063.c
@@ -238,8 +238,7 @@ static irqreturn_t pcf85063_rtc_handle_irq(int irq, void *dev_id)
 	if (val & PCF85063_CTRL2_AF) {
 		rtc_update_irq(pcf85063->rtc, 1, RTC_IRQF | RTC_AF);
 		regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
-				   PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF,
-				   0);
+				   PCF85063_CTRL2_AF, 0);
 		return IRQ_HANDLED;
 	}
 
-- 
2.55.0

Re: [PATCH v3 2/4] rtc: pcf85063: do not clear AIE in the interrupt handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing cleanup of wake IRQ and wakeup source for the SPI device leads to memory leaks and a use-after-free of the IRQ number upon unbind.
--

commit a20e38cbd0dbed144e6a870cac13aec94ffaa637
Author: Alexander Sverdlin <alexander.sverdlin@siemens.com>

rtc: pcf85063: do not clear AIE in the interrupt handler

The interrupt handler cleared the AIE bit while acknowledging the AF flag, which
could race with process-context updates. The RTC core already manages disabling
the alarm interrupt, so this patch limits the handler to only acknowledging AF.

> diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
> index ac9ab1c376c0b..af900fe8ae15e 100644
> --- a/drivers/rtc/rtc-pcf85063.c
> +++ b/drivers/rtc/rtc-pcf85063.c
> @@ -238,8 +238,7 @@ static irqreturn_t pcf85063_rtc_handle_irq(int irq, void *dev_id)
>  	if (val & PCF85063_CTRL2_AF) {
>  		rtc_update_irq(pcf85063->rtc, 1, RTC_IRQF | RTC_AF);
>  		regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> -				   PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF,
> -				   0);
> +				   PCF85063_CTRL2_AF, 0);
>  		return IRQ_HANDLED;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but I noticed a potential leak during driver
unbind for the SPI variant in this file.

In pcf85063_probe(), the wake IRQ and wakeup source are allocated without
using devres:

drivers/rtc/rtc-pcf85063.c:pcf85063_probe() {
    ...
		} else {
			set_bit(RTC_FEATURE_ALARM, pcf85063->rtc->features);
			device_init_wakeup(dev, true);
			err = dev_pm_set_wake_irq(dev, irq);
    ...
}

The SPI subsystem, unlike I2C, does not automatically clear the wake IRQ or
wakeup source on driver removal. Since the rv8063_driver SPI driver lacks a
.remove callback, could this leak the wake IRQ and wakeup source when the
driver is unbound?

drivers/rtc/rtc-pcf85063.c:
static struct spi_driver rv8063_driver = {
	.driver         = {
		.name   = "rv8063",
		.of_match_table = rv8063_of_match,
	},
	.probe          = rv8063_probe,
	.id_table	= rv8063_id,
};

When the driver is unbound, devm_request_threaded_irq() frees the IRQ via
devres, but dev->power.wakeirq still holds the freed IRQ number and the
wakeup_source remains registered. 

If the system subsequently suspends, dev_pm_arm_wake_irq() iterates over
registered wakeup sources and calls enable_irq_wake() with the freed IRQ
number. Could this access a stale IRQ descriptor and interfere with hardware
state, potentially leading to an unhandled IRQ storm on resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828164445.3907839-1-alexander.sverdlin@siemens.com?part=2

[PATCH v3 3/4] rtc: pcf85063: preserve the alarm flag in clkout register updates

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

The clkout rate and enable/disable helpers update CTRL2 with
regmap_update_bits() but leave the alarm flag (AF) out of the mask. AF is
write-0-to-clear, so an AF that gets asserted between the read and the
write of the read-modify-write is written back as 0 and the pending alarm
interrupt is lost.

Add AF to the mask and write it back as 1 (write-1-to-preserve) so a
concurrently asserted alarm flag survives a clkout reconfiguration.

Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
Changelog:
v3:
- new patch

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

diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
index af900fe8ae15e..a3af86456ccfc 100644
--- a/drivers/rtc/rtc-pcf85063.c
+++ b/drivers/rtc/rtc-pcf85063.c
@@ -431,7 +431,8 @@ static int pcf85063_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
 		if (clkout_rates[i] == rate)
 			return regmap_update_bits(pcf85063->regmap,
 				PCF85063_REG_CTRL2,
-				PCF85063_REG_CLKO_F_MASK, i);
+				PCF85063_REG_CLKO_F_MASK | PCF85063_CTRL2_AF,
+				i | PCF85063_CTRL2_AF);
 
 	return -EINVAL;
 }
@@ -460,7 +461,8 @@ static int pcf85063_clkout_control(struct clk_hw *hw, bool enable)
 	}
 
 	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
-					PCF85063_REG_CLKO_F_MASK, buf);
+				  PCF85063_REG_CLKO_F_MASK | PCF85063_CTRL2_AF,
+				  buf | PCF85063_CTRL2_AF);
 }
 
 static int pcf85063_clkout_prepare(struct clk_hw *hw)
-- 
2.55.0

Re: [PATCH v3 3/4] rtc: pcf85063: preserve the alarm flag in clkout register updates

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch correctly fixes the read-modify-write race condition for the alarm flag (AF) in the clkout helpers, but it completely misses the exact same bug in `pcf85063_rtc_alarm_irq_enable()`.
--

commit 10941c8a0cce0c4a8065c1c9da8c092b66df0282
Author: Alexander Sverdlin <alexander.sverdlin@siemens.com>

rtc: pcf85063: preserve the alarm flag in clkout register updates

This commit fixes a read-modify-write race condition for the alarm flag (AF)
in the clkout helpers by ensuring AF is preserved during regmap updates.

> diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
> index af900fe8ae15e..a3af86456ccfc 100644
> --- a/drivers/rtc/rtc-pcf85063.c
> +++ b/drivers/rtc/rtc-pcf85063.c
> @@ -431,7 +431,8 @@ static int pcf85063_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
>  		if (clkout_rates[i] == rate)
>  			return regmap_update_bits(pcf85063->regmap,
>  				PCF85063_REG_CTRL2,
> -				PCF85063_REG_CLKO_F_MASK, i);
> +				PCF85063_REG_CLKO_F_MASK | PCF85063_CTRL2_AF,
> +				i | PCF85063_CTRL2_AF);
>  
>  	return -EINVAL;
>  }
> @@ -460,7 +461,8 @@ static int pcf85063_clkout_control(struct clk_hw *hw, bool enable)
>  	}
>  
>  	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> -					PCF85063_REG_CLKO_F_MASK, buf);
> +				  PCF85063_REG_CLKO_F_MASK | PCF85063_CTRL2_AF,
> +				  buf | PCF85063_CTRL2_AF);
>  }

[Severity: High]
This is a pre-existing issue, but does pcf85063_rtc_alarm_irq_enable() also
suffer from the exact same race condition that this patch addresses?

Looking at drivers/rtc/rtc-pcf85063.c:pcf85063_rtc_alarm_irq_enable():

	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
				  PCF85063_CTRL2_AIE,
				  enabled ? PCF85063_CTRL2_AIE : 0);

If the alarm fires exactly during this read-modify-write operation, wouldn't
the hardware alarm flag (AF) be inadvertently cleared since it is omitted from
the mask? This could lead to a lost RTC alarm interrupt and cause the system
to miss critical wakeups.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828164445.3907839-1-alexander.sverdlin@siemens.com?part=3

Re: [PATCH v3 3/4] rtc: pcf85063: preserve the alarm flag in clkout register updates

[Sverdlin, Alexander]

Hi all,

On Fri, 2026-08-28 at 16:58 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The patch correctly fixes the read-modify-write race condition for the alarm flag (AF) in the clkout helpers, but it completely misses the exact same bug in `pcf85063_rtc_alarm_irq_enable()`.
> --
> 
> commit 10941c8a0cce0c4a8065c1c9da8c092b66df0282
> Author: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> 
> rtc: pcf85063: preserve the alarm flag in clkout register updates
> 
> This commit fixes a read-modify-write race condition for the alarm flag (AF)
> in the clkout helpers by ensuring AF is preserved during regmap updates.
> 
> > diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
> > index af900fe8ae15e..a3af86456ccfc 100644
> > --- a/drivers/rtc/rtc-pcf85063.c
> > +++ b/drivers/rtc/rtc-pcf85063.c
> > @@ -431,7 +431,8 @@ static int pcf85063_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
> >   		if (clkout_rates[i] == rate)
> >   			return regmap_update_bits(pcf85063->regmap,
> >   				PCF85063_REG_CTRL2,
> > -				PCF85063_REG_CLKO_F_MASK, i);
> > +				PCF85063_REG_CLKO_F_MASK | PCF85063_CTRL2_AF,
> > +				i | PCF85063_CTRL2_AF);
> >   
> >   	return -EINVAL;
> >   }
> > @@ -460,7 +461,8 @@ static int pcf85063_clkout_control(struct clk_hw *hw, bool enable)
> >   	}
> >   
> >   	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> > -					PCF85063_REG_CLKO_F_MASK, buf);
> > +				  PCF85063_REG_CLKO_F_MASK | PCF85063_CTRL2_AF,
> > +				  buf | PCF85063_CTRL2_AF);
> >   }
> 
> [Severity: High]
> This is a pre-existing issue, but does pcf85063_rtc_alarm_irq_enable() also
> suffer from the exact same race condition that this patch addresses?
> 
> Looking at drivers/rtc/rtc-pcf85063.c:pcf85063_rtc_alarm_irq_enable():
> 
> 	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> 				  PCF85063_CTRL2_AIE,
> 				  enabled ? PCF85063_CTRL2_AIE : 0);
> 
> If the alarm fires exactly during this read-modify-write operation, wouldn't
> the hardware alarm flag (AF) be inadvertently cleared since it is omitted from
> the mask? This could lead to a lost RTC alarm interrupt and cause the system
> to miss critical wakeups.

I believe, that's intended behaviour, to clear the pending interrupt flag on enable.

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com

[PATCH v3 4/4] rtc: pcf85063: add 1 Hz update interrupt (UIE) support

[A. Sverdlin]

From: Alexander Sverdlin <alexander.sverdlin@siemens.com>

The PCF85063A/RV8263 have no dedicated 1 Hz update interrupt, but their
countdown timer can be sourced from a 1 Hz clock (TCF = 1 Hz, T = 1) to
generate a periodic interrupt on the INT pin. Use it to offload UIE to
the hardware, following the pattern of other RTC drivers: the timer is
enabled from set_alarm()/alarm_irq_enable() whenever uie_rtctimer is
active, and the IRQ handler reports RTC_UF when the timer flag (TF) is
set.

RTC_FEATURE_UPDATE_INTERRUPT is now only cleared when no usable interrupt
is wired, preserving the polling emulation for those configurations.

Co-developed-by: Adrian Freihofer <adrian.freihofer@siemens.com>
Signed-off-by: Adrian Freihofer <adrian.freihofer@siemens.com>
Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
---
Changelog:
v3:
- replaced racy regmap_write() with regmap_update_bits() in IRQ handler
- the timer flag (TF) is write-0-to-clear, so every read-modify-write of
  CTRL2 preserves it by writing it back as 1
v2:
- preserve AF bit in pcf85063_set_timer_1hz()
- faster and less racy (regarding AF flag) pcf85063_rtc_handle_irq()
- https://lore.kernel.org/all/20260820112437.3715237-1-alexander.sverdlin@siemens.com/

 drivers/rtc/rtc-pcf85063.c | 121 +++++++++++++++++++++++++++++++++----
 1 file changed, 108 insertions(+), 13 deletions(-)

diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
index a3af86456ccfc..32952e73f2f1c 100644
--- a/drivers/rtc/rtc-pcf85063.c
+++ b/drivers/rtc/rtc-pcf85063.c
@@ -44,6 +44,7 @@
 #define PCF85063_REG_CTRL2		0x01
 #define PCF85063_CTRL2_AF		BIT(6)
 #define PCF85063_CTRL2_AIE		BIT(7)
+#define PCF85063_CTRL2_TF		BIT(3)
 
 #define PCF85063_REG_OFFSET		0x02
 #define PCF85063_OFFSET_SIGN_BIT	6	/* 2's complement sign bit */
@@ -63,6 +64,14 @@
 #define PCF85063_REG_ALM_S		0x0b
 #define PCF85063_AEN			BIT(7)
 
+#define PCF85063_REG_TIMER_VALUE	0x10
+#define PCF85063_REG_TIMER_MODE		0x11
+#define PCF85063_TIMER_MODE_TI_TP	BIT(0)
+#define PCF85063_TIMER_MODE_TIE		BIT(1)
+#define PCF85063_TIMER_MODE_TE		BIT(2)
+#define PCF85063_TIMER_MODE_TCF_MASK	GENMASK(4, 3)
+#define PCF85063_TIMER_MODE_TCF_1HZ	(2 << 3)
+
 struct pcf85063_config {
 	struct regmap_config regmap;
 	unsigned has_alarms:1;
@@ -188,20 +197,75 @@ static int pcf85063_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	return 0;
 }
 
+/*
+ * The chip has no dedicated 1 Hz update interrupt. Drive the countdown timer
+ * at 1 Hz to emulate it, so the RTC core can offload UIE to the hardware
+ * instead of polling. A free-running timer is left untouched to preserve its
+ * phase across the per-second re-arming done by the core.
+ */
+static int pcf85063_set_timer_1hz(struct pcf85063 *pcf85063, bool enable)
+{
+	unsigned int mask = PCF85063_TIMER_MODE_TCF_MASK |
+			    PCF85063_TIMER_MODE_TIE |
+			    PCF85063_TIMER_MODE_TI_TP |
+			    PCF85063_TIMER_MODE_TE;
+	unsigned int mode = 0;
+	unsigned int cur;
+	int ret;
+
+	if (enable)
+		mode = PCF85063_TIMER_MODE_TCF_1HZ | PCF85063_TIMER_MODE_TIE |
+		       PCF85063_TIMER_MODE_TI_TP | PCF85063_TIMER_MODE_TE;
+
+	ret = regmap_read(pcf85063->regmap, PCF85063_REG_TIMER_MODE, &cur);
+	if (ret)
+		return ret;
+
+	if ((cur & mask) == mode)
+		return 0;
+
+	/* Stop the counter before changing its reload value. */
+	ret = regmap_update_bits(pcf85063->regmap, PCF85063_REG_TIMER_MODE,
+				 PCF85063_TIMER_MODE_TE, 0);
+	if (ret)
+		return ret;
+
+	if (enable) {
+		ret = regmap_write(pcf85063->regmap, PCF85063_REG_TIMER_VALUE, 1);
+		if (ret)
+			return ret;
+
+		/* Clear TF but preserve AF */
+		ret = regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
+					 PCF85063_CTRL2_TF | PCF85063_CTRL2_AF, PCF85063_CTRL2_AF);
+		if (ret)
+			return ret;
+	}
+
+	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_TIMER_MODE,
+				  mask, mode);
+}
+
 static int pcf85063_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 {
 	struct pcf85063 *pcf85063 = dev_get_drvdata(dev);
 	u8 buf[5];
 	int ret;
 
+	ret = pcf85063_set_timer_1hz(pcf85063, pcf85063->rtc->uie_rtctimer.enabled);
+	if (ret)
+		return ret;
+
 	buf[0] = bin2bcd(alrm->time.tm_sec);
 	buf[1] = bin2bcd(alrm->time.tm_min);
 	buf[2] = bin2bcd(alrm->time.tm_hour);
 	buf[3] = bin2bcd(alrm->time.tm_mday);
 	buf[4] = PCF85063_AEN; /* Do not match on week day */
 
+	/* Preserve TF */
 	ret = regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
-				 PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF, 0);
+				 PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF | PCF85063_CTRL2_TF,
+				 PCF85063_CTRL2_TF);
 	if (ret)
 		return ret;
 
@@ -211,23 +275,33 @@ static int pcf85063_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 		return ret;
 
 	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
-				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF,
-				  alrm->enabled ? PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF : PCF85063_CTRL2_AF);
+				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF | PCF85063_CTRL2_TF,
+				  pcf85063->rtc->aie_timer.enabled ?
+				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF | PCF85063_CTRL2_TF :
+				  PCF85063_CTRL2_AF | PCF85063_CTRL2_TF);
 }
 
 static int pcf85063_rtc_alarm_irq_enable(struct device *dev,
 					 unsigned int enabled)
 {
 	struct pcf85063 *pcf85063 = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pcf85063_set_timer_1hz(pcf85063, pcf85063->rtc->uie_rtctimer.enabled);
+	if (ret)
+		return ret;
 
 	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
-				  PCF85063_CTRL2_AIE,
-				  enabled ? PCF85063_CTRL2_AIE : 0);
+				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_TF,
+				  pcf85063->rtc->aie_timer.enabled ?
+				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_TF : PCF85063_CTRL2_TF);
 }
 
 static irqreturn_t pcf85063_rtc_handle_irq(int irq, void *dev_id)
 {
 	struct pcf85063 *pcf85063 = dev_id;
+	unsigned long events = RTC_IRQF;
+	irqreturn_t ret = IRQ_NONE;
 	unsigned int val;
 	int err;
 
@@ -236,13 +310,33 @@ static irqreturn_t pcf85063_rtc_handle_irq(int irq, void *dev_id)
 		return IRQ_NONE;
 
 	if (val & PCF85063_CTRL2_AF) {
-		rtc_update_irq(pcf85063->rtc, 1, RTC_IRQF | RTC_AF);
+		events |= RTC_AF;
+		val &= ~PCF85063_CTRL2_AF;
+		ret = IRQ_HANDLED;
+	} else {
+		/* Preserve AF if it comes after the above regmap_read() */
+		val |= PCF85063_CTRL2_AF;
+	}
+
+	if (val & PCF85063_CTRL2_TF) {
+		events |= RTC_UF;
+		val &= ~PCF85063_CTRL2_TF;
+		ret = IRQ_HANDLED;
+	} else {
+		/*
+		 * While not documented, TF demonstrates the same
+		 * write-1-to-preserve semantics as AF in real HW
+		 */
+		val |= PCF85063_CTRL2_TF;
+	}
+
+	if (ret == IRQ_HANDLED) {
 		regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
-				   PCF85063_CTRL2_AF, 0);
-		return IRQ_HANDLED;
+				   PCF85063_CTRL2_AF | PCF85063_CTRL2_TF, val);
+		rtc_update_irq(pcf85063->rtc, 1, events);
 	}
 
-	return IRQ_NONE;
+	return ret;
 }
 
 static int pcf85063_read_offset(struct device *dev, long *offset)
@@ -431,8 +525,8 @@ static int pcf85063_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
 		if (clkout_rates[i] == rate)
 			return regmap_update_bits(pcf85063->regmap,
 				PCF85063_REG_CTRL2,
-				PCF85063_REG_CLKO_F_MASK | PCF85063_CTRL2_AF,
-				i | PCF85063_CTRL2_AF);
+				PCF85063_REG_CLKO_F_MASK | PCF85063_CTRL2_AF | PCF85063_CTRL2_TF,
+				i | PCF85063_CTRL2_AF | PCF85063_CTRL2_TF);
 
 	return -EINVAL;
 }
@@ -461,8 +555,8 @@ static int pcf85063_clkout_control(struct clk_hw *hw, bool enable)
 	}
 
 	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
-				  PCF85063_REG_CLKO_F_MASK | PCF85063_CTRL2_AF,
-				  buf | PCF85063_CTRL2_AF);
+				  PCF85063_REG_CLKO_F_MASK | PCF85063_CTRL2_AF | PCF85063_CTRL2_TF,
+				  buf | PCF85063_CTRL2_AF | PCF85063_CTRL2_TF);
 }
 
 static int pcf85063_clkout_prepare(struct clk_hw *hw)
@@ -609,6 +703,7 @@ static int pcf85063_probe(struct device *dev, struct regmap *regmap, int irq,
 			dev_warn(&pcf85063->rtc->dev,
 				 "unable to request IRQ, alarms disabled\n");
 		} else {
+			set_bit(RTC_FEATURE_UPDATE_INTERRUPT, pcf85063->rtc->features);
 			set_bit(RTC_FEATURE_ALARM, pcf85063->rtc->features);
 			device_init_wakeup(dev, true);
 			err = dev_pm_set_wake_irq(dev, irq);
-- 
2.55.0

Re: [PATCH v3 4/4] rtc: pcf85063: add 1 Hz update interrupt (UIE) support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The driver completely ignores the requested alarm enable parameters, breaking generic `alarmtimers` (POSIX timers) and system wakeups.

Pre-existing issues:
- [High] In `pcf85063_rtc_alarm_irq_enable()`, the `PCF85063_CTRL2_AF` (Alarm Flag) bit is omitted from the `regmap_update_bits` mask and value, which can inadvertently clear a pending alarm interrupt.
--

commit 72f77be444d700f19ce672423be21e8db8ec0a3d
Author: Alexander Sverdlin <alexander.sverdlin@siemens.com>

rtc: pcf85063: add 1 Hz update interrupt (UIE) support

The PCF85063A/RV8263 have no dedicated 1 Hz update interrupt, but their
countdown timer can be sourced from a 1 Hz clock (TCF = 1 Hz, T = 1) to
generate a periodic interrupt on the INT pin. Use it to offload UIE to
the hardware, following the pattern of other RTC drivers.

> diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
> index a3af86456ccfc..32952e73f2f1c 100644
> --- a/drivers/rtc/rtc-pcf85063.c
> +++ b/drivers/rtc/rtc-pcf85063.c

[ ... ]

> @@ -211,23 +275,33 @@ static int pcf85063_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  		return ret;
>  
>  	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> -				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF,
> -				  alrm->enabled ? PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF : PCF85063_CTRL2_AF);
> +				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF | PCF85063_CTRL2_TF,
> +				  pcf85063->rtc->aie_timer.enabled ?
> +				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF | PCF85063_CTRL2_TF :
> +				  PCF85063_CTRL2_AF | PCF85063_CTRL2_TF);
>  }

[Severity: High]
Does this code ignore the requested alrm->enabled parameter in favor of
pcf85063->rtc->aie_timer.enabled?

The RTC core multiplexes various timers onto the hardware alarm and passes the
desired hardware state via alrm->enabled here and via the enabled parameter in
pcf85063_rtc_alarm_irq_enable().

By checking aie_timer.enabled instead, it appears the driver will only enable
the hardware alarm for the legacy ioctl alarm. If a generic alarmtimer (such
as a system wakealarm or a POSIX timer) reaches the front of the queue, won't
aie_timer.enabled be false, silencing the requested alarm?

>  static int pcf85063_rtc_alarm_irq_enable(struct device *dev,
>  					 unsigned int enabled)
>  {
>  	struct pcf85063 *pcf85063 = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pcf85063_set_timer_1hz(pcf85063, pcf85063->rtc->uie_rtctimer.enabled);
> +	if (ret)
> +		return ret;
>  
>  	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> -				  PCF85063_CTRL2_AIE,
> -				  enabled ? PCF85063_CTRL2_AIE : 0);
> +				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_TF,
> +				  pcf85063->rtc->aie_timer.enabled ?
> +				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_TF : PCF85063_CTRL2_TF);
>  }

[Severity: High]
This is a pre-existing issue, but does this read-modify-write omit
PCF85063_CTRL2_AF from the mask, potentially clearing a pending alarm?

Since the AF bit in PCF85063_REG_CTRL2 is write-0-to-clear, if the hardware
asserts the AF interrupt between the internal register read and write of this
operation, the read value will have AF=0. Because AF is not masked,
regmap_update_bits() will write back AF=0, which unintentionally clears the
newly asserted alarm flag.

Also, similar to pcf85063_rtc_set_alarm() above, does this code
inappropriately ignore the enabled parameter passed by the core?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828164445.3907839-1-alexander.sverdlin@siemens.com?part=4