usb: typec: ucsi: add firmware flashing support
PATCH v4 2/2 usb: typec: ucsi: add firmware flashing support
Quoted 1 line · Ajay Gupta
From: Ajay Gupta <[email protected]>
Hi,
Quoted 10 lines
CCGx has two copies of the firmware in addition to the bootloader. If the device is running FW1, FW2 can be updated with the new version. Dual firmware mode allows the CCG device to stay in a PD contract and support USB PD and Type-C functionality while a firmware update is in progress.
First we read the currently flashed firmware version of both primary and secondary firmware and then compare it with version of firmware file to determine if flashing is required.
I am wondering about the security implications of this. You are persistently changing firmware of a device. It looks to me like there ought to be a check for capable'(CFAP_SYS_RAWIO) in the firmware update path.
Quoted 19 lines
Command framework is added to support sending commands to CCGx controller. We wait for response after sending the command and then read the response from
RAB_RESPONSEregister.Below commands are supported,
-ENTER_FLASHING
- RESET
-PDPORT_ENABLE
-JUMP_TO_BOOT
-FLASH_ROW_RW
-VALIDATE_FWCommand specific mutex lock is also added to sync between driver and user threads.
PD port number information is added which is required while sending
PD_PORT_ENABLEcommandSigned-off-by Ajay Gupta <[email protected]>
[..]
Review
Quoted 5 lines
(quoted diff) +5
-0+struct ccg_cmd { + u16 reg; + u32 data; + int len; + int delay; /* ms delay for cmd timeout */
unsigned? A negative delay is an interesting concept.
[..]
Review
Quoted 12 lines
(quoted diff) +12
-0+static int ccg_cmd_reset(struct ucsi_ccg *uc, bool extra_delay) +{ + struct ccg_cmd cmd; + u8 *p; + int ret; + + p = (u8 *)&cmd.data; + cmd.reg = CCGX_RAB_RESET_REQ; + p[0] = RESET_SIG; + p[1] = CMD_RESET_DEV; + cmd.len = 2; + cmd.delay = 2000 + (extra_delay ? 3000 : 0);
This is a maximum, right?
If a reset fails, this is very bad. So why not always allow the maximum timeout?
Quoted 17 lines
(quoted diff) +17
-0+ mutex_lock(&uc->lock); + + set_bit(RESET_PENDING, &uc->flags); + + ret = ccg_send_command(uc, &cmd); + if (ret != RESET_COMPLETE) + goto err_clear_flag; + + ret = 0; + +err_clear_flag: + clear_bit(RESET_PENDING, &uc->flags); + + mutex_unlock(&uc->lock); + + return ret; +}
[..]
Review
Quoted 13 lines
(quoted diff) +13
-0+static int +ccg_cmd_write_flash_row(struct ucsi_ccg *uc, u16 row, + const void *data, u8 fcmd) +{ + struct i2c_client *client = uc->client; + struct ccg_cmd cmd; + u8 buf[CCG4_ROW_SIZE + 2]; + u8 *p; + int ret; + + /* Copy the data into the flash read/write memory. */ + buf[0] = REG_FLASH_RW_MEM & 0xFF; + buf[1] = REG_FLASH_RW_MEM >> 8;
We have macros for converting endianness. Please use them. That is a kind of generic issue with this patch.
Quoted 9 lines
(quoted diff) +9
-0+ + memcpy(buf + 2, data, CCG4_ROW_SIZE); + + mutex_lock(&uc->lock); + + ret = i2c_master_send(client, buf, CCG4_ROW_SIZE + 2); + if (ret != CCG4_ROW_SIZE + 2) { + dev_err(uc->dev, "REG_FLASH_RW_MEM write fail %d\n", ret); + return ret < 0 ? ret : -EIO;
Review
Quoted 10 lines
(quoted diff) +10
-0+ } + + /* Use the FLASH_ROW_READ_WRITE register to trigger */ + /* writing of data to the desired flash row */ + p = (u8 *)&cmd.data; + cmd.reg = CCGX_RAB_FLASH_ROW_RW; + p[0] = FLASH_SIG; + p[1] = fcmd; + p[2] = row & 0xFF; + p[3] = row >> 8;
Again. The macros.
Quoted 17 lines
(quoted diff) +17
-0+ cmd.len = 4; + cmd.delay = 50; + if (fcmd == FLASH_FWCT_SIG_WR_CMD) + cmd.delay += 400; + if (row == 510) + cmd.delay += 220; + ret = ccg_send_command(uc, &cmd); + + mutex_unlock(&uc->lock); + + if (ret != CMD_SUCCESS) { + dev_err(uc->dev, "write flash row failed ret=%d\n", ret); + return ret; + } + + return 0; +}