Change in coreboot[master]: acpi,mb/*: Use motherboard_fill_fadt()

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

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41917 )

Change subject: acpi,mb/*: Use motherboard_fill_fadt()
......................................................................

acpi,mb/*: Use motherboard_fill_fadt()

Change-Id: I0453c0edc9aeb86cf4f5d912613b8fa8beb63ab4
Signed-off-by: Kyösti Mälkki 
---
M src/mainboard/facebook/fbg1701/acpi_tables.c
M src/mainboard/facebook/fbg1701/fadt.c
M src/mainboard/portwell/m107/acpi_tables.c
M src/mainboard/portwell/m107/fadt.c
M src/mainboard/protectli/vault_bsw/acpi_tables.c
M src/mainboard/protectli/vault_bsw/fadt.c
6 files changed, 18 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/41917/1

diff --git a/src/mainboard/facebook/fbg1701/acpi_tables.c b/src/mainboard/facebook/fbg1701/acpi_tables.c
index 539a74e..fc0aa60 100644
--- a/src/mainboard/facebook/fbg1701/acpi_tables.c
+++ b/src/mainboard/facebook/fbg1701/acpi_tables.c
@@ -37,3 +37,8 @@

return current;
 }
+
+void motherboard_fill_fadt(acpi_fadt_t *fadt)
+{
+	fadt->iapc_boot_arch &= ~ACPI_FADT_8042;
+}
diff --git a/src/mainboard/facebook/fbg1701/fadt.c b/src/mainboard/facebook/fbg1701/fadt.c
index 3785f15..9f8f058 100644
--- a/src/mainboard/facebook/fbg1701/fadt.c
+++ b/src/mainboard/facebook/fbg1701/fadt.c
@@ -26,8 +26,7 @@
    fadt->x_dsdt_h = 0;

acpi_fill_in_fadt(fadt);
-
-	fadt->iapc_boot_arch &= ~ACPI_FADT_8042;
+	motherboard_fill_fadt(fadt);

header->checksum =
        acpi_checksum((void *) fadt, header->length);
diff --git a/src/mainboard/portwell/m107/acpi_tables.c b/src/mainboard/portwell/m107/acpi_tables.c
index 539a74e..fc0aa60 100644
--- a/src/mainboard/portwell/m107/acpi_tables.c
+++ b/src/mainboard/portwell/m107/acpi_tables.c
@@ -37,3 +37,8 @@

return current;
 }
+
+void motherboard_fill_fadt(acpi_fadt_t *fadt)
+{
+	fadt->iapc_boot_arch &= ~ACPI_FADT_8042;
+}
diff --git a/src/mainboard/portwell/m107/fadt.c b/src/mainboard/portwell/m107/fadt.c
index 3785f15..9f8f058 100644
--- a/src/mainboard/portwell/m107/fadt.c
+++ b/src/mainboard/portwell/m107/fadt.c
@@ -26,8 +26,7 @@
    fadt->x_dsdt_h = 0;

acpi_fill_in_fadt(fadt);
-
-	fadt->iapc_boot_arch &= ~ACPI_FADT_8042;
+	motherboard_fill_fadt(fadt);

header->checksum =
        acpi_checksum((void *) fadt, header->length);
diff --git a/src/mainboard/protectli/vault_bsw/acpi_tables.c b/src/mainboard/protectli/vault_bsw/acpi_tables.c
index f502741..b80f83f 100644
--- a/src/mainboard/protectli/vault_bsw/acpi_tables.c
+++ b/src/mainboard/protectli/vault_bsw/acpi_tables.c
@@ -27,3 +27,8 @@

return current;
 }
+
+void motherboard_fill_fadt(acpi_fadt_t *fadt)
+{
+	fadt->iapc_boot_arch = ACPI_FADT_LEGACY_DEVICES;
+}
diff --git a/src/mainboard/protectli/vault_bsw/fadt.c b/src/mainboard/protectli/vault_bsw/fadt.c
index 755ac6e..8a23eba 100644
--- a/src/mainboard/protectli/vault_bsw/fadt.c
+++ b/src/mainboard/protectli/vault_bsw/fadt.c
@@ -27,8 +27,7 @@
    fadt->x_dsdt_h = 0;

acpi_fill_in_fadt(fadt);
-
-	fadt->iapc_boot_arch = ACPI_FADT_LEGACY_DEVICES;
+	motherboard_fill_fadt(fadt);

header->checksum = acpi_checksum((void *)fadt, header->length);
 }

[Wim Vervoorn (Code Review)]

Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41917 )

Change subject: acpi,mb/*: Use motherboard_fill_fadt()
......................................................................

Patch Set 2:

(3 comments)

https://review.coreboot.org/c/coreboot/+/41917/2/src/mainboard/facebook/fbg1... 
File src/mainboard/facebook/fbg1701/fadt.c:

https://review.coreboot.org/c/coreboot/+/41917/2/src/mainboard/facebook/fbg1... 
PS2, Line 30: 
The fadt->preferred_pm_profile will not be set to PM_MOBILE for this board as this board is not defined as a laptop or convertible (which is correct) but we still want this profile to the the default. So this should be added to the motherboard_fill_fadt(fadt) function.

https://review.coreboot.org/c/coreboot/+/41917/2/src/mainboard/portwell/m107... 
File src/mainboard/portwell/m107/acpi_tables.c:

https://review.coreboot.org/c/coreboot/+/41917/2/src/mainboard/portwell/m107... 
PS2, Line 43: 	fadt->iapc_boot_arch &= ~ACPI_FADT_8042;
The fadt->preferred_pm_profile will not be set to PM_MOBILE for this board as this board is not defined as a laptop or convertible (which is correct) but we still want this profile to the the default. So this should be added to the motherboard_fill_fadt(fadt) function.

https://review.coreboot.org/c/coreboot/+/41917/2/src/mainboard/protectli/vau... 
File src/mainboard/protectli/vault_bsw/acpi_tables.c:

https://review.coreboot.org/c/coreboot/+/41917/2/src/mainboard/protectli/vau... 
PS2, Line 34: }
The fadt->preferred_pm_profile will not be set to PM_MOBILE for this board as this board is not defined as a laptop or convertible. I guess this is the same issue as the portwell and facebook boards. But I don't know the details of this board so I am not sure if this will be a problem. It is a difference though.

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

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

Change subject: acpi,mb/*: Use motherboard_fill_fadt()
......................................................................

Patch Set 2:

Thanks, fixes land with next rebase.

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

Hello build bot (Jenkins), Michał Żygowski, Frans Hendriks, Wim Vervoorn, Piotr Król,

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

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

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

Change subject: acpi,mb/*: Use motherboard_fill_fadt()
......................................................................

acpi,mb/*: Use motherboard_fill_fadt()

By vendor choice override fadt->preferred_pm_profile here.

Change-Id: I0453c0edc9aeb86cf4f5d912613b8fa8beb63ab4
Signed-off-by: Kyösti Mälkki 
---
M src/mainboard/facebook/fbg1701/acpi_tables.c
M src/mainboard/facebook/fbg1701/fadt.c
M src/mainboard/portwell/m107/acpi_tables.c
M src/mainboard/portwell/m107/fadt.c
M src/mainboard/protectli/vault_bsw/acpi_tables.c
M src/mainboard/protectli/vault_bsw/fadt.c
6 files changed, 21 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/17/41917/3

[Wim Vervoorn (Code Review)]

Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41917 )

Change subject: acpi,mb/*: Use motherboard_fill_fadt()
......................................................................

Patch Set 3: Code-Review+2

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41917 )

Change subject: acpi,mb/*: Use motherboard_fill_fadt()
......................................................................

Patch Set 3: Code-Review+2

[Angel Pons (Code Review)]

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

Change subject: acpi,mb/*: Use motherboard_fill_fadt()
......................................................................

Patch Set 3: Code-Review+2

[Angel Pons (Code Review)]

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

Change subject: acpi,mb/*: Use motherboard_fill_fadt()
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41917/2/src/mainboard/facebook/fbg1... 
File src/mainboard/facebook/fbg1701/fadt.c:

https://review.coreboot.org/c/coreboot/+/41917/2/src/mainboard/facebook/fbg1... 
PS2, Line 30:
> The fadt->preferred_pm_profile will not be set to PM_MOBILE for this board as this board is not defi […]

Are these done?

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

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

Change subject: acpi,mb/*: Use motherboard_fill_fadt()
......................................................................

Patch Set 3:

(3 comments)

https://review.coreboot.org/c/coreboot/+/41917/2/src/mainboard/facebook/fbg1... 
File src/mainboard/facebook/fbg1701/fadt.c:

https://review.coreboot.org/c/coreboot/+/41917/2/src/mainboard/facebook/fbg1... 
PS2, Line 30:
> Are these done?

Done

https://review.coreboot.org/c/coreboot/+/41917/2/src/mainboard/portwell/m107... 
File src/mainboard/portwell/m107/acpi_tables.c:

https://review.coreboot.org/c/coreboot/+/41917/2/src/mainboard/portwell/m107... 
PS2, Line 43: 	fadt->iapc_boot_arch &= ~ACPI_FADT_8042;
> The fadt->preferred_pm_profile will not be set to PM_MOBILE for this board as this board is not defi […]

Done

https://review.coreboot.org/c/coreboot/+/41917/2/src/mainboard/protectli/vau... 
File src/mainboard/protectli/vault_bsw/acpi_tables.c:

https://review.coreboot.org/c/coreboot/+/41917/2/src/mainboard/protectli/vau... 
PS2, Line 34: }
> The fadt->preferred_pm_profile will not be set to PM_MOBILE for this board as this board is not defi […]

Done

[Patrick Georgi (Code Review)]

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

Change subject: acpi,mb/*: Use motherboard_fill_fadt()
......................................................................

acpi,mb/*: Use motherboard_fill_fadt()

By vendor choice override fadt->preferred_pm_profile here.

Change-Id: I0453c0edc9aeb86cf4f5d912613b8fa8beb63ab4
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41917
Reviewed-by: Wim Vervoorn 
Reviewed-by: Frans Hendriks 
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/facebook/fbg1701/acpi_tables.c
M src/mainboard/facebook/fbg1701/fadt.c
M src/mainboard/portwell/m107/acpi_tables.c
M src/mainboard/portwell/m107/fadt.c
M src/mainboard/protectli/vault_bsw/acpi_tables.c
M src/mainboard/protectli/vault_bsw/fadt.c
6 files changed, 21 insertions(+), 9 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Wim Vervoorn: Looks good to me, approved
  Frans Hendriks: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/mainboard/facebook/fbg1701/acpi_tables.c b/src/mainboard/facebook/fbg1701/acpi_tables.c
index 539a74e..8492704 100644
--- a/src/mainboard/facebook/fbg1701/acpi_tables.c
+++ b/src/mainboard/facebook/fbg1701/acpi_tables.c
@@ -37,3 +37,9 @@

return current;
 }
+
+void motherboard_fill_fadt(acpi_fadt_t *fadt)
+{
+	fadt->preferred_pm_profile = PM_MOBILE;
+	fadt->iapc_boot_arch &= ~ACPI_FADT_8042;
+}
diff --git a/src/mainboard/facebook/fbg1701/fadt.c b/src/mainboard/facebook/fbg1701/fadt.c
index 3785f15..cd15725 100644
--- a/src/mainboard/facebook/fbg1701/fadt.c
+++ b/src/mainboard/facebook/fbg1701/fadt.c
@@ -18,7 +18,6 @@

fadt->firmware_ctrl = (unsigned long) facs;
    fadt->dsdt = (unsigned long) dsdt;
-	fadt->preferred_pm_profile = PM_MOBILE;

fadt->x_firmware_ctl_l = (unsigned long)facs;
    fadt->x_firmware_ctl_h = 0;
@@ -26,8 +25,7 @@
    fadt->x_dsdt_h = 0;

acpi_fill_in_fadt(fadt);
-
-	fadt->iapc_boot_arch &= ~ACPI_FADT_8042;
+	motherboard_fill_fadt(fadt);

header->checksum =
        acpi_checksum((void *) fadt, header->length);
diff --git a/src/mainboard/portwell/m107/acpi_tables.c b/src/mainboard/portwell/m107/acpi_tables.c
index 539a74e..8492704 100644
--- a/src/mainboard/portwell/m107/acpi_tables.c
+++ b/src/mainboard/portwell/m107/acpi_tables.c
@@ -37,3 +37,9 @@

return current;
 }
+
+void motherboard_fill_fadt(acpi_fadt_t *fadt)
+{
+	fadt->preferred_pm_profile = PM_MOBILE;
+	fadt->iapc_boot_arch &= ~ACPI_FADT_8042;
+}
diff --git a/src/mainboard/portwell/m107/fadt.c b/src/mainboard/portwell/m107/fadt.c
index 3785f15..cd15725 100644
--- a/src/mainboard/portwell/m107/fadt.c
+++ b/src/mainboard/portwell/m107/fadt.c
@@ -18,7 +18,6 @@

fadt->firmware_ctrl = (unsigned long) facs;
    fadt->dsdt = (unsigned long) dsdt;
-	fadt->preferred_pm_profile = PM_MOBILE;

fadt->x_firmware_ctl_l = (unsigned long)facs;
    fadt->x_firmware_ctl_h = 0;
@@ -26,8 +25,7 @@
    fadt->x_dsdt_h = 0;

acpi_fill_in_fadt(fadt);
-
-	fadt->iapc_boot_arch &= ~ACPI_FADT_8042;
+	motherboard_fill_fadt(fadt);

header->checksum =
        acpi_checksum((void *) fadt, header->length);
diff --git a/src/mainboard/protectli/vault_bsw/acpi_tables.c b/src/mainboard/protectli/vault_bsw/acpi_tables.c
index f502741..9ecb5ac 100644
--- a/src/mainboard/protectli/vault_bsw/acpi_tables.c
+++ b/src/mainboard/protectli/vault_bsw/acpi_tables.c
@@ -27,3 +27,9 @@

return current;
 }
+
+void motherboard_fill_fadt(acpi_fadt_t *fadt)
+{
+	fadt->preferred_pm_profile = PM_MOBILE;
+	fadt->iapc_boot_arch = ACPI_FADT_LEGACY_DEVICES;
+}
diff --git a/src/mainboard/protectli/vault_bsw/fadt.c b/src/mainboard/protectli/vault_bsw/fadt.c
index 755ac6e..2a32c79 100644
--- a/src/mainboard/protectli/vault_bsw/fadt.c
+++ b/src/mainboard/protectli/vault_bsw/fadt.c
@@ -19,7 +19,6 @@

fadt->firmware_ctrl = (unsigned long)facs;
    fadt->dsdt = (unsigned long) dsdt;
-	fadt->preferred_pm_profile = PM_MOBILE;

fadt->x_firmware_ctl_l = (unsigned long)facs;
    fadt->x_firmware_ctl_h = 0;
@@ -27,8 +26,7 @@
    fadt->x_dsdt_h = 0;

acpi_fill_in_fadt(fadt);
-
-	fadt->iapc_boot_arch = ACPI_FADT_LEGACY_DEVICES;
+	motherboard_fill_fadt(fadt);

header->checksum = acpi_checksum((void *)fadt, header->length);
 }

[9elements QA (Code Review)]

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

Change subject: acpi,mb/*: Use motherboard_fill_fadt()
......................................................................

Patch Set 5:

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/5257
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/5256
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/5255
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/5254

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