| Message ID | 20190425071703.9647-1-sr@denx.de |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Stefan Roese |
| Headers | show |
| Series | [U-Boot,1/4,v5] watchdog: Implement generic watchdog_reset() version | expand |
On 25.04.19 09:17, Stefan Roese wrote: > This patch tries to implement a generic watchdog_reset() function that > can be used by all boards that want to service the watchdog device in > U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG. > > Without this approach, new boards or platforms needed to implement a > board specific version of this functionality, mostly copy'ing the same > code over and over again into their board or platforms code base. > > With this new generic function, the scattered other functions are now > removed to be replaced by the generic one. The new version also enables > the configuration of the watchdog timeout via the DT "timeout-sec" > property (if enabled via CONFIG_OF_CONTROL). > > This patch also adds a new flag to the GD flags, to flag that the > watchdog is ready to use and adds the pointer to the watchdog device > to the GD. This enables us to remove the global "watchdog_dev" > variable, which was prone to cause problems because of its potentially > very early use in watchdog_reset(), even before the BSS is cleared. > > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Heiko Schocher <hs@denx.de> > Cc: Tom Rini <trini@konsulko.com> > Cc: Michal Simek <michal.simek@xilinx.com> > Cc: "Marek Behún" <marek.behun@nic.cz> > Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> > Cc: Maxim Sloyko <maxims@google.com> > Cc: Erik van Luijk <evanluijk@interact.nl> > Cc: Ryder Lee <ryder.lee@mediatek.com> > Cc: Weijie Gao <weijie.gao@mediatek.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: "Álvaro Fernández Rojas" <noltari@gmail.com> > Cc: Philippe Reynes <philippe.reynes@softathome.com> > Cc: Christophe Leroy <christophe.leroy@c-s.fr> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> > Reviewed-by: Michal Simek <michal.simek@xilinx.com> > Tested-by: Michal Simek <michal.simek@xilinx.com> (on zcu100) Applied to u-boot-marvell/master. Thanks, Stefan
Hi Stefan, Am 25.04.2019 um 09:17 schrieb Stefan Roese: > This patch tries to implement a generic watchdog_reset() function that > can be used by all boards that want to service the watchdog device in > U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG. > > Without this approach, new boards or platforms needed to implement a > board specific version of this functionality, mostly copy'ing the same > code over and over again into their board or platforms code base. > > With this new generic function, the scattered other functions are now > removed to be replaced by the generic one. The new version also enables > the configuration of the watchdog timeout via the DT "timeout-sec" > property (if enabled via CONFIG_OF_CONTROL). > > This patch also adds a new flag to the GD flags, to flag that the > watchdog is ready to use and adds the pointer to the watchdog device > to the GD. This enables us to remove the global "watchdog_dev" > variable, which was prone to cause problems because of its potentially > very early use in watchdog_reset(), even before the BSS is cleared. > > Signed-off-by: Stefan Roese <sr@denx.de> <snip> > --- a/include/asm-generic/global_data.h > +++ b/include/asm-generic/global_data.h > @@ -133,6 +133,9 @@ typedef struct global_data { > struct spl_handoff *spl_handoff; > # endif > #endif > +#if defined(CONFIG_WDT) > + struct udevice *watchdog_dev; > +#endif > } gd_t; > #endif > > @@ -161,5 +164,6 @@ typedef struct global_data { > #define GD_FLG_ENV_DEFAULT 0x02000 /* Default variable flag */ > #define GD_FLG_SPL_EARLY_INIT 0x04000 /* Early SPL init is done */ > #define GD_FLG_LOG_READY 0x08000 /* Log system is ready for use */ > +#define GD_FLG_WDT_READY 0x10000 /* Watchdog is ready for use */ Sorry to warm up a thread that is more than 4 months old, but I just stumbled accross this line when searching for space in 'gd': The comment some lines above in global_data.h clearly states that the top 16 bits of flags are reserved for arch-specific flags, and your patch here uses the lowest of these 16 arch-specific flags for generic code. Is this a problem? Does any arch code use the upper 16 bits? I would have thought you'd at least need to adjust the comment to reflect your new usage... The reason I ask is that I'd need a place to put some (~5?) 'is_initialized' bits for some code running in SPL in the 'board_init_f' code where BSS shouldn't be used. gd->flags would be ideal for that, but I'm hesistant to dive in further into the 'arch-specific' upper 16 bits... Regards, Simon > > #endif /* __ASM_GENERIC_GBL_DATA_H */ > diff --git a/include/configs/turris_omnia.h b/include/configs/turris_omnia.h > index c7805cf36b..0e65a12345 100644 > --- a/include/configs/turris_omnia.h > +++ b/include/configs/turris_omnia.h > @@ -29,11 +29,6 @@ > #define CONFIG_SPL_I2C_MUX > #define CONFIG_SYS_I2C_MVTWSI > > -/* Watchdog support */ > -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION) > -# define CONFIG_WATCHDOG > -#endif > - > /* > * SDIO/MMC Card Configuration > */ > diff --git a/include/wdt.h b/include/wdt.h > index e9a7c5355a..aa77d3e9b4 100644 > --- a/include/wdt.h > +++ b/include/wdt.h > @@ -6,6 +6,9 @@ > #ifndef _WDT_H_ > #define _WDT_H_ > > +#include <dm.h> > +#include <dm/read.h> > + > /* > * Implement a simple watchdog uclass. Watchdog is basically a timer that > * is used to detect or recover from malfunction. During normal operation > @@ -103,4 +106,42 @@ struct wdt_ops { > int (*expire_now)(struct udevice *dev, ulong flags); > }; > > +#if defined(CONFIG_WDT) > +#ifndef CONFIG_WATCHDOG_TIMEOUT_MSECS > +#define CONFIG_WATCHDOG_TIMEOUT_MSECS (60 * 1000) > +#endif > +#define WATCHDOG_TIMEOUT_SECS (CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000) > + > +static inline int initr_watchdog(void) > +{ > + u32 timeout = WATCHDOG_TIMEOUT_SECS; > + > + /* > + * Init watchdog: This will call the probe function of the > + * watchdog driver, enabling the use of the device > + */ > + if (uclass_get_device_by_seq(UCLASS_WDT, 0, > + (struct udevice **)&gd->watchdog_dev)) { > + debug("WDT: Not found by seq!\n"); > + if (uclass_get_device(UCLASS_WDT, 0, > + (struct udevice **)&gd->watchdog_dev)) { > + printf("WDT: Not found!\n"); > + return 0; > + } > + } > + > + if (CONFIG_IS_ENABLED(OF_CONTROL)) { > + timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec", > + WATCHDOG_TIMEOUT_SECS); > + } > + > + wdt_start(gd->watchdog_dev, timeout * 1000, 0); > + gd->flags |= GD_FLG_WDT_READY; > + printf("WDT: Started with%s servicing (%ds timeout)\n", > + IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", timeout); > + > + return 0; > +} > +#endif > + > #endif /* _WDT_H_ */ >
Hi Simon, (added Simon Glass and Bin to Cc) On 13.08.19 22:16, Simon Goldschmidt wrote: > Am 25.04.2019 um 09:17 schrieb Stefan Roese: >> This patch tries to implement a generic watchdog_reset() function that >> can be used by all boards that want to service the watchdog device in >> U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG. >> >> Without this approach, new boards or platforms needed to implement a >> board specific version of this functionality, mostly copy'ing the same >> code over and over again into their board or platforms code base. >> >> With this new generic function, the scattered other functions are now >> removed to be replaced by the generic one. The new version also enables >> the configuration of the watchdog timeout via the DT "timeout-sec" >> property (if enabled via CONFIG_OF_CONTROL). >> >> This patch also adds a new flag to the GD flags, to flag that the >> watchdog is ready to use and adds the pointer to the watchdog device >> to the GD. This enables us to remove the global "watchdog_dev" >> variable, which was prone to cause problems because of its potentially >> very early use in watchdog_reset(), even before the BSS is cleared. >> >> Signed-off-by: Stefan Roese <sr@denx.de> > > <snip> > >> --- a/include/asm-generic/global_data.h >> +++ b/include/asm-generic/global_data.h >> @@ -133,6 +133,9 @@ typedef struct global_data { >> struct spl_handoff *spl_handoff; >> # endif >> #endif >> +#if defined(CONFIG_WDT) >> + struct udevice *watchdog_dev; >> +#endif >> } gd_t; >> #endif >> >> @@ -161,5 +164,6 @@ typedef struct global_data { >> #define GD_FLG_ENV_DEFAULT 0x02000 /* Default variable flag */ >> #define GD_FLG_SPL_EARLY_INIT 0x04000 /* Early SPL init is done */ >> #define GD_FLG_LOG_READY 0x08000 /* Log system is ready for use */ >> +#define GD_FLG_WDT_READY 0x10000 /* Watchdog is ready for use */ > > Sorry to warm up a thread that is more than 4 months old, but I just > stumbled accross this line when searching for space in 'gd': > > The comment some lines above in global_data.h clearly states that the > top 16 bits of flags are reserved for arch-specific flags, and your > patch here uses the lowest of these 16 arch-specific flags for generic code. I totally missed this comment. > Is this a problem? Does any arch code use the upper 16 bits? I would > have thought you'd at least need to adjust the comment to reflect your > new usage... As stated above, I did not check about any other (arch-specific) GD_FLG_ definitions outside of this file. > The reason I ask is that I'd need a place to put some (~5?) > 'is_initialized' bits for some code running in SPL in the 'board_init_f' > code where BSS shouldn't be used. gd->flags would be ideal for that, but > I'm hesistant to dive in further into the 'arch-specific' upper 16 bits... And you should be. A quick grep shows that we already have a problem with my patch touching the upper bits: $ git grep "define GD_FLG_" arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT 0x10000 /* Cold Boot */ arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x20000 /* Warm Boot */ This should definitely be fixed. I see 3 options right now: a) Reserve only the upper 8 bits for arch-specific stuff b) Use a new variable (gd->flags_arch ?) for this arch c) Remove the arch-specific GD_FLG's completely I can't tell if c) is doable - Bin and / or Simon Glass might know, if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that both are assigned in the .S files, but only GD_FLG_COLD_BOOT is referenced later on: arch/x86/cpu/coreboot/coreboot.c: if (gd->flags & GD_FLG_COLD_BOOT) If c) is not an option, then I would prefer to implement b). Here we could also only add this new "flags_arch" variable for arch's that implement such flags (e.g. x86 right now). I could work on such a patch, if we agree on this solution. Any comments / suggestions? Thanks, Stefan
Hi, On Wed, 14 Aug 2019 at 00:22, Stefan Roese <sr@denx.de> wrote: > > Hi Simon, > > (added Simon Glass and Bin to Cc) > > On 13.08.19 22:16, Simon Goldschmidt wrote: > > Am 25.04.2019 um 09:17 schrieb Stefan Roese: > >> This patch tries to implement a generic watchdog_reset() function that > >> can be used by all boards that want to service the watchdog device in > >> U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG. > >> > >> Without this approach, new boards or platforms needed to implement a > >> board specific version of this functionality, mostly copy'ing the same > >> code over and over again into their board or platforms code base. > >> > >> With this new generic function, the scattered other functions are now > >> removed to be replaced by the generic one. The new version also enables > >> the configuration of the watchdog timeout via the DT "timeout-sec" > >> property (if enabled via CONFIG_OF_CONTROL). > >> > >> This patch also adds a new flag to the GD flags, to flag that the > >> watchdog is ready to use and adds the pointer to the watchdog device > >> to the GD. This enables us to remove the global "watchdog_dev" > >> variable, which was prone to cause problems because of its potentially > >> very early use in watchdog_reset(), even before the BSS is cleared. > >> > >> Signed-off-by: Stefan Roese <sr@denx.de> > > > > <snip> > > > >> --- a/include/asm-generic/global_data.h > >> +++ b/include/asm-generic/global_data.h > >> @@ -133,6 +133,9 @@ typedef struct global_data { > >> struct spl_handoff *spl_handoff; > >> # endif > >> #endif > >> +#if defined(CONFIG_WDT) > >> + struct udevice *watchdog_dev; > >> +#endif > >> } gd_t; > >> #endif > >> > >> @@ -161,5 +164,6 @@ typedef struct global_data { > >> #define GD_FLG_ENV_DEFAULT 0x02000 /* Default variable flag */ > >> #define GD_FLG_SPL_EARLY_INIT 0x04000 /* Early SPL init is done */ > >> #define GD_FLG_LOG_READY 0x08000 /* Log system is ready for use */ > >> +#define GD_FLG_WDT_READY 0x10000 /* Watchdog is ready for use */ > > > > Sorry to warm up a thread that is more than 4 months old, but I just > > stumbled accross this line when searching for space in 'gd': > > > > The comment some lines above in global_data.h clearly states that the > > top 16 bits of flags are reserved for arch-specific flags, and your > > patch here uses the lowest of these 16 arch-specific flags for generic code. > > I totally missed this comment. > > > Is this a problem? Does any arch code use the upper 16 bits? I would > > have thought you'd at least need to adjust the comment to reflect your > > new usage... > > As stated above, I did not check about any other (arch-specific) > GD_FLG_ definitions outside of this file. > > > The reason I ask is that I'd need a place to put some (~5?) > > 'is_initialized' bits for some code running in SPL in the 'board_init_f' > > code where BSS shouldn't be used. gd->flags would be ideal for that, but > > I'm hesistant to dive in further into the 'arch-specific' upper 16 bits... > > And you should be. A quick grep shows that we already have a problem with > my patch touching the upper bits: > > $ git grep "define GD_FLG_" > arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT 0x10000 /* Cold Boot */ > arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x20000 /* Warm Boot */ > > This should definitely be fixed. I see 3 options right now: > > a) Reserve only the upper 8 bits for arch-specific stuff > b) Use a new variable (gd->flags_arch ?) for this arch > c) Remove the arch-specific GD_FLG's completely > > I can't tell if c) is doable - Bin and / or Simon Glass might know, > if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that > both are assigned in the .S files, but only GD_FLG_COLD_BOOT is > referenced later on: Probably we can drop warm boot. > > arch/x86/cpu/coreboot/coreboot.c: if (gd->flags & GD_FLG_COLD_BOOT) > > If c) is not an option, then I would prefer to implement b). Here > we could also only add this new "flags_arch" variable for arch's > that implement such flags (e.g. x86 right now). I could work on such > a patch, if we agree on this solution. > > Any comments / suggestions? I'm not keen on arch-specific flags here. They should be in gd->arch... if needed. global_data is current used by generic code. So maybe move the x86 flag to the generic header? Regards, Simon
Hi Simon, On 14.08.19 21:35, Simon Glass wrote: > Hi, > > On Wed, 14 Aug 2019 at 00:22, Stefan Roese <sr@denx.de> wrote: >> >> Hi Simon, >> >> (added Simon Glass and Bin to Cc) >> >> On 13.08.19 22:16, Simon Goldschmidt wrote: >>> Am 25.04.2019 um 09:17 schrieb Stefan Roese: >>>> This patch tries to implement a generic watchdog_reset() function that >>>> can be used by all boards that want to service the watchdog device in >>>> U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG. >>>> >>>> Without this approach, new boards or platforms needed to implement a >>>> board specific version of this functionality, mostly copy'ing the same >>>> code over and over again into their board or platforms code base. >>>> >>>> With this new generic function, the scattered other functions are now >>>> removed to be replaced by the generic one. The new version also enables >>>> the configuration of the watchdog timeout via the DT "timeout-sec" >>>> property (if enabled via CONFIG_OF_CONTROL). >>>> >>>> This patch also adds a new flag to the GD flags, to flag that the >>>> watchdog is ready to use and adds the pointer to the watchdog device >>>> to the GD. This enables us to remove the global "watchdog_dev" >>>> variable, which was prone to cause problems because of its potentially >>>> very early use in watchdog_reset(), even before the BSS is cleared. >>>> >>>> Signed-off-by: Stefan Roese <sr@denx.de> >>> >>> <snip> >>> >>>> --- a/include/asm-generic/global_data.h >>>> +++ b/include/asm-generic/global_data.h >>>> @@ -133,6 +133,9 @@ typedef struct global_data { >>>> struct spl_handoff *spl_handoff; >>>> # endif >>>> #endif >>>> +#if defined(CONFIG_WDT) >>>> + struct udevice *watchdog_dev; >>>> +#endif >>>> } gd_t; >>>> #endif >>>> >>>> @@ -161,5 +164,6 @@ typedef struct global_data { >>>> #define GD_FLG_ENV_DEFAULT 0x02000 /* Default variable flag */ >>>> #define GD_FLG_SPL_EARLY_INIT 0x04000 /* Early SPL init is done */ >>>> #define GD_FLG_LOG_READY 0x08000 /* Log system is ready for use */ >>>> +#define GD_FLG_WDT_READY 0x10000 /* Watchdog is ready for use */ >>> >>> Sorry to warm up a thread that is more than 4 months old, but I just >>> stumbled accross this line when searching for space in 'gd': >>> >>> The comment some lines above in global_data.h clearly states that the >>> top 16 bits of flags are reserved for arch-specific flags, and your >>> patch here uses the lowest of these 16 arch-specific flags for generic code. >> >> I totally missed this comment. >> >>> Is this a problem? Does any arch code use the upper 16 bits? I would >>> have thought you'd at least need to adjust the comment to reflect your >>> new usage... >> >> As stated above, I did not check about any other (arch-specific) >> GD_FLG_ definitions outside of this file. >> >>> The reason I ask is that I'd need a place to put some (~5?) >>> 'is_initialized' bits for some code running in SPL in the 'board_init_f' >>> code where BSS shouldn't be used. gd->flags would be ideal for that, but >>> I'm hesistant to dive in further into the 'arch-specific' upper 16 bits... >> >> And you should be. A quick grep shows that we already have a problem with >> my patch touching the upper bits: >> >> $ git grep "define GD_FLG_" >> arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT 0x10000 /* Cold Boot */ >> arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x20000 /* Warm Boot */ >> >> This should definitely be fixed. I see 3 options right now: >> >> a) Reserve only the upper 8 bits for arch-specific stuff >> b) Use a new variable (gd->flags_arch ?) for this arch >> c) Remove the arch-specific GD_FLG's completely >> >> I can't tell if c) is doable - Bin and / or Simon Glass might know, >> if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that >> both are assigned in the .S files, but only GD_FLG_COLD_BOOT is >> referenced later on: > > Probably we can drop warm boot. Bin, do you think so as well? >> >> arch/x86/cpu/coreboot/coreboot.c: if (gd->flags & GD_FLG_COLD_BOOT) >> >> If c) is not an option, then I would prefer to implement b). Here >> we could also only add this new "flags_arch" variable for arch's >> that implement such flags (e.g. x86 right now). I could work on such >> a patch, if we agree on this solution. >> >> Any comments / suggestions? > > I'm not keen on arch-specific flags here. They should be in > gd->arch... if needed. global_data is current used by generic code. > > So maybe move the x86 flag to the generic header? Yes, I also think that those flags should not be sprinkled in different headers but collected in the generic header. I'll prepare a patch to move the x86 flags and remove the comment about the upper 16 bits usage. We can remove the x86 warm boot flag later, if this isn't used at all. Thanks, Stefan
Hi Stefan, On Thu, Aug 15, 2019 at 2:07 PM Stefan Roese <sr@denx.de> wrote: > > Hi Simon, > > On 14.08.19 21:35, Simon Glass wrote: > > Hi, > > > > On Wed, 14 Aug 2019 at 00:22, Stefan Roese <sr@denx.de> wrote: > >> > >> Hi Simon, > >> > >> (added Simon Glass and Bin to Cc) > >> > >> On 13.08.19 22:16, Simon Goldschmidt wrote: > >>> Am 25.04.2019 um 09:17 schrieb Stefan Roese: > >>>> This patch tries to implement a generic watchdog_reset() function that > >>>> can be used by all boards that want to service the watchdog device in > >>>> U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG. > >>>> > >>>> Without this approach, new boards or platforms needed to implement a > >>>> board specific version of this functionality, mostly copy'ing the same > >>>> code over and over again into their board or platforms code base. > >>>> > >>>> With this new generic function, the scattered other functions are now > >>>> removed to be replaced by the generic one. The new version also enables > >>>> the configuration of the watchdog timeout via the DT "timeout-sec" > >>>> property (if enabled via CONFIG_OF_CONTROL). > >>>> > >>>> This patch also adds a new flag to the GD flags, to flag that the > >>>> watchdog is ready to use and adds the pointer to the watchdog device > >>>> to the GD. This enables us to remove the global "watchdog_dev" > >>>> variable, which was prone to cause problems because of its potentially > >>>> very early use in watchdog_reset(), even before the BSS is cleared. > >>>> > >>>> Signed-off-by: Stefan Roese <sr@denx.de> > >>> > >>> <snip> > >>> > >>>> --- a/include/asm-generic/global_data.h > >>>> +++ b/include/asm-generic/global_data.h > >>>> @@ -133,6 +133,9 @@ typedef struct global_data { > >>>> struct spl_handoff *spl_handoff; > >>>> # endif > >>>> #endif > >>>> +#if defined(CONFIG_WDT) > >>>> + struct udevice *watchdog_dev; > >>>> +#endif > >>>> } gd_t; > >>>> #endif > >>>> > >>>> @@ -161,5 +164,6 @@ typedef struct global_data { > >>>> #define GD_FLG_ENV_DEFAULT 0x02000 /* Default variable flag */ > >>>> #define GD_FLG_SPL_EARLY_INIT 0x04000 /* Early SPL init is done */ > >>>> #define GD_FLG_LOG_READY 0x08000 /* Log system is ready for use */ > >>>> +#define GD_FLG_WDT_READY 0x10000 /* Watchdog is ready for use */ > >>> > >>> Sorry to warm up a thread that is more than 4 months old, but I just > >>> stumbled accross this line when searching for space in 'gd': > >>> > >>> The comment some lines above in global_data.h clearly states that the > >>> top 16 bits of flags are reserved for arch-specific flags, and your > >>> patch here uses the lowest of these 16 arch-specific flags for generic code. > >> > >> I totally missed this comment. > >> > >>> Is this a problem? Does any arch code use the upper 16 bits? I would > >>> have thought you'd at least need to adjust the comment to reflect your > >>> new usage... > >> > >> As stated above, I did not check about any other (arch-specific) > >> GD_FLG_ definitions outside of this file. > >> > >>> The reason I ask is that I'd need a place to put some (~5?) > >>> 'is_initialized' bits for some code running in SPL in the 'board_init_f' > >>> code where BSS shouldn't be used. gd->flags would be ideal for that, but > >>> I'm hesistant to dive in further into the 'arch-specific' upper 16 bits... > >> > >> And you should be. A quick grep shows that we already have a problem with > >> my patch touching the upper bits: > >> > >> $ git grep "define GD_FLG_" > >> arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT 0x10000 /* Cold Boot */ > >> arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x20000 /* Warm Boot */ > >> > >> This should definitely be fixed. I see 3 options right now: > >> > >> a) Reserve only the upper 8 bits for arch-specific stuff > >> b) Use a new variable (gd->flags_arch ?) for this arch > >> c) Remove the arch-specific GD_FLG's completely > >> > >> I can't tell if c) is doable - Bin and / or Simon Glass might know, > >> if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that > >> both are assigned in the .S files, but only GD_FLG_COLD_BOOT is > >> referenced later on: > > > > Probably we can drop warm boot. > > Bin, do you think so as well? > I believe we can drop these 2 flags completely. Currently usage of warm/cold boot flags is only limited to coreboot codes. arch/x86/cpu/coreboot/coreboot.c::last_stage_init() if (gd->flags & GD_FLG_COLD_BOOT) timestamp_add_to_bootstage(); timestamp_add_to_bootstage() will never be called for coreboot. > >> > >> arch/x86/cpu/coreboot/coreboot.c: if (gd->flags & GD_FLG_COLD_BOOT) > >> > >> If c) is not an option, then I would prefer to implement b). Here > >> we could also only add this new "flags_arch" variable for arch's > >> that implement such flags (e.g. x86 right now). I could work on such > >> a patch, if we agree on this solution. > >> > >> Any comments / suggestions? > > > > I'm not keen on arch-specific flags here. They should be in > > gd->arch... if needed. global_data is current used by generic code. > > > > So maybe move the x86 flag to the generic header? > > Yes, I also think that those flags should not be sprinkled in different > headers but collected in the generic header. I'll prepare a patch to > move the x86 flags and remove the comment about the upper 16 bits > usage. We can remove the x86 warm boot flag later, if this isn't > used at all. > Regards, Bin
Hi Bin, On 15.08.19 16:19, Bin Meng wrote: > Hi Stefan, > > On Thu, Aug 15, 2019 at 2:07 PM Stefan Roese <sr@denx.de> wrote: >> >> Hi Simon, >> >> On 14.08.19 21:35, Simon Glass wrote: >>> Hi, >>> >>> On Wed, 14 Aug 2019 at 00:22, Stefan Roese <sr@denx.de> wrote: >>>> >>>> Hi Simon, >>>> >>>> (added Simon Glass and Bin to Cc) >>>> >>>> On 13.08.19 22:16, Simon Goldschmidt wrote: >>>>> Am 25.04.2019 um 09:17 schrieb Stefan Roese: >>>>>> This patch tries to implement a generic watchdog_reset() function that >>>>>> can be used by all boards that want to service the watchdog device in >>>>>> U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG. >>>>>> >>>>>> Without this approach, new boards or platforms needed to implement a >>>>>> board specific version of this functionality, mostly copy'ing the same >>>>>> code over and over again into their board or platforms code base. >>>>>> >>>>>> With this new generic function, the scattered other functions are now >>>>>> removed to be replaced by the generic one. The new version also enables >>>>>> the configuration of the watchdog timeout via the DT "timeout-sec" >>>>>> property (if enabled via CONFIG_OF_CONTROL). >>>>>> >>>>>> This patch also adds a new flag to the GD flags, to flag that the >>>>>> watchdog is ready to use and adds the pointer to the watchdog device >>>>>> to the GD. This enables us to remove the global "watchdog_dev" >>>>>> variable, which was prone to cause problems because of its potentially >>>>>> very early use in watchdog_reset(), even before the BSS is cleared. >>>>>> >>>>>> Signed-off-by: Stefan Roese <sr@denx.de> >>>>> >>>>> <snip> >>>>> >>>>>> --- a/include/asm-generic/global_data.h >>>>>> +++ b/include/asm-generic/global_data.h >>>>>> @@ -133,6 +133,9 @@ typedef struct global_data { >>>>>> struct spl_handoff *spl_handoff; >>>>>> # endif >>>>>> #endif >>>>>> +#if defined(CONFIG_WDT) >>>>>> + struct udevice *watchdog_dev; >>>>>> +#endif >>>>>> } gd_t; >>>>>> #endif >>>>>> >>>>>> @@ -161,5 +164,6 @@ typedef struct global_data { >>>>>> #define GD_FLG_ENV_DEFAULT 0x02000 /* Default variable flag */ >>>>>> #define GD_FLG_SPL_EARLY_INIT 0x04000 /* Early SPL init is done */ >>>>>> #define GD_FLG_LOG_READY 0x08000 /* Log system is ready for use */ >>>>>> +#define GD_FLG_WDT_READY 0x10000 /* Watchdog is ready for use */ >>>>> >>>>> Sorry to warm up a thread that is more than 4 months old, but I just >>>>> stumbled accross this line when searching for space in 'gd': >>>>> >>>>> The comment some lines above in global_data.h clearly states that the >>>>> top 16 bits of flags are reserved for arch-specific flags, and your >>>>> patch here uses the lowest of these 16 arch-specific flags for generic code. >>>> >>>> I totally missed this comment. >>>> >>>>> Is this a problem? Does any arch code use the upper 16 bits? I would >>>>> have thought you'd at least need to adjust the comment to reflect your >>>>> new usage... >>>> >>>> As stated above, I did not check about any other (arch-specific) >>>> GD_FLG_ definitions outside of this file. >>>> >>>>> The reason I ask is that I'd need a place to put some (~5?) >>>>> 'is_initialized' bits for some code running in SPL in the 'board_init_f' >>>>> code where BSS shouldn't be used. gd->flags would be ideal for that, but >>>>> I'm hesistant to dive in further into the 'arch-specific' upper 16 bits... >>>> >>>> And you should be. A quick grep shows that we already have a problem with >>>> my patch touching the upper bits: >>>> >>>> $ git grep "define GD_FLG_" >>>> arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT 0x10000 /* Cold Boot */ >>>> arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x20000 /* Warm Boot */ >>>> >>>> This should definitely be fixed. I see 3 options right now: >>>> >>>> a) Reserve only the upper 8 bits for arch-specific stuff >>>> b) Use a new variable (gd->flags_arch ?) for this arch >>>> c) Remove the arch-specific GD_FLG's completely >>>> >>>> I can't tell if c) is doable - Bin and / or Simon Glass might know, >>>> if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that >>>> both are assigned in the .S files, but only GD_FLG_COLD_BOOT is >>>> referenced later on: >>> >>> Probably we can drop warm boot. >> >> Bin, do you think so as well? >> > > I believe we can drop these 2 flags completely. Currently usage of > warm/cold boot flags is only limited to coreboot codes. > > arch/x86/cpu/coreboot/coreboot.c::last_stage_init() > > if (gd->flags & GD_FLG_COLD_BOOT) > timestamp_add_to_bootstage(); > > timestamp_add_to_bootstage() will never be called for coreboot. Why is this the case? Will GD_FLG_COLD_BOOT never be set for the coreboot target? Thanks, Stefan
Hi Stefan, On Fri, Aug 16, 2019 at 1:11 PM Stefan Roese <sr@denx.de> wrote: > > Hi Bin, > > On 15.08.19 16:19, Bin Meng wrote: > > Hi Stefan, > > > > On Thu, Aug 15, 2019 at 2:07 PM Stefan Roese <sr@denx.de> wrote: > >> > >> Hi Simon, > >> > >> On 14.08.19 21:35, Simon Glass wrote: > >>> Hi, > >>> > >>> On Wed, 14 Aug 2019 at 00:22, Stefan Roese <sr@denx.de> wrote: > >>>> > >>>> Hi Simon, > >>>> > >>>> (added Simon Glass and Bin to Cc) > >>>> > >>>> On 13.08.19 22:16, Simon Goldschmidt wrote: > >>>>> Am 25.04.2019 um 09:17 schrieb Stefan Roese: > >>>>>> This patch tries to implement a generic watchdog_reset() function that > >>>>>> can be used by all boards that want to service the watchdog device in > >>>>>> U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG. > >>>>>> > >>>>>> Without this approach, new boards or platforms needed to implement a > >>>>>> board specific version of this functionality, mostly copy'ing the same > >>>>>> code over and over again into their board or platforms code base. > >>>>>> > >>>>>> With this new generic function, the scattered other functions are now > >>>>>> removed to be replaced by the generic one. The new version also enables > >>>>>> the configuration of the watchdog timeout via the DT "timeout-sec" > >>>>>> property (if enabled via CONFIG_OF_CONTROL). > >>>>>> > >>>>>> This patch also adds a new flag to the GD flags, to flag that the > >>>>>> watchdog is ready to use and adds the pointer to the watchdog device > >>>>>> to the GD. This enables us to remove the global "watchdog_dev" > >>>>>> variable, which was prone to cause problems because of its potentially > >>>>>> very early use in watchdog_reset(), even before the BSS is cleared. > >>>>>> > >>>>>> Signed-off-by: Stefan Roese <sr@denx.de> > >>>>> > >>>>> <snip> > >>>>> > >>>>>> --- a/include/asm-generic/global_data.h > >>>>>> +++ b/include/asm-generic/global_data.h > >>>>>> @@ -133,6 +133,9 @@ typedef struct global_data { > >>>>>> struct spl_handoff *spl_handoff; > >>>>>> # endif > >>>>>> #endif > >>>>>> +#if defined(CONFIG_WDT) > >>>>>> + struct udevice *watchdog_dev; > >>>>>> +#endif > >>>>>> } gd_t; > >>>>>> #endif > >>>>>> > >>>>>> @@ -161,5 +164,6 @@ typedef struct global_data { > >>>>>> #define GD_FLG_ENV_DEFAULT 0x02000 /* Default variable flag */ > >>>>>> #define GD_FLG_SPL_EARLY_INIT 0x04000 /* Early SPL init is done */ > >>>>>> #define GD_FLG_LOG_READY 0x08000 /* Log system is ready for use */ > >>>>>> +#define GD_FLG_WDT_READY 0x10000 /* Watchdog is ready for use */ > >>>>> > >>>>> Sorry to warm up a thread that is more than 4 months old, but I just > >>>>> stumbled accross this line when searching for space in 'gd': > >>>>> > >>>>> The comment some lines above in global_data.h clearly states that the > >>>>> top 16 bits of flags are reserved for arch-specific flags, and your > >>>>> patch here uses the lowest of these 16 arch-specific flags for generic code. > >>>> > >>>> I totally missed this comment. > >>>> > >>>>> Is this a problem? Does any arch code use the upper 16 bits? I would > >>>>> have thought you'd at least need to adjust the comment to reflect your > >>>>> new usage... > >>>> > >>>> As stated above, I did not check about any other (arch-specific) > >>>> GD_FLG_ definitions outside of this file. > >>>> > >>>>> The reason I ask is that I'd need a place to put some (~5?) > >>>>> 'is_initialized' bits for some code running in SPL in the 'board_init_f' > >>>>> code where BSS shouldn't be used. gd->flags would be ideal for that, but > >>>>> I'm hesistant to dive in further into the 'arch-specific' upper 16 bits... > >>>> > >>>> And you should be. A quick grep shows that we already have a problem with > >>>> my patch touching the upper bits: > >>>> > >>>> $ git grep "define GD_FLG_" > >>>> arch/x86/include/asm/global_data.h:#define GD_FLG_COLD_BOOT 0x10000 /* Cold Boot */ > >>>> arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x20000 /* Warm Boot */ > >>>> > >>>> This should definitely be fixed. I see 3 options right now: > >>>> > >>>> a) Reserve only the upper 8 bits for arch-specific stuff > >>>> b) Use a new variable (gd->flags_arch ?) for this arch > >>>> c) Remove the arch-specific GD_FLG's completely > >>>> > >>>> I can't tell if c) is doable - Bin and / or Simon Glass might know, > >>>> if the x86 GD_FLG_foo_BOOT are really needed in gd->flags. I see that > >>>> both are assigned in the .S files, but only GD_FLG_COLD_BOOT is > >>>> referenced later on: > >>> > >>> Probably we can drop warm boot. > >> > >> Bin, do you think so as well? > >> > > > > I believe we can drop these 2 flags completely. Currently usage of > > warm/cold boot flags is only limited to coreboot codes. > > > > arch/x86/cpu/coreboot/coreboot.c::last_stage_init() > > > > if (gd->flags & GD_FLG_COLD_BOOT) > > timestamp_add_to_bootstage(); > > > > timestamp_add_to_bootstage() will never be called for coreboot. > > Why is this the case? Will GD_FLG_COLD_BOOT never be set for the > coreboot target? GD_FLG_COLD_BOOT is only set in the 16-bit start code while on coreboot it boots directly from the 32-bit start code. Regards, Bin
Le 25/04/2019 à 09:17, Stefan Roese a écrit : > This patch tries to implement a generic watchdog_reset() function that > can be used by all boards that want to service the watchdog device in > U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG. > > Without this approach, new boards or platforms needed to implement a > board specific version of this functionality, mostly copy'ing the same > code over and over again into their board or platforms code base. > > With this new generic function, the scattered other functions are now > removed to be replaced by the generic one. The new version also enables > the configuration of the watchdog timeout via the DT "timeout-sec" > property (if enabled via CONFIG_OF_CONTROL). > > This patch also adds a new flag to the GD flags, to flag that the > watchdog is ready to use and adds the pointer to the watchdog device > to the GD. This enables us to remove the global "watchdog_dev" > variable, which was prone to cause problems because of its potentially > very early use in watchdog_reset(), even before the BSS is cleared. > > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Heiko Schocher <hs@denx.de> > Cc: Tom Rini <trini@konsulko.com> > Cc: Michal Simek <michal.simek@xilinx.com> > Cc: "Marek Behún" <marek.behun@nic.cz> > Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> > Cc: Maxim Sloyko <maxims@google.com> > Cc: Erik van Luijk <evanluijk@interact.nl> > Cc: Ryder Lee <ryder.lee@mediatek.com> > Cc: Weijie Gao <weijie.gao@mediatek.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: "Álvaro Fernández Rojas" <noltari@gmail.com> > Cc: Philippe Reynes <philippe.reynes@softathome.com> > Cc: Christophe Leroy <christophe.leroy@c-s.fr> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> > Reviewed-by: Michal Simek <michal.simek@xilinx.com> > Tested-by: Michal Simek <michal.simek@xilinx.com> (on zcu100) > --- [...] > diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c > index 23b7e3360d..bbfac4f0f9 100644 > --- a/drivers/watchdog/wdt-uclass.c > +++ b/drivers/watchdog/wdt-uclass.c > @@ -10,6 +10,8 @@ > #include <dm/device-internal.h> > #include <dm/lists.h> > > +DECLARE_GLOBAL_DATA_PTR; > + > int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) > { > const struct wdt_ops *ops = device_get_ops(dev); > @@ -63,6 +65,30 @@ int wdt_expire_now(struct udevice *dev, ulong flags) > return ret; > } > > +#if defined(CONFIG_WATCHDOG) > +/* > + * Called by macro WATCHDOG_RESET. This function be called *very* early, > + * so we need to make sure, that the watchdog driver is ready before using > + * it in this function. > + */ > +void watchdog_reset(void) > +{ > + static ulong next_reset; > + ulong now; > + > + /* Exit if GD is not ready or watchdog is not initialized yet */ > + if (!gd || !(gd->flags & GD_FLG_WDT_READY)) > + return; > + > + /* Do not reset the watchdog too often */ > + now = get_timer(0); > + if (now > next_reset) { > + next_reset = now + 1000; /* reset every 1000ms */ > + wdt_reset(gd->watchdog_dev); > + } This is a problem for the MPC8xx. When running with a MPC8xx at 132MHz clock, the watchdog will fire about 1s after the last refresh. So the above makes the board unusable. Is that really necessary to restrict the refresh like this in the core part of Watchdog driver ? > +} > +#endif > + > static int wdt_post_bind(struct udevice *dev) > { > #if defined(CONFIG_NEEDS_MANUAL_RELOC) Thanks, Christophe
Hi Christophe On 19.02.20 20:21, Christophe Leroy wrote: > > > Le 25/04/2019 à 09:17, Stefan Roese a écrit : >> This patch tries to implement a generic watchdog_reset() function that >> can be used by all boards that want to service the watchdog device in >> U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG. >> >> Without this approach, new boards or platforms needed to implement a >> board specific version of this functionality, mostly copy'ing the same >> code over and over again into their board or platforms code base. >> >> With this new generic function, the scattered other functions are now >> removed to be replaced by the generic one. The new version also enables >> the configuration of the watchdog timeout via the DT "timeout-sec" >> property (if enabled via CONFIG_OF_CONTROL). >> >> This patch also adds a new flag to the GD flags, to flag that the >> watchdog is ready to use and adds the pointer to the watchdog device >> to the GD. This enables us to remove the global "watchdog_dev" >> variable, which was prone to cause problems because of its potentially >> very early use in watchdog_reset(), even before the BSS is cleared. >> >> Signed-off-by: Stefan Roese <sr@denx.de> >> Cc: Heiko Schocher <hs@denx.de> >> Cc: Tom Rini <trini@konsulko.com> >> Cc: Michal Simek <michal.simek@xilinx.com> >> Cc: "Marek Behún" <marek.behun@nic.cz> >> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >> Cc: Maxim Sloyko <maxims@google.com> >> Cc: Erik van Luijk <evanluijk@interact.nl> >> Cc: Ryder Lee <ryder.lee@mediatek.com> >> Cc: Weijie Gao <weijie.gao@mediatek.com> >> Cc: Simon Glass <sjg@chromium.org> >> Cc: "Álvaro Fernández Rojas" <noltari@gmail.com> >> Cc: Philippe Reynes <philippe.reynes@softathome.com> >> Cc: Christophe Leroy <christophe.leroy@c-s.fr> >> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> >> Reviewed-by: Michal Simek <michal.simek@xilinx.com> >> Tested-by: Michal Simek <michal.simek@xilinx.com> (on zcu100) >> --- > > [...] > >> diff --git a/drivers/watchdog/wdt-uclass.c >> b/drivers/watchdog/wdt-uclass.c >> index 23b7e3360d..bbfac4f0f9 100644 >> --- a/drivers/watchdog/wdt-uclass.c >> +++ b/drivers/watchdog/wdt-uclass.c >> @@ -10,6 +10,8 @@ >> #include <dm/device-internal.h> >> #include <dm/lists.h> >> +DECLARE_GLOBAL_DATA_PTR; >> + >> int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) >> { >> const struct wdt_ops *ops = device_get_ops(dev); >> @@ -63,6 +65,30 @@ int wdt_expire_now(struct udevice *dev, ulong flags) >> return ret; >> } >> +#if defined(CONFIG_WATCHDOG) >> +/* >> + * Called by macro WATCHDOG_RESET. This function be called *very* early, >> + * so we need to make sure, that the watchdog driver is ready before >> using >> + * it in this function. >> + */ >> +void watchdog_reset(void) >> +{ >> + static ulong next_reset; >> + ulong now; >> + >> + /* Exit if GD is not ready or watchdog is not initialized yet */ >> + if (!gd || !(gd->flags & GD_FLG_WDT_READY)) >> + return; >> + >> + /* Do not reset the watchdog too often */ >> + now = get_timer(0); >> + if (now > next_reset) { >> + next_reset = now + 1000; /* reset every 1000ms */ >> + wdt_reset(gd->watchdog_dev); >> + } > > This is a problem for the MPC8xx. > > When running with a MPC8xx at 132MHz clock, the watchdog will fire about > 1s after the last refresh. So the above makes the board unusable. So you need a shorted delay between the wdt_reset() calls? Is this correct? We could introduce a new Kconfig option which defaults to 1000 (ms) and you can "select" a shorter value for MPC8xx. Would this work for you? > Is that really necessary to restrict the refresh like this in the core > part of Watchdog driver ? It seems to work for most cases (all until I hear of this MPC8xx issue). Thanks, Stefan
On 02/20/2020 05:43 AM, Stefan Roese wrote: > Hi Christophe > > On 19.02.20 20:21, Christophe Leroy wrote: >> >> >> Le 25/04/2019 à 09:17, Stefan Roese a écrit : >>> This patch tries to implement a generic watchdog_reset() function that >>> can be used by all boards that want to service the watchdog device in >>> U-Boot. This watchdog servicing is enabled via CONFIG_WATCHDOG. >>> >>> Without this approach, new boards or platforms needed to implement a >>> board specific version of this functionality, mostly copy'ing the same >>> code over and over again into their board or platforms code base. >>> >>> With this new generic function, the scattered other functions are now >>> removed to be replaced by the generic one. The new version also enables >>> the configuration of the watchdog timeout via the DT "timeout-sec" >>> property (if enabled via CONFIG_OF_CONTROL). >>> >>> This patch also adds a new flag to the GD flags, to flag that the >>> watchdog is ready to use and adds the pointer to the watchdog device >>> to the GD. This enables us to remove the global "watchdog_dev" >>> variable, which was prone to cause problems because of its potentially >>> very early use in watchdog_reset(), even before the BSS is cleared. >>> >>> Signed-off-by: Stefan Roese <sr@denx.de> >>> Cc: Heiko Schocher <hs@denx.de> >>> Cc: Tom Rini <trini@konsulko.com> >>> Cc: Michal Simek <michal.simek@xilinx.com> >>> Cc: "Marek Behún" <marek.behun@nic.cz> >>> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> >>> Cc: Maxim Sloyko <maxims@google.com> >>> Cc: Erik van Luijk <evanluijk@interact.nl> >>> Cc: Ryder Lee <ryder.lee@mediatek.com> >>> Cc: Weijie Gao <weijie.gao@mediatek.com> >>> Cc: Simon Glass <sjg@chromium.org> >>> Cc: "Álvaro Fernández Rojas" <noltari@gmail.com> >>> Cc: Philippe Reynes <philippe.reynes@softathome.com> >>> Cc: Christophe Leroy <christophe.leroy@c-s.fr> >>> Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> >>> Reviewed-by: Michal Simek <michal.simek@xilinx.com> >>> Tested-by: Michal Simek <michal.simek@xilinx.com> (on zcu100) >>> --- >> >> [...] >> >>> diff --git a/drivers/watchdog/wdt-uclass.c >>> b/drivers/watchdog/wdt-uclass.c >>> index 23b7e3360d..bbfac4f0f9 100644 >>> --- a/drivers/watchdog/wdt-uclass.c >>> +++ b/drivers/watchdog/wdt-uclass.c >>> @@ -10,6 +10,8 @@ >>> #include <dm/device-internal.h> >>> #include <dm/lists.h> >>> +DECLARE_GLOBAL_DATA_PTR; >>> + >>> int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) >>> { >>> const struct wdt_ops *ops = device_get_ops(dev); >>> @@ -63,6 +65,30 @@ int wdt_expire_now(struct udevice *dev, ulong flags) >>> return ret; >>> } >>> +#if defined(CONFIG_WATCHDOG) >>> +/* >>> + * Called by macro WATCHDOG_RESET. This function be called *very* >>> early, >>> + * so we need to make sure, that the watchdog driver is ready before >>> using >>> + * it in this function. >>> + */ >>> +void watchdog_reset(void) >>> +{ >>> + static ulong next_reset; >>> + ulong now; >>> + >>> + /* Exit if GD is not ready or watchdog is not initialized yet */ >>> + if (!gd || !(gd->flags & GD_FLG_WDT_READY)) >>> + return; >>> + >>> + /* Do not reset the watchdog too often */ >>> + now = get_timer(0); >>> + if (now > next_reset) { >>> + next_reset = now + 1000; /* reset every 1000ms */ >>> + wdt_reset(gd->watchdog_dev); >>> + } >> >> This is a problem for the MPC8xx. >> >> When running with a MPC8xx at 132MHz clock, the watchdog will fire >> about 1s after the last refresh. So the above makes the board unusable. > > So you need a shorted delay between the wdt_reset() calls? Is this > correct? We could introduce a new Kconfig option which defaults to > 1000 (ms) and you can "select" a shorter value for MPC8xx. Exactly. However, why is this limitation needed at all ? Why is it a problem to refresh more often ? Christophe
On 20.02.20 07:38, Christophe Leroy wrote: <snip> >>>> +void watchdog_reset(void) >>>> +{ >>>> + static ulong next_reset; >>>> + ulong now; >>>> + >>>> + /* Exit if GD is not ready or watchdog is not initialized yet */ >>>> + if (!gd || !(gd->flags & GD_FLG_WDT_READY)) >>>> + return; >>>> + >>>> + /* Do not reset the watchdog too often */ >>>> + now = get_timer(0); >>>> + if (now > next_reset) { >>>> + next_reset = now + 1000; /* reset every 1000ms */ >>>> + wdt_reset(gd->watchdog_dev); >>>> + } >>> >>> This is a problem for the MPC8xx. >>> >>> When running with a MPC8xx at 132MHz clock, the watchdog will fire >>> about 1s after the last refresh. So the above makes the board unusable. >> >> So you need a shorted delay between the wdt_reset() calls? Is this >> correct? We could introduce a new Kconfig option which defaults to >> 1000 (ms) and you can "select" a shorter value for MPC8xx. > > Exactly. However, why is this limitation needed at all ? Why is it a > problem to refresh more often ? Very likely its not. What is a reasonable value for your platform? 100 or 500ms? I think we could change it to default to a shorter value, but such a change should go in early in the merge window, so that other platforms have a bit of time to test it. Please feel free to send a patch for this and please add a comment to explain, why the delay is this "short". Thanks, Stefan
On 20/02/2020 07.43, Stefan Roese wrote: > On 20.02.20 07:38, Christophe Leroy wrote: > > <snip> > >>>>> +void watchdog_reset(void) >>>>> +{ >>>>> + static ulong next_reset; >>>>> + ulong now; >>>>> + >>>>> + /* Exit if GD is not ready or watchdog is not initialized yet */ >>>>> + if (!gd || !(gd->flags & GD_FLG_WDT_READY)) >>>>> + return; >>>>> + >>>>> + /* Do not reset the watchdog too often */ >>>>> + now = get_timer(0); >>>>> + if (now > next_reset) { >>>>> + next_reset = now + 1000; /* reset every 1000ms */ >>>>> + wdt_reset(gd->watchdog_dev); >>>>> + } >>>> >>>> This is a problem for the MPC8xx. >>>> >>>> When running with a MPC8xx at 132MHz clock, the watchdog will fire >>>> about 1s after the last refresh. So the above makes the board unusable. >>> >>> So you need a shorted delay between the wdt_reset() calls? Is this >>> correct? We could introduce a new Kconfig option which defaults to >>> 1000 (ms) and you can "select" a shorter value for MPC8xx. >> >> Exactly. However, why is this limitation needed at all ? Why is it a >> problem to refresh more often ? > > Very likely its not. What is a reasonable value for your platform? 100 > or 500ms? I think we could change it to default to a shorter value, but > such a change should go in early in the merge window, so that other > platforms have a bit of time to test it. > > Please feel free to send a patch for this and please add a comment to > explain, why the delay is this "short". IMO, this should come from DT. For a gpio watchdog (which for some reason U-Boot doesn't have a generic driver for) the linux kernel uses a hw_margin_ms property that tells the core how often the watchdog must be pinged - that could be generalized to apply for all, with 1000ms as a default if not set. And I've seen boards with a gpio watchdog with a timeout of 200 ms. Also, I'm wondering why that generic _reset only handles one watchdog device? I can easily imagine needing to reset both, say, an external gpio-triggered one and also the SOC's/CPU's built-in one. Why not loop over all DM watchdogs, and have the next_reset/hw_margin etc. metadata live with the watchdog device instead of in static variable/build-time constants? Rasmus
On 20.02.20 08:43, Rasmus Villemoes wrote: > On 20/02/2020 07.43, Stefan Roese wrote: >> On 20.02.20 07:38, Christophe Leroy wrote: >> >> <snip> >> >>>>>> +void watchdog_reset(void) >>>>>> +{ >>>>>> + static ulong next_reset; >>>>>> + ulong now; >>>>>> + >>>>>> + /* Exit if GD is not ready or watchdog is not initialized yet */ >>>>>> + if (!gd || !(gd->flags & GD_FLG_WDT_READY)) >>>>>> + return; >>>>>> + >>>>>> + /* Do not reset the watchdog too often */ >>>>>> + now = get_timer(0); >>>>>> + if (now > next_reset) { >>>>>> + next_reset = now + 1000; /* reset every 1000ms */ >>>>>> + wdt_reset(gd->watchdog_dev); >>>>>> + } >>>>> >>>>> This is a problem for the MPC8xx. >>>>> >>>>> When running with a MPC8xx at 132MHz clock, the watchdog will fire >>>>> about 1s after the last refresh. So the above makes the board unusable. >>>> >>>> So you need a shorted delay between the wdt_reset() calls? Is this >>>> correct? We could introduce a new Kconfig option which defaults to >>>> 1000 (ms) and you can "select" a shorter value for MPC8xx. >>> >>> Exactly. However, why is this limitation needed at all ? Why is it a >>> problem to refresh more often ? >> >> Very likely its not. What is a reasonable value for your platform? 100 >> or 500ms? I think we could change it to default to a shorter value, but >> such a change should go in early in the merge window, so that other >> platforms have a bit of time to test it. >> >> Please feel free to send a patch for this and please add a comment to >> explain, why the delay is this "short". > > IMO, this should come from DT. Yes, I agree. I just wanted to keep the change "simple" here with this approach. > For a gpio watchdog (which for some > reason U-Boot doesn't have a generic driver for) the linux kernel uses a > hw_margin_ms property that tells the core how often the watchdog must be > pinged - that could be generalized to apply for all, with 1000ms as a > default if not set. And I've seen boards with a gpio watchdog with a > timeout of 200 ms. Ah, okay. Its good to know, that Linux already has such a DT property. > Also, I'm wondering why that generic _reset only handles one watchdog > device? I can easily imagine needing to reset both, say, an external > gpio-triggered one and also the SOC's/CPU's built-in one. Why not loop > over all DM watchdogs, and have the next_reset/hw_margin etc. metadata > live with the watchdog device instead of in static variable/build-time > constants? I agree, that the current WDT handling can be extended to support multiple (different) WDT instances. Currently there don't seem to be any (I might be incorrect here) board that need such a feature though. I would suggest to implement such a feature, once its really needed. Thanks, Stefan
diff --git a/arch/mips/mach-mt7620/cpu.c b/arch/mips/mach-mt7620/cpu.c index fe74f26a54..fcd0484a6d 100644 --- a/arch/mips/mach-mt7620/cpu.c +++ b/arch/mips/mach-mt7620/cpu.c @@ -69,28 +69,6 @@ int print_cpuinfo(void) return 0; } -#ifdef CONFIG_WATCHDOG -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL; - -/* Called by macro WATCHDOG_RESET */ -void watchdog_reset(void) -{ - static ulong next_reset; - ulong now; - - if (!watchdog_dev) - return; - - now = get_timer(0); - - /* Do not reset the watchdog too often */ - if (now > next_reset) { - next_reset = now + 1000; /* reset every 1000ms */ - wdt_reset(watchdog_dev); - } -} -#endif - int arch_misc_init(void) { /* @@ -103,19 +81,5 @@ int arch_misc_init(void) flush_dcache_range(gd->bd->bi_memstart, gd->bd->bi_memstart + gd->ram_size - 1); -#ifdef CONFIG_WATCHDOG - /* Init watchdog */ - if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) { - debug("Watchdog: Not found by seq!\n"); - if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) { - puts("Watchdog: Not found!\n"); - return 0; - } - } - - wdt_start(watchdog_dev, 60000, 0); /* 60 seconds */ - printf("Watchdog: Started\n"); -#endif - return 0; } diff --git a/board/CZ.NIC/turris_mox/turris_mox.c b/board/CZ.NIC/turris_mox/turris_mox.c index 96cb9c7e5c..8a4872343b 100644 --- a/board/CZ.NIC/turris_mox/turris_mox.c +++ b/board/CZ.NIC/turris_mox/turris_mox.c @@ -119,41 +119,11 @@ int board_fix_fdt(void *blob) } #endif -#ifdef CONFIG_WDT_ARMADA_37XX -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL; - -void watchdog_reset(void) -{ - static ulong next_reset; - ulong now; - - if (!watchdog_dev) - return; - - now = timer_get_us(); - - /* Do not reset the watchdog too often */ - if (now > next_reset) { - wdt_reset(watchdog_dev); - next_reset = now + 100000; - } -} -#endif - int board_init(void) { /* address of boot parameters */ gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100; -#ifdef CONFIG_WDT_ARMADA_37XX - if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) { - printf("Cannot find Armada 3720 watchdog!\n"); - } else { - printf("Enabling Armada 3720 watchdog (3 minutes timeout).\n"); - wdt_start(watchdog_dev, 180000, 0); - } -#endif - return 0; } diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c index 0287f23283..4c08f810a2 100644 --- a/board/CZ.NIC/turris_omnia/turris_omnia.c +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c @@ -364,25 +364,12 @@ static bool disable_mcu_watchdog(void) } #endif -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION) -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL; -#endif - int board_init(void) { /* adress of boot parameters */ gd->bd->bi_boot_params = mvebu_sdram_bar(0) + 0x100; #ifndef CONFIG_SPL_BUILD -# ifdef CONFIG_WDT_ORION - if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) { - puts("Cannot find Armada 385 watchdog!\n"); - } else { - puts("Enabling Armada 385 watchdog.\n"); - wdt_start(watchdog_dev, 120000, 0); - } -# endif - if (disable_mcu_watchdog()) puts("Disabled MCU startup watchdog.\n"); @@ -392,28 +379,6 @@ int board_init(void) return 0; } -#ifdef CONFIG_WATCHDOG -/* Called by macro WATCHDOG_RESET */ -void watchdog_reset(void) -{ -# if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION) - static ulong next_reset = 0; - ulong now; - - if (!watchdog_dev) - return; - - now = timer_get_us(); - - /* Do not reset the watchdog too often */ - if (now > next_reset) { - wdt_reset(watchdog_dev); - next_reset = now + 1000; - } -# endif -} -#endif - int board_late_init(void) { #ifndef CONFIG_SPL_BUILD diff --git a/board/alliedtelesis/x530/x530.c b/board/alliedtelesis/x530/x530.c index 6934fd8017..97dbed79dd 100644 --- a/board/alliedtelesis/x530/x530.c +++ b/board/alliedtelesis/x530/x530.c @@ -25,10 +25,6 @@ DECLARE_GLOBAL_DATA_PTR; #define CONFIG_NVS_LOCATION 0xf4800000 #define CONFIG_NVS_SIZE (512 << 10) -#ifdef CONFIG_WATCHDOG -static struct udevice *watchdog_dev; -#endif - static struct serdes_map board_serdes_map[] = { {PEX0, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0}, {DEFAULT_SERDES, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0}, @@ -80,10 +76,6 @@ struct mv_ddr_topology_map *mv_ddr_topology_map_get(void) int board_early_init_f(void) { -#ifdef CONFIG_WATCHDOG - watchdog_dev = NULL; -#endif - /* Configure MPP */ writel(0x00001111, MVEBU_MPP_BASE + 0x00); writel(0x00000000, MVEBU_MPP_BASE + 0x04); @@ -99,13 +91,6 @@ int board_early_init_f(void) void spl_board_init(void) { -#ifdef CONFIG_WATCHDOG - int ret; - - ret = uclass_get_device(UCLASS_WDT, 0, &watchdog_dev); - if (!ret) - wdt_start(watchdog_dev, 120000, 0); -#endif } int board_init(void) @@ -128,29 +113,10 @@ int board_init(void) void arch_preboot_os(void) { #ifdef CONFIG_WATCHDOG - wdt_stop(watchdog_dev); + wdt_stop(gd->watchdog_dev); #endif } -#ifdef CONFIG_WATCHDOG -void watchdog_reset(void) -{ - static ulong next_reset = 0; - ulong now; - - if (!watchdog_dev) - return; - - now = timer_get_us(); - - /* Do not reset the watchdog too often */ - if (now > next_reset) { - wdt_reset(watchdog_dev); - next_reset = now + 1000; - } -} -#endif - static int led_7seg_init(unsigned int segments) { int node; diff --git a/board/xilinx/microblaze-generic/microblaze-generic.c b/board/xilinx/microblaze-generic/microblaze-generic.c index 28c9efa3a2..ba82292e35 100644 --- a/board/xilinx/microblaze-generic/microblaze-generic.c +++ b/board/xilinx/microblaze-generic/microblaze-generic.c @@ -24,10 +24,6 @@ DECLARE_GLOBAL_DATA_PTR; -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT) -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL; -#endif /* !CONFIG_SPL_BUILD && CONFIG_WDT */ - ulong ram_base; int dram_init_banksize(void) @@ -43,44 +39,8 @@ int dram_init(void) return 0; }; -#ifdef CONFIG_WDT -/* Called by macro WATCHDOG_RESET */ -void watchdog_reset(void) -{ -#if !defined(CONFIG_SPL_BUILD) - ulong now; - static ulong next_reset; - - if (!watchdog_dev) - return; - - now = timer_get_us(); - - /* Do not reset the watchdog too often */ - if (now > next_reset) { - wdt_reset(watchdog_dev); - next_reset = now + 1000; - } -#endif /* !CONFIG_SPL_BUILD */ -} -#endif /* CONFIG_WDT */ - int board_late_init(void) { -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT) - watchdog_dev = NULL; - - if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) { - debug("Watchdog: Not found by seq!\n"); - if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) { - puts("Watchdog: Not found!\n"); - return 0; - } - } - - wdt_start(watchdog_dev, 0, 0); - puts("Watchdog: Started\n"); -#endif /* !CONFIG_SPL_BUILD && CONFIG_WDT */ #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SYSRESET_MICROBLAZE) int ret; diff --git a/board/xilinx/zynq/board.c b/board/xilinx/zynq/board.c index ea26aad16f..6857f2c0b8 100644 --- a/board/xilinx/zynq/board.c +++ b/board/xilinx/zynq/board.c @@ -18,10 +18,6 @@ DECLARE_GLOBAL_DATA_PTR; -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT) -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL; -#endif - #if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_BOARD_EARLY_INIT_F) int board_early_init_f(void) { @@ -31,19 +27,6 @@ int board_early_init_f(void) int board_init(void) { -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT) - if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) { - debug("Watchdog: Not found by seq!\n"); - if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) { - puts("Watchdog: Not found!\n"); - return 0; - } - } - - wdt_start(watchdog_dev, 0, 0); - puts("Watchdog: Started\n"); -# endif - return 0; } @@ -127,25 +110,3 @@ int dram_init(void) return 0; } #endif - -#if defined(CONFIG_WATCHDOG) -/* Called by macro WATCHDOG_RESET */ -void watchdog_reset(void) -{ -# if !defined(CONFIG_SPL_BUILD) - static ulong next_reset; - ulong now; - - if (!watchdog_dev) - return; - - now = timer_get_us(); - - /* Do not reset the watchdog too often */ - if (now > next_reset) { - wdt_reset(watchdog_dev); - next_reset = now + 1000; - } -# endif -} -#endif diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c index 5189925beb..c840e92d9c 100644 --- a/board/xilinx/zynqmp/zynqmp.c +++ b/board/xilinx/zynqmp/zynqmp.c @@ -24,10 +24,6 @@ DECLARE_GLOBAL_DATA_PTR; -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT) -static struct udevice *watchdog_dev __attribute__((section(".data"))) = NULL; -#endif - #if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_ZYNQMPPL) && \ !defined(CONFIG_SPL_BUILD) static xilinx_desc zynqmppl = XILINX_ZYNQMP_DESC; @@ -344,44 +340,9 @@ int board_init(void) } #endif -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT) - if (uclass_get_device_by_seq(UCLASS_WDT, 0, &watchdog_dev)) { - debug("Watchdog: Not found by seq!\n"); - if (uclass_get_device(UCLASS_WDT, 0, &watchdog_dev)) { - puts("Watchdog: Not found!\n"); - return 0; - } - } - - wdt_start(watchdog_dev, 0, 0); - puts("Watchdog: Started\n"); -#endif - return 0; } -#ifdef CONFIG_WATCHDOG -/* Called by macro WATCHDOG_RESET */ -void watchdog_reset(void) -{ -# if !defined(CONFIG_SPL_BUILD) - static ulong next_reset; - ulong now; - - if (!watchdog_dev) - return; - - now = timer_get_us(); - - /* Do not reset the watchdog too often */ - if (now > next_reset) { - wdt_reset(watchdog_dev); - next_reset = now + 1000; - } -# endif -} -#endif - int board_early_init_r(void) { u32 val; diff --git a/common/board_r.c b/common/board_r.c index 1ad44bbe3f..150e8cd424 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -48,6 +48,7 @@ #include <linux/compiler.h> #include <linux/err.h> #include <efi_loader.h> +#include <wdt.h> DECLARE_GLOBAL_DATA_PTR; @@ -677,6 +678,9 @@ static init_fnc_t init_sequence_r[] = { #ifdef CONFIG_DM initr_dm, #endif +#if defined(CONFIG_WDT) + initr_watchdog, +#endif #if defined(CONFIG_ARM) || defined(CONFIG_NDS32) || defined(CONFIG_RISCV) || \ defined(CONFIG_SANDBOX) board_init, /* Setup chipselects */ diff --git a/common/spl/spl.c b/common/spl/spl.c index 88d4b8a9bf..0a6a47c202 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -22,6 +22,7 @@ #include <linux/compiler.h> #include <fdt_support.h> #include <bootcount.h> +#include <wdt.h> DECLARE_GLOBAL_DATA_PTR; @@ -600,6 +601,10 @@ void board_init_r(gd_t *dummy1, ulong dummy2) spl_board_init(); #endif +#if defined(CONFIG_SPL_WATCHDOG_SUPPORT) && defined(CONFIG_WDT) + initr_watchdog(); +#endif + if (IS_ENABLED(CONFIG_SPL_OS_BOOT) || CONFIG_IS_ENABLED(HANDOFF)) dram_init_banksize(); diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 9d7f503b69..aa8e725573 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -51,6 +51,7 @@ config ULP_WATCHDOG config WDT bool "Enable driver model for watchdog timer drivers" depends on DM + imply WATCHDOG help Enable driver model for watchdog timer. At the moment the API is very simple and only supports four operations: diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c index 23b7e3360d..bbfac4f0f9 100644 --- a/drivers/watchdog/wdt-uclass.c +++ b/drivers/watchdog/wdt-uclass.c @@ -10,6 +10,8 @@ #include <dm/device-internal.h> #include <dm/lists.h> +DECLARE_GLOBAL_DATA_PTR; + int wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags) { const struct wdt_ops *ops = device_get_ops(dev); @@ -63,6 +65,30 @@ int wdt_expire_now(struct udevice *dev, ulong flags) return ret; } +#if defined(CONFIG_WATCHDOG) +/* + * Called by macro WATCHDOG_RESET. This function be called *very* early, + * so we need to make sure, that the watchdog driver is ready before using + * it in this function. + */ +void watchdog_reset(void) +{ + static ulong next_reset; + ulong now; + + /* Exit if GD is not ready or watchdog is not initialized yet */ + if (!gd || !(gd->flags & GD_FLG_WDT_READY)) + return; + + /* Do not reset the watchdog too often */ + now = get_timer(0); + if (now > next_reset) { + next_reset = now + 1000; /* reset every 1000ms */ + wdt_reset(gd->watchdog_dev); + } +} +#endif + static int wdt_post_bind(struct udevice *dev) { #if defined(CONFIG_NEEDS_MANUAL_RELOC) diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 78dcf40bff..d16f50f677 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -133,6 +133,9 @@ typedef struct global_data { struct spl_handoff *spl_handoff; # endif #endif +#if defined(CONFIG_WDT) + struct udevice *watchdog_dev; +#endif } gd_t; #endif @@ -161,5 +164,6 @@ typedef struct global_data { #define GD_FLG_ENV_DEFAULT 0x02000 /* Default variable flag */ #define GD_FLG_SPL_EARLY_INIT 0x04000 /* Early SPL init is done */ #define GD_FLG_LOG_READY 0x08000 /* Log system is ready for use */ +#define GD_FLG_WDT_READY 0x10000 /* Watchdog is ready for use */ #endif /* __ASM_GENERIC_GBL_DATA_H */ diff --git a/include/configs/turris_omnia.h b/include/configs/turris_omnia.h index c7805cf36b..0e65a12345 100644 --- a/include/configs/turris_omnia.h +++ b/include/configs/turris_omnia.h @@ -29,11 +29,6 @@ #define CONFIG_SPL_I2C_MUX #define CONFIG_SYS_I2C_MVTWSI -/* Watchdog support */ -#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_WDT_ORION) -# define CONFIG_WATCHDOG -#endif - /* * SDIO/MMC Card Configuration */ diff --git a/include/wdt.h b/include/wdt.h index e9a7c5355a..aa77d3e9b4 100644 --- a/include/wdt.h +++ b/include/wdt.h @@ -6,6 +6,9 @@ #ifndef _WDT_H_ #define _WDT_H_ +#include <dm.h> +#include <dm/read.h> + /* * Implement a simple watchdog uclass. Watchdog is basically a timer that * is used to detect or recover from malfunction. During normal operation @@ -103,4 +106,42 @@ struct wdt_ops { int (*expire_now)(struct udevice *dev, ulong flags); }; +#if defined(CONFIG_WDT) +#ifndef CONFIG_WATCHDOG_TIMEOUT_MSECS +#define CONFIG_WATCHDOG_TIMEOUT_MSECS (60 * 1000) +#endif +#define WATCHDOG_TIMEOUT_SECS (CONFIG_WATCHDOG_TIMEOUT_MSECS / 1000) + +static inline int initr_watchdog(void) +{ + u32 timeout = WATCHDOG_TIMEOUT_SECS; + + /* + * Init watchdog: This will call the probe function of the + * watchdog driver, enabling the use of the device + */ + if (uclass_get_device_by_seq(UCLASS_WDT, 0, + (struct udevice **)&gd->watchdog_dev)) { + debug("WDT: Not found by seq!\n"); + if (uclass_get_device(UCLASS_WDT, 0, + (struct udevice **)&gd->watchdog_dev)) { + printf("WDT: Not found!\n"); + return 0; + } + } + + if (CONFIG_IS_ENABLED(OF_CONTROL)) { + timeout = dev_read_u32_default(gd->watchdog_dev, "timeout-sec", + WATCHDOG_TIMEOUT_SECS); + } + + wdt_start(gd->watchdog_dev, timeout * 1000, 0); + gd->flags |= GD_FLG_WDT_READY; + printf("WDT: Started with%s servicing (%ds timeout)\n", + IS_ENABLED(CONFIG_WATCHDOG) ? "" : "out", timeout); + + return 0; +} +#endif + #endif /* _WDT_H_ */