Hello Marshall Dawson, Richard Spiegel,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/38044
to review the following change.
Change subject: amd_blobs: Always set default paths ......................................................................
amd_blobs: Always set default paths
Don't make the default paths to AMD blobs depend on USE_AMD_BLOBS. This way we get error messages about the missing files when the blobs repos aren't checked out.
Change-Id: I754fdc5e1414c8a3dc88b364bcfbea9a26b59eb0 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/soc/amd/stoneyridge/Kconfig M src/vendorcode/amd/pi/Kconfig 2 files changed, 5 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/38044/1
diff --git a/src/soc/amd/stoneyridge/Kconfig b/src/soc/amd/stoneyridge/Kconfig index a03b8f3..cc7354f 100644 --- a/src/soc/amd/stoneyridge/Kconfig +++ b/src/soc/amd/stoneyridge/Kconfig @@ -155,7 +155,6 @@
config VGA_BIOS_FILE string - default "" if !USE_AMD_BLOBS default "3rdparty/amd_blobs/stoneyridge/CarrizoGenericVbios.bin" if AMD_APU_MERLINFALCON default "3rdparty/amd_blobs/stoneyridge/StoneyGenericVbios.bin" if AMD_APU_PRAIRIEFALCON default "3rdparty/amd_blobs/stoneyridge/StoneyGenericVbios.bin" if AMD_APU_STONEYRIDGE @@ -197,7 +196,6 @@
config STONEYRIDGE_XHCI_FWM_FILE string "XHCI firmware path and filename" - default "" if !USE_AMD_BLOBS default "3rdparty/amd_blobs/stoneyridge/xhci.bin" depends on STONEYRIDGE_XHCI_FWM
@@ -207,7 +205,6 @@
config AMD_PUBKEY_FILE string "AMD public Key" - default "" if !USE_AMD_BLOBS default "3rdparty/amd_blobs/stoneyridge/PSP/CZ/AmdPubKeyCZ.bin" if AMD_APU_MERLINFALCON default "3rdparty/amd_blobs/stoneyridge/PSP/ST/AmdPubKeyST.bin" if AMD_APU_PRAIRIEFALCON default "3rdparty/amd_blobs/stoneyridge/PSP/ST/AmdPubKeyST.bin" if AMD_APU_STONEYRIDGE diff --git a/src/vendorcode/amd/pi/Kconfig b/src/vendorcode/amd/pi/Kconfig index 9dcdf34..ee2958a 100644 --- a/src/vendorcode/amd/pi/Kconfig +++ b/src/vendorcode/amd/pi/Kconfig @@ -43,9 +43,9 @@ string "AGESA PI binary file name" default "3rdparty/blobs/pi/amd/00630F01/FP3/AGESA.bin" if CPU_AMD_PI_00630F01 default "3rdparty/blobs/pi/amd/00730F01/FT3b/AGESA.bin" if CPU_AMD_PI_00730F01 - default "3rdparty/amd_blobs/stoneyridge/pi/CZ/$(CONFIG_AMD_SOC_PACKAGE)/AGESA.bin" if AMD_APU_MERLINFALCON && USE_AMD_BLOBS - default "3rdparty/amd_blobs/stoneyridge/pi/ST/$(CONFIG_AMD_SOC_PACKAGE)/AGESA.bin" if AMD_APU_PRAIRIEFALCON && USE_AMD_BLOBS - default "3rdparty/amd_blobs/stoneyridge/pi/ST/$(CONFIG_AMD_SOC_PACKAGE)/AGESA.bin" if AMD_APU_STONEYRIDGE && USE_AMD_BLOBS + default "3rdparty/amd_blobs/stoneyridge/pi/CZ/$(CONFIG_AMD_SOC_PACKAGE)/AGESA.bin" if AMD_APU_MERLINFALCON + default "3rdparty/amd_blobs/stoneyridge/pi/ST/$(CONFIG_AMD_SOC_PACKAGE)/AGESA.bin" if AMD_APU_PRAIRIEFALCON + default "3rdparty/amd_blobs/stoneyridge/pi/ST/$(CONFIG_AMD_SOC_PACKAGE)/AGESA.bin" if AMD_APU_STONEYRIDGE default "3rdparty/blobs/pi/amd/00660F01/FP4/AGESA.bin" if CPU_AMD_PI_00660F01 help Specify the binary file to use for AMD platform initialization. @@ -70,7 +70,7 @@ config AGESA_PRE_MEMORY_BINARY_PI_FILE string "Pre memory Binary PI file name" depends on AGESA_SPLIT_MEMORY_FILES - default "3rdparty/blobs/pi/amd/00670F00/FT4/AGESA_premem.elf" if SOC_AMD_STONEYRIDGE && USE_AMD_BLOBS + default "3rdparty/blobs/pi/amd/00670F00/FT4/AGESA_premem.elf" if SOC_AMD_STONEYRIDGE # FIXME: File doesn't exist help Specify the binary file to use for pre-memory AMD platform initialization. @@ -78,7 +78,7 @@ config AGESA_POST_MEMORY_BINARY_PI_FILE string "Post memory Binary PI file name" depends on AGESA_SPLIT_MEMORY_FILES - default "3rdparty/blobs/pi/amd/00670F00/FT4/AGESA_postmem.elf" if SOC_AMD_STONEYRIDGE && USE_AMD_BLOBS + default "3rdparty/blobs/pi/amd/00670F00/FT4/AGESA_postmem.elf" if SOC_AMD_STONEYRIDGE # FIXME: File doesn't exist help Specify the binary file to use for post-memory AMD platform initialization.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38044 )
Change subject: amd_blobs: Always set default paths ......................................................................
Patch Set 1:
Maybe I don't quite understand the intent behind this. Are you saying you want to force a build error when !USE_AMD_BLOBS && !(user setting CONFIG_ path strings)? Right now the Makefile.inc interprets empty strings as absence of the blobs and builds/runs OK with warning messages. My initial impression is that I would prefer it stay as-is.
AFAICS, the abuild problem should be solved without this change.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38044 )
Change subject: amd_blobs: Always set default paths ......................................................................
Patch Set 1: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38044 )
Change subject: amd_blobs: Always set default paths ......................................................................
Patch Set 1:
Maybe I don't quite understand the intent behind this. Are you saying you want to force a build error when !USE_AMD_BLOBS && !(user setting CONFIG_ path strings)? Right now the Makefile.inc interprets empty strings as absence of the blobs and builds/runs OK with warning messages. My initial impression is that I would prefer it stay as-is.
Right, that's exactly the two sides of the coin. And I expected some discussion here :)
For other targets, we decided to have build errors instead of warnings if a build is likely to brick when flashed (only likely, because one can add files to CBFS afterwards, but who does that?).
I have no strong feelings about it, just wanted to bring it up. If somebody prefers to add the files post-make, I'm ok with the current defaults. But if not, I see no reason not to force an error.
AFAICS, the abuild problem should be solved without this change.
Yep.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38044 )
Change subject: amd_blobs: Always set default paths ......................................................................
Patch Set 1:
And I expected some discussion here :) ... I have no strong feelings about it...
I'm having trouble deciding how much I really care about it. The only corner case I can think of is sort of ridiculous, i.e. if someone doesn't agree to AMD's simplified license but has functional blobs and rights to use or distribute them in a finished product. Of course, for bootleg copies or images harvested from a shipping image, I really don't care within the context of this discussion.
If I compare against Intel, FSP systems (I only looked at APL...) it won't build without the include files in 3rdparty/fsp but the path can be modified with menuconfig. Hmm, looks like CONFIG_ADD_FSP_BINARIES isn't set by default, and I didn't see a warning about that.
Anyway, I wonder if the aim should be to attempt to build anything where coreboot.org has vendors' assets available for download. Then for anything we're not allowed to redistribute, like Intel Flash Descriptor, we give an option to add it if the user possesses it; if they don't, build anyway but with a scary warning.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38044 )
Change subject: amd_blobs: Always set default paths ......................................................................
Patch Set 1:
And I expected some discussion here :) ... I have no strong feelings about it...
I'm having trouble deciding how much I really care about it. The only corner case I can think of is sort of ridiculous, i.e. if someone doesn't agree to AMD's simplified license but has functional blobs and rights to use or distribute them in a finished product. Of course, for bootleg copies or images harvested from a shipping image, I really don't care within the context of this discussion.
Even such case would still be possible, we'd only change defaults, without reducing flexibility.
If I compare against Intel, FSP systems (I only looked at APL...) it won't build without the include files in 3rdparty/fsp but the path can be modified with menuconfig. Hmm, looks like CONFIG_ADD_FSP_BINARIES isn't set by default, and I didn't see a warning about that.
Ah, yes I forgot, binary hook-up with FSP is still a mess. What I described should be true for older Intel targets with MRC binary (where available) and the microcode updates.
Anyway, I wonder if the aim should be to attempt to build anything where coreboot.org has vendors' assets available for download. Then for anything we're not allowed to redistribute, like Intel Flash Descriptor, we give an option to add it if the user possesses it; if they don't, build anyway but with a scary warning.
Ack. That's what I'm aim at.
(The IFD isn't the best example though, it's merely configuration data. That we don't check it in is mostly because it's not directly coreboot related and not needed: ODM/OEM should have it anyway and their customers get it with the hardware.)
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38044 )
Change subject: amd_blobs: Always set default paths ......................................................................
Patch Set 1:
I want to get this change in. Let me bump it here so that I'll remember to check it one more time the next time I look at Picasso.
Marshall Dawson has uploaded a new patch set (#2) to the change originally created by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/38044 )
Change subject: amd_blobs: Always set default paths ......................................................................
amd_blobs: Always set default paths
Don't make the default paths to AMD blobs depend on USE_AMD_BLOBS. This way we get error messages about the missing files when the blobs repos aren't checked out.
Change-Id: I754fdc5e1414c8a3dc88b364bcfbea9a26b59eb0 Signed-off-by: Nico Huber nico.h@gmx.de --- M src/soc/amd/stoneyridge/Kconfig M src/vendorcode/amd/pi/Kconfig 2 files changed, 5 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/38044/2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38044 )
Change subject: amd_blobs: Always set default paths ......................................................................
Patch Set 2:
PS2 is a manual rebase. Looks to me like we're good now.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38044 )
Change subject: amd_blobs: Always set default paths ......................................................................
Patch Set 2: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38044 )
Change subject: amd_blobs: Always set default paths ......................................................................
amd_blobs: Always set default paths
Don't make the default paths to AMD blobs depend on USE_AMD_BLOBS. This way we get error messages about the missing files when the blobs repos aren't checked out.
Change-Id: I754fdc5e1414c8a3dc88b364bcfbea9a26b59eb0 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/38044 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/stoneyridge/Kconfig M src/vendorcode/amd/pi/Kconfig 2 files changed, 5 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved
diff --git a/src/soc/amd/stoneyridge/Kconfig b/src/soc/amd/stoneyridge/Kconfig index d672726..e49b3db 100644 --- a/src/soc/amd/stoneyridge/Kconfig +++ b/src/soc/amd/stoneyridge/Kconfig @@ -141,7 +141,6 @@
config VGA_BIOS_FILE string - default "" if !USE_AMD_BLOBS default "3rdparty/amd_blobs/stoneyridge/CarrizoGenericVbios.bin" if AMD_APU_MERLINFALCON default "3rdparty/amd_blobs/stoneyridge/StoneyGenericVbios.bin" if AMD_APU_PRAIRIEFALCON default "3rdparty/amd_blobs/stoneyridge/StoneyGenericVbios.bin" if AMD_APU_STONEYRIDGE @@ -183,7 +182,6 @@
config STONEYRIDGE_XHCI_FWM_FILE string "XHCI firmware path and filename" - default "" if !USE_AMD_BLOBS default "3rdparty/amd_blobs/stoneyridge/xhci.bin" depends on STONEYRIDGE_XHCI_FWM
@@ -194,7 +192,6 @@ config AMDFW_CONFIG_FILE string string "AMD PSP Firmware config file" - default "" if !USE_AMD_BLOBS default "src/soc/amd/stoneyridge/fw_cz.cfg" if AMD_APU_MERLINFALCON default "src/soc/amd/stoneyridge/fw_st.cfg" if AMD_APU_PRAIRIEFALCON default "src/soc/amd/stoneyridge/fw_st.cfg" if AMD_APU_STONEYRIDGE diff --git a/src/vendorcode/amd/pi/Kconfig b/src/vendorcode/amd/pi/Kconfig index fb4935b..598c364 100644 --- a/src/vendorcode/amd/pi/Kconfig +++ b/src/vendorcode/amd/pi/Kconfig @@ -42,9 +42,9 @@ string "AGESA PI binary file name" default "3rdparty/blobs/pi/amd/00630F01/FP3/AGESA.bin" if CPU_AMD_PI_00630F01 default "3rdparty/blobs/pi/amd/00730F01/FT3b/AGESA.bin" if CPU_AMD_PI_00730F01 - default "3rdparty/amd_blobs/stoneyridge/pi/CZ/$(CONFIG_AMD_SOC_PACKAGE)/AGESA.bin" if AMD_APU_MERLINFALCON && USE_AMD_BLOBS - default "3rdparty/amd_blobs/stoneyridge/pi/ST/$(CONFIG_AMD_SOC_PACKAGE)/AGESA.bin" if AMD_APU_PRAIRIEFALCON && USE_AMD_BLOBS - default "3rdparty/amd_blobs/stoneyridge/pi/ST/$(CONFIG_AMD_SOC_PACKAGE)/AGESA.bin" if AMD_APU_STONEYRIDGE && USE_AMD_BLOBS + default "3rdparty/amd_blobs/stoneyridge/pi/CZ/$(CONFIG_AMD_SOC_PACKAGE)/AGESA.bin" if AMD_APU_MERLINFALCON + default "3rdparty/amd_blobs/stoneyridge/pi/ST/$(CONFIG_AMD_SOC_PACKAGE)/AGESA.bin" if AMD_APU_PRAIRIEFALCON + default "3rdparty/amd_blobs/stoneyridge/pi/ST/$(CONFIG_AMD_SOC_PACKAGE)/AGESA.bin" if AMD_APU_STONEYRIDGE help Specify the binary file to use for AMD platform initialization.
@@ -68,7 +68,7 @@ config AGESA_PRE_MEMORY_BINARY_PI_FILE string "Pre memory Binary PI file name" depends on AGESA_SPLIT_MEMORY_FILES - default "3rdparty/blobs/pi/amd/00670F00/FT4/AGESA_premem.elf" if SOC_AMD_STONEYRIDGE && USE_AMD_BLOBS + default "3rdparty/blobs/pi/amd/00670F00/FT4/AGESA_premem.elf" if SOC_AMD_STONEYRIDGE help Specify the binary file to use for pre-memory AMD platform initialization. @@ -76,7 +76,7 @@ config AGESA_POST_MEMORY_BINARY_PI_FILE string "Post memory Binary PI file name" depends on AGESA_SPLIT_MEMORY_FILES - default "3rdparty/blobs/pi/amd/00670F00/FT4/AGESA_postmem.elf" if SOC_AMD_STONEYRIDGE && USE_AMD_BLOBS + default "3rdparty/blobs/pi/amd/00670F00/FT4/AGESA_postmem.elf" if SOC_AMD_STONEYRIDGE help Specify the binary file to use for post-memory AMD platform initialization.
-
Felix Held (Code Review) -
Marshall Dawson (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)