[U-Boot] [PATCH 1/4 v5] watchdog: Implement generic watchdog_reset() version

[Simon Glass]

Hi,

On Wed, 14 Aug 2019 at 00:22, Stefan Roese <sr at 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 at 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