Change in coreboot[master]: sb/intel/common/acpi/irqlinks.asl: Add missing IRQs

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43158 )

Change subject: sb/intel/common/acpi/irqlinks.asl: Add missing IRQs
......................................................................

sb/intel/common/acpi/irqlinks.asl: Add missing IRQs

IRQ 10 and IRQ 11 are valid for all southbridges using this code.

Change-Id: Ib4504861ed316a95b9735e0ed79f108f18071b3b
Signed-off-by: Angel Pons 
---
M src/southbridge/intel/common/acpi/irqlinks.asl
1 file changed, 8 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/43158/1

diff --git a/src/southbridge/intel/common/acpi/irqlinks.asl b/src/southbridge/intel/common/acpi/irqlinks.asl
index 527aa58..3f3386d 100644
--- a/src/southbridge/intel/common/acpi/irqlinks.asl
+++ b/src/southbridge/intel/common/acpi/irqlinks.asl
@@ -15,7 +15,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 10, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link
@@ -74,7 +74,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 11, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link
@@ -133,7 +133,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 10, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link
@@ -192,7 +192,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 11, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link
@@ -251,7 +251,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 10, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link
@@ -310,7 +310,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 11, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link
@@ -369,7 +369,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 10, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link
@@ -428,7 +428,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 11, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43158 )

Change subject: sb/intel/common/acpi/irqlinks.asl: Add missing IRQs
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43158/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43158/1//COMMIT_MSG@9 
PS1, Line 9: IRQ 10 and IRQ 11 are valid for all southbridges using this code.
Where is this documented?

Did you find a comment why these were left out originally?

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43158 )

Change subject: sb/intel/common/acpi/irqlinks.asl: Add missing IRQs
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43158/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43158/1//COMMIT_MSG@9 
PS1, Line 9: IRQ 10 and IRQ 11 are valid for all southbridges using this code.
> Where is this documented? […]

In the datasheet, and I didn't find a reason

[Angel Pons (Code Review)]

Hello build bot (Jenkins), Patrick Rudolph,

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

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

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

Change subject: sb/intel/common/acpi/irqlinks.asl: Add missing IRQs
......................................................................

sb/intel/common/acpi/irqlinks.asl: Add missing IRQs

IRQ 10 and IRQ 11 are valid for all southbridges using this code, as per
their respective datasheets. So, add them for the sake of completeness.

Change-Id: Ib4504861ed316a95b9735e0ed79f108f18071b3b
Signed-off-by: Angel Pons 
---
M src/southbridge/intel/common/acpi/irqlinks.asl
1 file changed, 8 insertions(+), 8 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/43158/2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43158 )

Change subject: sb/intel/common/acpi/irqlinks.asl: Add missing IRQs
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43158/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43158/1//COMMIT_MSG@9 
PS1, Line 9: IRQ 10 and IRQ 11 are valid for all southbridges using this code.
> In the datasheet, and I didn't find a reason

Updated the commit message

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43158 )

Change subject: sb/intel/common/acpi/irqlinks.asl: Add missing IRQs
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43158/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43158/2//COMMIT_MSG@10 
PS2, Line 10: their respective datasheets. So, add them for the sake of completeness.
I can see some "irq 0x70 = 10" and "irq 0x70 = 0x0b" in devicetrees, I did not track which sb/ or soc/ exactly.

From what I remember, IRQs from SuperIO (well, SERIRQ in general) cannot be shared with PCI interrupts. Former are edge-triggered, latter level-triggered. Or vice versa.

The entire interrupt routing through PIC (as opposed to IOAPIC) may be very much an untested codepath. If a board has classic UART tied to IRQ3/4, specs probably say PIRQ-table and MP-table and ACPI must not advertise IRQ3/4 as available target for PIRQA-H. You probably need kernel cmdline with "nosmp noapic" to trigger possible errors.

Was there some mentions of IRQ1/12 in the sb datasheets? I find it odd that they would be treated differently.

I think IRQ14/15 were edge-triggered with (P)ATA operating in legacy mode.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43158 )

Change subject: sb/intel/common/acpi/irqlinks.asl: Add missing IRQs
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43158/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43158/2//COMMIT_MSG@10 
PS2, Line 10: their respective datasheets. So, add them for the sake of completeness.
> I can see some "irq 0x70 = 10" and "irq 0x70 = 0x0b" in devicetrees, I did not track which sb/ or so […]

I am basically aligning the ACPI code to allow bd82x6x to use the same irqlinks.asl, because someone added those IRQs in CB:22600 a while ago.

Reference code for Haswell/Lynxpoint uses {3,4,5,6,10,11,12,14,15} on all eight _PRS

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43158 )

Change subject: sb/intel/common/acpi/irqlinks.asl: Add missing IRQs
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43158/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43158/2//COMMIT_MSG@10 
PS2, Line 10: their respective datasheets. So, add them for the sake of completeness.
> I am basically aligning the ACPI code to allow bd82x6x to use the same irqlinks.asl, because someone added those IRQs in CB:22600 a while ago.

Yes, I can see why you want do this, and I do not object it. My feedback here is the existing ASL probably should be replaced with generated AML to take into account the IRQs that are reserved for SuperIO/HWM/EC or SERIRQ in general. And that maybe someone intentionally had excluded IRQ10 and IRQ11 for selected boards. We probably lost some details in deduplication over the years.

Again, I did not track down "irq 0x70 = 0xb" lines, but I feel those would conflict with PIRQ links routed to PCH_IRQ11 on the same supermicro/x11-lga1151-series boards.
> Reference code for Haswell/Lynxpoint uses {3,4,5,6,10,11,12,14,15} on all eight _PRS

For a chromebook without superio, that list might be just fine.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43158 )

Change subject: sb/intel/common/acpi/irqlinks.asl: Add missing IRQs
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43158/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43158/2//COMMIT_MSG@10 
PS2, Line 10: their respective datasheets. So, add them for the sake of completeness.
> > I am basically aligning the ACPI code to allow bd82x6x to use the same irqlinks. […]

x11-lga1151-series is Skylake, not affected by this patch. But yes, I've checked closer and I can see that other vendors generate the IRQ list dynamically. But this is for a later patch.

Can we get this in as-is, and handle it later once code is common?

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43158 )

Change subject: sb/intel/common/acpi/irqlinks.asl: Add missing IRQs
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43158/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/43158/2//COMMIT_MSG@10 
PS2, Line 10: their respective datasheets. So, add them for the sake of completeness.
> x11-lga1151-series is Skylake, not affected by this patch. […]

At least it's documented here now that one should not expect things to work properly without IOAPIC.

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43158 )

Change subject: sb/intel/common/acpi/irqlinks.asl: Add missing IRQs
......................................................................

Patch Set 3: Code-Review+2

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43158 )

Change subject: sb/intel/common/acpi/irqlinks.asl: Add missing IRQs
......................................................................

Patch Set 3: Code-Review+1

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43158 )

Change subject: sb/intel/common/acpi/irqlinks.asl: Add missing IRQs
......................................................................

sb/intel/common/acpi/irqlinks.asl: Add missing IRQs

IRQ 10 and IRQ 11 are valid for all southbridges using this code, as per
their respective datasheets. So, add them for the sake of completeness.

Change-Id: Ib4504861ed316a95b9735e0ed79f108f18071b3b
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/43158
Reviewed-by: Nico Huber 
Reviewed-by: Michael Niewöhner
Tested-by: build bot (Jenkins) 
---
M src/southbridge/intel/common/acpi/irqlinks.asl
1 file changed, 8 insertions(+), 8 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  Michael Niewöhner: Looks good to me, but someone else must approve

diff --git a/src/southbridge/intel/common/acpi/irqlinks.asl b/src/southbridge/intel/common/acpi/irqlinks.asl
index 527aa58..3f3386d 100644
--- a/src/southbridge/intel/common/acpi/irqlinks.asl
+++ b/src/southbridge/intel/common/acpi/irqlinks.asl
@@ -15,7 +15,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 10, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link
@@ -74,7 +74,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 11, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link
@@ -133,7 +133,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 10, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link
@@ -192,7 +192,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 11, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link
@@ -251,7 +251,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 10, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link
@@ -310,7 +310,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 11, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link
@@ -369,7 +369,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 10, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link
@@ -428,7 +428,7 @@
    Name (_PRS, ResourceTemplate()
    {
    	IRQ(Level, ActiveLow, Shared)
-			{ 3, 4, 5, 6, 7, 11, 12, 14, 15 }
+			{ 3, 4, 5, 6, 7, 10, 11, 12, 14, 15 }
    })

// Current Resource Settings for this link