On Fri, Jul 19, 2024 at 03:15:12PM +0530, Ayush Singh wrote: > Register with firmware upload API to allow updating firmware on cc1352p7 > without resorting to overlay for using the userspace flasher. > > Communication with the bootloader can be moved out of gb-beagleplay > driver if required, but I am keeping it here since there are no > immediate plans to use the on-board cc1352p7 for anything other than > greybus (BeagleConnect Technology). Additionally, there do not seem to > any other devices using cc1352p7 or it's cousins as a co-processor. > > Boot and Reset GPIOs are used to enable cc1352p7 bootloader backdoor for > flashing. The delays while starting bootloader are taken from the > userspace flasher since the technical specification does not provide > sufficient information regarding it. > > Flashing is skipped in case we are trying to flash the same > image as the one that is currently present. This is determined by CRC32 > calculation of the supplied firmware and Flash data. > > We also do a CRC32 check after flashing to ensure that the firmware was > flashed properly. > > Link: https://www.ti.com/lit/ug/swcu192/swcu192.pdf Ti CC1352p7 Tecnical Specification > Link: https://openbeagle.org/beagleconnect/cc1352-flasher Userspace > Flasher > > Signed-off-by: Ayush Singh <ayush@xxxxxxxxxxxxxxx> Hi Ayush, A few minor points from my side. ... > diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c ... > +/** > + * enum cc1352_bootloader_cmd: CC1352 Bootloader Commands nit: As this is a kernel doc, it should probably include documentation of the elements of the enum: * @COMMAND_DOWNLOAD: ... ... Flagged by W=1 allmodconfig builds and ./scripts/kernel-doc -none > + */ > +enum cc1352_bootloader_cmd { > + COMMAND_DOWNLOAD = 0x21, > + COMMAND_GET_STATUS = 0x23, > + COMMAND_SEND_DATA = 0x24, > + COMMAND_RESET = 0x25, > + COMMAND_CRC32 = 0x27, > + COMMAND_BANK_ERASE = 0x2c, > +}; > + > +/** > + * enum cc1352_bootloader_status: CC1352 Bootloader COMMAND_GET_STATUS response Likewise here. > + */ > +enum cc1352_bootloader_status { > + COMMAND_RET_SUCCESS = 0x40, > + COMMAND_RET_UNKNOWN_CMD = 0x41, > + COMMAND_RET_INVALID_CMD = 0x42, > + COMMAND_RET_INVALID_ADR = 0x43, > + COMMAND_RET_FLASH_FAIL = 0x44, > +}; ... > +/** > + * csum8: Calculate 8-bit checksum on data Similarly, as this is a kernel doc it should probably include documentation of of the function parameters. > + */ > +static u8 csum8(const u8 *data, size_t size, u8 base) > +{ > + size_t i; > + u8 sum = base; > + > + for (i = 0; i < size; ++i) > + sum += data[i]; > + > + return sum; > +} ... > +/** > + * cc1352_bootloader_empty_pkt: Calculate the number of empty bytes in the current packet Likewise here. > + */ > +static size_t cc1352_bootloader_empty_pkt(const u8 *data, size_t size) > +{ > + size_t i; > + > + for (i = 0; i < size && data[i] == 0xff; ++i) > + continue; > + > + return i; > +} ... > +static int gb_fw_init(struct gb_beagleplay *bg) > +{ > + int ret; > + struct fw_upload *fwl; > + struct gpio_desc *desc; > + > + bg->fwl = NULL; > + bg->boot_gpio = NULL; > + bg->rst_gpio = NULL; > + bg->flashing_mode = false; > + bg->fwl_cmd_response = 0; > + bg->fwl_ack = 0; > + init_completion(&bg->fwl_ack_com); > + init_completion(&bg->fwl_cmd_response_com); > + > + desc = devm_gpiod_get(&bg->sd->dev, "boot", GPIOD_IN); > + if (IS_ERR(desc)) > + return PTR_ERR(fwl); fwl is not initialised here, and it is desc that is tested for error. Perhaps this should be: return PTR_ERR(desc); Flagged by Smatch and Coccinelle. > + bg->boot_gpio = desc; > + > + desc = devm_gpiod_get(&bg->sd->dev, "reset", GPIOD_IN); > + if (IS_ERR(desc)) { > + ret = PTR_ERR(desc); > + goto free_boot; > + } > + bg->rst_gpio = desc; > + > + fwl = firmware_upload_register(THIS_MODULE, &bg->sd->dev, "cc1352p7", > + &cc1352_bootloader_ops, bg); > + if (IS_ERR(fwl)) { > + ret = PTR_ERR(fwl); > + goto free_reset; > + } > + bg->fwl = fwl; > + > + return 0; > + > +free_reset: > + devm_gpiod_put(&bg->sd->dev, bg->rst_gpio); > + bg->rst_gpio = NULL; > +free_boot: > + devm_gpiod_put(&bg->sd->dev, bg->boot_gpio); > + bg->boot_gpio = NULL; > + return ret; > +} > + > +static void gb_fw_deinit(struct gb_beagleplay *bg) > +{ > + firmware_upload_unregister(bg->fwl); > +} > + > static int gb_beagleplay_probe(struct serdev_device *serdev) > { > int ret = 0; > @@ -481,6 +1077,10 @@ static int gb_beagleplay_probe(struct serdev_device *serdev) > if (ret) > goto free_serdev; > > + ret = gb_fw_init(bg); > + if (ret) > + goto free_hdlc; > + > ret = gb_greybus_init(bg); > if (ret) > goto free_hdlc; I suspect this error path will leak resources allocated by gb_fw_init(). > @@ -500,6 +1100,7 @@ static void gb_beagleplay_remove(struct serdev_device *serdev) > { > struct gb_beagleplay *bg = serdev_device_get_drvdata(serdev); > > + gb_fw_deinit(bg); > gb_greybus_deinit(bg); > gb_beagleplay_stop_svc(bg); > hdlc_deinit(bg); > > -- > 2.45.2 > >
