Attention is currently required from: Rocky Phagura, Paul Menzel, Angel Pons, Tim Chu.
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68758 )
Change subject: drivers/ipmi/ocp: add PCIe SEL support ......................................................................
Patch Set 3:
(7 comments)
File src/drivers/ipmi/ocp/Kconfig:
https://review.coreboot.org/c/coreboot/+/68758/comment/f14c4535_eeece908 PS3, Line 8: bool
`depends on IPMI_OCP` maybe?
Ack
https://review.coreboot.org/c/coreboot/+/68758/comment/bb35cc58_6dc23344 PS3, Line 14: hex
`depends on IPMI_BMC_SEL` maybe?
Ack
File src/drivers/ipmi/ocp/ipmi_ocp.h:
https://review.coreboot.org/c/coreboot/+/68758/comment/29ce01d3_23dd8ef0 PS3, Line 111: /* PCIE CE */ : #define RECEIVER_ERROR 0x00 : #define BAD_TLP 0x01 : #define BAD_DLLP 0x02 : #define REPLAY_TIME_OUT 0x03 : #define REPLAY_NUMBER_ROLLOVER 0x04 : #define ADVISORY_NONFATAL_ERROR_STATUS 0x05 : #define CORRECTED_INTERNAL_ERROR_STATUS 0x06 : #define HEADER_LOG_OVERFLOW_STATUS 0x07 : : /* PCIE UCE */ : #define PCI_EXPRESS_DATA_LINK_PROTOCOL_ERROR 0x20 : #define SURPRISE_DOWN_ERROR 0x21 : #define RECEIVED_PCI_EXPRESS_POISONED_TLP 0x22 : #define PCI_EXPRESS_FLOW_CONTROL_PROTOCOL_ERROR 0x23 : #define COMPLETION_TIMEOUT_ON_NP_TRANSACTIONS_OUTSTANDING_ON_PCI_EXPRESS_DMI 0x24 : #define RECEIVED_A_REQUEST_FROM_A_DOWNSTREAM_COMPONENT_THAT_IS_TO_BE_COMPLETER_ABORTED 0x25 : #define RECEIVED_PCI_EXPRESS_UNEXPECTED_COMPLETION 0x26 : #define PCI_EXPRESS_RECEIVER_OVERFLOW 0x27 : #define PCI_EXPRESS_MALFORMED_TLP 0x28 : #define ECRC_ERROR_STATUS 0x29 : #define RECEIVED_A_REQUEST_FROM_A_DOWNSTREAM_COMPONENT_THAT_IS_UNSUPPORTED 0x2A : #define ACS_VIOLATION 0x2B : #define UNCORRECTABLE_INTERNAL_ERROR_STATUS 0x2C : #define MC_BLOCKED_TLP 0x2D : #define ATOMICOP_EGRESS_BLOCKED_STATUS 0x2E : #define TLP_PREFIX_BLOCKED_ERROR_STATUS 0x2F : #define POISONED_TLP_EGRESS_BLOCKED 0x30
These macros seem to be part of the PCIe spec, should we move them to a separate header so that they […]
I do not think they are part of the PCIe spec. They are part of the SEL record spec, albeit it is in a custom spec instead of public spec.
@Tim, could you confirm above?
File src/drivers/ipmi/ocp/ipmi_sel.c:
https://review.coreboot.org/c/coreboot/+/68758/comment/132130ed_cfdfd776 PS3, Line 20: if (CONFIG_BMC_KCS_BASE != 0) {
You can probably use a `_Static_assert()` instead, as this condition is a compile-time constant.
Ack
https://review.coreboot.org/c/coreboot/+/68758/comment/470800ac_016bdc23 PS3, Line 31: struct ipmi_sel_iio_err ubslp; : memset(&ubslp, 0, sizeof(struct ipmi_sel_iio_err)); : ubslp.record_id = SEL_RECORD_ID; : ubslp.record_type = CONFIG_RAS_SEL_VENDOR_ID; : ubslp.general_info = SEL_PCIE_IIO_ERR; : /* ubslp.loc = 0; Only socket 0 support for now */ : ubslp.iio_stack_num = iio_stack_num; : ubslp.iio_err_id = err_id;
You don't need to `memset()` the struct: […]
Ack
https://review.coreboot.org/c/coreboot/+/68758/comment/6f81333b_641958fb PS3, Line 47: struct ipmi_sel_pcie_dev_err ubslp; : memset(&ubslp, 0, sizeof(struct ipmi_sel_pcie_dev_err)); : struct pci_dev_fn *inbdf = (struct pci_dev_fn *)&bdf; : ubslp.record_id = SEL_RECORD_ID; : ubslp.record_type = CONFIG_RAS_SEL_VENDOR_ID; : ubslp.general_info = SEL_PCIE_DEV_ERR; : ubslp.timestamp = 0; /* BMC will apply timestamp */ : ubslp.aux_loc = 0; : ubslp.bdf.bus = inbdf->bus; : ubslp.bdf.dev = inbdf->dev; : ubslp.bdf.func = inbdf->func >> 12; : ubslp.primary_err_count = prmry_cnt; : ubslp.secondary_id = sec_id; : ubslp.primary_id = prmry_id;
Same as above
Ack
https://review.coreboot.org/c/coreboot/+/68758/comment/de40fd44_6380705c PS3, Line 70: struct ipmi_sel_pcie_dev_fail ubslp; : memset(&ubslp, 0, sizeof(struct ipmi_sel_pcie_dev_fail)); : ubslp.record_id = SEL_RECORD_ID; : ubslp.record_type = CONFIG_RAS_SEL_VENDOR_ID; : ubslp.general_info = SEL_PCIE_DEV_FAIL_ID; : ubslp.timestamp = 0; /* BMC will apply timestamp */ : ubslp.type = code; : ubslp.failure_details1 = sts_reg; : ubslp.failure_details2 = src_id;
Same as above
Ack