[U-Boot] [PATCH 0/3] Add generic FDT memory bank decoding and gd initialization
This series adds two functions for handling the memory bank decoding and initialization of global data for use by boards in their dram_init and dram_init_banksize functions.
The series also changes the zynq and zynqmp board implementations to use these functions to resolve a issue with static variable use.
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 | 25 ++++++++++ lib/fdtdec.c | 54 +++++++++++++++++++++ 4 files changed, 85 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 --- 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 | 25 +++++++++++++++++++++++++ lib/fdtdec.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+)
diff --git a/include/fdtdec.h b/include/fdtdec.h index 27887c8c21..59a204b571 100644 --- a/include/fdtdec.h +++ b/include/fdtdec.h @@ -976,6 +976,31 @@ 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 the boards dram_init(). + * + * @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 + * the boards dram_init_banksize(). + * + * @return 0 if OK, negative on error + */ +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..bc3be017b6 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1174,6 +1174,60 @@ 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; +} + +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 ret; + + 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; +} + int fdtdec_setup(void) { #if CONFIG_IS_ENABLED(OF_CONTROL)
Hi Nathan,
On 11 December 2016 at 08:58, Nathan Rossi nathan@nathanrossi.com wrote:
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
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 | 25 +++++++++++++++++++++++++ lib/fdtdec.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
Please see nit below.
diff --git a/include/fdtdec.h b/include/fdtdec.h index 27887c8c21..59a204b571 100644 --- a/include/fdtdec.h +++ b/include/fdtdec.h @@ -976,6 +976,31 @@ 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 the boards dram_init().
- @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
- the boards dram_init_banksize().
- @return 0 if OK, negative on error
Good to be specific, if e.g. it can only return -EINVAL.
- */
+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..bc3be017b6 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1174,6 +1174,60 @@ 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;+}
+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 ret;
The return above return -EINVAL, but this one returns a -FDT_ERR_... which is different. So my suggestion here would be to return -EINVAL here, unless you want to change the function to always return an FDT error (although fdtdec_decode_memory_region() returns an errno error so perhaps it is better to be consistent with that).
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;+}
int fdtdec_setup(void) {
#if CONFIG_IS_ENABLED(OF_CONTROL)
2.10.2
Regards, Simon
On 12 December 2016 at 06:27, Simon Glass sjg@chromium.org wrote:
Hi Nathan,
On 11 December 2016 at 08:58, Nathan Rossi nathan@nathanrossi.com wrote:
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
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 | 25 +++++++++++++++++++++++++ lib/fdtdec.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
Please see nit below.
diff --git a/include/fdtdec.h b/include/fdtdec.h index 27887c8c21..59a204b571 100644 --- a/include/fdtdec.h +++ b/include/fdtdec.h @@ -976,6 +976,31 @@ 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 the boards dram_init().
- @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
- the boards dram_init_banksize().
- @return 0 if OK, negative on error
Good to be specific, if e.g. it can only return -EINVAL.
I will update this based on the change below.
- */
+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..bc3be017b6 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1174,6 +1174,60 @@ 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;+}
+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 ret;The return above return -EINVAL, but this one returns a -FDT_ERR_... which is different. So my suggestion here would be to return -EINVAL here, unless you want to change the function to always return an FDT error (although fdtdec_decode_memory_region() returns an errno error so perhaps it is better to be consistent with that).
Agreed, returning only -EINVAL in both error conditions makes more sense than returning an -FDT_ERR_* error. This also makes it consistent with the function above this function. Will send out v2.
Regards, Nathan
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; }
Hi Nathan,
On 12/11/16 15:58, Nathan Rossi wrote:
This series adds two functions for handling the memory bank decoding and initialization of global data for use by boards in their dram_init and dram_init_banksize functions.
I might have missed some discussions on this meter, can you please provide the use cases for this? IMO, the bootloader's job is to initialize the DRAM, detect its size, and pass the detected DRAM configuration on to an OS.
The series also changes the zynq and zynqmp board implementations to use these functions to resolve a issue with static variable use.
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 | 25 ++++++++++ lib/fdtdec.c | 54 +++++++++++++++++++++ 4 files changed, 85 insertions(+), 218 deletions(-)
On 12 December 2016 at 01:08, Igor Grinberg grinberg@compulab.co.il wrote:
Hi Nathan,
On 12/11/16 15:58, Nathan Rossi wrote:
This series adds two functions for handling the memory bank decoding and initialization of global data for use by boards in their dram_init and dram_init_banksize functions.
I might have missed some discussions on this meter, can you please provide the use cases for this? IMO, the bootloader's job is to initialize the DRAM, detect its size, and pass the detected DRAM configuration on to an OS.
Hi Igor,
I do not think there have been any discussions on this (at least none that I am aware of).
Some boards (like Zynq and ZynqMP ones) are using CONFIG_SYS_SDRAM_SIZE to define the amount of memory that is available (since detection is not possible). However with the introduction of dtbs for some boards they are also capable of loading the size of memory from the embedded/appended dtb (instead of hardcoded). This allows for more of the board config to be loaded from the device tree instead of from include/configs/*.h. This however is up to the individual board to implement in its dram_init* functions.
The first patch of the series is only adding some decoding helper functions to make this generic between the Zynq and ZynqMP boards as well as to allow for any other boards that may want to use the same mechanism to get the memory size from the fdt. There is no requirement for boards to use these functions.
Regards, Nathan
The series also changes the zynq and zynqmp board implementations to use these functions to resolve a issue with static variable use.
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 | 25 ++++++++++ lib/fdtdec.c | 54 +++++++++++++++++++++ 4 files changed, 85 insertions(+), 218 deletions(-)
-- Regards, Igor.
On 12/11/16 18:47, Nathan Rossi wrote:
On 12 December 2016 at 01:08, Igor Grinberg grinberg@compulab.co.il wrote:
Hi Nathan,
On 12/11/16 15:58, Nathan Rossi wrote:
This series adds two functions for handling the memory bank decoding and initialization of global data for use by boards in their dram_init and dram_init_banksize functions.
I might have missed some discussions on this meter, can you please provide the use cases for this? IMO, the bootloader's job is to initialize the DRAM, detect its size, and pass the detected DRAM configuration on to an OS.
Hi Igor,
I do not think there have been any discussions on this (at least none that I am aware of).
Some boards (like Zynq and ZynqMP ones) are using CONFIG_SYS_SDRAM_SIZE to define the amount of memory that is available (since detection is not possible). However with the introduction of dtbs for some boards they are also capable of loading the size of memory from the embedded/appended dtb (instead of hardcoded). This allows for more of the board config to be loaded from the device tree instead of from include/configs/*.h. This however is up to the individual board to implement in its dram_init* functions.
Thanks for the explanation! I assume that the key point is "detection is not possible" and therefore we must rely on a user or a production process to place (append) the correct dtb. Makes sense to me now and looks like an improvement to the current situation.
The first patch of the series is only adding some decoding helper functions to make this generic between the Zynq and ZynqMP boards as well as to allow for any other boards that may want to use the same mechanism to get the memory size from the fdt. There is no requirement for boards to use these functions.
Can you please next time place a similar explanation in at least the cover letter. This way, the intent might be understood the first time ;-) I would also like to see some parts of the above explanation in the functions documentation (e.g. this allows to improve the DRAM configuration mechanics on boards that cannot detect its DRAM size/config).
Thanks!
Regards, Nathan
The series also changes the zynq and zynqmp board implementations to use these functions to resolve a issue with static variable use.
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 | 25 ++++++++++ lib/fdtdec.c | 54 +++++++++++++++++++++ 4 files changed, 85 insertions(+), 218 deletions(-)
-- Regards, Igor.
On 12 December 2016 at 03:08, Igor Grinberg grinberg@compulab.co.il wrote:
On 12/11/16 18:47, Nathan Rossi wrote:
On 12 December 2016 at 01:08, Igor Grinberg grinberg@compulab.co.il wrote:
Hi Nathan,
On 12/11/16 15:58, Nathan Rossi wrote:
This series adds two functions for handling the memory bank decoding and initialization of global data for use by boards in their dram_init and dram_init_banksize functions.
I might have missed some discussions on this meter, can you please provide the use cases for this? IMO, the bootloader's job is to initialize the DRAM, detect its size, and pass the detected DRAM configuration on to an OS.
Hi Igor,
I do not think there have been any discussions on this (at least none that I am aware of).
Some boards (like Zynq and ZynqMP ones) are using CONFIG_SYS_SDRAM_SIZE to define the amount of memory that is available (since detection is not possible). However with the introduction of dtbs for some boards they are also capable of loading the size of memory from the embedded/appended dtb (instead of hardcoded). This allows for more of the board config to be loaded from the device tree instead of from include/configs/*.h. This however is up to the individual board to implement in its dram_init* functions.
Thanks for the explanation! I assume that the key point is "detection is not possible" and therefore we must rely on a user or a production process to place (append) the correct dtb. Makes sense to me now and looks like an improvement to the current situation.
The first patch of the series is only adding some decoding helper functions to make this generic between the Zynq and ZynqMP boards as well as to allow for any other boards that may want to use the same mechanism to get the memory size from the fdt. There is no requirement for boards to use these functions.
Can you please next time place a similar explanation in at least the cover letter. This way, the intent might be understood the first time ;-)
Sorry about that, I will make sure future series have more complete descriptions in the cover letter. I have however updated the description for this in the v2 for completeness.
I would also like to see some parts of the above explanation in the functions documentation (e.g. this allows to improve the DRAM configuration mechanics on boards that cannot detect its DRAM size/config).
Will add in v2.
Regards, Nathan
Thanks!
Regards, Nathan
The series also changes the zynq and zynqmp board implementations to use these functions to resolve a issue with static variable use.
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 | 25 ++++++++++ lib/fdtdec.c | 54 +++++++++++++++++++++ 4 files changed, 85 insertions(+), 218 deletions(-)
-- Regards, Igor.
-- Regards, Igor.
participants (3)
-
Igor Grinberg -
Nathan Rossi -
Simon Glass