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

[Madhavan Srinivasan]

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.

Maddy


>
> This way we avoid having to look anything up.
>
> The only way this could break is if the subtree is particularly insane,
> but since we control it, it should never be (and the code could check
> for that and abort out).
>
> Unless I'm missing something, this should be both correct and simple, right?
>