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

[Stefan Roese]

Hi Bin,

On 15.08.19 16:19, Bin Meng wrote:
> Hi Stefan,
> 
> On Thu, Aug 15, 2019 at 2:07 PM Stefan Roese <sr at denx.de> wrote:
>>
>> 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?
>>
> 
> 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