Skip to contentSkip to conversation

Bluetooth: btintel: Fix WBS setting for Intel legacy ROM products

RFC PATCH In review Tedd Ho-Jeong An 3 participants 3 messages Jan 19, 2022 – Jan 20, 2022 Bluetooth drivers

lore
Share & tools
lore

Tedd Ho-Jeong An

author
#0 lore raw

RFC PATCH 1/1 Bluetooth: btintel: Fix WBS setting for Intel legacy ROM products

From: Tedd Ho-Jeong An <[email protected]>

Intel Legacy ROM Products don't support WBS except the SdP(8087:0aa7). But StP2(8087:0a2a) and SdP have the same version information, and btintel cannot distinguish between StP2 and SdP for WBS support.

This patch sets the WBS support flag for SdP based on USB idProduct and uses it in btintel to configure the WBS.

This flag is only applicable for Legacy ROM products.

Fixes 3df4dfbec0f29 ("Bluetooth: btintel: Move hci quirks to setup routine")
Signed-off-by Tedd Ho-Jeong An <[email protected]>
---
 drivers/bluetooth/btintel.c | 10 +++++++---
 drivers/bluetooth/btintel.h |  1 +
 drivers/bluetooth/btusb.c   | 12 ++++++++++++
 3 files changed, 20 insertions(+), 3 deletions(-)
drivers/bluetooth/btintel.c +7 -3
index 1a4f8b227eac..6730c9b2ae33 100644
@@ -2428,10 +2428,14 @@ static int btintel_setup_combined(struct hci_dev *hdev)
 
 			/* Apply the device specific HCI quirks
 			 *
-			 * WBS for SdP - SdP and Stp have a same hw_varaint but
-			 * different fw_variant
+			 * WBS for SdP - The version information is the same for
+			 * both StP2 and SdP, so it cannot be used to
+			 * distinguish between StP2 and SdP. Instead, it uses
+			 * the flag set by the transport driver(btusb) for
+			 * the Legacy ROM SKU and sets the quirk for WBS.
 			 */
-			if (ver.hw_variant == 0x08 && ver.fw_variant == 0x22)
+			if (btintel_test_flag(hdev,
+					      INTEL_ROM_LEGACY_WBS_SUPPORTED))
 				set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED,
 					&hdev->quirks);
drivers/bluetooth/btintel.h +1 -0
index c9b24e9299e2..efdb3d738abf 100644
@@ -152,6 +152,7 @@ enum {
 	INTEL_BROKEN_INITIAL_NCMD,
 	INTEL_BROKEN_SHUTDOWN_LED,
 	INTEL_ROM_LEGACY,
+	INTEL_ROM_LEGACY_WBS_SUPPORTED,
 
 	__INTEL_NUM_FLAGS,
 };
drivers/bluetooth/btusb.c +12 -0
index c30d131da784..286e2fa1ef44 100644
@@ -3742,6 +3742,18 @@ static int btusb_probe(struct usb_interface *intf,
 
 		if (id->driver_info & BTUSB_INTEL_BROKEN_SHUTDOWN_LED)
 			btintel_set_flag(hdev, INTEL_BROKEN_SHUTDOWN_LED);
+
+		/* Intel's Legacy ROM products don't support WBS except
+		 * the SdP(8087:0aa7). But the StP2(8087:0a2a) and SdP have the
+		 * same version information, and btintel can't distinguish
+		 * between StP2 and SdP for the WBS support.
+		 * It sets the flag here based on the USB PID to enable the WBS
+		 * support for legacy ROM products.
+		 * Note that this flag is only applicable to legacy ROM
+		 * products.
+		 */
+		if (id->idProduct == 0x0aa7)
+			btintel_set_flag(hdev, INTEL_ROM_LEGACY_WBS_SUPPORTED);
 	}
 
 	if (id->driver_info & BTUSB_MARVELL)
Signature · 1 line
2.25.1
maintainer
#1 lore raw

Hi Tedd,

Quoted 64 lines

Intel Legacy ROM Products don't support WBS except the SdP(8087:0aa7). But StP2(8087:0a2a) and SdP have the same version information, and btintel cannot distinguish between StP2 and SdP for WBS support.

This patch sets the WBS support flag for SdP based on USB idProduct and uses it in btintel to configure the WBS.

This flag is only applicable for Legacy ROM products.

Fixes 3df4dfbec0f29 ("Bluetooth: btintel: Move hci quirks to setup routine")
Signed-off-by Tedd Ho-Jeong An <[email protected]>

---

drivers/bluetooth/btintel.c | 10 +++++++---
drivers/bluetooth/btintel.h |  1 +
drivers/bluetooth/btusb.c   | 12 ++++++++++++
3 files changed, 20 insertions(+), 3 deletions(-)
drivers/bluetooth/btintel.c +7 -3
index 1a4f8b227eac..6730c9b2ae33 100644
@@ -2428,10 +2428,14 @@ static int btintel_setup_combined(struct hci_dev *hdev)
 
 			/* Apply the device specific HCI quirks
 			 *
-			 * WBS for SdP - SdP and Stp have a same hw_varaint but
-			 * different fw_variant
+			 * WBS for SdP - The version information is the same for
+			 * both StP2 and SdP, so it cannot be used to
+			 * distinguish between StP2 and SdP. Instead, it uses
+			 * the flag set by the transport driver(btusb) for
+			 * the Legacy ROM SKU and sets the quirk for WBS.
 			 */
-			if (ver.hw_variant == 0x08 && ver.fw_variant == 0x22)
+			if (btintel_test_flag(hdev,
+					      INTEL_ROM_LEGACY_WBS_SUPPORTED))
 				set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED,
 					&hdev->quirks);
drivers/bluetooth/btintel.h +1 -0
index c9b24e9299e2..efdb3d738abf 100644
@@ -152,6 +152,7 @@ enum {
 	INTEL_BROKEN_INITIAL_NCMD,
 	INTEL_BROKEN_SHUTDOWN_LED,
 	INTEL_ROM_LEGACY,
+	INTEL_ROM_LEGACY_WBS_SUPPORTED,
 
 	__INTEL_NUM_FLAGS,
 };
drivers/bluetooth/btusb.c +5 -0
index c30d131da784..286e2fa1ef44 100644
@@ -3742,6 +3742,18 @@ static int btusb_probe(struct usb_interface *intf,
 
 		if (id->driver_info & BTUSB_INTEL_BROKEN_SHUTDOWN_LED)
 			btintel_set_flag(hdev, INTEL_BROKEN_SHUTDOWN_LED);
+
+		/* Intel's Legacy ROM products don't support WBS except
+		 * the SdP(8087:0aa7). But the StP2(8087:0a2a) and SdP have the
+		 * same version information, and btintel can't distinguish
+		 * between StP2 and SdP for the WBS support.

drivers/bluetooth/btusb.c static int btusb_probe(struct usb_interface *intf, · line 3759

Quoted 10 lines
drivers/bluetooth/btusb.c +7 -0
+		 * It sets the flag here based on the USB PID to enable the WBS
+		 * support for legacy ROM products.
+		 * Note that this flag is only applicable to legacy ROM
+		 * products.
+		 */
+		if (id->idProduct == 0x0aa7)
+			btintel_set_flag(hdev, INTEL_ROM_LEGACY_WBS_SUPPORTED);
 	}
 
 	if (id->driver_info & BTUSB_MARVELL)

while this is a total mess from a hardware point of view, I prefer our quirking is kinda the same.

One way would be to give the idProduct to btintel.c and remove all quirks from btusb.c. Something like this:

drivers/bluetooth/btintel.h +7 -0
index c9b24e9299e2..4adb21cf5c20 100644
@@ -158,6 +158,7 @@ enum {
 
 struct btintel_data {
        DECLARE_BITMAP(flags, __INTEL_NUM_FLAGS);
+       u8 usb_pid;
 };
 
 #define btintel_set_flag(hdev, nr)                                     \

@@ -186,6 +187,12 @@ struct btintel_data {
 #define btintel_wait_on_flag_timeout(hdev, nr, m, to)                  \
                wait_on_bit_timeout(btintel_get_flag(hdev), (nr), m, to)
 
+#define btintel_set_usb_pid(hdev, pid)                                 \
+       do {                                                            \
+               struct btintel_data *intel = hci_get_priv((hdev));      \
+               intel->usb_pid = (pid);                                 \
+       } while (0)
+
 #if IS_ENABLED(CONFIG_BT_INTEL)
 
 int btintel_check_bdaddr(struct hci_dev *hdev);
drivers/bluetooth/btusb.c +2 -0
index c30d131da784..9b5348052421 100644
@@ -3737,6 +3737,8 @@ static int btusb_probe(struct usb_interface *intf,
                hdev->send = btusb_send_frame_intel;
                hdev->cmd_timeout = btusb_intel_cmd_timeout;
 
+               btintel_set_usb_pid(hdev, id->idProduct);
+
                if (id->driver_info & BTUSB_INTEL_BROKEN_INITIAL_NCMD)
                        btintel_set_flag(hdev, INTEL_BROKEN_INITIAL_NCMD);

Then we need to add a duplicated USB PID table to btintel.c, but everything would be there and all Intel quirks for faulty ROM loader or version information could go to btintel.c.

Or you create a BTUSB_INTEL_NO_WBS_SUPPORT and add it to 0xa2a and 0x07dc like this:

drivers/bluetooth/btusb.c +6 -0
index c30d131da784..a898df89585a 100644
@@ -62,6 +62,7 @@ static struct usb_driver btusb_driver;
 #define BTUSB_QCA_WCN6855      0x1000000
 #define BTUSB_INTEL_BROKEN_SHUTDOWN_LED        0x2000000
 #define BTUSB_INTEL_BROKEN_INITIAL_NCMD 0x4000000
+#define BTUSB_INTEL_NO_WBS_SUPPORT     0x8000000
 
 static const struct usb_device_id btusb_table[] = {
        /* Generic Bluetooth USB device */

@@ -385,9 +386,11 @@ static const struct usb_device_id blacklist_table[] = {
        { USB_DEVICE(0x8087, 0x0033), .driver_info = BTUSB_INTEL_COMBINED },
        { USB_DEVICE(0x8087, 0x07da), .driver_info = BTUSB_CSR },
        { USB_DEVICE(0x8087, 0x07dc), .driver_info = BTUSB_INTEL_COMBINED |
+                                                    BTUSB_INTEL_NO_WBS_SUPPORT |
                                                     BTUSB_INTEL_BROKEN_INITIAL_NCMD |
                                                     BTUSB_INTEL_BROKEN_SHUTDOWN_LED },
        { USB_DEVICE(0x8087, 0x0a2a), .driver_info = BTUSB_INTEL_COMBINED |
+                                                    BTUSB_INTEL_NO_WBS_SUPPORT |
                                                     BTUSB_INTEL_BROKEN_SHUTDOWN_LED },
        { USB_DEVICE(0x8087, 0x0a2b), .driver_info = BTUSB_INTEL_COMBINED },
        { USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL_COMBINED |

@@ -3737,6 +3740,9 @@ static int btusb_probe(struct usb_interface *intf,
                hdev->send = btusb_send_frame_intel;
                hdev->cmd_timeout = btusb_intel_cmd_timeout;
 
+               if (id->driver_info & BTUSB_INTEL_NO_WBS_SUPPORT)
+                       btintel_set_flag(hdev, INTEL_ROM_LEGACY_NO_WBS);
+
                if (id->driver_info & BTUSB_INTEL_BROKEN_INITIAL_NCMD)
                        btintel_set_flag(hdev, INTEL_BROKEN_INITIAL_NCMD);

Frankly, I have no idea what I would favor right now. I do swing a little bit towards the extra btusb.c quirk to keep the btintel.c transport agnostic.

Regards

Marcel

#2 lore raw

Hi Marcel,

Quoted 150 lines · Marcel Holtmann

Hi Tedd,

Quoted 64 lines

Intel Legacy ROM Products don't support WBS except the SdP(8087:0aa7). But StP2(8087:0a2a) and SdP have the same version information, and btintel cannot distinguish between StP2 and SdP for WBS support.

This patch sets the WBS support flag for SdP based on USB idProduct and uses it in btintel to configure the WBS.

This flag is only applicable for Legacy ROM products.

Fixes 3df4dfbec0f29 ("Bluetooth: btintel: Move hci quirks to setup routine")
Signed-off-by Tedd Ho-Jeong An <[email protected]>

---
drivers/bluetooth/btintel.c | 10 +++++++---
drivers/bluetooth/btintel.h |  1 +
drivers/bluetooth/btusb.c   | 12 ++++++++++++

3 files changed, 20 insertions(+), 3 deletions(-)
drivers/bluetooth/btintel.c +7 -3
index 1a4f8b227eac..6730c9b2ae33 100644
@@ -2428,10 +2428,14 @@ static int btintel_setup_combined(struct hci_dev *hdev)
 
                         /* Apply the device specific HCI quirks
                          *
-                        * WBS for SdP - SdP and Stp have a same hw_varaint but
-                        * different fw_variant
+                        * WBS for SdP - The version information is the same for
+                        * both StP2 and SdP, so it cannot be used to
+                        * distinguish between StP2 and SdP. Instead, it uses
+                        * the flag set by the transport driver(btusb) for
+                        * the Legacy ROM SKU and sets the quirk for WBS.
                          */
-                       if (ver.hw_variant == 0x08 && ver.fw_variant == 0x22)
+                       if (btintel_test_flag(hdev,
+                                             INTEL_ROM_LEGACY_WBS_SUPPORTED))
                                 set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED,
                                         &hdev->quirks);
drivers/bluetooth/btintel.h +1 -0
index c9b24e9299e2..efdb3d738abf 100644
@@ -152,6 +152,7 @@ enum {
         INTEL_BROKEN_INITIAL_NCMD,
         INTEL_BROKEN_SHUTDOWN_LED,
         INTEL_ROM_LEGACY,
+       INTEL_ROM_LEGACY_WBS_SUPPORTED,
 
         __INTEL_NUM_FLAGS,
 };
drivers/bluetooth/btusb.c +5 -0
index c30d131da784..286e2fa1ef44 100644
@@ -3742,6 +3742,18 @@ static int btusb_probe(struct usb_interface *intf,
 
                 if (id->driver_info & BTUSB_INTEL_BROKEN_SHUTDOWN_LED)
                         btintel_set_flag(hdev, INTEL_BROKEN_SHUTDOWN_LED);
+
+               /* Intel's Legacy ROM products don't support WBS except
+                * the SdP(8087:0aa7). But the StP2(8087:0a2a) and SdP have the
+                * same version information, and btintel can't distinguish
+                * between StP2 and SdP for the WBS support.

drivers/bluetooth/btusb.c static int btusb_probe(struct usb_interface *intf, · line 3759

Quoted 10 lines
drivers/bluetooth/btusb.c +7 -0
+                * It sets the flag here based on the USB PID to enable the WBS
+                * support for legacy ROM products.
+                * Note that this flag is only applicable to legacy ROM
+                * products.
+                */
+               if (id->idProduct == 0x0aa7)
+                       btintel_set_flag(hdev, INTEL_ROM_LEGACY_WBS_SUPPORTED);
         }
 
         if (id->driver_info & BTUSB_MARVELL)

while this is a total mess from a hardware point of view, I prefer our quirking is kinda the same.

One way would be to give the idProduct to btintel.c and remove all quirks from btusb.c. Something like this:

drivers/bluetooth/btintel.h +0 -0
index c9b24e9299e2..4adb21cf5c20 100644
@@ -158,6 +158,7 @@ enum {


 struct btintel_data {
        DECLARE_BITMAP(flags, __INTEL_NUM_FLAGS);
+       u8 usb_pid;
 };

 #define btintel_set_flag(hdev, nr)                                     \

drivers/bluetooth/btintel.h +0 -0
@@ -186,6 +187,12 @@ struct btintel_data {
  #define btintel_wait_on_flag_timeout(hdev, nr, m, to)                  \
                 wait_on_bit_timeout(btintel_get_flag(hdev), (nr), m, to)

drivers/bluetooth/btintel.h +6 -0
+#define btintel_set_usb_pid(hdev, pid)                                 \
+       do {                                                            \
+               struct btintel_data *intel = hci_get_priv((hdev));      \
+               intel->usb_pid = (pid);                                 \
+       } while (0)
+

 #if IS_ENABLED(CONFIG_BT_INTEL)

 int btintel_check_bdaddr(struct hci_dev *hdev);

drivers/bluetooth/btusb.c +0 -0
index c30d131da784..9b5348052421 100644
@@ -3737,6 +3737,8 @@ static int btusb_probe(struct usb_interface *intf,
                 hdev->send = btusb_send_frame_intel;
                 hdev->cmd_timeout = btusb_intel_cmd_timeout;


+               btintel_set_usb_pid(hdev, id->idProduct);
+
                if (id->driver_info & BTUSB_INTEL_BROKEN_INITIAL_NCMD) btintel_set_flag(hdev, INTEL_BROKEN_INITIAL_NCMD);

Then we need to add a duplicated USB PID table to btintel.c, but everything would be there and all Intel quirks for faulty ROM loader or version information could go to btintel.c.

Or you create a BTUSB_INTEL_NO_WBS_SUPPORT and add it to 0xa2a and 0x07dc like this:

drivers/bluetooth/btusb.c +1 -0
index c30d131da784..a898df89585a 100644
@@ -62,6 +62,7 @@ static struct usb_driver btusb_driver;
  #define BTUSB_QCA_WCN6855      0x1000000
  #define BTUSB_INTEL_BROKEN_SHUTDOWN_LED        0x2000000
  #define BTUSB_INTEL_BROKEN_INITIAL_NCMD 0x4000000
+#define BTUSB_INTEL_NO_WBS_SUPPORT     0x8000000


 static const struct usb_device_id btusb_table[] = { /* Generic Bluetooth USB device */

drivers/bluetooth/btusb.c +0 -0
@@ -385,9 +386,11 @@ static const struct usb_device_id blacklist_table[] = {
         { USB_DEVICE(0x8087, 0x0033), .driver_info = BTUSB_INTEL_COMBINED },
         { USB_DEVICE(0x8087, 0x07da), .driver_info = BTUSB_CSR },

        { USB_DEVICE(0x8087, 0x07dc), .driver_info = BTUSB_INTEL_COMBINED | +                                                    BTUSB_INTEL_NO_WBS_SUPPORT | BTUSB_INTEL_BROKEN_INITIAL_NCMD | BTUSB_INTEL_BROKEN_SHUTDOWN_LED }, { USB_DEVICE(0x8087, 0x0a2a), .driver_info = BTUSB_INTEL_COMBINED | +                                                    BTUSB_INTEL_NO_WBS_SUPPORT | BTUSB_INTEL_BROKEN_SHUTDOWN_LED }, { USB_DEVICE(0x8087, 0x0a2b), .driver_info = BTUSB_INTEL_COMBINED }, { USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL_COMBINED |

drivers/bluetooth/btusb.c +0 -0
@@ -3737,6 +3740,9 @@ static int btusb_probe(struct usb_interface *intf,
                 hdev->send = btusb_send_frame_intel;
                 hdev->cmd_timeout = btusb_intel_cmd_timeout;
Quoted 9 lines


+               if (id->driver_info & BTUSB_INTEL_NO_WBS_SUPPORT)
+                       btintel_set_flag(hdev, INTEL_ROM_LEGACY_NO_WBS);
+
                if (id->driver_info & BTUSB_INTEL_BROKEN_INITIAL_NCMD)
                        btintel_set_flag(hdev, INTEL_BROKEN_INITIAL_NCMD);

Frankly, I have no idea what I would favor right now. I do swing a little bit towards the extra btusb.c quirk to keep the btintel.c transport agnostic.

I will go with the second option and will send out the patch.

Quoted 4 lines

Regards

Marcel