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

[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 me it looks like that this patch is mixing different structures for
> > ram size from different sources.
> 
> Again, it started as a "simple" search and replace. It seems, that I
> need to change this patch, so that the "correct" value is used in
> place of bi_memstart & bi_memsize instead of always using bi_dram[].
> 
> I'll rework this patch again.
> 
> > It is really correct? Does not it break
> > other boards?
> > 
> > I think that there is missing explanation of these changes.
> 
> Perhaps its also the patch history that you are missing. This patch
> started with a simple remove of the unreferenced CONFIG_NR_DRAM_BANKS
> code paths (as the patch subject still mentions). And its extended,
> after Daniel pointed out, that with the move of CONFIG_NR_DRAM_BANKS
> the usage of bi_memstart etc is now not 100% correct any more. He
> suggested to replace those references with bi_dram[].

I have not seen patch history, only its current version which caused
u-boot failure on n900. So I was not sure what is intension of this
patch. Sorry for that.

I think 1:1 replacement is not such trivial as with more dram banks you
would need to join address space from bi_dram[0], bi_dram[1], ... as
previous code is expecting.

I would suggest that more people with different hardware affected by
this change would test if everything is working fine.

I do not know how many boards have full boot test scenarios (from
booting u-boot to booting kernel) like N900 and such tests are useful
when playing with patches which changes such big parts.

I would suggest to other board maintainers to write or provides test
suite as it can really help with catching such fatal errors.

I cannot believe that N900 was the only device on which u-boot stopped
booting kernel. From that code it looks like that all OMAP3 devices must
have been affected and possible any architecture with two or more dram
banks.

> I agree that this is a bit confusing. I'll rework this patch into
> multiple separate patches now instead. This will hopefully make it
> easier to follow the intention and review these changes.

Lokesh, do you have OMAP3 devices to test these future Stefan's patches?
For me it looks like that OMAP3 is good candidate which can be affected
by these changes if code is not properly reviewed.