Hello Zheng Bao,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/48524
to review the following change.
Change subject: amdfwtool: Register APCB and APCB_BK respectively ......................................................................
amdfwtool: Register APCB and APCB_BK respectively
We took the assumption the APCB(0x60) and APCB_BK(0x68) are the same file. For picasso, they are. For later programe, they are not.
Change-Id: Idea7847691c2b511b489c306f04a8cb8945fd057 Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/soc/amd/picasso/Makefile.inc M util/amdfwtool/amdfwtool.c 2 files changed, 10 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/48524/1
diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index 7cbae92..962081c 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -188,7 +188,10 @@
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 )) + --instance $(shell printf "%x" $$(($(i)-1))) --apcb ) ) +OPT_PSP_APCB_FILES_BK=$(foreach i, $(shell seq $(words $(PSP_APCB_FILES))), \ + $(call add_opt_prefix, $(word $(i), $(PSP_APCB_FILES)), \ + --instance $(shell printf "1%x" $$(($(i)-1))) --apcb ) )
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) @@ -217,6 +220,7 @@ DEP_FILES= $(patsubst %,$(FIRMWARE_LOCATION)/%, $(shell sed -e /^$(POUND_SIGN)/d -e /^FIRMWARE_LOCATION/d $(CONFIG_AMDFW_CONFIG_FILE) | awk '{print $$2}' ))
AMDFW_COMMON_ARGS=$(OPT_PSP_APCB_FILES) \ + $(OPT_PSP_APCB_FILES_BK) \ $(OPT_APOB_ADDR) \ $(OPT_PSP_BIOSBIN_FILE) \ $(OPT_PSP_BIOSBIN_DEST) \ diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 36d669b..4edf7bc 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -1333,9 +1333,11 @@ sub = 0; break; case 'a': - register_bdt_data(AMD_BIOS_APCB, sub, instance, optarg); - register_bdt_data(AMD_BIOS_APCB_BK, sub, - instance, optarg); + if ((instance & 0xF0) == 0) + register_bdt_data(AMD_BIOS_APCB, sub, instance & 0xF, optarg); + else + register_bdt_data(AMD_BIOS_APCB_BK, sub, + instance & 0xF, optarg); sub = instance = 0; break; case 'Q':
Marshall Dawson has uploaded a new patch set (#3) to the change originally created by Bao Zheng. ( https://review.coreboot.org/c/coreboot/+/48524 )
Change subject: amdfwtool: Register APCB and APCB_BK respectively ......................................................................
amdfwtool: Register APCB and APCB_BK respectively
We took the assumption the APCB(0x60) and APCB_BK(0x68) are the same file. For picasso, they are. For later programe, they are not.
Change-Id: Idea7847691c2b511b489c306f04a8cb8945fd057 Signed-off-by: Zheng Bao fishbaozi@gmail.com --- M src/soc/amd/picasso/Makefile.inc M util/amdfwtool/amdfwtool.c 2 files changed, 10 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/48524/3
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48524 )
Change subject: amdfwtool: Register APCB and APCB_BK respectively ......................................................................
Patch Set 3: Code-Review+2
verified that a timeless build is identical for mandolin and that it still boots after this and the previous change
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48524 )
Change subject: amdfwtool: Register APCB and APCB_BK respectively ......................................................................
amdfwtool: Register APCB and APCB_BK respectively
We took the assumption the APCB(0x60) and APCB_BK(0x68) are the same file. For picasso, they are. For later programe, they are not.
Change-Id: Idea7847691c2b511b489c306f04a8cb8945fd057 Signed-off-by: Zheng Bao fishbaozi@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/48524 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/picasso/Makefile.inc M util/amdfwtool/amdfwtool.c 2 files changed, 10 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved
diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index 42f1ef5..a0f3ec4 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -184,7 +184,10 @@
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 )) + --instance $(shell printf "%x" $$(($(i)-1))) --apcb ) ) +OPT_PSP_APCB_FILES_BK=$(foreach i, $(shell seq $(words $(PSP_APCB_FILES))), \ + $(call add_opt_prefix, $(word $(i), $(PSP_APCB_FILES)), \ + --instance $(shell printf "1%x" $$(($(i)-1))) --apcb ) )
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) @@ -213,6 +216,7 @@ DEP_FILES= $(patsubst %,$(FIRMWARE_LOCATION)/%, $(shell sed -e /^$(POUND_SIGN)/d -e /^FIRMWARE_LOCATION/d $(CONFIG_AMDFW_CONFIG_FILE) | awk '{print $$2}' ))
AMDFW_COMMON_ARGS=$(OPT_PSP_APCB_FILES) \ + $(OPT_PSP_APCB_FILES_BK) \ $(OPT_APOB_ADDR) \ $(OPT_PSP_BIOSBIN_FILE) \ $(OPT_PSP_BIOSBIN_DEST) \ diff --git a/util/amdfwtool/amdfwtool.c b/util/amdfwtool/amdfwtool.c index 401cf20..5ae6d06 100644 --- a/util/amdfwtool/amdfwtool.c +++ b/util/amdfwtool/amdfwtool.c @@ -1342,9 +1342,11 @@ sub = 0; break; case 'a': - register_bdt_data(AMD_BIOS_APCB, sub, instance, optarg); - register_bdt_data(AMD_BIOS_APCB_BK, sub, - instance, optarg); + if ((instance & 0xF0) == 0) + register_bdt_data(AMD_BIOS_APCB, sub, instance & 0xF, optarg); + else + register_bdt_data(AMD_BIOS_APCB_BK, sub, + instance & 0xF, optarg); sub = instance = 0; break; case 'Q':
-
Bao Zheng (Code Review) -
Felix Held (Code Review)
-
Marshall Dawson (Code Review)