Hi Stephen, On Tue, Jan 10, 2012 at 12:27 PM, Stephen Warren <swar...@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@c5008000"; >> + * usb1 = "/ehci@c5000000"; >> + * }; >> + * >> + * Pass "usb" as the name to this function and will return a list of two >> + * nodes offsets: /ehci@c5008000 and ehci@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 mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot