Change in coreboot[master]: soc/intel/xeon_sp: Don't add MC resource twice
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. */ -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 1 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-MessageType: newchange
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. -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 1 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Comment-Date: Tue, 03 Nov 2020 17:37:23 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 1 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Comment-Date: Tue, 03 Nov 2020 17:49:31 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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? -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 1 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Tue, 03 Nov 2020 18:34:57 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
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. -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 1 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Tue, 03 Nov 2020 18:50:01 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 1 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Thu, 05 Nov 2020 00:41:47 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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. -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 1 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Thu, 05 Nov 2020 10:21:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 2 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: newpatchset
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
-- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 2 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Fri, 06 Nov 2020 01:05:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 2 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Fri, 06 Nov 2020 01:44:44 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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? -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 2 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Fri, 06 Nov 2020 05:45:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 2 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Fri, 06 Nov 2020 15:24:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 3 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: newpatchset
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
-- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 3 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Fri, 06 Nov 2020 17:52:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Arthur Heymans <arthur@aheymans.xyz> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 3 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Fri, 06 Nov 2020 17:54:50 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 4 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: newpatchset
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? -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 4 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Fri, 06 Nov 2020 18:04:05 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 5 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: newpatchset
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
-- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 5 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Sat, 07 Nov 2020 16:37:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-MessageType: comment
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 5 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Sun, 08 Nov 2020 15:40:47 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
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. */ -- To view, visit https://review.coreboot.org/c/coreboot/+/47173 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: Ia336f604441ae8d30b8418300da7c34ab9907cae Gerrit-Change-Number: 47173 Gerrit-PatchSet: 7 Gerrit-Owner: Marc Jones <marc@marcjonesconsulting.com> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Jay Talbott <JayTalbott@sysproconsulting.com> Gerrit-Reviewer: Jonathan Zhang <jonzhang@fb.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer@coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: merged
participants (7)
-
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)