From: Philipp Rosenberger <p.rosenberger@kunbus.com>
To: Hugo Villeneuve <hugo@hugovil.com>
Cc: a.zummo@towertech.it, alexandre.belloni@bootlin.com,
robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
linux-rtc@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
Hugo Villeneuve <hvilleneuve@dimonoff.com>
Subject: Re: [PATCH v3 12/14] rtc: pcf2127: support generic watchdog timing configuration
Date: Wed, 18 Jan 2023 14:23:14 +0100 [thread overview]
Message-ID: <72514fec-12e2-5b51-261d-9e171b46f5bb@kunbus.com> (raw)
In-Reply-To: <20221215150214.1109074-13-hugo@hugovil.com>
Hi Hugo,
shouldn't the timeout set with pcf2127_wdt_set_timeout() be in seconds?
With your changes this value is dependent on the configuration of the
timer source clock for watchdog timer. So with a default of 1/4Hz this
will be almost seconds * 4.
I think we need to do the same calculations as in the
pcf2127_watchdog_init() when calculating the timeout from the
PCF2127_WD_VAL_DEFAULT.
Best regards,
Philipp
On 15.12.22 16:02, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
>
> Introduce in the configuration structure two new values to hold the
> watchdog clock source and the min_hw_heartbeat_ms value.
>
> The minimum and maximum timeout values are automatically computed from
> the watchdog clock source value for each variant.
>
> The PCF2131 has no 1Hz watchdog clock source, as is the case for
> PCF2127/29.
>
> The next best choice is using a 1/4Hz clock, giving a watchdog timeout
> range between 4 and 1016s. By using the same register configuration as
> for the PCF2127/29, the 1/4Hz clock source is selected.
>
> Note: the PCF2127 datasheet gives a min/max range between 1 and 255s,
> but it should be between 2 and 254s, because the watchdog is triggered
> when the timer value reaches 1, not 0.
>
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
> drivers/rtc/rtc-pcf2127.c | 56 +++++++++++++++++++++++++++++++++------
> 1 file changed, 48 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index 11fbdab6bf01..3fd2fee4978b 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -157,9 +157,29 @@
>
> /* 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
> +#define PCF2127_WD_VAL_DEFAULT 60 /* In seconds. */
> +/* PCF2127/29 watchdog timer value constants */
> +#define PCF2127_WD_CLOCK_HZ_X1000 1000 /* 1Hz */
> +#define PCF2127_WD_MIN_HW_HEARTBEAT_MS 500
> +/* PCF2131 watchdog timer value constants */
> +#define PCF2131_WD_CLOCK_HZ_X1000 250 /* 1/4Hz */
> +#define PCF2131_WD_MIN_HW_HEARTBEAT_MS 4000
> +/*
> + * Compute watchdog period, t, in seconds, from the WATCHDG_TIM_VAL register
> + * value, n, and the clock frequency, f, in Hz.
> + *
> + * The PCF2127/29 datasheet gives t as:
> + * t = n / f
> + * The PCF2131 datasheet gives t as:
> + * t = (n - 1) / f
> + * For both variants, the watchdog is triggered when the WATCHDG_TIM_VAL reaches
> + * the value 1, and not zero. Consequently, the equation from the PCF2131
> + * datasheet seems to be the correct one for both variants.
> + */
> +#define WD_PERIOD_S(_n_, _f1000_) ((1000 * ((_n_) - 1)) / (_f1000_))
> +
> +/* Compute value of WATCHDG_TIM_VAL to obtain period t, in seconds. */
> +#define WD_COUNTER(_t_, _f1000_) ((((_t_) * (_f1000_)) / 1000) + 1)
>
> /* Mask for currently enabled interrupts */
> #define PCF2127_CTRL1_IRQ_MASK (PCF2127_BIT_CTRL1_TSF1)
> @@ -202,6 +222,11 @@ struct pcf21xx_config {
> u8 reg_wd_val; /* Watchdog value register. */
> u8 reg_clkout; /* Clkout register. */
> u8 reg_reset; /* Reset register if available. */
> +
> + /* Watchdog configuration. */
> + int wdd_clock_hz_x1000; /* Value in Hz multiplicated by 1000 */
> + int wdd_min_hw_heartbeat_ms;
> +
> unsigned int ts_count;
> struct pcf21xx_ts_config ts[4];
> struct attribute_group attribute_group;
> @@ -496,10 +521,19 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
> 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;
> +
> + pcf2127->wdd.min_timeout =
> + WD_PERIOD_S(2, pcf2127->cfg->wdd_clock_hz_x1000);
> + pcf2127->wdd.max_timeout =
> + WD_PERIOD_S(255, pcf2127->cfg->wdd_clock_hz_x1000);
> + pcf2127->wdd.timeout = WD_COUNTER(PCF2127_WD_VAL_DEFAULT,
> + pcf2127->cfg->wdd_clock_hz_x1000);
> +
> + dev_dbg(dev, "%s min = %ds\n", __func__, pcf2127->wdd.min_timeout);
> + dev_dbg(dev, "%s max = %ds\n", __func__, pcf2127->wdd.max_timeout);
> + dev_dbg(dev, "%s def = %d\n", __func__, pcf2127->wdd.timeout);
> +
> + pcf2127->wdd.min_hw_heartbeat_ms = pcf2127->cfg->wdd_min_hw_heartbeat_ms;
> pcf2127->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
>
> watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
> @@ -926,6 +960,8 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
> .reg_wd_ctl = PCF2127_REG_WD_CTL,
> .reg_wd_val = PCF2127_REG_WD_VAL,
> .reg_clkout = PCF2127_REG_CLKOUT,
> + .wdd_clock_hz_x1000 = PCF2127_WD_CLOCK_HZ_X1000,
> + .wdd_min_hw_heartbeat_ms = PCF2127_WD_MIN_HW_HEARTBEAT_MS,
> .ts_count = 1,
> .ts[0] = {
> .regs_base = PCF2127_REG_TS1_BASE,
> @@ -951,6 +987,8 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
> .reg_wd_ctl = PCF2127_REG_WD_CTL,
> .reg_wd_val = PCF2127_REG_WD_VAL,
> .reg_clkout = PCF2127_REG_CLKOUT,
> + .wdd_clock_hz_x1000 = PCF2127_WD_CLOCK_HZ_X1000,
> + .wdd_min_hw_heartbeat_ms = PCF2127_WD_MIN_HW_HEARTBEAT_MS,
> .ts_count = 1,
> .ts[0] = {
> .regs_base = PCF2127_REG_TS1_BASE,
> @@ -977,6 +1015,8 @@ static struct pcf21xx_config pcf21xx_cfg[] = {
> .reg_wd_val = PCF2131_REG_WD_VAL,
> .reg_clkout = PCF2131_REG_CLKOUT,
> .reg_reset = PCF2131_REG_SR_RESET,
> + .wdd_clock_hz_x1000 = PCF2131_WD_CLOCK_HZ_X1000,
> + .wdd_min_hw_heartbeat_ms = PCF2131_WD_MIN_HW_HEARTBEAT_MS,
> .ts_count = 4,
> .ts[0] = {
> .regs_base = PCF2131_REG_TS1_BASE,
> @@ -1215,7 +1255,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>
> /*
> * Watchdog timer enabled and reset pin /RST activated when timed out.
> - * Select 1Hz clock source for watchdog timer.
> + * Select 1Hz clock source for watchdog timer (1/4Hz for PCF2131).
> * Note: Countdown timer disabled and not available.
> * For pca2129, pcf2129 and pcf2131, only bit[7] is for Symbol WD_CD
> * of register watchdg_tim_ctl. The bit[6] is labeled
next prev parent reply other threads:[~2023-01-18 13:52 UTC|newest]
Thread overview: 72+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-15 15:02 [PATCH v3 00/14] rtc: pcf2127: add PCF2131 driver Hugo Villeneuve
2022-12-15 15:02 ` [PATCH v3 01/14] rtc: pcf2127: add variant-specific configuration structure Hugo Villeneuve
2022-12-19 9:05 ` Bruno Thomsen
2022-12-19 15:15 ` Hugo Villeneuve
2022-12-19 17:17 ` Bruno Thomsen
2022-12-19 18:30 ` Hugo Villeneuve
2023-01-07 16:52 ` Bruno Thomsen
2022-12-15 15:02 ` [PATCH v3 02/14] rtc: pcf2127: adapt for time/date registers at any offset Hugo Villeneuve
2022-12-19 9:34 ` Bruno Thomsen
2022-12-19 16:27 ` Hugo Villeneuve
2023-01-07 16:49 ` Bruno Thomsen
2023-01-20 18:47 ` Alexandre Belloni
2023-01-23 15:54 ` Hugo Villeneuve
2023-06-21 18:18 ` Alexandre Belloni
2022-12-15 15:02 ` [PATCH v3 03/14] rtc: pcf2127: adapt for alarm " Hugo Villeneuve
2023-01-07 16:57 ` Bruno Thomsen
2023-01-20 17:10 ` Alexandre Belloni
2023-01-23 16:02 ` Hugo Villeneuve
2022-12-15 15:02 ` [PATCH v3 04/14] rtc: pcf2127: adapt for WD " Hugo Villeneuve
2023-01-07 16:59 ` Bruno Thomsen
2022-12-15 15:02 ` [PATCH v3 05/14] rtc: pcf2127: adapt for CLKOUT register " Hugo Villeneuve
2023-01-07 17:01 ` Bruno Thomsen
2022-12-15 15:02 ` [PATCH v3 06/14] rtc: pcf2127: add support for multiple TS functions Hugo Villeneuve
2023-01-07 17:58 ` Bruno Thomsen
2023-01-23 20:41 ` Hugo Villeneuve
2022-12-15 15:02 ` [PATCH v3 07/14] rtc: pcf2127: add support for PCF2131 RTC Hugo Villeneuve
2023-01-07 18:15 ` Bruno Thomsen
2023-01-23 19:06 ` Hugo Villeneuve
2023-01-20 18:57 ` Alexandre Belloni
2023-01-23 17:27 ` Hugo Villeneuve
2022-12-15 15:02 ` [PATCH v3 08/14] rtc: pcf2127: add support for PCF2131 interrupts on output INT_A Hugo Villeneuve
2023-01-07 18:17 ` Bruno Thomsen
2023-01-20 16:56 ` Alexandre Belloni
2023-01-23 20:52 ` Hugo Villeneuve
2023-05-11 17:19 ` Hugo Villeneuve
2023-06-21 19:24 ` Alexandre Belloni
2023-06-21 19:26 ` Alexandre Belloni
2023-06-22 14:21 ` Hugo Villeneuve
2022-12-15 15:02 ` [PATCH v3 09/14] rtc: pcf2127: set PWRMNG value for PCF2131 Hugo Villeneuve
2023-01-07 18:36 ` Bruno Thomsen
2023-01-20 16:39 ` Alexandre Belloni
2023-01-23 22:07 ` Hugo Villeneuve
2022-12-15 15:02 ` [PATCH v3 10/14] rtc: pcf2127: read and validate PCF2131 device signature Hugo Villeneuve
2023-01-20 17:01 ` Alexandre Belloni
2023-01-23 17:31 ` Hugo Villeneuve
2022-12-15 15:02 ` [PATCH v3 11/14] rtc: pcf2127: adapt time/date registers write sequence for PCF2131 Hugo Villeneuve
2023-01-07 18:44 ` Bruno Thomsen
2023-01-23 20:55 ` Hugo Villeneuve
2023-01-20 17:09 ` Alexandre Belloni
2023-01-23 21:57 ` Hugo Villeneuve
2023-06-21 19:36 ` Alexandre Belloni
2022-12-15 15:02 ` [PATCH v3 12/14] rtc: pcf2127: support generic watchdog timing configuration Hugo Villeneuve
2023-01-18 13:23 ` Philipp Rosenberger [this message]
2023-01-19 17:48 ` Hugo Villeneuve
2023-01-20 8:06 ` Philipp Rosenberger
2023-01-20 14:44 ` Hugo Villeneuve
2022-12-15 15:02 ` [PATCH v3 13/14] rtc: pcf2127: add flag for watchdog register value read support Hugo Villeneuve
2023-01-07 18:47 ` Bruno Thomsen
2022-12-15 15:02 ` [PATCH v3 14/14] dt-bindings: rtc: pcf2127: add PCF2131 Hugo Villeneuve
2022-12-16 13:24 ` Krzysztof Kozlowski
2022-12-19 9:14 ` Bruno Thomsen
2022-12-19 16:25 ` Hugo Villeneuve
2022-12-19 17:18 ` Bruno Thomsen
2022-12-19 18:31 ` Hugo Villeneuve
2023-01-20 19:05 ` [PATCH v3 00/14] rtc: pcf2127: add PCF2131 driver Alexandre Belloni
2023-01-23 15:51 ` Hugo Villeneuve
2023-06-21 14:14 ` Hugo Villeneuve
2023-06-21 16:59 ` Hugo Villeneuve
2023-06-21 18:14 ` Alexandre Belloni
2023-06-21 18:28 ` Hugo Villeneuve
2023-07-05 13:40 ` Hugo Villeneuve
2023-07-07 14:16 ` Alexandre Belloni
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=72514fec-12e2-5b51-261d-9e171b46f5bb@kunbus.com \
--to=p.rosenberger@kunbus.com \
--cc=a.zummo@towertech.it \
--cc=alexandre.belloni@bootlin.com \
--cc=devicetree@vger.kernel.org \
--cc=hugo@hugovil.com \
--cc=hvilleneuve@dimonoff.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rtc@vger.kernel.org \
--cc=robh+dt@kernel.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).