| Message ID | ee5f783dffac1a94acdd7bb7f8f20cc6c4af7019.1354786138.git.vipin.kumar@st.com |
|---|---|
| State | Rejected |
| Delegated to: | Wolfgang Denk |
| Headers | show |
On 12/06/2012 10:29 AM, Vipin Kumar wrote: > A lot of ARM boards are using board_init routine just to initialize boot_params > variable in the global data structure. > > This patch lets the board config files to define a CONFIG_BOOT_PARAMS_P option > which is assigned to gd->bd->bi_boot_params automatically > > Consequently, many board_init routines would not be required in the respective > board directories and a weak definition becomes necessary before their removal > from the code. > > Signed-off-by: Vipin Kumar <vipin.kumar@st.com> > --- > README | 6 ++++++ > arch/arm/lib/board.c | 12 ++++++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/README b/README > index 037513a..2077c3b 100644 > --- a/README > +++ b/README > @@ -550,6 +550,12 @@ The following options need to be configured: > in a single configuration file and the machine type is > runtime discoverable, do not have to use this setting. > > + CONFIG_BOOT_PARAMS_P [relevant for ARM only] > + > + This config option can provide a way to initialize > + bi_boot_params from the u-boot infrastructure itself. The > + board still has the option to override it in board_init routine > + > - vxWorks boot parameters: > > bootvx constructs a valid bootline using the following > diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c > index 92cad9a..fa161b8 100644 > --- a/arch/arm/lib/board.c > +++ b/arch/arm/lib/board.c > @@ -399,6 +399,11 @@ void board_init_f(ulong bootflag) > gd->bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */ > #endif > > +#ifdef CONFIG_BOOT_PARAMS_P > + /* Boot params passed to Linux */ > + gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P; > +#endif Again an ugly #ifdef. Why not something like this instead: Define a default earlier in the code (is 0x100 the best default?): #ifndef CONFIG_BOOT_PARAMS_P #define CONFIG_BOOT_PARAMS_P 0x100 #endif then here just: /* Boot params passed to Linux */ gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P; without the #ifdef. > + > addr_sp -= sizeof (gd_t); > id = (gd_t *) addr_sp; > debug("Reserving %zu Bytes for Global Data at: %08lx\n", > @@ -468,6 +473,13 @@ void board_init_f(ulong bootflag) > static char *failed = "*** failed ***\n"; > #endif > > +static int __def_board_init(bd_t *bis) > +{ > + return -1; Is -1 a good value to return as default board_init()? > +} > + > +int board_init(void) __attribute__((weak, alias("__def_board_init"))); > + Use __weak from inlcude/linux/compiler.h instead: int __weak board_init(bd_t *bis) ... Thanks, Stefan
On 12/6/2012 3:14 PM, Stefan Roese wrote: > On 12/06/2012 10:29 AM, Vipin Kumar wrote: >> A lot of ARM boards are using board_init routine just to initialize boot_params >> variable in the global data structure. >> >> This patch lets the board config files to define a CONFIG_BOOT_PARAMS_P option >> which is assigned to gd->bd->bi_boot_params automatically >> >> Consequently, many board_init routines would not be required in the respective >> board directories and a weak definition becomes necessary before their removal >> from the code. >> >> Signed-off-by: Vipin Kumar<vipin.kumar@st.com> >> --- >> README | 6 ++++++ >> arch/arm/lib/board.c | 12 ++++++++++++ >> 2 files changed, 18 insertions(+) >> >> diff --git a/README b/README >> index 037513a..2077c3b 100644 >> --- a/README >> +++ b/README >> @@ -550,6 +550,12 @@ The following options need to be configured: >> in a single configuration file and the machine type is >> runtime discoverable, do not have to use this setting. >> >> + CONFIG_BOOT_PARAMS_P [relevant for ARM only] >> + >> + This config option can provide a way to initialize >> + bi_boot_params from the u-boot infrastructure itself. The >> + board still has the option to override it in board_init routine >> + >> - vxWorks boot parameters: >> >> bootvx constructs a valid bootline using the following >> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c >> index 92cad9a..fa161b8 100644 >> --- a/arch/arm/lib/board.c >> +++ b/arch/arm/lib/board.c >> @@ -399,6 +399,11 @@ void board_init_f(ulong bootflag) >> gd->bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */ >> #endif >> >> +#ifdef CONFIG_BOOT_PARAMS_P >> + /* Boot params passed to Linux */ >> + gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P; >> +#endif > > Again an ugly #ifdef. Why not something like this instead: > > Define a default earlier in the code (is 0x100 the best default?): > > #ifndef CONFIG_BOOT_PARAMS_P > #define CONFIG_BOOT_PARAMS_P 0x100 > #endif > > then here just: > > /* Boot params passed to Linux */ > gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P; > This would mean that I am forcing the boot params at 0x100 for all boards. Is that the right thing to do Off-course, all of them might already be initializing gd->bd->bi_boot_params on their own > without the #ifdef. > >> + >> addr_sp -= sizeof (gd_t); >> id = (gd_t *) addr_sp; >> debug("Reserving %zu Bytes for Global Data at: %08lx\n", >> @@ -468,6 +473,13 @@ void board_init_f(ulong bootflag) >> static char *failed = "*** failed ***\n"; >> #endif >> >> +static int __def_board_init(bd_t *bis) >> +{ >> + return -1; > > Is -1 a good value to return as default board_init()? > The return value is not checked as of today >> +} >> + >> +int board_init(void) __attribute__((weak, alias("__def_board_init"))); >> + > > Use __weak from inlcude/linux/compiler.h instead: > > int __weak board_init(bd_t *bis) > ... OK, I would do that in v2 Vipin > > > Thanks, > Stefan > . >
On 12/06/2012 10:56 AM, Vipin Kumar wrote: <snip> >>> +#ifdef CONFIG_BOOT_PARAMS_P >>> + /* Boot params passed to Linux */ >>> + gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P; >>> +#endif >> >> Again an ugly #ifdef. Why not something like this instead: >> >> Define a default earlier in the code (is 0x100 the best default?): >> >> #ifndef CONFIG_BOOT_PARAMS_P >> #define CONFIG_BOOT_PARAMS_P 0x100 >> #endif >> >> then here just: >> >> /* Boot params passed to Linux */ >> gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P; >> > > This would mean that I am forcing the boot params at 0x100 for all > boards. Is that the right thing to do > > Off-course, all of them might already be initializing > gd->bd->bi_boot_params on their own Yes. Before or after this initialization? Will the board_init code overwrite this one here? But I also think this could be confusing. The generic way to implement this would be to remove *all* local board specific assignments of gd->bd->bi_boot_params and define CONFIG_BOOT_PARAMS_P in their board config header. But this will be quite intrusive and could be error-prone. >> without the #ifdef. >> >>> + >>> addr_sp -= sizeof (gd_t); >>> id = (gd_t *) addr_sp; >>> debug("Reserving %zu Bytes for Global Data at: %08lx\n", >>> @@ -468,6 +473,13 @@ void board_init_f(ulong bootflag) >>> static char *failed = "*** failed ***\n"; >>> #endif >>> >>> +static int __def_board_init(bd_t *bis) >>> +{ >>> + return -1; >> >> Is -1 a good value to return as default board_init()? >> > > The return value is not checked as of today Then I suggest to use 0. Thanks, Stefan
On 12/6/2012 4:56 PM, Stefan Roese wrote: > On 12/06/2012 10:56 AM, Vipin Kumar wrote: > > <snip> > >>>> +#ifdef CONFIG_BOOT_PARAMS_P >>>> + /* Boot params passed to Linux */ >>>> + gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P; >>>> +#endif >>> >>> Again an ugly #ifdef. Why not something like this instead: >>> >>> Define a default earlier in the code (is 0x100 the best default?): >>> >>> #ifndef CONFIG_BOOT_PARAMS_P >>> #define CONFIG_BOOT_PARAMS_P 0x100 >>> #endif >>> >>> then here just: >>> >>> /* Boot params passed to Linux */ >>> gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P; >>> >> >> This would mean that I am forcing the boot params at 0x100 for all >> boards. Is that the right thing to do >> >> Off-course, all of them might already be initializing >> gd->bd->bi_boot_params on their own > > Yes. Before or after this initialization? Will the board_init code > overwrite this one here? > board_init would overwrite it as it is called after relocation from board_init_r while this routine is called even before relocation > But I also think this could be confusing. The generic way to implement > this would be to remove *all* local board specific assignments of > gd->bd->bi_boot_params and define CONFIG_BOOT_PARAMS_P in their board > config header. But this will be quite intrusive and could be error-prone. > Yes, that is what I also thought.. Originally, I based my patch on the CONFIG_MACH_TYPE handling. If it is defined the board need not initialize gd->bd->bi_arch_number Can you please suggest what is the best way here. Wolfgang, Albert ? >>> without the #ifdef. >>> >>>> + >>>> addr_sp -= sizeof (gd_t); >>>> id = (gd_t *) addr_sp; >>>> debug("Reserving %zu Bytes for Global Data at: %08lx\n", >>>> @@ -468,6 +473,13 @@ void board_init_f(ulong bootflag) >>>> static char *failed = "*** failed ***\n"; >>>> #endif >>>> >>>> +static int __def_board_init(bd_t *bis) >>>> +{ >>>> + return -1; >>> >>> Is -1 a good value to return as default board_init()? >>> >> >> The return value is not checked as of today > > Then I suggest to use 0. > OK. Accepted > Thanks, > Stefan > > . >
Dear Vipin Kumar, In message <50C1B9A6.1020106@st.com> you wrote: > > Can you please suggest what is the best way here. Wolfgang, Albert ? I don't like this patch at all. It introduces yet more architecture specific stuff to lib/board.c, while we actually should be working on coming up with a common version for all architectures. If you want to simplify code, then please not by making it worse in other places. Best regards, Wolfgang Denk
On 12/7/2012 8:17 PM, Wolfgang Denk wrote: > Dear Vipin Kumar, > > In message<50C1B9A6.1020106@st.com> you wrote: >> >> Can you please suggest what is the best way here. Wolfgang, Albert ? > > I don't like this patch at all. It introduces yet more > architecture specific stuff to lib/board.c, while we actually should > be working on coming up with a common version for all architectures. > > If you want to simplify code, then please not by making it worse in > other places. > I agree, but this code adds an option to pass boot params pointer to board descriptor. Just two lines above the intended change, there is a mach_type initialization based on the definition of CONFIG_MACH_TYPE. Moreover, the lib/board.c exists in arch/arm as of today which means that it can contain all the stuff common for arm boards If I am right, the patch should also be OK Vipin > Best regards, > > Wolfgang Denk >
diff --git a/README b/README index 037513a..2077c3b 100644 --- a/README +++ b/README @@ -550,6 +550,12 @@ The following options need to be configured: in a single configuration file and the machine type is runtime discoverable, do not have to use this setting. + CONFIG_BOOT_PARAMS_P [relevant for ARM only] + + This config option can provide a way to initialize + bi_boot_params from the u-boot infrastructure itself. The + board still has the option to override it in board_init routine + - vxWorks boot parameters: bootvx constructs a valid bootline using the following diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c index 92cad9a..fa161b8 100644 --- a/arch/arm/lib/board.c +++ b/arch/arm/lib/board.c @@ -399,6 +399,11 @@ void board_init_f(ulong bootflag) gd->bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */ #endif +#ifdef CONFIG_BOOT_PARAMS_P + /* Boot params passed to Linux */ + gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P; +#endif + addr_sp -= sizeof (gd_t); id = (gd_t *) addr_sp; debug("Reserving %zu Bytes for Global Data at: %08lx\n", @@ -468,6 +473,13 @@ void board_init_f(ulong bootflag) static char *failed = "*** failed ***\n"; #endif +static int __def_board_init(bd_t *bis) +{ + return -1; +} + +int board_init(void) __attribute__((weak, alias("__def_board_init"))); + /* ************************************************************************ *
A lot of ARM boards are using board_init routine just to initialize boot_params variable in the global data structure. This patch lets the board config files to define a CONFIG_BOOT_PARAMS_P option which is assigned to gd->bd->bi_boot_params automatically Consequently, many board_init routines would not be required in the respective board directories and a weak definition becomes necessary before their removal from the code. Signed-off-by: Vipin Kumar <vipin.kumar@st.com> --- README | 6 ++++++ arch/arm/lib/board.c | 12 ++++++++++++ 2 files changed, 18 insertions(+)