Attention is currently required from: Tim Wawrzynczak, Nick Vaccaro, Zhuohao Lee. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/57069 )
Change subject: coreboot tables: Add tcss information to coreboot table ......................................................................
Patch Set 1:
(14 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/57069/comment/86c20acb_e0dc312d PS1, Line 7: tcss Rather than saying tcss, probably we should just call this type-c port information. tcss is very Intel-specific and I think the type-c port information structure can be applicable to more than just Intel and would be good to define it that way for reuse on other platforms as well (if required).
File payloads/libpayload/include/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/57069/comment/4d65c3bc_dd23d9dd PS1, Line 85: CB_TAG_TCSS_INFO `CB_TAG_TYPE_C_INFO`
https://review.coreboot.org/c/coreboot/+/57069/comment/6dd32069_64ddc971 PS1, Line 146: #if CONFIG(SOC_INTEL_COMMON_BLOCK_TCSS) This can be dropped. We don't really need to add a guard here.
https://review.coreboot.org/c/coreboot/+/57069/comment/3712fc29_430f9457 PS1, Line 152: struct tcss_config_info This needs to be defined in this file. Also, I think this can be organized just like other variable-length entries are organized e.g. `struct mac_address`:
``` struct type_c_port_info { ... };
struct cb_type_c_info { u32 tag; u32 size; u32 port_count; struct type_c_port_info ports[0]; }; ```
File payloads/libpayload/include/sysinfo.h:
https://review.coreboot.org/c/coreboot/+/57069/comment/1e92ec9e_7f851892 PS1, Line 152: #if CONFIG(SOC_INTEL_COMMON_BLOCK_TCSS) This can be dropped.
https://review.coreboot.org/c/coreboot/+/57069/comment/a74848ce_935da29b PS1, Line 155: sbu_orientation What are the options for the orientation?
https://review.coreboot.org/c/coreboot/+/57069/comment/ced46a64_8fed2ad5 PS1, Line 156: hsl I think hsl stands for high speed lanes? It would be good to add a comment. Or maybe just say data_orientation to indicate data lines?
https://review.coreboot.org/c/coreboot/+/57069/comment/44bf7ab0_5e4990bf PS1, Line 159: MAX_TYPE_C_PORTS This can be defined at the top of this file along with other `SYSINFO_MAX_*` macros
File payloads/libpayload/libc/coreboot.c:
https://review.coreboot.org/c/coreboot/+/57069/comment/3d484036_f8c0ce3c PS1, Line 249: #if CONFIG(SOC_INTEL_COMMON_BLOCK_TCSS) This can be dropped.
File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/57069/comment/034626bb_495a7e65 PS1, Line 6: #include <intelblocks/tcss.h> This is a common file. Intel-specific header file cannot be included here.
https://review.coreboot.org/c/coreboot/+/57069/comment/264066b8_33ce4155 PS1, Line 425: : #if CONFIG(SOC_INTEL_COMMON_BLOCK_TCSS) : /* : * USB TCSS Configuration Information : * This record contains board-specific TCSS configuration information. : * There will be one record per type-C port. : */ : struct tcss_config_info { : uint8_t usb2_port_number; : uint8_t usb3_port_number; : uint8_t sbu_orientation; : uint8_t hsl_orientation; : }; : : struct lb_tcss_info { : uint32_t tag; : uint32_t size; : uint32_t port_count; : struct tcss_config_info tcss_info[MAX_TYPE_C_PORTS]; : }; : #endif Same comments as the payload file.
File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/57069/comment/59390c32_48a6262a PS1, Line 36: #if CONFIG(SOC_INTEL_COMMON_BLOCK_TCSS) Same comment as the other file. No Intel-specific configs in common code.
https://review.coreboot.org/c/coreboot/+/57069/comment/8b69887d_e1a4cf57 PS1, Line 242: variant_get_tcss_sysinfo variant_* shouldn't really be used in common code. It is a mainboard-specific concept. Not all mainboards have variants.
https://review.coreboot.org/c/coreboot/+/57069/comment/12470485_da8d8b6b PS1, Line 248: lb_add_tcss_info So, I think this can be organized slightly differently. You can add a weak function:
``` __weak void lb_add_tcss_info(struct lb_header *header) {} ```
and let the actual function be implemented by SoC or EC code.