[PATCH 0/3] Bluetooth: hci_qca: Add delay after power-off pulse

[PATCH 0/3] Bluetooth: hci_qca: Add delay after power-off pulse

[Matthias Kaehlcke]

Initialization of the QCA WCN3990 often fails at boot time:

 [   15.205224] Bluetooth: hci0: setting up wcn3990
 [   17.341062] Bluetooth: hci0: command 0xfc00 tx timeout
 [   22.101453] ERROR: Bluetooth initialization failed
 [   25.337740] Bluetooth: hci0: Reading QCA version information failed (-110)

A short delay after sending a power-off pulse (which is done immediately
before sending a power-on pulse) fixes this.

Also move the delays to qca_send_power_pulse(), since they are directly
related with sending the pulses. Change the interface of
qca_send_power_pulse() to receive a boolean instead of a power pulse
command, this improves readability of the function with the delays
added.

Matthias Kaehlcke (3):
  Bluetooth: hci_qca: Pass boolean 'on/off' to qca_send_power_pulse()
  Bluetooth: hci_qca: Move boot delay to qca_send_power_pulse()
  Bluetooth: hci_qca: Add delay after power-off pulse

 drivers/bluetooth/hci_qca.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

-- 
2.21.0.rc0.258.g878e2cd30e-goog

[PATCH 1/3] Bluetooth: hci_qca: Pass boolean 'on/off' to qca_send_power_pulse()

[Matthias Kaehlcke]

There are only two types of power pulses 'on' or 'off', pass a boolean
instead of the power pulse 'command'.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/bluetooth/hci_qca.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 5e03504c4e0ca..e4128774e9686 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1004,10 +1004,11 @@ 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_uart *hu, u8 cmd)
+static int qca_send_power_pulse(struct hci_uart *hu, bool on)
 {
 	int ret;
 	int timeout = msecs_to_jiffies(POWER_PULSE_TRANS_TIMEOUT_MS);
+	u8 cmd = on ? QCA_WCN3990_POWERON_PULSE : QCA_WCN3990_POWEROFF_PULSE;
 
 	/* These power pulses are single byte command which are sent
 	 * at required baudrate to wcn3990. On wcn3990, we have an external
@@ -1138,12 +1139,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(hu, QCA_WCN3990_POWEROFF_PULSE);
+	ret = qca_send_power_pulse(hu, false);
 	if (ret)
 		return ret;
 
 	qca_set_speed(hu, QCA_INIT_SPEED);
-	ret = qca_send_power_pulse(hu, QCA_WCN3990_POWERON_PULSE);
+	ret = qca_send_power_pulse(hu, true);
 	if (ret)
 		return ret;
 
@@ -1289,7 +1290,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
 	spin_unlock_irqrestore(&qca->hci_ibs_lock, flags);
 
 	host_set_baudrate(hu, 2400);
-	qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE);
+	qca_send_power_pulse(hu, false);
 	qca_power_setup(hu, false);
 }
 
-- 
2.21.0.rc0.258.g878e2cd30e-goog

Re: [PATCH 1/3] Bluetooth: hci_qca: Pass boolean 'on/off' to qca_send_power_pulse()

[Balakrishna Godavarthi]

On 2019-02-26 05:19, Matthias Kaehlcke wrote:
> There are only two types of power pulses 'on' or 'off', pass a boolean
> instead of the power pulse 'command'.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/bluetooth/hci_qca.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 5e03504c4e0ca..e4128774e9686 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1004,10 +1004,11 @@ 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_uart *hu, u8 cmd)
> +static int qca_send_power_pulse(struct hci_uart *hu, bool on)
>  {
>  	int ret;
>  	int timeout = msecs_to_jiffies(POWER_PULSE_TRANS_TIMEOUT_MS);
> +	u8 cmd = on ? QCA_WCN3990_POWERON_PULSE : QCA_WCN3990_POWEROFF_PULSE;
> 
>  	/* These power pulses are single byte command which are sent
>  	 * at required baudrate to wcn3990. On wcn3990, we have an external
> @@ -1138,12 +1139,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(hu, QCA_WCN3990_POWEROFF_PULSE);
> +	ret = qca_send_power_pulse(hu, false);
>  	if (ret)
>  		return ret;
> 
>  	qca_set_speed(hu, QCA_INIT_SPEED);
> -	ret = qca_send_power_pulse(hu, QCA_WCN3990_POWERON_PULSE);
> +	ret = qca_send_power_pulse(hu, true);
>  	if (ret)
>  		return ret;
> 
> @@ -1289,7 +1290,7 @@ static void qca_power_shutdown(struct hci_uart 
> *hu)
>  	spin_unlock_irqrestore(&qca->hci_ibs_lock, flags);
> 
>  	host_set_baudrate(hu, 2400);
> -	qca_send_power_pulse(hu, QCA_WCN3990_POWEROFF_PULSE);
> +	qca_send_power_pulse(hu, false);
>  	qca_power_setup(hu, false);
>  }

Reviewed-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>

-- 
Regards
Balakrishna.

[PATCH 2/3] Bluetooth: hci_qca: Move boot delay to qca_send_power_pulse()

[Matthias Kaehlcke]

After sending a power on pulse the driver has a delay of 100ms
to allow the host controller to boot. Move the delay into
qca_send_power_pulse(), since it is directly related with the
power-on pulse.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/bluetooth/hci_qca.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index e4128774e9686..eacc108c422d0 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1036,6 +1036,9 @@ static int qca_send_power_pulse(struct hci_uart *hu, bool on)
 	usleep_range(100, 200);
 	hci_uart_set_flow_control(hu, false);
 
+	if (on)
+		msleep(100);
+
 	return 0;
 }
 
@@ -1148,9 +1151,6 @@ static int qca_wcn3990_init(struct hci_uart *hu)
 	if (ret)
 		return ret;
 
-	/* Wait for 100 ms for SoC to boot */
-	msleep(100);
-
 	/* Now the device is in ready state to communicate with host.
 	 * To sync host with device we need to reopen port.
 	 * Without this, we will have RTS and CTS synchronization
-- 
2.21.0.rc0.258.g878e2cd30e-goog

Re: [PATCH 2/3] Bluetooth: hci_qca: Move boot delay to qca_send_power_pulse()

[Balakrishna Godavarthi]

On 2019-02-26 05:19, Matthias Kaehlcke wrote:
> After sending a power on pulse the driver has a delay of 100ms
> to allow the host controller to boot. Move the delay into
> qca_send_power_pulse(), since it is directly related with the
> power-on pulse.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/bluetooth/hci_qca.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index e4128774e9686..eacc108c422d0 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1036,6 +1036,9 @@ static int qca_send_power_pulse(struct hci_uart
> *hu, bool on)
>  	usleep_range(100, 200);
>  	hci_uart_set_flow_control(hu, false);
> 
> +	if (on)
> +		msleep(100);
> +
>  	return 0;
>  }
> 
> @@ -1148,9 +1151,6 @@ static int qca_wcn3990_init(struct hci_uart *hu)
>  	if (ret)
>  		return ret;
> 
> -	/* Wait for 100 ms for SoC to boot */
> -	msleep(100);
> -
>  	/* Now the device is in ready state to communicate with host.
>  	 * To sync host with device we need to reopen port.
>  	 * Without this, we will have RTS and CTS synchronization

Reviewed-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>

-- 
Regards
Balakrishna.

Re: [PATCH 2/3] Bluetooth: hci_qca: Move boot delay to qca_send_power_pulse()

[Balakrishna Godavarthi]

hi Matthias,

On 2019-02-26 17:48, Balakrishna Godavarthi wrote:
> On 2019-02-26 05:19, Matthias Kaehlcke wrote:
>> After sending a power on pulse the driver has a delay of 100ms
>> to allow the host controller to boot. Move the delay into
>> qca_send_power_pulse(), since it is directly related with the
>> power-on pulse.
>> 
>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>  drivers/bluetooth/hci_qca.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index e4128774e9686..eacc108c422d0 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -1036,6 +1036,9 @@ static int qca_send_power_pulse(struct hci_uart
>> *hu, bool on)
>>  	usleep_range(100, 200);

[Bala] : i still doubt do we require this delay.

>>  	hci_uart_set_flow_control(hu, false);
>> 
>> +	if (on)
>> +		msleep(100);
>> +
>>  	return 0;
>>  }
>> 
>> @@ -1148,9 +1151,6 @@ static int qca_wcn3990_init(struct hci_uart *hu)
>>  	if (ret)
>>  		return ret;
>> 
>> -	/* Wait for 100 ms for SoC to boot */
>> -	msleep(100);
>> -
>>  	/* Now the device is in ready state to communicate with host.
>>  	 * To sync host with device we need to reopen port.
>>  	 * Without this, we will have RTS and CTS synchronization
> 
> Reviewed-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>

-- 
Regards
Balakrishna.

Re: [PATCH 2/3] Bluetooth: hci_qca: Move boot delay to qca_send_power_pulse()

[Matthias Kaehlcke]

Hi Balakrishna,

Thanks for the reviews!

On Tue, Feb 26, 2019 at 05:54:24PM +0530, Balakrishna Godavarthi wrote:
> hi Matthias,
> 
> On 2019-02-26 17:48, Balakrishna Godavarthi wrote:
> > On 2019-02-26 05:19, Matthias Kaehlcke wrote:
> > > After sending a power on pulse the driver has a delay of 100ms
> > > to allow the host controller to boot. Move the delay into
> > > qca_send_power_pulse(), since it is directly related with the
> > > power-on pulse.
> > > 
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > >  drivers/bluetooth/hci_qca.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > index e4128774e9686..eacc108c422d0 100644
> > > --- a/drivers/bluetooth/hci_qca.c
> > > +++ b/drivers/bluetooth/hci_qca.c
> > > @@ -1036,6 +1036,9 @@ static int qca_send_power_pulse(struct hci_uart
> > > *hu, bool on)
> > >  	usleep_range(100, 200);
> 
> [Bala] : i still doubt do we require this delay.

I don't know if it was necessary (or at least helped in the power-off
case) without this series, but my testing suggests it is not needed
with the delay after the power off pulse ("[3/3] Bluetooth: hci_qca:
Add delay after power-off pulse").

I'll send a new version without the delay.

Thanks

Matthias

[PATCH 3/3] Bluetooth: hci_qca: Add delay after power-off pulse

[Matthias Kaehlcke]

During initialization the power-on pulse is currently sent inmediately
after the prior power-off pulse. With this initialization often fails
at boot time:

[   15.205224] Bluetooth: hci0: setting up wcn3990
[   17.341062] Bluetooth: hci0: command 0xfc00 tx timeout
[   22.101453] ERROR: Bluetooth initialization failed
[   25.337740] Bluetooth: hci0: Reading QCA version information failed (-110)

After a power-off pulse wait 10ms to give the controller time to power
off.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/bluetooth/hci_qca.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index eacc108c422d0..c89c1ed87ffe9 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1036,8 +1036,11 @@ static int qca_send_power_pulse(struct hci_uart *hu, bool on)
 	usleep_range(100, 200);
 	hci_uart_set_flow_control(hu, false);
 
+	/* Give to controller time to boot/shutdown */
 	if (on)
 		msleep(100);
+	else
+		msleep(10);
 
 	return 0;
 }
-- 
2.21.0.rc0.258.g878e2cd30e-goog

Re: [PATCH 3/3] Bluetooth: hci_qca: Add delay after power-off pulse

[Balakrishna Godavarthi]

On 2019-02-26 05:19, Matthias Kaehlcke wrote:
> During initialization the power-on pulse is currently sent inmediately
> after the prior power-off pulse. With this initialization often fails
> at boot time:
> 
> [   15.205224] Bluetooth: hci0: setting up wcn3990
> [   17.341062] Bluetooth: hci0: command 0xfc00 tx timeout
> [   22.101453] ERROR: Bluetooth initialization failed
> [   25.337740] Bluetooth: hci0: Reading QCA version information failed 
> (-110)
> 
> After a power-off pulse wait 10ms to give the controller time to power
> off.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/bluetooth/hci_qca.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index eacc108c422d0..c89c1ed87ffe9 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1036,8 +1036,11 @@ static int qca_send_power_pulse(struct hci_uart
> *hu, bool on)
>  	usleep_range(100, 200);
>  	hci_uart_set_flow_control(hu, false);
> 
> +	/* Give to controller time to boot/shutdown */
>  	if (on)
>  		msleep(100);
> +	else
> +		msleep(10);
> 
>  	return 0;
>  }

Reviewed-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>

-- 
Regards
Balakrishna.