Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Newer versions of the Linux kernel would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device was moved from ec_lpc.c to ec.c, where this new generic code can live, and allows bus-specific code to still call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_lpc.c 3 files changed, 177 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38541/1
diff --git a/src/ec/google/chromeec/ec.c b/src/ec/google/chromeec/ec.c index 7a4f27f..5548541 100644 --- a/src/ec/google/chromeec/ec.c +++ b/src/ec/google/chromeec/ec.c @@ -38,6 +38,9 @@
#define INVALID_HCMD 0xFF
+#define GOOGLE_CHROMEEC_USBC_DEVICE_HID "GOOG0014" +#define GOOGLE_CHROMEEC_USBC_DEVICE_NAME "USBC" + /* * Map UHEPI masks to non UHEPI commands in order to support old EC FW * which does not support UHEPI command. @@ -1568,3 +1571,169 @@
return 1; } + + + +#if !DEVTREE_EARLY +static const char *google_chromeec_acpi_name(const struct device *dev) +{ + return "EC0"; +} + +static const char *power_role_to_str(enum ec_pd_power_role_caps power_role) +{ + switch (power_role) { + case EC_PD_POWER_ROLE_SOURCE: + return "source"; + case EC_PD_POWER_ROLE_SINK: + return "sink"; + case EC_PD_POWER_ROLE_DUAL: + return "dual"; + default: + return "unknown"; + } +} + +static const char *try_power_role_to_str( + enum ec_pd_try_power_role_caps try_power_role) +{ + switch (try_power_role) { + case EC_PD_TRY_POWER_ROLE_NONE: + return "none"; + case EC_PD_TRY_POWER_ROLE_SINK: + return "sink"; + case EC_PD_TRY_POWER_ROLE_SOURCE: + return"source"; + default: + return "unknown"; + } +} + +static const char *data_role_to_str(enum ec_pd_data_role_caps data_role) +{ + switch (data_role) { + case EC_PD_DATA_ROLE_DFP: + return "dfp"; + case EC_PD_DATA_ROLE_UFP: + return "ufp"; + case EC_PD_DATA_ROLE_DUAL: + return "dual"; + default: + return "unknown"; + } +} + +static const char *port_location_to_str(enum ec_pd_port_location port_location) +{ + switch (port_location) { + case EC_PD_PORT_LOCATION_LEFT: + return "LEFT"; + case EC_PD_PORT_LOCATION_RIGHT: + return "RIGHT"; + case EC_PD_PORT_LOCATION_BACK: + return "BACK"; + case EC_PD_PORT_LOCATION_FRONT: + return "FRONT"; + case EC_PD_PORT_LOCATION_LEFT_FRONT: + return "LEFT_FRONT"; + case EC_PD_PORT_LOCATION_LEFT_BACK: + return "LEFT_BACK"; + case EC_PD_PORT_LOCATION_RIGHT_FRONT: + return "RIGHT_FRONT"; + case EC_PD_PORT_LOCATION_RIGHT_BACK: + return "RIGHT_BACK"; + case EC_PD_PORT_LOCATION_BACK_LEFT: + return "BACK_LEFT"; + case EC_PD_PORT_LOCATION_BACK_RIGHT: + return "BACK_RIGHT"; + case EC_PD_PORT_LOCATION_UNKNOWN: /* intentional fallthrough */ + default: + return "UNKNOWN"; + } +} + +static void google_chromeec_fill_ssdt_generator(struct device *dev) +{ + /*struct ec_response_usb_pd_power_info port_info;*/ + enum ec_pd_power_role_caps power_role_cap; + enum ec_pd_try_power_role_caps try_power_role_cap; + enum ec_pd_data_role_caps data_role_cap; + enum ec_pd_port_location port_location; + struct acpi_dp *dsd = NULL; + char con_name[] = "CON0"; + int rv; + int i; + int num_ports = 0; + const char *scope = acpi_device_path(dev); + + acpigen_write_scope(scope); + acpigen_write_device(GOOGLE_CHROMEEC_USBC_DEVICE_NAME); + acpigen_write_name_string("_HID", GOOGLE_CHROMEEC_USBC_DEVICE_HID); + acpigen_write_name_string("_DDN", "ChromeOS EC Embedded Controller " + "USB Type-C Control"); + + rv = google_chromeec_get_num_pd_ports(&num_ports); + if (rv) + return; + + for (i = 0; i < num_ports; ++i) { + rv = google_chromeec_get_pd_port_caps(i, + &power_role_cap, + &try_power_role_cap, + &data_role_cap, + &port_location); + if (rv) + continue; + + con_name[3] = (char)i + '0'; + acpigen_write_device(con_name); + acpigen_write_name_integer("_ADR", (uint64_t)i); + + /* _DSD */ + dsd = acpi_dp_new_table("_DSD"); + acpi_dp_add_integer(dsd, "port-number", (uint64_t)i); + acpi_dp_add_string(dsd, "power-role", + power_role_to_str(power_role_cap)); + + if (try_power_role_cap != EC_PD_TRY_POWER_ROLE_NONE) + acpi_dp_add_string(dsd, "try-power-role", + try_power_role_to_str(try_power_role_cap)); + + acpi_dp_add_string(dsd, "data-role", + data_role_to_str(data_role_cap)); + acpi_dp_add_string(dsd, "port-location", + port_location_to_str(port_location)); + + acpi_dp_write(dsd); + acpigen_pop_len(); /* Device */ + } + + acpigen_pop_len(); /* Device */ + acpigen_pop_len(); /* Scope */ +} + +__weak void google_ec_enable_extra(struct device *dev) +{ +} + +static struct device_operations ec_chromeec_ops = { +#if CONFIG(HAVE_ACPI_TABLES) + .acpi_name = google_chromeec_acpi_name, + .acpi_fill_ssdt_generator = google_chromeec_fill_ssdt_generator, +#endif /* #if CONFIG(HAVE_ACPI_TABLES) */ +}; +#endif /* #if !DEVTREE_EARLY */ + +static void google_chromeec_enable(struct device *dev) +{ +#if !DEVTREE_EARLY + dev->ops = &ec_chromeec_ops; +#endif + + google_ec_enable_extra(dev); +} + +struct chip_operations ec_google_chromeec_ops = { + CHIP_NAME("ChromeOS EC") + .enable_dev = google_chromeec_enable +}; diff --git a/src/ec/google/chromeec/ec.h b/src/ec/google/chromeec/ec.h index 8051486..adb961a 100644 --- a/src/ec/google/chromeec/ec.h +++ b/src/ec/google/chromeec/ec.h @@ -18,6 +18,7 @@ #ifndef _EC_GOOGLE_CHROMEEC_EC_H #define _EC_GOOGLE_CHROMEEC_EC_H #include <types.h> +#include <device/device.h> #include "ec_commands.h"
/* Fill in base and size of the IO port resources used. */ @@ -323,4 +324,10 @@ enum ec_pd_try_power_role_caps *try_power_role_cap, enum ec_pd_data_role_caps *data_role_cap, enum ec_pd_port_location *port_location); + +/** + * Allows bus-specific EC code to perform actions when the device is enabled. + */ +void google_ec_enable_extra(struct device *dev); + #endif /* _EC_GOOGLE_CHROMEEC_EC_H */ diff --git a/src/ec/google/chromeec/ec_lpc.c b/src/ec/google/chromeec/ec_lpc.c index 6bc4fbd..9afb1fd 100644 --- a/src/ec/google/chromeec/ec_lpc.c +++ b/src/ec/google/chromeec/ec_lpc.c @@ -458,16 +458,11 @@ { NULL, 0, 0, 0, } };
-static void enable_dev(struct device *dev) +void google_ec_enable_extra(struct device *dev) { pnp_enable_devices(dev, &ops, ARRAY_SIZE(pnp_dev_info), pnp_dev_info); }
-struct chip_operations ec_google_chromeec_ops = { - CHIP_NAME("Google Chrome EC") - .enable_dev = enable_dev, -}; - static int google_chromeec_data_ready(u16 port) { return google_chromeec_status_check(port, EC_LPC_CMDR_DATA,
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38541
to look at the new patch set (#2).
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Newer versions of the Linux kernel would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device was moved from ec_lpc.c to ec.c, where this new generic code can live, and allows bus-specific code to still call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_lpc.c 3 files changed, 182 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38541/2
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38541
to look at the new patch set (#3).
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Newer versions of the Linux kernel would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device was moved from ec_lpc.c to ec.c, where this new generic code can live, and allows bus-specific code to still call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_lpc.c 3 files changed, 182 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38541/3
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/38541/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38541/3//COMMIT_MSG@9 PS3, Line 9: Newer versions of the Linux kernel would like to consume information Since what version exactly?
https://review.coreboot.org/c/coreboot/+/38541/3//COMMIT_MSG@14 PS3, Line 14: was is
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.h... PS3, Line 328: /** /*
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1721: Tab?
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec_l... File src/ec/google/chromeec/ec_lpc.c:
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec_l... PS3, Line 467: CHIP_NAME("Google Chrome EC") Did you change the name to ChromeOS EC?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38541/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38541/3//COMMIT_MSG@9 PS3, Line 9: Newer versions of the Linux kernel would like to consume information
Since what version exactly?
Not sure, it's in a maintainer's -next tree, I believe. I can check when it's expected to land.
https://review.coreboot.org/c/coreboot/+/38541/3//COMMIT_MSG@14 PS3, Line 14: was
is
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1721:
Tab?
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec_l... File src/ec/google/chromeec/ec_lpc.c:
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec_l... PS3, Line 467: CHIP_NAME("Google Chrome EC")
Did you change the name to ChromeOS EC?
Whoops, good catch Paul, thanks!
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1658: /*struct ec_response_usb_pd_power_info port_info;*/ left over?
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1668: scope since scope only gets used once it probably doesn't need to be a variable
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 3:
(10 comments)
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1578: DEVTREE_EARLY Just a thought: Can we add all this new code to ec_chip.c? Reasons:
1. That file can be added only to ramstage. No need for DEVTREE_EARLY guard. 2. We can slowly start moving all the static EC ASL code into ec_chip.c. All ACPI code can be kept in one place.
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1599: enum ec_pd_try_power_role_caps try_power_role) This can probably fit on the previous line?
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1607: " nit: space before "
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1631: LEFT Why are the strings for port locations upper case and the rest lower case? Not sure if it matters, but I think it would be good to be consistent?
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1663: = NULL No need to initialize this to NULL. dsd is set before it gets used.
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1667: = 0 No need to initialize to 0. It gets set before being used.
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1691: (uint64_t) Is the cast required?
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1695: (uint64_t) same here.
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1709: * CONx just to make it clear how it is different than the device on line 1712.
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1712: * GOOGLE_CHROMEEC_USBC_DEVICE_NAME
Hello Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38541
to look at the new patch set (#4).
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Upcoming versions of the Linux kernel would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device is moved from ec_lpc.c to a new file, ec_chip.c, where EC-related ACPI methods can live. It still allows other code to call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_chip.c M src/ec/google/chromeec/ec_lpc.c 5 files changed, 257 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38541/4
Hello Duncan Laurie, Shelley Chen, build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38541
to look at the new patch set (#6).
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Upcoming versions of the Linux kernel would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device is moved from ec_lpc.c to a new file, ec_chip.c, where EC-related ACPI methods can live. It still allows other code to call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_chip.c M src/ec/google/chromeec/ec_lpc.c 5 files changed, 262 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38541/6
Hello Duncan Laurie, Shelley Chen, build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38541
to look at the new patch set (#7).
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Upcoming versions of the Linux kernel would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device is moved from ec_lpc.c to a new file, ec_chip.c, where EC-related ACPI methods can live. It still allows other code to call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_chip.c M src/ec/google/chromeec/ec_lpc.c 5 files changed, 262 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38541/7
Hello Duncan Laurie, Shelley Chen, build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38541
to look at the new patch set (#8).
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Upcoming versions of the Linux kernel would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device is moved from ec_lpc.c to a new file, ec_chip.c, where EC-related ACPI methods can live. It still allows other code to call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_chip.c M src/ec/google/chromeec/ec_lpc.c 5 files changed, 262 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38541/8
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 8:
(14 comments)
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.h... PS3, Line 328: /**
/*
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1578: DEVTREE_EARLY
Just a thought: Can we add all this new code to ec_chip.c? Reasons: […]
I like that, will do.
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1599: enum ec_pd_try_power_role_caps try_power_role)
This can probably fit on the previous line?
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1607: "
nit: space before "
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1631: LEFT
Why are the strings for port locations upper case and the rest lower case? Not sure if it matters, b […]
These are straight from power_manager, and apparently, according to the bug are supposed to all be uppercase ¯_(ツ)_/¯ (see comment#9 on the bug)
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1658: /*struct ec_response_usb_pd_power_info port_info;*/
left over?
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1663: = NULL
No need to initialize this to NULL. dsd is set before it gets used.
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1667: = 0
No need to initialize to 0. It gets set before being used.
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1668: scope
since scope only gets used once it probably doesn't need to be a variable
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1691: (uint64_t)
Is the cast required?
Strictly speaking, no. Will remove.
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1695: (uint64_t)
same here.
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1709: *
CONx just to make it clear how it is different than the device on line 1712.
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1712: *
GOOGLE_CHROMEEC_USBC_DEVICE_NAME
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec_l... File src/ec/google/chromeec/ec_lpc.c:
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec_l... PS3, Line 467: CHIP_NAME("Google Chrome EC")
Whoops, good catch Paul, thanks!
Done
Hello Duncan Laurie, Shelley Chen, build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38541
to look at the new patch set (#9).
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Upcoming versions of the Linux kernel would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device is moved from ec_lpc.c to a new file, ec_chip.c, where EC-related ACPI methods can live. It still allows other code to call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_chip.c M src/ec/google/chromeec/ec_lpc.c 5 files changed, 265 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38541/9
Hello Duncan Laurie, Shelley Chen, build bot (Jenkins), Patrick Georgi, Martin Roth, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38541
to look at the new patch set (#10).
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Upcoming versions of the Linux kernel would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device is moved from ec_lpc.c to a new file, ec_chip.c, where EC-related ACPI methods can live. It still allows other code to call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_chip.c M src/ec/google/chromeec/ec_lpc.c 5 files changed, 266 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38541/10
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38541/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38541/11//COMMIT_MSG@9 PS11, Line 9: Upcoming versions of the Linux kernel Can you please add specific Linux versions?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38541/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38541/11//COMMIT_MSG@9 PS11, Line 9: Upcoming versions of the Linux kernel
Can you please add specific Linux versions?
Oops, forgot that. I'm asking right now.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 11:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38541/11/src/ec/google/chromeec/ec.... File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/38541/11/src/ec/google/chromeec/ec.... PS11, Line 342: board_get_usb_typec_port_paths Do we really need this? Just another thing that the mainboard needs to provide. We already have this information captured in the device tree. Is there a way we could use that instead of having to provide this callback in mainboard?
https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/google/hatc...
type ==> UPC_TYPE_C_USB2_SS_SWITCH group ==> ACPI_PLD_GROUP(1, 1)
https://review.coreboot.org/c/coreboot/+/38541/11/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_chip.c:
https://review.coreboot.org/c/coreboot/+/38541/11/src/ec/google/chromeec/ec_... PS11, Line 15: SOC_INTEL_COMMON_BLOCK_XHCI We should avoid intel specific references in common ec code. It would be better to have a proper API that can be implemented by any SoC.
https://review.coreboot.org/c/coreboot/+/38541/11/src/ec/google/chromeec/ec_... PS11, Line 125: #if CONFIG(SOC_INTEL_COMMON_BLOCK_XHCI) Instead of having this block here, what do you think about having a callback into the SoC to simply get the acpi path for a USB-C device? You can leave it upto the SoC to determine how to do it.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38541/11/src/ec/google/chromeec/ec.... File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/38541/11/src/ec/google/chromeec/ec.... PS11, Line 342: board_get_usb_typec_port_paths
Do we really need this? Just another thing that the mainboard needs to provide. […]
I hear you, I was trying to minimize the amount of manual work for new boards to implement this. I'll give it another think and see if I can come up with something more automatic based on that info.
https://review.coreboot.org/c/coreboot/+/38541/11/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_chip.c:
https://review.coreboot.org/c/coreboot/+/38541/11/src/ec/google/chromeec/ec_... PS11, Line 125: #if CONFIG(SOC_INTEL_COMMON_BLOCK_XHCI)
Instead of having this block here, what do you think about having a callback into the SoC to simply […]
Ah, that sounds like a good idea. Then this can go in intel common XHCI block.
Hello Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38541
to look at the new patch set (#12).
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Upcoming versions of the Linux kernel would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device is moved from ec_lpc.c to a new file, ec_chip.c, where EC-related ACPI methods can live. It still allows other code to call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_chip.c M src/ec/google/chromeec/ec_lpc.c 5 files changed, 245 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38541/12
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38541/12/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_chip.c:
https://review.coreboot.org/c/coreboot/+/38541/12/src/ec/google/chromeec/ec_... PS12, Line 161: if (!path || !name) Too many leading tabs - consider code refactoring
Hello Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38541
to look at the new patch set (#13).
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Upcoming versions of the Linux kernel would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device is moved from ec_lpc.c to a new file, ec_chip.c, where EC-related ACPI methods can live. It still allows other code to call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_chip.c M src/ec/google/chromeec/ec_lpc.c 5 files changed, 238 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38541/13
Hello Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38541
to look at the new patch set (#14).
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Upcoming versions of the Linux kernel (5.5.1) would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device is moved from ec_lpc.c to a new file, ec_chip.c, where EC-related ACPI methods can live. It still allows other code to call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_chip.c M src/ec/google/chromeec/ec_lpc.c 5 files changed, 238 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38541/14
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 14:
(6 comments)
https://review.coreboot.org/c/coreboot/+/38541/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38541/3//COMMIT_MSG@9 PS3, Line 9: Newer versions of the Linux kernel would like to consume information
Not sure, it's in a maintainer's -next tree, I believe. I can check when it's expected to land.
Done
https://review.coreboot.org/c/coreboot/+/38541/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38541/11//COMMIT_MSG@9 PS11, Line 9: Upcoming versions of the Linux kernel
Oops, forgot that. I'm asking right now.
Done
https://review.coreboot.org/c/coreboot/+/38541/11/src/ec/google/chromeec/ec.... File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/38541/11/src/ec/google/chromeec/ec.... PS11, Line 342: board_get_usb_typec_port_paths
I hear you, I was trying to minimize the amount of manual work for new boards to implement this. […]
Done
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/38541/3/src/ec/google/chromeec/ec.c... PS3, Line 1631: LEFT
These are straight from power_manager, and apparently, according to the bug are supposed to all be u […]
Done
https://review.coreboot.org/c/coreboot/+/38541/11/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_chip.c:
https://review.coreboot.org/c/coreboot/+/38541/11/src/ec/google/chromeec/ec_... PS11, Line 15: SOC_INTEL_COMMON_BLOCK_XHCI
We should avoid intel specific references in common ec code. […]
Done
https://review.coreboot.org/c/coreboot/+/38541/11/src/ec/google/chromeec/ec_... PS11, Line 125: #if CONFIG(SOC_INTEL_COMMON_BLOCK_XHCI)
Ah, that sounds like a good idea. Then this can go in intel common XHCI block.
Done
Hello Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38541
to look at the new patch set (#15).
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Upcoming versions of the Linux kernel (5.5.1) would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device is moved from ec_lpc.c to a new file, ec_chip.c, where EC-related ACPI methods can live. It still allows other code to call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_chip.c M src/ec/google/chromeec/ec_lpc.c 5 files changed, 259 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38541/15
Hello Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38541
to look at the new patch set (#16).
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Upcoming versions of the Linux kernel (5.5.1) would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device is moved from ec_lpc.c to ec.c, It still allows other code to call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_chip.c M src/ec/google/chromeec/ec_lpc.c 5 files changed, 259 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38541/16
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 16:
(7 comments)
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_chip.c:
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 45: EC_PD_TRY_POWER_ROLE_NONE Just noting: This case makes sense for completeness. But the way it is used currently, this case can never occur.
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 131: so malloc() : * and strdup() are used for that. No problem with doing this. But this can also be handled by: static const char usb2_port[] = "usb2-port"; static const char usb3_port[] = "usb3-port";
const char *usb_port_name; If port type is usb2 set usb_port_name to usb2_port, else if it is usb3, set usb_port_name to usb3_port.
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 138: if (port->enabled && port->path.type == DEVICE_PATH_USB && Just a nit. If negative conditions are checked and loop is skipped early on, code wouldn't require too much indentation.
if (!port->enabled || port->path.type != DEVICE_PATH_USB continue;
if (port->path.usb.port_type == 2) usb_port_name = usb2_port; else if (port->path.usb.port_type == 3) usb_port_name = usb3_port; else continue;
struct drivers_usb_acpi_config *config = port->chip_info; if (config->type == UPC_TYPE_INTERNAL) // I don't think this is correct. See comment below. continue;
if (config->group.token != (port_number + 1)) continue;
...
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 150: config->type != UPC_TYPE_INTERNAL Shouldn't this check ensure that config->type is one of the following: UPC_TYPE_C_USB2_SS_SWITCH UPC_TYPE_C_USB2_ONLY UPC_TYPE_C_USB2_SS
If a port is not of type UPC_TYPE_INTERNAL, it is not necessary that it will be of type C. Example: https://chromium.git.corp.google.com/chromiumos/third_party/coreboot/+/0c52a...
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 154: 10 sizeof(usb_port)?
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 159: 10 sizeof(usb_port)?
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 185: return If you return here, then device and scope acpigen_pop_len() would be skipped. I think it would be good to move this call to google_chromeec_get_num_pd_ports() to happen before acpigen_write_scope().
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 16:
(7 comments)
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_chip.c:
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 45: EC_PD_TRY_POWER_ROLE_NONE
Just noting: This case makes sense for completeness. […]
That's true; I can add a comment indicating so.
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 131: so malloc() : * and strdup() are used for that.
No problem with doing this. But this can also be handled by: […]
That is probably cleaner, thanks for the suggestion.
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 138: if (port->enabled && port->path.type == DEVICE_PATH_USB &&
Just a nit. […]
Done
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 150: config->type != UPC_TYPE_INTERNAL
Shouldn't this check ensure that config->type is one of the following: […]
I waffled back and forth on that... but I think you're right.
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 154: 10
sizeof(usb_port)?
Not needed anymore w/ your suggestion above.
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 159: 10
sizeof(usb_port)?
Not needed anymore w/ your suggestion above.
https://review.coreboot.org/c/coreboot/+/38541/16/src/ec/google/chromeec/ec_... PS16, Line 185: return
If you return here, then device and scope acpigen_pop_len() would be skipped. […]
Ah yes, good catch.
Hello Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38541
to look at the new patch set (#17).
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Upcoming patches for the Linux kernel (5.6 ?) would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device is moved from ec_lpc.c to a new file, ec_chip.c, where EC-related ACPI methods can live. It still allows other code to call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_chip.c M src/ec/google/chromeec/ec_lpc.c 5 files changed, 278 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/38541/17
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 17: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
ec/google/chromeec: Add SSDT generator for ChromeOS EC
Upcoming patches for the Linux kernel (5.6 ?) would like to consume information about the USB PD ports that are attached to the device. This information is obtained from the CrOS EC and exposed in the SSDT ACPI table.
Also, the device enable for this PCI device is moved from ec_lpc.c to a new file, ec_chip.c, where EC-related ACPI methods can live. It still allows other code to call functions on device enable (so that PnP enable for the LPC device still gets called).
BUG=b:146506369 BRANCH=none TEST=Verify the SSDT contains the expected information
Change-Id: I729caecd64d9320fb02c0404c8315122f010970b Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/38541 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/ec/google/chromeec/Makefile.inc M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h A src/ec/google/chromeec/ec_chip.c M src/ec/google/chromeec/ec_lpc.c 5 files changed, 278 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/ec/google/chromeec/Makefile.inc b/src/ec/google/chromeec/Makefile.inc index f2e0034..4994480 100644 --- a/src/ec/google/chromeec/Makefile.inc +++ b/src/ec/google/chromeec/Makefile.inc @@ -24,6 +24,7 @@ verstage-$(CONFIG_EC_GOOGLE_CHROMEEC_I2C) += ec_i2c.c verstage-$(CONFIG_EC_GOOGLE_CHROMEEC_LPC) += ec_lpc.c verstage-$(CONFIG_EC_GOOGLE_CHROMEEC_SPI) += ec_spi.c +ramstage-$(CONFIG_HAVE_ACPI_TABLES) += ec_chip.c
ramstage-$(CONFIG_VBOOT) += vboot_storage.c smm-$(CONFIG_VBOOT) += vboot_storage.c diff --git a/src/ec/google/chromeec/ec.c b/src/ec/google/chromeec/ec.c index 1d351c5..81e68d0 100644 --- a/src/ec/google/chromeec/ec.c +++ b/src/ec/google/chromeec/ec.c @@ -29,9 +29,7 @@ #include <stdlib.h> #include <timer.h>
-#include "chip.h" #include "ec.h" -#include "ec_commands.h"
#define INVALID_HCMD 0xFF
@@ -1527,3 +1525,28 @@
return 1; } + +#if CONFIG(HAVE_ACPI_TABLES) && !DEVTREE_EARLY +static struct device_operations ec_chromeec_ops = { + .acpi_name = google_chromeec_acpi_name, + .acpi_fill_ssdt_generator = google_chromeec_fill_ssdt_generator, +}; +#endif + +/* ec_lpc, ec_spi, or ec_i2c can override this */ +__weak void google_ec_enable_extra(struct device *dev) +{ +} + +static void google_chromeec_enable(struct device *dev) +{ +#if CONFIG(HAVE_ACPI_TABLES) && !DEVTREE_EARLY + dev->ops = &ec_chromeec_ops; +#endif + google_ec_enable_extra(dev); +} + +struct chip_operations ec_google_chromeec_ops = { + CHIP_NAME("Google Chrome EC") + .enable_dev = google_chromeec_enable +}; diff --git a/src/ec/google/chromeec/ec.h b/src/ec/google/chromeec/ec.h index 5ce375e..7341636 100644 --- a/src/ec/google/chromeec/ec.h +++ b/src/ec/google/chromeec/ec.h @@ -18,6 +18,7 @@ #ifndef _EC_GOOGLE_CHROMEEC_EC_H #define _EC_GOOGLE_CHROMEEC_EC_H #include <types.h> +#include <device/device.h> #include "ec_commands.h"
/* Fill in base and size of the IO port resources used. */ @@ -329,4 +330,26 @@ int google_chromeec_get_pd_port_caps(int port, struct usb_pd_port_caps *port_caps);
+#if CONFIG(HAVE_ACPI_TABLES) +/** + * Writes USB Type-C PD related information to the SSDT + * + * @param dev EC device + */ +void google_chromeec_fill_ssdt_generator(struct device *dev); + +/** + * Returns the ACPI name for the EC device. + * + * @param dev EC device + */ +const char *google_chromeec_acpi_name(const struct device *dev); + +#endif /* HAVE_ACPI_TABLES */ + +/* + * Allows bus-specific EC code to perform actions when the device is enabled. + */ +void google_ec_enable_extra(struct device *dev); + #endif /* _EC_GOOGLE_CHROMEEC_EC_H */ diff --git a/src/ec/google/chromeec/ec_chip.c b/src/ec/google/chromeec/ec_chip.c new file mode 100644 index 0000000..db78bdb --- /dev/null +++ b/src/ec/google/chromeec/ec_chip.c @@ -0,0 +1,228 @@ +/* + * This file is part of the coreboot project. + * + * Copyright 2020 The coreboot project Authors. + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include <arch/acpi.h> +#include <arch/acpi_device.h> +#include <arch/acpigen.h> +#include <console/console.h> +#include <drivers/usb/acpi/chip.h> +#include <stdlib.h> + +#include "chip.h" +#include "ec.h" +#include "ec_commands.h" + +#define GOOGLE_CHROMEEC_USBC_DEVICE_HID "GOOG0014" +#define GOOGLE_CHROMEEC_USBC_DEVICE_NAME "USBC" + +const char *google_chromeec_acpi_name(const struct device *dev) +{ + return "EC0"; +} + +static const char *power_role_to_str(enum ec_pd_power_role_caps power_role) +{ + switch (power_role) { + case EC_PD_POWER_ROLE_SOURCE: + return "source"; + case EC_PD_POWER_ROLE_SINK: + return "sink"; + case EC_PD_POWER_ROLE_DUAL: + return "dual"; + default: + return "unknown"; + } +} + +static const char *try_power_role_to_str(enum ec_pd_try_power_role_caps try_power_role) +{ + switch (try_power_role) { + case EC_PD_TRY_POWER_ROLE_NONE: + /* + * This should never get returned; if there is no try-power role for a device, + * then the try-power-role field is not added to the DSD. Thus, this is just + * for completeness. + */ + return "none"; + case EC_PD_TRY_POWER_ROLE_SINK: + return "sink"; + case EC_PD_TRY_POWER_ROLE_SOURCE: + return "source"; + default: + return "unknown"; + } +} + +static const char *data_role_to_str(enum ec_pd_data_role_caps data_role) +{ + switch (data_role) { + case EC_PD_DATA_ROLE_DFP: + return "host"; + case EC_PD_DATA_ROLE_UFP: + return "device"; + case EC_PD_DATA_ROLE_DUAL: + return "dual"; + default: + return "unknown"; + } +} + +/* + * Apparently these are supposed to be uppercase, in contrast to the other + * lowercase fields. + */ +static const char *port_location_to_str(enum ec_pd_port_location port_location) +{ + switch (port_location) { + case EC_PD_PORT_LOCATION_LEFT: + return "LEFT"; + case EC_PD_PORT_LOCATION_RIGHT: + return "RIGHT"; + case EC_PD_PORT_LOCATION_BACK: + return "BACK"; + case EC_PD_PORT_LOCATION_FRONT: + return "FRONT"; + case EC_PD_PORT_LOCATION_LEFT_FRONT: + return "LEFT_FRONT"; + case EC_PD_PORT_LOCATION_LEFT_BACK: + return "LEFT_BACK"; + case EC_PD_PORT_LOCATION_RIGHT_FRONT: + return "RIGHT_FRONT"; + case EC_PD_PORT_LOCATION_RIGHT_BACK: + return "RIGHT_BACK"; + case EC_PD_PORT_LOCATION_BACK_LEFT: + return "BACK_LEFT"; + case EC_PD_PORT_LOCATION_BACK_RIGHT: + return "BACK_RIGHT"; + case EC_PD_PORT_LOCATION_UNKNOWN: /* intentional fallthrough */ + default: + return "UNKNOWN"; + } +} + +/* Add port capabilities as DP properties */ +static void add_port_caps(struct acpi_dp *dsd, const struct usb_pd_port_caps *port_caps) +{ + acpi_dp_add_string(dsd, "power-role", power_role_to_str(port_caps->power_role_cap)); + + if (port_caps->try_power_role_cap != EC_PD_TRY_POWER_ROLE_NONE) + acpi_dp_add_string(dsd, "try-power-role", + try_power_role_to_str(port_caps->try_power_role_cap)); + + acpi_dp_add_string(dsd, "data-role", data_role_to_str(port_caps->data_role_cap)); + acpi_dp_add_string(dsd, "port-location", port_location_to_str( + port_caps->port_location)); +} + +/* + * Helper for fill_ssdt_generator. This adds references to the USB + * port objects so that the consumer of this information can know + * whether the port supports USB2 and/or USB3. + */ +static void add_usb_port_references(struct acpi_dp *dsd, int port_number) +{ + static const char usb2_port[] = "usb2-port"; + static const char usb3_port[] = "usb3-port"; + struct device *port = NULL; + const char *path; + const char *usb_port_type; + struct drivers_usb_acpi_config *config; + + /* + * Unfortunately, the acpi_dp_* API doesn't write out the data immediately, thus we need + * different storage areas for all of the strings, so strdup() is used for that. It is + * safe to use strdup() here, because the strings are generated at build-time and are + * guaranteed to be NUL-terminated (they come from the devicetree). + */ + while ((port = dev_find_path(port, DEVICE_PATH_USB)) != NULL) { + if (!port->enabled || port->path.type != DEVICE_PATH_USB) + continue; + + /* Looking for USB 2 & 3 port devices only */ + if (port->path.usb.port_type == 2) + usb_port_type = usb2_port; + else if (port->path.usb.port_type == 3) + usb_port_type = usb3_port; + else + continue; + + config = port->chip_info; + + /* + * Look at only USB Type-C ports, making sure they match the + * port number we're looking for (the 'token' field in 'group'). + * Also note that 'port_number' is 0-based, whereas the 'token' + * field is 1-based. + */ + if ((config->type != UPC_TYPE_C_USB2_ONLY) && + (config->type != UPC_TYPE_C_USB2_SS_SWITCH) && + (config->type != UPC_TYPE_C_USB2_SS)) + continue; + + if (config->group.token != (port_number + 1)) + continue; + + path = acpi_device_path(port); + if (path) { + path = strdup(path); + if (!path) + continue; + + acpi_dp_add_reference(dsd, usb_port_type, path); + } + } +} + +static void fill_ssdt_typec_device(struct device *dev) +{ + struct usb_pd_port_caps port_caps; + char con_name[] = "CONx"; + struct acpi_dp *dsd; + int num_ports; + int rv; + int i; + + rv = google_chromeec_get_num_pd_ports(&num_ports); + if (rv) + return; + + acpigen_write_device(GOOGLE_CHROMEEC_USBC_DEVICE_NAME); + acpigen_write_name_string("_HID", GOOGLE_CHROMEEC_USBC_DEVICE_HID); + acpigen_write_name_string("_DDN", "ChromeOS EC Embedded Controller " + "USB Type-C Control"); + + for (i = 0; i < num_ports; ++i) { + rv = google_chromeec_get_pd_port_caps(i, &port_caps); + if (rv) + continue; + + con_name[3] = (char)i + '0'; + acpigen_write_device(con_name); + acpigen_write_name_integer("_ADR", i); + + /* _DSD, Device-Specific Data */ + dsd = acpi_dp_new_table("_DSD"); + + acpi_dp_add_integer(dsd, "port-number", i); + add_port_caps(dsd, &port_caps); + add_usb_port_references(dsd, i); + + acpi_dp_write(dsd); + acpigen_pop_len(); /* Device CONx */ + } + + acpigen_pop_len(); /* Device GOOGLE_CHROMEEC_USBC_DEVICE_NAME */ +} + +void google_chromeec_fill_ssdt_generator(struct device *dev) +{ + /* Reference the existing device's scope */ + acpigen_write_scope(acpi_device_path(dev)); + fill_ssdt_typec_device(dev); + acpigen_pop_len(); /* Scope */ +} diff --git a/src/ec/google/chromeec/ec_lpc.c b/src/ec/google/chromeec/ec_lpc.c index 6bc4fbd..9afb1fd 100644 --- a/src/ec/google/chromeec/ec_lpc.c +++ b/src/ec/google/chromeec/ec_lpc.c @@ -458,16 +458,11 @@ { NULL, 0, 0, 0, } };
-static void enable_dev(struct device *dev) +void google_ec_enable_extra(struct device *dev) { pnp_enable_devices(dev, &ops, ARRAY_SIZE(pnp_dev_info), pnp_dev_info); }
-struct chip_operations ec_google_chromeec_ops = { - CHIP_NAME("Google Chrome EC") - .enable_dev = enable_dev, -}; - static int google_chromeec_data_ready(u16 port) { return google_chromeec_status_check(port, EC_LPC_CMDR_DATA,
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38541/18/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_lpc.c:
https://review.coreboot.org/c/coreboot/+/38541/18/src/ec/google/chromeec/ec_... PS18, Line 463: pnp_enable_devices(dev, &ops, ARRAY_SIZE(pnp_dev_info), pnp_dev_info); I assume this may fail now, because `dev->ops` is already set?
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38541/18/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_lpc.c:
https://review.coreboot.org/c/coreboot/+/38541/18/src/ec/google/chromeec/ec_... PS18, Line 463: pnp_enable_devices(dev, &ops, ARRAY_SIZE(pnp_dev_info), pnp_dev_info);
I assume this may fail now, because `dev->ops` is already set?
can confirm, lpc_ec_init()/google_chromeec_init() is no longer called any LPC-attached boards (which seriously breaks things on my downstream fork)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38541/18/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_lpc.c:
https://review.coreboot.org/c/coreboot/+/38541/18/src/ec/google/chromeec/ec_... PS18, Line 463: pnp_enable_devices(dev, &ops, ARRAY_SIZE(pnp_dev_info), pnp_dev_info);
can confirm, lpc_ec_init()/google_chromeec_init() is no longer called any LPC-attached boards (which […]
You are right. This is going to break things badly for device operations and lpc read resources. Let me fix this. Sorry about the regression.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38541/18/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_lpc.c:
https://review.coreboot.org/c/coreboot/+/38541/18/src/ec/google/chromeec/ec_... PS18, Line 463: pnp_enable_devices(dev, &ops, ARRAY_SIZE(pnp_dev_info), pnp_dev_info);
You are right. This is going to break things badly for device operations and lpc read resources. […]
Good catch, Nico, thank you!
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38541/18/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_lpc.c:
https://review.coreboot.org/c/coreboot/+/38541/18/src/ec/google/chromeec/ec_... PS18, Line 463: pnp_enable_devices(dev, &ops, ARRAY_SIZE(pnp_dev_info), pnp_dev_info);
Good catch, Nico, thank you!
Well, Matt already figured out that something is wrong with this patch. I just had to narrow it down :)
Regarding a fix, I don't think we need the pnp_* stuff here. What this effectively does is looking up a similar dev with `fn == 0`, then setting `.ops` if it isn't already. Assuming the passed `dev` is always the correct one, we could just override specific ops:
void google_ec_enable_extra(struct device *dev) { if (!dev->ops) return;
dev->ops.init = lpc_ec_init; dev->ops.read_resources = lpc_ec_read_resources; }
Assuming we will be called with the correct dev amongst others, we could add a check for `fn == 0`.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38541 )
Change subject: ec/google/chromeec: Add SSDT generator for ChromeOS EC ......................................................................
Patch Set 18:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38541/18/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_lpc.c:
https://review.coreboot.org/c/coreboot/+/38541/18/src/ec/google/chromeec/ec_... PS18, Line 463: pnp_enable_devices(dev, &ops, ARRAY_SIZE(pnp_dev_info), pnp_dev_info);
Well, Matt already figured out that something is wrong with this patch. I just […]
CL pushed here: https://review.coreboot.org/c/coreboot/+/39321. For now, I have retained the logic of pnp_enable_devices as is.
-
build bot (Jenkins) (Code Review) -
Duncan Laurie (Code Review)
-
Furquan Shaikh (Code Review)
-
Matt DeVillier (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Tim Wawrzynczak (Code Review)