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

Madhavan Srinivasan maddy at linux.vnet.ibm.com
Tue May 2 21:29:46 AEST 2017



On Tuesday 02 May 2017 09:54 AM, Stewart Smith wrote:
> 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);
>                     }
>                  }
>          }
> }

How about something like this.

+
+/*
+ * Function to fixup the phandle in the subtree.
+ */
+void dt_adjust_subtree_phandle(struct dt_node *dev, const char 
**prop_name )
+{
+       struct dt_node *node;
+       struct dt_property *prop;
+       u32 phandle, max_phandle, import_phandle = ++last_phandle;
+       const char **name;
+
+       dt_for_each_node(dev, node) {
+               node->phandle += import_phandle;
+
+               /*
+                * calculate max_pahndle(new_tree), needed to update
+                * last_phandle.
+                */
+               if (node->phandle >= max_phandle)
+                       max_phandle = node->phandle;
+
+               /* Add the import_phandle to each property in the prop 
array */
+               for (name = prop_name; *name != NULL && 
(__dt_find_property(node, *name)); name++) {
+                       prop = __dt_find_property(node, *name);
+                       phandle = dt_prop_get_u32(node, *name);
+                       phandle += import_phandle;
+                       memcpy((char *)&prop->prop, &phandle, prop->len);
+               }
+       }
+
+       update_last_phandle(max_phandle);
+}
+

Have also handled update of last_phande.

Maddy

> 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;
> }
>
> ?
>



More information about the Skiboot mailing list