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.
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, at the same time it
can only handle limited cases of 'reg' properties (single entry, <= 2
address and size cells).
Maybe fdt_setprop_addrsize() ?
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.
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.
> > 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.
>
> > 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
