Change in coreboot[master]: soc/amd/picasso: Give the mainboard the ability to modify the MADT table

[Raul Rangel (Code Review)]

Hello Martin Roth,

I'd like you to do a code review. Please visit

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

to review the following change.

Change subject: soc/amd/picasso: Give the mainboard the ability to modify the MADT table
......................................................................

soc/amd/picasso: Give the mainboard the ability to modify the MADT table

By default legacy ISA IRQs use edge triggering. Depending on what
devices are used the IRQ types might need to be changed. We add an
override method to allow the mainboard to configure the IRA IRQs.

BUG=b:145102877
TEST=Booted trembyle and was able to use the keyboard.

Change-Id: Ie95e8cc7ca835fb60bee8f10d5f28def6c2801dc
Signed-off-by: Raul E Rangel 
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+...
Reviewed-by: Martin Roth 
---
M src/soc/amd/picasso/acpi.c
M src/soc/amd/picasso/include/soc/acpi.h
2 files changed, 14 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/41577/1

diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c
index ac23d0f..8e12581 100644
--- a/src/soc/amd/picasso/acpi.c
+++ b/src/soc/amd/picasso/acpi.c
@@ -10,6 +10,7 @@
 #include <acpi/acpigen.h>
 #include <device/pci_ops.h>
 #include <arch/ioapic.h>
+#include <arch/smp/mpspec.h>
 #include <cpu/x86/smm.h>
 #include <cbmem.h>
 #include <device/device.h>
@@ -36,6 +37,12 @@
    return current;
 }

+unsigned long __weak acpi_mb_madt_irqoverride(unsigned long current)
+{
+	/* Add any additional overrides */
+	return current;
+}
+
 unsigned long acpi_fill_madt(unsigned long current)
 {
    /* create all subtables for processors */
@@ -51,8 +58,11 @@
    /* 5 mean: 0101 --> Edge-triggered, Active high */
    current += acpi_create_madt_irqoverride((acpi_madt_irqoverride_t *)
    					current, 0, 0, 2, 0);
-	current += acpi_create_madt_irqoverride((acpi_madt_irqoverride_t *)
-						current, 0, 9, 9, 0xf);
+	current += acpi_create_madt_irqoverride(
+		(acpi_madt_irqoverride_t *)current, 0, 9, 9,
+		MP_IRQ_TRIGGER_LEVEL | MP_IRQ_POLARITY_LOW);
+
+	current = acpi_mb_madt_irqoverride(current);

/* create all subtables for processors */
    current += acpi_create_madt_lapic_nmi((acpi_madt_lapic_nmi_t *)current,
diff --git a/src/soc/amd/picasso/include/soc/acpi.h b/src/soc/amd/picasso/include/soc/acpi.h
index 6250f86..159d4c7 100644
--- a/src/soc/amd/picasso/include/soc/acpi.h
+++ b/src/soc/amd/picasso/include/soc/acpi.h
@@ -16,4 +16,6 @@

const char *soc_acpi_name(const struct device *dev);

+unsigned long acpi_mb_madt_irqoverride(unsigned long current);
+
 #endif /* __SOC_PICASSO_ACPI_H__ */

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41577 )

Change subject: soc/amd/picasso: Give the mainboard the ability to modify the MADT table
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41577/1/src/soc/amd/picasso/acpi.c 
File src/soc/amd/picasso/acpi.c:

https://review.coreboot.org/c/coreboot/+/41577/1/src/soc/amd/picasso/acpi.c@... 
PS1, Line 40: unsigned long __weak acpi_mb_madt_irqoverride(unsigned long current)
Why is the ACPI table creation directly exposed? Can't we drive this through configuration tables instead of making the mainboards implement the proper sequence?

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41577 )

Change subject: soc/amd/picasso: Give the mainboard the ability to modify the MADT table
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41577/1/src/soc/amd/picasso/acpi.c 
File src/soc/amd/picasso/acpi.c:

https://review.coreboot.org/c/coreboot/+/41577/1/src/soc/amd/picasso/acpi.c@... 
PS1, Line 40: unsigned long __weak acpi_mb_madt_irqoverride(unsigned long current)
> Why is the ACPI table creation directly exposed? Can't we drive this through configuration tables in […]

I guess I can add something to the soc_amd_picasso_config to set these.

[Raul Rangel (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Aaron Durbin, Felix Held,

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

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

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

Change subject: soc/amd/picasso: Give the mainboard the ability to modify the MADT table
......................................................................

soc/amd/picasso: Give the mainboard the ability to modify the MADT table

By default legacy ISA IRQs use edge triggering. Depending on what
devices are used the IRQ types might need to be changed. We add a
setting to the device tree to allow the mainboard to configure the IRS
IRQs.

BUG=b:145102877
TEST=Booted trembyle and was able to use the keyboard.

Change-Id: Ie95e8cc7ca835fb60bee8f10d5f28def6c2801dc
Signed-off-by: Raul E Rangel 
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+...
Reviewed-by: Martin Roth 
---
M src/soc/amd/picasso/acpi.c
M src/soc/amd/picasso/chip.h
2 files changed, 33 insertions(+), 2 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/41577/2

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41577 )

Change subject: soc/amd/picasso: Give the mainboard the ability to modify the MADT table
......................................................................

Uploaded patch set 2.

[Raul Rangel (Code Review)]

Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41577 )

Change subject: soc/amd/picasso: Give the mainboard the ability to modify the MADT table
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41577/1/src/soc/amd/picasso/acpi.c 
File src/soc/amd/picasso/acpi.c:

https://review.coreboot.org/c/coreboot/+/41577/1/src/soc/amd/picasso/acpi.c@... 
PS1, Line 40: unsigned long __weak acpi_mb_madt_irqoverride(unsigned long current)
> I guess I can add something to the soc_amd_picasso_config to set these.

Done

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41577 )

Change subject: soc/amd/picasso: Give the mainboard the ability to modify the MADT table
......................................................................

Patch Set 2: Code-Review+2

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41577 )

Change subject: soc/amd/picasso: Give the mainboard the ability to modify the MADT table
......................................................................

soc/amd/picasso: Give the mainboard the ability to modify the MADT table

By default legacy ISA IRQs use edge triggering. Depending on what
devices are used the IRQ types might need to be changed. We add a
setting to the device tree to allow the mainboard to configure the IRS
IRQs.

BUG=b:145102877
TEST=Booted trembyle and was able to use the keyboard.

Change-Id: Ie95e8cc7ca835fb60bee8f10d5f28def6c2801dc
Signed-off-by: Raul E Rangel 
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/coreboot/+...
Reviewed-by: Martin Roth 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41577
Tested-by: build bot (Jenkins) 
Reviewed-by: Aaron Durbin 
---
M src/soc/amd/picasso/acpi.c
M src/soc/amd/picasso/chip.h
2 files changed, 33 insertions(+), 2 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved

diff --git a/src/soc/amd/picasso/acpi.c b/src/soc/amd/picasso/acpi.c
index ac23d0f..2eea6f9 100644
--- a/src/soc/amd/picasso/acpi.c
+++ b/src/soc/amd/picasso/acpi.c
@@ -10,6 +10,7 @@
 #include <acpi/acpigen.h>
 #include <device/pci_ops.h>
 #include <arch/ioapic.h>
+#include <arch/smp/mpspec.h>
 #include <cpu/x86/smm.h>
 #include <cbmem.h>
 #include <device/device.h>
@@ -23,6 +24,7 @@
 #include <soc/nvs.h>
 #include <soc/gpio.h>
 #include <version.h>
+#include "chip.h"

unsigned long acpi_fill_mcfg(unsigned long current)
 {
@@ -38,6 +40,11 @@

unsigned long acpi_fill_madt(unsigned long current)
 {
+	const struct soc_amd_picasso_config *cfg = config_of_soc();
+	unsigned int i;
+	uint8_t irq;
+	uint8_t flags;
+
    /* create all subtables for processors */
    current = acpi_create_madt_lapics(current);

@@ -51,8 +58,20 @@
    /* 5 mean: 0101 --> Edge-triggered, Active high */
    current += acpi_create_madt_irqoverride((acpi_madt_irqoverride_t *)
    					current, 0, 0, 2, 0);
-	current += acpi_create_madt_irqoverride((acpi_madt_irqoverride_t *)
-						current, 0, 9, 9, 0xf);
+	current += acpi_create_madt_irqoverride(
+		(acpi_madt_irqoverride_t *)current, 0, 9, 9,
+		MP_IRQ_TRIGGER_LEVEL | MP_IRQ_POLARITY_LOW);
+
+	for (i = 0; i < ARRAY_SIZE(cfg->irq_override); ++i) {
+		irq = cfg->irq_override[i].irq;
+		flags = cfg->irq_override[i].flags;
+
+		if (!flags)
+			continue;
+
+		current += acpi_create_madt_irqoverride((acpi_madt_irqoverride_t *)current, 0,
+							irq, irq, flags);
+	}

/* create all subtables for processors */
    current += acpi_create_madt_lapic_nmi((acpi_madt_lapic_nmi_t *)current,
diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h
index 80cb1ce..e52751a 100644
--- a/src/soc/amd/picasso/chip.h
+++ b/src/soc/amd/picasso/chip.h
@@ -12,6 +12,7 @@
 #include <soc/iomap.h>
 #include <soc/southbridge.h>
 #include <acpi/acpi_device.h>
+#include <arch/smp/mpspec.h>

struct soc_amd_picasso_config {
    struct soc_amd_common_config common_config;
@@ -34,6 +35,17 @@
    	I2S_PINS_UNCONF = 7,	/* All pads will be input mode */
    } acp_pin_cfg;

+	/**
+	 * IRQ 0 - 15 have a default trigger of edge and default polarity of high.
+	 * If you have a device that requires a different configuration you can override the
+	 * settings here.
+	 */
+	struct {
+		uint8_t irq;
+		/* See MP_IRQ_* from mpspec.h */
+		uint8_t flags;
+	} irq_override[16];
+
    /* Options for these are in src/arch/x86/include/acpi/acpi.h */
    uint8_t  fadt_pm_profile;
    uint16_t fadt_boot_arch;

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41577 )

Change subject: soc/amd/picasso: Give the mainboard the ability to modify the MADT table
......................................................................

Patch Set 3:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4
Emulation targets:
"QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/4015
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4014
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4013
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4012

Please note: This test is under development and might not be accurate at all!