Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40700 )
Change subject: soc/amd/picasso: Use a helper to set bits in PSP_SOFTFUSE ......................................................................
soc/amd/picasso: Use a helper to set bits in PSP_SOFTFUSE
This change updates Makefile.inc to use a helper function set-bit to set a bit for the soft fuses. It gets rid of the different checks that were done to set soft fuses to magic values in different places.
This is still not the best way to handle the fuses and instead this logic should be embedded within the amdfwtool by making it aware of specific platforms. But until that happens, we want to avoid having to add PSP_SOFTFUSE setting in various places with different values.
BUG=b:154880818
Change-Id: I73887eb9c56ca5bb1c08d298fa818d698da1080b Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/amd/picasso/Makefile.inc 1 file changed, 12 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/40700/1
diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index 446198e..012ab8b 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -108,9 +108,8 @@
# type = 0x9 PSP_SEC_DBG_KEY_FILE=$(top)/$(FIRMWARE_LOCATE)/RavenSecureDebug_PublicKey.bin - -# type = 0xb - See #55758 (NDA) for bit definitions. -PSP_SOFTFUSE="0x0000000010000001" +# Enable secure debug unlock +PSP_SOFTFUSE_BITS += 0
ifeq ($(CONFIG_USE_PSPSCUREOS),y) # types = 0x2 @@ -134,7 +133,8 @@ # BIOS type = 0x6a PSP_MP2CFG_FILE=$(top)/$(FIRMWARE_LOCATE)/MP2FWConfig.sbin else -PSP_SOFTFUSE="0x0000000030000001" +# Disable MP2 firmware loading +PSP_SOFTFUSE_BITS += 29 endif
# type = 0x28 @@ -199,6 +199,14 @@ PSP_UCODE_FILE2=$(top)/$(FIRMWARE_LOCATE)/UcodePatch_PCO_B0.bin PSP_UCODE_FILE3=$(top)/$(FIRMWARE_LOCATE)/UcodePatch_RV2_A0.bin
+# type = 0xb - See #55758 (NDA) for bit definitions. +PSP_SOFTFUSE_BITS += 28 + +# 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, \ + $(foreach bit,$(PSP_SOFTFUSE_BITS),$(call set-bit,$(bit)))); printf "0x%x" $$A) + # # Build the arguments to amdfwtool (order is unimportant). Missing file names # result in empty OPT_ variables, i.e. the argument is not passed to amdfwtool.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40700 )
Change subject: soc/amd/picasso: Use a helper to set bits in PSP_SOFTFUSE ......................................................................
Patch Set 1:
I don't like this but until the amdfwtool is udpated, I don't see how this can be handled in a clean way.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40700 )
Change subject: soc/amd/picasso: Use a helper to set bits in PSP_SOFTFUSE ......................................................................
Patch Set 1: Code-Review+1
Was it tested?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40700 )
Change subject: soc/amd/picasso: Use a helper to set bits in PSP_SOFTFUSE ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+1
Was it tested?
Yes. Added TEST= to commit message.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Marshall Dawson, Angel Pons, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40700
to look at the new patch set (#2).
Change subject: soc/amd/picasso: Use a helper to set bits in PSP_SOFTFUSE ......................................................................
soc/amd/picasso: Use a helper to set bits in PSP_SOFTFUSE
This change updates Makefile.inc to use a helper function set-bit to set a bit for the soft fuses. It gets rid of the different checks that were done to set soft fuses to magic values in different places.
This is still not the best way to handle the fuses and instead this logic should be embedded within the amdfwtool by making it aware of specific platforms. But until that happens, we want to avoid having to add PSP_SOFTFUSE setting in various places with different values.
BUG=b:154880818 TEST=Verified that the softfuse values are same with and without this change.
Change-Id: I73887eb9c56ca5bb1c08d298fa818d698da1080b Signed-off-by: Furquan Shaikh furquan@google.com --- M src/soc/amd/picasso/Makefile.inc 1 file changed, 12 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/40700/2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40700 )
Change subject: soc/amd/picasso: Use a helper to set bits in PSP_SOFTFUSE ......................................................................
Patch Set 2: Code-Review+2
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40700 )
Change subject: soc/amd/picasso: Use a helper to set bits in PSP_SOFTFUSE ......................................................................
soc/amd/picasso: Use a helper to set bits in PSP_SOFTFUSE
This change updates Makefile.inc to use a helper function set-bit to set a bit for the soft fuses. It gets rid of the different checks that were done to set soft fuses to magic values in different places.
This is still not the best way to handle the fuses and instead this logic should be embedded within the amdfwtool by making it aware of specific platforms. But until that happens, we want to avoid having to add PSP_SOFTFUSE setting in various places with different values.
BUG=b:154880818 TEST=Verified that the softfuse values are same with and without this change.
Change-Id: I73887eb9c56ca5bb1c08d298fa818d698da1080b Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40700 Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/picasso/Makefile.inc 1 file changed, 12 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Marshall Dawson: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve
diff --git a/src/soc/amd/picasso/Makefile.inc b/src/soc/amd/picasso/Makefile.inc index 224202f..d7cf9c0 100644 --- a/src/soc/amd/picasso/Makefile.inc +++ b/src/soc/amd/picasso/Makefile.inc @@ -111,9 +111,8 @@
# type = 0x9 PSP_SEC_DBG_KEY_FILE=$(top)/$(FIRMWARE_LOCATE)/RavenSecureDebug_PublicKey.bin - -# type = 0xb - See #55758 (NDA) for bit definitions. -PSP_SOFTFUSE="0x0000000010000001" +# Enable secure debug unlock +PSP_SOFTFUSE_BITS += 0
ifeq ($(CONFIG_USE_PSPSCUREOS),y) # types = 0x2 @@ -137,7 +136,8 @@ # BIOS type = 0x6a PSP_MP2CFG_FILE=$(top)/$(FIRMWARE_LOCATE)/MP2FWConfig.sbin else -PSP_SOFTFUSE="0x0000000030000001" +# Disable MP2 firmware loading +PSP_SOFTFUSE_BITS += 29 endif
# type = 0x28 @@ -206,6 +206,14 @@ PSP_UCODE_FILE2=$(top)/$(FIRMWARE_LOCATE)/UcodePatch_PCO_B0.bin PSP_UCODE_FILE3=$(top)/$(FIRMWARE_LOCATE)/UcodePatch_RV2_A0.bin
+# type = 0xb - See #55758 (NDA) for bit definitions. +PSP_SOFTFUSE_BITS += 28 + +# 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, \ + $(foreach bit,$(PSP_SOFTFUSE_BITS),$(call set-bit,$(bit)))); printf "0x%x" $$A) + # # Build the arguments to amdfwtool (order is unimportant). Missing file names # result in empty OPT_ variables, i.e. the argument is not passed to amdfwtool.
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40700 )
Change subject: soc/amd/picasso: Use a helper to set bits in PSP_SOFTFUSE ......................................................................
Patch Set 4:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2858 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2857 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2856 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/2855
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Furquan Shaikh (Code Review)
-
Marshall Dawson (Code Review)