Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41894 )
Change subject: vendorcode/google/chromeos: Add vendor_dptf_* functions ......................................................................
vendorcode/google/chromeos: Add vendor_dptf_* functions
This patch adds in the rest of the missing pieces from what is in the static DPTF ASL implementation. Much of it seems written for ChromeOS systems, making assumptions about _SB.PCI0.LPCB.EC0.* methods that may exist on other systems. Seeing also that they were only used for ChromeOS devices in coreboot, the corresponding generation of the AML was moved to vendorcode/google/chromeos.
BUG=b:143539650 TEST=compiles
Change-Id: Ib30072d1d0748b31bcab240a0fd0e2f12d34aaa4 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/vendorcode/google/chromeos/Makefile.inc A src/vendorcode/google/chromeos/dptf.c 2 files changed, 308 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/41894/1
diff --git a/src/vendorcode/google/chromeos/Makefile.inc b/src/vendorcode/google/chromeos/Makefile.inc index fc00374..936a298 100644 --- a/src/vendorcode/google/chromeos/Makefile.inc +++ b/src/vendorcode/google/chromeos/Makefile.inc @@ -13,6 +13,7 @@ ramstage-$(CONFIG_USE_SAR) += sar.c ramstage-$(CONFIG_CHROMEOS_DSM_CALIB) += dsm_calib.c ramstage-$(CONFIG_TPM_CR50) += cr50_enable_update.c +ramstage-$(CONFIG_DRIVERS_INTEL_DPTF) += dptf.c
bootblock-y += watchdog.c verstage-y += watchdog.c diff --git a/src/vendorcode/google/chromeos/dptf.c b/src/vendorcode/google/chromeos/dptf.c new file mode 100644 index 0000000..cbf7b48 --- /dev/null +++ b/src/vendorcode/google/chromeos/dptf.c @@ -0,0 +1,307 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <acpi/acpigen.h> +#include <acpi/acpigen_dptf.h> +#include <device/device.h> + +/* DPTF Event types */ +enum { + TRIP_POINTS_CHANGED_EVENT = 0x81, + THERMAL_EVENT = 0x90, +}; + +/* EC constants */ +enum { + EC_FAN_DUTY_AUTO = 0xFF, +}; + +static void write_charger_PPPC(void) +{ + acpigen_write_method_serialized("PPPC", 0); + + /* + * Convert size of PPSS table to index + * + * Store (SizeOf (PPSS), Local0) + * Decrement (Local0) + */ + acpigen_write_store(); + acpigen_emit_byte(SIZEOF_OP); + acpigen_emit_namestring("PPSS"); + acpigen_emit_byte(LOCAL0_OP); + acpigen_emit_byte(DECREMENT_OP); + acpigen_emit_byte(LOCAL0_OP); + + /* + * Check if charging is disabled (AC removed) + * + * If (_SB.PCI0.LPCB.EC0.ACEX () = Zero) { + * Return (Local0) + * } + */ + acpigen_write_if(); + acpigen_emit_byte(LEQUAL_OP); + acpigen_emit_namestring("\_SB.PCI0.LPCB.EC0.ACEX"); + acpigen_emit_byte(ZERO_OP); + acpigen_emit_byte(RETURN_OP); + acpigen_emit_byte(LOCAL0_OP); + acpigen_pop_len(); /* If */ + + /* + * Return highest power state + * + * Return (0) + */ + acpigen_emit_byte(RETURN_OP); + acpigen_emit_byte(ZERO_OP); + + acpigen_pop_len(); /* Method */ +} + +static void write_charger_SPPC(void) +{ + /* + * SPPC - Set charger current limit + * Method(SPPC, 1) { + * Store (DeRefOf (Index (DeRefOf (Index + * (PPSS, ToInteger (Arg0))), 4)), Local0) + * _SB.PCI0.LPCB.EC0.CHGS (Local0) + * } + */ + + acpigen_write_method_serialized("SPPC", 1); + + /* Retrieve Control (index 4) for specified PPSS level */ + acpigen_emit_byte(STORE_OP); + acpigen_emit_byte(DEREF_OP); + acpigen_emit_byte(INDEX_OP); + acpigen_emit_byte(DEREF_OP); + acpigen_emit_byte(INDEX_OP); + acpigen_emit_namestring("PPSS"); + acpigen_write_to_integer(ARG0_OP, ZERO_OP); + acpigen_emit_byte(ZERO_OP); /* 3rd arg to Index */ + acpigen_write_integer(4); /* Index */ + acpigen_emit_byte(ZERO_OP); /* 3rd arg to Index */ + acpigen_emit_byte(LOCAL0_OP); + + /* Pass Control value to EC to limit charging */ + acpigen_emit_namestring("\_SB.PCI0.LPCB.EC0.CHGS"); + acpigen_emit_byte(LOCAL0_OP); + acpigen_pop_len(); /* Method */ +} + +static void write_fan_fst(void) +{ + /* TFST is a package that is used to store data from FAND */ + acpigen_write_name("TFST"); + acpigen_write_package(3); + acpigen_write_integer(0); /* Revision */ + acpigen_write_integer(0); /* Control */ + acpigen_write_integer(0); /* Speed */ + acpigen_pop_len(); /* Package */ + + /* _FST */ + acpigen_write_method_serialized("_FST", 0); + acpigen_write_store(); + acpigen_emit_namestring("\_SB.PCI0.LPCB.EC0.FAND"); + acpigen_emit_byte(INDEX_OP); + acpigen_emit_namestring("TFST"); + acpigen_write_integer(1); + acpigen_emit_byte(ZERO_OP); /* 3rd arg to Index */ + acpigen_emit_byte(RETURN_OP); + acpigen_emit_namestring("TFST"); + acpigen_pop_len(); /* Method _FST */ +} + +static void write_fan_fsl(void) +{ + /* _FSL */ + acpigen_write_method_serialized("_FSL", 1); + acpigen_write_store(); + acpigen_emit_byte(ARG0_OP); + acpigen_emit_namestring("\_SB.PCI0.LPCB.EC0.FAND"); + acpigen_pop_len(); /* Method _FSL */ +} + +/* Note: requires manual insertion of acpigen_pop_len() for the If */ +static void write_is_policy_enabled(unsigned int index, bool enabled) +{ + /* + * If (And (LEqual (Deref (Index (IDSP, index)), Arg0)) + * (LEqual (Arg1, enabled))) + */ + acpigen_write_if(); + acpigen_emit_byte(LAND_OP); + acpigen_emit_byte(LEQUAL_OP); + acpigen_emit_byte(DEREF_OP); + acpigen_emit_byte(INDEX_OP); + acpigen_emit_namestring("IDSP"); + acpigen_write_integer(index); + acpigen_emit_byte(ZERO_OP); /* 3rd arg of index - unused */ + acpigen_emit_byte(ARG0_OP); /* end lequal */ + acpigen_emit_byte(LEQUAL_OP); + acpigen_emit_byte(ARG1_OP); + acpigen_write_integer(enabled ? 1 : 0); +} + +static void write_dptf_OSC(bool active_policy_enabled) +{ + /* + * Arg0: Buffer containing UUID + * Arg1: "Integer containing Revision ID of buffer format", but Linux passes whether + * it is enabling (1) or disabling (0) the policy in Arg1. + * Arg2: Integer containing count of entries in Arg3 + * Arg3: Buffer containing list of DWORD capabilities + * Return: Buffer containing list of DWORD capabilities + */ + acpigen_write_method_serialized("_OSC", 4); + + /* Is the OS passing us the Passive Policy? */ + write_is_policy_enabled(0, true); + acpigen_emit_namestring("^TINI"); + acpigen_emit_namestring("^TCHG.INIT"); + acpigen_pop_len(); /* If */ + + /* If the Active Policy is disabled, disable DPTF fan control in the EC */ + if (active_policy_enabled) { + write_is_policy_enabled(2, false); + acpigen_write_store(); + acpigen_write_integer(EC_FAN_DUTY_AUTO); + acpigen_emit_namestring("\_SB.PCI0.LPCB.EC0.FAND"); + acpigen_pop_len(); /* If */ + } + + acpigen_emit_byte(RETURN_OP); + acpigen_emit_byte(ARG3_OP); + acpigen_pop_len(); /* Method _OSC */ +} + +void vendor_dptf_write_dppm_methods(bool active_policy_enabled, bool tsr_en[4]) +{ + enum dptf_participant p; + char name[16]; + int i; + + acpigen_write_scope("\_SB.DPTF"); + write_dptf_OSC(active_policy_enabled); + + /* TEVT */ + if (CONFIG(EC_SUPPORTS_DPTF_TEVT)) { + acpigen_write_method("TEVT", 0); + + /* Local0 = ToInteger(Arg0) */ + acpigen_write_to_integer(ARG0_OP, LOCAL0_OP); + for (p = DPTF_TSR0, i = 0; p <= DPTF_TSR3; ++p, ++i) { + if (!tsr_en[i]) + continue; + + snprintf(name, sizeof(name), "^TSR%1d", i); + + /* If (Local0 = i) Notify (TSRx, 0x90) */ + acpigen_write_if(); + acpigen_emit_byte(LEQUAL_OP); + acpigen_emit_byte(LOCAL0_OP); + acpigen_write_integer(i); /* TMPI */ + acpigen_emit_byte(NOTIFY_OP); + acpigen_emit_namestring(name); + acpigen_write_integer(THERMAL_EVENT); + acpigen_pop_len(); /* If */ + } + + acpigen_pop_len(); /* Method */ + } + + /* TINI */ + acpigen_write_method("TINI", 0); + for (i = 0; i < DPTF_MAX_TSR; ++i) { + if (!tsr_en[i]) + continue; + snprintf(name, sizeof(name), "^TSR%1d.PATD", i); + acpigen_emit_namestring(name); + } + + acpigen_pop_len(); /* Method */ + + /* TPET */ + acpigen_write_method("TPET", 0); + for (p = DPTF_TSR0, i = 0; p <= DPTF_TSR3; ++p, ++i) { + if (!tsr_en[i]) + continue; + + snprintf(name, sizeof(name), "^TSR%1d", i); + acpigen_emit_byte(NOTIFY_OP); + acpigen_emit_namestring(name); + acpigen_write_integer(TRIP_POINTS_CHANGED_EVENT); + } + + acpigen_pop_len(); /* Method */ + acpigen_pop_len(); /* Scope */ +} + +static void write_charger_init(void) +{ + /* Initialize charger participant */ + acpigen_write_method_serialized("INIT", 0); + + /* Disable charge limit */ + acpigen_emit_namestring("\_SB.PCI0.LPCB.EC0.CHGD"); + acpigen_pop_len(); /* Method */ +} + +void vendor_dptf_write_charger_methods(void) +{ + dptf_write_scope(DPTF_CHARGER); + + write_charger_PPPC(); + write_charger_SPPC(); + write_charger_init(); + + acpigen_pop_len(); /* Scope */ +} + +void vendor_dptf_write_fan_methods(void) +{ + dptf_write_scope(DPTF_FAN); + + write_fan_fsl(); + write_fan_fst(); + + acpigen_pop_len(); /* Scope */ +} + +void vendor_dptf_write_thermal_methods(int tsr_index) +{ + dptf_write_scope((enum dptf_participant)(DPTF_TSR0 + tsr_index)); + + /* _TMP - read temperature from EC */ + acpigen_write_method_serialized("_TMP", 0); + acpigen_emit_byte(RETURN_OP); + acpigen_emit_namestring("\_SB.PCI0.LPCB.EC0.TSRD"); + acpigen_emit_namestring("TMPI"); + acpigen_pop_len(); /* Method _TMP */ + + /* PATC - Aux trip point count */ + acpigen_write_name_integer("PATC", 2); + + /* PAT0 - Set Aux trip point 0 */ + acpigen_write_method_serialized("PAT0", 1); + acpigen_emit_namestring("\_SB.PCI0.LPCB.EC0.PAT0"); + acpigen_emit_namestring("TMPI"); + acpigen_emit_byte(ARG0_OP); + acpigen_pop_len(); /* Method PAT0 */ + + /* PAT1 - Set Aux trip point 1 */ + acpigen_write_method_serialized("PAT1", 1); + acpigen_emit_namestring("\_SB.PCI0.LPCB.EC0.PAT1"); + acpigen_emit_namestring("TMPI"); + acpigen_emit_byte(ARG0_OP); + acpigen_pop_len(); /* Method PAT0 */ + + /* PATD - Disable Aux trip point */ + acpigen_write_method_serialized("PATD", 0); + acpigen_emit_namestring("\_SB.PCI0.LPCB.EC0.PATD"); + acpigen_emit_namestring("TMPI"); + acpigen_pop_len(); /* Method PAT0 */ + + acpigen_pop_len(); /* Scope */ +}
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41894
to look at the new patch set (#4).
Change subject: vendorcode/google/chromeos: Add vendor_dptf_* functions ......................................................................
vendorcode/google/chromeos: Add vendor_dptf_* functions
This patch adds in the rest of the missing pieces from what is in the static DPTF ASL implementation. Much of it seems written for ChromeOS systems, making assumptions about _SB.PCI0.LPCB.EC0.* methods that may exist on other systems. Seeing also that they were only used for ChromeOS devices in coreboot, the corresponding generation of the AML was moved to vendorcode/google/chromeos.
BUG=b:143539650 TEST=compiles
Change-Id: Ib30072d1d0748b31bcab240a0fd0e2f12d34aaa4 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/vendorcode/google/chromeos/Makefile.inc A src/vendorcode/google/chromeos/dptf.c 2 files changed, 308 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/41894/4
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41894 )
Change subject: vendorcode/google/chromeos: Add vendor_dptf_* functions ......................................................................
Patch Set 5:
Just a note as I have not looked at this in detail yet but some of these acpigen sequences are similar to ones I am adding helpers for: https://review.coreboot.org/c/coreboot/+/41985
Your use cases look a bit different and the functions might need different variants than what these use (string vs operator, etc) but it might be useful to see if any of the acpigen you are using could be similarly extracted.
If we had an acpi data type abstraction we could condense some of these multiple functions that all do the same thing with different data types...
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41894 )
Change subject: vendorcode/google/chromeos: Add vendor_dptf_* functions ......................................................................
Patch Set 5:
Patch Set 5:
Just a note as I have not looked at this in detail yet but some of these acpigen sequences are similar to ones I am adding helpers for: https://review.coreboot.org/c/coreboot/+/41985
Your use cases look a bit different and the functions might need different variants than what these use (string vs operator, etc) but it might be useful to see if any of the acpigen you are using could be similarly extracted.
If we had an acpi data type abstraction we could condense some of these multiple functions that all do the same thing with different data types...
Gotcha, will rebase and see if I can use some of those (the more the better).
Perhaps something like a tagged union of ACPI data types? enum asl_type { ASL_INTEGER, ASL_BYTE, ASL_STRING, etc. };
and struct asl_value { enum asl_type type; union { you get the idea }; };
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Duncan Laurie, Sumeet R Pawnikar, Subrata Banik, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41894
to look at the new patch set (#6).
Change subject: vendorcode/google/chromeos: Add vendor_dptf_* functions ......................................................................
vendorcode/google/chromeos: Add vendor_dptf_* functions
This patch adds in the rest of the missing pieces from what is in the static DPTF ASL implementation. Much of it seems written for ChromeOS systems, making assumptions about _SB.PCI0.LPCB.EC0.* methods that may exist on other systems. Seeing also that they were only used for ChromeOS devices in coreboot, the corresponding generation of the AML was moved to vendorcode/google/chromeos.
BUG=b:143539650 TEST=compiles
Change-Id: Ib30072d1d0748b31bcab240a0fd0e2f12d34aaa4 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/vendorcode/google/chromeos/Makefile.inc A src/vendorcode/google/chromeos/dptf.c 2 files changed, 301 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/41894/6
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Duncan Laurie, Sumeet R Pawnikar, Subrata Banik, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41894
to look at the new patch set (#7).
Change subject: vendorcode/google/chromeos: Add vendor_dptf_* functions ......................................................................
vendorcode/google/chromeos: Add vendor_dptf_* functions
This patch adds in the rest of the missing pieces from what is in the static DPTF ASL implementation. Much of it seems written for ChromeOS systems, making assumptions about _SB.PCI0.LPCB.EC0.* methods that may exist on other systems. Seeing also that they were only used for ChromeOS devices in coreboot, the corresponding generation of the AML was moved to vendorcode/google/chromeos.
BUG=b:143539650 TEST=compiles
Change-Id: Ib30072d1d0748b31bcab240a0fd0e2f12d34aaa4 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/vendorcode/google/chromeos/Makefile.inc A src/vendorcode/google/chromeos/dptf.c 2 files changed, 304 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/41894/7
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41894 )
Change subject: vendorcode/google/chromeos: Add vendor_dptf_* functions ......................................................................
Patch Set 7:
(3 comments)
I wonder if this shouldn't come from ec/google/chromeec rather than chromeos. in theory you could still use the chrome ec with DPTF but without CONFIG_CHROMEOS.
I guess that makes it hard for ec/google/wilco to use it, so maybe it needs something new in ec/google/dptf...
https://review.coreboot.org/c/coreboot/+/41894/7/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/dptf.c:
https://review.coreboot.org/c/coreboot/+/41894/7/src/vendorcode/google/chrom... PS7, Line 194: acpigen_write_if(); : acpigen_emit_byte(LEQUAL_OP); : acpigen_emit_byte(LOCAL0_OP); : acpigen_write_integer(i); /* TMPI */ is this acpigen_write_if_lequal_op_int()?
https://review.coreboot.org/c/coreboot/+/41894/7/src/vendorcode/google/chrom... PS7, Line 198: acpigen_emit_byte(NOTIFY_OP); : acpigen_emit_namestring(name); : acpigen_write_integer(THERMAL_EVENT); this seems like a good candidate for a new function like acpigen_notify(namestr, val) since it gets used a couple times.
https://review.coreboot.org/c/coreboot/+/41894/7/src/vendorcode/google/chrom... PS7, Line 276: "TMPI" could this just use tsr_index to build a name instead of relying on TMPI to be defined?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41894 )
Change subject: vendorcode/google/chromeos: Add vendor_dptf_* functions ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/41894/7/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/dptf.c:
https://review.coreboot.org/c/coreboot/+/41894/7/src/vendorcode/google/chrom... PS7, Line 178: \_SB.DPTF" note to self: this should probably be exported from acpigen_dptf.h instead and used here too.
https://review.coreboot.org/c/coreboot/+/41894/7/src/vendorcode/google/chrom... PS7, Line 194: acpigen_write_if(); : acpigen_emit_byte(LEQUAL_OP); : acpigen_emit_byte(LOCAL0_OP); : acpigen_write_integer(i); /* TMPI */
is this acpigen_write_if_lequal_op_int()?
Done
https://review.coreboot.org/c/coreboot/+/41894/7/src/vendorcode/google/chrom... PS7, Line 198: acpigen_emit_byte(NOTIFY_OP); : acpigen_emit_namestring(name); : acpigen_write_integer(THERMAL_EVENT);
this seems like a good candidate for a new function like acpigen_notify(namestr, val) since it gets […]
Done
https://review.coreboot.org/c/coreboot/+/41894/7/src/vendorcode/google/chrom... PS7, Line 276: "TMPI"
could this just use tsr_index to build a name instead of relying on TMPI to be defined?
It sure could; I wasn't sure whether TMPI was defined for some other reason or not... I didn't see anything, and the DPTF spec doesn't mention anything. It also didn't "need" to exist in the original implementation either, since each TSR is written out by hand, so I was a little hesitant to just remove it. But I guess since it isn't mentioned in the spec I can remove it.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41894 )
Change subject: vendorcode/google/chromeos: Add vendor_dptf_* functions ......................................................................
Patch Set 7:
Patch Set 7:
(3 comments)
I wonder if this shouldn't come from ec/google/chromeec rather than chromeos. in theory you could still use the chrome ec with DPTF but without CONFIG_CHROMEOS.
I guess that makes it hard for ec/google/wilco to use it, so maybe it needs something new in ec/google/dptf...
Interesting thought... almost everything (except vendor_dptf_write_dppm_methods()) is calls into our EC ASL... it's not in EC scope, but I could see an ec_dptf_helpers module instead... let me see how that looks.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41894 )
Change subject: vendorcode/google/chromeos: Add vendor_dptf_* functions ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41894/7/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/dptf.c:
https://review.coreboot.org/c/coreboot/+/41894/7/src/vendorcode/google/chrom... PS7, Line 198: acpigen_emit_byte(NOTIFY_OP); : acpigen_emit_namestring(name); : acpigen_write_integer(THERMAL_EVENT);
Done
It is only 2 times, usually the 3rd time is when I refactor 😉 but it seems useful enough on its own.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Duncan Laurie, Sumeet R Pawnikar, Subrata Banik, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41894
to look at the new patch set (#8).
Change subject: ec/google: Add function ec_fill_dptf_helpers() ......................................................................
ec/google: Add function ec_fill_dptf_helpers()
ec_fill_dptf_helpers() is used to generate all of the "helper" methods that DPTF requires. A system with a Chrome EC is typically in charge of fan PWM control as well as battery charging, so if DPTF needs to manipulate those, then it requires Methods provided by the EC.
BUG=b:143539650 TEST=compiles
Change-Id: Ib30072d1d0748b31bcab240a0fd0e2f12d34aaa4 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec_acpi.c A src/ec/google/chromeec/ec_dptf_helpers.c A src/ec/google/common/dptf.h 4 files changed, 330 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/41894/8
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41894 )
Change subject: ec/google: Add function ec_fill_dptf_helpers() ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41894/8/src/ec/google/chromeec/ec_d... File src/ec/google/chromeec/ec_dptf_helpers.c:
https://review.coreboot.org/c/coreboot/+/41894/8/src/ec/google/chromeec/ec_d... PS8, Line 17: TRIP_POINTS_CHANGED_EVENT = 0x81, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/41894/8/src/ec/google/chromeec/ec_d... PS8, Line 18: THERMAL_EVENT = 0x90, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/41894/8/src/ec/google/chromeec/ec_d... PS8, Line 23: EC_FAN_DUTY_AUTO = 0xFF, please, no spaces at the start of a line
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Duncan Laurie, Sumeet R Pawnikar, Subrata Banik, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41894
to look at the new patch set (#9).
Change subject: ec/google: Add function ec_fill_dptf_helpers() ......................................................................
ec/google: Add function ec_fill_dptf_helpers()
ec_fill_dptf_helpers() is used to generate all of the "helper" methods that DPTF requires. A system with a Chrome EC is typically in charge of fan PWM control as well as battery charging, so if DPTF needs to manipulate those, then it requires Methods provided by the EC.
BUG=b:143539650 TEST=compiles
Change-Id: Ib30072d1d0748b31bcab240a0fd0e2f12d34aaa4 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec_acpi.c A src/ec/google/chromeec/ec_dptf_helpers.c A src/ec/google/common/dptf.h 4 files changed, 330 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/41894/9
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Duncan Laurie, Sumeet R Pawnikar, Subrata Banik, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41894
to look at the new patch set (#10).
Change subject: ec/google: Add function ec_fill_dptf_helpers() ......................................................................
ec/google: Add function ec_fill_dptf_helpers()
ec_fill_dptf_helpers() is used to generate all of the "helper" methods that DPTF requires. A system with a Chrome EC is typically in charge of fan PWM control as well as battery charging, so if DPTF needs to manipulate those, then it requires Methods provided by the EC.
BUG=b:143539650 TEST=compiles
Change-Id: Ib30072d1d0748b31bcab240a0fd0e2f12d34aaa4 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec_acpi.c A src/ec/google/chromeec/ec_dptf_helpers.c A src/ec/google/common/dptf.h 4 files changed, 328 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/41894/10
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41894 )
Change subject: ec/google: Add function ec_fill_dptf_helpers() ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41894/13/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_acpi.c:
https://review.coreboot.org/c/coreboot/+/41894/13/src/ec/google/chromeec/ec_... PS13, Line 251: please check, we use space instead of tab.
https://review.coreboot.org/c/coreboot/+/41894/13/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_dptf_helpers.c:
https://review.coreboot.org/c/coreboot/+/41894/13/src/ec/google/chromeec/ec_... PS13, Line 5: device.h This is also included in src/ec/google/common/dptf.h header file.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41894 )
Change subject: ec/google: Add function ec_fill_dptf_helpers() ......................................................................
Patch Set 13:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41894/13/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_acpi.c:
https://review.coreboot.org/c/coreboot/+/41894/13/src/ec/google/chromeec/ec_... PS13, Line 251:
please check, we use space instead of tab.
Hmm, it appears that we are not very consistent with this, I see tabs in some files here and spaces in others (for entries in struct device_operations)... 😞 I tend to stick with tabs as that is our indentation method here in coreboot.
https://review.coreboot.org/c/coreboot/+/41894/13/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_dptf_helpers.c:
https://review.coreboot.org/c/coreboot/+/41894/13/src/ec/google/chromeec/ec_... PS13, Line 5: device.h
This is also included in src/ec/google/common/dptf.h header file.
Done
https://review.coreboot.org/c/coreboot/+/41894/7/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/dptf.c:
https://review.coreboot.org/c/coreboot/+/41894/7/src/vendorcode/google/chrom... PS7, Line 178: \_SB.DPTF"
note to self: this should probably be exported from acpigen_dptf.h instead and used here too.
Ack
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Duncan Laurie, Sumeet R Pawnikar, Subrata Banik, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41894
to look at the new patch set (#14).
Change subject: ec/google: Add function ec_fill_dptf_helpers() ......................................................................
ec/google: Add function ec_fill_dptf_helpers()
ec_fill_dptf_helpers() is used to generate all of the "helper" methods that DPTF requires. A system with a Chrome EC is typically in charge of fan PWM control as well as battery charging, so if DPTF needs to manipulate those, then it requires Methods provided by the EC.
BUG=b:143539650 TEST=compiles
Change-Id: Ib30072d1d0748b31bcab240a0fd0e2f12d34aaa4 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec_acpi.c A src/ec/google/chromeec/ec_dptf_helpers.c A src/ec/google/common/dptf.h 4 files changed, 327 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/41894/14
Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41894 )
Change subject: ec/google: Add function ec_fill_dptf_helpers() ......................................................................
Patch Set 16: Code-Review+1
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41894 )
Change subject: ec/google: Add function ec_fill_dptf_helpers() ......................................................................
Patch Set 16: Code-Review+2
Duncan Laurie has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41894 )
Change subject: ec/google: Add function ec_fill_dptf_helpers() ......................................................................
ec/google: Add function ec_fill_dptf_helpers()
ec_fill_dptf_helpers() is used to generate all of the "helper" methods that DPTF requires. A system with a Chrome EC is typically in charge of fan PWM control as well as battery charging, so if DPTF needs to manipulate those, then it requires Methods provided by the EC.
BUG=b:143539650 TEST=compiles
Change-Id: Ib30072d1d0748b31bcab240a0fd0e2f12d34aaa4 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/41894 Reviewed-by: Nick Vaccaro nvaccaro@google.com Reviewed-by: Duncan Laurie dlaurie@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec_acpi.c A src/ec/google/chromeec/ec_dptf_helpers.c A src/ec/google/common/dptf.h 4 files changed, 327 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved Nick Vaccaro: Looks good to me, but someone else must approve
diff --git a/src/ec/google/chromeec/Makefile.inc b/src/ec/google/chromeec/Makefile.inc index 1f7e03d..a37d673 100644 --- a/src/ec/google/chromeec/Makefile.inc +++ b/src/ec/google/chromeec/Makefile.inc @@ -42,6 +42,8 @@ romstage-$(CONFIG_EC_GOOGLE_CHROMEEC_SWITCHES) += switches.c ramstage-$(CONFIG_EC_GOOGLE_CHROMEEC_SWITCHES) += switches.c
+ramstage-$(CONFIG_DRIVERS_INTEL_DPTF) += ec_dptf_helpers.c + CHROMEEC_SOURCE ?= $(top)/3rdparty/chromeec
# These are Chrome EC firmware images that a payload (such as depthcharge) can diff --git a/src/ec/google/chromeec/ec_acpi.c b/src/ec/google/chromeec/ec_acpi.c index dca5285..8a76805 100644 --- a/src/ec/google/chromeec/ec_acpi.c +++ b/src/ec/google/chromeec/ec_acpi.c @@ -7,6 +7,7 @@ #include <acpi/acpigen_usb.h> #include <console/console.h> #include <drivers/usb/acpi/chip.h> +#include <ec/google/common/dptf.h>
#include "chip.h" #include "ec.h" @@ -241,11 +242,32 @@ !!(keybd.capabilities & KEYBD_CAP_SCRNLOCK_KEY)); }
+static const char *ec_acpi_name(const struct device *dev) +{ + return "EC0"; +} + +static struct device_operations ec_ops = { + .acpi_name = ec_acpi_name, +}; + void google_chromeec_fill_ssdt_generator(const struct device *dev) { + struct device_path path; + struct device *ec; + if (!dev->enabled) return;
+ /* Set up a minimal EC0 device to pass to the DPTF helpers */ + path.type = DEVICE_PATH_GENERIC; + path.generic.id = 0; + ec = alloc_find_dev(dev->bus, &path); + ec->ops = &ec_ops; + + if (CONFIG(DRIVERS_INTEL_DPTF)) + ec_fill_dptf_helpers(ec); + fill_ssdt_typec_device(dev); fill_ssdt_ps2_keyboard(dev); } diff --git a/src/ec/google/chromeec/ec_dptf_helpers.c b/src/ec/google/chromeec/ec_dptf_helpers.c new file mode 100644 index 0000000..74049ac --- /dev/null +++ b/src/ec/google/chromeec/ec_dptf_helpers.c @@ -0,0 +1,292 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <acpi/acpigen.h> +#include <acpi/acpigen_dptf.h> +#include <ec/google/common/dptf.h> + +/* + * The Chrome EC is typically in charge of many system functions, including battery charging and + * fan PWM control. This places it in the middle of a DPTF implementation and therefore, many of + * the "helper" ACPI Methods themselves call EC Methods. Because of that, the responsibility for + * producing the corresponding AML lies here. + */ + +/* DPTF Event types */ +enum { + TRIP_POINTS_CHANGED_EVENT = 0x81, + THERMAL_EVENT = 0x90, +}; + +/* EC constants */ +enum { + EC_FAN_DUTY_AUTO = 0xFF, +}; + +static void write_charger_PPPC(const struct device *ec) +{ + acpigen_write_method_serialized("PPPC", 0); + + /* + * Convert size of PPSS table to index + * + * Store (SizeOf (PPSS), Local0) + * Decrement (Local0) + */ + acpigen_write_store(); + acpigen_emit_byte(SIZEOF_OP); + acpigen_emit_namestring("PPSS"); + acpigen_emit_byte(LOCAL0_OP); + acpigen_emit_byte(DECREMENT_OP); + acpigen_emit_byte(LOCAL0_OP); + + /* + * Check if charging is disabled (AC removed) + * + * If (_SB.PCI0.LPCB.EC0.ACEX () = Zero) { + * Return (Local0) + * } + */ + acpigen_write_if(); + acpigen_emit_byte(LEQUAL_OP); + acpigen_emit_namestring(acpi_device_path_join(ec, "ACEX")); + acpigen_emit_byte(ZERO_OP); + acpigen_write_return_op(LOCAL0_OP); + acpigen_pop_len(); /* If */ + + /* Return highest power state (index 0) */ + acpigen_write_return_op(ZERO_OP); + + acpigen_pop_len(); /* Method */ +} + +static void write_charger_SPPC(const struct device *ec) +{ + /* + * SPPC - Set charger current limit + * Method(SPPC, 1) { + * Store (DeRefOf (Index (DeRefOf (Index + * (PPSS, ToInteger (Arg0))), 4)), Local0) + * _SB.PCI0.LPCB.EC0.CHGS (Local0) + * } + */ + + acpigen_write_method_serialized("SPPC", 1); + + /* Retrieve Control (index 4) for specified PPSS level */ + acpigen_emit_byte(STORE_OP); + acpigen_emit_byte(DEREF_OP); + acpigen_emit_byte(INDEX_OP); + acpigen_emit_byte(DEREF_OP); + acpigen_emit_byte(INDEX_OP); + acpigen_emit_namestring("PPSS"); + acpigen_write_to_integer(ARG0_OP, ZERO_OP); + acpigen_emit_byte(ZERO_OP); /* 3rd arg to Index */ + acpigen_write_integer(4); /* Index */ + acpigen_emit_byte(ZERO_OP); /* 3rd arg to Index */ + acpigen_emit_byte(LOCAL0_OP); + + /* Pass Control value to EC to limit charging */ + acpigen_emit_namestring(acpi_device_path_join(ec, "CHGS")); + acpigen_emit_byte(LOCAL0_OP); + acpigen_pop_len(); /* Method */ +} + +static void write_fan_fst(const struct device *ec) +{ + /* TFST is a package that is used to store data from FAND */ + acpigen_write_name("TFST"); + acpigen_write_package(3); + acpigen_write_integer(0); /* Revision */ + acpigen_write_integer(0); /* Control */ + acpigen_write_integer(0); /* Speed */ + acpigen_pop_len(); /* Package */ + + /* _FST */ + acpigen_write_method_serialized("_FST", 0); + acpigen_write_store(); + acpigen_emit_namestring(acpi_device_path_join(ec, "FAND")); + acpigen_emit_byte(INDEX_OP); + acpigen_emit_namestring("TFST"); + acpigen_write_integer(1); + acpigen_emit_byte(ZERO_OP); /* 3rd arg to Index */ + acpigen_emit_byte(RETURN_OP); + acpigen_emit_namestring("TFST"); + acpigen_pop_len(); /* Method _FST */ +} + +static void write_fan_fsl(const struct device *ec) +{ + /* _FSL */ + acpigen_write_method_serialized("_FSL", 1); + acpigen_write_store(); + acpigen_emit_byte(ARG0_OP); + acpigen_emit_namestring(acpi_device_path_join(ec, "FAND")); + acpigen_pop_len(); /* Method _FSL */ +} + +/* Note: requires manual insertion of acpigen_pop_len() for the If */ +static void write_is_policy_enabled(unsigned int index, bool enabled) +{ + /* + * If (And (LEqual (Deref (Index (IDSP, index)), Arg0)) + * (LEqual (Arg1, enabled))) + */ + acpigen_write_if(); + acpigen_emit_byte(LAND_OP); + acpigen_emit_byte(LEQUAL_OP); + acpigen_emit_byte(DEREF_OP); + acpigen_emit_byte(INDEX_OP); + acpigen_emit_namestring("IDSP"); + acpigen_write_integer(index); + acpigen_emit_byte(ZERO_OP); /* 3rd arg of index - unused */ + acpigen_emit_byte(ARG0_OP); /* end lequal */ + acpigen_emit_byte(LEQUAL_OP); + acpigen_emit_byte(ARG1_OP); + acpigen_write_integer(enabled ? 1 : 0); +} + +static void write_dptf_OSC(const struct device *ec) +{ + char name[16]; + int i; + + /* + * Arg0: Buffer containing UUID + * Arg1: "Integer containing Revision ID of buffer format", but Linux passes whether + * it is enabling (1) or disabling (0) the policy in Arg1. + * Arg2: Integer containing count of entries in Arg3 + * Arg3: Buffer containing list of DWORD capabilities + * Return: Buffer containing list of DWORD capabilities + */ + acpigen_write_method_serialized("_OSC", 4); + + /* + * If the Passive Policy is enabled: + * 1) Disable temperature sensor trip points in the EC (replaces TINI) + * 2) Disable the charge limit in the EC (replaces TCHG.INIT) + */ + write_is_policy_enabled(0, true); + for (i = 0; i < DPTF_MAX_TSR; ++i) { + snprintf(name, sizeof(name), "^TSR%1d.PATD", i); + acpigen_emit_namestring(name); + } + + acpigen_emit_namestring(acpi_device_path_join(ec, "CHGD")); + acpigen_pop_len(); /* If */ + + /* If the Active Policy is disabled, disable DPTF fan control in the EC */ + write_is_policy_enabled(2, false); + acpigen_write_store(); + acpigen_write_integer(EC_FAN_DUTY_AUTO); + acpigen_emit_namestring(acpi_device_path_join(ec, "FAND")); + acpigen_pop_len(); /* If */ + + acpigen_write_return_op(ARG3_OP); + acpigen_pop_len(); /* Method _OSC */ +} + +static void write_dppm_methods(const struct device *ec) +{ + enum dptf_participant p; + char name[16]; + int i; + + acpigen_write_scope("\_SB.DPTF"); + write_dptf_OSC(ec); + + /* TEVT */ + if (CONFIG(EC_SUPPORTS_DPTF_TEVT)) { + acpigen_write_method("TEVT", 0); + + /* Local0 = ToInteger(Arg0) */ + acpigen_write_to_integer(ARG0_OP, LOCAL0_OP); + for (p = DPTF_TEMP_SENSOR_0, i = 0; p <= DPTF_TEMP_SENSOR_3; ++p, ++i) { + snprintf(name, sizeof(name), "^TSR%1d", i); + acpigen_write_if_lequal_op_int(LOCAL0_OP, i); + acpigen_notify(name, THERMAL_EVENT); + acpigen_pop_len(); /* If */ + } + + acpigen_pop_len(); /* Method */ + } + + /* TPET */ + acpigen_write_method("TPET", 0); + for (p = DPTF_TEMP_SENSOR_0, i = 0; p <= DPTF_TEMP_SENSOR_3; ++p, ++i) { + snprintf(name, sizeof(name), "^TSR%1d", i); + acpigen_notify(name, TRIP_POINTS_CHANGED_EVENT); + } + + acpigen_pop_len(); /* Method */ + acpigen_pop_len(); /* Scope */ +} + +static void write_charger_methods(const struct device *ec) +{ + dptf_write_scope(DPTF_CHARGER); + write_charger_PPPC(ec); + write_charger_SPPC(ec); + acpigen_pop_len(); /* Scope */ +} + +static void write_fan_methods(const struct device *ec) +{ + dptf_write_scope(DPTF_FAN); + write_fan_fsl(ec); + write_fan_fst(ec); + acpigen_pop_len(); /* Scope */ +} + +static void write_thermal_methods(const struct device *ec, enum dptf_participant participant, + int tsr_index) +{ + dptf_write_scope(participant); + + /* GTSH - Amount of hysteresis inherent in temperature reading */ + acpigen_write_name_integer("GTSH", 2); + + /* _TMP - read temperature from EC */ + acpigen_write_method_serialized("_TMP", 0); + acpigen_emit_byte(RETURN_OP); + acpigen_emit_namestring(acpi_device_path_join(ec, "TSRD")); + acpigen_write_integer(tsr_index); + acpigen_pop_len(); /* Method _TMP */ + + /* PATC - Aux trip point count */ + acpigen_write_name_integer("PATC", 2); + + /* PAT0 - Set Aux trip point 0 */ + acpigen_write_method_serialized("PAT0", 1); + acpigen_emit_namestring(acpi_device_path_join(ec, "PAT0")); + acpigen_write_integer(tsr_index); + acpigen_emit_byte(ARG0_OP); + acpigen_pop_len(); /* Method PAT0 */ + + /* PAT1 - Set Aux trip point 1 */ + acpigen_write_method_serialized("PAT1", 1); + acpigen_emit_namestring(acpi_device_path_join(ec, "PAT1")); + acpigen_write_integer(tsr_index); + acpigen_emit_byte(ARG0_OP); + acpigen_pop_len(); /* Method PAT0 */ + + /* PATD - Disable Aux trip point */ + acpigen_write_method_serialized("PATD", 0); + acpigen_emit_namestring(acpi_device_path_join(ec, "PATD")); + acpigen_write_integer(tsr_index); + acpigen_pop_len(); /* Method PAT0 */ + + acpigen_pop_len(); /* Scope */ +} + +void ec_fill_dptf_helpers(const struct device *ec) +{ + enum dptf_participant p; + int i; + + write_dppm_methods(ec); + write_charger_methods(ec); + write_fan_methods(ec); + + for (p = DPTF_TEMP_SENSOR_0, i = 0; p <= DPTF_TEMP_SENSOR_3; ++p, ++i) + write_thermal_methods(ec, p, i); +} diff --git a/src/ec/google/common/dptf.h b/src/ec/google/common/dptf.h new file mode 100644 index 0000000..a59ee0b --- /dev/null +++ b/src/ec/google/common/dptf.h @@ -0,0 +1,11 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef EC_GOOGLE_COMMON_DPTF_H +#define EC_GOOGLE_COMMON_DPTF_H + +#include <device/device.h> + +/* Called by google_chromeec_fill_ssdt_generator */ +void ec_fill_dptf_helpers(const struct device *dev); + +#endif /* EC_GOOGLE_COMMON_DPTF_H */
-
build bot (Jenkins) (Code Review) -
Duncan Laurie (Code Review)
-
Nick Vaccaro (Code Review)
-
Sumeet R Pawnikar (Code Review)
-
Tim Wawrzynczak (Code Review)