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

public inbox for linux-bluetooth@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH RFC] Bluetooth: btintel: Fix WBS setting for Intel legacy ROM products
@ 2022-01-19  4:51 Tedd Ho-Jeong An
  2022-01-19 17:03 ` Marcel Holtmann
  0 siblings, 1 reply; 3+ messages in thread
From: Tedd Ho-Jeong An @ 2022-01-19  4:51 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Tedd Ho-Jeong An

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(-)

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)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH RFC] Bluetooth: btintel: Fix WBS setting for Intel legacy ROM products
  2022-01-19  4:51 [PATCH RFC] Bluetooth: btintel: Fix WBS setting for Intel legacy ROM products Tedd Ho-Jeong An
@ 2022-01-19 17:03 ` Marcel Holtmann
  2022-01-20  7:37   ` An, Tedd
  0 siblings, 1 reply; 3+ messages in thread
From: Marcel Holtmann @ 2022-01-19 17:03 UTC (permalink / raw)
  To: Tedd Ho-Jeong An; +Cc: linux-bluetooth, Tedd Ho-Jeong An

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


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH RFC] Bluetooth: btintel: Fix WBS setting for Intel legacy ROM products
  2022-01-19 17:03 ` Marcel Holtmann
@ 2022-01-20  7:37   ` An, Tedd
  0 siblings, 0 replies; 3+ messages in thread
From: An, Tedd @ 2022-01-20  7:37 UTC (permalink / raw)
  To: hj.tedd.an@gmail.com, marcel@holtmann.org; +Cc: linux-bluetooth@vger.kernel.org

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
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2022-01-20  7:38 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-01-19  4:51 [PATCH RFC] Bluetooth: btintel: Fix WBS setting for Intel legacy ROM products Tedd Ho-Jeong An
2022-01-19 17:03 ` Marcel Holtmann
2022-01-20  7:37   ` An, Tedd

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox