Hi Stefan, On Fri, Aug 16, 2019 at 1:11 PM Stefan Roese <s...@denx.de> wrote: > > Hi Bin, > > On 15.08.19 16:19, Bin Meng wrote: > > Hi Stefan, > > > > On Thu, Aug 15, 2019 at 2:07 PM Stefan Roese <s...@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 <s...@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 <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: > >>> > >>> 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?
GD_FLG_COLD_BOOT is only set in the 16-bit start code while on coreboot it boots directly from the 32-bit start code. Regards, Bin _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot