Change in ...coreboot[master]: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC
Richard Spiegel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33624
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC
Merlinfalcon is essentially a carrizo CPU in a SOC presentation. Just as stoneyridge, they are both family 15h CPU, thus most of the code is similar if not identical. Add changes base on config parameter SOC_AMD_MERLINFALCON_FP4 to accommodate for merlinfalcon SOC.
BUG=b:none. TEST=Tested later with padmelon board.
Change-Id: I00fe832324500bcb07fca292a0a55f7258a2d82f Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/stoneyridge/chip.h M src/soc/amd/stoneyridge/cpu.c M src/soc/amd/stoneyridge/include/soc/pci_devs.h M src/soc/amd/stoneyridge/northbridge.c 4 files changed, 61 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/33624/1
diff --git a/src/soc/amd/stoneyridge/chip.h b/src/soc/amd/stoneyridge/chip.h index d1a7d30..6eb2ec7 100644 --- a/src/soc/amd/stoneyridge/chip.h +++ b/src/soc/amd/stoneyridge/chip.h @@ -20,12 +20,18 @@ #include <stdint.h> #include <commonlib/helpers.h> #include <drivers/i2c/designware/dw_i2c.h> +#include <soc/cpu.h> #include <soc/i2c.h> #include <arch/acpi_device.h>
#define MAX_NODES 1 +#if CONFIG(SOC_AMD_MERLINFALCON_FP4) +#define MAX_DRAM_CH 2 +#define MAX_DIMMS_PER_CH 4 +#else #define MAX_DRAM_CH 1 #define MAX_DIMMS_PER_CH 2 +#endif
#define STONEY_I2C_DEV_MAX 4
diff --git a/src/soc/amd/stoneyridge/cpu.c b/src/soc/amd/stoneyridge/cpu.c index 1d9804d..f751dc8 100644 --- a/src/soc/amd/stoneyridge/cpu.c +++ b/src/soc/amd/stoneyridge/cpu.c @@ -145,6 +145,7 @@ };
static struct cpu_device_id cpu_table[] = { + { X86_VENDOR_AMD, 0x660f01 }, { X86_VENDOR_AMD, 0x670f00 }, { 0, 0 }, }; diff --git a/src/soc/amd/stoneyridge/include/soc/pci_devs.h b/src/soc/amd/stoneyridge/include/soc/pci_devs.h index 02fed7a..01a0b7c 100644 --- a/src/soc/amd/stoneyridge/include/soc/pci_devs.h +++ b/src/soc/amd/stoneyridge/include/soc/pci_devs.h @@ -39,17 +39,24 @@ #define IOMMU_DEVFN PCI_DEVFN(IOMMU_DEV, IOMMU_FUNC) #define SOC_IOMMU_DEV _SOC_DEV(IOMMU_DEV, IOMMU_FUNC)
-/* Internal Graphics */ +/* + * Internal Graphics + * Device IDs subject to SKU/OPN variation + * GFX_DEVID for merlinfalcon PCI_DEVICE_ID_AMD_15H_MODEL_606F_GFX + * GFX_DEVID for stoneyridge PCI_DEVICE_ID_AMD_15H_MODEL_707F_GFX + */ #define GFX_DEV 0x1 #define GFX_FUNC 0 -#define GFX_DEVID 0x98e4 /* subject to SKU/OPN variation */ #define GFX_DEVFN PCI_DEVFN(GFX_DEV, GFX_FUNC) #define SOC_GFX_DEV _SOC_DEV(GFX_DEV, GFX_FUNC)
-/* HD Audio 0 */ +/* HD Audio 0 + * Device IDs + * HDA0_DEVID PCI_DEVICE_ID_AMD_15H_MODEL_606F_HDA + * HDA0_DEVID PCI_DEVICE_ID_AMD_15H_MODEL_707F_HDA + */ #define HDA0_DEV 0x1 #define HDA0_FUNC 1 -#define HDA0_DEVID 0x15b3 #define HDA0_DEVFN PCI_DEVFN(HDA0_DEV, HDA0_FUNC) #define SOC_HDA0_DEV _SOC_DEV(HDA0_DEV, HDA0_FUNC)
@@ -109,45 +116,63 @@ #define HDA1_DEVFN PCI_DEVFN(HDA1_DEV, HDA1_FUNC) #define SOC_HDA1_DEV _SOC_DEV(HDA1_DEV, HDA1_FUNC)
-/* HT Configuration */ +/* HT Configuration + * Device IDs + * HT_DEVID for merlinfalcon PCI_DEVICE_ID_AMD_15H_MODEL_606F_NB_HT + * HT_DEVID for stoneyridge PCI_DEVICE_ID_AMD_15H_MODEL_707F_NB_HT + */ #define HT_DEV 0x18 #define HT_FUNC 0 -#define HT_DEVID 0x15b0 #define HT_DEVFN PCI_DEVFN(HT_DEV, HT_FUNC) #define SOC_HT_DEV _SOC_DEV(HT_DEV, HT_FUNC)
-/* Address Maps */ +/* Address Maps + * Device IDs + * ADDR_DEVID for merlinfalcon 0x1571 + * ADDR_DEVID for stoneyridge 0x15b1 + */ #define ADDR_DEV 0x18 #define ADDR_FUNC 1 -#define ADDR_DEVID 0x15b1 #define ADDR_DEVFN PCI_DEVFN(ADDR_DEV, ADDR_FUNC) #define SOC_ADDR_DEV _SOC_DEV(ADDR_DEV, ADDR_FUNC)
-/* DRAM Configuration */ +/* DRAM Configuration + * Device IDs + * DCT_DEVID for merlinfalcon 0x1572 + * DCT_DEVID for stoneyridge 0x15b2 + */ #define DCT_DEV 0x18 #define DCT_FUNC 2 -#define DCT_DEVID 0x15b2 #define DCT_DEVFN PCI_DEVFN(DCT_DEV, DCT_FUNC) #define SOC_DCT_DEV _SOC_DEV(DCT_DEV, DCT_FUNC)
-/* Misc. Configuration */ +/* Misc. Configuration + * Device IDs + * MISC_DEVID for merlinfalcon 0x1573 + * MISC_DEVID for stoneyridge 0x15b3 + */ #define MISC_DEV 0x18 #define MISC_FUNC 3 -#define MISC_DEVID 0x15b3 #define MISC_DEVFN PCI_DEVFN(MISC_DEV, MISC_FUNC) #define SOC_MISC_DEV _SOC_DEV(MISC_DEV, MISC_FUNC)
-/* PM Configuration */ +/* PM Configuration + * Device IDs + * PM_DEVID for merlinfalcon 0x1574 + * PM_DEVID for stoneyridge 0x15b4 + */ #define PM_DEV 0x18 #define PM_FUNC 4 -#define PM_DEVID 0x15b4 #define PM_DEVFN PCI_DEVFN(PM_DEV, PM_FUNC) #define SOC_PM_DEV _SOC_DEV(PM_DEV, PM_FUNC)
-/* Northbridge Configuration */ +/* Northbridge Configuration + * Device IDs + * NB_DEVID for merlinfalcon 0x1575 + * NB_DEVID for stoneyridge 0x15b5 + */ #define NB_DEV 0x18 #define NB_FUNC 5 -#define NB_DEVID 0x15b5 #define NB_DEVFN PCI_DEVFN(NB_DEV, NB_FUNC) #define SOC_NB_DEV _SOC_DEV(NB_DEV, NB_FUNC)
diff --git a/src/soc/amd/stoneyridge/northbridge.c b/src/soc/amd/stoneyridge/northbridge.c index 5985832..7284f23 100644 --- a/src/soc/amd/stoneyridge/northbridge.c +++ b/src/soc/amd/stoneyridge/northbridge.c @@ -347,10 +347,15 @@ .ops_pci = 0, };
+static const unsigned short pci_device_ids[] = { + PCI_DEVICE_ID_AMD_15H_MODEL_606F_NB_HT, + PCI_DEVICE_ID_AMD_15H_MODEL_707F_NB_HT, + 0 }; + static const struct pci_driver family15_northbridge __pci_driver = { .ops = &northbridge_operations, .vendor = PCI_VENDOR_ID_AMD, - .device = PCI_DEVICE_ID_AMD_15H_MODEL_707F_NB_HT, + .devices = pci_device_ids, };
/* @@ -464,9 +469,13 @@ u32 map_oprom_vendev(u32 vendev) { u32 new_vendev; - new_vendev = - ((vendev >= 0x100298e0) && (vendev <= 0x100298ef)) ? - 0x100298e0 : vendev; + + if ((vendev >= 0x100298e0) && (vendev <= 0x100298ef)) + new_vendev = 0x100298e0; + else if ((vendev >= 0x10029870) && (vendev <= 0x1002987f)) + new_vendev = 0x10029870; + else + new_vendev = vendev;
if (vendev != new_vendev) printk(BIOS_NOTICE, "Mapping PCI device %8x to %8x\n",
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33624/2/src/soc/amd/stoneyridge/northbridge.... File src/soc/amd/stoneyridge/northbridge.c:
https://review.coreboot.org/#/c/33624/2/src/soc/amd/stoneyridge/northbridge.... PS2, Line 353: That's a lot of tabs. Why not align like other structs and arrays and lead with a single tab?
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/33624/2/src/soc/amd/stoneyridge/northbridge.... File src/soc/amd/stoneyridge/northbridge.c:
https://review.coreboot.org/#/c/33624/2/src/soc/amd/stoneyridge/northbridge.... PS2, Line 353:
That's a lot of tabs. […]
Because I copy paste an example from elsewhere, just changing names. Will do.
Hello Charles Marslett, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33624
to look at the new patch set (#3).
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC
Merlinfalcon is essentially a carrizo CPU in a SOC presentation. Just as stoneyridge, they are both family 15h CPU, thus most of the code is similar if not identical. Add changes base on config parameter SOC_AMD_MERLINFALCON to accommodate for merlinfalcon SOC.
BUG=b:none. TEST=Tested later with padmelon board.
Change-Id: I00fe832324500bcb07fca292a0a55f7258a2d82f Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/stoneyridge/acpi/cpu.asl M src/soc/amd/stoneyridge/chip.h M src/soc/amd/stoneyridge/cpu.c M src/soc/amd/stoneyridge/include/soc/pci_devs.h M src/soc/amd/stoneyridge/northbridge.c 5 files changed, 82 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/33624/3
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/acpi/cpu.asl File src/soc/amd/stoneyridge/acpi/cpu.asl:
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/acpi/cpu.asl... PS3, Line 37: If (LGreaterEqual (\PCNT, 8)) I kind of thought 2 CUs, i.e. 4 cores was the max for Models 60-6F. Are you just trying to cover all possible bases here, or do you know of a MF that has 8 cores?
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/include/soc/... File src/soc/amd/stoneyridge/include/soc/pci_devs.h:
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/include/soc/... PS3, Line 44: * Device IDs For the record, I'm OK with not #define-ing the device IDs here. BTW, it's a shame we don't use the Gfx IDs in the detection used for the vbios.
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/northbridge.... File src/soc/amd/stoneyridge/northbridge.c:
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/northbridge.... PS3, Line 351: Can you remove these tabs so that it matches the nearby source? I thought you said you'd change it. Maybe it's your editor did it automatically?
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/acpi/cpu.asl File src/soc/amd/stoneyridge/acpi/cpu.asl:
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/acpi/cpu.asl... PS3, Line 37: If (LGreaterEqual (\PCNT, 8))
I kind of thought 2 CUs, i.e. 4 cores was the max for Models 60-6F. […]
I'll have to double check, but when I used the original BIOS and booted to windows, there were some indication of 4 cores making 8 processing units (hyperthread ?)
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/northbridge.... File src/soc/amd/stoneyridge/northbridge.c:
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/northbridge.... PS3, Line 358: .devices = pci_device_ids, I noticed I missed, uploaded a patch 4, but for some unknown reason it did not show up here. I decided to wait, just in case it would come up later... apparently did not. Will resubmit.
Hello Charles Marslett, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33624
to look at the new patch set (#4).
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC
Merlinfalcon is essentially a carrizo CPU in a SOC presentation. Just as stoneyridge, they are both family 15h CPU, thus most of the code is similar if not identical. Add changes base on config parameter SOC_AMD_MERLINFALCON to accommodate for merlinfalcon SOC.
BUG=b:none. TEST=Tested later with padmelon board.
Change-Id: I00fe832324500bcb07fca292a0a55f7258a2d82f Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/stoneyridge/acpi/cpu.asl M src/soc/amd/stoneyridge/chip.h M src/soc/amd/stoneyridge/cpu.c M src/soc/amd/stoneyridge/include/soc/pci_devs.h M src/soc/amd/stoneyridge/northbridge.c 5 files changed, 82 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/33624/4
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/acpi/cpu.asl File src/soc/amd/stoneyridge/acpi/cpu.asl:
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/acpi/cpu.asl... PS3, Line 37: If (LGreaterEqual (\PCNT, 8))
I'll have to double check, but when I used the original BIOS and booted to windows, there were some […]
Look at RX-421BD https://www.amd.com/en/products/embedded-r-series-soc https://www.cpubenchmark.net/cpu.php?cpu=AMD+Embedded+R-Series+RX-421BD&... Also RWEverything shows 4 groups of MSR (I believe hyperthread shares MSR). So yes, 4 cores, 8 processing units (hyperthread)
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/acpi/cpu.asl File src/soc/amd/stoneyridge/acpi/cpu.asl:
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/acpi/cpu.asl... PS3, Line 37: If (LGreaterEqual (\PCNT, 8))
Look at RX-421BD […]
They're not hyperthreaded.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/acpi/cpu.asl File src/soc/amd/stoneyridge/acpi/cpu.asl:
https://review.coreboot.org/#/c/33624/3/src/soc/amd/stoneyridge/acpi/cpu.asl... PS3, Line 37: If (LGreaterEqual (\PCNT, 8))
They're not hyperthreaded.
Really? So I only need P000 through P003.
Hello Charles Marslett, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33624
to look at the new patch set (#5).
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC
Merlinfalcon is essentially a carrizo CPU in a SOC presentation. Just as stoneyridge, they are both family 15h CPU, thus most of the code is similar if not identical. Add changes base on config parameter SOC_AMD_MERLINFALCON to accommodate for merlinfalcon SOC.
BUG=b:none. TEST=Tested later with padmelon board.
Change-Id: I00fe832324500bcb07fca292a0a55f7258a2d82f Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/stoneyridge/acpi/cpu.asl M src/soc/amd/stoneyridge/chip.h M src/soc/amd/stoneyridge/cpu.c M src/soc/amd/stoneyridge/include/soc/pci_devs.h M src/soc/amd/stoneyridge/northbridge.c 5 files changed, 70 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/33624/5
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33624/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33624/6//COMMIT_MSG@11 PS6, Line 11: base based
https://review.coreboot.org/c/coreboot/+/33624/6/src/soc/amd/stoneyridge/chi... File src/soc/amd/stoneyridge/chip.h:
https://review.coreboot.org/c/coreboot/+/33624/6/src/soc/amd/stoneyridge/chi... PS6, Line 23: #include <soc/cpu.h> Why is this needed?
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33624/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33624/6//COMMIT_MSG@11 PS6, Line 11: base
based
Thanks
https://review.coreboot.org/c/coreboot/+/33624/6/src/soc/amd/stoneyridge/chi... File src/soc/amd/stoneyridge/chip.h:
https://review.coreboot.org/c/coreboot/+/33624/6/src/soc/amd/stoneyridge/chi... PS6, Line 23: #include <soc/cpu.h>
Why is this needed?
Not sure... either leftover from earlier version, or more likely copy/paste error. Will remove.
Hello Charles Marslett, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33624
to look at the new patch set (#7).
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC
Merlinfalcon is essentially a carrizo CPU in a SOC presentation. Just as stoneyridge, they are both family 15h CPU, thus most of the code is similar if not identical. Add changes based on config parameter SOC_AMD_MERLINFALCON to accommodate for merlinfalcon SOC.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I00fe832324500bcb07fca292a0a55f7258a2d82f Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/stoneyridge/acpi/cpu.asl M src/soc/amd/stoneyridge/chip.h M src/soc/amd/stoneyridge/cpu.c M src/soc/amd/stoneyridge/include/soc/pci_devs.h M src/soc/amd/stoneyridge/northbridge.c 5 files changed, 69 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/33624/7
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
Patch Set 9: Code-Review+2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
Patch Set 9:
(3 comments)
https://review.coreboot.org/c/coreboot/+/33624/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33624/9//COMMIT_MSG@9 PS9, Line 9: carrizo CPU in a SOC presentation I would prefer you don't mention this. AMD Embedded has been very careful to use their own branding. Carrizo is also in an SOC. You don't even need to say "as stoneyridge...". You could mention that Merlin Falcon is Family 15h Models 60h-6Fh. Stoney Ridge is Family 15h models 70h-7Fh, so you're making this code backward compatible to support Merlin Falcon.
https://review.coreboot.org/c/coreboot/+/33624/9//COMMIT_MSG@12 PS9, Line 12: merlinfalcon You also might want to develop the habit of capitalizing the first letters of codenames, e.g. "Merlin Falcon". There is no merlinfalcon SOC. And all lower case for directory names, e.g. stoneyridge.
https://review.coreboot.org/c/coreboot/+/33624/9/src/soc/amd/stoneyridge/chi... File src/soc/amd/stoneyridge/chip.h:
https://review.coreboot.org/c/coreboot/+/33624/9/src/soc/amd/stoneyridge/chi... PS9, Line 29: MAX_DIMMS_PER_CH Are you certain of this? Can you direct me to the document? I only see 2 DIMMs in the BKDG. If you picked it up from the 00660F01 code, it could be wrong.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
Patch Set 9:
(3 comments)
https://review.coreboot.org/c/coreboot/+/33624/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33624/9//COMMIT_MSG@9 PS9, Line 9: carrizo CPU in a SOC presentation
I would prefer you don't mention this. […]
Ok, will do.
https://review.coreboot.org/c/coreboot/+/33624/9//COMMIT_MSG@12 PS9, Line 12: merlinfalcon
You also might want to develop the habit of capitalizing the first letters of codenames, e.g. […]
I was not sure if Merlin Falcon was a single word or 2 words (which looks better). Will do.
https://review.coreboot.org/c/coreboot/+/33624/9/src/soc/amd/stoneyridge/chi... File src/soc/amd/stoneyridge/chip.h:
https://review.coreboot.org/c/coreboot/+/33624/9/src/soc/amd/stoneyridge/chi... PS9, Line 29: MAX_DIMMS_PER_CH
Are you certain of this? Can you direct me to the document? I only see 2 DIMMs in the BKDG. […]
I got it from Marc's code. I have no documentation to say either way. It's probably due to 4 cores against stoney's 2 cores.
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33624/9/src/soc/amd/stoneyridge/chi... File src/soc/amd/stoneyridge/chip.h:
https://review.coreboot.org/c/coreboot/+/33624/9/src/soc/amd/stoneyridge/chi... PS9, Line 29: MAX_DIMMS_PER_CH
I got it from Marc's code. I have no documentation to say either way. […]
I can assure you it has nothing to do with the number of cores. Please see my original statement above and investigate where Marc may have gotten it.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate merlinfalcon SOC ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33624/9/src/soc/amd/stoneyridge/chi... File src/soc/amd/stoneyridge/chip.h:
https://review.coreboot.org/c/coreboot/+/33624/9/src/soc/amd/stoneyridge/chi... PS9, Line 29: MAX_DIMMS_PER_CH
I can assure you it has nothing to do with the number of cores. […]
Can't. I never had Merlin Falcon BKDG. Never found it.
Hello Charles Marslett, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33624
to look at the new patch set (#10).
Change subject: soc/amd/stoneyridge: Change code to accommodate Merlin Falcon SOC ......................................................................
soc/amd/stoneyridge: Change code to accommodate Merlin Falcon SOC
Stoney Ridge is family 15h models 70h-7Fh, Merlin Falcon is family 15h models 60h-6Fh. Add changes based on config parameter SOC_AMD_MERLINFALCON to make the code backward compatible with Merlin Falcon.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I00fe832324500bcb07fca292a0a55f7258a2d82f Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/stoneyridge/acpi/cpu.asl M src/soc/amd/stoneyridge/chip.h M src/soc/amd/stoneyridge/cpu.c M src/soc/amd/stoneyridge/include/soc/pci_devs.h M src/soc/amd/stoneyridge/northbridge.c 5 files changed, 69 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/33624/10
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate Merlin Falcon SOC ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33624/9/src/soc/amd/stoneyridge/chi... File src/soc/amd/stoneyridge/chip.h:
https://review.coreboot.org/c/coreboot/+/33624/9/src/soc/amd/stoneyridge/chi... PS9, Line 29: MAX_DIMMS_PER_CH
Can't. I never had Merlin Falcon BKDG. Never found it.
I will contact you offline
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate Merlin Falcon SOC ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33624/9/src/soc/amd/stoneyridge/chi... File src/soc/amd/stoneyridge/chip.h:
https://review.coreboot.org/c/coreboot/+/33624/9/src/soc/amd/stoneyridge/chi... PS9, Line 29: MAX_DIMMS_PER_CH
I will contact you offline
You are correct, there's some confusion on the BKDG talking about 4 DIMM, but when you get the details it's really 2 DIMM per channel making a total of 4 DIMM. Will fix.
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate Merlin Falcon SOC ......................................................................
Patch Set 10:
Please don't merge it yet. There will be a new patch that will need review.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate Merlin Falcon SOC ......................................................................
Patch Set 10: -Code-Review
It's in the middle of a patch stack, It wouldn't get merged until the earlier patches are merged. Removing +2 to address marshall's concern though.
Hello Charles Marslett, Marshall Dawson, Paul Menzel, build bot (Jenkins), Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33624
to look at the new patch set (#11).
Change subject: soc/amd/stoneyridge: Change code to accommodate Merlin Falcon SOC ......................................................................
soc/amd/stoneyridge: Change code to accommodate Merlin Falcon SOC
Stoney Ridge is family 15h models 70h-7Fh, Merlin Falcon is family 15h models 60h-6Fh. Add changes based on config parameter SOC_AMD_MERLINFALCON to make the code backward compatible with Merlin Falcon.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I00fe832324500bcb07fca292a0a55f7258a2d82f Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com --- M src/soc/amd/stoneyridge/acpi/cpu.asl M src/soc/amd/stoneyridge/chip.h M src/soc/amd/stoneyridge/cpu.c M src/soc/amd/stoneyridge/include/soc/pci_devs.h M src/soc/amd/stoneyridge/northbridge.c 5 files changed, 70 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/24/33624/11
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate Merlin Falcon SOC ......................................................................
Patch Set 14:
(3 comments)
https://review.coreboot.org/c/coreboot/+/33624/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33624/9//COMMIT_MSG@9 PS9, Line 9: carrizo CPU in a SOC presentation
Ok, will do.
Done
https://review.coreboot.org/c/coreboot/+/33624/9//COMMIT_MSG@12 PS9, Line 12: merlinfalcon
I was not sure if Merlin Falcon was a single word or 2 words (which looks better). Will do.
Done
https://review.coreboot.org/c/coreboot/+/33624/9/src/soc/amd/stoneyridge/chi... File src/soc/amd/stoneyridge/chip.h:
https://review.coreboot.org/c/coreboot/+/33624/9/src/soc/amd/stoneyridge/chi... PS9, Line 29: MAX_DIMMS_PER_CH
You are correct, there's some confusion on the BKDG talking about 4 DIMM, but when you get the detai […]
Done
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate Merlin Falcon SOC ......................................................................
Patch Set 14:
(6 comments)
https://review.coreboot.org/c/coreboot/+/33624/3/src/soc/amd/stoneyridge/acp... File src/soc/amd/stoneyridge/acpi/cpu.asl:
https://review.coreboot.org/c/coreboot/+/33624/3/src/soc/amd/stoneyridge/acp... PS3, Line 37: If (LGreaterEqual (\PCNT, 8))
Really? So I only need P000 through P003.
Done
https://review.coreboot.org/c/coreboot/+/33624/3/src/soc/amd/stoneyridge/inc... File src/soc/amd/stoneyridge/include/soc/pci_devs.h:
https://review.coreboot.org/c/coreboot/+/33624/3/src/soc/amd/stoneyridge/inc... PS3, Line 44: * Device IDs
For the record, I'm OK with not #define-ing the device IDs here. […]
Done
https://review.coreboot.org/c/coreboot/+/33624/2/src/soc/amd/stoneyridge/nor... File src/soc/amd/stoneyridge/northbridge.c:
https://review.coreboot.org/c/coreboot/+/33624/2/src/soc/amd/stoneyridge/nor... PS2, Line 353:
Because I copy paste an example from elsewhere, just changing names. Will do.
Done
https://review.coreboot.org/c/coreboot/+/33624/2/src/soc/amd/stoneyridge/nor... PS2, Line 353:
Because I copy paste an example from elsewhere, just changing names. Will do.
Done
https://review.coreboot.org/c/coreboot/+/33624/3/src/soc/amd/stoneyridge/nor... File src/soc/amd/stoneyridge/northbridge.c:
https://review.coreboot.org/c/coreboot/+/33624/3/src/soc/amd/stoneyridge/nor... PS3, Line 351:
Can you remove these tabs so that it matches the nearby source? I thought you said you'd change it. […]
Done
https://review.coreboot.org/c/coreboot/+/33624/3/src/soc/amd/stoneyridge/nor... PS3, Line 358: .devices = pci_device_ids,
I noticed I missed, uploaded a patch 4, but for some unknown reason it did not show up here. […]
Done
Richard Spiegel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate Merlin Falcon SOC ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33624/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33624/6//COMMIT_MSG@11 PS6, Line 11: base
Thanks
Done
https://review.coreboot.org/c/coreboot/+/33624/6/src/soc/amd/stoneyridge/chi... File src/soc/amd/stoneyridge/chip.h:
https://review.coreboot.org/c/coreboot/+/33624/6/src/soc/amd/stoneyridge/chi... PS6, Line 23: #include <soc/cpu.h>
Not sure... either leftover from earlier version, or more likely copy/paste error. Will remove.
Done
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate Merlin Falcon SOC ......................................................................
Patch Set 14: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33624 )
Change subject: soc/amd/stoneyridge: Change code to accommodate Merlin Falcon SOC ......................................................................
soc/amd/stoneyridge: Change code to accommodate Merlin Falcon SOC
Stoney Ridge is family 15h models 70h-7Fh, Merlin Falcon is family 15h models 60h-6Fh. Add changes based on config parameter SOC_AMD_MERLINFALCON to make the code backward compatible with Merlin Falcon.
BUG=none. TEST=Tested later with padmelon board.
Change-Id: I00fe832324500bcb07fca292a0a55f7258a2d82f Signed-off-by: Richard Spiegel richard.spiegel@silverbackltd.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33624 Reviewed-by: Martin Roth martinroth@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/stoneyridge/acpi/cpu.asl M src/soc/amd/stoneyridge/chip.h M src/soc/amd/stoneyridge/cpu.c M src/soc/amd/stoneyridge/include/soc/pci_devs.h M src/soc/amd/stoneyridge/northbridge.c 5 files changed, 70 insertions(+), 21 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved
diff --git a/src/soc/amd/stoneyridge/acpi/cpu.asl b/src/soc/amd/stoneyridge/acpi/cpu.asl index 414326e..94638b0 100644 --- a/src/soc/amd/stoneyridge/acpi/cpu.asl +++ b/src/soc/amd/stoneyridge/acpi/cpu.asl @@ -34,7 +34,15 @@ /* Return a package containing enabled processor entries */ Method (PPKG) { - If (LGreaterEqual (\PCNT, 2)) { + If (LGreaterEqual (\PCNT, 4)) { + Return (Package () + { + _PR.P000, + _PR.P001, + _PR.P002, + _PR.P003 + }) + } ElseIf (LGreaterEqual (\PCNT, 2)) { Return (Package () { _PR.P000, diff --git a/src/soc/amd/stoneyridge/chip.h b/src/soc/amd/stoneyridge/chip.h index d1a7d30..00b675c 100644 --- a/src/soc/amd/stoneyridge/chip.h +++ b/src/soc/amd/stoneyridge/chip.h @@ -23,9 +23,15 @@ #include <soc/i2c.h> #include <arch/acpi_device.h>
+/* Merlin Falcon supports 2 channels, Prairie Falcon only 1 (channel B) */ #define MAX_NODES 1 +#if CONFIG(SOC_AMD_MERLINFALCON) && CONFIG(HAVE_MERLINFALCON_BINARIES) +#define MAX_DRAM_CH 2 +#define MAX_DIMMS_PER_CH 2 +#else #define MAX_DRAM_CH 1 #define MAX_DIMMS_PER_CH 2 +#endif
#define STONEY_I2C_DEV_MAX 4
diff --git a/src/soc/amd/stoneyridge/cpu.c b/src/soc/amd/stoneyridge/cpu.c index 1d9804d..f751dc8 100644 --- a/src/soc/amd/stoneyridge/cpu.c +++ b/src/soc/amd/stoneyridge/cpu.c @@ -145,6 +145,7 @@ };
static struct cpu_device_id cpu_table[] = { + { X86_VENDOR_AMD, 0x660f01 }, { X86_VENDOR_AMD, 0x670f00 }, { 0, 0 }, }; diff --git a/src/soc/amd/stoneyridge/include/soc/pci_devs.h b/src/soc/amd/stoneyridge/include/soc/pci_devs.h index 02fed7a..01a0b7c 100644 --- a/src/soc/amd/stoneyridge/include/soc/pci_devs.h +++ b/src/soc/amd/stoneyridge/include/soc/pci_devs.h @@ -39,17 +39,24 @@ #define IOMMU_DEVFN PCI_DEVFN(IOMMU_DEV, IOMMU_FUNC) #define SOC_IOMMU_DEV _SOC_DEV(IOMMU_DEV, IOMMU_FUNC)
-/* Internal Graphics */ +/* + * Internal Graphics + * Device IDs subject to SKU/OPN variation + * GFX_DEVID for merlinfalcon PCI_DEVICE_ID_AMD_15H_MODEL_606F_GFX + * GFX_DEVID for stoneyridge PCI_DEVICE_ID_AMD_15H_MODEL_707F_GFX + */ #define GFX_DEV 0x1 #define GFX_FUNC 0 -#define GFX_DEVID 0x98e4 /* subject to SKU/OPN variation */ #define GFX_DEVFN PCI_DEVFN(GFX_DEV, GFX_FUNC) #define SOC_GFX_DEV _SOC_DEV(GFX_DEV, GFX_FUNC)
-/* HD Audio 0 */ +/* HD Audio 0 + * Device IDs + * HDA0_DEVID PCI_DEVICE_ID_AMD_15H_MODEL_606F_HDA + * HDA0_DEVID PCI_DEVICE_ID_AMD_15H_MODEL_707F_HDA + */ #define HDA0_DEV 0x1 #define HDA0_FUNC 1 -#define HDA0_DEVID 0x15b3 #define HDA0_DEVFN PCI_DEVFN(HDA0_DEV, HDA0_FUNC) #define SOC_HDA0_DEV _SOC_DEV(HDA0_DEV, HDA0_FUNC)
@@ -109,45 +116,63 @@ #define HDA1_DEVFN PCI_DEVFN(HDA1_DEV, HDA1_FUNC) #define SOC_HDA1_DEV _SOC_DEV(HDA1_DEV, HDA1_FUNC)
-/* HT Configuration */ +/* HT Configuration + * Device IDs + * HT_DEVID for merlinfalcon PCI_DEVICE_ID_AMD_15H_MODEL_606F_NB_HT + * HT_DEVID for stoneyridge PCI_DEVICE_ID_AMD_15H_MODEL_707F_NB_HT + */ #define HT_DEV 0x18 #define HT_FUNC 0 -#define HT_DEVID 0x15b0 #define HT_DEVFN PCI_DEVFN(HT_DEV, HT_FUNC) #define SOC_HT_DEV _SOC_DEV(HT_DEV, HT_FUNC)
-/* Address Maps */ +/* Address Maps + * Device IDs + * ADDR_DEVID for merlinfalcon 0x1571 + * ADDR_DEVID for stoneyridge 0x15b1 + */ #define ADDR_DEV 0x18 #define ADDR_FUNC 1 -#define ADDR_DEVID 0x15b1 #define ADDR_DEVFN PCI_DEVFN(ADDR_DEV, ADDR_FUNC) #define SOC_ADDR_DEV _SOC_DEV(ADDR_DEV, ADDR_FUNC)
-/* DRAM Configuration */ +/* DRAM Configuration + * Device IDs + * DCT_DEVID for merlinfalcon 0x1572 + * DCT_DEVID for stoneyridge 0x15b2 + */ #define DCT_DEV 0x18 #define DCT_FUNC 2 -#define DCT_DEVID 0x15b2 #define DCT_DEVFN PCI_DEVFN(DCT_DEV, DCT_FUNC) #define SOC_DCT_DEV _SOC_DEV(DCT_DEV, DCT_FUNC)
-/* Misc. Configuration */ +/* Misc. Configuration + * Device IDs + * MISC_DEVID for merlinfalcon 0x1573 + * MISC_DEVID for stoneyridge 0x15b3 + */ #define MISC_DEV 0x18 #define MISC_FUNC 3 -#define MISC_DEVID 0x15b3 #define MISC_DEVFN PCI_DEVFN(MISC_DEV, MISC_FUNC) #define SOC_MISC_DEV _SOC_DEV(MISC_DEV, MISC_FUNC)
-/* PM Configuration */ +/* PM Configuration + * Device IDs + * PM_DEVID for merlinfalcon 0x1574 + * PM_DEVID for stoneyridge 0x15b4 + */ #define PM_DEV 0x18 #define PM_FUNC 4 -#define PM_DEVID 0x15b4 #define PM_DEVFN PCI_DEVFN(PM_DEV, PM_FUNC) #define SOC_PM_DEV _SOC_DEV(PM_DEV, PM_FUNC)
-/* Northbridge Configuration */ +/* Northbridge Configuration + * Device IDs + * NB_DEVID for merlinfalcon 0x1575 + * NB_DEVID for stoneyridge 0x15b5 + */ #define NB_DEV 0x18 #define NB_FUNC 5 -#define NB_DEVID 0x15b5 #define NB_DEVFN PCI_DEVFN(NB_DEV, NB_FUNC) #define SOC_NB_DEV _SOC_DEV(NB_DEV, NB_FUNC)
diff --git a/src/soc/amd/stoneyridge/northbridge.c b/src/soc/amd/stoneyridge/northbridge.c index 5985832..044a1b0 100644 --- a/src/soc/amd/stoneyridge/northbridge.c +++ b/src/soc/amd/stoneyridge/northbridge.c @@ -347,10 +347,15 @@ .ops_pci = 0, };
+static const unsigned short pci_device_ids[] = { + PCI_DEVICE_ID_AMD_15H_MODEL_606F_NB_HT, + PCI_DEVICE_ID_AMD_15H_MODEL_707F_NB_HT, + 0 }; + static const struct pci_driver family15_northbridge __pci_driver = { .ops = &northbridge_operations, .vendor = PCI_VENDOR_ID_AMD, - .device = PCI_DEVICE_ID_AMD_15H_MODEL_707F_NB_HT, + .devices = pci_device_ids, };
/* @@ -464,9 +469,13 @@ u32 map_oprom_vendev(u32 vendev) { u32 new_vendev; - new_vendev = - ((vendev >= 0x100298e0) && (vendev <= 0x100298ef)) ? - 0x100298e0 : vendev; + + if ((vendev >= 0x100298e0) && (vendev <= 0x100298ef)) + new_vendev = 0x100298e0; + else if ((vendev >= 0x10029870) && (vendev <= 0x1002987f)) + new_vendev = 0x10029870; + else + new_vendev = vendev;
if (vendev != new_vendev) printk(BIOS_NOTICE, "Mapping PCI device %8x to %8x\n",
-
Marshall Dawson (Code Review) -
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Richard Spiegel (Code Review)