Re: [PATCH v1 2/2] Bluetooth: btnxpuart: Fix nxp_setup

[Marcel Ziswiler]

Hi Neeraj

On Wed, 2023-10-18 at 15:28 +0000, Neeraj sanjay kale wrote:
> Hi Marcel,
> 
> Thank you for your patch. This change looks good to me.
> 
> I think the scenario you are testing/resolving is:
> 1) Load btnxpuart.ko first (which "may" load BT-only FW if chip is powered on)
> 2) Power cycle or power on chip
> 3) Load WLAN driver with combo FW
> Right?

Yes, kinda, but there are really a slew of issues with this driver or the combination of it with mwifiex_sdio:

1. Shared power-down pin (PD#) between Bluetooth and Wi-Fi
Issue: There is currently no concept in the Linux kernel to achieve this. Last failed attempt [1].
Workaround: Instead of using mmc-pwrseq tied to the Wi-Fi driver (mwifiex_sdio) this could be hogged at boot.
However, then it may no longer easily be controlled e.g. for a (manual) power-cycle.
A novel approach might be using a regulator which could be shared, however, this would require us implementing
regulator support in btnxpuart. Not sure whether you would actually approve us doing so.

2. Bluetooth driver (btnxpuart) vs. Wi-Fi driver (mwifiex) load order
Issue: By default, the Bluetooth driver (btnxpuart) loads before the Wi-Fi driver (mwifiex) and using regular
mmc-pwrseq for mwifiex_sdio will only power-on the module late.
Workaround: The install directive in modprobe.conf could be (ab-)used to enforce mwifiex/mwifiex_sdio to be
loaded first. However, doing so adds an unnecessary dependency with user space and is in general discouraged.

3. Distinguish powered-on vs. powered-off state
Issue: Without that knowledge the driver has a hard time figuring out whether or not it needs to load the
firmware as only if it is powered-on (and/or without firmware) will the Bluetooth part of the module send its
boot signature. So the absence of such boot signature may mean either firmware is already loaded or module
powered-off.
Workaround: The Bluetooth UART RTS pin seems to activate an internal pull-up upon the module being powered on.
However, programmatically it is rather hard to determine this as the regular UART driver (now driving RTS)
usually gets loaded.

4. Determine whether or not to load the firmware
Issue: If it is without firmware (and powered-on) the boot loader will send its boot signature. If there is no
boot signature it could as well also still be powered-off.
Workaround: Also check CTS as it goes up if the firmware is loaded. Unfortunately, integrating this in
btnxpuart is not so trivial as serdev introduces its own asynchronous concurrency which is already used in
existing checks.

5. Deploy separate firmware
Issue: Does not really solve anything resp. as the power-down pin is shared this seems kinda pointless.

Your input on any of those topics is much appreciated.

Thanks!

> Thanks,
> Neeraj

Cheers

Marcel

> > From: Marcel Ziswiler <marcel@xxxxxxxxxxxx>
> > Sent: Wednesday, October 18, 2023 8:26 PM
> > To: linux-bluetooth@xxxxxxxxxxxxxxx
> > Cc: Sherry Sun <sherry.sun@xxxxxxx>; Johan Hedberg
> > <johan.hedberg@xxxxxxxxx>; Luiz Augusto von Dentz
> > <luiz.dentz@xxxxxxxxx>; Neeraj sanjay kale <neeraj.sanjaykale@xxxxxxx>;
> > linux-kernel@xxxxxxxxxxxxxxx; Marcel Holtmann <marcel@xxxxxxxxxxxx>;
> > Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx>; Amitkumar Karwar
> > <amitkumar.karwar@xxxxxxx>; Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > Subject: [PATCH v1 2/2] Bluetooth: btnxpuart: Fix nxp_setup
> > 
> > From: Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx>
> > 
> > Unfortunately, nxp_setup() may inadvertently assume that the firmware is
> > already running while the module is not even powered yet.
> > Fix this by waiting up to 10 seconds for the CTS to go up as the combo
> > firmware might be loaded by the Wi-Fi driver over SDIO (mwifiex_sdio).
> > 
> > Fixes: 689ca16e5232 ("Bluetooth: NXP: Add protocol support for NXP
> > Bluetooth chipsets")
> > 
> > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx>
> > 
> > ---
> > This is what may happen without this fix:
> > [  284.588177] Bluetooth: hci0: Opcode 0x0c03 failed: -110 [  286.636167]
> > Bluetooth: hci0: Setting wake-up method failed (-110) Unfortunately, even
> > re-loading the btnxpuart kernel module would not recover from this
> > condition.
> > 
> >  drivers/bluetooth/btnxpuart.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
> > index 9cb7529eef09..4b83a0aa3459 100644
> > --- a/drivers/bluetooth/btnxpuart.c
> > +++ b/drivers/bluetooth/btnxpuart.c
> > @@ -1021,6 +1021,16 @@ static int nxp_setup(struct hci_dev *hdev)
> >                 if (err < 0)
> >                         return err;
> >         } else {
> > +               /* The combo firmware might be loaded by the Wi-Fi driver over
> > SDIO (mwifiex_sdio).
> > +                * We wait up to 10s for the CTS to go up. Afterwards, we know that
> > the firmware is
> > +                * really ready.
> > +                */
> > +               err = serdev_device_wait_for_cts(nxpdev->serdev, true, 10000);
> > +               if (err) {
> > +                       bt_dev_err(nxpdev->hdev, "Wait for CTS failed with %d", err);
> > +                       return err;
> > +               }
> > +
> >                 bt_dev_dbg(hdev, "FW already running.");
> >                 clear_bit(BTNXPUART_FW_DOWNLOADING, &nxpdev->tx_state);
> >         }
> > --
> > 2.36.1