Attention is currently required from: Furquan Shaikh, Tim Wawrzynczak, Paul Menzel, Sridhar Siricilla, Patrick Rudolph. Deepti Deshatty has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54733 )
Change subject: mb/google/volteer:intel/common/block: Move mainboard api to tcss common block ......................................................................
Patch Set 3:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/54733/comment/9a06cf30_84cf81f1 PS2, Line 7: mb/google/volteer:intel/common/block: move api to tcss common block.
Please remove the dot/period at the end of the commit message summary.
Ack
https://review.coreboot.org/c/coreboot/+/54733/comment/16e741f3_7e27ca77 PS2, Line 9: change id 54090
Please use the real change-id.
Yes the change id is correct.
File src/soc/intel/common/block/tcss/tcss.c:
https://review.coreboot.org/c/coreboot/+/54733/comment/0b7151b1_9aa78b5e PS2, Line 17: extern struct chip_operations drivers_intel_pmc_mux_conn_ops;
In coreboot, we're trying to avoid having to add these `extern struct chip_operations .... […]
Ack
https://review.coreboot.org/c/coreboot/+/54733/comment/ac430c48_b3e30eb1 PS2, Line 344: const struct drivers_intel_pmc_mux_conn_config *get_mux_connector_config( : const struct device *mux, size_t port) : { : const struct drivers_intel_pmc_mux_conn_config *mux_config = NULL; : const struct device *connector; : const struct device_path conn_path[] = { : { .type = DEVICE_PATH_GENERIC, .generic.id = port, .generic.subid = 0}, : }; : : connector = find_dev_nested_path(mux->link_list, conn_path, : ARRAY_SIZE(conn_path)); : : if (connector && connector->chip_ops == &drivers_intel_pmc_mux_conn_ops) : mux_config = connector->chip_info; : : return mux_config; : }
Since we want to move the chip driver-specific code to `pmc_mux/conn/conn. […]
Ack
https://review.coreboot.org/c/coreboot/+/54733/comment/8e6a87eb_4ce4a747 PS2, Line 362: const struct device *get_pmc_mux(void) : { : const struct device *pmc; : const struct device *mux; : : pmc = pcidev_path_on_root(PCH_DEVFN_PMC); : if (!pmc || !pmc->link_list) { : printk(BIOS_ERR, "%s: unable to find PMC device or its mux\n", __func__); : return NULL; : } : : mux = pmc->link_list->children; : return mux; : }
this function isn't needed, see following comment
Ack
https://review.coreboot.org/c/coreboot/+/54733/comment/bad9bb59_395b429a PS2, Line 377: { : static struct tcss_port_map port_map[MAX_TYPE_C_PORTS]; : const struct drivers_intel_pmc_mux_conn_config *mux_config; : const struct device *mux; : size_t active_ports = 0; : size_t port; : : mux = get_pmc_mux(); : : if (!mux || !mux->link_list) { : printk(BIOS_ERR, "%s: unable to find PMC mux device or its connector\n", : __func__); : return NULL; : } : : for (port = 0; port < MAX_TYPE_C_PORTS; port++) { : mux_config = get_mux_connector_config(mux, port); : : if (!mux_config) : continue; : : port_map[active_ports].usb2_port = mux_config->usb2_port_number; : port_map[active_ports].usb3_port = mux_config->usb3_port_number; : active_ports++; : } : : *num_ports = active_ports; : return port_map; : }
I am thinking this function now can look something more like: […]
yes this looks simple, after verification, the 'conn_path' looks like below, need to add mux device to this.
const struct device_path conn_path[] = { { .type = DEVICE_PATH_PCI, .pci.devfn = PCH_DEVFN_PMC }, {.type = DEVICE_PATH_GENERIC, .generic.id=0, .generic.subid=0 }, { .type = DEVICE_PATH_GENERIC, .generic.id = i }, };
Instead of above, i thought to make the 'conn_path' simple, by including the sub function get_pmc_mux(). Please comment.