Messages in this thread | | | Date | Thu, 24 Jan 2019 16:50:29 +0530 | From | Balakrishna Godavarthi <> | Subject | Re: [PATCH v8 1/3] Bluetooth: hci_qca: use wait_until_sent() for power pulses |
| |
Hi Matthias,
On 2019-01-17 01:52, Matthias Kaehlcke wrote: > On Wed, Jan 16, 2019 at 05:16:01PM +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> >> --- >> Changes in v8: >> * Updated 1 second timeout instead of indefinite wait. >> >> Changes in v7: >> * updated the wait time to 5 ms after sending power pulses. >> >> Changes in v6: >> * added serdev_device_write_flush() in qca_send_power_pulse >> instead during the power off pulse. >> >> Changes in v5: >> * added serdev_device_write_flush() in qca_power_off(). >> --- >> drivers/bluetooth/hci_qca.c | 46 >> ++++++++++++++++--------------------- >> 1 file changed, 20 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> index f036c8f98ea3..681bfa30467e 100644 >> --- a/drivers/bluetooth/hci_qca.c >> +++ b/drivers/bluetooth/hci_qca.c >> @@ -60,6 +60,7 @@ >> #define IBS_WAKE_RETRANS_TIMEOUT_MS 100 >> #define IBS_TX_IDLE_TIMEOUT_MS 2000 >> #define BAUDRATE_SETTLE_TIMEOUT_MS 300 >> +#define POWER_PULSE_TRANS_TIMEOUT_MS 1000 > > nit: Not that it should make a different in normal operation, but 1s > seems extreme. Is there really any chance that the byte hasn't been > sent after say 100ms (which is still an eternity for a single byte)? > [Bala]: i missed to address this. for now let us have 1 second delay. based on stress test or further enchantments we can reduce this delay.
>> /* susclk rate */ >> #define SUSCLK_RATE_32KHZ 32768 >> @@ -1013,11 +1014,10 @@ 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; >> + int timeout = __msecs_to_jiffies(POWER_PULSE_TRANS_TIMEOUT_MS); > > use msecs_to_jiffies() > >> /* These power pulses are single byte command which are sent >> * at required baudrate to wcn3990. On wcn3990, we have an external >> @@ -1029,22 +1029,22 @@ 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 controller", cmd); >> >> + serdev_device_write_flush(hu->serdev); >> 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", 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); >> - >> - /* Wait for 100 uS for SoC to settle down */ >> - usleep_range(100, 200); >> + serdev_device_wait_until_sent(hu->serdev, timeout); >> + /* Wait of 5ms is required for assuring to send the byte on the Tx >> + * line and also for the controller to settle down for the received >> + * byte. >> + */ >> + usleep_range(5000, 6000); > > I incorrectly claimed that there might be still bytes sitting in the > UART FIFO when serdev_device_wait_until_sent() returns, Johan > corrected me on that (thanks!). So if it takes the SoC 100us to settle > down we should be good with the original code. > > Cheers > > Matthias
-- Regards Balakrishna.
| |