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 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 218: --soc-family Instead of taking family/model-min/model-max as inputs, would it make sense to have family name as input e.g. picasso, stoneyridge or PCO, STR. Then, amdfwtool can apply the translation from family-name to the right EFS fields without really having to sanitize the family/model-min/model-max values. This also relieves the SoC from having to pass in multiple values to identify itself.
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 239: <HEX_VAL> I think this will have to be a boolean flag or a tri-state flag like NO_MICRON_SUPPORT: Micron SPI flash is not supported by this board ONLY_MICRON_SUPPORT: Only Micron SPI flash is supported by this board MIXED_MICRON_SUPPORT: Both Micron and non-Micron SPI flash are supported by this board
Based on the value of this flag, amdfwtool will have to set values differently for `qpr_dummy_cycle_f17_mod_00_2f` and `micron_detect_f17_mod_30_3f`.
For `qpr_dummy_cycle_f17_mod_00_2f`: Set 0x0A for ONLY_MICRON_SUPPORT and 0xFF for NO_MICRON_SUPPORT. I don't think MIXED_MICRON_SUPPORT is available here.
For `micron_detect_f17_mod_30_3f`: Set 0x55 for MIXED_MICRON_SUPPORT, set 0xAA for ONLY_MICRON_SUPPORT and set 0x00/0xff for NO_MICRON_SUPPORT.
https://review.coreboot.org/c/coreboot/+/42566/3/util/amdfwtool/amdfwtool.c@... PS3, Line 1088: LONGOPT_FAMILY = 256, : LONGOPT_MODEL = 257, These are not really used.