Tim Wawrzynczak 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 10:
(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.0-or-later */
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 31: tcss_make_alt_mode_cmd_1 could we think of more descriptive names than alt_mode_cmd_1 and _2 ?
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 😊
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; : } All 3 of these are virtually the same, looks like the only difference is the 3rd argument to pmc_make_ipc_cmd(), a refactor would be much easier to read
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 188: nit: extra blank line
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 219: nit: extra blank line
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 221: nit: extra blank line
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
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 319: nit: extra blank line
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 339: nit: extra blank line
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.
https://review.coreboot.org/c/coreboot/+/42079/10/src/soc/intel/tigerlake/ea... PS10, Line 371: //Add check for connected maybe? I think we should check, otherwise we're wasting boot time sending another host command and a PMC IPC as well.