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

[Daniel Schwierzeck]

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?

-- 
- Daniel