[PATCH v1 1/2] Bluetooth: btnxpuart: Add msleep() after changing the baudrate

[PATCH v1 1/2] Bluetooth: btnxpuart: Add msleep() after changing the baudrate

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

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);
 		}
 		bt_dev_dbg(hdev, "Set baudrate response: status=%d, baudrate=%d",
 			   *status, nxpdev->new_baudrate);
-- 
2.25.1

[PATCH v1 2/2] Bluetooth: btnxpuart: Add an error message if FW dump trigger fails

[Neeraj Sanjay Kale]

This prints an error message if the FW Dump trigger command fails. This
scenario is mainly observed in legacy chipsets 8987 and 8997 and also
IW416, where this feature is unavailable due to memory constraints.

Fixes: 998e447f443f ("Bluetooth: btnxpuart: Add support for HCI coredump feature")
Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
---
 drivers/bluetooth/btnxpuart.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
index e26fabe8fb3d..57c88a1cb660 100644
--- a/drivers/bluetooth/btnxpuart.c
+++ b/drivers/bluetooth/btnxpuart.c
@@ -1288,7 +1288,9 @@ static void nxp_coredump(struct hci_dev *hdev)
 	u8 pcmd = 2;
 
 	skb = nxp_drv_send_cmd(hdev, HCI_NXP_TRIGGER_DUMP, 1, &pcmd);
-	if (!IS_ERR(skb))
+	if (IS_ERR(skb))
+		bt_dev_err(hdev, "Failed to trigger FW Dump. (%ld)", PTR_ERR(skb));
+	else
 		kfree_skb(skb);
 }
 
-- 
2.25.1

Re: [PATCH v1 1/2] Bluetooth: btnxpuart: Add msleep() after changing the baudrate

[Paul Menzel]

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

Re: [PATCH v1 1/2] Bluetooth: btnxpuart: Add msleep() after changing the baudrate

[Neeraj Sanjay Kale]

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

RE: [PATCH v1 1/2] Bluetooth: btnxpuart: Add msleep() after changing the baudrate

[Sherry Sun]

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

Re: [PATCH v1 1/2] Bluetooth: btnxpuart: Add msleep() after changing the baudrate

[patchwork-bot+bluetooth]

Hello:

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

On Thu, 27 Mar 2025 23:55:22 +0530 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [v1,1/2] Bluetooth: btnxpuart: Add msleep() after changing the baudrate
    (no matching commit)
  - [v1,2/2] Bluetooth: btnxpuart: Add an error message if FW dump trigger fails
    https://git.kernel.org/bluetooth/bluetooth-next/c/061e4972c48c

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

Re: [PATCH v1 1/2] Bluetooth: btnxpuart: Add msleep() after changing the baudrate

[Neeraj Sanjay Kale]

Hi Luiz,

Please do not merge this patch series.

I have sent out 2 different patches instead titled:
1) [PATCH v2] Bluetooth: btnxpuart: Add an error message if FW dump trigger fails
2) [PATCH v1] Bluetooth: btnxpuart: Revert baudrate change in nxp_shutdown

Thanks,
Neeraj


>
> Hello:
>
> This series was applied to bluetooth/bluetooth-next.git (master) by Luiz
> Augusto von Dentz <luiz.von.dentz@intel.com>:
>
> On Thu, 27 Mar 2025 23:55:22 +0530 you wrote:
> > 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.
> >
> > [...]
>
> Here is the summary with links:
>   - [v1,1/2] Bluetooth: btnxpuart: Add msleep() after changing the baudrate
>     (no matching commit)
>   - [v1,2/2] Bluetooth: btnxpuart: Add an error message if FW dump trigger
> fails
>
> https://git.kern/
> el.org%2Fbluetooth%2Fbluetooth-
> next%2Fc%2F061e4972c48c&data=05%7C02%7Cneeraj.sanjaykale%40nxp.co
> m%7C08b8a55d25e04b812fd008dd72ecea34%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C638793084075362630%7CUnknown%7CTWFpbGZsb
> 3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkF
> OIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=8u4Uw6T820eP6
> mAFNCXU1xuUFpXYUB32nrYKuj8th%2BQ%3D&reserved=0
>
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.do/
> cs.kernel.org%2Fpatchwork%2Fpwbot.html&data=05%7C02%7Cneeraj.sanjay
> kale%40nxp.com%7C08b8a55d25e04b812fd008dd72ecea34%7C686ea1d3bc2
> b4c6fa92cd99c5c301635%7C0%7C0%7C638793084075397504%7CUnknown%
> 7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOi
> JXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=th
> Hgu6Dw8RWjMWGzjv7C%2F4JPJzBwwC7RzDUmor8bq4M%3D&reserved=0
>