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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Johan,

On 2018-12-06 16:10, Balakrishna Godavarthi wrote:
Hi Johan,

On 2018-12-05 11:55, Johan Hovold wrote:
On Fri, Nov 30, 2018 at 08:32:44PM +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>
---
v3:
  * no change.
v2:
  * Updated function qca_send_power_pulse()
  * addressed reviewer comments.

Please make sure to include reviewers on CC when resending, and as
someone else already mentioned, be a bit more specific about what
changes you actually made in response to the review feedback you
received.


[Bala]: sure will add and provide more info in version change history.

v1:
 * initial patch
---
drivers/bluetooth/hci_qca.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index f036c8f98ea3..f5dd323c1967 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(hu->serdev, &cmd, sizeof(cmd), 0);

You're still using 0 as a timeout here which is broken, as I already
told you.


[Bala]: got the change now will update to timeout to non zero value.

From 4.21 this will result in an indefinite timeout, but currently
implies not to wait for a full write buffer to drain at all.

As I also mentioned, you need to to make sure to call
serdev_device_write_wakeup() in the write_wakup() path if you are going
to use serdev_device_write() at all.


[Bala]: this where i am confused.
        calling serdev_device_write is calling an wakeup internally.
below is the flow

        ttyport_write_buf:
              * calling serdev_device_write() will call write_buf() in
this call we are enabling bit "TTY_DO_WRITE_WAKEUP" and calling
write()
                i.e. uart_write() where we call in start_tx. this will
go to the vendor specific write where in isr we call
uart_write_wakeup()

https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/qcom_geni_serial.c#L756


uart_write_wakeup()->ttyport_write_wakeup()->serdev_controller_write_wakeup()->hci_uart_write_wakeup()->hci_uart_tx_wakeup()

        the above is flow when serdev_device_write() is called, it is
indirectly calling serdev_write_wakeup().

        Why actual we need to call an serdev_write_wakeup() is this
wakeup related to the UART port or for the BT chip.

Johan

Can you help me to understand, whether my understating is correct wrt serdev_wakeup().

--
Regards
Balakrishna.



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux