On Tue, Jan 08, 2019 at 04:16:54PM +0900, AKASHI, Takahiro wrote:
> 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.)
Hrm, I'm not sure that re-using #address-cells and #size-cells for
something other than 'reg' properties and things necessarily in the
same address space is a good idea.
> 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.
Yes, it is. In fact in C it's very close to impossible.
> > 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()?
Hm. Still don't like it very much. fdt_setprop_addrrange()?
> > 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.
It's pretty clearly useful for 'reg' properties - you can build them
up one entry at a time.
> None of fdt_setprop_xxx() variants do a similar thing.
No, if we did this it would need to be renamed fdt_appendprop_xxx() as
well.
> > 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?
I prefer the address and size as separate, explicit parameters.
>
> 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
Attachment:
signature.asc
Description: PGP signature
