| Message ID | 20181220144639.15928-3-bgodavar@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Bug fixes for Qualcomm BT chip wcn3990 | expand |
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
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
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
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
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
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.
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
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.
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
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.
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
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>
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
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
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
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
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)
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
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)
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.
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
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
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;