* [PATCH v8 2/5] rtc: pcf85363: support reporting battery switch-over via RTC_VL
2026-10-26 9:47 [PATCH v8 1/5] dt-bindings: rtc: nxp,pcf85363: add timestamp mode config Lakshay Piplani
@ 2026-10-26 9:47 ` Lakshay Piplani
2026-10-26 10:36 ` sashiko-bot
2026-10-26 9:47 ` [PATCH v8 3/5] rtc: pcf85363: add timestamp support with configurable timestamp mode Lakshay Piplani
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Lakshay Piplani @ 2026-10-26 9:47 UTC (permalink / raw)
To: alexandre.belloni, linux-rtc, linux-kernel, robh, krzk+dt,
conor+dt, devicetree, wim, linux-watchdog, linux
Cc: vikash.bansal, priyanka.jain, Lakshay Piplani
Report battery switch-over on PCF85263/PCF85363 through the standard
RTC_VL_* ioctls: RTC_VL_READ exposes the BSF flag and RTC_VL_CLR clears
it. The flag is cached before it is cleared so it survives re-arming, and
it is collected from both the IRQ handler and RTC_VL_READ so poll-only
setups still see it.
CTRL_FLAGS is write-0-to-clear, so a read-modify-write can drop an
unrelated flag. Add pcf85363_clear_flags(), which writes the complement
of the mask to clear only the requested bits.
Probe no longer blanket-clears CTRL_FLAGS, so pre-existing events are
preserved. As the battery-backed registers can hold stale flags across a
power cycle and storm the level-triggered INTA line, probe masks off only
the interrupt sources and Alarm2 enables this driver never services and
clears their flags, and the IRQ handler clears any unserviced A2F/PIF
flags.
Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
---
V7 -> V8:
- No change.
V6 -> V7:
- Track IRQ acknowledgement failures in the threaded handler and return
IRQ_NONE when an asserted source cannot be serviced or cleared, so the
genirq spurious-interrupt detector can shut down a permanently stuck
INTA line instead of looping forever. Apply the policy consistently to
the A1F, battery-switch and unserviced A2F/PIF paths, and only mark the
interrupt handled when a source was actually cleared.
- Use dev_err_ratelimited() on the flag-clear failure paths so an I/O
error on the level-triggered line cannot flood the log.
V5 -> V6:
- Mask INT_WDIE alongside the other unused INTA sources at probe so a
battery-backed watchdog-enable left by the bootloader cannot storm the
level-triggered INTA line before watchdog support is added later in
the series
V4 -> V5:
- Add pcf85363_clear_flags() for the write-0-to-clear CTRL_FLAGS register
and use it everywhere, so a read-modify-write no longer drops flags.
- Cache BSF before clearing it and collect it from both the IRQ handler
and RTC_VL_READ so poll-only setups still observe it.
- Latch pending events in probe instead of blanket-clearing CTRL_FLAGS,
and mask/clear only the unused INTA sources to avoid an interrupt storm.
- Propagate register-access errors out of pcf85363_collect_events(); the
IRQ handler logs them but still acknowledges the interrupt.
- Serialise the cached event state with a mutex.
V3 -> V4:
- No changes in v4.
V2 -> V3:
- Split into separate patches as suggested:
- Battery switch-over detection.
- Timestamp recording for TS pin and battery switch-over events.
- Offset calibration.
- Watchdog timer (to be reviewed by watchdog maintainers).
- Dropped Alarm2 support
- Switched to rtc_add_group() for sysfs attributes
V1 -> V2:
- Watchdog related changes due to removal of vendor specific properties
from device tree
* remove vendor DT knobs (enable/timeout/stepsize/repeat)
* use watchdog_init_timeout (with 10s default)
* derive clock_sel from final timeout
* default, repeat=true (repeat mode)
- Fixed uninitalised warning on 'ret' (reported by kernel test robot)
- Use dev_dbg instead of dev_info for debug related print messages
- Minor cleanup and comments.
drivers/rtc/rtc-pcf85363.c | 205 ++++++++++++++++++++++++++++++++++---
1 file changed, 191 insertions(+), 14 deletions(-)
diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index ccc7834e5759..00f37ffcca26 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -14,6 +14,8 @@
#include <linux/err.h>
#include <linux/errno.h>
#include <linux/bcd.h>
+#include <linux/device.h>
+#include <linux/mutex.h>
#include <linux/of.h>
#include <linux/regmap.h>
@@ -113,6 +115,9 @@
struct pcf85363 {
struct rtc_device *rtc;
struct regmap *regmap;
+ /* Serialises access to the cached event state below. */
+ struct mutex lock;
+ bool bsf;
};
struct pcf85x63_config {
@@ -120,6 +125,15 @@ struct pcf85x63_config {
unsigned int num_nvram;
};
+/*
+ * CTRL_FLAGS is write-0-to-clear, so write the complement of the mask to
+ * clear only the requested bits without disturbing the others.
+ */
+static int pcf85363_clear_flags(struct pcf85363 *pcf85363, u8 mask)
+{
+ return regmap_write(pcf85363->regmap, CTRL_FLAGS, (u8)~mask);
+}
+
static int pcf85363_load_capacitance(struct pcf85363 *pcf85363, struct device_node *node)
{
u32 load = 7000;
@@ -253,7 +267,7 @@ static int _pcf85363_rtc_alarm_irq_enable(struct pcf85363 *pcf85363, unsigned
return ret;
/* clear current flags */
- return regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_A1F, 0);
+ return pcf85363_clear_flags(pcf85363, FLAGS_A1F);
}
static int pcf85363_rtc_alarm_irq_enable(struct device *dev,
@@ -292,9 +306,41 @@ static int pcf85363_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
return _pcf85363_rtc_alarm_irq_enable(pcf85363, alrm->enabled);
}
+/*
+ * Latch a pending battery-switch event into the software cache. Runs from
+ * the IRQ handler and the RTC_VL_READ path, so it is seen with or without
+ * an interrupt line. BSF is cached before it is cleared so it survives
+ * re-arming. Caller must hold pcf85363->lock. Returns a negative errno on
+ * a register-access failure, otherwise the handled flag mask.
+ */
+static int pcf85363_collect_events(struct pcf85363 *pcf85363)
+{
+ unsigned int flags;
+ int handled = 0;
+ int ret;
+
+ ret = regmap_read(pcf85363->regmap, CTRL_FLAGS, &flags);
+ if (ret)
+ return ret;
+
+ if (flags & FLAGS_BSF) {
+ pcf85363->bsf = true;
+
+ ret = pcf85363_clear_flags(pcf85363, FLAGS_BSF);
+ if (ret)
+ return ret;
+
+ handled |= FLAGS_BSF;
+ }
+
+ return handled;
+}
+
static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
{
struct pcf85363 *pcf85363 = i2c_get_clientdata(dev_id);
+ bool ack_failed = false;
+ bool handled = false;
unsigned int flags;
int err;
@@ -302,16 +348,108 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
if (err)
return IRQ_NONE;
+ if (flags) {
+ dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s\n",
+ flags, (flags & FLAGS_A1F) ? " [A1F]" : "",
+ (flags & FLAGS_BSF) ? " [BSF]" : "");
+ }
+
if (flags & FLAGS_A1F) {
- rtc_update_irq(pcf85363->rtc, 1, RTC_IRQF | RTC_AF);
- regmap_update_bits(pcf85363->regmap, CTRL_FLAGS, FLAGS_A1F, 0);
- return IRQ_HANDLED;
+ err = pcf85363_clear_flags(pcf85363, FLAGS_A1F);
+ if (err) {
+ dev_err_ratelimited(&pcf85363->rtc->dev,
+ "failed to clear alarm flag: %d\n",
+ err);
+ ack_failed = true;
+ } else {
+ rtc_update_irq(pcf85363->rtc, 1, RTC_IRQF | RTC_AF);
+ handled = true;
+ }
+ }
+
+ if (flags & FLAGS_BSF) {
+ guard(mutex)(&pcf85363->lock);
+
+ err = pcf85363_collect_events(pcf85363);
+ if (err < 0) {
+ dev_err_ratelimited(&pcf85363->rtc->dev,
+ "failed to collect events: %d\n",
+ err);
+ ack_failed = true;
+ } else if (err) {
+ handled = true;
+ }
+ }
+
+ /*
+ * Clear flags this handler does not service (e.g. A2F/PIF); otherwise
+ * they hold the level-triggered INTA line asserted and storm the IRQ.
+ */
+ if (flags & (FLAGS_A2F | FLAGS_PIF)) {
+ err = pcf85363_clear_flags(pcf85363, FLAGS_A2F | FLAGS_PIF);
+ if (err) {
+ dev_err_ratelimited(&pcf85363->rtc->dev,
+ "failed to clear unhandled flags: %d\n",
+ err);
+ ack_failed = true;
+ } else {
+ handled = true;
+ }
+ }
+
+ /*
+ * If any asserted source could not be serviced or cleared, the INTA
+ * line stays asserted; return IRQ_NONE so the spurious-IRQ detector can
+ * disable a permanently stuck line instead of looping forever.
+ */
+ if (ack_failed)
+ return IRQ_NONE;
+
+ return handled ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static int pcf85363_rtc_ioctl(struct device *dev,
+ unsigned int cmd, unsigned long arg)
+{
+ struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+ int ret;
+
+ switch (cmd) {
+ case RTC_VL_READ: {
+ u32 status = 0;
+
+ guard(mutex)(&pcf85363->lock);
+
+ /* Refresh so a poll-only setup still latches the BSF flag. */
+ ret = pcf85363_collect_events(pcf85363);
+ if (ret < 0)
+ return ret;
+
+ if (pcf85363->bsf)
+ status |= RTC_VL_BACKUP_SWITCH;
+
+ return put_user(status, (u32 __user *)arg);
}
- return IRQ_NONE;
+ case RTC_VL_CLR: {
+ guard(mutex)(&pcf85363->lock);
+
+ ret = pcf85363_clear_flags(pcf85363, FLAGS_BSF);
+ if (ret)
+ return ret;
+
+ pcf85363->bsf = false;
+
+ return 0;
+ }
+
+ default:
+ return -ENOIOCTLCMD;
+ }
}
static const struct rtc_class_ops rtc_ops = {
+ .ioctl = pcf85363_rtc_ioctl,
.read_time = pcf85363_rtc_read_time,
.set_time = pcf85363_rtc_set_time,
.read_alarm = pcf85363_rtc_read_alarm,
@@ -412,6 +550,10 @@ static int pcf85363_probe(struct i2c_client *client)
if (!pcf85363)
return -ENOMEM;
+ ret = devm_mutex_init(&client->dev, &pcf85363->lock);
+ if (ret)
+ return ret;
+
pcf85363->regmap = devm_regmap_init_i2c(client, &config->regmap);
if (IS_ERR(pcf85363->regmap)) {
dev_err(&client->dev, "regmap allocation failed\n");
@@ -435,17 +577,52 @@ static int pcf85363_probe(struct i2c_client *client)
wakeup_source = device_property_read_bool(&client->dev,
"wakeup-source");
- if (client->irq > 0 || wakeup_source) {
- err = regmap_write(pcf85363->regmap, CTRL_FLAGS, 0);
- if (err)
- return dev_err_probe(&client->dev, err,
- "failed to clear flags\n");
- err = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
+ /*
+ * Latch and clear any battery-switch event that occurred before
+ * probe (for example while the main supply was off) instead of
+ * blanket clearing CTRL_FLAGS, so the cached state is preserved.
+ */
+ scoped_guard(mutex, &pcf85363->lock) {
+ ret = pcf85363_collect_events(pcf85363);
+ if (ret < 0)
+ return dev_err_probe(&client->dev, ret,
+ "Failed to latch boot-time events\n");
+ }
+
+ /*
+ * Battery-backed registers can retain stale state across a power cycle.
+ * A stale asserted flag would storm the level-triggered INTA line, so
+ * disable only the interrupt sources and Alarm2 enables this driver
+ * never services and clear their flags; leave the managed sources
+ * (A1IE, BSIE) and Alarm1 for their own paths to arm. WDIE is masked
+ * too so a bootloader-armed watchdog cannot storm INTA before the
+ * driver is ready to service it.
+ */
+ ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN,
+ INT_WDIE | INT_A2IE | INT_OIE | INT_PIE | INT_ILP,
+ 0);
+ if (ret)
+ return dev_err_probe(&client->dev, ret,
+ "Failed to mask unused INTA sources\n");
+
+ ret = regmap_update_bits(pcf85363->regmap, DT_ALARM_EN,
+ ALRM_MIN_A2E | ALRM_HR_A2E | ALRM_DAY_A2E, 0);
+ if (ret)
+ return dev_err_probe(&client->dev, ret,
+ "Failed to mask Alarm2 enables\n");
+
+ ret = pcf85363_clear_flags(pcf85363, FLAGS_A2F | FLAGS_PIF);
+ if (ret)
+ return dev_err_probe(&client->dev, ret,
+ "Failed to clear stale flags\n");
+
+ if (client->irq > 0 || wakeup_source) {
+ ret = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
PIN_IO_INTAPM, PIN_IO_INTA_OUT);
- if (err)
- return dev_err_probe(&client->dev, err,
- "failed to set interrupt pin mode\n");
+ if (ret)
+ return dev_err_probe(&client->dev, ret,
+ "Failed to configure INTA pin\n");
}
if (client->irq > 0) {
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v8 2/5] rtc: pcf85363: support reporting battery switch-over via RTC_VL
2026-10-26 9:47 ` [PATCH v8 2/5] rtc: pcf85363: support reporting battery switch-over via RTC_VL Lakshay Piplani
@ 2026-10-26 10:36 ` sashiko-bot
0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-10-26 10:36 UTC (permalink / raw)
To: Lakshay Piplani, linux-rtc, devicetree, linux-watchdog
Cc: robh, conor+dt, Alexandre Belloni
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rtc: pcf85363: unacknowledged FLAGS_A1F holds INTA low in wakeup-only mode
--
commit 655ad6de5d4c27a6bb5b02bf6ca7a7a0b68d6b38
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
rtc: pcf85363: support reporting battery switch-over via RTC_VL
This commit adds support for reporting and clearing the battery switch-over
flag (FLAGS_BSF) via the RTC_VL_READ and RTC_VL_CLR ioctls, introduces
pcf85363_clear_flags() for write-0-to-clear flag handling, and updates
pcf85363_probe() to selectively mask unused interrupt sources and clear
stale flags instead of zeroing CTRL_FLAGS.
> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index ccc7834e57599..00f37ffcca260 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
[ ... ]
> @@ -435,17 +577,52 @@ static int pcf85363_probe(struct i2c_client *client)
>
> wakeup_source = device_property_read_bool(&client->dev,
> "wakeup-source");
> - if (client->irq > 0 || wakeup_source) {
> - err = regmap_write(pcf85363->regmap, CTRL_FLAGS, 0);
> - if (err)
> - return dev_err_probe(&client->dev, err,
> - "failed to clear flags\n");
>
> - err = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
> + /*
> + * Latch and clear any battery-switch event that occurred before
> + * probe (for example while the main supply was off) instead of
> + * blanket clearing CTRL_FLAGS, so the cached state is preserved.
> + */
> + scoped_guard(mutex, &pcf85363->lock) {
> + ret = pcf85363_collect_events(pcf85363);
> + if (ret < 0)
> + return dev_err_probe(&client->dev, ret,
> + "Failed to latch boot-time events\n");
> + }
> +
> + /*
> + * Battery-backed registers can retain stale state across a power cycle.
> + * A stale asserted flag would storm the level-triggered INTA line, so
> + * disable only the interrupt sources and Alarm2 enables this driver
> + * never services and clear their flags; leave the managed sources
> + * (A1IE, BSIE) and Alarm1 for their own paths to arm. WDIE is masked
> + * too so a bootloader-armed watchdog cannot storm INTA before the
> + * driver is ready to service it.
> + */
> + ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN,
> + INT_WDIE | INT_A2IE | INT_OIE | INT_PIE | INT_ILP,
> + 0);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "Failed to mask unused INTA sources\n");
> +
> + ret = regmap_update_bits(pcf85363->regmap, DT_ALARM_EN,
> + ALRM_MIN_A2E | ALRM_HR_A2E | ALRM_DAY_A2E, 0);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "Failed to mask Alarm2 enables\n");
> +
> + ret = pcf85363_clear_flags(pcf85363, FLAGS_A2F | FLAGS_PIF);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "Failed to clear stale flags\n");
> +
> + if (client->irq > 0 || wakeup_source) {
> + ret = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
> PIN_IO_INTAPM, PIN_IO_INTA_OUT);
> - if (err)
> - return dev_err_probe(&client->dev, err,
> - "failed to set interrupt pin mode\n");
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "Failed to configure INTA pin\n");
> }
[Severity: Medium]
On boards configured with wakeup-source where client->irq <= 0 (for example,
when INTA is wired to a PMIC or power-on circuit rather than a dedicated SoC
IRQ, or when devm_request_threaded_irq() fails and clears client->irq),
can leaving FLAGS_A1F uncleared in pcf85363_probe() keep the level-low INTA
pin permanently asserted after an alarm wakes or boots the system?
Previously, pcf85363_probe() cleared all flags in CTRL_FLAGS when
client->irq > 0 || wakeup_source, which acknowledged a boot-time FLAGS_A1F
event and released INTA.
With this change, pcf85363_probe() only clears FLAGS_BSF (via
pcf85363_collect_events()) and FLAGS_A2F | FLAGS_PIF, leaving INT_A1IE and
FLAGS_A1F asserted. When client->irq <= 0, pcf85363_rtc_handle_irq() is not
registered to clear FLAGS_A1F. In addition, devm_rtc_register_device() ->
rtc_initialize_alarm() -> __rtc_read_alarm() -> pcf85363_rtc_read_alarm()
reads alrm->enabled = 1 with tm_year = -1, which rolls the expired alarm
forward to the next year and enqueues rtc->aie_timer without touching the
hardware registers, leaving INTA driven low and causing wakealarm_store() to
return -EBUSY unless the alarm is explicitly disabled or overwritten.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261026094712.205214-1-lakshay.piplani@nxp.com?part=2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v8 3/5] rtc: pcf85363: add timestamp support with configurable timestamp mode
2026-10-26 9:47 [PATCH v8 1/5] dt-bindings: rtc: nxp,pcf85363: add timestamp mode config Lakshay Piplani
2026-10-26 9:47 ` [PATCH v8 2/5] rtc: pcf85363: support reporting battery switch-over via RTC_VL Lakshay Piplani
@ 2026-10-26 9:47 ` Lakshay Piplani
2026-10-26 10:52 ` sashiko-bot
2026-10-26 9:47 ` [PATCH v8 4/5] rtc: pcf85363: add oscillator offset calibration support Lakshay Piplani
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Lakshay Piplani @ 2026-10-26 9:47 UTC (permalink / raw)
To: alexandre.belloni, linux-rtc, linux-kernel, robh, krzk+dt,
conor+dt, devicetree, wim, linux-watchdog, linux
Cc: vikash.bansal, priyanka.jain, Lakshay Piplani
Add support for the timestamp capture registers on PCF85263/PCF85363,
which latch the time on selected events (TS pin or battery switch-over).
The capture source is selected via the nxp,timestamp-mode device tree
property and latched values are exported through read-only sysfs
attributes, refreshed on read so poll-only boards still latch events.
The two capture families need different handling: first-event modes hold
the value until the flag is cleared, so it is cached before clearing;
last-event modes overwrite on every event, so the flag is cleared first
and the block re-read until two reads agree.
While at it, register the attributes with rtc_add_group(), preserve
CTRL_INTA_EN with regmap_update_bits() instead of overwriting it, only
clear RTC_FEATURE_ALARM when no interrupt is available, and make the
nvmem_config array probe-local since it is patched per-device.
Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
---
V7 -> V8:
- Classify the timestamp capture modes from the value read back from
TSR_mode instead of from the device tree property, so a last-event
mode left configured by firmware still gets the torn-read handling
and the timestamp interrupt when "nxp,timestamp-mode" is absent.
V6 -> V7:
- Separate data errors from acknowledgement failures in the shared
IRQ policy from patch 2: a last-event flag clear is the
acknowledgement, so the source is marked handled once the clear
succeeds and a later read failure is logged and skipped instead of
reported as an ack failure.
- Leave a first-event flag asserted on a read failure so a later IRQ
or sysfs read can retry instead of dropping the event.
V5 -> V6:
- Return -EPROBE_DEFER from probe when the IRQ request is deferred
instead of silently disabling alarms; on any other failure warn and
keep RTC timekeeping alive with interrupt-driven alarms disabled
- Configure the INTA pin and keep RTC_FEATURE_ALARM for
wakeup-source-only boards (INTA routed to a PMIC) so rtcwake still
works even without our own IRQ line
V4 -> V5:
- Handle first-event (cache before clearing) and last-event (clear, then
re-read until stable, -EAGAIN on a torn read) capture modes separately,
and propagate register-access errors, surfaced on the sysfs read path.
- Refresh the cache on sysfs read so poll-only boards still latch events.
- Treat a failed DT_TS_MODE write as fatal, and drive the TS pin only for
the TS-pin modes: set TSPM and leave TSIM at its reset default.
- Preserve CTRL_INTA_EN via regmap_update_bits(); enable INT_TSRIE only
when a timestamp mode is configured, and check the INTA writes.
- Clear RTC_FEATURE_ALARM only when no interrupt is available.
- Make the nvmem_config array probe-local, mask the reserved seconds bit,
and don't hardcode IRQF_TRIGGER_LOW for a firmware-node interrupt.
V3 -> V4:
- No changes in v4.
V2 -> V3:
- Split into separate patches as suggested:
- Battery switch-over detection.
- Timestamp recording for TS pin and battery switch-over events.
- Offset calibration.
- Watchdog timer (to be reviewed by watchdog maintainers).
- Dropped Alarm2 support
- Switched to rtc_add_group() for sysfs attributes
V1 -> V2:
- Watchdog related changes due to removal of vendor specific properties
from device tree
* remove vendor DT knobs (enable/timeout/stepsize/repeat)
* use watchdog_init_timeout (with 10s default)
* derive clock_sel from final timeout
* default, repeat=true (repeat mode)
- Fixed uninitalised warning on 'ret' (reported by kernel test robot)
- Use dev_dbg instead of dev_info for debug related print messages
- Minor cleanup and comments.
drivers/rtc/rtc-pcf85363.c | 425 ++++++++++++++++++++++++++++++++-----
1 file changed, 372 insertions(+), 53 deletions(-)
diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index 00f37ffcca26..db832cad0b0b 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -5,6 +5,10 @@
* Driver for NXP PCF85363 real-time clock.
*
* Copyright (C) 2017 Eric Nelson
+ *
+ * Copyright 2025-2026 NXP
+ * Added support for timestamps, battery switch-over,
+ * watchdog, offset calibration.
*/
#include <linux/module.h>
#include <linux/i2c.h>
@@ -18,6 +22,9 @@
#include <linux/mutex.h>
#include <linux/of.h>
#include <linux/regmap.h>
+#include <linux/string.h>
+
+#include <dt-bindings/rtc/pcf85363-tsr.h>
/*
* Date/Time registers
@@ -102,22 +109,54 @@
#define PIN_IO_INTA_OUT 2
#define PIN_IO_INTA_HIZ 3
+#define PIN_IO_TSPM GENMASK(3, 2)
+#define PIN_IO_TSIM BIT(4)
+
#define OSC_CAP_SEL GENMASK(1, 0)
#define OSC_CAP_6000 0x01
#define OSC_CAP_12500 0x02
#define STOP_EN_STOP BIT(0)
+#define RTCM_BIT BIT(4)
+
#define RESET_CPR 0xa4
#define NVRAM_SIZE 0x40
+#define TSR1_MASK 0x03
+#define TSR2_MASK 0x07
+#define TSR3_MASK 0x03
+#define TSR1_SHIFT 0
+#define TSR2_SHIFT 2
+#define TSR3_SHIFT 6
+/* All implemented TSR_mode fields; bit 5 is not used by the device. */
+#define TS_MODE_MASK ((TSR1_MASK << TSR1_SHIFT) | \
+ (TSR2_MASK << TSR2_SHIFT) | \
+ (TSR3_MASK << TSR3_SHIFT))
+
+#define PCF85363_NUM_TS 3
+/* Bytes latched per timestamp register (sec, min, hour, day, mon, year). */
+#define PCF85363_TS_LEN 6
+/* Bit 7 of the seconds byte is reserved, not time data; mask it off. */
+#define PCF85363_SEC_MASK 0x7F
+#define PCF85363_TS_READ_RETRIES 3
+
+/* Cached timestamp; the flag is cleared once the value is copied here. */
+struct pcf85363_ts {
+ bool valid;
+ u8 regs[PCF85363_TS_LEN];
+};
+
struct pcf85363 {
struct rtc_device *rtc;
struct regmap *regmap;
/* Serialises access to the cached event state below. */
struct mutex lock;
bool bsf;
+ struct pcf85363_ts ts[PCF85363_NUM_TS];
+ /* Per-TSR: true when the register uses a last-event capture mode. */
+ bool ts_last_event[PCF85363_NUM_TS];
};
struct pcf85x63_config {
@@ -306,23 +345,131 @@ static int pcf85363_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
return _pcf85363_rtc_alarm_irq_enable(pcf85363, alrm->enabled);
}
+static const u8 pcf85363_ts_base[PCF85363_NUM_TS] = {
+ DT_TIMESTAMP1, DT_TIMESTAMP2, DT_TIMESTAMP3,
+};
+
+static const u8 pcf85363_ts_flag[PCF85363_NUM_TS] = {
+ FLAGS_TSR1F, FLAGS_TSR2F, FLAGS_TSR3F,
+};
+
+/* Mark which TSRs use a last-event mode; those need torn-read handling. */
+static void pcf85363_classify_ts_modes(struct pcf85363 *pcf85363, u8 tsmode)
+{
+ u8 m1 = (tsmode >> TSR1_SHIFT) & TSR1_MASK;
+ u8 m2 = (tsmode >> TSR2_SHIFT) & TSR2_MASK;
+ u8 m3 = (tsmode >> TSR3_SHIFT) & TSR3_MASK;
+
+ pcf85363->ts_last_event[0] = (m1 == PCF85363_TSR1_LE);
+ pcf85363->ts_last_event[1] = (m2 == PCF85363_TSR2_LB ||
+ m2 == PCF85363_TSR2_LV ||
+ m2 == PCF85363_TSR2_LE);
+ pcf85363->ts_last_event[2] = (m3 == PCF85363_TSR3_LB ||
+ m3 == PCF85363_TSR3_LV);
+}
+
+/*
+ * Last-event registers can change under us; re-read the block until two
+ * consecutive reads agree, then publish the stable value. Reject a torn
+ * read with -EAGAIN once the retry budget is exhausted.
+ */
+static int pcf85363_read_ts_stable(struct pcf85363 *pcf85363, int i)
+{
+ u8 prev[PCF85363_TS_LEN], cur[PCF85363_TS_LEN];
+ int retries, ret;
+
+ ret = regmap_bulk_read(pcf85363->regmap, pcf85363_ts_base[i],
+ prev, PCF85363_TS_LEN);
+ if (ret)
+ return ret;
+
+ for (retries = 0; retries < PCF85363_TS_READ_RETRIES; retries++) {
+ ret = regmap_bulk_read(pcf85363->regmap, pcf85363_ts_base[i],
+ cur, PCF85363_TS_LEN);
+ if (ret)
+ return ret;
+
+ if (!memcmp(prev, cur, PCF85363_TS_LEN)) {
+ memcpy(pcf85363->ts[i].regs, cur, PCF85363_TS_LEN);
+ return 0;
+ }
+
+ memcpy(prev, cur, PCF85363_TS_LEN);
+ }
+
+ return -EAGAIN;
+}
+
/*
- * Latch a pending battery-switch event into the software cache. Runs from
- * the IRQ handler and the RTC_VL_READ path, so it is seen with or without
- * an interrupt line. BSF is cached before it is cleared so it survives
- * re-arming. Caller must hold pcf85363->lock. Returns a negative errno on
- * a register-access failure, otherwise the handled flag mask.
+ * Latch pending events into the cache. First-event modes hold the value
+ * until the flag is cleared, so cache before clearing; last-event modes
+ * overwrite every event, so clear first then read until stable. Runs from
+ * the IRQ and sysfs paths; caller holds the lock. Returns a negative errno
+ * on a register-access failure (including -EAGAIN for a torn last-event
+ * read), otherwise the handled flag mask.
*/
static int pcf85363_collect_events(struct pcf85363 *pcf85363)
{
unsigned int flags;
int handled = 0;
- int ret;
+ int i, ret;
ret = regmap_read(pcf85363->regmap, CTRL_FLAGS, &flags);
if (ret)
return ret;
+ for (i = 0; i < PCF85363_NUM_TS; i++) {
+ if (!(flags & pcf85363_ts_flag[i]))
+ continue;
+
+ if (pcf85363->ts_last_event[i]) {
+ /*
+ * Clearing the flag acknowledges the interrupt, so mark
+ * the source handled as soon as the clear succeeds. A
+ * later data-read failure is a data error, not an
+ * acknowledgment failure: log it and move on to the
+ * remaining flags instead of returning an error.
+ */
+ ret = pcf85363_clear_flags(pcf85363,
+ pcf85363_ts_flag[i]);
+ if (ret)
+ return ret;
+
+ handled |= pcf85363_ts_flag[i];
+
+ ret = pcf85363_read_ts_stable(pcf85363, i);
+ if (ret) {
+ dev_warn_ratelimited(&pcf85363->rtc->dev,
+ "timestamp %d read failed: %d\n",
+ i + 1, ret);
+ continue;
+ }
+
+ pcf85363->ts[i].valid = true;
+ } else {
+ /*
+ * First-event data stays latched while its flag is
+ * set. Leave the flag asserted on a read failure so a
+ * later IRQ or sysfs read can retry.
+ */
+ ret = regmap_bulk_read(pcf85363->regmap,
+ pcf85363_ts_base[i],
+ pcf85363->ts[i].regs,
+ PCF85363_TS_LEN);
+ if (ret)
+ return ret;
+
+ pcf85363->ts[i].valid = true;
+
+ ret = pcf85363_clear_flags(pcf85363,
+ pcf85363_ts_flag[i]);
+ if (ret)
+ return ret;
+
+ handled |= pcf85363_ts_flag[i];
+ }
+ }
+
if (flags & FLAGS_BSF) {
pcf85363->bsf = true;
@@ -349,8 +496,11 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
return IRQ_NONE;
if (flags) {
- dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s\n",
+ dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s%s%s%s\n",
flags, (flags & FLAGS_A1F) ? " [A1F]" : "",
+ (flags & FLAGS_TSR1F) ? " [TSR1F]" : "",
+ (flags & FLAGS_TSR2F) ? " [TSR2F]" : "",
+ (flags & FLAGS_TSR3F) ? " [TSR3F]" : "",
(flags & FLAGS_BSF) ? " [BSF]" : "");
}
@@ -367,7 +517,7 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
}
}
- if (flags & FLAGS_BSF) {
+ if (flags & (FLAGS_TSR1F | FLAGS_TSR2F | FLAGS_TSR3F | FLAGS_BSF)) {
guard(mutex)(&pcf85363->lock);
err = pcf85363_collect_events(pcf85363);
@@ -517,12 +667,88 @@ static const struct pcf85x63_config pcf_85363_config = {
.num_nvram = 2
};
+/* Six BCD bytes; bit 7 of the seconds byte is reserved, not time data. */
+static ssize_t pcf85363_format_timestamp(const u8 *regs, char *buf)
+{
+ struct rtc_time tm;
+
+ tm.tm_sec = bcd2bin(regs[0] & PCF85363_SEC_MASK);
+ tm.tm_min = bcd2bin(regs[1]);
+ tm.tm_hour = bcd2bin(regs[2]);
+ tm.tm_mday = bcd2bin(regs[3]);
+ tm.tm_mon = bcd2bin(regs[4]) - 1;
+ tm.tm_year = bcd2bin(regs[5]) + 100;
+
+ return sysfs_emit(buf, "%04d-%02d-%02d %02d:%02d:%02d\n",
+ tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+ tm.tm_hour, tm.tm_min, tm.tm_sec);
+}
+
+/* Refresh from hardware first so poll-only setups still latch events. */
+static ssize_t pcf85363_timestamp_show(struct device *dev, char *buf,
+ unsigned int index)
+{
+ struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+ int ret;
+
+ guard(mutex)(&pcf85363->lock);
+
+ ret = pcf85363_collect_events(pcf85363);
+ if (ret < 0)
+ return ret;
+
+ if (!pcf85363->ts[index].valid)
+ return sysfs_emit(buf, "00-00-00 00:00:00\n");
+
+ return pcf85363_format_timestamp(pcf85363->ts[index].regs, buf);
+}
+
+static ssize_t timestamp1_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return pcf85363_timestamp_show(dev, buf, 0);
+}
+static DEVICE_ATTR_RO(timestamp1);
+
+static ssize_t timestamp2_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return pcf85363_timestamp_show(dev, buf, 1);
+}
+static DEVICE_ATTR_RO(timestamp2);
+
+static ssize_t timestamp3_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return pcf85363_timestamp_show(dev, buf, 2);
+}
+static DEVICE_ATTR_RO(timestamp3);
+
+static struct attribute *pcf85363_attrs[] = {
+ &dev_attr_timestamp1.attr,
+ &dev_attr_timestamp2.attr,
+ &dev_attr_timestamp3.attr,
+ NULL,
+};
+
+static const struct attribute_group pcf85363_attr_group = {
+ .attrs = pcf85363_attrs,
+};
+
static int pcf85363_probe(struct i2c_client *client)
{
- struct pcf85363 *pcf85363;
const struct pcf85x63_config *config = &pcf_85363_config;
const void *data = of_device_get_match_data(&client->dev);
- static struct nvmem_config nvmem_cfg[] = {
+ struct device *dev = &client->dev;
+ struct pcf85363 *pcf85363;
+ int irq_a = client->irq;
+ bool ts_mode_configured = false;
+ bool wakeup_source;
+ int ret, i, err;
+ u32 tsr_mode[3];
+ unsigned int val;
+
+ struct nvmem_config nvmem_cfg[] = {
{
.name = "pcf85x63-",
.word_size = 1,
@@ -539,29 +765,84 @@ static int pcf85363_probe(struct i2c_client *client)
.reg_write = pcf85363_nvram_write,
},
};
- int ret, i, err;
- bool wakeup_source;
if (data)
config = data;
- pcf85363 = devm_kzalloc(&client->dev, sizeof(struct pcf85363),
- GFP_KERNEL);
+ pcf85363 = devm_kzalloc(&client->dev, sizeof(*pcf85363), GFP_KERNEL);
if (!pcf85363)
return -ENOMEM;
- ret = devm_mutex_init(&client->dev, &pcf85363->lock);
+ ret = devm_mutex_init(dev, &pcf85363->lock);
if (ret)
return ret;
pcf85363->regmap = devm_regmap_init_i2c(client, &config->regmap);
- if (IS_ERR(pcf85363->regmap)) {
- dev_err(&client->dev, "regmap allocation failed\n");
- return PTR_ERR(pcf85363->regmap);
- }
+
+ if (IS_ERR(pcf85363->regmap))
+ return dev_err_probe(dev, PTR_ERR(pcf85363->regmap), "regmap init failed\n");
i2c_set_clientdata(client, pcf85363);
+ ret = regmap_update_bits(pcf85363->regmap, CTRL_FUNCTION, RTCM_BIT, 0);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable RTC mode\n");
+
+ if (!device_property_read_u32_array(dev, "nxp,timestamp-mode", tsr_mode, 3)) {
+ bool ts_pin_used;
+
+ tsr_mode[0] &= TSR1_MASK;
+ tsr_mode[1] &= TSR2_MASK;
+ tsr_mode[2] &= TSR3_MASK;
+
+ val = (tsr_mode[2] << TSR3_SHIFT) |
+ (tsr_mode[1] << TSR2_SHIFT) |
+ (tsr_mode[0] << TSR1_SHIFT);
+
+ ret = regmap_write(pcf85363->regmap, DT_TS_MODE, val);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to write timestamp mode register\n");
+
+ /*
+ * Only the TS-pin capture modes drive the TS pin. Select the
+ * timestamp function (TSPM) for those and leave the input mode
+ * (TSIM) at its reset default rather than forcing the
+ * mechanical-switch detector. TSPM = 11b is the TS pin input
+ * mode, so the field mask doubles as the value to program.
+ */
+ ts_pin_used = tsr_mode[0] == PCF85363_TSR1_FE ||
+ tsr_mode[0] == PCF85363_TSR1_LE ||
+ tsr_mode[1] == PCF85363_TSR2_FE ||
+ tsr_mode[1] == PCF85363_TSR2_LE;
+
+ if (ts_pin_used) {
+ ret = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
+ PIN_IO_TSPM, PIN_IO_TSPM);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to configure TS pin\n");
+ }
+
+ dev_dbg(dev, "Timestamp mode set: TSR1=0x%x TSR2=0x%x TSR3=0x%x\n",
+ tsr_mode[0], tsr_mode[1], tsr_mode[2]);
+ }
+
+ /*
+ * Classify from the effective hardware configuration rather than from
+ * the property, which may be absent while firmware has already
+ * programmed a capture mode. A last-event mode left behind that way
+ * still needs the torn-read handling and the timestamp interrupt.
+ */
+ ret = regmap_read(pcf85363->regmap, DT_TS_MODE, &val);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to read timestamp mode register\n");
+
+ pcf85363_classify_ts_modes(pcf85363, val);
+
+ ts_mode_configured = val & TS_MODE_MASK;
+
pcf85363->rtc = devm_rtc_allocate_device(&client->dev);
if (IS_ERR(pcf85363->rtc))
return PTR_ERR(pcf85363->rtc);
@@ -575,80 +856,118 @@ static int pcf85363_probe(struct i2c_client *client)
pcf85363->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
pcf85363->rtc->range_max = RTC_TIMESTAMP_END_2099;
- wakeup_source = device_property_read_bool(&client->dev,
- "wakeup-source");
+ wakeup_source = device_property_read_bool(dev, "wakeup-source");
/*
- * Latch and clear any battery-switch event that occurred before
- * probe (for example while the main supply was off) instead of
- * blanket clearing CTRL_FLAGS, so the cached state is preserved.
+ * Latch pre-probe events instead of blanket-clearing CTRL_FLAGS, so
+ * pre-existing timestamps and the battery-switch flag are not lost.
*/
scoped_guard(mutex, &pcf85363->lock) {
ret = pcf85363_collect_events(pcf85363);
if (ret < 0)
- return dev_err_probe(&client->dev, ret,
+ return dev_err_probe(dev, ret,
"Failed to latch boot-time events\n");
}
/*
* Battery-backed registers can retain stale state across a power cycle.
* A stale asserted flag would storm the level-triggered INTA line, so
- * disable only the interrupt sources and Alarm2 enables this driver
- * never services and clear their flags; leave the managed sources
- * (A1IE, BSIE) and Alarm1 for their own paths to arm. WDIE is masked
- * too so a bootloader-armed watchdog cannot storm INTA before the
- * driver is ready to service it.
+ * disable the interrupt sources and Alarm2 enables this driver does not
+ * arm here and clear their flags; leave the managed sources (A1IE, BSIE,
+ * TSRIE) and Alarm1 for their own paths to arm. WDIE is masked too so a
+ * bootloader-armed watchdog cannot storm INTA before the driver is ready
+ * to service it.
*/
ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN,
INT_WDIE | INT_A2IE | INT_OIE | INT_PIE | INT_ILP,
0);
if (ret)
- return dev_err_probe(&client->dev, ret,
- "Failed to mask unused INTA sources\n");
+ return dev_err_probe(dev, ret, "Failed to mask unused INTA sources\n");
ret = regmap_update_bits(pcf85363->regmap, DT_ALARM_EN,
ALRM_MIN_A2E | ALRM_HR_A2E | ALRM_DAY_A2E, 0);
if (ret)
- return dev_err_probe(&client->dev, ret,
- "Failed to mask Alarm2 enables\n");
+ return dev_err_probe(dev, ret, "Failed to mask Alarm2 enables\n");
ret = pcf85363_clear_flags(pcf85363, FLAGS_A2F | FLAGS_PIF);
if (ret)
- return dev_err_probe(&client->dev, ret,
- "Failed to clear stale flags\n");
+ return dev_err_probe(dev, ret, "Failed to clear stale flags\n");
- if (client->irq > 0 || wakeup_source) {
- ret = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
- PIN_IO_INTAPM, PIN_IO_INTA_OUT);
- if (ret)
- return dev_err_probe(&client->dev, ret,
- "Failed to configure INTA pin\n");
- }
-
- if (client->irq > 0) {
+ if (irq_a > 0) {
unsigned long irqflags = IRQF_TRIGGER_LOW;
if (dev_fwnode(&client->dev))
irqflags = 0;
- ret = devm_request_threaded_irq(&client->dev, client->irq,
- NULL, pcf85363_rtc_handle_irq,
+
+ ret = devm_request_threaded_irq(dev, irq_a, NULL,
+ pcf85363_rtc_handle_irq,
irqflags | IRQF_ONESHOT,
- "pcf85363", client);
+ "pcf85363-inta", client);
+ /*
+ * Let the driver core retry when the interrupt provider is not
+ * ready yet; only a genuine failure disables interrupt-driven
+ * alarms while leaving RTC timekeeping (and any wakeup-source
+ * path) intact.
+ */
+ if (ret == -EPROBE_DEFER)
+ return ret;
if (ret) {
- dev_warn(&client->dev,
- "unable to request IRQ, alarms disabled\n");
- client->irq = 0;
+ dev_warn(dev, "unable to request IRQ, alarms disabled: %d\n",
+ ret);
+ irq_a = 0;
}
}
- if (client->irq > 0 || wakeup_source) {
- device_init_wakeup(&client->dev, true);
+ if (irq_a > 0 || wakeup_source) {
+ /*
+ * The alarm can be delivered either through our own IRQ line or
+ * via an external wakeup path (INTA routed to a PMIC), so route
+ * INTA to its interrupt output and keep the alarm feature in
+ * both cases; rtcwake relies on it for wakeup-source-only boards.
+ */
+ ret = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
+ PIN_IO_INTAPM, PIN_IO_INTA_OUT);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to configure INTA pin\n");
+
+ if (irq_a > 0) {
+ u8 inta_en = INT_BSIE;
+
+ /*
+ * Enable the timestamp interrupt only when a capture
+ * mode is configured; always enable battery-switch.
+ * Only relevant when we service the IRQ ourselves.
+ */
+ if (ts_mode_configured)
+ inta_en |= INT_TSRIE;
+
+ ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN,
+ INT_BSIE | INT_TSRIE, inta_en);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to enable INTA sources\n");
+ }
+
+ device_init_wakeup(dev, true);
set_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
} else {
+ /*
+ * Neither an interrupt line nor a wakeup source: the alarm
+ * cannot be delivered, so drop the alarm feature.
+ */
clear_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
}
+ dev_set_drvdata(&pcf85363->rtc->dev, pcf85363);
+
+ ret = rtc_add_group(pcf85363->rtc, &pcf85363_attr_group);
+ if (ret)
+ return ret;
+
ret = devm_rtc_register_device(pcf85363->rtc);
+ if (ret)
+ return dev_err_probe(dev, ret, "RTC registration failed\n");
for (i = 0; i < config->num_nvram; i++) {
nvmem_cfg[i].priv = pcf85363;
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v8 3/5] rtc: pcf85363: add timestamp support with configurable timestamp mode
2026-10-26 9:47 ` [PATCH v8 3/5] rtc: pcf85363: add timestamp support with configurable timestamp mode Lakshay Piplani
@ 2026-10-26 10:52 ` sashiko-bot
0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-10-26 10:52 UTC (permalink / raw)
To: Lakshay Piplani, linux-rtc, devicetree, linux-watchdog
Cc: robh, conor+dt, Alexandre Belloni
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] rtc: pcf85363: lost event and swallowed read error in pcf85363_collect_events()
- [Medium] rtc: pcf85363: unmasked INT_BSIE and INT_TSRIE in pcf85363_probe() when no IRQ
--
commit f94f2e86fc8ade444598e079370c102ba8cc732b
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
rtc: pcf85363: add timestamp support with configurable timestamp mode
This commit adds timestamp capture register support for PCF85263/PCF85363,
configuring the capture source via the nxp,timestamp-mode property and
exposing captured timestamps via sysfs attributes.
> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index 00f37ffcca260..db832cad0b0b6 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
[ ... ]
> @@ -306,23 +345,131 @@ static int pcf85363_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
[ ... ]
> /*
> - * Latch a pending battery-switch event into the software cache. Runs from
> - * the IRQ handler and the RTC_VL_READ path, so it is seen with or without
> - * an interrupt line. BSF is cached before it is cleared so it survives
> - * re-arming. Caller must hold pcf85363->lock. Returns a negative errno on
> - * a register-access failure, otherwise the handled flag mask.
> + * Latch pending events into the cache. First-event modes hold the value
> + * until the flag is cleared, so cache before clearing; last-event modes
> + * overwrite every event, so clear first then read until stable. Runs from
> + * the IRQ and sysfs paths; caller holds the lock. Returns a negative errno
> + * on a register-access failure (including -EAGAIN for a torn last-event
> + * read), otherwise the handled flag mask.
> */
> static int pcf85363_collect_events(struct pcf85363 *pcf85363)
> {
> unsigned int flags;
> int handled = 0;
> - int ret;
> + int i, ret;
>
> ret = regmap_read(pcf85363->regmap, CTRL_FLAGS, &flags);
> if (ret)
> return ret;
>
> + for (i = 0; i < PCF85363_NUM_TS; i++) {
> + if (!(flags & pcf85363_ts_flag[i]))
> + continue;
> +
> + if (pcf85363->ts_last_event[i]) {
> + /*
> + * Clearing the flag acknowledges the interrupt, so mark
> + * the source handled as soon as the clear succeeds. A
> + * later data-read failure is a data error, not an
> + * acknowledgment failure: log it and move on to the
> + * remaining flags instead of returning an error.
> + */
> + ret = pcf85363_clear_flags(pcf85363,
> + pcf85363_ts_flag[i]);
> + if (ret)
> + return ret;
> +
> + handled |= pcf85363_ts_flag[i];
> +
> + ret = pcf85363_read_ts_stable(pcf85363, i);
> + if (ret) {
> + dev_warn_ratelimited(&pcf85363->rtc->dev,
> + "timestamp %d read failed: %d\n",
> + i + 1, ret);
> + continue;
> + }
> +
> + pcf85363->ts[i].valid = true;
[Severity: Medium]
When a timestamp register uses a last-event mode (such as PCF85363_TSR1_LE,
PCF85363_TSR2_LB, PCF85363_TSR2_LV, PCF85363_TSR2_LE, PCF85363_TSR3_LB, or
PCF85363_TSR3_LV) and pcf85363_read_ts_stable() fails with -EAGAIN on a torn
read or an I2C error from regmap_bulk_read(), does skipping the error via
continue lose the event and hide the failure from sysfs callers?
In pcf85363_timestamp_show():
ret = pcf85363_collect_events(pcf85363);
if (ret < 0)
return ret;
if (!pcf85363->ts[index].valid)
return sysfs_emit(buf, "00-00-00 00:00:00\n");
return pcf85363_format_timestamp(pcf85363->ts[index].regs, buf);
Because pcf85363_collect_events() swallows the error from
pcf85363_read_ts_stable() and returns the non-negative handled mask
(contrary to the function comment above it), a sysfs read in
pcf85363_timestamp_show() will not see a negative errno and will instead
return "00-00-00 00:00:00" or stale cached data from an earlier event.
Furthermore, because pcf85363_clear_flags() already cleared FLAGS_TSRnF in
CTRL_FLAGS before pcf85363_read_ts_stable() failed, if no further hardware
event arrives to re-assert FLAGS_TSRnF, subsequent calls to
pcf85363_collect_events() will see !(flags & pcf85363_ts_flag[i]) and never
retry reading DT_TIMESTAMPn, leaving the captured timestamp in hardware
unreachable.
[ ... ]
> @@ -575,80 +856,118 @@ static int pcf85363_probe(struct i2c_client *client)
[ ... ]
> ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN,
> INT_WDIE | INT_A2IE | INT_OIE | INT_PIE | INT_ILP,
> 0);
> if (ret)
> - return dev_err_probe(&client->dev, ret,
> - "Failed to mask unused INTA sources\n");
> + return dev_err_probe(dev, ret, "Failed to mask unused INTA sources\n");
[ ... ]
> - if (client->irq > 0 || wakeup_source) {
> - device_init_wakeup(&client->dev, true);
> + if (irq_a > 0 || wakeup_source) {
> + /*
> + * The alarm can be delivered either through our own IRQ line or
> + * via an external wakeup path (INTA routed to a PMIC), so route
> + * INTA to its interrupt output and keep the alarm feature in
> + * both cases; rtcwake relies on it for wakeup-source-only boards.
> + */
> + ret = regmap_update_bits(pcf85363->regmap, CTRL_PIN_IO,
> + PIN_IO_INTAPM, PIN_IO_INTA_OUT);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to configure INTA pin\n");
> +
> + if (irq_a > 0) {
> + u8 inta_en = INT_BSIE;
> +
> + /*
> + * Enable the timestamp interrupt only when a capture
> + * mode is configured; always enable battery-switch.
> + * Only relevant when we service the IRQ ourselves.
> + */
> + if (ts_mode_configured)
> + inta_en |= INT_TSRIE;
> +
> + ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN,
> + INT_BSIE | INT_TSRIE, inta_en);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to enable INTA sources\n");
> + }
[Severity: Medium]
What happens in pcf85363_probe() when irq_a <= 0 (for example on a
wakeup-source-only board where INTA is routed to a PMIC, or if
devm_request_threaded_irq() fails with a non-deferral error) and the
battery-backed CTRL_INTA_EN register already has INT_BSIE or INT_TSRIE set
from firmware or a previous boot?
The earlier CTRL_INTA_EN update only clears INT_WDIE | INT_A2IE | INT_OIE |
INT_PIE | INT_ILP, leaving INT_BSIE and INT_TSRIE untouched, and the block
above only updates INT_BSIE | INT_TSRIE when irq_a > 0. When wakeup_source
is true and irq_a <= 0, CTRL_PIN_IO is configured to PIN_IO_INTA_OUT while
stale INT_BSIE or INT_TSRIE bits remain enabled in CTRL_INTA_EN with no IRQ
handler registered to service and clear them, which could leave the INTA
wakeup line asserted if a battery switch-over or timestamp event occurs.
Should INT_BSIE | INT_TSRIE be cleared when irq_a <= 0?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261026094712.205214-1-lakshay.piplani@nxp.com?part=3
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v8 4/5] rtc: pcf85363: add oscillator offset calibration support
2026-10-26 9:47 [PATCH v8 1/5] dt-bindings: rtc: nxp,pcf85363: add timestamp mode config Lakshay Piplani
2026-10-26 9:47 ` [PATCH v8 2/5] rtc: pcf85363: support reporting battery switch-over via RTC_VL Lakshay Piplani
2026-10-26 9:47 ` [PATCH v8 3/5] rtc: pcf85363: add timestamp support with configurable timestamp mode Lakshay Piplani
@ 2026-10-26 9:47 ` Lakshay Piplani
2026-10-26 10:56 ` sashiko-bot
2026-10-26 9:47 ` [PATCH v8 5/5] rtc: pcf85363: add watchdog support with configurable step size Lakshay Piplani
2026-10-26 10:22 ` [PATCH v8 1/5] dt-bindings: rtc: nxp,pcf85363: add timestamp mode config sashiko-bot
4 siblings, 1 reply; 10+ messages in thread
From: Lakshay Piplani @ 2026-10-26 9:47 UTC (permalink / raw)
To: alexandre.belloni, linux-rtc, linux-kernel, robh, krzk+dt,
conor+dt, devicetree, wim, linux-watchdog, linux
Cc: vikash.bansal, priyanka.jain, Lakshay Piplani
Expose the oscillator offset register of PCF85263/PCF85363 through the
read_offset and set_offset rtc_class_ops callbacks, so userspace can
apply frequency correction for drift compensation.
The offset is in parts per billion (ppb). CTRL_OFFSET is a signed 8-bit
step count whose size depends on the OFFM bit: 2170 ppb/step normal and
2034.5 ppb/step fast, the latter computed with an x10-scaled constant to
keep the arithmetic integer. Values are range-checked before that scaling
so the multiply cannot overflow, and out-of-range values are rejected
with -ERANGE rather than clamped.
Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
---
V7 -> V8:
- No change.
V6 -> V7:
- No change.
V5 -> V6:
- No change
V4 -> V5:
- Report and accept the offset in ppb (per the RTC offset ABI) using the
step selected by OFFM (2170 ppb, or 2034.5 ppb fast via x10 scaling).
- Range-check before the x10 scaling to avoid overflow, and reject
out-of-range values with -ERANGE instead of clamping.
V3 -> V4:
- No changes in v4.
V2 -> V3:
- Split into separate patches as suggested:
- Battery switch-over detection.
- Timestamp recording for TS pin and battery switch-over events.
- Offset calibration.
- Watchdog timer (to be reviewed by watchdog maintainers).
- Dropped Alarm2 support
- Switched to rtc_add_group() for sysfs attributes
V1 -> V2:
- Watchdog related changes due to removal of vendor specific properties
from device tree
* remove vendor DT knobs (enable/timeout/stepsize/repeat)
* use watchdog_init_timeout (with 10s default)
* derive clock_sel from final timeout
* default, repeat=true (repeat mode)
- Fixed uninitalised warning on 'ret' (reported by kernel test robot)
- Use dev_dbg instead of dev_info for debug related print messages
- Minor cleanup and comments.
drivers/rtc/rtc-pcf85363.c | 67 ++++++++++++++++++++++++++++++++++++++
1 file changed, 67 insertions(+)
diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index db832cad0b0b..71b82f92a003 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -115,6 +115,7 @@
#define OSC_CAP_SEL GENMASK(1, 0)
#define OSC_CAP_6000 0x01
#define OSC_CAP_12500 0x02
+#define OSC_OFFM BIT(6)
#define STOP_EN_STOP BIT(0)
@@ -135,6 +136,14 @@
(TSR2_MASK << TSR2_SHIFT) | \
(TSR3_MASK << TSR3_SHIFT))
+#define OFFSET_SIGN_BIT 7
+#define OFFSET_MASK 0xFF
+/* Offset step in ppb; fast mode (OFFM=1) is scaled x10 for integer maths. */
+#define OFFSET_STEP_PPB 2170
+#define OFFSET_STEP_PPB_FAST_X10 20345
+#define OFFSET_PPB_MAX (127 * OFFSET_STEP_PPB)
+#define OFFSET_PPB_MIN (-128 * OFFSET_STEP_PPB)
+
#define PCF85363_NUM_TS 3
/* Bytes latched per timestamp register (sec, min, hour, day, mon, year). */
#define PCF85363_TS_LEN 6
@@ -558,6 +567,62 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
return handled ? IRQ_HANDLED : IRQ_NONE;
}
+/*
+ * CTRL_OFFSET is a signed step count; the step is 2170 ppb (normal) or
+ * 2034.5 ppb (fast/OFFM, scaled x10 to keep the arithmetic integer).
+ */
+static int pcf85363_read_offset(struct device *dev, long *offset)
+{
+ struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+ unsigned int val, osc;
+ long steps;
+ int ret;
+
+ ret = regmap_read(pcf85363->regmap, CTRL_OFFSET, &val);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(pcf85363->regmap, CTRL_OSCILLATOR, &osc);
+ if (ret)
+ return ret;
+
+ steps = sign_extend32(val, OFFSET_SIGN_BIT);
+
+ if (osc & OSC_OFFM)
+ *offset = steps * OFFSET_STEP_PPB_FAST_X10 / 10;
+ else
+ *offset = steps * OFFSET_STEP_PPB;
+
+ return 0;
+}
+
+static int pcf85363_set_offset(struct device *dev, long offset)
+{
+ struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+ unsigned int osc;
+ long steps;
+ int ret;
+
+ ret = regmap_read(pcf85363->regmap, CTRL_OSCILLATOR, &osc);
+ if (ret)
+ return ret;
+
+ /* Range-check before the x10 scaling so the multiply cannot overflow. */
+ if (offset > OFFSET_PPB_MAX || offset < OFFSET_PPB_MIN)
+ return -ERANGE;
+
+ if (osc & OSC_OFFM)
+ steps = DIV_ROUND_CLOSEST(offset * 10, OFFSET_STEP_PPB_FAST_X10);
+ else
+ steps = DIV_ROUND_CLOSEST(offset, OFFSET_STEP_PPB);
+
+ if (steps < -128 || steps > 127)
+ return -ERANGE;
+
+ return regmap_write(pcf85363->regmap, CTRL_OFFSET,
+ steps & OFFSET_MASK);
+}
+
static int pcf85363_rtc_ioctl(struct device *dev,
unsigned int cmd, unsigned long arg)
{
@@ -605,6 +670,8 @@ static const struct rtc_class_ops rtc_ops = {
.read_alarm = pcf85363_rtc_read_alarm,
.set_alarm = pcf85363_rtc_set_alarm,
.alarm_irq_enable = pcf85363_rtc_alarm_irq_enable,
+ .read_offset = pcf85363_read_offset,
+ .set_offset = pcf85363_set_offset,
};
static int pcf85363_nvram_read(void *priv, unsigned int offset, void *val,
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v8 5/5] rtc: pcf85363: add watchdog support with configurable step size
2026-10-26 9:47 [PATCH v8 1/5] dt-bindings: rtc: nxp,pcf85363: add timestamp mode config Lakshay Piplani
` (2 preceding siblings ...)
2026-10-26 9:47 ` [PATCH v8 4/5] rtc: pcf85363: add oscillator offset calibration support Lakshay Piplani
@ 2026-10-26 9:47 ` Lakshay Piplani
2026-10-26 11:09 ` sashiko-bot
2026-10-26 10:22 ` [PATCH v8 1/5] dt-bindings: rtc: nxp,pcf85363: add timestamp mode config sashiko-bot
4 siblings, 1 reply; 10+ messages in thread
From: Lakshay Piplani @ 2026-10-26 9:47 UTC (permalink / raw)
To: alexandre.belloni, linux-rtc, linux-kernel, robh, krzk+dt,
conor+dt, devicetree, wim, linux-watchdog, linux
Cc: vikash.bansal, priyanka.jain, Lakshay Piplani
Add watchdog support to PCF85263/PCF85363 using the watchdog subsystem.
The 5-bit count selects the timeout with a clock step: timeouts up to
30 seconds use the 1 Hz step, longer ones the 0.25 Hz step (max 120 s),
reported back through the watchdog device. One count of margin is added
because the first period after a reload lasts between WDR and WDR-1
counts, so the timeout is never shorter than requested. Select
WATCHDOG_CORE if WATCHDOG so the device can register.
The chip has no reset output; expiry is only signalled via the WDF flag
routed to INTA, so this is an alarm-only watchdog (WDIOF_ALARMONLY). WDIE
is toggled on start/stop with rollback-safe register ordering, expiry is
rate-limited from the IRQ handler, and the watchdog is only registered
when an interrupt line is present. WDF is cleared through the
write-0-to-clear helper.
Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
---
V7 -> V8:
- No change.
V6 -> V7:
- Return IRQ_NONE and rate-limit the message when the WDF flag cannot
be cleared, following the shared IRQ policy from patch 2.
- Factor arming into pcf85363_wdt_arm(): mask WDIE, clear stale WDF,
reload, then enable WDIE so a latched flag cannot assert INTA early.
On .start() a failed WDIE-enable halts the counter (the core only
tracks the device on success); on resume the counter is never halted
as the watchdog may already be active with nowayout set.
- Only mask WDIE in the devres cleanup instead of stopping the counter,
so unbinding a nowayout watchdog does not bypass its stop policy while
still detaching WDF from INTA before the managed IRQ is released.
- Mask WDIE on suspend without halting the counter (honouring nowayout)
and re-arm on resume via pcf85363_wdt_arm(), still paired with
watchdog_stop_ping_on_suspend().
V5 -> V6:
- Adopt an already-running watchdog (bootloader- or battery-backed):
read CTRL_WDOG until two reads agree, set WDOG_HW_RUNNING and re-arm
WDIE when it is running, otherwise leave WDIE masked. Do not decode
the live countdown as the configured timeout; keep the fixed default
and let the core's first keep-alive reload it
- Report watchdog probe failures via dev_err_probe()
V4 -> V5:
- Select WATCHDOG_CORE if WATCHDOG so the device can register.
- Scale the timeout by the clock select with one count of margin (1 Hz
step up to 30 s, else 0.25 Hz, max 120 s) and report it back.
- Alarm-only watchdog (WDIOF_ALARMONLY): expiry is only signalled via WDF
routed to INTA; toggle WDIE on start/stop with rollback-safe ordering.
- Don't program WDR from .set_timeout when inactive (it would start the
watchdog); only reload when watchdog_active(), and drop the clamp.
- Register the watchdog only when an interrupt line is present.
- Use a fixed default timeout (no timeout-sec); userspace can change it.
- Give the watchdog struct a back-pointer to struct pcf85363 and clear
WDF via the shared write-0-to-clear helper.
V3 -> V4:
- Use watchdog_init_timeout(&wd->wdd, 0, dev) to allow devicetree or module parameter overrides;
fallback to WD_DEFAULT_TIMEOUT if not provided.
- Centralized clock selection logic in pcf85363_wdt_select_clock() and applied dynamically
whenever timeout changes.
- Removed unused repeat variable and simplified timeout handling for clarity.
V2 -> V3:
- Split into separate patches as suggested:
- Battery switch-over detection.
- Timestamp recording for TS pin and battery switch-over events.
- Offset calibration.
- Watchdog timer (to be reviewed by watchdog maintainers).
- Dropped Alarm2 support
- Switched to rtc_add_group() for sysfs attributes
V1 -> V2:
- Watchdog related changes due to removal of vendor specific properties
from device tree
* remove vendor DT knobs (enable/timeout/stepsize/repeat)
* use watchdog_init_timeout (with 10s default)
* derive clock_sel from final timeout
* default, repeat=true (repeat mode)
- Fixed uninitalised warning on 'ret' (reported by kernel test robot)
- Use dev_dbg instead of dev_info for debug related print messages
- Minor cleanup and comments.
drivers/rtc/Kconfig | 1 +
drivers/rtc/rtc-pcf85363.c | 392 ++++++++++++++++++++++++++++++++++++-
2 files changed, 391 insertions(+), 2 deletions(-)
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 05b9233b9418..4871cb3fb24d 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -516,6 +516,7 @@ config RTC_DRV_PCF8523
config RTC_DRV_PCF85363
tristate "NXP PCF85363"
select REGMAP_I2C
+ select WATCHDOG_CORE if WATCHDOG
help
If you say yes here you get support for the PCF85363 RTC chip.
diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
index 71b82f92a003..e5ebfad1a2ce 100644
--- a/drivers/rtc/rtc-pcf85363.c
+++ b/drivers/rtc/rtc-pcf85363.c
@@ -21,8 +21,10 @@
#include <linux/device.h>
#include <linux/mutex.h>
#include <linux/of.h>
+#include <linux/pm.h>
#include <linux/regmap.h>
#include <linux/string.h>
+#include <linux/watchdog.h>
#include <dt-bindings/rtc/pcf85363-tsr.h>
@@ -151,12 +153,32 @@
#define PCF85363_SEC_MASK 0x7F
#define PCF85363_TS_READ_RETRIES 3
+#define WD_TIMEOUT_SHIFT 2
+#define WD_CLKSEL_MASK GENMASK(1, 0)
+#define WD_CLKSEL_0_25HZ 0x00
+#define WD_CLKSEL_1HZ 0x01
+#define WD_CLKSEL_4HZ 0x02
+#define WD_CLKSEL_16HZ 0x03
+#define WD_MODE_REPEAT BIT(7)
+
+#define WD_DEFAULT_TIMEOUT 10
+#define WD_TIMEOUT_MIN 1
+#define WD_COUNT_MAX 0x1F
+/* Longest guaranteed timeout at the 0.25 Hz step (WDR=31, one count margin). */
+#define WD_TIMEOUT_MAX 120
+/* Longest timeout served by the 1 Hz step (WDR=31, one count margin). */
+#define WD_TIMEOUT_1HZ_MAX 30
+/* CTRL_WDOG reads back the live countdown; retry until two reads agree. */
+#define WD_READ_RETRIES 3
+
/* Cached timestamp; the flag is cleared once the value is copied here. */
struct pcf85363_ts {
bool valid;
u8 regs[PCF85363_TS_LEN];
};
+struct pcf85363_watchdog;
+
struct pcf85363 {
struct rtc_device *rtc;
struct regmap *regmap;
@@ -166,6 +188,7 @@ struct pcf85363 {
struct pcf85363_ts ts[PCF85363_NUM_TS];
/* Per-TSR: true when the register uses a last-event capture mode. */
bool ts_last_event[PCF85363_NUM_TS];
+ struct pcf85363_watchdog *watchdog;
};
struct pcf85x63_config {
@@ -173,6 +196,14 @@ struct pcf85x63_config {
unsigned int num_nvram;
};
+struct pcf85363_watchdog {
+ struct watchdog_device wdd;
+ struct pcf85363 *pcf85363;
+ u8 timeout_val;
+ u8 clock_sel;
+ bool suspended;
+};
+
/*
* CTRL_FLAGS is write-0-to-clear, so write the complement of the mask to
* clear only the requested bits without disturbing the others.
@@ -505,12 +536,13 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
return IRQ_NONE;
if (flags) {
- dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s%s%s%s\n",
+ dev_dbg(&pcf85363->rtc->dev, "IRQ flags: 0x%02x%s%s%s%s%s%s\n",
flags, (flags & FLAGS_A1F) ? " [A1F]" : "",
(flags & FLAGS_TSR1F) ? " [TSR1F]" : "",
(flags & FLAGS_TSR2F) ? " [TSR2F]" : "",
(flags & FLAGS_TSR3F) ? " [TSR3F]" : "",
- (flags & FLAGS_BSF) ? " [BSF]" : "");
+ (flags & FLAGS_BSF) ? " [BSF]" : "",
+ (flags & FLAGS_WDF) ? " [WDF]" : "");
}
if (flags & FLAGS_A1F) {
@@ -540,6 +572,21 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
}
}
+ if (flags & FLAGS_WDF) {
+ dev_warn_ratelimited(&pcf85363->rtc->dev,
+ "watchdog timer expired\n");
+
+ err = pcf85363_clear_flags(pcf85363, FLAGS_WDF);
+ if (err) {
+ dev_err_ratelimited(&pcf85363->rtc->dev,
+ "failed to clear watchdog flag: %d\n",
+ err);
+ ack_failed = true;
+ } else {
+ handled = true;
+ }
+ }
+
/*
* Clear flags this handler does not service (e.g. A2F/PIF); otherwise
* they hold the level-triggered INTA line asserted and storm the IRQ.
@@ -734,6 +781,287 @@ static const struct pcf85x63_config pcf_85363_config = {
.num_nvram = 2
};
+/*
+ * Program the watchdog counter (WDR) so the reported timeout is never
+ * shorter than requested: the first period after a reload lasts between
+ * WDR and WDR-1 counts, so add one count of margin. Timeouts up to 30 s
+ * use the 1 Hz step (1 s/count); longer ones the 0.25 Hz step (4 s/count).
+ */
+static void pcf85363_wdt_select_clock(struct pcf85363_watchdog *wd)
+{
+ unsigned int timeout = wd->wdd.timeout;
+
+ if (timeout <= WD_TIMEOUT_1HZ_MAX) {
+ wd->clock_sel = WD_CLKSEL_1HZ;
+ wd->timeout_val = timeout + 1;
+ wd->wdd.timeout = timeout;
+ } else {
+ wd->clock_sel = WD_CLKSEL_0_25HZ;
+ wd->timeout_val = DIV_ROUND_UP(timeout, 4) + 1;
+ wd->wdd.timeout = DIV_ROUND_UP(timeout, 4) * 4;
+ }
+}
+
+/* Repeat mode restarts the watchdog automatically after each period. */
+static int pcf85363_wdt_reload(struct pcf85363_watchdog *wd)
+{
+ u8 val;
+
+ val = WD_MODE_REPEAT |
+ ((wd->timeout_val & WD_COUNT_MAX) << WD_TIMEOUT_SHIFT) |
+ (wd->clock_sel & WD_CLKSEL_MASK);
+
+ return regmap_write(wd->pcf85363->regmap, CTRL_WDOG, val);
+}
+
+/*
+ * Arm the watchdog: mask WDIE, clear any stale WDF, reload the counter, then
+ * enable WDIE last so a latched flag cannot assert INTA before the counter is
+ * reloaded. pcf85363_wdt_reload() starts the counter, so if the final WDIE
+ * enable fails the watchdog is already running.
+ *
+ * On the .start() path (rollback) the watchdog core only marks the device
+ * active/hw-running after .start() returns success, so a failure here must
+ * leave the hardware stopped or the running watchdog would be left untracked:
+ * halt the counter and keep WDIE masked before returning the error. On the
+ * resume path (!rollback) the watchdog may already be active - possibly with
+ * nowayout set - so never halt the counter on failure; just report the error.
+ */
+static int pcf85363_wdt_arm(struct pcf85363_watchdog *wd, bool rollback)
+{
+ struct regmap *regmap = wd->pcf85363->regmap;
+ int ret;
+
+ /* Mask WDF from INTA while the watchdog is being re-armed. */
+ ret = regmap_update_bits(regmap, CTRL_INTA_EN, INT_WDIE, 0);
+ if (ret)
+ return ret;
+
+ ret = pcf85363_clear_flags(wd->pcf85363, FLAGS_WDF);
+ if (ret)
+ return ret;
+
+ ret = pcf85363_wdt_reload(wd);
+ if (ret)
+ return ret;
+
+ /* Route WDF to INTA only after the flag is clear and counter reloaded. */
+ ret = regmap_update_bits(regmap, CTRL_INTA_EN, INT_WDIE, INT_WDIE);
+ if (ret && rollback) {
+ /* Counter is running but WDIE enable failed; undo the start. */
+ regmap_write(regmap, CTRL_WDOG, 0);
+ return ret;
+ }
+
+ return ret;
+}
+
+static int pcf85363_wdt_start(struct watchdog_device *wdd)
+{
+ return pcf85363_wdt_arm(watchdog_get_drvdata(wdd), true);
+}
+
+static int pcf85363_wdt_stop(struct watchdog_device *wdd)
+{
+ struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
+ int ret, irq_ret;
+
+ /* Halt the counter first so a failed disable cannot leave it armed. */
+ ret = regmap_write(wd->pcf85363->regmap, CTRL_WDOG, 0);
+
+ /*
+ * Always attempt to mask WDIE. Even if halting the counter failed the
+ * interrupt must not stay routed to INTA after the driver is removed.
+ */
+ irq_ret = regmap_update_bits(wd->pcf85363->regmap, CTRL_INTA_EN,
+ INT_WDIE, 0);
+
+ return ret ? ret : irq_ret;
+}
+
+static int pcf85363_wdt_ping(struct watchdog_device *wdd)
+{
+ struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
+ int ret;
+
+ ret = pcf85363_clear_flags(wd->pcf85363, FLAGS_WDF);
+ if (ret)
+ return ret;
+
+ return pcf85363_wdt_reload(wd);
+}
+
+static int pcf85363_wdt_set_timeout(struct watchdog_device *wdd,
+ unsigned int timeout)
+{
+ struct pcf85363_watchdog *wd = watchdog_get_drvdata(wdd);
+
+ wdd->timeout = timeout;
+
+ pcf85363_wdt_select_clock(wd);
+
+ /*
+ * Programming the counter with a non-zero value starts it, so only
+ * reprogram when the watchdog is already running; the core keeps the
+ * new timeout for the next start otherwise.
+ */
+ if (!watchdog_active(wdd))
+ return 0;
+
+ return pcf85363_wdt_reload(wd);
+}
+
+static const struct watchdog_info pcf85363_wdt_info = {
+ .identity = "PCF85363 Watchdog",
+ .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT | WDIOF_ALARMONLY,
+};
+
+static const struct watchdog_ops pcf85363_wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = pcf85363_wdt_start,
+ .stop = pcf85363_wdt_stop,
+ .ping = pcf85363_wdt_ping,
+ .set_timeout = pcf85363_wdt_set_timeout,
+};
+
+/*
+ * CTRL_WDOG reads back the live countdown, which decrements while the
+ * watchdog runs, so a single read can catch a transient value. Read until
+ * two consecutive reads agree before deciding whether the watchdog is armed.
+ */
+static int pcf85363_read_wdog_stable(struct pcf85363 *pcf85363, unsigned int *out)
+{
+ unsigned int prev, cur;
+ int retries, ret;
+
+ ret = regmap_read(pcf85363->regmap, CTRL_WDOG, &prev);
+ if (ret)
+ return ret;
+
+ for (retries = 0; retries < WD_READ_RETRIES; retries++) {
+ ret = regmap_read(pcf85363->regmap, CTRL_WDOG, &cur);
+ if (ret)
+ return ret;
+
+ if (cur == prev) {
+ *out = cur;
+ return 0;
+ }
+
+ prev = cur;
+ }
+
+ *out = cur;
+ return 0;
+}
+
+/*
+ * Registered as a devres action before the watchdog device, so on unbind it
+ * runs after watchdog unregister but before the managed IRQ is released.
+ *
+ * Mask WDIE only; do not halt the counter. Halting the counter is the
+ * nowayout-governed stop that the watchdog core performs on unregister (and
+ * deliberately refuses when nowayout is set). The core only stops a WDOG_ACTIVE
+ * device, so an adopted WDOG_HW_RUNNING watchdog userspace never opened, or an
+ * active nowayout watchdog, is still counting here. Masking WDIE disconnects
+ * WDF from the level-triggered INTA line so it cannot storm the interrupt once
+ * the handler is freed, in every case, without bypassing nowayout.
+ */
+static void pcf85363_wdt_cleanup(void *data)
+{
+ struct pcf85363_watchdog *wd = data;
+ int ret;
+
+ ret = regmap_update_bits(wd->pcf85363->regmap, CTRL_INTA_EN,
+ INT_WDIE, 0);
+ if (ret)
+ dev_warn(wd->wdd.parent,
+ "failed to mask watchdog interrupt during cleanup: %d\n",
+ ret);
+}
+
+static int pcf85363_watchdog_init(struct device *dev, struct pcf85363 *pcf85363)
+{
+ struct pcf85363_watchdog *wd;
+ unsigned int regval;
+ bool running;
+ int ret;
+
+ if (!IS_ENABLED(CONFIG_WATCHDOG))
+ return 0;
+
+ wd = devm_kzalloc(dev, sizeof(*wd), GFP_KERNEL);
+ if (!wd)
+ return -ENOMEM;
+
+ wd->pcf85363 = pcf85363;
+
+ wd->wdd.info = &pcf85363_wdt_info;
+ wd->wdd.ops = &pcf85363_wdt_ops;
+ wd->wdd.min_timeout = WD_TIMEOUT_MIN;
+ wd->wdd.max_timeout = WD_TIMEOUT_MAX;
+ wd->wdd.timeout = WD_DEFAULT_TIMEOUT;
+ wd->wdd.parent = dev;
+ wd->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
+
+ /*
+ * Fixed default timeout; userspace can change it via WDIOC_SETTIMEOUT.
+ * CTRL_WDOG reads back the live countdown, not the programmed reload
+ * value, so it must not be decoded into wdd.timeout: adopt the known
+ * default and let the watchdog core's first keep-alive reload it.
+ */
+ pcf85363_wdt_select_clock(wd);
+
+ /*
+ * The watchdog state is battery-backed and may already be running at
+ * probe. Adopt it so the watchdog core keeps it serviced; only a
+ * stopped watchdog gets WDIE masked so a stale enable cannot storm the
+ * level-triggered INTA line.
+ */
+ ret = pcf85363_read_wdog_stable(pcf85363, ®val);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to read watchdog register\n");
+
+ running = ((regval >> WD_TIMEOUT_SHIFT) & WD_COUNT_MAX) != 0;
+
+ ret = pcf85363_clear_flags(pcf85363, FLAGS_WDF);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to clear WDF\n");
+
+ /*
+ * Once WDOG_HW_RUNNING is set the core keeps the watchdog alive via
+ * .ping(), which does not touch WDIE, so arm WDIE here for an adopted
+ * watchdog; otherwise keep it masked.
+ */
+ ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN, INT_WDIE,
+ running ? INT_WDIE : 0);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to configure WDIE\n");
+
+ if (running)
+ set_bit(WDOG_HW_RUNNING, &wd->wdd.status);
+
+ watchdog_set_drvdata(&wd->wdd, wd);
+ watchdog_stop_on_unregister(&wd->wdd);
+ watchdog_stop_ping_on_suspend(&wd->wdd);
+
+ /*
+ * Register the cleanup before the watchdog device so devres unwinds it
+ * after watchdog unregister but before the managed IRQ is freed.
+ */
+ ret = devm_add_action_or_reset(dev, pcf85363_wdt_cleanup, wd);
+ if (ret)
+ return ret;
+
+ ret = devm_watchdog_register_device(dev, &wd->wdd);
+ if (ret)
+ return ret;
+
+ pcf85363->watchdog = wd;
+
+ return 0;
+}
+
/* Six BCD bytes; bit 7 of the seconds byte is reserved, not time data. */
static ssize_t pcf85363_format_timestamp(const u8 *regs, char *buf)
{
@@ -1026,6 +1354,16 @@ static int pcf85363_probe(struct i2c_client *client)
clear_bit(RTC_FEATURE_ALARM, pcf85363->rtc->features);
}
+ /*
+ * Watchdog expiry is only signalled via INTA (no reset output), so it
+ * needs an interrupt line to be usable.
+ */
+ if (irq_a > 0) {
+ ret = pcf85363_watchdog_init(dev, pcf85363);
+ if (ret)
+ return dev_err_probe(dev, ret, "Watchdog init failed\n");
+ }
+
dev_set_drvdata(&pcf85363->rtc->dev, pcf85363);
ret = rtc_add_group(pcf85363->rtc, &pcf85363_attr_group);
@@ -1044,6 +1382,55 @@ static int pcf85363_probe(struct i2c_client *client)
return ret;
}
+/*
+ * The watchdog only signals expiry through INTA, which is a system wakeup
+ * source, so a running watchdog would fire a spurious wakeup during sleep.
+ * Mask WDIE on suspend to suppress that wakeup without halting the counter,
+ * so nowayout is honoured, and re-arm it on resume; the RTC alarm can still
+ * wake the system.
+ */
+static int pcf85363_suspend(struct device *dev)
+{
+ struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+ struct pcf85363_watchdog *wd = pcf85363->watchdog;
+ int ret;
+
+ if (!wd || !(watchdog_active(&wd->wdd) || watchdog_hw_running(&wd->wdd)))
+ return 0;
+
+ /* Disconnect WDF from the wakeup-capable INTA line; keep counting. */
+ ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN, INT_WDIE, 0);
+ if (ret)
+ return ret;
+
+ wd->suspended = true;
+
+ return 0;
+}
+
+static int pcf85363_resume(struct device *dev)
+{
+ struct pcf85363 *pcf85363 = dev_get_drvdata(dev);
+ struct pcf85363_watchdog *wd = pcf85363->watchdog;
+ int ret;
+
+ if (!wd || !wd->suspended)
+ return 0;
+
+ /*
+ * The watchdog may already be active - possibly with nowayout set - so
+ * re-arm without ever halting the counter on failure.
+ */
+ ret = pcf85363_wdt_arm(wd, false);
+ if (!ret)
+ wd->suspended = false;
+
+ return ret;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(pcf85363_pm_ops, pcf85363_suspend,
+ pcf85363_resume);
+
static const __maybe_unused struct of_device_id dev_ids[] = {
{ .compatible = "nxp,pcf85263", .data = &pcf_85263_config },
{ .compatible = "nxp,pcf85363", .data = &pcf_85363_config },
@@ -1055,6 +1442,7 @@ static struct i2c_driver pcf85363_driver = {
.driver = {
.name = "pcf85363",
.of_match_table = of_match_ptr(dev_ids),
+ .pm = pm_sleep_ptr(&pcf85363_pm_ops),
},
.probe = pcf85363_probe,
};
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v8 5/5] rtc: pcf85363: add watchdog support with configurable step size
2026-10-26 9:47 ` [PATCH v8 5/5] rtc: pcf85363: add watchdog support with configurable step size Lakshay Piplani
@ 2026-10-26 11:09 ` sashiko-bot
0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-10-26 11:09 UTC (permalink / raw)
To: Lakshay Piplani, linux-rtc, devicetree, linux-watchdog
Cc: robh, conor+dt, Alexandre Belloni
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rtc: pcf85363: spurious WDF handling in pcf85363_rtc_handle_irq()
--
commit 16cc51c19e1316f4895386e586c6d2ab87d77232
Author: Lakshay Piplani <lakshay.piplani@nxp.com>
rtc: pcf85363: add watchdog support with configurable step size
This commit adds alarm-only watchdog support to the PCF85263/PCF85363 RTC
driver using the watchdog subsystem, with dynamic 1 Hz and 0.25 Hz step
selection, interrupt handling via INTA, and suspend/resume support.
> diff --git a/drivers/rtc/rtc-pcf85363.c b/drivers/rtc/rtc-pcf85363.c
> index 71b82f92a0031..e5ebfad1a2cea 100644
> --- a/drivers/rtc/rtc-pcf85363.c
> +++ b/drivers/rtc/rtc-pcf85363.c
[ ... ]
> @@ -540,6 +572,21 @@ static irqreturn_t pcf85363_rtc_handle_irq(int irq, void *dev_id)
> }
> }
>
> + if (flags & FLAGS_WDF) {
> + dev_warn_ratelimited(&pcf85363->rtc->dev,
> + "watchdog timer expired\n");
> +
> + err = pcf85363_clear_flags(pcf85363, FLAGS_WDF);
> + if (err) {
> + dev_err_ratelimited(&pcf85363->rtc->dev,
> + "failed to clear watchdog flag: %d\n",
> + err);
> + ack_failed = true;
> + } else {
> + handled = true;
> + }
> + }
[Severity: Medium]
Can pcf85363_rtc_handle_irq() in drivers/rtc/rtc-pcf85363.c log false
"watchdog timer expired" warnings and return spurious IRQ_HANDLED on a
shared interrupt line when FLAGS_WDF is latched in CTRL_FLAGS while
INT_WDIE is masked in CTRL_INTA_EN?
On PCF85263/PCF85363, CTRL_FLAGS latches FLAGS_WDF whenever the CTRL_WDOG
counter reaches 0, regardless of whether INT_WDIE is set in CTRL_INTA_EN
(which only controls routing of WDF to the INTA pin).
In pcf85363_suspend(), INT_WDIE is masked while the CTRL_WDOG counter is
left running in repeat mode:
/* Disconnect WDF from the wakeup-capable INTA line; keep counting. */
ret = regmap_update_bits(pcf85363->regmap, CTRL_INTA_EN, INT_WDIE, 0);
if (ret)
return ret;
wd->suspended = true;
During any system sleep longer than the watchdog timeout, the counter
expires and latches FLAGS_WDF. On wakeup, resume_device_irqs() in
dpm_resume_noirq() re-enables device interrupts before dpm_resume() runs
pcf85363_resume(). If an RTC alarm (FLAGS_A1F), timestamp (FLAGS_TSR1F,
FLAGS_TSR2F, FLAGS_TSR3F), battery switch-over (FLAGS_BSF), or another
device on a shared IRQ line fires before pcf85363_resume() runs,
pcf85363_rtc_handle_irq() executes while wd->suspended is still true and
INT_WDIE is still masked, reporting a false watchdog expiry and setting
handled = true.
A similar state can also occur in two other paths:
1. In pcf85363_probe(), INT_WDIE is masked and only FLAGS_A2F | FLAGS_PIF
are cleared before devm_request_threaded_irq() is called. When
CONFIG_WATCHDOG=n, pcf85363_watchdog_init() returns early without clearing
a stale FLAGS_WDF or halting a bootloader- or battery-backed CTRL_WDOG
counter.
2. In pcf85363_wdt_stop(), CTRL_WDOG is halted and INT_WDIE is masked
without clearing any FLAGS_WDF that latched concurrently (or that latches
later if writing CTRL_WDOG fails while masking INT_WDIE succeeds).
Should pcf85363_rtc_handle_irq() ignore (or silently clear without setting
handled = true or logging a warning) FLAGS_WDF when the watchdog interrupt
is not armed, and should pcf85363_probe() and pcf85363_wdt_stop() also
ensure FLAGS_WDF is cleared when masking INT_WDIE?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261026094712.205214-1-lakshay.piplani@nxp.com?part=5
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v8 1/5] dt-bindings: rtc: nxp,pcf85363: add timestamp mode config
2026-10-26 9:47 [PATCH v8 1/5] dt-bindings: rtc: nxp,pcf85363: add timestamp mode config Lakshay Piplani
` (3 preceding siblings ...)
2026-10-26 9:47 ` [PATCH v8 5/5] rtc: pcf85363: add watchdog support with configurable step size Lakshay Piplani
@ 2026-10-26 10:22 ` sashiko-bot
4 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-10-26 10:22 UTC (permalink / raw)
To: Lakshay Piplani, linux-rtc; +Cc: Alexandre Belloni
> NXP PCF85263/PCF85363 provides three timestamp registers (TSR1-TSR3)
> which latch the current time when a selected event occurs. Add a
> vendor specific property, nxp,timestamp-mode, to select the event
> source for each register.
>
> Also introduce a new header 'pcf85363-tsr.h' to expose
> macros for timestamp mode fields, improving readability
> of device tree file.
>
> Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
Sashiko has reviewed this patch and found no issues.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261026094712.205214-1-lakshay.piplani@nxp.com?part=1
^ permalink raw reply [flat|nested] 10+ messages in thread