| Message ID | 20200605114552.1.I7bcad9d672455473177ddbc7db08cc1adcdee1dc@changeid (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Marcel Holtmann |
| Headers | show |
| Series | | expand |
Hi, On Fri, Jun 5, 2020 at 11:46 AM Matthias Kaehlcke <mka@chromium.org> wrote: > > qca_suspend() removes the vote for the UART TX clock after > writing an IBS sleep request to the serial buffer. This is > not a good idea since there is no guarantee that the request > has been sent at this point. Instead remove the vote after > successfully entering IBS sleep. This also fixes the issue > of the vote being removed in case of an aborted suspend due > to a failure of entering IBS sleep. > > Fixes: 41d5b25fed0a0 ("Bluetooth: hci_qca: add PM support") > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > drivers/bluetooth/hci_qca.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index ece9f91cc3deb..b1d82d32892e9 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -2083,8 +2083,6 @@ static int __maybe_unused qca_suspend(struct device *dev) > > qca->tx_ibs_state = HCI_IBS_TX_ASLEEP; > qca->ibs_sent_slps++; > - > - qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); > break; > > case HCI_IBS_TX_ASLEEP: > @@ -2112,8 +2110,10 @@ static int __maybe_unused qca_suspend(struct device *dev) > qca->rx_ibs_state == HCI_IBS_RX_ASLEEP, > msecs_to_jiffies(IBS_BTSOC_TX_IDLE_TIMEOUT_MS)); > > - if (ret > 0) > + if (ret > 0) { > + qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); > return 0; > + } > > if (ret == 0) > ret = -ETIMEDOUT; > -- > 2.27.0.278.ge193c7cf3a9-goog > I checked the order of calls and it looks correct per commit message. Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Hi Matthias, On 2020-06-06 00:16, Matthias Kaehlcke wrote: > qca_suspend() removes the vote for the UART TX clock after > writing an IBS sleep request to the serial buffer. This is > not a good idea since there is no guarantee that the request > has been sent at this point. Instead remove the vote after > successfully entering IBS sleep. This also fixes the issue > of the vote being removed in case of an aborted suspend due > to a failure of entering IBS sleep. > > Fixes: 41d5b25fed0a0 ("Bluetooth: hci_qca: add PM support") > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > drivers/bluetooth/hci_qca.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index ece9f91cc3deb..b1d82d32892e9 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -2083,8 +2083,6 @@ static int __maybe_unused qca_suspend(struct > device *dev) > > qca->tx_ibs_state = HCI_IBS_TX_ASLEEP; > qca->ibs_sent_slps++; > - > - qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); > break; > > case HCI_IBS_TX_ASLEEP: > @@ -2112,8 +2110,10 @@ static int __maybe_unused qca_suspend(struct > device *dev) > qca->rx_ibs_state == HCI_IBS_RX_ASLEEP, > msecs_to_jiffies(IBS_BTSOC_TX_IDLE_TIMEOUT_MS)); > > - if (ret > 0) > + if (ret > 0) { > + qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); [Bala]: qca_wq_serial_tx_clock_vote_off votes for Tx clock off, when both Tx clock and Rx clock voted to off. then only actual call to clock off is called. https://elixir.bootlin.com/linux/latest/source/drivers/bluetooth/hci_qca.c#L312 I would recommend to vote Tx clock off after sending SLEEP BYTE from HOST TO BT SOC. > return 0; > + } > > if (ret == 0) > ret = -ETIMEDOUT;
Hi Bala, On Sat, Jun 06, 2020 at 06:23:13PM +0530, bgodavar@codeaurora.org wrote: > Hi Matthias, > > On 2020-06-06 00:16, Matthias Kaehlcke wrote: > > qca_suspend() removes the vote for the UART TX clock after > > writing an IBS sleep request to the serial buffer. This is > > not a good idea since there is no guarantee that the request > > has been sent at this point. Instead remove the vote after > > successfully entering IBS sleep. This also fixes the issue > > of the vote being removed in case of an aborted suspend due > > to a failure of entering IBS sleep. > > > > Fixes: 41d5b25fed0a0 ("Bluetooth: hci_qca: add PM support") > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > > --- > > > > drivers/bluetooth/hci_qca.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > > index ece9f91cc3deb..b1d82d32892e9 100644 > > --- a/drivers/bluetooth/hci_qca.c > > +++ b/drivers/bluetooth/hci_qca.c > > @@ -2083,8 +2083,6 @@ static int __maybe_unused qca_suspend(struct > > device *dev) > > > > qca->tx_ibs_state = HCI_IBS_TX_ASLEEP; > > qca->ibs_sent_slps++; > > - > > - qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); > > break; > > > > case HCI_IBS_TX_ASLEEP: > > @@ -2112,8 +2110,10 @@ static int __maybe_unused qca_suspend(struct > > device *dev) > > qca->rx_ibs_state == HCI_IBS_RX_ASLEEP, > > msecs_to_jiffies(IBS_BTSOC_TX_IDLE_TIMEOUT_MS)); > > > > - if (ret > 0) > > + if (ret > 0) { > > + qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); > [Bala]: qca_wq_serial_tx_clock_vote_off votes for Tx clock off, when both Tx > clock and Rx clock voted to off. > then only actual call to clock off is called. > https://elixir.bootlin.com/linux/latest/source/drivers/bluetooth/hci_qca.c#L312 > I would recommend to vote Tx clock off after sending SLEEP BYTE from HOST TO > BT SOC. Are you suggesting to move the vote after _wait_until_sent() and before waiting for RX_SLEEP? I think if the vote is done before RX_SLEEP and going to RX_SLEEP fails the variables qca->tx_vote and qca->tx_votes_off would have the wrong state, even if that doesn't lead to actually switching the clock off. I might be missing something though, I'm not very familiar with this part.
Hi Matthias, > qca_suspend() removes the vote for the UART TX clock after > writing an IBS sleep request to the serial buffer. This is > not a good idea since there is no guarantee that the request > has been sent at this point. Instead remove the vote after > successfully entering IBS sleep. This also fixes the issue > of the vote being removed in case of an aborted suspend due > to a failure of entering IBS sleep. > > Fixes: 41d5b25fed0a0 ("Bluetooth: hci_qca: add PM support") > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > drivers/bluetooth/hci_qca.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) patch has been applied to bluetooth-next tree. Regards Marcel
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c index ece9f91cc3deb..b1d82d32892e9 100644 --- a/drivers/bluetooth/hci_qca.c +++ b/drivers/bluetooth/hci_qca.c @@ -2083,8 +2083,6 @@ static int __maybe_unused qca_suspend(struct device *dev) qca->tx_ibs_state = HCI_IBS_TX_ASLEEP; qca->ibs_sent_slps++; - - qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); break; case HCI_IBS_TX_ASLEEP: @@ -2112,8 +2110,10 @@ static int __maybe_unused qca_suspend(struct device *dev) qca->rx_ibs_state == HCI_IBS_RX_ASLEEP, msecs_to_jiffies(IBS_BTSOC_TX_IDLE_TIMEOUT_MS)); - if (ret > 0) + if (ret > 0) { + qca_wq_serial_tx_clock_vote_off(&qca->ws_tx_vote_off); return 0; + } if (ret == 0) ret = -ETIMEDOUT;
qca_suspend() removes the vote for the UART TX clock after writing an IBS sleep request to the serial buffer. This is not a good idea since there is no guarantee that the request has been sent at this point. Instead remove the vote after successfully entering IBS sleep. This also fixes the issue of the vote being removed in case of an aborted suspend due to a failure of entering IBS sleep. Fixes: 41d5b25fed0a0 ("Bluetooth: hci_qca: add PM support") Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- drivers/bluetooth/hci_qca.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)