From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthias Kaehlcke Subject: Re: [PATCH v7 1/4] Bluetooth: hci_qca: use wait_until_sent() for power pulses Date: Fri, 28 Dec 2018 09:20:05 -0800 Message-ID: <20181228172005.GM261387@google.com> References: <20181228114819.17479-1-bgodavar@codeaurora.org> <20181228114819.17479-2-bgodavar@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Return-path: Content-Disposition: inline In-Reply-To: <20181228114819.17479-2-bgodavar@codeaurora.org> Sender: linux-kernel-owner@vger.kernel.org To: Balakrishna Godavarthi 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 List-Id: linux-arm-msm@vger.kernel.org On Fri, Dec 28, 2018 at 05:18:16PM +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 > --- > 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 | 43 ++++++++++++++++--------------------- > 1 file changed, 18 insertions(+), 25 deletions(-) > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index f036c8f98ea3..2aab34b3e696 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,22 +1027,23 @@ 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); > + serdev_device_wait_until_sent(hu->serdev, 0); > > - /* Wait for 100 uS for SoC to settle down */ > - usleep_range(100, 200); > + /* 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); We could be more flexible with the range, e.g. 5 to 10 ms, but it's also fine as is. > hci_uart_set_flow_control(hu, false); > > return 0; > @@ -1116,7 +1115,6 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type) > > static int qca_wcn3990_init(struct hci_uart *hu) > { > - struct hci_dev *hdev = hu->hdev; > struct qca_serdev *qcadev; > int ret; > > @@ -1139,12 +1137,12 @@ static int qca_wcn3990_init(struct hci_uart *hu) > > /* Forcefully enable wcn3990 to enter in to boot mode. */ > host_set_baudrate(hu, 2400); > - ret = qca_send_power_pulse(hdev, QCA_WCN3990_POWEROFF_PULSE); > + ret = qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE); > if (ret) > return ret; > > qca_set_speed(hu, QCA_INIT_SPEED); > - ret = qca_send_power_pulse(hdev, QCA_WCN3990_POWERON_PULSE); > + ret = qca_send_power_pulse(hu, QCA_WCN3990_POWERON_PULSE); > if (ret) > return ret; > > @@ -1274,13 +1272,8 @@ static const struct qca_vreg_data qca_soc_data = { > > static void qca_power_shutdown(struct hci_uart *hu) > { > - struct serdev_device *serdev = hu->serdev; > - unsigned char cmd = QCA_WCN3990_POWEROFF_PULSE; > - > host_set_baudrate(hu, 2400); > - hci_uart_set_flow_control(hu, true); > - serdev_device_write_buf(serdev, &cmd, sizeof(cmd)); > - hci_uart_set_flow_control(hu, false); > + qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE); > qca_power_setup(hu, false); > } > Reviewed-by: Matthias Kaehlcke