David, Rob,
On Wed, Jan 02, 2019 at 02:49:44PM +1100, David Gibson wrote:
> On Tue, Dec 18, 2018 at 09:49:02AM +0900, AKASHI, Takahiro wrote:
> > Rob,
> >
> > On Mon, Dec 17, 2018 at 11:56:28AM -0600, Rob Herring wrote:
> > > On Thu, Dec 13, 2018 at 12:37 AM AKASHI Takahiro
> > > <takahiro.akashi@xxxxxxxxxx> wrote:
> > > >
> > > > This function will add a memory region property based on parent node's
> > > > "#address-cells" and "#size-cells" properties.
> > > >
> > > > It will be used in implementing kdump with kexec_file_load system call
> > > > at linux kernel for arm64 once it is merged into kernel tree.
> > > >
> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@xxxxxxxxxx>
> > > > ---
> > > > libfdt/fdt_addresses.c | 49 ++++++++++++++++++++++++++++++++++++++++++
> > > > libfdt/libfdt.h | 30 ++++++++++++++++++++++++++
> > > > 2 files changed, 79 insertions(+)
> > > >
> > > > diff --git a/libfdt/fdt_addresses.c b/libfdt/fdt_addresses.c
> > > > index f13a87dfa068..08615c9f669a 100644
> > > > --- a/libfdt/fdt_addresses.c
> > > > +++ b/libfdt/fdt_addresses.c
> > > > @@ -95,3 +95,52 @@ int fdt_size_cells(const void *fdt, int nodeoffset)
> > > > return 1;
> > > > return val;
> > > > }
> > > > +
> > > > +static void cpu64_to_fdt_cells(void *prop, uint64_t val, int cells)
> > > > +{
> > > > + fdt32_t val32;
> > > > +
> > > > + while (cells) {
> > > > + val32 = cpu_to_fdt32(val >> (32 * (--cells)));
> > > > + memcpy(prop, &val32, sizeof(val32));
> > > > + prop = (uint8_t *)prop + sizeof(val32);
> > > > + }
> > > > +}
> > > > +
> > > > +/* This function assumes that [address|size]_cells is 1 or 2 */
> > > > +int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> > >
> > > Do you have any use for name besides 'reg'? There are some I think
> > > (assigned-addresses?),
> >
> > Yes. For kdump, we will use "linux,usable-memory-range" and
> > "linux,elfcorehdr." So we need a "name" argument.
>
> Are these really depending on the parent nodes #address-cells and
> #size-cells though? That seems very dubious.
When I first introduced this function in linux kernel, I intended
to use #[address|size]_cells under the "root" as my main usage
of this function is for "chosen" node.
(Top means a parent in this case.)
To generalize it, however, to merge it into dtc/libfdt, I changed
the semantics as described above.
>
> I'm not a great fan of this patch so far.
>
> At the very least, I don't like the current function name: it says
> "reg", but actually can be used for other things,
That is what I intended for generalization.
> at the same time it
> can only handle limited cases of 'reg' properties (single entry, <= 2
> address and size cells).
It is quite hard to define a generic function interface for
variable-length numbers.
> Maybe fdt_setprop_addrsize() ?
I don't care a function name very much, but it sounds to me that
the above name suggests it will change #[address|size]_cells values.
My compromise would be fdt_setprop_reg64()?
> Second thing is to consider whether making it append rather than
> rewrite the property would be a good idea - makes it more flexible for
> things with multiple entries.
I'm not confident that such a semantics is very useful although
it's doable. None of fdt_setprop_xxx() variants do a similar thing.
> Third thing is that this function is non obviously expensive - getting
> the parent node to find the address and size cells requires scanning
> the whole of the device tree from the beginning to this node. It
> might be better to make this take both a "bus" node (the parent) and a
> "device" node (the child) - in many cases I'd guess users of this
> would in fact already know the offset of the bus node.
Makes sense. What do you think, Rob?
> > > but I can't think of any widely used. So
> > > perhaps drop 'name'.
> > >
> > > > + uint64_t addr, uint64_t size)
> > >
> > > Once someone wants more than 1 entry, this function doesn't work.
> > > IIRC, u-boot would need that to use this. It would be better to take
> > > an array of values here. Probably up to some fixed max of 4 or 8
> > > entries would be fine.
> >
> > So, something like this?
> > int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> > int num_reg, uint64_t (*reg)[2]);
> >
> > Alternatively,
> > int fdt_setprop_reg(void *fdt, int nodeoffset, const char *name,
> > int num_reg, fdt_reserve_entry *reg);
> > In this case, converting values to fdt64_t is a user's job.
Any idea on this?
Thanks,
-Takahiro Akashi
> >
> > > And maybe do a wrapper for the single entry case.
> >
> > Sure.
> >
> > -Takahiro Akashi
> >
> > > > +{
> > > > + int parent, addr_cells, size_cells, buf_size;
> > > > + char buf[sizeof(fdt32_t) * 2 * 2];
> > > > + void *prop;
> >
>
> --
> 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
