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 72DBAC41513 for ; Fri, 11 Aug 2023 07:38:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234089AbjHKHiX (ORCPT ); Fri, 11 Aug 2023 03:38:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:38526 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229992AbjHKHiW (ORCPT ); Fri, 11 Aug 2023 03:38:22 -0400 X-Greylist: delayed 46964 seconds by postgrey-1.37 at lindbergh.monkeyblade.net; Fri, 11 Aug 2023 00:38:19 PDT Received: from mail11.truemail.it (mail11.truemail.it [217.194.8.81]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 17E41E73; Fri, 11 Aug 2023 00:38:19 -0700 (PDT) Received: from francesco-nb.int.toradex.com (93-49-2-63.ip317.fastwebnet.it [93.49.2.63]) by mail11.truemail.it (Postfix) with ESMTPA id D7179206D9; Fri, 11 Aug 2023 09:38:16 +0200 (CEST) Date: Fri, 11 Aug 2023 09:38:15 +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 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