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

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