Attention is currently required from: Jonathan Zhang, Rocky Phagura, Paul Menzel, Shuming Chu (Shuming).
Angel Pons 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: Code-Review+1
(7 comments)
File src/drivers/ipmi/ocp/Kconfig:
https://review.coreboot.org/c/coreboot/+/68758/comment/7e4e559b_ab542b9c PS3, Line 8: bool `depends on IPMI_OCP` maybe?
https://review.coreboot.org/c/coreboot/+/68758/comment/304d423a_7cd0aeb0 PS3, Line 14: hex `depends on IPMI_BMC_SEL` maybe?
File src/drivers/ipmi/ocp/ipmi_ocp.h:
https://review.coreboot.org/c/coreboot/+/68758/comment/8fdde1f0_d9ae152c 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 can be reused?
File src/drivers/ipmi/ocp/ipmi_sel.c:
https://review.coreboot.org/c/coreboot/+/68758/comment/0f64c7a2_8ff67570 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.
https://review.coreboot.org/c/coreboot/+/68758/comment/dba7fdbc_f164123b 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:
``` struct ipmi_sel_iio_err ubslp = { .record_id = SEL_RECORD_ID, .record_type = CONFIG_RAS_SEL_VENDOR_ID, .general_info = SEL_PCIE_IIO_ERR, .loc = 0, /* Only socket 0 support for now */ .iio_stack_num = iio_stack_num, .iio_err_id = err_id, }; ```
This will initialize the specified fields, and set the rest to 0.
https://review.coreboot.org/c/coreboot/+/68758/comment/97eb3da2_10bafd72 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
https://review.coreboot.org/c/coreboot/+/68758/comment/d6708616_dd2f4dae 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