[U-Boot] [PATCH] board_f: reserve noncached space below malloc area

[Vikas MANOCHA]

Hi Simon,

> -----Original Message-----
> From: Simon Glass <sjg at chromium.org>
> Sent: Friday, August 9, 2019 10:00 AM
> To: Vikas MANOCHA <vikas.manocha at st.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Bin Meng
> <bmeng.cn at gmail.com>; CITOOLS <smet-aci-
> reviews at lists.codex.cro.st.com>; Mario Six <mario.six at gdsys.cc>; Patrick
> DELAUNAY <patrick.delaunay at st.com>
> Subject: Re: [PATCH] board_f: reserve noncached space below malloc area
> 
> Hi Vikas,
> 
> On Thu, 8 Aug 2019 at 18:24, Vikas Manocha <vikas.manocha at st.com>
> wrote:
> >
> > Noncached area at present is being initialized to random space after
> > malloc area. It works in most the cases as it goes to stack area &
> > stack is not overwriting it being far from it.
> 
> Please can you add a motivation for this patch and what it does.

We are not reserving non-cached memory area (like for malloc, gd etc before relocation). 
With this patch we are reserving area just below memory area allocated to dynamic allocation.

> 
> >
> > Signed-off-by: Vikas Manocha <vikas.manocha at st.com>
> > ---
> >  common/board_f.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/common/board_f.c b/common/board_f.c index
> > 59745d5..4910051 100644
> > --- a/common/board_f.c
> > +++ b/common/board_f.c
> > @@ -439,12 +439,25 @@ static int reserve_uboot(void)
> >         return 0;
> >  }
> >
> > +#ifdef CONFIG_SYS_NONCACHED_MEMORY
> > +static int reserve_noncached(void)
> > +{
> > +       /* round down to SECTION SIZE (typicaly 1MB) limit */
> > +       gd->start_addr_sp &= ~(MMU_SECTION_SIZE - 1);
> > +       gd->start_addr_sp -= CONFIG_SYS_NONCACHED_MEMORY;
> 
> blank line before return.

Sure, I will add it in v2.

> > +       return 0;
> > +}
> > +#endif
> > +
> >  /* reserve memory for malloc() area */  static int
> > reserve_malloc(void)  {
> >         gd->start_addr_sp = gd->start_addr_sp - TOTAL_MALLOC_LEN;
> >         debug("Reserving %dk for malloc() at: %08lx\n",
> >               TOTAL_MALLOC_LEN >> 10, gd->start_addr_sp);
> > +#ifdef CONFIG_SYS_NONCACHED_MEMORY
> > +       reserve_noncached(void);
> > +#endif
> 
> How about a new function for this (with the #ifndef inside it), and a debug()?

That is what I did first but this (non cached) memory area being tied to just below malloc, having it here enforces this contiguity.

> 
> Also I see noncached_init() calculates the size which seems brittle.
> Should it be added to gd?

Good point, creating new entry/ies in gd for it would make it bit cleaner but useless after initialization.
Static variables are used for this non cached memory init & allocation. It is just like for malloc, we don’t have entry in gd for it & I
think the reason is same.

Cheers,
Vikas

> 
> >         return 0;
> >  }
> >
> > --
> > 2.7.4
> >
> 
> Regards,
> Simon