diff mbox

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

Message ID 20180101204217.26165-8-martin.blumenstingl@googlemail.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Martin Blumenstingl Jan. 1, 2018, 8:42 p.m. UTC
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.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/bluetooth/btrtl.c | 43 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

Comments

Marcel Holtmann Jan. 2, 2018, 11:11 a.m. UTC | #1
Hi Martin,

> 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.

I might have asked before, but can we get a userspace similar to nokfw included in bluez.git that can parse and maybe even create/modify these blobs.

Regards

Marcel
Carlo Caione Jan. 2, 2018, 11:15 a.m. UTC | #2
On Tue, Jan 2, 2018 at 11:11 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Martin,
>
>> 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)?

Cheers,
Marcel Holtmann Jan. 2, 2018, 11:19 a.m. UTC | #3
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.

Regards

Marcel
Carlo Caione Jan. 2, 2018, 11:31 a.m. UTC | #4
On Tue, Jan 2, 2018 at 11:19 AM, Marcel Holtmann <marcel@holtmann.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

Cheers,
Marcel Holtmann Jan. 2, 2018, 11:38 a.m. UTC | #5
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 the BTHx entries normally come at least with the UART configuration. It would be useful check if it is actually correct. And then I think similar handling like what is done in hci_bcm.c and hci_intel.c needs to happen here.

I think that we extended serdev to ACPI as well. Don’t recall of the top of my head if these patches were merged or not. But if they are then it is as simple as a serdev DT based driver. Just add the appropriate _HID and got from there.

However now I think that moving towards making hci_h5.c more like generic abstraction like hci_h4.c and having a hci_rtl.c be specific for Realtek chips seems a bit cleaner direction. Frankly only H:4 and H:5 plain protocols should be used by btattach. And all others should go via serdev.

Regards

Marcel
Martin Blumenstingl Jan. 2, 2018, 9:43 p.m. UTC | #6
Hi Marcel, Hi Carlo,

On Tue, Jan 2, 2018 at 12:38 PM, Marcel Holtmann <marcel@holtmann.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 the BTHx entries normally come at least with the UART configuration. It would be useful check if it is actually correct. And then I think similar handling like what is done in hci_bcm.c and hci_intel.c needs to happen here.
what I can see is (which also matches the settings I use on my Amlogic
based boards):
- initial speed = 115200
- 8 data bits
- 1 stop bit
- even parity
- HW flow control enabled

> I think that we extended serdev to ACPI as well. Don’t recall of the top of my head if these patches were merged or not. But if they are then it is as simple as a serdev DT based driver. Just add the appropriate _HID and got from there.
yes, serdev recently gained ACPI support iirc
however, can we agree on implementing this step-by-step:
- I do not have any hardware that uses a RTL8723BS or RTL8723DS and
ACPI (I have these chips in Amlogic SoC based "TV boxes" where DT is
the leading firmware, so I cannot test the ACPI part)
- my goal is to get rid of the rtk_hciattach userspace tool dependency
(mostly Allwinner and Amlogic based devices will benefit from this)
- it would be great if I could get feedback what to consider or to
avoid when implementing this so ACPI support can be added easily on
top of my patches

> However now I think that moving towards making hci_h5.c more like generic abstraction like hci_h4.c and having a hci_rtl.c be specific for Realtek chips seems a bit cleaner direction. Frankly only H:4 and H:5 plain protocols should be used by btattach. And all others should go via serdev.
I agree with you here (as already discussed in the other patch)


Regards
Martin
Martin Blumenstingl Jan. 2, 2018, 9:46 p.m. UTC | #7
Hi Carlo,

On Tue, Jan 2, 2018 at 12:31 PM, Carlo Caione <carlo@endlessm.com> wrote:
> On Tue, Jan 2, 2018 at 11:19 AM, Marcel Holtmann <marcel@holtmann.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?


Regards
Martin
Carlo Caione Jan. 2, 2018, 11:06 p.m. UTC | #8
On Tue, Jan 2, 2018 at 9:46 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> Hi Carlo,

Hi Martin,

> On Tue, Jan 2, 2018 at 12:31 PM, Carlo Caione <carlo@endlessm.com> wrote:
>> On Tue, Jan 2, 2018 at 11:19 AM, Marcel Holtmann <marcel@holtmann.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).

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.

I'll test your patches on the hardware I have.

Cheers,
Martin Blumenstingl Jan. 3, 2018, 8:50 p.m. UTC | #9
Hi Carlo,

On Wed, Jan 3, 2018 at 12:06 AM, Carlo Caione <carlo@endlessm.com> wrote:
> On Tue, Jan 2, 2018 at 9:46 PM, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>> Hi Carlo,
>
> Hi Martin,
>
>> On Tue, Jan 2, 2018 at 12:31 PM, Carlo Caione <carlo@endlessm.com> wrote:
>>> On Tue, Jan 2, 2018 at 11:19 AM, Marcel Holtmann <marcel@holtmann.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
Carlo Caione Jan. 4, 2018, 9:46 a.m. UTC | #10
On Wed, Jan 3, 2018 at 8:50 PM, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
> Hi Carlo,

Hi Martin,

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

What's in the config blobs besides UART configuration?

It's odd because reading into hciattach_rtk.c it seems that the config
file is actually only used by the userspace tools (hciattach) to
retrieve the UART configuration and nothing else, whereas in the
kernel driver the config blob is appended to the firmware.

> have you tested this (= uploading the firmware without the config
> blob) on your x86 board before?

No, on the cherry-trail I have I'm using hciattach to upload the
firmware and AFAICT only the firmware is uploaded and (as written
before) the config blob is only used to get the UART configuration.

> 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

If you are shipping already the config blob in userspace I don't see
why you would do the update since you have already all the info you
need.
I would rather check why (2) didn't work fine. Have you any
documentation how the config blobs are generated? The code in
hciattach_rtk.c seems a good starting point.

> 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?

Yes

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

Cheers!
Marcel Holtmann Jan. 5, 2018, 2:57 p.m. UTC | #11
Hi Carlo,

>>> 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)
> 
> What's in the config blobs besides UART configuration?

is anybody writing a rtlfw.c tool (like nokfw.c) so that we can print out what we actually have in these config files?

> It's odd because reading into hciattach_rtk.c it seems that the config
> file is actually only used by the userspace tools (hciattach) to
> retrieve the UART configuration and nothing else, whereas in the
> kernel driver the config blob is appended to the firmware.

Frankly, I am inclined to not use the config file even for DT based system and just allow specifying the UART settings via normal DT properties like we do for Broadcom and others.

>> have you tested this (= uploading the firmware without the config
>> blob) on your x86 board before?
> 
> No, on the cherry-trail I have I'm using hciattach to upload the
> firmware and AFAICT only the firmware is uploaded and (as written
> before) the config blob is only used to get the UART configuration.
> 
>> 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
> 
> If you are shipping already the config blob in userspace I don't see
> why you would do the update since you have already all the info you
> need.
> I would rather check why (2) didn't work fine. Have you any
> documentation how the config blobs are generated? The code in
> hciattach_rtk.c seems a good starting point.

I would almost bet it needs a HCI commands or some HCI command is embedded in the config blob somewhere that essentially tells the firmware it is done and ready. Hence my ask above, we need to know what is in these config file.

Even settings like crystal timing or some RF pieces or even enabled/disabled features are board specific and should be in DT rather than a config blob on the general purpose filesystem.

Regards

Marcel
Marcel Holtmann Jan. 5, 2018, 4:15 p.m. UTC | #12
Hi Carlo,

>>>> 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)
>> 
>> What's in the config blobs besides UART configuration?
> 
> is anybody writing a rtlfw.c tool (like nokfw.c) so that we can print out what we actually have in these config files?
> 
>> It's odd because reading into hciattach_rtk.c it seems that the config
>> file is actually only used by the userspace tools (hciattach) to
>> retrieve the UART configuration and nothing else, whereas in the
>> kernel driver the config blob is appended to the firmware.
> 
> Frankly, I am inclined to not use the config file even for DT based system and just allow specifying the UART settings via normal DT properties like we do for Broadcom and others.

so I googled for a few config files and this is what this turns into:

Analyzing rtl8723d_config_1000000_noflow.dms
Signature:   0x8723ab55
Data length: 41
len=1   offset=f4,{ 01 }
len=2   offset=f6,{ 81 00 }
len=2   offset=fa,{ 12 80 }
len=16  offset=0c,{ 04 50 00 00 50 c5 ea 19 e1 1b fd af 5b 01 a4 0b }
len=1   offset=d9,{ 0f }
len=1   offset=e4,{ 08 }
Analyzing rtl8723d_config.dms
Signature:   0x8723ab55
Data length: 41
len=1   offset=f4,{ 01 }
len=2   offset=f6,{ 81 00 }
len=2   offset=fa,{ 12 80 }
len=16  offset=0c,{ 02 80 92 04 50 c5 ea 19 e1 1b fd af 5f 01 a4 0b }
len=1   offset=d9,{ 0f }
len=1   offset=e4,{ 08 }
Analyzing rtl8822b_config.bin
Signature:   0x8723ab55
Data length: 8
len=1   offset=d9,{ 0f }
len=1   offset=e4,{ 08 }

The first two are some UART based ones and the last one is USB based.

So the 0x3c offset seems to be the BD_ADDR and 0x0c offset is the UART configuration. It would be good to know which settings the other ones control.

Also the 16 octet UART config blob seems to be decoded like this:

	uart_config {
		le32 baudrate;
		u8[8] reserved1;
		u8 flowctl;
		u8[3] reserved2;
	}

Actually hciattach_rtk just takes the baud rate and and hardware flow control bit out of this file. That is clearly two things that are better written in plain text in the DT file.

Regards

Marcel
Marcel Holtmann Jan. 5, 2018, 8:44 p.m. UTC | #13
Hi Carlo,

>>>>> 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)
>>> 
>>> What's in the config blobs besides UART configuration?
>> 
>> is anybody writing a rtlfw.c tool (like nokfw.c) so that we can print out what we actually have in these config files?
>> 
>>> It's odd because reading into hciattach_rtk.c it seems that the config
>>> file is actually only used by the userspace tools (hciattach) to
>>> retrieve the UART configuration and nothing else, whereas in the
>>> kernel driver the config blob is appended to the firmware.
>> 
>> Frankly, I am inclined to not use the config file even for DT based system and just allow specifying the UART settings via normal DT properties like we do for Broadcom and others.
> 
> so I googled for a few config files and this is what this turns into:
> 
> Analyzing rtl8723d_config_1000000_noflow.dms
> Signature:   0x8723ab55
> Data length: 41
> len=1   offset=f4,{ 01 }
> len=2   offset=f6,{ 81 00 }
> len=2   offset=fa,{ 12 80 }
> len=16  offset=0c,{ 04 50 00 00 50 c5 ea 19 e1 1b fd af 5b 01 a4 0b }
> len=1   offset=d9,{ 0f }
> len=1   offset=e4,{ 08 }
> Analyzing rtl8723d_config.dms
> Signature:   0x8723ab55
> Data length: 41
> len=1   offset=f4,{ 01 }
> len=2   offset=f6,{ 81 00 }
> len=2   offset=fa,{ 12 80 }
> len=16  offset=0c,{ 02 80 92 04 50 c5 ea 19 e1 1b fd af 5f 01 a4 0b }
> len=1   offset=d9,{ 0f }
> len=1   offset=e4,{ 08 }
> Analyzing rtl8822b_config.bin
> Signature:   0x8723ab55
> Data length: 8
> len=1   offset=d9,{ 0f }
> len=1   offset=e4,{ 08 }
> 
> The first two are some UART based ones and the last one is USB based.
> 
> So the 0x3c offset seems to be the BD_ADDR and 0x0c offset is the UART configuration. It would be good to know which settings the other ones control.
> 
> Also the 16 octet UART config blob seems to be decoded like this:
> 
> 	uart_config {
> 		le32 baudrate;
> 		u8[8] reserved1;
> 		u8 flowctl;
> 		u8[3] reserved2;
> 	}
> 
> Actually hciattach_rtk just takes the baud rate and and hardware flow control bit out of this file. That is clearly two things that are better written in plain text in the DT file.

so this is actually some funny stuff if you start to understand it.

Analyzing rtl8723b_config.dms
Signature: 0x8723ab55
Data len:  38
len=8   offset=00f4,{ 01 00 00 00 05 50 00 00 }
len=16  offset=000c,{ 02 80 92 04 50 c5 ea 19 e1 1b f1 af 5f 01 a4 0b },UART_CONFIG
len=1   offset=0027,{ 63 }
len=1   offset=00fe,{ 01 }
Analyzing rtl8723d_config_1000000_noflow.dms
Signature: 0x8723ab55
Data len:  41
len=1   offset=00f4,{ 01 }
len=2   offset=00f6,{ 81 00 }
len=2   offset=00fa,{ 12 80 }
len=16  offset=000c,{ 04 50 00 00 50 c5 ea 19 e1 1b fd af 5b 01 a4 0b },UART_CONFIG
len=1   offset=00d9,{ 0f }
len=1   offset=00e4,{ 08 }

Seems like Realtek really defines memory offsets in this file and they can be defined in various different ways.

So 00f4,{ 01 00 00 00 05 50 00 00 } defines the whole PCM settings for interface 1 and 2. And 00f4,{ 01 } + 00f6,{ 81 00 } + 00fa,{ 12 80 } is the same PCM settings, but with only pieces of it defined.

This also means a 000c,{ 04 50 00 00 50 } for just setting the baud rate is as valid if flow control defaults to off and all other values are actual defaults. So code inside hciattach_rtk.c is also kind faulty on how it handles the flow control bit. It works if the config files all have offset 000c in it, but if not, then they are going funky.

Since these are efuse settings, I really wonder if there is just a HCI vendor command to read out the defaults and we use that to compare. And what I would really like to know is what these settings are suppose to change. Since even for USB, we are actually not even applying them.

Anyway, I am certain that for Realtek UART devices, we just want to specify max-speed DT property like we do with the others. And then maybe a flow-control DT property to control that one (following what nfcmrvl.txt does). We can use the rtlfw tool that I wrote to extract the values from Realtek provided config files. Frankly the PCM settings we have to deal with as well at some point. But that is also missing for Broadcom and others.

Also if there is no config file, we should be able to just assume no flow control, 115200 baud rate and H:5 as protocol. That means that almost all chips will just work. They are just slow since we do not deal with the max-speed setting.

At the end of the day, this is the same as for Broadcom and ACPI vs DT. This should be no different. The only bit nasty part is that this is H:5 as protocol and we have not really abstracted that one nicely to just have a tiny hci_rtl.c for vendor specific pieces.

Regards

Marcel
Martin Blumenstingl Jan. 7, 2018, 8:07 p.m. UTC | #14
Hi Marcel,

thank you for digging into this!

On Fri, Jan 5, 2018 at 9:44 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Carlo,
>
>>>>>> 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)
>>>>
>>>> What's in the config blobs besides UART configuration?
>>>
>>> is anybody writing a rtlfw.c tool (like nokfw.c) so that we can print out what we actually have in these config files?
>>>
>>>> It's odd because reading into hciattach_rtk.c it seems that the config
>>>> file is actually only used by the userspace tools (hciattach) to
>>>> retrieve the UART configuration and nothing else, whereas in the
>>>> kernel driver the config blob is appended to the firmware.
>>>
>>> Frankly, I am inclined to not use the config file even for DT based system and just allow specifying the UART settings via normal DT properties like we do for Broadcom and others.
>>
>> so I googled for a few config files and this is what this turns into:
>>
>> Analyzing rtl8723d_config_1000000_noflow.dms
>> Signature:   0x8723ab55
>> Data length: 41
>> len=1   offset=f4,{ 01 }
>> len=2   offset=f6,{ 81 00 }
>> len=2   offset=fa,{ 12 80 }
>> len=16  offset=0c,{ 04 50 00 00 50 c5 ea 19 e1 1b fd af 5b 01 a4 0b }
>> len=1   offset=d9,{ 0f }
>> len=1   offset=e4,{ 08 }
>> Analyzing rtl8723d_config.dms
>> Signature:   0x8723ab55
>> Data length: 41
>> len=1   offset=f4,{ 01 }
>> len=2   offset=f6,{ 81 00 }
>> len=2   offset=fa,{ 12 80 }
>> len=16  offset=0c,{ 02 80 92 04 50 c5 ea 19 e1 1b fd af 5f 01 a4 0b }
>> len=1   offset=d9,{ 0f }
>> len=1   offset=e4,{ 08 }
>> Analyzing rtl8822b_config.bin
>> Signature:   0x8723ab55
>> Data length: 8
>> len=1   offset=d9,{ 0f }
>> len=1   offset=e4,{ 08 }
>>
>> The first two are some UART based ones and the last one is USB based.
>>
>> So the 0x3c offset seems to be the BD_ADDR and 0x0c offset is the UART configuration. It would be good to know which settings the other ones control.
this matches my findings from the hciattach_rtk tool for rtl8723bs_bt
and rtl8723ds_bt

>> Also the 16 octet UART config blob seems to be decoded like this:
>>
>>       uart_config {
>>               le32 baudrate;
>>               u8[8] reserved1;
>>               u8 flowctl;
>>               u8[3] reserved2;
>>       }
>>
I could not find this in any of the rtl8723bs_bt or rtl8723ds_bt sources
so thank you for sharing this (even though this descriptin is missing
a few bytes)!

>> Actually hciattach_rtk just takes the baud rate and and hardware flow control bit out of this file. That is clearly two things that are better written in plain text in the DT file.
this matches my findings apart from that hciattach_rtk also appends
the config blob to the "firmware patch" that is being uploaded to the
device
if you are brave you can have a look at [0]

> so this is actually some funny stuff if you start to understand it.
>
> Analyzing rtl8723b_config.dms
> Signature: 0x8723ab55
> Data len:  38
> len=8   offset=00f4,{ 01 00 00 00 05 50 00 00 }
> len=16  offset=000c,{ 02 80 92 04 50 c5 ea 19 e1 1b f1 af 5f 01 a4 0b },UART_CONFIG
> len=1   offset=0027,{ 63 }
> len=1   offset=00fe,{ 01 }
> Analyzing rtl8723d_config_1000000_noflow.dms
> Signature: 0x8723ab55
> Data len:  41
> len=1   offset=00f4,{ 01 }
> len=2   offset=00f6,{ 81 00 }
> len=2   offset=00fa,{ 12 80 }
> len=16  offset=000c,{ 04 50 00 00 50 c5 ea 19 e1 1b fd af 5b 01 a4 0b },UART_CONFIG
> len=1   offset=00d9,{ 0f }
> len=1   offset=00e4,{ 08 }
>
> Seems like Realtek really defines memory offsets in this file and they can be defined in various different ways.
wow, that is interesting - I was wondering why they called it "offset"
instead of "config_id" (or something similar)

> So 00f4,{ 01 00 00 00 05 50 00 00 } defines the whole PCM settings for interface 1 and 2. And 00f4,{ 01 } + 00f6,{ 81 00 } + 00fa,{ 12 80 } is the same PCM settings, but with only pieces of it defined.
>
> This also means a 000c,{ 04 50 00 00 50 } for just setting the baud rate is as valid if flow control defaults to off and all other values are actual defaults. So code inside hciattach_rtk.c is also kind faulty on how it handles the flow control bit. It works if the config files all have offset 000c in it, but if not, then they are going funky.
>
> Since these are efuse settings, I really wonder if there is just a HCI vendor command to read out the defaults and we use that to compare. And what I would really like to know is what these settings are suppose to change. Since even for USB, we are actually not even applying them.
the idea with the vendor command to read out existing memory is interesting

based on the code in "btrtl.c" it seems that we are applying the
settings from the config blob.
we are simply appending it at the end of the "firmware patch", see [1]

> Anyway, I am certain that for Realtek UART devices, we just want to specify max-speed DT property like we do with the others. And then maybe a flow-control DT property to control that one (following what nfcmrvl.txt does). We can use the rtlfw tool that I wrote to extract the values from Realtek provided config files. Frankly the PCM settings we have to deal with as well at some point. But that is also missing for Broadcom and others.
just to make sure I understand you correctly: would you generate the
config blob in-memory (in a function in btrtl.c) and hardcode all
unknown bits (the reserved bits in the UART config entry for example)
or would you mandate that a config blob is present (so
request_firmware can fetch it) for "high-speed" operation?

for PCM: I cannot test that anyways since the Amlogic platform does
not have audio support yet

> Also if there is no config file, we should be able to just assume no flow control, 115200 baud rate and H:5 as protocol. That means that almost all chips will just work. They are just slow since we do not deal with the max-speed setting.
my problem so far was that uploading the firmware patch without
appending a config blob (see [1]) broke the UART communication (it was
not 115200 baud as before, and I also tried other speeds such as
1500000 and 1000000 baud - neither worked)
however, extracting the existing values from the efuse using a vendor
command might give a hint why (maybe they are fallling back to some
exotic baudrate in that case, etc...)

> At the end of the day, this is the same as for Broadcom and ACPI vs DT. This should be no different. The only bit nasty part is that this is H:5 as protocol and we have not really abstracted that one nicely to just have a tiny hci_rtl.c for vendor specific pieces.
yes, you already suggested that I should start making that re-usable
(this is still on my TODO-list)


Regards
Martin


[0] https://github.com/NextThingCo/rtl8723ds_bt/blob/master/rtk_hciattach/hciattach_rtk.c#L2742
[1] https://elixir.free-electrons.com/linux/v4.14.11/source/drivers/bluetooth/btrtl.c#L377
Marcel Holtmann Jan. 9, 2018, 3:26 p.m. UTC | #15
Hi Martin,

>>>>>>> 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)
>>>>> 
>>>>> What's in the config blobs besides UART configuration?
>>>> 
>>>> is anybody writing a rtlfw.c tool (like nokfw.c) so that we can print out what we actually have in these config files?
>>>> 
>>>>> It's odd because reading into hciattach_rtk.c it seems that the config
>>>>> file is actually only used by the userspace tools (hciattach) to
>>>>> retrieve the UART configuration and nothing else, whereas in the
>>>>> kernel driver the config blob is appended to the firmware.
>>>> 
>>>> Frankly, I am inclined to not use the config file even for DT based system and just allow specifying the UART settings via normal DT properties like we do for Broadcom and others.
>>> 
>>> so I googled for a few config files and this is what this turns into:
>>> 
>>> Analyzing rtl8723d_config_1000000_noflow.dms
>>> Signature:   0x8723ab55
>>> Data length: 41
>>> len=1   offset=f4,{ 01 }
>>> len=2   offset=f6,{ 81 00 }
>>> len=2   offset=fa,{ 12 80 }
>>> len=16  offset=0c,{ 04 50 00 00 50 c5 ea 19 e1 1b fd af 5b 01 a4 0b }
>>> len=1   offset=d9,{ 0f }
>>> len=1   offset=e4,{ 08 }
>>> Analyzing rtl8723d_config.dms
>>> Signature:   0x8723ab55
>>> Data length: 41
>>> len=1   offset=f4,{ 01 }
>>> len=2   offset=f6,{ 81 00 }
>>> len=2   offset=fa,{ 12 80 }
>>> len=16  offset=0c,{ 02 80 92 04 50 c5 ea 19 e1 1b fd af 5f 01 a4 0b }
>>> len=1   offset=d9,{ 0f }
>>> len=1   offset=e4,{ 08 }
>>> Analyzing rtl8822b_config.bin
>>> Signature:   0x8723ab55
>>> Data length: 8
>>> len=1   offset=d9,{ 0f }
>>> len=1   offset=e4,{ 08 }
>>> 
>>> The first two are some UART based ones and the last one is USB based.
>>> 
>>> So the 0x3c offset seems to be the BD_ADDR and 0x0c offset is the UART configuration. It would be good to know which settings the other ones control.
> this matches my findings from the hciattach_rtk tool for rtl8723bs_bt
> and rtl8723ds_bt
> 
>>> Also the 16 octet UART config blob seems to be decoded like this:
>>> 
>>>      uart_config {
>>>              le32 baudrate;
>>>              u8[8] reserved1;
>>>              u8 flowctl;
>>>              u8[3] reserved2;
>>>      }
>>> 
> I could not find this in any of the rtl8723bs_bt or rtl8723ds_bt sources
> so thank you for sharing this (even though this descriptin is missing
> a few bytes)!
> 
>>> Actually hciattach_rtk just takes the baud rate and and hardware flow control bit out of this file. That is clearly two things that are better written in plain text in the DT file.
> this matches my findings apart from that hciattach_rtk also appends
> the config blob to the "firmware patch" that is being uploaded to the
> device
> if you are brave you can have a look at [0]

so this makes kinda sense. If you do not load the config file, you end up with the ROM defaults for all their “efuse” settings. Since it is essentially just a memory area where the config file overwrites defaults, this is all fine.

They have to extract the UART settings out of it since they also need to handled special during the firmware loading / boot process. This is also means if there is no config file or not UART settings in the config file, the process should use the default values.

>> so this is actually some funny stuff if you start to understand it.
>> 
>> Analyzing rtl8723b_config.dms
>> Signature: 0x8723ab55
>> Data len:  38
>> len=8   offset=00f4,{ 01 00 00 00 05 50 00 00 }
>> len=16  offset=000c,{ 02 80 92 04 50 c5 ea 19 e1 1b f1 af 5f 01 a4 0b },UART_CONFIG
>> len=1   offset=0027,{ 63 }
>> len=1   offset=00fe,{ 01 }
>> Analyzing rtl8723d_config_1000000_noflow.dms
>> Signature: 0x8723ab55
>> Data len:  41
>> len=1   offset=00f4,{ 01 }
>> len=2   offset=00f6,{ 81 00 }
>> len=2   offset=00fa,{ 12 80 }
>> len=16  offset=000c,{ 04 50 00 00 50 c5 ea 19 e1 1b fd af 5b 01 a4 0b },UART_CONFIG
>> len=1   offset=00d9,{ 0f }
>> len=1   offset=00e4,{ 08 }
>> 
>> Seems like Realtek really defines memory offsets in this file and they can be defined in various different ways.
> wow, that is interesting - I was wondering why they called it "offset"
> instead of "config_id" (or something similar)

It makes sense, they are just “patching” their configuration space.

>> So 00f4,{ 01 00 00 00 05 50 00 00 } defines the whole PCM settings for interface 1 and 2. And 00f4,{ 01 } + 00f6,{ 81 00 } + 00fa,{ 12 80 } is the same PCM settings, but with only pieces of it defined.
>> 
>> This also means a 000c,{ 04 50 00 00 50 } for just setting the baud rate is as valid if flow control defaults to off and all other values are actual defaults. So code inside hciattach_rtk.c is also kind faulty on how it handles the flow control bit. It works if the config files all have offset 000c in it, but if not, then they are going funky.
>> 
>> Since these are efuse settings, I really wonder if there is just a HCI vendor command to read out the defaults and we use that to compare. And what I would really like to know is what these settings are suppose to change. Since even for USB, we are actually not even applying them.
> the idea with the vendor command to read out existing memory is interesting
> 
> based on the code in "btrtl.c" it seems that we are applying the
> settings from the config blob.
> we are simply appending it at the end of the "firmware patch", see [1]

I am pretty sure it is all the same for USB and UART.

>> Anyway, I am certain that for Realtek UART devices, we just want to specify max-speed DT property like we do with the others. And then maybe a flow-control DT property to control that one (following what nfcmrvl.txt does). We can use the rtlfw tool that I wrote to extract the values from Realtek provided config files. Frankly the PCM settings we have to deal with as well at some point. But that is also missing for Broadcom and others.
> just to make sure I understand you correctly: would you generate the
> config blob in-memory (in a function in btrtl.c) and hardcode all
> unknown bits (the reserved bits in the UART config entry for example)
> or would you mandate that a config blob is present (so
> request_firmware can fetch it) for "high-speed" operation?
> 
> for PCM: I cannot test that anyways since the Amlogic platform does
> not have audio support yet
> 
>> Also if there is no config file, we should be able to just assume no flow control, 115200 baud rate and H:5 as protocol. That means that almost all chips will just work. They are just slow since we do not deal with the max-speed setting.
> my problem so far was that uploading the firmware patch without
> appending a config blob (see [1]) broke the UART communication (it was
> not 115200 baud as before, and I also tried other speeds such as
> 1500000 and 1000000 baud - neither worked)
> however, extracting the existing values from the efuse using a vendor
> command might give a hint why (maybe they are fallling back to some
> exotic baudrate in that case, etc…)

Have you tried to just create a config file with just the signature 0x8723ab55 and data len set to 0. So essentially an empty config file. I am curious if such a default config is good enough to get things working. Or if they have actually faulty defaults in their ROM that need overwriting to make things fly.

Btw. I am curious why the UART speed is in that file anyway if we have to change it anyway via HCI command. So normally the boot UART speed is what is needed to know, not what final high speed we are using. So this is a bit odd.

I wonder if just loading the config file if present and then use a DT property max-speed to select the speed (even if it is different from the config file).

For me this means that we can have a general config file on disk, but actually allow overwriting the UART speed via DT. Otherwise we might just want to add extra offset entries for the config blob loading from disk and extend it with the DT values. So it would be some sort of overload.

Regards

Marcel
diff mbox

Patch

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 25e9743643f5..1494269cf4d7 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -18,6 +18,7 @@ 
 #include <linux/module.h>
 #include <linux/firmware.h>
 #include <asm/unaligned.h>
+#include <linux/of.h>
 #include <linux/usb.h>
 
 #include <net/bluetooth/bluetooth.h>
@@ -379,6 +380,41 @@  void btrtl_free(struct btrtl_device_info *btrtl_dev)
 }
 EXPORT_SYMBOL_GPL(btrtl_free);
 
+static int rtl_load_config_from_dt(struct hci_dev *hdev,
+				   struct btrtl_device_info *btrtl_dev)
+{
+	struct device_node *np = hdev->dev.parent->of_node;
+	int ret, config_len;
+
+	if (!of_device_is_available(np))
+		return -ENOENT;
+
+	if (!of_find_property(np, "realtek,config-data", NULL))
+		return -ENOENT;
+
+	config_len = of_property_count_u8_elems(np, "realtek,config-data");
+	if (config_len <= 0)
+		return -ENOENT;
+
+	btrtl_dev->cfg_data = kzalloc(config_len, GFP_KERNEL);
+	if (!btrtl_dev->cfg_data)
+		return -ENOMEM;
+
+	ret = of_property_read_u8_array(np, "realtek,config-data",
+					btrtl_dev->cfg_data, config_len);
+	if (ret) {
+		kfree(btrtl_dev->cfg_data);
+		return ret;
+	}
+
+	btrtl_dev->cfg_len = config_len;
+
+	bt_dev_dbg(hdev, "rtl: using config data with len %d from DT",
+		   config_len);
+
+	return 0;
+}
+
 struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
 {
 	struct btrtl_device_info *btrtl_dev;
@@ -480,12 +516,15 @@  struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
 		goto err_free;
 	}
 
-	if (cfg_name) {
+	/* try loading the config blob from device-tree first: */
+	ret = rtl_load_config_from_dt(hdev, btrtl_dev);
+	/* fall back to loading the config via request_firmware: */
+	if (ret && cfg_name) {
 		btrtl_dev->cfg_len = rtl_load_file(hdev, cfg_name,
 						   &btrtl_dev->cfg_data);
 		if (cfg_needed && btrtl_dev->cfg_len <= 0) {
 			bt_dev_err(hdev,
-				   "mandatory config file %s not found\n",
+				   "mandatory config blob not found in %s or DT\n",
 				   cfg_name);
 			ret = btrtl_dev->fw_len;
 			goto err_free;