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

[Neeraj sanjay kale]

Hi Marcel

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

I have tested this patch on my setup, and the addition of CTS check is useful,
as it may not seem to be a good idea to rely on bootloader signatures to determine
if FW is running.
I have also verified that this patch does not break any existing functionality (for other customers).

This seem good to me.

Reviewed-by: Neeraj Sanjay Kale <neeraj.sanjaykale@xxxxxxx>

Thanks,
Neeraj

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