On Wed, 16 Jan 2019 at 14:49, Tom Rini <tr...@konsulko.com> wrote:
>
> On Wed, Jan 16, 2019 at 10:44:16PM +0100, Simon Goldschmidt wrote:
> > Am Mi., 16. Jan. 2019, 22:34 hat Simon Glass <s...@chromium.org>
> > geschrieben:
> >
> > > Hi Simon,
> > >
> > > On Mon, 14 Jan 2019 at 14:38, Simon Goldschmidt
> > > <simon.k.r.goldschm...@gmail.com> wrote:
> > > >
> > > > This adds two new functions, lmb_alloc_addr and
> > > > lmb_get_unreserved_size.
> > > >
> > > > lmb_alloc_addr behaves like lmb_alloc, but it tries to allocate a
> > > > pre-specified address range. Unlike lmb_reserve, this address range
> > > > must be inside one of the memory ranges that has been set up with
> > > > lmb_add.
> > > >
> > > > lmb_get_unreserved_size returns the number of bytes that can be
> > > > used up to the next reserved region or the end of valid ram. This
> > > > can be 0 if the address passed is reserved.
> > > >
> > > > Added test for these new functions.
> > > >
> > > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschm...@gmail.com>
> > > > ---
> > > >
> > > > Changes in v10: None
> > > > Changes in v9: None
> > > > Changes in v8: None
> > > > Changes in v7: None
> > > > Changes in v6: None
> > > > Changes in v5:
> > > > - fixed lmb_alloc_addr when resulting reserved ranges get combined
> > > > - added test for these new functions
> > > >
> > > > Changes in v4: None
> > > > Changes in v2:
> > > > - added lmb_get_unreserved_size() for tftp
> > > >
> > > > include/lmb.h | 3 +
> > > > lib/lmb.c | 53 +++++++++++++
> > > > test/lib/lmb.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++++
> > > > 3 files changed, 258 insertions(+)
> > >
> > > Reviewed-by: Simon Glass <s...@chromium.org>
> > >
> > > But please see suggestions/nits below.
> > >
> > > >
> > > > diff --git a/include/lmb.h b/include/lmb.h
> > > > index f04d058093..7d7e2a78dc 100644
> > > > --- a/include/lmb.h
> > > > +++ b/include/lmb.h
> > > > @@ -38,6 +38,9 @@ extern phys_addr_t lmb_alloc_base(struct lmb *lmb,
> > > phys_size_t size, ulong align
> > > > phys_addr_t max_addr);
> > > > extern phys_addr_t __lmb_alloc_base(struct lmb *lmb, phys_size_t size,
> > > ulong align,
> > > > phys_addr_t max_addr);
> > > > +extern phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base,
> > > > + phys_size_t size);
> > >
> > > Can you please add full comments in the header for new functions.
> > >
> >
> > Sure I can but wouldn't it look odd to have one function documented in the
> > header but not the rest?
> >
> >
> > > > +extern phys_size_t lmb_get_unreserved_size(struct lmb *lmb, phys_addr_t
> > > addr);
> > > > extern int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr);
> > > > extern long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t
> > > size);
> > > >
> > > > diff --git a/lib/lmb.c b/lib/lmb.c
> > > > index cd297f8202..e380a0a722 100644
> > > > --- a/lib/lmb.c
> > > > +++ b/lib/lmb.c
> > > > @@ -313,6 +313,59 @@ phys_addr_t __lmb_alloc_base(struct lmb *lmb,
> > > phys_size_t size, ulong align, phy
> > > > return 0;
> > > > }
> > > >
> > > > +/*
> > > > + * Try to allocate a specific address range: must be in defined memory
> > > but not
> > > > + * reserved
> > > > + */
> > > > +phys_addr_t lmb_alloc_addr(struct lmb *lmb, phys_addr_t base,
> > > phys_size_t size)
> > > > +{
> > > > + long j;
> > >
> > > How about addr instead of j? I think single-char vars are OK for loop
> > > counters, etc. but this is not that.
> > >
> >
> > Sure.
> >
> >
> > > > +
> > > > + /* Check if the requested address is in one of the memory
> > > regions */
> > > > + j = lmb_overlaps_region(&lmb->memory, base, size);
> > > > + if (j >= 0) {
> > > > + /*
> > > > + * Check if the requested end address is in the same
> > > memory
> > > > + * region we found.
> > > > + */
> > > > + if (lmb_addrs_overlap(lmb->memory.region[j].base,
> > > > + lmb->memory.region[j].size, base +
> > > size -
> > > > + 1, 1)) {
> > > > + /* ok, reserve the memory */
> > > > + if (lmb_reserve(lmb, base, size) >= 0)
> > > > + return base;
> > > > + }
> > > > + }
> > > > + return 0;
> > > > +}
> > > > +
> > > > +/* Return number of bytes from a given address that are free */
> > > > +phys_size_t lmb_get_unreserved_size(struct lmb *lmb, phys_addr_t addr)
> > >
> > > I support you use 'unreserved' instead of 'free' due to some subtle
> > > difference in meaning? Can you add a comment somewhere about this?
> > >
> >
> > Actually no, the name could be changed to 'lmb_get_free_size' if you like.
> >
> >
> > > > +{
> > > > + int i;
> > > > + long j;
> > >
> > > Here too - addr?
> > >
> >
> > Yes.
> >
> >
> > > > +
> > > > + /* check if the requested address is in the memory regions */
> > > > + j = lmb_overlaps_region(&lmb->memory, addr, 1);
> > > > + if (j >= 0) {
> > > > + for (i = 0; i < lmb->reserved.cnt; i++) {
> > > > + if (addr < lmb->reserved.region[i].base) {
> > > > + /* first reserved range > requested
> > > address */
> > > > + return lmb->reserved.region[i].base -
> > > addr;
> > > > + }
> > > > + if (lmb->reserved.region[i].base +
> > > > + lmb->reserved.region[i].size > addr) {
> > > > + /* requested addr is in this reserved
> > > range */
> > > > + return 0;
> > > > + }
> > > > + }
> > > > + /* if we come here: no reserved ranges above requested
> > > addr */
> > > > + return lmb->memory.region[lmb->memory.cnt - 1].base +
> > > > + lmb->memory.region[lmb->memory.cnt - 1].size -
> > > addr;
> > > > + }
> > > > + return 0;
> > > > +}
> > > > +
> > >
> >
> > Sigh, I'll re-spin again, but I won't find the time to do so before next
> > week...
>
> Do it as a follow-up please, I'm testing v10 atm. Thanks!
Yes, these are all minor points and you have my review tag, thank you.
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
