lkml.org 
[lkml]   [2018]   [Dec]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for power pulses
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.

\
 
 \ /
  Last update: 2018-12-27 04:25    [W:0.103 / U:1.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site