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

[Maximilian Brune (Code Review)]

Attention is currently required from: Anand Vaikar, Bao Zheng, Felix Held.

Maximilian Brune 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:

(6 comments)

Patchset:

PS4: 
Sry for the late reply. I had some other stuff on my TODO list.

File util/amdfwtool/amdfwtool.c:

https://review.coreboot.org/c/coreboot/+/84373/comment/5fd81587_d13809e4?usp... :
PS4, Line 374: BDT_BOTH
> not sure what's correct here, but this is inconsistent with the rest. […]

I don't know the right answer either. What do you think how we should proceed?
I am honestly not sure what many of these binaries actually so I don't know if we need them all in both directory levels.

File util/amdfwtool/data_parse.c:

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

not sure how that happened.

https://review.coreboot.org/c/coreboot/+/84373/comment/8ae440f0_fa6f8a68?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. […]

I got that from AMD, but I also assumed microcode update went through PSP, since no AMD SOC uses `CONFIG_SUPPORT_CPU_UCODE_IN_CBFS`. But now I see it goes through `CONFIG_SOC_AMD_COMMON_BLOCK_UCODE`.
Anyway I removed it and now use coreboot (CBFS) to apply the microcode.

https://review.coreboot.org/c/coreboot/+/84373/comment/202cd67a_011cf600?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. […]

Done

https://review.coreboot.org/c/coreboot/+/84373/comment/a5d71100_8039c532?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 th […]

I can only say that it is a fairly big file (~300K), I don't know its actual contents.