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

[Pali Rohár]

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.

Anyway, look at my reply. That patch is either incorrect or incomplete
because when I added at one place #if 0 ... #endif Maemo kernel started
booting, even after removing those members from struct bd_info.

> BTW it is great that we have a test for this board and were able to
> catch this quickly. I hope we will have more such tests!

I'm happy that my test script is useful! N900 support in u-boot was
broken more times and finding problematic commit took me time. My idea
was that this could save (my) time in finding change which is breaking
something.

>From my past experience I know that only full-integration tests can
catch problems which unit tests or "one command line" tests in most
cases could not.