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

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

[Nathan Rossi]

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(-)

-- 
2.10.2

[U-Boot] [PATCH 1/3] fdt: add memory bank decoding functions for board setup

[Nathan Rossi]

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)
-- 
2.10.2

[U-Boot] [PATCH 2/3] ARM: zynq: Replace board specific with generic memory bank decoding

[Nathan Rossi]

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();
 
-- 
2.10.2

[U-Boot] [PATCH 3/3] ARM64: zynqmp: Replace board specific with generic memory bank decoding

[Nathan Rossi]

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;
 }
-- 
2.10.2

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

[Igor Grinberg]

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(-)
> 

-- 
Regards,
Igor.

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

[Nathan Rossi]

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.

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

[Igor Grinberg]

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.
> 

-- 
Regards,
Igor.

[U-Boot] [PATCH 1/3] fdt: add memory bank decoding functions for board setup

[Simon Glass]

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

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

[Nathan Rossi]

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.

[U-Boot] [PATCH 1/3] fdt: add memory bank decoding functions for board setup

[Nathan Rossi]

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

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

[Michal Simek]

On 12.12.2016 08:13, Nathan Rossi wrote:
> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>
>> 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).
>>
>> May I ask why is detection not possible on these boards (may be SoCs)?
> 
> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
> of cases are used in boards which have fixed memory device setups
> (without any SPD or equivalent).

For example zcu102 zynqmp development board is using modules with SPD on
it but if you look at generic SPD support then you will find out that
FSL(drivers/ddr/fsl) is one of the major platform which is using it for
ddr size detection.
Anyway as Nathan wrote above the majority of boards with zynq/zynqmp
devices need to be configured at build time or at run time by DTB.

There is also topic.nl boards which contain ddr configuration the same
for different ddr sizes and Mike sent this patch to get it work
http://lists.denx.de/pipermail/u-boot/2016-November/273385.html

Anyway in general there are some ways how to configure DDR size
- build time setup (CONFIG_SYS_SDRAM*)
- run time setup
	- ddr detection
		- via SPD (like FSL)
		- via algorithm (like topic.nl)
	- configuration
		- read DTB

Nathan's path is trying to address last run time DTB configuration
method to be shared across platforms because similar stuff Uniphier is
using too. And it doesn't make sense to copy that functions everywhere
that's why this should go core parts.

Thanks,
Michal

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

[Michal Simek]

On 12.12.2016 09:05, Igor Grinberg wrote:
> On 12/12/16 09:13, Nathan Rossi wrote:
>> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>>
>>> 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).
>>>
>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>
>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>> of cases are used in boards which have fixed memory device setups
>> (without any SPD or equivalent).
> 
> Ok. That is understood. Yet, it does not explain why the detection
> cannot be done.. For example, you know which memory device setups you
> _can_ have on the board, so the detection is just to figure out which
> one is assembled. We are doing this on i.MXes, OMAPs, PXAs, and others.
> 
> I was working on many ARM boards w/o SPD and still we almost always develop
> a detection mechanism which has some assumptions and knowledge of the board
> but it is much better then using some static data like compiled in or a dtb.
> 
> Have you considered a detection mechanism at all?

If you look at my previous email as you see that topic.nl is using this.

But the question is if this will work with all cases or just for
particular configuration. All zynq/zynqmp boards requires initial
setting which is created in Xilinx design tools. Export for these uniq
configurations are in ps7_init* or psu_init* files where DDR
configuration is part of this.

Devices contain various configurations for various memory types. Also
there is an option to add memory controller to programmable logic and
use it.
At the end of the day we won't be able to find out generic way for all
zynq/zynqmp boards which will simply work everywhere.

Just a summary of this. If you have one line of products which you have
tested and you know how they work then topic.nl solution is a way to go.
But I don't think I want to risk to have this only one method for all
zynq/zynqmp SoC.

Maybe thinking a little bit to the future. U-Boot is using linked-lists
more than before and we could provide all options as I described (and
maybe more) call them in loop. Limit this via Kconfig etc.

Thanks,
Michal

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

[Igor Grinberg]

On 12/12/16 10:02, Michal Simek wrote:
> On 12.12.2016 08:13, Nathan Rossi wrote:
>> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>>
>>> 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).
>>>
>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>
>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>> of cases are used in boards which have fixed memory device setups
>> (without any SPD or equivalent).
> 
> For example zcu102 zynqmp development board is using modules with SPD on
> it but if you look at generic SPD support then you will find out that
> FSL(drivers/ddr/fsl) is one of the major platform which is using it for
> ddr size detection.
> Anyway as Nathan wrote above the majority of boards with zynq/zynqmp
> devices need to be configured at build time or at run time by DTB.
> 
> There is also topic.nl boards which contain ddr configuration the same
> for different ddr sizes and Mike sent this patch to get it work
> http://lists.denx.de/pipermail/u-boot/2016-November/273385.html

That's exactly my point. I think Mike's patch does a way better job
detecting at run time than any compiled in or a DT based pseudo detection...

> 
> Anyway in general there are some ways how to configure DDR size
> - build time setup (CONFIG_SYS_SDRAM*)
> - run time setup
> 	- ddr detection
> 		- via SPD (like FSL)
> 		- via algorithm (like topic.nl)
> 	- configuration
> 		- read DTB
> 
> Nathan's path is trying to address last run time DTB configuration
> method to be shared across platforms because similar stuff Uniphier is
> using too. And it doesn't make sense to copy that functions everywhere
> that's why this should go core parts.

Yep. That's exactly what I thought. I see Nathan's patch set as an
improvement of the current situation anyway.

Also I think Mike's patch does a much better job on this.

-- 
Regards,
Igor.

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

[Michal Simek]

On 12.12.2016 09:24, Igor Grinberg wrote:
> On 12/12/16 10:02, Michal Simek wrote:
>> On 12.12.2016 08:13, Nathan Rossi wrote:
>>> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>>>
>>>> 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).
>>>>
>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>
>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>> of cases are used in boards which have fixed memory device setups
>>> (without any SPD or equivalent).
>>
>> For example zcu102 zynqmp development board is using modules with SPD on
>> it but if you look at generic SPD support then you will find out that
>> FSL(drivers/ddr/fsl) is one of the major platform which is using it for
>> ddr size detection.
>> Anyway as Nathan wrote above the majority of boards with zynq/zynqmp
>> devices need to be configured at build time or at run time by DTB.
>>
>> There is also topic.nl boards which contain ddr configuration the same
>> for different ddr sizes and Mike sent this patch to get it work
>> http://lists.denx.de/pipermail/u-boot/2016-November/273385.html
> 
> That's exactly my point. I think Mike's patch does a way better job
> detecting at run time than any compiled in or a DT based pseudo detection...
> 
>>
>> Anyway in general there are some ways how to configure DDR size
>> - build time setup (CONFIG_SYS_SDRAM*)
>> - run time setup
>> 	- ddr detection
>> 		- via SPD (like FSL)
>> 		- via algorithm (like topic.nl)
>> 	- configuration
>> 		- read DTB
>>
>> Nathan's path is trying to address last run time DTB configuration
>> method to be shared across platforms because similar stuff Uniphier is
>> using too. And it doesn't make sense to copy that functions everywhere
>> that's why this should go core parts.
> 
> Yep. That's exactly what I thought. I see Nathan's patch set as an
> improvement of the current situation anyway.
> 
> Also I think Mike's patch does a much better job on this.
> 

Just keep in your mind just in case that you know that your
configuration supports it. If you don't have DDR connected to hard block
and you use ddr to PL you don't even know where to look for DDR.
And with arm64 it is pretty huge space.
It means we really have to provide ways how to do it and these patches
are just generic way how to do it via reading DTB instead of code
duplication (+ bug) which we have now.

Thanks,
Michal

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

[Igor Grinberg]

On 12/12/16 10:18, Michal Simek wrote:
> On 12.12.2016 09:05, Igor Grinberg wrote:
>> On 12/12/16 09:13, Nathan Rossi wrote:
>>> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>>>
>>>> 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).
>>>>
>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>
>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>> of cases are used in boards which have fixed memory device setups
>>> (without any SPD or equivalent).
>>
>> Ok. That is understood. Yet, it does not explain why the detection
>> cannot be done.. For example, you know which memory device setups you
>> _can_ have on the board, so the detection is just to figure out which
>> one is assembled. We are doing this on i.MXes, OMAPs, PXAs, and others.
>>
>> I was working on many ARM boards w/o SPD and still we almost always develop
>> a detection mechanism which has some assumptions and knowledge of the board
>> but it is much better then using some static data like compiled in or a dtb.
>>
>> Have you considered a detection mechanism at all?
> 
> If you look at my previous email as you see that topic.nl is using this.

No, I sent it before seeing the email and Mike's patches.

> 
> But the question is if this will work with all cases or just for
> particular configuration. All zynq/zynqmp boards requires initial
> setting which is created in Xilinx design tools. Export for these uniq
> configurations are in ps7_init* or psu_init* files where DDR
> configuration is part of this.

Well, I think the board can provide the configuration as the board maintainer
probably knows it. I can be with the vendor provided tools or some other ways.
Once the configuration is provided, a common code should be able to do
the detection.

> 
> Devices contain various configurations for various memory types. Also
> there is an option to add memory controller to programmable logic and
> use it.
> At the end of the day we won't be able to find out generic way for all
> zynq/zynqmp boards which will simply work everywhere.

That's sounds correct. You can abstract the configuration process
and each board should provide its configuration data.

> 
> Just a summary of this. If you have one line of products which you have
> tested and you know how they work then topic.nl solution is a way to go.
> But I don't think I want to risk to have this only one method for all
> zynq/zynqmp SoC.

I don't think you need to.

> 
> Maybe thinking a little bit to the future. U-Boot is using linked-lists
> more than before and we could provide all options as I described (and
> maybe more) call them in loop. Limit this via Kconfig etc.

Yep. All the above sounds sane to me.


-- 
Regards,
Igor.

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

[Igor Grinberg]

On 12/12/16 10:27, Michal Simek wrote:
> On 12.12.2016 09:24, Igor Grinberg wrote:
>> On 12/12/16 10:02, Michal Simek wrote:
>>> On 12.12.2016 08:13, Nathan Rossi wrote:
>>>> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>>>>
>>>>> 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).
>>>>>
>>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>>
>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>> of cases are used in boards which have fixed memory device setups
>>>> (without any SPD or equivalent).
>>>
>>> For example zcu102 zynqmp development board is using modules with SPD on
>>> it but if you look at generic SPD support then you will find out that
>>> FSL(drivers/ddr/fsl) is one of the major platform which is using it for
>>> ddr size detection.
>>> Anyway as Nathan wrote above the majority of boards with zynq/zynqmp
>>> devices need to be configured at build time or at run time by DTB.
>>>
>>> There is also topic.nl boards which contain ddr configuration the same
>>> for different ddr sizes and Mike sent this patch to get it work
>>> http://lists.denx.de/pipermail/u-boot/2016-November/273385.html
>>
>> That's exactly my point. I think Mike's patch does a way better job
>> detecting at run time than any compiled in or a DT based pseudo detection...
>>
>>>
>>> Anyway in general there are some ways how to configure DDR size
>>> - build time setup (CONFIG_SYS_SDRAM*)
>>> - run time setup
>>> 	- ddr detection
>>> 		- via SPD (like FSL)
>>> 		- via algorithm (like topic.nl)
>>> 	- configuration
>>> 		- read DTB
>>>
>>> Nathan's path is trying to address last run time DTB configuration
>>> method to be shared across platforms because similar stuff Uniphier is
>>> using too. And it doesn't make sense to copy that functions everywhere
>>> that's why this should go core parts.
>>
>> Yep. That's exactly what I thought. I see Nathan's patch set as an
>> improvement of the current situation anyway.
>>
>> Also I think Mike's patch does a much better job on this.
>>
> 
> Just keep in your mind just in case that you know that your
> configuration supports it. If you don't have DDR connected to hard block
> and you use ddr to PL you don't even know where to look for DDR.
> And with arm64 it is pretty huge space.

I see this as exactly the type of information that should be provided by
the board code or a dtb.

> It means we really have to provide ways how to do it and these patches
> are just generic way how to do it via reading DTB instead of code
> duplication (+ bug) which we have now.

No doubt, Nathan's patch improves the situation.

-- 
Regards,
Igor.

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

[Michal Simek]

On 12.12.2016 09:54, Igor Grinberg wrote:
> On 12/12/16 10:27, Michal Simek wrote:
>> On 12.12.2016 09:24, Igor Grinberg wrote:
>>> On 12/12/16 10:02, Michal Simek wrote:
>>>> On 12.12.2016 08:13, Nathan Rossi wrote:
>>>>> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>>>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>>>>>
>>>>>> 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).
>>>>>>
>>>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>>>
>>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>>> of cases are used in boards which have fixed memory device setups
>>>>> (without any SPD or equivalent).
>>>>
>>>> For example zcu102 zynqmp development board is using modules with SPD on
>>>> it but if you look at generic SPD support then you will find out that
>>>> FSL(drivers/ddr/fsl) is one of the major platform which is using it for
>>>> ddr size detection.
>>>> Anyway as Nathan wrote above the majority of boards with zynq/zynqmp
>>>> devices need to be configured at build time or at run time by DTB.
>>>>
>>>> There is also topic.nl boards which contain ddr configuration the same
>>>> for different ddr sizes and Mike sent this patch to get it work
>>>> http://lists.denx.de/pipermail/u-boot/2016-November/273385.html
>>>
>>> That's exactly my point. I think Mike's patch does a way better job
>>> detecting at run time than any compiled in or a DT based pseudo detection...
>>>
>>>>
>>>> Anyway in general there are some ways how to configure DDR size
>>>> - build time setup (CONFIG_SYS_SDRAM*)
>>>> - run time setup
>>>> 	- ddr detection
>>>> 		- via SPD (like FSL)
>>>> 		- via algorithm (like topic.nl)
>>>> 	- configuration
>>>> 		- read DTB
>>>>
>>>> Nathan's path is trying to address last run time DTB configuration
>>>> method to be shared across platforms because similar stuff Uniphier is
>>>> using too. And it doesn't make sense to copy that functions everywhere
>>>> that's why this should go core parts.
>>>
>>> Yep. That's exactly what I thought. I see Nathan's patch set as an
>>> improvement of the current situation anyway.
>>>
>>> Also I think Mike's patch does a much better job on this.
>>>
>>
>> Just keep in your mind just in case that you know that your
>> configuration supports it. If you don't have DDR connected to hard block
>> and you use ddr to PL you don't even know where to look for DDR.
>> And with arm64 it is pretty huge space.
> 
> I see this as exactly the type of information that should be provided by
> the board code or a dtb.

I tend to remove all board files for zynq/zynqmp targets. Will see how
we can do it in future.

Thanks,
Michal

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

[Igor Grinberg]

On 12/12/16 11:03, Michal Simek wrote:
> On 12.12.2016 09:54, Igor Grinberg wrote:
>> On 12/12/16 10:27, Michal Simek wrote:
>>> On 12.12.2016 09:24, Igor Grinberg wrote:
>>>> On 12/12/16 10:02, Michal Simek wrote:
>>>>> On 12.12.2016 08:13, Nathan Rossi wrote:
>>>>>> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>>>>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>>>>>>
>>>>>>> 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).
>>>>>>>
>>>>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>>>>
>>>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>>>> of cases are used in boards which have fixed memory device setups
>>>>>> (without any SPD or equivalent).
>>>>>
>>>>> For example zcu102 zynqmp development board is using modules with SPD on
>>>>> it but if you look at generic SPD support then you will find out that
>>>>> FSL(drivers/ddr/fsl) is one of the major platform which is using it for
>>>>> ddr size detection.
>>>>> Anyway as Nathan wrote above the majority of boards with zynq/zynqmp
>>>>> devices need to be configured at build time or at run time by DTB.
>>>>>
>>>>> There is also topic.nl boards which contain ddr configuration the same
>>>>> for different ddr sizes and Mike sent this patch to get it work
>>>>> http://lists.denx.de/pipermail/u-boot/2016-November/273385.html
>>>>
>>>> That's exactly my point. I think Mike's patch does a way better job
>>>> detecting at run time than any compiled in or a DT based pseudo detection...
>>>>
>>>>>
>>>>> Anyway in general there are some ways how to configure DDR size
>>>>> - build time setup (CONFIG_SYS_SDRAM*)
>>>>> - run time setup
>>>>> 	- ddr detection
>>>>> 		- via SPD (like FSL)
>>>>> 		- via algorithm (like topic.nl)
>>>>> 	- configuration
>>>>> 		- read DTB
>>>>>
>>>>> Nathan's path is trying to address last run time DTB configuration
>>>>> method to be shared across platforms because similar stuff Uniphier is
>>>>> using too. And it doesn't make sense to copy that functions everywhere
>>>>> that's why this should go core parts.
>>>>
>>>> Yep. That's exactly what I thought. I see Nathan's patch set as an
>>>> improvement of the current situation anyway.
>>>>
>>>> Also I think Mike's patch does a much better job on this.
>>>>
>>>
>>> Just keep in your mind just in case that you know that your
>>> configuration supports it. If you don't have DDR connected to hard block
>>> and you use ddr to PL you don't even know where to look for DDR.
>>> And with arm64 it is pretty huge space.
>>
>> I see this as exactly the type of information that should be provided by
>> the board code or a dtb.
> 
> I tend to remove all board files for zynq/zynqmp targets. Will see how
> we can do it in future.

This is not really related to current thread...
I'm not sure how this can be done... We're in a bootloader world here...
It should be board specific by definition...
There should be board specific code (not static data) somewhere, and
I think we had agreed a year or so ago... on this meter.
I think if you go this way, we will end up having board specific middleware
all around... and lots of tools for changing the static data (e.g. dtbs).


-- 
Regards,
Igor.

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

[Michal Simek]

On 12.12.2016 12:10, Igor Grinberg wrote:
> On 12/12/16 11:03, Michal Simek wrote:
>> On 12.12.2016 09:54, Igor Grinberg wrote:
>>> On 12/12/16 10:27, Michal Simek wrote:
>>>> On 12.12.2016 09:24, Igor Grinberg wrote:
>>>>> On 12/12/16 10:02, Michal Simek wrote:
>>>>>> On 12.12.2016 08:13, Nathan Rossi wrote:
>>>>>>> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>>>>>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>>>>>>>
>>>>>>>> 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).
>>>>>>>>
>>>>>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>>>>>
>>>>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>>>>> of cases are used in boards which have fixed memory device setups
>>>>>>> (without any SPD or equivalent).
>>>>>>
>>>>>> For example zcu102 zynqmp development board is using modules with SPD on
>>>>>> it but if you look at generic SPD support then you will find out that
>>>>>> FSL(drivers/ddr/fsl) is one of the major platform which is using it for
>>>>>> ddr size detection.
>>>>>> Anyway as Nathan wrote above the majority of boards with zynq/zynqmp
>>>>>> devices need to be configured at build time or at run time by DTB.
>>>>>>
>>>>>> There is also topic.nl boards which contain ddr configuration the same
>>>>>> for different ddr sizes and Mike sent this patch to get it work
>>>>>> http://lists.denx.de/pipermail/u-boot/2016-November/273385.html
>>>>>
>>>>> That's exactly my point. I think Mike's patch does a way better job
>>>>> detecting at run time than any compiled in or a DT based pseudo detection...
>>>>>
>>>>>>
>>>>>> Anyway in general there are some ways how to configure DDR size
>>>>>> - build time setup (CONFIG_SYS_SDRAM*)
>>>>>> - run time setup
>>>>>> 	- ddr detection
>>>>>> 		- via SPD (like FSL)
>>>>>> 		- via algorithm (like topic.nl)
>>>>>> 	- configuration
>>>>>> 		- read DTB
>>>>>>
>>>>>> Nathan's path is trying to address last run time DTB configuration
>>>>>> method to be shared across platforms because similar stuff Uniphier is
>>>>>> using too. And it doesn't make sense to copy that functions everywhere
>>>>>> that's why this should go core parts.
>>>>>
>>>>> Yep. That's exactly what I thought. I see Nathan's patch set as an
>>>>> improvement of the current situation anyway.
>>>>>
>>>>> Also I think Mike's patch does a much better job on this.
>>>>>
>>>>
>>>> Just keep in your mind just in case that you know that your
>>>> configuration supports it. If you don't have DDR connected to hard block
>>>> and you use ddr to PL you don't even know where to look for DDR.
>>>> And with arm64 it is pretty huge space.
>>>
>>> I see this as exactly the type of information that should be provided by
>>> the board code or a dtb.
>>
>> I tend to remove all board files for zynq/zynqmp targets. Will see how
>> we can do it in future.
> 
> This is not really related to current thread...

not directly but there is a connection.

> I'm not sure how this can be done... We're in a bootloader world here...
> It should be board specific by definition...
> There should be board specific code (not static data) somewhere, and
> I think we had agreed a year or so ago... on this meter.
> I think if you go this way, we will end up having board specific middleware
> all around... and lots of tools for changing the static data (e.g. dtbs).

Look at microblaze. I am not accepting any board to be added for it
because every configuration is different. The same can be done for
zynq/zynqmp boards when we move stuff to DM.
Then for supporting new platform you don't need to create folder in
board and file include/configs/ and all you need is dts file and
defconfig. In case of SPL for us we need to also put somewhere ps7*/psu*
init files.
DT is design to describe hardware and current configuration which I
believe we can.
There is also DT overlay stuff which has been merged recently.
All this together is going to end up in situation that you will have
generic hardcoded config for core stuff and the rest in DTS file.

In connection to this thread. If you have all ddr start/size algorithms
prepared then you can probably select them via defconfig because static
part is board specific which means it should go to Kconfig/defconfig.

Thanks,
Michal

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

[Mike Looijmans]

?On 12-12-16 09:18, Michal Simek wrote:
> On 12.12.2016 09:05, Igor Grinberg wrote:
>> On 12/12/16 09:13, Nathan Rossi wrote:
>>> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>>>
>>>> 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).
>>>>
>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>
>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>> of cases are used in boards which have fixed memory device setups
>>> (without any SPD or equivalent).
>>
>> Ok. That is understood. Yet, it does not explain why the detection
>> cannot be done.. For example, you know which memory device setups you
>> _can_ have on the board, so the detection is just to figure out which
>> one is assembled. We are doing this on i.MXes, OMAPs, PXAs, and others.
>>
>> I was working on many ARM boards w/o SPD and still we almost always develop
>> a detection mechanism which has some assumptions and knowledge of the board
>> but it is much better then using some static data like compiled in or a dtb.
>>
>> Have you considered a detection mechanism at all?
>
> If you look at my previous email as you see that topic.nl is using this.
>
> But the question is if this will work with all cases or just for
> particular configuration. All zynq/zynqmp boards requires initial
> setting which is created in Xilinx design tools. Export for these uniq
> configurations are in ps7_init* or psu_init* files where DDR
> configuration is part of this.
>
> Devices contain various configurations for various memory types. Also
> there is an option to add memory controller to programmable logic and
> use it.

And the static memory controller can probably also be used to drive SRAM...

But you could combine the two. The devicetree could set up the area's to 
search, and then a detection routine to check what's really there. This has 
the added value of a quick test that the memory actually works before starting 
to use it.

> At the end of the day we won't be able to find out generic way for all
> zynq/zynqmp boards which will simply work everywhere.
>
> Just a summary of this. If you have one line of products which you have
> tested and you know how they work then topic.nl solution is a way to go.
> But I don't think I want to risk to have this only one method for all
> zynq/zynqmp SoC.
>
> Maybe thinking a little bit to the future. U-Boot is using linked-lists
> more than before and we could provide all options as I described (and
> maybe more) call them in loop. Limit this via Kconfig etc.
>
> Thanks,
> Michal
>
>
>
>
>
>
>
> 

Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans at topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail





_______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

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

[Igor Grinberg]

On 12/12/16 13:56, Michal Simek wrote:
> On 12.12.2016 12:10, Igor Grinberg wrote:
>> On 12/12/16 11:03, Michal Simek wrote:
>>> On 12.12.2016 09:54, Igor Grinberg wrote:
>>>> On 12/12/16 10:27, Michal Simek wrote:
>>>>> On 12.12.2016 09:24, Igor Grinberg wrote:
>>>>>> On 12/12/16 10:02, Michal Simek wrote:
>>>>>>> On 12.12.2016 08:13, Nathan Rossi wrote:
>>>>>>>> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>>>>>>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>>>>>>>>
>>>>>>>>> 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).
>>>>>>>>>
>>>>>>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>>>>>>
>>>>>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>>>>>> of cases are used in boards which have fixed memory device setups
>>>>>>>> (without any SPD or equivalent).
>>>>>>>
>>>>>>> For example zcu102 zynqmp development board is using modules with SPD on
>>>>>>> it but if you look at generic SPD support then you will find out that
>>>>>>> FSL(drivers/ddr/fsl) is one of the major platform which is using it for
>>>>>>> ddr size detection.
>>>>>>> Anyway as Nathan wrote above the majority of boards with zynq/zynqmp
>>>>>>> devices need to be configured at build time or at run time by DTB.
>>>>>>>
>>>>>>> There is also topic.nl boards which contain ddr configuration the same
>>>>>>> for different ddr sizes and Mike sent this patch to get it work
>>>>>>> http://lists.denx.de/pipermail/u-boot/2016-November/273385.html
>>>>>>
>>>>>> That's exactly my point. I think Mike's patch does a way better job
>>>>>> detecting at run time than any compiled in or a DT based pseudo detection...
>>>>>>
>>>>>>>
>>>>>>> Anyway in general there are some ways how to configure DDR size
>>>>>>> - build time setup (CONFIG_SYS_SDRAM*)
>>>>>>> - run time setup
>>>>>>> 	- ddr detection
>>>>>>> 		- via SPD (like FSL)
>>>>>>> 		- via algorithm (like topic.nl)
>>>>>>> 	- configuration
>>>>>>> 		- read DTB
>>>>>>>
>>>>>>> Nathan's path is trying to address last run time DTB configuration
>>>>>>> method to be shared across platforms because similar stuff Uniphier is
>>>>>>> using too. And it doesn't make sense to copy that functions everywhere
>>>>>>> that's why this should go core parts.
>>>>>>
>>>>>> Yep. That's exactly what I thought. I see Nathan's patch set as an
>>>>>> improvement of the current situation anyway.
>>>>>>
>>>>>> Also I think Mike's patch does a much better job on this.
>>>>>>
>>>>>
>>>>> Just keep in your mind just in case that you know that your
>>>>> configuration supports it. If you don't have DDR connected to hard block
>>>>> and you use ddr to PL you don't even know where to look for DDR.
>>>>> And with arm64 it is pretty huge space.
>>>>
>>>> I see this as exactly the type of information that should be provided by
>>>> the board code or a dtb.
>>>
>>> I tend to remove all board files for zynq/zynqmp targets. Will see how
>>> we can do it in future.
>>
>> This is not really related to current thread...
> 
> not directly but there is a connection.
> 
>> I'm not sure how this can be done... We're in a bootloader world here...
>> It should be board specific by definition...
>> There should be board specific code (not static data) somewhere, and
>> I think we had agreed a year or so ago... on this meter.
>> I think if you go this way, we will end up having board specific middleware
>> all around... and lots of tools for changing the static data (e.g. dtbs).
> 
> Look at microblaze. I am not accepting any board to be added for it
> because every configuration is different.

I'm not sure I get your point.

> The same can be done for
> zynq/zynqmp boards when we move stuff to DM.
> Then for supporting new platform you don't need to create folder in
> board and file include/configs/ and all you need is dts file and
> defconfig. In case of SPL for us we need to also put somewhere ps7*/psu*
> init files.

Not exactly...
This can be true if you force all vendors to copy the ref design...
I believe you can't do this.
There are many boards out there that copy the reference design and this
will work with them, but there are also boards who don't follow the ref design
and this will only push them back from upstream...

> DT is design to describe hardware and current configuration which I
> believe we can.

Yes. Only static data...

> There is also DT overlay stuff which has been merged recently.

This does improve the situation, but you still need code for detection and
and loading of the overlays.

> All this together is going to end up in situation that you will have
> generic hardcoded config for core stuff and the rest in DTS file.

You will always have boards (e.g. SoMs) that have its different wiring
and requires dynamic code. Unless you force everyone to ref design, but
what's the point then... ARMs strength is in diversity and flexibility.

> 
> In connection to this thread. If you have all ddr start/size algorithms
> prepared then you can probably select them via defconfig because static
> part is board specific which means it should go to Kconfig/defconfig.

That might work...
From my humble experience, I have never had a board with the same
algorithm used in between SoC generations and even different algorithms
on the same SoC generation but different boards.


-- 
Regards,
Igor.

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

[Igor Grinberg]

On 12/12/16 14:05, Mike Looijmans wrote:
> On 12-12-16 09:18, Michal Simek wrote:
>> On 12.12.2016 09:05, Igor Grinberg wrote:
>>> On 12/12/16 09:13, Nathan Rossi wrote:
>>>> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>>>>
>>>>> 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).
>>>>>
>>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>>
>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>> of cases are used in boards which have fixed memory device setups
>>>> (without any SPD or equivalent).
>>>
>>> Ok. That is understood. Yet, it does not explain why the detection
>>> cannot be done.. For example, you know which memory device setups you
>>> _can_ have on the board, so the detection is just to figure out which
>>> one is assembled. We are doing this on i.MXes, OMAPs, PXAs, and others.
>>>
>>> I was working on many ARM boards w/o SPD and still we almost always develop
>>> a detection mechanism which has some assumptions and knowledge of the board
>>> but it is much better then using some static data like compiled in or a dtb.
>>>
>>> Have you considered a detection mechanism at all?
>>
>> If you look at my previous email as you see that topic.nl is using this.
>>
>> But the question is if this will work with all cases or just for
>> particular configuration. All zynq/zynqmp boards requires initial
>> setting which is created in Xilinx design tools. Export for these uniq
>> configurations are in ps7_init* or psu_init* files where DDR
>> configuration is part of this.
>>
>> Devices contain various configurations for various memory types. Also
>> there is an option to add memory controller to programmable logic and
>> use it.
> 
> And the static memory controller can probably also be used to drive SRAM...
> 
> But you could combine the two. The devicetree could set up the area's to search, and then a detection routine to check what's really there. This has the added value of a quick test that the memory actually works before starting to use it.

That's exactly the point I was trying to show.
Thanks Mike.


-- 
Regards,
Igor.

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

[Michal Simek]

On 13.12.2016 09:53, Igor Grinberg wrote:
> On 12/12/16 13:56, Michal Simek wrote:
>> On 12.12.2016 12:10, Igor Grinberg wrote:
>>> On 12/12/16 11:03, Michal Simek wrote:
>>>> On 12.12.2016 09:54, Igor Grinberg wrote:
>>>>> On 12/12/16 10:27, Michal Simek wrote:
>>>>>> On 12.12.2016 09:24, Igor Grinberg wrote:
>>>>>>> On 12/12/16 10:02, Michal Simek wrote:
>>>>>>>> On 12.12.2016 08:13, Nathan Rossi wrote:
>>>>>>>>> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>>>>>>>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>>>>>>>>>
>>>>>>>>>> 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).
>>>>>>>>>>
>>>>>>>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>>>>>>>
>>>>>>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>>>>>>> of cases are used in boards which have fixed memory device setups
>>>>>>>>> (without any SPD or equivalent).
>>>>>>>>
>>>>>>>> For example zcu102 zynqmp development board is using modules with SPD on
>>>>>>>> it but if you look at generic SPD support then you will find out that
>>>>>>>> FSL(drivers/ddr/fsl) is one of the major platform which is using it for
>>>>>>>> ddr size detection.
>>>>>>>> Anyway as Nathan wrote above the majority of boards with zynq/zynqmp
>>>>>>>> devices need to be configured at build time or at run time by DTB.
>>>>>>>>
>>>>>>>> There is also topic.nl boards which contain ddr configuration the same
>>>>>>>> for different ddr sizes and Mike sent this patch to get it work
>>>>>>>> http://lists.denx.de/pipermail/u-boot/2016-November/273385.html
>>>>>>>
>>>>>>> That's exactly my point. I think Mike's patch does a way better job
>>>>>>> detecting at run time than any compiled in or a DT based pseudo detection...
>>>>>>>
>>>>>>>>
>>>>>>>> Anyway in general there are some ways how to configure DDR size
>>>>>>>> - build time setup (CONFIG_SYS_SDRAM*)
>>>>>>>> - run time setup
>>>>>>>> 	- ddr detection
>>>>>>>> 		- via SPD (like FSL)
>>>>>>>> 		- via algorithm (like topic.nl)
>>>>>>>> 	- configuration
>>>>>>>> 		- read DTB
>>>>>>>>
>>>>>>>> Nathan's path is trying to address last run time DTB configuration
>>>>>>>> method to be shared across platforms because similar stuff Uniphier is
>>>>>>>> using too. And it doesn't make sense to copy that functions everywhere
>>>>>>>> that's why this should go core parts.
>>>>>>>
>>>>>>> Yep. That's exactly what I thought. I see Nathan's patch set as an
>>>>>>> improvement of the current situation anyway.
>>>>>>>
>>>>>>> Also I think Mike's patch does a much better job on this.
>>>>>>>
>>>>>>
>>>>>> Just keep in your mind just in case that you know that your
>>>>>> configuration supports it. If you don't have DDR connected to hard block
>>>>>> and you use ddr to PL you don't even know where to look for DDR.
>>>>>> And with arm64 it is pretty huge space.
>>>>>
>>>>> I see this as exactly the type of information that should be provided by
>>>>> the board code or a dtb.
>>>>
>>>> I tend to remove all board files for zynq/zynqmp targets. Will see how
>>>> we can do it in future.
>>>
>>> This is not really related to current thread...
>>
>> not directly but there is a connection.
>>
>>> I'm not sure how this can be done... We're in a bootloader world here...
>>> It should be board specific by definition...
>>> There should be board specific code (not static data) somewhere, and
>>> I think we had agreed a year or so ago... on this meter.
>>> I think if you go this way, we will end up having board specific middleware
>>> all around... and lots of tools for changing the static data (e.g. dtbs).
>>
>> Look at microblaze. I am not accepting any board to be added for it
>> because every configuration is different.
> 
> I'm not sure I get your point.
> 
>> The same can be done for
>> zynq/zynqmp boards when we move stuff to DM.
>> Then for supporting new platform you don't need to create folder in
>> board and file include/configs/ and all you need is dts file and
>> defconfig. In case of SPL for us we need to also put somewhere ps7*/psu*
>> init files.
> 
> Not exactly...
> This can be true if you force all vendors to copy the ref design...
> I believe you can't do this.
> There are many boards out there that copy the reference design and this
> will work with them, but there are also boards who don't follow the ref design
> and this will only push them back from upstream...
> 
>> DT is design to describe hardware and current configuration which I
>> believe we can.
> 
> Yes. Only static data...

Static initial configuration - yes.

> 
>> There is also DT overlay stuff which has been merged recently.
> 
> This does improve the situation, but you still need code for detection and
> and loading of the overlays.

You need to manage this process. In most cases I don't think that there
will be auto detection option in place. And it will be up to user to
decide what to do.


>> All this together is going to end up in situation that you will have
>> generic hardcoded config for core stuff and the rest in DTS file.
> 
> You will always have boards (e.g. SoMs) that have its different wiring
> and requires dynamic code. Unless you force everyone to ref design, but
> what's the point then... ARMs strength is in diversity and flexibility.

It is just about how to decide to sort this and handle it.
Different combinations end up in sort of static configuration and
definitely if you have an option to detect stuff at run time you can use
it.

>> In connection to this thread. If you have all ddr start/size algorithms
>> prepared then you can probably select them via defconfig because static
>> part is board specific which means it should go to Kconfig/defconfig.
> 
> That might work...
> From my humble experience, I have never had a board with the same
> algorithm used in between SoC generations and even different algorithms
> on the same SoC generation but different boards.


TBH maybe we are just don't understand each other properly. I will be
the last person
not to move to run-time case. We do fpgas and I don't know any other
chips which are more capable in run time configurations then fpgas.
We have partial reconfiguration in place if you want change HW at run time.
And this patch is simply just trying to improve one particular problem
which is loading memory configuration from DTB about DDR setting.
If there are other ways how to detect DDR size that's different topic.
If we can combine detection algorithms together that's also something
what we can discuss.

I am not marketing guy to say how often there are designs with only
different DDR size like Mike's example but in fpga world you are not
buying this chip to have only static part but you want to use fpga part
and for that you need to use design tools. Because every design is
unique you can generate device tree description directly from design
tools which covers your target and this is what I believe people use.

Thanks,
Michal

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

[Michal Simek]

On 13.12.2016 09:56, Igor Grinberg wrote:
> On 12/12/16 14:05, Mike Looijmans wrote:
>> On 12-12-16 09:18, Michal Simek wrote:
>>> On 12.12.2016 09:05, Igor Grinberg wrote:
>>>> On 12/12/16 09:13, Nathan Rossi wrote:
>>>>> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>>>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>>>>>
>>>>>> 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).
>>>>>>
>>>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>>>
>>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>>> of cases are used in boards which have fixed memory device setups
>>>>> (without any SPD or equivalent).
>>>>
>>>> Ok. That is understood. Yet, it does not explain why the detection
>>>> cannot be done.. For example, you know which memory device setups you
>>>> _can_ have on the board, so the detection is just to figure out which
>>>> one is assembled. We are doing this on i.MXes, OMAPs, PXAs, and others.
>>>>
>>>> I was working on many ARM boards w/o SPD and still we almost always develop
>>>> a detection mechanism which has some assumptions and knowledge of the board
>>>> but it is much better then using some static data like compiled in or a dtb.
>>>>
>>>> Have you considered a detection mechanism at all?
>>>
>>> If you look at my previous email as you see that topic.nl is using this.
>>>
>>> But the question is if this will work with all cases or just for
>>> particular configuration. All zynq/zynqmp boards requires initial
>>> setting which is created in Xilinx design tools. Export for these uniq
>>> configurations are in ps7_init* or psu_init* files where DDR
>>> configuration is part of this.
>>>
>>> Devices contain various configurations for various memory types. Also
>>> there is an option to add memory controller to programmable logic and
>>> use it.
>>
>> And the static memory controller can probably also be used to drive SRAM...
>>
>> But you could combine the two. The devicetree could set up the area's to search, and then a detection routine to check
 what's really there. This has the added value of a quick test that the
memory actually works before starting to use it.
> 
> That's exactly the point I was trying to show.

No problem with this but for me this is generic configuration option.
It means this should be covered by Kconfig to be selected for specific
platform. This code should go to common folder not to board folder or
arm-mach folder.

Thanks,
Michal

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

[Igor Grinberg]

On 12/13/16 13:07, Michal Simek wrote:
> On 13.12.2016 09:53, Igor Grinberg wrote:
>> On 12/12/16 13:56, Michal Simek wrote:
>>> On 12.12.2016 12:10, Igor Grinberg wrote:
>>>> On 12/12/16 11:03, Michal Simek wrote:
>>>>> On 12.12.2016 09:54, Igor Grinberg wrote:
>>>>>> On 12/12/16 10:27, Michal Simek wrote:
>>>>>>> On 12.12.2016 09:24, Igor Grinberg wrote:
>>>>>>>> On 12/12/16 10:02, Michal Simek wrote:
>>>>>>>>> On 12.12.2016 08:13, Nathan Rossi wrote:
>>>>>>>>>> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>>>>>>>>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>>>>>>>>>>
>>>>>>>>>>> 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).
>>>>>>>>>>>
>>>>>>>>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>>>>>>>>
>>>>>>>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>>>>>>>> of cases are used in boards which have fixed memory device setups
>>>>>>>>>> (without any SPD or equivalent).
>>>>>>>>>
>>>>>>>>> For example zcu102 zynqmp development board is using modules with SPD on
>>>>>>>>> it but if you look at generic SPD support then you will find out that
>>>>>>>>> FSL(drivers/ddr/fsl) is one of the major platform which is using it for
>>>>>>>>> ddr size detection.
>>>>>>>>> Anyway as Nathan wrote above the majority of boards with zynq/zynqmp
>>>>>>>>> devices need to be configured at build time or at run time by DTB.
>>>>>>>>>
>>>>>>>>> There is also topic.nl boards which contain ddr configuration the same
>>>>>>>>> for different ddr sizes and Mike sent this patch to get it work
>>>>>>>>> http://lists.denx.de/pipermail/u-boot/2016-November/273385.html
>>>>>>>>
>>>>>>>> That's exactly my point. I think Mike's patch does a way better job
>>>>>>>> detecting at run time than any compiled in or a DT based pseudo detection...
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Anyway in general there are some ways how to configure DDR size
>>>>>>>>> - build time setup (CONFIG_SYS_SDRAM*)
>>>>>>>>> - run time setup
>>>>>>>>> 	- ddr detection
>>>>>>>>> 		- via SPD (like FSL)
>>>>>>>>> 		- via algorithm (like topic.nl)
>>>>>>>>> 	- configuration
>>>>>>>>> 		- read DTB
>>>>>>>>>
>>>>>>>>> Nathan's path is trying to address last run time DTB configuration
>>>>>>>>> method to be shared across platforms because similar stuff Uniphier is
>>>>>>>>> using too. And it doesn't make sense to copy that functions everywhere
>>>>>>>>> that's why this should go core parts.
>>>>>>>>
>>>>>>>> Yep. That's exactly what I thought. I see Nathan's patch set as an
>>>>>>>> improvement of the current situation anyway.
>>>>>>>>
>>>>>>>> Also I think Mike's patch does a much better job on this.
>>>>>>>>
>>>>>>>
>>>>>>> Just keep in your mind just in case that you know that your
>>>>>>> configuration supports it. If you don't have DDR connected to hard block
>>>>>>> and you use ddr to PL you don't even know where to look for DDR.
>>>>>>> And with arm64 it is pretty huge space.
>>>>>>
>>>>>> I see this as exactly the type of information that should be provided by
>>>>>> the board code or a dtb.
>>>>>
>>>>> I tend to remove all board files for zynq/zynqmp targets. Will see how
>>>>> we can do it in future.
>>>>
>>>> This is not really related to current thread...
>>>
>>> not directly but there is a connection.
>>>
>>>> I'm not sure how this can be done... We're in a bootloader world here...
>>>> It should be board specific by definition...
>>>> There should be board specific code (not static data) somewhere, and
>>>> I think we had agreed a year or so ago... on this meter.
>>>> I think if you go this way, we will end up having board specific middleware
>>>> all around... and lots of tools for changing the static data (e.g. dtbs).
>>>
>>> Look at microblaze. I am not accepting any board to be added for it
>>> because every configuration is different.
>>
>> I'm not sure I get your point.
>>
>>> The same can be done for
>>> zynq/zynqmp boards when we move stuff to DM.
>>> Then for supporting new platform you don't need to create folder in
>>> board and file include/configs/ and all you need is dts file and
>>> defconfig. In case of SPL for us we need to also put somewhere ps7*/psu*
>>> init files.
>>
>> Not exactly...
>> This can be true if you force all vendors to copy the ref design...
>> I believe you can't do this.
>> There are many boards out there that copy the reference design and this
>> will work with them, but there are also boards who don't follow the ref design
>> and this will only push them back from upstream...
>>
>>> DT is design to describe hardware and current configuration which I
>>> believe we can.
>>
>> Yes. Only static data...
> 
> Static initial configuration - yes.
> 
>>
>>> There is also DT overlay stuff which has been merged recently.
>>
>> This does improve the situation, but you still need code for detection and
>> and loading of the overlays.
> 
> You need to manage this process. In most cases I don't think that there
> will be auto detection option in place. And it will be up to user to
> decide what to do.

If you let the board provide its logic for auto detection, there certainly
will be auto detection. I don't think that users should mess with
this stuff.
We have plenty of users who barely can follow our documentation...

> 
> 
>>> All this together is going to end up in situation that you will have
>>> generic hardcoded config for core stuff and the rest in DTS file.
>>
>> You will always have boards (e.g. SoMs) that have its different wiring
>> and requires dynamic code. Unless you force everyone to ref design, but
>> what's the point then... ARMs strength is in diversity and flexibility.
> 
> It is just about how to decide to sort this and handle it.
> Different combinations end up in sort of static configuration and
> definitely if you have an option to detect stuff at run time you can use
> it.
> 
>>> In connection to this thread. If you have all ddr start/size algorithms
>>> prepared then you can probably select them via defconfig because static
>>> part is board specific which means it should go to Kconfig/defconfig.
>>
>> That might work...
>> From my humble experience, I have never had a board with the same
>> algorithm used in between SoC generations and even different algorithms
>> on the same SoC generation but different boards.
> 
> 
> TBH maybe we are just don't understand each other properly. I will be
> the last person
> not to move to run-time case. We do fpgas and I don't know any other
> chips which are more capable in run time configurations then fpgas.
> We have partial reconfiguration in place if you want change HW at run time.
> And this patch is simply just trying to improve one particular problem
> which is loading memory configuration from DTB about DDR setting.

As already said, I think this patch is an improvement.
But I wouldn't like people to think that this is the direction that
everybody should take.

> If there are other ways how to detect DDR size that's different topic.
> If we can combine detection algorithms together that's also something
> what we can discuss.

Yep. That's great with me!

> 
> I am not marketing guy to say how often there are designs with only
> different DDR size like Mike's example but in fpga world you are not
> buying this chip to have only static part but you want to use fpga part
> and for that you need to use design tools. Because every design is
> unique you can generate device tree description directly from design
> tools which covers your target and this is what I believe people use.

Sort of...
Additionally, there are SoMs that are a kind of partial design as it is not
a final product.
I think that is exactly, why there should be a kind of limited freedom
for board to provides its stuff.

-- 
Regards,
Igor.

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

[Igor Grinberg]

On 12/13/16 13:12, Michal Simek wrote:
> On 13.12.2016 09:56, Igor Grinberg wrote:
>> On 12/12/16 14:05, Mike Looijmans wrote:
>>> On 12-12-16 09:18, Michal Simek wrote:
>>>> On 12.12.2016 09:05, Igor Grinberg wrote:
>>>>> On 12/12/16 09:13, Nathan Rossi wrote:
>>>>>> On 12 December 2016 at 03:11, Igor Grinberg <grinberg@compulab.co.il> wrote:
>>>>>>> dropping the list for this one as the question seems to me irrelevant to your patch set.
>>>>>>>
>>>>>>> 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).
>>>>>>>
>>>>>>> May I ask why is detection not possible on these boards (may be SoCs)?
>>>>>>
>>>>>> That is correct, Zynq and ZynqMP are ARM SoCs. Which in the majority
>>>>>> of cases are used in boards which have fixed memory device setups
>>>>>> (without any SPD or equivalent).
>>>>>
>>>>> Ok. That is understood. Yet, it does not explain why the detection
>>>>> cannot be done.. For example, you know which memory device setups you
>>>>> _can_ have on the board, so the detection is just to figure out which
>>>>> one is assembled. We are doing this on i.MXes, OMAPs, PXAs, and others.
>>>>>
>>>>> I was working on many ARM boards w/o SPD and still we almost always develop
>>>>> a detection mechanism which has some assumptions and knowledge of the board
>>>>> but it is much better then using some static data like compiled in or a dtb.
>>>>>
>>>>> Have you considered a detection mechanism at all?
>>>>
>>>> If you look at my previous email as you see that topic.nl is using this.
>>>>
>>>> But the question is if this will work with all cases or just for
>>>> particular configuration. All zynq/zynqmp boards requires initial
>>>> setting which is created in Xilinx design tools. Export for these uniq
>>>> configurations are in ps7_init* or psu_init* files where DDR
>>>> configuration is part of this.
>>>>
>>>> Devices contain various configurations for various memory types. Also
>>>> there is an option to add memory controller to programmable logic and
>>>> use it.
>>>
>>> And the static memory controller can probably also be used to drive SRAM...
>>>
>>> But you could combine the two. The devicetree could set up the area's to search, and then a detection routine to check
>  what's really there. This has the added value of a quick test that the
> memory actually works before starting to use it.
>>
>> That's exactly the point I was trying to show.
> 
> No problem with this but for me this is generic configuration option.
> It means this should be covered by Kconfig to be selected for specific
> platform. This code should go to common folder not to board folder or
> arm-mach folder.

Well, if it is generic enough it really should.

-- 
Regards,
Igor.

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

[Mike Looijmans]

?
> I am not marketing guy to say how often there are designs with only
> different DDR size like Mike's example but in fpga world you are not
> buying this chip to have only static part but you want to use fpga part
> and for that you need to use design tools. Because every design is
> unique you can generate device tree description directly from design
> tools which covers your target and this is what I believe people use.

Well, I can't speak for everyone...

Most people don't want to write (or even compile) a new bootloader for each 
and every project. For our Miami SOMs, there are already more full-custom 
carrier boards than evaluation boards. If we had to build a bootloader for 
each such design, there'd be dozens of them.

What we try to do is just use the generic bootloader to get the SOM up and 
running, and then provide all the project hardware details in the kernel's 
final devicetree. That includes changing pinmuxing and clocks and stuff, which 
is easy to do.




Kind regards,

Mike Looijmans
System Expert

TOPIC Products
Materiaalweg 4, NL-5681 RJ Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
E-mail: mike.looijmans at topicproducts.com
Website: www.topicproducts.com

Please consider the environment before printing this e-mail

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

[Michal Simek]

On 14.12.2016 16:00, Mike Looijmans wrote:
> 
>> I am not marketing guy to say how often there are designs with only
>> different DDR size like Mike's example but in fpga world you are not
>> buying this chip to have only static part but you want to use fpga part
>> and for that you need to use design tools. Because every design is
>> unique you can generate device tree description directly from design
>> tools which covers your target and this is what I believe people use.
> 
> Well, I can't speak for everyone...
> 
> Most people don't want to write (or even compile) a new bootloader for
> each and every project. For our Miami SOMs, there are already more
> full-custom carrier boards than evaluation boards. If we had to build a
> bootloader for each such design, there'd be dozens of them.
> 
> What we try to do is just use the generic bootloader to get the SOM up
> and running, and then provide all the project hardware details in the
> kernel's final devicetree. That includes changing pinmuxing and clocks
> and stuff, which is easy to do.

That's nothing against what I have said. Having as much flexibility you
need is great. We should support several method how to setup stuff and
it is up to user if this method is suitable for you or not and doing
these selection via Kconfig is the way we need to go.
For all these autodetection algorithms you have to be sure that it is
working fine on your platform based on testing.

Thanks,
Michal

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

[Michal Simek]

On 15.12.2016 08:21, Michal Simek wrote:
> On 14.12.2016 16:00, Mike Looijmans wrote:
>>
>>> I am not marketing guy to say how often there are designs with only
>>> different DDR size like Mike's example but in fpga world you are not
>>> buying this chip to have only static part but you want to use fpga part
>>> and for that you need to use design tools. Because every design is
>>> unique you can generate device tree description directly from design
>>> tools which covers your target and this is what I believe people use.
>>
>> Well, I can't speak for everyone...
>>
>> Most people don't want to write (or even compile) a new bootloader for
>> each and every project. For our Miami SOMs, there are already more
>> full-custom carrier boards than evaluation boards. If we had to build a
>> bootloader for each such design, there'd be dozens of them.
>>
>> What we try to do is just use the generic bootloader to get the SOM up
>> and running, and then provide all the project hardware details in the
>> kernel's final devicetree. That includes changing pinmuxing and clocks
>> and stuff, which is easy to do.
> 
> That's nothing against what I have said. Having as much flexibility you
> need is great. We should support several method how to setup stuff and
> it is up to user if this method is suitable for you or not and doing
> these selection via Kconfig is the way we need to go.
> For all these autodetection algorithms you have to be sure that it is
> working fine on your platform based on testing.

Just a note for everybody. V2 patches contain compilation issue for
several boards. I have reported it back to Nathan to fix it.

Here is a log.
https://travis-ci.org/michalsimek-test/u-boot/jobs/184187944

Thanks,
Michal