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 <s...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to