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@beagleboard.org
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