[PATCH v1] Bluetooth: btnxpuart: Add support to download helper FW file for w8997

[PATCH v1] Bluetooth: btnxpuart: Add support to download helper FW file for w8997

[Neeraj Sanjay Kale]

This adds support to download helper FW file for the legacy NXP chipset
88w8997 for the btnxpuart driver. This helper FW file is necessary to
set the bootloader baudrate to 3000000 after which the actual BT FW file
can be downloaded. This change helps bring the FW download time from
around 10 sec to less than 2 sec for 88w8997 chip. For newer chipsets,
both V1 and V3 bootloader, driver sends the cmd5 and cmd7 to the chip
bootloader, and does not need a helper FW file.

Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
---
 drivers/bluetooth/btnxpuart.c | 67 ++++++++++++++++++++++++++++++-----
 1 file changed, 59 insertions(+), 8 deletions(-)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 7c22c1ac087a..34f44da9ef4d 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -34,6 +34,7 @@
 #define FIRMWARE_W9098	"nxp/uartuart9098_bt_v1.bin"
 #define FIRMWARE_IW416	"nxp/uartiw416_bt_v0.bin"
 #define FIRMWARE_IW612	"nxp/uartspi_n61x_v1.bin.se"
+#define FIRMWARE_HELPER	"nxp/helper_uart_3000000.bin"
 
 #define CHIP_ID_W9098		0x5c03
 #define CHIP_ID_IW416		0x7201
@@ -123,7 +124,7 @@ struct psmode_cmd_payload {
 } __packed;
 
 struct btnxpuart_data {
-	bool fw_download_3M_baudrate;
+	const char *helper_fw_name;
 	const char *fw_name;
 };
 
@@ -150,6 +151,7 @@ struct btnxpuart_dev {
 	u32 fw_init_baudrate;
 	bool timeout_changed;
 	bool baudrate_changed;
+	bool helper_downloaded;
 
 	struct ps_data psdata;
 	struct btnxpuart_data *nxp_data;
@@ -168,6 +170,13 @@ struct btnxpuart_dev {
 
 #define HDR_LEN			16
 
+#define NXP_RECV_CHIP_VER_V1 \
+	.type = NXP_V1_CHIP_VER_PKT, \
+	.hlen = 4, \
+	.loff = 0, \
+	.lsize = 0, \
+	.maxlen = 4
+
 #define NXP_RECV_FW_REQ_V1 \
 	.type = NXP_V1_FW_REQ_PKT, \
 	.hlen = 4, \
@@ -194,6 +203,11 @@ struct v1_data_req {
 	__le16 len_comp;
 } __packed;
 
+struct v1_start_ind {
+	__le16 chip_id;
+	__le16 chip_id_comp;
+} __packed;
+
 struct v3_data_req {
 	__le16 len;
 	__le32 offset;
@@ -518,6 +532,7 @@ static int nxp_download_firmware(struct hci_dev *hdev)
 	nxpdev->fw_v3_offset_correction = 0;
 	nxpdev->baudrate_changed = false;
 	nxpdev->timeout_changed = false;
+	nxpdev->helper_downloaded = false;
 
 	serdev_device_set_baudrate(nxpdev->serdev, HCI_NXP_PRI_BAUDRATE);
 	serdev_device_set_flow_control(nxpdev->serdev, 0);
@@ -664,6 +679,29 @@ static int nxp_request_firmware(struct hci_dev *hdev, const char *fw_name)
 }
 
 /* for legacy chipsets with V1 bootloader */
+static int nxp_recv_chip_ver_v1(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
+	struct v1_start_ind *req;
+
+	req = (struct v1_start_ind *)skb_pull_data(skb, sizeof(struct v1_start_ind));
+	if (!req)
+		goto free_skb;
+
+	if ((req->chip_id ^ req->chip_id_comp) == 0xffff) {
+		nxpdev->fw_dnld_v1_offset = 0;
+		nxpdev->fw_v1_sent_bytes = 0;
+		nxpdev->fw_v1_expected_len = HDR_LEN;
+		release_firmware(nxpdev->fw);
+		memset(nxpdev->fw_name, 0, sizeof(nxpdev->fw_name));
+		nxp_send_ack(NXP_ACK_V1, hdev);
+	}
+
+free_skb:
+	kfree_skb(skb);
+	return 0;
+}
+
 static int nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
@@ -685,7 +723,7 @@ static int nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb)
 	}
 	nxp_send_ack(NXP_ACK_V1, hdev);
 
-	if (nxp_data->fw_download_3M_baudrate) {
+	if (!nxp_data->helper_fw_name) {
 		if (!nxpdev->timeout_changed) {
 			nxpdev->timeout_changed = nxp_fw_change_timeout(hdev, req->len);
 			goto free_skb;
@@ -702,14 +740,26 @@ static int nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb)
 		}
 	}
 
-	if (nxp_request_firmware(hdev, nxp_data->fw_name))
-		goto free_skb;
+	if (!nxp_data->helper_fw_name || nxpdev->helper_downloaded) {
+		if (nxp_request_firmware(hdev, nxp_data->fw_name))
+			goto free_skb;
+	} else if (nxp_data->helper_fw_name && !nxpdev->helper_downloaded) {
+		if (nxp_request_firmware(hdev, nxp_data->helper_fw_name))
+			goto free_skb;
+	}
 
 	requested_len = req->len;
 	if (requested_len == 0) {
 		bt_dev_dbg(hdev, "FW Downloaded Successfully: %zu bytes", nxpdev->fw->size);
-		clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
-		wake_up_interruptible(&nxpdev->fw_dnld_done_wait_q);
+		if (nxp_data->helper_fw_name && !nxpdev->helper_downloaded) {
+			nxpdev->helper_downloaded = true;
+			serdev_device_wait_until_sent(nxpdev->serdev, 0);
+			serdev_device_set_baudrate(nxpdev->serdev, HCI_NXP_SEC_BAUDRATE);
+			serdev_device_set_flow_control(nxpdev->serdev, 1);
+		} else {
+			clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
+			wake_up_interruptible(&nxpdev->fw_dnld_done_wait_q);
+		}
 		goto free_skb;
 	}
 	if (requested_len & 0x01) {
@@ -1142,6 +1192,7 @@ static const struct h4_recv_pkt nxp_recv_pkts[] = {
 	{ H4_RECV_ACL,          .recv = hci_recv_frame },
 	{ H4_RECV_SCO,          .recv = hci_recv_frame },
 	{ H4_RECV_EVENT,        .recv = hci_recv_frame },
+	{ NXP_RECV_CHIP_VER_V1, .recv = nxp_recv_chip_ver_v1 },
 	{ NXP_RECV_FW_REQ_V1,   .recv = nxp_recv_fw_req_v1 },
 	{ NXP_RECV_CHIP_VER_V3, .recv = nxp_recv_chip_ver_v3 },
 	{ NXP_RECV_FW_REQ_V3,   .recv = nxp_recv_fw_req_v3 },
@@ -1252,12 +1303,12 @@ static void nxp_serdev_remove(struct serdev_device *serdev)
 }
 
 static struct btnxpuart_data w8987_data = {
-	.fw_download_3M_baudrate = true,
+	.helper_fw_name = NULL,
 	.fw_name = FIRMWARE_W8987,
 };
 
 static struct btnxpuart_data w8997_data = {
-	.fw_download_3M_baudrate = false,
+	.helper_fw_name = FIRMWARE_HELPER,
 	.fw_name = FIRMWARE_W8997,
 };
 
-- 
2.34.1

[PATCH v1] Bluetooth: btnxpuart: Deasset UART break before closing serdev device

[Neeraj Sanjay Kale]

This adds a call to ps_wakeup() before closing the serdev device, to
de-assert UART break.

Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
---
 drivers/bluetooth/btnxpuart.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 34f44da9ef4d..ad9e25e0c350 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -1167,6 +1167,7 @@ static int btnxpuart_close(struct hci_dev *hdev)
 {
 	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
 
+	ps_wakeup(nxpdev);
 	serdev_device_close(nxpdev->serdev);
 	clear_bit(BTNXPUART_SERDEV_OPEN, &nxpdev->tx_state);
 	return 0;
-- 
2.34.1

Re: [PATCH v1] Bluetooth: btnxpuart: Deasset UART break before closing serdev device

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Mon,  3 Apr 2023 17:54:28 +0530 you wrote:
> This adds a call to ps_wakeup() before closing the serdev device, to
> de-assert UART break.
> 
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> ---
>  drivers/bluetooth/btnxpuart.c | 1 +
>  1 file changed, 1 insertion(+)

Here is the summary with links:
  - [v1] Bluetooth: btnxpuart: Deasset UART break before closing serdev device
    https://git.kernel.org/bluetooth/bluetooth-next/c/051711980f5a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[PATCH v1] Bluetooth: btnxpuart: Disable Power Save feature on startup

[Neeraj Sanjay Kale]

This sets the default power save mode setting to disabled.
With this setting, this driver will behave like a normal h4 driver.
If user needs to use the power save feature, it can be enabled
using the following vendor command:
hcitool cmd 3f 23 02 00 00 (HCI_NXP_AUTO_SLEEP_MODE)

Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
---
 drivers/bluetooth/btnxpuart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index ad9e25e0c350..93f3afc0c0c8 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -262,7 +262,7 @@ static u8 crc8_table[CRC8_TABLE_SIZE];
 
 /* Default configurations */
 #define DEFAULT_H2C_WAKEUP_MODE	WAKEUP_METHOD_BREAK
-#define DEFAULT_PS_MODE		PS_MODE_ENABLE
+#define DEFAULT_PS_MODE		PS_MODE_DISABLE
 #define FW_INIT_BAUDRATE	HCI_NXP_PRI_BAUDRATE
 
 static struct sk_buff *nxp_drv_send_cmd(struct hci_dev *hdev, u16 opcode,
-- 
2.34.1

Re: [PATCH v1] Bluetooth: btnxpuart: Disable Power Save feature on startup

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Mon,  3 Apr 2023 17:54:29 +0530 you wrote:
> This sets the default power save mode setting to disabled.
> With this setting, this driver will behave like a normal h4 driver.
> If user needs to use the power save feature, it can be enabled
> using the following vendor command:
> hcitool cmd 3f 23 02 00 00 (HCI_NXP_AUTO_SLEEP_MODE)
> 
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> 
> [...]

Here is the summary with links:
  - [v1] Bluetooth: btnxpuart: Disable Power Save feature on startup
    https://git.kernel.org/bluetooth/bluetooth-next/c/3f8dae828fb2

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[PATCH v1] Bluetooth: btnxpuart: No need to check the received bootloader signature

[Neeraj Sanjay Kale]

We can never assume the uart will deliver a complete packet to the BT
layer at once, the expected packet may be divided into several parts by
uart as uart doesn't know the received packet size, the received data
count may mismatch with the expected packet size, so here
is_valid_bootloader_signature() check may always return false.

Even we remove the count check in is_valid_bootloader_signature(), then
the first part of the data which includes the packet type can pass the
is_valid_bootloader_signature() check, but the remaining parts don't
have the packet type data still cannot pass the check, here return
directly will cause the data loss.

So need to remove the received bootloader signature check here, the
h4_recv_buf() can help us combine the different data received into one
packet. If any out-of-sync or incomplete bootloader signature is received,
it is safe to ignore and discard it, and process the next bootloader
signature.

Co-developed-by: Sherry Sun <sherry.sun@nxp.com>
Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
---
 drivers/bluetooth/btnxpuart.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 5f641466d695..7c22c1ac087a 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -1147,33 +1147,20 @@ static const struct h4_recv_pkt nxp_recv_pkts[] = {
 	{ NXP_RECV_FW_REQ_V3,   .recv = nxp_recv_fw_req_v3 },
 };
 
-static bool is_valid_bootloader_signature(const u8 *data, size_t count)
-{
-	if ((*data == NXP_V1_FW_REQ_PKT && count == sizeof(struct v1_data_req) + 1) ||
-	    (*data == NXP_V3_FW_REQ_PKT && count == sizeof(struct v3_data_req) + 1) ||
-	    (*data == NXP_V3_CHIP_VER_PKT && count == sizeof(struct v3_start_ind) + 1))
-		return true;
-	else
-		return false;
-}
-
 static int btnxpuart_receive_buf(struct serdev_device *serdev, const u8 *data,
 				 size_t count)
 {
 	struct btnxpuart_dev *nxpdev = serdev_device_get_drvdata(serdev);
 
-	if (is_fw_downloading(nxpdev) && !is_valid_bootloader_signature(data, count)) {
-		/* Unknown bootloader signature, skip without returning error */
-		return count;
-	}
-
 	ps_start_timer(nxpdev);
 
 	nxpdev->rx_skb = h4_recv_buf(nxpdev->hdev, nxpdev->rx_skb, data, count,
 				     nxp_recv_pkts, ARRAY_SIZE(nxp_recv_pkts));
 	if (IS_ERR(nxpdev->rx_skb)) {
 		int err = PTR_ERR(nxpdev->rx_skb);
-
+		/* Safe to ignore out-of-sync bootloader signatures */
+		if (is_fw_downloading(nxpdev))
+			return count;
 		bt_dev_err(nxpdev->hdev, "Frame reassembly failed (%d)", err);
 		nxpdev->rx_skb = NULL;
 		return err;
-- 
2.34.1

Re: [PATCH v1] Bluetooth: btnxpuart: No need to check the received bootloader signature

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Mon,  3 Apr 2023 17:54:30 +0530 you wrote:
> We can never assume the uart will deliver a complete packet to the BT
> layer at once, the expected packet may be divided into several parts by
> uart as uart doesn't know the received packet size, the received data
> count may mismatch with the expected packet size, so here
> is_valid_bootloader_signature() check may always return false.
> 
> Even we remove the count check in is_valid_bootloader_signature(), then
> the first part of the data which includes the packet type can pass the
> is_valid_bootloader_signature() check, but the remaining parts don't
> have the packet type data still cannot pass the check, here return
> directly will cause the data loss.
> 
> [...]

Here is the summary with links:
  - [v1] Bluetooth: btnxpuart: No need to check the received bootloader signature
    https://git.kernel.org/bluetooth/bluetooth-next/c/b1ff41fd0ee6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v1] Bluetooth: btnxpuart: Add support to download helper FW file for w8997

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Mon,  3 Apr 2023 17:54:27 +0530 you wrote:
> This adds support to download helper FW file for the legacy NXP chipset
> 88w8997 for the btnxpuart driver. This helper FW file is necessary to
> set the bootloader baudrate to 3000000 after which the actual BT FW file
> can be downloaded. This change helps bring the FW download time from
> around 10 sec to less than 2 sec for 88w8997 chip. For newer chipsets,
> both V1 and V3 bootloader, driver sends the cmd5 and cmd7 to the chip
> bootloader, and does not need a helper FW file.
> 
> [...]

Here is the summary with links:
  - [v1] Bluetooth: btnxpuart: Add support to download helper FW file for w8997
    https://git.kernel.org/bluetooth/bluetooth-next/c/e68e21986cfe

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[PATCH v1] Bluetooth: btnxpuart: Disable Power Save feature on startup

[Neeraj Sanjay Kale]

This sets the default power save mode setting to enabled.
The power save feature is now stable and stress test issues,
such as the TX timeout error, has been resolved.
With this setting, the driver will send the vendor command
to FW at startup, to enable power save feature.
User can disable this feature using the following vendor
command:
hcitool cmd 3f 23 03 00 00 (HCI_NXP_AUTO_SLEEP_MODE)

Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
---
 drivers/bluetooth/btnxpuart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index 7f88b6f52f26..42e929f0d141 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -281,7 +281,7 @@ static u8 crc8_table[CRC8_TABLE_SIZE];
 
 /* Default configurations */
 #define DEFAULT_H2C_WAKEUP_MODE	WAKEUP_METHOD_BREAK
-#define DEFAULT_PS_MODE		PS_MODE_DISABLE
+#define DEFAULT_PS_MODE		PS_MODE_ENABLE
 #define FW_INIT_BAUDRATE	HCI_NXP_PRI_BAUDRATE
 
 static struct sk_buff *nxp_drv_send_cmd(struct hci_dev *hdev, u16 opcode,
-- 
2.34.1

Re: [PATCH v1] Bluetooth: btnxpuart: Disable Power Save feature on startup

[Paul Menzel]

Dear Neeraj,


Thank you for your patch. Some nits below.

Am 11.01.24 um 09:06 schrieb Neeraj Sanjay Kale:
> This sets the default power save mode setting to enabled.

This contradicts the commit message summary, which says “Disable”.

> The power save feature is now stable and stress test issues,
> such as the TX timeout error, has been resolved.

have been

Please reference one commit, that fixed this.

> With this setting, the driver will send the vendor command
> to FW at startup, to enable power save feature.
> User can disable this feature using the following vendor
> command:
> hcitool cmd 3f 23 03 00 00 (HCI_NXP_AUTO_SLEEP_MODE)

It would help me, if you used the full 75 characters per line for commit 
messages, and would not break the line after every sentence. If a 
paragraph makes sense, please separate it by a  blank line.

> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> ---
>   drivers/bluetooth/btnxpuart.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> index 7f88b6f52f26..42e929f0d141 100644
> --- a/drivers/bluetooth/btnxpuart.c
> +++ b/drivers/bluetooth/btnxpuart.c
> @@ -281,7 +281,7 @@ static u8 crc8_table[CRC8_TABLE_SIZE];
>   
>   /* Default configurations */
>   #define DEFAULT_H2C_WAKEUP_MODE	WAKEUP_METHOD_BREAK
> -#define DEFAULT_PS_MODE		PS_MODE_DISABLE
> +#define DEFAULT_PS_MODE		PS_MODE_ENABLE
>   #define FW_INIT_BAUDRATE	HCI_NXP_PRI_BAUDRATE
>   
>   static struct sk_buff *nxp_drv_send_cmd(struct hci_dev *hdev, u16 opcode,

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul