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

[Stefan Roese]

On 12.08.20 16:21, Daniel Schwierzeck wrote:
> 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 */

Yes. A new patchset will follow soon, which removes these assignments
in setup_bdinfo() but also adds a default assignment to
dram_init_banksize().

> 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()).

I agree. Some of this is addressed in the upcoming patchset.

> 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.

I agree in general, but:

If we want to support booting of the old kernels (at least on PowerPC),
then we can't remove bi_memstart/memsize from bd_info. This is a
decision that we need to make. Currently my patches remove it and
therefore at least booting of old PowerPC kernels using bd_info to
transfer the memory infos to the kernel will not work any more. This
could easily be changed by not removing those values from the struct
and adding one assignment of them to some common place. I'm not sure
if its worth it though. And I'm not even sure, if I can test this
somehow, as I would need a PowerPC board that is supported in U-Boot &
Linux for a very long time.

Thanks,
Stefan