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