Tim Wawrzynczak 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
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: ``` if (send_ec_command(EC_FAB_ID_CMD) == 0) { id = recv_ec_data() << 8; id |= recv_ec_data(); } ```
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 😊
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
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... PS7, Line 20: 0x1F) & 0x7; symbolic constants would be nice
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
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
https://review.coreboot.org/c/coreboot/+/46091/7/src/mainboard/intel/adlrvp/... PS7, Line 43: disable disabled
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