Rob Barnes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45222 )
Change subject: soc/amd/picasso: Moves APCB generation out of picasso ......................................................................
soc/amd/picasso: Moves APCB generation out of picasso
Moved APCB generation out of the picasso makefile and into the mainboard makefile. APCB generation tends to be mainboard specific and does not belong in the soc makefile.
BUG=b:168099242 TEST=Build mandolin and check for APCB in coreboot binary Build and boot ezkinil
Change-Id: Ib85ad94e515f2ffad58aafe06c1f1d4043e9303c Signed-off-by: Rob Barnes robbarnes@google.com --- M src/mainboard/amd/mandolin/Makefile.inc M src/mainboard/google/zork/spd/Makefile.inc M src/soc/amd/picasso/Makefile.inc 3 files changed, 23 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/45222/1
diff --git a/src/mainboard/amd/mandolin/Makefile.inc b/src/mainboard/amd/mandolin/Makefile.inc index 1865e74..6d7bf3a 100644 --- a/src/mainboard/amd/mandolin/Makefile.inc +++ b/src/mainboard/amd/mandolin/Makefile.inc @@ -10,8 +10,7 @@ ramstage-y += emmc_gpio.c endif
-# APCB_mandolin.bin -APCB_SOURCES = mandolin +APCB_SOURCES = $(MAINBOARD_BLOBS_DIR)/APCB_mandolin.bin
PHONY+=add_mchp_fw INTERMEDIATE+=add_mchp_fw diff --git a/src/mainboard/google/zork/spd/Makefile.inc b/src/mainboard/google/zork/spd/Makefile.inc index 66957d8..1042d91 100644 --- a/src/mainboard/google/zork/spd/Makefile.inc +++ b/src/mainboard/google/zork/spd/Makefile.inc @@ -2,3 +2,23 @@
# This directory SPD_SOURCES_DIR := src/mainboard/$(MAINBOARDDIR)/spd + +APCB_SOURCES=$(foreach f, $(basename $(SPD_SOURCES)), $(obj)/APCB_$(f).gen) + +# APCB binary with magic numbers to be replaced by apcb_edit tool +APCB_MAGIC_BLOB:=$(FIRMWARE_LOCATE)/APCB_magic.bin + +$(obj)/APCB_%.gen: $(SPD_SOURCES_DIR)/%.hex \ + $(APCB_EDIT_TOOL) \ + $(APCB_MAGIC_BLOB) + $(APCB_EDIT_TOOL) \ + $(APCB_MAGIC_BLOB) \ + $@ \ + --hex \ + --strip_manufacturer_information \ + --spd_0_0 $< \ + $(if $(APCB_POPULATE_2ND_CHANNEL), --spd_1_0 $<, ) \ + --board_id_gpio0 $(APCB_BOARD_ID_GPIO0) \ + --board_id_gpio1 $(APCB_BOARD_ID_GPIO1) \ + --board_id_gpio2 $(APCB_BOARD_ID_GPIO2) \ + --board_id_gpio3 $(APCB_BOARD_ID_GPIO3) diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index f4ec6c9..8789694 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -196,9 +196,8 @@ # # BIOS Directory Table items - proper ordering is managed by amdfwtool # - # type = 0x60 -PSP_APCB_FILES=$(foreach f, $(basename $(SPD_SOURCES)), $(obj)/APCB_$(f).bin) +PSP_APCB_FILES=$(APCB_SOURCES)
# type = 0x61 PSP_APOB_BASE=$(CONFIG_PSP_APOB_DRAM_ADDRESS) @@ -383,28 +382,6 @@ --soc-name "Picasso" \ --flashsize $(CONFIG_ROM_SIZE)
-# Copy prebuild APCBs if they exist -$(obj)/APCB_%.bin: $(MAINBOARD_BLOBS_DIR)/APCB_%.bin - cp $< $@ - -# APCB binary with magic numbers to be replaced by apcb_edit tool -APCB_MAGIC_BLOB:=$(FIRMWARE_LOCATE)/APCB_magic.bin - -$(obj)/APCB_%.bin: $$(SPD_SOURCES_DIR)/%.hex \ - $(APCB_EDIT_TOOL) \ - $(APCB_MAGIC_BLOB) - $(APCB_EDIT_TOOL) \ - $(APCB_MAGIC_BLOB) \ - $@ \ - --hex \ - --strip_manufacturer_information \ - --spd_0_0 $< \ - $(if $(APCB_POPULATE_2ND_CHANNEL), --spd_1_0 $<, ) \ - --board_id_gpio0 $(APCB_BOARD_ID_GPIO0) \ - --board_id_gpio1 $(APCB_BOARD_ID_GPIO1) \ - --board_id_gpio2 $(APCB_BOARD_ID_GPIO2) \ - --board_id_gpio3 $(APCB_BOARD_ID_GPIO3) - $(obj)/amdfw.rom: $(call strip_quotes, $(CONFIG_AMD_PUBKEY_FILE)) \ $(call strip_quotes, $(PSPBTLDR_FILE)) \ $(call strip_quotes, $(PSPSCUREOS_FILE)) \ @@ -443,6 +420,7 @@ $$(PSP_APCB_FILES) \ $(AMDFWTOOL) \ $(obj)/fmap.fmd + $(if $(PSP_APCB_FILES), ,$(error APCB_SOURCES is not set)) rm -f $@ @printf " AMDFWTOOL $(subst $(obj)/,,$(@))\n" $(AMDFWTOOL) \
Hello Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45222
to look at the new patch set (#2).
Change subject: soc/amd/picasso: Moves APCB generation out of picasso ......................................................................
soc/amd/picasso: Moves APCB generation out of picasso
Moved APCB generation out of the picasso makefile and into the mainboard makefile. APCB generation tends to be mainboard specific and does not belong in the soc makefile.
BUG=b:168099242 TEST=Build mandolin and check for APCB in coreboot binary Build and boot ezkinil
Change-Id: Ib85ad94e515f2ffad58aafe06c1f1d4043e9303c Signed-off-by: Rob Barnes robbarnes@google.com --- M Documentation/soc/amd/family17h.md M src/mainboard/amd/mandolin/Makefile.inc M src/mainboard/google/zork/spd/Makefile.inc M src/soc/amd/picasso/Makefile.inc 4 files changed, 26 insertions(+), 63 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/45222/2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45222 )
Change subject: soc/amd/picasso: Moves APCB generation out of picasso ......................................................................
Patch Set 2: Code-Review+1
this fixes the regression on Mandolin. will do a proper code review later
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45222 )
Change subject: soc/amd/picasso: Moves APCB generation out of picasso ......................................................................
Patch Set 2: Code-Review+2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45222 )
Change subject: soc/amd/picasso: Moves APCB generation out of picasso ......................................................................
Patch Set 2: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45222 )
Change subject: soc/amd/picasso: Moves APCB generation out of picasso ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45222/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45222/2//COMMIT_MSG@7 PS2, Line 7: Moves Please use imperative mood.
Move …
https://review.coreboot.org/c/coreboot/+/45222/2//COMMIT_MSG@9 PS2, Line 9: Moved APCB generation out of the picasso makefile and into the mainboard Normally present tense is used in summaries.
Hello build bot (Jenkins), Martin Roth, Patrick Georgi, Martin Roth, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45222
to look at the new patch set (#3).
Change subject: soc/amd/picasso: Move APCB generation out of picasso ......................................................................
soc/amd/picasso: Move APCB generation out of picasso
Move APCB generation out of the picasso makefile and into the mainboard makefile. APCB generation tends to be mainboard specific and does not belong in the soc makefile.
BUG=b:168099242 TEST=Build mandolin and check for APCB in coreboot binary Build and boot ezkinil
Change-Id: Ib85ad94e515f2ffad58aafe06c1f1d4043e9303c Signed-off-by: Rob Barnes robbarnes@google.com --- M Documentation/soc/amd/family17h.md M src/mainboard/amd/mandolin/Makefile.inc M src/mainboard/google/zork/spd/Makefile.inc M src/soc/amd/picasso/Makefile.inc 4 files changed, 26 insertions(+), 63 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/45222/3
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45222 )
Change subject: soc/amd/picasso: Move APCB generation out of picasso ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45222/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45222/2//COMMIT_MSG@7 PS2, Line 7: Moves
Please use imperative mood. […]
Done
https://review.coreboot.org/c/coreboot/+/45222/2//COMMIT_MSG@9 PS2, Line 9: Moved APCB generation out of the picasso makefile and into the mainboard
Normally present tense is used in summaries.
Done
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45222 )
Change subject: soc/amd/picasso: Move APCB generation out of picasso ......................................................................
Patch Set 3: Code-Review+2
Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45222 )
Change subject: soc/amd/picasso: Move APCB generation out of picasso ......................................................................
soc/amd/picasso: Move APCB generation out of picasso
Move APCB generation out of the picasso makefile and into the mainboard makefile. APCB generation tends to be mainboard specific and does not belong in the soc makefile.
BUG=b:168099242 TEST=Build mandolin and check for APCB in coreboot binary Build and boot ezkinil
Change-Id: Ib85ad94e515f2ffad58aafe06c1f1d4043e9303c Signed-off-by: Rob Barnes robbarnes@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45222 Reviewed-by: Raul Rangel rrangel@chromium.org Reviewed-by: Martin Roth martinroth@google.com Reviewed-by: Felix Held felix-coreboot@felixheld.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M Documentation/soc/amd/family17h.md M src/mainboard/amd/mandolin/Makefile.inc M src/mainboard/google/zork/spd/Makefile.inc M src/soc/amd/picasso/Makefile.inc 4 files changed, 26 insertions(+), 63 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Felix Held: Looks good to me, approved Raul Rangel: Looks good to me, approved
diff --git a/Documentation/soc/amd/family17h.md b/Documentation/soc/amd/family17h.md index 23088cd..fffe25b 100755 --- a/Documentation/soc/amd/family17h.md +++ b/Documentation/soc/amd/family17h.md @@ -240,47 +240,12 @@ ## APCB setup
APCBs are used to provide the PSP with SPD information and optionally a set of -GPIOs to use for selecting which SPD to load. - -### Prebuilt -The picasso `Makefile` expects APCBs to be located in -`3rdparty/blobs/mainboard/$(MAINBOARDDIR)`. If you have a pre-built binary just -add the following to your mainboard's Makefile. - -``` -# i.e., 3rdparty/blobs/mainboard/amd/mandolin/APCB_mandolin.bin -APCB_SOURCES = mandolin -``` +GPIOs to use for selecting which SPD to load. A list of APCB files should be +specified in `APCB_SOURCES`.
### Generating APCBs If you have a template APCB file, the `apcb_edit` tool can be used to inject the -SPD and GPIOs used to select the correct slot. Entries should match this -pattern `{NAME}_x{1,2}`. There should be a matching SPD hex file in -`SPD_SOURCES_DIR` matching the pattern `{NAME}.spd.hex`. -The `_x{1,2}` suffix denotes single or dual channel. Up to 16 slots can be used. -If a slot is empty, the special empty keyword can be used. This will generate -an APCB with an empty SPD. - -``` -APCB_SOURCES = hynix-HMA851S6CJR6N-VK_x1 # 0b0000 -APCB_SOURCES += hynix-HMAA1GS6CMR6N-VK_x2 # 0b0001 -APCB_SOURCES += empty # 0b0010 -APCB_SOURCES += samsung-K4A8G165WC-BCWE_x1 # 0b0011 -``` - -#### APCB Board ID GPIO configuration. -The GPIOs determine which memory SPD will be used during boot. -``` -# APCB_BOARD_ID_GPIO[0-3] = GPIO_NUMBER GPIO_IO_MUX GPIO_BANK_CTL -# GPIO_NUMBER: FCH GPIO number -# GPIO_IO_MUX: Value write to IOMUX to configure this GPIO -# GPIO_BANK_CTL: Value write to GPIOBankCtl[23:16] to configure this GPIO - -APCB_BOARD_ID_GPIO0 = 121 1 0 -APCB_BOARD_ID_GPIO1 = 120 1 0 -APCB_BOARD_ID_GPIO2 = 131 3 0 -APCB_BOARD_ID_GPIO3 = 116 1 0 -``` +SPD and GPIOs used to select the correct slot.
## Footnotes
diff --git a/src/mainboard/amd/mandolin/Makefile.inc b/src/mainboard/amd/mandolin/Makefile.inc index 1865e74..6d7bf3a 100644 --- a/src/mainboard/amd/mandolin/Makefile.inc +++ b/src/mainboard/amd/mandolin/Makefile.inc @@ -10,8 +10,7 @@ ramstage-y += emmc_gpio.c endif
-# APCB_mandolin.bin -APCB_SOURCES = mandolin +APCB_SOURCES = $(MAINBOARD_BLOBS_DIR)/APCB_mandolin.bin
PHONY+=add_mchp_fw INTERMEDIATE+=add_mchp_fw diff --git a/src/mainboard/google/zork/spd/Makefile.inc b/src/mainboard/google/zork/spd/Makefile.inc index 66957d8..1042d91 100644 --- a/src/mainboard/google/zork/spd/Makefile.inc +++ b/src/mainboard/google/zork/spd/Makefile.inc @@ -2,3 +2,23 @@
# This directory SPD_SOURCES_DIR := src/mainboard/$(MAINBOARDDIR)/spd + +APCB_SOURCES=$(foreach f, $(basename $(SPD_SOURCES)), $(obj)/APCB_$(f).gen) + +# APCB binary with magic numbers to be replaced by apcb_edit tool +APCB_MAGIC_BLOB:=$(FIRMWARE_LOCATE)/APCB_magic.bin + +$(obj)/APCB_%.gen: $(SPD_SOURCES_DIR)/%.hex \ + $(APCB_EDIT_TOOL) \ + $(APCB_MAGIC_BLOB) + $(APCB_EDIT_TOOL) \ + $(APCB_MAGIC_BLOB) \ + $@ \ + --hex \ + --strip_manufacturer_information \ + --spd_0_0 $< \ + $(if $(APCB_POPULATE_2ND_CHANNEL), --spd_1_0 $<, ) \ + --board_id_gpio0 $(APCB_BOARD_ID_GPIO0) \ + --board_id_gpio1 $(APCB_BOARD_ID_GPIO1) \ + --board_id_gpio2 $(APCB_BOARD_ID_GPIO2) \ + --board_id_gpio3 $(APCB_BOARD_ID_GPIO3) diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index f4ec6c9..eb2be50 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -198,7 +198,7 @@ #
# type = 0x60 -PSP_APCB_FILES=$(foreach f, $(basename $(SPD_SOURCES)), $(obj)/APCB_$(f).bin) +PSP_APCB_FILES=$(APCB_SOURCES)
# type = 0x61 PSP_APOB_BASE=$(CONFIG_PSP_APOB_DRAM_ADDRESS) @@ -383,28 +383,6 @@ --soc-name "Picasso" \ --flashsize $(CONFIG_ROM_SIZE)
-# Copy prebuild APCBs if they exist -$(obj)/APCB_%.bin: $(MAINBOARD_BLOBS_DIR)/APCB_%.bin - cp $< $@ - -# APCB binary with magic numbers to be replaced by apcb_edit tool -APCB_MAGIC_BLOB:=$(FIRMWARE_LOCATE)/APCB_magic.bin - -$(obj)/APCB_%.bin: $$(SPD_SOURCES_DIR)/%.hex \ - $(APCB_EDIT_TOOL) \ - $(APCB_MAGIC_BLOB) - $(APCB_EDIT_TOOL) \ - $(APCB_MAGIC_BLOB) \ - $@ \ - --hex \ - --strip_manufacturer_information \ - --spd_0_0 $< \ - $(if $(APCB_POPULATE_2ND_CHANNEL), --spd_1_0 $<, ) \ - --board_id_gpio0 $(APCB_BOARD_ID_GPIO0) \ - --board_id_gpio1 $(APCB_BOARD_ID_GPIO1) \ - --board_id_gpio2 $(APCB_BOARD_ID_GPIO2) \ - --board_id_gpio3 $(APCB_BOARD_ID_GPIO3) - $(obj)/amdfw.rom: $(call strip_quotes, $(CONFIG_AMD_PUBKEY_FILE)) \ $(call strip_quotes, $(PSPBTLDR_FILE)) \ $(call strip_quotes, $(PSPSCUREOS_FILE)) \ @@ -443,6 +421,7 @@ $$(PSP_APCB_FILES) \ $(AMDFWTOOL) \ $(obj)/fmap.fmd + $(if $(PSP_APCB_FILES), ,$(error APCB_SOURCES is not set)) rm -f $@ @printf " AMDFWTOOL $(subst $(obj)/,,$(@))\n" $(AMDFWTOOL) \
-
Aaron Durbin (Code Review) -
Felix Held (Code Review)
-
Martin Roth (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)
-
Rob Barnes (Code Review)