[RFC v1 5/8] Bluetooth: btrtl: add support for the RTL8723BS and RTL8723DS chips

[Martin Blumenstingl]

Hi Marcel,

On Sun, Nov 19, 2017 at 10:17 PM, Marcel Holtmann <marcel at holtmann.org> wrote:
> Hi Martin,
>
>>>> The Realtek RTL8723BS and RTL8723DS chipsets are SDIO wifi chips. They
>>>> also contain a Bluetooth module which is connected via UART to the host.
>>>>
>>>> Realtek's userspace initialization tool (rtk_hciattach) differentiates
>>>> these two via the HCI version and revision returned by the
>>>> HCI_OP_READ_LOCAL_VERSION command.
>>>> Additionally we apply these checks only the for UART devices. Everything
>>>> else is assumed to be a "RTL8723B" which was originally supported by the
>>>> driver (communicating via USB).
>>>>
>>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl at googlemail.com>
>>>> ---
>>>> drivers/bluetooth/btrtl.c | 32 ++++++++++++++++++++++++++++++--
>>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
>>>> index 45b872f5ad22..d896f9421250 100644
>>>> --- a/drivers/bluetooth/btrtl.c
>>>> +++ b/drivers/bluetooth/btrtl.c
>>>> @@ -418,9 +418,33 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
>>>>              has_rom_version = false;
>>>>              break;
>>>>      case RTL_ROM_LMP_8723B:
>>>> -             fw_name = "rtl_bt/rtl8723b_fw.bin";
>>>> -             cfg_name = "rtl_bt/rtl8723b_config.bin";
>>>> +             /* all variants support reading the ROM version */
>>>>              has_rom_version = true;
>>>> +
>>>> +             /*
>>>> +              * RTL8723 devices exist in different variants:
>>>> +              * - RTL8723BS (SDIO chip with UART Bluetooth)
>>>> +              * - RTL8723DS (SDIO chip with UART Bluetooth)
>>>> +              * - for backwards-compatibility everything else is assumed to
>>>> +              *   be an RTL8723B communicating over USB
>>>> +              *
>>>> +              * Only UART devices really need the config because that
>>>> +              * contains the UART speed / flow control settings.
>>>> +              */
>>>> +             if (hdev->bus == HCI_UART && resp->hci_ver == 6 &&
>>>> +                 le16_to_cpu(resp->hci_rev) == 0xb) {
>>>> +                     fw_name = "rtl_bt/rtl8723bs_fw.bin";
>>>> +                     cfg_name = "rtl_bt/rtl8723bs_config.bin";
>>>> +                     cfg_needed = true;
>>>> +             } else if (hdev->bus == HCI_UART && resp->hci_ver == 8 &&
>>>> +                        le16_to_cpu(resp->hci_rev) == 0xd) {
>>>> +                     fw_name = "rtl_bt/rtl8723ds_fw.bin";
>>>> +                     cfg_name = "rtl_bt/rtl872ds_config.bin";
>>>> +                     cfg_needed = true;
>>>> +             } else {
>>>> +                     fw_name = "rtl_bt/rtl8723b_fw.bin";
>>>> +                     cfg_name = "rtl_bt/rtl8723b_config.bin";
>>>> +             }
>>>
>>> so the main question is why is this a config file in the first place? So far all other drivers have expressed UART settings via DT (or even via ACPI). If we are just getting the baudrates, then better have this in DT. Since otherwise we end up with board specific filesystems again where different config files need to be installed. That is really not useful in the end.
>> this is an excellent question (and also the reason why it's an "RFC" patch)!
>> I'll try to figure out whether:
>> a) we can skip the config file completely
skipping the config blob causes it to time out at the end of the
firmware download (where the config seems to be applied internally)

>> b) we can generate the config file in-memory
I tried this as well with a very minimal version (where only the
"known" bits are used)
unfortunately this shows the same symptoms as skipping the config blob
I'll try experimenting with this to see if I can get it to work somehow

>> c) we have to stick with this config file
I'm afraid this would be our last option if I can't get b) working
a datasheet for these chips or some advice from someone who has access
to the datasheet/knowledge about the internals would be of great help

>>
>>> What is actually in these config files? Can we have some parsing and friendly display tool in bluez.git like we have for other manufactures.
>> to my knowledge this config file contains:
>> - the baudrate
>> - whether flow control is enabled or disabled
>
> these two are better done as part of the DT.
agreed, I'd also rather have this in DT than some blob

>> - (in some cases) the local-bd-address
>
> There is essentially DT support for that as well, but we also have Set Public Address mgmt command to handle this. Then again, first of all the hdev->set_bdaddr callback needs to be supported for that.
>
>> do you mean a tool that is similar to bluez.git tools/nokfw.c?
>
> Yes.
>
>> [0] https://github.com/NextThingCo/rtl8723ds_bt/blob/f56ef37217665070e574253d23a595ee1ca5ca23/rtk_hciattach/hciattach_rtk.c#L1781
>
> Please never ever make me look at code that defined struct sk_buff in userspace ;)
sorry for not putting a huge warning/disclaimer in front of that link!
as bad as it sounds: this is all I have as reference :(


Regards
Martin