[PATCH v2] CONFIG_NR_DRAM_BANKS: Remove unreferenced code as its always defined
Stefan Roese
sr at denx.de
Wed Aug 12 07:04:02 CEST 2020
Hi Pali,
On 11.08.20 18:19, Pali Rohár wrote:
> 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 <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?
>
> 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 <mgroeger at sysgo.de>
> */
>
> +#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.
Yes. Thanks for looking into this. I'll post a new patch version later
today, which fixes this issue.
> 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
Yes. Some platforms fill the bd_dram[x] values in local code and I
need to make sure not to overwrite it in this case.
Thanks,
Stefan
More information about the U-Boot
mailing list