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

[Heinrich Schuchardt]

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 guess this should work for all systems as error indicator.

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.
>