From: Marcel Ziswiler <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
> Sent: Wednesday, October 18, 2023 8:26 PM
> To: [email protected]
> Cc: Sherry Sun <[email protected]>; Johan Hedberg
> <[email protected]>; Luiz Augusto von Dentz
> <[email protected]>; Neeraj sanjay kale <[email protected]>;
> [email protected]; Marcel Holtmann <[email protected]>;
> Marcel Ziswiler <[email protected]>; Amitkumar Karwar
> <[email protected]>; Ilpo J?rvinen <[email protected]>
> Subject: [PATCH v1 2/2] Bluetooth: btnxpuart: Fix nxp_setup
>
> From: Marcel Ziswiler <[email protected]>
>
> 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 <[email protected]>
>
> ---
> 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
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 <[email protected]>
Thanks,
Neeraj
>
> > > From: Marcel Ziswiler <[email protected]>
> > > Sent: Wednesday, October 18, 2023 8:26 PM
> > > To: [email protected]
> > > Cc: Sherry Sun <[email protected]>; Johan Hedberg
> > > <[email protected]>; Luiz Augusto von Dentz
> > > <[email protected]>; Neeraj sanjay kale
> > > <[email protected]>; [email protected]; Marcel
> > > Holtmann <[email protected]>; Marcel Ziswiler
> > > <[email protected]>; Amitkumar Karwar
> > > <[email protected]>; Ilpo Järvinen
> > > <[email protected]>
> > > Subject: [PATCH v1 2/2] Bluetooth: btnxpuart: Fix nxp_setup
> > >
> > > From: Marcel Ziswiler <[email protected]>
> > >
> > > 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 <[email protected]>
> > >
> > > ---
> > > 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
Dear Marcel,
Thank you for your patch.
Am 18.10.23 um 16:55 schrieb Marcel Ziswiler:
> From: Marcel Ziswiler <[email protected]>
“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 <[email protected]>
>
> ---
> 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
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