[PATCH v4 0/3] Bluetooth: hci_qca: Add serdev support

[PATCH v4 0/3] Bluetooth: hci_qca: Add serdev support

[Thierry Escande]

Hi,

This patchset enables the Qualcomm BT controller QCA6174 node in the
device tree of the db820c board. This allows the bluetooth chipset to
be probed and registered against the hci layer by using the serdev
framework.

This patchset also contains the documentation for the compatible
string "qcom,qca6174-bt" related to this chipset.

v4:
- Fix dt binding documentation
- Address some other issues in patch #3

v3:
- Address comments for patch #3 (details in patch)

v2:
- Fix author email

Thierry Escande (3):
  arm64: dts: apq8096-db820c: enable bluetooth node
  dt-bindings: net: bluetooth: Add qualcomm-bluetooth
  Bluetooth: hci_qca: Add serdev support

 .../devicetree/bindings/net/qualcomm-bluetooth.txt |  34 +++++++
 arch/arm64/boot/dts/qcom/apq8096-db820c-pins.dtsi  |  14 +++
 .../boot/dts/qcom/apq8096-db820c-pmic-pins.dtsi    |  17 ++++
 arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi       |  32 ++++++
 arch/arm64/boot/dts/qcom/msm8996.dtsi              |  10 ++
 drivers/bluetooth/Kconfig                          |   1 +
 drivers/bluetooth/hci_qca.c                        | 109 ++++++++++++++++++++-
 7 files changed, 215 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt

-- 
2.14.1

[PATCH v4 1/3] arm64: dts: apq8096-db820c: enable bluetooth node

[Thierry Escande]

Add a new serial node for the Qualcomm BT controller QCA6174. This
allows automatic probing and hci registration through the serdev
framework instead of relying on the userspace helpers.

Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
---

v4: no change
v3: no change

v2:
- Fix author email

 arch/arm64/boot/dts/qcom/apq8096-db820c-pins.dtsi  | 14 ++++++++++
 .../boot/dts/qcom/apq8096-db820c-pmic-pins.dtsi    | 17 ++++++++++++
 arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi       | 32 ++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/msm8996.dtsi              | 10 +++++++
 4 files changed, 73 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c-pins.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c-pins.dtsi
index 24552f19b3fa..172165d84669 100644
--- a/arch/arm64/boot/dts/qcom/apq8096-db820c-pins.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8096-db820c-pins.dtsi
@@ -36,4 +36,18 @@
 			drive-strength = <2>;	/* 2 MA */
 		};
 	};
+
+	blsp1_uart1_default: blsp1_uart1_default {
+		function = "blsp_uart2";
+		pins = "gpio41", "gpio42", "gpio43", "gpio44";
+		drive-strength = <16>;
+		bias-disable;
+	};
+
+	blsp1_uart1_sleep: blsp1_uart1_sleep {
+		function = "gpio";
+		pins = "gpio41", "gpio42", "gpio43", "gpio44";
+		drive-strength = <2>;
+		bias-disable;
+	};
 };
diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c-pmic-pins.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c-pmic-pins.dtsi
index 59b29ddfb6e9..f8d2a3b10b1f 100644
--- a/arch/arm64/boot/dts/qcom/apq8096-db820c-pmic-pins.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8096-db820c-pmic-pins.dtsi
@@ -26,6 +26,23 @@
 		};
 	};
 
+	divclk4_pin_a: divclk4 {
+		pins = "gpio18";
+		function = "func2";
+
+		bias-disable;
+		power-source = <PM8994_GPIO_S4>;
+	};
+
+	bt_en_pin_a: bt-en-active {
+		pins = "gpio19";
+		function = "normal";
+
+		output-low;
+		power-source = <PM8994_GPIO_S4>;
+		qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>;
+	};
+
 	usb3_vbus_det_gpio: pm8996_gpio22 {
 		pinconf {
 			pins = "gpio22";
diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
index 1c8f1b86472d..b05d6bc0b856 100644
--- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
@@ -23,6 +23,7 @@
 	aliases {
 		serial0 = &blsp2_uart1;
 		serial1 = &blsp2_uart2;
+		serial2 = &blsp1_uart1;
 		i2c0	= &blsp1_i2c2;
 		i2c1	= &blsp2_i2c1;
 		i2c2	= &blsp2_i2c0;
@@ -34,7 +35,38 @@
 		stdout-path = "serial0:115200n8";
 	};
 
+	clocks {
+		divclk4: divclk4 {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <32768>;
+			clock-output-names = "divclk4";
+
+			pinctrl-names = "default";
+			pinctrl-0 = <&divclk4_pin_a>;
+		};
+	};
+
 	soc {
+		serial@7570000 {
+			label = "BT-UART";
+			status = "okay";
+			pinctrl-names = "default", "sleep";
+			pinctrl-0 = <&blsp1_uart1_default>;
+			pinctrl-1 = <&blsp1_uart1_sleep>;
+
+			bluetooth {
+				compatible = "qcom,qca6174-bt";
+
+				bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;
+
+				pinctrl-names = "default";
+				pinctrl-0 = <&bt_en_pin_a>;
+
+				clocks = <&divclk4>;
+			};
+		};
+
 		serial@75b0000 {
 			label = "LS-UART1";
 			status = "okay";
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 0a6f7952bbb1..2d54a86a027f 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -408,6 +408,16 @@
 			#clock-cells = <1>;
 		};
 
+		blsp1_uart1: serial@7570000 {
+			compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
+			reg = <0x07570000 0x1000>;
+			interrupts = <GIC_SPI 108 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&gcc GCC_BLSP1_UART2_APPS_CLK>,
+				 <&gcc GCC_BLSP1_AHB_CLK>;
+			clock-names = "core", "iface";
+			status = "disabled";
+		};
+
 		blsp1_spi0: spi@7575000 {
 			compatible = "qcom,spi-qup-v2.2.1";
 			reg = <0x07575000 0x600>;
-- 
2.14.1

[PATCH v4 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Thierry Escande]

Add binding document for serial bluetooth chips using Qualcomm protocol.

Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
---

v4:
- Move bt-disable-n-gpios to required properties section
- Add clocks and pinctrl-0 as required properties

v3: no change
v2: no change

 .../devicetree/bindings/net/qualcomm-bluetooth.txt | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt

diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
new file mode 100644
index 000000000000..cdb14b96c229
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
@@ -0,0 +1,34 @@
+Qualcomm Bluetooth Chips
+---------------------
+
+This documents the binding structure and common properties for serial
+attached Qualcomm devices.
+
+Serial attached Qualcomm devices shall be a child node of the host UART
+device the slave device is attached to.
+
+Required properties:
+ - compatible: should contain one of the following:
+   * "qcom,qca6174-bt"
+ - bt-disable-n-gpios: gpio specifier, used to enable chip during probe
+ - pinctrl-0: pin phandle for bt_en gpio
+ - clocks: clock phandle for SUSCLK_32KHZ
+
+Example:
+
+serial@7570000 {
+	pinctrl-names = "default", "sleep";
+	pinctrl-0 = <&blsp1_uart1_default>;
+	pinctrl-1 = <&blsp1_uart1_sleep>;
+
+	bluetooth {
+		compatible = "qcom,qca6174-bt";
+
+		bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&bt_en_pin_a>;
+
+		clocks = <&divclk4>;
+	};
+};
-- 
2.14.1

[PATCH v4 3/3] Bluetooth: hci_qca: Add serdev support

[Thierry Escande]

Add support for Qualcomm serial slave devices. Probe the serial device,
retrieve its maximum speed and register a new hci uart device.

Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
---

v4:
- Rename divclk4 as susclk (its name in the bt chip)
- Use gpiod_set_value_cansleep()
- Replace #include <linux/of.h> with <linux/mod_devicetable.h>
- Restore dependency on BT_HCIUART

v3:
- Remove redundant call to gpiod_set_value() after devm_gpiod_get()
- Check returned values for clk_set_rate() and clk_prepare_enable()
- Use clk_disable_unprepare()

v2:
- Fix author email

 drivers/bluetooth/Kconfig   |   1 +
 drivers/bluetooth/hci_qca.c | 109 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 108 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 07e55cd8f8c8..e0f1a6609b68 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -196,6 +196,7 @@ config BT_HCIUART_BCM
 config BT_HCIUART_QCA
 	bool "Qualcomm Atheros protocol support"
 	depends on BT_HCIUART
+	depends on BT_HCIUART_SERDEV
 	select BT_HCIUART_H4
 	select BT_QCA
 	help
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 05ec530b8a3a..6cf0d1d4595a 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -29,7 +29,11 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/clk.h>
 #include <linux/debugfs.h>
+#include <linux/gpio/consumer.h>
+#include <linux/mod_devicetable.h>
+#include <linux/serdev.h>
 
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
@@ -50,6 +54,9 @@
 #define IBS_TX_IDLE_TIMEOUT_MS		2000
 #define BAUDRATE_SETTLE_TIMEOUT_MS	300
 
+/* susclk rate */
+#define SUSCLK_RATE_32KHZ	32768
+
 /* HCI_IBS transmit side sleep protocol states */
 enum tx_ibs_states {
 	HCI_IBS_TX_ASLEEP,
@@ -111,6 +118,12 @@ struct qca_data {
 	u64 votes_off;
 };
 
+struct qca_serdev {
+	struct hci_uart	 serdev_hu;
+	struct gpio_desc *bt_en;
+	struct clk	 *susclk;
+};
+
 static void __serial_clock_on(struct tty_struct *tty)
 {
 	/* TODO: Some chipset requires to enable UART clock on client
@@ -386,6 +399,7 @@ static void hci_ibs_wake_retrans_timeout(struct timer_list *t)
 /* Initialize protocol */
 static int qca_open(struct hci_uart *hu)
 {
+	struct qca_serdev *qcadev;
 	struct qca_data *qca;
 
 	BT_DBG("hu %p qca_open", hu);
@@ -444,6 +458,13 @@ static int qca_open(struct hci_uart *hu)
 	timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0);
 	qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS;
 
+	if (hu->serdev) {
+		serdev_device_open(hu->serdev);
+
+		qcadev = serdev_device_get_drvdata(hu->serdev);
+		gpiod_set_value_cansleep(qcadev->bt_en, 1);
+	}
+
 	BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u",
 	       qca->tx_idle_delay, qca->wake_retrans);
 
@@ -512,6 +533,7 @@ 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);
@@ -525,6 +547,13 @@ static int qca_close(struct hci_uart *hu)
 	destroy_workqueue(qca->workqueue);
 	qca->hu = NULL;
 
+	if (hu->serdev) {
+		serdev_device_close(hu->serdev);
+
+		qcadev = serdev_device_get_drvdata(hu->serdev);
+		gpiod_set_value_cansleep(qcadev->bt_en, 0);
+	}
+
 	kfree_skb(qca->rx_skb);
 
 	hu->priv = NULL;
@@ -885,6 +914,14 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
 	return 0;
 }
 
+static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
+{
+	if (hu->serdev)
+		serdev_device_set_baudrate(hu->serdev, speed);
+	else
+		hci_uart_set_baudrate(hu, speed);
+}
+
 static int qca_setup(struct hci_uart *hu)
 {
 	struct hci_dev *hdev = hu->hdev;
@@ -905,7 +942,7 @@ static int qca_setup(struct hci_uart *hu)
 		speed = hu->proto->init_speed;
 
 	if (speed)
-		hci_uart_set_baudrate(hu, speed);
+		host_set_baudrate(hu, speed);
 
 	/* Setup user speed if needed */
 	speed = 0;
@@ -924,7 +961,7 @@ static int qca_setup(struct hci_uart *hu)
 				   ret);
 			return ret;
 		}
-		hci_uart_set_baudrate(hu, speed);
+		host_set_baudrate(hu, speed);
 	}
 
 	/* Setup patch / NVM configurations */
@@ -958,12 +995,80 @@ static struct hci_uart_proto qca_proto = {
 	.dequeue	= qca_dequeue,
 };
 
+static int qca_serdev_probe(struct serdev_device *serdev)
+{
+	struct qca_serdev *qcadev;
+	int err;
+
+	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
+	if (!qcadev)
+		return -ENOMEM;
+
+	qcadev->serdev_hu.serdev = serdev;
+	serdev_device_set_drvdata(serdev, qcadev);
+
+	qcadev->bt_en = devm_gpiod_get(&serdev->dev, "bt-disable-n",
+				       GPIOD_OUT_LOW);
+	if (IS_ERR(qcadev->bt_en)) {
+		dev_err(&serdev->dev, "failed to acquire bt-disable-n gpio\n");
+		return PTR_ERR(qcadev->bt_en);
+	}
+
+	qcadev->susclk = devm_clk_get(&serdev->dev, NULL);
+	if (IS_ERR(qcadev->susclk)) {
+		dev_err(&serdev->dev, "failed to acquire clk\n");
+		return PTR_ERR(qcadev->susclk);
+	}
+
+	err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
+	if (err)
+		return err;
+
+	err = clk_prepare_enable(qcadev->susclk);
+	if (err)
+		return err;
+
+	err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
+	if (err)
+		clk_disable_unprepare(qcadev->susclk);
+
+	return err;
+}
+
+static void qca_serdev_remove(struct serdev_device *serdev)
+{
+	struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
+
+	hci_uart_unregister_device(&qcadev->serdev_hu);
+
+	clk_disable_unprepare(qcadev->susclk);
+}
+
+static const struct of_device_id qca_bluetooth_of_match[] = {
+	{ .compatible = "qcom,qca6174-bt" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);
+
+static struct serdev_device_driver qca_serdev_driver = {
+	.probe = qca_serdev_probe,
+	.remove = qca_serdev_remove,
+	.driver = {
+		.name = "hci_uart_qca",
+		.of_match_table = qca_bluetooth_of_match,
+	},
+};
+
 int __init qca_init(void)
 {
+	serdev_device_driver_register(&qca_serdev_driver);
+
 	return hci_uart_register_proto(&qca_proto);
 }
 
 int __exit qca_deinit(void)
 {
+	serdev_device_driver_unregister(&qca_serdev_driver);
+
 	return hci_uart_unregister_proto(&qca_proto);
 }
-- 
2.14.1

Re: [PATCH v4 3/3] Bluetooth: hci_qca: Add serdev support

[Andy Shevchenko]

On Wed, 2018-03-14 at 16:55 +0100, Thierry Escande wrote:
> Add support for Qualcomm serial slave devices. Probe the serial
> device,
> retrieve its maximum speed and register a new hci uart device.
> 

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
> ---
> 
> v4:
> - Rename divclk4 as susclk (its name in the bt chip)
> - Use gpiod_set_value_cansleep()
> - Replace #include <linux/of.h> with <linux/mod_devicetable.h>
> - Restore dependency on BT_HCIUART
> 
> v3:
> - Remove redundant call to gpiod_set_value() after devm_gpiod_get()
> - Check returned values for clk_set_rate() and clk_prepare_enable()
> - Use clk_disable_unprepare()
> 
> v2:
> - Fix author email
> 
>  drivers/bluetooth/Kconfig   |   1 +
>  drivers/bluetooth/hci_qca.c | 109
> +++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 108 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 07e55cd8f8c8..e0f1a6609b68 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -196,6 +196,7 @@ config BT_HCIUART_BCM
>  config BT_HCIUART_QCA
>  	bool "Qualcomm Atheros protocol support"
>  	depends on BT_HCIUART
> +	depends on BT_HCIUART_SERDEV
>  	select BT_HCIUART_H4
>  	select BT_QCA
>  	help
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 05ec530b8a3a..6cf0d1d4595a 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -29,7 +29,11 @@
>   */
>  
>  #include <linux/kernel.h>
> +#include <linux/clk.h>
>  #include <linux/debugfs.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/serdev.h>
>  
>  #include <net/bluetooth/bluetooth.h>
>  #include <net/bluetooth/hci_core.h>
> @@ -50,6 +54,9 @@
>  #define IBS_TX_IDLE_TIMEOUT_MS		2000
>  #define BAUDRATE_SETTLE_TIMEOUT_MS	300
>  
> +/* susclk rate */
> +#define SUSCLK_RATE_32KHZ	32768
> +
>  /* HCI_IBS transmit side sleep protocol states */
>  enum tx_ibs_states {
>  	HCI_IBS_TX_ASLEEP,
> @@ -111,6 +118,12 @@ struct qca_data {
>  	u64 votes_off;
>  };
>  
> +struct qca_serdev {
> +	struct hci_uart	 serdev_hu;
> +	struct gpio_desc *bt_en;
> +	struct clk	 *susclk;
> +};
> +
>  static void __serial_clock_on(struct tty_struct *tty)
>  {
>  	/* TODO: Some chipset requires to enable UART clock on client
> @@ -386,6 +399,7 @@ static void hci_ibs_wake_retrans_timeout(struct
> timer_list *t)
>  /* Initialize protocol */
>  static int qca_open(struct hci_uart *hu)
>  {
> +	struct qca_serdev *qcadev;
>  	struct qca_data *qca;
>  
>  	BT_DBG("hu %p qca_open", hu);
> @@ -444,6 +458,13 @@ static int qca_open(struct hci_uart *hu)
>  	timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0);
>  	qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS;
>  
> +	if (hu->serdev) {
> +		serdev_device_open(hu->serdev);
> +
> +		qcadev = serdev_device_get_drvdata(hu->serdev);
> +		gpiod_set_value_cansleep(qcadev->bt_en, 1);
> +	}
> +
>  	BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u,
> wake_retrans=%u",
>  	       qca->tx_idle_delay, qca->wake_retrans);
>  
> @@ -512,6 +533,7 @@ 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);
> @@ -525,6 +547,13 @@ static int qca_close(struct hci_uart *hu)
>  	destroy_workqueue(qca->workqueue);
>  	qca->hu = NULL;
>  
> +	if (hu->serdev) {
> +		serdev_device_close(hu->serdev);
> +
> +		qcadev = serdev_device_get_drvdata(hu->serdev);
> +		gpiod_set_value_cansleep(qcadev->bt_en, 0);
> +	}
> +
>  	kfree_skb(qca->rx_skb);
>  
>  	hu->priv = NULL;
> @@ -885,6 +914,14 @@ static int qca_set_baudrate(struct hci_dev *hdev,
> uint8_t baudrate)
>  	return 0;
>  }
>  
> +static inline void host_set_baudrate(struct hci_uart *hu, unsigned
> int speed)
> +{
> +	if (hu->serdev)
> +		serdev_device_set_baudrate(hu->serdev, speed);
> +	else
> +		hci_uart_set_baudrate(hu, speed);
> +}
> +
>  static int qca_setup(struct hci_uart *hu)
>  {
>  	struct hci_dev *hdev = hu->hdev;
> @@ -905,7 +942,7 @@ static int qca_setup(struct hci_uart *hu)
>  		speed = hu->proto->init_speed;
>  
>  	if (speed)
> -		hci_uart_set_baudrate(hu, speed);
> +		host_set_baudrate(hu, speed);
>  
>  	/* Setup user speed if needed */
>  	speed = 0;
> @@ -924,7 +961,7 @@ static int qca_setup(struct hci_uart *hu)
>  				   ret);
>  			return ret;
>  		}
> -		hci_uart_set_baudrate(hu, speed);
> +		host_set_baudrate(hu, speed);
>  	}
>  
>  	/* Setup patch / NVM configurations */
> @@ -958,12 +995,80 @@ static struct hci_uart_proto qca_proto = {
>  	.dequeue	= qca_dequeue,
>  };
>  
> +static int qca_serdev_probe(struct serdev_device *serdev)
> +{
> +	struct qca_serdev *qcadev;
> +	int err;
> +
> +	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev),
> GFP_KERNEL);
> +	if (!qcadev)
> +		return -ENOMEM;
> +
> +	qcadev->serdev_hu.serdev = serdev;
> +	serdev_device_set_drvdata(serdev, qcadev);
> +
> +	qcadev->bt_en = devm_gpiod_get(&serdev->dev, "bt-disable-n",
> +				       GPIOD_OUT_LOW);
> +	if (IS_ERR(qcadev->bt_en)) {
> +		dev_err(&serdev->dev, "failed to acquire bt-disable-n 
> gpio\n");
> +		return PTR_ERR(qcadev->bt_en);
> +	}
> +
> +	qcadev->susclk = devm_clk_get(&serdev->dev, NULL);
> +	if (IS_ERR(qcadev->susclk)) {
> +		dev_err(&serdev->dev, "failed to acquire clk\n");
> +		return PTR_ERR(qcadev->susclk);
> +	}
> +
> +	err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
> +	if (err)
> +		return err;
> +
> +	err = clk_prepare_enable(qcadev->susclk);
> +	if (err)
> +		return err;
> +
> +	err = hci_uart_register_device(&qcadev->serdev_hu,
> &qca_proto);
> +	if (err)
> +		clk_disable_unprepare(qcadev->susclk);
> +
> +	return err;
> +}
> +
> +static void qca_serdev_remove(struct serdev_device *serdev)
> +{
> +	struct qca_serdev *qcadev =
> serdev_device_get_drvdata(serdev);
> +
> +	hci_uart_unregister_device(&qcadev->serdev_hu);
> +
> +	clk_disable_unprepare(qcadev->susclk);
> +}
> +
> +static const struct of_device_id qca_bluetooth_of_match[] = {
> +	{ .compatible = "qcom,qca6174-bt" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);
> +
> +static struct serdev_device_driver qca_serdev_driver = {
> +	.probe = qca_serdev_probe,
> +	.remove = qca_serdev_remove,
> +	.driver = {
> +		.name = "hci_uart_qca",
> +		.of_match_table = qca_bluetooth_of_match,
> +	},
> +};
> +
>  int __init qca_init(void)
>  {
> +	serdev_device_driver_register(&qca_serdev_driver);
> +
>  	return hci_uart_register_proto(&qca_proto);
>  }
>  
>  int __exit qca_deinit(void)
>  {
> +	serdev_device_driver_unregister(&qca_serdev_driver);
> +
>  	return hci_uart_unregister_proto(&qca_proto);
>  }

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v4 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Marcel Holtmann]

Hi Thierry,

> Add binding document for serial bluetooth chips using Qualcomm protocol.
> 
> Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
> ---
> 
> v4:
> - Move bt-disable-n-gpios to required properties section
> - Add clocks and pinctrl-0 as required properties
> 
> v3: no change
> v2: no change
> 
> .../devicetree/bindings/net/qualcomm-bluetooth.txt | 34 ++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> new file mode 100644
> index 000000000000..cdb14b96c229
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/qualcomm-bluetooth.txt
> @@ -0,0 +1,34 @@
> +Qualcomm Bluetooth Chips
> +---------------------
> +
> +This documents the binding structure and common properties for serial
> +attached Qualcomm devices.
> +
> +Serial attached Qualcomm devices shall be a child node of the host UART
> +device the slave device is attached to.
> +
> +Required properties:
> + - compatible: should contain one of the following:
> +   * "qcom,qca6174-bt"
> + - bt-disable-n-gpios: gpio specifier, used to enable chip during probe
> + - pinctrl-0: pin phandle for bt_en gpio
> + - clocks: clock phandle for SUSCLK_32KHZ
> +
> +Example:
> +
> +serial@7570000 {
> +	pinctrl-names = "default", "sleep";
> +	pinctrl-0 = <&blsp1_uart1_default>;
> +	pinctrl-1 = <&blsp1_uart1_sleep>;
> +
> +	bluetooth {
> +		compatible = "qcom,qca6174-bt";
> +
> +		bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;

can we use a common name here. I think that Nokia and Broadcom drivers define one. And if this is the enable/shutdown GPIO, we should name it consistently across all manufacturers. It essentially does the same on Bluetooth UART chips no matter what chip is behind them.

Regards

Marcel

Re: [PATCH v4 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Bjorn Andersson]

On Wed 14 Mar 09:13 PDT 2018, Marcel Holtmann wrote:
> > +		bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;
> 
> can we use a common name here. I think that Nokia and Broadcom drivers
> define one. And if this is the enable/shutdown GPIO, we should name it
> consistently across all manufacturers. It essentially does the same on
> Bluetooth UART chips no matter what chip is behind them.
> 

Broadcomm has a device-wakup-gpios and Nokia has bluetooth-wakup-gpios.
It might be that these behave in the same way, but from the description
they only trigger the wakeup.

The reason for the proposed naming here is that the pin is named
"BT_DISABLE_N" in the datasheet.

Regards,
Bjorn

Re: [PATCH v4 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Marcel Holtmann]

Hi Bjoern,

>>> +		bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;
>> 
>> can we use a common name here. I think that Nokia and Broadcom drivers
>> define one. And if this is the enable/shutdown GPIO, we should name it
>> consistently across all manufacturers. It essentially does the same on
>> Bluetooth UART chips no matter what chip is behind them.
>> 
> 
> Broadcomm has a device-wakup-gpios and Nokia has bluetooth-wakup-gpios.
> It might be that these behave in the same way, but from the description
> they only trigger the wakeup.

that is something that we might need to start fixing. I really prefer if we name the GPIOs a bit more consistent.

> The reason for the proposed naming here is that the pin is named
> "BT_DISABLE_N" in the datasheet.

That is not a reason I buy. So the next board comes around that labels it in the data sheet BT_DISABLE_YEAH_SUPER_GREAT and you send me a patch to the driver to look for that name. If the GPIO does the same thing, I couldn’t care less what the data sheet says. That might be a comment in the DT file, but it should not pollute the driver code.

A new board should not require driver changes, you just ship a new DT for that board and an existing driver hopefully just does the job. No matter how someone named a GPIO in a piece of paper.

Regards

Marcel

Re: [PATCH v4 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Bjorn Andersson]

On Wed 14 Mar 11:42 PDT 2018, Marcel Holtmann wrote:

> Hi Bjoern,
> 
> >>> +		bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;
> >> 
> >> can we use a common name here. I think that Nokia and Broadcom drivers
> >> define one. And if this is the enable/shutdown GPIO, we should name it
> >> consistently across all manufacturers. It essentially does the same on
> >> Bluetooth UART chips no matter what chip is behind them.
> >> 
> > 
> > Broadcomm has a device-wakup-gpios and Nokia has bluetooth-wakup-gpios.
> > It might be that these behave in the same way, but from the description
> > they only trigger the wakeup.
> 
> that is something that we might need to start fixing. I really prefer
> if we name the GPIOs a bit more consistent.
> 
> > The reason for the proposed naming here is that the pin is named
> > "BT_DISABLE_N" in the datasheet.
> 
> That is not a reason I buy. So the next board comes around that labels
> it in the data sheet BT_DISABLE_YEAH_SUPER_GREAT and you send me a
> patch to the driver to look for that name. If the GPIO does the same
> thing, I couldn’t care less what the data sheet says. That might be
> a comment in the DT file, but it should not pollute the driver code.
> 

BT_DISABLE_N is the name of this pin in the datasheet of the QCA chip,
not on the board, so this name is the same regardless of what you name
the line or gpio your board connect it to.

> A new board should not require driver changes, you just ship a new DT
> for that board and an existing driver hopefully just does the job. No
> matter how someone named a GPIO in a piece of paper.
> 

I totally agree with the fact that the board should not affect the
naming of the gpio in the driver. But I do enjoy when we refer to pins
by their real name - instead of having to guess which pin in the _chip_
specification the driver actually refer to.


That said, what name would you prefer for this?

Afaict this is not "wakeup" and there are a few extra steps between the
disabled state and "bluetooth is enabled", so "enable" feels slightly
wrong. And it probably should be "bluetooth" and not just "device" as
this refers to a pin on a WiFi/BT combo chip.

Regards,
Bjorn

Re: [PATCH v4 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Marcel Holtmann]

Hi Bjorn,

>>>>> +		bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;
>>>> 
>>>> can we use a common name here. I think that Nokia and Broadcom drivers
>>>> define one. And if this is the enable/shutdown GPIO, we should name it
>>>> consistently across all manufacturers. It essentially does the same on
>>>> Bluetooth UART chips no matter what chip is behind them.
>>>> 
>>> 
>>> Broadcomm has a device-wakup-gpios and Nokia has bluetooth-wakup-gpios.
>>> It might be that these behave in the same way, but from the description
>>> they only trigger the wakeup.
>> 
>> that is something that we might need to start fixing. I really prefer
>> if we name the GPIOs a bit more consistent.
>> 
>>> The reason for the proposed naming here is that the pin is named
>>> "BT_DISABLE_N" in the datasheet.
>> 
>> That is not a reason I buy. So the next board comes around that labels
>> it in the data sheet BT_DISABLE_YEAH_SUPER_GREAT and you send me a
>> patch to the driver to look for that name. If the GPIO does the same
>> thing, I couldn’t care less what the data sheet says. That might be
>> a comment in the DT file, but it should not pollute the driver code.
>> 
> 
> BT_DISABLE_N is the name of this pin in the datasheet of the QCA chip,
> not on the board, so this name is the same regardless of what you name
> the line or gpio your board connect it to.

and QCA chip v1 and QCA chip v2 will use the same driver and same firmware loading mechanism. So why do we have to add a new GPIO naming if they decide to change the name in the data sheet. With Bluetooth it is pretty much all the same. Every UART chip has a shutdown/reset GPIO to enable/disable the chip behind the UART.

>> A new board should not require driver changes, you just ship a new DT
>> for that board and an existing driver hopefully just does the job. No
>> matter how someone named a GPIO in a piece of paper.
>> 
> 
> I totally agree with the fact that the board should not affect the
> naming of the gpio in the driver. But I do enjoy when we refer to pins
> by their real name - instead of having to guess which pin in the _chip_
> specification the driver actually refer to.
> 
> 
> That said, what name would you prefer for this?
> 
> Afaict this is not "wakeup" and there are a few extra steps between the
> disabled state and "bluetooth is enabled", so "enable" feels slightly
> wrong. And it probably should be "bluetooth" and not just "device" as
> this refers to a pin on a WiFi/BT combo chip.

The Broadcom side called it shutdown GPIO, it is essentially the shutdown/reset GPIO or power on/off GPIO. Personally I do not care what it is named, but it will be all the same for all Bluetooth chips. Take a poll from Broadcom, Intel, Realtek and Qualcomm and you can pick a reasonable common name.

For the wakeup GPIOs, these are different and depend on if there is some low-power mode provided. You would need to check the data sheet to see if they provide more advanced low-power state handling.

Regards

Marcel

Re: [PATCH v4 3/3] Bluetooth: hci_qca: Add serdev support

[Marcel Holtmann]

Hi Thierry,

> Add support for Qualcomm serial slave devices. Probe the serial device,
> retrieve its maximum speed and register a new hci uart device.
> 
> Signed-off-by: Thierry Escande <thierry.escande@linaro.org>
> ---
> 
> v4:
> - Rename divclk4 as susclk (its name in the bt chip)
> - Use gpiod_set_value_cansleep()
> - Replace #include <linux/of.h> with <linux/mod_devicetable.h>
> - Restore dependency on BT_HCIUART
> 
> v3:
> - Remove redundant call to gpiod_set_value() after devm_gpiod_get()
> - Check returned values for clk_set_rate() and clk_prepare_enable()
> - Use clk_disable_unprepare()
> 
> v2:
> - Fix author email
> 
> drivers/bluetooth/Kconfig   |   1 +
> drivers/bluetooth/hci_qca.c | 109 +++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 108 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 07e55cd8f8c8..e0f1a6609b68 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -196,6 +196,7 @@ config BT_HCIUART_BCM
> config BT_HCIUART_QCA
> 	bool "Qualcomm Atheros protocol support"
> 	depends on BT_HCIUART
> +	depends on BT_HCIUART_SERDEV
> 	select BT_HCIUART_H4
> 	select BT_QCA
> 	help
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 05ec530b8a3a..6cf0d1d4595a 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -29,7 +29,11 @@
>  */
> 
> #include <linux/kernel.h>
> +#include <linux/clk.h>
> #include <linux/debugfs.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/serdev.h>
> 
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -50,6 +54,9 @@
> #define IBS_TX_IDLE_TIMEOUT_MS		2000
> #define BAUDRATE_SETTLE_TIMEOUT_MS	300
> 
> +/* susclk rate */
> +#define SUSCLK_RATE_32KHZ	32768
> +
> /* HCI_IBS transmit side sleep protocol states */
> enum tx_ibs_states {
> 	HCI_IBS_TX_ASLEEP,
> @@ -111,6 +118,12 @@ struct qca_data {
> 	u64 votes_off;
> };
> 
> +struct qca_serdev {
> +	struct hci_uart	 serdev_hu;
> +	struct gpio_desc *bt_en;
> +	struct clk	 *susclk;
> +};
> +
> static void __serial_clock_on(struct tty_struct *tty)
> {
> 	/* TODO: Some chipset requires to enable UART clock on client
> @@ -386,6 +399,7 @@ static void hci_ibs_wake_retrans_timeout(struct timer_list *t)
> /* Initialize protocol */
> static int qca_open(struct hci_uart *hu)
> {
> +	struct qca_serdev *qcadev;
> 	struct qca_data *qca;
> 
> 	BT_DBG("hu %p qca_open", hu);
> @@ -444,6 +458,13 @@ static int qca_open(struct hci_uart *hu)
> 	timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0);
> 	qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS;
> 
> +	if (hu->serdev) {
> +		serdev_device_open(hu->serdev);
> +
> +		qcadev = serdev_device_get_drvdata(hu->serdev);
> +		gpiod_set_value_cansleep(qcadev->bt_en, 1);
> +	}
> +
> 	BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u",
> 	       qca->tx_idle_delay, qca->wake_retrans);
> 
> @@ -512,6 +533,7 @@ 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);
> @@ -525,6 +547,13 @@ static int qca_close(struct hci_uart *hu)
> 	destroy_workqueue(qca->workqueue);
> 	qca->hu = NULL;
> 
> +	if (hu->serdev) {
> +		serdev_device_close(hu->serdev);
> +
> +		qcadev = serdev_device_get_drvdata(hu->serdev);
> +		gpiod_set_value_cansleep(qcadev->bt_en, 0);
> +	}
> +
> 	kfree_skb(qca->rx_skb);
> 
> 	hu->priv = NULL;
> @@ -885,6 +914,14 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
> 	return 0;
> }
> 
> +static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
> +{
> +	if (hu->serdev)
> +		serdev_device_set_baudrate(hu->serdev, speed);
> +	else
> +		hci_uart_set_baudrate(hu, speed);
> +}
> +
> static int qca_setup(struct hci_uart *hu)
> {
> 	struct hci_dev *hdev = hu->hdev;
> @@ -905,7 +942,7 @@ static int qca_setup(struct hci_uart *hu)
> 		speed = hu->proto->init_speed;
> 
> 	if (speed)
> -		hci_uart_set_baudrate(hu, speed);
> +		host_set_baudrate(hu, speed);
> 
> 	/* Setup user speed if needed */
> 	speed = 0;
> @@ -924,7 +961,7 @@ static int qca_setup(struct hci_uart *hu)
> 				   ret);
> 			return ret;
> 		}
> -		hci_uart_set_baudrate(hu, speed);
> +		host_set_baudrate(hu, speed);
> 	}
> 
> 	/* Setup patch / NVM configurations */
> @@ -958,12 +995,80 @@ static struct hci_uart_proto qca_proto = {
> 	.dequeue	= qca_dequeue,
> };
> 
> +static int qca_serdev_probe(struct serdev_device *serdev)
> +{
> +	struct qca_serdev *qcadev;
> +	int err;
> +
> +	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
> +	if (!qcadev)
> +		return -ENOMEM;
> +
> +	qcadev->serdev_hu.serdev = serdev;
> +	serdev_device_set_drvdata(serdev, qcadev);
> +
> +	qcadev->bt_en = devm_gpiod_get(&serdev->dev, "bt-disable-n",
> +				       GPIOD_OUT_LOW);
> +	if (IS_ERR(qcadev->bt_en)) {
> +		dev_err(&serdev->dev, "failed to acquire bt-disable-n gpio\n");
> +		return PTR_ERR(qcadev->bt_en);
> +	}
> +
> +	qcadev->susclk = devm_clk_get(&serdev->dev, NULL);
> +	if (IS_ERR(qcadev->susclk)) {
> +		dev_err(&serdev->dev, "failed to acquire clk\n");
> +		return PTR_ERR(qcadev->susclk);
> +	}
> +
> +	err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ);
> +	if (err)
> +		return err;
> +
> +	err = clk_prepare_enable(qcadev->susclk);
> +	if (err)
> +		return err;
> +
> +	err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto);
> +	if (err)
> +		clk_disable_unprepare(qcadev->susclk);
> +
> +	return err;
> +}
> +
> +static void qca_serdev_remove(struct serdev_device *serdev)
> +{
> +	struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
> +
> +	hci_uart_unregister_device(&qcadev->serdev_hu);
> +
> +	clk_disable_unprepare(qcadev->susclk);
> +}
> +
> +static const struct of_device_id qca_bluetooth_of_match[] = {
> +	{ .compatible = "qcom,qca6174-bt" },
> +	{ /* sentinel */ }
> +};

so I am fine taking this patch after we agree on the GPIO naming in the DT file, however ..

While looking into the 3-Wire serdev patches to support Realtek devices, I realized that hacking serdev support into hci_uart.ko is actually pretty much a huge mess. It works, but it is also not pretty and makes the code really complicated and spaghetti like.

The nice thing about hci_uart.ko is that all vendor specific stuff is in a separate file (in comparison to btusb.ko), but the not so nice part is that it is largely messy and complicated.

So I have a basic btuart.ko driver ready that speaks only H:4 and serdev. It is clean and tiny and I added already basic Broadcom support to it. So maybe going the path like btusb.ko to select different setup routines is good enough. I need to work out a few kinks in it, but it is a lot cleaner from a code perspective. In addition, I have the basics for a bt3wire.ko driver that will then take H:5 instead of H:4.

I will send them around shortly and maybe it makes sense to go down this path and integrate QCA support there. I currently only have RPi3 on my desk with serdev attached Broadcom chip. So it would needs others testing if this can work.

Regards

Marcel

Re: [PATCH v4 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Thierry Escande]

Hi Marcel,

On 14/03/2018 20:51, Marcel Holtmann wrote:
> Hi Bjorn,
> 
>>>>>> +		bt-disable-n-gpios = <&pm8994_gpios 19 GPIO_ACTIVE_HIGH>;
>>>>>
>>>>> can we use a common name here. I think that Nokia and Broadcom drivers
>>>>> define one. And if this is the enable/shutdown GPIO, we should name it
>>>>> consistently across all manufacturers. It essentially does the same on
>>>>> Bluetooth UART chips no matter what chip is behind them.
>>>>>
>>>>
>>>> Broadcomm has a device-wakup-gpios and Nokia has bluetooth-wakup-gpios.
>>>> It might be that these behave in the same way, but from the description
>>>> they only trigger the wakeup.
>>>
>>> that is something that we might need to start fixing. I really prefer
>>> if we name the GPIOs a bit more consistent.
>>>
>>>> The reason for the proposed naming here is that the pin is named
>>>> "BT_DISABLE_N" in the datasheet.
>>>
>>> That is not a reason I buy. So the next board comes around that labels
>>> it in the data sheet BT_DISABLE_YEAH_SUPER_GREAT and you send me a
>>> patch to the driver to look for that name. If the GPIO does the same
>>> thing, I couldn’t care less what the data sheet says. That might be
>>> a comment in the DT file, but it should not pollute the driver code.
>>>
>>
>> BT_DISABLE_N is the name of this pin in the datasheet of the QCA chip,
>> not on the board, so this name is the same regardless of what you name
>> the line or gpio your board connect it to.
> 
> and QCA chip v1 and QCA chip v2 will use the same driver and same firmware loading mechanism. So why do we have to add a new GPIO naming if they decide to change the name in the data sheet. With Bluetooth it is pretty much all the same. Every UART chip has a shutdown/reset GPIO to enable/disable the chip behind the UART.
> 
>>> A new board should not require driver changes, you just ship a new DT
>>> for that board and an existing driver hopefully just does the job. No
>>> matter how someone named a GPIO in a piece of paper.
>>>
>>
>> I totally agree with the fact that the board should not affect the
>> naming of the gpio in the driver. But I do enjoy when we refer to pins
>> by their real name - instead of having to guess which pin in the _chip_
>> specification the driver actually refer to.
>>
>>
>> That said, what name would you prefer for this?
>>
>> Afaict this is not "wakeup" and there are a few extra steps between the
>> disabled state and "bluetooth is enabled", so "enable" feels slightly
>> wrong. And it probably should be "bluetooth" and not just "device" as
>> this refers to a pin on a WiFi/BT combo chip.
> 
> The Broadcom side called it shutdown GPIO, it is essentially the shutdown/reset GPIO or power on/off GPIO. Personally I do not care what it is named, but it will be all the same for all Bluetooth chips. Take a poll from Broadcom, Intel, Realtek and Qualcomm and you can pick a reasonable common name.

The Nokia driver has "bluetooth-wakeup" gpio. The Broadcom one has 
"device-wakeup" and "shutdown". The "shutdown" gpio is set to its active 
state to power on the chip which sounds reversed logic. Same for the 
"bt-disable-n" gpio in the Qualcomm driver, configured as ACTIVE_HIGH, 
and which is set to 1 to enable it...

So for consistency, naming it as "shutdown" to stick to the bcm driver 
but it should be configured as ACTIVE_LOW in the dts so we actually do a 
gpiod_set_value(0) to un-shutdown it. Does that sound ok?

Regards,
Thierry

> For the wakeup GPIOs, these are different and depend on if there is some low-power mode provided. You would need to check the data sheet to see if they provide more advanced low-power state handling.
> 
> Regards
> 
> Marcel
> 

Re: [PATCH v4 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Sebastian Reichel]

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

Hi Thierry,

On Thu, Mar 15, 2018 at 12:07:44PM +0100, Thierry Escande wrote:
> > > BT_DISABLE_N is the name of this pin in the datasheet of the QCA chip,
> > > not on the board, so this name is the same regardless of what you name
> > > the line or gpio your board connect it to.
> > 
> > and QCA chip v1 and QCA chip v2 will use the same driver and
> > same firmware loading mechanism. So why do we have to add a new
> > GPIO naming if they decide to change the name in the data sheet.
> > With Bluetooth it is pretty much all the same. Every UART chip
> > has a shutdown/reset GPIO to enable/disable the chip behind the
> > UART.
> > 
> > > > A new board should not require driver changes, you just ship a new DT
> > > > for that board and an existing driver hopefully just does the job. No
> > > > matter how someone named a GPIO in a piece of paper.
> > > > 
> > > 
> > > I totally agree with the fact that the board should not affect the
> > > naming of the gpio in the driver. But I do enjoy when we refer to pins
> > > by their real name - instead of having to guess which pin in the _chip_
> > > specification the driver actually refer to.
> > > 
> > > 
> > > That said, what name would you prefer for this?
> > > 
> > > Afaict this is not "wakeup" and there are a few extra steps between the
> > > disabled state and "bluetooth is enabled", so "enable" feels slightly
> > > wrong. And it probably should be "bluetooth" and not just "device" as
> > > this refers to a pin on a WiFi/BT combo chip.
> > 
> > The Broadcom side called it shutdown GPIO, it is essentially the
> > shutdown/reset GPIO or power on/off GPIO. Personally I do not
> > care what it is named, but it will be all the same for all
> > Bluetooth chips. Take a poll from Broadcom, Intel, Realtek and
> > Qualcomm and you can pick a reasonable common name.
> 
> The Nokia driver has "bluetooth-wakeup" gpio. The Broadcom one has
> "device-wakeup" and "shutdown". The "shutdown" gpio is set to its active
> state to power on the chip which sounds reversed logic. Same for the
> "bt-disable-n" gpio in the Qualcomm driver, configured as ACTIVE_HIGH, and
> which is set to 1 to enable it...
>
> So for consistency, naming it as "shutdown" to stick to the bcm driver but
> it should be configured as ACTIVE_LOW in the dts so we actually do a
> gpiod_set_value(0) to un-shutdown it. Does that sound ok?

FWIW you picked the wrong gpio from the nokia bluetooth binding. The
gpio for shutdown would be "reset". The "bluetooth-wakeup" is
required for normal operation to exit idle mode. The "reset" name
used by the nokia binding is quite common for DT:

Documentation/devicetree/bindings $ git grep reset-gpios | wc -l
212

I guess it only makes sense when the device is actually being
reset, though (i.e. for Nokia the settings are back to defaults
and you need to re-upload the FW).

-- Sebastian

> Regards,
> Thierry
> 
> > For the wakeup GPIOs, these are different and depend on if there
> > is some low-power mode provided. You would need to check the
> > data sheet to see if they provide more advanced low-power state
> > handling.
> > 
> > Regards
> > 
> > Marcel
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Marcel Holtmann]

Hi Sebastian,

>>>> BT_DISABLE_N is the name of this pin in the datasheet of the QCA chip,
>>>> not on the board, so this name is the same regardless of what you name
>>>> the line or gpio your board connect it to.
>>> 
>>> and QCA chip v1 and QCA chip v2 will use the same driver and
>>> same firmware loading mechanism. So why do we have to add a new
>>> GPIO naming if they decide to change the name in the data sheet.
>>> With Bluetooth it is pretty much all the same. Every UART chip
>>> has a shutdown/reset GPIO to enable/disable the chip behind the
>>> UART.
>>> 
>>>>> A new board should not require driver changes, you just ship a new DT
>>>>> for that board and an existing driver hopefully just does the job. No
>>>>> matter how someone named a GPIO in a piece of paper.
>>>>> 
>>>> 
>>>> I totally agree with the fact that the board should not affect the
>>>> naming of the gpio in the driver. But I do enjoy when we refer to pins
>>>> by their real name - instead of having to guess which pin in the _chip_
>>>> specification the driver actually refer to.
>>>> 
>>>> 
>>>> That said, what name would you prefer for this?
>>>> 
>>>> Afaict this is not "wakeup" and there are a few extra steps between the
>>>> disabled state and "bluetooth is enabled", so "enable" feels slightly
>>>> wrong. And it probably should be "bluetooth" and not just "device" as
>>>> this refers to a pin on a WiFi/BT combo chip.
>>> 
>>> The Broadcom side called it shutdown GPIO, it is essentially the
>>> shutdown/reset GPIO or power on/off GPIO. Personally I do not
>>> care what it is named, but it will be all the same for all
>>> Bluetooth chips. Take a poll from Broadcom, Intel, Realtek and
>>> Qualcomm and you can pick a reasonable common name.
>> 
>> The Nokia driver has "bluetooth-wakeup" gpio. The Broadcom one has
>> "device-wakeup" and "shutdown". The "shutdown" gpio is set to its active
>> state to power on the chip which sounds reversed logic. Same for the
>> "bt-disable-n" gpio in the Qualcomm driver, configured as ACTIVE_HIGH, and
>> which is set to 1 to enable it...
>> 
>> So for consistency, naming it as "shutdown" to stick to the bcm driver but
>> it should be configured as ACTIVE_LOW in the dts so we actually do a
>> gpiod_set_value(0) to un-shutdown it. Does that sound ok?
> 
> FWIW you picked the wrong gpio from the nokia bluetooth binding. The
> gpio for shutdown would be "reset". The "bluetooth-wakeup" is
> required for normal operation to exit idle mode. The "reset" name
> used by the nokia binding is quite common for DT:
> 
> Documentation/devicetree/bindings $ git grep reset-gpios | wc -l
> 212
> 
> I guess it only makes sense when the device is actually being
> reset, though (i.e. for Nokia the settings are back to defaults
> and you need to re-upload the FW).

that is actually a good point. I like to differentiate between a shutdown GPIO (and we can argue about the inversion here) and the reset GPIO. If we loose the firmware and the programmed BD_ADDR, then it is a hard-reset, otherwise it is shutdown/power GPIO.

So can we agree on how we name the hard-reset, shutdown, wakeup etc. GPIOs for Bluetooth UART devices?

Regards

Marcel

Re: [PATCH v4 2/3] dt-bindings: net: bluetooth: Add qualcomm-bluetooth

[Rob Herring]

On Thu, Mar 15, 2018 at 02:46:45PM +0100, Sebastian Reichel wrote:
> Hi Thierry,
> 
> On Thu, Mar 15, 2018 at 12:07:44PM +0100, Thierry Escande wrote:
> > > > BT_DISABLE_N is the name of this pin in the datasheet of the QCA chip,
> > > > not on the board, so this name is the same regardless of what you name
> > > > the line or gpio your board connect it to.
> > > 
> > > and QCA chip v1 and QCA chip v2 will use the same driver and
> > > same firmware loading mechanism. So why do we have to add a new
> > > GPIO naming if they decide to change the name in the data sheet.
> > > With Bluetooth it is pretty much all the same. Every UART chip
> > > has a shutdown/reset GPIO to enable/disable the chip behind the
> > > UART.
> > > 
> > > > > A new board should not require driver changes, you just ship a new DT
> > > > > for that board and an existing driver hopefully just does the job. No
> > > > > matter how someone named a GPIO in a piece of paper.
> > > > > 
> > > > 
> > > > I totally agree with the fact that the board should not affect the
> > > > naming of the gpio in the driver. But I do enjoy when we refer to pins
> > > > by their real name - instead of having to guess which pin in the _chip_
> > > > specification the driver actually refer to.
> > > > 
> > > > 
> > > > That said, what name would you prefer for this?
> > > > 
> > > > Afaict this is not "wakeup" and there are a few extra steps between the
> > > > disabled state and "bluetooth is enabled", so "enable" feels slightly
> > > > wrong. And it probably should be "bluetooth" and not just "device" as
> > > > this refers to a pin on a WiFi/BT combo chip.
> > > 
> > > The Broadcom side called it shutdown GPIO, it is essentially the
> > > shutdown/reset GPIO or power on/off GPIO. Personally I do not
> > > care what it is named, but it will be all the same for all
> > > Bluetooth chips. Take a poll from Broadcom, Intel, Realtek and
> > > Qualcomm and you can pick a reasonable common name.
> > 
> > The Nokia driver has "bluetooth-wakeup" gpio. The Broadcom one has
> > "device-wakeup" and "shutdown". The "shutdown" gpio is set to its active
> > state to power on the chip which sounds reversed logic. Same for the
> > "bt-disable-n" gpio in the Qualcomm driver, configured as ACTIVE_HIGH, and
> > which is set to 1 to enable it...
> >
> > So for consistency, naming it as "shutdown" to stick to the bcm driver but
> > it should be configured as ACTIVE_LOW in the dts so we actually do a
> > gpiod_set_value(0) to un-shutdown it. Does that sound ok?
> 
> FWIW you picked the wrong gpio from the nokia bluetooth binding. The
> gpio for shutdown would be "reset". The "bluetooth-wakeup" is
> required for normal operation to exit idle mode. The "reset" name
> used by the nokia binding is quite common for DT:
> 
> Documentation/devicetree/bindings $ git grep reset-gpios | wc -l
> 212
> 
> I guess it only makes sense when the device is actually being
> reset, though (i.e. for Nokia the settings are back to defaults
> and you need to re-upload the FW).

The standardish names are reset-gpios, enable-gpios, and powerdown-gpios. 
Pick one or some of those.

Rob