On Fri, Jan 11, 2019 at 11:08:46AM +0900, AKASHI, Takahiro wrote:
> David,
>
> On Fri, Jan 11, 2019 at 11:44:53AM +1100, David Gibson wrote:
> > 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.
>
> We have discussed this issue in the past on arm-kernel ML, and
> in kexec_load (not kexec_file_load syscall) case, we have already
> supported this semantics and there's no chance to change it.
Ok. I'm assuming these values are in terms of physical addresses as
used by the cpu? Which should be the same as the dt root bus address
space, so I guess it's ok.
>
> > > 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.
>
> After all, do you agree with the following interface?
>
> int fdt_appendprop_addrrange(void *fdt, int parent_offset, int node_offset,
> const char *name, int num_rng, uint64_t addr[], uint64_t size[]);
>
> Here, if "name" doesn't exist under node_offset, it will be created.
> Otherwise, the values are appended. If num_rng == 0, "name" will be
> deleted.
Part of the point of using the append interface is that you don't need
to support multiple entries, which requires the ugly use of arrays
here. You can drop num_rng, have just one addr & size parameter and
call the function multiple times to build up a multi-entry value.
>
> Thanks,
> -Takahiro Akashi
>
> > >
> > > 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
