Date:   Mon, 25 Feb 2019 15:56:20 -0800
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Balakrishna Godavarthi <bgodavar@codeaurora.org>
Cc:     marcel@holtmann.org, johan.hedberg@gmail.com,
        linux-kernel@vger.kernel.org, linux-bluetooth@vger.kernel.org,
        hemantg@codeaurora.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v1] Bluetooth: hci_qca: Update baudrate change wait time
 for wcn3990
Message-ID: <20190225235620.GA67723@google.com>
References: <20190220112516.19179-1-bgodavar@codeaurora.org>
 <20190220234302.GB116191@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20190220234302.GB116191@google.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi,

On Wed, Feb 20, 2019 at 03:43:02PM -0800, Matthias Kaehlcke wrote:
> Hi Balakrishna,
> 
> On Wed, Feb 20, 2019 at 04:55:16PM +0530, Balakrishna Godavarthi wrote:
> > This patch will update the baudrate change request wait time from
> > 300 ms to 100 ms. When host sends the change baudrate request to
> > the controller, controller sets its clock and wait until the
> > clocks settle down. Here the Wait time is required for both
> > host and controller to be on sync.
> > 
> > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > ---
> >  drivers/bluetooth/hci_qca.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > index 5e03504c4e0c..22f3c983f868 100644
> > --- a/drivers/bluetooth/hci_qca.c
> > +++ b/drivers/bluetooth/hci_qca.c
> > @@ -59,7 +59,8 @@
> >  
> >  #define IBS_WAKE_RETRANS_TIMEOUT_MS	100
> >  #define IBS_TX_IDLE_TIMEOUT_MS		2000
> > -#define BAUDRATE_SETTLE_TIMEOUT_MS	300
> > +#define ROME_BD_SETTLE_TIMEOUT_MS	300
> > +#define WCN3990_BD_SETTLE_TIMEOUT_MS	100
> 
> nit: _BR_ instead of _BD_?
> 
> >  #define POWER_PULSE_TRANS_TIMEOUT_MS	100
> >  
> >  /* susclk rate */
> > @@ -965,8 +966,11 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
> >  	struct hci_uart *hu = hci_get_drvdata(hdev);
> >  	struct qca_data *qca = hu->priv;
> >  	struct sk_buff *skb;
> > +	struct qca_serdev *qcadev;
> > +	unsigned int bd_settling_timeout;
> 
> nit: from the context the purpose of the variable is fairly clear,
> calling it just 'timeout' (or 'settling_time', it's not really a
> timeout) should be fine.
> 
> That said, I have a similar change in my pipeline, which further
> reduces the time to the 'strictly necessary'. It's slightly more code
> though. I guess I'll send it and we can discuss/let Marcel decide
> what to adopt.

I haven't sent my patches yet since I encountered initialization
errors when testing, however it turns out these errors are not
related with my changes.

This series fixes the problem:

https://lore.kernel.org/patchwork/project/lkml/list/?series=384571

I'm now retesting my changes and will send them soon unless I
encounter other issues.

Thanks

Matthias