[PATCH 00/13] Bluetooth: Add RTL8723BS support

[PATCH 00/13] Bluetooth: Add RTL8723BS support

[Hans de Goede]

Hi All,

Here is a series of patches adding support for RTL8723BS' bluetooth
component through serdev enumration.

This builds on the great work done on this by Martin Blumenstingl
which makes the btrtl.c code also support firmware+config loading
for the RTL8723BS, as well as Jeremy's Cline work to add ACPI
enumeration support on top of this.

I've tested this series on several Intel boards with a RTL8723BS
chip. As such this series only hooks up ACPI enumerated devices.

Adding support for devicetree enumeration using the bindings already
defined for this by Martin should be trivial for someone who has
the necessary hardware to develop this.

Regards,

Hans

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

[Hans de Goede]

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

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

[PATCH 02/13] Bluetooth: btrtl: add MODULE_FIRMWARE declarations

[Hans de Goede]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

This makes the firmware names show up in modinfo.

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>
---
 drivers/bluetooth/btrtl.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 437f080deaab..c08f63e3bc14 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -528,3 +528,12 @@ MODULE_AUTHOR("Daniel Drake <drake@endlessm.com>");
 MODULE_DESCRIPTION("Bluetooth support for Realtek devices ver " VERSION);
 MODULE_VERSION(VERSION);
 MODULE_LICENSE("GPL");
+MODULE_FIRMWARE("rtl_bt/rtl8723a_fw.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8723b_fw.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8723b_config.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8761a_fw.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8761a_config.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8821a_fw.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8821a_config.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8822b_fw.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8822b_config.bin");
-- 
2.17.0

[PATCH 03/13] Bluetooth: btrtl: split the device initialization into smaller parts

[Hans de Goede]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

This prepares the btrtl code so it can be used to initialize Bluetooth
modules connected via UART (these are found for example on the RTL8723BS
and RTL8723DS SDIO chips, which come with an embedded UART Bluetooth
module).

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 (currently not supported)
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)

The main reason why the initialization has to be split is step #7. This
requires changes to the underlying "bus", which should be kept outside
of the "generic" btrtl driver.
The idea for this split is borrowed from the btbcm driver but adjusted
where needed (the btrtl driver for example needs two blobs: firmware and
config, while the btbcm only needs one).

This also prepares the code for step #5 (parsing the config blob) by
centralizing the code which loads the firmware and config blobs and
storing the result in the new struct btrtl_device_info.

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>
---
 drivers/bluetooth/btrtl.c | 278 +++++++++++++++++++++++---------------
 drivers/bluetooth/btrtl.h |  21 +++
 2 files changed, 193 insertions(+), 106 deletions(-)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index c08f63e3bc14..d7b44338cc8a 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -47,48 +47,96 @@ struct id_table {
 	__u16 lmp_subver;
 	__u16 hci_rev;
 	bool config_needed;
+	bool has_rom_version;
 	char *fw_name;
 	char *cfg_name;
 };
 
+struct btrtl_device_info {
+	const struct id_table *ic_info;
+	u8 rom_version;
+	u8 *fw_data;
+	int fw_len;
+	u8 *cfg_data;
+	int cfg_len;
+};
+
 static const struct id_table ic_id_table[] = {
+	{ IC_MATCH_FL_LMPSUBV, RTL_ROM_LMP_8723A, 0x0,
+	  .config_needed = false,
+	  .has_rom_version = false,
+	  .fw_name = "rtl_bt/rtl8723a_fw.bin",
+	  .cfg_name = NULL },
+
+	{ IC_MATCH_FL_LMPSUBV, RTL_ROM_LMP_3499, 0x0,
+	  .config_needed = false,
+	  .has_rom_version = false,
+	  .fw_name = "rtl_bt/rtl8723a_fw.bin",
+	  .cfg_name = NULL },
+
 	/* 8723B */
 	{ IC_INFO(RTL_ROM_LMP_8723B, 0xb),
 	  .config_needed = false,
+	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8723b_fw.bin",
 	  .cfg_name = "rtl_bt/rtl8723b_config.bin" },
 
 	/* 8723D */
 	{ IC_INFO(RTL_ROM_LMP_8723B, 0xd),
 	  .config_needed = true,
+	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8723d_fw.bin",
 	  .cfg_name = "rtl_bt/rtl8723d_config.bin" },
 
 	/* 8821A */
 	{ IC_INFO(RTL_ROM_LMP_8821A, 0xa),
 	  .config_needed = false,
+	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8821a_fw.bin",
 	  .cfg_name = "rtl_bt/rtl8821a_config.bin" },
 
 	/* 8821C */
 	{ IC_INFO(RTL_ROM_LMP_8821A, 0xc),
 	  .config_needed = false,
+	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8821c_fw.bin",
 	  .cfg_name = "rtl_bt/rtl8821c_config.bin" },
 
 	/* 8761A */
 	{ IC_MATCH_FL_LMPSUBV, RTL_ROM_LMP_8761A, 0x0,
 	  .config_needed = false,
+	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8761a_fw.bin",
 	  .cfg_name = "rtl_bt/rtl8761a_config.bin" },
 
 	/* 8822B */
 	{ IC_INFO(RTL_ROM_LMP_8822B, 0xb),
 	  .config_needed = true,
+	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8822b_fw.bin",
 	  .cfg_name = "rtl_bt/rtl8822b_config.bin" },
 	};
 
+static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ic_id_table); i++) {
+		if ((ic_id_table[i].match_flags & IC_MATCH_FL_LMPSUBV) &&
+		    (ic_id_table[i].lmp_subver != lmp_subver))
+			continue;
+		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIREV) &&
+		    (ic_id_table[i].hci_rev != hci_rev))
+			continue;
+
+		break;
+	}
+	if (i >= ARRAY_SIZE(ic_id_table))
+		return NULL;
+
+	return &ic_id_table[i];
+}
+
 static int rtl_read_rom_version(struct hci_dev *hdev, u8 *version)
 {
 	struct rtl_rom_version_evt *rom_version;
@@ -118,16 +166,16 @@ static int rtl_read_rom_version(struct hci_dev *hdev, u8 *version)
 	return 0;
 }
 
-static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
-				const struct firmware *fw,
+static int rtlbt_parse_firmware(struct hci_dev *hdev,
+				struct btrtl_device_info *btrtl_dev,
 				unsigned char **_buf)
 {
 	const u8 extension_sig[] = { 0x51, 0x04, 0xfd, 0x77 };
 	struct rtl_epatch_header *epatch_info;
 	unsigned char *buf;
-	int i, ret, len;
+	int i, len;
 	size_t min_size;
-	u8 opcode, length, data, rom_version = 0;
+	u8 opcode, length, data;
 	int project_id = -1;
 	const unsigned char *fwptr, *chip_id_base;
 	const unsigned char *patch_length_base, *patch_offset_base;
@@ -146,15 +194,11 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
 		{ RTL_ROM_LMP_8821A, 10 },	/* 8821C */
 	};
 
-	ret = rtl_read_rom_version(hdev, &rom_version);
-	if (ret)
-		return ret;
-
 	min_size = sizeof(struct rtl_epatch_header) + sizeof(extension_sig) + 3;
-	if (fw->size < min_size)
+	if (btrtl_dev->fw_len < min_size)
 		return -EINVAL;
 
-	fwptr = fw->data + fw->size - sizeof(extension_sig);
+	fwptr = btrtl_dev->fw_data + btrtl_dev->fw_len - sizeof(extension_sig);
 	if (memcmp(fwptr, extension_sig, sizeof(extension_sig)) != 0) {
 		BT_ERR("%s: extension section signature mismatch", hdev->name);
 		return -EINVAL;
@@ -166,7 +210,7 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
 	 * Once we have that, we double-check that that project_id is suitable
 	 * for the hardware we are working with.
 	 */
-	while (fwptr >= fw->data + (sizeof(struct rtl_epatch_header) + 3)) {
+	while (fwptr >= btrtl_dev->fw_data + (sizeof(*epatch_info) + 3)) {
 		opcode = *--fwptr;
 		length = *--fwptr;
 		data = *--fwptr;
@@ -206,13 +250,14 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
 		return -EINVAL;
 	}
 
-	if (lmp_subver != project_id_to_lmp_subver[i].lmp_subver) {
+	if (btrtl_dev->ic_info->lmp_subver != project_id_to_lmp_subver[i].lmp_subver) {
 		BT_ERR("%s: firmware is for %x but this is a %x", hdev->name,
-		       project_id_to_lmp_subver[i].lmp_subver, lmp_subver);
+		       project_id_to_lmp_subver[i].lmp_subver,
+		       btrtl_dev->ic_info->lmp_subver);
 		return -EINVAL;
 	}
 
-	epatch_info = (struct rtl_epatch_header *)fw->data;
+	epatch_info = (struct rtl_epatch_header *)btrtl_dev->fw_data;
 	if (memcmp(epatch_info->signature, RTL_EPATCH_SIGNATURE, 8) != 0) {
 		BT_ERR("%s: bad EPATCH signature", hdev->name);
 		return -EINVAL;
@@ -229,16 +274,16 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
 	 * Find the right patch for this chip.
 	 */
 	min_size += 8 * num_patches;
-	if (fw->size < min_size)
+	if (btrtl_dev->fw_len < min_size)
 		return -EINVAL;
 
-	chip_id_base = fw->data + sizeof(struct rtl_epatch_header);
+	chip_id_base = btrtl_dev->fw_data + sizeof(struct rtl_epatch_header);
 	patch_length_base = chip_id_base + (sizeof(u16) * num_patches);
 	patch_offset_base = patch_length_base + (sizeof(u16) * num_patches);
 	for (i = 0; i < num_patches; i++) {
 		u16 chip_id = get_unaligned_le16(chip_id_base +
 						 (i * sizeof(u16)));
-		if (chip_id == rom_version + 1) {
+		if (chip_id == btrtl_dev->rom_version + 1) {
 			patch_length = get_unaligned_le16(patch_length_base +
 							  (i * sizeof(u16)));
 			patch_offset = get_unaligned_le32(patch_offset_base +
@@ -249,20 +294,21 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
 
 	if (!patch_offset) {
 		BT_ERR("%s: didn't find patch for chip id %d",
-		       hdev->name, rom_version);
+		       hdev->name, btrtl_dev->rom_version);
 		return -EINVAL;
 	}
 
 	BT_DBG("length=%x offset=%x index %d", patch_length, patch_offset, i);
 	min_size = patch_offset + patch_length;
-	if (fw->size < min_size)
+	if (btrtl_dev->fw_len < min_size)
 		return -EINVAL;
 
 	/* Copy the firmware into a new buffer and write the version at
 	 * the end.
 	 */
 	len = patch_length;
-	buf = kmemdup(fw->data + patch_offset, patch_length, GFP_KERNEL);
+	buf = kmemdup(btrtl_dev->fw_data + patch_offset, patch_length,
+		      GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
@@ -324,7 +370,7 @@ static int rtl_download_firmware(struct hci_dev *hdev,
 	return ret;
 }
 
-static int rtl_load_config(struct hci_dev *hdev, const char *name, u8 **buff)
+static int rtl_load_file(struct hci_dev *hdev, const char *name, u8 **buff)
 {
 	const struct firmware *fw;
 	int ret;
@@ -343,96 +389,37 @@ static int rtl_load_config(struct hci_dev *hdev, const char *name, u8 **buff)
 	return ret;
 }
 
-static int btrtl_setup_rtl8723a(struct hci_dev *hdev)
+static int btrtl_setup_rtl8723a(struct hci_dev *hdev,
+				struct btrtl_device_info *btrtl_dev)
 {
-	const struct firmware *fw;
-	int ret;
-
-	bt_dev_info(hdev, "rtl: loading rtl_bt/rtl8723a_fw.bin");
-	ret = request_firmware(&fw, "rtl_bt/rtl8723a_fw.bin", &hdev->dev);
-	if (ret < 0) {
-		BT_ERR("%s: Failed to load rtl_bt/rtl8723a_fw.bin", hdev->name);
-		return ret;
-	}
-
-	if (fw->size < 8) {
-		ret = -EINVAL;
-		goto out;
-	}
+	if (btrtl_dev->fw_len < 8)
+		return -EINVAL;
 
 	/* Check that the firmware doesn't have the epatch signature
 	 * (which is only for RTL8723B and newer).
 	 */
-	if (!memcmp(fw->data, RTL_EPATCH_SIGNATURE, 8)) {
+	if (!memcmp(btrtl_dev->fw_data, RTL_EPATCH_SIGNATURE, 8)) {
 		BT_ERR("%s: unexpected EPATCH signature!", hdev->name);
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
-	ret = rtl_download_firmware(hdev, fw->data, fw->size);
-
-out:
-	release_firmware(fw);
-	return ret;
+	return rtl_download_firmware(hdev, btrtl_dev->fw_data,
+				     btrtl_dev->fw_len);
 }
 
-static int btrtl_setup_rtl8723b(struct hci_dev *hdev, u16 hci_rev,
-				u16 lmp_subver)
+static int btrtl_setup_rtl8723b(struct hci_dev *hdev,
+				struct btrtl_device_info *btrtl_dev)
 {
 	unsigned char *fw_data = NULL;
-	const struct firmware *fw;
 	int ret;
-	int cfg_sz;
-	u8 *cfg_buff = NULL;
 	u8 *tbuff;
-	char *cfg_name = NULL;
-	char *fw_name = NULL;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(ic_id_table); i++) {
-		if ((ic_id_table[i].match_flags & IC_MATCH_FL_LMPSUBV) &&
-		    (ic_id_table[i].lmp_subver != lmp_subver))
-			continue;
-		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIREV) &&
-		    (ic_id_table[i].hci_rev != hci_rev))
-			continue;
-
-		break;
-	}
-
-	if (i >= ARRAY_SIZE(ic_id_table)) {
-		BT_ERR("%s: unknown IC info, lmp subver %04x, hci rev %04x",
-		       hdev->name, lmp_subver, hci_rev);
-		return -EINVAL;
-	}
-
-	cfg_name = ic_id_table[i].cfg_name;
-
-	if (cfg_name) {
-		cfg_sz = rtl_load_config(hdev, cfg_name, &cfg_buff);
-		if (cfg_sz < 0) {
-			cfg_sz = 0;
-			if (ic_id_table[i].config_needed)
-				BT_ERR("Necessary config file %s not found\n",
-				       cfg_name);
-		}
-	} else
-		cfg_sz = 0;
-
-	fw_name = ic_id_table[i].fw_name;
-	bt_dev_info(hdev, "rtl: loading %s", fw_name);
-	ret = request_firmware(&fw, fw_name, &hdev->dev);
-	if (ret < 0) {
-		BT_ERR("%s: Failed to load %s", hdev->name, fw_name);
-		goto err_req_fw;
-	}
 
-	ret = rtlbt_parse_firmware(hdev, lmp_subver, fw, &fw_data);
+	ret = rtlbt_parse_firmware(hdev, btrtl_dev, &fw_data);
 	if (ret < 0)
 		goto out;
 
-	if (cfg_sz) {
-		tbuff = kzalloc(ret + cfg_sz, GFP_KERNEL);
+	if (btrtl_dev->cfg_len > 0) {
+		tbuff = kzalloc(ret + btrtl_dev->cfg_len, GFP_KERNEL);
 		if (!tbuff) {
 			ret = -ENOMEM;
 			goto out;
@@ -441,22 +428,18 @@ static int btrtl_setup_rtl8723b(struct hci_dev *hdev, u16 hci_rev,
 		memcpy(tbuff, fw_data, ret);
 		kfree(fw_data);
 
-		memcpy(tbuff + ret, cfg_buff, cfg_sz);
-		ret += cfg_sz;
+		memcpy(tbuff + ret, btrtl_dev->cfg_data, btrtl_dev->cfg_len);
+		ret += btrtl_dev->cfg_len;
 
 		fw_data = tbuff;
 	}
 
-	bt_dev_info(hdev, "cfg_sz %d, total size %d", cfg_sz, ret);
+	bt_dev_info(hdev, "cfg_sz %d, total size %d", btrtl_dev->cfg_len, ret);
 
 	ret = rtl_download_firmware(hdev, fw_data, ret);
 
 out:
-	release_firmware(fw);
 	kfree(fw_data);
-err_req_fw:
-	if (cfg_sz)
-		kfree(cfg_buff);
 	return ret;
 }
 
@@ -482,15 +465,33 @@ static struct sk_buff *btrtl_read_local_version(struct hci_dev *hdev)
 	return skb;
 }
 
-int btrtl_setup_realtek(struct hci_dev *hdev)
+void btrtl_free(struct btrtl_device_info *btrtl_dev)
+{
+	kfree(btrtl_dev->fw_data);
+	kfree(btrtl_dev->cfg_data);
+	kfree(btrtl_dev);
+}
+EXPORT_SYMBOL_GPL(btrtl_free);
+
+struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
 {
+	struct btrtl_device_info *btrtl_dev;
 	struct sk_buff *skb;
 	struct hci_rp_read_local_version *resp;
 	u16 hci_rev, lmp_subver;
+	int ret;
+
+	btrtl_dev = kzalloc(sizeof(*btrtl_dev), GFP_KERNEL);
+	if (!btrtl_dev) {
+		ret = -ENOMEM;
+		goto err_alloc;
+	}
 
 	skb = btrtl_read_local_version(hdev);
-	if (IS_ERR(skb))
-		return -PTR_ERR(skb);
+	if (IS_ERR(skb)) {
+		ret = PTR_ERR(skb);
+		goto err_free;
+	}
 
 	resp = (struct hci_rp_read_local_version *)skb->data;
 	bt_dev_info(hdev, "rtl: examining hci_ver=%02x hci_rev=%04x "
@@ -502,26 +503,91 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
 	lmp_subver = le16_to_cpu(resp->lmp_subver);
 	kfree_skb(skb);
 
+	btrtl_dev->ic_info = btrtl_match_ic(lmp_subver, hci_rev);
+	if (!btrtl_dev->ic_info) {
+		bt_dev_err(hdev, "rtl: unknown IC info, lmp subver %04x, hci "
+			   "rev %04x", lmp_subver, hci_rev);
+		ret = -EINVAL;
+		goto err_free;
+	}
+
+	if (btrtl_dev->ic_info->has_rom_version) {
+		ret = rtl_read_rom_version(hdev, &btrtl_dev->rom_version);
+		if (ret)
+			goto err_free;
+	}
+
+	btrtl_dev->fw_len = rtl_load_file(hdev, btrtl_dev->ic_info->fw_name,
+					  &btrtl_dev->fw_data);
+	if (btrtl_dev->fw_len < 0) {
+		bt_dev_err(hdev, "firmware file %s not found\n",
+			   btrtl_dev->ic_info->fw_name);
+		ret = btrtl_dev->fw_len;
+		goto err_free;
+	}
+
+	if (btrtl_dev->ic_info->cfg_name) {
+		btrtl_dev->cfg_len = rtl_load_file(hdev,
+						   btrtl_dev->ic_info->cfg_name,
+						   &btrtl_dev->cfg_data);
+		if (btrtl_dev->ic_info->config_needed && btrtl_dev->cfg_len <= 0) {
+			bt_dev_err(hdev,
+				   "mandatory config file %s not found\n",
+				   btrtl_dev->ic_info->cfg_name);
+			ret = btrtl_dev->cfg_len;
+			goto err_free;
+		}
+	}
+
+	return btrtl_dev;
+
+err_free:
+	btrtl_free(btrtl_dev);
+err_alloc:
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(btrtl_initialize);
+
+int btrtl_download_firmware(struct hci_dev *hdev,
+			    struct btrtl_device_info *btrtl_dev)
+{
 	/* Match a set of subver values that correspond to stock firmware,
 	 * which is not compatible with standard btusb.
 	 * If matched, upload an alternative firmware that does conform to
 	 * standard btusb. Once that firmware is uploaded, the subver changes
 	 * to a different value.
 	 */
-	switch (lmp_subver) {
+	switch (btrtl_dev->ic_info->lmp_subver) {
 	case RTL_ROM_LMP_8723A:
 	case RTL_ROM_LMP_3499:
-		return btrtl_setup_rtl8723a(hdev);
+		return btrtl_setup_rtl8723a(hdev, btrtl_dev);
 	case RTL_ROM_LMP_8723B:
 	case RTL_ROM_LMP_8821A:
 	case RTL_ROM_LMP_8761A:
 	case RTL_ROM_LMP_8822B:
-		return btrtl_setup_rtl8723b(hdev, hci_rev, lmp_subver);
+		return btrtl_setup_rtl8723b(hdev, btrtl_dev);
 	default:
 		bt_dev_info(hdev, "rtl: assuming no firmware upload needed");
 		return 0;
 	}
 }
+EXPORT_SYMBOL_GPL(btrtl_download_firmware);
+
+int btrtl_setup_realtek(struct hci_dev *hdev)
+{
+	struct btrtl_device_info *btrtl_dev;
+	int ret;
+
+	btrtl_dev = btrtl_initialize(hdev);
+	if (IS_ERR(btrtl_dev))
+		return PTR_ERR(btrtl_dev);
+
+	ret = btrtl_download_firmware(hdev, btrtl_dev);
+
+	btrtl_free(btrtl_dev);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(btrtl_setup_realtek);
 
 MODULE_AUTHOR("Daniel Drake <drake@endlessm.com>");
diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
index 38ffe4890cd1..21c28dcbe5e0 100644
--- a/drivers/bluetooth/btrtl.h
+++ b/drivers/bluetooth/btrtl.h
@@ -17,6 +17,8 @@
 
 #define RTL_FRAG_LEN 252
 
+struct btrtl_device_info;
+
 struct rtl_download_cmd {
 	__u8 index;
 	__u8 data[RTL_FRAG_LEN];
@@ -40,10 +42,29 @@ struct rtl_epatch_header {
 
 #if IS_ENABLED(CONFIG_BT_RTL)
 
+struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev);
+void btrtl_free(struct btrtl_device_info *btrtl_dev);
+int btrtl_download_firmware(struct hci_dev *hdev,
+			    struct btrtl_device_info *btrtl_dev);
 int btrtl_setup_realtek(struct hci_dev *hdev);
 
 #else
 
+static inline struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline void btrtl_free(struct btrtl_device_info *btrtl_dev)
+{
+}
+
+static inline int btrtl_download_firmware(struct hci_dev *hdev,
+					  struct btrtl_device_info *btrtl_dev)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int btrtl_setup_realtek(struct hci_dev *hdev)
 {
 	return -EOPNOTSUPP;
-- 
2.17.0

[PATCH 04/13] Bluetooth: btrtl: add support for retrieving the UART settings

[Hans de Goede]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

The UART settings are embedded in the config blob. This has to be parsed
to successfully initialize the Bluetooth part of the RTL8723BS (which is
an SDIO chip, but the Bluetooth part is connected via UART).

The Realtek "rtl8723bs_bt" and "rtl8723ds_bt" userspace Bluetooth UART
initialization tools (rtk_hciattach) use the following sequence:
- send H5 sync pattern (already supported by hci_h5)
- get LMP version (already supported by btrtl)
- get ROM version (already supported by btrtl)
- load the firmware and config for the current chipset (already
  supported by btrtl)
- read UART settings from the config blob (part of this patch)
- 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)
- change the baudrate and flow control settings on the host
- send the firmware and config blob to the device (already supported by
  btrtl)

Sending the last firmware and config blob download command
(rtl_download_cmd) fails if the UART settings are not updated
beforehand. This is presumably because the device applies the config
right after the firmware and config blob download - which means that at
this point the host is using different UART settings than the device
(which will obviously result in non-working communication).

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>
---
 drivers/bluetooth/btrtl.c | 112 ++++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/btrtl.h |  25 +++++++++
 2 files changed, 137 insertions(+)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index d7b44338cc8a..bc56ef7fcac3 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -34,6 +34,7 @@
 #define RTL_ROM_LMP_8821A	0x8821
 #define RTL_ROM_LMP_8761A	0x8761
 #define RTL_ROM_LMP_8822B	0x8822
+#define RTL_CONFIG_MAGIC	0x8723ab55
 
 #define IC_MATCH_FL_LMPSUBV	(1 << 0)
 #define IC_MATCH_FL_HCIREV	(1 << 1)
@@ -590,6 +591,117 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
 }
 EXPORT_SYMBOL_GPL(btrtl_setup_realtek);
 
+unsigned int btrtl_convert_baudrate(u32 device_baudrate)
+{
+	switch (device_baudrate) {
+	case 0x0252a00a:
+		return 230400;
+
+	case 0x05f75004:
+		return 921600;
+
+	case 0x00005004:
+		return 1000000;
+
+	case 0x04928002:
+	case 0x01128002:
+		return 1500000;
+
+	case 0x00005002:
+		return 2000000;
+
+	case 0x0000b001:
+		return 2500000;
+
+	case 0x04928001:
+		return 3000000;
+
+	case 0x052a6001:
+		return 3500000;
+
+	case 0x00005001:
+		return 4000000;
+
+	case 0x0252c014:
+	default:
+		return 115200;
+	}
+}
+
+int btrtl_get_uart_settings(struct hci_dev *hdev,
+			    struct btrtl_device_info *btrtl_dev,
+			    unsigned int *controller_baudrate,
+			    u32 *device_baudrate, bool *flow_control)
+{
+	struct rtl_vendor_config *config;
+	struct rtl_vendor_config_entry *entry;
+	int i, total_data_len;
+	bool found = false;
+
+	total_data_len = btrtl_dev->cfg_len - sizeof(*config);
+	if (total_data_len <= 0) {
+		bt_dev_warn(hdev, "rtl: no config loaded");
+		return -EINVAL;
+	}
+
+	config = (struct rtl_vendor_config *)btrtl_dev->cfg_data;
+	if (le32_to_cpu(config->signature) != RTL_CONFIG_MAGIC) {
+		bt_dev_err(hdev, "rtl: invalid config magic");
+		return -EINVAL;
+	}
+
+	if (total_data_len < le16_to_cpu(config->total_len)) {
+		bt_dev_err(hdev, "rtl: config is too short");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < total_data_len; ) {
+		entry = ((void *)config->entry) + i;
+
+		switch (le16_to_cpu(entry->offset)) {
+		case 0xc:
+			if (entry->len < sizeof(*device_baudrate)) {
+				bt_dev_err(hdev,
+					   "rtl: invalid UART config entry");
+				return -EINVAL;
+			}
+
+			*device_baudrate = get_unaligned_le32(entry->data);
+			*controller_baudrate = btrtl_convert_baudrate(
+							*device_baudrate);
+
+			if (entry->len >= 13)
+				*flow_control = !!(entry->data[12] & BIT(2));
+			else
+				*flow_control = false;
+
+			found = true;
+			break;
+
+		default:
+			bt_dev_dbg(hdev,
+				   "rtl: skipping config entry 0x%x (len %u)",
+				   le16_to_cpu(entry->offset), entry->len);
+			break;
+		};
+
+		i += sizeof(*entry) + entry->len;
+	}
+
+	if (!found) {
+		bt_dev_err(hdev, "rtl: no UART config entry found");
+		return -ENOENT;
+	}
+
+	bt_dev_dbg(hdev, "rtl: device baudrate = 0x%08x", *device_baudrate);
+	bt_dev_dbg(hdev, "rtl: controller baudrate = %u",
+		   *controller_baudrate);
+	bt_dev_dbg(hdev, "rtl: flow control %d", *flow_control);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(btrtl_get_uart_settings);
+
 MODULE_AUTHOR("Daniel Drake <drake@endlessm.com>");
 MODULE_DESCRIPTION("Bluetooth support for Realtek devices ver " VERSION);
 MODULE_VERSION(VERSION);
diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
index 21c28dcbe5e0..2e7856fa5ac7 100644
--- a/drivers/bluetooth/btrtl.h
+++ b/drivers/bluetooth/btrtl.h
@@ -40,6 +40,18 @@ struct rtl_epatch_header {
 	__le16 num_patches;
 } __packed;
 
+struct rtl_vendor_config_entry {
+	__le16 offset;
+	__u8 len;
+	__u8 data[0];
+} __packed;
+
+struct rtl_vendor_config {
+	__le32 signature;
+	__le16 total_len;
+	struct rtl_vendor_config_entry entry[0];
+} __packed;
+
 #if IS_ENABLED(CONFIG_BT_RTL)
 
 struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev);
@@ -47,6 +59,10 @@ void btrtl_free(struct btrtl_device_info *btrtl_dev);
 int btrtl_download_firmware(struct hci_dev *hdev,
 			    struct btrtl_device_info *btrtl_dev);
 int btrtl_setup_realtek(struct hci_dev *hdev);
+int btrtl_get_uart_settings(struct hci_dev *hdev,
+			    struct btrtl_device_info *btrtl_dev,
+			    unsigned int *controller_baudrate,
+			    u32 *device_baudrate, bool *flow_control);
 
 #else
 
@@ -70,4 +86,13 @@ static inline int btrtl_setup_realtek(struct hci_dev *hdev)
 	return -EOPNOTSUPP;
 }
 
+static inline int btrtl_get_uart_settings(struct hci_dev *hdev,
+					  struct btrtl_device_info *btrtl_dev,
+					  unsigned int *controller_baudrate,
+					  u32 *device_baudrate,
+					  bool *flow_control)
+{
+	return -ENOENT;
+}
+
 #endif
-- 
2.17.0

[PATCH 05/13] Bluetooth: btrtl: add support for the RTL8723BS and RTL8723DS chips

[Hans de Goede]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

The Realtek RTL8723BS and RTL8723DS chipsets are SDIO wifi chips. They
also contain a Bluetooth module which is connected via UART to the host.

Realtek's userspace initialization tool (rtk_hciattach) differentiates
these two via the HCI version and revision returned by the
HCI_OP_READ_LOCAL_VERSION command.
Additionally we apply these checks only the for UART devices. Everything
else is assumed to be a "RTL8723B" which was originally supported by the
driver (communicating via USB).

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/btrtl.c | 50 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index bc56ef7fcac3..134eac0c8fe7 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -38,6 +38,8 @@
 
 #define IC_MATCH_FL_LMPSUBV	(1 << 0)
 #define IC_MATCH_FL_HCIREV	(1 << 1)
+#define IC_MATCH_FL_HCIVER	(1 << 2)
+#define IC_MATCH_FL_HCIBUS	(1 << 3)
 #define IC_INFO(lmps, hcir) \
 	.match_flags = IC_MATCH_FL_LMPSUBV | IC_MATCH_FL_HCIREV, \
 	.lmp_subver = (lmps), \
@@ -47,6 +49,8 @@ struct id_table {
 	__u16 match_flags;
 	__u16 lmp_subver;
 	__u16 hci_rev;
+	__u8 hci_ver;
+	__u8 hci_bus;
 	bool config_needed;
 	bool has_rom_version;
 	char *fw_name;
@@ -75,6 +79,18 @@ static const struct id_table ic_id_table[] = {
 	  .fw_name = "rtl_bt/rtl8723a_fw.bin",
 	  .cfg_name = NULL },
 
+	/* 8723BS */
+	{ .match_flags = IC_MATCH_FL_LMPSUBV | IC_MATCH_FL_HCIREV | \
+			 IC_MATCH_FL_HCIVER | IC_MATCH_FL_HCIBUS,
+	  .lmp_subver = RTL_ROM_LMP_8723B,
+	  .hci_rev = 0xb,
+	  .hci_ver = 6,
+	  .hci_bus = HCI_UART,
+	  .config_needed = true,
+	  .has_rom_version = true,
+	  .fw_name  = "rtl_bt/rtl8723bs_fw.bin",
+	  .cfg_name = "rtl_bt/rtl8723bs_config.bin" },
+
 	/* 8723B */
 	{ IC_INFO(RTL_ROM_LMP_8723B, 0xb),
 	  .config_needed = false,
@@ -89,6 +105,18 @@ static const struct id_table ic_id_table[] = {
 	  .fw_name  = "rtl_bt/rtl8723d_fw.bin",
 	  .cfg_name = "rtl_bt/rtl8723d_config.bin" },
 
+	/* 8723DS */
+	{ .match_flags = IC_MATCH_FL_LMPSUBV | IC_MATCH_FL_HCIREV | \
+			 IC_MATCH_FL_HCIVER | IC_MATCH_FL_HCIBUS,
+	  .lmp_subver = RTL_ROM_LMP_8723B,
+	  .hci_rev = 0xd,
+	  .hci_ver = 8,
+	  .hci_bus = HCI_UART,
+	  .config_needed = true,
+	  .has_rom_version = true,
+	  .fw_name  = "rtl_bt/rtl8723ds_fw.bin",
+	  .cfg_name = "rtl_bt/rtl8723ds_config.bin" },
+
 	/* 8821A */
 	{ IC_INFO(RTL_ROM_LMP_8821A, 0xa),
 	  .config_needed = false,
@@ -118,7 +146,8 @@ static const struct id_table ic_id_table[] = {
 	  .cfg_name = "rtl_bt/rtl8822b_config.bin" },
 	};
 
-static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev)
+static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev,
+					     u8 hci_ver, u8 hci_bus)
 {
 	int i;
 
@@ -129,6 +158,12 @@ static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev)
 		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIREV) &&
 		    (ic_id_table[i].hci_rev != hci_rev))
 			continue;
+		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIVER) &&
+		    (ic_id_table[i].hci_ver != hci_ver))
+			continue;
+		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIBUS) &&
+		    (ic_id_table[i].hci_bus != hci_bus))
+			continue;
 
 		break;
 	}
@@ -480,6 +515,7 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
 	struct sk_buff *skb;
 	struct hci_rp_read_local_version *resp;
 	u16 hci_rev, lmp_subver;
+	u8 hci_ver;
 	int ret;
 
 	btrtl_dev = kzalloc(sizeof(*btrtl_dev), GFP_KERNEL);
@@ -500,14 +536,18 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
 		    resp->hci_ver, resp->hci_rev,
 		    resp->lmp_ver, resp->lmp_subver);
 
+	hci_ver = resp->hci_ver;
 	hci_rev = le16_to_cpu(resp->hci_rev);
 	lmp_subver = le16_to_cpu(resp->lmp_subver);
 	kfree_skb(skb);
 
-	btrtl_dev->ic_info = btrtl_match_ic(lmp_subver, hci_rev);
+	btrtl_dev->ic_info = btrtl_match_ic(lmp_subver, hci_rev, hci_ver,
+					    hdev->bus);
+
 	if (!btrtl_dev->ic_info) {
 		bt_dev_err(hdev, "rtl: unknown IC info, lmp subver %04x, hci "
-			   "rev %04x", lmp_subver, hci_rev);
+			   "rev %04x, hci ver %04x", lmp_subver, hci_rev,
+			   hci_ver);
 		ret = -EINVAL;
 		goto err_free;
 	}
@@ -709,6 +749,10 @@ MODULE_LICENSE("GPL");
 MODULE_FIRMWARE("rtl_bt/rtl8723a_fw.bin");
 MODULE_FIRMWARE("rtl_bt/rtl8723b_fw.bin");
 MODULE_FIRMWARE("rtl_bt/rtl8723b_config.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8723bs_fw.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8723bs_config.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8723ds_fw.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8723ds_config.bin");
 MODULE_FIRMWARE("rtl_bt/rtl8761a_fw.bin");
 MODULE_FIRMWARE("rtl_bt/rtl8761a_config.bin");
 MODULE_FIRMWARE("rtl_bt/rtl8821a_fw.bin");
-- 
2.17.0

[PATCH 06/13] Bluetooth: btrtl: load the config blob from devicetree when available

[Hans de Goede]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Some Realtek bluetooth devices need a "config" blob. The btrtl driver
currently only allows loading this config blob via the request_firmware
mechanism.

The UART Bluetooth chips use this config blob to specify the baudrate,
whether flow control is used and some other unknown bits. This means
that the config blob is board-specific - thus loading it via
request_firmware means that the rootfs is tied to a specific board.

The UART Bluetooth chips are implemented through serdev. This means
there is also a devicetree node which describes the Bluetooth chip.
Thus we can also load the blob from the devicetree node to keep the
filesystem independent of any board configuration data. In the future
this could be extended to support ACPI as well (in case that's needed).

Parse the devicetree node if it exists and obtain the config blob from
there. Otherwise fall back to using the "old" request_firmware
mechanism.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/btrtl.c | 43 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

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

[PATCH 07/13] Bluetooth: hci_uart: Restore hci_dev->flush callback on open()

[Hans de Goede]

For reasons explained in detail in commit 3611f4d2a5e0 ("hci_ldisc:
fix null pointer deref") the hci_uart_close() functions sets
hci_dev->flush to NULL.

But the device may be re-opened after a close, this commit restores the
hci_dev->flush callback on open().

Note this commit also moves the nearly empty defition of hci_uart_open()
a bit down in the file to avoid the need for forward declaring
hci_uart_flush().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_ldisc.c  | 20 +++++++++++---------
 drivers/bluetooth/hci_serdev.c | 19 +++++++++++--------
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index b6a71705b7d6..8da0f53b5912 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -229,15 +229,6 @@ int hci_uart_init_ready(struct hci_uart *hu)
 }
 
 /* ------- Interface to HCI layer ------ */
-/* Initialize device */
-static int hci_uart_open(struct hci_dev *hdev)
-{
-	BT_DBG("%s %p", hdev->name, hdev);
-
-	/* Nothing to do for UART driver */
-	return 0;
-}
-
 /* Reset device */
 static int hci_uart_flush(struct hci_dev *hdev)
 {
@@ -264,6 +255,17 @@ static int hci_uart_flush(struct hci_dev *hdev)
 	return 0;
 }
 
+/* Initialize device */
+static int hci_uart_open(struct hci_dev *hdev)
+{
+	BT_DBG("%s %p", hdev->name, hdev);
+
+	/* Undo clearing this from hci_uart_close() */
+	hdev->flush = hci_uart_flush;
+
+	return 0;
+}
+
 /* Close device */
 static int hci_uart_close(struct hci_dev *hdev)
 {
diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c
index e0e6461b9200..da6eb5bcfa6f 100644
--- a/drivers/bluetooth/hci_serdev.c
+++ b/drivers/bluetooth/hci_serdev.c
@@ -101,14 +101,6 @@ static void hci_uart_write_work(struct work_struct *work)
 
 /* ------- Interface to HCI layer ------ */
 
-/* Initialize device */
-static int hci_uart_open(struct hci_dev *hdev)
-{
-	BT_DBG("%s %p", hdev->name, hdev);
-
-	return 0;
-}
-
 /* Reset device */
 static int hci_uart_flush(struct hci_dev *hdev)
 {
@@ -129,6 +121,17 @@ static int hci_uart_flush(struct hci_dev *hdev)
 	return 0;
 }
 
+/* Initialize device */
+static int hci_uart_open(struct hci_dev *hdev)
+{
+	BT_DBG("%s %p", hdev->name, hdev);
+
+	/* Undo clearing this from hci_uart_close() */
+	hdev->flush = hci_uart_flush;
+
+	return 0;
+}
+
 /* Close device */
 static int hci_uart_close(struct hci_dev *hdev)
 {
-- 
2.17.0

[PATCH 08/13] Bluetooth: hci_serdev: Move serdev_device_close/open into common hci_serdev code

[Hans de Goede]

Make hci_uart_register_device() and hci_uart_unregister_device() call
serdev_device_close()/open() themselves instead of relying on the various
hci_uart drivers to do this for them.

Besides reducing code complexity, this also ensures correct error checking
of serdev_device_open(), which was missing in a few drivers.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_bcm.c    | 10 +---------
 drivers/bluetooth/hci_ll.c     |  3 ---
 drivers/bluetooth/hci_nokia.c  |  3 ---
 drivers/bluetooth/hci_serdev.c |  9 ++++++++-
 4 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index f06f0f1132fb..ddbd8c6a0ceb 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -380,10 +380,6 @@ static int bcm_open(struct hci_uart *hu)
 	mutex_lock(&bcm_device_lock);
 
 	if (hu->serdev) {
-		err = serdev_device_open(hu->serdev);
-		if (err)
-			goto err_free;
-
 		bcm->dev = serdev_device_get_drvdata(hu->serdev);
 		goto out;
 	}
@@ -420,13 +416,10 @@ static int bcm_open(struct hci_uart *hu)
 	return 0;
 
 err_unset_hu:
-	if (hu->serdev)
-		serdev_device_close(hu->serdev);
 #ifdef CONFIG_PM
-	else
+	if (!hu->serdev)
 		bcm->dev->hu = NULL;
 #endif
-err_free:
 	mutex_unlock(&bcm_device_lock);
 	hu->priv = NULL;
 	kfree(bcm);
@@ -445,7 +438,6 @@ static int bcm_close(struct hci_uart *hu)
 	mutex_lock(&bcm_device_lock);
 
 	if (hu->serdev) {
-		serdev_device_close(hu->serdev);
 		bdev = serdev_device_get_drvdata(hu->serdev);
 	} else if (bcm_device_exists(bcm->dev)) {
 		bdev = bcm->dev;
diff --git a/drivers/bluetooth/hci_ll.c b/drivers/bluetooth/hci_ll.c
index 27e414b4e3a2..3e767f245ed5 100644
--- a/drivers/bluetooth/hci_ll.c
+++ b/drivers/bluetooth/hci_ll.c
@@ -141,7 +141,6 @@ static int ll_open(struct hci_uart *hu)
 
 	if (hu->serdev) {
 		struct ll_device *lldev = serdev_device_get_drvdata(hu->serdev);
-		serdev_device_open(hu->serdev);
 		if (!IS_ERR(lldev->ext_clk))
 			clk_prepare_enable(lldev->ext_clk);
 	}
@@ -179,8 +178,6 @@ static int ll_close(struct hci_uart *hu)
 		gpiod_set_value_cansleep(lldev->enable_gpio, 0);
 
 		clk_disable_unprepare(lldev->ext_clk);
-
-		serdev_device_close(hu->serdev);
 	}
 
 	hu->priv = NULL;
diff --git a/drivers/bluetooth/hci_nokia.c b/drivers/bluetooth/hci_nokia.c
index 3539fd03f47e..14d159e2042d 100644
--- a/drivers/bluetooth/hci_nokia.c
+++ b/drivers/bluetooth/hci_nokia.c
@@ -477,8 +477,6 @@ static int nokia_open(struct hci_uart *hu)
 
 	dev_dbg(dev, "protocol open");
 
-	serdev_device_open(hu->serdev);
-
 	pm_runtime_enable(dev);
 
 	return 0;
@@ -513,7 +511,6 @@ static int nokia_close(struct hci_uart *hu)
 	gpiod_set_value(btdev->wakeup_bt, 0);
 
 	pm_runtime_disable(&btdev->serdev->dev);
-	serdev_device_close(btdev->serdev);
 
 	return 0;
 }
diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c
index da6eb5bcfa6f..ce024142f5d3 100644
--- a/drivers/bluetooth/hci_serdev.c
+++ b/drivers/bluetooth/hci_serdev.c
@@ -285,10 +285,14 @@ int hci_uart_register_device(struct hci_uart *hu,
 
 	serdev_device_set_client_ops(hu->serdev, &hci_serdev_client_ops);
 
-	err = p->open(hu);
+	err = serdev_device_open(hu->serdev);
 	if (err)
 		return err;
 
+	err = p->open(hu);
+	if (err)
+		goto err_open;
+
 	hu->proto = p;
 	set_bit(HCI_UART_PROTO_READY, &hu->flags);
 
@@ -354,6 +358,8 @@ int hci_uart_register_device(struct hci_uart *hu,
 err_alloc:
 	clear_bit(HCI_UART_PROTO_READY, &hu->flags);
 	p->close(hu);
+err_open:
+	serdev_device_close(hu->serdev);
 	return err;
 }
 EXPORT_SYMBOL_GPL(hci_uart_register_device);
@@ -368,5 +374,6 @@ void hci_uart_unregister_device(struct hci_uart *hu)
 	cancel_work_sync(&hu->write_work);
 
 	hu->proto->close(hu);
+	serdev_device_close(hu->serdev);
 }
 EXPORT_SYMBOL_GPL(hci_uart_unregister_device);
-- 
2.17.0

[PATCH 09/13] Bluetooth: hci_serdev: Fix HCI_UART_INIT_PENDING not working

[Hans de Goede]

Init hci_uart->init_ready so that hci_uart_init_ready() works properly.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_ldisc.c  | 2 +-
 drivers/bluetooth/hci_serdev.c | 1 +
 drivers/bluetooth/hci_uart.h   | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 8da0f53b5912..1b240022dc19 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -195,7 +195,7 @@ 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)
+void hci_uart_init_work(struct work_struct *work)
 {
 	struct hci_uart *hu = container_of(work, struct hci_uart, init_ready);
 	int err;
diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c
index ce024142f5d3..cf1ade833fcd 100644
--- a/drivers/bluetooth/hci_serdev.c
+++ b/drivers/bluetooth/hci_serdev.c
@@ -309,6 +309,7 @@ int hci_uart_register_device(struct hci_uart *hu,
 	hdev->bus = HCI_UART;
 	hci_set_drvdata(hdev, hu);
 
+	INIT_WORK(&hu->init_ready, hci_uart_init_work);
 	INIT_WORK(&hu->write_work, hci_uart_write_work);
 	percpu_init_rwsem(&hu->proto_lock);
 
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 66e8c68e4607..00cab2fd7a1b 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -116,6 +116,7 @@ void hci_uart_unregister_device(struct hci_uart *hu);
 
 int hci_uart_tx_wakeup(struct hci_uart *hu);
 int hci_uart_init_ready(struct hci_uart *hu);
+void hci_uart_init_work(struct work_struct *work);
 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,
-- 
2.17.0

[PATCH 10/13] Bluetooth: hci_h5: Add support for serdev enumerated devices

[Hans de Goede]

Add basic support for serdev enumerated devices, note sine this does
not (yet) declare any of / ACPI ids to bind to atm this is a nop.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_h5.c | 51 +++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index 6a8d0d06aba7..11bfe28edb86 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -23,6 +23,7 @@
 
 #include <linux/kernel.h>
 #include <linux/errno.h>
+#include <linux/serdev.h>
 #include <linux/skbuff.h>
 
 #include <net/bluetooth/bluetooth.h>
@@ -65,6 +66,9 @@ enum {
 };
 
 struct h5 {
+	/* Must be the first member, hci_serdev.c expects this. */
+	struct hci_uart		serdev_hu;
+
 	struct sk_buff_head	unack;		/* Unack'ed packets queue */
 	struct sk_buff_head	rel;		/* Reliable packets queue */
 	struct sk_buff_head	unrel;		/* Unreliable packets queue */
@@ -193,9 +197,13 @@ static int h5_open(struct hci_uart *hu)
 
 	BT_DBG("hu %p", hu);
 
-	h5 = kzalloc(sizeof(*h5), GFP_KERNEL);
-	if (!h5)
-		return -ENOMEM;
+	if (hu->serdev) {
+		h5 = serdev_device_get_drvdata(hu->serdev);
+	} else {
+		h5 = kzalloc(sizeof(*h5), GFP_KERNEL);
+		if (!h5)
+			return -ENOMEM;
+	}
 
 	hu->priv = h5;
 	h5->hu = hu;
@@ -229,7 +237,8 @@ static int h5_close(struct hci_uart *hu)
 	skb_queue_purge(&h5->rel);
 	skb_queue_purge(&h5->unrel);
 
-	kfree(h5);
+	if (!hu->serdev)
+		kfree(h5);
 
 	return 0;
 }
@@ -750,12 +759,46 @@ static const struct hci_uart_proto h5p = {
 	.flush		= h5_flush,
 };
 
+static int h5_serdev_probe(struct serdev_device *serdev)
+{
+	struct h5 *h5;
+
+	h5 = devm_kzalloc(&serdev->dev, sizeof(*h5), GFP_KERNEL);
+	if (!h5)
+		return -ENOMEM;
+
+	set_bit(HCI_UART_RESET_ON_INIT, &h5->serdev_hu.flags);
+
+	h5->hu = &h5->serdev_hu;
+	h5->serdev_hu.serdev = serdev;
+	serdev_device_set_drvdata(serdev, h5);
+
+	return hci_uart_register_device(&h5->serdev_hu, &h5p);
+}
+
+static void h5_serdev_remove(struct serdev_device *serdev)
+{
+	struct h5 *h5 = serdev_device_get_drvdata(serdev);
+
+	hci_uart_unregister_device(&h5->serdev_hu);
+}
+
+static struct serdev_device_driver h5_serdev_driver = {
+	.probe = h5_serdev_probe,
+	.remove = h5_serdev_remove,
+	.driver = {
+		.name = "hci_uart_h5",
+	},
+};
+
 int __init h5_init(void)
 {
+	serdev_device_driver_register(&h5_serdev_driver);
 	return hci_uart_register_proto(&h5p);
 }
 
 int __exit h5_deinit(void)
 {
+	serdev_device_driver_unregister(&h5_serdev_driver);
 	return hci_uart_unregister_proto(&h5p);
 }
-- 
2.17.0

[PATCH 11/13] Bluetooth: hci_h5: Add vendor setup, open, and close callbacks

[Hans de Goede]

From: Jeremy Cline <jeremy@jcline.org>

Allow vendor-specific setup, open, and close functions to be defined.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
[hdegoede@redhat.com: Port from bt3wire.c to hci_h5.c]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_h5.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index 11bfe28edb86..db0d531d7813 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -21,8 +21,10 @@
  *
  */
 
-#include <linux/kernel.h>
+#include <linux/acpi.h>
 #include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/of_device.h>
 #include <linux/serdev.h>
 #include <linux/skbuff.h>
 
@@ -99,6 +101,14 @@ struct h5 {
 		H5_SLEEPING,
 		H5_WAKING_UP,
 	} sleep;
+
+	const struct h5_vnd *vnd;
+};
+
+struct h5_vnd {
+	int (*setup)(struct h5 *h5);
+	void (*open)(struct h5 *h5);
+	void (*close)(struct h5 *h5);
 };
 
 static void h5_reset_rx(struct h5 *h5);
@@ -218,6 +228,9 @@ static int h5_open(struct hci_uart *hu)
 
 	h5->tx_win = H5_TX_WIN_MAX;
 
+	if (h5->vnd && h5->vnd->open)
+		h5->vnd->open(h5);
+
 	set_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags);
 
 	/* Send initial sync request */
@@ -237,12 +250,25 @@ static int h5_close(struct hci_uart *hu)
 	skb_queue_purge(&h5->rel);
 	skb_queue_purge(&h5->unrel);
 
+	if (h5->vnd && h5->vnd->close)
+		h5->vnd->close(h5);
+
 	if (!hu->serdev)
 		kfree(h5);
 
 	return 0;
 }
 
+static int h5_setup(struct hci_uart *hu)
+{
+	struct h5 *h5 = hu->priv;
+
+	if (h5->vnd && h5->vnd->setup)
+		return h5->vnd->setup(h5);
+
+	return 0;
+}
+
 static void h5_pkt_cull(struct h5 *h5)
 {
 	struct sk_buff *skb, *tmp;
@@ -753,6 +779,7 @@ static const struct hci_uart_proto h5p = {
 	.name		= "Three-wire (H5)",
 	.open		= h5_open,
 	.close		= h5_close,
+	.setup		= h5_setup,
 	.recv		= h5_recv,
 	.enqueue	= h5_enqueue,
 	.dequeue	= h5_dequeue,
@@ -773,6 +800,11 @@ static int h5_serdev_probe(struct serdev_device *serdev)
 	h5->serdev_hu.serdev = serdev;
 	serdev_device_set_drvdata(serdev, h5);
 
+	if (has_acpi_companion(&serdev->dev))
+		h5->vnd = acpi_device_get_match_data(&serdev->dev);
+	else
+		h5->vnd = of_device_get_match_data(&serdev->dev);
+
 	return hci_uart_register_device(&h5->serdev_hu, &h5p);
 }
 
-- 
2.17.0

[PATCH 12/13] Bluetooth: hci_h5: Add support for the RTL8723BS

[Hans de Goede]

From: Jeremy Cline <jeremy@jcline.org>

Implement support for the RTL8723BS chip.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
[hdegoede@redhat.com: Port from bt3wire.c to hci_h5.c, drop broken GPIO code]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_h5.c | 69 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index db0d531d7813..d81f950cea27 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -31,6 +31,7 @@
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 
+#include "btrtl.h"
 #include "hci_uart.h"
 
 #define HCI_3WIRE_ACK_PKT	0
@@ -815,11 +816,79 @@ static void h5_serdev_remove(struct serdev_device *serdev)
 	hci_uart_unregister_device(&h5->serdev_hu);
 }
 
+static int h5_btrtl_setup(struct h5 *h5)
+{
+	struct btrtl_device_info *btrtl_dev;
+	struct sk_buff *skb;
+	__le32 baudrate_data;
+	u32 device_baudrate;
+	unsigned int controller_baudrate;
+	bool flow_control;
+	int err;
+
+	btrtl_dev = btrtl_initialize(h5->hu->hdev);
+	if (IS_ERR(btrtl_dev))
+		return PTR_ERR(btrtl_dev);
+
+	err = btrtl_get_uart_settings(h5->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(h5->hu->hdev, 0xfc17, sizeof(baudrate_data),
+			     &baudrate_data, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		bt_dev_err(h5->hu->hdev, "set baud rate command failed");
+		err = PTR_ERR(skb);
+		goto out_free;
+	} else {
+		kfree_skb(skb);
+	}
+	/* Give the device some time to set up the new baudrate. */
+	msleep(10);
+
+	serdev_device_set_baudrate(h5->hu->serdev, controller_baudrate);
+	serdev_device_set_flow_control(h5->hu->serdev, flow_control);
+
+	err = btrtl_download_firmware(h5->hu->hdev, btrtl_dev);
+	/* Give the device some time before the hci-core sends it a reset */
+	msleep(10);
+
+out_free:
+	btrtl_free(btrtl_dev);
+
+	return err;
+}
+
+static void h5_btrtl_open(struct h5 *h5)
+{
+	/* Devices always start with these fixed parameters */
+	serdev_device_set_flow_control(h5->hu->serdev, false);
+	serdev_device_set_parity(h5->hu->serdev, SERDEV_PARITY_EVEN);
+	serdev_device_set_baudrate(h5->hu->serdev, 115200);
+}
+
+static struct h5_vnd rtl_vnd = {
+	.setup		= h5_btrtl_setup,
+	.open		= h5_btrtl_open,
+};
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id h5_acpi_match[] = {
+	{ "OBDA8723", (kernel_ulong_t)&rtl_vnd },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, h5_acpi_match);
+#endif
+
 static struct serdev_device_driver h5_serdev_driver = {
 	.probe = h5_serdev_probe,
 	.remove = h5_serdev_remove,
 	.driver = {
 		.name = "hci_uart_h5",
+		.acpi_match_table = ACPI_PTR(h5_acpi_match),
 	},
 };
 
-- 
2.17.0

[PATCH 13/13] Bluetooth: hci_h5: Add support for enable and device-wake GPIOs

[Hans de Goede]

Add support for the enable and device-wake GPIOs used on ACPI enumerated
RTL8723BS devices.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_h5.c | 47 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index d81f950cea27..177eabd00d49 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -23,6 +23,7 @@
 
 #include <linux/acpi.h>
 #include <linux/errno.h>
+#include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/of_device.h>
 #include <linux/serdev.h>
@@ -104,12 +105,16 @@ struct h5 {
 	} sleep;
 
 	const struct h5_vnd *vnd;
+
+	struct gpio_desc *enable_gpio;
+	struct gpio_desc *device_wake_gpio;
 };
 
 struct h5_vnd {
 	int (*setup)(struct h5 *h5);
 	void (*open)(struct h5 *h5);
 	void (*close)(struct h5 *h5);
+	const struct acpi_gpio_mapping *acpi_gpios;
 };
 
 static void h5_reset_rx(struct h5 *h5);
@@ -801,10 +806,25 @@ static int h5_serdev_probe(struct serdev_device *serdev)
 	h5->serdev_hu.serdev = serdev;
 	serdev_device_set_drvdata(serdev, h5);
 
-	if (has_acpi_companion(&serdev->dev))
+	if (has_acpi_companion(&serdev->dev)) {
 		h5->vnd = acpi_device_get_match_data(&serdev->dev);
-	else
+		if (h5->vnd && h5->vnd->acpi_gpios)
+			devm_acpi_dev_add_driver_gpios(&serdev->dev,
+						       h5->vnd->acpi_gpios);
+	} else {
 		h5->vnd = of_device_get_match_data(&serdev->dev);
+	}
+
+	h5->enable_gpio = devm_gpiod_get_optional(&serdev->dev, "enable",
+						  GPIOD_OUT_LOW);
+	if (IS_ERR(h5->enable_gpio))
+		return PTR_ERR(h5->enable_gpio);
+
+	h5->device_wake_gpio = devm_gpiod_get_optional(&serdev->dev,
+						       "device-wake",
+						       GPIOD_OUT_LOW);
+	if (IS_ERR(h5->device_wake_gpio))
+		return PTR_ERR(h5->device_wake_gpio);
 
 	return hci_uart_register_device(&h5->serdev_hu, &h5p);
 }
@@ -868,11 +888,34 @@ static void h5_btrtl_open(struct h5 *h5)
 	serdev_device_set_flow_control(h5->hu->serdev, false);
 	serdev_device_set_parity(h5->hu->serdev, SERDEV_PARITY_EVEN);
 	serdev_device_set_baudrate(h5->hu->serdev, 115200);
+
+	/* The controller needs up to 500ms to wakeup */
+	gpiod_set_value_cansleep(h5->enable_gpio, 1);
+	gpiod_set_value_cansleep(h5->device_wake_gpio, 1);
+	msleep(500);
 }
 
+static void h5_btrtl_close(struct h5 *h5)
+{
+	gpiod_set_value_cansleep(h5->device_wake_gpio, 0);
+	gpiod_set_value_cansleep(h5->enable_gpio, 0);
+}
+
+static const struct acpi_gpio_params btrtl_device_wake_gpios = { 0, 0, false };
+static const struct acpi_gpio_params btrtl_enable_gpios = { 1, 0, false };
+static const struct acpi_gpio_params btrtl_host_wake_gpios = { 2, 0, false };
+static const struct acpi_gpio_mapping acpi_btrtl_gpios[] = {
+	{ "device-wake-gpios", &btrtl_device_wake_gpios, 1 },
+	{ "enable-gpios", &btrtl_enable_gpios, 1 },
+	{ "host-wake-gpios", &btrtl_host_wake_gpios, 1 },
+	{},
+};
+
 static struct h5_vnd rtl_vnd = {
 	.setup		= h5_btrtl_setup,
 	.open		= h5_btrtl_open,
+	.close		= h5_btrtl_close,
+	.acpi_gpios	= acpi_btrtl_gpios,
 };
 
 #ifdef CONFIG_ACPI
-- 
2.17.0

Re: [PATCH 00/13] Bluetooth: Add RTL8723BS support

[Ian W MORRISON]

On Mon, 28 May 2018 at 05:05, Hans de Goede <hdegoede@redhat.com> wrote:

> Here is a series of patches adding support for RTL8723BS' bluetooth
> component through serdev enumration.


Hi All,

I've tested this series on six (6) different Intel based devices which use
the RTL8723BS chip and Bluetooth worked without issue. I added the patch
series to the v4.17-rc7 kernel and used both an Ubuntu and Lubuntu 18.04
user space together with the firmware as referenced by Martin in the
original RFC ([0] https://github.com/lwfinger/rtl8723bs_bt in [RFC v1 0/8]
Realtek Bluetooth serdev support (H5 protocol)).

Besides the successful pairing of a Bluetooth mouse I ran some additional
confirmatory commands and include the output below.

I offer my
Tested-by: Ian W MORRISON <ianwmorrison@gmail.com>
if it helps.

Best regards,
Ian

root@lubuntu:~# tree -d /sys/bus/acpi/devices/OBDA8723:00
/sys/bus/acpi/devices/OBDA8723:00
├── physical_node -> ../../../../platform/80860F0A:00/serial0/serial0-0
├── power
└── subsystem -> ../../../../../bus/acpi

3 directories
root@lubuntu:~# tree -ndL 4
/sys/devices/platform/80860F0A:00/serial0/serial0-0
/sys/devices/platform/80860F0A:00/serial0/serial0-0
├── bluetooth
│   └── hci0
│       ├── device -> ../../../serial0-0
│       ├── hci0:2
│       │   ├── 0005:046D:B010.0007
│       │   ├── device -> ../../hci0
│       │   ├── power
│       │   └── subsystem -> ../../../../../../../../class/bluetooth
│       ├── power
│       ├── rfkill1
│       │   ├── device -> ../../hci0
│       │   ├── power
│       │   └── subsystem -> ../../../../../../../../class/rfkill
│       └── subsystem -> ../../../../../../../class/bluetooth
├── driver -> ../../../../../bus/serial/drivers/hci_uart_h5
├── firmware_node ->
../../../../LNXSYSTM:00/LNXSYBUS:00/80860F0A:00/OBDA8723:00
├── power
└── subsystem -> ../../../../../bus/serial

18 directories
root@lubuntu:~#

root@lubuntu:~# modinfo btrtl
filename:
/lib/modules/4.17.0-041700rc7-linuxium/kernel/drivers/bluetooth/btrtl.ko
firmware:       rtl_bt/rtl8822b_config.bin
firmware:       rtl_bt/rtl8822b_fw.bin
firmware:       rtl_bt/rtl8821a_config.bin
firmware:       rtl_bt/rtl8821a_fw.bin
firmware:       rtl_bt/rtl8761a_config.bin
firmware:       rtl_bt/rtl8761a_fw.bin
firmware:       rtl_bt/rtl8723ds_config.bin
firmware:       rtl_bt/rtl8723ds_fw.bin
firmware:       rtl_bt/rtl8723bs_config.bin
firmware:       rtl_bt/rtl8723bs_fw.bin
firmware:       rtl_bt/rtl8723b_config.bin
firmware:       rtl_bt/rtl8723b_fw.bin
firmware:       rtl_bt/rtl8723a_fw.bin
license:        GPL
version:        0.1
description:    Bluetooth support for Realtek devices ver 0.1
author:         Daniel Drake <drake@endlessm.com>
srcversion:     9C1AD30DDA9DA320E135F91
depends:        bluetooth
retpoline:      Y
intree:         Y
name:           btrtl
vermagic:       4.17.0-041700rc7-linuxium SMP mod_unload
signat:         PKCS#7
signer:
sig_key:
sig_hashalgo:   md4
root@lubuntu:~#

root@lubuntu:~# dmesg | grep Bluetooth
[   23.511094] Bluetooth: Core ver 2.22
[   23.511140] Bluetooth: HCI device and connection manager initialized
[   23.511148] Bluetooth: HCI socket layer initialized
[   23.511153] Bluetooth: L2CAP socket layer initialized
[   23.511170] Bluetooth: SCO socket layer initialized
[   23.686184] Bluetooth: HCI UART driver ver 2.3
[   23.686189] Bluetooth: HCI UART protocol H4 registered
[   23.686191] Bluetooth: HCI UART protocol BCSP registered
[   23.686234] Bluetooth: HCI UART protocol LL registered
[   23.686236] Bluetooth: HCI UART protocol ATH3K registered
[   23.686266] Bluetooth: HCI UART protocol Three-wire (H5) registered
[   23.686356] Bluetooth: HCI UART protocol Intel registered
[   23.686530] Bluetooth: HCI UART protocol Broadcom registered
[   23.686532] Bluetooth: HCI UART protocol QCA registered
[   23.686534] Bluetooth: HCI UART protocol AG6XX registered
[   23.686536] Bluetooth: HCI UART protocol Marvell registered
[   24.348107] Bluetooth: hci0: rtl: examining hci_ver=06 hci_rev=000b
lmp_ver=06 lmp_subver=8723
[   24.357025] Bluetooth: hci0: rom_version status=0 version=1
[   24.357032] Bluetooth: hci0: rtl: loading rtl_bt/rtl8723bs_fw.bin
[   24.375237] Bluetooth: hci0: rtl: loading rtl_bt/rtl8723bs_config.bin
[   24.400183] Bluetooth: hci0: cfg_sz 55, total size 24263
[   24.518884] Bluetooth: BNEP (Ethernet Emulation) ver 1.3
[   24.518888] Bluetooth: BNEP filters: protocol multicast
[   24.518896] Bluetooth: BNEP socket layer initialized
[   59.619637] Bluetooth: hci0: last event is not cmd complete (0x0f)
[   69.137958] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
[   69.137967] Bluetooth: HIDP socket layer initialized
[   70.602740] input: Bluetooth Mouse M557 as
/devices/platform/80860F0A:00/serial0/serial0-0/bluetooth/hci0/hci0:2/0005:046D:B010.0007/input/input11
[   70.605395] hid-generic 0005:046D:B010.0007: input,hidraw4: BLUETOOTH
HID v10.01 Mouse [Bluetooth Mouse M557] on 28:c2:dd:a0:eb:84
root@lubuntu:~#

Re: [PATCH 00/13] Bluetooth: Add RTL8723BS support

[Ian W MORRISON]

On Mon, 28 May 2018 at 05:05, Hans de Goede <hdegoede@redhat.com> wrote:

> Here is a series of patches adding support for RTL8723BS' bluetooth
> component through serdev enumration.


Hi All,

I've tested this series on six (6) different Intel based devices which use
the RTL8723BS chip and Bluetooth worked without issue. I added the patch
series to the v4.17-rc7 kernel and used both an Ubuntu and Lubuntu 18.04
user space together with the firmware as referenced by Martin in the
original RFC ([0] https://github.com/lwfinger/rtl8723bs_bt in [RFC v1 0/8]
Realtek Bluetooth serdev support (H5 protocol)).

Besides the successful pairing of a Bluetooth mouse I ran some additional
confirmatory commands and include the output below.

I offer my
Tested-by: Ian W MORRISON <ianwmorrison@gmail.com>
if it helps.

Best regards,
Ian

root@lubuntu:~# tree -d /sys/bus/acpi/devices/OBDA8723:00
/sys/bus/acpi/devices/OBDA8723:00
=E2=94=9C=E2=94=80=E2=94=80 physical_node -> ../../../../platform/80860F0A:=
00/serial0/serial0-0
=E2=94=9C=E2=94=80=E2=94=80 power
=E2=94=94=E2=94=80=E2=94=80 subsystem -> ../../../../../bus/acpi

3 directories
root@lubuntu:~# tree -ndL 4
/sys/devices/platform/80860F0A:00/serial0/serial0-0
/sys/devices/platform/80860F0A:00/serial0/serial0-0
=E2=94=9C=E2=94=80=E2=94=80 bluetooth
=E2=94=82   =E2=94=94=E2=94=80=E2=94=80 hci0
=E2=94=82       =E2=94=9C=E2=94=80=E2=94=80 device -> ../../../serial0-0
=E2=94=82       =E2=94=9C=E2=94=80=E2=94=80 hci0:2
=E2=94=82       =E2=94=82   =E2=94=9C=E2=94=80=E2=94=80 0005:046D:B010.0007
=E2=94=82       =E2=94=82   =E2=94=9C=E2=94=80=E2=94=80 device -> ../../hci=
0
=E2=94=82       =E2=94=82   =E2=94=9C=E2=94=80=E2=94=80 power
=E2=94=82       =E2=94=82   =E2=94=94=E2=94=80=E2=94=80 subsystem -> ../../=
../../../../../../class/bluetooth
=E2=94=82       =E2=94=9C=E2=94=80=E2=94=80 power
=E2=94=82       =E2=94=9C=E2=94=80=E2=94=80 rfkill1
=E2=94=82       =E2=94=82   =E2=94=9C=E2=94=80=E2=94=80 device -> ../../hci=
0
=E2=94=82       =E2=94=82   =E2=94=9C=E2=94=80=E2=94=80 power
=E2=94=82       =E2=94=82   =E2=94=94=E2=94=80=E2=94=80 subsystem -> ../../=
../../../../../../class/rfkill
=E2=94=82       =E2=94=94=E2=94=80=E2=94=80 subsystem -> ../../../../../../=
../class/bluetooth
=E2=94=9C=E2=94=80=E2=94=80 driver -> ../../../../../bus/serial/drivers/hci=
_uart_h5
=E2=94=9C=E2=94=80=E2=94=80 firmware_node ->
../../../../LNXSYSTM:00/LNXSYBUS:00/80860F0A:00/OBDA8723:00
=E2=94=9C=E2=94=80=E2=94=80 power
=E2=94=94=E2=94=80=E2=94=80 subsystem -> ../../../../../bus/serial

18 directories
root@lubuntu:~#

root@lubuntu:~# modinfo btrtl
filename:
/lib/modules/4.17.0-041700rc7-linuxium/kernel/drivers/bluetooth/btrtl.ko
firmware:       rtl_bt/rtl8822b_config.bin
firmware:       rtl_bt/rtl8822b_fw.bin
firmware:       rtl_bt/rtl8821a_config.bin
firmware:       rtl_bt/rtl8821a_fw.bin
firmware:       rtl_bt/rtl8761a_config.bin
firmware:       rtl_bt/rtl8761a_fw.bin
firmware:       rtl_bt/rtl8723ds_config.bin
firmware:       rtl_bt/rtl8723ds_fw.bin
firmware:       rtl_bt/rtl8723bs_config.bin
firmware:       rtl_bt/rtl8723bs_fw.bin
firmware:       rtl_bt/rtl8723b_config.bin
firmware:       rtl_bt/rtl8723b_fw.bin
firmware:       rtl_bt/rtl8723a_fw.bin
license:        GPL
version:        0.1
description:    Bluetooth support for Realtek devices ver 0.1
author:         Daniel Drake <drake@endlessm.com>
srcversion:     9C1AD30DDA9DA320E135F91
depends:        bluetooth
retpoline:      Y
intree:         Y
name:           btrtl
vermagic:       4.17.0-041700rc7-linuxium SMP mod_unload
signat:         PKCS#7
signer:
sig_key:
sig_hashalgo:   md4
root@lubuntu:~#

root@lubuntu:~# dmesg | grep Bluetooth
[   23.511094] Bluetooth: Core ver 2.22
[   23.511140] Bluetooth: HCI device and connection manager initialized
[   23.511148] Bluetooth: HCI socket layer initialized
[   23.511153] Bluetooth: L2CAP socket layer initialized
[   23.511170] Bluetooth: SCO socket layer initialized
[   23.686184] Bluetooth: HCI UART driver ver 2.3
[   23.686189] Bluetooth: HCI UART protocol H4 registered
[   23.686191] Bluetooth: HCI UART protocol BCSP registered
[   23.686234] Bluetooth: HCI UART protocol LL registered
[   23.686236] Bluetooth: HCI UART protocol ATH3K registered
[   23.686266] Bluetooth: HCI UART protocol Three-wire (H5) registered
[   23.686356] Bluetooth: HCI UART protocol Intel registered
[   23.686530] Bluetooth: HCI UART protocol Broadcom registered
[   23.686532] Bluetooth: HCI UART protocol QCA registered
[   23.686534] Bluetooth: HCI UART protocol AG6XX registered
[   23.686536] Bluetooth: HCI UART protocol Marvell registered
[   24.348107] Bluetooth: hci0: rtl: examining hci_ver=3D06 hci_rev=3D000b
lmp_ver=3D06 lmp_subver=3D8723
[   24.357025] Bluetooth: hci0: rom_version status=3D0 version=3D1
[   24.357032] Bluetooth: hci0: rtl: loading rtl_bt/rtl8723bs_fw.bin
[   24.375237] Bluetooth: hci0: rtl: loading rtl_bt/rtl8723bs_config.bin
[   24.400183] Bluetooth: hci0: cfg_sz 55, total size 24263
[   24.518884] Bluetooth: BNEP (Ethernet Emulation) ver 1.3
[   24.518888] Bluetooth: BNEP filters: protocol multicast
[   24.518896] Bluetooth: BNEP socket layer initialized
[   59.619637] Bluetooth: hci0: last event is not cmd complete (0x0f)
[   69.137958] Bluetooth: HIDP (Human Interface Emulation) ver 1.2
[   69.137967] Bluetooth: HIDP socket layer initialized
[   70.602740] input: Bluetooth Mouse M557 as
/devices/platform/80860F0A:00/serial0/serial0-0/bluetooth/hci0/hci0:2/0005:=
046D:B010.0007/input/input11
[   70.605395] hid-generic 0005:046D:B010.0007: input,hidraw4: BLUETOOTH
HID v10.01 Mouse [Bluetooth Mouse M557] on 28:c2:dd:a0:eb:84
root@lubuntu:~#

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

[Marcel Holtmann]

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

Re: [PATCH 07/13] Bluetooth: hci_uart: Restore hci_dev->flush callback on open()

[Marcel Holtmann]

Hi Hans,

> For reasons explained in detail in commit 3611f4d2a5e0 ("hci_ldisc:
> fix null pointer deref") the hci_uart_close() functions sets
> hci_dev->flush to NULL.
> 
> But the device may be re-opened after a close, this commit restores the
> hci_dev->flush callback on open().
> 
> Note this commit also moves the nearly empty defition of hci_uart_open()
> a bit down in the file to avoid the need for forward declaring
> hci_uart_flush().
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/bluetooth/hci_ldisc.c  | 20 +++++++++++---------
> drivers/bluetooth/hci_serdev.c | 19 +++++++++++--------
> 2 files changed, 22 insertions(+), 17 deletions(-)

this patch has been cherry-picked and applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH 08/13] Bluetooth: hci_serdev: Move serdev_device_close/open into common hci_serdev code

[Marcel Holtmann]

Hi Hans,

> Make hci_uart_register_device() and hci_uart_unregister_device() call
> serdev_device_close()/open() themselves instead of relying on the various
> hci_uart drivers to do this for them.
> 
> Besides reducing code complexity, this also ensures correct error checking
> of serdev_device_open(), which was missing in a few drivers.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/bluetooth/hci_bcm.c    | 10 +---------
> drivers/bluetooth/hci_ll.c     |  3 ---
> drivers/bluetooth/hci_nokia.c  |  3 ---
> drivers/bluetooth/hci_serdev.c |  9 ++++++++-
> 4 files changed, 9 insertions(+), 16 deletions(-)

this patch has been cherry-picked and applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH 09/13] Bluetooth: hci_serdev: Fix HCI_UART_INIT_PENDING not working

[Marcel Holtmann]

Hi Hans,

> Init hci_uart->init_ready so that hci_uart_init_ready() works properly.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/bluetooth/hci_ldisc.c  | 2 +-
> drivers/bluetooth/hci_serdev.c | 1 +
> drivers/bluetooth/hci_uart.h   | 1 +
> 3 files changed, 3 insertions(+), 1 deletion(-)

this patch has been cherry-picked and applied to bluetooth-next tree.

Regards

Marcel

Re: [PATCH 12/13] Bluetooth: hci_h5: Add support for the RTL8723BS

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]

Hi Jeremy,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bluetooth/master]
[also build test ERROR on v4.17-rc7 next-20180529]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/Bluetooth-Add-RTL8723BS-support/20180530-141115
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master
config: i386-randconfig-a1-05291352 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "serdev_device_set_parity" [drivers/bluetooth/hci_uart.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27967 bytes --]

Re: [PATCH 12/13] Bluetooth: hci_h5: Add support for the RTL8723BS

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]

Hi Jeremy,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bluetooth/master]
[also build test ERROR on v4.17-rc7 next-20180529]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/Bluetooth-Add-RTL8723BS-support/20180530-141115
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master
config: i386-randconfig-a1-05291352 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "serdev_device_set_parity" [drivers/bluetooth/hci_uart.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27967 bytes --]

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

[Hans de Goede]

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

Re: [PATCH 04/13] Bluetooth: btrtl: add support for retrieving the UART settings

[kbuild test robot]

Hi Martin,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth/master]
[also build test WARNING on v4.17-rc7 next-20180529]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/Bluetooth-Add-RTL8723BS-support/20180530-141115
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/bluetooth/btrtl.c:594:14: sparse: symbol 'btrtl_convert_baudrate' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[RFC PATCH] Bluetooth: btrtl: btrtl_convert_baudrate() can be static

[kbuild test robot]

Fixes: 547b9528309e ("Bluetooth: btrtl: add support for retrieving the UART settings")
Signed-off-by: kbuild test robot <fengguang.wu@intel.com>
---
 btrtl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index bc56ef7..99b77f6 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -591,7 +591,7 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
 }
 EXPORT_SYMBOL_GPL(btrtl_setup_realtek);
 
-unsigned int btrtl_convert_baudrate(u32 device_baudrate)
+static unsigned int btrtl_convert_baudrate(u32 device_baudrate)
 {
 	switch (device_baudrate) {
 	case 0x0252a00a:

Re: [PATCH 04/13] Bluetooth: btrtl: add support for retrieving the UART settings

[kbuild test robot]

Hi Martin,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth/master]
[also build test WARNING on v4.17-rc7 next-20180529]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/Bluetooth-Add-RTL8723BS-support/20180530-141115
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth.git master
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/bluetooth/btrtl.c:594:14: sparse: symbol 'btrtl_convert_baudrate' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[RFC PATCH] Bluetooth: btrtl: btrtl_convert_baudrate() can be static

[kbuild test robot]

Fixes: 547b9528309e ("Bluetooth: btrtl: add support for retrieving the UART settings")
Signed-off-by: kbuild test robot <fengguang.wu@intel.com>
---
 btrtl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index bc56ef7..99b77f6 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -591,7 +591,7 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
 }
 EXPORT_SYMBOL_GPL(btrtl_setup_realtek);
 
-unsigned int btrtl_convert_baudrate(u32 device_baudrate)
+static unsigned int btrtl_convert_baudrate(u32 device_baudrate)
 {
 	switch (device_baudrate) {
 	case 0x0252a00a:

Re: [PATCH 09/13] Bluetooth: hci_serdev: Fix HCI_UART_INIT_PENDING not working

[Rob Herring]

On Sun, May 27, 2018 at 2:04 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Init hci_uart->init_ready so that hci_uart_init_ready() works properly.

Why do you need to init in a wq? For serdev devices, probe is async
already. So my thought was this wouldn't be needed.

Rob

Re: [PATCH 09/13] Bluetooth: hci_serdev: Fix HCI_UART_INIT_PENDING not working

[Hans de Goede]

Hi,

On 30-05-18 15:25, Rob Herring wrote:
> On Sun, May 27, 2018 at 2:04 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Init hci_uart->init_ready so that hci_uart_init_ready() works properly.
> 
> Why do you need to init in a wq? For serdev devices, probe is async
> already. So my thought was this wouldn't be needed.

The H5 protocol needs to exchange a number of messages to
sync the 2 sides before it can accept any HCI commands, so
it uses the delayed-registration feature of the hci_ldisc code by
setting the HCI_UART_INIT_PENDING. When doing serdev based enumeration
we could do something different but that would require significant
changes to the hci_h5.c code.

Regards,

Hans

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

[Rob Herring]

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

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

[Hans de Goede]

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

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

[Marcel Holtmann]

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

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

[Hans de Goede]

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

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

[Marcel Holtmann]

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

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

[Hans de Goede]

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

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

[Marcel Holtmann]

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