Change in coreboot[master]: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps
Nikolai Vyssotski has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45278 )
Change subject: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps ......................................................................
WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps
Use GPIOs mapping defined in APCB GET_SPD_FROM_GPIO to fetch correct SPD block from PSP BIOS table entry type 0x69 (BIOS_SPD_DATA). There is only one APCB now with no SPD data in it. No need for board ID getting method. This will save ROM space consumed by multiple APCB copies in the past.
This is a working example of an implementation tested on Dalboz. It does not require patching with APCB_EDIT_TOOL. All SPDs are assembled in one type 0x69 blob and inserted into PSP BIOS directory.
BUG=b:127505025 TEST=Dalboz with CABLRV20090300 and test CB and boot loader code BRANCH=Zork
Change-Id: I9adf7a016ee481214b47d8adc6d53fd8ce0c9bd4 Signed-off-by: Nikolai Vyssotski nikolai.vyssotski@amd.corp-partner.google.com --- M src/soc/amd/picasso/Makefile.inc M util/amdfwtool/amdfwtool.c 2 files changed, 42 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/45278/1
diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index eb2be50..e83e08d 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -198,7 +198,7 @@ #
# type = 0x60 -PSP_APCB_FILES=$(APCB_SOURCES) +PSP_SPD_FILES=$(foreach f, $(basename $(SPD_SOURCES)), $(obj)/SPD_$(f).bin)
# type = 0x61 PSP_APOB_BASE=$(CONFIG_PSP_APOB_DRAM_ADDRESS) @@ -262,6 +262,12 @@ # type = 0xb - See #55758 (NDA) for bit definitions. PSP_SOFTFUSE_BITS += 28
+# SPD data array blob (with SPDs for all possible memory IDs for this platform) +SPD_BLOB:=$(obj)/SPD_array.bin + +# APCB binary without SPD data +APCB_NO_SPD_BLOB:=$(FIRMWARE_LOCATE)/APCB_magic.bin + # Helper function to return a value with given bit set set-bit=$(call int-shift-left, 1 $(call _toint,$1)) PSP_SOFTFUSE=$(shell A=$(call int-add, \ @@ -304,9 +310,8 @@ OPT_VERSTAGE_FILE=$(call add_opt_prefix, $(PSP_VERSTAGE_FILE), --verstage) OPT_VERSTAGE_SIG_FILE=$(call add_opt_prefix, $(PSP_VERSTAGE_SIG_FILE), --verstage_sig)
-OPT_PSP_APCB_FILES=$(foreach i, $(shell seq $(words $(PSP_APCB_FILES))), \ - $(call add_opt_prefix, $(word $(i), $(PSP_APCB_FILES)), \ - --instance $(shell printf "%x" $$(($(i)-1))) --apcb )) +OPT_PSP_APCB_FILE=$(call add_opt_prefix, $(APCB_NO_SPD_BLOB), --instance 0 --apcb) +OPT_SPD_BLOB=$(call add_opt_prefix, $(SPD_BLOB), --spd-data-blob)
OPT_APOB_ADDR=$(call add_opt_prefix, $(PSP_APOB_BASE), --apob-base) OPT_PSP_BIOSBIN_FILE=$(call add_opt_prefix, $(PSP_BIOSBIN_FILE), --bios-bin) @@ -341,7 +346,8 @@ $(OPT_SMUFW2_SUB2_FILE) \ $(OPT_SMUFW1_SUB1_FILE) \ $(OPT_SMUFW2_SUB1_FILE) \ - $(OPT_PSP_APCB_FILES) \ + $(OPT_PSP_APCB_FILE) \ + $(OPT_SPD_BLOB) \ $(OPT_APOB_ADDR) \ $(OPT_PSP_BIOSBIN_FILE) \ $(OPT_PSP_BIOSBIN_DEST) \ @@ -383,6 +389,16 @@ --soc-name "Picasso" \ --flashsize $(CONFIG_ROM_SIZE)
+# Copy prebuild APCBs if they exist +$(obj)/APCB_%.bin: $(MAINBOARD_BLOBS_DIR)/APCB_%.bin + cp $< $@ + +$(obj)/SPD_%.bin: $$(SPD_SOURCES_DIR)/%.hex + xxd -r -p $< > $@ + +$(SPD_BLOB): $$(PSP_SPD_FILES) + cat $+ > $@ + $(obj)/amdfw.rom: $(call strip_quotes, $(CONFIG_AMD_PUBKEY_FILE)) \ $(call strip_quotes, $(PSPBTLDR_FILE)) \ $(call strip_quotes, $(PSPSCUREOS_FILE)) \ @@ -418,12 +434,22 @@ $(call_strip_quotes, $(PSP_SEC_DEBUG_FILE)) \ $(PSP_VERSTAGE_FILE) \ $(PSP_VERSTAGE_SIG_FILE) \ - $$(PSP_APCB_FILES) \ + $(PSP_APCB_FILE) \ + $(SPD_BLOB) \ $(AMDFWTOOL) \ $(obj)/fmap.fmd $(if $(PSP_APCB_FILES), ,$(error APCB_SOURCES is not set)) rm -f $@ @printf " AMDFWTOOL $(subst $(obj)/,,$(@))\n" + echo $(AMDFWTOOL) \ + $(OPT_PSPBTLDR_FILE) \ + $(AMDFW_COMMON_ARGS) \ + $(OPT_APOB0_NV_SIZE) \ + $(OPT_APOB0_NV_BASE) \ + $(OPT_VERSTAGE_FILE) \ + $(OPT_VERSTAGE_SIG_FILE) \ + --location $(shell printf "%#x" $(PICASSO_FWM_POSITION)) \ + --output $@ $(AMDFWTOOL) \ $(OPT_PSPBTLDR_FILE) \ $(AMDFW_COMMON_ARGS) \ diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index f385068..2192a04 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -215,6 +215,7 @@ printf("-q | --anywhere Use any 64-byte aligned addr for Directory\n"); printf("-R | --sharedmem Location of PSP/FW shared memory\n"); printf("-P | --sharedmem-size Maximum size of the PSP/FW shared memory area\n"); + printf("--spd-data-blob Add SPD array\n"); printf("-C | --soc-name <socname> Specify SOC name. Supported names are\n"); printf(" Stoneyridge, Raven, Picasso, Renoir or Lucienne"); printf("-h | --help show this help\n"); @@ -251,6 +252,7 @@ AMD_BIOS_PMUD = 0x65, AMD_BIOS_UCODE = 0x66, AMD_BIOS_APCB_BK = 0x68, + AMD_BIOS_SPD_DATA = 0x69, AMD_BIOS_MP2_CFG = 0x6a, AMD_BIOS_PSP_SHARED_MEM = 0x6b, AMD_BIOS_L2_PTR = 0x70, @@ -409,6 +411,7 @@ { .type = AMD_BIOS_APCB_BK, .inst = 13, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB_BK, .inst = 14, .level = BDT_BOTH }, { .type = AMD_BIOS_APCB_BK, .inst = 15, .level = BDT_BOTH }, + { .type = AMD_BIOS_SPD_DATA, .inst = 0, .level = BDT_BOTH }, { .type = AMD_BIOS_APOB, .level = BDT_BOTH }, { .type = AMD_BIOS_BIN, .reset = 1, .copy = 1, .zlib = 1, .level = BDT_BOTH }, @@ -1085,6 +1088,7 @@ LONGOPT_SPI_READ_MODE = 256, LONGOPT_SPI_SPEED = 257, LONGOPT_SPI_MICRON_FLAG = 258, + LONGOPT_SPD_DATA = 259, };
// Unused values: D @@ -1136,6 +1140,7 @@ {"mp2-config", required_argument, 0, 'X' }, {"apob-nv-base", required_argument, 0, 'F' }, {"apob-nv-size", required_argument, 0, 'H' }, + {"spd-data-blob", required_argument, 0, LONGOPT_SPD_DATA }, /* Embedded Firmware Structure items*/ {"spi-read-mode", required_argument, 0, LONGOPT_SPI_READ_MODE }, {"spi-speed", required_argument, 0, LONGOPT_SPI_SPEED }, @@ -1450,6 +1455,11 @@ instance, optarg); sub = instance = 0; break; + case LONGOPT_SPD_DATA: + /* optional SPD data array outside of APCB */ + register_bdt_data(AMD_BIOS_SPD_DATA, sub, instance, optarg); + sub = instance = 0; + break; case 'Q': /* APOB destination */ register_fw_addr(AMD_BIOS_APOB, 0, optarg, 0);
Nikolai Vyssotski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45278 )
Change subject: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps ......................................................................
Patch Set 1:
This change is ready for review.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45278 )
Change subject: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps ......................................................................
Patch Set 1:
(3 comments)
I suspect you're going to need a phased approach and that may be why your Jenkins builds are failing. Maybe add the capability to amdfwtool and Makefile.inc, change the mainboards, then obsolete the old behavior.
https://review.coreboot.org/c/coreboot/+/45278/1/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45278/1/src/soc/amd/picasso/Makefil... PS1, Line 396: : Spaces here and on l.399
https://review.coreboot.org/c/coreboot/+/45278/1/src/soc/amd/picasso/Makefil... PS1, Line 443: : echo $(AMDFWTOOL) \ : $(OPT_PSPBTLDR_FILE) \ : $(AMDFW_COMMON_ARGS) \ : $(OPT_APOB0_NV_SIZE) \ : $(OPT_APOB0_NV_BASE) \ : $(OPT_VERSTAGE_FILE) \ : $(OPT_VERSTAGE_SIG_FILE) \ : --location $(shell printf "%#x" $(PICASSO_FWM_POSITION)) \ : --output $@ We probably want to omit this. We should be able to get the same info by running "make V=1". If you really intend to change the info printed during the build, it should be in a separate patch since it's unrelated.
https://review.coreboot.org/c/coreboot/+/45278/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/45278/1/util/amdfwtool/amdfwtool.c@... PS1, Line 218: Convert tabs to spaces
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45278 )
Change subject: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45278/1/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45278/1/src/soc/amd/picasso/Makefil... PS1, Line 265: # SPD data array blob (with SPDs for all possible memory IDs for this platform) : SPD_BLOB:=$(obj)/SPD_array.bin : : # APCB binary without SPD data : APCB_NO_SPD_BLOB:=$(FIRMWARE_LOCATE)/APCB_magic.bin We just removed all the SPD information from the picasso Makefile and put it into the Mainboard files because it's really mainboard specific. A lot of these changes undo what was done there.
https://review.coreboot.org/c/coreboot/+/45222
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45278 )
Change subject: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45278/1/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45278/1/src/soc/amd/picasso/Makefil... PS1, Line 265: # SPD data array blob (with SPDs for all possible memory IDs for this platform) : SPD_BLOB:=$(obj)/SPD_array.bin : : # APCB binary without SPD data : APCB_NO_SPD_BLOB:=$(FIRMWARE_LOCATE)/APCB_magic.bin
We just removed all the SPD information from the picasso Makefile and put it into the Mainboard file […]
We switched to passing APCB files to picasso. But with this change we should probably switch back to passing in SPD bins since we don't need the hacky logic for injecting SPDs into APCBs anymore.
We shouldn't need the APCB_magic.bin anymore right?
https://review.coreboot.org/c/coreboot/+/45278/1/src/soc/amd/picasso/Makefil... PS1, Line 396: :
Spaces here and on l. […]
There's an open CL for switching from hex to bin files. If you rebase on that CL this rule won't be needed: https://review.coreboot.org/c/coreboot/+/44775
Nikolai Vyssotski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45278 )
Change subject: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps ......................................................................
Patch Set 2: Code-Review-1
(3 comments)
RFC from Google. This is meant as a reference, not necessarily the final solution.
https://review.coreboot.org/c/coreboot/+/45278/1/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45278/1/src/soc/amd/picasso/Makefil... PS1, Line 265: # SPD data array blob (with SPDs for all possible memory IDs for this platform) : SPD_BLOB:=$(obj)/SPD_array.bin : : # APCB binary without SPD data : APCB_NO_SPD_BLOB:=$(FIRMWARE_LOCATE)/APCB_magic.bin
We switched to passing APCB files to picasso. […]
Reworked the patch to follow Rob's lead. APCB_magic.bin is just a name now - it can be anything. It happens to be that name because our APCB script make it that based on SpdInfo suffix in APCB sources directory.
https://review.coreboot.org/c/coreboot/+/45278/1/src/soc/amd/picasso/Makefil... PS1, Line 396: :
There's an open CL for switching from hex to bin files. […]
I agree. It will be superfluous when it gets merged. I will remove the rule when the change is in. For now I am keeping it so I can easily test it with the most recent code.
https://review.coreboot.org/c/coreboot/+/45278/1/src/soc/amd/picasso/Makefil... PS1, Line 443: : echo $(AMDFWTOOL) \ : $(OPT_PSPBTLDR_FILE) \ : $(AMDFW_COMMON_ARGS) \ : $(OPT_APOB0_NV_SIZE) \ : $(OPT_APOB0_NV_BASE) \ : $(OPT_VERSTAGE_FILE) \ : $(OPT_VERSTAGE_SIG_FILE) \ : --location $(shell printf "%#x" $(PICASSO_FWM_POSITION)) \ : --output $@
We probably want to omit this. We should be able to get the same info by running "make V=1". […]
Thanks, missed that one - did not mean to push it.
Hello build bot (Jenkins), Martin Roth, Patrick Georgi, Marshall Dawson, Rob Barnes, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45278
to look at the new patch set (#3).
Change subject: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps ......................................................................
WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps
Use GPIOs mapping defined in APCB GET_SPD_FROM_GPIO to fetch correct SPD block from PSP BIOS table entry type 0x69 (BIOS_SPD_DATA). There is only one APCB now with no SPD data in it. No need for board ID getting method. This will save ROM space consumed by multiple APCB copies in the past.
This is a working example of an implementation tested on Dalboz. It does not require patching with APCB_EDIT_TOOL. All SPDs are assembled in one type 0x69 blob and inserted into PSP BIOS directory.
BUG=b:127505025 TEST=Dalboz with CABLRV20090300 and test CB and boot loader code BRANCH=Zork
Change-Id: I9adf7a016ee481214b47d8adc6d53fd8ce0c9bd4 Signed-off-by: Nikolai Vyssotski nikolai.vyssotski@amd.corp-partner.google.com --- M src/mainboard/google/zork/spd/Makefile.inc M src/soc/amd/picasso/Makefile.inc M util/amdfwtool/amdfwtool.c 3 files changed, 31 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/45278/3
Nikolai Vyssotski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45278 )
Change subject: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps ......................................................................
Patch Set 3:
Fixed Mandolin build issue. It does not use multiple SPDs. Check if SPD_SOURCES is defined. If not, use the conventional way of building where SPD information is embedded into a single APCB.
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45278 )
Change subject: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45278/3/src/mainboard/google/zork/s... File src/mainboard/google/zork/spd/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45278/3/src/mainboard/google/zork/s... PS3, Line 7: SPD_BLOB:=$(obj)/SPD_array.bin Does this need to be here? It's constructed in the picasso makefile.
https://review.coreboot.org/c/coreboot/+/45278/3/src/mainboard/google/zork/s... PS3, Line 10: APCB_SOURCES:=$(FIRMWARE_LOCATE)/APCB_magic.bin Why is one even needed? Is this just to pass the validation check? If so, let's remove the check or make in an OR.
https://review.coreboot.org/c/coreboot/+/45278/3/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45278/3/src/soc/amd/picasso/Makefil... PS3, Line 243: #ifdef SPD_SOURCES Is this ifdef necessary? PSP_SPD_FILES will just be empty when its not defined.
https://review.coreboot.org/c/coreboot/+/45278/3/src/soc/amd/picasso/Makefil... PS3, Line 247: SPD_BLOB:=$(obj)/SPD_array.bin Maybe this be SPD_ARRAY_BLOB since it may contain multiple SPDs
Hello build bot (Jenkins), Martin Roth, Patrick Georgi, Marshall Dawson, Rob Barnes, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45278
to look at the new patch set (#4).
Change subject: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps ......................................................................
WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps
Use GPIOs mapping defined in APCB GET_SPD_FROM_GPIO to fetch correct SPD block from PSP BIOS table entry type 0x69 (BIOS_SPD_DATA). There is only one APCB now with no SPD data in it. No need for board ID getting method. This will save ROM space consumed by multiple APCB copies in the past.
This is a working example of an implementation tested on Dalboz. It does not require patching with APCB_EDIT_TOOL. All SPDs are assembled in one type 0x69 blob and inserted into PSP BIOS directory.
BUG=b:127505025 TEST=Dalboz with CABLRV20090300 and test CB and boot loader code BRANCH=Zork
Change-Id: I9adf7a016ee481214b47d8adc6d53fd8ce0c9bd4 Signed-off-by: Nikolai Vyssotski nikolai.vyssotski@amd.corp-partner.google.com --- M src/mainboard/google/zork/spd/Makefile.inc M src/soc/amd/picasso/Makefile.inc M util/amdfwtool/amdfwtool.c M util/apcb/apcb_edit.py 4 files changed, 105 insertions(+), 60 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/45278/4
Nikolai Vyssotski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45278 )
Change subject: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps ......................................................................
Patch Set 3:
(4 comments)
Zork and Dalboz use different GPIOs for memory configuration. It looks like we will still have to use APCB magic for integrating GPIO information into APCB. Until APCB build is open sourced we will have to do patching of the APCB_magic binary.
https://review.coreboot.org/c/coreboot/+/45278/3/src/mainboard/google/zork/s... File src/mainboard/google/zork/spd/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45278/3/src/mainboard/google/zork/s... PS3, Line 7: SPD_BLOB:=$(obj)/SPD_array.bin
Does this need to be here? It's constructed in the picasso makefile.
Missed that one. I meant to move it from top level make but on second thought it probably should stay there and get removed here.
https://review.coreboot.org/c/coreboot/+/45278/3/src/mainboard/google/zork/s... PS3, Line 10: APCB_SOURCES:=$(FIRMWARE_LOCATE)/APCB_magic.bin
Why is one even needed? Is this just to pass the validation check? If so, let's remove the check or […]
Presence of this variable indicates that this platform supports the new way of packing SPDs into type 0x69 with APCB not having any SPD info. This is how we can differentiate between cases of APCB with SPD(s) (Mandolin) and the the new SPD-agnostic APCB (SPD in type 0x69). We could add Kconfig option as an alternative.
https://review.coreboot.org/c/coreboot/+/45278/3/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45278/3/src/soc/amd/picasso/Makefil... PS3, Line 243: #ifdef SPD_SOURCES
Is this ifdef necessary? PSP_SPD_FILES will just be empty when its not defined.
Yes, unfortunately it is needed. Without it Mandolin build will be broken (no idea how build bot managed to mark it passing). I was actually experimenting with commenting them out prior to committing to confirm that they were indeed necessary. I managed to push my experimental version where they are both commented out. It should be ifdef (without #). It is hard to catch it switching between C and makefiles - I am glad you drew my attention to it. Without it SPD_BLOB gets defined which causes AMDFWTOOL to fail as it is looking for non-existing SPD_array.bin (from OPT_SPD_BLOB).
https://review.coreboot.org/c/coreboot/+/45278/3/src/soc/amd/picasso/Makefil... PS3, Line 247: SPD_BLOB:=$(obj)/SPD_array.bin
Maybe this be SPD_ARRAY_BLOB since it may contain multiple SPDs
Good idea.
Hello build bot (Jenkins), Martin Roth, Patrick Georgi, Marshall Dawson, Rob Barnes, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45278
to look at the new patch set (#5).
Change subject: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps ......................................................................
WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps
Use GPIOs mapping defined in APCB GET_SPD_FROM_GPIO to fetch correct SPD block from PSP BIOS table entry type 0x69 (BIOS_SPD_DATA). There is only one APCB now with no SPD data in it. No need for board ID getting method. This will save ROM space consumed by multiple APCB copies in the past.
This is a working example of an implementation tested on Dalboz. It does not require patching with APCB_EDIT_TOOL. All SPDs are assembled in one type 0x69 blob and inserted into PSP BIOS directory.
BUG=b:127505025 TEST=Dalboz with CABLRV20090300 and test CB and boot loader code BRANCH=Zork
Change-Id: I9adf7a016ee481214b47d8adc6d53fd8ce0c9bd4 Signed-off-by: Nikolai Vyssotski nikolai.vyssotski@amd.corp-partner.google.com --- M src/mainboard/google/zork/spd/Makefile.inc M src/soc/amd/picasso/Makefile.inc M util/amdfwtool/amdfwtool.c M util/apcb/apcb_edit.py 4 files changed, 104 insertions(+), 59 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/45278/5
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45278 )
Change subject: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/45278/5/src/mainboard/google/zork/s... File src/mainboard/google/zork/spd/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45278/5/src/mainboard/google/zork/s... PS5, Line 17: --hex \ The switch to binary has stalled. So --hex is still needed for now.
https://review.coreboot.org/c/coreboot/+/45278/5/src/mainboard/google/zork/s... PS5, Line 20: $(if $(APCB_POPULATE_2ND_CHANNEL), --chan_mask 5, --chan_mask 1) \ Where does chan_mask 5 come from?
https://review.coreboot.org/c/coreboot/+/45278/3/src/soc/amd/picasso/Makefil... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/45278/3/src/soc/amd/picasso/Makefil... PS3, Line 243: #ifdef SPD_SOURCES
Yes, unfortunately it is needed. […]
Does add_opt_prefix return "" when SPD_BLOB is undefined? This seems fragile. Maybe this ifdef should be around OPT_SPD_BLOB?
https://review.coreboot.org/c/coreboot/+/45278/5/util/apcb/apcb_edit.py File util/apcb/apcb_edit.py:
https://review.coreboot.org/c/coreboot/+/45278/5/util/apcb/apcb_edit.py@128 PS5, Line 128: # specifying which channels are actually populated (no need for dedicated GPIO84) This does not replace GPIO84. GPIO84 is needed on boards that support both 1 and 2 channel dram. On these boards both must be populated. Boards that never support 2 channels need to only populate 1 channel here because GPIO84 isn't set on these boards.
Nikolai Vyssotski has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45278 )
Change subject: WIP: Use SPDs from PSP BIOS table type 0x69 based on GPIO straps ......................................................................
Abandoned
-
Marshall Dawson (Code Review) -
Martin Roth (Code Review)
-
Nikolai Vyssotski (Code Review)
-
Rob Barnes (Code Review)