Marc Jones has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add MC resource twice ......................................................................
soc/intel/xeon_sp: Don't add MC resource twice
Don't add the MC resources for each multiple sockets/CPUs. Only do it for the boot CPU.
Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/soc/intel/xeon_sp/uncore.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/47173/1
diff --git a/src/soc/intel/xeon_sp/uncore.c b/src/soc/intel/xeon_sp/uncore.c index 332b9a4..15a9f0f 100644 --- a/src/soc/intel/xeon_sp/uncore.c +++ b/src/soc/intel/xeon_sp/uncore.c @@ -155,6 +155,10 @@ struct resource *resource; int index = *res_count;
+ /* Only add dram resources once. */ + if (dev->bus->secondary != 0) + return; + fsp_find_reserved_memory(&fsp_mem);
/* Read in the MAP registers and report their values. */
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add MC resource twice ......................................................................
Patch Set 1:
I think the better fix would be to have this as part of a chip ops instead a __pci_driver.
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add MC resource twice ......................................................................
Patch Set 1: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add MC resource twice ......................................................................
Patch Set 1:
How does this actually work? Do both memory controllers have the same memory map set up? Do they align somehow on things like TSEG, or does each have its own TSEG relative to its own DRAM?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add MC resource twice ......................................................................
Patch Set 1:
Patch Set 1:
How does this actually work? Do both memory controllers have the same memory map set up? Do they align somehow on things like TSEG, or does each have its own TSEG relative to its own DRAM?
I would expect that multi-socket systems only expose one copy of these registers. I don't have a multi-socket system available nearby, though.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add MC resource twice ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
In this case the values are actually coming from FSP, and not directly read from hardware. They are somewhat arbitrarily tied to the MC device (of which there is one per CPU) because it makes sense to have memory sizing be owned by the memory controller. But in this instance there is no actual hardware touched.
I don't have the specs to this memory controller in front of me, but normally these sorts of systems have to have each processor configured with the same mappings (that is what we used to do on the original AMD64 multi processor systems, too)
https://review.coreboot.org/c/coreboot/+/47173/1/src/soc/intel/xeon_sp/uncor... File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/47173/1/src/soc/intel/xeon_sp/uncor... PS1, Line 186: meomoy memory
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add MC resource twice ......................................................................
Patch Set 1:
(1 comment)
In this case the values are actually coming from FSP, and not directly read from hardware. They are somewhat arbitrarily tied to the MC device (of which there is one per CPU) because it makes sense to have memory sizing be owned by the memory controller. But in this instance there is no actual hardware touched.
I'm not convinced, see inline comment.
https://review.coreboot.org/c/coreboot/+/47173/1/src/soc/intel/xeon_sp/uncor... File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/47173/1/src/soc/intel/xeon_sp/uncor... PS1, Line 66: value |= (uint64_t)pci_read_config32(dev, entry->reg); This looks like a hardware access to me.
Hello build bot (Jenkins), Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47173
to look at the new patch set (#2).
Change subject: soc/intel/xeon_sp: Don't add memory resource twice ......................................................................
soc/intel/xeon_sp: Don't add memory resource twice
Only add the imemory resource map from the boot CPU, not for each socket/CPU. This is a NUMA architecture and has a shared memory map. All the resources must match across the sockets/CPUs.
Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/soc/intel/xeon_sp/uncore.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/47173/2
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add memory resource twice ......................................................................
Patch Set 2:
(1 comment)
I updated the commit message. I hope it clarifies the change. The resource map is the same for all cpus. Don't add the same map multiple times.
https://review.coreboot.org/c/coreboot/+/47173/1/src/soc/intel/xeon_sp/uncor... File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/47173/1/src/soc/intel/xeon_sp/uncor... PS1, Line 66: value |= (uint64_t)pci_read_config32(dev, entry->reg);
This looks like a hardware access to me.
ack
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add memory resource twice ......................................................................
Patch Set 2: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add memory resource twice ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47173/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47173/2//COMMIT_MSG@7 PS2, Line 7: Don't add memory resource twice Maybe the commit message can state that with multiple CPUs there are more than one PCI devices with the same VID/DID (that report these resources), but that they will appear on different busses?
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add memory resource twice ......................................................................
Patch Set 2: Code-Review+1
Hello build bot (Jenkins), Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47173
to look at the new patch set (#3).
Change subject: soc/intel/xeon_sp: Don't add memory resource twice ......................................................................
soc/intel/xeon_sp: Don't add memory resource twice
Only add the memory resource map from the boot CPU, not for each socket/CPU. This is a NUMA architecture and has a shared memory map. All the resources must match across the sockets/CPUs, so they should only be added to the map once.
Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/soc/intel/xeon_sp/uncore.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/47173/3
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add memory resource twice ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47173/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47173/2//COMMIT_MSG@7 PS2, Line 7: Don't add memory resource twice
Maybe the commit message can state that with multiple CPUs there are more than one PCI devices with […]
Done
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add memory resource twice ......................................................................
Patch Set 3: Code-Review+2
Hello build bot (Jenkins), Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47173
to look at the new patch set (#4).
Change subject: soc/intel/xeon_sp: Don't add memory resource twice ......................................................................
soc/intel/xeon_sp: Don't add memory resource twice
The resource function is called for each device VID/DID. Only add the memory resource map from the boot CPUi (bus 0) and not for each socket/CPU. This is a NUMA architecture and has a shared memory map. All the resources must match across the sockets/CPUs, so they should only be added to the map once.
Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/soc/intel/xeon_sp/uncore.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/47173/4
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add memory resource twice ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47173/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47173/4//COMMIT_MSG@10 PS4, Line 10: i typo?
Hello build bot (Jenkins), Jonathan Zhang, Jay Talbott, Stefan Reinauer, Angel Pons, Arthur Heymans, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47173
to look at the new patch set (#5).
Change subject: soc/intel/xeon_sp: Don't add memory resource twice ......................................................................
soc/intel/xeon_sp: Don't add memory resource twice
The resource function is called for each device VID/DID. Only add the memory resource map from the boot CPU (bus 0) and not for each socket/CPU. This is a NUMA architecture and has a shared memory map. All the resources must match across the sockets/CPUs, so they should only be added to the map once.
Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/soc/intel/xeon_sp/uncore.c 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/73/47173/5
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add memory resource twice ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47173/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47173/4//COMMIT_MSG@10 PS4, Line 10: i
typo?
Ack
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add memory resource twice ......................................................................
Patch Set 5: Code-Review+1
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47173 )
Change subject: soc/intel/xeon_sp: Don't add memory resource twice ......................................................................
soc/intel/xeon_sp: Don't add memory resource twice
The resource function is called for each device VID/DID. Only add the memory resource map from the boot CPU (bus 0) and not for each socket/CPU. This is a NUMA architecture and has a shared memory map. All the resources must match across the sockets/CPUs, so they should only be added to the map once.
Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Signed-off-by: Marc Jones marcjones@sysproconsulting.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47173 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Jay Talbott JayTalbott@sysproconsulting.com Reviewed-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/xeon_sp/uncore.c 1 file changed, 4 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved Jay Talbott: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/xeon_sp/uncore.c b/src/soc/intel/xeon_sp/uncore.c index 332b9a4..15a9f0f 100644 --- a/src/soc/intel/xeon_sp/uncore.c +++ b/src/soc/intel/xeon_sp/uncore.c @@ -155,6 +155,10 @@ struct resource *resource; int index = *res_count;
+ /* Only add dram resources once. */ + if (dev->bus->secondary != 0) + return; + fsp_find_reserved_memory(&fsp_mem);
/* Read in the MAP registers and report their values. */
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Jay Talbott (Code Review)
-
Marc Jones (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Stefan Reinauer (Code Review)