David, Rob,
On Fri, Jan 11, 2019 at 03:17:35PM +1100, David Gibson wrote:
> 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.
Yes.
> >
> > > > 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.
Okay.
I will prepare a new version of patch in this way
unless Rob opposes it.
Rob?
-Takahiro Akashi
>
> >
> > 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
