[U-Boot] [RFC PATCH 0/2] fdt: Deal correctly with alias nodes

[U-Boot] [RFC PATCH 0/2] fdt: Deal correctly with alias nodes

[Simon Glass]

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

It includes an example of converting USB code over to use this new
function.

Note: At present it does not deal automatically with disabled nodes,
but perhaps it should? Or perhaps this is better as an option. Something
to leave for later perhaps.

Note 2: The actual logic of this function is not properly tested yet.


Simon Glass (2):
  fdt: Add fdtdec_find_aliases() to deal with alias nodes
  tegra: Use fdtdec_find_aliases() to find USB ports

 arch/arm/cpu/armv7/tegra2/usb.c |   17 ++++----
 include/fdtdec.h                |   51 ++++++++++++++++++++++
 lib/fdtdec.c                    |   89 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 148 insertions(+), 9 deletions(-)

-- 
1.7.3.1

[U-Boot] [RFC PATCH 1/2] fdt: Add fdtdec_find_aliases() to deal with alias nodes

[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.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 include/fdtdec.h |   51 +++++++++++++++++++++++++++++++
 lib/fdtdec.c     |   89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 140 insertions(+), 0 deletions(-)

diff --git a/include/fdtdec.h b/include/fdtdec.h
index 5547676..0992130 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -228,3 +228,54 @@ int fdtdec_decode_gpio(const void *blob, int node, const char *prop_name,
  * @return 0 if all ok or gpio was FDT_GPIO_NONE; -1 on error
  */
 int fdtdec_setup_gpio(struct fdt_gpio_state *gpio);
+
+/**
+ * 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.
+ *
+ * The array returned will not have any gaps.
+ *
+ * If there is a gap in the aliases, then this function will only return up
+ * to the number of nodes it found until the gap. It will also print a warning
+ * in this case. As an example, say you define aliases for usb2 and usb3, and
+ * have 3 nodes. Then in this case the node without an alias will become usb0
+ * and the aliases will be use for usb2 and usb3. But since there is no
+ * usb1, this function will only list one node (usb0), and will print a
+ * warning.
+ *
+ * This function does not check node properties - so it is possible that the
+ * node is marked disabled (status = "disabled"). The caller is expected to
+ * deal with this.
+ * TBD: It might be nicer to handle this here since we don't want a
+ * discontiguous list to result in the caller.
+ *
+ * Note: the algorithm used is O(maxcount).
+ *
+ * @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(const void *blob, const char *name,
+			enum fdt_compat_id id, int *node_list, int maxcount);
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index fb3d79d..b01978d 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -147,6 +147,95 @@ int fdtdec_next_alias(const void *blob, const char *name,
 	return node;
 }
 
+/* TODO: Can we tighten this code up a little? */
+int fdtdec_find_aliases(const void *blob, const char *name,
+			enum fdt_compat_id id, int *node_list, int maxcount)
+{
+	int nodes[maxcount];
+	int num_found = 0;
+	int alias_node;
+	int node;
+	int count;
+	int i, j;
+
+	/* find the alias node if present */
+	alias_node = fdt_path_offset(blob, "/aliases");
+
+	/*
+	 * 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 = fdtdec_next_compatible(blob, node, id);
+		if (node > 0)
+			nodes[count++] = node;
+	}
+
+	/* Now find all the aliases */
+	memset(node_list, '\0', sizeof(*node_list) * maxcount);
+	for (i = 0; i < maxcount; i++) {
+		int found;
+		const char *path;
+
+		path = fdt_getprop(blob, alias_node, name, NULL);
+		node = path ? fdt_path_offset(blob, path) : 0;
+		if (node <= 0)
+			continue;
+
+		/* Make sure the node we found is in our list! */
+		found = -1;
+		for (j = 0; j < count; j++)
+			if (nodes[j] == node) {
+				found = j;
+				break;
+			}
+
+		if (found == -1) {
+			printf("%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;
+		}
+
+		/*
+		 * Add this node to our list in the right place, and mark it
+		 * as done.
+		 */
+		node_list[i] = node;
+		nodes[j] = 0;
+		if (j >= num_found)
+			num_found = j + 1;
+	}
+
+	/* Add any nodes not mentioned by an alias */
+	for (i = j = 0; i < maxcount; i++) {
+		if (!node_list[i]) {
+			for (; j < maxcount && !nodes[j]; j++)
+				;
+
+			/* Have we run out of nodes to add? */
+			if (j == maxcount)
+				break;
+
+			node_list[i] = nodes[j];
+		}
+	}
+
+	if (i != num_found) {
+		printf("%s: warning: for alias '%s' we found aliases up to "
+			"%d but only %d nodes, thus leaving a gap. Returning "
+			"only %d nodes\n", __func__, name, num_found, i, i);
+	}
+
+	return i;
+}
+
 /*
  * This function is a little odd in that it accesses global data. At some
  * point if the architecture board.c files merge this will make more sense.
-- 
1.7.3.1

[U-Boot] [RFC PATCH 2/2] tegra: Use fdtdec_find_aliases() to find USB ports

[Simon Glass]

Use the new fdtdec_find_aliases() function instead of
fdtdec_next_alias() to locate our USB nodes.

As this example shows, the impact on drivers should be minimal.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/cpu/armv7/tegra2/usb.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/arm/cpu/armv7/tegra2/usb.c b/arch/arm/cpu/armv7/tegra2/usb.c
index 215d0d3..5b2bd7e 100644
--- a/arch/arm/cpu/armv7/tegra2/usb.c
+++ b/arch/arm/cpu/armv7/tegra2/usb.c
@@ -395,21 +395,20 @@ int fdt_decode_usb(const void *blob, int node, unsigned osc_frequency_mhz,
 int board_usb_init(const void *blob)
 {
 	struct fdt_usb config;
-	int node, upto = 0;
 	unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
 	enum clock_osc_freq freq;
+	int node_list[USB_PORTS_MAX];
+	int node, count, i;
 
 	/* Set up the USB clocks correctly based on our oscillator frequency */
 	freq = clock_get_osc_freq();
 	config_clock(usb_pll[freq]);
 
-	do {
-		node = fdtdec_next_alias(blob, "usb",
-					 COMPAT_NVIDIA_TEGRA20_USB, &upto);
-		if (node < 0) {
-			debug("Cannot find usb%d alias in fdt\n", upto);
-			break;
-		}
+	/* count may return <0 on error */
+	count = fdtdec_find_aliases(blob, "usb", COMPAT_NVIDIA_TEGRA20_USB,
+				    node_list, USB_PORTS_MAX);
+	for (i = 0; i < count; i++) {
+		node = node_list[i];
 		if (fdt_decode_usb(blob, node, osc_freq, &config)) {
 			debug("Cannot decode USB node %s\n",
 			      fdt_get_name(blob, node, NULL));
@@ -423,7 +422,7 @@ int board_usb_init(const void *blob)
 
 		if (add_port(&config, usb_pll[freq]))
 			return -1;
-	} while (node);
+	}
 	usb_set_host_mode();
 	port_current = -1;
 	return 0;
-- 
1.7.3.1

[U-Boot] [RFC PATCH 1/2] fdt: Add fdtdec_find_aliases() to deal with alias nodes

[Simon Glass]

Hi,

On Mon, Dec 26, 2011 at 2:31 PM, Simon Glass <sjg@chromium.org> wrote:
> 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 <sjg@chromium.org>

Does this look like a reasonable solution to the problem as described?
Any comments before I respin as a real patch?

Regards,
Simon

> ---
> ?include/fdtdec.h | ? 51 +++++++++++++++++++++++++++++++
> ?lib/fdtdec.c ? ? | ? 89 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ?2 files changed, 140 insertions(+), 0 deletions(-)
>
> diff --git a/include/fdtdec.h b/include/fdtdec.h
> index 5547676..0992130 100644
> --- a/include/fdtdec.h
> +++ b/include/fdtdec.h
> @@ -228,3 +228,54 @@ int fdtdec_decode_gpio(const void *blob, int node, const char *prop_name,
> ?* @return 0 if all ok or gpio was FDT_GPIO_NONE; -1 on error
> ?*/
> ?int fdtdec_setup_gpio(struct fdt_gpio_state *gpio);
> +
> +/**
> + * 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.
> + *
> + * The array returned will not have any gaps.
> + *
> + * If there is a gap in the aliases, then this function will only return up
> + * to the number of nodes it found until the gap. It will also print a warning
> + * in this case. As an example, say you define aliases for usb2 and usb3, and
> + * have 3 nodes. Then in this case the node without an alias will become usb0
> + * and the aliases will be use for usb2 and usb3. But since there is no
> + * usb1, this function will only list one node (usb0), and will print a
> + * warning.
> + *
> + * This function does not check node properties - so it is possible that the
> + * node is marked disabled (status = "disabled"). The caller is expected to
> + * deal with this.
> + * TBD: It might be nicer to handle this here since we don't want a
> + * discontiguous list to result in the caller.
> + *
> + * Note: the algorithm used is O(maxcount).
> + *
> + * @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(const void *blob, const char *name,
> + ? ? ? ? ? ? ? ? ? ? ? enum fdt_compat_id id, int *node_list, int maxcount);
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index fb3d79d..b01978d 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -147,6 +147,95 @@ int fdtdec_next_alias(const void *blob, const char *name,
> ? ? ? ?return node;
> ?}
>
> +/* TODO: Can we tighten this code up a little? */
> +int fdtdec_find_aliases(const void *blob, const char *name,
> + ? ? ? ? ? ? ? ? ? ? ? enum fdt_compat_id id, int *node_list, int maxcount)
> +{
> + ? ? ? int nodes[maxcount];
> + ? ? ? int num_found = 0;
> + ? ? ? int alias_node;
> + ? ? ? int node;
> + ? ? ? int count;
> + ? ? ? int i, j;
> +
> + ? ? ? /* find the alias node if present */
> + ? ? ? alias_node = fdt_path_offset(blob, "/aliases");
> +
> + ? ? ? /*
> + ? ? ? ?* 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 = fdtdec_next_compatible(blob, node, id);
> + ? ? ? ? ? ? ? if (node > 0)
> + ? ? ? ? ? ? ? ? ? ? ? nodes[count++] = node;
> + ? ? ? }
> +
> + ? ? ? /* Now find all the aliases */
> + ? ? ? memset(node_list, '\0', sizeof(*node_list) * maxcount);
> + ? ? ? for (i = 0; i < maxcount; i++) {
> + ? ? ? ? ? ? ? int found;
> + ? ? ? ? ? ? ? const char *path;
> +
> + ? ? ? ? ? ? ? path = fdt_getprop(blob, alias_node, name, NULL);
> + ? ? ? ? ? ? ? node = path ? fdt_path_offset(blob, path) : 0;
> + ? ? ? ? ? ? ? if (node <= 0)
> + ? ? ? ? ? ? ? ? ? ? ? continue;
> +
> + ? ? ? ? ? ? ? /* Make sure the node we found is in our list! */
> + ? ? ? ? ? ? ? found = -1;
> + ? ? ? ? ? ? ? for (j = 0; j < count; j++)
> + ? ? ? ? ? ? ? ? ? ? ? if (nodes[j] == node) {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? found = j;
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
> + ? ? ? ? ? ? ? ? ? ? ? }
> +
> + ? ? ? ? ? ? ? if (found == -1) {
> + ? ? ? ? ? ? ? ? ? ? ? printf("%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;
> + ? ? ? ? ? ? ? }
> +
> + ? ? ? ? ? ? ? /*
> + ? ? ? ? ? ? ? ?* Add this node to our list in the right place, and mark it
> + ? ? ? ? ? ? ? ?* as done.
> + ? ? ? ? ? ? ? ?*/
> + ? ? ? ? ? ? ? node_list[i] = node;
> + ? ? ? ? ? ? ? nodes[j] = 0;
> + ? ? ? ? ? ? ? if (j >= num_found)
> + ? ? ? ? ? ? ? ? ? ? ? num_found = j + 1;
> + ? ? ? }
> +
> + ? ? ? /* Add any nodes not mentioned by an alias */
> + ? ? ? for (i = j = 0; i < maxcount; i++) {
> + ? ? ? ? ? ? ? if (!node_list[i]) {
> + ? ? ? ? ? ? ? ? ? ? ? for (; j < maxcount && !nodes[j]; j++)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ;
> +
> + ? ? ? ? ? ? ? ? ? ? ? /* Have we run out of nodes to add? */
> + ? ? ? ? ? ? ? ? ? ? ? if (j == maxcount)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? break;
> +
> + ? ? ? ? ? ? ? ? ? ? ? node_list[i] = nodes[j];
> + ? ? ? ? ? ? ? }
> + ? ? ? }
> +
> + ? ? ? if (i != num_found) {
> + ? ? ? ? ? ? ? printf("%s: warning: for alias '%s' we found aliases up to "
> + ? ? ? ? ? ? ? ? ? ? ? "%d but only %d nodes, thus leaving a gap. Returning "
> + ? ? ? ? ? ? ? ? ? ? ? "only %d nodes\n", __func__, name, num_found, i, i);
> + ? ? ? }
> +
> + ? ? ? return i;
> +}
> +
> ?/*
> ?* This function is a little odd in that it accesses global data. At some
> ?* point if the architecture board.c files merge this will make more sense.
> --
> 1.7.3.1
>

[U-Boot] [RFC PATCH 1/2] fdt: Add fdtdec_find_aliases() to deal with alias nodes

[Stephen Warren]

On 12/26/2011 03:31 PM, Simon Glass wrote:
> 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).
...
> +/**
> + * 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.
> + *
> + * The array returned will not have any gaps.

You can't make that guarantee without incorrectly parsing the device
tree; I don't believe there's any restriction on the IDs in the aliases
being contiguous. Maybe in practice this restriction will be fine, but
it doesn't seem like a great idea.

> + * If there is a gap in the aliases, then this function will only return up
> + * to the number of nodes it found until the gap. It will also print a warning
> + * in this case. As an example, say you define aliases for usb2 and usb3, and
> + * have 3 nodes. Then in this case the node without an alias will become usb0
> + * and the aliases will be use for usb2 and usb3. But since there is no
> + * usb1, this function will only list one node (usb0), and will print a
> + * warning.
> + *
> + * This function does not check node properties - so it is possible that the
> + * node is marked disabled (status = "disabled"). The caller is expected to
> + * deal with this.
> + * TBD: It might be nicer to handle this here since we don't want a
> + * discontiguous list to result in the caller.

Yes, I think handling disabled is a requirement; Tegra has quite a few
instances of each HW module, and in many cases, not all of them are used
by a given board design, so they're marked disabled.

I don't think this has any impact on handling discontiguous device IDs;
I think we need that anyway.

The itself array could always be contiguous if each entry were a pair
(id, node) instead of the ID being implied by the array index.

> + *
> + * Note: the algorithm used is O(maxcount).
> + *
> + * @param blob		FDT blob to use
> + * @param name		Root name of alias to search for
> + * @param id		Compatible ID to look for

That's a little restrictive. Many drivers will handle multiple
compatible values, e.g. N manufactures each making identical chips but
giving each its own marketing name. These need different compatible
flags in case some bug WAR needs to differentiate between them. Equally,
Tegra30's say I2C controllers will be compatible with both
nvidia,tegra30-i2c and nvidia,tegra20-i2c. While missing out the Tegra20
compatible value would probably technically be a bug in the device tree,
it does seem reasonable to expect the driver to still match on the
Tegra30 compatible value.

> + * @param node		Place to put list of found nodes
> + * @param maxcount	Maximum number of nodes to find

It'd be nice not to have maxcount; it seems slightly restrictive for a
helper function. I suppose that most drivers can supply a reasonable
value for this since there's a certain max number of devices possible
given extant HW designs, but when you start talking about e.g. a driver
for an I2C bus multiplexer, where there's one instance per chip on a
board, the number begins to get a bit arbitrary.

> + * @return number of nodes found on success, FTD_ERR_... on error
> + */
> +int fdtdec_find_aliases(const void *blob, const char *name,
> +			enum fdt_compat_id id, int *node_list, int maxcount);

Overall, I was expecting something more like:

Enumeration:
* Enumerate device tree solely based on compatible values

After enumeration is complete:
* Walk aliases node, and for each entry assign that ID value to the
appropriate device.
* Walk all devices, and assign a free ID to each device that doesn't
already have one.

Still, I suppose in practice this current code will probably achieve
equivalent results, and it doesn't have any impact of the device tree
syntax/content (unless people change the way they write .dts files to
WAR the issues above instead of enhancing the code), so can easily be
enhanced to address the issues above as needed. So, I guess it'll do
(with the exception of needing to handled disabled devices).

-- 
nvpublic

[U-Boot] [RFC PATCH 1/2] fdt: Add fdtdec_find_aliases() to deal with alias nodes

[Simon Glass]

Hi Stephen,

On Tue, Jan 10, 2012 at 12:27 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 12/26/2011 03:31 PM, Simon Glass wrote:
>> 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).
> ...
>> +/**
>> + * 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.
>> + *
>> + * The array returned will not have any gaps.

Thanks for the detailed comments - much appreciated.

>
> You can't make that guarantee without incorrectly parsing the device
> tree; I don't believe there's any restriction on the IDs in the aliases
> being contiguous. Maybe in practice this restriction will be fine, but
> it doesn't seem like a great idea.

Well actually I was thinking from a U-Boot POV since if someone uses a
device that doesn't exist U-Boot may just crash or hang. So having
such a hole would normally be a bug. But since there is no restriction
in the fdt format, and since I suppose we have to assume the user
knows what he is doing, I will remove this restriction.

>
>> + * If there is a gap in the aliases, then this function will only return up
>> + * to the number of nodes it found until the gap. It will also print a warning
>> + * in this case. As an example, say you define aliases for usb2 and usb3, and
>> + * have 3 nodes. Then in this case the node without an alias will become usb0
>> + * and the aliases will be use for usb2 and usb3. But since there is no
>> + * usb1, this function will only list one node (usb0), and will print a
>> + * warning.
>> + *
>> + * This function does not check node properties - so it is possible that the
>> + * node is marked disabled (status = "disabled"). The caller is expected to
>> + * deal with this.
>> + * TBD: It might be nicer to handle this here since we don't want a
>> + * discontiguous list to result in the caller.
>
> Yes, I think handling disabled is a requirement; Tegra has quite a few
> instances of each HW module, and in many cases, not all of them are used
> by a given board design, so they're marked disabled.
>
> I don't think this has any impact on handling discontiguous device IDs;
> I think we need that anyway.

Yes ok. In that case I will make the code check for status =
"disabled" at the same time. It is convenient.

>
> The itself array could always be contiguous if each entry were a pair
> (id, node) instead of the ID being implied by the array index.

Slightly easier to do it this way I think. Not completely sure yet.

>
>> + *
>> + * Note: the algorithm used is O(maxcount).
>> + *
>> + * @param blob ? ? ? ? ? ? ? FDT blob to use
>> + * @param name ? ? ? ? ? ? ? Root name of alias to search for
>> + * @param id ? ? ? ? Compatible ID to look for
>
> That's a little restrictive. Many drivers will handle multiple
> compatible values, e.g. N manufactures each making identical chips but
> giving each its own marketing name. These need different compatible
> flags in case some bug WAR needs to differentiate between them. Equally,
> Tegra30's say I2C controllers will be compatible with both
> nvidia,tegra30-i2c and nvidia,tegra20-i2c. While missing out the Tegra20
> compatible value would probably technically be a bug in the device tree,
> it does seem reasonable to expect the driver to still match on the
> Tegra30 compatible value.

I think you are asking then for a list of IDs to match on. Is that
right? How about I rename this function to
fdtdec_find_aliases_for_id() and we then can create a
fdtdec_find_aliases() function later when needed for T30? That way
callers don't need to allocate and pass an array of IDs yet?

>
>> + * @param node ? ? ? ? ? ? ? Place to put list of found nodes
>> + * @param maxcount ? Maximum number of nodes to find
>
> It'd be nice not to have maxcount; it seems slightly restrictive for a
> helper function. I suppose that most drivers can supply a reasonable
> value for this since there's a certain max number of devices possible
> given extant HW designs, but when you start talking about e.g. a driver
> for an I2C bus multiplexer, where there's one instance per chip on a
> board, the number begins to get a bit arbitrary.

Do you mean that you want the function to allocate the memory for an
array and return it? I would rather avoid that sort of overhead in
U-Boot if I can. Again if we find that devices might need an arbitrary
number of nodes we can support it later.

>
>> + * @return number of nodes found on success, FTD_ERR_... on error
>> + */
>> +int fdtdec_find_aliases(const void *blob, const char *name,
>> + ? ? ? ? ? ? ? ? ? ? enum fdt_compat_id id, int *node_list, int maxcount);
>
> Overall, I was expecting something more like:
>
> Enumeration:
> * Enumerate device tree solely based on compatible values
>
> After enumeration is complete:
> * Walk aliases node, and for each entry assign that ID value to the
> appropriate device.

OK I will see if I can change the algorithm to do this. Now that we
allow holes it will be more efficient anyway.

> * Walk all devices, and assign a free ID to each device that doesn't
> already have one.

It already does this at the end of the function I think.

>
> Still, I suppose in practice this current code will probably achieve
> equivalent results, and it doesn't have any impact of the device tree
> syntax/content (unless people change the way they write .dts files to
> WAR the issues above instead of enhancing the code), so can easily be
> enhanced to address the issues above as needed. So, I guess it'll do
> (with the exception of needing to handled disabled devices).

Will wait for your response on the above and then send a v2 patch.

Regards,

>
> --
> nvpublic

[U-Boot] [RFC PATCH 1/2] fdt: Add fdtdec_find_aliases() to deal with alias nodes

[Stephen Warren]

On 01/10/2012 02:22 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Tue, Jan 10, 2012 at 12:27 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> On 12/26/2011 03:31 PM, Simon Glass wrote:
>>> 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).
>> ...
>>> +/**
>>> + * 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.
>>> + *
>>> + * The array returned will not have any gaps.
> 
> Thanks for the detailed comments - much appreciated.
> 
>>
>> You can't make that guarantee without incorrectly parsing the device
>> tree; I don't believe there's any restriction on the IDs in the aliases
>> being contiguous. Maybe in practice this restriction will be fine, but
>> it doesn't seem like a great idea.
> 
> Well actually I was thinking from a U-Boot POV since if someone uses a
> device that doesn't exist U-Boot may just crash or hang. So having
> such a hole would normally be a bug. But since there is no restriction
> in the fdt format, and since I suppose we have to assume the user
> knows what he is doing, I will remove this restriction.

Great!

>>> + * If there is a gap in the aliases, then this function will only return up
>>> + * to the number of nodes it found until the gap. It will also print a warning
>>> + * in this case. As an example, say you define aliases for usb2 and usb3, and
>>> + * have 3 nodes. Then in this case the node without an alias will become usb0
>>> + * and the aliases will be use for usb2 and usb3. But since there is no
>>> + * usb1, this function will only list one node (usb0), and will print a
>>> + * warning.
>>> + *
>>> + * This function does not check node properties - so it is possible that the
>>> + * node is marked disabled (status = "disabled"). The caller is expected to
>>> + * deal with this.
>>> + * TBD: It might be nicer to handle this here since we don't want a
>>> + * discontiguous list to result in the caller.
>>
>> Yes, I think handling disabled is a requirement; Tegra has quite a few
>> instances of each HW module, and in many cases, not all of them are used
>> by a given board design, so they're marked disabled.
>>
>> I don't think this has any impact on handling discontiguous device IDs;
>> I think we need that anyway.
> 
> Yes ok. In that case I will make the code check for status =
> "disabled" at the same time. It is convenient.

Thanks.

>> The itself array could always be contiguous if each entry were a pair
>> (id, node) instead of the ID being implied by the array index.
> 
> Slightly easier to do it this way I think. Not completely sure yet.
> 
>>
>>> + *
>>> + * Note: the algorithm used is O(maxcount).
>>> + *
>>> + * @param blob               FDT blob to use
>>> + * @param name               Root name of alias to search for
>>> + * @param id         Compatible ID to look for
>>
>> That's a little restrictive. Many drivers will handle multiple
>> compatible values, e.g. N manufactures each making identical chips but
>> giving each its own marketing name. These need different compatible
>> flags in case some bug WAR needs to differentiate between them. Equally,
>> Tegra30's say I2C controllers will be compatible with both
>> nvidia,tegra30-i2c and nvidia,tegra20-i2c. While missing out the Tegra20
>> compatible value would probably technically be a bug in the device tree,
>> it does seem reasonable to expect the driver to still match on the
>> Tegra30 compatible value.
> 
> I think you are asking then for a list of IDs to match on. Is that
> right? How about I rename this function to
> fdtdec_find_aliases_for_id() and we then can create a
> fdtdec_find_aliases() function later when needed for T30? That way
> callers don't need to allocate and pass an array of IDs yet?

OK, that'll work.

>>> + * @param node               Place to put list of found nodes
>>> + * @param maxcount   Maximum number of nodes to find
>>
>> It'd be nice not to have maxcount; it seems slightly restrictive for a
>> helper function. I suppose that most drivers can supply a reasonable
>> value for this since there's a certain max number of devices possible
>> given extant HW designs, but when you start talking about e.g. a driver
>> for an I2C bus multiplexer, where there's one instance per chip on a
>> board, the number begins to get a bit arbitrary.
> 
> Do you mean that you want the function to allocate the memory for an
> array and return it? I would rather avoid that sort of overhead in
> U-Boot if I can. Again if we find that devices might need an arbitrary
> number of nodes we can support it later.

Yes, that's what I meant. I guess as you say we can add it later; the
failure mode is pretty easy to diagnose if we ever hit this case.

-- 
nvpublic

[U-Boot] [RFC PATCH 1/2] fdt: Add fdtdec_find_aliases() to deal with alias nodes

[Simon Glass]

Hi Stephen,

On Tue, Jan 10, 2012 at 1:41 PM, Stephen Warren <swarren@nvidia.com> wrote:
> On 01/10/2012 02:22 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Tue, Jan 10, 2012 at 12:27 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>> On 12/26/2011 03:31 PM, Simon Glass wrote:
>>>> 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).
>>> ...
>>>> +/**
>>>> + * 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.
>>>> + *
>>>> + * The array returned will not have any gaps.
>>
>> Thanks for the detailed comments - much appreciated.
>>
>>>
>>> You can't make that guarantee without incorrectly parsing the device
>>> tree; I don't believe there's any restriction on the IDs in the aliases
>>> being contiguous. Maybe in practice this restriction will be fine, but
>>> it doesn't seem like a great idea.
>>
>> Well actually I was thinking from a U-Boot POV since if someone uses a
>> device that doesn't exist U-Boot may just crash or hang. So having
>> such a hole would normally be a bug. But since there is no restriction
>> in the fdt format, and since I suppose we have to assume the user
>> knows what he is doing, I will remove this restriction.
>
> Great!
>
>>>> + * If there is a gap in the aliases, then this function will only return up
>>>> + * to the number of nodes it found until the gap. It will also print a warning
>>>> + * in this case. As an example, say you define aliases for usb2 and usb3, and
>>>> + * have 3 nodes. Then in this case the node without an alias will become usb0
>>>> + * and the aliases will be use for usb2 and usb3. But since there is no
>>>> + * usb1, this function will only list one node (usb0), and will print a
>>>> + * warning.
>>>> + *
>>>> + * This function does not check node properties - so it is possible that the
>>>> + * node is marked disabled (status = "disabled"). The caller is expected to
>>>> + * deal with this.
>>>> + * TBD: It might be nicer to handle this here since we don't want a
>>>> + * discontiguous list to result in the caller.
>>>
>>> Yes, I think handling disabled is a requirement; Tegra has quite a few
>>> instances of each HW module, and in many cases, not all of them are used
>>> by a given board design, so they're marked disabled.
>>>
>>> I don't think this has any impact on handling discontiguous device IDs;
>>> I think we need that anyway.
>>
>> Yes ok. In that case I will make the code check for status =
>> "disabled" at the same time. It is convenient.
>
> Thanks.
>
>>> The itself array could always be contiguous if each entry were a pair
>>> (id, node) instead of the ID being implied by the array index.
>>
>> Slightly easier to do it this way I think. Not completely sure yet.
>>
>>>
>>>> + *
>>>> + * Note: the algorithm used is O(maxcount).
>>>> + *
>>>> + * @param blob ? ? ? ? ? ? ? FDT blob to use
>>>> + * @param name ? ? ? ? ? ? ? Root name of alias to search for
>>>> + * @param id ? ? ? ? Compatible ID to look for
>>>
>>> That's a little restrictive. Many drivers will handle multiple
>>> compatible values, e.g. N manufactures each making identical chips but
>>> giving each its own marketing name. These need different compatible
>>> flags in case some bug WAR needs to differentiate between them. Equally,
>>> Tegra30's say I2C controllers will be compatible with both
>>> nvidia,tegra30-i2c and nvidia,tegra20-i2c. While missing out the Tegra20
>>> compatible value would probably technically be a bug in the device tree,
>>> it does seem reasonable to expect the driver to still match on the
>>> Tegra30 compatible value.
>>
>> I think you are asking then for a list of IDs to match on. Is that
>> right? How about I rename this function to
>> fdtdec_find_aliases_for_id() and we then can create a
>> fdtdec_find_aliases() function later when needed for T30? That way
>> callers don't need to allocate and pass an array of IDs yet?
>
> OK, that'll work.

Actually I just hit this with i2c. However I have solved this another
way - see what you think when you see the series, probably tomorrow.

>
>>>> + * @param node ? ? ? ? ? ? ? Place to put list of found nodes
>>>> + * @param maxcount ? Maximum number of nodes to find
>>>
>>> It'd be nice not to have maxcount; it seems slightly restrictive for a
>>> helper function. I suppose that most drivers can supply a reasonable
>>> value for this since there's a certain max number of devices possible
>>> given extant HW designs, but when you start talking about e.g. a driver
>>> for an I2C bus multiplexer, where there's one instance per chip on a
>>> board, the number begins to get a bit arbitrary.
>>
>> Do you mean that you want the function to allocate the memory for an
>> array and return it? I would rather avoid that sort of overhead in
>> U-Boot if I can. Again if we find that devices might need an arbitrary
>> number of nodes we can support it later.
>
> Yes, that's what I meant. I guess as you say we can add it later; the
> failure mode is pretty easy to diagnose if we ever hit this case.

Yes.

Regards
Simon

>
> --
> nvpublic

[U-Boot] [RFC PATCH 0/2] fdt: Deal correctly with alias nodes

[Marek Vasut]

> This series proposes a new way to deal with alias nodes and introduces a
> function to take care of it.
> 
> It includes an example of converting USB code over to use this new
> function.
> 
> Note: At present it does not deal automatically with disabled nodes,
> but perhaps it should? Or perhaps this is better as an option. Something
> to leave for later perhaps.
> 
> Note 2: The actual logic of this function is not properly tested yet.
> 
> 
> Simon Glass (2):
>   fdt: Add fdtdec_find_aliases() to deal with alias nodes
>   tegra: Use fdtdec_find_aliases() to find USB ports
> 
>  arch/arm/cpu/armv7/tegra2/usb.c |   17 ++++----
>  include/fdtdec.h                |   51 ++++++++++++++++++++++
>  lib/fdtdec.c                    |   89
> +++++++++++++++++++++++++++++++++++++++ 3 files changed, 148
> insertions(+), 9 deletions(-)

Hi,

what's the status of this patch/patchset?

Thanks
M

[U-Boot] [RFC PATCH 0/2] fdt: Deal correctly with alias nodes

[Tom Warren]

Marek/Simon,

> -----Original Message-----
> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
> Sent: Sunday, February 26, 2012 7:55 PM
> To: Marek Vasut; Tom Warren
> Subject: Re: [U-Boot] [RFC PATCH 0/2] fdt: Deal correctly with alias nodes
> 
> +Tom, tegra maintainer
> 
> Hi Marek,
> 
> On Sun, Feb 26, 2012 at 3:09 PM, Marek Vasut <marex@denx.de> wrote:
> >> This series proposes a new way to deal with alias nodes and
> >> introduces a function to take care of it.
> >>
> >> It includes an example of converting USB code over to use this new
> >> function.
> >>
> >> Note: At present it does not deal automatically with disabled nodes,
> >> but perhaps it should? Or perhaps this is better as an option.
> >> Something to leave for later perhaps.
> >>
> >> Note 2: The actual logic of this function is not properly tested yet.
> >>
> >>
> >> Simon Glass (2):
> >> ? fdt: Add fdtdec_find_aliases() to deal with alias nodes
> >> ? tegra: Use fdtdec_find_aliases() to find USB ports
> >>
> >> ?arch/arm/cpu/armv7/tegra2/usb.c | ? 17 ++++----
> >> ?include/fdtdec.h ? ? ? ? ? ? ? ?| ? 51 ++++++++++++++++++++++
> >> ?lib/fdtdec.c ? ? ? ? ? ? ? ? ? ?| ? 89
> >> +++++++++++++++++++++++++++++++++++++++ 3 files changed, 148
> >> insertions(+), 9 deletions(-)
> >
> > Hi,
> >
> > what's the status of this patch/patchset?
> 
> I believe it is sitting in the tegra tree ready for a pull request, but
> waiting on USB. Tom will know.
> 
Simon is correct. The 18 fdt/USB patches (v5 and 2 from v6) are sitting in u-boot-tegra/test waiting on a final ACK for one clock binding DT change from Simon based on Stephen's review. As Simon says in answer to a separate email, he's working on getting all these to jibe so that I can apply them cleanly to u-boot-tegra. As soon as they're ready, I'll apply & test and if all goes well, submit a pull request. Then we can move on the I2C, LCD, kbd, etc. for Tegra2.

Tom

-- 
nvpublic

> Regards,
> Simon
> 
> >
> > Thanks
> > M

[U-Boot] [RFC PATCH 0/2] fdt: Deal correctly with alias nodes

[Marek Vasut]

> Marek/Simon,
> 
> > -----Original Message-----
> > From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
> > Sent: Sunday, February 26, 2012 7:55 PM
> > To: Marek Vasut; Tom Warren
> > Subject: Re: [U-Boot] [RFC PATCH 0/2] fdt: Deal correctly with alias
> > nodes
> > 
> > +Tom, tegra maintainer
> > 
> > Hi Marek,
> > 
> > On Sun, Feb 26, 2012 at 3:09 PM, Marek Vasut <marex@denx.de> wrote:
> > >> This series proposes a new way to deal with alias nodes and
> > >> introduces a function to take care of it.
> > >> 
> > >> It includes an example of converting USB code over to use this new
> > >> function.
> > >> 
> > >> Note: At present it does not deal automatically with disabled nodes,
> > >> but perhaps it should? Or perhaps this is better as an option.
> > >> Something to leave for later perhaps.
> > >> 
> > >> Note 2: The actual logic of this function is not properly tested yet.
> > >> 
> > >> 
> > >> Simon Glass (2):
> > >>   fdt: Add fdtdec_find_aliases() to deal with alias nodes
> > >>   tegra: Use fdtdec_find_aliases() to find USB ports
> > >> 
> > >>  arch/arm/cpu/armv7/tegra2/usb.c |   17 ++++----
> > >>  include/fdtdec.h                |   51 ++++++++++++++++++++++
> > >>  lib/fdtdec.c                    |   89
> > >> +++++++++++++++++++++++++++++++++++++++ 3 files changed, 148
> > >> insertions(+), 9 deletions(-)
> > > 
> > > Hi,
> > > 
> > > what's the status of this patch/patchset?
> > 
> > I believe it is sitting in the tegra tree ready for a pull request, but
> > waiting on USB. Tom will know.
> 
> Simon is correct. The 18 fdt/USB patches (v5 and 2 from v6) are sitting in
> u-boot-tegra/test waiting on a final ACK for one clock binding DT change
> from Simon based on Stephen's review. As Simon says in answer to a
> separate email, he's working on getting all these to jibe so that I can
> apply them cleanly to u-boot-tegra. As soon as they're ready, I'll apply &
> test and if all goes well, submit a pull request. Then we can move on the
> I2C, LCD, kbd, etc. for Tegra2.
> 
That's good then, I won't care for these and let them go through the tegra tree. 
Thank you for your good work.

M