* [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities @ 2021-05-13 8:54 Archie Pusaka 2021-05-13 10:12 ` bluez.test.bot 2021-05-13 15:14 ` [PATCH] " Marcel Holtmann 0 siblings, 2 replies; 14+ messages in thread From: Archie Pusaka @ 2021-05-13 8:54 UTC (permalink / raw) To: linux-bluetooth, Marcel Holtmann Cc: CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi, Johan Hedberg, Luiz Augusto von Dentz, linux-kernel From: Archie Pusaka <apusaka@chromium.org> RTL8822 chipset supports WBS, and this information is conveyed in btusb.c. However, the UART driver doesn't have this information just yet. Signed-off-by: Archie Pusaka <apusaka@chromium.org> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> --- drivers/bluetooth/btrtl.c | 26 ++++++++++++++++---------- drivers/bluetooth/btrtl.h | 2 ++ drivers/bluetooth/hci_h5.c | 5 +---- 3 files changed, 19 insertions(+), 14 deletions(-) diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c index e7fe5fb22753..988a09860c6b 100644 --- a/drivers/bluetooth/btrtl.c +++ b/drivers/bluetooth/btrtl.c @@ -719,17 +719,8 @@ int btrtl_download_firmware(struct hci_dev *hdev, } EXPORT_SYMBOL_GPL(btrtl_download_firmware); -int btrtl_setup_realtek(struct hci_dev *hdev) +void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev) { - struct btrtl_device_info *btrtl_dev; - int ret; - - btrtl_dev = btrtl_initialize(hdev, NULL); - if (IS_ERR(btrtl_dev)) - return PTR_ERR(btrtl_dev); - - ret = btrtl_download_firmware(hdev, btrtl_dev); - /* Enable controller to do both LE scan and BR/EDR inquiry * simultaneously. */ @@ -750,6 +741,21 @@ int btrtl_setup_realtek(struct hci_dev *hdev) rtl_dev_dbg(hdev, "WBS supported not enabled."); break; } +} +EXPORT_SYMBOL_GPL(btrtl_set_quirks); + +int btrtl_setup_realtek(struct hci_dev *hdev) +{ + struct btrtl_device_info *btrtl_dev; + int ret; + + btrtl_dev = btrtl_initialize(hdev, NULL); + if (IS_ERR(btrtl_dev)) + return PTR_ERR(btrtl_dev); + + ret = btrtl_download_firmware(hdev, btrtl_dev); + + btrtl_set_quirks(hdev, btrtl_dev); btrtl_free(btrtl_dev); return ret; diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h index 2a582682136d..260167f01b08 100644 --- a/drivers/bluetooth/btrtl.h +++ b/drivers/bluetooth/btrtl.h @@ -54,6 +54,8 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev, void btrtl_free(struct btrtl_device_info *btrtl_dev); int btrtl_download_firmware(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev); +void btrtl_set_quirks(struct hci_dev *hdev, + struct btrtl_device_info *btrtl_dev); int btrtl_setup_realtek(struct hci_dev *hdev); int btrtl_shutdown_realtek(struct hci_dev *hdev); int btrtl_get_uart_settings(struct hci_dev *hdev, diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c index 27e96681d583..e0520639f4ba 100644 --- a/drivers/bluetooth/hci_h5.c +++ b/drivers/bluetooth/hci_h5.c @@ -906,10 +906,7 @@ static int h5_btrtl_setup(struct h5 *h5) /* Give the device some time before the hci-core sends it a reset */ usleep_range(10000, 20000); - /* Enable controller to do both LE scan and BR/EDR inquiry - * simultaneously. - */ - set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks); + btrtl_set_quirks(h5->hu->hdev, btrtl_dev); out_free: btrtl_free(btrtl_dev); -- 2.31.1.751.gd2f1c929bd-goog ^ permalink raw reply related [flat|nested] 14+ messages in thread
* RE: Bluetooth: hci_h5: Add RTL8822CS capabilities 2021-05-13 8:54 [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities Archie Pusaka @ 2021-05-13 10:12 ` bluez.test.bot 2021-05-13 15:14 ` [PATCH] " Marcel Holtmann 1 sibling, 0 replies; 14+ messages in thread From: bluez.test.bot @ 2021-05-13 10:12 UTC (permalink / raw) To: linux-bluetooth, apusaka [-- Attachment #1: Type: text/plain, Size: 2630 bytes --] 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=481725 ---Test result--- Test Summary: CheckPatch PASS 0.77 seconds GitLint PASS 0.10 seconds BuildKernel PASS 514.32 seconds TestRunner: Setup PASS 337.54 seconds TestRunner: l2cap-tester PASS 2.57 seconds TestRunner: bnep-tester PASS 1.87 seconds TestRunner: mgmt-tester PASS 26.81 seconds TestRunner: rfcomm-tester PASS 2.04 seconds TestRunner: sco-tester PASS 1.96 seconds TestRunner: smp-tester PASS 2.06 seconds TestRunner: userchan-tester PASS 1.89 seconds Details ############################## Test: CheckPatch - PASS - 0.77 seconds Run checkpatch.pl script with rule in .checkpatch.conf ############################## Test: GitLint - PASS - 0.10 seconds Run gitlint with rule in .gitlint ############################## Test: BuildKernel - PASS - 514.32 seconds Build Kernel with minimal configuration supports Bluetooth ############################## Test: TestRunner: Setup - PASS - 337.54 seconds Setup environment for running Test Runner ############################## Test: TestRunner: l2cap-tester - PASS - 2.57 seconds Run test-runner with l2cap-tester Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: bnep-tester - PASS - 1.87 seconds Run test-runner with bnep-tester Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: mgmt-tester - PASS - 26.81 seconds Run test-runner with mgmt-tester Total: 416, Passed: 403 (96.9%), Failed: 0, Not Run: 13 ############################## Test: TestRunner: rfcomm-tester - PASS - 2.04 seconds Run test-runner with rfcomm-tester Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: sco-tester - PASS - 1.96 seconds Run test-runner with sco-tester Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: smp-tester - PASS - 2.06 seconds Run test-runner with smp-tester Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: userchan-tester - PASS - 1.89 seconds Run test-runner with userchan-tester Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0 --- Regards, Linux Bluetooth [-- Attachment #2: l2cap-tester.log --] [-- Type: application/octet-stream, Size: 44230 bytes --] [-- Attachment #3: bnep-tester.log --] [-- Type: application/octet-stream, Size: 3557 bytes --] [-- Attachment #4: mgmt-tester.log --] [-- Type: application/octet-stream, Size: 546745 bytes --] [-- Attachment #5: rfcomm-tester.log --] [-- Type: application/octet-stream, Size: 11677 bytes --] [-- Attachment #6: sco-tester.log --] [-- Type: application/octet-stream, Size: 9912 bytes --] [-- Attachment #7: smp-tester.log --] [-- Type: application/octet-stream, Size: 11823 bytes --] [-- Attachment #8: userchan-tester.log --] [-- Type: application/octet-stream, Size: 5454 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities 2021-05-13 8:54 [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities Archie Pusaka 2021-05-13 10:12 ` bluez.test.bot @ 2021-05-13 15:14 ` Marcel Holtmann 2021-05-13 16:32 ` Archie Pusaka 1 sibling, 1 reply; 14+ messages in thread From: Marcel Holtmann @ 2021-05-13 15:14 UTC (permalink / raw) To: Archie Pusaka Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi, Johan Hedberg, Luiz Augusto von Dentz, linux-kernel Hi Archie, > RTL8822 chipset supports WBS, and this information is conveyed in > btusb.c. However, the UART driver doesn't have this information just > yet. > > Signed-off-by: Archie Pusaka <apusaka@chromium.org> > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > --- > > drivers/bluetooth/btrtl.c | 26 ++++++++++++++++---------- > drivers/bluetooth/btrtl.h | 2 ++ > drivers/bluetooth/hci_h5.c | 5 +---- > 3 files changed, 19 insertions(+), 14 deletions(-) > > diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c > index e7fe5fb22753..988a09860c6b 100644 > --- a/drivers/bluetooth/btrtl.c > +++ b/drivers/bluetooth/btrtl.c > @@ -719,17 +719,8 @@ int btrtl_download_firmware(struct hci_dev *hdev, > } > EXPORT_SYMBOL_GPL(btrtl_download_firmware); > > -int btrtl_setup_realtek(struct hci_dev *hdev) > +void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev) > { > - struct btrtl_device_info *btrtl_dev; > - int ret; > - > - btrtl_dev = btrtl_initialize(hdev, NULL); > - if (IS_ERR(btrtl_dev)) > - return PTR_ERR(btrtl_dev); > - > - ret = btrtl_download_firmware(hdev, btrtl_dev); > - > /* Enable controller to do both LE scan and BR/EDR inquiry > * simultaneously. > */ > @@ -750,6 +741,21 @@ int btrtl_setup_realtek(struct hci_dev *hdev) > rtl_dev_dbg(hdev, "WBS supported not enabled."); > break; > } > +} > +EXPORT_SYMBOL_GPL(btrtl_set_quirks); > + > +int btrtl_setup_realtek(struct hci_dev *hdev) > +{ > + struct btrtl_device_info *btrtl_dev; > + int ret; > + > + btrtl_dev = btrtl_initialize(hdev, NULL); > + if (IS_ERR(btrtl_dev)) > + return PTR_ERR(btrtl_dev); > + > + ret = btrtl_download_firmware(hdev, btrtl_dev); > + > + btrtl_set_quirks(hdev, btrtl_dev); > > btrtl_free(btrtl_dev); > return ret; > diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h > index 2a582682136d..260167f01b08 100644 > --- a/drivers/bluetooth/btrtl.h > +++ b/drivers/bluetooth/btrtl.h > @@ -54,6 +54,8 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev, > void btrtl_free(struct btrtl_device_info *btrtl_dev); > int btrtl_download_firmware(struct hci_dev *hdev, > struct btrtl_device_info *btrtl_dev); > +void btrtl_set_quirks(struct hci_dev *hdev, > + struct btrtl_device_info *btrtl_dev); > int btrtl_setup_realtek(struct hci_dev *hdev); > int btrtl_shutdown_realtek(struct hci_dev *hdev); > int btrtl_get_uart_settings(struct hci_dev *hdev, > diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c > index 27e96681d583..e0520639f4ba 100644 > --- a/drivers/bluetooth/hci_h5.c > +++ b/drivers/bluetooth/hci_h5.c > @@ -906,10 +906,7 @@ static int h5_btrtl_setup(struct h5 *h5) > /* Give the device some time before the hci-core sends it a reset */ > usleep_range(10000, 20000); > > - /* Enable controller to do both LE scan and BR/EDR inquiry > - * simultaneously. > - */ > - set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks); > + btrtl_set_quirks(h5->hu->hdev, btrtl_dev); any reason why not just setting WBS quirk here? Regards Marcel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities 2021-05-13 15:14 ` [PATCH] " Marcel Holtmann @ 2021-05-13 16:32 ` Archie Pusaka 2021-05-13 19:03 ` Marcel Holtmann 0 siblings, 1 reply; 14+ messages in thread From: Archie Pusaka @ 2021-05-13 16:32 UTC (permalink / raw) To: Marcel Holtmann Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi, Johan Hedberg, Luiz Augusto von Dentz, LKML [Re-sending in plaintext, sorry for spam] Hi Marcel, On Thu, 13 May 2021 at 23:14, Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Archie, > > > RTL8822 chipset supports WBS, and this information is conveyed in > > btusb.c. However, the UART driver doesn't have this information just > > yet. > > > > Signed-off-by: Archie Pusaka <apusaka@chromium.org> > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > --- > > > > drivers/bluetooth/btrtl.c | 26 ++++++++++++++++---------- > > drivers/bluetooth/btrtl.h | 2 ++ > > drivers/bluetooth/hci_h5.c | 5 +---- > > 3 files changed, 19 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c > > index e7fe5fb22753..988a09860c6b 100644 > > --- a/drivers/bluetooth/btrtl.c > > +++ b/drivers/bluetooth/btrtl.c > > @@ -719,17 +719,8 @@ int btrtl_download_firmware(struct hci_dev *hdev, > > } > > EXPORT_SYMBOL_GPL(btrtl_download_firmware); > > > > -int btrtl_setup_realtek(struct hci_dev *hdev) > > +void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev) > > { > > - struct btrtl_device_info *btrtl_dev; > > - int ret; > > - > > - btrtl_dev = btrtl_initialize(hdev, NULL); > > - if (IS_ERR(btrtl_dev)) > > - return PTR_ERR(btrtl_dev); > > - > > - ret = btrtl_download_firmware(hdev, btrtl_dev); > > - > > /* Enable controller to do both LE scan and BR/EDR inquiry > > * simultaneously. > > */ > > @@ -750,6 +741,21 @@ int btrtl_setup_realtek(struct hci_dev *hdev) > > rtl_dev_dbg(hdev, "WBS supported not enabled."); > > break; > > } > > +} > > +EXPORT_SYMBOL_GPL(btrtl_set_quirks); > > + > > +int btrtl_setup_realtek(struct hci_dev *hdev) > > +{ > > + struct btrtl_device_info *btrtl_dev; > > + int ret; > > + > > + btrtl_dev = btrtl_initialize(hdev, NULL); > > + if (IS_ERR(btrtl_dev)) > > + return PTR_ERR(btrtl_dev); > > + > > + ret = btrtl_download_firmware(hdev, btrtl_dev); > > + > > + btrtl_set_quirks(hdev, btrtl_dev); > > > > btrtl_free(btrtl_dev); > > return ret; > > diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h > > index 2a582682136d..260167f01b08 100644 > > --- a/drivers/bluetooth/btrtl.h > > +++ b/drivers/bluetooth/btrtl.h > > @@ -54,6 +54,8 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev, > > void btrtl_free(struct btrtl_device_info *btrtl_dev); > > int btrtl_download_firmware(struct hci_dev *hdev, > > struct btrtl_device_info *btrtl_dev); > > +void btrtl_set_quirks(struct hci_dev *hdev, > > + struct btrtl_device_info *btrtl_dev); > > int btrtl_setup_realtek(struct hci_dev *hdev); > > int btrtl_shutdown_realtek(struct hci_dev *hdev); > > int btrtl_get_uart_settings(struct hci_dev *hdev, > > diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c > > index 27e96681d583..e0520639f4ba 100644 > > --- a/drivers/bluetooth/hci_h5.c > > +++ b/drivers/bluetooth/hci_h5.c > > @@ -906,10 +906,7 @@ static int h5_btrtl_setup(struct h5 *h5) > > /* Give the device some time before the hci-core sends it a reset */ > > usleep_range(10000, 20000); > > > > - /* Enable controller to do both LE scan and BR/EDR inquiry > > - * simultaneously. > > - */ > > - set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks); > > + btrtl_set_quirks(h5->hu->hdev, btrtl_dev); > > any reason why not just setting WBS quirk here? Hmm, I think WBS is the feature of the chipset and not the transport. Therefore isn't it better to just have it set in one place? Setting the quirks here means we need to copy paste the settings from btrtl.c. Cheers, Archie ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities 2021-05-13 16:32 ` Archie Pusaka @ 2021-05-13 19:03 ` Marcel Holtmann 2021-05-14 11:40 ` Archie Pusaka 0 siblings, 1 reply; 14+ messages in thread From: Marcel Holtmann @ 2021-05-13 19:03 UTC (permalink / raw) To: Archie Pusaka Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi, Johan Hedberg, Luiz Augusto von Dentz, LKML Hi Archie, >>> RTL8822 chipset supports WBS, and this information is conveyed in >>> btusb.c. However, the UART driver doesn't have this information just >>> yet. >>> >>> Signed-off-by: Archie Pusaka <apusaka@chromium.org> >>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> >>> --- >>> >>> drivers/bluetooth/btrtl.c | 26 ++++++++++++++++---------- >>> drivers/bluetooth/btrtl.h | 2 ++ >>> drivers/bluetooth/hci_h5.c | 5 +---- >>> 3 files changed, 19 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c >>> index e7fe5fb22753..988a09860c6b 100644 >>> --- a/drivers/bluetooth/btrtl.c >>> +++ b/drivers/bluetooth/btrtl.c >>> @@ -719,17 +719,8 @@ int btrtl_download_firmware(struct hci_dev *hdev, >>> } >>> EXPORT_SYMBOL_GPL(btrtl_download_firmware); >>> >>> -int btrtl_setup_realtek(struct hci_dev *hdev) >>> +void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev) >>> { >>> - struct btrtl_device_info *btrtl_dev; >>> - int ret; >>> - >>> - btrtl_dev = btrtl_initialize(hdev, NULL); >>> - if (IS_ERR(btrtl_dev)) >>> - return PTR_ERR(btrtl_dev); >>> - >>> - ret = btrtl_download_firmware(hdev, btrtl_dev); >>> - >>> /* Enable controller to do both LE scan and BR/EDR inquiry >>> * simultaneously. >>> */ >>> @@ -750,6 +741,21 @@ int btrtl_setup_realtek(struct hci_dev *hdev) >>> rtl_dev_dbg(hdev, "WBS supported not enabled."); >>> break; >>> } >>> +} >>> +EXPORT_SYMBOL_GPL(btrtl_set_quirks); >>> + >>> +int btrtl_setup_realtek(struct hci_dev *hdev) >>> +{ >>> + struct btrtl_device_info *btrtl_dev; >>> + int ret; >>> + >>> + btrtl_dev = btrtl_initialize(hdev, NULL); >>> + if (IS_ERR(btrtl_dev)) >>> + return PTR_ERR(btrtl_dev); >>> + >>> + ret = btrtl_download_firmware(hdev, btrtl_dev); >>> + >>> + btrtl_set_quirks(hdev, btrtl_dev); >>> >>> btrtl_free(btrtl_dev); >>> return ret; >>> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h >>> index 2a582682136d..260167f01b08 100644 >>> --- a/drivers/bluetooth/btrtl.h >>> +++ b/drivers/bluetooth/btrtl.h >>> @@ -54,6 +54,8 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev, >>> void btrtl_free(struct btrtl_device_info *btrtl_dev); >>> int btrtl_download_firmware(struct hci_dev *hdev, >>> struct btrtl_device_info *btrtl_dev); >>> +void btrtl_set_quirks(struct hci_dev *hdev, >>> + struct btrtl_device_info *btrtl_dev); >>> int btrtl_setup_realtek(struct hci_dev *hdev); >>> int btrtl_shutdown_realtek(struct hci_dev *hdev); >>> int btrtl_get_uart_settings(struct hci_dev *hdev, >>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c >>> index 27e96681d583..e0520639f4ba 100644 >>> --- a/drivers/bluetooth/hci_h5.c >>> +++ b/drivers/bluetooth/hci_h5.c >>> @@ -906,10 +906,7 @@ static int h5_btrtl_setup(struct h5 *h5) >>> /* Give the device some time before the hci-core sends it a reset */ >>> usleep_range(10000, 20000); >>> >>> - /* Enable controller to do both LE scan and BR/EDR inquiry >>> - * simultaneously. >>> - */ >>> - set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks); >>> + btrtl_set_quirks(h5->hu->hdev, btrtl_dev); >> >> any reason why not just setting WBS quirk here? > > Hmm, I think WBS is the feature of the chipset and not the transport. > Therefore isn't it better to just have it set in one place? > Setting the quirks here means we need to copy paste the settings from btrtl.c. but since you are already setting HCI_QUIRK_SIMULTANEOUS_DISCOVERY right now, I don’t see the difference. Can we actually verify that we still need the WBS quirk. I think we fixed the broken errerrnous packet flag handling. Regards Marcel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities 2021-05-13 19:03 ` Marcel Holtmann @ 2021-05-14 11:40 ` Archie Pusaka 2021-05-17 4:31 ` Archie Pusaka 0 siblings, 1 reply; 14+ messages in thread From: Archie Pusaka @ 2021-05-14 11:40 UTC (permalink / raw) To: Marcel Holtmann Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi, Johan Hedberg, Luiz Augusto von Dentz, LKML Hi Marcel, On Fri, 14 May 2021 at 03:03, Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Archie, > > >>> RTL8822 chipset supports WBS, and this information is conveyed in > >>> btusb.c. However, the UART driver doesn't have this information just > >>> yet. > >>> > >>> Signed-off-by: Archie Pusaka <apusaka@chromium.org> > >>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > >>> --- > >>> > >>> drivers/bluetooth/btrtl.c | 26 ++++++++++++++++---------- > >>> drivers/bluetooth/btrtl.h | 2 ++ > >>> drivers/bluetooth/hci_h5.c | 5 +---- > >>> 3 files changed, 19 insertions(+), 14 deletions(-) > >>> > >>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c > >>> index e7fe5fb22753..988a09860c6b 100644 > >>> --- a/drivers/bluetooth/btrtl.c > >>> +++ b/drivers/bluetooth/btrtl.c > >>> @@ -719,17 +719,8 @@ int btrtl_download_firmware(struct hci_dev *hdev, > >>> } > >>> EXPORT_SYMBOL_GPL(btrtl_download_firmware); > >>> > >>> -int btrtl_setup_realtek(struct hci_dev *hdev) > >>> +void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev) > >>> { > >>> - struct btrtl_device_info *btrtl_dev; > >>> - int ret; > >>> - > >>> - btrtl_dev = btrtl_initialize(hdev, NULL); > >>> - if (IS_ERR(btrtl_dev)) > >>> - return PTR_ERR(btrtl_dev); > >>> - > >>> - ret = btrtl_download_firmware(hdev, btrtl_dev); > >>> - > >>> /* Enable controller to do both LE scan and BR/EDR inquiry > >>> * simultaneously. > >>> */ > >>> @@ -750,6 +741,21 @@ int btrtl_setup_realtek(struct hci_dev *hdev) > >>> rtl_dev_dbg(hdev, "WBS supported not enabled."); > >>> break; > >>> } > >>> +} > >>> +EXPORT_SYMBOL_GPL(btrtl_set_quirks); > >>> + > >>> +int btrtl_setup_realtek(struct hci_dev *hdev) > >>> +{ > >>> + struct btrtl_device_info *btrtl_dev; > >>> + int ret; > >>> + > >>> + btrtl_dev = btrtl_initialize(hdev, NULL); > >>> + if (IS_ERR(btrtl_dev)) > >>> + return PTR_ERR(btrtl_dev); > >>> + > >>> + ret = btrtl_download_firmware(hdev, btrtl_dev); > >>> + > >>> + btrtl_set_quirks(hdev, btrtl_dev); > >>> > >>> btrtl_free(btrtl_dev); > >>> return ret; > >>> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h > >>> index 2a582682136d..260167f01b08 100644 > >>> --- a/drivers/bluetooth/btrtl.h > >>> +++ b/drivers/bluetooth/btrtl.h > >>> @@ -54,6 +54,8 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev, > >>> void btrtl_free(struct btrtl_device_info *btrtl_dev); > >>> int btrtl_download_firmware(struct hci_dev *hdev, > >>> struct btrtl_device_info *btrtl_dev); > >>> +void btrtl_set_quirks(struct hci_dev *hdev, > >>> + struct btrtl_device_info *btrtl_dev); > >>> int btrtl_setup_realtek(struct hci_dev *hdev); > >>> int btrtl_shutdown_realtek(struct hci_dev *hdev); > >>> int btrtl_get_uart_settings(struct hci_dev *hdev, > >>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c > >>> index 27e96681d583..e0520639f4ba 100644 > >>> --- a/drivers/bluetooth/hci_h5.c > >>> +++ b/drivers/bluetooth/hci_h5.c > >>> @@ -906,10 +906,7 @@ static int h5_btrtl_setup(struct h5 *h5) > >>> /* Give the device some time before the hci-core sends it a reset */ > >>> usleep_range(10000, 20000); > >>> > >>> - /* Enable controller to do both LE scan and BR/EDR inquiry > >>> - * simultaneously. > >>> - */ > >>> - set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks); > >>> + btrtl_set_quirks(h5->hu->hdev, btrtl_dev); > >> > >> any reason why not just setting WBS quirk here? > > > > Hmm, I think WBS is the feature of the chipset and not the transport. > > Therefore isn't it better to just have it set in one place? > > Setting the quirks here means we need to copy paste the settings from btrtl.c. > > but since you are already setting HCI_QUIRK_SIMULTANEOUS_DISCOVERY right now, I don’t see the difference. Sorry, I don't get what you mean. With this patch I also moved HCI_QUIRK_SIMULTANEOUS_DISCOVERY into btrtl.c, so it's together with the WBS quirk. > Can we actually verify that we still need the WBS quirk. I think we fixed the broken errerrnous packet flag handling. To be honest, I am not aware about the story of the broken erroneous packet flag. Last time I checked I still needed the quirk to have RTL8822 on UART properly run WBS, but that was months ago... Let me verify whether this quirk is still needed. Cheers, Archie ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities 2021-05-14 11:40 ` Archie Pusaka @ 2021-05-17 4:31 ` Archie Pusaka 2021-05-20 4:45 ` Archie Pusaka 2021-05-20 15:18 ` Marcel Holtmann 0 siblings, 2 replies; 14+ messages in thread From: Archie Pusaka @ 2021-05-17 4:31 UTC (permalink / raw) To: Marcel Holtmann Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi, Johan Hedberg, Luiz Augusto von Dentz, LKML Hi Marcel, On Fri, 14 May 2021 at 19:40, Archie Pusaka <apusaka@google.com> wrote: > > Hi Marcel, > > On Fri, 14 May 2021 at 03:03, Marcel Holtmann <marcel@holtmann.org> wrote: > > > > Hi Archie, > > > > >>> RTL8822 chipset supports WBS, and this information is conveyed in > > >>> btusb.c. However, the UART driver doesn't have this information just > > >>> yet. > > >>> > > >>> Signed-off-by: Archie Pusaka <apusaka@chromium.org> > > >>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > >>> --- > > >>> > > >>> drivers/bluetooth/btrtl.c | 26 ++++++++++++++++---------- > > >>> drivers/bluetooth/btrtl.h | 2 ++ > > >>> drivers/bluetooth/hci_h5.c | 5 +---- > > >>> 3 files changed, 19 insertions(+), 14 deletions(-) > > >>> > > >>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c > > >>> index e7fe5fb22753..988a09860c6b 100644 > > >>> --- a/drivers/bluetooth/btrtl.c > > >>> +++ b/drivers/bluetooth/btrtl.c > > >>> @@ -719,17 +719,8 @@ int btrtl_download_firmware(struct hci_dev *hdev, > > >>> } > > >>> EXPORT_SYMBOL_GPL(btrtl_download_firmware); > > >>> > > >>> -int btrtl_setup_realtek(struct hci_dev *hdev) > > >>> +void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev) > > >>> { > > >>> - struct btrtl_device_info *btrtl_dev; > > >>> - int ret; > > >>> - > > >>> - btrtl_dev = btrtl_initialize(hdev, NULL); > > >>> - if (IS_ERR(btrtl_dev)) > > >>> - return PTR_ERR(btrtl_dev); > > >>> - > > >>> - ret = btrtl_download_firmware(hdev, btrtl_dev); > > >>> - > > >>> /* Enable controller to do both LE scan and BR/EDR inquiry > > >>> * simultaneously. > > >>> */ > > >>> @@ -750,6 +741,21 @@ int btrtl_setup_realtek(struct hci_dev *hdev) > > >>> rtl_dev_dbg(hdev, "WBS supported not enabled."); > > >>> break; > > >>> } > > >>> +} > > >>> +EXPORT_SYMBOL_GPL(btrtl_set_quirks); > > >>> + > > >>> +int btrtl_setup_realtek(struct hci_dev *hdev) > > >>> +{ > > >>> + struct btrtl_device_info *btrtl_dev; > > >>> + int ret; > > >>> + > > >>> + btrtl_dev = btrtl_initialize(hdev, NULL); > > >>> + if (IS_ERR(btrtl_dev)) > > >>> + return PTR_ERR(btrtl_dev); > > >>> + > > >>> + ret = btrtl_download_firmware(hdev, btrtl_dev); > > >>> + > > >>> + btrtl_set_quirks(hdev, btrtl_dev); > > >>> > > >>> btrtl_free(btrtl_dev); > > >>> return ret; > > >>> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h > > >>> index 2a582682136d..260167f01b08 100644 > > >>> --- a/drivers/bluetooth/btrtl.h > > >>> +++ b/drivers/bluetooth/btrtl.h > > >>> @@ -54,6 +54,8 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev, > > >>> void btrtl_free(struct btrtl_device_info *btrtl_dev); > > >>> int btrtl_download_firmware(struct hci_dev *hdev, > > >>> struct btrtl_device_info *btrtl_dev); > > >>> +void btrtl_set_quirks(struct hci_dev *hdev, > > >>> + struct btrtl_device_info *btrtl_dev); > > >>> int btrtl_setup_realtek(struct hci_dev *hdev); > > >>> int btrtl_shutdown_realtek(struct hci_dev *hdev); > > >>> int btrtl_get_uart_settings(struct hci_dev *hdev, > > >>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c > > >>> index 27e96681d583..e0520639f4ba 100644 > > >>> --- a/drivers/bluetooth/hci_h5.c > > >>> +++ b/drivers/bluetooth/hci_h5.c > > >>> @@ -906,10 +906,7 @@ static int h5_btrtl_setup(struct h5 *h5) > > >>> /* Give the device some time before the hci-core sends it a reset */ > > >>> usleep_range(10000, 20000); > > >>> > > >>> - /* Enable controller to do both LE scan and BR/EDR inquiry > > >>> - * simultaneously. > > >>> - */ > > >>> - set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks); > > >>> + btrtl_set_quirks(h5->hu->hdev, btrtl_dev); > > >> > > >> any reason why not just setting WBS quirk here? > > > > > > Hmm, I think WBS is the feature of the chipset and not the transport. > > > Therefore isn't it better to just have it set in one place? > > > Setting the quirks here means we need to copy paste the settings from btrtl.c. > > > > but since you are already setting HCI_QUIRK_SIMULTANEOUS_DISCOVERY right now, I don’t see the difference. > > Sorry, I don't get what you mean. > With this patch I also moved HCI_QUIRK_SIMULTANEOUS_DISCOVERY into > btrtl.c, so it's together with the WBS quirk. > > > Can we actually verify that we still need the WBS quirk. I think we fixed the broken errerrnous packet flag handling. > > To be honest, I am not aware about the story of the broken erroneous > packet flag. > Last time I checked I still needed the quirk to have RTL8822 on UART > properly run WBS, but that was months ago... > Let me verify whether this quirk is still needed. It looks like we still need the WBS quirk because otherwise the host wouldn't know whether the controller supports WBS or not. It's used in get_supported_settings() in mgmt.c. > Cheers, > Archie ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities 2021-05-17 4:31 ` Archie Pusaka @ 2021-05-20 4:45 ` Archie Pusaka 2021-05-20 15:18 ` Marcel Holtmann 1 sibling, 0 replies; 14+ messages in thread From: Archie Pusaka @ 2021-05-20 4:45 UTC (permalink / raw) To: Marcel Holtmann Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi, Johan Hedberg, Luiz Augusto von Dentz, LKML Hi Marcel, Friendly ping to review this patch again. Thanks! On Mon, 17 May 2021 at 12:31, Archie Pusaka <apusaka@google.com> wrote: > > Hi Marcel, > > On Fri, 14 May 2021 at 19:40, Archie Pusaka <apusaka@google.com> wrote: > > > > Hi Marcel, > > > > On Fri, 14 May 2021 at 03:03, Marcel Holtmann <marcel@holtmann.org> wrote: > > > > > > Hi Archie, > > > > > > >>> RTL8822 chipset supports WBS, and this information is conveyed in > > > >>> btusb.c. However, the UART driver doesn't have this information just > > > >>> yet. > > > >>> > > > >>> Signed-off-by: Archie Pusaka <apusaka@chromium.org> > > > >>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > >>> --- > > > >>> > > > >>> drivers/bluetooth/btrtl.c | 26 ++++++++++++++++---------- > > > >>> drivers/bluetooth/btrtl.h | 2 ++ > > > >>> drivers/bluetooth/hci_h5.c | 5 +---- > > > >>> 3 files changed, 19 insertions(+), 14 deletions(-) > > > >>> > > > >>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c > > > >>> index e7fe5fb22753..988a09860c6b 100644 > > > >>> --- a/drivers/bluetooth/btrtl.c > > > >>> +++ b/drivers/bluetooth/btrtl.c > > > >>> @@ -719,17 +719,8 @@ int btrtl_download_firmware(struct hci_dev *hdev, > > > >>> } > > > >>> EXPORT_SYMBOL_GPL(btrtl_download_firmware); > > > >>> > > > >>> -int btrtl_setup_realtek(struct hci_dev *hdev) > > > >>> +void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev) > > > >>> { > > > >>> - struct btrtl_device_info *btrtl_dev; > > > >>> - int ret; > > > >>> - > > > >>> - btrtl_dev = btrtl_initialize(hdev, NULL); > > > >>> - if (IS_ERR(btrtl_dev)) > > > >>> - return PTR_ERR(btrtl_dev); > > > >>> - > > > >>> - ret = btrtl_download_firmware(hdev, btrtl_dev); > > > >>> - > > > >>> /* Enable controller to do both LE scan and BR/EDR inquiry > > > >>> * simultaneously. > > > >>> */ > > > >>> @@ -750,6 +741,21 @@ int btrtl_setup_realtek(struct hci_dev *hdev) > > > >>> rtl_dev_dbg(hdev, "WBS supported not enabled."); > > > >>> break; > > > >>> } > > > >>> +} > > > >>> +EXPORT_SYMBOL_GPL(btrtl_set_quirks); > > > >>> + > > > >>> +int btrtl_setup_realtek(struct hci_dev *hdev) > > > >>> +{ > > > >>> + struct btrtl_device_info *btrtl_dev; > > > >>> + int ret; > > > >>> + > > > >>> + btrtl_dev = btrtl_initialize(hdev, NULL); > > > >>> + if (IS_ERR(btrtl_dev)) > > > >>> + return PTR_ERR(btrtl_dev); > > > >>> + > > > >>> + ret = btrtl_download_firmware(hdev, btrtl_dev); > > > >>> + > > > >>> + btrtl_set_quirks(hdev, btrtl_dev); > > > >>> > > > >>> btrtl_free(btrtl_dev); > > > >>> return ret; > > > >>> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h > > > >>> index 2a582682136d..260167f01b08 100644 > > > >>> --- a/drivers/bluetooth/btrtl.h > > > >>> +++ b/drivers/bluetooth/btrtl.h > > > >>> @@ -54,6 +54,8 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev, > > > >>> void btrtl_free(struct btrtl_device_info *btrtl_dev); > > > >>> int btrtl_download_firmware(struct hci_dev *hdev, > > > >>> struct btrtl_device_info *btrtl_dev); > > > >>> +void btrtl_set_quirks(struct hci_dev *hdev, > > > >>> + struct btrtl_device_info *btrtl_dev); > > > >>> int btrtl_setup_realtek(struct hci_dev *hdev); > > > >>> int btrtl_shutdown_realtek(struct hci_dev *hdev); > > > >>> int btrtl_get_uart_settings(struct hci_dev *hdev, > > > >>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c > > > >>> index 27e96681d583..e0520639f4ba 100644 > > > >>> --- a/drivers/bluetooth/hci_h5.c > > > >>> +++ b/drivers/bluetooth/hci_h5.c > > > >>> @@ -906,10 +906,7 @@ static int h5_btrtl_setup(struct h5 *h5) > > > >>> /* Give the device some time before the hci-core sends it a reset */ > > > >>> usleep_range(10000, 20000); > > > >>> > > > >>> - /* Enable controller to do both LE scan and BR/EDR inquiry > > > >>> - * simultaneously. > > > >>> - */ > > > >>> - set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks); > > > >>> + btrtl_set_quirks(h5->hu->hdev, btrtl_dev); > > > >> > > > >> any reason why not just setting WBS quirk here? > > > > > > > > Hmm, I think WBS is the feature of the chipset and not the transport. > > > > Therefore isn't it better to just have it set in one place? > > > > Setting the quirks here means we need to copy paste the settings from btrtl.c. > > > > > > but since you are already setting HCI_QUIRK_SIMULTANEOUS_DISCOVERY right now, I don’t see the difference. > > > > Sorry, I don't get what you mean. > > With this patch I also moved HCI_QUIRK_SIMULTANEOUS_DISCOVERY into > > btrtl.c, so it's together with the WBS quirk. > > > > > Can we actually verify that we still need the WBS quirk. I think we fixed the broken errerrnous packet flag handling. > > > > To be honest, I am not aware about the story of the broken erroneous > > packet flag. > > Last time I checked I still needed the quirk to have RTL8822 on UART > > properly run WBS, but that was months ago... > > Let me verify whether this quirk is still needed. > > It looks like we still need the WBS quirk because otherwise the host > wouldn't know whether the controller supports WBS or not. It's used in > get_supported_settings() in mgmt.c. > > > Cheers, > > Archie ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities 2021-05-17 4:31 ` Archie Pusaka 2021-05-20 4:45 ` Archie Pusaka @ 2021-05-20 15:18 ` Marcel Holtmann 2021-05-20 16:02 ` Archie Pusaka 1 sibling, 1 reply; 14+ messages in thread From: Marcel Holtmann @ 2021-05-20 15:18 UTC (permalink / raw) To: Archie Pusaka Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi, Johan Hedberg, Luiz Augusto von Dentz, LKML Hi Archie, >>>>>> RTL8822 chipset supports WBS, and this information is conveyed in >>>>>> btusb.c. However, the UART driver doesn't have this information just >>>>>> yet. >>>>>> >>>>>> Signed-off-by: Archie Pusaka <apusaka@chromium.org> >>>>>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> >>>>>> --- >>>>>> >>>>>> drivers/bluetooth/btrtl.c | 26 ++++++++++++++++---------- >>>>>> drivers/bluetooth/btrtl.h | 2 ++ >>>>>> drivers/bluetooth/hci_h5.c | 5 +---- >>>>>> 3 files changed, 19 insertions(+), 14 deletions(-) >>>>>> >>>>>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c >>>>>> index e7fe5fb22753..988a09860c6b 100644 >>>>>> --- a/drivers/bluetooth/btrtl.c >>>>>> +++ b/drivers/bluetooth/btrtl.c >>>>>> @@ -719,17 +719,8 @@ int btrtl_download_firmware(struct hci_dev *hdev, >>>>>> } >>>>>> EXPORT_SYMBOL_GPL(btrtl_download_firmware); >>>>>> >>>>>> -int btrtl_setup_realtek(struct hci_dev *hdev) >>>>>> +void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev) >>>>>> { >>>>>> - struct btrtl_device_info *btrtl_dev; >>>>>> - int ret; >>>>>> - >>>>>> - btrtl_dev = btrtl_initialize(hdev, NULL); >>>>>> - if (IS_ERR(btrtl_dev)) >>>>>> - return PTR_ERR(btrtl_dev); >>>>>> - >>>>>> - ret = btrtl_download_firmware(hdev, btrtl_dev); >>>>>> - >>>>>> /* Enable controller to do both LE scan and BR/EDR inquiry >>>>>> * simultaneously. >>>>>> */ >>>>>> @@ -750,6 +741,21 @@ int btrtl_setup_realtek(struct hci_dev *hdev) >>>>>> rtl_dev_dbg(hdev, "WBS supported not enabled."); >>>>>> break; >>>>>> } >>>>>> +} >>>>>> +EXPORT_SYMBOL_GPL(btrtl_set_quirks); >>>>>> + >>>>>> +int btrtl_setup_realtek(struct hci_dev *hdev) >>>>>> +{ >>>>>> + struct btrtl_device_info *btrtl_dev; >>>>>> + int ret; >>>>>> + >>>>>> + btrtl_dev = btrtl_initialize(hdev, NULL); >>>>>> + if (IS_ERR(btrtl_dev)) >>>>>> + return PTR_ERR(btrtl_dev); >>>>>> + >>>>>> + ret = btrtl_download_firmware(hdev, btrtl_dev); >>>>>> + >>>>>> + btrtl_set_quirks(hdev, btrtl_dev); >>>>>> >>>>>> btrtl_free(btrtl_dev); >>>>>> return ret; >>>>>> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h >>>>>> index 2a582682136d..260167f01b08 100644 >>>>>> --- a/drivers/bluetooth/btrtl.h >>>>>> +++ b/drivers/bluetooth/btrtl.h >>>>>> @@ -54,6 +54,8 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev, >>>>>> void btrtl_free(struct btrtl_device_info *btrtl_dev); >>>>>> int btrtl_download_firmware(struct hci_dev *hdev, >>>>>> struct btrtl_device_info *btrtl_dev); >>>>>> +void btrtl_set_quirks(struct hci_dev *hdev, >>>>>> + struct btrtl_device_info *btrtl_dev); >>>>>> int btrtl_setup_realtek(struct hci_dev *hdev); >>>>>> int btrtl_shutdown_realtek(struct hci_dev *hdev); >>>>>> int btrtl_get_uart_settings(struct hci_dev *hdev, >>>>>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c >>>>>> index 27e96681d583..e0520639f4ba 100644 >>>>>> --- a/drivers/bluetooth/hci_h5.c >>>>>> +++ b/drivers/bluetooth/hci_h5.c >>>>>> @@ -906,10 +906,7 @@ static int h5_btrtl_setup(struct h5 *h5) >>>>>> /* Give the device some time before the hci-core sends it a reset */ >>>>>> usleep_range(10000, 20000); >>>>>> >>>>>> - /* Enable controller to do both LE scan and BR/EDR inquiry >>>>>> - * simultaneously. >>>>>> - */ >>>>>> - set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks); >>>>>> + btrtl_set_quirks(h5->hu->hdev, btrtl_dev); >>>>> >>>>> any reason why not just setting WBS quirk here? >>>> >>>> Hmm, I think WBS is the feature of the chipset and not the transport. >>>> Therefore isn't it better to just have it set in one place? >>>> Setting the quirks here means we need to copy paste the settings from btrtl.c. >>> >>> but since you are already setting HCI_QUIRK_SIMULTANEOUS_DISCOVERY right now, I don’t see the difference. >> >> Sorry, I don't get what you mean. >> With this patch I also moved HCI_QUIRK_SIMULTANEOUS_DISCOVERY into >> btrtl.c, so it's together with the WBS quirk. >> >>> Can we actually verify that we still need the WBS quirk. I think we fixed the broken errerrnous packet flag handling. >> >> To be honest, I am not aware about the story of the broken erroneous >> packet flag. >> Last time I checked I still needed the quirk to have RTL8822 on UART >> properly run WBS, but that was months ago... >> Let me verify whether this quirk is still needed. > > It looks like we still need the WBS quirk because otherwise the host > wouldn't know whether the controller supports WBS or not. It's used in > get_supported_settings() in mgmt.c. and why not set it unconditionally for all Realtek chips? Regards Marcel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities 2021-05-20 15:18 ` Marcel Holtmann @ 2021-05-20 16:02 ` Archie Pusaka 2021-05-26 5:49 ` Archie Pusaka 0 siblings, 1 reply; 14+ messages in thread From: Archie Pusaka @ 2021-05-20 16:02 UTC (permalink / raw) To: Marcel Holtmann Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi, Johan Hedberg, Luiz Augusto von Dentz, LKML Hi Marcel, On Thu, 20 May 2021 at 23:18, Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Archie, > > >>>>>> RTL8822 chipset supports WBS, and this information is conveyed in > >>>>>> btusb.c. However, the UART driver doesn't have this information just > >>>>>> yet. > >>>>>> > >>>>>> Signed-off-by: Archie Pusaka <apusaka@chromium.org> > >>>>>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > >>>>>> --- > >>>>>> > >>>>>> drivers/bluetooth/btrtl.c | 26 ++++++++++++++++---------- > >>>>>> drivers/bluetooth/btrtl.h | 2 ++ > >>>>>> drivers/bluetooth/hci_h5.c | 5 +---- > >>>>>> 3 files changed, 19 insertions(+), 14 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c > >>>>>> index e7fe5fb22753..988a09860c6b 100644 > >>>>>> --- a/drivers/bluetooth/btrtl.c > >>>>>> +++ b/drivers/bluetooth/btrtl.c > >>>>>> @@ -719,17 +719,8 @@ int btrtl_download_firmware(struct hci_dev *hdev, > >>>>>> } > >>>>>> EXPORT_SYMBOL_GPL(btrtl_download_firmware); > >>>>>> > >>>>>> -int btrtl_setup_realtek(struct hci_dev *hdev) > >>>>>> +void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev) > >>>>>> { > >>>>>> - struct btrtl_device_info *btrtl_dev; > >>>>>> - int ret; > >>>>>> - > >>>>>> - btrtl_dev = btrtl_initialize(hdev, NULL); > >>>>>> - if (IS_ERR(btrtl_dev)) > >>>>>> - return PTR_ERR(btrtl_dev); > >>>>>> - > >>>>>> - ret = btrtl_download_firmware(hdev, btrtl_dev); > >>>>>> - > >>>>>> /* Enable controller to do both LE scan and BR/EDR inquiry > >>>>>> * simultaneously. > >>>>>> */ > >>>>>> @@ -750,6 +741,21 @@ int btrtl_setup_realtek(struct hci_dev *hdev) > >>>>>> rtl_dev_dbg(hdev, "WBS supported not enabled."); > >>>>>> break; > >>>>>> } > >>>>>> +} > >>>>>> +EXPORT_SYMBOL_GPL(btrtl_set_quirks); > >>>>>> + > >>>>>> +int btrtl_setup_realtek(struct hci_dev *hdev) > >>>>>> +{ > >>>>>> + struct btrtl_device_info *btrtl_dev; > >>>>>> + int ret; > >>>>>> + > >>>>>> + btrtl_dev = btrtl_initialize(hdev, NULL); > >>>>>> + if (IS_ERR(btrtl_dev)) > >>>>>> + return PTR_ERR(btrtl_dev); > >>>>>> + > >>>>>> + ret = btrtl_download_firmware(hdev, btrtl_dev); > >>>>>> + > >>>>>> + btrtl_set_quirks(hdev, btrtl_dev); > >>>>>> > >>>>>> btrtl_free(btrtl_dev); > >>>>>> return ret; > >>>>>> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h > >>>>>> index 2a582682136d..260167f01b08 100644 > >>>>>> --- a/drivers/bluetooth/btrtl.h > >>>>>> +++ b/drivers/bluetooth/btrtl.h > >>>>>> @@ -54,6 +54,8 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev, > >>>>>> void btrtl_free(struct btrtl_device_info *btrtl_dev); > >>>>>> int btrtl_download_firmware(struct hci_dev *hdev, > >>>>>> struct btrtl_device_info *btrtl_dev); > >>>>>> +void btrtl_set_quirks(struct hci_dev *hdev, > >>>>>> + struct btrtl_device_info *btrtl_dev); > >>>>>> int btrtl_setup_realtek(struct hci_dev *hdev); > >>>>>> int btrtl_shutdown_realtek(struct hci_dev *hdev); > >>>>>> int btrtl_get_uart_settings(struct hci_dev *hdev, > >>>>>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c > >>>>>> index 27e96681d583..e0520639f4ba 100644 > >>>>>> --- a/drivers/bluetooth/hci_h5.c > >>>>>> +++ b/drivers/bluetooth/hci_h5.c > >>>>>> @@ -906,10 +906,7 @@ static int h5_btrtl_setup(struct h5 *h5) > >>>>>> /* Give the device some time before the hci-core sends it a reset */ > >>>>>> usleep_range(10000, 20000); > >>>>>> > >>>>>> - /* Enable controller to do both LE scan and BR/EDR inquiry > >>>>>> - * simultaneously. > >>>>>> - */ > >>>>>> - set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks); > >>>>>> + btrtl_set_quirks(h5->hu->hdev, btrtl_dev); > >>>>> > >>>>> any reason why not just setting WBS quirk here? > >>>> > >>>> Hmm, I think WBS is the feature of the chipset and not the transport. > >>>> Therefore isn't it better to just have it set in one place? > >>>> Setting the quirks here means we need to copy paste the settings from btrtl.c. > >>> > >>> but since you are already setting HCI_QUIRK_SIMULTANEOUS_DISCOVERY right now, I don’t see the difference. > >> > >> Sorry, I don't get what you mean. > >> With this patch I also moved HCI_QUIRK_SIMULTANEOUS_DISCOVERY into > >> btrtl.c, so it's together with the WBS quirk. > >> > >>> Can we actually verify that we still need the WBS quirk. I think we fixed the broken errerrnous packet flag handling. > >> > >> To be honest, I am not aware about the story of the broken erroneous > >> packet flag. > >> Last time I checked I still needed the quirk to have RTL8822 on UART > >> properly run WBS, but that was months ago... > >> Let me verify whether this quirk is still needed. > > > > It looks like we still need the WBS quirk because otherwise the host > > wouldn't know whether the controller supports WBS or not. It's used in > > get_supported_settings() in mgmt.c. > > and why not set it unconditionally for all Realtek chips? Not all Realtek chips supports WBS, therefore HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED is only set on some of them. Thanks Archie ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities 2021-05-20 16:02 ` Archie Pusaka @ 2021-05-26 5:49 ` Archie Pusaka 2021-05-26 14:59 ` Marcel Holtmann 0 siblings, 1 reply; 14+ messages in thread From: Archie Pusaka @ 2021-05-26 5:49 UTC (permalink / raw) To: Marcel Holtmann Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi, Johan Hedberg, Luiz Augusto von Dentz, LKML Hi Marcel, On Fri, 21 May 2021 at 00:02, Archie Pusaka <apusaka@google.com> wrote: > > Hi Marcel, > > On Thu, 20 May 2021 at 23:18, Marcel Holtmann <marcel@holtmann.org> wrote: > > > > Hi Archie, > > > > >>>>>> RTL8822 chipset supports WBS, and this information is conveyed in > > >>>>>> btusb.c. However, the UART driver doesn't have this information just > > >>>>>> yet. > > >>>>>> > > >>>>>> Signed-off-by: Archie Pusaka <apusaka@chromium.org> > > >>>>>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > >>>>>> --- > > >>>>>> > > >>>>>> drivers/bluetooth/btrtl.c | 26 ++++++++++++++++---------- > > >>>>>> drivers/bluetooth/btrtl.h | 2 ++ > > >>>>>> drivers/bluetooth/hci_h5.c | 5 +---- > > >>>>>> 3 files changed, 19 insertions(+), 14 deletions(-) > > >>>>>> > > >>>>>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c > > >>>>>> index e7fe5fb22753..988a09860c6b 100644 > > >>>>>> --- a/drivers/bluetooth/btrtl.c > > >>>>>> +++ b/drivers/bluetooth/btrtl.c > > >>>>>> @@ -719,17 +719,8 @@ int btrtl_download_firmware(struct hci_dev *hdev, > > >>>>>> } > > >>>>>> EXPORT_SYMBOL_GPL(btrtl_download_firmware); > > >>>>>> > > >>>>>> -int btrtl_setup_realtek(struct hci_dev *hdev) > > >>>>>> +void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev) > > >>>>>> { > > >>>>>> - struct btrtl_device_info *btrtl_dev; > > >>>>>> - int ret; > > >>>>>> - > > >>>>>> - btrtl_dev = btrtl_initialize(hdev, NULL); > > >>>>>> - if (IS_ERR(btrtl_dev)) > > >>>>>> - return PTR_ERR(btrtl_dev); > > >>>>>> - > > >>>>>> - ret = btrtl_download_firmware(hdev, btrtl_dev); > > >>>>>> - > > >>>>>> /* Enable controller to do both LE scan and BR/EDR inquiry > > >>>>>> * simultaneously. > > >>>>>> */ > > >>>>>> @@ -750,6 +741,21 @@ int btrtl_setup_realtek(struct hci_dev *hdev) > > >>>>>> rtl_dev_dbg(hdev, "WBS supported not enabled."); > > >>>>>> break; > > >>>>>> } > > >>>>>> +} > > >>>>>> +EXPORT_SYMBOL_GPL(btrtl_set_quirks); > > >>>>>> + > > >>>>>> +int btrtl_setup_realtek(struct hci_dev *hdev) > > >>>>>> +{ > > >>>>>> + struct btrtl_device_info *btrtl_dev; > > >>>>>> + int ret; > > >>>>>> + > > >>>>>> + btrtl_dev = btrtl_initialize(hdev, NULL); > > >>>>>> + if (IS_ERR(btrtl_dev)) > > >>>>>> + return PTR_ERR(btrtl_dev); > > >>>>>> + > > >>>>>> + ret = btrtl_download_firmware(hdev, btrtl_dev); > > >>>>>> + > > >>>>>> + btrtl_set_quirks(hdev, btrtl_dev); > > >>>>>> > > >>>>>> btrtl_free(btrtl_dev); > > >>>>>> return ret; > > >>>>>> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h > > >>>>>> index 2a582682136d..260167f01b08 100644 > > >>>>>> --- a/drivers/bluetooth/btrtl.h > > >>>>>> +++ b/drivers/bluetooth/btrtl.h > > >>>>>> @@ -54,6 +54,8 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev, > > >>>>>> void btrtl_free(struct btrtl_device_info *btrtl_dev); > > >>>>>> int btrtl_download_firmware(struct hci_dev *hdev, > > >>>>>> struct btrtl_device_info *btrtl_dev); > > >>>>>> +void btrtl_set_quirks(struct hci_dev *hdev, > > >>>>>> + struct btrtl_device_info *btrtl_dev); > > >>>>>> int btrtl_setup_realtek(struct hci_dev *hdev); > > >>>>>> int btrtl_shutdown_realtek(struct hci_dev *hdev); > > >>>>>> int btrtl_get_uart_settings(struct hci_dev *hdev, > > >>>>>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c > > >>>>>> index 27e96681d583..e0520639f4ba 100644 > > >>>>>> --- a/drivers/bluetooth/hci_h5.c > > >>>>>> +++ b/drivers/bluetooth/hci_h5.c > > >>>>>> @@ -906,10 +906,7 @@ static int h5_btrtl_setup(struct h5 *h5) > > >>>>>> /* Give the device some time before the hci-core sends it a reset */ > > >>>>>> usleep_range(10000, 20000); > > >>>>>> > > >>>>>> - /* Enable controller to do both LE scan and BR/EDR inquiry > > >>>>>> - * simultaneously. > > >>>>>> - */ > > >>>>>> - set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks); > > >>>>>> + btrtl_set_quirks(h5->hu->hdev, btrtl_dev); > > >>>>> > > >>>>> any reason why not just setting WBS quirk here? > > >>>> > > >>>> Hmm, I think WBS is the feature of the chipset and not the transport. > > >>>> Therefore isn't it better to just have it set in one place? > > >>>> Setting the quirks here means we need to copy paste the settings from btrtl.c. > > >>> > > >>> but since you are already setting HCI_QUIRK_SIMULTANEOUS_DISCOVERY right now, I don’t see the difference. > > >> > > >> Sorry, I don't get what you mean. > > >> With this patch I also moved HCI_QUIRK_SIMULTANEOUS_DISCOVERY into > > >> btrtl.c, so it's together with the WBS quirk. > > >> > > >>> Can we actually verify that we still need the WBS quirk. I think we fixed the broken errerrnous packet flag handling. > > >> > > >> To be honest, I am not aware about the story of the broken erroneous > > >> packet flag. > > >> Last time I checked I still needed the quirk to have RTL8822 on UART > > >> properly run WBS, but that was months ago... > > >> Let me verify whether this quirk is still needed. > > > > > > It looks like we still need the WBS quirk because otherwise the host > > > wouldn't know whether the controller supports WBS or not. It's used in > > > get_supported_settings() in mgmt.c. > > > > and why not set it unconditionally for all Realtek chips? > > Not all Realtek chips supports WBS, therefore > HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED is only set on some of them. Are there any other concerns you might have? Cheers, Archie ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities 2021-05-26 5:49 ` Archie Pusaka @ 2021-05-26 14:59 ` Marcel Holtmann 2021-05-27 7:05 ` Archie Pusaka 0 siblings, 1 reply; 14+ messages in thread From: Marcel Holtmann @ 2021-05-26 14:59 UTC (permalink / raw) To: Archie Pusaka Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi, Johan Hedberg, Luiz Augusto von Dentz, LKML Hi Archie, >>>>>>>>> RTL8822 chipset supports WBS, and this information is conveyed in >>>>>>>>> btusb.c. However, the UART driver doesn't have this information just >>>>>>>>> yet. >>>>>>>>> >>>>>>>>> Signed-off-by: Archie Pusaka <apusaka@chromium.org> >>>>>>>>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> >>>>>>>>> --- >>>>>>>>> >>>>>>>>> drivers/bluetooth/btrtl.c | 26 ++++++++++++++++---------- >>>>>>>>> drivers/bluetooth/btrtl.h | 2 ++ >>>>>>>>> drivers/bluetooth/hci_h5.c | 5 +---- >>>>>>>>> 3 files changed, 19 insertions(+), 14 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c >>>>>>>>> index e7fe5fb22753..988a09860c6b 100644 >>>>>>>>> --- a/drivers/bluetooth/btrtl.c >>>>>>>>> +++ b/drivers/bluetooth/btrtl.c >>>>>>>>> @@ -719,17 +719,8 @@ int btrtl_download_firmware(struct hci_dev *hdev, >>>>>>>>> } >>>>>>>>> EXPORT_SYMBOL_GPL(btrtl_download_firmware); >>>>>>>>> >>>>>>>>> -int btrtl_setup_realtek(struct hci_dev *hdev) >>>>>>>>> +void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev) >>>>>>>>> { >>>>>>>>> - struct btrtl_device_info *btrtl_dev; >>>>>>>>> - int ret; >>>>>>>>> - >>>>>>>>> - btrtl_dev = btrtl_initialize(hdev, NULL); >>>>>>>>> - if (IS_ERR(btrtl_dev)) >>>>>>>>> - return PTR_ERR(btrtl_dev); >>>>>>>>> - >>>>>>>>> - ret = btrtl_download_firmware(hdev, btrtl_dev); >>>>>>>>> - >>>>>>>>> /* Enable controller to do both LE scan and BR/EDR inquiry >>>>>>>>> * simultaneously. >>>>>>>>> */ >>>>>>>>> @@ -750,6 +741,21 @@ int btrtl_setup_realtek(struct hci_dev *hdev) >>>>>>>>> rtl_dev_dbg(hdev, "WBS supported not enabled."); >>>>>>>>> break; >>>>>>>>> } >>>>>>>>> +} >>>>>>>>> +EXPORT_SYMBOL_GPL(btrtl_set_quirks); >>>>>>>>> + >>>>>>>>> +int btrtl_setup_realtek(struct hci_dev *hdev) >>>>>>>>> +{ >>>>>>>>> + struct btrtl_device_info *btrtl_dev; >>>>>>>>> + int ret; >>>>>>>>> + >>>>>>>>> + btrtl_dev = btrtl_initialize(hdev, NULL); >>>>>>>>> + if (IS_ERR(btrtl_dev)) >>>>>>>>> + return PTR_ERR(btrtl_dev); >>>>>>>>> + >>>>>>>>> + ret = btrtl_download_firmware(hdev, btrtl_dev); >>>>>>>>> + >>>>>>>>> + btrtl_set_quirks(hdev, btrtl_dev); >>>>>>>>> >>>>>>>>> btrtl_free(btrtl_dev); >>>>>>>>> return ret; >>>>>>>>> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h >>>>>>>>> index 2a582682136d..260167f01b08 100644 >>>>>>>>> --- a/drivers/bluetooth/btrtl.h >>>>>>>>> +++ b/drivers/bluetooth/btrtl.h >>>>>>>>> @@ -54,6 +54,8 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev, >>>>>>>>> void btrtl_free(struct btrtl_device_info *btrtl_dev); >>>>>>>>> int btrtl_download_firmware(struct hci_dev *hdev, >>>>>>>>> struct btrtl_device_info *btrtl_dev); >>>>>>>>> +void btrtl_set_quirks(struct hci_dev *hdev, >>>>>>>>> + struct btrtl_device_info *btrtl_dev); >>>>>>>>> int btrtl_setup_realtek(struct hci_dev *hdev); >>>>>>>>> int btrtl_shutdown_realtek(struct hci_dev *hdev); >>>>>>>>> int btrtl_get_uart_settings(struct hci_dev *hdev, >>>>>>>>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c >>>>>>>>> index 27e96681d583..e0520639f4ba 100644 >>>>>>>>> --- a/drivers/bluetooth/hci_h5.c >>>>>>>>> +++ b/drivers/bluetooth/hci_h5.c >>>>>>>>> @@ -906,10 +906,7 @@ static int h5_btrtl_setup(struct h5 *h5) >>>>>>>>> /* Give the device some time before the hci-core sends it a reset */ >>>>>>>>> usleep_range(10000, 20000); >>>>>>>>> >>>>>>>>> - /* Enable controller to do both LE scan and BR/EDR inquiry >>>>>>>>> - * simultaneously. >>>>>>>>> - */ >>>>>>>>> - set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks); >>>>>>>>> + btrtl_set_quirks(h5->hu->hdev, btrtl_dev); >>>>>>>> >>>>>>>> any reason why not just setting WBS quirk here? >>>>>>> >>>>>>> Hmm, I think WBS is the feature of the chipset and not the transport. >>>>>>> Therefore isn't it better to just have it set in one place? >>>>>>> Setting the quirks here means we need to copy paste the settings from btrtl.c. >>>>>> >>>>>> but since you are already setting HCI_QUIRK_SIMULTANEOUS_DISCOVERY right now, I don’t see the difference. >>>>> >>>>> Sorry, I don't get what you mean. >>>>> With this patch I also moved HCI_QUIRK_SIMULTANEOUS_DISCOVERY into >>>>> btrtl.c, so it's together with the WBS quirk. >>>>> >>>>>> Can we actually verify that we still need the WBS quirk. I think we fixed the broken errerrnous packet flag handling. >>>>> >>>>> To be honest, I am not aware about the story of the broken erroneous >>>>> packet flag. >>>>> Last time I checked I still needed the quirk to have RTL8822 on UART >>>>> properly run WBS, but that was months ago... >>>>> Let me verify whether this quirk is still needed. >>>> >>>> It looks like we still need the WBS quirk because otherwise the host >>>> wouldn't know whether the controller supports WBS or not. It's used in >>>> get_supported_settings() in mgmt.c. >>> >>> and why not set it unconditionally for all Realtek chips? >> >> Not all Realtek chips supports WBS, therefore >> HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED is only set on some of them. > > Are there any other concerns you might have? can we do the quirk setting in btrtl_setup_realtek() instead of creating another exported function. Regards Marcel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities 2021-05-26 14:59 ` Marcel Holtmann @ 2021-05-27 7:05 ` Archie Pusaka 2021-05-27 15:13 ` Marcel Holtmann 0 siblings, 1 reply; 14+ messages in thread From: Archie Pusaka @ 2021-05-27 7:05 UTC (permalink / raw) To: Marcel Holtmann Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi, Johan Hedberg, Luiz Augusto von Dentz, LKML Hi Marcel, On Wed, 26 May 2021 at 22:59, Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Archie, > > >>>>>>>>> RTL8822 chipset supports WBS, and this information is conveyed in > >>>>>>>>> btusb.c. However, the UART driver doesn't have this information just > >>>>>>>>> yet. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Archie Pusaka <apusaka@chromium.org> > >>>>>>>>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > >>>>>>>>> --- > >>>>>>>>> > >>>>>>>>> drivers/bluetooth/btrtl.c | 26 ++++++++++++++++---------- > >>>>>>>>> drivers/bluetooth/btrtl.h | 2 ++ > >>>>>>>>> drivers/bluetooth/hci_h5.c | 5 +---- > >>>>>>>>> 3 files changed, 19 insertions(+), 14 deletions(-) > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c > >>>>>>>>> index e7fe5fb22753..988a09860c6b 100644 > >>>>>>>>> --- a/drivers/bluetooth/btrtl.c > >>>>>>>>> +++ b/drivers/bluetooth/btrtl.c > >>>>>>>>> @@ -719,17 +719,8 @@ int btrtl_download_firmware(struct hci_dev *hdev, > >>>>>>>>> } > >>>>>>>>> EXPORT_SYMBOL_GPL(btrtl_download_firmware); > >>>>>>>>> > >>>>>>>>> -int btrtl_setup_realtek(struct hci_dev *hdev) > >>>>>>>>> +void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev) > >>>>>>>>> { > >>>>>>>>> - struct btrtl_device_info *btrtl_dev; > >>>>>>>>> - int ret; > >>>>>>>>> - > >>>>>>>>> - btrtl_dev = btrtl_initialize(hdev, NULL); > >>>>>>>>> - if (IS_ERR(btrtl_dev)) > >>>>>>>>> - return PTR_ERR(btrtl_dev); > >>>>>>>>> - > >>>>>>>>> - ret = btrtl_download_firmware(hdev, btrtl_dev); > >>>>>>>>> - > >>>>>>>>> /* Enable controller to do both LE scan and BR/EDR inquiry > >>>>>>>>> * simultaneously. > >>>>>>>>> */ > >>>>>>>>> @@ -750,6 +741,21 @@ int btrtl_setup_realtek(struct hci_dev *hdev) > >>>>>>>>> rtl_dev_dbg(hdev, "WBS supported not enabled."); > >>>>>>>>> break; > >>>>>>>>> } > >>>>>>>>> +} > >>>>>>>>> +EXPORT_SYMBOL_GPL(btrtl_set_quirks); > >>>>>>>>> + > >>>>>>>>> +int btrtl_setup_realtek(struct hci_dev *hdev) > >>>>>>>>> +{ > >>>>>>>>> + struct btrtl_device_info *btrtl_dev; > >>>>>>>>> + int ret; > >>>>>>>>> + > >>>>>>>>> + btrtl_dev = btrtl_initialize(hdev, NULL); > >>>>>>>>> + if (IS_ERR(btrtl_dev)) > >>>>>>>>> + return PTR_ERR(btrtl_dev); > >>>>>>>>> + > >>>>>>>>> + ret = btrtl_download_firmware(hdev, btrtl_dev); > >>>>>>>>> + > >>>>>>>>> + btrtl_set_quirks(hdev, btrtl_dev); > >>>>>>>>> > >>>>>>>>> btrtl_free(btrtl_dev); > >>>>>>>>> return ret; > >>>>>>>>> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h > >>>>>>>>> index 2a582682136d..260167f01b08 100644 > >>>>>>>>> --- a/drivers/bluetooth/btrtl.h > >>>>>>>>> +++ b/drivers/bluetooth/btrtl.h > >>>>>>>>> @@ -54,6 +54,8 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev, > >>>>>>>>> void btrtl_free(struct btrtl_device_info *btrtl_dev); > >>>>>>>>> int btrtl_download_firmware(struct hci_dev *hdev, > >>>>>>>>> struct btrtl_device_info *btrtl_dev); > >>>>>>>>> +void btrtl_set_quirks(struct hci_dev *hdev, > >>>>>>>>> + struct btrtl_device_info *btrtl_dev); > >>>>>>>>> int btrtl_setup_realtek(struct hci_dev *hdev); > >>>>>>>>> int btrtl_shutdown_realtek(struct hci_dev *hdev); > >>>>>>>>> int btrtl_get_uart_settings(struct hci_dev *hdev, > >>>>>>>>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c > >>>>>>>>> index 27e96681d583..e0520639f4ba 100644 > >>>>>>>>> --- a/drivers/bluetooth/hci_h5.c > >>>>>>>>> +++ b/drivers/bluetooth/hci_h5.c > >>>>>>>>> @@ -906,10 +906,7 @@ static int h5_btrtl_setup(struct h5 *h5) > >>>>>>>>> /* Give the device some time before the hci-core sends it a reset */ > >>>>>>>>> usleep_range(10000, 20000); > >>>>>>>>> > >>>>>>>>> - /* Enable controller to do both LE scan and BR/EDR inquiry > >>>>>>>>> - * simultaneously. > >>>>>>>>> - */ > >>>>>>>>> - set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks); > >>>>>>>>> + btrtl_set_quirks(h5->hu->hdev, btrtl_dev); > >>>>>>>> > >>>>>>>> any reason why not just setting WBS quirk here? > >>>>>>> > >>>>>>> Hmm, I think WBS is the feature of the chipset and not the transport. > >>>>>>> Therefore isn't it better to just have it set in one place? > >>>>>>> Setting the quirks here means we need to copy paste the settings from btrtl.c. > >>>>>> > >>>>>> but since you are already setting HCI_QUIRK_SIMULTANEOUS_DISCOVERY right now, I don’t see the difference. > >>>>> > >>>>> Sorry, I don't get what you mean. > >>>>> With this patch I also moved HCI_QUIRK_SIMULTANEOUS_DISCOVERY into > >>>>> btrtl.c, so it's together with the WBS quirk. > >>>>> > >>>>>> Can we actually verify that we still need the WBS quirk. I think we fixed the broken errerrnous packet flag handling. > >>>>> > >>>>> To be honest, I am not aware about the story of the broken erroneous > >>>>> packet flag. > >>>>> Last time I checked I still needed the quirk to have RTL8822 on UART > >>>>> properly run WBS, but that was months ago... > >>>>> Let me verify whether this quirk is still needed. > >>>> > >>>> It looks like we still need the WBS quirk because otherwise the host > >>>> wouldn't know whether the controller supports WBS or not. It's used in > >>>> get_supported_settings() in mgmt.c. > >>> > >>> and why not set it unconditionally for all Realtek chips? > >> > >> Not all Realtek chips supports WBS, therefore > >> HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED is only set on some of them. > > > > Are there any other concerns you might have? > > can we do the quirk setting in btrtl_setup_realtek() instead of creating another exported function. It cannot be done easily since the first part of btrtl_setup_realtek() is used exclusively for btusb, which is done differently in hci_h5. We can have it another way: define btrtl_setup_realtek_h5() to do the setup for h5 part in btrtl.c. This would effectively move all of h5_btrtl_setup() inside hci_h5.c, most notably the serdev setup. In turn, we don't have to expose btrtl_set_quirks(), and we can even hide btrtl_initialize(), btrtl_free(), and btrtl_download_firmware() inside btrtl.c. I'm not sure though why would one want that? We still need to export the new btrtl_setup_realtek_h5(). BTW I just noticed I missed a declaration in btrtl.h, so I will send a v2 patch to fix it. Cheers, Archie ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities 2021-05-27 7:05 ` Archie Pusaka @ 2021-05-27 15:13 ` Marcel Holtmann 0 siblings, 0 replies; 14+ messages in thread From: Marcel Holtmann @ 2021-05-27 15:13 UTC (permalink / raw) To: Archie Pusaka Cc: linux-bluetooth, CrosBT Upstreaming, Archie Pusaka, Abhishek Pandit-Subedi, Johan Hedberg, Luiz Augusto von Dentz, LKML Hi Archie, >>>>>>>>>>> RTL8822 chipset supports WBS, and this information is conveyed in >>>>>>>>>>> btusb.c. However, the UART driver doesn't have this information just >>>>>>>>>>> yet. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Archie Pusaka <apusaka@chromium.org> >>>>>>>>>>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> >>>>>>>>>>> --- >>>>>>>>>>> >>>>>>>>>>> drivers/bluetooth/btrtl.c | 26 ++++++++++++++++---------- >>>>>>>>>>> drivers/bluetooth/btrtl.h | 2 ++ >>>>>>>>>>> drivers/bluetooth/hci_h5.c | 5 +---- >>>>>>>>>>> 3 files changed, 19 insertions(+), 14 deletions(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c >>>>>>>>>>> index e7fe5fb22753..988a09860c6b 100644 >>>>>>>>>>> --- a/drivers/bluetooth/btrtl.c >>>>>>>>>>> +++ b/drivers/bluetooth/btrtl.c >>>>>>>>>>> @@ -719,17 +719,8 @@ int btrtl_download_firmware(struct hci_dev *hdev, >>>>>>>>>>> } >>>>>>>>>>> EXPORT_SYMBOL_GPL(btrtl_download_firmware); >>>>>>>>>>> >>>>>>>>>>> -int btrtl_setup_realtek(struct hci_dev *hdev) >>>>>>>>>>> +void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev) >>>>>>>>>>> { >>>>>>>>>>> - struct btrtl_device_info *btrtl_dev; >>>>>>>>>>> - int ret; >>>>>>>>>>> - >>>>>>>>>>> - btrtl_dev = btrtl_initialize(hdev, NULL); >>>>>>>>>>> - if (IS_ERR(btrtl_dev)) >>>>>>>>>>> - return PTR_ERR(btrtl_dev); >>>>>>>>>>> - >>>>>>>>>>> - ret = btrtl_download_firmware(hdev, btrtl_dev); >>>>>>>>>>> - >>>>>>>>>>> /* Enable controller to do both LE scan and BR/EDR inquiry >>>>>>>>>>> * simultaneously. >>>>>>>>>>> */ >>>>>>>>>>> @@ -750,6 +741,21 @@ int btrtl_setup_realtek(struct hci_dev *hdev) >>>>>>>>>>> rtl_dev_dbg(hdev, "WBS supported not enabled."); >>>>>>>>>>> break; >>>>>>>>>>> } >>>>>>>>>>> +} >>>>>>>>>>> +EXPORT_SYMBOL_GPL(btrtl_set_quirks); >>>>>>>>>>> + >>>>>>>>>>> +int btrtl_setup_realtek(struct hci_dev *hdev) >>>>>>>>>>> +{ >>>>>>>>>>> + struct btrtl_device_info *btrtl_dev; >>>>>>>>>>> + int ret; >>>>>>>>>>> + >>>>>>>>>>> + btrtl_dev = btrtl_initialize(hdev, NULL); >>>>>>>>>>> + if (IS_ERR(btrtl_dev)) >>>>>>>>>>> + return PTR_ERR(btrtl_dev); >>>>>>>>>>> + >>>>>>>>>>> + ret = btrtl_download_firmware(hdev, btrtl_dev); >>>>>>>>>>> + >>>>>>>>>>> + btrtl_set_quirks(hdev, btrtl_dev); >>>>>>>>>>> >>>>>>>>>>> btrtl_free(btrtl_dev); >>>>>>>>>>> return ret; >>>>>>>>>>> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h >>>>>>>>>>> index 2a582682136d..260167f01b08 100644 >>>>>>>>>>> --- a/drivers/bluetooth/btrtl.h >>>>>>>>>>> +++ b/drivers/bluetooth/btrtl.h >>>>>>>>>>> @@ -54,6 +54,8 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev, >>>>>>>>>>> void btrtl_free(struct btrtl_device_info *btrtl_dev); >>>>>>>>>>> int btrtl_download_firmware(struct hci_dev *hdev, >>>>>>>>>>> struct btrtl_device_info *btrtl_dev); >>>>>>>>>>> +void btrtl_set_quirks(struct hci_dev *hdev, >>>>>>>>>>> + struct btrtl_device_info *btrtl_dev); >>>>>>>>>>> int btrtl_setup_realtek(struct hci_dev *hdev); >>>>>>>>>>> int btrtl_shutdown_realtek(struct hci_dev *hdev); >>>>>>>>>>> int btrtl_get_uart_settings(struct hci_dev *hdev, >>>>>>>>>>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c >>>>>>>>>>> index 27e96681d583..e0520639f4ba 100644 >>>>>>>>>>> --- a/drivers/bluetooth/hci_h5.c >>>>>>>>>>> +++ b/drivers/bluetooth/hci_h5.c >>>>>>>>>>> @@ -906,10 +906,7 @@ static int h5_btrtl_setup(struct h5 *h5) >>>>>>>>>>> /* Give the device some time before the hci-core sends it a reset */ >>>>>>>>>>> usleep_range(10000, 20000); >>>>>>>>>>> >>>>>>>>>>> - /* Enable controller to do both LE scan and BR/EDR inquiry >>>>>>>>>>> - * simultaneously. >>>>>>>>>>> - */ >>>>>>>>>>> - set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks); >>>>>>>>>>> + btrtl_set_quirks(h5->hu->hdev, btrtl_dev); >>>>>>>>>> >>>>>>>>>> any reason why not just setting WBS quirk here? >>>>>>>>> >>>>>>>>> Hmm, I think WBS is the feature of the chipset and not the transport. >>>>>>>>> Therefore isn't it better to just have it set in one place? >>>>>>>>> Setting the quirks here means we need to copy paste the settings from btrtl.c. >>>>>>>> >>>>>>>> but since you are already setting HCI_QUIRK_SIMULTANEOUS_DISCOVERY right now, I don’t see the difference. >>>>>>> >>>>>>> Sorry, I don't get what you mean. >>>>>>> With this patch I also moved HCI_QUIRK_SIMULTANEOUS_DISCOVERY into >>>>>>> btrtl.c, so it's together with the WBS quirk. >>>>>>> >>>>>>>> Can we actually verify that we still need the WBS quirk. I think we fixed the broken errerrnous packet flag handling. >>>>>>> >>>>>>> To be honest, I am not aware about the story of the broken erroneous >>>>>>> packet flag. >>>>>>> Last time I checked I still needed the quirk to have RTL8822 on UART >>>>>>> properly run WBS, but that was months ago... >>>>>>> Let me verify whether this quirk is still needed. >>>>>> >>>>>> It looks like we still need the WBS quirk because otherwise the host >>>>>> wouldn't know whether the controller supports WBS or not. It's used in >>>>>> get_supported_settings() in mgmt.c. >>>>> >>>>> and why not set it unconditionally for all Realtek chips? >>>> >>>> Not all Realtek chips supports WBS, therefore >>>> HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED is only set on some of them. >>> >>> Are there any other concerns you might have? >> >> can we do the quirk setting in btrtl_setup_realtek() instead of creating another exported function. > > It cannot be done easily since the first part of btrtl_setup_realtek() > is used exclusively for btusb, which is done differently in hci_h5. > > We can have it another way: define btrtl_setup_realtek_h5() to do the > setup for h5 part in btrtl.c. This would effectively move all of > h5_btrtl_setup() inside hci_h5.c, most notably the serdev setup. In > turn, we don't have to expose btrtl_set_quirks(), and we can even hide > btrtl_initialize(), btrtl_free(), and btrtl_download_firmware() inside > btrtl.c. > I'm not sure though why would one want that? We still need to export > the new btrtl_setup_realtek_h5(). I am a bit disappointed that nobody took up the work on bt3wire.c so that we can have a clean serdev based driver for 3-Wire / H:5 support. That would make supporting USB and UART vendor setups from the same manufacturer a lot easier. The consistent hci_h5.c hacking is not doing anybody any favor in the long run. It will get more and more complicated especially since the underlying core design is a line discipline. This is a hint with a massively large hammer. Regards Marcel ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-05-27 15:13 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-13 8:54 [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities Archie Pusaka 2021-05-13 10:12 ` bluez.test.bot 2021-05-13 15:14 ` [PATCH] " Marcel Holtmann 2021-05-13 16:32 ` Archie Pusaka 2021-05-13 19:03 ` Marcel Holtmann 2021-05-14 11:40 ` Archie Pusaka 2021-05-17 4:31 ` Archie Pusaka 2021-05-20 4:45 ` Archie Pusaka 2021-05-20 15:18 ` Marcel Holtmann 2021-05-20 16:02 ` Archie Pusaka 2021-05-26 5:49 ` Archie Pusaka 2021-05-26 14:59 ` Marcel Holtmann 2021-05-27 7:05 ` Archie Pusaka 2021-05-27 15:13 ` Marcel Holtmann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.