Attention is currently required from: Bao Zheng, Jason Glenesk, Raul Rangel, Matt DeVillier, Zheng Bao, Fred Reitberger, Felix Held.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69429 )
Change subject: amdfwtool: Set the base address of AMDFW as relative ......................................................................
Patch Set 2:
(7 comments)
Patchset:
PS2: I think we might want to look at rearchitecting this somewhat. Maybe consider using the offset from the TOP of the rom space instead of the bottom. Then the rom size isn't needed.
File src/mainboard/amd/mandolin/Kconfig:
https://review.coreboot.org/c/coreboot/+/69429/comment/fb2ba2c8_a28b4ad1 PS2, Line 73: default 5 if BOARD_AMD_MANDOLIN why change this?
File src/soc/amd/cezanne/Kconfig:
https://review.coreboot.org/c/coreboot/+/69429/comment/61986a30_b95c479e PS2, Line 322: default 5 Why?
https://review.coreboot.org/c/coreboot/+/69429/comment/16638832_52fd3c1c PS2, Line 334: comment "AMD Firmware Directory Table set to location 0xFA0000" You've updated the addresses from a memory location to the location in the spi rom without saying so.
Maybe update the comments to something like:
AMD EFS Table set to SPI ROM location 0xFA0000
File src/soc/amd/cezanne/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/69429/comment/603632cf_79f3663e PS2, Line 69: CEZANNE_FWM_POSITION_REL=$(call int-add, \ : $(call int-subtract, 0xffffff \ : $(call int-shift-left, \ : 0x80000 $(CONFIG_AMD_FWM_POSITION_INDEX))) 0x20000 1) : CEZANNE_FWM_POSITION_PHY=$(call int-add, \ : $(call int-subtract, 0xffffffff $(CONFIG_ROM_SIZE)) \ : $(CEZANNE_FWM_POSITION_REL) 1) : : # 0x40 accounts for the cbfs_file struct + filename + metadata structs, aligned to 64 bytes : # Building the cbfs image will fail if the offset isn't large enough : AMD_FW_AB_POSITION := 0x40 : : CEZANNE_FW_A_POSITION=$(call int-subtract, \ : $(call int-add, \ : $(shell awk '$$2 == "FMAP_SECTION_FW_MAIN_A_START" {print $$3}' $(obj)/fmap_config.h) \ : $(AMD_FW_AB_POSITION)) \ : $(shell awk '$$2 == "FMAP_SECTION_FLASH_START" {print $$3}' $(obj)/fmap_config.h)) : : CEZANNE_FW_B_POSITION=$(call int-subtract, \ : $(call int-add, \ : $(shell awk '$$2 == "FMAP_SECTION_FW_MAIN_B_START" {print $$3}' $(obj)/fmap_config.h) \ : $(AMD_FW_AB_POSITION)) \ : $(shell awk '$$2 == "FMAP_SECTION_FLASH_START" {print $$3}' $(obj)/fmap_config.h)) I thought we were getting rid of the calculations and just setting the actual values in Kconfig.
File src/soc/amd/mendocino/Kconfig:
https://review.coreboot.org/c/coreboot/+/69429/comment/7ef5ade2_faf44b9a PS2, Line 327: config AMD_FWM_POSITION_INDEX : int "Firmware Directory Table location (0 to 5)" : range 0 5 : default 5 If we're repeating the same stuff over and over, why not make a single common Kconfig file that has all of this instead of updating it in every location?
File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/69429/comment/664d8212_ba80d9b7 PS2, Line 2293: dir_location Can we change everywhere it formerly said location to now say offset?
Previously it was an address, now it's the offset from the start of the SPI rom, right?
-
Martin Roth (Code Review)