From: Andres Salomon <dilinger@queued.net>
To: Grant Likely <grant.likely@secretlab.ca>
Cc: devicetree-discuss@lists.ozlabs.org,
Daniel Drake <dsd@laptop.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] of/flattree: use of_attach_node to build tree, and associated cleanups
Date: Sat, 12 Mar 2011 11:37:26 -0800 [thread overview]
Message-ID: <20110312113726.0bf060da@queued.net> (raw)
In-Reply-To: <20110312091056.GG9347@angua.secretlab.ca>
On Sat, 12 Mar 2011 02:10:56 -0700
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Mar 09, 2011 at 04:16:07PM -0800, Andres Salomon wrote:
> > Use a common function (of_attach_node) to build the device tree.
> > This simplifies the flat device tree creation a bit, and as an
> > added bonus allows us to drop a (now unused) field from the
> > device_node struct.
> >
> > There are a few minor cleanups snuck in here as well (comment
> > additions, etc).
> >
> > Signed-off-by: Andres Salomon <dilinger@queued.net>
>
> In addition to my comment about changing the tree order on the last
> patch, unfortunately this patch will also break the newly added
> of_fdt_unflatten_tree(). of_fdt_unflatten_tree() allows a driver to
> unflatten a private dtb for its own use without it being attached to
> the global tree or the global list of all nodes. I had also forgotten
> about this. Shoot.
Ah, I was wondering what that was all about. So we'd probably end up
with something like:
void of_attach_node(struct device_node *dp)
{
unsigned long flags;
write_lock_irqsave(devtree_lock, &flags);
__of_attach_node(allnodes, dp);
write_unlock_irqrestore(devtree_lock, &flags);
}
Most stuff could get away with just calling of_attach_node, with the
unflatten_dt_node calling __of_attach_node (and either not caring
about devtree_lock, as is currently the case, or grabbing it from
unflatten_device_tree).
>
> The solution would be a variant of of_attach_node which accepts a
> private allnodes pointer. That would also help with the ordering
> issues because the order of the list could be explicitly reversed
> before assigning the value to the real allnodes pointer. Another
> possible option would be an optional 'tail' pointer argument to
> of_attach_node() which if present it would use as the insertion point
> for adding the node, which would preserve the current sort order.
I was leaning towards a tail pointer, but I need to take a closer look
at the two options.
>
> g.
>
> > ---
> > drivers/of/fdt.c | 42
> > ++++++++++++++++-------------------------- include/linux/of.h |
> > 1 - 2 files changed, 16 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index af824e7..e70cee8 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -139,16 +139,17 @@ static void *unflatten_dt_alloc(unsigned long
> > *mem, unsigned long size, /**
> > * unflatten_dt_node - Alloc and populate a device_node from the
> > flat tree
> > * @blob: The parent device tree blob
> > + * @mem: memory chunk to use for allocating device nodes and
> > properties
> > * @p: pointer to node in flat tree
> > * @dad: Parent struct device_node
> > - * @allnextpp: pointer to ->allnext from last allocated device_node
> > + * @size_scan: are we figuring out amount of memory to allocate?
> > * @fpsize: Size of the node path up at the current depth.
> > */
> > unsigned long unflatten_dt_node(struct boot_param_header *blob,
> > unsigned long mem,
> > unsigned long *p,
> > struct device_node *dad,
> > - struct device_node ***allnextpp,
> > + bool size_scan,
> > unsigned long fpsize)
> > {
> > struct device_node *np;
> > @@ -195,7 +196,7 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob,
> > np = unflatten_dt_alloc(&mem, sizeof(struct device_node) +
> > allocl, __alignof__(struct device_node));
> > - if (allnextpp) {
> > + if (!size_scan) {
> > memset(np, 0, sizeof(*np));
> > np->full_name = ((char *)np) + sizeof(struct
> > device_node); if (new_format) {
> > @@ -217,19 +218,10 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, } else
> > memcpy(np->full_name, pathp, l);
> > prev_pp = &np->properties;
> > - **allnextpp = np;
> > - *allnextpp = &np->allnext;
> > - if (dad != NULL) {
> > - np->parent = dad;
> > - /* we temporarily use the next field as
> > `last_child'*/
> > - if (dad->next == NULL)
> > - dad->child = np;
> > - else
> > - dad->next->sibling = np;
> > - dad->next = np;
> > - }
> > + np->parent = dad;
> > kref_init(&np->kref);
> > }
> > + /* process properties */
> > while (1) {
> > u32 sz, noff;
> > char *pname;
> > @@ -258,7 +250,7 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, l = strlen(pname) + 1;
> > pp = unflatten_dt_alloc(&mem, sizeof(struct
> > property), __alignof__(struct property));
> > - if (allnextpp) {
> > + if (!size_scan) {
> > /* We accept flattened tree phandles
> > either in
> > * ePAPR-style "phandle" properties, or the
> > * legacy "linux,phandle" properties. If
> > both @@ -301,7 +293,7 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, sz = (pa - ps) + 1;
> > pp = unflatten_dt_alloc(&mem, sizeof(struct
> > property) + sz, __alignof__(struct property));
> > - if (allnextpp) {
> > + if (!size_scan) {
> > pp->name = "name";
> > pp->length = sz;
> > pp->value = pp + 1;
> > @@ -313,7 +305,7 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, (char *)pp->value);
> > }
> > }
> > - if (allnextpp) {
> > + if (!size_scan) {
> > *prev_pp = NULL;
> > np->name = of_get_property(np, "name", NULL);
> > np->type = of_get_property(np, "device_type",
> > NULL); @@ -322,12 +314,14 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, np->name = "<NULL>";
> > if (!np->type)
> > np->type = "<NULL>";
> > +
> > + of_attach_node(np);
> > }
> > while (tag == OF_DT_BEGIN_NODE || tag == OF_DT_NOP) {
> > if (tag == OF_DT_NOP)
> > *p += 4;
> > else
> > - mem = unflatten_dt_node(blob, mem, p, np,
> > allnextpp,
> > + mem = unflatten_dt_node(blob, mem, p, np,
> > size_scan, fpsize);
> > tag = be32_to_cpup((__be32 *)(*p));
> > }
> > @@ -347,16 +341,13 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob,
> > * pointers of the nodes so the normal device-tree walking
> > functions
> > * can be used.
> > * @blob: The blob to expand
> > - * @mynodes: The device_node tree created by the call
> > * @dt_alloc: An allocator that provides a virtual address to
> > memory
> > * for the resulting tree
> > */
> > void __unflatten_device_tree(struct boot_param_header *blob,
> > - struct device_node **mynodes,
> > void * (*dt_alloc)(u64 size, u64
> > align)) {
> > unsigned long start, mem, size;
> > - struct device_node **allnextp = mynodes;
> >
> > pr_debug(" -> unflatten_device_tree()\n");
> >
> > @@ -378,7 +369,7 @@ void __unflatten_device_tree(struct
> > boot_param_header *blob, /* First pass, scan for size */
> > start = ((unsigned long)blob) +
> > be32_to_cpu(blob->off_dt_struct);
> > - size = unflatten_dt_node(blob, 0, &start, NULL, NULL, 0);
> > + size = unflatten_dt_node(blob, 0, &start, NULL, true, 0);
> > size = (size | 3) + 1;
> >
> > pr_debug(" size is %lx, allocating...\n", size);
> > @@ -394,13 +385,12 @@ void __unflatten_device_tree(struct
> > boot_param_header *blob, /* Second pass, do actual unflattening */
> > start = ((unsigned long)blob) +
> > be32_to_cpu(blob->off_dt_struct);
> > - unflatten_dt_node(blob, mem, &start, NULL, &allnextp, 0);
> > + unflatten_dt_node(blob, mem, &start, NULL, false, 0);
> > if (be32_to_cpup((__be32 *)start) != OF_DT_END)
> > pr_warning("Weird tag at end of tree: %08x\n",
> > *((u32 *)start)); if (be32_to_cpu(((__be32 *)mem)[size / 4]) !=
> > 0xdeadbeef) pr_warning("End of tree marker overwritten: %08x\n",
> > be32_to_cpu(((__be32 *)mem)[size / 4]));
> > - *allnextp = NULL;
> >
> > pr_debug(" <- unflatten_device_tree()\n");
> > }
> > @@ -423,7 +413,7 @@ void of_fdt_unflatten_tree(unsigned long *blob,
> > {
> > struct boot_param_header *device_tree =
> > (struct boot_param_header *)blob;
> > - __unflatten_device_tree(device_tree, mynodes,
> > &kernel_tree_alloc);
> > + __unflatten_device_tree(device_tree, &kernel_tree_alloc);
> > }
> > EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
> >
> > @@ -702,7 +692,7 @@ int __init early_init_dt_scan_chosen(unsigned
> > long node, const char *uname, */
> > void __init unflatten_device_tree(void)
> > {
> > - __unflatten_device_tree(initial_boot_params, &allnodes,
> > + __unflatten_device_tree(initial_boot_params,
> > early_init_dt_alloc_memory_arch);
> >
> > /* Get pointer to OF "/chosen" node for use everywhere */
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index bb36473..95754ca 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -50,7 +50,6 @@ struct device_node {
> > struct device_node *parent;
> > struct device_node *child;
> > struct device_node *sibling;
> > - struct device_node *next; /* next device of
> > same type */ struct device_node *allnext; /* next in
> > list of all nodes */ struct proc_dir_entry *pde; /*
> > this node's proc directory */ struct kref kref;
> > --
> > 1.7.2.3
> >
WARNING: multiple messages have this Message-ID (diff)
From: Andres Salomon <dilinger-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>
To: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
Daniel Drake <dsd-2X9k7bc8m7Mdnm+yROfE0A@public.gmane.org>,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH 3/3] of/flattree: use of_attach_node to build tree, and associated cleanups
Date: Sat, 12 Mar 2011 11:37:26 -0800 [thread overview]
Message-ID: <20110312113726.0bf060da@queued.net> (raw)
In-Reply-To: <20110312091056.GG9347-MrY2KI0G/OVr83L8+7iqerDks+cytr/Z@public.gmane.org>
On Sat, 12 Mar 2011 02:10:56 -0700
Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> On Wed, Mar 09, 2011 at 04:16:07PM -0800, Andres Salomon wrote:
> > Use a common function (of_attach_node) to build the device tree.
> > This simplifies the flat device tree creation a bit, and as an
> > added bonus allows us to drop a (now unused) field from the
> > device_node struct.
> >
> > There are a few minor cleanups snuck in here as well (comment
> > additions, etc).
> >
> > Signed-off-by: Andres Salomon <dilinger-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>
>
> In addition to my comment about changing the tree order on the last
> patch, unfortunately this patch will also break the newly added
> of_fdt_unflatten_tree(). of_fdt_unflatten_tree() allows a driver to
> unflatten a private dtb for its own use without it being attached to
> the global tree or the global list of all nodes. I had also forgotten
> about this. Shoot.
Ah, I was wondering what that was all about. So we'd probably end up
with something like:
void of_attach_node(struct device_node *dp)
{
unsigned long flags;
write_lock_irqsave(devtree_lock, &flags);
__of_attach_node(allnodes, dp);
write_unlock_irqrestore(devtree_lock, &flags);
}
Most stuff could get away with just calling of_attach_node, with the
unflatten_dt_node calling __of_attach_node (and either not caring
about devtree_lock, as is currently the case, or grabbing it from
unflatten_device_tree).
>
> The solution would be a variant of of_attach_node which accepts a
> private allnodes pointer. That would also help with the ordering
> issues because the order of the list could be explicitly reversed
> before assigning the value to the real allnodes pointer. Another
> possible option would be an optional 'tail' pointer argument to
> of_attach_node() which if present it would use as the insertion point
> for adding the node, which would preserve the current sort order.
I was leaning towards a tail pointer, but I need to take a closer look
at the two options.
>
> g.
>
> > ---
> > drivers/of/fdt.c | 42
> > ++++++++++++++++-------------------------- include/linux/of.h |
> > 1 - 2 files changed, 16 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index af824e7..e70cee8 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -139,16 +139,17 @@ static void *unflatten_dt_alloc(unsigned long
> > *mem, unsigned long size, /**
> > * unflatten_dt_node - Alloc and populate a device_node from the
> > flat tree
> > * @blob: The parent device tree blob
> > + * @mem: memory chunk to use for allocating device nodes and
> > properties
> > * @p: pointer to node in flat tree
> > * @dad: Parent struct device_node
> > - * @allnextpp: pointer to ->allnext from last allocated device_node
> > + * @size_scan: are we figuring out amount of memory to allocate?
> > * @fpsize: Size of the node path up at the current depth.
> > */
> > unsigned long unflatten_dt_node(struct boot_param_header *blob,
> > unsigned long mem,
> > unsigned long *p,
> > struct device_node *dad,
> > - struct device_node ***allnextpp,
> > + bool size_scan,
> > unsigned long fpsize)
> > {
> > struct device_node *np;
> > @@ -195,7 +196,7 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob,
> > np = unflatten_dt_alloc(&mem, sizeof(struct device_node) +
> > allocl, __alignof__(struct device_node));
> > - if (allnextpp) {
> > + if (!size_scan) {
> > memset(np, 0, sizeof(*np));
> > np->full_name = ((char *)np) + sizeof(struct
> > device_node); if (new_format) {
> > @@ -217,19 +218,10 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, } else
> > memcpy(np->full_name, pathp, l);
> > prev_pp = &np->properties;
> > - **allnextpp = np;
> > - *allnextpp = &np->allnext;
> > - if (dad != NULL) {
> > - np->parent = dad;
> > - /* we temporarily use the next field as
> > `last_child'*/
> > - if (dad->next == NULL)
> > - dad->child = np;
> > - else
> > - dad->next->sibling = np;
> > - dad->next = np;
> > - }
> > + np->parent = dad;
> > kref_init(&np->kref);
> > }
> > + /* process properties */
> > while (1) {
> > u32 sz, noff;
> > char *pname;
> > @@ -258,7 +250,7 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, l = strlen(pname) + 1;
> > pp = unflatten_dt_alloc(&mem, sizeof(struct
> > property), __alignof__(struct property));
> > - if (allnextpp) {
> > + if (!size_scan) {
> > /* We accept flattened tree phandles
> > either in
> > * ePAPR-style "phandle" properties, or the
> > * legacy "linux,phandle" properties. If
> > both @@ -301,7 +293,7 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, sz = (pa - ps) + 1;
> > pp = unflatten_dt_alloc(&mem, sizeof(struct
> > property) + sz, __alignof__(struct property));
> > - if (allnextpp) {
> > + if (!size_scan) {
> > pp->name = "name";
> > pp->length = sz;
> > pp->value = pp + 1;
> > @@ -313,7 +305,7 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, (char *)pp->value);
> > }
> > }
> > - if (allnextpp) {
> > + if (!size_scan) {
> > *prev_pp = NULL;
> > np->name = of_get_property(np, "name", NULL);
> > np->type = of_get_property(np, "device_type",
> > NULL); @@ -322,12 +314,14 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob, np->name = "<NULL>";
> > if (!np->type)
> > np->type = "<NULL>";
> > +
> > + of_attach_node(np);
> > }
> > while (tag == OF_DT_BEGIN_NODE || tag == OF_DT_NOP) {
> > if (tag == OF_DT_NOP)
> > *p += 4;
> > else
> > - mem = unflatten_dt_node(blob, mem, p, np,
> > allnextpp,
> > + mem = unflatten_dt_node(blob, mem, p, np,
> > size_scan, fpsize);
> > tag = be32_to_cpup((__be32 *)(*p));
> > }
> > @@ -347,16 +341,13 @@ unsigned long unflatten_dt_node(struct
> > boot_param_header *blob,
> > * pointers of the nodes so the normal device-tree walking
> > functions
> > * can be used.
> > * @blob: The blob to expand
> > - * @mynodes: The device_node tree created by the call
> > * @dt_alloc: An allocator that provides a virtual address to
> > memory
> > * for the resulting tree
> > */
> > void __unflatten_device_tree(struct boot_param_header *blob,
> > - struct device_node **mynodes,
> > void * (*dt_alloc)(u64 size, u64
> > align)) {
> > unsigned long start, mem, size;
> > - struct device_node **allnextp = mynodes;
> >
> > pr_debug(" -> unflatten_device_tree()\n");
> >
> > @@ -378,7 +369,7 @@ void __unflatten_device_tree(struct
> > boot_param_header *blob, /* First pass, scan for size */
> > start = ((unsigned long)blob) +
> > be32_to_cpu(blob->off_dt_struct);
> > - size = unflatten_dt_node(blob, 0, &start, NULL, NULL, 0);
> > + size = unflatten_dt_node(blob, 0, &start, NULL, true, 0);
> > size = (size | 3) + 1;
> >
> > pr_debug(" size is %lx, allocating...\n", size);
> > @@ -394,13 +385,12 @@ void __unflatten_device_tree(struct
> > boot_param_header *blob, /* Second pass, do actual unflattening */
> > start = ((unsigned long)blob) +
> > be32_to_cpu(blob->off_dt_struct);
> > - unflatten_dt_node(blob, mem, &start, NULL, &allnextp, 0);
> > + unflatten_dt_node(blob, mem, &start, NULL, false, 0);
> > if (be32_to_cpup((__be32 *)start) != OF_DT_END)
> > pr_warning("Weird tag at end of tree: %08x\n",
> > *((u32 *)start)); if (be32_to_cpu(((__be32 *)mem)[size / 4]) !=
> > 0xdeadbeef) pr_warning("End of tree marker overwritten: %08x\n",
> > be32_to_cpu(((__be32 *)mem)[size / 4]));
> > - *allnextp = NULL;
> >
> > pr_debug(" <- unflatten_device_tree()\n");
> > }
> > @@ -423,7 +413,7 @@ void of_fdt_unflatten_tree(unsigned long *blob,
> > {
> > struct boot_param_header *device_tree =
> > (struct boot_param_header *)blob;
> > - __unflatten_device_tree(device_tree, mynodes,
> > &kernel_tree_alloc);
> > + __unflatten_device_tree(device_tree, &kernel_tree_alloc);
> > }
> > EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
> >
> > @@ -702,7 +692,7 @@ int __init early_init_dt_scan_chosen(unsigned
> > long node, const char *uname, */
> > void __init unflatten_device_tree(void)
> > {
> > - __unflatten_device_tree(initial_boot_params, &allnodes,
> > + __unflatten_device_tree(initial_boot_params,
> > early_init_dt_alloc_memory_arch);
> >
> > /* Get pointer to OF "/chosen" node for use everywhere */
> > diff --git a/include/linux/of.h b/include/linux/of.h
> > index bb36473..95754ca 100644
> > --- a/include/linux/of.h
> > +++ b/include/linux/of.h
> > @@ -50,7 +50,6 @@ struct device_node {
> > struct device_node *parent;
> > struct device_node *child;
> > struct device_node *sibling;
> > - struct device_node *next; /* next device of
> > same type */ struct device_node *allnext; /* next in
> > list of all nodes */ struct proc_dir_entry *pde; /*
> > this node's proc directory */ struct kref kref;
> > --
> > 1.7.2.3
> >
next prev parent reply other threads:[~2011-03-12 19:37 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-03-10 0:16 [PATCH 0/3] switch DT creation to using of_attach_node Andres Salomon
2011-03-10 0:16 ` Andres Salomon
2011-03-10 0:16 ` [PATCH 1/3] of: remove OF_DYNAMIC config option Andres Salomon
2011-03-10 0:16 ` Andres Salomon
2011-03-12 9:00 ` Grant Likely
2011-03-12 9:00 ` Grant Likely
[not found] ` <1299716167-9656-1-git-send-email-dilinger-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>
2011-03-10 0:16 ` [PATCH 2/3] of/promtree: switch to building the DT using of_attach_node Andres Salomon
2011-03-10 0:16 ` Andres Salomon
2011-03-10 0:16 ` Andres Salomon
2011-03-12 9:00 ` [PATCH 2/3] of/promtree: switch to building the DT using Grant Likely
2011-03-12 9:00 ` [PATCH 2/3] of/promtree: switch to building the DT using of_attach_node Grant Likely
2011-03-10 0:16 ` [PATCH 3/3] of/flattree: use of_attach_node to build tree, and associated cleanups Andres Salomon
2011-03-12 9:10 ` Grant Likely
2011-03-12 9:10 ` Grant Likely
2011-03-12 19:37 ` Andres Salomon [this message]
2011-03-12 19:37 ` Andres Salomon
2011-03-15 3:34 ` Grant Likely
2011-03-15 3:34 ` Grant Likely
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20110312113726.0bf060da@queued.net \
--to=dilinger@queued.net \
--cc=devicetree-discuss@lists.ozlabs.org \
--cc=dsd@laptop.org \
--cc=grant.likely@secretlab.ca \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.