Hello Divya S Sasidharan,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/42078
to review the following change.
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM)
EC being the TCPM decides the mux configuration after negotiating with the port partner on the Type-C port. The APIS added here will give the current essential mux state information for a given port.
BUG=None BRANCH=None TEST= Tested boots from USB Type-C flash drive and Type-C to Type-A dongle on Volteer
Change-Id: If994a459288ef31b0e6da8c6cdfd0ce3a0303981 Signed-off-by: Divya Sasidharan divya.s.sasidharan@intel.com Signed-off-by: Brandon Breitenstein brandon.breitenstein@intel.com --- M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_commands.h 3 files changed, 105 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/42078/1
diff --git a/src/ec/google/chromeec/ec.c b/src/ec/google/chromeec/ec.c index a97dfb3..22e57d2 100644 --- a/src/ec/google/chromeec/ec.c +++ b/src/ec/google/chromeec/ec.c @@ -1374,7 +1374,7 @@ return ec_image_type; }
-int google_chromeec_get_num_pd_ports(int *num_ports) +int google_chromeec_get_num_pd_ports(uint8_t *num_ports) { struct ec_response_charge_port_count resp = {}; struct chromeec_command cmd = { @@ -1435,6 +1435,91 @@ return (google_chromeec_get_current_image() == EC_IMAGE_RO); }
+int google_chromeec_usb_pd_control(int port, bool *ufp, bool *dbg_acc, uint8_t *dp_mode) +{ + struct ec_params_usb_pd_control pd_control = { + .port = port, + .role = USB_PD_CTRL_ROLE_NO_CHANGE, + .mux = USB_PD_CTRL_ROLE_NO_CHANGE, + .swap = USB_PD_CTRL_SWAP_NONE, + }; + struct ec_response_usb_pd_control_v2 resp = {}; + struct chromeec_command cmd = { + .cmd_code = EC_CMD_USB_PD_CONTROL, + .cmd_version = 2, + .cmd_data_in = &pd_control, + .cmd_size_in = sizeof(pd_control), + .cmd_data_out = &resp, + .cmd_size_out = sizeof(resp), + .cmd_dev_index = 0, + }; + + if (google_chromeec_command(&cmd) < 0) + return -1; + + *ufp = (resp.cc_state == PD_CC_DFP_ATTACHED); + *dbg_acc = (resp.cc_state == PD_CC_DFP_DEBUG_ACC); + *dp_mode = resp.dp_mode; + + return 0; +} + +/** + * Return USB2 port mapping in bit 0:3 + * USB3 port mapping in bit 4:7 + */ +int google_chromeec_pd_get_port_info(int port, uint8_t *port_map) +{ + struct ec_params_locate_chip req = { + .type = EC_CHIP_TYPE_TCPC, + .index = port, + }; + struct ec_response_locate_chip resp = {}; + struct chromeec_command cmd = { + .cmd_code = EC_CMD_LOCATE_CHIP, + .cmd_version = 0, + .cmd_data_in = &req, + .cmd_size_in = sizeof(req), + .cmd_data_out = &resp, + .cmd_size_out = sizeof(resp), + }; + + if (google_chromeec_command(&cmd) < 0) + return -1; + + *port_map = resp.reserved; + return 0; +} + +/** + * Check for the current mux state in EC + * Flags representing mux state can be + * found in ec_commands.h + */ +int google_chromeec_usb_get_pd_mux_info(int port, uint8_t *flags) +{ + if (port < 0) + return -1; + struct ec_params_usb_pd_mux_info req_mux = { + .port = port, + }; + struct ec_response_usb_pd_mux_info resp_mux = {}; + struct chromeec_command cmd = { + .cmd_code = EC_CMD_USB_PD_MUX_INFO, + .cmd_version = 0, + .cmd_data_in = &req_mux, + .cmd_size_in = sizeof(req_mux), + .cmd_data_out = &resp_mux, + .cmd_size_out = sizeof(resp_mux), + .cmd_dev_index = 0, + }; + if (google_chromeec_command(&cmd) < 0) + return -1; + + *flags = resp_mux.flags; + return 0; +} + /** * Check if EC/TCPM is in an alternate mode or not. * @@ -1443,22 +1528,16 @@ */ int google_chromeec_pd_get_amode(uint16_t svid) { - struct ec_response_usb_pd_ports resp; - struct chromeec_command cmd = { - .cmd_code = EC_CMD_USB_PD_PORTS, - .cmd_version = 0, - .cmd_data_in = NULL, - .cmd_size_in = 0, - .cmd_data_out = &resp, - .cmd_size_out = sizeof(resp), - .cmd_dev_index = 0, - }; + uint8_t num_ports; + int ret; + struct chromeec_command cmd; int i;
- if (google_chromeec_command(&cmd) < 0) + ret = google_chromeec_get_num_pd_ports(&num_ports); + if (ret < 0) return -1;
- for (i = 0; i < resp.num_ports; i++) { + for (i = 0; i < num_ports; i++) { struct ec_params_usb_pd_get_mode_request params; struct ec_params_usb_pd_get_mode_response resp2; int svid_idx = 0; diff --git a/src/ec/google/chromeec/ec.h b/src/ec/google/chromeec/ec.h index aead5f7..3d95c16 100644 --- a/src/ec/google/chromeec/ec.h +++ b/src/ec/google/chromeec/ec.h @@ -27,6 +27,17 @@ enum ec_image google_chromeec_get_current_image(void); void google_chromeec_init(void); int google_chromeec_pd_get_amode(uint16_t svid); +/* Check for the current mux state in EC */ +int google_chromeec_usb_get_pd_mux_info(int port, uint8_t *flags); +/* + * USB2 and USB3 port numbers between EC and AP + * is not one to one mapping, this function will return the + * correct mapped AP port number in port_map. + */ +int google_chromeec_pd_get_port_info(int port, uint8_t *port_map); +/* Returns data role and type of device connected */ +int google_chromeec_usb_pd_control(int port, bool *ufp, bool *dbg_acc, + uint8_t *dp_mode); int google_chromeec_wait_for_displayport(long timeout);
/* Device events */ @@ -306,7 +317,7 @@ * of PD-capable USB ports according to the EC. * @return 0 on success, -1 on error */ -int google_chromeec_get_num_pd_ports(int *num_ports); +int google_chromeec_get_num_pd_ports(uint8_t *num_ports);
/* Structure representing the capabilities of a USB-PD port */ struct usb_pd_port_caps { diff --git a/src/ec/google/chromeec/ec_commands.h b/src/ec/google/chromeec/ec_commands.h index 62761a2..b4a8ee1 100644 --- a/src/ec/google/chromeec/ec_commands.h +++ b/src/ec/google/chromeec/ec_commands.h @@ -5331,10 +5331,6 @@ /* Active Link Uni-Direction */ #define USB_PD_CTRL_ACTIVE_LINK_UNIDIR BIT(3)
-/* - * Underdevelopement : - * Please remove this tag if using _v2 outside platform/ec - */ struct ec_response_usb_pd_control_v2 { uint8_t enabled; uint8_t role; @@ -5641,7 +5637,7 @@ #define USB_PD_MUX_DOCK (USB_PD_MUX_USB_ENABLED | USB_PD_MUX_DP_ENABLED)
struct ec_response_usb_pd_mux_info { - uint8_t flags; /* USB_PD_MUX_*-encoded USB mux state */ + uint8_t flags; /* USB_PD_CTRL_*-encoded USB mux state */ } __ec_align1;
#define EC_CMD_PD_CHIP_INFO 0x011B
Hello build bot (Jenkins), Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42078
to look at the new patch set (#2).
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM)
EC being the TCPM decides the mux configuration after negotiating with the port partner on the Type-C port. The APIS added here will give the current essential mux state information for a given port.
BUG=None BRANCH=None TEST= Tested boots from USB Type-C flash drive and Type-C to Type-A dongle on Volteer
Change-Id: If994a459288ef31b0e6da8c6cdfd0ce3a0303981 Signed-off-by: Divya Sasidharan divya.s.sasidharan@intel.com Signed-off-by: Brandon Breitenstein brandon.breitenstein@intel.com --- M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_acpi.c M src/ec/google/chromeec/ec_commands.h 4 files changed, 107 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/42078/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 2:
(12 comments)
https://review.coreboot.org/c/coreboot/+/42078/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42078/2//COMMIT_MSG@10 PS2, Line 10: APIS APIs
https://review.coreboot.org/c/coreboot/+/42078/2//COMMIT_MSG@17 PS2, Line 17: Type-A dongle on Volteer no need to indent here
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h... PS2, Line 30: /* Check for the current mux state in EC */ I realize the comments in here right now are terrible, but it doesn't have to be that way! Can we improve these new ones? e.g., it looks like *flags is an `out` parameter, can we document that? and also that `port` is the port number from the EC's point of view?
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h... PS2, Line 33: * USB2 and USB3 port numbers between EC and AP : * is not one to one mapping, this function will return the : * correct mapped AP port number in port_map. 80 chars wide is OK here.
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h... PS2, Line 36: */ Same thing, more documentation please. *port_map is another `out` parameter, and `port` is from EC's point of view?
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h... PS2, Line 39: int google_chromeec_usb_pd_control(int port, bool *ufp, bool *dbg_acc, What are the possible values of dp_mode?
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1446: struct ec_response_usb_pd_control_v2 resp = {}; : struct chromeec_command cmd = { : .cmd_code = EC_CMD_USB_PD_CONTROL, : .cmd_version = 2, : .cmd_data_in = &pd_control, : .cmd_size_in = sizeof(pd_control), : .cmd_data_out = &resp, : .cmd_size_out = sizeof(resp), : .cmd_dev_index = 0, : }; : Are we sure about this interface yet? The EC code looks like it's still has some TODOs, etc.
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1460: PD_CC_DFP_ATTACHED what about PD_CC_DFP_DEBUG_ACC ? That would still indicate it's UFP rather than DFP. Otherwise how do you tell the difference between PD_CC_DFP_DEBUG_ACC and PD_CC_UFP_DEBUG_ACC?
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1495: * Check for the current mux state in EC : * Flags representing mux state can be : * found in ec_commands.h 80 chars wide
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1501: if (port < 0) : return -1; this should go after the declarations
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1516: if (google_chromeec_command(&cmd) < 0) blank line after declarations
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1531: uint8_t num_ports; : int ret; : struct chromeec_command cmd; : int i; : : ret = google_chromeec_get_num_pd_ports(&num_ports); : if (ret < 0) : return -1; : : for (i = 0; i < num_ports; i++) Thanks for the refactor! But would you mind dumping this into a separate commit?
Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h... PS2, Line 30: /* Check for the current mux state in EC */
I realize the comments in here right now are terrible, but it doesn't have to be that way! Can we im […]
I will update these to the best of my ability but they are not my area of expertise. These are taken directly from the EC commands and made to make the values more easily accessible
Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1446: struct ec_response_usb_pd_control_v2 resp = {}; : struct chromeec_command cmd = { : .cmd_code = EC_CMD_USB_PD_CONTROL, : .cmd_version = 2, : .cmd_data_in = &pd_control, : .cmd_size_in = sizeof(pd_control), : .cmd_data_out = &resp, : .cmd_size_out = sizeof(resp), : .cmd_dev_index = 0, : }; :
Are we sure about this interface yet? The EC code looks like it's still has some TODOs, etc.
this is currently being used in depthcharge as well so it should be stable
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1460: PD_CC_DFP_ATTACHED
what about PD_CC_DFP_DEBUG_ACC ? That would still indicate it's UFP rather than DFP. […]
I will have to verify why it was done this way with Divya
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1495: * Check for the current mux state in EC : * Flags representing mux state can be : * found in ec_commands.h
80 chars wide
Ack
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1501: if (port < 0) : return -1;
this should go after the declarations
Ack
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1516: if (google_chromeec_command(&cmd) < 0)
blank line after declarations
Ack
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1531: uint8_t num_ports; : int ret; : struct chromeec_command cmd; : int i; : : ret = google_chromeec_get_num_pd_ports(&num_ports); : if (ret < 0) : return -1; : : for (i = 0; i < num_ports; i++)
Thanks for the refactor! But would you mind dumping this into a separate commit?
Any specific reason why? the function was added here so only logical to me that it is propagated across the file
Hello build bot (Jenkins), Tim Wawrzynczak, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42078
to look at the new patch set (#3).
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM)
EC being the TCPM decides the mux configuration after negotiating with the port partner on the Type-C port. The APIs added here will give the current essential mux state information for a given port.
BUG=None BRANCH=None TEST= Tested boots from USB Type-C flash drive and Type-C to Type-A dongle on Volteer
Change-Id: If994a459288ef31b0e6da8c6cdfd0ce3a0303981 Signed-off-by: Divya Sasidharan divya.s.sasidharan@intel.com Signed-off-by: Brandon Breitenstein brandon.breitenstein@intel.com --- M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_acpi.c M src/ec/google/chromeec/ec_commands.h 4 files changed, 113 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/42078/3
Divya Sasidharan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1460: PD_CC_DFP_ATTACHED
I will have to verify why it was done this way with Divya
ACK. This above check is fine as it is to see if DFP or UFP is attached.
Although for below change we need to include for UFP case as well PD_CC_UFP_DEBUG_ACC.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1531: uint8_t num_ports; : int ret; : struct chromeec_command cmd; : int i; : : ret = google_chromeec_get_num_pd_ports(&num_ports); : if (ret < 0) : return -1; : : for (i = 0; i < num_ports; i++)
Any specific reason why? the function was added here so only logical to me that it is propagated acr […]
google_chromeec_pd_get_amode() alreacdy existed, so changing it from sending a "raw" command to calling google_chromeec_get_num_pd_ports() is worth its own commit IMO
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1446: struct ec_response_usb_pd_control_v2 resp = {}; : struct chromeec_command cmd = { : .cmd_code = EC_CMD_USB_PD_CONTROL, : .cmd_version = 2, : .cmd_data_in = &pd_control, : .cmd_size_in = sizeof(pd_control), : .cmd_data_out = &resp, : .cmd_size_out = sizeof(resp), : .cmd_dev_index = 0, : }; :
this is currently being used in depthcharge as well so it should be stable
Ack
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1460: PD_CC_DFP_ATTACHED
ACK. This above check is fine as it is to see if DFP or UFP is attached. […]
Ok, then maybe a comment would help explain this, because I read this as `true` for `bool ufp` means that the EC is an UFP (there is something attached downstream, regardless of what type it is), but it seems to mean that it's only a non-debug device attached.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42078/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42078/5//COMMIT_MSG@9 PS5, Line 9: EC being the TCPM decides the mux configuration after : negotiating with the port partner on the Type-C port. The APIs : added here will give the current essential mux state information : for a given port. Please re-flow for 75 characters per line.
https://review.coreboot.org/c/coreboot/+/42078/5/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/42078/5/src/ec/google/chromeec/ec.h... PS5, Line 33: * usb capability, dp capability, cable type, etc Please use spaces for alignment.
https://review.coreboot.org/c/coreboot/+/42078/5/src/ec/google/chromeec/ec.h... PS5, Line 30: /* Check for the current mux state in EC : * in: int port physical port number of the type-c port : * out: uint8_t flags flags representing the status of the mux such as : * usb capability, dp capability, cable type, etc : */ https://doc.coreboot.org/coding_style.html#commenting
https://review.coreboot.org/c/coreboot/+/42078/5/src/ec/google/chromeec/ec.h... PS5, Line 325: int google_chromeec_get_num_pd_ports(uint8_t *num_ports); Why is the signature changed? Please stay with the “native” types like `unsigned int` or `size_t`.
Hello build bot (Jenkins), Tim Wawrzynczak, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42078
to look at the new patch set (#6).
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM)
EC being the TCPM decides the mux configuration after negotiating with the port partner on the Type-C port. The APIs added here will give the current essential mux state information for a given port.
BUG=None BRANCH=None TEST= Tested boots from USB Type-C flash drive and Type-C to Type-A dongle on Volteer
Change-Id: If994a459288ef31b0e6da8c6cdfd0ce3a0303981 Signed-off-by: Divya Sasidharan divya.s.sasidharan@intel.com Signed-off-by: Brandon Breitenstein brandon.breitenstein@intel.com --- M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_acpi.c M src/ec/google/chromeec/ec_commands.h 4 files changed, 113 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/42078/6
Hello build bot (Jenkins), Tim Wawrzynczak, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42078
to look at the new patch set (#7).
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM)
EC being the TCPM decides the mux configuration after negotiating with the port partner on the Type-C port. The APIs added here will give the current essential mux state information for a given port.
BUG=None BRANCH=None TEST= Tested boots from USB Type-C flash drive and Type-C to Type-A dongle on Volteer
Change-Id: If994a459288ef31b0e6da8c6cdfd0ce3a0303981 Signed-off-by: Divya Sasidharan divya.s.sasidharan@intel.com Signed-off-by: Brandon Breitenstein brandon.breitenstein@intel.com --- M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_acpi.c M src/ec/google/chromeec/ec_commands.h 4 files changed, 107 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/42078/7
Hello build bot (Jenkins), Tim Wawrzynczak, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42078
to look at the new patch set (#8).
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM)
EC being the TCPM decides the mux configuration after negotiating with the port partner on the Type-C port. The APIs added here will give the current essential mux state information for a given port.
BUG=None BRANCH=None TEST= Tested boots from USB Type-C flash drive and Type-C to Type-A dongle on Volteer
Change-Id: If994a459288ef31b0e6da8c6cdfd0ce3a0303981 Signed-off-by: Divya Sasidharan divya.s.sasidharan@intel.com Signed-off-by: Brandon Breitenstein brandon.breitenstein@intel.com --- M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_acpi.c M src/ec/google/chromeec/ec_commands.h 4 files changed, 107 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/42078/8
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 8:
(3 comments)
when similar code went into depthcharge, there was a lot of push-back on querying the EC for data that's effectively static configuration. for example, the number of type-C ports and the underlying USB port mapping is static configuration.
can we extract such information from the device tree?
https://review.coreboot.org/c/coreboot/+/42078/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42078/8//COMMIT_MSG@15 PS8, Line 15: TEST= Tested boots from USB Type-C flash drive and Type-C to Type-A : dongle on Volteer this code isn't used to boot from USB.
https://review.coreboot.org/c/coreboot/+/42078/8/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/8/src/ec/google/chromeec/ec.c... PS8, Line 1381: EC_CMD_CHARGE_PORT_COUNT EC_CMD_USB_PD_PORTS
https://review.coreboot.org/c/coreboot/+/42078/8/src/ec/google/chromeec/ec.c... PS8, Line 1490: reserved we're planning to remove this hack.
Hello build bot (Jenkins), Caveh Jalali, Duncan Laurie, Tim Wawrzynczak, Nick Vaccaro, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42078
to look at the new patch set (#9).
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM)
EC being the TCPM decides the mux configuration after negotiating with the port partner on the Type-C port. The APIs added here will give the current essential mux state information for a given port.
BUG=None BRANCH=None TEST=Built coreboot image and verified that using this patch mux is being set for display during boot
Change-Id: If994a459288ef31b0e6da8c6cdfd0ce3a0303981 Signed-off-by: Divya Sasidharan divya.s.sasidharan@intel.com Signed-off-by: Brandon Breitenstein brandon.breitenstein@intel.com --- M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_acpi.c M src/ec/google/chromeec/ec_commands.h 4 files changed, 108 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/42078/9
Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 9:
(12 comments)
https://review.coreboot.org/c/coreboot/+/42078/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42078/2//COMMIT_MSG@10 PS2, Line 10: APIS
APIs
Done
https://review.coreboot.org/c/coreboot/+/42078/2//COMMIT_MSG@10 PS2, Line 10: APIS
APIs
Done
https://review.coreboot.org/c/coreboot/+/42078/2//COMMIT_MSG@17 PS2, Line 17: Type-A dongle on Volteer
no need to indent here
Done
https://review.coreboot.org/c/coreboot/+/42078/2//COMMIT_MSG@17 PS2, Line 17: Type-A dongle on Volteer
no need to indent here
Done
https://review.coreboot.org/c/coreboot/+/42078/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42078/5//COMMIT_MSG@9 PS5, Line 9: EC being the TCPM decides the mux configuration after : negotiating with the port partner on the Type-C port. The APIs : added here will give the current essential mux state information : for a given port.
Please re-flow for 75 characters per line.
Done
https://review.coreboot.org/c/coreboot/+/42078/5/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/42078/5/src/ec/google/chromeec/ec.h... PS5, Line 30: /* Check for the current mux state in EC : * in: int port physical port number of the type-c port : * out: uint8_t flags flags representing the status of the mux such as : * usb capability, dp capability, cable type, etc : */
Done
https://review.coreboot.org/c/coreboot/+/42078/5/src/ec/google/chromeec/ec.h... PS5, Line 325: int google_chromeec_get_num_pd_ports(uint8_t *num_ports);
Why is the signature changed? Please stay with the “native” types like `unsigned int` or `size_t`.
changed this to unsigned int...there is no need for this to be an integer since there is no possible negative amount of ports
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h... PS2, Line 30: /* Check for the current mux state in EC */
I will update these to the best of my ability but they are not my area of expertise. […]
Done
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h... PS2, Line 33: * USB2 and USB3 port numbers between EC and AP : * is not one to one mapping, this function will return the : * correct mapped AP port number in port_map.
80 chars wide is OK here.
Done
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h... PS2, Line 36: */
Same thing, more documentation please. […]
Done
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.h... PS2, Line 39: int google_chromeec_usb_pd_control(int port, bool *ufp, bool *dbg_acc,
What are the possible values of dp_mode?
Done
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1531: uint8_t num_ports; : int ret; : struct chromeec_command cmd; : int i; : : ret = google_chromeec_get_num_pd_ports(&num_ports); : if (ret < 0) : return -1; : : for (i = 0; i < num_ports; i++)
google_chromeec_pd_get_amode() alreacdy existed, so changing it from sending a "raw" command to call […]
ok sure I'll remove it in the next update
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c... PS9, Line 1490: *port_map = resp.reserved; It would be nice to update ec_commands.h so we don't have to use a field named `reserved`... in the meantime, can you add a comment that this is the USB2 & USB3 port mappings?
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec_c... File src/ec/google/chromeec/ec_commands.h:
PS9: Please don't edit this file directly, we just copy it straight from the EC codebase into coreboot when necessary so that it stays in sync.
Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 9:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c... PS9, Line 1490: *port_map = resp.reserved;
It would be nice to update ec_commands.h so we don't have to use a field named `reserved`... […]
Done
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec_c... File src/ec/google/chromeec/ec_commands.h:
PS9:
Please don't edit this file directly, we just copy it straight from the EC codebase into coreboot wh […]
Done
Hello build bot (Jenkins), Caveh Jalali, Tim Wawrzynczak, Duncan Laurie, Nick Vaccaro, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42078
to look at the new patch set (#10).
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM)
EC being the TCPM decides the mux configuration after negotiating with the port partner on the Type-C port. The APIs added here will give the current essential mux state information for a given port.
BUG=None BRANCH=None TEST=Built coreboot image and verified that using this patch mux is being set for display during boot
Change-Id: If994a459288ef31b0e6da8c6cdfd0ce3a0303981 Signed-off-by: Divya Sasidharan divya.s.sasidharan@intel.com Signed-off-by: Brandon Breitenstein brandon.breitenstein@intel.com --- M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_acpi.c 3 files changed, 108 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/42078/10
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42078/11/src/ec/google/chromeec/ec.... File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/11/src/ec/google/chromeec/ec.... PS11, Line 1505: int how about making this unsigned too? then the check on line 1521 can go away
https://review.coreboot.org/c/coreboot/+/42078/11/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_acpi.c:
PS11: The change in this unrelated file is why I think a separate change, underneath this, that has your changes to google_chromeec_get_num_pd_ports(), would be preferable.
Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 12:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42078/5/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/42078/5/src/ec/google/chromeec/ec.h... PS5, Line 33: * usb capability, dp capability, cable type, etc
Please use spaces for alignment.
Done
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1531: uint8_t num_ports; : int ret; : struct chromeec_command cmd; : int i; : : ret = google_chromeec_get_num_pd_ports(&num_ports); : if (ret < 0) : return -1; : : for (i = 0; i < num_ports; i++)
ok sure I'll remove it in the next update
Done
https://review.coreboot.org/c/coreboot/+/42078/8/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/8/src/ec/google/chromeec/ec.c... PS8, Line 1381: EC_CMD_CHARGE_PORT_COUNT
EC_CMD_USB_PD_PORTS
Done
https://review.coreboot.org/c/coreboot/+/42078/8/src/ec/google/chromeec/ec.c... PS8, Line 1490: reserved
we're planning to remove this hack.
Will update this when that is fixed
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 12: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 14:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42078/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42078/8//COMMIT_MSG@15 PS8, Line 15: TEST= Tested boots from USB Type-C flash drive and Type-C to Type-A : dongle on Volteer
this code isn't used to boot from USB.
Ack
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/2/src/ec/google/chromeec/ec.c... PS2, Line 1460: PD_CC_DFP_ATTACHED
Ok, then maybe a comment would help explain this, because I read this as `true` for `bool ufp` means […]
Ack
https://review.coreboot.org/c/coreboot/+/42078/11/src/ec/google/chromeec/ec.... File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/11/src/ec/google/chromeec/ec.... PS11, Line 1505: int
how about making this unsigned too? then the check on line 1521 can go away
Ack
https://review.coreboot.org/c/coreboot/+/42078/11/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_acpi.c:
PS11:
The change in this unrelated file is why I think a separate change, underneath this, that has your c […]
Ack
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 14: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c... PS9, Line 1490: *port_map = resp.reserved;
Done
we're ready to remove the reporting of the port mapping using this field. please do not get the port mapping info from the EC.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 14: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c... PS9, Line 1490: *port_map = resp.reserved;
we're ready to remove the reporting of the port mapping […]
Great! Where do we get it from now?
Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c... PS9, Line 1490: *port_map = resp.reserved;
we're ready to remove the reporting of the port mapping […]
is there a patch set that I can check out to implement? This patch has been sitting +2 for over a month
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c... PS9, Line 1490: *port_map = resp.reserved;
is there a patch set that I can check out to implement? This patch has been sitting +2 for over a mo […]
i wanted to use this method in depthcharge but it was rejected there. see b/149883933 and discussions on related patches.
there is no patch that does this currently, this will be a new functionality. the way this was addressed in depthcharge was to put the port config into the config and use a variant specific config given that the port mapping is static topology info.
coreboot should be less problematic. i think you'll have to walk the device tree to use the same info that's passed to the kernel to plumb up USB-C. i think you'll need to get sbu_orientation from there as well to support DP.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c... PS9, Line 1490: *port_map = resp.reserved;
i wanted to use this method in depthcharge but it was rejected there. […]
Ah, I see. That's true, it can be obtained that way, but we do not have any of that info in the devicetree yet. It is being added right now, see CB:45878.
So then, the way to get at the data would be (on top of the patch mentioned above): In ec_acpi, use the EC's `struct device *dev` config (chip_info) to get mux_conn[0] for port 0 and mux_conn[1] for port 1. Those dev's config has `usb2_port_number` and `usb3_port_number` fields, which are what we need here. That should go in the mainboard code too, as it's for chromeec boards right now.
Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c... PS9, Line 1490: *port_map = resp.reserved;
Ah, I see. […]
just for clarification, the port mapping info has been in the device tree for a while CB:41414. it's the aliasing mechanism that's pending.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c... PS9, Line 1490: *port_map = resp.reserved;
just for clarification, the port mapping info has been in […]
Sorry I wasn't very clear, yes it's there, but this is what makes it palatable to get references to those devices 😄
Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c... PS9, Line 1490: *port_map = resp.reserved;
Sorry I wasn't very clear, yes it's there, but this is what makes it palatable to get references to […]
so should I just hard code it for now? since it's in mainboard there really isn't a downside to just hardcoding it
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c... PS9, Line 1490: *port_map = resp.reserved;
so should I just hard code it for now? since it's in mainboard there really isn't a downside to just […]
Either hardcode it now and I'll go back and fix it up later when the alias patchset lands, or you could rebase it on top of the current end of the set: https://review.coreboot.org/c/coreboot/+/45939
Hello build bot (Jenkins), Caveh Jalali, Tim Wawrzynczak, Duncan Laurie, Nick Vaccaro, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42078
to look at the new patch set (#15).
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM)
EC being the TCPM decides the mux configuration after negotiating with the port partner on the Type-C port. The APIs added here will give the current essential mux state information for a given port.
BUG=None BRANCH=None TEST=Built coreboot image and verified that using this patch mux is being set for display during boot
Change-Id: If994a459288ef31b0e6da8c6cdfd0ce3a0303981 Signed-off-by: Divya Sasidharan divya.s.sasidharan@intel.com Signed-off-by: Brandon Breitenstein brandon.breitenstein@intel.com --- M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_acpi.c 3 files changed, 80 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/42078/15
Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/42078/9/src/ec/google/chromeec/ec.c... PS9, Line 1490: *port_map = resp.reserved;
Either hardcode it now and I'll go back and fix it up later when the alias patchset lands, or you co […]
Hard coded in mainboard patch and removed this function from the ec code
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42078/16/src/ec/google/chromeec/ec.... File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/42078/16/src/ec/google/chromeec/ec.... PS16, Line 36: /* : * USB2 and USB3 port numbers between EC and AP is not one to one mapping : * this function will return the correctly mapped AP port number in port_map. : * in: int port physical port number of the type-c port : * out: uint8_t port_map actual port number used by the SOC for USB 3 : */ : int google_chromeec_pd_get_port_info(int port, uint8_t *port_map); This was removed
Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Tim Wawrzynczak, Duncan Laurie, Nick Vaccaro, Divya S Sasidharan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42078
to look at the new patch set (#17).
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM)
EC being the TCPM decides the mux configuration after negotiating with the port partner on the Type-C port. The APIs added here will give the current essential mux state information for a given port.
BUG=None BRANCH=None TEST=Built coreboot image and verified that using this patch mux is being set for display during boot
Change-Id: If994a459288ef31b0e6da8c6cdfd0ce3a0303981 Signed-off-by: Divya Sasidharan divya.s.sasidharan@intel.com Signed-off-by: Brandon Breitenstein brandon.breitenstein@intel.com --- M src/ec/google/chromeec/ec.c M src/ec/google/chromeec/ec.h M src/ec/google/chromeec/ec_acpi.c 3 files changed, 73 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/42078/17
Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42078/16/src/ec/google/chromeec/ec.... File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/42078/16/src/ec/google/chromeec/ec.... PS16, Line 36: /* : * USB2 and USB3 port numbers between EC and AP is not one to one mapping : * this function will return the correctly mapped AP port number in port_map. : * in: int port physical port number of the type-c port : * out: uint8_t port_map actual port number used by the SOC for USB 3 : */ : int google_chromeec_pd_get_port_info(int port, uint8_t *port_map);
This was removed
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 17: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42078 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM)
EC being the TCPM decides the mux configuration after negotiating with the port partner on the Type-C port. The APIs added here will give the current essential mux state information for a given port.
BUG=None BRANCH=None TEST=Built coreboot image and verified that using this patch mux is being set for display during boot
Change-Id: If994a459288ef31b0e6da8c6cdfd0ce3a0303981 Signed-off-by: Divya Sasidharan divya.s.sasidharan@intel.com Signed-off-by: Brandon Breitenstein brandon.breitenstein@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42078 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-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_acpi.c 3 files changed, 73 insertions(+), 4 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/ec/google/chromeec/ec.c b/src/ec/google/chromeec/ec.c index 40285dc..82de088 100644 --- a/src/ec/google/chromeec/ec.c +++ b/src/ec/google/chromeec/ec.c @@ -1380,11 +1380,11 @@ return ec_image_type; }
-int google_chromeec_get_num_pd_ports(int *num_ports) +int google_chromeec_get_num_pd_ports(unsigned int *num_ports) { struct ec_response_charge_port_count resp = {}; struct chromeec_command cmd = { - .cmd_code = EC_CMD_CHARGE_PORT_COUNT, + .cmd_code = EC_CMD_USB_PD_PORTS, .cmd_version = 0, .cmd_data_out = &resp, .cmd_size_in = 0, @@ -1441,6 +1441,65 @@ return (google_chromeec_get_current_image() == EC_IMAGE_RO); }
+int google_chromeec_usb_pd_control(int port, bool *ufp, bool *dbg_acc, uint8_t *dp_mode) +{ + struct ec_params_usb_pd_control pd_control = { + .port = port, + .role = USB_PD_CTRL_ROLE_NO_CHANGE, + .mux = USB_PD_CTRL_ROLE_NO_CHANGE, + .swap = USB_PD_CTRL_SWAP_NONE, + }; + struct ec_response_usb_pd_control_v2 resp = {}; + struct chromeec_command cmd = { + .cmd_code = EC_CMD_USB_PD_CONTROL, + .cmd_version = 2, + .cmd_data_in = &pd_control, + .cmd_size_in = sizeof(pd_control), + .cmd_data_out = &resp, + .cmd_size_out = sizeof(resp), + .cmd_dev_index = 0, + }; + + if (google_chromeec_command(&cmd) < 0) + return -1; + + *ufp = (resp.cc_state == PD_CC_DFP_ATTACHED); + *dbg_acc = (resp.cc_state == PD_CC_DFP_DEBUG_ACC); + *dp_mode = resp.dp_mode; + + return 0; +} + +/** + * Check for the current mux state in EC. Flags representing the mux state found + * in ec_commands.h + */ +int google_chromeec_usb_get_pd_mux_info(int port, uint8_t *flags) +{ + struct ec_params_usb_pd_mux_info req_mux = { + .port = port, + }; + struct ec_response_usb_pd_mux_info resp_mux = {}; + struct chromeec_command cmd = { + .cmd_code = EC_CMD_USB_PD_MUX_INFO, + .cmd_version = 0, + .cmd_data_in = &req_mux, + .cmd_size_in = sizeof(req_mux), + .cmd_data_out = &resp_mux, + .cmd_size_out = sizeof(resp_mux), + .cmd_dev_index = 0, + }; + + if (port < 0) + return -1; + + if (google_chromeec_command(&cmd) < 0) + return -1; + + *flags = resp_mux.flags; + return 0; +} + /** * Check if EC/TCPM is in an alternate mode or not. * diff --git a/src/ec/google/chromeec/ec.h b/src/ec/google/chromeec/ec.h index 9d4e588..ad3768c 100644 --- a/src/ec/google/chromeec/ec.h +++ b/src/ec/google/chromeec/ec.h @@ -27,6 +27,15 @@ enum ec_image google_chromeec_get_current_image(void); void google_chromeec_init(void); int google_chromeec_pd_get_amode(uint16_t svid); +/* Check for the current mux state in EC + * in: int port physical port number of the type-c port + * out: uint8_t flags flags representing the status of the mux such as + * usb capability, dp capability, cable type, etc + */ +int google_chromeec_usb_get_pd_mux_info(int port, uint8_t *flags); +/* Returns data role and type of device connected */ +int google_chromeec_usb_pd_control(int port, bool *ufp, bool *dbg_acc, + uint8_t *dp_mode); int google_chromeec_wait_for_displayport(long timeout);
/* Device events */ @@ -306,7 +315,7 @@ * of PD-capable USB ports according to the EC. * @return 0 on success, -1 on error */ -int google_chromeec_get_num_pd_ports(int *num_ports); +int google_chromeec_get_num_pd_ports(unsigned int *num_ports);
/* Structure representing the capabilities of a USB-PD port */ struct usb_pd_port_caps { diff --git a/src/ec/google/chromeec/ec_acpi.c b/src/ec/google/chromeec/ec_acpi.c index 8a76805..b768316 100644 --- a/src/ec/google/chromeec/ec_acpi.c +++ b/src/ec/google/chromeec/ec_acpi.c @@ -144,7 +144,8 @@ static void fill_ssdt_typec_device(const struct device *dev) { int rv; - int i, num_ports; + int i; + unsigned int num_ports; struct device *usb2_port; struct device *usb3_port; struct device *usb4_port;
-
Brandon Breitenstein (Code Review) -
Caveh Jalali (Code Review)
-
Divya Sasidharan (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Tim Wawrzynczak (Code Review)