Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37867 )
Change subject: src/ec/google/chromeec: Get Type-C Mux info from EC (TCPM) ......................................................................
Patch Set 17:
(21 comments)
https://review.coreboot.org/c/coreboot/+/37867/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37867/2//COMMIT_MSG@8 PS2, Line 8:
Please add more details, what the problem and the solution is.
Done
https://review.coreboot.org/c/coreboot/+/37867/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37867/5//COMMIT_MSG@7 PS5, Line 7: rc/ec/google/chromeec: Get Type-C Mux info from EC : : BUG=None : BRANCH=None : TEST= Tested boots from USB-TypeC flash drive : on TGLRVP
add more details in the commit msg
Done
https://review.coreboot.org/c/coreboot/+/37867/14//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37867/14//COMMIT_MSG@9 PS14, 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 reflow for 75 characters per line.
Done
https://review.coreboot.org/c/coreboot/+/37867/14//COMMIT_MSG@17 PS14, Line 17: USB-TypeC flash drive
Please give the model.
Done
https://review.coreboot.org/c/coreboot/+/37867/14//COMMIT_MSG@18 PS14, Line 18: on TGLRVP
Ditto.
Done
https://review.coreboot.org/c/coreboot/+/37867/14/src/ec/google/chromeec/ec.... File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/37867/14/src/ec/google/chromeec/ec.... PS14, Line 41: int
unit8_t
Done
https://review.coreboot.org/c/coreboot/+/37867/14/src/ec/google/chromeec/ec.... PS14, Line 43: google_chromeec_usb_get_pd_ports
What about google_chromeec_get_num_pd_ports(int *num_ports) on line 321?
Done
https://review.coreboot.org/c/coreboot/+/37867/14/src/ec/google/chromeec/ec.... PS14, Line 51: int
uint8_t
Done
https://review.coreboot.org/c/coreboot/+/37867/2/src/ec/google/chromeec/ec.h File src/ec/google/chromeec/ec.h:
https://review.coreboot.org/c/coreboot/+/37867/2/src/ec/google/chromeec/ec.h... PS2, Line 40: int google_chromeec_usb_get_pd_mux_info(int port); : int google_chromeec_usb_get_pd_ports(void); : uint8_t google_chromeec_pd_get_port_info(int port); : int google_chromeec_usb_pd_control(int port, bool *ufp, bool *dbg_acc);
Can you please add comments to these functions indicating what they do, what params they expect and […]
Done
https://review.coreboot.org/c/coreboot/+/37867/2/src/ec/google/chromeec/ec.c File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/37867/2/src/ec/google/chromeec/ec.c... PS2, Line 1417: #define PD_CC_DFP_ATTACHED 5 : #define PD_CC_DEBUG_ACC 6 :
Sure can add that.
Done
https://review.coreboot.org/c/coreboot/+/37867/2/src/ec/google/chromeec/ec.c... PS2, Line 1420: /** : * : */
this is not good practice, either prefer to use comment section else don't keep this at all
Done
https://review.coreboot.org/c/coreboot/+/37867/2/src/ec/google/chromeec/ec.c... PS2, Line 1434: cmd.cmd_code = EC_CMD_USB_PD_CONTROL; : cmd.cmd_version = 3; : cmd.cmd_data_in = &pd_control; : cmd.cmd_size_in = sizeof(pd_control); : cmd.cmd_data_out = &resp; : cmd.cmd_size_out = sizeof(resp); : cmd.cmd_dev_index = 0;
Its a good idea.
Done
https://review.coreboot.org/c/coreboot/+/37867/2/src/ec/google/chromeec/ec.c... PS2, Line 1521: cmd.cmd_code = EC_CMD_USB_PD_MUX_INFO;
nit: add line between 1520 and 1521
Done
https://review.coreboot.org/c/coreboot/+/37867/11/src/ec/google/chromeec/ec.... File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/37867/11/src/ec/google/chromeec/ec.... PS11, Line 1542: /** : * Check for the current mux state in EC : * Flags representing mux state : * USB_PD_MUX_USB_ENABLED BIT(0) : * USB_PD_MUX_DP_ENABLED BIT(1) : * USB_PD_MUX_POLARITY_INVERTED BIT(2) : * USB_PD_MUX_HPD_IRQ BIT(3) : * USB_PD_MUX_HPD_LVL BIT(4) : * USB_PD_MUX_SAFE_MODE BIT(5) : * : */
I will remove this and say the flags can be found in ec_commands. […]
Done
https://review.coreboot.org/c/coreboot/+/37867/12/src/ec/google/chromeec/ec.... File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/37867/12/src/ec/google/chromeec/ec.... PS12, Line 1485: int
uint8_t
Done
https://review.coreboot.org/c/coreboot/+/37867/12/src/ec/google/chromeec/ec.... PS12, Line 1518: uint8_t google_chromeec_pd_get_port_info(int port)
how about? […]
Done
https://review.coreboot.org/c/coreboot/+/37867/14/src/ec/google/chromeec/ec.... File src/ec/google/chromeec/ec.c:
https://review.coreboot.org/c/coreboot/+/37867/14/src/ec/google/chromeec/ec.... PS14, Line 1487: struct ec_params_usb_pd_control pd_control; : struct ec_response_usb_pd_control_v2 resp; : struct chromeec_command cmd; : : pd_control.port = port; : pd_control.role = USB_PD_CTRL_ROLE_NO_CHANGE; : pd_control.mux = USB_PD_CTRL_ROLE_NO_CHANGE; : pd_control.swap = USB_PD_CTRL_SWAP_NONE; : : cmd.cmd_code = EC_CMD_USB_PD_CONTROL; : cmd.cmd_version = 2; : cmd.cmd_data_in = &pd_control; : cmd.cmd_size_in = sizeof(pd_control); : cmd.cmd_data_out = &resp; : cmd.cmd_size_out = sizeof(resp); : cmd.cmd_dev_index = 0;
Please follow the style of struct initialization in this file.
Done
https://review.coreboot.org/c/coreboot/+/37867/14/src/ec/google/chromeec/ec.... PS14, Line 1520: struct ec_params_locate_chip req; : struct ec_response_locate_chip resp; : struct chromeec_command cmd; : : req.type = EC_CHIP_TYPE_TCPC; : req.index = port; : : cmd.cmd_code = EC_CMD_LOCATE_CHIP; : cmd.cmd_version = 0; : cmd.cmd_data_in = &req; : cmd.cmd_size_in = sizeof(req); : cmd.cmd_data_out = &resp; : cmd.cmd_size_out = sizeof(resp); : cmd.cmd_dev_index = 0; :
Same here.
Done
https://review.coreboot.org/c/coreboot/+/37867/14/src/ec/google/chromeec/ec.... PS14, Line 1549: struct chromeec_command cmd; : struct ec_response_usb_pd_mux_info resp_mux; : struct ec_params_usb_pd_mux_info req_mux; : : if (port < 0) : return -1; : : cmd.cmd_code = EC_CMD_USB_PD_MUX_INFO; : cmd.cmd_version = 0; : req_mux.port = port; : cmd.cmd_data_in = &req_mux; : cmd.cmd_size_in = sizeof(req_mux); : cmd.cmd_data_out = &resp_mux; : cmd.cmd_size_out = sizeof(resp_mux); : cmd.cmd_dev_index = 0;
Same here.
Done
https://review.coreboot.org/c/coreboot/+/37867/11/src/ec/google/chromeec/ec_... File src/ec/google/chromeec/ec_commands.h:
https://review.coreboot.org/c/coreboot/+/37867/11/src/ec/google/chromeec/ec_... PS11, Line 5253: uint8_t control_flags; /* USB_PD_CTRL_*flags */
The comment refers to USB_PD_CTRL* flags, but they are still called USB_PD_MUX* above. […]
Done
https://review.coreboot.org/c/coreboot/+/37867/2/src/ec/google/chromeec/ec_c... File src/ec/google/chromeec/ec_commands.h:
https://review.coreboot.org/c/coreboot/+/37867/2/src/ec/google/chromeec/ec_c... PS2, Line 4993: } __ec_align1;
True. The new EC code base has inculcated this change. […]
Done