[Skiboot] [PATCH v6 4/9] Catalog parser function to detect Nest units

[Madhavan Srinivasan]

On Tuesday 23 February 2016 08:56 AM, Sukadev Bhattiprolu wrote:
> Madhavan Srinivasan [maddy at linux.vnet.ibm.com] wrote:
>> Catalog have "group" structure to group events with similiar functions.
>> All the events within a group will be from same nest unit or domain.
>> A Nest unit can have multiple groups for its events in the catalog.
>> But, device tree will have only one node for a nest units and all the
> nit. s/units/unit/.
>
>> events supported by that nest units should end up under it.
> ditto
>
> Also, since the DT nodes are also under our control, can you rephrase
> this to:
>
> 	Create a single DT node for each nest unit and under that node,
> 	create one node for each event-group supported by that nest-unit.
>
> From what I understand, we are parsing the catalog to locate nest units,
> and their event groups and then creating device tree nodes for both of
> them.  Along the way, we have to tweak the nest unit names from the catalog
> to be suitable for their dt-node names.

Yes.
>
> If my understaing is correct, renaming some the variables/function names
> for consistency will help. Feel free to ignore the nits below.
>
>> Two data structures are added:
>> 	1)struct "lookup_grp_name_to_device_name"
> Since this table maps event group names to DT node names, how about
> renaming this to 'map_grp_name_to_dt_name'?
>
>> 	2)struct "nest_catalog_grps_device"
> and this to 'map_nest_unit_to_dt_node'?
>
> While shortening "group" to "grp", we could also shorten "device" to
> "dev" without ambiguity.

Yes. can change these.

>> Function detect_nest_units(), scans all the chip groups in the
>> catalog file and passes each chip group to update_catalog_to_devices().
>> update_catalog_to_devices() routine using lookup table identify the
>> group to a nest unit. Function then save the group address in
>> the "nest_catalog_grps_device" structure under the identified nest unit.
> Explanation of the functions could go into the function headers?

I have added comments in the code. But this is just to sum up.

>
>> Signed-off-by: Madhavan Srinivasan <maddy at linux.vnet.ibm.com>
>> ---
>>  hw/nest.c      | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/nest.h | 29 +++++++++++++++++++++
>>  2 files changed, 108 insertions(+)
>>
>> diff --git a/hw/nest.c b/hw/nest.c
>> index 4aa5fa0e5332..23a61d80f101 100644
>> --- a/hw/nest.c
>> +++ b/hw/nest.c
>> @@ -33,6 +33,24 @@
>>  struct nest_catalog_desc *catalog_desc;
>>  
>>  /*
>> + * Lookup table to map the catalog nest group to
>> + * hardware nest unit.
>> + */
>> +struct lookup_grp_name_to_device_name lookup_table[] = {
>> +	{"PowerBus", "powerbus", 8, POWERBUS},
>> +	{"Pumps_and_Retries", "powerbus", 17, POWERBUS},
>> +	{"MCS_Read", "mcs_read",8, MCS_READ},
>> +	{"MCS_Write", "mcs_write", 9, MCS_WRITE},
>> +	{"X-link", "xbus", 6, XBUS},
>> +	{"A-link", "abus", 6, ABUS},
> Nit. Align the columns to make it easier to read?
>
>> +};
>> +
>> +/*
>> + * Structure to hold catalog groups information for each nest unit
>> + */
>> +struct nest_catalog_grps_device dev_grp_map[NEST_DT_DEVICE_MAX];
>> +
>> +/*
>>   * Catalog Page-0 (each page is 4K bytes long) contains
>>   * information about event, group, formula structure offsets
>>   * in the catalog. Cache these offsets in struct nest_catalog_page_0
>> @@ -40,6 +58,60 @@ struct nest_catalog_desc *catalog_desc;
>>   */
>>  struct nest_catalog_page_0 *page0_desc;
>>  
>> +static void update_catalog_to_devices(struct nest_catalog_group_data *gptr)
> How about 'add_grp_to_nest_unit_dt()?'
>
> and s/gptr/grp/ or group?
>
>> +{
>> +	int i, rc, dev_id, cnt;
> Doesn't dev_id represent a nest_unit index? If so call it 'unit'?
>
>> +	int len;
>> +
>> +	for (i=0; i< NEST_DT_DEVICE_MAX; i++) {
> Nit. Spaces around operators = and <?
>
>> +		len = lookup_table[i].length;
>> +		if (len > gptr->group_name_len)
>> +			len = gptr->group_name_len;
>> +
>> +		rc = strncmp(gptr->remainder, lookup_table[i].catalog_grp_name,len);
> s/remainder/name/?
>
> space after comma.
>
>
>> +		if (!rc) {
>> +			dev_id = lookup_table[i].devices_id;
>> +			cnt = dev_grp_map[dev_id].nest_device_grps_cnt;
>> +			dev_grp_map[dev_id].grp_ptr_arr[cnt] = (u64) gptr;
>> +			dev_grp_map[dev_id].nest_device_grps_cnt++;
>> +
>> +			/*
>> +			 * Link the device name if not linked already.
>> +			 * This is needed when creating the device tree.
>> +			 */
>> +			 if (!dev_grp_map[dev_id].name)
>> +				dev_grp_map[dev_id].name = lookup_table[i].dt_device_name;
> This is the "nest unit name" correct?
>
> BTW, we seem to be discarding the event-group name during this
> processing right? Can we preserve the group name as another
> property of the group's dt-node?

Because just incase if we want to map to catalog?
Will think about it.

>
>> +		}
>> +	}
>> +}
>> +
>> +static int detect_nest_units(struct dt_node *ima)
>> +{
>> +	struct nest_catalog_group_data *group_ptr;
> s/_ptr//?
>
>> +	char *marker;
>> +	int rc = 0;
>> +
>> +	/*
>> +	 * Scan all available groups in the chip domain from
>> +	 * the CATALOG and update the nest_catalog_grps_device struct.
> s/struct/table/.
>
>> +	 * Reason for this 1) Catalog can have multiple groups for a
> s/Reason for this/Note that/
>> +	 * same nest unit. 2) All the events for a nest unit should end up
> Do we add events or event groups to the nest unit?

We add the all the events to the nest unit.

>
>> +	 * in the same dt node even though they are placed in different groups.
>> +	 * ex. Catalog could have groups like "X-link_data" and "X-link_idle",
>> +	 * but events from both these groups should end up in the device tree
>> +	 * node of "xbus".
>> +	 */
>> +	marker = CHIP_GROUP_ENTRY(catalog_desc);
>> +	group_ptr = (struct nest_catalog_group_data *)marker;
>> +	while(group_ptr->domain == DOMAIN_CHIP) {
> Space after 'while'
>> +		update_catalog_to_devices(group_ptr);
>> +		marker += group_ptr->length;
>> +		group_ptr = (struct nest_catalog_group_data *)marker;
>> +	};
>> +
>> +	return rc;
>> +}
>> +
>>  int preload_catalog_lid()
>>  {
>>  	struct proc_chip *chip = get_chip(this_cpu()->chip_id);
>> @@ -177,6 +249,13 @@ void nest_pmu_init(int loaded)
>>  		dt_add_property_cells(chip_dev, "#size-cells", 1);
>>  		dt_add_property_cells(chip_dev, "ranges", 0, hi32(addr),
>>  						lo32(addr), SLW_IMA_SIZE);
>> +
>> +		/*
>> +		 * Now parse catalog and add nest units and their events
> again, adding events or groups to DT?

Will changes it.
>
>
>> +		 * to the device tree.
>> +		 */
>> +		if (detect_nest_units(chip_dev))
>> +			goto fail;
>>  	}
>>  
>>  	return;
>> diff --git a/include/nest.h b/include/nest.h
>> index a27907fdf62a..b464e5b72c0e 100644
>> --- a/include/nest.h
>> +++ b/include/nest.h
>> @@ -204,6 +204,35 @@ struct nest_catalog_desc {
>>  /*Size of Nest Catalog LID (256KBytes) */
> Space after the '*'.
>
>>  #define NEST_CATALOG_SIZE             0x40000
>>  
>> +/* Max number of groups for a nest units in catalog */
> s/units/unit/
>
>> +#define MAX_GRPS_PER_DEVICE	5
>> +
>> +/*
>> + * If new nest units are added to catalog then append to the enum.
>> + */
>> +enum nest_dt_devices {
> How about "nest_unit_types"?

sure.

>
>> +	POWERBUS,
>> +	MCS_READ,
>> +	MCS_WRITE,
>> +	XBUS,
>> +	ABUS,
>> +	NEST_DT_DEVICE_MAX,
>> +};
>> +
>> +struct lookup_grp_name_to_device_name {
> Rename to 'map_nest_unit_to_dt_node'?
>
>> +	char catalog_grp_name[32];
> s/catalog_//
>
>> +	char dt_device_name[32];
> dt_node_name?
>
>> +	u32 length;
>> +	u32 devices_id;
> nest_unit?
>
>> +};
>> +
>> +struct nest_catalog_grps_device {
> As mentioned above, rename to 'map_grp_name_to_dt_name'?
>
>> +	char *name;
> nest_unit_name?
>
>> +	u64 grp_ptr_arr[MAX_GRPS_PER_DEVICE];
> s/grp_ptr_arr/groups/? or event_groups?
>
>> +	u32 nest_device_grps_cnt;
> grp_count?
>
>> +	struct dt_node *ptr;
> s/ptr/node/ or dt_node?
>
>> +};
>> +
>>  /*
>>   * Function prototypes
>>   */
>> --