| Message ID | 1457437036-22938-1-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
On Tue, Mar 08, 2016 at 08:37:16PM +0900, Masahiro Yamada wrote: > We are generally supposed to implement SoC/board-specific SPL init > code in spl_board_init(), but it is called after spl_init() where the > FDT is setup and devices are bound. > > This new stub spl_early_board_init() would be useful to put something > really SoC-specific, for example, debug_uart_init(). > > In fact, I was hit by some problems on FDT setup when I was tackling > on a completely new platform. I wished I could use the debug UART > earlier in situations like that. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> This is usually done with s_init() and uniphier opts out of that. I would conceed however that things could use some further clean-up and organization here.
Hi Masahiro, On 8 March 2016 at 04:37, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > We are generally supposed to implement SoC/board-specific SPL init > code in spl_board_init(), but it is called after spl_init() where the > FDT is setup and devices are bound. > > This new stub spl_early_board_init() would be useful to put something > really SoC-specific, for example, debug_uart_init(). > > In fact, I was hit by some problems on FDT setup when I was tackling > on a completely new platform. I wished I could use the debug UART > earlier in situations like that. > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > common/spl/spl.c | 6 ++++++ > include/spl.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/common/spl/spl.c b/common/spl/spl.c > index e5167bf..df85b09 100644 > --- a/common/spl/spl.c > +++ b/common/spl/spl.c > @@ -150,6 +150,10 @@ static int spl_ram_load_image(void) > } > #endif > > +void __weak spl_early_board_init(void) > +{ > +} > + > int spl_init(void) > { > int ret; > @@ -344,6 +348,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2) > { > int i; > > + spl_early_board_init(); Why not put this in board_init_f()? That is a little earlier. In fact you can replace that function with your own version. (although it would be better if we had a single board_init_f() and called out to board-specific code from there.) > + > debug(">>spl:board_init_r()\n"); > [snip] Regards, Simon
Hi Tom, 2016-03-09 8:23 GMT+09:00 Tom Rini <trini@konsulko.com>: > On Tue, Mar 08, 2016 at 08:37:16PM +0900, Masahiro Yamada wrote: > >> We are generally supposed to implement SoC/board-specific SPL init >> code in spl_board_init(), but it is called after spl_init() where the >> FDT is setup and devices are bound. >> >> This new stub spl_early_board_init() would be useful to put something >> really SoC-specific, for example, debug_uart_init(). >> >> In fact, I was hit by some problems on FDT setup when I was tackling >> on a completely new platform. I wished I could use the debug UART >> earlier in situations like that. >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > > This is usually done with s_init() and uniphier opts out of that. Yes, ARM32 UniPhier needs to do some tricky initialization right after the start-up, so it has its own lowlevel_init. ARM64 UniPhier is more like the standard ARM architecture, so hopefully I will be able to reuse more common code. > I > would conceed however that things could use some further clean-up and > organization here. As Simon pointed out, an alternative would be to override board_init_f(). I can live with that, too.
Hi Simon, 2016-03-09 8:33 GMT+09:00 Simon Glass <sjg@chromium.org>: > Hi Masahiro, > > On 8 March 2016 at 04:37, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: >> We are generally supposed to implement SoC/board-specific SPL init >> code in spl_board_init(), but it is called after spl_init() where the >> FDT is setup and devices are bound. >> >> This new stub spl_early_board_init() would be useful to put something >> really SoC-specific, for example, debug_uart_init(). >> >> In fact, I was hit by some problems on FDT setup when I was tackling >> on a completely new platform. I wished I could use the debug UART >> earlier in situations like that. >> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >> --- >> >> common/spl/spl.c | 6 ++++++ >> include/spl.h | 1 + >> 2 files changed, 7 insertions(+) >> >> diff --git a/common/spl/spl.c b/common/spl/spl.c >> index e5167bf..df85b09 100644 >> --- a/common/spl/spl.c >> +++ b/common/spl/spl.c >> @@ -150,6 +150,10 @@ static int spl_ram_load_image(void) >> } >> #endif >> >> +void __weak spl_early_board_init(void) >> +{ >> +} >> + >> int spl_init(void) >> { >> int ret; >> @@ -344,6 +348,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2) >> { >> int i; >> >> + spl_early_board_init(); > > Why not put this in board_init_f()? That is a little earlier. The reason is board_init_f() for SPL is not placed in the common place. We can move it there if it is OK to deal with it per arch. > In fact you can replace that function with your own version. Right. This is an alternative. The definition of board_init_f() in arch/arm/lib/spl.c is short enough. I can copy it into arch/arm/mach-uniphier/ and adjust it for my own use. > (although it would be better if we had a single board_init_f() and > called out to board-specific code from there.) > >> + >> debug(">>spl:board_init_r()\n"); >> So, what shall we do about this?
Hi Masahiro, On 9 March 2016 at 02:49, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi Simon, > > > 2016-03-09 8:33 GMT+09:00 Simon Glass <sjg@chromium.org>: >> Hi Masahiro, >> >> On 8 March 2016 at 04:37, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: >>> We are generally supposed to implement SoC/board-specific SPL init >>> code in spl_board_init(), but it is called after spl_init() where the >>> FDT is setup and devices are bound. >>> >>> This new stub spl_early_board_init() would be useful to put something >>> really SoC-specific, for example, debug_uart_init(). >>> >>> In fact, I was hit by some problems on FDT setup when I was tackling >>> on a completely new platform. I wished I could use the debug UART >>> earlier in situations like that. >>> >>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> >>> --- >>> >>> common/spl/spl.c | 6 ++++++ >>> include/spl.h | 1 + >>> 2 files changed, 7 insertions(+) >>> >>> diff --git a/common/spl/spl.c b/common/spl/spl.c >>> index e5167bf..df85b09 100644 >>> --- a/common/spl/spl.c >>> +++ b/common/spl/spl.c >>> @@ -150,6 +150,10 @@ static int spl_ram_load_image(void) >>> } >>> #endif >>> >>> +void __weak spl_early_board_init(void) >>> +{ >>> +} >>> + >>> int spl_init(void) >>> { >>> int ret; >>> @@ -344,6 +348,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2) >>> { >>> int i; >>> >>> + spl_early_board_init(); >> >> Why not put this in board_init_f()? That is a little earlier. > > The reason is board_init_f() for SPL is not placed in the common place. > > We can move it there if it is OK to deal with it per arch. > > >> In fact you can replace that function with your own version. > > Right. This is an alternative. > > The definition of board_init_f() in arch/arm/lib/spl.c is short enough. > > I can copy it into arch/arm/mach-uniphier/ and adjust it for my own use. > > >> (although it would be better if we had a single board_init_f() and >> called out to board-specific code from there.) >> >>> + >>> debug(">>spl:board_init_r()\n"); >>> > > So, what shall we do about this? Well, a refactor, I think. I have not looked at how hard it is, but it's probably the next thing to do to make SPL more similar for all boards. Regards, Simon
diff --git a/common/spl/spl.c b/common/spl/spl.c index e5167bf..df85b09 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -150,6 +150,10 @@ static int spl_ram_load_image(void) } #endif +void __weak spl_early_board_init(void) +{ +} + int spl_init(void) { int ret; @@ -344,6 +348,8 @@ void board_init_r(gd_t *dummy1, ulong dummy2) { int i; + spl_early_board_init(); + debug(">>spl:board_init_r()\n"); #if defined(CONFIG_SYS_SPL_MALLOC_START) diff --git a/include/spl.h b/include/spl.h index 92cdc04..e3c1873 100644 --- a/include/spl.h +++ b/include/spl.h @@ -93,6 +93,7 @@ int spl_load_image_ext_os(block_dev_desc_t *block_dev, int partition); */ int spl_init(void); +void spl_early_board_init(void); #ifdef CONFIG_SPL_BOARD_INIT void spl_board_init(void); #endif
We are generally supposed to implement SoC/board-specific SPL init code in spl_board_init(), but it is called after spl_init() where the FDT is setup and devices are bound. This new stub spl_early_board_init() would be useful to put something really SoC-specific, for example, debug_uart_init(). In fact, I was hit by some problems on FDT setup when I was tackling on a completely new platform. I wished I could use the debug UART earlier in situations like that. Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- common/spl/spl.c | 6 ++++++ include/spl.h | 1 + 2 files changed, 7 insertions(+)