fdt: Deal correctly with alias nodes

PATCH Superseded Simon Glass 2 participants 7 messages Jan 11, 2012 – Jan 17, 2012 Library code

lore

Share & tools

lore

Simon Glass

author

2012-01-11 23:08 UTC #0 lore raw

v2 cover fdt: Deal correctly with alias nodes

This series proposes a new way to deal with alias nodes and introduces a function to take care of it.

Drivers can now request an ordered list of compatible nodes in a single step. This simplifies node discovery for drivers (and keeps this nasty code in a common place).

If DEBUG is defined the alias function will print messages in the event of a warning/error.

Changes in v2:
- Rename function to fdtdec_find_aliases_for_id()
- Improve alias checking algorithm to reduce run time
- Skip nodes marked as disabled
- Allow gaps in the list returned to the caller
- Add test_fdtdec command to test fdtdec_find_aliases_for_id()

Simon Glass (2):
fdt: Add fdtdec_find_aliases() to deal with alias nodes
fdt: Add tests for fdtdec

 include/fdtdec.h  |   47 +++++++++++
 lib/Makefile      |    1 +
 lib/fdtdec-test.c |  228 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/fdtdec.c      |  114 ++++++++++++++++++++++++++
 4 files changed, 390 insertions(+), 0 deletions(-)
 create mode 100644 lib/fdtdec-test.c

Signature · 1 line

1.7.3.1

Simon Glass

author

2012-01-11 23:08 UTC #1 lore raw

PATCH v2 1/2 fdt: Add fdtdec_find_aliases() to deal with alias nodes

Stephen Warren pointed out that we should use nodes whether or not they have an alias in the /aliases section. The aliases section specifies the order so far as it can, but is not essential. Operating without alisses is useful when the enumerated order of nodes does not matter (admittedly rare in U-Boot).

This is considerably more complex, and it is important to keep this complexity out of driver code. This patch creates a function fdtdec_find_aliases() which returns an ordered list of node offsets for a particular compatible ID, taking account of alias nodes.

Signed-off-by Simon Glass <[email protected]>

---
Changes in v2:
- Rename function to fdtdec_find_aliases_for_id()
- Improve alias checking algorithm to reduce run time
- Skip nodes marked as disabled
- Allow gaps in the list returned to the caller

 include/fdtdec.h |   47 ++++++++++++++++++++++
 lib/fdtdec.c     |  114 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 161 insertions(+), 0 deletions(-)

include/fdtdec.h +47 -0

index d871cdd..3a15f55 100644
@@ -126,3 +126,50 @@ int fdtdec_get_is_enabled(const void *blob, int node, int default_val);
  * if not.
  */
 int fdtdec_check_fdt(void);
+
+/**
+ * Find the nodes for a peripheral and return a list of them in the correct
+ * order. This is used to enumerate all the peripherals of a certain type.
+ *
+ * To use this, optionally set up a /aliases node with alias properties for
+ * a peripheral. For example, for usb you could have:
+ *
+ * aliases {
+ *		usb0 = "/ehci at c5008000";
+ *		usb1 = "/ehci at c5000000";
+ * };
+ *
+ * Pass "usb" as the name to this function and will return a list of two
+ * nodes offsets: /ehci at c5008000 and ehci at c5000000.
+ *
+ * All nodes returned will match the compatible ID, as it is assumed that
+ * all peripherals use the same driver.
+ *
+ * If no alias node is found, then the node list will be returned in the
+ * order found in the fdt. If the aliases mention a node which doesn't
+ * exist, then this will be ignored. If nodes are found with no aliases,
+ * they will be added in any order.
+ *
+ * If there is a gap in the aliases, then this function return a 0 node at
+ * that position. The return value will also count these gaps.
+ *
+ * This function checks node properties and will not return node which are
+ * marked disabled (status = "disabled").
+ *
+ * @param blob		FDT blob to use
+ * @param name		Root name of alias to search for
+ * @param id		Compatible ID to look for
+ * @param node		Place to put list of found nodes
+ * @param maxcount	Maximum number of nodes to find
+ * @return number of nodes found on success, FTD_ERR_... on error
+ */
+int fdtdec_find_aliases_for_id(const void *blob, const char *name,
+			enum fdt_compat_id id, int *node_list, int maxcount);
+
+/*
+ * Get the name for a compatible ID
+ *
+ * @param id		Compatible ID to look for
+ * @return compatible string for that id
+ */
+const char *fdtdec_get_compatible(enum fdt_compat_id id);

lib/fdtdec.c +114 -0

126 lines not on this page · raw message

Signature · 1 line

1.7.3.1

Stephen Warren

2012-01-13 21:41 UTC #2 lore raw

PATCH v2 1/2 fdt: Add fdtdec_find_aliases() to deal with alias nodes

Quoted 10 lines · Simon Glass

Stephen Warren pointed out that we should use nodes whether or not they have an alias in the /aliases section. The aliases section specifies the order so far as it can, but is not essential. Operating without alisses is useful when the enumerated order of nodes does not matter (admittedly rare in U-Boot).
This is considerably more complex, and it is important to keep this complexity out of driver code. This patch creates a function fdtdec_find_aliases() which returns an ordered list of node offsets for a particular compatible ID, taking account of alias nodes.

Overall, this looks basically OK. Just a few minor comments below, but once those are fixed,

Acked-by Stephen Warren <[email protected]>

Quoted 4 lines

include/fdtdec.h +2 -0

 
+/**
+ * Find the nodes for a peripheral and return a list of them in the correct

...

Quoted 1 line

+ * This function checks node properties and will not return node which are

s/node/nodes/

Quoted 6 lines

+ * marked disabled (status = "disabled").
+ *
+ * @param blob FDT blob to use
+ * @param name Root name of alias to search for
+ * @param id Compatible ID to look for
+ * @param node Place to put list of found nodes

s/node/node_list/

Quoted 10 lines

+ * @param maxcount Maximum number of nodes to find
+ * @return number of nodes found on success, FTD_ERR_... on error
+ */
include/fdtdec.h +3 -0
+int fdtdec_find_aliases_for_id(const void *blob, const char *name,
+			enum fdt_compat_id id, int *node_list, int maxcount);
+

lib/fdtdec.c DECLARE_GLOBAL_DATA_PTR; · line 40

Quoted 6 lines

lib/fdtdec.c +3 -0

@@ -35,6 +35,11 @@ DECLARE_GLOBAL_DATA_PTR;
 static const char * const compat_names[COMPAT_COUNT] = {
 };
 
+const char *fdtdec_get_compatible(enum fdt_compat_id id)
+{
+	return compat_names[id];

Range-check?

Quoted 14 lines

lib/fdtdec.c +14 -0

+/* TODO: Can we tighten this code up a little? */
+int fdtdec_find_aliases_for_id(const void *blob, const char *name,
+			enum fdt_compat_id id, int *node_list, int maxcount)
+{
+	int name_len = strlen(name);
+	int nodes[maxcount];
+	int num_found = 0;
+	int offset, node;
+	int alias_node;
+	int count;
+	int i, j;
+
+	/* find the alias node if present */
+	alias_node = fdt_path_offset(blob, "/aliases");

lib/fdtdec.c int fdtdec_next_alias(const void *blob, const char *name, · line 163

Quoted 10 lines

lib/fdtdec.c +10 -0

+
+	/*
+	 * start with nothing, and we can assume that the root node can't
+	 * match
+	 */
+	memset(nodes, '\0', sizeof(nodes));
+
+	/* First find all the compatible nodes */
+	node = 0;
+	for (node = count = 0; node >= 0 && count < maxcount;) {

node = 0 is duplicated on those two lines.

Quoted 47 lines

lib/fdtdec.c +47 -0

+		node = fdtdec_next_compatible(blob, node, id);
+		if (node >= 0)
+			nodes[count++] = node;
+	}
+	if (node >= 0)
+		debug("%s: warning: maxcount exceeded with alias '%s'\n",
+		       __func__, name);
+
+	/* Now find all the aliases */
+	memset(node_list, '\0', sizeof(*node_list) * maxcount);
+
+	for (offset = fdt_first_property_offset(blob, alias_node);
+			offset > 0;
+			offset = fdt_next_property_offset(blob, offset)) {
+		const struct fdt_property *prop;
+		const char *path;
+		int number;
+		int found;
+
+		node = 0;
+		prop = fdt_get_property_by_offset(blob, offset, NULL);
+		path = fdt_string(blob, fdt32_to_cpu(prop->nameoff));
+		if (prop->len && 0 == strncmp(path, name, name_len))
+			node = fdt_path_offset(blob, prop->data);
+		if (node <= 0)
+			continue;
+
+		/* Get the alias number */
+		number = simple_strtoul(path + name_len, NULL, 10);
+		if (number < 0 || number >= maxcount) {
+			debug("%s: warning: alias '%s' is out of range\n",
+			       __func__, path);
+			continue;
+		}
+
+		/* Make sure the node we found is actually in our list! */
+		found = -1;
+		for (j = 0; j < count; j++)
+			if (nodes[j] == node) {
+				found = j;
+				break;
+			}
+
+		if (found == -1) {
+			debug("%s: warning: alias '%s' points to a node "
+				"'%s' that is missing or is not compatible "
+				" with '%s'\n", __func__, path,

lib/fdtdec.c int fdtdec_next_alias(const void *blob, const char *name, · line 220

Quoted 10 lines

lib/fdtdec.c +10 -0

+				fdt_get_name(blob, node, NULL),
+			       compat_names[id]);
+			continue;
+		}
+
+		/*
+		 * Add this node to our list in the right place, and mark
+		 * it as done.
+		 */
+		if (fdtdec_get_is_enabled(blob, node)) {

Do you want to warn here if node_list[number] is already set; if the user creates multiple aliases with the same ID?

Quoted 8 lines

lib/fdtdec.c +8 -0

+			node_list[number] = node;
+			if (number >= num_found)
+				num_found = number + 1;
+		}
+		nodes[j] = 0;
+	}
+
+	/* Add any nodes not mentioned by an alias */

lib/fdtdec.c int fdtdec_next_alias(const void *blob, const char *name, · line 238

Quoted 10 lines

lib/fdtdec.c +10 -0

+	for (i = j = 0; i < maxcount; i++) {
+		if (!node_list[i]) {
+			for (; j < maxcount; j++)
+				if (nodes[j] &&
+					fdtdec_get_is_enabled(blob, nodes[j]))
+					break;
+
+			/* Have we run out of nodes to add? */
+			if (j == maxcount)
+				continue;

break?

Quoted 10 lines

lib/fdtdec.c +10 -0

+
+			node_list[i] = nodes[j++];
+			if (i >= num_found)
+				num_found = i + 1;
+		}
+	}
+
+	return num_found;
+}
+

Signature · 1 line

nvpublic

Simon Glass

author

2012-01-17 18:19 UTC #3 lore raw

PATCH v2 1/2 fdt: Add fdtdec_find_aliases() to deal with alias nodes

Hi Stephen,

Quoted 48 lines · Stephen Warren

Quoted 10 lines · Simon Glass
Stephen Warren pointed out that we should use nodes whether or not they have an alias in the /aliases section. The aliases section specifies the order so far as it can, but is not essential. Operating without alisses is useful when the enumerated order of nodes does not matter (admittedly rare in U-Boot).
This is considerably more complex, and it is important to keep this complexity out of driver code. This patch creates a function fdtdec_find_aliases() which returns an ordered list of node offsets for a particular compatible ID, taking account of alias nodes.
Overall, this looks basically OK. Just a few minor comments below, but once those are fixed,
Acked-by Stephen Warren <[email protected]>
Quoted 4 lines
include/fdtdec.h +2 -0
 
+/**
+ * Find the nodes for a peripheral and return a list of them in the correct
...
Quoted 1 line
+ * This function checks node properties and will not return node which are
s/node/nodes/
Quoted 6 lines
+ * marked disabled (status = "disabled").
+ *
+ * @param blob ? ? ? ? ? ? ? FDT blob to use
+ * @param name ? ? ? ? ? ? ? Root name of alias to search for
+ * @param id ? ? ? ? Compatible ID to look for
+ * @param node ? ? ? ? ? ? ? Place to put list of found nodes
s/node/node_list/
Quoted 13 lines
+ * @param maxcount ? Maximum number of nodes to find
+ * @return number of nodes found on success, FTD_ERR_... on error
+ */
include/fdtdec.h +3 -0
+int fdtdec_find_aliases_for_id(const void *blob, const char *name,
+ ? ? ? ? ? ? ? ? ? ? enum fdt_compat_id id, int *node_list, int maxcount);
+
lib/fdtdec.c +0 -0
@@ -35,6 +35,11 @@ DECLARE_GLOBAL_DATA_PTR;
 ?static const char * const compat_names[COMPAT_COUNT] = {
 ?};
 

Quoted 5 lines

lib/fdtdec.c DECLARE_GLOBAL_DATA_PTR;
Quoted 3 lines
lib/fdtdec.c +3 -0
+const char *fdtdec_get_compatible(enum fdt_compat_id id)
+{
+ ? ? return compat_names[id];
Range-check?

These and other changes made - but see one comment below.

Quoted 80 lines

Quoted 15 lines

lib/fdtdec.c +14 -0

+/* TODO: Can we tighten this code up a little? */
+int fdtdec_find_aliases_for_id(const void *blob, const char *name,
+ ? ? ? ? ? ? ? ? ? ? enum fdt_compat_id id, int *node_list, int maxcount)
+{
+ ? ? int name_len = strlen(name);
+ ? ? int nodes[maxcount];
+ ? ? int num_found = 0;
+ ? ? int offset, node;
+ ? ? int alias_node;
+ ? ? int count;
+ ? ? int i, j;
+
+ ? ? /* find the alias node if present */
+ ? ? alias_node = fdt_path_offset(blob, "/aliases");

lib/fdtdec.c DECLARE_GLOBAL_DATA_PTR;

Quoted 10 lines

lib/fdtdec.c +10 -0

+
+ ? ? /*
+ ? ? ?* start with nothing, and we can assume that the root node can't
+ ? ? ?* match
+ ? ? ?*/
+ ? ? memset(nodes, '\0', sizeof(nodes));
+
+ ? ? /* First find all the compatible nodes */
+ ? ? node = 0;
+ ? ? for (node = count = 0; node >= 0 && count < maxcount;) {

node = 0 is duplicated on those two lines.

Quoted 52 lines

+ ? ? ? ? ? ? node = fdtdec_next_compatible(blob, node, id);
+ ? ? ? ? ? ? if (node >= 0)
+ ? ? ? ? ? ? ? ? ? ? nodes[count++] = node;
+ ? ? }
+ ? ? if (node >= 0)
+ ? ? ? ? ? ? debug("%s: warning: maxcount exceeded with alias '%s'\n",
+ ? ? ? ? ? ? ? ? ? ?__func__, name);

lib/fdtdec.c +45 -0

+
+ ? ? /* Now find all the aliases */
+ ? ? memset(node_list, '\0', sizeof(*node_list) * maxcount);
+
+ ? ? for (offset = fdt_first_property_offset(blob, alias_node);
+ ? ? ? ? ? ? ? ? ? ? offset > 0;
+ ? ? ? ? ? ? ? ? ? ? offset = fdt_next_property_offset(blob, offset)) {
+ ? ? ? ? ? ? const struct fdt_property *prop;
+ ? ? ? ? ? ? const char *path;
+ ? ? ? ? ? ? int number;
+ ? ? ? ? ? ? int found;
+
+ ? ? ? ? ? ? node = 0;
+ ? ? ? ? ? ? prop = fdt_get_property_by_offset(blob, offset, NULL);
+ ? ? ? ? ? ? path = fdt_string(blob, fdt32_to_cpu(prop->nameoff));
+ ? ? ? ? ? ? if (prop->len && 0 == strncmp(path, name, name_len))
+ ? ? ? ? ? ? ? ? ? ? node = fdt_path_offset(blob, prop->data);
+ ? ? ? ? ? ? if (node <= 0)
+ ? ? ? ? ? ? ? ? ? ? continue;
+
+ ? ? ? ? ? ? /* Get the alias number */
+ ? ? ? ? ? ? number = simple_strtoul(path + name_len, NULL, 10);
+ ? ? ? ? ? ? if (number < 0 || number >= maxcount) {
+ ? ? ? ? ? ? ? ? ? ? debug("%s: warning: alias '%s' is out of range\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?__func__, path);
+ ? ? ? ? ? ? ? ? ? ? continue;
+ ? ? ? ? ? ? }
+
+ ? ? ? ? ? ? /* Make sure the node we found is actually in our list! */
+ ? ? ? ? ? ? found = -1;
+ ? ? ? ? ? ? for (j = 0; j < count; j++)
+ ? ? ? ? ? ? ? ? ? ? if (nodes[j] == node) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? found = j;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? ? ? ? }
+
+ ? ? ? ? ? ? if (found == -1) {
+ ? ? ? ? ? ? ? ? ? ? debug("%s: warning: alias '%s' points to a node "
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? "'%s' that is missing or is not compatible "
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? " with '%s'\n", __func__, path,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? fdt_get_name(blob, node, NULL),
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?compat_names[id]);
+ ? ? ? ? ? ? ? ? ? ? continue;
+ ? ? ? ? ? ? }
+

Quoted 8 lines

Quoted 5 lines
+ ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ?* Add this node to our list in the right place, and mark
+ ? ? ? ? ? ? ?* it as done.
+ ? ? ? ? ? ? ?*/
+ ? ? ? ? ? ? if (fdtdec_get_is_enabled(blob, node)) {
Do you want to warn here if node_list[number] is already set; if the user creates multiple aliases with the same ID?

I deal with this in a later series which supports multiple compatible IDs - for now I will add an assert().

Quoted 13 lines

Quoted 13 lines
+ ? ? ? ? ? ? ? ? ? ? node_list[number] = node;
+ ? ? ? ? ? ? ? ? ? ? if (number >= num_found)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? num_found = number + 1;
+ ? ? ? ? ? ? }
+ ? ? ? ? ? ? nodes[j] = 0;
+ ? ? }
+
+ ? ? /* Add any nodes not mentioned by an alias */
+ ? ? for (i = j = 0; i < maxcount; i++) {
+ ? ? ? ? ? ? if (!node_list[i]) {
+ ? ? ? ? ? ? ? ? ? ? for (; j < maxcount; j++)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? if (nodes[j] &&

Quoted 8 lines

Quoted 6 lines
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? fdtdec_get_is_enabled(blob, nodes[j]))
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
+
+ ? ? ? ? ? ? ? ? ? ? /* Have we run out of nodes to add? */
+ ? ? ? ? ? ? ? ? ? ? if (j == maxcount)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? continue;
break?

Yes, they are equivalent so I have changed it.

Quoted 12 lines

lib/fdtdec.c

Quoted 11 lines

lib/fdtdec.c +10 -0

+
+ ? ? ? ? ? ? ? ? ? ? node_list[i] = nodes[j++];
+ ? ? ? ? ? ? ? ? ? ? if (i >= num_found)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? num_found = i + 1;
+ ? ? ? ? ? ? }
+ ? ? }
+
+ ? ? return num_found;
+}
+

Regards,
Simon

Quoted 2 lines

--
nvpublic

Simon Glass

author

2012-01-11 23:08 UTC #4 lore raw

PATCH v2 2/2 fdt: Add tests for fdtdec

In reply to Simon Glass

The fdtdec_find_aliases_for_id() function is complicated enough that it really should have some tests. This does not necessarily need to be committed to U-Boot, but it might be useful.

Signed-off-by Simon Glass <[email protected]>

---
Changes in v2:
- Add test_fdtdec command to test fdtdec_find_aliases_for_id()

 lib/Makefile      |    1 +
 lib/fdtdec-test.c |  228 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 229 insertions(+), 0 deletions(-)
 create mode 100644 lib/fdtdec-test.c

lib/Makefile +1 -0

index 35ba7ff..421b7ac 100644
@@ -39,6 +39,7 @@ COBJS-y += crc32.o
 COBJS-y += display_options.o
 COBJS-y += errno.o
 COBJS-$(CONFIG_OF_CONTROL) += fdtdec.o
+COBJS-$(CONFIG_TEST_FDTDEC) += fdtdec-test.o
 COBJS-$(CONFIG_GZIP) += gunzip.o
 COBJS-y += hashtable.o
 COBJS-$(CONFIG_LMB) += lmb.o

lib/fdtdec-test.c new file +228 -0

228 lines not on this page · raw message

Signature · 1 line

1.7.3.1

Stephen Warren

2012-01-13 21:49 UTC #5 lore raw

PATCH v2 2/2 fdt: Add tests for fdtdec

Quoted 7 lines · Simon Glass

The fdtdec_find_aliases_for_id() function is complicated enough that it really should have some tests. This does not necessarily need to be committed to U-Boot, but it might be useful.
Signed-off-by Simon Glass <[email protected]>
+static int run_test(const char *aliases, const char *nodes, const char *expect)

...

Quoted 1 line

lib/fdtdec-test.c new file +1 -0
+	va_list ptr;

...

lib/fdtdec-test.c line 174

Quoted 1 line

lib/fdtdec-test.c new file +1 -0
+	va_end(ptr);

ptr doesn't seem to be used, and this isn't a varargs function.

Otherwise,

Acked-by Stephen Warren <[email protected]>

Signature · 1 line

nvpublic

Simon Glass

author

2012-01-17 18:17 UTC #6 lore raw

PATCH v2 2/2 fdt: Add tests for fdtdec

Hi Stephen,

Quoted 7 lines · Stephen Warren

Quoted 5 lines · Simon Glass
The fdtdec_find_aliases_for_id() function is complicated enough that it really should have some tests. This does not necessarily need to be committed to U-Boot, but it might be useful.
Signed-off-by Simon Glass <[email protected]>

Quoted 7 lines

Quoted 1 line
+static int run_test(const char *aliases, const char *nodes, const char *expect)
...
Quoted 1 line
+ ? ? va_list ptr;
...
Quoted 1 line
+ ? ? va_end(ptr);
ptr doesn't seem to be used, and this isn't a varargs function.

Yes, it's a hangover from my previous non-string implementation. I will send a new series as there are a couple of changes.

Regards,
Simon

Quoted 7 lines

Otherwise,
Acked-by Stephen Warren <[email protected]>
--
nvpublic