Realtek Bluetooth serdev support (H5 protocol)

Some Bluetooth modules (for example the ones found in Realtek RTL8723BS and RTL8723DS) want to communicate with the host with even parity enabled.
Add a new function and the corresponding internal callbacks so parity can be configured. This supports enabling and disabling parity as well as setting the type to odd or even.

Signed-off-by Martin Blumenstingl <[email protected]>

---

drivers/tty/serdev/core.c           | 12 ++++++++++++
drivers/tty/serdev/serdev-ttyport.c | 21 +++++++++++++++++++++
include/linux/serdev.h              |  5 +++++
3 files changed, 38 insertions(+)

drivers/tty/serdev/core.c +2 -0

index 1bef39828ca7..d327b02980f5 100644
@@ -225,6 +225,18 @@ void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable)
 }
 EXPORT_SYMBOL_GPL(serdev_device_set_flow_control);
 
+void serdev_device_set_parity(struct serdev_device *serdev, bool enable,
+			      bool odd)

Hi Martin,

Quoted 22 lines

Some Bluetooth modules (for example the ones found in Realtek RTL8723BS and RTL8723DS) want to communicate with the host with even parity enabled.
Add a new function and the corresponding internal callbacks so parity can be configured. This supports enabling and disabling parity as well as setting the type to odd or even.

Signed-off-by Martin Blumenstingl <[email protected]>

---

drivers/tty/serdev/core.c           | 12 ++++++++++++
drivers/tty/serdev/serdev-ttyport.c | 21 +++++++++++++++++++++
include/linux/serdev.h              |  5 +++++
3 files changed, 38 insertions(+)

drivers/tty/serdev/core.c +0 -0

index 1bef39828ca7..d327b02980f5 100644
@@ -225,6 +225,18 @@ void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable)
 }
 EXPORT_SYMBOL_GPL(serdev_device_set_flow_control);
 

drivers/tty/serdev/core.c void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable) · line 239

Quoted 12 lines

drivers/tty/serdev/core.c +12 -0

+void serdev_device_set_parity(struct serdev_device *serdev, bool enable,
+                           bool odd)
+{
+     struct serdev_controller *ctrl = serdev->ctrl;
+
+     if (!ctrl || !ctrl->ops->set_parity)
+             return;
+
+     ctrl->ops->set_parity(ctrl, enable, odd);
+}
+EXPORT_SYMBOL_GPL(serdev_device_set_parity);
+

this really needs Rob?s ACK before I take the patch.

Hi Marcel, Hi Rob,

Quoted 24 lines · Marcel Holtmann

Hi Martin,

Quoted 22 lines

Some Bluetooth modules (for example the ones found in Realtek RTL8723BS and RTL8723DS) want to communicate with the host with even parity enabled.
Add a new function and the corresponding internal callbacks so parity can be configured. This supports enabling and disabling parity as well as setting the type to odd or even.

Signed-off-by Martin Blumenstingl <[email protected]>

---

drivers/tty/serdev/core.c           | 12 ++++++++++++
drivers/tty/serdev/serdev-ttyport.c | 21 +++++++++++++++++++++
include/linux/serdev.h              |  5 +++++
3 files changed, 38 insertions(+)

drivers/tty/serdev/core.c +0 -0

index 1bef39828ca7..d327b02980f5 100644
@@ -225,6 +225,18 @@ void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable)
 }
 EXPORT_SYMBOL_GPL(serdev_device_set_flow_control);
 

Quoted 14 lines

drivers/tty/serdev/core.c void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable) · line 239

Quoted 12 lines

drivers/tty/serdev/core.c +12 -0

+void serdev_device_set_parity(struct serdev_device *serdev, bool enable,
+                           bool odd)
+{
+     struct serdev_controller *ctrl = serdev->ctrl;
+
+     if (!ctrl || !ctrl->ops->set_parity)
+             return;
+
+     ctrl->ops->set_parity(ctrl, enable, odd);
+}
+EXPORT_SYMBOL_GPL(serdev_device_set_parity);
+

this really needs Rob?s ACK before I take the patch.

sure

I could even live with a NACK in case these two bool parameters are considered to be ugly
in that case I would propose an enum with three values: DISABLED, EVEN, ODD so the arguments would look like this:
void serdev_device_set_parity(struct serdev_device *serdev, enum parity)

Quoted 41 lines · Martin Blumenstingl

Hi Marcel, Hi Rob,

Quoted 24 lines · Marcel Holtmann

Hi Martin,

Quoted 22 lines

Some Bluetooth modules (for example the ones found in Realtek RTL8723BS and RTL8723DS) want to communicate with the host with even parity enabled.
Add a new function and the corresponding internal callbacks so parity can be configured. This supports enabling and disabling parity as well as setting the type to odd or even.

Signed-off-by Martin Blumenstingl <[email protected]>

---

drivers/tty/serdev/core.c           | 12 ++++++++++++
drivers/tty/serdev/serdev-ttyport.c | 21 +++++++++++++++++++++
include/linux/serdev.h              |  5 +++++
3 files changed, 38 insertions(+)

drivers/tty/serdev/core.c +0 -0

index 1bef39828ca7..d327b02980f5 100644
@@ -225,6 +225,18 @@ void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable)
 }
 EXPORT_SYMBOL_GPL(serdev_device_set_flow_control);
 

Quoted 14 lines

drivers/tty/serdev/core.c void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable) · line 239

Quoted 12 lines

drivers/tty/serdev/core.c +12 -0

+void serdev_device_set_parity(struct serdev_device *serdev, bool enable,
+                           bool odd)
+{
+     struct serdev_controller *ctrl = serdev->ctrl;
+
+     if (!ctrl || !ctrl->ops->set_parity)
+             return;
+
+     ctrl->ops->set_parity(ctrl, enable, odd);
+}
+EXPORT_SYMBOL_GPL(serdev_device_set_parity);
+

this really needs Rob?s ACK before I take the patch.

Quoted 7 lines

sure

I could even live with a NACK in case these two bool parameters are considered to be ugly
in that case I would propose an enum with three values: DISABLED, EVEN, ODD so the arguments would look like this:
void serdev_device_set_parity(struct serdev_device *serdev, enum parity)

I just discovered: such a patch was already posted by Ulrich Hecht: [0]

[0] https://patchwork.kernel.org/patch/9903787/

Quoted 36 lines

Quoted 22 lines

Quoted 22 lines

Some Bluetooth modules (for example the ones found in Realtek RTL8723BS and RTL8723DS) want to communicate with the host with even parity enabled.
Add a new function and the corresponding internal callbacks so parity can be configured. This supports enabling and disabling parity as well as setting the type to odd or even.

Signed-off-by Martin Blumenstingl <[email protected]>

---

drivers/tty/serdev/core.c           | 12 ++++++++++++
drivers/tty/serdev/serdev-ttyport.c | 21 +++++++++++++++++++++
include/linux/serdev.h              |  5 +++++
3 files changed, 38 insertions(+)

drivers/tty/serdev/core.c +0 -0

index 1bef39828ca7..d327b02980f5 100644
@@ -225,6 +225,18 @@ void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable)
 }
 EXPORT_SYMBOL_GPL(serdev_device_set_flow_control);
 

Quoted 14 lines

drivers/tty/serdev/core.c void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable) · line 239

Quoted 12 lines

drivers/tty/serdev/core.c +12 -0

+void serdev_device_set_parity(struct serdev_device *serdev, bool enable,
+                           bool odd)
+{
+     struct serdev_controller *ctrl = serdev->ctrl;
+
+     if (!ctrl || !ctrl->ops->set_parity)
+             return;
+
+     ctrl->ops->set_parity(ctrl, enable, odd);
+}
+EXPORT_SYMBOL_GPL(serdev_device_set_parity);
+

this really needs Rob?s ACK before I take the patch.

Quoted 7 lines

sure

I could even live with a NACK in case these two bool parameters are considered to be ugly
in that case I would propose an enum with three values: DISABLED, EVEN, ODD so the arguments would look like this:
void serdev_device_set_parity(struct serdev_device *serdev, enum parity)

I just discovered: such a patch was already posted by Ulrich Hecht: [0]

[0] https://patchwork.kernel.org/patch/9903787/

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 <[email protected]>

---
.../devicetree/bindings/net/realtek-bluetooth.txt | 41 ++++++++++++++++++++++

1 file changed, 41 insertions(+)

create mode 100644 Documentation/devicetree/bindings/net/realtek-bluetooth.txt

Documentation/devicetree/bindings/net/realtek-bluetooth.txt new file +11 -0

new file mode 100644
index 000000000000..1491329c4cd1
@@ -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:

Documentation/devicetree/bindings/net/realtek-bluetooth.txt new file +10 -0

+- enable-gpios: GPIO specifier, used to enable/disable the BT module
+- reset-gpios: GPIO specifier, used to reset the BT module
+
+
+Example:
+
+&uart {
+	...
+
+	bluetooth {

Hi Martin,

Quoted 32 lines

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 <[email protected]>

---
.../devicetree/bindings/net/realtek-bluetooth.txt | 41 ++++++++++++++++++++++

1 file changed, 41 insertions(+)

create mode 100644 Documentation/devicetree/bindings/net/realtek-bluetooth.txt

Documentation/devicetree/bindings/net/realtek-bluetooth.txt new file +15 -0

new file mode 100644
index 000000000000..1491329c4cd1
@@ -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"
+

Documentation/devicetree/bindings/net/realtek-bluetooth.txt line 21

Quoted 6 lines

Documentation/devicetree/bindings/net/realtek-bluetooth.txt new file +6 -0

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

any chance we can at least include the basic format of these config blobs. And I prefer at least an ACK from Rob here.

Quoted 12 lines

Documentation/devicetree/bindings/net/realtek-bluetooth.txt new file +12 -0

+- 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>;

Documentation/devicetree/bindings/net/realtek-bluetooth.txt line 41

Quoted 8 lines

Documentation/devicetree/bindings/net/realtek-bluetooth.txt new file +8 -0

+             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>;
+     };
+};

Hi Marcel,

Quoted 34 lines · Marcel Holtmann

Hi Martin,

Quoted 32 lines

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 <[email protected]>

---
.../devicetree/bindings/net/realtek-bluetooth.txt | 41 ++++++++++++++++++++++

1 file changed, 41 insertions(+)

create mode 100644 Documentation/devicetree/bindings/net/realtek-bluetooth.txt

Documentation/devicetree/bindings/net/realtek-bluetooth.txt new file +15 -0

new file mode 100644
index 000000000000..1491329c4cd1
@@ -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"
+

Quoted 8 lines

Documentation/devicetree/bindings/net/realtek-bluetooth.txt line 21

Quoted 6 lines

Documentation/devicetree/bindings/net/realtek-bluetooth.txt new file +6 -0

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

any chance we can at least include the basic format of these config blobs. And I prefer at least an ACK from Rob here.

with including the basic format you mean a description that the config blob should start with 0x55 0xab 0x23 0x87 (which translates to:
0x8723ab55)?

I think all non-trivial dt-binding patches should be ACKed by the DT maintainers, so waiting for Rob's ACK is perfectly fine for me

Quoted 2 lines

Quoted 2 lines

Documentation/devicetree/bindings/net/realtek-bluetooth.txt new file +2 -0

+- enable-gpios: GPIO specifier, used to enable/disable the BT module
+- reset-gpios: GPIO specifier, used to reset the BT module

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.

Hi Martin,

Quoted 15 lines

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

Quoted 3 lines

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.

Quoted 19 lines

Quoted 18 lines

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.

Quoted 1 line

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

Hi Carlo,

Quoted 19 lines

Quoted 19 lines

Quoted 18 lines

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.

Quoted 5 lines

Quoted 1 line

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.

Quoted 25 lines

Quoted 24 lines

Quoted 15 lines

Quoted 15 lines

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

Quoted 5 lines

Quoted 3 lines

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

Quoted 1 line

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

Hi Carlo,

Quoted 25 lines

Quoted 25 lines

Quoted 24 lines

Quoted 15 lines

Quoted 15 lines

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

Quoted 5 lines

Quoted 3 lines

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

Quoted 6 lines

Quoted 1 line

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.

Quoted 27 lines · Marcel Holtmann

Hi Carlo,

Quoted 24 lines

Quoted 15 lines

Quoted 15 lines

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

Quoted 5 lines

Quoted 3 lines

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

Quoted 1 line

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

Hi Carlo,

Quoted 33 lines · Marcel Holtmann

Quoted 21 lines

Hi Carlo,

Quoted 19 lines

Quoted 19 lines

Quoted 18 lines

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.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

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

Quoted 1 line

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

Quoted 1 line · Martin Blumenstingl

Hi Carlo,

Hi Martin,

Quoted 39 lines · Carlo Caione

Quoted 28 lines · Marcel Holtmann

Quoted 27 lines

Hi Carlo,

Quoted 24 lines

Quoted 15 lines

Quoted 15 lines

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

Quoted 5 lines

Quoted 3 lines

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

Quoted 6 lines

Quoted 1 line

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

Quoted 3 lines

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.

Hi Carlo,

Quoted 4 lines

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)

Quoted 4 lines

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?

Quoted 1 line

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

great, thank you!

Quoted 9 lines

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 7 lines

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

Quoted 5 lines

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.

Quoted 11 lines

Quoted 9 lines

Quoted 4 lines

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?

Quoted 4 lines

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.

Quoted 12 lines

Quoted 11 lines

Quoted 9 lines

Quoted 4 lines

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?

Quoted 8 lines

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

Quoted 4 lines

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.

Hi Carlo,

Quoted 50 lines

Quoted 12 lines

Quoted 11 lines

Quoted 9 lines

Quoted 4 lines

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?

Quoted 8 lines

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

Quoted 4 lines

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.

Quoted 9 lines

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;
      }

Quoted 1 line

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.

Quoted 47 lines

Quoted 47 lines

Quoted 12 lines

Quoted 11 lines

Quoted 9 lines

Quoted 4 lines

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?

Quoted 8 lines

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

Quoted 4 lines

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 }

Quoted 3 lines

Quoted 3 lines

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

Quoted 9 lines

Quoted 9 lines

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

Quoted 1 line

Quoted 1 line

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]

Quoted 19 lines

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 }

Quoted 1 line

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)

Quoted 5 lines

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]

Quoted 1 line

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

Quoted 1 line

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

The three-wire (H5) protocol is the only protocol which uses HCI_UART_INIT_PENDING.
Unfortunately the benefits of using this flag are currently unknown. It was added in commit 9f2aee848fe6 ("Bluetooth: Add delayed init sequence support for UART controllers"). In my experiments (with the "rtk_hciattach" tool - a customized version of hciattach for Realtek chipsets) I started the tool before and after this patch while the Bluetooth chipset was disabled (by pulling it's enable GPIO LOW). In both cases hci0 was not created - thus HCI_UART_INIT_PENDING is not required in that case.

Removing this code also has another benefit: hci_serdev.c does not support the delayed initialization / registration. Thus the protocol implementation (hci_h5) never receives any data with this check still in place. For the H5 protocol this means that the initialization never completes (because the sync response never arrives). Even if the initialization would succeed later on the drivers would call hci_uart_init_ready() which schedules the registration which is currently not implemented by hci_serdev.c.

Removing the HCI_UART_INIT_PENDING check makes the code easier to read and also fixes the initalization of devices (implemented with the serdev library) which use the H5 protocol.

Signed-off-by Martin Blumenstingl <[email protected]>

---

drivers/bluetooth/hci_h5.c     |  3 ---
drivers/bluetooth/hci_ldisc.c  | 38 --------------------------------------
drivers/bluetooth/hci_serdev.c |  3 ---
drivers/bluetooth/hci_uart.h   |  4 +---
4 files changed, 1 insertion(+), 47 deletions(-)

drivers/bluetooth/hci_h5.c +0 -3

index 6a8d0d06aba7..6cfc2f276250 100644
@@ -210,8 +210,6 @@ static int h5_open(struct hci_uart *hu)
 
 	h5->tx_win = H5_TX_WIN_MAX;
 
-	set_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags);
-
 	/* Send initial sync request */
 	h5_link_control(hu, sync, sizeof(sync));
 	mod_timer(&h5->timer, jiffies + H5_SYNC_TIMEOUT);

@@ -316,7 +314,6 @@ static void h5_handle_internal_rx(struct hci_uart *hu)
 			h5->tx_win = (data[2] & 0x07);
 		BT_DBG("Three-wire init complete. tx_win %u", h5->tx_win);
 		h5->state = H5_ACTIVE;
-		hci_uart_init_ready(hu);
 		return;
 	} else if (memcmp(data, sleep_req, 2) == 0) {
 		BT_DBG("Peer went to sleep");

drivers/bluetooth/hci_ldisc.c +0 -38

index c823914b3a80..5dd3e1bebfe4 100644
@@ -195,39 +195,6 @@ static void hci_uart_write_work(struct work_struct *work)
 	clear_bit(HCI_UART_SENDING, &hu->tx_state);
 }
 
-static void hci_uart_init_work(struct work_struct *work)
-{
-	struct hci_uart *hu = container_of(work, struct hci_uart, init_ready);
-	int err;
-	struct hci_dev *hdev;
-
-	if (!test_and_clear_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
-		return;
-
-	err = hci_register_dev(hu->hdev);
-	if (err < 0) {
-		BT_ERR("Can't register HCI device");
-		hdev = hu->hdev;
-		hu->hdev = NULL;
-		hci_free_dev(hdev);
-		clear_bit(HCI_UART_PROTO_READY, &hu->flags);
-		hu->proto->close(hu);
-		return;
-	}
-
-	set_bit(HCI_UART_REGISTERED, &hu->flags);
-}
-
-int hci_uart_init_ready(struct hci_uart *hu)
-{
-	if (!test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
-		return -EALREADY;
-
-	schedule_work(&hu->init_ready);
-
-	return 0;
-}
-
 /* ------- Interface to HCI layer ------ */
 /* Initialize device */
 static int hci_uart_open(struct hci_dev *hdev)

@@ -490,7 +457,6 @@ static int hci_uart_tty_open(struct tty_struct *tty)
 	hu->alignment = 1;
 	hu->padding = 0;
 
-	INIT_WORK(&hu->init_ready, hci_uart_init_work);
 	INIT_WORK(&hu->write_work, hci_uart_write_work);
 
 	percpu_init_rwsem(&hu->proto_lock);

@@ -653,9 +619,6 @@ static int hci_uart_register_dev(struct hci_uart *hu)
 	else
 		hdev->dev_type = HCI_PRIMARY;
 
-	if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
-		return 0;
-
 	if (hci_register_dev(hdev) < 0) {
 		BT_ERR("Can't register HCI device");
 		hu->hdev = NULL;

@@ -699,7 +662,6 @@ static int hci_uart_set_flags(struct hci_uart *hu, unsigned long flags)
 	unsigned long valid_flags = BIT(HCI_UART_RAW_DEVICE) |
 				    BIT(HCI_UART_RESET_ON_INIT) |
 				    BIT(HCI_UART_CREATE_AMP) |
-				    BIT(HCI_UART_INIT_PENDING) |
 				    BIT(HCI_UART_EXT_CONFIG) |
 				    BIT(HCI_UART_VND_DETECT);
 

drivers/bluetooth/hci_serdev.c +0 -3

index e0e6461b9200..fe67eb6d4278 100644
@@ -333,9 +333,6 @@ int hci_uart_register_device(struct hci_uart *hu,
 	else
 		hdev->dev_type = HCI_PRIMARY;
 
-	if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
-		return 0;
-
 	if (hci_register_dev(hdev) < 0) {
 		BT_ERR("Can't register HCI device");
 		err = -ENODEV;

drivers/bluetooth/hci_uart.h +0 -0

index 66e8c68e4607..47e755ff4092 100644

drivers/bluetooth/hci_uart.h +0 -1

 #define HCI_UART_EXT_CONFIG	4
 #define HCI_UART_VND_DETECT	5
 

@@ -83,7 +83,6 @@ struct hci_uart {
 	unsigned long		flags;
 	unsigned long		hdev_flags;
 
-	struct work_struct	init_ready;
 	struct work_struct	write_work;
 
 	const struct hci_uart_proto *proto;

Hi Martin,

Quoted 20 lines

The three-wire (H5) protocol is the only protocol which uses HCI_UART_INIT_PENDING.
Unfortunately the benefits of using this flag are currently unknown. It was added in commit 9f2aee848fe6 ("Bluetooth: Add delayed init sequence support for UART controllers"). In my experiments (with the "rtk_hciattach" tool - a customized version of hciattach for Realtek chipsets) I started the tool before and after this patch while the Bluetooth chipset was disabled (by pulling it's enable GPIO LOW). In both cases hci0 was not created - thus HCI_UART_INIT_PENDING is not required in that case.

Removing this code also has another benefit: hci_serdev.c does not support the delayed initialization / registration. Thus the protocol implementation (hci_h5) never receives any data with this check still in place. For the H5 protocol this means that the initialization never completes (because the sync response never arrives). Even if the initialization would succeed later on the drivers would call hci_uart_init_ready() which schedules the registration which is currently not implemented by hci_serdev.c.

Quoted 5 lines

Removing the HCI_UART_INIT_PENDING check makes the code easier to read and also fixes the initalization of devices (implemented with the serdev library) which use the H5 protocol.

Signed-off-by Martin Blumenstingl <[email protected]>

I really like Johan to ack this one, but generally I am fine with removing unneeded code.

We might also want to look at hciattach to btattach code and make sure it gets removed there as well. I am not even sure anybody used hciattach with H:5 ever.

Quoted 117 lines

---

drivers/bluetooth/hci_h5.c     |  3 ---
drivers/bluetooth/hci_ldisc.c  | 38 --------------------------------------
drivers/bluetooth/hci_serdev.c |  3 ---
drivers/bluetooth/hci_uart.h   |  4 +---
4 files changed, 1 insertion(+), 47 deletions(-)

drivers/bluetooth/hci_h5.c +0 -3

index 6a8d0d06aba7..6cfc2f276250 100644
@@ -210,8 +210,6 @@ static int h5_open(struct hci_uart *hu)
 
      h5->tx_win = H5_TX_WIN_MAX;
 
-     set_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags);
-
      /* Send initial sync request */
      h5_link_control(hu, sync, sizeof(sync));
      mod_timer(&h5->timer, jiffies + H5_SYNC_TIMEOUT);

@@ -316,7 +314,6 @@ static void h5_handle_internal_rx(struct hci_uart *hu)
                      h5->tx_win = (data[2] & 0x07);
              BT_DBG("Three-wire init complete. tx_win %u", h5->tx_win);
              h5->state = H5_ACTIVE;
-             hci_uart_init_ready(hu);
              return;
      } else if (memcmp(data, sleep_req, 2) == 0) {
              BT_DBG("Peer went to sleep");

drivers/bluetooth/hci_ldisc.c +0 -38

index c823914b3a80..5dd3e1bebfe4 100644
@@ -195,39 +195,6 @@ static void hci_uart_write_work(struct work_struct *work)
      clear_bit(HCI_UART_SENDING, &hu->tx_state);
 }
 
-static void hci_uart_init_work(struct work_struct *work)
-{
-     struct hci_uart *hu = container_of(work, struct hci_uart, init_ready);
-     int err;
-     struct hci_dev *hdev;
-
-     if (!test_and_clear_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
-             return;
-
-     err = hci_register_dev(hu->hdev);
-     if (err < 0) {
-             BT_ERR("Can't register HCI device");
-             hdev = hu->hdev;
-             hu->hdev = NULL;
-             hci_free_dev(hdev);
-             clear_bit(HCI_UART_PROTO_READY, &hu->flags);
-             hu->proto->close(hu);
-             return;
-     }
-
-     set_bit(HCI_UART_REGISTERED, &hu->flags);
-}
-
-int hci_uart_init_ready(struct hci_uart *hu)
-{
-     if (!test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
-             return -EALREADY;
-
-     schedule_work(&hu->init_ready);
-
-     return 0;
-}
-
 /* ------- Interface to HCI layer ------ */
 /* Initialize device */
 static int hci_uart_open(struct hci_dev *hdev)

@@ -490,7 +457,6 @@ static int hci_uart_tty_open(struct tty_struct *tty)
      hu->alignment = 1;
      hu->padding = 0;
 
-     INIT_WORK(&hu->init_ready, hci_uart_init_work);
      INIT_WORK(&hu->write_work, hci_uart_write_work);
 
      percpu_init_rwsem(&hu->proto_lock);

@@ -653,9 +619,6 @@ static int hci_uart_register_dev(struct hci_uart *hu)
      else
              hdev->dev_type = HCI_PRIMARY;
 
-     if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
-             return 0;
-
      if (hci_register_dev(hdev) < 0) {
              BT_ERR("Can't register HCI device");
              hu->hdev = NULL;

@@ -699,7 +662,6 @@ static int hci_uart_set_flags(struct hci_uart *hu, unsigned long flags)
      unsigned long valid_flags = BIT(HCI_UART_RAW_DEVICE) |
                                  BIT(HCI_UART_RESET_ON_INIT) |
                                  BIT(HCI_UART_CREATE_AMP) |
-                                 BIT(HCI_UART_INIT_PENDING) |
                                  BIT(HCI_UART_EXT_CONFIG) |
                                  BIT(HCI_UART_VND_DETECT);
 

drivers/bluetooth/hci_serdev.c +0 -3

index e0e6461b9200..fe67eb6d4278 100644
@@ -333,9 +333,6 @@ int hci_uart_register_device(struct hci_uart *hu,
      else
              hdev->dev_type = HCI_PRIMARY;
 
-     if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
-             return 0;
-
      if (hci_register_dev(hdev) < 0) {
              BT_ERR("Can't register HCI device");
              err = -ENODEV;

drivers/bluetooth/hci_uart.h +0 -0

index 66e8c68e4607..47e755ff4092 100644

drivers/bluetooth/hci_uart.h line 56

Quoted 5 lines

drivers/bluetooth/hci_uart.h +1 -1

@@ -53,7 +53,7 @@
 #define HCI_UART_RAW_DEVICE   0
 #define HCI_UART_RESET_ON_INIT        1
 #define HCI_UART_CREATE_AMP   2
-#define HCI_UART_INIT_PENDING        3
+/* 3 is unused - was HCI_UART_INIT_PENDING */

#define HCI_UART_INIT_PENDING 3 /* unused */

I prefer it this way since it is easier on the eyes.

Quoted 14 lines

drivers/bluetooth/hci_uart.h +0 -1

 #define HCI_UART_EXT_CONFIG   4
 #define HCI_UART_VND_DETECT   5
 

@@ -83,7 +83,6 @@ struct hci_uart {
      unsigned long           flags;
      unsigned long           hdev_flags;
 
-     struct work_struct      init_ready;
      struct work_struct      write_work;
 
      const struct hci_uart_proto *proto;

@@ -115,7 +114,6 @@ int hci_uart_register_device(struct hci_uart *hu, const struct hci_uart_proto *p
 void hci_uart_unregister_device(struct hci_uart *hu);
 

drivers/bluetooth/hci_uart.h int hci_uart_register_device(struct hci_uart *hu, const struct hci_uart_proto *p · line 119

Quoted 5 lines

drivers/bluetooth/hci_uart.h +0 -1

 int hci_uart_tx_wakeup(struct hci_uart *hu);
-int hci_uart_init_ready(struct hci_uart *hu);
 void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed);
 void hci_uart_set_flow_control(struct hci_uart *hu, bool enable);
 void hci_uart_set_speeds(struct hci_uart *hu, unsigned int init_speed,

Hi Marcel,

thank you for looking into this latest version!

Quoted 27 lines · Marcel Holtmann

Hi Martin,

Quoted 25 lines

The three-wire (H5) protocol is the only protocol which uses HCI_UART_INIT_PENDING.
Unfortunately the benefits of using this flag are currently unknown. It was added in commit 9f2aee848fe6 ("Bluetooth: Add delayed init sequence support for UART controllers"). In my experiments (with the "rtk_hciattach" tool - a customized version of hciattach for Realtek chipsets) I started the tool before and after this patch while the Bluetooth chipset was disabled (by pulling it's enable GPIO LOW). In both cases hci0 was not created - thus HCI_UART_INIT_PENDING is not required in that case.

Removing this code also has another benefit: hci_serdev.c does not support the delayed initialization / registration. Thus the protocol implementation (hci_h5) never receives any data with this check still in place. For the H5 protocol this means that the initialization never completes (because the sync response never arrives). Even if the initialization would succeed later on the drivers would call hci_uart_init_ready() which schedules the registration which is currently not implemented by hci_serdev.c.

Removing the HCI_UART_INIT_PENDING check makes the code easier to read and also fixes the initalization of devices (implemented with the serdev library) which use the H5 protocol.

Signed-off-by Martin Blumenstingl <[email protected]>

Hi Martin,

Quoted 31 lines · Martin Blumenstingl

Hi Marcel,

thank you for looking into this latest version!

Quoted 26 lines · Marcel Holtmann

Hi Martin,

Quoted 24 lines

The three-wire (H5) protocol is the only protocol which uses HCI_UART_INIT_PENDING.
Unfortunately the benefits of using this flag are currently unknown. It was added in commit 9f2aee848fe6 ("Bluetooth: Add delayed init sequence support for UART controllers"). In my experiments (with the "rtk_hciattach" tool - a customized version of hciattach for Realtek chipsets) I started the tool before and after this patch while the Bluetooth chipset was disabled (by pulling it's enable GPIO LOW). In both cases hci0 was not created - thus HCI_UART_INIT_PENDING is not required in that case.

Removing this code also has another benefit: hci_serdev.c does not support the delayed initialization / registration. Thus the protocol implementation (hci_h5) never receives any data with this check still in place. For the H5 protocol this means that the initialization never completes (because the sync response never arrives). Even if the initialization would succeed later on the drivers would call hci_uart_init_ready() which schedules the registration which is currently not implemented by hci_serdev.c.

Removing the HCI_UART_INIT_PENDING check makes the code easier to read and also fixes the initalization of devices (implemented with the serdev library) which use the H5 protocol.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Signed-off-by Martin Blumenstingl <[email protected]>

I think the original goal is to perform H5 init peacefully. The H5 protocol needs to be open in order to send/receive H5 link packets during the H5 initialization/synchronization step. During this stage, driver prevents upper stack to send any HCI packet by delaying the HCI device registration.

The three-wire (H5) protocol is the only protocol which uses HCI_UART_INIT_PENDING.

Unfortunately the benefits of using this flag are currently unknown. It was added in commit 9f2aee848fe6 ("Bluetooth: Add delayed init sequence support for UART controllers"). In my experiments (with the "rtk_hciattach" tool - a customized version of hciattach for Realtek chipsets) I started the tool before and after this patch while the Bluetooth chipset was disabled (by pulling it's enable GPIO LOW). In both cases hci0 was not created - thus HCI_UART_INIT_PENDING is not required in that case.

Removing this code also has another benefit: hci_serdev.c does not support the delayed initialization / registration.

Thus the protocol
implementation (hci_h5) never receives any data with this check still in place. For the H5 protocol this means that the initialization never completes (because the sync response never arrives). Even if the initialization would succeed later on the drivers would call hci_uart_init_ready() which schedules the registration which is currently not implemented by hci_serdev.c.

Removing the HCI_UART_INIT_PENDING check makes the code easier to read and also fixes the initalization of devices (implemented with the serdev library) which use the H5 protocol.

Signed-off-by Martin Blumenstingl <[email protected]>

---
 drivers/bluetooth/hci_h5.c     |  3 ---
 drivers/bluetooth/hci_ldisc.c  | 38 --------------------------------------
 drivers/bluetooth/hci_serdev.c |  3 ---
 drivers/bluetooth/hci_uart.h   |  4 +---
 4 files changed, 1 insertion(+), 47 deletions(-)

Quoted 2 lines · Martin Blumenstingl

The three-wire (H5) protocol is the only protocol which uses HCI_UART_INIT_PENDING.

I think I'd also found that this flag wasn't really needed.

Quoted 9 lines

Unfortunately the benefits of using this flag are currently unknown. It was added in commit 9f2aee848fe6 ("Bluetooth: Add delayed init sequence support for UART controllers"). In my experiments (with the "rtk_hciattach" tool - a customized version of hciattach for Realtek chipsets) I started the tool before and after this patch while the Bluetooth chipset was disabled (by pulling it's enable GPIO LOW). In both cases hci0 was not created - thus HCI_UART_INIT_PENDING is not required in that case.

Quoted 2 lines

Removing this code also has another benefit: hci_serdev.c does not support the delayed initialization / registration.

This statement is misleading. serdev *always* supports async init as it forces async probe of drivers. It doesn't need to support the private workq init mechanism. At least that was my intent.

Quoted 12 lines

Thus the protocol
implementation (hci_h5) never receives any data with this check still in place. For the H5 protocol this means that the initialization never completes (because the sync response never arrives). Even if the initialization would succeed later on the drivers would call hci_uart_init_ready() which schedules the registration which is currently not implemented by hci_serdev.c.

Removing the HCI_UART_INIT_PENDING check makes the code easier to read and also fixes the initalization of devices (implemented with the serdev library) which use the H5 protocol.

Quoted 7 lines

Signed-off-by Martin Blumenstingl <[email protected]>

---
 drivers/bluetooth/hci_h5.c     |  3 ---
 drivers/bluetooth/hci_ldisc.c  | 38 --------------------------------------
 drivers/bluetooth/hci_serdev.c |  3 ---
 drivers/bluetooth/hci_uart.h   |  4 +---
 4 files changed, 1 insertion(+), 47 deletions(-)

Realtek RTL8723BS and RTL8723DS are SDIO wifi chips with an embedded Bluetooth controller which connects to the host via UART. The H5 protocol is used for communication between host and device.

The Realtek "rtl8723bs_bt" and "rtl8723ds_bt" userspace Bluetooth UART initialization tools (rtk_hciattach) use the following sequence:
1) send H5 sync pattern (already supported by hci_h5)
2) get LMP version (already supported by btrtl)
3) get ROM version (already supported by btrtl)
4) load the firmware and config for the current chipset (already supported by btrtl)
5) read UART settings from the config blob (already supported by btrtl)
6) send UART settings via a vendor command to the device (which changes the baudrate of the device and enables or disables flow control depending on the config)
7) change the baudrate and flow control settings on the host
8) send the firmware and config blob to the device (already supported by btrtl)

This uses the serdev library as well as the existing btrtl driver to initialize the Bluetooth functionality, which consists of:
- identifying the device and loading the corresponding firmware and config blobs (steps #2, #3 and #4)
- configuring the baudrate and flow control (steps #6 and #7)
- uploading the firmware to the device (step #8)

Signed-off-by Martin Blumenstingl <[email protected]>

---
drivers/bluetooth/Kconfig | 1 +
drivers/bluetooth/hci_h5.c | 205 +++++++++++++++++++++++++++++++++++++++++++++

2 files changed, 206 insertions(+)

drivers/bluetooth/Kconfig +0 -0

index 60e1c7d6986d..3001f1200c72 100644
@@ -146,6 +146,7 @@ config BT_HCIUART_LL

config BT_HCIUART_3WIRE
bool "Three-wire UART (H5) protocol support" depends on BT_HCIUART

drivers/bluetooth/Kconfig +1 -0

+	select BT_RTL if SERIAL_DEV_BUS

help
The HCI Three-wire UART Transport Layer makes it possible to user the Bluetooth HCI over a serial port interface. The HCI

drivers/bluetooth/hci_h5.c +139 -0

index 6cfc2f276250..a03acc3b1b52 100644
@@ -28,7 +28,14 @@
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/serdev.h>
+
 #include "hci_uart.h"
+#include "btrtl.h"
 
 #define HCI_3WIRE_ACK_PKT	0
 #define HCI_3WIRE_LINK_PKT	15

@@ -97,6 +104,13 @@ struct h5 {
 	} sleep;
 };
 
+struct h5_device {
+	struct hci_uart hu;
+	struct gpio_desc *enable_gpio;
+	struct gpio_desc *reset_gpio;
+	int (*vendor_setup)(struct h5_device *h5_dev);
+};
+
 static void h5_reset_rx(struct h5 *h5);
 
 static void h5_link_control(struct hci_uart *hu, const void *data, size_t len)

@@ -190,6 +204,7 @@ static int h5_open(struct hci_uart *hu)
 {
 	struct h5 *h5;
 	const unsigned char sync[] = { 0x01, 0x7e };
+	int err;
 
 	BT_DBG("hu %p", hu);
 

@@ -210,6 +225,14 @@ static int h5_open(struct hci_uart *hu)
 
 	h5->tx_win = H5_TX_WIN_MAX;
 
+	if (hu->serdev) {
+		err = serdev_device_open(hu->serdev);
+		if (err) {
+			bt_dev_err(hu->hdev, "failed to open serdev: %d", err);
+			return err;
+		}
+	}
+
 	/* Send initial sync request */
 	h5_link_control(hu, sync, sizeof(sync));
 	mod_timer(&h5->timer, jiffies + H5_SYNC_TIMEOUT);

@@ -217,6 +240,25 @@ static int h5_open(struct hci_uart *hu)
 	return 0;
 }
 
+static int h5_setup(struct hci_uart *hu)
+{
+	int err;
+	struct h5_device *h5_dev;
+
+	if (!hu->serdev)
+		return 0;
+
+	h5_dev = serdev_device_get_drvdata(hu->serdev);
+
+	if (h5_dev->vendor_setup) {
+		err = h5_dev->vendor_setup(h5_dev);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static int h5_close(struct hci_uart *hu)
 {
 	struct h5 *h5 = hu->priv;

@@ -227,6 +269,15 @@ static int h5_close(struct hci_uart *hu)
 	skb_queue_purge(&h5->rel);
 	skb_queue_purge(&h5->unrel);
 
+	if (hu->serdev) {
+		struct h5_device *h5_dev;
+
+		h5_dev = serdev_device_get_drvdata(hu->serdev);
+		gpiod_set_value_cansleep(h5_dev->enable_gpio, 0);
+
+		serdev_device_close(hu->serdev);
+	}
+
 	kfree(h5);
 
 	return 0;

@@ -736,10 +787,160 @@ static int h5_flush(struct hci_uart *hu)
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_SERIAL_DEV_BUS)
+static int h5_setup_realtek(struct h5_device *h5_dev)
+{
+	struct hci_uart *hu = &h5_dev->hu;
+	int err = 0, retry = 3;
+	struct sk_buff *skb;
+	struct btrtl_device_info *btrtl_dev;
+	__le32 baudrate_data;
+	u32 device_baudrate;
+	unsigned int controller_baudrate;
+	bool flow_control;
+
+	/* devices always start with flow control disabled and even parity */
+	serdev_device_set_flow_control(hu->serdev, false);
+	serdev_device_set_parity(hu->serdev, true, false);
+
+	do {
+		/* disable the device and put it into reset. some devices only
+		 * have one of these lines, so we toggle both here to support
+		 * all combinations.
+		 */
+		gpiod_set_value_cansleep(h5_dev->reset_gpio, 1);
+		gpiod_set_value_cansleep(h5_dev->enable_gpio, 0);
+
+		/* wait until the device is disabled and/or reset. 500ms are
+		 * chosen by manually testing on a RTL8723BS. shorter wait
+		 * times lead to a non-responding device.
+		 */
+		msleep(500);
+
+		/* take the device out of reset and enable it. */
+		gpiod_set_value_cansleep(h5_dev->reset_gpio, 0);
+		gpiod_set_value_cansleep(h5_dev->enable_gpio, 1);
+
+		/* after that we need to wait 500ms, otherwise the device might
+		 * not respond in all cases. this was determined by testing
+		 * with a RTL8723BS.
+		 */
+		msleep(500);
+
+		btrtl_dev = btrtl_initialize(hu->hdev);
+		if (!IS_ERR(btrtl_dev))
+			break;
+
+		/* Toggle the enable and reset pins above and try again */
+	} while (retry--);
+
+	if (IS_ERR(btrtl_dev))
+		return PTR_ERR(btrtl_dev);
+
+	err = btrtl_get_uart_settings(hu->hdev, btrtl_dev,
+				      &controller_baudrate, &device_baudrate,
+				      &flow_control);
+	if (err)
+		goto out_free;
+
+	baudrate_data = cpu_to_le32(device_baudrate);
+	skb = __hci_cmd_sync(hu->hdev, 0xfc17, sizeof(baudrate_data),
+			     &baudrate_data, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		bt_dev_err(hu->hdev, "set baud rate command failed");
+		err = -PTR_ERR(skb);
+		goto out_free;
+	} else {
+		kfree_skb(skb);
+	}
+
+	serdev_device_set_baudrate(hu->serdev, controller_baudrate);
+	serdev_device_set_flow_control(hu->serdev, flow_control);
+
+	err = btrtl_download_firmware(hu->hdev, btrtl_dev);
+
+out_free:
+	btrtl_free(btrtl_dev);
+
+	return err;
+}
+
+static const struct hci_uart_proto h5p;
+
+static int hci_h5_probe(struct serdev_device *serdev)
+{
+	struct hci_uart *hu;
+	struct h5_device *h5_dev;
+
+	h5_dev = devm_kzalloc(&serdev->dev, sizeof(*h5_dev), GFP_KERNEL);
+	if (!h5_dev)
+		return -ENOMEM;

drivers/bluetooth/hci_h5.c +53 -0

+	if (IS_ERR(h5_dev->enable_gpio))
+		return PTR_ERR(h5_dev->enable_gpio);
+
+	h5_dev->reset_gpio = devm_gpiod_get_optional(&serdev->dev, "reset",
+						     GPIOD_OUT_LOW);
+	if (IS_ERR(h5_dev->reset_gpio))
+		return PTR_ERR(h5_dev->reset_gpio);
+
+	hci_uart_set_speeds(hu, 115200, 0);
+
+	return hci_uart_register_device(hu, &h5p);
+}
+
+static void hci_h5_remove(struct serdev_device *serdev)
+{
+	struct h5_device *h5_dev = serdev_device_get_drvdata(serdev);
+	struct hci_uart *hu = &h5_dev->hu;
+	struct hci_dev *hdev = hu->hdev;
+
+	cancel_work_sync(&hu->write_work);
+
+	hci_unregister_dev(hdev);
+	hci_free_dev(hdev);
+	hu->proto->close(hu);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id hci_h5_of_match[] = {
+	{
+		.compatible = "realtek,rtl8723bs-bluetooth",
+		.data = h5_setup_realtek
+	},
+	{
+		.compatible = "realtek,rtl8723ds-bluetooth",
+		.data = h5_setup_realtek
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, hci_h5_of_match);
+#endif
+
+static struct serdev_device_driver hci_h5_drv = {
+	.driver		= {
+		.name	= "hci-h5",
+		.of_match_table = of_match_ptr(hci_h5_of_match),
+	},
+	.probe	= hci_h5_probe,
+	.remove	= hci_h5_remove,
+};
+#endif
+
 static const struct hci_uart_proto h5p = {
 	.id		= HCI_UART_3WIRE,
 	.name		= "Three-wire (H5)",
 	.open		= h5_open,
+	.setup		= h5_setup,
 	.close		= h5_close,
 	.recv		= h5_recv,
 	.enqueue	= h5_enqueue,

@@ -749,10 +950,14 @@ static const struct hci_uart_proto h5p = {
 
 int __init h5_init(void)
 {
+	serdev_device_driver_register(&hci_h5_drv);

Hi Martin,

Quoted 32 lines

Realtek RTL8723BS and RTL8723DS are SDIO wifi chips with an embedded Bluetooth controller which connects to the host via UART. The H5 protocol is used for communication between host and device.

The Realtek "rtl8723bs_bt" and "rtl8723ds_bt" userspace Bluetooth UART initialization tools (rtk_hciattach) use the following sequence:
1) send H5 sync pattern (already supported by hci_h5)
2) get LMP version (already supported by btrtl)
3) get ROM version (already supported by btrtl)
4) load the firmware and config for the current chipset (already supported by btrtl)
5) read UART settings from the config blob (already supported by btrtl)
6) send UART settings via a vendor command to the device (which changes the baudrate of the device and enables or disables flow control depending on the config)
7) change the baudrate and flow control settings on the host
8) send the firmware and config blob to the device (already supported by btrtl)

This uses the serdev library as well as the existing btrtl driver to initialize the Bluetooth functionality, which consists of:
- identifying the device and loading the corresponding firmware and config blobs (steps #2, #3 and #4)
- configuring the baudrate and flow control (steps #6 and #7)
- uploading the firmware to the device (step #8)

Signed-off-by Martin Blumenstingl <[email protected]>

---
drivers/bluetooth/Kconfig | 1 +
drivers/bluetooth/hci_h5.c | 205 +++++++++++++++++++++++++++++++++++++++++++++

2 files changed, 206 insertions(+)

Quoted 5 lines

drivers/bluetooth/Kconfig +0 -0

index 60e1c7d6986d..3001f1200c72 100644
@@ -146,6 +146,7 @@ config BT_HCIUART_LL

Quoted 7 lines

config BT_HCIUART_3WIRE
bool "Three-wire UART (H5) protocol support" depends on BT_HCIUART
+ select BT_RTL if SERIAL_DEV_BUS
help
The HCI Three-wire UART Transport Layer makes it possible to user the Bluetooth HCI over a serial port interface. The HCI

while this is fine initially, I think long term this is not sustainable. So we need to abstract the H:5 part and the vendor specific setup part so that you can have a hci_rtl.c that uses H:5 instead of H:4 and still is as tiny and simple as our H:4 drivers.

One other think I am not really happy about here is the Kconfig entry itself. You need to know that you need 3WIRE and SERDEV to magically enable RTL UART support. Maybe it would be better to just have a CONFIG_HCIUART_RTL and duplicate the config entry just specific to RTL. It then can select RTL, but in the Makefile it will result in using the same hci_h5.c file.

Quoted 194 lines

drivers/bluetooth/hci_h5.c +143 -0

index 6cfc2f276250..a03acc3b1b52 100644
@@ -28,7 +28,14 @@
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/serdev.h>
+
 #include "hci_uart.h"
+#include "btrtl.h"
 
 #define HCI_3WIRE_ACK_PKT     0
 #define HCI_3WIRE_LINK_PKT    15

@@ -97,6 +104,13 @@ struct h5 {
      } sleep;
 };
 
+struct h5_device {
+     struct hci_uart hu;
+     struct gpio_desc *enable_gpio;
+     struct gpio_desc *reset_gpio;
+     int (*vendor_setup)(struct h5_device *h5_dev);
+};
+
 static void h5_reset_rx(struct h5 *h5);
 
 static void h5_link_control(struct hci_uart *hu, const void *data, size_t len)

@@ -190,6 +204,7 @@ static int h5_open(struct hci_uart *hu)
 {
      struct h5 *h5;
      const unsigned char sync[] = { 0x01, 0x7e };
+     int err;
 
      BT_DBG("hu %p", hu);
 

@@ -210,6 +225,14 @@ static int h5_open(struct hci_uart *hu)
 
      h5->tx_win = H5_TX_WIN_MAX;
 
+     if (hu->serdev) {
+             err = serdev_device_open(hu->serdev);
+             if (err) {
+                     bt_dev_err(hu->hdev, "failed to open serdev: %d", err);
+                     return err;
+             }
+     }
+
      /* Send initial sync request */
      h5_link_control(hu, sync, sizeof(sync));
      mod_timer(&h5->timer, jiffies + H5_SYNC_TIMEOUT);

@@ -217,6 +240,25 @@ static int h5_open(struct hci_uart *hu)
      return 0;
 }
 
+static int h5_setup(struct hci_uart *hu)
+{
+     int err;
+     struct h5_device *h5_dev;
+
+     if (!hu->serdev)
+             return 0;
+
+     h5_dev = serdev_device_get_drvdata(hu->serdev);
+
+     if (h5_dev->vendor_setup) {
+             err = h5_dev->vendor_setup(h5_dev);
+             if (err)
+                     return err;
+     }
+
+     return 0;
+}
+
 static int h5_close(struct hci_uart *hu)
 {
      struct h5 *h5 = hu->priv;

@@ -227,6 +269,15 @@ static int h5_close(struct hci_uart *hu)
      skb_queue_purge(&h5->rel);
      skb_queue_purge(&h5->unrel);
 
+     if (hu->serdev) {
+             struct h5_device *h5_dev;
+
+             h5_dev = serdev_device_get_drvdata(hu->serdev);
+             gpiod_set_value_cansleep(h5_dev->enable_gpio, 0);
+
+             serdev_device_close(hu->serdev);
+     }
+
      kfree(h5);
 
      return 0;

@@ -736,10 +787,160 @@ static int h5_flush(struct hci_uart *hu)
      return 0;
 }
 
+#if IS_ENABLED(CONFIG_SERIAL_DEV_BUS)
+static int h5_setup_realtek(struct h5_device *h5_dev)
+{
+     struct hci_uart *hu = &h5_dev->hu;
+     int err = 0, retry = 3;
+     struct sk_buff *skb;
+     struct btrtl_device_info *btrtl_dev;
+     __le32 baudrate_data;
+     u32 device_baudrate;
+     unsigned int controller_baudrate;
+     bool flow_control;
+
+     /* devices always start with flow control disabled and even parity */
+     serdev_device_set_flow_control(hu->serdev, false);
+     serdev_device_set_parity(hu->serdev, true, false);
+
+     do {
+             /* disable the device and put it into reset. some devices only
+              * have one of these lines, so we toggle both here to support
+              * all combinations.
+              */
+             gpiod_set_value_cansleep(h5_dev->reset_gpio, 1);
+             gpiod_set_value_cansleep(h5_dev->enable_gpio, 0);
+
+             /* wait until the device is disabled and/or reset. 500ms are
+              * chosen by manually testing on a RTL8723BS. shorter wait
+              * times lead to a non-responding device.
+              */
+             msleep(500);
+
+             /* take the device out of reset and enable it. */
+             gpiod_set_value_cansleep(h5_dev->reset_gpio, 0);
+             gpiod_set_value_cansleep(h5_dev->enable_gpio, 1);
+
+             /* after that we need to wait 500ms, otherwise the device might
+              * not respond in all cases. this was determined by testing
+              * with a RTL8723BS.
+              */
+             msleep(500);
+
+             btrtl_dev = btrtl_initialize(hu->hdev);
+             if (!IS_ERR(btrtl_dev))
+                     break;
+
+             /* Toggle the enable and reset pins above and try again */
+     } while (retry--);
+
+     if (IS_ERR(btrtl_dev))
+             return PTR_ERR(btrtl_dev);
+
+     err = btrtl_get_uart_settings(hu->hdev, btrtl_dev,
+                                   &controller_baudrate, &device_baudrate,
+                                   &flow_control);
+     if (err)
+             goto out_free;
+
+     baudrate_data = cpu_to_le32(device_baudrate);
+     skb = __hci_cmd_sync(hu->hdev, 0xfc17, sizeof(baudrate_data),
+                          &baudrate_data, HCI_INIT_TIMEOUT);
+     if (IS_ERR(skb)) {
+             bt_dev_err(hu->hdev, "set baud rate command failed");
+             err = -PTR_ERR(skb);
+             goto out_free;
+     } else {
+             kfree_skb(skb);
+     }
+
+     serdev_device_set_baudrate(hu->serdev, controller_baudrate);
+     serdev_device_set_flow_control(hu->serdev, flow_control);
+
+     err = btrtl_download_firmware(hu->hdev, btrtl_dev);
+
+out_free:
+     btrtl_free(btrtl_dev);
+
+     return err;
+}
+
+static const struct hci_uart_proto h5p;
+
+static int hci_h5_probe(struct serdev_device *serdev)
+{
+     struct hci_uart *hu;
+     struct h5_device *h5_dev;
+
+     h5_dev = devm_kzalloc(&serdev->dev, sizeof(*h5_dev), GFP_KERNEL);
+     if (!h5_dev)
+             return -ENOMEM;
+
+     hu = &h5_dev->hu;
+     hu->serdev = serdev;
+

drivers/bluetooth/hci_h5.c static int h5_flush(struct hci_uart *hu) · line 887

Quoted 6 lines

drivers/bluetooth/hci_h5.c +6 -0

+     serdev_device_set_drvdata(serdev, h5_dev);
+
+     h5_dev->vendor_setup = of_device_get_match_data(&serdev->dev);
+
+     h5_dev->enable_gpio = devm_gpiod_get_optional(&serdev->dev, "enable",
+                                                    GPIOD_OUT_HIGH);

Indentation mistake.

Quoted 68 lines

drivers/bluetooth/hci_h5.c +54 -0

+     if (IS_ERR(h5_dev->enable_gpio))
+             return PTR_ERR(h5_dev->enable_gpio);
+
+     h5_dev->reset_gpio = devm_gpiod_get_optional(&serdev->dev, "reset",
+                                                  GPIOD_OUT_LOW);
+     if (IS_ERR(h5_dev->reset_gpio))
+             return PTR_ERR(h5_dev->reset_gpio);
+
+     hci_uart_set_speeds(hu, 115200, 0);
+
+     return hci_uart_register_device(hu, &h5p);
+}
+
+static void hci_h5_remove(struct serdev_device *serdev)
+{
+     struct h5_device *h5_dev = serdev_device_get_drvdata(serdev);
+     struct hci_uart *hu = &h5_dev->hu;
+     struct hci_dev *hdev = hu->hdev;
+
+     cancel_work_sync(&hu->write_work);
+
+     hci_unregister_dev(hdev);
+     hci_free_dev(hdev);
+     hu->proto->close(hu);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id hci_h5_of_match[] = {
+     {
+             .compatible = "realtek,rtl8723bs-bluetooth",
+             .data = h5_setup_realtek
+     },
+     {
+             .compatible = "realtek,rtl8723ds-bluetooth",
+             .data = h5_setup_realtek
+     },
+     {},
+};
+MODULE_DEVICE_TABLE(of, hci_h5_of_match);
+#endif
+
+static struct serdev_device_driver hci_h5_drv = {
+     .driver         = {
+             .name   = "hci-h5",
+             .of_match_table = of_match_ptr(hci_h5_of_match),
+     },
+     .probe  = hci_h5_probe,
+     .remove = hci_h5_remove,
+};
+#endif
+
 static const struct hci_uart_proto h5p = {
      .id             = HCI_UART_3WIRE,
      .name           = "Three-wire (H5)",
      .open           = h5_open,
+     .setup          = h5_setup,
      .close          = h5_close,
      .recv           = h5_recv,
      .enqueue        = h5_enqueue,

@@ -749,10 +950,14 @@ static const struct hci_uart_proto h5p = {
 
 int __init h5_init(void)
 {
+     serdev_device_driver_register(&hci_h5_drv);
+
      return hci_uart_register_proto(&h5p);
 }
 

drivers/bluetooth/hci_h5.c static const struct hci_uart_proto h5p = { · line 963

Quoted 6 lines

drivers/bluetooth/hci_h5.c +2 -0

 int __exit h5_deinit(void)
 {
+     serdev_device_driver_unregister(&hci_h5_drv);
+
      return hci_uart_unregister_proto(&h5p);
 }