[RFC v1 0/8] Realtek Bluetooth serdev support (H5 protocol)

[RFC v1 0/8] Realtek Bluetooth serdev support (H5 protocol)

[Martin Blumenstingl]

Hello,

I am sending this series because I want to get feedback.
please don't expect it to be perfect (yet). especially since it's
touching parts of the kernel I've not worked with before.

my goal was NOT to use the user-space initialization tool for the
Bluetooth part of one of my devices (which uses a Realtek RTL8723BS).
my hope was that I only had to hook up serdev support to hci_h5 and
re-use the setup function provided by btrtl. unfortunately it wasn't
that easy.

there are no datasheets for the RTL8723BS or RTL8723DS out there.
however, there are some userspace tools which go through the whole
initialization process - this is what I used as reference:
- RTL8723BS Bluetooth sources from [0]
- RTL8723DS Bluetooth sources from [1]

These modules require a firmware and a config file (both don't seem
to be compatible with what we have in linux-firmware at the moment).

since it's an RFC I also have some questions:
- I guess patch #1 ("serdev: implement parity configuration") and patch
  #7 ("Bluetooth: hci_serdev: remove the HCI_UART_INIT_PENDING check")
  can go on their own as these don't depend on anything else.
  should I split the reset into two series (btrtl + hci_h5 Realtek
  serdev support) or keep it as one?
- what about the Bluetooth firmware and config blobs? how to get them
  into the linux-firmware tree? maybe Larry Finger can help here :)
- what are <your name here> comments about this series?


Regards
Martin


[0] https://github.com/lwfinger/rtl8723bs_bt
[1] https://github.com/NextThingCo/rtl8723ds_bt

Martin Blumenstingl (8):
  serdev: implement parity configuration
  Bluetooth: btrtl: add MODULE_FIRMWARE declarations
  Bluetooth: btrtl: split the device initialization into smaller parts
  Bluetooth: btrtl: add support for retrieving the UART settings
  Bluetooth: btrtl: add support for the RTL8723BS and RTL8723DS chips
  Bluetooth: hci_h5: add support for Realtek UART Bluetooth modules
  Bluetooth: hci_serdev: remove the HCI_UART_INIT_PENDING check
  dt-bindings: net: bluetooth: add support for Realtek Bluetooth chips

 .../devicetree/bindings/net/realtek-bluetooth.txt  |  31 ++
 drivers/bluetooth/Kconfig                          |   1 +
 drivers/bluetooth/btrtl.c                          | 426 +++++++++++++++------
 drivers/bluetooth/btrtl.h                          |  46 +++
 drivers/bluetooth/hci_h5.c                         | 195 +++++++++-
 drivers/bluetooth/hci_serdev.c                     |   3 -
 drivers/tty/serdev/core.c                          |  12 +
 drivers/tty/serdev/serdev-ttyport.c                |  21 +
 include/linux/serdev.h                             |   3 +
 9 files changed, 620 insertions(+), 118 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/realtek-bluetooth.txt

-- 
2.15.0

[RFC v1 1/8] serdev: implement parity configuration

[Martin Blumenstingl]

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

Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 drivers/tty/serdev/core.c           | 12 ++++++++++++
 drivers/tty/serdev/serdev-ttyport.c | 21 +++++++++++++++++++++
 include/linux/serdev.h              |  3 +++
 3 files changed, 36 insertions(+)

diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index 1bef39828ca7..d327b02980f5 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -225,6 +225,18 @@ void serdev_device_set_flow_control(struct serdev_device *serdev, bool enable)
 }
 EXPORT_SYMBOL_GPL(serdev_device_set_flow_control);
 
+void serdev_device_set_parity(struct serdev_device *serdev, bool enable,
+			      bool odd)
+{
+	struct serdev_controller *ctrl = serdev->ctrl;
+
+	if (!ctrl || !ctrl->ops->set_parity)
+		return;
+
+	ctrl->ops->set_parity(ctrl, enable, odd);
+}
+EXPORT_SYMBOL_GPL(serdev_device_set_parity);
+
 void serdev_device_wait_until_sent(struct serdev_device *serdev, long timeout)
 {
 	struct serdev_controller *ctrl = serdev->ctrl;
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c
index ce7ad0acee7a..418548ba8992 100644
--- a/drivers/tty/serdev/serdev-ttyport.c
+++ b/drivers/tty/serdev/serdev-ttyport.c
@@ -170,6 +170,26 @@ static void ttyport_set_flow_control(struct serdev_controller *ctrl, bool enable
 	tty_set_termios(tty, &ktermios);
 }
 
+static void ttyport_set_parity(struct serdev_controller *ctrl, bool enable,
+			       bool odd)
+{
+	struct serport *serport = serdev_controller_get_drvdata(ctrl);
+	struct tty_struct *tty = serport->tty;
+	struct ktermios ktermios = tty->termios;
+
+	if (enable)
+		ktermios.c_cflag |= PARENB;
+	else
+		ktermios.c_cflag &= ~PARENB;
+
+	if (odd)
+		ktermios.c_cflag |= PARODD;
+	else
+		ktermios.c_cflag &= ~PARODD;
+
+	tty_set_termios(tty, &ktermios);
+}
+
 static void ttyport_wait_until_sent(struct serdev_controller *ctrl, long timeout)
 {
 	struct serport *serport = serdev_controller_get_drvdata(ctrl);
@@ -207,6 +227,7 @@ static const struct serdev_controller_ops ctrl_ops = {
 	.open = ttyport_open,
 	.close = ttyport_close,
 	.set_flow_control = ttyport_set_flow_control,
+	.set_parity = ttyport_set_parity,
 	.set_baudrate = ttyport_set_baudrate,
 	.wait_until_sent = ttyport_wait_until_sent,
 	.get_tiocm = ttyport_get_tiocm,
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index e69402d4a8ae..ccc541aa5c70 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -86,6 +86,7 @@ struct serdev_controller_ops {
 	int (*open)(struct serdev_controller *);
 	void (*close)(struct serdev_controller *);
 	void (*set_flow_control)(struct serdev_controller *, bool);
+	void (*set_parity)(struct serdev_controller *, bool, bool);
 	unsigned int (*set_baudrate)(struct serdev_controller *, unsigned int);
 	void (*wait_until_sent)(struct serdev_controller *, long);
 	int (*get_tiocm)(struct serdev_controller *);
@@ -195,6 +196,7 @@ int serdev_device_open(struct serdev_device *);
 void serdev_device_close(struct serdev_device *);
 unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int);
 void serdev_device_set_flow_control(struct serdev_device *, bool);
+void serdev_device_set_parity(struct serdev_device *, bool, bool);
 int serdev_device_write_buf(struct serdev_device *, const unsigned char *, size_t);
 void serdev_device_wait_until_sent(struct serdev_device *, long);
 int serdev_device_get_tiocm(struct serdev_device *);
@@ -237,6 +239,7 @@ static inline unsigned int serdev_device_set_baudrate(struct serdev_device *sdev
 	return 0;
 }
 static inline void serdev_device_set_flow_control(struct serdev_device *sdev, bool enable) {}
+static inline void serdev_device_set_parity(struct serdev_device *, bool, bool) {}
 static inline int serdev_device_write_buf(struct serdev_device *serdev,
 					  const unsigned char *buf,
 					  size_t count)
-- 
2.15.0

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

[RFC v1 2/8] Bluetooth: btrtl: add MODULE_FIRMWARE declarations

[Martin Blumenstingl]

This makes the firmware names show up in modinfo.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 drivers/bluetooth/btrtl.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 6e2ad748abba..3e5296287808 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -477,3 +477,12 @@ MODULE_AUTHOR("Daniel Drake <drake-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>");
 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.15.0

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

[RFC v1 3/8] Bluetooth: btrtl: split the device initialization into smaller parts

[Martin Blumenstingl]

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-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 drivers/bluetooth/btrtl.c | 277 +++++++++++++++++++++++++++-------------------
 drivers/bluetooth/btrtl.h |  21 ++++
 2 files changed, 184 insertions(+), 114 deletions(-)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 3e5296287808..07f96b8000f1 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -35,6 +35,15 @@
 #define RTL_ROM_LMP_8761A	0x8761
 #define RTL_ROM_LMP_8822B	0x8822
 
+struct btrtl_device_info {
+	u16 lmp_subver;
+	u8 rom_version;
+	u8 *fw_data;
+	int fw_len;
+	u8 *cfg_data;
+	int cfg_len;
+};
+
 static int rtl_read_rom_version(struct hci_dev *hdev, u8 *version)
 {
 	struct rtl_rom_version_evt *rom_version;
@@ -64,16 +73,16 @@ static int rtl_read_rom_version(struct hci_dev *hdev, u8 *version)
 	return 0;
 }
 
-static int rtl8723b_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
-				   const struct firmware *fw,
+static int rtl8723b_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;
@@ -90,15 +99,11 @@ static int rtl8723b_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
 		{ RTL_ROM_LMP_8822B, 8 },
 	};
 
-	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;
@@ -110,7 +115,7 @@ static int rtl8723b_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;
@@ -150,13 +155,14 @@ static int rtl8723b_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->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->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;
@@ -173,16 +179,16 @@ static int rtl8723b_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 +
@@ -193,20 +199,21 @@ static int rtl8723b_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;
 
@@ -268,7 +275,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;
@@ -287,95 +294,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 lmp_subver,
-				const char *fw_name)
+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;
-	bool config_needed = false;
-
-	switch (lmp_subver) {
-	case RTL_ROM_LMP_8723B:
-		cfg_name = "rtl_bt/rtl8723b_config.bin";
-		break;
-	case RTL_ROM_LMP_8821A:
-		cfg_name = "rtl_bt/rtl8821a_config.bin";
-		break;
-	case RTL_ROM_LMP_8761A:
-		cfg_name = "rtl_bt/rtl8761a_config.bin";
-		break;
-	case RTL_ROM_LMP_8822B:
-		cfg_name = "rtl_bt/rtl8822b_config.bin";
-		config_needed = true;
-		break;
-	default:
-		BT_ERR("%s: rtl: no config according to lmp_subver %04x",
-		       hdev->name, lmp_subver);
-		break;
-	}
 
-	if (cfg_name) {
-		cfg_sz = rtl_load_config(hdev, cfg_name, &cfg_buff);
-		if (cfg_sz < 0) {
-			cfg_sz = 0;
-			if (config_needed)
-				BT_ERR("Necessary config file %s not found\n",
-				       cfg_name);
-		}
-	} else
-		cfg_sz = 0;
-
-	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 = rtl8723b_parse_firmware(hdev, lmp_subver, fw, &fw_data);
+	ret = rtl8723b_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;
@@ -384,22 +333,18 @@ static int btrtl_setup_rtl8723b(struct hci_dev *hdev, u16 lmp_subver,
 		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;
 }
 
@@ -425,15 +370,34 @@ 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 lmp_subver;
+	bool cfg_needed = false, has_rom_version;
+	const char *cfg_name, *fw_name;
+	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 "
@@ -441,36 +405,121 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
 		    resp->hci_ver, resp->hci_rev,
 		    resp->lmp_ver, resp->lmp_subver);
 
-	lmp_subver = le16_to_cpu(resp->lmp_subver);
+	btrtl_dev->lmp_subver = le16_to_cpu(resp->lmp_subver);
+
 	kfree_skb(skb);
 
+	switch (btrtl_dev->lmp_subver) {
+	case RTL_ROM_LMP_8723A:
+	case RTL_ROM_LMP_3499:
+		fw_name = "rtl_bt/rtl8723a_fw.bin";
+		cfg_name = NULL;
+		has_rom_version = false;
+		break;
+	case RTL_ROM_LMP_8723B:
+		fw_name = "rtl_bt/rtl8723b_fw.bin";
+		cfg_name = "rtl_bt/rtl8723b_config.bin";
+		has_rom_version = true;
+		break;
+	case RTL_ROM_LMP_8821A:
+		fw_name = "rtl_bt/rtl8821a_fw.bin";
+		cfg_name = "rtl_bt/rtl8821a_config.bin";
+		has_rom_version = true;
+		break;
+	case RTL_ROM_LMP_8761A:
+		fw_name = "rtl_bt/rtl8761a_fw.bin";
+		cfg_name = "rtl_bt/rtl8761a_config.bin";
+		has_rom_version = true;
+		break;
+	case RTL_ROM_LMP_8822B:
+		fw_name = "rtl_bt/rtl8822b_fw.bin";
+		cfg_name = "rtl_bt/rtl8822b_config.bin";
+		has_rom_version = true;
+		cfg_needed = true;
+		break;
+	default:
+		bt_dev_info(hdev, "rtl: assuming no firmware upload needed");
+		return btrtl_dev;
+	}
+
+	if (has_rom_version) {
+		ret = rtl_read_rom_version(hdev, &btrtl_dev->rom_version);
+		if (ret)
+			goto err_free;
+	}
+
+	if (fw_name) {
+		btrtl_dev->fw_len = rtl_load_file(hdev, fw_name,
+						  &btrtl_dev->fw_data);
+		if (btrtl_dev->fw_len < 0) {
+			bt_dev_err(hdev, "firmware file %s not found\n",
+				   fw_name);
+			ret = btrtl_dev->fw_len;
+			goto err_free;
+		}
+	}
+
+	if (cfg_name) {
+		btrtl_dev->cfg_len = rtl_load_file(hdev, cfg_name,
+						   &btrtl_dev->cfg_data);
+		if (cfg_needed && btrtl_dev->cfg_len <= 0) {
+			bt_dev_err(hdev,
+				   "mandatory config file %s not found\n",
+				   cfg_name);
+			ret = btrtl_dev->fw_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->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:
-		return btrtl_setup_rtl8723b(hdev, lmp_subver,
-					    "rtl_bt/rtl8723b_fw.bin");
 	case RTL_ROM_LMP_8821A:
-		return btrtl_setup_rtl8723b(hdev, lmp_subver,
-					    "rtl_bt/rtl8821a_fw.bin");
 	case RTL_ROM_LMP_8761A:
-		return btrtl_setup_rtl8723b(hdev, lmp_subver,
-					    "rtl_bt/rtl8761a_fw.bin");
 	case RTL_ROM_LMP_8822B:
-		return btrtl_setup_rtl8723b(hdev, lmp_subver,
-					    "rtl_bt/rtl8822b_fw.bin");
+		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-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>");
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.15.0

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

[RFC v1 4/8] Bluetooth: btrtl: add support for retrieving the UART settings

[Martin Blumenstingl]

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-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 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 07f96b8000f1..45b872f5ad22 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
 
 struct btrtl_device_info {
 	u16 lmp_subver;
@@ -522,6 +523,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-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>");
 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.15.0

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

[RFC v1 5/8] Bluetooth: btrtl: add support for the RTL8723BS and RTL8723DS chips

[Martin Blumenstingl]

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-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 drivers/bluetooth/btrtl.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 45b872f5ad22..d896f9421250 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -418,9 +418,33 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
 		has_rom_version = false;
 		break;
 	case RTL_ROM_LMP_8723B:
-		fw_name = "rtl_bt/rtl8723b_fw.bin";
-		cfg_name = "rtl_bt/rtl8723b_config.bin";
+		/* all variants support reading the ROM version */
 		has_rom_version = true;
+
+		/*
+		 * RTL8723 devices exist in different variants:
+		 * - RTL8723BS (SDIO chip with UART Bluetooth)
+		 * - RTL8723DS (SDIO chip with UART Bluetooth)
+		 * - for backwards-compatibility everything else is assumed to
+		 *   be an RTL8723B communicating over USB
+		 *
+		 * Only UART devices really need the config because that
+		 * contains the UART speed / flow control settings.
+		 */
+		if (hdev->bus == HCI_UART && resp->hci_ver == 6 &&
+		    le16_to_cpu(resp->hci_rev) == 0xb) {
+			fw_name = "rtl_bt/rtl8723bs_fw.bin";
+			cfg_name = "rtl_bt/rtl8723bs_config.bin";
+			cfg_needed = true;
+		} else if (hdev->bus == HCI_UART && resp->hci_ver == 8 &&
+			   le16_to_cpu(resp->hci_rev) == 0xd) {
+			fw_name = "rtl_bt/rtl8723ds_fw.bin";
+			cfg_name = "rtl_bt/rtl872ds_config.bin";
+			cfg_needed = true;
+		} else {
+			fw_name = "rtl_bt/rtl8723b_fw.bin";
+			cfg_name = "rtl_bt/rtl8723b_config.bin";
+		}
 		break;
 	case RTL_ROM_LMP_8821A:
 		fw_name = "rtl_bt/rtl8821a_fw.bin";
@@ -641,6 +665,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.15.0

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

Re: [RFC v1 5/8] Bluetooth: btrtl: add support for the RTL8723BS and RTL8723DS chips

[Marcel Holtmann]

Hi Martin,

> 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-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> ---
> drivers/bluetooth/btrtl.c | 32 ++++++++++++++++++++++++++++++--
> 1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index 45b872f5ad22..d896f9421250 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -418,9 +418,33 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
> 		has_rom_version = false;
> 		break;
> 	case RTL_ROM_LMP_8723B:
> -		fw_name = "rtl_bt/rtl8723b_fw.bin";
> -		cfg_name = "rtl_bt/rtl8723b_config.bin";
> +		/* all variants support reading the ROM version */
> 		has_rom_version = true;
> +
> +		/*
> +		 * RTL8723 devices exist in different variants:
> +		 * - RTL8723BS (SDIO chip with UART Bluetooth)
> +		 * - RTL8723DS (SDIO chip with UART Bluetooth)
> +		 * - for backwards-compatibility everything else is assumed to
> +		 *   be an RTL8723B communicating over USB
> +		 *
> +		 * Only UART devices really need the config because that
> +		 * contains the UART speed / flow control settings.
> +		 */
> +		if (hdev->bus == HCI_UART && resp->hci_ver == 6 &&
> +		    le16_to_cpu(resp->hci_rev) == 0xb) {
> +			fw_name = "rtl_bt/rtl8723bs_fw.bin";
> +			cfg_name = "rtl_bt/rtl8723bs_config.bin";
> +			cfg_needed = true;
> +		} else if (hdev->bus == HCI_UART && resp->hci_ver == 8 &&
> +			   le16_to_cpu(resp->hci_rev) == 0xd) {
> +			fw_name = "rtl_bt/rtl8723ds_fw.bin";
> +			cfg_name = "rtl_bt/rtl872ds_config.bin";
> +			cfg_needed = true;
> +		} else {
> +			fw_name = "rtl_bt/rtl8723b_fw.bin";
> +			cfg_name = "rtl_bt/rtl8723b_config.bin";
> +		}

so the main question is why is this a config file in the first place? So far all other drivers have expressed UART settings via DT (or even via ACPI). If we are just getting the baudrates, then better have this in DT. Since otherwise we end up with board specific filesystems again where different config files need to be installed. That is really not useful in the end.

What is actually in these config files? Can we have some parsing and friendly display tool in bluez.git like we have for other manufactures.

Regards

Marcel

Re: [RFC v1 5/8] Bluetooth: btrtl: add support for the RTL8723BS and RTL8723DS chips

[Martin Blumenstingl]

Hi Marcel,

On Sun, Nov 19, 2017 at 9:25 AM, Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org> wrote:
> Hi Martin,
>
>> 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-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>> ---
>> drivers/bluetooth/btrtl.c | 32 ++++++++++++++++++++++++++++++--
>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
>> index 45b872f5ad22..d896f9421250 100644
>> --- a/drivers/bluetooth/btrtl.c
>> +++ b/drivers/bluetooth/btrtl.c
>> @@ -418,9 +418,33 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
>>               has_rom_version = false;
>>               break;
>>       case RTL_ROM_LMP_8723B:
>> -             fw_name = "rtl_bt/rtl8723b_fw.bin";
>> -             cfg_name = "rtl_bt/rtl8723b_config.bin";
>> +             /* all variants support reading the ROM version */
>>               has_rom_version = true;
>> +
>> +             /*
>> +              * RTL8723 devices exist in different variants:
>> +              * - RTL8723BS (SDIO chip with UART Bluetooth)
>> +              * - RTL8723DS (SDIO chip with UART Bluetooth)
>> +              * - for backwards-compatibility everything else is assumed to
>> +              *   be an RTL8723B communicating over USB
>> +              *
>> +              * Only UART devices really need the config because that
>> +              * contains the UART speed / flow control settings.
>> +              */
>> +             if (hdev->bus == HCI_UART && resp->hci_ver == 6 &&
>> +                 le16_to_cpu(resp->hci_rev) == 0xb) {
>> +                     fw_name = "rtl_bt/rtl8723bs_fw.bin";
>> +                     cfg_name = "rtl_bt/rtl8723bs_config.bin";
>> +                     cfg_needed = true;
>> +             } else if (hdev->bus == HCI_UART && resp->hci_ver == 8 &&
>> +                        le16_to_cpu(resp->hci_rev) == 0xd) {
>> +                     fw_name = "rtl_bt/rtl8723ds_fw.bin";
>> +                     cfg_name = "rtl_bt/rtl872ds_config.bin";
>> +                     cfg_needed = true;
>> +             } else {
>> +                     fw_name = "rtl_bt/rtl8723b_fw.bin";
>> +                     cfg_name = "rtl_bt/rtl8723b_config.bin";
>> +             }
>
> so the main question is why is this a config file in the first place? So far all other drivers have expressed UART settings via DT (or even via ACPI). If we are just getting the baudrates, then better have this in DT. Since otherwise we end up with board specific filesystems again where different config files need to be installed. That is really not useful in the end.
this is an excellent question (and also the reason why it's an "RFC" patch)!
I'll try to figure out whether:
a) we can skip the config file completely
b) we can generate the config file in-memory
c) we have to stick with this config file

> What is actually in these config files? Can we have some parsing and friendly display tool in bluez.git like we have for other manufactures.
to my knowledge this config file contains:
- the baudrate
- whether flow control is enabled or disabled
- (in some cases) the local-bd-address

do you mean a tool that is similar to bluez.git tools/nokfw.c?


Regards
Martin


[0] https://github.com/NextThingCo/rtl8723ds_bt/blob/f56ef37217665070e574253d23a595ee1ca5ca23/rtk_hciattach/hciattach_rtk.c#L1781
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v1 5/8] Bluetooth: btrtl: add support for the RTL8723BS and RTL8723DS chips

[Marcel Holtmann]

Hi Martin,

>>> 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-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>>> ---
>>> drivers/bluetooth/btrtl.c | 32 ++++++++++++++++++++++++++++++--
>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
>>> index 45b872f5ad22..d896f9421250 100644
>>> --- a/drivers/bluetooth/btrtl.c
>>> +++ b/drivers/bluetooth/btrtl.c
>>> @@ -418,9 +418,33 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
>>>              has_rom_version = false;
>>>              break;
>>>      case RTL_ROM_LMP_8723B:
>>> -             fw_name = "rtl_bt/rtl8723b_fw.bin";
>>> -             cfg_name = "rtl_bt/rtl8723b_config.bin";
>>> +             /* all variants support reading the ROM version */
>>>              has_rom_version = true;
>>> +
>>> +             /*
>>> +              * RTL8723 devices exist in different variants:
>>> +              * - RTL8723BS (SDIO chip with UART Bluetooth)
>>> +              * - RTL8723DS (SDIO chip with UART Bluetooth)
>>> +              * - for backwards-compatibility everything else is assumed to
>>> +              *   be an RTL8723B communicating over USB
>>> +              *
>>> +              * Only UART devices really need the config because that
>>> +              * contains the UART speed / flow control settings.
>>> +              */
>>> +             if (hdev->bus == HCI_UART && resp->hci_ver == 6 &&
>>> +                 le16_to_cpu(resp->hci_rev) == 0xb) {
>>> +                     fw_name = "rtl_bt/rtl8723bs_fw.bin";
>>> +                     cfg_name = "rtl_bt/rtl8723bs_config.bin";
>>> +                     cfg_needed = true;
>>> +             } else if (hdev->bus == HCI_UART && resp->hci_ver == 8 &&
>>> +                        le16_to_cpu(resp->hci_rev) == 0xd) {
>>> +                     fw_name = "rtl_bt/rtl8723ds_fw.bin";
>>> +                     cfg_name = "rtl_bt/rtl872ds_config.bin";
>>> +                     cfg_needed = true;
>>> +             } else {
>>> +                     fw_name = "rtl_bt/rtl8723b_fw.bin";
>>> +                     cfg_name = "rtl_bt/rtl8723b_config.bin";
>>> +             }
>> 
>> so the main question is why is this a config file in the first place? So far all other drivers have expressed UART settings via DT (or even via ACPI). If we are just getting the baudrates, then better have this in DT. Since otherwise we end up with board specific filesystems again where different config files need to be installed. That is really not useful in the end.
> this is an excellent question (and also the reason why it's an "RFC" patch)!
> I'll try to figure out whether:
> a) we can skip the config file completely
> b) we can generate the config file in-memory
> c) we have to stick with this config file
> 
>> What is actually in these config files? Can we have some parsing and friendly display tool in bluez.git like we have for other manufactures.
> to my knowledge this config file contains:
> - the baudrate
> - whether flow control is enabled or disabled

these two are better done as part of the DT.

> - (in some cases) the local-bd-address

There is essentially DT support for that as well, but we also have Set Public Address mgmt command to handle this. Then again, first of all the hdev->set_bdaddr callback needs to be supported for that.

> do you mean a tool that is similar to bluez.git tools/nokfw.c?

Yes.

> [0] https://github.com/NextThingCo/rtl8723ds_bt/blob/f56ef37217665070e574253d23a595ee1ca5ca23/rtk_hciattach/hciattach_rtk.c#L1781

Please never ever make me look at code that defined struct sk_buff in userspace ;)

Regards

Marcel

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

Re: [RFC v1 5/8] Bluetooth: btrtl: add support for the RTL8723BS and RTL8723DS chips

[Martin Blumenstingl]

Hi Marcel,

On Sun, Nov 19, 2017 at 10:17 PM, Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org> wrote:
> Hi Martin,
>
>>>> 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-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>>>> ---
>>>> drivers/bluetooth/btrtl.c | 32 ++++++++++++++++++++++++++++++--
>>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
>>>> index 45b872f5ad22..d896f9421250 100644
>>>> --- a/drivers/bluetooth/btrtl.c
>>>> +++ b/drivers/bluetooth/btrtl.c
>>>> @@ -418,9 +418,33 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
>>>>              has_rom_version = false;
>>>>              break;
>>>>      case RTL_ROM_LMP_8723B:
>>>> -             fw_name = "rtl_bt/rtl8723b_fw.bin";
>>>> -             cfg_name = "rtl_bt/rtl8723b_config.bin";
>>>> +             /* all variants support reading the ROM version */
>>>>              has_rom_version = true;
>>>> +
>>>> +             /*
>>>> +              * RTL8723 devices exist in different variants:
>>>> +              * - RTL8723BS (SDIO chip with UART Bluetooth)
>>>> +              * - RTL8723DS (SDIO chip with UART Bluetooth)
>>>> +              * - for backwards-compatibility everything else is assumed to
>>>> +              *   be an RTL8723B communicating over USB
>>>> +              *
>>>> +              * Only UART devices really need the config because that
>>>> +              * contains the UART speed / flow control settings.
>>>> +              */
>>>> +             if (hdev->bus == HCI_UART && resp->hci_ver == 6 &&
>>>> +                 le16_to_cpu(resp->hci_rev) == 0xb) {
>>>> +                     fw_name = "rtl_bt/rtl8723bs_fw.bin";
>>>> +                     cfg_name = "rtl_bt/rtl8723bs_config.bin";
>>>> +                     cfg_needed = true;
>>>> +             } else if (hdev->bus == HCI_UART && resp->hci_ver == 8 &&
>>>> +                        le16_to_cpu(resp->hci_rev) == 0xd) {
>>>> +                     fw_name = "rtl_bt/rtl8723ds_fw.bin";
>>>> +                     cfg_name = "rtl_bt/rtl872ds_config.bin";
>>>> +                     cfg_needed = true;
>>>> +             } else {
>>>> +                     fw_name = "rtl_bt/rtl8723b_fw.bin";
>>>> +                     cfg_name = "rtl_bt/rtl8723b_config.bin";
>>>> +             }
>>>
>>> so the main question is why is this a config file in the first place? So far all other drivers have expressed UART settings via DT (or even via ACPI). If we are just getting the baudrates, then better have this in DT. Since otherwise we end up with board specific filesystems again where different config files need to be installed. That is really not useful in the end.
>> this is an excellent question (and also the reason why it's an "RFC" patch)!
>> I'll try to figure out whether:
>> a) we can skip the config file completely
skipping the config blob causes it to time out at the end of the
firmware download (where the config seems to be applied internally)

>> b) we can generate the config file in-memory
I tried this as well with a very minimal version (where only the
"known" bits are used)
unfortunately this shows the same symptoms as skipping the config blob
I'll try experimenting with this to see if I can get it to work somehow

>> c) we have to stick with this config file
I'm afraid this would be our last option if I can't get b) working
a datasheet for these chips or some advice from someone who has access
to the datasheet/knowledge about the internals would be of great help

>>
>>> What is actually in these config files? Can we have some parsing and friendly display tool in bluez.git like we have for other manufactures.
>> to my knowledge this config file contains:
>> - the baudrate
>> - whether flow control is enabled or disabled
>
> these two are better done as part of the DT.
agreed, I'd also rather have this in DT than some blob

>> - (in some cases) the local-bd-address
>
> There is essentially DT support for that as well, but we also have Set Public Address mgmt command to handle this. Then again, first of all the hdev->set_bdaddr callback needs to be supported for that.
>
>> do you mean a tool that is similar to bluez.git tools/nokfw.c?
>
> Yes.
>
>> [0] https://github.com/NextThingCo/rtl8723ds_bt/blob/f56ef37217665070e574253d23a595ee1ca5ca23/rtk_hciattach/hciattach_rtk.c#L1781
>
> Please never ever make me look at code that defined struct sk_buff in userspace ;)
sorry for not putting a huge warning/disclaimer in front of that link!
as bad as it sounds: this is all I have as reference :(


Regards
Martin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v1 5/8] Bluetooth: btrtl: add support for the RTL8723BS and RTL8723DS chips

[Emil Lenngren]

Hi,

2017-11-26 23:23 GMT+01:00 Martin Blumenstingl
<martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>:
>>> c) we have to stick with this config file
> I'm afraid this would be our last option if I can't get b) working
> a datasheet for these chips or some advice from someone who has access
> to the datasheet/knowledge about the internals would be of great help
>

FYI I happen to have the confidential 37 pages long datasheet for
rtl8723ds, but unfortunately it doesn't contain any info about the
format of the config file. I suggest you should reach out to the guys
at NextThing and ask if they have some documentation from Realtek.

/Emil

Re: [RFC v1 5/8] Bluetooth: btrtl: add support for the RTL8723BS and RTL8723DS chips

[Marcel Holtmann]

Hi Martin,

>>>>> 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-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>>>>> ---
>>>>> drivers/bluetooth/btrtl.c | 32 ++++++++++++++++++++++++++++++--
>>>>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>>>> 
>>>>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
>>>>> index 45b872f5ad22..d896f9421250 100644
>>>>> --- a/drivers/bluetooth/btrtl.c
>>>>> +++ b/drivers/bluetooth/btrtl.c
>>>>> @@ -418,9 +418,33 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
>>>>>             has_rom_version = false;
>>>>>             break;
>>>>>     case RTL_ROM_LMP_8723B:
>>>>> -             fw_name = "rtl_bt/rtl8723b_fw.bin";
>>>>> -             cfg_name = "rtl_bt/rtl8723b_config.bin";
>>>>> +             /* all variants support reading the ROM version */
>>>>>             has_rom_version = true;
>>>>> +
>>>>> +             /*
>>>>> +              * RTL8723 devices exist in different variants:
>>>>> +              * - RTL8723BS (SDIO chip with UART Bluetooth)
>>>>> +              * - RTL8723DS (SDIO chip with UART Bluetooth)
>>>>> +              * - for backwards-compatibility everything else is assumed to
>>>>> +              *   be an RTL8723B communicating over USB
>>>>> +              *
>>>>> +              * Only UART devices really need the config because that
>>>>> +              * contains the UART speed / flow control settings.
>>>>> +              */
>>>>> +             if (hdev->bus == HCI_UART && resp->hci_ver == 6 &&
>>>>> +                 le16_to_cpu(resp->hci_rev) == 0xb) {
>>>>> +                     fw_name = "rtl_bt/rtl8723bs_fw.bin";
>>>>> +                     cfg_name = "rtl_bt/rtl8723bs_config.bin";
>>>>> +                     cfg_needed = true;
>>>>> +             } else if (hdev->bus == HCI_UART && resp->hci_ver == 8 &&
>>>>> +                        le16_to_cpu(resp->hci_rev) == 0xd) {
>>>>> +                     fw_name = "rtl_bt/rtl8723ds_fw.bin";
>>>>> +                     cfg_name = "rtl_bt/rtl872ds_config.bin";
>>>>> +                     cfg_needed = true;
>>>>> +             } else {
>>>>> +                     fw_name = "rtl_bt/rtl8723b_fw.bin";
>>>>> +                     cfg_name = "rtl_bt/rtl8723b_config.bin";
>>>>> +             }
>>>> 
>>>> so the main question is why is this a config file in the first place? So far all other drivers have expressed UART settings via DT (or even via ACPI). If we are just getting the baudrates, then better have this in DT. Since otherwise we end up with board specific filesystems again where different config files need to be installed. That is really not useful in the end.
>>> this is an excellent question (and also the reason why it's an "RFC" patch)!
>>> I'll try to figure out whether:
>>> a) we can skip the config file completely
> skipping the config blob causes it to time out at the end of the
> firmware download (where the config seems to be applied internally)
> 
>>> b) we can generate the config file in-memory
> I tried this as well with a very minimal version (where only the
> "known" bits are used)
> unfortunately this shows the same symptoms as skipping the config blob
> I'll try experimenting with this to see if I can get it to work somehow
> 
>>> c) we have to stick with this config file
> I'm afraid this would be our last option if I can't get b) working
> a datasheet for these chips or some advice from someone who has access
> to the datasheet/knowledge about the internals would be of great help

since they are all HCI commands, lets add tracing support in btmon for these and then check which one is needed and which one can be done via DT information. It might be as simple as that the chip needs a special reset command and the coded it in the config blob. I have seen some manufactures do that.

For starters just run btmon -w trace.log before you load the module (or attach the device) and then see what goes on during loading phase.

Regards

Marcel

[RFC v1 6/8] Bluetooth: hci_h5: add support for Realtek UART Bluetooth modules

[Martin Blumenstingl]

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

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

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

Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 drivers/bluetooth/Kconfig  |   1 +
 drivers/bluetooth/hci_h5.c | 195 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 195 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 60e1c7d6986d..3001f1200c72 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -146,6 +146,7 @@ config BT_HCIUART_LL
 config BT_HCIUART_3WIRE
 	bool "Three-wire UART (H5) protocol support"
 	depends on BT_HCIUART
+	select BT_RTL if SERIAL_DEV_BUS
 	help
 	  The HCI Three-wire UART Transport Layer makes it possible to
 	  user the Bluetooth HCI over a serial port interface. The HCI
diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index 6a8d0d06aba7..7d584e5928bf 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -28,7 +28,14 @@
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/serdev.h>
+
 #include "hci_uart.h"
+#include "btrtl.h"
 
 #define HCI_3WIRE_ACK_PKT	0
 #define HCI_3WIRE_LINK_PKT	15
@@ -97,6 +104,13 @@ struct h5 {
 	} sleep;
 };
 
+struct h5_device {
+	struct hci_uart hu;
+	struct gpio_desc *disable_gpio;
+	struct gpio_desc *reset_gpio;
+	int (*vendor_setup)(struct h5_device *h5_dev);
+};
+
 static void h5_reset_rx(struct h5 *h5);
 
 static void h5_link_control(struct hci_uart *hu, const void *data, size_t len)
@@ -190,6 +204,7 @@ static int h5_open(struct hci_uart *hu)
 {
 	struct h5 *h5;
 	const unsigned char sync[] = { 0x01, 0x7e };
+	int err;
 
 	BT_DBG("hu %p", hu);
 
@@ -210,6 +225,14 @@ static int h5_open(struct hci_uart *hu)
 
 	h5->tx_win = H5_TX_WIN_MAX;
 
+	if (hu->serdev) {
+		err = serdev_device_open(hu->serdev);
+		if (err) {
+			bt_dev_err(hu->hdev, "failed to open serdev: %d", err);
+			return err;
+		}
+	}
+
 	set_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags);
 
 	/* Send initial sync request */
@@ -219,6 +242,23 @@ static int h5_open(struct hci_uart *hu)
 	return 0;
 }
 
+static int h5_setup(struct hci_uart *hu)
+{
+	int err;
+	struct h5_device *h5_dev;
+
+	if (!hu->serdev)
+		return 0;
+
+	h5_dev = serdev_device_get_drvdata(hu->serdev);
+
+	err = h5_dev->vendor_setup(h5_dev);
+	if (err)
+		return err;
+
+	return 0;
+}
+
 static int h5_close(struct hci_uart *hu)
 {
 	struct h5 *h5 = hu->priv;
@@ -229,6 +269,15 @@ static int h5_close(struct hci_uart *hu)
 	skb_queue_purge(&h5->rel);
 	skb_queue_purge(&h5->unrel);
 
+	if (hu->serdev) {
+		struct h5_device *h5_dev;
+
+		h5_dev = serdev_device_get_drvdata(hu->serdev);
+		gpiod_set_value_cansleep(h5_dev->disable_gpio, 1);
+
+		serdev_device_close(hu->serdev);
+	}
+
 	kfree(h5);
 
 	return 0;
@@ -316,7 +365,10 @@ static void h5_handle_internal_rx(struct hci_uart *hu)
 			h5->tx_win = (data[2] & 0x07);
 		BT_DBG("Three-wire init complete. tx_win %u", h5->tx_win);
 		h5->state = H5_ACTIVE;
-		hci_uart_init_ready(hu);
+
+		/* serdev does not support the "init_ready" signal */
+		if (!hu->serdev)
+			hci_uart_init_ready(hu);
 		return;
 	} else if (memcmp(data, sleep_req, 2) == 0) {
 		BT_DBG("Peer went to sleep");
@@ -739,10 +791,147 @@ static int h5_flush(struct hci_uart *hu)
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_SERIAL_DEV_BUS)
+static int h5_setup_realtek(struct h5_device *h5_dev)
+{
+	struct hci_uart *hu = &h5_dev->hu;
+	int err = 0, retry = 3;
+	struct sk_buff *skb;
+	struct btrtl_device_info *btrtl_dev;
+	__le32 baudrate_data;
+	u32 device_baudrate;
+	unsigned int controller_baudrate;
+	bool flow_control;
+
+	/* devices always start with flow control disabled and even parity */
+	serdev_device_set_flow_control(hu->serdev, false);
+	serdev_device_set_parity(hu->serdev, true, false);
+
+	do {
+		/* Configure BT_DISn and BT_RST_N to LOW state */
+		gpiod_set_value_cansleep(h5_dev->reset_gpio, 1);
+		gpiod_set_value_cansleep(h5_dev->disable_gpio, 1);
+		msleep(500);
+		gpiod_set_value_cansleep(h5_dev->reset_gpio, 0);
+		gpiod_set_value_cansleep(h5_dev->disable_gpio, 0);
+		msleep(500);
+
+		btrtl_dev = btrtl_initialize(hu->hdev);
+		if (!IS_ERR(btrtl_dev))
+			break;
+
+		/* Toggle BT_DISn and retry */
+	} while (retry--);
+
+	if (IS_ERR(btrtl_dev))
+		return PTR_ERR(btrtl_dev);
+
+	err = btrtl_get_uart_settings(hu->hdev, btrtl_dev,
+				      &controller_baudrate, &device_baudrate,
+				      &flow_control);
+	if (err)
+		goto out_free;
+
+	baudrate_data = cpu_to_le32(device_baudrate);
+	skb = __hci_cmd_sync(hu->hdev, 0xfc17, sizeof(baudrate_data),
+			     &baudrate_data, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		bt_dev_err(hu->hdev, "set baud rate command failed");
+		err = -PTR_ERR(skb);
+		goto out_free;
+	} else {
+		kfree_skb(skb);
+	}
+
+	msleep(500);
+
+	serdev_device_set_baudrate(hu->serdev, controller_baudrate);
+	serdev_device_set_flow_control(hu->serdev, flow_control);
+
+	err = btrtl_download_firmware(hu->hdev, btrtl_dev);
+
+out_free:
+	btrtl_free(btrtl_dev);
+
+	return err;
+}
+
+static const struct hci_uart_proto h5p;
+
+static int hci_h5_probe(struct serdev_device *serdev)
+{
+	struct hci_uart *hu;
+	struct h5_device *h5_dev;
+
+	h5_dev = devm_kzalloc(&serdev->dev, sizeof(*h5_dev), GFP_KERNEL);
+	if (!h5_dev)
+		return -ENOMEM;
+
+	hu = &h5_dev->hu;
+	hu->serdev = serdev;
+
+	serdev_device_set_drvdata(serdev, h5_dev);
+
+	h5_dev->vendor_setup = of_device_get_match_data(&serdev->dev);
+
+	h5_dev->disable_gpio = devm_gpiod_get_optional(&serdev->dev, "disable",
+						       GPIOD_OUT_LOW);
+	if (IS_ERR(h5_dev->disable_gpio))
+		return PTR_ERR(h5_dev->disable_gpio);
+
+	h5_dev->reset_gpio = devm_gpiod_get_optional(&serdev->dev, "reset",
+						     GPIOD_OUT_LOW);
+	if (IS_ERR(h5_dev->reset_gpio))
+		return PTR_ERR(h5_dev->reset_gpio);
+
+	hci_uart_set_speeds(hu, 115200, 0);
+
+	return hci_uart_register_device(hu, &h5p);
+}
+
+static void hci_h5_remove(struct serdev_device *serdev)
+{
+	struct h5_device *h5_dev = serdev_device_get_drvdata(serdev);
+	struct hci_uart *hu = &h5_dev->hu;
+	struct hci_dev *hdev = hu->hdev;
+
+	cancel_work_sync(&hu->write_work);
+
+	hci_unregister_dev(hdev);
+	hci_free_dev(hdev);
+	hu->proto->close(hu);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id hci_h5_of_match[] = {
+	{
+		.compatible = "realtek,rtl8723bs-bluetooth",
+		.data = h5_setup_realtek
+	},
+	{
+		.compatible = "realtek,rtl8723ds-bluetooth",
+		.data = h5_setup_realtek
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, hci_h5_of_match);
+#endif
+
+static struct serdev_device_driver hci_h5_drv = {
+	.driver		= {
+		.name	= "hci-h5",
+		.of_match_table = of_match_ptr(hci_h5_of_match),
+	},
+	.probe	= hci_h5_probe,
+	.remove	= hci_h5_remove,
+};
+#endif
+
 static const struct hci_uart_proto h5p = {
 	.id		= HCI_UART_3WIRE,
 	.name		= "Three-wire (H5)",
 	.open		= h5_open,
+	.setup		= h5_setup,
 	.close		= h5_close,
 	.recv		= h5_recv,
 	.enqueue	= h5_enqueue,
@@ -752,10 +941,14 @@ static const struct hci_uart_proto h5p = {
 
 int __init h5_init(void)
 {
+	serdev_device_driver_register(&hci_h5_drv);
+
 	return hci_uart_register_proto(&h5p);
 }
 
 int __exit h5_deinit(void)
 {
+	serdev_device_driver_unregister(&hci_h5_drv);
+
 	return hci_uart_unregister_proto(&h5p);
 }
-- 
2.15.0

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

Re: [RFC v1 6/8] Bluetooth: hci_h5: add support for Realtek UART Bluetooth modules

[Marcel Holtmann]

Hi Martin,

> Realtek RTL8723BS and RTL8723DS are SDIO wifi chips with an embedded
> Bluetooth controller which connects to the host via UART.
> The H5 protocol is used for communication between host and device.
> 
> The Realtek "rtl8723bs_bt" and "rtl8723ds_bt" userspace Bluetooth UART
> initialization tools (rtk_hciattach) use the following sequence:
> 1) send H5 sync pattern (already supported by hci_h5)
> 2) get LMP version (already supported by btrtl)
> 3) get ROM version (already supported by btrtl)
> 4) load the firmware and config for the current chipset (already
>   supported by btrtl)
> 5) read UART settings from the config blob (already supported by btrtl)
> 6) send UART settings via a vendor command to the device (which changes
>   the baudrate of the device and enables or disables flow control
>   depending on the config)
> 7) change the baudrate and flow control settings on the host
> 8) send the firmware and config blob to the device (already supported by
>   btrtl)
> 
> This uses the serdev library as well as the existing btrtl driver to
> initialize the Bluetooth functionality, which consists of:
> - identifying the device and loading the corresponding firmware and
>  config blobs (steps #2, #3 and #4)
> - configuring the baudrate and flow control (steps #6 and #7)
> - uploading the firmware to the device (step #8)
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> ---
> drivers/bluetooth/Kconfig  |   1 +
> drivers/bluetooth/hci_h5.c | 195 ++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 195 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 60e1c7d6986d..3001f1200c72 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -146,6 +146,7 @@ config BT_HCIUART_LL
> config BT_HCIUART_3WIRE
> 	bool "Three-wire UART (H5) protocol support"
> 	depends on BT_HCIUART
> +	select BT_RTL if SERIAL_DEV_BUS
> 	help
> 	  The HCI Three-wire UART Transport Layer makes it possible to
> 	  user the Bluetooth HCI over a serial port interface. The HCI
> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
> index 6a8d0d06aba7..7d584e5928bf 100644
> --- a/drivers/bluetooth/hci_h5.c
> +++ b/drivers/bluetooth/hci_h5.c
> @@ -28,7 +28,14 @@
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> 
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/serdev.h>
> +
> #include "hci_uart.h"
> +#include "btrtl.h"
> 
> #define HCI_3WIRE_ACK_PKT	0
> #define HCI_3WIRE_LINK_PKT	15
> @@ -97,6 +104,13 @@ struct h5 {
> 	} sleep;
> };
> 
> +struct h5_device {
> +	struct hci_uart hu;
> +	struct gpio_desc *disable_gpio;
> +	struct gpio_desc *reset_gpio;
> +	int (*vendor_setup)(struct h5_device *h5_dev);
> +};
> +
> static void h5_reset_rx(struct h5 *h5);
> 
> static void h5_link_control(struct hci_uart *hu, const void *data, size_t len)
> @@ -190,6 +204,7 @@ static int h5_open(struct hci_uart *hu)
> {
> 	struct h5 *h5;
> 	const unsigned char sync[] = { 0x01, 0x7e };
> +	int err;
> 
> 	BT_DBG("hu %p", hu);
> 
> @@ -210,6 +225,14 @@ static int h5_open(struct hci_uart *hu)
> 
> 	h5->tx_win = H5_TX_WIN_MAX;
> 
> +	if (hu->serdev) {
> +		err = serdev_device_open(hu->serdev);
> +		if (err) {
> +			bt_dev_err(hu->hdev, "failed to open serdev: %d", err);
> +			return err;
> +		}
> +	}
> +
> 	set_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags);
> 
> 	/* Send initial sync request */
> @@ -219,6 +242,23 @@ static int h5_open(struct hci_uart *hu)
> 	return 0;
> }
> 
> +static int h5_setup(struct hci_uart *hu)
> +{
> +	int err;
> +	struct h5_device *h5_dev;
> +
> +	if (!hu->serdev)
> +		return 0;
> +
> +	h5_dev = serdev_device_get_drvdata(hu->serdev);
> +
> +	err = h5_dev->vendor_setup(h5_dev);
> +	if (err)
> +		return err;

	if (h5_dev->vendor_setup)
		return h5_dev->vendor_setup(h5_dev);

> +
> +	return 0;
> +}
> +
> static int h5_close(struct hci_uart *hu)
> {
> 	struct h5 *h5 = hu->priv;
> @@ -229,6 +269,15 @@ static int h5_close(struct hci_uart *hu)
> 	skb_queue_purge(&h5->rel);
> 	skb_queue_purge(&h5->unrel);
> 
> +	if (hu->serdev) {
> +		struct h5_device *h5_dev;
> +
> +		h5_dev = serdev_device_get_drvdata(hu->serdev);
> +		gpiod_set_value_cansleep(h5_dev->disable_gpio, 1);
> +
> +		serdev_device_close(hu->serdev);
> +	}
> +
> 	kfree(h5);
> 
> 	return 0;
> @@ -316,7 +365,10 @@ static void h5_handle_internal_rx(struct hci_uart *hu)
> 			h5->tx_win = (data[2] & 0x07);
> 		BT_DBG("Three-wire init complete. tx_win %u", h5->tx_win);
> 		h5->state = H5_ACTIVE;
> -		hci_uart_init_ready(hu);
> +
> +		/* serdev does not support the "init_ready" signal */
> +		if (!hu->serdev)
> +			hci_uart_init_ready(hu);
> 		return;
> 	} else if (memcmp(data, sleep_req, 2) == 0) {
> 		BT_DBG("Peer went to sleep");
> @@ -739,10 +791,147 @@ static int h5_flush(struct hci_uart *hu)
> 	return 0;
> }
> 
> +#if IS_ENABLED(CONFIG_SERIAL_DEV_BUS)
> +static int h5_setup_realtek(struct h5_device *h5_dev)
> +{
> +	struct hci_uart *hu = &h5_dev->hu;
> +	int err = 0, retry = 3;
> +	struct sk_buff *skb;
> +	struct btrtl_device_info *btrtl_dev;
> +	__le32 baudrate_data;
> +	u32 device_baudrate;
> +	unsigned int controller_baudrate;
> +	bool flow_control;
> +
> +	/* devices always start with flow control disabled and even parity */
> +	serdev_device_set_flow_control(hu->serdev, false);
> +	serdev_device_set_parity(hu->serdev, true, false);
> +
> +	do {
> +		/* Configure BT_DISn and BT_RST_N to LOW state */
> +		gpiod_set_value_cansleep(h5_dev->reset_gpio, 1);
> +		gpiod_set_value_cansleep(h5_dev->disable_gpio, 1);
> +		msleep(500);
> +		gpiod_set_value_cansleep(h5_dev->reset_gpio, 0);
> +		gpiod_set_value_cansleep(h5_dev->disable_gpio, 0);
> +		msleep(500);

I really hate random msleep() without comments. Explain in the comment block why this specific wait is good.

> +
> +		btrtl_dev = btrtl_initialize(hu->hdev);
> +		if (!IS_ERR(btrtl_dev))
> +			break;
> +
> +		/* Toggle BT_DISn and retry */
> +	} while (retry--);
> +
> +	if (IS_ERR(btrtl_dev))
> +		return PTR_ERR(btrtl_dev);
> +
> +	err = btrtl_get_uart_settings(hu->hdev, btrtl_dev,
> +				      &controller_baudrate, &device_baudrate,
> +				      &flow_control);
> +	if (err)
> +		goto out_free;
> +
> +	baudrate_data = cpu_to_le32(device_baudrate);
> +	skb = __hci_cmd_sync(hu->hdev, 0xfc17, sizeof(baudrate_data),
> +			     &baudrate_data, HCI_INIT_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		bt_dev_err(hu->hdev, "set baud rate command failed");
> +		err = -PTR_ERR(skb);
> +		goto out_free;
> +	} else {
> +		kfree_skb(skb);
> +	}
> +
> +	msleep(500);

Same here, explain why this time is the right time to wait.

> +
> +	serdev_device_set_baudrate(hu->serdev, controller_baudrate);
> +	serdev_device_set_flow_control(hu->serdev, flow_control);
> +
> +	err = btrtl_download_firmware(hu->hdev, btrtl_dev);
> +
> +out_free:
> +	btrtl_free(btrtl_dev);
> +
> +	return err;
> +}
> +
> +static const struct hci_uart_proto h5p;
> +
> +static int hci_h5_probe(struct serdev_device *serdev)
> +{
> +	struct hci_uart *hu;
> +	struct h5_device *h5_dev;
> +
> +	h5_dev = devm_kzalloc(&serdev->dev, sizeof(*h5_dev), GFP_KERNEL);
> +	if (!h5_dev)
> +		return -ENOMEM;
> +
> +	hu = &h5_dev->hu;
> +	hu->serdev = serdev;
> +
> +	serdev_device_set_drvdata(serdev, h5_dev);
> +
> +	h5_dev->vendor_setup = of_device_get_match_data(&serdev->dev);
> +
> +	h5_dev->disable_gpio = devm_gpiod_get_optional(&serdev->dev, "disable",
> +						       GPIOD_OUT_LOW);
> +	if (IS_ERR(h5_dev->disable_gpio))
> +		return PTR_ERR(h5_dev->disable_gpio);
> +
> +	h5_dev->reset_gpio = devm_gpiod_get_optional(&serdev->dev, "reset",
> +						     GPIOD_OUT_LOW);
> +	if (IS_ERR(h5_dev->reset_gpio))
> +		return PTR_ERR(h5_dev->reset_gpio);
> +
> +	hci_uart_set_speeds(hu, 115200, 0);
> +
> +	return hci_uart_register_device(hu, &h5p);
> +}
> +
> +static void hci_h5_remove(struct serdev_device *serdev)
> +{
> +	struct h5_device *h5_dev = serdev_device_get_drvdata(serdev);
> +	struct hci_uart *hu = &h5_dev->hu;
> +	struct hci_dev *hdev = hu->hdev;
> +
> +	cancel_work_sync(&hu->write_work);
> +
> +	hci_unregister_dev(hdev);
> +	hci_free_dev(hdev);
> +	hu->proto->close(hu);
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id hci_h5_of_match[] = {
> +	{
> +		.compatible = "realtek,rtl8723bs-bluetooth",
> +		.data = h5_setup_realtek
> +	},
> +	{
> +		.compatible = "realtek,rtl8723ds-bluetooth",
> +		.data = h5_setup_realtek
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, hci_h5_of_match);
> +#endif
> +
> +static struct serdev_device_driver hci_h5_drv = {
> +	.driver		= {
> +		.name	= "hci-h5",
> +		.of_match_table = of_match_ptr(hci_h5_of_match),
> +	},
> +	.probe	= hci_h5_probe,
> +	.remove	= hci_h5_remove,
> +};
> +#endif
> +
> static const struct hci_uart_proto h5p = {
> 	.id		= HCI_UART_3WIRE,
> 	.name		= "Three-wire (H5)",
> 	.open		= h5_open,
> +	.setup		= h5_setup,
> 	.close		= h5_close,
> 	.recv		= h5_recv,
> 	.enqueue	= h5_enqueue,
> @@ -752,10 +941,14 @@ static const struct hci_uart_proto h5p = {
> 
> int __init h5_init(void)
> {
> +	serdev_device_driver_register(&hci_h5_drv);
> +
> 	return hci_uart_register_proto(&h5p);
> }
> 
> int __exit h5_deinit(void)
> {
> +	serdev_device_driver_unregister(&hci_h5_drv);
> +
> 	return hci_uart_unregister_proto(&h5p);
> }

Regards

Marcel

Re: [RFC v1 6/8] Bluetooth: hci_h5: add support for Realtek UART Bluetooth modules

[Martin Blumenstingl]

Hi Marcel,

On Sun, Nov 19, 2017 at 9:29 AM, Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org> wrote:
> Hi Martin,
>
>> Realtek RTL8723BS and RTL8723DS are SDIO wifi chips with an embedded
>> Bluetooth controller which connects to the host via UART.
>> The H5 protocol is used for communication between host and device.
>>
>> The Realtek "rtl8723bs_bt" and "rtl8723ds_bt" userspace Bluetooth UART
>> initialization tools (rtk_hciattach) use the following sequence:
>> 1) send H5 sync pattern (already supported by hci_h5)
>> 2) get LMP version (already supported by btrtl)
>> 3) get ROM version (already supported by btrtl)
>> 4) load the firmware and config for the current chipset (already
>>   supported by btrtl)
>> 5) read UART settings from the config blob (already supported by btrtl)
>> 6) send UART settings via a vendor command to the device (which changes
>>   the baudrate of the device and enables or disables flow control
>>   depending on the config)
>> 7) change the baudrate and flow control settings on the host
>> 8) send the firmware and config blob to the device (already supported by
>>   btrtl)
>>
>> This uses the serdev library as well as the existing btrtl driver to
>> initialize the Bluetooth functionality, which consists of:
>> - identifying the device and loading the corresponding firmware and
>>  config blobs (steps #2, #3 and #4)
>> - configuring the baudrate and flow control (steps #6 and #7)
>> - uploading the firmware to the device (step #8)
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>> ---
>> drivers/bluetooth/Kconfig  |   1 +
>> drivers/bluetooth/hci_h5.c | 195 ++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 195 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
>> index 60e1c7d6986d..3001f1200c72 100644
>> --- a/drivers/bluetooth/Kconfig
>> +++ b/drivers/bluetooth/Kconfig
>> @@ -146,6 +146,7 @@ config BT_HCIUART_LL
>> config BT_HCIUART_3WIRE
>>       bool "Three-wire UART (H5) protocol support"
>>       depends on BT_HCIUART
>> +     select BT_RTL if SERIAL_DEV_BUS
>>       help
>>         The HCI Three-wire UART Transport Layer makes it possible to
>>         user the Bluetooth HCI over a serial port interface. The HCI
>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
>> index 6a8d0d06aba7..7d584e5928bf 100644
>> --- a/drivers/bluetooth/hci_h5.c
>> +++ b/drivers/bluetooth/hci_h5.c
>> @@ -28,7 +28,14 @@
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/serdev.h>
>> +
>> #include "hci_uart.h"
>> +#include "btrtl.h"
>>
>> #define HCI_3WIRE_ACK_PKT     0
>> #define HCI_3WIRE_LINK_PKT    15
>> @@ -97,6 +104,13 @@ struct h5 {
>>       } sleep;
>> };
>>
>> +struct h5_device {
>> +     struct hci_uart hu;
>> +     struct gpio_desc *disable_gpio;
>> +     struct gpio_desc *reset_gpio;
>> +     int (*vendor_setup)(struct h5_device *h5_dev);
>> +};
>> +
>> static void h5_reset_rx(struct h5 *h5);
>>
>> static void h5_link_control(struct hci_uart *hu, const void *data, size_t len)
>> @@ -190,6 +204,7 @@ static int h5_open(struct hci_uart *hu)
>> {
>>       struct h5 *h5;
>>       const unsigned char sync[] = { 0x01, 0x7e };
>> +     int err;
>>
>>       BT_DBG("hu %p", hu);
>>
>> @@ -210,6 +225,14 @@ static int h5_open(struct hci_uart *hu)
>>
>>       h5->tx_win = H5_TX_WIN_MAX;
>>
>> +     if (hu->serdev) {
>> +             err = serdev_device_open(hu->serdev);
>> +             if (err) {
>> +                     bt_dev_err(hu->hdev, "failed to open serdev: %d", err);
>> +                     return err;
>> +             }
>> +     }
>> +
>>       set_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags);
>>
>>       /* Send initial sync request */
>> @@ -219,6 +242,23 @@ static int h5_open(struct hci_uart *hu)
>>       return 0;
>> }
>>
>> +static int h5_setup(struct hci_uart *hu)
>> +{
>> +     int err;
>> +     struct h5_device *h5_dev;
>> +
>> +     if (!hu->serdev)
>> +             return 0;
>> +
>> +     h5_dev = serdev_device_get_drvdata(hu->serdev);
>> +
>> +     err = h5_dev->vendor_setup(h5_dev);
>> +     if (err)
>> +             return err;
>
>         if (h5_dev->vendor_setup)
>                 return h5_dev->vendor_setup(h5_dev);
sounds reasonable, this way new devices can be added which don't need
any vendor setup
I'll fix this in the next version, thanks for spotting this

>> +
>> +     return 0;
>> +}
>> +
>> static int h5_close(struct hci_uart *hu)
>> {
>>       struct h5 *h5 = hu->priv;
>> @@ -229,6 +269,15 @@ static int h5_close(struct hci_uart *hu)
>>       skb_queue_purge(&h5->rel);
>>       skb_queue_purge(&h5->unrel);
>>
>> +     if (hu->serdev) {
>> +             struct h5_device *h5_dev;
>> +
>> +             h5_dev = serdev_device_get_drvdata(hu->serdev);
>> +             gpiod_set_value_cansleep(h5_dev->disable_gpio, 1);
>> +
>> +             serdev_device_close(hu->serdev);
>> +     }
>> +
>>       kfree(h5);
>>
>>       return 0;
>> @@ -316,7 +365,10 @@ static void h5_handle_internal_rx(struct hci_uart *hu)
>>                       h5->tx_win = (data[2] & 0x07);
>>               BT_DBG("Three-wire init complete. tx_win %u", h5->tx_win);
>>               h5->state = H5_ACTIVE;
>> -             hci_uart_init_ready(hu);
>> +
>> +             /* serdev does not support the "init_ready" signal */
>> +             if (!hu->serdev)
>> +                     hci_uart_init_ready(hu);
>>               return;
>>       } else if (memcmp(data, sleep_req, 2) == 0) {
>>               BT_DBG("Peer went to sleep");
>> @@ -739,10 +791,147 @@ static int h5_flush(struct hci_uart *hu)
>>       return 0;
>> }
>>
>> +#if IS_ENABLED(CONFIG_SERIAL_DEV_BUS)
>> +static int h5_setup_realtek(struct h5_device *h5_dev)
>> +{
>> +     struct hci_uart *hu = &h5_dev->hu;
>> +     int err = 0, retry = 3;
>> +     struct sk_buff *skb;
>> +     struct btrtl_device_info *btrtl_dev;
>> +     __le32 baudrate_data;
>> +     u32 device_baudrate;
>> +     unsigned int controller_baudrate;
>> +     bool flow_control;
>> +
>> +     /* devices always start with flow control disabled and even parity */
>> +     serdev_device_set_flow_control(hu->serdev, false);
>> +     serdev_device_set_parity(hu->serdev, true, false);
>> +
>> +     do {
>> +             /* Configure BT_DISn and BT_RST_N to LOW state */
>> +             gpiod_set_value_cansleep(h5_dev->reset_gpio, 1);
>> +             gpiod_set_value_cansleep(h5_dev->disable_gpio, 1);
>> +             msleep(500);
>> +             gpiod_set_value_cansleep(h5_dev->reset_gpio, 0);
>> +             gpiod_set_value_cansleep(h5_dev->disable_gpio, 0);
>> +             msleep(500);
>
> I really hate random msleep() without comments. Explain in the comment block why this specific wait is good.
OK, I'll add a comment *why* it's needed (without a delay between
toggling the GPIOs the device does not respond in the following
btrtl_initialize call)
the numbers however are based on "trial and error" as I could not find
any documentation for these

>> +
>> +             btrtl_dev = btrtl_initialize(hu->hdev);
>> +             if (!IS_ERR(btrtl_dev))
>> +                     break;
>> +
>> +             /* Toggle BT_DISn and retry */
>> +     } while (retry--);
>> +
>> +     if (IS_ERR(btrtl_dev))
>> +             return PTR_ERR(btrtl_dev);
>> +
>> +     err = btrtl_get_uart_settings(hu->hdev, btrtl_dev,
>> +                                   &controller_baudrate, &device_baudrate,
>> +                                   &flow_control);
>> +     if (err)
>> +             goto out_free;
>> +
>> +     baudrate_data = cpu_to_le32(device_baudrate);
>> +     skb = __hci_cmd_sync(hu->hdev, 0xfc17, sizeof(baudrate_data),
>> +                          &baudrate_data, HCI_INIT_TIMEOUT);
>> +     if (IS_ERR(skb)) {
>> +             bt_dev_err(hu->hdev, "set baud rate command failed");
>> +             err = -PTR_ERR(skb);
>> +             goto out_free;
>> +     } else {
>> +             kfree_skb(skb);
>> +     }
>> +
>> +     msleep(500);
>
> Same here, explain why this time is the right time to wait.
you are right, this may be obsolete since the __hci_cmd_sync call
above is already waiting for a reply.
I'll verify this and either remove this msleep or add a comment why it's needed

>> +
>> +     serdev_device_set_baudrate(hu->serdev, controller_baudrate);
>> +     serdev_device_set_flow_control(hu->serdev, flow_control);
>> +
>> +     err = btrtl_download_firmware(hu->hdev, btrtl_dev);
>> +
>> +out_free:
>> +     btrtl_free(btrtl_dev);
>> +
>> +     return err;
>> +}
>> +
>> +static const struct hci_uart_proto h5p;
>> +
>> +static int hci_h5_probe(struct serdev_device *serdev)
>> +{
>> +     struct hci_uart *hu;
>> +     struct h5_device *h5_dev;
>> +
>> +     h5_dev = devm_kzalloc(&serdev->dev, sizeof(*h5_dev), GFP_KERNEL);
>> +     if (!h5_dev)
>> +             return -ENOMEM;
>> +
>> +     hu = &h5_dev->hu;
>> +     hu->serdev = serdev;
>> +
>> +     serdev_device_set_drvdata(serdev, h5_dev);
>> +
>> +     h5_dev->vendor_setup = of_device_get_match_data(&serdev->dev);
>> +
>> +     h5_dev->disable_gpio = devm_gpiod_get_optional(&serdev->dev, "disable",
>> +                                                    GPIOD_OUT_LOW);
>> +     if (IS_ERR(h5_dev->disable_gpio))
>> +             return PTR_ERR(h5_dev->disable_gpio);
>> +
>> +     h5_dev->reset_gpio = devm_gpiod_get_optional(&serdev->dev, "reset",
>> +                                                  GPIOD_OUT_LOW);
>> +     if (IS_ERR(h5_dev->reset_gpio))
>> +             return PTR_ERR(h5_dev->reset_gpio);
>> +
>> +     hci_uart_set_speeds(hu, 115200, 0);
>> +
>> +     return hci_uart_register_device(hu, &h5p);
>> +}
>> +
>> +static void hci_h5_remove(struct serdev_device *serdev)
>> +{
>> +     struct h5_device *h5_dev = serdev_device_get_drvdata(serdev);
>> +     struct hci_uart *hu = &h5_dev->hu;
>> +     struct hci_dev *hdev = hu->hdev;
>> +
>> +     cancel_work_sync(&hu->write_work);
>> +
>> +     hci_unregister_dev(hdev);
>> +     hci_free_dev(hdev);
>> +     hu->proto->close(hu);
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id hci_h5_of_match[] = {
>> +     {
>> +             .compatible = "realtek,rtl8723bs-bluetooth",
>> +             .data = h5_setup_realtek
>> +     },
>> +     {
>> +             .compatible = "realtek,rtl8723ds-bluetooth",
>> +             .data = h5_setup_realtek
>> +     },
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(of, hci_h5_of_match);
>> +#endif
>> +
>> +static struct serdev_device_driver hci_h5_drv = {
>> +     .driver         = {
>> +             .name   = "hci-h5",
>> +             .of_match_table = of_match_ptr(hci_h5_of_match),
>> +     },
>> +     .probe  = hci_h5_probe,
>> +     .remove = hci_h5_remove,
>> +};
>> +#endif
>> +
>> static const struct hci_uart_proto h5p = {
>>       .id             = HCI_UART_3WIRE,
>>       .name           = "Three-wire (H5)",
>>       .open           = h5_open,
>> +     .setup          = h5_setup,
>>       .close          = h5_close,
>>       .recv           = h5_recv,
>>       .enqueue        = h5_enqueue,
>> @@ -752,10 +941,14 @@ static const struct hci_uart_proto h5p = {
>>
>> int __init h5_init(void)
>> {
>> +     serdev_device_driver_register(&hci_h5_drv);
>> +
>>       return hci_uart_register_proto(&h5p);
>> }
>>
>> int __exit h5_deinit(void)
>> {
>> +     serdev_device_driver_unregister(&hci_h5_drv);
>> +
>>       return hci_uart_unregister_proto(&h5p);
>> }
>
> Regards
>
> Marcel
>


Regards
Martin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v1 6/8] Bluetooth: hci_h5: add support for Realtek UART Bluetooth modules

[Marcel Holtmann]

Hi Martin,

> Realtek RTL8723BS and RTL8723DS are SDIO wifi chips with an embedded
> Bluetooth controller which connects to the host via UART.
> The H5 protocol is used for communication between host and device.
> 
> The Realtek "rtl8723bs_bt" and "rtl8723ds_bt" userspace Bluetooth UART
> initialization tools (rtk_hciattach) use the following sequence:
> 1) send H5 sync pattern (already supported by hci_h5)
> 2) get LMP version (already supported by btrtl)
> 3) get ROM version (already supported by btrtl)
> 4) load the firmware and config for the current chipset (already
>   supported by btrtl)
> 5) read UART settings from the config blob (already supported by btrtl)
> 6) send UART settings via a vendor command to the device (which changes
>   the baudrate of the device and enables or disables flow control
>   depending on the config)
> 7) change the baudrate and flow control settings on the host
> 8) send the firmware and config blob to the device (already supported by
>   btrtl)
> 
> This uses the serdev library as well as the existing btrtl driver to
> initialize the Bluetooth functionality, which consists of:
> - identifying the device and loading the corresponding firmware and
>  config blobs (steps #2, #3 and #4)
> - configuring the baudrate and flow control (steps #6 and #7)
> - uploading the firmware to the device (step #8)
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> drivers/bluetooth/Kconfig  |   1 +
> drivers/bluetooth/hci_h5.c | 195 ++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 195 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 60e1c7d6986d..3001f1200c72 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -146,6 +146,7 @@ config BT_HCIUART_LL
> config BT_HCIUART_3WIRE
> 	bool "Three-wire UART (H5) protocol support"
> 	depends on BT_HCIUART
> +	select BT_RTL if SERIAL_DEV_BUS
> 	help
> 	  The HCI Three-wire UART Transport Layer makes it possible to
> 	  user the Bluetooth HCI over a serial port interface. The HCI

so I just posted a bt3wire.c driver that is serdev only and written from scratch. On a RPi3 Broadcom chip it kinda works. I think there is a lot of extra work to be done, but this might be a better starting point for Realtek UART devices.

Regards

Marcel

Re: [RFC v1 6/8] Bluetooth: hci_h5: add support for Realtek UART Bluetooth modules

[Jeremy Cline]

Hi Marcel, Martin,

On 03/16/2018 03:22 PM, Marcel Holtmann wrote:
> Hi Martin,
> 
>> Realtek RTL8723BS and RTL8723DS are SDIO wifi chips with an embedded
>> Bluetooth controller which connects to the host via UART.
>> The H5 protocol is used for communication between host and device.
>>
>> The Realtek "rtl8723bs_bt" and "rtl8723ds_bt" userspace Bluetooth UART
>> initialization tools (rtk_hciattach) use the following sequence:
>> 1) send H5 sync pattern (already supported by hci_h5)
>> 2) get LMP version (already supported by btrtl)
>> 3) get ROM version (already supported by btrtl)
>> 4) load the firmware and config for the current chipset (already
>>   supported by btrtl)
>> 5) read UART settings from the config blob (already supported by btrtl)
>> 6) send UART settings via a vendor command to the device (which changes
>>   the baudrate of the device and enables or disables flow control
>>   depending on the config)
>> 7) change the baudrate and flow control settings on the host
>> 8) send the firmware and config blob to the device (already supported by
>>   btrtl)
>>
>> This uses the serdev library as well as the existing btrtl driver to
>> initialize the Bluetooth functionality, which consists of:
>> - identifying the device and loading the corresponding firmware and
>>  config blobs (steps #2, #3 and #4)
>> - configuring the baudrate and flow control (steps #6 and #7)
>> - uploading the firmware to the device (step #8)
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>> drivers/bluetooth/Kconfig  |   1 +
>> drivers/bluetooth/hci_h5.c | 195 ++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 195 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
>> index 60e1c7d6986d..3001f1200c72 100644
>> --- a/drivers/bluetooth/Kconfig
>> +++ b/drivers/bluetooth/Kconfig
>> @@ -146,6 +146,7 @@ config BT_HCIUART_LL
>> config BT_HCIUART_3WIRE
>> 	bool "Three-wire UART (H5) protocol support"
>> 	depends on BT_HCIUART
>> +	select BT_RTL if SERIAL_DEV_BUS
>> 	help
>> 	  The HCI Three-wire UART Transport Layer makes it possible to
>> 	  user the Bluetooth HCI over a serial port interface. The HCI
> 
> so I just posted a bt3wire.c driver that is serdev only and written from scratch. On a RPi3 Broadcom chip it kinda works. I think there is a lot of extra work to be done, but this might be a better starting point for Realtek UART devices.

I picked up the v2 version of this RFC and rebased it to the latest
bluetooth-next and added ACPI support. It also kinda works, but there's
a few problems left to track down and I still haven't looked into
getting rid of the config blobs.

It sounded like Martin wasn't going to be able to get to this until
at least the 4.18 cycle, so unless he objects I'm going to look at
getting what I've got working with that bt3wire.c driver.

Regards,
Jeremy

Re: [RFC v1 6/8] Bluetooth: hci_h5: add support for Realtek UART Bluetooth modules

[Marcel Holtmann]

Hi Jeremy,

>>> Realtek RTL8723BS and RTL8723DS are SDIO wifi chips with an embedded
>>> Bluetooth controller which connects to the host via UART.
>>> The H5 protocol is used for communication between host and device.
>>> 
>>> The Realtek "rtl8723bs_bt" and "rtl8723ds_bt" userspace Bluetooth UART
>>> initialization tools (rtk_hciattach) use the following sequence:
>>> 1) send H5 sync pattern (already supported by hci_h5)
>>> 2) get LMP version (already supported by btrtl)
>>> 3) get ROM version (already supported by btrtl)
>>> 4) load the firmware and config for the current chipset (already
>>>  supported by btrtl)
>>> 5) read UART settings from the config blob (already supported by btrtl)
>>> 6) send UART settings via a vendor command to the device (which changes
>>>  the baudrate of the device and enables or disables flow control
>>>  depending on the config)
>>> 7) change the baudrate and flow control settings on the host
>>> 8) send the firmware and config blob to the device (already supported by
>>>  btrtl)
>>> 
>>> This uses the serdev library as well as the existing btrtl driver to
>>> initialize the Bluetooth functionality, which consists of:
>>> - identifying the device and loading the corresponding firmware and
>>> config blobs (steps #2, #3 and #4)
>>> - configuring the baudrate and flow control (steps #6 and #7)
>>> - uploading the firmware to the device (step #8)
>>> 
>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>> ---
>>> drivers/bluetooth/Kconfig  |   1 +
>>> drivers/bluetooth/hci_h5.c | 195 ++++++++++++++++++++++++++++++++++++++++++++-
>>> 2 files changed, 195 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
>>> index 60e1c7d6986d..3001f1200c72 100644
>>> --- a/drivers/bluetooth/Kconfig
>>> +++ b/drivers/bluetooth/Kconfig
>>> @@ -146,6 +146,7 @@ config BT_HCIUART_LL
>>> config BT_HCIUART_3WIRE
>>> 	bool "Three-wire UART (H5) protocol support"
>>> 	depends on BT_HCIUART
>>> +	select BT_RTL if SERIAL_DEV_BUS
>>> 	help
>>> 	  The HCI Three-wire UART Transport Layer makes it possible to
>>> 	  user the Bluetooth HCI over a serial port interface. The HCI
>> 
>> so I just posted a bt3wire.c driver that is serdev only and written from scratch. On a RPi3 Broadcom chip it kinda works. I think there is a lot of extra work to be done, but this might be a better starting point for Realtek UART devices.
> 
> I picked up the v2 version of this RFC and rebased it to the latest
> bluetooth-next and added ACPI support. It also kinda works, but there's
> a few problems left to track down and I still haven't looked into
> getting rid of the config blobs.
> 
> It sounded like Martin wasn't going to be able to get to this until
> at least the 4.18 cycle, so unless he objects I'm going to look at
> getting what I've got working with that bt3wire.c driver.

yes please, have a look at the bt3wire.c code that I send and see if you can make it work with Realtek controllers.

On the RPi3 with the Broadcom controller it kinda works, but firmware loading and btmgmt power on/off seem to have issues. The power on/off might be caused because I moved the serdev_device_open and link establishment into the hdev->open callback and don’t clean up properly on hdev->close. However I really think it belongs there and thus we should make this work. You will notice that the link establishment is also kinda slow (at least on Broadcom devices) where the messages only happen once per second. We can try to send every 250 msec and see if that changes things. If the Bluetooth stack is powered down, we should also not have the serial device opened. On USB it is the same since it is bound, but no URBs are active.

For me the real advantage of a standalone bt3wire.c is that we no longer have spagetti code through hci_ldisc.c and hci_serdev.c and hci_*.c drivers that got rather complicated to follow.

On a side note, in btuart.c, I put all the serdev settings into the hdev->setup callback, but I think with bt3wire.c that is not possible due to 3-Wire protocol. So I assume we need vendor_open, vendor_close and vendor_setup additions in bt3wire.c

Regards

Marcel

Re: [RFC v1 6/8] Bluetooth: hci_h5: add support for Realtek UART Bluetooth modules

[Martin Blumenstingl]

Hi Jeremy,

On Sat, Mar 17, 2018 at 11:50 PM, Jeremy Cline <jeremy@jcline.org> wrote:
> Hi Marcel, Martin,
>
> On 03/16/2018 03:22 PM, Marcel Holtmann wrote:
>> Hi Martin,
>>
>>> Realtek RTL8723BS and RTL8723DS are SDIO wifi chips with an embedded
>>> Bluetooth controller which connects to the host via UART.
>>> The H5 protocol is used for communication between host and device.
>>>
>>> The Realtek "rtl8723bs_bt" and "rtl8723ds_bt" userspace Bluetooth UART
>>> initialization tools (rtk_hciattach) use the following sequence:
>>> 1) send H5 sync pattern (already supported by hci_h5)
>>> 2) get LMP version (already supported by btrtl)
>>> 3) get ROM version (already supported by btrtl)
>>> 4) load the firmware and config for the current chipset (already
>>>   supported by btrtl)
>>> 5) read UART settings from the config blob (already supported by btrtl)
>>> 6) send UART settings via a vendor command to the device (which changes
>>>   the baudrate of the device and enables or disables flow control
>>>   depending on the config)
>>> 7) change the baudrate and flow control settings on the host
>>> 8) send the firmware and config blob to the device (already supported by
>>>   btrtl)
>>>
>>> This uses the serdev library as well as the existing btrtl driver to
>>> initialize the Bluetooth functionality, which consists of:
>>> - identifying the device and loading the corresponding firmware and
>>>  config blobs (steps #2, #3 and #4)
>>> - configuring the baudrate and flow control (steps #6 and #7)
>>> - uploading the firmware to the device (step #8)
>>>
>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>> ---
>>> drivers/bluetooth/Kconfig  |   1 +
>>> drivers/bluetooth/hci_h5.c | 195 ++++++++++++++++++++++++++++++++++++++++++++-
>>> 2 files changed, 195 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
>>> index 60e1c7d6986d..3001f1200c72 100644
>>> --- a/drivers/bluetooth/Kconfig
>>> +++ b/drivers/bluetooth/Kconfig
>>> @@ -146,6 +146,7 @@ config BT_HCIUART_LL
>>> config BT_HCIUART_3WIRE
>>>      bool "Three-wire UART (H5) protocol support"
>>>      depends on BT_HCIUART
>>> +    select BT_RTL if SERIAL_DEV_BUS
>>>      help
>>>        The HCI Three-wire UART Transport Layer makes it possible to
>>>        user the Bluetooth HCI over a serial port interface. The HCI
>>
>> so I just posted a bt3wire.c driver that is serdev only and written from scratch. On a RPi3 Broadcom chip it kinda works. I think there is a lot of extra work to be done, but this might be a better starting point for Realtek UART devices.
>
> I picked up the v2 version of this RFC and rebased it to the latest
> bluetooth-next and added ACPI support. It also kinda works, but there's
> a few problems left to track down and I still haven't looked into
> getting rid of the config blobs.
>
> It sounded like Martin wasn't going to be able to get to this until
> at least the 4.18 cycle, so unless he objects I'm going to look at
> getting what I've got working with that bt3wire.c driver.
please go ahead - I am still busy with other things
thank you for taking this over!


Regards
Martin

[RFC v1 7/8] Bluetooth: hci_serdev: remove the HCI_UART_INIT_PENDING check

[Martin Blumenstingl]

The three-wire (H5) protocol is the only protocol which uses
HCI_UART_INIT_PENDING.
Unfortunately the protocol implementation never receives data with this
check still in place. For the H5 protocol this means that the
initialization never completes and thus the firmware download never
starts. Even if the initialization would succeed later on the drivers
would call hci_uart_init_ready() which schedules the registration which
is currently not implemented by hci_serdev.c.

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

Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 drivers/bluetooth/hci_serdev.c | 3 ---
 1 file changed, 3 deletions(-)

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

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

Re: [RFC v1 7/8] Bluetooth: hci_serdev: remove the HCI_UART_INIT_PENDING check

[Marcel Holtmann]

Hi Martin,

> The three-wire (H5) protocol is the only protocol which uses
> HCI_UART_INIT_PENDING.
> Unfortunately the protocol implementation never receives data with this
> check still in place. For the H5 protocol this means that the
> initialization never completes and thus the firmware download never
> starts. Even if the initialization would succeed later on the drivers
> would call hci_uart_init_ready() which schedules the registration which
> is currently not implemented by hci_serdev.c.
> 
> Removing the HCI_UART_INIT_PENDING check makes the code easier to read
> and also fixes the initalization of devices (implemented with the serdev
> library) which use the H5 protocol.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> ---
> drivers/bluetooth/hci_serdev.c | 3 ---
> 1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c
> index e0e6461b9200..fe67eb6d4278 100644
> --- a/drivers/bluetooth/hci_serdev.c
> +++ b/drivers/bluetooth/hci_serdev.c
> @@ -333,9 +333,6 @@ int hci_uart_register_device(struct hci_uart *hu,
> 	else
> 		hdev->dev_type = HCI_PRIMARY;
> 
> -	if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
> -		return 0;
> -

then lets also remove the flag definition itself. Or if that is still needed for some legacy operation, then describe this. For example I also see it used in hci_serdev.c and main question would be if it is used there as well or some legacy cruft.

Regards

Marcel

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

Re: [RFC v1 7/8] Bluetooth: hci_serdev: remove the HCI_UART_INIT_PENDING check

[Martin Blumenstingl]

Hi Marcel,

thank you for going through my patches!

On Sun, Nov 19, 2017 at 9:21 AM, Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org> wrote:
> Hi Martin,
>
>> The three-wire (H5) protocol is the only protocol which uses
>> HCI_UART_INIT_PENDING.
>> Unfortunately the protocol implementation never receives data with this
>> check still in place. For the H5 protocol this means that the
>> initialization never completes and thus the firmware download never
>> starts. Even if the initialization would succeed later on the drivers
>> would call hci_uart_init_ready() which schedules the registration which
>> is currently not implemented by hci_serdev.c.
>>
>> Removing the HCI_UART_INIT_PENDING check makes the code easier to read
>> and also fixes the initalization of devices (implemented with the serdev
>> library) which use the H5 protocol.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>> ---
>> drivers/bluetooth/hci_serdev.c | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c
>> index e0e6461b9200..fe67eb6d4278 100644
>> --- a/drivers/bluetooth/hci_serdev.c
>> +++ b/drivers/bluetooth/hci_serdev.c
>> @@ -333,9 +333,6 @@ int hci_uart_register_device(struct hci_uart *hu,
>>       else
>>               hdev->dev_type = HCI_PRIMARY;
>>
>> -     if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
>> -             return 0;
>> -
>
> then lets also remove the flag definition itself. Or if that is still needed for some legacy operation, then describe this. For example I also see it used in hci_serdev.c and main question would be if it is used there as well or some legacy cruft.
this flag is still used in hci_ldisc.c (if that's what you mean
instead of hci_serdev.c):
as far as I understand the code it's used to postpone the
hci_register_dev() call until the sync response is received

Johan Hedberg added this code in 9f2aee848fe6 ("Bluetooth: Add delayed
init sequence support for UART controllers") - it would be great if he
could comment on this (he is CC'ed on this mail)


Regards
Martin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC v1 7/8] Bluetooth: hci_serdev: remove the HCI_UART_INIT_PENDING check

[Johan Hedberg]

Hi Martin,

On Sun, Nov 19, 2017, Martin Blumenstingl wrote:
> >> @@ -333,9 +333,6 @@ int hci_uart_register_device(struct hci_uart *hu,
> >>       else
> >>               hdev->dev_type = HCI_PRIMARY;
> >>
> >> -     if (test_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags))
> >> -             return 0;
> >> -
> >
> > then lets also remove the flag definition itself. Or if that is
> > still needed for some legacy operation, then describe this. For
> > example I also see it used in hci_serdev.c and main question would
> > be if it is used there as well or some legacy cruft.
> 
> this flag is still used in hci_ldisc.c (if that's what you mean
> instead of hci_serdev.c):
> as far as I understand the code it's used to postpone the
> hci_register_dev() call until the sync response is received
> 
> Johan Hedberg added this code in 9f2aee848fe6 ("Bluetooth: Add delayed
> init sequence support for UART controllers") - it would be great if he
> could comment on this (he is CC'ed on this mail)

That patch is over five years old, so don't count on me remembering the
details ;)  That said, your analysis of its use sounds correct. It's
possible however that we've since then given HCI drivers other
capabilities that would make the INIT_PENDING flag redundant. E.g. the
setup callback didn't exist at the time that I wrote the patch.

Johan

[RFC v1 8/8] dt-bindings: net: bluetooth: add support for Realtek Bluetooth chips

[Martin Blumenstingl]

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-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
 .../devicetree/bindings/net/realtek-bluetooth.txt  | 31 ++++++++++++++++++++++
 1 file changed, 31 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..c919e06469f8
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
@@ -0,0 +1,31 @@
+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:
+- disable-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";
+		disable-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
+		reset-gpios = <&gpio 11 GPIO_ACTIVE_HIGH>;
+	};
+};
-- 
2.15.0

Re: [RFC v1 8/8] dt-bindings: net: bluetooth: add support for Realtek Bluetooth chips

[Rob Herring]

On Fri, Nov 17, 2017 at 11:35:43PM +0100, Martin Blumenstingl wrote:
> 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-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> ---
>  .../devicetree/bindings/net/realtek-bluetooth.txt  | 31 ++++++++++++++++++++++
>  1 file changed, 31 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..c919e06469f8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/realtek-bluetooth.txt
> @@ -0,0 +1,31 @@
> +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:
> +- disable-gpios: GPIO specifier, used to enable/disable the BT module

We generally use 'enable-gpios' or 'powerdown-gpios' even if the h/w 
name is somewhat different.

> +- reset-gpios: GPIO specifier, used to reset the BT module
> +
> +
> +Example:
> +
> +&uart {
> +	...
> +
> +	bluetooth {
> +		compatible = "realtek,rtl8723bs-bluetooth";
> +		disable-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
> +		reset-gpios = <&gpio 11 GPIO_ACTIVE_HIGH>;
> +	};
> +};
> -- 
> 2.15.0
>