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

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Stefan Roese
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 <[hidden email]>
Cc: Daniel Schwierzeck <[hidden email]>
Cc: Tom Rini <[hidden email]>
Cc: Ramon Fried <[hidden email]>
Cc: Simon Glass <[hidden email]>
Cc: Michal Simek <[hidden email]>
---
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);
 }
 
--
2.28.0

Reply | Threaded
Open this post in threaded view
|

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

Daniel Schwierzeck-2
> 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 <[hidden email]>
> Cc: Daniel Schwierzeck <[hidden email]>
> Cc: Tom Rini <[hidden email]>
> Cc: Ramon Fried <[hidden email]>
> Cc: Simon Glass <[hidden email]>
> Cc: Michal Simek <[hidden email]>
> ---
> 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 <[hidden email]>

--
- Daniel

Reply | Threaded
Open this post in threaded view
|

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

Tom Rini-4
In reply to this post by Stefan Roese
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 <[hidden email]>
> Cc: Daniel Schwierzeck <[hidden email]>
> Cc: Tom Rini <[hidden email]>
> Cc: Ramon Fried <[hidden email]>
> Cc: Simon Glass <[hidden email]>
> Cc: Michal Simek <[hidden email]>
> Reviewed-by: Daniel Schwierzeck <[hidden email]>
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!

--
Tom

signature.asc (673 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Stefan Roese
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 <[hidden email]>
>> Cc: Daniel Schwierzeck <[hidden email]>
>> Cc: Tom Rini <[hidden email]>
>> Cc: Ramon Fried <[hidden email]>
>> Cc: Simon Glass <[hidden email]>
>> Cc: Michal Simek <[hidden email]>
>> Reviewed-by: Daniel Schwierzeck <[hidden email]>
>
> 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-config-nr-dram-banks-v3-2020-08-11

The failing Azure CI report can be found here:

https://dev.azure.com/sr0718/u-boot/_build/results?buildId=20&view=results

A working Azure report is here for comparison:

https://dev.azure.com/sr0718/u-boot/_build/results?buildId=18&view=results

Thanks,
Stefan
Reply | Threaded
Open this post in threaded view
|

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

Pali Rohár-2
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 <[hidden email]>
> > > Cc: Daniel Schwierzeck <[hidden email]>
> > > Cc: Tom Rini <[hidden email]>
> > > Cc: Ramon Fried <[hidden email]>
> > > Cc: Simon Glass <[hidden email]>
> > > Cc: Michal Simek <[hidden email]>
> > > Reviewed-by: Daniel Schwierzeck <[hidden email]>
> >
> > 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-config-nr-dram-banks-v3-2020-08-11
>
> The failing Azure CI report can be found here:
>
> https://dev.azure.com/sr0718/u-boot/_build/results?buildId=20&view=results
>
> A working Azure report is here for comparison:
>
> https://dev.azure.com/sr0718/u-boot/_build/results?buildId=18&view=results
>
> Thanks,
> Stefan
Reply | Threaded
Open this post in threaded view
|

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

Stefan Roese
Hi Pali,

On 11.08.20 10:00, Pali Rohár wrote:

> Hello!
>
> On Tuesday 11 August 2020 08:37:46 Stefan Roese wrote:
>> Hi Tom,
>> Hi Pali,
>>
>> (added Pali because of the Nokia RX51 issue)
>
> Could you please send me a link to "problematic" patch? As you have not
> inlined it in this email.

https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6

Its a rebase of the original patch.

>> On 07.08.20 21:21, Tom Rini wrote:
>>> On Thu, Jul 30, 2020 at 12:51:10PM +0200, Stefan Roese wrote:
>>>
>>>> Since commit 86cf1c82850f ("configs: Migrate CONFIG_NR_DRAM_BANKS") &
>>>> commit 999a772d9f24 ("Kconfig: Migrate CONFIG_NR_DRAM_BANKS"),
>>>> CONFIG_NR_DRAM_BANKS is always defined with a value (4 is default).
>>>> It makes no sense to still carry code that is guarded with
>>>> "#ifndef CONFIG_NR_DRAM_BANKS" (and similar). This patch removes
>>>> all these unreferenced code paths.
>>>>
>>>> Also complete remove bi_memstart & bi_memsize from the board-info
>>>> struct. As now bi_dram[] is always enabled and should be used instead.
>>>> This removes the redundant varriables.
>>>>
>>>> Signed-off-by: Stefan Roese <[hidden email]>
>>>> Cc: Daniel Schwierzeck <[hidden email]>
>>>> Cc: Tom Rini <[hidden email]>
>>>> Cc: Ramon Fried <[hidden email]>
>>>> Cc: Simon Glass <[hidden email]>
>>>> Cc: Michal Simek <[hidden email]>
>>>> Reviewed-by: Daniel Schwierzeck <[hidden email]>
>>>
>>> I don't see quite how, but this is breaking running
>>> test/nokia_rx51_test.sh (or, my fixup of this to apply to current master
>>> was wrong, and is what's breaking the test).  Please rebase and confirm
>>> that test passes as well, thanks!
>>
>> I've checked the issue with nokia_rx51_test.sh and could not find any
>> issues in the patch. My assumption now is, that the very old Linux
>> kernel (2.6.28) that is used in this Nokia test, still uses the bd_info
>> struct in Linux to pass the memory information (via bd_memstart &
>> bi_memsize), as was also done in the very old PowerPC days. With this
>> patch now and the removal of these fields from bd_info, this might
>> explain why this kernel does not boot any more (no output on the serial
>> console at all).
>>
>> Pali, could you please check if my assumption is correct here? And if
>> yes, could please switch the test to using a newer kernel version? Or
>> remove the Linux kernel booting from the test?
>
> Yes, Nokia N900 uses "old" ATAGs. But we cannot remove it. New kernel
> version does not contain Maemo patches which are required for Maemo
> system which is still widely used. And yes, people are using it with
> U-Boot.
>
> Second reason why we cannot remove support for ATAGs is that Nokia's
> signed first stage bootloader pass other setup data via ATAGs for kernel
> and U-Boot N900 board code parses it, reuse it and pass to kernel.
>
> And replacing first stage bootloader is not possible because it is
> signed and signing keys are secret (now probably lost).

Okay. But this patch is not removing ATAG support, but removes 2
member from the bd_info struct (bi_memstart & bi_memsize), which are
replaced (superseeded) by the bi_dram[].start/.size values. And AFAICT,
the generation of the memory ATAG is already done based on these
values:

setup_memory_tags() in arch/arm/lib/bootm.c

So I still fail to understand, why booting of this old kernel using
ATAGs does not work with this patch applied. Perhaps you could give
it a quick test? That would be really helpful. Here again the gitlab
branch that you can use for some tests:

https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commits/remove-config-nr-dram-banks-v3-2020-08-11

Thanks,
Stefan
Reply | Threaded
Open this post in threaded view
|

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

Tom Rini-4
On Tue, Aug 11, 2020 at 02:39:53PM +0200, Stefan Roese wrote:

> Hi Pali,
>
> On 11.08.20 10:00, Pali Rohár wrote:
> > Hello!
> >
> > On Tuesday 11 August 2020 08:37:46 Stefan Roese wrote:
> > > Hi Tom,
> > > Hi Pali,
> > >
> > > (added Pali because of the Nokia RX51 issue)
> >
> > Could you please send me a link to "problematic" patch? As you have not
> > inlined it in this email.
>
> https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6
>
> Its a rebase of the original patch.
>
> > > On 07.08.20 21:21, Tom Rini wrote:
> > > > On Thu, Jul 30, 2020 at 12:51:10PM +0200, Stefan Roese wrote:
> > > >
> > > > > Since commit 86cf1c82850f ("configs: Migrate CONFIG_NR_DRAM_BANKS") &
> > > > > commit 999a772d9f24 ("Kconfig: Migrate CONFIG_NR_DRAM_BANKS"),
> > > > > CONFIG_NR_DRAM_BANKS is always defined with a value (4 is default).
> > > > > It makes no sense to still carry code that is guarded with
> > > > > "#ifndef CONFIG_NR_DRAM_BANKS" (and similar). This patch removes
> > > > > all these unreferenced code paths.
> > > > >
> > > > > Also complete remove bi_memstart & bi_memsize from the board-info
> > > > > struct. As now bi_dram[] is always enabled and should be used instead.
> > > > > This removes the redundant varriables.
> > > > >
> > > > > Signed-off-by: Stefan Roese <[hidden email]>
> > > > > Cc: Daniel Schwierzeck <[hidden email]>
> > > > > Cc: Tom Rini <[hidden email]>
> > > > > Cc: Ramon Fried <[hidden email]>
> > > > > Cc: Simon Glass <[hidden email]>
> > > > > Cc: Michal Simek <[hidden email]>
> > > > > Reviewed-by: Daniel Schwierzeck <[hidden email]>
> > > >
> > > > I don't see quite how, but this is breaking running
> > > > test/nokia_rx51_test.sh (or, my fixup of this to apply to current master
> > > > was wrong, and is what's breaking the test).  Please rebase and confirm
> > > > that test passes as well, thanks!
> > >
> > > I've checked the issue with nokia_rx51_test.sh and could not find any
> > > issues in the patch. My assumption now is, that the very old Linux
> > > kernel (2.6.28) that is used in this Nokia test, still uses the bd_info
> > > struct in Linux to pass the memory information (via bd_memstart &
> > > bi_memsize), as was also done in the very old PowerPC days. With this
> > > patch now and the removal of these fields from bd_info, this might
> > > explain why this kernel does not boot any more (no output on the serial
> > > console at all).
> > >
> > > Pali, could you please check if my assumption is correct here? And if
> > > yes, could please switch the test to using a newer kernel version? Or
> > > remove the Linux kernel booting from the test?
> >
> > Yes, Nokia N900 uses "old" ATAGs. But we cannot remove it. New kernel
> > version does not contain Maemo patches which are required for Maemo
> > system which is still widely used. And yes, people are using it with
> > U-Boot.
> >
> > Second reason why we cannot remove support for ATAGs is that Nokia's
> > signed first stage bootloader pass other setup data via ATAGs for kernel
> > and U-Boot N900 board code parses it, reuse it and pass to kernel.
> >
> > And replacing first stage bootloader is not possible because it is
> > signed and signing keys are secret (now probably lost).
>
> Okay. But this patch is not removing ATAG support, but removes 2
> member from the bd_info struct (bi_memstart & bi_memsize), which are
> replaced (superseeded) by the bi_dram[].start/.size values. And AFAICT,
> the generation of the memory ATAG is already done based on these
> values:
>
> setup_memory_tags() in arch/arm/lib/bootm.c
>
> So I still fail to understand, why booting of this old kernel using
> ATAGs does not work with this patch applied. Perhaps you could give
> it a quick test? That would be really helpful. Here again the gitlab
> branch that you can use for some tests:
>
> https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commits/remove-config-nr-dram-banks-v3-2020-08-11
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.

--
Tom

signature.asc (673 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Pali Rohár-2
On Tuesday 11 August 2020 09:08:33 Tom Rini wrote:

> On Tue, Aug 11, 2020 at 02:39:53PM +0200, Stefan Roese wrote:
> > Hi Pali,
> >
> > On 11.08.20 10:00, Pali Rohár wrote:
> > > Hello!
> > >
> > > On Tuesday 11 August 2020 08:37:46 Stefan Roese wrote:
> > > > Hi Tom,
> > > > Hi Pali,
> > > >
> > > > (added Pali because of the Nokia RX51 issue)
> > >
> > > Could you please send me a link to "problematic" patch? As you have not
> > > inlined it in this email.
> >
> > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6
> >
> > Its a rebase of the original patch.
> >
> > > > On 07.08.20 21:21, Tom Rini wrote:
> > > > > On Thu, Jul 30, 2020 at 12:51:10PM +0200, Stefan Roese wrote:
> > > > >
> > > > > > Since commit 86cf1c82850f ("configs: Migrate CONFIG_NR_DRAM_BANKS") &
> > > > > > commit 999a772d9f24 ("Kconfig: Migrate CONFIG_NR_DRAM_BANKS"),
> > > > > > CONFIG_NR_DRAM_BANKS is always defined with a value (4 is default).
> > > > > > It makes no sense to still carry code that is guarded with
> > > > > > "#ifndef CONFIG_NR_DRAM_BANKS" (and similar). This patch removes
> > > > > > all these unreferenced code paths.
> > > > > >
> > > > > > Also complete remove bi_memstart & bi_memsize from the board-info
> > > > > > struct. As now bi_dram[] is always enabled and should be used instead.
> > > > > > This removes the redundant varriables.
> > > > > >
> > > > > > Signed-off-by: Stefan Roese <[hidden email]>
> > > > > > Cc: Daniel Schwierzeck <[hidden email]>
> > > > > > Cc: Tom Rini <[hidden email]>
> > > > > > Cc: Ramon Fried <[hidden email]>
> > > > > > Cc: Simon Glass <[hidden email]>
> > > > > > Cc: Michal Simek <[hidden email]>
> > > > > > Reviewed-by: Daniel Schwierzeck <[hidden email]>
> > > > >
> > > > > I don't see quite how, but this is breaking running
> > > > > test/nokia_rx51_test.sh (or, my fixup of this to apply to current master
> > > > > was wrong, and is what's breaking the test).  Please rebase and confirm
> > > > > that test passes as well, thanks!
> > > >
> > > > I've checked the issue with nokia_rx51_test.sh and could not find any
> > > > issues in the patch. My assumption now is, that the very old Linux
> > > > kernel (2.6.28) that is used in this Nokia test, still uses the bd_info
> > > > struct in Linux to pass the memory information (via bd_memstart &
> > > > bi_memsize), as was also done in the very old PowerPC days. With this
> > > > patch now and the removal of these fields from bd_info, this might
> > > > explain why this kernel does not boot any more (no output on the serial
> > > > console at all).
> > > >
> > > > Pali, could you please check if my assumption is correct here? And if
> > > > yes, could please switch the test to using a newer kernel version? Or
> > > > remove the Linux kernel booting from the test?
> > >
> > > Yes, Nokia N900 uses "old" ATAGs. But we cannot remove it. New kernel
> > > version does not contain Maemo patches which are required for Maemo
> > > system which is still widely used. And yes, people are using it with
> > > U-Boot.
> > >
> > > Second reason why we cannot remove support for ATAGs is that Nokia's
> > > signed first stage bootloader pass other setup data via ATAGs for kernel
> > > and U-Boot N900 board code parses it, reuse it and pass to kernel.
> > >
> > > And replacing first stage bootloader is not possible because it is
> > > signed and signing keys are secret (now probably lost).
> >
> > Okay. But this patch is not removing ATAG support, but removes 2
> > member from the bd_info struct (bi_memstart & bi_memsize), which are
> > replaced (superseeded) by the bi_dram[].start/.size values. And AFAICT,
> > the generation of the memory ATAG is already done based on these
> > values:
> >
> > setup_memory_tags() in arch/arm/lib/bootm.c
> >
> > So I still fail to understand, why booting of this old kernel using
> > ATAGs does not work with this patch applied. Perhaps you could give
> > it a quick test? That would be really helpful. Here again the gitlab
> > branch that you can use for some tests:
> >
> > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commits/remove-config-nr-dram-banks-v3-2020-08-11

I can do some tests on real N900 device at the end of week. But I doubt
that I could debug something on device if kernel does not print any
message.

Running u-boot and kernel in N900 emulator with debug serial console is
easier and you can do it too locally. If you look at that test script
test/nokia_rx51_test.sh you could see that it downloads & compile qemu
and prepare OneNAND MTD image with compiled u-boot + kernel binary.

You can also run that test script locally, it does not require root and
all data it stores into temporary "nokia_rx51_tmp" directory. This could
help you to test your changes without need to push them to travis/gitlab
ci testing.

> So, I also thought maybe changing bd_t was it, and reverted just that
> part locally, and it was still breaking.  It's also only breaking in one
> of the three boot tests, if I follow things correctly.

What about trying to *not* remove first two members of struct bd_info?
Maybe it is part of ABI structure?
https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6#688c66555c917f123c1d054aabd1c7023c6c4522_31_30
Reply | Threaded
Open this post in threaded view
|

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

Tom Rini-4
On Tue, Aug 11, 2020 at 03:22:48PM +0200, Pali Rohár wrote:

> On Tuesday 11 August 2020 09:08:33 Tom Rini wrote:
> > On Tue, Aug 11, 2020 at 02:39:53PM +0200, Stefan Roese wrote:
> > > Hi Pali,
> > >
> > > On 11.08.20 10:00, Pali Rohár wrote:
> > > > Hello!
> > > >
> > > > On Tuesday 11 August 2020 08:37:46 Stefan Roese wrote:
> > > > > Hi Tom,
> > > > > Hi Pali,
> > > > >
> > > > > (added Pali because of the Nokia RX51 issue)
> > > >
> > > > Could you please send me a link to "problematic" patch? As you have not
> > > > inlined it in this email.
> > >
> > > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6
> > >
> > > Its a rebase of the original patch.
> > >
> > > > > On 07.08.20 21:21, Tom Rini wrote:
> > > > > > On Thu, Jul 30, 2020 at 12:51:10PM +0200, Stefan Roese wrote:
> > > > > >
> > > > > > > Since commit 86cf1c82850f ("configs: Migrate CONFIG_NR_DRAM_BANKS") &
> > > > > > > commit 999a772d9f24 ("Kconfig: Migrate CONFIG_NR_DRAM_BANKS"),
> > > > > > > CONFIG_NR_DRAM_BANKS is always defined with a value (4 is default).
> > > > > > > It makes no sense to still carry code that is guarded with
> > > > > > > "#ifndef CONFIG_NR_DRAM_BANKS" (and similar). This patch removes
> > > > > > > all these unreferenced code paths.
> > > > > > >
> > > > > > > Also complete remove bi_memstart & bi_memsize from the board-info
> > > > > > > struct. As now bi_dram[] is always enabled and should be used instead.
> > > > > > > This removes the redundant varriables.
> > > > > > >
> > > > > > > Signed-off-by: Stefan Roese <[hidden email]>
> > > > > > > Cc: Daniel Schwierzeck <[hidden email]>
> > > > > > > Cc: Tom Rini <[hidden email]>
> > > > > > > Cc: Ramon Fried <[hidden email]>
> > > > > > > Cc: Simon Glass <[hidden email]>
> > > > > > > Cc: Michal Simek <[hidden email]>
> > > > > > > Reviewed-by: Daniel Schwierzeck <[hidden email]>
> > > > > >
> > > > > > I don't see quite how, but this is breaking running
> > > > > > test/nokia_rx51_test.sh (or, my fixup of this to apply to current master
> > > > > > was wrong, and is what's breaking the test).  Please rebase and confirm
> > > > > > that test passes as well, thanks!
> > > > >
> > > > > I've checked the issue with nokia_rx51_test.sh and could not find any
> > > > > issues in the patch. My assumption now is, that the very old Linux
> > > > > kernel (2.6.28) that is used in this Nokia test, still uses the bd_info
> > > > > struct in Linux to pass the memory information (via bd_memstart &
> > > > > bi_memsize), as was also done in the very old PowerPC days. With this
> > > > > patch now and the removal of these fields from bd_info, this might
> > > > > explain why this kernel does not boot any more (no output on the serial
> > > > > console at all).
> > > > >
> > > > > Pali, could you please check if my assumption is correct here? And if
> > > > > yes, could please switch the test to using a newer kernel version? Or
> > > > > remove the Linux kernel booting from the test?
> > > >
> > > > Yes, Nokia N900 uses "old" ATAGs. But we cannot remove it. New kernel
> > > > version does not contain Maemo patches which are required for Maemo
> > > > system which is still widely used. And yes, people are using it with
> > > > U-Boot.
> > > >
> > > > Second reason why we cannot remove support for ATAGs is that Nokia's
> > > > signed first stage bootloader pass other setup data via ATAGs for kernel
> > > > and U-Boot N900 board code parses it, reuse it and pass to kernel.
> > > >
> > > > And replacing first stage bootloader is not possible because it is
> > > > signed and signing keys are secret (now probably lost).
> > >
> > > Okay. But this patch is not removing ATAG support, but removes 2
> > > member from the bd_info struct (bi_memstart & bi_memsize), which are
> > > replaced (superseeded) by the bi_dram[].start/.size values. And AFAICT,
> > > the generation of the memory ATAG is already done based on these
> > > values:
> > >
> > > setup_memory_tags() in arch/arm/lib/bootm.c
> > >
> > > So I still fail to understand, why booting of this old kernel using
> > > ATAGs does not work with this patch applied. Perhaps you could give
> > > it a quick test? That would be really helpful. Here again the gitlab
> > > branch that you can use for some tests:
> > >
> > > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commits/remove-config-nr-dram-banks-v3-2020-08-11
>
> I can do some tests on real N900 device at the end of week. But I doubt
> that I could debug something on device if kernel does not print any
> message.
>
> Running u-boot and kernel in N900 emulator with debug serial console is
> easier and you can do it too locally. If you look at that test script
> test/nokia_rx51_test.sh you could see that it downloads & compile qemu
> and prepare OneNAND MTD image with compiled u-boot + kernel binary.
>
> You can also run that test script locally, it does not require root and
> all data it stores into temporary "nokia_rx51_tmp" directory. This could
> help you to test your changes without need to push them to travis/gitlab
> ci testing.
>
> > So, I also thought maybe changing bd_t was it, and reverted just that
> > part locally, and it was still breaking.  It's also only breaking in one
> > of the three boot tests, if I follow things correctly.
>
> What about trying to *not* remove first two members of struct bd_info?
> Maybe it is part of ABI structure?
> https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6#688c66555c917f123c1d054aabd1c7023c6c4522_31_30
Not changing the structure is what I tried and it still failed, unless I
made a thinko when I was checking it out.  Can you please look in to
this a bit?

--
Tom

signature.asc (673 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Daniel Schwierzeck-2
Am Dienstag, den 11.08.2020, 10:58 -0400 schrieb Tom Rini:

> On Tue, Aug 11, 2020 at 03:22:48PM +0200, Pali Rohár wrote:
> > On Tuesday 11 August 2020 09:08:33 Tom Rini wrote:
> > > On Tue, Aug 11, 2020 at 02:39:53PM +0200, Stefan Roese wrote:
> > > > Hi Pali,
> > > >
> > > > On 11.08.20 10:00, Pali Rohár wrote:
> > > > > Hello!
> > > > >
> > > > > On Tuesday 11 August 2020 08:37:46 Stefan Roese wrote:
> > > > > > Hi Tom,
> > > > > > Hi Pali,
> > > > > >
> > > > > > (added Pali because of the Nokia RX51 issue)
> > > > >
> > > > > Could you please send me a link to "problematic" patch? As you have not
> > > > > inlined it in this email.
> > > >
> > > > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6
> > > >
> > > > Its a rebase of the original patch.
> > > >
> > > > > > On 07.08.20 21:21, Tom Rini wrote:
> > > > > > > On Thu, Jul 30, 2020 at 12:51:10PM +0200, Stefan Roese wrote:
> > > > > > >
> > > > > > > > Since commit 86cf1c82850f ("configs: Migrate CONFIG_NR_DRAM_BANKS") &
> > > > > > > > commit 999a772d9f24 ("Kconfig: Migrate CONFIG_NR_DRAM_BANKS"),
> > > > > > > > CONFIG_NR_DRAM_BANKS is always defined with a value (4 is default).
> > > > > > > > It makes no sense to still carry code that is guarded with
> > > > > > > > "#ifndef CONFIG_NR_DRAM_BANKS" (and similar). This patch removes
> > > > > > > > all these unreferenced code paths.
> > > > > > > >
> > > > > > > > Also complete remove bi_memstart & bi_memsize from the board-info
> > > > > > > > struct. As now bi_dram[] is always enabled and should be used instead.
> > > > > > > > This removes the redundant varriables.
> > > > > > > >
> > > > > > > > Signed-off-by: Stefan Roese <[hidden email]>
> > > > > > > > Cc: Daniel Schwierzeck <[hidden email]>
> > > > > > > > Cc: Tom Rini <[hidden email]>
> > > > > > > > Cc: Ramon Fried <[hidden email]>
> > > > > > > > Cc: Simon Glass <[hidden email]>
> > > > > > > > Cc: Michal Simek <[hidden email]>
> > > > > > > > Reviewed-by: Daniel Schwierzeck <[hidden email]>
> > > > > > >
> > > > > > > I don't see quite how, but this is breaking running
> > > > > > > test/nokia_rx51_test.sh (or, my fixup of this to apply to current master
> > > > > > > was wrong, and is what's breaking the test).  Please rebase and confirm
> > > > > > > that test passes as well, thanks!
> > > > > >
> > > > > > I've checked the issue with nokia_rx51_test.sh and could not find any
> > > > > > issues in the patch. My assumption now is, that the very old Linux
> > > > > > kernel (2.6.28) that is used in this Nokia test, still uses the bd_info
> > > > > > struct in Linux to pass the memory information (via bd_memstart &
> > > > > > bi_memsize), as was also done in the very old PowerPC days. With this
> > > > > > patch now and the removal of these fields from bd_info, this might
> > > > > > explain why this kernel does not boot any more (no output on the serial
> > > > > > console at all).
> > > > > >
> > > > > > Pali, could you please check if my assumption is correct here? And if
> > > > > > yes, could please switch the test to using a newer kernel version? Or
> > > > > > remove the Linux kernel booting from the test?
> > > > >
> > > > > Yes, Nokia N900 uses "old" ATAGs. But we cannot remove it. New kernel
> > > > > version does not contain Maemo patches which are required for Maemo
> > > > > system which is still widely used. And yes, people are using it with
> > > > > U-Boot.
> > > > >
> > > > > Second reason why we cannot remove support for ATAGs is that Nokia's
> > > > > signed first stage bootloader pass other setup data via ATAGs for kernel
> > > > > and U-Boot N900 board code parses it, reuse it and pass to kernel.
> > > > >
> > > > > And replacing first stage bootloader is not possible because it is
> > > > > signed and signing keys are secret (now probably lost).
> > > >
> > > > Okay. But this patch is not removing ATAG support, but removes 2
> > > > member from the bd_info struct (bi_memstart & bi_memsize), which are
> > > > replaced (superseeded) by the bi_dram[].start/.size values. And AFAICT,
> > > > the generation of the memory ATAG is already done based on these
> > > > values:
> > > >
> > > > setup_memory_tags() in arch/arm/lib/bootm.c
> > > >
> > > > So I still fail to understand, why booting of this old kernel using
> > > > ATAGs does not work with this patch applied. Perhaps you could give
> > > > it a quick test? That would be really helpful. Here again the gitlab
> > > > branch that you can use for some tests:
> > > >
> > > > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commits/remove-config-nr-dram-banks-v3-2020-08-11
> >
> > I can do some tests on real N900 device at the end of week. But I doubt
> > that I could debug something on device if kernel does not print any
> > message.
> >
> > Running u-boot and kernel in N900 emulator with debug serial console is
> > easier and you can do it too locally. If you look at that test script
> > test/nokia_rx51_test.sh you could see that it downloads & compile qemu
> > and prepare OneNAND MTD image with compiled u-boot + kernel binary.
> >
> > You can also run that test script locally, it does not require root and
> > all data it stores into temporary "nokia_rx51_tmp" directory. This could
> > help you to test your changes without need to push them to travis/gitlab
> > ci testing.
> >
> > > So, I also thought maybe changing bd_t was it, and reverted just that
> > > part locally, and it was still breaking.  It's also only breaking in one
> > > of the three boot tests, if I follow things correctly.
> >
> > What about trying to *not* remove first two members of struct bd_info?
> > Maybe it is part of ABI structure?
> > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6#688c66555c917f123c1d054aabd1c7023c6c4522_31_30
>
> Not changing the structure is what I tried and it still failed, unless I
> made a thinko when I was checking it out.  Can you please look in to
> this a bit?
>

if I understand this correctly, the Maemo support in Linux parses
bd_info but do not use bi_memstart and bi_memsize but other member
variables. If we remove bi_memstart and bi_memsize, the offsets of all
other member variables are changing and therefore the "ABI" to Linux
breaks.

If so, we can remove all usages of bi_memstart and bi_memsize like
Stefan did, but not the variables themselves. Maybe we should just
update the inline documentation for bi_memstart and bi_memsize to state
that them should not be used anymore and that offsets in bd_info must
not be changed (e.g. by removing variables or by changing variable
types to different sizes) to maintain ABI compatibility?

--
- Daniel

Reply | Threaded
Open this post in threaded view
|

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

Pali Rohár-2
In reply to this post by Tom Rini-4
On Tuesday 11 August 2020 10:58:47 Tom Rini wrote:

> On Tue, Aug 11, 2020 at 03:22:48PM +0200, Pali Rohár wrote:
> > On Tuesday 11 August 2020 09:08:33 Tom Rini wrote:
> > > On Tue, Aug 11, 2020 at 02:39:53PM +0200, Stefan Roese wrote:
> > > > Hi Pali,
> > > >
> > > > On 11.08.20 10:00, Pali Rohár wrote:
> > > > > Hello!
> > > > >
> > > > > On Tuesday 11 August 2020 08:37:46 Stefan Roese wrote:
> > > > > > Hi Tom,
> > > > > > Hi Pali,
> > > > > >
> > > > > > (added Pali because of the Nokia RX51 issue)
> > > > >
> > > > > Could you please send me a link to "problematic" patch? As you have not
> > > > > inlined it in this email.
> > > >
> > > > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6
> > > >
> > > > Its a rebase of the original patch.
> > > >
> > > > > > On 07.08.20 21:21, Tom Rini wrote:
> > > > > > > On Thu, Jul 30, 2020 at 12:51:10PM +0200, Stefan Roese wrote:
> > > > > > >
> > > > > > > > Since commit 86cf1c82850f ("configs: Migrate CONFIG_NR_DRAM_BANKS") &
> > > > > > > > commit 999a772d9f24 ("Kconfig: Migrate CONFIG_NR_DRAM_BANKS"),
> > > > > > > > CONFIG_NR_DRAM_BANKS is always defined with a value (4 is default).
> > > > > > > > It makes no sense to still carry code that is guarded with
> > > > > > > > "#ifndef CONFIG_NR_DRAM_BANKS" (and similar). This patch removes
> > > > > > > > all these unreferenced code paths.
> > > > > > > >
> > > > > > > > Also complete remove bi_memstart & bi_memsize from the board-info
> > > > > > > > struct. As now bi_dram[] is always enabled and should be used instead.
> > > > > > > > This removes the redundant varriables.
> > > > > > > >
> > > > > > > > Signed-off-by: Stefan Roese <[hidden email]>
> > > > > > > > Cc: Daniel Schwierzeck <[hidden email]>
> > > > > > > > Cc: Tom Rini <[hidden email]>
> > > > > > > > Cc: Ramon Fried <[hidden email]>
> > > > > > > > Cc: Simon Glass <[hidden email]>
> > > > > > > > Cc: Michal Simek <[hidden email]>
> > > > > > > > Reviewed-by: Daniel Schwierzeck <[hidden email]>
> > > > > > >
> > > > > > > I don't see quite how, but this is breaking running
> > > > > > > test/nokia_rx51_test.sh (or, my fixup of this to apply to current master
> > > > > > > was wrong, and is what's breaking the test).  Please rebase and confirm
> > > > > > > that test passes as well, thanks!
> > > > > >
> > > > > > I've checked the issue with nokia_rx51_test.sh and could not find any
> > > > > > issues in the patch. My assumption now is, that the very old Linux
> > > > > > kernel (2.6.28) that is used in this Nokia test, still uses the bd_info
> > > > > > struct in Linux to pass the memory information (via bd_memstart &
> > > > > > bi_memsize), as was also done in the very old PowerPC days. With this
> > > > > > patch now and the removal of these fields from bd_info, this might
> > > > > > explain why this kernel does not boot any more (no output on the serial
> > > > > > console at all).
> > > > > >
> > > > > > Pali, could you please check if my assumption is correct here? And if
> > > > > > yes, could please switch the test to using a newer kernel version? Or
> > > > > > remove the Linux kernel booting from the test?
> > > > >
> > > > > Yes, Nokia N900 uses "old" ATAGs. But we cannot remove it. New kernel
> > > > > version does not contain Maemo patches which are required for Maemo
> > > > > system which is still widely used. And yes, people are using it with
> > > > > U-Boot.
> > > > >
> > > > > Second reason why we cannot remove support for ATAGs is that Nokia's
> > > > > signed first stage bootloader pass other setup data via ATAGs for kernel
> > > > > and U-Boot N900 board code parses it, reuse it and pass to kernel.
> > > > >
> > > > > And replacing first stage bootloader is not possible because it is
> > > > > signed and signing keys are secret (now probably lost).
> > > >
> > > > Okay. But this patch is not removing ATAG support, but removes 2
> > > > member from the bd_info struct (bi_memstart & bi_memsize), which are
> > > > replaced (superseeded) by the bi_dram[].start/.size values. And AFAICT,
> > > > the generation of the memory ATAG is already done based on these
> > > > values:
> > > >
> > > > setup_memory_tags() in arch/arm/lib/bootm.c
> > > >
> > > > So I still fail to understand, why booting of this old kernel using
> > > > ATAGs does not work with this patch applied. Perhaps you could give
> > > > it a quick test? That would be really helpful. Here again the gitlab
> > > > branch that you can use for some tests:
> > > >
> > > > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commits/remove-config-nr-dram-banks-v3-2020-08-11
> >
> > I can do some tests on real N900 device at the end of week. But I doubt
> > that I could debug something on device if kernel does not print any
> > message.
> >
> > Running u-boot and kernel in N900 emulator with debug serial console is
> > easier and you can do it too locally. If you look at that test script
> > test/nokia_rx51_test.sh you could see that it downloads & compile qemu
> > and prepare OneNAND MTD image with compiled u-boot + kernel binary.
> >
> > You can also run that test script locally, it does not require root and
> > all data it stores into temporary "nokia_rx51_tmp" directory. This could
> > help you to test your changes without need to push them to travis/gitlab
> > ci testing.
> >
> > > So, I also thought maybe changing bd_t was it, and reverted just that
> > > part locally, and it was still breaking.  It's also only breaking in one
> > > of the three boot tests, if I follow things correctly.
> >
> > What about trying to *not* remove first two members of struct bd_info?
> > Maybe it is part of ABI structure?
> > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6#688c66555c917f123c1d054aabd1c7023c6c4522_31_30
>
> Not changing the structure is what I tried and it still failed, unless I
> made a thinko when I was checking it out.  Can you please look in to
> this a bit?

Yes, I looked at that patch and seems it is really breaking something
related to dram banks.

I applied following debug patch

diff --git a/common/board_f.c b/common/board_f.c
index 51e9544b8e..ae6d37b574 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -9,6 +9,8 @@
  * Marius Groeger <[hidden email]>
  */
 
+#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.
Reply | Threaded
Open this post in threaded view
|

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

Simon Glass-3
In reply to this post by Daniel Schwierzeck-2
Hi Daniel,

On Tue, 11 Aug 2020 at 10:10, Daniel Schwierzeck
<[hidden email]> wrote:

>
> Am Dienstag, den 11.08.2020, 10:58 -0400 schrieb Tom Rini:
> > On Tue, Aug 11, 2020 at 03:22:48PM +0200, Pali Rohár wrote:
> > > On Tuesday 11 August 2020 09:08:33 Tom Rini wrote:
> > > > On Tue, Aug 11, 2020 at 02:39:53PM +0200, Stefan Roese wrote:
> > > > > Hi Pali,
> > > > >
> > > > > On 11.08.20 10:00, Pali Rohár wrote:
> > > > > > Hello!
> > > > > >
> > > > > > On Tuesday 11 August 2020 08:37:46 Stefan Roese wrote:
> > > > > > > Hi Tom,
> > > > > > > Hi Pali,
> > > > > > >
> > > > > > > (added Pali because of the Nokia RX51 issue)
> > > > > >
> > > > > > Could you please send me a link to "problematic" patch? As you have not
> > > > > > inlined it in this email.
> > > > >
> > > > > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6
> > > > >
> > > > > Its a rebase of the original patch.
> > > > >
> > > > > > > On 07.08.20 21:21, Tom Rini wrote:
> > > > > > > > On Thu, Jul 30, 2020 at 12:51:10PM +0200, Stefan Roese wrote:
> > > > > > > >
> > > > > > > > > Since commit 86cf1c82850f ("configs: Migrate CONFIG_NR_DRAM_BANKS") &
> > > > > > > > > commit 999a772d9f24 ("Kconfig: Migrate CONFIG_NR_DRAM_BANKS"),
> > > > > > > > > CONFIG_NR_DRAM_BANKS is always defined with a value (4 is default).
> > > > > > > > > It makes no sense to still carry code that is guarded with
> > > > > > > > > "#ifndef CONFIG_NR_DRAM_BANKS" (and similar). This patch removes
> > > > > > > > > all these unreferenced code paths.
> > > > > > > > >
> > > > > > > > > Also complete remove bi_memstart & bi_memsize from the board-info
> > > > > > > > > struct. As now bi_dram[] is always enabled and should be used instead.
> > > > > > > > > This removes the redundant varriables.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Stefan Roese <[hidden email]>
> > > > > > > > > Cc: Daniel Schwierzeck <[hidden email]>
> > > > > > > > > Cc: Tom Rini <[hidden email]>
> > > > > > > > > Cc: Ramon Fried <[hidden email]>
> > > > > > > > > Cc: Simon Glass <[hidden email]>
> > > > > > > > > Cc: Michal Simek <[hidden email]>
> > > > > > > > > Reviewed-by: Daniel Schwierzeck <[hidden email]>
> > > > > > > >
> > > > > > > > I don't see quite how, but this is breaking running
> > > > > > > > test/nokia_rx51_test.sh (or, my fixup of this to apply to current master
> > > > > > > > was wrong, and is what's breaking the test).  Please rebase and confirm
> > > > > > > > that test passes as well, thanks!
> > > > > > >
> > > > > > > I've checked the issue with nokia_rx51_test.sh and could not find any
> > > > > > > issues in the patch. My assumption now is, that the very old Linux
> > > > > > > kernel (2.6.28) that is used in this Nokia test, still uses the bd_info
> > > > > > > struct in Linux to pass the memory information (via bd_memstart &
> > > > > > > bi_memsize), as was also done in the very old PowerPC days. With this
> > > > > > > patch now and the removal of these fields from bd_info, this might
> > > > > > > explain why this kernel does not boot any more (no output on the serial
> > > > > > > console at all).
> > > > > > >
> > > > > > > Pali, could you please check if my assumption is correct here? And if
> > > > > > > yes, could please switch the test to using a newer kernel version? Or
> > > > > > > remove the Linux kernel booting from the test?
> > > > > >
> > > > > > Yes, Nokia N900 uses "old" ATAGs. But we cannot remove it. New kernel
> > > > > > version does not contain Maemo patches which are required for Maemo
> > > > > > system which is still widely used. And yes, people are using it with
> > > > > > U-Boot.
> > > > > >
> > > > > > Second reason why we cannot remove support for ATAGs is that Nokia's
> > > > > > signed first stage bootloader pass other setup data via ATAGs for kernel
> > > > > > and U-Boot N900 board code parses it, reuse it and pass to kernel.
> > > > > >
> > > > > > And replacing first stage bootloader is not possible because it is
> > > > > > signed and signing keys are secret (now probably lost).
> > > > >
> > > > > Okay. But this patch is not removing ATAG support, but removes 2
> > > > > member from the bd_info struct (bi_memstart & bi_memsize), which are
> > > > > replaced (superseeded) by the bi_dram[].start/.size values. And AFAICT,
> > > > > the generation of the memory ATAG is already done based on these
> > > > > values:
> > > > >
> > > > > setup_memory_tags() in arch/arm/lib/bootm.c
> > > > >
> > > > > So I still fail to understand, why booting of this old kernel using
> > > > > ATAGs does not work with this patch applied. Perhaps you could give
> > > > > it a quick test? That would be really helpful. Here again the gitlab
> > > > > branch that you can use for some tests:
> > > > >
> > > > > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commits/remove-config-nr-dram-banks-v3-2020-08-11
> > >
> > > I can do some tests on real N900 device at the end of week. But I doubt
> > > that I could debug something on device if kernel does not print any
> > > message.
> > >
> > > Running u-boot and kernel in N900 emulator with debug serial console is
> > > easier and you can do it too locally. If you look at that test script
> > > test/nokia_rx51_test.sh you could see that it downloads & compile qemu
> > > and prepare OneNAND MTD image with compiled u-boot + kernel binary.
> > >
> > > You can also run that test script locally, it does not require root and
> > > all data it stores into temporary "nokia_rx51_tmp" directory. This could
> > > help you to test your changes without need to push them to travis/gitlab
> > > ci testing.
> > >
> > > > So, I also thought maybe changing bd_t was it, and reverted just that
> > > > part locally, and it was still breaking.  It's also only breaking in one
> > > > of the three boot tests, if I follow things correctly.
> > >
> > > What about trying to *not* remove first two members of struct bd_info?
> > > Maybe it is part of ABI structure?
> > > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6#688c66555c917f123c1d054aabd1c7023c6c4522_31_30
> >
> > Not changing the structure is what I tried and it still failed, unless I
> > made a thinko when I was checking it out.  Can you please look in to
> > this a bit?
> >
>
> if I understand this correctly, the Maemo support in Linux parses
> bd_info but do not use bi_memstart and bi_memsize but other member
> variables. If we remove bi_memstart and bi_memsize, the offsets of all
> other member variables are changing and therefore the "ABI" to Linux
> breaks.
>
> If so, we can remove all usages of bi_memstart and bi_memsize like
> Stefan did, but not the variables themselves. Maybe we should just
> update the inline documentation for bi_memstart and bi_memsize to state
> that them should not be used anymore and that offsets in bd_info must
> not be changed (e.g. by removing variables or by changing variable
> types to different sizes) to maintain ABI compatibility?

I don't think we should keep bd_info around long term, let alone fix
its format. We use devicetree now.

If we want to support such old kernels on this one board, how about
adding a Kconfig and a special header which has the file in the format
that this one board needs? That way it shouldn't break in future, but
we are not prevented from making future changes.

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
Reply | Threaded
Open this post in threaded view
|

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

Pali Rohár-2
On Tuesday 11 August 2020 10:22:44 Simon Glass wrote:

> Hi Daniel,
>
> On Tue, 11 Aug 2020 at 10:10, Daniel Schwierzeck
> <[hidden email]> wrote:
> >
> > Am Dienstag, den 11.08.2020, 10:58 -0400 schrieb Tom Rini:
> > > On Tue, Aug 11, 2020 at 03:22:48PM +0200, Pali Rohár wrote:
> > > > On Tuesday 11 August 2020 09:08:33 Tom Rini wrote:
> > > > > On Tue, Aug 11, 2020 at 02:39:53PM +0200, Stefan Roese wrote:
> > > > > > Hi Pali,
> > > > > >
> > > > > > On 11.08.20 10:00, Pali Rohár wrote:
> > > > > > > Hello!
> > > > > > >
> > > > > > > On Tuesday 11 August 2020 08:37:46 Stefan Roese wrote:
> > > > > > > > Hi Tom,
> > > > > > > > Hi Pali,
> > > > > > > >
> > > > > > > > (added Pali because of the Nokia RX51 issue)
> > > > > > >
> > > > > > > Could you please send me a link to "problematic" patch? As you have not
> > > > > > > inlined it in this email.
> > > > > >
> > > > > > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6
> > > > > >
> > > > > > Its a rebase of the original patch.
> > > > > >
> > > > > > > > On 07.08.20 21:21, Tom Rini wrote:
> > > > > > > > > On Thu, Jul 30, 2020 at 12:51:10PM +0200, Stefan Roese wrote:
> > > > > > > > >
> > > > > > > > > > Since commit 86cf1c82850f ("configs: Migrate CONFIG_NR_DRAM_BANKS") &
> > > > > > > > > > commit 999a772d9f24 ("Kconfig: Migrate CONFIG_NR_DRAM_BANKS"),
> > > > > > > > > > CONFIG_NR_DRAM_BANKS is always defined with a value (4 is default).
> > > > > > > > > > It makes no sense to still carry code that is guarded with
> > > > > > > > > > "#ifndef CONFIG_NR_DRAM_BANKS" (and similar). This patch removes
> > > > > > > > > > all these unreferenced code paths.
> > > > > > > > > >
> > > > > > > > > > Also complete remove bi_memstart & bi_memsize from the board-info
> > > > > > > > > > struct. As now bi_dram[] is always enabled and should be used instead.
> > > > > > > > > > This removes the redundant varriables.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Stefan Roese <[hidden email]>
> > > > > > > > > > Cc: Daniel Schwierzeck <[hidden email]>
> > > > > > > > > > Cc: Tom Rini <[hidden email]>
> > > > > > > > > > Cc: Ramon Fried <[hidden email]>
> > > > > > > > > > Cc: Simon Glass <[hidden email]>
> > > > > > > > > > Cc: Michal Simek <[hidden email]>
> > > > > > > > > > Reviewed-by: Daniel Schwierzeck <[hidden email]>
> > > > > > > > >
> > > > > > > > > I don't see quite how, but this is breaking running
> > > > > > > > > test/nokia_rx51_test.sh (or, my fixup of this to apply to current master
> > > > > > > > > was wrong, and is what's breaking the test).  Please rebase and confirm
> > > > > > > > > that test passes as well, thanks!
> > > > > > > >
> > > > > > > > I've checked the issue with nokia_rx51_test.sh and could not find any
> > > > > > > > issues in the patch. My assumption now is, that the very old Linux
> > > > > > > > kernel (2.6.28) that is used in this Nokia test, still uses the bd_info
> > > > > > > > struct in Linux to pass the memory information (via bd_memstart &
> > > > > > > > bi_memsize), as was also done in the very old PowerPC days. With this
> > > > > > > > patch now and the removal of these fields from bd_info, this might
> > > > > > > > explain why this kernel does not boot any more (no output on the serial
> > > > > > > > console at all).
> > > > > > > >
> > > > > > > > Pali, could you please check if my assumption is correct here? And if
> > > > > > > > yes, could please switch the test to using a newer kernel version? Or
> > > > > > > > remove the Linux kernel booting from the test?
> > > > > > >
> > > > > > > Yes, Nokia N900 uses "old" ATAGs. But we cannot remove it. New kernel
> > > > > > > version does not contain Maemo patches which are required for Maemo
> > > > > > > system which is still widely used. And yes, people are using it with
> > > > > > > U-Boot.
> > > > > > >
> > > > > > > Second reason why we cannot remove support for ATAGs is that Nokia's
> > > > > > > signed first stage bootloader pass other setup data via ATAGs for kernel
> > > > > > > and U-Boot N900 board code parses it, reuse it and pass to kernel.
> > > > > > >
> > > > > > > And replacing first stage bootloader is not possible because it is
> > > > > > > signed and signing keys are secret (now probably lost).
> > > > > >
> > > > > > Okay. But this patch is not removing ATAG support, but removes 2
> > > > > > member from the bd_info struct (bi_memstart & bi_memsize), which are
> > > > > > replaced (superseeded) by the bi_dram[].start/.size values. And AFAICT,
> > > > > > the generation of the memory ATAG is already done based on these
> > > > > > values:
> > > > > >
> > > > > > setup_memory_tags() in arch/arm/lib/bootm.c
> > > > > >
> > > > > > So I still fail to understand, why booting of this old kernel using
> > > > > > ATAGs does not work with this patch applied. Perhaps you could give
> > > > > > it a quick test? That would be really helpful. Here again the gitlab
> > > > > > branch that you can use for some tests:
> > > > > >
> > > > > > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commits/remove-config-nr-dram-banks-v3-2020-08-11
> > > >
> > > > I can do some tests on real N900 device at the end of week. But I doubt
> > > > that I could debug something on device if kernel does not print any
> > > > message.
> > > >
> > > > Running u-boot and kernel in N900 emulator with debug serial console is
> > > > easier and you can do it too locally. If you look at that test script
> > > > test/nokia_rx51_test.sh you could see that it downloads & compile qemu
> > > > and prepare OneNAND MTD image with compiled u-boot + kernel binary.
> > > >
> > > > You can also run that test script locally, it does not require root and
> > > > all data it stores into temporary "nokia_rx51_tmp" directory. This could
> > > > help you to test your changes without need to push them to travis/gitlab
> > > > ci testing.
> > > >
> > > > > So, I also thought maybe changing bd_t was it, and reverted just that
> > > > > part locally, and it was still breaking.  It's also only breaking in one
> > > > > of the three boot tests, if I follow things correctly.
> > > >
> > > > What about trying to *not* remove first two members of struct bd_info?
> > > > Maybe it is part of ABI structure?
> > > > https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6#688c66555c917f123c1d054aabd1c7023c6c4522_31_30
> > >
> > > Not changing the structure is what I tried and it still failed, unless I
> > > made a thinko when I was checking it out.  Can you please look in to
> > > this a bit?
> > >
> >
> > if I understand this correctly, the Maemo support in Linux parses
> > bd_info but do not use bi_memstart and bi_memsize but other member
> > variables. If we remove bi_memstart and bi_memsize, the offsets of all
> > other member variables are changing and therefore the "ABI" to Linux
> > breaks.
> >
> > If so, we can remove all usages of bi_memstart and bi_memsize like
> > Stefan did, but not the variables themselves. Maybe we should just
> > update the inline documentation for bi_memstart and bi_memsize to state
> > that them should not be used anymore and that offsets in bd_info must
> > not be changed (e.g. by removing variables or by changing variable
> > types to different sizes) to maintain ABI compatibility?
>
> I don't think we should keep bd_info around long term, let alone fix
> its format. We use devicetree now.
>
> If we want to support such old kernels on this one board, how about
> adding a Kconfig and a special header which has the file in the format
> that this one board needs? That way it shouldn't break in future, but
> we are not prevented from making future changes.

ATAGs for N900 are really required. It is not only for kernel, but also
for reading parameters from first stage bootloader which starts uboot.

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

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

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

From my past experience I know that only full-integration tests can
catch problems which unit tests or "one command line" tests in most
cases could not.
Reply | Threaded
Open this post in threaded view
|

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

Stefan Roese
In reply to this post by Daniel Schwierzeck-2
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/7196447f2e708a83c653f39c97f2439960482ad6
>>>>>
>>>>> Its a rebase of the original patch.
>>>>>
>>>>>>> On 07.08.20 21:21, Tom Rini wrote:
>>>>>>>> On Thu, Jul 30, 2020 at 12:51:10PM +0200, Stefan Roese wrote:
>>>>>>>>
>>>>>>>>> Since commit 86cf1c82850f ("configs: Migrate CONFIG_NR_DRAM_BANKS") &
>>>>>>>>> commit 999a772d9f24 ("Kconfig: Migrate CONFIG_NR_DRAM_BANKS"),
>>>>>>>>> CONFIG_NR_DRAM_BANKS is always defined with a value (4 is default).
>>>>>>>>> It makes no sense to still carry code that is guarded with
>>>>>>>>> "#ifndef CONFIG_NR_DRAM_BANKS" (and similar). This patch removes
>>>>>>>>> all these unreferenced code paths.
>>>>>>>>>
>>>>>>>>> Also complete remove bi_memstart & bi_memsize from the board-info
>>>>>>>>> struct. As now bi_dram[] is always enabled and should be used instead.
>>>>>>>>> This removes the redundant varriables.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Stefan Roese <[hidden email]>
>>>>>>>>> Cc: Daniel Schwierzeck <[hidden email]>
>>>>>>>>> Cc: Tom Rini <[hidden email]>
>>>>>>>>> Cc: Ramon Fried <[hidden email]>
>>>>>>>>> Cc: Simon Glass <[hidden email]>
>>>>>>>>> Cc: Michal Simek <[hidden email]>
>>>>>>>>> Reviewed-by: Daniel Schwierzeck <[hidden email]>
>>>>>>>>
>>>>>>>> I don't see quite how, but this is breaking running
>>>>>>>> test/nokia_rx51_test.sh (or, my fixup of this to apply to current master
>>>>>>>> was wrong, and is what's breaking the test).  Please rebase and confirm
>>>>>>>> that test passes as well, thanks!
>>>>>>>
>>>>>>> I've checked the issue with nokia_rx51_test.sh and could not find any
>>>>>>> issues in the patch. My assumption now is, that the very old Linux
>>>>>>> kernel (2.6.28) that is used in this Nokia test, still uses the bd_info
>>>>>>> struct in Linux to pass the memory information (via bd_memstart &
>>>>>>> bi_memsize), as was also done in the very old PowerPC days. With this
>>>>>>> patch now and the removal of these fields from bd_info, this might
>>>>>>> explain why this kernel does not boot any more (no output on the serial
>>>>>>> console at all).
>>>>>>>
>>>>>>> Pali, could you please check if my assumption is correct here? And if
>>>>>>> yes, could please switch the test to using a newer kernel version? Or
>>>>>>> remove the Linux kernel booting from the test?
>>>>>>
>>>>>> Yes, Nokia N900 uses "old" ATAGs. But we cannot remove it. New kernel
>>>>>> version does not contain Maemo patches which are required for Maemo
>>>>>> system which is still widely used. And yes, people are using it with
>>>>>> U-Boot.
>>>>>>
>>>>>> Second reason why we cannot remove support for ATAGs is that Nokia's
>>>>>> signed first stage bootloader pass other setup data via ATAGs for kernel
>>>>>> and U-Boot N900 board code parses it, reuse it and pass to kernel.
>>>>>>
>>>>>> And replacing first stage bootloader is not possible because it is
>>>>>> signed and signing keys are secret (now probably lost).
>>>>>
>>>>> Okay. But this patch is not removing ATAG support, but removes 2
>>>>> member from the bd_info struct (bi_memstart & bi_memsize), which are
>>>>> replaced (superseeded) by the bi_dram[].start/.size values. And AFAICT,
>>>>> the generation of the memory ATAG is already done based on these
>>>>> values:
>>>>>
>>>>> setup_memory_tags() in arch/arm/lib/bootm.c
>>>>>
>>>>> So I still fail to understand, why booting of this old kernel using
>>>>> ATAGs does not work with this patch applied. Perhaps you could give
>>>>> it a quick test? That would be really helpful. Here again the gitlab
>>>>> branch that you can use for some tests:
>>>>>
>>>>> https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commits/remove-config-nr-dram-banks-v3-2020-08-11
>>>
>>> I can do some tests on real N900 device at the end of week. But I doubt
>>> that I could debug something on device if kernel does not print any
>>> message.
>>>
>>> Running u-boot and kernel in N900 emulator with debug serial console is
>>> easier and you can do it too locally. If you look at that test script
>>> test/nokia_rx51_test.sh you could see that it downloads & compile qemu
>>> and prepare OneNAND MTD image with compiled u-boot + kernel binary.
>>>
>>> You can also run that test script locally, it does not require root and
>>> all data it stores into temporary "nokia_rx51_tmp" directory. This could
>>> help you to test your changes without need to push them to travis/gitlab
>>> ci testing.
>>>
>>>> So, I also thought maybe changing bd_t was it, and reverted just that
>>>> part locally, and it was still breaking.  It's also only breaking in one
>>>> of the three boot tests, if I follow things correctly.
>>>
>>> What about trying to *not* remove first two members of struct bd_info?
>>> Maybe it is part of ABI structure?
>>> https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6#688c66555c917f123c1d054aabd1c7023c6c4522_31_30
>>
>> Not changing the structure is what I tried and it still failed, unless I
>> made a thinko when I was checking it out.  Can you please look in to
>> this a bit?
>>
>
> if I understand this correctly, the Maemo support in Linux parses
> bd_info but do not use bi_memstart and bi_memsize but other member
> variables. If we remove bi_memstart and bi_memsize, the offsets of all
> other member variables are changing and therefore the "ABI" to Linux
> breaks.

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
Reply | Threaded
Open this post in threaded view
|

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

Stefan Roese
In reply to this post by Pali Rohár-2
Hi Pali,

On 11.08.20 18:19, Pali Rohár wrote:

> On Tuesday 11 August 2020 10:58:47 Tom Rini wrote:
>> On Tue, Aug 11, 2020 at 03:22:48PM +0200, Pali Rohár wrote:
>>> On Tuesday 11 August 2020 09:08:33 Tom Rini wrote:
>>>> On Tue, Aug 11, 2020 at 02:39:53PM +0200, Stefan Roese wrote:
>>>>> Hi Pali,
>>>>>
>>>>> On 11.08.20 10:00, Pali Rohár wrote:
>>>>>> Hello!
>>>>>>
>>>>>> On Tuesday 11 August 2020 08:37:46 Stefan Roese wrote:
>>>>>>> Hi Tom,
>>>>>>> Hi Pali,
>>>>>>>
>>>>>>> (added Pali because of the Nokia RX51 issue)
>>>>>>
>>>>>> Could you please send me a link to "problematic" patch? As you have not
>>>>>> inlined it in this email.
>>>>>
>>>>> https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6
>>>>>
>>>>> Its a rebase of the original patch.
>>>>>
>>>>>>> On 07.08.20 21:21, Tom Rini wrote:
>>>>>>>> On Thu, Jul 30, 2020 at 12:51:10PM +0200, Stefan Roese wrote:
>>>>>>>>
>>>>>>>>> Since commit 86cf1c82850f ("configs: Migrate CONFIG_NR_DRAM_BANKS") &
>>>>>>>>> commit 999a772d9f24 ("Kconfig: Migrate CONFIG_NR_DRAM_BANKS"),
>>>>>>>>> CONFIG_NR_DRAM_BANKS is always defined with a value (4 is default).
>>>>>>>>> It makes no sense to still carry code that is guarded with
>>>>>>>>> "#ifndef CONFIG_NR_DRAM_BANKS" (and similar). This patch removes
>>>>>>>>> all these unreferenced code paths.
>>>>>>>>>
>>>>>>>>> Also complete remove bi_memstart & bi_memsize from the board-info
>>>>>>>>> struct. As now bi_dram[] is always enabled and should be used instead.
>>>>>>>>> This removes the redundant varriables.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Stefan Roese <[hidden email]>
>>>>>>>>> Cc: Daniel Schwierzeck <[hidden email]>
>>>>>>>>> Cc: Tom Rini <[hidden email]>
>>>>>>>>> Cc: Ramon Fried <[hidden email]>
>>>>>>>>> Cc: Simon Glass <[hidden email]>
>>>>>>>>> Cc: Michal Simek <[hidden email]>
>>>>>>>>> Reviewed-by: Daniel Schwierzeck <[hidden email]>
>>>>>>>>
>>>>>>>> I don't see quite how, but this is breaking running
>>>>>>>> test/nokia_rx51_test.sh (or, my fixup of this to apply to current master
>>>>>>>> was wrong, and is what's breaking the test).  Please rebase and confirm
>>>>>>>> that test passes as well, thanks!
>>>>>>>
>>>>>>> I've checked the issue with nokia_rx51_test.sh and could not find any
>>>>>>> issues in the patch. My assumption now is, that the very old Linux
>>>>>>> kernel (2.6.28) that is used in this Nokia test, still uses the bd_info
>>>>>>> struct in Linux to pass the memory information (via bd_memstart &
>>>>>>> bi_memsize), as was also done in the very old PowerPC days. With this
>>>>>>> patch now and the removal of these fields from bd_info, this might
>>>>>>> explain why this kernel does not boot any more (no output on the serial
>>>>>>> console at all).
>>>>>>>
>>>>>>> Pali, could you please check if my assumption is correct here? And if
>>>>>>> yes, could please switch the test to using a newer kernel version? Or
>>>>>>> remove the Linux kernel booting from the test?
>>>>>>
>>>>>> Yes, Nokia N900 uses "old" ATAGs. But we cannot remove it. New kernel
>>>>>> version does not contain Maemo patches which are required for Maemo
>>>>>> system which is still widely used. And yes, people are using it with
>>>>>> U-Boot.
>>>>>>
>>>>>> Second reason why we cannot remove support for ATAGs is that Nokia's
>>>>>> signed first stage bootloader pass other setup data via ATAGs for kernel
>>>>>> and U-Boot N900 board code parses it, reuse it and pass to kernel.
>>>>>>
>>>>>> And replacing first stage bootloader is not possible because it is
>>>>>> signed and signing keys are secret (now probably lost).
>>>>>
>>>>> Okay. But this patch is not removing ATAG support, but removes 2
>>>>> member from the bd_info struct (bi_memstart & bi_memsize), which are
>>>>> replaced (superseeded) by the bi_dram[].start/.size values. And AFAICT,
>>>>> the generation of the memory ATAG is already done based on these
>>>>> values:
>>>>>
>>>>> setup_memory_tags() in arch/arm/lib/bootm.c
>>>>>
>>>>> So I still fail to understand, why booting of this old kernel using
>>>>> ATAGs does not work with this patch applied. Perhaps you could give
>>>>> it a quick test? That would be really helpful. Here again the gitlab
>>>>> branch that you can use for some tests:
>>>>>
>>>>> https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commits/remove-config-nr-dram-banks-v3-2020-08-11
>>>
>>> I can do some tests on real N900 device at the end of week. But I doubt
>>> that I could debug something on device if kernel does not print any
>>> message.
>>>
>>> Running u-boot and kernel in N900 emulator with debug serial console is
>>> easier and you can do it too locally. If you look at that test script
>>> test/nokia_rx51_test.sh you could see that it downloads & compile qemu
>>> and prepare OneNAND MTD image with compiled u-boot + kernel binary.
>>>
>>> You can also run that test script locally, it does not require root and
>>> all data it stores into temporary "nokia_rx51_tmp" directory. This could
>>> help you to test your changes without need to push them to travis/gitlab
>>> ci testing.
>>>
>>>> So, I also thought maybe changing bd_t was it, and reverted just that
>>>> part locally, and it was still breaking.  It's also only breaking in one
>>>> of the three boot tests, if I follow things correctly.
>>>
>>> What about trying to *not* remove first two members of struct bd_info?
>>> Maybe it is part of ABI structure?
>>> https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6#688c66555c917f123c1d054aabd1c7023c6c4522_31_30
>>
>> Not changing the structure is what I tried and it still failed, unless I
>> made a thinko when I was checking it out.  Can you please look in to
>> this a bit?
>
> Yes, I looked at that patch and seems it is really breaking something
> related to dram banks.
>
> I applied following debug patch
>
> diff --git a/common/board_f.c b/common/board_f.c
> index 51e9544b8e..ae6d37b574 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -9,6 +9,8 @@
>    * Marius Groeger <[hidden email]>
>    */
>  
> +#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
Reply | Threaded
Open this post in threaded view
|

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

Stefan Roese
In reply to this post by Pali Rohár-2
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
>> <[hidden email]> wrote:
>>>
>>> Am Dienstag, den 11.08.2020, 10:58 -0400 schrieb Tom Rini:
>>>> On Tue, Aug 11, 2020 at 03:22:48PM +0200, Pali Rohár wrote:
>>>>> On Tuesday 11 August 2020 09:08:33 Tom Rini wrote:
>>>>>> On Tue, Aug 11, 2020 at 02:39:53PM +0200, Stefan Roese wrote:
>>>>>>> Hi Pali,
>>>>>>>
>>>>>>> On 11.08.20 10:00, Pali Rohár wrote:
>>>>>>>> Hello!
>>>>>>>>
>>>>>>>> On Tuesday 11 August 2020 08:37:46 Stefan Roese wrote:
>>>>>>>>> Hi Tom,
>>>>>>>>> Hi Pali,
>>>>>>>>>
>>>>>>>>> (added Pali because of the Nokia RX51 issue)
>>>>>>>>
>>>>>>>> Could you please send me a link to "problematic" patch? As you have not
>>>>>>>> inlined it in this email.
>>>>>>>
>>>>>>> https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6
>>>>>>>
>>>>>>> Its a rebase of the original patch.
>>>>>>>
>>>>>>>>> On 07.08.20 21:21, Tom Rini wrote:
>>>>>>>>>> On Thu, Jul 30, 2020 at 12:51:10PM +0200, Stefan Roese wrote:
>>>>>>>>>>
>>>>>>>>>>> Since commit 86cf1c82850f ("configs: Migrate CONFIG_NR_DRAM_BANKS") &
>>>>>>>>>>> commit 999a772d9f24 ("Kconfig: Migrate CONFIG_NR_DRAM_BANKS"),
>>>>>>>>>>> CONFIG_NR_DRAM_BANKS is always defined with a value (4 is default).
>>>>>>>>>>> It makes no sense to still carry code that is guarded with
>>>>>>>>>>> "#ifndef CONFIG_NR_DRAM_BANKS" (and similar). This patch removes
>>>>>>>>>>> all these unreferenced code paths.
>>>>>>>>>>>
>>>>>>>>>>> Also complete remove bi_memstart & bi_memsize from the board-info
>>>>>>>>>>> struct. As now bi_dram[] is always enabled and should be used instead.
>>>>>>>>>>> This removes the redundant varriables.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Stefan Roese <[hidden email]>
>>>>>>>>>>> Cc: Daniel Schwierzeck <[hidden email]>
>>>>>>>>>>> Cc: Tom Rini <[hidden email]>
>>>>>>>>>>> Cc: Ramon Fried <[hidden email]>
>>>>>>>>>>> Cc: Simon Glass <[hidden email]>
>>>>>>>>>>> Cc: Michal Simek <[hidden email]>
>>>>>>>>>>> Reviewed-by: Daniel Schwierzeck <[hidden email]>
>>>>>>>>>>
>>>>>>>>>> I don't see quite how, but this is breaking running
>>>>>>>>>> test/nokia_rx51_test.sh (or, my fixup of this to apply to current master
>>>>>>>>>> was wrong, and is what's breaking the test).  Please rebase and confirm
>>>>>>>>>> that test passes as well, thanks!
>>>>>>>>>
>>>>>>>>> I've checked the issue with nokia_rx51_test.sh and could not find any
>>>>>>>>> issues in the patch. My assumption now is, that the very old Linux
>>>>>>>>> kernel (2.6.28) that is used in this Nokia test, still uses the bd_info
>>>>>>>>> struct in Linux to pass the memory information (via bd_memstart &
>>>>>>>>> bi_memsize), as was also done in the very old PowerPC days. With this
>>>>>>>>> patch now and the removal of these fields from bd_info, this might
>>>>>>>>> explain why this kernel does not boot any more (no output on the serial
>>>>>>>>> console at all).
>>>>>>>>>
>>>>>>>>> Pali, could you please check if my assumption is correct here? And if
>>>>>>>>> yes, could please switch the test to using a newer kernel version? Or
>>>>>>>>> remove the Linux kernel booting from the test?
>>>>>>>>
>>>>>>>> Yes, Nokia N900 uses "old" ATAGs. But we cannot remove it. New kernel
>>>>>>>> version does not contain Maemo patches which are required for Maemo
>>>>>>>> system which is still widely used. And yes, people are using it with
>>>>>>>> U-Boot.
>>>>>>>>
>>>>>>>> Second reason why we cannot remove support for ATAGs is that Nokia's
>>>>>>>> signed first stage bootloader pass other setup data via ATAGs for kernel
>>>>>>>> and U-Boot N900 board code parses it, reuse it and pass to kernel.
>>>>>>>>
>>>>>>>> And replacing first stage bootloader is not possible because it is
>>>>>>>> signed and signing keys are secret (now probably lost).
>>>>>>>
>>>>>>> Okay. But this patch is not removing ATAG support, but removes 2
>>>>>>> member from the bd_info struct (bi_memstart & bi_memsize), which are
>>>>>>> replaced (superseeded) by the bi_dram[].start/.size values. And AFAICT,
>>>>>>> the generation of the memory ATAG is already done based on these
>>>>>>> values:
>>>>>>>
>>>>>>> setup_memory_tags() in arch/arm/lib/bootm.c
>>>>>>>
>>>>>>> So I still fail to understand, why booting of this old kernel using
>>>>>>> ATAGs does not work with this patch applied. Perhaps you could give
>>>>>>> it a quick test? That would be really helpful. Here again the gitlab
>>>>>>> branch that you can use for some tests:
>>>>>>>
>>>>>>> https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commits/remove-config-nr-dram-banks-v3-2020-08-11
>>>>>
>>>>> I can do some tests on real N900 device at the end of week. But I doubt
>>>>> that I could debug something on device if kernel does not print any
>>>>> message.
>>>>>
>>>>> Running u-boot and kernel in N900 emulator with debug serial console is
>>>>> easier and you can do it too locally. If you look at that test script
>>>>> test/nokia_rx51_test.sh you could see that it downloads & compile qemu
>>>>> and prepare OneNAND MTD image with compiled u-boot + kernel binary.
>>>>>
>>>>> You can also run that test script locally, it does not require root and
>>>>> all data it stores into temporary "nokia_rx51_tmp" directory. This could
>>>>> help you to test your changes without need to push them to travis/gitlab
>>>>> ci testing.
>>>>>
>>>>>> So, I also thought maybe changing bd_t was it, and reverted just that
>>>>>> part locally, and it was still breaking.  It's also only breaking in one
>>>>>> of the three boot tests, if I follow things correctly.
>>>>>
>>>>> What about trying to *not* remove first two members of struct bd_info?
>>>>> Maybe it is part of ABI structure?
>>>>> https://gitlab.denx.de/u-boot/custodians/u-boot-marvell/-/commit/7196447f2e708a83c653f39c97f2439960482ad6#688c66555c917f123c1d054aabd1c7023c6c4522_31_30
>>>>
>>>> Not changing the structure is what I tried and it still failed, unless I
>>>> made a thinko when I was checking it out.  Can you please look in to
>>>> this a bit?
>>>>
>>>
>>> if I understand this correctly, the Maemo support in Linux parses
>>> bd_info but do not use bi_memstart and bi_memsize but other member
>>> variables. If we remove bi_memstart and bi_memsize, the offsets of all
>>> other member variables are changing and therefore the "ABI" to Linux
>>> breaks.
>>>
>>> If so, we can remove all usages of bi_memstart and bi_memsize like
>>> Stefan did, but not the variables themselves. Maybe we should just
>>> update the inline documentation for bi_memstart and bi_memsize to state
>>> that them should not be used anymore and that offsets in bd_info must
>>> not be changed (e.g. by removing variables or by changing variable
>>> types to different sizes) to maintain ABI compatibility?
>>
>> I don't think we should keep bd_info around long term, let alone fix
>> its format. We use devicetree now.
>>
>> If we want to support such old kernels on this one board, how about
>> adding a Kconfig and a special header which has the file in the format
>> that this one board needs? That way it shouldn't break in future, but
>> we are not prevented from making future changes.
>
> ATAGs for N900 are really required. It is not only for kernel, but also
> for reading parameters from first stage bootloader which starts uboot.

Just to make is clear: ATAGs are *not* removed with this patch. Its
"just" a change to the bd_info struct.

Thanks,
Stefan