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
>
>
_______________________________________________
greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx
To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx
|