Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41943 )
Change subject: sb/intel/lynxpoint: Clean up code ......................................................................
Patch Set 3:
(7 comments)
https://review.coreboot.org/c/coreboot/+/41943/3/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/elog.c:
https://review.coreboot.org/c/coreboot/+/41943/3/src/southbridge/intel/lynxp... PS3, Line 143: /*
missed that one?
I left this one like this on purpose, because there's several wake sources (the events listed below)
https://review.coreboot.org/c/coreboot/+/41943/3/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/me.h:
https://review.coreboot.org/c/coreboot/+/41943/3/src/southbridge/intel/lynxp... PS3, Line 111: #define ME_HFS2_STATE_BUP_FLOW_DET 4 : #define ME_HFS2_STATE_BUP_VSCC_ERR 8 : #define ME_HFS2_STATE_BUP_CHECK_STRAP 0xa : #define ME_HFS2_STATE_BUP_PWR_OK_TIMEOUT 0xb : #define ME_HFS2_STATE_BUP_MANUF_OVRD_STRAP 0xd
maybe everything as hex?
Probably, I'd rather do it on a follow-up though
https://review.coreboot.org/c/coreboot/+/41943/3/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/me_status.c:
https://review.coreboot.org/c/coreboot/+/41943/3/src/southbridge/intel/lynxp... PS3, Line 78: /* Progress Code 1 states */ : static const char *me_progress_bup_values[] = { : [ME_HFS2_STATE_BUP_INIT] = "Initialization starts", : [ME_HFS2_STATE_BUP_DIS_HOST_WAKE] = "Disable the host wake event", : [ME_HFS2_STATE_BUP_FLOW_DET] = "Flow determination start process", : [ME_HFS2_STATE_BUP_VSCC_ERR] = "Error reading/matching the VSCC table in the descriptor", : [ME_HFS2_STATE_BUP_CHECK_STRAP] = "Check to see if straps say ME DISABLED", : [ME_HFS2_STATE_BUP_PWR_OK_TIMEOUT] = "Timeout waiting for PWROK", : [ME_HFS2_STATE_BUP_MANUF_OVRD_STRAP] = "Possibly handle BUP manufacturing override strap", : [ME_HFS2_STATE_BUP_M3] = "Bringup in M3", : [ME_HFS2_STATE_BUP_M0] = "Bringup in M0", : [ME_HFS2_STATE_BUP_FLOW_DET_ERR] = "Flow detection error", : [ME_HFS2_STATE_BUP_M3_CLK_ERR] = "M3 clock switching error", : [ME_HFS2_STATE_BUP_CPU_RESET_DID_TIMEOUT_MEM_MISSING] = "Host error - CPU reset timeout, DID timeout, memory missing", : [ME_HFS2_STATE_BUP_M3_KERN_LOAD] = "M3 kernel load", : [ME_HFS2_STATE_BUP_T32_MISSING] = "T34 missing - cannot program ICC", : [ME_HFS2_STATE_BUP_WAIT_DID] = "Waiting for DID BIOS message", : [ME_HFS2_STATE_BUP_WAIT_DID_FAIL] = "Waiting for DID BIOS message failure", : [ME_HFS2_STATE_BUP_DID_NO_FAIL] = "DID reported no error", : [ME_HFS2_STATE_BUP_ENABLE_UMA] = "Enabling UMA", : [ME_HFS2_STATE_BUP_ENABLE_UMA_ERR] = "Enabling UMA error", : [ME_HFS2_STATE_BUP_SEND_DID_ACK] = "Sending DID Ack to BIOS", : [ME_HFS2_STATE_BUP_SEND_DID_ACK_ERR] = "Sending DID Ack to BIOS error", : [ME_HFS2_STATE_BUP_M0_CLK] = "Switching clocks in M0", : [ME_HFS2_STATE_BUP_M0_CLK_ERR] = "Switching clocks in M0 error", : [ME_HFS2_STATE_BUP_TEMP_DIS] = "ME in temp disable", : [ME_HFS2_STATE_BUP_M0_KERN_LOAD] = "M0 kernel load", : }; : : /* Progress Code 3 states */ : static const char *me_progress_policy_values[] = { : [ME_HFS2_STATE_POLICY_ENTRY] = "Entry into Policy Module", : [ME_HFS2_STATE_POLICY_RCVD_S3] = "Received S3 entry", : [ME_HFS2_STATE_POLICY_RCVD_S4] = "Received S4 entry", : [ME_HFS2_STATE_POLICY_RCVD_S5] = "Received S5 entry", : [ME_HFS2_STATE_POLICY_RCVD_UPD] = "Received UPD entry", : [ME_HFS2_STATE_POLICY_RCVD_PCR] = "Received PCR entry", : [ME_HFS2_STATE_POLICY_RCVD_NPCR] = "Received NPCR entry", : [ME_HFS2_STATE_POLICY_RCVD_HOST_WAKE] = "Received host wake", : [ME_HFS2_STATE_POLICY_RCVD_AC_DC] = "Received AC<>DC switch", : [ME_HFS2_STATE_POLICY_RCVD_DID] = "Received DRAM Init Done", : [ME_HFS2_STATE_POLICY_VSCC_NOT_FOUND] = "VSCC Data not found for flash device", : [ME_HFS2_STATE_POLICY_VSCC_INVALID] = "VSCC Table is not valid", : [ME_HFS2_STATE_POLICY_FPB_ERR] = "Flash Partition Boundary is outside address space", : [ME_HFS2_STATE_POLICY_DESCRIPTOR_ERR] = "ME cannot access the chipset descriptor region", : [ME_HFS2_STATE_POLICY_VSCC_NO_MATCH] = "Required VSCC values for flash part
what about these?
They go over 96 characters, so I didn't feel like touching them much.
https://review.coreboot.org/c/coreboot/+/41943/3/src/southbridge/intel/lynxp... File src/southbridge/intel/lynxpoint/pch.h:
https://review.coreboot.org/c/coreboot/+/41943/3/src/southbridge/intel/lynxp... PS3, Line 403: K 0x800 : #define SIO_REG_PPR_CLOCK_EN (1 << 0) : #define SIO_REG_PPR_RST 0x804 : #define SIO_REG_PPR_RST_ASSERT 0x3 : #define SIO_REG_PPR_GEN 0x808 : #define SIO_REG_PPR_GEN_LTR_MODE_MASK (1 << 2) : #define SIO_REG_PPR_GEN_VOLTAGE_MASK (1 << 3) : #define SIO_REG_PPR_GEN_VOLTAGE(x) ((x & 1) << 3) : #define SIO_REG_AUTO_LTR 0x814 : : #define SIO_REG_SDIO_PPR_GEN 0x1008 : #define SIO_REG_SDIO_PPR_SW_LTR 0x1010 : #define SIO_REG_SDIO_PPR_CMD12 0x3c : #define SIO_REG_SDIO_PPR_CMD12_B30 (1 << 30) :
alignment
I don't see any problems. If you are saying that some of these have an extra space, it's because they are fields inside a register.
https://review.coreboot.org/c/coreboot/+/41943/3/src/southbridge/intel/lynxp... PS3, Line 435: PIOBASE 0x48 : : #define PMBASE 0x40 :
same
*sigh*
https://review.coreboot.org/c/coreboot/+/41943/3/src/southbridge/intel/lynxp... PS3, Line 582: 3400 /* 32bit */ : #define HPTC 0x3404 /* 32bit */ : #define GCS 0x3410 /* 32bit */ : #define BUC 0x3414 /* 32bit */ : #define PCH_DISABLE_GBE (1 << 5) : #define FD 0x3418 /* 32bit */ : #define DISPBDF 0x3424 /* 16bit */ : #define FD2 0x3428 /* 32bit */ : #define CG 0x341c /* 32bit */
once again, alignment
This is aligned. PCH_DISABLE_GBE is a bit inside a register, so it should stick out.
https://review.coreboot.org/c/coreboot/+/41943/3/src/southbridge/intel/lynxp... PS3, Line 703: x) RCBA8(x + SPIBAR_OFFSET) : #define SPIBAR16(x) RCBA16
another one :)
... *sigh*