[Skiboot] [PATCH v2 4/6] hw/npu2, platform: Add NPU2 platform device detection callback
Andrew Donnellan
andrew.donnellan at au1.ibm.com
Thu Aug 30 14:39:54 AEST 2018
On 30/08/18 02:46, Frederic Barrat wrote:
>> + if (platform.ocapi) {
>> + /* Find I2C port for handling device presence/reset */
>> + snprintf(port_name, sizeof(port_name), "p8_%08x_e%dp%d",
>> + gcid, platform.ocapi->i2c_engine,
>> + platform.ocapi->i2c_port);
>> + prlog(PR_DEBUG, "NPU: Looking for I2C port %s\n", port_name);
>> +
>> + dt_for_each_compatible(dt_root, np, "ibm,power9-i2c-port") {
>> + if (streq(port_name, dt_prop_get(np, "ibm,port-name"))) {
>> + npu->i2c_port_id_ocapi = dt_prop_get_u32(np,
>> "ibm,opal-id");
>> + break;
>> + }
>> + }
>> +
>> + if (!npu->i2c_port_id_ocapi) {
>> + prlog(PR_ERR, "NPU: Couldn't find I2C port %s\n",
>> + port_name);
>> + }
>
> If we cannot find the i2c port, the error is ignored. Later on, when
> doing presence detect, we'll use a 0 port number and I wouldn't expect
> anything good out of it.
> I think it should be a hard error. If there's no I2C for reset, we are
> doomed.
Good catch.
>
>
>> + }
>> +
>> npu->devices = npumem + sizeof(struct npu2);
>>
>> dt_for_each_compatible(dn, np, "ibm,npu-link") {
>> @@ -145,7 +204,8 @@ static struct npu2 *setup_npu(struct dt_node *dn)
>> dev->link_index = dt_prop_get_u32(np, "ibm,npu-link-index");
>> /* May be overridden by platform presence detection */
>> dev->brick_index = dev->link_index;
>> - dev->type = npu2_dt_link_dev_type(np);
>> + /* Will be overridden by presence detection */
>> + dev->type = NPU2_DEV_TYPE_UNKNOWN;
>> dev->npu = npu;
>> dev->dt_node = np;
>> dev->pl_xscom_base = dt_prop_get_u64(np, "ibm,npu-phy");
>> @@ -176,13 +236,22 @@ static void setup_devices(struct npu2 *npu)
>> switch (dev->type) {
>> case NPU2_DEV_TYPE_NVLINK:
>> nvlink_detected = true;
>> + dt_add_property_strings(dev->dt_node,
>> + "ibm,npu-link-type",
>> + "nvlink");
>> break;
>> case NPU2_DEV_TYPE_OPENCAPI:
>> ocapi_detected = true;
>> + dt_add_property_strings(dev->dt_node,
>> + "ibm,npu-link-type",
>> + "opencapi");
>> break;
>> default:
>> prlog(PR_INFO, "NPU: Link %d device not present\n",
>> npu->devices[i].link_index);
>> + dt_add_property_strings(dev->dt_node,
>> + "ibm,npu-link-type",
>> + "unknown");
>> }
>> }
>>
>> @@ -220,8 +289,14 @@ void probe_npu2(void)
>> prlog(PR_WARNING, "NPU2: Using ZCAL impedance override =
>> %d\n", nv_zcal_nominal);
>> }
>>
>> + if (!platform.npu2_device_detect) {
>> + prlog(PR_INFO, "NPU: Platform does not support NPU\n");
>> + return;
>> + }
>> +
>
>
> This series doesn't define npu2_device_detect callback for ZZ
> (hostboot). Is that intentional, since some more work is needed anyway?
Ah, I should add that. I'd neglected ZZ because of the continuing issues
with HDAT...
>
>
>
>> dt_for_each_compatible(dt_root, np, "ibm,power9-npu") {
>> npu = setup_npu(np);
>> + platform.npu2_device_detect(npu);
>> setup_devices(npu);
>> }
>> }
>> diff --git a/hw/npu2-opencapi.c b/hw/npu2-opencapi.c
>> index ead8ec011341..fa28633bb150 100644
>> --- a/hw/npu2-opencapi.c
>> +++ b/hw/npu2-opencapi.c
>> @@ -52,14 +52,6 @@
>> #include <i2c.h>
>> #include <nvram.h>
>>
>> -#define OCAPIDBG(dev, fmt, a...) prlog(PR_DEBUG, "OCAPI[%d:%d]: "
>> fmt, \
>> - dev->npu->chip_id, dev->brick_index, ## a)
>> -#define OCAPIINF(dev, fmt, a...) prlog(PR_INFO, "OCAPI[%d:%d]: "
>> fmt, \
>> - dev->npu->chip_id, dev->brick_index, ## a)
>> -#define OCAPIERR(dev, fmt, a...) prlog(PR_ERR, "OCAPI[%d:%d]: "
>> fmt, \
>> - dev->npu->chip_id, dev->brick_index, ## a)
>> -
>> -
>> #define NPU_IRQ_LEVELS 35
>> #define NPU_IRQ_LEVELS_XSL 23
>> #define MAX_PE_HANDLE ((1 << 15) - 1)
>> @@ -842,7 +834,7 @@ static void assert_reset(struct npu2_dev *dev)
>> * register and a pin is in output mode if its value is 0
>> */
>> data = ~pin;
>> - rc = i2c_request_send(dev->i2c_port_id_ocapi,
>> + rc = i2c_request_send(dev->npu->i2c_port_id_ocapi,
>> platform.ocapi->i2c_reset_addr, SMBUS_WRITE,
>> 0x3, 1,
>> &data, sizeof(data), 120);
>> @@ -851,7 +843,7 @@ static void assert_reset(struct npu2_dev *dev)
>>
>> /* register 1 controls the signal, reset is active low */
>> data = ~pin;
>> - rc = i2c_request_send(dev->i2c_port_id_ocapi,
>> + rc = i2c_request_send(dev->npu->i2c_port_id_ocapi,
>> platform.ocapi->i2c_reset_addr, SMBUS_WRITE,
>> 0x1, 1,
>> &data, sizeof(data), 120);
>> @@ -874,7 +866,7 @@ static void deassert_reset(struct npu2_dev *dev)
>> int rc;
>>
>> data = 0xFF;
>> - rc = i2c_request_send(dev->i2c_port_id_ocapi,
>> + rc = i2c_request_send(dev->npu->i2c_port_id_ocapi,
>> platform.ocapi->i2c_reset_addr, SMBUS_WRITE,
>> 0x1, 1,
>> &data, sizeof(data), 120);
>> @@ -888,43 +880,6 @@ static void deassert_reset(struct npu2_dev *dev)
>> }
>> }
>>
>> -static bool i2c_presence_detect(struct npu2_dev *dev)
>> -{
>> - uint8_t state, data;
>> - int rc;
>> -
>> - /*
>> - * Opencapi presence detection is done through i2c
>> - *
>> - * Lagrange platforms (ZZ, Zaius) use the same default mechanism.
>> - * Witherspoon will need a specific implementation, TBD.
>> - */
>> - rc = i2c_request_send(dev->i2c_port_id_ocapi,
>> - platform.ocapi->i2c_presence_addr,
>> - SMBUS_READ, 0, 1,
>> - &state, 1, 120);
>> - if (rc) {
>> - OCAPIERR(dev, "error detecting link presence: %d\n", rc);
>> - return true; /* assume link exists */
>> - }
>> -
>> - OCAPIDBG(dev, "I2C presence detect: 0x%x\n", state);
>> -
>> - switch (dev->brick_index) { // TODO(ajd): Link or brick index?
>> - case 2:
>> - data = platform.ocapi->i2c_presence_odl0;
>> - break;
>> - case 3:
>> - data = platform.ocapi->i2c_presence_odl1;
>> - break;
>> - default:
>> - OCAPIERR(dev, "presence detection on invalid link\n");
>> - return true;
>> - }
>> - /* Presence detect bits are active low */
>> - return !(state & data);
>> -}
>> -
>> static void reset_odl(uint32_t gcid, struct npu2_dev *dev)
>> {
>> uint64_t reg, config_xscom;
>> @@ -1015,17 +970,10 @@ static void start_training(uint32_t gcid,
>> struct npu2_dev *dev)
>> xscom_write(gcid, config_xscom, reg);
>> }
>>
>> -static int64_t npu2_opencapi_get_presence_state(struct pci_slot *slot,
>> +static int64_t npu2_opencapi_get_presence_state(struct pci_slot
>> __unused *slot,
>> uint8_t *val)
>> {
>> - bool present;
>> - struct npu2_dev *dev = phb_to_npu2_dev_ocapi(slot->phb);
>> -
>> - if (platform.ocapi->force_presence)
>> - present = true;
>> - else
>> - present = i2c_presence_detect(dev);
>> - *val = present;
>> + *val = true;
>> return OPAL_SUCCESS;
>> }
>
>
> Long pause here... Are we unconditionally returning true because if
> there's no card, the device will have been typed as
> NPU2_DEV_TYPE_UNKNOWN earlier, and therefore no slot created for it? If
> so, it's probably worth a comment.
That's correct - you can't get the presence state of a device we never
created.
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan at au1.ibm.com IBM Australia Limited
More information about the Skiboot
mailing list