Marc Jones has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46599 )
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes
Prepare for common ACPI. This primarily makes the skx madt table generation match cpx. There are a few other small changes to make the files match.
Change-Id: I71a59181226d79c40a4af405653c50c970fb720b Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/soc/intel/xeon_sp/skx/acpi.c 1 file changed, 60 insertions(+), 60 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/46599/1
diff --git a/src/soc/intel/xeon_sp/skx/acpi.c b/src/soc/intel/xeon_sp/skx/acpi.c index c14e7c8..1e760d3 100644 --- a/src/soc/intel/xeon_sp/skx/acpi.c +++ b/src/soc/intel/xeon_sp/skx/acpi.c @@ -24,7 +24,7 @@ static int acpi_sci_irq(void) { int sci_irq = 9; - int32_t scis; + uint32_t scis;
scis = soc_read_sci_irq_select(); scis &= SCI_IRQ_SEL; @@ -74,55 +74,50 @@
unsigned long acpi_fill_madt(unsigned long current) { - size_t hob_size = 0; - const uint8_t fsp_hob_iio_universal_data_guid[16] = - FSP_HOB_IIO_UNIVERSAL_DATA_GUID; - const IIO_UDS *hob; - int cur_stack; + int cur_index; + struct iiostack_resource stack_info = {0};
+ /* With XEON-SP FSP, PCH IOAPIC is allocated with first 120 GSIs. */ int gsi_bases[] = { 0, 0x18, 0x20, 0x28, 0x30, 0x48, 0x50, 0x58, 0x60 }; int ioapic_ids[] = { 0x8, 0x9, 0xa, 0xb, 0xc, 0xf, 0x10, 0x11, 0x12 };
/* Local APICs */ current = xeonsp_acpi_create_madt_lapics(current);
- hob = fsp_find_extension_hob_by_guid(fsp_hob_iio_universal_data_guid, &hob_size); - assert(hob != NULL && hob_size != 0); + cur_index = 0; + get_iiostack_info(&stack_info);
- cur_stack = 0; - for (int socket = 0; socket < hob->PlatformData.numofIIO; ++socket) { - for (int stack = 0; stack < MAX_IIO_STACK; ++stack) { - const STACK_RES *ri = - &hob->PlatformData.IIO_resource[socket].StackRes[stack]; - // TODO: do we have situation with only bus 0 and one stack? - if (ri->BusBase != ri->BusLimit) { - assert(cur_stack < ARRAY_SIZE(ioapic_ids)); - assert(cur_stack < ARRAY_SIZE(gsi_bases)); - int ioapic_id = ioapic_ids[cur_stack]; - int gsi_base = gsi_bases[cur_stack]; - printk(BIOS_DEBUG, "Adding MADT IOAPIC for socket: %d, stack: %d, ioapic_id: 0x%x, " - "ioapic_base: 0x%x, gsi_base: 0x%x\n", - socket, stack, ioapic_id, ri->IoApicBase, gsi_base); - current += acpi_create_madt_ioapic( - (acpi_madt_ioapic_t *)current, - ioapic_id, ri->IoApicBase, gsi_base); - ++cur_stack; + for (int stack = 0; stack < stack_info.no_of_stacks; ++stack) { + const STACK_RES *ri = &stack_info.res[stack]; + assert(cur_index < ARRAY_SIZE(ioapic_ids)); + assert(cur_index < ARRAY_SIZE(gsi_bases)); + int ioapic_id = ioapic_ids[cur_index]; + int gsi_base = gsi_bases[cur_index]; + printk(BIOS_DEBUG, "Adding MADT IOAPIC for stack: %d, ioapic_id: 0x%x, " + "ioapic_base: 0x%x, gsi_base: 0x%x\n", + stack, ioapic_id, ri->IoApicBase, gsi_base); + current += acpi_create_madt_ioapic( + (acpi_madt_ioapic_t *)current, + ioapic_id, ri->IoApicBase, gsi_base); + ++cur_index;
- if (socket == 0 && stack == 0) { - assert(cur_stack < ARRAY_SIZE(ioapic_ids)); - assert(cur_stack < ARRAY_SIZE(gsi_bases)); - ioapic_id = ioapic_ids[cur_stack]; - gsi_base = gsi_bases[cur_stack]; - printk(BIOS_DEBUG, "Adding MADT IOAPIC for socket: %d, stack: %d, ioapic_id: 0x%x, " - "ioapic_base: 0x%x, gsi_base: 0x%x\n", - socket, stack, ioapic_id, - ri->IoApicBase + 0x1000, gsi_base); - current += acpi_create_madt_ioapic( - (acpi_madt_ioapic_t *)current, - ioapic_id, ri->IoApicBase + 0x1000, gsi_base); - ++cur_stack; - } - } + /* + * Stack 0 has non-PCH IOAPIC and PCH IOAPIC. + * Add entry for PCH IOAPIC. + */ + if (stack == 0) { /* PCH IOAPIC */ + assert(cur_index < ARRAY_SIZE(ioapic_ids)); + assert(cur_index < ARRAY_SIZE(gsi_bases)); + ioapic_id = ioapic_ids[cur_index]; + gsi_base = gsi_bases[cur_index]; + printk(BIOS_DEBUG, "Adding MADT IOAPIC for stack: %d, ioapic_id: 0x%x, " + "ioapic_base: 0x%x, gsi_base: 0x%x\n", + stack, ioapic_id, + ri->IoApicBase + 0x1000, gsi_base); + current += acpi_create_madt_ioapic( + (acpi_madt_ioapic_t *)current, + ioapic_id, ri->IoApicBase + 0x1000, gsi_base); + ++cur_index; } }
@@ -220,27 +215,31 @@ } }
+int calculate_power(int tdp, int p1_ratio, int ratio) +{ + u32 m; + u32 power;
+ /* + * M = ((1.1 - ((p1_ratio - ratio) * 0.00625)) / 1.1) ^ 2 + * + * Power = (ratio / p1_ratio) * m * tdp + */
-static acpi_tstate_t xeon_sp_tss_table[] = { - { 100, 1000, 0, 0x00, 0 }, - { 88, 875, 0, 0x1e, 0 }, - { 75, 750, 0, 0x1c, 0 }, - { 63, 625, 0, 0x1a, 0 }, - { 50, 500, 0, 0x18, 0 }, - { 38, 375, 0, 0x16, 0 }, - { 25, 250, 0, 0x14, 0 }, - { 13, 125, 0, 0x12, 0 }, -}; + m = (110000 - ((p1_ratio - ratio) * 625)) / 11; + m = (m * m) / 1000; + + power = ((ratio * 100000 / p1_ratio) / 100); + power *= (m / 100) * (tdp / 1000); + power /= 1000; + + return (int)power; +}
acpi_tstate_t *soc_get_tss_table(int *entries) { - *entries = ARRAY_SIZE(xeon_sp_tss_table); - return xeon_sp_tss_table; -} - -void generate_t_state_entries(int core, int cores_per_package) -{ + *entries = 0; + return NULL; }
void generate_cpu_entries(const struct device *device) @@ -267,12 +266,13 @@
/* NOTE: Intel idle driver doesn't use ACPI C-state tables */
- /* TODO: Soc specific power states generation */ + /* Soc specific power states generation */ + soc_power_states_generation(core_id, threads_per_package); + acpigen_pop_len(); } } - /* PPKG is usually used for thermal management - of the first and only package. */ + /* PPKG is usually used for thermal management of the first and only package. */ acpigen_write_processor_package("PPKG", 0, threads_per_package);
/* Add a method to notify processor nodes */
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46599 )
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46599/1/src/soc/intel/xeon_sp/skx/a... File src/soc/intel/xeon_sp/skx/acpi.c:
https://review.coreboot.org/c/coreboot/+/46599/1/src/soc/intel/xeon_sp/skx/a... PS1, Line 93: for (int socket = 0; socket < hob->PlatformData.numofIIO; ++socket) { Where did the for-socket loop go?
https://review.coreboot.org/c/coreboot/+/46599/1/src/soc/intel/xeon_sp/skx/a... PS1, Line 242: return NULL; Huh? Is this intentional?
It could also be that the code for SKX-SP was copy-pasta'd from Skylake and is wrong, in which case I'd prefer to remove this on a separate commit (so that the commit message can explain why it is being removed).
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46599 )
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46599/1/src/soc/intel/xeon_sp/skx/a... File src/soc/intel/xeon_sp/skx/acpi.c:
https://review.coreboot.org/c/coreboot/+/46599/1/src/soc/intel/xeon_sp/skx/a... PS1, Line 93: for (int socket = 0; socket < hob->PlatformData.numofIIO; ++socket) {
Where did the for-socket loop go?
Into the get_iiostack_info call.
https://review.coreboot.org/c/coreboot/+/46599/1/src/soc/intel/xeon_sp/skx/a... PS1, Line 242: return NULL;
Huh? Is this intentional? […]
It was never called. void generate_t_state_entries(int core, int cores_per_package) was empty. It isn't called used in cpx either. We really don't need more steps to the next patch to combine these files.
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46599 )
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
Patch Set 1:
Wait for CB:46503, which up dates the madt generation.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46599 )
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46599/1/src/soc/intel/xeon_sp/skx/a... File src/soc/intel/xeon_sp/skx/acpi.c:
https://review.coreboot.org/c/coreboot/+/46599/1/src/soc/intel/xeon_sp/skx/a... PS1, Line 242: return NULL;
It was never called. void generate_t_state_entries(int core, int cores_per_package) was empty. […]
Oh, it is unused. I see.
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46599
to look at the new patch set (#2).
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes
Prepare for common ACPI. This primarily makes the skx madt table generation match cpx. There are a few other small changes to remove unused code and make the files match.
Change-Id: I71a59181226d79c40a4af405653c50c970fb720b Signed-off-by: Marc Jones marcjones@sysproconsulting.com --- M src/soc/intel/xeon_sp/skx/acpi.c 1 file changed, 60 insertions(+), 60 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/46599/2
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46599 )
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
Patch Set 2: Code-Review-1
Arthur's change causes boot failure. He is working on it.
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46599 )
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
Patch Set 2: Code-Review+1
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46599 )
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
Patch Set 2: -Code-Review
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46599 )
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
Patch Set 2:
Patch Set 2: Code-Review-1
Arthur's change causes boot failure. He is working on it.
Great, This doesn't have his change, just the original cpx code. We will update the merge when Arthur's change is ready.
Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46599 )
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
Patch Set 3:
Patch Set 2: Code-Review-1
Arthur's change causes boot failure. He is working on it.
Lets not hold up this change and the madt change can go on top of this.
Jay Talbott has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46599 )
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
Patch Set 3: Code-Review+1
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46599 )
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
Patch Set 3: Code-Review+2
Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46599 )
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
Patch Set 3: Code-Review+1
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46599 )
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
Patch Set 3: Code-Review+2
Marc Jones has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46599 )
Change subject: soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes ......................................................................
soc/intel/xeon_sp/skx/acpi.c: Update with cpx changes
Prepare for common ACPI. This primarily makes the skx madt table generation match cpx. There are a few other small changes to remove unused code and make the files match.
Change-Id: I71a59181226d79c40a4af405653c50c970fb720b Signed-off-by: Marc Jones marcjones@sysproconsulting.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46599 Reviewed-by: Jay Talbott JayTalbott@sysproconsulting.com Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Jonathan Zhang jonzhang@fb.com Reviewed-by: Stefan Reinauer stefan.reinauer@coreboot.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/xeon_sp/skx/acpi.c 1 file changed, 60 insertions(+), 60 deletions(-)
Approvals: build bot (Jenkins): Verified Stefan Reinauer: Looks good to me, approved Arthur Heymans: Looks good to me, approved Jay Talbott: Looks good to me, but someone else must approve Jonathan Zhang: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/xeon_sp/skx/acpi.c b/src/soc/intel/xeon_sp/skx/acpi.c index c14e7c8..1e760d3 100644 --- a/src/soc/intel/xeon_sp/skx/acpi.c +++ b/src/soc/intel/xeon_sp/skx/acpi.c @@ -24,7 +24,7 @@ static int acpi_sci_irq(void) { int sci_irq = 9; - int32_t scis; + uint32_t scis;
scis = soc_read_sci_irq_select(); scis &= SCI_IRQ_SEL; @@ -74,55 +74,50 @@
unsigned long acpi_fill_madt(unsigned long current) { - size_t hob_size = 0; - const uint8_t fsp_hob_iio_universal_data_guid[16] = - FSP_HOB_IIO_UNIVERSAL_DATA_GUID; - const IIO_UDS *hob; - int cur_stack; + int cur_index; + struct iiostack_resource stack_info = {0};
+ /* With XEON-SP FSP, PCH IOAPIC is allocated with first 120 GSIs. */ int gsi_bases[] = { 0, 0x18, 0x20, 0x28, 0x30, 0x48, 0x50, 0x58, 0x60 }; int ioapic_ids[] = { 0x8, 0x9, 0xa, 0xb, 0xc, 0xf, 0x10, 0x11, 0x12 };
/* Local APICs */ current = xeonsp_acpi_create_madt_lapics(current);
- hob = fsp_find_extension_hob_by_guid(fsp_hob_iio_universal_data_guid, &hob_size); - assert(hob != NULL && hob_size != 0); + cur_index = 0; + get_iiostack_info(&stack_info);
- cur_stack = 0; - for (int socket = 0; socket < hob->PlatformData.numofIIO; ++socket) { - for (int stack = 0; stack < MAX_IIO_STACK; ++stack) { - const STACK_RES *ri = - &hob->PlatformData.IIO_resource[socket].StackRes[stack]; - // TODO: do we have situation with only bus 0 and one stack? - if (ri->BusBase != ri->BusLimit) { - assert(cur_stack < ARRAY_SIZE(ioapic_ids)); - assert(cur_stack < ARRAY_SIZE(gsi_bases)); - int ioapic_id = ioapic_ids[cur_stack]; - int gsi_base = gsi_bases[cur_stack]; - printk(BIOS_DEBUG, "Adding MADT IOAPIC for socket: %d, stack: %d, ioapic_id: 0x%x, " - "ioapic_base: 0x%x, gsi_base: 0x%x\n", - socket, stack, ioapic_id, ri->IoApicBase, gsi_base); - current += acpi_create_madt_ioapic( - (acpi_madt_ioapic_t *)current, - ioapic_id, ri->IoApicBase, gsi_base); - ++cur_stack; + for (int stack = 0; stack < stack_info.no_of_stacks; ++stack) { + const STACK_RES *ri = &stack_info.res[stack]; + assert(cur_index < ARRAY_SIZE(ioapic_ids)); + assert(cur_index < ARRAY_SIZE(gsi_bases)); + int ioapic_id = ioapic_ids[cur_index]; + int gsi_base = gsi_bases[cur_index]; + printk(BIOS_DEBUG, "Adding MADT IOAPIC for stack: %d, ioapic_id: 0x%x, " + "ioapic_base: 0x%x, gsi_base: 0x%x\n", + stack, ioapic_id, ri->IoApicBase, gsi_base); + current += acpi_create_madt_ioapic( + (acpi_madt_ioapic_t *)current, + ioapic_id, ri->IoApicBase, gsi_base); + ++cur_index;
- if (socket == 0 && stack == 0) { - assert(cur_stack < ARRAY_SIZE(ioapic_ids)); - assert(cur_stack < ARRAY_SIZE(gsi_bases)); - ioapic_id = ioapic_ids[cur_stack]; - gsi_base = gsi_bases[cur_stack]; - printk(BIOS_DEBUG, "Adding MADT IOAPIC for socket: %d, stack: %d, ioapic_id: 0x%x, " - "ioapic_base: 0x%x, gsi_base: 0x%x\n", - socket, stack, ioapic_id, - ri->IoApicBase + 0x1000, gsi_base); - current += acpi_create_madt_ioapic( - (acpi_madt_ioapic_t *)current, - ioapic_id, ri->IoApicBase + 0x1000, gsi_base); - ++cur_stack; - } - } + /* + * Stack 0 has non-PCH IOAPIC and PCH IOAPIC. + * Add entry for PCH IOAPIC. + */ + if (stack == 0) { /* PCH IOAPIC */ + assert(cur_index < ARRAY_SIZE(ioapic_ids)); + assert(cur_index < ARRAY_SIZE(gsi_bases)); + ioapic_id = ioapic_ids[cur_index]; + gsi_base = gsi_bases[cur_index]; + printk(BIOS_DEBUG, "Adding MADT IOAPIC for stack: %d, ioapic_id: 0x%x, " + "ioapic_base: 0x%x, gsi_base: 0x%x\n", + stack, ioapic_id, + ri->IoApicBase + 0x1000, gsi_base); + current += acpi_create_madt_ioapic( + (acpi_madt_ioapic_t *)current, + ioapic_id, ri->IoApicBase + 0x1000, gsi_base); + ++cur_index; } }
@@ -220,27 +215,31 @@ } }
+int calculate_power(int tdp, int p1_ratio, int ratio) +{ + u32 m; + u32 power;
+ /* + * M = ((1.1 - ((p1_ratio - ratio) * 0.00625)) / 1.1) ^ 2 + * + * Power = (ratio / p1_ratio) * m * tdp + */
-static acpi_tstate_t xeon_sp_tss_table[] = { - { 100, 1000, 0, 0x00, 0 }, - { 88, 875, 0, 0x1e, 0 }, - { 75, 750, 0, 0x1c, 0 }, - { 63, 625, 0, 0x1a, 0 }, - { 50, 500, 0, 0x18, 0 }, - { 38, 375, 0, 0x16, 0 }, - { 25, 250, 0, 0x14, 0 }, - { 13, 125, 0, 0x12, 0 }, -}; + m = (110000 - ((p1_ratio - ratio) * 625)) / 11; + m = (m * m) / 1000; + + power = ((ratio * 100000 / p1_ratio) / 100); + power *= (m / 100) * (tdp / 1000); + power /= 1000; + + return (int)power; +}
acpi_tstate_t *soc_get_tss_table(int *entries) { - *entries = ARRAY_SIZE(xeon_sp_tss_table); - return xeon_sp_tss_table; -} - -void generate_t_state_entries(int core, int cores_per_package) -{ + *entries = 0; + return NULL; }
void generate_cpu_entries(const struct device *device) @@ -267,12 +266,13 @@
/* NOTE: Intel idle driver doesn't use ACPI C-state tables */
- /* TODO: Soc specific power states generation */ + /* Soc specific power states generation */ + soc_power_states_generation(core_id, threads_per_package); + acpigen_pop_len(); } } - /* PPKG is usually used for thermal management - of the first and only package. */ + /* PPKG is usually used for thermal management of the first and only package. */ acpigen_write_processor_package("PPKG", 0, threads_per_package);
/* Add a method to notify processor nodes */
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Jay Talbott (Code Review)
-
Jonathan Zhang (Code Review)
-
Marc Jones (Code Review)
-
Stefan Reinauer (Code Review)