diff mbox series

[RFC] Bluetooth: btintel: Fix WBS setting for Intel legacy ROM products

Message ID 20220119045119.132191-1-hj.tedd.an@gmail.com
State New
Headers show
Series [RFC] Bluetooth: btintel: Fix WBS setting for Intel legacy ROM products | expand

Commit Message

Tedd Ho-Jeong An Jan. 19, 2022, 4:51 a.m. UTC
From: Tedd Ho-Jeong An <tedd.an@intel.com>

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 <tedd.an@intel.com>
---
 drivers/bluetooth/btintel.c | 10 +++++++---
 drivers/bluetooth/btintel.h |  1 +
 drivers/bluetooth/btusb.c   | 12 ++++++++++++
 3 files changed, 20 insertions(+), 3 deletions(-)

Comments

Marcel Holtmann Jan. 19, 2022, 5:03 p.m. UTC | #1
Hi Tedd,

> 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 <tedd.an@intel.com>
> ---
> drivers/bluetooth/btintel.c | 10 +++++++---
> drivers/bluetooth/btintel.h |  1 +
> drivers/bluetooth/btusb.c   | 12 ++++++++++++
> 3 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index 1a4f8b227eac..6730c9b2ae33 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -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);
> 
> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> index c9b24e9299e2..efdb3d738abf 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -152,6 +152,7 @@ enum {
> 	INTEL_BROKEN_INITIAL_NCMD,
> 	INTEL_BROKEN_SHUTDOWN_LED,
> 	INTEL_ROM_LEGACY,
> +	INTEL_ROM_LEGACY_WBS_SUPPORTED,
> 
> 	__INTEL_NUM_FLAGS,
> };
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index c30d131da784..286e2fa1ef44 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -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)

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:

diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index c9b24e9299e2..4adb21cf5c20 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -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);
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index c30d131da784..9b5348052421 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -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:

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index c30d131da784..a898df89585a 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -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
An, Tedd Jan. 20, 2022, 7:37 a.m. UTC | #2
Hi Marcel,

On Wed, 2022-01-19 at 18:03 +0100, Marcel Holtmann wrote:
> Hi Tedd,
> 
> > 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 <tedd.an@intel.com>
> > ---
> > drivers/bluetooth/btintel.c | 10 +++++++---
> > drivers/bluetooth/btintel.h |  1 +
> > drivers/bluetooth/btusb.c   | 12 ++++++++++++
> > 3 files changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index 1a4f8b227eac..6730c9b2ae33 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -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);
> > 
> > diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> > index c9b24e9299e2..efdb3d738abf 100644
> > --- a/drivers/bluetooth/btintel.h
> > +++ b/drivers/bluetooth/btintel.h
> > @@ -152,6 +152,7 @@ enum {
> >         INTEL_BROKEN_INITIAL_NCMD,
> >         INTEL_BROKEN_SHUTDOWN_LED,
> >         INTEL_ROM_LEGACY,
> > +       INTEL_ROM_LEGACY_WBS_SUPPORTED,
> > 
> >         __INTEL_NUM_FLAGS,
> > };
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index c30d131da784..286e2fa1ef44 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -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)
> 
> 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:
> 
> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> index c9b24e9299e2..4adb21cf5c20 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -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);
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index c30d131da784..9b5348052421 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -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:
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index c30d131da784..a898df89585a 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -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.
> 

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


> Regards
> 
> Marcel
>
diff mbox series

Patch

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 1a4f8b227eac..6730c9b2ae33 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -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);
 
diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
index c9b24e9299e2..efdb3d738abf 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -152,6 +152,7 @@  enum {
 	INTEL_BROKEN_INITIAL_NCMD,
 	INTEL_BROKEN_SHUTDOWN_LED,
 	INTEL_ROM_LEGACY,
+	INTEL_ROM_LEGACY_WBS_SUPPORTED,
 
 	__INTEL_NUM_FLAGS,
 };
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index c30d131da784..286e2fa1ef44 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -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)