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_ADDR CONFIG_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
