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

[Andrew Donnellan]

On 17/1/19 5:30 pm, Alexey Kardashevskiy wrote:
>> -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;
> 
> ?

I don't think it causes any harm to have a zero-sized interrupt map but 
perhaps we should return there anyway...

> 
>> +
>> +	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.

ok

> 
> 
>> +		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 :(

Just following the spec :)

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

Hence the comment "the interrupt level is determined by the brick 
number", because the brick numbering, not the NDL numbering, is what's 
used to determine the order of the interrupt levels.

> 
> 
>> +		 * 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);
>>
> 

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan at au1.ibm.com  IBM Australia Limited