[PATCH 0/3] Add Firmware Upload support for beagleplay cc1352

[PATCH 0/3] Add Firmware Upload support for beagleplay cc1352

[Ayush Singh]

Adds support for beagleplay cc1352 co-processor firmware upgrade using
kernel Firmware Upload API. Uses ROM based bootloader present in
cc13x2x7 and cc26x2x7 platforms for flashing over UART. 

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. 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

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
Ayush Singh (3):
      dt-bindings: net: ti,cc1352p7: Add boot-gpio
      arm64: dts: ti: k3-am625-beagleplay: Add boot-gpios to cc1352p7
      greybus: gb-beagleplay: Add firmware upload API

 .../devicetree/bindings/net/ti,cc1352p7.yaml       |   4 +
 arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts     |   3 +-
 drivers/greybus/Kconfig                            |   1 +
 drivers/greybus/gb-beagleplay.c                    | 625 ++++++++++++++++++++-
 4 files changed, 620 insertions(+), 13 deletions(-)
---
base-commit: f76698bd9a8ca01d3581236082d786e9a6b72bb7
change-id: 20240715-beagleplay_fw_upgrade-43e6cceb0d3d

Best regards,
-- 
Ayush Singh <ayush@beagleboard.org>

[PATCH 1/3] dt-bindings: net: ti,cc1352p7: Add boot-gpio

[Ayush Singh]

boot-gpio (along with reset-gpio) is used to enable bootloader backdoor
for flashing new firmware.

The pin and pin level to enabel bootloader backdoor is configed using
the following CCFG variables in cc1352p7:
- SET_CCFG_BL_CONFIG_BL_PIN_NO
- SET_CCFG_BL_CONFIG_BL_LEVEL

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
index 3dde10de4630..a3511bb59b05 100644
--- a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
+++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
@@ -29,6 +29,9 @@ properties:
   reset-gpios:
     maxItems: 1
 
+  boot-gpios:
+    maxItems: 1
+
   vdds-supply: true
 
 required:
@@ -46,6 +49,7 @@ examples:
         clocks = <&sclk_hf 0>, <&sclk_lf 25>;
         clock-names = "sclk_hf", "sclk_lf";
         reset-gpios = <&pio 35 GPIO_ACTIVE_LOW>;
+        boot-gpios = <&pio 36 GPIO_ACTIVE_LOW>;
         vdds-supply = <&vdds>;
       };
     };

-- 
2.45.2

Re: [PATCH 1/3] dt-bindings: net: ti,cc1352p7: Add boot-gpio

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1467 bytes --]

On Fri, Jul 19, 2024 at 03:15:10PM +0530, Ayush Singh wrote:
> boot-gpio (along with reset-gpio) is used to enable bootloader backdoor
> for flashing new firmware.
> 
> The pin and pin level to enabel bootloader backdoor is configed using
> the following CCFG variables in cc1352p7:
> - SET_CCFG_BL_CONFIG_BL_PIN_NO
> - SET_CCFG_BL_CONFIG_BL_LEVEL
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  Documentation/devicetree/bindings/net/ti,cc1352p7.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> index 3dde10de4630..a3511bb59b05 100644
> --- a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> @@ -29,6 +29,9 @@ properties:
>    reset-gpios:
>      maxItems: 1
>  
> +  boot-gpios:
> +    maxItems: 1

I think this needs a description that explains what this is actually
for, and "boot-gpios" is not really an accurate name for what it is used
for IMO.

> +
>    vdds-supply: true
>  
>  required:
> @@ -46,6 +49,7 @@ examples:
>          clocks = <&sclk_hf 0>, <&sclk_lf 25>;
>          clock-names = "sclk_hf", "sclk_lf";
>          reset-gpios = <&pio 35 GPIO_ACTIVE_LOW>;
> +        boot-gpios = <&pio 36 GPIO_ACTIVE_LOW>;
>          vdds-supply = <&vdds>;
>        };
>      };
> 
> -- 
> 2.45.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] dt-bindings: net: ti,cc1352p7: Add boot-gpio

[Ayush Singh]

On 7/19/24 20:25, Conor Dooley wrote:
> On Fri, Jul 19, 2024 at 03:15:10PM +0530, Ayush Singh wrote:
>> boot-gpio (along with reset-gpio) is used to enable bootloader backdoor
>> for flashing new firmware.
>>
>> The pin and pin level to enabel bootloader backdoor is configed using
>> the following CCFG variables in cc1352p7:
>> - SET_CCFG_BL_CONFIG_BL_PIN_NO
>> - SET_CCFG_BL_CONFIG_BL_LEVEL
>>
>> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
>> ---
>>   Documentation/devicetree/bindings/net/ti,cc1352p7.yaml | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
>> index 3dde10de4630..a3511bb59b05 100644
>> --- a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
>> +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
>> @@ -29,6 +29,9 @@ properties:
>>     reset-gpios:
>>       maxItems: 1
>>   
>> +  boot-gpios:
>> +    maxItems: 1
> I think this needs a description that explains what this is actually
> for, and "boot-gpios" is not really an accurate name for what it is used
> for IMO.

I was using the name `boot-gpios` since cc1352-flasher uses the name 
boot-line. Anyway, would `bsl-gpios` be better? Or for more descriptive 
names, I guess it can be `bootloader-config-gpios` or 
`bootloader-backdoor-gpios`.


Ayush Singh

Re: [PATCH 1/3] dt-bindings: net: ti,cc1352p7: Add boot-gpio

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On Mon, Jul 22, 2024 at 04:15:41PM +0530, Ayush Singh wrote:
> 
> On 7/19/24 20:25, Conor Dooley wrote:
> > On Fri, Jul 19, 2024 at 03:15:10PM +0530, Ayush Singh wrote:
> > > boot-gpio (along with reset-gpio) is used to enable bootloader backdoor
> > > for flashing new firmware.
> > > 
> > > The pin and pin level to enabel bootloader backdoor is configed using
> > > the following CCFG variables in cc1352p7:
> > > - SET_CCFG_BL_CONFIG_BL_PIN_NO
> > > - SET_CCFG_BL_CONFIG_BL_LEVEL
> > > 
> > > Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> > > ---
> > >   Documentation/devicetree/bindings/net/ti,cc1352p7.yaml | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> > > index 3dde10de4630..a3511bb59b05 100644
> > > --- a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> > > +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> > > @@ -29,6 +29,9 @@ properties:
> > >     reset-gpios:
> > >       maxItems: 1
> > > +  boot-gpios:
> > > +    maxItems: 1
> > I think this needs a description that explains what this is actually
> > for, and "boot-gpios" is not really an accurate name for what it is used
> > for IMO.
> 
> I was using the name `boot-gpios` since cc1352-flasher uses the name
> boot-line. Anyway, would `bsl-gpios` be better?

I dunno, I think that "bsl" is worse.

> Or for more descriptive
> names, I guess it can be `bootloader-config-gpios` or

> `bootloader-backdoor-gpios`.

This is the most descriptive and therefore, IMO, best.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] dt-bindings: net: ti,cc1352p7: Add boot-gpio

[Simon Horman]

On Fri, Jul 19, 2024 at 03:15:10PM +0530, Ayush Singh wrote:
> boot-gpio (along with reset-gpio) is used to enable bootloader backdoor
> for flashing new firmware.
> 
> The pin and pin level to enabel bootloader backdoor is configed using

nit: enable

     Flagged by checkpatch.pl --codespell

> the following CCFG variables in cc1352p7:
> - SET_CCFG_BL_CONFIG_BL_PIN_NO
> - SET_CCFG_BL_CONFIG_BL_LEVEL
> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  Documentation/devicetree/bindings/net/ti,cc1352p7.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> index 3dde10de4630..a3511bb59b05 100644
> --- a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> @@ -29,6 +29,9 @@ properties:
>    reset-gpios:
>      maxItems: 1
>  
> +  boot-gpios:
> +    maxItems: 1
> +
>    vdds-supply: true
>  
>  required:
> @@ -46,6 +49,7 @@ examples:
>          clocks = <&sclk_hf 0>, <&sclk_lf 25>;
>          clock-names = "sclk_hf", "sclk_lf";
>          reset-gpios = <&pio 35 GPIO_ACTIVE_LOW>;
> +        boot-gpios = <&pio 36 GPIO_ACTIVE_LOW>;
>          vdds-supply = <&vdds>;
>        };
>      };
> 
> -- 
> 2.45.2
> 
> 

Re: [PATCH 1/3] dt-bindings: net: ti,cc1352p7: Add boot-gpio

[Simon Horman]

On Sun, Jul 21, 2024 at 10:00:14AM +0100, Simon Horman wrote:
> On Fri, Jul 19, 2024 at 03:15:10PM +0530, Ayush Singh wrote:
> > boot-gpio (along with reset-gpio) is used to enable bootloader backdoor
> > for flashing new firmware.
> > 
> > The pin and pin level to enabel bootloader backdoor is configed using
> 
> nit: enable

Sorry, one more: configured

> 
>      Flagged by checkpatch.pl --codespell

...

[PATCH 2/3] arm64: dts: ti: k3-am625-beagleplay: Add boot-gpios to cc1352p7

[Ayush Singh]

Add boot-gpios which is required for enabling bootloader backdoor for
flashing firmware to cc1352p7.

Also fix the incorrect reset-gpio.

Signed-off-by: Ayush Singh <ayush@beagleboard.org>
---
 arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
index 70de288d728e..6c9d1dc26379 100644
--- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
+++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
@@ -888,7 +888,8 @@ &main_uart6 {
 
 	mcu {
 		compatible = "ti,cc1352p7";
-		reset-gpios = <&main_gpio0 72 GPIO_ACTIVE_LOW>;
+		boot-gpios = <&main_gpio0 13 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&main_gpio0 14 GPIO_ACTIVE_HIGH>;
 		vdds-supply = <&vdd_3v3>;
 	};
 };

-- 
2.45.2

[PATCH 3/3] greybus: gb-beagleplay: Add firmware upload API

[Ayush Singh]

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>
---
 drivers/greybus/Kconfig         |   1 +
 drivers/greybus/gb-beagleplay.c | 625 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 614 insertions(+), 12 deletions(-)

diff --git a/drivers/greybus/Kconfig b/drivers/greybus/Kconfig
index ab81ceceb337..d485a99959cb 100644
--- a/drivers/greybus/Kconfig
+++ b/drivers/greybus/Kconfig
@@ -21,6 +21,7 @@ config GREYBUS_BEAGLEPLAY
 	tristate "Greybus BeaglePlay driver"
 	depends on SERIAL_DEV_BUS
 	select CRC_CCITT
+	select FW_UPLOAD
 	help
 	  Select this option if you have a BeaglePlay where CC1352
 	  co-processor acts as Greybus SVC.
diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
index 33f8fad70260..aecbfb5b5eaf 100644
--- a/drivers/greybus/gb-beagleplay.c
+++ b/drivers/greybus/gb-beagleplay.c
@@ -6,21 +6,18 @@
  * Copyright (c) 2023 BeagleBoard.org Foundation
  */
 
-#include <linux/gfp.h>
+#include <asm-generic/unaligned.h>
+#include <linux/crc32.h>
+#include <linux/gpio/consumer.h>
+#include <linux/firmware.h>
 #include <linux/greybus.h>
-#include <linux/module.h>
-#include <linux/of.h>
-#include <linux/printk.h>
 #include <linux/serdev.h>
-#include <linux/tty.h>
-#include <linux/tty_driver.h>
-#include <linux/greybus/hd.h>
-#include <linux/init.h>
-#include <linux/device.h>
 #include <linux/crc-ccitt.h>
 #include <linux/circ_buf.h>
-#include <linux/types.h>
-#include <linux/workqueue.h>
+
+#define CC1352_BOOTLOADER_TIMEOUT 2000
+#define CC1352_BOOTLOADER_ACK 0xcc
+#define CC1352_BOOTLOADER_NACK 0x33
 
 #define RX_HDLC_PAYLOAD 256
 #define CRC_LEN 2
@@ -57,6 +54,17 @@
  * @rx_buffer_len: length of receive buffer filled.
  * @rx_buffer: hdlc frame receive buffer
  * @rx_in_esc: hdlc rx flag to indicate ESC frame
+ *
+ * @fwl: underlying firmware upload device
+ * @boot_gpio: cc1352p7 boot gpio
+ * @rst_gpio: cc1352p7 reset gpio
+ * @flashing_mode: flag to indicate that flashing is currently in progress
+ * @fwl_ack_com: completion to signal an Ack/Nack
+ * @fwl_ack: Ack/Nack byte received
+ * @fwl_cmd_response_com: completion to signal a bootloader command response
+ * @fwl_cmd_response: bootloader command response data
+ * @fwl_crc32: crc32 of firmware to flash
+ * @fwl_reset_addr: flag to indicate if we need to send COMMAND_DOWNLOAD again
  */
 struct gb_beagleplay {
 	struct serdev_device *sd;
@@ -72,6 +80,17 @@ struct gb_beagleplay {
 	u16 rx_buffer_len;
 	bool rx_in_esc;
 	u8 rx_buffer[MAX_RX_HDLC];
+
+	struct fw_upload *fwl;
+	struct gpio_desc *boot_gpio;
+	struct gpio_desc *rst_gpio;
+	bool flashing_mode;
+	struct completion fwl_ack_com;
+	u8 fwl_ack;
+	struct completion fwl_cmd_response_com;
+	u32 fwl_cmd_response;
+	u32 fwl_crc32;
+	bool fwl_reset_addr;
 };
 
 /**
@@ -100,6 +119,69 @@ struct hdlc_greybus_frame {
 	u8 payload[];
 } __packed;
 
+/**
+ * enum cc1352_bootloader_cmd: CC1352 Bootloader Commands
+ */
+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
+ */
+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,
+};
+
+/**
+ * struct cc1352_bootloader_packet: CC1352 Bootloader Request Packet
+ *
+ * @len: length of packet + optional request data
+ * @checksum: 8-bit checksum excluding len
+ * @cmd: bootloader command
+ */
+struct cc1352_bootloader_packet {
+	u8 len;
+	u8 checksum;
+	u8 cmd;
+} __packed;
+
+#define CC1352_BOOTLOADER_PKT_MAX_SIZE \
+	(U8_MAX - sizeof(struct cc1352_bootloader_packet))
+
+/**
+ * struct cc1352_bootloader_download_cmd_data: CC1352 Bootloader COMMAND_DOWNLOAD request data
+ *
+ * @addr: address to start programming data into
+ * @size: size of data that will be sent
+ */
+struct cc1352_bootloader_download_cmd_data {
+	__be32 addr;
+	__be32 size;
+} __packed;
+
+/**
+ * struct cc1352_bootloader_crc32_cmd_data: CC1352 Bootloader COMMAND_CRC32 request data
+ *
+ * @addr: address where crc32 calculation starts
+ * @size: number of bytes comprised by crc32 calculation
+ * @read_repeat: number of read repeats for each data location
+ */
+struct cc1352_bootloader_crc32_cmd_data {
+	__be32 addr;
+	__be32 size;
+	__be32 read_repeat;
+} __packed;
+
 static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
 {
 	struct hdlc_greybus_frame *gb_frame = (struct hdlc_greybus_frame *)buf;
@@ -331,11 +413,131 @@ static void hdlc_deinit(struct gb_beagleplay *bg)
 	flush_work(&bg->tx_work);
 }
 
+/**
+ * csum8: Calculate 8-bit checksum on data
+ */
+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;
+}
+
+static void cc1352_bootloader_send_ack(struct gb_beagleplay *bg)
+{
+	static const u8 ack[] = { 0x00, CC1352_BOOTLOADER_ACK };
+
+	serdev_device_write_buf(bg->sd, ack, sizeof(ack));
+}
+
+static void cc1352_bootloader_send_nack(struct gb_beagleplay *bg)
+{
+	static const u8 nack[] = { 0x00, CC1352_BOOTLOADER_NACK };
+
+	serdev_device_write_buf(bg->sd, nack, sizeof(nack));
+}
+
+/**
+ * cc1352_bootloader_pkt_rx: Process a CC1352 Bootloader Packet
+ *
+ * @bg: beagleplay greybus driver
+ * @data: packet buffer
+ * @count: packet buffer size
+ *
+ * @return: number of bytes processed
+ *
+ * Here are the steps to successfully receive a packet from cc1352 bootloader
+ * according to the docs:
+ * 1. Wait for nonzero data to be returned from the device. This is important
+ *    as the device may send zero bytes between a sent and a received data
+ *    packet. The first nonzero byte received is the size of the packet that is
+ *    being received.
+ * 2. Read the next byte, which is the checksum for the packet.
+ * 3. Read the data bytes from the device. During the data phase, packet size
+ *    minus 2 bytes is sent.
+ * 4. Calculate the checksum of the data bytes and verify it matches the
+ *    checksum received in the packet.
+ * 5. Send an acknowledge byte or a not-acknowledge byte to the device to
+ *    indicate the successful or unsuccessful reception of the packet.
+ */
+static int cc1352_bootloader_pkt_rx(struct gb_beagleplay *bg, const u8 *data,
+				    size_t count)
+{
+	bool is_valid = false;
+
+	switch (data[0]) {
+	/* Skip 0x00 bytes.  */
+	case 0x00:
+		return 1;
+	case CC1352_BOOTLOADER_ACK:
+	case CC1352_BOOTLOADER_NACK:
+		WRITE_ONCE(bg->fwl_ack, data[0]);
+		complete(&bg->fwl_ack_com);
+		return 1;
+	case 3:
+		if (count < 3)
+			return 0;
+		is_valid = data[1] == data[2];
+		WRITE_ONCE(bg->fwl_cmd_response, (u32)data[2]);
+		break;
+	case 6:
+		if (count < 6)
+			return 0;
+		is_valid = csum8(&data[2], sizeof(__be32), 0) == data[1];
+		WRITE_ONCE(bg->fwl_cmd_response, get_unaligned_be32(&data[2]));
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (is_valid) {
+		cc1352_bootloader_send_ack(bg);
+		complete(&bg->fwl_cmd_response_com);
+	} else {
+		dev_warn(&bg->sd->dev,
+			 "Dropping bootloader packet with invalid checksum");
+		cc1352_bootloader_send_nack(bg);
+	}
+
+	return data[0];
+}
+
+static size_t cc1352_bootloader_rx(struct gb_beagleplay *bg, const u8 *data,
+				   size_t count)
+{
+	int ret;
+	size_t off = 0;
+
+	memcpy(bg->rx_buffer + bg->rx_buffer_len, data, count);
+	bg->rx_buffer_len += count;
+
+	do {
+		ret = cc1352_bootloader_pkt_rx(bg, bg->rx_buffer + off,
+					       bg->rx_buffer_len - off);
+		if (ret < 0)
+			return dev_err_probe(&bg->sd->dev, ret,
+					     "Invalid Packet");
+		off += ret;
+	} while (ret > 0 && off < count);
+
+	bg->rx_buffer_len -= off;
+	memmove(bg->rx_buffer, bg->rx_buffer + off, bg->rx_buffer_len);
+
+	return count;
+}
+
 static size_t gb_tty_receive(struct serdev_device *sd, const u8 *data,
 			     size_t count)
 {
 	struct gb_beagleplay *bg = serdev_device_get_drvdata(sd);
 
+	if (READ_ONCE(bg->flashing_mode))
+		return cc1352_bootloader_rx(bg, data, count);
+
 	return hdlc_rx(bg, data, count);
 }
 
@@ -343,7 +545,8 @@ static void gb_tty_wakeup(struct serdev_device *serdev)
 {
 	struct gb_beagleplay *bg = serdev_device_get_drvdata(serdev);
 
-	schedule_work(&bg->tx_work);
+	if (!READ_ONCE(bg->flashing_mode))
+		schedule_work(&bg->tx_work);
 }
 
 static struct serdev_device_ops gb_beagleplay_ops = {
@@ -412,6 +615,192 @@ static void gb_beagleplay_stop_svc(struct gb_beagleplay *bg)
 	hdlc_tx_frames(bg, ADDRESS_CONTROL, 0x03, &payload, 1);
 }
 
+static int cc1352_bootloader_wait_for_ack(struct gb_beagleplay *bg)
+{
+	int ret;
+
+	ret = wait_for_completion_timeout(
+		&bg->fwl_ack_com, msecs_to_jiffies(CC1352_BOOTLOADER_TIMEOUT));
+	if (ret < 0)
+		return dev_err_probe(&bg->sd->dev, ret,
+				     "Failed to acquire ack semaphore");
+
+	switch (READ_ONCE(bg->fwl_ack)) {
+	case CC1352_BOOTLOADER_ACK:
+		return 0;
+	case CC1352_BOOTLOADER_NACK:
+		return -EAGAIN;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int cc1352_bootloader_sync(struct gb_beagleplay *bg)
+{
+	static const u8 sync_bytes[] = { 0x55, 0x55 };
+
+	serdev_device_write_buf(bg->sd, sync_bytes, sizeof(sync_bytes));
+	return cc1352_bootloader_wait_for_ack(bg);
+}
+
+static int cc1352_bootloader_get_status(struct gb_beagleplay *bg)
+{
+	int ret;
+	static const struct cc1352_bootloader_packet pkt = {
+		.len = sizeof(pkt),
+		.checksum = COMMAND_GET_STATUS,
+		.cmd = COMMAND_GET_STATUS
+	};
+
+	serdev_device_write_buf(bg->sd, (const u8 *)&pkt, sizeof(pkt));
+	ret = cc1352_bootloader_wait_for_ack(bg);
+	if (ret < 0)
+		return ret;
+
+	ret = wait_for_completion_timeout(
+		&bg->fwl_cmd_response_com,
+		msecs_to_jiffies(CC1352_BOOTLOADER_TIMEOUT));
+	if (ret < 0)
+		return dev_err_probe(&bg->sd->dev, ret,
+				     "Failed to acquire last status semaphore");
+
+	switch (READ_ONCE(bg->fwl_cmd_response)) {
+	case COMMAND_RET_SUCCESS:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int cc1352_bootloader_erase(struct gb_beagleplay *bg)
+{
+	int ret;
+	static const struct cc1352_bootloader_packet pkt = {
+		.len = sizeof(pkt),
+		.checksum = COMMAND_BANK_ERASE,
+		.cmd = COMMAND_BANK_ERASE
+	};
+
+	serdev_device_write_buf(bg->sd, (const u8 *)&pkt, sizeof(pkt));
+
+	ret = cc1352_bootloader_wait_for_ack(bg);
+	if (ret < 0)
+		return ret;
+
+	return cc1352_bootloader_get_status(bg);
+}
+
+static int cc1352_bootloader_reset(struct gb_beagleplay *bg)
+{
+	static const struct cc1352_bootloader_packet pkt = {
+		.len = sizeof(pkt),
+		.checksum = COMMAND_RESET,
+		.cmd = COMMAND_RESET
+	};
+
+	serdev_device_write_buf(bg->sd, (const u8 *)&pkt, sizeof(pkt));
+
+	return cc1352_bootloader_wait_for_ack(bg);
+}
+
+/**
+ * cc1352_bootloader_empty_pkt: Calculate the number of empty bytes in the current packet
+ */
+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 cc1352_bootloader_crc32(struct gb_beagleplay *bg, u32 *crc32)
+{
+	int ret;
+	static const struct cc1352_bootloader_crc32_cmd_data cmd_data = {
+		.addr = 0, .size = cpu_to_be32(704 * 1024), .read_repeat = 0
+	};
+	const struct cc1352_bootloader_packet pkt = {
+		.len = sizeof(pkt) + sizeof(cmd_data),
+		.checksum = csum8((const void *)&cmd_data, sizeof(cmd_data),
+				  COMMAND_CRC32),
+		.cmd = COMMAND_CRC32
+	};
+
+	serdev_device_write_buf(bg->sd, (const u8 *)&pkt, sizeof(pkt));
+	serdev_device_write_buf(bg->sd, (const u8 *)&cmd_data,
+				sizeof(cmd_data));
+
+	ret = cc1352_bootloader_wait_for_ack(bg);
+	if (ret < 0)
+		return ret;
+
+	ret = wait_for_completion_timeout(
+		&bg->fwl_cmd_response_com,
+		msecs_to_jiffies(CC1352_BOOTLOADER_TIMEOUT));
+	if (ret < 0)
+		return dev_err_probe(&bg->sd->dev, ret,
+				     "Failed to acquire last status semaphore");
+
+	*crc32 = READ_ONCE(bg->fwl_cmd_response);
+
+	return 0;
+}
+
+static int cc1352_bootloader_download(struct gb_beagleplay *bg, u32 size,
+				      u32 addr)
+{
+	int ret;
+	const struct cc1352_bootloader_download_cmd_data cmd_data = {
+		.addr = cpu_to_be32(addr),
+		.size = cpu_to_be32(size),
+	};
+	const struct cc1352_bootloader_packet pkt = {
+		.len = sizeof(pkt) + sizeof(cmd_data),
+		.checksum = csum8((const void *)&cmd_data, sizeof(cmd_data),
+				  COMMAND_DOWNLOAD),
+		.cmd = COMMAND_DOWNLOAD
+	};
+
+	serdev_device_write_buf(bg->sd, (const u8 *)&pkt, sizeof(pkt));
+	serdev_device_write_buf(bg->sd, (const u8 *)&cmd_data,
+				sizeof(cmd_data));
+
+	ret = cc1352_bootloader_wait_for_ack(bg);
+	if (ret < 0)
+		return ret;
+
+	return cc1352_bootloader_get_status(bg);
+}
+
+static int cc1352_bootloader_send_data(struct gb_beagleplay *bg, const u8 *data,
+				       size_t size)
+{
+	int ret, rem = min(size, CC1352_BOOTLOADER_PKT_MAX_SIZE);
+	const struct cc1352_bootloader_packet pkt = {
+		.len = sizeof(pkt) + rem,
+		.checksum = csum8(data, rem, COMMAND_SEND_DATA),
+		.cmd = COMMAND_SEND_DATA
+	};
+
+	serdev_device_write_buf(bg->sd, (const u8 *)&pkt, sizeof(pkt));
+	serdev_device_write_buf(bg->sd, data, rem);
+
+	ret = cc1352_bootloader_wait_for_ack(bg);
+	if (ret < 0)
+		return ret;
+
+	ret = cc1352_bootloader_get_status(bg);
+	if (ret < 0)
+		return ret;
+
+	return rem;
+}
+
 static void gb_greybus_deinit(struct gb_beagleplay *bg)
 {
 	gb_hd_del(bg->gb_hd);
@@ -442,6 +831,154 @@ static int gb_greybus_init(struct gb_beagleplay *bg)
 	return ret;
 }
 
+static enum fw_upload_err cc1352_prepare(struct fw_upload *fw_upload,
+					 const u8 *data, u32 size)
+{
+	int ret;
+	u32 curr_crc32;
+	struct gb_beagleplay *bg = fw_upload->dd_handle;
+
+	dev_info(&bg->sd->dev, "CC1352 Start Flashing...");
+
+	/* Might involve network calls */
+	gb_greybus_deinit(bg);
+	msleep(5 * MSEC_PER_SEC);
+
+	gb_beagleplay_stop_svc(bg);
+	msleep(200);
+	flush_work(&bg->tx_work);
+
+	serdev_device_wait_until_sent(bg->sd, CC1352_BOOTLOADER_TIMEOUT);
+
+	WRITE_ONCE(bg->flashing_mode, true);
+
+	gpiod_direction_output(bg->boot_gpio, 0);
+	gpiod_direction_output(bg->rst_gpio, 0);
+	msleep(200);
+
+	gpiod_set_value(bg->rst_gpio, 1);
+	msleep(200);
+
+	gpiod_set_value(bg->boot_gpio, 1);
+	msleep(200);
+
+	gpiod_direction_input(bg->boot_gpio);
+	gpiod_direction_input(bg->rst_gpio);
+
+	ret = cc1352_bootloader_sync(bg);
+	if (ret < 0)
+		return dev_err_probe(&bg->sd->dev, FW_UPLOAD_ERR_HW_ERROR,
+				     "Failed to sync");
+
+	ret = cc1352_bootloader_crc32(bg, &curr_crc32);
+	if (ret < 0)
+		return dev_err_probe(&bg->sd->dev, FW_UPLOAD_ERR_HW_ERROR,
+				     "Failed to fetch crc32");
+
+	bg->fwl_crc32 = crc32(0xffffffff, data, size) ^ 0xffffffff;
+
+	/* Check if attempting to reflash same firmware */
+	if (bg->fwl_crc32 == curr_crc32) {
+		dev_warn(&bg->sd->dev, "Skipping reflashing same image");
+		cc1352_bootloader_reset(bg);
+		WRITE_ONCE(bg->flashing_mode, false);
+		msleep(200);
+		gb_greybus_init(bg);
+		gb_beagleplay_start_svc(bg);
+		return FW_UPLOAD_ERR_FW_INVALID;
+	}
+
+	ret = cc1352_bootloader_erase(bg);
+	if (ret < 0)
+		return dev_err_probe(&bg->sd->dev, FW_UPLOAD_ERR_HW_ERROR,
+				     "Failed to erase");
+
+	bg->fwl_reset_addr = true;
+
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static void cc1352_cleanup(struct fw_upload *fw_upload)
+{
+	struct gb_beagleplay *bg = fw_upload->dd_handle;
+
+	WRITE_ONCE(bg->flashing_mode, false);
+}
+
+static enum fw_upload_err cc1352_write(struct fw_upload *fw_upload,
+				       const u8 *data, u32 offset, u32 size,
+				       u32 *written)
+{
+	int ret;
+	size_t empty_bytes;
+	struct gb_beagleplay *bg = fw_upload->dd_handle;
+
+	/* Skip 0xff packets. Significant performance improvement */
+	empty_bytes = cc1352_bootloader_empty_pkt(data + offset, size);
+	if (empty_bytes >= CC1352_BOOTLOADER_PKT_MAX_SIZE) {
+		bg->fwl_reset_addr = true;
+		*written = empty_bytes;
+		return FW_UPLOAD_ERR_NONE;
+	}
+
+	if (bg->fwl_reset_addr) {
+		ret = cc1352_bootloader_download(bg, size, offset);
+		if (ret < 0)
+			return dev_err_probe(&bg->sd->dev,
+					     FW_UPLOAD_ERR_HW_ERROR,
+					     "Failed to send download cmd");
+
+		bg->fwl_reset_addr = false;
+	}
+
+	ret = cc1352_bootloader_send_data(bg, data + offset, size);
+	if (ret < 0)
+		return dev_err_probe(&bg->sd->dev, FW_UPLOAD_ERR_HW_ERROR,
+				     "Failed to flash firmware");
+	*written = ret;
+
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static enum fw_upload_err cc1352_poll_complete(struct fw_upload *fw_upload)
+{
+	u32 curr_crc32;
+	struct gb_beagleplay *bg = fw_upload->dd_handle;
+
+	if (cc1352_bootloader_crc32(bg, &curr_crc32) < 0)
+		return dev_err_probe(&bg->sd->dev, FW_UPLOAD_ERR_HW_ERROR,
+				     "Failed to fetch crc32");
+
+	if (bg->fwl_crc32 != curr_crc32)
+		return dev_err_probe(&bg->sd->dev, FW_UPLOAD_ERR_FW_INVALID,
+				     "Invalid CRC32");
+
+	if (cc1352_bootloader_reset(bg) < 0)
+		return dev_err_probe(&bg->sd->dev, FW_UPLOAD_ERR_HW_ERROR,
+				     "Failed to reset");
+
+	dev_info(&bg->sd->dev, "CC1352 Flashing Successful");
+	WRITE_ONCE(bg->flashing_mode, false);
+	msleep(200);
+
+	if (gb_greybus_init(bg) < 0)
+		return dev_err_probe(&bg->sd->dev, FW_UPLOAD_ERR_RW_ERROR,
+				     "Failed to initialize greybus");
+
+	gb_beagleplay_start_svc(bg);
+
+	return FW_UPLOAD_ERR_NONE;
+}
+
+static void cc1352_cancel(struct fw_upload *fw_upload)
+{
+	struct gb_beagleplay *bg = fw_upload->dd_handle;
+
+	dev_info(&bg->sd->dev, "CC1352 Bootloader Cancel");
+
+	cc1352_bootloader_reset(bg);
+}
+
 static void gb_serdev_deinit(struct gb_beagleplay *bg)
 {
 	serdev_device_close(bg->sd);
@@ -463,6 +1000,65 @@ static int gb_serdev_init(struct gb_beagleplay *bg)
 	return 0;
 }
 
+static const struct fw_upload_ops cc1352_bootloader_ops = {
+	.prepare = cc1352_prepare,
+	.write = cc1352_write,
+	.poll_complete = cc1352_poll_complete,
+	.cancel = cc1352_cancel,
+	.cleanup = cc1352_cleanup
+};
+
+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);
+	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;
@@ -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

Re: [PATCH 3/3] greybus: gb-beagleplay: Add firmware upload API

[Hariprasad Kelam]

On 2024-07-19 at 15:15:12, Ayush Singh (ayush@beagleboard.org) 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>
> ---
>  drivers/greybus/Kconfig         |   1 +
>  drivers/greybus/gb-beagleplay.c | 625 +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 614 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/greybus/Kconfig b/drivers/greybus/Kconfig
> index ab81ceceb337..d485a99959cb 100644
> --- a/drivers/greybus/Kconfig
> +++ b/drivers/greybus/Kconfig
> @@ -21,6 +21,7 @@ config GREYBUS_BEAGLEPLAY
>  	tristate "Greybus BeaglePlay driver"
>  	depends on SERIAL_DEV_BUS
>  	select CRC_CCITT
> +	select FW_UPLOAD
>  	help
>  	  Select this option if you have a BeaglePlay where CC1352
>  	  co-processor acts as Greybus SVC.
> diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
> index 33f8fad70260..aecbfb5b5eaf 100644
> --- a/drivers/greybus/gb-beagleplay.c
> +++ b/drivers/greybus/gb-beagleplay.c
> @@ -6,21 +6,18 @@
>   * Copyright (c) 2023 BeagleBoard.org Foundation
>   */
>  
> -#include <linux/gfp.h>
> +#include <asm-generic/unaligned.h>
> +#include <linux/crc32.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/firmware.h>
>  #include <linux/greybus.h>
> -#include <linux/module.h>
> -#include <linux/of.h>
> -#include <linux/printk.h>
>  #include <linux/serdev.h>
> -#include <linux/tty.h>
> -#include <linux/tty_driver.h>
> -#include <linux/greybus/hd.h>
> -#include <linux/init.h>
> -#include <linux/device.h>
>  #include <linux/crc-ccitt.h>
>  #include <linux/circ_buf.h>
> -#include <linux/types.h>
> -#include <linux/workqueue.h>
> +
> +#define CC1352_BOOTLOADER_TIMEOUT 2000
> +#define CC1352_BOOTLOADER_ACK 0xcc
> +#define CC1352_BOOTLOADER_NACK 0x33
>  
>  #define RX_HDLC_PAYLOAD 256
>  #define CRC_LEN 2
> @@ -57,6 +54,17 @@
>   * @rx_buffer_len: length of receive buffer filled.
>   * @rx_buffer: hdlc frame receive buffer
>   * @rx_in_esc: hdlc rx flag to indicate ESC frame
> + *
> + * @fwl: underlying firmware upload device
> + * @boot_gpio: cc1352p7 boot gpio
> + * @rst_gpio: cc1352p7 reset gpio
> + * @flashing_mode: flag to indicate that flashing is currently in progress
> + * @fwl_ack_com: completion to signal an Ack/Nack
> + * @fwl_ack: Ack/Nack byte received
> + * @fwl_cmd_response_com: completion to signal a bootloader command response
> + * @fwl_cmd_response: bootloader command response data
> + * @fwl_crc32: crc32 of firmware to flash
> + * @fwl_reset_addr: flag to indicate if we need to send COMMAND_DOWNLOAD again
>   */
>  struct gb_beagleplay {
>  	struct serdev_device *sd;
> @@ -72,6 +80,17 @@ struct gb_beagleplay {
>  	u16 rx_buffer_len;
>  	bool rx_in_esc;
>  	u8 rx_buffer[MAX_RX_HDLC];
> +
> +	struct fw_upload *fwl;
> +	struct gpio_desc *boot_gpio;
> +	struct gpio_desc *rst_gpio;
> +	bool flashing_mode;
> +	struct completion fwl_ack_com;
> +	u8 fwl_ack;
> +	struct completion fwl_cmd_response_com;
> +	u32 fwl_cmd_response;
> +	u32 fwl_crc32;
> +	bool fwl_reset_addr;
>  };
>  
>  /**
> @@ -100,6 +119,69 @@ struct hdlc_greybus_frame {
>  	u8 payload[];
>  } __packed;
>  
> +/**
> + * enum cc1352_bootloader_cmd: CC1352 Bootloader Commands
> + */
> +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
> + */
> +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,
> +};
> +
> +/**
> + * struct cc1352_bootloader_packet: CC1352 Bootloader Request Packet
> + *
> + * @len: length of packet + optional request data
> + * @checksum: 8-bit checksum excluding len
> + * @cmd: bootloader command
> + */
> +struct cc1352_bootloader_packet {
> +	u8 len;
> +	u8 checksum;
> +	u8 cmd;
> +} __packed;
> +
> +#define CC1352_BOOTLOADER_PKT_MAX_SIZE \
> +	(U8_MAX - sizeof(struct cc1352_bootloader_packet))
> +
> +/**
> + * struct cc1352_bootloader_download_cmd_data: CC1352 Bootloader COMMAND_DOWNLOAD request data
> + *
> + * @addr: address to start programming data into
> + * @size: size of data that will be sent
> + */
> +struct cc1352_bootloader_download_cmd_data {
> +	__be32 addr;
> +	__be32 size;
> +} __packed;
> +
> +/**
> + * struct cc1352_bootloader_crc32_cmd_data: CC1352 Bootloader COMMAND_CRC32 request data
> + *
> + * @addr: address where crc32 calculation starts
> + * @size: number of bytes comprised by crc32 calculation
> + * @read_repeat: number of read repeats for each data location
> + */
> +struct cc1352_bootloader_crc32_cmd_data {
> +	__be32 addr;
> +	__be32 size;
> +	__be32 read_repeat;
> +} __packed;
> +
>  static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
>  {
>  	struct hdlc_greybus_frame *gb_frame = (struct hdlc_greybus_frame *)buf;
> @@ -331,11 +413,131 @@ static void hdlc_deinit(struct gb_beagleplay *bg)
>  	flush_work(&bg->tx_work);
>  }
>  
> +/**
> + * csum8: Calculate 8-bit checksum on data
> + */
> +static u8 csum8(const u8 *data, size_t size, u8 base)
> +{
> +	size_t i;
> +	u8 sum = base;
follow reverse x-mas tree
> +
> +	for (i = 0; i < size; ++i)
> +		sum += data[i];
> +
> +	return sum;
> +}
> +
> +static void cc1352_bootloader_send_ack(struct gb_beagleplay *bg)
> +{
> +	static const u8 ack[] = { 0x00, CC1352_BOOTLOADER_ACK };
> +
> +	serdev_device_write_buf(bg->sd, ack, sizeof(ack));
> +}
> +
> +static void cc1352_bootloader_send_nack(struct gb_beagleplay *bg)
> +{
> +	static const u8 nack[] = { 0x00, CC1352_BOOTLOADER_NACK };
> +
> +	serdev_device_write_buf(bg->sd, nack, sizeof(nack));
> +}
> +
> +/**
> + * cc1352_bootloader_pkt_rx: Process a CC1352 Bootloader Packet
> + *
> + * @bg: beagleplay greybus driver
> + * @data: packet buffer
> + * @count: packet buffer size
> + *
> + * @return: number of bytes processed
> + *
> + * Here are the steps to successfully receive a packet from cc1352 bootloader
> + * according to the docs:
> + * 1. Wait for nonzero data to be returned from the device. This is important
> + *    as the device may send zero bytes between a sent and a received data
> + *    packet. The first nonzero byte received is the size of the packet that is
> + *    being received.
> + * 2. Read the next byte, which is the checksum for the packet.
> + * 3. Read the data bytes from the device. During the data phase, packet size
> + *    minus 2 bytes is sent.
> + * 4. Calculate the checksum of the data bytes and verify it matches the
> + *    checksum received in the packet.
> + * 5. Send an acknowledge byte or a not-acknowledge byte to the device to
> + *    indicate the successful or unsuccessful reception of the packet.
> + */
> +static int cc1352_bootloader_pkt_rx(struct gb_beagleplay *bg, const u8 *data,
> +				    size_t count)
> +{
> +	bool is_valid = false;
> +
> +	switch (data[0]) {
> +	/* Skip 0x00 bytes.  */
> +	case 0x00:
> +		return 1;
> +	case CC1352_BOOTLOADER_ACK:
> +	case CC1352_BOOTLOADER_NACK:
> +		WRITE_ONCE(bg->fwl_ack, data[0]);
> +		complete(&bg->fwl_ack_com);
> +		return 1;
> +	case 3:
> +		if (count < 3)
> +			return 0;
> +		is_valid = data[1] == data[2];
> +		WRITE_ONCE(bg->fwl_cmd_response, (u32)data[2]);
> +		break;
> +	case 6:
> +		if (count < 6)
> +			return 0;
> +		is_valid = csum8(&data[2], sizeof(__be32), 0) == data[1];
> +		WRITE_ONCE(bg->fwl_cmd_response, get_unaligned_be32(&data[2]));
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (is_valid) {
> +		cc1352_bootloader_send_ack(bg);
> +		complete(&bg->fwl_cmd_response_com);
> +	} else {
> +		dev_warn(&bg->sd->dev,
> +			 "Dropping bootloader packet with invalid checksum");
> +		cc1352_bootloader_send_nack(bg);
> +	}
> +
> +	return data[0];
> +}
> +
> +static size_t cc1352_bootloader_rx(struct gb_beagleplay *bg, const u8 *data,
> +				   size_t count)
> +{
> +	int ret;
> +	size_t off = 0;
> +
        Same here
      
> +	memcpy(bg->rx_buffer + bg->rx_buffer_len, data, count);
> +	bg->rx_buffer_len += count;
> +
> +	do {
> +		ret = cc1352_bootloader_pkt_rx(bg, bg->rx_buffer + off,
> +					       bg->rx_buffer_len - off);
> +		if (ret < 0)
> +			return dev_err_probe(&bg->sd->dev, ret,
> +					     "Invalid Packet");
> +		off += ret;
> +	} while (ret > 0 && off < count);
> +
> +	bg->rx_buffer_len -= off;
> +	memmove(bg->rx_buffer, bg->rx_buffer + off, bg->rx_buffer_len);
> +
> +	return count;
> +}
> +
>  static size_t gb_tty_receive(struct serdev_device *sd, const u8 *data,
>  			     size_t count)
>  {
>  	struct gb_beagleplay *bg = serdev_device_get_drvdata(sd);
>  
> +	if (READ_ONCE(bg->flashing_mode))
> +		return cc1352_bootloader_rx(bg, data, count);
> +
>  	return hdlc_rx(bg, data, count);
>  }
>  
> @@ -343,7 +545,8 @@ static void gb_tty_wakeup(struct serdev_device *serdev)
>  {
>  	struct gb_beagleplay *bg = serdev_device_get_drvdata(serdev);
>  
> -	schedule_work(&bg->tx_work);
> +	if (!READ_ONCE(bg->flashing_mode))
> +		schedule_work(&bg->tx_work);
>  }
>  
>  static struct serdev_device_ops gb_beagleplay_ops = {
> @@ -412,6 +615,192 @@ static void gb_beagleplay_stop_svc(struct gb_beagleplay *bg)
>  	hdlc_tx_frames(bg, ADDRESS_CONTROL, 0x03, &payload, 1);
>  }
>  
> +static int cc1352_bootloader_wait_for_ack(struct gb_beagleplay *bg)
> +{
> +	int ret;
> +
> +	ret = wait_for_completion_timeout(
> +		&bg->fwl_ack_com, msecs_to_jiffies(CC1352_BOOTLOADER_TIMEOUT));
> +	if (ret < 0)
> +		return dev_err_probe(&bg->sd->dev, ret,
> +				     "Failed to acquire ack semaphore");
> +
> +	switch (READ_ONCE(bg->fwl_ack)) {
> +	case CC1352_BOOTLOADER_ACK:
> +		return 0;
> +	case CC1352_BOOTLOADER_NACK:
> +		return -EAGAIN;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int cc1352_bootloader_sync(struct gb_beagleplay *bg)
> +{
> +	static const u8 sync_bytes[] = { 0x55, 0x55 };
> +
> +	serdev_device_write_buf(bg->sd, sync_bytes, sizeof(sync_bytes));
> +	return cc1352_bootloader_wait_for_ack(bg);
> +}
> +
> +static int cc1352_bootloader_get_status(struct gb_beagleplay *bg)
> +{
> +	int ret;
> +	static const struct cc1352_bootloader_packet pkt = {
> +		.len = sizeof(pkt),
> +		.checksum = COMMAND_GET_STATUS,
> +		.cmd = COMMAND_GET_STATUS
> +	};
> +
     same here. please run checkpatch before submitting to know coding
     style issues.

Re: [PATCH 3/3] greybus: gb-beagleplay: Add firmware upload API

[Andrew Lunn]

> >  drivers/greybus/Kconfig         |   1 +
> >  drivers/greybus/gb-beagleplay.c | 625 +++++++++++++++++++++++++++++++++++++++-

> > +static u8 csum8(const u8 *data, size_t size, u8 base)
> > +{
> > +	size_t i;
> > +	u8 sum = base;
> follow reverse x-mas tree

Since this is not networking, even thought it was posted to the netdev
list, this comment might not be correct. I had a quick look at some
greybus code and reverse x-mas tree is not strictly used.

Please see what the Graybus Maintainers say.

	Andrew

Re: [PATCH 3/3] greybus: gb-beagleplay: Add firmware upload API

[Alex Elder]

On 7/19/24 2:15 PM, Andrew Lunn wrote:
>>>   drivers/greybus/Kconfig         |   1 +
>>>   drivers/greybus/gb-beagleplay.c | 625 +++++++++++++++++++++++++++++++++++++++-
> 
>>> +static u8 csum8(const u8 *data, size_t size, u8 base)
>>> +{
>>> +	size_t i;
>>> +	u8 sum = base;
>> follow reverse x-mas tree
> 
> Since this is not networking, even thought it was posted to the netdev
> list, this comment might not be correct. I had a quick look at some
> greybus code and reverse x-mas tree is not strictly used.
> 
> Please see what the Graybus Maintainers say.

Andrew is correct.  The Greybus code does not strictly follow
the "reverse christmas tree" convention, so there is no need
to do that here.  Please understand that, while checkpatch.pl
offers good and well-intentioned advice, not everything it
warns about must be fixed, and in some cases it suggests things
certain maintainers don't agree with.

					-Alex

> 	Andrew

Re: [PATCH 3/3] greybus: gb-beagleplay: Add firmware upload API

[Hariprasad Kelam]

On 2024-07-20 at 03:09:55, Alex Elder (elder@ieee.org) wrote:
> On 7/19/24 2:15 PM, Andrew Lunn wrote:
> > > >   drivers/greybus/Kconfig         |   1 +
> > > >   drivers/greybus/gb-beagleplay.c | 625 +++++++++++++++++++++++++++++++++++++++-
> > 
> > > > +static u8 csum8(const u8 *data, size_t size, u8 base)
> > > > +{
> > > > +	size_t i;
> > > > +	u8 sum = base;
> > > follow reverse x-mas tree
> > 
> > Since this is not networking, even thought it was posted to the netdev
> > list, this comment might not be correct. I had a quick look at some
> > greybus code and reverse x-mas tree is not strictly used.
> > 
> > Please see what the Graybus Maintainers say.
> 
> Andrew is correct.  The Greybus code does not strictly follow
> the "reverse christmas tree" convention, so there is no need
> to do that here.  Please understand that, while checkpatch.pl
> offers good and well-intentioned advice, not everything it
> warns about must be fixed, and in some cases it suggests things
> certain maintainers don't agree with.
> 
> 					-Alex
> 
> > 	Andrew
> 
> Ok got it. 

Re: [PATCH 3/3] greybus: gb-beagleplay: Add firmware upload API

[kernel test robot]

Hi Ayush,

kernel test robot noticed the following build warnings:

[auto build test WARNING on f76698bd9a8ca01d3581236082d786e9a6b72bb7]

url:    https://github.com/intel-lab-lkp/linux/commits/Ayush-Singh/dt-bindings-net-ti-cc1352p7-Add-boot-gpio/20240719-180050
base:   f76698bd9a8ca01d3581236082d786e9a6b72bb7
patch link:    https://lore.kernel.org/r/20240719-beagleplay_fw_upgrade-v1-3-8664d4513252%40beagleboard.org
patch subject: [PATCH 3/3] greybus: gb-beagleplay: Add firmware upload API
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20240721/202407210006.Wvm3bOm9-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240721/202407210006.Wvm3bOm9-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407210006.Wvm3bOm9-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/greybus/gb-beagleplay.c:132: warning: Enum value 'COMMAND_DOWNLOAD' not described in enum 'cc1352_bootloader_cmd'
>> drivers/greybus/gb-beagleplay.c:132: warning: Enum value 'COMMAND_GET_STATUS' not described in enum 'cc1352_bootloader_cmd'
>> drivers/greybus/gb-beagleplay.c:132: warning: Enum value 'COMMAND_SEND_DATA' not described in enum 'cc1352_bootloader_cmd'
>> drivers/greybus/gb-beagleplay.c:132: warning: Enum value 'COMMAND_RESET' not described in enum 'cc1352_bootloader_cmd'
>> drivers/greybus/gb-beagleplay.c:132: warning: Enum value 'COMMAND_CRC32' not described in enum 'cc1352_bootloader_cmd'
>> drivers/greybus/gb-beagleplay.c:132: warning: Enum value 'COMMAND_BANK_ERASE' not described in enum 'cc1352_bootloader_cmd'
>> drivers/greybus/gb-beagleplay.c:143: warning: Enum value 'COMMAND_RET_SUCCESS' not described in enum 'cc1352_bootloader_status'
>> drivers/greybus/gb-beagleplay.c:143: warning: Enum value 'COMMAND_RET_UNKNOWN_CMD' not described in enum 'cc1352_bootloader_status'
>> drivers/greybus/gb-beagleplay.c:143: warning: Enum value 'COMMAND_RET_INVALID_CMD' not described in enum 'cc1352_bootloader_status'
>> drivers/greybus/gb-beagleplay.c:143: warning: Enum value 'COMMAND_RET_INVALID_ADR' not described in enum 'cc1352_bootloader_status'
>> drivers/greybus/gb-beagleplay.c:143: warning: Enum value 'COMMAND_RET_FLASH_FAIL' not described in enum 'cc1352_bootloader_status'
>> drivers/greybus/gb-beagleplay.c:420: warning: Function parameter or struct member 'data' not described in 'csum8'
>> drivers/greybus/gb-beagleplay.c:420: warning: Function parameter or struct member 'size' not described in 'csum8'
>> drivers/greybus/gb-beagleplay.c:420: warning: Function parameter or struct member 'base' not described in 'csum8'
>> drivers/greybus/gb-beagleplay.c:712: warning: Function parameter or struct member 'data' not described in 'cc1352_bootloader_empty_pkt'
>> drivers/greybus/gb-beagleplay.c:712: warning: Function parameter or struct member 'size' not described in 'cc1352_bootloader_empty_pkt'


vim +132 drivers/greybus/gb-beagleplay.c

   121	
   122	/**
   123	 * enum cc1352_bootloader_cmd: CC1352 Bootloader Commands
   124	 */
   125	enum cc1352_bootloader_cmd {
   126		COMMAND_DOWNLOAD = 0x21,
   127		COMMAND_GET_STATUS = 0x23,
   128		COMMAND_SEND_DATA = 0x24,
   129		COMMAND_RESET = 0x25,
   130		COMMAND_CRC32 = 0x27,
   131		COMMAND_BANK_ERASE = 0x2c,
 > 132	};
   133	
   134	/**
   135	 * enum cc1352_bootloader_status: CC1352 Bootloader COMMAND_GET_STATUS response
   136	 */
   137	enum cc1352_bootloader_status {
   138		COMMAND_RET_SUCCESS = 0x40,
   139		COMMAND_RET_UNKNOWN_CMD = 0x41,
   140		COMMAND_RET_INVALID_CMD = 0x42,
   141		COMMAND_RET_INVALID_ADR = 0x43,
   142		COMMAND_RET_FLASH_FAIL = 0x44,
 > 143	};
   144	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/3] greybus: gb-beagleplay: Add firmware upload API

[Simon Horman]

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
> 
>