Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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 -- Regards Balakrishna.
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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.
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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) -- Regards Balakrishna.
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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)
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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]: prev
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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 hardwar
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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. -- Regards Balakrishna.
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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. -- Regards Balakrishna.
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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 Tested-by: Matthias Kaehlcke Reviewed-by: Matthias Kaehlcke --- 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_320) @@ -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. -- Regards Balakrishna.
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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 > Tested-by: Matthias Kaehlcke > Reviewed-by: Matthias Kaehlcke > --- > 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_320) > @@ -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
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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 > > Tested-by: Matthias Kaehlcke > > Reviewed-by: Matthias Kaehlcke > > --- > > 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_320) > > @@ -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
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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 > > > Tested-by: Matthias Kaehlcke > > > Reviewed-by: Matthias Kaehlcke > > > --- > > > 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_320) > > > @@ -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 hav
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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 Tested-by: Matthias Kaehlcke Reviewed-by: Matthias Kaehlcke --- 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_320) @@ -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
Re: [PATCH v5 2/5] Bluetooth: hci_qca: Deassert RTS while baudrate change command
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 > Tested-by: Matthias Kaehlcke > Reviewed-by: Matthias Kaehlcke > --- > 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_320) > @@ -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