[U-Boot] [PATCH 1/4] Enable CONFIG_TIMER_EARLY with bootstage

[Bin Meng]

On Tue, Oct 2, 2018 at 9:25 PM Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Thu, Sep 27, 2018 at 9:42 PM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On 25 September 2018 at 23:39, Bin Meng <bmeng.cn at gmail.com> wrote:
> > > Hi Simon,
> > >
> > > On Wed, Sep 26, 2018 at 1:42 PM Simon Glass <sjg at chromium.org> wrote:
> > >>
> > >> Hi Bin,
> > >>
> > >> On 4 September 2018 at 03:06, Bin Meng <bmeng.cn at gmail.com> wrote:
> > >> > Hi Simon,
> > >> >
> > >> > On Mon, Sep 3, 2018 at 7:02 AM Simon Glass <sjg at chromium.org> wrote:
> > >> >>
> > >> >> In initr_bootstage() we call bootstage_mark_name() which ends up calling
> > >> >> timer_get_us(). This call happens before initr_dm(), which inits driver
> > >> >> model.
> > >> >>
> > >> >> On x86 we set gd->timer to NULL in the transition from board_init_f()
> > >> >
> > >> > It's not just x86 we set gd->timer to NULL. It applied to all
> > >> > architectures when CONFIG_TIMER is on.
> > >> >
> > >> >> to board_init_r(). See board_init_f_r() for this assignment. So U-Boot
> > >> >> knows there is no timer available in the period immediately after
> > >> >> relocation.
> > >> >>
> > >> >> On x86 the timer_get_us() call is implemented as calls to get_ticks() and
> > >> >> get_tbclk(). Both of these call dm_timer_init() to set up the timer, if
> > >> >> gd->timer is NULL and the early timer is not available.
> > >> >>
> > >> >> However dm_timer_init() cannot succeed before initr_dm() is called.
> > >> >>
> > >> >> So it seems that on x86 if we want to use CONFIG_BOOTSTAGE we must enable
> > >> >> CONFIG_TIMER_EARLY. Update the Kconfig to handle this.
> > >> >>
> > >> >> Note: On most architectures we can rely on the pre-relocation memory still
> > >> >> being available, so that gd->timer pointers to a valid timer device and
> > >> >> everything works correctly. Admittedly this is not strictly correct since
> > >> >> the timer device is set up by pre-relocation U-Boot, but normally this is
> > >> >> fine. On x86 the 'CAR' (cache-as-RAM) memory used by pre-relocation U-Boot
> > >> >> disappears in board_init_f_r() and any attempt to access it will hang.
> > >> >> This is the reason why we must mark the timer as invalid when we get to
> > >> >> board_init_f_r().
> > >> >>
> > >> >> Signed-off-by: Simon Glass <sjg at chromium.org>
> > >> >> ---
> > >> >>
> > >> >>  drivers/timer/Kconfig | 3 +++
> > >> >>  1 file changed, 3 insertions(+)
> > >> >>
> > >> >> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
> > >> >> index 5ab6749193c..ff434de6f7c 100644
> > >> >> --- a/drivers/timer/Kconfig
> > >> >> +++ b/drivers/timer/Kconfig
> > >> >> @@ -30,6 +30,9 @@ config TPL_TIMER
> > >> >>  config TIMER_EARLY
> > >> >>         bool "Allow timer to be used early in U-Boot"
> > >> >>         depends on TIMER
> > >> >> +       # initr_bootstage() requires a timer and is called before initr_dm()
> > >> >> +       # so only the early timer is available
> > >> >> +       default y if X86 && BOOTSTAGE
> > >> >
> > >> > Since this applies not only on x86, and given without TIMER_EARLY
> > >> > BOOTSTAGE is broken, shouldn't we do this in BOOTSTAGE config instead:
> > >> >
> > >> > config BOOTSTAGE
> > >> >          select TIMER_EARLY
> > >>
> > >> Well we could, but I suspect that would break things since the early
> > >> timer is not supported by many boards. Also for most boards this
> > >> doesn't actually work fine. x86 is really quite awful in that it has
> > >> no SRAM and the CAR becomes inaccessible as soon as you turn on the
> > >> cache!
> > >
> > > It's true that early timer is supported by some limited boards, but
> > > that's a different issue. For now that patch does not fix anything. If
> > > we add BOOTSTAGE from either defconfig or 'menuconfig' for a board,
> > > without adding TIMER_EARLY, it will for sure break.
> >
> > Is this because of code called in board_f.c ? I don't quite follow.
>
> I re-read the codes and your comments. I think what you said below:
>
> "Note: On most architectures we can rely on the pre-relocation memory
> still being available, so that gd->timer pointers to a valid timer
> device and everything works correctly."
>
> makes sense. So adding 'select TIMER_EARLY' to BOOTSTAGE seems
> over-killing things. Let's use your approach.
>
> Reviewed-by: Bin Meng <bmeng.cn at gmail.com>

applied to u-boot-x86, thanks!