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