| Message ID | 20241108211506.1348528-1-alexander.sverdlin@siemens.com |
|---|---|
| State | Accepted |
| Commit | 5b5124e3d5cabac31d18fcd97c934aa13819fef6 |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | watchdog: rti: support SPL (or re-start) | expand |
On 08.11.24 22:15, A. Sverdlin wrote: > From: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > If the RTI watchdog has been enabled in SPL, enabling it in U-Boot proper > fails because it can only be enabled once in HW and never stopped. This > however leads to a situation that wdt_cyclic() watchdog trigger is not > being started any longer and the WDT fires at some point. Nothing against this change, just pointing out that, when a watchdog is supposed to monitor a complete boot, you normally don't want it to be triggered anymore before that boot is complete, neither from the kernel (watchdog.handle_boot_enabled=0) nor from U-Boot (CONFIG_WATCHDOG_AUTOSTART=n + script-based starting, or skip in in proper when started by SPL). Anything else always comes with the risk of running into a logically stuck boot that happily pets the watchdog until eternity - and you have to ask someone to manually reset your device. > > Allow for WDT re-start by not bailing out if the [previously] configured > period matches the one to be configured. > > Enabling in [A53] SPL has been tested on AM62x-based HW (where [A53] SPL is > responsible for loading R5 DM firmware and not this driver). > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> > --- > drivers/watchdog/rti_wdt.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c > index 99168d0cad03..320c5ca19e0a 100644 > --- a/drivers/watchdog/rti_wdt.c > +++ b/drivers/watchdog/rti_wdt.c > @@ -131,18 +131,19 @@ static int rti_wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) > u32 timer_margin; > int ret; > > - if (readl(priv->regs + RTIDWDCTRL) == WDENABLE_KEY) > + timer_margin = timeout_ms * priv->clk_hz / 1000; > + timer_margin >>= WDT_PRELOAD_SHIFT; > + if (timer_margin > WDT_PRELOAD_MAX) > + timer_margin = WDT_PRELOAD_MAX; > + > + if (readl(priv->regs + RTIDWDCTRL) == WDENABLE_KEY && > + readl(priv->regs + RTIDWDPRLD) != timer_margin) > return -EBUSY; > > ret = rti_wdt_load_fw(dev); > if (ret < 0) > return ret; > > - timer_margin = timeout_ms * priv->clk_hz / 1000; > - timer_margin >>= WDT_PRELOAD_SHIFT; > - if (timer_margin > WDT_PRELOAD_MAX) > - timer_margin = WDT_PRELOAD_MAX; > - > writel(timer_margin, priv->regs + RTIDWDPRLD); > writel(RTIWWDRX_NMI, priv->regs + RTIWWDRXCTRL); > writel(RTIWWDSIZE_50P, priv->regs + RTIWWDSIZECTRL); Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com> Jan
Hi Stefan! On Fri, 2024-11-08 at 22:15 +0100, A. Sverdlin wrote: > From: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > If the RTI watchdog has been enabled in SPL, enabling it in U-Boot proper > fails because it can only be enabled once in HW and never stopped. This > however leads to a situation that wdt_cyclic() watchdog trigger is not > being started any longer and the WDT fires at some point. > > Allow for WDT re-start by not bailing out if the [previously] configured > period matches the one to be configured. > > Enabling in [A53] SPL has been tested on AM62x-based HW (where [A53] SPL is > responsible for loading R5 DM firmware and not this driver). > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> I saw that this patch has been set to "Superseded" in the patchwork [1], but I'm not sure by which patch. Could it be that it happened by mistake? My other patch for the rti watchdog [2] is addressing some unrelated issue... 1. https://patchwork.ozlabs.org/project/uboot/patch/20241108211506.1348528-1-alexander.sverdlin@siemens.com/ 2. https://patchwork.ozlabs.org/project/uboot/patch/20241120222412.1873381-1-alexander.sverdlin@siemens.com/ https://patchwork.ozlabs.org/project/uboot/patch/20241121080326.1913067-1-alexander.sverdlin@siemens.com/ > --- > drivers/watchdog/rti_wdt.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c > index 99168d0cad03..320c5ca19e0a 100644 > --- a/drivers/watchdog/rti_wdt.c > +++ b/drivers/watchdog/rti_wdt.c > @@ -131,18 +131,19 @@ static int rti_wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) > u32 timer_margin; > int ret; > > - if (readl(priv->regs + RTIDWDCTRL) == WDENABLE_KEY) > + timer_margin = timeout_ms * priv->clk_hz / 1000; > + timer_margin >>= WDT_PRELOAD_SHIFT; > + if (timer_margin > WDT_PRELOAD_MAX) > + timer_margin = WDT_PRELOAD_MAX; > + > + if (readl(priv->regs + RTIDWDCTRL) == WDENABLE_KEY && > + readl(priv->regs + RTIDWDPRLD) != timer_margin) > return -EBUSY; > > ret = rti_wdt_load_fw(dev); > if (ret < 0) > return ret; > > - timer_margin = timeout_ms * priv->clk_hz / 1000; > - timer_margin >>= WDT_PRELOAD_SHIFT; > - if (timer_margin > WDT_PRELOAD_MAX) > - timer_margin = WDT_PRELOAD_MAX; > - > writel(timer_margin, priv->regs + RTIDWDPRLD); > writel(RTIWWDRX_NMI, priv->regs + RTIWWDRXCTRL); > writel(RTIWWDSIZE_50P, priv->regs + RTIWWDSIZECTRL);
Hi Alexander, On 10.12.24 13:45, Sverdlin, Alexander wrote: > Hi Stefan! > > On Fri, 2024-11-08 at 22:15 +0100, A. Sverdlin wrote: >> From: Alexander Sverdlin <alexander.sverdlin@siemens.com> >> >> If the RTI watchdog has been enabled in SPL, enabling it in U-Boot proper >> fails because it can only be enabled once in HW and never stopped. This >> however leads to a situation that wdt_cyclic() watchdog trigger is not >> being started any longer and the WDT fires at some point. >> >> Allow for WDT re-start by not bailing out if the [previously] configured >> period matches the one to be configured. >> >> Enabling in [A53] SPL has been tested on AM62x-based HW (where [A53] SPL is >> responsible for loading R5 DM firmware and not this driver). >> >> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > I saw that this patch has been set to "Superseded" in the patchwork [1], > but I'm not sure by which patch. Could it be that it happened by mistake? Might be. I need to take a look... > My other patch for the rti watchdog [2] is addressing some unrelated issue... > > 1. > https://patchwork.ozlabs.org/project/uboot/patch/20241108211506.1348528-1-alexander.sverdlin@siemens.com/ > > 2. > https://patchwork.ozlabs.org/project/uboot/patch/20241120222412.1873381-1-alexander.sverdlin@siemens.com/ > https://patchwork.ozlabs.org/project/uboot/patch/20241121080326.1913067-1-alexander.sverdlin@siemens.com/ Sorry, I missed handling the watchdog patches in this release cycle. And now we are pretty late in the release cycle and I would prefer to defer pulling these patches after the upcoming release beginning of January. Please excuse the inconvenience. Thanks, Stefan >> --- >> drivers/watchdog/rti_wdt.c | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c >> index 99168d0cad03..320c5ca19e0a 100644 >> --- a/drivers/watchdog/rti_wdt.c >> +++ b/drivers/watchdog/rti_wdt.c >> @@ -131,18 +131,19 @@ static int rti_wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) >> u32 timer_margin; >> int ret; >> >> - if (readl(priv->regs + RTIDWDCTRL) == WDENABLE_KEY) >> + timer_margin = timeout_ms * priv->clk_hz / 1000; >> + timer_margin >>= WDT_PRELOAD_SHIFT; >> + if (timer_margin > WDT_PRELOAD_MAX) >> + timer_margin = WDT_PRELOAD_MAX; >> + >> + if (readl(priv->regs + RTIDWDCTRL) == WDENABLE_KEY && >> + readl(priv->regs + RTIDWDPRLD) != timer_margin) >> return -EBUSY; >> >> ret = rti_wdt_load_fw(dev); >> if (ret < 0) >> return ret; >> >> - timer_margin = timeout_ms * priv->clk_hz / 1000; >> - timer_margin >>= WDT_PRELOAD_SHIFT; >> - if (timer_margin > WDT_PRELOAD_MAX) >> - timer_margin = WDT_PRELOAD_MAX; >> - >> writel(timer_margin, priv->regs + RTIDWDPRLD); >> writel(RTIWWDRX_NMI, priv->regs + RTIWWDRXCTRL); >> writel(RTIWWDSIZE_50P, priv->regs + RTIWWDSIZECTRL); > Viele Grüße, Stefan Roese
Hi Stefan! On Sat, 2024-12-14 at 14:36 +0100, Stefan Roese wrote: > > > If the RTI watchdog has been enabled in SPL, enabling it in U-Boot proper > > > fails because it can only be enabled once in HW and never stopped. This > > > however leads to a situation that wdt_cyclic() watchdog trigger is not > > > being started any longer and the WDT fires at some point. > > > > > > Allow for WDT re-start by not bailing out if the [previously] configured > > > period matches the one to be configured. > > > > > > Enabling in [A53] SPL has been tested on AM62x-based HW (where [A53] SPL is > > > responsible for loading R5 DM firmware and not this driver). > > > > > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > > > I saw that this patch has been set to "Superseded" in the patchwork [1], > > but I'm not sure by which patch. Could it be that it happened by mistake? > > Might be. I need to take a look... > > > My other patch for the rti watchdog [2] is addressing some unrelated issue... > > > > 1. > > https://patchwork.ozlabs.org/project/uboot/patch/20241108211506.1348528-1-alexander.sverdlin@siemens.com/ > > > > 2. > > https://patchwork.ozlabs.org/project/uboot/patch/20241120222412.1873381-1-alexander.sverdlin@siemens.com/ > > https://patchwork.ozlabs.org/project/uboot/patch/20241121080326.1913067-1-alexander.sverdlin@siemens.com/ > > Sorry, I missed handling the watchdog patches in this release cycle. > And now we are pretty late in the release cycle and I would prefer > to defer pulling these patches after the upcoming release beginning > of January. Please excuse the inconvenience. Thanks for the quick reply! No problem, thank you for your efforts!
On 08.11.24 22:15, A. Sverdlin wrote: > From: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > If the RTI watchdog has been enabled in SPL, enabling it in U-Boot proper > fails because it can only be enabled once in HW and never stopped. This > however leads to a situation that wdt_cyclic() watchdog trigger is not > being started any longer and the WDT fires at some point. > > Allow for WDT re-start by not bailing out if the [previously] configured > period matches the one to be configured. > > Enabling in [A53] SPL has been tested on AM62x-based HW (where [A53] SPL is > responsible for loading R5 DM firmware and not this driver). > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> Reviewed-by: Stefan Roese <sr@denx.de> Thanks, Stefan > --- > drivers/watchdog/rti_wdt.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c > index 99168d0cad03..320c5ca19e0a 100644 > --- a/drivers/watchdog/rti_wdt.c > +++ b/drivers/watchdog/rti_wdt.c > @@ -131,18 +131,19 @@ static int rti_wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) > u32 timer_margin; > int ret; > > - if (readl(priv->regs + RTIDWDCTRL) == WDENABLE_KEY) > + timer_margin = timeout_ms * priv->clk_hz / 1000; > + timer_margin >>= WDT_PRELOAD_SHIFT; > + if (timer_margin > WDT_PRELOAD_MAX) > + timer_margin = WDT_PRELOAD_MAX; > + > + if (readl(priv->regs + RTIDWDCTRL) == WDENABLE_KEY && > + readl(priv->regs + RTIDWDPRLD) != timer_margin) > return -EBUSY; > > ret = rti_wdt_load_fw(dev); > if (ret < 0) > return ret; > > - timer_margin = timeout_ms * priv->clk_hz / 1000; > - timer_margin >>= WDT_PRELOAD_SHIFT; > - if (timer_margin > WDT_PRELOAD_MAX) > - timer_margin = WDT_PRELOAD_MAX; > - > writel(timer_margin, priv->regs + RTIDWDPRLD); > writel(RTIWWDRX_NMI, priv->regs + RTIWWDRXCTRL); > writel(RTIWWDSIZE_50P, priv->regs + RTIWWDSIZECTRL); Viele Grüße, Stefan Roese
On 08.11.24 22:15, A. Sverdlin wrote: > From: Alexander Sverdlin <alexander.sverdlin@siemens.com> > > If the RTI watchdog has been enabled in SPL, enabling it in U-Boot proper > fails because it can only be enabled once in HW and never stopped. This > however leads to a situation that wdt_cyclic() watchdog trigger is not > being started any longer and the WDT fires at some point. > > Allow for WDT re-start by not bailing out if the [previously] configured > period matches the one to be configured. > > Enabling in [A53] SPL has been tested on AM62x-based HW (where [A53] SPL is > responsible for loading R5 DM firmware and not this driver). > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@siemens.com> Applied to u-boot-watchdog/master Thanks, Stefan > --- > drivers/watchdog/rti_wdt.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c > index 99168d0cad03..320c5ca19e0a 100644 > --- a/drivers/watchdog/rti_wdt.c > +++ b/drivers/watchdog/rti_wdt.c > @@ -131,18 +131,19 @@ static int rti_wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) > u32 timer_margin; > int ret; > > - if (readl(priv->regs + RTIDWDCTRL) == WDENABLE_KEY) > + timer_margin = timeout_ms * priv->clk_hz / 1000; > + timer_margin >>= WDT_PRELOAD_SHIFT; > + if (timer_margin > WDT_PRELOAD_MAX) > + timer_margin = WDT_PRELOAD_MAX; > + > + if (readl(priv->regs + RTIDWDCTRL) == WDENABLE_KEY && > + readl(priv->regs + RTIDWDPRLD) != timer_margin) > return -EBUSY; > > ret = rti_wdt_load_fw(dev); > if (ret < 0) > return ret; > > - timer_margin = timeout_ms * priv->clk_hz / 1000; > - timer_margin >>= WDT_PRELOAD_SHIFT; > - if (timer_margin > WDT_PRELOAD_MAX) > - timer_margin = WDT_PRELOAD_MAX; > - > writel(timer_margin, priv->regs + RTIDWDPRLD); > writel(RTIWWDRX_NMI, priv->regs + RTIWWDRXCTRL); > writel(RTIWWDSIZE_50P, priv->regs + RTIWWDSIZECTRL); Viele Grüße, Stefan Roese
diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c index 99168d0cad03..320c5ca19e0a 100644 --- a/drivers/watchdog/rti_wdt.c +++ b/drivers/watchdog/rti_wdt.c @@ -131,18 +131,19 @@ static int rti_wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) u32 timer_margin; int ret; - if (readl(priv->regs + RTIDWDCTRL) == WDENABLE_KEY) + timer_margin = timeout_ms * priv->clk_hz / 1000; + timer_margin >>= WDT_PRELOAD_SHIFT; + if (timer_margin > WDT_PRELOAD_MAX) + timer_margin = WDT_PRELOAD_MAX; + + if (readl(priv->regs + RTIDWDCTRL) == WDENABLE_KEY && + readl(priv->regs + RTIDWDPRLD) != timer_margin) return -EBUSY; ret = rti_wdt_load_fw(dev); if (ret < 0) return ret; - timer_margin = timeout_ms * priv->clk_hz / 1000; - timer_margin >>= WDT_PRELOAD_SHIFT; - if (timer_margin > WDT_PRELOAD_MAX) - timer_margin = WDT_PRELOAD_MAX; - writel(timer_margin, priv->regs + RTIDWDPRLD); writel(RTIWWDRX_NMI, priv->regs + RTIWWDRXCTRL); writel(RTIWWDSIZE_50P, priv->regs + RTIWWDSIZECTRL);