Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42079 )
Change subject: soc/intel/tigerlake: Add code for early tcss ......................................................................
Patch Set 11:
(12 comments)
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... File src/soc/intel/tigerlake/early_tcss.c:
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 1: /* : * This file is part of the coreboot project. : * : * Copyright 2020 The coreboot project Authors. : * : * SPDX-License-Identifier: GPL-2.0-or-later : */
/* SPDX-License-Identifier: GPL-2. […]
Done
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 31: tcss_make_alt_mode_cmd_1
These literally correspond to Buffer 0 and buffer 1 I can name it as such there really isn't a more […]
Done
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 55: /* : * commenting this section out for now as probably not needed : static void *iom_reg(unsigned int iom_reg_offset) : { : const uintptr_t iombase = IOM_BASE_ADDRESS; : : return (void *)(iombase + iom_reg_offset); : } : : static const void *port_status_reg(int port) : { : uintptr_t sts_offset; : : sts_offset = IOM_PORT_STATUS_OFFSET + IOM_REG_LEN * port; : return (const void *)iom_reg(sts_offset); : } : : static bool is_port_connected(int port) : { : uint32_t sts; : : sts = read32(port_status_reg(port)); : return !!(sts & IOM_PORT_STATUS_CONNECTED); : } : */
then go ahead and delete it, if you need it later, you can find it here in gerrit 😊
Done
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 81: static int send_conn_disc_req(const struct pmc_ipc_buffer *req, : struct pmc_ipc_buffer *res) : { : : uint32_t cmd_reg; : uint32_t res_reg; : int tries = 2; : int r; : : cmd_reg = pmc_make_ipc_cmd(PMC_IPC_USBC_CMD_ID, PMC_IPC_USBC_SUBCMD_ID, : PMC_IPC_CONN_REQ_SIZE); : : do { : r = pmc_send_ipc_cmd(cmd_reg, req, res); : if (r < 0) { : printk(BIOS_ERR, "pmc_send_ipc_cmd failed\n"); : return -1; : } : res_reg = res->buf[0]; : : if (!TCSS_STATUS_HAS_FAILED(res_reg)) { : printk(BIOS_DEBUG, "pmc_send_ipc_cmd succeeded\n"); : return 0; : } : : if (TCSS_STATUS_IS_FATAL(res_reg)) { : printk(BIOS_ERR, "pmc_send_ipc_cmd status: fatal\n"); : return -1; : } : } while (--tries >= 0); : : printk(BIOS_ERR, "pmc_send_ipc_cmd failed after retries\n"); : return -1; : } : : static int send_safe_mode_req(const struct pmc_ipc_buffer *req, : struct pmc_ipc_buffer *res) : { : uint32_t cmd_reg; : uint32_t res_reg; : int tries = 2; : int r; : : cmd_reg = pmc_make_ipc_cmd(PMC_IPC_USBC_CMD_ID, PMC_IPC_USBC_SUBCMD_ID, : PMC_IPC_ALT_REQ_SIZE); : : do { : r = pmc_send_ipc_cmd(cmd_reg, req, res); : if (r < 0) { : printk(BIOS_ERR, "pmc_send_ipc_cmd failed\n"); : return -1; : } : res_reg = res->buf[0]; : : if (!TCSS_STATUS_HAS_FAILED(res_reg)) { : printk(BIOS_DEBUG, "pmc_send_ipc_cmd succeeded\n"); : return 0; : } : : if (TCSS_STATUS_IS_FATAL(res_reg)) { : printk(BIOS_ERR, "pmc_send_ipc_cmd status: fatal\n"); : return -1; : } : } while (--tries >= 0); : : printk(BIOS_ERR, "pmc_send_ipc_cmd failed after retries\n"); : return -1; : } : : static int send_dp_mode_req(const struct pmc_ipc_buffer *req, : struct pmc_ipc_buffer *res) : { : uint32_t cmd_reg; : uint32_t res_reg; : int tries = 2; : int r; : : cmd_reg = pmc_make_ipc_cmd(PMC_IPC_USBC_CMD_ID, PMC_IPC_USBC_SUBCMD_ID, : PMC_IPC_ALT_REQ_SIZE); : : do { : r = pmc_send_ipc_cmd(cmd_reg, req, res); : if (r < 0) { : printk(BIOS_ERR, "pmc_send_ipc_cmd failed\n"); : return -1; : } : res_reg = res->buf[0]; : : if (!TCSS_STATUS_HAS_FAILED(res_reg)) { : printk(BIOS_DEBUG, "pmc_send_ipc_cmd succeeded\n"); : return 0; : } : : if (TCSS_STATUS_IS_FATAL(res_reg)) { : printk(BIOS_ERR, "pmc_send_ipc_cmd status: fatal\n"); : return -1; : } : } while (--tries >= 0); : : printk(BIOS_ERR, "pmc_send_ipc_cmd failed after retries\n"); : return -1; : }
Good point I'll consolidate this
Done
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 188:
nit: extra blank line
Done
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 219:
nit: extra blank line
Done
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 221:
nit: extra blank line
Done
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 259: if (mux_data.dp_mode <= MODE_DP_PIN_F) {
suggestion: remove this if, and use `default` instead
Done
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 319:
nit: extra blank line
Done
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 339:
nit: extra blank line
Done
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 350: return;
probably should have another big error message here.
Done
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 371: //Add check for connected maybe?
Nothing before this point can set this...which is the only reason I'm not sure this check is needed. […]
Done