diff mbox series

[3/4] rtc: pcf8525: Add watchdog support

Message ID 20260807082855.3762394-4-shivprakash.gupta@nxp.com
State New
Headers show
Series rtc: pcf8525: Add NXP PCF8525 RTC driver | expand

Commit Message

Shiv Prakash Gupta Aug. 7, 2026, 8:28 a.m. UTC
Add watchdog support for NXP PCF8525 real time clock(RTC) device

Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
Signed-off-by: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>
---
 drivers/rtc/Kconfig       |   1 +
 drivers/rtc/rtc-pcf8525.c | 245 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 244 insertions(+), 2 deletions(-)

Comments

sashiko-bot@kernel.org Aug. 7, 2026, 8:52 a.m. UTC | #1
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [Critical] Unmasking watchdog interrupt without clearing persistent WDTF flag causes inescapable boot loop.
- [High] Watchdog IRQ handler silently clears timeout flag, aborting hardware resets and defeating the watchdog.
- [High] Missing WDOG_HW_RUNNING reporting causes spurious system resets if the watchdog is armed by the bootloader.
- [High] Requesting INTB without IRQF_SHARED breaks probe on boards tying INTA and INTB together.
- [Medium] Watchdog initialization occurs after RTC registration, creating a race condition exposing the uninitialized device.

Pre-existing issues:
- [High] Read-modify-write on CTRL2 causes lost hardware interrupts for RTC alarm and watchdog.
--

--- Patch [3]: [PATCH 3/4] rtc: pcf8525: Add watchdog support ---
commit 3cb38601cbc74987657b19d7480456e2da5babc7
Author: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>

rtc: pcf8525: Add watchdog support

This commit adds watchdog support for the NXP PCF8525 real time clock device.

> diff --git a/drivers/rtc/rtc-pcf8525.c b/drivers/rtc/rtc-pcf8525.c
> index 7dfb4f0f64047..4ef648bdfc5b9 100644
> --- a/drivers/rtc/rtc-pcf8525.c
> +++ b/drivers/rtc/rtc-pcf8525.c
[ ... ]
> +static irqreturn_t pcf8525_wdt_irq(int irq, void *data)
> +{
> +	struct device *dev = data;
> +	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
> +	unsigned int ctrl2;
> +	int ret;
> +
> +	ret = regmap_read(pcf8525->regmap, PCF8525_REG_CTRL2, &ctrl2);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	if (!(ctrl2 & PCF8525_CTRL2_WDTF))
> +		return IRQ_NONE;
> +
> +	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL2,
> +				 PCF8525_CTRL2_WDTF, 0);

[Severity: High]
Does returning IRQ_HANDLED here without notifying the watchdog core or
invoking panic() silently abort the hardware reset? If INTB is routed to
a reset controller, de-asserting it before the reset cycle completes might
leave the system in a hung state rather than resetting it.

[Severity: High]
This isn't a bug introduced by this patch, but this read-modify-write
on the CTRL2 register extends a pre-existing race condition to the
watchdog handler thread.

CTRL2 contains write-0-to-clear flags like AF, MSF, and WDTF. Because
regmap_update_bits() reads the register and writes it back, if a hardware
flag asserts after the read but before the write, could this write back
a 0 and accidentally clear the newly asserted flag?

This same pattern exists in pcf8525_irq() when clearing AF, which could
cause lost hardware interrupts.

> +	if (ret)
> +		return IRQ_NONE;
> +
> +	return IRQ_HANDLED;
> +}
[ ... ]
> +static int pcf8525_watchdog_config(struct device *dev,
> +				   struct pcf8525 *pcf8525)
> +{
[ ... ]
> +	m1 |= PCF8525_MASK1_BIE |
> +	      PCF8525_MASK1_AIE |
> +	      PCF8525_MASK1_OSIE |
> +	      PCF8525_MASK1_SI |
> +	      PCF8525_MASK1_MI;
> +	m1 &= ~PCF8525_MASK1_WD_CD;
> +
> +	ret = regmap_write(pcf8525->regmap, PCF8525_REG_INTB_MASK1, m1);

[Severity: Critical]
If the watchdog resets the system, the battery-backed RTC retains the WDTF
(Watchdog Timeout Flag) set to 1. On reboot, when this unmasks the watchdog
interrupt on the INTB pin, will it immediately assert INTB since WDTF is
still 1? This regression could trigger the hardware reset again and cause
an inescapable boot loop.

> +	if (ret)
> +		return ret;
[ ... ]
> +static int pcf8525_watchdog_init(struct device *dev,
> +				 struct pcf8525 *pcf8525)
> +{
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_WATCHDOG_CORE) ||
> +	    !device_property_read_bool(dev, "reset-source"))
> +		return 0;
> +
> +	if (pcf8525->irq_intb > 0) {
> +		ret = devm_request_threaded_irq(dev, pcf8525->irq_intb,
> +						NULL, pcf8525_wdt_irq,
> +						IRQF_ONESHOT,
> +						"pcf8525-wdt", dev);

[Severity: High]
Many hardware designs tie open-drain I2C interrupts (INTA and INTB) to a
single SoC GPIO. If they are mapped to the same IRQ, will this request fail
because it lacks IRQF_SHARED and uses the same dev_id token (dev)?
This could result in a -EBUSY error and cause the entire device probe
to fail.

[ ... ]
> +	pcf8525->wdd.timeout = PCF8525_WD_DEFAULT_TIMEOUT_S;
> +	watchdog_init_timeout(&pcf8525->wdd, 0, dev);
> +	pcf8525->wdd.min_hw_heartbeat_ms = PCF8525_WD_MIN_HW_HEARTBEAT_MS;
> +	pcf8525->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
> +
> +	watchdog_set_drvdata(&pcf8525->wdd, pcf8525);
> +	watchdog_stop_on_reboot(&pcf8525->wdd);
> +
> +	return devm_watchdog_register_device(dev, &pcf8525->wdd);

[Severity: High]
If the bootloader starts the watchdog, does unconditionally setting the
status without checking if the hardware timer is active cause issues?
Because it fails to set WDOG_HW_RUNNING, the watchdog core will not
initiate the ping worker, which might allow the timer to expire and
cause a spurious system reboot during boot.

[ ... ]
> @@ -902,6 +1139,10 @@ static int pcf8525_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	ret = pcf8525_watchdog_init(dev, pcf8525);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }

[Severity: Medium]
Could initializing the watchdog after RTC registration expose the RTC
device to userspace prematurely? If pcf8525_watchdog_init() fails,
probe will return an error and devres will unregister the RTC,
which might cause operations on the opened device to fail.
Julian Braha Aug. 14, 2026, 4:08 p.m. UTC | #2
Hi Shiv,

On 8/7/26 09:28, Shiv Prakash Gupta wrote:
> Add watchdog support for NXP PCF8525 real time clock(RTC) device
> 
> Signed-off-by: Lakshay Piplani <lakshay.piplani@nxp.com>
> Signed-off-by: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>
> ---
>  drivers/rtc/Kconfig       |   1 +
>  drivers/rtc/rtc-pcf8525.c | 245 +++++++++++++++++++++++++++++++++++++-
>  2 files changed, 244 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 3c22c7253aae..6c1c7b3d27b6 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -516,6 +516,7 @@ config RTC_DRV_PCF8523
>  config RTC_DRV_PCF8525
>          tristate "NXP PCF8525"
>          select REGMAP_I2C
> +	select WATCHDOG_CORE if WATCHDOG
>          help
Nitpick, sorry: this Kconfig file uses spaces for indentation, but your
addition uses a tab.

- Julian Braha
diff mbox series

Patch

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 3c22c7253aae..6c1c7b3d27b6 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -516,6 +516,7 @@  config RTC_DRV_PCF8523
 config RTC_DRV_PCF8525
         tristate "NXP PCF8525"
         select REGMAP_I2C
+	select WATCHDOG_CORE if WATCHDOG
         help
           If you say yes here you get support for the NXP PCF8525 RTC
           chips.
diff --git a/drivers/rtc/rtc-pcf8525.c b/drivers/rtc/rtc-pcf8525.c
index 7dfb4f0f6404..4ef648bdfc5b 100644
--- a/drivers/rtc/rtc-pcf8525.c
+++ b/drivers/rtc/rtc-pcf8525.c
@@ -45,6 +45,8 @@ 
 #include <linux/slab.h>
 #include <linux/bitfield.h>
 #include <linux/uaccess.h>
+#include <linux/kernel.h>
+#include <linux/watchdog.h>
 
 /* Registers */
 #define PCF8525_REG_CTRL1		0x00
@@ -94,6 +96,8 @@ 
 #define PCF8525_REG_INTB_MASK1		0x2A
 #define PCF8525_REG_INTB_MASK2		0x2B
 
+#define PCF8525_REG_WD_CTL              0x2C
+#define PCF8525_REG_WD_VAL              0x2D
 #define PCF8525_REG_TEMP		0x2E
 
 /* CTRL1 */
@@ -104,6 +108,7 @@ 
 /* CTRL2 */
 #define PCF8525_CTRL2_MSF		BIT(7)
 #define PCF8525_CTRL2_TI_TP		BIT(6)
+#define PCF8525_CTRL2_WDTF              BIT(5)
 #define PCF8525_CTRL2_AF		BIT(4)
 #define PCF8525_CTRL2_OSFE_MASK		GENMASK(3, 2)
 #define PCF8525_CTRL2_AIE		BIT(1)
@@ -120,6 +125,9 @@ 
 #define PCF8525_CTRL5_CL		BIT(1)
 #define PCF8525_CTRL5_XTL_TYP		BIT(0)
 
+/* CLKOUT_ctl */
+#define PCF8525_CLKOUT_CLKOE            BIT(3)
+
 /* Timestamp control */
 #define PCF8525_TS_CTL1_TSM             BIT(7)
 #define PCF8525_TS_CTL1_TSOFF           BIT(6)
@@ -138,6 +146,7 @@ 
 #define PCF8525_RESET_SR_CMD		0x2C /* software reset */
 
 /* Interrupt masks (mask bit = 1 means masked/disabled) - INTA_MASK1 bits */
+#define PCF8525_MASK1_WD_CD            BIT(0)
 #define PCF8525_MASK1_BIE		BIT(1)
 #define PCF8525_MASK1_AIE		BIT(2)
 #define PCF8525_MASK1_OSIE		BIT(3)
@@ -148,6 +157,14 @@ 
 
 #define PCF8525_ALM_AE_MONTH		BIT(7)
 #define PCF8525_ALM_AE_YEAR		BIT(6)
+#define PCF8525_WD_CTL_WD_CD            BIT(7)
+#define PCF8525_WD_CTL_TF0              BIT(0)
+#define PCF8525_WD_CTL_TF1              BIT(1)
+
+#define PCF8525_WD_CLOCK_HZ_X1000       250 /* 1/4 Hz */
+#define PCF8525_WD_MIN_HW_HEARTBEAT_MS  4000
+#define PCF8525_WD_VAL_STOP             0
+#define PCF8525_WD_DEFAULT_TIMEOUT_S    60
 
 #define PCF8525_REG_AGING_OFFSET_HI	0x26
 #define PCF8525_REG_AGING_OFFSET_LO	0x27
@@ -163,10 +180,13 @@ 
 
 struct pcf8525 {
 	struct rtc_device *rtc;
+	struct watchdog_device wdd;
 	struct regmap *regmap;
 	bool irq_enabled;
 	time64_t ts[2];
 	bool ts_valid[2];
+	int irq_inta;
+	int irq_intb;
 };
 
 static const struct regmap_config pcf8525_regmap_cfg = {
@@ -175,6 +195,218 @@  static const struct regmap_config pcf8525_regmap_cfg = {
 	.max_register = PCF8525_REG_TEMP,
 };
 
+static unsigned int pcf8525_wdt_timeout_to_val(unsigned int timeout)
+{
+	unsigned int val;
+
+	val = DIV_ROUND_UP(timeout * PCF8525_WD_CLOCK_HZ_X1000, 1000) + 1;
+
+	if (val < 2)
+		return 2;
+	if (val > 255)
+		return 255;
+
+	return val;
+}
+
+static int pcf8525_wdt_ping(struct watchdog_device *wdd)
+{
+	struct pcf8525 *pcf8525 = watchdog_get_drvdata(wdd);
+	unsigned int wd_val;
+
+	wd_val = pcf8525_wdt_timeout_to_val(wdd->timeout);
+
+	return regmap_write(pcf8525->regmap, PCF8525_REG_WD_VAL, wd_val);
+}
+
+static int pcf8525_wdt_active_ping(struct watchdog_device *wdd)
+{
+	if (watchdog_active(wdd))
+		return pcf8525_wdt_ping(wdd);
+
+	return 0;
+}
+
+static int pcf8525_wdt_start(struct watchdog_device *wdd)
+{
+	return pcf8525_wdt_ping(wdd);
+}
+
+static int pcf8525_wdt_stop(struct watchdog_device *wdd)
+{
+	struct pcf8525 *pcf8525 = watchdog_get_drvdata(wdd);
+
+	return regmap_write(pcf8525->regmap, PCF8525_REG_WD_VAL,
+				    PCF8525_WD_VAL_STOP);
+}
+
+static int pcf8525_wdt_set_timeout(struct watchdog_device *wdd,
+				   unsigned int timeout)
+{
+	wdd->timeout = timeout;
+
+	return pcf8525_wdt_active_ping(wdd);
+}
+
+static const struct watchdog_info pcf8525_wdt_info = {
+	.identity = "NXP PCF8525 Watchdog",
+	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+};
+
+static const struct watchdog_ops pcf8525_watchdog_ops = {
+	.owner = THIS_MODULE,
+	.start = pcf8525_wdt_start,
+	.stop = pcf8525_wdt_stop,
+	.ping = pcf8525_wdt_ping,
+	.set_timeout = pcf8525_wdt_set_timeout,
+};
+
+static int pcf8525_watchdog_get_period(int n, int f1000)
+{
+	return (1000 * (n - 1)) / f1000;
+}
+
+static irqreturn_t pcf8525_wdt_irq(int irq, void *data)
+{
+	struct device *dev = data;
+	struct pcf8525 *pcf8525 = dev_get_drvdata(dev);
+	unsigned int ctrl2;
+	int ret;
+
+	ret = regmap_read(pcf8525->regmap, PCF8525_REG_CTRL2, &ctrl2);
+	if (ret)
+		return IRQ_NONE;
+
+	if (!(ctrl2 & PCF8525_CTRL2_WDTF))
+		return IRQ_NONE;
+
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CTRL2,
+				 PCF8525_CTRL2_WDTF, 0);
+	if (ret)
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+static int pcf8525_watchdog_config(struct device *dev,
+				   struct pcf8525 *pcf8525)
+{
+	unsigned int m1, m2;
+	int ret;
+
+	/* Configure nINTB/CLKOUT as nINTB open-drain interrupt output. */
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_CLKOUT,
+				 PCF8525_CLKOUT_CLKOE, 0);
+	if (ret)
+		return ret;
+
+	/* Enable watchdog interrupt and select 1/4 Hz source: TF[1:0] = 10. */
+	ret = regmap_update_bits(pcf8525->regmap, PCF8525_REG_WD_CTL,
+				 PCF8525_WD_CTL_WD_CD |
+				 PCF8525_WD_CTL_TF1 |
+				 PCF8525_WD_CTL_TF0,
+				 PCF8525_WD_CTL_WD_CD |
+				 PCF8525_WD_CTL_TF1);
+	if (ret)
+		return ret;
+
+	/* Keep watchdog masked on INTA. */
+	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTA_MASK1, &m1);
+	if (ret)
+		return ret;
+
+	m1 |= PCF8525_MASK1_WD_CD;
+	ret = regmap_write(pcf8525->regmap, PCF8525_REG_INTA_MASK1, m1);
+	if (ret)
+		return ret;
+
+	/* Route watchdog only to INTB, and keep RTC interrupts masked on INTB. */
+	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTB_MASK1, &m1);
+	if (ret)
+		return ret;
+
+	m1 |= PCF8525_MASK1_BIE |
+	      PCF8525_MASK1_AIE |
+	      PCF8525_MASK1_OSIE |
+	      PCF8525_MASK1_SI |
+	      PCF8525_MASK1_MI;
+	m1 &= ~PCF8525_MASK1_WD_CD;
+
+	ret = regmap_write(pcf8525->regmap, PCF8525_REG_INTB_MASK1, m1);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(pcf8525->regmap, PCF8525_REG_INTB_MASK2, &m2);
+	if (ret)
+		return ret;
+
+	m2 |= PCF8525_MASK2_TSIE;
+
+	return regmap_write(pcf8525->regmap, PCF8525_REG_INTB_MASK2, m2);
+}
+
+static int pcf8525_watchdog_init(struct device *dev,
+				 struct pcf8525 *pcf8525)
+{
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_WATCHDOG_CORE) ||
+	    !device_property_read_bool(dev, "reset-source"))
+		return 0;
+
+	if (pcf8525->irq_intb > 0) {
+		ret = devm_request_threaded_irq(dev, pcf8525->irq_intb,
+						NULL, pcf8525_wdt_irq,
+						IRQF_ONESHOT,
+						"pcf8525-wdt", dev);
+		if (ret)
+			return ret;
+	}
+
+	ret = pcf8525_watchdog_config(dev, pcf8525);
+	if (ret)
+		return ret;
+
+	pcf8525->wdd.parent = dev;
+	pcf8525->wdd.info = &pcf8525_wdt_info;
+	pcf8525->wdd.ops = &pcf8525_watchdog_ops;
+	pcf8525->wdd.min_timeout = pcf8525_watchdog_get_period(2,
+							       PCF8525_WD_CLOCK_HZ_X1000);
+	pcf8525->wdd.max_timeout = pcf8525_watchdog_get_period(255,
+							       PCF8525_WD_CLOCK_HZ_X1000);
+	pcf8525->wdd.timeout = PCF8525_WD_DEFAULT_TIMEOUT_S;
+	watchdog_init_timeout(&pcf8525->wdd, 0, dev);
+	pcf8525->wdd.min_hw_heartbeat_ms = PCF8525_WD_MIN_HW_HEARTBEAT_MS;
+	pcf8525->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
+
+	watchdog_set_drvdata(&pcf8525->wdd, pcf8525);
+	watchdog_stop_on_reboot(&pcf8525->wdd);
+
+	return devm_watchdog_register_device(dev, &pcf8525->wdd);
+}
+
+static int pcf8525_get_irqs(struct i2c_client *client, struct pcf8525 *pcf8525)
+{
+	struct device *dev = &client->dev;
+	int irq;
+
+	irq = fwnode_irq_get_byname(dev_fwnode(dev), "inta");
+	if (irq == -ENOENT || irq == -EINVAL)
+		irq = client->irq;
+	else if (irq < 0)
+		return irq;
+	pcf8525->irq_inta = irq;
+
+	irq = fwnode_irq_get_byname(dev_fwnode(dev), "intb");
+	if (irq == -ENOENT || irq == -EINVAL)
+		irq = 0;
+	else if (irq < 0)
+		return irq;
+	pcf8525->irq_intb = irq;
+
+	return 0;
+}
+
 /*
  * Configure only the crystal fields explicitly provided by firmware.
  * If a property is absent, preserve the current hardware setting.
@@ -809,6 +1041,7 @@  static int pcf8525_unmask_irqs_intA(struct pcf8525 *pcf8525)
 	if (ret)
 		return ret;
 
+	m1 |= PCF8525_MASK1_WD_CD;
 	m1 &= ~PCF8525_MASK1_AIE;    /* unmask alarm interrupt on INTA */
 	m2 &= ~PCF8525_MASK2_TSIE;   /* unmask timestamp interrupt on INTA */
 
@@ -836,6 +1069,10 @@  static int pcf8525_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, pcf8525);
 
+	ret = pcf8525_get_irqs(client, pcf8525);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to get IRQs\n");
+
 	pcf8525->rtc = devm_rtc_allocate_device(dev);
 	if (IS_ERR(pcf8525->rtc))
 		return dev_err_probe(dev, PTR_ERR(pcf8525->rtc),
@@ -877,8 +1114,8 @@  static int pcf8525_probe(struct i2c_client *client)
 		return ret;
 
 	/* Optional IRQ */
-	if (client->irq > 0) {
-		ret = devm_request_threaded_irq(dev, client->irq,
+	if (pcf8525->irq_inta > 0) {
+		ret = devm_request_threaded_irq(dev, pcf8525->irq_inta,
 						NULL, pcf8525_irq,
 						IRQF_ONESHOT,
 						dev_name(dev), dev);
@@ -902,6 +1139,10 @@  static int pcf8525_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	ret = pcf8525_watchdog_init(dev, pcf8525);
+	if (ret)
+		return ret;
+
 	return 0;
 }