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

[Stefan Roese]

Hi Pali,

On 11.08.20 18:37, Pali Rohár wrote:
> On Tuesday 11 August 2020 10:22:44 Simon Glass wrote:
>> Hi Daniel,
>>
>> On Tue, 11 Aug 2020 at 10:10, Daniel Schwierzeck
>> <daniel.schwierzeck at gmail.com> wrote:
>>>
>>> Am Dienstag, den 11.08.2020, 10:58 -0400 schrieb Tom Rini:
>>>> On Tue, Aug 11, 2020 at 03:22:48PM +0200, Pali Rohár wrote:
>>>>> On Tuesday 11 August 2020 09:08:33 Tom Rini wrote:
>>>>>> On Tue, Aug 11, 2020 at 02:39:53PM +0200, Stefan Roese wrote:
>>>>>>> Hi Pali,
>>>>>>>
>>>>>>> On 11.08.20 10:00, Pali Rohár wrote:
>>>>>>>> Hello!
>>>>>>>>
>>>>>>>> On Tuesday 11 August 2020 08:37:46 Stefan Roese wrote:
>>>>>>>>> Hi Tom,
>>>>>>>>> Hi Pali,
>>>>>>>>>
>>>>>>>>> (added Pali because of the Nokia RX51 issue)
>>>>>>>>
>>>>>>>> Could you please send me a link to "problematic" patch? As you have not
>>>>>>>> inlined it in this email.
>>>>>>>
>>>>>>> https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6
>>>>>>>
>>>>>>> Its a rebase of the original patch.
>>>>>>>
>>>>>>>>> On 07.08.20 21:21, Tom Rini wrote:
>>>>>>>>>> On Thu, Jul 30, 2020 at 12:51:10PM +0200, Stefan Roese wrote:
>>>>>>>>>>
>>>>>>>>>>> Since commit 86cf1c82850f ("configs: Migrate CONFIG_NR_DRAM_BANKS") &
>>>>>>>>>>> commit 999a772d9f24 ("Kconfig: Migrate CONFIG_NR_DRAM_BANKS"),
>>>>>>>>>>> CONFIG_NR_DRAM_BANKS is always defined with a value (4 is default).
>>>>>>>>>>> It makes no sense to still carry code that is guarded with
>>>>>>>>>>> "#ifndef CONFIG_NR_DRAM_BANKS" (and similar). This patch removes
>>>>>>>>>>> all these unreferenced code paths.
>>>>>>>>>>>
>>>>>>>>>>> Also complete remove bi_memstart & bi_memsize from the board-info
>>>>>>>>>>> struct. As now bi_dram[] is always enabled and should be used instead.
>>>>>>>>>>> This removes the redundant varriables.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>>>>>>>>>> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
>>>>>>>>>>> Cc: Tom Rini <trini at konsulko.com>
>>>>>>>>>>> Cc: Ramon Fried <ramon.fried at gmail.com>
>>>>>>>>>>> Cc: Simon Glass <sjg at chromium.org>
>>>>>>>>>>> Cc: Michal Simek <michal.simek at xilinx.com>
>>>>>>>>>>> Reviewed-by: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
>>>>>>>>>>
>>>>>>>>>> I don't see quite how, but this is breaking running
>>>>>>>>>> test/nokia_rx51_test.sh (or, my fixup of this to apply to current master
>>>>>>>>>> was wrong, and is what's breaking the test).  Please rebase and confirm
>>>>>>>>>> that test passes as well, thanks!
>>>>>>>>>
>>>>>>>>> I've checked the issue with nokia_rx51_test.sh and could not find any
>>>>>>>>> issues in the patch. My assumption now is, that the very old Linux
>>>>>>>>> kernel (2.6.28) that is used in this Nokia test, still uses the bd_info
>>>>>>>>> struct in Linux to pass the memory information (via bd_memstart &
>>>>>>>>> bi_memsize), as was also done in the very old PowerPC days. With this
>>>>>>>>> patch now and the removal of these fields from bd_info, this might
>>>>>>>>> explain why this kernel does not boot any more (no output on the serial
>>>>>>>>> console at all).
>>>>>>>>>
>>>>>>>>> Pali, could you please check if my assumption is correct here? And if
>>>>>>>>> yes, could please switch the test to using a newer kernel version? Or
>>>>>>>>> remove the Linux kernel booting from the test?
>>>>>>>>
>>>>>>>> Yes, Nokia N900 uses "old" ATAGs. But we cannot remove it. New kernel
>>>>>>>> version does not contain Maemo patches which are required for Maemo
>>>>>>>> system which is still widely used. And yes, people are using it with
>>>>>>>> U-Boot.
>>>>>>>>
>>>>>>>> Second reason why we cannot remove support for ATAGs is that Nokia's
>>>>>>>> signed first stage bootloader pass other setup data via ATAGs for kernel
>>>>>>>> and U-Boot N900 board code parses it, reuse it and pass to kernel.
>>>>>>>>
>>>>>>>> And replacing first stage bootloader is not possible because it is
>>>>>>>> signed and signing keys are secret (now probably lost).
>>>>>>>
>>>>>>> Okay. But this patch is not removing ATAG support, but removes 2
>>>>>>> member from the bd_info struct (bi_memstart & bi_memsize), which are
>>>>>>> replaced (superseeded) by the bi_dram[].start/.size values. And AFAICT,
>>>>>>> the generation of the memory ATAG is already done based on these
>>>>>>> values:
>>>>>>>
>>>>>>> setup_memory_tags() in arch/arm/lib/bootm.c
>>>>>>>
>>>>>>> So I still fail to understand, why booting of this old kernel using
>>>>>>> ATAGs does not work with this patch applied. Perhaps you could give
>>>>>>> it a quick test? That would be really helpful. Here again the gitlab
>>>>>>> branch that you can use for some tests:
>>>>>>>
>>>>>>> https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commits/remove-config-nr-dram-banks-v3-2020-08-11
>>>>>
>>>>> I can do some tests on real N900 device at the end of week. But I doubt
>>>>> that I could debug something on device if kernel does not print any
>>>>> message.
>>>>>
>>>>> Running u-boot and kernel in N900 emulator with debug serial console is
>>>>> easier and you can do it too locally. If you look at that test script
>>>>> test/nokia_rx51_test.sh you could see that it downloads & compile qemu
>>>>> and prepare OneNAND MTD image with compiled u-boot + kernel binary.
>>>>>
>>>>> You can also run that test script locally, it does not require root and
>>>>> all data it stores into temporary "nokia_rx51_tmp" directory. This could
>>>>> help you to test your changes without need to push them to travis/gitlab
>>>>> ci testing.
>>>>>
>>>>>> So, I also thought maybe changing bd_t was it, and reverted just that
>>>>>> part locally, and it was still breaking.  It's also only breaking in one
>>>>>> of the three boot tests, if I follow things correctly.
>>>>>
>>>>> What about trying to *not* remove first two members of struct bd_info?
>>>>> Maybe it is part of ABI structure?
>>>>> https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6#688c66555c917f123c1d054aabd1c7023c6c4522_31_30
>>>>
>>>> Not changing the structure is what I tried and it still failed, unless I
>>>> made a thinko when I was checking it out.  Can you please look in to
>>>> this a bit?
>>>>
>>>
>>> if I understand this correctly, the Maemo support in Linux parses
>>> bd_info but do not use bi_memstart and bi_memsize but other member
>>> variables. If we remove bi_memstart and bi_memsize, the offsets of all
>>> other member variables are changing and therefore the "ABI" to Linux
>>> breaks.
>>>
>>> If so, we can remove all usages of bi_memstart and bi_memsize like
>>> Stefan did, but not the variables themselves. Maybe we should just
>>> update the inline documentation for bi_memstart and bi_memsize to state
>>> that them should not be used anymore and that offsets in bd_info must
>>> not be changed (e.g. by removing variables or by changing variable
>>> types to different sizes) to maintain ABI compatibility?
>>
>> I don't think we should keep bd_info around long term, let alone fix
>> its format. We use devicetree now.
>>
>> If we want to support such old kernels on this one board, how about
>> adding a Kconfig and a special header which has the file in the format
>> that this one board needs? That way it shouldn't break in future, but
>> we are not prevented from making future changes.
> 
> ATAGs for N900 are really required. It is not only for kernel, but also
> for reading parameters from first stage bootloader which starts uboot.

Just to make is clear: ATAGs are *not* removed with this patch. Its
"just" a change to the bd_info struct.

Thanks,
Stefan