Attention is currently required from: Nico Huber, Nick Vaccaro. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57069 )
Change subject: coreboot tables: Add type-c port info to coreboot table ......................................................................
Patch Set 23:
(8 comments)
File payloads/libpayload/include/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/57069/comment/a44ecd75_c7d32562 PS23, Line 146: #define CB_TYPEC_ORIENTATION_FOLLOW_CC 0 : #define CB_TYPEC_ORIENTATION_NORMAL 1 : #define CB_TYPEC_ORIENTATION_REVERSE 2 Probably use an `enum type_c_orientation` and you can add a comment for sbu_orientation and data/hsl_orientation that they need to use `enum type_c_orientation`.
Also, https://github.com/torvalds/linux/blob/master/include/linux/usb/typec.h#L70 defines the following orientations:
``` enum typec_orientation { TYPEC_ORIENTATION_NONE, TYPEC_ORIENTATION_NORMAL, TYPEC_ORIENTATION_REVERSE, }; ```
Should we keep the definitions in coreboot aligned with that i.e. use TYPEC_ORIENTATION_NONE instead of TYPEC_ORIENTATION_FOLLOW_CC?
https://review.coreboot.org/c/coreboot/+/57069/comment/e8b31fc1_a7a7f17c PS23, Line 159: u32 port_count; : struct type_c_port_info ports[0]; I had posted a comment on https://review.coreboot.org/c/coreboot/+/57345/5..7/src/drivers/intel/pmc_mu.... I think creating a structure `struct type_c_info` which includes the port_count as well can allow you to simply allocate the same structure in cbmem and copy it over when filling coreboot tables.
File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/57069/comment/f43c3073_49cc5c4d PS23, Line 158: * Orientation fields should be set to either : * CB_TYPEC_ORIENTATION_FOLLOW_CC, : * CB_TYPEC_ORIENTATION_NORMAL, or : * CB_TYPEC_ORIENTATION_REVERSE Is this comment required here?
https://review.coreboot.org/c/coreboot/+/57069/comment/f5db6193_0899af89 PS23, Line 163: struct type_c_port_info type_c_port_info[SYSINFO_MAX_TYPE_C_PORTS]; As posted on the other CL, this structure is supposed to be in CBMEM already. Do we need to copy it from CBMEM to coreboot table to a structure in sysinfo? We can probably avoid both the copies by simply passing pointer to the CBMEM entry in the coreboot table instead.
i.e. ``` struct type_c_info *type_c_info; ```
(P.S. I have intentionally used `struct type_c_info` instead of `struct type_c_port_info` to encapsulate both port_count and array of ports).
With the above change, you can get rid of the `SYSINFO_MAX_TYPE_C_PORTS` as well since we no longer need to restrict this to some randomly chosen value and go with what the mainboard actually supports.
File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/57069/comment/bc1b1a64_c99ad20c PS1, Line 155: sbu_orientation
The options are currently defined in drivers/intel/pmc_mux/conn/chip.h. […]
Yeah, we shouldn't really include any SoC specific code or comments in common code. The common code should be more aligned with type-c spec rather than being tied to a particular SoC implementation.
File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/57069/comment/5b7fc0cc_d4a43e89 PS23, Line 424: : /* Maximum of 4 USB Type-C ports */ : #define LB_MAX_TYPE_C_PORTS 4 This is not really required.
https://review.coreboot.org/c/coreboot/+/57069/comment/cd3b8e16_0025a4ae PS23, Line 456: uint32_t port_count; : struct type_c_port_info type_c_info[0]; Same comment as https://review.coreboot.org/c/coreboot/+/57069/23/payloads/libpayload/includ...
File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/57069/comment/2b388d48_328bcf85 PS23, Line 295: {CBMEM_ID_VBOOT_WORKBUF, LB_TAG_VBOOT_WORKBUF}, I was thinking that we can pass pointer to the type-c info table here directly instead of copying it to coreboot table.
``` {CBMEM_ID_TYPE_C_INFO, LB_TAG_TYPE_C_INFO}, ```
-
Furquan Shaikh (Code Review)