[PATCH v2] CONFIG_NR_DRAM_BANKS: Remove unreferenced code as its always defined
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@denx.de Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Ramon Fried ramon.fried@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Michal Simek michal.simek@xilinx.com --- v2: - Change all references to bi_memstart & bi_memsize to bi_dram[0].start/ size and remove it from the bd_info struct completely, as suggested by Daniel
api/api_platform-mips.c | 4 ++-- api/api_platform-powerpc.c | 2 +- arch/arc/lib/cpu.c | 4 ++-- arch/mips/lib/boot.c | 2 +- arch/mips/lib/bootm.c | 2 +- arch/powerpc/cpu/mpc83xx/fdt.c | 2 +- arch/powerpc/cpu/mpc83xx/traps.c | 2 +- arch/powerpc/cpu/mpc85xx/fdt.c | 4 ++-- arch/powerpc/cpu/mpc85xx/traps.c | 2 +- arch/powerpc/cpu/mpc86xx/fdt.c | 2 +- arch/powerpc/cpu/mpc86xx/traps.c | 2 +- arch/powerpc/cpu/mpc8xx/fdt.c | 2 +- arch/powerpc/lib/bootm.c | 4 ++-- arch/x86/cpu/broadwell/cpu_from_spl.c | 2 -- arch/xtensa/lib/bootm.c | 4 ++-- board/Arcturus/ucp1020/spl.c | 4 ++-- board/cadence/xtfpga/xtfpga.c | 4 ++-- board/freescale/p1010rdb/spl.c | 4 ++-- board/freescale/p1_p2_rdb_pc/spl.c | 4 ++-- board/freescale/t102xrdb/spl.c | 4 ++-- board/freescale/t104xrdb/spl.c | 4 ++-- board/freescale/t208xqds/spl.c | 4 ++-- board/freescale/t208xrdb/spl.c | 4 ++-- board/freescale/t4rdb/spl.c | 4 ++-- board/xilinx/zynqmp/zynqmp.c | 2 -- cmd/bdinfo.c | 6 ++---- cmd/bedbug.c | 2 +- common/board_f.c | 10 +++------- common/image.c | 12 ------------ common/init/handoff.c | 4 ---- drivers/pci/pci-uclass.c | 17 +---------------- drivers/video/cfb_console.c | 8 +------- include/asm-generic/u-boot.h | 4 ---- include/handoff.h | 2 -- lib/fdtdec.c | 5 ----- lib/lmb.c | 7 ------- 36 files changed, 45 insertions(+), 110 deletions(-)
diff --git a/api/api_platform-mips.c b/api/api_platform-mips.c index 51cd328b3d..89acb8cbe6 100644 --- a/api/api_platform-mips.c +++ b/api/api_platform-mips.c @@ -24,8 +24,8 @@ DECLARE_GLOBAL_DATA_PTR; int platform_sys_info(struct sys_info *si) {
- platform_set_mr(si, gd->bd->bi_memstart, - gd->bd->bi_memsize, MR_ATTR_DRAM); + platform_set_mr(si, gd->bd-> bi_dram[0].start, + gd->bd->bi_dram[0].size, MR_ATTR_DRAM);
return 1; } diff --git a/api/api_platform-powerpc.c b/api/api_platform-powerpc.c index 15930cfdb6..8fff6e4cae 100644 --- a/api/api_platform-powerpc.c +++ b/api/api_platform-powerpc.c @@ -42,7 +42,7 @@ int platform_sys_info(struct sys_info *si) si->bar = 0; #endif
- platform_set_mr(si, gd->bd->bi_memstart, gd->bd->bi_memsize, MR_ATTR_DRAM); + platform_set_mr(si, gd->bd->bi_dram[0].start, gd->bd->bi_dram[0].size, MR_ATTR_DRAM); platform_set_mr(si, gd->bd->bi_flashstart, gd->bd->bi_flashsize, MR_ATTR_FLASH); platform_set_mr(si, gd->bd->bi_sramstart, gd->bd->bi_sramsize, MR_ATTR_SRAM);
diff --git a/arch/arc/lib/cpu.c b/arch/arc/lib/cpu.c index 27b5832a0c..80d68f0a67 100644 --- a/arch/arc/lib/cpu.c +++ b/arch/arc/lib/cpu.c @@ -27,8 +27,8 @@ int arch_cpu_init(void)
int arch_early_init_r(void) { - gd->bd->bi_memstart = CONFIG_SYS_SDRAM_BASE; - gd->bd->bi_memsize = CONFIG_SYS_SDRAM_SIZE; + gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE; + gd->bd->bi_dram[0].size = CONFIG_SYS_SDRAM_SIZE; return 0; }
diff --git a/arch/mips/lib/boot.c b/arch/mips/lib/boot.c index db862f6379..3b960691c5 100644 --- a/arch/mips/lib/boot.c +++ b/arch/mips/lib/boot.c @@ -17,7 +17,7 @@ unsigned long do_go_exec(ulong (*entry)(int, char * const []), * whole SDRAM area, since we don't know the size of the image * that was loaded. */ - flush_cache(gd->bd->bi_memstart, gd->ram_top - gd->bd->bi_memstart); + flush_cache(gd->bd->bi_dram[0].start, gd->ram_top - gd->bd->bi_dram[0].start);
return entry(argc, argv); } diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c index 0a13f6edb7..b3ef15963e 100644 --- a/arch/mips/lib/bootm.c +++ b/arch/mips/lib/bootm.c @@ -242,7 +242,7 @@ static int boot_reloc_fdt(bootm_headers_t *images) #if CONFIG_IS_ENABLED(MIPS_BOOT_FDT) && CONFIG_IS_ENABLED(OF_LIBFDT) int arch_fixup_fdt(void *blob) { - u64 mem_start = virt_to_phys((void *)gd->bd->bi_memstart); + u64 mem_start = virt_to_phys((void *)gd->bd->bi_dram[0].start); u64 mem_size = gd->ram_size;
return fdt_fixup_memory_banks(blob, &mem_start, &mem_size, 1); diff --git a/arch/powerpc/cpu/mpc83xx/fdt.c b/arch/powerpc/cpu/mpc83xx/fdt.c index ebdedb2888..05523ecc67 100644 --- a/arch/powerpc/cpu/mpc83xx/fdt.c +++ b/arch/powerpc/cpu/mpc83xx/fdt.c @@ -121,7 +121,7 @@ void ft_cpu_setup(void *blob, struct bd_info *bd) "clock-frequency", get_serial_clock(), 1); #endif
- fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd->bi_memsize); + fdt_fixup_memory(blob, (u64)bd->bi_dram[0].start, (u64)bd->bi_dram[0].size);
#if defined(CONFIG_BOOTCOUNT_LIMIT) && \ (defined(CONFIG_QE) && !defined(CONFIG_ARCH_MPC831X)) diff --git a/arch/powerpc/cpu/mpc83xx/traps.c b/arch/powerpc/cpu/mpc83xx/traps.c index c3cc119d65..5a8cd36420 100644 --- a/arch/powerpc/cpu/mpc83xx/traps.c +++ b/arch/powerpc/cpu/mpc83xx/traps.c @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; /* Returns 0 if exception not found and fixup otherwise. */ extern unsigned long search_exception_table(unsigned long);
-#define END_OF_MEM (gd->bd->bi_memstart + gd->bd->bi_memsize) +#define END_OF_MEM (gd->bd->bi_dram[0].start + gd->bd->bi_dram[0].size)
/* * Trap & Exception support diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c index 9569c1a64b..2cc567a5f4 100644 --- a/arch/powerpc/cpu/mpc85xx/fdt.c +++ b/arch/powerpc/cpu/mpc85xx/fdt.c @@ -672,10 +672,10 @@ void ft_cpu_setup(void *blob, struct bd_info *bd) "clock-frequency", get_bus_freq(0), 1); #endif
- fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd->bi_memsize); + fdt_fixup_memory(blob, (u64)bd->bi_dram[0].start, (u64)bd->bi_dram[0].size);
#ifdef CONFIG_MP - ft_fixup_cpu(blob, (u64)bd->bi_memstart + (u64)bd->bi_memsize); + ft_fixup_cpu(blob, (u64)bd->bi_dram[0].start + (u64)bd->bi_dram[0].size); ft_fixup_num_cores(blob); #endif
diff --git a/arch/powerpc/cpu/mpc85xx/traps.c b/arch/powerpc/cpu/mpc85xx/traps.c index f37a45e269..061b7b8f6e 100644 --- a/arch/powerpc/cpu/mpc85xx/traps.c +++ b/arch/powerpc/cpu/mpc85xx/traps.c @@ -37,7 +37,7 @@ extern unsigned long search_exception_table(unsigned long); * amount of memory on the system if we're unable to keep all * the memory mapped in. */ -#define END_OF_MEM (gd->bd->bi_memstart + get_effective_memsize()) +#define END_OF_MEM (gd->bd->bi_dram[0].start + get_effective_memsize())
static __inline__ void set_tsr(unsigned long val) { diff --git a/arch/powerpc/cpu/mpc86xx/fdt.c b/arch/powerpc/cpu/mpc86xx/fdt.c index 24e53115ec..143575befd 100644 --- a/arch/powerpc/cpu/mpc86xx/fdt.c +++ b/arch/powerpc/cpu/mpc86xx/fdt.c @@ -27,7 +27,7 @@ void ft_cpu_setup(void *blob, struct bd_info *bd) do_fixup_by_prop_u32(blob, "device_type", "soc", 4, "bus-frequency", bd->bi_busfreq, 1);
- fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd->bi_memsize); + fdt_fixup_memory(blob, (u64)bd->bi_dram[0].start, (u64)bd->bi_dram[0].size);
#ifdef CONFIG_SYS_NS16550 do_fixup_by_compat_u32(blob, "ns16550", diff --git a/arch/powerpc/cpu/mpc86xx/traps.c b/arch/powerpc/cpu/mpc86xx/traps.c index c0161e3379..532df93c3c 100644 --- a/arch/powerpc/cpu/mpc86xx/traps.c +++ b/arch/powerpc/cpu/mpc86xx/traps.c @@ -30,7 +30,7 @@ extern unsigned long search_exception_table(unsigned long); * amount of memory on the system if we're unable to keep all * the memory mapped in. */ -#define END_OF_MEM (gd->bd->bi_memstart + get_effective_memsize()) +#define END_OF_MEM (gd->bd->bi_dram[0].start + get_effective_memsize())
/* * Trap & Exception support diff --git a/arch/powerpc/cpu/mpc8xx/fdt.c b/arch/powerpc/cpu/mpc8xx/fdt.c index 4d952a3882..e593242631 100644 --- a/arch/powerpc/cpu/mpc8xx/fdt.c +++ b/arch/powerpc/cpu/mpc8xx/fdt.c @@ -25,5 +25,5 @@ void ft_cpu_setup(void *blob, struct bd_info *bd) do_fixup_by_compat_u32(blob, "fsl,cpm-brg", "clock-frequency", gd->arch.brg_clk, 1);
- fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd->bi_memsize); + fdt_fixup_memory(blob, (u64)bd->bi_dram[0].start, (u64)bd->bi_dram[0].size); } diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c index 8c8ed99cd3..d8bf5a14f6 100644 --- a/arch/powerpc/lib/bootm.c +++ b/arch/powerpc/lib/bootm.c @@ -298,8 +298,8 @@ void boot_prep_vxworks(bootm_headers_t *images) if (!images->ft_addr) return;
- base = (u64)gd->bd->bi_memstart; - size = (u64)gd->bd->bi_memsize; + base = (u64)gd->bd->bi_dram[0].start; + size = (u64)gd->bd->bi_dram[0].size;
off = fdt_path_offset(images->ft_addr, "/memory"); if (off < 0) diff --git a/arch/x86/cpu/broadwell/cpu_from_spl.c b/arch/x86/cpu/broadwell/cpu_from_spl.c index 6567d50653..4d4cdafa2b 100644 --- a/arch/x86/cpu/broadwell/cpu_from_spl.c +++ b/arch/x86/cpu/broadwell/cpu_from_spl.c @@ -53,14 +53,12 @@ void board_debug_uart_init(void)
int dram_init_banksize(void) { -#ifdef CONFIG_NR_DRAM_BANKS struct spl_handoff *ho;
ho = bloblist_find(BLOBLISTT_SPL_HANDOFF, sizeof(*ho)); if (!ho) return log_msg_ret("Missing SPL hand-off info", -ENOENT); handoff_load_dram_banks(ho); -#endif
return 0; } diff --git a/arch/xtensa/lib/bootm.c b/arch/xtensa/lib/bootm.c index 458eaf95c0..16ab51dbc9 100644 --- a/arch/xtensa/lib/bootm.c +++ b/arch/xtensa/lib/bootm.c @@ -48,8 +48,8 @@ static struct bp_tag *setup_memory_tag(struct bp_tag *params) params->size = sizeof(struct meminfo); mem = (struct meminfo *)params->data; mem->type = MEMORY_TYPE_CONVENTIONAL; - mem->start = bd->bi_memstart; - mem->end = bd->bi_memstart + bd->bi_memsize; + mem->start = bd->bi_dram[0].start; + mem->end = bd->bi_dram[0].start + bd->bi_dram[0].size;
printf(" MEMORY: tag:0x%04x, type:0X%lx, start:0X%lx, end:0X%lx\n", BP_TAG_MEMORY, mem->type, mem->start, mem->end); diff --git a/board/Arcturus/ucp1020/spl.c b/board/Arcturus/ucp1020/spl.c index 5416a5b663..8eb641ef8d 100644 --- a/board/Arcturus/ucp1020/spl.c +++ b/board/Arcturus/ucp1020/spl.c @@ -83,8 +83,8 @@ void board_init_r(gd_t *gd, ulong dest_addr) bd = (struct bd_info *)(CONFIG_SPL_GD_ADDR + sizeof(gd_t)); memset(bd, 0, sizeof(struct bd_info)); gd->bd = bd; - bd->bi_memstart = CONFIG_SYS_INIT_L2_ADDR; - bd->bi_memsize = CONFIG_SYS_L2_SIZE; + bd->bi_dram[0].start = CONFIG_SYS_INIT_L2_ADDR; + bd->bi_dram[0].size = CONFIG_SYS_L2_SIZE;
arch_cpu_init(); get_clocks(); diff --git a/board/cadence/xtfpga/xtfpga.c b/board/cadence/xtfpga/xtfpga.c index 2869e5cf68..95620b0a43 100644 --- a/board/cadence/xtfpga/xtfpga.c +++ b/board/cadence/xtfpga/xtfpga.c @@ -51,8 +51,8 @@ int checkboard(void)
int dram_init_banksize(void) { - gd->bd->bi_memstart = PHYSADDR(CONFIG_SYS_SDRAM_BASE); - gd->bd->bi_memsize = CONFIG_SYS_SDRAM_SIZE; + gd->bd->bi_dram[0].start = PHYSADDR(CONFIG_SYS_SDRAM_BASE); + gd->bd->bi_dram[0].size = CONFIG_SYS_SDRAM_SIZE;
return 0; } diff --git a/board/freescale/p1010rdb/spl.c b/board/freescale/p1010rdb/spl.c index 4ee4573d2b..ddc0af3faa 100644 --- a/board/freescale/p1010rdb/spl.c +++ b/board/freescale/p1010rdb/spl.c @@ -69,8 +69,8 @@ void board_init_r(gd_t *gd, ulong dest_addr) bd = (struct bd_info *)(CONFIG_SPL_GD_ADDR + sizeof(gd_t)); memset(bd, 0, sizeof(struct bd_info)); gd->bd = bd; - bd->bi_memstart = CONFIG_SYS_INIT_L2_ADDR; - bd->bi_memsize = CONFIG_SYS_L2_SIZE; + bd->bi_dram[0].start = CONFIG_SYS_INIT_L2_ADDR; + bd->bi_dram[0].size = CONFIG_SYS_L2_SIZE;
arch_cpu_init(); get_clocks(); diff --git a/board/freescale/p1_p2_rdb_pc/spl.c b/board/freescale/p1_p2_rdb_pc/spl.c index e76c3e82c3..f67b0fcbb9 100644 --- a/board/freescale/p1_p2_rdb_pc/spl.c +++ b/board/freescale/p1_p2_rdb_pc/spl.c @@ -75,8 +75,8 @@ void board_init_r(gd_t *gd, ulong dest_addr) bd = (struct bd_info *)(CONFIG_SPL_GD_ADDR + sizeof(gd_t)); memset(bd, 0, sizeof(struct bd_info)); gd->bd = bd; - bd->bi_memstart = CONFIG_SYS_INIT_L2_ADDR; - bd->bi_memsize = CONFIG_SYS_L2_SIZE; + bd->bi_dram[0].start = CONFIG_SYS_INIT_L2_ADDR; + bd->bi_dram[0].size = CONFIG_SYS_L2_SIZE;
arch_cpu_init(); get_clocks(); diff --git a/board/freescale/t102xrdb/spl.c b/board/freescale/t102xrdb/spl.c index da442fcc18..ce7ccfe41d 100644 --- a/board/freescale/t102xrdb/spl.c +++ b/board/freescale/t102xrdb/spl.c @@ -103,8 +103,8 @@ void board_init_r(gd_t *gd, ulong dest_addr) bd = (struct bd_info *)(gd + sizeof(gd_t)); memset(bd, 0, sizeof(struct bd_info)); gd->bd = bd; - bd->bi_memstart = CONFIG_SYS_INIT_L3_ADDR; - bd->bi_memsize = CONFIG_SYS_L3_SIZE; + bd->bi_dram[0].start = CONFIG_SYS_INIT_L3_ADDR; + bd->bi_dram[0].size = CONFIG_SYS_L3_SIZE;
arch_cpu_init(); get_clocks(); diff --git a/board/freescale/t104xrdb/spl.c b/board/freescale/t104xrdb/spl.c index f83d69ba15..4b5fb8b955 100644 --- a/board/freescale/t104xrdb/spl.c +++ b/board/freescale/t104xrdb/spl.c @@ -94,8 +94,8 @@ void board_init_r(gd_t *gd, ulong dest_addr) bd = (struct bd_info *)(gd + sizeof(gd_t)); memset(bd, 0, sizeof(struct bd_info)); gd->bd = bd; - bd->bi_memstart = CONFIG_SYS_INIT_L3_ADDR; - bd->bi_memsize = CONFIG_SYS_L3_SIZE; + bd->bi_dram[0].start = CONFIG_SYS_INIT_L3_ADDR; + bd->bi_dram[0].size = CONFIG_SYS_L3_SIZE;
arch_cpu_init(); get_clocks(); diff --git a/board/freescale/t208xqds/spl.c b/board/freescale/t208xqds/spl.c index c197884421..dc0046d93c 100644 --- a/board/freescale/t208xqds/spl.c +++ b/board/freescale/t208xqds/spl.c @@ -102,8 +102,8 @@ void board_init_r(gd_t *gd, ulong dest_addr) bd = (struct bd_info *)(gd + sizeof(gd_t)); memset(bd, 0, sizeof(struct bd_info)); gd->bd = bd; - bd->bi_memstart = CONFIG_SYS_INIT_L3_ADDR; - bd->bi_memsize = CONFIG_SYS_L3_SIZE; + bd->bi_dram[0].start = CONFIG_SYS_INIT_L3_ADDR; + bd->bi_dram[0].size = CONFIG_SYS_L3_SIZE;
arch_cpu_init(); get_clocks(); diff --git a/board/freescale/t208xrdb/spl.c b/board/freescale/t208xrdb/spl.c index 07aab6349c..f523622de9 100644 --- a/board/freescale/t208xrdb/spl.c +++ b/board/freescale/t208xrdb/spl.c @@ -72,8 +72,8 @@ void board_init_r(gd_t *gd, ulong dest_addr) bd = (struct bd_info *)(gd + sizeof(gd_t)); memset(bd, 0, sizeof(struct bd_info)); gd->bd = bd; - bd->bi_memstart = CONFIG_SYS_INIT_L3_ADDR; - bd->bi_memsize = CONFIG_SYS_L3_SIZE; + bd->bi_dram[0].start = CONFIG_SYS_INIT_L3_ADDR; + bd->bi_dram[0].size = CONFIG_SYS_L3_SIZE;
arch_cpu_init(); get_clocks(); diff --git a/board/freescale/t4rdb/spl.c b/board/freescale/t4rdb/spl.c index 64d2753da8..a5351e813a 100644 --- a/board/freescale/t4rdb/spl.c +++ b/board/freescale/t4rdb/spl.c @@ -75,8 +75,8 @@ void board_init_r(gd_t *gd, ulong dest_addr) bd = (struct bd_info *)(gd + sizeof(gd_t)); memset(bd, 0, sizeof(struct bd_info)); gd->bd = bd; - bd->bi_memstart = CONFIG_SYS_INIT_L3_ADDR; - bd->bi_memsize = CONFIG_SYS_L3_SIZE; + bd->bi_dram[0].start = CONFIG_SYS_INIT_L3_ADDR; + bd->bi_dram[0].size = CONFIG_SYS_L3_SIZE;
arch_cpu_init(); get_clocks(); diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c index ebb7172908..4cc5cb6fd7 100644 --- a/board/xilinx/zynqmp/zynqmp.c +++ b/board/xilinx/zynqmp/zynqmp.c @@ -467,10 +467,8 @@ int dram_init(void) #else int dram_init_banksize(void) { -#if defined(CONFIG_NR_DRAM_BANKS) gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE; gd->bd->bi_dram[0].size = get_effective_memsize(); -#endif
mem_map_fill();
diff --git a/cmd/bdinfo.c b/cmd/bdinfo.c index 8b2c105e77..39e5401e3d 100644 --- a/cmd/bdinfo.c +++ b/cmd/bdinfo.c @@ -47,7 +47,6 @@ void bdinfo_print_mhz(const char *name, unsigned long hz)
static void print_bi_dram(const struct bd_info *bd) { -#ifdef CONFIG_NR_DRAM_BANKS int i;
for (i = 0; i < CONFIG_NR_DRAM_BANKS; ++i) { @@ -57,7 +56,6 @@ static void print_bi_dram(const struct bd_info *bd) bdinfo_print_num("-> size", bd->bi_dram[i].size); } } -#endif }
__weak void arch_print_bdinfo(void) @@ -73,8 +71,8 @@ int do_bdinfo(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) #endif bdinfo_print_num("boot_params", (ulong)bd->bi_boot_params); print_bi_dram(bd); - bdinfo_print_num("memstart", (ulong)bd->bi_memstart); - print_lnum("memsize", (u64)bd->bi_memsize); + bdinfo_print_num("memstart", (ulong)bd->bi_dram[0].start); + print_lnum("memsize", (u64)bd->bi_dram[0].size); bdinfo_print_num("flashstart", (ulong)bd->bi_flashstart); bdinfo_print_num("flashsize", (ulong)bd->bi_flashsize); bdinfo_print_num("flashoffset", (ulong)bd->bi_flashoffset); diff --git a/cmd/bedbug.c b/cmd/bedbug.c index 81ce256480..0db42d7eed 100644 --- a/cmd/bedbug.c +++ b/cmd/bedbug.c @@ -348,7 +348,7 @@ int do_bedbug_stack(struct cmd_tbl *cmdtp, int flag, int argc, return 1; }
- top = gd->bd->bi_memstart + gd->bd->bi_memsize; + top = gd->bd->bi_dram[0].start + gd->bd->bi_dram[0].size; depth = 0;
printf ("Depth PC\n"); diff --git a/common/board_f.c b/common/board_f.c index 88ff0424a7..51a06c60d4 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -216,7 +216,6 @@ static int show_dram_config(void) { unsigned long long size;
-#ifdef CONFIG_NR_DRAM_BANKS int i;
debug("\nRAM Configuration:\n"); @@ -229,9 +228,6 @@ static int show_dram_config(void) #endif } debug("\nDRAM: "); -#else - size = gd->ram_size; -#endif
print_size(size, ""); board_add_ram_info(0); @@ -242,7 +238,7 @@ static int show_dram_config(void)
__weak int dram_init_banksize(void) { -#if defined(CONFIG_NR_DRAM_BANKS) && defined(CONFIG_SYS_SDRAM_BASE) +#if defined(CONFIG_SYS_SDRAM_BASE) gd->bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE; gd->bd->bi_dram[0].size = get_effective_memsize(); #endif @@ -607,8 +603,8 @@ static int setup_board_part1(void) /* * Save local variables to board info struct */ - bd->bi_memstart = CONFIG_SYS_SDRAM_BASE; /* start of memory */ - bd->bi_memsize = gd->ram_size; /* size in bytes */ + bd->bi_dram[0].start = CONFIG_SYS_SDRAM_BASE; /* start of memory */ + bd->bi_dram[0].size = gd->ram_size; /* size in bytes */
#ifdef CONFIG_SYS_SRAM_BASE bd->bi_sramstart = CONFIG_SYS_SRAM_BASE; /* start of SRAM */ diff --git a/common/image.c b/common/image.c index 9d7d5c17d1..fd55dfd0dd 100644 --- a/common/image.c +++ b/common/image.c @@ -666,13 +666,7 @@ ulong env_get_bootm_low(void) return tmp; }
-#if defined(CONFIG_SYS_SDRAM_BASE) - return CONFIG_SYS_SDRAM_BASE; -#elif defined(CONFIG_ARM) || defined(CONFIG_MICROBLAZE) return gd->bd->bi_dram[0].start; -#else - return 0; -#endif }
phys_size_t env_get_bootm_size(void) @@ -685,14 +679,8 @@ phys_size_t env_get_bootm_size(void) return tmp; }
-#if (defined(CONFIG_ARM) || defined(CONFIG_MICROBLAZE)) && \ - defined(CONFIG_NR_DRAM_BANKS) start = gd->bd->bi_dram[0].start; size = gd->bd->bi_dram[0].size; -#else - start = gd->bd->bi_memstart; - size = gd->bd->bi_memsize; -#endif
s = env_get("bootm_low"); if (s) diff --git a/common/init/handoff.c b/common/init/handoff.c index e00b43e6a7..c20fbf78b8 100644 --- a/common/init/handoff.c +++ b/common/init/handoff.c @@ -13,7 +13,6 @@ DECLARE_GLOBAL_DATA_PTR; void handoff_save_dram(struct spl_handoff *ho) { ho->ram_size = gd->ram_size; -#ifdef CONFIG_NR_DRAM_BANKS { struct bd_info *bd = gd->bd; int i; @@ -23,7 +22,6 @@ void handoff_save_dram(struct spl_handoff *ho) ho->ram_bank[i].size = bd->bi_dram[i].size; } } -#endif }
void handoff_load_dram_size(struct spl_handoff *ho) @@ -33,7 +31,6 @@ void handoff_load_dram_size(struct spl_handoff *ho)
void handoff_load_dram_banks(struct spl_handoff *ho) { -#ifdef CONFIG_NR_DRAM_BANKS { struct bd_info *bd = gd->bd; int i; @@ -43,5 +40,4 @@ void handoff_load_dram_banks(struct spl_handoff *ho) bd->bi_dram[i].size = ho->ram_bank[i].size; } } -#endif } diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index 834526c5a4..69fb46d3f4 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -871,6 +871,7 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, ofnode node) { int pci_addr_cells, addr_cells, size_cells; + struct bd_info *bd = gd->bd; int cells_per_record; const u32 *prop; int len; @@ -938,9 +939,6 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, }
/* Add a region for our local memory */ -#ifdef CONFIG_NR_DRAM_BANKS - struct bd_info *bd = gd->bd; - if (!bd) return;
@@ -958,19 +956,6 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node, PCI_REGION_MEM | PCI_REGION_SYS_MEMORY); } } -#else - phys_addr_t base = 0, size; - - size = gd->ram_size; -#ifdef CONFIG_SYS_SDRAM_BASE - base = CONFIG_SYS_SDRAM_BASE; -#endif - if (gd->pci_ram_top && gd->pci_ram_top < base + size) - size = gd->pci_ram_top - base; - if (size) - pci_set_region(hose->regions + hose->region_count++, base, - base, size, PCI_REGION_MEM | PCI_REGION_SYS_MEMORY); -#endif
return; } diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c index badade353e..3f07f4eb29 100644 --- a/drivers/video/cfb_console.c +++ b/drivers/video/cfb_console.c @@ -1983,8 +1983,6 @@ static void *video_logo(void) static int cfb_fb_is_in_dram(void) { struct bd_info *bd = gd->bd; -#if defined(CONFIG_ARM) || defined(CONFIG_NDS32) || \ -defined(CONFIG_SANDBOX) || defined(CONFIG_X86) ulong start, end; int i;
@@ -1995,11 +1993,7 @@ defined(CONFIG_SANDBOX) || defined(CONFIG_X86) (ulong)video_fb_address < end) return 1; } -#else - if ((ulong)video_fb_address >= bd->bi_memstart && - (ulong)video_fb_address < bd->bi_memstart + bd->bi_memsize) - return 1; -#endif + return 0; }
diff --git a/include/asm-generic/u-boot.h b/include/asm-generic/u-boot.h index 62e61d41cc..637de0c455 100644 --- a/include/asm-generic/u-boot.h +++ b/include/asm-generic/u-boot.h @@ -27,8 +27,6 @@ #include <linux/types.h>
struct bd_info { - unsigned long bi_memstart; /* start of DRAM memory */ - phys_size_t bi_memsize; /* size of DRAM memory in bytes */ unsigned long bi_flashstart; /* start of FLASH memory */ unsigned long bi_flashsize; /* size of FLASH memory */ unsigned long bi_flashoffset; /* reserved area for startup monitor */ @@ -70,12 +68,10 @@ struct bd_info { #endif ulong bi_arch_number; /* unique id for this board */ ulong bi_boot_params; /* where this board expects params */ -#ifdef CONFIG_NR_DRAM_BANKS struct { /* RAM configuration */ phys_addr_t start; phys_size_t size; } bi_dram[CONFIG_NR_DRAM_BANKS]; -#endif /* CONFIG_NR_DRAM_BANKS */ };
#endif /* __ASSEMBLY__ */ diff --git a/include/handoff.h b/include/handoff.h index 75d19b1f6e..070a79c1b9 100644 --- a/include/handoff.h +++ b/include/handoff.h @@ -20,12 +20,10 @@ struct spl_handoff { struct arch_spl_handoff arch; u64 ram_size; -#ifdef CONFIG_NR_DRAM_BANKS struct { u64 start; u64 size; } ram_bank[CONFIG_NR_DRAM_BANKS]; -#endif };
void handoff_save_dram(struct spl_handoff *ho); diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 78576b530f..ecbf10121d 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1055,8 +1055,6 @@ int fdtdec_setup_mem_size_base(void) return 0; }
-#if defined(CONFIG_NR_DRAM_BANKS) - static int get_next_memory_node(const void *blob, int mem) { do { @@ -1106,7 +1104,6 @@ int fdtdec_setup_memory_banksize(void)
return 0; } -#endif
#if CONFIG_IS_ENABLED(MULTI_DTB_FIT) # if CONFIG_IS_ENABLED(MULTI_DTB_FIT_GZIP) ||\ @@ -1569,7 +1566,6 @@ int fdtdec_resetup(int *rescan) } #endif
-#ifdef CONFIG_NR_DRAM_BANKS int fdtdec_decode_ram_size(const void *blob, const char *area, int board_id, phys_addr_t *basep, phys_size_t *sizep, struct bd_info *bd) @@ -1675,6 +1671,5 @@ int fdtdec_decode_ram_size(const void *blob, const char *area, int board_id,
return 0; } -#endif /* CONFIG_NR_DRAM_BANKS */
#endif /* !USE_HOSTCC */ diff --git a/lib/lmb.c b/lib/lmb.c index 2d680d8d02..f0adc9592e 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -113,22 +113,15 @@ static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob) /* Initialize the struct, add memory and call arch/board reserve functions */ void lmb_init_and_reserve(struct lmb *lmb, struct bd_info *bd, void *fdt_blob) { -#ifdef CONFIG_NR_DRAM_BANKS int i; -#endif
lmb_init(lmb); -#ifdef CONFIG_NR_DRAM_BANKS for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) { if (bd->bi_dram[i].size) { lmb_add(lmb, bd->bi_dram[i].start, bd->bi_dram[i].size); } } -#else - if (bd->bi_memsize) - lmb_add(lmb, bd->bi_memstart, bd->bi_memsize); -#endif lmb_reserve_common(lmb, fdt_blob); }
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@denx.de Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Ramon Fried ramon.fried@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Michal Simek michal.simek@xilinx.com
v2:
- Change all references to bi_memstart & bi_memsize to bi_dram[0].start/ size and remove it from the bd_info struct completely, as suggested by Daniel
api/api_platform-mips.c | 4 ++-- api/api_platform-powerpc.c | 2 +- arch/arc/lib/cpu.c | 4 ++-- arch/mips/lib/boot.c | 2 +- arch/mips/lib/bootm.c | 2 +- arch/powerpc/cpu/mpc83xx/fdt.c | 2 +- arch/powerpc/cpu/mpc83xx/traps.c | 2 +- arch/powerpc/cpu/mpc85xx/fdt.c | 4 ++-- arch/powerpc/cpu/mpc85xx/traps.c | 2 +- arch/powerpc/cpu/mpc86xx/fdt.c | 2 +- arch/powerpc/cpu/mpc86xx/traps.c | 2 +- arch/powerpc/cpu/mpc8xx/fdt.c | 2 +- arch/powerpc/lib/bootm.c | 4 ++-- arch/x86/cpu/broadwell/cpu_from_spl.c | 2 -- arch/xtensa/lib/bootm.c | 4 ++-- board/Arcturus/ucp1020/spl.c | 4 ++-- board/cadence/xtfpga/xtfpga.c | 4 ++-- board/freescale/p1010rdb/spl.c | 4 ++-- board/freescale/p1_p2_rdb_pc/spl.c | 4 ++-- board/freescale/t102xrdb/spl.c | 4 ++-- board/freescale/t104xrdb/spl.c | 4 ++-- board/freescale/t208xqds/spl.c | 4 ++-- board/freescale/t208xrdb/spl.c | 4 ++-- board/freescale/t4rdb/spl.c | 4 ++-- board/xilinx/zynqmp/zynqmp.c | 2 -- cmd/bdinfo.c | 6 ++---- cmd/bedbug.c | 2 +- common/board_f.c | 10 +++------- common/image.c | 12 ------------ common/init/handoff.c | 4 ---- drivers/pci/pci-uclass.c | 17 +---------------- drivers/video/cfb_console.c | 8 +------- include/asm-generic/u-boot.h | 4 ---- include/handoff.h | 2 -- lib/fdtdec.c | 5 ----- lib/lmb.c | 7 ------- 36 files changed, 45 insertions(+), 110 deletions(-)
nice cleanup, thanks.
Reviewed-by: Daniel Schwierzeck daniel.schwierzeck@gmail.com
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@denx.de Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Ramon Fried ramon.fried@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Michal Simek michal.simek@xilinx.com Reviewed-by: Daniel Schwierzeck daniel.schwierzeck@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!
Hi Tom, Hi Pali,
(added Pali because of the Nokia RX51 issue)
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@denx.de Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Ramon Fried ramon.fried@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Michal Simek michal.simek@xilinx.com Reviewed-by: Daniel Schwierzeck daniel.schwierzeck@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?
I've pushed the latest patch version into this branch (based on top of the latest TOT):
https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commits/remove-con...
The failing Azure CI report can be found here:
https://dev.azure.com/sr0718/u-boot/_build/results?buildId=20&view=resul...
A working Azure report is here for comparison:
https://dev.azure.com/sr0718/u-boot/_build/results?buildId=18&view=resul...
Thanks, Stefan
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.
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@denx.de Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Ramon Fried ramon.fried@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Michal Simek michal.simek@xilinx.com Reviewed-by: Daniel Schwierzeck daniel.schwierzeck@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).
I've pushed the latest patch version into this branch (based on top of the latest TOT):
https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commits/remove-con...
The failing Azure CI report can be found here:
https://dev.azure.com/sr0718/u-boot/_build/results?buildId=20&view=resul...
A working Azure report is here for comparison:
https://dev.azure.com/sr0718/u-boot/_build/results?buildId=18&view=resul...
Thanks, Stefan
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/7196447f2e7...
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@denx.de Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Ramon Fried ramon.fried@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Michal Simek michal.simek@xilinx.com Reviewed-by: Daniel Schwierzeck daniel.schwierzeck@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-con...
Thanks, Stefan
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/7196447f2e7...
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@denx.de Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Ramon Fried ramon.fried@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Michal Simek michal.simek@xilinx.com Reviewed-by: Daniel Schwierzeck daniel.schwierzeck@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-con...
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.
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/7196447f2e7...
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@denx.de Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: Tom Rini trini@konsulko.com Cc: Ramon Fried ramon.fried@gmail.com Cc: Simon Glass sjg@chromium.org Cc: Michal Simek michal.simek@xilinx.com Reviewed-by: Daniel Schwierzeck daniel.schwierzeck@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-con...
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/7196447f2e7...
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/7196447f2e7...
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@denx.de > Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com > Cc: Tom Rini trini@konsulko.com > Cc: Ramon Fried ramon.fried@gmail.com > Cc: Simon Glass sjg@chromium.org > Cc: Michal Simek michal.simek@xilinx.com > Reviewed-by: Daniel Schwierzeck daniel.schwierzeck@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-con...
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/7196447f2e7...
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?
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/7196447f2e7...
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@denx.de > > Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com > > Cc: Tom Rini trini@konsulko.com > > Cc: Ramon Fried ramon.fried@gmail.com > > Cc: Simon Glass sjg@chromium.org > > Cc: Michal Simek michal.simek@xilinx.com > > Reviewed-by: Daniel Schwierzeck daniel.schwierzeck@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-con...
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/7196447f2e7...
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?
Hi Daniel,
On Tue, 11 Aug 2020 at 10:10, Daniel Schwierzeck daniel.schwierzeck@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/7196447f2e7...
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@denx.de > > > Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com > > > Cc: Tom Rini trini@konsulko.com > > > Cc: Ramon Fried ramon.fried@gmail.com > > > Cc: Simon Glass sjg@chromium.org > > > Cc: Michal Simek michal.simek@xilinx.com > > > Reviewed-by: Daniel Schwierzeck daniel.schwierzeck@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-con...
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/7196447f2e7...
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.
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!
Regards, Simon
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@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/7196447f2e7...
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@denx.de > > > > Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com > > > > Cc: Tom Rini trini@konsulko.com > > > > Cc: Ramon Fried ramon.fried@gmail.com > > > > Cc: Simon Glass sjg@chromium.org > > > > Cc: Michal Simek michal.simek@xilinx.com > > > > Reviewed-by: Daniel Schwierzeck daniel.schwierzeck@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-con...
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/7196447f2e7...
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.
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@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/7196447f2e7... > > 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@denx.de >>>>> Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com >>>>> Cc: Tom Rini trini@konsulko.com >>>>> Cc: Ramon Fried ramon.fried@gmail.com >>>>> Cc: Simon Glass sjg@chromium.org >>>>> Cc: Michal Simek michal.simek@xilinx.com >>>>> Reviewed-by: Daniel Schwierzeck daniel.schwierzeck@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-con...
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/7196447f2e7...
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
Hi Daniel,
On 11.08.20 18:10, Daniel Schwierzeck 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/7196447f2e7...
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@denx.de >>> Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com >>> Cc: Tom Rini trini@konsulko.com >>> Cc: Ramon Fried ramon.fried@gmail.com >>> Cc: Simon Glass sjg@chromium.org >>> Cc: Michal Simek michal.simek@xilinx.com >>> Reviewed-by: Daniel Schwierzeck daniel.schwierzeck@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-con...
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/7196447f2e7...
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.
IIRC, only old PowerPC Linux uses the bd_info struct to pass the infos from the bootloader to the kernel.
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've changed the patch according to the "fix" from Pali (will post later today). And with this change all tests pass. So we need to decide if we want to maintain ABI compatibility (at least with old PowerPC kernels) or make the cut here and move forward with the "cleanup". My personal feeling is that we should just move on and remove the variables from bd_info.
Thanks, Stefan
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/7196447f2e7...
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@denx.de > > Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com > > Cc: Tom Rini trini@konsulko.com > > Cc: Ramon Fried ramon.fried@gmail.com > > Cc: Simon Glass sjg@chromium.org > > Cc: Michal Simek michal.simek@xilinx.com > > Reviewed-by: Daniel Schwierzeck daniel.schwierzeck@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-con...
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/7196447f2e7...
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@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.
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.
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/7196447f2e7...
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@denx.de >>> Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com >>> Cc: Tom Rini trini@konsulko.com >>> Cc: Ramon Fried ramon.fried@gmail.com >>> Cc: Simon Glass sjg@chromium.org >>> Cc: Michal Simek michal.simek@xilinx.com >>> Reviewed-by: Daniel Schwierzeck daniel.schwierzeck@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-con...
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/7196447f2e7...
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@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
participants (5)
-
Daniel Schwierzeck -
Pali Rohár -
Simon Glass -
Stefan Roese -
Tom Rini