[Skiboot] [PATCH v2 07/15] hw/npu2: Rework npu2_add_interrupt_map() and skip non-NVLink devices

[Alexey Kardashevskiy]

On 11/01/2019 12:09, Andrew Donnellan wrote:
> Rework npu2_add_interrupt_map() so it only includes NVLink devices. Use the
> existing struct npu2_devs rather than accessing the device tree.
> 
> Use each device's brick index to determine its interrupt number, as
> interrupt levels depend on the ODL number and thus the brick index rather
> than link index.
> 
> Signed-off-by: Andrew Donnellan <andrew.donnellan at au1.ibm.com>
> 
> ---
> 
> This is difficult to test as I don't know how to inject faults in NVLink.
> 
> v1->v2:
> - add more info to commit message and comment about interrupt level
> number (Alexey)
> ---
>  hw/npu2.c | 41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/npu2.c b/hw/npu2.c
> index 106b32150994..78233579d3b7 100644
> --- a/hw/npu2.c
> +++ b/hw/npu2.c
> @@ -1638,11 +1638,11 @@ static void npu2_configure_devices(struct npu2 *p)
>  	}
>  }
>  
> -static void npu2_add_interrupt_map(struct npu2 *p,
> -				  struct dt_node *dn)
> +static void npu2_add_interrupt_map(struct npu2 *p)
>  {
> -	struct dt_node *npu2_dn, *link, *phb_dn;
> -	uint32_t npu2_phandle, index = 0, i;
> +	struct dt_node *phb_dn;
> +	struct npu2_dev *dev;
> +	int index, i = 0, nv_devices = 0;
>  	uint32_t icsp = get_ics_phandle();
>  	uint32_t *map;
>  	size_t map_size;
> @@ -1651,23 +1651,32 @@ static void npu2_add_interrupt_map(struct npu2 *p,
>  	assert(p->phb_nvlink.dt_node);
>  	phb_dn = p->phb_nvlink.dt_node;
>  
> -	npu2_phandle = dt_prop_get_u32(dn, "ibm,npcq");
> -	npu2_dn = dt_find_by_phandle(dt_root, npu2_phandle);
> -	assert(npu2_dn);
> -	map_size = 7 * sizeof(*map) * p->total_devices;
> +	for (index = 0; index < p->total_devices; index++) {
> +		if (p->devices[index].type == NPU2_DEV_TYPE_NVLINK)
> +			nv_devices++;
> +	}


if (!nv_devices)
	return;

?

> +
> +	map_size = 7 * sizeof(*map) * nv_devices;
>  	map = malloc(map_size);
> -	index = 0;
> -	dt_for_each_compatible(npu2_dn, link, "ibm,npu-link") {
> -		i = index * 7;
> -		map[i + 0] = (p->devices[index].bdfn << 8);
> +
> +	for (index = 0; index < p->total_devices; index++) {

index = 0, i = 0 ? I mean keep both initializations together.


> +		dev = &p->devices[index];
> +		if (dev->type != NPU2_DEV_TYPE_NVLINK)
> +			continue;
> +
> +		map[i + 0] = (dev->bdfn << 8);
>  		map[i + 1] = 0;
>  		map[i + 2] = 0;
> -
>  		map[i + 3] = 1; /* INT A */
>  		map[i + 4] = icsp; /* interrupt-parent */
> -		map[i + 5] = p->base_lsi + (index * 2) + 1; /* NDL No-Stall Event */
> +
> +		/*
> +		 * NDL No-Stall Event - the interrupt level is determined by


Oh. The "level" from the "interrupt level" is not really a level but a
number but the spec uses the word "level" anyway. Noiiiiice :(

The workbook also says:
Note 1: NDL 3 is connected to NTL 5. This NDL/NTL pair represents brick 5.
Note 2: NDL 5 is connected to NTL 3. This NDL/NTL pair represents brick 3.

It would help to mention in the commit log why we do not need swap
bricks 3 and 5 here.


> +		 * the brick number, per the NPU workbook
> +		 */
> +		map[i + 5] = p->base_lsi + (dev->brick_index * 2) + 1;
>  		map[i + 6] = 0; /* 0 = EDGE, 1 = LEVEL. */
> -		index++;
> +		i += 7;
>  	}
>  	dt_add_property(phb_dn, "interrupt-map", map, map_size);
>  	free(map);
> @@ -1836,7 +1845,7 @@ void npu2_nvlink_create_phb(struct npu2 *npu, struct dt_node *dn)
>  
>  	npu2_setup_irqs(npu);
>  	npu2_configure_devices(npu);
> -	npu2_add_interrupt_map(npu, dn);
> +	npu2_add_interrupt_map(npu);
>  	npu2_add_phb_properties(npu);
>  
>  	slot = npu2_slot_create(&npu->phb_nvlink);
> 

-- 
Alexey