[v2,4/5] rtc: pcf2127: add watchdog feature support
diff mbox series

Message ID 20190813153600.12406-5-bruno.thomsen@gmail.com
State New
Headers show
Series
  • rtc: pcf2127: tamper timestamp and watchdog feature support
Related show

Commit Message

Bruno Thomsen Aug. 13, 2019, 3:35 p.m. UTC
Add partial support for the watchdog functionality of
both PCF2127 and PCF2129 chips.

The programmable watchdog timer is currently using a fixed
clock source of 1Hz. This result in a selectable range of
1-255 seconds, which covers most embedded Linux use-cases.

Clock sources of 4096Hz, 64Hz and 1/60Hz is mostly useful
in MCU use-cases.

Countdown timer not available when using watchdog feature.

Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
---
 drivers/rtc/Kconfig       |   7 ++-
 drivers/rtc/rtc-pcf2127.c | 127 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 133 insertions(+), 1 deletion(-)

Comments

Guenter Roeck Aug. 13, 2019, 4:19 p.m. UTC | #1
On Tue, Aug 13, 2019 at 05:35:59PM +0200, Bruno Thomsen wrote:
> Add partial support for the watchdog functionality of
> both PCF2127 and PCF2129 chips.
> 
> The programmable watchdog timer is currently using a fixed
> clock source of 1Hz. This result in a selectable range of
> 1-255 seconds, which covers most embedded Linux use-cases.
> 
> Clock sources of 4096Hz, 64Hz and 1/60Hz is mostly useful
> in MCU use-cases.
> 
> Countdown timer not available when using watchdog feature.
> 
> Signed-off-by: Bruno Thomsen <bruno.thomsen@gmail.com>
> ---
>  drivers/rtc/Kconfig       |   7 ++-
>  drivers/rtc/rtc-pcf2127.c | 127 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 133 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index e72f65b61176..a3bb58a08879 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -876,7 +876,12 @@ config RTC_DRV_PCF2127
>  	depends on RTC_I2C_AND_SPI
>  	help
>  	  If you say yes here you get support for the NXP PCF2127/29 RTC
> -	  chips.
> +	  chips with integrated quartz crystal for industrial applications.
> +	  Both chips also have watchdog timer and tamper switch detection
> +	  features.
> +
> +	  PCF2127 has an additional feature of 512 bytes battery backed
> +	  memory that's accessible using nvmem interface.
>  
>  	  This driver can also be built as a module. If so, the module
>  	  will be called rtc-pcf2127.
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index ee4921e4a47c..700db7dd3eef 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -5,6 +5,9 @@
>   *
>   * Author: Renaud Cerrato <r.cerrato@til-technologies.fr>
>   *
> + * Watchdog and tamper functions
> + * Author: Bruno Thomsen <bruno.thomsen@gmail.com>
> + *
>   * based on the other drivers in this same directory.
>   *
>   * Datasheet: http://cache.nxp.com/documents/data_sheet/PCF2127.pdf
> @@ -18,6 +21,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/regmap.h>
> +#include <linux/watchdog.h>
>  
>  /* Control register 1 */
>  #define PCF2127_REG_CTRL1		0x00
> @@ -35,6 +39,13 @@
>  #define PCF2127_REG_DW			0x07
>  #define PCF2127_REG_MO			0x08
>  #define PCF2127_REG_YR			0x09
> +/* Watchdog registers */
> +#define PCF2127_REG_WD_CTL		0x10
> +#define PCF2127_BIT_WD_CTL_TF0			BIT(0)
> +#define PCF2127_BIT_WD_CTL_TF1			BIT(1)
> +#define PCF2127_BIT_WD_CTL_CD0			BIT(6)
> +#define PCF2127_BIT_WD_CTL_CD1			BIT(7)
> +#define PCF2127_REG_WD_VAL		0x11
>  /*
>   * RAM registers
>   * PCF2127 has 512 bytes general-purpose static RAM (SRAM) that is
> @@ -45,9 +56,15 @@
>  #define PCF2127_REG_RAM_WRT_CMD		0x1C
>  #define PCF2127_REG_RAM_RD_CMD		0x1D
>  
> +/* Watchdog timer value constants */
> +#define PCF2127_WD_VAL_STOP		0
> +#define PCF2127_WD_VAL_MIN		2
> +#define PCF2127_WD_VAL_MAX		255
> +#define PCF2127_WD_VAL_DEFAULT		60
>  
>  struct pcf2127 {
>  	struct rtc_device *rtc;
> +	struct watchdog_device wdd;
>  	struct regmap *regmap;
>  };
>  
> @@ -220,6 +237,81 @@ static int pcf2127_nvmem_write(void *priv, unsigned int offset,
>  	return ret ?: bytes;
>  }
>  
> +/* watchdog driver */
> +
> +static int pcf2127_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct pcf2127 *pcf2127 = watchdog_get_drvdata(wdd);
> +
> +	return regmap_write(pcf2127->regmap, PCF2127_REG_WD_VAL, wdd->timeout);
> +}
> +
> +/*
> + * Restart watchdog timer if feature is active.
> + *
> + * Note: Reading CTRL2 register causes watchdog to stop which is unfortunate,
> + * since register also contain control/status flags for other features.
> + * Always call this function after reading CTRL2 register.
> + */
> +static int pcf2127_wdt_active_ping(struct watchdog_device *wdd)
> +{
> +	int ret = 0;
> +
> +	if (watchdog_active(wdd)) {
> +		ret = pcf2127_wdt_ping(wdd);
> +		if (ret)
> +			dev_err(wdd->parent,
> +				"%s: watchdog restart failed, ret=%d\n",
> +				__func__, ret);
> +	}
> +
> +	return ret;
> +}
> +
> +static int pcf2127_wdt_start(struct watchdog_device *wdd)
> +{
> +	dev_info(wdd->parent, "watchdog enabled\n");
> +
> +	return pcf2127_wdt_ping(wdd);
> +}
> +
> +static int pcf2127_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct pcf2127 *pcf2127 = watchdog_get_drvdata(wdd);
> +
> +	dev_info(wdd->parent, "watchdog disabled\n");
> +

There is a lot of noise in this driver. Please reconsider.

Guenter

> +	return regmap_write(pcf2127->regmap, PCF2127_REG_WD_VAL,
> +			    PCF2127_WD_VAL_STOP);
> +}
> +
> +static int pcf2127_wdt_set_timeout(struct watchdog_device *wdd,
> +				   unsigned int new_timeout)
> +{
> +	int ret = 0;
> +
> +	dev_info(wdd->parent, "new watchdog timeout: %is (old: %is)\n",
> +		new_timeout, wdd->timeout);
> +
> +	wdd->timeout = new_timeout;
> +	ret = pcf2127_wdt_active_ping(wdd);
> +
> +	return ret;
> +}
> +
> +static const struct watchdog_info pcf2127_wdt_info = {
> +	.identity = "NXP PCF2127/PCF2129 Watchdog",
> +	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
> +};
> +
> +static const struct watchdog_ops pcf2127_watchdog_ops = {
> +	.owner = THIS_MODULE,
> +	.start = pcf2127_wdt_start,
> +	.stop = pcf2127_wdt_stop,
> +	.ping = pcf2127_wdt_ping,
> +	.set_timeout = pcf2127_wdt_set_timeout,
> +};
> +
>  static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  			const char *name, bool has_nvmem)
>  {
> @@ -242,6 +334,16 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  
>  	pcf2127->rtc->ops = &pcf2127_rtc_ops;
>  
> +	pcf2127->wdd.parent = dev;
> +	pcf2127->wdd.info = &pcf2127_wdt_info;
> +	pcf2127->wdd.ops = &pcf2127_watchdog_ops;
> +	pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
> +	pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
> +	pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
> +	pcf2127->wdd.min_hw_heartbeat_ms = 500;
> +
> +	watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
> +
>  	if (has_nvmem) {
>  		struct nvmem_config nvmem_cfg = {
>  			.priv = pcf2127,
> @@ -253,6 +355,31 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  		ret = rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
>  	}
>  
> +	/*
> +	 * Watchdog timer enabled and reset pin /RST activated when timed out.
> +	 * Select 1Hz clock source for watchdog timer.
> +	 * Timer is not started until WD_VAL is loaded with a valid value.
> +	 * Note: Countdown timer disabled and not available.
> +	 */
> +	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
> +				 PCF2127_BIT_WD_CTL_CD1 |
> +				 PCF2127_BIT_WD_CTL_CD0 |
> +				 PCF2127_BIT_WD_CTL_TF1 |
> +				 PCF2127_BIT_WD_CTL_TF0,
> +				 PCF2127_BIT_WD_CTL_CD1 |
> +				 PCF2127_BIT_WD_CTL_CD0 |
> +				 PCF2127_BIT_WD_CTL_TF1);
> +	if (ret) {
> +		dev_err(dev, "%s: watchdog config (wd_ctl) failed\n", __func__);
> +		return ret;
> +	}
> +
> +	ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
> +	if (ret) {
> +		dev_err(dev, "%s: watchdog registering failed\n", __func__);
> +		return ret;
> +	}
> +
>  	return rtc_register_device(pcf2127->rtc);
>  }
>  
> -- 
> 2.21.0
>
Bruno Thomsen Aug. 14, 2019, 1:25 p.m. UTC | #2
Hi Guenter

Thanks for the quick review.

Den tir. 13. aug. 2019 kl. 18.19 skrev Guenter Roeck <linux@roeck-us.net>:
>
> On Tue, Aug 13, 2019 at 05:35:59PM +0200, Bruno Thomsen wrote:
> > +static int pcf2127_wdt_start(struct watchdog_device *wdd)
> > +{
> > +     dev_info(wdd->parent, "watchdog enabled\n");
> > +
> > +     return pcf2127_wdt_ping(wdd);
> > +}
> > +
> > +static int pcf2127_wdt_stop(struct watchdog_device *wdd)
> > +{
> > +     struct pcf2127 *pcf2127 = watchdog_get_drvdata(wdd);
> > +
> > +     dev_info(wdd->parent, "watchdog disabled\n");
> > +
>
> There is a lot of noise in this driver. Please reconsider.

Would it be better if I remove the following lines:

dev_info(wdd->parent, "watchdog enabled\n");
dev_info(wdd->parent, "watchdog disabled\n");
dev_err(dev, "%s: watchdog registering failed\n", __func__);

and change this line to a dev_dbg():

dev_info(wdd->parent, "new watchdog timeout: %is (old: %is)\n",
         new_timeout, wdd->timeout);

?

Just checking so I don't waste your time on v3 review.

/Bruno
Guenter Roeck Aug. 14, 2019, 1:34 p.m. UTC | #3
On Wed, Aug 14, 2019 at 03:25:55PM +0200, Bruno Thomsen wrote:
> Hi Guenter
> 
> Thanks for the quick review.
> 
> Den tir. 13. aug. 2019 kl. 18.19 skrev Guenter Roeck <linux@roeck-us.net>:
> >
> > On Tue, Aug 13, 2019 at 05:35:59PM +0200, Bruno Thomsen wrote:
> > > +static int pcf2127_wdt_start(struct watchdog_device *wdd)
> > > +{
> > > +     dev_info(wdd->parent, "watchdog enabled\n");
> > > +
> > > +     return pcf2127_wdt_ping(wdd);
> > > +}
> > > +
> > > +static int pcf2127_wdt_stop(struct watchdog_device *wdd)
> > > +{
> > > +     struct pcf2127 *pcf2127 = watchdog_get_drvdata(wdd);
> > > +
> > > +     dev_info(wdd->parent, "watchdog disabled\n");
> > > +
> >
> > There is a lot of noise in this driver. Please reconsider.
> 
> Would it be better if I remove the following lines:
> 
> dev_info(wdd->parent, "watchdog enabled\n");
> dev_info(wdd->parent, "watchdog disabled\n");
> dev_err(dev, "%s: watchdog registering failed\n", __func__);
> 
> and change this line to a dev_dbg():
> 
> dev_info(wdd->parent, "new watchdog timeout: %is (old: %is)\n",
>          new_timeout, wdd->timeout);
> 
> ?
> 
Yes.

Thanks,
Guenter

Patch
diff mbox series

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index e72f65b61176..a3bb58a08879 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -876,7 +876,12 @@  config RTC_DRV_PCF2127
 	depends on RTC_I2C_AND_SPI
 	help
 	  If you say yes here you get support for the NXP PCF2127/29 RTC
-	  chips.
+	  chips with integrated quartz crystal for industrial applications.
+	  Both chips also have watchdog timer and tamper switch detection
+	  features.
+
+	  PCF2127 has an additional feature of 512 bytes battery backed
+	  memory that's accessible using nvmem interface.
 
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-pcf2127.
diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index ee4921e4a47c..700db7dd3eef 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -5,6 +5,9 @@ 
  *
  * Author: Renaud Cerrato <r.cerrato@til-technologies.fr>
  *
+ * Watchdog and tamper functions
+ * Author: Bruno Thomsen <bruno.thomsen@gmail.com>
+ *
  * based on the other drivers in this same directory.
  *
  * Datasheet: http://cache.nxp.com/documents/data_sheet/PCF2127.pdf
@@ -18,6 +21,7 @@ 
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/regmap.h>
+#include <linux/watchdog.h>
 
 /* Control register 1 */
 #define PCF2127_REG_CTRL1		0x00
@@ -35,6 +39,13 @@ 
 #define PCF2127_REG_DW			0x07
 #define PCF2127_REG_MO			0x08
 #define PCF2127_REG_YR			0x09
+/* Watchdog registers */
+#define PCF2127_REG_WD_CTL		0x10
+#define PCF2127_BIT_WD_CTL_TF0			BIT(0)
+#define PCF2127_BIT_WD_CTL_TF1			BIT(1)
+#define PCF2127_BIT_WD_CTL_CD0			BIT(6)
+#define PCF2127_BIT_WD_CTL_CD1			BIT(7)
+#define PCF2127_REG_WD_VAL		0x11
 /*
  * RAM registers
  * PCF2127 has 512 bytes general-purpose static RAM (SRAM) that is
@@ -45,9 +56,15 @@ 
 #define PCF2127_REG_RAM_WRT_CMD		0x1C
 #define PCF2127_REG_RAM_RD_CMD		0x1D
 
+/* Watchdog timer value constants */
+#define PCF2127_WD_VAL_STOP		0
+#define PCF2127_WD_VAL_MIN		2
+#define PCF2127_WD_VAL_MAX		255
+#define PCF2127_WD_VAL_DEFAULT		60
 
 struct pcf2127 {
 	struct rtc_device *rtc;
+	struct watchdog_device wdd;
 	struct regmap *regmap;
 };
 
@@ -220,6 +237,81 @@  static int pcf2127_nvmem_write(void *priv, unsigned int offset,
 	return ret ?: bytes;
 }
 
+/* watchdog driver */
+
+static int pcf2127_wdt_ping(struct watchdog_device *wdd)
+{
+	struct pcf2127 *pcf2127 = watchdog_get_drvdata(wdd);
+
+	return regmap_write(pcf2127->regmap, PCF2127_REG_WD_VAL, wdd->timeout);
+}
+
+/*
+ * Restart watchdog timer if feature is active.
+ *
+ * Note: Reading CTRL2 register causes watchdog to stop which is unfortunate,
+ * since register also contain control/status flags for other features.
+ * Always call this function after reading CTRL2 register.
+ */
+static int pcf2127_wdt_active_ping(struct watchdog_device *wdd)
+{
+	int ret = 0;
+
+	if (watchdog_active(wdd)) {
+		ret = pcf2127_wdt_ping(wdd);
+		if (ret)
+			dev_err(wdd->parent,
+				"%s: watchdog restart failed, ret=%d\n",
+				__func__, ret);
+	}
+
+	return ret;
+}
+
+static int pcf2127_wdt_start(struct watchdog_device *wdd)
+{
+	dev_info(wdd->parent, "watchdog enabled\n");
+
+	return pcf2127_wdt_ping(wdd);
+}
+
+static int pcf2127_wdt_stop(struct watchdog_device *wdd)
+{
+	struct pcf2127 *pcf2127 = watchdog_get_drvdata(wdd);
+
+	dev_info(wdd->parent, "watchdog disabled\n");
+
+	return regmap_write(pcf2127->regmap, PCF2127_REG_WD_VAL,
+			    PCF2127_WD_VAL_STOP);
+}
+
+static int pcf2127_wdt_set_timeout(struct watchdog_device *wdd,
+				   unsigned int new_timeout)
+{
+	int ret = 0;
+
+	dev_info(wdd->parent, "new watchdog timeout: %is (old: %is)\n",
+		new_timeout, wdd->timeout);
+
+	wdd->timeout = new_timeout;
+	ret = pcf2127_wdt_active_ping(wdd);
+
+	return ret;
+}
+
+static const struct watchdog_info pcf2127_wdt_info = {
+	.identity = "NXP PCF2127/PCF2129 Watchdog",
+	.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT,
+};
+
+static const struct watchdog_ops pcf2127_watchdog_ops = {
+	.owner = THIS_MODULE,
+	.start = pcf2127_wdt_start,
+	.stop = pcf2127_wdt_stop,
+	.ping = pcf2127_wdt_ping,
+	.set_timeout = pcf2127_wdt_set_timeout,
+};
+
 static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 			const char *name, bool has_nvmem)
 {
@@ -242,6 +334,16 @@  static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 
 	pcf2127->rtc->ops = &pcf2127_rtc_ops;
 
+	pcf2127->wdd.parent = dev;
+	pcf2127->wdd.info = &pcf2127_wdt_info;
+	pcf2127->wdd.ops = &pcf2127_watchdog_ops;
+	pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
+	pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
+	pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
+	pcf2127->wdd.min_hw_heartbeat_ms = 500;
+
+	watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
+
 	if (has_nvmem) {
 		struct nvmem_config nvmem_cfg = {
 			.priv = pcf2127,
@@ -253,6 +355,31 @@  static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 		ret = rtc_nvmem_register(pcf2127->rtc, &nvmem_cfg);
 	}
 
+	/*
+	 * Watchdog timer enabled and reset pin /RST activated when timed out.
+	 * Select 1Hz clock source for watchdog timer.
+	 * Timer is not started until WD_VAL is loaded with a valid value.
+	 * Note: Countdown timer disabled and not available.
+	 */
+	ret = regmap_update_bits(pcf2127->regmap, PCF2127_REG_WD_CTL,
+				 PCF2127_BIT_WD_CTL_CD1 |
+				 PCF2127_BIT_WD_CTL_CD0 |
+				 PCF2127_BIT_WD_CTL_TF1 |
+				 PCF2127_BIT_WD_CTL_TF0,
+				 PCF2127_BIT_WD_CTL_CD1 |
+				 PCF2127_BIT_WD_CTL_CD0 |
+				 PCF2127_BIT_WD_CTL_TF1);
+	if (ret) {
+		dev_err(dev, "%s: watchdog config (wd_ctl) failed\n", __func__);
+		return ret;
+	}
+
+	ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
+	if (ret) {
+		dev_err(dev, "%s: watchdog registering failed\n", __func__);
+		return ret;
+	}
+
 	return rtc_register_device(pcf2127->rtc);
 }