[PATCH v2 06/14] board_f: m68k: Factor out m68k-specific bdinfo setup

[Simon Glass]

Hi Ovidiu,

On Wed, 15 Jul 2020 at 08:07, Ovidiu Panait <ovidiu.panait at 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.panait at 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.panait at 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 <sjg at 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