On Mon, Dec 05, 2016 at 11:46:31AM -0600, Benjamin Fair wrote:
> On 12/01/2016 09:12 PM, David Gibson wrote:
> > On Wed, Nov 30, 2016 at 01:35:04PM -0600, Benjamin Fair wrote:
> > > On 11/25/2016 04:51 AM, David Gibson wrote:
> > > > On Wed, Nov 09, 2016 at 10:58:32AM -0600, Benjamin Fair wrote:
> > > [...]
> > > >
> > > > I like the concept of a helper to read entries from reg, but there
> > > > some things about the execution of it I think need some more thought.
> > > >
> > >
> > > Thanks for the review.
> > >
> > > [...]
> > > > > diff --git a/libfdt/fdt_addresses.c b/libfdt/fdt_addresses.c
> > > > > index eff4dbc..92cbed9 100644
> > > > > --- a/libfdt/fdt_addresses.c
> > > > > +++ b/libfdt/fdt_addresses.c
> > > > > @@ -55,6 +55,9 @@
> > > > >
> > > > > #include "libfdt_internal.h"
> > > > >
> > > > > +#define BYTES_PER_CELL 4
> > > > > +#define BITS_PER_CELL 32
> > > >
> > > > You shouldn't need these. BYTES_PER_CELL == sizeof(fdt32_t).
> > > >
> > >
> > > Of course. Thanks. I'll get rid of them.
> > >
> > > > > +
> > > > > int fdt_address_cells(const void *fdt, int nodeoffset)
> > > > > {
> > > > > const fdt32_t *ac;
> > > > > @@ -94,3 +97,62 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
> > > > >
> > > > > return val;
> > > > > }
> > > > > +
> > > > > +static uint64_t _fdt_read_cells(const fdt32_t *cells, int n)
> > > >
> > > > This is a reasonable helper, but the name is bad. "read_cells"
> > > > suggests it can read some arbitrary number of cells, but in fact all
> > > > it can do is read a 32-bit int or a 64-bit int. Plus everything is
> > > > made up of cells, but more specifically what you're doing here is
> > > > interpreting several cells as an integer in the usual encoding.
> > > >
> > >
> > > Would "cells_to_integer" be a better name? Or would you recommend something
> > > else for this?
> >
> > cells_to_integer would be ok. Or just fdt_read_integer().
> >
>
> Thanks for the suggestion. I changed it to fdt_read_integer()
>
> > >
> > > > > +{
> > > > > + int i;
> > > > > + uint64_t res;
> > > > > +
> > > > > + /* TODO: Support values larger than 2 cells */
> > > >
> > > > I don't really see any way you could support >2 cells without
> > > > completely changing the interface.
> > > >
> > >
> > > True. I wanted to have the result be a 128 bit integer, but couldn't find a
> > > portable way to do so. Is there a better way to go about this? Or is it fine
> > > to only support at most 2 cells, even though the rest of libfdt supports 4?
> >
> > I think just supporting 2 cells is ok - it's useful in enough
> > practical cases. However, I'd suggest thinking of this in a more
> > layered way.
> >
> > First, create a helper which just locates the relevant pieces of reg
> > entries, returning addresses and sizes as (fdt32_t *). That's at
> > least somewhat useful for the >2 cells case, even though you have to
> > then parse the address/size yourself. In the most common case for
> > this, PCI (address-cells == 3), you probably want to do that anyway.
> > This may also be useful for cases which are <= 2 cells, but the
> > encoding of the address is not just a plain integer.
> >
> > You can then polish up fdt_read_integer() a bit and export it.
> >
> > Finally you can add another helper which combines these to directly
> > get you the addr+size as u64 for buses where that's suitable. Make
> > sure to return an error if #a or #s > 2, of course.
> >
>
> That seems like a good idea. I implemented this for the next revision.
>
> > > > > + if (n > 2)
> > > > > + return -FDT_ERR_BADNCELLS;
> > > > > +
> > > > > + res = 0;
> > > > > + for (i = 0; i < n; i++) {
> > > > > + res <<= BITS_PER_CELL;
> > > > > + res |= fdt32_to_cpu(cells[i]);
> > > > > + }
> > > > > +
> > > > > + return res;
> > > > > +}
> > > > > +
> > > > > +int fdt_simple_addr_size(const void *fdt, int nodeoffset, int idx,
> > > > > + uintptr_t *addr, size_t *size)
> > > > > +{
> > > > > + int parent;
> > > > > + int ac, sc, reg_stride;
> > > > > + int res;
> > > > > + const fdt32_t *reg;
> > > > > +
> > > > > + reg = fdt_getprop(fdt, nodeoffset, "reg", &res);
> > > > > + if (res < 0)
> > > > > + return res;
> > > > > +
> > > > > + parent = fdt_parent_offset(fdt, nodeoffset);
> > > > > + if (parent < 0)
> > > > > + return res;
> > > >
> > > > So, fdt_parent_offset() is very expensive, I wouldn't recommend it in
> > > > a function that's likely to be called a lot like this. Instead I'd
> > > > suggest a function which takes the parent offset as a parameter, and
> > > > optionally a wrapper that uses fdt_parent_offset().
> > >
> > > Great idea, I'll do this in the next revision once we have a solution for
> > > the rest of the comments.
> > >
>
> Would it be OK to pass in the number of address cells and number of size
> cells directly instead of the parent offset?
Yes, that seems reasonable. Though you might want one function which
does that, then another which will do it for you based on parent
offset.
> I was thinking about this in order to minimize repeated work. Plus, the
> calling function needs to know this information in order to interpret the
> results anyway.
True.
> > > >
> > > > > +
> > > > > + ac = fdt_address_cells(fdt, parent);
> > > > > + if (ac < 0)
> > > > > + return ac;
> > > > > +
> > > > > + sc = fdt_size_cells(fdt, parent);
> > > > > + if (sc < 0)
> > > > > + return sc;
> > > > > +
> > > > > + reg_stride = ac + sc;
> > > > > + /*
> > > > > + * res is the number of bytes read and must be an even multiple of the
> > > > > + * sum of ac and sc.
> > > > > + */
> > > > > + if ((res % (reg_stride * BYTES_PER_CELL)) != 0)
> > > > > + return -FDT_ERR_BADVALUE;
> > > > > +
> > > > > + if (addr)
> > > > > + *addr = (uintptr_t) _fdt_read_cells(®[reg_stride * idx], ac);
> > > >
> > > > I don't think uintptr_t makes sense here. The addresses in the device
> > > > tree are in whatever bus they're in, and there are a whole stack of
> > > > reasons that could be unrelated to the pointer size of environment
> > > > libfdt is running in:
> > > > - The device may be on a subordinate bus whose addresses need
> > > > to be translated
> > > > - Even at the top-level, the reg properties represent
> > > > *physical* addresses, which may not be the same as virtual
> > > > addresses in code running on the system
> > > > - libfdt may be running on a completely different system from the
> > > > one the device tree in question is aimed at (bootloaders are
> > > > only one use case for libfdt).
> > > >
> > > > > + if (size)
> > > > > + *size = (size_t) _fdt_read_cells(®[ac + reg_stride * idx],
> > > > > + sc);
> > > >
> > > > Likewise size_t isn't necessarily right here, although I suspect it's
> > > > less likely to break in practice.
> > > >
> > >
> > > Hmm... Is it fine to use uint64_t for both of these instead then?
> >
> > Yes, I think that's reasonable. Or you could even use unsigned long
> > or unsigned long long - as long as you error whenever #cells is too
> > big for the size of that type. uint64_t is probably clearer and more
> > consistent, though.
> >
>
> Thanks, I changed it to use uint64_t
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
