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.
