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

[Balakrishna Godavarthi]

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@xxxxxxxxxxxxxx>
> > > > ---
> > > >  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.