Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46091 )
Change subject: mb/intel/adlrvp: Add ADL-P romstage mainboard code ......................................................................
Patch Set 7:
(9 comments)
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/board_id.h:
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... PS7, Line 6: #include <stdint.h>
not required
Ack
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/board_id.c:
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... PS7, Line 29: uint8_t buffer[2]; : size_t index; : if (send_ec_command(EC_FAB_ID_CMD) == 0) { : for (index = 0; index < sizeof(buffer); index++) : buffer[index] = recv_ec_data(); : id = (buffer[0] << 8) | buffer[1]; : }
suggestion: I would rewrite this buffer and the loop: […]
Ack
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... PS7, Line 39: 0x3f
symbolic constant here would be nice, even if it's BOARD_ID_MASK 😊
Ack
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/romstage_fsp_params.c:
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... PS7, Line 13: mainboard_get_spd_index
nit: this function is a "local" function, no need for the `mainboard` prefix
Ack
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... PS7, Line 20: 0x1F) & 0x7;
symbolic constants would be nice
Ack
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... PS7, Line 48: bool
const and can go up at the top w/ the other locals
Ack
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/variants/adlrvp_p/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... PS7, Line 8: register "SaGv" = "SaGv_Disabled"
nit: this is the default value
Tim, Default is subject to change over FSP version, in latest code I'm seeing default is 0x5 enable but for early SOC i would like to make Sagv disable
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... PS7, Line 43: disable
disabled
Ack
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... File src/mainboard/intel/adlrvp/variants/baseboard/include/baseboard/variants.h:
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... PS7, Line 13: adl_p_lp4_1 = 0x10, : adl_p_lp4_2 = 0x11, : /* ADL-P DDR4 RVPs */ : adl_p_ddr4_1 = 0x14, : adl_p_ddr4_2 = 0x3F, :
typically prefer uppercase enum values
Ack