Re: [U-Boot] [PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version
Hi Stefan,
On Fri, Aug 16, 2019 at 1:11 PM Stefan Roese 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 wrote:
> >>
> >> Hi Simon,
> >>
> >> On 14.08.19 21:35, Simon Glass wrote:
> >>> Hi,
> >>>
> >>> On Wed, 14 Aug 2019 at 00:22, Stefan Roese 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
> >
> >
> >
> >> --- 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_DEFAULT0x02000 /* 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_READY0x1 /* 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 0x1
> /* Cold Boot */
> arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x2
> /* 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
Re: [U-Boot] [PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version
Hi Bin,
On 15.08.19 16:19, Bin Meng wrote:
Hi Stefan,
On Thu, Aug 15, 2019 at 2:07 PM Stefan Roese wrote:
Hi Simon,
On 14.08.19 21:35, Simon Glass wrote:
Hi,
On Wed, 14 Aug 2019 at 00:22, Stefan Roese 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
--- 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_DEFAULT0x02000 /* 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_READY0x1 /* 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 0x1 /* Cold
Boot */
arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x2 /* 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version
Hi Stefan,
On Thu, Aug 15, 2019 at 2:07 PM Stefan Roese wrote:
>
> Hi Simon,
>
> On 14.08.19 21:35, Simon Glass wrote:
> > Hi,
> >
> > On Wed, 14 Aug 2019 at 00:22, Stefan Roese 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
> >>>
> >>>
> >>>
> --- 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_DEFAULT0x02000 /* 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_READY0x1 /* 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 0x1 /*
> >> Cold Boot */
> >> arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x2 /*
> >> 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 solut
Re: [U-Boot] [PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version
Hi Simon,
On 14.08.19 21:35, Simon Glass wrote:
Hi,
On Wed, 14 Aug 2019 at 00:22, Stefan Roese 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
--- 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_DEFAULT0x02000 /* 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_READY0x1 /* 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 0x1 /* Cold
Boot */
arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x2 /* 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version
Hi,
On Wed, 14 Aug 2019 at 00:22, Stefan Roese 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
> >
> >
> >
> >> --- 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_DEFAULT0x02000 /* 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_READY0x1 /* 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 0x1 /*
> Cold Boot */
> arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x2 /*
> 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version
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
--- 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_INIT0x04000 /* Early SPL init is done
*/
#define GD_FLG_LOG_READY 0x08000 /* Log system is ready for use */
+#define GD_FLG_WDT_READY 0x1 /* 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 0x1 /* Cold
Boot */
arch/x86/include/asm/global_data.h:#define GD_FLG_WARM_BOOT 0x2 /* 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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version
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
--- 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_DEFAULT0x02000 /* 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 0x1 /* 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
+#include
+
/*
* 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_ */
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version
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 Cc: Heiko Schocher Cc: Tom Rini Cc: Michal Simek Cc: "Marek Behún" Cc: Daniel Schwierzeck Cc: Maxim Sloyko Cc: Erik van Luijk Cc: Ryder Lee Cc: Weijie Gao Cc: Simon Glass Cc: "Álvaro Fernández Rojas" Cc: Philippe Reynes Cc: Christophe Leroy Cc: Chris Packham Reviewed-by: Michal Simek Tested-by: Michal Simek (on zcu100) Applied to u-boot-marvell/master. Thanks, Stefan ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
