Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Paul Menzel, Angel Pons, Aamir Bohra, Fred Reitberger, Felix Held. ritul guru has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60968 )
Change subject: soc/amd/common/block/psp: Add platform secure boot support ......................................................................
Patch Set 18:
(12 comments)
File src/soc/amd/common/block/psp/psb.c:
https://review.coreboot.org/c/coreboot/+/60968/comment/bb39ff1f_40682d0e PS17, Line 18: /* PSB Test Status and Error Codes (doc#56654) */
See my comments below, PSB_TEST_STATUS_PASS can be used in the check before printing the error strin […]
Ack
https://review.coreboot.org/c/coreboot/+/60968/comment/2c97d768_2af9d0a8 PS17, Line 18: /* PSB Test Status and Error Codes (doc#56654) */
See my comments below, PSB_TEST_STATUS_PASS can be used in the check before printing the error strin […]
Ack
https://review.coreboot.org/c/coreboot/+/60968/comment/adbf59eb_3e56dd5e PS17, Line 30:
See my comments below, FUSE_STATUS_SUCCESS can be used in the check before printing the error string […]
Ack
https://review.coreboot.org/c/coreboot/+/60968/comment/e6f685ed_0e7f56da PS17, Line 30:
See my comments below, FUSE_STATUS_SUCCESS can be used in the check before printing the error string […]
Ack
https://review.coreboot.org/c/coreboot/+/60968/comment/029824a4_c9ac33a0 PS17, Line 35: error:
Is this prefix needed? These strings are printed with BIOS_ERR log level.
Ack
https://review.coreboot.org/c/coreboot/+/60968/comment/c47c652a_1292f881 PS17, Line 127: read32
You can use `read32p` and drop the cast
Ack
https://review.coreboot.org/c/coreboot/+/60968/comment/183d3b71_a2402e49 PS17, Line 167: psb_test_status = status & PSB_TEST_STATUS_MASK;
nit: I'd make this const and declare/initialize it in one line: […]
Ack
https://review.coreboot.org/c/coreboot/+/60968/comment/992e845a_c847fe96 PS17, Line 169: if (psb_test_status) {
This assumes that 0 means success. […]
Ack
https://review.coreboot.org/c/coreboot/+/60968/comment/459ca284_86599c0f PS17, Line 181: cmd_status = send_psp_command(MBOX_BIOS_CMD_PSB_AUTO_FUSING, &buffer);
nit: I'd make this const and declare/initialize it in one line: […]
Ack
https://review.coreboot.org/c/coreboot/+/60968/comment/f8a69681_8200c888 PS17, Line 190: fuse_status = read32(&buffer.header.status);
nit: I'd make this const and declare/initialize it in one line: […]
Ack
https://review.coreboot.org/c/coreboot/+/60968/comment/29458179_c43f90f8 PS17, Line 191: if (fuse_status) {
This assumes that 0 means success. […]
Ack
https://review.coreboot.org/c/coreboot/+/60968/comment/5b75e865_0b7cda0a PS17, Line 206: *
nit: drop empty comment line
Ack
-
ritul guru (Code Review)