| Message ID | 20200710101907.11214-6-ovidiu.panait@windriver.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v2,01/14] Kconfig: Introduce CONFIG_SYS_HAS_SRAM | expand |
On Fri, 10 Jul 2020 at 04:25, Ovidiu Panait <ovidiu.panait@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@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@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 ? > + > + bd->bi_mbar_base = CONFIG_SYS_MBAR; /* base of internal registers */ > + > + bd->bi_intfreq = gd->cpu_clk; /* Internal Freq, in Hz */ > + bd->bi_busfreq = gd->bus_clk; /* Bus Freq, in Hz */ > + > + if (IS_ENABLED(CONFIG_PCI)) > + bd->bi_pcifreq = gd->pci_clk; > + > +#if defined(CONFIG_EXTRA_CLOCK) > + bd->bi_inpfreq = gd->arch.inp_clk; /* input Freq in Hz */ > + bd->bi_vcofreq = gd->arch.vco_clk; /* vco Freq in Hz */ > + bd->bi_flbfreq = gd->arch.flb_clk; /* flexbus Freq in Hz */ > +#endif > + > + return 0; > +} > + > void arch_print_bdinfo(void) > { > bd_t *bd = gd->bd; > diff --git a/common/board_f.c b/common/board_f.c > index e597749d2f..7d65879b93 100644 > --- a/common/board_f.c > +++ b/common/board_f.c > @@ -602,7 +602,7 @@ __weak int arch_setup_bdinfo(void) > return 0; > } > > -#if defined(CONFIG_M68K) || defined(CONFIG_MIPS) || defined(CONFIG_PPC) || \ > +#if defined(CONFIG_MIPS) || defined(CONFIG_PPC) || \ > defined(CONFIG_SH) > static int setup_board_part1(void) > { > @@ -622,9 +622,6 @@ static int setup_board_part1(void) > #if defined(CONFIG_E500) || defined(CONFIG_MPC86xx) > bd->bi_immr_base = CONFIG_SYS_IMMR; /* base of IMMR register */ > #endif > -#if defined(CONFIG_M68K) > - bd->bi_mbar_base = CONFIG_SYS_MBAR; /* base of internal registers */ > -#endif > #if defined(CONFIG_MPC83xx) > bd->bi_immrbar = CONFIG_SYS_IMMR; > #endif > @@ -633,7 +630,7 @@ static int setup_board_part1(void) > } > #endif > > -#if defined(CONFIG_PPC) || defined(CONFIG_M68K) > +#if defined(CONFIG_PPC) > static int setup_board_part2(void) > { > bd_t *bd = gd->bd; > @@ -646,9 +643,6 @@ static int setup_board_part2(void) > bd->bi_sccfreq = gd->arch.scc_clk; > bd->bi_vco = gd->arch.vco_out; > #endif /* CONFIG_CPM2 */ > -#if defined(CONFIG_M68K) && defined(CONFIG_PCI) > - bd->bi_pcifreq = gd->pci_clk; > -#endif > #if defined(CONFIG_EXTRA_CLOCK) > bd->bi_inpfreq = gd->arch.inp_clk; /* input Freq in Hz */ > bd->bi_vcofreq = gd->arch.vco_clk; /* vco Freq in Hz */ > @@ -980,11 +974,11 @@ static const init_fnc_t init_sequence_f[] = { > dram_init_banksize, > show_dram_config, > arch_setup_bdinfo, > -#if defined(CONFIG_M68K) || defined(CONFIG_MIPS) || defined(CONFIG_PPC) || \ > +#if defined(CONFIG_MIPS) || defined(CONFIG_PPC) || \ > defined(CONFIG_SH) > setup_board_part1, > #endif > -#if defined(CONFIG_PPC) || defined(CONFIG_M68K) > +#if defined(CONFIG_PPC) > INIT_FUNC_WATCHDOG_RESET > setup_board_part2, > #endif > -- > 2.17.1 >
Hi Simon, On 15.07.2020 04:05, Simon Glass wrote: > On Fri, 10 Jul 2020 at 04:25, Ovidiu Panait <ovidiu.panait@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@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@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? Ovidiu >> + >> + bd->bi_mbar_base = CONFIG_SYS_MBAR; /* base of internal registers */ >> + >> + bd->bi_intfreq = gd->cpu_clk; /* Internal Freq, in Hz */ >> + bd->bi_busfreq = gd->bus_clk; /* Bus Freq, in Hz */ >> + >> + if (IS_ENABLED(CONFIG_PCI)) >> + bd->bi_pcifreq = gd->pci_clk; >> + >> +#if defined(CONFIG_EXTRA_CLOCK) >> + bd->bi_inpfreq = gd->arch.inp_clk; /* input Freq in Hz */ >> + bd->bi_vcofreq = gd->arch.vco_clk; /* vco Freq in Hz */ >> + bd->bi_flbfreq = gd->arch.flb_clk; /* flexbus Freq in Hz */ >> +#endif >> + >> + return 0; >> +} >> + >> void arch_print_bdinfo(void) >> { >> bd_t *bd = gd->bd; >> diff --git a/common/board_f.c b/common/board_f.c >> index e597749d2f..7d65879b93 100644 >> --- a/common/board_f.c >> +++ b/common/board_f.c >> @@ -602,7 +602,7 @@ __weak int arch_setup_bdinfo(void) >> return 0; >> } >> >> -#if defined(CONFIG_M68K) || defined(CONFIG_MIPS) || defined(CONFIG_PPC) || \ >> +#if defined(CONFIG_MIPS) || defined(CONFIG_PPC) || \ >> defined(CONFIG_SH) >> static int setup_board_part1(void) >> { >> @@ -622,9 +622,6 @@ static int setup_board_part1(void) >> #if defined(CONFIG_E500) || defined(CONFIG_MPC86xx) >> bd->bi_immr_base = CONFIG_SYS_IMMR; /* base of IMMR register */ >> #endif >> -#if defined(CONFIG_M68K) >> - bd->bi_mbar_base = CONFIG_SYS_MBAR; /* base of internal registers */ >> -#endif >> #if defined(CONFIG_MPC83xx) >> bd->bi_immrbar = CONFIG_SYS_IMMR; >> #endif >> @@ -633,7 +630,7 @@ static int setup_board_part1(void) >> } >> #endif >> >> -#if defined(CONFIG_PPC) || defined(CONFIG_M68K) >> +#if defined(CONFIG_PPC) >> static int setup_board_part2(void) >> { >> bd_t *bd = gd->bd; >> @@ -646,9 +643,6 @@ static int setup_board_part2(void) >> bd->bi_sccfreq = gd->arch.scc_clk; >> bd->bi_vco = gd->arch.vco_out; >> #endif /* CONFIG_CPM2 */ >> -#if defined(CONFIG_M68K) && defined(CONFIG_PCI) >> - bd->bi_pcifreq = gd->pci_clk; >> -#endif >> #if defined(CONFIG_EXTRA_CLOCK) >> bd->bi_inpfreq = gd->arch.inp_clk; /* input Freq in Hz */ >> bd->bi_vcofreq = gd->arch.vco_clk; /* vco Freq in Hz */ >> @@ -980,11 +974,11 @@ static const init_fnc_t init_sequence_f[] = { >> dram_init_banksize, >> show_dram_config, >> arch_setup_bdinfo, >> -#if defined(CONFIG_M68K) || defined(CONFIG_MIPS) || defined(CONFIG_PPC) || \ >> +#if defined(CONFIG_MIPS) || defined(CONFIG_PPC) || \ >> defined(CONFIG_SH) >> setup_board_part1, >> #endif >> -#if defined(CONFIG_PPC) || defined(CONFIG_M68K) >> +#if defined(CONFIG_PPC) >> INIT_FUNC_WATCHDOG_RESET >> setup_board_part2, >> #endif >> -- >> 2.17.1 >>
Hi Ovidiu, On Wed, 15 Jul 2020 at 08:07, Ovidiu Panait <ovidiu.panait@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@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@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@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
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 */ + } + + bd->bi_mbar_base = CONFIG_SYS_MBAR; /* base of internal registers */ + + bd->bi_intfreq = gd->cpu_clk; /* Internal Freq, in Hz */ + bd->bi_busfreq = gd->bus_clk; /* Bus Freq, in Hz */ + + if (IS_ENABLED(CONFIG_PCI)) + bd->bi_pcifreq = gd->pci_clk; + +#if defined(CONFIG_EXTRA_CLOCK) + bd->bi_inpfreq = gd->arch.inp_clk; /* input Freq in Hz */ + bd->bi_vcofreq = gd->arch.vco_clk; /* vco Freq in Hz */ + bd->bi_flbfreq = gd->arch.flb_clk; /* flexbus Freq in Hz */ +#endif + + return 0; +} + void arch_print_bdinfo(void) { bd_t *bd = gd->bd; diff --git a/common/board_f.c b/common/board_f.c index e597749d2f..7d65879b93 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -602,7 +602,7 @@ __weak int arch_setup_bdinfo(void) return 0; } -#if defined(CONFIG_M68K) || defined(CONFIG_MIPS) || defined(CONFIG_PPC) || \ +#if defined(CONFIG_MIPS) || defined(CONFIG_PPC) || \ defined(CONFIG_SH) static int setup_board_part1(void) { @@ -622,9 +622,6 @@ static int setup_board_part1(void) #if defined(CONFIG_E500) || defined(CONFIG_MPC86xx) bd->bi_immr_base = CONFIG_SYS_IMMR; /* base of IMMR register */ #endif -#if defined(CONFIG_M68K) - bd->bi_mbar_base = CONFIG_SYS_MBAR; /* base of internal registers */ -#endif #if defined(CONFIG_MPC83xx) bd->bi_immrbar = CONFIG_SYS_IMMR; #endif @@ -633,7 +630,7 @@ static int setup_board_part1(void) } #endif -#if defined(CONFIG_PPC) || defined(CONFIG_M68K) +#if defined(CONFIG_PPC) static int setup_board_part2(void) { bd_t *bd = gd->bd; @@ -646,9 +643,6 @@ static int setup_board_part2(void) bd->bi_sccfreq = gd->arch.scc_clk; bd->bi_vco = gd->arch.vco_out; #endif /* CONFIG_CPM2 */ -#if defined(CONFIG_M68K) && defined(CONFIG_PCI) - bd->bi_pcifreq = gd->pci_clk; -#endif #if defined(CONFIG_EXTRA_CLOCK) bd->bi_inpfreq = gd->arch.inp_clk; /* input Freq in Hz */ bd->bi_vcofreq = gd->arch.vco_clk; /* vco Freq in Hz */ @@ -980,11 +974,11 @@ static const init_fnc_t init_sequence_f[] = { dram_init_banksize, show_dram_config, arch_setup_bdinfo, -#if defined(CONFIG_M68K) || defined(CONFIG_MIPS) || defined(CONFIG_PPC) || \ +#if defined(CONFIG_MIPS) || defined(CONFIG_PPC) || \ defined(CONFIG_SH) setup_board_part1, #endif -#if defined(CONFIG_PPC) || defined(CONFIG_M68K) +#if defined(CONFIG_PPC) INIT_FUNC_WATCHDOG_RESET setup_board_part2, #endif
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@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(-)