Hi Francesco
Thank you for reviewing this patch.
> > --- 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.
Thanks,
Neeraj
On Thu, Aug 10, 2023 at 06:02:32PM +0000, Neeraj sanjay kale wrote:
> Hi Francesco
>
> Thank you for reviewing this patch.
>
> > > --- 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.
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?
Francesco
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
Hello Neeraj,
On Fri, Aug 11, 2023 at 06:19:12AM +0000, Neeraj sanjay kale wrote:
> > > > > --- 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.
I would probably do the other way around, first the fix, and then the
IW624 addition. You can just send a single series with both patches.
BTW: your email client is somehow messing up the email, you should do
something on that regards, it makes more difficult to reply to your
emails.
Francesco