Change in coreboot[master]: mainboard/puff: Fix ACPI tables to advertise correct features
Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38454 )
Change subject: mainboard/puff: Fix ACPI tables to advertise correct features ......................................................................
mainboard/puff: Fix ACPI tables to advertise correct features
Provide Puff with it's own copy of ec.h copied from the baseboard/includes however with the battery, lid and ps2 defines stripped.
This is to ensure the correct ASL is generated so that we don't advertise PS2 keyboard support and battery/lid interrupts which don't exist.
BUG=b:147850335 BRANCH=none TEST=builds
Change-Id: If13bd124c7229ced996efb841980604d13be09af Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/variants/puff/include/variant/ec.h 1 file changed, 57 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/38454/1
diff --git a/src/mainboard/google/hatch/variants/puff/include/variant/ec.h b/src/mainboard/google/hatch/variants/puff/include/variant/ec.h index 768987d..dd11b3d 100644 --- a/src/mainboard/google/hatch/variants/puff/include/variant/ec.h +++ b/src/mainboard/google/hatch/variants/puff/include/variant/ec.h @@ -1,6 +1,7 @@ /* * This file is part of the coreboot project. * + * Copyright (C) 2018 Intel Corporation. * Copyright 2019 Google LLC * * This program is free software; you can redistribute it and/or modify @@ -16,6 +17,60 @@ #ifndef VARIANT_EC_H #define VARIANT_EC_H
-#include <baseboard/ec.h> +#include <ec/google/chromeec/ec_commands.h> +#include <variant/gpio.h>
-#endif +#define MAINBOARD_EC_SCI_EVENTS \ + (EC_HOST_EVENT_MASK(EC_HOST_EVENT_LID_CLOSED) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_AC_CONNECTED) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_AC_DISCONNECTED) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_THERMAL_THRESHOLD) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_THROTTLE_START) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_THROTTLE_STOP) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_MODE_CHANGE) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_MKBP)) + +#define MAINBOARD_EC_SMI_EVENTS \ + (EC_HOST_EVENT_MASK(EC_HOST_EVENT_LID_CLOSED)) + +/* EC can wake from S5 with power button */ +#define MAINBOARD_EC_S5_WAKE_EVENTS \ + (EC_HOST_EVENT_MASK(EC_HOST_EVENT_POWER_BUTTON)) + +/* + * EC can wake from S3 with power button or key press or + * mode change event. + */ +#define MAINBOARD_EC_S3_WAKE_EVENTS \ + (MAINBOARD_EC_S5_WAKE_EVENTS |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_KEY_PRESSED) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_MODE_CHANGE)) + +#define MAINBOARD_EC_S0IX_WAKE_EVENTS \ + (MAINBOARD_EC_S3_WAKE_EVENTS | \ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_HANG_DETECT)) + +/* Log EC wake events plus EC shutdown events */ +#define MAINBOARD_EC_LOG_EVENTS \ + (EC_HOST_EVENT_MASK(EC_HOST_EVENT_THERMAL_SHUTDOWN) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_BATTERY_SHUTDOWN) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_PANIC)) + +/* + * ACPI related definitions for ASL code. + */ + +/* Enable EC backed ALS device in ACPI */ +#define EC_ENABLE_ALS_DEVICE + +/* Enable EC backed PD MCU device in ACPI */ +#define EC_ENABLE_PD_MCU_DEVICE + +#define SIO_EC_MEMMAP_ENABLE /* EC Memory Map Resources */ +#define SIO_EC_HOST_ENABLE /* EC Host Interface Resources */ + +/* Enable EC sync interrupt, EC_SYNC_IRQ is defined in baseboard/gpio.h */ +#define EC_ENABLE_SYNC_IRQ + +#endif /* VARIANT_EC_H */
Hello Kangheui Won, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38454
to look at the new patch set (#2).
Change subject: mainboard/puff: Fix ACPI tables to advertise correct features ......................................................................
mainboard/puff: Fix ACPI tables to advertise correct features
Provide Puff with it's own copy of ec.h copied from the baseboard/includes however with the battery, lid and ps2 defines stripped.
This is to ensure the correct ASL is generated so that we don't advertise PS2 keyboard support and battery/lid interrupts which don't exist.
Unfortunately the EC_HOST_EVENT_LID_CLOSED smi event defined here is baked into the smihandler.c in the baseboard so we leave that in for now.
V.2: drop EC_ENABLE_ALS_DEVICE as well.
BUG=b:147850335 BRANCH=none TEST=builds
Change-Id: If13bd124c7229ced996efb841980604d13be09af Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/variants/puff/include/variant/ec.h 1 file changed, 54 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/38454/2
Hello Kangheui Won, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38454
to look at the new patch set (#3).
Change subject: mainboard/puff: Fix ACPI tables to advertise correct features ......................................................................
mainboard/puff: Fix ACPI tables to advertise correct features
Provide Puff with it's own copy of ec.h copied from the baseboard/includes however with the battery, lid and ps2 defines stripped.
This is to ensure the correct ASL is generated so that we don't advertise PS2 keyboard support and battery/lid interrupts which don't exist.
V.2: drop EC_ENABLE_ALS_DEVICE as well. V.3: set MAINBOARD_EC_SMI_EVENTS to 0 and drop EC_HOST_EVENT_LID_CLOSED smi event.
BUG=b:147850335 BRANCH=none TEST=builds
Change-Id: If13bd124c7229ced996efb841980604d13be09af Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/variants/puff/include/variant/ec.h 1 file changed, 52 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/38454/3
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38454 )
Change subject: mainboard/puff: Fix ACPI tables to advertise correct features ......................................................................
Patch Set 3:
I think we still need this, #define EC_ENABLE_WAKE_PIN GPE_EC_WAKE
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38454 )
Change subject: mainboard/puff: Fix ACPI tables to advertise correct features ......................................................................
Patch Set 3:
(4 comments)
Patch Set 3:
I think we still need this, #define EC_ENABLE_WAKE_PIN GPE_EC_WAKE
+1
https://review.coreboot.org/c/coreboot/+/38454/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/include/variant/ec.h:
https://review.coreboot.org/c/coreboot/+/38454/3/src/mainboard/google/hatch/... PS3, Line 30: EC_HOST_EVENT_MASK(EC_HOST_EVENT_MODE_CHANGE) This won't be required for Puff. It is based on mode change between laptop <-> tablet mode.
https://review.coreboot.org/c/coreboot/+/38454/3/src/mainboard/google/hatch/... PS3, Line 45: EC_HOST_EVENT_KEY_PRESSED This won't be required for Puff. It is basically keyboard controller on the EC indicating a key press.
https://review.coreboot.org/c/coreboot/+/38454/3/src/mainboard/google/hatch/... PS3, Line 46: EC_HOST_EVENT_MODE_CHANGE This won't be required for Puff.
https://review.coreboot.org/c/coreboot/+/38454/3/src/mainboard/google/hatch/... PS3, Line 55: EC_HOST_EVENT_BATTERY_SHUTDOWN I don't think this will be required?
Hello Kangheui Won, Daniel Kurtz, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38454
to look at the new patch set (#4).
Change subject: mainboard/puff: Fix ACPI tables to advertise correct features ......................................................................
mainboard/puff: Fix ACPI tables to advertise correct features
Provide Puff with it's own copy of ec.h copied from the baseboard/includes however with the battery, lid and ps2 defines stripped.
This is to ensure the correct ASL is generated so that we don't advertise PS2 keyboard support and battery/lid interrupts which don't exist.
V.2: drop EC_ENABLE_ALS_DEVICE as well. V.3: set MAINBOARD_EC_SMI_EVENTS to 0 and drop EC_HOST_EVENT_LID_CLOSED smi event. V.4: drop EC_HOST_EVENT_MODE_CHANGE && provide wake pin for EC for _PRW WoL method
BUG=b:147850335 BRANCH=none TEST=builds
Change-Id: If13bd124c7229ced996efb841980604d13be09af Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/variants/puff/include/variant/ec.h 1 file changed, 53 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/38454/4
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38454 )
Change subject: mainboard/puff: Fix ACPI tables to advertise correct features ......................................................................
Patch Set 3:
Patch Set 3:
I think we still need this, #define EC_ENABLE_WAKE_PIN GPE_EC_WAKE
Ah yes you are correct for WoL to work. Nice catch, Fixed!
Hello Kangheui Won, Daniel Kurtz, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38454
to look at the new patch set (#5).
Change subject: mainboard/puff: Fix ACPI tables to advertise correct features ......................................................................
mainboard/puff: Fix ACPI tables to advertise correct features
Provide Puff with it's own copy of ec.h copied from the baseboard/includes however with the battery, lid and ps2 defines stripped.
This is to ensure the correct ASL is generated so that we don't advertise PS2 keyboard support and battery/lid interrupts which don't exist.
V.2: drop EC_ENABLE_ALS_DEVICE as well. V.3: set MAINBOARD_EC_SMI_EVENTS to 0 and drop EC_HOST_EVENT_LID_CLOSED smi event. V.4: drop EC_HOST_EVENT_MODE_CHANGE && provide wake pin for EC for _PRW WoL method V.5: drop EC_HOST_EVENT_KEY_PRESSED
BUG=b:147850335 BRANCH=none TEST=builds
Change-Id: If13bd124c7229ced996efb841980604d13be09af Signed-off-by: Edward O'Callaghan quasisec@google.com --- M src/mainboard/google/hatch/variants/puff/include/variant/ec.h 1 file changed, 47 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/38454/5
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38454 )
Change subject: mainboard/puff: Fix ACPI tables to advertise correct features ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38454/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/include/variant/ec.h:
https://review.coreboot.org/c/coreboot/+/38454/3/src/mainboard/google/hatch/... PS3, Line 30: EC_HOST_EVENT_MASK(EC_HOST_EVENT_MODE_CHANGE)
This won't be required for Puff. It is based on mode change between laptop <-> tablet mode.
yep, fixed already.
https://review.coreboot.org/c/coreboot/+/38454/3/src/mainboard/google/hatch/... PS3, Line 45: EC_HOST_EVENT_KEY_PRESSED
This won't be required for Puff. […]
Ah I see.
https://review.coreboot.org/c/coreboot/+/38454/3/src/mainboard/google/hatch/... PS3, Line 46: EC_HOST_EVENT_MODE_CHANGE
This won't be required for Puff.
.
https://review.coreboot.org/c/coreboot/+/38454/3/src/mainboard/google/hatch/... PS3, Line 55: EC_HOST_EVENT_BATTERY_SHUTDOWN
I don't think this will be required?
thx i missed one bat case.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38454 )
Change subject: mainboard/puff: Fix ACPI tables to advertise correct features ......................................................................
Patch Set 5: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38454 )
Change subject: mainboard/puff: Fix ACPI tables to advertise correct features ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38454/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38454/5//COMMIT_MSG@7 PS5, Line 7: mainboard/puff: Fix ACPI tables to advertise correct features I believe the variant prefix for Chrome devices is different.
Also, maybe:
Advertise correct features in ACPI tables
is shorter, and it’s clear, it’s a fix.
https://review.coreboot.org/c/coreboot/+/38454/5//COMMIT_MSG@15 PS5, Line 15: don't exist. Text width is 75 characters.
https://review.coreboot.org/c/coreboot/+/38454/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/include/variant/ec.h:
https://review.coreboot.org/c/coreboot/+/38454/5/src/mainboard/google/hatch/... PS5, Line 4: * Copyright (C) 2018 Intel Corporation. Why? You are employed by Google?
https://review.coreboot.org/c/coreboot/+/38454/5/src/mainboard/google/hatch/... PS5, Line 51: * ACPI related definitions for ASL code. Dot/period could be removed.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38454 )
Change subject: mainboard/puff: Fix ACPI tables to advertise correct features ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38454/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38454/5//COMMIT_MSG@7 PS5, Line 7: mainboard/puff: Fix ACPI tables to advertise correct features
I believe the variant prefix for Chrome devices is different. […]
I disagree.
https://review.coreboot.org/c/coreboot/+/38454/5//COMMIT_MSG@15 PS5, Line 15: don't exist.
Text width is 75 characters.
It is perfectly readable Paul and doesn't exceed limits in unreasonable ways.
https://review.coreboot.org/c/coreboot/+/38454/5/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/puff/include/variant/ec.h:
https://review.coreboot.org/c/coreboot/+/38454/5/src/mainboard/google/hatch/... PS5, Line 4: * Copyright (C) 2018 Intel Corporation.
Why? You are employed by Google?
Patch review isn't the correct forum or place to ask such personal questions.
https://review.coreboot.org/c/coreboot/+/38454/5/src/mainboard/google/hatch/... PS5, Line 51: * ACPI related definitions for ASL code.
Dot/period could be removed.
Ack
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38454 )
Change subject: mainboard/puff: Fix ACPI tables to advertise correct features ......................................................................
mainboard/puff: Fix ACPI tables to advertise correct features
Provide Puff with it's own copy of ec.h copied from the baseboard/includes however with the battery, lid and ps2 defines stripped.
This is to ensure the correct ASL is generated so that we don't advertise PS2 keyboard support and battery/lid interrupts which don't exist.
V.2: drop EC_ENABLE_ALS_DEVICE as well. V.3: set MAINBOARD_EC_SMI_EVENTS to 0 and drop EC_HOST_EVENT_LID_CLOSED smi event. V.4: drop EC_HOST_EVENT_MODE_CHANGE && provide wake pin for EC for _PRW WoL method V.5: drop EC_HOST_EVENT_KEY_PRESSED
BUG=b:147850335 BRANCH=none TEST=builds
Change-Id: If13bd124c7229ced996efb841980604d13be09af Signed-off-by: Edward O'Callaghan quasisec@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38454 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/mainboard/google/hatch/variants/puff/include/variant/ec.h 1 file changed, 47 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/mainboard/google/hatch/variants/puff/include/variant/ec.h b/src/mainboard/google/hatch/variants/puff/include/variant/ec.h index 768987d..501fab0 100644 --- a/src/mainboard/google/hatch/variants/puff/include/variant/ec.h +++ b/src/mainboard/google/hatch/variants/puff/include/variant/ec.h @@ -1,6 +1,7 @@ /* * This file is part of the coreboot project. * + * Copyright (C) 2018 Intel Corporation. * Copyright 2019 Google LLC * * This program is free software; you can redistribute it and/or modify @@ -16,6 +17,50 @@ #ifndef VARIANT_EC_H #define VARIANT_EC_H
-#include <baseboard/ec.h> +#include <ec/google/chromeec/ec_commands.h> +#include <variant/gpio.h>
-#endif +#define MAINBOARD_EC_SCI_EVENTS \ + (EC_HOST_EVENT_MASK(EC_HOST_EVENT_AC_CONNECTED) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_AC_DISCONNECTED) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_THERMAL_THRESHOLD) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_THROTTLE_START) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_THROTTLE_STOP) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_MKBP)) + +#define MAINBOARD_EC_SMI_EVENTS 0 + +/* EC can wake from S5 with power button */ +#define MAINBOARD_EC_S5_WAKE_EVENTS \ + (EC_HOST_EVENT_MASK(EC_HOST_EVENT_POWER_BUTTON)) + +/* EC can wake from S3 with power button */ +#define MAINBOARD_EC_S3_WAKE_EVENTS (MAINBOARD_EC_S5_WAKE_EVENTS) + +#define MAINBOARD_EC_S0IX_WAKE_EVENTS \ + (MAINBOARD_EC_S3_WAKE_EVENTS | \ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_HANG_DETECT)) + +/* Log EC wake events plus EC shutdown events */ +#define MAINBOARD_EC_LOG_EVENTS \ + (EC_HOST_EVENT_MASK(EC_HOST_EVENT_THERMAL_SHUTDOWN) |\ + EC_HOST_EVENT_MASK(EC_HOST_EVENT_PANIC)) + +/* + * ACPI related definitions for ASL code. + */ + +/* Enable EC backed PD MCU device in ACPI */ +#define EC_ENABLE_PD_MCU_DEVICE + +/* Provide wake pin for EC for _PRW WoL method */ +#define EC_ENABLE_WAKE_PIN GPE_EC_WAKE + +#define SIO_EC_MEMMAP_ENABLE /* EC Memory Map Resources */ +#define SIO_EC_HOST_ENABLE /* EC Host Interface Resources */ + +/* Enable EC sync interrupt, EC_SYNC_IRQ is defined in baseboard/gpio.h */ +#define EC_ENABLE_SYNC_IRQ + +#endif /* VARIANT_EC_H */
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38454 )
Change subject: mainboard/puff: Fix ACPI tables to advertise correct features ......................................................................
Patch Set 6:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/148EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/147EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/146 Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Edward O'Callaghan (Code Review)
-
Furquan Shaikh (Code Review)
-
Kangheui Won (Code Review)
-
Paul Menzel (Code Review)