Re: [RFC PATCH] Bluetooth: btnxpuart: Fix nxp_setup in case chip is powered on late

Francesco Dolcini
2024-01-17T09:09:46Z

On Wed, Jan 17, 2024 at 08:35:01AM +0530, Neeraj Sanjay Kale wrote:
> This adds a setup retry mechanism in case the chip is powered on after the
> btnxpuart driver is loaded.
>
> The NXP chipsets have a common PDn pin shared between Wi-Fi and Bluetooth.
>
> When customers use mwifiex_sdio drivers for Wi-Fi, the pwrseq tied to the
> driver toggles the GPIO connected to the chip's PDn pin, powering it on.
>
> The btnxpuart driver is loaded before mwifiex, and the setup function does
> not receive any bootloader signature, as PDn is held low at this moment.
> The driver inadvertently assumes that FW is already running on the chip.
>
> The nxp_setup exits with a success, and BT subsystem proceeds with sending
> initialization commands, which result in a timeout.
> [ 284.588177] Bluetooth: hci0: Opcode 0x0c03 failed: -110
> [ 286.636167] Bluetooth: hci0: Setting wake-up method failed (-110)
>
> Later when mwifiex is loaded, the pwrseq makes PDn pin high, and downloads
> either WiFi or combo FW.
>
> However, the btnxpuart is in a bad state, and re-loading btnxpuart module
> does not help.
>
> This fix adds a check for CTS pin, in case no bootloader signatures are
> received. If CTS is high, it means that the chip is currently powered off,
> and nxp_setup will return an error, preventing any HCI initialization
> commands to be sent by the BT subsystem.
>
> The driver attempts to check for bootloader signatures and CTS again, by
> scheduling the hci power_on work after every 5 seconds, as long as the
> btnxpuart module is inserted in the kernel.
>
> This fix attempts to improvise the fix provided my Marcel Ziswiler and
> handle this scenario gracefully.
> https://patchwork.kernel.org/project/bluetooth/patch/20231018145540.34014-3-marcel@ziswiler.com/
>
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> Reported-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Closes: https://patchwork.kernel.org/project/bluetooth/patch/20231018145540.34014-3-marcel@ziswiler.com/
> ---
> drivers/bluetooth/btnxpuart.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> index 7f88b6f52f26..20a3a5bd5529 100644
> --- a/drivers/bluetooth/btnxpuart.c
> +++ b/drivers/bluetooth/btnxpuart.c
> @@ -1036,6 +1048,13 @@ static int nxp_setup(struct hci_dev *hdev)
> err = nxp_download_firmware(hdev);
> if (err < 0)
> return err;
> + } else if (!serdev_device_get_cts(nxpdev->serdev)) {
> + /* CTS is high and no bootloader signatures detected */
> + bt_dev_dbg(hdev, "Controller not detected. Will check again in %d msec",
> + NXP_SETUP_RETRY_TIME_MS);
> + schedule_delayed_work(&nxpdev->setup_retry_work,
> + msecs_to_jiffies(NXP_SETUP_RETRY_TIME_MS));
> + return -ENODEV;
why not just

return -EPROBE_DEFER;

and remove everything else, no need for any kind of retry or delayed work
if the driver core takes care of it.

Francesco