[PATCH v3 2/6] lmb: replace the lmb_alloc() and lmb_alloc_base() API's

[Sughosh Ganu]

On Tue, 3 Jun 2025 at 23:52, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> Am 3. Juni 2025 18:22:46 MESZ schrieb Tom Rini <trini at konsulko.com>:
> >On Tue, Jun 03, 2025 at 12:14:44PM +0530, Sughosh Ganu wrote:
> >> On Fri, 30 May 2025 at 15:41, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> >> >
> >> > On Fri, 30 May 2025 at 13:17, Ilias Apalodimas
> >> > <ilias.apalodimas at linaro.org> wrote:
> >> > >
> >> > > > +static phys_addr_t lmb_alloc(phys_size_t size)
> >> > > > +{
> >> > > > +       int ret;
> >> > > > +       phys_addr_t addr;
> >> > > > +
> >> > > > +       /* All memory regions allocated with a 2MiB alignment */
> >> > > > +       ret = lmb_alloc_mem(LMB_MEM_ALLOC_ANY, SZ_2M, &addr, size, LMB_NONE);
> >> > > > +       if (ret)
> >> > > > +               return 0;
> >> > > > +
> >> > > > +       return addr;
> >> > > > +}
> >> > > > +
> >> > >
> >> > > I think we  need a better naming for these. Right now we have
> >> > > lmb_alloc() here and in tests, addr_alloc() in snapdragon code.
> >> > > I'd say either export them as API if you think they would be useful,
> >> > > or get rid of the wrappers.
> >> >
> >> > I kept these functions as is to reduce the amount of change resulting
> >> > from introducing the API. Also, the newly introduced API has
> >> > parameters which are common across all the callers, which also was why
> >> > I kept a wrapper. This is especially true in the tests, where it would
> >> > be required to change the function names in a bunch of places without
> >> > much benefit.
> >> >
> >> > >
> >> > > [...]
> >> > >
> >> > > > +static phys_addr_t lmb_alloc(phys_size_t size, ulong align)
> >> > > > +{
> >> > > > +       int err;
> >> > > > +       phys_addr_t addr;
> >> > > > +
> >> > > > +       err = lmb_alloc_mem(LMB_MEM_ALLOC_ANY, align, &addr, size, LMB_NONE);
> >> > > > +       if (err)
> >> > > > +               return 0;
>
> Would lmb ever allocate at (phys_addr_t)-1 ?

I think this would depend on the memory map of the platform, and the
type of allocation requested -- whether it is requesting an address,
or simply asking for a region of memory.

>
> I guess this should work for all systems as error indicator.

The API would be returning an int variable, so it would return
negative values. And the actual address allocated would be returned as
part of the 'addr' parameter -- this is similar to the interface
provided by efi_allocate_pages().

-sughosh

>
> Best regards
>
> Heinrich
>
> >> > >
> >> > >
> >> > > This tends to blow up in random ways. See
> >> > > commit 67be24906fe. TL;DR 0 is a valid address in some systems.
> >> >
> >> > Yes, I see your point. I think the calling function will have to be
> >> > re-written such that the env variables get stored only when the API
> >> > returns successfully. Then at least the platform will not have an env
> >> > variable with some junk value.
> >>
> >> Thinking about this a bit, I think in these two instances, returning a
> >> value of 0 might not be an issue if the DRAM memory does not start at
> >> 0x0. Don't get me wrong, what you are suggesting is definitely
> >> correct. I am only thinking about increasing code size on these
> >> platforms if 0x0 is not a valid address, and moreover since the
> >> platforms were already setting 0x0 in case of an error. If it is okay
> >> to increase code size on these platforms, I will change the calling
> >> function, such that the variable does not get set in case of an error.
> >> Maybe Casey and Mark can comment? Thanks.
> >
> >Systems where 0x0 is not valid memory are common enough I think, yes.
> >You can see what the size growth is by checking say j721e_evm_a72
> >before/after, and if you haven't been using buildman for size
> >comparisons already, I posted
> >https://source.denx.de/u-boot/u-boot-extras/-/blob/master/contrib/trini/u-boot-size-test.sh?ref_type=heads
> >a while back now. Thanks.
> >
>