Paul Menzel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40007 )
Change subject: mb/asus/p2b{,-ls}: Get rid of power button device ......................................................................
mb/asus/p2b{,-ls}: Get rid of power button device
Currently, two power buttons are exposed in ACPI, and detected by the operating system.
[ 0.000000] Linux version 4.4.14-smp (root@hive) (gcc version 5.3.0 (GCC) ) #2 SMP Fri Jun 24 14:44:24 CDT 2016 […] [ 0.000000] DMI: ASUS P2B-LS/P2B-LS, BIOS 4.11-839-g11c5b3b180 01/10/2020 […] [ 23.887594] input: Power Button as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0C:00/input/input3 [ 23.887837] ACPI: Power Button [PWRB] [ 23.888158] input: Power Button as /devices/LNXSYSTM:00/LNXPWRBN:00/input/input4 [ 23.888383] ACPI: Power Button [PWRF]
Apply commit d7b88dcb (mb/google/x86-boards: Get rid of power button device in coreboot, CB:27272) [1] to Intel 440BX boards [1]:
As per the ACPI specification, there are two types of power button devices:
- Fixed hardware power button
- Generic hardware power button
Fixed hardware power button is added by the OSPM if POWER_BUTTON flag is not set in FADT by the BIOS. This device has its programming model in PM1x_EVT_BLK. All ACPI compliant OSes are expected to add this power button device by default if the power button FADT flag is not set.
On the other hand, generic hardware power button can be used by platforms if fixed register space cannot be used for the power button device. In order to support this, power button device object with HID PNP0C0C is expected to be added to ACPI tables. Additionally, POWER_BUTTON flag should be set to indicate the presence of control method for power button.
Chrome EC mainboards implemented the generic hardware power button in a broken manner i.e. power button object with HID PNP0C0C is added to ACPI however none of the boards set POWER_BUTTON flag in FADT. This results in Linux kernel adding both fixed hardware power button as well as generic hardware power button to the list of devices present on the system. Though this is mostly harmless, it is logically incorrect and can confuse any userspace utilities scanning the ACPI devices.
This change gets rid of the generic hardware power button from all google mainboards and relies completely on the fixed hardware power button.
[1]: https://review.coreboot.org/27272
TEST=Verify, Linux detects only the fixed hardware power button:
ACPI: Power Button [PWRF]
that means, the message below is absent from the Linux log.
ACPI: Power Button [PWRB]
Change-Id: I0f5b7aaf32366360de3cce58cd742651a2bb46ba Signed-off-by: Paul Menzel pmenzel@molgen.mpg.de --- M src/mainboard/asus/p2b-ls/dsdt.asl M src/mainboard/asus/p2b/dsdt.asl 2 files changed, 0 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/40007/1
diff --git a/src/mainboard/asus/p2b-ls/dsdt.asl b/src/mainboard/asus/p2b-ls/dsdt.asl index c79b786..c0ac141 100644 --- a/src/mainboard/asus/p2b-ls/dsdt.asl +++ b/src/mainboard/asus/p2b-ls/dsdt.asl @@ -107,15 +107,6 @@ /* Root of the bus hierarchy */ Scope (_SB) { - Device (PWRB) - { - /* Power Button Device */ - Name (_HID, EisaId ("PNP0C0C")) - Method (_STA, 0, NotSerialized) - { - Return (0x0B) - } - } #include <southbridge/intel/i82371eb/acpi/intx.asl>
PCI_INTX_DEV(LNKA, _SB.PCI0.PX40.PIRA, 1) diff --git a/src/mainboard/asus/p2b/dsdt.asl b/src/mainboard/asus/p2b/dsdt.asl index b52b4569..02b44f4 100644 --- a/src/mainboard/asus/p2b/dsdt.asl +++ b/src/mainboard/asus/p2b/dsdt.asl @@ -102,15 +102,6 @@ /* Root of the bus hierarchy */ Scope (_SB) { - Device (PWRB) - { - /* Power Button Device */ - Name (_HID, EisaId ("PNP0C0C")) - Method (_STA, 0, NotSerialized) - { - Return (0x0B) - } - } #include <southbridge/intel/i82371eb/acpi/intx.asl>
PCI_INTX_DEV(LNKA, _SB.PCI0.PX40.PIRA, 1)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40007 )
Change subject: mb/asus/p2b{,-ls}: Get rid of power button device ......................................................................
Patch Set 2: Code-Review+2
Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40007 )
Change subject: mb/asus/p2b{,-ls}: Get rid of power button device ......................................................................
Patch Set 2:
A patch train converting these boards into variants (CB:40007 et al) is in progress. They are eventually going to share one DSDT with some conditionals re IRQ routings. Can we wait till they get submitted and apply this work there?
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40007 )
Change subject: mb/asus/p2b{,-ls}: Get rid of power button device ......................................................................
Patch Set 2:
Patch Set 2:
A patch train converting these boards into variants (CB:40007 et al) is in progress. They are eventually going to share one DSDT with some conditionals re IRQ routings. Can we wait till they get submitted and apply this work there?
Fine by me. Feel free to also take this over.
Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40007 )
Change subject: mb/asus/p2b{,-ls}: Get rid of power button device ......................................................................
Patch Set 2:
I looked at the vendor DSDT dump deeper. The flag in question is hardcoded in sb/intel/i82371eb/fadt.c yet matches the value in the vendor dump, yet it also has PWRB. I want to try controlling flag from a config option and flip it for boards that do have PWRB... then again it's just 3 boards in one family and they all have it. Another reason to not proceed with this change.
Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40007 )
Change subject: mb/asus/p2b{,-ls}: Get rid of power button device ......................................................................
Patch Set 2:
Patch Set 2:
I looked at the vendor DSDT dump deeper. The flag in question is hardcoded in sb/intel/i82371eb/fadt.c yet matches the value in the vendor dump, yet it also has PWRB. I want to try controlling flag from a config option and flip it for boards that do have PWRB... then again it's just 3 boards in one family and they all have it. Another reason to not proceed with this change.
Looks like this issue is much more widespread. Even mb/amd/thatcher is like this - has PWRB in DSDT and the flag not set in FADT.
Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40007 )
Change subject: mb/asus/p2b{,-ls}: Get rid of power button device ......................................................................
Patch Set 2: Code-Review-1
I propose we add a a config option HAVE_ACPI_POWER_BUTTON which then changes the flag in generated FADT for all southbridges. This will need extensive boot testing though.
Keith Hui has uploaded a new patch set (#3) to the change originally created by Paul Menzel. ( https://review.coreboot.org/c/coreboot/+/40007 )
Change subject: mb/asus/p2b*: Get rid of power button device ......................................................................
mb/asus/p2b*: Get rid of power button device
These boards have the same issue as [27272]:
Currently, two power buttons are exposed in ACPI, and detected by the operating system.
As per the ACPI specification, there are two types of power button devices:
- Fixed hardware power button
- Generic hardware power button
Fixed hardware power button is added by the OSPM if POWER_BUTTON flag is not set in FADT by the BIOS. This device has its programming model in PM1x_EVT_BLK. All ACPI compliant OSes are expected to add this power button device by default if the power button FADT flag is not set.
On the other hand, generic hardware power button can be used by platforms if fixed register space cannot be used for the power button device. In order to support this, power button device object with HID PNP0C0C is expected to be added to ACPI tables. Additionally, POWER_BUTTON flag should be set to indicate the presence of control method for power button.
[i440BX] mainboards implemented the generic hardware power button in a broken manner i.e. power button object with HID PNP0C0C is added to ACPI however none of the boards set POWER_BUTTON flag in FADT. This results in Linux kernel adding both fixed hardware power button as well as generic hardware power button to the list of devices present on the system. Though this is mostly harmless, it is logically incorrect and can confuse any userspace utilities scanning the ACPI devices.
Hardware tests on the P2B-LS shows the generic hardware power button is not working anyway - with FADT power button flag set, the board could not power off with the button.
This change removes the generic hardware power button from all P2B mainboards and relies completely on the fixed hardware power button.
TEST=Booted on P2B-LS, Linux detects only fixed hardware power button, button still powers off.
[27272]: https://review.coreboot.org/27272
Change-Id: I0f5b7aaf32366360de3cce58cd742651a2bb46ba Signed-off-by: Keith Hui buurin@gmail.com --- M src/mainboard/asus/p2b/dsdt.asl 1 file changed, 0 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/40007/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40007 )
Change subject: mb/asus/p2b*: Get rid of power button device ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40007 )
Change subject: mb/asus/p2b*: Get rid of power button device ......................................................................
mb/asus/p2b*: Get rid of power button device
These boards have the same issue as [27272]:
Currently, two power buttons are exposed in ACPI, and detected by the operating system.
As per the ACPI specification, there are two types of power button devices:
- Fixed hardware power button
- Generic hardware power button
Fixed hardware power button is added by the OSPM if POWER_BUTTON flag is not set in FADT by the BIOS. This device has its programming model in PM1x_EVT_BLK. All ACPI compliant OSes are expected to add this power button device by default if the power button FADT flag is not set.
On the other hand, generic hardware power button can be used by platforms if fixed register space cannot be used for the power button device. In order to support this, power button device object with HID PNP0C0C is expected to be added to ACPI tables. Additionally, POWER_BUTTON flag should be set to indicate the presence of control method for power button.
[i440BX] mainboards implemented the generic hardware power button in a broken manner i.e. power button object with HID PNP0C0C is added to ACPI however none of the boards set POWER_BUTTON flag in FADT. This results in Linux kernel adding both fixed hardware power button as well as generic hardware power button to the list of devices present on the system. Though this is mostly harmless, it is logically incorrect and can confuse any userspace utilities scanning the ACPI devices.
Hardware tests on the P2B-LS shows the generic hardware power button is not working anyway - with FADT power button flag set, the board could not power off with the button.
This change removes the generic hardware power button from all P2B mainboards and relies completely on the fixed hardware power button.
TEST=Booted on P2B-LS, Linux detects only fixed hardware power button, button still powers off.
[27272]: https://review.coreboot.org/27272
Change-Id: I0f5b7aaf32366360de3cce58cd742651a2bb46ba Signed-off-by: Keith Hui buurin@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40007 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/asus/p2b/dsdt.asl 1 file changed, 0 insertions(+), 9 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/mainboard/asus/p2b/dsdt.asl b/src/mainboard/asus/p2b/dsdt.asl index 5bc5d72..e119871 100644 --- a/src/mainboard/asus/p2b/dsdt.asl +++ b/src/mainboard/asus/p2b/dsdt.asl @@ -97,15 +97,6 @@ /* Root of the bus hierarchy */ Scope (_SB) { - Device (PWRB) - { - /* Power Button Device */ - Name (_HID, EisaId ("PNP0C0C")) - Method (_STA, 0, NotSerialized) - { - Return (0x0B) - } - } #include <southbridge/intel/i82371eb/acpi/intx.asl>
PCI_INTX_DEV(LNKA, _SB.PCI0.PX40.PIRA, 1)
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40007 )
Change subject: mb/asus/p2b*: Get rid of power button device ......................................................................
Patch Set 4:
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/3129 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3128 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3127 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3126
Please note: This test is under development and might not be accurate at all!
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40007 )
Change subject: mb/asus/p2b*: Get rid of power button device ......................................................................
Patch Set 4:
Thank you!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Keith Hui (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)