[v1,2/2] Bluetooth: btnxpuart: Fix nxp_setup

Commit Message

Marcel Ziswiler Oct. 18, 2023, 2:55 p.m. UTC

From: Marcel Ziswiler <[e-mail]>

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 <[e-mail]>

---
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(+)

Comments

Neeraj Sanjay Kale Oct. 18, 2023, 3:28 p.m. UTC | #1

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?

Thanks,
Neeraj

> From: Marcel Ziswiler <[e-mail]>
> Sent: Wednesday, October 18, 2023 8:26 PM
> To: [e-mail]
> Cc: Sherry Sun <[e-mail]>; Johan Hedberg
> <[e-mail]>; Luiz Augusto von Dentz
> <[e-mail]>; Neeraj sanjay kale <[e-mail]>;
> [e-mail]; Marcel Holtmann <[e-mail]>;
> Marcel Ziswiler <[e-mail]>; Amitkumar Karwar
> <[e-mail]>; Ilpo Järvinen <[e-mail]>
> Subject: [PATCH v1 2/2] Bluetooth: btnxpuart: Fix nxp_setup
> 
> From: Marcel Ziswiler <[e-mail]>
> 
> 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 <[e-mail]>
> 
> ---
> 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

Marcel Ziswiler Oct. 18, 2023, 4:41 p.m. UTC | #2

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 <[e-mail]>
> > Sent: Wednesday, October 18, 2023 8:26 PM
> > To: [e-mail]
> > Cc: Sherry Sun <[e-mail]>; Johan Hedberg
> > <[e-mail]>; Luiz Augusto von Dentz
> > <[e-mail]>; Neeraj sanjay kale <[e-mail]>;
> > [e-mail]; Marcel Holtmann <[e-mail]>;
> > Marcel Ziswiler <[e-mail]>; Amitkumar Karwar
> > <[e-mail]>; Ilpo Järvinen <[e-mail]>
> > Subject: [PATCH v1 2/2] Bluetooth: btnxpuart: Fix nxp_setup
> > 
> > From: Marcel Ziswiler <[e-mail]>
> > 
> > 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 <[e-mail]>
> > 
> > ---
> > 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

Neeraj Sanjay Kale Oct. 19, 2023, 9:58 a.m. UTC | #3

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 <[e-mail]>

Thanks,
Neeraj

> 
> > > From: Marcel Ziswiler <[e-mail]>
> > > Sent: Wednesday, October 18, 2023 8:26 PM
> > > To: [e-mail]
> > > Cc: Sherry Sun <[e-mail]>; Johan Hedberg
> > > <[e-mail]>; Luiz Augusto von Dentz
> > > <[e-mail]>; Neeraj sanjay kale
> > > <[e-mail]>; [e-mail]; Marcel
> > > Holtmann <[e-mail]>; Marcel Ziswiler
> > > <[e-mail]>; Amitkumar Karwar
> > > <[e-mail]>; Ilpo Järvinen
> > > <[e-mail]>
> > > Subject: [PATCH v1 2/2] Bluetooth: btnxpuart: Fix nxp_setup
> > >
> > > From: Marcel Ziswiler <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > >
> > > ---
> > > 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

Paul Menzel Oct. 19, 2023, 4:44 p.m. UTC | #4

Dear Marcel,


Thank you for your patch.

Am 18.10.23 um 16:55 schrieb Marcel Ziswiler:
> From: Marcel Ziswiler <[e-mail]>

“Fix nxp_setup” is really generic. A more specific message would be 
great. Maybe: Wait up to 10 s for firmware.

> 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 <[e-mail]>
> 
> ---
> 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.

I’d add that information to the commit message.

>   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);
>   	}

Isn’t there another way to find out, if the firmware is there? Adding an 
arbitrary timeout of ten seconds sounds like a fundamental flaw in the 
driver?


Kind regards,

Paul

Marcel Ziswiler Oct. 20, 2023, 7:56 a.m. UTC | #5

Hi Paul

On Thu, 2023-10-19 at 18:44 +0200, Paul Menzel wrote:
> Dear Marcel,
> 
> 
> Thank you for your patch.

Thanks for your feedback.

> Am 18.10.23 um 16:55 schrieb Marcel Ziswiler:
> > From: Marcel Ziswiler <[e-mail]>
> 
> “Fix nxp_setup” is really generic. A more specific message would be 
> great. Maybe: Wait up to 10 s for firmware.

Okay, I can do that.

I admit, I had a hard time finding a real good description as the whole thing is rather completely flawed.

> > 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 <[e-mail]>
> > 
> > ---
> > 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.
> 
> I’d add that information to the commit message.

Okay, will do.

> >   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);
> >   	}
> 
> Isn’t there another way to find out, if the firmware is there? Adding an 
> arbitrary timeout of ten seconds sounds like a fundamental flaw in the 
> driver?

Yes, exactly. You might have read my previous message [1]. The whole driver/firmware, well the entire thing is
basically rather flawed. But then I am just trying to incrementally make it at least half-way usable for our
use case as well.

[1] https://lore.kernel.org/all/[e-mail]

> Kind regards,
> 
> Paul

Cheers

Marcel

Sherry Sun Oct. 20, 2023, 10:39 a.m. UTC | #6

> -----Original Message-----
> From: Marcel Ziswiler <[e-mail]>
> Sent: 2023年10月18日 22:56
> To: [e-mail]
> Cc: Sherry Sun <[e-mail]>; Johan Hedberg
> <[e-mail]>; Luiz Augusto von Dentz
> <[e-mail]>; Neeraj sanjay kale <[e-mail]>;
> [e-mail]; Marcel Holtmann <[e-mail]>;
> Marcel Ziswiler <[e-mail]>; Amitkumar Karwar
> <[e-mail]>; Ilpo Järvinen <[e-mail]>
> Subject: [PATCH v1 2/2] Bluetooth: btnxpuart: Fix nxp_setup
> 
> From: Marcel Ziswiler <[e-mail]>
> 
> 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 <[e-mail]>
> 
> ---
> 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).

Hi Marcel,

Please remove the description of "over SDIO (mwifiex_sdio)" here, because the Wi-Fi driver corresponding to the NXP BT chip not only supports the SDIO interface, but also supports the PCIe bus.
That is to say, combo firmware can be loaded via the SDIO or PCIe bus.

Best Regards
Sherry

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

Neeraj Sanjay Kale Jan. 17, 2024, 5:18 a.m. UTC | #7

Hi Marcel,

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

I have sent a new patch with subject: "[RFC PATCH] Bluetooth: btnxpuart: Fix nxp_setup in case chip is powered on late"

I would very much appreciate if you could have a look at the patch and let me know if it solves the above issues you mentioned in a neat way.

As per the new change, if no bootloader signatures are read, and CTS is high, the setup function returns an error, preventing HCI intializations.
After every 5 seconds, we attempt a power-on, which results in calling the setup function again to check for bootloader signatures and CTS line.
This "polling" sequence will continue as long as the module stays inserted, or until the btnxpuart driver downloads BT-only FW or the mwifiex downloads the combo FW (or both modules download BT-only/Wi-Fi only respective FWs).

This handling is supposed to come into picture only when mwifiex pwrseq controls the shared PDn pin of the chip, while the normal setup sequence should work as it did earlier for others.

Thanks,
Neeraj

Patch

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);
 	}