diff mbox series

[1/1] Bluetooth: btusb: mediatek: add a recovery method for MT7922

Message ID 20231215063714.7684-1-hao.qin@mediatek.com (mailing list archive)
State New, archived
Headers show
Series [1/1] Bluetooth: btusb: mediatek: add a recovery method for MT7922 | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint success Gitlint PASS
tedd_an/SubjectPrefix success Gitlint PASS
tedd_an/BuildKernel success BuildKernel PASS
tedd_an/CheckAllWarning success CheckAllWarning PASS
tedd_an/CheckSparse success CheckSparse PASS
tedd_an/CheckSmatch success CheckSparse PASS
tedd_an/BuildKernel32 success BuildKernel32 PASS
tedd_an/TestRunnerSetup success TestRunnerSetup PASS
tedd_an/TestRunner_l2cap-tester success TestRunner PASS
tedd_an/TestRunner_iso-tester success TestRunner PASS
tedd_an/TestRunner_bnep-tester success TestRunner PASS
tedd_an/TestRunner_mgmt-tester fail TestRunner_mgmt-tester: Total: 497, Passed: 495 (99.6%), Failed: 1, Not Run: 1
tedd_an/TestRunner_rfcomm-tester success TestRunner PASS
tedd_an/TestRunner_sco-tester success TestRunner PASS
tedd_an/TestRunner_ioctl-tester success TestRunner PASS
tedd_an/TestRunner_mesh-tester success TestRunner PASS
tedd_an/TestRunner_smp-tester success TestRunner PASS
tedd_an/TestRunner_userchan-tester success TestRunner PASS
tedd_an/IncrementalBuild success Incremental Build PASS

Commit Message

Hao Qin Dec. 15, 2023, 6:37 a.m. UTC 
From: "hao.qin" <hao.qin@mediatek.com>

For MT7922, perform a reset before patch download to avoid
unexpected problems caused by inconsistency between upper layer
status and dongle status. Add USB reset retry to recover from
unexpected patch download failure.

Signed-off-by: hao.qin <hao.qin@mediatek.com>
---
 drivers/bluetooth/btusb.c | 66 +++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 20 deletions(-)

Comments

bluez.test.bot@gmail.com Dec. 15, 2023, 7:32 a.m. UTC | #1 
This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=810339

---Test result---

Test Summary:
CheckPatch                    PASS      0.68 seconds
GitLint                       PASS      0.68 seconds
SubjectPrefix                 PASS      0.13 seconds
BuildKernel                   PASS      27.89 seconds
CheckAllWarning               PASS      30.45 seconds
CheckSparse                   PASS      35.75 seconds
CheckSmatch                   PASS      99.39 seconds
BuildKernel32                 PASS      27.14 seconds
TestRunnerSetup               PASS      424.70 seconds
TestRunner_l2cap-tester       PASS      23.16 seconds
TestRunner_iso-tester         PASS      51.26 seconds
TestRunner_bnep-tester        PASS      6.98 seconds
TestRunner_mgmt-tester        FAIL      164.58 seconds
TestRunner_rfcomm-tester      PASS      10.89 seconds
TestRunner_sco-tester         PASS      14.40 seconds
TestRunner_ioctl-tester       PASS      11.99 seconds
TestRunner_mesh-tester        PASS      8.82 seconds
TestRunner_smp-tester         PASS      9.79 seconds
TestRunner_userchan-tester    PASS      7.40 seconds
IncrementalBuild              PASS      25.50 seconds

Details
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 497, Passed: 495 (99.6%), Failed: 1, Not Run: 1

Failed Test Cases
LL Privacy - Remove Device 4 (Disable Adv)           Timed out    2.189 seconds


---
Regards,
Linux Bluetooth
Sean Wang Dec. 19, 2023, 12:29 a.m. UTC | #2 
Hi Hao,

On Fri, Dec 15, 2023 at 12:40 AM Hao Qin <hao.qin@mediatek.com> wrote:
>
> From: "hao.qin" <hao.qin@mediatek.com>
>
> For MT7922, perform a reset before patch download to avoid
> unexpected problems caused by inconsistency between upper layer
> status and dongle status. Add USB reset retry to recover from
> unexpected patch download failure.

After looking through the patch, I guess the patch providing the reset
mechanism for
1) the core layer to call when cmd is timeout
2) resetting the device before the patch download
3) handling situations where something goes wrong during the patch
download phase
right ? if so, the description should be revised to make the patch
close to what it does

>
> Signed-off-by: hao.qin <hao.qin@mediatek.com>
> ---
>  drivers/bluetooth/btusb.c | 66 +++++++++++++++++++++++++++------------
>  1 file changed, 46 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 0926e4451802..778e1a0d9cae 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2812,7 +2812,7 @@ static int btusb_mtk_hci_wmt_sync(struct hci_dev *hdev,
>          * WMT command.
>          */
>         err = wait_on_bit_timeout(&data->flags, BTUSB_TX_WAIT_VND_EVT,
> -                                 TASK_INTERRUPTIBLE, HCI_INIT_TIMEOUT);
> +                                 TASK_INTERRUPTIBLE, HCI_CMD_TIMEOUT);

The changes are not specific to MT7922 and do not align with the
commit description. Kindly consider moving them to another patch and
provide clear reasons why they are necessary for MT7922 or any other
chipsets.

>         if (err == -EINTR) {
>                 bt_dev_err(hdev, "Execution of wmt command interrupted");
>                 clear_bit(BTUSB_TX_WAIT_VND_EVT, &data->flags);
> @@ -2994,28 +2994,22 @@ static u32 btusb_mtk_reset_done(struct hci_dev *hdev)
>         return val & MTK_BT_RST_DONE;
>  }
>
> -static int btusb_mtk_reset(struct hci_dev *hdev, void *rst_data)
> +static int btusb_mtk_subsys_reset(struct hci_dev *hdev, u32 dev_id)
>  {
>         struct btusb_data *data = hci_get_drvdata(hdev);
> -       struct btmediatek_data *mediatek;
>         u32 val;
>         int err;
>
> -       /* It's MediaTek specific bluetooth reset mechanism via USB */
> -       if (test_and_set_bit(BTUSB_HW_RESET_ACTIVE, &data->flags)) {
> -               bt_dev_err(hdev, "last reset failed? Not resetting again");
> -               return -EBUSY;
> -       }
> -
> -       err = usb_autopm_get_interface(data->intf);
> -       if (err < 0)
> -               return err;
> -
> -       btusb_stop_traffic(data);
> -       usb_kill_anchored_urbs(&data->tx_anchor);
> -       mediatek = hci_get_priv(hdev);
> -

It seems that you refactored the existing btusb_mtk_reset in the
patch. Could you please create another patch specifically for the
refactoring without introducing any functional changes of MT7922 prior
to adding the patch? This way would be good for ease of review and
tracking in the future. Additionally, it ensures that those working on
MT7921 are aware of the patch and its modifications.

> -       if (mediatek->dev_id == 0x7925) {
> +       if (dev_id == 0x7922) {
> +               btusb_mtk_uhw_reg_read(data, MTK_BT_SUBSYS_RST, &val);
> +               val |= 0x00002020;
> +               btusb_mtk_uhw_reg_write(data, MTK_BT_SUBSYS_RST, val);
> +               btusb_mtk_uhw_reg_write(data, MTK_EP_RST_OPT, 0x00010001);
> +               btusb_mtk_uhw_reg_read(data, MTK_BT_SUBSYS_RST, &val);
> +               val |= (1 << 0);

We can utilize BIT(0) instead of 1 << 0

> +               btusb_mtk_uhw_reg_write(data, MTK_BT_SUBSYS_RST, val);
> +               msleep(100);
> +       } else if (dev_id == 0x7925) {
>                 btusb_mtk_uhw_reg_read(data, MTK_BT_RESET_REG_CONNV3, &val);
>                 val |= (1 << 5);

We can utilize BIT(5) instead of 1 << 5

>                 btusb_mtk_uhw_reg_write(data, MTK_BT_RESET_REG_CONNV3, val);
> @@ -3053,14 +3047,41 @@ static int btusb_mtk_reset(struct hci_dev *hdev, void *rst_data)
>         err = readx_poll_timeout(btusb_mtk_reset_done, hdev, val,
>                                  val & MTK_BT_RST_DONE, 20000, 1000000);
>         if (err < 0)
> -               bt_dev_err(hdev, "Reset timeout");
> +               bt_dev_err(hdev, "Subsys reset timeout");

I guess it would be better to avoid the unnecessary change unrelated
to the patch.

> +
> +       if (dev_id == 0x7922)
> +               btusb_mtk_uhw_reg_write(data, MTK_UDMA_INT_STA_BT, 0x000000FF);
>
>         btusb_mtk_id_get(data, 0x70010200, &val);
>         if (!val)
>                 bt_dev_err(hdev, "Can't get device id, subsys reset fail.");
>
> -       usb_queue_reset_device(data->intf);
> +       return err;
> +}
> +
> +static int btusb_mtk_reset(struct hci_dev *hdev, void *rst_data)
> +{
> +       struct btusb_data *data = hci_get_drvdata(hdev);
> +       struct btmediatek_data *mediatek;
> +       int err;
> +
> +       /* It's MediaTek specific bluetooth reset mechanism via USB */
> +       if (test_and_set_bit(BTUSB_HW_RESET_ACTIVE, &data->flags)) {
> +               bt_dev_err(hdev, "last reset failed? Not resetting again");
> +               return -EBUSY;
> +       }
> +
> +       err = usb_autopm_get_interface(data->intf);
> +       if (err < 0)
> +               return err;
> +
> +       btusb_stop_traffic(data);
> +       usb_kill_anchored_urbs(&data->tx_anchor);
> +       mediatek = hci_get_priv(hdev);
>
> +       err = btusb_mtk_subsys_reset(hdev, mediatek->dev_id);
> +
> +       usb_queue_reset_device(data->intf);
>         clear_bit(BTUSB_HW_RESET_ACTIVE, &data->flags);
>
>         return err;
> @@ -3119,6 +3140,8 @@ static int btusb_mtk_setup(struct hci_dev *hdev)
>                 fwname = FIRMWARE_MT7668;
>                 break;
>         case 0x7922:
> +               btusb_mtk_subsys_reset(hdev, dev_id);
> +               fallthrough;
>         case 0x7961:
>         case 0x7925:
>                 if (dev_id == 0x7925)
> @@ -3134,6 +3157,9 @@ static int btusb_mtk_setup(struct hci_dev *hdev)
>                                                 btusb_mtk_hci_wmt_sync);
>                 if (err < 0) {
>                         bt_dev_err(hdev, "Failed to set up firmware (%d)", err);
> +                       btusb_stop_traffic(data);
> +                       usb_kill_anchored_urbs(&data->tx_anchor);
> +                       usb_queue_reset_device(data->intf);

The change doesn't appear to be specific to MT7922, as the commit
description suggests. Please consider splitting it into another patch
to address any existing chipset issues, if necessary.

>                         return err;
>                 }
>
> --
> 2.18.0
>
>
diff mbox series

Patch

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 0926e4451802..778e1a0d9cae 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -2812,7 +2812,7 @@  static int btusb_mtk_hci_wmt_sync(struct hci_dev *hdev,
 	 * WMT command.
 	 */
 	err = wait_on_bit_timeout(&data->flags, BTUSB_TX_WAIT_VND_EVT,
-				  TASK_INTERRUPTIBLE, HCI_INIT_TIMEOUT);
+				  TASK_INTERRUPTIBLE, HCI_CMD_TIMEOUT);
 	if (err == -EINTR) {
 		bt_dev_err(hdev, "Execution of wmt command interrupted");
 		clear_bit(BTUSB_TX_WAIT_VND_EVT, &data->flags);
@@ -2994,28 +2994,22 @@  static u32 btusb_mtk_reset_done(struct hci_dev *hdev)
 	return val & MTK_BT_RST_DONE;
 }
 
-static int btusb_mtk_reset(struct hci_dev *hdev, void *rst_data)
+static int btusb_mtk_subsys_reset(struct hci_dev *hdev, u32 dev_id)
 {
 	struct btusb_data *data = hci_get_drvdata(hdev);
-	struct btmediatek_data *mediatek;
 	u32 val;
 	int err;
 
-	/* It's MediaTek specific bluetooth reset mechanism via USB */
-	if (test_and_set_bit(BTUSB_HW_RESET_ACTIVE, &data->flags)) {
-		bt_dev_err(hdev, "last reset failed? Not resetting again");
-		return -EBUSY;
-	}
-
-	err = usb_autopm_get_interface(data->intf);
-	if (err < 0)
-		return err;
-
-	btusb_stop_traffic(data);
-	usb_kill_anchored_urbs(&data->tx_anchor);
-	mediatek = hci_get_priv(hdev);
-
-	if (mediatek->dev_id == 0x7925) {
+	if (dev_id == 0x7922) {
+		btusb_mtk_uhw_reg_read(data, MTK_BT_SUBSYS_RST, &val);
+		val |= 0x00002020;
+		btusb_mtk_uhw_reg_write(data, MTK_BT_SUBSYS_RST, val);
+		btusb_mtk_uhw_reg_write(data, MTK_EP_RST_OPT, 0x00010001);
+		btusb_mtk_uhw_reg_read(data, MTK_BT_SUBSYS_RST, &val);
+		val |= (1 << 0);
+		btusb_mtk_uhw_reg_write(data, MTK_BT_SUBSYS_RST, val);
+		msleep(100);
+	} else if (dev_id == 0x7925) {
 		btusb_mtk_uhw_reg_read(data, MTK_BT_RESET_REG_CONNV3, &val);
 		val |= (1 << 5);
 		btusb_mtk_uhw_reg_write(data, MTK_BT_RESET_REG_CONNV3, val);
@@ -3053,14 +3047,41 @@  static int btusb_mtk_reset(struct hci_dev *hdev, void *rst_data)
 	err = readx_poll_timeout(btusb_mtk_reset_done, hdev, val,
 				 val & MTK_BT_RST_DONE, 20000, 1000000);
 	if (err < 0)
-		bt_dev_err(hdev, "Reset timeout");
+		bt_dev_err(hdev, "Subsys reset timeout");
+
+	if (dev_id == 0x7922)
+		btusb_mtk_uhw_reg_write(data, MTK_UDMA_INT_STA_BT, 0x000000FF);
 
 	btusb_mtk_id_get(data, 0x70010200, &val);
 	if (!val)
 		bt_dev_err(hdev, "Can't get device id, subsys reset fail.");
 
-	usb_queue_reset_device(data->intf);
+	return err;
+}
+
+static int btusb_mtk_reset(struct hci_dev *hdev, void *rst_data)
+{
+	struct btusb_data *data = hci_get_drvdata(hdev);
+	struct btmediatek_data *mediatek;
+	int err;
+
+	/* It's MediaTek specific bluetooth reset mechanism via USB */
+	if (test_and_set_bit(BTUSB_HW_RESET_ACTIVE, &data->flags)) {
+		bt_dev_err(hdev, "last reset failed? Not resetting again");
+		return -EBUSY;
+	}
+
+	err = usb_autopm_get_interface(data->intf);
+	if (err < 0)
+		return err;
+
+	btusb_stop_traffic(data);
+	usb_kill_anchored_urbs(&data->tx_anchor);
+	mediatek = hci_get_priv(hdev);
 
+	err = btusb_mtk_subsys_reset(hdev, mediatek->dev_id);
+
+	usb_queue_reset_device(data->intf);
 	clear_bit(BTUSB_HW_RESET_ACTIVE, &data->flags);
 
 	return err;
@@ -3119,6 +3140,8 @@  static int btusb_mtk_setup(struct hci_dev *hdev)
 		fwname = FIRMWARE_MT7668;
 		break;
 	case 0x7922:
+		btusb_mtk_subsys_reset(hdev, dev_id);
+		fallthrough;
 	case 0x7961:
 	case 0x7925:
 		if (dev_id == 0x7925)
@@ -3134,6 +3157,9 @@  static int btusb_mtk_setup(struct hci_dev *hdev)
 						btusb_mtk_hci_wmt_sync);
 		if (err < 0) {
 			bt_dev_err(hdev, "Failed to set up firmware (%d)", err);
+			btusb_stop_traffic(data);
+			usb_kill_anchored_urbs(&data->tx_anchor);
+			usb_queue_reset_device(data->intf);
 			return err;
 		}