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

[Stefan Roese]

Hi Simon,

On 14.08.19 21:35, Simon Glass wrote:
> 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.

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