| Message ID | 20200709080457.26850-5-ovidiu.panait@windriver.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [01/10] board_f: Introduce arch_setup_bdinfo initcall | expand |
On 09.07.20 10:04, Ovidiu Panait wrote: > Factor out mips-specific bdinfo setup from generic init sequence to > arch_setup_bdinfo in arch/mips/lib/boot.c. > > Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com> > --- > > arch/mips/lib/boot.c | 18 ++++++++++++++++++ > common/board_f.c | 25 +------------------------ > 2 files changed, 19 insertions(+), 24 deletions(-) > > diff --git a/arch/mips/lib/boot.c b/arch/mips/lib/boot.c > index db862f6379..b3a48ce10f 100644 > --- a/arch/mips/lib/boot.c > +++ b/arch/mips/lib/boot.c > @@ -9,6 +9,24 @@ > > DECLARE_GLOBAL_DATA_PTR; > > +int arch_setup_bdinfo(void) > +{ > + bd_t *bd = gd->bd; > + > + /* > + * Save local variables to board info struct > + */ > + bd->bi_memstart = CONFIG_SYS_SDRAM_BASE; /* start of memory */ > + bd->bi_memsize = gd->ram_size; /* size in bytes */ > + > +#ifdef CONFIG_SYS_SRAM_BASE We want to get rid of #ifdef where possible. So it is preferable to write: if IS_ENABLED(CONFIG_SYS_SRAM_BASE) { One benefit is that static code analysis will consider the code. Best regards Heinrich > + bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ > + bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM */ > +#endif > + > + return 0; > +} > + > unsigned long do_go_exec(ulong (*entry)(int, char * const []), > int argc, char * const argv[]) > { > diff --git a/common/board_f.c b/common/board_f.c > index 9bfcd6b236..fd7e6a17ad 100644 > --- a/common/board_f.c > +++ b/common/board_f.c > @@ -602,26 +602,6 @@ __weak int arch_setup_bdinfo(void) > return 0; > } > > -#if defined(CONFIG_MIPS) > -static int setup_board_part1(void) > -{ > - bd_t *bd = gd->bd; > - > - /* > - * Save local variables to board info struct > - */ > - bd->bi_memstart = CONFIG_SYS_SDRAM_BASE; /* start of memory */ > - bd->bi_memsize = gd->ram_size; /* size in bytes */ > - > -#ifdef CONFIG_SYS_SRAM_BASE > - bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ > - bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM */ > -#endif > - > - return 0; > -} > -#endif > - > #ifdef CONFIG_POST > static int init_post(void) > { > @@ -942,11 +922,8 @@ static const init_fnc_t init_sequence_f[] = { > reserve_stacks, > dram_init_banksize, > show_dram_config, > - arch_setup_bdinfo, > -#if defined(CONFIG_MIPS) > - setup_board_part1, > INIT_FUNC_WATCHDOG_RESET > -#endif > + arch_setup_bdinfo, > display_new_sp, > #ifdef CONFIG_OF_BOARD_FIXUP > fix_fdt, >
Hi, On 09.07.2020 12:15, Heinrich Schuchardt wrote: > On 09.07.20 10:04, Ovidiu Panait wrote: >> Factor out mips-specific bdinfo setup from generic init sequence to >> arch_setup_bdinfo in arch/mips/lib/boot.c. >> >> Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com> >> --- >> >> arch/mips/lib/boot.c | 18 ++++++++++++++++++ >> common/board_f.c | 25 +------------------------ >> 2 files changed, 19 insertions(+), 24 deletions(-) >> >> diff --git a/arch/mips/lib/boot.c b/arch/mips/lib/boot.c >> index db862f6379..b3a48ce10f 100644 >> --- a/arch/mips/lib/boot.c >> +++ b/arch/mips/lib/boot.c >> @@ -9,6 +9,24 @@ >> >> DECLARE_GLOBAL_DATA_PTR; >> >> +int arch_setup_bdinfo(void) >> +{ >> + bd_t *bd = gd->bd; >> + >> + /* >> + * Save local variables to board info struct >> + */ >> + bd->bi_memstart = CONFIG_SYS_SDRAM_BASE; /* start of memory */ >> + bd->bi_memsize = gd->ram_size; /* size in bytes */ >> + >> +#ifdef CONFIG_SYS_SRAM_BASE > We want to get rid of #ifdef where possible. So it is preferable to write: > > if IS_ENABLED(CONFIG_SYS_SRAM_BASE) { > > One benefit is that static code analysis will consider the code. > > Best regards > > Heinrich My understanding is that IS_ENABLED() only works with with boolean and tristate options. In this case, CONFIG_SYS_SRAM_BASE is a hex value: include/configs/pic32mzdask.h:22:#define CONFIG_SYS_SRAM_BASE 0x80000000 Switching to IS_ENABLED() produces the following build errors for qemu mips: $ git diff diff --git a/arch/mips/lib/boot.c b/arch/mips/lib/boot.c index b3a48ce10f..aa047335ec 100644 --- a/arch/mips/lib/boot.c +++ b/arch/mips/lib/boot.c @@ -19,10 +19,10 @@ int arch_setup_bdinfo(void) bd->bi_memstart = CONFIG_SYS_SDRAM_BASE; /* start of memory */ bd->bi_memsize = gd->ram_size; /* size in bytes */ -#ifdef CONFIG_SYS_SRAM_BASE - bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ - bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM */ -#endif + if (IS_ENABLED(CONFIG_SYS_SRAM_BASE)) { + bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ + bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM */ + } return 0; } $ make ... arch/mips/lib/boot.c: In function 'arch_setup_bdinfo': CC common/init/board_init.o arch/mips/lib/boot.c:23:22: error: 'CONFIG_SYS_SRAM_BASE' undeclared (first use in this function); did you mean 'CONFIG_SYS_SDRAM_BASE'? 23 | bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ | ^~~~~~~~~~~~~~~~~~~~ | CONFIG_SYS_SDRAM_BASE arch/mips/lib/boot.c:23:22: note: each undeclared identifier is reported only once for each function it appears in arch/mips/lib/boot.c:24:21: error: 'CONFIG_SYS_SRAM_SIZE' undeclared (first use in this function); did you mean 'CONFIG_SYS_SDRAM_BASE'? 24 | bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM */ | ^~~~~~~~~~~~~~~~~~~~ | CONFIG_SYS_SDRAM_BASE Thanks, Ovidiu >> + bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ >> + bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM */ >> +#endif >> + >> + return 0; >> +} >> + >> unsigned long do_go_exec(ulong (*entry)(int, char * const []), >> int argc, char * const argv[]) >> { >> diff --git a/common/board_f.c b/common/board_f.c >> index 9bfcd6b236..fd7e6a17ad 100644 >> --- a/common/board_f.c >> +++ b/common/board_f.c >> @@ -602,26 +602,6 @@ __weak int arch_setup_bdinfo(void) >> return 0; >> } >> >> -#if defined(CONFIG_MIPS) >> -static int setup_board_part1(void) >> -{ >> - bd_t *bd = gd->bd; >> - >> - /* >> - * Save local variables to board info struct >> - */ >> - bd->bi_memstart = CONFIG_SYS_SDRAM_BASE; /* start of memory */ >> - bd->bi_memsize = gd->ram_size; /* size in bytes */ >> - >> -#ifdef CONFIG_SYS_SRAM_BASE >> - bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ >> - bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM */ >> -#endif >> - >> - return 0; >> -} >> -#endif >> - >> #ifdef CONFIG_POST >> static int init_post(void) >> { >> @@ -942,11 +922,8 @@ static const init_fnc_t init_sequence_f[] = { >> reserve_stacks, >> dram_init_banksize, >> show_dram_config, >> - arch_setup_bdinfo, >> -#if defined(CONFIG_MIPS) >> - setup_board_part1, >> INIT_FUNC_WATCHDOG_RESET >> -#endif >> + arch_setup_bdinfo, >> display_new_sp, >> #ifdef CONFIG_OF_BOARD_FIXUP >> fix_fdt, >>
On 09.07.20 12:27, Ovidiu Panait wrote: > Hi, > > On 09.07.2020 12:15, Heinrich Schuchardt wrote: >> On 09.07.20 10:04, Ovidiu Panait wrote: >>> Factor out mips-specific bdinfo setup from generic init sequence to >>> arch_setup_bdinfo in arch/mips/lib/boot.c. >>> >>> Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com> >>> --- >>> >>> arch/mips/lib/boot.c | 18 ++++++++++++++++++ >>> common/board_f.c | 25 +------------------------ >>> 2 files changed, 19 insertions(+), 24 deletions(-) >>> >>> diff --git a/arch/mips/lib/boot.c b/arch/mips/lib/boot.c >>> index db862f6379..b3a48ce10f 100644 >>> --- a/arch/mips/lib/boot.c >>> +++ b/arch/mips/lib/boot.c >>> @@ -9,6 +9,24 @@ >>> >>> DECLARE_GLOBAL_DATA_PTR; >>> >>> +int arch_setup_bdinfo(void) >>> +{ >>> + bd_t *bd = gd->bd; >>> + >>> + /* >>> + * Save local variables to board info struct >>> + */ >>> + bd->bi_memstart = CONFIG_SYS_SDRAM_BASE; /* start of memory */ >>> + bd->bi_memsize = gd->ram_size; /* size in bytes */ >>> + >>> +#ifdef CONFIG_SYS_SRAM_BASE >> We want to get rid of #ifdef where possible. So it is preferable to >> write: >> >> if IS_ENABLED(CONFIG_SYS_SRAM_BASE) { >> >> One benefit is that static code analysis will consider the code. >> >> Best regards >> >> Heinrich > > My understanding is that IS_ENABLED() only works with with boolean and > tristate options. > > In this case, CONFIG_SYS_SRAM_BASE is a hex value: > > include/configs/pic32mzdask.h:22:#define CONFIG_SYS_SRAM_BASE > 0x80000000 > > > Switching to IS_ENABLED() produces the following build errors for qemu > mips: You could add the following helper macro to include/linux/kconfig.h #define config_defined(cfg) _config_defined(cfg, #cfg) #define _config_defined(a1, a2) !!__builtin_strcmp(#a1, a2) config_defined(cfg) evaluates to true if: * cfg is defined and * cfg is not defined as cfg. As long as optimization is switch on __builtin_strcmp() is evaluated at compile time. Best regards Heinrich > > $ git diff > diff --git a/arch/mips/lib/boot.c b/arch/mips/lib/boot.c > index b3a48ce10f..aa047335ec 100644 > --- a/arch/mips/lib/boot.c > +++ b/arch/mips/lib/boot.c > @@ -19,10 +19,10 @@ int arch_setup_bdinfo(void) > bd->bi_memstart = CONFIG_SYS_SDRAM_BASE; /* start of > memory */ > bd->bi_memsize = gd->ram_size; /* size in bytes */ > > -#ifdef CONFIG_SYS_SRAM_BASE > - bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ > - bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM */ > -#endif > + if (IS_ENABLED(CONFIG_SYS_SRAM_BASE)) { > + bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of > SRAM */ > + bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM */ > + } > > return 0; > } > > $ make > > ... > > arch/mips/lib/boot.c: In function 'arch_setup_bdinfo': > CC common/init/board_init.o > arch/mips/lib/boot.c:23:22: error: 'CONFIG_SYS_SRAM_BASE' undeclared > (first use in this function); did you mean 'CONFIG_SYS_SDRAM_BASE'? > 23 | bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ > | ^~~~~~~~~~~~~~~~~~~~ > | CONFIG_SYS_SDRAM_BASE > arch/mips/lib/boot.c:23:22: note: each undeclared identifier is reported > only once for each function it appears in > arch/mips/lib/boot.c:24:21: error: 'CONFIG_SYS_SRAM_SIZE' undeclared > (first use in this function); did you mean 'CONFIG_SYS_SDRAM_BASE'? > 24 | bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM */ > | ^~~~~~~~~~~~~~~~~~~~ > | CONFIG_SYS_SDRAM_BASE > > Thanks, > > Ovidiu > >>> + bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ >>> + bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM */ >>> +#endif >>> + >>> + return 0; >>> +} >>> + >>> unsigned long do_go_exec(ulong (*entry)(int, char * const []), >>> int argc, char * const argv[]) >>> { >>> diff --git a/common/board_f.c b/common/board_f.c >>> index 9bfcd6b236..fd7e6a17ad 100644 >>> --- a/common/board_f.c >>> +++ b/common/board_f.c >>> @@ -602,26 +602,6 @@ __weak int arch_setup_bdinfo(void) >>> return 0; >>> } >>> >>> -#if defined(CONFIG_MIPS) >>> -static int setup_board_part1(void) >>> -{ >>> - bd_t *bd = gd->bd; >>> - >>> - /* >>> - * Save local variables to board info struct >>> - */ >>> - bd->bi_memstart = CONFIG_SYS_SDRAM_BASE; /* start of memory */ >>> - bd->bi_memsize = gd->ram_size; /* size in bytes */ >>> - >>> -#ifdef CONFIG_SYS_SRAM_BASE >>> - bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ >>> - bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM */ >>> -#endif >>> - >>> - return 0; >>> -} >>> -#endif >>> - >>> #ifdef CONFIG_POST >>> static int init_post(void) >>> { >>> @@ -942,11 +922,8 @@ static const init_fnc_t init_sequence_f[] = { >>> reserve_stacks, >>> dram_init_banksize, >>> show_dram_config, >>> - arch_setup_bdinfo, >>> -#if defined(CONFIG_MIPS) >>> - setup_board_part1, >>> INIT_FUNC_WATCHDOG_RESET >>> -#endif >>> + arch_setup_bdinfo, >>> display_new_sp, >>> #ifdef CONFIG_OF_BOARD_FIXUP >>> fix_fdt, >>>
Hi, On Thu, 9 Jul 2020 at 06:28, Heinrich Schuchardt <xypron.debian@gmx.de> wrote: > > On 09.07.20 12:27, Ovidiu Panait wrote: > > Hi, > > > > On 09.07.2020 12:15, Heinrich Schuchardt wrote: > >> On 09.07.20 10:04, Ovidiu Panait wrote: > >>> Factor out mips-specific bdinfo setup from generic init sequence to > >>> arch_setup_bdinfo in arch/mips/lib/boot.c. > >>> > >>> Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com> > >>> --- > >>> > >>> arch/mips/lib/boot.c | 18 ++++++++++++++++++ > >>> common/board_f.c | 25 +------------------------ > >>> 2 files changed, 19 insertions(+), 24 deletions(-) > >>> > >>> diff --git a/arch/mips/lib/boot.c b/arch/mips/lib/boot.c > >>> index db862f6379..b3a48ce10f 100644 > >>> --- a/arch/mips/lib/boot.c > >>> +++ b/arch/mips/lib/boot.c > >>> @@ -9,6 +9,24 @@ > >>> > >>> DECLARE_GLOBAL_DATA_PTR; > >>> > >>> +int arch_setup_bdinfo(void) > >>> +{ > >>> + bd_t *bd = gd->bd; > >>> + > >>> + /* > >>> + * Save local variables to board info struct > >>> + */ > >>> + bd->bi_memstart = CONFIG_SYS_SDRAM_BASE; /* start of memory */ > >>> + bd->bi_memsize = gd->ram_size; /* size in bytes */ > >>> + > >>> +#ifdef CONFIG_SYS_SRAM_BASE > >> We want to get rid of #ifdef where possible. So it is preferable to > >> write: > >> > >> if IS_ENABLED(CONFIG_SYS_SRAM_BASE) { > >> > >> One benefit is that static code analysis will consider the code. > >> > >> Best regards > >> > >> Heinrich > > > > My understanding is that IS_ENABLED() only works with with boolean and > > tristate options. > > > > In this case, CONFIG_SYS_SRAM_BASE is a hex value: > > > > include/configs/pic32mzdask.h:22:#define CONFIG_SYS_SRAM_BASE > > 0x80000000 > > > > > > Switching to IS_ENABLED() produces the following build errors for qemu > > mips: > > You could add the following helper macro to include/linux/kconfig.h > > #define config_defined(cfg) _config_defined(cfg, #cfg) > #define _config_defined(a1, a2) !!__builtin_strcmp(#a1, a2) > > config_defined(cfg) evaluates to true if: > * cfg is defined and > * cfg is not defined as cfg. > > As long as optimization is switch on __builtin_strcmp() is evaluated at > compile time. I think we should have a new CONFIG_SYS_HAS_SRAM. Regards, Simon
diff --git a/arch/mips/lib/boot.c b/arch/mips/lib/boot.c index db862f6379..b3a48ce10f 100644 --- a/arch/mips/lib/boot.c +++ b/arch/mips/lib/boot.c @@ -9,6 +9,24 @@ DECLARE_GLOBAL_DATA_PTR; +int arch_setup_bdinfo(void) +{ + bd_t *bd = gd->bd; + + /* + * Save local variables to board info struct + */ + bd->bi_memstart = CONFIG_SYS_SDRAM_BASE; /* start of memory */ + bd->bi_memsize = gd->ram_size; /* size in bytes */ + +#ifdef CONFIG_SYS_SRAM_BASE + bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ + bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM */ +#endif + + return 0; +} + unsigned long do_go_exec(ulong (*entry)(int, char * const []), int argc, char * const argv[]) { diff --git a/common/board_f.c b/common/board_f.c index 9bfcd6b236..fd7e6a17ad 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -602,26 +602,6 @@ __weak int arch_setup_bdinfo(void) return 0; } -#if defined(CONFIG_MIPS) -static int setup_board_part1(void) -{ - bd_t *bd = gd->bd; - - /* - * Save local variables to board info struct - */ - bd->bi_memstart = CONFIG_SYS_SDRAM_BASE; /* start of memory */ - bd->bi_memsize = gd->ram_size; /* size in bytes */ - -#ifdef CONFIG_SYS_SRAM_BASE - bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ - bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM */ -#endif - - return 0; -} -#endif - #ifdef CONFIG_POST static int init_post(void) { @@ -942,11 +922,8 @@ static const init_fnc_t init_sequence_f[] = { reserve_stacks, dram_init_banksize, show_dram_config, - arch_setup_bdinfo, -#if defined(CONFIG_MIPS) - setup_board_part1, INIT_FUNC_WATCHDOG_RESET -#endif + arch_setup_bdinfo, display_new_sp, #ifdef CONFIG_OF_BOARD_FIXUP fix_fdt,
Factor out mips-specific bdinfo setup from generic init sequence to arch_setup_bdinfo in arch/mips/lib/boot.c. Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com> --- arch/mips/lib/boot.c | 18 ++++++++++++++++++ common/board_f.c | 25 +------------------------ 2 files changed, 19 insertions(+), 24 deletions(-)