[U-Boot] [PATCH v3 0/3] Add generic FDT memory bank decoding and gd initialization

Currently most boards that have static memory config (which cannot be detected automatically) use CONFIG_SYS_SDRAM_SIZE to define the size of memory which is hard coded into U-Boot. With the addition of device tree support into U-Boot, boards that have device tree data can instead query the memory bank information from the embedded or appended device tree. This allows for more dynamic memory configuration and avoids the need to hard code memory configuration into U-Boot.
The first patch of this series adds two helper functions for handling the memory bank decoding and initialization of global data which can be used by boards in their dram_init and dram_init_banksize functions. The purpose of these helper functions is to provide generic functions that handle the decoding and setup of memory and memory banks for boards that intend to use this mechanism.
The series also changes the zynq and zynqmp board implementations to use these functions to resolve a issue with static variable use.
Changes in v2: * Make fdtdec_setup_memory_banksize() return value consistent * Add more detail into the function documentation
Changes in v3: * Wrap the fdtdec_setup_memory_banksize() function with an #if on CONFIG_NR_DRAM_BANKS, so that the function is only available for configs that have memory banks available in bd_t.
Nathan Rossi (3): fdt: add memory bank decoding functions for board setup ARM: zynq: Replace board specific with generic memory bank decoding ARM64: zynqmp: Replace board specific with generic memory bank decoding
board/xilinx/zynq/board.c | 112 ++----------------------------------------- board/xilinx/zynqmp/zynqmp.c | 112 ++----------------------------------------- include/fdtdec.h | 34 +++++++++++++ lib/fdtdec.c | 56 ++++++++++++++++++++++ 4 files changed, 96 insertions(+), 218 deletions(-)

Add two functions for use by board implementations to decode the memory banks of the /memory node so as to populate the global data with ram_size and board info for memory banks.
The fdtdec_setup_memory_size() function decodes the first memory bank and sets up the gd->ram_size with the size of the memory bank. This function should be called from the boards dram_init().
The fdtdec_setup_memory_banksize() function decode the memory banks (up to the CONFIG_NR_DRAM_BANKS) and populates the base address and size into the gd->bd->bi_dram array of banks. This function should be called from the boards dram_init_banksize().
Signed-off-by: Nathan Rossi nathan@nathanrossi.com Cc: Simon Glass sjg@chromium.org Cc: Michal Simek monstr@monstr.eu Reviewed-by: Simon Glass sjg@chromium.org --- v2: * Make fdtdec_setup_memory_banksize() return value consistent * Add more detail into the function documentation v3: * Wrap the fdtdec_setup_memory_banksize() function with an #if on CONFIG_NR_DRAM_BANKS, so that the function is only available for configs that have memory banks available in bd_t.
This implementation of decoding has been tested on zynq and zynqmp boards with address/size cells of (1, 1), (1, 2), (2, 1), (2, 2) and up to 2 memory banks. --- include/fdtdec.h | 34 ++++++++++++++++++++++++++++++++++ lib/fdtdec.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+)
diff --git a/include/fdtdec.h b/include/fdtdec.h index 27887c8c21..d074478f14 100644 --- a/include/fdtdec.h +++ b/include/fdtdec.h @@ -976,6 +976,40 @@ struct display_timing { */ int fdtdec_decode_display_timing(const void *blob, int node, int index, struct display_timing *config); + +/** + * fdtdec_setup_memory_size() - decode and setup gd->ram_size + * + * Decode the /memory 'reg' property to determine the size of the first memory + * bank, populate the global data with the size of the first bank of memory. + * + * This function should be called from a boards dram_init(). This helper + * function allows for boards to query the device tree for DRAM size instead of + * hard coding the value in the case where the memory size cannot be detected + * automatically. + * + * @return 0 if OK, -EINVAL if the /memory node or reg property is missing or + * invalid + */ +int fdtdec_setup_memory_size(void); + +/** + * fdtdec_setup_memory_banksize() - decode and populate gd->bd->bi_dram + * + * Decode the /memory 'reg' property to determine the address and size of the + * memory banks. Use this data to populate the global data board info with the + * phys address and size of memory banks. + * + * This function should be called from a boards dram_init_banksize(). This + * helper function allows for boards to query the device tree for memory bank + * information instead of hard coding the information in cases where it cannot + * be detected automatically. + * + * @return 0 if OK, -EINVAL if the /memory node or reg property is missing or + * invalid + */ +int fdtdec_setup_memory_banksize(void); + /** * Set up the device tree ready for use */ diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 4e619c49a2..81f47ef2c7 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1174,6 +1174,62 @@ int fdtdec_decode_display_timing(const void *blob, int parent, int index, return ret; }
+int fdtdec_setup_memory_size(void) +{ + int ret, mem; + struct fdt_resource res; + + mem = fdt_path_offset(gd->fdt_blob, "/memory"); + if (mem < 0) { + debug("%s: Missing /memory node\n", __func__); + return -EINVAL; + } + + ret = fdt_get_resource(gd->fdt_blob, mem, "reg", 0, &res); + if (ret != 0) { + debug("%s: Unable to decode first memory bank\n", __func__); + return -EINVAL; + } + + gd->ram_size = (phys_size_t)(res.end - res.start + 1); + debug("%s: Initial DRAM size %llx\n", __func__, (u64)gd->ram_size); + + return 0; +} + +#if defined(CONFIG_NR_DRAM_BANKS) +int fdtdec_setup_memory_banksize(void) +{ + int bank, ret, mem; + struct fdt_resource res; + + mem = fdt_path_offset(gd->fdt_blob, "/memory"); + if (mem < 0) { + debug("%s: Missing /memory node\n", __func__); + return -EINVAL; + } + + for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) { + ret = fdt_get_resource(gd->fdt_blob, mem, "reg", bank, &res); + if (ret == -FDT_ERR_NOTFOUND) + break; + if (ret != 0) + return -EINVAL; + + gd->bd->bi_dram[bank].start = (phys_addr_t)res.start; + gd->bd->bi_dram[bank].size = + (phys_size_t)(res.end - res.start + 1); + + debug("%s: DRAM Bank #%d: start = 0x%llx, size = 0x%llx\n", + __func__, bank, + (unsigned long long)gd->bd->bi_dram[bank].start, + (unsigned long long)gd->bd->bi_dram[bank].size); + } + + return 0; +} +#endif + int fdtdec_setup(void) { #if CONFIG_IS_ENABLED(OF_CONTROL)

The dram_init and dram_init_banksize functions were using a board specific implementation for decoding the memory banks from the fdt. This board specific implementation uses a static variable 'tmp' which makes these functions unsafe for execution from within the board_init_f context.
This unsafe use of a static variable was causing a specific bug when using the zynq_zybo configuration, U-Boot would generate the following error during image load. This was caused due to dram_init overwriting the relocations for the 'image' variable within the do_bootm function. Out of coincidence the un-initialized memory has a compression type which is the same as the value for the relocation type R_ARM_RELATIVE.
Uncompressing Invalid Image ... Unimplemented compression type 23
It should be noted that this is just one way the issue could surface, other cases my not be observed in normal boot flow. Depending on the size of various sections, and location of relocations within __rel_dyn and the compiler/linker the outcome of this bug can differ greatly.
This change makes the dram_init* functions use a generic implementation of decoding and populating memory bank and size data.
Signed-off-by: Nathan Rossi nathan@nathanrossi.com Fixes: 758f29d0f8 ("ARM: zynq: Support systems with more memory banks") Cc: Michal Simek monstr@monstr.eu --- board/xilinx/zynq/board.c | 112 ++-------------------------------------------- 1 file changed, 3 insertions(+), 109 deletions(-)
diff --git a/board/xilinx/zynq/board.c b/board/xilinx/zynq/board.c index 2c86940957..5cd9bbf711 100644 --- a/board/xilinx/zynq/board.c +++ b/board/xilinx/zynq/board.c @@ -124,121 +124,15 @@ int zynq_board_read_rom_ethaddr(unsigned char *ethaddr) }
#if !defined(CONFIG_SYS_SDRAM_BASE) && !defined(CONFIG_SYS_SDRAM_SIZE) -/* - * fdt_get_reg - Fill buffer by information from DT - */ -static phys_size_t fdt_get_reg(const void *fdt, int nodeoffset, void *buf, - const u32 *cell, int n) -{ - int i = 0, b, banks; - int parent_offset = fdt_parent_offset(fdt, nodeoffset); - int address_cells = fdt_address_cells(fdt, parent_offset); - int size_cells = fdt_size_cells(fdt, parent_offset); - char *p = buf; - u64 val; - u64 vals; - - debug("%s: addr_cells=%x, size_cell=%x, buf=%p, cell=%p\n", - __func__, address_cells, size_cells, buf, cell); - - /* Check memory bank setup */ - banks = n % (address_cells + size_cells); - if (banks) - panic("Incorrect memory setup cells=%d, ac=%d, sc=%d\n", - n, address_cells, size_cells); - - banks = n / (address_cells + size_cells); - - for (b = 0; b < banks; b++) { - debug("%s: Bank #%d:\n", __func__, b); - if (address_cells == 2) { - val = cell[i + 1]; - val <<= 32; - val |= cell[i]; - val = fdt64_to_cpu(val); - debug("%s: addr64=%llx, ptr=%p, cell=%p\n", - __func__, val, p, &cell[i]); - *(phys_addr_t *)p = val; - } else { - debug("%s: addr32=%x, ptr=%p\n", - __func__, fdt32_to_cpu(cell[i]), p); - *(phys_addr_t *)p = fdt32_to_cpu(cell[i]); - } - p += sizeof(phys_addr_t); - i += address_cells; - - debug("%s: pa=%p, i=%x, size=%zu\n", __func__, p, i, - sizeof(phys_addr_t)); - - if (size_cells == 2) { - vals = cell[i + 1]; - vals <<= 32; - vals |= cell[i]; - vals = fdt64_to_cpu(vals); - - debug("%s: size64=%llx, ptr=%p, cell=%p\n", - __func__, vals, p, &cell[i]); - *(phys_size_t *)p = vals; - } else { - debug("%s: size32=%x, ptr=%p\n", - __func__, fdt32_to_cpu(cell[i]), p); - *(phys_size_t *)p = fdt32_to_cpu(cell[i]); - } - p += sizeof(phys_size_t); - i += size_cells; - - debug("%s: ps=%p, i=%x, size=%zu\n", - __func__, p, i, sizeof(phys_size_t)); - } - - /* Return the first address size */ - return *(phys_size_t *)((char *)buf + sizeof(phys_addr_t)); -} - -#define FDT_REG_SIZE sizeof(u32) -/* Temp location for sharing data for storing */ -/* Up to 64-bit address + 64-bit size */ -static u8 tmp[CONFIG_NR_DRAM_BANKS * 16]; - void dram_init_banksize(void) { - int bank; - - memcpy(&gd->bd->bi_dram[0], &tmp, sizeof(tmp)); - - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) { - debug("Bank #%d: start %llx\n", bank, - (unsigned long long)gd->bd->bi_dram[bank].start); - debug("Bank #%d: size %llx\n", bank, - (unsigned long long)gd->bd->bi_dram[bank].size); - } + fdtdec_setup_memory_banksize(); }
int dram_init(void) { - int node, len; - const void *blob = gd->fdt_blob; - const u32 *cell; - - memset(&tmp, 0, sizeof(tmp)); - - /* find or create "/memory" node. */ - node = fdt_subnode_offset(blob, 0, "memory"); - if (node < 0) { - printf("%s: Can't get memory node\n", __func__); - return node; - } - - /* Get pointer to cells and lenght of it */ - cell = fdt_getprop(blob, node, "reg", &len); - if (!cell) { - printf("%s: Can't get reg property\n", __func__); - return -1; - } - - gd->ram_size = fdt_get_reg(blob, node, &tmp, cell, len / FDT_REG_SIZE); - - debug("%s: Initial DRAM size %llx\n", __func__, (u64)gd->ram_size); + if (fdtdec_setup_memory_size() != 0) + return -EINVAL;
zynq_ddrc_init();

The dram_init and dram_init_banksize functions were using a board specific implementation for decoding the memory banks from the fdt. This board specific implementation uses a static variable 'tmp' which makes these functions unsafe for execution from within the board_init_f context.
This change makes the dram_init* functions use a generic implementation of decoding and populating memory bank and size data.
Signed-off-by: Nathan Rossi nathan@nathanrossi.com Fixes: 8d59d7f63b ("ARM64: zynqmp: Read RAM information from DT") Cc: Michal Simek michal.simek@xilinx.com --- board/xilinx/zynqmp/zynqmp.c | 112 ++----------------------------------------- 1 file changed, 3 insertions(+), 109 deletions(-)
diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c index cef1f6a13a..8a3d0043b9 100644 --- a/board/xilinx/zynqmp/zynqmp.c +++ b/board/xilinx/zynqmp/zynqmp.c @@ -180,121 +180,15 @@ int zynq_board_read_rom_ethaddr(unsigned char *ethaddr) }
#if !defined(CONFIG_SYS_SDRAM_BASE) && !defined(CONFIG_SYS_SDRAM_SIZE) -/* - * fdt_get_reg - Fill buffer by information from DT - */ -static phys_size_t fdt_get_reg(const void *fdt, int nodeoffset, void *buf, - const u32 *cell, int n) -{ - int i = 0, b, banks; - int parent_offset = fdt_parent_offset(fdt, nodeoffset); - int address_cells = fdt_address_cells(fdt, parent_offset); - int size_cells = fdt_size_cells(fdt, parent_offset); - char *p = buf; - u64 val; - u64 vals; - - debug("%s: addr_cells=%x, size_cell=%x, buf=%p, cell=%p\n", - __func__, address_cells, size_cells, buf, cell); - - /* Check memory bank setup */ - banks = n % (address_cells + size_cells); - if (banks) - panic("Incorrect memory setup cells=%d, ac=%d, sc=%d\n", - n, address_cells, size_cells); - - banks = n / (address_cells + size_cells); - - for (b = 0; b < banks; b++) { - debug("%s: Bank #%d:\n", __func__, b); - if (address_cells == 2) { - val = cell[i + 1]; - val <<= 32; - val |= cell[i]; - val = fdt64_to_cpu(val); - debug("%s: addr64=%llx, ptr=%p, cell=%p\n", - __func__, val, p, &cell[i]); - *(phys_addr_t *)p = val; - } else { - debug("%s: addr32=%x, ptr=%p\n", - __func__, fdt32_to_cpu(cell[i]), p); - *(phys_addr_t *)p = fdt32_to_cpu(cell[i]); - } - p += sizeof(phys_addr_t); - i += address_cells; - - debug("%s: pa=%p, i=%x, size=%zu\n", __func__, p, i, - sizeof(phys_addr_t)); - - if (size_cells == 2) { - vals = cell[i + 1]; - vals <<= 32; - vals |= cell[i]; - vals = fdt64_to_cpu(vals); - - debug("%s: size64=%llx, ptr=%p, cell=%p\n", - __func__, vals, p, &cell[i]); - *(phys_size_t *)p = vals; - } else { - debug("%s: size32=%x, ptr=%p\n", - __func__, fdt32_to_cpu(cell[i]), p); - *(phys_size_t *)p = fdt32_to_cpu(cell[i]); - } - p += sizeof(phys_size_t); - i += size_cells; - - debug("%s: ps=%p, i=%x, size=%zu\n", - __func__, p, i, sizeof(phys_size_t)); - } - - /* Return the first address size */ - return *(phys_size_t *)((char *)buf + sizeof(phys_addr_t)); -} - -#define FDT_REG_SIZE sizeof(u32) -/* Temp location for sharing data for storing */ -/* Up to 64-bit address + 64-bit size */ -static u8 tmp[CONFIG_NR_DRAM_BANKS * 16]; - void dram_init_banksize(void) { - int bank; - - memcpy(&gd->bd->bi_dram[0], &tmp, sizeof(tmp)); - - for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) { - debug("Bank #%d: start %llx\n", bank, - (unsigned long long)gd->bd->bi_dram[bank].start); - debug("Bank #%d: size %llx\n", bank, - (unsigned long long)gd->bd->bi_dram[bank].size); - } + fdtdec_setup_memory_banksize(); }
int dram_init(void) { - int node, len; - const void *blob = gd->fdt_blob; - const u32 *cell; - - memset(&tmp, 0, sizeof(tmp)); - - /* find or create "/memory" node. */ - node = fdt_subnode_offset(blob, 0, "memory"); - if (node < 0) { - printf("%s: Can't get memory node\n", __func__); - return node; - } - - /* Get pointer to cells and lenght of it */ - cell = fdt_getprop(blob, node, "reg", &len); - if (!cell) { - printf("%s: Can't get reg property\n", __func__); - return -1; - } - - gd->ram_size = fdt_get_reg(blob, node, &tmp, cell, len / FDT_REG_SIZE); - - debug("%s: Initial DRAM size %llx\n", __func__, (u64)gd->ram_size); + if (fdtdec_setup_memory_size() != 0) + return -EINVAL;
return 0; }

On 18.12.2016 15:03, Nathan Rossi wrote:
The dram_init and dram_init_banksize functions were using a board specific implementation for decoding the memory banks from the fdt. This board specific implementation uses a static variable 'tmp' which makes these functions unsafe for execution from within the board_init_f context.
This change makes the dram_init* functions use a generic implementation of decoding and populating memory bank and size data.
Signed-off-by: Nathan Rossi nathan@nathanrossi.com Fixes: 8d59d7f63b ("ARM64: zynqmp: Read RAM information from DT") Cc: Michal Simek michal.simek@xilinx.com
board/xilinx/zynqmp/zynqmp.c | 112 ++----------------------------------------- 1 file changed, 3 insertions(+), 109 deletions(-)
diff --git a/board/xilinx/zynqmp/zynqmp.c b/board/xilinx/zynqmp/zynqmp.c index cef1f6a13a..8a3d0043b9 100644 --- a/board/xilinx/zynqmp/zynqmp.c +++ b/board/xilinx/zynqmp/zynqmp.c @@ -180,121 +180,15 @@ int zynq_board_read_rom_ethaddr(unsigned char *ethaddr) }
#if !defined(CONFIG_SYS_SDRAM_BASE) && !defined(CONFIG_SYS_SDRAM_SIZE) -/*
- fdt_get_reg - Fill buffer by information from DT
- */
-static phys_size_t fdt_get_reg(const void *fdt, int nodeoffset, void *buf,
const u32 *cell, int n)
-{
- int i = 0, b, banks;
- int parent_offset = fdt_parent_offset(fdt, nodeoffset);
- int address_cells = fdt_address_cells(fdt, parent_offset);
- int size_cells = fdt_size_cells(fdt, parent_offset);
- char *p = buf;
- u64 val;
- u64 vals;
- debug("%s: addr_cells=%x, size_cell=%x, buf=%p, cell=%p\n",
__func__, address_cells, size_cells, buf, cell);
- /* Check memory bank setup */
- banks = n % (address_cells + size_cells);
- if (banks)
panic("Incorrect memory setup cells=%d, ac=%d, sc=%d\n",
n, address_cells, size_cells);
- banks = n / (address_cells + size_cells);
- for (b = 0; b < banks; b++) {
debug("%s: Bank #%d:\n", __func__, b);
if (address_cells == 2) {
val = cell[i + 1];
val <<= 32;
val |= cell[i];
val = fdt64_to_cpu(val);
debug("%s: addr64=%llx, ptr=%p, cell=%p\n",
__func__, val, p, &cell[i]);
*(phys_addr_t *)p = val;
} else {
debug("%s: addr32=%x, ptr=%p\n",
__func__, fdt32_to_cpu(cell[i]), p);
*(phys_addr_t *)p = fdt32_to_cpu(cell[i]);
}
p += sizeof(phys_addr_t);
i += address_cells;
debug("%s: pa=%p, i=%x, size=%zu\n", __func__, p, i,
sizeof(phys_addr_t));
if (size_cells == 2) {
vals = cell[i + 1];
vals <<= 32;
vals |= cell[i];
vals = fdt64_to_cpu(vals);
debug("%s: size64=%llx, ptr=%p, cell=%p\n",
__func__, vals, p, &cell[i]);
*(phys_size_t *)p = vals;
} else {
debug("%s: size32=%x, ptr=%p\n",
__func__, fdt32_to_cpu(cell[i]), p);
*(phys_size_t *)p = fdt32_to_cpu(cell[i]);
}
p += sizeof(phys_size_t);
i += size_cells;
debug("%s: ps=%p, i=%x, size=%zu\n",
__func__, p, i, sizeof(phys_size_t));
- }
- /* Return the first address size */
- return *(phys_size_t *)((char *)buf + sizeof(phys_addr_t));
-}
-#define FDT_REG_SIZE sizeof(u32) -/* Temp location for sharing data for storing */ -/* Up to 64-bit address + 64-bit size */ -static u8 tmp[CONFIG_NR_DRAM_BANKS * 16];
void dram_init_banksize(void) {
- int bank;
- memcpy(&gd->bd->bi_dram[0], &tmp, sizeof(tmp));
- for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
debug("Bank #%d: start %llx\n", bank,
(unsigned long long)gd->bd->bi_dram[bank].start);
debug("Bank #%d: size %llx\n", bank,
(unsigned long long)gd->bd->bi_dram[bank].size);
- }
- fdtdec_setup_memory_banksize();
}
int dram_init(void) {
- int node, len;
- const void *blob = gd->fdt_blob;
- const u32 *cell;
- memset(&tmp, 0, sizeof(tmp));
- /* find or create "/memory" node. */
- node = fdt_subnode_offset(blob, 0, "memory");
- if (node < 0) {
printf("%s: Can't get memory node\n", __func__);
return node;
- }
- /* Get pointer to cells and lenght of it */
- cell = fdt_getprop(blob, node, "reg", &len);
- if (!cell) {
printf("%s: Can't get reg property\n", __func__);
return -1;
- }
- gd->ram_size = fdt_get_reg(blob, node, &tmp, cell, len / FDT_REG_SIZE);
- debug("%s: Initial DRAM size %llx\n", __func__, (u64)gd->ram_size);
if (fdtdec_setup_memory_size() != 0)
return -EINVAL;
return 0;
}
I have tested it on zynq and zynqmp platforms and all looks good. https://travis-ci.org/michalsimek-test/u-boot/builds/185160761 Build for others look good too.
Applied all to my tree
Thanks, Michal
participants (2)
-
Michal Simek
-
Nathan Rossi