Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities

[Archie Pusaka]

Hi Marcel,

On Fri, 14 May 2021 at 19:40, Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
>
> Hi Marcel,
>
> On Fri, 14 May 2021 at 03:03, Marcel Holtmann <marcel@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > >>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>
> > >>> ---
> > >>>
> > >>> 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