Change in coreboot[master]: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit
Richard Spiegel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit
Current code surreptitiously selected Prairie Falcon if Merlin Falcon is selected and no Merlin Falcon binaries are available. This is not optimal, make the selection an explicit choice.
Main differences between Stoneyridge, Merlin Falcon and Prairie Falcon are related to number of memory channels, binaries used and if PSP is capable of dual load (combo capable). Though Prairie Falcon internally uses the same CPU (and thus same binaries) as Stoneyridge, only Stoneyridge is combo capable.
Merlin Falcon uses the same internal CPU as Carrizo, thus it has dual memory channel while Prairie Falcon and Stoney ridge have only one channel. However, only one particular version of Carrizo binaries were tested and worked for Merlin Falcon, so for the time being it must use its own binary image (not all Carrizo images work).
BUG=None TEST=Build padmelon with Prarie Falcon and Merlin Falcon
Change-Id: I309c5918fdc98d9927641466bbe6149b97b250f0 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/stoneyridge/Kconfig M src/soc/amd/stoneyridge/Makefile.inc M src/vendorcode/amd/pi/00670F00/Makefile.inc M src/vendorcode/amd/pi/Kconfig 4 files changed, 15 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/36823/1
diff --git a/src/soc/amd/stoneyridge/Kconfig b/src/soc/amd/stoneyridge/Kconfig index 0d6f2ff..965ca5d 100644 --- a/src/soc/amd/stoneyridge/Kconfig +++ b/src/soc/amd/stoneyridge/Kconfig @@ -28,6 +28,11 @@ help AMD Merlin Falcon FP4 support
+config SOC_AMD_PRAIRIEFALCON + bool + help + AMD Prairie Falcon FP4 support + config HAVE_MERLINFALCON_BINARIES depends on SOC_AMD_MERLINFALCON bool "Merlinfalcon binaries are present" @@ -36,7 +41,7 @@ This config option will be removed once the binaries are merged to the blobs repo. See 33615.
-if SOC_AMD_STONEYRIDGE_FP4 || SOC_AMD_STONEYRIDGE_FT4 || SOC_AMD_MERLINFALCON +if SOC_AMD_STONEYRIDGE_FP4 || SOC_AMD_STONEYRIDGE_FT4 || SOC_AMD_MERLINFALCON || SOC_AMD_PRAIRIEFALCON
config CPU_SPECIFIC_OPTIONS def_bool y @@ -315,7 +320,7 @@
config SOC_AMD_PSP_SELECTABLE_SMU_FW bool - default n if SOC_AMD_MERLINFALCON + default n if (SOC_AMD_MERLINFALCON || SOC_AMD_PRAIRIEFALCON) default y help Some ST implementations allow storing SMU firmware into cbfs and @@ -402,4 +407,4 @@ return to S0. Otherwise the system will remain in S5 once power is restored.
-endif # SOC_AMD_STONEYRIDGE_FP4 || SOC_AMD_STONEYRIDGE_FT4 || SOC_AMD_MERLINFALCON +endif # SOC_AMD_STONEYRIDGE_FP4 || SOC_AMD_STONEYRIDGE_FT4 || SOC_AMD_MERLINFALCON || SOC_AMD_PRAIRIEFALCON diff --git a/src/soc/amd/stoneyridge/Makefile.inc b/src/soc/amd/stoneyridge/Makefile.inc index 53aabf5..0866425 100644 --- a/src/soc/amd/stoneyridge/Makefile.inc +++ b/src/soc/amd/stoneyridge/Makefile.inc @@ -27,7 +27,7 @@ # SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. # #***************************************************************************** -ifeq ($(CONFIG_SOC_AMD_MERLINFALCON)$(CONFIG_SOC_AMD_STONEYRIDGE_FP4)$(CONFIG_SOC_AMD_STONEYRIDGE_FT4),y) +ifeq ($(CONFIG_SOC_AMD_PRAIRIEFALCON)$(CONFIG_SOC_AMD_MERLINFALCON)$(CONFIG_SOC_AMD_STONEYRIDGE_FP4)$(CONFIG_SOC_AMD_STONEYRIDGE_FT4),y)
subdirs-y += ../../../cpu/amd/mtrr/ subdirs-y += ../../../cpu/x86/tsc @@ -218,7 +218,7 @@ OPT_SMUFWM_FN_FILE=$(call add_opt_prefix, $(SMUFWM_FN_FILE), --subprogram $(SUBPROG_FN_SMU_FW) --smufirmware) OPT_SMUFIRMWARE2_FN_FILE=$(call add_opt_prefix, $(SMUFIRMWARE2_FN_FILE), --subprogram $(SUBPROG_FN_SMU_FW) --smufirmware2)
-ifeq ($(FIRMWARE_TYPE),ST) +ifeq ($(CONFIG_SOC_AMD_STONEYRIDGE_FP4)$(CONFIG_SOC_AMD_STONEYRIDGE_FT4),y) OPT_COMBOCAPABLE=--combo-capable endif
@@ -320,4 +320,4 @@
endif # ifeq ($(CONFIG_SOC_AMD_PSP_SELECTABLE_SMU_FW),y)
-endif # ($(CONFIG_SOC_AMD_MERLINFALCON)$(CONFIG_SOC_AMD_STONEYRIDGE_FP4)$(CONFIG_SOC_AMD_STONEYRIDGE_FT4),y) +endif # ($(CONFIG_SOC_AMD_PRAIRIEFALCON)$(CONFIG_SOC_AMD_MERLINFALCON)$(CONFIG_SOC_AMD_STONEYRIDGE_FP4)$(CONFIG_SOC_AMD_STONEYRIDGE_FT4),y) diff --git a/src/vendorcode/amd/pi/00670F00/Makefile.inc b/src/vendorcode/amd/pi/00670F00/Makefile.inc index fef7dff..6f52916 100644 --- a/src/vendorcode/amd/pi/00670F00/Makefile.inc +++ b/src/vendorcode/amd/pi/00670F00/Makefile.inc @@ -28,7 +28,7 @@ # #*****************************************************************************
-ifeq ($(CONFIG_SOC_AMD_MERLINFALCON)$(CONFIG_SOC_AMD_STONEYRIDGE_FP4)$(CONFIG_SOC_AMD_STONEYRIDGE_FT4),y) +ifeq ($(CONFIG_SOC_AMD_PRAIRIEFALCON)$(CONFIG_SOC_AMD_MERLINFALCON)$(CONFIG_SOC_AMD_STONEYRIDGE_FP4)$(CONFIG_SOC_AMD_STONEYRIDGE_FT4),y) # AGESA V5 Files
AGESA_ROOT = $(call strip_quotes,$(CONFIG_AGESA_BINARY_PI_VENDORCODE_PATH)) diff --git a/src/vendorcode/amd/pi/Kconfig b/src/vendorcode/amd/pi/Kconfig index 08e7cc6..c21360b 100644 --- a/src/vendorcode/amd/pi/Kconfig +++ b/src/vendorcode/amd/pi/Kconfig @@ -26,13 +26,14 @@ # SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #
-if CPU_AMD_PI_00630F01 || CPU_AMD_PI_00730F01 || CPU_AMD_PI_00660F01 || SOC_AMD_STONEYRIDGE_FP4 || SOC_AMD_STONEYRIDGE_FT4 || SOC_AMD_MERLINFALCON +if CPU_AMD_PI_00630F01 || CPU_AMD_PI_00730F01 || CPU_AMD_PI_00660F01 || SOC_AMD_STONEYRIDGE_FP4 || SOC_AMD_STONEYRIDGE_FT4 || SOC_AMD_MERLINFALCON || SOC_AMD_PRAIRIEFALCON
config AGESA_BINARY_PI_VENDORCODE_PATH string "AGESA PI directory path" default "src/vendorcode/amd/pi/00630F01" if CPU_AMD_PI_00630F01 default "src/vendorcode/amd/pi/00730F01" if CPU_AMD_PI_00730F01 default "src/vendorcode/amd/pi/00670F00" if SOC_AMD_MERLINFALCON + default "src/vendorcode/amd/pi/00670F00" if SOC_AMD_PRAIRIEFALCON default "src/vendorcode/amd/pi/00670F00" if SOC_AMD_STONEYRIDGE_FP4 default "src/vendorcode/amd/pi/00670F00" if SOC_AMD_STONEYRIDGE_FT4 default "src/vendorcode/amd/pi/00660F01" if CPU_AMD_PI_00660F01 @@ -46,6 +47,7 @@ default "3rdparty/blobs/pi/amd/00730F01/FT3b/AGESA.bin" if CPU_AMD_PI_00730F01 default "3rdparty/blobs/pi/amd/merlinfalcon/FP4/AGESA_CZ_FP4.bin" if SOC_AMD_MERLINFALCON && HAVE_MERLINFALCON_BINARIES default "3rdparty/blobs/pi/amd/00670F00/FP4/AGESA.bin" if SOC_AMD_MERLINFALCON && !HAVE_MERLINFALCON_BINARIES + default "3rdparty/blobs/pi/amd/00670F00/FP4/AGESA.bin" if SOC_AMD_PRAIRIEFALCON default "3rdparty/blobs/pi/amd/00670F00/FP4/AGESA.bin" if SOC_AMD_STONEYRIDGE_FP4 default "3rdparty/blobs/pi/amd/00670F00/FT4/AGESA.bin" if SOC_AMD_STONEYRIDGE_FT4 default "3rdparty/blobs/pi/amd/00660F01/FP4/AGESA.bin" if CPU_AMD_PI_00660F01
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36823/1/src/soc/amd/stoneyridge/Mak... File src/soc/amd/stoneyridge/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36823/1/src/soc/amd/stoneyridge/Mak... PS1, Line 140: ifeq ($(CONFIG_HAVE_MERLINFALCON_BINARIES),y) This section might need expansion, as you could declare Prairie Falcon and have Merlin Falcon binaries. It's also important for the future when "have binaries" becomes not important. Will add change once first code review happens.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@9 PS1, Line 9: Current code surreptitiously selected Prairie Falcon if Merlin Falcon is : selected and no Merlin Falcon binaries are available. I don't understand what you mean about Prairie Falcon being selected - you're adding it to this commit. From your changes, it sort of looks like Stoney Ridge blobs may have been used in Makefile.inc.
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@16 PS1, Line 16: only Stoneyridge is combo capable Can you elaborate why are you confident that Prairie Falcon is not combo-capable? That product is Family 15h Models 70h-7Fh, just like Stoney Ridge, and according to the PSP spec, it looks like these should support combo.
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@18 PS1, Line 18: Merlin Falcon uses the same internal CPU as Carrizo, thus it has dual memory : channel while Prairie Falcon and Stoney ridge have only one channel. This really has nothing to do with the PSP blobs, and is more of a feature definition for the APU type for a given package. I would remove it.
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@20 PS1, Line 20: one particular version of Carrizo binaries Not sure what you mean by version(s) of Carrizo binaries. Are you talking about the Bristol Ridge products?
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@9 PS1, Line 9: Current code surreptitiously selected Prairie Falcon if Merlin Falcon is : selected and no Merlin Falcon binaries are available.
I don't understand what you mean about Prairie Falcon being selected - you're adding it to this comm […]
If you build padmelon using Merlin Falcon, but without Merlin Falcon binaries, the resulting image will boot a padmelon using Prairie Falcon. I wanted to do it more explicit, as only I knew the fact...
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@16 PS1, Line 16: only Stoneyridge is combo capable
Can you elaborate why are you confident that Prairie Falcon is not combo-capable? That product is F […]
Combo capable was explicitly developed for Google. Google never used Prairie Falcon, thus I assume that its base code (permanently programmed to it, which will then load external code from SPI) never got such capability... I might be wrong, it may be possible to add the capability to one such external code.
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@18 PS1, Line 18: Merlin Falcon uses the same internal CPU as Carrizo, thus it has dual memory : channel while Prairie Falcon and Stoney ridge have only one channel.
This really has nothing to do with the PSP blobs, and is more of a feature definition for the APU ty […]
I just wanted to differentiate between Merlin Falcon and Prairie Falcon. It's the only difference I'm aware between these 2 SOCs. If you want, I can always remove, but I believe it's a valid comparison.
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@20 PS1, Line 20: one particular version of Carrizo binaries
Not sure what you mean by version(s) of Carrizo binaries. […]
Yes...Maybe I should have said Bristol Ridge instead of Carrizo. Not sure why, but some BR binaries I tried did not work with Merlin Falcon (though IIRC the latest one did). Will replace with Bristol Ridge.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 1:
(2 comments)
It looks like the UEFI image puts the two Models 60h-6Fh in a table pointed to by offset 0x10 and the 70h-7Fh in a separate table pointed to by offset 0x14. The 2 older ones include both, not either-or. They use the submodel to distinguish between the two (same as RV2 and PCO in soc//picasso). While we don't currently build two separate tables, I'm starting to think we should consider making the amdfwtool arguments more overt instead of one-size-fits-all.
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@16 PS1, Line 16: only Stoneyridge is combo capable To be more clear, we never build combo directories. It's that the 0x14th location points to a directory table that may be tradition or it may be combo. We use the --combo-capable argument to simply say use offset 0x14 for the pointer, not the older 0x10.
Combo capable was explicitly developed for Google.
Sorry, but the phone range before I hit Send on the comment above. But no, Google is not part of this topic in any way. I have no idea what you're confusing this with. The ability to load SMU firmware on the fly?
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@18 PS1, Line 18: Merlin Falcon uses the same internal CPU as Carrizo, thus it has dual memory : channel while Prairie Falcon and Stoney ridge have only one channel.
I just wanted to differentiate between Merlin Falcon and Prairie Falcon. […]
It's not that the comparison is invalid. It's that it's not relevant to the patch and will likely confuse the reader if they try to fully understand the purpose of your changes in the source code, which is only about building in the incorrect binaries on a per-product basis. That has nothing to do with number of channels, etc.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@16 PS1, Line 16: only Stoneyridge is combo capable
To be more clear, we never build combo directories. […]
So I confused some... yes SMU firmware. However, that's how I changed the code, with only stoney being combo capable , not enabling it for Prairie Falcon. Was I wrong, and thus should enable it also for Prairie Falcon?
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@18 PS1, Line 18: Merlin Falcon uses the same internal CPU as Carrizo, thus it has dual memory : channel while Prairie Falcon and Stoney ridge have only one channel.
It's not that the comparison is invalid. […]
Will remove
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@18 PS1, Line 18: Merlin Falcon uses the same internal CPU as Carrizo, thus it has dual memory : channel while Prairie Falcon and Stoney ridge have only one channel.
Will remove
Done
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@20 PS1, Line 20: one particular version of Carrizo binaries
Yes...Maybe I should have said Bristol Ridge instead of Carrizo. […]
Done
https://review.coreboot.org/c/coreboot/+/36823/1/src/soc/amd/stoneyridge/Mak... File src/soc/amd/stoneyridge/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36823/1/src/soc/amd/stoneyridge/Mak... PS1, Line 140: ifeq ($(CONFIG_HAVE_MERLINFALCON_BINARIES),y)
This section might need expansion, as you could declare Prairie Falcon and have Merlin Falcon binari […]
Done
Hello Marshall Dawson, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36823
to look at the new patch set (#2).
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit
Current code surreptitiously selected Prairie Falcon if Merlin Falcon is selected and no Merlin Falcon binaries are available. This is not optimal, make the selection an explicit choice.
Main differences between Stoneyridge, Merlin Falcon and Prairie Falcon are related to number of memory channels, binaries used and if PSP is capable of dual load (combo capable). Though Prairie Falcon internally uses the same CPU (and thus same binaries) as Stoneyridge, only Stoneyridge is combo capable.
Only one particular version of Bristol Ridge binaries were tested and worked for Merlin Falcon, so for the time being it must use its own binary image (not all Bristol Ridge images work).
BUG=None TEST=Build padmelon with Prarie Falcon and Merlin Falcon
Change-Id: I309c5918fdc98d9927641466bbe6149b97b250f0 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/stoneyridge/Kconfig M src/soc/amd/stoneyridge/Makefile.inc M src/vendorcode/amd/pi/00670F00/Makefile.inc M src/vendorcode/amd/pi/Kconfig 4 files changed, 16 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/36823/2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@16 PS1, Line 16: only Stoneyridge is combo capable
So I confused some... yes SMU firmware. […]
The spec says Stoney and PF should support using either 0x10 or 0x14. The other two must use only 0x10.
https://review.coreboot.org/c/coreboot/+/36823/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36823/2//COMMIT_MSG@13 PS2, Line 13: Stoneyridge Use "Stoney Ridge" when using the code name.
https://review.coreboot.org/c/coreboot/+/36823/2//COMMIT_MSG@16 PS2, Line 16: only Stoneyridge is combo capable. I still believe this sounds incorrect.
https://review.coreboot.org/c/coreboot/+/36823/2/src/soc/amd/stoneyridge/Mak... File src/soc/amd/stoneyridge/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36823/2/src/soc/amd/stoneyridge/Mak... PS2, Line 140: && ($(CONFIG_HAVE_MERLINFALCON_BINARIES),y)) Need to nest it
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@16 PS1, Line 16: only Stoneyridge is combo capable
The spec says Stoney and PF should support using either 0x10 or 0x14. […]
Ok, so I'll fix code and here.
https://review.coreboot.org/c/coreboot/+/36823/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36823/2//COMMIT_MSG@13 PS2, Line 13: Stoneyridge
Use "Stoney Ridge" when using the code name.
Will do.
https://review.coreboot.org/c/coreboot/+/36823/2//COMMIT_MSG@16 PS2, Line 16: only Stoneyridge is combo capable.
I still believe this sounds incorrect.
Will fix.
https://review.coreboot.org/c/coreboot/+/36823/2/src/soc/amd/stoneyridge/Mak... File src/soc/amd/stoneyridge/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36823/2/src/soc/amd/stoneyridge/Mak... PS2, Line 140: && ($(CONFIG_HAVE_MERLINFALCON_BINARIES),y))
Need to nest it
How? ifeq ($(CONFIG_SOC_AMD_MERLINFALCON)$(CONFIG_HAVE_MERLINFALCON_BINARIES),y) Does not look correct, just one y would trigger it, I want both.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36823/1//COMMIT_MSG@16 PS1, Line 16: only Stoneyridge is combo capable
Ok, so I'll fix code and here.
Done
https://review.coreboot.org/c/coreboot/+/36823/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36823/2//COMMIT_MSG@13 PS2, Line 13: Stoneyridge
Will do.
Done
https://review.coreboot.org/c/coreboot/+/36823/2//COMMIT_MSG@16 PS2, Line 16: only Stoneyridge is combo capable.
Will fix.
Done
Hello Marshall Dawson, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36823
to look at the new patch set (#3).
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit
Current code surreptitiously selected Prairie Falcon if Merlin Falcon is selected and no Merlin Falcon binaries are available. This is not optimal, make the selection an explicit choice.
Main differences between Stoney Ridge, Merlin Falcon and Prairie Falcon are related to number of memory channels, binaries used and if PSP is capable of dual load (combo capable).
Only one particular version of Bristol Ridge binaries were tested and worked for Merlin Falcon, so for the time being it must use its own binary image (not all Bristol Ridge images work).
BUG=None TEST=Build padmelon with Prarie Falcon and Merlin Falcon
Change-Id: I309c5918fdc98d9927641466bbe6149b97b250f0 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/stoneyridge/Kconfig M src/soc/amd/stoneyridge/Makefile.inc M src/vendorcode/amd/pi/00670F00/Makefile.inc M src/vendorcode/amd/pi/Kconfig 4 files changed, 15 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/36823/3
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36823/2/src/soc/amd/stoneyridge/Mak... File src/soc/amd/stoneyridge/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36823/2/src/soc/amd/stoneyridge/Mak... PS2, Line 140: && ($(CONFIG_HAVE_MERLINFALCON_BINARIES),y))
How? […]
I guess I figured out. Ugly though.
Hello Marshall Dawson, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36823
to look at the new patch set (#4).
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit
Current code surreptitiously selected Prairie Falcon if Merlin Falcon is selected and no Merlin Falcon binaries are available. This is not optimal, make the selection an explicit choice.
Main differences between Stoney Ridge, Merlin Falcon and Prairie Falcon are related to number of memory channels, binaries used and if PSP is capable of dual load (combo capable).
Only one particular version of Bristol Ridge binaries were tested and worked for Merlin Falcon, so for the time being it must use its own binary image (not all Bristol Ridge images work).
BUG=None TEST=Build padmelon with Prarie Falcon and Merlin Falcon
Change-Id: I309c5918fdc98d9927641466bbe6149b97b250f0 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/stoneyridge/Kconfig M src/soc/amd/stoneyridge/Makefile.inc M src/vendorcode/amd/pi/00670F00/Makefile.inc M src/vendorcode/amd/pi/Kconfig 4 files changed, 20 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/36823/4
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36823/4/src/soc/amd/stoneyridge/Mak... File src/soc/amd/stoneyridge/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36823/4/src/soc/amd/stoneyridge/Mak... PS4, Line 140: ifeq ($(CONFIG_SOC_AMD_MERLINFALCON),y) : : ifeq ($(CONFIG_HAVE_MERLINFALCON_BINARIES),y) : FIRMWARE_TYPE=CZ : else : FIRMWARE_TYPE=ST : endif Don't you still have the same problem you're trying to avoid? That is, when you select Merlin Falcon when you don't have the right binaries, don't you silently still get ST?
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36823/4/src/soc/amd/stoneyridge/Mak... File src/soc/amd/stoneyridge/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36823/4/src/soc/amd/stoneyridge/Mak... PS4, Line 140: ifeq ($(CONFIG_SOC_AMD_MERLINFALCON),y) : : ifeq ($(CONFIG_HAVE_MERLINFALCON_BINARIES),y) : FIRMWARE_TYPE=CZ : else : FIRMWARE_TYPE=ST : endif
Don't you still have the same problem you're trying to avoid? That is, when you select Merlin Falco […]
That is true, but unfortunately needed until binaries become available. Than the inner nest can be dropped an become simply FIRMWARE_TYPE=CZ. Also the actual config HAVE_MERLINFALCON_BINARIES will be dropped than.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 4:
Ping... review please.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36823/4/src/soc/amd/stoneyridge/Mak... File src/soc/amd/stoneyridge/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36823/4/src/soc/amd/stoneyridge/Mak... PS4, Line 140: ifeq ($(CONFIG_SOC_AMD_MERLINFALCON),y) : : ifeq ($(CONFIG_HAVE_MERLINFALCON_BINARIES),y) : FIRMWARE_TYPE=CZ : else : FIRMWARE_TYPE=ST : endif
That is true, but unfortunately needed until binaries become available. […]
My point is this. If you add a patch that purports to solve a problem, but doesn't actually change the behavior, why have the patch at all? The first paragraph of your commit message sets the premise of ST binaries being built in if you don't possess MF binaries. Don't you still have that same problem with the way you've designed the logic above.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36823/4/src/soc/amd/stoneyridge/Mak... File src/soc/amd/stoneyridge/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36823/4/src/soc/amd/stoneyridge/Mak... PS4, Line 140: ifeq ($(CONFIG_SOC_AMD_MERLINFALCON),y) : : ifeq ($(CONFIG_HAVE_MERLINFALCON_BINARIES),y) : FIRMWARE_TYPE=CZ : else : FIRMWARE_TYPE=ST : endif
My point is this. […]
So maybe change commit message? The truth is, I can't remove this until binaries are available... but AMD wanted Prairie Falcon, which is available "under the hood" and I'm the only one who knew it. So I made Prairie Falcon explicitly available.
Hello Marshall Dawson, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36823
to look at the new patch set (#5).
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit
Current code surreptitiously selected Prairie Falcon if Merlin Falcon is selected and no Merlin Falcon binaries are available. This is not optimal, make the selection an explicit choice. The objective is to make it possible for user to select Prairie Falcon without the roundabout of using the absence of Merlin Falcon (which is not obvious).
Main differences between Stoney Ridge, Merlin Falcon and Prairie Falcon are related to number of memory channels, binaries used and if PSP is capable of dual load (combo capable).
Only one particular version of Bristol Ridge binaries were tested and worked for Merlin Falcon, so for the time being it must use its own binary image (not all Bristol Ridge images work).
BUG=None TEST=Build padmelon with Prarie Falcon and Merlin Falcon
Change-Id: I309c5918fdc98d9927641466bbe6149b97b250f0 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/stoneyridge/Kconfig M src/soc/amd/stoneyridge/Makefile.inc M src/vendorcode/amd/pi/00670F00/Makefile.inc M src/vendorcode/amd/pi/Kconfig 4 files changed, 20 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/36823/5
Hello Marshall Dawson, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36823
to look at the new patch set (#6).
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit
Current code surreptitiously selected Prairie Falcon if Merlin Falcon is selected and no Merlin Falcon binaries are available. This is not optimal, make the selection an explicit choice. The objective is to make it possible for user to select Prairie Falcon without the roundabout of using the absence of Merlin Falcon binaries (which is not obvious).
Main differences between Stoney Ridge, Merlin Falcon and Prairie Falcon are related to number of memory channels, binaries used and if PSP is capable of dual load (combo capable).
Only one particular version of Bristol Ridge binaries were tested and worked for Merlin Falcon, so for the time being it must use its own binary image (not all Bristol Ridge images work).
BUG=None TEST=Build padmelon with Prarie Falcon and Merlin Falcon
Change-Id: I309c5918fdc98d9927641466bbe6149b97b250f0 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/stoneyridge/Kconfig M src/soc/amd/stoneyridge/Makefile.inc M src/vendorcode/amd/pi/00670F00/Makefile.inc M src/vendorcode/amd/pi/Kconfig 4 files changed, 20 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/36823/6
Richard Spiegel has removed Martin Roth from this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Removed reviewer Martin Roth.
Richard Spiegel has removed Marshall Dawson from this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Removed reviewer Marshall Dawson.
Richard Spiegel has removed Patrick Georgi from this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Removed reviewer Patrick Georgi.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 6: Code-Review-1
I would prefer modifying src//stoneyridge to use the new amd_blobs repo. See CB:37119. Since cloning that requires an explicit action on the developer's part (look under General setup), all builds need to be capable of completing successfully without blobs included.
First making that change, then modifying default values is a better way to go.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 6:
Patch Set 6: Code-Review-1
I would prefer modifying src//stoneyridge to use the new amd_blobs repo. See CB:37119. Since cloning that requires an explicit action on the developer's part (look under General setup), all builds need to be capable of completing successfully without blobs included.
First making that change, then modifying default values is a better way to go.
I just found that the Merlin Falcon blob was already merged. So I'll remove the need for the binaries present, thus complying with your request
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 6: Code-Review-1
I would prefer modifying src//stoneyridge to use the new amd_blobs repo. See CB:37119. Since cloning that requires an explicit action on the developer's part (look under General setup), all builds need to be capable of completing successfully without blobs included.
First making that change, then modifying default values is a better way to go.
I just found that the Merlin Falcon blob was already merged. So I'll remove the need for the binaries present, thus complying with your request
Someone made a mistake and only committed part of the binaries. Can't remove the request for binaries yet.
Hello Kyösti Mälkki, Edward O'Callaghan, Marshall Dawson, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36823
to look at the new patch set (#7).
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit
Current code surreptitiously selected Prairie Falcon if Merlin Falcon is selected and no Merlin Falcon binaries are available. This is not optimal, make the selection an explicit choice. The objective is to make it possible for user to select Prairie Falcon without the roundabout of using the absence of Merlin Falcon binaries (which is not obvious).
Main differences between Stoney Ridge, Merlin Falcon and Prairie Falcon are related to number of memory channels, binaries used and if PSP is capable of dual load (combo capable).
Only one particular version of Bristol Ridge binaries were tested and worked for Merlin Falcon, so for the time being it must use its own binary image (not all Bristol Ridge images work).
BUG=None TEST=Build padmelon with Prarie Falcon and Merlin Falcon
Change-Id: I309c5918fdc98d9927641466bbe6149b97b250f0 Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/stoneyridge/Makefile.inc 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/36823/7
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36823/7/src/soc/amd/stoneyridge/Mak... File src/soc/amd/stoneyridge/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36823/7/src/soc/amd/stoneyridge/Mak... PS7, Line 159: endif Extra, to be removed.
Richard Spiegel has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36823 )
Change subject: soc/amd/stoneyridge/kconfig: Make Prairie Falcon selection explicit ......................................................................
Abandoned
After rebasing on Marshall's code, I found his code has all that I'm implementing he. He used what he saw when reviewing this patch to create his patch, thus making this one useless.
-
Marshall Dawson (Code Review) -
Richard Spiegel (Code Review)