Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41272 )
Change subject: soc/amd/common/block: Add support for configuring eSPI connection to slave ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/41272/6/src/soc/amd/common/block/lp... File src/soc/amd/common/block/lpc/espi_util.c:
https://review.coreboot.org/c/coreboot/+/41272/6/src/soc/amd/common/block/lp... PS6, Line 309: #define ESPI_DN_TX_HDR0 0x00 : enum espi_cmd_type { : CMD_TYPE_SET_CONFIGURATION = 0, : CMD_TYPE_GET_CONFIGURATION = 1, : CMD_TYPE_IN_BAND_RESET = 2, : CMD_TYPE_PERIPHERAL = 4, : CMD_TYPE_VW = 5, : CMD_TYPE_OOB = 6, : CMD_TYPE_FLASH = 7, : }; : : #define ESPI_DN_TX_HDR1 0x04 : #define ESPI_DN_TX_HDR2 0x08 : #define ESPI_DN_TX_DATA 0x0c : : #define ESPI_MASTER_CAP 0x2c : #define ESPI_VW_MAX_SIZE_SHIFT 13 : #define ESPI_VW_MAX_SIZE_MASK (0x3f << ESPI_VW_MAX_SIZE_SHIFT) : : #define ESPI_GLOBAL_CONTROL_1 0x34 : #define ESPI_SUB_DECODE_SLV_SHIFT 3 : #define ESPI_SUB_DECODE_SLV_MASK (0x3 << ESPI_SUB_DECODE_SLV_SHIFT) : #define ESPI_SUB_DECODE_EN (1 << 2) : : #define SLAVE0_INT_STS 0x70 : #define ESPI_STATUS_DNCMD_COMPLETE (1 << 28) : #define ESPI_STATUS_NON_FATAL_ERROR (1 << 6) : #define ESPI_STATUS_FATAL_ERROR (1 << 5) : #define ESPI_STATUS_NO_RESPONSE (1 << 4) : #define ESPI_STATUS_CRC_ERR (1 << 2) : #define ESPI_STATUS_WAIT_TIMEOUT (1 << 1) : #define ESPI_STATUS_BUS_ERROR (1 << 0) : : #define ESPI_RXVW_POLARITY 0xac I wonder why these are in the .c file and not in the .h file like the rest of the defines. also the polarity register is defined in here, but the macros for the values are defined in the .h file
https://review.coreboot.org/c/coreboot/+/41272/6/src/soc/amd/common/block/lp... PS6, Line 393: __packed this struct doesn't need to be marked as packed
https://review.coreboot.org/c/coreboot/+/41272/6/src/soc/amd/common/block/lp... PS6, Line 423: check from this part of the function name I wouldn't expect it to poll SLAVE0_INT_STS to become non-zero
https://review.coreboot.org/c/coreboot/+/41272/6/src/soc/amd/common/block/lp... PS6, Line 487: maybe add an "unexpected" here?