From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1EECBC001DE for ; Thu, 10 Aug 2023 18:35:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235478AbjHJSfk (ORCPT ); Thu, 10 Aug 2023 14:35:40 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60090 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229677AbjHJSfi (ORCPT ); Thu, 10 Aug 2023 14:35:38 -0400 Received: from mail11.truemail.it (mail11.truemail.it [217.194.8.81]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D69792715 for ; Thu, 10 Aug 2023 11:35:35 -0700 (PDT) Received: from gaggiata.pivistrello.it (93-49-2-63.ip317.fastwebnet.it [93.49.2.63]) by mail11.truemail.it (Postfix) with ESMTPA id BEA95207DB; Thu, 10 Aug 2023 20:35:33 +0200 (CEST) Received: from livingston (unknown [192.168.42.11]) by gaggiata.pivistrello.it (Postfix) with ESMTP id 5C2F87FBA0; Thu, 10 Aug 2023 20:35:33 +0200 (CEST) Received: from pivi by livingston with local (Exim 4.96) (envelope-from ) id 1qUAVR-0001Go-0f; Thu, 10 Aug 2023 20:35:33 +0200 Date: Thu, 10 Aug 2023 20:35:33 +0200 From: Francesco Dolcini To: Neeraj sanjay kale Cc: Francesco Dolcini , "marcel@holtmann.org" , "johan.hedberg@gmail.com" , "luiz.dentz@gmail.com" , Amitkumar Karwar , Rohit Fule , Sherry Sun , "linux-bluetooth@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v1] Bluetooth: btnxpuart: Add support for IW624 chipset Message-ID: References: <20230810094802.832652-1-neeraj.sanjaykale@nxp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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