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

[Francesco Dolcini]

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@xxxxxxxxxxxx/
> 
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@xxxxxxx>
> Reported-by: Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx>
> Closes: https://patchwork.kernel.org/project/bluetooth/patch/20231018145540.34014-3-marcel@xxxxxxxxxxxx/
> ---
>  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