diff mbox series

[05/10] board_f: mips: Factor out mips-specific bdinfo setup

Message ID acf6d2fc-5390-d808-1f06-0e87dc091c29@windriver.com
State New
Headers show
Series None | expand

Commit Message

Ovidiu Panait July 9, 2020, 10:27 a.m. UTC 
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 at 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
 ?}

$ 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,
>>

Comments

Heinrich Schuchardt July 9, 2020, 12:27 p.m. UTC | #1 
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 at 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,
>>>
Simon Glass July 10, 2020, 12:35 a.m. UTC | #2 
Hi,

On Thu, 9 Jul 2020 at 06:28, Heinrich Schuchardt <xypron.debian at 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 at 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 mbox series

Patch

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;