Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 219: are ST, RV, PCO, RN, or LCN\ nit: It might be helpful to add full names as well so that it is clear what they mean:
ST = Stoneyridge PCO = Picasso
etc.
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1593: if (soc_name != NULL) { nit: This block can be moved into its own function. Something like `set_spi_params()`.
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1603: : if
else if
+1. Then you can also drop the `soc_name_set` variable.
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1625: amd_romsig->micron_detect_f17_mod_30_3f = efs_spi_micron_flag; This is not correct. `micron_detect_f17_mod_30_3f` should be set in the switch case below instead of `qpr_dummy_cycle_f17_mod_00_2f`.
https://review.coreboot.org/c/coreboot/+/42566/4/util/amdfwtool/amdfwtool.c@... PS4, Line 1629: qpr_dummy_cycle_f17_mod_00_2f This is not used by Fam17H Mod30-3fH. Instead it should be `micron_detect_f17_mod_30_3f`