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

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

[Balakrishna Godavarthi]

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

The below are difference between new and old chips.

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

 *Note: Turning on sequence differs between two chips.

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

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

v10:
 * update current load with actual values of WCN3990.
 * added support  for itterative connect with BT chip if it fails to respond to the HOST.
 * Added regulator current dtbindings.
 * addressed review comments.

v9:
 * removed smps regs.
 * hidded flow control in respective functions.
 * moved qca_soc_ver to qca_setup
 * addressed review comments.
 
v8:
  * Squashed [v7 2/8] and [v7 3/8] to one patch.
  * updated functions to set and get the UART speeds.
  * addressed review comments.

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

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

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

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

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

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


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

 .../bindings/net/qualcomm-bluetooth.txt       |  42 +-
 drivers/bluetooth/btqca.c                     | 116 ++--
 drivers/bluetooth/btqca.h                     |  23 +-
 drivers/bluetooth/hci_qca.c                   | 564 ++++++++++++++++--
 4 files changed, 629 insertions(+), 116 deletions(-)

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

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

[Balakrishna Godavarthi]

This patch enables regulators for the Qualcomm Bluetooth wcn3990
controller.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v10:
    * added entry for regulator currents

Changes in v9:
    * updated with latest reg handle and names.
    * updated max-speed definition. 

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

Changes in v7:
    * no change.

Changes in v6:

    * Changed the oper-speed to max-speed.

Changes in v5:

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

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

diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
index 0ea18a53cc29..ca04c4981048 100644
--- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
+++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
@@ -10,12 +10,34 @@ device the slave device is attached to.
 Required properties:
  - compatible: should contain one of the following:
    * "qcom,qca6174-bt"
+   * "qcom,wcn3990-bt"
+
+Optional properties for compatible string qcom,qca6174-bt:
 
-Optional properties:
  - enable-gpios: gpio specifier used to enable chip
  - clocks: clock provided to the controller (SUSCLK_32KHZ)
 
-Example:
+Optional properties for compatible string qcom,wcn3990-bt:
+
+ - vddio-supply: Bluetooth wcn3990 VDD_IO supply regulator handle.
+ - vddxo-supply: Bluetooth wcn3990 VDD_XO supply regulator handle.
+ - vddrf-supply: Bluetooth wcn3990 VDD_RF supply regulator handle.
+ - vddch0-supply: Bluetooth wcn3990 VDD_CH0 supply regulator handle.
+
+ - If WCN3990 is connected to platform where RPMH PMIC processor is used
+   then the load values will be 1uA. if it is connected to platform where RPM
+   PMIC processor is used then load value will be 10000 uA.
+   if it is connected to different platform, where current values are fixed
+   as in data sheet then below property are not required.
+
+   - vddio-current: Bluetooth wcn3990 VDD_IO current level in uA.
+   - vddxo-current: Bluetooth wcn3990 VDD_XO current level in uA.
+   - vddrf-current: Bluetooth wcn3990 VDD_RF current level in uA.
+   - vddch0-current: Bluetooth wcn3990 VDD_CH0 current level in uA.
+
+ - max-speed: see Documentation/devicetree/bindings/serial/slave-device.txt
+
+Examples:
 
 serial@7570000 {
 	label = "BT-UART";
@@ -28,3 +50,19 @@ serial@7570000 {
 		clocks = <&divclk4>;
 	};
 };
+
+serial@898000 {
+	bluetooth {
+		compatible = "qcom,wcn3990-bt";
+
+		vddio-supply = <&vreg_s4a_1p8>;
+		vddxo-supply = <&vreg_l7a_1p8>;
+		vddrf-supply = <&vreg_l17a_1p3>;
+		vddch0-supply = <&vreg_l25a_3p3>;
+		vddio-current = <1>;
+		vddxo-current = <1>;
+		vddrf-current = <1>;
+		vddch0-current = <1>;
+		max-speed = <3200000>;
+	};
+};
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Matthias Kaehlcke]

On Fri, Jul 20, 2018 at 07:02:37PM +0530, Balakrishna Godavarthi wrote:
> This patch enables regulators for the Qualcomm Bluetooth wcn3990
> controller.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> Changes in v10:
>     * added entry for regulator currents
> 
> Changes in v9:
>     * updated with latest reg handle and names.
>     * updated max-speed definition. 
> 
> Changes in v8:
>     * Separated the optional entries between two chips
> 
> Changes in v7:
>     * no change.
> 
> Changes in v6:
> 
>     * Changed the oper-speed to max-speed.
> 
> Changes in v5:
> 
>     * Added entry for oper-speed and init-speed.
> 
> ---
>  .../bindings/net/qualcomm-bluetooth.txt       | 42 ++++++++++++++++++-
>  1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> index 0ea18a53cc29..ca04c4981048 100644
> --- a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> @@ -10,12 +10,34 @@ device the slave device is attached to.
>  Required properties:
>   - compatible: should contain one of the following:
>     * "qcom,qca6174-bt"
> +   * "qcom,wcn3990-bt"
> +
> +Optional properties for compatible string qcom,qca6174-bt:
>  
> -Optional properties:
>   - enable-gpios: gpio specifier used to enable chip
>   - clocks: clock provided to the controller (SUSCLK_32KHZ)
>  
> -Example:
> +Optional properties for compatible string qcom,wcn3990-bt:
> +
> + - vddio-supply: Bluetooth wcn3990 VDD_IO supply regulator handle.
> + - vddxo-supply: Bluetooth wcn3990 VDD_XO supply regulator handle.
> + - vddrf-supply: Bluetooth wcn3990 VDD_RF supply regulator handle.
> + - vddch0-supply: Bluetooth wcn3990 VDD_CH0 supply regulator handle.
> +
> + - If WCN3990 is connected to platform where RPMH PMIC processor is used
> +   then the load values will be 1uA. if it is connected to platform where RPM
> +   PMIC processor is used then load value will be 10000 uA.
> +   if it is connected to different platform, where current values are fixed
> +   as in data sheet then below property are not required.

Please provide details why these magic values are needed for RPMh and
RPM PMICs.

For RPMh it looks like a value of 1uA would cause the regulator to
enter idle mode:

static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA)
{
	struct rpmh_vreg *vreg = rdev_get_drvdata(rdev);
	unsigned int mode;

	if (load_uA >= vreg->hw_data->hpm_min_load_uA)
		mode = REGULATOR_MODE_NORMAL;
	else
		mode = REGULATOR_MODE_IDLE;

	return rpmh_regulator_vrm_set_mode(rdev, mode);
}

https://patchwork.kernel.org/patch/10524299/

Is that really intended and if so why? It might make sense to save
power when really in idle mode, but I assume you somehow have to tell
the regulator to switch to normal mode when BT is used.

I also commented about this on patch "[7/7] Bluetooth: hci_qca: Add
support for Qualcomm Bluetooth chip wcn3990". I suggest to center the
discussion here and reply with a link to the other patch.

[PATCH v10 2/7] Bluetooth: btqca: Rename ROME specific functions to generic functions

[Balakrishna Godavarthi]

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

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/bluetooth/btqca.c   | 85 ++++++++++++++++++-------------------
 drivers/bluetooth/btqca.h   | 10 ++++-
 drivers/bluetooth/hci_qca.c |  2 +-
 3 files changed, 51 insertions(+), 46 deletions(-)

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

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

[Balakrishna Godavarthi]

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

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

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
---
 drivers/bluetooth/btqca.c   | 20 +++++++-------------
 drivers/bluetooth/btqca.h   | 13 +++++++++++--
 drivers/bluetooth/hci_qca.c | 10 +++++++++-
 3 files changed, 27 insertions(+), 16 deletions(-)

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

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

[Matthias Kaehlcke]

On Fri, Jul 20, 2018 at 07:02:39PM +0530, Balakrishna Godavarthi wrote:
> Redefinition of qca_uart_setup will help future Qualcomm Bluetooth
> SoC, to use the same function instead of duplicating the function.
> Added new arguments soc_type and soc_ver to the functions.
> 
> These arguments will help to decide type of firmware files
> to be loaded into Bluetooth chip.
> soc_type holds the Bluetooth chip connected to APPS processor.
> soc_ver holds the Bluetooth chip version.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/bluetooth/btqca.c   | 20 +++++++-------------
>  drivers/bluetooth/btqca.h   | 13 +++++++++++--
>  drivers/bluetooth/hci_qca.c | 10 +++++++++-
>  3 files changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index c5cf9cab438a..b556710ee1bd 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -85,6 +85,9 @@ int qca_read_soc_version(struct hci_dev *hdev, u32 *soc_version)
>  out:
>  	kfree_skb(skb);
>  
> +	if (err < 0 || *soc_version == 0)
> +		bt_dev_err(hdev, "QCA Failed to get version (%d)", err);

You also have to set 'err' if soc_version is 0, so the caller can skip
the check for soc_version == 0

I'd suggest:

  // directly after setting soc_version
  if (*soc_version == 0)
    err = -EILSEQ; // or should it be a different error code?

  ...

  if (err)
    bt_dev_err(hdev, "QCA Failed to get version (%d)", err);

You could also limit the error log to the 'if (*soc_version == 0)'
branch, for all other errors there will already be a more specific log
entry.

> +
>  	return err;
>  }

Cheers

Matthias

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

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-07-23 23:10, Matthias Kaehlcke wrote:
> On Fri, Jul 20, 2018 at 07:02:39PM +0530, Balakrishna Godavarthi wrote:
>> Redefinition of qca_uart_setup will help future Qualcomm Bluetooth
>> SoC, to use the same function instead of duplicating the function.
>> Added new arguments soc_type and soc_ver to the functions.
>> 
>> These arguments will help to decide type of firmware files
>> to be loaded into Bluetooth chip.
>> soc_type holds the Bluetooth chip connected to APPS processor.
>> soc_ver holds the Bluetooth chip version.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>  drivers/bluetooth/btqca.c   | 20 +++++++-------------
>>  drivers/bluetooth/btqca.h   | 13 +++++++++++--
>>  drivers/bluetooth/hci_qca.c | 10 +++++++++-
>>  3 files changed, 27 insertions(+), 16 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
>> index c5cf9cab438a..b556710ee1bd 100644
>> --- a/drivers/bluetooth/btqca.c
>> +++ b/drivers/bluetooth/btqca.c
>> @@ -85,6 +85,9 @@ int qca_read_soc_version(struct hci_dev *hdev, u32 
>> *soc_version)
>>  out:
>>  	kfree_skb(skb);
>> 
>> +	if (err < 0 || *soc_version == 0)
>> +		bt_dev_err(hdev, "QCA Failed to get version (%d)", err);
> 
> You also have to set 'err' if soc_version is 0, so the caller can skip
> the check for soc_version == 0
> 
> I'd suggest:
> 
>   // directly after setting soc_version
>   if (*soc_version == 0)
>     err = -EILSEQ; // or should it be a different error code?
> 
>   ...
> 
>   if (err)
>     bt_dev_err(hdev, "QCA Failed to get version (%d)", err);
> 
> You could also limit the error log to the 'if (*soc_version == 0)'
> branch, for all other errors there will already be a more specific log
> entry.
> 

[Bala]: will update.

     if(!err && *soc_version == 0)
         err = -EINVAL; // as soc_version zero is invalid.

    if (err)
       bt_dev_err(hdev, "QCA Failed to get version (%d)", err);

>> +
>>  	return err;
>>  }
> 
> Cheers
> 
> Matthias
-- 
Regards
Balakrishna.

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

[Balakrishna Godavarthi]

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

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

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 5f1c0a8fd5cd..5f8a74d65bec 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -119,6 +119,11 @@ struct qca_data {
 	u64 votes_off;
 };
 
+enum qca_speed_type {
+	QCA_INIT_SPEED = 1,
+	QCA_OPER_SPEED
+};
+
 struct qca_serdev {
 	struct hci_uart	 serdev_hu;
 	struct gpio_desc *bt_en;
@@ -923,6 +928,61 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
 		hci_uart_set_baudrate(hu, speed);
 }
 
+static unsigned int qca_get_speed(struct hci_uart *hu,
+				  enum qca_speed_type speed_type)
+{
+	unsigned int speed = 0;
+
+	if (speed_type == QCA_INIT_SPEED) {
+		if (hu->init_speed)
+			speed = hu->init_speed;
+		else if (hu->proto->init_speed)
+			speed = hu->proto->init_speed;
+	} else {
+		if (hu->oper_speed)
+			speed = hu->oper_speed;
+		else if (hu->proto->oper_speed)
+			speed = hu->proto->oper_speed;
+	}
+
+	return speed;
+}
+
+static int qca_check_speeds(struct hci_uart *hu)
+{
+	if (!qca_get_speed(hu, QCA_INIT_SPEED) ||
+	    !qca_get_speed(hu, QCA_OPER_SPEED))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
+{
+	unsigned int speed, qca_baudrate;
+	int ret;
+
+	if (speed_type == QCA_INIT_SPEED) {
+		speed = qca_get_speed(hu, QCA_INIT_SPEED);
+		if (speed)
+			host_set_baudrate(hu, speed);
+	} else {
+		speed = qca_get_speed(hu, QCA_OPER_SPEED);
+		if (!speed)
+			return 0;
+
+		qca_baudrate = qca_get_baudrate_value(speed);
+		bt_dev_info(hu->hdev, "Set UART speed to %d", speed);
+		ret = qca_set_baudrate(hu->hdev, qca_baudrate);
+		if (ret)
+			return ret;
+
+		host_set_baudrate(hu, speed);
+	}
+
+	return 0;
+}
+
 static int qca_setup(struct hci_uart *hu)
 {
 	struct hci_dev *hdev = hu->hdev;
@@ -933,37 +993,24 @@ static int qca_setup(struct hci_uart *hu)
 
 	bt_dev_info(hdev, "ROME setup");
 
+	ret = qca_check_speeds(hu);
+	if (ret)
+		return ret;
+
 	/* 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);
+	qca_set_speed(hu, QCA_INIT_SPEED);
 
 	/* Setup user speed if needed */
-	speed = 0;
-	if (hu->oper_speed)
-		speed = hu->oper_speed;
-	else if (hu->proto->oper_speed)
-		speed = hu->proto->oper_speed;
-
+	speed = qca_get_speed(hu, QCA_OPER_SPEED);
 	if (speed) {
-		qca_baudrate = qca_get_baudrate_value(speed);
-
-		bt_dev_info(hdev, "Set UART speed to %d", speed);
-		ret = qca_set_baudrate(hdev, qca_baudrate);
-		if (ret) {
-			bt_dev_err(hdev, "Failed to change the baud rate (%d)",
-				   ret);
+		ret = qca_set_speed(hu, QCA_OPER_SPEED);
+		if (ret)
 			return ret;
-		}
-		host_set_baudrate(hu, speed);
+
+		qca_baudrate = qca_get_baudrate_value(speed);
 	}
 
 	/* Get QCA version information */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Matthias Kaehlcke]

On Fri, Jul 20, 2018 at 07:02:40PM +0530, Balakrishna Godavarthi wrote:
> In function qca_setup, we set initial and operating speeds for Qualcomm
> Bluetooth SoC's. This block of code is common across different
> Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
> a wrapper function to set the speeds. So that future coming SoC's
> can use these wrapper functions to set speeds.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  drivers/bluetooth/hci_qca.c | 93 ++++++++++++++++++++++++++++---------
>  1 file changed, 70 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 5f1c0a8fd5cd..5f8a74d65bec 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -119,6 +119,11 @@ struct qca_data {
>  	u64 votes_off;
>  };
>  
> +enum qca_speed_type {
> +	QCA_INIT_SPEED = 1,
> +	QCA_OPER_SPEED
> +};
> +
>  struct qca_serdev {
>  	struct hci_uart	 serdev_hu;
>  	struct gpio_desc *bt_en;
> @@ -923,6 +928,61 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
>  		hci_uart_set_baudrate(hu, speed);
>  }
>  
> +static unsigned int qca_get_speed(struct hci_uart *hu,
> +				  enum qca_speed_type speed_type)
> +{
> +	unsigned int speed = 0;
> +
> +	if (speed_type == QCA_INIT_SPEED) {
> +		if (hu->init_speed)
> +			speed = hu->init_speed;
> +		else if (hu->proto->init_speed)
> +			speed = hu->proto->init_speed;
> +	} else {
> +		if (hu->oper_speed)
> +			speed = hu->oper_speed;
> +		else if (hu->proto->oper_speed)
> +			speed = hu->proto->oper_speed;
> +	}
> +
> +	return speed;
> +}
> +
> +static int qca_check_speeds(struct hci_uart *hu)
> +{
> +	if (!qca_get_speed(hu, QCA_INIT_SPEED) ||
> +	    !qca_get_speed(hu, QCA_OPER_SPEED))
> +		return -EINVAL;

You changed this from:

	/* One or the other speeds should be non zero. */
	if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
	    !qca_get_speed(hu, QCA_OPER_SPEED))
		return -EINVAL;

There is no entry in the change log. What is the reason for this
change?

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

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-07-23 23:26, Matthias Kaehlcke wrote:
> On Fri, Jul 20, 2018 at 07:02:40PM +0530, Balakrishna Godavarthi wrote:
>> In function qca_setup, we set initial and operating speeds for 
>> Qualcomm
>> Bluetooth SoC's. This block of code is common across different
>> Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
>> a wrapper function to set the speeds. So that future coming SoC's
>> can use these wrapper functions to set speeds.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>>  drivers/bluetooth/hci_qca.c | 93 
>> ++++++++++++++++++++++++++++---------
>>  1 file changed, 70 insertions(+), 23 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 5f1c0a8fd5cd..5f8a74d65bec 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -119,6 +119,11 @@ struct qca_data {
>>  	u64 votes_off;
>>  };
>> 
>> +enum qca_speed_type {
>> +	QCA_INIT_SPEED = 1,
>> +	QCA_OPER_SPEED
>> +};
>> +
>>  struct qca_serdev {
>>  	struct hci_uart	 serdev_hu;
>>  	struct gpio_desc *bt_en;
>> @@ -923,6 +928,61 @@ static inline void host_set_baudrate(struct 
>> hci_uart *hu, unsigned int speed)
>>  		hci_uart_set_baudrate(hu, speed);
>>  }
>> 
>> +static unsigned int qca_get_speed(struct hci_uart *hu,
>> +				  enum qca_speed_type speed_type)
>> +{
>> +	unsigned int speed = 0;
>> +
>> +	if (speed_type == QCA_INIT_SPEED) {
>> +		if (hu->init_speed)
>> +			speed = hu->init_speed;
>> +		else if (hu->proto->init_speed)
>> +			speed = hu->proto->init_speed;
>> +	} else {
>> +		if (hu->oper_speed)
>> +			speed = hu->oper_speed;
>> +		else if (hu->proto->oper_speed)
>> +			speed = hu->proto->oper_speed;
>> +	}
>> +
>> +	return speed;
>> +}
>> +
>> +static int qca_check_speeds(struct hci_uart *hu)
>> +{
>> +	if (!qca_get_speed(hu, QCA_INIT_SPEED) ||
>> +	    !qca_get_speed(hu, QCA_OPER_SPEED))
>> +		return -EINVAL;
> 
> You changed this from:
> 
> 	/* One or the other speeds should be non zero. */
> 	if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
> 	    !qca_get_speed(hu, QCA_OPER_SPEED))
> 		return -EINVAL;
> 
> There is no entry in the change log. What is the reason for this
> change?

[Bala] : My bad i missed to add the log,as the above  if for ROME chip, 
having INIT or OPER speed is fine.

-- 
Regards
Balakrishna.

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

[Matthias Kaehlcke]

On Tue, Jul 24, 2018 at 12:32:36PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-07-23 23:26, Matthias Kaehlcke wrote:
> > On Fri, Jul 20, 2018 at 07:02:40PM +0530, Balakrishna Godavarthi wrote:
> > > In function qca_setup, we set initial and operating speeds for
> > > Qualcomm
> > > Bluetooth SoC's. This block of code is common across different
> > > Qualcomm Bluetooth SoC's. Instead of duplicating the code, created
> > > a wrapper function to set the speeds. So that future coming SoC's
> > > can use these wrapper functions to set speeds.
> > > 
> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> > > Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> > > ---
> > >  drivers/bluetooth/hci_qca.c | 93
> > > ++++++++++++++++++++++++++++---------
> > >  1 file changed, 70 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > index 5f1c0a8fd5cd..5f8a74d65bec 100644
> > > --- a/drivers/bluetooth/hci_qca.c
> > > +++ b/drivers/bluetooth/hci_qca.c
> > > @@ -119,6 +119,11 @@ struct qca_data {
> > >  	u64 votes_off;
> > >  };
> > > 
> > > +enum qca_speed_type {
> > > +	QCA_INIT_SPEED = 1,
> > > +	QCA_OPER_SPEED
> > > +};
> > > +
> > >  struct qca_serdev {
> > >  	struct hci_uart	 serdev_hu;
> > >  	struct gpio_desc *bt_en;
> > > @@ -923,6 +928,61 @@ static inline void host_set_baudrate(struct
> > > hci_uart *hu, unsigned int speed)
> > >  		hci_uart_set_baudrate(hu, speed);
> > >  }
> > > 
> > > +static unsigned int qca_get_speed(struct hci_uart *hu,
> > > +				  enum qca_speed_type speed_type)
> > > +{
> > > +	unsigned int speed = 0;
> > > +
> > > +	if (speed_type == QCA_INIT_SPEED) {
> > > +		if (hu->init_speed)
> > > +			speed = hu->init_speed;
> > > +		else if (hu->proto->init_speed)
> > > +			speed = hu->proto->init_speed;
> > > +	} else {
> > > +		if (hu->oper_speed)
> > > +			speed = hu->oper_speed;
> > > +		else if (hu->proto->oper_speed)
> > > +			speed = hu->proto->oper_speed;
> > > +	}
> > > +
> > > +	return speed;
> > > +}
> > > +
> > > +static int qca_check_speeds(struct hci_uart *hu)
> > > +{
> > > +	if (!qca_get_speed(hu, QCA_INIT_SPEED) ||
> > > +	    !qca_get_speed(hu, QCA_OPER_SPEED))
> > > +		return -EINVAL;
> > 
> > You changed this from:
> > 
> > 	/* One or the other speeds should be non zero. */
> > 	if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
> > 	    !qca_get_speed(hu, QCA_OPER_SPEED))
> > 		return -EINVAL;
> > 
> > There is no entry in the change log. What is the reason for this
> > change?
> 
> [Bala] : My bad i missed to add the log,as the above  if for ROME chip,
> having INIT or OPER speed is fine.

Ok, thanks for the clarification, I recall this was discussed earlier.

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

[Balakrishna Godavarthi]

Enable Qualcomm chips to operate at 3.2Mbps.

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

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

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

[Balakrishna Godavarthi]

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

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

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

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

[Balakrishna Godavarthi]

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

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
changes in v10:
    * added support to read regulator currents from dts.
    * added support to try to connect with chip if it fails to respond to initial commands
    * updated review comments.

changes in v9:
    * moved flow control to vendor and set_baudarte functions.
    * removed parent regs.

changes in v8:
    * closing qca buffer, if qca_power_setup fails
    * chnaged ibs start timer function call location.
    * updated review comments.
  
changes in v7:
    * addressed review comments.

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

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

---
 drivers/bluetooth/btqca.h   |   4 +
 drivers/bluetooth/hci_qca.c | 481 ++++++++++++++++++++++++++++++++----
 2 files changed, 439 insertions(+), 46 deletions(-)

diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index a9c2779f3e07..9e2bbcf5c002 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -37,6 +37,10 @@
 #define EDL_TAG_ID_HCI			(17)
 #define EDL_TAG_ID_DEEP_SLEEP		(27)
 
+#define QCA_WCN3990_POWERON_PULSE	0xFC
+#define QCA_WCN3990_POWEROFF_PULSE	0xC0
+#define QCA_WCN3990_FORCE_BOOT_PULSE	0xC0
+
 enum qca_bardrate {
 	QCA_BAUDRATE_115200 	= 0,
 	QCA_BAUDRATE_57600,
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index a6e7d38ef931..7ed35eb3f480 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -5,7 +5,7 @@
  *  protocol extension to H4.
  *
  *  Copyright (C) 2007 Texas Instruments, Inc.
- *  Copyright (c) 2010, 2012 The Linux Foundation. All rights reserved.
+ *  Copyright (c) 2010, 2012, 2018 The Linux Foundation. All rights reserved.
  *
  *  Acknowledgements:
  *  This file is based on hci_ll.c, which was...
@@ -31,9 +31,14 @@
 #include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/gpio/consumer.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
 #include <linux/serdev.h>
 
 #include <net/bluetooth/bluetooth.h>
@@ -124,12 +129,46 @@ enum qca_speed_type {
 	QCA_OPER_SPEED
 };
 
+/*
+ * Voltage regulator information required for configuring the
+ * QCA Bluetooth chipset
+ */
+struct qca_vreg {
+	const char *name;
+	unsigned int min_uV;
+	unsigned int max_uV;
+	unsigned int load_uA;
+};
+
+struct qca_vreg_data {
+	enum qca_btsoc_type soc_type;
+	struct qca_vreg *vregs;
+	size_t num_vregs;
+};
+
+/*
+ * Platform data for the QCA Bluetooth power driver.
+ */
+struct qca_power {
+	struct device *dev;
+	struct qca_vreg_data *vreg_data;
+	struct regulator_bulk_data *vreg_bulk;
+	bool vregs_on;
+};
+
 struct qca_serdev {
 	struct hci_uart	 serdev_hu;
 	struct gpio_desc *bt_en;
 	struct clk	 *susclk;
+	enum qca_btsoc_type btsoc_type;
+	struct qca_power *bt_power;
+	u32 init_speed;
+	u32 oper_speed;
 };
 
+static int qca_power_setup(struct hci_uart *hu, bool on);
+static int qca_power_shutdown(struct hci_dev *hdev);
+
 static void __serial_clock_on(struct tty_struct *tty)
 {
 	/* TODO: Some chipset requires to enable UART clock on client
@@ -407,6 +446,7 @@ static int qca_open(struct hci_uart *hu)
 {
 	struct qca_serdev *qcadev;
 	struct qca_data *qca;
+	int ret;
 
 	BT_DBG("hu %p qca_open", hu);
 
@@ -458,19 +498,32 @@ static int qca_open(struct hci_uart *hu)
 
 	hu->priv = qca;
 
-	timer_setup(&qca->wake_retrans_timer, hci_ibs_wake_retrans_timeout, 0);
-	qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS;
-
-	timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0);
-	qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS;
-
 	if (hu->serdev) {
 		serdev_device_open(hu->serdev);
 
 		qcadev = serdev_device_get_drvdata(hu->serdev);
-		gpiod_set_value_cansleep(qcadev->bt_en, 1);
+		if (qcadev->btsoc_type != QCA_WCN3990) {
+			gpiod_set_value_cansleep(qcadev->bt_en, 1);
+		} else {
+			hu->init_speed = qcadev->init_speed;
+			hu->oper_speed = qcadev->oper_speed;
+			ret = qca_power_setup(hu, true);
+			if (ret) {
+				destroy_workqueue(qca->workqueue);
+				kfree_skb(qca->rx_skb);
+				hu->priv = NULL;
+				kfree(qca);
+				return ret;
+			}
+		}
 	}
 
+	timer_setup(&qca->wake_retrans_timer, hci_ibs_wake_retrans_timeout, 0);
+	qca->wake_retrans = IBS_WAKE_RETRANS_TIMEOUT_MS;
+
+	timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0);
+	qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS;
+
 	BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u",
 	       qca->tx_idle_delay, qca->wake_retrans);
 
@@ -554,10 +607,13 @@ static int qca_close(struct hci_uart *hu)
 	qca->hu = NULL;
 
 	if (hu->serdev) {
-		serdev_device_close(hu->serdev);
-
 		qcadev = serdev_device_get_drvdata(hu->serdev);
-		gpiod_set_value_cansleep(qcadev->bt_en, 0);
+		if (qcadev->btsoc_type == QCA_WCN3990)
+			qca_power_shutdown(hu->hdev);
+		else
+			gpiod_set_value_cansleep(qcadev->bt_en, 0);
+
+		serdev_device_close(hu->serdev);
 	}
 
 	kfree_skb(qca->rx_skb);
@@ -891,6 +947,7 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
 	struct hci_uart *hu = hci_get_drvdata(hdev);
 	struct qca_data *qca = hu->priv;
 	struct sk_buff *skb;
+	struct qca_serdev *qcadev;
 	u8 cmd[] = { 0x01, 0x48, 0xFC, 0x01, 0x00 };
 
 	if (baudrate > QCA_BAUDRATE_3200000)
@@ -904,6 +961,13 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
 		return -ENOMEM;
 	}
 
+	/* Disabling hardware flow control is mandatory while
+	 * sending change baudrate request to wcn3990 SoC.
+	 */
+	qcadev = serdev_device_get_drvdata(hu->serdev);
+	if (qcadev->btsoc_type == QCA_WCN3990)
+		hci_uart_set_flow_control(hu, true);
+
 	/* Assign commands to change baudrate and packet type. */
 	skb_put_data(skb, cmd, sizeof(cmd));
 	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
@@ -919,6 +983,9 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
 	schedule_timeout(msecs_to_jiffies(BAUDRATE_SETTLE_TIMEOUT_MS));
 	set_current_state(TASK_RUNNING);
 
+	if (qcadev->btsoc_type == QCA_WCN3990)
+		hci_uart_set_flow_control(hu, false);
+
 	return 0;
 }
 
@@ -930,6 +997,43 @@ static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
 		hci_uart_set_baudrate(hu, speed);
 }
 
+static int qca_send_vendor_pulse(struct hci_dev *hdev, u8 cmd)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+	struct qca_data *qca = hu->priv;
+	struct sk_buff *skb;
+
+	/* These vendor pulses are single byte command which are sent
+	 * at required baudrate to WCN3990. on WCN3990, we have an external
+	 * circuit at Tx pin which decodes the pulse sent at specific baudrate.
+	 * For example, as WCN3990 supports RF COEX frequency for both Wi-Fi/BT
+	 * and also, we use the same power inputs to turn ON and OFF for
+	 * Wi-Fi/BT. Powering up the power sources will not enable BT, until
+	 * we send a POWER ON pulse at 115200. This algorithm will help to
+	 * save power. Disabling hardware flow control is mandatory while
+	 * sending vendor pulses to SoC.
+	 */
+	bt_dev_dbg(hdev, "sending vendor pulse %02x to SoC", cmd);
+
+	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	hci_uart_set_flow_control(hu, true);
+
+	skb_put_u8(skb, cmd);
+	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
+
+	skb_queue_tail(&qca->txq, skb);
+	hci_uart_tx_wakeup(hu);
+
+	/* Wait for 100 uS for SoC to settle down */
+	usleep_range(100, 200);
+	hci_uart_set_flow_control(hu, false);
+
+	return 0;
+}
+
 static unsigned int qca_get_speed(struct hci_uart *hu,
 				  enum qca_speed_type speed_type)
 {
@@ -952,9 +1056,18 @@ static unsigned int qca_get_speed(struct hci_uart *hu,
 
 static int qca_check_speeds(struct hci_uart *hu)
 {
-	if (!qca_get_speed(hu, QCA_INIT_SPEED) ||
-	    !qca_get_speed(hu, QCA_OPER_SPEED))
-		return -EINVAL;
+	struct qca_serdev *qcadev;
+
+	qcadev = serdev_device_get_drvdata(hu->serdev);
+	if (qcadev->btsoc_type == QCA_WCN3990) {
+		if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
+		    !qca_get_speed(hu, QCA_OPER_SPEED))
+			return -EINVAL;
+	} else {
+		if (!qca_get_speed(hu, QCA_INIT_SPEED) ||
+		    !qca_get_speed(hu, QCA_OPER_SPEED))
+			return -EINVAL;
+	}
 
 	return 0;
 }
@@ -985,15 +1098,78 @@ static int qca_set_speed(struct hci_uart *hu, enum qca_speed_type speed_type)
 	return 0;
 }
 
+static int qca_wcn3990_init(struct hci_uart *hu, u32 *soc_ver)
+{
+	struct hci_dev *hdev = hu->hdev;
+	int i, ret = 1;
+
+	/* WCN3990 is a discrete Bluetooth chip connected to APPS processor.
+	 * sometimes we will face communication synchronization issues,
+	 * like reading version command timeouts. In which HCI_SETUP fails,
+	 * to overcome these issues, we try to communicate by performing an
+	 * COLD power OFF and ON.
+	 */
+	for (i = 1; i <= 10 && ret; i++) {
+		/* This helper will turn ON chip if it is powered off.
+		 * if the chip is already powered ON, function call will
+		 * return zero.
+		 */
+		ret = qca_power_setup(hu, true);
+		if (ret)
+			goto regs_off;
+
+		/* Forcefully enable wcn3990 to enter in to boot mode. */
+		host_set_baudrate(hu, 2400);
+		ret = qca_send_vendor_pulse(hdev, QCA_WCN3990_FORCE_BOOT_PULSE);
+		if (ret)
+			goto regs_off;
+
+		qca_set_speed(hu, QCA_INIT_SPEED);
+		ret = qca_send_vendor_pulse(hdev, QCA_WCN3990_POWERON_PULSE);
+		if (ret)
+			goto regs_off;
+
+		/* Wait for 100 ms for SoC to boot */
+		msleep(100);
+
+		/* Now the device is in ready state to communicate with host.
+		 * To sync HOST with device we need to reopen port.
+		 * Without this, we will have RTS and CTS synchronization
+		 * issues.
+		 */
+		serdev_device_close(hu->serdev);
+		ret = serdev_device_open(hu->serdev);
+		if (ret) {
+			bt_dev_err(hu->hdev, "failed to open port");
+			break;
+		}
+
+		hci_uart_set_flow_control(hu, false);
+		ret = qca_read_soc_version(hdev, soc_ver);
+		if (ret < 0 || soc_ver == 0)
+			bt_dev_err(hdev, "Failed to get version:%d", ret);
+
+		if (!ret)
+			break;
+
+regs_off:
+		bt_dev_err(hdev, "retrying to establish communication: %d", i);
+		qca_power_shutdown(hdev);
+	}
+
+	return ret;
+}
+
 static int qca_setup(struct hci_uart *hu)
 {
 	struct hci_dev *hdev = hu->hdev;
 	struct qca_data *qca = hu->priv;
 	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
+	struct qca_serdev *qcadev;
 	int ret;
 	int soc_ver = 0;
 
-	bt_dev_info(hdev, "ROME setup");
+	qcadev = serdev_device_get_drvdata(hu->serdev);
 
 	ret = qca_check_speeds(hu);
 	if (ret)
@@ -1002,8 +1178,15 @@ static int qca_setup(struct hci_uart *hu)
 	/* Patch downloading has to be done without IBS mode */
 	clear_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
 
-	/* Setup initial baudrate */
-	qca_set_speed(hu, QCA_INIT_SPEED);
+	if (qcadev->btsoc_type == QCA_WCN3990) {
+		bt_dev_info(hdev, "setting up wcn3990");
+		ret = qca_wcn3990_init(hu, &soc_ver);
+		if (ret)
+			return ret;
+	} else {
+		bt_dev_info(hdev, "ROME setup");
+		qca_set_speed(hu, QCA_INIT_SPEED);
+	}
 
 	/* Setup user speed if needed */
 	speed = qca_get_speed(hu, QCA_OPER_SPEED);
@@ -1015,15 +1198,16 @@ static int qca_setup(struct hci_uart *hu)
 		qca_baudrate = qca_get_baudrate_value(speed);
 	}
 
-	/* Get QCA version information */
-	ret = qca_read_soc_version(hdev, &soc_ver);
-	if (ret)
-		return ret;
+	if (qcadev->btsoc_type != QCA_WCN3990) {
+		/* Get QCA version information */
+		ret = qca_read_soc_version(hdev, &soc_ver);
+		if (ret)
+			return ret;
+	}
 
 	bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver);
-
 	/* Setup patch / NVM configurations */
-	ret = qca_uart_setup(hdev, qca_baudrate, QCA_ROME, soc_ver);
+	ret = qca_uart_setup(hdev, qca_baudrate, qcadev->btsoc_type, soc_ver);
 	if (!ret) {
 		set_bit(STATE_IN_BAND_SLEEP_ENABLED, &qca->flags);
 		qca_debugfs_init(hdev);
@@ -1059,9 +1243,174 @@ static struct hci_uart_proto qca_proto = {
 	.dequeue	= qca_dequeue,
 };
 
+static const struct qca_vreg_data qca_soc_data = {
+	.soc_type = QCA_WCN3990,
+	.vregs = (struct qca_vreg []) {
+		{ "vddio",   1800000, 1800000,  15000  },
+		{ "vddxo",   1800000, 1800000,  80000  },
+		{ "vddrf",   1304000, 1304000,  300000 },
+		{ "vddch0",  3000000, 3312000,  450000 },
+	},
+	.num_vregs = 4,
+};
+
+static int qca_power_shutdown(struct hci_dev *hdev)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+
+	host_set_baudrate(hu, 2400);
+	qca_send_vendor_pulse(hdev, QCA_WCN3990_POWEROFF_PULSE);
+	return qca_power_setup(hu, false);
+}
+
+static int qca_enable_regulator(struct qca_vreg vregs,
+				struct regulator *regulator)
+{
+	int ret;
+
+	ret = regulator_set_voltage(regulator, vregs.min_uV,
+				    vregs.max_uV);
+	if (ret)
+		return ret;
+
+	if (vregs.load_uA)
+		ret = regulator_set_load(regulator,
+					 vregs.load_uA);
+
+	if (ret)
+		return ret;
+
+	return regulator_enable(regulator);
+
+}
+
+static void qca_disable_regulator(struct qca_vreg vregs,
+				  struct regulator *regulator)
+{
+	regulator_disable(regulator);
+	regulator_set_voltage(regulator, 0, vregs.max_uV);
+	if (vregs.load_uA)
+		regulator_set_load(regulator, 0);
+
+}
+
+static int qca_power_setup(struct hci_uart *hu, bool on)
+{
+	struct qca_vreg *vregs;
+	struct regulator_bulk_data *vreg_bulk;
+	struct qca_serdev *qcadev;
+	int i, num_vregs, ret = 0;
+
+	qcadev = serdev_device_get_drvdata(hu->serdev);
+	if (!qcadev || !qcadev->bt_power || !qcadev->bt_power->vreg_data ||
+	    !qcadev->bt_power->vreg_bulk)
+		return -EINVAL;
+
+	vregs = qcadev->bt_power->vreg_data->vregs;
+	vreg_bulk = qcadev->bt_power->vreg_bulk;
+	num_vregs = qcadev->bt_power->vreg_data->num_vregs;
+	BT_DBG("on: %d", on);
+	if (on  && !qcadev->bt_power->vregs_on) {
+		for (i = 0; i < num_vregs; i++) {
+			ret = qca_enable_regulator(vregs[i],
+						   vreg_bulk[i].consumer);
+			if (ret)
+				break;
+		}
+
+		if (ret) {
+			BT_ERR("failed to enable regulator:%s", vregs[i].name);
+			/* turn off regulators which are enabled */
+			for (i = i - 1; i >= 0; i--)
+				qca_disable_regulator(vregs[i],
+						      vreg_bulk[i].consumer);
+		} else {
+			qcadev->bt_power->vregs_on = true;
+		}
+	} else if (!on && qcadev->bt_power->vregs_on) {
+		/* turn off regulator in reverse order */
+		i = qcadev->bt_power->vreg_data->num_vregs - 1;
+		for ( ; i >= 0; i--)
+			qca_disable_regulator(vregs[i], vreg_bulk[i].consumer);
+
+		qcadev->bt_power->vregs_on = false;
+	}
+
+	return ret;
+}
+
+static void qca_regulator_get_current(struct device *dev,
+				      struct qca_vreg *vregs)
+{
+	char prop_name[32]; /* 32 is max size of property name */
+
+	/* We have different platforms where the load value is controlled
+	 * via PMIC controllers. In such cases load required to power ON
+	 * Bluetooth chips are defined in the PMIC. We have option to set
+	 * operation mode like high or low power modes.
+	 * We do have some platforms where driver need to enable the load for
+	 * WCN3990. Based on the current property value defined for the
+	 * regulators, driver will decide the regulator output load.
+	 * If the current property for the regulator is defined in the dts
+	 * we will read from dts tree, else from the default load values.
+	 */
+	snprintf(prop_name, 32, "%s-current", vregs->name);
+	BT_DBG("Looking up %s from device tree\n", prop_name);
+
+	if (device_property_read_bool(dev, prop_name))
+		device_property_read_u32(dev, prop_name, &vregs->load_uA);
+
+	BT_DBG("current %duA selected for regulator %s", vregs->load_uA,
+		vregs->name);
+}
+
+static int qca_init_regulators(struct qca_power *qca,
+				const struct qca_vreg_data *data)
+{
+	int i, num_vregs;
+	int load_uA;
+
+	num_vregs = data->num_vregs;
+	qca->vreg_bulk = devm_kzalloc(qca->dev, num_vregs *
+				      sizeof(struct regulator_bulk_data),
+				      GFP_KERNEL);
+	if (!qca->vreg_bulk)
+		return -ENOMEM;
+
+	qca->vreg_data = devm_kzalloc(qca->dev, sizeof(struct qca_vreg_data),
+				      GFP_KERNEL);
+	if (!qca->vreg_data)
+		return -ENOMEM;
+
+	qca->vreg_data->num_vregs = data->num_vregs;
+	qca->vreg_data->soc_type = data->soc_type;
+
+	qca->vreg_data->vregs = devm_kzalloc(qca->dev, num_vregs *
+				      sizeof(struct qca_vreg_data),
+				      GFP_KERNEL);
+
+	if (!qca->vreg_data->vregs)
+		return -ENOMEM;
+
+	for (i = 0; i < num_vregs; i++) {
+		/* copy regulator name, min voltage, max voltage */
+		qca->vreg_data->vregs[i].name = data->vregs[i].name;
+		qca->vreg_data->vregs[i].min_uV = data->vregs[i].min_uV;
+		qca->vreg_data->vregs[i].max_uV = data->vregs[i].max_uV;
+		load_uA = data->vregs[i].load_uA;
+		qca->vreg_data->vregs[i].load_uA = load_uA;
+		qca_regulator_get_current(qca->dev, &qca->vreg_data->vregs[i]);
+
+		qca->vreg_bulk[i].supply = qca->vreg_data->vregs[i].name;
+	}
+
+	return devm_regulator_bulk_get(qca->dev, num_vregs, qca->vreg_bulk);
+}
+
 static int qca_serdev_probe(struct serdev_device *serdev)
 {
 	struct qca_serdev *qcadev;
+	const struct qca_vreg_data *data;
 	int err;
 
 	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
@@ -1069,47 +1418,87 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		return -ENOMEM;
 
 	qcadev->serdev_hu.serdev = serdev;
+	data = of_device_get_match_data(&serdev->dev);
 	serdev_device_set_drvdata(serdev, qcadev);
+	if (data && data->soc_type == QCA_WCN3990) {
+		qcadev->btsoc_type = QCA_WCN3990;
+		qcadev->bt_power = devm_kzalloc(&serdev->dev,
+						sizeof(struct qca_power),
+						GFP_KERNEL);
+		if (!qcadev->bt_power)
+			return -ENOMEM;
+
+		qcadev->bt_power->dev = &serdev->dev;
+		err = qca_init_regulators(qcadev->bt_power, data);
+		if (err) {
+			BT_ERR("Failed to init regulators:%d", err);
+			goto out;
+		}
 
-	qcadev->bt_en = devm_gpiod_get(&serdev->dev, "enable",
-				       GPIOD_OUT_LOW);
-	if (IS_ERR(qcadev->bt_en)) {
-		dev_err(&serdev->dev, "failed to acquire enable gpio\n");
-		return PTR_ERR(qcadev->bt_en);
-	}
+		qcadev->bt_power->vregs_on = false;
 
-	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);
-	}
+		/* Read max speed supported by wcn3990 from dts
+		 * tree. if max-speed property is not enabled in
+		 * dts, QCA driver will use default operating speed
+		 * from proto structure.
+		 */
+		device_property_read_u32(&serdev->dev, "max-speed",
+					 &qcadev->oper_speed);
+		if (!qcadev->oper_speed)
+			BT_INFO("UART will pick default operating speed");
+
+		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
+		if (err) {
+			BT_ERR("wcn3990 serdev registration failed");
+			goto out;
+		}
+	} else {
+		qcadev->btsoc_type = QCA_ROME;
+		qcadev->bt_en = devm_gpiod_get(&serdev->dev, "enable",
+					       GPIOD_OUT_LOW);
+		if (IS_ERR(qcadev->bt_en)) {
+			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+			return PTR_ERR(qcadev->bt_en);
+		}
 
-	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)
 {
 	struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
 
-	hci_uart_unregister_device(&qcadev->serdev_hu);
+	if (qcadev->btsoc_type == QCA_WCN3990)
+		qca_power_shutdown(qcadev->serdev_hu.hdev);
+	else
+		clk_disable_unprepare(qcadev->susclk);
 
-	clk_disable_unprepare(qcadev->susclk);
+	hci_uart_unregister_device(&qcadev->serdev_hu);
 }
 
 static const struct of_device_id qca_bluetooth_of_match[] = {
 	{ .compatible = "qcom,qca6174-bt" },
+	{ .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Matthias Kaehlcke]

On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
> Add support to set voltage/current of various regulators
> to power up/down Bluetooth chip wcn3990.
> 
> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
> ---
> changes in v10:
>     * added support to read regulator currents from dts.

I commented on this below

>     * added support to try to connect with chip if it fails to respond to initial commands
>     * updated review comments.
> 
> changes in v9:
>     * moved flow control to vendor and set_baudarte functions.
>     * removed parent regs.
> 
> changes in v8:
>     * closing qca buffer, if qca_power_setup fails
>     * chnaged ibs start timer function call location.
>     * updated review comments.
>   
> changes in v7:
>     * addressed review comments.
> 
> changes in v6:
>     * Hooked up qca_power to qca_serdev.
>     * renamed all the naming inconsistency functions with qca_*
>     * leveraged common code of ROME for wcn3990.
>     * created wrapper functions for re-usable blocks.
>     * updated function of _*regulator_enable and _*regualtor_disable.  
>     * removed redundant comments and functions.
>     * addressed review comments.
> 
> Changes in v5:
>     * updated regulator vddpa min_uV to 1304000.
>       * addressed review comments.
>  
> Changes in v4:
>     * Segregated the changes of btqca from hci_qca
>     * rebased all changes on top of bluetooth-next.
>     * addressed review comments.
> 
> ---
>  drivers/bluetooth/btqca.h   |   4 +
>  drivers/bluetooth/hci_qca.c | 481 ++++++++++++++++++++++++++++++++----
>  2 files changed, 439 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index a9c2779f3e07..9e2bbcf5c002 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -37,6 +37,10 @@
>  #define EDL_TAG_ID_HCI			(17)
>  #define EDL_TAG_ID_DEEP_SLEEP		(27)
>  
> +#define QCA_WCN3990_POWERON_PULSE	0xFC
> +#define QCA_WCN3990_POWEROFF_PULSE	0xC0
> +#define QCA_WCN3990_FORCE_BOOT_PULSE	0xC0

This is the same value as QCA_WCN3990_POWEROFF_PULSE. From the usage
it seems it's really just a power off pulse, so let's stick to this
name, instead of having two names for the same thing.

> +static int qca_send_vendor_pulse(struct hci_dev *hdev, u8 cmd)

My understanding from earlier discussion is that these pulses are
limited to power on/off. If that is correct this should probably be
called qca_send_power_pulse().

> +{
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +	struct qca_data *qca = hu->priv;
> +	struct sk_buff *skb;
> +
> +	/* These vendor pulses are single byte command which are sent
> +	 * at required baudrate to WCN3990. on WCN3990, we have an external

s/on/On/

> +	 * circuit at Tx pin which decodes the pulse sent at specific baudrate.
> +	 * For example, as WCN3990 supports RF COEX frequency for both Wi-Fi/BT
> +	 * and also, we use the same power inputs to turn ON and OFF for

nit: not sure how much value is added by (sometimes) using upper case
for certain things (ON, OFF, COLD, HOST, ...).

> +	 * Wi-Fi/BT. Powering up the power sources will not enable BT, until
> +	 * we send a POWER ON pulse at 115200. This algorithm will help to

115200 what? bps I guess.

> +static int qca_wcn3990_init(struct hci_uart *hu, u32 *soc_ver)
> +{
> +	struct hci_dev *hdev = hu->hdev;
> +	int i, ret = 1;

Initialization not necessary, more details below.

> +
> +	/* WCN3990 is a discrete Bluetooth chip connected to APPS processor.

APPS is a Qualcomm specific term, and some QCA docs also call it
APSS. Just say 'SoC' which is universally understood.

> +	 * sometimes we will face communication synchronization issues,
> +	 * like reading version command timeouts. In which HCI_SETUP fails,
> +	 * to overcome these issues, we try to communicate by performing an
> +	 * COLD power OFF and ON.
> +	 */
> +	for (i = 1; i <= 10 && ret; i++) {

Is it really that bad that more than say 3 iterations might be needed?

Common practice is to start loops with index 0.

The check for ret is not needed. All jumps to 'regs_off' are done
when an error is detected. The loop is left when 'ret == 0' at the
bottom.

> +		/* This helper will turn ON chip if it is powered off.
> +		 * if the chip is already powered ON, function call will
> +		 * return zero.
> +		 */

Comments are great when they add value, IMO this one doesn't and just
adds distraction. Most readers will assume that after
qca_power_setup(hu, true) the chip is powered on, regardless of the
previous power state.

> +		ret = qca_power_setup(hu, true);
> +		if (ret)
> +			goto regs_off;
> +
> +		/* Forcefully enable wcn3990 to enter in to boot mode. */

nit: Sometimes the comments and logs name the chip wcn3990, others
WCN3990. Personally I don't care which spelling is used, but please be
consistent.

> +		host_set_baudrate(hu, 2400);
> +		ret = qca_send_vendor_pulse(hdev, QCA_WCN3990_FORCE_BOOT_PULSE);
> +		if (ret)
> +			goto regs_off;
> +
> +		qca_set_speed(hu, QCA_INIT_SPEED);
> +		ret = qca_send_vendor_pulse(hdev, QCA_WCN3990_POWERON_PULSE);
> +		if (ret)
> +			goto regs_off;
> +
> +		/* Wait for 100 ms for SoC to boot */
> +		msleep(100);
> +
> +		/* Now the device is in ready state to communicate with host.
> +		 * To sync HOST with device we need to reopen port.
> +		 * Without this, we will have RTS and CTS synchronization
> +		 * issues.
> +		 */
> +		serdev_device_close(hu->serdev);
> +		ret = serdev_device_open(hu->serdev);
> +		if (ret) {
> +			bt_dev_err(hu->hdev, "failed to open port");
> +			break;
> +		}
> +
> +		hci_uart_set_flow_control(hu, false);
> +		ret = qca_read_soc_version(hdev, soc_ver);
> +		if (ret < 0 || soc_ver == 0)
> +			bt_dev_err(hdev, "Failed to get version:%d", ret);

The check for soc_ver is/should be done in qca_read_soc_version(),
same for the error log.

> +		if (!ret)
> +			break;
> +
> +regs_off:
> +		bt_dev_err(hdev, "retrying to establish communication: %d", i);

Use i + 1 if starting the loop at 0.

> +static const struct qca_vreg_data qca_soc_data = {
> +	.soc_type = QCA_WCN3990,
> +	.vregs = (struct qca_vreg []) {
> +		{ "vddio",   1800000, 1800000,  15000  },
> +		{ "vddxo",   1800000, 1800000,  80000  },
> +		{ "vddrf",   1304000, 1304000,  300000 },
> +		{ "vddch0",  3000000, 3312000,  450000 },
> +	},

The currents of 300mA and 450mA seem high for Bluetooth, I'm not an
expert in this area though, they might be reasonable peak currents for
certain use cases.

> +static int qca_power_shutdown(struct hci_dev *hdev)
> +{
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +
> +	host_set_baudrate(hu, 2400);
> +	qca_send_vendor_pulse(hdev, QCA_WCN3990_POWEROFF_PULSE);
> +	return qca_power_setup(hu, false);
> +}

The return value changed from void to int, but nobody ever checks it ...

> +static void qca_regulator_get_current(struct device *dev,
> +				      struct qca_vreg *vregs)
> +{
> +	char prop_name[32]; /* 32 is max size of property name */
> +
> +	/* We have different platforms where the load value is controlled
> +	 * via PMIC controllers. In such cases load required to power ON
> +	 * Bluetooth chips are defined in the PMIC. We have option to set
> +	 * operation mode like high or low power modes.
> +	 * We do have some platforms where driver need to enable the load for
> +	 * WCN3990. Based on the current property value defined for the
> +	 * regulators, driver will decide the regulator output load.
> +	 * If the current property for the regulator is defined in the dts
> +	 * we will read from dts tree, else from the default load values.
> +	 */

Let's make sure we all really understand why this is needed. You
mentioned RPMh regulators earlier and said a special value of 1uA
would be needed to enable high power mode. Later when I pointed to the
RPMh regulator code you agreed that this special value wouldn't make
any difference.

Now the defaults are higher:

> +		{ "vddio",   1800000, 1800000,  15000  },
> +		{ "vddxo",   1800000, 1800000,  80000  },
> +		{ "vddrf",   1304000, 1304000,  300000 },
> +		{ "vddch0",  3000000, 3312000,  450000 },

What would supposedly go wrong if these values were passed to one of
the PMICs you are concerned about? Please be more specific than the
above comment.

> +	snprintf(prop_name, 32, "%s-current", vregs->name);
> +	BT_DBG("Looking up %s from device tree\n", prop_name);

'\n' not needed with BT_DBG()

> +
> +	if (device_property_read_bool(dev, prop_name))
> +		device_property_read_u32(dev, prop_name, &vregs->load_uA);

Why device_property_read_bool()?

> +	BT_DBG("current %duA selected for regulator %s", vregs->load_uA,
> +		vregs->name);
> +}
> +
> +static int qca_init_regulators(struct qca_power *qca,
> +				const struct qca_vreg_data *data)
> +{
> +	int i, num_vregs;
> +	int load_uA;
> +
> +	num_vregs = data->num_vregs;
> +	qca->vreg_bulk = devm_kzalloc(qca->dev, num_vregs *
> +				      sizeof(struct regulator_bulk_data),
> +				      GFP_KERNEL);
> +	if (!qca->vreg_bulk)
> +		return -ENOMEM;
> +
> +	qca->vreg_data = devm_kzalloc(qca->dev, sizeof(struct qca_vreg_data),
> +				      GFP_KERNEL);
> +	if (!qca->vreg_data)
> +		return -ENOMEM;
> +
> +	qca->vreg_data->num_vregs = data->num_vregs;
> +	qca->vreg_data->soc_type = data->soc_type;
> +
> +	qca->vreg_data->vregs = devm_kzalloc(qca->dev, num_vregs *
> +				      sizeof(struct qca_vreg_data),

sizeof(struct qca_vreg)

> +				      GFP_KERNEL);
> +
> +	if (!qca->vreg_data->vregs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_vregs; i++) {
> +		/* copy regulator name, min voltage, max voltage */
> +		qca->vreg_data->vregs[i].name = data->vregs[i].name;
> +		qca->vreg_data->vregs[i].min_uV = data->vregs[i].min_uV;
> +		qca->vreg_data->vregs[i].max_uV = data->vregs[i].max_uV;
> +		load_uA = data->vregs[i].load_uA;
> +		qca->vreg_data->vregs[i].load_uA = load_uA;

memcpy(&qca->vreg_data->vregs[i], &data->vregs[i]); ?

Or do it outside of the loop for all regulators at once.

>  static int qca_serdev_probe(struct serdev_device *serdev)
>  {
>  	struct qca_serdev *qcadev;
> +	const struct qca_vreg_data *data;
>  	int err;
>  
>  	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
> @@ -1069,47 +1418,87 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  		return -ENOMEM;
>  
>  	qcadev->serdev_hu.serdev = serdev;
> +	data = of_device_get_match_data(&serdev->dev);
>  	serdev_device_set_drvdata(serdev, qcadev);
> +	if (data && data->soc_type == QCA_WCN3990) {
> +		qcadev->btsoc_type = QCA_WCN3990;
> +		qcadev->bt_power = devm_kzalloc(&serdev->dev,
> +						sizeof(struct qca_power),
> +						GFP_KERNEL);
> +		if (!qcadev->bt_power)
> +			return -ENOMEM;
> +
> +		qcadev->bt_power->dev = &serdev->dev;
> +		err = qca_init_regulators(qcadev->bt_power, data);
> +		if (err) {
> +			BT_ERR("Failed to init regulators:%d", err);
> +			goto out;
> +		}
>  
> -	qcadev->bt_en = devm_gpiod_get(&serdev->dev, "enable",
> -				       GPIOD_OUT_LOW);
> -	if (IS_ERR(qcadev->bt_en)) {
> -		dev_err(&serdev->dev, "failed to acquire enable gpio\n");
> -		return PTR_ERR(qcadev->bt_en);
> -	}
> +		qcadev->bt_power->vregs_on = false;
>  
> -	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);
> -	}
> +		/* Read max speed supported by wcn3990 from dts
> +		 * tree. if max-speed property is not enabled in
> +		 * dts, QCA driver will use default operating speed
> +		 * from proto structure.
> +		 */

The comment doesn't add much value.

> +		device_property_read_u32(&serdev->dev, "max-speed",
> +					 &qcadev->oper_speed);
> +		if (!qcadev->oper_speed)
> +			BT_INFO("UART will pick default operating speed");

Not a change in this version, but BT_INFO seems a bit verbose, we
should avoid spamming the kernel log.

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

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-07-24 01:24, Matthias Kaehlcke wrote:
> On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
>> Add support to set voltage/current of various regulators
>> to power up/down Bluetooth chip wcn3990.
>> 
>> Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
>> ---
>> changes in v10:
>>     * added support to read regulator currents from dts.
> 
> I commented on this below
> 
>>     * added support to try to connect with chip if it fails to respond 
>> to initial commands
>>     * updated review comments.
>> 
>> changes in v9:
>>     * moved flow control to vendor and set_baudarte functions.
>>     * removed parent regs.
>> 
>> changes in v8:
>>     * closing qca buffer, if qca_power_setup fails
>>     * chnaged ibs start timer function call location.
>>     * updated review comments.
>> 
>> changes in v7:
>>     * addressed review comments.
>> 
>> changes in v6:
>>     * Hooked up qca_power to qca_serdev.
>>     * renamed all the naming inconsistency functions with qca_*
>>     * leveraged common code of ROME for wcn3990.
>>     * created wrapper functions for re-usable blocks.
>>     * updated function of _*regulator_enable and _*regualtor_disable.
>>     * removed redundant comments and functions.
>>     * addressed review comments.
>> 
>> Changes in v5:
>>     * updated regulator vddpa min_uV to 1304000.
>>       * addressed review comments.
>> 
>> Changes in v4:
>>     * Segregated the changes of btqca from hci_qca
>>     * rebased all changes on top of bluetooth-next.
>>     * addressed review comments.
>> 
>> ---
>>  drivers/bluetooth/btqca.h   |   4 +
>>  drivers/bluetooth/hci_qca.c | 481 
>> ++++++++++++++++++++++++++++++++----
>>  2 files changed, 439 insertions(+), 46 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
>> index a9c2779f3e07..9e2bbcf5c002 100644
>> --- a/drivers/bluetooth/btqca.h
>> +++ b/drivers/bluetooth/btqca.h
>> @@ -37,6 +37,10 @@
>>  #define EDL_TAG_ID_HCI			(17)
>>  #define EDL_TAG_ID_DEEP_SLEEP		(27)
>> 
>> +#define QCA_WCN3990_POWERON_PULSE	0xFC
>> +#define QCA_WCN3990_POWEROFF_PULSE	0xC0
>> +#define QCA_WCN3990_FORCE_BOOT_PULSE	0xC0
> 
> This is the same value as QCA_WCN3990_POWEROFF_PULSE. From the usage
> it seems it's really just a power off pulse, so let's stick to this
> name, instead of having two names for the same thing.
> 
[Bala] : will update.

>> +static int qca_send_vendor_pulse(struct hci_dev *hdev, u8 cmd)
> 
> My understanding from earlier discussion is that these pulses are
> limited to power on/off. If that is correct this should probably be
> called qca_send_power_pulse().
> 
[Bala]: will update the function name to qca_send_power_pulse().

>> +{
>> +	struct hci_uart *hu = hci_get_drvdata(hdev);
>> +	struct qca_data *qca = hu->priv;
>> +	struct sk_buff *skb;
>> +
>> +	/* These vendor pulses are single byte command which are sent
>> +	 * at required baudrate to WCN3990. on WCN3990, we have an external
> 
> s/on/On/
[Bala]: will update.

> 
>> +	 * circuit at Tx pin which decodes the pulse sent at specific 
>> baudrate.
>> +	 * For example, as WCN3990 supports RF COEX frequency for both 
>> Wi-Fi/BT
>> +	 * and also, we use the same power inputs to turn ON and OFF for
> 
> nit: not sure how much value is added by (sometimes) using upper case
> for certain things (ON, OFF, COLD, HOST, ...).
> 
[Bala] : will use lower case.

>> +	 * Wi-Fi/BT. Powering up the power sources will not enable BT, until
>> +	 * we send a POWER ON pulse at 115200. This algorithm will help to
> 
> 115200 what? bps I guess.
> 
[Bala] : will add bps.

>> +static int qca_wcn3990_init(struct hci_uart *hu, u32 *soc_ver)
>> +{
>> +	struct hci_dev *hdev = hu->hdev;
>> +	int i, ret = 1;
> 
> Initialization not necessary, more details below.
> 
>> +
>> +	/* WCN3990 is a discrete Bluetooth chip connected to APPS processor.
> 
> APPS is a Qualcomm specific term, and some QCA docs also call it
> APSS. Just say 'SoC' which is universally understood.
> 
[Bala]: will update to SoC.

>> +	 * sometimes we will face communication synchronization issues,
>> +	 * like reading version command timeouts. In which HCI_SETUP fails,
>> +	 * to overcome these issues, we try to communicate by performing an
>> +	 * COLD power OFF and ON.
>> +	 */
>> +	for (i = 1; i <= 10 && ret; i++) {
> 
> Is it really that bad that more than say 3 iterations might be needed?
> 
[Bala]: will restrict to 3 iterations.

> Common practice is to start loops with index 0.
> 

[Bala] : will init i=0

> The check for ret is not needed. All jumps to 'regs_off' are done
> when an error is detected. The loop is left when 'ret == 0' at the
> bottom.
> 
[Bala]: will update.

>> +		/* This helper will turn ON chip if it is powered off.
>> +		 * if the chip is already powered ON, function call will
>> +		 * return zero.
>> +		 */
> 
> Comments are great when they add value, IMO this one doesn't and just
> adds distraction. Most readers will assume that after
> qca_power_setup(hu, true) the chip is powered on, regardless of the
> previous power state.
> 
[Bala] : will remove the comment.

>> +		ret = qca_power_setup(hu, true);
>> +		if (ret)
>> +			goto regs_off;
>> +
>> +		/* Forcefully enable wcn3990 to enter in to boot mode. */
> 
> nit: Sometimes the comments and logs name the chip wcn3990, others
> WCN3990. Personally I don't care which spelling is used, but please be
> consistent.
> 
[Bala]: will use "wcn3990" through our the code.

>> +		host_set_baudrate(hu, 2400);
>> +		ret = qca_send_vendor_pulse(hdev, QCA_WCN3990_FORCE_BOOT_PULSE);
>> +		if (ret)
>> +			goto regs_off;
>> +
>> +		qca_set_speed(hu, QCA_INIT_SPEED);
>> +		ret = qca_send_vendor_pulse(hdev, QCA_WCN3990_POWERON_PULSE);
>> +		if (ret)
>> +			goto regs_off;
>> +
>> +		/* Wait for 100 ms for SoC to boot */
>> +		msleep(100);
>> +
>> +		/* Now the device is in ready state to communicate with host.
>> +		 * To sync HOST with device we need to reopen port.
>> +		 * Without this, we will have RTS and CTS synchronization
>> +		 * issues.
>> +		 */
>> +		serdev_device_close(hu->serdev);
>> +		ret = serdev_device_open(hu->serdev);
>> +		if (ret) {
>> +			bt_dev_err(hu->hdev, "failed to open port");
>> +			break;
>> +		}
>> +
>> +		hci_uart_set_flow_control(hu, false);
>> +		ret = qca_read_soc_version(hdev, soc_ver);
>> +		if (ret < 0 || soc_ver == 0)
>> +			bt_dev_err(hdev, "Failed to get version:%d", ret);
> 
> The check for soc_ver is/should be done in qca_read_soc_version(),
> same for the error log.
> 
[Bala]: will remove the check here.

>> +		if (!ret)
>> +			break;
>> +
>> +regs_off:
>> +		bt_dev_err(hdev, "retrying to establish communication: %d", i);
> 
> Use i + 1 if starting the loop at 0.
> 

[Bala] : will update.

>> +static const struct qca_vreg_data qca_soc_data = {
>> +	.soc_type = QCA_WCN3990,
>> +	.vregs = (struct qca_vreg []) {
>> +		{ "vddio",   1800000, 1800000,  15000  },
>> +		{ "vddxo",   1800000, 1800000,  80000  },
>> +		{ "vddrf",   1304000, 1304000,  300000 },
>> +		{ "vddch0",  3000000, 3312000,  450000 },
>> +	},
> 
> The currents of 300mA and 450mA seem high for Bluetooth, I'm not an
> expert in this area though, they might be reasonable peak currents for
> certain use cases.
> 

[Bala]: rf and ch0 are required for rf antenna of wcn3990. will double 
confirm on this.

>> +static int qca_power_shutdown(struct hci_dev *hdev)
>> +{
>> +	struct hci_uart *hu = hci_get_drvdata(hdev);
>> +
>> +	host_set_baudrate(hu, 2400);
>> +	qca_send_vendor_pulse(hdev, QCA_WCN3990_POWEROFF_PULSE);
>> +	return qca_power_setup(hu, false);
>> +}
> 
> The return value changed from void to int, but nobody ever checks it 
> ...
> 
[Bala] : will update.

>> +static void qca_regulator_get_current(struct device *dev,
>> +				      struct qca_vreg *vregs)
>> +{
>> +	char prop_name[32]; /* 32 is max size of property name */
>> +
>> +	/* We have different platforms where the load value is controlled
>> +	 * via PMIC controllers. In such cases load required to power ON
>> +	 * Bluetooth chips are defined in the PMIC. We have option to set
>> +	 * operation mode like high or low power modes.
>> +	 * We do have some platforms where driver need to enable the load 
>> for
>> +	 * WCN3990. Based on the current property value defined for the
>> +	 * regulators, driver will decide the regulator output load.
>> +	 * If the current property for the regulator is defined in the dts
>> +	 * we will read from dts tree, else from the default load values.
>> +	 */
> 
> Let's make sure we all really understand why this is needed. You
> mentioned RPMh regulators earlier and said a special value of 1uA
> would be needed to enable high power mode. Later when I pointed to the
> RPMh regulator code you agreed that this special value wouldn't make
> any difference.
> 
> Now the defaults are higher:
> 
[Bala]: today i got the info from the power teams here, currently in the 
downstream what we have is different wrt to the
         patch "https://patchwork.kernel.org/patch/10524299/" by David 
Collins.
         prior to his patch we have different architecture where 1uA will 
change the mode to HPM mode.
         which is not valid, so 1uA will not work any more. we have go 
with actual current values.

         coming back to reading current values from dts. we have reason 
for it.
         let us assume that later stages of wcn3990 if we have less 
current values than default values.
         instead of updating the driver again, we can assign the current 
no in the dts, which we will read.

         This is how it works.

         if(current value for the reg is declared in dts tree)
               consider the current value from the dts.
         else
            go with default value.

       pls let me know if you any queries.

>> +		{ "vddio",   1800000, 1800000,  15000  },
>> +		{ "vddxo",   1800000, 1800000,  80000  },
>> +		{ "vddrf",   1304000, 1304000,  300000 },
>> +		{ "vddch0",  3000000, 3312000,  450000 },
> 
> What would supposedly go wrong if these values were passed to one of
> the PMICs you are concerned about? Please be more specific than the
> above comment.
> 

>> +	snprintf(prop_name, 32, "%s-current", vregs->name);
>> +	BT_DBG("Looking up %s from device tree\n", prop_name);
> 
> '\n' not needed with BT_DBG()
> 
[Bala]: will update.
>> +
>> +	if (device_property_read_bool(dev, prop_name))
>> +		device_property_read_u32(dev, prop_name, &vregs->load_uA);
> 
> Why device_property_read_bool()?
> 
[Bala]: if the current prop is present we read from dts. else we go with 
default current no's.
         if block  is used to check whether the property is present in 
dts or not.
         this is required because before calling _regualtor_get_current() 
we hold the default current in the vregs[].
         if we skip the read bool here, if the current property is not 
present then the function call of device_property_read_u32() will assign 
zero the vregs[].
         so we miss the default current values.

         this how it work if we miss read_bool check

         //vregs hold default current
          device_property_read_u32(dev, prop_name, &vregs->load_uA);
          the above will read the current property value from dts store 
in the vregs.. if the property is missing in dts it will store zero.

>> +	BT_DBG("current %duA selected for regulator %s", vregs->load_uA,
>> +		vregs->name);
>> +}
>> +
>> +static int qca_init_regulators(struct qca_power *qca,
>> +				const struct qca_vreg_data *data)
>> +{
>> +	int i, num_vregs;
>> +	int load_uA;
>> +
>> +	num_vregs = data->num_vregs;
>> +	qca->vreg_bulk = devm_kzalloc(qca->dev, num_vregs *
>> +				      sizeof(struct regulator_bulk_data),
>> +				      GFP_KERNEL);
>> +	if (!qca->vreg_bulk)
>> +		return -ENOMEM;
>> +
>> +	qca->vreg_data = devm_kzalloc(qca->dev, sizeof(struct 
>> qca_vreg_data),
>> +				      GFP_KERNEL);
>> +	if (!qca->vreg_data)
>> +		return -ENOMEM;
>> +
>> +	qca->vreg_data->num_vregs = data->num_vregs;
>> +	qca->vreg_data->soc_type = data->soc_type;
>> +
>> +	qca->vreg_data->vregs = devm_kzalloc(qca->dev, num_vregs *
>> +				      sizeof(struct qca_vreg_data),
> 
> sizeof(struct qca_vreg)
> 
>> +				      GFP_KERNEL);
>> +
>> +	if (!qca->vreg_data->vregs)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < num_vregs; i++) {
>> +		/* copy regulator name, min voltage, max voltage */
>> +		qca->vreg_data->vregs[i].name = data->vregs[i].name;
>> +		qca->vreg_data->vregs[i].min_uV = data->vregs[i].min_uV;
>> +		qca->vreg_data->vregs[i].max_uV = data->vregs[i].max_uV;
>> +		load_uA = data->vregs[i].load_uA;
>> +		qca->vreg_data->vregs[i].load_uA = load_uA;
> 
> memcpy(&qca->vreg_data->vregs[i], &data->vregs[i]); ?
> 
> Or do it outside of the loop for all regulators at once.
> 
[Bala] : will use memcpy

>>  static int qca_serdev_probe(struct serdev_device *serdev)
>>  {
>>  	struct qca_serdev *qcadev;
>> +	const struct qca_vreg_data *data;
>>  	int err;
>> 
>>  	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
>> @@ -1069,47 +1418,87 @@ static int qca_serdev_probe(struct 
>> serdev_device *serdev)
>>  		return -ENOMEM;
>> 
>>  	qcadev->serdev_hu.serdev = serdev;
>> +	data = of_device_get_match_data(&serdev->dev);
>>  	serdev_device_set_drvdata(serdev, qcadev);
>> +	if (data && data->soc_type == QCA_WCN3990) {
>> +		qcadev->btsoc_type = QCA_WCN3990;
>> +		qcadev->bt_power = devm_kzalloc(&serdev->dev,
>> +						sizeof(struct qca_power),
>> +						GFP_KERNEL);
>> +		if (!qcadev->bt_power)
>> +			return -ENOMEM;
>> +
>> +		qcadev->bt_power->dev = &serdev->dev;
>> +		err = qca_init_regulators(qcadev->bt_power, data);
>> +		if (err) {
>> +			BT_ERR("Failed to init regulators:%d", err);
>> +			goto out;
>> +		}
>> 
>> -	qcadev->bt_en = devm_gpiod_get(&serdev->dev, "enable",
>> -				       GPIOD_OUT_LOW);
>> -	if (IS_ERR(qcadev->bt_en)) {
>> -		dev_err(&serdev->dev, "failed to acquire enable gpio\n");
>> -		return PTR_ERR(qcadev->bt_en);
>> -	}
>> +		qcadev->bt_power->vregs_on = false;
>> 
>> -	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);
>> -	}
>> +		/* Read max speed supported by wcn3990 from dts
>> +		 * tree. if max-speed property is not enabled in
>> +		 * dts, QCA driver will use default operating speed
>> +		 * from proto structure.
>> +		 */
> 
> The comment doesn't add much value.

[Bala]: will remove the comment.

> 
>> +		device_property_read_u32(&serdev->dev, "max-speed",
>> +					 &qcadev->oper_speed);
>> +		if (!qcadev->oper_speed)
>> +			BT_INFO("UART will pick default operating speed");
> 
> Not a change in this version, but BT_INFO seems a bit verbose, we
> should avoid spamming the kernel log.
[Bala] : will move above BT_INFO to BT_DBG.

Thanks for reviewing the patch. Pls let me know if you have quires.
will send v11 with updated comments

-- 
Regards
Balakrishna.

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

[Matthias Kaehlcke]

On Tue, Jul 24, 2018 at 09:25:16PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-07-24 01:24, Matthias Kaehlcke wrote:
> > On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
> > > +	 * sometimes we will face communication synchronization issues,
> > > +	 * like reading version command timeouts. In which HCI_SETUP fails,
> > > +	 * to overcome these issues, we try to communicate by performing an
> > > +	 * COLD power OFF and ON.
> > > +	 */
> > > +	for (i = 1; i <= 10 && ret; i++) {
> > 
> > Is it really that bad that more than say 3 iterations might be needed?
> > 
> [Bala]: will restrict to 3 iterations.

Is 3x expected to be enough to 'guarantee' as successful
initialization? Just wondered about the 10x since it suddendly changed
from 1x. What is the failure rate without retries?

Could you provide more information about the 'communication
synchronization issues'? Is the root cause understood? Maybe there is
a better way than retries.

> > > +static void qca_regulator_get_current(struct device *dev,
> > > +				      struct qca_vreg *vregs)
> > > +{
> > > +	char prop_name[32]; /* 32 is max size of property name */
> > > +
> > > +	/* We have different platforms where the load value is controlled
> > > +	 * via PMIC controllers. In such cases load required to power ON
> > > +	 * Bluetooth chips are defined in the PMIC. We have option to set
> > > +	 * operation mode like high or low power modes.
> > > +	 * We do have some platforms where driver need to enable the load
> > > for
> > > +	 * WCN3990. Based on the current property value defined for the
> > > +	 * regulators, driver will decide the regulator output load.
> > > +	 * If the current property for the regulator is defined in the dts
> > > +	 * we will read from dts tree, else from the default load values.
> > > +	 */
> > 
> > Let's make sure we all really understand why this is needed. You
> > mentioned RPMh regulators earlier and said a special value of 1uA
> > would be needed to enable high power mode. Later when I pointed to the
> > RPMh regulator code you agreed that this special value wouldn't make
> > any difference.
> > 
> > Now the defaults are higher:
> > 
> [Bala]: today i got the info from the power teams here, currently in the
> downstream what we have is different wrt to the
>         patch "https://patchwork.kernel.org/patch/10524299/" by David
> Collins.
>         prior to his patch we have different architecture where 1uA will
> change the mode to HPM mode.
>         which is not valid, so 1uA will not work any more. we have go with
> actual current values.

Ok, in any case downstream drivers shouldn't impact the design of
upstream drivers. If there are incompatibilities the BT driver needs
to be hacked in the downstream tree.

>         coming back to reading current values from dts. we have reason for
> it.
>         let us assume that later stages of wcn3990 if we have less current
> values than default values.
>         instead of updating the driver again, we can assign the current no
> in the dts, which we will read.
> 
>         This is how it works.
> 
>         if(current value for the reg is declared in dts tree)
>               consider the current value from the dts.
>         else
>            go with default value.
> 
>       pls let me know if you any queries.

If I understand correctly you describe a hypothetical situation of a
future wcn3990 variant having lower power requirements. I'd say let's
deal with this when these chips actually exist and need to be
supported by Linux. As of now it seems there is no need for current
limits in the DT.

> > > +	if (device_property_read_bool(dev, prop_name))
> > > +		device_property_read_u32(dev, prop_name, &vregs->load_uA);
> > 
> > Why device_property_read_bool()?
> > 
> [Bala]: if the current prop is present we read from dts. else we go with
> default current no's.
>         if block  is used to check whether the property is present in dts or
> not.
>         this is required because before calling _regualtor_get_current() we
> hold the default current in the vregs[].
>         if we skip the read bool here, if the current property is not
> present then the function call of device_property_read_u32() will assign
> zero the vregs[].
>         so we miss the default current values.
> 
>         this how it work if we miss read_bool check
> 
>         //vregs hold default current
>          device_property_read_u32(dev, prop_name, &vregs->load_uA);
>          the above will read the current property value from dts store in
> the vregs.. if the property is missing in dts it will store zero.

Where does of_property_read_u32() set the value to zero when the
property does not exist?

A simple test in a probe function:

{
	u32 v = 123;
	of_property_read_u32(node, "no-such-property", &v);
	printk("DBG: v = %d\n", v);
}

[    7.598366] DBG: v = 123

And looking at the code, of_property_read_u32() ends up in a call to
of_property_read_variable_u32_array():

int of_property_read_variable_u32_array(const struct device_node *np,
			       const char *propname, u32 *out_values,
			       size_t sz_min, size_t sz_max)
{
	size_t sz, count;
	const __be32 *val = of_find_property_value_of_size(np, propname,
						(sz_min * sizeof(*out_values)),
						(sz_max * sizeof(*out_values)),
						&sz);

	if (IS_ERR(val))
		return PTR_ERR(val);

	...
}

i.e. 'out_values' is not modified when the property does not exit.

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

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-07-26 00:01, Matthias Kaehlcke wrote:
> On Tue, Jul 24, 2018 at 09:25:16PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-07-24 01:24, Matthias Kaehlcke wrote:
>> > On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
>> > > +	 * sometimes we will face communication synchronization issues,
>> > > +	 * like reading version command timeouts. In which HCI_SETUP fails,
>> > > +	 * to overcome these issues, we try to communicate by performing an
>> > > +	 * COLD power OFF and ON.
>> > > +	 */
>> > > +	for (i = 1; i <= 10 && ret; i++) {
>> >
>> > Is it really that bad that more than say 3 iterations might be needed?
>> >
>> [Bala]: will restrict to 3 iterations.
> 
> Is 3x expected to be enough to 'guarantee' as successful
> initialization? Just wondered about the 10x since it suddendly changed
> from 1x. What is the failure rate without retries?
> 
> Could you provide more information about the 'communication
> synchronization issues'? Is the root cause understood? Maybe there is
> a better way than retries.
> 

[Bala]: basically before sending a every patch series we run a stress 
test to the driver to detect the bugs.
         in recent test results found one interesting bug that BT setups 
fails with version request timeouts,
         after we do a reboot for the device.
         we debugged the issue and found that wcn3900 is not responding 
to the version request commands
         sent by HOST. this is because before reboot, wcn3990 is in on 
state i.e. we are communicating to device.
         then we did a reboot and HOST is not sending a power off request 
to the regulators to turn off.
         so after reboot wcn3990 is still in ON state where it will not 
respond to version request commands which in turn fails HCI_SETUP.
         so we are sending the power off pulse and then sending the power 
on pulse.
         coming back to 3x or 10x iteration this is to avoid any such 
synchronization issues.
         i agreed for 3x because of stress test results. we have success 
rate of 99% for single iteration, where as 3x iterations will helps to 
handle 1% fails cases.

>> > > +static void qca_regulator_get_current(struct device *dev,
>> > > +				      struct qca_vreg *vregs)
>> > > +{
>> > > +	char prop_name[32]; /* 32 is max size of property name */
>> > > +
>> > > +	/* We have different platforms where the load value is controlled
>> > > +	 * via PMIC controllers. In such cases load required to power ON
>> > > +	 * Bluetooth chips are defined in the PMIC. We have option to set
>> > > +	 * operation mode like high or low power modes.
>> > > +	 * We do have some platforms where driver need to enable the load
>> > > for
>> > > +	 * WCN3990. Based on the current property value defined for the
>> > > +	 * regulators, driver will decide the regulator output load.
>> > > +	 * If the current property for the regulator is defined in the dts
>> > > +	 * we will read from dts tree, else from the default load values.
>> > > +	 */
>> >
>> > Let's make sure we all really understand why this is needed. You
>> > mentioned RPMh regulators earlier and said a special value of 1uA
>> > would be needed to enable high power mode. Later when I pointed to the
>> > RPMh regulator code you agreed that this special value wouldn't make
>> > any difference.
>> >
>> > Now the defaults are higher:
>> >
>> [Bala]: today i got the info from the power teams here, currently in 
>> the
>> downstream what we have is different wrt to the
>>         patch "https://patchwork.kernel.org/patch/10524299/" by David
>> Collins.
>>         prior to his patch we have different architecture where 1uA 
>> will
>> change the mode to HPM mode.
>>         which is not valid, so 1uA will not work any more. we have go 
>> with
>> actual current values.
> 
> Ok, in any case downstream drivers shouldn't impact the design of
> upstream drivers. If there are incompatibilities the BT driver needs
> to be hacked in the downstream tree.
> 
>>         coming back to reading current values from dts. we have reason 
>> for
>> it.
>>         let us assume that later stages of wcn3990 if we have less 
>> current
>> values than default values.
>>         instead of updating the driver again, we can assign the 
>> current no
>> in the dts, which we will read.
>> 
>>         This is how it works.
>> 
>>         if(current value for the reg is declared in dts tree)
>>               consider the current value from the dts.
>>         else
>>            go with default value.
>> 
>>       pls let me know if you any queries.
> 
> If I understand correctly you describe a hypothetical situation of a
> future wcn3990 variant having lower power requirements. I'd say let's
> deal with this when these chips actually exist and need to be
> supported by Linux. As of now it seems there is no need for current
> limits in the DT.
> 

[Bala]: will remove current property for dts.
         in previous mail you asked me a question for currents
         "The currents of 300mA and 450mA seem high for Bluetooth, I'm 
not an
          expert in this area though, they might be reasonable peak 
currents for
          certain use cases."

          yes we require 450mA and 300mA of current for rf and ch0 pins.  
setting regulator to required load will not pump load current to wcn3990
         it depends on operations, typical the above are the max current 
drawn by the two pins.

>> > > +	if (device_property_read_bool(dev, prop_name))
>> > > +		device_property_read_u32(dev, prop_name, &vregs->load_uA);
>> >
>> > Why device_property_read_bool()?
>> >
>> [Bala]: if the current prop is present we read from dts. else we go 
>> with
>> default current no's.
>>         if block  is used to check whether the property is present in 
>> dts or
>> not.
>>         this is required because before calling 
>> _regualtor_get_current() we
>> hold the default current in the vregs[].
>>         if we skip the read bool here, if the current property is not
>> present then the function call of device_property_read_u32() will 
>> assign
>> zero the vregs[].
>>         so we miss the default current values.
>> 
>>         this how it work if we miss read_bool check
>> 
>>         //vregs hold default current
>>          device_property_read_u32(dev, prop_name, &vregs->load_uA);
>>          the above will read the current property value from dts store 
>> in
>> the vregs.. if the property is missing in dts it will store zero.
> 
> Where does of_property_read_u32() set the value to zero when the
> property does not exist?
> 
> A simple test in a probe function:
> 
> {
> 	u32 v = 123;
> 	of_property_read_u32(node, "no-such-property", &v);
> 	printk("DBG: v = %d\n", v);
> }
> 
> [    7.598366] DBG: v = 123
> 
> And looking at the code, of_property_read_u32() ends up in a call to
> of_property_read_variable_u32_array():
> 
> int of_property_read_variable_u32_array(const struct device_node *np,
> 			       const char *propname, u32 *out_values,
> 			       size_t sz_min, size_t sz_max)
> {
> 	size_t sz, count;
> 	const __be32 *val = of_find_property_value_of_size(np, propname,
> 						(sz_min * sizeof(*out_values)),
> 						(sz_max * sizeof(*out_values)),
> 						&sz);
> 
> 	if (IS_ERR(val))
> 		return PTR_ERR(val);
> 
> 	...
> }
> 
> i.e. 'out_values' is not modified when the property does not exit.

[Bala]: Thanks for clarifying me.

-- 
Regards
Balakrishna.

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

[Matthias Kaehlcke]

On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-07-26 00:01, Matthias Kaehlcke wrote:
> > On Tue, Jul 24, 2018 at 09:25:16PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > > 
> > > On 2018-07-24 01:24, Matthias Kaehlcke wrote:
> > > > On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
> > > > > +	 * sometimes we will face communication synchronization issues,
> > > > > +	 * like reading version command timeouts. In which HCI_SETUP fails,
> > > > > +	 * to overcome these issues, we try to communicate by performing an
> > > > > +	 * COLD power OFF and ON.
> > > > > +	 */
> > > > > +	for (i = 1; i <= 10 && ret; i++) {
> > > >
> > > > Is it really that bad that more than say 3 iterations might be needed?
> > > >
> > > [Bala]: will restrict to 3 iterations.
> > 
> > Is 3x expected to be enough to 'guarantee' as successful
> > initialization? Just wondered about the 10x since it suddendly changed
> > from 1x. What is the failure rate without retries?
> > 
> > Could you provide more information about the 'communication
> > synchronization issues'? Is the root cause understood? Maybe there is
> > a better way than retries.
> > 
> 
> [Bala]: basically before sending a every patch series we run a stress test
> to the driver to detect the bugs.
>         in recent test results found one interesting bug that BT setups
> fails with version request timeouts,
>         after we do a reboot for the device.
>         we debugged the issue and found that wcn3900 is not responding to
> the version request commands
>         sent by HOST. this is because before reboot, wcn3990 is in on state
> i.e. we are communicating to device.
>         then we did a reboot and HOST is not sending a power off request to
> the regulators to turn off.
>         so after reboot wcn3990 is still in ON state where it will not
> respond to version request commands which in turn fails HCI_SETUP.
>         so we are sending the power off pulse and then sending the power on
> pulse.
>         coming back to 3x or 10x iteration this is to avoid any such
> synchronization issues.
>         i agreed for 3x because of stress test results. we have success rate
> of 99% for single iteration, where as 3x iterations will helps to handle 1%
> fails cases.

Thanks for the clarification. Couldn't you assure the device is in a
defined state by calling qca_power_shutdown() as one of the first
things in qca_wcn3990_init()?

Some more comments on the functions, for if the retry loop is kept:

> +static int qca_wcn3990_init(struct hci_uart *hu, u32 *soc_ver)
> +{
> +	struct hci_dev *hdev = hu->hdev;
> +	int i, ret = 1;
> +
> +	/* WCN3990 is a discrete Bluetooth chip connected to APPS processor.
> +	 * sometimes we will face communication synchronization issues,
> +	 * like reading version command timeouts. In which HCI_SETUP fails,
> +	 * to overcome these issues, we try to communicate by performing an
> +	 * COLD power OFF and ON.
> +	 */
> +	for (i = 1; i <= 10 && ret; i++) {
> +		/* This helper will turn ON chip if it is powered off.
> +		 * if the chip is already powered ON, function call will
> +		 * return zero.
> +		 */
> +		ret = qca_power_setup(hu, true);
> +		if (ret)
> +			goto regs_off;

A failure here is not caused by a communication problem, so this
should probably be a 'return' instead of a 'goto'.

> +
> +		/* Forcefully enable wcn3990 to enter in to boot mode. */
> +		host_set_baudrate(hu, 2400);
> +		ret = qca_send_vendor_pulse(hdev, QCA_WCN3990_FORCE_BOOT_PULSE);
> +		if (ret)
> +			goto regs_off;
> +
> +		qca_set_speed(hu, QCA_INIT_SPEED);
> +		ret = qca_send_vendor_pulse(hdev, QCA_WCN3990_POWERON_PULSE);
> +		if (ret)
> +			goto regs_off;
> +
> +		/* Wait for 100 ms for SoC to boot */
> +		msleep(100);
> +
> +		/* Now the device is in ready state to communicate with host.
> +		 * To sync HOST with device we need to reopen port.
> +		 * Without this, we will have RTS and CTS synchronization
> +		 * issues.
> +		 */
> +		serdev_device_close(hu->serdev);
> +		ret = serdev_device_open(hu->serdev);
> +		if (ret) {
> +			bt_dev_err(hu->hdev, "failed to open port");
> +			break;

			return ret;

> +		}
 > +
> +		hci_uart_set_flow_control(hu, false);
> +		ret = qca_read_soc_version(hdev, soc_ver);
> +		if (ret < 0 || soc_ver == 0)
> +			bt_dev_err(hdev, "Failed to get version:%d", ret);

nit: add a space between ':' and '%d'

> +
> +		if (!ret)
> +			break;

			return 0;

> +
> +regs_off:
> +		bt_dev_err(hdev, "retrying to establish communication: %d", i);
> +		qca_power_shutdown(hdev);

Is qca_power_shutdown() needed or would qca_power_setup(hu,
false) be enough? This is qca_power_shutdown():

static int qca_power_shutdown(struct hci_dev *hdev)
{
        struct hci_uart *hu = hci_get_drvdata(hdev);

        host_set_baudrate(hu, 2400);
        qca_send_vendor_pulse(hdev, QCA_WCN3990_POWEROFF_PULSE);
        return qca_power_setup(hu, false);
}

It additionally sends the power off pulse, which is also done in the
loop (though it is currently called QCA_WCN3990_FORCE_BOOT_PULSE).

The code flow with the gotos and the error handling at the end of the
loop is a bit messy. Moving the power down to the top of the loop
(basically in line with my comment above to get rid of the loop) would
help here. In this case checking 'ret' in the loop condition (which I
suggested to remove) would make sense, since it elimninates the need
for the break/return in the success case. But if we can do without the
loop even better :)

> > If I understand correctly you describe a hypothetical situation of a
> > future wcn3990 variant having lower power requirements. I'd say let's
> > deal with this when these chips actually exist and need to be
> > supported by Linux. As of now it seems there is no need for current
> > limits in the DT.
> > 
> 
> [Bala]: will remove current property for dts.
>         in previous mail you asked me a question for currents
>         "The currents of 300mA and 450mA seem high for Bluetooth, I'm not an
>          expert in this area though, they might be reasonable peak currents
> for
>          certain use cases."
> 
>          yes we require 450mA and 300mA of current for rf and ch0 pins.
> setting regulator to required load will not pump load current to wcn3990
>         it depends on operations, typical the above are the max current
> drawn by the two pins.

Ok, thanks for confirming.

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

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-07-26 00:01, Matthias Kaehlcke wrote:
>> > On Tue, Jul 24, 2018 at 09:25:16PM +0530, Balakrishna Godavarthi wrote:
>> > > Hi Matthias,
>> > >
>> > > On 2018-07-24 01:24, Matthias Kaehlcke wrote:
>> > > > On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
>> > > > > +	 * sometimes we will face communication synchronization issues,
>> > > > > +	 * like reading version command timeouts. In which HCI_SETUP fails,
>> > > > > +	 * to overcome these issues, we try to communicate by performing an
>> > > > > +	 * COLD power OFF and ON.
>> > > > > +	 */
>> > > > > +	for (i = 1; i <= 10 && ret; i++) {
>> > > >
>> > > > Is it really that bad that more than say 3 iterations might be needed?
>> > > >
>> > > [Bala]: will restrict to 3 iterations.
>> >
>> > Is 3x expected to be enough to 'guarantee' as successful
>> > initialization? Just wondered about the 10x since it suddendly changed
>> > from 1x. What is the failure rate without retries?
>> >
>> > Could you provide more information about the 'communication
>> > synchronization issues'? Is the root cause understood? Maybe there is
>> > a better way than retries.
>> >
>> 
>> [Bala]: basically before sending a every patch series we run a stress 
>> test
>> to the driver to detect the bugs.
>>         in recent test results found one interesting bug that BT 
>> setups
>> fails with version request timeouts,
>>         after we do a reboot for the device.
>>         we debugged the issue and found that wcn3900 is not responding 
>> to
>> the version request commands
>>         sent by HOST. this is because before reboot, wcn3990 is in on 
>> state
>> i.e. we are communicating to device.
>>         then we did a reboot and HOST is not sending a power off 
>> request to
>> the regulators to turn off.
>>         so after reboot wcn3990 is still in ON state where it will not
>> respond to version request commands which in turn fails HCI_SETUP.
>>         so we are sending the power off pulse and then sending the 
>> power on
>> pulse.
>>         coming back to 3x or 10x iteration this is to avoid any such
>> synchronization issues.
>>         i agreed for 3x because of stress test results. we have 
>> success rate
>> of 99% for single iteration, where as 3x iterations will helps to 
>> handle 1%
>> fails cases.
> 
> Thanks for the clarification. Couldn't you assure the device is in a
> defined state by calling qca_power_shutdown() as one of the first
> things in qca_wcn3990_init()?

[Bala]:  we have reasons behind writing qca_power_setup(true) at the 
start.

         1. the reason to add iteration here, is to handle BT fails cases 
either due to communication failure of wcn3900 or due to regulator 
issues.
            before calling qca_setup(), we have our regulator turned on 
and in qca_setup i.e. init routine if we added power_shutdown as first 
statement before
            communicating with chip then regulator will be off and again 
we need to call function to ON regulators.
            so it could be some thing like this

            init(){

                for () {
                      shutdown() // regs are off
                      poweron(true) // regs are on.
                      if(!start communicating with chip()) {
                         break;
                       }

                }
            }
            as the reason to add the iteration handling is to overcome 1% 
of fail cases, so every time when we call it will turn off the regs and 
turn it back. which require an turning in off regs and on it back for 
99% pass
             cases.

         2. this is the one of the main reason for adding 
qca_power_setup(true) in the init() function first.
            as we know that power management is so critical for long 
lasting of battery.
            now present implementation is when we off BT from UI i.e. 
hci0 down, we put BT into an suspend or low power mode, as soon as we 
turn ON the BT back from UI we make hci0 up.
            the above is putting device into suspend state and bring it 
back where the regulator are still on state. so we will have leakage 
currents which can be minimal or may be in few mA.
            to over come the above case, we want to do an cold on/off for 
BT chip wcn3990. i.e. when bt is off from UI, we will off the regulators 
and turn on it again once the BT is ON from UI.
            every time we disable i.e. off BT from UI we will call 
hdev->shutdown() i.e. completely powering off the chip.
            so it require an reprogram again, when we turn ON BT from UI. 
it will call qca_setup()--> init().. so here actually qca_power_on(true) 
will turn on the chip and dump the fw files into it.
            so that is also a reason behind to write power on first.

            the above feature is under testing state, will post a patch 
series once the driver code merged to bt-next.

> 
> Some more comments on the functions, for if the retry loop is kept:
> 
>> +static int qca_wcn3990_init(struct hci_uart *hu, u32 *soc_ver)
>> +{
>> +	struct hci_dev *hdev = hu->hdev;
>> +	int i, ret = 1;
>> +
>> +	/* WCN3990 is a discrete Bluetooth chip connected to APPS processor.
>> +	 * sometimes we will face communication synchronization issues,
>> +	 * like reading version command timeouts. In which HCI_SETUP fails,
>> +	 * to overcome these issues, we try to communicate by performing an
>> +	 * COLD power OFF and ON.
>> +	 */
>> +	for (i = 1; i <= 10 && ret; i++) {
>> +		/* This helper will turn ON chip if it is powered off.
>> +		 * if the chip is already powered ON, function call will
>> +		 * return zero.
>> +		 */
>> +		ret = qca_power_setup(hu, true);
>> +		if (ret)
>> +			goto regs_off;
> 
> A failure here is not caused by a communication problem, so this
> should probably be a 'return' instead of a 'goto'.
> 

[Bala]: yes you are true, but if any chance we have issue with regulator 
to turn on, we try to turn on them again.
        so that HCI_SETUP should not fail.

>> +
>> +		/* Forcefully enable wcn3990 to enter in to boot mode. */
>> +		host_set_baudrate(hu, 2400);
>> +		ret = qca_send_vendor_pulse(hdev, QCA_WCN3990_FORCE_BOOT_PULSE);
>> +		if (ret)
>> +			goto regs_off;
>> +
>> +		qca_set_speed(hu, QCA_INIT_SPEED);
>> +		ret = qca_send_vendor_pulse(hdev, QCA_WCN3990_POWERON_PULSE);
>> +		if (ret)
>> +			goto regs_off;
>> +
>> +		/* Wait for 100 ms for SoC to boot */
>> +		msleep(100);
>> +
>> +		/* Now the device is in ready state to communicate with host.
>> +		 * To sync HOST with device we need to reopen port.
>> +		 * Without this, we will have RTS and CTS synchronization
>> +		 * issues.
>> +		 */
>> +		serdev_device_close(hu->serdev);
>> +		ret = serdev_device_open(hu->serdev);
>> +		if (ret) {
>> +			bt_dev_err(hu->hdev, "failed to open port");
>> +			break;
> 
> 			return ret;
> 
>> +		}
>  > +
>> +		hci_uart_set_flow_control(hu, false);
>> +		ret = qca_read_soc_version(hdev, soc_ver);
>> +		if (ret < 0 || soc_ver == 0)
>> +			bt_dev_err(hdev, "Failed to get version:%d", ret);
> 
> nit: add a space between ':' and '%d'
> 

[Bala]: will update.

>> +
>> +		if (!ret)
>> +			break;
> 
> 			return 0;
> 
>> +
>> +regs_off:
>> +		bt_dev_err(hdev, "retrying to establish communication: %d", i);
>> +		qca_power_shutdown(hdev);
> 
> Is qca_power_shutdown() needed or would qca_power_setup(hu,
> false) be enough? This is qca_power_shutdown():
> 

[Bala]: qca_power_shutdown is needed, as we need to send power pulse at 
2400 bps before turning off the soc.
         this is an complete turn off BT portion in wcn3990.

> static int qca_power_shutdown(struct hci_dev *hdev)
> {
>         struct hci_uart *hu = hci_get_drvdata(hdev);
> 
>         host_set_baudrate(hu, 2400);
>         qca_send_vendor_pulse(hdev, QCA_WCN3990_POWEROFF_PULSE);
>         return qca_power_setup(hu, false);
> }
> 
> It additionally sends the power off pulse, which is also done in the
> loop (though it is currently called QCA_WCN3990_FORCE_BOOT_PULSE).
> 
> The code flow with the gotos and the error handling at the end of the
> loop is a bit messy. Moving the power down to the top of the loop
> (basically in line with my comment above to get rid of the loop) would
> help here. In this case checking 'ret' in the loop condition (which I
> suggested to remove) would make sense, since it elimninates the need
> for the break/return in the success case. But if we can do without the
> loop even better :)
> 

[Bala]: there is a reason to add the loop here, here we go with reason 
to add.
         let us assume that qca_setup fails to establish a communication 
with wcn3990
         then next steps will not be pass and we can't populate hci0 
rfkill entry.
         in traditional bluez stack i.e. bluetoothd daemon will looks for 
hci0, if we have entry for hci0
         then only BT option is visible in UI or else BT option will not 
be available in UI.
         we don't have any mechanism handled in bluez user space to 
reinitiate the communication at latest to try for second time to make 
hci0 up.
         so that is reason behind to add so that we can handle fault 
handling of wcn3990 and establish the communication to make BT option 
available in BT.

>> > If I understand correctly you describe a hypothetical situation of a
>> > future wcn3990 variant having lower power requirements. I'd say let's
>> > deal with this when these chips actually exist and need to be
>> > supported by Linux. As of now it seems there is no need for current
>> > limits in the DT.
>> >
>> 
>> [Bala]: will remove current property for dts.
>>         in previous mail you asked me a question for currents
>>         "The currents of 300mA and 450mA seem high for Bluetooth, I'm 
>> not an
>>          expert in this area though, they might be reasonable peak 
>> currents
>> for
>>          certain use cases."
>> 
>>          yes we require 450mA and 300mA of current for rf and ch0 
>> pins.
>> setting regulator to required load will not pump load current to 
>> wcn3990
>>         it depends on operations, typical the above are the max 
>> current
>> drawn by the two pins.
> 
> Ok, thanks for confirming.

Pls let me know if you require more info :)

-- 
Regards
Balakrishna.

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

[Matthias Kaehlcke]

On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > > 
> > > On 2018-07-26 00:01, Matthias Kaehlcke wrote:
> > > > On Tue, Jul 24, 2018 at 09:25:16PM +0530, Balakrishna Godavarthi wrote:
> > > > > Hi Matthias,
> > > > >
> > > > > On 2018-07-24 01:24, Matthias Kaehlcke wrote:
> > > > > > On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
> > > > > > > +	 * sometimes we will face communication synchronization issues,
> > > > > > > +	 * like reading version command timeouts. In which HCI_SETUP fails,
> > > > > > > +	 * to overcome these issues, we try to communicate by performing an
> > > > > > > +	 * COLD power OFF and ON.
> > > > > > > +	 */
> > > > > > > +	for (i = 1; i <= 10 && ret; i++) {
> > > > > >
> > > > > > Is it really that bad that more than say 3 iterations might be needed?
> > > > > >
> > > > > [Bala]: will restrict to 3 iterations.
> > > >
> > > > Is 3x expected to be enough to 'guarantee' as successful
> > > > initialization? Just wondered about the 10x since it suddendly changed
> > > > from 1x. What is the failure rate without retries?
> > > >
> > > > Could you provide more information about the 'communication
> > > > synchronization issues'? Is the root cause understood? Maybe there is
> > > > a better way than retries.
> > > >
> > > 
> > > [Bala]: basically before sending a every patch series we run a
> > > stress test
> > > to the driver to detect the bugs.
> > >         in recent test results found one interesting bug that BT
> > > setups
> > > fails with version request timeouts,
> > >         after we do a reboot for the device.
> > >         we debugged the issue and found that wcn3900 is not
> > > responding to
> > > the version request commands
> > >         sent by HOST. this is because before reboot, wcn3990 is in
> > > on state
> > > i.e. we are communicating to device.
> > >         then we did a reboot and HOST is not sending a power off
> > > request to
> > > the regulators to turn off.
> > >         so after reboot wcn3990 is still in ON state where it will not
> > > respond to version request commands which in turn fails HCI_SETUP.
> > >         so we are sending the power off pulse and then sending the
> > > power on
> > > pulse.
> > >         coming back to 3x or 10x iteration this is to avoid any such
> > > synchronization issues.
> > >         i agreed for 3x because of stress test results. we have
> > > success rate
> > > of 99% for single iteration, where as 3x iterations will helps to
> > > handle 1%
> > > fails cases.
> > 
> > Thanks for the clarification. Couldn't you assure the device is in a
> > defined state by calling qca_power_shutdown() as one of the first
> > things in qca_wcn3990_init()?
> 
> [Bala]:  we have reasons behind writing qca_power_setup(true) at the start.
> 
>         1. the reason to add iteration here, is to handle BT fails cases
> either due to communication failure of wcn3900 or due to regulator issues.
>            before calling qca_setup(), we have our regulator turned on and
> in qca_setup i.e. init routine if we added power_shutdown as first statement
> before
>            communicating with chip then regulator will be off and again we
> need to call function to ON regulators.
>            so it could be some thing like this
> 
>            init(){
> 
>                for () {
>                      shutdown() // regs are off
>                      poweron(true) // regs are on.
>                      if(!start communicating with chip()) {
>                         break;
>                       }
> 
>                }
>            }
>            as the reason to add the iteration handling is to overcome 1% of
> fail cases, so every time when we call it will turn off the regs and turn it
> back. which require an turning in off regs and on it back for 99% pass
>             cases.

But would turning off the regs really add a significant delay here?
The setup is already really slow, with a 100ms delay in the
loop (still wonder if booting the chip without loading firmware really
takes that long) and later the firmware is loaded.

If the chip needs to be in a defined state we should make sure to put
in into that state, unless there is a significant overhead wrt 'try
first and only reset in case of failure'. As a nice side effect the
code would be cleaner and we probably could get rid of the loop
completely, since it's supposed to address the case where the chip
wasn't properly reset on a reboot.

>         2. this is the one of the main reason for adding
> qca_power_setup(true) in the init() function first.
>            as we know that power management is so critical for long lasting
> of battery.
>            now present implementation is when we off BT from UI i.e. hci0
> down, we put BT into an suspend or low power mode, as soon as we turn ON the
> BT back from UI we make hci0 up.
>            the above is putting device into suspend state and bring it back
> where the regulator are still on state. so we will have leakage currents
> which can be minimal or may be in few mA.
>            to over come the above case, we want to do an cold on/off for BT
> chip wcn3990. i.e. when bt is off from UI, we will off the regulators and
> turn on it again once the BT is ON from UI.
>            every time we disable i.e. off BT from UI we will call
> hdev->shutdown() i.e. completely powering off the chip.
>            so it require an reprogram again, when we turn ON BT from UI. it
> will call qca_setup()--> init().. so here actually qca_power_on(true) will
> turn on the chip and dump the fw files into it.
>            so that is also a reason behind to write power on first.
> 
>            the above feature is under testing state, will post a patch
> series once the driver code merged to bt-next.

Thanks for the info.

If I understand correctly what you describe isn't incompatible with
performing a proper reset. 'vregs_on' can be checked to avoid
disabling already disabled regulators:

  if (qcadev->bt_power->vregs_on)
          qca_power_shutdown(hdev);

  // short delay needed here?

  qca_power_setup(hu, true);

Unless there are drawbacks that I'm missing I think that's preferable
over the retry loop.

> > The code flow with the gotos and the error handling at the end of the
> > loop is a bit messy. Moving the power down to the top of the loop
> > (basically in line with my comment above to get rid of the loop) would
> > help here. In this case checking 'ret' in the loop condition (which I
> > suggested to remove) would make sense, since it elimninates the need
> > for the break/return in the success case. But if we can do without the
> > loop even better :)
> > 
> 
> [Bala]: there is a reason to add the loop here, here we go with reason to
> add.
>         let us assume that qca_setup fails to establish a communication with
> wcn3990
>         then next steps will not be pass and we can't populate hci0 rfkill
> entry.
>         in traditional bluez stack i.e. bluetoothd daemon will looks for
> hci0, if we have entry for hci0
>         then only BT option is visible in UI or else BT option will not be
> available in UI.
>         we don't have any mechanism handled in bluez user space to
> reinitiate the communication at latest to try for second time to make hci0
> up.
>         so that is reason behind to add so that we can handle fault handling
> of wcn3990 and establish the communication to make BT option available in
> BT.

The loop is supposed to address the following case (quoting you from
earlier discussion in the thread):

> found one interesting bug that BT setups fails with version request
> timeouts, after we do a reboot for the device. we debugged the issue
> and found that wcn3900 is not responding to the version request
> commands sent by HOST. this is because before reboot, wcn3990 is in
> on state i.e. we are communicating to device. then we did a reboot
> and HOST is not sending a power off request to the regulators to
> turn off. so after reboot wcn3990 is still in ON state where it will
> not respond to version request commands which in turn fails
> HCI_SETUP. so we are sending the power off pulse and then sending the
> power on pulse.

My suggestion to address this failure case is to reset/power off the
chip before initializing it. With that the loop shouldn't be needed,
actually it wasn't there before you found this specific error.

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

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-07-31 01:37, Matthias Kaehlcke wrote:
> On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-07-27 01:21, Matthias Kaehlcke wrote:
>> > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
>> > > Hi Matthias,
>> > >
>> > > On 2018-07-26 00:01, Matthias Kaehlcke wrote:
>> > > > On Tue, Jul 24, 2018 at 09:25:16PM +0530, Balakrishna Godavarthi wrote:
>> > > > > Hi Matthias,
>> > > > >
>> > > > > On 2018-07-24 01:24, Matthias Kaehlcke wrote:
>> > > > > > On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
>> > > > > > > +	 * sometimes we will face communication synchronization issues,
>> > > > > > > +	 * like reading version command timeouts. In which HCI_SETUP fails,
>> > > > > > > +	 * to overcome these issues, we try to communicate by performing an
>> > > > > > > +	 * COLD power OFF and ON.
>> > > > > > > +	 */
>> > > > > > > +	for (i = 1; i <= 10 && ret; i++) {
>> > > > > >
>> > > > > > Is it really that bad that more than say 3 iterations might be needed?
>> > > > > >
>> > > > > [Bala]: will restrict to 3 iterations.
>> > > >
>> > > > Is 3x expected to be enough to 'guarantee' as successful
>> > > > initialization? Just wondered about the 10x since it suddendly changed
>> > > > from 1x. What is the failure rate without retries?
>> > > >
>> > > > Could you provide more information about the 'communication
>> > > > synchronization issues'? Is the root cause understood? Maybe there is
>> > > > a better way than retries.
>> > > >
>> > >
>> > > [Bala]: basically before sending a every patch series we run a
>> > > stress test
>> > > to the driver to detect the bugs.
>> > >         in recent test results found one interesting bug that BT
>> > > setups
>> > > fails with version request timeouts,
>> > >         after we do a reboot for the device.
>> > >         we debugged the issue and found that wcn3900 is not
>> > > responding to
>> > > the version request commands
>> > >         sent by HOST. this is because before reboot, wcn3990 is in
>> > > on state
>> > > i.e. we are communicating to device.
>> > >         then we did a reboot and HOST is not sending a power off
>> > > request to
>> > > the regulators to turn off.
>> > >         so after reboot wcn3990 is still in ON state where it will not
>> > > respond to version request commands which in turn fails HCI_SETUP.
>> > >         so we are sending the power off pulse and then sending the
>> > > power on
>> > > pulse.
>> > >         coming back to 3x or 10x iteration this is to avoid any such
>> > > synchronization issues.
>> > >         i agreed for 3x because of stress test results. we have
>> > > success rate
>> > > of 99% for single iteration, where as 3x iterations will helps to
>> > > handle 1%
>> > > fails cases.
>> >
>> > Thanks for the clarification. Couldn't you assure the device is in a
>> > defined state by calling qca_power_shutdown() as one of the first
>> > things in qca_wcn3990_init()?
>> 
>> [Bala]:  we have reasons behind writing qca_power_setup(true) at the 
>> start.
>> 
>>         1. the reason to add iteration here, is to handle BT fails 
>> cases
>> either due to communication failure of wcn3900 or due to regulator 
>> issues.
>>            before calling qca_setup(), we have our regulator turned on 
>> and
>> in qca_setup i.e. init routine if we added power_shutdown as first 
>> statement
>> before
>>            communicating with chip then regulator will be off and 
>> again we
>> need to call function to ON regulators.
>>            so it could be some thing like this
>> 
>>            init(){
>> 
>>                for () {
>>                      shutdown() // regs are off
>>                      poweron(true) // regs are on.
>>                      if(!start communicating with chip()) {
>>                         break;
>>                       }
>> 
>>                }
>>            }
>>            as the reason to add the iteration handling is to overcome 
>> 1% of
>> fail cases, so every time when we call it will turn off the regs and 
>> turn it
>> back. which require an turning in off regs and on it back for 99% pass
>>             cases.
> 
> But would turning off the regs really add a significant delay here?
> The setup is already really slow, with a 100ms delay in the
> loop (still wonder if booting the chip without loading firmware really
> takes that long) and later the firmware is loaded.
> 
[Bala]: By default we will have an firmware loaded in ROM of wcn3990, 
100 ms delay will help wcn3990 boot up with default firmware.
         Once it is booted up with default firmware on ROM, we will 
download the firmware from the firmware files, these firmware files
         contains bug fixes of wcn3990. Once the firmware files are 
loaded we will send the reset command to wcn3990.
         wcn3990 will start working with latest firmware which is loaded.

> If the chip needs to be in a defined state we should make sure to put
> in into that state, unless there is a significant overhead wrt 'try
> first and only reset in case of failure'. As a nice side effect the
> code would be cleaner and we probably could get rid of the loop
> completely, since it's supposed to address the case where the chip
> wasn't properly reset on a reboot.
> 

[Bala]: i too agree with you, Now we have observed issue because of 
reboot. But let us take a real time example here.
         if clocks of UART are not stable or there is an issue of UART 
GPIO's or some thing related might have broken in UART. Then will have 
communication issues with BT chip.
         where HCI_SETUP fails, instead of giving a fails status, we are 
trying to communicate once again and these is also be in 1% of fail 
cases.

>>         2. this is the one of the main reason for adding
>> qca_power_setup(true) in the init() function first.
>>            as we know that power management is so critical for long 
>> lasting
>> of battery.
>>            now present implementation is when we off BT from UI i.e. 
>> hci0
>> down, we put BT into an suspend or low power mode, as soon as we turn 
>> ON the
>> BT back from UI we make hci0 up.
>>            the above is putting device into suspend state and bring it 
>> back
>> where the regulator are still on state. so we will have leakage 
>> currents
>> which can be minimal or may be in few mA.
>>            to over come the above case, we want to do an cold on/off 
>> for BT
>> chip wcn3990. i.e. when bt is off from UI, we will off the regulators 
>> and
>> turn on it again once the BT is ON from UI.
>>            every time we disable i.e. off BT from UI we will call
>> hdev->shutdown() i.e. completely powering off the chip.
>>            so it require an reprogram again, when we turn ON BT from 
>> UI. it
>> will call qca_setup()--> init().. so here actually qca_power_on(true) 
>> will
>> turn on the chip and dump the fw files into it.
>>            so that is also a reason behind to write power on first.
>> 
>>            the above feature is under testing state, will post a patch
>> series once the driver code merged to bt-next.
> 
> Thanks for the info.
> 
> If I understand correctly what you describe isn't incompatible with
> performing a proper reset. 'vregs_on' can be checked to avoid
> disabling already disabled regulators:
> 
>   if (qcadev->bt_power->vregs_on)
>           qca_power_shutdown(hdev);
> 
>   // short delay needed here?
> 
>   qca_power_setup(hu, true);
> 
> Unless there are drawbacks that I'm missing I think that's preferable
> over the retry loop.
> 
>> > The code flow with the gotos and the error handling at the end of the
>> > loop is a bit messy. Moving the power down to the top of the loop
>> > (basically in line with my comment above to get rid of the loop) would
>> > help here. In this case checking 'ret' in the loop condition (which I
>> > suggested to remove) would make sense, since it elimninates the need
>> > for the break/return in the success case. But if we can do without the
>> > loop even better :)
>> >
>> 
>> [Bala]: there is a reason to add the loop here, here we go with reason 
>> to
>> add.
>>         let us assume that qca_setup fails to establish a 
>> communication with
>> wcn3990
>>         then next steps will not be pass and we can't populate hci0 
>> rfkill
>> entry.
>>         in traditional bluez stack i.e. bluetoothd daemon will looks 
>> for
>> hci0, if we have entry for hci0
>>         then only BT option is visible in UI or else BT option will 
>> not be
>> available in UI.
>>         we don't have any mechanism handled in bluez user space to
>> reinitiate the communication at latest to try for second time to make 
>> hci0
>> up.
>>         so that is reason behind to add so that we can handle fault 
>> handling
>> of wcn3990 and establish the communication to make BT option available 
>> in
>> BT.
> 
> The loop is supposed to address the following case (quoting you from
> earlier discussion in the thread):
> 
>> found one interesting bug that BT setups fails with version request
>> timeouts, after we do a reboot for the device. we debugged the issue
>> and found that wcn3900 is not responding to the version request
>> commands sent by HOST. this is because before reboot, wcn3990 is in
>> on state i.e. we are communicating to device. then we did a reboot
>> and HOST is not sending a power off request to the regulators to
>> turn off. so after reboot wcn3990 is still in ON state where it will
>> not respond to version request commands which in turn fails
>> HCI_SETUP. so we are sending the power off pulse and then sending the
>> power on pulse.
> 
> My suggestion to address this failure case is to reset/power off the
> chip before initializing it. With that the loop shouldn't be needed,
> actually it wasn't there before you found this specific error.

[Bala]: I will update loop according to your suggestions, But i am 
little bit worried, if the HCI_SETUP fails due to some
         external issue of UART, so in that cases we can't able to handle 
with out loop and as i said, some operating system,
        will not handle HCI SETUP failures, we at the driver need to 
handle such cases too.

-- 
Regards
Balakrishna.

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

[Matthias Kaehlcke]

On Tue, Jul 31, 2018 at 08:08:40PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-07-31 01:37, Matthias Kaehlcke wrote:
> > On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > > 
> > > On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> > > > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
> > > > > Hi Matthias,
> > > > >
> > > > > On 2018-07-26 00:01, Matthias Kaehlcke wrote:
> > > > > > On Tue, Jul 24, 2018 at 09:25:16PM +0530, Balakrishna Godavarthi wrote:
> > > > > > > Hi Matthias,
> > > > > > >
> > > > > > > On 2018-07-24 01:24, Matthias Kaehlcke wrote:
> > > > > > > > On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
> > > > > > > > > +	 * sometimes we will face communication synchronization issues,
> > > > > > > > > +	 * like reading version command timeouts. In which HCI_SETUP fails,
> > > > > > > > > +	 * to overcome these issues, we try to communicate by performing an
> > > > > > > > > +	 * COLD power OFF and ON.
> > > > > > > > > +	 */
> > > > > > > > > +	for (i = 1; i <= 10 && ret; i++) {
> > > > > > > >
> > > > > > > > Is it really that bad that more than say 3 iterations might be needed?
> > > > > > > >
> > > > > > > [Bala]: will restrict to 3 iterations.
> > > > > >
> > > > > > Is 3x expected to be enough to 'guarantee' as successful
> > > > > > initialization? Just wondered about the 10x since it suddendly changed
> > > > > > from 1x. What is the failure rate without retries?
> > > > > >
> > > > > > Could you provide more information about the 'communication
> > > > > > synchronization issues'? Is the root cause understood? Maybe there is
> > > > > > a better way than retries.
> > > > > >
> > > > >
> > > > > [Bala]: basically before sending a every patch series we run a
> > > > > stress test
> > > > > to the driver to detect the bugs.
> > > > >         in recent test results found one interesting bug that BT
> > > > > setups
> > > > > fails with version request timeouts,
> > > > >         after we do a reboot for the device.
> > > > >         we debugged the issue and found that wcn3900 is not
> > > > > responding to
> > > > > the version request commands
> > > > >         sent by HOST. this is because before reboot, wcn3990 is in
> > > > > on state
> > > > > i.e. we are communicating to device.
> > > > >         then we did a reboot and HOST is not sending a power off
> > > > > request to
> > > > > the regulators to turn off.
> > > > >         so after reboot wcn3990 is still in ON state where it will not
> > > > > respond to version request commands which in turn fails HCI_SETUP.
> > > > >         so we are sending the power off pulse and then sending the
> > > > > power on
> > > > > pulse.
> > > > >         coming back to 3x or 10x iteration this is to avoid any such
> > > > > synchronization issues.
> > > > >         i agreed for 3x because of stress test results. we have
> > > > > success rate
> > > > > of 99% for single iteration, where as 3x iterations will helps to
> > > > > handle 1%
> > > > > fails cases.
> > > >
> > > > Thanks for the clarification. Couldn't you assure the device is in a
> > > > defined state by calling qca_power_shutdown() as one of the first
> > > > things in qca_wcn3990_init()?
> > > 
> > > [Bala]:  we have reasons behind writing qca_power_setup(true) at the
> > > start.
> > > 
> > >         1. the reason to add iteration here, is to handle BT fails
> > > cases
> > > either due to communication failure of wcn3900 or due to regulator
> > > issues.
> > >            before calling qca_setup(), we have our regulator turned
> > > on and
> > > in qca_setup i.e. init routine if we added power_shutdown as first
> > > statement
> > > before
> > >            communicating with chip then regulator will be off and
> > > again we
> > > need to call function to ON regulators.
> > >            so it could be some thing like this
> > > 
> > >            init(){
> > > 
> > >                for () {
> > >                      shutdown() // regs are off
> > >                      poweron(true) // regs are on.
> > >                      if(!start communicating with chip()) {
> > >                         break;
> > >                       }
> > > 
> > >                }
> > >            }
> > >            as the reason to add the iteration handling is to
> > > overcome 1% of
> > > fail cases, so every time when we call it will turn off the regs and
> > > turn it
> > > back. which require an turning in off regs and on it back for 99% pass
> > >             cases.
> > 
> > But would turning off the regs really add a significant delay here?
> > The setup is already really slow, with a 100ms delay in the
> > loop (still wonder if booting the chip without loading firmware really
> > takes that long) and later the firmware is loaded.
> > 
> [Bala]: By default we will have an firmware loaded in ROM of wcn3990, 100 ms
> delay will help wcn3990 boot up with default firmware.

Ok, thanks.

>         Once it is booted up with default firmware on ROM, we will download
> the firmware from the firmware files, these firmware files
>         contains bug fixes of wcn3990. Once the firmware files are loaded we
> will send the reset command to wcn3990.
>         wcn3990 will start working with latest firmware which is loaded.
> 
> > If the chip needs to be in a defined state we should make sure to put
> > in into that state, unless there is a significant overhead wrt 'try
> > first and only reset in case of failure'. As a nice side effect the
> > code would be cleaner and we probably could get rid of the loop
> > completely, since it's supposed to address the case where the chip
> > wasn't properly reset on a reboot.
> > 
> 
> [Bala]: i too agree with you, Now we have observed issue because of reboot.
> But let us take a real time example here.
>         if clocks of UART are not stable or there is an issue of UART GPIO's
> or some thing related might have broken in UART. Then will have
> communication issues with BT chip.
>         where HCI_SETUP fails, instead of giving a fails status, we are
> trying to communicate once again and these is also be in 1% of fail cases.

If there are issues with unstable clocks, GPIOs or similar the problem
should be fixed at it's root, instead of papering over it in the BT
driver. Kernel drivers would be a complete mess if they tried to
recover from all possible problems in other subsystems.

I don't have general objections agains a retry loop when it is really
needed, but I don't have the impressions this is the case here. If
your stress tests reveal problems that can't be addresses otherwise
please let us know and we might end up with the retry loop after all.

> > >         2. this is the one of the main reason for adding
> > > qca_power_setup(true) in the init() function first.
> > >            as we know that power management is so critical for long
> > > lasting
> > > of battery.
> > >            now present implementation is when we off BT from UI i.e.
> > > hci0
> > > down, we put BT into an suspend or low power mode, as soon as we
> > > turn ON the
> > > BT back from UI we make hci0 up.
> > >            the above is putting device into suspend state and bring
> > > it back
> > > where the regulator are still on state. so we will have leakage
> > > currents
> > > which can be minimal or may be in few mA.
> > >            to over come the above case, we want to do an cold on/off
> > > for BT
> > > chip wcn3990. i.e. when bt is off from UI, we will off the
> > > regulators and
> > > turn on it again once the BT is ON from UI.
> > >            every time we disable i.e. off BT from UI we will call
> > > hdev->shutdown() i.e. completely powering off the chip.
> > >            so it require an reprogram again, when we turn ON BT from
> > > UI. it
> > > will call qca_setup()--> init().. so here actually
> > > qca_power_on(true) will
> > > turn on the chip and dump the fw files into it.
> > >            so that is also a reason behind to write power on first.
> > > 
> > >            the above feature is under testing state, will post a patch
> > > series once the driver code merged to bt-next.
> > 
> > Thanks for the info.
> > 
> > If I understand correctly what you describe isn't incompatible with
> > performing a proper reset. 'vregs_on' can be checked to avoid
> > disabling already disabled regulators:
> > 
> >   if (qcadev->bt_power->vregs_on)
> >           qca_power_shutdown(hdev);
> > 
> >   // short delay needed here?
> > 
> >   qca_power_setup(hu, true);
> > 
> > Unless there are drawbacks that I'm missing I think that's preferable
> > over the retry loop.
> > 
> > > > The code flow with the gotos and the error handling at the end of the
> > > > loop is a bit messy. Moving the power down to the top of the loop
> > > > (basically in line with my comment above to get rid of the loop) would
> > > > help here. In this case checking 'ret' in the loop condition (which I
> > > > suggested to remove) would make sense, since it elimninates the need
> > > > for the break/return in the success case. But if we can do without the
> > > > loop even better :)
> > > >
> > > 
> > > [Bala]: there is a reason to add the loop here, here we go with
> > > reason to
> > > add.
> > >         let us assume that qca_setup fails to establish a
> > > communication with
> > > wcn3990
> > >         then next steps will not be pass and we can't populate hci0
> > > rfkill
> > > entry.
> > >         in traditional bluez stack i.e. bluetoothd daemon will looks
> > > for
> > > hci0, if we have entry for hci0
> > >         then only BT option is visible in UI or else BT option will
> > > not be
> > > available in UI.
> > >         we don't have any mechanism handled in bluez user space to
> > > reinitiate the communication at latest to try for second time to
> > > make hci0
> > > up.
> > >         so that is reason behind to add so that we can handle fault
> > > handling
> > > of wcn3990 and establish the communication to make BT option
> > > available in
> > > BT.
> > 
> > The loop is supposed to address the following case (quoting you from
> > earlier discussion in the thread):
> > 
> > > found one interesting bug that BT setups fails with version request
> > > timeouts, after we do a reboot for the device. we debugged the issue
> > > and found that wcn3900 is not responding to the version request
> > > commands sent by HOST. this is because before reboot, wcn3990 is in
> > > on state i.e. we are communicating to device. then we did a reboot
> > > and HOST is not sending a power off request to the regulators to
> > > turn off. so after reboot wcn3990 is still in ON state where it will
> > > not respond to version request commands which in turn fails
> > > HCI_SETUP. so we are sending the power off pulse and then sending the
> > > power on pulse.
> > 
> > My suggestion to address this failure case is to reset/power off the
> > chip before initializing it. With that the loop shouldn't be needed,
> > actually it wasn't there before you found this specific error.
> 
> [Bala]: I will update loop according to your suggestions, But i am little
> bit worried, if the HCI_SETUP fails due to some
>         external issue of UART, so in that cases we can't able to handle
> with out loop and as i said, some operating system,
>        will not handle HCI SETUP failures, we at the driver need to handle
> such cases too.

As I said above, in most cases the problem should be addressed in the
driver that causes it. Not hiding issues may allow to fix them
properly, which also benefits other users of the UART/component.

Cheers

Matthias

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

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-07-31 21:33, Matthias Kaehlcke wrote:
> On Tue, Jul 31, 2018 at 08:08:40PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-07-31 01:37, Matthias Kaehlcke wrote:
>> > On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
>> > > Hi Matthias,
>> > >
>> > > On 2018-07-27 01:21, Matthias Kaehlcke wrote:
>> > > > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
>> > > > > Hi Matthias,
>> > > > >
>> > > > > On 2018-07-26 00:01, Matthias Kaehlcke wrote:
>> > > > > > On Tue, Jul 24, 2018 at 09:25:16PM +0530, Balakrishna Godavarthi wrote:
>> > > > > > > Hi Matthias,
>> > > > > > >
>> > > > > > > On 2018-07-24 01:24, Matthias Kaehlcke wrote:
>> > > > > > > > On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
>> > > > > > > > > +	 * sometimes we will face communication synchronization issues,
>> > > > > > > > > +	 * like reading version command timeouts. In which HCI_SETUP fails,
>> > > > > > > > > +	 * to overcome these issues, we try to communicate by performing an
>> > > > > > > > > +	 * COLD power OFF and ON.
>> > > > > > > > > +	 */
>> > > > > > > > > +	for (i = 1; i <= 10 && ret; i++) {
>> > > > > > > >
>> > > > > > > > Is it really that bad that more than say 3 iterations might be needed?
>> > > > > > > >
>> > > > > > > [Bala]: will restrict to 3 iterations.
>> > > > > >
>> > > > > > Is 3x expected to be enough to 'guarantee' as successful
>> > > > > > initialization? Just wondered about the 10x since it suddendly changed
>> > > > > > from 1x. What is the failure rate without retries?
>> > > > > >
>> > > > > > Could you provide more information about the 'communication
>> > > > > > synchronization issues'? Is the root cause understood? Maybe there is
>> > > > > > a better way than retries.
>> > > > > >
>> > > > >
>> > > > > [Bala]: basically before sending a every patch series we run a
>> > > > > stress test
>> > > > > to the driver to detect the bugs.
>> > > > >         in recent test results found one interesting bug that BT
>> > > > > setups
>> > > > > fails with version request timeouts,
>> > > > >         after we do a reboot for the device.
>> > > > >         we debugged the issue and found that wcn3900 is not
>> > > > > responding to
>> > > > > the version request commands
>> > > > >         sent by HOST. this is because before reboot, wcn3990 is in
>> > > > > on state
>> > > > > i.e. we are communicating to device.
>> > > > >         then we did a reboot and HOST is not sending a power off
>> > > > > request to
>> > > > > the regulators to turn off.
>> > > > >         so after reboot wcn3990 is still in ON state where it will not
>> > > > > respond to version request commands which in turn fails HCI_SETUP.
>> > > > >         so we are sending the power off pulse and then sending the
>> > > > > power on
>> > > > > pulse.
>> > > > >         coming back to 3x or 10x iteration this is to avoid any such
>> > > > > synchronization issues.
>> > > > >         i agreed for 3x because of stress test results. we have
>> > > > > success rate
>> > > > > of 99% for single iteration, where as 3x iterations will helps to
>> > > > > handle 1%
>> > > > > fails cases.
>> > > >
>> > > > Thanks for the clarification. Couldn't you assure the device is in a
>> > > > defined state by calling qca_power_shutdown() as one of the first
>> > > > things in qca_wcn3990_init()?
>> > >
>> > > [Bala]:  we have reasons behind writing qca_power_setup(true) at the
>> > > start.
>> > >
>> > >         1. the reason to add iteration here, is to handle BT fails
>> > > cases
>> > > either due to communication failure of wcn3900 or due to regulator
>> > > issues.
>> > >            before calling qca_setup(), we have our regulator turned
>> > > on and
>> > > in qca_setup i.e. init routine if we added power_shutdown as first
>> > > statement
>> > > before
>> > >            communicating with chip then regulator will be off and
>> > > again we
>> > > need to call function to ON regulators.
>> > >            so it could be some thing like this
>> > >
>> > >            init(){
>> > >
>> > >                for () {
>> > >                      shutdown() // regs are off
>> > >                      poweron(true) // regs are on.
>> > >                      if(!start communicating with chip()) {
>> > >                         break;
>> > >                       }
>> > >
>> > >                }
>> > >            }
>> > >            as the reason to add the iteration handling is to
>> > > overcome 1% of
>> > > fail cases, so every time when we call it will turn off the regs and
>> > > turn it
>> > > back. which require an turning in off regs and on it back for 99% pass
>> > >             cases.
>> >
>> > But would turning off the regs really add a significant delay here?
>> > The setup is already really slow, with a 100ms delay in the
>> > loop (still wonder if booting the chip without loading firmware really
>> > takes that long) and later the firmware is loaded.
>> >
>> [Bala]: By default we will have an firmware loaded in ROM of wcn3990, 
>> 100 ms
>> delay will help wcn3990 boot up with default firmware.
> 
> Ok, thanks.
> 
>>         Once it is booted up with default firmware on ROM, we will 
>> download
>> the firmware from the firmware files, these firmware files
>>         contains bug fixes of wcn3990. Once the firmware files are 
>> loaded we
>> will send the reset command to wcn3990.
>>         wcn3990 will start working with latest firmware which is 
>> loaded.
>> 
>> > If the chip needs to be in a defined state we should make sure to put
>> > in into that state, unless there is a significant overhead wrt 'try
>> > first and only reset in case of failure'. As a nice side effect the
>> > code would be cleaner and we probably could get rid of the loop
>> > completely, since it's supposed to address the case where the chip
>> > wasn't properly reset on a reboot.
>> >
>> 
>> [Bala]: i too agree with you, Now we have observed issue because of 
>> reboot.
>> But let us take a real time example here.
>>         if clocks of UART are not stable or there is an issue of UART 
>> GPIO's
>> or some thing related might have broken in UART. Then will have
>> communication issues with BT chip.
>>         where HCI_SETUP fails, instead of giving a fails status, we 
>> are
>> trying to communicate once again and these is also be in 1% of fail 
>> cases.
> 
> If there are issues with unstable clocks, GPIOs or similar the problem
> should be fixed at it's root, instead of papering over it in the BT
> driver. Kernel drivers would be a complete mess if they tried to
> recover from all possible problems in other subsystems.
> 
> I don't have general objections agains a retry loop when it is really
> needed, but I don't have the impressions this is the case here. If
> your stress tests reveal problems that can't be addresses otherwise
> please let us know and we might end up with the retry loop after all.
> 
[Bala]: current results point out failures cases only while reboot.
         if we have any issue that causes the problem of BT failure then 
we will add
         the loop. For now we will go with out retry loop :)

>> > >         2. this is the one of the main reason for adding
>> > > qca_power_setup(true) in the init() function first.
>> > >            as we know that power management is so critical for long
>> > > lasting
>> > > of battery.
>> > >            now present implementation is when we off BT from UI i.e.
>> > > hci0
>> > > down, we put BT into an suspend or low power mode, as soon as we
>> > > turn ON the
>> > > BT back from UI we make hci0 up.
>> > >            the above is putting device into suspend state and bring
>> > > it back
>> > > where the regulator are still on state. so we will have leakage
>> > > currents
>> > > which can be minimal or may be in few mA.
>> > >            to over come the above case, we want to do an cold on/off
>> > > for BT
>> > > chip wcn3990. i.e. when bt is off from UI, we will off the
>> > > regulators and
>> > > turn on it again once the BT is ON from UI.
>> > >            every time we disable i.e. off BT from UI we will call
>> > > hdev->shutdown() i.e. completely powering off the chip.
>> > >            so it require an reprogram again, when we turn ON BT from
>> > > UI. it
>> > > will call qca_setup()--> init().. so here actually
>> > > qca_power_on(true) will
>> > > turn on the chip and dump the fw files into it.
>> > >            so that is also a reason behind to write power on first.
>> > >
>> > >            the above feature is under testing state, will post a patch
>> > > series once the driver code merged to bt-next.
>> >
>> > Thanks for the info.
>> >
>> > If I understand correctly what you describe isn't incompatible with
>> > performing a proper reset. 'vregs_on' can be checked to avoid
>> > disabling already disabled regulators:
>> >
>> >   if (qcadev->bt_power->vregs_on)
>> >           qca_power_shutdown(hdev);
>> >
>> >   // short delay needed here?
>> >
>> >   qca_power_setup(hu, true);
>> >
>> > Unless there are drawbacks that I'm missing I think that's preferable
>> > over the retry loop.
>> >
>> > > > The code flow with the gotos and the error handling at the end of the
>> > > > loop is a bit messy. Moving the power down to the top of the loop
>> > > > (basically in line with my comment above to get rid of the loop) would
>> > > > help here. In this case checking 'ret' in the loop condition (which I
>> > > > suggested to remove) would make sense, since it elimninates the need
>> > > > for the break/return in the success case. But if we can do without the
>> > > > loop even better :)
>> > > >
>> > >
>> > > [Bala]: there is a reason to add the loop here, here we go with
>> > > reason to
>> > > add.
>> > >         let us assume that qca_setup fails to establish a
>> > > communication with
>> > > wcn3990
>> > >         then next steps will not be pass and we can't populate hci0
>> > > rfkill
>> > > entry.
>> > >         in traditional bluez stack i.e. bluetoothd daemon will looks
>> > > for
>> > > hci0, if we have entry for hci0
>> > >         then only BT option is visible in UI or else BT option will
>> > > not be
>> > > available in UI.
>> > >         we don't have any mechanism handled in bluez user space to
>> > > reinitiate the communication at latest to try for second time to
>> > > make hci0
>> > > up.
>> > >         so that is reason behind to add so that we can handle fault
>> > > handling
>> > > of wcn3990 and establish the communication to make BT option
>> > > available in
>> > > BT.
>> >
>> > The loop is supposed to address the following case (quoting you from
>> > earlier discussion in the thread):
>> >
>> > > found one interesting bug that BT setups fails with version request
>> > > timeouts, after we do a reboot for the device. we debugged the issue
>> > > and found that wcn3900 is not responding to the version request
>> > > commands sent by HOST. this is because before reboot, wcn3990 is in
>> > > on state i.e. we are communicating to device. then we did a reboot
>> > > and HOST is not sending a power off request to the regulators to
>> > > turn off. so after reboot wcn3990 is still in ON state where it will
>> > > not respond to version request commands which in turn fails
>> > > HCI_SETUP. so we are sending the power off pulse and then sending the
>> > > power on pulse.
>> >
>> > My suggestion to address this failure case is to reset/power off the
>> > chip before initializing it. With that the loop shouldn't be needed,
>> > actually it wasn't there before you found this specific error.
>> 
>> [Bala]: I will update loop according to your suggestions, But i am 
>> little
>> bit worried, if the HCI_SETUP fails due to some
>>         external issue of UART, so in that cases we can't able to 
>> handle
>> with out loop and as i said, some operating system,
>>        will not handle HCI SETUP failures, we at the driver need to 
>> handle
>> such cases too.
> 
> As I said above, in most cases the problem should be addressed in the
> driver that causes it. Not hiding issues may allow to fix them
> properly, which also benefits other users of the UART/component.
> 
> Cheers
> 
> Matthias

[Bala]: below is how qca_wcn3990_init() after update.

qca_wcn3990_init()
{
        /* Forcefully enable wcn3990 to enter in to boot mode. */
         host_set_baudrate(hu, 2400);
         ret = qca_send_power_pulse(hdev, QCA_WCN3990_POWEROFF_PULSE);
         if (ret)
                 return ret;

         qca_set_speed(hu, QCA_INIT_SPEED);
         ret = qca_send_power_pulse(hdev, QCA_WCN3990_POWERON_PULSE);
         if (ret)
                 return ret;

         /* Wait for 100 ms for SoC to boot */
         msleep(100);

         /* Now the device is in ready state to communicate with host.
          * To sync host with device we need to reopen port.
          * Without this, we will have RTS and CTS synchronization
          * issues.
          */
         serdev_device_close(hu->serdev);
         ret = serdev_device_open(hu->serdev);
         if (ret) {
                 bt_dev_err(hu->hdev, "failed to open port");
                 return ret;
         }

         hci_uart_set_flow_control(hu, false);
         ret = qca_read_soc_version(hdev, soc_ver);

         return ret;

}

-- 
Regards
Balakrishna.

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

[Matthias Kaehlcke]

On Wed, Aug 01, 2018 at 07:29:29PM +0530, Balakrishna Godavarthi wrote:
> Hi Matthias,
> 
> On 2018-07-31 21:33, Matthias Kaehlcke wrote:
> > On Tue, Jul 31, 2018 at 08:08:40PM +0530, Balakrishna Godavarthi wrote:
> > > Hi Matthias,
> > > 
> > > On 2018-07-31 01:37, Matthias Kaehlcke wrote:
> > > > On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
> > > > > Hi Matthias,
> > > > >
> > > > > On 2018-07-27 01:21, Matthias Kaehlcke wrote:
> > > > > > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
> > > > > > > Hi Matthias,
> > > > > > >
> > > > > > > On 2018-07-26 00:01, Matthias Kaehlcke wrote:
> > > > > > > > On Tue, Jul 24, 2018 at 09:25:16PM +0530, Balakrishna Godavarthi wrote:
> > > > > > > > > Hi Matthias,
> > > > > > > > >
> > > > > > > > > On 2018-07-24 01:24, Matthias Kaehlcke wrote:
> > > > > > > > > > On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
> > > > > > > > > > > +	 * sometimes we will face communication synchronization issues,
> > > > > > > > > > > +	 * like reading version command timeouts. In which HCI_SETUP fails,
> > > > > > > > > > > +	 * to overcome these issues, we try to communicate by performing an
> > > > > > > > > > > +	 * COLD power OFF and ON.
> > > > > > > > > > > +	 */
> > > > > > > > > > > +	for (i = 1; i <= 10 && ret; i++) {
> > > > > > > > > >
> > > > > > > > > > Is it really that bad that more than say 3 iterations might be needed?
> > > > > > > > > >
> > > > > > > > > [Bala]: will restrict to 3 iterations.
> > > > > > > >
> > > > > > > > Is 3x expected to be enough to 'guarantee' as successful
> > > > > > > > initialization? Just wondered about the 10x since it suddendly changed
> > > > > > > > from 1x. What is the failure rate without retries?
> > > > > > > >
> > > > > > > > Could you provide more information about the 'communication
> > > > > > > > synchronization issues'? Is the root cause understood? Maybe there is
> > > > > > > > a better way than retries.
> > > > > > > >
> > > > > > >
> > > > > > > [Bala]: basically before sending a every patch series we run a
> > > > > > > stress test
> > > > > > > to the driver to detect the bugs.
> > > > > > >         in recent test results found one interesting bug that BT
> > > > > > > setups
> > > > > > > fails with version request timeouts,
> > > > > > >         after we do a reboot for the device.
> > > > > > >         we debugged the issue and found that wcn3900 is not
> > > > > > > responding to
> > > > > > > the version request commands
> > > > > > >         sent by HOST. this is because before reboot, wcn3990 is in
> > > > > > > on state
> > > > > > > i.e. we are communicating to device.
> > > > > > >         then we did a reboot and HOST is not sending a power off
> > > > > > > request to
> > > > > > > the regulators to turn off.
> > > > > > >         so after reboot wcn3990 is still in ON state where it will not
> > > > > > > respond to version request commands which in turn fails HCI_SETUP.
> > > > > > >         so we are sending the power off pulse and then sending the
> > > > > > > power on
> > > > > > > pulse.
> > > > > > >         coming back to 3x or 10x iteration this is to avoid any such
> > > > > > > synchronization issues.
> > > > > > >         i agreed for 3x because of stress test results. we have
> > > > > > > success rate
> > > > > > > of 99% for single iteration, where as 3x iterations will helps to
> > > > > > > handle 1%
> > > > > > > fails cases.
> > > > > >
> > > > > > Thanks for the clarification. Couldn't you assure the device is in a
> > > > > > defined state by calling qca_power_shutdown() as one of the first
> > > > > > things in qca_wcn3990_init()?
> > > > >
> > > > > [Bala]:  we have reasons behind writing qca_power_setup(true) at the
> > > > > start.
> > > > >
> > > > >         1. the reason to add iteration here, is to handle BT fails
> > > > > cases
> > > > > either due to communication failure of wcn3900 or due to regulator
> > > > > issues.
> > > > >            before calling qca_setup(), we have our regulator turned
> > > > > on and
> > > > > in qca_setup i.e. init routine if we added power_shutdown as first
> > > > > statement
> > > > > before
> > > > >            communicating with chip then regulator will be off and
> > > > > again we
> > > > > need to call function to ON regulators.
> > > > >            so it could be some thing like this
> > > > >
> > > > >            init(){
> > > > >
> > > > >                for () {
> > > > >                      shutdown() // regs are off
> > > > >                      poweron(true) // regs are on.
> > > > >                      if(!start communicating with chip()) {
> > > > >                         break;
> > > > >                       }
> > > > >
> > > > >                }
> > > > >            }
> > > > >            as the reason to add the iteration handling is to
> > > > > overcome 1% of
> > > > > fail cases, so every time when we call it will turn off the regs and
> > > > > turn it
> > > > > back. which require an turning in off regs and on it back for 99% pass
> > > > >             cases.
> > > >
> > > > But would turning off the regs really add a significant delay here?
> > > > The setup is already really slow, with a 100ms delay in the
> > > > loop (still wonder if booting the chip without loading firmware really
> > > > takes that long) and later the firmware is loaded.
> > > >
> > > [Bala]: By default we will have an firmware loaded in ROM of
> > > wcn3990, 100 ms
> > > delay will help wcn3990 boot up with default firmware.
> > 
> > Ok, thanks.
> > 
> > >         Once it is booted up with default firmware on ROM, we will
> > > download
> > > the firmware from the firmware files, these firmware files
> > >         contains bug fixes of wcn3990. Once the firmware files are
> > > loaded we
> > > will send the reset command to wcn3990.
> > >         wcn3990 will start working with latest firmware which is
> > > loaded.
> > > 
> > > > If the chip needs to be in a defined state we should make sure to put
> > > > in into that state, unless there is a significant overhead wrt 'try
> > > > first and only reset in case of failure'. As a nice side effect the
> > > > code would be cleaner and we probably could get rid of the loop
> > > > completely, since it's supposed to address the case where the chip
> > > > wasn't properly reset on a reboot.
> > > >
> > > 
> > > [Bala]: i too agree with you, Now we have observed issue because of
> > > reboot.
> > > But let us take a real time example here.
> > >         if clocks of UART are not stable or there is an issue of
> > > UART GPIO's
> > > or some thing related might have broken in UART. Then will have
> > > communication issues with BT chip.
> > >         where HCI_SETUP fails, instead of giving a fails status, we
> > > are
> > > trying to communicate once again and these is also be in 1% of fail
> > > cases.
> > 
> > If there are issues with unstable clocks, GPIOs or similar the problem
> > should be fixed at it's root, instead of papering over it in the BT
> > driver. Kernel drivers would be a complete mess if they tried to
> > recover from all possible problems in other subsystems.
> > 
> > I don't have general objections agains a retry loop when it is really
> > needed, but I don't have the impressions this is the case here. If
> > your stress tests reveal problems that can't be addresses otherwise
> > please let us know and we might end up with the retry loop after all.
> > 
> [Bala]: current results point out failures cases only while reboot.
>         if we have any issue that causes the problem of BT failure then we
> will add
>         the loop. For now we will go with out retry loop :)
> 
> > > > >         2. this is the one of the main reason for adding
> > > > > qca_power_setup(true) in the init() function first.
> > > > >            as we know that power management is so critical for long
> > > > > lasting
> > > > > of battery.
> > > > >            now present implementation is when we off BT from UI i.e.
> > > > > hci0
> > > > > down, we put BT into an suspend or low power mode, as soon as we
> > > > > turn ON the
> > > > > BT back from UI we make hci0 up.
> > > > >            the above is putting device into suspend state and bring
> > > > > it back
> > > > > where the regulator are still on state. so we will have leakage
> > > > > currents
> > > > > which can be minimal or may be in few mA.
> > > > >            to over come the above case, we want to do an cold on/off
> > > > > for BT
> > > > > chip wcn3990. i.e. when bt is off from UI, we will off the
> > > > > regulators and
> > > > > turn on it again once the BT is ON from UI.
> > > > >            every time we disable i.e. off BT from UI we will call
> > > > > hdev->shutdown() i.e. completely powering off the chip.
> > > > >            so it require an reprogram again, when we turn ON BT from
> > > > > UI. it
> > > > > will call qca_setup()--> init().. so here actually
> > > > > qca_power_on(true) will
> > > > > turn on the chip and dump the fw files into it.
> > > > >            so that is also a reason behind to write power on first.
> > > > >
> > > > >            the above feature is under testing state, will post a patch
> > > > > series once the driver code merged to bt-next.
> > > >
> > > > Thanks for the info.
> > > >
> > > > If I understand correctly what you describe isn't incompatible with
> > > > performing a proper reset. 'vregs_on' can be checked to avoid
> > > > disabling already disabled regulators:
> > > >
> > > >   if (qcadev->bt_power->vregs_on)
> > > >           qca_power_shutdown(hdev);
> > > >
> > > >   // short delay needed here?
> > > >
> > > >   qca_power_setup(hu, true);
> > > >
> > > > Unless there are drawbacks that I'm missing I think that's preferable
> > > > over the retry loop.
> > > >
> > > > > > The code flow with the gotos and the error handling at the end of the
> > > > > > loop is a bit messy. Moving the power down to the top of the loop
> > > > > > (basically in line with my comment above to get rid of the loop) would
> > > > > > help here. In this case checking 'ret' in the loop condition (which I
> > > > > > suggested to remove) would make sense, since it elimninates the need
> > > > > > for the break/return in the success case. But if we can do without the
> > > > > > loop even better :)
> > > > > >
> > > > >
> > > > > [Bala]: there is a reason to add the loop here, here we go with
> > > > > reason to
> > > > > add.
> > > > >         let us assume that qca_setup fails to establish a
> > > > > communication with
> > > > > wcn3990
> > > > >         then next steps will not be pass and we can't populate hci0
> > > > > rfkill
> > > > > entry.
> > > > >         in traditional bluez stack i.e. bluetoothd daemon will looks
> > > > > for
> > > > > hci0, if we have entry for hci0
> > > > >         then only BT option is visible in UI or else BT option will
> > > > > not be
> > > > > available in UI.
> > > > >         we don't have any mechanism handled in bluez user space to
> > > > > reinitiate the communication at latest to try for second time to
> > > > > make hci0
> > > > > up.
> > > > >         so that is reason behind to add so that we can handle fault
> > > > > handling
> > > > > of wcn3990 and establish the communication to make BT option
> > > > > available in
> > > > > BT.
> > > >
> > > > The loop is supposed to address the following case (quoting you from
> > > > earlier discussion in the thread):
> > > >
> > > > > found one interesting bug that BT setups fails with version request
> > > > > timeouts, after we do a reboot for the device. we debugged the issue
> > > > > and found that wcn3900 is not responding to the version request
> > > > > commands sent by HOST. this is because before reboot, wcn3990 is in
> > > > > on state i.e. we are communicating to device. then we did a reboot
> > > > > and HOST is not sending a power off request to the regulators to
> > > > > turn off. so after reboot wcn3990 is still in ON state where it will
> > > > > not respond to version request commands which in turn fails
> > > > > HCI_SETUP. so we are sending the power off pulse and then sending the
> > > > > power on pulse.
> > > >
> > > > My suggestion to address this failure case is to reset/power off the
> > > > chip before initializing it. With that the loop shouldn't be needed,
> > > > actually it wasn't there before you found this specific error.
> > > 
> > > [Bala]: I will update loop according to your suggestions, But i am
> > > little
> > > bit worried, if the HCI_SETUP fails due to some
> > >         external issue of UART, so in that cases we can't able to
> > > handle
> > > with out loop and as i said, some operating system,
> > >        will not handle HCI SETUP failures, we at the driver need to
> > > handle
> > > such cases too.
> > 
> > As I said above, in most cases the problem should be addressed in the
> > driver that causes it. Not hiding issues may allow to fix them
> > properly, which also benefits other users of the UART/component.
> > 
> > Cheers
> > 
> > Matthias
> 
> [Bala]: below is how qca_wcn3990_init() after update.
> 
> qca_wcn3990_init()
> {
>        /* Forcefully enable wcn3990 to enter in to boot mode. */
>         host_set_baudrate(hu, 2400);
>         ret = qca_send_power_pulse(hdev, QCA_WCN3990_POWEROFF_PULSE);
>         if (ret)
>                 return ret;
> 
>         qca_set_speed(hu, QCA_INIT_SPEED);
>         ret = qca_send_power_pulse(hdev, QCA_WCN3990_POWERON_PULSE);
>         if (ret)
>                 return ret;
> 
>         /* Wait for 100 ms for SoC to boot */
>         msleep(100);
> 
>         /* Now the device is in ready state to communicate with host.
>          * To sync host with device we need to reopen port.
>          * Without this, we will have RTS and CTS synchronization
>          * issues.
>          */
>         serdev_device_close(hu->serdev);
>         ret = serdev_device_open(hu->serdev);
>         if (ret) {
>                 bt_dev_err(hu->hdev, "failed to open port");
>                 return ret;
>         }
> 
>         hci_uart_set_flow_control(hu, false);
>         ret = qca_read_soc_version(hdev, soc_ver);
> 
>         return ret;
> 
> }

Thanks, looks pretty good!

No need to toggle the regulators without the loop? I had doubts about
that part anyway since the regulators wouldn't be necessarily switched
off if they are shared with other devices. Or do the hardware design
guidelines of the wcn3990 require that some of the voltage rails are used
exclusively by the chip?

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

[Balakrishna Godavarthi]

Hi Matthias,

On 2018-08-01 21:46, Matthias Kaehlcke wrote:
> On Wed, Aug 01, 2018 at 07:29:29PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-07-31 21:33, Matthias Kaehlcke wrote:
>> > On Tue, Jul 31, 2018 at 08:08:40PM +0530, Balakrishna Godavarthi wrote:
>> > > Hi Matthias,
>> > >
>> > > On 2018-07-31 01:37, Matthias Kaehlcke wrote:
>> > > > On Fri, Jul 27, 2018 at 05:09:02PM +0530, Balakrishna Godavarthi wrote:
>> > > > > Hi Matthias,
>> > > > >
>> > > > > On 2018-07-27 01:21, Matthias Kaehlcke wrote:
>> > > > > > On Thu, Jul 26, 2018 at 07:51:13PM +0530, Balakrishna Godavarthi wrote:
>> > > > > > > Hi Matthias,
>> > > > > > >
>> > > > > > > On 2018-07-26 00:01, Matthias Kaehlcke wrote:
>> > > > > > > > On Tue, Jul 24, 2018 at 09:25:16PM +0530, Balakrishna Godavarthi wrote:
>> > > > > > > > > Hi Matthias,
>> > > > > > > > >
>> > > > > > > > > On 2018-07-24 01:24, Matthias Kaehlcke wrote:
>> > > > > > > > > > On Fri, Jul 20, 2018 at 07:02:43PM +0530, Balakrishna Godavarthi wrote:
>> > > > > > > > > > > +	 * sometimes we will face communication synchronization issues,
>> > > > > > > > > > > +	 * like reading version command timeouts. In which HCI_SETUP fails,
>> > > > > > > > > > > +	 * to overcome these issues, we try to communicate by performing an
>> > > > > > > > > > > +	 * COLD power OFF and ON.
>> > > > > > > > > > > +	 */
>> > > > > > > > > > > +	for (i = 1; i <= 10 && ret; i++) {
>> > > > > > > > > >
>> > > > > > > > > > Is it really that bad that more than say 3 iterations might be needed?
>> > > > > > > > > >
>> > > > > > > > > [Bala]: will restrict to 3 iterations.
>> > > > > > > >
>> > > > > > > > Is 3x expected to be enough to 'guarantee' as successful
>> > > > > > > > initialization? Just wondered about the 10x since it suddendly changed
>> > > > > > > > from 1x. What is the failure rate without retries?
>> > > > > > > >
>> > > > > > > > Could you provide more information about the 'communication
>> > > > > > > > synchronization issues'? Is the root cause understood? Maybe there is
>> > > > > > > > a better way than retries.
>> > > > > > > >
>> > > > > > >
>> > > > > > > [Bala]: basically before sending a every patch series we run a
>> > > > > > > stress test
>> > > > > > > to the driver to detect the bugs.
>> > > > > > >         in recent test results found one interesting bug that BT
>> > > > > > > setups
>> > > > > > > fails with version request timeouts,
>> > > > > > >         after we do a reboot for the device.
>> > > > > > >         we debugged the issue and found that wcn3900 is not
>> > > > > > > responding to
>> > > > > > > the version request commands
>> > > > > > >         sent by HOST. this is because before reboot, wcn3990 is in
>> > > > > > > on state
>> > > > > > > i.e. we are communicating to device.
>> > > > > > >         then we did a reboot and HOST is not sending a power off
>> > > > > > > request to
>> > > > > > > the regulators to turn off.
>> > > > > > >         so after reboot wcn3990 is still in ON state where it will not
>> > > > > > > respond to version request commands which in turn fails HCI_SETUP.
>> > > > > > >         so we are sending the power off pulse and then sending the
>> > > > > > > power on
>> > > > > > > pulse.
>> > > > > > >         coming back to 3x or 10x iteration this is to avoid any such
>> > > > > > > synchronization issues.
>> > > > > > >         i agreed for 3x because of stress test results. we have
>> > > > > > > success rate
>> > > > > > > of 99% for single iteration, where as 3x iterations will helps to
>> > > > > > > handle 1%
>> > > > > > > fails cases.
>> > > > > >
>> > > > > > Thanks for the clarification. Couldn't you assure the device is in a
>> > > > > > defined state by calling qca_power_shutdown() as one of the first
>> > > > > > things in qca_wcn3990_init()?
>> > > > >
>> > > > > [Bala]:  we have reasons behind writing qca_power_setup(true) at the
>> > > > > start.
>> > > > >
>> > > > >         1. the reason to add iteration here, is to handle BT fails
>> > > > > cases
>> > > > > either due to communication failure of wcn3900 or due to regulator
>> > > > > issues.
>> > > > >            before calling qca_setup(), we have our regulator turned
>> > > > > on and
>> > > > > in qca_setup i.e. init routine if we added power_shutdown as first
>> > > > > statement
>> > > > > before
>> > > > >            communicating with chip then regulator will be off and
>> > > > > again we
>> > > > > need to call function to ON regulators.
>> > > > >            so it could be some thing like this
>> > > > >
>> > > > >            init(){
>> > > > >
>> > > > >                for () {
>> > > > >                      shutdown() // regs are off
>> > > > >                      poweron(true) // regs are on.
>> > > > >                      if(!start communicating with chip()) {
>> > > > >                         break;
>> > > > >                       }
>> > > > >
>> > > > >                }
>> > > > >            }
>> > > > >            as the reason to add the iteration handling is to
>> > > > > overcome 1% of
>> > > > > fail cases, so every time when we call it will turn off the regs and
>> > > > > turn it
>> > > > > back. which require an turning in off regs and on it back for 99% pass
>> > > > >             cases.
>> > > >
>> > > > But would turning off the regs really add a significant delay here?
>> > > > The setup is already really slow, with a 100ms delay in the
>> > > > loop (still wonder if booting the chip without loading firmware really
>> > > > takes that long) and later the firmware is loaded.
>> > > >
>> > > [Bala]: By default we will have an firmware loaded in ROM of
>> > > wcn3990, 100 ms
>> > > delay will help wcn3990 boot up with default firmware.
>> >
>> > Ok, thanks.
>> >
>> > >         Once it is booted up with default firmware on ROM, we will
>> > > download
>> > > the firmware from the firmware files, these firmware files
>> > >         contains bug fixes of wcn3990. Once the firmware files are
>> > > loaded we
>> > > will send the reset command to wcn3990.
>> > >         wcn3990 will start working with latest firmware which is
>> > > loaded.
>> > >
>> > > > If the chip needs to be in a defined state we should make sure to put
>> > > > in into that state, unless there is a significant overhead wrt 'try
>> > > > first and only reset in case of failure'. As a nice side effect the
>> > > > code would be cleaner and we probably could get rid of the loop
>> > > > completely, since it's supposed to address the case where the chip
>> > > > wasn't properly reset on a reboot.
>> > > >
>> > >
>> > > [Bala]: i too agree with you, Now we have observed issue because of
>> > > reboot.
>> > > But let us take a real time example here.
>> > >         if clocks of UART are not stable or there is an issue of
>> > > UART GPIO's
>> > > or some thing related might have broken in UART. Then will have
>> > > communication issues with BT chip.
>> > >         where HCI_SETUP fails, instead of giving a fails status, we
>> > > are
>> > > trying to communicate once again and these is also be in 1% of fail
>> > > cases.
>> >
>> > If there are issues with unstable clocks, GPIOs or similar the problem
>> > should be fixed at it's root, instead of papering over it in the BT
>> > driver. Kernel drivers would be a complete mess if they tried to
>> > recover from all possible problems in other subsystems.
>> >
>> > I don't have general objections agains a retry loop when it is really
>> > needed, but I don't have the impressions this is the case here. If
>> > your stress tests reveal problems that can't be addresses otherwise
>> > please let us know and we might end up with the retry loop after all.
>> >
>> [Bala]: current results point out failures cases only while reboot.
>>         if we have any issue that causes the problem of BT failure 
>> then we
>> will add
>>         the loop. For now we will go with out retry loop :)
>> 
>> > > > >         2. this is the one of the main reason for adding
>> > > > > qca_power_setup(true) in the init() function first.
>> > > > >            as we know that power management is so critical for long
>> > > > > lasting
>> > > > > of battery.
>> > > > >            now present implementation is when we off BT from UI i.e.
>> > > > > hci0
>> > > > > down, we put BT into an suspend or low power mode, as soon as we
>> > > > > turn ON the
>> > > > > BT back from UI we make hci0 up.
>> > > > >            the above is putting device into suspend state and bring
>> > > > > it back
>> > > > > where the regulator are still on state. so we will have leakage
>> > > > > currents
>> > > > > which can be minimal or may be in few mA.
>> > > > >            to over come the above case, we want to do an cold on/off
>> > > > > for BT
>> > > > > chip wcn3990. i.e. when bt is off from UI, we will off the
>> > > > > regulators and
>> > > > > turn on it again once the BT is ON from UI.
>> > > > >            every time we disable i.e. off BT from UI we will call
>> > > > > hdev->shutdown() i.e. completely powering off the chip.
>> > > > >            so it require an reprogram again, when we turn ON BT from
>> > > > > UI. it
>> > > > > will call qca_setup()--> init().. so here actually
>> > > > > qca_power_on(true) will
>> > > > > turn on the chip and dump the fw files into it.
>> > > > >            so that is also a reason behind to write power on first.
>> > > > >
>> > > > >            the above feature is under testing state, will post a patch
>> > > > > series once the driver code merged to bt-next.
>> > > >
>> > > > Thanks for the info.
>> > > >
>> > > > If I understand correctly what you describe isn't incompatible with
>> > > > performing a proper reset. 'vregs_on' can be checked to avoid
>> > > > disabling already disabled regulators:
>> > > >
>> > > >   if (qcadev->bt_power->vregs_on)
>> > > >           qca_power_shutdown(hdev);
>> > > >
>> > > >   // short delay needed here?
>> > > >
>> > > >   qca_power_setup(hu, true);
>> > > >
>> > > > Unless there are drawbacks that I'm missing I think that's preferable
>> > > > over the retry loop.
>> > > >
>> > > > > > The code flow with the gotos and the error handling at the end of the
>> > > > > > loop is a bit messy. Moving the power down to the top of the loop
>> > > > > > (basically in line with my comment above to get rid of the loop) would
>> > > > > > help here. In this case checking 'ret' in the loop condition (which I
>> > > > > > suggested to remove) would make sense, since it elimninates the need
>> > > > > > for the break/return in the success case. But if we can do without the
>> > > > > > loop even better :)
>> > > > > >
>> > > > >
>> > > > > [Bala]: there is a reason to add the loop here, here we go with
>> > > > > reason to
>> > > > > add.
>> > > > >         let us assume that qca_setup fails to establish a
>> > > > > communication with
>> > > > > wcn3990
>> > > > >         then next steps will not be pass and we can't populate hci0
>> > > > > rfkill
>> > > > > entry.
>> > > > >         in traditional bluez stack i.e. bluetoothd daemon will looks
>> > > > > for
>> > > > > hci0, if we have entry for hci0
>> > > > >         then only BT option is visible in UI or else BT option will
>> > > > > not be
>> > > > > available in UI.
>> > > > >         we don't have any mechanism handled in bluez user space to
>> > > > > reinitiate the communication at latest to try for second time to
>> > > > > make hci0
>> > > > > up.
>> > > > >         so that is reason behind to add so that we can handle fault
>> > > > > handling
>> > > > > of wcn3990 and establish the communication to make BT option
>> > > > > available in
>> > > > > BT.
>> > > >
>> > > > The loop is supposed to address the following case (quoting you from
>> > > > earlier discussion in the thread):
>> > > >
>> > > > > found one interesting bug that BT setups fails with version request
>> > > > > timeouts, after we do a reboot for the device. we debugged the issue
>> > > > > and found that wcn3900 is not responding to the version request
>> > > > > commands sent by HOST. this is because before reboot, wcn3990 is in
>> > > > > on state i.e. we are communicating to device. then we did a reboot
>> > > > > and HOST is not sending a power off request to the regulators to
>> > > > > turn off. so after reboot wcn3990 is still in ON state where it will
>> > > > > not respond to version request commands which in turn fails
>> > > > > HCI_SETUP. so we are sending the power off pulse and then sending the
>> > > > > power on pulse.
>> > > >
>> > > > My suggestion to address this failure case is to reset/power off the
>> > > > chip before initializing it. With that the loop shouldn't be needed,
>> > > > actually it wasn't there before you found this specific error.
>> > >
>> > > [Bala]: I will update loop according to your suggestions, But i am
>> > > little
>> > > bit worried, if the HCI_SETUP fails due to some
>> > >         external issue of UART, so in that cases we can't able to
>> > > handle
>> > > with out loop and as i said, some operating system,
>> > >        will not handle HCI SETUP failures, we at the driver need to
>> > > handle
>> > > such cases too.
>> >
>> > As I said above, in most cases the problem should be addressed in the
>> > driver that causes it. Not hiding issues may allow to fix them
>> > properly, which also benefits other users of the UART/component.
>> >
>> > Cheers
>> >
>> > Matthias
>> 
>> [Bala]: below is how qca_wcn3990_init() after update.
>> 
>> qca_wcn3990_init()
>> {
>>        /* Forcefully enable wcn3990 to enter in to boot mode. */
>>         host_set_baudrate(hu, 2400);
>>         ret = qca_send_power_pulse(hdev, QCA_WCN3990_POWEROFF_PULSE);
>>         if (ret)
>>                 return ret;
>> 
>>         qca_set_speed(hu, QCA_INIT_SPEED);
>>         ret = qca_send_power_pulse(hdev, QCA_WCN3990_POWERON_PULSE);
>>         if (ret)
>>                 return ret;
>> 
>>         /* Wait for 100 ms for SoC to boot */
>>         msleep(100);
>> 
>>         /* Now the device is in ready state to communicate with host.
>>          * To sync host with device we need to reopen port.
>>          * Without this, we will have RTS and CTS synchronization
>>          * issues.
>>          */
>>         serdev_device_close(hu->serdev);
>>         ret = serdev_device_open(hu->serdev);
>>         if (ret) {
>>                 bt_dev_err(hu->hdev, "failed to open port");
>>                 return ret;
>>         }
>> 
>>         hci_uart_set_flow_control(hu, false);
>>         ret = qca_read_soc_version(hdev, soc_ver);
>> 
>>         return ret;
>> 
>> }
> 
> Thanks, looks pretty good!
> 
> No need to toggle the regulators without the loop? I had doubts about
> that part anyway since the regulators wouldn't be necessarily switched
> off if they are shared with other devices. Or do the hardware design
> guidelines of the wcn3990 require that some of the voltage rails are 
> used
> exclusively by the chip?

As BT and wifi uses the same RF antenna on wcn3990, so we share 
regulator with wifi. So in above case toggling of regulators are not 
required.
To differentiate BT and wifi we have an different hardware circuit at 
wcn3990, that decodes the command received on Tx pin. i.e. power on and 
off pulses.

-- 
Regards
Balakrishna.