[PATCH v8 0/7] Enable Bluetooth functionality for WCN3990

[PATCH v8 0/7] Enable Bluetooth functionality for WCN3990

[Balakrishna Godavarthi]

These patches enables Bluetooth functinalties for new Qualcomm
Bluetooth chip wnc3990. As this is latest chip with new features, 
along with some common features to old chip "qcom,qca6174-bt".
we have updated names of functions that are used for both the chips 
to keep this generic and would help in future when we would have new 
BT SoC.

The below are difference between new and old chips.

*Power:
 *New chip: we use voltage regulators to turn on/off the chip and we 
  must send a command on Tx line to turn on/off the chip completely.
 *Old chip: We turn on/off by setting a GPIO.

 *Note: Turning on sequence differs between two chips.

*Firmware download:
  Only firmware file name differ from New and Old chip.
  
  So we reused some functions/structure/variables which are used for old 
  chip to New chip by generic naming (may be future chips may use same functions).

 * As we have multiple changes for functions names,we have tested these patches 
   on both chips.

v8:
  * Squashed [v7 2/8] and [v7 3/8] to one patch.
  * updated functions to set and get the UART speeds.
  * addressed review comments.

v7:
  * Renamed all the possible function of ROME to generic.
  * updated with review comments for hci_qca and btqca
  * fixed kbuild errors.
  * created wrapper functions for re-usable blocks.
 
v6:
  * Hooked up qca_power to qca_serdev.
  * renamed all the naming inconsistency functions with qca_*
  * leveraged common code of ROME for wcn3990.
  * created wrapper functions for re-usable blocks.
  * updated function of _*regulator_enable and _*regualtor_disable.  
  * removed redundant comments and functions.
  * add code to derive the firmware files based on ROM Version.
  * created new patch for common function of ROME and wcn3990.
  * enables Qualcomm chip to operate at 3.2Mbps.
  * updated names of functions that are used for both the chips to keep this generic. 

v5:
  * updated with review comments for hci_qca and btqca
 
v4:
  * rebased all changes on top of bluetooth-next.
  * addressed review comments.
  * New patch created for firmware download.

v3:
  * Rebased all changes on top of bluetooth-next.
  * dt-bindings: net: bluetooth: Add device tree bindings for QTI chip wcn3990
    - added dt-binding snippet for bluetooth chip wcn3990.
  * Bluetooth: Add support for wcn3990 soc.
    - updated BT_INFO with bt_dev_info, where ever necessary.

v2:
   * Add support for wcn3990:
    These changes are aligned as per serdev frame work.
          We implemented these changes on top of https://patchwork.kernel.org/patch/10316093/
        As this patch enable serdev for one of the Qualcomm BT chip.

        The changes in the patch include.
        - Adding support to voting up/down regulators for WCN3990.
           - Bluetooth firmware download for wcn3990.

  * Add device tree bindings for Atheros chips:
          These changes are on top of https://patchwork.kernel.org/patch/10316097/.
          - Description of device tree bindings.

Balakrishna Godavarthi (7):
  dt-bindings: net: bluetooth: Add device tree bindings for QTI chip
    wcn3990
  Bluetooth: btqca: Rename ROME specific functions to Generic functions
  Bluetooth: btqca: Redefine qca_uart_setup() to generic function.
  Bluetooth: hci_qca: Add wrapper functions for setting UART speed
  Bluetooth: hci_qca: Enable 3.2 Mbps operating speed.
  Bluetooth: btqca: Add wcn3990 firmware download support.
  Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990

 .../bindings/net/qualcomm-bluetooth.txt       |  30 +-
 drivers/bluetooth/btqca.c                     | 120 ++---
 drivers/bluetooth/btqca.h                     |  22 +-
 drivers/bluetooth/hci_qca.c                   | 434 +++++++++++++++---
 4 files changed, 491 insertions(+), 115 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v8 1/7] dt-bindings: net: bluetooth: Add device tree bindings for QTI chip wcn3990

[Balakrishna Godavarthi]

This patch enables regulators for the Qualcomm Bluetooth wcn3990
controller.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---

Changes in v8:
    * Separated the optional entries between two chips

Changes in v7:
    * no change.

Changes in v6:

    * Changed the oper-speed to max-speed.

Changes in v5:

    * Added entry for oper-speed and init-speed.

---
 .../bindings/net/qualcomm-bluetooth.txt       | 30 +++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
index 0ea18a53cc29..c2b09dd2bf31 100644
--- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
+++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
@@ -10,12 +10,23 @@ device the slave device is attached to.
 Required properties:
  - compatible: should contain one of the following:
    * "qcom,qca6174-bt"
+   * "qcom,wcn3990-bt"
+
+Optional properties for compatible string qcom,qca6174-bt:
 
-Optional properties:
  - enable-gpios: gpio specifier used to enable chip
  - clocks: clock provided to the controller (SUSCLK_32KHZ)
 
-Example:
+Optional properties for compatible string qcom,wcn3990-bt:
+
+ - vddio-supply: Bluetooth VDD IO regulator handle.
+ - vddxtal-supply: Bluetooth VDD XTAL regulator handle.
+ - vddpa-supply: Bluetooth VDD PA regulator handle.
+ - vddldo-supply: Bluetooth VDD LDO regulator handle.
+ - vddcore-supply: Bluetooth VDD CORE regulator handle.
+ - max-speed: Maximum operating speed of the chip.
+
+Examples:
 
 serial@7570000 {
 	label = "BT-UART";
@@ -28,3 +39,18 @@ serial@7570000 {
 		clocks = <&divclk4>;
 	};
 };
+
+serial@898000 {
+	label = "BT-UART";
+	status = "okay";
+
+	bluetooth {
+		compatible = "qcom,wcn3990-bt";
+		vddio-supply = <&pm8998_s3>;
+		vddxtal-supply = <&pm8998_s5>;
+		vddcore-supply = <&pm8998_l7>;
+		vddpa-supply = <&pm8998_l17>;
+		vddldo-supply = <&pm8998_l25>;
+		max-speed = <3200000>;
+	};
+};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v8 2/7] Bluetooth: btqca: Rename ROME specific functions to Generic functions

[Balakrishna Godavarthi]

Some of the QCA BTSoC ROME functions, are used for different versions
or different make of BTSoC's. Instead of duplicating the same functions
for new chip, updating names of the functions that are used for both
chip's to keep this generic and would help in future when we would have
new BT SoC and to have generic text in bt_dev_info, bt_dev_err and
bt_dev_dbg updated from ROME to QCA where ever possible. This avoids
confusion to user, when using the future Qualcomm Bluetooth SoC's.
Updated BT_DBG, BT_ERR and BT_INFO with bt_dev_dbg, bt_dev_err and
bt_dev_info where ever applicable.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
Changes in v8:
    * squashed [v7 2/8] and [v7 3/8] into one.
    * updated review comments
	
Changes in v7:
    * updated the all the functions of ROME to generic.

Changes in v6:
    * initial patch
    * updated names of functions that are used for both the chips to 
      keep this generic and would help in future when we would have 
      new BT SoC.
---
 drivers/bluetooth/btqca.c   | 85 ++++++++++++++++++-------------------
 drivers/bluetooth/btqca.h   | 10 ++++-
 drivers/bluetooth/hci_qca.c |  2 +-
 3 files changed, 51 insertions(+), 46 deletions(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index 8219816c54a0..c5cf9cab438a 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -27,7 +27,7 @@
 
 #define VERSION "0.1"
 
-static int rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
+int qca_read_soc_version(struct hci_dev *hdev, u32 *soc_version)
 {
 	struct sk_buff *skb;
 	struct edl_event_hdr *edl;
@@ -35,36 +35,35 @@ static int rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
 	char cmd;
 	int err = 0;
 
-	BT_DBG("%s: ROME Patch Version Request", hdev->name);
+	bt_dev_dbg(hdev, "QCA Version Request");
 
 	cmd = EDL_PATCH_VER_REQ_CMD;
 	skb = __hci_cmd_sync_ev(hdev, EDL_PATCH_CMD_OPCODE, EDL_PATCH_CMD_LEN,
 				&cmd, HCI_VENDOR_PKT, HCI_INIT_TIMEOUT);
 	if (IS_ERR(skb)) {
 		err = PTR_ERR(skb);
-		BT_ERR("%s: Failed to read version of ROME (%d)", hdev->name,
-		       err);
+		bt_dev_err(hdev, "Reading QCA version information failed (%d)",
+			   err);
 		return err;
 	}
 
 	if (skb->len != sizeof(*edl) + sizeof(*ver)) {
-		BT_ERR("%s: Version size mismatch len %d", hdev->name,
-		       skb->len);
+		bt_dev_err(hdev, "QCA Version size mismatch len %d", skb->len);
 		err = -EILSEQ;
 		goto out;
 	}
 
 	edl = (struct edl_event_hdr *)(skb->data);
 	if (!edl) {
-		BT_ERR("%s: TLV with no header", hdev->name);
+		bt_dev_err(hdev, "QCA TLV with no header");
 		err = -EILSEQ;
 		goto out;
 	}
 
 	if (edl->cresp != EDL_CMD_REQ_RES_EVT ||
 	    edl->rtype != EDL_APP_VER_RES_EVT) {
-		BT_ERR("%s: Wrong packet received %d %d", hdev->name,
-		       edl->cresp, edl->rtype);
+		bt_dev_err(hdev, "QCA Wrong packet received %d %d", edl->cresp,
+			   edl->rtype);
 		err = -EIO;
 		goto out;
 	}
@@ -76,11 +75,11 @@ static int rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
 	BT_DBG("%s: ROM    :0x%08x", hdev->name, le16_to_cpu(ver->rome_ver));
 	BT_DBG("%s: SOC    :0x%08x", hdev->name, le32_to_cpu(ver->soc_id));
 
-	/* ROME chipset version can be decided by patch and SoC
+	/* QCA chipset version can be decided by patch and SoC
 	 * version, combination with upper 2 bytes from SoC
 	 * and lower 2 bytes from patch will be used.
 	 */
-	*rome_version = (le32_to_cpu(ver->soc_id) << 16) |
+	*soc_version = (le32_to_cpu(ver->soc_id) << 16) |
 			(le16_to_cpu(ver->rome_ver) & 0x0000ffff);
 
 out:
@@ -88,18 +87,19 @@ static int rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(qca_read_soc_version);
 
-static int rome_reset(struct hci_dev *hdev)
+static int qca_send_reset(struct hci_dev *hdev)
 {
 	struct sk_buff *skb;
 	int err;
 
-	BT_DBG("%s: ROME HCI_RESET", hdev->name);
+	bt_dev_dbg(hdev, "QCA HCI_RESET");
 
 	skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
 	if (IS_ERR(skb)) {
 		err = PTR_ERR(skb);
-		BT_ERR("%s: Reset failed (%d)", hdev->name, err);
+		bt_dev_err(hdev, "QCA Reset failed (%d)", err);
 		return err;
 	}
 
@@ -108,7 +108,7 @@ static int rome_reset(struct hci_dev *hdev)
 	return 0;
 }
 
-static void rome_tlv_check_data(struct rome_config *config,
+static void qca_tlv_check_data(struct rome_config *config,
 				const struct firmware *fw)
 {
 	const u8 *data;
@@ -207,7 +207,7 @@ static void rome_tlv_check_data(struct rome_config *config,
 	}
 }
 
-static int rome_tlv_send_segment(struct hci_dev *hdev, int seg_size,
+static int qca_tlv_send_segment(struct hci_dev *hdev, int seg_size,
 				 const u8 *data, enum rome_tlv_dnld_mode mode)
 {
 	struct sk_buff *skb;
@@ -228,19 +228,19 @@ static int rome_tlv_send_segment(struct hci_dev *hdev, int seg_size,
 				HCI_VENDOR_PKT, HCI_INIT_TIMEOUT);
 	if (IS_ERR(skb)) {
 		err = PTR_ERR(skb);
-		BT_ERR("%s: Failed to send TLV segment (%d)", hdev->name, err);
+		bt_dev_err(hdev, "QCA Failed to send TLV segment (%d)", err);
 		return err;
 	}
 
 	if (skb->len != sizeof(*edl) + sizeof(*tlv_resp)) {
-		BT_ERR("%s: TLV response size mismatch", hdev->name);
+		bt_dev_err(hdev, "QCA TLV response size mismatch");
 		err = -EILSEQ;
 		goto out;
 	}
 
 	edl = (struct edl_event_hdr *)(skb->data);
 	if (!edl) {
-		BT_ERR("%s: TLV with no header", hdev->name);
+		bt_dev_err(hdev, "TLV with no header");
 		err = -EILSEQ;
 		goto out;
 	}
@@ -249,8 +249,8 @@ static int rome_tlv_send_segment(struct hci_dev *hdev, int seg_size,
 
 	if (edl->cresp != EDL_CMD_REQ_RES_EVT ||
 	    edl->rtype != EDL_TVL_DNLD_RES_EVT || tlv_resp->result != 0x00) {
-		BT_ERR("%s: TLV with error stat 0x%x rtype 0x%x (0x%x)",
-		       hdev->name, edl->cresp, edl->rtype, tlv_resp->result);
+		bt_dev_err(hdev, "QCA TLV with error stat 0x%x rtype 0x%x (0x%x)",
+			   edl->cresp, edl->rtype, tlv_resp->result);
 		err = -EIO;
 	}
 
@@ -260,23 +260,23 @@ static int rome_tlv_send_segment(struct hci_dev *hdev, int seg_size,
 	return err;
 }
 
-static int rome_download_firmware(struct hci_dev *hdev,
+static int qca_download_firmware(struct hci_dev *hdev,
 				  struct rome_config *config)
 {
 	const struct firmware *fw;
 	const u8 *segment;
 	int ret, remain, i = 0;
 
-	bt_dev_info(hdev, "ROME Downloading %s", config->fwname);
+	bt_dev_info(hdev, "QCA Downloading %s", config->fwname);
 
 	ret = request_firmware(&fw, config->fwname, &hdev->dev);
 	if (ret) {
-		BT_ERR("%s: Failed to request file: %s (%d)", hdev->name,
-		       config->fwname, ret);
+		bt_dev_err(hdev, "QCA Failed to request file: %s (%d)",
+			   config->fwname, ret);
 		return ret;
 	}
 
-	rome_tlv_check_data(config, fw);
+	qca_tlv_check_data(config, fw);
 
 	segment = fw->data;
 	remain = fw->size;
@@ -290,7 +290,7 @@ static int rome_download_firmware(struct hci_dev *hdev,
 		if (!remain || segsize < MAX_SIZE_PER_TLV_SEGMENT)
 			config->dnld_mode = ROME_SKIP_EVT_NONE;
 
-		ret = rome_tlv_send_segment(hdev, segsize, segment,
+		ret = qca_tlv_send_segment(hdev, segsize, segment,
 					    config->dnld_mode);
 		if (ret)
 			break;
@@ -317,8 +317,7 @@ int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
 				HCI_VENDOR_PKT, HCI_INIT_TIMEOUT);
 	if (IS_ERR(skb)) {
 		err = PTR_ERR(skb);
-		BT_ERR("%s: Change address command failed (%d)",
-		       hdev->name, err);
+		bt_dev_err(hdev, "QCA Change address command failed (%d)", err);
 		return err;
 	}
 
@@ -328,32 +327,32 @@ int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
 }
 EXPORT_SYMBOL_GPL(qca_set_bdaddr_rome);
 
-int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate)
+int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate)
 {
 	u32 rome_ver = 0;
 	struct rome_config config;
 	int err;
 
-	BT_DBG("%s: ROME setup on UART", hdev->name);
+	bt_dev_dbg(hdev, "QCA setup on UART");
 
 	config.user_baud_rate = baudrate;
 
-	/* Get ROME version information */
-	err = rome_patch_ver_req(hdev, &rome_ver);
+	/* Get QCA version information */
+	err = qca_read_soc_version(hdev, &rome_ver);
 	if (err < 0 || rome_ver == 0) {
-		BT_ERR("%s: Failed to get version 0x%x", hdev->name, err);
+		bt_dev_err(hdev, "QCA Failed to get version %d", err);
 		return err;
 	}
 
-	bt_dev_info(hdev, "ROME controller version 0x%08x", rome_ver);
+	bt_dev_info(hdev, "QCA controller version 0x%08x", rome_ver);
 
 	/* Download rampatch file */
 	config.type = TLV_TYPE_PATCH;
 	snprintf(config.fwname, sizeof(config.fwname), "qca/rampatch_%08x.bin",
 		 rome_ver);
-	err = rome_download_firmware(hdev, &config);
+	err = qca_download_firmware(hdev, &config);
 	if (err < 0) {
-		BT_ERR("%s: Failed to download patch (%d)", hdev->name, err);
+		bt_dev_err(hdev, "QCA Failed to download patch (%d)", err);
 		return err;
 	}
 
@@ -361,24 +360,24 @@ int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate)
 	config.type = TLV_TYPE_NVM;
 	snprintf(config.fwname, sizeof(config.fwname), "qca/nvm_%08x.bin",
 		 rome_ver);
-	err = rome_download_firmware(hdev, &config);
+	err = qca_download_firmware(hdev, &config);
 	if (err < 0) {
-		BT_ERR("%s: Failed to download NVM (%d)", hdev->name, err);
+		bt_dev_err(hdev, "QCA Failed to download NVM (%d)", err);
 		return err;
 	}
 
 	/* Perform HCI reset */
-	err = rome_reset(hdev);
+	err = qca_send_reset(hdev);
 	if (err < 0) {
-		BT_ERR("%s: Failed to run HCI_RESET (%d)", hdev->name, err);
+		bt_dev_err(hdev, "QCA Failed to run HCI_RESET (%d)", err);
 		return err;
 	}
 
-	bt_dev_info(hdev, "ROME setup on UART is completed");
+	bt_dev_info(hdev, "QCA setup on UART is completed");
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(qca_uart_setup_rome);
+EXPORT_SYMBOL_GPL(qca_uart_setup);
 
 MODULE_AUTHOR("Ben Young Tae Kim <ytkim@qca.qualcomm.com>");
 MODULE_DESCRIPTION("Bluetooth support for Qualcomm Atheros family ver " VERSION);
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index 13d77fd873b6..5c9851b11838 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -127,7 +127,8 @@ struct tlv_type_hdr {
 #if IS_ENABLED(CONFIG_BT_QCA)
 
 int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
-int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate);
+int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate);
+int qca_read_soc_version(struct hci_dev *hdev, u32 *soc_version);
 
 #else
 
@@ -136,7 +137,12 @@ static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdad
 	return -EOPNOTSUPP;
 }
 
-static inline int qca_uart_setup_rome(struct hci_dev *hdev, int speed)
+static inline int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int qca_read_soc_version(struct hci_dev *hdev, u32 *soc_version)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 51790dd02afb..d7b60669b656 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -966,7 +966,7 @@ static int qca_setup(struct hci_uart *hu)
 	}
 
 	/* Setup patch / NVM configurations */
-	ret = qca_uart_setup_rome(hdev, qca_baudrate);
+	ret = qca_uart_setup(hdev, qca_baudrate);
 	if (!ret) {
 		set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
 		qca_debugfs_init(hdev);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v8 3/7] Bluetooth: btqca: Redefine qca_uart_setup() to generic function.

[Balakrishna Godavarthi]

Redefinition of qca_uart_setup will help future Qualcomm Bluetooth
SoC, to use the same function instead of duplicating the function.
Added new arguments soc_type and soc_ver to the functions.

These arguments will help to decide type of firmware files
to be loaded into Bluetooth chip.
soc_type holds the Bluetooth chip connected to APPS processor.
soc_ver holds the Bluetooth chip version.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
Changes in v8:
    * updated soc_type with enum.

Changes in v7:
    * initial patch
    * redefined qca_uart_setup function to generic.
---
 drivers/bluetooth/btqca.c   | 23 ++++++++++++-----------
 drivers/bluetooth/btqca.h   | 13 +++++++++++--
 drivers/bluetooth/hci_qca.c |  3 ++-
 3 files changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index c5cf9cab438a..3b25be1be19c 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -327,9 +327,9 @@ int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
 }
 EXPORT_SYMBOL_GPL(qca_set_bdaddr_rome);
 
-int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate)
+int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
+		   enum qca_btsoc_type soc_type, u32 soc_ver)
 {
-	u32 rome_ver = 0;
 	struct rome_config config;
 	int err;
 
@@ -337,19 +337,20 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate)
 
 	config.user_baud_rate = baudrate;
 
-	/* Get QCA version information */
-	err = qca_read_soc_version(hdev, &rome_ver);
-	if (err < 0 || rome_ver == 0) {
-		bt_dev_err(hdev, "QCA Failed to get version %d", err);
-		return err;
+	if (!soc_ver) {
+		/* Get QCA version information */
+		err = qca_read_soc_version(hdev, &soc_ver);
+		if (err < 0 || soc_ver == 0) {
+			bt_dev_err(hdev, "QCA Failed to get version (%d)", err);
+			return err;
+		}
+		bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver);
 	}
 
-	bt_dev_info(hdev, "QCA controller version 0x%08x", rome_ver);
-
 	/* Download rampatch file */
 	config.type = TLV_TYPE_PATCH;
 	snprintf(config.fwname, sizeof(config.fwname), "qca/rampatch_%08x.bin",
-		 rome_ver);
+		 soc_ver);
 	err = qca_download_firmware(hdev, &config);
 	if (err < 0) {
 		bt_dev_err(hdev, "QCA Failed to download patch (%d)", err);
@@ -359,7 +360,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate)
 	/* Download NVM configuration */
 	config.type = TLV_TYPE_NVM;
 	snprintf(config.fwname, sizeof(config.fwname), "qca/nvm_%08x.bin",
-		 rome_ver);
+		 soc_ver);
 	err = qca_download_firmware(hdev, &config);
 	if (err < 0) {
 		bt_dev_err(hdev, "QCA Failed to download NVM (%d)", err);
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index 5c9851b11838..24d6667eecf1 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -124,10 +124,18 @@ struct tlv_type_hdr {
 	__u8   data[0];
 } __packed;
 
+enum qca_btsoc_type {
+	QCA_INVALID = -1,
+	QCA_AR3002,
+	QCA_ROME,
+	QCA_WCN3990
+};
+
 #if IS_ENABLED(CONFIG_BT_QCA)
 
 int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
-int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate);
+int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
+		   enum qca_btsoc_type soc_type, u32 soc_ver);
 int qca_read_soc_version(struct hci_dev *hdev, u32 *soc_version);
 
 #else
@@ -137,7 +145,8 @@ static inline int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdad
 	return -EOPNOTSUPP;
 }
 
-static inline int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate)
+static inline int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
+				 enum qca_btsoc_type soc_type, u32 soc_ver);
 {
 	return -EOPNOTSUPP;
 }
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index d7b60669b656..fe62420ef838 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -929,6 +929,7 @@ static int qca_setup(struct hci_uart *hu)
 	struct qca_data *qca = hu->priv;
 	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
 	int ret;
+	int soc_ver = 0;
 
 	bt_dev_info(hdev, "ROME setup");
 
@@ -966,7 +967,7 @@ static int qca_setup(struct hci_uart *hu)
 	}
 
 	/* Setup patch / NVM configurations */
-	ret = qca_uart_setup(hdev, qca_baudrate);
+	ret = qca_uart_setup(hdev, qca_baudrate, QCA_ROME, soc_ver);
 	if (!ret) {
 		set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
 		qca_debugfs_init(hdev);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v8 4/7] Bluetooth: hci_qca: Add wrapper functions for setting UART speed

[Balakrishna Godavarthi]

In function qca_setup, we set initial and operating speeds for Qualcomm
Bluetooth SoC's. This block of code is common across different
Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
a wrapper function to set the speeds. So that future coming SoC's
can use these wrapper functions to set speeds.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
Changes in v8:
    * common function to set INIT and operating speeds.
    * moved hardware flow control to qca_set_speed().
	
Changes in v7:
    * initial patch
    * created wrapper functions for init and operating speeds.
---
 drivers/bluetooth/hci_qca.c | 89 +++++++++++++++++++++++++++----------
 1 file changed, 65 insertions(+), 24 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index fe62420ef838..38b7dbe6c897 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -119,6 +119,11 @@ struct qca_data {
 	u64 votes_off;
 };
 
+enum qca_speed_type {
+	QCA_INIT_SPEED = 1,
+	QCA_OPER_SPEED
+};
+
 struct qca_serdev {
 	struct hci_uart	 serdev_hu;
 	struct gpio_desc *bt_en;
@@ -923,6 +928,60 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
 		hci_uart_set_baudrate(hu, speed);
 }
 
+static unsigned int qca_get_speed(struct hci_uart *hu,
+				  enum qca_speed_type speed_type)
+{
+	unsigned int speed = 0;
+
+	if (speed_type == QCA_INIT_SPEED) {
+		if (hu->init_speed)
+			speed = hu->init_speed;
+		else if (hu->proto->init_speed)
+			speed = hu->proto->init_speed;
+	} else {
+		if (hu->oper_speed)
+			speed = hu->oper_speed;
+		else if (hu->proto->oper_speed)
+			speed = hu->proto->oper_speed;
+	}
+
+	return speed;
+}
+
+static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
+{
+	unsigned int speed, qca_baudrate;
+	int ret;
+
+	if (speed_type == QCA_INIT_SPEED) {
+		speed = qca_get_speed(hu, QCA_INIT_SPEED);
+		if (speed)
+			host_set_baudrate(hu, speed);
+		else
+			bt_dev_err(hu->hdev, "Init speed should be non zero");
+
+		return 0;
+	}
+
+	speed = qca_get_speed(hu, QCA_OPER_SPEED);
+	if (!speed) {
+		bt_dev_err(hu->hdev, "operating speed should be non zero");
+		return 0;
+	}
+
+	qca_baudrate = qca_get_baudrate_value(speed);
+	bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
+	ret = qca_set_baudrate(hu->hdev, qca_baudrate);
+	if (ret) {
+		bt_dev_err(hu->hdev, "Failed to change the baudrate (%d)", ret);
+		return ret;
+	}
+
+	host_set_baudrate(hu, speed);
+
+	return ret;
+}
+
 static int qca_setup(struct hci_uart *hu)
 {
 	struct hci_dev *hdev = hu->hdev;
@@ -937,35 +996,17 @@ static int qca_setup(struct hci_uart *hu)
 	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
 
 	/* Setup initial baudrate */
-	speed = 0;
-	if (hu->init_speed)
-		speed = hu->init_speed;
-	else if (hu->proto->init_speed)
-		speed = hu->proto->init_speed;
-
-	if (speed)
-		host_set_baudrate(hu, speed);
+	qca_set_speed(hu, QCA_INIT_SPEED);
 
 	/* Setup user speed if needed */
-	speed = 0;
-	if (hu->oper_speed)
-		speed = hu->oper_speed;
-	else if (hu->proto->oper_speed)
-		speed = hu->proto->oper_speed;
+	ret = qca_set_speed(hu, QCA_OPER_SPEED);
+	if (ret)
+		return ret;
 
-	if (speed) {
+	speed = qca_get_speed(hu, QCA_OPER_SPEED);
+	if (speed)
 		qca_baudrate = qca_get_baudrate_value(speed);
 
-		bt_dev_info(hdev, "Set UART speed to %d", speed);
-		ret = qca_set_baudrate(hdev, qca_baudrate);
-		if (ret) {
-			bt_dev_err(hdev, "Failed to change the baud rate (%d)",
-				   ret);
-			return ret;
-		}
-		host_set_baudrate(hu, speed);
-	}
-
 	/* Setup patch / NVM configurations */
 	ret = qca_uart_setup(hdev, qca_baudrate, QCA_ROME, soc_ver);
 	if (!ret) {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v8 5/7] Bluetooth: hci_qca: Enable 3.2 Mbps operating speed.

[Balakrishna Godavarthi]

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/bluetooth/hci_qca.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 38b7dbe6c897..28187a89b850 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -877,6 +877,8 @@ static uint8_t qca_get_baudrate_value(int speed)
 		return QCA_BAUDRATE_2000000;
 	case 3000000:
 		return QCA_BAUDRATE_3000000;
+	case 3200000:
+		return QCA_BAUDRATE_3200000;
 	case 3500000:
 		return QCA_BAUDRATE_3500000;
 	default:
@@ -891,7 +893,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
 	struct sk_buff *skb;
 	u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
 
-	if (baudrate > QCA_BAUDRATE_3000000)
+	if (baudrate > QCA_BAUDRATE_3200000)
 		return -EINVAL;
 
 	cmd[4] = baudrate;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v8 6/7] Bluetooth: btqca: Add wcn3990 firmware download support.

[Balakrishna Godavarthi]

This patch enables the RAM and NV patch download for wcn3990.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/bluetooth/btqca.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index 3b25be1be19c..25ef67739de0 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -332,6 +332,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 {
 	struct rome_config config;
 	int err;
+	u8 rom_ver;
 
 	bt_dev_dbg(hdev, "QCA setup on UART");
 
@@ -346,11 +347,21 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 		}
 		bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver);
 	}
-
 	/* Download rampatch file */
 	config.type = TLV_TYPE_PATCH;
-	snprintf(config.fwname, sizeof(config.fwname), "qca/rampatch_%08x.bin",
-		 soc_ver);
+	if (soc_type == QCA_WCN3990) {
+		/* Firmware files to download are based on ROM version.
+		 * ROM version is derived from last two bytes of soc_ver.
+		 */
+		rom_ver = ((soc_ver & 0x00000f00) >> 0x04) |
+			    (soc_ver & 0x0000000f);
+		snprintf(config.fwname, sizeof(config.fwname),
+			 "qca/crbtfw%02x.tlv", rom_ver);
+	} else {
+		snprintf(config.fwname, sizeof(config.fwname),
+			 "qca/rampatch_%08x.bin", soc_ver);
+	}
+
 	err = qca_download_firmware(hdev, &config);
 	if (err < 0) {
 		bt_dev_err(hdev, "QCA Failed to download patch (%d)", err);
@@ -359,8 +370,13 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 
 	/* Download NVM configuration */
 	config.type = TLV_TYPE_NVM;
-	snprintf(config.fwname, sizeof(config.fwname), "qca/nvm_%08x.bin",
-		 soc_ver);
+	if (soc_type == QCA_WCN3990)
+		snprintf(config.fwname, sizeof(config.fwname),
+			 "qca/crnv%02x.bin", rom_ver);
+	else
+		snprintf(config.fwname, sizeof(config.fwname),
+			 "qca/nvm_%08x.bin", soc_ver);
+
 	err = qca_download_firmware(hdev, &config);
 	if (err < 0) {
 		bt_dev_err(hdev, "QCA Failed to download NVM (%d)", err);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v8 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990

[Balakrishna Godavarthi]

Add support to set voltage/current of various regulators
to power up/down Bluetooth chip wcn3990.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
changes in v8:
    * closing qca buffer, if qca_power_setup fails
    * chnaged ibs start timer function call location.
    * updated review comments.
  	
changes in v7:
    * addressed review comments.

changes in v6:
    * Hooked up qca_power to qca_serdev.
    * renamed all the naming inconsistency functions with qca_*
    * leveraged common code of ROME for wcn3990.
    * created wrapper functions for re-usable blocks.
    * updated function of _*regulator_enable and _*regualtor_disable.  
    * removed redundant comments and functions.
    * addressed review comments.

Changes in v5:
    * updated regulator vddpa min_uV to 1304000.
      * addressed review comments.
 
Changes in v4:
    * Segregated the changes of btqca from hci_qca
    * rebased all changes on top of bluetooth-next.
    * addressed review comments.

---
 drivers/bluetooth/btqca.h   |   3 +
 drivers/bluetooth/hci_qca.c | 346 ++++++++++++++++++++++++++++++++----
 2 files changed, 312 insertions(+), 37 deletions(-)

diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index 24d6667eecf1..ba6bdbef9869 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -37,6 +37,9 @@
 #define EDL_TAG_ID_HCI			(17)
 #define EDL_TAG_ID_DEEP_SLEEP		(27)
 
+#define QCA_WCN3990_POWERON_PULSE	0xFC
+#define QCA_WCN3990_POWEROFF_PULSE	0xC0
+
 enum qca_bardrate {
 	QCA_BAUDRATE_115200 	= 0,
 	QCA_BAUDRATE_57600,
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 28187a89b850..bd4c9a78716f 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -5,7 +5,7 @@
  *  protocol extension to H4.
  *
  *  Copyright (C) 2007 Texas Instruments, Inc.
- *  Copyright (c) 2010, 2012 The Linux Foundation. All rights reserved.
+ *  Copyright (c) 2010, 2012, 2018 The Linux Foundation. All rights reserved.
  *
  *  Acknowledgements:
  *  This file is based on hci_ll.c, which was...
@@ -31,9 +31,14 @@
 #include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/gpio/consumer.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
 #include <linux/serdev.h>
 
 #include <net/bluetooth/bluetooth.h>
@@ -124,12 +129,46 @@ enum qca_speed_type {
 	QCA_OPER_SPEED
 };
 
+/*
+ * Voltage regulator information required for configuring the
+ * QCA Bluetooth chipset
+ */
+struct qca_vreg {
+	const char *name;
+	unsigned int min_uV;
+	unsigned int max_uV;
+	unsigned int load_uA;
+};
+
+struct qca_vreg_data {
+	enum qca_btsoc_type soc_type;
+	struct qca_vreg *vregs;
+	size_t num_vregs;
+};
+
+/*
+ * Platform data for the QCA Bluetooth power driver.
+ */
+struct qca_power {
+	struct device *dev;
+	const struct qca_vreg_data *vreg_data;
+	struct regulator_bulk_data *vreg_bulk;
+	bool vregs_on;
+};
+
 struct qca_serdev {
 	struct hci_uart	 serdev_hu;
 	struct gpio_desc *bt_en;
 	struct clk	 *susclk;
+	enum qca_btsoc_type btsoc_type;
+	struct qca_power *bt_power;
+	u32 init_speed;
+	u32 oper_speed;
 };
 
+static int qca_power_setup(struct hci_uart *hu, bool on);
+static void qca_power_shutdown(struct hci_uart *hu);
+
 static void __serial_clock_on(struct tty_struct *tty)
 {
 	/* TODO: Some chipset requires to enable UART clock on client
@@ -407,6 +446,7 @@ static int qca_open(struct hci_uart *hu)
 {
 	struct qca_serdev *qcadev;
 	struct qca_data *qca;
+	int ret;
 
 	BT_DBG("hu %p qca_open", hu);
 
@@ -458,19 +498,32 @@ static int qca_open(struct hci_uart *hu)
 
 	hu->priv = qca;
 
-	timer_setup(&qca->wake_retrans_timer, hci_ibs_wake_retrans_timeout, 0);
-	qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS;
-
-	timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0);
-	qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS;
-
 	if (hu->serdev) {
 		serdev_device_open(hu->serdev);
 
 		qcadev = serdev_device_get_drvdata(hu->serdev);
-		gpiod_set_value_cansleep(qcadev->bt_en, 1);
+		if (qcadev->btsoc_type != QCA_WCN3990) {
+			gpiod_set_value_cansleep(qcadev->bt_en, 1);
+		} else {
+			hu->init_speed = qcadev->init_speed;
+			hu->oper_speed = qcadev->oper_speed;
+			ret = qca_power_setup(hu, true);
+			if (ret) {
+				destroy_workqueue(qca->workqueue);
+				kfree_skb(qca->rx_skb);
+				hu->priv = NULL;
+				kfree(qca);
+				return ret;
+			}
+		}
 	}
 
+	timer_setup(&qca->wake_retrans_timer, hci_ibs_wake_retrans_timeout, 0);
+	qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS;
+
+	timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0);
+	qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS;
+
 	BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u",
 	       qca->tx_idle_delay, qca->wake_retrans);
 
@@ -554,10 +607,13 @@ static int qca_close(struct hci_uart *hu)
 	qca->hu = NULL;
 
 	if (hu->serdev) {
-		serdev_device_close(hu->serdev);
-
 		qcadev = serdev_device_get_drvdata(hu->serdev);
-		gpiod_set_value_cansleep(qcadev->bt_en, 0);
+		if (qcadev->btsoc_type == QCA_WCN3990)
+			qca_power_shutdown(hu);
+		else
+			gpiod_set_value_cansleep(qcadev->bt_en, 0);
+
+		serdev_device_close(hu->serdev);
 	}
 
 	kfree_skb(qca->rx_skb);
@@ -930,6 +986,32 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
 		hci_uart_set_baudrate(hu, speed);
 }
 
+static int qca_send_vendor_cmd(struct hci_dev *hdev, u8 cmd)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+	struct qca_data *qca = hu->priv;
+	struct sk_buff *skb;
+
+	bt_dev_dbg(hdev, "sending command %02x to SoC", cmd);
+
+	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
+	if (!skb) {
+		bt_dev_err(hdev, "Failed to allocate memory for vendor packet");
+		return -ENOMEM;
+	}
+
+	skb_put_u8(skb, cmd);
+	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
+
+	skb_queue_tail(&qca->txq, skb);
+	hci_uart_tx_wakeup(hu);
+
+	/* Wait for 100 uS for SoC to settle down */
+	usleep_range(100, 200);
+
+	return 0;
+}
+
 static unsigned int qca_get_speed(struct hci_uart *hu,
 				  enum qca_speed_type speed_type)
 {
@@ -952,6 +1034,7 @@ static unsigned int qca_get_speed(struct hci_uart *hu,
 
 static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 {
+	struct qca_serdev *qcadev;
 	unsigned int speed, qca_baudrate;
 	int ret;
 
@@ -971,6 +1054,13 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 		return 0;
 	}
 
+	qcadev = serdev_device_get_drvdata(hu->serdev);
+	/* Disabling hardware flow control is preferred while
+	 * sending change baud rate command to SoC.
+	 */
+	if (qcadev->btsoc_type == QCA_WCN3990)
+		hci_uart_set_flow_control(hu, true);
+
 	qca_baudrate = qca_get_baudrate_value(speed);
 	bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
 	ret = qca_set_baudrate(hu->hdev, qca_baudrate);
@@ -980,8 +1070,10 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 	}
 
 	host_set_baudrate(hu, speed);
+	if (qcadev->btsoc_type == QCA_WCN3990)
+		hci_uart_set_flow_control(hu, false);
 
-	return ret;
+	return 0;
 }
 
 static int qca_setup(struct hci_uart *hu)
@@ -989,10 +1081,11 @@ static int qca_setup(struct hci_uart *hu)
 	struct hci_dev *hdev = hu->hdev;
 	struct qca_data *qca = hu->priv;
 	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
+	struct qca_serdev *qcadev;
 	int ret;
 	int soc_ver = 0;
 
-	bt_dev_info(hdev, "ROME setup");
+	qcadev = serdev_device_get_drvdata(hu->serdev);
 
 	/* Patch downloading has to be done without IBS mode */
 	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
@@ -1000,6 +1093,35 @@ static int qca_setup(struct hci_uart *hu)
 	/* Setup initial baudrate */
 	qca_set_speed(hu, QCA_INIT_SPEED);
 
+	if (qcadev->btsoc_type == QCA_WCN3990) {
+		bt_dev_dbg(hdev, "setting up wcn3990");
+		hci_uart_set_flow_control(hu, true);
+		ret = qca_send_vendor_cmd(hdev, QCA_WCN3990_POWERON_PULSE);
+		if (ret)
+			return ret;
+
+		hci_uart_set_flow_control(hu, false);
+		serdev_device_close(hu->serdev);
+		ret = serdev_device_open(hu->serdev);
+		if (ret) {
+			bt_dev_err(hdev, "failed to open port");
+			return ret;
+		}
+
+		msleep(100);
+		/* Setup initial baudrate */
+		qca_set_speed(hu, QCA_INIT_SPEED);
+		hci_uart_set_flow_control(hu, false);
+		ret = qca_read_soc_version(hdev, &soc_ver);
+		if (ret < 0 || soc_ver == 0) {
+			bt_dev_err(hdev, "Failed to get version %d", ret);
+			return ret;
+		}
+		bt_dev_info(hdev, "wcn3990 controller version 0x%08x", soc_ver);
+	} else {
+		bt_dev_info(hdev, "ROME setup");
+	}
+
 	/* Setup user speed if needed */
 	ret = qca_set_speed(hu, QCA_OPER_SPEED);
 	if (ret)
@@ -1010,7 +1132,7 @@ static int qca_setup(struct hci_uart *hu)
 		qca_baudrate = qca_get_baudrate_value(speed);
 
 	/* Setup patch / NVM configurations */
-	ret = qca_uart_setup(hdev, qca_baudrate, QCA_ROME, soc_ver);
+	ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, soc_ver);
 	if (!ret) {
 		set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
 		qca_debugfs_init(hdev);
@@ -1046,9 +1168,124 @@ static struct hci_uart_proto qca_proto = {
 	.dequeue	= qca_dequeue,
 };
 
+static const struct qca_vreg_data qca_soc_data = {
+	.soc_type = QCA_WCN3990,
+	.vregs = (struct qca_vreg []) {
+		{ "vddio",   1352000, 1352000,  0 },
+		{ "vddxtal", 1904000, 2040000,  0 },
+		{ "vddcore", 1800000, 1800000,  1 },
+		{ "vddpa",   1304000, 1304000,  1 },
+		{ "vddldo",  3000000, 3312000,  1 },
+	},
+	.num_vregs = 5,
+};
+
+static void qca_power_shutdown(struct hci_uart *hu)
+{
+	struct hci_dev *hdev = hu->hdev;
+
+	host_set_baudrate(hu, 2400);
+	qca_send_vendor_cmd(hdev, QCA_WCN3990_POWEROFF_PULSE);
+	qca_power_setup(hu, false);
+}
+
+static int qca_enable_regulator(struct qca_vreg vregs,
+				struct regulator *regulator)
+{
+	int ret;
+
+	ret = regulator_set_voltage(regulator, vregs.min_uV,
+				    vregs.max_uV);
+	if (ret)
+		return ret;
+
+	if (vregs.load_uA)
+		ret = regulator_set_load(regulator,
+					 vregs.load_uA);
+
+	if (ret)
+		return ret;
+
+	return regulator_enable(regulator);
+
+}
+
+static void qca_disable_regulator(struct qca_vreg vregs,
+				  struct regulator *regulator)
+{
+	regulator_disable(regulator);
+	regulator_set_voltage(regulator, 0, vregs.max_uV);
+	if (vregs.load_uA)
+		regulator_set_load(regulator, 0);
+
+}
+
+static int qca_power_setup(struct hci_uart *hu, bool on)
+{
+	struct qca_vreg *vregs;
+	struct regulator_bulk_data *vreg_bulk;
+	struct qca_serdev *qcadev;
+	int i, num_vregs, ret = 0;
+
+	qcadev = serdev_device_get_drvdata(hu->serdev);
+	if (!qcadev || !qcadev->bt_power || !qcadev->bt_power->vreg_data ||
+	    !qcadev->bt_power->vreg_bulk)
+		return -EINVAL;
+
+	vregs = qcadev->bt_power->vreg_data->vregs;
+	vreg_bulk = qcadev->bt_power->vreg_bulk;
+	num_vregs = qcadev->bt_power->vreg_data->num_vregs;
+	BT_DBG("on: %d", on);
+	if (on  && !qcadev->bt_power->vregs_on) {
+		for (i = 0; i < num_vregs; i++) {
+			ret = qca_enable_regulator(vregs[i],
+						   vreg_bulk[i].consumer);
+			if (ret)
+				break;
+		}
+
+		if (ret) {
+			BT_ERR("failed to enable regulator:%s", vregs[i].name);
+			/* turn off regulators which are enabled */
+			for (i = i - 1; i >= 0; i--)
+				qca_disable_regulator(vregs[i],
+						      vreg_bulk[i].consumer);
+		} else {
+			qcadev->bt_power->vregs_on = true;
+		}
+	} else if (!on && qcadev->bt_power->vregs_on) {
+		/* turn off regulator in reverse order */
+		i = qcadev->bt_power->vreg_data->num_vregs - 1;
+		for ( ; i >= 0; i--)
+			qca_disable_regulator(vregs[i], vreg_bulk[i].consumer);
+
+		qcadev->bt_power->vregs_on = false;
+	}
+
+	return ret;
+}
+
+static int qca_init_regulators(struct qca_power *qca,
+			       const struct qca_vreg *vregs, size_t num_vregs)
+{
+	int i;
+
+	qca->vreg_bulk = devm_kzalloc(qca->dev, num_vregs *
+				      sizeof(struct regulator_bulk_data),
+				      GFP_KERNEL);
+	if (!qca->vreg_bulk)
+		return -ENOMEM;
+
+	for (i = 0; i < num_vregs; i++)
+		qca->vreg_bulk[i].supply = vregs[i].name;
+
+	return devm_regulator_bulk_get(qca->dev, num_vregs, qca->vreg_bulk);
+}
+
 static int qca_serdev_probe(struct serdev_device *serdev)
 {
 	struct qca_serdev *qcadev;
+	const struct qca_vreg_data *data;
 	int err;
 
 	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
@@ -1056,47 +1293,82 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		return -ENOMEM;
 
 	qcadev->serdev_hu.serdev = serdev;
+	data = of_device_get_match_data(&serdev->dev);
 	serdev_device_set_drvdata(serdev, qcadev);
+	if (data && data->soc_type == QCA_WCN3990) {
+		qcadev->btsoc_type = QCA_WCN3990;
+		qcadev->bt_power = devm_kzalloc(&serdev->dev,
+						sizeof(struct qca_power),
+						GFP_KERNEL);
+		if (!qcadev->bt_power)
+			return -ENOMEM;
+
+		qcadev->bt_power->dev = &serdev->dev;
+		qcadev->bt_power->vreg_data = data;
+		err = qca_init_regulators(qcadev->bt_power, data->vregs,
+					  data->num_vregs);
+		if (err) {
+			BT_ERR("Failed to init regulators:%d", err);
+			goto out;
+		}
 
-	qcadev->bt_en = devm_gpiod_get(&serdev->dev, "enable",
-				       GPIOD_OUT_LOW);
-	if (IS_ERR(qcadev->bt_en)) {
-		dev_err(&serdev->dev, "failed to acquire enable gpio\n");
-		return PTR_ERR(qcadev->bt_en);
-	}
+		qcadev->bt_power->vregs_on = false;
+		device_property_read_u32(&serdev->dev, "max-speed",
+					 &qcadev->oper_speed);
+		if (!qcadev->oper_speed)
+			BT_INFO("UART will pick default operating speed");
+		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
+		if (err) {
+			BT_ERR("wcn3990 serdev registration failed");
+			goto out;
+		}
+	} else {
+		qcadev->btsoc_type = QCA_ROME;
+		qcadev->bt_en = devm_gpiod_get(&serdev->dev, "enable",
+					       GPIOD_OUT_LOW);
+		if (IS_ERR(qcadev->bt_en)) {
+			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+			return PTR_ERR(qcadev->bt_en);
+		}
 
-	qcadev->susclk = devm_clk_get(&serdev->dev, NULL);
-	if (IS_ERR(qcadev->susclk)) {
-		dev_err(&serdev->dev, "failed to acquire clk\n");
-		return PTR_ERR(qcadev->susclk);
-	}
+		qcadev->susclk = devm_clk_get(&serdev->dev, NULL);
+		if (IS_ERR(qcadev->susclk)) {
+			dev_err(&serdev->dev, "failed to acquire clk\n");
+			return PTR_ERR(qcadev->susclk);
+		}
 
-	err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
-	if (err)
-		return err;
+		err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
+		if (err)
+			return err;
 
-	err = clk_prepare_enable(qcadev->susclk);
-	if (err)
-		return err;
+		err = clk_prepare_enable(qcadev->susclk);
+		if (err)
+			return err;
 
-	err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
-	if (err)
-		clk_disable_unprepare(qcadev->susclk);
+		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
+		if (err)
+			clk_disable_unprepare(qcadev->susclk);
+	}
+
+out:	return err;
 
-	return err;
 }
 
 static void qca_serdev_remove(struct serdev_device *serdev)
 {
 	struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
 
-	hci_uart_unregister_device(&qcadev->serdev_hu);
+	if (qcadev->btsoc_type == QCA_WCN3990)
+		qca_power_shutdown(&qcadev->serdev_hu);
+	else
+		clk_disable_unprepare(qcadev->susclk);
 
-	clk_disable_unprepare(qcadev->susclk);
+	hci_uart_unregister_device(&qcadev->serdev_hu);
 }
 
 static const struct of_device_id qca_bluetooth_of_match[] = {
 	{ .compatible = "qcom,qca6174-bt" },
+	{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v8 1/7] dt-bindings: net: bluetooth: Add device tree bindings for QTI chip wcn3990

[Rob Herring]

On Mon, Jun 25, 2018 at 07:10:07PM +0530, Balakrishna Godavarthi wrote:
> This patch enables regulators for the Qualcomm Bluetooth wcn3990
> controller.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> 
> Changes in v8:
>     * Separated the optional entries between two chips
> 
> Changes in v7:
>     * no change.
> 
> Changes in v6:
> 
>     * Changed the oper-speed to max-speed.
> 
> Changes in v5:
> 
>     * Added entry for oper-speed and init-speed.
> 
> ---
>  .../bindings/net/qualcomm-bluetooth.txt       | 30 +++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> index 0ea18a53cc29..c2b09dd2bf31 100644
> --- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> @@ -10,12 +10,23 @@ device the slave device is attached to.
>  Required properties:
>   - compatible: should contain one of the following:
>     * "qcom,qca6174-bt"
> +   * "qcom,wcn3990-bt"
> +
> +Optional properties for compatible string qcom,qca6174-bt:
>  
> -Optional properties:
>   - enable-gpios: gpio specifier used to enable chip
>   - clocks: clock provided to the controller (SUSCLK_32KHZ)
>  
> -Example:
> +Optional properties for compatible string qcom,wcn3990-bt:
> +
> + - vddio-supply: Bluetooth VDD IO regulator handle.
> + - vddxtal-supply: Bluetooth VDD XTAL regulator handle.
> + - vddpa-supply: Bluetooth VDD PA regulator handle.
> + - vddldo-supply: Bluetooth VDD LDO regulator handle.
> + - vddcore-supply: Bluetooth VDD CORE regulator handle.
> + - max-speed: Maximum operating speed of the chip.

This shouldn't be per compatible really. The definition here is wrong 
too. You should know the max baud for the chip based on the compatible. 
This property is for when the max is less than the max of either the 
chip or host UART.

Rob

Re: [PATCH v8 2/7] Bluetooth: btqca: Rename ROME specific functions to Generic functions

[Matthias Kaehlcke]

Seems you have to respin anyway due to comments in other patches, so I
squeeze in some optional improvements of the commit message.

> Subject: Rename ROME specific functions to Generic functions

nit: s/Generic/generic/

On Mon, Jun 25, 2018 at 07:10:08PM +0530, Balakrishna Godavarthi wrote:
> Some of the QCA BTSoC ROME functions, are used for different versions
> or different make of BTSoC's. Instead of duplicating the same functions
> for new chip, updating names of the functions that are used for both

nit: s/updating/update/

> chip's to keep this generic and would help in future when we would have

nit: s/chip's/chips/

> new BT SoC

nit: you might want to start a new sentence here, it's a long churn to
read :) optionally you could just say 'logs' instead of 'bt_dev_info,
bt_dev_err and bt_dev_dbg'.

> and to have generic text in bt_dev_info, bt_dev_err and
> bt_dev_dbg updated from ROME to QCA where ever possible. This avoids
> confusion to user, when using the future Qualcomm Bluetooth SoC's.
> Updated BT_DBG, BT_ERR and BT_INFO with bt_dev_dbg, bt_dev_err and
> bt_dev_info where ever applicable.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH v8 3/7] Bluetooth: btqca: Redefine qca_uart_setup() to generic function.

[Matthias Kaehlcke]

On Mon, Jun 25, 2018 at 07:10:09PM +0530, Balakrishna Godavarthi wrote:
> Redefinition of qca_uart_setup will help future Qualcomm Bluetooth
> SoC, to use the same function instead of duplicating the function.
> Added new arguments soc_type and soc_ver to the functions.
> 
> These arguments will help to decide type of firmware files
> to be loaded into Bluetooth chip.
> soc_type holds the Bluetooth chip connected to APPS processor.
> soc_ver holds the Bluetooth chip version.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> Changes in v8:
>     * updated soc_type with enum.
> 
> Changes in v7:
>     * initial patch
>     * redefined qca_uart_setup function to generic.
> ---
>  drivers/bluetooth/btqca.c   | 23 ++++++++++++-----------
>  drivers/bluetooth/btqca.h   | 13 +++++++++++--
>  drivers/bluetooth/hci_qca.c |  3 ++-
>  3 files changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index c5cf9cab438a..3b25be1be19c 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -327,9 +327,9 @@ int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>  }
>  EXPORT_SYMBOL_GPL(qca_set_bdaddr_rome);
>  
> -int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate)
> +int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> +		   enum qca_btsoc_type soc_type, u32 soc_ver)
>  {
> -	u32 rome_ver = 0;
>  	struct rome_config config;
>  	int err;
>  
> @@ -337,19 +337,20 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate)
>  
>  	config.user_baud_rate = baudrate;
>  
> -	/* Get QCA version information */
> -	err = qca_read_soc_version(hdev, &rome_ver);
> -	if (err < 0 || rome_ver == 0) {
> -		bt_dev_err(hdev, "QCA Failed to get version %d", err);
> -		return err;
> +	if (!soc_ver) {
> +		/* Get QCA version information */
> +		err = qca_read_soc_version(hdev, &soc_ver);
> +		if (err < 0 || soc_ver == 0) {
> +			bt_dev_err(hdev, "QCA Failed to get version (%d)", err);
> +			return err;
> +		}
> +		bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver);
>  	}

I thought we agreed in the discussion on "[v7,4/8] Bluetooth: btqca:
Redefine qca_uart_setup() to generic function" to call
qca_read_soc_version() in common code. Did I misinterpret that?

> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index 5c9851b11838..24d6667eecf1 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> ...
> -static inline int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate)
> +static inline int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> +				 enum qca_btsoc_type soc_type, u32 soc_ver);

Remove trailing semicolon.

Re: [PATCH v8 4/7] Bluetooth: hci_qca: Add wrapper functions for setting UART speed

[Matthias Kaehlcke]

This is a nice improvement, a few remaining questions inline.

On Mon, Jun 25, 2018 at 07:10:10PM +0530, Balakrishna Godavarthi wrote:
> In function qca_setup, we set initial and operating speeds for Qualcomm
> Bluetooth SoC's. This block of code is common across different
> Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
> a wrapper function to set the speeds. So that future coming SoC's
> can use these wrapper functions to set speeds.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> Changes in v8:
>     * common function to set INIT and operating speeds.
>     * moved hardware flow control to qca_set_speed().
> 	
> Changes in v7:
>     * initial patch
>     * created wrapper functions for init and operating speeds.
> ---
>  drivers/bluetooth/hci_qca.c | 89 +++++++++++++++++++++++++++----------
>  1 file changed, 65 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index fe62420ef838..38b7dbe6c897 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -119,6 +119,11 @@ struct qca_data {
>  	u64 votes_off;
>  };
>  
> +enum qca_speed_type {
> +	QCA_INIT_SPEED = 1,
> +	QCA_OPER_SPEED
> +};
> +
>  struct qca_serdev {
>  	struct hci_uart	 serdev_hu;
>  	struct gpio_desc *bt_en;
> @@ -923,6 +928,60 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
>  		hci_uart_set_baudrate(hu, speed);
>  }
>  
> +static unsigned int qca_get_speed(struct hci_uart *hu,
> +				  enum qca_speed_type speed_type)
> +{
> +	unsigned int speed = 0;
> +
> +	if (speed_type == QCA_INIT_SPEED) {
> +		if (hu->init_speed)
> +			speed = hu->init_speed;
> +		else if (hu->proto->init_speed)
> +			speed = hu->proto->init_speed;
> +	} else {
> +		if (hu->oper_speed)
> +			speed = hu->oper_speed;
> +		else if (hu->proto->oper_speed)
> +			speed = hu->proto->oper_speed;
> +	}
> +
> +	return speed;
> +}
> +
> +static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
> +{
> +	unsigned int speed, qca_baudrate;
> +	int ret;
> +
> +	if (speed_type == QCA_INIT_SPEED) {
> +		speed = qca_get_speed(hu, QCA_INIT_SPEED);
> +		if (speed)
> +			host_set_baudrate(hu, speed);
> +		else
> +			bt_dev_err(hu->hdev, "Init speed should be non zero");

The check for 'speed == 0' is done in multiple places. From this
code I deduce that it is expected that both INIT and OPER speed are
set to non-zero values. What happens if either of them is zero? Is the
driver still operational?

> +		return 0;
> +	}
> +
> +	speed = qca_get_speed(hu, QCA_OPER_SPEED);
> +	if (!speed) {
> +		bt_dev_err(hu->hdev, "operating speed should be non zero");
> +		return 0;
> +	}
> +
> +	qca_baudrate = qca_get_baudrate_value(speed);
> +	bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
> +	ret = qca_set_baudrate(hu->hdev, qca_baudrate);
> +	if (ret) {
> +		bt_dev_err(hu->hdev, "Failed to change the baudrate (%d)", ret);
> +		return ret;
> +	}
> +
> +	host_set_baudrate(hu, speed);
> +
> +	return ret;
> +}

In the discussion on "[v7,8/8] Bluetooth: hci_qca: Add support for
Qualcomm Bluetooth chip wcn3990" you mentioned the possbility to move
the hci_uart_set_flow_control() calls into _set_speed(). This seemed
interesting but finally it isn't done in this series. Did you
encounter that it is not feasible/desirable for some reason?

> +
>  static int qca_setup(struct hci_uart *hu)
>  {
>  	struct hci_dev *hdev = hu->hdev;
> @@ -937,35 +996,17 @@ static int qca_setup(struct hci_uart *hu)
>  	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>  
>  	/* Setup initial baudrate */
> -	speed = 0;
> -	if (hu->init_speed)
> -		speed = hu->init_speed;
> -	else if (hu->proto->init_speed)
> -		speed = hu->proto->init_speed;
> -
> -	if (speed)
> -		host_set_baudrate(hu, speed);
> +	qca_set_speed(hu, QCA_INIT_SPEED);
>  
>  	/* Setup user speed if needed */
> -	speed = 0;
> -	if (hu->oper_speed)
> -		speed = hu->oper_speed;
> -	else if (hu->proto->oper_speed)
> -		speed = hu->proto->oper_speed;
> +	ret = qca_set_speed(hu, QCA_OPER_SPEED);
> +	if (ret)
> +		return ret;
>  
> -	if (speed) {
> +	speed = qca_get_speed(hu, QCA_OPER_SPEED);
> +	if (speed)
>  		qca_baudrate = qca_get_baudrate_value(speed);

Is the check here necessary? qca_get_baudrate_value() returns
QCA_BAUDRATE_115200 for a zero speed.

Re: [PATCH v8 4/7] Bluetooth: hci_qca: Add wrapper functions for setting UART speed

[Matthias Kaehlcke]

On Mon, Jun 25, 2018 at 04:43:54PM -0700, Matthias Kaehlcke wrote:
> This is a nice improvement, a few remaining questions inline.
> 
> On Mon, Jun 25, 2018 at 07:10:10PM +0530, Balakrishna Godavarthi wrote:
> > In function qca_setup, we set initial and operating speeds for Qualcomm
> > Bluetooth SoC's. This block of code is common across different
> > Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
> > a wrapper function to set the speeds. So that future coming SoC's
> > can use these wrapper functions to set speeds.
> > 
> > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > ---
> > Changes in v8:
> >     * common function to set INIT and operating speeds.
> >     * moved hardware flow control to qca_set_speed().
> > 	
> > Changes in v7:
> >     * initial patch
> >     * created wrapper functions for init and operating speeds.
> > ---
> >  drivers/bluetooth/hci_qca.c | 89 +++++++++++++++++++++++++++----------
> >  1 file changed, 65 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > index fe62420ef838..38b7dbe6c897 100644
> > --- a/drivers/bluetooth/hci_qca.c
> > +++ b/drivers/bluetooth/hci_qca.c
> > @@ -119,6 +119,11 @@ struct qca_data {
> >  	u64 votes_off;
> >  };
> >  
> > +enum qca_speed_type {
> > +	QCA_INIT_SPEED = 1,
> > +	QCA_OPER_SPEED
> > +};
> > +
> >  struct qca_serdev {
> >  	struct hci_uart	 serdev_hu;
> >  	struct gpio_desc *bt_en;
> > @@ -923,6 +928,60 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
> >  		hci_uart_set_baudrate(hu, speed);
> >  }
> >  
> > +static unsigned int qca_get_speed(struct hci_uart *hu,
> > +				  enum qca_speed_type speed_type)
> > +{
> > +	unsigned int speed = 0;
> > +
> > +	if (speed_type == QCA_INIT_SPEED) {
> > +		if (hu->init_speed)
> > +			speed = hu->init_speed;
> > +		else if (hu->proto->init_speed)
> > +			speed = hu->proto->init_speed;
> > +	} else {
> > +		if (hu->oper_speed)
> > +			speed = hu->oper_speed;
> > +		else if (hu->proto->oper_speed)
> > +			speed = hu->proto->oper_speed;
> > +	}
> > +
> > +	return speed;
> > +}
> > +
> > +static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
> > +{
> > +	unsigned int speed, qca_baudrate;
> > +	int ret;
> > +
> > +	if (speed_type == QCA_INIT_SPEED) {
> > +		speed = qca_get_speed(hu, QCA_INIT_SPEED);
> > +		if (speed)
> > +			host_set_baudrate(hu, speed);
> > +		else
> > +			bt_dev_err(hu->hdev, "Init speed should be non zero");
> 
> The check for 'speed == 0' is done in multiple places. From this
> code I deduce that it is expected that both INIT and OPER speed are
> set to non-zero values. What happens if either of them is zero? Is the
> driver still operational?
> 
> > +		return 0;
> > +	}
> > +
> > +	speed = qca_get_speed(hu, QCA_OPER_SPEED);
> > +	if (!speed) {
> > +		bt_dev_err(hu->hdev, "operating speed should be non zero");
> > +		return 0;
> > +	}
> > +
> > +	qca_baudrate = qca_get_baudrate_value(speed);
> > +	bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
> > +	ret = qca_set_baudrate(hu->hdev, qca_baudrate);
> > +	if (ret) {
> > +		bt_dev_err(hu->hdev, "Failed to change the baudrate (%d)", ret);
> > +		return ret;
> > +	}
> > +
> > +	host_set_baudrate(hu, speed);
> > +
> > +	return ret;
> > +}
> 
> In the discussion on "[v7,8/8] Bluetooth: hci_qca: Add support for
> Qualcomm Bluetooth chip wcn3990" you mentioned the possbility to move
> the hci_uart_set_flow_control() calls into _set_speed(). This seemed
> interesting but finally it isn't done in this series. Did you
> encounter that it is not feasible/desirable for some reason?

I got that half wrong. "[v8,7/7] Bluetooth: hci_qca: Add support for
Qualcomm Bluetooth chip wcn3990" adds the flow control calls to
_set_speed() however there are still_set_flow_control() calls in
qca_setup(), which confused/s me.

Could you provide a brief summary on the situations (relevant for this
driver) in which flow controls needs to be enabled/disabled?

Re: [PATCH v8 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990

[Matthias Kaehlcke]

On Mon, Jun 25, 2018 at 07:10:13PM +0530, Balakrishna Godavarthi wrote:
> Add support to set voltage/current of various regulators
> to power up/down Bluetooth chip wcn3990.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> changes in v8:
>     * closing qca buffer, if qca_power_setup fails
>     * chnaged ibs start timer function call location.
>     * updated review comments.
>   	
> changes in v7:
>     * addressed review comments.
> 
> changes in v6:
>     * Hooked up qca_power to qca_serdev.
>     * renamed all the naming inconsistency functions with qca_*
>     * leveraged common code of ROME for wcn3990.
>     * created wrapper functions for re-usable blocks.
>     * updated function of _*regulator_enable and _*regualtor_disable.  
>     * removed redundant comments and functions.
>     * addressed review comments.
> 
> Changes in v5:
>     * updated regulator vddpa min_uV to 1304000.
>       * addressed review comments.
>  
> Changes in v4:
>     * Segregated the changes of btqca from hci_qca
>     * rebased all changes on top of bluetooth-next.
>     * addressed review comments.
> 
> ---
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 28187a89b850..bd4c9a78716f 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> ...
> +static int qca_send_vendor_cmd(struct hci_dev *hdev, u8 cmd)
> +{
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +	struct qca_data *qca = hu->priv;
> +	struct sk_buff *skb;
> +
> +	bt_dev_dbg(hdev, "sending command %02x to SoC", cmd);
> +
> +	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
> +	if (!skb) {
> +		bt_dev_err(hdev, "Failed to allocate memory for vendor packet");

As mentioned on v7, custom OOM messages should be avoided.

>  static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  {
> +	struct qca_serdev *qcadev;
>  	unsigned int speed, qca_baudrate;
>  	int ret;
>  
> @@ -971,6 +1054,13 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  		return 0;
>  	}
>  
> +	qcadev = serdev_device_get_drvdata(hu->serdev);
> +	/* Disabling hardware flow control is preferred while
> +	 * sending change baud rate command to SoC.
> +	 */

Is it only preferred or must be?

> +	if (qcadev->btsoc_type == QCA_WCN3990)
> +		hci_uart_set_flow_control(hu, true);
> +

nit: consider doing this just before qca_set_baudrate(). It doesn't
make a difference but leaves it clearer what exactly needs to be
'protected' (analogy to locking).

>  	qca_baudrate = qca_get_baudrate_value(speed);
>  	bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
>  	ret = qca_set_baudrate(hu->hdev, qca_baudrate);
> @@ -980,8 +1070,10 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>  	}
>  
>  	host_set_baudrate(hu, speed);
> +	if (qcadev->btsoc_type == QCA_WCN3990)
> +		hci_uart_set_flow_control(hu, false);

>  static int qca_setup(struct hci_uart *hu)
> @@ -989,10 +1081,11 @@ static int qca_setup(struct hci_uart *hu)
>  	struct hci_dev *hdev = hu->hdev;
>  	struct qca_data *qca = hu->priv;
>  	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
> +	struct qca_serdev *qcadev;
>  	int ret;
>  	int soc_ver = 0;
>  
> -	bt_dev_info(hdev, "ROME setup");
> +	qcadev = serdev_device_get_drvdata(hu->serdev);
>  
>  	/* Patch downloading has to be done without IBS mode */
>  	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> @@ -1000,6 +1093,35 @@ static int qca_setup(struct hci_uart *hu)
>  	/* Setup initial baudrate */
>  	qca_set_speed(hu, QCA_INIT_SPEED);
>  
> +	if (qcadev->btsoc_type == QCA_WCN3990) {
> +		bt_dev_dbg(hdev, "setting up wcn3990");
> +		hci_uart_set_flow_control(hu, true);
> +		ret = qca_send_vendor_cmd(hdev, QCA_WCN3990_POWERON_PULSE);
> +		if (ret)
> +			return ret;
> +
> +		hci_uart_set_flow_control(hu, false);
> +		serdev_device_close(hu->serdev);
> +		ret = serdev_device_open(hu->serdev);
> +		if (ret) {
> +			bt_dev_err(hdev, "failed to open port");
> +			return ret;
> +		}
> +
> +		msleep(100);

Is the sleep really related with _open() or is it rather that the
device needs to settle after the power on pulse? In the latter case
I'd suggest to do the sleep before _open(), if it doesn't make a
functional difference (makes the code a bit more self documenting).

> +		/* Setup initial baudrate */
> +		qca_set_speed(hu, QCA_INIT_SPEED);
> +		hci_uart_set_flow_control(hu, false);

This is still a bit noisy with all the open/close and flow control
stuff. If I understand correctly this essentially switches the
controller on (or resets it?) and brings it (and the driver) into a
sane state. Would it make sense to move the above block into a
wcn3990_init/reset() or so?

Re: [PATCH v8 3/7] Bluetooth: btqca: Redefine qca_uart_setup() to generic function.

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-06-26 04:50, Matthias Kaehlcke wrote:
> On Mon, Jun 25, 2018 at 07:10:09PM +0530, Balakrishna Godavarthi wrote:
>> Redefinition of qca_uart_setup will help future Qualcomm Bluetooth
>> SoC, to use the same function instead of duplicating the function.
>> Added new arguments soc_type and soc_ver to the functions.
>> 
>> These arguments will help to decide type of firmware files
>> to be loaded into Bluetooth chip.
>> soc_type holds the Bluetooth chip connected to APPS processor.
>> soc_ver holds the Bluetooth chip version.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> Changes in v8:
>>     * updated soc_type with enum.
>> 
>> Changes in v7:
>>     * initial patch
>>     * redefined qca_uart_setup function to generic.
>> ---
>>  drivers/bluetooth/btqca.c   | 23 ++++++++++++-----------
>>  drivers/bluetooth/btqca.h   | 13 +++++++++++--
>>  drivers/bluetooth/hci_qca.c |  3 ++-
>>  3 files changed, 25 insertions(+), 14 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>> index c5cf9cab438a..3b25be1be19c 100644
>> --- a/drivers/bluetooth/btqca.c
>> +++ b/drivers/bluetooth/btqca.c
>> @@ -327,9 +327,9 @@ int qca_set_bdaddr_rome(struct hci_dev *hdev, 
>> const bdaddr_t *bdaddr)
>>  }
>>  EXPORT_SYMBOL_GPL(qca_set_bdaddr_rome);
>> 
>> -int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate)
>> +int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>> +		   enum qca_btsoc_type soc_type, u32 soc_ver)
>>  {
>> -	u32 rome_ver = 0;
>>  	struct rome_config config;
>>  	int err;
>> 
>> @@ -337,19 +337,20 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t 
>> baudrate)
>> 
>>  	config.user_baud_rate = baudrate;
>> 
>> -	/* Get QCA version information */
>> -	err = qca_read_soc_version(hdev, &rome_ver);
>> -	if (err < 0 || rome_ver == 0) {
>> -		bt_dev_err(hdev, "QCA Failed to get version %d", err);
>> -		return err;
>> +	if (!soc_ver) {
>> +		/* Get QCA version information */
>> +		err = qca_read_soc_version(hdev, &soc_ver);
>> +		if (err < 0 || soc_ver == 0) {
>> +			bt_dev_err(hdev, "QCA Failed to get version (%d)", err);
>> +			return err;
>> +		}
>> +		bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver);
>>  	}
> 
> I thought we agreed in the discussion on "[v7,4/8] Bluetooth: btqca:
> Redefine qca_uart_setup() to generic function" to call
> qca_read_soc_version() in common code. Did I misinterpret that?
> 
[Bala]: After integrating wcn3990, calling qca_read_soc_version() in 
qca_setup()
         is not preferable. as we will have multiple common blocks of 
code in qca_setup.
         calling function to set an operator speed is required in the 
both the if -else blcoks


code snippet: (this i copied from previous conversation, before this 
patch series. so you may see a different function call)

    static int qca_setup(struct hci_uart *hu)
       {
         ...

         qcadev = serdev_device_get_drvdata(hu->serdev);

         /* Patch downloading has to be done without IBS mode */
         clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);

         /* Setup initial baudrate */
        qca_set_speed(hu, speed, QCA_INIT_SPEED);

         if (qcadev->btsoc_type == QCA_WCN3990) {
                 bt_dev_dbg(hdev, "setting up wcn3990");
                 hci_uart_set_flow_control(hu, true);
                 ret = qca_send_vendor_cmd(hdev, 
QCA_WCN3990_POWERON_PULSE);
                 if (ret) {
                         bt_dev_err(hdev, "failed to send power on 
command");
                         return ret;
                 }

                 serdev_device_close(hu->serdev);
                 ret = serdev_device_open(hu->serdev);
                 if (ret) {
                         bt_dev_err(hdev, "failed to open port");
                         return ret;
                 }

                 msleep(100);
                 speed = qca_get_speed(hu, QCA_INIT_SPEED);
                 if (speed)
                         qca_set_speed(hu, speed, QCA_INIT_SPEED);

                 hci_uart_set_flow_control(hu, false);
         } else {
                 bt_dev_info(hdev, "ROME setup");
                 /* Setup user speed if needed */
                 speed = qca_get_speed(hu, QCA_OPER_SPEED);
                 if (speed) {
                         ret = qca_set_speed(hu, speed, QCA_OPER_SPEED);
                         if (ret)
                                 return ret;

                         qca_baudrate = qca_get_baudrate_value(speed);
                 }
         }

         ret = qca_read_soc_version(hdev, &soc_ver);
         if (ret < 0 || soc_ver == 0) {
                 bt_dev_err(hdev, "Failed to get version %d", ret);
                 return ret;
         }
         bt_dev_info(hdev, "wcn3990 controller version 0x%08x", soc_ver);


         if (qcadev->btsoc_type == QCA_WCN3990) {
                 hci_uart_set_flow_control(hu, true);
                 /* Setup user speed if needed */
                 speed = qca_get_speed(hu, QCA_OPER_SPEED);
                 if (speed) {
                         ret = qca_set_speed(hu, speed, QCA_OPER_SPEED);
                         if (ret)
                                 return ret;

                 qca_baudrate = qca_get_baudrate_value(speed);

         }

         /* Setup patch / NVM configurations */
         ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, 
soc_ver);
         if (!ret) {
                 set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
                 qca_debugfs_init(hdev);
         } else if (ret == -ENOENT) {
                 /* No patch/nvm-config found, run with original 
fw/config */
                 ret = 0;
         } else if (ret == -EAGAIN) {
                 /*
       ...
       }

>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>> index 5c9851b11838..24d6667eecf1 100644
>> --- a/drivers/bluetooth/btqca.h
>> +++ b/drivers/bluetooth/btqca.h
>> ...
>> -static inline int qca_uart_setup(struct hci_dev *hdev, uint8_t 
>> baudrate)
>> +static inline int qca_uart_setup(struct hci_dev *hdev, uint8_t 
>> baudrate,
>> +				 enum qca_btsoc_type soc_type, u32 soc_ver);
> 
> Remove trailing semicolon.

[Bala]: i didn't get you.


-- 
Regards
Balakrishna.

Re: [PATCH v8 4/7] Bluetooth: hci_qca: Add wrapper functions for setting UART speed

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-06-26 05:13, Matthias Kaehlcke wrote:
> This is a nice improvement, a few remaining questions inline.
> 
> On Mon, Jun 25, 2018 at 07:10:10PM +0530, Balakrishna Godavarthi wrote:
>> In function qca_setup, we set initial and operating speeds for 
>> Qualcomm
>> Bluetooth SoC's. This block of code is common across different
>> Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
>> a wrapper function to set the speeds. So that future coming SoC's
>> can use these wrapper functions to set speeds.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> Changes in v8:
>>     * common function to set INIT and operating speeds.
>>     * moved hardware flow control to qca_set_speed().
>> 
>> Changes in v7:
>>     * initial patch
>>     * created wrapper functions for init and operating speeds.
>> ---
>>  drivers/bluetooth/hci_qca.c | 89 
>> +++++++++++++++++++++++++++----------
>>  1 file changed, 65 insertions(+), 24 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index fe62420ef838..38b7dbe6c897 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -119,6 +119,11 @@ struct qca_data {
>>  	u64 votes_off;
>>  };
>> 
>> +enum qca_speed_type {
>> +	QCA_INIT_SPEED = 1,
>> +	QCA_OPER_SPEED
>> +};
>> +
>>  struct qca_serdev {
>>  	struct hci_uart	 serdev_hu;
>>  	struct gpio_desc *bt_en;
>> @@ -923,6 +928,60 @@ static inline void host_set_baudrate(struct 
>> hci_uart *hu, unsigned int speed)
>>  		hci_uart_set_baudrate(hu, speed);
>>  }
>> 
>> +static unsigned int qca_get_speed(struct hci_uart *hu,
>> +				  enum qca_speed_type speed_type)
>> +{
>> +	unsigned int speed = 0;
>> +
>> +	if (speed_type == QCA_INIT_SPEED) {
>> +		if (hu->init_speed)
>> +			speed = hu->init_speed;
>> +		else if (hu->proto->init_speed)
>> +			speed = hu->proto->init_speed;
>> +	} else {
>> +		if (hu->oper_speed)
>> +			speed = hu->oper_speed;
>> +		else if (hu->proto->oper_speed)
>> +			speed = hu->proto->oper_speed;
>> +	}
>> +
>> +	return speed;
>> +}
>> +
>> +static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type 
>> speed_type)
>> +{
>> +	unsigned int speed, qca_baudrate;
>> +	int ret;
>> +
>> +	if (speed_type == QCA_INIT_SPEED) {
>> +		speed = qca_get_speed(hu, QCA_INIT_SPEED);
>> +		if (speed)
>> +			host_set_baudrate(hu, speed);
>> +		else
>> +			bt_dev_err(hu->hdev, "Init speed should be non zero");
> 
> The check for 'speed == 0' is done in multiple places. From this
> code I deduce that it is expected that both INIT and OPER speed are
> set to non-zero values. What happens if either of them is zero? Is the
> driver still operational?
> 
[Bala]: yes in hci_uart_setup()(hci_serdev.c) function before calling 
qca_setup().
         we actually set baudrate, but int  above code missed to restrict 
the further call to qca_setup()
        if speed =0. so we are checking the same in the qca_setup().. 
i.e. qca_get_speed().


>> +		return 0;
>> +	}
>> +
>> +	speed = qca_get_speed(hu, QCA_OPER_SPEED);
>> +	if (!speed) {
>> +		bt_dev_err(hu->hdev, "operating speed should be non zero");
>> +		return 0;
>> +	}
>> +
>> +	qca_baudrate = qca_get_baudrate_value(speed);
>> +	bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
>> +	ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>> +	if (ret) {
>> +		bt_dev_err(hu->hdev, "Failed to change the baudrate (%d)", ret);
>> +		return ret;
>> +	}
>> +
>> +	host_set_baudrate(hu, speed);
>> +
>> +	return ret;
>> +}
> 
> In the discussion on "[v7,8/8] Bluetooth: hci_qca: Add support for
> Qualcomm Bluetooth chip wcn3990" you mentioned the possbility to move
> the hci_uart_set_flow_control() calls into _set_speed(). This seemed
> interesting but finally it isn't done in this series. Did you
> encounter that it is not feasible/desirable for some reason?
> 

[Bala]: this patch is for rome where flow control is not used.
         after we integrate wcn3990, flow control is hidden in the 
qca_set_speed()
         Pls check [v8 7/7] patch.

>> +
>>  static int qca_setup(struct hci_uart *hu)
>>  {
>>  	struct hci_dev *hdev = hu->hdev;
>> @@ -937,35 +996,17 @@ static int qca_setup(struct hci_uart *hu)
>>  	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> 
>>  	/* Setup initial baudrate */
>> -	speed = 0;
>> -	if (hu->init_speed)
>> -		speed = hu->init_speed;
>> -	else if (hu->proto->init_speed)
>> -		speed = hu->proto->init_speed;
>> -
>> -	if (speed)
>> -		host_set_baudrate(hu, speed);
>> +	qca_set_speed(hu, QCA_INIT_SPEED);
>> 
>>  	/* Setup user speed if needed */
>> -	speed = 0;
>> -	if (hu->oper_speed)
>> -		speed = hu->oper_speed;
>> -	else if (hu->proto->oper_speed)
>> -		speed = hu->proto->oper_speed;
>> +	ret = qca_set_speed(hu, QCA_OPER_SPEED);
>> +	if (ret)
>> +		return ret;
>> 
>> -	if (speed) {
>> +	speed = qca_get_speed(hu, QCA_OPER_SPEED);
>> +	if (speed)
>>  		qca_baudrate = qca_get_baudrate_value(speed);
> 
> Is the check here necessary? qca_get_baudrate_value() returns
> QCA_BAUDRATE_115200 for a zero speed.

this if for no zero operating speed,

-- 
Regards
Balakrishna.

Re: [PATCH v8 4/7] Bluetooth: hci_qca: Add wrapper functions for setting UART speed

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-06-26 05:35, Matthias Kaehlcke wrote:
> On Mon, Jun 25, 2018 at 04:43:54PM -0700, Matthias Kaehlcke wrote:
>> This is a nice improvement, a few remaining questions inline.
>> 
>> On Mon, Jun 25, 2018 at 07:10:10PM +0530, Balakrishna Godavarthi 
>> wrote:
>> > In function qca_setup, we set initial and operating speeds for Qualcomm
>> > Bluetooth SoC's. This block of code is common across different
>> > Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
>> > a wrapper function to set the speeds. So that future coming SoC's
>> > can use these wrapper functions to set speeds.
>> >
>> > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> > ---
>> > Changes in v8:
>> >     * common function to set INIT and operating speeds.
>> >     * moved hardware flow control to qca_set_speed().
>> >
>> > Changes in v7:
>> >     * initial patch
>> >     * created wrapper functions for init and operating speeds.
>> > ---
>> >  drivers/bluetooth/hci_qca.c | 89 +++++++++++++++++++++++++++----------
>> >  1 file changed, 65 insertions(+), 24 deletions(-)
>> >
>> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > index fe62420ef838..38b7dbe6c897 100644
>> > --- a/drivers/bluetooth/hci_qca.c
>> > +++ b/drivers/bluetooth/hci_qca.c
>> > @@ -119,6 +119,11 @@ struct qca_data {
>> >  	u64 votes_off;
>> >  };
>> >
>> > +enum qca_speed_type {
>> > +	QCA_INIT_SPEED = 1,
>> > +	QCA_OPER_SPEED
>> > +};
>> > +
>> >  struct qca_serdev {
>> >  	struct hci_uart	 serdev_hu;
>> >  	struct gpio_desc *bt_en;
>> > @@ -923,6 +928,60 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
>> >  		hci_uart_set_baudrate(hu, speed);
>> >  }
>> >
>> > +static unsigned int qca_get_speed(struct hci_uart *hu,
>> > +				  enum qca_speed_type speed_type)
>> > +{
>> > +	unsigned int speed = 0;
>> > +
>> > +	if (speed_type == QCA_INIT_SPEED) {
>> > +		if (hu->init_speed)
>> > +			speed = hu->init_speed;
>> > +		else if (hu->proto->init_speed)
>> > +			speed = hu->proto->init_speed;
>> > +	} else {
>> > +		if (hu->oper_speed)
>> > +			speed = hu->oper_speed;
>> > +		else if (hu->proto->oper_speed)
>> > +			speed = hu->proto->oper_speed;
>> > +	}
>> > +
>> > +	return speed;
>> > +}
>> > +
>> > +static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>> > +{
>> > +	unsigned int speed, qca_baudrate;
>> > +	int ret;
>> > +
>> > +	if (speed_type == QCA_INIT_SPEED) {
>> > +		speed = qca_get_speed(hu, QCA_INIT_SPEED);
>> > +		if (speed)
>> > +			host_set_baudrate(hu, speed);
>> > +		else
>> > +			bt_dev_err(hu->hdev, "Init speed should be non zero");
>> 
>> The check for 'speed == 0' is done in multiple places. From this
>> code I deduce that it is expected that both INIT and OPER speed are
>> set to non-zero values. What happens if either of them is zero? Is the
>> driver still operational?
>> 
>> > +		return 0;
>> > +	}
>> > +
>> > +	speed = qca_get_speed(hu, QCA_OPER_SPEED);
>> > +	if (!speed) {
>> > +		bt_dev_err(hu->hdev, "operating speed should be non zero");
>> > +		return 0;
>> > +	}
>> > +
>> > +	qca_baudrate = qca_get_baudrate_value(speed);
>> > +	bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
>> > +	ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>> > +	if (ret) {
>> > +		bt_dev_err(hu->hdev, "Failed to change the baudrate (%d)", ret);
>> > +		return ret;
>> > +	}
>> > +
>> > +	host_set_baudrate(hu, speed);
>> > +
>> > +	return ret;
>> > +}
>> 
>> In the discussion on "[v7,8/8] Bluetooth: hci_qca: Add support for
>> Qualcomm Bluetooth chip wcn3990" you mentioned the possbility to move
>> the hci_uart_set_flow_control() calls into _set_speed(). This seemed
>> interesting but finally it isn't done in this series. Did you
>> encounter that it is not feasible/desirable for some reason?
> 
> I got that half wrong. "[v8,7/7] Bluetooth: hci_qca: Add support for
> Qualcomm Bluetooth chip wcn3990" adds the flow control calls to
> _set_speed() however there are still_set_flow_control() calls in
> qca_setup(), which confused/s me.
> 
> Could you provide a brief summary on the situations (relevant for this
> driver) in which flow controls needs to be enabled/disabled?

you will not find enable or disable of hardware flow control in this 
patch.
there is no hardware flow control in ROME chip.
you will find hardware flow control in wcn3990 i.e. patch [v8 7/7]

in wcn3990. we disable hardware flow control, when we sent mandatory 
commands to BT chip.

i.e while sending power on pulse i.e 0xFC byte for wcn3990 to boot up 
completely and sending change baudrate request to BT chip.
before sending these commands, we disable the chip flow control and 
enable flow control once we sent these commands.

so in our current code after integrating wcn3990, we disable flow 
control two times.

1. Before sending power on pulse i.e. qca_send_vendor_cmd(hdev, 
QCA_WCN3990_POWERON_PULSE); in qca_setup.
    so we find disable or enable hardware flow control in qca_setup()
2. Before sending change BT CHIP baudrate request i.e.  
qca_set_baudrate(hu->hdev, qca_baudrate); in qca_set_speed().



-- 
Regards
Balakrishna.

Re: [PATCH v8 4/7] Bluetooth: hci_qca: Add wrapper functions for setting UART speed

[Matthias Kaehlcke]

On Tue, Jun 26, 2018 at 10:43:31AM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-06-26 05:35, Matthias Kaehlcke wrote:
> > On Mon, Jun 25, 2018 at 04:43:54PM -0700, Matthias Kaehlcke wrote:
> > > This is a nice improvement, a few remaining questions inline.
> > > 
> > > On Mon, Jun 25, 2018 at 07:10:10PM +0530, Balakrishna Godavarthi
> > > wrote:
> > > > In function qca_setup, we set initial and operating speeds for Qualcomm
> > > > Bluetooth SoC's. This block of code is common across different
> > > > Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
> > > > a wrapper function to set the speeds. So that future coming SoC's
> > > > can use these wrapper functions to set speeds.
> > > >
> > > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > > > ---
> > > > Changes in v8:
> > > >     * common function to set INIT and operating speeds.
> > > >     * moved hardware flow control to qca_set_speed().
> > > >
> > > > Changes in v7:
> > > >     * initial patch
> > > >     * created wrapper functions for init and operating speeds.
> > > > ---
> > > >  drivers/bluetooth/hci_qca.c | 89 +++++++++++++++++++++++++++----------
> > > >  1 file changed, 65 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > > index fe62420ef838..38b7dbe6c897 100644
> > > > --- a/drivers/bluetooth/hci_qca.c
> > > > +++ b/drivers/bluetooth/hci_qca.c
> > > > @@ -119,6 +119,11 @@ struct qca_data {
> > > >  	u64 votes_off;
> > > >  };
> > > >
> > > > +enum qca_speed_type {
> > > > +	QCA_INIT_SPEED = 1,
> > > > +	QCA_OPER_SPEED
> > > > +};
> > > > +
> > > >  struct qca_serdev {
> > > >  	struct hci_uart	 serdev_hu;
> > > >  	struct gpio_desc *bt_en;
> > > > @@ -923,6 +928,60 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
> > > >  		hci_uart_set_baudrate(hu, speed);
> > > >  }
> > > >
> > > > +static unsigned int qca_get_speed(struct hci_uart *hu,
> > > > +				  enum qca_speed_type speed_type)
> > > > +{
> > > > +	unsigned int speed = 0;
> > > > +
> > > > +	if (speed_type == QCA_INIT_SPEED) {
> > > > +		if (hu->init_speed)
> > > > +			speed = hu->init_speed;
> > > > +		else if (hu->proto->init_speed)
> > > > +			speed = hu->proto->init_speed;
> > > > +	} else {
> > > > +		if (hu->oper_speed)
> > > > +			speed = hu->oper_speed;
> > > > +		else if (hu->proto->oper_speed)
> > > > +			speed = hu->proto->oper_speed;
> > > > +	}
> > > > +
> > > > +	return speed;
> > > > +}
> > > > +
> > > > +static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
> > > > +{
> > > > +	unsigned int speed, qca_baudrate;
> > > > +	int ret;
> > > > +
> > > > +	if (speed_type == QCA_INIT_SPEED) {
> > > > +		speed = qca_get_speed(hu, QCA_INIT_SPEED);
> > > > +		if (speed)
> > > > +			host_set_baudrate(hu, speed);
> > > > +		else
> > > > +			bt_dev_err(hu->hdev, "Init speed should be non zero");
> > > 
> > > The check for 'speed == 0' is done in multiple places. From this
> > > code I deduce that it is expected that both INIT and OPER speed are
> > > set to non-zero values. What happens if either of them is zero? Is the
> > > driver still operational?
> > > 
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	speed = qca_get_speed(hu, QCA_OPER_SPEED);
> > > > +	if (!speed) {
> > > > +		bt_dev_err(hu->hdev, "operating speed should be non zero");
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	qca_baudrate = qca_get_baudrate_value(speed);
> > > > +	bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
> > > > +	ret = qca_set_baudrate(hu->hdev, qca_baudrate);
> > > > +	if (ret) {
> > > > +		bt_dev_err(hu->hdev, "Failed to change the baudrate (%d)", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	host_set_baudrate(hu, speed);
> > > > +
> > > > +	return ret;
> > > > +}
> > > 
> > > In the discussion on "[v7,8/8] Bluetooth: hci_qca: Add support for
> > > Qualcomm Bluetooth chip wcn3990" you mentioned the possbility to move
> > > the hci_uart_set_flow_control() calls into _set_speed(). This seemed
> > > interesting but finally it isn't done in this series. Did you
> > > encounter that it is not feasible/desirable for some reason?
> > 
> > I got that half wrong. "[v8,7/7] Bluetooth: hci_qca: Add support for
> > Qualcomm Bluetooth chip wcn3990" adds the flow control calls to
> > _set_speed() however there are still_set_flow_control() calls in
> > qca_setup(), which confused/s me.
> > 
> > Could you provide a brief summary on the situations (relevant for this
> > driver) in which flow controls needs to be enabled/disabled?
> 
> you will not find enable or disable of hardware flow control in this patch.
> there is no hardware flow control in ROME chip.
> you will find hardware flow control in wcn3990 i.e. patch [v8 7/7]

Makes sense, when looking first at this I forgot the flow control
handling was only added for wcn3990.

> in wcn3990. we disable hardware flow control, when we sent mandatory
> commands to BT chip.
> 
> i.e while sending power on pulse i.e 0xFC byte for wcn3990 to boot up
> completely and sending change baudrate request to BT chip.
> before sending these commands, we disable the chip flow control and enable
> flow control once we sent these commands.
> 
> so in our current code after integrating wcn3990, we disable flow control
> two times.
> 
> 1. Before sending power on pulse i.e. qca_send_vendor_cmd(hdev,
> QCA_WCN3990_POWERON_PULSE); in qca_setup.
>    so we find disable or enable hardware flow control in qca_setup()
> 2. Before sending change BT CHIP baudrate request i.e.
> qca_set_baudrate(hu->hdev, qca_baudrate); in qca_set_speed().

Thanks for the information!

If there are more mandatory commands it could be an option to have a
wrapper for them or do the flow control handling in
qca_send_vendor_cmd() to avoid cluttering the main code path.

Just an idea, no need to do this now.

Re: [PATCH v8 4/7] Bluetooth: hci_qca: Add wrapper functions for setting UART speed

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-06-27 00:02, Matthias Kaehlcke wrote:
> On Tue, Jun 26, 2018 at 10:43:31AM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-06-26 05:35, Matthias Kaehlcke wrote:
>> > On Mon, Jun 25, 2018 at 04:43:54PM -0700, Matthias Kaehlcke wrote:
>> > > This is a nice improvement, a few remaining questions inline.
>> > >
>> > > On Mon, Jun 25, 2018 at 07:10:10PM +0530, Balakrishna Godavarthi
>> > > wrote:
>> > > > In function qca_setup, we set initial and operating speeds for Qualcomm
>> > > > Bluetooth SoC's. This block of code is common across different
>> > > > Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
>> > > > a wrapper function to set the speeds. So that future coming SoC's
>> > > > can use these wrapper functions to set speeds.
>> > > >
>> > > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> > > > ---
>> > > > Changes in v8:
>> > > >     * common function to set INIT and operating speeds.
>> > > >     * moved hardware flow control to qca_set_speed().
>> > > >
>> > > > Changes in v7:
>> > > >     * initial patch
>> > > >     * created wrapper functions for init and operating speeds.
>> > > > ---
>> > > >  drivers/bluetooth/hci_qca.c | 89 +++++++++++++++++++++++++++----------
>> > > >  1 file changed, 65 insertions(+), 24 deletions(-)
>> > > >
>> > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > > > index fe62420ef838..38b7dbe6c897 100644
>> > > > --- a/drivers/bluetooth/hci_qca.c
>> > > > +++ b/drivers/bluetooth/hci_qca.c
>> > > > @@ -119,6 +119,11 @@ struct qca_data {
>> > > >  	u64 votes_off;
>> > > >  };
>> > > >
>> > > > +enum qca_speed_type {
>> > > > +	QCA_INIT_SPEED = 1,
>> > > > +	QCA_OPER_SPEED
>> > > > +};
>> > > > +
>> > > >  struct qca_serdev {
>> > > >  	struct hci_uart	 serdev_hu;
>> > > >  	struct gpio_desc *bt_en;
>> > > > @@ -923,6 +928,60 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
>> > > >  		hci_uart_set_baudrate(hu, speed);
>> > > >  }
>> > > >
>> > > > +static unsigned int qca_get_speed(struct hci_uart *hu,
>> > > > +				  enum qca_speed_type speed_type)
>> > > > +{
>> > > > +	unsigned int speed = 0;
>> > > > +
>> > > > +	if (speed_type == QCA_INIT_SPEED) {
>> > > > +		if (hu->init_speed)
>> > > > +			speed = hu->init_speed;
>> > > > +		else if (hu->proto->init_speed)
>> > > > +			speed = hu->proto->init_speed;
>> > > > +	} else {
>> > > > +		if (hu->oper_speed)
>> > > > +			speed = hu->oper_speed;
>> > > > +		else if (hu->proto->oper_speed)
>> > > > +			speed = hu->proto->oper_speed;
>> > > > +	}
>> > > > +
>> > > > +	return speed;
>> > > > +}
>> > > > +
>> > > > +static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
>> > > > +{
>> > > > +	unsigned int speed, qca_baudrate;
>> > > > +	int ret;
>> > > > +
>> > > > +	if (speed_type == QCA_INIT_SPEED) {
>> > > > +		speed = qca_get_speed(hu, QCA_INIT_SPEED);
>> > > > +		if (speed)
>> > > > +			host_set_baudrate(hu, speed);
>> > > > +		else
>> > > > +			bt_dev_err(hu->hdev, "Init speed should be non zero");
>> > >
>> > > The check for 'speed == 0' is done in multiple places. From this
>> > > code I deduce that it is expected that both INIT and OPER speed are
>> > > set to non-zero values. What happens if either of them is zero? Is the
>> > > driver still operational?
>> > >
>> > > > +		return 0;
>> > > > +	}
>> > > > +
>> > > > +	speed = qca_get_speed(hu, QCA_OPER_SPEED);
>> > > > +	if (!speed) {
>> > > > +		bt_dev_err(hu->hdev, "operating speed should be non zero");
>> > > > +		return 0;
>> > > > +	}
>> > > > +
>> > > > +	qca_baudrate = qca_get_baudrate_value(speed);
>> > > > +	bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
>> > > > +	ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>> > > > +	if (ret) {
>> > > > +		bt_dev_err(hu->hdev, "Failed to change the baudrate (%d)", ret);
>> > > > +		return ret;
>> > > > +	}
>> > > > +
>> > > > +	host_set_baudrate(hu, speed);
>> > > > +
>> > > > +	return ret;
>> > > > +}
>> > >
>> > > In the discussion on "[v7,8/8] Bluetooth: hci_qca: Add support for
>> > > Qualcomm Bluetooth chip wcn3990" you mentioned the possbility to move
>> > > the hci_uart_set_flow_control() calls into _set_speed(). This seemed
>> > > interesting but finally it isn't done in this series. Did you
>> > > encounter that it is not feasible/desirable for some reason?
>> >
>> > I got that half wrong. "[v8,7/7] Bluetooth: hci_qca: Add support for
>> > Qualcomm Bluetooth chip wcn3990" adds the flow control calls to
>> > _set_speed() however there are still_set_flow_control() calls in
>> > qca_setup(), which confused/s me.
>> >
>> > Could you provide a brief summary on the situations (relevant for this
>> > driver) in which flow controls needs to be enabled/disabled?
>> 
>> you will not find enable or disable of hardware flow control in this 
>> patch.
>> there is no hardware flow control in ROME chip.
>> you will find hardware flow control in wcn3990 i.e. patch [v8 7/7]
> 
> Makes sense, when looking first at this I forgot the flow control
> handling was only added for wcn3990.
> 
>> in wcn3990. we disable hardware flow control, when we sent mandatory
>> commands to BT chip.
>> 
>> i.e while sending power on pulse i.e 0xFC byte for wcn3990 to boot up
>> completely and sending change baudrate request to BT chip.
>> before sending these commands, we disable the chip flow control and 
>> enable
>> flow control once we sent these commands.
>> 
>> so in our current code after integrating wcn3990, we disable flow 
>> control
>> two times.
>> 
>> 1. Before sending power on pulse i.e. qca_send_vendor_cmd(hdev,
>> QCA_WCN3990_POWERON_PULSE); in qca_setup.
>>    so we find disable or enable hardware flow control in qca_setup()
>> 2. Before sending change BT CHIP baudrate request i.e.
>> qca_set_baudrate(hu->hdev, qca_baudrate); in qca_set_speed().
> 
> Thanks for the information!
> 
> If there are more mandatory commands it could be an option to have a
> wrapper for them or do the flow control handling in
> qca_send_vendor_cmd() to avoid cluttering the main code path.
> 
> Just an idea, no need to do this now.

I will check the possibilites of hiding flow control in 
qca_send_vendor_command()
in  [v8 7/8] patch

Pls let me know if they are any changes required in this patch.

Thanks for reviewing.

-- 
Regards
Balakrishna.

Re: [PATCH v8 4/7] Bluetooth: hci_qca: Add wrapper functions for setting UART speed

[Matthias Kaehlcke]

On Tue, Jun 26, 2018 at 07:01:18AM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-06-26 05:13, Matthias Kaehlcke wrote:
> > This is a nice improvement, a few remaining questions inline.
> > 
> > On Mon, Jun 25, 2018 at 07:10:10PM +0530, Balakrishna Godavarthi wrote:
> > > In function qca_setup, we set initial and operating speeds for
> > > Qualcomm
> > > Bluetooth SoC's. This block of code is common across different
> > > Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
> > > a wrapper function to set the speeds. So that future coming SoC's
> > > can use these wrapper functions to set speeds.
> > > 
> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > > ---
> > > Changes in v8:
> > >     * common function to set INIT and operating speeds.
> > >     * moved hardware flow control to qca_set_speed().
> > > 
> > > Changes in v7:
> > >     * initial patch
> > >     * created wrapper functions for init and operating speeds.
> > > ---
> > >  drivers/bluetooth/hci_qca.c | 89
> > > +++++++++++++++++++++++++++----------
> > >  1 file changed, 65 insertions(+), 24 deletions(-)
> > > 
> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > index fe62420ef838..38b7dbe6c897 100644
> > > --- a/drivers/bluetooth/hci_qca.c
> > > +++ b/drivers/bluetooth/hci_qca.c
> > > @@ -119,6 +119,11 @@ struct qca_data {
> > >  	u64 votes_off;
> > >  };
> > > 
> > > +enum qca_speed_type {
> > > +	QCA_INIT_SPEED = 1,
> > > +	QCA_OPER_SPEED
> > > +};
> > > +
> > >  struct qca_serdev {
> > >  	struct hci_uart	 serdev_hu;
> > >  	struct gpio_desc *bt_en;
> > > @@ -923,6 +928,60 @@ static inline void host_set_baudrate(struct
> > > hci_uart *hu, unsigned int speed)
> > >  		hci_uart_set_baudrate(hu, speed);
> > >  }
> > > 
> > > +static unsigned int qca_get_speed(struct hci_uart *hu,
> > > +				  enum qca_speed_type speed_type)
> > > +{
> > > +	unsigned int speed = 0;
> > > +
> > > +	if (speed_type == QCA_INIT_SPEED) {
> > > +		if (hu->init_speed)
> > > +			speed = hu->init_speed;
> > > +		else if (hu->proto->init_speed)
> > > +			speed = hu->proto->init_speed;
> > > +	} else {
> > > +		if (hu->oper_speed)
> > > +			speed = hu->oper_speed;
> > > +		else if (hu->proto->oper_speed)
> > > +			speed = hu->proto->oper_speed;
> > > +	}
> > > +
> > > +	return speed;
> > > +}
> > > +
> > > +static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type
> > > speed_type)
> > > +{
> > > +	unsigned int speed, qca_baudrate;
> > > +	int ret;
> > > +
> > > +	if (speed_type == QCA_INIT_SPEED) {
> > > +		speed = qca_get_speed(hu, QCA_INIT_SPEED);
> > > +		if (speed)
> > > +			host_set_baudrate(hu, speed);
> > > +		else
> > > +			bt_dev_err(hu->hdev, "Init speed should be non zero");
> > 
> > The check for 'speed == 0' is done in multiple places. From this
> > code I deduce that it is expected that both INIT and OPER speed are
> > set to non-zero values. What happens if either of them is zero? Is the
> > driver still operational?
> > 
> [Bala]: yes in hci_uart_setup()(hci_serdev.c) function before calling
> qca_setup().
>         we actually set baudrate, but int  above code missed to restrict the
> further call to qca_setup()
>        if speed =0. so we are checking the same in the qca_setup().. i.e.
> qca_get_speed().

Sorry, didn't quite get you here. Yes, the driver is still operational?

Breaking it down in multiple questions:

1. Is it an error if one of the speeds isn't specified?

If yes we should probably check this early once and return an error
early, instead of doing the check repeatedly

2. If it is not an error, what is the driver supposed to do?

> > In the discussion on "[v7,8/8] Bluetooth: hci_qca: Add support for
> > Qualcomm Bluetooth chip wcn3990" you mentioned the possbility to move
> > the hci_uart_set_flow_control() calls into _set_speed(). This seemed
> > interesting but finally it isn't done in this series. Did you
> > encounter that it is not feasible/desirable for some reason?
> > 
> 
> [Bala]: this patch is for rome where flow control is not used.
>         after we integrate wcn3990, flow control is hidden in the
> qca_set_speed()
>         Pls check [v8 7/7] patch.

Sorry, my confusion

> > >  static int qca_setup(struct hci_uart *hu)
> > >  {
> > >  	struct hci_dev *hdev = hu->hdev;
> > > @@ -937,35 +996,17 @@ static int qca_setup(struct hci_uart *hu)
> > >  	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> > > 
> > >  	/* Setup initial baudrate */
> > > -	speed = 0;
> > > -	if (hu->init_speed)
> > > -		speed = hu->init_speed;
> > > -	else if (hu->proto->init_speed)
> > > -		speed = hu->proto->init_speed;
> > > -
> > > -	if (speed)
> > > -		host_set_baudrate(hu, speed);
> > > +	qca_set_speed(hu, QCA_INIT_SPEED);
> > > 
> > >  	/* Setup user speed if needed */
> > > -	speed = 0;
> > > -	if (hu->oper_speed)
> > > -		speed = hu->oper_speed;
> > > -	else if (hu->proto->oper_speed)
> > > -		speed = hu->proto->oper_speed;
> > > +	ret = qca_set_speed(hu, QCA_OPER_SPEED);
> > > +	if (ret)
> > > +		return ret;
> > > 
> > > -	if (speed) {
> > > +	speed = qca_get_speed(hu, QCA_OPER_SPEED);
> > > +	if (speed)
> > >  		qca_baudrate = qca_get_baudrate_value(speed);
> > 
> > Is the check here necessary? qca_get_baudrate_value() returns
> > QCA_BAUDRATE_115200 for a zero speed.
> 
> this if for no zero operating speed,

My point is:

static int qca_setup(struct hci_uart *hu)
{
	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;

	...

	if (speed)
		qca_baudrate = qca_get_baudrate_value(speed);
}

static uint8_t qca_get_baudrate_value(int speed)
{
        switch (speed) {

	...

        default:
                return QCA_BAUDRATE_115200;
        }
}

If qca_get_baudrate_value() is called with 'speed == 0' it returns
QCA_BAUDRATE_115200, which is the same value with which
QCA_BAUDRATE_115200 is initialized.

It seems the initialization and the check for 'speed == 0' could be removed.

Re: [PATCH v8 3/7] Bluetooth: btqca: Redefine qca_uart_setup() to generic function.

[Matthias Kaehlcke]

On Tue, Jun 26, 2018 at 06:53:47AM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-06-26 04:50, Matthias Kaehlcke wrote:
> > On Mon, Jun 25, 2018 at 07:10:09PM +0530, Balakrishna Godavarthi wrote:
> > > Redefinition of qca_uart_setup will help future Qualcomm Bluetooth
> > > SoC, to use the same function instead of duplicating the function.
> > > Added new arguments soc_type and soc_ver to the functions.
> > > 
> > > These arguments will help to decide type of firmware files
> > > to be loaded into Bluetooth chip.
> > > soc_type holds the Bluetooth chip connected to APPS processor.
> > > soc_ver holds the Bluetooth chip version.
> > > 
> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > > ---
> > > Changes in v8:
> > >     * updated soc_type with enum.
> > > 
> > > Changes in v7:
> > >     * initial patch
> > >     * redefined qca_uart_setup function to generic.
> > > ---
> > >  drivers/bluetooth/btqca.c   | 23 ++++++++++++-----------
> > >  drivers/bluetooth/btqca.h   | 13 +++++++++++--
> > >  drivers/bluetooth/hci_qca.c |  3 ++-
> > >  3 files changed, 25 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> > > index c5cf9cab438a..3b25be1be19c 100644
> > > --- a/drivers/bluetooth/btqca.c
> > > +++ b/drivers/bluetooth/btqca.c
> > > @@ -327,9 +327,9 @@ int qca_set_bdaddr_rome(struct hci_dev *hdev,
> > > const bdaddr_t *bdaddr)
> > >  }
> > >  EXPORT_SYMBOL_GPL(qca_set_bdaddr_rome);
> > > 
> > > -int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate)
> > > +int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> > > +		   enum qca_btsoc_type soc_type, u32 soc_ver)
> > >  {
> > > -	u32 rome_ver = 0;
> > >  	struct rome_config config;
> > >  	int err;
> > > 
> > > @@ -337,19 +337,20 @@ int qca_uart_setup(struct hci_dev *hdev,
> > > uint8_t baudrate)
> > > 
> > >  	config.user_baud_rate = baudrate;
> > > 
> > > -	/* Get QCA version information */
> > > -	err = qca_read_soc_version(hdev, &rome_ver);
> > > -	if (err < 0 || rome_ver == 0) {
> > > -		bt_dev_err(hdev, "QCA Failed to get version %d", err);
> > > -		return err;
> > > +	if (!soc_ver) {
> > > +		/* Get QCA version information */
> > > +		err = qca_read_soc_version(hdev, &soc_ver);
> > > +		if (err < 0 || soc_ver == 0) {
> > > +			bt_dev_err(hdev, "QCA Failed to get version (%d)", err);
> > > +			return err;
> > > +		}
> > > +		bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver);
> > >  	}
> > 
> > I thought we agreed in the discussion on "[v7,4/8] Bluetooth: btqca:
> > Redefine qca_uart_setup() to generic function" to call
> > qca_read_soc_version() in common code. Did I misinterpret that?
> > 
> [Bala]: After integrating wcn3990, calling qca_read_soc_version() in
> qca_setup()
>         is not preferable. as we will have multiple common blocks of code in
> qca_setup.
>         calling function to set an operator speed is required in the both
> the if -else blcoks

We can probably agree that there is no ideal solution, there is some
ugliness in on way or the other. IMO the conditional
qca_read_soc_version() in qca_uart_setup() based on the vale of
'soc_ver' is far worse than a small piece of redundant code.

If qca_read_soc_version() was done in qca_setup() the code could look
something like this:

static int qca_setup(struct hci_uart *hu)
{
	...
	if (qcadev->btsoc_type == QCA_WCN3990) {
		...
		qca_read_soc_version();
		ret = qca_set_speed(hu, QCA_OPER_SPEED);
                if (ret)
                        return ret;
	} else {
		ret = qca_set_speed(hu, QCA_OPER_SPEED);
                if (ret)
                        return ret;
                qca_read_soc_version();
	}

	speed = qca_get_speed(hu, QCA_OPER_SPEED);
	qca_baudrate = qca_get_baudrate_value(speed);

	 /* Setup patch / NVM configurations */
        ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, soc_ver);
	...
}

Yes, 'qca_set_speed(hu, QCA_OPER_SPEED)' and the error handling is
redundant, but it's only 3 lines of trivial code in exchange for
making qca_uart_setup() more consistent and not spreading
the qca_read_soc_version() calls over multiple files, depending on the
SoC version.

If you are super-convinced that the split is superior leave it as is,
I might already be doing too much bike-shedding, and after all it
isn't my code.

> > > diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> > > index 5c9851b11838..24d6667eecf1 100644
> > > --- a/drivers/bluetooth/btqca.h
> > > +++ b/drivers/bluetooth/btqca.h
> > > ...
> > > -static inline int qca_uart_setup(struct hci_dev *hdev, uint8_t
> > > baudrate)
> > > +static inline int qca_uart_setup(struct hci_dev *hdev, uint8_t
> > > baudrate,
> > > +				 enum qca_btsoc_type soc_type, u32 soc_ver);
> > 
> > Remove trailing semicolon.
> 
> [Bala]: i didn't get you.

Sorry, I should have left more context:

> static inline int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> 				 enum qca_btsoc_type soc_type, u32 soc_ver);
> {
> 	return -EOPNOTSUPP;
> }

This is a function definition, not just a declaration. The semicolon
would make it a declaration and make the compiler unhappy about a
function body where it doesn't expect it.

Re: [PATCH v8 4/7] Bluetooth: hci_qca: Add wrapper functions for setting UART speed

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-06-27 00:32, Matthias Kaehlcke wrote:
> On Tue, Jun 26, 2018 at 07:01:18AM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-06-26 05:13, Matthias Kaehlcke wrote:
>> > This is a nice improvement, a few remaining questions inline.
>> >
>> > On Mon, Jun 25, 2018 at 07:10:10PM +0530, Balakrishna Godavarthi wrote:
>> > > In function qca_setup, we set initial and operating speeds for
>> > > Qualcomm
>> > > Bluetooth SoC's. This block of code is common across different
>> > > Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
>> > > a wrapper function to set the speeds. So that future coming SoC's
>> > > can use these wrapper functions to set speeds.
>> > >
>> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> > > ---
>> > > Changes in v8:
>> > >     * common function to set INIT and operating speeds.
>> > >     * moved hardware flow control to qca_set_speed().
>> > >
>> > > Changes in v7:
>> > >     * initial patch
>> > >     * created wrapper functions for init and operating speeds.
>> > > ---
>> > >  drivers/bluetooth/hci_qca.c | 89
>> > > +++++++++++++++++++++++++++----------
>> > >  1 file changed, 65 insertions(+), 24 deletions(-)
>> > >
>> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > > index fe62420ef838..38b7dbe6c897 100644
>> > > --- a/drivers/bluetooth/hci_qca.c
>> > > +++ b/drivers/bluetooth/hci_qca.c
>> > > @@ -119,6 +119,11 @@ struct qca_data {
>> > >  	u64 votes_off;
>> > >  };
>> > >
>> > > +enum qca_speed_type {
>> > > +	QCA_INIT_SPEED = 1,
>> > > +	QCA_OPER_SPEED
>> > > +};
>> > > +
>> > >  struct qca_serdev {
>> > >  	struct hci_uart	 serdev_hu;
>> > >  	struct gpio_desc *bt_en;
>> > > @@ -923,6 +928,60 @@ static inline void host_set_baudrate(struct
>> > > hci_uart *hu, unsigned int speed)
>> > >  		hci_uart_set_baudrate(hu, speed);
>> > >  }
>> > >
>> > > +static unsigned int qca_get_speed(struct hci_uart *hu,
>> > > +				  enum qca_speed_type speed_type)
>> > > +{
>> > > +	unsigned int speed = 0;
>> > > +
>> > > +	if (speed_type == QCA_INIT_SPEED) {
>> > > +		if (hu->init_speed)
>> > > +			speed = hu->init_speed;
>> > > +		else if (hu->proto->init_speed)
>> > > +			speed = hu->proto->init_speed;
>> > > +	} else {
>> > > +		if (hu->oper_speed)
>> > > +			speed = hu->oper_speed;
>> > > +		else if (hu->proto->oper_speed)
>> > > +			speed = hu->proto->oper_speed;
>> > > +	}
>> > > +
>> > > +	return speed;
>> > > +}
>> > > +
>> > > +static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type
>> > > speed_type)
>> > > +{
>> > > +	unsigned int speed, qca_baudrate;
>> > > +	int ret;
>> > > +
>> > > +	if (speed_type == QCA_INIT_SPEED) {
>> > > +		speed = qca_get_speed(hu, QCA_INIT_SPEED);
>> > > +		if (speed)
>> > > +			host_set_baudrate(hu, speed);
>> > > +		else
>> > > +			bt_dev_err(hu->hdev, "Init speed should be non zero");
>> >
>> > The check for 'speed == 0' is done in multiple places. From this
>> > code I deduce that it is expected that both INIT and OPER speed are
>> > set to non-zero values. What happens if either of them is zero? Is the
>> > driver still operational?
>> >
>> [Bala]: yes in hci_uart_setup()(hci_serdev.c) function before calling
>> qca_setup().
>>         we actually set baudrate, but int  above code missed to 
>> restrict the
>> further call to qca_setup()
>>        if speed =0. so we are checking the same in the qca_setup().. 
>> i.e.
>> qca_get_speed().
> 
> Sorry, didn't quite get you here. Yes, the driver is still operational?
> 
> Breaking it down in multiple questions:
> 
> 1. Is it an error if one of the speeds isn't specified?
> 

[Bala]:  i want to break this issue for two chips.
         for rome:  No, it is not a error if one of speeds are missing.
                    but it is mandate to have at least one of the speeds.
                    to current implementation we are strictly not 
checking this case.
                    i.e. terminate qca_setup() if both speeds missing.
                    will do this change in this patch.
         for wnc3990: yes it an error if any of the speeds are missing.
                      it is mandate to have both the speeds.
                      to current implementation we are strictly not 
checking this case.
                      i.e. terminate qca_setup() if any speeds missing.
                      will integrate this in patch add support for 
wcn3990.

        "The check for 'speed == 0' is done in multiple places. From this
        code I deduce that it is expected that both INIT and OPER speed 
are
        set to non-zero values. What happens if either of them is zero? 
Is the
        driver still operational"

       still we will have speed ==0 check for BT chip ROME. pls find code 
snippet below

          thanks for catching this corner case.

> If yes we should probably check this early once and return an error
> early, instead of doing the check repeatedly
> 
> 2. If it is not an error, what is the driver supposed to do?
> 
>> > In the discussion on "[v7,8/8] Bluetooth: hci_qca: Add support for
>> > Qualcomm Bluetooth chip wcn3990" you mentioned the possbility to move
>> > the hci_uart_set_flow_control() calls into _set_speed(). This seemed
>> > interesting but finally it isn't done in this series. Did you
>> > encounter that it is not feasible/desirable for some reason?
>> >
>> 
>> [Bala]: this patch is for rome where flow control is not used.
>>         after we integrate wcn3990, flow control is hidden in the
>> qca_set_speed()
>>         Pls check [v8 7/7] patch.
> 
> Sorry, my confusion
> 
>> > >  static int qca_setup(struct hci_uart *hu)
>> > >  {
>> > >  	struct hci_dev *hdev = hu->hdev;
>> > > @@ -937,35 +996,17 @@ static int qca_setup(struct hci_uart *hu)
>> > >  	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> > >
>> > >  	/* Setup initial baudrate */
>> > > -	speed = 0;
>> > > -	if (hu->init_speed)
>> > > -		speed = hu->init_speed;
>> > > -	else if (hu->proto->init_speed)
>> > > -		speed = hu->proto->init_speed;
>> > > -
>> > > -	if (speed)
>> > > -		host_set_baudrate(hu, speed);
>> > > +	qca_set_speed(hu, QCA_INIT_SPEED);
>> > >
>> > >  	/* Setup user speed if needed */
>> > > -	speed = 0;
>> > > -	if (hu->oper_speed)
>> > > -		speed = hu->oper_speed;
>> > > -	else if (hu->proto->oper_speed)
>> > > -		speed = hu->proto->oper_speed;
>> > > +	ret = qca_set_speed(hu, QCA_OPER_SPEED);
>> > > +	if (ret)
>> > > +		return ret;
>> > >
>> > > -	if (speed) {
>> > > +	speed = qca_get_speed(hu, QCA_OPER_SPEED);
>> > > +	if (speed)
>> > >  		qca_baudrate = qca_get_baudrate_value(speed);
>> >
>> > Is the check here necessary? qca_get_baudrate_value() returns
>> > QCA_BAUDRATE_115200 for a zero speed.
>> 
>> this if for no zero operating speed,
> 
> My point is:
> 
> static int qca_setup(struct hci_uart *hu)
> {
> 	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
> 
> 	...
> 
> 	if (speed)
> 		qca_baudrate = qca_get_baudrate_value(speed);
> }
> 
> static uint8_t qca_get_baudrate_value(int speed)
> {
>         switch (speed) {
> 
> 	...
> 
>         default:
>                 return QCA_BAUDRATE_115200;
>         }
> }
> 
> If qca_get_baudrate_value() is called with 'speed == 0' it returns
> QCA_BAUDRATE_115200, which is the same value with which
> QCA_BAUDRATE_115200 is initialized.
> 
> It seems the initialization and the check for 'speed == 0' could be 
> removed.

[Bala]: the above speed == 0 is highly recommended, let us assume ROME 
chip init speed moved from 115200 to 2000000 and operating speed is 
defined as zero.
        then this check will help us. else if we remove speed ==0 then in 
qca_baudrate we will end up having a different value.
         Pls find the code snippet for better understanding.

after your suggestion. code snippet along with wcn3990 integration.


[Bala]: for rome any one of the speeds are required.
         for wcn3990 requires both speeds.

static int qca_check_speeds(struct hci_uart *hu)
{
         struct qca_serdev *qcadev;

         qcadev = serdev_device_get_drvdata(hu->serdev);
         if (qcadev->btsoc_type == QCA_WCN3990) {
                 /* QCA WCN3990 requires both the speed values. */
                 if (qca_get_speed(hu, QCA_INIT_SPEED) &&
                     qca_get_speed(hu, QCA_OPER_SPEED))

                         return 0;

                 bt_dev_err(hu->hdev, "Both the speeds should be non 
zero");
                 return 1;
         }

         if (qca_get_speed(hu, QCA_INIT_SPEED) ||
              qca_get_speed(hu, QCA_OPER_SPEED))
                 return 0;

         bt_dev_err(hu->hdev, "either of the speeds should be non zero");
         return 1;
         [Bala]: pls suggest, what could be error no if above criteria is 
not met.
}

static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type 
speed_type)
{
         struct qca_serdev *qcadev;
         unsigned int speed, qca_baudrate;
         int ret;

         if (speed_type == QCA_INIT_SPEED) {
                 speed = qca_get_speed(hu, QCA_INIT_SPEED);
                 if (speed)
                         host_set_baudrate(hu, speed);
                 [Bala]: this speed check is required for ROME, that if 
ROME init speed is zero.
                 return 0;
         }

         speed = qca_get_speed(hu, QCA_OPER_SPEED);
         if (!speed)
                 return 0;
         [Bala]: this speed check is required for ROME, that if ROME 
operating speed is zero.

[Bala]: this is speeds in both OPER and init speeds are required.. as we 
move forward if any one of the speed is set.

         qcadev = serdev_device_get_drvdata(hu->serdev);
         /* Disabling hardware flow control is preferred while
          * sending change baud rate command to SoC.
if (qcadev->btsoc_type == QCA_WCN3990)
                 hci_uart_set_flow_control(hu, true);

         qca_baudrate = qca_get_baudrate_value(speed);
         bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
         ret = qca_set_baudrate(hu->hdev, qca_baudrate);
         if (ret) {
                 bt_dev_err(hu->hdev, "Failed to change the baudrate 
(%d)", ret);
                 return ret;
         }

         host_set_baudrate(hu, speed);
         if (qcadev->btsoc_type == QCA_WCN3990)
                 hci_uart_set_flow_control(hu, false);

         return 0;
}

static int qca_setup(struct hci_uart *hu)
{
         struct hci_dev *hdev = hu->hdev;
         struct qca_data *qca = hu->priv;
         unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
         struct qca_serdev *qcadev;
         int ret;
         int soc_ver = 0;

         qcadev = serdev_device_get_drvdata(hu->serdev);

         /* Patch downloading has to be done without IBS mode */
         clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);

         ret = qca_check_speeds(hu);
         if (ret)
                 return ret;

         /* Setup initial baudrate */
         qca_set_speed(hu, QCA_INIT_SPEED);
         speed = qca_get_speed(hu, QCA_INIT_SPEED);
         if (speed)
                 qca_baudrate = qca_get_baudrate_value(speed);

[Bala]: i have newly add above statement. initial speed of both ROME and 
wcn3990 when the chip boot up is 115200.
         in future, if we have new chip where initial  speed of chip 
during boot up is 200000. then this will help us to get the actual 
initial speed i.e. based upon the init speed.

         if (qcadev->btsoc_type == QCA_WCN3990) {
                                                                
bt_dev_dbg(hdev, "setting up wcn3990");
                 hci_uart_set_flow_control(hu, true);
                 ret = qca_send_vendor_cmd(hdev, 
QCA_WCN3990_POWERON_PULSE);
                 if (ret)
                         return ret;

                 hci_uart_set_flow_control(hu, false);
                 serdev_device_close(hu->serdev);
                 ret = serdev_device_open(hu->serdev);
                 if (ret) {
                         bt_dev_err(hdev, "failed to open port");
                         return ret;
                 }

                 msleep(100);
                 /* Setup initial baudrate */
                 qca_set_speed(hu, QCA_INIT_SPEED);
                 hci_uart_set_flow_control(hu, false);
                 ret = qca_read_soc_version(hdev, &soc_ver);
                 if (ret < 0 || soc_ver == 0) {
                         bt_dev_err(hdev, "Failed to get version %d", 
ret);
                         return ret;
                 }
                 bt_dev_info(hdev, "wcn3990 controller version 0x%08x", 
soc_ver);
         } else {
                 bt_dev_info(hdev, "ROME setup");
         }

         /* Setup user speed if needed */
         ret = qca_set_speed(hu, QCA_OPER_SPEED);
         if (ret)
                 return ret;

         speed = qca_get_speed(hu, QCA_OPER_SPEED);
         if (speed)
                 qca_baudrate = qca_get_baudrate_value(speed);

       [Bala]: the above speed == 0 is highly recommended, let us assume 
ROME chip init speed moved from 115200 to 2000000 and operating speed is 
defined as zero.
        then this check will help us. else if we remove speed ==0 then in 
qca_baudrate we will end up having a different value.
        ….
         }

  Pls let me know, whether i have clarified your queries.
-- 
Regards
Balakrishna.

Re: [PATCH v8 3/7] Bluetooth: btqca: Redefine qca_uart_setup() to generic function.

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-06-27 01:23, Matthias Kaehlcke wrote:
> On Tue, Jun 26, 2018 at 06:53:47AM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-06-26 04:50, Matthias Kaehlcke wrote:
>> > On Mon, Jun 25, 2018 at 07:10:09PM +0530, Balakrishna Godavarthi wrote:
>> > > Redefinition of qca_uart_setup will help future Qualcomm Bluetooth
>> > > SoC, to use the same function instead of duplicating the function.
>> > > Added new arguments soc_type and soc_ver to the functions.
>> > >
>> > > These arguments will help to decide type of firmware files
>> > > to be loaded into Bluetooth chip.
>> > > soc_type holds the Bluetooth chip connected to APPS processor.
>> > > soc_ver holds the Bluetooth chip version.
>> > >
>> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> > > ---
>> > > Changes in v8:
>> > >     * updated soc_type with enum.
>> > >
>> > > Changes in v7:
>> > >     * initial patch
>> > >     * redefined qca_uart_setup function to generic.
>> > > ---
>> > >  drivers/bluetooth/btqca.c   | 23 ++++++++++++-----------
>> > >  drivers/bluetooth/btqca.h   | 13 +++++++++++--
>> > >  drivers/bluetooth/hci_qca.c |  3 ++-
>> > >  3 files changed, 25 insertions(+), 14 deletions(-)
>> > >
>> > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>> > > index c5cf9cab438a..3b25be1be19c 100644
>> > > --- a/drivers/bluetooth/btqca.c
>> > > +++ b/drivers/bluetooth/btqca.c
>> > > @@ -327,9 +327,9 @@ int qca_set_bdaddr_rome(struct hci_dev *hdev,
>> > > const bdaddr_t *bdaddr)
>> > >  }
>> > >  EXPORT_SYMBOL_GPL(qca_set_bdaddr_rome);
>> > >
>> > > -int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate)
>> > > +int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>> > > +		   enum qca_btsoc_type soc_type, u32 soc_ver)
>> > >  {
>> > > -	u32 rome_ver = 0;
>> > >  	struct rome_config config;
>> > >  	int err;
>> > >
>> > > @@ -337,19 +337,20 @@ int qca_uart_setup(struct hci_dev *hdev,
>> > > uint8_t baudrate)
>> > >
>> > >  	config.user_baud_rate = baudrate;
>> > >
>> > > -	/* Get QCA version information */
>> > > -	err = qca_read_soc_version(hdev, &rome_ver);
>> > > -	if (err < 0 || rome_ver == 0) {
>> > > -		bt_dev_err(hdev, "QCA Failed to get version %d", err);
>> > > -		return err;
>> > > +	if (!soc_ver) {
>> > > +		/* Get QCA version information */
>> > > +		err = qca_read_soc_version(hdev, &soc_ver);
>> > > +		if (err < 0 || soc_ver == 0) {
>> > > +			bt_dev_err(hdev, "QCA Failed to get version (%d)", err);
>> > > +			return err;
>> > > +		}
>> > > +		bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver);
>> > >  	}
>> >
>> > I thought we agreed in the discussion on "[v7,4/8] Bluetooth: btqca:
>> > Redefine qca_uart_setup() to generic function" to call
>> > qca_read_soc_version() in common code. Did I misinterpret that?
>> >
>> [Bala]: After integrating wcn3990, calling qca_read_soc_version() in
>> qca_setup()
>>         is not preferable. as we will have multiple common blocks of 
>> code in
>> qca_setup.
>>         calling function to set an operator speed is required in the 
>> both
>> the if -else blcoks
> 
> We can probably agree that there is no ideal solution, there is some
> ugliness in on way or the other. IMO the conditional
> qca_read_soc_version() in qca_uart_setup() based on the vale of
> 'soc_ver' is far worse than a small piece of redundant code.
> 
> If qca_read_soc_version() was done in qca_setup() the code could look
> something like this:
> 
> static int qca_setup(struct hci_uart *hu)
> {
> 	...
> 	if (qcadev->btsoc_type == QCA_WCN3990) {
> 		...
> 		qca_read_soc_version();
> 		ret = qca_set_speed(hu, QCA_OPER_SPEED);
>                 if (ret)
>                         return ret;
> 	} else {
> 		ret = qca_set_speed(hu, QCA_OPER_SPEED);
>                 if (ret)
>                         return ret;
>                 qca_read_soc_version();
> 	}
> 
> 	speed = qca_get_speed(hu, QCA_OPER_SPEED);
> 	qca_baudrate = qca_get_baudrate_value(speed);
> 
> 	 /* Setup patch / NVM configurations */
>         ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, 
> soc_ver);
> 	...
> }
> 
> Yes, 'qca_set_speed(hu, QCA_OPER_SPEED)' and the error handling is
> redundant, but it's only 3 lines of trivial code in exchange for
> making qca_uart_setup() more consistent and not spreading
> the qca_read_soc_version() calls over multiple files, depending on the
> SoC version.
> 
> If you are super-convinced that the split is superior leave it as is,
> I might already be doing too much bike-shedding, and after all it
> isn't my code.
> 

[Bala]: not a problem. will update as suggested.

>> > > diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>> > > index 5c9851b11838..24d6667eecf1 100644
>> > > --- a/drivers/bluetooth/btqca.h
>> > > +++ b/drivers/bluetooth/btqca.h
>> > > ...
>> > > -static inline int qca_uart_setup(struct hci_dev *hdev, uint8_t
>> > > baudrate)
>> > > +static inline int qca_uart_setup(struct hci_dev *hdev, uint8_t
>> > > baudrate,
>> > > +				 enum qca_btsoc_type soc_type, u32 soc_ver);
>> >
>> > Remove trailing semicolon.
>> 
>> [Bala]: i didn't get you.
> 
> Sorry, I should have left more context:
> 
>> static inline int qca_uart_setup(struct hci_dev *hdev, uint8_t 
>> baudrate,
>> 				 enum qca_btsoc_type soc_type, u32 soc_ver);
>> {
>> 	return -EOPNOTSUPP;
>> }
> 
> This is a function definition, not just a declaration. The semicolon
> would make it a declaration and make the compiler unhappy about a
> function body where it doesn't expect it.

[Bala]: i will update.

-- 
Regards
Balakrishna.

Re: [PATCH v8 7/7] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-06-26 06:35, Matthias Kaehlcke wrote:
> On Mon, Jun 25, 2018 at 07:10:13PM +0530, Balakrishna Godavarthi wrote:
>> Add support to set voltage/current of various regulators
>> to power up/down Bluetooth chip wcn3990.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> changes in v8:
>>     * closing qca buffer, if qca_power_setup fails
>>     * chnaged ibs start timer function call location.
>>     * updated review comments.
>> 
>> changes in v7:
>>     * addressed review comments.
>> 
>> changes in v6:
>>     * Hooked up qca_power to qca_serdev.
>>     * renamed all the naming inconsistency functions with qca_*
>>     * leveraged common code of ROME for wcn3990.
>>     * created wrapper functions for re-usable blocks.
>>     * updated function of _*regulator_enable and _*regualtor_disable.
>>     * removed redundant comments and functions.
>>     * addressed review comments.
>> 
>> Changes in v5:
>>     * updated regulator vddpa min_uV to 1304000.
>>       * addressed review comments.
>> 
>> Changes in v4:
>>     * Segregated the changes of btqca from hci_qca
>>     * rebased all changes on top of bluetooth-next.
>>     * addressed review comments.
>> 
>> ---
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 28187a89b850..bd4c9a78716f 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> ...
>> +static int qca_send_vendor_cmd(struct hci_dev *hdev, u8 cmd)
>> +{
>> +	struct hci_uart *hu = hci_get_drvdata(hdev);
>> +	struct qca_data *qca = hu->priv;
>> +	struct sk_buff *skb;
>> +
>> +	bt_dev_dbg(hdev, "sending command %02x to SoC", cmd);
>> +
>> +	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
>> +	if (!skb) {
>> +		bt_dev_err(hdev, "Failed to allocate memory for vendor packet");
> 
> As mentioned on v7, custom OOM messages should be avoided.
> 

[Bala]: sry i might have missed it, will update.

>>  static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type 
>> speed_type)
>>  {
>> +	struct qca_serdev *qcadev;
>>  	unsigned int speed, qca_baudrate;
>>  	int ret;
>> 
>> @@ -971,6 +1054,13 @@ static int qca_set_speed(struct hci_uart *hu, 
>> enum qca_speed_type speed_type)
>>  		return 0;
>>  	}
>> 
>> +	qcadev = serdev_device_get_drvdata(hu->serdev);
>> +	/* Disabling hardware flow control is preferred while
>> +	 * sending change baud rate command to SoC.
>> +	 */
> 
> Is it only preferred or must be?
> 

[Bala]: must be. will update.

>> +	if (qcadev->btsoc_type == QCA_WCN3990)
>> +		hci_uart_set_flow_control(hu, true);
>> +
> 
> nit: consider doing this just before qca_set_baudrate(). It doesn't
> make a difference but leaves it clearer what exactly needs to be
> 'protected' (analogy to locking).

[Bala] : will do it.

> 
>>  	qca_baudrate = qca_get_baudrate_value(speed);
>>  	bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
>>  	ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>> @@ -980,8 +1070,10 @@ static int qca_set_speed(struct hci_uart *hu, 
>> enum qca_speed_type speed_type)
>>  	}
>> 
>>  	host_set_baudrate(hu, speed);
>> +	if (qcadev->btsoc_type == QCA_WCN3990)
>> +		hci_uart_set_flow_control(hu, false);
> 
>>  static int qca_setup(struct hci_uart *hu)
>> @@ -989,10 +1081,11 @@ static int qca_setup(struct hci_uart *hu)
>>  	struct hci_dev *hdev = hu->hdev;
>>  	struct qca_data *qca = hu->priv;
>>  	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
>> +	struct qca_serdev *qcadev;
>>  	int ret;
>>  	int soc_ver = 0;
>> 
>> -	bt_dev_info(hdev, "ROME setup");
>> +	qcadev = serdev_device_get_drvdata(hu->serdev);
>> 
>>  	/* Patch downloading has to be done without IBS mode */
>>  	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> @@ -1000,6 +1093,35 @@ static int qca_setup(struct hci_uart *hu)
>>  	/* Setup initial baudrate */
>>  	qca_set_speed(hu, QCA_INIT_SPEED);
>> 
>> +	if (qcadev->btsoc_type == QCA_WCN3990) {
>> +		bt_dev_dbg(hdev, "setting up wcn3990");
>> +		hci_uart_set_flow_control(hu, true);
>> +		ret = qca_send_vendor_cmd(hdev, QCA_WCN3990_POWERON_PULSE);
>> +		if (ret)
>> +			return ret;
>> +
>> +		hci_uart_set_flow_control(hu, false);
>> +		serdev_device_close(hu->serdev);
>> +		ret = serdev_device_open(hu->serdev);
>> +		if (ret) {
>> +			bt_dev_err(hdev, "failed to open port");
>> +			return ret;
>> +		}
>> +
>> +		msleep(100);
> 
> Is the sleep really related with _open() or is it rather that the
> device needs to settle after the power on pulse? In the latter case
> I'd suggest to do the sleep before _open(), if it doesn't make a
> functional difference (makes the code a bit more self documenting).
> 
>> +		/* Setup initial baudrate */
>> +		qca_set_speed(hu, QCA_INIT_SPEED);
>> +		hci_uart_set_flow_control(hu, false);
> 
> This is still a bit noisy with all the open/close and flow control
> stuff. If I understand correctly this essentially switches the
> controller on (or resets it?) and brings it (and the driver) into a
> sane state. Would it make sense to move the above block into a
> wcn3990_init/reset() or so?

[Bala]: It is very good idea, may be future chips also will flow same 
functions with some initial setup changes.
         will group these functions into a common functions.


-- 
Regards
Balakrishna.

Re: [PATCH v8 4/7] Bluetooth: hci_qca: Add wrapper functions for setting UART speed

[Matthias Kaehlcke]

On Fri, Jun 29, 2018 at 08:59:38PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-06-27 00:32, Matthias Kaehlcke wrote:
> > On Tue, Jun 26, 2018 at 07:01:18AM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > > 
> > > On 2018-06-26 05:13, Matthias Kaehlcke wrote:
> > > > This is a nice improvement, a few remaining questions inline.
> > > >
> > > > On Mon, Jun 25, 2018 at 07:10:10PM +0530, Balakrishna Godavarthi wrote:
> > > > > In function qca_setup, we set initial and operating speeds for
> > > > > Qualcomm
> > > > > Bluetooth SoC's. This block of code is common across different
> > > > > Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
> > > > > a wrapper function to set the speeds. So that future coming SoC's
> > > > > can use these wrapper functions to set speeds.
> > > > >
> > > > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > > > > ---
> > > > > Changes in v8:
> > > > >     * common function to set INIT and operating speeds.
> > > > >     * moved hardware flow control to qca_set_speed().
> > > > >
> > > > > Changes in v7:
> > > > >     * initial patch
> > > > >     * created wrapper functions for init and operating speeds.
> > > > > ---
> > > > >  drivers/bluetooth/hci_qca.c | 89
> > > > > +++++++++++++++++++++++++++----------
> > > > >  1 file changed, 65 insertions(+), 24 deletions(-)
> > > > >
> > > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > > > index fe62420ef838..38b7dbe6c897 100644
> > > > > --- a/drivers/bluetooth/hci_qca.c
> > > > > +++ b/drivers/bluetooth/hci_qca.c
> > > > > @@ -119,6 +119,11 @@ struct qca_data {
> > > > >  	u64 votes_off;
> > > > >  };
> > > > >
> > > > > +enum qca_speed_type {
> > > > > +	QCA_INIT_SPEED = 1,
> > > > > +	QCA_OPER_SPEED
> > > > > +};
> > > > > +
> > > > >  struct qca_serdev {
> > > > >  	struct hci_uart	 serdev_hu;
> > > > >  	struct gpio_desc *bt_en;
> > > > > @@ -923,6 +928,60 @@ static inline void host_set_baudrate(struct
> > > > > hci_uart *hu, unsigned int speed)
> > > > >  		hci_uart_set_baudrate(hu, speed);
> > > > >  }
> > > > >
> > > > > +static unsigned int qca_get_speed(struct hci_uart *hu,
> > > > > +				  enum qca_speed_type speed_type)
> > > > > +{
> > > > > +	unsigned int speed = 0;
> > > > > +
> > > > > +	if (speed_type == QCA_INIT_SPEED) {
> > > > > +		if (hu->init_speed)
> > > > > +			speed = hu->init_speed;
> > > > > +		else if (hu->proto->init_speed)
> > > > > +			speed = hu->proto->init_speed;
> > > > > +	} else {
> > > > > +		if (hu->oper_speed)
> > > > > +			speed = hu->oper_speed;
> > > > > +		else if (hu->proto->oper_speed)
> > > > > +			speed = hu->proto->oper_speed;
> > > > > +	}
> > > > > +
> > > > > +	return speed;
> > > > > +}
> > > > > +
> > > > > +static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type
> > > > > speed_type)
> > > > > +{
> > > > > +	unsigned int speed, qca_baudrate;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (speed_type == QCA_INIT_SPEED) {
> > > > > +		speed = qca_get_speed(hu, QCA_INIT_SPEED);
> > > > > +		if (speed)
> > > > > +			host_set_baudrate(hu, speed);
> > > > > +		else
> > > > > +			bt_dev_err(hu->hdev, "Init speed should be non zero");
> > > >
> > > > The check for 'speed == 0' is done in multiple places. From this
> > > > code I deduce that it is expected that both INIT and OPER speed are
> > > > set to non-zero values. What happens if either of them is zero? Is the
> > > > driver still operational?
> > > >
> > > [Bala]: yes in hci_uart_setup()(hci_serdev.c) function before calling
> > > qca_setup().
> > >         we actually set baudrate, but int  above code missed to
> > > restrict the
> > > further call to qca_setup()
> > >        if speed =0. so we are checking the same in the qca_setup()..
> > > i.e.
> > > qca_get_speed().
> > 
> > Sorry, didn't quite get you here. Yes, the driver is still operational?
> > 
> > Breaking it down in multiple questions:
> > 
> > 1. Is it an error if one of the speeds isn't specified?
> > 
> 
> [Bala]:  i want to break this issue for two chips.
>         for rome:  No, it is not a error if one of speeds are missing.
>                    but it is mandate to have at least one of the speeds.
>                    to current implementation we are strictly not checking
> this case.
>                    i.e. terminate qca_setup() if both speeds missing.
>                    will do this change in this patch.
>         for wnc3990: yes it an error if any of the speeds are missing.
>                      it is mandate to have both the speeds.
>                      to current implementation we are strictly not checking
> this case.
>                      i.e. terminate qca_setup() if any speeds missing.
>                      will integrate this in patch add support for wcn3990.
> 
>        "The check for 'speed == 0' is done in multiple places. From this
>        code I deduce that it is expected that both INIT and OPER speed are
>        set to non-zero values. What happens if either of them is zero? Is
> the
>        driver still operational"
> 
>       still we will have speed ==0 check for BT chip ROME. pls find code
> snippet below
> 
>          thanks for catching this corner case.

Thanks for the clarification

> > If yes we should probably check this early once and return an error
> > early, instead of doing the check repeatedly
> > 
> > 2. If it is not an error, what is the driver supposed to do?
> > 
> > > > In the discussion on "[v7,8/8] Bluetooth: hci_qca: Add support for
> > > > Qualcomm Bluetooth chip wcn3990" you mentioned the possbility to move
> > > > the hci_uart_set_flow_control() calls into _set_speed(). This seemed
> > > > interesting but finally it isn't done in this series. Did you
> > > > encounter that it is not feasible/desirable for some reason?
> > > >
> > > 
> > > [Bala]: this patch is for rome where flow control is not used.
> > >         after we integrate wcn3990, flow control is hidden in the
> > > qca_set_speed()
> > >         Pls check [v8 7/7] patch.
> > 
> > Sorry, my confusion
> > 
> > > > >  static int qca_setup(struct hci_uart *hu)
> > > > >  {
> > > > >  	struct hci_dev *hdev = hu->hdev;
> > > > > @@ -937,35 +996,17 @@ static int qca_setup(struct hci_uart *hu)
> > > > >  	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> > > > >
> > > > >  	/* Setup initial baudrate */
> > > > > -	speed = 0;
> > > > > -	if (hu->init_speed)
> > > > > -		speed = hu->init_speed;
> > > > > -	else if (hu->proto->init_speed)
> > > > > -		speed = hu->proto->init_speed;
> > > > > -
> > > > > -	if (speed)
> > > > > -		host_set_baudrate(hu, speed);
> > > > > +	qca_set_speed(hu, QCA_INIT_SPEED);
> > > > >
> > > > >  	/* Setup user speed if needed */
> > > > > -	speed = 0;
> > > > > -	if (hu->oper_speed)
> > > > > -		speed = hu->oper_speed;
> > > > > -	else if (hu->proto->oper_speed)
> > > > > -		speed = hu->proto->oper_speed;
> > > > > +	ret = qca_set_speed(hu, QCA_OPER_SPEED);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > >
> > > > > -	if (speed) {
> > > > > +	speed = qca_get_speed(hu, QCA_OPER_SPEED);
> > > > > +	if (speed)
> > > > >  		qca_baudrate = qca_get_baudrate_value(speed);
> > > >
> > > > Is the check here necessary? qca_get_baudrate_value() returns
> > > > QCA_BAUDRATE_115200 for a zero speed.
> > > 
> > > this if for no zero operating speed,
> > 
> > My point is:
> > 
> > static int qca_setup(struct hci_uart *hu)
> > {
> > 	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
> > 
> > 	...
> > 
> > 	if (speed)
> > 		qca_baudrate = qca_get_baudrate_value(speed);
> > }
> > 
> > static uint8_t qca_get_baudrate_value(int speed)
> > {
> >         switch (speed) {
> > 
> > 	...
> > 
> >         default:
> >                 return QCA_BAUDRATE_115200;
> >         }
> > }
> > 
> > If qca_get_baudrate_value() is called with 'speed == 0' it returns
> > QCA_BAUDRATE_115200, which is the same value with which
> > QCA_BAUDRATE_115200 is initialized.
> > 
> > It seems the initialization and the check for 'speed == 0' could be
> > removed.
> 
> [Bala]: the above speed == 0 is highly recommended, let us assume ROME chip
> init speed moved from 115200 to 2000000 and operating speed is defined as
> zero.
>        then this check will help us. else if we remove speed ==0 then in
> qca_baudrate we will end up having a different value.
>         Pls find the code snippet for better understanding.

Hm, still not clear to me, I'm no Bluetooth expert, maybe I'm missing
something that seems obvious to others.

What exactly do you mean with "init speed moved from 115200 to
2000000"?

Is it that the first qca_set_speed(hu, QCA_INIT_SPEED) call in
qca_setup() uses hu->proto->init_speed and the second one
hu->init_speed after it was initialized in qca_open()?

I saw you added a new assignment of qca_baudrate below, which probably
is related, though you don't mention it here:

        /* Setup initial baudrate */
        qca_set_speed(hu, QCA_INIT_SPEED);
        speed = qca_get_speed(hu, QCA_INIT_SPEED);
        if (speed)
                qca_baudrate = qca_get_baudrate_value(speed);


I find it really hard to discuss in code snippets, it's probably best
to send a new version of the patch and discuss it with the full
context.

Regarding the new qca_get_speed() and assignment of qca_baudrate: if
this needs to be done it should probably be moved just before this block
to keep things together:

        speed = qca_get_speed(hu, QCA_OPER_SPEED);
        if (speed)
                qca_baudrate = qca_get_baudrate_value(speed);

If I didn't lose myself jumping back and forth between the snippets
and v8 qca_baudrate isn't used before. And if that is correct then it
probably shouldn't be moved before the block, but rewritten to:

        speed = qca_get_speed(hu, QCA_OPER_SPEED);
        if (!speed)
	        // Note mka@: no need to check 'speed', we know at
		// least one of them is set
	        speed = qca_get_speed(hu, QCA_INIT_SPEED);

        qca_baudrate = qca_get_baudrate_value(speed);

Not sure if that's correct, but it seems reasonable in the sense that
you said that at least one of the speeds needs to be set and it
certainly looks cleaner than the spread out _get_speed() and
qca_get_baudrate_value(). But maybe I just got it wrong and reality is
more ugly, best sent a new patch and have the full context.

> after your suggestion. code snippet along with wcn3990 integration.
> 
> 
> [Bala]: for rome any one of the speeds are required.
>         for wcn3990 requires both speeds.
> 
> static int qca_check_speeds(struct hci_uart *hu)
> {
>         struct qca_serdev *qcadev;
> 
>         qcadev = serdev_device_get_drvdata(hu->serdev);
>         if (qcadev->btsoc_type == QCA_WCN3990) {
>                 /* QCA WCN3990 requires both the speed values. */
>                 if (qca_get_speed(hu, QCA_INIT_SPEED) &&
>                     qca_get_speed(hu, QCA_OPER_SPEED))
> 
>                         return 0;
> 
>                 bt_dev_err(hu->hdev, "Both the speeds should be non zero");
>                 return 1;
>         }

It would probably be clearer to invert the logic, and check for the
error condition and return 0 in the main branch (or even better, at
the end of the function).


>         if (qca_get_speed(hu, QCA_INIT_SPEED) ||
>              qca_get_speed(hu, QCA_OPER_SPEED))
>                 return 0;
> 
>         bt_dev_err(hu->hdev, "either of the speeds should be non zero");
>         return 1;

ditto

>         [Bala]: pls suggest, what could be error no if above criteria is not
> met.

I think you could use -EINVAL as it would be a configuration error.

> static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type
> speed_type)
> {
>         struct qca_serdev *qcadev;
>         unsigned int speed, qca_baudrate;
>         int ret;
> 
>         if (speed_type == QCA_INIT_SPEED) {
>                 speed = qca_get_speed(hu, QCA_INIT_SPEED);
>                 if (speed)
>                         host_set_baudrate(hu, speed);
>                 [Bala]: this speed check is required for ROME, that if ROME
> init speed is zero.
>                 return 0;
>         }

Just noticed, I think this would be clearer with an else branch. The
current structure might be based on a comment from me on an earlier
version of the _set/get_speed() rework, where I suggested to return to
save a level of indentation. Here we set either INIT or OPER speed,
not having the else branch can give the impression that both might be
set.

>         speed = qca_get_speed(hu, QCA_OPER_SPEED);
>         if (!speed)
>                 return 0;
>         [Bala]: this speed check is required for ROME, that if ROME
> operating speed is zero.
> 
> [Bala]: this is speeds in both OPER and init speeds are required.. as we
> move forward if any one of the speed is set.
> 
>         qcadev = serdev_device_get_drvdata(hu->serdev);
>         /* Disabling hardware flow control is preferred while
>          * sending change baud rate command to SoC.
> if (qcadev->btsoc_type == QCA_WCN3990)
>                 hci_uart_set_flow_control(hu, true);
> 
>         qca_baudrate = qca_get_baudrate_value(speed);
>         bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
>         ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>         if (ret) {
>                 bt_dev_err(hu->hdev, "Failed to change the baudrate (%d)",
> ret);
>                 return ret;
>         }
> 
>         host_set_baudrate(hu, speed);
>         if (qcadev->btsoc_type == QCA_WCN3990)
>                 hci_uart_set_flow_control(hu, false);
> 
>         return 0;
> }
> 
> static int qca_setup(struct hci_uart *hu)
> {
>         struct hci_dev *hdev = hu->hdev;
>         struct qca_data *qca = hu->priv;
>         unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
>         struct qca_serdev *qcadev;
>         int ret;
>         int soc_ver = 0;
> 
>         qcadev = serdev_device_get_drvdata(hu->serdev);
> 
>         /* Patch downloading has to be done without IBS mode */
>         clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> 
>         ret = qca_check_speeds(hu);
>         if (ret)
>                 return ret;
> 
>         /* Setup initial baudrate */
>         qca_set_speed(hu, QCA_INIT_SPEED);
>         speed = qca_get_speed(hu, QCA_INIT_SPEED);
>         if (speed)
>                 qca_baudrate = qca_get_baudrate_value(speed);
> 
> [Bala]: i have newly add above statement. initial speed of both ROME and
> wcn3990 when the chip boot up is 115200.
>         in future, if we have new chip where initial  speed of chip during
> boot up is 200000. then this will help us to get the actual initial speed
> i.e. based upon the init speed.

As mentioned above, this should probably move further down.

>         if (qcadev->btsoc_type == QCA_WCN3990) {
> bt_dev_dbg(hdev, "setting up wcn3990");
>                 hci_uart_set_flow_control(hu, true);
>                 ret = qca_send_vendor_cmd(hdev, QCA_WCN3990_POWERON_PULSE);
>                 if (ret)
>                         return ret;
> 
>                 hci_uart_set_flow_control(hu, false);
>                 serdev_device_close(hu->serdev);
>                 ret = serdev_device_open(hu->serdev);
>                 if (ret) {
>                         bt_dev_err(hdev, "failed to open port");
>                         return ret;
>                 }
> 
>                 msleep(100);
>                 /* Setup initial baudrate */
>                 qca_set_speed(hu, QCA_INIT_SPEED);
>                 hci_uart_set_flow_control(hu, false);
>                 ret = qca_read_soc_version(hdev, &soc_ver);
>                 if (ret < 0 || soc_ver == 0) {
>                         bt_dev_err(hdev, "Failed to get version %d", ret);
>                         return ret;
>                 }
>                 bt_dev_info(hdev, "wcn3990 controller version 0x%08x",
> soc_ver);
>         } else {
>                 bt_dev_info(hdev, "ROME setup");
>         }
> 
>         /* Setup user speed if needed */
>         ret = qca_set_speed(hu, QCA_OPER_SPEED);
>         if (ret)
>                 return ret;
> 
>         speed = qca_get_speed(hu, QCA_OPER_SPEED);
>         if (speed)
>                 qca_baudrate = qca_get_baudrate_value(speed);
> 
>       [Bala]: the above speed == 0 is highly recommended, let us assume ROME
> chip init speed moved from 115200 to 2000000 and operating speed is defined
> as zero.
>        then this check will help us. else if we remove speed ==0 then in
> qca_baudrate we will end up having a different value.
>        ….
>         }
> 
>  Pls let me know, whether i have clarified your queries.

Partially, let's continue the discussion on v9 unless you have
questions/comments on my comments before you post.

Thanks

Matthias

Re: [PATCH v8 4/7] Bluetooth: hci_qca: Add wrapper functions for setting UART speed

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-06-30 02:31, Matthias Kaehlcke wrote:
> On Fri, Jun 29, 2018 at 08:59:38PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-06-27 00:32, Matthias Kaehlcke wrote:
>> > On Tue, Jun 26, 2018 at 07:01:18AM +0530, Balakrishna Godavarthi wrote:
>> > > Hi Matthias,
>> > >
>> > > On 2018-06-26 05:13, Matthias Kaehlcke wrote:
>> > > > This is a nice improvement, a few remaining questions inline.
>> > > >
>> > > > On Mon, Jun 25, 2018 at 07:10:10PM +0530, Balakrishna Godavarthi wrote:
>> > > > > In function qca_setup, we set initial and operating speeds for
>> > > > > Qualcomm
>> > > > > Bluetooth SoC's. This block of code is common across different
>> > > > > Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
>> > > > > a wrapper function to set the speeds. So that future coming SoC's
>> > > > > can use these wrapper functions to set speeds.
>> > > > >
>> > > > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> > > > > ---
>> > > > > Changes in v8:
>> > > > >     * common function to set INIT and operating speeds.
>> > > > >     * moved hardware flow control to qca_set_speed().
>> > > > >
>> > > > > Changes in v7:
>> > > > >     * initial patch
>> > > > >     * created wrapper functions for init and operating speeds.
>> > > > > ---
>> > > > >  drivers/bluetooth/hci_qca.c | 89
>> > > > > +++++++++++++++++++++++++++----------
>> > > > >  1 file changed, 65 insertions(+), 24 deletions(-)
>> > > > >
>> > > > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > > > > index fe62420ef838..38b7dbe6c897 100644
>> > > > > --- a/drivers/bluetooth/hci_qca.c
>> > > > > +++ b/drivers/bluetooth/hci_qca.c
>> > > > > @@ -119,6 +119,11 @@ struct qca_data {
>> > > > >  	u64 votes_off;
>> > > > >  };
>> > > > >
>> > > > > +enum qca_speed_type {
>> > > > > +	QCA_INIT_SPEED = 1,
>> > > > > +	QCA_OPER_SPEED
>> > > > > +};
>> > > > > +
>> > > > >  struct qca_serdev {
>> > > > >  	struct hci_uart	 serdev_hu;
>> > > > >  	struct gpio_desc *bt_en;
>> > > > > @@ -923,6 +928,60 @@ static inline void host_set_baudrate(struct
>> > > > > hci_uart *hu, unsigned int speed)
>> > > > >  		hci_uart_set_baudrate(hu, speed);
>> > > > >  }
>> > > > >
>> > > > > +static unsigned int qca_get_speed(struct hci_uart *hu,
>> > > > > +				  enum qca_speed_type speed_type)
>> > > > > +{
>> > > > > +	unsigned int speed = 0;
>> > > > > +
>> > > > > +	if (speed_type == QCA_INIT_SPEED) {
>> > > > > +		if (hu->init_speed)
>> > > > > +			speed = hu->init_speed;
>> > > > > +		else if (hu->proto->init_speed)
>> > > > > +			speed = hu->proto->init_speed;
>> > > > > +	} else {
>> > > > > +		if (hu->oper_speed)
>> > > > > +			speed = hu->oper_speed;
>> > > > > +		else if (hu->proto->oper_speed)
>> > > > > +			speed = hu->proto->oper_speed;
>> > > > > +	}
>> > > > > +
>> > > > > +	return speed;
>> > > > > +}
>> > > > > +
>> > > > > +static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type
>> > > > > speed_type)
>> > > > > +{
>> > > > > +	unsigned int speed, qca_baudrate;
>> > > > > +	int ret;
>> > > > > +
>> > > > > +	if (speed_type == QCA_INIT_SPEED) {
>> > > > > +		speed = qca_get_speed(hu, QCA_INIT_SPEED);
>> > > > > +		if (speed)
>> > > > > +			host_set_baudrate(hu, speed);
>> > > > > +		else
>> > > > > +			bt_dev_err(hu->hdev, "Init speed should be non zero");
>> > > >
>> > > > The check for 'speed == 0' is done in multiple places. From this
>> > > > code I deduce that it is expected that both INIT and OPER speed are
>> > > > set to non-zero values. What happens if either of them is zero? Is the
>> > > > driver still operational?
>> > > >
>> > > [Bala]: yes in hci_uart_setup()(hci_serdev.c) function before calling
>> > > qca_setup().
>> > >         we actually set baudrate, but int  above code missed to
>> > > restrict the
>> > > further call to qca_setup()
>> > >        if speed =0. so we are checking the same in the qca_setup()..
>> > > i.e.
>> > > qca_get_speed().
>> >
>> > Sorry, didn't quite get you here. Yes, the driver is still operational?
>> >
>> > Breaking it down in multiple questions:
>> >
>> > 1. Is it an error if one of the speeds isn't specified?
>> >
>> 
>> [Bala]:  i want to break this issue for two chips.
>>         for rome:  No, it is not a error if one of speeds are missing.
>>                    but it is mandate to have at least one of the 
>> speeds.
>>                    to current implementation we are strictly not 
>> checking
>> this case.
>>                    i.e. terminate qca_setup() if both speeds missing.
>>                    will do this change in this patch.
>>         for wnc3990: yes it an error if any of the speeds are missing.
>>                      it is mandate to have both the speeds.
>>                      to current implementation we are strictly not 
>> checking
>> this case.
>>                      i.e. terminate qca_setup() if any speeds missing.
>>                      will integrate this in patch add support for 
>> wcn3990.
>> 
>>        "The check for 'speed == 0' is done in multiple places. From 
>> this
>>        code I deduce that it is expected that both INIT and OPER speed 
>> are
>>        set to non-zero values. What happens if either of them is zero? 
>> Is
>> the
>>        driver still operational"
>> 
>>       still we will have speed ==0 check for BT chip ROME. pls find 
>> code
>> snippet below
>> 
>>          thanks for catching this corner case.
> 
> Thanks for the clarification
> 
>> > If yes we should probably check this early once and return an error
>> > early, instead of doing the check repeatedly
>> >
>> > 2. If it is not an error, what is the driver supposed to do?
>> >
>> > > > In the discussion on "[v7,8/8] Bluetooth: hci_qca: Add support for
>> > > > Qualcomm Bluetooth chip wcn3990" you mentioned the possbility to move
>> > > > the hci_uart_set_flow_control() calls into _set_speed(). This seemed
>> > > > interesting but finally it isn't done in this series. Did you
>> > > > encounter that it is not feasible/desirable for some reason?
>> > > >
>> > >
>> > > [Bala]: this patch is for rome where flow control is not used.
>> > >         after we integrate wcn3990, flow control is hidden in the
>> > > qca_set_speed()
>> > >         Pls check [v8 7/7] patch.
>> >
>> > Sorry, my confusion
>> >
>> > > > >  static int qca_setup(struct hci_uart *hu)
>> > > > >  {
>> > > > >  	struct hci_dev *hdev = hu->hdev;
>> > > > > @@ -937,35 +996,17 @@ static int qca_setup(struct hci_uart *hu)
>> > > > >  	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> > > > >
>> > > > >  	/* Setup initial baudrate */
>> > > > > -	speed = 0;
>> > > > > -	if (hu->init_speed)
>> > > > > -		speed = hu->init_speed;
>> > > > > -	else if (hu->proto->init_speed)
>> > > > > -		speed = hu->proto->init_speed;
>> > > > > -
>> > > > > -	if (speed)
>> > > > > -		host_set_baudrate(hu, speed);
>> > > > > +	qca_set_speed(hu, QCA_INIT_SPEED);
>> > > > >
>> > > > >  	/* Setup user speed if needed */
>> > > > > -	speed = 0;
>> > > > > -	if (hu->oper_speed)
>> > > > > -		speed = hu->oper_speed;
>> > > > > -	else if (hu->proto->oper_speed)
>> > > > > -		speed = hu->proto->oper_speed;
>> > > > > +	ret = qca_set_speed(hu, QCA_OPER_SPEED);
>> > > > > +	if (ret)
>> > > > > +		return ret;
>> > > > >
>> > > > > -	if (speed) {
>> > > > > +	speed = qca_get_speed(hu, QCA_OPER_SPEED);
>> > > > > +	if (speed)
>> > > > >  		qca_baudrate = qca_get_baudrate_value(speed);
>> > > >
>> > > > Is the check here necessary? qca_get_baudrate_value() returns
>> > > > QCA_BAUDRATE_115200 for a zero speed.
>> > >
>> > > this if for no zero operating speed,
>> >
>> > My point is:
>> >
>> > static int qca_setup(struct hci_uart *hu)
>> > {
>> > 	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
>> >
>> > 	...
>> >
>> > 	if (speed)
>> > 		qca_baudrate = qca_get_baudrate_value(speed);
>> > }
>> >
>> > static uint8_t qca_get_baudrate_value(int speed)
>> > {
>> >         switch (speed) {
>> >
>> > 	...
>> >
>> >         default:
>> >                 return QCA_BAUDRATE_115200;
>> >         }
>> > }
>> >
>> > If qca_get_baudrate_value() is called with 'speed == 0' it returns
>> > QCA_BAUDRATE_115200, which is the same value with which
>> > QCA_BAUDRATE_115200 is initialized.
>> >
>> > It seems the initialization and the check for 'speed == 0' could be
>> > removed.
>> 
>> [Bala]: the above speed == 0 is highly recommended, let us assume ROME 
>> chip
>> init speed moved from 115200 to 2000000 and operating speed is defined 
>> as
>> zero.
>>        then this check will help us. else if we remove speed ==0 then 
>> in
>> qca_baudrate we will end up having a different value.
>>         Pls find the code snippet for better understanding.
> 
> Hm, still not clear to me, I'm no Bluetooth expert, maybe I'm missing
> something that seems obvious to others.
> 
> What exactly do you mean with "init speed moved from 115200 to
> 2000000"?
> 
> Is it that the first qca_set_speed(hu, QCA_INIT_SPEED) call in
> qca_setup() uses hu->proto->init_speed and the second one
> hu->init_speed after it was initialized in qca_open()?
> 
> I saw you added a new assignment of qca_baudrate below, which probably
> is related, though you don't mention it here:
> 
>         /* Setup initial baudrate */
>         qca_set_speed(hu, QCA_INIT_SPEED);
>         speed = qca_get_speed(hu, QCA_INIT_SPEED);
>         if (speed)
>                 qca_baudrate = qca_get_baudrate_value(speed);
> 
> 
> I find it really hard to discuss in code snippets, it's probably best
> to send a new version of the patch and discuss it with the full
> context.
> 
> Regarding the new qca_get_speed() and assignment of qca_baudrate: if
> this needs to be done it should probably be moved just before this 
> block
> to keep things together:
> 
>         speed = qca_get_speed(hu, QCA_OPER_SPEED);
>         if (speed)
>                 qca_baudrate = qca_get_baudrate_value(speed);
> 
> If I didn't lose myself jumping back and forth between the snippets
> and v8 qca_baudrate isn't used before. And if that is correct then it
> probably shouldn't be moved before the block, but rewritten to:
> 
>         speed = qca_get_speed(hu, QCA_OPER_SPEED);
>         if (!speed)
> 	        // Note mka@: no need to check 'speed', we know at
> 		// least one of them is set
> 	        speed = qca_get_speed(hu, QCA_INIT_SPEED);
> 
>         qca_baudrate = qca_get_baudrate_value(speed);
> 
> Not sure if that's correct, but it seems reasonable in the sense that
> you said that at least one of the speeds needs to be set and it
> certainly looks cleaner than the spread out _get_speed() and
> qca_get_baudrate_value(). But maybe I just got it wrong and reality is
> more ugly, best sent a new patch and have the full context.
> 
>> after your suggestion. code snippet along with wcn3990 integration.
>> 
>> 
>> [Bala]: for rome any one of the speeds are required.
>>         for wcn3990 requires both speeds.
>> 
>> static int qca_check_speeds(struct hci_uart *hu)
>> {
>>         struct qca_serdev *qcadev;
>> 
>>         qcadev = serdev_device_get_drvdata(hu->serdev);
>>         if (qcadev->btsoc_type == QCA_WCN3990) {
>>                 /* QCA WCN3990 requires both the speed values. */
>>                 if (qca_get_speed(hu, QCA_INIT_SPEED) &&
>>                     qca_get_speed(hu, QCA_OPER_SPEED))
>> 
>>                         return 0;
>> 
>>                 bt_dev_err(hu->hdev, "Both the speeds should be non 
>> zero");
>>                 return 1;
>>         }
> 
> It would probably be clearer to invert the logic, and check for the
> error condition and return 0 in the main branch (or even better, at
> the end of the function).
> 
> 
>>         if (qca_get_speed(hu, QCA_INIT_SPEED) ||
>>              qca_get_speed(hu, QCA_OPER_SPEED))
>>                 return 0;
>> 
>>         bt_dev_err(hu->hdev, "either of the speeds should be non 
>> zero");
>>         return 1;
> 
> ditto
> 
>>         [Bala]: pls suggest, what could be error no if above criteria 
>> is not
>> met.
> 
> I think you could use -EINVAL as it would be a configuration error.
> 
>> static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type
>> speed_type)
>> {
>>         struct qca_serdev *qcadev;
>>         unsigned int speed, qca_baudrate;
>>         int ret;
>> 
>>         if (speed_type == QCA_INIT_SPEED) {
>>                 speed = qca_get_speed(hu, QCA_INIT_SPEED);
>>                 if (speed)
>>                         host_set_baudrate(hu, speed);
>>                 [Bala]: this speed check is required for ROME, that if 
>> ROME
>> init speed is zero.
>>                 return 0;
>>         }
> 
> Just noticed, I think this would be clearer with an else branch. The
> current structure might be based on a comment from me on an earlier
> version of the _set/get_speed() rework, where I suggested to return to
> save a level of indentation. Here we set either INIT or OPER speed,
> not having the else branch can give the impression that both might be
> set.
> 
>>         speed = qca_get_speed(hu, QCA_OPER_SPEED);
>>         if (!speed)
>>                 return 0;
>>         [Bala]: this speed check is required for ROME, that if ROME
>> operating speed is zero.
>> 
>> [Bala]: this is speeds in both OPER and init speeds are required.. as 
>> we
>> move forward if any one of the speed is set.
>> 
>>         qcadev = serdev_device_get_drvdata(hu->serdev);
>>         /* Disabling hardware flow control is preferred while
>>          * sending change baud rate command to SoC.
>> if (qcadev->btsoc_type == QCA_WCN3990)
>>                 hci_uart_set_flow_control(hu, true);
>> 
>>         qca_baudrate = qca_get_baudrate_value(speed);
>>         bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
>>         ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>>         if (ret) {
>>                 bt_dev_err(hu->hdev, "Failed to change the baudrate 
>> (%d)",
>> ret);
>>                 return ret;
>>         }
>> 
>>         host_set_baudrate(hu, speed);
>>         if (qcadev->btsoc_type == QCA_WCN3990)
>>                 hci_uart_set_flow_control(hu, false);
>> 
>>         return 0;
>> }
>> 
>> static int qca_setup(struct hci_uart *hu)
>> {
>>         struct hci_dev *hdev = hu->hdev;
>>         struct qca_data *qca = hu->priv;
>>         unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
>>         struct qca_serdev *qcadev;
>>         int ret;
>>         int soc_ver = 0;
>> 
>>         qcadev = serdev_device_get_drvdata(hu->serdev);
>> 
>>         /* Patch downloading has to be done without IBS mode */
>>         clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> 
>>         ret = qca_check_speeds(hu);
>>         if (ret)
>>                 return ret;
>> 
>>         /* Setup initial baudrate */
>>         qca_set_speed(hu, QCA_INIT_SPEED);
>>         speed = qca_get_speed(hu, QCA_INIT_SPEED);
>>         if (speed)
>>                 qca_baudrate = qca_get_baudrate_value(speed);
>> 
>> [Bala]: i have newly add above statement. initial speed of both ROME 
>> and
>> wcn3990 when the chip boot up is 115200.
>>         in future, if we have new chip where initial  speed of chip 
>> during
>> boot up is 200000. then this will help us to get the actual initial 
>> speed
>> i.e. based upon the init speed.
> 
> As mentioned above, this should probably move further down.
> 
>>         if (qcadev->btsoc_type == QCA_WCN3990) {
>> bt_dev_dbg(hdev, "setting up wcn3990");
>>                 hci_uart_set_flow_control(hu, true);
>>                 ret = qca_send_vendor_cmd(hdev, 
>> QCA_WCN3990_POWERON_PULSE);
>>                 if (ret)
>>                         return ret;
>> 
>>                 hci_uart_set_flow_control(hu, false);
>>                 serdev_device_close(hu->serdev);
>>                 ret = serdev_device_open(hu->serdev);
>>                 if (ret) {
>>                         bt_dev_err(hdev, "failed to open port");
>>                         return ret;
>>                 }
>> 
>>                 msleep(100);
>>                 /* Setup initial baudrate */
>>                 qca_set_speed(hu, QCA_INIT_SPEED);
>>                 hci_uart_set_flow_control(hu, false);
>>                 ret = qca_read_soc_version(hdev, &soc_ver);
>>                 if (ret < 0 || soc_ver == 0) {
>>                         bt_dev_err(hdev, "Failed to get version %d", 
>> ret);
>>                         return ret;
>>                 }
>>                 bt_dev_info(hdev, "wcn3990 controller version 0x%08x",
>> soc_ver);
>>         } else {
>>                 bt_dev_info(hdev, "ROME setup");
>>         }
>> 
>>         /* Setup user speed if needed */
>>         ret = qca_set_speed(hu, QCA_OPER_SPEED);
>>         if (ret)
>>                 return ret;
>> 
>>         speed = qca_get_speed(hu, QCA_OPER_SPEED);
>>         if (speed)
>>                 qca_baudrate = qca_get_baudrate_value(speed);
>> 
>>       [Bala]: the above speed == 0 is highly recommended, let us 
>> assume ROME
>> chip init speed moved from 115200 to 2000000 and operating speed is 
>> defined
>> as zero.
>>        then this check will help us. else if we remove speed ==0 then 
>> in
>> qca_baudrate we will end up having a different value.
>>        ….
>>         }
>> 
>>  Pls let me know, whether i have clarified your queries.
> 
> Partially, let's continue the discussion on v9 unless you have
> questions/comments on my comments before you post.
> 
> Thanks
> 
> Matthias

Thanks for inputs, will fix your comments and send you v9 patch set 
tomorrow.

-- 
Regards
Balakrishna.

Re: [PATCH v8 1/7] dt-bindings: net: bluetooth: Add device tree bindings for QTI chip wcn3990

[Balakrishna Godavarthi]

Hi Rob,

On 2018-06-25 20:28, Rob Herring wrote:
> On Mon, Jun 25, 2018 at 07:10:07PM +0530, Balakrishna Godavarthi wrote:
>> This patch enables regulators for the Qualcomm Bluetooth wcn3990
>> controller.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> 
>> Changes in v8:
>>     * Separated the optional entries between two chips
>> 
>> Changes in v7:
>>     * no change.
>> 
>> Changes in v6:
>> 
>>     * Changed the oper-speed to max-speed.
>> 
>> Changes in v5:
>> 
>>     * Added entry for oper-speed and init-speed.
>> 
>> ---
>>  .../bindings/net/qualcomm-bluetooth.txt       | 30 
>> +++++++++++++++++--
>>  1 file changed, 28 insertions(+), 2 deletions(-)
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt 
>> b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> index 0ea18a53cc29..c2b09dd2bf31 100644
>> --- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> @@ -10,12 +10,23 @@ device the slave device is attached to.
>>  Required properties:
>>   - compatible: should contain one of the following:
>>     * "qcom,qca6174-bt"
>> +   * "qcom,wcn3990-bt"
>> +
>> +Optional properties for compatible string qcom,qca6174-bt:
>> 
>> -Optional properties:
>>   - enable-gpios: gpio specifier used to enable chip
>>   - clocks: clock provided to the controller (SUSCLK_32KHZ)
>> 
>> -Example:
>> +Optional properties for compatible string qcom,wcn3990-bt:
>> +
>> + - vddio-supply: Bluetooth VDD IO regulator handle.
>> + - vddxtal-supply: Bluetooth VDD XTAL regulator handle.
>> + - vddpa-supply: Bluetooth VDD PA regulator handle.
>> + - vddldo-supply: Bluetooth VDD LDO regulator handle.
>> + - vddcore-supply: Bluetooth VDD CORE regulator handle.
>> + - max-speed: Maximum operating speed of the chip.
> 
> This shouldn't be per compatible really. The definition here is wrong
> too. You should know the max baud for the chip based on the compatible.
> This property is for when the max is less than the max of either the
> chip or host UART.
> 
> Rob

I have enabled max-speed in probe for compatible "qcom,wcn3990-bt", so 
it under the specific compatible string.
will update definition.

-- 
Regards
Balakrishna.