From mboxrd@z Thu Jan  1 00:00:00 1970
From: Graeme Russ <graeme.russ@gmail.com>
Date: Wed, 19 Sep 2012 09:29:04 +1000
Subject: [U-Boot] [PATCHv4] [RFC] DM: early_malloc for DM added.
In-Reply-To: <201209181257.53329.marex@denx.de>
References: <1346069529-27397-1-git-send-email-tmshlvck@gmail.com>
	<1347952436-5722-1-git-send-email-tmshlvck@gmail.com>
	<201209181257.53329.marex@denx.de>
Message-ID: <CALButCJ0WeaWp=jbUZY9bArU0bngH0zyQNu+BJ+0kOu=szdNUg@mail.gmail.com>
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

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

>
> [...]
>
>> +
>> +#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_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