[v5,2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
diff mbox series

Message ID 20181220144639.15928-3-bgodavar@codeaurora.org
State Superseded
Headers show
Series
  • Bug fixes for Qualcomm BT chip wcn3990
Related show

Commit Message

Balakrishna Godavarthi Dec. 20, 2018, 2:46 p.m. UTC
This patch will help to stop frame reassembly errors while changing
the baudrate. This is because host send a change baudrate request
command to the chip with 115200 bps, Whereas chip will change their
UART clocks to the enable for new baudrate and sends the response
for the change request command with newer baudrate, On host side
we are still operating in 115200 bps which results of reading garbage
data. Here we are pulling RTS line, so that chip we will wait to send data
to host until host change its baudrate.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
Tested-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/bluetooth/hci_qca.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

Comments

Matthias Kaehlcke Dec. 22, 2018, 12:31 a.m. UTC | #1
On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
> This patch will help to stop frame reassembly errors while changing
> the baudrate. This is because host send a change baudrate request
> command to the chip with 115200 bps, Whereas chip will change their
> UART clocks to the enable for new baudrate and sends the response
> for the change request command with newer baudrate, On host side
> we are still operating in 115200 bps which results of reading garbage
> data. Here we are pulling RTS line, so that chip we will wait to send data
> to host until host change its baudrate.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/bluetooth/hci_qca.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 5a07c2370289..1680ead6cc3d 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -963,7 +963,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>  	struct hci_uart *hu = hci_get_drvdata(hdev);
>  	struct qca_data *qca = hu->priv;
>  	struct sk_buff *skb;
> -	struct qca_serdev *qcadev;
>  	u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
>  
>  	if (baudrate > QCA_BAUDRATE_3200000)
> @@ -977,13 +976,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>  		return -ENOMEM;
>  	}
>  
> -	/* Disabling hardware flow control is mandatory while
> -	 * sending change baudrate request to wcn3990 SoC.
> -	 */
> -	qcadev = serdev_device_get_drvdata(hu->serdev);
> -	if (qcadev->btsoc_type == QCA_WCN3990)
> -		hci_uart_set_flow_control(hu, true);
> -
>  	/* Assign commands to change baudrate and packet type. */
>  	skb_put_data(skb, cmd, sizeof(cmd));
>  	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
> @@ -999,9 +991,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>  	schedule_timeout(msecs_to_jiffies(BAUDRATE_SETTLE_TIMEOUT_MS));
>  	set_current_state(TASK_RUNNING);
>  
> -	if (qcadev->btsoc_type == QCA_WCN3990)
> -		hci_uart_set_flow_control(hu, false);
> -
>  	return 0;
>  }
>  
> @@ -1086,6 +1075,7 @@ static int qca_check_speeds(struct hci_uart *hu)
>  static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  {
>  	unsigned int speed, qca_baudrate;
> +	struct qca_serdev *qcadev;
>  	int ret;
>  
>  	if (speed_type == QCA_INIT_SPEED) {
> @@ -1097,6 +1087,15 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  		if (!speed)
>  			return 0;
>  
> +		/* Deassert RTS while changing the baudrate of chip and host.
> +		 * This will prevent chip from transmitting its response with
> +		 * the new baudrate while the host port is still operating at
> +		 * the old speed.
> +		 */
> +		qcadev = serdev_device_get_drvdata(hu->serdev);
> +		if (qcadev->btsoc_type == QCA_WCN3990)
> +			serdev_device_set_rts(hu->serdev, false);
> +
>  		qca_baudrate = qca_get_baudrate_value(speed);
>  		bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
>  		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
> @@ -1104,6 +1103,9 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  			return ret;
>  
>  		host_set_baudrate(hu, speed);
> +
> +		if (qcadev->btsoc_type == QCA_WCN3990)
> +			serdev_device_set_rts(hu->serdev, true);
>  	}
>  
>  	return 0;

I looked for ways to do without this change, but didn't find a good
solution. There are several possible problems with baudrate changes:

1) send request to BT controller to change the baudrate

  this is an asynchronous operation, the actual baudrate change can
  be delayed for multiple reasons, e.g.:

  - request sits in the BT driver's TX queue

    this could be worked around by checking skb_queue_empty()

  - request sits in the UART buffer

    a workaround for this could be calling
    serdev_device_wait_until_sent() (only available with serdev though)

  - the request sits in the UART FIFO

    will be sent out 'immediately'. no neat solution available AFAIK,
    a short sleep could be an effective workaround

  - the controller may have a short delay to apply the change

    Also no neat solution here. A/the same short sleep could work
    around this

2) change baudrate of the host UART
  - this must not happen before the baudrate change request has been
    sent to the BT controller, otherwise things are messed up
    seriously

    Ideally set_termios would make sure all pending data is sent
    before the change is applied, some UART drivers do this, others
    don't, so we can't rely on this.

3) BT controller sends data after baudrate change

  a few ms after a baudrate change the BT controller sends data
  (4, 255, 2, 146, 1, 4, 14, 4, 1, 0, 0, 0) with the new baudrate

  - dunno what the data stands for, but the BT stack/driver appears to
    be fine with it, as long as the host UART operates at the new
    baudrate when the data is received.

  - if the data is received before the baudrate of the host UART is
    changes we see 'frame reassembly' errors


In summary, I think it should be feasible to guarantee that the
baudrate change of the host UART is always done after the controller
changed it's baudrate, however we can't guarantee at the same time
that the baudrate change of the host controller is completed before
the BT controller sends its 'response'.

Using the RTS signal seems a reasonable way to delay the controller
data until the host is ready, the only thing I don't like too much
is that in this patch set we currently have two mechanisms to
suppress/delay unwanted data. Unfortunately the RTS method isn't
effective at initialization time.

Not the scope of this patch set, but I really dislike the 300 ms delay
(BAUDRATE_SETTLE_TIMEOUT_MS) in qca_set_baudrate(), and wonder if it
is actually needed (I seriously doubt that it takes the BT controller
300 ms to change its baudrate). I guess it's more a combination of what I
described above in 1), once we are done with this series I might try
to improve this, unless somebody is really, really convinced that such
a gigantic delay is actually needed.

Cheers

Matthias
Balakrishna Godavarthi Dec. 26, 2018, 5:45 a.m. UTC | #2
Hi Matthias,

On 2018-12-22 06:01, Matthias Kaehlcke wrote:
> On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
>> This patch will help to stop frame reassembly errors while changing
>> the baudrate. This is because host send a change baudrate request
>> command to the chip with 115200 bps, Whereas chip will change their
>> UART clocks to the enable for new baudrate and sends the response
>> for the change request command with newer baudrate, On host side
>> we are still operating in 115200 bps which results of reading garbage
>> data. Here we are pulling RTS line, so that chip we will wait to send 
>> data
>> to host until host change its baudrate.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> Tested-by: Matthias Kaehlcke <mka@chromium.org>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>  drivers/bluetooth/hci_qca.c | 24 +++++++++++++-----------
>>  1 file changed, 13 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 5a07c2370289..1680ead6cc3d 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -963,7 +963,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, 
>> uint8_t baudrate)
>>  	struct hci_uart *hu = hci_get_drvdata(hdev);
>>  	struct qca_data *qca = hu->priv;
>>  	struct sk_buff *skb;
>> -	struct qca_serdev *qcadev;
>>  	u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
>> 
>>  	if (baudrate > QCA_BAUDRATE_3200000)
>> @@ -977,13 +976,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, 
>> uint8_t baudrate)
>>  		return -ENOMEM;
>>  	}
>> 
>> -	/* Disabling hardware flow control is mandatory while
>> -	 * sending change baudrate request to wcn3990 SoC.
>> -	 */
>> -	qcadev = serdev_device_get_drvdata(hu->serdev);
>> -	if (qcadev->btsoc_type == QCA_WCN3990)
>> -		hci_uart_set_flow_control(hu, true);
>> -
>>  	/* Assign commands to change baudrate and packet type. */
>>  	skb_put_data(skb, cmd, sizeof(cmd));
>>  	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
>> @@ -999,9 +991,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, 
>> uint8_t baudrate)
>>  	schedule_timeout(msecs_to_jiffies(BAUDRATE_SETTLE_TIMEOUT_MS));
>>  	set_current_state(TASK_RUNNING);
>> 
>> -	if (qcadev->btsoc_type == QCA_WCN3990)
>> -		hci_uart_set_flow_control(hu, false);
>> -
>>  	return 0;
>>  }
>> 
>> @@ -1086,6 +1075,7 @@ static int qca_check_speeds(struct hci_uart *hu)
>>  static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type 
>> speed_type)
>>  {
>>  	unsigned int speed, qca_baudrate;
>> +	struct qca_serdev *qcadev;
>>  	int ret;
>> 
>>  	if (speed_type == QCA_INIT_SPEED) {
>> @@ -1097,6 +1087,15 @@ static int qca_set_speed(struct hci_uart *hu, 
>> enum qca_speed_type speed_type)
>>  		if (!speed)
>>  			return 0;
>> 
>> +		/* Deassert RTS while changing the baudrate of chip and host.
>> +		 * This will prevent chip from transmitting its response with
>> +		 * the new baudrate while the host port is still operating at
>> +		 * the old speed.
>> +		 */
>> +		qcadev = serdev_device_get_drvdata(hu->serdev);
>> +		if (qcadev->btsoc_type == QCA_WCN3990)
>> +			serdev_device_set_rts(hu->serdev, false);
>> +
>>  		qca_baudrate = qca_get_baudrate_value(speed);
>>  		bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
>>  		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>> @@ -1104,6 +1103,9 @@ static int qca_set_speed(struct hci_uart *hu, 
>> enum qca_speed_type speed_type)
>>  			return ret;
>> 
>>  		host_set_baudrate(hu, speed);
>> +
>> +		if (qcadev->btsoc_type == QCA_WCN3990)
>> +			serdev_device_set_rts(hu->serdev, true);
>>  	}
>> 
>>  	return 0;
> 
> I looked for ways to do without this change, but didn't find a good
> solution. There are several possible problems with baudrate changes:
> 
> 1) send request to BT controller to change the baudrate
> 
>   this is an asynchronous operation, the actual baudrate change can
>   be delayed for multiple reasons, e.g.:
> 
>   - request sits in the BT driver's TX queue
> 
>     this could be worked around by checking skb_queue_empty()
> 
>   - request sits in the UART buffer
> 
>     a workaround for this could be calling
>     serdev_device_wait_until_sent() (only available with serdev though)
> 
>   - the request sits in the UART FIFO
> 
>     will be sent out 'immediately'. no neat solution available AFAIK,
>     a short sleep could be an effective workaround
> 
>   - the controller may have a short delay to apply the change
> 
>     Also no neat solution here. A/the same short sleep could work
>     around this
> 
> 2) change baudrate of the host UART
>   - this must not happen before the baudrate change request has been
>     sent to the BT controller, otherwise things are messed up
>     seriously
> 
>     Ideally set_termios would make sure all pending data is sent
>     before the change is applied, some UART drivers do this, others
>     don't, so we can't rely on this.
> 
> 3) BT controller sends data after baudrate change
> 
>   a few ms after a baudrate change the BT controller sends data
>   (4, 255, 2, 146, 1, 4, 14, 4, 1, 0, 0, 0) with the new baudrate
> 
>   - dunno what the data stands for, but the BT stack/driver appears to
>     be fine with it, as long as the host UART operates at the new
>     baudrate when the data is received.
> 
>   - if the data is received before the baudrate of the host UART is
>     changes we see 'frame reassembly' errors
> 
> 
[Bala]: the data is an vendor specific event and command complete event,
          4, 255, 2, 146, 1, : vendor specific event
          4, 14, 4, 1, 0, 0, 0: command complete event.

> In summary, I think it should be feasible to guarantee that the
> baudrate change of the host UART is always done after the controller
> changed it's baudrate, however we can't guarantee at the same time
> that the baudrate change of the host controller is completed before
> the BT controller sends its 'response'.
> 
> Using the RTS signal seems a reasonable way to delay the controller
> data until the host is ready, the only thing I don't like too much
> is that in this patch set we currently have two mechanisms to
> suppress/delay unwanted data. Unfortunately the RTS method isn't
> effective at initialization time.
> 
> Not the scope of this patch set, but I really dislike the 300 ms delay
> (BAUDRATE_SETTLE_TIMEOUT_MS) in qca_set_baudrate(), and wonder if it
> is actually needed (I seriously doubt that it takes the BT controller
> 300 ms to change its baudrate). I guess it's more a combination of what 
> I
> described above in 1), once we are done with this series I might try
> to improve this, unless somebody is really, really convinced that such
> a gigantic delay is actually needed.
> 
[Bala]:  Thanks for detail analysis.
         even i feel the same whether is it really required to have an 
delay of 300ms.
         But during our testing we found the it depends on the controller 
clock settling time.
         all observations are less than 100 ms. will update this change 
in separate patch series.

> Cheers
> 
> Matthias
Matthias Kaehlcke Dec. 26, 2018, 8:25 p.m. UTC | #3
Hi Balakrishna,

On Wed, Dec 26, 2018 at 11:15:30AM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-12-22 06:01, Matthias Kaehlcke wrote:
> > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
> > > This patch will help to stop frame reassembly errors while changing
> > > the baudrate. This is because host send a change baudrate request
> > > command to the chip with 115200 bps, Whereas chip will change their
> > > UART clocks to the enable for new baudrate and sends the response
> > > for the change request command with newer baudrate, On host side
> > > we are still operating in 115200 bps which results of reading garbage
> > > data. Here we are pulling RTS line, so that chip we will wait to
> > > send data
> > > to host until host change its baudrate.
> > > 
> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > > Tested-by: Matthias Kaehlcke <mka@chromium.org>
> > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > >  drivers/bluetooth/hci_qca.c | 24 +++++++++++++-----------
> > >  1 file changed, 13 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > index 5a07c2370289..1680ead6cc3d 100644
> > > --- a/drivers/bluetooth/hci_qca.c
> > > +++ b/drivers/bluetooth/hci_qca.c
> > > @@ -963,7 +963,6 @@ static int qca_set_baudrate(struct hci_dev
> > > *hdev, uint8_t baudrate)
> > >  	struct hci_uart *hu = hci_get_drvdata(hdev);
> > >  	struct qca_data *qca = hu->priv;
> > >  	struct sk_buff *skb;
> > > -	struct qca_serdev *qcadev;
> > >  	u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
> > > 
> > >  	if (baudrate > QCA_BAUDRATE_3200000)
> > > @@ -977,13 +976,6 @@ static int qca_set_baudrate(struct hci_dev
> > > *hdev, uint8_t baudrate)
> > >  		return -ENOMEM;
> > >  	}
> > > 
> > > -	/* Disabling hardware flow control is mandatory while
> > > -	 * sending change baudrate request to wcn3990 SoC.
> > > -	 */
> > > -	qcadev = serdev_device_get_drvdata(hu->serdev);
> > > -	if (qcadev->btsoc_type == QCA_WCN3990)
> > > -		hci_uart_set_flow_control(hu, true);
> > > -
> > >  	/* Assign commands to change baudrate and packet type. */
> > >  	skb_put_data(skb, cmd, sizeof(cmd));
> > >  	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
> > > @@ -999,9 +991,6 @@ static int qca_set_baudrate(struct hci_dev
> > > *hdev, uint8_t baudrate)
> > >  	schedule_timeout(msecs_to_jiffies(BAUDRATE_SETTLE_TIMEOUT_MS));
> > >  	set_current_state(TASK_RUNNING);
> > > 
> > > -	if (qcadev->btsoc_type == QCA_WCN3990)
> > > -		hci_uart_set_flow_control(hu, false);
> > > -
> > >  	return 0;
> > >  }
> > > 
> > > @@ -1086,6 +1075,7 @@ static int qca_check_speeds(struct hci_uart *hu)
> > >  static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type
> > > speed_type)
> > >  {
> > >  	unsigned int speed, qca_baudrate;
> > > +	struct qca_serdev *qcadev;
> > >  	int ret;
> > > 
> > >  	if (speed_type == QCA_INIT_SPEED) {
> > > @@ -1097,6 +1087,15 @@ static int qca_set_speed(struct hci_uart *hu,
> > > enum qca_speed_type speed_type)
> > >  		if (!speed)
> > >  			return 0;
> > > 
> > > +		/* Deassert RTS while changing the baudrate of chip and host.
> > > +		 * This will prevent chip from transmitting its response with
> > > +		 * the new baudrate while the host port is still operating at
> > > +		 * the old speed.
> > > +		 */
> > > +		qcadev = serdev_device_get_drvdata(hu->serdev);
> > > +		if (qcadev->btsoc_type == QCA_WCN3990)
> > > +			serdev_device_set_rts(hu->serdev, false);
> > > +
> > >  		qca_baudrate = qca_get_baudrate_value(speed);
> > >  		bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
> > >  		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
> > > @@ -1104,6 +1103,9 @@ static int qca_set_speed(struct hci_uart *hu,
> > > enum qca_speed_type speed_type)
> > >  			return ret;
> > > 
> > >  		host_set_baudrate(hu, speed);
> > > +
> > > +		if (qcadev->btsoc_type == QCA_WCN3990)
> > > +			serdev_device_set_rts(hu->serdev, true);
> > >  	}
> > > 
> > >  	return 0;
> > 
> > I looked for ways to do without this change, but didn't find a good
> > solution. There are several possible problems with baudrate changes:
> > 
> > 1) send request to BT controller to change the baudrate
> > 
> >   this is an asynchronous operation, the actual baudrate change can
> >   be delayed for multiple reasons, e.g.:
> > 
> >   - request sits in the BT driver's TX queue
> > 
> >     this could be worked around by checking skb_queue_empty()
> > 
> >   - request sits in the UART buffer
> > 
> >     a workaround for this could be calling
> >     serdev_device_wait_until_sent() (only available with serdev though)
> > 
> >   - the request sits in the UART FIFO
> > 
> >     will be sent out 'immediately'. no neat solution available AFAIK,
> >     a short sleep could be an effective workaround
> > 
> >   - the controller may have a short delay to apply the change
> > 
> >     Also no neat solution here. A/the same short sleep could work
> >     around this
> > 
> > 2) change baudrate of the host UART
> >   - this must not happen before the baudrate change request has been
> >     sent to the BT controller, otherwise things are messed up
> >     seriously
> > 
> >     Ideally set_termios would make sure all pending data is sent
> >     before the change is applied, some UART drivers do this, others
> >     don't, so we can't rely on this.
> > 
> > 3) BT controller sends data after baudrate change
> > 
> >   a few ms after a baudrate change the BT controller sends data
> >   (4, 255, 2, 146, 1, 4, 14, 4, 1, 0, 0, 0) with the new baudrate
> > 
> >   - dunno what the data stands for, but the BT stack/driver appears to
> >     be fine with it, as long as the host UART operates at the new
> >     baudrate when the data is received.
> > 
> >   - if the data is received before the baudrate of the host UART is
> >     changes we see 'frame reassembly' errors
> > 
> > 
> [Bala]: the data is an vendor specific event and command complete event,
>          4, 255, 2, 146, 1, : vendor specific event
>          4, 14, 4, 1, 0, 0, 0: command complete event.

Thanks!

> > In summary, I think it should be feasible to guarantee that the
> > baudrate change of the host UART is always done after the controller
> > changed it's baudrate, however we can't guarantee at the same time
> > that the baudrate change of the host controller is completed before
> > the BT controller sends its 'response'.
> > 
> > Using the RTS signal seems a reasonable way to delay the controller
> > data until the host is ready, the only thing I don't like too much
> > is that in this patch set we currently have two mechanisms to
> > suppress/delay unwanted data. Unfortunately the RTS method isn't
> > effective at initialization time.
> > 
> > Not the scope of this patch set, but I really dislike the 300 ms delay
> > (BAUDRATE_SETTLE_TIMEOUT_MS) in qca_set_baudrate(), and wonder if it
> > is actually needed (I seriously doubt that it takes the BT controller
> > 300 ms to change its baudrate). I guess it's more a combination of what
> > I
> > described above in 1), once we are done with this series I might try
> > to improve this, unless somebody is really, really convinced that such
> > a gigantic delay is actually needed.
> > 
> [Bala]:  Thanks for detail analysis.
>         even i feel the same whether is it really required to have an delay
> of 300ms.
>         But during our testing we found the it depends on the controller
> clock settling time.
>         all observations are less than 100 ms. will update this change in
> separate patch series.

100 ms is definitely better than 300 ms if that's not really
needed. Did you see the need for a 100 ms delay with the WCN3990 or
some other QCA controller?

Cheers

Matthias
Balakrishna Godavarthi Dec. 27, 2018, 3:20 a.m. UTC | #4
Hi Matthias,

On 2018-12-27 01:55, Matthias Kaehlcke wrote:
> Hi Balakrishna,
> 
> On Wed, Dec 26, 2018 at 11:15:30AM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-12-22 06:01, Matthias Kaehlcke wrote:
>> > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
>> > > This patch will help to stop frame reassembly errors while changing
>> > > the baudrate. This is because host send a change baudrate request
>> > > command to the chip with 115200 bps, Whereas chip will change their
>> > > UART clocks to the enable for new baudrate and sends the response
>> > > for the change request command with newer baudrate, On host side
>> > > we are still operating in 115200 bps which results of reading garbage
>> > > data. Here we are pulling RTS line, so that chip we will wait to
>> > > send data
>> > > to host until host change its baudrate.
>> > >
>> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> > > Tested-by: Matthias Kaehlcke <mka@chromium.org>
>> > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> > > ---
>> > >  drivers/bluetooth/hci_qca.c | 24 +++++++++++++-----------
>> > >  1 file changed, 13 insertions(+), 11 deletions(-)
>> > >
>> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > > index 5a07c2370289..1680ead6cc3d 100644
>> > > --- a/drivers/bluetooth/hci_qca.c
>> > > +++ b/drivers/bluetooth/hci_qca.c
>> > > @@ -963,7 +963,6 @@ static int qca_set_baudrate(struct hci_dev
>> > > *hdev, uint8_t baudrate)
>> > >  	struct hci_uart *hu = hci_get_drvdata(hdev);
>> > >  	struct qca_data *qca = hu->priv;
>> > >  	struct sk_buff *skb;
>> > > -	struct qca_serdev *qcadev;
>> > >  	u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
>> > >
>> > >  	if (baudrate > QCA_BAUDRATE_3200000)
>> > > @@ -977,13 +976,6 @@ static int qca_set_baudrate(struct hci_dev
>> > > *hdev, uint8_t baudrate)
>> > >  		return -ENOMEM;
>> > >  	}
>> > >
>> > > -	/* Disabling hardware flow control is mandatory while
>> > > -	 * sending change baudrate request to wcn3990 SoC.
>> > > -	 */
>> > > -	qcadev = serdev_device_get_drvdata(hu->serdev);
>> > > -	if (qcadev->btsoc_type == QCA_WCN3990)
>> > > -		hci_uart_set_flow_control(hu, true);
>> > > -
>> > >  	/* Assign commands to change baudrate and packet type. */
>> > >  	skb_put_data(skb, cmd, sizeof(cmd));
>> > >  	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
>> > > @@ -999,9 +991,6 @@ static int qca_set_baudrate(struct hci_dev
>> > > *hdev, uint8_t baudrate)
>> > >  	schedule_timeout(msecs_to_jiffies(BAUDRATE_SETTLE_TIMEOUT_MS));
>> > >  	set_current_state(TASK_RUNNING);
>> > >
>> > > -	if (qcadev->btsoc_type == QCA_WCN3990)
>> > > -		hci_uart_set_flow_control(hu, false);
>> > > -
>> > >  	return 0;
>> > >  }
>> > >
>> > > @@ -1086,6 +1075,7 @@ static int qca_check_speeds(struct hci_uart *hu)
>> > >  static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type
>> > > speed_type)
>> > >  {
>> > >  	unsigned int speed, qca_baudrate;
>> > > +	struct qca_serdev *qcadev;
>> > >  	int ret;
>> > >
>> > >  	if (speed_type == QCA_INIT_SPEED) {
>> > > @@ -1097,6 +1087,15 @@ static int qca_set_speed(struct hci_uart *hu,
>> > > enum qca_speed_type speed_type)
>> > >  		if (!speed)
>> > >  			return 0;
>> > >
>> > > +		/* Deassert RTS while changing the baudrate of chip and host.
>> > > +		 * This will prevent chip from transmitting its response with
>> > > +		 * the new baudrate while the host port is still operating at
>> > > +		 * the old speed.
>> > > +		 */
>> > > +		qcadev = serdev_device_get_drvdata(hu->serdev);
>> > > +		if (qcadev->btsoc_type == QCA_WCN3990)
>> > > +			serdev_device_set_rts(hu->serdev, false);
>> > > +
>> > >  		qca_baudrate = qca_get_baudrate_value(speed);
>> > >  		bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
>> > >  		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>> > > @@ -1104,6 +1103,9 @@ static int qca_set_speed(struct hci_uart *hu,
>> > > enum qca_speed_type speed_type)
>> > >  			return ret;
>> > >
>> > >  		host_set_baudrate(hu, speed);
>> > > +
>> > > +		if (qcadev->btsoc_type == QCA_WCN3990)
>> > > +			serdev_device_set_rts(hu->serdev, true);
>> > >  	}
>> > >
>> > >  	return 0;
>> >
>> > I looked for ways to do without this change, but didn't find a good
>> > solution. There are several possible problems with baudrate changes:
>> >
>> > 1) send request to BT controller to change the baudrate
>> >
>> >   this is an asynchronous operation, the actual baudrate change can
>> >   be delayed for multiple reasons, e.g.:
>> >
>> >   - request sits in the BT driver's TX queue
>> >
>> >     this could be worked around by checking skb_queue_empty()
>> >
>> >   - request sits in the UART buffer
>> >
>> >     a workaround for this could be calling
>> >     serdev_device_wait_until_sent() (only available with serdev though)
>> >
>> >   - the request sits in the UART FIFO
>> >
>> >     will be sent out 'immediately'. no neat solution available AFAIK,
>> >     a short sleep could be an effective workaround
>> >
>> >   - the controller may have a short delay to apply the change
>> >
>> >     Also no neat solution here. A/the same short sleep could work
>> >     around this
>> >
>> > 2) change baudrate of the host UART
>> >   - this must not happen before the baudrate change request has been
>> >     sent to the BT controller, otherwise things are messed up
>> >     seriously
>> >
>> >     Ideally set_termios would make sure all pending data is sent
>> >     before the change is applied, some UART drivers do this, others
>> >     don't, so we can't rely on this.
>> >
>> > 3) BT controller sends data after baudrate change
>> >
>> >   a few ms after a baudrate change the BT controller sends data
>> >   (4, 255, 2, 146, 1, 4, 14, 4, 1, 0, 0, 0) with the new baudrate
>> >
>> >   - dunno what the data stands for, but the BT stack/driver appears to
>> >     be fine with it, as long as the host UART operates at the new
>> >     baudrate when the data is received.
>> >
>> >   - if the data is received before the baudrate of the host UART is
>> >     changes we see 'frame reassembly' errors
>> >
>> >
>> [Bala]: the data is an vendor specific event and command complete 
>> event,
>>          4, 255, 2, 146, 1, : vendor specific event
>>          4, 14, 4, 1, 0, 0, 0: command complete event.
> 
> Thanks!
> 
>> > In summary, I think it should be feasible to guarantee that the
>> > baudrate change of the host UART is always done after the controller
>> > changed it's baudrate, however we can't guarantee at the same time
>> > that the baudrate change of the host controller is completed before
>> > the BT controller sends its 'response'.
>> >
>> > Using the RTS signal seems a reasonable way to delay the controller
>> > data until the host is ready, the only thing I don't like too much
>> > is that in this patch set we currently have two mechanisms to
>> > suppress/delay unwanted data. Unfortunately the RTS method isn't
>> > effective at initialization time.
>> >
>> > Not the scope of this patch set, but I really dislike the 300 ms delay
>> > (BAUDRATE_SETTLE_TIMEOUT_MS) in qca_set_baudrate(), and wonder if it
>> > is actually needed (I seriously doubt that it takes the BT controller
>> > 300 ms to change its baudrate). I guess it's more a combination of what
>> > I
>> > described above in 1), once we are done with this series I might try
>> > to improve this, unless somebody is really, really convinced that such
>> > a gigantic delay is actually needed.
>> >
>> [Bala]:  Thanks for detail analysis.
>>         even i feel the same whether is it really required to have an 
>> delay
>> of 300ms.
>>         But during our testing we found the it depends on the 
>> controller
>> clock settling time.
>>         all observations are less than 100 ms. will update this change 
>> in
>> separate patch series.
> 
> 100 ms is definitely better than 300 ms if that's not really
> needed. Did you see the need for a 100 ms delay with the WCN3990 or
> some other QCA controller?

[Bala]: i am not sure about other controller will check that. but for 
wcn3990 we can go
         with the 100ms.

> 
> Cheers
> 
> Matthias
Johan Hovold Jan. 9, 2019, 2:52 p.m. UTC | #5
On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
> This patch will help to stop frame reassembly errors while changing
> the baudrate. This is because host send a change baudrate request
> command to the chip with 115200 bps, Whereas chip will change their
> UART clocks to the enable for new baudrate and sends the response
> for the change request command with newer baudrate, On host side
> we are still operating in 115200 bps which results of reading garbage
> data. Here we are pulling RTS line, so that chip we will wait to send data
> to host until host change its baudrate.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/bluetooth/hci_qca.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 5a07c2370289..1680ead6cc3d 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -963,7 +963,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>  	struct hci_uart *hu = hci_get_drvdata(hdev);
>  	struct qca_data *qca = hu->priv;
>  	struct sk_buff *skb;
> -	struct qca_serdev *qcadev;
>  	u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
>  
>  	if (baudrate > QCA_BAUDRATE_3200000)
> @@ -977,13 +976,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>  		return -ENOMEM;
>  	}
>  
> -	/* Disabling hardware flow control is mandatory while
> -	 * sending change baudrate request to wcn3990 SoC.
> -	 */
> -	qcadev = serdev_device_get_drvdata(hu->serdev);
> -	if (qcadev->btsoc_type == QCA_WCN3990)
> -		hci_uart_set_flow_control(hu, true);
> -
>  	/* Assign commands to change baudrate and packet type. */
>  	skb_put_data(skb, cmd, sizeof(cmd));
>  	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
> @@ -999,9 +991,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>  	schedule_timeout(msecs_to_jiffies(BAUDRATE_SETTLE_TIMEOUT_MS));
>  	set_current_state(TASK_RUNNING);
>  
> -	if (qcadev->btsoc_type == QCA_WCN3990)
> -		hci_uart_set_flow_control(hu, false);
> -
>  	return 0;
>  }
>  
> @@ -1086,6 +1075,7 @@ static int qca_check_speeds(struct hci_uart *hu)
>  static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  {
>  	unsigned int speed, qca_baudrate;
> +	struct qca_serdev *qcadev;
>  	int ret;
>  
>  	if (speed_type == QCA_INIT_SPEED) {
> @@ -1097,6 +1087,15 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  		if (!speed)
>  			return 0;
>  
> +		/* Deassert RTS while changing the baudrate of chip and host.
> +		 * This will prevent chip from transmitting its response with
> +		 * the new baudrate while the host port is still operating at
> +		 * the old speed.
> +		 */
> +		qcadev = serdev_device_get_drvdata(hu->serdev);
> +		if (qcadev->btsoc_type == QCA_WCN3990)
> +			serdev_device_set_rts(hu->serdev, false);
> +

This may not do what you want unless you also disable hardware flow
control.

Johan
Balakrishna Godavarthi Jan. 10, 2019, 2:34 p.m. UTC | #6
Hi Johan,

On 2019-01-09 20:22, Johan Hovold wrote:
> On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
>> This patch will help to stop frame reassembly errors while changing
>> the baudrate. This is because host send a change baudrate request
>> command to the chip with 115200 bps, Whereas chip will change their
>> UART clocks to the enable for new baudrate and sends the response
>> for the change request command with newer baudrate, On host side
>> we are still operating in 115200 bps which results of reading garbage
>> data. Here we are pulling RTS line, so that chip we will wait to send 
>> data
>> to host until host change its baudrate.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> Tested-by: Matthias Kaehlcke <mka@chromium.org>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>  drivers/bluetooth/hci_qca.c | 24 +++++++++++++-----------
>>  1 file changed, 13 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 5a07c2370289..1680ead6cc3d 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -963,7 +963,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, 
>> uint8_t baudrate)
>>  	struct hci_uart *hu = hci_get_drvdata(hdev);
>>  	struct qca_data *qca = hu->priv;
>>  	struct sk_buff *skb;
>> -	struct qca_serdev *qcadev;
>>  	u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
>> 
>>  	if (baudrate > QCA_BAUDRATE_3200000)
>> @@ -977,13 +976,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, 
>> uint8_t baudrate)
>>  		return -ENOMEM;
>>  	}
>> 
>> -	/* Disabling hardware flow control is mandatory while
>> -	 * sending change baudrate request to wcn3990 SoC.
>> -	 */
>> -	qcadev = serdev_device_get_drvdata(hu->serdev);
>> -	if (qcadev->btsoc_type == QCA_WCN3990)
>> -		hci_uart_set_flow_control(hu, true);
>> -
>>  	/* Assign commands to change baudrate and packet type. */
>>  	skb_put_data(skb, cmd, sizeof(cmd));
>>  	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
>> @@ -999,9 +991,6 @@ static int qca_set_baudrate(struct hci_dev *hdev, 
>> uint8_t baudrate)
>>  	schedule_timeout(msecs_to_jiffies(BAUDRATE_SETTLE_TIMEOUT_MS));
>>  	set_current_state(TASK_RUNNING);
>> 
>> -	if (qcadev->btsoc_type == QCA_WCN3990)
>> -		hci_uart_set_flow_control(hu, false);
>> -
>>  	return 0;
>>  }
>> 
>> @@ -1086,6 +1075,7 @@ static int qca_check_speeds(struct hci_uart *hu)
>>  static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type 
>> speed_type)
>>  {
>>  	unsigned int speed, qca_baudrate;
>> +	struct qca_serdev *qcadev;
>>  	int ret;
>> 
>>  	if (speed_type == QCA_INIT_SPEED) {
>> @@ -1097,6 +1087,15 @@ static int qca_set_speed(struct hci_uart *hu, 
>> enum qca_speed_type speed_type)
>>  		if (!speed)
>>  			return 0;
>> 
>> +		/* Deassert RTS while changing the baudrate of chip and host.
>> +		 * This will prevent chip from transmitting its response with
>> +		 * the new baudrate while the host port is still operating at
>> +		 * the old speed.
>> +		 */
>> +		qcadev = serdev_device_get_drvdata(hu->serdev);
>> +		if (qcadev->btsoc_type == QCA_WCN3990)
>> +			serdev_device_set_rts(hu->serdev, false);
>> +
> 
> This may not do what you want unless you also disable hardware flow
> control.
> 
> Johan


Here my requirement here is to block the chip to send its data before 
HOST changes
it is baudrate. So if i disable flow control lines of HOST which will be 
in low state.
so that the chip will send it data before HOST change the baudrate of 
HOST. which results in
frame reassembly error.
Johan Hovold Jan. 10, 2019, 2:39 p.m. UTC | #7
On Thu, Jan 10, 2019 at 08:04:12PM +0530, Balakrishna Godavarthi wrote:
> Hi Johan,
> 
> On 2019-01-09 20:22, Johan Hovold wrote:
> > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
> >> This patch will help to stop frame reassembly errors while changing
> >> the baudrate. This is because host send a change baudrate request
> >> command to the chip with 115200 bps, Whereas chip will change their
> >> UART clocks to the enable for new baudrate and sends the response
> >> for the change request command with newer baudrate, On host side
> >> we are still operating in 115200 bps which results of reading garbage
> >> data. Here we are pulling RTS line, so that chip we will wait to send 
> >> data
> >> to host until host change its baudrate.

> >> +		/* Deassert RTS while changing the baudrate of chip and host.
> >> +		 * This will prevent chip from transmitting its response with
> >> +		 * the new baudrate while the host port is still operating at
> >> +		 * the old speed.
> >> +		 */
> >> +		qcadev = serdev_device_get_drvdata(hu->serdev);
> >> +		if (qcadev->btsoc_type == QCA_WCN3990)
> >> +			serdev_device_set_rts(hu->serdev, false);
> >> +
> > 
> > This may not do what you want unless you also disable hardware flow
> > control.

> Here my requirement here is to block the chip to send its data before
> HOST changes it is baudrate. So if i disable flow control lines of
> HOST which will be in low state.  so that the chip will send it data
> before HOST change the baudrate of HOST. which results in frame
> reassembly error.

Not sure I understand what you're trying to say above. My point is that
you cannot reliable control RTS when you have automatic flow control
enabled (i.e. it is managed by hardware and it's state reflects whether
there's room in the UART receive FIFO).

Johan
Balakrishna Godavarthi Jan. 10, 2019, 2:52 p.m. UTC | #8
Hi Johan,

On 2019-01-10 20:09, Johan Hovold wrote:
> On Thu, Jan 10, 2019 at 08:04:12PM +0530, Balakrishna Godavarthi wrote:
>> Hi Johan,
>> 
>> On 2019-01-09 20:22, Johan Hovold wrote:
>> > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
>> >> This patch will help to stop frame reassembly errors while changing
>> >> the baudrate. This is because host send a change baudrate request
>> >> command to the chip with 115200 bps, Whereas chip will change their
>> >> UART clocks to the enable for new baudrate and sends the response
>> >> for the change request command with newer baudrate, On host side
>> >> we are still operating in 115200 bps which results of reading garbage
>> >> data. Here we are pulling RTS line, so that chip we will wait to send
>> >> data
>> >> to host until host change its baudrate.
> 
>> >> +		/* Deassert RTS while changing the baudrate of chip and host.
>> >> +		 * This will prevent chip from transmitting its response with
>> >> +		 * the new baudrate while the host port is still operating at
>> >> +		 * the old speed.
>> >> +		 */
>> >> +		qcadev = serdev_device_get_drvdata(hu->serdev);
>> >> +		if (qcadev->btsoc_type == QCA_WCN3990)
>> >> +			serdev_device_set_rts(hu->serdev, false);
>> >> +
>> >
>> > This may not do what you want unless you also disable hardware flow
>> > control.
> 
>> Here my requirement here is to block the chip to send its data before
>> HOST changes it is baudrate. So if i disable flow control lines of
>> HOST which will be in low state.  so that the chip will send it data
>> before HOST change the baudrate of HOST. which results in frame
>> reassembly error.
> 
> Not sure I understand what you're trying to say above. My point is that
> you cannot reliable control RTS when you have automatic flow control
> enabled (i.e. it is managed by hardware and it's state reflects whether
> there's room in the UART receive FIFO).
> 
> Johan

[Bala]: Yes i got your point, but our driver will not support automatic 
flow control (based on the FIFO status)
         unless we explicitly enabled via software. i.e. if we enable the 
flow, hardware will look for it.
         else it will not looks for CTS or RTS Line.
Matthias Kaehlcke Jan. 11, 2019, 1:37 a.m. UTC | #9
On Thu, Jan 10, 2019 at 08:22:12PM +0530, Balakrishna Godavarthi wrote:
> Hi Johan,
> 
> On 2019-01-10 20:09, Johan Hovold wrote:
> > On Thu, Jan 10, 2019 at 08:04:12PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Johan,
> > > 
> > > On 2019-01-09 20:22, Johan Hovold wrote:
> > > > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
> > > >> This patch will help to stop frame reassembly errors while changing
> > > >> the baudrate. This is because host send a change baudrate request
> > > >> command to the chip with 115200 bps, Whereas chip will change their
> > > >> UART clocks to the enable for new baudrate and sends the response
> > > >> for the change request command with newer baudrate, On host side
> > > >> we are still operating in 115200 bps which results of reading garbage
> > > >> data. Here we are pulling RTS line, so that chip we will wait to send
> > > >> data
> > > >> to host until host change its baudrate.
> > 
> > > >> +		/* Deassert RTS while changing the baudrate of chip and host.
> > > >> +		 * This will prevent chip from transmitting its response with
> > > >> +		 * the new baudrate while the host port is still operating at
> > > >> +		 * the old speed.
> > > >> +		 */
> > > >> +		qcadev = serdev_device_get_drvdata(hu->serdev);
> > > >> +		if (qcadev->btsoc_type == QCA_WCN3990)
> > > >> +			serdev_device_set_rts(hu->serdev, false);
> > > >> +
> > > >
> > > > This may not do what you want unless you also disable hardware flow
> > > > control.
> > 
> > > Here my requirement here is to block the chip to send its data before
> > > HOST changes it is baudrate. So if i disable flow control lines of
> > > HOST which will be in low state.  so that the chip will send it data
> > > before HOST change the baudrate of HOST. which results in frame
> > > reassembly error.
> > 
> > Not sure I understand what you're trying to say above. My point is that
> > you cannot reliable control RTS when you have automatic flow control
> > enabled (i.e. it is managed by hardware and it's state reflects whether
> > there's room in the UART receive FIFO).
> > 
> > Johan
> 
> [Bala]: Yes i got your point, but our driver

I suppose with "our driver" you refer to a Qualcomm UART driver like
qcom_geni_serial.c. Unless the Bluetooth controller is really tied to
some specific SoC (e.g. because it is on-chip) you shouldn't make
assumptions about the UART driver or hardware beyond standard
behavior.

But even if we assume that the driver you mention is used, I think you
are rather confirming Johan's concern than dispersing it:

> will not support automatic flow control (based on the FIFO status)
> unless we explicitly enabled via software. i.e. if we enable the
> flow, hardware will look for it else it will not looks for CTS or
> RTS Line.

So we agree that the UART hardware may change RTS if hardware flow
control is enabled?

static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
{
  ...
  hci_uart_set_flow_control(hu, false);
  ...
}

I still find it utterly confusing that set_flow_control(false) enables
flow control, but that's what it does, hence after
qca_send_power_pulse() flow control is (re-)enabled.

So far I haven't seen problems with qcom_geni_serial.c overriding the
level set with serdev_device_set_rts(), but I tend to agree with Johan
that this could be a problem (if not with this UART (driver) then with
another). I'm not keen about adding more flow control on/off clutter,
but if that is needed for the driver to operate reliably across
platforms so be it.

Cheers

Matthias
Balakrishna Godavarthi Jan. 11, 2019, 3:07 p.m. UTC | #10
Hi Matthias,

On 2019-01-11 07:07, Matthias Kaehlcke wrote:
> On Thu, Jan 10, 2019 at 08:22:12PM +0530, Balakrishna Godavarthi wrote:
>> Hi Johan,
>> 
>> On 2019-01-10 20:09, Johan Hovold wrote:
>> > On Thu, Jan 10, 2019 at 08:04:12PM +0530, Balakrishna Godavarthi wrote:
>> > > Hi Johan,
>> > >
>> > > On 2019-01-09 20:22, Johan Hovold wrote:
>> > > > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
>> > > >> This patch will help to stop frame reassembly errors while changing
>> > > >> the baudrate. This is because host send a change baudrate request
>> > > >> command to the chip with 115200 bps, Whereas chip will change their
>> > > >> UART clocks to the enable for new baudrate and sends the response
>> > > >> for the change request command with newer baudrate, On host side
>> > > >> we are still operating in 115200 bps which results of reading garbage
>> > > >> data. Here we are pulling RTS line, so that chip we will wait to send
>> > > >> data
>> > > >> to host until host change its baudrate.
>> >
>> > > >> +		/* Deassert RTS while changing the baudrate of chip and host.
>> > > >> +		 * This will prevent chip from transmitting its response with
>> > > >> +		 * the new baudrate while the host port is still operating at
>> > > >> +		 * the old speed.
>> > > >> +		 */
>> > > >> +		qcadev = serdev_device_get_drvdata(hu->serdev);
>> > > >> +		if (qcadev->btsoc_type == QCA_WCN3990)
>> > > >> +			serdev_device_set_rts(hu->serdev, false);
>> > > >> +
>> > > >
>> > > > This may not do what you want unless you also disable hardware flow
>> > > > control.
>> >
>> > > Here my requirement here is to block the chip to send its data before
>> > > HOST changes it is baudrate. So if i disable flow control lines of
>> > > HOST which will be in low state.  so that the chip will send it data
>> > > before HOST change the baudrate of HOST. which results in frame
>> > > reassembly error.
>> >
>> > Not sure I understand what you're trying to say above. My point is that
>> > you cannot reliable control RTS when you have automatic flow control
>> > enabled (i.e. it is managed by hardware and it's state reflects whether
>> > there's room in the UART receive FIFO).
>> >
>> > Johan
>> 
>> [Bala]: Yes i got your point, but our driver
> 
> I suppose with "our driver" you refer to a Qualcomm UART driver like
> qcom_geni_serial.c. Unless the Bluetooth controller is really tied to
> some specific SoC (e.g. because it is on-chip) you shouldn't make
> assumptions about the UART driver or hardware beyond standard
> behavior.
> 
> But even if we assume that the driver you mention is used, I think you
> are rather confirming Johan's concern than dispersing it:
> 

[Bala]: now understood the point.

>> will not support automatic flow control (based on the FIFO status)
>> unless we explicitly enabled via software. i.e. if we enable the
>> flow, hardware will look for it else it will not looks for CTS or
>> RTS Line.
> 
> So we agree that the UART hardware may change RTS if hardware flow
> control is enabled?
> 
> static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
> {
>   ...
>   hci_uart_set_flow_control(hu, false);
>   ...
> }
> 
> I still find it utterly confusing that set_flow_control(false) enables
> flow control, but that's what it does, hence after
> qca_send_power_pulse() flow control is (re-)enabled.
> 
> So far I haven't seen problems with qcom_geni_serial.c overriding the
> level set with serdev_device_set_rts(), but I tend to agree with Johan
> that this could be a problem (if not with this UART (driver) then with
> another). I'm not keen about adding more flow control on/off clutter,
> but if that is needed for the driver to operate reliably across
> platforms so be it.
> 
> Cheers
> 
> Matthias

[Bala]: previously we have disabling the flow control, that is not 
pulling the RTS line if it disabled.
         so that the reason we are explicilty pulling it by calling 
set_rts() with false.

         Johan concern can be fixed either of two ways.

         1. disable the flow control, but the uart driver should pull the 
RTS line high. as the line is unused
         2. disable the flow control and call set_rts with false that 
will helps us to pull the RTS line.

will experiment more on this and post the change.
Matthias Kaehlcke Jan. 11, 2019, 11:56 p.m. UTC | #11
On Fri, Jan 11, 2019 at 08:37:12PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2019-01-11 07:07, Matthias Kaehlcke wrote:
> > On Thu, Jan 10, 2019 at 08:22:12PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Johan,
> > > 
> > > On 2019-01-10 20:09, Johan Hovold wrote:
> > > > On Thu, Jan 10, 2019 at 08:04:12PM +0530, Balakrishna Godavarthi wrote:
> > > > > Hi Johan,
> > > > >
> > > > > On 2019-01-09 20:22, Johan Hovold wrote:
> > > > > > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
> > > > > >> This patch will help to stop frame reassembly errors while changing
> > > > > >> the baudrate. This is because host send a change baudrate request
> > > > > >> command to the chip with 115200 bps, Whereas chip will change their
> > > > > >> UART clocks to the enable for new baudrate and sends the response
> > > > > >> for the change request command with newer baudrate, On host side
> > > > > >> we are still operating in 115200 bps which results of reading garbage
> > > > > >> data. Here we are pulling RTS line, so that chip we will wait to send
> > > > > >> data
> > > > > >> to host until host change its baudrate.
> > > >
> > > > > >> +		/* Deassert RTS while changing the baudrate of chip and host.
> > > > > >> +		 * This will prevent chip from transmitting its response with
> > > > > >> +		 * the new baudrate while the host port is still operating at
> > > > > >> +		 * the old speed.
> > > > > >> +		 */
> > > > > >> +		qcadev = serdev_device_get_drvdata(hu->serdev);
> > > > > >> +		if (qcadev->btsoc_type == QCA_WCN3990)
> > > > > >> +			serdev_device_set_rts(hu->serdev, false);
> > > > > >> +
> > > > > >
> > > > > > This may not do what you want unless you also disable hardware flow
> > > > > > control.
> > > >
> > > > > Here my requirement here is to block the chip to send its data before
> > > > > HOST changes it is baudrate. So if i disable flow control lines of
> > > > > HOST which will be in low state.  so that the chip will send it data
> > > > > before HOST change the baudrate of HOST. which results in frame
> > > > > reassembly error.
> > > >
> > > > Not sure I understand what you're trying to say above. My point is that
> > > > you cannot reliable control RTS when you have automatic flow control
> > > > enabled (i.e. it is managed by hardware and it's state reflects whether
> > > > there's room in the UART receive FIFO).
> > > >
> > > > Johan
> > > 
> > > [Bala]: Yes i got your point, but our driver
> > 
> > I suppose with "our driver" you refer to a Qualcomm UART driver like
> > qcom_geni_serial.c. Unless the Bluetooth controller is really tied to
> > some specific SoC (e.g. because it is on-chip) you shouldn't make
> > assumptions about the UART driver or hardware beyond standard
> > behavior.
> > 
> > But even if we assume that the driver you mention is used, I think you
> > are rather confirming Johan's concern than dispersing it:
> > 
> 
> [Bala]: now understood the point.
> 
> > > will not support automatic flow control (based on the FIFO status)
> > > unless we explicitly enabled via software. i.e. if we enable the
> > > flow, hardware will look for it else it will not looks for CTS or
> > > RTS Line.
> > 
> > So we agree that the UART hardware may change RTS if hardware flow
> > control is enabled?
> > 
> > static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
> > {
> >   ...
> >   hci_uart_set_flow_control(hu, false);
> >   ...
> > }
> > 
> > I still find it utterly confusing that set_flow_control(false) enables
> > flow control, but that's what it does, hence after
> > qca_send_power_pulse() flow control is (re-)enabled.
> > 
> > So far I haven't seen problems with qcom_geni_serial.c overriding the
> > level set with serdev_device_set_rts(), but I tend to agree with Johan
> > that this could be a problem (if not with this UART (driver) then with
> > another). I'm not keen about adding more flow control on/off clutter,
> > but if that is needed for the driver to operate reliably across
> > platforms so be it.
> > 
> > Cheers
> > 
> > Matthias
> 
> [Bala]: previously we have disabling the flow control, that is not pulling
> the RTS line if it disabled.
>         so that the reason we are explicilty pulling it by calling set_rts()
> with false.
> 
>         Johan concern can be fixed either of two ways.
> 
>         1. disable the flow control, but the uart driver should pull the RTS
> line high. as the line is unused
>         2. disable the flow control and call set_rts with false that will
> helps us to pull the RTS line.

I don't think you can rely on 1. You might succeed to convince a
specific UART driver/hardware to do this, however you'd have to ensure
the same behavior on all other types of UARTs that could be used in
combination with the chip, which doesn't seem feasible.
In case the hardware completely relinquishes control of the RTS pin
upon disabling flow control the state of the signal could depend on
the pin configuration, i.e. whether Linux (or the bootloader) enables
a pull-up/down, which may vary across boards, even if they use the
same SoC.

I think it will have to be the second option.

Cheers

Matthias
Balakrishna Godavarthi Jan. 14, 2019, 2:52 p.m. UTC | #12
Hi Matthias,

On 2019-01-12 05:26, Matthias Kaehlcke wrote:
> On Fri, Jan 11, 2019 at 08:37:12PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2019-01-11 07:07, Matthias Kaehlcke wrote:
>> > On Thu, Jan 10, 2019 at 08:22:12PM +0530, Balakrishna Godavarthi wrote:
>> > > Hi Johan,
>> > >
>> > > On 2019-01-10 20:09, Johan Hovold wrote:
>> > > > On Thu, Jan 10, 2019 at 08:04:12PM +0530, Balakrishna Godavarthi wrote:
>> > > > > Hi Johan,
>> > > > >
>> > > > > On 2019-01-09 20:22, Johan Hovold wrote:
>> > > > > > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
>> > > > > >> This patch will help to stop frame reassembly errors while changing
>> > > > > >> the baudrate. This is because host send a change baudrate request
>> > > > > >> command to the chip with 115200 bps, Whereas chip will change their
>> > > > > >> UART clocks to the enable for new baudrate and sends the response
>> > > > > >> for the change request command with newer baudrate, On host side
>> > > > > >> we are still operating in 115200 bps which results of reading garbage
>> > > > > >> data. Here we are pulling RTS line, so that chip we will wait to send
>> > > > > >> data
>> > > > > >> to host until host change its baudrate.
>> > > >
>> > > > > >> +		/* Deassert RTS while changing the baudrate of chip and host.
>> > > > > >> +		 * This will prevent chip from transmitting its response with
>> > > > > >> +		 * the new baudrate while the host port is still operating at
>> > > > > >> +		 * the old speed.
>> > > > > >> +		 */
>> > > > > >> +		qcadev = serdev_device_get_drvdata(hu->serdev);
>> > > > > >> +		if (qcadev->btsoc_type == QCA_WCN3990)
>> > > > > >> +			serdev_device_set_rts(hu->serdev, false);
>> > > > > >> +
>> > > > > >
>> > > > > > This may not do what you want unless you also disable hardware flow
>> > > > > > control.
>> > > >
>> > > > > Here my requirement here is to block the chip to send its data before
>> > > > > HOST changes it is baudrate. So if i disable flow control lines of
>> > > > > HOST which will be in low state.  so that the chip will send it data
>> > > > > before HOST change the baudrate of HOST. which results in frame
>> > > > > reassembly error.
>> > > >
>> > > > Not sure I understand what you're trying to say above. My point is that
>> > > > you cannot reliable control RTS when you have automatic flow control
>> > > > enabled (i.e. it is managed by hardware and it's state reflects whether
>> > > > there's room in the UART receive FIFO).
>> > > >
>> > > > Johan
>> > >
>> > > [Bala]: Yes i got your point, but our driver
>> >
>> > I suppose with "our driver" you refer to a Qualcomm UART driver like
>> > qcom_geni_serial.c. Unless the Bluetooth controller is really tied to
>> > some specific SoC (e.g. because it is on-chip) you shouldn't make
>> > assumptions about the UART driver or hardware beyond standard
>> > behavior.
>> >
>> > But even if we assume that the driver you mention is used, I think you
>> > are rather confirming Johan's concern than dispersing it:
>> >
>> 
>> [Bala]: now understood the point.
>> 
>> > > will not support automatic flow control (based on the FIFO status)
>> > > unless we explicitly enabled via software. i.e. if we enable the
>> > > flow, hardware will look for it else it will not looks for CTS or
>> > > RTS Line.
>> >
>> > So we agree that the UART hardware may change RTS if hardware flow
>> > control is enabled?
>> >
>> > static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
>> > {
>> >   ...
>> >   hci_uart_set_flow_control(hu, false);
>> >   ...
>> > }
>> >
>> > I still find it utterly confusing that set_flow_control(false) enables
>> > flow control, but that's what it does, hence after
>> > qca_send_power_pulse() flow control is (re-)enabled.
>> >
>> > So far I haven't seen problems with qcom_geni_serial.c overriding the
>> > level set with serdev_device_set_rts(), but I tend to agree with Johan
>> > that this could be a problem (if not with this UART (driver) then with
>> > another). I'm not keen about adding more flow control on/off clutter,
>> > but if that is needed for the driver to operate reliably across
>> > platforms so be it.
>> >
>> > Cheers
>> >
>> > Matthias
>> 
>> [Bala]: previously we have disabling the flow control, that is not 
>> pulling
>> the RTS line if it disabled.
>>         so that the reason we are explicilty pulling it by calling 
>> set_rts()
>> with false.
>> 
>>         Johan concern can be fixed either of two ways.
>> 
>>         1. disable the flow control, but the uart driver should pull 
>> the RTS
>> line high. as the line is unused
>>         2. disable the flow control and call set_rts with false that 
>> will
>> helps us to pull the RTS line.
> 
> I don't think you can rely on 1. You might succeed to convince a
> specific UART driver/hardware to do this, however you'd have to ensure
> the same behavior on all other types of UARTs that could be used in
> combination with the chip, which doesn't seem feasible.
> In case the hardware completely relinquishes control of the RTS pin
> upon disabling flow control the state of the signal could depend on
> the pin configuration, i.e. whether Linux (or the bootloader) enables
> a pull-up/down, which may vary across boards, even if they use the
> same SoC.
> 
> I think it will have to be the second option.
> 
> Cheers
> 
> Matthias

[Bala]: i have tried option 2, but sill i see frame reassembly errors.
         1. disabling flow control
         2. pull RTS

         But even we have dynamic flow control, we will not have any 
issue.
         let us assume we have an dynamic flow control and RTS line is 
pulled high.
         but the during this stage for sure our FIFO we not be full, 
because this is an init sequence.

         01 48 fc 01 11(command we send and wait here for 300 ms)
         04 ff 02 92 01(command specific event)
         04 0e 04 01 00 00 00(command complete event)

        so we will only have 5 bytes to be sent. i don't think dynamic 
flow control will not active.

        I have an doubt that if HOST supports dynamic flow control, how 
would it helps BT chip it may cause the byte corruption.
        here is the scenario, if the chip is not ready to accept any data 
from the HOST where as host as lot of data to sent,
        then enabling dynamic flow control will cause an data loss 
between BT chip and HOST>
Matthias Kaehlcke Jan. 15, 2019, 11:46 p.m. UTC | #13
On Mon, Jan 14, 2019 at 08:22:12PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2019-01-12 05:26, Matthias Kaehlcke wrote:
> > On Fri, Jan 11, 2019 at 08:37:12PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > > 
> > > On 2019-01-11 07:07, Matthias Kaehlcke wrote:
> > > > On Thu, Jan 10, 2019 at 08:22:12PM +0530, Balakrishna Godavarthi wrote:
> > > > > Hi Johan,
> > > > >
> > > > > On 2019-01-10 20:09, Johan Hovold wrote:
> > > > > > On Thu, Jan 10, 2019 at 08:04:12PM +0530, Balakrishna Godavarthi wrote:
> > > > > > > Hi Johan,
> > > > > > >
> > > > > > > On 2019-01-09 20:22, Johan Hovold wrote:
> > > > > > > > On Thu, Dec 20, 2018 at 08:16:36PM +0530, Balakrishna Godavarthi wrote:
> > > > > > > >> This patch will help to stop frame reassembly errors while changing
> > > > > > > >> the baudrate. This is because host send a change baudrate request
> > > > > > > >> command to the chip with 115200 bps, Whereas chip will change their
> > > > > > > >> UART clocks to the enable for new baudrate and sends the response
> > > > > > > >> for the change request command with newer baudrate, On host side
> > > > > > > >> we are still operating in 115200 bps which results of reading garbage
> > > > > > > >> data. Here we are pulling RTS line, so that chip we will wait to send
> > > > > > > >> data
> > > > > > > >> to host until host change its baudrate.
> > > > > >
> > > > > > > >> +		/* Deassert RTS while changing the baudrate of chip and host.
> > > > > > > >> +		 * This will prevent chip from transmitting its response with
> > > > > > > >> +		 * the new baudrate while the host port is still operating at
> > > > > > > >> +		 * the old speed.
> > > > > > > >> +		 */
> > > > > > > >> +		qcadev = serdev_device_get_drvdata(hu->serdev);
> > > > > > > >> +		if (qcadev->btsoc_type == QCA_WCN3990)
> > > > > > > >> +			serdev_device_set_rts(hu->serdev, false);
> > > > > > > >> +
> > > > > > > >
> > > > > > > > This may not do what you want unless you also disable hardware flow
> > > > > > > > control.
> > > > > >
> > > > > > > Here my requirement here is to block the chip to send its data before
> > > > > > > HOST changes it is baudrate. So if i disable flow control lines of
> > > > > > > HOST which will be in low state.  so that the chip will send it data
> > > > > > > before HOST change the baudrate of HOST. which results in frame
> > > > > > > reassembly error.
> > > > > >
> > > > > > Not sure I understand what you're trying to say above. My point is that
> > > > > > you cannot reliable control RTS when you have automatic flow control
> > > > > > enabled (i.e. it is managed by hardware and it's state reflects whether
> > > > > > there's room in the UART receive FIFO).
> > > > > >
> > > > > > Johan
> > > > >
> > > > > [Bala]: Yes i got your point, but our driver
> > > >
> > > > I suppose with "our driver" you refer to a Qualcomm UART driver like
> > > > qcom_geni_serial.c. Unless the Bluetooth controller is really tied to
> > > > some specific SoC (e.g. because it is on-chip) you shouldn't make
> > > > assumptions about the UART driver or hardware beyond standard
> > > > behavior.
> > > >
> > > > But even if we assume that the driver you mention is used, I think you
> > > > are rather confirming Johan's concern than dispersing it:
> > > >
> > > 
> > > [Bala]: now understood the point.
> > > 
> > > > > will not support automatic flow control (based on the FIFO status)
> > > > > unless we explicitly enabled via software. i.e. if we enable the
> > > > > flow, hardware will look for it else it will not looks for CTS or
> > > > > RTS Line.
> > > >
> > > > So we agree that the UART hardware may change RTS if hardware flow
> > > > control is enabled?
> > > >
> > > > static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
> > > > {
> > > >   ...
> > > >   hci_uart_set_flow_control(hu, false);
> > > >   ...
> > > > }
> > > >
> > > > I still find it utterly confusing that set_flow_control(false) enables
> > > > flow control, but that's what it does, hence after
> > > > qca_send_power_pulse() flow control is (re-)enabled.
> > > >
> > > > So far I haven't seen problems with qcom_geni_serial.c overriding the
> > > > level set with serdev_device_set_rts(), but I tend to agree with Johan
> > > > that this could be a problem (if not with this UART (driver) then with
> > > > another). I'm not keen about adding more flow control on/off clutter,
> > > > but if that is needed for the driver to operate reliably across
> > > > platforms so be it.
> > > >
> > > > Cheers
> > > >
> > > > Matthias
> > > 
> > > [Bala]: previously we have disabling the flow control, that is not
> > > pulling
> > > the RTS line if it disabled.
> > >         so that the reason we are explicilty pulling it by calling
> > > set_rts()
> > > with false.
> > > 
> > >         Johan concern can be fixed either of two ways.
> > > 
> > >         1. disable the flow control, but the uart driver should pull
> > > the RTS
> > > line high. as the line is unused
> > >         2. disable the flow control and call set_rts with false that
> > > will
> > > helps us to pull the RTS line.
> > 
> > I don't think you can rely on 1. You might succeed to convince a
> > specific UART driver/hardware to do this, however you'd have to ensure
> > the same behavior on all other types of UARTs that could be used in
> > combination with the chip, which doesn't seem feasible.
> > In case the hardware completely relinquishes control of the RTS pin
> > upon disabling flow control the state of the signal could depend on
> > the pin configuration, i.e. whether Linux (or the bootloader) enables
> > a pull-up/down, which may vary across boards, even if they use the
> > same SoC.
> > 
> > I think it will have to be the second option.
> > 
> > Cheers
> > 
> > Matthias
> 
> [Bala]: i have tried option 2, but sill i see frame reassembly errors.
>         1. disabling flow control
>         2. pull RTS

I can reproduce this. Apparently the geni serial port doesn't raise
RTS when flow control is disabled, even when told to do so. I don't
know if this is a bug or just undefined behavior when flow control is
disabled (e.g. the port might not even have a RTS signal
(configured)).

>         But even we have dynamic flow control, we will not have any issue.
>         let us assume we have an dynamic flow control and RTS line is pulled
> high.
>         but the during this stage for sure our FIFO we not be full, because
> this is an init sequence.
> 
>         01 48 fc 01 11(command we send and wait here for 300 ms)
>         04 ff 02 92 01(command specific event)
>         04 0e 04 01 00 00 00(command complete event)
> 
>        so we will only have 5 bytes to be sent. i don't think dynamic flow
> control will not active.

Flow control will always be active unless we disable it, I think you
are referring to the status of the RTS signal.

It seems you are talking about the TX FIFO ("5 bytes to be sent"),
however that shouldn't affect RTS.

I believe Johan is referring to the case where the port/driver asserts
RTS, because it is ready to receive more data.

>        I have an doubt that if HOST supports dynamic flow control, how would
> it helps BT chip it may cause the byte corruption.
>        here is the scenario, if the chip is not ready to accept any data
> from the HOST where as host as lot of data to sent,
>        then enabling dynamic flow control will cause an data loss between BT
> chip and HOST>

Enabled flow control is supposed to prevent that situation, I imagine
you refer to disabled flow control? That is a potential problem.

Using RTS seems|ed like a nice solutions, since it's the native way to
prevent the controller from sending data, instead of doing some custom
hack. However Johan seems to be fairly convinced that flow control and
manual toggling of RTS can be problematic, and we have seen that
disabling flow control has its own problems. Maybe it's time to
consider other solutions like the DISCARD_RX flag we discussed
earlier. Not that I really liked this custom solution, but in the end
it might be a more robust way to address the issue.

Johan/Marcel/others: Do you have any further ideas or input on this?

Thanks

Matthias
Johan Hovold Jan. 17, 2019, 4:09 p.m. UTC | #14
On Tue, Jan 15, 2019 at 03:46:28PM -0800, Matthias Kaehlcke wrote:

> Using RTS seems|ed like a nice solutions, since it's the native way to
> prevent the controller from sending data, instead of doing some custom
> hack. However Johan seems to be fairly convinced that flow control and
> manual toggling of RTS can be problematic, and we have seen that
> disabling flow control has its own problems. Maybe it's time to
> consider other solutions like the DISCARD_RX flag we discussed
> earlier. Not that I really liked this custom solution, but in the end
> it might be a more robust way to address the issue.
> 
> Johan/Marcel/others: Do you have any further ideas or input on this?

I don't see why deasserting RTS wouldn't work, well at least as long as
the RTS signal is wired correctly.

My point was simply that calling serdev_device_set_rts() will generally
not work unless you first disable automatic (i.e. hw-managed) flow
control using serdev_device_set_flow_control(). The exact behaviour is
serial-controller dependent, but I assume the driver needs to be
platform agnostic.

Johan
Matthias Kaehlcke Jan. 17, 2019, 5:21 p.m. UTC | #15
On Thu, Jan 17, 2019 at 05:09:44PM +0100, Johan Hovold wrote:
> On Tue, Jan 15, 2019 at 03:46:28PM -0800, Matthias Kaehlcke wrote:
> 
> > Using RTS seems|ed like a nice solutions, since it's the native way to
> > prevent the controller from sending data, instead of doing some custom
> > hack. However Johan seems to be fairly convinced that flow control and
> > manual toggling of RTS can be problematic, and we have seen that
> > disabling flow control has its own problems. Maybe it's time to
> > consider other solutions like the DISCARD_RX flag we discussed
> > earlier. Not that I really liked this custom solution, but in the end
> > it might be a more robust way to address the issue.
> > 
> > Johan/Marcel/others: Do you have any further ideas or input on this?
> 
> I don't see why deasserting RTS wouldn't work, well at least as long as
> the RTS signal is wired correctly.
> 
> My point was simply that calling serdev_device_set_rts() will generally
> not work unless you first disable automatic (i.e. hw-managed) flow
> control using serdev_device_set_flow_control(). The exact behaviour is
> serial-controller dependent, but I assume the driver needs to be
> platform agnostic.

I observed that the qcom_geni_serial driver doesn't raise RTS with
flow control disabled. It seems we have to investigate why that's the
case. I agree that the driver should be platform agnostic.

Cheers

Matthias
Johan Hovold Jan. 18, 2019, 9:44 a.m. UTC | #16
On Thu, Jan 17, 2019 at 09:21:09AM -0800, Matthias Kaehlcke wrote:

> I observed that the qcom_geni_serial driver doesn't raise RTS with
> flow control disabled. It seems we have to investigate why that's the
> case. I agree that the driver should be platform agnostic.

Sounds like a driver bug, unless the hardware is just "odd". The
driver implementation of this looks very non-standard judging from a
quick peek.

In fact, qcom_geni_serial_get_mctrl() is currently a no-op if hardware
flow control is not enabled:

	if (uart_console(uport) || !uart_cts_enabled(uport))
		return;

Perhaps dropping the !uart_cts_enabled() test is sufficient.

Johan
Matthias Kaehlcke Jan. 19, 2019, 12:31 a.m. UTC | #17
On Fri, Jan 18, 2019 at 10:44:16AM +0100, Johan Hovold wrote:
> On Thu, Jan 17, 2019 at 09:21:09AM -0800, Matthias Kaehlcke wrote:
> 
> > I observed that the qcom_geni_serial driver doesn't raise RTS with
> > flow control disabled. It seems we have to investigate why that's the
> > case. I agree that the driver should be platform agnostic.
> 
> Sounds like a driver bug, unless the hardware is just "odd". The
> driver implementation of this looks very non-standard judging from a
> quick peek.
> 
> In fact, qcom_geni_serial_get_mctrl() is currently a no-op if hardware
> flow control is not enabled:
> 
> 	if (uart_console(uport) || !uart_cts_enabled(uport))
> 		return;
> 
> Perhaps dropping the !uart_cts_enabled() test is sufficient.

Thanks for taking a look Johan, that was indeed the problem (also
in set_mctrl()). I posted a fix: https://lore.kernel.org/patchwork/patch/1033611/

Balakrishna, the following (applied on top of your patch) works for me
with the UART patch above:

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 9d5e41f159c78f..60bfdf01f72841 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1080,7 +1080,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 {
 	unsigned int speed, qca_baudrate;
 	struct qca_serdev *qcadev;
-	int ret;
+	int ret = 0;
 
 	if (speed_type == QCA_INIT_SPEED) {
 		speed = qca_get_speed(hu, QCA_INIT_SPEED);
@@ -1097,22 +1097,27 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 		 * the old speed.
 		 */
 		qcadev = serdev_device_get_drvdata(hu->serdev);
-		if (qcadev->btsoc_type == QCA_WCN3990)
+		if (qcadev->btsoc_type == QCA_WCN3990) {
+			hci_uart_set_flow_control(hu, true);
 			serdev_device_set_rts(hu->serdev, false);
+		}
 
 		qca_baudrate = qca_get_baudrate_value(speed);
 		bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
 		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
 		if (ret)
-			return ret;
+			goto out;
 
 		host_set_baudrate(hu, speed);
 
-		if (qcadev->btsoc_type == QCA_WCN3990)
+out:
+		if (qcadev->btsoc_type == QCA_WCN3990) {
+			hci_uart_set_flow_control(hu, false);
 			serdev_device_set_rts(hu->serdev, true);
+		}
 	}
 
-	return 0;
+	return ret;
 }
 
 static int qca_wcn3990_init(struct hci_uart *hu)
Johan Hovold Jan. 21, 2019, 8:56 a.m. UTC | #18
On Fri, Jan 18, 2019 at 04:31:09PM -0800, Matthias Kaehlcke wrote:
> On Fri, Jan 18, 2019 at 10:44:16AM +0100, Johan Hovold wrote:
> > On Thu, Jan 17, 2019 at 09:21:09AM -0800, Matthias Kaehlcke wrote:
> > 
> > > I observed that the qcom_geni_serial driver doesn't raise RTS with
> > > flow control disabled. It seems we have to investigate why that's the
> > > case. I agree that the driver should be platform agnostic.
> > 
> > Sounds like a driver bug, unless the hardware is just "odd". The
> > driver implementation of this looks very non-standard judging from a
> > quick peek.
> > 
> > In fact, qcom_geni_serial_get_mctrl() is currently a no-op if hardware
> > flow control is not enabled:
> > 
> > 	if (uart_console(uport) || !uart_cts_enabled(uport))
> > 		return;
> > 
> > Perhaps dropping the !uart_cts_enabled() test is sufficient.
> 
> Thanks for taking a look Johan, that was indeed the problem (also
> in set_mctrl()). I posted a fix: https://lore.kernel.org/patchwork/patch/1033611/

Nice (I did mean set_mctrl() above, as I think you noticed).

> Balakrishna, the following (applied on top of your patch) works for me
> with the UART patch above:
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 9d5e41f159c78f..60bfdf01f72841 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1080,7 +1080,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  {
>  	unsigned int speed, qca_baudrate;
>  	struct qca_serdev *qcadev;
> -	int ret;
> +	int ret = 0;
>  
>  	if (speed_type == QCA_INIT_SPEED) {
>  		speed = qca_get_speed(hu, QCA_INIT_SPEED);
> @@ -1097,22 +1097,27 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  		 * the old speed.
>  		 */
>  		qcadev = serdev_device_get_drvdata(hu->serdev);
> -		if (qcadev->btsoc_type == QCA_WCN3990)
> +		if (qcadev->btsoc_type == QCA_WCN3990) {
> +			hci_uart_set_flow_control(hu, true);

Wow, yeah, this parameter inversion is indeed confusing...

>  			serdev_device_set_rts(hu->serdev, false);
> +		}

But looking at hci_uart_set_flow_control() now, it actually also
deasserts RTS. So all you need here is the hci_uart_set_flow_control()
call.  

And that makes the inversion make a bit more sense too, even if the
naming is a bit unfortunate with respect to
serdev_device_set_flow_control() at least.

>  		qca_baudrate = qca_get_baudrate_value(speed);
>  		bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
>  		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>  		if (ret)
> -			return ret;
> +			goto out;
>  
>  		host_set_baudrate(hu, speed);
>  
> -		if (qcadev->btsoc_type == QCA_WCN3990)
> +out:
> +		if (qcadev->btsoc_type == QCA_WCN3990) {
> +			hci_uart_set_flow_control(hu, false);
>  			serdev_device_set_rts(hu->serdev, true);
> +		}

And same here.

Johan
Balakrishna Godavarthi Jan. 21, 2019, 2:41 p.m. UTC | #19
Hi Matthias,

On 2019-01-19 06:01, Matthias Kaehlcke wrote:
> On Fri, Jan 18, 2019 at 10:44:16AM +0100, Johan Hovold wrote:
>> On Thu, Jan 17, 2019 at 09:21:09AM -0800, Matthias Kaehlcke wrote:
>> 
>> > I observed that the qcom_geni_serial driver doesn't raise RTS with
>> > flow control disabled. It seems we have to investigate why that's the
>> > case. I agree that the driver should be platform agnostic.
>> 
>> Sounds like a driver bug, unless the hardware is just "odd". The
>> driver implementation of this looks very non-standard judging from a
>> quick peek.
>> 
>> In fact, qcom_geni_serial_get_mctrl() is currently a no-op if hardware
>> flow control is not enabled:
>> 
>> 	if (uart_console(uport) || !uart_cts_enabled(uport))
>> 		return;
>> 
>> Perhaps dropping the !uart_cts_enabled() test is sufficient.
> 
> Thanks for taking a look Johan, that was indeed the problem (also
> in set_mctrl()). I posted a fix:
> https://lore.kernel.org/patchwork/patch/1033611/
> 
> Balakrishna, the following (applied on top of your patch) works for me
> with the UART patch above:
> 

[Bala]: will test and update BT patch accordingly.


> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 9d5e41f159c78f..60bfdf01f72841 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1080,7 +1080,7 @@ static int qca_set_speed(struct hci_uart *hu,
> enum qca_speed_type speed_type)
>  {
>  	unsigned int speed, qca_baudrate;
>  	struct qca_serdev *qcadev;
> -	int ret;
> +	int ret = 0;
> 
>  	if (speed_type == QCA_INIT_SPEED) {
>  		speed = qca_get_speed(hu, QCA_INIT_SPEED);
> @@ -1097,22 +1097,27 @@ static int qca_set_speed(struct hci_uart *hu,
> enum qca_speed_type speed_type)
>  		 * the old speed.
>  		 */
>  		qcadev = serdev_device_get_drvdata(hu->serdev);
> -		if (qcadev->btsoc_type == QCA_WCN3990)
> +		if (qcadev->btsoc_type == QCA_WCN3990) {
> +			hci_uart_set_flow_control(hu, true);
>  			serdev_device_set_rts(hu->serdev, false);
> +		}
> 
>  		qca_baudrate = qca_get_baudrate_value(speed);
>  		bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
>  		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>  		if (ret)
> -			return ret;
> +			goto out;
> 
>  		host_set_baudrate(hu, speed);
> 
> -		if (qcadev->btsoc_type == QCA_WCN3990)
> +out:
> +		if (qcadev->btsoc_type == QCA_WCN3990) {
> +			hci_uart_set_flow_control(hu, false);
>  			serdev_device_set_rts(hu->serdev, true);
> +		}
>  	}
> 
> -	return 0;
> +	return ret;
>  }
> 
>  static int qca_wcn3990_init(struct hci_uart *hu)
Matthias Kaehlcke Jan. 22, 2019, 9:39 p.m. UTC | #20
On Mon, Jan 21, 2019 at 09:56:13AM +0100, Johan Hovold wrote:
> On Fri, Jan 18, 2019 at 04:31:09PM -0800, Matthias Kaehlcke wrote:
> > On Fri, Jan 18, 2019 at 10:44:16AM +0100, Johan Hovold wrote:
> > > On Thu, Jan 17, 2019 at 09:21:09AM -0800, Matthias Kaehlcke wrote:
> > > 
> > > > I observed that the qcom_geni_serial driver doesn't raise RTS with
> > > > flow control disabled. It seems we have to investigate why that's the
> > > > case. I agree that the driver should be platform agnostic.
> > > 
> > > Sounds like a driver bug, unless the hardware is just "odd". The
> > > driver implementation of this looks very non-standard judging from a
> > > quick peek.
> > > 
> > > In fact, qcom_geni_serial_get_mctrl() is currently a no-op if hardware
> > > flow control is not enabled:
> > > 
> > > 	if (uart_console(uport) || !uart_cts_enabled(uport))
> > > 		return;
> > > 
> > > Perhaps dropping the !uart_cts_enabled() test is sufficient.
> > 
> > Thanks for taking a look Johan, that was indeed the problem (also
> > in set_mctrl()). I posted a fix: https://lore.kernel.org/patchwork/patch/1033611/
> 
> Nice (I did mean set_mctrl() above, as I think you noticed).
> 
> > Balakrishna, the following (applied on top of your patch) works for me
> > with the UART patch above:
> > 
> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > index 9d5e41f159c78f..60bfdf01f72841 100644
> > --- a/drivers/bluetooth/hci_qca.c
> > +++ b/drivers/bluetooth/hci_qca.c
> > @@ -1080,7 +1080,7 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
> >  {
> >  	unsigned int speed, qca_baudrate;
> >  	struct qca_serdev *qcadev;
> > -	int ret;
> > +	int ret = 0;
> >  
> >  	if (speed_type == QCA_INIT_SPEED) {
> >  		speed = qca_get_speed(hu, QCA_INIT_SPEED);
> > @@ -1097,22 +1097,27 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
> >  		 * the old speed.
> >  		 */
> >  		qcadev = serdev_device_get_drvdata(hu->serdev);
> > -		if (qcadev->btsoc_type == QCA_WCN3990)
> > +		if (qcadev->btsoc_type == QCA_WCN3990) {
> > +			hci_uart_set_flow_control(hu, true);
> 
> Wow, yeah, this parameter inversion is indeed confusing...
> 
> >  			serdev_device_set_rts(hu->serdev, false);
> > +		}
> 
> But looking at hci_uart_set_flow_control() now, it actually also
> deasserts RTS. So all you need here is the hci_uart_set_flow_control()
> call.  

Great, thanks for pointing that out!

> And that makes the inversion make a bit more sense too, even if the
> naming is a bit unfortunate with respect to
> serdev_device_set_flow_control() at least.
Matthias Kaehlcke Jan. 22, 2019, 9:53 p.m. UTC | #21
On Mon, Jan 21, 2019 at 08:11:39PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2019-01-19 06:01, Matthias Kaehlcke wrote:
> > On Fri, Jan 18, 2019 at 10:44:16AM +0100, Johan Hovold wrote:
> > > On Thu, Jan 17, 2019 at 09:21:09AM -0800, Matthias Kaehlcke wrote:
> > > 
> > > > I observed that the qcom_geni_serial driver doesn't raise RTS with
> > > > flow control disabled. It seems we have to investigate why that's the
> > > > case. I agree that the driver should be platform agnostic.
> > > 
> > > Sounds like a driver bug, unless the hardware is just "odd". The
> > > driver implementation of this looks very non-standard judging from a
> > > quick peek.
> > > 
> > > In fact, qcom_geni_serial_get_mctrl() is currently a no-op if hardware
> > > flow control is not enabled:
> > > 
> > > 	if (uart_console(uport) || !uart_cts_enabled(uport))
> > > 		return;
> > > 
> > > Perhaps dropping the !uart_cts_enabled() test is sufficient.
> > 
> > Thanks for taking a look Johan, that was indeed the problem (also
> > in set_mctrl()). I posted a fix:
> > https://lore.kernel.org/patchwork/patch/1033611/
> > 
> > Balakrishna, the following (applied on top of your patch) works for me
> > with the UART patch above:
> > 
> 
> [Bala]: will test and update BT patch accordingly.

Note that Johan pointed out that hci_uart_set_flow_control() deasserts
RTS when flow control is disabled, so the _set_rts() calls can be
removed. With that only a single action needs to be undone in case of
an error, you can consider to keep the return instead of using the
goto introduced by my patch.

Please keep/adapt the "Deassert RTS while changing the baudrate ..."
comment to leave it clear to posterity why flow control is disabled
during baudrate changes. It's fairly obvious once you understand the
problem and that disabling flow control deasserts RTS, but it took us
a while to get there, initially we only had a comment saying
"disabling flow control is mandatory" (I recall I inquired about this
multiple times during the initial review of the wcn3990 patches ;-)

Thanks

Matthias
Balakrishna Godavarthi Jan. 23, 2019, 12:57 p.m. UTC | #22
Hi Matthias,

On 2019-01-23 03:23, Matthias Kaehlcke wrote:
> On Mon, Jan 21, 2019 at 08:11:39PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2019-01-19 06:01, Matthias Kaehlcke wrote:
>> > On Fri, Jan 18, 2019 at 10:44:16AM +0100, Johan Hovold wrote:
>> > > On Thu, Jan 17, 2019 at 09:21:09AM -0800, Matthias Kaehlcke wrote:
>> > >
>> > > > I observed that the qcom_geni_serial driver doesn't raise RTS with
>> > > > flow control disabled. It seems we have to investigate why that's the
>> > > > case. I agree that the driver should be platform agnostic.
>> > >
>> > > Sounds like a driver bug, unless the hardware is just "odd". The
>> > > driver implementation of this looks very non-standard judging from a
>> > > quick peek.
>> > >
>> > > In fact, qcom_geni_serial_get_mctrl() is currently a no-op if hardware
>> > > flow control is not enabled:
>> > >
>> > > 	if (uart_console(uport) || !uart_cts_enabled(uport))
>> > > 		return;
>> > >
>> > > Perhaps dropping the !uart_cts_enabled() test is sufficient.
>> >
>> > Thanks for taking a look Johan, that was indeed the problem (also
>> > in set_mctrl()). I posted a fix:
>> > https://lore.kernel.org/patchwork/patch/1033611/
>> >
>> > Balakrishna, the following (applied on top of your patch) works for me
>> > with the UART patch above:
>> >
>> 
>> [Bala]: will test and update BT patch accordingly.
> 
> Note that Johan pointed out that hci_uart_set_flow_control() deasserts
> RTS when flow control is disabled, so the _set_rts() calls can be
> removed. With that only a single action needs to be undone in case of
> an error, you can consider to keep the return instead of using the
> goto introduced by my patch.
> 

[Bala]: ok sure. will note this.

> Please keep/adapt the "Deassert RTS while changing the baudrate ..."
> comment to leave it clear to posterity why flow control is disabled
> during baudrate changes. It's fairly obvious once you understand the
> problem and that disabling flow control deasserts RTS, but it took us
> a while to get there, initially we only had a comment saying
> "disabling flow control is mandatory" (I recall I inquired about this
> multiple times during the initial review of the wcn3990 patches ;-)
> 
> Thanks
> 
> Matthias

Patch
diff mbox series

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 5a07c2370289..1680ead6cc3d 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -963,7 +963,6 @@  static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
 	struct hci_uart *hu = hci_get_drvdata(hdev);
 	struct qca_data *qca = hu->priv;
 	struct sk_buff *skb;
-	struct qca_serdev *qcadev;
 	u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
 
 	if (baudrate > QCA_BAUDRATE_3200000)
@@ -977,13 +976,6 @@  static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
 		return -ENOMEM;
 	}
 
-	/* Disabling hardware flow control is mandatory while
-	 * sending change baudrate request to wcn3990 SoC.
-	 */
-	qcadev = serdev_device_get_drvdata(hu->serdev);
-	if (qcadev->btsoc_type == QCA_WCN3990)
-		hci_uart_set_flow_control(hu, true);
-
 	/* Assign commands to change baudrate and packet type. */
 	skb_put_data(skb, cmd, sizeof(cmd));
 	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
@@ -999,9 +991,6 @@  static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
 	schedule_timeout(msecs_to_jiffies(BAUDRATE_SETTLE_TIMEOUT_MS));
 	set_current_state(TASK_RUNNING);
 
-	if (qcadev->btsoc_type == QCA_WCN3990)
-		hci_uart_set_flow_control(hu, false);
-
 	return 0;
 }
 
@@ -1086,6 +1075,7 @@  static int qca_check_speeds(struct hci_uart *hu)
 static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 {
 	unsigned int speed, qca_baudrate;
+	struct qca_serdev *qcadev;
 	int ret;
 
 	if (speed_type == QCA_INIT_SPEED) {
@@ -1097,6 +1087,15 @@  static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 		if (!speed)
 			return 0;
 
+		/* Deassert RTS while changing the baudrate of chip and host.
+		 * This will prevent chip from transmitting its response with
+		 * the new baudrate while the host port is still operating at
+		 * the old speed.
+		 */
+		qcadev = serdev_device_get_drvdata(hu->serdev);
+		if (qcadev->btsoc_type == QCA_WCN3990)
+			serdev_device_set_rts(hu->serdev, false);
+
 		qca_baudrate = qca_get_baudrate_value(speed);
 		bt_dev_dbg(hu->hdev, "Set UART speed to %d", speed);
 		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
@@ -1104,6 +1103,9 @@  static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 			return ret;
 
 		host_set_baudrate(hu, speed);
+
+		if (qcadev->btsoc_type == QCA_WCN3990)
+			serdev_device_set_rts(hu->serdev, true);
 	}
 
 	return 0;