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

[Sukadev Bhattiprolu]

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.

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.

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

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

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

> +	 * 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?


> +		 * 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"?

> +	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
>   */
> --