Re: [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses

From: Balakrishna Godavarthi <bgodavar@codeaurora.org>
To: Matthias Kaehlcke <mka@chromium.org>
Cc: marcel@holtmann.org, johan.hedberg@gmail.com, johan@kernel.org,
	linux-kernel@vger.kernel.org, linux-bluetooth@vger.kernel.org,
	hemantg@codeaurora.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses
Date: Thu, 27 Dec 2018 08:53:56 +0530	[thread overview]
Message-ID: <75a8b12023f38a8af5e281ea1902cb73@codeaurora.org> (raw)
In-Reply-To: <20181226222148.GI261387@google.com>

Hi Matthias,

On 2018-12-27 03:51, Matthias Kaehlcke wrote:
> On Wed, Dec 26, 2018 at 12:01:51PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-12-22 07:29, Matthias Kaehlcke wrote:
>> > On Thu, Dec 20, 2018 at 08:16:35PM +0530, Balakrishna Godavarthi wrote:
>> > > wcn3990 requires a power pulse to turn ON/OFF along with
>> > > regulators. Sometimes we are observing the power pulses are sent
>> > > out with some time delay, due to queuing these commands. This is
>> > > causing synchronization issues with chip, which intern delay the
>> > > chip setup or may end up with communication issues.
>> > >
>> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> > > ---
>> > >  drivers/bluetooth/hci_qca.c | 38
>> > > ++++++++++++++-----------------------
>> > >  1 file changed, 14 insertions(+), 24 deletions(-)
>> > >
>> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > > index f036c8f98ea3..5a07c2370289 100644
>> > > --- a/drivers/bluetooth/hci_qca.c
>> > > +++ b/drivers/bluetooth/hci_qca.c
>> > > @@ -1013,11 +1013,9 @@ static inline void host_set_baudrate(struct
>> > > hci_uart *hu, unsigned int speed)
>> > >  		hci_uart_set_baudrate(hu, speed);
>> > >  }
>> > >
>> > > -static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
>> > > +static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
>> > >  {
>> > > -	struct hci_uart *hu = hci_get_drvdata(hdev);
>> > > -	struct qca_data *qca = hu->priv;
>> > > -	struct sk_buff *skb;
>> > > +	int ret;
>> > >
>> > >  	/* These power pulses are single byte command which are sent
>> > >  	 * at required baudrate to wcn3990. On wcn3990, we have an external
>> > > @@ -1029,19 +1027,16 @@ static int qca_send_power_pulse(struct
>> > > hci_dev *hdev, u8 cmd)
>> > >  	 * save power. Disabling hardware flow control is mandatory while
>> > >  	 * sending power pulses to SoC.
>> > >  	 */
>> > > -	bt_dev_dbg(hdev, "sending power pulse %02x to SoC", cmd);
>> > > -
>> > > -	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
>> > > -	if (!skb)
>> > > -		return -ENOMEM;
>> > > -
>> > > +	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
>> > >  	hci_uart_set_flow_control(hu, true);
>> > > +	ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
>> > > +	if (ret < 0) {
>> > > +		bt_dev_err(hu->hdev, "failed to send power pulse %02x to SoC",
>> > > +			   cmd);
>> > > +		return ret;
>> > > +	}
>> > >
>> > > -	skb_put_u8(skb, cmd);
>> > > -	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
>> > > -
>> > > -	skb_queue_tail(&qca->txq, skb);
>> > > -	hci_uart_tx_wakeup(hu);
>> > > +	serdev_device_wait_until_sent(hu->serdev, 0);
>> >
>> > serdev_device_wait_until_sent() might only guarantee that the UART
>> > circular buffer is empty (see
>> > https://elixir.bootlin.com/linux/v4.19/source/drivers/tty/tty_ioctl.c#L225),
>> > not that the data has actually sent (e.g. it might be sitting in
>> > the UART FIFO). However with this:
>> >
>> > >  	/* Wait for 100 uS for SoC to settle down */
>> > >  	usleep_range(100, 200);
>> >
>> > we should probably be fine, unless there's tons of data in the
>> > FIFO.
>> >
>> > You currently call serdev_device_write_flush() in
>> > qca_power_shutdown(), I wonder if it would make sense to call it in
>> > qca_send_power_pulse(), regardless of whether it's an on or off
>> 
>> [Bala]: during sending the ON pulse we will not have any data in the
>>         UART circular buffer as this is the first command to send and 
>> we are
>> sending it as soon as we open the port.
>>         so i taught why should be flush the circular as it is already 
>> empty.
> 
>> > pulse. In any case we don't care if the chip receives any 'pending'
>> > data when we switch it on or off, right?
>> >
>> 
>> [Bala]: during on we freshly open port and i see that flush() called 
>> while
>> port opening.
>> 
>> https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/serial_core.c#L207
> 
> I would argue that the serdev_device_write_flush() call in
> qca_power_shutdown() is related with/needed for sending the power off
> pulse, hence it should be part of qca_send_power_pulse(), unless it
> adds a significant overhead and we really want to call it only in the
> shutdown case.
> 
> Flushing the buffer should be fairly lightweight and power pulses are
> only sent when the device is switched on or off, so the overhead
> should be negligible. You *could* restrict the flush to the power off
> pulse, assuming that the driver always re-opens the port in
> qca_wcn3990_init() (tests with this patch set suggest this might not
> be needed) and that serdev_device_open() flushes the buffer (which
> seems a sane assumption). Yet given the minimal overhead I'd suggest
> to not make assumptions about what happened previously in other code
> and avoid the clutter of a condition that doesn't add much value.
> 
[Bala]: will call the flush() while sending the power pulses 
irrespective of the pulse type.

> Cheers
> 
> Matthias

-- 
Regards
Balakrishna.