Re: [PATCH v1] Bluetooth: btnxpuart: Add support for IW624 chipset

[Neeraj sanjay kale]

Hi Francesco

> >
> > > > --- a/drivers/bluetooth/btnxpuart.c
> > > > +++ b/drivers/bluetooth/btnxpuart.c
> > > ...
> > > > @@ -547,7 +553,7 @@ static int nxp_download_firmware(struct
> > > > hci_dev
> > > *hdev)
> > > >       serdev_device_set_flow_control(nxpdev->serdev, false);
> > > >       nxpdev->current_baudrate = HCI_NXP_PRI_BAUDRATE;
> > > >
> > > > -     /* Wait till FW is downloaded and CTS becomes low */
> > > > +     /* Wait till FW is downloaded */
> > > >       err = wait_event_interruptible_timeout(nxpdev-
> >fw_dnld_done_wait_q,
> > > >
> > > > !test_bit(BTNXPUART_FW_DOWNLOADING,
> > > >
> > > > &nxpdev->tx_state), @@ -558,16 +564,11 @@ static int
> > > nxp_download_firmware(struct hci_dev *hdev)
> > > >       }
> > > >
> > > >       serdev_device_set_flow_control(nxpdev->serdev, true);
> > > > -     err = serdev_device_wait_for_cts(nxpdev->serdev, 1, 60000);
> > > > -     if (err < 0) {
> > > > -             bt_dev_err(hdev, "CTS is still high. FW Download failed.");
> > > > -             return err;
> > > > -     }
> > > this seems like an unrelated change, and it's moving from a 60secs
> > > timeout polling CTS to nothing.
> > >
> > > What's the reason for this? Should be this a separate commit with a
> > > proper explanation?
> > >
> > While working on integrating IW624 in btnxpuart driver, I observed
> > that the first reset command was getting timed out, after FW download
> > was complete 2 out of 10 times. On further timing analysis, I noticed
> > that this wait for CTS code did not actually help much, since CTS is
> > already low after FW download, and becomes high after few more
> > milli-seconds, and then low again after FW is initialized.  So it was
> > either adding a "wait for CTS high" followed by "wait for CTS low", or
> simply increasing the sleep delay from 1000msec to 1200msec.
> > I chose the later as it seemed more cleaner, and did the job
> > perfectly, and tested all previously supported chipsets to make sure
> > nothing is broke.  But you are right, I should add an explanation for
> > this change in the commit message in the v2 patch.
> 
> This should be a separate commit, and probably it should have a fixes tag,
> since this is solving a bug. I recently noted some bugs around this, I just did
> not have the time to reproduce on the latest mainline kernel to report those.
Sure I will revert this change and add the wait for CTS back. I will remove it later in a separate fixes patch.
Please do let us know if you encounter any issues here.

> 
> One more question on this, what about the use case in which a combo
> firmware is used and no firmware is loaded here? Will this use case be
> affected?
No in that case this part of code won't be executed.

In nxp_setup() -> nxp_check_boot_sign() waits for 1 second listening to any bootloader signatures from the chip.

If any bootloader signature is received, the driver performs this nxp_download_firmware()  routine.
If 1 second times out (which does in case of combo FW), it means FW is already running, and the driver proceeds with its initialization routine.

Thanks,
Neeraj