[PATCH v3] CONFIG_NR_DRAM_BANKS: Remove unreferenced code as its always defined

[Daniel Schwierzeck]

Am Mittwoch, den 12.08.2020, 14:37 +0200 schrieb Pali Rohár:
> Hello!
> 
> > > > diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c
> > > > index 0a13f6edb7..b3ef15963e 100644
> > > > --- a/arch/mips/lib/bootm.c
> > > > +++ b/arch/mips/lib/bootm.c
> > > > @@ -242,7 +242,7 @@ static int boot_reloc_fdt(bootm_headers_t *images)
> > > >   #if CONFIG_IS_ENABLED(MIPS_BOOT_FDT) && CONFIG_IS_ENABLED(OF_LIBFDT)
> > > >   int arch_fixup_fdt(void *blob)
> > > >   {
> > > > -	u64 mem_start = virt_to_phys((void *)gd->bd->bi_memstart);
> > > > +	u64 mem_start = virt_to_phys((void *)gd->bd->bi_dram[0].start);
> > > >   	u64 mem_size = gd->ram_size;
> > > 
> > > I do not fully understand this change. Why on some places you are suing
> > > bi_dram[0].size and on some places gd->ram_size?
> > 
> > This patch is mainly a search and replace
> > s/bd->bi_memstart/bd->bi_dram[0].start. But as your test has shown, this is
> > not always
> > correct.
> 
> Yes, this search+replace needs to be better reviewed and tested. Such
> big change has potential to break random stuff which nobody think about.
> 
> > > > @@ -607,8 +603,11 @@ int setup_bdinfo(void)
> > > >   {
> > > >   	struct bd_info *bd = gd->bd;
> > > > -	bd->bi_memstart = gd->ram_base;  /* start of memory */
> > > > -	bd->bi_memsize = gd->ram_size;   /* size in bytes */
> > > > +	/* Only overwrite bi_dram[0] values if not already set */
> > > > +	if (!bd->bi_dram[0].size) {
> > > 
> > > This still look suspicious. When is bi_dram[0].size zero? I tried to
> > > trace source code. dram_init_banksize() is always called before
> > > setup_bdinfo() which fills bi_dram[], so when can be dram size zero?
> > > Or I'm missing something?
> > 
> > dram_init_banksize() is always called earlier, yes. But its weak default
> > only sets the bi_dram[] values when CONFIG_SYS_SDRAM_BASE is configured.
> > I'm checking right now, if this might be a problem.
> 
> Should not be in this case extended dram_init_banksize() to always
> properly fill bi_dram[] structure? Setting bi_dram[] on more places and
> doing overwriting could complicate things for future debugging or
> extending.

for all users of bi_memstart/bi_memsize the search+replace should be
enough. But in setup_bdinfo() it is wrong because it overrides the
initialization done in dram_init_banksize(). But setup_bdinfo() is a
new function due to some other bd_info refactoring merged two weeks ago
and was not available in v2 of this patch. I guess N900 was still
working with v2 and v3 now has this kind of merge conflict regression. 

Just removing those lines without replacing with bi_dram should fix
this:

bd->bi_memstart = gd->ram_base;  /* start of memory */
bd->bi_memsize = gd->ram_size;   /* size in bytes */


BTW: looking deeper in the code and history I think bd_info.bi_dram is
heavily misused as storage for DRAM configuration. Such information
should be stored in global_data where generic code can access it if
needed (e.g. LMB, fdt_fixup_memory_banks()). bd_info should just be
initialized and used when needed to boot old kernels. Also there should
be a Kconfig option to completely disable the support of bd_info.

-- 
- Daniel