Skip to contentSkip to conversation

Revert "common/memsize.c: Simplify RAM size detection"

PATCH New Hans de Goede 4 participants 6 messages Feb 9, 2016 – Feb 10, 2016

lore
Share & tools
lore

Hans de Goede

author
#0 lore raw

PATCH 1/1 Revert "common/memsize.c: Simplify RAM size detection"

This commit breaks bootup on sunxi boards, the get stuck when running the main u-boot binary at:

CPU:   Allwinner H3 (SUN8I)
I2C:   ready

DRAM:

This reverts commit 8e7cba048baae68ee0916a8f52b4304277328d5e.

Signed-off-by Hans de Goede <[email protected]>
---
 common/memsize.c | 47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)
common/memsize.c +26 -21
index 5c0d279..0fb9ba5 100644
@@ -33,28 +33,38 @@ long get_ram_size(long *base, long maxsize)
 	long           size;
 	int            i = 0;
 
-	for (cnt = (maxsize / sizeof(long)) >> 1; cnt >= 0; cnt >>= 1) {
+	for (cnt = (maxsize / sizeof(long)) >> 1; cnt > 0; cnt >>= 1) {
 		addr = base + cnt;	/* pointer arith! */
 		sync();
-		save[i] = *addr;
+		save[i++] = *addr;
 		sync();
-		if (cnt) {
-			i++;
-			*addr = ~cnt;
-		} else {
-			*addr = 0;
-		}
+		*addr = ~cnt;
 	}
 
+	addr = base;
+	sync();
+	save[i] = *addr;
+	sync();
+	*addr = 0;
+
 	sync();
-	cnt = 0;
-	do {
+	if ((val = *addr) != 0) {
+		/* Restore the original data before leaving the function. */
+		sync();
+		*addr = save[i];
+		for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) {
+			addr  = base + cnt;
+			sync();
+			*addr = save[--i];
+		}
+		return (0);
+	}
+
+	for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) {
 		addr = base + cnt;	/* pointer arith! */
 		val = *addr;
-		*addr = save[i--];
-		sync();
-		if (((cnt == 0) && (val != 0)) ||
-		    ((cnt != 0) && (val != ~cnt))) {
+		*addr = save[--i];
+		if (val != ~cnt) {
 			size = cnt * sizeof(long);
 			/*
 			 * Restore the original data

@@ -64,16 +74,11 @@ long get_ram_size(long *base, long maxsize)
 			     cnt < maxsize / sizeof(long);
 			     cnt <<= 1) {
 				addr  = base + cnt;
-				*addr = save[i--];
+				*addr = save[--i];
 			}
 			return (size);
 		}
-
-		if (cnt)
-			cnt = cnt << 1;
-		else
-			cnt = 1;
-	} while (cnt < maxsize / sizeof(long));
+	}
 
 	return (maxsize);
 }
Signature · 1 line
2.5.0
#1 lore raw

PATCH 1/1 Revert "common/memsize.c: Simplify RAM size detection"

Hi Hans,

I sent a fixup patch yesterday (I managed somehow to send an outdated patch), did you try the fixup patch?

http://lists.denx.de/pipermail/u-boot/2016-February/245080.html
Pe 9 feb. 2016 11:38 p.m., "Hans de Goede" <[email protected]> a scris:

Quoted 95 lines

This commit breaks bootup on sunxi boards, the get stuck when running the main u-boot binary at:

CPU:   Allwinner H3 (SUN8I)
I2C:   ready

DRAM:

This reverts commit 8e7cba048baae68ee0916a8f52b4304277328d5e.

Signed-off-by Hans de Goede <[email protected]>
---
 common/memsize.c | 47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)
common/memsize.c +24 -14
index 5c0d279..0fb9ba5 100644
@@ -33,28 +33,38 @@ long get_ram_size(long *base, long maxsize)
        long           size;
        int            i = 0;
 
-       for (cnt = (maxsize / sizeof(long)) >> 1; cnt >= 0; cnt >>= 1) {
+       for (cnt = (maxsize / sizeof(long)) >> 1; cnt > 0; cnt >>= 1) {
                addr = base + cnt;      /* pointer arith! */
                sync();
-               save[i] = *addr;
+               save[i++] = *addr;
                sync();
-               if (cnt) {
-                       i++;
-                       *addr = ~cnt;
-               } else {
-                       *addr = 0;
-               }
+               *addr = ~cnt;
        }
 
+       addr = base;
+       sync();
+       save[i] = *addr;
+       sync();
+       *addr = 0;
+
        sync();
-       cnt = 0;
-       do {
+       if ((val = *addr) != 0) {
+               /* Restore the original data before leaving the function.
 */
+               sync();
+               *addr = save[i];
+               for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) {
+                       addr  = base + cnt;
+                       sync();
+                       *addr = save[--i];
+               }
+               return (0);
+       }
+
+       for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) {
                addr = base + cnt;      /* pointer arith! */
                val = *addr;
-               *addr = save[i--];
-               sync();
-               if (((cnt == 0) && (val != 0)) ||
-                   ((cnt != 0) && (val != ~cnt))) {
+               *addr = save[--i];
+               if (val != ~cnt) {
                        size = cnt * sizeof(long);
                        /*

* Restore the original data

common/memsize.c +2 -8
@@ -64,16 +74,11 @@ long get_ram_size(long *base, long maxsize)
                             cnt < maxsize / sizeof(long);
                             cnt <<= 1) {
                                addr  = base + cnt;
-                               *addr = save[i--];
+                               *addr = save[--i];
                        }
                        return (size);
                }
-
-               if (cnt)
-                       cnt = cnt << 1;
-               else
-                       cnt = 1;
-       } while (cnt < maxsize / sizeof(long));
+       }
 
        return (maxsize);
 }
--

2.5.0

author
#2 lore raw

PATCH 1/1 Revert "common/memsize.c: Simplify RAM size detection"

Hi,

Quoted 4 lines · Eddy Petri?or

Hi Hans,

I sent a fixup patch yesterday (I managed somehow to send an outdated patch),

Ah I did not notice that one.

Quoted 1 line

did you try the fixup patch?

I just did try it, same results as Hannes the hang is gone, but now the RAM size is misdedetected (it detects 2 GiB on a 1 GiB board).

It seems this code was as it was for a reason and that a revert of your simplification is probably the best solution.

Regards,

Hans

Quoted 100 lines

http://lists.denx.de/pipermail/u-boot/2016-February/245080.html
Pe 9 feb. 2016 11:38 p.m., "Hans de Goede" <[email protected]> a scris:

Quoted 95 lines

This commit breaks bootup on sunxi boards, the get stuck when running the main u-boot binary at:

CPU:   Allwinner H3 (SUN8I)
I2C:   ready

DRAM:

This reverts commit 8e7cba048baae68ee0916a8f52b4304277328d5e.

Signed-off-by Hans de Goede <[email protected]>
---
  common/memsize.c | 47 ++++++++++++++++++++++++++---------------------
  1 file changed, 26 insertions(+), 21 deletions(-)
common/memsize.c +24 -14
index 5c0d279..0fb9ba5 100644
@@ -33,28 +33,38 @@ long get_ram_size(long *base, long maxsize)
         long           size;
         int            i = 0;
 
-       for (cnt = (maxsize / sizeof(long)) >> 1; cnt >= 0; cnt >>= 1) {
+       for (cnt = (maxsize / sizeof(long)) >> 1; cnt > 0; cnt >>= 1) {
                 addr = base + cnt;      /* pointer arith! */
                 sync();
-               save[i] = *addr;
+               save[i++] = *addr;
                 sync();
-               if (cnt) {
-                       i++;
-                       *addr = ~cnt;
-               } else {
-                       *addr = 0;
-               }
+               *addr = ~cnt;
         }
 
+       addr = base;
+       sync();
+       save[i] = *addr;
+       sync();
+       *addr = 0;
+
         sync();
-       cnt = 0;
-       do {
+       if ((val = *addr) != 0) {
+               /* Restore the original data before leaving the function.
 */
+               sync();
+               *addr = save[i];
+               for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) {
+                       addr  = base + cnt;
+                       sync();
+                       *addr = save[--i];
+               }
+               return (0);
+       }
+
+       for (cnt = 1; cnt < maxsize / sizeof(long); cnt <<= 1) {
                 addr = base + cnt;      /* pointer arith! */
                 val = *addr;
-               *addr = save[i--];
-               sync();
-               if (((cnt == 0) && (val != 0)) ||
-                   ((cnt != 0) && (val != ~cnt))) {
+               *addr = save[--i];
+               if (val != ~cnt) {
                         size = cnt * sizeof(long);
                         /*

* Restore the original data

common/memsize.c +2 -8
@@ -64,16 +74,11 @@ long get_ram_size(long *base, long maxsize)
                              cnt < maxsize / sizeof(long);
                              cnt <<= 1) {
                                 addr  = base + cnt;
-                               *addr = save[i--];
+                               *addr = save[--i];
                         }
                         return (size);
                 }
-
-               if (cnt)
-                       cnt = cnt << 1;
-               else
-                       cnt = 1;
-       } while (cnt < maxsize / sizeof(long));
+       }
 
         return (maxsize);
  }
--

2.5.0

#3 lore raw

PATCH 1/1 Revert "common/memsize.c: Simplify RAM size detection"

2016-02-10 10:20 GMT+02:00 Hans de Goede <[email protected]>:

Quoted 12 lines

Hi,

Quoted 5 lines · Eddy Petri?or

Hi Hans,

I sent a fixup patch yesterday (I managed somehow to send an outdated patch),

Ah I did not notice that one.

Quoted 8 lines
Quoted 1 line

did you try the fixup patch?

I just did try it, same results as Hannes the hang is gone, but now the RAM size is misdedetected (it detects 2 GiB on a 1 GiB board).

It seems this code was as it was for a reason and that a revert of your simplification is probably the best solution.

OK, agreed, that makes sense. My buggy HW (DDR unstable) doesn't help.

I will probably rework the patch and target the v2016.05 release.

#4 lore raw

PATCH 1/1 Revert "common/memsize.c: Simplify RAM size detection"

In reply to Hans de Goede

Dear Hans,

Quoted 3 lines · In message <[email protected]> you wrote:

It seems this code was as it was for a reason and that a revert of your simplification is probably the best solution.

Indeed. Over the past 15 years I have seen several attempts to improve or simplify the mesize code. Most did not make it into mainline as they worked for the poster, but caused problems on some other boards.

We had a similar "simplification" in 2012, see commit b8496cce and revert in 3ab270d5, but all these attempts caused problems of some kind, so the basic structure of the code is still exactly the same as in the original version of 2004 (commit c83bf6a2).

Probably this is just Perfect Code (TM) which cannot improved further :-)

Best regards,

Wolfgang Denk

Signature · 7 lines
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The word "fit", as I understand it, means "appropriate to a purpose",
and I would say the body of the Dean is supremely appropriate to  the
purpose of sitting around all day and eating big heavy meals.
                                 - Terry Pratchett, _Moving Pictures_
#5 lore raw

PATCH 1/1 Revert "common/memsize.c: Simplify RAM size detection"

In reply to Hans de Goede
Quoted 10 lines · Hans de Goede

This commit breaks bootup on sunxi boards, the get stuck when running the main u-boot binary at:

CPU:   Allwinner H3 (SUN8I)
I2C:   ready

DRAM:

This reverts commit 8e7cba048baae68ee0916a8f52b4304277328d5e.

Signed-off-by Hans de Goede <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160210/17ab767d/attachment.sig>