diff mbox series

[v1] Bluetooth: btnxpuart: Add support for IW624 chipset

Message ID 20230810094802.832652-1-neeraj.sanjaykale@nxp.com (mailing list archive)
State Superseded
Headers show
Series [v1] Bluetooth: btnxpuart: Add support for IW624 chipset | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint success Gitlint PASS
tedd_an/SubjectPrefix success Gitlint PASS
tedd_an/BuildKernel success BuildKernel PASS
tedd_an/CheckAllWarning success CheckAllWarning PASS
tedd_an/CheckSparse success CheckSparse PASS
tedd_an/CheckSmatch success CheckSparse PASS
tedd_an/BuildKernel32 success BuildKernel32 PASS
tedd_an/TestRunnerSetup success TestRunnerSetup PASS
tedd_an/TestRunner_l2cap-tester success TestRunner PASS
tedd_an/TestRunner_iso-tester success TestRunner PASS
tedd_an/TestRunner_bnep-tester success TestRunner PASS
tedd_an/TestRunner_mgmt-tester success TestRunner PASS
tedd_an/TestRunner_rfcomm-tester success TestRunner PASS
tedd_an/TestRunner_sco-tester success TestRunner PASS
tedd_an/TestRunner_ioctl-tester success TestRunner PASS
tedd_an/TestRunner_mesh-tester success TestRunner PASS
tedd_an/TestRunner_smp-tester success TestRunner PASS
tedd_an/TestRunner_userchan-tester success TestRunner PASS
tedd_an/IncrementalBuild success Incremental Build PASS

Commit Message

Neeraj Sanjay Kale Aug. 10, 2023, 9:48 a.m. UTC
This adds support for NXP IW624 chipset in btnxpuart driver
by adding FW name and bootloader signature. Based on the
loader version bits 7:6 of the bootloader signature, the
driver can choose between selecting secure and non-secure
FW files.
For cmd5 payload during FW download, this chip has addresses
of few registers offset by 1, so added boot_reg_offset to
handle the chip specific offset.

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

Comments

bluez.test.bot@gmail.com Aug. 10, 2023, 10:36 a.m. UTC | #1
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=774873

---Test result---

Test Summary:
CheckPatch                    PASS      0.75 seconds
GitLint                       PASS      0.30 seconds
SubjectPrefix                 PASS      0.11 seconds
BuildKernel                   PASS      32.96 seconds
CheckAllWarning               PASS      36.07 seconds
CheckSparse                   PASS      41.09 seconds
CheckSmatch                   PASS      111.35 seconds
BuildKernel32                 PASS      32.05 seconds
TestRunnerSetup               PASS      481.20 seconds
TestRunner_l2cap-tester       PASS      23.22 seconds
TestRunner_iso-tester         PASS      46.83 seconds
TestRunner_bnep-tester        PASS      10.68 seconds
TestRunner_mgmt-tester        PASS      217.68 seconds
TestRunner_rfcomm-tester      PASS      16.00 seconds
TestRunner_sco-tester         PASS      19.12 seconds
TestRunner_ioctl-tester       PASS      18.01 seconds
TestRunner_mesh-tester        PASS      13.45 seconds
TestRunner_smp-tester         PASS      14.26 seconds
TestRunner_userchan-tester    PASS      11.18 seconds
IncrementalBuild              PASS      30.09 seconds



---
Regards,
Linux Bluetooth
Francesco Dolcini Aug. 10, 2023, 4:46 p.m. UTC | #2
Hello,

On Thu, Aug 10, 2023 at 03:18:02PM +0530, Neeraj Sanjay Kale wrote:
> This adds support for NXP IW624 chipset in btnxpuart driver
> by adding FW name and bootloader signature. Based on the
> loader version bits 7:6 of the bootloader signature, the
> driver can choose between selecting secure and non-secure
> FW files.
> For cmd5 payload during FW download, this chip has addresses
> of few registers offset by 1, so added boot_reg_offset to
> handle the chip specific offset.
> 
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> ---
>  drivers/bluetooth/btnxpuart.c | 44 ++++++++++++++++++++++++-----------
>  1 file changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> index ee6f6c872a34..b42572ca63af 100644
> --- a/drivers/bluetooth/btnxpuart.c
> +++ b/drivers/bluetooth/btnxpuart.c
...
> @@ -547,7 +553,7 @@ static int nxp_download_firmware(struct hci_dev *hdev)
>  	serdev_device_set_flow_control(nxpdev->serdev, false);
>  	nxpdev->current_baudrate = HCI_NXP_PRI_BAUDRATE;
>  
> -	/* Wait till FW is downloaded and CTS becomes low */
> +	/* Wait till FW is downloaded */
>  	err = wait_event_interruptible_timeout(nxpdev->fw_dnld_done_wait_q,
>  					       !test_bit(BTNXPUART_FW_DOWNLOADING,
>  							 &nxpdev->tx_state),
> @@ -558,16 +564,11 @@ static int nxp_download_firmware(struct hci_dev *hdev)
>  	}
>  
>  	serdev_device_set_flow_control(nxpdev->serdev, true);
> -	err = serdev_device_wait_for_cts(nxpdev->serdev, 1, 60000);
> -	if (err < 0) {
> -		bt_dev_err(hdev, "CTS is still high. FW Download failed.");
> -		return err;
> -	}
this seems like an unrelated change, and it's moving from a 60secs
timeout polling CTS to nothing.

What's the reason for this? Should be this a separate commit with a
proper explanation?

Francesco
Neeraj Sanjay Kale Aug. 10, 2023, 6:02 p.m. UTC | #3
Hi Francesco

Thank you for reviewing this patch.

> > --- a/drivers/bluetooth/btnxpuart.c
> > +++ b/drivers/bluetooth/btnxpuart.c
> ...
> > @@ -547,7 +553,7 @@ static int nxp_download_firmware(struct hci_dev
> *hdev)
> >       serdev_device_set_flow_control(nxpdev->serdev, false);
> >       nxpdev->current_baudrate = HCI_NXP_PRI_BAUDRATE;
> >
> > -     /* Wait till FW is downloaded and CTS becomes low */
> > +     /* Wait till FW is downloaded */
> >       err = wait_event_interruptible_timeout(nxpdev->fw_dnld_done_wait_q,
> >                                              !test_bit(BTNXPUART_FW_DOWNLOADING,
> >
> > &nxpdev->tx_state), @@ -558,16 +564,11 @@ static int
> nxp_download_firmware(struct hci_dev *hdev)
> >       }
> >
> >       serdev_device_set_flow_control(nxpdev->serdev, true);
> > -     err = serdev_device_wait_for_cts(nxpdev->serdev, 1, 60000);
> > -     if (err < 0) {
> > -             bt_dev_err(hdev, "CTS is still high. FW Download failed.");
> > -             return err;
> > -     }
> this seems like an unrelated change, and it's moving from a 60secs timeout
> polling CTS to nothing.
> 
> What's the reason for this? Should be this a separate commit with a proper
> explanation?
> 
While working on integrating IW624 in btnxpuart driver, I observed that the first reset command was getting timed out, after FW download was complete 2 out of 10 times. On further timing analysis, I noticed that this wait for CTS code did not actually help much, since CTS is already low after FW download, and becomes high after few more milli-seconds, and then low again after FW is initialized.
 So it was either adding a "wait for CTS high" followed by "wait for CTS low", or simply increasing the sleep delay from 1000msec to 1200msec.
I chose the later as it seemed more cleaner, and did the job perfectly, and tested all previously supported chipsets to make sure nothing is broke.
But you are right, I should add an explanation for this change in the commit message in the v2 patch.

Thanks,
Neeraj
Francesco Dolcini Aug. 10, 2023, 6:35 p.m. UTC | #4
On Thu, Aug 10, 2023 at 06:02:32PM +0000, Neeraj sanjay kale wrote:
> Hi Francesco
> 
> Thank you for reviewing this patch.
> 
> > > --- a/drivers/bluetooth/btnxpuart.c
> > > +++ b/drivers/bluetooth/btnxpuart.c
> > ...
> > > @@ -547,7 +553,7 @@ static int nxp_download_firmware(struct hci_dev
> > *hdev)
> > >       serdev_device_set_flow_control(nxpdev->serdev, false);
> > >       nxpdev->current_baudrate = HCI_NXP_PRI_BAUDRATE;
> > >
> > > -     /* Wait till FW is downloaded and CTS becomes low */
> > > +     /* Wait till FW is downloaded */
> > >       err = wait_event_interruptible_timeout(nxpdev->fw_dnld_done_wait_q,
> > >                                              !test_bit(BTNXPUART_FW_DOWNLOADING,
> > >
> > > &nxpdev->tx_state), @@ -558,16 +564,11 @@ static int
> > nxp_download_firmware(struct hci_dev *hdev)
> > >       }
> > >
> > >       serdev_device_set_flow_control(nxpdev->serdev, true);
> > > -     err = serdev_device_wait_for_cts(nxpdev->serdev, 1, 60000);
> > > -     if (err < 0) {
> > > -             bt_dev_err(hdev, "CTS is still high. FW Download failed.");
> > > -             return err;
> > > -     }
> > this seems like an unrelated change, and it's moving from a 60secs timeout
> > polling CTS to nothing.
> > 
> > What's the reason for this? Should be this a separate commit with a proper
> > explanation?
> > 
> While working on integrating IW624 in btnxpuart driver, I observed that the
> first reset command was getting timed out, after FW download was complete 2
> out of 10 times. On further timing analysis, I noticed that this wait for CTS
> code did not actually help much, since CTS is already low after FW download,
> and becomes high after few more milli-seconds, and then low again after FW is
> initialized.  So it was either adding a "wait for CTS high" followed by "wait
> for CTS low", or simply increasing the sleep delay from 1000msec to 1200msec.
> I chose the later as it seemed more cleaner, and did the job perfectly, and
> tested all previously supported chipsets to make sure nothing is broke.  But
> you are right, I should add an explanation for this change in the commit
> message in the v2 patch.

This should be a separate commit, and probably it should have a fixes tag,
since this is solving a bug. I recently noted some bugs around this, I just did
not have the time to reproduce on the latest mainline kernel to report those.

One more question on this, what about the use case in which a combo firmware
is used and no firmware is loaded here? Will this use case be affected?

Francesco
Neeraj Sanjay Kale Aug. 11, 2023, 6:19 a.m. UTC | #5
Hi Francesco

> >
> > > > --- a/drivers/bluetooth/btnxpuart.c
> > > > +++ b/drivers/bluetooth/btnxpuart.c
> > > ...
> > > > @@ -547,7 +553,7 @@ static int nxp_download_firmware(struct
> > > > hci_dev
> > > *hdev)
> > > >       serdev_device_set_flow_control(nxpdev->serdev, false);
> > > >       nxpdev->current_baudrate = HCI_NXP_PRI_BAUDRATE;
> > > >
> > > > -     /* Wait till FW is downloaded and CTS becomes low */
> > > > +     /* Wait till FW is downloaded */
> > > >       err = wait_event_interruptible_timeout(nxpdev-
> >fw_dnld_done_wait_q,
> > > >
> > > > !test_bit(BTNXPUART_FW_DOWNLOADING,
> > > >
> > > > &nxpdev->tx_state), @@ -558,16 +564,11 @@ static int
> > > nxp_download_firmware(struct hci_dev *hdev)
> > > >       }
> > > >
> > > >       serdev_device_set_flow_control(nxpdev->serdev, true);
> > > > -     err = serdev_device_wait_for_cts(nxpdev->serdev, 1, 60000);
> > > > -     if (err < 0) {
> > > > -             bt_dev_err(hdev, "CTS is still high. FW Download failed.");
> > > > -             return err;
> > > > -     }
> > > this seems like an unrelated change, and it's moving from a 60secs
> > > timeout polling CTS to nothing.
> > >
> > > What's the reason for this? Should be this a separate commit with a
> > > proper explanation?
> > >
> > While working on integrating IW624 in btnxpuart driver, I observed
> > that the first reset command was getting timed out, after FW download
> > was complete 2 out of 10 times. On further timing analysis, I noticed
> > that this wait for CTS code did not actually help much, since CTS is
> > already low after FW download, and becomes high after few more
> > milli-seconds, and then low again after FW is initialized.  So it was
> > either adding a "wait for CTS high" followed by "wait for CTS low", or
> simply increasing the sleep delay from 1000msec to 1200msec.
> > I chose the later as it seemed more cleaner, and did the job
> > perfectly, and tested all previously supported chipsets to make sure
> > nothing is broke.  But you are right, I should add an explanation for
> > this change in the commit message in the v2 patch.
> 
> This should be a separate commit, and probably it should have a fixes tag,
> since this is solving a bug. I recently noted some bugs around this, I just did
> not have the time to reproduce on the latest mainline kernel to report those.
Sure I will revert this change and add the wait for CTS back. I will remove it later in a separate fixes patch.
Please do let us know if you encounter any issues here.

> 
> One more question on this, what about the use case in which a combo
> firmware is used and no firmware is loaded here? Will this use case be
> affected?
No in that case this part of code won't be executed.

In nxp_setup() -> nxp_check_boot_sign() waits for 1 second listening to any bootloader signatures from the chip.

If any bootloader signature is received, the driver performs this nxp_download_firmware()  routine.
If 1 second times out (which does in case of combo FW), it means FW is already running, and the driver proceeds with its initialization routine.

Thanks,
Neeraj
Francesco Dolcini Aug. 11, 2023, 7:38 a.m. UTC | #6
Hello Neeraj,

On Fri, Aug 11, 2023 at 06:19:12AM +0000, Neeraj sanjay kale wrote:
> > > > > --- a/drivers/bluetooth/btnxpuart.c
> > > > > +++ b/drivers/bluetooth/btnxpuart.c
> > > > ...
> > > > > @@ -547,7 +553,7 @@ static int nxp_download_firmware(struct
> > > > > hci_dev
> > > > *hdev)
> > > > >       serdev_device_set_flow_control(nxpdev->serdev, false);
> > > > >       nxpdev->current_baudrate = HCI_NXP_PRI_BAUDRATE;
> > > > >
> > > > > -     /* Wait till FW is downloaded and CTS becomes low */
> > > > > +     /* Wait till FW is downloaded */
> > > > >       err = wait_event_interruptible_timeout(nxpdev-
> > >fw_dnld_done_wait_q,
> > > > >
> > > > > !test_bit(BTNXPUART_FW_DOWNLOADING,
> > > > >
> > > > > &nxpdev->tx_state), @@ -558,16 +564,11 @@ static int
> > > > nxp_download_firmware(struct hci_dev *hdev)
> > > > >       }
> > > > >
> > > > >       serdev_device_set_flow_control(nxpdev->serdev, true);
> > > > > -     err = serdev_device_wait_for_cts(nxpdev->serdev, 1, 60000);
> > > > > -     if (err < 0) {
> > > > > -             bt_dev_err(hdev, "CTS is still high. FW Download failed.");
> > > > > -             return err;
> > > > > -     }
> > > > this seems like an unrelated change, and it's moving from a 60secs
> > > > timeout polling CTS to nothing.
> > > >
> > > > What's the reason for this? Should be this a separate commit with a
> > > > proper explanation?
> > > >
> > > While working on integrating IW624 in btnxpuart driver, I observed
> > > that the first reset command was getting timed out, after FW download
> > > was complete 2 out of 10 times. On further timing analysis, I noticed
> > > that this wait for CTS code did not actually help much, since CTS is
> > > already low after FW download, and becomes high after few more
> > > milli-seconds, and then low again after FW is initialized.  So it was
> > > either adding a "wait for CTS high" followed by "wait for CTS low", or
> > simply increasing the sleep delay from 1000msec to 1200msec.
> > > I chose the later as it seemed more cleaner, and did the job
> > > perfectly, and tested all previously supported chipsets to make sure
> > > nothing is broke.  But you are right, I should add an explanation for
> > > this change in the commit message in the v2 patch.
> > 
> > This should be a separate commit, and probably it should have a fixes tag,
> > since this is solving a bug. I recently noted some bugs around this, I just did
> > not have the time to reproduce on the latest mainline kernel to report those.
> Sure I will revert this change and add the wait for CTS back. I will
> remove it later in a separate fixes patch.  Please do let us know if
> you encounter any issues here.

I would probably do the other way around, first the fix, and then the
IW624 addition. You can just send a single series with both patches.

BTW: your email client is somehow messing up the email, you should do
something on that regards, it makes more difficult to reply to your
emails.

Francesco
diff mbox series

Patch

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index ee6f6c872a34..b42572ca63af 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -34,6 +34,8 @@ 
 #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_IW624		"nxp/uartiw624_bt.bin"
+#define FIRMWARE_SECURE_IW624	"nxp/uartiw624_bt.bin.se"
 #define FIRMWARE_AW693		"nxp/uartaw693_bt.bin"
 #define FIRMWARE_SECURE_AW693	"nxp/uartaw693_bt.bin.se"
 #define FIRMWARE_HELPER		"nxp/helper_uart_3000000.bin"
@@ -41,6 +43,8 @@ 
 #define CHIP_ID_W9098		0x5c03
 #define CHIP_ID_IW416		0x7201
 #define CHIP_ID_IW612		0x7601
+#define CHIP_ID_IW624a		0x8000
+#define CHIP_ID_IW624c		0x8001
 #define CHIP_ID_AW693		0x8200
 
 #define FW_SECURE_MASK		0xc0
@@ -152,6 +156,7 @@  struct btnxpuart_dev {
 	u32 fw_v1_sent_bytes;
 	u32 fw_v3_offset_correction;
 	u32 fw_v1_expected_len;
+	u32 boot_reg_offset;
 	wait_queue_head_t fw_dnld_done_wait_q;
 	wait_queue_head_t check_boot_sign_wait_q;
 
@@ -538,6 +543,7 @@  static int nxp_download_firmware(struct hci_dev *hdev)
 	nxpdev->fw_dnld_v1_offset = 0;
 	nxpdev->fw_v1_sent_bytes = 0;
 	nxpdev->fw_v1_expected_len = HDR_LEN;
+	nxpdev->boot_reg_offset = 0;
 	nxpdev->fw_v3_offset_correction = 0;
 	nxpdev->baudrate_changed = false;
 	nxpdev->timeout_changed = false;
@@ -547,7 +553,7 @@  static int nxp_download_firmware(struct hci_dev *hdev)
 	serdev_device_set_flow_control(nxpdev->serdev, false);
 	nxpdev->current_baudrate = HCI_NXP_PRI_BAUDRATE;
 
-	/* Wait till FW is downloaded and CTS becomes low */
+	/* Wait till FW is downloaded */
 	err = wait_event_interruptible_timeout(nxpdev->fw_dnld_done_wait_q,
 					       !test_bit(BTNXPUART_FW_DOWNLOADING,
 							 &nxpdev->tx_state),
@@ -558,16 +564,11 @@  static int nxp_download_firmware(struct hci_dev *hdev)
 	}
 
 	serdev_device_set_flow_control(nxpdev->serdev, true);
-	err = serdev_device_wait_for_cts(nxpdev->serdev, 1, 60000);
-	if (err < 0) {
-		bt_dev_err(hdev, "CTS is still high. FW Download failed.");
-		return err;
-	}
 	release_firmware(nxpdev->fw);
 	memset(nxpdev->fw_name, 0, sizeof(nxpdev->fw_name));
 
 	/* Allow the downloaded FW to initialize */
-	usleep_range(800 * USEC_PER_MSEC, 1 * USEC_PER_SEC);
+	msleep(1200);
 
 	return 0;
 }
@@ -591,6 +592,12 @@  static bool nxp_fw_change_baudrate(struct hci_dev *hdev, u16 req_len)
 	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
 	struct nxp_bootloader_cmd nxp_cmd5;
 	struct uart_config uart_config;
+	u32 clkdivaddr = CLKDIVADDR - nxpdev->boot_reg_offset;
+	u32 uartdivaddr = UARTDIVADDR - nxpdev->boot_reg_offset;
+	u32 uartmcraddr = UARTMCRADDR - nxpdev->boot_reg_offset;
+	u32 uartreinitaddr = UARTREINITADDR - nxpdev->boot_reg_offset;
+	u32 uarticraddr = UARTICRADDR - nxpdev->boot_reg_offset;
+	u32 uartfcraddr = UARTFCRADDR - nxpdev->boot_reg_offset;
 
 	if (req_len == sizeof(nxp_cmd5)) {
 		nxp_cmd5.header = __cpu_to_le32(5);
@@ -603,17 +610,17 @@  static bool nxp_fw_change_baudrate(struct hci_dev *hdev, u16 req_len)
 		serdev_device_write_buf(nxpdev->serdev, (u8 *)&nxp_cmd5, sizeof(nxp_cmd5));
 		nxpdev->fw_v3_offset_correction += req_len;
 	} else if (req_len == sizeof(uart_config)) {
-		uart_config.clkdiv.address = __cpu_to_le32(CLKDIVADDR);
+		uart_config.clkdiv.address = __cpu_to_le32(clkdivaddr);
 		uart_config.clkdiv.value = __cpu_to_le32(0x00c00000);
-		uart_config.uartdiv.address = __cpu_to_le32(UARTDIVADDR);
+		uart_config.uartdiv.address = __cpu_to_le32(uartdivaddr);
 		uart_config.uartdiv.value = __cpu_to_le32(1);
-		uart_config.mcr.address = __cpu_to_le32(UARTMCRADDR);
+		uart_config.mcr.address = __cpu_to_le32(uartmcraddr);
 		uart_config.mcr.value = __cpu_to_le32(MCR);
-		uart_config.re_init.address = __cpu_to_le32(UARTREINITADDR);
+		uart_config.re_init.address = __cpu_to_le32(uartreinitaddr);
 		uart_config.re_init.value = __cpu_to_le32(INIT);
-		uart_config.icr.address = __cpu_to_le32(UARTICRADDR);
+		uart_config.icr.address = __cpu_to_le32(uarticraddr);
 		uart_config.icr.value = __cpu_to_le32(ICR);
-		uart_config.fcr.address = __cpu_to_le32(UARTFCRADDR);
+		uart_config.fcr.address = __cpu_to_le32(uartfcraddr);
 		uart_config.fcr.value = __cpu_to_le32(FCR);
 		/* FW expects swapped CRC bytes */
 		uart_config.crc = __cpu_to_be32(crc32_be(0UL, (char *)&uart_config,
@@ -827,6 +834,7 @@  static int nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb)
 static char *nxp_get_fw_name_from_chipid(struct hci_dev *hdev, u16 chipid,
 					 u8 loader_ver)
 {
+	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
 	char *fw_name = NULL;
 
 	switch (chipid) {
@@ -839,6 +847,16 @@  static char *nxp_get_fw_name_from_chipid(struct hci_dev *hdev, u16 chipid,
 	case CHIP_ID_IW612:
 		fw_name = FIRMWARE_IW612;
 		break;
+	case CHIP_ID_IW624a:
+	case CHIP_ID_IW624c:
+		nxpdev->boot_reg_offset = 1;
+		if ((loader_ver & FW_SECURE_MASK) == FW_OPEN)
+			fw_name = FIRMWARE_IW624;
+		else if ((loader_ver & FW_SECURE_MASK) != FW_AUTH_ILLEGAL)
+			fw_name = FIRMWARE_SECURE_IW624;
+		else
+			bt_dev_err(hdev, "Illegal loader version %02x", loader_ver);
+		break;
 	case CHIP_ID_AW693:
 		if ((loader_ver & FW_SECURE_MASK) == FW_OPEN)
 			fw_name = FIRMWARE_AW693;