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

Andrew Donnellan andrew.donnellan at au1.ibm.com
Tue Aug 21 17:01:36 AEST 2018

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

More information about the Skiboot mailing list