[coreboot-gerrit] [M] Change in coreboot[main]: util/amdfwtool: Add binaries

[Felix Held (Code Review)]

Attention is currently required from: Bao Zheng, Maximilian Brune.

Felix Held has posted comments on this change by Maximilian Brune. ( https://review.coreboot.org/c/coreboot/+/84373?usp=email )

Change subject: util/amdfwtool: Add binaries
......................................................................

Patch Set 4:

(5 comments)

File util/amdfwtool/amdfwtool.c:

https://review.coreboot.org/c/coreboot/+/84373/comment/e5666656_8e307fbc?usp... :
PS4, Line 374: BDT_BOTH 
not sure what's correct here, but this is inconsistent with the rest.
also iirc the newer socs don't have a level 2 bios directory table; this might apply to other entries in amd_bios_table too that have the level set or changed to BDT_BOTH  in this patch

File util/amdfwtool/data_parse.c:

https://review.coreboot.org/c/coreboot/+/84373/comment/4d3f070f_dd3601ca?usp... :
PS4, Line 556: 			subprog = 0;
why is this removed?

https://review.coreboot.org/c/coreboot/+/84373/comment/7097ccd4_d6bdff3b?usp... :
PS4, Line 629: 	} else if (strcmp(fw_name, "PSP_BDT_UCODE_INS0") == 0) {
             : 		fw_type = AMD_BIOS_UCODE;
             : 		subprog = 0;
             : 		instance = 0;
             : 	} else if (strcmp(fw_name, "PSP_BDT_UCODE_INS1") == 0) {
             : 		fw_type = AMD_BIOS_UCODE;
             : 		subprog = 0;
             : 		instance = 1;
is this needed? for other socs, we keep the ucode update in cbfs and apply it from coreboot. also looks like this is already handled in util/amdfwtool/opts.c?

https://review.coreboot.org/c/coreboot/+/84373/comment/c8dcdf06_e0ea0ed5?usp... :
PS4, Line 637: 	} else if (strcmp(fw_name, "PSP_BDT_APCB_BK_INS0") == 0) {
             : 		fw_type = AMD_BIOS_APCB_BK;
             : 		subprog = 0;
             : 		instance = 0;
             : 	} else if (strcmp(fw_name, "PSP_BDT_APCB_BK_INS8") == 0) {
             : 		fw_type = AMD_BIOS_APCB_BK;
             : 		subprog = 0;
             : 		instance = 8;
this should be already handled in util/amdfwtool/opts.c

https://review.coreboot.org/c/coreboot/+/84373/comment/9e686fdb_516f4924?usp... :
PS4, Line 649: 		fw_type = AMD_BIOS_NV_ST;
probably not the right location in the code to comment about this, but i wonder if this is a file that has the nv range info of if this entry is just supposed to provide the range like it's done for a few other things in util/amdfwtool/opts.c and not have some file added for that