diff mbox series

[01/13] dt-bindings: net: bluetooth: add support for Realtek Bluetooth chips

Message ID 20180527190457.2632-2-hdegoede@redhat.com
State Changes Requested, archived
Headers show
Series Bluetooth: Add RTL8723BS support | expand

Commit Message

Hans de Goede May 27, 2018, 7:04 p.m. UTC
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

This adds the documentation for Bluetooth functionality of the Realtek
RTL8723BS and RTL8723DS.
Both are SDIO wifi chips with an additional Bluetooth module which is
connected via UART to the host.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../bindings/net/realtek-bluetooth.txt        | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/realtek-bluetooth.txt

Comments

Marcel Holtmann May 30, 2018, 6:42 a.m. UTC | #1
Hi Hans,

> This adds the documentation for Bluetooth functionality of the Realtek
> RTL8723BS and RTL8723DS.
> Both are SDIO wifi chips with an additional Bluetooth module which is
> connected via UART to the host.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Signed-off-by: Jeremy Cline <jeremy@jcline.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> .../bindings/net/realtek-bluetooth.txt        | 41 +++++++++++++++++++
> 1 file changed, 41 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/realtek-bluetooth.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/realtek-bluetooth.txt b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
> new file mode 100644
> index 000000000000..1491329c4cd1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
> @@ -0,0 +1,41 @@
> +Realtek Bluetooth Chips
> +-----------------------
> +
> +This documents the binding structure and common properties for serial
> +attached Realtek devices.
> +
> +Serial attached Realtek devices shall be a child node of the host UART
> +device the slave device is attached to. See ../serial/slave-device.txt
> +for more information
> +
> +Required properties:
> +- compatible: should contain one of the following:
> +    * "realtek,rtl8723bs-bluetooth"
> +    * "realtek,rtl8723ds-bluetooth"
> +
> +Optional properties:
> +- realtek,config-data: Bluetooth chipset configuration data which is
> +			needed for communication (it typically contains
> +			board specific settings like the baudrate and
> +			whether flow-control is used).
> +			This is an array of u8 values.
> +- enable-gpios: GPIO specifier, used to enable/disable the BT module
> +- reset-gpios: GPIO specifier, used to reset the BT module
> +
> +
> +Example:
> +
> +&uart {
> +	...
> +
> +	bluetooth {
> +		compatible = "realtek,rtl8723bs-bluetooth";
> +		enable-gpios = <&gpio 20 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpio 11 GPIO_ACTIVE_HIGH>;
> +		realtek,config-data = /bits/ 8 <
> +			0x55 0xab 0x23 0x87 0x29 0x00 0xf4 0x00 0x01 0x01 0xf6 0x00
> +			0x02 0x81 0x00 0xfa 0x00 0x02 0x12 0x80 0x0c 0x00 0x10 0x02
> +			0x80 0x92 0x04 0x50 0xc5 0xea 0x19 0xe1 0x1b 0xfd 0xaf 0x5f
> +			0x01 0xa4 0x0b 0xd9 0x00 0x01 0x0f 0xe4 0x00 0x01 0x08>;
> +	};
> +};

we actually need to agree on this config-data. As I wrote in an earlier email some time ago, the actual config-data files are just memory portions that will overload the default config area. I wrote tools/rtlfw.c to parse these.

So now I wonder if we can just read the current configuration and run with that when no extra blob is provided. That way it would always work and we might just give the config-data filename here. Mostly this is the for PCM and UART settings anyway.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede May 30, 2018, 8:04 a.m. UTC | #2
Hi,

On 30-05-18 08:42, Marcel Holtmann wrote:
> Hi Hans,
> 
>> This adds the documentation for Bluetooth functionality of the Realtek
>> RTL8723BS and RTL8723DS.
>> Both are SDIO wifi chips with an additional Bluetooth module which is
>> connected via UART to the host.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Signed-off-by: Jeremy Cline <jeremy@jcline.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> .../bindings/net/realtek-bluetooth.txt        | 41 +++++++++++++++++++
>> 1 file changed, 41 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/realtek-bluetooth.txt b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>> new file mode 100644
>> index 000000000000..1491329c4cd1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>> @@ -0,0 +1,41 @@
>> +Realtek Bluetooth Chips
>> +-----------------------
>> +
>> +This documents the binding structure and common properties for serial
>> +attached Realtek devices.
>> +
>> +Serial attached Realtek devices shall be a child node of the host UART
>> +device the slave device is attached to. See ../serial/slave-device.txt
>> +for more information
>> +
>> +Required properties:
>> +- compatible: should contain one of the following:
>> +    * "realtek,rtl8723bs-bluetooth"
>> +    * "realtek,rtl8723ds-bluetooth"
>> +
>> +Optional properties:
>> +- realtek,config-data: Bluetooth chipset configuration data which is
>> +			needed for communication (it typically contains
>> +			board specific settings like the baudrate and
>> +			whether flow-control is used).
>> +			This is an array of u8 values.
>> +- enable-gpios: GPIO specifier, used to enable/disable the BT module
>> +- reset-gpios: GPIO specifier, used to reset the BT module
>> +
>> +
>> +Example:
>> +
>> +&uart {
>> +	...
>> +
>> +	bluetooth {
>> +		compatible = "realtek,rtl8723bs-bluetooth";
>> +		enable-gpios = <&gpio 20 GPIO_ACTIVE_HIGH>;
>> +		reset-gpios = <&gpio 11 GPIO_ACTIVE_HIGH>;
>> +		realtek,config-data = /bits/ 8 <
>> +			0x55 0xab 0x23 0x87 0x29 0x00 0xf4 0x00 0x01 0x01 0xf6 0x00
>> +			0x02 0x81 0x00 0xfa 0x00 0x02 0x12 0x80 0x0c 0x00 0x10 0x02
>> +			0x80 0x92 0x04 0x50 0xc5 0xea 0x19 0xe1 0x1b 0xfd 0xaf 0x5f
>> +			0x01 0xa4 0x0b 0xd9 0x00 0x01 0x0f 0xe4 0x00 0x01 0x08>;
>> +	};
>> +};
> 
> we actually need to agree on this config-data. As I wrote in an earlier email some time ago, the actual config-data files are just memory portions that will overload the default config area. I wrote tools/rtlfw.c to parse these.
> 
> So now I wonder if we can just read the current configuration and run with that when no extra blob is provided. That way it would always work and we might just give the config-data filename here. Mostly this is the for PCM and UART settings anyway.

I've been thinking about this too and 2 different solutions come to mind.

Note this is all x86 specific, I think it best to solve this for x86 first
and then we can apply the lessons learned there to ARM/devicetree when
someone comes along who wants to hook-up things on ARM.

My first idea was to stick with a config blob using the firmware_load
mechanism, but to add a postfix to the filename based on the ACPI
HID (hardware-id) of the serdev device, so in practice this means
we would try to load:

/lib/firmware/rtl_bt/rtl8723bs_config-OBDA8723.bin

On most x86 devices, so far all my testing on a bunch of different
devices has shown that the same config works for all x86 devices
(I took the config from a Chuwi Vi8 tablet which uses 3000000bps
+ hardware flowcontrol).

This way we can put the config in linux-firmware, without it
getting loaded on ARM boards where it might very well be wrong.


My second idea is to include a default config inside the driver,
which can be optionally overridden by a file in
/lib/firmware/rtl_bt and/or dt. Combined with module-options to
override the baudrate and flowcontrol setting (and patch the
builtin config accordingly).


Your idea to read back the config from the device is also
interesting, but I don't think that will gain us anything because
AFAIK the whole purpose of the board specific config file
bits is that the chip lack eeprom space to store this info,
so we will just always get some generic defaults.


I've run your rtlfw tool on a bunch of different config files and
there are a lot of unknown fields, and even of the known fields
I don't think we understand all the bits. So all in all the
config file is a bit of a black box and as such I believe it
would be best to just treat it as such and I personally
prefer my first idea, which is to add a postfix to the
firmware filename request, so on x86 load:

/lib/firmware/rtl_bt/rtl8723bs_config-OBDA8723.bin

And on devicetree we could postfix things with the machine
model string (as returned by  of_flat_dt_get_machine_name())


So Marcel, which direction do you think we should go?

Regards,

Hans

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring (Arm) May 31, 2018, 4:42 p.m. UTC | #3
On Wed, May 30, 2018 at 10:04:37AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 30-05-18 08:42, Marcel Holtmann wrote:
> > Hi Hans,
> > 
> > > This adds the documentation for Bluetooth functionality of the Realtek
> > > RTL8723BS and RTL8723DS.
> > > Both are SDIO wifi chips with an additional Bluetooth module which is
> > > connected via UART to the host.
> > > 
> > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > > Signed-off-by: Jeremy Cline <jeremy@jcline.org>
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > > .../bindings/net/realtek-bluetooth.txt        | 41 +++++++++++++++++++
> > > 1 file changed, 41 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/net/realtek-bluetooth.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/realtek-bluetooth.txt b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
> > > new file mode 100644
> > > index 000000000000..1491329c4cd1
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
> > > @@ -0,0 +1,41 @@
> > > +Realtek Bluetooth Chips
> > > +-----------------------
> > > +
> > > +This documents the binding structure and common properties for serial
> > > +attached Realtek devices.
> > > +
> > > +Serial attached Realtek devices shall be a child node of the host UART
> > > +device the slave device is attached to. See ../serial/slave-device.txt
> > > +for more information
> > > +
> > > +Required properties:
> > > +- compatible: should contain one of the following:
> > > +    * "realtek,rtl8723bs-bluetooth"
> > > +    * "realtek,rtl8723ds-bluetooth"
> > > +
> > > +Optional properties:
> > > +- realtek,config-data: Bluetooth chipset configuration data which is
> > > +			needed for communication (it typically contains
> > > +			board specific settings like the baudrate and
> > > +			whether flow-control is used).
> > > +			This is an array of u8 values.
> > > +- enable-gpios: GPIO specifier, used to enable/disable the BT module
> > > +- reset-gpios: GPIO specifier, used to reset the BT module
> > > +
> > > +
> > > +Example:
> > > +
> > > +&uart {
> > > +	...
> > > +
> > > +	bluetooth {
> > > +		compatible = "realtek,rtl8723bs-bluetooth";
> > > +		enable-gpios = <&gpio 20 GPIO_ACTIVE_HIGH>;
> > > +		reset-gpios = <&gpio 11 GPIO_ACTIVE_HIGH>;
> > > +		realtek,config-data = /bits/ 8 <
> > > +			0x55 0xab 0x23 0x87 0x29 0x00 0xf4 0x00 0x01 0x01 0xf6 0x00
> > > +			0x02 0x81 0x00 0xfa 0x00 0x02 0x12 0x80 0x0c 0x00 0x10 0x02
> > > +			0x80 0x92 0x04 0x50 0xc5 0xea 0x19 0xe1 0x1b 0xfd 0xaf 0x5f
> > > +			0x01 0xa4 0x0b 0xd9 0x00 0x01 0x0f 0xe4 0x00 0x01 0x08>;
> > > +	};
> > > +};
> > 
> > we actually need to agree on this config-data. As I wrote in an earlier email some time ago, the actual config-data files are just memory portions that will overload the default config area. I wrote tools/rtlfw.c to parse these.
> > 
> > So now I wonder if we can just read the current configuration and run with that when no extra blob is provided. That way it would always work and we might just give the config-data filename here. Mostly this is the for PCM and UART settings anyway.
> 
> I've been thinking about this too and 2 different solutions come to mind.
> 
> Note this is all x86 specific, I think it best to solve this for x86 first
> and then we can apply the lessons learned there to ARM/devicetree when
> someone comes along who wants to hook-up things on ARM.
> 
> My first idea was to stick with a config blob using the firmware_load
> mechanism, but to add a postfix to the filename based on the ACPI
> HID (hardware-id) of the serdev device, so in practice this means
> we would try to load:
> 
> /lib/firmware/rtl_bt/rtl8723bs_config-OBDA8723.bin
> 
> On most x86 devices, so far all my testing on a bunch of different
> devices has shown that the same config works for all x86 devices
> (I took the config from a Chuwi Vi8 tablet which uses 3000000bps
> + hardware flowcontrol).
> 
> This way we can put the config in linux-firmware, without it
> getting loaded on ARM boards where it might very well be wrong.
> 
> 
> My second idea is to include a default config inside the driver,
> which can be optionally overridden by a file in
> /lib/firmware/rtl_bt and/or dt. Combined with module-options to
> override the baudrate and flowcontrol setting (and patch the
> builtin config accordingly).
> 
> 
> Your idea to read back the config from the device is also
> interesting, but I don't think that will gain us anything because
> AFAIK the whole purpose of the board specific config file
> bits is that the chip lack eeprom space to store this info,
> so we will just always get some generic defaults.
> 
> 
> I've run your rtlfw tool on a bunch of different config files and
> there are a lot of unknown fields, and even of the known fields
> I don't think we understand all the bits. So all in all the
> config file is a bit of a black box and as such I believe it
> would be best to just treat it as such and I personally
> prefer my first idea, which is to add a postfix to the
> firmware filename request, so on x86 load:
> 
> /lib/firmware/rtl_bt/rtl8723bs_config-OBDA8723.bin
> 
> And on devicetree we could postfix things with the machine
> model string (as returned by  of_flat_dt_get_machine_name())

If the firmware file is going to depend on the DT, then you might as 
well just put the data into the DT.

There's a similar issue on QCom Dragonboards. The WiFi (and BT?) 
firmware files are supposedly board specific, so folks don't want to put 
them into linux-firmware. But it also seems to be unknown as to which 
parts are board specific are not.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede May 31, 2018, 8:50 p.m. UTC | #4
Hi,

On 31-05-18 18:42, Rob Herring wrote:
> On Wed, May 30, 2018 at 10:04:37AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 30-05-18 08:42, Marcel Holtmann wrote:
>>> Hi Hans,
>>>
>>>> This adds the documentation for Bluetooth functionality of the Realtek
>>>> RTL8723BS and RTL8723DS.
>>>> Both are SDIO wifi chips with an additional Bluetooth module which is
>>>> connected via UART to the host.
>>>>
>>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>>> Signed-off-by: Jeremy Cline <jeremy@jcline.org>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> .../bindings/net/realtek-bluetooth.txt        | 41 +++++++++++++++++++
>>>> 1 file changed, 41 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/realtek-bluetooth.txt b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>>> new file mode 100644
>>>> index 000000000000..1491329c4cd1
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>>> @@ -0,0 +1,41 @@
>>>> +Realtek Bluetooth Chips
>>>> +-----------------------
>>>> +
>>>> +This documents the binding structure and common properties for serial
>>>> +attached Realtek devices.
>>>> +
>>>> +Serial attached Realtek devices shall be a child node of the host UART
>>>> +device the slave device is attached to. See ../serial/slave-device.txt
>>>> +for more information
>>>> +
>>>> +Required properties:
>>>> +- compatible: should contain one of the following:
>>>> +    * "realtek,rtl8723bs-bluetooth"
>>>> +    * "realtek,rtl8723ds-bluetooth"
>>>> +
>>>> +Optional properties:
>>>> +- realtek,config-data: Bluetooth chipset configuration data which is
>>>> +			needed for communication (it typically contains
>>>> +			board specific settings like the baudrate and
>>>> +			whether flow-control is used).
>>>> +			This is an array of u8 values.
>>>> +- enable-gpios: GPIO specifier, used to enable/disable the BT module
>>>> +- reset-gpios: GPIO specifier, used to reset the BT module
>>>> +
>>>> +
>>>> +Example:
>>>> +
>>>> +&uart {
>>>> +	...
>>>> +
>>>> +	bluetooth {
>>>> +		compatible = "realtek,rtl8723bs-bluetooth";
>>>> +		enable-gpios = <&gpio 20 GPIO_ACTIVE_HIGH>;
>>>> +		reset-gpios = <&gpio 11 GPIO_ACTIVE_HIGH>;
>>>> +		realtek,config-data = /bits/ 8 <
>>>> +			0x55 0xab 0x23 0x87 0x29 0x00 0xf4 0x00 0x01 0x01 0xf6 0x00
>>>> +			0x02 0x81 0x00 0xfa 0x00 0x02 0x12 0x80 0x0c 0x00 0x10 0x02
>>>> +			0x80 0x92 0x04 0x50 0xc5 0xea 0x19 0xe1 0x1b 0xfd 0xaf 0x5f
>>>> +			0x01 0xa4 0x0b 0xd9 0x00 0x01 0x0f 0xe4 0x00 0x01 0x08>;
>>>> +	};
>>>> +};
>>>
>>> we actually need to agree on this config-data. As I wrote in an earlier email some time ago, the actual config-data files are just memory portions that will overload the default config area. I wrote tools/rtlfw.c to parse these.
>>>
>>> So now I wonder if we can just read the current configuration and run with that when no extra blob is provided. That way it would always work and we might just give the config-data filename here. Mostly this is the for PCM and UART settings anyway.
>>
>> I've been thinking about this too and 2 different solutions come to mind.
>>
>> Note this is all x86 specific, I think it best to solve this for x86 first
>> and then we can apply the lessons learned there to ARM/devicetree when
>> someone comes along who wants to hook-up things on ARM.
>>
>> My first idea was to stick with a config blob using the firmware_load
>> mechanism, but to add a postfix to the filename based on the ACPI
>> HID (hardware-id) of the serdev device, so in practice this means
>> we would try to load:
>>
>> /lib/firmware/rtl_bt/rtl8723bs_config-OBDA8723.bin
>>
>> On most x86 devices, so far all my testing on a bunch of different
>> devices has shown that the same config works for all x86 devices
>> (I took the config from a Chuwi Vi8 tablet which uses 3000000bps
>> + hardware flowcontrol).
>>
>> This way we can put the config in linux-firmware, without it
>> getting loaded on ARM boards where it might very well be wrong.
>>
>>
>> My second idea is to include a default config inside the driver,
>> which can be optionally overridden by a file in
>> /lib/firmware/rtl_bt and/or dt. Combined with module-options to
>> override the baudrate and flowcontrol setting (and patch the
>> builtin config accordingly).
>>
>>
>> Your idea to read back the config from the device is also
>> interesting, but I don't think that will gain us anything because
>> AFAIK the whole purpose of the board specific config file
>> bits is that the chip lack eeprom space to store this info,
>> so we will just always get some generic defaults.
>>
>>
>> I've run your rtlfw tool on a bunch of different config files and
>> there are a lot of unknown fields, and even of the known fields
>> I don't think we understand all the bits. So all in all the
>> config file is a bit of a black box and as such I believe it
>> would be best to just treat it as such and I personally
>> prefer my first idea, which is to add a postfix to the
>> firmware filename request, so on x86 load:
>>
>> /lib/firmware/rtl_bt/rtl8723bs_config-OBDA8723.bin
>>
>> And on devicetree we could postfix things with the machine
>> model string (as returned by  of_flat_dt_get_machine_name())
> 
> If the firmware file is going to depend on the DT, then you might as
> well just put the data into the DT.

Ok, note we don't need the entire firmware in DT, just a (binary
format) config file for the firmware. The question is if we are
going to try and break up the info from the config-file and then
regenerate it inside the kernel driver. Or if we are just going
to put the say 60 bytes in DT as an u8 array as the original
binding proposed by Martin Blumenstingl suggests.

Since we only know the meaning for a couple of the bytes in
the file, which is typically 40 - 60 bytes big, I think it
is probably best to just treat it as a blob.

> There's a similar issue on QCom Dragonboards. The WiFi (and BT?)
> firmware files are supposedly board specific, so folks don't want to put
> them into linux-firmware. But it also seems to be unknown as to which
> parts are board specific are not.

In my experience with broadcom and now the rtl8723bs the
actual firmware and the needed config data are separate.

Hmm that is actually no entirely true, for broadcom the
bluetooth patchram file depends on the clockcrystal freq
used on the board, so there are 2 versions of it for a
single chip, 1 for each of the 2 different freqs used.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcel Holtmann June 4, 2018, 10:17 a.m. UTC | #5
Hi Hans,

>>>>> This adds the documentation for Bluetooth functionality of the Realtek
>>>>> RTL8723BS and RTL8723DS.
>>>>> Both are SDIO wifi chips with an additional Bluetooth module which is
>>>>> connected via UART to the host.
>>>>> 
>>>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>>>> Signed-off-by: Jeremy Cline <jeremy@jcline.org>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>> .../bindings/net/realtek-bluetooth.txt        | 41 +++++++++++++++++++
>>>>> 1 file changed, 41 insertions(+)
>>>>> create mode 100644 Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>>>> 
>>>>> diff --git a/Documentation/devicetree/bindings/net/realtek-bluetooth.txt b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>>>> new file mode 100644
>>>>> index 000000000000..1491329c4cd1
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>>>> @@ -0,0 +1,41 @@
>>>>> +Realtek Bluetooth Chips
>>>>> +-----------------------
>>>>> +
>>>>> +This documents the binding structure and common properties for serial
>>>>> +attached Realtek devices.
>>>>> +
>>>>> +Serial attached Realtek devices shall be a child node of the host UART
>>>>> +device the slave device is attached to. See ../serial/slave-device.txt
>>>>> +for more information
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible: should contain one of the following:
>>>>> +    * "realtek,rtl8723bs-bluetooth"
>>>>> +    * "realtek,rtl8723ds-bluetooth"
>>>>> +
>>>>> +Optional properties:
>>>>> +- realtek,config-data: Bluetooth chipset configuration data which is
>>>>> +			needed for communication (it typically contains
>>>>> +			board specific settings like the baudrate and
>>>>> +			whether flow-control is used).
>>>>> +			This is an array of u8 values.
>>>>> +- enable-gpios: GPIO specifier, used to enable/disable the BT module
>>>>> +- reset-gpios: GPIO specifier, used to reset the BT module
>>>>> +
>>>>> +
>>>>> +Example:
>>>>> +
>>>>> +&uart {
>>>>> +	...
>>>>> +
>>>>> +	bluetooth {
>>>>> +		compatible = "realtek,rtl8723bs-bluetooth";
>>>>> +		enable-gpios = <&gpio 20 GPIO_ACTIVE_HIGH>;
>>>>> +		reset-gpios = <&gpio 11 GPIO_ACTIVE_HIGH>;
>>>>> +		realtek,config-data = /bits/ 8 <
>>>>> +			0x55 0xab 0x23 0x87 0x29 0x00 0xf4 0x00 0x01 0x01 0xf6 0x00
>>>>> +			0x02 0x81 0x00 0xfa 0x00 0x02 0x12 0x80 0x0c 0x00 0x10 0x02
>>>>> +			0x80 0x92 0x04 0x50 0xc5 0xea 0x19 0xe1 0x1b 0xfd 0xaf 0x5f
>>>>> +			0x01 0xa4 0x0b 0xd9 0x00 0x01 0x0f 0xe4 0x00 0x01 0x08>;
>>>>> +	};
>>>>> +};
>>>> 
>>>> we actually need to agree on this config-data. As I wrote in an earlier email some time ago, the actual config-data files are just memory portions that will overload the default config area. I wrote tools/rtlfw.c to parse these.
>>>> 
>>>> So now I wonder if we can just read the current configuration and run with that when no extra blob is provided. That way it would always work and we might just give the config-data filename here. Mostly this is the for PCM and UART settings anyway.
>>> 
>>> I've been thinking about this too and 2 different solutions come to mind.
>>> 
>>> Note this is all x86 specific, I think it best to solve this for x86 first
>>> and then we can apply the lessons learned there to ARM/devicetree when
>>> someone comes along who wants to hook-up things on ARM.
>>> 
>>> My first idea was to stick with a config blob using the firmware_load
>>> mechanism, but to add a postfix to the filename based on the ACPI
>>> HID (hardware-id) of the serdev device, so in practice this means
>>> we would try to load:
>>> 
>>> /lib/firmware/rtl_bt/rtl8723bs_config-OBDA8723.bin
>>> 
>>> On most x86 devices, so far all my testing on a bunch of different
>>> devices has shown that the same config works for all x86 devices
>>> (I took the config from a Chuwi Vi8 tablet which uses 3000000bps
>>> + hardware flowcontrol).
>>> 
>>> This way we can put the config in linux-firmware, without it
>>> getting loaded on ARM boards where it might very well be wrong.
>>> 
>>> 
>>> My second idea is to include a default config inside the driver,
>>> which can be optionally overridden by a file in
>>> /lib/firmware/rtl_bt and/or dt. Combined with module-options to
>>> override the baudrate and flowcontrol setting (and patch the
>>> builtin config accordingly).
>>> 
>>> 
>>> Your idea to read back the config from the device is also
>>> interesting, but I don't think that will gain us anything because
>>> AFAIK the whole purpose of the board specific config file
>>> bits is that the chip lack eeprom space to store this info,
>>> so we will just always get some generic defaults.
>>> 
>>> 
>>> I've run your rtlfw tool on a bunch of different config files and
>>> there are a lot of unknown fields, and even of the known fields
>>> I don't think we understand all the bits. So all in all the
>>> config file is a bit of a black box and as such I believe it
>>> would be best to just treat it as such and I personally
>>> prefer my first idea, which is to add a postfix to the
>>> firmware filename request, so on x86 load:
>>> 
>>> /lib/firmware/rtl_bt/rtl8723bs_config-OBDA8723.bin
>>> 
>>> And on devicetree we could postfix things with the machine
>>> model string (as returned by  of_flat_dt_get_machine_name())
>> If the firmware file is going to depend on the DT, then you might as
>> well just put the data into the DT.
> 
> Ok, note we don't need the entire firmware in DT, just a (binary
> format) config file for the firmware. The question is if we are
> going to try and break up the info from the config-file and then
> regenerate it inside the kernel driver. Or if we are just going
> to put the say 60 bytes in DT as an u8 array as the original
> binding proposed by Martin Blumenstingl suggests.
> 
> Since we only know the meaning for a couple of the bytes in
> the file, which is typically 40 - 60 bytes big, I think it
> is probably best to just treat it as a blob.

wouldn’t it be still better to then just reference the firmware file or an identifier to build it? Putting the pure binary blob into the DT seems kinda counterproductive and hardware to update. After all it most likely depends on the firmware loaded since it is the config ram space that this changes.

As a note, there are some hints that the config file is required for some firmware since it changes some memory around to make it successful boot.

>> There's a similar issue on QCom Dragonboards. The WiFi (and BT?)
>> firmware files are supposedly board specific, so folks don't want to put
>> them into linux-firmware. But it also seems to be unknown as to which
>> parts are board specific are not.
> 
> In my experience with broadcom and now the rtl8723bs the
> actual firmware and the needed config data are separate.
> 
> Hmm that is actually no entirely true, for broadcom the
> bluetooth patchram file depends on the clockcrystal freq
> used on the board, so there are 2 versions of it for a
> single chip, 1 for each of the 2 different freqs used.

are you using .hex or .hcd files for Broadcom? The .hex files are pure patchram, while the .hcd can actually contain extra HCI commands. I remember that some .hcd files contain also HCI commands to do extra settings. Maybe we need a tool that shows the details of these files. We have this for nokfw and rtlfw now.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede June 4, 2018, 1:58 p.m. UTC | #6
Hi,

On 04-06-18 12:17, Marcel Holtmann wrote:
> Hi Hans,
> 
>>>>>> This adds the documentation for Bluetooth functionality of the Realtek
>>>>>> RTL8723BS and RTL8723DS.
>>>>>> Both are SDIO wifi chips with an additional Bluetooth module which is
>>>>>> connected via UART to the host.
>>>>>>
>>>>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>>>>> Signed-off-by: Jeremy Cline <jeremy@jcline.org>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>> .../bindings/net/realtek-bluetooth.txt        | 41 +++++++++++++++++++
>>>>>> 1 file changed, 41 insertions(+)
>>>>>> create mode 100644 Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/net/realtek-bluetooth.txt b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>>>>> new file mode 100644
>>>>>> index 000000000000..1491329c4cd1
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>>>>> @@ -0,0 +1,41 @@
>>>>>> +Realtek Bluetooth Chips
>>>>>> +-----------------------
>>>>>> +
>>>>>> +This documents the binding structure and common properties for serial
>>>>>> +attached Realtek devices.
>>>>>> +
>>>>>> +Serial attached Realtek devices shall be a child node of the host UART
>>>>>> +device the slave device is attached to. See ../serial/slave-device.txt
>>>>>> +for more information
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: should contain one of the following:
>>>>>> +    * "realtek,rtl8723bs-bluetooth"
>>>>>> +    * "realtek,rtl8723ds-bluetooth"
>>>>>> +
>>>>>> +Optional properties:
>>>>>> +- realtek,config-data: Bluetooth chipset configuration data which is
>>>>>> +			needed for communication (it typically contains
>>>>>> +			board specific settings like the baudrate and
>>>>>> +			whether flow-control is used).
>>>>>> +			This is an array of u8 values.
>>>>>> +- enable-gpios: GPIO specifier, used to enable/disable the BT module
>>>>>> +- reset-gpios: GPIO specifier, used to reset the BT module
>>>>>> +
>>>>>> +
>>>>>> +Example:
>>>>>> +
>>>>>> +&uart {
>>>>>> +	...
>>>>>> +
>>>>>> +	bluetooth {
>>>>>> +		compatible = "realtek,rtl8723bs-bluetooth";
>>>>>> +		enable-gpios = <&gpio 20 GPIO_ACTIVE_HIGH>;
>>>>>> +		reset-gpios = <&gpio 11 GPIO_ACTIVE_HIGH>;
>>>>>> +		realtek,config-data = /bits/ 8 <
>>>>>> +			0x55 0xab 0x23 0x87 0x29 0x00 0xf4 0x00 0x01 0x01 0xf6 0x00
>>>>>> +			0x02 0x81 0x00 0xfa 0x00 0x02 0x12 0x80 0x0c 0x00 0x10 0x02
>>>>>> +			0x80 0x92 0x04 0x50 0xc5 0xea 0x19 0xe1 0x1b 0xfd 0xaf 0x5f
>>>>>> +			0x01 0xa4 0x0b 0xd9 0x00 0x01 0x0f 0xe4 0x00 0x01 0x08>;
>>>>>> +	};
>>>>>> +};
>>>>>
>>>>> we actually need to agree on this config-data. As I wrote in an earlier email some time ago, the actual config-data files are just memory portions that will overload the default config area. I wrote tools/rtlfw.c to parse these.
>>>>>
>>>>> So now I wonder if we can just read the current configuration and run with that when no extra blob is provided. That way it would always work and we might just give the config-data filename here. Mostly this is the for PCM and UART settings anyway.
>>>>
>>>> I've been thinking about this too and 2 different solutions come to mind.
>>>>
>>>> Note this is all x86 specific, I think it best to solve this for x86 first
>>>> and then we can apply the lessons learned there to ARM/devicetree when
>>>> someone comes along who wants to hook-up things on ARM.
>>>>
>>>> My first idea was to stick with a config blob using the firmware_load
>>>> mechanism, but to add a postfix to the filename based on the ACPI
>>>> HID (hardware-id) of the serdev device, so in practice this means
>>>> we would try to load:
>>>>
>>>> /lib/firmware/rtl_bt/rtl8723bs_config-OBDA8723.bin
>>>>
>>>> On most x86 devices, so far all my testing on a bunch of different
>>>> devices has shown that the same config works for all x86 devices
>>>> (I took the config from a Chuwi Vi8 tablet which uses 3000000bps
>>>> + hardware flowcontrol).
>>>>
>>>> This way we can put the config in linux-firmware, without it
>>>> getting loaded on ARM boards where it might very well be wrong.
>>>>
>>>>
>>>> My second idea is to include a default config inside the driver,
>>>> which can be optionally overridden by a file in
>>>> /lib/firmware/rtl_bt and/or dt. Combined with module-options to
>>>> override the baudrate and flowcontrol setting (and patch the
>>>> builtin config accordingly).
>>>>
>>>>
>>>> Your idea to read back the config from the device is also
>>>> interesting, but I don't think that will gain us anything because
>>>> AFAIK the whole purpose of the board specific config file
>>>> bits is that the chip lack eeprom space to store this info,
>>>> so we will just always get some generic defaults.
>>>>
>>>>
>>>> I've run your rtlfw tool on a bunch of different config files and
>>>> there are a lot of unknown fields, and even of the known fields
>>>> I don't think we understand all the bits. So all in all the
>>>> config file is a bit of a black box and as such I believe it
>>>> would be best to just treat it as such and I personally
>>>> prefer my first idea, which is to add a postfix to the
>>>> firmware filename request, so on x86 load:
>>>>
>>>> /lib/firmware/rtl_bt/rtl8723bs_config-OBDA8723.bin
>>>>
>>>> And on devicetree we could postfix things with the machine
>>>> model string (as returned by  of_flat_dt_get_machine_name())
>>> If the firmware file is going to depend on the DT, then you might as
>>> well just put the data into the DT.
>>
>> Ok, note we don't need the entire firmware in DT, just a (binary
>> format) config file for the firmware. The question is if we are
>> going to try and break up the info from the config-file and then
>> regenerate it inside the kernel driver. Or if we are just going
>> to put the say 60 bytes in DT as an u8 array as the original
>> binding proposed by Martin Blumenstingl suggests.
>>
>> Since we only know the meaning for a couple of the bytes in
>> the file, which is typically 40 - 60 bytes big, I think it
>> is probably best to just treat it as a blob.
> 
> wouldn’t it be still better to then just reference the firmware file or an identifier to build it? Putting the pure binary blob into the DT seems kinda counterproductive and hardware to update. After all it most likely depends on the firmware loaded since it is the config ram space that this changes.

Ah I see, so you suggest simply putting an identifier
in the DT which we then use to build the config-file
filename to request. That pretty much matches with what
I have in mind for the x86 code-paths as well as for
the somewhat related broadcom wifi nvram files.

So yes that sounds like a good idea.

With that said, given that I'm only testing this on x86
I think that for v2 of the patch-set it is probably best
to just drop the DT specific bits. Then when someone
comes along who wants to add the necessary glue for DT
based platforms they can brush the dust of the DT specific
patches and submit an actually tested version of them.

So for v2 I plan to just drop the DT bits and add a
patch to use the ACPI HID as an identifier in the
config-file filename requested on ACPI platforms.

> As a note, there are some hints that the config file is required for some firmware since it changes some memory around to make it successful boot.
> 
>>> There's a similar issue on QCom Dragonboards. The WiFi (and BT?)
>>> firmware files are supposedly board specific, so folks don't want to put
>>> them into linux-firmware. But it also seems to be unknown as to which
>>> parts are board specific are not.
>>
>> In my experience with broadcom and now the rtl8723bs the
>> actual firmware and the needed config data are separate.
>>
>> Hmm that is actually no entirely true, for broadcom the
>> bluetooth patchram file depends on the clockcrystal freq
>> used on the board, so there are 2 versions of it for a
>> single chip, 1 for each of the 2 different freqs used.
> 
> are you using .hex or .hcd files for Broadcom? The .hex files are pure patchram, while the .hcd can actually contain extra HCI commands. I remember that some .hcd files contain also HCI commands to do extra settings. Maybe we need a tool that shows the details of these files. We have this for nokfw and rtlfw now.

I'm using hcd files for broadcom.

Regards,

Hans

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcel Holtmann June 4, 2018, 7:11 p.m. UTC | #7
Hi Hans,

>>>>>>> This adds the documentation for Bluetooth functionality of the Realtek
>>>>>>> RTL8723BS and RTL8723DS.
>>>>>>> Both are SDIO wifi chips with an additional Bluetooth module which is
>>>>>>> connected via UART to the host.
>>>>>>> 
>>>>>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>>>>>> Signed-off-by: Jeremy Cline <jeremy@jcline.org>
>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>> ---
>>>>>>> .../bindings/net/realtek-bluetooth.txt        | 41 +++++++++++++++++++
>>>>>>> 1 file changed, 41 insertions(+)
>>>>>>> create mode 100644 Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>>>>>> 
>>>>>>> diff --git a/Documentation/devicetree/bindings/net/realtek-bluetooth.txt b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..1491329c4cd1
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
>>>>>>> @@ -0,0 +1,41 @@
>>>>>>> +Realtek Bluetooth Chips
>>>>>>> +-----------------------
>>>>>>> +
>>>>>>> +This documents the binding structure and common properties for serial
>>>>>>> +attached Realtek devices.
>>>>>>> +
>>>>>>> +Serial attached Realtek devices shall be a child node of the host UART
>>>>>>> +device the slave device is attached to. See ../serial/slave-device.txt
>>>>>>> +for more information
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +- compatible: should contain one of the following:
>>>>>>> +    * "realtek,rtl8723bs-bluetooth"
>>>>>>> +    * "realtek,rtl8723ds-bluetooth"
>>>>>>> +
>>>>>>> +Optional properties:
>>>>>>> +- realtek,config-data: Bluetooth chipset configuration data which is
>>>>>>> +			needed for communication (it typically contains
>>>>>>> +			board specific settings like the baudrate and
>>>>>>> +			whether flow-control is used).
>>>>>>> +			This is an array of u8 values.
>>>>>>> +- enable-gpios: GPIO specifier, used to enable/disable the BT module
>>>>>>> +- reset-gpios: GPIO specifier, used to reset the BT module
>>>>>>> +
>>>>>>> +
>>>>>>> +Example:
>>>>>>> +
>>>>>>> +&uart {
>>>>>>> +	...
>>>>>>> +
>>>>>>> +	bluetooth {
>>>>>>> +		compatible = "realtek,rtl8723bs-bluetooth";
>>>>>>> +		enable-gpios = <&gpio 20 GPIO_ACTIVE_HIGH>;
>>>>>>> +		reset-gpios = <&gpio 11 GPIO_ACTIVE_HIGH>;
>>>>>>> +		realtek,config-data = /bits/ 8 <
>>>>>>> +			0x55 0xab 0x23 0x87 0x29 0x00 0xf4 0x00 0x01 0x01 0xf6 0x00
>>>>>>> +			0x02 0x81 0x00 0xfa 0x00 0x02 0x12 0x80 0x0c 0x00 0x10 0x02
>>>>>>> +			0x80 0x92 0x04 0x50 0xc5 0xea 0x19 0xe1 0x1b 0xfd 0xaf 0x5f
>>>>>>> +			0x01 0xa4 0x0b 0xd9 0x00 0x01 0x0f 0xe4 0x00 0x01 0x08>;
>>>>>>> +	};
>>>>>>> +};
>>>>>> 
>>>>>> we actually need to agree on this config-data. As I wrote in an earlier email some time ago, the actual config-data files are just memory portions that will overload the default config area. I wrote tools/rtlfw.c to parse these.
>>>>>> 
>>>>>> So now I wonder if we can just read the current configuration and run with that when no extra blob is provided. That way it would always work and we might just give the config-data filename here. Mostly this is the for PCM and UART settings anyway.
>>>>> 
>>>>> I've been thinking about this too and 2 different solutions come to mind.
>>>>> 
>>>>> Note this is all x86 specific, I think it best to solve this for x86 first
>>>>> and then we can apply the lessons learned there to ARM/devicetree when
>>>>> someone comes along who wants to hook-up things on ARM.
>>>>> 
>>>>> My first idea was to stick with a config blob using the firmware_load
>>>>> mechanism, but to add a postfix to the filename based on the ACPI
>>>>> HID (hardware-id) of the serdev device, so in practice this means
>>>>> we would try to load:
>>>>> 
>>>>> /lib/firmware/rtl_bt/rtl8723bs_config-OBDA8723.bin
>>>>> 
>>>>> On most x86 devices, so far all my testing on a bunch of different
>>>>> devices has shown that the same config works for all x86 devices
>>>>> (I took the config from a Chuwi Vi8 tablet which uses 3000000bps
>>>>> + hardware flowcontrol).
>>>>> 
>>>>> This way we can put the config in linux-firmware, without it
>>>>> getting loaded on ARM boards where it might very well be wrong.
>>>>> 
>>>>> 
>>>>> My second idea is to include a default config inside the driver,
>>>>> which can be optionally overridden by a file in
>>>>> /lib/firmware/rtl_bt and/or dt. Combined with module-options to
>>>>> override the baudrate and flowcontrol setting (and patch the
>>>>> builtin config accordingly).
>>>>> 
>>>>> 
>>>>> Your idea to read back the config from the device is also
>>>>> interesting, but I don't think that will gain us anything because
>>>>> AFAIK the whole purpose of the board specific config file
>>>>> bits is that the chip lack eeprom space to store this info,
>>>>> so we will just always get some generic defaults.
>>>>> 
>>>>> 
>>>>> I've run your rtlfw tool on a bunch of different config files and
>>>>> there are a lot of unknown fields, and even of the known fields
>>>>> I don't think we understand all the bits. So all in all the
>>>>> config file is a bit of a black box and as such I believe it
>>>>> would be best to just treat it as such and I personally
>>>>> prefer my first idea, which is to add a postfix to the
>>>>> firmware filename request, so on x86 load:
>>>>> 
>>>>> /lib/firmware/rtl_bt/rtl8723bs_config-OBDA8723.bin
>>>>> 
>>>>> And on devicetree we could postfix things with the machine
>>>>> model string (as returned by  of_flat_dt_get_machine_name())
>>>> If the firmware file is going to depend on the DT, then you might as
>>>> well just put the data into the DT.
>>> 
>>> Ok, note we don't need the entire firmware in DT, just a (binary
>>> format) config file for the firmware. The question is if we are
>>> going to try and break up the info from the config-file and then
>>> regenerate it inside the kernel driver. Or if we are just going
>>> to put the say 60 bytes in DT as an u8 array as the original
>>> binding proposed by Martin Blumenstingl suggests.
>>> 
>>> Since we only know the meaning for a couple of the bytes in
>>> the file, which is typically 40 - 60 bytes big, I think it
>>> is probably best to just treat it as a blob.
>> wouldn’t it be still better to then just reference the firmware file or an identifier to build it? Putting the pure binary blob into the DT seems kinda counterproductive and hardware to update. After all it most likely depends on the firmware loaded since it is the config ram space that this changes.
> 
> Ah I see, so you suggest simply putting an identifier
> in the DT which we then use to build the config-file
> filename to request. That pretty much matches with what
> I have in mind for the x86 code-paths as well as for
> the somewhat related broadcom wifi nvram files.
> 
> So yes that sounds like a good idea.
> 
> With that said, given that I'm only testing this on x86
> I think that for v2 of the patch-set it is probably best
> to just drop the DT specific bits. Then when someone
> comes along who wants to add the necessary glue for DT
> based platforms they can brush the dust of the DT specific
> patches and submit an actually tested version of them.
> 
> So for v2 I plan to just drop the DT bits and add a
> patch to use the ACPI HID as an identifier in the
> config-file filename requested on ACPI platforms.

that might be a good idea. We can also discuss to actually load a mapping file via request firmware. Something that I have been working on for Broadcom devices. So for companies that are not so Linux friendly, you can easily grab the Windows driver and extract the firmware files.

>> As a note, there are some hints that the config file is required for some firmware since it changes some memory around to make it successful boot.
>>>> There's a similar issue on QCom Dragonboards. The WiFi (and BT?)
>>>> firmware files are supposedly board specific, so folks don't want to put
>>>> them into linux-firmware. But it also seems to be unknown as to which
>>>> parts are board specific are not.
>>> 
>>> In my experience with broadcom and now the rtl8723bs the
>>> actual firmware and the needed config data are separate.
>>> 
>>> Hmm that is actually no entirely true, for broadcom the
>>> bluetooth patchram file depends on the clockcrystal freq
>>> used on the board, so there are 2 versions of it for a
>>> single chip, 1 for each of the 2 different freqs used.
>> are you using .hex or .hcd files for Broadcom? The .hex files are pure patchram, while the .hcd can actually contain extra HCI commands. I remember that some .hcd files contain also HCI commands to do extra settings. Maybe we need a tool that shows the details of these files. We have this for nokfw and rtlfw now.
> 
> I'm using hcd files for broadcom.

so I added tools/bcmfw to analyze HCD files. We might also need a compare HCD files option. However you might be able to compare the HCD files that you think are different for some of your hardware.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede June 9, 2018, 11:29 a.m. UTC | #8
Hi,

On 04-06-18 21:11, Marcel Holtmann wrote:

<snip>

>>>> Hmm that is actually no entirely true, for broadcom the
>>>> bluetooth patchram file depends on the clockcrystal freq
>>>> used on the board, so there are 2 versions of it for a
>>>> single chip, 1 for each of the 2 different freqs used.
>>> are you using .hex or .hcd files for Broadcom? The .hex files are pure patchram, while the .hcd can actually contain extra HCI commands. I remember that some .hcd files contain also HCI commands to do extra settings. Maybe we need a tool that shows the details of these files. We have this for nokfw and rtlfw now.
>>
>> I'm using hcd files for broadcom.
> 
> so I added tools/bcmfw to analyze HCD files. We might also need a compare HCD files option. However you might be able to compare the HCD files that you think are different for some of your hardware.

The problem is that I don't think I can get the exact same
version / build of the patchram in 26MHz and 37.4 Mhz versions
and comparing different versions is not really going to be useful.

All I really know here is that I can usually take any 37.4MHz build,
as indicated by the build string in the hcd file, e.g. :

BCM43438A1 37.4MHz Raspberry Pi 3-0043O

And use that on a board where the wifi nvram file says the
crystal is 37.4MHz and things will work fine. Where as any
hcd with 26Mhz in the buildstring will not work.

My work on the brcm bluetooth code has left me to believe
that the ACPI HID is (supposed to be) unique per board.

I'm thinking of adding a postfix string as driver_data
in the acpi_match_id table for ACPI HIDs which I can test
and then make the bcm-bt code first try e.g. firmware_request
BCM4356A2-26M.hcd before calling BCM4356A2.hcd .

I could instead simply postfix the ACPI HID (as I plan to
do for the realtek btconfig) but as said in all my testing
simply using the latest build, with the right crystal
freq seems to work best. So I would prefer to go with
e.g. BCM4356A2-26M.hcd rather then BCM4356A2-BCM2E74.hcd
this way we will only need 2 hcd files (26M and 37.4M) per
chipset rather a whole lot of them.

Marcel, what do you think of this, would you accept a (to-be-written)
patchset adding -26M / -37.4M postfixes as described above?

This is all aimed at getting the hcd files into linux-firmware,
where less files definitely would be more. Note I'm only talking
about serdev attached bcm-bt devices here. For usb we can keep
using the existing scheme or come up with a new scheme.

Regards,

Hans


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcel Holtmann June 11, 2018, 1:32 p.m. UTC | #9
Hi Hans,

>>>>> Hmm that is actually no entirely true, for broadcom the
>>>>> bluetooth patchram file depends on the clockcrystal freq
>>>>> used on the board, so there are 2 versions of it for a
>>>>> single chip, 1 for each of the 2 different freqs used.
>>>> are you using .hex or .hcd files for Broadcom? The .hex files are pure patchram, while the .hcd can actually contain extra HCI commands. I remember that some .hcd files contain also HCI commands to do extra settings. Maybe we need a tool that shows the details of these files. We have this for nokfw and rtlfw now.
>>> 
>>> I'm using hcd files for broadcom.
>> so I added tools/bcmfw to analyze HCD files. We might also need a compare HCD files option. However you might be able to compare the HCD files that you think are different for some of your hardware.
> 
> The problem is that I don't think I can get the exact same
> version / build of the patchram in 26MHz and 37.4 Mhz versions
> and comparing different versions is not really going to be useful.
> 
> All I really know here is that I can usually take any 37.4MHz build,
> as indicated by the build string in the hcd file, e.g. :
> 
> BCM43438A1 37.4MHz Raspberry Pi 3-0043O
> 
> And use that on a board where the wifi nvram file says the
> crystal is 37.4MHz and things will work fine. Where as any
> hcd with 26Mhz in the buildstring will not work.
> 
> My work on the brcm bluetooth code has left me to believe
> that the ACPI HID is (supposed to be) unique per board.

for ACPI it is really suppose to be unique. Do you happen to have a link for a good download of the latest Broadcom firmware?

> I'm thinking of adding a postfix string as driver_data
> in the acpi_match_id table for ACPI HIDs which I can test
> and then make the bcm-bt code first try e.g. firmware_request
> BCM4356A2-26M.hcd before calling BCM4356A2.hcd .
> 
> I could instead simply postfix the ACPI HID (as I plan to
> do for the realtek btconfig) but as said in all my testing
> simply using the latest build, with the right crystal
> freq seems to work best. So I would prefer to go with
> e.g. BCM4356A2-26M.hcd rather then BCM4356A2-BCM2E74.hcd
> this way we will only need 2 hcd files (26M and 37.4M) per
> chipset rather a whole lot of them.
> 
> Marcel, what do you think of this, would you accept a (to-be-written)
> patchset adding -26M / -37.4M postfixes as described above?

I am not in favor of it. I prefer giving the vendor the chance to list the right firmware for their hardware instead of us guessing. Whatever comes out of the Broadcom Windows driver should be all least passed certification.

> This is all aimed at getting the hcd files into linux-firmware,
> where less files definitely would be more. Note I'm only talking
> about serdev attached bcm-bt devices here. For usb we can keep
> using the existing scheme or come up with a new scheme.

Seems that Broadcom opted for per ACPI HID firmware files. This is in contrast to Intel for example where we read hardware information and pick the firmware based on that information.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/realtek-bluetooth.txt b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
new file mode 100644
index 000000000000..1491329c4cd1
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
@@ -0,0 +1,41 @@ 
+Realtek Bluetooth Chips
+-----------------------
+
+This documents the binding structure and common properties for serial
+attached Realtek devices.
+
+Serial attached Realtek devices shall be a child node of the host UART
+device the slave device is attached to. See ../serial/slave-device.txt
+for more information
+
+Required properties:
+- compatible: should contain one of the following:
+    * "realtek,rtl8723bs-bluetooth"
+    * "realtek,rtl8723ds-bluetooth"
+
+Optional properties:
+- realtek,config-data: Bluetooth chipset configuration data which is
+			needed for communication (it typically contains
+			board specific settings like the baudrate and
+			whether flow-control is used).
+			This is an array of u8 values.
+- enable-gpios: GPIO specifier, used to enable/disable the BT module
+- reset-gpios: GPIO specifier, used to reset the BT module
+
+
+Example:
+
+&uart {
+	...
+
+	bluetooth {
+		compatible = "realtek,rtl8723bs-bluetooth";
+		enable-gpios = <&gpio 20 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio 11 GPIO_ACTIVE_HIGH>;
+		realtek,config-data = /bits/ 8 <
+			0x55 0xab 0x23 0x87 0x29 0x00 0xf4 0x00 0x01 0x01 0xf6 0x00
+			0x02 0x81 0x00 0xfa 0x00 0x02 0x12 0x80 0x0c 0x00 0x10 0x02
+			0x80 0x92 0x04 0x50 0xc5 0xea 0x19 0xe1 0x1b 0xfd 0xaf 0x5f
+			0x01 0xa4 0x0b 0xd9 0x00 0x01 0x0f 0xe4 0x00 0x01 0x08>;
+	};
+};