Change in coreboot[master]: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47301 )

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry

The PCH IOAPIC ID is 0x8 so it needs to be generated before the IIO
IOAPICs. Since we will get rid of the ioapic_id array this makes it
more readable.

Change-Id: I64a3b259e438ef666fb68a433cceda10aebdb1bf
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/acpi.c
1 file changed, 17 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/47301/1

diff --git a/src/soc/intel/xeon_sp/acpi.c b/src/soc/intel/xeon_sp/acpi.c
index fcd5f00..c70945a 100644
--- a/src/soc/intel/xeon_sp/acpi.c
+++ b/src/soc/intel/xeon_sp/acpi.c
@@ -101,6 +101,16 @@
    current = xeonsp_acpi_create_madt_lapics(current);

cur_index = 0;
+	{
+		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];
+		current += add_madt_ioapic(current, 0, 0, ioapic_id,
+					   hob->PlatformData.IIO_resource[0].StackRes[0].IoApicBase,
+					   gsi_base);
+		++cur_index;
+	}

for (int socket = 0; socket < hob->PlatformData.numofIIO; ++socket) {
    	for (int stack = 0; stack < MAX_IIO_STACK; ++stack) {
@@ -111,22 +121,17 @@
    		assert(cur_index < ARRAY_SIZE(gsi_bases));
    		int ioapic_id = ioapic_ids[cur_index];
    		int gsi_base = gsi_bases[cur_index];
-			current += add_madt_ioapic(current, socket, stack, ioapic_id, ri->IoApicBase, gsi_base);
-			++cur_index;
-
+			uint32_t ioapic_base = ri->IoApicBase;
    		/*
    		 * Stack 0 has non-PCH IOAPIC and PCH IOAPIC.
    		 * Add entry for PCH IOAPIC.
    		 */
-			if (stack == 0 && socket == 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];
-				current += add_madt_ioapic(current, socket, stack, ioapic_id,
-							   ri->IoApicBase + 0x1000, gsi_base);
-				++cur_index;
-			}
+			if (stack == 0 && socket == 0)
+				ioapic_base += 0x1000;
+
+			current += add_madt_ioapic(current, socket, stack, ioapic_id, ioapic_base,
+						   gsi_base);
+			++cur_index;
    	}
    }

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47301 )

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/47301/1/src/soc/intel/xeon_sp/acpi.... 
File src/soc/intel/xeon_sp/acpi.c:

https://review.coreboot.org/c/coreboot/+/47301/1/src/soc/intel/xeon_sp/acpi.... 
PS1, Line 110: 					   hob->PlatformData.IIO_resource[0].StackRes[0].IoApicBase,
line over 96 characters

https://review.coreboot.org/c/coreboot/+/47301/1/src/soc/intel/xeon_sp/acpi.... 
PS1, Line 132: 			current += add_madt_ioapic(current, socket, stack, ioapic_id, ioapic_base,
line over 96 characters

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Marc Jones, Jonathan Zhang, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47301

to look at the new patch set (#2).

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry

The PCH IOAPIC ID is 0x8 so it needs to be generated before the IIO
IOAPICs. Since we will get rid of the ioapic_id array this makes it
more readable.

Change-Id: I64a3b259e438ef666fb68a433cceda10aebdb1bf
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/acpi.c
1 file changed, 16 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/47301/2

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47301 )

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

Patch Set 2: Code-Review+2

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47301 )

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/47301/2/src/soc/intel/xeon_sp/acpi.... 
File src/soc/intel/xeon_sp/acpi.c:

https://review.coreboot.org/c/coreboot/+/47301/2/src/soc/intel/xeon_sp/acpi.... 
PS2, Line 111: 					   hob->PlatformData.IIO_resource[0].StackRes[0].IoApicBase,
line over 96 characters

https://review.coreboot.org/c/coreboot/+/47301/2/src/soc/intel/xeon_sp/acpi.... 
PS2, Line 137: 			current += add_madt_ioapic(current, socket, stack, ioapic_id, ioapic_base,
line over 96 characters

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Marc Jones, Jonathan Zhang, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47301

to look at the new patch set (#3).

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry

The PCH IOAPIC ID is 0x8 so it needs to be generated before the IIO
IOAPICs. Since we will get rid of the ioapic_id array this makes it
more readable.

Change-Id: I64a3b259e438ef666fb68a433cceda10aebdb1bf
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/acpi.c
1 file changed, 16 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/47301/3

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47301 )

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47301/3/src/soc/intel/xeon_sp/acpi.... 
File src/soc/intel/xeon_sp/acpi.c:

https://review.coreboot.org/c/coreboot/+/47301/3/src/soc/intel/xeon_sp/acpi.... 
PS3, Line 111: 					   hob->PlatformData.IIO_resource[0].StackRes[0].IoApicBase,
line over 96 characters

[Arthur Heymans (Code Review)]

Hello build bot (Jenkins), Marc Jones, Jonathan Zhang, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47301

to look at the new patch set (#4).

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry

The PCH IOAPIC ID is 0x8 so it needs to be generated before the IIO
IOAPICs. Since we will get rid of the ioapic_id array this makes it
more readable.

Change-Id: I64a3b259e438ef666fb68a433cceda10aebdb1bf
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/acpi.c
1 file changed, 17 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/47301/4

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47301 )

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47301/4/src/soc/intel/xeon_sp/acpi.... 
File src/soc/intel/xeon_sp/acpi.c:

https://review.coreboot.org/c/coreboot/+/47301/4/src/soc/intel/xeon_sp/acpi.... 
PS4, Line 111: 					   hob->PlatformData.IIO_resource[0].StackRes[0].IoApicBase,
line over 96 characters

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47301 )

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47301/5/src/soc/intel/xeon_sp/acpi.... 
File src/soc/intel/xeon_sp/acpi.c:

https://review.coreboot.org/c/coreboot/+/47301/5/src/soc/intel/xeon_sp/acpi.... 
PS5, Line 111: 					   hob->PlatformData.IIO_resource[0].StackRes[0].IoApicBase,
line over 96 characters

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47301 )

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/47301/6/src/soc/intel/xeon_sp/acpi.... 
File src/soc/intel/xeon_sp/acpi.c:

https://review.coreboot.org/c/coreboot/+/47301/6/src/soc/intel/xeon_sp/acpi.... 
PS6, Line 111: 					   hob->PlatformData.IIO_resource[0].StackRes[0].IoApicBase,
line over 96 characters

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47301 )

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

Patch Set 6:

(2 comments)

A couple of nits. It is almost ready to +2.

https://review.coreboot.org/c/coreboot/+/47301/6/src/soc/intel/xeon_sp/acpi.... 
File src/soc/intel/xeon_sp/acpi.c:

https://review.coreboot.org/c/coreboot/+/47301/6/src/soc/intel/xeon_sp/acpi.... 
PS6, Line 108: 		int ioapic_id = ioapic_ids[cur_index];
             : 		int gsi_base = gsi_bases[cur_index];
Is there a reason to not declare these above so we don't don't do it twice and need a new scope here? A comment that this is setting the PCH IOAPIC and then the IIO APICs would be good. I know it is easier to see that in the next patch.

https://review.coreboot.org/c/coreboot/+/47301/6/src/soc/intel/xeon_sp/acpi.... 
PS6, Line 130: 			 * Add entry for PCH IOAPIC.
Does this comment need an update? I think that this is the IIO APIC entry.

[Arthur Heymans (Code Review)]

Hello Marc Jones, build bot (Jenkins), Jonathan Zhang, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/47301

to look at the new patch set (#7).

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry

The PCH IOAPIC ID is 0x8 so it needs to be generated before the IIO
IOAPICs. Since we will get rid of the ioapic_id array this makes it
more readable.

Change-Id: I64a3b259e438ef666fb68a433cceda10aebdb1bf
Signed-off-by: Arthur Heymans 
---
M src/soc/intel/xeon_sp/acpi.c
1 file changed, 18 insertions(+), 15 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/47301/7

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47301 )

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

Patch Set 7:

(2 comments)

https://review.coreboot.org/c/coreboot/+/47301/6/src/soc/intel/xeon_sp/acpi.... 
File src/soc/intel/xeon_sp/acpi.c:

https://review.coreboot.org/c/coreboot/+/47301/6/src/soc/intel/xeon_sp/acpi.... 
PS6, Line 108: 		int ioapic_id = ioapic_ids[cur_index];
             : 		int gsi_base = gsi_bases[cur_index];
> Is there a reason to not declare these above so we don't don't do it twice and need a new scope here […]

Done

https://review.coreboot.org/c/coreboot/+/47301/6/src/soc/intel/xeon_sp/acpi.... 
PS6, Line 130: 			 * Add entry for PCH IOAPIC.
> Does this comment need an update? I think that this is the IIO APIC entry.

Done

[Marc Jones (Code Review)]

Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47301 )

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

Patch Set 7: Code-Review+2

[Arthur Heymans (Code Review)]

Arthur Heymans has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47301 )

Change subject: soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry
......................................................................

soc/intel/xeon_sp: Improve generating PCH IOAPIC MADT entry

The PCH IOAPIC ID is 0x8 so it needs to be generated before the IIO
IOAPICs. Since we will get rid of the ioapic_id array this makes it
more readable.

Change-Id: I64a3b259e438ef666fb68a433cceda10aebdb1bf
Signed-off-by: Arthur Heymans 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/47301
Tested-by: build bot (Jenkins) 
Reviewed-by: Marc Jones 
---
M src/soc/intel/xeon_sp/acpi.c
1 file changed, 18 insertions(+), 15 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Marc Jones: Looks good to me, approved

diff --git a/src/soc/intel/xeon_sp/acpi.c b/src/soc/intel/xeon_sp/acpi.c
index f747f87..3b4b713 100644
--- a/src/soc/intel/xeon_sp/acpi.c
+++ b/src/soc/intel/xeon_sp/acpi.c
@@ -86,6 +86,8 @@
 unsigned long acpi_fill_madt(unsigned long current)
 {
    int cur_index;
+	int ioapic_id;
+	int gsi_base;
    const IIO_UDS *hob = get_iio_uds();

/* With XEON-SP FSP, PCH IOAPIC is allocated with first 120 GSIs. */
@@ -102,6 +104,12 @@
    current = xeonsp_acpi_create_madt_lapics(current);

cur_index = 0;
+	ioapic_id = ioapic_ids[cur_index];
+	gsi_base = gsi_bases[cur_index];
+	current += add_madt_ioapic(current, 0, 0, ioapic_id,
+				   hob->PlatformData.IIO_resource[0].StackRes[0].IoApicBase,
+				   gsi_base);
+		++cur_index;

for (int socket = 0; socket < hob->PlatformData.numofIIO; ++socket) {
    	for (int stack = 0; stack < MAX_IIO_STACK; ++stack) {
@@ -111,25 +119,20 @@
    			continue;
    		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];
-			current += add_madt_ioapic(current, socket, stack, ioapic_id,
-						   ri->IoApicBase, gsi_base);
-			++cur_index;
+			ioapic_id = ioapic_ids[cur_index];
+			gsi_base = gsi_bases[cur_index];
+			uint32_t ioapic_base = ri->IoApicBase;

/*
    		 * Stack 0 has non-PCH IOAPIC and PCH IOAPIC.
-			 * Add entry for PCH IOAPIC.
+			 * The IIO IOAPIC is placed at 0x1000 from the reported base.
    		 */
-			if (stack == 0 && socket == 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];
-				current += add_madt_ioapic(current, socket, stack, ioapic_id,
-							   ri->IoApicBase + 0x1000, gsi_base);
-				++cur_index;
-			}
+			if (stack == 0 && socket == 0)
+				ioapic_base += 0x1000;
+
+			current += add_madt_ioapic(current, socket, stack, ioapic_id,
+						   ioapic_base, gsi_base);
+			++cur_index;
    	}
    }