Hi Ovidiu,
On Wed, 15 Jul 2020 at 08:07, Ovidiu Panait <ovidiu.pan...@windriver.com> wrote:
>
> Hi Simon,
>
> On 15.07.2020 04:05, Simon Glass wrote:
> > On Fri, 10 Jul 2020 at 04:25, Ovidiu Panait <ovidiu.pan...@windriver.com>
> > wrote:
> >> Factor out m68k-specific bdinfo setup to arch_setup_bdinfo in
> >> arch/m68k/lib/bdinfo.c. Also, use if(IS_ENABLED()) instead of #ifdef where
> >> possible.
> >>
> >> Signed-off-by: Ovidiu Panait <ovidiu.pan...@windriver.com>
> >> ---
> >> v2 updates:
> >> - use "if (IS_ENABLED(CONFIG_SYS_HAS_SRAM))" instead of
> >> "#ifdef CONFIG_SYS_SRAM_BASE"
> >>
> >> arch/m68k/lib/bdinfo.c | 32 ++++++++++++++++++++++++++++++++
> >> common/board_f.c | 14 ++++----------
> >> 2 files changed, 36 insertions(+), 10 deletions(-)
> > Reviewed-by: Simon Glass <s...@chromium.org>
> >
> > See below
> >
> >> diff --git a/arch/m68k/lib/bdinfo.c b/arch/m68k/lib/bdinfo.c
> >> index 971c47c306..3257195add 100644
> >> --- a/arch/m68k/lib/bdinfo.c
> >> +++ b/arch/m68k/lib/bdinfo.c
> >> @@ -11,6 +11,38 @@
> >>
> >> 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 */
> >> +
> >> + if (IS_ENABLED(CONFIG_SYS_HAS_SRAM)) {
> >> + bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM
> >> */
> >> + bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM
> >> */
> >> + }
> > I wonder if we can do better - can't we move the SDRAM code back to
> > the generic bdinfo.c ?
>
> I am not sure that I understand, could you please explain it further?
>
> When talking about the generic bdinfo.c, you are referring to
> cmd/bdinfo.c? I see that cmd/bdinfo.c currently only takes care of
> printing various bd->bi_* fields, not assigning them.
>
> Where exactly should I move the "if (IS_ENABLED(CONFIG_SYS_HAS_SRAM))
> {...}" block?
>
>
> Do you mean to create a generic setup_bdinfo that populates generic
> bd->bi_* fields and at the end calls the arch-specific variant
> arch_setup_bdinfo?
Er yes, that's what I meant! You will go far with your psychic abilities.
Regards,
Simon
