| Message ID | 20240415134903.8084-1-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] watchdog: stm32_iwdg: Add pretimeout support | expand |
On 4/15/24 3:48 PM, Marek Vasut wrote: > The STM32MP15xx IWDG adds registers which permit this IP to generate > pretimeout interrupt. This interrupt can also be used to wake the CPU > from suspend. Implement support for generating this interrupt and let > userspace configure the pretimeout. In case the pretimeout is not > configured by user, set pretimeout to 3/4 of the WDT timeout cycle. > > Reviewed-by: Clément Le Goffic <clement.legoffic@foss.st.com> > Tested-by: Clément Le Goffic <clement.legoffic@foss.st.com> > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-stm32@st-md-mailman.stormreply.com > Cc: linux-watchdog@vger.kernel.org > --- > V2: - Subtract the pretimeout value from timeout value before writing it > into the IWDG pretimeout register, because the watchdog counter > register is counting down, and the pretimeout interrupt triggers > when watchdog counter register matches the pretimeout register > content. > - Set default pretimeout to 3/4 of timeout . > V3: - Use dev instead of pdev->dev > - Swap order of ret/return 0 > - Split this from the DT changes, which are orthogonal > - Uh, this patch got stuck in upstreaming queue, sorry > V4: - Update commit message to match V2 default pretimeout to 3/4 > - Add RB/TB from Clément Hi, Are there still any open topics with this patch ?
On 9/5/24 3:12 PM, Marek Vasut wrote: > On 6/23/24 8:18 PM, Marek Vasut wrote: >> On 4/15/24 3:48 PM, Marek Vasut wrote: >>> The STM32MP15xx IWDG adds registers which permit this IP to generate >>> pretimeout interrupt. This interrupt can also be used to wake the CPU >>> from suspend. Implement support for generating this interrupt and let >>> userspace configure the pretimeout. In case the pretimeout is not >>> configured by user, set pretimeout to 3/4 of the WDT timeout cycle. >>> >>> Reviewed-by: Clément Le Goffic <clement.legoffic@foss.st.com> >>> Tested-by: Clément Le Goffic <clement.legoffic@foss.st.com> >>> Signed-off-by: Marek Vasut <marex@denx.de> >>> --- >>> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> >>> Cc: Guenter Roeck <linux@roeck-us.net> >>> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> >>> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> >>> Cc: linux-arm-kernel@lists.infradead.org >>> Cc: linux-stm32@st-md-mailman.stormreply.com >>> Cc: linux-watchdog@vger.kernel.org >>> --- >>> V2: - Subtract the pretimeout value from timeout value before writing it >>> into the IWDG pretimeout register, because the watchdog counter >>> register is counting down, and the pretimeout interrupt triggers >>> when watchdog counter register matches the pretimeout register >>> content. >>> - Set default pretimeout to 3/4 of timeout . >>> V3: - Use dev instead of pdev->dev >>> - Swap order of ret/return 0 >>> - Split this from the DT changes, which are orthogonal >>> - Uh, this patch got stuck in upstreaming queue, sorry >>> V4: - Update commit message to match V2 default pretimeout to 3/4 >>> - Add RB/TB from Clément >> >> Hi, >> >> Are there still any open topics with this patch ? > > Anything ? Can this be pulled via the stm32 SoC tree ?
On 4/15/24 06:48, Marek Vasut wrote: > The STM32MP15xx IWDG adds registers which permit this IP to generate > pretimeout interrupt. This interrupt can also be used to wake the CPU > from suspend. Implement support for generating this interrupt and let > userspace configure the pretimeout. In case the pretimeout is not > configured by user, set pretimeout to 3/4 of the WDT timeout cycle. > > Reviewed-by: Clément Le Goffic <clement.legoffic@foss.st.com> > Tested-by: Clément Le Goffic <clement.legoffic@foss.st.com> > Signed-off-by: Marek Vasut <marex@denx.de> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
On 9/30/24 8:23 PM, Guenter Roeck wrote: > On 4/15/24 06:48, Marek Vasut wrote: >> The STM32MP15xx IWDG adds registers which permit this IP to generate >> pretimeout interrupt. This interrupt can also be used to wake the CPU >> from suspend. Implement support for generating this interrupt and let >> userspace configure the pretimeout. In case the pretimeout is not >> configured by user, set pretimeout to 3/4 of the WDT timeout cycle. >> >> Reviewed-by: Clément Le Goffic <clement.legoffic@foss.st.com> >> Tested-by: Clément Le Goffic <clement.legoffic@foss.st.com> >> Signed-off-by: Marek Vasut <marex@denx.de> > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> Thank you
On 4/15/24 15:48, Marek Vasut wrote: > The STM32MP15xx IWDG adds registers which permit this IP to generate > pretimeout interrupt. This interrupt can also be used to wake the CPU > from suspend. Implement support for generating this interrupt and let > userspace configure the pretimeout. In case the pretimeout is not > configured by user, set pretimeout to 3/4 of the WDT timeout cycle. > > Reviewed-by: Clément Le Goffic <clement.legoffic@foss.st.com> > Tested-by: Clément Le Goffic <clement.legoffic@foss.st.com> > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> > Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-stm32@st-md-mailman.stormreply.com > Cc: linux-watchdog@vger.kernel.org > --- > V2: - Subtract the pretimeout value from timeout value before writing it > into the IWDG pretimeout register, because the watchdog counter > register is counting down, and the pretimeout interrupt triggers > when watchdog counter register matches the pretimeout register > content. > - Set default pretimeout to 3/4 of timeout . > V3: - Use dev instead of pdev->dev > - Swap order of ret/return 0 > - Split this from the DT changes, which are orthogonal > - Uh, this patch got stuck in upstreaming queue, sorry > V4: - Update commit message to match V2 default pretimeout to 3/4 > - Add RB/TB from Clément > --- > drivers/watchdog/stm32_iwdg.c | 95 ++++++++++++++++++++++++++++++++++- > 1 file changed, 94 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c > index 5404e03876202..d700e0d49bb95 100644 > --- a/drivers/watchdog/stm32_iwdg.c > +++ b/drivers/watchdog/stm32_iwdg.c > > [.....] > > + > + irq = platform_get_irq(pdev, 0); > + if (irq <= 0) > + return 0; Hi Marek, After re-evaluating this patch, it seems it lacks of a dt-bindings update that tackles the 'interrupts' property you are adding. That said, the interrupt line should not be mandatory for the driver to probe. For backward compatibility with existing DT, I recommend to use the 'platform_get_irq_optional()' API to not fail during the probe of the driver. Best regards, Clément
On 12/11/24 10:02 AM, Clement LE GOFFIC wrote: > On 4/15/24 15:48, Marek Vasut wrote: >> The STM32MP15xx IWDG adds registers which permit this IP to generate >> pretimeout interrupt. This interrupt can also be used to wake the CPU >> from suspend. Implement support for generating this interrupt and let >> userspace configure the pretimeout. In case the pretimeout is not >> configured by user, set pretimeout to 3/4 of the WDT timeout cycle. >> >> Reviewed-by: Clément Le Goffic <clement.legoffic@foss.st.com> >> Tested-by: Clément Le Goffic <clement.legoffic@foss.st.com> >> Signed-off-by: Marek Vasut <marex@denx.de> >> --- >> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> >> Cc: Guenter Roeck <linux@roeck-us.net> >> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com> >> Cc: Wim Van Sebroeck <wim@linux-watchdog.org> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-stm32@st-md-mailman.stormreply.com >> Cc: linux-watchdog@vger.kernel.org >> --- >> V2: - Subtract the pretimeout value from timeout value before writing it >> into the IWDG pretimeout register, because the watchdog counter >> register is counting down, and the pretimeout interrupt triggers >> when watchdog counter register matches the pretimeout register >> content. >> - Set default pretimeout to 3/4 of timeout . >> V3: - Use dev instead of pdev->dev >> - Swap order of ret/return 0 >> - Split this from the DT changes, which are orthogonal >> - Uh, this patch got stuck in upstreaming queue, sorry >> V4: - Update commit message to match V2 default pretimeout to 3/4 >> - Add RB/TB from Clément >> --- >> drivers/watchdog/stm32_iwdg.c | 95 ++++++++++++++++++++++++++++++++++- >> 1 file changed, 94 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/ >> stm32_iwdg.c >> index 5404e03876202..d700e0d49bb95 100644 >> --- a/drivers/watchdog/stm32_iwdg.c >> +++ b/drivers/watchdog/stm32_iwdg.c >> >> [.....] >> >> + >> + irq = platform_get_irq(pdev, 0); >> + if (irq <= 0) >> + return 0; > > Hi Marek, > > After re-evaluating this patch, it seems it lacks of a dt-bindings > update that tackles the 'interrupts' property you are adding. > > That said, the interrupt line should not be mandatory for the driver to > probe. For backward compatibility with existing DT, I recommend to use > the 'platform_get_irq_optional()' API to not fail during the probe of > the driver. I saw the fix [PATCH] watchdog: stm32_iwdg: fix DT backward compatibility Thank you for that. I'll wait for V2 with updated commit message . As far as I understood the problem, the goal is to remove error message printed by the platform_get_irq() in case the DT interrupt property is missing, but this is only an esthetic fix, not a functional one, because the driver probes even if the interrupts DT property is missing, it only prints the error message while at it, correct ?
diff --git a/drivers/watchdog/stm32_iwdg.c b/drivers/watchdog/stm32_iwdg.c index 5404e03876202..d700e0d49bb95 100644 --- a/drivers/watchdog/stm32_iwdg.c +++ b/drivers/watchdog/stm32_iwdg.c @@ -18,6 +18,7 @@ #include <linux/module.h> #include <linux/of.h> #include <linux/platform_device.h> +#include <linux/pm_wakeirq.h> #include <linux/watchdog.h> #define DEFAULT_TIMEOUT 10 @@ -28,6 +29,7 @@ #define IWDG_RLR 0x08 /* ReLoad Register */ #define IWDG_SR 0x0C /* Status Register */ #define IWDG_WINR 0x10 /* Windows Register */ +#define IWDG_EWCR 0x14 /* Early Wake-up Register */ /* IWDG_KR register bit mask */ #define KR_KEY_RELOAD 0xAAAA /* reload counter enable */ @@ -47,22 +49,29 @@ #define SR_PVU BIT(0) /* Watchdog prescaler value update */ #define SR_RVU BIT(1) /* Watchdog counter reload value update */ +#define EWCR_EWIT GENMASK(11, 0) /* Watchdog counter window value */ +#define EWCR_EWIC BIT(14) /* Watchdog early interrupt acknowledge */ +#define EWCR_EWIE BIT(15) /* Watchdog early interrupt enable */ + /* set timeout to 100000 us */ #define TIMEOUT_US 100000 #define SLEEP_US 1000 struct stm32_iwdg_data { bool has_pclk; + bool has_early_wakeup; u32 max_prescaler; }; static const struct stm32_iwdg_data stm32_iwdg_data = { .has_pclk = false, + .has_early_wakeup = false, .max_prescaler = 256, }; static const struct stm32_iwdg_data stm32mp1_iwdg_data = { .has_pclk = true, + .has_early_wakeup = true, .max_prescaler = 1024, }; @@ -88,13 +97,18 @@ static inline void reg_write(void __iomem *base, u32 reg, u32 val) static int stm32_iwdg_start(struct watchdog_device *wdd) { struct stm32_iwdg *wdt = watchdog_get_drvdata(wdd); - u32 tout, presc, iwdg_rlr, iwdg_pr, iwdg_sr; + u32 tout, ptot, presc, iwdg_rlr, iwdg_ewcr, iwdg_pr, iwdg_sr; int ret; dev_dbg(wdd->parent, "%s\n", __func__); + if (!wdd->pretimeout) + wdd->pretimeout = 3 * wdd->timeout / 4; + tout = clamp_t(unsigned int, wdd->timeout, wdd->min_timeout, wdd->max_hw_heartbeat_ms / 1000); + ptot = clamp_t(unsigned int, tout - wdd->pretimeout, + wdd->min_timeout, tout); presc = DIV_ROUND_UP(tout * wdt->rate, RLR_MAX + 1); @@ -102,6 +116,7 @@ static int stm32_iwdg_start(struct watchdog_device *wdd) presc = roundup_pow_of_two(presc); iwdg_pr = presc <= 1 << PR_SHIFT ? 0 : ilog2(presc) - PR_SHIFT; iwdg_rlr = ((tout * wdt->rate) / presc) - 1; + iwdg_ewcr = ((ptot * wdt->rate) / presc) - 1; /* enable write access */ reg_write(wdt->regs, IWDG_KR, KR_KEY_EWA); @@ -109,6 +124,8 @@ static int stm32_iwdg_start(struct watchdog_device *wdd) /* set prescaler & reload registers */ reg_write(wdt->regs, IWDG_PR, iwdg_pr); reg_write(wdt->regs, IWDG_RLR, iwdg_rlr); + if (wdt->data->has_early_wakeup) + reg_write(wdt->regs, IWDG_EWCR, iwdg_ewcr | EWCR_EWIE); reg_write(wdt->regs, IWDG_KR, KR_KEY_ENABLE); /* wait for the registers to be updated (max 100ms) */ @@ -151,6 +168,34 @@ static int stm32_iwdg_set_timeout(struct watchdog_device *wdd, return 0; } +static int stm32_iwdg_set_pretimeout(struct watchdog_device *wdd, + unsigned int pretimeout) +{ + dev_dbg(wdd->parent, "%s pretimeout: %d sec\n", __func__, pretimeout); + + wdd->pretimeout = pretimeout; + + if (watchdog_active(wdd)) + return stm32_iwdg_start(wdd); + + return 0; +} + +static irqreturn_t stm32_iwdg_isr(int irq, void *wdog_arg) +{ + struct watchdog_device *wdd = wdog_arg; + struct stm32_iwdg *wdt = watchdog_get_drvdata(wdd); + u32 reg; + + reg = reg_read(wdt->regs, IWDG_EWCR); + reg |= EWCR_EWIC; + reg_write(wdt->regs, IWDG_EWCR, reg); + + watchdog_notify_pretimeout(wdd); + + return IRQ_HANDLED; +} + static void stm32_clk_disable_unprepare(void *data) { clk_disable_unprepare(data); @@ -207,11 +252,20 @@ static const struct watchdog_info stm32_iwdg_info = { .identity = "STM32 Independent Watchdog", }; +static const struct watchdog_info stm32_iwdg_preinfo = { + .options = WDIOF_SETTIMEOUT | + WDIOF_MAGICCLOSE | + WDIOF_KEEPALIVEPING | + WDIOF_PRETIMEOUT, + .identity = "STM32 Independent Watchdog", +}; + static const struct watchdog_ops stm32_iwdg_ops = { .owner = THIS_MODULE, .start = stm32_iwdg_start, .ping = stm32_iwdg_ping, .set_timeout = stm32_iwdg_set_timeout, + .set_pretimeout = stm32_iwdg_set_pretimeout, }; static const struct of_device_id stm32_iwdg_of_match[] = { @@ -221,6 +275,40 @@ static const struct of_device_id stm32_iwdg_of_match[] = { }; MODULE_DEVICE_TABLE(of, stm32_iwdg_of_match); +static int stm32_iwdg_irq_init(struct platform_device *pdev, + struct stm32_iwdg *wdt) +{ + struct device_node *np = pdev->dev.of_node; + struct watchdog_device *wdd = &wdt->wdd; + struct device *dev = &pdev->dev; + int irq, ret; + + if (!wdt->data->has_early_wakeup) + return 0; + + irq = platform_get_irq(pdev, 0); + if (irq <= 0) + return 0; + + if (of_property_read_bool(np, "wakeup-source")) { + ret = device_init_wakeup(dev, true); + if (ret) + return ret; + + ret = dev_pm_set_wake_irq(dev, irq); + if (ret) + return ret; + } + + ret = devm_request_irq(dev, irq, stm32_iwdg_isr, 0, + dev_name(dev), wdd); + if (ret) + return ret; + + wdd->info = &stm32_iwdg_preinfo; + return 0; +} + static int stm32_iwdg_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -255,6 +343,11 @@ static int stm32_iwdg_probe(struct platform_device *pdev) wdd->max_hw_heartbeat_ms = ((RLR_MAX + 1) * wdt->data->max_prescaler * 1000) / wdt->rate; + /* Initialize IRQ, this might override wdd->info, hence it is here. */ + ret = stm32_iwdg_irq_init(pdev, wdt); + if (ret) + return ret; + watchdog_set_drvdata(wdd, wdt); watchdog_set_nowayout(wdd, WATCHDOG_NOWAYOUT); watchdog_init_timeout(wdd, 0, dev);