Patchwork [U-Boot] Make it possible to allocate physical address 0 with lmb

login
register
mail settings
Submitter Grant Likely
Date Jan. 26, 2011, 4:57 p.m.
Message ID <20110126165340.14306.98359.stgit@localhost6.localdomain6>
Download mbox | patch
Permalink /patch/80530/
State RFC
Headers show

Comments

Grant Likely - Jan. 26, 2011, 4:57 p.m.
LMB doesn't currently handle allocating regions based at physical
address 0.  This patch reworks the lmb_alloc functions to return
all ones when allocation fails instead of zero so that callers can
Grant Likely - Jan. 26, 2011, 4:59 p.m.
On Wed, Jan 26, 2011 at 9:57 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> LMB doesn't currently handle allocating regions based at physical
> address 0.  This patch reworks the lmb_alloc functions to return
> all ones when allocation fails instead of zero so that callers can
> differentiate between a region allocated at zero and a failed
> allocation
>
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

Oops, sent this one out too early.  It will not apply against current
mainline since it it based on an older tree.  Still, please take a
look and let me know it this looks appropriate.  I'll get this rebased
and retested against top of tree.

g.

> ---
>  common/image.c |    8 ++++----
>  include/lmb.h  |    1 +
>  lib/lmb.c      |   21 ++++++++++++++++-----
>  3 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/common/image.c b/common/image.c
> index 11504de..c1757bd 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -1053,7 +1053,7 @@ int boot_ramdisk_high (struct lmb *lmb, ulong rd_data, ulong rd_len,
>                        else
>                                *initrd_start = (ulong)lmb_alloc (lmb, rd_len, 0x1000);
>
> -                       if (*initrd_start == 0) {
> +                       if (*initrd_start == LMB_ALLOC_ERROR) {
>                                puts ("ramdisk - allocation error\n");
>                                goto error;
>                        }
> @@ -1228,7 +1228,7 @@ int boot_relocate_fdt (struct lmb *lmb, ulong bootmap_base,
>                of_start = (unsigned long)lmb_alloc_base(lmb, of_len, 0x1000,
>                                (CONFIG_SYS_BOOTMAPSZ + bootmap_base));
>
> -               if (of_start == 0) {
> +               if (of_start == (unsigned long)LMB_ALLOC_ERROR) {
>                        puts("device tree - allocation error\n");
>                        goto error;
>                }
> @@ -1614,7 +1614,7 @@ int boot_get_cmdline (struct lmb *lmb, ulong *cmd_start, ulong *cmd_end,
>        cmdline = (char *)(ulong)lmb_alloc_base(lmb, CONFIG_SYS_BARGSIZE, 0xf,
>                                         CONFIG_SYS_BOOTMAPSZ + bootmap_base);
>
> -       if (cmdline == NULL)
> +       if (cmdline == (char*)(ulong)LMB_ALLOC_ERROR)
>                return -1;
>
>        if ((s = getenv("bootargs")) == NULL)
> @@ -1651,7 +1651,7 @@ int boot_get_kbd (struct lmb *lmb, bd_t **kbd, ulong bootmap_base)
>  {
>        *kbd = (bd_t *)(ulong)lmb_alloc_base(lmb, sizeof(bd_t), 0xf,
>                                      CONFIG_SYS_BOOTMAPSZ + bootmap_base);
> -       if (*kbd == NULL)
> +       if (*kbd == (bd_t *)(ulong)LMB_ALLOC_ERROR)
>                return -1;
>
>        **kbd = *(gd->bd);
> diff --git a/include/lmb.h b/include/lmb.h
> index 43082a3..f927b86 100644
> --- a/include/lmb.h
> +++ b/include/lmb.h
> @@ -15,6 +15,7 @@
>  */
>
>  #define MAX_LMB_REGIONS 8
> +#define LMB_ALLOC_ERROR ((phys_addr_t) ~0)
>
>  struct lmb_property {
>        phys_addr_t base;
> diff --git a/lib/lmb.c b/lib/lmb.c
> index c5e75fb..de3c325 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -114,6 +114,9 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
>        unsigned long coalesced = 0;
>        long adjacent, i;
>
> +       debug("lmb_add_region(lmb=%x, base=%.8lx, size=%.8lx)\n",
> +               (ulong)rgn, (ulong)base, (ulong)size);
> +
>        if ((rgn->cnt == 1) && (rgn->region[0].size == 0)) {
>                rgn->region[0].base = base;
>                rgn->region[0].size = size;
> @@ -266,9 +269,11 @@ phys_addr_t lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phys_
>
>        alloc = __lmb_alloc_base(lmb, size, align, max_addr);
>
> -       if (alloc == 0)
> -               printf("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n",
> +       if (alloc == LMB_ALLOC_ERROR) {
> +               debug("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n",
>                      (ulong)size, (ulong)max_addr);
> +               lmb_dump_all(lmb);
> +       }
>
>        return alloc;
>  }
> @@ -289,10 +294,15 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phy
>        phys_addr_t base = 0;
>        phys_addr_t res_base;
>
> +       debug("lmb_alloc_base(lmb=%lx, size=%lx, align=%li, max_addr=%lx)\n",
> +               (ulong)lmb, (ulong)size, (ulong)align, (ulong)max_addr);
> +
>        for (i = lmb->memory.cnt-1; i >= 0; i--) {
>                phys_addr_t lmbbase = lmb->memory.region[i].base;
>                phys_size_t lmbsize = lmb->memory.region[i].size;
>
> +               debug("--loop i=%i, lmbbase=%lx, lmbsize=%lx\n",
> +                       i, (ulong)lmbbase, (ulong)lmbsize);
>                if (lmbsize < size)
>                        continue;
>                if (max_addr == LMB_ALLOC_ANYWHERE)
> @@ -303,14 +313,15 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phy
>                } else
>                        continue;
>
> -               while (base && lmbbase <= base) {
> +               while (lmbbase <= base) {
>                        j = lmb_overlaps_region(&lmb->reserved, base, size);
> +                       debug("----loop base=%lx, j=%i\n", (ulong)base, j);
>                        if (j < 0) {
>                                /* This area isn't reserved, take it */
>                                if (lmb_add_region(&lmb->reserved, base,
>                                                        lmb_align_up(size,
>                                                                align)) < 0)
> -                                       return 0;
> +                                       return LMB_ALLOC_ERROR;
>                                return base;
>                        }
>                        res_base = lmb->reserved.region[j].base;
> @@ -319,7 +330,7 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phy
>                        base = lmb_align_down(res_base - size, align);
>                }
>        }
> -       return 0;
> +       return LMB_ALLOC_ERROR;
>  }
>
>  int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)
>
>
Wolfgang Denk - Jan. 26, 2011, 6:30 p.m.
Dear Grant Likely,

In message <20110126165340.14306.98359.stgit@localhost6.localdomain6> you wrote:
> LMB doesn't currently handle allocating regions based at physical
> address 0.  This patch reworks the lmb_alloc functions to return
> all ones when allocation fails instead of zero so that callers can
> differentiate between a region allocated at zero and a failed
> allocation
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
...
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -114,6 +114,9 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
...
> -	if (alloc == 0)
> -		printf("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n",
> +	if (alloc == LMB_ALLOC_ERROR) {
> +		debug("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n",
>  		      (ulong)size, (ulong)max_addr);
> +		lmb_dump_all(lmb);
> +	}

Do you intentionally change the printf() into a debug() here?  If yes,
what is your rationale for not printing a proper error message in
production code?

Best regards,

Wolfgang Denk
Grant Likely - Jan. 26, 2011, 6:39 p.m.
On Wed, Jan 26, 2011 at 11:30 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Grant Likely,
>
> In message <20110126165340.14306.98359.stgit@localhost6.localdomain6> you wrote:
>> LMB doesn't currently handle allocating regions based at physical
>> address 0.  This patch reworks the lmb_alloc functions to return
>> all ones when allocation fails instead of zero so that callers can
>> differentiate between a region allocated at zero and a failed
>> allocation
>>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ...
>> --- a/lib/lmb.c
>> +++ b/lib/lmb.c
>> @@ -114,6 +114,9 @@ static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
> ...
>> -     if (alloc == 0)
>> -             printf("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n",
>> +     if (alloc == LMB_ALLOC_ERROR) {
>> +             debug("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n",
>>                     (ulong)size, (ulong)max_addr);
>> +             lmb_dump_all(lmb);
>> +     }
>
> Do you intentionally change the printf() into a debug() here?  If yes,
> what is your rationale for not printing a proper error message in
> production code?

No, that was a mistake.  I'll correct it.

g.

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> "I knew then (in 1970) that a 4-kbyte minicomputer would cost as much
> as a house. So I reasoned  that  after  college,  I'd  have  to  live
> cheaply in an apartment and put all my money into owning a computer."
>      - Apple co-founder Steve Wozniak, EE Times, June 6, 1988, pg 45
>

Patch

differentiate between a region allocated at zero and a failed
allocation

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
 common/image.c |    8 ++++----
 include/lmb.h  |    1 +
 lib/lmb.c      |   21 ++++++++++++++++-----
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/common/image.c b/common/image.c
index 11504de..c1757bd 100644
--- a/common/image.c
+++ b/common/image.c
@@ -1053,7 +1053,7 @@  int boot_ramdisk_high (struct lmb *lmb, ulong rd_data, ulong rd_len,
 			else
 				*initrd_start = (ulong)lmb_alloc (lmb, rd_len, 0x1000);
 
-			if (*initrd_start == 0) {
+			if (*initrd_start == LMB_ALLOC_ERROR) {
 				puts ("ramdisk - allocation error\n");
 				goto error;
 			}
@@ -1228,7 +1228,7 @@  int boot_relocate_fdt (struct lmb *lmb, ulong bootmap_base,
 		of_start = (unsigned long)lmb_alloc_base(lmb, of_len, 0x1000,
 				(CONFIG_SYS_BOOTMAPSZ + bootmap_base));
 
-		if (of_start == 0) {
+		if (of_start == (unsigned long)LMB_ALLOC_ERROR) {
 			puts("device tree - allocation error\n");
 			goto error;
 		}
@@ -1614,7 +1614,7 @@  int boot_get_cmdline (struct lmb *lmb, ulong *cmd_start, ulong *cmd_end,
 	cmdline = (char *)(ulong)lmb_alloc_base(lmb, CONFIG_SYS_BARGSIZE, 0xf,
 					 CONFIG_SYS_BOOTMAPSZ + bootmap_base);
 
-	if (cmdline == NULL)
+	if (cmdline == (char*)(ulong)LMB_ALLOC_ERROR)
 		return -1;
 
 	if ((s = getenv("bootargs")) == NULL)
@@ -1651,7 +1651,7 @@  int boot_get_kbd (struct lmb *lmb, bd_t **kbd, ulong bootmap_base)
 {
 	*kbd = (bd_t *)(ulong)lmb_alloc_base(lmb, sizeof(bd_t), 0xf,
 				      CONFIG_SYS_BOOTMAPSZ + bootmap_base);
-	if (*kbd == NULL)
+	if (*kbd == (bd_t *)(ulong)LMB_ALLOC_ERROR)
 		return -1;
 
 	**kbd = *(gd->bd);
diff --git a/include/lmb.h b/include/lmb.h
index 43082a3..f927b86 100644
--- a/include/lmb.h
+++ b/include/lmb.h
@@ -15,6 +15,7 @@ 
  */
 
 #define MAX_LMB_REGIONS 8
+#define LMB_ALLOC_ERROR ((phys_addr_t) ~0)
 
 struct lmb_property {
 	phys_addr_t base;
diff --git a/lib/lmb.c b/lib/lmb.c
index c5e75fb..de3c325 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -114,6 +114,9 @@  static long lmb_add_region(struct lmb_region *rgn, phys_addr_t base, phys_size_t
 	unsigned long coalesced = 0;
 	long adjacent, i;
 
+	debug("lmb_add_region(lmb=%x, base=%.8lx, size=%.8lx)\n",
+		(ulong)rgn, (ulong)base, (ulong)size);
+
 	if ((rgn->cnt == 1) && (rgn->region[0].size == 0)) {
 		rgn->region[0].base = base;
 		rgn->region[0].size = size;
@@ -266,9 +269,11 @@  phys_addr_t lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phys_
 
 	alloc = __lmb_alloc_base(lmb, size, align, max_addr);
 
-	if (alloc == 0)
-		printf("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n",
+	if (alloc == LMB_ALLOC_ERROR) {
+		debug("ERROR: Failed to allocate 0x%lx bytes below 0x%lx.\n",
 		      (ulong)size, (ulong)max_addr);
+		lmb_dump_all(lmb);
+	}
 
 	return alloc;
 }
@@ -289,10 +294,15 @@  phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phy
 	phys_addr_t base = 0;
 	phys_addr_t res_base;
 
+	debug("lmb_alloc_base(lmb=%lx, size=%lx, align=%li, max_addr=%lx)\n",
+		(ulong)lmb, (ulong)size, (ulong)align, (ulong)max_addr);
+
 	for (i = lmb->memory.cnt-1; i >= 0; i--) {
 		phys_addr_t lmbbase = lmb->memory.region[i].base;
 		phys_size_t lmbsize = lmb->memory.region[i].size;
 
+		debug("--loop i=%i, lmbbase=%lx, lmbsize=%lx\n",
+			i, (ulong)lmbbase, (ulong)lmbsize);
 		if (lmbsize < size)
 			continue;
 		if (max_addr == LMB_ALLOC_ANYWHERE)
@@ -303,14 +313,15 @@  phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phy
 		} else
 			continue;
 
-		while (base && lmbbase <= base) {
+		while (lmbbase <= base) {
 			j = lmb_overlaps_region(&lmb->reserved, base, size);
+			debug("----loop base=%lx, j=%i\n", (ulong)base, j);
 			if (j < 0) {
 				/* This area isn't reserved, take it */
 				if (lmb_add_region(&lmb->reserved, base,
 							lmb_align_up(size,
 								align)) < 0)
-					return 0;
+					return LMB_ALLOC_ERROR;
 				return base;
 			}
 			res_base = lmb->reserved.region[j].base;
@@ -319,7 +330,7 @@  phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size, ulong align, phy
 			base = lmb_align_down(res_base - size, align);
 		}
 	}
-	return 0;
+	return LMB_ALLOC_ERROR;
 }
 
 int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr)