[Skiboot] [PATCH v9 05/11] dt: Add phandle fixup helpers

[Stewart Smith]

Madhavan Srinivasan <maddy at linux.vnet.ibm.com> writes:

> On Monday 01 May 2017 12:28 PM, Stewart Smith wrote:
>> Anju T Sudhakar <anju at linux.vnet.ibm.com> writes:
>>> Patch to add a phandle fixup function for a new
>> You don't need to say "patch to add". You can drop the whole first
>> sentence.
>>
>>> incoming subtree. When attaching a "subtree" to
>>> main dt, subtree could have "property" referring
>>> different nodes or multiple nodes via phandle as
>>> "labels". For this reason, new subtree will have
>>> "phandle" already assigned to some of it's nodes.
>>> If this new subtree is attached to main dt without
>>> phandle fixup, could lead to duplicate phandle errors
>>>
>>> dt_fixup_populate_hash() does two main things. It first
>>> uses a hash primitive to create a hashtable using phandle
>>> as key. It then loops through the list of properties specified
>>> as an input to this function (which needs to be fixed up),
>>> and calls the fixup "handler".
>>>
>>> Signed-off-by: Anju T Sudhakar <anju at linux.vnet.ibm.com>
>>> Signed-off-by: Madhavan Srinivasan <maddy at linux.vnet.ibm.com>
>>> ---
>>>   core/device.c    | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>>>   include/device.h | 19 +++++++++++++++++++
>>>   2 files changed, 66 insertions(+)
>>>
>>> diff --git a/core/device.c b/core/device.c
>>> index 170e0ee..e933c27 100644
>>> --- a/core/device.c
>>> +++ b/core/device.c
>>> @@ -17,6 +17,7 @@
>>>   #include <device.h>
>>>   #include <stdlib.h>
>>>   #include <skiboot.h>
>>> +#include <hash.h>
>>>   #include <libfdt/libfdt.h>
>>>   #include <libfdt/libfdt_internal.h>
>>>   #include <ccan/str/str.h>
>>> @@ -849,6 +850,52 @@ void dt_expand(const void *fdt)
>>>   		abort();
>>>   }
>>>
>>> +/* dt_fixup_populate_hash: populate the hash table, and invoke the fixup-handler
>>> + *			   to fixup the phandle value of the nodes.
>>> + */
>>> +int dt_fixup_populate_hash(struct dt_node *dev, hash_table_p hash, phandle_fixup_p list_prop )
>>> +{
>>> +	struct dt_node *node;
>>> +	hash_entry_p entry;
>>> +	fixup_prop_list *list_pp;
>>> +	u32 phandle;
>>> +	phandle_fixup_n *val;
>>> +
>>> +	/* Initialize the hashtable */
>>> +	hash_init(hash);
>>> +
>>> +	/* For each node of 'dev', insert the node-phandle pair into hashtable */
>>> +	dt_for_each_node(dev, node) {
>>> +		if (node->phandle > 0) {
>>> +			val = (phandle_fixup_n *)malloc(sizeof(phandle_fixup_n));
>>> +			if(!val) {
>>> +				prerror("Failed to allocate memory");
>>> +				return -1;
>>> +			}
>>> +			val->node = node;
>>> +			val->fixed = false;
>>> +			entry = hash_insert_entry(hash, node->phandle, val);
>>> +			if(!entry) {
>>> +				prerror("Failed to make hash table entry");
>>> +				return -1;
>>> +			}
>>> +		}
>>> +	}
>>> +	list_for_each(&list_prop->list, list_pp, list) {
>>> +		phandle = dt_prop_get_u32(list_pp->node, list_pp->prop->name);
>>> +		/* look for the entry, with phandle as the key to hashtable */
>>> +		entry = hash_lookup_entry(hash, phandle);
>>> +		if (entry) {
>>> +			/* If find a valid entry in the hashtable, do the fixup */
>>> +			list_prop->fixup(entry, list_pp->prop);
>>> +		} else {
>>> +			prerror("%s: Missing hash entry phandle = %x\n", __FUNCTION__, phandle);
>>> +			return -1;
>>> +		}
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>> This looks overly complex for what needs to be done.
>>
>> If we're importing a subtree that uses phandles say 1-100, and our
>> existing last_phandle=42, then all we need to do is go through and add
>> 42 to all of the phandles in the subtree, and at the end, set
>> last_phandle=42+100.
>
> Basically this function does two things.
> 1)Fix the phandle
> 2)properties that refers to the phandle
>
> For example (from imc dtb), node here carries the events supported by 
> "mcs" unit.
> And this node is common and referred by all "mcs" units supported.
>
> +        nest-mcs-events {
> +                #address-cells = <0x1>;
> +                #size-cells = <0x1>;
> +                phandle = <0x10000242>;
> +
>
> Here is the "mcs" unit node which refers to the above event node via
> a property called "events". "events" property contains the phandle.
>
> +        mcs3 {
> +                compatible = "ibm,imc-counters-nest";
> +                events-prefix = "PM_MCS3_"; /* denotes event name to be prefixed to get complete event name supported by this device */
> +
> +                phandle = <0x10000241>;
> +                events = <0x10000242>; /* phandle of the events node supported by this device */
>
>
> So when we fix the incoming dtb for the phandles, we also need to
> look for any property that refers that phandle.
>
> Now my initial implementation was using llist from the
> current ccan lib in the skiboot, But benh suggested to
> have a simple hash primitive. Since this is a generic
> function, he suggested to have a list of properties from
> the caller along with a "handler" function which knows to fix.
>
> Hope this clarifies.

I don't see why any of that would require anything more complex than a
single pass through the subtree we're adding though.

We just need a function like this:

int dt_adjust_subtree_phandle(dt_node *subtree,
                              const char** (get_properties_to_fix*)(dt_node *n))
{
        phandle_import_start = ++last_phandle;

        foreach node and subnode {
                phandle += phandle_import_start;
                const char **properties_to_update;
                props_to_update = get_properties_to_fix(node);
                if (props_to_update) {
                   for (const char* p = props_to_update; *p != NULL; p++) {
                       set_value(prop, get_value(prop) + phandle_import_start);
                   }
                }
        }
}

With this for IMC:

const char** imc_properties_to_fix(dt_node *node)
{
        if (node_is_compatible("ibm,imc-counters-nest"))
           return { "events" , NULL };
        else
           return NULL;
}

?

-- 
Stewart Smith
OPAL Architect, IBM.