| 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 |
| 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 |
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
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
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
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
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
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 --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;
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(-)