Hello Anjaneya "Reddy" Chagam,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/42439
to review the following change.
Change subject: soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count() ......................................................................
soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count()
Rename function from xeon_sp_get_cpu_count() to xeon_sp_get_sku_count(). The function returns CPU socket count, by getting it from the field named as numCpus in FSP HOB.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Reddy Chagam anjaneya.chagam@intel.com Change-Id: Ic96bdf4ab042ac15d43f9b636185627c63fbf8a1 --- M src/soc/intel/xeon_sp/cpx/acpi.c M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/soc/intel/xeon_sp/cpx/soc_util.c 3 files changed, 8 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/42439/1
diff --git a/src/soc/intel/xeon_sp/cpx/acpi.c b/src/soc/intel/xeon_sp/cpx/acpi.c index 33c85ca..654f761 100644 --- a/src/soc/intel/xeon_sp/cpx/acpi.c +++ b/src/soc/intel/xeon_sp/cpx/acpi.c @@ -454,7 +454,7 @@
static unsigned long acpi_fill_slit(unsigned long current) { - unsigned int nodes = xeon_sp_get_cpu_count(); + unsigned int nodes = xeon_sp_get_skt_count();
uint8_t *p = (uint8_t *)current; memset(p, 0, 8 + nodes * nodes); diff --git a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h index ee2b68b..1bcd222 100644 --- a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h +++ b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h @@ -34,7 +34,8 @@ void get_core_thread_bits(uint32_t *core_bits, uint32_t *thread_bits); void get_cpu_info_from_apicid(uint32_t apicid, uint32_t core_bits, uint32_t thread_bits, uint8_t *package, uint8_t *core, uint8_t *thread); -unsigned int xeon_sp_get_cpu_count(void); +/* Return socket count, as obtained from FSP HOB */ +unsigned int xeon_sp_get_skt_count(void);
int get_platform_thread_count(void); int get_threads_per_package(void); diff --git a/src/soc/intel/xeon_sp/cpx/soc_util.c b/src/soc/intel/xeon_sp/cpx/soc_util.c index 9837cd9..09237d0 100644 --- a/src/soc/intel/xeon_sp/cpx/soc_util.c +++ b/src/soc/intel/xeon_sp/cpx/soc_util.c @@ -22,7 +22,7 @@
int get_platform_thread_count(void) { - return xeon_sp_get_cpu_count() * get_threads_per_package(); + return xeon_sp_get_skt_count() * get_threads_per_package(); }
const struct SystemMemoryMapHob *get_system_memory_map(void) @@ -82,8 +82,9 @@ return hob; }
-unsigned int xeon_sp_get_cpu_count(void) +unsigned int xeon_sp_get_skt_count(void) { + /* The FSP IIO UDS HOB has field numCpus, it is actually socket count */ return get_iio_uds()->SystemStatus.numCpus; }
@@ -114,9 +115,7 @@ if (num_apics > 1) bubblesort(apic_ids, num_apics, NUM_ASCENDING);
- /* Here num_cpus is the number of processors */ - /* The FSP HOB parameter has it named as num_cpus */ - num_cpus = xeon_sp_get_cpu_count(); + num_cpus = xeon_sp_get_skt_count(); cpu_read_topology(&core_count, &thread_count); assert(num_apics == (num_cpus * thread_count));
@@ -307,7 +306,7 @@ * According to the BIOS writer's guide, this needs to be set on non-SBSP * first, before set on SBSP. */ - for (uint32_t socket = 0; socket < xeon_sp_get_cpu_count(); ++socket) { + for (uint32_t socket = 0; socket < xeon_sp_get_skt_count(); ++socket) { if (socket == sbsp_socket_id) continue; set_bios_init_completion_for_package(socket);
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42439 )
Change subject: soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count() ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42439/2/src/soc/intel/xeon_sp/cpx/s... File src/soc/intel/xeon_sp/cpx/soc_util.c:
https://review.coreboot.org/c/coreboot/+/42439/2/src/soc/intel/xeon_sp/cpx/s... PS2, Line 85: skt why not 'socket' ?
Hello Philipp Deppenwiese, build bot (Jenkins), Anjaneya "Reddy" Chagam, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42439
to look at the new patch set (#3).
Change subject: soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count() ......................................................................
soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count()
Rename function from xeon_sp_get_cpu_count() to xeon_sp_get_socket_count().
This function returns CPU socket count, by getting it from the field named as numCpus in FSP HOB.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Reddy Chagam anjaneya.chagam@intel.com Change-Id: Ic96bdf4ab042ac15d43f9b636185627c63fbf8a1 --- M src/soc/intel/xeon_sp/cpx/acpi.c M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/soc/intel/xeon_sp/cpx/soc_util.c 3 files changed, 8 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/42439/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42439 )
Change subject: soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count() ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42439/4/src/soc/intel/xeon_sp/cpx/s... File src/soc/intel/xeon_sp/cpx/soc_util.c:
https://review.coreboot.org/c/coreboot/+/42439/4/src/soc/intel/xeon_sp/cpx/s... PS4, Line 118: num_cpus This variable should be renamed as well, to `num_sockets`
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42439 )
Change subject: soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count() ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42439/4/src/soc/intel/xeon_sp/cpx/s... File src/soc/intel/xeon_sp/cpx/soc_util.c:
https://review.coreboot.org/c/coreboot/+/42439/4/src/soc/intel/xeon_sp/cpx/s... PS4, Line 118: num_cpus
This variable should be renamed as well, to `num_sockets`
Makes total sense. Thanks for pointing this out.
Hello Philipp Deppenwiese, build bot (Jenkins), Anjaneya "Reddy" Chagam, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42439
to look at the new patch set (#6).
Change subject: soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count() ......................................................................
soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count()
Rename function from xeon_sp_get_cpu_count() to xeon_sp_get_socket_count().
This function returns CPU socket count, by getting it from the field named as numCpus in FSP HOB.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Reddy Chagam anjaneya.chagam@intel.com Change-Id: Ic96bdf4ab042ac15d43f9b636185627c63fbf8a1 --- M src/soc/intel/xeon_sp/cpx/acpi.c M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/soc/intel/xeon_sp/cpx/soc_util.c 3 files changed, 10 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/42439/6
Hello Philipp Deppenwiese, build bot (Jenkins), Anjaneya "Reddy" Chagam, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42439
to look at the new patch set (#7).
Change subject: soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count() ......................................................................
soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count()
Rename function from xeon_sp_get_cpu_count() to xeon_sp_get_socket_count().
This function returns CPU socket count, by getting it from the field named as numCpus in FSP HOB.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Reddy Chagam anjaneya.chagam@intel.com Change-Id: Ic96bdf4ab042ac15d43f9b636185627c63fbf8a1 --- M src/soc/intel/xeon_sp/cpx/acpi.c M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/soc/intel/xeon_sp/cpx/soc_util.c 3 files changed, 10 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/42439/7
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42439 )
Change subject: soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count() ......................................................................
Patch Set 7: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42439/2/src/soc/intel/xeon_sp/cpx/s... File src/soc/intel/xeon_sp/cpx/soc_util.c:
https://review.coreboot.org/c/coreboot/+/42439/2/src/soc/intel/xeon_sp/cpx/s... PS2, Line 85: skt
why not 'socket' ?
Done
Hello Philipp Deppenwiese, build bot (Jenkins), Anjaneya "Reddy" Chagam, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42439
to look at the new patch set (#8).
Change subject: soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count() ......................................................................
soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count()
Rename function from xeon_sp_get_cpu_count() to xeon_sp_get_socket_count().
This function returns CPU socket count, by getting it from the field named as numCpus in FSP HOB.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Reddy Chagam anjaneya.chagam@intel.com Change-Id: Ic96bdf4ab042ac15d43f9b636185627c63fbf8a1 --- M src/soc/intel/xeon_sp/cpx/acpi.c M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/soc/intel/xeon_sp/cpx/soc_util.c 3 files changed, 10 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/42439/8
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42439 )
Change subject: soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count() ......................................................................
soc/intel/xeon_sp/cpx: rename xeon_sp_get_cpu_count()
Rename function from xeon_sp_get_cpu_count() to xeon_sp_get_socket_count().
This function returns CPU socket count, by getting it from the field named as numCpus in FSP HOB.
Signed-off-by: Jonathan Zhang jonzhang@fb.com Signed-off-by: Reddy Chagam anjaneya.chagam@intel.com Change-Id: Ic96bdf4ab042ac15d43f9b636185627c63fbf8a1 Reviewed-on: https://review.coreboot.org/c/coreboot/+/42439 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/xeon_sp/cpx/acpi.c M src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h M src/soc/intel/xeon_sp/cpx/soc_util.c 3 files changed, 10 insertions(+), 10 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/soc/intel/xeon_sp/cpx/acpi.c b/src/soc/intel/xeon_sp/cpx/acpi.c index 16663b0..abaa453 100644 --- a/src/soc/intel/xeon_sp/cpx/acpi.c +++ b/src/soc/intel/xeon_sp/cpx/acpi.c @@ -449,7 +449,7 @@
static unsigned long acpi_fill_slit(unsigned long current) { - unsigned int nodes = xeon_sp_get_cpu_count(); + unsigned int nodes = xeon_sp_get_socket_count();
uint8_t *p = (uint8_t *)current; memset(p, 0, 8 + nodes * nodes); diff --git a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h index 2d33d0e..412730b 100644 --- a/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h +++ b/src/soc/intel/xeon_sp/cpx/include/soc/soc_util.h @@ -34,7 +34,8 @@ void get_core_thread_bits(uint32_t *core_bits, uint32_t *thread_bits); void get_cpu_info_from_apicid(uint32_t apicid, uint32_t core_bits, uint32_t thread_bits, uint8_t *package, uint8_t *core, uint8_t *thread); -unsigned int xeon_sp_get_cpu_count(void); +/* Return socket count, as obtained from FSP HOB */ +unsigned int xeon_sp_get_socket_count(void);
int get_platform_thread_count(void); int get_threads_per_package(void); diff --git a/src/soc/intel/xeon_sp/cpx/soc_util.c b/src/soc/intel/xeon_sp/cpx/soc_util.c index f7cebb5..fb78910 100644 --- a/src/soc/intel/xeon_sp/cpx/soc_util.c +++ b/src/soc/intel/xeon_sp/cpx/soc_util.c @@ -22,7 +22,7 @@
int get_platform_thread_count(void) { - return xeon_sp_get_cpu_count() * get_threads_per_package(); + return xeon_sp_get_socket_count() * get_threads_per_package(); }
const struct SystemMemoryMapHob *get_system_memory_map(void) @@ -82,8 +82,9 @@ return hob; }
-unsigned int xeon_sp_get_cpu_count(void) +unsigned int xeon_sp_get_socket_count(void) { + /* The FSP IIO UDS HOB has field numCpus, it is actually socket count */ return get_iio_uds()->SystemStatus.numCpus; }
@@ -94,7 +95,7 @@ int num_apics = 0; uint32_t core_bits, thread_bits; unsigned int core_count, thread_count; - unsigned int num_cpus; + unsigned int num_sockets;
/* * sort APIC ids in asending order to identify apicid ranges for @@ -114,11 +115,9 @@ if (num_apics > 1) bubblesort(apic_ids, num_apics, NUM_ASCENDING);
- /* Here num_cpus is the number of processors */ - /* The FSP HOB parameter has it named as num_cpus */ - num_cpus = xeon_sp_get_cpu_count(); + num_sockets = xeon_sp_get_socket_count(); cpu_read_topology(&core_count, &thread_count); - assert(num_apics == (num_cpus * thread_count)); + assert(num_apics == (num_sockets * thread_count));
/* sort them by thread i.e., all cores with thread 0 and then thread 1 */ int index = 0; @@ -308,7 +307,7 @@ * to receive the BIOS init completion message. So, we send it to all non-SBSP * sockets first. */ - for (uint32_t socket = 0; socket < xeon_sp_get_cpu_count(); ++socket) { + for (uint32_t socket = 0; socket < xeon_sp_get_socket_count(); ++socket) { if (socket == sbsp_socket_id) continue; set_bios_init_completion_for_package(socket);
-
Angel Pons (Code Review) -
Jonathan Zhang (Code Review)
-
Patrick Georgi (Code Review)