Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v1 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off()
@ 2019-12-25  6:03 Rocky Liao
  2019-12-25  6:03 ` [PATCH v1 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
                   ` (6 more replies)
  0 siblings, 7 replies; 34+ messages in thread
From: Rocky Liao @ 2019-12-25  6:03 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: linux-kernel, linux-bluetooth, linux-arm-msm, Rocky Liao

We will need to call the qca_power_off() API to power off Rome, add the
support into it. QCA Rome is using bt_en GPIO for power off, so we just
need to pull down this bt_en GPIO to power off it.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---
 drivers/bluetooth/hci_qca.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index b602ed01505b..43fd84028786 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1413,13 +1413,26 @@ static void qca_power_shutdown(struct hci_uart *hu)
 static int qca_power_off(struct hci_dev *hdev)
 {
 	struct hci_uart *hu = hci_get_drvdata(hdev);
+	struct qca_serdev *qcadev;
+	enum qca_btsoc_type soc_type = qca_soc_type(hu);
+
+	if (qca_is_wcn399x(soc_type)) {
+		/* Perform pre shutdown command */
+		qca_send_pre_shutdown_cmd(hdev);
+
+		usleep_range(8000, 10000);
 
-	/* Perform pre shutdown command */
-	qca_send_pre_shutdown_cmd(hdev);
+		qca_power_shutdown(hu);
+	} else {
+		if (hu->serdev) {
+			qcadev = serdev_device_get_drvdata(hu->serdev);
+
+			gpiod_set_value_cansleep(qcadev->bt_en, 0);
 
-	usleep_range(8000, 10000);
+			usleep_range(8000, 10000);
+		}
+	}
 
-	qca_power_shutdown(hu);
 	return 0;
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v1 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails
  2019-12-25  6:03 [PATCH v1 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
@ 2019-12-25  6:03 ` Rocky Liao
  2019-12-25  6:03 ` [PATCH v1 3/4] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 34+ messages in thread
From: Rocky Liao @ 2019-12-25  6:03 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: linux-kernel, linux-bluetooth, linux-arm-msm, Rocky Liao

This patch adds the retry of btsoc initialization when it fails. There are
reports that the btsoc initialization may fail on some platforms but the
repro ratio is very low. The failure may be caused by UART, platform HW or
the btsoc itself but it's very difficlut to root cause, given the repro
ratio is very low. Add a retry for the btsoc initialization will resolve
most of the failures and make Bluetooth finally works.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---
 drivers/bluetooth/hci_qca.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 43fd84028786..45042aa27fa4 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -53,6 +53,9 @@
 /* Controller debug log header */
 #define QCA_DEBUG_HANDLE	0x2EDC
 
+/* max retry count when init fails */
+#define QCA_MAX_INIT_RETRY_COUNT 3
+
 enum qca_flags {
 	QCA_IBS_ENABLED,
 	QCA_DROP_VENDOR_EVENT,
@@ -1257,7 +1260,9 @@ static int qca_setup(struct hci_uart *hu)
 {
 	struct hci_dev *hdev = hu->hdev;
 	struct qca_data *qca = hu->priv;
+	struct qca_serdev *qcadev;
 	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
+	unsigned int init_retry_count = 0;
 	enum qca_btsoc_type soc_type = qca_soc_type(hu);
 	const char *firmware_name = qca_get_firmware_name(hu);
 	int ret;
@@ -1275,6 +1280,7 @@ static int qca_setup(struct hci_uart *hu)
 	 */
 	set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 
+retry:
 	if (qca_is_wcn399x(soc_type)) {
 		bt_dev_info(hdev, "setting up wcn3990");
 
@@ -1293,6 +1299,12 @@ static int qca_setup(struct hci_uart *hu)
 			return ret;
 	} else {
 		bt_dev_info(hdev, "ROME setup");
+		if (hu->serdev) {
+			qcadev = serdev_device_get_drvdata(hu->serdev);
+			gpiod_set_value_cansleep(qcadev->bt_en, 1);
+			/* Controller needs time to bootup. */
+			msleep(150);
+		}
 		qca_set_speed(hu, QCA_INIT_SPEED);
 	}
 
@@ -1329,6 +1341,20 @@ static int qca_setup(struct hci_uart *hu)
 		 * patch/nvm-config is found, so run with original fw/config.
 		 */
 		ret = 0;
+	} else {
+		if (init_retry_count < QCA_MAX_INIT_RETRY_COUNT) {
+			qca_power_off(hdev);
+			if (hu->serdev) {
+				serdev_device_close(hu->serdev);
+				ret = serdev_device_open(hu->serdev);
+				if (ret) {
+					bt_dev_err(hu->hdev, "open port fail");
+					return ret;
+				}
+			}
+			init_retry_count++;
+			goto retry;
+		}
 	}
 
 	/* Setup bdaddr */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v1 3/4] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome
  2019-12-25  6:03 [PATCH v1 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
  2019-12-25  6:03 ` [PATCH v1 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
@ 2019-12-25  6:03 ` Rocky Liao
  2019-12-25  6:03 ` [PATCH v1 4/4] Bluetooth: hci_qca: Add HCI command timeout handling Rocky Liao
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 34+ messages in thread
From: Rocky Liao @ 2019-12-25  6:03 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: linux-kernel, linux-bluetooth, linux-arm-msm, Rocky Liao

This patch registers hdev->shutdown() callback and also sets
HCI_QUIRK_NON_PERSISTENT_SETUP for QCA Rome. It will power-off the BT chip
during hci down and power-on/initialize the chip again during hci up.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---
 drivers/bluetooth/hci_qca.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 45042aa27fa4..7e202041ed77 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1300,6 +1300,11 @@ static int qca_setup(struct hci_uart *hu)
 	} else {
 		bt_dev_info(hdev, "ROME setup");
 		if (hu->serdev) {
+			/* Enable NON_PERSISTENT_SETUP QUIRK to ensure to
+			 * execute setup for every hci up.
+			 */
+			set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
+			hu->hdev->shutdown = qca_power_off;
 			qcadev = serdev_device_get_drvdata(hu->serdev);
 			gpiod_set_value_cansleep(qcadev->bt_en, 1);
 			/* Controller needs time to bootup. */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v1 4/4] Bluetooth: hci_qca: Add HCI command timeout handling
  2019-12-25  6:03 [PATCH v1 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
  2019-12-25  6:03 ` [PATCH v1 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
  2019-12-25  6:03 ` [PATCH v1 3/4] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
@ 2019-12-25  6:03 ` Rocky Liao
  2019-12-26  3:04   ` kbuild test robot
                     ` (2 more replies)
  2019-12-26  6:45 ` [PATCH v2 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
                   ` (3 subsequent siblings)
  6 siblings, 3 replies; 34+ messages in thread
From: Rocky Liao @ 2019-12-25  6:03 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: linux-kernel, linux-bluetooth, linux-arm-msm, Rocky Liao

This patch adds the HCI command timeout handling, it will trigger btsoc
to report its memory dump via vendor specific events when hit the defined
max HCI command timeout count. After all the memory dump VSE are sent, the
btsoc will also send a HCI_HW_ERROR event to host and this will cause a new
hci down/up process and the btsoc will be re-initialized.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---
 drivers/bluetooth/hci_qca.c | 40 +++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 7e202041ed77..bc74d69b3d80 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -47,6 +47,8 @@
 #define IBS_HOST_TX_IDLE_TIMEOUT_MS	2000
 #define CMD_TRANS_TIMEOUT_MS		100
 
+#define QCA_BTSOC_DUMP_CMD	0xFB
+
 /* susclk rate */
 #define SUSCLK_RATE_32KHZ	32768
 
@@ -56,6 +58,9 @@
 /* max retry count when init fails */
 #define QCA_MAX_INIT_RETRY_COUNT 3
 
+/* when hit the max cmd time out count, trigger btsoc dump */
+#define QCA_MAX_CMD_TIMEOUT_COUNT 3
+
 enum qca_flags {
 	QCA_IBS_ENABLED,
 	QCA_DROP_VENDOR_EVENT,
@@ -170,6 +175,7 @@ static int qca_regulator_enable(struct qca_serdev *qcadev);
 static void qca_regulator_disable(struct qca_serdev *qcadev);
 static void qca_power_shutdown(struct hci_uart *hu);
 static int qca_power_off(struct hci_dev *hdev);
+static void qca_cmd_timeout(struct hci_uart *hu);
 
 static enum qca_btsoc_type qca_soc_type(struct hci_uart *hu)
 {
@@ -1337,6 +1343,8 @@ static int qca_setup(struct hci_uart *hu)
 	if (!ret) {
 		set_bit(QCA_IBS_ENABLED, &qca->flags);
 		qca_debugfs_init(hdev);
+		hdev->cmd_timeout = qca_cmd_timeout;
+		qca->cmd_timeout_cnt = 0;
 	} else if (ret == -ENOENT) {
 		/* No patch/nvm-config found, run with original fw/config */
 		ret = 0;
@@ -1467,6 +1475,38 @@ static int qca_power_off(struct hci_dev *hdev)
 	return 0;
 }
 
+static int qca_send_btsoc_dump_cmd(struct hci_uart *hu)
+{
+	int err = 0;
+	struct sk_buff *skb = NULL;
+	struct qca_data *qca = hu->priv;
+
+	BT_DBG("hu %p sending btsoc dump command", hu);
+
+	skb = bt_skb_alloc(1, GFP_ATOMIC);
+	if (!skb) {
+		BT_ERR("Failed to allocate memory for qca dump command");
+		return -ENOMEM;
+	}
+
+	skb_put_u8(skb, QCA_BTSOC_DUMP_CMD);
+
+	skb_queue_tail(&qca->txq, skb);
+
+	return err;
+}
+
+
+static void qca_cmd_timeout(struct hci_uart *hu)
+{
+	struct qca_data *qca = hu->priv;
+
+	BT_ERR("hu %p hci cmd timeout count=0x%x", hu, ++qca->cmd_timeout_cnt);
+
+	if (qca->cmd_timeout_cnt >= QCA_MAX_CMD_TIMEOUT_COUNT)
+		qca_send_btsoc_dump_cmd(hu);
+}
+
 static int qca_regulator_enable(struct qca_serdev *qcadev)
 {
 	struct qca_power *power = qcadev->bt_power;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v1 4/4] Bluetooth: hci_qca: Add HCI command timeout handling
  2019-12-25  6:03 ` [PATCH v1 4/4] Bluetooth: hci_qca: Add HCI command timeout handling Rocky Liao
@ 2019-12-26  3:04   ` kbuild test robot
  2019-12-26  7:46   ` kbuild test robot
  2019-12-26 23:33   ` kbuild test robot
  2 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2019-12-26  3:04 UTC (permalink / raw)
  To: Rocky Liao
  Cc: kbuild-all, marcel, johan.hedberg, linux-kernel, linux-bluetooth,
	linux-arm-msm, Rocky Liao

[-- Attachment #1: Type: text/plain, Size: 6555 bytes --]

Hi Rocky,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bluetooth-next/master]
[also build test ERROR on linux/master linus/master v5.5-rc3 next-20191220]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Rocky-Liao/Bluetooth-hci_qca-Add-QCA-Rome-power-off-support-to-the-qca_power_off/20191226-050217
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: nds32-allyesconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=9.2.0 make.cross ARCH=nds32 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/bluetooth/hci_qca.c: In function 'qca_setup':
>> drivers/bluetooth/hci_qca.c:1346:21: error: assignment to 'void (*)(struct hci_dev *)' from incompatible pointer type 'void (*)(struct hci_uart *)' [-Werror=incompatible-pointer-types]
    1346 |   hdev->cmd_timeout = qca_cmd_timeout;
         |                     ^
   drivers/bluetooth/hci_qca.c:1347:6: error: 'struct qca_data' has no member named 'cmd_timeout_cnt'
    1347 |   qca->cmd_timeout_cnt = 0;
         |      ^~
   In file included from drivers/bluetooth/hci_qca.c:33:
   drivers/bluetooth/hci_qca.c: In function 'qca_cmd_timeout':
   drivers/bluetooth/hci_qca.c:1504:54: error: 'struct qca_data' has no member named 'cmd_timeout_cnt'
    1504 |  BT_ERR("hu %p hci cmd timeout count=0x%x", hu, ++qca->cmd_timeout_cnt);
         |                                                      ^~
   include/net/bluetooth/bluetooth.h:138:45: note: in definition of macro 'BT_ERR'
     138 | #define BT_ERR(fmt, ...) bt_err(fmt "\n", ##__VA_ARGS__)
         |                                             ^~~~~~~~~~~
   drivers/bluetooth/hci_qca.c:1506:9: error: 'struct qca_data' has no member named 'cmd_timeout_cnt'
    1506 |  if (qca->cmd_timeout_cnt >= QCA_MAX_CMD_TIMEOUT_COUNT)
         |         ^~
   cc1: some warnings being treated as errors

vim +1346 drivers/bluetooth/hci_qca.c

  1264	
  1265	static int qca_setup(struct hci_uart *hu)
  1266	{
  1267		struct hci_dev *hdev = hu->hdev;
  1268		struct qca_data *qca = hu->priv;
  1269		struct qca_serdev *qcadev;
  1270		unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
  1271		unsigned int init_retry_count = 0;
  1272		enum qca_btsoc_type soc_type = qca_soc_type(hu);
  1273		const char *firmware_name = qca_get_firmware_name(hu);
  1274		int ret;
  1275		int soc_ver = 0;
  1276	
  1277		ret = qca_check_speeds(hu);
  1278		if (ret)
  1279			return ret;
  1280	
  1281		/* Patch downloading has to be done without IBS mode */
  1282		clear_bit(QCA_IBS_ENABLED, &qca->flags);
  1283	
  1284		/* Enable controller to do both LE scan and BR/EDR inquiry
  1285		 * simultaneously.
  1286		 */
  1287		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
  1288	
  1289	retry:
  1290		if (qca_is_wcn399x(soc_type)) {
  1291			bt_dev_info(hdev, "setting up wcn3990");
  1292	
  1293			/* Enable NON_PERSISTENT_SETUP QUIRK to ensure to execute
  1294			 * setup for every hci up.
  1295			 */
  1296			set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
  1297			set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
  1298			hu->hdev->shutdown = qca_power_off;
  1299			ret = qca_wcn3990_init(hu);
  1300			if (ret)
  1301				return ret;
  1302	
  1303			ret = qca_read_soc_version(hdev, &soc_ver, soc_type);
  1304			if (ret)
  1305				return ret;
  1306		} else {
  1307			bt_dev_info(hdev, "ROME setup");
  1308			if (hu->serdev) {
  1309				/* Enable NON_PERSISTENT_SETUP QUIRK to ensure to
  1310				 * execute setup for every hci up.
  1311				 */
  1312				set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
  1313				hu->hdev->shutdown = qca_power_off;
  1314				qcadev = serdev_device_get_drvdata(hu->serdev);
  1315				gpiod_set_value_cansleep(qcadev->bt_en, 1);
  1316				/* Controller needs time to bootup. */
  1317				msleep(150);
  1318			}
  1319			qca_set_speed(hu, QCA_INIT_SPEED);
  1320		}
  1321	
  1322		/* Setup user speed if needed */
  1323		speed = qca_get_speed(hu, QCA_OPER_SPEED);
  1324		if (speed) {
  1325			ret = qca_set_speed(hu, QCA_OPER_SPEED);
  1326			if (ret)
  1327				return ret;
  1328	
  1329			qca_baudrate = qca_get_baudrate_value(speed);
  1330		}
  1331	
  1332		if (!qca_is_wcn399x(soc_type)) {
  1333			/* Get QCA version information */
  1334			ret = qca_read_soc_version(hdev, &soc_ver, soc_type);
  1335			if (ret)
  1336				return ret;
  1337		}
  1338	
  1339		bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver);
  1340		/* Setup patch / NVM configurations */
  1341		ret = qca_uart_setup(hdev, qca_baudrate, soc_type, soc_ver,
  1342				firmware_name);
  1343		if (!ret) {
  1344			set_bit(QCA_IBS_ENABLED, &qca->flags);
  1345			qca_debugfs_init(hdev);
> 1346			hdev->cmd_timeout = qca_cmd_timeout;
  1347			qca->cmd_timeout_cnt = 0;
  1348		} else if (ret == -ENOENT) {
  1349			/* No patch/nvm-config found, run with original fw/config */
  1350			ret = 0;
  1351		} else if (ret == -EAGAIN) {
  1352			/*
  1353			 * Userspace firmware loader will return -EAGAIN in case no
  1354			 * patch/nvm-config is found, so run with original fw/config.
  1355			 */
  1356			ret = 0;
  1357		} else {
  1358			if (init_retry_count < QCA_MAX_INIT_RETRY_COUNT) {
  1359				qca_power_off(hdev);
  1360				if (hu->serdev) {
  1361					serdev_device_close(hu->serdev);
  1362					ret = serdev_device_open(hu->serdev);
  1363					if (ret) {
  1364						bt_dev_err(hu->hdev, "open port fail");
  1365						return ret;
  1366					}
  1367				}
  1368				init_retry_count++;
  1369				goto retry;
  1370			}
  1371		}
  1372	
  1373		/* Setup bdaddr */
  1374		if (qca_is_wcn399x(soc_type))
  1375			hu->hdev->set_bdaddr = qca_set_bdaddr;
  1376		else
  1377			hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
  1378	
  1379		return ret;
  1380	}
  1381	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54011 bytes --]

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off()
  2019-12-25  6:03 [PATCH v1 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
                   ` (2 preceding siblings ...)
  2019-12-25  6:03 ` [PATCH v1 4/4] Bluetooth: hci_qca: Add HCI command timeout handling Rocky Liao
@ 2019-12-26  6:45 ` Rocky Liao
  2019-12-26  6:45   ` [PATCH v2 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
                     ` (2 more replies)
  2019-12-27  7:21 ` [PATCH v3 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
                   ` (2 subsequent siblings)
  6 siblings, 3 replies; 34+ messages in thread
From: Rocky Liao @ 2019-12-26  6:45 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: linux-kernel, linux-bluetooth, linux-arm-msm, Rocky Liao

We will need to call the qca_power_off() API to power off Rome, add the
support into it. QCA Rome is using bt_en GPIO for power off, so we just
need to pull down this bt_en GPIO to power off it.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---

Changes in v2: None

 drivers/bluetooth/hci_qca.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index b602ed01505b..43fd84028786 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1413,13 +1413,26 @@ static void qca_power_shutdown(struct hci_uart *hu)
 static int qca_power_off(struct hci_dev *hdev)
 {
 	struct hci_uart *hu = hci_get_drvdata(hdev);
+	struct qca_serdev *qcadev;
+	enum qca_btsoc_type soc_type = qca_soc_type(hu);
+
+	if (qca_is_wcn399x(soc_type)) {
+		/* Perform pre shutdown command */
+		qca_send_pre_shutdown_cmd(hdev);
+
+		usleep_range(8000, 10000);
 
-	/* Perform pre shutdown command */
-	qca_send_pre_shutdown_cmd(hdev);
+		qca_power_shutdown(hu);
+	} else {
+		if (hu->serdev) {
+			qcadev = serdev_device_get_drvdata(hu->serdev);
+
+			gpiod_set_value_cansleep(qcadev->bt_en, 0);
 
-	usleep_range(8000, 10000);
+			usleep_range(8000, 10000);
+		}
+	}
 
-	qca_power_shutdown(hu);
 	return 0;
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails
  2019-12-26  6:45 ` [PATCH v2 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
@ 2019-12-26  6:45   ` Rocky Liao
  2019-12-26  6:45   ` [PATCH v2 3/4] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
  2019-12-26  6:45   ` [PATCH v2 4/4] Bluetooth: hci_qca: Add HCI command timeout handling Rocky Liao
  2 siblings, 0 replies; 34+ messages in thread
From: Rocky Liao @ 2019-12-26  6:45 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: linux-kernel, linux-bluetooth, linux-arm-msm, Rocky Liao

This patch adds the retry of btsoc initialization when it fails. There are
reports that the btsoc initialization may fail on some platforms but the
repro ratio is very low. The failure may be caused by UART, platform HW or
the btsoc itself but it's very difficlut to root cause, given the repro
ratio is very low. Add a retry for the btsoc initialization will resolve
most of the failures and make Bluetooth finally works.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---

Changes in v2: None

 drivers/bluetooth/hci_qca.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 43fd84028786..45042aa27fa4 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -53,6 +53,9 @@
 /* Controller debug log header */
 #define QCA_DEBUG_HANDLE	0x2EDC
 
+/* max retry count when init fails */
+#define QCA_MAX_INIT_RETRY_COUNT 3
+
 enum qca_flags {
 	QCA_IBS_ENABLED,
 	QCA_DROP_VENDOR_EVENT,
@@ -1257,7 +1260,9 @@ static int qca_setup(struct hci_uart *hu)
 {
 	struct hci_dev *hdev = hu->hdev;
 	struct qca_data *qca = hu->priv;
+	struct qca_serdev *qcadev;
 	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
+	unsigned int init_retry_count = 0;
 	enum qca_btsoc_type soc_type = qca_soc_type(hu);
 	const char *firmware_name = qca_get_firmware_name(hu);
 	int ret;
@@ -1275,6 +1280,7 @@ static int qca_setup(struct hci_uart *hu)
 	 */
 	set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 
+retry:
 	if (qca_is_wcn399x(soc_type)) {
 		bt_dev_info(hdev, "setting up wcn3990");
 
@@ -1293,6 +1299,12 @@ static int qca_setup(struct hci_uart *hu)
 			return ret;
 	} else {
 		bt_dev_info(hdev, "ROME setup");
+		if (hu->serdev) {
+			qcadev = serdev_device_get_drvdata(hu->serdev);
+			gpiod_set_value_cansleep(qcadev->bt_en, 1);
+			/* Controller needs time to bootup. */
+			msleep(150);
+		}
 		qca_set_speed(hu, QCA_INIT_SPEED);
 	}
 
@@ -1329,6 +1341,20 @@ static int qca_setup(struct hci_uart *hu)
 		 * patch/nvm-config is found, so run with original fw/config.
 		 */
 		ret = 0;
+	} else {
+		if (init_retry_count < QCA_MAX_INIT_RETRY_COUNT) {
+			qca_power_off(hdev);
+			if (hu->serdev) {
+				serdev_device_close(hu->serdev);
+				ret = serdev_device_open(hu->serdev);
+				if (ret) {
+					bt_dev_err(hu->hdev, "open port fail");
+					return ret;
+				}
+			}
+			init_retry_count++;
+			goto retry;
+		}
 	}
 
 	/* Setup bdaddr */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 3/4] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome
  2019-12-26  6:45 ` [PATCH v2 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
  2019-12-26  6:45   ` [PATCH v2 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
@ 2019-12-26  6:45   ` Rocky Liao
  2019-12-26 20:30     ` Marcel Holtmann
  2019-12-26  6:45   ` [PATCH v2 4/4] Bluetooth: hci_qca: Add HCI command timeout handling Rocky Liao
  2 siblings, 1 reply; 34+ messages in thread
From: Rocky Liao @ 2019-12-26  6:45 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: linux-kernel, linux-bluetooth, linux-arm-msm, Rocky Liao

This patch registers hdev->shutdown() callback and also sets
HCI_QUIRK_NON_PERSISTENT_SETUP for QCA Rome. It will power-off the BT chip
during hci down and power-on/initialize the chip again during hci up.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---

Changes in v2: None

 drivers/bluetooth/hci_qca.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 45042aa27fa4..7e202041ed77 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1300,6 +1300,11 @@ static int qca_setup(struct hci_uart *hu)
 	} else {
 		bt_dev_info(hdev, "ROME setup");
 		if (hu->serdev) {
+			/* Enable NON_PERSISTENT_SETUP QUIRK to ensure to
+			 * execute setup for every hci up.
+			 */
+			set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
+			hu->hdev->shutdown = qca_power_off;
 			qcadev = serdev_device_get_drvdata(hu->serdev);
 			gpiod_set_value_cansleep(qcadev->bt_en, 1);
 			/* Controller needs time to bootup. */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v2 4/4] Bluetooth: hci_qca: Add HCI command timeout handling
  2019-12-26  6:45 ` [PATCH v2 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
  2019-12-26  6:45   ` [PATCH v2 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
  2019-12-26  6:45   ` [PATCH v2 3/4] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
@ 2019-12-26  6:45   ` Rocky Liao
  2019-12-26 20:29     ` Marcel Holtmann
  2 siblings, 1 reply; 34+ messages in thread
From: Rocky Liao @ 2019-12-26  6:45 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: linux-kernel, linux-bluetooth, linux-arm-msm, Rocky Liao

This patch adds the HCI command timeout handling, it will trigger btsoc
to report its memory dump via vendor specific events when hit the defined
max HCI command timeout count. After all the memory dump VSE are sent, the
btsoc will also send a HCI_HW_ERROR event to host and this will cause a new
hci down/up process and the btsoc will be re-initialized.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---

Changes in v2:
- Fix build error

 drivers/bluetooth/hci_qca.c | 43 +++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 7e202041ed77..34ef73daebb2 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -47,6 +47,8 @@
 #define IBS_HOST_TX_IDLE_TIMEOUT_MS	2000
 #define CMD_TRANS_TIMEOUT_MS		100
 
+#define QCA_BTSOC_DUMP_CMD	0xFB
+
 /* susclk rate */
 #define SUSCLK_RATE_32KHZ	32768
 
@@ -56,6 +58,9 @@
 /* max retry count when init fails */
 #define QCA_MAX_INIT_RETRY_COUNT 3
 
+/* when hit the max cmd time out count, trigger btsoc dump */
+#define QCA_MAX_CMD_TIMEOUT_COUNT 3
+
 enum qca_flags {
 	QCA_IBS_ENABLED,
 	QCA_DROP_VENDOR_EVENT,
@@ -123,6 +128,8 @@ struct qca_data {
 	u64 rx_votes_off;
 	u64 votes_on;
 	u64 votes_off;
+
+	u32 cmd_timeout_cnt;
 };
 
 enum qca_speed_type {
@@ -170,6 +177,7 @@ static int qca_regulator_enable(struct qca_serdev *qcadev);
 static void qca_regulator_disable(struct qca_serdev *qcadev);
 static void qca_power_shutdown(struct hci_uart *hu);
 static int qca_power_off(struct hci_dev *hdev);
+static void qca_cmd_timeout(struct hci_dev *hdev);
 
 static enum qca_btsoc_type qca_soc_type(struct hci_uart *hu)
 {
@@ -1337,6 +1345,8 @@ static int qca_setup(struct hci_uart *hu)
 	if (!ret) {
 		set_bit(QCA_IBS_ENABLED, &qca->flags);
 		qca_debugfs_init(hdev);
+		hdev->cmd_timeout = qca_cmd_timeout;
+		qca->cmd_timeout_cnt = 0;
 	} else if (ret == -ENOENT) {
 		/* No patch/nvm-config found, run with original fw/config */
 		ret = 0;
@@ -1467,6 +1477,39 @@ static int qca_power_off(struct hci_dev *hdev)
 	return 0;
 }
 
+static int qca_send_btsoc_dump_cmd(struct hci_uart *hu)
+{
+	int err = 0;
+	struct sk_buff *skb = NULL;
+	struct qca_data *qca = hu->priv;
+
+	BT_DBG("hu %p sending btsoc dump command", hu);
+
+	skb = bt_skb_alloc(1, GFP_ATOMIC);
+	if (!skb) {
+		BT_ERR("Failed to allocate memory for qca dump command");
+		return -ENOMEM;
+	}
+
+	skb_put_u8(skb, QCA_BTSOC_DUMP_CMD);
+
+	skb_queue_tail(&qca->txq, skb);
+
+	return err;
+}
+
+
+static void qca_cmd_timeout(struct hci_dev *hdev)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+	struct qca_data *qca = hu->priv;
+
+	BT_ERR("hu %p hci cmd timeout count=0x%x", hu, ++qca->cmd_timeout_cnt);
+
+	if (qca->cmd_timeout_cnt >= QCA_MAX_CMD_TIMEOUT_COUNT)
+		qca_send_btsoc_dump_cmd(hu);
+}
+
 static int qca_regulator_enable(struct qca_serdev *qcadev)
 {
 	struct qca_power *power = qcadev->bt_power;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v1 4/4] Bluetooth: hci_qca: Add HCI command timeout handling
  2019-12-25  6:03 ` [PATCH v1 4/4] Bluetooth: hci_qca: Add HCI command timeout handling Rocky Liao
  2019-12-26  3:04   ` kbuild test robot
@ 2019-12-26  7:46   ` kbuild test robot
  2019-12-26 23:33   ` kbuild test robot
  2 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2019-12-26  7:46 UTC (permalink / raw)
  To: Rocky Liao
  Cc: kbuild-all, marcel, johan.hedberg, linux-kernel, linux-bluetooth,
	linux-arm-msm, Rocky Liao

[-- Attachment #1: Type: text/plain, Size: 6419 bytes --]

Hi Rocky,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bluetooth-next/master]
[also build test ERROR on linux/master linus/master v5.5-rc3 next-20191220]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Rocky-Liao/Bluetooth-hci_qca-Add-QCA-Rome-power-off-support-to-the-qca_power_off/20191226-050217
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/bluetooth/hci_qca.c: In function 'qca_setup':
>> drivers/bluetooth/hci_qca.c:1346:21: error: assignment from incompatible pointer type [-Werror=incompatible-pointer-types]
      hdev->cmd_timeout = qca_cmd_timeout;
                        ^
>> drivers/bluetooth/hci_qca.c:1347:6: error: 'struct qca_data' has no member named 'cmd_timeout_cnt'
      qca->cmd_timeout_cnt = 0;
         ^~
   In file included from drivers/bluetooth/hci_qca.c:33:0:
   drivers/bluetooth/hci_qca.c: In function 'qca_cmd_timeout':
   drivers/bluetooth/hci_qca.c:1504:54: error: 'struct qca_data' has no member named 'cmd_timeout_cnt'
     BT_ERR("hu %p hci cmd timeout count=0x%x", hu, ++qca->cmd_timeout_cnt);
                                                         ^
   include/net/bluetooth/bluetooth.h:138:45: note: in definition of macro 'BT_ERR'
    #define BT_ERR(fmt, ...) bt_err(fmt "\n", ##__VA_ARGS__)
                                                ^~~~~~~~~~~
   drivers/bluetooth/hci_qca.c:1506:9: error: 'struct qca_data' has no member named 'cmd_timeout_cnt'
     if (qca->cmd_timeout_cnt >= QCA_MAX_CMD_TIMEOUT_COUNT)
            ^~
   cc1: some warnings being treated as errors

vim +1346 drivers/bluetooth/hci_qca.c

  1264	
  1265	static int qca_setup(struct hci_uart *hu)
  1266	{
  1267		struct hci_dev *hdev = hu->hdev;
  1268		struct qca_data *qca = hu->priv;
  1269		struct qca_serdev *qcadev;
  1270		unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
  1271		unsigned int init_retry_count = 0;
  1272		enum qca_btsoc_type soc_type = qca_soc_type(hu);
  1273		const char *firmware_name = qca_get_firmware_name(hu);
  1274		int ret;
  1275		int soc_ver = 0;
  1276	
  1277		ret = qca_check_speeds(hu);
  1278		if (ret)
  1279			return ret;
  1280	
  1281		/* Patch downloading has to be done without IBS mode */
  1282		clear_bit(QCA_IBS_ENABLED, &qca->flags);
  1283	
  1284		/* Enable controller to do both LE scan and BR/EDR inquiry
  1285		 * simultaneously.
  1286		 */
  1287		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
  1288	
  1289	retry:
  1290		if (qca_is_wcn399x(soc_type)) {
  1291			bt_dev_info(hdev, "setting up wcn3990");
  1292	
  1293			/* Enable NON_PERSISTENT_SETUP QUIRK to ensure to execute
  1294			 * setup for every hci up.
  1295			 */
  1296			set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
  1297			set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
  1298			hu->hdev->shutdown = qca_power_off;
  1299			ret = qca_wcn3990_init(hu);
  1300			if (ret)
  1301				return ret;
  1302	
  1303			ret = qca_read_soc_version(hdev, &soc_ver, soc_type);
  1304			if (ret)
  1305				return ret;
  1306		} else {
  1307			bt_dev_info(hdev, "ROME setup");
  1308			if (hu->serdev) {
  1309				/* Enable NON_PERSISTENT_SETUP QUIRK to ensure to
  1310				 * execute setup for every hci up.
  1311				 */
  1312				set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
  1313				hu->hdev->shutdown = qca_power_off;
  1314				qcadev = serdev_device_get_drvdata(hu->serdev);
  1315				gpiod_set_value_cansleep(qcadev->bt_en, 1);
  1316				/* Controller needs time to bootup. */
  1317				msleep(150);
  1318			}
  1319			qca_set_speed(hu, QCA_INIT_SPEED);
  1320		}
  1321	
  1322		/* Setup user speed if needed */
  1323		speed = qca_get_speed(hu, QCA_OPER_SPEED);
  1324		if (speed) {
  1325			ret = qca_set_speed(hu, QCA_OPER_SPEED);
  1326			if (ret)
  1327				return ret;
  1328	
  1329			qca_baudrate = qca_get_baudrate_value(speed);
  1330		}
  1331	
  1332		if (!qca_is_wcn399x(soc_type)) {
  1333			/* Get QCA version information */
  1334			ret = qca_read_soc_version(hdev, &soc_ver, soc_type);
  1335			if (ret)
  1336				return ret;
  1337		}
  1338	
  1339		bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver);
  1340		/* Setup patch / NVM configurations */
  1341		ret = qca_uart_setup(hdev, qca_baudrate, soc_type, soc_ver,
  1342				firmware_name);
  1343		if (!ret) {
  1344			set_bit(QCA_IBS_ENABLED, &qca->flags);
  1345			qca_debugfs_init(hdev);
> 1346			hdev->cmd_timeout = qca_cmd_timeout;
> 1347			qca->cmd_timeout_cnt = 0;
  1348		} else if (ret == -ENOENT) {
  1349			/* No patch/nvm-config found, run with original fw/config */
  1350			ret = 0;
  1351		} else if (ret == -EAGAIN) {
  1352			/*
  1353			 * Userspace firmware loader will return -EAGAIN in case no
  1354			 * patch/nvm-config is found, so run with original fw/config.
  1355			 */
  1356			ret = 0;
  1357		} else {
  1358			if (init_retry_count < QCA_MAX_INIT_RETRY_COUNT) {
  1359				qca_power_off(hdev);
  1360				if (hu->serdev) {
  1361					serdev_device_close(hu->serdev);
  1362					ret = serdev_device_open(hu->serdev);
  1363					if (ret) {
  1364						bt_dev_err(hu->hdev, "open port fail");
  1365						return ret;
  1366					}
  1367				}
  1368				init_retry_count++;
  1369				goto retry;
  1370			}
  1371		}
  1372	
  1373		/* Setup bdaddr */
  1374		if (qca_is_wcn399x(soc_type))
  1375			hu->hdev->set_bdaddr = qca_set_bdaddr;
  1376		else
  1377			hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
  1378	
  1379		return ret;
  1380	}
  1381	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51913 bytes --]

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 4/4] Bluetooth: hci_qca: Add HCI command timeout handling
  2019-12-26  6:45   ` [PATCH v2 4/4] Bluetooth: hci_qca: Add HCI command timeout handling Rocky Liao
@ 2019-12-26 20:29     ` Marcel Holtmann
  0 siblings, 0 replies; 34+ messages in thread
From: Marcel Holtmann @ 2019-12-26 20:29 UTC (permalink / raw)
  To: Rocky Liao; +Cc: Johan Hedberg, linux-kernel, linux-bluetooth, linux-arm-msm

Hi Rocky,

> This patch adds the HCI command timeout handling, it will trigger btsoc
> to report its memory dump via vendor specific events when hit the defined
> max HCI command timeout count. After all the memory dump VSE are sent, the
> btsoc will also send a HCI_HW_ERROR event to host and this will cause a new
> hci down/up process and the btsoc will be re-initialized.
> 
> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
> ---
> 
> Changes in v2:
> - Fix build error
> 
> drivers/bluetooth/hci_qca.c | 43 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 7e202041ed77..34ef73daebb2 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -47,6 +47,8 @@
> #define IBS_HOST_TX_IDLE_TIMEOUT_MS	2000
> #define CMD_TRANS_TIMEOUT_MS		100
> 
> +#define QCA_BTSOC_DUMP_CMD	0xFB
> +
> /* susclk rate */
> #define SUSCLK_RATE_32KHZ	32768
> 
> @@ -56,6 +58,9 @@
> /* max retry count when init fails */
> #define QCA_MAX_INIT_RETRY_COUNT 3
> 
> +/* when hit the max cmd time out count, trigger btsoc dump */
> +#define QCA_MAX_CMD_TIMEOUT_COUNT 3
> +
> enum qca_flags {
> 	QCA_IBS_ENABLED,
> 	QCA_DROP_VENDOR_EVENT,
> @@ -123,6 +128,8 @@ struct qca_data {
> 	u64 rx_votes_off;
> 	u64 votes_on;
> 	u64 votes_off;
> +
> +	u32 cmd_timeout_cnt;
> };
> 
> enum qca_speed_type {
> @@ -170,6 +177,7 @@ static int qca_regulator_enable(struct qca_serdev *qcadev);
> static void qca_regulator_disable(struct qca_serdev *qcadev);
> static void qca_power_shutdown(struct hci_uart *hu);
> static int qca_power_off(struct hci_dev *hdev);
> +static void qca_cmd_timeout(struct hci_dev *hdev);

I thought that I already mentioned that these forward declaration have to be removed. I have no plan to take patches that even add more forward declarations.

> 
> static enum qca_btsoc_type qca_soc_type(struct hci_uart *hu)
> {
> @@ -1337,6 +1345,8 @@ static int qca_setup(struct hci_uart *hu)
> 	if (!ret) {
> 		set_bit(QCA_IBS_ENABLED, &qca->flags);
> 		qca_debugfs_init(hdev);
> +		hdev->cmd_timeout = qca_cmd_timeout;

Why set it here? Set in the probe() callback.

> +		qca->cmd_timeout_cnt = 0;

This is a race conditions if not all chip types will use NON_PERSISTENT_SETUP.

> 	} else if (ret == -ENOENT) {
> 		/* No patch/nvm-config found, run with original fw/config */
> 		ret = 0;
> @@ -1467,6 +1477,39 @@ static int qca_power_off(struct hci_dev *hdev)
> 	return 0;
> }
> 
> +static int qca_send_btsoc_dump_cmd(struct hci_uart *hu)
> +{
> +	int err = 0;
> +	struct sk_buff *skb = NULL;
> +	struct qca_data *qca = hu->priv;
> +
> +	BT_DBG("hu %p sending btsoc dump command", hu);
> +
> +	skb = bt_skb_alloc(1, GFP_ATOMIC);
> +	if (!skb) {
> +		BT_ERR("Failed to allocate memory for qca dump command");
> +		return -ENOMEM;
> +	}
> +
> +	skb_put_u8(skb, QCA_BTSOC_DUMP_CMD);
> +
> +	skb_queue_tail(&qca->txq, skb);
> +
> +	return err;
> +}
> +
> +

No double empty lines.

> +static void qca_cmd_timeout(struct hci_dev *hdev)
> +{
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +	struct qca_data *qca = hu->priv;
> +
> +	BT_ERR("hu %p hci cmd timeout count=0x%x", hu, ++qca->cmd_timeout_cnt);
> +
> +	if (qca->cmd_timeout_cnt >= QCA_MAX_CMD_TIMEOUT_COUNT)
> +		qca_send_btsoc_dump_cmd(hu);
> +}
> +
> static int qca_regulator_enable(struct qca_serdev *qcadev)
> {
> 	struct qca_power *power = qcadev->bt_power;

Regards

Marcel


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v2 3/4] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome
  2019-12-26  6:45   ` [PATCH v2 3/4] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
@ 2019-12-26 20:30     ` Marcel Holtmann
  0 siblings, 0 replies; 34+ messages in thread
From: Marcel Holtmann @ 2019-12-26 20:30 UTC (permalink / raw)
  To: Rocky Liao; +Cc: Johan Hedberg, linux-kernel, linux-bluetooth, linux-arm-msm

Hi Rocky,

> This patch registers hdev->shutdown() callback and also sets
> HCI_QUIRK_NON_PERSISTENT_SETUP for QCA Rome. It will power-off the BT chip
> during hci down and power-on/initialize the chip again during hci up.
> 
> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
> ---
> 
> Changes in v2: None
> 
> drivers/bluetooth/hci_qca.c | 5 +++++
> 1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 45042aa27fa4..7e202041ed77 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1300,6 +1300,11 @@ static int qca_setup(struct hci_uart *hu)
> 	} else {
> 		bt_dev_info(hdev, "ROME setup");
> 		if (hu->serdev) {
> +			/* Enable NON_PERSISTENT_SETUP QUIRK to ensure to
> +			 * execute setup for every hci up.
> +			 */
> +			set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
> +			hu->hdev->shutdown = qca_power_off;

why are you setting it in the ->setup callback and not in the ->probe callback?

Regards

Marcel


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v1 4/4] Bluetooth: hci_qca: Add HCI command timeout handling
  2019-12-25  6:03 ` [PATCH v1 4/4] Bluetooth: hci_qca: Add HCI command timeout handling Rocky Liao
  2019-12-26  3:04   ` kbuild test robot
  2019-12-26  7:46   ` kbuild test robot
@ 2019-12-26 23:33   ` kbuild test robot
  2 siblings, 0 replies; 34+ messages in thread
From: kbuild test robot @ 2019-12-26 23:33 UTC (permalink / raw)
  To: Rocky Liao
  Cc: kbuild-all, marcel, johan.hedberg, linux-kernel, linux-bluetooth,
	linux-arm-msm, Rocky Liao

Hi Rocky,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on linux/master linus/master v5.5-rc3 next-20191220]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Rocky-Liao/Bluetooth-hci_qca-Add-QCA-Rome-power-off-support-to-the-qca_power_off/20191226-050217
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-129-g341daf20-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

   drivers/bluetooth/hci_qca.c:1504:9: sparse: sparse: no member 'cmd_timeout_cnt' in struct qca_data
   drivers/bluetooth/hci_qca.c:1506:16: sparse: sparse: no member 'cmd_timeout_cnt' in struct qca_data
>> drivers/bluetooth/hci_qca.c:1346:35: sparse: sparse: incorrect type in assignment (incompatible argument 1 (different base types))
>> drivers/bluetooth/hci_qca.c:1346:35: sparse:    expected void ( *cmd_timeout )( ... )
>> drivers/bluetooth/hci_qca.c:1346:35: sparse:    got void ( * )( ... )
   drivers/bluetooth/hci_qca.c:1347:20: sparse: sparse: no member 'cmd_timeout_cnt' in struct qca_data

vim +1346 drivers/bluetooth/hci_qca.c

  1264	
  1265	static int qca_setup(struct hci_uart *hu)
  1266	{
  1267		struct hci_dev *hdev = hu->hdev;
  1268		struct qca_data *qca = hu->priv;
  1269		struct qca_serdev *qcadev;
  1270		unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
  1271		unsigned int init_retry_count = 0;
  1272		enum qca_btsoc_type soc_type = qca_soc_type(hu);
  1273		const char *firmware_name = qca_get_firmware_name(hu);
  1274		int ret;
  1275		int soc_ver = 0;
  1276	
  1277		ret = qca_check_speeds(hu);
  1278		if (ret)
  1279			return ret;
  1280	
  1281		/* Patch downloading has to be done without IBS mode */
  1282		clear_bit(QCA_IBS_ENABLED, &qca->flags);
  1283	
  1284		/* Enable controller to do both LE scan and BR/EDR inquiry
  1285		 * simultaneously.
  1286		 */
  1287		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
  1288	
  1289	retry:
  1290		if (qca_is_wcn399x(soc_type)) {
  1291			bt_dev_info(hdev, "setting up wcn3990");
  1292	
  1293			/* Enable NON_PERSISTENT_SETUP QUIRK to ensure to execute
  1294			 * setup for every hci up.
  1295			 */
  1296			set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
  1297			set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
  1298			hu->hdev->shutdown = qca_power_off;
  1299			ret = qca_wcn3990_init(hu);
  1300			if (ret)
  1301				return ret;
  1302	
  1303			ret = qca_read_soc_version(hdev, &soc_ver, soc_type);
  1304			if (ret)
  1305				return ret;
  1306		} else {
  1307			bt_dev_info(hdev, "ROME setup");
  1308			if (hu->serdev) {
  1309				/* Enable NON_PERSISTENT_SETUP QUIRK to ensure to
  1310				 * execute setup for every hci up.
  1311				 */
  1312				set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
  1313				hu->hdev->shutdown = qca_power_off;
  1314				qcadev = serdev_device_get_drvdata(hu->serdev);
  1315				gpiod_set_value_cansleep(qcadev->bt_en, 1);
  1316				/* Controller needs time to bootup. */
  1317				msleep(150);
  1318			}
  1319			qca_set_speed(hu, QCA_INIT_SPEED);
  1320		}
  1321	
  1322		/* Setup user speed if needed */
  1323		speed = qca_get_speed(hu, QCA_OPER_SPEED);
  1324		if (speed) {
  1325			ret = qca_set_speed(hu, QCA_OPER_SPEED);
  1326			if (ret)
  1327				return ret;
  1328	
  1329			qca_baudrate = qca_get_baudrate_value(speed);
  1330		}
  1331	
  1332		if (!qca_is_wcn399x(soc_type)) {
  1333			/* Get QCA version information */
  1334			ret = qca_read_soc_version(hdev, &soc_ver, soc_type);
  1335			if (ret)
  1336				return ret;
  1337		}
  1338	
  1339		bt_dev_info(hdev, "QCA controller version 0x%08x", soc_ver);
  1340		/* Setup patch / NVM configurations */
  1341		ret = qca_uart_setup(hdev, qca_baudrate, soc_type, soc_ver,
  1342				firmware_name);
  1343		if (!ret) {
  1344			set_bit(QCA_IBS_ENABLED, &qca->flags);
  1345			qca_debugfs_init(hdev);
> 1346			hdev->cmd_timeout = qca_cmd_timeout;
  1347			qca->cmd_timeout_cnt = 0;
  1348		} else if (ret == -ENOENT) {
  1349			/* No patch/nvm-config found, run with original fw/config */
  1350			ret = 0;
  1351		} else if (ret == -EAGAIN) {
  1352			/*
  1353			 * Userspace firmware loader will return -EAGAIN in case no
  1354			 * patch/nvm-config is found, so run with original fw/config.
  1355			 */
  1356			ret = 0;
  1357		} else {
  1358			if (init_retry_count < QCA_MAX_INIT_RETRY_COUNT) {
  1359				qca_power_off(hdev);
  1360				if (hu->serdev) {
  1361					serdev_device_close(hu->serdev);
  1362					ret = serdev_device_open(hu->serdev);
  1363					if (ret) {
  1364						bt_dev_err(hu->hdev, "open port fail");
  1365						return ret;
  1366					}
  1367				}
  1368				init_retry_count++;
  1369				goto retry;
  1370			}
  1371		}
  1372	
  1373		/* Setup bdaddr */
  1374		if (qca_is_wcn399x(soc_type))
  1375			hu->hdev->set_bdaddr = qca_set_bdaddr;
  1376		else
  1377			hu->hdev->set_bdaddr = qca_set_bdaddr_rome;
  1378	
  1379		return ret;
  1380	}
  1381	
  1382	static const struct hci_uart_proto qca_proto = {
  1383		.id		= HCI_UART_QCA,
  1384		.name		= "QCA",
  1385		.manufacturer	= 29,
  1386		.init_speed	= 115200,
  1387		.oper_speed	= 3000000,
  1388		.open		= qca_open,
  1389		.close		= qca_close,
  1390		.flush		= qca_flush,
  1391		.setup		= qca_setup,
  1392		.recv		= qca_recv,
  1393		.enqueue	= qca_enqueue,
  1394		.dequeue	= qca_dequeue,
  1395	};
  1396	
  1397	static const struct qca_vreg_data qca_soc_data_wcn3990 = {
  1398		.soc_type = QCA_WCN3990,
  1399		.vregs = (struct qca_vreg []) {
  1400			{ "vddio", 15000  },
  1401			{ "vddxo", 80000  },
  1402			{ "vddrf", 300000 },
  1403			{ "vddch0", 450000 },
  1404		},
  1405		.num_vregs = 4,
  1406	};
  1407	
  1408	static const struct qca_vreg_data qca_soc_data_wcn3991 = {
  1409		.soc_type = QCA_WCN3991,
  1410		.vregs = (struct qca_vreg []) {
  1411			{ "vddio", 15000  },
  1412			{ "vddxo", 80000  },
  1413			{ "vddrf", 300000 },
  1414			{ "vddch0", 450000 },
  1415		},
  1416		.num_vregs = 4,
  1417	};
  1418	
  1419	static const struct qca_vreg_data qca_soc_data_wcn3998 = {
  1420		.soc_type = QCA_WCN3998,
  1421		.vregs = (struct qca_vreg []) {
  1422			{ "vddio", 10000  },
  1423			{ "vddxo", 80000  },
  1424			{ "vddrf", 300000 },
  1425			{ "vddch0", 450000 },
  1426		},
  1427		.num_vregs = 4,
  1428	};
  1429	
  1430	static void qca_power_shutdown(struct hci_uart *hu)
  1431	{
  1432		struct qca_serdev *qcadev;
  1433		struct qca_data *qca = hu->priv;
  1434		unsigned long flags;
  1435	
  1436		qcadev = serdev_device_get_drvdata(hu->serdev);
  1437	
  1438		/* From this point we go into power off state. But serial port is
  1439		 * still open, stop queueing the IBS data and flush all the buffered
  1440		 * data in skb's.
  1441		 */
  1442		spin_lock_irqsave(&qca->hci_ibs_lock, flags);
  1443		clear_bit(QCA_IBS_ENABLED, &qca->flags);
  1444		qca_flush(hu);
  1445		spin_unlock_irqrestore(&qca->hci_ibs_lock, flags);
  1446	
  1447		host_set_baudrate(hu, 2400);
  1448		qca_send_power_pulse(hu, false);
  1449		qca_regulator_disable(qcadev);
  1450	}
  1451	
  1452	static int qca_power_off(struct hci_dev *hdev)
  1453	{
  1454		struct hci_uart *hu = hci_get_drvdata(hdev);
  1455		struct qca_serdev *qcadev;
  1456		enum qca_btsoc_type soc_type = qca_soc_type(hu);
  1457	
  1458		if (qca_is_wcn399x(soc_type)) {
  1459			/* Perform pre shutdown command */
  1460			qca_send_pre_shutdown_cmd(hdev);
  1461	
  1462			usleep_range(8000, 10000);
  1463	
  1464			qca_power_shutdown(hu);
  1465		} else {
  1466			if (hu->serdev) {
  1467				qcadev = serdev_device_get_drvdata(hu->serdev);
  1468	
  1469				gpiod_set_value_cansleep(qcadev->bt_en, 0);
  1470	
  1471				usleep_range(8000, 10000);
  1472			}
  1473		}
  1474	
  1475		return 0;
  1476	}
  1477	
  1478	static int qca_send_btsoc_dump_cmd(struct hci_uart *hu)
  1479	{
  1480		int err = 0;
  1481		struct sk_buff *skb = NULL;
  1482		struct qca_data *qca = hu->priv;
  1483	
  1484		BT_DBG("hu %p sending btsoc dump command", hu);
  1485	
  1486		skb = bt_skb_alloc(1, GFP_ATOMIC);
  1487		if (!skb) {
  1488			BT_ERR("Failed to allocate memory for qca dump command");
  1489			return -ENOMEM;
  1490		}
  1491	
  1492		skb_put_u8(skb, QCA_BTSOC_DUMP_CMD);
  1493	
  1494		skb_queue_tail(&qca->txq, skb);
  1495	
  1496		return err;
  1497	}
  1498	
  1499	
  1500	static void qca_cmd_timeout(struct hci_uart *hu)
  1501	{
  1502		struct qca_data *qca = hu->priv;
  1503	
> 1504		BT_ERR("hu %p hci cmd timeout count=0x%x", hu, ++qca->cmd_timeout_cnt);
  1505	
  1506		if (qca->cmd_timeout_cnt >= QCA_MAX_CMD_TIMEOUT_COUNT)
  1507			qca_send_btsoc_dump_cmd(hu);
  1508	}
  1509	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v3 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off()
  2019-12-25  6:03 [PATCH v1 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
                   ` (3 preceding siblings ...)
  2019-12-26  6:45 ` [PATCH v2 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
@ 2019-12-27  7:21 ` Rocky Liao
  2019-12-27  7:21   ` [PATCH v3 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
                     ` (2 more replies)
  2020-01-15  8:55 ` [PATCH v4 1/3] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_shutdown() Rocky Liao
  2020-01-16  3:22 ` [PATCH v5] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
  6 siblings, 3 replies; 34+ messages in thread
From: Rocky Liao @ 2019-12-27  7:21 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: linux-kernel, linux-bluetooth, linux-arm-msm, Rocky Liao

We will need to call the qca_power_off() API to power off Rome, add the
support into it. QCA Rome is using bt_en GPIO for power off, so we just
need to pull down this bt_en GPIO to power off it.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---

Changes in v2: None
Changes in v3: None

 drivers/bluetooth/hci_qca.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index b602ed01505b..43fd84028786 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1413,13 +1413,26 @@ static void qca_power_shutdown(struct hci_uart *hu)
 static int qca_power_off(struct hci_dev *hdev)
 {
 	struct hci_uart *hu = hci_get_drvdata(hdev);
+	struct qca_serdev *qcadev;
+	enum qca_btsoc_type soc_type = qca_soc_type(hu);
+
+	if (qca_is_wcn399x(soc_type)) {
+		/* Perform pre shutdown command */
+		qca_send_pre_shutdown_cmd(hdev);
+
+		usleep_range(8000, 10000);
 
-	/* Perform pre shutdown command */
-	qca_send_pre_shutdown_cmd(hdev);
+		qca_power_shutdown(hu);
+	} else {
+		if (hu->serdev) {
+			qcadev = serdev_device_get_drvdata(hu->serdev);
+
+			gpiod_set_value_cansleep(qcadev->bt_en, 0);
 
-	usleep_range(8000, 10000);
+			usleep_range(8000, 10000);
+		}
+	}
 
-	qca_power_shutdown(hu);
 	return 0;
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v3 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails
  2019-12-27  7:21 ` [PATCH v3 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
@ 2019-12-27  7:21   ` Rocky Liao
  2020-01-02 18:41     ` Matthias Kaehlcke
  2019-12-27  7:21   ` [PATCH v3 3/4] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
  2019-12-27  7:21   ` [PATCH v3 4/4] Bluetooth: hci_qca: Add HCI command timeout handling Rocky Liao
  2 siblings, 1 reply; 34+ messages in thread
From: Rocky Liao @ 2019-12-27  7:21 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: linux-kernel, linux-bluetooth, linux-arm-msm, Rocky Liao

This patch adds the retry of btsoc initialization when it fails. There are
reports that the btsoc initialization may fail on some platforms but the
repro ratio is very low. The failure may be caused by UART, platform HW or
the btsoc itself but it's very difficlut to root cause, given the repro
ratio is very low. Add a retry for the btsoc initialization will resolve
most of the failures and make Bluetooth finally works.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---

Changes in v2: None
Changes in v3: None

 drivers/bluetooth/hci_qca.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 43fd84028786..45042aa27fa4 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -53,6 +53,9 @@
 /* Controller debug log header */
 #define QCA_DEBUG_HANDLE	0x2EDC
 
+/* max retry count when init fails */
+#define QCA_MAX_INIT_RETRY_COUNT 3
+
 enum qca_flags {
 	QCA_IBS_ENABLED,
 	QCA_DROP_VENDOR_EVENT,
@@ -1257,7 +1260,9 @@ static int qca_setup(struct hci_uart *hu)
 {
 	struct hci_dev *hdev = hu->hdev;
 	struct qca_data *qca = hu->priv;
+	struct qca_serdev *qcadev;
 	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
+	unsigned int init_retry_count = 0;
 	enum qca_btsoc_type soc_type = qca_soc_type(hu);
 	const char *firmware_name = qca_get_firmware_name(hu);
 	int ret;
@@ -1275,6 +1280,7 @@ static int qca_setup(struct hci_uart *hu)
 	 */
 	set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 
+retry:
 	if (qca_is_wcn399x(soc_type)) {
 		bt_dev_info(hdev, "setting up wcn3990");
 
@@ -1293,6 +1299,12 @@ static int qca_setup(struct hci_uart *hu)
 			return ret;
 	} else {
 		bt_dev_info(hdev, "ROME setup");
+		if (hu->serdev) {
+			qcadev = serdev_device_get_drvdata(hu->serdev);
+			gpiod_set_value_cansleep(qcadev->bt_en, 1);
+			/* Controller needs time to bootup. */
+			msleep(150);
+		}
 		qca_set_speed(hu, QCA_INIT_SPEED);
 	}
 
@@ -1329,6 +1341,20 @@ static int qca_setup(struct hci_uart *hu)
 		 * patch/nvm-config is found, so run with original fw/config.
 		 */
 		ret = 0;
+	} else {
+		if (init_retry_count < QCA_MAX_INIT_RETRY_COUNT) {
+			qca_power_off(hdev);
+			if (hu->serdev) {
+				serdev_device_close(hu->serdev);
+				ret = serdev_device_open(hu->serdev);
+				if (ret) {
+					bt_dev_err(hu->hdev, "open port fail");
+					return ret;
+				}
+			}
+			init_retry_count++;
+			goto retry;
+		}
 	}
 
 	/* Setup bdaddr */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v3 3/4] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome
  2019-12-27  7:21 ` [PATCH v3 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
  2019-12-27  7:21   ` [PATCH v3 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
@ 2019-12-27  7:21   ` Rocky Liao
  2019-12-27  7:21   ` [PATCH v3 4/4] Bluetooth: hci_qca: Add HCI command timeout handling Rocky Liao
  2 siblings, 0 replies; 34+ messages in thread
From: Rocky Liao @ 2019-12-27  7:21 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: linux-kernel, linux-bluetooth, linux-arm-msm, Rocky Liao

This patch registers hdev->shutdown() callback and also sets
HCI_QUIRK_NON_PERSISTENT_SETUP for QCA Rome. It will power-off the BT chip
during hci down and power-on/initialize the chip again during hci up.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---

Changes in v2: None
Changes in v3: 
- Move the quirk and callback register to probe()

 drivers/bluetooth/hci_qca.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 45042aa27fa4..ca0b38f065e5 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1532,6 +1532,7 @@ static int qca_init_regulators(struct qca_power *qca,
 static int qca_serdev_probe(struct serdev_device *serdev)
 {
 	struct qca_serdev *qcadev;
+	struct hci_dev *hdev;
 	const struct qca_vreg_data *data;
 	int err;
 
@@ -1596,8 +1597,14 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 			return err;
 
 		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
-		if (err)
+		if (err) {
 			clk_disable_unprepare(qcadev->susclk);
+			goto out;
+		}
+
+		hdev = qcadev->serdev_hu.hdev;
+		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
+		hdev->shutdown = qca_power_off;
 	}
 
 out:	return err;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v3 4/4] Bluetooth: hci_qca: Add HCI command timeout handling
  2019-12-27  7:21 ` [PATCH v3 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
  2019-12-27  7:21   ` [PATCH v3 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
  2019-12-27  7:21   ` [PATCH v3 3/4] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
@ 2019-12-27  7:21   ` Rocky Liao
  2020-01-02 19:07     ` Matthias Kaehlcke
  2 siblings, 1 reply; 34+ messages in thread
From: Rocky Liao @ 2019-12-27  7:21 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: linux-kernel, linux-bluetooth, linux-arm-msm, Rocky Liao

This patch adds the HCI command timeout handling, it will trigger btsoc
to report its memory dump via vendor specific events when hit the defined
max HCI command timeout count. After all the memory dump VSE are sent, the
btsoc will also send a HCI_HW_ERROR event to host and this will cause a new
hci down/up process and the btsoc will be re-initialized.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---

Changes in v2:
- Fix build error
Changes in v3:
- Remove the function declaration 
- Move the cmd_timeout() callback register to probe()
- Remove the redundant empty line

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

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index ca0b38f065e5..026e2e2cdd30 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -47,6 +47,8 @@
 #define IBS_HOST_TX_IDLE_TIMEOUT_MS	2000
 #define CMD_TRANS_TIMEOUT_MS		100
 
+#define QCA_BTSOC_DUMP_CMD	0xFB
+
 /* susclk rate */
 #define SUSCLK_RATE_32KHZ	32768
 
@@ -56,6 +58,9 @@
 /* max retry count when init fails */
 #define QCA_MAX_INIT_RETRY_COUNT 3
 
+/* when hit the max cmd time out count, trigger btsoc dump */
+#define QCA_MAX_CMD_TIMEOUT_COUNT 3
+
 enum qca_flags {
 	QCA_IBS_ENABLED,
 	QCA_DROP_VENDOR_EVENT,
@@ -123,6 +128,8 @@ struct qca_data {
 	u64 rx_votes_off;
 	u64 votes_on;
 	u64 votes_off;
+
+	u32 cmd_timeout_cnt;
 };
 
 enum qca_speed_type {
@@ -1332,6 +1339,11 @@ static int qca_setup(struct hci_uart *hu)
 	if (!ret) {
 		set_bit(QCA_IBS_ENABLED, &qca->flags);
 		qca_debugfs_init(hdev);
+
+		/* clear the command time out count every time after
+		 * initializaiton done
+		 */
+		qca->cmd_timeout_cnt = 0;
 	} else if (ret == -ENOENT) {
 		/* No patch/nvm-config found, run with original fw/config */
 		ret = 0;
@@ -1462,6 +1474,38 @@ static int qca_power_off(struct hci_dev *hdev)
 	return 0;
 }
 
+static int qca_send_btsoc_dump_cmd(struct hci_uart *hu)
+{
+	int err = 0;
+	struct sk_buff *skb = NULL;
+	struct qca_data *qca = hu->priv;
+
+	BT_DBG("hu %p sending btsoc dump command", hu);
+
+	skb = bt_skb_alloc(1, GFP_ATOMIC);
+	if (!skb) {
+		BT_ERR("Failed to allocate memory for qca dump command");
+		return -ENOMEM;
+	}
+
+	skb_put_u8(skb, QCA_BTSOC_DUMP_CMD);
+
+	skb_queue_tail(&qca->txq, skb);
+
+	return err;
+}
+
+static void qca_cmd_timeout(struct hci_dev *hdev)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+	struct qca_data *qca = hu->priv;
+
+	BT_ERR("hu %p hci cmd timeout count=0x%x", hu, ++qca->cmd_timeout_cnt);
+
+	if (qca->cmd_timeout_cnt >= QCA_MAX_CMD_TIMEOUT_COUNT)
+		qca_send_btsoc_dump_cmd(hu);
+}
+
 static int qca_regulator_enable(struct qca_serdev *qcadev)
 {
 	struct qca_power *power = qcadev->bt_power;
@@ -1605,6 +1649,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		hdev = qcadev->serdev_hu.hdev;
 		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
 		hdev->shutdown = qca_power_off;
+		hdev->cmd_timeout = qca_cmd_timeout;
 	}
 
 out:	return err;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails
  2019-12-27  7:21   ` [PATCH v3 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
@ 2020-01-02 18:41     ` Matthias Kaehlcke
  2020-01-03  6:31       ` rjliao
  0 siblings, 1 reply; 34+ messages in thread
From: Matthias Kaehlcke @ 2020-01-02 18:41 UTC (permalink / raw)
  To: Rocky Liao
  Cc: marcel, johan.hedberg, linux-kernel, linux-bluetooth, linux-arm-msm

Hi Rocky,

On Fri, Dec 27, 2019 at 03:21:28PM +0800, Rocky Liao wrote:
> This patch adds the retry of btsoc initialization when it fails. There are
> reports that the btsoc initialization may fail on some platforms but the
> repro ratio is very low. The failure may be caused by UART, platform HW or
> the btsoc itself but it's very difficlut to root cause, given the repro
> ratio is very low. Add a retry for the btsoc initialization will resolve
> most of the failures and make Bluetooth finally works.

Is this problem specific to a certain chipset?

What are the symptoms?

> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
> ---
> 
> Changes in v2: None
> Changes in v3: None
> 
>  drivers/bluetooth/hci_qca.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 43fd84028786..45042aa27fa4 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -53,6 +53,9 @@
>  /* Controller debug log header */
>  #define QCA_DEBUG_HANDLE	0x2EDC
>  
> +/* max retry count when init fails */
> +#define QCA_MAX_INIT_RETRY_COUNT 3

nit: MAX_RETRIES or MAX_INIT_RETRIES?

The QCA prefix just adds noise here IMO, there's no need to disambiguate
the constant from other retries since it is defined in hci_qca.c.

> +
>  enum qca_flags {
>  	QCA_IBS_ENABLED,
>  	QCA_DROP_VENDOR_EVENT,
> @@ -1257,7 +1260,9 @@ static int qca_setup(struct hci_uart *hu)
>  {
>  	struct hci_dev *hdev = hu->hdev;
>  	struct qca_data *qca = hu->priv;
> +	struct qca_serdev *qcadev;
>  	unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
> +	unsigned int init_retry_count = 0;

nit: the name is a bit clunky, how about 'retries'?

>  	enum qca_btsoc_type soc_type = qca_soc_type(hu);
>  	const char *firmware_name = qca_get_firmware_name(hu);
>  	int ret;
> @@ -1275,6 +1280,7 @@ static int qca_setup(struct hci_uart *hu)
>  	 */
>  	set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>  
> +retry:
>  	if (qca_is_wcn399x(soc_type)) {
>  		bt_dev_info(hdev, "setting up wcn3990");
>  
> @@ -1293,6 +1299,12 @@ static int qca_setup(struct hci_uart *hu)
>  			return ret;
>  	} else {
>  		bt_dev_info(hdev, "ROME setup");
> +		if (hu->serdev) {
> +			qcadev = serdev_device_get_drvdata(hu->serdev);
> +			gpiod_set_value_cansleep(qcadev->bt_en, 1);
> +			/* Controller needs time to bootup. */
> +			msleep(150);

Shouldn't this be in qca_power_on(), analogous to the power off code from
"[1/4]Bluetooth: hci_qca: Add QCA Rome power off support to the
qca_power_off()"?

qca_power_on() should then also be called for ROME. If you opt for this it
should be done in a separate patch, or possibly merged into the one
mentioned above.

> +		}
>  		qca_set_speed(hu, QCA_INIT_SPEED);
>  	}
>  
> @@ -1329,6 +1341,20 @@ static int qca_setup(struct hci_uart *hu)
>  		 * patch/nvm-config is found, so run with original fw/config.
>  		 */
>  		ret = 0;
> +	} else {
> +		if (init_retry_count < QCA_MAX_INIT_RETRY_COUNT) {
> +			qca_power_off(hdev);
> +			if (hu->serdev) {
> +				serdev_device_close(hu->serdev);
> +				ret = serdev_device_open(hu->serdev);
> +				if (ret) {
> +					bt_dev_err(hu->hdev, "open port fail");

nit: "failed to open port"

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3 4/4] Bluetooth: hci_qca: Add HCI command timeout handling
  2019-12-27  7:21   ` [PATCH v3 4/4] Bluetooth: hci_qca: Add HCI command timeout handling Rocky Liao
@ 2020-01-02 19:07     ` Matthias Kaehlcke
  2020-01-03  6:33       ` rjliao
  0 siblings, 1 reply; 34+ messages in thread
From: Matthias Kaehlcke @ 2020-01-02 19:07 UTC (permalink / raw)
  To: Rocky Liao
  Cc: marcel, johan.hedberg, linux-kernel, linux-bluetooth, linux-arm-msm

Hi Rocky,

On Fri, Dec 27, 2019 at 03:21:30PM +0800, Rocky Liao wrote:
> This patch adds the HCI command timeout handling, it will trigger btsoc
> to report its memory dump via vendor specific events when hit the defined
> max HCI command timeout count. After all the memory dump VSE are sent, the
> btsoc will also send a HCI_HW_ERROR event to host and this will cause a new
> hci down/up process and the btsoc will be re-initialized.
> 
> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
> ---
> 
> Changes in v2:
> - Fix build error
> Changes in v3:
> - Remove the function declaration 
> - Move the cmd_timeout() callback register to probe()
> - Remove the redundant empty line
> 
>  drivers/bluetooth/hci_qca.c | 45 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index ca0b38f065e5..026e2e2cdd30 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -47,6 +47,8 @@
>  #define IBS_HOST_TX_IDLE_TIMEOUT_MS	2000
>  #define CMD_TRANS_TIMEOUT_MS		100
>  
> +#define QCA_BTSOC_DUMP_CMD	0xFB
> +
>  /* susclk rate */
>  #define SUSCLK_RATE_32KHZ	32768
>  
> @@ -56,6 +58,9 @@
>  /* max retry count when init fails */
>  #define QCA_MAX_INIT_RETRY_COUNT 3
>  
> +/* when hit the max cmd time out count, trigger btsoc dump */
> +#define QCA_MAX_CMD_TIMEOUT_COUNT 3

nit: MAX_CMD_TIMEOUTS?

Similar to QCA_MAX_INIT_RETRY_COUNT on which I commented earlier I don't
think the 'QCA' prefix adds value here. The constant is defined in the driver
itself and isn't related to hardware.

> +
>  enum qca_flags {
>  	QCA_IBS_ENABLED,
>  	QCA_DROP_VENDOR_EVENT,
> @@ -123,6 +128,8 @@ struct qca_data {
>  	u64 rx_votes_off;
>  	u64 votes_on;
>  	u64 votes_off;
> +
> +	u32 cmd_timeout_cnt;

nit: cmd_timeouts?

>  };
>  
>  enum qca_speed_type {
> @@ -1332,6 +1339,11 @@ static int qca_setup(struct hci_uart *hu)
>  	if (!ret) {
>  		set_bit(QCA_IBS_ENABLED, &qca->flags);
>  		qca_debugfs_init(hdev);
> +
> +		/* clear the command time out count every time after
> +		 * initializaiton done
> +		 */
> +		qca->cmd_timeout_cnt = 0;
>  	} else if (ret == -ENOENT) {
>  		/* No patch/nvm-config found, run with original fw/config */
>  		ret = 0;
> @@ -1462,6 +1474,38 @@ static int qca_power_off(struct hci_dev *hdev)
>  	return 0;
>  }
>  
> +static int qca_send_btsoc_dump_cmd(struct hci_uart *hu)
> +{
> +	int err = 0;

The variable is pointless, just return 0 at the end of the function.

> +	struct sk_buff *skb = NULL;
> +	struct qca_data *qca = hu->priv;
> +
> +	BT_DBG("hu %p sending btsoc dump command", hu);
> +
> +	skb = bt_skb_alloc(1, GFP_ATOMIC);
> +	if (!skb) {
> +		BT_ERR("Failed to allocate memory for qca dump command");

"These generic allocation functions all emit a stack dump on failure when used
without __GFP_NOWARN so there is no use in emitting an additional failure
message when NULL is returned."

Documentation/process/coding-style.rst

hence the logging is redundant, drop it.

> +		return -ENOMEM;
> +	}
> +
> +	skb_put_u8(skb, QCA_BTSOC_DUMP_CMD);
> +
> +	skb_queue_tail(&qca->txq, skb);
> +
> +	return err;
> +}
> +
> +static void qca_cmd_timeout(struct hci_dev *hdev)
> +{
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +	struct qca_data *qca = hu->priv;
> +
> +	BT_ERR("hu %p hci cmd timeout count=0x%x", hu, ++qca->cmd_timeout_cnt);

Is there any particular reason to print the counter in hex instead of
decimal?

Should this use bt_dev_err() since we have a hdev in this context?

> +
> +	if (qca->cmd_timeout_cnt >= QCA_MAX_CMD_TIMEOUT_COUNT)
> +		qca_send_btsoc_dump_cmd(hu);
> +}
> +
>  static int qca_regulator_enable(struct qca_serdev *qcadev)
>  {
>  	struct qca_power *power = qcadev->bt_power;
> @@ -1605,6 +1649,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  		hdev = qcadev->serdev_hu.hdev;
>  		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
>  		hdev->shutdown = qca_power_off;
> +		hdev->cmd_timeout = qca_cmd_timeout;
>  	}
>  
>  out:	return err;
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails
  2020-01-02 18:41     ` Matthias Kaehlcke
@ 2020-01-03  6:31       ` rjliao
  2020-01-03 16:27         ` Matthias Kaehlcke
  0 siblings, 1 reply; 34+ messages in thread
From: rjliao @ 2020-01-03  6:31 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: marcel, johan.hedberg, linux-kernel, linux-bluetooth,
	linux-arm-msm, linux-bluetooth-owner

在 2020-01-03 02:41,Matthias Kaehlcke 写道:

> Hi Rocky,
> 
> On Fri, Dec 27, 2019 at 03:21:28PM +0800, Rocky Liao wrote:
> 
>> This patch adds the retry of btsoc initialization when it fails. There 
>> are
>> reports that the btsoc initialization may fail on some platforms but 
>> the
>> repro ratio is very low. The failure may be caused by UART, platform 
>> HW or
>> the btsoc itself but it's very difficlut to root cause, given the 
>> repro
>> ratio is very low. Add a retry for the btsoc initialization will 
>> resolve
>> most of the failures and make Bluetooth finally works.
> 
> Is this problem specific to a certain chipset?
> 
> What are the symptoms?

It's reported on Rome so far but I think the patch is potentially 
helpful for
wcn399x as well.

The symptoms is the firmware downloading failed due to the UART write 
timed out.

>> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
>> ---
>> 
>> Changes in v2: None
>> Changes in v3: None
>> 
>> drivers/bluetooth/hci_qca.c | 26 ++++++++++++++++++++++++++
>> 1 file changed, 26 insertions(+)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 43fd84028786..45042aa27fa4 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -53,6 +53,9 @@
>> /* Controller debug log header */
>> #define QCA_DEBUG_HANDLE    0x2EDC
>> 
>> +/* max retry count when init fails */
>> +#define QCA_MAX_INIT_RETRY_COUNT 3
> 
> nit: MAX_RETRIES or MAX_INIT_RETRIES?
> 
> The QCA prefix just adds noise here IMO, there's no need to 
> disambiguate
> the constant from other retries since it is defined in hci_qca.c.
> 
>> +
>> enum qca_flags {
>> QCA_IBS_ENABLED,
>> QCA_DROP_VENDOR_EVENT,
>> @@ -1257,7 +1260,9 @@ static int qca_setup(struct hci_uart *hu)
>> {
>> struct hci_dev *hdev = hu->hdev;
>> struct qca_data *qca = hu->priv;
>> +    struct qca_serdev *qcadev;
>> unsigned int speed, qca_baudrate = QCA_BAUDRATE_115200;
>> +    unsigned int init_retry_count = 0;
> 
> nit: the name is a bit clunky, how about 'retries'?
> 
>> enum qca_btsoc_type soc_type = qca_soc_type(hu);
>> const char *firmware_name = qca_get_firmware_name(hu);
>> int ret;
>> @@ -1275,6 +1280,7 @@ static int qca_setup(struct hci_uart *hu)
>> */
>> set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>> 
>> +retry:
>> if (qca_is_wcn399x(soc_type)) {
>> bt_dev_info(hdev, "setting up wcn3990");
>> 
>> @@ -1293,6 +1299,12 @@ static int qca_setup(struct hci_uart *hu)
>> return ret;
>> } else {
>> bt_dev_info(hdev, "ROME setup");
>> +        if (hu->serdev) {
>> +            qcadev = serdev_device_get_drvdata(hu->serdev);
>> +            gpiod_set_value_cansleep(qcadev->bt_en, 1);
>> +            /* Controller needs time to bootup. */
>> +            msleep(150);
> 
> Shouldn't this be in qca_power_on(), analogous to the power off code 
> from
> "[1/4]Bluetooth: hci_qca: Add QCA Rome power off support to the
> qca_power_off()"?
> 
> qca_power_on() should then also be called for ROME. If you opt for this 
> it
> should be done in a separate patch, or possibly merged into the one
> mentioned above.
> 

There is no qca_power_on() func and wcn399x is calling 
qca_wcn3990_init() to
do power on, I prefer to not do this change this time. If it's needed it 
should
be a new patch to add qca_power_on() which supports both Rome and 
wcn399x.

>> +        }
>> qca_set_speed(hu, QCA_INIT_SPEED);
>> }
>> 
>> @@ -1329,6 +1341,20 @@ static int qca_setup(struct hci_uart *hu)
>> * patch/nvm-config is found, so run with original fw/config.
>> */
>> ret = 0;
>> +    } else {
>> +        if (init_retry_count < QCA_MAX_INIT_RETRY_COUNT) {
>> +            qca_power_off(hdev);
>> +            if (hu->serdev) {
>> +                serdev_device_close(hu->serdev);
>> +                ret = serdev_device_open(hu->serdev);
>> +                if (ret) {
>> +                    bt_dev_err(hu->hdev, "open port fail");
> 
> nit: "failed to open port"

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3 4/4] Bluetooth: hci_qca: Add HCI command timeout handling
  2020-01-02 19:07     ` Matthias Kaehlcke
@ 2020-01-03  6:33       ` rjliao
  2020-01-03  9:02         ` Rocky Liao
  0 siblings, 1 reply; 34+ messages in thread
From: rjliao @ 2020-01-03  6:33 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: marcel, johan.hedberg, linux-kernel, linux-bluetooth, linux-arm-msm

在 2020-01-03 03:07,Matthias Kaehlcke 写道:
> Hi Rocky,
> 
> On Fri, Dec 27, 2019 at 03:21:30PM +0800, Rocky Liao wrote:
>> This patch adds the HCI command timeout handling, it will trigger 
>> btsoc
>> to report its memory dump via vendor specific events when hit the 
>> defined
>> max HCI command timeout count. After all the memory dump VSE are sent, 
>> the
>> btsoc will also send a HCI_HW_ERROR event to host and this will cause 
>> a new
>> hci down/up process and the btsoc will be re-initialized.
>> 
>> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
>> ---
>> 
>> Changes in v2:
>> - Fix build error
>> Changes in v3:
>> - Remove the function declaration
>> - Move the cmd_timeout() callback register to probe()
>> - Remove the redundant empty line
>> 
>>  drivers/bluetooth/hci_qca.c | 45 
>> +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 45 insertions(+)
>> 
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index ca0b38f065e5..026e2e2cdd30 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -47,6 +47,8 @@
>>  #define IBS_HOST_TX_IDLE_TIMEOUT_MS	2000
>>  #define CMD_TRANS_TIMEOUT_MS		100
>> 
>> +#define QCA_BTSOC_DUMP_CMD	0xFB
>> +
>>  /* susclk rate */
>>  #define SUSCLK_RATE_32KHZ	32768
>> 
>> @@ -56,6 +58,9 @@
>>  /* max retry count when init fails */
>>  #define QCA_MAX_INIT_RETRY_COUNT 3
>> 
>> +/* when hit the max cmd time out count, trigger btsoc dump */
>> +#define QCA_MAX_CMD_TIMEOUT_COUNT 3
> 
> nit: MAX_CMD_TIMEOUTS?
> 
> Similar to QCA_MAX_INIT_RETRY_COUNT on which I commented earlier I 
> don't
> think the 'QCA' prefix adds value here. The constant is defined in the 
> driver
> itself and isn't related to hardware.
> 

OK

>> +
>>  enum qca_flags {
>>  	QCA_IBS_ENABLED,
>>  	QCA_DROP_VENDOR_EVENT,
>> @@ -123,6 +128,8 @@ struct qca_data {
>>  	u64 rx_votes_off;
>>  	u64 votes_on;
>>  	u64 votes_off;
>> +
>> +	u32 cmd_timeout_cnt;
> 
> nit: cmd_timeouts?
> 
>>  };
>> 
>>  enum qca_speed_type {
>> @@ -1332,6 +1339,11 @@ static int qca_setup(struct hci_uart *hu)
>>  	if (!ret) {
>>  		set_bit(QCA_IBS_ENABLED, &qca->flags);
>>  		qca_debugfs_init(hdev);
>> +
>> +		/* clear the command time out count every time after
>> +		 * initializaiton done
>> +		 */
>> +		qca->cmd_timeout_cnt = 0;
>>  	} else if (ret == -ENOENT) {
>>  		/* No patch/nvm-config found, run with original fw/config */
>>  		ret = 0;
>> @@ -1462,6 +1474,38 @@ static int qca_power_off(struct hci_dev *hdev)
>>  	return 0;
>>  }
>> 
>> +static int qca_send_btsoc_dump_cmd(struct hci_uart *hu)
>> +{
>> +	int err = 0;
> 
> The variable is pointless, just return 0 at the end of the function.
> 
OK

>> +	struct sk_buff *skb = NULL;
>> +	struct qca_data *qca = hu->priv;
>> +
>> +	BT_DBG("hu %p sending btsoc dump command", hu);
>> +
>> +	skb = bt_skb_alloc(1, GFP_ATOMIC);
>> +	if (!skb) {
>> +		BT_ERR("Failed to allocate memory for qca dump command");
> 
> "These generic allocation functions all emit a stack dump on failure 
> when used
> without __GFP_NOWARN so there is no use in emitting an additional 
> failure
> message when NULL is returned."
> 
> Documentation/process/coding-style.rst
> 
> hence the logging is redundant, drop it.
> 

OK

>> +		return -ENOMEM;
>> +	}
>> +
>> +	skb_put_u8(skb, QCA_BTSOC_DUMP_CMD);
>> +
>> +	skb_queue_tail(&qca->txq, skb);
>> +
>> +	return err;
>> +}
>> +
>> +static void qca_cmd_timeout(struct hci_dev *hdev)
>> +{
>> +	struct hci_uart *hu = hci_get_drvdata(hdev);
>> +	struct qca_data *qca = hu->priv;
>> +
>> +	BT_ERR("hu %p hci cmd timeout count=0x%x", hu, 
>> ++qca->cmd_timeout_cnt);
> 
> Is there any particular reason to print the counter in hex instead of
> decimal?
> 
> Should this use bt_dev_err() since we have a hdev in this context?
> 

OK

>> +
>> +	if (qca->cmd_timeout_cnt >= QCA_MAX_CMD_TIMEOUT_COUNT)
>> +		qca_send_btsoc_dump_cmd(hu);
>> +}
>> +
>>  static int qca_regulator_enable(struct qca_serdev *qcadev)
>>  {
>>  	struct qca_power *power = qcadev->bt_power;
>> @@ -1605,6 +1649,7 @@ static int qca_serdev_probe(struct serdev_device 
>> *serdev)
>>  		hdev = qcadev->serdev_hu.hdev;
>>  		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
>>  		hdev->shutdown = qca_power_off;
>> +		hdev->cmd_timeout = qca_cmd_timeout;
>>  	}
>> 
>>  out:	return err;
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3 4/4] Bluetooth: hci_qca: Add HCI command timeout handling
  2020-01-03  6:33       ` rjliao
@ 2020-01-03  9:02         ` Rocky Liao
  0 siblings, 0 replies; 34+ messages in thread
From: Rocky Liao @ 2020-01-03  9:02 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: marcel, johan.hedberg, linux-kernel, linux-bluetooth,
	linux-arm-msm, linux-bluetooth-owner

在 2020-01-03 14:33,rjliao@codeaurora.org 写道:
> 在 2020-01-03 03:07,Matthias Kaehlcke 写道:
>> Hi Rocky,
>> 
>> On Fri, Dec 27, 2019 at 03:21:30PM +0800, Rocky Liao wrote:
>>> This patch adds the HCI command timeout handling, it will trigger 
>>> btsoc
>>> to report its memory dump via vendor specific events when hit the 
>>> defined
>>> max HCI command timeout count. After all the memory dump VSE are 
>>> sent, the
>>> btsoc will also send a HCI_HW_ERROR event to host and this will cause 
>>> a new
>>> hci down/up process and the btsoc will be re-initialized.
>>> 
>>> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
>>> ---
>>> 
>>> Changes in v2:
>>> - Fix build error
>>> Changes in v3:
>>> - Remove the function declaration
>>> - Move the cmd_timeout() callback register to probe()
>>> - Remove the redundant empty line
>>> 
>>>  drivers/bluetooth/hci_qca.c | 45 
>>> +++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 45 insertions(+)
>>> 
>>> diff --git a/drivers/bluetooth/hci_qca.c 
>>> b/drivers/bluetooth/hci_qca.c
>>> index ca0b38f065e5..026e2e2cdd30 100644
>>> --- a/drivers/bluetooth/hci_qca.c
>>> +++ b/drivers/bluetooth/hci_qca.c
>>> @@ -47,6 +47,8 @@
>>>  #define IBS_HOST_TX_IDLE_TIMEOUT_MS	2000
>>>  #define CMD_TRANS_TIMEOUT_MS		100
>>> 
>>> +#define QCA_BTSOC_DUMP_CMD	0xFB
>>> +
>>>  /* susclk rate */
>>>  #define SUSCLK_RATE_32KHZ	32768
>>> 
>>> @@ -56,6 +58,9 @@
>>>  /* max retry count when init fails */
>>>  #define QCA_MAX_INIT_RETRY_COUNT 3
>>> 
>>> +/* when hit the max cmd time out count, trigger btsoc dump */
>>> +#define QCA_MAX_CMD_TIMEOUT_COUNT 3
>> 
>> nit: MAX_CMD_TIMEOUTS?
>> 
>> Similar to QCA_MAX_INIT_RETRY_COUNT on which I commented earlier I 
>> don't
>> think the 'QCA' prefix adds value here. The constant is defined in the 
>> driver
>> itself and isn't related to hardware.
>> 
> 
> OK
> 
>>> +
>>>  enum qca_flags {
>>>  	QCA_IBS_ENABLED,
>>>  	QCA_DROP_VENDOR_EVENT,
>>> @@ -123,6 +128,8 @@ struct qca_data {
>>>  	u64 rx_votes_off;
>>>  	u64 votes_on;
>>>  	u64 votes_off;
>>> +
>>> +	u32 cmd_timeout_cnt;
>> 
>> nit: cmd_timeouts?
>> 

btusb is also using cmd_timeout_cnt, prefer to align with that

>>>  };
>>> 
>>>  enum qca_speed_type {
>>> @@ -1332,6 +1339,11 @@ static int qca_setup(struct hci_uart *hu)
>>>  	if (!ret) {
>>>  		set_bit(QCA_IBS_ENABLED, &qca->flags);
>>>  		qca_debugfs_init(hdev);
>>> +
>>> +		/* clear the command time out count every time after
>>> +		 * initializaiton done
>>> +		 */
>>> +		qca->cmd_timeout_cnt = 0;
>>>  	} else if (ret == -ENOENT) {
>>>  		/* No patch/nvm-config found, run with original fw/config */
>>>  		ret = 0;
>>> @@ -1462,6 +1474,38 @@ static int qca_power_off(struct hci_dev *hdev)
>>>  	return 0;
>>>  }
>>> 
>>> +static int qca_send_btsoc_dump_cmd(struct hci_uart *hu)
>>> +{
>>> +	int err = 0;
>> 
>> The variable is pointless, just return 0 at the end of the function.
>> 
> OK
> 
>>> +	struct sk_buff *skb = NULL;
>>> +	struct qca_data *qca = hu->priv;
>>> +
>>> +	BT_DBG("hu %p sending btsoc dump command", hu);
>>> +
>>> +	skb = bt_skb_alloc(1, GFP_ATOMIC);
>>> +	if (!skb) {
>>> +		BT_ERR("Failed to allocate memory for qca dump command");
>> 
>> "These generic allocation functions all emit a stack dump on failure 
>> when used
>> without __GFP_NOWARN so there is no use in emitting an additional 
>> failure
>> message when NULL is returned."
>> 
>> Documentation/process/coding-style.rst
>> 
>> hence the logging is redundant, drop it.
>> 
> 
> OK
> 
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	skb_put_u8(skb, QCA_BTSOC_DUMP_CMD);
>>> +
>>> +	skb_queue_tail(&qca->txq, skb);
>>> +
>>> +	return err;
>>> +}
>>> +
>>> +static void qca_cmd_timeout(struct hci_dev *hdev)
>>> +{
>>> +	struct hci_uart *hu = hci_get_drvdata(hdev);
>>> +	struct qca_data *qca = hu->priv;
>>> +
>>> +	BT_ERR("hu %p hci cmd timeout count=0x%x", hu, 
>>> ++qca->cmd_timeout_cnt);
>> 
>> Is there any particular reason to print the counter in hex instead of
>> decimal?
>> 
>> Should this use bt_dev_err() since we have a hdev in this context?
>> 
> 
> OK
> 
>>> +
>>> +	if (qca->cmd_timeout_cnt >= QCA_MAX_CMD_TIMEOUT_COUNT)
>>> +		qca_send_btsoc_dump_cmd(hu);
>>> +}
>>> +
>>>  static int qca_regulator_enable(struct qca_serdev *qcadev)
>>>  {
>>>  	struct qca_power *power = qcadev->bt_power;
>>> @@ -1605,6 +1649,7 @@ static int qca_serdev_probe(struct 
>>> serdev_device *serdev)
>>>  		hdev = qcadev->serdev_hu.hdev;
>>>  		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
>>>  		hdev->shutdown = qca_power_off;
>>> +		hdev->cmd_timeout = qca_cmd_timeout;
>>>  	}
>>> 
>>>  out:	return err;
>>> --
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>>> Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails
  2020-01-03  6:31       ` rjliao
@ 2020-01-03 16:27         ` Matthias Kaehlcke
  2020-01-07  5:34           ` Rocky Liao
  0 siblings, 1 reply; 34+ messages in thread
From: Matthias Kaehlcke @ 2020-01-03 16:27 UTC (permalink / raw)
  To: rjliao
  Cc: marcel, johan.hedberg, linux-kernel, linux-bluetooth,
	linux-arm-msm, linux-bluetooth-owner

On Fri, Jan 03, 2020 at 02:31:46PM +0800, rjliao@codeaurora.org wrote:
> 在 2020-01-03 02:41,Matthias Kaehlcke 写道:
> 
> > Hi Rocky,
> > 
> > On Fri, Dec 27, 2019 at 03:21:28PM +0800, Rocky Liao wrote:
> > 
> > > This patch adds the retry of btsoc initialization when it fails.
> > > There are
> > > reports that the btsoc initialization may fail on some platforms but
> > > the
> > > repro ratio is very low. The failure may be caused by UART, platform
> > > HW or
> > > the btsoc itself but it's very difficlut to root cause, given the
> > > repro
> > > ratio is very low. Add a retry for the btsoc initialization will
> > > resolve
> > > most of the failures and make Bluetooth finally works.
> > 
> > Is this problem specific to a certain chipset?
> > 
> > What are the symptoms?
> 
> It's reported on Rome so far but I think the patch is potentially helpful
> for
> wcn399x as well.
> 
> The symptoms is the firmware downloading failed due to the UART write timed
> out.

Working around this with retries seems ok for now if the repro rate is
really low, but it shouldn't necessarily be interpreted as "the problem is
fixed". Please mention the symptoms in the commit message for documentation,
then the retries can potentially be removed in the futures when the root
cause is fixed.

> > > enum qca_btsoc_type soc_type = qca_soc_type(hu);
> > > const char *firmware_name = qca_get_firmware_name(hu);
> > > int ret;
> > > @@ -1275,6 +1280,7 @@ static int qca_setup(struct hci_uart *hu)
> > > */
> > > set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
> > > 
> > > +retry:
> > > if (qca_is_wcn399x(soc_type)) {
> > > bt_dev_info(hdev, "setting up wcn3990");
> > > 
> > > @@ -1293,6 +1299,12 @@ static int qca_setup(struct hci_uart *hu)
> > > return ret;
> > > } else {
> > > bt_dev_info(hdev, "ROME setup");
> > > +        if (hu->serdev) {
> > > +            qcadev = serdev_device_get_drvdata(hu->serdev);
> > > +            gpiod_set_value_cansleep(qcadev->bt_en, 1);
> > > +            /* Controller needs time to bootup. */
> > > +            msleep(150);
> > 
> > Shouldn't this be in qca_power_on(), analogous to the power off code
> > from
> > "[1/4]Bluetooth: hci_qca: Add QCA Rome power off support to the
> > qca_power_off()"?
> > 
> > qca_power_on() should then also be called for ROME. If you opt for this
> > it
> > should be done in a separate patch, or possibly merged into the one
> > mentioned above.
> > 
> 
> There is no qca_power_on() func and wcn399x is calling qca_wcn3990_init() to
> do power on, I prefer to not do this change this time.

I would say it's precisely the right time to add this function. Patch 1 of this
series adds handling of the BT_EN GPIO to qca_power_off(), now this patch
duplicates the code of the BT_EN handling in qca_open().

> If it's needed

'needed' is a relative term. It certainly isn't needed from a purely functional
POV. However it is desirable for encapsulation and to avoid code duplication.

> it should be a new patch to add qca_power_on() which supports both Rome and wcn399x.

Agreed

m.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v3 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails
  2020-01-03 16:27         ` Matthias Kaehlcke
@ 2020-01-07  5:34           ` Rocky Liao
  0 siblings, 0 replies; 34+ messages in thread
From: Rocky Liao @ 2020-01-07  5:34 UTC (permalink / raw)
  To: Matthias Kaehlcke
  Cc: marcel, johan.hedberg, linux-kernel, linux-bluetooth,
	linux-arm-msm, linux-bluetooth-owner

在 2020-01-04 00:27,Matthias Kaehlcke 写道:
> On Fri, Jan 03, 2020 at 02:31:46PM +0800, rjliao@codeaurora.org wrote:
>> 在 2020-01-03 02:41,Matthias Kaehlcke 写道:
>> 
>> > Hi Rocky,
>> >
>> > On Fri, Dec 27, 2019 at 03:21:28PM +0800, Rocky Liao wrote:
>> >
>> > > This patch adds the retry of btsoc initialization when it fails.
>> > > There are
>> > > reports that the btsoc initialization may fail on some platforms but
>> > > the
>> > > repro ratio is very low. The failure may be caused by UART, platform
>> > > HW or
>> > > the btsoc itself but it's very difficlut to root cause, given the
>> > > repro
>> > > ratio is very low. Add a retry for the btsoc initialization will
>> > > resolve
>> > > most of the failures and make Bluetooth finally works.
>> >
>> > Is this problem specific to a certain chipset?
>> >
>> > What are the symptoms?
>> 
>> It's reported on Rome so far but I think the patch is potentially 
>> helpful
>> for
>> wcn399x as well.
>> 
>> The symptoms is the firmware downloading failed due to the UART write 
>> timed
>> out.
> 
> Working around this with retries seems ok for now if the repro rate is
> really low, but it shouldn't necessarily be interpreted as "the problem 
> is
> fixed". Please mention the symptoms in the commit message for 
> documentation,
> then the retries can potentially be removed in the futures when the 
> root
> cause is fixed.
> 
>> > > enum qca_btsoc_type soc_type = qca_soc_type(hu);
>> > > const char *firmware_name = qca_get_firmware_name(hu);
>> > > int ret;
>> > > @@ -1275,6 +1280,7 @@ static int qca_setup(struct hci_uart *hu)
>> > > */
>> > > set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>> > >
>> > > +retry:
>> > > if (qca_is_wcn399x(soc_type)) {
>> > > bt_dev_info(hdev, "setting up wcn3990");
>> > >
>> > > @@ -1293,6 +1299,12 @@ static int qca_setup(struct hci_uart *hu)
>> > > return ret;
>> > > } else {
>> > > bt_dev_info(hdev, "ROME setup");
>> > > +        if (hu->serdev) {
>> > > +            qcadev = serdev_device_get_drvdata(hu->serdev);
>> > > +            gpiod_set_value_cansleep(qcadev->bt_en, 1);
>> > > +            /* Controller needs time to bootup. */
>> > > +            msleep(150);
>> >
>> > Shouldn't this be in qca_power_on(), analogous to the power off code
>> > from
>> > "[1/4]Bluetooth: hci_qca: Add QCA Rome power off support to the
>> > qca_power_off()"?
>> >
>> > qca_power_on() should then also be called for ROME. If you opt for this
>> > it
>> > should be done in a separate patch, or possibly merged into the one
>> > mentioned above.
>> >
>> 
>> There is no qca_power_on() func and wcn399x is calling 
>> qca_wcn3990_init() to
>> do power on, I prefer to not do this change this time.
> 
> I would say it's precisely the right time to add this function. Patch 1 
> of this
> series adds handling of the BT_EN GPIO to qca_power_off(), now this 
> patch
> duplicates the code of the BT_EN handling in qca_open().
> 
>> If it's needed
> 
> 'needed' is a relative term. It certainly isn't needed from a purely 
> functional
> POV. However it is desirable for encapsulation and to avoid code 
> duplication.
> 
>> it should be a new patch to add qca_power_on() which supports both 
>> Rome and wcn399x.
> 
> Agreed
> 

Have sent out a new patch to add the qca_power_on() API, will refresh 
this patch after that new patch merged.

> m.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 1/3] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_shutdown()
  2019-12-25  6:03 [PATCH v1 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
                   ` (4 preceding siblings ...)
  2019-12-27  7:21 ` [PATCH v3 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
@ 2020-01-15  8:55 ` Rocky Liao
  2020-01-15  8:55   ` [PATCH v4 2/3] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
                     ` (3 more replies)
  2020-01-16  3:22 ` [PATCH v5] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
  6 siblings, 4 replies; 34+ messages in thread
From: Rocky Liao @ 2020-01-15  8:55 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: mka, linux-kernel, linux-bluetooth, linux-arm-msm, bgodavar,
	hemantg, Rocky Liao

Current qca_power_shutdown() only supports wcn399x, this patch adds Rome
power off support to it. For Rome it just needs to pull down the bt_en
GPIO to power off it. This patch also replaces all the power off operation
in qca_close() with the unified qca_power_shutdown() call.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---

Changes in v2: None
Changes in v3: None
Changes in v4:
  -rebased the patch with latest code base
  -moved the change from qca_power_off() to qca_power_shutdown()
  -replaced all the power off operation in qca_close() with
   qca_power_shutdown()
  -updated commit message

 drivers/bluetooth/hci_qca.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 992622dc1263..ecb74965be10 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -663,7 +663,6 @@ static int qca_flush(struct hci_uart *hu)
 /* Close protocol */
 static int qca_close(struct hci_uart *hu)
 {
-	struct qca_serdev *qcadev;
 	struct qca_data *qca = hu->priv;
 
 	BT_DBG("hu %p qca close", hu);
@@ -679,14 +678,7 @@ static int qca_close(struct hci_uart *hu)
 	destroy_workqueue(qca->workqueue);
 	qca->hu = NULL;
 
-	if (hu->serdev) {
-		qcadev = serdev_device_get_drvdata(hu->serdev);
-		if (qca_is_wcn399x(qcadev->btsoc_type))
-			qca_power_shutdown(hu);
-		else
-			gpiod_set_value_cansleep(qcadev->bt_en, 0);
-
-	}
+	qca_power_shutdown(hu);
 
 	kfree_skb(qca->rx_skb);
 
@@ -1685,6 +1677,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
 	struct qca_serdev *qcadev;
 	struct qca_data *qca = hu->priv;
 	unsigned long flags;
+	enum qca_btsoc_type soc_type = qca_soc_type(hu);
 
 	qcadev = serdev_device_get_drvdata(hu->serdev);
 
@@ -1697,11 +1690,22 @@ static void qca_power_shutdown(struct hci_uart *hu)
 	qca_flush(hu);
 	spin_unlock_irqrestore(&qca->hci_ibs_lock, flags);
 
-	host_set_baudrate(hu, 2400);
-	qca_send_power_pulse(hu, false);
-	qca_regulator_disable(qcadev);
 	hu->hdev->hw_error = NULL;
 	hu->hdev->cmd_timeout = NULL;
+
+	/* Non-serdev device usually is powered by external power
+	 * and don't need additional action in driver for power down
+	 */
+	if (!hu->serdev)
+		return;
+
+	if (qca_is_wcn399x(soc_type)) {
+		host_set_baudrate(hu, 2400);
+		qca_send_power_pulse(hu, false);
+		qca_regulator_disable(qcadev);
+	} else {
+		gpiod_set_value_cansleep(qcadev->bt_en, 0);
+	}
 }
 
 static int qca_power_off(struct hci_dev *hdev)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 2/3] Bluetooth: hci_qca: Retry btsoc initialize when it fails
  2020-01-15  8:55 ` [PATCH v4 1/3] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_shutdown() Rocky Liao
@ 2020-01-15  8:55   ` Rocky Liao
  2020-01-15 17:33     ` Matthias Kaehlcke
  2020-01-15 21:37     ` Marcel Holtmann
  2020-01-15  8:55   ` [PATCH v4 3/3] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
                     ` (2 subsequent siblings)
  3 siblings, 2 replies; 34+ messages in thread
From: Rocky Liao @ 2020-01-15  8:55 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: mka, linux-kernel, linux-bluetooth, linux-arm-msm, bgodavar,
	hemantg, Rocky Liao

This patch adds the retry of btsoc initialization when it fails. There are
reports that the btsoc initialization may fail on some platforms but the
repro ratio is very low. The symptoms is the firmware downloading failed
due to the UART write timed out. The failure may be caused by UART,
platform HW or the btsoc itself but it's very difficlut to root cause,
given the repro ratio is very low. Add a retry for the btsoc initialization
can work around most of the failures and make Bluetooth finally works.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---

Changes in v2: None
Changes in v3: None
Changes in v4:
  -rebased the patch with latet code
  -refined macro and variable name
  -updated commit message

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

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index ecb74965be10..1139142e8eed 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -55,6 +55,9 @@
 /* Controller debug log header */
 #define QCA_DEBUG_HANDLE	0x2EDC
 
+/* max retry count when init fails */
+#define MAX_INIT_RETRIES 3
+
 /* Controller dump header */
 #define QCA_SSR_DUMP_HANDLE		0x0108
 #define QCA_DUMP_PACKET_SIZE		255
@@ -1539,6 +1542,7 @@ 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;
+	unsigned int retries = 0;
 	enum qca_btsoc_type soc_type = qca_soc_type(hu);
 	const char *firmware_name = qca_get_firmware_name(hu);
 	int ret;
@@ -1559,6 +1563,7 @@ static int qca_setup(struct hci_uart *hu)
 	bt_dev_info(hdev, "setting up %s",
 		qca_is_wcn399x(soc_type) ? "wcn399x" : "ROME");
 
+retry:
 	ret = qca_power_on(hdev);
 	if (ret)
 		return ret;
@@ -1613,6 +1618,20 @@ static int qca_setup(struct hci_uart *hu)
 		 * patch/nvm-config is found, so run with original fw/config.
 		 */
 		ret = 0;
+	} else {
+		if (retries < MAX_INIT_RETRIES) {
+			qca_power_shutdown(hu);
+			if (hu->serdev) {
+				serdev_device_close(hu->serdev);
+				ret = serdev_device_open(hu->serdev);
+				if (ret) {
+					bt_dev_err(hdev, "failed to open port");
+					return ret;
+				}
+			}
+			retries++;
+			goto retry;
+		}
 	}
 
 	/* Setup bdaddr */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v4 3/3] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome
  2020-01-15  8:55 ` [PATCH v4 1/3] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_shutdown() Rocky Liao
  2020-01-15  8:55   ` [PATCH v4 2/3] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
@ 2020-01-15  8:55   ` Rocky Liao
  2020-01-15 18:02     ` Matthias Kaehlcke
  2020-01-15 17:26   ` [PATCH v4 1/3] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_shutdown() Matthias Kaehlcke
  2020-01-15 21:37   ` Marcel Holtmann
  3 siblings, 1 reply; 34+ messages in thread
From: Rocky Liao @ 2020-01-15  8:55 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: mka, linux-kernel, linux-bluetooth, linux-arm-msm, bgodavar,
	hemantg, Rocky Liao

This patch registers hdev->shutdown() callback and also sets
HCI_QUIRK_NON_PERSISTENT_SETUP for QCA Rome. It will power-off the BT chip
during hci down and power-on/initialize the chip again during hci up. As
wcn399x already enabled this, this patch also removed the callback register
and QUIRK setting in qca_setup() for wcn399x and uniformly do this in the
probe() routine.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---

Changes in v2: None
Changes in v3: 
  -moved the quirk and callback register to probe()
Changes in v4:
  -rebased the patch with latest code
  -moved the quirk and callback register to probe() for wcn399x
  -updated commit message

 drivers/bluetooth/hci_qca.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 1139142e8eed..3c6c6bd20177 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1569,12 +1569,7 @@ static int qca_setup(struct hci_uart *hu)
 		return ret;
 
 	if (qca_is_wcn399x(soc_type)) {
-		/* Enable NON_PERSISTENT_SETUP QUIRK to ensure to execute
-		 * setup for every hci up.
-		 */
-		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
-		hu->hdev->shutdown = qca_power_off;
 
 		ret = qca_read_soc_version(hdev, &soc_ver, soc_type);
 		if (ret)
@@ -1813,6 +1808,7 @@ static int qca_init_regulators(struct qca_power *qca,
 static int qca_serdev_probe(struct serdev_device *serdev)
 {
 	struct qca_serdev *qcadev;
+	struct hci_dev *hdev;
 	const struct qca_vreg_data *data;
 	int err;
 
@@ -1881,7 +1877,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 			clk_disable_unprepare(qcadev->susclk);
 	}
 
-out:	return err;
+out:
+	if (!err) {
+		hdev = qcadev->serdev_hu.hdev;
+		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
+		hdev->shutdown = qca_power_off;
+	}
+	return err;
 
 }
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 1/3] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_shutdown()
  2020-01-15  8:55 ` [PATCH v4 1/3] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_shutdown() Rocky Liao
  2020-01-15  8:55   ` [PATCH v4 2/3] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
  2020-01-15  8:55   ` [PATCH v4 3/3] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
@ 2020-01-15 17:26   ` Matthias Kaehlcke
  2020-01-15 21:37   ` Marcel Holtmann
  3 siblings, 0 replies; 34+ messages in thread
From: Matthias Kaehlcke @ 2020-01-15 17:26 UTC (permalink / raw)
  To: Rocky Liao
  Cc: marcel, johan.hedberg, linux-kernel, linux-bluetooth,
	linux-arm-msm, bgodavar, hemantg

On Wed, Jan 15, 2020 at 04:55:50PM +0800, Rocky Liao wrote:
> Current qca_power_shutdown() only supports wcn399x, this patch adds Rome
> power off support to it. For Rome it just needs to pull down the bt_en
> GPIO to power off it. This patch also replaces all the power off operation
> in qca_close() with the unified qca_power_shutdown() call.
> 
> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
> ---
> 
> Changes in v2: None
> Changes in v3: None
> Changes in v4:
>   -rebased the patch with latest code base
>   -moved the change from qca_power_off() to qca_power_shutdown()
>   -replaced all the power off operation in qca_close() with
>    qca_power_shutdown()
>   -updated commit message
> 
>  drivers/bluetooth/hci_qca.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 992622dc1263..ecb74965be10 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -663,7 +663,6 @@ static int qca_flush(struct hci_uart *hu)
>  /* Close protocol */
>  static int qca_close(struct hci_uart *hu)
>  {
> -	struct qca_serdev *qcadev;
>  	struct qca_data *qca = hu->priv;
>  
>  	BT_DBG("hu %p qca close", hu);
> @@ -679,14 +678,7 @@ static int qca_close(struct hci_uart *hu)
>  	destroy_workqueue(qca->workqueue);
>  	qca->hu = NULL;
>  
> -	if (hu->serdev) {
> -		qcadev = serdev_device_get_drvdata(hu->serdev);
> -		if (qca_is_wcn399x(qcadev->btsoc_type))
> -			qca_power_shutdown(hu);
> -		else
> -			gpiod_set_value_cansleep(qcadev->bt_en, 0);
> -
> -	}
> +	qca_power_shutdown(hu);
>  
>  	kfree_skb(qca->rx_skb);
>  
> @@ -1685,6 +1677,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
>  	struct qca_serdev *qcadev;
>  	struct qca_data *qca = hu->priv;
>  	unsigned long flags;
> +	enum qca_btsoc_type soc_type = qca_soc_type(hu);
>  
>  	qcadev = serdev_device_get_drvdata(hu->serdev);
>  
> @@ -1697,11 +1690,22 @@ static void qca_power_shutdown(struct hci_uart *hu)
>  	qca_flush(hu);
>  	spin_unlock_irqrestore(&qca->hci_ibs_lock, flags);
>  
> -	host_set_baudrate(hu, 2400);
> -	qca_send_power_pulse(hu, false);
> -	qca_regulator_disable(qcadev);
>  	hu->hdev->hw_error = NULL;
>  	hu->hdev->cmd_timeout = NULL;

This is now done before the power off and not after, I suppose it doesn't
make a difference.

> +
> +	/* Non-serdev device usually is powered by external power
> +	 * and don't need additional action in driver for power down
> +	 */
> +	if (!hu->serdev)
> +		return;
> +
> +	if (qca_is_wcn399x(soc_type)) {
> +		host_set_baudrate(hu, 2400);
> +		qca_send_power_pulse(hu, false);
> +		qca_regulator_disable(qcadev);
> +	} else {
> +		gpiod_set_value_cansleep(qcadev->bt_en, 0);
> +	}
>  }

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 2/3] Bluetooth: hci_qca: Retry btsoc initialize when it fails
  2020-01-15  8:55   ` [PATCH v4 2/3] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
@ 2020-01-15 17:33     ` Matthias Kaehlcke
  2020-01-15 21:37     ` Marcel Holtmann
  1 sibling, 0 replies; 34+ messages in thread
From: Matthias Kaehlcke @ 2020-01-15 17:33 UTC (permalink / raw)
  To: Rocky Liao
  Cc: marcel, johan.hedberg, linux-kernel, linux-bluetooth,
	linux-arm-msm, bgodavar, hemantg

On Wed, Jan 15, 2020 at 04:55:51PM +0800, Rocky Liao wrote:
> This patch adds the retry of btsoc initialization when it fails. There are
> reports that the btsoc initialization may fail on some platforms but the
> repro ratio is very low. The symptoms is the firmware downloading failed
> due to the UART write timed out. The failure may be caused by UART,
> platform HW or the btsoc itself but it's very difficlut to root cause,
> given the repro ratio is very low. Add a retry for the btsoc initialization
> can work around most of the failures and make Bluetooth finally works.
> 
> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
> ---
> 
> Changes in v2: None
> Changes in v3: None
> Changes in v4:
>   -rebased the patch with latet code
>   -refined macro and variable name
>   -updated commit message
> 
>  drivers/bluetooth/hci_qca.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index ecb74965be10..1139142e8eed 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -55,6 +55,9 @@
>  /* Controller debug log header */
>  #define QCA_DEBUG_HANDLE	0x2EDC
>  
> +/* max retry count when init fails */
> +#define MAX_INIT_RETRIES 3
> +
>  /* Controller dump header */
>  #define QCA_SSR_DUMP_HANDLE		0x0108
>  #define QCA_DUMP_PACKET_SIZE		255
> @@ -1539,6 +1542,7 @@ 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;
> +	unsigned int retries = 0;
>  	enum qca_btsoc_type soc_type = qca_soc_type(hu);
>  	const char *firmware_name = qca_get_firmware_name(hu);
>  	int ret;
> @@ -1559,6 +1563,7 @@ static int qca_setup(struct hci_uart *hu)
>  	bt_dev_info(hdev, "setting up %s",
>  		qca_is_wcn399x(soc_type) ? "wcn399x" : "ROME");
>  
> +retry:
>  	ret = qca_power_on(hdev);
>  	if (ret)
>  		return ret;
> @@ -1613,6 +1618,20 @@ static int qca_setup(struct hci_uart *hu)
>  		 * patch/nvm-config is found, so run with original fw/config.
>  		 */
>  		ret = 0;
> +	} else {
> +		if (retries < MAX_INIT_RETRIES) {
> +			qca_power_shutdown(hu);
> +			if (hu->serdev) {
> +				serdev_device_close(hu->serdev);
> +				ret = serdev_device_open(hu->serdev);
> +				if (ret) {
> +					bt_dev_err(hdev, "failed to open port");
> +					return ret;
> +				}
> +			}
> +			retries++;
> +			goto retry;
> +		}
>  	}
>  
>  	/* Setup bdaddr */
> -- 

Assuming that this is really a rare condition:

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 3/3] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome
  2020-01-15  8:55   ` [PATCH v4 3/3] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
@ 2020-01-15 18:02     ` Matthias Kaehlcke
  0 siblings, 0 replies; 34+ messages in thread
From: Matthias Kaehlcke @ 2020-01-15 18:02 UTC (permalink / raw)
  To: Rocky Liao
  Cc: marcel, johan.hedberg, linux-kernel, linux-bluetooth,
	linux-arm-msm, bgodavar, hemantg

On Wed, Jan 15, 2020 at 04:55:52PM +0800, Rocky Liao wrote:
> This patch registers hdev->shutdown() callback and also sets
> HCI_QUIRK_NON_PERSISTENT_SETUP for QCA Rome. It will power-off the BT chip
> during hci down and power-on/initialize the chip again during hci up. As
> wcn399x already enabled this, this patch also removed the callback register
> and QUIRK setting in qca_setup() for wcn399x and uniformly do this in the
> probe() routine.
> 
> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
> ---
> 
> Changes in v2: None
> Changes in v3: 
>   -moved the quirk and callback register to probe()
> Changes in v4:
>   -rebased the patch with latest code
>   -moved the quirk and callback register to probe() for wcn399x
>   -updated commit message
> 
>  drivers/bluetooth/hci_qca.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 1139142e8eed..3c6c6bd20177 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -1569,12 +1569,7 @@ static int qca_setup(struct hci_uart *hu)
>  		return ret;
>  
>  	if (qca_is_wcn399x(soc_type)) {
> -		/* Enable NON_PERSISTENT_SETUP QUIRK to ensure to execute
> -		 * setup for every hci up.
> -		 */
> -		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
>  		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);

I guess this should also move to _probe() eventually, but it's not really
the scope of this patch.

> -		hu->hdev->shutdown = qca_power_off;
>  
>  		ret = qca_read_soc_version(hdev, &soc_ver, soc_type);
>  		if (ret)
> @@ -1813,6 +1808,7 @@ static int qca_init_regulators(struct qca_power *qca,
>  static int qca_serdev_probe(struct serdev_device *serdev)
>  {
>  	struct qca_serdev *qcadev;
> +	struct hci_dev *hdev;
>  	const struct qca_vreg_data *data;
>  	int err;
>  
> @@ -1881,7 +1877,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  			clk_disable_unprepare(qcadev->susclk);
>  	}
>  
> -out:	return err;
> +out:
> +	if (!err) {
> +		hdev = qcadev->serdev_hu.hdev;
> +		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
> +		hdev->shutdown = qca_power_off;
> +	}
> +	return err;

Since there is no unwinding in case of an error I would suggest to
change the jumps to the 'out' label further above to 'return err;'
and change the above lines to:

	hdev = qcadev->serdev_hu.hdev;
	set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
	hdev->shutdown = qca_power_off;

	return 0;

This will also require to return directly when hci_uart_register_device()
fails.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 1/3] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_shutdown()
  2020-01-15  8:55 ` [PATCH v4 1/3] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_shutdown() Rocky Liao
                     ` (2 preceding siblings ...)
  2020-01-15 17:26   ` [PATCH v4 1/3] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_shutdown() Matthias Kaehlcke
@ 2020-01-15 21:37   ` Marcel Holtmann
  3 siblings, 0 replies; 34+ messages in thread
From: Marcel Holtmann @ 2020-01-15 21:37 UTC (permalink / raw)
  To: Rocky Liao
  Cc: Johan Hedberg, Matthias Kaehlcke, Linux Kernel Mailing List,
	Bluez mailing list, MSM, Balakrishna Godavarthi, hemantg

Hi Rocky,

> Current qca_power_shutdown() only supports wcn399x, this patch adds Rome
> power off support to it. For Rome it just needs to pull down the bt_en
> GPIO to power off it. This patch also replaces all the power off operation
> in qca_close() with the unified qca_power_shutdown() call.
> 
> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
> ---
> 
> Changes in v2: None
> Changes in v3: None
> Changes in v4:
>  -rebased the patch with latest code base
>  -moved the change from qca_power_off() to qca_power_shutdown()
>  -replaced all the power off operation in qca_close() with
>   qca_power_shutdown()
>  -updated commit message
> 
> drivers/bluetooth/hci_qca.c | 28 ++++++++++++++++------------
> 1 file changed, 16 insertions(+), 12 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v4 2/3] Bluetooth: hci_qca: Retry btsoc initialize when it fails
  2020-01-15  8:55   ` [PATCH v4 2/3] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
  2020-01-15 17:33     ` Matthias Kaehlcke
@ 2020-01-15 21:37     ` Marcel Holtmann
  1 sibling, 0 replies; 34+ messages in thread
From: Marcel Holtmann @ 2020-01-15 21:37 UTC (permalink / raw)
  To: Rocky Liao
  Cc: Johan Hedberg, mka, linux-kernel, linux-bluetooth, linux-arm-msm,
	bgodavar, hemantg

Hi Rocky,

> This patch adds the retry of btsoc initialization when it fails. There are
> reports that the btsoc initialization may fail on some platforms but the
> repro ratio is very low. The symptoms is the firmware downloading failed
> due to the UART write timed out. The failure may be caused by UART,
> platform HW or the btsoc itself but it's very difficlut to root cause,
> given the repro ratio is very low. Add a retry for the btsoc initialization
> can work around most of the failures and make Bluetooth finally works.
> 
> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
> ---
> 
> Changes in v2: None
> Changes in v3: None
> Changes in v4:
>  -rebased the patch with latet code
>  -refined macro and variable name
>  -updated commit message
> 
> drivers/bluetooth/hci_qca.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH v5] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome
  2019-12-25  6:03 [PATCH v1 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
                   ` (5 preceding siblings ...)
  2020-01-15  8:55 ` [PATCH v4 1/3] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_shutdown() Rocky Liao
@ 2020-01-16  3:22 ` Rocky Liao
  2020-01-16  5:31   ` Marcel Holtmann
  6 siblings, 1 reply; 34+ messages in thread
From: Rocky Liao @ 2020-01-16  3:22 UTC (permalink / raw)
  To: marcel, johan.hedberg
  Cc: mka, linux-kernel, linux-bluetooth, linux-arm-msm, bgodavar,
	hemantg, Rocky Liao

This patch registers hdev->shutdown() callback and also sets
HCI_QUIRK_NON_PERSISTENT_SETUP for QCA Rome. It will power-off the BT chip
during hci down and power-on/initialize the chip again during hci up. As
wcn399x already enabled this, this patch also removed the callback register
and QUIRK setting in qca_setup() for wcn399x and uniformly do this in the
probe() routine.

Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
---

Changes in v2: None
Changes in v3:
  -moved the quirk and callback register to probe()
Changes in v4:
  -rebased the patch with latest code
  -moved the quirk and callback register to probe() for wcn399x
  -updated commit message
Changed in v5:
  -removed the "out" label and return err when fails

 drivers/bluetooth/hci_qca.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 1139142e8eed..d6e0c99ee5eb 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1569,12 +1569,7 @@ static int qca_setup(struct hci_uart *hu)
 		return ret;
 
 	if (qca_is_wcn399x(soc_type)) {
-		/* Enable NON_PERSISTENT_SETUP QUIRK to ensure to execute
-		 * setup for every hci up.
-		 */
-		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
-		hu->hdev->shutdown = qca_power_off;
 
 		ret = qca_read_soc_version(hdev, &soc_ver, soc_type);
 		if (ret)
@@ -1813,6 +1808,7 @@ static int qca_init_regulators(struct qca_power *qca,
 static int qca_serdev_probe(struct serdev_device *serdev)
 {
 	struct qca_serdev *qcadev;
+	struct hci_dev *hdev;
 	const struct qca_vreg_data *data;
 	int err;
 
@@ -1838,7 +1834,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 					  data->num_vregs);
 		if (err) {
 			BT_ERR("Failed to init regulators:%d", err);
-			goto out;
+			return err;
 		}
 
 		qcadev->bt_power->vregs_on = false;
@@ -1851,7 +1847,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
 		if (err) {
 			BT_ERR("wcn3990 serdev registration failed");
-			goto out;
+			return err;
 		}
 	} else {
 		qcadev->btsoc_type = QCA_ROME;
@@ -1877,12 +1873,18 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 			return err;
 
 		err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
-		if (err)
+		if (err) {
+			BT_ERR("Rome serdev registration failed");
 			clk_disable_unprepare(qcadev->susclk);
+			return err;
+		}
 	}
 
-out:	return err;
+	hdev = qcadev->serdev_hu.hdev;
+	set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
+	hdev->shutdown = qca_power_off;
 
+	return 0;
 }
 
 static void qca_serdev_remove(struct serdev_device *serdev)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH v5] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome
  2020-01-16  3:22 ` [PATCH v5] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
@ 2020-01-16  5:31   ` Marcel Holtmann
  0 siblings, 0 replies; 34+ messages in thread
From: Marcel Holtmann @ 2020-01-16  5:31 UTC (permalink / raw)
  To: Rocky Liao
  Cc: Johan Hedberg, Matthias Kaehlcke, Linux Kernel Mailing List,
	Bluez mailing list, MSM, Balakrishna Godavarthi, hemantg

Hi Rocky,

> This patch registers hdev->shutdown() callback and also sets
> HCI_QUIRK_NON_PERSISTENT_SETUP for QCA Rome. It will power-off the BT chip
> during hci down and power-on/initialize the chip again during hci up. As
> wcn399x already enabled this, this patch also removed the callback register
> and QUIRK setting in qca_setup() for wcn399x and uniformly do this in the
> probe() routine.
> 
> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
> ---
> 
> Changes in v2: None
> Changes in v3:
>  -moved the quirk and callback register to probe()
> Changes in v4:
>  -rebased the patch with latest code
>  -moved the quirk and callback register to probe() for wcn399x
>  -updated commit message
> Changed in v5:
>  -removed the "out" label and return err when fails
> 
> drivers/bluetooth/hci_qca.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, back to index

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-25  6:03 [PATCH v1 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
2019-12-25  6:03 ` [PATCH v1 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
2019-12-25  6:03 ` [PATCH v1 3/4] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
2019-12-25  6:03 ` [PATCH v1 4/4] Bluetooth: hci_qca: Add HCI command timeout handling Rocky Liao
2019-12-26  3:04   ` kbuild test robot
2019-12-26  7:46   ` kbuild test robot
2019-12-26 23:33   ` kbuild test robot
2019-12-26  6:45 ` [PATCH v2 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
2019-12-26  6:45   ` [PATCH v2 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
2019-12-26  6:45   ` [PATCH v2 3/4] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
2019-12-26 20:30     ` Marcel Holtmann
2019-12-26  6:45   ` [PATCH v2 4/4] Bluetooth: hci_qca: Add HCI command timeout handling Rocky Liao
2019-12-26 20:29     ` Marcel Holtmann
2019-12-27  7:21 ` [PATCH v3 1/4] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_off() Rocky Liao
2019-12-27  7:21   ` [PATCH v3 2/4] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
2020-01-02 18:41     ` Matthias Kaehlcke
2020-01-03  6:31       ` rjliao
2020-01-03 16:27         ` Matthias Kaehlcke
2020-01-07  5:34           ` Rocky Liao
2019-12-27  7:21   ` [PATCH v3 3/4] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
2019-12-27  7:21   ` [PATCH v3 4/4] Bluetooth: hci_qca: Add HCI command timeout handling Rocky Liao
2020-01-02 19:07     ` Matthias Kaehlcke
2020-01-03  6:33       ` rjliao
2020-01-03  9:02         ` Rocky Liao
2020-01-15  8:55 ` [PATCH v4 1/3] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_shutdown() Rocky Liao
2020-01-15  8:55   ` [PATCH v4 2/3] Bluetooth: hci_qca: Retry btsoc initialize when it fails Rocky Liao
2020-01-15 17:33     ` Matthias Kaehlcke
2020-01-15 21:37     ` Marcel Holtmann
2020-01-15  8:55   ` [PATCH v4 3/3] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
2020-01-15 18:02     ` Matthias Kaehlcke
2020-01-15 17:26   ` [PATCH v4 1/3] Bluetooth: hci_qca: Add QCA Rome power off support to the qca_power_shutdown() Matthias Kaehlcke
2020-01-15 21:37   ` Marcel Holtmann
2020-01-16  3:22 ` [PATCH v5] Bluetooth: hci_qca: Enable power off/on support during hci down/up for QCA Rome Rocky Liao
2020-01-16  5:31   ` Marcel Holtmann

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org
	public-inbox-index linux-bluetooth

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git