[Skiboot] [PATCH 3/6] hw/npu2: Common NPU2 init routine between NVLink and OpenCAPI

[Andrew Donnellan]

On 21/08/18 16:34, Alistair Popple wrote:
> Couple of minor comments below, but nothing major.
> 
> Reviewed-By: Alistair Popple <alistair at popple.id.au>

Thanks :)

>> +	dt_for_each_compatible(dt_root, np, "ibm,power9-npu") {
>> +	        npu = setup_npu(np);
>> +		if (npu) {
> 
> Not sure if you're just being defensive but setup_npu() can never return
> npu == NULL (it asserts instead) so the check is somewhat superfluous.

I think at some point it was able to return NULL and then I shuffled 
things around a bit...

>> diff --git a/include/npu2.h b/include/npu2.h
>> index 10742031ec0f..9fdc438bb4c1 100644
>> --- a/include/npu2.h
>> +++ b/include/npu2.h
>> @@ -147,6 +147,7 @@ struct npu2 {
>>   	uint32_t	chip_id;
>>   	uint64_t	xscom_base;
>>   	void		*regs;
>> +	void		*ocapi_global_mmio_base;
> 
> This is only one value so at this point it doesn't matter but if we start adding
> more values to struct npu2 that are exclusive to NPU2 xor OCAPI it would be nice
> to have these in different structs so it's obvious which ones are
> initialised/used in each case.

I've gone back and forth on this... in principle I agree but C doesn't 
make nested struct namespacing look particularly elegant and I keep 
screwing up . vs -> when I do that... but yes, if we need to add more 
fields then maybe it's worth doing that.


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