| Message ID | 20250327182523.524534-1-neeraj.sanjaykale@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] Bluetooth: btnxpuart: Add msleep() after changing the baudrate | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| 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/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 | fail | TestRunner_mgmt-tester: Total: 490, Passed: 483 (98.6%), Failed: 3, Not Run: 4 |
| 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 |
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=947856 ---Test result--- Test Summary: CheckPatch PENDING 0.36 seconds GitLint PENDING 0.24 seconds SubjectPrefix PASS 0.22 seconds BuildKernel PASS 24.08 seconds CheckAllWarning PASS 26.46 seconds CheckSparse PASS 30.51 seconds BuildKernel32 PASS 23.99 seconds TestRunnerSetup PASS 426.74 seconds TestRunner_l2cap-tester PASS 21.02 seconds TestRunner_iso-tester PASS 32.10 seconds TestRunner_bnep-tester PASS 4.73 seconds TestRunner_mgmt-tester FAIL 118.29 seconds TestRunner_rfcomm-tester PASS 7.81 seconds TestRunner_sco-tester PASS 12.50 seconds TestRunner_ioctl-tester PASS 8.26 seconds TestRunner_mesh-tester PASS 5.91 seconds TestRunner_smp-tester PASS 7.18 seconds TestRunner_userchan-tester PASS 4.93 seconds IncrementalBuild PENDING 0.63 seconds Details ############################## Test: CheckPatch - PENDING Desc: Run checkpatch.pl script Output: ############################## Test: GitLint - PENDING Desc: Run gitlint Output: ############################## Test: TestRunner_mgmt-tester - FAIL Desc: Run mgmt-tester with test-runner Output: Total: 490, Passed: 483 (98.6%), Failed: 3, Not Run: 4 Failed Test Cases LL Privacy - Set Flags 1 (Add to RL) Failed 0.130 seconds LL Privacy - Set Flags 2 (Enable RL) Failed 0.139 seconds LL Privacy - Start Discovery 2 (Disable RL) Failed 0.159 seconds ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth
Dear Neeraj, Thank you for your patch. For the summary/title, I suggest: Sleep 100 ms after baud rate change Am 27.03.25 um 19:25 schrieb Neeraj Sanjay Kale: > This adds a 100 millisec sleep after change baudrate vendor command. > > It is observed that when the baudrate change command changes the > baudrate from 3000000 to 115200, any immediate HCI command returns an > error -22 (Device Busy). Really 3 million? Is this happening with every change, or only decreasing the baud rate with the values you listed? > Adding a small delay after the change baudrate command complete event is > received helps fix the issue. 100 ms is not small to me. Is this issue documented in the hardware documentation? Are there other ways like polling, if the command succeeded? > Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com> > --- > drivers/bluetooth/btnxpuart.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c > index 5091dea762a0..e26fabe8fb3d 100644 > --- a/drivers/bluetooth/btnxpuart.c > +++ b/drivers/bluetooth/btnxpuart.c > @@ -1238,6 +1238,8 @@ static int nxp_set_baudrate_cmd(struct hci_dev *hdev, void *data) > if (*status == 0) { > serdev_device_set_baudrate(nxpdev->serdev, nxpdev->new_baudrate); > nxpdev->current_baudrate = nxpdev->new_baudrate; > + /* Allow sufficiant time for chip to switch to new baudrate */ Please add the datasheet section. sufficient > + sleep(100); > } > bt_dev_dbg(hdev, "Set baudrate response: status=%d, baudrate=%d", > *status, nxpdev->new_baudrate); Kind regards, Paul
> -----Original Message----- > From: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com> > Sent: Friday, March 28, 2025 2:25 AM > To: marcel@holtmann.org; luiz.dentz@gmail.com > Cc: linux-bluetooth@vger.kernel.org; linux-kernel@vger.kernel.org; > Amitkumar Karwar <amitkumar.karwar@nxp.com>; Neeraj Sanjay Kale > <neeraj.sanjaykale@nxp.com>; Sherry Sun <sherry.sun@nxp.com> > Subject: [PATCH v1 1/2] Bluetooth: btnxpuart: Add msleep() after changing the > baudrate > > This adds a 100 millisec sleep after change baudrate vendor command. > > It is observed that when the baudrate change command changes the > baudrate from 3000000 to 115200, any immediate HCI command returns an > error -22 (Device Busy). > > Adding a small delay after the change baudrate command complete event is > received helps fix the issue. > > Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com> > --- > drivers/bluetooth/btnxpuart.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c > index 5091dea762a0..e26fabe8fb3d 100644 > --- a/drivers/bluetooth/btnxpuart.c > +++ b/drivers/bluetooth/btnxpuart.c > @@ -1238,6 +1238,8 @@ static int nxp_set_baudrate_cmd(struct hci_dev > *hdev, void *data) > if (*status == 0) { > serdev_device_set_baudrate(nxpdev->serdev, > nxpdev->new_baudrate); > nxpdev->current_baudrate = nxpdev->new_baudrate; > + /* Allow sufficiant time for chip to switch to new > baudrate */ > + sleep(100); Hi Neeraj, Assuming that msleep() should be used here, sleep (100) means 100 seconds of sleep, too crazy :) Best Regards Sherry > } > bt_dev_dbg(hdev, "Set baudrate response: status=%d, > baudrate=%d", > *status, nxpdev->new_baudrate); > -- > 2.25.1
Hi Paul, Thank you for reviewing the patch. > > Dear Neeraj, > > > Thank you for your patch. For the summary/title, I suggest: > > Sleep 100 ms after baud rate change I will update this in V2 patch. > > Am 27.03.25 um 19:25 schrieb Neeraj Sanjay Kale: > > This adds a 100 millisec sleep after change baudrate vendor command. > > > > It is observed that when the baudrate change command changes the > > baudrate from 3000000 to 115200, any immediate HCI command returns an > > error -22 (Device Busy). Correction. It returns error -110 (Command Timeout) > > Really 3 million? NXP chips mainly use these 2 baudrates: #define HCI_NXP_PRI_BAUDRATE 115200 #define HCI_NXP_SEC_BAUDRATE 3000000 > > Is this happening with every change, or only decreasing the baud rate with > the values you listed? Let me share some background first: After FW download, the FW initializes at 115200 baudrate. Initial implementation of BTNXPUART changed the baudrate from 115200 to 3000000 in nxp_setup() after modprobe. During rmmod, driver reverted the baudrate back to 115200. However, since after modprobe hci0 is DOWN, the change baudrate command failed during rmmod. It succeeded if hci0 is already UP and RUNNING, thus next modprobe command would also succeed. This logic made sure BTNXPUART and controller are in sync even if FW is already present in the controller, but rmmod when hci0 is down failed, leaving the controller running at 3000000 baudrate. In next iteration modprobe command would eventually fail. This problem was fixed in: https://github.com/bluez/bluetooth-next/commit/6fca6781d19dfadbc3d96b3c10daf1f2e1239092 (Merged) Which had a change in nxp_shutdown() function picked up from: https://patchwork.kernel.org/project/bluetooth/patch/20250226151340.1017790-1-loic.poulain@linaro.org/ (Not merged, dropped) With this change, the driver changed the baudrate from 115200 to 3000000 on modprobe in nxp_post_init(). After the initialization, when hci0 became DOWN, nxp_shutdown() changed the baudrate back to 115200. Later, when hci0 becomes UP, nxp_post_init() changed it back to 3000000. It fixed the rmmod problem when hci0 is DOWN. However, this logic introduced a new issue with "hciconfig hci0 reset" command, which makes hci0 DOWN and UP immediately. So, 1) modprobe btnxpuart (FW download, change baudrate from 115200 to 3000000) 2) After HCI init done, hci0 interface is down (Baudrate switches back to 115200) 3) hciconfig hci0 up (Baudrate switches back to 3000000) 4) hciconfig hci0 reset (hci0 down sets baudrate to 115200, first reset command in hci0 UP times out) On Logic Analyzer, we see 3 millisec gap between the *change baudrate command complete event* from controller, and *HCI_RESET* command from host at 115200 baudrate. This HCI_RESET command is missed by the controller as it is not completely switched to 115200 baudrate. This is a completely new scenario, which never occurred before above-mentioned commit was merged. With some trial and error, I was able to get the hciconfig reset command working if the gap between *change baudrate command complete event* and first *HCI_RESET* command was increased to 50 millisec. The reason for choosing 100 millisec was to provide sufficient buffer time. Now that I'm thinking, yes it seems reasonably high. And yes, the chip should not take such a long time to change baudrate. Also, this issue is not observed with "hciconfig hci0 up" or "bluetoothctl power on" commands. > > > Adding a small delay after the change baudrate command complete event > > is received helps fix the issue. > > 100 ms is not small to me. Is this issue documented in the hardware > documentation? Are there other ways like polling, if the command > succeeded? I could not find such a reference but looks like this issue is specific to *hciconfig hci0 reset* command only. Today I ran these 2 commands in a loop without delay, and did not see any timeout errors: 1) hcitool -i hci0 cmd 3f 09 00 c2 01 00 // where 0x00012c00 => 115200 2) hcitool -i hci0 cmd 3f 09 c0 c6 2d 00 // where 0x002dc6c0 => 3000000 Although hciconfig and hcitool commands are deprecated, we'd want these commands to work fine. Thank you for bearing with my extensive response. Any alternatives/suggestion are welcome. Regards, Neeraj
diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c index 5091dea762a0..e26fabe8fb3d 100644 --- a/drivers/bluetooth/btnxpuart.c +++ b/drivers/bluetooth/btnxpuart.c @@ -1238,6 +1238,8 @@ static int nxp_set_baudrate_cmd(struct hci_dev *hdev, void *data) if (*status == 0) { serdev_device_set_baudrate(nxpdev->serdev, nxpdev->new_baudrate); nxpdev->current_baudrate = nxpdev->new_baudrate; + /* Allow sufficiant time for chip to switch to new baudrate */ + sleep(100); } bt_dev_dbg(hdev, "Set baudrate response: status=%d, baudrate=%d", *status, nxpdev->new_baudrate);
This adds a 100 millisec sleep after change baudrate vendor command. It is observed that when the baudrate change command changes the baudrate from 3000000 to 115200, any immediate HCI command returns an error -22 (Device Busy). Adding a small delay after the change baudrate command complete event is received helps fix the issue. Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com> --- drivers/bluetooth/btnxpuart.c | 2 ++ 1 file changed, 2 insertions(+)