[U-Boot] [PATCH 0/4] x86: Fix up some bootstage problems

[U-Boot] [PATCH 0/4] x86: Fix up some bootstage problems

[Simon Glass]

There is a subtle problem with bootstage on x86. This series aims to fix
it so that it can be re-enabled on x86.


Simon Glass (4):
  Enable CONFIG_TIMER_EARLY with bootstage
  chromebook_samus: Increase pre-relocation memory
  binman: Add support for Intel reference code
  Revert "x86: galileo: Fix boot failure"

 configs/chromebook_samus_defconfig  |  2 +-
 configs/galileo_defconfig           |  3 +++
 drivers/timer/Kconfig               |  3 +++
 tools/binman/etype/intel_refcode.py | 27 +++++++++++++++++++++++++++
 4 files changed, 34 insertions(+), 1 deletion(-)
 create mode 100644 tools/binman/etype/intel_refcode.py

-- 
2.19.0.rc1.350.ge57e33dbd1-goog

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

[Simon Glass]

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()
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@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
 	help
 	  In some cases the timer must be accessible before driver model is
 	  active. Examples include when using CONFIG_TRACE to trace U-Boot's
-- 
2.19.0.rc1.350.ge57e33dbd1-goog

[U-Boot] [PATCH 2/4] chromebook_samus: Increase pre-relocation memory

[Simon Glass]

With bootstage now allocating pre-relocation memory the current amount
available is insufficient. Increase it a little.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 configs/chromebook_samus_defconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configs/chromebook_samus_defconfig b/configs/chromebook_samus_defconfig
index e4c51f84d9f..cbfc6f05785 100644
--- a/configs/chromebook_samus_defconfig
+++ b/configs/chromebook_samus_defconfig
@@ -1,6 +1,6 @@
 CONFIG_X86=y
 CONFIG_SYS_TEXT_BASE=0xFFE00000
-CONFIG_SYS_MALLOC_F_LEN=0x1800
+CONFIG_SYS_MALLOC_F_LEN=0x1a00
 CONFIG_DEBUG_UART_BOARD_INIT=y
 CONFIG_DEBUG_UART_BASE=0x3f8
 CONFIG_DEBUG_UART_CLOCK=1843200
-- 
2.19.0.rc1.350.ge57e33dbd1-goog

[U-Boot] [PATCH 3/4] binman: Add support for Intel reference code

[Simon Glass]

Some platforms use this instead of FSP to set up the platform, including
memory. Add support for this in binman. This is needed for
chromebook_samus, for example.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/etype/intel_refcode.py | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 tools/binman/etype/intel_refcode.py

diff --git a/tools/binman/etype/intel_refcode.py b/tools/binman/etype/intel_refcode.py
new file mode 100644
index 00000000000..045db589d17
--- /dev/null
+++ b/tools/binman/etype/intel_refcode.py
@@ -0,0 +1,27 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2016 Google, Inc
+# Written by Simon Glass <sjg@chromium.org>
+#
+# Entry-type module for Intel Memory Reference Code binary blob
+#
+
+from entry import Entry
+from blob import Entry_blob
+
+class Entry_intel_refcode(Entry_blob):
+    """Entry containing an Intel Reference Code file
+
+    Properties / Entry arguments:
+        - filename: Filename of file to read into entry
+
+    This file contains code for setting up the platform on some Intel systems.
+    This is executed by U-Boot when needed early during startup. A typical
+    filename is 'refcode.bin'.
+
+    See README.x86 for information about x86 binary blobs.
+    """
+    def __init__(self, section, etype, node):
+        Entry_blob.__init__(self, section, etype, node)
+
+    def GetDefaultFilename(self):
+        return 'refcode.bin'
-- 
2.19.0.rc1.350.ge57e33dbd1-goog

[U-Boot] [PATCH 4/4] Revert "x86: galileo: Fix boot failure"

[Simon Glass]

The root cause of this problem should now be fixed. Renable bootstage.

(Note, if this does not fix it, and instead a -ENOMEM error is produced,
then we probably need to increase CONFIG_SYS_MALLOC_F_LEN a bit).

This reverts commit 7995dd3782f90e1939969a4ead800a5e98e2d197.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 configs/galileo_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/configs/galileo_defconfig b/configs/galileo_defconfig
index 63973028a71..9c58fa3a1af 100644
--- a/configs/galileo_defconfig
+++ b/configs/galileo_defconfig
@@ -8,6 +8,8 @@ CONFIG_GENERATE_MP_TABLE=y
 CONFIG_GENERATE_ACPI_TABLE=y
 CONFIG_NR_DRAM_BANKS=8
 CONFIG_FIT=y
+CONFIG_BOOTSTAGE=y
+CONFIG_BOOTSTAGE_REPORT=y
 CONFIG_USE_BOOTARGS=y
 CONFIG_BOOTARGS="root=/dev/sdb3 init=/sbin/init rootwait ro"
 CONFIG_SYS_CONSOLE_INFO_QUIET=y
@@ -27,6 +29,7 @@ CONFIG_CMD_DHCP=y
 # CONFIG_CMD_NFS is not set
 CONFIG_CMD_PING=y
 CONFIG_CMD_TIME=y
+CONFIG_CMD_BOOTSTAGE=y
 CONFIG_CMD_EXT2=y
 CONFIG_CMD_EXT4=y
 CONFIG_CMD_EXT4_WRITE=y
-- 
2.19.0.rc1.350.ge57e33dbd1-goog

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

[Bin Meng]

Hi Simon,

On Mon, Sep 3, 2018 at 7:02 AM Simon Glass <sjg@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@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

>         help
>           In some cases the timer must be accessible before driver model is
>           active. Examples include when using CONFIG_TRACE to trace U-Boot's
> --

Regards,
Bin

[U-Boot] [PATCH 2/4] chromebook_samus: Increase pre-relocation memory

[Bin Meng]

On Mon, Sep 3, 2018 at 7:02 AM Simon Glass <sjg@chromium.org> wrote:
>
> With bootstage now allocating pre-relocation memory the current amount
> available is insufficient. Increase it a little.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  configs/chromebook_samus_defconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH 3/4] binman: Add support for Intel reference code

[Bin Meng]

On Mon, Sep 3, 2018 at 7:02 AM Simon Glass <sjg@chromium.org> wrote:
>
> Some platforms use this instead of FSP to set up the platform, including
> memory. Add support for this in binman. This is needed for
> chromebook_samus, for example.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  tools/binman/etype/intel_refcode.py | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 tools/binman/etype/intel_refcode.py
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Although I was a bit surprised that we missed this before.

[U-Boot] [PATCH 4/4] Revert "x86: galileo: Fix boot failure"

[Bin Meng]

Hi Simon,

On Mon, Sep 3, 2018 at 7:02 AM Simon Glass <sjg@chromium.org> wrote:
>
> The root cause of this problem should now be fixed. Renable bootstage.
>
> (Note, if this does not fix it, and instead a -ENOMEM error is produced,
> then we probably need to increase CONFIG_SYS_MALLOC_F_LEN a bit).
>
> This reverts commit 7995dd3782f90e1939969a4ead800a5e98e2d197.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  configs/galileo_defconfig | 3 +++
>  1 file changed, 3 insertions(+)
>

I think this breaks Galileo, though I do not have a board to test right now.

If you look at galileo.dts, it explicitly describes TSC frequency in
the DT, but current TSC timer early support codes does not support
reading TSC frequency in the DT. Maybe we need a config option for
such hardware that can't calibrate TSC frequency?

Regards,
Bin

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

[Simon Glass]

Hi Bin,

On 4 September 2018 at 03:06, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Simon,
>
> On Mon, Sep 3, 2018 at 7:02 AM Simon Glass <sjg@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@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!

Regards,
Simon

[U-Boot] [PATCH 4/4] Revert "x86: galileo: Fix boot failure"

[Simon Glass]

Hi Bin,

On 4 September 2018 at 03:07, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Simon,
>
> On Mon, Sep 3, 2018 at 7:02 AM Simon Glass <sjg@chromium.org> wrote:
>>
>> The root cause of this problem should now be fixed. Renable bootstage.
>>
>> (Note, if this does not fix it, and instead a -ENOMEM error is produced,
>> then we probably need to increase CONFIG_SYS_MALLOC_F_LEN a bit).
>>
>> This reverts commit 7995dd3782f90e1939969a4ead800a5e98e2d197.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  configs/galileo_defconfig | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>
> I think this breaks Galileo, though I do not have a board to test right now.
>
> If you look at galileo.dts, it explicitly describes TSC frequency in
> the DT, but current TSC timer early support codes does not support
> reading TSC frequency in the DT. Maybe we need a config option for
> such hardware that can't calibrate TSC frequency?

Yes we could. Until then perhaps we can park this patch.

Regards,
Simon

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

[Bin Meng]

Hi Simon,

On Wed, Sep 26, 2018 at 1:42 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On 4 September 2018 at 03:06, Bin Meng <bmeng.cn@gmail.com> wrote:
> > Hi Simon,
> >
> > On Mon, Sep 3, 2018 at 7:02 AM Simon Glass <sjg@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@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.

Regards,
Bin

[U-Boot] [PATCH 4/4] Revert "x86: galileo: Fix boot failure"

[Bin Meng]

Hi Simon,

On Wed, Sep 26, 2018 at 1:42 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On 4 September 2018 at 03:07, Bin Meng <bmeng.cn@gmail.com> wrote:
> > Hi Simon,
> >
> > On Mon, Sep 3, 2018 at 7:02 AM Simon Glass <sjg@chromium.org> wrote:
> >>
> >> The root cause of this problem should now be fixed. Renable bootstage.
> >>
> >> (Note, if this does not fix it, and instead a -ENOMEM error is produced,
> >> then we probably need to increase CONFIG_SYS_MALLOC_F_LEN a bit).
> >>
> >> This reverts commit 7995dd3782f90e1939969a4ead800a5e98e2d197.
> >>
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> >> ---
> >>
> >>  configs/galileo_defconfig | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >
> > I think this breaks Galileo, though I do not have a board to test right now.
> >
> > If you look at galileo.dts, it explicitly describes TSC frequency in
> > the DT, but current TSC timer early support codes does not support
> > reading TSC frequency in the DT. Maybe we need a config option for
> > such hardware that can't calibrate TSC frequency?
>
> Yes we could. Until then perhaps we can park this patch.

Thanks. Then I will apply patch 2 and 3 in this series.

Regards,
Bin

[U-Boot] [PATCH 2/4] chromebook_samus: Increase pre-relocation memory

[Bin Meng]

On Tue, Sep 4, 2018 at 5:06 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Mon, Sep 3, 2018 at 7:02 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > With bootstage now allocating pre-relocation memory the current amount
> > available is insufficient. Increase it a little.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  configs/chromebook_samus_defconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

applied to u-boot-x86, thanks!

[U-Boot] [PATCH 3/4] binman: Add support for Intel reference code

[Bin Meng]

On Tue, Sep 4, 2018 at 5:06 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Mon, Sep 3, 2018 at 7:02 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Some platforms use this instead of FSP to set up the platform, including
> > memory. Add support for this in binman. This is needed for
> > chromebook_samus, for example.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  tools/binman/etype/intel_refcode.py | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >  create mode 100644 tools/binman/etype/intel_refcode.py
> >
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>
> Although I was a bit surprised that we missed this before.

applied to u-boot-x86, thanks!

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

[Simon Glass]

Hi Bin,

On 25 September 2018 at 23:39, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Simon,
>
> On Wed, Sep 26, 2018 at 1:42 PM Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Bin,
>>
>> On 4 September 2018 at 03:06, Bin Meng <bmeng.cn@gmail.com> wrote:
>> > Hi Simon,
>> >
>> > On Mon, Sep 3, 2018 at 7:02 AM Simon Glass <sjg@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@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.

Regards,
Simon

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

[Bin Meng]

Hi Simon,

On Thu, Sep 27, 2018 at 9:42 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On 25 September 2018 at 23:39, Bin Meng <bmeng.cn@gmail.com> wrote:
> > Hi Simon,
> >
> > On Wed, Sep 26, 2018 at 1:42 PM Simon Glass <sjg@chromium.org> wrote:
> >>
> >> Hi Bin,
> >>
> >> On 4 September 2018 at 03:06, Bin Meng <bmeng.cn@gmail.com> wrote:
> >> > Hi Simon,
> >> >
> >> > On Mon, Sep 3, 2018 at 7:02 AM Simon Glass <sjg@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@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@gmail.com>

Regards,
Bin

[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@gmail.com> wrote:
>
> Hi Simon,
>
> On Thu, Sep 27, 2018 at 9:42 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On 25 September 2018 at 23:39, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > Hi Simon,
> > >
> > > On Wed, Sep 26, 2018 at 1:42 PM Simon Glass <sjg@chromium.org> wrote:
> > >>
> > >> Hi Bin,
> > >>
> > >> On 4 September 2018 at 03:06, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >> > Hi Simon,
> > >> >
> > >> > On Mon, Sep 3, 2018 at 7:02 AM Simon Glass <sjg@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@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@gmail.com>

applied to u-boot-x86, thanks!