common/memsize.c: restore content of the base address

I detect a issue in get_ram_size() function when maxsize is available; in this case, the base address is not restored.

Please, check if these 2 patch don't introduce regression on your board because this code is really sensible.

Issue detected on my board (not yet up-streamed) with
- DDR base = 0xC0000000
- expected detected size = maxsize = 0x40000000
The loop is completely executed so the size is correctly detected, but content of memory at address 0xC0000000 is not restored:
I sill have 0xC0000000 = 0x0 at the end of the function.

In the correction (patch 2 of the serie), I restore this content when no error is detected (for the last return).

NB: I also add a comment for the previous return to avoid regression
for the case size<max_size
because for this case, the higher bit of address are not used and the higher addresses are overlapped with
the lower addresses (same physical content)
=> the original data at address 0x0 is already restore
because *(base + size) and *base is physically the same physical address = ~(size-1) & logical address
=> address overlap detected with (val != ~cnt)
*base was forced to 0
but content for *(base + size) already restore

Test on Sandbox (v2018.01)
--------------------------

I test the issue and the correction in sandbox by adding a debug command "test_ram_size" in cmd/mem.c which:
1/ fill memory buffer with random value
2/ use get_ram_size() on this buffer
3/ check buffer content modification

static int do_test_ram_size(cmd_tbl_t *cmdtp, int flag, int argc,

		       char * const argv[])
{
	unsigned int seed = rand();
	long *ptr = NULL;
	long size;
	unsigned int offset, expec, read;

	ptr = map_sysmem(MEM_TEST_BASE, MEM_TEST_SIZE);
	for (offset = 0; offset < MEM_TEST_SIZE / sizeof(long);
	     offset ++)
		*(ptr + offset) = rand();

	size = get_ram_size(ptr, MEM_TEST_SIZE);

	srand(seed);

	for (offset = 0; offset < MEM_TEST_SIZE / sizeof(long);
	     offset ++) {
		expec = rand();
		read = *(ptr + offset);
		if (read != expec)
			printf(" ERROR: %p <= %lx, %x, expected %x\n",
			       ptr + offset,
			       MEM_TEST_BASE + (offset * sizeof(long)),
			       read, expec);
	}
	unmap_sysmem(ptr);
	printf("get_ram_size(%x,%x)=%lx\n", MEM_TEST_BASE, MEM_TEST_SIZE, size);
	return 0;
}

U_BOOT_CMD(

	test_ram_size,	1,	1,	do_test_ram_size,
	"test memory",
	""

);

after the first patch: the error is detected@address 0x0, the value become 0x0

=> test_ram_size

    ERROR: 00007f6b0018d008 <= 0, 0, expected 4080601
    get_ram_size(0,100000)=100000

=> test_ram_size

    ERROR: 00007f6b0018d008 <= 0, 0, expected 9a61a0f3
    get_ram_size(0,100000)=100000

after the second patch, all the memory is correctly restored

=> test_ram_size

    get_ram_size(0,100000)=100000

Test on real ARM target
-----------------------

My target is a ARMv7 Cortex A7 board not yet upstreamed with DDR.

I also use the same test by patching SPL:
+ initialize the DDR controller
+ write random content in all the DDR
+ use get_ram_size(0xC0000000, 0x40000000) on all the possible DDR range
+ check content of the DDR

Test 1, 1GB DDR (2 chipsx16bits => 32 bits access) @ 0xC0000000
=> detected size is 0x40000000 (1GB)
=> test OK memory is restored (but test failed without patch 2)

=> the memory is correctly restored for last case (size=max_size)

Test 2, I change the controller parameter to reduce the used bits for DDR
(using one chip => 16 bits access and only 512MB)
=> detected size is 0x20000000 (overlap detected for 0xE0000000 address:

     same physical address 0x0 in DDR than 0xC0000000)

=> test OK memory is restored (test OK with or without patch 2)

=> the memory is already correctly restored for the overlap case
(where I just add comment)

Changes in v2:
- Split patch in 2 part to more easily understood regression
- Solve issue for overlap case: don't restore saved value (invalid) and add comment to avoid to have this issue again

Patrick Delaunay (2):
common/memsize.c: prepare get_ram_size update common/memsize.c: restore content of the base address

 common/memsize.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

2.7.4

Save the content of the base address in a new variable in stack (save_base) to prepare restore this content.

This patch don't modified code behavior and stack usage.

---

Changes in v2:
- Split patch in 2 part to more easily understood regression

 common/memsize.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

index 0fb9ba5..6dc4bb2 100644
@@ -27,7 +27,8 @@ DECLARE_GLOBAL_DATA_PTR;
 long get_ram_size(long *base, long maxsize)
 {
 	volatile long *addr;
-	long           save[32];
+	long           save[31];
+	long           save_base;
 	long           cnt;
 	long           val;
 	long           size;

@@ -43,7 +44,7 @@ long get_ram_size(long *base, long maxsize)
 
 	addr = base;
 	sync();
-	save[i] = *addr;
+	save_base = *addr;
 	sync();
 	*addr = 0;
 

@@ -51,7 +52,7 @@ long get_ram_size(long *base, long maxsize)
 	if ((val = *addr) != 0) {
 		/* Restore the original data before leaving the function. */
 		sync();
-		*addr = save[i];
+		*base = save_base;
 		for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) {
 			addr  = base + cnt;
 			sync();

2.7.4

Applied to u-boot/master, thanks!

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180314/135ce8a5/attachment.sig>

For the last return of function get_ram_size(), when size=maxsize, restore the base address (*base) content.

Add comment for the remaining case to avoid regression:
this case is already correctly handled.

---

Changes in v2:
- Solve issue for overlap case: don't restore saved value (invalid) and add comment to avoid to have this issue again

 common/memsize.c | 7 +++++++
 1 file changed, 7 insertions(+)

index 6dc4bb2..c315275 100644
@@ -77,9 +77,16 @@ long get_ram_size(long *base, long maxsize)
 				addr  = base + cnt;
 				*addr = save[--i];
 			}
+			/* warning: don't restore save_base in this case,
+			 * it is already done in the loop because
+			 * base and base+size share the same physical memory
+			 * and *base is saved after *(base+size) modification
+			 * in first loop
+			 */
 			return (size);
 		}
 	}
+	*base = save_base;
 
 	return (maxsize);
 }

2.7.4

Applied to u-boot/master, thanks!

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180314/14dd3254/attachment.sig>