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 <marcel.ziswiler@xxxxxxxxxxx>
>
> “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 <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.
>
> 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/dca8bc7fec5f527cac2e280cd8ed4edae1f473ea.camel@xxxxxxxxxxx
> Kind regards,
>
> Paul
Cheers
Marcel
