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

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