From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Wed, 19 Sep 2012 01:33:09 +0200
Subject: [U-Boot] [PATCHv4] [RFC] DM: early_malloc for DM added.
In-Reply-To: <CALButCJ0WeaWp=jbUZY9bArU0bngH0zyQNu+BJ+0kOu=szdNUg@mail.gmail.com>
References: <1346069529-27397-1-git-send-email-tmshlvck@gmail.com>
	<201209181257.53329.marex@denx.de>
	<CALButCJ0WeaWp=jbUZY9bArU0bngH0zyQNu+BJ+0kOu=szdNUg@mail.gmail.com>
Message-ID: <201209190133.09945.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Graeme Russ,

> Hi Marek,
> 
> On Tue, Sep 18, 2012 at 8:57 PM, Marek Vasut <marex@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 <tmshlvck@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