[PATCH v2 14/32] lmb: introduce a function to add memory to the lmb memory map

[Simon Glass]

Hi Sughosh,

On Wed, 21 Aug 2024 at 08:40, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> On Wed, 21 Aug 2024 at 19:30, Simon Glass <sjg at chromium.org> wrote:
> >
> > +Bin Meng
> >
> > Hi Sughosh,
> >
> > On Tue, 20 Aug 2024 at 23:29, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > >
> > > On Wed, 21 Aug 2024 at 07:41, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > On Tue, 20 Aug 2024 at 02:17, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > >
> > > > > On Fri, 16 Aug 2024 at 02:04, Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Wed, 14 Aug 2024 at 12:01, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > >
> > > > > > > Introduce a function lmb_add_memory() to add available memory to the
> > > > > > > LMB memory map. Call this function during board init once the LMB data
> > > > > > > structures have been initialised.
> > > > > > >
> > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > > > ---
> > > > > > > Changes since V1:
> > > > > > > * Call the lmb_add_memory() from lmb_init() instead of
> > > > > > >   lmb_mem_regions_init().
> > > > > > >
> > > > > > >
> > > > > > >  include/lmb.h | 10 ++++++++++
> > > > > > >  lib/lmb.c     | 42 ++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  2 files changed, 52 insertions(+)
> > > > > > >
> > > > > >
> > > > > > Reviewed-by: Simon Glass <sjg at chromium.org>
> > > > > >
> > > > > > But this should not be weak.
> > > > >
> > > > > This is being made weak, as there would be lmb_add_memory()
> > > > > definitions added for powerpc and x86 arch's in the EFI part of my
> > > > > patches. Moreover, the lmb_add_memory() function would be called even
> > > > > in the SPL stage when LMB is enabled for that stage. So I am not sure
> > > > > how do we get around this. You can check the relevant branch [1] on my
> > > > > github to check for the specific commits [2][3] that I am referring
> > > > > to. Thanks.
> > > >
> > > > This is really strange.
> > > >
> > > > The e820 is different on each x86 board. I'm not sure we want that in
> > > > the lmb. What is the purpose of that? It is somewhat circular in most
> > > > cases, since U-Boot sets it up itself. Where it comes from coreboot,
> > > > it looks like we are mirroring it in the EFI memory map. I'm not sure
> > > > I understand all this very well.
> > >
> > > Yes, me neither. And I want to keep the behaviour same as before the
> > > patches. You would know that the function efi_add_known_memory() gets
> > > the memory map from a function install_e820_map() which includes
> > > conventional memory, which is the ram memory. And I am basically now
> > > doing this as part of the lmb_add_memory() function instead. Are you
> > > sure that we can do away with this function, and instead use the
> > > ram_base and ram_top values from the global data structure instead? I
> > > believe you have boards which exercise this code? So it will be great
> > > if you can test this if I remove the function for the e820 module.
> >
> > We are suffering here from too many ways to do the same thing and too
> > much confusion about the overall goal here.
> >
> > Typically the e820 thing is created in U-Boot - see
> > install_e820_map(). The ISA and PCI ranges needs to be added, so EFI
> > needs a way to know to do that.
> >
> > e820 is used for the normal kernel boot flow - the table is passed
> > directly to Linux.
> >
> > With EFI the kernel calls back into U-Boot to get the memory map, so
> > we want to add the same reservations to the EFI tables.
> >
> > So I would favour having EFI sending an event before booting, to allow
> > other code to add new reservations. Then we can let e820 do its thing
> > and not affect EFI. In fact the e820 code will never be called on an
> > EFI boot.
> >
> > I say we can clean this up later, so what you have here will do for now.
> >
> > >
> > > >
> > > > For fsl, perhaps copy the #ifdef and handle arch.resv_ram in your code?
> > >
> > > This is for adding ram to the lmb memory map, but yes, I can check by
> > > putting an ifdef in the function. Although the function might look
> > > ugly and hackish. Thanks.
> >
> > Again, we can clean this up date. It is just one arch doing strange things.
>
> Can you please take a look at the branch[1] which I pointed to in my
> earlier reply. I believe this caters to your review comment on doing
> away with the weak function, as well as providing the option to boards
> to define their own memory map. Thanks.

Yes your latest patch looks fine thank you.

Regards,
Simon



>
> -sughosh
>
> [1] - https://github.com/sughoshg/u-boot/tree/lmb_efi_sep_apis_nrfc_next_noweak_v3
>
> >
> > > >
> > > > [..]
> > > >
> > > > > [1] - https://github.com/sughoshg/u-boot/tree/lmb_efi_sep_apis_nrfc_next_v3
> > > > > [2] - https://github.com/u-boot/u-boot/commit/077ced7aaa6d495b1b87b324fb1c60658c203ce1
> > > > > [3] - https://github.com/u-boot/u-boot/commit/d0fa3a89865b796f3bbebffebbe4f7b5b048c140
> > > > >
> >
> > Regards,
> > Simon