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

[PATCH] 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>
---
 drivers/rtc/rtc-pcf85063.c | 97 ++++++++++++++++++++++++++++++++++----
 1 file changed, 89 insertions(+), 8 deletions(-)

diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
index 01e209d88f5f3..76675b75388e4 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,12 +197,64 @@ 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;
+
+		ret = regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
+					 PCF85063_CTRL2_TF, 0);
+		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);
@@ -212,22 +273,32 @@ static int pcf85063_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 
 	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->rtc->aie_timer.enabled ?
+				  PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF : PCF85063_CTRL2_AF);
 }
 
 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->rtc->aie_timer.enabled ?
+				  PCF85063_CTRL2_AIE : 0);
 }
 
 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 mask = 0;
 	unsigned int val;
 	int err;
 
@@ -236,14 +307,23 @@ 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);
-		regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
-				   PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF,
-				   0);
-		return IRQ_HANDLED;
+		events |= RTC_AF;
+		mask |= PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF;
+		ret = IRQ_HANDLED;
+	}
+
+	if (val & PCF85063_CTRL2_TF) {
+		events |= RTC_UF;
+		mask |= PCF85063_CTRL2_TF;
+		ret = IRQ_HANDLED;
+	}
+
+	if (ret == IRQ_HANDLED) {
+		regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2, mask, 0);
+		rtc_update_irq(pcf85063->rtc, 1, events);
 	}
 
-	return IRQ_NONE;
+	return ret;
 }
 
 static int pcf85063_read_offset(struct device *dev, long *offset)
@@ -607,6 +687,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] rtc: pcf85063: add 1 Hz update interrupt (UIE) support

[sashiko-bot]

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

New issues:
- [High] Naive read-modify-write operations on the CTRL2 register can drop the write-0-to-clear AF and TF interrupt flags.
- [High] The UIE timer interrupt is configured in pulse mode (`TI_TP = 1`), causing an interrupt storm with level-triggered IRQs.

Pre-existing issues:
- [High] Time-of-check to time-of-use (TOCTOU) race in the threaded IRQ handler clobbers concurrent alarm enable operations.
--

--- Patch [1]: [PATCH] rtc: pcf85063: add 1 Hz update interrupt (UIE) support ---
commit 283190d20c79bb78192c9f090cf19e5f426cb933
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 to generate a periodic
interrupt. Use it to offload UIE to the hardware.

> diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
> index 01e209d88f5f3..76675b75388e4 100644
> --- a/drivers/rtc/rtc-pcf85063.c
> +++ b/drivers/rtc/rtc-pcf85063.c
[ ... ]
> @@ -188,12 +197,64 @@ 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;

[Severity: High]
Does configuring the timer in pulse mode (PCF85063_TIMER_MODE_TI_TP = 1) cause
an interrupt storm on platforms using level-triggered interrupts?

If the hardware emits a fixed-width pulse on the INT pin instead of following
the TF flag, when the threaded handler clears TF and returns, the INT pin might
remain physically low for the remainder of the pulse.

Upon unmasking, would the level-triggered IRQ controller immediately fire again,
leading to repeated IRQ_NONE returns and eventually disabling the IRQ?

> +
> +	ret = regmap_read(pcf85063->regmap, PCF85063_REG_TIMER_MODE, &cur);
> +	if (ret)
> +		return ret;
[ ... ]
> +	if (enable) {
> +		ret = regmap_write(pcf85063->regmap, PCF85063_REG_TIMER_VALUE, 1);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> +					 PCF85063_CTRL2_TF, 0);

[Severity: High]
Can this read-modify-write operation drop write-0-to-clear interrupt flags?

If the AF flag was read as 0 internally by regmap_update_bits(), but asserts
in the hardware before the write, wouldn't the write of 0 to the unmasked bits
inadvertently clear the newly arrived alarm interrupt?

> +		if (ret)
> +			return ret;
> +	}
> +
> +	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_TIMER_MODE,
> +				  mask, mode);
> +}
[ ... ]
> @@ -236,14 +307,23 @@ 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);
> -		regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> -				   PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF,
> -				   0);
> -		return IRQ_HANDLED;
> +		events |= RTC_AF;
> +		mask |= PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF;

[Severity: High]
This is a pre-existing issue, but does caching the decision to clear AIE here
create a time-of-check to time-of-use race that clobbers concurrent alarm
enable operations?

If a concurrent thread sets a new alarm (e.g., writing AIE=1) after CTRL2 was
read at the start of this handler, wouldn't the delayed regmap_update_bits()
at the end of the handler overwrite AIE back to 0, discarding the user's
alarm request?

> +		ret = IRQ_HANDLED;
> +	}
> +
> +	if (val & PCF85063_CTRL2_TF) {
> +		events |= RTC_UF;
> +		mask |= PCF85063_CTRL2_TF;
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	if (ret == IRQ_HANDLED) {
> +		regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2, mask, 0);

[Severity: High]
Can this read-modify-write operation lose interrupts that assert during the
RMW window?

Since both AF and TF are write-0-to-clear flags, if an interrupt arrives after
the regmap_update_bits() read step but before the write step, the unmasked bits
will be written back as 0, permanently losing the interrupt.

> +		rtc_update_irq(pcf85063->rtc, 1, events);
>  	}
>  
> -	return IRQ_NONE;
> +	return ret;
>  }

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

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

[Sverdlin, Alexander]

On Fri, 2026-08-07 at 13:15 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] Naive read-modify-write operations on the CTRL2 register can drop the write-0-to-clear AF and TF interrupt flags.
> - [High] The UIE timer interrupt is configured in pulse mode (`TI_TP = 1`), causing an interrupt storm with level-triggered IRQs.
> 
> Pre-existing issues:
> - [High] Time-of-check to time-of-use (TOCTOU) race in the threaded IRQ handler clobbers concurrent alarm enable operations.
> --
> 
> --- Patch [1]: [PATCH] rtc: pcf85063: add 1 Hz update interrupt (UIE) support ---
> commit 283190d20c79bb78192c9f090cf19e5f426cb933
> 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 to generate a periodic
> interrupt. Use it to offload UIE to the hardware.
> 
> > diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
> > index 01e209d88f5f3..76675b75388e4 100644
> > --- a/drivers/rtc/rtc-pcf85063.c
> > +++ b/drivers/rtc/rtc-pcf85063.c
> [ ... ]
> > @@ -188,12 +197,64 @@ 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;
> 
> [Severity: High]
> Does configuring the timer in pulse mode (PCF85063_TIMER_MODE_TI_TP = 1) cause
> an interrupt storm on platforms using level-triggered interrupts?
> 
> If the hardware emits a fixed-width pulse on the INT pin instead of following
> the TF flag, when the threaded handler clears TF and returns, the INT pin might
> remain physically low for the remainder of the pulse.
> 
> Upon unmasking, would the level-triggered IRQ controller immediately fire again,
> leading to repeated IRQ_NONE returns and eventually disabling the IRQ?

I don't think TI_TP bit is controlling edge vs level INT pin behaviour, but
rather:
--- datasheet ---
7.2.2.3 Timer Flag (TF)
The bit TF is set logic 1 on the first trigger of MI, HMI, or the countdown timer.
The purpose of the flag is to allow the controlling system to interrogate what
caused the interrupt: timer or alarm. The flag can be read and cleared by command.
The status of the timer flag TF can affect the INT pulse generation depending on
the setting of TI_TP (see Section 7.6.2):
• When TI_TP is set to logic 1, the following conditions occur:
– An INT pulse is generated independent of the status of the timer flag TF.
– TF stays set until it is cleared.
– TF does not affect INT.
– The countdown timer runs in a repetitive loop and keeps generating timed periods.
• When TI_TP is set to logic 0, the following conditions occur:
– The INT generation follows the TF flag.
– TF stays set until it is cleared.
– If TF is not cleared before the next coming interrupt, no INT is generated.
– The countdown timer stops after the first countdown.
--- end datasheet ---

While datasheet never mentions, if the ~INT pin is designed for edge- or
level-triggered input logic, all the diagrams suggest that the INT output is
rather edge-triggering.

> > +
> > +	ret = regmap_read(pcf85063->regmap, PCF85063_REG_TIMER_MODE, &cur);
> > +	if (ret)
> > +		return ret;
> [ ... ]
> > +	if (enable) {
> > +		ret = regmap_write(pcf85063->regmap, PCF85063_REG_TIMER_VALUE, 1);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> > +					 PCF85063_CTRL2_TF, 0);
> 
> [Severity: High]
> Can this read-modify-write operation drop write-0-to-clear interrupt flags?
> 
> If the AF flag was read as 0 internally by regmap_update_bits(), but asserts
> in the hardware before the write, wouldn't the write of 0 to the unmasked bits
> inadvertently clear the newly arrived alarm interrupt?

Makes sense! I'll rework to preserve the AF status bit.

> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return regmap_update_bits(pcf85063->regmap, PCF85063_REG_TIMER_MODE,
> > +				  mask, mode);
> > +}
> [ ... ]
> > @@ -236,14 +307,23 @@ 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);
> > -		regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2,
> > -				   PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF,
> > -				   0);
> > -		return IRQ_HANDLED;
> > +		events |= RTC_AF;
> > +		mask |= PCF85063_CTRL2_AIE | PCF85063_CTRL2_AF;
> 
> [Severity: High]
> This is a pre-existing issue, but does caching the decision to clear AIE here
> create a time-of-check to time-of-use race that clobbers concurrent alarm
> enable operations?
> 
> If a concurrent thread sets a new alarm (e.g., writing AIE=1) after CTRL2 was
> read at the start of this handler, wouldn't the delayed regmap_update_bits()
> at the end of the handler overwrite AIE back to 0, discarding the user's
> alarm request?
> 
> > +		ret = IRQ_HANDLED;
> > +	}
> > +
> > +	if (val & PCF85063_CTRL2_TF) {
> > +		events |= RTC_UF;
> > +		mask |= PCF85063_CTRL2_TF;
> > +		ret = IRQ_HANDLED;
> > +	}
> > +
> > +	if (ret == IRQ_HANDLED) {
> > +		regmap_update_bits(pcf85063->regmap, PCF85063_REG_CTRL2, mask, 0);
> 
> [Severity: High]
> Can this read-modify-write operation lose interrupts that assert during the
> RMW window?
> 
> Since both AF and TF are write-0-to-clear flags, if an interrupt arrives after
> the regmap_update_bits() read step but before the write step, the unmasked bits
> will be written back as 0, permanently losing the interrupt.

I don't see how this HW allows for completely race-free handling, but the
current handling can be improved indeed, at least regarding the AF interrupt,
I'll do it in v2.

> > +		rtc_update_irq(pcf85063->rtc, 1, events);
> >  	}
> >  
> > -	return IRQ_NONE;
> > +	return ret;
> >  }

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com