Re: [RFC v2 7/9] bluetooth: btrtl: load the config blob from devicetree when available

[Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>]

Hi Carlo,

On Wed, Jan 3, 2018 at 12:06 AM, Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org> wrote:
> On Tue, Jan 2, 2018 at 9:46 PM, Martin Blumenstingl
> <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>> Hi Carlo,
>
> Hi Martin,
>
>> On Tue, Jan 2, 2018 at 12:31 PM, Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org> wrote:
>>> On Tue, Jan 2, 2018 at 11:19 AM, Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org> wrote:
>>>> Hi Carlo,
>>>>
>>>>>>> Some Realtek bluetooth devices need a "config" blob. The btrtl driver
>>>>>>> currently only allows loading this config blob via the request_firmware
>>>>>>> mechanism.
>>>>>>>
>>>>>>> The UART Bluetooth chips use this config blob to specify the baudrate,
>>>>>>> whether flow control is used and some other unknown bits. This means
>>>>>>> that the config blob is board-specific - thus loading it via
>>>>>>> request_firmware means that the rootfs is tied to a specific board.
>>>>>>>
>>>>>>> The UART Bluetooth chips are implemented through serdev. This means
>>>>>>> there is also a devicetree node which describes the Bluetooth chip.
>>>>>>> Thus we can also load the blob from the devicetree node to keep the
>>>>>>> filesystem independent of any board configuration data. In the future
>>>>>>> this could be extended to support ACPI as well (in case that's needed).
>>>>>>>
>>>>>>> Parse the devicetree node if it exists and obtain the config blob from
>>>>>>> there. Otherwise fall back to using the "old" request_firmware
>>>>>>> mechanism.
>>>>>>
>>>>>> where are these config blobs coming from? I think we also need to give people a helping hand on how to add them to DT. I still wonder if the only pieces we are using are the UART config, then maybe skipping the config blob and allowing for clear named values in DT might be better.
>>>>>
>>>>> What about x86 platforms where we do not have DT (I didn't check but I
>>>>> don't think that the UART config in that case is shipped in the ACPI
>>>>> tables)?
>>>>
>>>> if we have this hardware in x86 systems, then I would really like to see ACPI table dumps. Some pieces might need hardcoding based on ACPI ID.
>>>
>>> Yes, we have, especially on cherry-trail SoCs. In [0] the DSDT of a
>>> cherry-trail laptop shipping the rtl8723bs (device OBDA8723).
>>>
>>> [0] https://gist.github.com/carlocaione/82bff95ababb67dd33f52a86e94ce3ff
>> so this shows that the UART settings (initial baudrate, HW flow
>> control, etc.) are part of the DSDT
>> however, the actual config blob is not
>>
>> the description of this patch explains: "Parse the devicetree node ...
>> [or] ... fall back to using the "old" request_firmware mechanism."
>> do you have any other solution in mind?
>
> As Marcel suggested we can assume that the information in the DSDT is
> correct so that we can get rid of the config blob also for x86
> platforms (assuming that the only useful information in the config
> blobs is the UART configuration).
in my tests I tried to send only the firmware without the config to my
RTL8723BS. unfortunately the last firmware chunk (sent to the
controller) times out in that case (even if I set a proper baudrate
before or if I specify no baudrate at all and keep the serdev at
115200)
have you tested this (= uploading the firmware without the config
blob) on your x86 board before?

so far the following solutions for the config blob were discussed:
1) put the config blob in userspace (/lib/firmware/...) -> not good
because it makes the rootfs board-specific
2) auto-generate the config blob -> didn't work for me, last firmware
chunk times out (just as if I had no config at all)
3) putting the config blob in DT -> possible but not very nice to read

I had another idea:
what if we mix solution 1) and 2)?
the idea: load the config blob from userspace (/lib/firmware/...) and
update it's settings with the values we got from devicetree-properties
/ DSDT
I have not tested this yet, but I just want to hear everyone's (at
least Marcel, Rob and Carlo) opinion on that
(this would also allow us to fully auto-generate the config blob in
the future once we figure out how to do that)

> Adding the ACPI support on top of your patches is (hopefully) trivial,
> just follow what was done for hci_bcm.c, basically adding a new _HID
> and reading the configuration for GPIOs and UART, all the rest should
> be transparent for serdev.
thanks for the reference to hci_bcm.c - I will have a look at this for
the next version
one question: "_HID" would be OBDA8723 in our case?

> I'll test your patches on the hardware I have.
great, thank you!


Regards
Martin