Hi Oliver,
> > 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.
There is built-in security which allows only signed firmware from only supported
vendors to get flashed. Please see flash path in do_flash() where fw config table
is flashed first.
>
> > 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_RESPONSE register.
> >
> > Below commands are supported,
> > - ENTER_FLASHING
> > - RESET
> > - PDPORT_ENABLE
> > - JUMP_TO_BOOT
> > - FLASH_ROW_RW
> > - VALIDATE_FW
> >
> > Command 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_ENABLE command
> >
> > Signed-off-by: Ajay Gupta <ajayg@xxxxxxxxxx>
>
> [..]
>
> > +struct ccg_cmd {
> > + u16 reg;
> > + u32 data;
> > + int len;
> > + int delay; /* ms delay for cmd timeout */
>
> unsigned? A negative delay is an interesting concept.
Will fix.
>
> [..]
>
> > +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?
Make sense so will fix it.
>
> > + 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;
> > +}
>
> [..]
>
> > +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.
Sure.
>
> > +
> > + 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;
> > + }
> > +
> > + /* 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.
Ok
thanks
> nvpublic
> > + 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;
> > +}
