[PATCH v5 0/3] Enable Bluetooth functionality for WCN3990

[PATCH v5 0/3] Enable Bluetooth functionality for WCN3990

[Balakrishna Godavarthi]

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 (3):
  dt-bindings: net: bluetooth: Add device tree bindings for QTI chip
    wcn3990
  Bluetooth: btqca: Add wcn3990 firmware download support.
  Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.

 .../devicetree/bindings/net/qualcomm-bluetooth.txt |  31 +-
 drivers/bluetooth/btqca.c                          |  46 +-
 drivers/bluetooth/btqca.h                          |  19 +
 drivers/bluetooth/hci_qca.c                        | 547 ++++++++++++++++++---
 4 files changed, 567 insertions(+), 76 deletions(-)

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

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

[Balakrishna Godavarthi]

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
 .../devicetree/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 0ea18a5..63cae49 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.
+ - oper-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>;
+		oper-speed = <3200000>;
+	};
+};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v5 2/3] 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>
---
 drivers/bluetooth/btqca.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
 drivers/bluetooth/btqca.h | 13 +++++++++++++
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index 8219816..40c6b4f 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 rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
 {
 	struct sk_buff *skb;
 	struct edl_event_hdr *edl;
@@ -88,6 +88,7 @@ static int rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(rome_patch_ver_req);
 
 static int rome_reset(struct hci_dev *hdev)
 {
@@ -380,6 +381,49 @@ int qca_uart_setup_rome(struct hci_dev *hdev, uint8_t baudrate)
 }
 EXPORT_SYMBOL_GPL(qca_uart_setup_rome);
 
+int qca_uart_setup_cherokee(struct hci_dev *hdev, uint8_t baudrate,
+			    u32 *soc_ver)
+{
+	struct rome_config config;
+	int err;
+
+	/* we are using the existing funciton of ROME,
+	 * instead of duplicating the function for wcn3990.
+	 */
+	config.user_baud_rate = baudrate;
+	/* Download rampatch file */
+	config.type = TLV_TYPE_PATCH;
+	snprintf(config.fwname, sizeof(config.fwname), "qca/rampatch_%08x.tlv",
+		 *soc_ver);
+	err = rome_download_firmware(hdev, &config);
+	if (err < 0) {
+		BT_ERR("%s: Failed to download patch (%d)", hdev->name, err);
+		return err;
+	}
+
+	/* Download NVM configuration */
+	config.type = TLV_TYPE_NVM;
+	snprintf(config.fwname, sizeof(config.fwname), "qca/nvm_%08x.bin",
+		 *soc_ver);
+	err = rome_download_firmware(hdev, &config);
+	if (err < 0) {
+		BT_ERR("%s: Failed to download NVM (%d)", hdev->name, 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);
+		return err;
+	}
+
+	bt_dev_info(hdev, "wcn3990 setup on UART is completed");
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(qca_uart_setup_cherokee);
+
 MODULE_AUTHOR("Ben Young Tae Kim <ytkim@qca.qualcomm.com>");
 MODULE_DESCRIPTION("Bluetooth support for Qualcomm Atheros family ver " VERSION);
 MODULE_VERSION(VERSION);
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index 13d77fd..2a7366b 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -128,6 +128,9 @@ struct tlv_type_hdr {
 
 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_cherokee(struct hci_dev *hdev, uint8_t baudrate,
+			    u32 *soc_ver);
+int rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version);
 
 #else
 
@@ -141,4 +144,14 @@ static inline int qca_uart_setup_rome(struct hci_dev *hdev, int speed)
 	return -EOPNOTSUPP;
 }
 
+static int qca_uart_setup_cherokee(struct hci_dev *hdev, uint8_t baudrate,
+				   u32 *soc_ver)
+{
+	return -EOPNOTSUPP;
+}
+
+static int rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
+{
+	return -EOPNOTSUPP;
+}
 #endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v5 3/3] 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.
Add support to read baudrate from dts.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
 drivers/bluetooth/btqca.h   |   6 +
 drivers/bluetooth/hci_qca.c | 547 ++++++++++++++++++++++++++++++++++++++------
 2 files changed, 480 insertions(+), 73 deletions(-)

diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index 2a7366b..8e2142a 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_bardrate {
 	QCA_BAUDRATE_115200 	= 0,
 	QCA_BAUDRATE_57600,
@@ -124,6 +127,9 @@ struct tlv_type_hdr {
 	__u8   data[0];
 } __packed;
 
+int btqca_power_setup(bool on);
+int qca_btsoc_cleanup(struct hci_dev *hdev);
+
 #if IS_ENABLED(CONFIG_BT_QCA)
 
 int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index f05382b..71f9591 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...
@@ -35,6 +35,11 @@
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/serdev.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/of_device.h>
+#include <linux/device.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -119,12 +124,51 @@ struct qca_data {
 	u64 votes_off;
 };
 
+enum btqca_soc_t {
+	BTQCA_INVALID = -1,
+	BTQCA_AR3002,
+	BTQCA_ROME,
+	BTQCA_CHEROKEE
+};
+
 struct qca_serdev {
 	struct hci_uart	 serdev_hu;
 	struct gpio_desc *bt_en;
 	struct clk	 *susclk;
+	enum btqca_soc_t btsoc_type;
+	u32 init_speed;
+	u32 oper_speed;
+};
+
+/*
+ * voltage regulator information required for configuring the
+ * QCA bluetooth chipset
+ */
+struct btqca_vreg {
+	const char *name;
+	unsigned int min_v;
+	unsigned int max_v;
+	unsigned int load_ua;
+};
+
+struct btqca_vreg_data {
+	enum btqca_soc_t soc_type;
+	struct btqca_vreg *vregs;
+	size_t num_vregs;
+};
+
+/*
+ * Platform data for the QCA bluetooth power driver.
+ */
+struct btqca_power {
+	struct device *dev;
+	const struct btqca_vreg_data *vreg_data;
+	struct regulator_bulk_data *vreg_bulk;
+	bool vregs_on;
 };
 
+static struct btqca_power *qca;
+
 static void __serial_clock_on(struct tty_struct *tty)
 {
 	/* TODO: Some chipset requires to enable UART clock on client
@@ -402,6 +446,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 +508,22 @@ 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 == BTQCA_CHEROKEE) {
+			hu->init_speed = qcadev->init_speed;
+			hu->oper_speed = qcadev->oper_speed;
+			ret = btqca_power_setup(true);
+			if (ret)
+				goto out;
+		} 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;
+out:
+	return ret;
 }
 
 static void qca_debugfs_init(struct hci_dev *hdev)
@@ -552,7 +606,10 @@ static int qca_close(struct hci_uart *hu)
 		serdev_device_close(hu->serdev);
 
 		qcadev = serdev_device_get_drvdata(hu->serdev);
-		gpiod_set_value_cansleep(qcadev->bt_en, 0);
+		if (qcadev->btsoc_type == BTQCA_CHEROKEE)
+			qca_btsoc_cleanup(hu->hdev);
+		else
+			gpiod_set_value_cansleep(qcadev->bt_en, 0);
 	}
 
 	kfree_skb(qca->rx_skb);
@@ -872,6 +929,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 +945,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;
@@ -923,68 +982,260 @@ 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_DBG("%s:sending command %02x to SoC", hdev->name, cmd);
+
+	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
+	if (!skb) {
+		BT_ERR("Failed to allocate memory for skb  packet");
+		return -ENOMEM;
+	}
+
+	skb_put_data(skb, &cmd, sizeof(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 */
+	set_current_state(TASK_UNINTERRUPTIBLE);
+	schedule_timeout(usecs_to_jiffies(100));
+	set_current_state(TASK_INTERRUPTIBLE);
+
+	return 0;
+}
+
+static int qca_serdev_open(struct hci_uart *hu)
+{
+	int ret = 0;
+
+	if (hu->serdev) {
+		serdev_device_open(hu->serdev);
+	} else {
+		bt_dev_err(hu->hdev, "open operation not supported");
+		ret = 1;
+	}
+
+	return ret;
+}
+
+int qca_btsoc_cleanup(struct hci_dev *hdev)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+
+	/* change host baud rate before sending power off command */
+	host_set_baudrate(hu, 2400);
+	/* send 0xC0 command to btsoc before turning off regulators */
+	qca_send_vendor_cmd(hdev, CHEROKEE_POWEROFF_PULSE);
+	/* turn off btsoc */
+	return btqca_power_setup(false);
+}
+
+static int qca_serdev_close(struct hci_uart *hu)
+{
+	int ret = 0;
+
+	if (hu->serdev) {
+		serdev_device_close(hu->serdev);
+	} else {
+		bt_dev_err(hu->hdev, "close operation not supported");
+		ret = 1;
+	}
+
+	return ret;
+}
 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;
+
+	qcadev = serdev_device_get_drvdata(hu->serdev);
+
+	switch (qcadev->btsoc_type) {
+	case BTQCA_CHEROKEE:
+		bt_dev_info(hdev, "setting up wcn3990");
+		/* Patch downloading has to be done without IBS mode */
+		clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
+		/* Setup initial baudrate */
+		speed = 0;
+		if (hu->init_speed)
+			speed = hu->init_speed;
+		else if (hu->proto->init_speed)
+			speed = hu->proto->init_speed;
+
+		if (speed) {
+			host_set_baudrate(hu, speed);
+		} else {
+			bt_dev_err(hdev, "initial speed %u", speed);
+			return -1;
+		}
+
+		/* disable flow control, as chip is still not turned on */
+		hci_uart_set_flow_control(hu, true);
+		/* send 0xFC  command to btsoc */
+		ret = qca_send_vendor_cmd(hdev, CHEROKEE_POWERON_PULSE);
+		if (ret) {
+			bt_dev_err(hdev, "Failed to send power on command");
+			return ret;
+		}
+
+		/* close serial port */
+		ret = qca_serdev_close(hu);
+		if (ret)
+			return ret;
+		/* open serial port */
+		ret = qca_serdev_open(hu);
+		if (ret)
+			return ret;
 
-	bt_dev_info(hdev, "ROME setup");
+		/* Setup initial baudrate */
+		speed = 0;
+		if (hu->init_speed)
+			speed = hu->init_speed;
+		else if (hu->proto->init_speed)
+			speed = hu->proto->init_speed;
+		if (speed) {
+			host_set_baudrate(hu, speed);
+		} else {
+			BT_ERR("%s:initial speed %u", hdev->name, speed);
+			return -1;
+		}
 
-	/* Patch downloading has to be done without IBS mode */
-	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
+		/* Enable flow control */
+		hci_uart_set_flow_control(hu, false);
+		/*  wait until flow control settled */
+		msleep(100);
+		bt_dev_info(hdev, "wcn3990 Patch Version Request");
+		ret = rome_patch_ver_req(hdev, &soc_ver);
+		if (ret < 0 || soc_ver == 0) {
+			BT_ERR("%s: Failed to get version 0x%x", hdev->name,
+				ret);
+			return ret;
+		}
 
-	/* Setup initial baudrate */
-	speed = 0;
-	if (hu->init_speed)
-		speed = hu->init_speed;
-	else if (hu->proto->init_speed)
-		speed = hu->proto->init_speed;
+		bt_dev_info(hdev, "wcn3990 controller version 0x%08x", soc_ver);
+
+		/* clear flow control */
+		hci_uart_set_flow_control(hu, true);
+		/* set operating speed */
+		speed = 0;
+		if (hu->oper_speed)
+			speed = hu->oper_speed;
+		else if (hu->proto->oper_speed)
+			speed = hu->proto->oper_speed;
+		if (speed) {
+			qca_baudrate = qca_get_baudrate_value(speed);
+			bt_dev_info(hdev, "Set UART speed to %d", speed);
+			ret = qca_set_baudrate(hdev, qca_baudrate);
+			if (ret) {
+				BT_ERR("%s:Failed to change the baud rate(%d)",
+					hdev->name, ret);
+				return ret;
+			}
+			if (speed) {
+				host_set_baudrate(hu, speed);
+			} else {
+				BT_ERR("%s:Error in setting operator speed:%u",
+					hdev->name, speed);
+				return -1;
+			}
+		}
 
-	if (speed)
-		host_set_baudrate(hu, speed);
+		/* Set flow control */
+		hci_uart_set_flow_control(hu, false);
+		/* Setup patch and NVM configurations */
+		ret = qca_uart_setup_cherokee(hdev, qca_baudrate, &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 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;
+		/* Setup wcn3990 bdaddr */
+		hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
 
-	if (speed) {
-		qca_baudrate = qca_get_baudrate_value(speed);
+		return ret;
 
-		bt_dev_info(hdev, "Set UART speed to %d", speed);
-		ret = qca_set_baudrate(hdev, qca_baudrate);
-		if (ret) {
-			bt_dev_err(hdev, "Failed to change the baud rate (%d)",
-				   ret);
+	default:
+		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 */
+		speed = 0;
+		if (hu->init_speed)
+			speed = hu->init_speed;
+		else if (hu->proto->init_speed)
+			speed = hu->proto->init_speed;
+
+		if (speed)
+			host_set_baudrate(hu, speed);
+
+		/* 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;
+
+		if (speed) {
+			qca_baudrate = qca_get_baudrate_value(speed);
+
+			bt_dev_info(hdev, "Set UART speed to %d", speed);
+			ret = qca_set_baudrate(hdev, qca_baudrate);
+			if (ret) {
+				bt_dev_err(hdev, "Failed to change the baud rate (%d)",
+					    ret);
 			return ret;
+			}
+			host_set_baudrate(hu, speed);
 		}
-		host_set_baudrate(hu, speed);
-	}
 
-	/* Setup patch / NVM configurations */
-	ret = qca_uart_setup_rome(hdev, qca_baudrate);
-	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 patch / NVM configurations */
+		ret = qca_uart_setup_rome(hdev, qca_baudrate);
+		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;
+		/* Setup bdaddr */
+		hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
 
-	return ret;
+		return ret;
+	}
 }
 
 static struct hci_uart_proto qca_proto = {
@@ -1002,44 +1253,189 @@ static int qca_setup(struct hci_uart *hu)
 	.dequeue	= qca_dequeue,
 };
 
+static const struct btqca_vreg_data cherokee_data = {
+	.soc_type = BTQCA_CHEROKEE,
+	.vregs = (struct btqca_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 btqca_enable_regulators(int i, struct btqca_vreg *vregs)
+{
+	int ret = 0;
+
+	ret = regulator_set_voltage(qca->vreg_bulk[i].consumer, vregs[i].min_v,
+				    vregs[i].max_v);
+	if (ret)
+		goto out;
+
+	if (vregs[i].load_ua)
+		ret = regulator_set_load(qca->vreg_bulk[i].consumer,
+					 vregs[i].load_ua);
+
+	if (ret)
+		goto out;
+
+	ret = regulator_enable(qca->vreg_bulk[i].consumer);
+
+out:
+	return ret;
+
+}
+
+static void btqca_disable_regulators(int reg_nu, struct btqca_vreg *vregs)
+{
+	int i;
+
+	/* disable the regulator if requested by user
+	 * or when fault to enable any regulator.
+	 */
+	for (i = reg_nu - 1; i >= 0 ; i--) {
+		regulator_disable(qca->vreg_bulk[i].consumer);
+		regulator_set_voltage(qca->vreg_bulk[i].consumer, 0,
+				      vregs[i].max_v);
+		if (vregs[i].load_ua)
+			regulator_set_load(qca->vreg_bulk[i].consumer, 0);
+	}
+}
+
+int btqca_power_setup(bool on)
+{
+	int ret = 0;
+	int i;
+	struct btqca_vreg *vregs;
+
+	if (!qca || !qca->vreg_data || !qca->vreg_bulk)
+		return -EINVAL;
+	vregs = qca->vreg_data->vregs;
+
+	BT_DBG("on: %d", on);
+	/* turn on if regualtors are off */
+	if (on == true && qca->vregs_on == false) {
+		qca->vregs_on = true;
+		for (i = 0; i < qca->vreg_data->num_vregs; i++) {
+			ret = btqca_enable_regulators(i, vregs);
+			if (ret)
+				break;
+		}
+	} else if (on == false && qca->vregs_on == true) {
+		qca->vregs_on = false;
+		/* turn off regulator in reverse order */
+		btqca_disable_regulators(qca->vreg_data->num_vregs, vregs);
+	}
+
+	/* regulators failed */
+	if (ret) {
+		qca->vregs_on = false;
+		BT_ERR("failed to enable regulator:%s", vregs[i].name);
+		/* turn off regulators which are enabled */
+		btqca_disable_regulators(i, vregs);
+	}
+
+	return ret;
+}
+
+static int init_regulators(struct btqca_power *qca,
+			   const struct btqca_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;
-	int err;
+	const struct btqca_vreg_data *data;
+	int err = 0;
 
 	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
 	if (!qcadev)
 		return -ENOMEM;
 
 	qcadev->serdev_hu.serdev = serdev;
+	data = of_device_get_match_data(&serdev->dev);
+	if (data && data->soc_type == BTQCA_CHEROKEE)
+		qcadev->btsoc_type = BTQCA_CHEROKEE;
+	else
+		qcadev->btsoc_type = BTQCA_ROME;
+
 	serdev_device_set_drvdata(serdev, qcadev);
+	if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
+		qca = devm_kzalloc(&serdev->dev, sizeof(struct btqca_power),
+				   GFP_KERNEL);
+		if (!qca)
+			return -ENOMEM;
+
+		qca->dev = &serdev->dev;
+		qca->vreg_data = data;
+		err = init_regulators(qca, data->vregs, data->num_vregs);
+		if (err) {
+			BT_ERR("Failed to init regulators:%d", err);
+			kfree(qca);
+			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);
-	}
+		/* set voltage regulator status as false */
+		qca->vregs_on = false;
+		/* get operating speed */
+		device_property_read_u32(&serdev->dev, "oper-speed",
+					 &qcadev->oper_speed);
+		device_property_read_u32(&serdev->dev, "init-speed",
+					 &qcadev->init_speed);
+		if (!qcadev->oper_speed)
+			BT_INFO("UART will pick default proto operating speed");
+		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
+		if (err) {
+			BT_ERR("wcn3990 serdev registration failed");
+			kfree(qca);
+			goto out;
+		}
 
-	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);
-	}
+	} 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);
+		}
 
-	err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
-	if (err)
-		return err;
+		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_prepare_enable(qcadev->susclk);
-	if (err)
-		return err;
+		err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
+		if (err)
+			return err;
 
-	err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
-	if (err)
-		clk_disable_unprepare(qcadev->susclk);
+		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);
+	}
+
+out:	return err;
 
-	return err;
 }
 
 static void qca_serdev_remove(struct serdev_device *serdev)
@@ -1047,12 +1443,17 @@ 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);
-
-	clk_disable_unprepare(qcadev->susclk);
+	if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
+		btqca_power_setup(false);
+		kfree(qca);
+	} else {
+		clk_disable_unprepare(qcadev->susclk);
+	}
 }
 
 static const struct of_device_id qca_bluetooth_of_match[] = {
 	{ .compatible = "qcom,qca6174-bt" },
+	{ .compatible = "qcom,wcn3990-bt", .data = &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 v5 2/3] Bluetooth: btqca: Add wcn3990 firmware download support.

[Matthias Kaehlcke]

Hi Bala,

On Fri, May 11, 2018 at 05:51:02PM +0530, Balakrishna Godavarthi wrote:
> This patch enables the RAM and NV patch download for wcn3990.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
>  drivers/bluetooth/btqca.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/bluetooth/btqca.h | 13 +++++++++++++
>  2 files changed, 58 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index 8219816..40c6b4f 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 rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)

If this and other functions aren't really Rome specific they should
probably be renamed to qca_...

> +int qca_uart_setup_cherokee(struct hci_dev *hdev, uint8_t baudrate,
> +			    u32 *soc_ver)
> +{
> +	struct rome_config config;
> +	int err;
> +
> +	/* we are using the existing funciton of ROME,
> +	 * instead of duplicating the function for wcn3990.
> +	 */

The comment isn't quite accurate, the qca_uart_setup_cherokee() calls
existing functions, but is essentially a copy of qca_uart_setup_rome().

The main difference is that the patch version is passed as a parameter
instead of determining it in the uart setup function. There seems to
be no need to pass the version number in, or if you prefer to do it
this way, you could change the Rome code to do this.

The other delta is the filename extension of the rampatch file, which
is .bin for Rome and .tlv for Cherokee. Is there a good reason to use
a different extension? If not just stick to the existing naming
scheme, otherwise you could pass the chip type as a parameter and
chose the extensions based on that.

Thanks

Matthias

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

[Marcel Holtmann]

Hi Balakrishna,

> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> .../devicetree/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 0ea18a5..63cae49 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.
> + - oper-speed: operating speed of the chip.

we actually agreed on using max-speed for these. And it should optional to limit speed for “broken” devices or that have some limit because of the platform.

Regards

Marcel

Re: [PATCH v5 3/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.

[Marcel Holtmann]

Hi Balakrishna,

> Add support to set voltage/current of various regulators
> to power up/down Bluetooth chip wcn3990.
> Add support to read baudrate from dts.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> drivers/bluetooth/btqca.h   |   6 +
> drivers/bluetooth/hci_qca.c | 547 ++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 480 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index 2a7366b..8e2142a 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_bardrate {
> 	QCA_BAUDRATE_115200 	= 0,
> 	QCA_BAUDRATE_57600,
> @@ -124,6 +127,9 @@ struct tlv_type_hdr {
> 	__u8   data[0];
> } __packed;
> 
> +int btqca_power_setup(bool on);

my initial comment on this has not yet been addressed. They are all per hci_dev or I am not going to merge it.

> +int qca_btsoc_cleanup(struct hci_dev *hdev);
> +
> #if IS_ENABLED(CONFIG_BT_QCA)
> 
> int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index f05382b..71f9591 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...
> @@ -35,6 +35,11 @@
> #include <linux/mod_devicetable.h>
> #include <linux/module.h>
> #include <linux/serdev.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/of_device.h>
> +#include <linux/device.h>
> 
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -119,12 +124,51 @@ struct qca_data {
> 	u64 votes_off;
> };
> 
> +enum btqca_soc_t {
> +	BTQCA_INVALID = -1,
> +	BTQCA_AR3002,
> +	BTQCA_ROME,
> +	BTQCA_CHEROKEE
> +};
> +
> struct qca_serdev {
> 	struct hci_uart	 serdev_hu;
> 	struct gpio_desc *bt_en;
> 	struct clk	 *susclk;
> +	enum btqca_soc_t btsoc_type;
> +	u32 init_speed;
> +	u32 oper_speed;
> +};
> +
> +/*
> + * voltage regulator information required for configuring the
> + * QCA bluetooth chipset
> + */
> +struct btqca_vreg {
> +	const char *name;
> +	unsigned int min_v;
> +	unsigned int max_v;
> +	unsigned int load_ua;
> +};
> +
> +struct btqca_vreg_data {
> +	enum btqca_soc_t soc_type;
> +	struct btqca_vreg *vregs;
> +	size_t num_vregs;
> +};
> +
> +/*
> + * Platform data for the QCA bluetooth power driver.
> + */
> +struct btqca_power {
> +	struct device *dev;
> +	const struct btqca_vreg_data *vreg_data;
> +	struct regulator_bulk_data *vreg_bulk;
> +	bool vregs_on;
> };
> 
> +static struct btqca_power *qca;
> +

No such global variable.

> static void __serial_clock_on(struct tty_struct *tty)
> {
> 	/* TODO: Some chipset requires to enable UART clock on client
> @@ -402,6 +446,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 +508,22 @@ 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 == BTQCA_CHEROKEE) {
> +			hu->init_speed = qcadev->init_speed;
> +			hu->oper_speed = qcadev->oper_speed;
> +			ret = btqca_power_setup(true);
> +			if (ret)
> +				goto out;

Doing return btqca_power_setup is the same and less complicated.

> +		} 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;
> +out:
> +	return ret;
> }
> 
> static void qca_debugfs_init(struct hci_dev *hdev)
> @@ -552,7 +606,10 @@ static int qca_close(struct hci_uart *hu)
> 		serdev_device_close(hu->serdev);
> 
> 		qcadev = serdev_device_get_drvdata(hu->serdev);
> -		gpiod_set_value_cansleep(qcadev->bt_en, 0);
> +		if (qcadev->btsoc_type == BTQCA_CHEROKEE)
> +			qca_btsoc_cleanup(hu->hdev);
> +		else
> +			gpiod_set_value_cansleep(qcadev->bt_en, 0);
> 	}
> 
> 	kfree_skb(qca->rx_skb);
> @@ -872,6 +929,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 +945,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;

The baud rate addition can be a separate patch.

> 
> 	cmd[4] = baudrate;
> @@ -923,68 +982,260 @@ 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_DBG("%s:sending command %02x to SoC", hdev->name, cmd);
> +
> +	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
> +	if (!skb) {
> +		BT_ERR("Failed to allocate memory for skb  packet");
> +		return -ENOMEM;
> +	}
> +
> +	skb_put_data(skb, &cmd, sizeof(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 */
> +	set_current_state(TASK_UNINTERRUPTIBLE);
> +	schedule_timeout(usecs_to_jiffies(100));
> +	set_current_state(TASK_INTERRUPTIBLE);

Any reason to not use msleep.

> +
> +	return 0;
> +}
> +
> +static int qca_serdev_open(struct hci_uart *hu)
> +{
> +	int ret = 0;
> +
> +	if (hu->serdev) {
> +		serdev_device_open(hu->serdev);
> +	} else {
> +		bt_dev_err(hu->hdev, "open operation not supported");
> +		ret = 1;
> +	}
> +
> +	return ret;
> +}
> +
> +int qca_btsoc_cleanup(struct hci_dev *hdev)
> +{
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +
> +	/* change host baud rate before sending power off command */
> +	host_set_baudrate(hu, 2400);
> +	/* send 0xC0 command to btsoc before turning off regulators */
> +	qca_send_vendor_cmd(hdev, CHEROKEE_POWEROFF_PULSE);
> +	/* turn off btsoc */
> +	return btqca_power_setup(false);
> +}
> +
> +static int qca_serdev_close(struct hci_uart *hu)
> +{
> +	int ret = 0;
> +
> +	if (hu->serdev) {
> +		serdev_device_close(hu->serdev);
> +	} else {
> +		bt_dev_err(hu->hdev, "close operation not supported");
> +		ret = 1;
> +	}
> +
> +	return ret;
> +}
> 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;
> +
> +	qcadev = serdev_device_get_drvdata(hu->serdev);
> +
> +	switch (qcadev->btsoc_type) {
> +	case BTQCA_CHEROKEE:
> +		bt_dev_info(hdev, "setting up wcn3990”);

Nope. This is debug level stuff.

> +		/* Patch downloading has to be done without IBS mode */
> +		clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> +		/* Setup initial baudrate */
> +		speed = 0;
> +		if (hu->init_speed)
> +			speed = hu->init_speed;
> +		else if (hu->proto->init_speed)
> +			speed = hu->proto->init_speed;
> +
> +		if (speed) {
> +			host_set_baudrate(hu, speed);
> +		} else {
> +			bt_dev_err(hdev, "initial speed %u", speed);
> +			return -1;
> +		}
> +
> +		/* disable flow control, as chip is still not turned on */
> +		hci_uart_set_flow_control(hu, true);
> +		/* send 0xFC  command to btsoc */
> +		ret = qca_send_vendor_cmd(hdev, CHEROKEE_POWERON_PULSE);
> +		if (ret) {
> +			bt_dev_err(hdev, "Failed to send power on command");
> +			return ret;
> +		}
> +
> +		/* close serial port */
> +		ret = qca_serdev_close(hu);
> +		if (ret)
> +			return ret;
> +		/* open serial port */
> +		ret = qca_serdev_open(hu);
> +		if (ret)
> +			return ret;
> 
> -	bt_dev_info(hdev, "ROME setup");
> +		/* Setup initial baudrate */
> +		speed = 0;
> +		if (hu->init_speed)
> +			speed = hu->init_speed;
> +		else if (hu->proto->init_speed)
> +			speed = hu->proto->init_speed;
> +		if (speed) {
> +			host_set_baudrate(hu, speed);
> +		} else {
> +			BT_ERR("%s:initial speed %u", hdev->name, speed);
> +			return -1;
> +		}
> 
> -	/* Patch downloading has to be done without IBS mode */
> -	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> +		/* Enable flow control */
> +		hci_uart_set_flow_control(hu, false);
> +		/*  wait until flow control settled */
> +		msleep(100);
> +		bt_dev_info(hdev, "wcn3990 Patch Version Request");
> +		ret = rome_patch_ver_req(hdev, &soc_ver);
> +		if (ret < 0 || soc_ver == 0) {
> +			BT_ERR("%s: Failed to get version 0x%x", hdev->name,
> +				ret);
> +			return ret;
> +		}
> 
> -	/* Setup initial baudrate */
> -	speed = 0;
> -	if (hu->init_speed)
> -		speed = hu->init_speed;
> -	else if (hu->proto->init_speed)
> -		speed = hu->proto->init_speed;
> +		bt_dev_info(hdev, "wcn3990 controller version 0x%08x", soc_ver);
> +
> +		/* clear flow control */
> +		hci_uart_set_flow_control(hu, true);
> +		/* set operating speed */
> +		speed = 0;
> +		if (hu->oper_speed)
> +			speed = hu->oper_speed;
> +		else if (hu->proto->oper_speed)
> +			speed = hu->proto->oper_speed;
> +		if (speed) {
> +			qca_baudrate = qca_get_baudrate_value(speed);
> +			bt_dev_info(hdev, "Set UART speed to %d", speed);
> +			ret = qca_set_baudrate(hdev, qca_baudrate);
> +			if (ret) {
> +				BT_ERR("%s:Failed to change the baud rate(%d)",
> +					hdev->name, ret);
> +				return ret;
> +			}
> +			if (speed) {
> +				host_set_baudrate(hu, speed);
> +			} else {
> +				BT_ERR("%s:Error in setting operator speed:%u",
> +					hdev->name, speed);
> +				return -1;
> +			}
> +		}
> 
> -	if (speed)
> -		host_set_baudrate(hu, speed);
> +		/* Set flow control */
> +		hci_uart_set_flow_control(hu, false);
> +		/* Setup patch and NVM configurations */
> +		ret = qca_uart_setup_cherokee(hdev, qca_baudrate, &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 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;
> +		/* Setup wcn3990 bdaddr */
> +		hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
> 
> -	if (speed) {
> -		qca_baudrate = qca_get_baudrate_value(speed);
> +		return ret;
> 
> -		bt_dev_info(hdev, "Set UART speed to %d", speed);
> -		ret = qca_set_baudrate(hdev, qca_baudrate);
> -		if (ret) {
> -			bt_dev_err(hdev, "Failed to change the baud rate (%d)",
> -				   ret);
> +	default:
> +		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 */
> +		speed = 0;
> +		if (hu->init_speed)
> +			speed = hu->init_speed;
> +		else if (hu->proto->init_speed)
> +			speed = hu->proto->init_speed;
> +
> +		if (speed)
> +			host_set_baudrate(hu, speed);
> +
> +		/* 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;
> +
> +		if (speed) {
> +			qca_baudrate = qca_get_baudrate_value(speed);
> +
> +			bt_dev_info(hdev, "Set UART speed to %d", speed);
> +			ret = qca_set_baudrate(hdev, qca_baudrate);
> +			if (ret) {
> +				bt_dev_err(hdev, "Failed to change the baud rate (%d)",
> +					    ret);
> 			return ret;
> +			}
> +			host_set_baudrate(hu, speed);
> 		}
> -		host_set_baudrate(hu, speed);
> -	}
> 
> -	/* Setup patch / NVM configurations */
> -	ret = qca_uart_setup_rome(hdev, qca_baudrate);
> -	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 patch / NVM configurations */
> +		ret = qca_uart_setup_rome(hdev, qca_baudrate);
> +		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;
> +		/* Setup bdaddr */
> +		hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
> 
> -	return ret;
> +		return ret;
> +	}
> }

This change is so complex and convoluted that I can not review it.

> 
> static struct hci_uart_proto qca_proto = {
> @@ -1002,44 +1253,189 @@ static int qca_setup(struct hci_uart *hu)
> 	.dequeue	= qca_dequeue,
> };
> 
> +static const struct btqca_vreg_data cherokee_data = {
> +	.soc_type = BTQCA_CHEROKEE,
> +	.vregs = (struct btqca_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 btqca_enable_regulators(int i, struct btqca_vreg *vregs)
> +{
> +	int ret = 0;
> +
> +	ret = regulator_set_voltage(qca->vreg_bulk[i].consumer, vregs[i].min_v,
> +				    vregs[i].max_v);
> +	if (ret)
> +		goto out;
> +
> +	if (vregs[i].load_ua)
> +		ret = regulator_set_load(qca->vreg_bulk[i].consumer,
> +					 vregs[i].load_ua);
> +
> +	if (ret)
> +		goto out;
> +
> +	ret = regulator_enable(qca->vreg_bulk[i].consumer);
> +
> +out:
> +	return ret;
> +
> +}
> +
> +static void btqca_disable_regulators(int reg_nu, struct btqca_vreg *vregs)
> +{
> +	int i;
> +
> +	/* disable the regulator if requested by user
> +	 * or when fault to enable any regulator.
> +	 */
> +	for (i = reg_nu - 1; i >= 0 ; i--) {
> +		regulator_disable(qca->vreg_bulk[i].consumer);
> +		regulator_set_voltage(qca->vreg_bulk[i].consumer, 0,
> +				      vregs[i].max_v);
> +		if (vregs[i].load_ua)
> +			regulator_set_load(qca->vreg_bulk[i].consumer, 0);
> +	}
> +}
> +
> +int btqca_power_setup(bool on)
> +{
> +	int ret = 0;
> +	int i;
> +	struct btqca_vreg *vregs;
> +
> +	if (!qca || !qca->vreg_data || !qca->vreg_bulk)
> +		return -EINVAL;
> +	vregs = qca->vreg_data->vregs;
> +
> +	BT_DBG("on: %d", on);
> +	/* turn on if regualtors are off */
> +	if (on == true && qca->vregs_on == false) {
> +		qca->vregs_on = true;
> +		for (i = 0; i < qca->vreg_data->num_vregs; i++) {
> +			ret = btqca_enable_regulators(i, vregs);
> +			if (ret)
> +				break;
> +		}
> +	} else if (on == false && qca->vregs_on == true) {
> +		qca->vregs_on = false;
> +		/* turn off regulator in reverse order */
> +		btqca_disable_regulators(qca->vreg_data->num_vregs, vregs);
> +	}

I do not get this function separation. Please clean this up. And use x and !x instead of == false and == true.

> +
> +	/* regulators failed */
> +	if (ret) {
> +		qca->vregs_on = false;
> +		BT_ERR("failed to enable regulator:%s", vregs[i].name);
> +		/* turn off regulators which are enabled */
> +		btqca_disable_regulators(i, vregs);
> +	}
> +
> +	return ret;
> +}
> +
> +static int init_regulators(struct btqca_power *qca,
> +			   const struct btqca_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;
> -	int err;
> +	const struct btqca_vreg_data *data;
> +	int err = 0;
> 
> 	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
> 	if (!qcadev)
> 		return -ENOMEM;
> 
> 	qcadev->serdev_hu.serdev = serdev;
> +	data = of_device_get_match_data(&serdev->dev);
> +	if (data && data->soc_type == BTQCA_CHEROKEE)
> +		qcadev->btsoc_type = BTQCA_CHEROKEE;
> +	else
> +		qcadev->btsoc_type = BTQCA_ROME;
> +
> 	serdev_device_set_drvdata(serdev, qcadev);
> +	if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
> +		qca = devm_kzalloc(&serdev->dev, sizeof(struct btqca_power),
> +				   GFP_KERNEL);
> +		if (!qca)
> +			return -ENOMEM;
> +
> +		qca->dev = &serdev->dev;
> +		qca->vreg_data = data;
> +		err = init_regulators(qca, data->vregs, data->num_vregs);
> +		if (err) {
> +			BT_ERR("Failed to init regulators:%d", err);
> +			kfree(qca);
> +			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);
> -	}
> +		/* set voltage regulator status as false */
> +		qca->vregs_on = false;
> +		/* get operating speed */
> +		device_property_read_u32(&serdev->dev, "oper-speed",
> +					 &qcadev->oper_speed);
> +		device_property_read_u32(&serdev->dev, "init-speed",
> +					 &qcadev->init_speed);

Not even defined in DT.

> +		if (!qcadev->oper_speed)
> +			BT_INFO("UART will pick default proto operating speed");
> +		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
> +		if (err) {
> +			BT_ERR("wcn3990 serdev registration failed");
> +			kfree(qca);
> +			goto out;
> +		}
> 
> -	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);
> -	}
> +	} 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);
> +		}
> 
> -	err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
> -	if (err)
> -		return err;
> +		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_prepare_enable(qcadev->susclk);
> -	if (err)
> -		return err;
> +		err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
> +		if (err)
> +			return err;
> 
> -	err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
> -	if (err)
> -		clk_disable_unprepare(qcadev->susclk);
> +		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);
> +	}
> +
> +out:	return err;
> 
> -	return err;
> }
> 
> static void qca_serdev_remove(struct serdev_device *serdev)
> @@ -1047,12 +1443,17 @@ 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);
> -
> -	clk_disable_unprepare(qcadev->susclk);
> +	if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
> +		btqca_power_setup(false);

I am really not merging this. A driver that supports only one instance is not okay.

> +		kfree(qca);
> +	} else {
> +		clk_disable_unprepare(qcadev->susclk);
> +	}
> }
> 
> static const struct of_device_id qca_bluetooth_of_match[] = {
> 	{ .compatible = "qcom,qca6174-bt" },
> +	{ .compatible = "qcom,wcn3990-bt", .data = &cherokee_data},
> 	{ /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);

Regards

Marcel

Re: [PATCH v5 3/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.

[Matthias Kaehlcke]

Hi Bala,

On Fri, May 11, 2018 at 05:51:03PM +0530, Balakrishna Godavarthi wrote:
> Add support to set voltage/current of various regulators
> to power up/down Bluetooth chip wcn3990.
> Add support to read baudrate from dts.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---

Please include a change log also in the individual patches, not just
in the cover letter. In a revision after many comments it may be
sufficient to say "addressed <reviewers> comments", if the number if
changes is limited it is preferable to briefly list the individual
changes.

>  drivers/bluetooth/btqca.h   |   6 +
>  drivers/bluetooth/hci_qca.c | 547 ++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 480 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index 2a7366b..8e2142a 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)

The parentheses are not needed around a literal. That they are used
for the other values is IMO no good reason to introduce more of them.
You might want to squeeze in a clean up patch that removes them for
the other defines.

> +int btqca_power_setup(bool on);
> +int qca_btsoc_cleanup(struct hci_dev *hdev);

nit: inconsistent naming.

If maintainers and authors have no objections you might want to
squeeze in a patch that renames everything to btqca. Just a suggestion
though.

> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index f05382b..71f9591 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...
> @@ -35,6 +35,11 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/serdev.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/of_device.h>
> +#include <linux/device.h>

As I mentioned on v4, the includes should be in alphabetical order.

> +enum btqca_soc_t {

nit: Again, inconsistent naming, some functions/structs are called
qca_... other btqca_... Personally I prefer btqca_ to avoid clashes.

> +/*
> + * voltage regulator information required for configuring the
> + * QCA bluetooth chipset

nit: Voltage, Bluetooth

> + */
> +struct btqca_vreg {
> +	const char *name;
> +	unsigned int min_v;
> +	unsigned int max_v;
> +	unsigned int load_ua;
> +};

nit: consider min_uV, max_uV, load_uA (as in the regulator framework).

> +
> +struct btqca_vreg_data {
> +	enum btqca_soc_t soc_type;
> +	struct btqca_vreg *vregs;
> +	size_t num_vregs;
> +};
> +
> +/*
> + * Platform data for the QCA bluetooth power driver.

nit: Bluetooth

> +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_DBG("%s:sending command %02x to SoC", hdev->name, cmd);

bt_dev_dbg()

> +
> +	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
> +	if (!skb) {
> +		BT_ERR("Failed to allocate memory for skb  packet");

bt_dev_err()

> +	/* Wait for 100 us for soc to settle down */

nit: uS, SoC

> +static int qca_serdev_open(struct hci_uart *hu)
> +{
> +	int ret = 0;
> +
> +	if (hu->serdev) {
> +		serdev_device_open(hu->serdev);
> +	} else {
> +		bt_dev_err(hu->hdev, "open operation not supported");
> +		ret = 1;
> +	}
> +
> +	return ret;
> +}

>From v4:

> > Check return value.
> [Bala]: as we are not checking in qca_open, the return in this
> function is to know the serdev function availability.

Sorry, your comment didn't really clarify things for me. The function
is called from qca_setup() apparently with the intention to open the
serial port, not to check if the function is available. If the serial
port can't be opened for whatever reason the function should return an
error (typically a negative value).

Also the error message is misleading, the underlying problem is not
that the open operations is not supported, but that no serdev device
is associated with the hci_uart. Actually it seems this could never
happen:

In qca_serdev_probe():

  qcadev->serdev_hu.serdev = serdev;

And qca_serdev_open() is only called from qca_setup(). Basically the
first thing qca_setup() does is:

  qcadev = serdev_device_get_drvdata(hu->serdev);

Thus if hu->serdev is NULL qca_setup() will crash shortly after:

  switch (qcadev->btsoc_type) {

I think we can get rid of qca_serdev_open/close() and just call
directly serdev_device_open().

> +int qca_btsoc_cleanup(struct hci_dev *hdev)
> +{
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +
> +	/* change host baud rate before sending power off command */
> +	host_set_baudrate(hu, 2400);
> +	/* send 0xC0 command to btsoc before turning off regulators */
> +	qca_send_vendor_cmd(hdev, CHEROKEE_POWEROFF_PULSE);

The comment doesn't provide any useful information. From the code it's
evident that a CHEROKEE_POWEROFF_PULSE is sent, if anybody is
interested in the hex code they can look up the definition of
CHEROKEE_POWEROFF_PULSE.

> +	/* turn off btsoc */
> +	return btqca_power_setup(false);

This comment laso doesn't provide any value.

> +static int qca_serdev_close(struct hci_uart *hu)
> +{
> +	int ret = 0;
> +
> +	if (hu->serdev) {
> +		serdev_device_close(hu->serdev);
> +	} else {
> +		bt_dev_err(hu->hdev, "close operation not supported");
> +		ret = 1;
> +	}
> +
> +	return ret;
> +}

Same commants as for open().

Preferably keep open() and close() together, without squeezing the
cleanup function in between.

>  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;
> +
> +	qcadev = serdev_device_get_drvdata(hu->serdev);
> +
> +	switch (qcadev->btsoc_type) {
> +	case BTQCA_CHEROKEE:

I still thinks this is has a lot of common code with the default/Rome
case, but let's first clarify and possibly improve a few things.

> +		bt_dev_info(hdev, "setting up wcn3990");
> +		/* Patch downloading has to be done without IBS mode */
> +		clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
> +		/* Setup initial baudrate */
> +		speed = 0;
> +		if (hu->init_speed)
> +			speed = hu->init_speed;
> +		else if (hu->proto->init_speed)
> +			speed = hu->proto->init_speed;

This pattern is repeated a several times for initial and operating
speeds. Helper functions like btqca_get_init_speed() and
btqca_get_oper_speed() would make the code more readable and compact.

> +
> +		if (speed) {
> +			host_set_baudrate(hu, speed);
> +		} else {
> +			bt_dev_err(hdev, "initial speed %u", speed);
> +			return -1;
> +		}

Note: Up to here Rome and Cherokee do exactly the same.

> +		/* disable flow control, as chip is still not turned on */
> +		hci_uart_set_flow_control(hu, true);
> +		/* send 0xFC  command to btsoc */
> +		ret = qca_send_vendor_cmd(hdev, CHEROKEE_POWERON_PULSE);

Delete useless comment.

> +		if (ret) {
> +			bt_dev_err(hdev, "Failed to send power on command");
> +			return ret;
> +		}
> +
> +		/* close serial port */
> +		ret = qca_serdev_close(hu);
> +		if (ret)
> +			return ret;
> +		/* open serial port */
> +		ret = qca_serdev_open(hu);
> +		if (ret)
> +			return ret;

The comments are pointless, it is evident from the code that the
serial port is opened/closed. Much more interesting would be to
explain why it is necessary to close and re-open the port.

> -	bt_dev_info(hdev, "ROME setup");
> +		/* Setup initial baudrate */
> +		speed = 0;
> +		if (hu->init_speed)
> +			speed = hu->init_speed;
> +		else if (hu->proto->init_speed)
> +			speed = hu->proto->init_speed;
> +		if (speed) {
> +			host_set_baudrate(hu, speed);
> +		} else {
> +			BT_ERR("%s:initial speed %u", hdev->name, speed);

bt_dev_err()

> +			return -1;
> +		}

The initial baudrate has already been set above, is it necessary to
configure it again because the port was closed?

This baudrate code is extremely noisy. Besides introducing the helper
functions mentioned above you could log the error message in
host_set_baudrate() or even have an addtional wrapper that determines
and sets the baudrate. The latter would reduce the above to:

if (xyz_set_baudrate(hu, INIT_RATE))
	return -1;

And that multiple times.

> +		/* clear flow control */
> +		hci_uart_set_flow_control(hu, true);
> +		/* set operating speed */

Note: Starting from here the code for Cherokee and Rome is essentially
the same, except for enabling flow control.

If you prefer keep the somewhat redundant code paths in the next
revision, but please consider the improvements I suggested. With less
noisy code it will be easier to determine if it is reasonable to join
the two code paths.

> +static int btqca_enable_regulators(int i, struct btqca_vreg *vregs)

The common convention is to pass first the structure a function acts
on, then other parameters.  Use a more expressive name for the
number of regulators, like 'num_regs' or 'nregs'.


> +{
> +	int ret = 0;

Initialization is not necessary, ret is assigned in the next line.

> +static void btqca_disable_regulators(int reg_nu, struct btqca_vreg *vregs)
> +{

Same as for the enable function. Please use the same name for the
argument with the number of regulators on both functions, personally I
don't think 'reg_nu' is a great choice, but that's just my opinion ;-)

> +int btqca_power_setup(bool on)
> +{
> +	int ret = 0;
> +	int i;
> +	struct btqca_vreg *vregs;
> +
> +	if (!qca || !qca->vreg_data || !qca->vreg_bulk)
> +		return -EINVAL;
> +	vregs = qca->vreg_data->vregs;
> +
> +	BT_DBG("on: %d", on);
> +	/* turn on if regualtors are off */

regulators

Consider dropping the comment, IMO the code speaks for itself.

> +	if (on == true && qca->vregs_on == false) {

if (on && !qca->vreg_on) {

> +		qca->vregs_on = true;

Typically you'd change the variable after having performed the
operation with success.

> +		for (i = 0; i < qca->vreg_data->num_vregs; i++) {
> +			ret = btqca_enable_regulators(i, vregs);
> +			if (ret)
> +				break;
> +		}
> +	} else if (on == false && qca->vregs_on == true) {

if (!on && qca->vreg_on) {

> +		qca->vregs_on = false;

Better done after having disabled the regulators.

> +		/* turn off regulator in reverse order */
> +		btqca_disable_regulators(qca->vreg_data->num_vregs, vregs);
> +	}
> +
> +	/* regulators failed */
> +	if (ret) {
> +		qca->vregs_on = false;
> +		BT_ERR("failed to enable regulator:%s", vregs[i].name);
> +		/* turn off regulators which are enabled */
> +		btqca_disable_regulators(i, vregs);
> +	}

Why not do this directly when the problem is detected? The code also
relies on 'ret' only being set in the 'on' path. Which isn't intuitive
for the reader and *might* change in the future.

> +
> +	return ret;
> +}
> +
> +static int init_regulators(struct btqca_power *qca,

Preferably keep use the same prefix (btqca_) consistently, unless
names become awfully long or otherwise unreadable.

>  static int qca_serdev_probe(struct serdev_device *serdev)
>  {
>	struct qca_serdev *qcadev;
>	const struct btqca_vreg_data *data;
> 	int err = 0;

unnecessary initialization

>		/* set voltage regulator status as false */
>		qca->vregs_on = false;

delete pointless comment

>		/* get operating speed */
>		device_property_read_u32(&serdev->dev, "oper-speed",

comment doesn't add much value

> +		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
> +		if (err) {
> +			BT_ERR("wcn3990 serdev registration failed");
> +			kfree(qca);
> +			goto out;

disable regulators

Thanks

Matthias

Re: [PATCH v5 2/3] Bluetooth: btqca: Add wcn3990 firmware download support.

[Balakrishna Godavarthi]

Hi Matthias,

as i was on vacation i couldn't able to reply to your comments.
find my comments inline.

On 2018-05-11 23:10, Matthias Kaehlcke wrote:
> Hi Bala,
> 
> On Fri, May 11, 2018 at 05:51:02PM +0530, Balakrishna Godavarthi wrote:
>> This patch enables the RAM and NV patch download for wcn3990.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>>  drivers/bluetooth/btqca.c | 46 
>> +++++++++++++++++++++++++++++++++++++++++++++-
>>  drivers/bluetooth/btqca.h | 13 +++++++++++++
>>  2 files changed, 58 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>> index 8219816..40c6b4f 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 rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
> 
> If this and other functions aren't really Rome specific they should
> probably be renamed to qca_...
> 
[Bala]:- will update the functions name in separate patch.

>> +int qca_uart_setup_cherokee(struct hci_dev *hdev, uint8_t baudrate,
>> +			    u32 *soc_ver)
>> +{
>> +	struct rome_config config;
>> +	int err;
>> +
>> +	/* we are using the existing funciton of ROME,
>> +	 * instead of duplicating the function for wcn3990.
>> +	 */
> 
> The comment isn't quite accurate, the qca_uart_setup_cherokee() calls
> existing functions, but is essentially a copy of qca_uart_setup_rome().
> 
> The main difference is that the patch version is passed as a parameter
> instead of determining it in the uart setup function. There seems to
> be no need to pass the version number in, or if you prefer to do it
> this way, you could change the Rome code to do this.
> 
> The other delta is the filename extension of the rampatch file, which
> is .bin for Rome and .tlv for Cherokee. Is there a good reason to use
> a different extension? If not just stick to the existing naming
> scheme, otherwise you could pass the chip type as a parameter and
> chose the extensions based on that.
> 
[Bala]:- as we see the process to setup of BTchip ROME and wcn3990 
setup.
          For Rome:
          1. change the operating speed to 3.0mbps.
          2. request the version
          3. download RAM patch (.bin extestion)
          4. downlaod NV patch (.bin extersnion)
          5. Reset ROME

         steps 2 to 5 are done in the function qca_uart_setup_rome()

         For WCN3990:
         1. Set baudrate to 115200.
         2. request the version
         3. set baudrate to operating speed.
         4. download RAM patch (.tlv extestion)
         5. downlaod NV patch (.bin extersnion)
         6. Reset wcn3990

        steps 2 is done in function rome_patch_ver_req(), where as steps 
4 to 6 qca_uart_setup_cherokee()

        the big difference is .bin extension for RAM patch is deprecated 
and BTCCHIP vendor will not support .bin for ram patch any more for 
latest chips.
        so that could be reason we using a separate function 
qca_uart_setup_cherokee(). if i use the same function by passing the 
chip name to decide the file extension.
        but again unnecessary i have to request the version command to 
BTCHIP.


> Thanks
> 
> Matthias

-- 
Regards
Balakrishna.

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

[Balakrishna Godavarthi]

Hi Marcel,

On 2018-05-11 23:58, Marcel Holtmann wrote:
> Hi Balakrishna,
> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> .../devicetree/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 0ea18a5..63cae49 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.
>> + - oper-speed: operating speed of the chip.
> 
> we actually agreed on using max-speed for these. And it should
> optional to limit speed for “broken” devices or that have some limit
> because of the platform.
> 
> Regards
> 
> Marcel

will update the same in incremental patch.

-- 
Regards
Balakrishna.

Re: [PATCH v5 3/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.

[Balakrishna Godavarthi]

Hi Marcel,

Find my comments in line.

On 2018-05-12 01:40, Marcel Holtmann wrote:
> Hi Balakrishna,
> 
>> Add support to set voltage/current of various regulators
>> to power up/down Bluetooth chip wcn3990.
>> Add support to read baudrate from dts.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> drivers/bluetooth/btqca.h   |   6 +
>> drivers/bluetooth/hci_qca.c | 547 
>> ++++++++++++++++++++++++++++++++++++++------
>> 2 files changed, 480 insertions(+), 73 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>> index 2a7366b..8e2142a 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_bardrate {
>> 	QCA_BAUDRATE_115200 	= 0,
>> 	QCA_BAUDRATE_57600,
>> @@ -124,6 +127,9 @@ struct tlv_type_hdr {
>> 	__u8   data[0];
>> } __packed;
>> 
>> +int btqca_power_setup(bool on);
> 
> my initial comment on this has not yet been addressed. They are all
> per hci_dev or I am not going to merge it.
> 
[Bala]- might have missed. will update.

>> +int qca_btsoc_cleanup(struct hci_dev *hdev);
>> +
>> #if IS_ENABLED(CONFIG_BT_QCA)
>> 
>> int qca_set_bdaddr_rome(struct hci_dev *hdev, const bdaddr_t *bdaddr);
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index f05382b..71f9591 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...
>> @@ -35,6 +35,11 @@
>> #include <linux/mod_devicetable.h>
>> #include <linux/module.h>
>> #include <linux/serdev.h>
>> +#include <linux/delay.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/of_device.h>
>> +#include <linux/device.h>
>> 
>> #include <net/bluetooth/bluetooth.h>
>> #include <net/bluetooth/hci_core.h>
>> @@ -119,12 +124,51 @@ struct qca_data {
>> 	u64 votes_off;
>> };
>> 
>> +enum btqca_soc_t {
>> +	BTQCA_INVALID = -1,
>> +	BTQCA_AR3002,
>> +	BTQCA_ROME,
>> +	BTQCA_CHEROKEE
>> +};
>> +
>> struct qca_serdev {
>> 	struct hci_uart	 serdev_hu;
>> 	struct gpio_desc *bt_en;
>> 	struct clk	 *susclk;
>> +	enum btqca_soc_t btsoc_type;
>> +	u32 init_speed;
>> +	u32 oper_speed;
>> +};
>> +
>> +/*
>> + * voltage regulator information required for configuring the
>> + * QCA bluetooth chipset
>> + */
>> +struct btqca_vreg {
>> +	const char *name;
>> +	unsigned int min_v;
>> +	unsigned int max_v;
>> +	unsigned int load_ua;
>> +};
>> +
>> +struct btqca_vreg_data {
>> +	enum btqca_soc_t soc_type;
>> +	struct btqca_vreg *vregs;
>> +	size_t num_vregs;
>> +};
>> +
>> +/*
>> + * Platform data for the QCA bluetooth power driver.
>> + */
>> +struct btqca_power {
>> +	struct device *dev;
>> +	const struct btqca_vreg_data *vreg_data;
>> +	struct regulator_bulk_data *vreg_bulk;
>> +	bool vregs_on;
>> };
>> 
>> +static struct btqca_power *qca;
>> +
> 
> No such global variable.
[Bala]:- will hook this variable to struct qca_uart_setup_cherokee().

> 
>> static void __serial_clock_on(struct tty_struct *tty)
>> {
>> 	/* TODO: Some chipset requires to enable UART clock on client
>> @@ -402,6 +446,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 +508,22 @@ 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 == BTQCA_CHEROKEE) {
>> +			hu->init_speed = qcadev->init_speed;
>> +			hu->oper_speed = qcadev->oper_speed;
>> +			ret = btqca_power_setup(true);
>> +			if (ret)
>> +				goto out;
> 
> Doing return btqca_power_setup is the same and less complicated.
> 

[Bala]: will update

>> +		} 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;
>> +out:
>> +	return ret;
>> }
>> 
>> static void qca_debugfs_init(struct hci_dev *hdev)
>> @@ -552,7 +606,10 @@ static int qca_close(struct hci_uart *hu)
>> 		serdev_device_close(hu->serdev);
>> 
>> 		qcadev = serdev_device_get_drvdata(hu->serdev);
>> -		gpiod_set_value_cansleep(qcadev->bt_en, 0);
>> +		if (qcadev->btsoc_type == BTQCA_CHEROKEE)
>> +			qca_btsoc_cleanup(hu->hdev);
>> +		else
>> +			gpiod_set_value_cansleep(qcadev->bt_en, 0);
>> 	}
>> 
>> 	kfree_skb(qca->rx_skb);
>> @@ -872,6 +929,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 +945,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;
> 
> The baud rate addition can be a separate patch.
> 

[Bala]:- will update.

>> 
>> 	cmd[4] = baudrate;
>> @@ -923,68 +982,260 @@ 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_DBG("%s:sending command %02x to SoC", hdev->name, cmd);
>> +
>> +	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
>> +	if (!skb) {
>> +		BT_ERR("Failed to allocate memory for skb  packet");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	skb_put_data(skb, &cmd, sizeof(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 */
>> +	set_current_state(TASK_UNINTERRUPTIBLE);
>> +	schedule_timeout(usecs_to_jiffies(100));
>> +	set_current_state(TASK_INTERRUPTIBLE);
> 
> Any reason to not use msleep.
> 

[Bala]- i can achieve the same with msleep too. will update.

>> +
>> +	return 0;
>> +}
>> +
>> +static int qca_serdev_open(struct hci_uart *hu)
>> +{
>> +	int ret = 0;
>> +
>> +	if (hu->serdev) {
>> +		serdev_device_open(hu->serdev);
>> +	} else {
>> +		bt_dev_err(hu->hdev, "open operation not supported");
>> +		ret = 1;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +int qca_btsoc_cleanup(struct hci_dev *hdev)
>> +{
>> +	struct hci_uart *hu = hci_get_drvdata(hdev);
>> +
>> +	/* change host baud rate before sending power off command */
>> +	host_set_baudrate(hu, 2400);
>> +	/* send 0xC0 command to btsoc before turning off regulators */
>> +	qca_send_vendor_cmd(hdev, CHEROKEE_POWEROFF_PULSE);
>> +	/* turn off btsoc */
>> +	return btqca_power_setup(false);
>> +}
>> +
>> +static int qca_serdev_close(struct hci_uart *hu)
>> +{
>> +	int ret = 0;
>> +
>> +	if (hu->serdev) {
>> +		serdev_device_close(hu->serdev);
>> +	} else {
>> +		bt_dev_err(hu->hdev, "close operation not supported");
>> +		ret = 1;
>> +	}
>> +
>> +	return ret;
>> +}
>> 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;
>> +
>> +	qcadev = serdev_device_get_drvdata(hu->serdev);
>> +
>> +	switch (qcadev->btsoc_type) {
>> +	case BTQCA_CHEROKEE:
>> +		bt_dev_info(hdev, "setting up wcn3990”);
> 
> Nope. This is debug level stuff.

[Bala]:- will update

> 
>> +		/* Patch downloading has to be done without IBS mode */
>> +		clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> +		/* Setup initial baudrate */
>> +		speed = 0;
>> +		if (hu->init_speed)
>> +			speed = hu->init_speed;
>> +		else if (hu->proto->init_speed)
>> +			speed = hu->proto->init_speed;
>> +
>> +		if (speed) {
>> +			host_set_baudrate(hu, speed);
>> +		} else {
>> +			bt_dev_err(hdev, "initial speed %u", speed);
>> +			return -1;
>> +		}
>> +
>> +		/* disable flow control, as chip is still not turned on */
>> +		hci_uart_set_flow_control(hu, true);
>> +		/* send 0xFC  command to btsoc */
>> +		ret = qca_send_vendor_cmd(hdev, CHEROKEE_POWERON_PULSE);
>> +		if (ret) {
>> +			bt_dev_err(hdev, "Failed to send power on command");
>> +			return ret;
>> +		}
>> +
>> +		/* close serial port */
>> +		ret = qca_serdev_close(hu);
>> +		if (ret)
>> +			return ret;
>> +		/* open serial port */
>> +		ret = qca_serdev_open(hu);
>> +		if (ret)
>> +			return ret;
>> 
>> -	bt_dev_info(hdev, "ROME setup");
>> +		/* Setup initial baudrate */
>> +		speed = 0;
>> +		if (hu->init_speed)
>> +			speed = hu->init_speed;
>> +		else if (hu->proto->init_speed)
>> +			speed = hu->proto->init_speed;
>> +		if (speed) {
>> +			host_set_baudrate(hu, speed);
>> +		} else {
>> +			BT_ERR("%s:initial speed %u", hdev->name, speed);
>> +			return -1;
>> +		}
>> 
>> -	/* Patch downloading has to be done without IBS mode */
>> -	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> +		/* Enable flow control */
>> +		hci_uart_set_flow_control(hu, false);
>> +		/*  wait until flow control settled */
>> +		msleep(100);
>> +		bt_dev_info(hdev, "wcn3990 Patch Version Request");
>> +		ret = rome_patch_ver_req(hdev, &soc_ver);
>> +		if (ret < 0 || soc_ver == 0) {
>> +			BT_ERR("%s: Failed to get version 0x%x", hdev->name,
>> +				ret);
>> +			return ret;
>> +		}
>> 
>> -	/* Setup initial baudrate */
>> -	speed = 0;
>> -	if (hu->init_speed)
>> -		speed = hu->init_speed;
>> -	else if (hu->proto->init_speed)
>> -		speed = hu->proto->init_speed;
>> +		bt_dev_info(hdev, "wcn3990 controller version 0x%08x", soc_ver);
>> +
>> +		/* clear flow control */
>> +		hci_uart_set_flow_control(hu, true);
>> +		/* set operating speed */
>> +		speed = 0;
>> +		if (hu->oper_speed)
>> +			speed = hu->oper_speed;
>> +		else if (hu->proto->oper_speed)
>> +			speed = hu->proto->oper_speed;
>> +		if (speed) {
>> +			qca_baudrate = qca_get_baudrate_value(speed);
>> +			bt_dev_info(hdev, "Set UART speed to %d", speed);
>> +			ret = qca_set_baudrate(hdev, qca_baudrate);
>> +			if (ret) {
>> +				BT_ERR("%s:Failed to change the baud rate(%d)",
>> +					hdev->name, ret);
>> +				return ret;
>> +			}
>> +			if (speed) {
>> +				host_set_baudrate(hu, speed);
>> +			} else {
>> +				BT_ERR("%s:Error in setting operator speed:%u",
>> +					hdev->name, speed);
>> +				return -1;
>> +			}
>> +		}
>> 
>> -	if (speed)
>> -		host_set_baudrate(hu, speed);
>> +		/* Set flow control */
>> +		hci_uart_set_flow_control(hu, false);
>> +		/* Setup patch and NVM configurations */
>> +		ret = qca_uart_setup_cherokee(hdev, qca_baudrate, &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 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;
>> +		/* Setup wcn3990 bdaddr */
>> +		hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
>> 
>> -	if (speed) {
>> -		qca_baudrate = qca_get_baudrate_value(speed);
>> +		return ret;
>> 
>> -		bt_dev_info(hdev, "Set UART speed to %d", speed);
>> -		ret = qca_set_baudrate(hdev, qca_baudrate);
>> -		if (ret) {
>> -			bt_dev_err(hdev, "Failed to change the baud rate (%d)",
>> -				   ret);
>> +	default:
>> +		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 */
>> +		speed = 0;
>> +		if (hu->init_speed)
>> +			speed = hu->init_speed;
>> +		else if (hu->proto->init_speed)
>> +			speed = hu->proto->init_speed;
>> +
>> +		if (speed)
>> +			host_set_baudrate(hu, speed);
>> +
>> +		/* 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;
>> +
>> +		if (speed) {
>> +			qca_baudrate = qca_get_baudrate_value(speed);
>> +
>> +			bt_dev_info(hdev, "Set UART speed to %d", speed);
>> +			ret = qca_set_baudrate(hdev, qca_baudrate);
>> +			if (ret) {
>> +				bt_dev_err(hdev, "Failed to change the baud rate (%d)",
>> +					    ret);
>> 			return ret;
>> +			}
>> +			host_set_baudrate(hu, speed);
>> 		}
>> -		host_set_baudrate(hu, speed);
>> -	}
>> 
>> -	/* Setup patch / NVM configurations */
>> -	ret = qca_uart_setup_rome(hdev, qca_baudrate);
>> -	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 patch / NVM configurations */
>> +		ret = qca_uart_setup_rome(hdev, qca_baudrate);
>> +		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;
>> +		/* Setup bdaddr */
>> +		hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
>> 
>> -	return ret;
>> +		return ret;
>> +	}
>> }
> 
> This change is so complex and convoluted that I can not review it.
> 
>> 
>> static struct hci_uart_proto qca_proto = {
>> @@ -1002,44 +1253,189 @@ static int qca_setup(struct hci_uart *hu)
>> 	.dequeue	= qca_dequeue,
>> };
>> 
>> +static const struct btqca_vreg_data cherokee_data = {
>> +	.soc_type = BTQCA_CHEROKEE,
>> +	.vregs = (struct btqca_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 btqca_enable_regulators(int i, struct btqca_vreg *vregs)
>> +{
>> +	int ret = 0;
>> +
>> +	ret = regulator_set_voltage(qca->vreg_bulk[i].consumer, 
>> vregs[i].min_v,
>> +				    vregs[i].max_v);
>> +	if (ret)
>> +		goto out;
>> +
>> +	if (vregs[i].load_ua)
>> +		ret = regulator_set_load(qca->vreg_bulk[i].consumer,
>> +					 vregs[i].load_ua);
>> +
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = regulator_enable(qca->vreg_bulk[i].consumer);
>> +
>> +out:
>> +	return ret;
>> +
>> +}
>> +
>> +static void btqca_disable_regulators(int reg_nu, struct btqca_vreg 
>> *vregs)
>> +{
>> +	int i;
>> +
>> +	/* disable the regulator if requested by user
>> +	 * or when fault to enable any regulator.
>> +	 */
>> +	for (i = reg_nu - 1; i >= 0 ; i--) {
>> +		regulator_disable(qca->vreg_bulk[i].consumer);
>> +		regulator_set_voltage(qca->vreg_bulk[i].consumer, 0,
>> +				      vregs[i].max_v);
>> +		if (vregs[i].load_ua)
>> +			regulator_set_load(qca->vreg_bulk[i].consumer, 0);
>> +	}
>> +}
>> +
>> +int btqca_power_setup(bool on)
>> +{
>> +	int ret = 0;
>> +	int i;
>> +	struct btqca_vreg *vregs;
>> +
>> +	if (!qca || !qca->vreg_data || !qca->vreg_bulk)
>> +		return -EINVAL;
>> +	vregs = qca->vreg_data->vregs;
>> +
>> +	BT_DBG("on: %d", on);
>> +	/* turn on if regualtors are off */
>> +	if (on == true && qca->vregs_on == false) {
>> +		qca->vregs_on = true;
>> +		for (i = 0; i < qca->vreg_data->num_vregs; i++) {
>> +			ret = btqca_enable_regulators(i, vregs);
>> +			if (ret)
>> +				break;
>> +		}
>> +	} else if (on == false && qca->vregs_on == true) {
>> +		qca->vregs_on = false;
>> +		/* turn off regulator in reverse order */
>> +		btqca_disable_regulators(qca->vreg_data->num_vregs, vregs);
>> +	}
> 
> I do not get this function separation. Please clean this up. And use x
> and !x instead of == false and == true.

[Bala]:will update

> 
>> +
>> +	/* regulators failed */
>> +	if (ret) {
>> +		qca->vregs_on = false;
>> +		BT_ERR("failed to enable regulator:%s", vregs[i].name);
>> +		/* turn off regulators which are enabled */
>> +		btqca_disable_regulators(i, vregs);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int init_regulators(struct btqca_power *qca,
>> +			   const struct btqca_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;
>> -	int err;
>> +	const struct btqca_vreg_data *data;
>> +	int err = 0;
>> 
>> 	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
>> 	if (!qcadev)
>> 		return -ENOMEM;
>> 
>> 	qcadev->serdev_hu.serdev = serdev;
>> +	data = of_device_get_match_data(&serdev->dev);
>> +	if (data && data->soc_type == BTQCA_CHEROKEE)
>> +		qcadev->btsoc_type = BTQCA_CHEROKEE;
>> +	else
>> +		qcadev->btsoc_type = BTQCA_ROME;
>> +
>> 	serdev_device_set_drvdata(serdev, qcadev);
>> +	if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
>> +		qca = devm_kzalloc(&serdev->dev, sizeof(struct btqca_power),
>> +				   GFP_KERNEL);
>> +		if (!qca)
>> +			return -ENOMEM;
>> +
>> +		qca->dev = &serdev->dev;
>> +		qca->vreg_data = data;
>> +		err = init_regulators(qca, data->vregs, data->num_vregs);
>> +		if (err) {
>> +			BT_ERR("Failed to init regulators:%d", err);
>> +			kfree(qca);
>> +			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);
>> -	}
>> +		/* set voltage regulator status as false */
>> +		qca->vregs_on = false;
>> +		/* get operating speed */
>> +		device_property_read_u32(&serdev->dev, "oper-speed",
>> +					 &qcadev->oper_speed);
>> +		device_property_read_u32(&serdev->dev, "init-speed",
>> +					 &qcadev->init_speed);
> 
> Not even defined in DT.

[Bala]: will update in dt,bindings .txt file.

> 
>> +		if (!qcadev->oper_speed)
>> +			BT_INFO("UART will pick default proto operating speed");
>> +		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
>> +		if (err) {
>> +			BT_ERR("wcn3990 serdev registration failed");
>> +			kfree(qca);
>> +			goto out;
>> +		}
>> 
>> -	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);
>> -	}
>> +	} 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);
>> +		}
>> 
>> -	err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
>> -	if (err)
>> -		return err;
>> +		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_prepare_enable(qcadev->susclk);
>> -	if (err)
>> -		return err;
>> +		err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
>> +		if (err)
>> +			return err;
>> 
>> -	err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
>> -	if (err)
>> -		clk_disable_unprepare(qcadev->susclk);
>> +		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);
>> +	}
>> +
>> +out:	return err;
>> 
>> -	return err;
>> }
>> 
>> static void qca_serdev_remove(struct serdev_device *serdev)
>> @@ -1047,12 +1443,17 @@ 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);
>> -
>> -	clk_disable_unprepare(qcadev->susclk);
>> +	if (qcadev->btsoc_type == BTQCA_CHEROKEE) {
>> +		btqca_power_setup(false);
> 
> I am really not merging this. A driver that supports only one instance
> is not okay.

[Bala]: will study on this.

> 
>> +		kfree(qca);
>> +	} else {
>> +		clk_disable_unprepare(qcadev->susclk);
>> +	}
>> }
>> 
>> static const struct of_device_id qca_bluetooth_of_match[] = {
>> 	{ .compatible = "qcom,qca6174-bt" },
>> +	{ .compatible = "qcom,wcn3990-bt", .data = &cherokee_data},
>> 	{ /* sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);
> 
> Regards
> 
> Marcel

-- 
Regards
Balakrishna.

Re: [PATCH v5 3/3] Bluetooth: hci_qca: Add support for Qualcomm Bluetooth chip wcn3990.

[Balakrishna Godavarthi]

Hi Matthias,

please find my comment inline.

On 2018-05-12 02:55, Matthias Kaehlcke wrote:
> Hi Bala,
> 
> On Fri, May 11, 2018 at 05:51:03PM +0530, Balakrishna Godavarthi wrote:
>> Add support to set voltage/current of various regulators
>> to power up/down Bluetooth chip wcn3990.
>> Add support to read baudrate from dts.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
> 
> Please include a change log also in the individual patches, not just
> in the cover letter. In a revision after many comments it may be
> sufficient to say "addressed <reviewers> comments", if the number if
> changes is limited it is preferable to briefly list the individual
> changes.
> 

[Bala]: will add from next patch set.

>>  drivers/bluetooth/btqca.h   |   6 +
>>  drivers/bluetooth/hci_qca.c | 547 
>> ++++++++++++++++++++++++++++++++++++++------
>>  2 files changed, 480 insertions(+), 73 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>> index 2a7366b..8e2142a 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)
> 
> The parentheses are not needed around a literal. That they are used
> for the other values is IMO no good reason to introduce more of them.
> You might want to squeeze in a clean up patch that removes them for
> the other defines.
> 
>> +int btqca_power_setup(bool on);
>> +int qca_btsoc_cleanup(struct hci_dev *hdev);
> 
> nit: inconsistent naming.
> 
> If maintainers and authors have no objections you might want to
> squeeze in a patch that renames everything to btqca. Just a suggestion
> though.
> 

[bala]: changed the name of function to qca_power_setup

>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index f05382b..71f9591 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...
>> @@ -35,6 +35,11 @@
>>  #include <linux/mod_devicetable.h>
>>  #include <linux/module.h>
>>  #include <linux/serdev.h>
>> +#include <linux/delay.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/of_device.h>
>> +#include <linux/device.h>
> 
> As I mentioned on v4, the includes should be in alphabetical order.
> 

[Bala]: some thing like this?

     #include <linux/delay.h>
     #include <linux/device.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>


>> +enum btqca_soc_t {
> 
> nit: Again, inconsistent naming, some functions/structs are called
> qca_... other btqca_... Personally I prefer btqca_ to avoid clashes.
> 
[Bala]: will append qca_.* for the function and structure which are 
newly added. to aling with already existing functions and strcutures
        so btqca_soc_t will be qca_soc_type.
        let me know your comments on this.


>> +/*
>> + * voltage regulator information required for configuring the
>> + * QCA bluetooth chipset
> 
> nit: Voltage, Bluetooth
> 

[Bala]: will update

>> + */
>> +struct btqca_vreg {
>> +	const char *name;
>> +	unsigned int min_v;
>> +	unsigned int max_v;
>> +	unsigned int load_ua;
>> +};
> 
> nit: consider min_uV, max_uV, load_uA (as in the regulator framework).
> 

[Bala]: will update

>> +
>> +struct btqca_vreg_data {
>> +	enum btqca_soc_t soc_type;
>> +	struct btqca_vreg *vregs;
>> +	size_t num_vregs;
>> +};
>> +
>> +/*
>> + * Platform data for the QCA bluetooth power driver.
> 
> nit: Bluetooth
> 

[Bala]: 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_DBG("%s:sending command %02x to SoC", hdev->name, cmd);
> 
> bt_dev_dbg()
> 

[Bala]: will update

>> +
>> +	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
>> +	if (!skb) {
>> +		BT_ERR("Failed to allocate memory for skb  packet");
> 
> bt_dev_err()
> 

[Bala]: will update

>> +	/* Wait for 100 us for soc to settle down */
> 
> nit: uS, SoC
> 

[Bala]: will update

>> +static int qca_serdev_open(struct hci_uart *hu)
>> +{
>> +	int ret = 0;
>> +
>> +	if (hu->serdev) {
>> +		serdev_device_open(hu->serdev);
>> +	} else {
>> +		bt_dev_err(hu->hdev, "open operation not supported");
>> +		ret = 1;
>> +	}
>> +
>> +	return ret;
>> +}
> 
> From v4:
> 
>> > Check return value.
>> [Bala]: as we are not checking in qca_open, the return in this
>> function is to know the serdev function availability.
> 
> Sorry, your comment didn't really clarify things for me. The function
> is called from qca_setup() apparently with the intention to open the
> serial port, not to check if the function is available. If the serial
> port can't be opened for whatever reason the function should return an
> error (typically a negative value).
> 
> Also the error message is misleading, the underlying problem is not
> that the open operations is not supported, but that no serdev device
> is associated with the hci_uart. Actually it seems this could never
> happen:
> 
> In qca_serdev_probe():
> 
>   qcadev->serdev_hu.serdev = serdev;
> 
> And qca_serdev_open() is only called from qca_setup(). Basically the
> first thing qca_setup() does is:
> 
>   qcadev = serdev_device_get_drvdata(hu->serdev);
> 
> Thus if hu->serdev is NULL qca_setup() will crash shortly after:
> 
>   switch (qcadev->btsoc_type) {
> 
> I think we can get rid of qca_serdev_open/close() and just call
> directly serdev_device_open().
> 

[Bala]: yes your correct. will call the serdev_.* close and open from 
qca_setup()
         Do we really need to check hu->serdev == NULL in qca_setup?

>> +int qca_btsoc_cleanup(struct hci_dev *hdev)
>> +{
>> +	struct hci_uart *hu = hci_get_drvdata(hdev);
>> +
>> +	/* change host baud rate before sending power off command */
>> +	host_set_baudrate(hu, 2400);
>> +	/* send 0xC0 command to btsoc before turning off regulators */
>> +	qca_send_vendor_cmd(hdev, CHEROKEE_POWEROFF_PULSE);
> 
> The comment doesn't provide any useful information. From the code it's
> evident that a CHEROKEE_POWEROFF_PULSE is sent, if anybody is
> interested in the hex code they can look up the definition of
> CHEROKEE_POWEROFF_PULSE.
> 

[Bala]: will remove the comment.

>> +	/* turn off btsoc */
>> +	return btqca_power_setup(false);
> 
> This comment laso doesn't provide any value.
> 

[Bala]: will remove

>> +static int qca_serdev_close(struct hci_uart *hu)
>> +{
>> +	int ret = 0;
>> +
>> +	if (hu->serdev) {
>> +		serdev_device_close(hu->serdev);
>> +	} else {
>> +		bt_dev_err(hu->hdev, "close operation not supported");
>> +		ret = 1;
>> +	}
>> +
>> +	return ret;
>> +}
> 
> Same commants as for open().
> 
> Preferably keep open() and close() together, without squeezing the
> cleanup function in between.
> 

[Bala]: will remove these functions and directly call them from serdev_* 
close or open from qca_setup()


>>  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;
>> +
>> +	qcadev = serdev_device_get_drvdata(hu->serdev);
>> +
>> +	switch (qcadev->btsoc_type) {
>> +	case BTQCA_CHEROKEE:
> 
> I still thinks this is has a lot of common code with the default/Rome
> case, but let's first clarify and possibly improve a few things.
> 

[Bala]: actually i haven't edited any part of code for ROME i.e. default 
case expect the indentation.
         Yes we have little common code, but if i do any changes to ROME 
code i.e. default case,i don't have ROME Chip to test.
         i am little bit worried if some thing in ROME got broken with my 
change. that will create problem.


>> +		bt_dev_info(hdev, "setting up wcn3990");
>> +		/* Patch downloading has to be done without IBS mode */
>> +		clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
>> +		/* Setup initial baudrate */
>> +		speed = 0;
>> +		if (hu->init_speed)
>> +			speed = hu->init_speed;
>> +		else if (hu->proto->init_speed)
>> +			speed = hu->proto->init_speed;
> 
> This pattern is repeated a several times for initial and operating
> speeds. Helper functions like btqca_get_init_speed() and
> btqca_get_oper_speed() would make the code more readable and compact.

[Bala]: will create a new functions.

> 
>> +
>> +		if (speed) {
>> +			host_set_baudrate(hu, speed);
>> +		} else {
>> +			bt_dev_err(hdev, "initial speed %u", speed);
>> +			return -1;
>> +		}
> 
> Note: Up to here Rome and Cherokee do exactly the same.
> 
>> +		/* disable flow control, as chip is still not turned on */
>> +		hci_uart_set_flow_control(hu, true);
>> +		/* send 0xFC  command to btsoc */
>> +		ret = qca_send_vendor_cmd(hdev, CHEROKEE_POWERON_PULSE);
> 
> Delete useless comment.
> 

[Bala]: will update.

>> +		if (ret) {
>> +			bt_dev_err(hdev, "Failed to send power on command");
>> +			return ret;
>> +		}
>> +
>> +		/* close serial port */
>> +		ret = qca_serdev_close(hu);
>> +		if (ret)
>> +			return ret;
>> +		/* open serial port */
>> +		ret = qca_serdev_open(hu);
>> +		if (ret)
>> +			return ret;
> 
> The comments are pointless, it is evident from the code that the
> serial port is opened/closed. Much more interesting would be to
> explain why it is necessary to close and re-open the port.
> 

[Bala]: will update.

>> -	bt_dev_info(hdev, "ROME setup");
>> +		/* Setup initial baudrate */
>> +		speed = 0;
>> +		if (hu->init_speed)
>> +			speed = hu->init_speed;
>> +		else if (hu->proto->init_speed)
>> +			speed = hu->proto->init_speed;
>> +		if (speed) {
>> +			host_set_baudrate(hu, speed);
>> +		} else {
>> +			BT_ERR("%s:initial speed %u", hdev->name, speed);
> 
> bt_dev_err()
> 
>> +			return -1;
>> +		}
> 
> The initial baudrate has already been set above, is it necessary to
> configure it again because the port was closed?
> 

[Bala]: for safer side we setting init speed again after close and open.

> This baudrate code is extremely noisy. Besides introducing the helper
> functions mentioned above you could log the error message in
> host_set_baudrate() or even have an addtional wrapper that determines
> and sets the baudrate. The latter would reduce the above to:
> 
> if (xyz_set_baudrate(hu, INIT_RATE))
> 	return -1;
> 
> And that multiple times.
> 

[Bala]: will create wrapper to handle baudrate setting.

>> +		/* clear flow control */
>> +		hci_uart_set_flow_control(hu, true);
>> +		/* set operating speed */
> 
> Note: Starting from here the code for Cherokee and Rome is essentially
> the same, except for enabling flow control.
> 
> If you prefer keep the somewhat redundant code paths in the next
> revision, but please consider the improvements I suggested. With less
> noisy code it will be easier to determine if it is reasonable to join
> the two code paths.
> 

[Bala]: will try to use common code.

>> +static int btqca_enable_regulators(int i, struct btqca_vreg *vregs)
> 
> The common convention is to pass first the structure a function acts
> on, then other parameters.  Use a more expressive name for the
> number of regulators, like 'num_regs' or 'nregs'.
> 
> 

[Bala]:will update.

>> +{
>> +	int ret = 0;
> 
> Initialization is not necessary, ret is assigned in the next line.
> 

[Bala]:will remove.

>> +static void btqca_disable_regulators(int reg_nu, struct btqca_vreg 
>> *vregs)
>> +{
> 
> Same as for the enable function. Please use the same name for the
> argument with the number of regulators on both functions, personally I
> don't think 'reg_nu' is a great choice, but that's just my opinion ;-)
> 

[Bala]: in btqca_enable_regulator() we are enabling one regulator. i.e. 
this is called by btqc_power_setup() function.
         in btqca_disable_regulator() we are disabling all the 
regulators.
         i think it require a checkup. will update both in the same way.

>> +int btqca_power_setup(bool on)
>> +{
>> +	int ret = 0;
>> +	int i;
>> +	struct btqca_vreg *vregs;
>> +
>> +	if (!qca || !qca->vreg_data || !qca->vreg_bulk)
>> +		return -EINVAL;
>> +	vregs = qca->vreg_data->vregs;
>> +
>> +	BT_DBG("on: %d", on);
>> +	/* turn on if regualtors are off */
> 
> regulators
> 
> Consider dropping the comment, IMO the code speaks for itself.
> 

[Bala]: will remove

>> +	if (on == true && qca->vregs_on == false) {
> 
> if (on && !qca->vreg_on) {
> 
>> +		qca->vregs_on = true;
> 
> Typically you'd change the variable after having performed the
> operation with success.
> 

[Bala]: will update.

>> +		for (i = 0; i < qca->vreg_data->num_vregs; i++) {
>> +			ret = btqca_enable_regulators(i, vregs);
>> +			if (ret)
>> +				break;
>> +		}
>> +	} else if (on == false && qca->vregs_on == true) {
> 
> if (!on && qca->vreg_on) {
> 
>> +		qca->vregs_on = false;
> 
> Better done after having disabled the regulators.

[Bala]: will update.

> 
>> +		/* turn off regulator in reverse order */
>> +		btqca_disable_regulators(qca->vreg_data->num_vregs, vregs);
>> +	}
>> +
>> +	/* regulators failed */
>> +	if (ret) {
>> +		qca->vregs_on = false;
>> +		BT_ERR("failed to enable regulator:%s", vregs[i].name);
>> +		/* turn off regulators which are enabled */
>> +		btqca_disable_regulators(i, vregs);
>> +	}
> 
> Why not do this directly when the problem is detected? The code also
> relies on 'ret' only being set in the 'on' path. Which isn't intuitive
> for the reader and *might* change in the future.
> 

[Bala]: will copy the same in on block.


>> +
>> +	return ret;
>> +}
>> +
>> +static int init_regulators(struct btqca_power *qca,
> 
> Preferably keep use the same prefix (btqca_) consistently, unless
> names become awfully long or otherwise unreadable.
> 
[Bala]: will rename as qca_init_regulators() and also changes functions 
starting with btqca_.* to qca_.*

>>  static int qca_serdev_probe(struct serdev_device *serdev)
>>  {
>> 	struct qca_serdev *qcadev;
>> 	const struct btqca_vreg_data *data;
>> 	int err = 0;
> 
> unnecessary initialization

[Bala]: will remove.

> 
>> 		/* set voltage regulator status as false */
>> 		qca->vregs_on = false;
> 
> delete pointless comment

[Bala]: will remove

> 
>> 		/* get operating speed */
>> 		device_property_read_u32(&serdev->dev, "oper-speed",
> 
> comment doesn't add much value
> 

[Bala]: will remove

>> +		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
>> +		if (err) {
>> +			BT_ERR("wcn3990 serdev registration failed");
>> +			kfree(qca);
>> +			goto out;
> 
> disable regulators

[Bala]: we didn't enable regulators yet. we just get the regulators 
address. explicitly disabling the regulators is not safe.

> 
> Thanks
> 
> Matthias

-- 
Regards
Balakrishna.

Re: [PATCH v5 2/3] Bluetooth: btqca: Add wcn3990 firmware download support.

[Matthias Kaehlcke]

On Fri, May 18, 2018 at 08:04:46PM +0530, Balakrishna Godavarthi wrote:

> as i was on vacation i couldn't able to reply to your comments.
> find my comments inline.

Also sorry for my late reply

> On 2018-05-11 23:10, Matthias Kaehlcke wrote:
> > Hi Bala,
> > 
> > On Fri, May 11, 2018 at 05:51:02PM +0530, Balakrishna Godavarthi wrote:
> > > This patch enables the RAM and NV patch download for wcn3990.
> > > 
> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > > ---
> > >  drivers/bluetooth/btqca.c | 46
> > > +++++++++++++++++++++++++++++++++++++++++++++-
> > >  drivers/bluetooth/btqca.h | 13 +++++++++++++
> > >  2 files changed, 58 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> > > index 8219816..40c6b4f 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 rome_patch_ver_req(struct hci_dev *hdev, u32 *rome_version)
> > 
> > If this and other functions aren't really Rome specific they should
> > probably be renamed to qca_...
> > 
> [Bala]:- will update the functions name in separate patch.
> 
> > > +int qca_uart_setup_cherokee(struct hci_dev *hdev, uint8_t baudrate,
> > > +			    u32 *soc_ver)
> > > +{
> > > +	struct rome_config config;
> > > +	int err;
> > > +
> > > +	/* we are using the existing funciton of ROME,
> > > +	 * instead of duplicating the function for wcn3990.
> > > +	 */
> > 
> > The comment isn't quite accurate, the qca_uart_setup_cherokee() calls
> > existing functions, but is essentially a copy of qca_uart_setup_rome().
> > 
> > The main difference is that the patch version is passed as a parameter
> > instead of determining it in the uart setup function. There seems to
> > be no need to pass the version number in, or if you prefer to do it
> > this way, you could change the Rome code to do this.
> > 
> > The other delta is the filename extension of the rampatch file, which
> > is .bin for Rome and .tlv for Cherokee. Is there a good reason to use
> > a different extension? If not just stick to the existing naming
> > scheme, otherwise you could pass the chip type as a parameter and
> > chose the extensions based on that.
> > 
> [Bala]:- as we see the process to setup of BTchip ROME and wcn3990 setup.
>          For Rome:
>          1. change the operating speed to 3.0mbps.
>          2. request the version
>          3. download RAM patch (.bin extestion)
>          4. downlaod NV patch (.bin extersnion)
>          5. Reset ROME
> 
>         steps 2 to 5 are done in the function qca_uart_setup_rome()
> 
>         For WCN3990:
>         1. Set baudrate to 115200.
>         2. request the version
>         3. set baudrate to operating speed.
>         4. download RAM patch (.tlv extestion)
>         5. downlaod NV patch (.bin extersnion)
>         6. Reset wcn3990
> 
>        steps 2 is done in function rome_patch_ver_req(), where as steps 4 to
> 6 qca_uart_setup_cherokee()
> 
>        the big difference is .bin extension for RAM patch is deprecated and
> BTCCHIP vendor will not support .bin for ram patch any more for latest
> chips.
>        so that could be reason we using a separate function
> qca_uart_setup_cherokee(). if i use the same function by passing the chip
> name to decide the file extension.
>        but again unnecessary i have to request the version command to
> BTCHIP.

You could just pass the version as parameter in both cases, Rome and
Cherokee could go through their respective hoops to obtain it before
calling qca_uart_setup().

The different extension and format strings could easily be fixed. I
admit though that the length of the function is on the edge of where
the redundance is an issue, so if Marcel is fine with it I won't inisit.

(slightly off-topic: is qca_uart_setup_xyz() actually the appropriate
name for the function? Admittedly I know little about the platform,
but it seems the most important thing the function does is to load the
two firmware files, which sounds more like qca_setup_xyz() ...)