Re: [U-Boot] [PATCHv4] [RFC] DM: early_malloc for DM added.

2012-09-21 Thread Tomas Hlavacek 
Hello all,

I have sent a new version. Although I tried to take into account all
the opinions and comments I might have missed something.

On Wed, Sep 19, 2012 at 1:29 AM, Graeme Russ graeme.r...@gmail.com wrote:

 I'd say, pull out the modification of global data into separate patch and 
 put it
 before this patch. That'd make review of the core code much easier.

 NAK - The addition of the global data member is intrinsic to the early
 malloc implmentaion. Keep them together

Yes, I think that in this case one does not make sense with the other.
I kept them rather together.


 +
 +struct early_heap_header *early_brk(size_t size)
 + __attribute__((weak, alias(def_early_brk)));

 what about using (it needs linux/compiler.h):

 __weak struct early_heap_header *early_brk(size_t size)
 {
 ...
 body
 ...
 }

 We already have a lot of the former - I prefer not to add additional
 semantics (unless you want to do a wholesale search/replace ;))

This time I used the shorter / newer variant. Hope this would be better.

Tomas

-- 
Tomáš Hlaváček tmshl...@gmail.com
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCHv4] [RFC] DM: early_malloc for DM added.

2012-09-19 Thread Tom Rini 
On Wed, Sep 19, 2012 at 09:29:04AM +1000, Graeme Russ wrote:
 Hi Marek,
 
 On Tue, Sep 18, 2012 at 8:57 PM, Marek Vasut ma...@denx.de wrote:
  Dear Tomas Hlavacek,
[snip]
  +struct early_heap_header *early_brk(size_t size)
  + __attribute__((weak, alias(def_early_brk)));
 
  what about using (it needs linux/compiler.h):
 
  __weak struct early_heap_header *early_brk(size_t size)
  {
  ...
  body
  ...
  }
 
 We already have a lot of the former - I prefer not to add additional
 semantics (unless you want to do a wholesale search/replace ;))

Migrating everyone but the real tricky uses (alias =2 functions to one
weak dummy) to the linux/compiler.h semantics is something on my TODO
list from ages ago.  So yes please, unless it's a complex case, find and
use the __shortform that linux/compiler.h and company provide (and a
git grep for your attribute when in doubt, sometimes it's in
linux/compiler-gcc.h or so).

And a general I know everyone should know but please, checkpatch.pl your
work before posting.  Getting everyone doing DM series patches to use
patman might be a little bit more overhead (sorry!) but it'll help with
a lot of the little things like formatting and some of the bigger things
like changelog tracking.  Don't have to switch but it might make your
lives easier.

-- 
Tom
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCHv4] [RFC] DM: early_malloc for DM added.

2012-09-18 Thread Marek Vasut 
Dear Tomas Hlavacek,

 early_malloc for DM with support for more heaps and lightweight
 first heap on stack.
 
 Adaptation layer for seamless calling of early_malloc or dlmalloc from
 DM based on init stage added (dmmalloc() and related functions).
 
 Signed-off-by: Tomas Hlavacek tmshl...@gmail.com
 ---

It looks mostly OK, few comments

I'd say, pull out the modification of global data into separate patch and put 
it 
before this patch. That'd make review of the core code much easier.

[...]

 +
 +#include common.h /* for ROUND_UP */
 +#include asm/u-boot.h
 +#include asm/global_data.h /* for gd_t and gd */
 +#include asm/types.h /* for phys_addr_t and size_addt_t */
 +
 +#include dmmalloc.h
 +#include malloc.h
 +
 +DECLARE_GLOBAL_DATA_PTR;
 +
 +#ifdef CONFIG_SYS_EARLY_MALLOC
 +static struct early_heap_header *def_early_brk(size_t size)
 +{
 + struct early_heap_header *h =
 + (struct early_heap_header *)CONFIG_SYS_EARLY_HEAP_ADDR;
 +
 + h-free_space_pointer = (void *)(roundup(
 + (phys_addr_t)CONFIG_SYS_EARLY_HEAP_ADDR +
 + sizeof(struct early_heap_header),
 + sizeof(phys_addr_t)));
 + h-free_bytes = size - roundup(sizeof(struct early_heap_header),
 + sizeof(phys_addr_t));
 + h-next_early_heap = NULL;
 +
 + return h;
 +}
 +
 +struct early_heap_header *early_brk(size_t size)
 + __attribute__((weak, alias(def_early_brk)));

what about using (it needs linux/compiler.h):

__weak struct early_heap_header *early_brk(size_t size)
{
...
body
...
}

 +void *early_malloc(size_t size)
 +{
 + phys_addr_t addr;
 + struct early_heap_header *h;
 +
 + /* Align size. */
 + size = roundup(size, sizeof(phys_addr_t));
 +
 + /* Choose early_heap with enough space. */
 + h = gd-early_heap_first;
 + while ((h-free_bytes  size)  (h-next_early_heap != NULL))
 + h = h-next_early_heap;
 +
 + if (h-free_bytes  size) {
 + debug(Early heap overflow. Heap %p, free %d, required %d.,
 + h, h-free_bytes, size);
 + return NULL;
 + }
 +
 + /* Choose block beginning address and mark next free space. */
 + addr = (phys_addr_t)h-free_space_pointer;
 +
 + h-free_space_pointer += size;
 + h-free_bytes -= size;
 +
 + return (void *)addr;
 +}
 +
 +static int is_early_malloc_active(void)
 +{
 + if (gd-flags  GD_FLG_RELOC)
 + return 0;
 +
 + return 1;
 +}
 +
 +#endif /* CONFIG_SYS_EARLY_MALLOC */
 +
 +void *dmmalloc(size_t size)
 +{
 +#ifdef CONFIG_SYS_EARLY_MALLOC
 + if (is_early_malloc_active())
 + return early_malloc(size);
 +#endif /* CONFIG_SYS_EARLY_MALLOC */

Or you can implement empty prototypes for these functions in case CONFIG_SYS 
... 
isn't defined to punt this preprocessor bloat.

 + return malloc(size);
 +}

[...]

 diff --git a/include/configs/zipitz2.h b/include/configs/zipitz2.h
 index 26204af..5cd0dcb 100644
 --- a/include/configs/zipitz2.h
 +++ b/include/configs/zipitz2.h
 @@ -176,8 +176,13 @@ unsigned char zipitz2_spi_read(void);
 
  #define  CONFIG_SYS_LOAD_ADDRCONFIG_SYS_DRAM_BASE
 
 +#define CONFIG_SYS_EARLY_HEAP_ADDR (GENERATED_GBL_DATA_SIZE + \
 + PHYS_SDRAM_1)
 +#define CONFIG_SYS_EARLY_HEAP_SIZE 256
 +

1) Pull this file into separate patch and order it afterwards this patch.
2) You're putting your early thingie into SDRAM, which works on PXA (sadly) ... 
looking through the PXA init code, it needs cleanup, damn

... there's no real hint so far, just a rant.

  #define CONFIG_SYS_SDRAM_BASEPHYS_SDRAM_1
 -#define  CONFIG_SYS_INIT_SP_ADDR (GENERATED_GBL_DATA_SIZE + 
PHYS_SDRAM_1 +
 2048) +#define CONFIG_SYS_INIT_SP_ADDR
 (GENERATED_GBL_DATA_SIZE 
+ \
 + CONFIG_SYS_EARLY_HEAP_SIZE + PHYS_SDRAM_1 + 2048)
 
  /*
   * NOR FLASH
 @@ -260,4 +265,9 @@ unsigned char zipitz2_spi_read(void);
  #define CONFIG_SYS_MCIO0_VAL 0x0001430f
  #define CONFIG_SYS_MCIO1_VAL 0x0001430f

[...]
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCHv4] [RFC] DM: early_malloc for DM added.

2012-09-18 Thread Graeme Russ 
Hi Tomas

On Tue, Sep 18, 2012 at 5:13 PM, Tomas Hlavacek tmshl...@gmail.com wrote:
 early_malloc for DM with support for more heaps and lightweight
 first heap on stack.

Technically, you are not putting the first heap on the stack - you are
sacrificing some early stack space to create the early heap


 Adaptation layer for seamless calling of early_malloc or dlmalloc from
 DM based on init stage added (dmmalloc() and related functions).

 Signed-off-by: Tomas Hlavacek tmshl...@gmail.com
 ---
  arch/arm/include/asm/config.h |3 +
  arch/arm/include/asm/global_data.h|1 +
  arch/arm/lib/board.c  |7 ++
  arch/avr32/include/asm/global_data.h  |1 +
  arch/avr32/lib/board.c|6 ++
  arch/blackfin/include/asm/global_data.h   |1 +
  arch/blackfin/lib/board.c |7 ++
  arch/m68k/include/asm/global_data.h   |1 +
  arch/m68k/lib/board.c |7 ++
  arch/microblaze/include/asm/global_data.h |1 +
  arch/microblaze/lib/board.c   |8 ++
  arch/mips/include/asm/global_data.h   |1 +
  arch/mips/lib/board.c |7 ++
  arch/nds32/include/asm/global_data.h  |1 +
  arch/nds32/lib/board.c|6 ++
  arch/nios2/include/asm/global_data.h  |1 +
  arch/nios2/lib/board.c|6 ++
  arch/openrisc/include/asm/global_data.h   |1 +
  arch/openrisc/lib/board.c |6 ++
  arch/powerpc/include/asm/global_data.h|1 +
  arch/powerpc/lib/board.c  |6 ++
  arch/sandbox/include/asm/global_data.h|1 +
  arch/sandbox/lib/board.c  |6 ++
  arch/sh/include/asm/global_data.h |1 +
  arch/sh/lib/board.c   |6 ++
  arch/sparc/include/asm/global_data.h  |1 +
  arch/sparc/lib/board.c|6 ++
  arch/x86/include/asm/global_data.h|1 +
  arch/x86/lib/board.c  |   14 
  common/Makefile   |1 +
  common/dmmalloc.c |  128 
 +
  include/configs/zipitz2.h |   12 ++-

WTF! - Move this out into another patch

  include/dmmalloc.h|   51 
  33 files changed, 306 insertions(+), 1 deletion(-)
  create mode 100644 common/dmmalloc.c
  create mode 100644 include/dmmalloc.h

 diff --git a/arch/arm/include/asm/config.h b/arch/arm/include/asm/config.h
 index c60dba2..8e2f67b 100644
 --- a/arch/arm/include/asm/config.h
 +++ b/arch/arm/include/asm/config.h
 @@ -23,4 +23,7 @@

  #define CONFIG_LMB
  #define CONFIG_SYS_BOOT_RAMDISK_HIGH
 +
 +#define CONFIG_SYS_EARLY_MALLOC
  #endif
 +

Why are you adding this define to ARM and nothing else? Shouldn't it be an
all-in or none-in proposition?

(and why the stray eol white-space?)

 diff --git a/arch/arm/include/asm/global_data.h 
 b/arch/arm/include/asm/global_data.h
 index c3ff789..8563d49 100644
 --- a/arch/arm/include/asm/global_data.h
 +++ b/arch/arm/include/asm/global_data.h
 @@ -84,6 +84,7 @@ typedef   struct  global_data {
 unsigned long   post_log_res; /* success of POST test */
 unsigned long   post_init_f_time; /* When post_init_f started */
  #endif
 +   void*early_heap_first; /* early heap for early_malloc */
  } gd_t;

early_heap_first is only used if CONFIG_SYS_EARLY_MALLOC is defined, so
wrap a #ifdef around it. This will also help to detect unintended usage
if CONFIG_SYS_EARLY_MALLOC is not defined.


  /*
 diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
 index 500e216..33e74da 100644
 --- a/arch/arm/lib/board.c
 +++ b/arch/arm/lib/board.c
 @@ -52,6 +52,7 @@
  #include fdtdec.h
  #include post.h
  #include logbuff.h
 +#include dmmalloc.h

  #ifdef CONFIG_BITBANGMII
  #include miiphy.h
 @@ -273,6 +274,12 @@ void board_init_f(ulong bootflag)

 memset((void *)gd, 0, sizeof(gd_t));

 +
 +#ifdef CONFIG_SYS_EARLY_MALLOC
 +   /* Initialize early_malloc */
 +   gd-early_heap_first = early_brk(CONFIG_SYS_EARLY_HEAP_SIZE);
 +#endif /* CONFIG_SYS_EARLY_MALLOC */
 +

Did you checkpatch? You've added an additional blank line (for a total of
two)

[snip]

 diff --git a/arch/x86/lib/board.c b/arch/x86/lib/board.c
 index 5f0b62c..b609dbe 100644
 --- a/arch/x86/lib/board.c
 +++ b/arch/x86/lib/board.c
 @@ -40,6 +40,8 @@
  #include asm/init_helpers.h
  #include asm/init_wrappers.h

 +#include dmmalloc.h
 +
  /*
   * Breath some life into the board...
   *
 @@ -85,6 +87,17 @@
  typedef int (init_fnc_t) (void);

  /*
 + * Initialize early heap (when enabled by config).
 + */
 +static void early_malloc_init(void)
 +{
 +#ifdef CONFIG_SYS_EARLY_MALLOC
 +   /* Initialize early_malloc */
 +   gd-early_heap_first = early_brk(CONFIG_SYS_EARLY_HEAP_SIZE);
 +#endif /* CONFIG_SYS_EARLY_MALLOC */
 +}
 +
 +/*
   * init_sequence_f is the list of init

Re: [U-Boot] [PATCHv4] [RFC] DM: early_malloc for DM added.

2012-09-18 Thread Graeme Russ 
Hi Marek,

On Tue, Sep 18, 2012 at 8:57 PM, Marek Vasut ma...@denx.de wrote:
 Dear Tomas Hlavacek,

 early_malloc for DM with support for more heaps and lightweight
 first heap on stack.

 Adaptation layer for seamless calling of early_malloc or dlmalloc from
 DM based on init stage added (dmmalloc() and related functions).

 Signed-off-by: Tomas Hlavacek tmshl...@gmail.com
 ---

 It looks mostly OK, few comments

 I'd say, pull out the modification of global data into separate patch and put 
 it
 before this patch. That'd make review of the core code much easier.

NAK - The addition of the global data member is intrinsic to the early
malloc implmentaion. Keep them together


 [...]

 +
 +#include common.h /* for ROUND_UP */
 +#include asm/u-boot.h
 +#include asm/global_data.h /* for gd_t and gd */
 +#include asm/types.h /* for phys_addr_t and size_addt_t */
 +
 +#include dmmalloc.h
 +#include malloc.h
 +
 +DECLARE_GLOBAL_DATA_PTR;
 +
 +#ifdef CONFIG_SYS_EARLY_MALLOC
 +static struct early_heap_header *def_early_brk(size_t size)
 +{
 + struct early_heap_header *h =
 + (struct early_heap_header *)CONFIG_SYS_EARLY_HEAP_ADDR;
 +
 + h-free_space_pointer = (void *)(roundup(
 + (phys_addr_t)CONFIG_SYS_EARLY_HEAP_ADDR +
 + sizeof(struct early_heap_header),
 + sizeof(phys_addr_t)));
 + h-free_bytes = size - roundup(sizeof(struct early_heap_header),
 + sizeof(phys_addr_t));
 + h-next_early_heap = NULL;
 +
 + return h;
 +}
 +
 +struct early_heap_header *early_brk(size_t size)
 + __attribute__((weak, alias(def_early_brk)));

 what about using (it needs linux/compiler.h):

 __weak struct early_heap_header *early_brk(size_t size)
 {
 ...
 body
 ...
 }

We already have a lot of the former - I prefer not to add additional
semantics (unless you want to do a wholesale search/replace ;))


 +void *dmmalloc(size_t size)
 +{
 +#ifdef CONFIG_SYS_EARLY_MALLOC
 + if (is_early_malloc_active())
 + return early_malloc(size);
 +#endif /* CONFIG_SYS_EARLY_MALLOC */

 Or you can implement empty prototypes for these functions in case CONFIG_SYS 
 ...
 isn't defined to punt this preprocessor bloat.

Agree


 + return malloc(size);
 +}

 [...]

 diff --git a/include/configs/zipitz2.h b/include/configs/zipitz2.h
 index 26204af..5cd0dcb 100644
 --- a/include/configs/zipitz2.h
 +++ b/include/configs/zipitz2.h
 @@ -176,8 +176,13 @@ unsigned char zipitz2_spi_read(void);

  #define  CONFIG_SYS_LOAD_ADDRCONFIG_SYS_DRAM_BASE

 +#define CONFIG_SYS_EARLY_HEAP_ADDR (GENERATED_GBL_DATA_SIZE + \
 + PHYS_SDRAM_1)
 +#define CONFIG_SYS_EARLY_HEAP_SIZE 256
 +

 1) Pull this file into separate patch and order it afterwards this patch.

Already agreed :)

Regards,

Graeme
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCHv4] [RFC] DM: early_malloc for DM added.

2012-09-18 Thread Marek Vasut 
Dear Graeme Russ,

 Hi Marek,
 
 On Tue, Sep 18, 2012 at 8:57 PM, Marek Vasut ma...@denx.de wrote:
  Dear Tomas Hlavacek,
  
  early_malloc for DM with support for more heaps and lightweight
  first heap on stack.
  
  Adaptation layer for seamless calling of early_malloc or dlmalloc from
  DM based on init stage added (dmmalloc() and related functions).
  
  Signed-off-by: Tomas Hlavacek tmshl...@gmail.com
  ---
  
  It looks mostly OK, few comments
  
  I'd say, pull out the modification of global data into separate patch and
  put it before this patch. That'd make review of the core code much
  easier.
 
 NAK - The addition of the global data member is intrinsic to the early
 malloc implmentaion. Keep them together

Very pleasant to review too, I almost didn't manage to find the core dmmalloc 
code in all that bloat.

  [...]
  
  +
  +#include common.h /* for ROUND_UP */
  +#include asm/u-boot.h
  +#include asm/global_data.h /* for gd_t and gd */
  +#include asm/types.h /* for phys_addr_t and size_addt_t */
  +
  +#include dmmalloc.h
  +#include malloc.h
  +
  +DECLARE_GLOBAL_DATA_PTR;
  +
  +#ifdef CONFIG_SYS_EARLY_MALLOC
  +static struct early_heap_header *def_early_brk(size_t size)
  +{
  + struct early_heap_header *h =
  + (struct early_heap_header *)CONFIG_SYS_EARLY_HEAP_ADDR;
  +
  + h-free_space_pointer = (void *)(roundup(
  + (phys_addr_t)CONFIG_SYS_EARLY_HEAP_ADDR +
  + sizeof(struct early_heap_header),
  + sizeof(phys_addr_t)));
  + h-free_bytes = size - roundup(sizeof(struct early_heap_header),
  + sizeof(phys_addr_t));
  + h-next_early_heap = NULL;
  +
  + return h;
  +}
  +
  +struct early_heap_header *early_brk(size_t size)
  + __attribute__((weak, alias(def_early_brk)));
  
  what about using (it needs linux/compiler.h):
  
  __weak struct early_heap_header *early_brk(size_t size)
  {
  ...
  body
  ...
  }
 
 We already have a lot of the former - I prefer not to add additional
 semantics (unless you want to do a wholesale search/replace ;))

The former looks like shit and the later is more linux-friendly. I'd say stick 
with the later to avoid this insane __attribute__(()) construct.

  +void *dmmalloc(size_t size)
  +{
  +#ifdef CONFIG_SYS_EARLY_MALLOC
  + if (is_early_malloc_active())
  + return early_malloc(size);
  +#endif /* CONFIG_SYS_EARLY_MALLOC */
  
  Or you can implement empty prototypes for these functions in case
  CONFIG_SYS ... isn't defined to punt this preprocessor bloat.
 
 Agree
 
  + return malloc(size);
  +}
  
  [...]
  
  diff --git a/include/configs/zipitz2.h b/include/configs/zipitz2.h
  index 26204af..5cd0dcb 100644
  --- a/include/configs/zipitz2.h
  +++ b/include/configs/zipitz2.h
  @@ -176,8 +176,13 @@ unsigned char zipitz2_spi_read(void);
  
   #define  CONFIG_SYS_LOAD_ADDRCONFIG_SYS_DRAM_BASE
  
  +#define CONFIG_SYS_EARLY_HEAP_ADDR (GENERATED_GBL_DATA_SIZE + \
  + PHYS_SDRAM_1)
  +#define CONFIG_SYS_EARLY_HEAP_SIZE 256
  +
  
  1) Pull this file into separate patch and order it afterwards this patch.
 
 Already agreed :)
 
 Regards,
 
 Graeme

Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCHv4] [RFC] DM: early_malloc for DM added.

2012-09-18 Thread Graeme Russ 
Hi Marek,

On Wed, Sep 19, 2012 at 9:33 AM, Marek Vasut ma...@denx.de wrote:
 Dear Graeme Russ,


[snip]

  +struct early_heap_header *early_brk(size_t size)
  + __attribute__((weak, alias(def_early_brk)));
 
  what about using (it needs linux/compiler.h):
 
  __weak struct early_heap_header *early_brk(size_t size)
  {
  ...
  body
  ...
  }

 We already have a lot of the former - I prefer not to add additional
 semantics (unless you want to do a wholesale search/replace ;))

 The former looks like shit and the later is more linux-friendly. I'd say stick
 with the later to avoid this insane __attribute__(()) construct.

I agree, but, consistently bad is worse than inconsistently bad :)

I'm look forward to reviewing a cleanup patch ;)

Regards,

Graeme
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCHv4] [RFC] DM: early_malloc for DM added.

2012-09-18 Thread Marek Vasut 
Dear Graeme Russ,

 Hi Marek,
 
 On Wed, Sep 19, 2012 at 9:33 AM, Marek Vasut ma...@denx.de wrote:
  Dear Graeme Russ,
 
 [snip]
 
   +struct early_heap_header *early_brk(size_t size)
   + __attribute__((weak, alias(def_early_brk)));
   
   what about using (it needs linux/compiler.h):
   
   __weak struct early_heap_header *early_brk(size_t size)
   {
   ...
   body
   ...
   }
  
  We already have a lot of the former - I prefer not to add additional
  semantics (unless you want to do a wholesale search/replace ;))
  
  The former looks like shit and the later is more linux-friendly. I'd say
  stick with the later to avoid this insane __attribute__(()) construct.
 
 I agree, but, consistently bad is worse than inconsistently bad :)

To weed out the old crap, we need linux/compiler.h enabled across whole uboot 
... it's in the todo.

 I'm look forward to reviewing a cleanup patch ;)

Wait until I get it completely built. linux/compiler.h causes breakage on some 
ancient PPC crap.

 Regards,
 
 Graeme

Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot