[Skiboot] [PATCH 4/6] hw/npu2, platform: Add NPU2 platform device detection callback

[Alistair Popple]

On Tuesday, 21 August 2018 5:14:03 PM AEST Andrew Donnellan wrote:
> On 21/08/18 16:48, Alistair Popple wrote:
> >> Move the existing I2C-based presence detection for OpenCAPI devices on
> >> Zaius/ZZ into common code, which we use by default for platforms which do
> >> not define a callback. Clean up the use of the ibm,npu-link-type property,
> >> which will now be exposed solely for debugging and not consumed internally.
> > 
> > <snip>
> > 
> >> +/*
> >> + * A default presence detection implementation for platforms like ZZ and Zaius
> >> + * that don't provide their own. Assumes all devices found will be OpenCAPI.
> >> + */
> >> +static void i2c_presence_detect(struct npu2 *npu)
> >> +{
> >> +	struct npu2_dev *dev;
> >> +	for (int i = 0; i < npu->total_devices; i++) {
> >> +		dev = &npu->devices[i];
> >> +		if (platform.ocapi->force_presence ||
> >> +		    _i2c_presence_detect(dev))
> >> +			dev->type = NPU2_DEV_TYPE_OPENCAPI;
> >> +		else
> >> +			dev->type = NPU2_DEV_TYPE_UNKNOWN;
> > 
> > What are the chances of i2c detection working on unknown platforms? We should
> > really only default to something if there is a reasonable expectation that it
> > will work correctly. If not we should default to setting everything to
> > NPU2_DEV_TYPE_UNKNOWN and have the ZZ and Zaius platforms set
> > i2c_presence_detect() as their npu2_device_detect callback.
> 
> If it's an unknown platform it won't have platform.ocapi defined.
> 
> ...aaaaand we don't null-check that in this path even though we 
> dereference it. Will fix.

:-)

> I see your point and maybe I'll export it in npu2.h so we can reference 
> it as a platform callback.

That would be my preference. Then you could can print an obvious error message
in the case of NPU2_DEV_TYPE_UNKNOWN rather than obscure i2c failure messages
from the presence detect.

- Alistair