[PATCH v7 0/8] Enable Bluetooth functionality for WCN3990

[PATCH v7 0/8] 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.

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 (8):
  dt-bindings: net: bluetooth: Add device tree bindings for QTI chip
    wcn3990
  Bluetooth: btqca: Rename string ROME to QCA in logs.
  Bluetooth: btqca: Rename ROME related functions to Generic functions
  Bluetooth: btqca: Redefine qca_uart_setup() to generic function.
  Bluetooth: hci_qca: Defined wrapper functions for setting UART speeds
  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       |  31 +-
 drivers/bluetooth/btqca.c                     | 120 +++---
 drivers/bluetooth/btqca.h                     |  24 +-
 drivers/bluetooth/hci_qca.c                   | 382 +++++++++++++++---
 4 files changed, 454 insertions(+), 103 deletions(-)

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

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

[Balakrishna Godavarthi]

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
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       | 31 +++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
index 0ea18a53cc29..c4590bc87390 100644
--- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
+++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
@@ -10,12 +10,24 @@ device the slave device is attached to.
 Required properties:
  - compatible: should contain one of the following:
    * "qcom,qca6174-bt"
+   * "qcom,wcn3990-bt"
 
 Optional properties:
  - enable-gpios: gpio specifier used to enable chip
  - clocks: clock provided to the controller (SUSCLK_32KHZ)
-
-Example:
+ - vddpa-supply: Bluetooth VDD PA regulator handle
+ - vddio-supply: Bluetooth VDD IO regulator handle
+ - vddldo-supply: Bluetooth VDD LDO regulator handle. Kept under optional
+		  parameters as some of the chipsets doesn't require ldo or
+		  it may use from same vddio.
+ - vddxtal-supply: Bluetooth VDD XTAL regulator handle
+ - vddcore-supply: Bluetooth VDD CORE regulator handle
+ - vddpwd-supply: Chip power down gpio is required when bluetooth module
+		  and other modules like wifi co-exist in a singe chip and
+		  shares a common gpio to bring chip out of reset.
+ - max-speed: operating speed of the chip.
+
+Examples:
 
 serial@7570000 {
 	label = "BT-UART";
@@ -28,3 +40,18 @@ serial@7570000 {
 		clocks = <&divclk4>;
 	};
 };
+
+serial@898000 {
+	label = "BT-UART";
+	status = "okay";
+
+	bluetooth: bt_wcn3990 {
+		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 v7 2/8] Bluetooth: btqca: Rename string ROME to QCA in logs.

[Balakrishna Godavarthi]

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 v7:
	* initial patch.
	* Updated the logs to generic.

---
 drivers/bluetooth/btqca.c | 56 +++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 29 deletions(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index 8219816c54a0..a36fae1f3c2c 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -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,7 +75,7 @@ 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.
 	 */
@@ -94,12 +93,12 @@ static int rome_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;
 	}
 
@@ -228,19 +227,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 +248,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;
 	}
 
@@ -267,12 +266,12 @@ static int rome_download_firmware(struct hci_dev *hdev,
 	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;
 	}
 
@@ -317,8 +316,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;
 	}
 
@@ -334,18 +332,18 @@ int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate)
 	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 */
+	/* Get QCA version information */
 	err = rome_patch_ver_req(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;
@@ -353,7 +351,7 @@ int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate)
 		 rome_ver);
 	err = rome_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;
 	}
 
@@ -363,18 +361,18 @@ int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate)
 		 rome_ver);
 	err = rome_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);
 	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;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v7 3/8] Bluetooth: btqca: Rename ROME related 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.

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

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   | 29 +++++++++++++++--------------
 drivers/bluetooth/btqca.h   | 12 +++++++++---
 drivers/bluetooth/hci_qca.c |  2 +-
 3 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index a36fae1f3c2c..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;
@@ -79,7 +79,7 @@ static int rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
 	 * 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:
@@ -87,8 +87,9 @@ 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;
@@ -107,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;
@@ -206,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;
@@ -259,7 +260,7 @@ 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;
@@ -275,7 +276,7 @@ static int rome_download_firmware(struct hci_dev *hdev,
 		return ret;
 	}
 
-	rome_tlv_check_data(config, fw);
+	qca_tlv_check_data(config, fw);
 
 	segment = fw->data;
 	remain = fw->size;
@@ -289,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;
@@ -326,7 +327,7 @@ 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;
@@ -337,7 +338,7 @@ int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate)
 	config.user_baud_rate = baudrate;
 
 	/* Get QCA version information */
-	err = rome_patch_ver_req(hdev, &rome_ver);
+	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;
@@ -349,7 +350,7 @@ int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate)
 	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_dev_err(hdev, "QCA Failed to download patch (%d)", err);
 		return err;
@@ -359,14 +360,14 @@ 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_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_dev_err(hdev, "QCA Failed to run HCI_RESET (%d)", err);
 		return err;
@@ -376,7 +377,7 @@ int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate)
 
 	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..d0ee96540636 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -37,7 +37,7 @@
 #define EDL_TAG_ID_HCI			(17)
 #define EDL_TAG_ID_DEEP_SLEEP		(27)
 
-enum qca_bardrate {
+enum qca_baudrate {
 	QCA_BAUDRATE_115200 	= 0,
 	QCA_BAUDRATE_57600,
 	QCA_BAUDRATE_38400,
@@ -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 v7 4/8] 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 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..f748730039cf 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, uint8_t 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 d0ee96540636..c7a5d1754f47 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, uint8_t 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,
+				 uint8_t 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 v7 5/8] Bluetooth: hci_qca: Defined wrapper functions for setting UART speeds

[Balakrishna Godavarthi]

In 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 v7:
	* initial patch
	* created wrapper functions for init and operating speeds.

---
 drivers/bluetooth/hci_qca.c | 64 ++++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 19 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index fe62420ef838..3e09c6223baf 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -923,21 +923,10 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
 		hci_uart_set_baudrate(hu, speed);
 }
 
-static int qca_setup(struct hci_uart *hu)
+static void qca_set_init_speed(struct hci_uart *hu)
 {
-	struct hci_dev *hdev = hu->hdev;
-	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");
-
-	/* Patch downloading has to be done without IBS mode */
-	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
+	unsigned int speed = 0;
 
-	/* Setup initial baudrate */
-	speed = 0;
 	if (hu->init_speed)
 		speed = hu->init_speed;
 	else if (hu->proto->init_speed)
@@ -945,27 +934,64 @@ static int qca_setup(struct hci_uart *hu)
 
 	if (speed)
 		host_set_baudrate(hu, speed);
+}
+
+static int qca_get_oper_speed(struct hci_uart *hu)
+{
+	unsigned int speed = 0;
 
-	/* 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;
 
+	return speed;
+}
+
+static int qca_set_oper_speed(struct hci_uart *hu)
+{
+	unsigned int speed = 0, qca_baudrate;
+	int ret = 0;
+
+	speed = qca_get_oper_speed(hu);
 	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);
+		bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
+		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
 		if (ret) {
-			bt_dev_err(hdev, "Failed to change the baud rate (%d)",
+			bt_dev_err(hu->hdev, "Failed to change the baud rate (%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;
+	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");
+
+	/* Patch downloading has to be done without IBS mode */
+	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
+
+	/* Setup initial baudrate */
+	qca_set_init_speed(hu);
+	/* Setup user speed if needed */
+	ret = qca_set_oper_speed(hu);
+	if (ret)
+		return ret;
+	speed = qca_get_oper_speed(hu);
+	if (speed)
+		qca_baudrate = qca_get_baudrate_value(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 v7 6/8] 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 3e09c6223baf..28ae6a17a595 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -872,6 +872,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:
@@ -886,7 +888,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 v7 7/8] 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>
---
Changes in v7:
	* used redefined function qca_uart_setup for firmware download.

Changes in v6:
	* add code to derive the firmware files based on ROM Version.
	* created new patch for common function of ROME and wcn3990.

Changes in v5:
	* moved changes related to hci_qca.
	* addressed review comments.
 
Changes in v4:
	* initial patch.
---
 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 f748730039cf..f0a4205068b8 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, uint8_t soc_type,
 {
 	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, uint8_t soc_type,
 		}
 		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, uint8_t soc_type,
 
 	/* 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 v7 8/8] 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 v6:
	* 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 | 327 ++++++++++++++++++++++++++++++++----
 2 files changed, 297 insertions(+), 33 deletions(-)

diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index c7a5d1754f47..d49c1d847e1a 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 CHEROKEE_POWERON_PULSE          0xFC
+#define CHEROKEE_POWEROFF_PULSE         0xC0
+
 enum qca_baudrate {
 	QCA_BAUDRATE_115200 	= 0,
 	QCA_BAUDRATE_57600,
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 28ae6a17a595..1961e313aae7 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>
@@ -119,12 +124,46 @@ struct qca_data {
 	u64 votes_off;
 };
 
+/*
+ * 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_btsoc_power_setup(struct hci_uart *hu, bool on);
+static int qca_btsoc_shutdown(struct hci_uart *hu);
+
 static void __serial_clock_on(struct tty_struct *tty)
 {
 	/* TODO: Some chipset requires to enable UART clock on client
@@ -402,6 +441,7 @@ static int qca_open(struct hci_uart *hu)
 {
 	struct qca_serdev *qcadev;
 	struct qca_data *qca;
+	int ret = 0;
 
 	BT_DBG("hu %p qca_open", hu);
 
@@ -463,13 +503,19 @@ static int qca_open(struct hci_uart *hu)
 		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) {
+			hu->init_speed = qcadev->init_speed;
+			hu->oper_speed = qcadev->oper_speed;
+			ret = qca_btsoc_power_setup(hu, true);
+		} else {
+			gpiod_set_value_cansleep(qcadev->bt_en, 1);
+		}
 	}
 
 	BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u",
 	       qca->tx_idle_delay, qca->wake_retrans);
 
-	return 0;
+	return ret;
 }
 
 static void qca_debugfs_init(struct hci_dev *hdev)
@@ -549,10 +595,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_btsoc_shutdown(hu);
+		else
+			gpiod_set_value_cansleep(qcadev->bt_en, 0);
+
+		serdev_device_close(hu->serdev);
 	}
 
 	kfree_skb(qca->rx_skb);
@@ -925,6 +974,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 skb 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 void qca_set_init_speed(struct hci_uart *hu)
 {
 	unsigned int speed = 0;
@@ -971,21 +1046,60 @@ static int qca_set_oper_speed(struct hci_uart *hu)
 	return ret;
 }
 
+static int qca_btsoc_shutdown(struct hci_uart *hu)
+{
+	struct hci_dev *hdev = hu->hdev;
+
+	host_set_baudrate(hu, 2400);
+	qca_send_vendor_cmd(hdev, CHEROKEE_POWEROFF_PULSE);
+	return qca_btsoc_power_setup(hu, false);
+}
+
 static int qca_setup(struct hci_uart *hu)
 {
 	struct hci_dev *hdev = hu->hdev;
 	struct qca_data *qca = hu->priv;
+	struct qca_serdev *qcadev;
 	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
 	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);
-
-	/* Setup initial baudrate */
 	qca_set_init_speed(hu);
+
+	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, CHEROKEE_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);
+		qca_set_init_speed(hu);
+		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);
+		hci_uart_set_flow_control(hu, true);
+
+	} else {
+		bt_dev_info(hdev, "ROME setup");
+	}
+
 	/* Setup user speed if needed */
 	ret = qca_set_oper_speed(hu);
 	if (ret)
@@ -995,7 +1109,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);
@@ -1031,9 +1145,118 @@ static struct hci_uart_proto qca_proto = {
 	.dequeue	= qca_dequeue,
 };
 
+static const struct qca_vreg_data qca_cherokee_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 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)
+		goto out;
+
+	if (vregs.load_uA)
+		ret = regulator_set_load(regulator,
+					 vregs.load_uA);
+
+	if (ret)
+		goto out;
+
+	ret = regulator_enable(regulator);
+
+out:
+	return ret;
+
+}
+
+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_btsoc_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);
@@ -1041,47 +1264,85 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		return -ENOMEM;
 
 	qcadev->serdev_hu.serdev = serdev;
+	data = of_device_get_match_data(&serdev->dev);
+	if (data && data->soc_type == QCA_WCN3990)
+		qcadev->btsoc_type = QCA_WCN3990;
+	else
+		qcadev->btsoc_type = QCA_ROME;
+
 	serdev_device_set_drvdata(serdev, qcadev);
+	if (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->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_btsoc_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_cherokee_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 v7 1/8] dt-bindings: net: bluetooth: Add device tree bindings for QTI chip wcn3990

[Rob Herring]

On Sat, Jun 16, 2018 at 12:27 AM, Balakrishna Godavarthi
<bgodavar@codeaurora.org> wrote:
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>

Commit messag missing.

Please use get_maintainers.pl and send patches to the right lists. DT
list in this case.

> ---
> 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       | 31 +++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> index 0ea18a53cc29..c4590bc87390 100644
> --- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> @@ -10,12 +10,24 @@ device the slave device is attached to.
>  Required properties:
>   - compatible: should contain one of the following:
>     * "qcom,qca6174-bt"
> +   * "qcom,wcn3990-bt"
>
>  Optional properties:
>   - enable-gpios: gpio specifier used to enable chip
>   - clocks: clock provided to the controller (SUSCLK_32KHZ)
> -
> -Example:
> + - vddpa-supply: Bluetooth VDD PA regulator handle
> + - vddio-supply: Bluetooth VDD IO regulator handle
> + - vddldo-supply: Bluetooth VDD LDO regulator handle. Kept under optional
> +                 parameters as some of the chipsets doesn't require ldo or
> +                 it may use from same vddio.

You need to be explicit as to which properties apply or don't apply to
specific compatible strings.

Supplies should be based on inputs to the chip, so if 1 regulator
supplies 2 inputs you still need both.

> + - vddxtal-supply: Bluetooth VDD XTAL regulator handle
> + - vddcore-supply: Bluetooth VDD CORE regulator handle
> + - vddpwd-supply: Chip power down gpio is required when bluetooth module
> +                 and other modules like wifi co-exist in a singe chip and
> +                 shares a common gpio to bring chip out of reset.
> + - max-speed: operating speed of the chip.
> +
> +Examples:
>
>  serial@7570000 {
>         label = "BT-UART";
> @@ -28,3 +40,18 @@ serial@7570000 {
>                 clocks = <&divclk4>;
>         };
>  };
> +
> +serial@898000 {
> +       label = "BT-UART";
> +       status = "okay";
> +
> +       bluetooth: bt_wcn3990 {

Reverse these. The node name should be "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
>

Re: [PATCH v7 8/8] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990

[Stephen Boyd]

Quoting Balakrishna Godavarthi (2018-06-15 23:27:18)
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 28ae6a17a595..1961e313aae7 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1031,9 +1145,118 @@ static struct hci_uart_proto qca_proto =3D {
>         .dequeue        =3D qca_dequeue,
>  };
>  =

> +static const struct qca_vreg_data qca_cherokee_data =3D {
> +       .soc_type =3D QCA_WCN3990,
> +       .vregs =3D (struct qca_vreg []) {
> +               { "vddio",   1352000, 1352000,  0 },
> +               { "vddxtal", 1904000, 2040000,  0 },
> +               { "vddcore", 1800000, 1800000,  1 },
> +               { "vddpa",   1304000, 1304000,  1 },
> +               { "vddldo",  3000000, 3312000,  1 },

Load of 0 and 1 seems sort of odd. Are these made up to indicate "some
load" vs. "no load"?

> +       },
> +       .num_vregs =3D 5,
> +};
> +
> +static int qca_enable_regulator(struct qca_vreg vregs,
> +                               struct regulator *regulator)
> +{
> +       int ret;
> +
> +       ret =3D regulator_set_voltage(regulator, vregs.min_uV,
> +                                   vregs.max_uV);
> +       if (ret)
> +               goto out;

Just return ret;

> +
> +       if (vregs.load_uA)
> +               ret =3D regulator_set_load(regulator,
> +                                        vregs.load_uA);
> +
> +       if (ret)
> +               goto out;
> +

Same.

> +       ret =3D regulator_enable(regulator);
> +
> +out:
> +       return ret;

And make this return regulator_enable(regulator).

> +
> +}
> +

Re: [PATCH v7 8/8] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990

[Balakrishna Godavarthi]

Hi Stephen,

Please find my comments inline.

On 2018-06-18 22:12, Stephen Boyd wrote:
> Quoting Balakrishna Godavarthi (2018-06-15 23:27:18)
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 28ae6a17a595..1961e313aae7 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -1031,9 +1145,118 @@ static struct hci_uart_proto qca_proto = {
>>         .dequeue        = qca_dequeue,
>>  };
>> 
>> +static const struct qca_vreg_data qca_cherokee_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 },
> 
> Load of 0 and 1 seems sort of odd. Are these made up to indicate "some
> load" vs. "no load"?
> 

[Bala]: this value specifies the output load current required to turn on 
the wcn3990.
         in struct defined vddio\vddxtal are smps, with fixed load.
         regs from vddcore/vddpa/vddldo are programmable line regulators, 
in which we need to set the basic load.


>> +       },
>> +       .num_vregs = 5,
>> +};
>> +
>> +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)
>> +               goto out;
> 
> Just return ret;

[Bala]: will update.

> 
>> +
>> +       if (vregs.load_uA)
>> +               ret = regulator_set_load(regulator,
>> +                                        vregs.load_uA);
>> +
>> +       if (ret)
>> +               goto out;
>> +
> 
> Same.
> 

[Bala]: will update.

>> +       ret = regulator_enable(regulator);
>> +
>> +out:
>> +       return ret;
> 
> And make this return regulator_enable(regulator).
> 

[Bala]: will update.

>> +
>> +}
>> +

Thanks for reviewing the patch.

-- 
Regards
Balakrishna.

Re: [PATCH v7 2/8] Bluetooth: btqca: Rename string ROME to QCA in logs.

[Matthias Kaehlcke]

On Sat, Jun 16, 2018 at 11:57:12AM +0530, Balakrishna Godavarthi wrote:
> 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 v7:
> 	* initial patch.
> 	* Updated the logs to generic.
> 
> ---
>  drivers/bluetooth/btqca.c | 56 +++++++++++++++++++--------------------
>  1 file changed, 27 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index 8219816c54a0..a36fae1f3c2c 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -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);

[Not really the scope of this patch/series, but an idea for a future
improvement:

Some error messages put the error code inside parentheses, others put
it after a colon. Would be good to unify this (my preference would be
after the colon).]

Shouldn't this patch be squashed with "[3/8] Bluetooth: btqca: Rename
ROME related functions to Generic functions"? It's a bit odd to do the
renaming of the functions and log entries in two steps, even though
the end result is the same.

What would make sense is to have a separate patch for BT_XYZ() to
bt_dev_xyz(), though it's probably ok to do it in the patch that
changes the log strings.

Re: [PATCH v7 3/8] Bluetooth: btqca: Rename ROME related functions to Generic functions

[Matthias Kaehlcke]

On Sat, Jun 16, 2018 at 11:57:13AM +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
> chip's to keep this generic and would help in future when we would have
> new BT SoC.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> 
> 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.
> 
> ---

As per my comment on "[2/8] Bluetooth: btqca: Rename string ROME to
QCA in logs.", it would probably make sense to squash these two
patches.

> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index 13d77fd873b6..d0ee96540636 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -37,7 +37,7 @@
>  #define EDL_TAG_ID_HCI			(17)
>  #define EDL_TAG_ID_DEEP_SLEEP		(27)
>  
> -enum qca_bardrate {
> +enum qca_baudrate {
>  	QCA_BAUDRATE_115200 	= 0,
>  	QCA_BAUDRATE_57600,
>  	QCA_BAUDRATE_38400,

This is a bit sneaky, it is not related with the change nor mentioned
in the commit message.

It is certainly a welcome change, but should be done in a separate
patch (not necessarily in this series).

The fact that only the enum declaration needs to be fixed indicates
that the enum isn't used for variables/paramters/return values of this
type. Also beyond the scope of this series, but using the enum type
instead of int types would be another possible future improvement.

Re: [PATCH v7 4/8] Bluetooth: btqca: Redefine qca_uart_setup() to generic function.

[Matthias Kaehlcke]

On Sat, Jun 16, 2018 at 11:57:14AM +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 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..f748730039cf 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, uint8_t soc_type,

Use 'enum qca_btsoc_type' as type for '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);
>  	}

Why is this needed? A caller that passes in soc_ver = 0 could just
call qca_read_soc_version() themselves.

Re: [PATCH v7 5/8] Bluetooth: hci_qca: Defined wrapper functions for setting UART speeds

[Matthias Kaehlcke]

On Sat, Jun 16, 2018 at 11:57:15AM +0530, Balakrishna Godavarthi wrote:
> Subject: Bluetooth: hci_qca: Defined wrapper functions for setting
>          UART speeds

s/Defined/Define/

or

s/Defined/Add/

>
> In 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 v7:
> 	* initial patch
> 	* created wrapper functions for init and operating speeds.
> 
> ---
>  drivers/bluetooth/hci_qca.c | 64 ++++++++++++++++++++++++++-----------
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index fe62420ef838..3e09c6223baf 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -923,21 +923,10 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
>  		hci_uart_set_baudrate(hu, speed);
>  }
>  
> -static int qca_setup(struct hci_uart *hu)
> +static void qca_set_init_speed(struct hci_uart *hu)
>  {
> -	struct hci_dev *hdev = hu->hdev;
> -	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");
> -
> -	/* Patch downloading has to be done without IBS mode */
> -	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> +	unsigned int speed = 0;
>  
> -	/* Setup initial baudrate */
> -	speed = 0;
>  	if (hu->init_speed)
>  		speed = hu->init_speed;
>  	else if (hu->proto->init_speed)
> @@ -945,27 +934,64 @@ static int qca_setup(struct hci_uart *hu)
>  
>  	if (speed)
>  		host_set_baudrate(hu, speed);
> +}
> +
> +static int qca_get_oper_speed(struct hci_uart *hu)

Return type should be unsigned int.

> +{
> +	unsigned int speed = 0;
>  
> -	/* 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;
>  
> +	return speed;
> +}
> +
> +static int qca_set_oper_speed(struct hci_uart *hu)
> +{
> +	unsigned int speed = 0, qca_baudrate;

initialization is not needed.

> +	int ret = 0;
> +
> +	speed = qca_get_oper_speed(hu);
>  	if (speed) {

nit:
	if (!speed)
		return 0;


>  		qca_baudrate = qca_get_baudrate_value(speed);
> -
> -		bt_dev_info(hdev, "Set UART speed to %d", speed);
> -		ret = qca_set_baudrate(hdev, qca_baudrate);
> +		bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
> +		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>  		if (ret) {
> -			bt_dev_err(hdev, "Failed to change the baud rate (%d)",
> +			bt_dev_err(hu->hdev, "Failed to change the baud rate (%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;
> +	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");
> +
> +	/* Patch downloading has to be done without IBS mode */
> +	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> +
> +	/* Setup initial baudrate */
> +	qca_set_init_speed(hu);
> +	/* Setup user speed if needed */
> +	ret = qca_set_oper_speed(hu);
> +	if (ret)
> +		return ret;
> +	speed = qca_get_oper_speed(hu);
> +	if (speed)
> +		qca_baudrate = qca_get_baudrate_value(speed);

nit: the sequence

qca_set_oper_speed()
qca_get_oper_speed()

is slightly awkward to read. This could be avoided if the speed was
passed as parameter, though I understand this isn't done for symmetry
with qca_set_init_speed(). An alternative could be:

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

	if (speed_type == QCA_OPER_SPEED) {
		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 baud rate (%d)",
				ret);
			return ret;
		}
	}

	host_set_baudrate(hu, speed);

	/* If the order doesn't matter set the host baudrate first and
	   return if speed_type != QCA_OPER_SPEED */

	return 0;
}

static int qca_setup(struct hci_uart *hu)
{
	...
	speed = qca_get_init_speed(hu);
	if (speed)
		qca_set_speed(hu, speed, QCA_INIT_SPEED);

	speed = qca_get_oper_speed(hu);
	if (speed) {
	   	qca_set_speed(hu, speed, QCA_OPER_SPEED);
		qca_baudrate = qca_get_baudrate_value(speed);
	}
	...
}

Just a suggestion, ok for me if you prefer to keep it as is.

Re: [PATCH v7 7/8] Bluetooth: btqca: Add wcn3990 firmware download support.

[Matthias Kaehlcke]

On Sat, Jun 16, 2018 at 11:57:17AM +0530, Balakrishna Godavarthi wrote:
> This patch enables the RAM and NV patch download for wcn3990.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> Changes in v7:
> 	* used redefined function qca_uart_setup for firmware download.
> 
> Changes in v6:
> 	* add code to derive the firmware files based on ROM Version.
> 	* created new patch for common function of ROME and wcn3990.
> 
> Changes in v5:
> 	* moved changes related to hci_qca.
> 	* addressed review comments.
>  
> Changes in v4:
> 	* initial patch.
> ---
>  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 f748730039cf..f0a4205068b8 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, uint8_t soc_type,
>  {
>  	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, uint8_t soc_type,
>  		}
>  		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.
> +		 */

nit: new line after '/*' (https://www.kernel.org/doc/html/v4.17/process/coding-style.html#commenting))

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

Re: [PATCH v7 7/8] Bluetooth: btqca: Add wcn3990 firmware download support.

[Marcel Holtmann]

Hi Matthias,

>> This patch enables the RAM and NV patch download for wcn3990.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> Changes in v7:
>> 	* used redefined function qca_uart_setup for firmware download.
>> 
>> Changes in v6:
>> 	* add code to derive the firmware files based on ROM Version.
>> 	* created new patch for common function of ROME and wcn3990.
>> 
>> Changes in v5:
>> 	* moved changes related to hci_qca.
>> 	* addressed review comments.
>> 
>> Changes in v4:
>> 	* initial patch.
>> ---
>> 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 f748730039cf..f0a4205068b8 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, uint8_t soc_type,
>> {
>> 	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, uint8_t soc_type,
>> 		}
>> 		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.
>> +		 */
> 
> nit: new line after '/*' (https://www.kernel.org/doc/html/v4.17/process/coding-style.html#commenting))

actually not in the networking code. It has special commenting rules ;)

Regards

Marcel

Re: [PATCH v7 2/8] Bluetooth: btqca: Rename string ROME to QCA in logs.

[Balakrishna Godavarthi]

Hi Matthias,

Please find my comments inline.

On 2018-06-19 01:12, Matthias Kaehlcke wrote:
> On Sat, Jun 16, 2018 at 11:57:12AM +0530, Balakrishna Godavarthi wrote:
>> 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 v7:
>> 	* initial patch.
>> 	* Updated the logs to generic.
>> 
>> ---
>>  drivers/bluetooth/btqca.c | 56 
>> +++++++++++++++++++--------------------
>>  1 file changed, 27 insertions(+), 29 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>> index 8219816c54a0..a36fae1f3c2c 100644
>> --- a/drivers/bluetooth/btqca.c
>> +++ b/drivers/bluetooth/btqca.c
>> @@ -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);
> 
> [Not really the scope of this patch/series, but an idea for a future
> improvement:
> 
> Some error messages put the error code inside parentheses, others put
> it after a colon. Would be good to unify this (my preference would be
> after the colon).]
> 

[Bala]: will update.

> Shouldn't this patch be squashed with "[3/8] Bluetooth: btqca: Rename
> ROME related functions to Generic functions"? It's a bit odd to do the
> renaming of the functions and log entries in two steps, even though
> the end result is the same.
> 
> What would make sense is to have a separate patch for BT_XYZ() to
> bt_dev_xyz(), though it's probably ok to do it in the patch that
> changes the log strings.

[Bala]: to make review easy, i have segregated into two patches. will 
squash this into [3/8] patch.


-- 
Regards
Balakrishna.

Re: [PATCH v7 3/8] Bluetooth: btqca: Rename ROME related functions to Generic functions

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-06-19 01:29, Matthias Kaehlcke wrote:
> On Sat, Jun 16, 2018 at 11:57:13AM +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
>> chip's to keep this generic and would help in future when we would 
>> have
>> new BT SoC.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> 
>> 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.
>> 
>> ---
> 
> As per my comment on "[2/8] Bluetooth: btqca: Rename string ROME to
> QCA in logs.", it would probably make sense to squash these two
> patches.
> 

[Bala]: will squash [2/8] and [3/8] patches into one.

>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>> index 13d77fd873b6..d0ee96540636 100644
>> --- a/drivers/bluetooth/btqca.h
>> +++ b/drivers/bluetooth/btqca.h
>> @@ -37,7 +37,7 @@
>>  #define EDL_TAG_ID_HCI			(17)
>>  #define EDL_TAG_ID_DEEP_SLEEP		(27)
>> 
>> -enum qca_bardrate {
>> +enum qca_baudrate {
>>  	QCA_BAUDRATE_115200 	= 0,
>>  	QCA_BAUDRATE_57600,
>>  	QCA_BAUDRATE_38400,
> 
> This is a bit sneaky, it is not related with the change nor mentioned
> in the commit message.
> 
> It is certainly a welcome change, but should be done in a separate
> patch (not necessarily in this series).
> 
> The fact that only the enum declaration needs to be fixed indicates
> that the enum isn't used for variables/paramters/return values of this
> type. Also beyond the scope of this series, but using the enum type
> instead of int types would be another possible future improvement.

[Bala]: while reviewing the code, i found this. will leave it for now.
         once this series of patches are done. will send this change in 
different series.

-- 
Regards
Balakrishna.

Re: [PATCH v7 4/8] Bluetooth: btqca: Redefine qca_uart_setup() to generic function.

[Balakrishna Godavarthi]

Hi Matthias,

Please find my comments inline.

On 2018-06-19 02:49, Matthias Kaehlcke wrote:
> On Sat, Jun 16, 2018 at 11:57:14AM +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 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..f748730039cf 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, uint8_t 
>> soc_type,
> 
> Use 'enum qca_btsoc_type' as type for 'soc_type'.
> 

[Bala]: will update.

>> +		   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);
>>  	}
> 
> Why is this needed? A caller that passes in soc_ver = 0 could just
> call qca_read_soc_version() themselves.
[Bala]: hci_qca support two chip one ROME (already up-streamed) and 
other wcn3990.
         there setup sequence differs.

         wcn3900:
         1. set baudrate to 115200
         2. read soc version
         3. set baudarte to 3.2Mbps
         4. download firmware. (calls qca_uart_setup)

        ROME:
        1. set baudrate to 3.0 mbps
        2. calls qca_uart_setup to read soc version and download 
firmware.

       so to make use of existing function qca_setup_uart(). passing 
soc_ver as argument.
       if soc_ver is zero, then read soc version (i.e. for ROME chip).

       Pls let me know, if you have any queries.
-- 
Regards
Balakrishna.

Re: [PATCH v7 5/8] Bluetooth: hci_qca: Defined wrapper functions for setting UART speeds

[Balakrishna Godavarthi]

HI Matthias,

Please find my comments in line.

On 2018-06-19 03:21, Matthias Kaehlcke wrote:
> On Sat, Jun 16, 2018 at 11:57:15AM +0530, Balakrishna Godavarthi wrote:
>> Subject: Bluetooth: hci_qca: Defined wrapper functions for setting
>>          UART speeds
> 
> s/Defined/Define/
> 
> or
> 
> s/Defined/Add/
> 
[Bala]: will update the text.
>> 
>> In 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 v7:
>> 	* initial patch
>> 	* created wrapper functions for init and operating speeds.
>> 
>> ---
>>  drivers/bluetooth/hci_qca.c | 64 
>> ++++++++++++++++++++++++++-----------
>>  1 file changed, 45 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index fe62420ef838..3e09c6223baf 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -923,21 +923,10 @@ static inline void host_set_baudrate(struct 
>> hci_uart *hu, unsigned int speed)
>>  		hci_uart_set_baudrate(hu, speed);
>>  }
>> 
>> -static int qca_setup(struct hci_uart *hu)
>> +static void qca_set_init_speed(struct hci_uart *hu)
>>  {
>> -	struct hci_dev *hdev = hu->hdev;
>> -	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");
>> -
>> -	/* Patch downloading has to be done without IBS mode */
>> -	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> +	unsigned int speed = 0;
>> 
>> -	/* Setup initial baudrate */
>> -	speed = 0;
>>  	if (hu->init_speed)
>>  		speed = hu->init_speed;
>>  	else if (hu->proto->init_speed)
>> @@ -945,27 +934,64 @@ static int qca_setup(struct hci_uart *hu)
>> 
>>  	if (speed)
>>  		host_set_baudrate(hu, speed);
>> +}
>> +
>> +static int qca_get_oper_speed(struct hci_uart *hu)
> 
> Return type should be unsigned int.
> 
[Bala]: will update
>> +{
>> +	unsigned int speed = 0;
>> 
>> -	/* 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;
>> 
>> +	return speed;
>> +}
>> +
>> +static int qca_set_oper_speed(struct hci_uart *hu)
>> +{
>> +	unsigned int speed = 0, qca_baudrate;
> 
> initialization is not needed.
> 
[Bala]: will update

>> +	int ret = 0;
>> +
>> +	speed = qca_get_oper_speed(hu);
>>  	if (speed) {
> 
> nit:
> 	if (!speed)
> 		return 0;
> 
> 
[Bala]: will update.

>>  		qca_baudrate = qca_get_baudrate_value(speed);
>> -
>> -		bt_dev_info(hdev, "Set UART speed to %d", speed);
>> -		ret = qca_set_baudrate(hdev, qca_baudrate);
>> +		bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
>> +		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>>  		if (ret) {
>> -			bt_dev_err(hdev, "Failed to change the baud rate (%d)",
>> +			bt_dev_err(hu->hdev, "Failed to change the baud rate (%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;
>> +	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");
>> +
>> +	/* Patch downloading has to be done without IBS mode */
>> +	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> +
>> +	/* Setup initial baudrate */
>> +	qca_set_init_speed(hu);
>> +	/* Setup user speed if needed */
>> +	ret = qca_set_oper_speed(hu);
>> +	if (ret)
>> +		return ret;
>> +	speed = qca_get_oper_speed(hu);
>> +	if (speed)
>> +		qca_baudrate = qca_get_baudrate_value(speed);
> 
> nit: the sequence
> 
> qca_set_oper_speed()
> qca_get_oper_speed()
> 
> is slightly awkward to read. This could be avoided if the speed was
> passed as parameter, though I understand this isn't done for symmetry
> with qca_set_init_speed(). An alternative could be:
> 
> static int qca_set_speed(struct hci_uart *hu, unsigned int speed, enum
>     qca_speed_type speed_type)
> {
> 	unsigned int qca_baudrate;
> 
> 	if (speed_type == QCA_OPER_SPEED) {
> 		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 baud rate (%d)",
> 				ret);
> 			return ret;
> 		}
> 	}
> 
> 	host_set_baudrate(hu, speed);
> 
> 	/* If the order doesn't matter set the host baudrate first and
> 	   return if speed_type != QCA_OPER_SPEED */
> 
> 	return 0;
> }
> 
> static int qca_setup(struct hci_uart *hu)
> {
> 	...
> 	speed = qca_get_init_speed(hu);
> 	if (speed)
> 		qca_set_speed(hu, speed, QCA_INIT_SPEED);
> 
> 	speed = qca_get_oper_speed(hu);
> 	if (speed) {
> 	   	qca_set_speed(hu, speed, QCA_OPER_SPEED);
> 		qca_baudrate = qca_get_baudrate_value(speed);
> 	}
> 	...
> }
> 
> Just a suggestion, ok for me if you prefer to keep it as is.

[Bala]: will study and update the same.
-- 
Regards
Balakrishna.

Re: [PATCH v7 4/8] Bluetooth: btqca: Redefine qca_uart_setup() to generic function.

[Matthias Kaehlcke]

On Tue, Jun 19, 2018 at 12:39:07PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> Please find my comments inline.
> 
> On 2018-06-19 02:49, Matthias Kaehlcke wrote:
> > On Sat, Jun 16, 2018 at 11:57:14AM +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 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..f748730039cf 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, uint8_t
> > > soc_type,
> > 
> > Use 'enum qca_btsoc_type' as type for 'soc_type'.
> > 
> 
> [Bala]: will update.
> 
> > > +		   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);
> > >  	}
> > 
> > Why is this needed? A caller that passes in soc_ver = 0 could just
> > call qca_read_soc_version() themselves.
> [Bala]: hci_qca support two chip one ROME (already up-streamed) and other
> wcn3990.
>         there setup sequence differs.
> 
>         wcn3900:
>         1. set baudrate to 115200
>         2. read soc version
>         3. set baudarte to 3.2Mbps
>         4. download firmware. (calls qca_uart_setup)
> 
>        ROME:
>        1. set baudrate to 3.0 mbps
>        2. calls qca_uart_setup to read soc version and download firmware.
> 
>       so to make use of existing function qca_setup_uart(). passing soc_ver
> as argument.
>       if soc_ver is zero, then read soc version (i.e. for ROME chip).
> 
>       Pls let me know, if you have any queries.

I don't really understand this argumentation. Let's look at the code
at the end of this series, where both Rome and wcn3900 are supported:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1059

The version could be read (for Rome only) after setting the operating speed:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1111

I concede that having two "if (qcadev->btsoc_type == XYZ)" branches in
qca_setup() is kinda ugly, but I think it's still better than the
conditional call of qca_read_soc_version() in qca_uart_setup().

Re: [PATCH v7 8/8] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990

[Matthias Kaehlcke]

On Sat, Jun 16, 2018 at 11:57:18AM +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>
> ---
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 28ae6a17a595..1961e313aae7 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -402,6 +441,7 @@ static int qca_open(struct hci_uart *hu)
>  {
>  	struct qca_serdev *qcadev;
>  	struct qca_data *qca;
> +	int ret = 0;
>  
>  	BT_DBG("hu %p qca_open", hu);
>  
> @@ -463,13 +503,19 @@ static int qca_open(struct hci_uart *hu)
>  		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) {
> +			hu->init_speed = qcadev->init_speed;
> +			hu->oper_speed = qcadev->oper_speed;
> +			ret = qca_btsoc_power_setup(hu, true);

Better do this before starting the timers, otherwise you need to take
care of stopping them in case of failure.

If qca_btsoc_power_setup() fails you also have to free
'qca->workqueue' and 'qca'.

> +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 skb packet");

I was told that custom OOM messages should not be used, since the
mm code will complain loudly in this case.

> +static int qca_btsoc_shutdown(struct hci_uart *hu)

The return value is not evaluated by the only caller, so this should
probably be void.

>  static int qca_setup(struct hci_uart *hu)
>  {
>  	struct hci_dev *hdev = hu->hdev;
>  	struct qca_data *qca = hu->priv;
> +	struct qca_serdev *qcadev;
>  	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
>  	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);
> -
> -	/* Setup initial baudrate */
>  	qca_set_init_speed(hu);
> +
> +	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, CHEROKEE_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);
> +		qca_set_init_speed(hu);
> +		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);

serdev_device_close() ?

Also applies to other error paths in this function.

>  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);
> @@ -1041,47 +1264,85 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  		return -ENOMEM;
>  
>  	qcadev->serdev_hu.serdev = serdev;
> +	data = of_device_get_match_data(&serdev->dev);
> +	if (data && data->soc_type == QCA_WCN3990)
> +		qcadev->btsoc_type = QCA_WCN3990;
> +	else
> +		qcadev->btsoc_type = QCA_ROME;
> +
>  	serdev_device_set_drvdata(serdev, qcadev);
> +	if (qcadev->btsoc_type == QCA_WCN3990) {

nit: the double "if (soc_type == QCA_WCN3990)" is a bit odd. Consider
changing this condition to "if (data && data->soc_type == QCA_WCN3990)"
and assign qcadev->btsoc_type in the corresponding branch.

Re: [PATCH v7 4/8] Bluetooth: btqca: Redefine qca_uart_setup() to generic function.

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-06-20 01:33, Matthias Kaehlcke wrote:
> On Tue, Jun 19, 2018 at 12:39:07PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> Please find my comments inline.
>> 
>> On 2018-06-19 02:49, Matthias Kaehlcke wrote:
>> > On Sat, Jun 16, 2018 at 11:57:14AM +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 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..f748730039cf 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, uint8_t
>> > > soc_type,
>> >
>> > Use 'enum qca_btsoc_type' as type for 'soc_type'.
>> >
>> 
>> [Bala]: will update.
>> 
>> > > +		   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);
>> > >  	}
>> >
>> > Why is this needed? A caller that passes in soc_ver = 0 could just
>> > call qca_read_soc_version() themselves.
>> [Bala]: hci_qca support two chip one ROME (already up-streamed) and 
>> other
>> wcn3990.
>>         there setup sequence differs.
>> 
>>         wcn3900:
>>         1. set baudrate to 115200
>>         2. read soc version
>>         3. set baudarte to 3.2Mbps
>>         4. download firmware. (calls qca_uart_setup)
>> 
>>        ROME:
>>        1. set baudrate to 3.0 mbps
>>        2. calls qca_uart_setup to read soc version and download 
>> firmware.
>> 
>>       so to make use of existing function qca_setup_uart(). passing 
>> soc_ver
>> as argument.
>>       if soc_ver is zero, then read soc version (i.e. for ROME chip).
>> 
>>       Pls let me know, if you have any queries.
> 
> I don't really understand this argumentation. Let's look at the code
> at the end of this series, where both Rome and wcn3900 are supported:
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1059
> 
> The version could be read (for Rome only) after setting the operating 
> speed:

[Bala]: yes we can read SoC version after setting operating baud rate. 
an advice from SoC vendor that, read SoC version before setting 
operating speed.
         one advantage from reading SoC version before setting operating 
baudrate,it will helps us to understand whether the soc is responding to 
any commands in default baud rate
         before setting operating speed.
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1111
> 
> I concede that having two "if (qcadev->btsoc_type == XYZ)" branches in
> qca_setup() is kinda ugly, but I think it's still better than the
> conditional call of qca_read_soc_version() in qca_uart_setup().

[Bala]: we require an if-else block  for soc type.As wcn3900 support 
hardware flow control where as ROME is not supporting hardware flow 
control.

-- 
Regards
Balakrishna.

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

[Balakrishna Godavarthi]

Hi Rob,

please find my comments inline.

On 2018-06-18 18:59, Rob Herring wrote:
> On Sat, Jun 16, 2018 at 12:27 AM, Balakrishna Godavarthi
> <bgodavar@codeaurora.org> wrote:
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> 
> Commit messag missing.
> 
[Bala]: my bad, i some how missed. will update.

> Please use get_maintainers.pl and send patches to the right lists. DT
> list in this case.
> 
[Bala]: will find the correct POC.


>> ---
>> 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       | 31 
>> +++++++++++++++++--
>>  1 file changed, 29 insertions(+), 2 deletions(-)
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt 
>> b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> index 0ea18a53cc29..c4590bc87390 100644
>> --- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
>> @@ -10,12 +10,24 @@ device the slave device is attached to.
>>  Required properties:
>>   - compatible: should contain one of the following:
>>     * "qcom,qca6174-bt"
>> +   * "qcom,wcn3990-bt"
>> 
>>  Optional properties:
>>   - enable-gpios: gpio specifier used to enable chip
>>   - clocks: clock provided to the controller (SUSCLK_32KHZ)
>> -
>> -Example:
>> + - vddpa-supply: Bluetooth VDD PA regulator handle
>> + - vddio-supply: Bluetooth VDD IO regulator handle
>> + - vddldo-supply: Bluetooth VDD LDO regulator handle. Kept under 
>> optional
>> +                 parameters as some of the chipsets doesn't require 
>> ldo or
>> +                 it may use from same vddio.
> 
> You need to be explicit as to which properties apply or don't apply to
> specific compatible strings.
> 

[Bala]: will update.

> Supplies should be based on inputs to the chip, so if 1 regulator
> supplies 2 inputs you still need both.
> 
[Bala]:  sorry i did't get you, all these regulators are single output 
line regulators.
          i.e. <supply-name>- supply, need single regulator input as a 
handle.
          and these line regulators generate fixed voltage output.
          please let me know, if i have cleared your query

>> + - vddxtal-supply: Bluetooth VDD XTAL regulator handle
>> + - vddcore-supply: Bluetooth VDD CORE regulator handle
>> + - vddpwd-supply: Chip power down gpio is required when bluetooth 
>> module
>> +                 and other modules like wifi co-exist in a singe chip 
>> and
>> +                 shares a common gpio to bring chip out of reset.
>> + - max-speed: operating speed of the chip.
>> +
>> +Examples:
>> 
>>  serial@7570000 {
>>         label = "BT-UART";
>> @@ -28,3 +40,18 @@ serial@7570000 {
>>                 clocks = <&divclk4>;
>>         };
>>  };
>> +
>> +serial@898000 {
>> +       label = "BT-UART";
>> +       status = "okay";
>> +
>> +       bluetooth: bt_wcn3990 {
> 
> Reverse these. The node name should be "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
>> 

Thanks for reviewing, will send a incremental patch.

-- 
Regards
Balakrishna.

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

[Rob Herring]

On Wed, Jun 20, 2018 at 5:33 AM, Balakrishna Godavarthi
<bgodavar@codeaurora.org> wrote:
> Hi Rob,
>
> please find my comments inline.
>
> On 2018-06-18 18:59, Rob Herring wrote:
>>
>> On Sat, Jun 16, 2018 at 12:27 AM, Balakrishna Godavarthi
>> <bgodavar@codeaurora.org> wrote:
>>>
>>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>

[...]

>>> -Example:
>>> + - vddpa-supply: Bluetooth VDD PA regulator handle
>>> + - vddio-supply: Bluetooth VDD IO regulator handle
>>> + - vddldo-supply: Bluetooth VDD LDO regulator handle. Kept under
>>> optional
>>> +                 parameters as some of the chipsets doesn't require ldo
>>> or
>>> +                 it may use from same vddio.
>>
>>
>> You need to be explicit as to which properties apply or don't apply to
>> specific compatible strings.
>>
>
> [Bala]: will update.
>
>> Supplies should be based on inputs to the chip, so if 1 regulator
>> supplies 2 inputs you still need both.
>>
> [Bala]:  sorry i did't get you, all these regulators are single output line
> regulators.
>          i.e. <supply-name>- supply, need single regulator input as a
> handle.
>          and these line regulators generate fixed voltage output.
>          please let me know, if i have cleared your query

I was referring to "it may use from same vddio". If you mean vddldo is
left disconnected, then omitting the property is fine. If you mean
vddio and vddldo are connected to the same regulator, then you should
have both properties in the binding.

Rob

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

[Balakrishna Godavarthi]

Hi Rob,

On 2018-06-20 20:24, Rob Herring wrote:
> On Wed, Jun 20, 2018 at 5:33 AM, Balakrishna Godavarthi
> <bgodavar@codeaurora.org> wrote:
>> Hi Rob,
>> 
>> please find my comments inline.
>> 
>> On 2018-06-18 18:59, Rob Herring wrote:
>>> 
>>> On Sat, Jun 16, 2018 at 12:27 AM, Balakrishna Godavarthi
>>> <bgodavar@codeaurora.org> wrote:
>>>> 
>>>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> 
> [...]
> 
>>>> -Example:
>>>> + - vddpa-supply: Bluetooth VDD PA regulator handle
>>>> + - vddio-supply: Bluetooth VDD IO regulator handle
>>>> + - vddldo-supply: Bluetooth VDD LDO regulator handle. Kept under
>>>> optional
>>>> +                 parameters as some of the chipsets doesn't require 
>>>> ldo
>>>> or
>>>> +                 it may use from same vddio.
>>> 
>>> 
>>> You need to be explicit as to which properties apply or don't apply 
>>> to
>>> specific compatible strings.
>>> 
>> 
>> [Bala]: will update.
>> 
>>> Supplies should be based on inputs to the chip, so if 1 regulator
>>> supplies 2 inputs you still need both.
>>> 
>> [Bala]:  sorry i did't get you, all these regulators are single output 
>> line
>> regulators.
>>          i.e. <supply-name>- supply, need single regulator input as a
>> handle.
>>          and these line regulators generate fixed voltage output.
>>          please let me know, if i have cleared your query
> 
> I was referring to "it may use from same vddio". If you mean vddldo is
> left disconnected, then omitting the property is fine. If you mean
> vddio and vddldo are connected to the same regulator, then you should
> have both properties in the binding.
> 
> Rob

  may be pharse is confusing, will correct the text.
  we require both vregs to turn on wcn3990.

-- 
Regards
Balakrishna.

Re: [PATCH v7 5/8] Bluetooth: hci_qca: Defined wrapper functions for setting UART speeds

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-06-19 12:41, Balakrishna Godavarthi wrote:
> HI Matthias,
> 
> Please find my comments in line.
> 
> On 2018-06-19 03:21, Matthias Kaehlcke wrote:
>> On Sat, Jun 16, 2018 at 11:57:15AM +0530, Balakrishna Godavarthi 
>> wrote:
>>> Subject: Bluetooth: hci_qca: Defined wrapper functions for setting
>>>          UART speeds
>> 
>> s/Defined/Define/
>> 
>> or
>> 
>> s/Defined/Add/
>> 
> [Bala]: will update the text.
>>> 
>>> In 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 v7:
>>> 	* initial patch
>>> 	* created wrapper functions for init and operating speeds.
>>> 
>>> ---
>>>  drivers/bluetooth/hci_qca.c | 64 
>>> ++++++++++++++++++++++++++-----------
>>>  1 file changed, 45 insertions(+), 19 deletions(-)
>>> 
>>> diff --git a/drivers/bluetooth/hci_qca.c 
>>> b/drivers/bluetooth/hci_qca.c
>>> index fe62420ef838..3e09c6223baf 100644
>>> --- a/drivers/bluetooth/hci_qca.c
>>> +++ b/drivers/bluetooth/hci_qca.c
>>> @@ -923,21 +923,10 @@ static inline void host_set_baudrate(struct 
>>> hci_uart *hu, unsigned int speed)
>>>  		hci_uart_set_baudrate(hu, speed);
>>>  }
>>> 
>>> -static int qca_setup(struct hci_uart *hu)
>>> +static void qca_set_init_speed(struct hci_uart *hu)
>>>  {
>>> -	struct hci_dev *hdev = hu->hdev;
>>> -	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");
>>> -
>>> -	/* Patch downloading has to be done without IBS mode */
>>> -	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>>> +	unsigned int speed = 0;
>>> 
>>> -	/* Setup initial baudrate */
>>> -	speed = 0;
>>>  	if (hu->init_speed)
>>>  		speed = hu->init_speed;
>>>  	else if (hu->proto->init_speed)
>>> @@ -945,27 +934,64 @@ static int qca_setup(struct hci_uart *hu)
>>> 
>>>  	if (speed)
>>>  		host_set_baudrate(hu, speed);
>>> +}
>>> +
>>> +static int qca_get_oper_speed(struct hci_uart *hu)
>> 
>> Return type should be unsigned int.
>> 
> [Bala]: will update
>>> +{
>>> +	unsigned int speed = 0;
>>> 
>>> -	/* 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;
>>> 
>>> +	return speed;
>>> +}
>>> +
>>> +static int qca_set_oper_speed(struct hci_uart *hu)
>>> +{
>>> +	unsigned int speed = 0, qca_baudrate;
>> 
>> initialization is not needed.
>> 
> [Bala]: will update
> 
>>> +	int ret = 0;
>>> +
>>> +	speed = qca_get_oper_speed(hu);
>>>  	if (speed) {
>> 
>> nit:
>> 	if (!speed)
>> 		return 0;
>> 
>> 
> [Bala]: will update.
> 
>>>  		qca_baudrate = qca_get_baudrate_value(speed);
>>> -
>>> -		bt_dev_info(hdev, "Set UART speed to %d", speed);
>>> -		ret = qca_set_baudrate(hdev, qca_baudrate);
>>> +		bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
>>> +		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
>>>  		if (ret) {
>>> -			bt_dev_err(hdev, "Failed to change the baud rate (%d)",
>>> +			bt_dev_err(hu->hdev, "Failed to change the baud rate (%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;
>>> +	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");
>>> +
>>> +	/* Patch downloading has to be done without IBS mode */
>>> +	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>>> +
>>> +	/* Setup initial baudrate */
>>> +	qca_set_init_speed(hu);
>>> +	/* Setup user speed if needed */
>>> +	ret = qca_set_oper_speed(hu);
>>> +	if (ret)
>>> +		return ret;
>>> +	speed = qca_get_oper_speed(hu);
>>> +	if (speed)
>>> +		qca_baudrate = qca_get_baudrate_value(speed);
>> 
>> nit: the sequence
>> 
>> qca_set_oper_speed()
>> qca_get_oper_speed()
>> 
>> is slightly awkward to read. This could be avoided if the speed was
>> passed as parameter, though I understand this isn't done for symmetry
>> with qca_set_init_speed(). An alternative could be:
>> 
>> static int qca_set_speed(struct hci_uart *hu, unsigned int speed, enum
>>     qca_speed_type speed_type)
>> {
>> 	unsigned int qca_baudrate;
>> 
>> 	if (speed_type == QCA_OPER_SPEED) {
>> 		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 baud rate (%d)",
>> 				ret);
>> 			return ret;
>> 		}
>> 	}
>> 
>> 	host_set_baudrate(hu, speed);
>> 
>> 	/* If the order doesn't matter set the host baudrate first and
>> 	   return if speed_type != QCA_OPER_SPEED */
>> 
>> 	return 0;
>> }
>> 
>> static int qca_setup(struct hci_uart *hu)
>> {
>> 	...
>> 	speed = qca_get_init_speed(hu);
>> 	if (speed)
>> 		qca_set_speed(hu, speed, QCA_INIT_SPEED);
>> 
>> 	speed = qca_get_oper_speed(hu);
>> 	if (speed) {
>> 	   	qca_set_speed(hu, speed, QCA_OPER_SPEED);
>> 		qca_baudrate = qca_get_baudrate_value(speed);
>> 	}
>> 	...
>> }
>> 
>> Just a suggestion, ok for me if you prefer to keep it as is.
> 
> [Bala]: will study and update the same.

[Bala]: you are suggestion looks ok to me. i am thinking to optimize 
further.

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, unsigned int speed,
                          enum qca_speed_type speed_type)
{
         unsigned int qca_baudrate;
         int ret;

         if (speed_type == QCA_OPER_SPEED) {
                 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)
  {
  	...
     /* Setup initial baudrate */
         speed = qca_get_speed(hu, QCA_INIT_SPEED);
         if (speed)
                 qca_set_speed(hu, speed, QCA_INIT_SPEED);

         /* 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);
         }

}

will have two functions,
qca_set_speed(): for setting speed
qca_get_speed(): to get the speed.

is this preferable?

-- 
Regards
Balakrishna.

Re: [PATCH v7 5/8] Bluetooth: hci_qca: Defined wrapper functions for setting UART speeds

[Matthias Kaehlcke]

Hi Balakrishna,

On Thu, Jun 21, 2018 at 01:19:00AM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-06-19 12:41, Balakrishna Godavarthi wrote:
> > HI Matthias,
> > 
> > Please find my comments in line.
> > 
> > On 2018-06-19 03:21, Matthias Kaehlcke wrote:
> > > On Sat, Jun 16, 2018 at 11:57:15AM +0530, Balakrishna Godavarthi
> > > wrote:
> > > > Subject: Bluetooth: hci_qca: Defined wrapper functions for setting
> > > >          UART speeds
> > > 
> > > s/Defined/Define/
> > > 
> > > or
> > > 
> > > s/Defined/Add/
> > > 
> > [Bala]: will update the text.
> > > > 
> > > > In 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 v7:
> > > > 	* initial patch
> > > > 	* created wrapper functions for init and operating speeds.
> > > > 
> > > > ---
> > > >  drivers/bluetooth/hci_qca.c | 64
> > > > ++++++++++++++++++++++++++-----------
> > > >  1 file changed, 45 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/bluetooth/hci_qca.c
> > > > b/drivers/bluetooth/hci_qca.c
> > > > index fe62420ef838..3e09c6223baf 100644
> > > > --- a/drivers/bluetooth/hci_qca.c
> > > > +++ b/drivers/bluetooth/hci_qca.c
> > > > @@ -923,21 +923,10 @@ static inline void
> > > > host_set_baudrate(struct hci_uart *hu, unsigned int speed)
> > > >  		hci_uart_set_baudrate(hu, speed);
> > > >  }
> > > > 
> > > > -static int qca_setup(struct hci_uart *hu)
> > > > +static void qca_set_init_speed(struct hci_uart *hu)
> > > >  {
> > > > -	struct hci_dev *hdev = hu->hdev;
> > > > -	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");
> > > > -
> > > > -	/* Patch downloading has to be done without IBS mode */
> > > > -	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> > > > +	unsigned int speed = 0;
> > > > 
> > > > -	/* Setup initial baudrate */
> > > > -	speed = 0;
> > > >  	if (hu->init_speed)
> > > >  		speed = hu->init_speed;
> > > >  	else if (hu->proto->init_speed)
> > > > @@ -945,27 +934,64 @@ static int qca_setup(struct hci_uart *hu)
> > > > 
> > > >  	if (speed)
> > > >  		host_set_baudrate(hu, speed);
> > > > +}
> > > > +
> > > > +static int qca_get_oper_speed(struct hci_uart *hu)
> > > 
> > > Return type should be unsigned int.
> > > 
> > [Bala]: will update
> > > > +{
> > > > +	unsigned int speed = 0;
> > > > 
> > > > -	/* 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;
> > > > 
> > > > +	return speed;
> > > > +}
> > > > +
> > > > +static int qca_set_oper_speed(struct hci_uart *hu)
> > > > +{
> > > > +	unsigned int speed = 0, qca_baudrate;
> > > 
> > > initialization is not needed.
> > > 
> > [Bala]: will update
> > 
> > > > +	int ret = 0;
> > > > +
> > > > +	speed = qca_get_oper_speed(hu);
> > > >  	if (speed) {
> > > 
> > > nit:
> > > 	if (!speed)
> > > 		return 0;
> > > 
> > > 
> > [Bala]: will update.
> > 
> > > >  		qca_baudrate = qca_get_baudrate_value(speed);
> > > > -
> > > > -		bt_dev_info(hdev, "Set UART speed to %d", speed);
> > > > -		ret = qca_set_baudrate(hdev, qca_baudrate);
> > > > +		bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
> > > > +		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
> > > >  		if (ret) {
> > > > -			bt_dev_err(hdev, "Failed to change the baud rate (%d)",
> > > > +			bt_dev_err(hu->hdev, "Failed to change the baud rate (%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;
> > > > +	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");
> > > > +
> > > > +	/* Patch downloading has to be done without IBS mode */
> > > > +	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> > > > +
> > > > +	/* Setup initial baudrate */
> > > > +	qca_set_init_speed(hu);
> > > > +	/* Setup user speed if needed */
> > > > +	ret = qca_set_oper_speed(hu);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +	speed = qca_get_oper_speed(hu);
> > > > +	if (speed)
> > > > +		qca_baudrate = qca_get_baudrate_value(speed);
> > > 
> > > nit: the sequence
> > > 
> > > qca_set_oper_speed()
> > > qca_get_oper_speed()
> > > 
> > > is slightly awkward to read. This could be avoided if the speed was
> > > passed as parameter, though I understand this isn't done for symmetry
> > > with qca_set_init_speed(). An alternative could be:
> > > 
> > > static int qca_set_speed(struct hci_uart *hu, unsigned int speed, enum
> > >     qca_speed_type speed_type)
> > > {
> > > 	unsigned int qca_baudrate;
> > > 
> > > 	if (speed_type == QCA_OPER_SPEED) {
> > > 		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 baud rate (%d)",
> > > 				ret);
> > > 			return ret;
> > > 		}
> > > 	}
> > > 
> > > 	host_set_baudrate(hu, speed);
> > > 
> > > 	/* If the order doesn't matter set the host baudrate first and
> > > 	   return if speed_type != QCA_OPER_SPEED */
> > > 
> > > 	return 0;
> > > }
> > > 
> > > static int qca_setup(struct hci_uart *hu)
> > > {
> > > 	...
> > > 	speed = qca_get_init_speed(hu);
> > > 	if (speed)
> > > 		qca_set_speed(hu, speed, QCA_INIT_SPEED);
> > > 
> > > 	speed = qca_get_oper_speed(hu);
> > > 	if (speed) {
> > > 	   	qca_set_speed(hu, speed, QCA_OPER_SPEED);
> > > 		qca_baudrate = qca_get_baudrate_value(speed);
> > > 	}
> > > 	...
> > > }
> > > 
> > > Just a suggestion, ok for me if you prefer to keep it as is.
> > 
> > [Bala]: will study and update the same.
> 
> [Bala]: you are suggestion looks ok to me. i am thinking to optimize
> further.
> 
> 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, unsigned int speed,
>                          enum qca_speed_type speed_type)
> {
>         unsigned int qca_baudrate;
>         int ret;
> 
>         if (speed_type == QCA_OPER_SPEED) {
>                 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);

nit: if the order doesn't matter consider setting the host baudrate
first and return if 'speed_type != QCA_OPER_SPEED', which reduces the
indentiation (just a minor not really important thing though).

I also see a point in not setting the host baudrate if the chip
baudrate couldn't be set, so whatever ;-)

> 
>         return ret;
> }
> 
>  static int qca_setup(struct hci_uart *hu)
>  {
>  	...
>     /* Setup initial baudrate */
>         speed = qca_get_speed(hu, QCA_INIT_SPEED);
>         if (speed)
>                 qca_set_speed(hu, speed, QCA_INIT_SPEED);
> 
>         /* 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);
>         }
> 
> }
> 
> will have two functions,
> qca_set_speed(): for setting speed
> qca_get_speed(): to get the speed.
> 
> is this preferable?

Looks good to me, thanks!

Re: [PATCH v7 4/8] Bluetooth: btqca: Redefine qca_uart_setup() to generic function.

[Matthias Kaehlcke]

Hi Balakrishna,

On Wed, Jun 20, 2018 at 04:23:25PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-06-20 01:33, Matthias Kaehlcke wrote:
> > On Tue, Jun 19, 2018 at 12:39:07PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > > 
> > > Please find my comments inline.
> > > 
> > > On 2018-06-19 02:49, Matthias Kaehlcke wrote:
> > > > On Sat, Jun 16, 2018 at 11:57:14AM +0530, Balakrishna Godavarthi wrote:
> > > >
> > > > > @@ -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);
> > > > >  	}
> > > >
> > > > Why is this needed? A caller that passes in soc_ver = 0 could just
> > > > call qca_read_soc_version() themselves.
> > > [Bala]: hci_qca support two chip one ROME (already up-streamed) and
> > > other
> > > wcn3990.
> > >         there setup sequence differs.
> > > 
> > >         wcn3900:
> > >         1. set baudrate to 115200
> > >         2. read soc version
> > >         3. set baudarte to 3.2Mbps
> > >         4. download firmware. (calls qca_uart_setup)
> > > 
> > >        ROME:
> > >        1. set baudrate to 3.0 mbps
> > >        2. calls qca_uart_setup to read soc version and download
> > > firmware.
> > > 
> > >       so to make use of existing function qca_setup_uart(). passing
> > > soc_ver
> > > as argument.
> > >       if soc_ver is zero, then read soc version (i.e. for ROME chip).
> > > 
> > >       Pls let me know, if you have any queries.
> > 
> > I don't really understand this argumentation. Let's look at the code
> > at the end of this series, where both Rome and wcn3900 are supported:
> > 
> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1059
> > 
> > The version could be read (for Rome only) after setting the operating
> > speed:
> 
> [Bala]: yes we can read SoC version after setting operating baud rate. an
> advice from SoC vendor that, read SoC version before setting operating
> speed.
>         one advantage from reading SoC version before setting operating
> baudrate,it will helps us to understand whether the soc is responding to any
> commands in default baud rate
>         before setting operating speed.

Is the recommendation for Rome or wcn3990? I was referring to Rome and
the current code sets the operating speed and then reads the SoC
version (inside qca_uart_setup())

> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1111
> > 
> > I concede that having two "if (qcadev->btsoc_type == XYZ)" branches in
> > qca_setup() is kinda ugly, but I think it's still better than the
> > conditional call of qca_read_soc_version() in qca_uart_setup().
> 
> [Bala]: we require an if-else block  for soc type.As wcn3900 support
> hardware flow control where as ROME is not supporting hardware flow control.

Since you mention if-else and flow control I'm not sure if you
understood correctly what I suggested. My suggestion is to add
something like this:

if (qcadev->btsoc_type == QCA_ROME) {
	ret = qca_read_soc_version(hdev, &soc_ver);
	if (ret < 0 || soc_ver == 0) {
	   	// Note mka@: we might want to skip this log,
		// qca_read_soc_version() already logs in all error paths
		bt_dev_err(hdev, "Failed to get version %d", ret);
		return ret;
	}
}

right here:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1111

which is functionally equivalent to the current code.

You could even do the error check in the common code (just checking
for 'soc_ver == 0', assuming that qca_read_soc_version() doesn't set
it to something different in case of error).

Anyway, I won't insist if you prefer it as is and Marcel is ok with it.

Re: [PATCH v7 4/8] Bluetooth: btqca: Redefine qca_uart_setup() to generic function.

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-06-21 05:03, Matthias Kaehlcke wrote:
> Hi Balakrishna,
> 
> On Wed, Jun 20, 2018 at 04:23:25PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-06-20 01:33, Matthias Kaehlcke wrote:
>> > On Tue, Jun 19, 2018 at 12:39:07PM +0530, Balakrishna Godavarthi wrote:
>> > > Hi Matthias,
>> > >
>> > > Please find my comments inline.
>> > >
>> > > On 2018-06-19 02:49, Matthias Kaehlcke wrote:
>> > > > On Sat, Jun 16, 2018 at 11:57:14AM +0530, Balakrishna Godavarthi wrote:
>> > > >
>> > > > > @@ -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);
>> > > > >  	}
>> > > >
>> > > > Why is this needed? A caller that passes in soc_ver = 0 could just
>> > > > call qca_read_soc_version() themselves.
>> > > [Bala]: hci_qca support two chip one ROME (already up-streamed) and
>> > > other
>> > > wcn3990.
>> > >         there setup sequence differs.
>> > >
>> > >         wcn3900:
>> > >         1. set baudrate to 115200
>> > >         2. read soc version
>> > >         3. set baudarte to 3.2Mbps
>> > >         4. download firmware. (calls qca_uart_setup)
>> > >
>> > >        ROME:
>> > >        1. set baudrate to 3.0 mbps
>> > >        2. calls qca_uart_setup to read soc version and download
>> > > firmware.
>> > >
>> > >       so to make use of existing function qca_setup_uart(). passing
>> > > soc_ver
>> > > as argument.
>> > >       if soc_ver is zero, then read soc version (i.e. for ROME chip).
>> > >
>> > >       Pls let me know, if you have any queries.
>> >
>> > I don't really understand this argumentation. Let's look at the code
>> > at the end of this series, where both Rome and wcn3900 are supported:
>> >
>> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1059
>> >
>> > The version could be read (for Rome only) after setting the operating
>> > speed:
>> 
>> [Bala]: yes we can read SoC version after setting operating baud rate. 
>> an
>> advice from SoC vendor that, read SoC version before setting operating
>> speed.
>>         one advantage from reading SoC version before setting 
>> operating
>> baudrate,it will helps us to understand whether the soc is responding 
>> to any
>> commands in default baud rate
>>         before setting operating speed.
> 
> Is the recommendation for Rome or wcn3990? I was referring to Rome and
> the current code sets the operating speed and then reads the SoC
> version (inside qca_uart_setup())
> 

[Bala]: my statement is wrt to wcn3990. yes, ROME will set operating 
speed and read version.

>> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1111
>> >
>> > I concede that having two "if (qcadev->btsoc_type == XYZ)" branches in
>> > qca_setup() is kinda ugly, but I think it's still better than the
>> > conditional call of qca_read_soc_version() in qca_uart_setup().
>> 
>> [Bala]: we require an if-else block  for soc type.As wcn3900 support
>> hardware flow control where as ROME is not supporting hardware flow 
>> control.
> 
> Since you mention if-else and flow control I'm not sure if you
> understood correctly what I suggested. My suggestion is to add
> something like this:
> 
> if (qcadev->btsoc_type == QCA_ROME) {
> 	ret = qca_read_soc_version(hdev, &soc_ver);
> 	if (ret < 0 || soc_ver == 0) {
> 	   	// Note mka@: we might want to skip this log,
> 		// qca_read_soc_version() already logs in all error paths
> 		bt_dev_err(hdev, "Failed to get version %d", ret);
> 		return ret;
> 	}
> }
> 
> right here:
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1111
> 
> which is functionally equivalent to the current code.
> 
> You could even do the error check in the common code (just checking
> for 'soc_ver == 0', assuming that qca_read_soc_version() doesn't set
> it to something different in case of error).
> 
> Anyway, I won't insist if you prefer it as is and Marcel is ok with it.

[Bala]: thanks for clarifying my doubt, but if remove the 
qca_read_soc_version() from qca_uart_setup() and handle it qca_setup().
         in that case, we will have common blocks of code, when we 
integrate wcn3990 into existing qca_setup()

        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 */
         speed = qca_get_speed(hu, QCA_INIT_SPEED);
         if (speed)
                 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) {
                 /*
       ...
       }


-- 
Regards
Balakrishna.

Re: [PATCH v7 8/8] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-06-20 03:23, Matthias Kaehlcke wrote:
> On Sat, Jun 16, 2018 at 11:57:18AM +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>
>> ---
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 28ae6a17a595..1961e313aae7 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -402,6 +441,7 @@ static int qca_open(struct hci_uart *hu)
>>  {
>>  	struct qca_serdev *qcadev;
>>  	struct qca_data *qca;
>> +	int ret = 0;
>> 
>>  	BT_DBG("hu %p qca_open", hu);
>> 
>> @@ -463,13 +503,19 @@ static int qca_open(struct hci_uart *hu)
>>  		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) {
>> +			hu->init_speed = qcadev->init_speed;
>> +			hu->oper_speed = qcadev->oper_speed;
>> +			ret = qca_btsoc_power_setup(hu, true);
> 
> Better do this before starting the timers, otherwise you need to take
> care of stopping them in case of failure.
> 
> If qca_btsoc_power_setup() fails you also have to free
> 'qca->workqueue' and 'qca'.
> 

[Bala]: i missed this.will update

>> +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 skb packet");
> 
> I was told that custom OOM messages should not be used, since the
> mm code will complain loudly in this case.
> 
>> +static int qca_btsoc_shutdown(struct hci_uart *hu)
> 
> The return value is not evaluated by the only caller, so this should
> probably be void.
> 

[Bala]: will update

>>  static int qca_setup(struct hci_uart *hu)
>>  {
>>  	struct hci_dev *hdev = hu->hdev;
>>  	struct qca_data *qca = hu->priv;
>> +	struct qca_serdev *qcadev;
>>  	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
>>  	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);
>> -
>> -	/* Setup initial baudrate */
>>  	qca_set_init_speed(hu);
>> +
>> +	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, CHEROKEE_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);
>> +		qca_set_init_speed(hu);
>> +		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);
> 
> serdev_device_close() ?
> 
> Also applies to other error paths in this function.
> 
[Bala]: sorry, i didn't get you.

>>  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);
>> @@ -1041,47 +1264,85 @@ static int qca_serdev_probe(struct 
>> serdev_device *serdev)
>>  		return -ENOMEM;
>> 
>>  	qcadev->serdev_hu.serdev = serdev;
>> +	data = of_device_get_match_data(&serdev->dev);
>> +	if (data && data->soc_type == QCA_WCN3990)
>> +		qcadev->btsoc_type = QCA_WCN3990;
>> +	else
>> +		qcadev->btsoc_type = QCA_ROME;
>> +
>>  	serdev_device_set_drvdata(serdev, qcadev);
>> +	if (qcadev->btsoc_type == QCA_WCN3990) {
> 
> nit: the double "if (soc_type == QCA_WCN3990)" is a bit odd. Consider
> changing this condition to "if (data && data->soc_type == QCA_WCN3990)"
> and assign qcadev->btsoc_type in the corresponding branch.

[bala]: will update.

I have idea of removing flow control from qca_setup() and use them in 
qca_set_speed()
"i need to disable hardware flow control when sending change baudrate 
request to WCN3990.
enabling it after setting host baudrate. this is only for wcn3990.
"

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

         if (speed_type == QCA_INIT_SPEED)
                 goto change_host_baudrate;

         if (soc_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;
         }

change_host_baudrate:

         host_set_baudrate(hu, speed);
         if (soc_type == QCA_WCN3990)
                 hci_uart_set_flow_control(hu, false);

         return ret;
}


is it good idea?
-- 
Regards
Balakrishna.

Re: [PATCH v7 8/8] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990

[Matthias Kaehlcke]

Hi Balakrishna,

On Thu, Jun 21, 2018 at 07:30:25PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-06-20 03:23, Matthias Kaehlcke wrote:
> > On Sat, Jun 16, 2018 at 11:57:18AM +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>
> > > ---
> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > index 28ae6a17a595..1961e313aae7 100644
> > > --- a/drivers/bluetooth/hci_qca.c
> > > +++ b/drivers/bluetooth/hci_qca.c
> > >
> > >  static int qca_setup(struct hci_uart *hu)
> > >  {
> > >  	struct hci_dev *hdev = hu->hdev;
> > >  	struct qca_data *qca = hu->priv;
> > > +	struct qca_serdev *qcadev;
> > >  	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
> > >  	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);
> > > -
> > > -	/* Setup initial baudrate */
> > >  	qca_set_init_speed(hu);
> > > +
> > > +	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, CHEROKEE_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);
> > > +		qca_set_init_speed(hu);
> > > +		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);
> > 
> > serdev_device_close() ?
> > 
> > Also applies to other error paths in this function.
> > 
> [Bala]: sorry, i didn't get you.

A few lines above serdev_device_open() is called, in the error paths
the device should be closed.

> > >  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);
> > > @@ -1041,47 +1264,85 @@ static int qca_serdev_probe(struct
> > > serdev_device *serdev)
> > >  		return -ENOMEM;
> > > 
> > >  	qcadev->serdev_hu.serdev = serdev;
> > > +	data = of_device_get_match_data(&serdev->dev);
> > > +	if (data && data->soc_type == QCA_WCN3990)
> > > +		qcadev->btsoc_type = QCA_WCN3990;
> > > +	else
> > > +		qcadev->btsoc_type = QCA_ROME;
> > > +
> > >  	serdev_device_set_drvdata(serdev, qcadev);
> > > +	if (qcadev->btsoc_type == QCA_WCN3990) {
> > 
> > nit: the double "if (soc_type == QCA_WCN3990)" is a bit odd. Consider
> > changing this condition to "if (data && data->soc_type == QCA_WCN3990)"
> > and assign qcadev->btsoc_type in the corresponding branch.
> 
> [bala]: will update.
> 
> I have idea of removing flow control from qca_setup() and use them in
> qca_set_speed()
> "i need to disable hardware flow control when sending change baudrate
> request to WCN3990.
> enabling it after setting host baudrate. this is only for wcn3990.
> "

I definitely think it's positive to hide the
hci_uart_set_flow_control() call in qca_set_speed(). A few comments
inline.


> static int qca_set_speed(struct hci_uart *hu, unsigned int speed,
>                          enum qca_speed_type speed_type,
>                          enum qca_btsoc_type soc_type)
> {

That's a lot of parameters, in particular 'soc_type' seems a bit
off-topic in a function called qca_set_speed(). Passing struct
qca_serdev instead of struct hci_uart would make the 'soc_type'
parameter unnecessary.

>         unsigned int qca_baudrate;
>         int ret;
> 
>         if (speed_type == QCA_INIT_SPEED)
>                 goto change_host_baudrate;

If the order of setting host and chip baudrate can't be changed I
think it's preferable to set the init speed right here in the if
branch instead of doing the goto. I earlier mentioned the short cut of
a return (if possible) to save a level of indentation, but a goto
IMO doesn't improve readability and it's the first level of
indentation anyway.

>         if (soc_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;
>         }
> 
> change_host_baudrate:
> 
>         host_set_baudrate(hu, speed);
>         if (soc_type == QCA_WCN3990)
>                 hci_uart_set_flow_control(hu, false);
> 
>         return ret;
> }
> 
> 
> is it good idea?

In general I think it is.

Re: [PATCH v7 4/8] Bluetooth: btqca: Redefine qca_uart_setup() to generic function.

[Matthias Kaehlcke]

On Thu, Jun 21, 2018 at 04:50:28PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-06-21 05:03, Matthias Kaehlcke wrote:
> > Hi Balakrishna,
> > 
> > On Wed, Jun 20, 2018 at 04:23:25PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > > 
> > > On 2018-06-20 01:33, Matthias Kaehlcke wrote:
> > > > On Tue, Jun 19, 2018 at 12:39:07PM +0530, Balakrishna Godavarthi wrote:
> > > > > Hi Matthias,
> > > > >
> > > > > Please find my comments inline.
> > > > >
> > > > > On 2018-06-19 02:49, Matthias Kaehlcke wrote:
> > > > > > On Sat, Jun 16, 2018 at 11:57:14AM +0530, Balakrishna Godavarthi wrote:
> > > > > >
> > > > > > > @@ -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);
> > > > > > >  	}
> > > > > >
> > > > > > Why is this needed? A caller that passes in soc_ver = 0 could just
> > > > > > call qca_read_soc_version() themselves.
> > > > > [Bala]: hci_qca support two chip one ROME (already up-streamed) and
> > > > > other
> > > > > wcn3990.
> > > > >         there setup sequence differs.
> > > > >
> > > > >         wcn3900:
> > > > >         1. set baudrate to 115200
> > > > >         2. read soc version
> > > > >         3. set baudarte to 3.2Mbps
> > > > >         4. download firmware. (calls qca_uart_setup)
> > > > >
> > > > >        ROME:
> > > > >        1. set baudrate to 3.0 mbps
> > > > >        2. calls qca_uart_setup to read soc version and download
> > > > > firmware.
> > > > >
> > > > >       so to make use of existing function qca_setup_uart(). passing
> > > > > soc_ver
> > > > > as argument.
> > > > >       if soc_ver is zero, then read soc version (i.e. for ROME chip).
> > > > >
> > > > >       Pls let me know, if you have any queries.
> > > >
> > > > I don't really understand this argumentation. Let's look at the code
> > > > at the end of this series, where both Rome and wcn3900 are supported:
> > > >
> > > > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1059
> > > >
> > > > The version could be read (for Rome only) after setting the operating
> > > > speed:
> > > 
> > > [Bala]: yes we can read SoC version after setting operating baud
> > > rate. an
> > > advice from SoC vendor that, read SoC version before setting operating
> > > speed.
> > >         one advantage from reading SoC version before setting
> > > operating
> > > baudrate,it will helps us to understand whether the soc is
> > > responding to any
> > > commands in default baud rate
> > >         before setting operating speed.
> > 
> > Is the recommendation for Rome or wcn3990? I was referring to Rome and
> > the current code sets the operating speed and then reads the SoC
> > version (inside qca_uart_setup())
> > 
> 
> [Bala]: my statement is wrt to wcn3990. yes, ROME will set operating speed
> and read version.
> 
> > > > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1111
> > > >
> > > > I concede that having two "if (qcadev->btsoc_type == XYZ)" branches in
> > > > qca_setup() is kinda ugly, but I think it's still better than the
> > > > conditional call of qca_read_soc_version() in qca_uart_setup().
> > > 
> > > [Bala]: we require an if-else block  for soc type.As wcn3900 support
> > > hardware flow control where as ROME is not supporting hardware flow
> > > control.
> > 
> > Since you mention if-else and flow control I'm not sure if you
> > understood correctly what I suggested. My suggestion is to add
> > something like this:
> > 
> > if (qcadev->btsoc_type == QCA_ROME) {
> > 	ret = qca_read_soc_version(hdev, &soc_ver);
> > 	if (ret < 0 || soc_ver == 0) {
> > 	   	// Note mka@: we might want to skip this log,
> > 		// qca_read_soc_version() already logs in all error paths
> > 		bt_dev_err(hdev, "Failed to get version %d", ret);
> > 		return ret;
> > 	}
> > }
> > 
> > right here:
> > 
> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1111
> > 
> > which is functionally equivalent to the current code.
> > 
> > You could even do the error check in the common code (just checking
> > for 'soc_ver == 0', assuming that qca_read_soc_version() doesn't set
> > it to something different in case of error).
> > 
> > Anyway, I won't insist if you prefer it as is and Marcel is ok with it.
> 
> [Bala]: thanks for clarifying my doubt, but if remove the
> qca_read_soc_version() from qca_uart_setup() and handle it qca_setup().
>         in that case, we will have common blocks of code, when we integrate
> wcn3990 into existing qca_setup()

Excellent!

>        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 */
>         speed = qca_get_speed(hu, QCA_INIT_SPEED);
>         if (speed)
>                 qca_set_speed(hu, speed, QCA_INIT_SPEED);

I said earlier that a _set_speed() followed by a _get_speed() is a bit
odd, and that it would be better to get the speed first and pass it as
a parameter. After looking again over the larger code I'm not
convinced anymore this is such a good idea, since the above pattern is
repeated multiple times, when it could be just:

qca_set_speed(hu, QCA_INIT_SPEED)

(+ error handling, which is also missing in the code above)

Just a thought, not asking you to change it back, do whatever seems
best to you.

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

It seems setting 'qca_baudrate' could also be done in the common
path. Would require an extra qca_get_speed(hu, QCA_OPER_SPEED) call,
in exchange this would be done in a single place closer to where
'qca_baudrate' is actually used.

Actually as it is now 'qca_baudrate' could remain uninitialized for
Rome if no operating speed is defined, which judging from
qca_get_baudrate_value() should result in QCA_BAUDRATE_115200.

>                 }
>         }
> 
>         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) {
>                 /*
>       ...
>       }

Overall looks good and should still become a bit more compact if the
handling of flow control is done in _set_speed() as you suggested in
"[PATCH v7 8/8] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth
chip wcn3990"

Re: [PATCH v7 8/8] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990

[Stephen Boyd]

Quoting Balakrishna Godavarthi (2018-06-18 10:07:31)
> Hi Stephen,
> =

> Please find my comments inline.
> =

> On 2018-06-18 22:12, Stephen Boyd wrote:
> > Quoting Balakrishna Godavarthi (2018-06-15 23:27:18)
> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> >> index 28ae6a17a595..1961e313aae7 100644
> >> --- a/drivers/bluetooth/hci_qca.c
> >> +++ b/drivers/bluetooth/hci_qca.c
> >> @@ -1031,9 +1145,118 @@ static struct hci_uart_proto qca_proto =3D {
> >>         .dequeue        =3D qca_dequeue,
> >>  };
> >> =

> >> +static const struct qca_vreg_data qca_cherokee_data =3D {
> >> +       .soc_type =3D QCA_WCN3990,
> >> +       .vregs =3D (struct qca_vreg []) {
> >> +               { "vddio",   1352000, 1352000,  0 },
> >> +               { "vddxtal", 1904000, 2040000,  0 },
> >> +               { "vddcore", 1800000, 1800000,  1 },
> >> +               { "vddpa",   1304000, 1304000,  1 },
> >> +               { "vddldo",  3000000, 3312000,  1 },
> > =

> > Load of 0 and 1 seems sort of odd. Are these made up to indicate "some
> > load" vs. "no load"?
> > =

> =

> [Bala]: this value specifies the output load current required to turn on =

> the wcn3990.
>          in struct defined vddio\vddxtal are smps, with fixed load.
>          regs from vddcore/vddpa/vddldo are programmable line regulators, =

> in which we need to set the basic load.

I got that part, but a load of 1 still seems like a nonsensical value.
Is it to workaround some issue with the regulator driver?

It's also pretty weird to be setting regulator voltages here in the
driver. Typically that's configured on the board and then the DT has the
right voltage already. The only case when the voltage may need to be set
by the driver is if the hardware can dynamically scale the voltage for
things like DVFS. It looks like here that the voltage is always fixed,
so probably we don't need to have any sort of voltage setting in this
driver and can rely on the DTS file to get things right.

Re: [PATCH v7 8/8] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-06-22 02:46, Matthias Kaehlcke wrote:
> Hi Balakrishna,
> 
> On Thu, Jun 21, 2018 at 07:30:25PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-06-20 03:23, Matthias Kaehlcke wrote:
>> > On Sat, Jun 16, 2018 at 11:57:18AM +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>
>> > > ---
>> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > > index 28ae6a17a595..1961e313aae7 100644
>> > > --- a/drivers/bluetooth/hci_qca.c
>> > > +++ b/drivers/bluetooth/hci_qca.c
>> > >
>> > >  static int qca_setup(struct hci_uart *hu)
>> > >  {
>> > >  	struct hci_dev *hdev = hu->hdev;
>> > >  	struct qca_data *qca = hu->priv;
>> > > +	struct qca_serdev *qcadev;
>> > >  	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
>> > >  	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);
>> > > -
>> > > -	/* Setup initial baudrate */
>> > >  	qca_set_init_speed(hu);
>> > > +
>> > > +	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, CHEROKEE_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);
>> > > +		qca_set_init_speed(hu);
>> > > +		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);
>> >
>> > serdev_device_close() ?
>> >
>> > Also applies to other error paths in this function.
>> >
>> [Bala]: sorry, i didn't get you.
> 
> A few lines above serdev_device_open() is called, in the error paths
> the device should be closed.
> 

[Bala]: i don't think closing of port is required.
         qca_send_vendor_cmd() fails if skb_alloc() fails. that is not 
linked with serdev_close().
         we need to close the port, if we have issues from reading or 
writing into the port.
         pls correct me if i am wrong.

>> > >  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);
>> > > @@ -1041,47 +1264,85 @@ static int qca_serdev_probe(struct
>> > > serdev_device *serdev)
>> > >  		return -ENOMEM;
>> > >
>> > >  	qcadev->serdev_hu.serdev = serdev;
>> > > +	data = of_device_get_match_data(&serdev->dev);
>> > > +	if (data && data->soc_type == QCA_WCN3990)
>> > > +		qcadev->btsoc_type = QCA_WCN3990;
>> > > +	else
>> > > +		qcadev->btsoc_type = QCA_ROME;
>> > > +
>> > >  	serdev_device_set_drvdata(serdev, qcadev);
>> > > +	if (qcadev->btsoc_type == QCA_WCN3990) {
>> >
>> > nit: the double "if (soc_type == QCA_WCN3990)" is a bit odd. Consider
>> > changing this condition to "if (data && data->soc_type == QCA_WCN3990)"
>> > and assign qcadev->btsoc_type in the corresponding branch.
>> 
>> [bala]: will update.
>> 
>> I have idea of removing flow control from qca_setup() and use them in
>> qca_set_speed()
>> "i need to disable hardware flow control when sending change baudrate
>> request to WCN3990.
>> enabling it after setting host baudrate. this is only for wcn3990.
>> "
> 
> I definitely think it's positive to hide the
> hci_uart_set_flow_control() call in qca_set_speed(). A few comments
> inline.
> 
> 
>> static int qca_set_speed(struct hci_uart *hu, unsigned int speed,
>>                          enum qca_speed_type speed_type,
>>                          enum qca_btsoc_type soc_type)
>> {
> 
> That's a lot of parameters, in particular 'soc_type' seems a bit
> off-topic in a function called qca_set_speed(). Passing struct
> qca_serdev instead of struct hci_uart would make the 'soc_type'
> parameter unnecessary.
> 

[Bala]: will update.

>>         unsigned int qca_baudrate;
>>         int ret;
>> 
>>         if (speed_type == QCA_INIT_SPEED)
>>                 goto change_host_baudrate;
> 
> If the order of setting host and chip baudrate can't be changed I
> think it's preferable to set the init speed right here in the if
> branch instead of doing the goto. I earlier mentioned the short cut of
> a return (if possible) to save a level of indentation, but a goto
> IMO doesn't improve readability and it's the first level of
> indentation anyway.
> 

[Bala]: will update.

>>         if (soc_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;
>>         }
>> 
>> change_host_baudrate:
>> 
>>         host_set_baudrate(hu, speed);
>>         if (soc_type == QCA_WCN3990)
>>                 hci_uart_set_flow_control(hu, false);
>> 
>>         return ret;
>> }
>> 
>> 
>> is it good idea?
> 
> In general I think it is.

-- 
Regards
Balakrishna.

Re: [PATCH v7 4/8] Bluetooth: btqca: Redefine qca_uart_setup() to generic function.

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-06-22 03:39, Matthias Kaehlcke wrote:
> On Thu, Jun 21, 2018 at 04:50:28PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-06-21 05:03, Matthias Kaehlcke wrote:
>> > Hi Balakrishna,
>> >
>> > On Wed, Jun 20, 2018 at 04:23:25PM +0530, Balakrishna Godavarthi wrote:
>> > > Hi Matthias,
>> > >
>> > > On 2018-06-20 01:33, Matthias Kaehlcke wrote:
>> > > > On Tue, Jun 19, 2018 at 12:39:07PM +0530, Balakrishna Godavarthi wrote:
>> > > > > Hi Matthias,
>> > > > >
>> > > > > Please find my comments inline.
>> > > > >
>> > > > > On 2018-06-19 02:49, Matthias Kaehlcke wrote:
>> > > > > > On Sat, Jun 16, 2018 at 11:57:14AM +0530, Balakrishna Godavarthi wrote:
>> > > > > >
>> > > > > > > @@ -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);
>> > > > > > >  	}
>> > > > > >
>> > > > > > Why is this needed? A caller that passes in soc_ver = 0 could just
>> > > > > > call qca_read_soc_version() themselves.
>> > > > > [Bala]: hci_qca support two chip one ROME (already up-streamed) and
>> > > > > other
>> > > > > wcn3990.
>> > > > >         there setup sequence differs.
>> > > > >
>> > > > >         wcn3900:
>> > > > >         1. set baudrate to 115200
>> > > > >         2. read soc version
>> > > > >         3. set baudarte to 3.2Mbps
>> > > > >         4. download firmware. (calls qca_uart_setup)
>> > > > >
>> > > > >        ROME:
>> > > > >        1. set baudrate to 3.0 mbps
>> > > > >        2. calls qca_uart_setup to read soc version and download
>> > > > > firmware.
>> > > > >
>> > > > >       so to make use of existing function qca_setup_uart(). passing
>> > > > > soc_ver
>> > > > > as argument.
>> > > > >       if soc_ver is zero, then read soc version (i.e. for ROME chip).
>> > > > >
>> > > > >       Pls let me know, if you have any queries.
>> > > >
>> > > > I don't really understand this argumentation. Let's look at the code
>> > > > at the end of this series, where both Rome and wcn3900 are supported:
>> > > >
>> > > > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1059
>> > > >
>> > > > The version could be read (for Rome only) after setting the operating
>> > > > speed:
>> > >
>> > > [Bala]: yes we can read SoC version after setting operating baud
>> > > rate. an
>> > > advice from SoC vendor that, read SoC version before setting operating
>> > > speed.
>> > >         one advantage from reading SoC version before setting
>> > > operating
>> > > baudrate,it will helps us to understand whether the soc is
>> > > responding to any
>> > > commands in default baud rate
>> > >         before setting operating speed.
>> >
>> > Is the recommendation for Rome or wcn3990? I was referring to Rome and
>> > the current code sets the operating speed and then reads the SoC
>> > version (inside qca_uart_setup())
>> >
>> 
>> [Bala]: my statement is wrt to wcn3990. yes, ROME will set operating 
>> speed
>> and read version.
>> 
>> > > > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1111
>> > > >
>> > > > I concede that having two "if (qcadev->btsoc_type == XYZ)" branches in
>> > > > qca_setup() is kinda ugly, but I think it's still better than the
>> > > > conditional call of qca_read_soc_version() in qca_uart_setup().
>> > >
>> > > [Bala]: we require an if-else block  for soc type.As wcn3900 support
>> > > hardware flow control where as ROME is not supporting hardware flow
>> > > control.
>> >
>> > Since you mention if-else and flow control I'm not sure if you
>> > understood correctly what I suggested. My suggestion is to add
>> > something like this:
>> >
>> > if (qcadev->btsoc_type == QCA_ROME) {
>> > 	ret = qca_read_soc_version(hdev, &soc_ver);
>> > 	if (ret < 0 || soc_ver == 0) {
>> > 	   	// Note mka@: we might want to skip this log,
>> > 		// qca_read_soc_version() already logs in all error paths
>> > 		bt_dev_err(hdev, "Failed to get version %d", ret);
>> > 		return ret;
>> > 	}
>> > }
>> >
>> > right here:
>> >
>> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1111
>> >
>> > which is functionally equivalent to the current code.
>> >
>> > You could even do the error check in the common code (just checking
>> > for 'soc_ver == 0', assuming that qca_read_soc_version() doesn't set
>> > it to something different in case of error).
>> >
>> > Anyway, I won't insist if you prefer it as is and Marcel is ok with it.
>> 
>> [Bala]: thanks for clarifying my doubt, but if remove the
>> qca_read_soc_version() from qca_uart_setup() and handle it 
>> qca_setup().
>>         in that case, we will have common blocks of code, when we 
>> integrate
>> wcn3990 into existing qca_setup()
> 
> Excellent!
> 
>>        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 */
>>         speed = qca_get_speed(hu, QCA_INIT_SPEED);
>>         if (speed)
>>                 qca_set_speed(hu, speed, QCA_INIT_SPEED);
> 
> I said earlier that a _set_speed() followed by a _get_speed() is a bit
> odd, and that it would be better to get the speed first and pass it as
> a parameter. After looking again over the larger code I'm not
> convinced anymore this is such a good idea, since the above pattern is
> repeated multiple times, when it could be just:
> 
> qca_set_speed(hu, QCA_INIT_SPEED)
> 
> (+ error handling, which is also missing in the code above)
> 
> Just a thought, not asking you to change it back, do whatever seems
> best to you.
> 
>>         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);
> 
> It seems setting 'qca_baudrate' could also be done in the common
> path. Would require an extra qca_get_speed(hu, QCA_OPER_SPEED) call,
> in exchange this would be done in a single place closer to where
> 'qca_baudrate' is actually used.
> 
> Actually as it is now 'qca_baudrate' could remain uninitialized for
> Rome if no operating speed is defined, which judging from
> qca_get_baudrate_value() should result in QCA_BAUDRATE_115200.
> 
>>                 }
>>         }
>> 
>>         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) {
>>                 /*
>>       ...
>>       }
> 
> Overall looks good and should still become a bit more compact if the
> handling of flow control is done in _set_speed() as you suggested in
> "[PATCH v7 8/8] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth
> chip wcn3990"

[Bala}: to over come calling qca_get_speed() for qca_baudrate.
         i prefer passing qca_baudrate as pointer argument to 
qca_set_speed().
         that  will actually helps to lower function calls. intern 
increase the speed of execution too.

code snippet after integrating wcn3990:

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

         if (speed_type == QCA_INIT_SPEED) {
                 speed = qca_get_speed(hu, QCA_INIT_SPEED);
                 if (speed) {
                         host_set_baudrate(hu, speed);
                         *qca_baudrate = qca_get_baudrate_value(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;
         }

         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)
{
  ....
  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, &qca_baudrate, 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;

                 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_baudrate, 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_baudrate, QCA_OPER_SPEED);
         if (ret)
                 return ret;
         /* 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) {
                 /*
                  * Userspace firmware loader will return -EAGAIN in case 
no
                  * patch/nvm-config is found, so run with original 
fw/config.
                  */
                 ret = 0;
         }

         /* Setup bdaddr */
         hu->hdev->set_bdaddr = qca_set_bdaddr_rome;

         return ret;

}


-- 
Regards
Balakrishna.

Re: [PATCH v7 8/8] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990

[Balakrishna Godavarthi]

Hi Stephen,

On 2018-06-22 06:58, Stephen Boyd wrote:
> Quoting Balakrishna Godavarthi (2018-06-18 10:07:31)
>> Hi Stephen,
>> 
>> Please find my comments inline.
>> 
>> On 2018-06-18 22:12, Stephen Boyd wrote:
>> > Quoting Balakrishna Godavarthi (2018-06-15 23:27:18)
>> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> >> index 28ae6a17a595..1961e313aae7 100644
>> >> --- a/drivers/bluetooth/hci_qca.c
>> >> +++ b/drivers/bluetooth/hci_qca.c
>> >> @@ -1031,9 +1145,118 @@ static struct hci_uart_proto qca_proto = {
>> >>         .dequeue        = qca_dequeue,
>> >>  };
>> >>
>> >> +static const struct qca_vreg_data qca_cherokee_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 },
>> >
>> > Load of 0 and 1 seems sort of odd. Are these made up to indicate "some
>> > load" vs. "no load"?
>> >
>> 
>> [Bala]: this value specifies the output load current required to turn 
>> on
>> the wcn3990.
>>          in struct defined vddio\vddxtal are smps, with fixed load.
>>          regs from vddcore/vddpa/vddldo are programmable line 
>> regulators,
>> in which we need to set the basic load.
> 
> I got that part, but a load of 1 still seems like a nonsensical value.
> Is it to workaround some issue with the regulator driver?
> 

[Bala]: i don't think, it is issue with regulator driver.

> It's also pretty weird to be setting regulator voltages here in the
> driver. Typically that's configured on the board and then the DT has 
> the
> right voltage already. The only case when the voltage may need to be 
> set
> by the driver is if the hardware can dynamically scale the voltage for
> things like DVFS. It looks like here that the voltage is always fixed,
> so probably we don't need to have any sort of voltage setting in this
> driver and can rely on the DTS file to get things right.


[Bala]: voltage required by wcn3990 are fixed. now i am using a 
programmable line regulators.
         in future, if i have an platform where we have a variable 
voltage regulators used.  i.e. an voltage can be scaled to the required 
voltage to wcn3990.
         in that case, we explicitly restrict the regulator output 
voltage to wcn3990 standards.

         let us take an example. i am using an programmable regulator, 
with varying output voltage ranges from 1v to 5v.
         but from wcn3990 we require an voltage 3.3v. while enabling the 
regulators.. so that output of regulator will be 3.3v fixed.

        one more case, every time when we have platform change.. we need 
to ensure that regulators voltage level should be matching with wcn3990.
        instead we expect platform vendor to do all these. i want to 
restrict the voltages in driver itself.

        please let me know, if cleared your query.


-- 
Regards
Balakrishna.

Re: [PATCH v7 4/8] Bluetooth: btqca: Redefine qca_uart_setup() to generic function.

[Matthias Kaehlcke]

On Fri, Jun 22, 2018 at 08:41:01PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-06-22 03:39, Matthias Kaehlcke wrote:
> > On Thu, Jun 21, 2018 at 04:50:28PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > > 
> > > On 2018-06-21 05:03, Matthias Kaehlcke wrote:
> > > > Hi Balakrishna,
> > > >
> > > > On Wed, Jun 20, 2018 at 04:23:25PM +0530, Balakrishna Godavarthi wrote:
> > > > > Hi Matthias,
> > > > >
> > > > > On 2018-06-20 01:33, Matthias Kaehlcke wrote:
> > > > > > On Tue, Jun 19, 2018 at 12:39:07PM +0530, Balakrishna Godavarthi wrote:
> > > > > > > Hi Matthias,
> > > > > > >
> > > > > > > Please find my comments inline.
> > > > > > >
> > > > > > > On 2018-06-19 02:49, Matthias Kaehlcke wrote:
> > > > > > > > On Sat, Jun 16, 2018 at 11:57:14AM +0530, Balakrishna Godavarthi wrote:
> > > > > > > >
> > > > > > > > > @@ -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);
> > > > > > > > >  	}
> > > > > > > >
> > > > > > > > Why is this needed? A caller that passes in soc_ver = 0 could just
> > > > > > > > call qca_read_soc_version() themselves.
> > > > > > > [Bala]: hci_qca support two chip one ROME (already up-streamed) and
> > > > > > > other
> > > > > > > wcn3990.
> > > > > > >         there setup sequence differs.
> > > > > > >
> > > > > > >         wcn3900:
> > > > > > >         1. set baudrate to 115200
> > > > > > >         2. read soc version
> > > > > > >         3. set baudarte to 3.2Mbps
> > > > > > >         4. download firmware. (calls qca_uart_setup)
> > > > > > >
> > > > > > >        ROME:
> > > > > > >        1. set baudrate to 3.0 mbps
> > > > > > >        2. calls qca_uart_setup to read soc version and download
> > > > > > > firmware.
> > > > > > >
> > > > > > >       so to make use of existing function qca_setup_uart(). passing
> > > > > > > soc_ver
> > > > > > > as argument.
> > > > > > >       if soc_ver is zero, then read soc version (i.e. for ROME chip).
> > > > > > >
> > > > > > >       Pls let me know, if you have any queries.
> > > > > >
> > > > > > I don't really understand this argumentation. Let's look at the code
> > > > > > at the end of this series, where both Rome and wcn3900 are supported:
> > > > > >
> > > > > > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1059
> > > > > >
> > > > > > The version could be read (for Rome only) after setting the operating
> > > > > > speed:
> > > > >
> > > > > [Bala]: yes we can read SoC version after setting operating baud
> > > > > rate. an
> > > > > advice from SoC vendor that, read SoC version before setting operating
> > > > > speed.
> > > > >         one advantage from reading SoC version before setting
> > > > > operating
> > > > > baudrate,it will helps us to understand whether the soc is
> > > > > responding to any
> > > > > commands in default baud rate
> > > > >         before setting operating speed.
> > > >
> > > > Is the recommendation for Rome or wcn3990? I was referring to Rome and
> > > > the current code sets the operating speed and then reads the SoC
> > > > version (inside qca_uart_setup())
> > > >
> > > 
> > > [Bala]: my statement is wrt to wcn3990. yes, ROME will set operating
> > > speed
> > > and read version.
> > > 
> > > > > > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1111
> > > > > >
> > > > > > I concede that having two "if (qcadev->btsoc_type == XYZ)" branches in
> > > > > > qca_setup() is kinda ugly, but I think it's still better than the
> > > > > > conditional call of qca_read_soc_version() in qca_uart_setup().
> > > > >
> > > > > [Bala]: we require an if-else block  for soc type.As wcn3900 support
> > > > > hardware flow control where as ROME is not supporting hardware flow
> > > > > control.
> > > >
> > > > Since you mention if-else and flow control I'm not sure if you
> > > > understood correctly what I suggested. My suggestion is to add
> > > > something like this:
> > > >
> > > > if (qcadev->btsoc_type == QCA_ROME) {
> > > > 	ret = qca_read_soc_version(hdev, &soc_ver);
> > > > 	if (ret < 0 || soc_ver == 0) {
> > > > 	   	// Note mka@: we might want to skip this log,
> > > > 		// qca_read_soc_version() already logs in all error paths
> > > > 		bt_dev_err(hdev, "Failed to get version %d", ret);
> > > > 		return ret;
> > > > 	}
> > > > }
> > > >
> > > > right here:
> > > >
> > > > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1045608/6/drivers/bluetooth/hci_qca.c#1111
> > > >
> > > > which is functionally equivalent to the current code.
> > > >
> > > > You could even do the error check in the common code (just checking
> > > > for 'soc_ver == 0', assuming that qca_read_soc_version() doesn't set
> > > > it to something different in case of error).
> > > >
> > > > Anyway, I won't insist if you prefer it as is and Marcel is ok with it.
> > > 
> > > [Bala]: thanks for clarifying my doubt, but if remove the
> > > qca_read_soc_version() from qca_uart_setup() and handle it
> > > qca_setup().
> > >         in that case, we will have common blocks of code, when we
> > > integrate
> > > wcn3990 into existing qca_setup()
> > 
> > Excellent!
> > 
> > >        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 */
> > >         speed = qca_get_speed(hu, QCA_INIT_SPEED);
> > >         if (speed)
> > >                 qca_set_speed(hu, speed, QCA_INIT_SPEED);
> > 
> > I said earlier that a _set_speed() followed by a _get_speed() is a bit
> > odd, and that it would be better to get the speed first and pass it as
> > a parameter. After looking again over the larger code I'm not
> > convinced anymore this is such a good idea, since the above pattern is
> > repeated multiple times, when it could be just:
> > 
> > qca_set_speed(hu, QCA_INIT_SPEED)
> > 
> > (+ error handling, which is also missing in the code above)
> > 
> > Just a thought, not asking you to change it back, do whatever seems
> > best to you.
> > 
> > >         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);
> > 
> > It seems setting 'qca_baudrate' could also be done in the common
> > path. Would require an extra qca_get_speed(hu, QCA_OPER_SPEED) call,
> > in exchange this would be done in a single place closer to where
> > 'qca_baudrate' is actually used.
> > 
> > Actually as it is now 'qca_baudrate' could remain uninitialized for
> > Rome if no operating speed is defined, which judging from
> > qca_get_baudrate_value() should result in QCA_BAUDRATE_115200.
> > 
> > >                 }
> > >         }
> > > 
> > >         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) {
> > >                 /*
> > >       ...
> > >       }
> > 
> > Overall looks good and should still become a bit more compact if the
> > handling of flow control is done in _set_speed() as you suggested in
> > "[PATCH v7 8/8] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth
> > chip wcn3990"
> 
> [Bala}: to over come calling qca_get_speed() for qca_baudrate.
>         i prefer passing qca_baudrate as pointer argument to
> qca_set_speed().
>         that  will actually helps to lower function calls. intern increase
> the speed of execution too.

I argued earlier against something similar, where speed was a pointer
that was set inside _set_speed(). IMO this interface is unintuitive
and makes the code harder to read, even if it saves a function
call. The optimization isn't really an important factor here,
_get_speed() is cheap and it isn't called in a hot code path.

It's also an option to add an enum qca_speed_type parameter to
qca_get_baudrate_value() and hide the _get_speed() call there.