On Tue, Jun 03, 2025 at 12:14:44PM +0530, Sughosh Ganu wrote:
> On Fri, 30 May 2025 at 15:41, Sughosh Ganu <sughosh.g...@linaro.org> wrote:
> >
> > On Fri, 30 May 2025 at 13:17, Ilias Apalodimas
> > <ilias.apalodi...@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;
> > >
> > >
> > > 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. -- Tom
signature.asc
Description: PGP signature
