On Tuesday 11 August 2020 10:58:47 Tom Rini wrote: > 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 <[email protected]> > > > > > > > > Cc: Daniel Schwierzeck <[email protected]> > > > > > > > > Cc: Tom Rini <[email protected]> > > > > > > > > Cc: Ramon Fried <[email protected]> > > > > > > > > Cc: Simon Glass <[email protected]> > > > > > > > > Cc: Michal Simek <[email protected]> > > > > > > > > Reviewed-by: Daniel Schwierzeck <[email protected]> > > > > > > > > > > > > > > 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?
Yes, I looked at that patch and seems it is really breaking something related to dram banks. I applied following debug patch diff --git a/common/board_f.c b/common/board_f.c index 51e9544b8e..ae6d37b574 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -9,6 +9,8 @@ * Marius Groeger <[email protected]> */ +#define DEBUG + #include <common.h> #include <bloblist.h> #include <bootstage.h> @@ -603,9 +606,17 @@ int setup_bdinfo(void) { struct bd_info *bd = gd->bd; + debug("Calling setup_bdinfo\n"); + + debug("CONFIG_NR_DRAM_BANKS is %u\n", CONFIG_NR_DRAM_BANKS); + + debug("Previous bd->bi_dram[0].start=%u bd->bi_dram[0].size=%u\n", bd->bi_dram[0].start, bd->bi_dram[0].size); + bd->bi_dram[0].start = gd->ram_base; /* start of memory */ bd->bi_dram[0].size = gd->ram_size; /* size in bytes */ + debug("New bd->bi_dram[0].start=%u bd->bi_dram[0].size=%u\n", bd->bi_dram[0].start, bd->bi_dram[0].size); + if (IS_ENABLED(CONFIG_SYS_HAS_SRAM)) { bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ bd->bi_sramsize = CONFIG_SYS_SRAM_SIZE; /* size of SRAM */ and run u-boot in qemu n900 emulator. It printed following: Calling setup_bdinfo CONFIG_NR_DRAM_BANKS is 2 Previous bd->bi_dram[0].start=2147483648 bd->bi_dram[0].size=134217728 New bd->bi_dram[0].start=2147483648 bd->bi_dram[0].size=268435456 So it looks like that Stefan's patch is overwriting bi_dram with incorrect value. It sets only bi_diram[0] (first member) even there are two banks. I applied following patch on top of Stefan's diff --git a/common/board_f.c b/common/board_f.c index 51e9544b8e..d6bf23e397 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -603,8 +606,10 @@ int setup_bdinfo(void) { struct bd_info *bd = gd->bd; +#if 0 bd->bi_dram[0].start = gd->ram_base; /* start of memory */ bd->bi_dram[0].size = gd->ram_size; /* size in bytes */ +#endif if (IS_ENABLED(CONFIG_SYS_HAS_SRAM)) { bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ and then booting Maemo kernel started working. So this just prove that above overwriting of bd->di_dram is incorrect or incomplete. For me the most suspicious is why Stefan's patch is overwriting only bd->bi_dram[0] even when CONFIG_NR_DRAM_BANKS is set to 2. It looks like that for OMAP3 is bi_dram structure filled by function dram_init_banksize() from file arch/arm/mach-omap2/omap3/sdrc.c I hope that these information helps you to debug and fix Stefan patch. Now thanks to nokia_rx51_test.sh it is easy to test and debug all changes for N900 in simulator. Moreover it is possible to use gdb to connect to qemu which in which is running u-boot. So these kind of problems should be possible to debug without real N900 hw, just in qemu and gdb.
