Hi Simon,
On Wed, 9 Oct 2024 at 04:50, Simon Glass <s...@chromium.org> wrote:
>
> The call to malloc() is a bit strange. The naming of the arguments
> suggests that an address is passed, but in fact it is a pointer, at
> least in the board_init_r() function and SPL equivalent.
>
> Update it to work as described. Add a function comment as well.
>
> Note that this does adjustment does not extend into the malloc()
> implementation itself, apart from changing mem_malloc_init(), since
> there are lots of casts and pointers and integers are used
> interchangeably.
>
> Signed-off-by: Simon Glass <s...@chromium.org>
> ---
>
> common/board_r.c | 3 +--
> common/dlmalloc.c | 8 +++++---
> common/spl/spl.c | 4 +---
> include/malloc.h | 8 ++++++++
> 4 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/common/board_r.c b/common/board_r.c
> index 4faaa202421..fbb10fea494 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -203,8 +203,7 @@ static int initr_malloc(void)
> */
> start = gd->relocaddr - TOTAL_MALLOC_LEN;
> gd_set_malloc_start(start);
> - mem_malloc_init((ulong)map_sysmem(start, TOTAL_MALLOC_LEN),
> - TOTAL_MALLOC_LEN);
> + mem_malloc_init(start, TOTAL_MALLOC_LEN);
> return 0;
> }
>
> diff --git a/common/dlmalloc.c b/common/dlmalloc.c
> index 1ac7ce3f43c..cc4d3a0a028 100644
> --- a/common/dlmalloc.c
> +++ b/common/dlmalloc.c
> @@ -16,6 +16,8 @@
> #include <asm/global_data.h>
>
> #include <malloc.h>
> +#include <mapmem.h>
> +#include <string.h>
> #include <asm/io.h>
> #include <valgrind/memcheck.h>
>
> @@ -598,9 +600,9 @@ void *sbrk(ptrdiff_t increment)
>
> void mem_malloc_init(ulong start, ulong size)
> {
> - mem_malloc_start = start;
> - mem_malloc_end = start + size;
> - mem_malloc_brk = start;
> + mem_malloc_start = (ulong)map_sysmem(start, size);
> + mem_malloc_end = mem_malloc_start + size;
> + mem_malloc_brk = mem_malloc_start;
>
> #ifdef CONFIG_SYS_MALLOC_DEFAULT_TO_INIT
> malloc_init();
> diff --git a/common/spl/spl.c b/common/spl/spl.c
> index c13b2b8f714..fc8b38862e8 100644
> --- a/common/spl/spl.c
> +++ b/common/spl/spl.c
> @@ -679,9 +679,7 @@ void board_init_r(gd_t *dummy1, ulong dummy2)
> spl_set_bd();
>
> if (IS_ENABLED(CONFIG_SPL_SYS_MALLOC)) {
> - mem_malloc_init((ulong)map_sysmem(SPL_SYS_MALLOC_START,
> - SPL_SYS_MALLOC_SIZE),
> - SPL_SYS_MALLOC_SIZE);
> + mem_malloc_init(SPL_SYS_MALLOC_START, SPL_SYS_MALLOC_SIZE);
> gd->flags |= GD_FLG_FULL_MALLOC_INIT;
> }
> if (!(gd->flags & GD_FLG_SPL_INIT)) {
> diff --git a/include/malloc.h b/include/malloc.h
> index 07d3e90a855..9e0be482416 100644
> --- a/include/malloc.h
> +++ b/include/malloc.h
> @@ -981,6 +981,14 @@ extern ulong mem_malloc_start;
> extern ulong mem_malloc_end;
> extern ulong mem_malloc_brk;
>
> +/**
> + * mem_malloc_init() - Set up the malloc() pool
> + *
> + * Sets the region of memory to be used for all future calls to malloc(),
> etc.
> + *
> + * @start: Start address
> + * @size: Size in bytes
> + */
> void mem_malloc_init(ulong start, ulong size);
>
> #ifdef __cplusplus
> --
> 2.43.0
>
This looks ok as long as map_sysmem() is a 1:1 mapping for non-sandbox
archs. NXP seems to be calling these with an address already.
I guess since we only define an alternative map_sysmem() for sandbox this is ok
Reviewed-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>
