From: Guenter Roeck <linux@roeck-us.net>
To: Bruno Thomsen <bruno.thomsen@gmail.com>
Cc: linux-rtc@vger.kernel.org, linux-watchdog@vger.kernel.org,
alexandre.belloni@bootlin.com, a.zummo@towertech.it,
wim@linux-watchdog.org, u.kleine-koenig@pengutronix.de,
sean.nyekjaer@prevas.dk, bth@kamstrup.com
Subject: Re: [PATCH v2 4/5] rtc: pcf2127: add watchdog feature support
Date: Tue, 13 Aug 2019 09:19:08 -0700 [thread overview]
Message-ID: <20190813161908.GA7857@roeck-us.net> (raw)
In-Reply-To: <20190813153600.12406-5-bruno.thomsen@gmail.com>
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
>
next prev parent reply other threads:[~2019-08-13 16:19 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-13 15:35 [PATCH v2 0/5] rtc: pcf2127: tamper timestamp and watchdog feature support Bruno Thomsen
2019-08-13 15:35 ` [PATCH v2 1/5] rtc: pcf2127: convert to devm_rtc_allocate_device Bruno Thomsen
2019-08-13 15:35 ` [PATCH v2 2/5] rtc: pcf2127: cleanup register and bit defines Bruno Thomsen
2019-08-13 15:35 ` [PATCH v2 3/5] rtc: pcf2127: bugfix: read rtc disables watchdog Bruno Thomsen
2019-08-13 15:35 ` [PATCH v2 4/5] rtc: pcf2127: add watchdog feature support Bruno Thomsen
2019-08-13 16:19 ` Guenter Roeck [this message]
2019-08-14 13:25 ` Bruno Thomsen
2019-08-14 13:34 ` Guenter Roeck
2019-08-13 15:36 ` [PATCH v2 5/5] rtc: pcf2127: add tamper detection support Bruno Thomsen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190813161908.GA7857@roeck-us.net \
--to=linux@roeck-us.net \
--cc=a.zummo@towertech.it \
--cc=alexandre.belloni@bootlin.com \
--cc=bruno.thomsen@gmail.com \
--cc=bth@kamstrup.com \
--cc=linux-rtc@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=sean.nyekjaer@prevas.dk \
--cc=u.kleine-koenig@pengutronix.de \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).