| Message ID | 1590546759-27387-1-git-send-email-zijuhu@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] bluetooth: hci_qca: Fix qca6390 enable failure after warm reboot | expand |
Hi Zijun, On Wed, May 27, 2020 at 10:32:39AM +0800, Zijun Hu wrote: > Warm reboot can not restore qca6390 controller baudrate > to default due to lack of controllable BT_EN pin or power > supply, so fails to download firmware after warm reboot. > > Fixed by sending EDL_SOC_RESET VSC to reset controller > within added device shutdown implementation. > > Signed-off-by: Zijun Hu <zijuhu@codeaurora.org> > --- > drivers/bluetooth/hci_qca.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index e4a6823..4b6f8b6 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -1975,6 +1975,34 @@ static void qca_serdev_remove(struct serdev_device *serdev) > hci_uart_unregister_device(&qcadev->serdev_hu); > } > > +static void qca_serdev_shutdown(struct device *dev) > +{ > + int ret; > + int timeout = msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS); > + struct serdev_device *serdev = to_serdev_device(dev); > + struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev); > + const u8 ibs_wake_cmd[] = { 0xFD }; > + const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 }; > + > + if (qcadev->btsoc_type == QCA_QCA6390) { > + serdev_device_write_flush(serdev); > + serdev_device_write_buf(serdev, > + ibs_wake_cmd, sizeof(ibs_wake_cmd)); > + serdev_device_wait_until_sent(serdev, timeout); Why no check of the return value of serdev_device_write_buf() here, does it make sense to continue if sending the wakeup command failed? Couldn't serdev_device_write() be used instead of the _write_buf() + _wait_until_sent() combo? > + usleep_range(8000, 10000); > + > + serdev_device_write_flush(serdev); I suppose the flush is done because _wait_until_sent() could have timed out. Another reason to use _device_write() (if suitable), since it returns -ETIMEDOUT in that case? > + ret = serdev_device_write_buf(serdev, > + edl_reset_soc_cmd, sizeof(edl_reset_soc_cmd)); > + if (ret < 0) { > + BT_ERR("QCA send EDL_RESET_REQ error: %d", ret); > + return; > + } > + serdev_device_wait_until_sent(serdev, timeout); > + usleep_range(8000, 10000); > + } > +} > + > static int __maybe_unused qca_suspend(struct device *dev) > { > struct hci_dev *hdev = container_of(dev, struct hci_dev, dev); > @@ -2100,6 +2128,7 @@ static struct serdev_device_driver qca_serdev_driver = { > .name = "hci_uart_qca", > .of_match_table = of_match_ptr(qca_bluetooth_of_match), > .acpi_match_table = ACPI_PTR(qca_bluetooth_acpi_match), > + .shutdown = qca_serdev_shutdown, > .pm = &qca_pm_ops, > }, > }; > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project >
On 5/28/2020 12:48 AM, Matthias Kaehlcke wrote: > Hi Zijun, > > On Wed, May 27, 2020 at 10:32:39AM +0800, Zijun Hu wrote: >> Warm reboot can not restore qca6390 controller baudrate >> to default due to lack of controllable BT_EN pin or power >> supply, so fails to download firmware after warm reboot. >> >> Fixed by sending EDL_SOC_RESET VSC to reset controller >> within added device shutdown implementation. >> >> Signed-off-by: Zijun Hu <zijuhu@codeaurora.org> >> --- >> drivers/bluetooth/hci_qca.c | 29 +++++++++++++++++++++++++++++ >> 1 file changed, 29 insertions(+) >> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >> index e4a6823..4b6f8b6 100644 >> --- a/drivers/bluetooth/hci_qca.c >> +++ b/drivers/bluetooth/hci_qca.c >> @@ -1975,6 +1975,34 @@ static void qca_serdev_remove(struct serdev_device *serdev) >> hci_uart_unregister_device(&qcadev->serdev_hu); >> } >> >> +static void qca_serdev_shutdown(struct device *dev) >> +{ >> + int ret; >> + int timeout = msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS); >> + struct serdev_device *serdev = to_serdev_device(dev); >> + struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev); >> + const u8 ibs_wake_cmd[] = { 0xFD }; >> + const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 }; >> + >> + if (qcadev->btsoc_type == QCA_QCA6390) { >> + serdev_device_write_flush(serdev); >> + serdev_device_write_buf(serdev, >> + ibs_wake_cmd, sizeof(ibs_wake_cmd)); >> + serdev_device_wait_until_sent(serdev, timeout); > > Why no check of the return value of serdev_device_write_buf() here, > does it make sense to continue if sending the wakeup command failed? > i will correct it at v4 patch > Couldn't serdev_device_write() be used instead of the _write_buf() + > _wait_until_sent() combo? > i don't think so, serdev_device_write() is not appropriate at here. serdev_device_write_wakeup() should be used to release completion hold by serdev_device_write(), however @hci_serdev_client_ops doesn't use serdev_device_write_wakeup() to implement its write_wakeup operation. we don't want to touch common hci_serdev.c code. >> + usleep_range(8000, 10000); >> + >> + serdev_device_write_flush(serdev); > > I suppose the flush is done because _wait_until_sent() could have timed out. > Another reason to use _device_write() (if suitable), since it returns > -ETIMEDOUT in that case? > flush is prefixed at write operation to speed up shutdown procedure in case of unexpected data injected during waiting for controller wakeup. the combo have been used and i just follow it>> + ret = serdev_device_write_buf(serdev, >> + edl_reset_soc_cmd, sizeof(edl_reset_soc_cmd)); >> + if (ret < 0) { >> + BT_ERR("QCA send EDL_RESET_REQ error: %d", ret); >> + return; >> + } >> + serdev_device_wait_until_sent(serdev, timeout); >> + usleep_range(8000, 10000); >> + } >> +} >> + >> static int __maybe_unused qca_suspend(struct device *dev) >> { >> struct hci_dev *hdev = container_of(dev, struct hci_dev, dev); >> @@ -2100,6 +2128,7 @@ static struct serdev_device_driver qca_serdev_driver = { >> .name = "hci_uart_qca", >> .of_match_table = of_match_ptr(qca_bluetooth_of_match), >> .acpi_match_table = ACPI_PTR(qca_bluetooth_acpi_match), >> + .shutdown = qca_serdev_shutdown, >> .pm = &qca_pm_ops, >> }, >> }; >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project >>
On Thu, May 28, 2020 at 01:04:25PM +0800, Zijun Hu wrote: > > > On 5/28/2020 12:48 AM, Matthias Kaehlcke wrote: > > Hi Zijun, > > > > On Wed, May 27, 2020 at 10:32:39AM +0800, Zijun Hu wrote: > >> Warm reboot can not restore qca6390 controller baudrate > >> to default due to lack of controllable BT_EN pin or power > >> supply, so fails to download firmware after warm reboot. > >> > >> Fixed by sending EDL_SOC_RESET VSC to reset controller > >> within added device shutdown implementation. > >> > >> Signed-off-by: Zijun Hu <zijuhu@codeaurora.org> > >> --- > >> drivers/bluetooth/hci_qca.c | 29 +++++++++++++++++++++++++++++ > >> 1 file changed, 29 insertions(+) > >> > >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > >> index e4a6823..4b6f8b6 100644 > >> --- a/drivers/bluetooth/hci_qca.c > >> +++ b/drivers/bluetooth/hci_qca.c > >> @@ -1975,6 +1975,34 @@ static void qca_serdev_remove(struct serdev_device *serdev) > >> hci_uart_unregister_device(&qcadev->serdev_hu); > >> } > >> > >> +static void qca_serdev_shutdown(struct device *dev) > >> +{ > >> + int ret; > >> + int timeout = msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS); > >> + struct serdev_device *serdev = to_serdev_device(dev); > >> + struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev); > >> + const u8 ibs_wake_cmd[] = { 0xFD }; > >> + const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 }; > >> + > >> + if (qcadev->btsoc_type == QCA_QCA6390) { > >> + serdev_device_write_flush(serdev); > >> + serdev_device_write_buf(serdev, > >> + ibs_wake_cmd, sizeof(ibs_wake_cmd)); > >> + serdev_device_wait_until_sent(serdev, timeout); > > > > Why no check of the return value of serdev_device_write_buf() here, > > does it make sense to continue if sending the wakeup command failed? > > > i will correct it at v4 patch > > Couldn't serdev_device_write() be used instead of the _write_buf() + > > _wait_until_sent() combo? > > > i don't think so, serdev_device_write() is not appropriate at here. > serdev_device_write_wakeup() should be used to release completion hold > by serdev_device_write(), however @hci_serdev_client_ops doesn't use > serdev_device_write_wakeup() to implement its write_wakeup operation. > we don't want to touch common hci_serdev.c code. Thanks for the clarification! > >> + usleep_range(8000, 10000); > >> + > >> + serdev_device_write_flush(serdev); > > > > I suppose the flush is done because _wait_until_sent() could have timed out. > > Another reason to use _device_write() (if suitable), since it returns > > -ETIMEDOUT in that case? > > > flush is prefixed at write operation to speed up > shutdown procedure in case of unexpected data injected > during waiting for controller wakeup. hm, wouldn't it be a bug if unexpected data is injected during shutdown? It seems it would be better to detect such a problem and fix the root cause, rather than papering over it. Also, a flush doesn't really guarantee that there is no unexpected data when serdev_device_write_buf() is called, it could be injected just after returning from _flush().
On 5/28/2020 11:44 PM, Matthias Kaehlcke wrote: > On Thu, May 28, 2020 at 01:04:25PM +0800, Zijun Hu wrote: >> >> >> On 5/28/2020 12:48 AM, Matthias Kaehlcke wrote: >>> Hi Zijun, >>> >>> On Wed, May 27, 2020 at 10:32:39AM +0800, Zijun Hu wrote: >>>> Warm reboot can not restore qca6390 controller baudrate >>>> to default due to lack of controllable BT_EN pin or power >>>> supply, so fails to download firmware after warm reboot. >>>> >>>> Fixed by sending EDL_SOC_RESET VSC to reset controller >>>> within added device shutdown implementation. >>>> >>>> Signed-off-by: Zijun Hu <zijuhu@codeaurora.org> >>>> --- >>>> drivers/bluetooth/hci_qca.c | 29 +++++++++++++++++++++++++++++ >>>> 1 file changed, 29 insertions(+) >>>> >>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c >>>> index e4a6823..4b6f8b6 100644 >>>> --- a/drivers/bluetooth/hci_qca.c >>>> +++ b/drivers/bluetooth/hci_qca.c >>>> @@ -1975,6 +1975,34 @@ static void qca_serdev_remove(struct serdev_device *serdev) >>>> hci_uart_unregister_device(&qcadev->serdev_hu); >>>> } >>>> >>>> +static void qca_serdev_shutdown(struct device *dev) >>>> +{ >>>> + int ret; >>>> + int timeout = msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS); >>>> + struct serdev_device *serdev = to_serdev_device(dev); >>>> + struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev); >>>> + const u8 ibs_wake_cmd[] = { 0xFD }; >>>> + const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 }; >>>> + >>>> + if (qcadev->btsoc_type == QCA_QCA6390) { >>>> + serdev_device_write_flush(serdev); >>>> + serdev_device_write_buf(serdev, >>>> + ibs_wake_cmd, sizeof(ibs_wake_cmd)); >>>> + serdev_device_wait_until_sent(serdev, timeout); >>> >>> Why no check of the return value of serdev_device_write_buf() here, >>> does it make sense to continue if sending the wakeup command failed? >>> >> i will correct it at v4 patch >>> Couldn't serdev_device_write() be used instead of the _write_buf() + >>> _wait_until_sent() combo? >>> >> i don't think so, serdev_device_write() is not appropriate at here. >> serdev_device_write_wakeup() should be used to release completion hold >> by serdev_device_write(), however @hci_serdev_client_ops doesn't use >> serdev_device_write_wakeup() to implement its write_wakeup operation. >> we don't want to touch common hci_serdev.c code. > > Thanks for the clarification! > >>>> + usleep_range(8000, 10000); >>>> + >>>> + serdev_device_write_flush(serdev); >>> >>> I suppose the flush is done because _wait_until_sent() could have timed out. >>> Another reason to use _device_write() (if suitable), since it returns >>> -ETIMEDOUT in that case? >>> >> flush is prefixed at write operation to speed up >> shutdown procedure in case of unexpected data injected >> during waiting for controller wakeup. > > hm, wouldn't it be a bug if unexpected data is injected during shutdown? It > seems it would be better to detect such a problem and fix the root cause, > rather than papering over it. > > Also, a flush doesn't really guarantee that there is no unexpected data when > serdev_device_write_buf() is called, it could be injected just after returning > from _flush(). > actually, we never see these unexpected data injection scenario and it is impossible to happen theoretically. the main purpose of prefixing flush before the secondary writing is to make code look more perfect and harmonious visually. BTW, i have updated this patch to v5 version in order to fix these issue pointed.
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c index e4a6823..4b6f8b6 100644 --- a/drivers/bluetooth/hci_qca.c +++ b/drivers/bluetooth/hci_qca.c @@ -1975,6 +1975,34 @@ static void qca_serdev_remove(struct serdev_device *serdev) hci_uart_unregister_device(&qcadev->serdev_hu); } +static void qca_serdev_shutdown(struct device *dev) +{ + int ret; + int timeout = msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS); + struct serdev_device *serdev = to_serdev_device(dev); + struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev); + const u8 ibs_wake_cmd[] = { 0xFD }; + const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 }; + + if (qcadev->btsoc_type == QCA_QCA6390) { + serdev_device_write_flush(serdev); + serdev_device_write_buf(serdev, + ibs_wake_cmd, sizeof(ibs_wake_cmd)); + serdev_device_wait_until_sent(serdev, timeout); + usleep_range(8000, 10000); + + serdev_device_write_flush(serdev); + ret = serdev_device_write_buf(serdev, + edl_reset_soc_cmd, sizeof(edl_reset_soc_cmd)); + if (ret < 0) { + BT_ERR("QCA send EDL_RESET_REQ error: %d", ret); + return; + } + serdev_device_wait_until_sent(serdev, timeout); + usleep_range(8000, 10000); + } +} + static int __maybe_unused qca_suspend(struct device *dev) { struct hci_dev *hdev = container_of(dev, struct hci_dev, dev); @@ -2100,6 +2128,7 @@ static struct serdev_device_driver qca_serdev_driver = { .name = "hci_uart_qca", .of_match_table = of_match_ptr(qca_bluetooth_of_match), .acpi_match_table = ACPI_PTR(qca_bluetooth_acpi_match), + .shutdown = qca_serdev_shutdown, .pm = &qca_pm_ops, }, };
Warm reboot can not restore qca6390 controller baudrate to default due to lack of controllable BT_EN pin or power supply, so fails to download firmware after warm reboot. Fixed by sending EDL_SOC_RESET VSC to reset controller within added device shutdown implementation. Signed-off-by: Zijun Hu <zijuhu@codeaurora.org> --- drivers/bluetooth/hci_qca.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)