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

[Stefan Roese]

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:

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