linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support
@ 2019-11-18 19:21 Abhishek Pandit-Subedi
  2019-11-18 19:21 ` [PATCH v6 1/4] Bluetooth: hci_bcm: Disallow set_baudrate for BCM4354 Abhishek Pandit-Subedi
                   ` (4 more replies)
  0 siblings, 5 replies; 25+ messages in thread
From: Abhishek Pandit-Subedi @ 2019-11-18 19:21 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Rob Herring
  Cc: linux-bluetooth, dianders, Abhishek Pandit-Subedi, devicetree,
	David S. Miller, netdev, linux-kernel, Ondrej Jirman,
	Mark Rutland, Chen-Yu Tsai


While adding support for the BCM4354, I discovered a few more things
that weren't working as they should have.

First, we disallow serdev from setting the baudrate on BCM4354. Serdev
sets the oper_speed first before calling hu->setup() in
hci_uart_setup(). On the BCM4354, this results in bcm_setup() failing
when the hci reset times out.

Next, we add support for setting the PCM parameters, which consists of
a pair of vendor specific opcodes to set the pcm parameters. The
documentation for these params are available in the brcm_patchram_plus
package (i.e. https://github.com/balena-os/brcm_patchram_plus). This is
necessary for PCM to work properly.

All changes were tested with rk3288-veyron-minnie.dts.


Changes in v6:
- Added btbcm_read_pcm_int_params and change pcm params to first read
  the pcm params before setting it

Changes in v5:
- Rename parameters to bt-* and read as integer instead of bytestring
- Update documentation with defaults and put values in header
- Changed patch order

Changes in v4:
- Fix incorrect function name in hci_bcm

Changes in v3:
- Change disallow baudrate setting to return -EBUSY if called before
  ready. bcm_proto is no longer modified and is back to being const.
- Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
- Changed brcm,sco-routing to brcm,bt-sco-routing

Changes in v2:
- Use match data to disallow baudrate setting
- Parse pcm parameters by name instead of as a byte string
- Fix prefix for dt-bindings commit

Abhishek Pandit-Subedi (4):
  Bluetooth: hci_bcm: Disallow set_baudrate for BCM4354
  Bluetooth: btbcm: Support pcm configuration
  dt-bindings: net: broadcom-bluetooth: Add pcm config
  Bluetooth: hci_bcm: Support pcm params in dts

 .../bindings/net/broadcom-bluetooth.txt       | 16 ++++
 drivers/bluetooth/btbcm.c                     | 47 ++++++++++
 drivers/bluetooth/btbcm.h                     | 16 ++++
 drivers/bluetooth/hci_bcm.c                   | 88 ++++++++++++++++++-
 include/dt-bindings/bluetooth/brcm.h          | 32 +++++++
 5 files changed, 197 insertions(+), 2 deletions(-)
 create mode 100644 include/dt-bindings/bluetooth/brcm.h

-- 
2.24.0.432.g9d3f5f5b63-goog


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

* [PATCH v6 1/4] Bluetooth: hci_bcm: Disallow set_baudrate for BCM4354
  2019-11-18 19:21 [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support Abhishek Pandit-Subedi
@ 2019-11-18 19:21 ` Abhishek Pandit-Subedi
  2019-11-19  5:25   ` Marcel Holtmann
  2019-11-18 19:21 ` [PATCH v6 2/4] Bluetooth: btbcm: Support pcm configuration Abhishek Pandit-Subedi
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 25+ messages in thread
From: Abhishek Pandit-Subedi @ 2019-11-18 19:21 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Rob Herring
  Cc: linux-bluetooth, dianders, Abhishek Pandit-Subedi, linux-kernel

Without updating the patchram, the BCM4354 does not support a higher
operating speed. The normal bcm_setup follows the correct order
(init_speed, patchram and then oper_speed) but the serdev driver will
set the operating speed before calling the hu->setup function. Thus,
for the BCM4354, don't set the operating speed before patchram.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/bluetooth/hci_bcm.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 0f851c0dde7f..ee40003008d8 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -47,6 +47,14 @@
 
 #define BCM_NUM_SUPPLIES 2
 
+/**
+ * struct bcm_device_data - device specific data
+ * @no_early_set_baudrate: Disallow set baudrate before driver setup()
+ */
+struct bcm_device_data {
+	bool	no_early_set_baudrate;
+};
+
 /**
  * struct bcm_device - device driver resources
  * @serdev_hu: HCI UART controller struct
@@ -79,6 +87,7 @@
  * @hu: pointer to HCI UART controller struct,
  *	used to disable flow control during runtime suspend and system sleep
  * @is_suspended: whether flow control is currently disabled
+ * @no_early_set_baudrate: don't set_baudrate before setup()
  */
 struct bcm_device {
 	/* Must be the first member, hci_serdev.c expects this. */
@@ -112,6 +121,7 @@ struct bcm_device {
 	struct hci_uart		*hu;
 	bool			is_suspended;
 #endif
+	bool			no_early_set_baudrate;
 };
 
 /* generic bcm uart resources */
@@ -447,7 +457,13 @@ static int bcm_open(struct hci_uart *hu)
 	if (bcm->dev) {
 		hci_uart_set_flow_control(hu, true);
 		hu->init_speed = bcm->dev->init_speed;
-		hu->oper_speed = bcm->dev->oper_speed;
+
+		/* If oper_speed is set, ldisc/serdev will set the baudrate
+		 * before calling setup()
+		 */
+		if (!bcm->dev->no_early_set_baudrate)
+			hu->oper_speed = bcm->dev->oper_speed;
+
 		err = bcm_gpio_set_power(bcm->dev, true);
 		hci_uart_set_flow_control(hu, false);
 		if (err)
@@ -565,6 +581,8 @@ static int bcm_setup(struct hci_uart *hu)
 	/* Operational speed if any */
 	if (hu->oper_speed)
 		speed = hu->oper_speed;
+	else if (bcm->dev && bcm->dev->oper_speed)
+		speed = bcm->dev->oper_speed;
 	else if (hu->proto->oper_speed)
 		speed = hu->proto->oper_speed;
 	else
@@ -1374,6 +1392,7 @@ static struct platform_driver bcm_driver = {
 static int bcm_serdev_probe(struct serdev_device *serdev)
 {
 	struct bcm_device *bcmdev;
+	const struct bcm_device_data *data;
 	int err;
 
 	bcmdev = devm_kzalloc(&serdev->dev, sizeof(*bcmdev), GFP_KERNEL);
@@ -1408,6 +1427,10 @@ static int bcm_serdev_probe(struct serdev_device *serdev)
 	if (err)
 		dev_err(&serdev->dev, "Failed to power down\n");
 
+	data = device_get_match_data(bcmdev->dev);
+	if (data)
+		bcmdev->no_early_set_baudrate = data->no_early_set_baudrate;
+
 	return hci_uart_register_device(&bcmdev->serdev_hu, &bcm_proto);
 }
 
@@ -1419,12 +1442,16 @@ static void bcm_serdev_remove(struct serdev_device *serdev)
 }
 
 #ifdef CONFIG_OF
+struct bcm_device_data bcm4354_device_data = {
+	.no_early_set_baudrate = true,
+};
+
 static const struct of_device_id bcm_bluetooth_of_match[] = {
 	{ .compatible = "brcm,bcm20702a1" },
 	{ .compatible = "brcm,bcm4345c5" },
 	{ .compatible = "brcm,bcm4330-bt" },
 	{ .compatible = "brcm,bcm43438-bt" },
-	{ .compatible = "brcm,bcm43540-bt" },
+	{ .compatible = "brcm,bcm43540-bt", .data = &bcm4354_device_data },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, bcm_bluetooth_of_match);
-- 
2.24.0.432.g9d3f5f5b63-goog


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

* [PATCH v6 2/4] Bluetooth: btbcm: Support pcm configuration
  2019-11-18 19:21 [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support Abhishek Pandit-Subedi
  2019-11-18 19:21 ` [PATCH v6 1/4] Bluetooth: hci_bcm: Disallow set_baudrate for BCM4354 Abhishek Pandit-Subedi
@ 2019-11-18 19:21 ` Abhishek Pandit-Subedi
  2019-11-19  5:35   ` Marcel Holtmann
  2019-11-18 19:21 ` [PATCH v6 3/4] dt-bindings: net: broadcom-bluetooth: Add pcm config Abhishek Pandit-Subedi
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 25+ messages in thread
From: Abhishek Pandit-Subedi @ 2019-11-18 19:21 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Rob Herring
  Cc: linux-bluetooth, dianders, Abhishek Pandit-Subedi, linux-kernel

Add BCM vendor specific command to configure PCM parameters. The new
vendor opcode allows us to set the sco routing, the pcm interface rate,
and a few other pcm specific options (frame sync, sync mode, and clock
mode). See broadcom-bluetooth.txt in Documentation for more information
about valid values for those settings.

Here is an example trace where this opcode was used to configure
a BCM4354:

        < HCI Command: Vendor (0x3f|0x001c) plen 5
                01 02 00 01 01
        > HCI Event: Command Complete (0x0e) plen 4
        Vendor (0x3f|0x001c) ncmd 1
                Status: Success (0x00)

We can read back the values as well with ocf 0x001d to confirm the
values that were set:
        $ hcitool cmd 0x3f 0x001d
        < HCI Command: ogf 0x3f, ocf 0x001d, plen 0
        > HCI Event: 0x0e plen 9
        01 1D FC 00 01 02 00 01 01

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/bluetooth/btbcm.c | 47 +++++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/btbcm.h | 16 +++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index 2d2e6d862068..df90841d29c5 100644
--- a/drivers/bluetooth/btbcm.c
+++ b/drivers/bluetooth/btbcm.c
@@ -105,6 +105,53 @@ int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
 }
 EXPORT_SYMBOL_GPL(btbcm_set_bdaddr);
 
+int btbcm_read_pcm_int_params(struct hci_dev *hdev,
+			      struct bcm_set_pcm_int_params *int_params)
+{
+	struct sk_buff *skb;
+	int err = 0;
+
+	skb = __hci_cmd_sync(hdev, 0xfc1d, 5, int_params, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		err = PTR_ERR(skb);
+		bt_dev_err(hdev, "BCM: Read PCM int params failed (%d)", err);
+		return err;
+	}
+
+	if (!skb->data[0] && skb->len == sizeof(*int_params) + 1) {
+		memcpy(int_params, &skb->data[1], sizeof(*int_params));
+	} else {
+		bt_dev_err(hdev,
+			   "BCM: Read PCM int params failed (%d), Length (%d)",
+			   skb->data[0], skb->len);
+		err = -EINVAL;
+	}
+
+	kfree_skb(skb);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(btbcm_read_pcm_int_params);
+
+int btbcm_write_pcm_int_params(struct hci_dev *hdev,
+			       const struct bcm_set_pcm_int_params *int_params)
+{
+	struct sk_buff *skb;
+	int err;
+
+	/* Vendor ocf 0x001c sets the pcm parameters and 0x001d reads it */
+	skb = __hci_cmd_sync(hdev, 0xfc1c, 5, int_params, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		err = PTR_ERR(skb);
+		bt_dev_err(hdev, "BCM: Write PCM int params failed (%d)", err);
+		return err;
+	}
+	kfree_skb(skb);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(btbcm_write_pcm_int_params);
+
 int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw)
 {
 	const struct hci_command_hdr *cmd;
diff --git a/drivers/bluetooth/btbcm.h b/drivers/bluetooth/btbcm.h
index d204be8a84bf..29ca3956ea1c 100644
--- a/drivers/bluetooth/btbcm.h
+++ b/drivers/bluetooth/btbcm.h
@@ -54,6 +54,10 @@ struct bcm_set_pcm_format_params {
 int btbcm_check_bdaddr(struct hci_dev *hdev);
 int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr);
 int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw);
+int btbcm_read_pcm_int_params(struct hci_dev *hdev,
+			      struct bcm_set_pcm_int_params *int_params);
+int btbcm_write_pcm_int_params(struct hci_dev *hdev,
+			       const struct bcm_set_pcm_int_params *int_params);
 
 int btbcm_setup_patchram(struct hci_dev *hdev);
 int btbcm_setup_apple(struct hci_dev *hdev);
@@ -74,6 +78,18 @@ static inline int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
 	return -EOPNOTSUPP;
 }
 
+int btbcm_read_pcm_int_params(struct hci_dev *hdev,
+			      struct bcm_set_pcm_int_params *int_params)
+{
+	return -EOPNOTSUPP;
+}
+
+int btbcm_write_pcm_int_params(struct hci_dev *hdev,
+			       const struct bcm_set_pcm_int_params *int_params)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw)
 {
 	return -EOPNOTSUPP;
-- 
2.24.0.432.g9d3f5f5b63-goog


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

* [PATCH v6 3/4] dt-bindings: net: broadcom-bluetooth: Add pcm config
  2019-11-18 19:21 [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support Abhishek Pandit-Subedi
  2019-11-18 19:21 ` [PATCH v6 1/4] Bluetooth: hci_bcm: Disallow set_baudrate for BCM4354 Abhishek Pandit-Subedi
  2019-11-18 19:21 ` [PATCH v6 2/4] Bluetooth: btbcm: Support pcm configuration Abhishek Pandit-Subedi
@ 2019-11-18 19:21 ` Abhishek Pandit-Subedi
  2019-11-19  5:39   ` Marcel Holtmann
  2019-11-21 21:29   ` Rob Herring
  2019-11-18 19:21 ` [PATCH v6 4/4] Bluetooth: hci_bcm: Support pcm params in dts Abhishek Pandit-Subedi
  2019-11-23 10:04 ` [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support Marcel Holtmann
  4 siblings, 2 replies; 25+ messages in thread
From: Abhishek Pandit-Subedi @ 2019-11-18 19:21 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Rob Herring
  Cc: linux-bluetooth, dianders, Abhishek Pandit-Subedi, devicetree,
	David S. Miller, netdev, linux-kernel, Ondrej Jirman,
	Mark Rutland, Chen-Yu Tsai

Add documentation for pcm parameters.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 .../bindings/net/broadcom-bluetooth.txt       | 16 ++++++++++
 include/dt-bindings/bluetooth/brcm.h          | 32 +++++++++++++++++++
 2 files changed, 48 insertions(+)
 create mode 100644 include/dt-bindings/bluetooth/brcm.h

diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
index c749dc297624..8561e4684378 100644
--- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
+++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
@@ -29,10 +29,20 @@ Optional properties:
    - "lpo": external low power 32.768 kHz clock
  - vbat-supply: phandle to regulator supply for VBAT
  - vddio-supply: phandle to regulator supply for VDDIO
+ - brcm,bt-sco-routing: PCM, Transport, Codec, I2S
+ - brcm,bt-pcm-interface-rate: 128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps
+ - brcm,bt-pcm-frame-type: short, long
+ - brcm,bt-pcm-sync-mode: slave, master
+ - brcm,bt-pcm-clock-mode: slave, master
 
+See include/dt-bindings/bluetooth/brcm.h for SCO/PCM parameters. The default
+value for all these values are 0 (except for brcm,bt-sco-routing which requires
+a value) if you choose to leave it out.
 
 Example:
 
+#include <dt-bindings/bluetooth/brcm.h>
+
 &uart2 {
        pinctrl-names = "default";
        pinctrl-0 = <&uart2_pins>;
@@ -40,5 +50,11 @@ Example:
        bluetooth {
                compatible = "brcm,bcm43438-bt";
                max-speed = <921600>;
+
+               brcm,bt-sco-routing        = <BRCM_SCO_ROUTING_TRANSPORT>;
+               brcm,bt-pcm-interface-rate = <BRCM_PCM_IF_RATE_512KBPS>;
+               brcm,bt-pcm-frame-type     = <BRCM_PCM_FRAME_TYPE_SHORT>;
+               brcm,bt-pcm-sync-mode      = <BRCM_PCM_SYNC_MODE_MASTER>;
+               brcm,bt-pcm-clock-mode     = <BRCM_PCM_CLOCK_MODE_MASTER>;
        };
 };
diff --git a/include/dt-bindings/bluetooth/brcm.h b/include/dt-bindings/bluetooth/brcm.h
new file mode 100644
index 000000000000..8b86f90d7dd2
--- /dev/null
+++ b/include/dt-bindings/bluetooth/brcm.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
+/*
+ * This header provides constants for Broadcom bluetooth dt-bindings.
+ */
+#ifndef _DT_BINDINGS_BLUETOOTH_BRCM_H
+#define _DT_BINDINGS_BLUETOOTH_BRCM_H
+
+#define BRCM_BT_SCO_ROUTING_PCM			0
+#define BRCM_BT_SCO_ROUTING_TRANSPORT		1
+#define BRCM_BT_SCO_ROUTING_CODEC		2
+#define BRCM_BT_SCO_ROUTING_I2S			3
+
+/* Default is 128KBPs */
+#define BRCM_BT_PCM_INTERFACE_RATE_128KBPS	0
+#define BRCM_BT_PCM_INTERFACE_RATE_256KBPS	1
+#define BRCM_BT_PCM_INTERFACE_RATE_512KBPS	2
+#define BRCM_BT_PCM_INTERFACE_RATE_1024KBPS	3
+#define BRCM_BT_PCM_INTERFACE_RATE_2048KBPS	4
+
+/* Default should be short */
+#define BRCM_BT_PCM_FRAME_TYPE_SHORT		0
+#define BRCM_BT_PCM_FRAME_TYPE_LONG		1
+
+/* Default should be master */
+#define BRCM_BT_PCM_SYNC_MODE_SLAVE		0
+#define BRCM_BT_PCM_SYNC_MODE_MASTER		1
+
+/* Default should be master */
+#define BRCM_BT_PCM_CLOCK_MODE_SLAVE		0
+#define BRCM_BT_PCM_CLOCK_MODE_MASTER		1
+
+#endif /* _DT_BINDINGS_BLUETOOTH_BRCM_H */
-- 
2.24.0.432.g9d3f5f5b63-goog


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

* [PATCH v6 4/4] Bluetooth: hci_bcm: Support pcm params in dts
  2019-11-18 19:21 [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support Abhishek Pandit-Subedi
                   ` (2 preceding siblings ...)
  2019-11-18 19:21 ` [PATCH v6 3/4] dt-bindings: net: broadcom-bluetooth: Add pcm config Abhishek Pandit-Subedi
@ 2019-11-18 19:21 ` Abhishek Pandit-Subedi
  2019-11-19  5:44   ` Marcel Holtmann
  2019-11-23 10:04 ` [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support Marcel Holtmann
  4 siblings, 1 reply; 25+ messages in thread
From: Abhishek Pandit-Subedi @ 2019-11-18 19:21 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg, Rob Herring
  Cc: linux-bluetooth, dianders, Abhishek Pandit-Subedi, linux-kernel

BCM chips may require configuration of PCM to operate correctly and
there is a vendor specific HCI command to do this. Add support in the
hci_bcm driver to parse this from devicetree and configure the chip.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

Changes in v6:
- Added btbcm_read_pcm_int_params and change pcm params to first read
  the pcm params before setting it

Changes in v5:
- Rename parameters to bt-* and read as integer instead of bytestring
- Update documentation with defaults and put values in header
- Changed patch order

Changes in v4:
- Fix incorrect function name in hci_bcm

Changes in v3:
- Change disallow baudrate setting to return -EBUSY if called before
  ready. bcm_proto is no longer modified and is back to being const.
- Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
- Changed brcm,sco-routing to brcm,bt-sco-routing

Changes in v2:
- Use match data to disallow baudrate setting
- Parse pcm parameters by name instead of as a byte string
- Fix prefix for dt-bindings commit

 drivers/bluetooth/hci_bcm.c | 57 +++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index ee40003008d8..2ce3fac2c5dd 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -25,6 +25,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/serdev.h>
 
+#include <dt-bindings/bluetooth/brcm.h>
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 
@@ -88,6 +89,7 @@ struct bcm_device_data {
  *	used to disable flow control during runtime suspend and system sleep
  * @is_suspended: whether flow control is currently disabled
  * @no_early_set_baudrate: don't set_baudrate before setup()
+ * @pcm_params: PCM and routing parameters
  */
 struct bcm_device {
 	/* Must be the first member, hci_serdev.c expects this. */
@@ -122,6 +124,8 @@ struct bcm_device {
 	bool			is_suspended;
 #endif
 	bool			no_early_set_baudrate;
+
+	struct bcm_set_pcm_int_params	pcm_params;
 };
 
 /* generic bcm uart resources */
@@ -541,6 +545,7 @@ static int bcm_flush(struct hci_uart *hu)
 static int bcm_setup(struct hci_uart *hu)
 {
 	struct bcm_data *bcm = hu->priv;
+	struct bcm_set_pcm_int_params p;
 	char fw_name[64];
 	const struct firmware *fw;
 	unsigned int speed;
@@ -594,6 +599,31 @@ static int bcm_setup(struct hci_uart *hu)
 			host_set_baudrate(hu, speed);
 	}
 
+	/* PCM parameters if any*/
+	err = btbcm_read_pcm_int_params(hu->hdev, &p);
+	if (!err) {
+		if (bcm->dev->pcm_params.routing == 0xff)
+			bcm->dev->pcm_params.routing = p.routing;
+		if (bcm->dev->pcm_params.rate == 0xff)
+			bcm->dev->pcm_params.rate = p.rate;
+		if (bcm->dev->pcm_params.frame_sync == 0xff)
+			bcm->dev->pcm_params.frame_sync = p.frame_sync;
+		if (bcm->dev->pcm_params.sync_mode == 0xff)
+			bcm->dev->pcm_params.sync_mode = p.sync_mode;
+		if (bcm->dev->pcm_params.clock_mode == 0xff)
+			bcm->dev->pcm_params.clock_mode = p.clock_mode;
+
+		/* Write only when there are changes */
+		if (memcmp(&p, &bcm->dev->pcm_params, sizeof(p)))
+			err = btbcm_write_pcm_int_params(hu->hdev,
+							 &bcm->dev->pcm_params);
+
+		if (err)
+			bt_dev_warn(hu->hdev, "BCM: Write pcm params failed (%d)",
+				    err);
+	} else
+		bt_dev_warn(hu->hdev, "BCM: Read pcm params failed (%d)", err);
+
 finalize:
 	release_firmware(fw);
 
@@ -1128,9 +1158,36 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 }
 #endif /* CONFIG_ACPI */
 
+static int property_read_u8(struct device *dev, const char *prop, u8 *value)
+{
+	int err;
+	u32 tmp;
+
+	err = device_property_read_u32(dev, prop, &tmp);
+
+	if (!err)
+		*value = (u8)tmp;
+
+	return err;
+}
+
 static int bcm_of_probe(struct bcm_device *bdev)
 {
 	device_property_read_u32(bdev->dev, "max-speed", &bdev->oper_speed);
+
+	memset(&bdev->pcm_params, 0xff, sizeof(bdev->pcm_params));
+
+	property_read_u8(bdev->dev, "brcm,bt-sco-routing",
+			 &bdev->pcm_params.routing);
+	property_read_u8(bdev->dev, "brcm,bt-pcm-interface-rate",
+			 &bdev->pcm_params.rate);
+	property_read_u8(bdev->dev, "brcm,bt-pcm-frame-type",
+			 &bdev->pcm_params.frame_sync);
+	property_read_u8(bdev->dev, "brcm,bt-pcm-sync-mode",
+			 &bdev->pcm_params.sync_mode);
+	property_read_u8(bdev->dev, "brcm,bt-pcm-clock-mode",
+			 &bdev->pcm_params.clock_mode);
+
 	return 0;
 }
 
-- 
2.24.0.432.g9d3f5f5b63-goog


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

* Re: [PATCH v6 1/4] Bluetooth: hci_bcm: Disallow set_baudrate for BCM4354
  2019-11-18 19:21 ` [PATCH v6 1/4] Bluetooth: hci_bcm: Disallow set_baudrate for BCM4354 Abhishek Pandit-Subedi
@ 2019-11-19  5:25   ` Marcel Holtmann
  0 siblings, 0 replies; 25+ messages in thread
From: Marcel Holtmann @ 2019-11-19  5:25 UTC (permalink / raw)
  To: Abhishek Pandit-Subedi
  Cc: Johan Hedberg, Rob Herring, linux-bluetooth, dianders, linux-kernel

Hi Abhishek,

> Without updating the patchram, the BCM4354 does not support a higher
> operating speed. The normal bcm_setup follows the correct order
> (init_speed, patchram and then oper_speed) but the serdev driver will
> set the operating speed before calling the hu->setup function. Thus,
> for the BCM4354, don't set the operating speed before patchram.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> 
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
> drivers/bluetooth/hci_bcm.c | 31 +++++++++++++++++++++++++++++--
> 1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 0f851c0dde7f..ee40003008d8 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -47,6 +47,14 @@
> 
> #define BCM_NUM_SUPPLIES 2
> 
> +/**
> + * struct bcm_device_data - device specific data
> + * @no_early_set_baudrate: Disallow set baudrate before driver setup()
> + */
> +struct bcm_device_data {
> +	bool	no_early_set_baudrate;
> +};
> +
> /**
>  * struct bcm_device - device driver resources
>  * @serdev_hu: HCI UART controller struct
> @@ -79,6 +87,7 @@
>  * @hu: pointer to HCI UART controller struct,
>  *	used to disable flow control during runtime suspend and system sleep
>  * @is_suspended: whether flow control is currently disabled
> + * @no_early_set_baudrate: don't set_baudrate before setup()
>  */
> struct bcm_device {
> 	/* Must be the first member, hci_serdev.c expects this. */
> @@ -112,6 +121,7 @@ struct bcm_device {
> 	struct hci_uart		*hu;
> 	bool			is_suspended;
> #endif
> +	bool			no_early_set_baudrate;
> };
> 
> /* generic bcm uart resources */
> @@ -447,7 +457,13 @@ static int bcm_open(struct hci_uart *hu)
> 	if (bcm->dev) {
> 		hci_uart_set_flow_control(hu, true);
> 		hu->init_speed = bcm->dev->init_speed;
> -		hu->oper_speed = bcm->dev->oper_speed;
> +
> +		/* If oper_speed is set, ldisc/serdev will set the baudrate
> +		 * before calling setup()
> +		 */
> +		if (!bcm->dev->no_early_set_baudrate)
> +			hu->oper_speed = bcm->dev->oper_speed;
> +
> 		err = bcm_gpio_set_power(bcm->dev, true);
> 		hci_uart_set_flow_control(hu, false);
> 		if (err)
> @@ -565,6 +581,8 @@ static int bcm_setup(struct hci_uart *hu)
> 	/* Operational speed if any */
> 	if (hu->oper_speed)
> 		speed = hu->oper_speed;
> +	else if (bcm->dev && bcm->dev->oper_speed)
> +		speed = bcm->dev->oper_speed;
> 	else if (hu->proto->oper_speed)
> 		speed = hu->proto->oper_speed;
> 	else
> @@ -1374,6 +1392,7 @@ static struct platform_driver bcm_driver = {
> static int bcm_serdev_probe(struct serdev_device *serdev)
> {
> 	struct bcm_device *bcmdev;
> +	const struct bcm_device_data *data;
> 	int err;
> 
> 	bcmdev = devm_kzalloc(&serdev->dev, sizeof(*bcmdev), GFP_KERNEL);
> @@ -1408,6 +1427,10 @@ static int bcm_serdev_probe(struct serdev_device *serdev)
> 	if (err)
> 		dev_err(&serdev->dev, "Failed to power down\n");
> 
> +	data = device_get_match_data(bcmdev->dev);
> +	if (data)
> +		bcmdev->no_early_set_baudrate = data->no_early_set_baudrate;
> +
> 	return hci_uart_register_device(&bcmdev->serdev_hu, &bcm_proto);
> }
> 
> @@ -1419,12 +1442,16 @@ static void bcm_serdev_remove(struct serdev_device *serdev)
> }
> 
> #ifdef CONFIG_OF
> +struct bcm_device_data bcm4354_device_data = {
> +	.no_early_set_baudrate = true,
> +};
> +
> static const struct of_device_id bcm_bluetooth_of_match[] = {
> 	{ .compatible = "brcm,bcm20702a1" },
> 	{ .compatible = "brcm,bcm4345c5" },
> 	{ .compatible = "brcm,bcm4330-bt" },
> 	{ .compatible = "brcm,bcm43438-bt" },
> -	{ .compatible = "brcm,bcm43540-bt" },
> +	{ .compatible = "brcm,bcm43540-bt", .data = &bcm4354_device_data },
> 	{ },
> };
> MODULE_DEVICE_TABLE(of, bcm_bluetooth_of_match);

this patch looks good to me. I just like to get a few Tested-By lines from people using the other devices where we can change the baud rate early on.

Regards

Marcel


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

* Re: [PATCH v6 2/4] Bluetooth: btbcm: Support pcm configuration
  2019-11-18 19:21 ` [PATCH v6 2/4] Bluetooth: btbcm: Support pcm configuration Abhishek Pandit-Subedi
@ 2019-11-19  5:35   ` Marcel Holtmann
  2019-11-19 20:48     ` Abhishek Pandit-Subedi
  0 siblings, 1 reply; 25+ messages in thread
From: Marcel Holtmann @ 2019-11-19  5:35 UTC (permalink / raw)
  To: Abhishek Pandit-Subedi
  Cc: Johan Hedberg, Rob Herring, linux-bluetooth, dianders, linux-kernel

Hi Abhishek,

> Add BCM vendor specific command to configure PCM parameters. The new
> vendor opcode allows us to set the sco routing, the pcm interface rate,
> and a few other pcm specific options (frame sync, sync mode, and clock
> mode). See broadcom-bluetooth.txt in Documentation for more information
> about valid values for those settings.
> 
> Here is an example trace where this opcode was used to configure
> a BCM4354:
> 
>        < HCI Command: Vendor (0x3f|0x001c) plen 5
>                01 02 00 01 01
>> HCI Event: Command Complete (0x0e) plen 4
>        Vendor (0x3f|0x001c) ncmd 1
>                Status: Success (0x00)
> 
> We can read back the values as well with ocf 0x001d to confirm the
> values that were set:
>        $ hcitool cmd 0x3f 0x001d
>        < HCI Command: ogf 0x3f, ocf 0x001d, plen 0
>> HCI Event: 0x0e plen 9
>        01 1D FC 00 01 02 00 01 01
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> 
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
> drivers/bluetooth/btbcm.c | 47 +++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/btbcm.h | 16 +++++++++++++
> 2 files changed, 63 insertions(+)
> 
> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
> index 2d2e6d862068..df90841d29c5 100644
> --- a/drivers/bluetooth/btbcm.c
> +++ b/drivers/bluetooth/btbcm.c
> @@ -105,6 +105,53 @@ int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> }
> EXPORT_SYMBOL_GPL(btbcm_set_bdaddr);
> 
> +int btbcm_read_pcm_int_params(struct hci_dev *hdev,
> +			      struct bcm_set_pcm_int_params *int_params)
> +{

the name should be _param and not _params since if I remember correctly that is how Broadcom specified it. Also just use param as variable name.

> +	struct sk_buff *skb;
> +	int err = 0;
> +
> +	skb = __hci_cmd_sync(hdev, 0xfc1d, 5, int_params, HCI_INIT_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		err = PTR_ERR(skb);
> +		bt_dev_err(hdev, "BCM: Read PCM int params failed (%d)", err);
> +		return err;
> +	}
> +
> +	if (!skb->data[0] && skb->len == sizeof(*int_params) + 1) {
> +		memcpy(int_params, &skb->data[1], sizeof(*int_params));
> +	} else {
> +		bt_dev_err(hdev,
> +			   "BCM: Read PCM int params failed (%d), Length (%d)",
> +			   skb->data[0], skb->len);
> +		err = -EINVAL;
> +	}
> +
> +	kfree_skb(skb);

I find these harder to read actually and it can be still fault at data[0] access.

	if (skb->len != sizeof(*param) || skb->data[0]) {
		bt_dev_err(hdev, "BCM: Read SCO PCM int parameter failure");    
		kfree_skb(skb);                                                  
		return -EIO;
	}

	memcpy(param, skb->data + 1, sizeof(*param));
	kfree_skb(skb);
	return 0;
}
	
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(btbcm_read_pcm_int_params);
> +
> +int btbcm_write_pcm_int_params(struct hci_dev *hdev,
> +			       const struct bcm_set_pcm_int_params *int_params)
> +{
> +	struct sk_buff *skb;
> +	int err;
> +
> +	/* Vendor ocf 0x001c sets the pcm parameters and 0x001d reads it */

Scrap this comment.

> +	skb = __hci_cmd_sync(hdev, 0xfc1c, 5, int_params, HCI_INIT_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		err = PTR_ERR(skb);
> +		bt_dev_err(hdev, "BCM: Write PCM int params failed (%d)", err);
> +		return err;
> +	}
> +	kfree_skb(skb);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(btbcm_write_pcm_int_params);
> +
> int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw)
> {

Otherwise this looks good.

Regards

Marcel


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

* Re: [PATCH v6 3/4] dt-bindings: net: broadcom-bluetooth: Add pcm config
  2019-11-18 19:21 ` [PATCH v6 3/4] dt-bindings: net: broadcom-bluetooth: Add pcm config Abhishek Pandit-Subedi
@ 2019-11-19  5:39   ` Marcel Holtmann
  2019-11-19 16:50     ` Doug Anderson
  2019-11-21 21:29   ` Rob Herring
  1 sibling, 1 reply; 25+ messages in thread
From: Marcel Holtmann @ 2019-11-19  5:39 UTC (permalink / raw)
  To: Abhishek Pandit-Subedi
  Cc: Johan Hedberg, Rob Herring, linux-bluetooth, dianders,
	devicetree, David S. Miller, netdev, linux-kernel, Ondrej Jirman,
	Mark Rutland, Chen-Yu Tsai

Hi Abhishek,

> Add documentation for pcm parameters.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> 
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
> .../bindings/net/broadcom-bluetooth.txt       | 16 ++++++++++
> include/dt-bindings/bluetooth/brcm.h          | 32 +++++++++++++++++++
> 2 files changed, 48 insertions(+)
> create mode 100644 include/dt-bindings/bluetooth/brcm.h
> 
> diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> index c749dc297624..8561e4684378 100644
> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> @@ -29,10 +29,20 @@ Optional properties:
>    - "lpo": external low power 32.768 kHz clock
>  - vbat-supply: phandle to regulator supply for VBAT
>  - vddio-supply: phandle to regulator supply for VDDIO
> + - brcm,bt-sco-routing: PCM, Transport, Codec, I2S
> + - brcm,bt-pcm-interface-rate: 128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps
> + - brcm,bt-pcm-frame-type: short, long
> + - brcm,bt-pcm-sync-mode: slave, master
> + - brcm,bt-pcm-clock-mode: slave, master
> 
> +See include/dt-bindings/bluetooth/brcm.h for SCO/PCM parameters. The default
> +value for all these values are 0 (except for brcm,bt-sco-routing which requires
> +a value) if you choose to leave it out.
> 
> Example:
> 
> +#include <dt-bindings/bluetooth/brcm.h>
> +
> &uart2 {
>        pinctrl-names = "default";
>        pinctrl-0 = <&uart2_pins>;
> @@ -40,5 +50,11 @@ Example:
>        bluetooth {
>                compatible = "brcm,bcm43438-bt";
>                max-speed = <921600>;
> +
> +               brcm,bt-sco-routing        = <BRCM_SCO_ROUTING_TRANSPORT>;

in case you use transport which means HCI, you would not have values below. It is rather PCM here in the example.

> +               brcm,bt-pcm-interface-rate = <BRCM_PCM_IF_RATE_512KBPS>;
> +               brcm,bt-pcm-frame-type     = <BRCM_PCM_FRAME_TYPE_SHORT>;
> +               brcm,bt-pcm-sync-mode      = <BRCM_PCM_SYNC_MODE_MASTER>;
> +               brcm,bt-pcm-clock-mode     = <BRCM_PCM_CLOCK_MODE_MASTER>;
>        };
> };

And I am asking this again. Is this adding any value to use an extra include file? Inside the driver we are not really needing these values since they are handed to the hardware.

Regards

Marcel


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

* Re: [PATCH v6 4/4] Bluetooth: hci_bcm: Support pcm params in dts
  2019-11-18 19:21 ` [PATCH v6 4/4] Bluetooth: hci_bcm: Support pcm params in dts Abhishek Pandit-Subedi
@ 2019-11-19  5:44   ` Marcel Holtmann
  2019-11-19 20:45     ` Abhishek Pandit-Subedi
  0 siblings, 1 reply; 25+ messages in thread
From: Marcel Holtmann @ 2019-11-19  5:44 UTC (permalink / raw)
  To: Abhishek Pandit-Subedi
  Cc: Johan Hedberg, Rob Herring, linux-bluetooth, dianders, linux-kernel

Hi Abhishek,

> BCM chips may require configuration of PCM to operate correctly and
> there is a vendor specific HCI command to do this. Add support in the
> hci_bcm driver to parse this from devicetree and configure the chip.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> 
> Changes in v6:
> - Added btbcm_read_pcm_int_params and change pcm params to first read
>  the pcm params before setting it
> 
> Changes in v5:
> - Rename parameters to bt-* and read as integer instead of bytestring
> - Update documentation with defaults and put values in header
> - Changed patch order
> 
> Changes in v4:
> - Fix incorrect function name in hci_bcm
> 
> Changes in v3:
> - Change disallow baudrate setting to return -EBUSY if called before
>  ready. bcm_proto is no longer modified and is back to being const.
> - Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
> - Changed brcm,sco-routing to brcm,bt-sco-routing
> 
> Changes in v2:
> - Use match data to disallow baudrate setting
> - Parse pcm parameters by name instead of as a byte string
> - Fix prefix for dt-bindings commit
> 
> drivers/bluetooth/hci_bcm.c | 57 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index ee40003008d8..2ce3fac2c5dd 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -25,6 +25,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/serdev.h>
> 
> +#include <dt-bindings/bluetooth/brcm.h>
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> 
> @@ -88,6 +89,7 @@ struct bcm_device_data {
>  *	used to disable flow control during runtime suspend and system sleep
>  * @is_suspended: whether flow control is currently disabled
>  * @no_early_set_baudrate: don't set_baudrate before setup()
> + * @pcm_params: PCM and routing parameters
>  */
> struct bcm_device {
> 	/* Must be the first member, hci_serdev.c expects this. */
> @@ -122,6 +124,8 @@ struct bcm_device {
> 	bool			is_suspended;
> #endif
> 	bool			no_early_set_baudrate;
> +
> +	struct bcm_set_pcm_int_params	pcm_params;
> };
> 
> /* generic bcm uart resources */
> @@ -541,6 +545,7 @@ static int bcm_flush(struct hci_uart *hu)
> static int bcm_setup(struct hci_uart *hu)
> {
> 	struct bcm_data *bcm = hu->priv;
> +	struct bcm_set_pcm_int_params p;
> 	char fw_name[64];
> 	const struct firmware *fw;
> 	unsigned int speed;
> @@ -594,6 +599,31 @@ static int bcm_setup(struct hci_uart *hu)
> 			host_set_baudrate(hu, speed);
> 	}
> 
> +	/* PCM parameters if any*/
> +	err = btbcm_read_pcm_int_params(hu->hdev, &p);
> +	if (!err) {
> +		if (bcm->dev->pcm_params.routing == 0xff)
> +			bcm->dev->pcm_params.routing = p.routing;
> +		if (bcm->dev->pcm_params.rate == 0xff)
> +			bcm->dev->pcm_params.rate = p.rate;
> +		if (bcm->dev->pcm_params.frame_sync == 0xff)
> +			bcm->dev->pcm_params.frame_sync = p.frame_sync;
> +		if (bcm->dev->pcm_params.sync_mode == 0xff)
> +			bcm->dev->pcm_params.sync_mode = p.sync_mode;
> +		if (bcm->dev->pcm_params.clock_mode == 0xff)
> +			bcm->dev->pcm_params.clock_mode = p.clock_mode;

Frankly, I wouldn’t bother here. If the read HCI command failed, then we abort bcm_setup and fail the whole procedure. These commands have been around the first Broadcom chips and you can assume they are present. And if at some point they do fail, I want to know about it.

> +
> +		/* Write only when there are changes */
> +		if (memcmp(&p, &bcm->dev->pcm_params, sizeof(p)))
> +			err = btbcm_write_pcm_int_params(hu->hdev,
> +							 &bcm->dev->pcm_params);
> +
> +		if (err)
> +			bt_dev_warn(hu->hdev, "BCM: Write pcm params failed (%d)",
> +				    err);
> +	} else
> +		bt_dev_warn(hu->hdev, "BCM: Read pcm params failed (%d)", err);
> +
> finalize:
> 	release_firmware(fw);
> 
> @@ -1128,9 +1158,36 @@ static int bcm_acpi_probe(struct bcm_device *dev)
> }
> #endif /* CONFIG_ACPI */
> 
> +static int property_read_u8(struct device *dev, const char *prop, u8 *value)
> +{
> +	int err;
> +	u32 tmp;
> +
> +	err = device_property_read_u32(dev, prop, &tmp);
> +
> +	if (!err)
> +		*value = (u8)tmp;
> +
> +	return err;
> +}

I think this really needs to be done in the generic property code if this is wanted.

> +
> static int bcm_of_probe(struct bcm_device *bdev)
> {
> 	device_property_read_u32(bdev->dev, "max-speed", &bdev->oper_speed);
> +
> +	memset(&bdev->pcm_params, 0xff, sizeof(bdev->pcm_params));

Scrap this memset. We will read the values first.

> +
> +	property_read_u8(bdev->dev, "brcm,bt-sco-routing",
> +			 &bdev->pcm_params.routing);
> +	property_read_u8(bdev->dev, "brcm,bt-pcm-interface-rate",
> +			 &bdev->pcm_params.rate);
> +	property_read_u8(bdev->dev, "brcm,bt-pcm-frame-type",
> +			 &bdev->pcm_params.frame_sync);
> +	property_read_u8(bdev->dev, "brcm,bt-pcm-sync-mode",
> +			 &bdev->pcm_params.sync_mode);
> +	property_read_u8(bdev->dev, "brcm,bt-pcm-clock-mode",
> +			 &bdev->pcm_params.clock_mode);
> +
> 	return 0;
> }

Regards

Marcel


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

* Re: [PATCH v6 3/4] dt-bindings: net: broadcom-bluetooth: Add pcm config
  2019-11-19  5:39   ` Marcel Holtmann
@ 2019-11-19 16:50     ` Doug Anderson
  0 siblings, 0 replies; 25+ messages in thread
From: Doug Anderson @ 2019-11-19 16:50 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Abhishek Pandit-Subedi, Johan Hedberg, Rob Herring,
	linux-bluetooth,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	David S. Miller, netdev, LKML, Ondrej Jirman, Mark Rutland,
	Chen-Yu Tsai

Hi,

On Mon, Nov 18, 2019 at 9:39 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Abhishek,
>
> > Add documentation for pcm parameters.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes in v6: None
> > Changes in v5: None
> > Changes in v4: None
> > Changes in v3: None
> > Changes in v2: None
> >
> > .../bindings/net/broadcom-bluetooth.txt       | 16 ++++++++++
> > include/dt-bindings/bluetooth/brcm.h          | 32 +++++++++++++++++++
> > 2 files changed, 48 insertions(+)
> > create mode 100644 include/dt-bindings/bluetooth/brcm.h
> >
> > diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> > index c749dc297624..8561e4684378 100644
> > --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> > +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> > @@ -29,10 +29,20 @@ Optional properties:
> >    - "lpo": external low power 32.768 kHz clock
> >  - vbat-supply: phandle to regulator supply for VBAT
> >  - vddio-supply: phandle to regulator supply for VDDIO
> > + - brcm,bt-sco-routing: PCM, Transport, Codec, I2S
> > + - brcm,bt-pcm-interface-rate: 128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps
> > + - brcm,bt-pcm-frame-type: short, long
> > + - brcm,bt-pcm-sync-mode: slave, master
> > + - brcm,bt-pcm-clock-mode: slave, master
> >
> > +See include/dt-bindings/bluetooth/brcm.h for SCO/PCM parameters. The default
> > +value for all these values are 0 (except for brcm,bt-sco-routing which requires
> > +a value) if you choose to leave it out.
> >
> > Example:
> >
> > +#include <dt-bindings/bluetooth/brcm.h>
> > +
> > &uart2 {
> >        pinctrl-names = "default";
> >        pinctrl-0 = <&uart2_pins>;
> > @@ -40,5 +50,11 @@ Example:
> >        bluetooth {
> >                compatible = "brcm,bcm43438-bt";
> >                max-speed = <921600>;
> > +
> > +               brcm,bt-sco-routing        = <BRCM_SCO_ROUTING_TRANSPORT>;
>
> in case you use transport which means HCI, you would not have values below. It is rather PCM here in the example.
>
> > +               brcm,bt-pcm-interface-rate = <BRCM_PCM_IF_RATE_512KBPS>;
> > +               brcm,bt-pcm-frame-type     = <BRCM_PCM_FRAME_TYPE_SHORT>;
> > +               brcm,bt-pcm-sync-mode      = <BRCM_PCM_SYNC_MODE_MASTER>;
> > +               brcm,bt-pcm-clock-mode     = <BRCM_PCM_CLOCK_MODE_MASTER>;
> >        };
> > };
>
> And I am asking this again. Is this adding any value to use an extra include file? Inside the driver we are not really needing these values since they are handed to the hardware.

Personally I find that they add value in that it makes it easier for
someone tweaking the device tree to know what the expected valid
values are and what they mean.  I think Matthias also found value in
them since he suggested them in:

https://lore.kernel.org/r/20191114175836.GI27773@google.com

There, he said:

> I'd suggest to define constants in include/dt-bindings/bluetooth/brcm.h
> and use them instead of literals, with this we wouldn't rely on (optional)
> comments to make the configuration human readable.

...which seems to make sense to me.

-Doug

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

* Re: [PATCH v6 4/4] Bluetooth: hci_bcm: Support pcm params in dts
  2019-11-19  5:44   ` Marcel Holtmann
@ 2019-11-19 20:45     ` Abhishek Pandit-Subedi
  2019-11-19 23:42       ` Marcel Holtmann
  0 siblings, 1 reply; 25+ messages in thread
From: Abhishek Pandit-Subedi @ 2019-11-19 20:45 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Rob Herring, linux-bluetooth, Douglas Anderson, LKML

On Mon, Nov 18, 2019 at 9:44 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Abhishek,
>
> > BCM chips may require configuration of PCM to operate correctly and
> > there is a vendor specific HCI command to do this. Add support in the
> > hci_bcm driver to parse this from devicetree and configure the chip.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes in v6:
> > - Added btbcm_read_pcm_int_params and change pcm params to first read
> >  the pcm params before setting it
> >
> > Changes in v5:
> > - Rename parameters to bt-* and read as integer instead of bytestring
> > - Update documentation with defaults and put values in header
> > - Changed patch order
> >
> > Changes in v4:
> > - Fix incorrect function name in hci_bcm
> >
> > Changes in v3:
> > - Change disallow baudrate setting to return -EBUSY if called before
> >  ready. bcm_proto is no longer modified and is back to being const.
> > - Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
> > - Changed brcm,sco-routing to brcm,bt-sco-routing
> >
> > Changes in v2:
> > - Use match data to disallow baudrate setting
> > - Parse pcm parameters by name instead of as a byte string
> > - Fix prefix for dt-bindings commit
> >
> > drivers/bluetooth/hci_bcm.c | 57 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 57 insertions(+)
> >
> > diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> > index ee40003008d8..2ce3fac2c5dd 100644
> > --- a/drivers/bluetooth/hci_bcm.c
> > +++ b/drivers/bluetooth/hci_bcm.c
> > @@ -25,6 +25,7 @@
> > #include <linux/pm_runtime.h>
> > #include <linux/serdev.h>
> >
> > +#include <dt-bindings/bluetooth/brcm.h>
> > #include <net/bluetooth/bluetooth.h>
> > #include <net/bluetooth/hci_core.h>
> >
> > @@ -88,6 +89,7 @@ struct bcm_device_data {
> >  *    used to disable flow control during runtime suspend and system sleep
> >  * @is_suspended: whether flow control is currently disabled
> >  * @no_early_set_baudrate: don't set_baudrate before setup()
> > + * @pcm_params: PCM and routing parameters
> >  */
> > struct bcm_device {
> >       /* Must be the first member, hci_serdev.c expects this. */
> > @@ -122,6 +124,8 @@ struct bcm_device {
> >       bool                    is_suspended;
> > #endif
> >       bool                    no_early_set_baudrate;
> > +
> > +     struct bcm_set_pcm_int_params   pcm_params;
> > };
> >
> > /* generic bcm uart resources */
> > @@ -541,6 +545,7 @@ static int bcm_flush(struct hci_uart *hu)
> > static int bcm_setup(struct hci_uart *hu)
> > {
> >       struct bcm_data *bcm = hu->priv;
> > +     struct bcm_set_pcm_int_params p;
> >       char fw_name[64];
> >       const struct firmware *fw;
> >       unsigned int speed;
> > @@ -594,6 +599,31 @@ static int bcm_setup(struct hci_uart *hu)
> >                       host_set_baudrate(hu, speed);
> >       }
> >
> > +     /* PCM parameters if any*/
> > +     err = btbcm_read_pcm_int_params(hu->hdev, &p);
> > +     if (!err) {
> > +             if (bcm->dev->pcm_params.routing == 0xff)
> > +                     bcm->dev->pcm_params.routing = p.routing;
> > +             if (bcm->dev->pcm_params.rate == 0xff)
> > +                     bcm->dev->pcm_params.rate = p.rate;
> > +             if (bcm->dev->pcm_params.frame_sync == 0xff)
> > +                     bcm->dev->pcm_params.frame_sync = p.frame_sync;
> > +             if (bcm->dev->pcm_params.sync_mode == 0xff)
> > +                     bcm->dev->pcm_params.sync_mode = p.sync_mode;
> > +             if (bcm->dev->pcm_params.clock_mode == 0xff)
> > +                     bcm->dev->pcm_params.clock_mode = p.clock_mode;
>
> Frankly, I wouldn’t bother here. If the read HCI command failed, then we abort bcm_setup and fail the whole procedure. These commands have been around the first Broadcom chips and you can assume they are present. And if at some point they do fail, I want to know about it.
Ok -- will change to return error if it fails.

>
> > +
> > +             /* Write only when there are changes */
> > +             if (memcmp(&p, &bcm->dev->pcm_params, sizeof(p)))
> > +                     err = btbcm_write_pcm_int_params(hu->hdev,
> > +                                                      &bcm->dev->pcm_params);
> > +
> > +             if (err)
> > +                     bt_dev_warn(hu->hdev, "BCM: Write pcm params failed (%d)",
> > +                                 err);
> > +     } else
> > +             bt_dev_warn(hu->hdev, "BCM: Read pcm params failed (%d)", err);
> > +
> > finalize:
> >       release_firmware(fw);
> >
> > @@ -1128,9 +1158,36 @@ static int bcm_acpi_probe(struct bcm_device *dev)
> > }
> > #endif /* CONFIG_ACPI */
> >
> > +static int property_read_u8(struct device *dev, const char *prop, u8 *value)
> > +{
> > +     int err;
> > +     u32 tmp;
> > +
> > +     err = device_property_read_u32(dev, prop, &tmp);
> > +
> > +     if (!err)
> > +             *value = (u8)tmp;
> > +
> > +     return err;
> > +}
>
> I think this really needs to be done in the generic property code if this is wanted.
Yes, this should be device_property_read_u8. For some reason, I
thought that wasn't working before (I'll have to retest it with
straight integer values).

>
> > +
> > static int bcm_of_probe(struct bcm_device *bdev)
> > {
> >       device_property_read_u32(bdev->dev, "max-speed", &bdev->oper_speed);
> > +
> > +     memset(&bdev->pcm_params, 0xff, sizeof(bdev->pcm_params));
>
> Scrap this memset. We will read the values first.

I added this memset is bcm_of_probe occurs before patchram and without
setting some magic value in the pcm_params, we don't know which values
are valid (since 0 has some meaning in the params).
It doesn't make sense to me to read pcm params outside setup (I want
patchram to complete first) and it doesn't make sense to do property
reads inside setup.


>
> > +
> > +     property_read_u8(bdev->dev, "brcm,bt-sco-routing",
> > +                      &bdev->pcm_params.routing);
> > +     property_read_u8(bdev->dev, "brcm,bt-pcm-interface-rate",
> > +                      &bdev->pcm_params.rate);
> > +     property_read_u8(bdev->dev, "brcm,bt-pcm-frame-type",
> > +                      &bdev->pcm_params.frame_sync);
> > +     property_read_u8(bdev->dev, "brcm,bt-pcm-sync-mode",
> > +                      &bdev->pcm_params.sync_mode);
> > +     property_read_u8(bdev->dev, "brcm,bt-pcm-clock-mode",
> > +                      &bdev->pcm_params.clock_mode);
> > +
> >       return 0;
> > }
>
> Regards
>
> Marcel
>

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

* Re: [PATCH v6 2/4] Bluetooth: btbcm: Support pcm configuration
  2019-11-19  5:35   ` Marcel Holtmann
@ 2019-11-19 20:48     ` Abhishek Pandit-Subedi
  2019-11-19 23:45       ` Marcel Holtmann
  0 siblings, 1 reply; 25+ messages in thread
From: Abhishek Pandit-Subedi @ 2019-11-19 20:48 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Rob Herring, linux-bluetooth, Douglas Anderson, LKML

On Mon, Nov 18, 2019 at 9:35 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Abhishek,
>
> > Add BCM vendor specific command to configure PCM parameters. The new
> > vendor opcode allows us to set the sco routing, the pcm interface rate,
> > and a few other pcm specific options (frame sync, sync mode, and clock
> > mode). See broadcom-bluetooth.txt in Documentation for more information
> > about valid values for those settings.
> >
> > Here is an example trace where this opcode was used to configure
> > a BCM4354:
> >
> >        < HCI Command: Vendor (0x3f|0x001c) plen 5
> >                01 02 00 01 01
> >> HCI Event: Command Complete (0x0e) plen 4
> >        Vendor (0x3f|0x001c) ncmd 1
> >                Status: Success (0x00)
> >
> > We can read back the values as well with ocf 0x001d to confirm the
> > values that were set:
> >        $ hcitool cmd 0x3f 0x001d
> >        < HCI Command: ogf 0x3f, ocf 0x001d, plen 0
> >> HCI Event: 0x0e plen 9
> >        01 1D FC 00 01 02 00 01 01
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > ---
> >
> > Changes in v6: None
> > Changes in v5: None
> > Changes in v4: None
> > Changes in v3: None
> > Changes in v2: None
> >
> > drivers/bluetooth/btbcm.c | 47 +++++++++++++++++++++++++++++++++++++++
> > drivers/bluetooth/btbcm.h | 16 +++++++++++++
> > 2 files changed, 63 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
> > index 2d2e6d862068..df90841d29c5 100644
> > --- a/drivers/bluetooth/btbcm.c
> > +++ b/drivers/bluetooth/btbcm.c
> > @@ -105,6 +105,53 @@ int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
> > }
> > EXPORT_SYMBOL_GPL(btbcm_set_bdaddr);
> >
> > +int btbcm_read_pcm_int_params(struct hci_dev *hdev,
> > +                           struct bcm_set_pcm_int_params *int_params)
> > +{
>
> the name should be _param and not _params since if I remember correctly that is how Broadcom specified it. Also just use param as variable name.

Technically, you are configuring multiple PCM params :)

>
> > +     struct sk_buff *skb;
> > +     int err = 0;
> > +
> > +     skb = __hci_cmd_sync(hdev, 0xfc1d, 5, int_params, HCI_INIT_TIMEOUT);
> > +     if (IS_ERR(skb)) {
> > +             err = PTR_ERR(skb);
> > +             bt_dev_err(hdev, "BCM: Read PCM int params failed (%d)", err);
> > +             return err;
> > +     }
> > +
> > +     if (!skb->data[0] && skb->len == sizeof(*int_params) + 1) {
> > +             memcpy(int_params, &skb->data[1], sizeof(*int_params));
> > +     } else {
> > +             bt_dev_err(hdev,
> > +                        "BCM: Read PCM int params failed (%d), Length (%d)",
> > +                        skb->data[0], skb->len);
> > +             err = -EINVAL;
> > +     }
> > +
> > +     kfree_skb(skb);
>
> I find these harder to read actually and it can be still fault at data[0] access.
>
>         if (skb->len != sizeof(*param) || skb->data[0]) {
>                 bt_dev_err(hdev, "BCM: Read SCO PCM int parameter failure");
>                 kfree_skb(skb);
>                 return -EIO;
>         }
>
>         memcpy(param, skb->data + 1, sizeof(*param));
>         kfree_skb(skb);
>         return 0;
> }
>

Sure. skb->len should be sizeof(*param) + 1 because there's an extra
byte for the status as well.

> > +
> > +     return err;
> > +}
> > +EXPORT_SYMBOL_GPL(btbcm_read_pcm_int_params);
> > +
> > +int btbcm_write_pcm_int_params(struct hci_dev *hdev,
> > +                            const struct bcm_set_pcm_int_params *int_params)
> > +{
> > +     struct sk_buff *skb;
> > +     int err;
> > +
> > +     /* Vendor ocf 0x001c sets the pcm parameters and 0x001d reads it */
>
> Scrap this comment.
>
> > +     skb = __hci_cmd_sync(hdev, 0xfc1c, 5, int_params, HCI_INIT_TIMEOUT);
> > +     if (IS_ERR(skb)) {
> > +             err = PTR_ERR(skb);
> > +             bt_dev_err(hdev, "BCM: Write PCM int params failed (%d)", err);
> > +             return err;
> > +     }
> > +     kfree_skb(skb);
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(btbcm_write_pcm_int_params);
> > +
> > int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw)
> > {
>
> Otherwise this looks good.
>
> Regards
>
> Marcel
>

So generally, I've done a whole new patch series with every change.
Would you prefer to see singular updates on the same email thread or
should I keep doing new patch series?

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

* Re: [PATCH v6 4/4] Bluetooth: hci_bcm: Support pcm params in dts
  2019-11-19 20:45     ` Abhishek Pandit-Subedi
@ 2019-11-19 23:42       ` Marcel Holtmann
  0 siblings, 0 replies; 25+ messages in thread
From: Marcel Holtmann @ 2019-11-19 23:42 UTC (permalink / raw)
  To: Abhishek Pandit-Subedi
  Cc: Johan Hedberg, Rob Herring, linux-bluetooth, Douglas Anderson, LKML

Hi Abhishek,

>>> BCM chips may require configuration of PCM to operate correctly and
>>> there is a vendor specific HCI command to do this. Add support in the
>>> hci_bcm driver to parse this from devicetree and configure the chip.
>>> 
>>> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>>> ---
>>> 
>>> Changes in v6:
>>> - Added btbcm_read_pcm_int_params and change pcm params to first read
>>> the pcm params before setting it
>>> 
>>> Changes in v5:
>>> - Rename parameters to bt-* and read as integer instead of bytestring
>>> - Update documentation with defaults and put values in header
>>> - Changed patch order
>>> 
>>> Changes in v4:
>>> - Fix incorrect function name in hci_bcm
>>> 
>>> Changes in v3:
>>> - Change disallow baudrate setting to return -EBUSY if called before
>>> ready. bcm_proto is no longer modified and is back to being const.
>>> - Changed btbcm_set_pcm_params to btbcm_set_pcm_int_params
>>> - Changed brcm,sco-routing to brcm,bt-sco-routing
>>> 
>>> Changes in v2:
>>> - Use match data to disallow baudrate setting
>>> - Parse pcm parameters by name instead of as a byte string
>>> - Fix prefix for dt-bindings commit
>>> 
>>> drivers/bluetooth/hci_bcm.c | 57 +++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 57 insertions(+)
>>> 
>>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>>> index ee40003008d8..2ce3fac2c5dd 100644
>>> --- a/drivers/bluetooth/hci_bcm.c
>>> +++ b/drivers/bluetooth/hci_bcm.c
>>> @@ -25,6 +25,7 @@
>>> #include <linux/pm_runtime.h>
>>> #include <linux/serdev.h>
>>> 
>>> +#include <dt-bindings/bluetooth/brcm.h>
>>> #include <net/bluetooth/bluetooth.h>
>>> #include <net/bluetooth/hci_core.h>
>>> 
>>> @@ -88,6 +89,7 @@ struct bcm_device_data {
>>> *    used to disable flow control during runtime suspend and system sleep
>>> * @is_suspended: whether flow control is currently disabled
>>> * @no_early_set_baudrate: don't set_baudrate before setup()
>>> + * @pcm_params: PCM and routing parameters
>>> */
>>> struct bcm_device {
>>>      /* Must be the first member, hci_serdev.c expects this. */
>>> @@ -122,6 +124,8 @@ struct bcm_device {
>>>      bool                    is_suspended;
>>> #endif
>>>      bool                    no_early_set_baudrate;
>>> +
>>> +     struct bcm_set_pcm_int_params   pcm_params;
>>> };
>>> 
>>> /* generic bcm uart resources */
>>> @@ -541,6 +545,7 @@ static int bcm_flush(struct hci_uart *hu)
>>> static int bcm_setup(struct hci_uart *hu)
>>> {
>>>      struct bcm_data *bcm = hu->priv;
>>> +     struct bcm_set_pcm_int_params p;
>>>      char fw_name[64];
>>>      const struct firmware *fw;
>>>      unsigned int speed;
>>> @@ -594,6 +599,31 @@ static int bcm_setup(struct hci_uart *hu)
>>>                      host_set_baudrate(hu, speed);
>>>      }
>>> 
>>> +     /* PCM parameters if any*/
>>> +     err = btbcm_read_pcm_int_params(hu->hdev, &p);
>>> +     if (!err) {
>>> +             if (bcm->dev->pcm_params.routing == 0xff)
>>> +                     bcm->dev->pcm_params.routing = p.routing;
>>> +             if (bcm->dev->pcm_params.rate == 0xff)
>>> +                     bcm->dev->pcm_params.rate = p.rate;
>>> +             if (bcm->dev->pcm_params.frame_sync == 0xff)
>>> +                     bcm->dev->pcm_params.frame_sync = p.frame_sync;
>>> +             if (bcm->dev->pcm_params.sync_mode == 0xff)
>>> +                     bcm->dev->pcm_params.sync_mode = p.sync_mode;
>>> +             if (bcm->dev->pcm_params.clock_mode == 0xff)
>>> +                     bcm->dev->pcm_params.clock_mode = p.clock_mode;
>> 
>> Frankly, I wouldn’t bother here. If the read HCI command failed, then we abort bcm_setup and fail the whole procedure. These commands have been around the first Broadcom chips and you can assume they are present. And if at some point they do fail, I want to know about it.
> Ok -- will change to return error if it fails.
> 
>> 
>>> +
>>> +             /* Write only when there are changes */
>>> +             if (memcmp(&p, &bcm->dev->pcm_params, sizeof(p)))
>>> +                     err = btbcm_write_pcm_int_params(hu->hdev,
>>> +                                                      &bcm->dev->pcm_params);
>>> +
>>> +             if (err)
>>> +                     bt_dev_warn(hu->hdev, "BCM: Write pcm params failed (%d)",
>>> +                                 err);
>>> +     } else
>>> +             bt_dev_warn(hu->hdev, "BCM: Read pcm params failed (%d)", err);
>>> +
>>> finalize:
>>>      release_firmware(fw);
>>> 
>>> @@ -1128,9 +1158,36 @@ static int bcm_acpi_probe(struct bcm_device *dev)
>>> }
>>> #endif /* CONFIG_ACPI */
>>> 
>>> +static int property_read_u8(struct device *dev, const char *prop, u8 *value)
>>> +{
>>> +     int err;
>>> +     u32 tmp;
>>> +
>>> +     err = device_property_read_u32(dev, prop, &tmp);
>>> +
>>> +     if (!err)
>>> +             *value = (u8)tmp;
>>> +
>>> +     return err;
>>> +}
>> 
>> I think this really needs to be done in the generic property code if this is wanted.
> Yes, this should be device_property_read_u8. For some reason, I
> thought that wasn't working before (I'll have to retest it with
> straight integer values).
> 
>> 
>>> +
>>> static int bcm_of_probe(struct bcm_device *bdev)
>>> {
>>>      device_property_read_u32(bdev->dev, "max-speed", &bdev->oper_speed);
>>> +
>>> +     memset(&bdev->pcm_params, 0xff, sizeof(bdev->pcm_params));
>> 
>> Scrap this memset. We will read the values first.
> 
> I added this memset is bcm_of_probe occurs before patchram and without
> setting some magic value in the pcm_params, we don't know which values
> are valid (since 0 has some meaning in the params).
> It doesn't make sense to me to read pcm params outside setup (I want
> patchram to complete first) and it doesn't make sense to do property
> reads inside setup.

I wonder if we should just fail if bt-sco-routing is PCM and not all values are provided in the DT. Looks like there is no clean way of doing this gracefully otherwise.

Regards

Marcel


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

* Re: [PATCH v6 2/4] Bluetooth: btbcm: Support pcm configuration
  2019-11-19 20:48     ` Abhishek Pandit-Subedi
@ 2019-11-19 23:45       ` Marcel Holtmann
  0 siblings, 0 replies; 25+ messages in thread
From: Marcel Holtmann @ 2019-11-19 23:45 UTC (permalink / raw)
  To: Abhishek Pandit-Subedi
  Cc: Johan Hedberg, Rob Herring, linux-bluetooth, Douglas Anderson, LKML

Hi Abhishek,

>>> Add BCM vendor specific command to configure PCM parameters. The new
>>> vendor opcode allows us to set the sco routing, the pcm interface rate,
>>> and a few other pcm specific options (frame sync, sync mode, and clock
>>> mode). See broadcom-bluetooth.txt in Documentation for more information
>>> about valid values for those settings.
>>> 
>>> Here is an example trace where this opcode was used to configure
>>> a BCM4354:
>>> 
>>>       < HCI Command: Vendor (0x3f|0x001c) plen 5
>>>               01 02 00 01 01
>>>> HCI Event: Command Complete (0x0e) plen 4
>>>       Vendor (0x3f|0x001c) ncmd 1
>>>               Status: Success (0x00)
>>> 
>>> We can read back the values as well with ocf 0x001d to confirm the
>>> values that were set:
>>>       $ hcitool cmd 0x3f 0x001d
>>>       < HCI Command: ogf 0x3f, ocf 0x001d, plen 0
>>>> HCI Event: 0x0e plen 9
>>>       01 1D FC 00 01 02 00 01 01
>>> 
>>> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>>> ---
>>> 
>>> Changes in v6: None
>>> Changes in v5: None
>>> Changes in v4: None
>>> Changes in v3: None
>>> Changes in v2: None
>>> 
>>> drivers/bluetooth/btbcm.c | 47 +++++++++++++++++++++++++++++++++++++++
>>> drivers/bluetooth/btbcm.h | 16 +++++++++++++
>>> 2 files changed, 63 insertions(+)
>>> 
>>> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
>>> index 2d2e6d862068..df90841d29c5 100644
>>> --- a/drivers/bluetooth/btbcm.c
>>> +++ b/drivers/bluetooth/btbcm.c
>>> @@ -105,6 +105,53 @@ int btbcm_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr)
>>> }
>>> EXPORT_SYMBOL_GPL(btbcm_set_bdaddr);
>>> 
>>> +int btbcm_read_pcm_int_params(struct hci_dev *hdev,
>>> +                           struct bcm_set_pcm_int_params *int_params)
>>> +{
>> 
>> the name should be _param and not _params since if I remember correctly that is how Broadcom specified it. Also just use param as variable name.
> 
> Technically, you are configuring multiple PCM params :)

I know and maybe they renamed the command internally by now. It is just when I read the Broadcom HCI vendor commands, it was named that way. Anyway, I am fine if you want to use _params and params argument variable name. Might make sense since we somehow named the struct that way as well and it is pre-existing.

>>> +     struct sk_buff *skb;
>>> +     int err = 0;
>>> +
>>> +     skb = __hci_cmd_sync(hdev, 0xfc1d, 5, int_params, HCI_INIT_TIMEOUT);
>>> +     if (IS_ERR(skb)) {
>>> +             err = PTR_ERR(skb);
>>> +             bt_dev_err(hdev, "BCM: Read PCM int params failed (%d)", err);
>>> +             return err;
>>> +     }
>>> +
>>> +     if (!skb->data[0] && skb->len == sizeof(*int_params) + 1) {
>>> +             memcpy(int_params, &skb->data[1], sizeof(*int_params));
>>> +     } else {
>>> +             bt_dev_err(hdev,
>>> +                        "BCM: Read PCM int params failed (%d), Length (%d)",
>>> +                        skb->data[0], skb->len);
>>> +             err = -EINVAL;
>>> +     }
>>> +
>>> +     kfree_skb(skb);
>> 
>> I find these harder to read actually and it can be still fault at data[0] access.
>> 
>>        if (skb->len != sizeof(*param) || skb->data[0]) {
>>                bt_dev_err(hdev, "BCM: Read SCO PCM int parameter failure");
>>                kfree_skb(skb);
>>                return -EIO;
>>        }
>> 
>>        memcpy(param, skb->data + 1, sizeof(*param));
>>        kfree_skb(skb);
>>        return 0;
>> }
>> 
> 
> Sure. skb->len should be sizeof(*param) + 1 because there's an extra
> byte for the status as well.

Good point. I forgot about the status octet.

> 
>>> +
>>> +     return err;
>>> +}
>>> +EXPORT_SYMBOL_GPL(btbcm_read_pcm_int_params);
>>> +
>>> +int btbcm_write_pcm_int_params(struct hci_dev *hdev,
>>> +                            const struct bcm_set_pcm_int_params *int_params)
>>> +{
>>> +     struct sk_buff *skb;
>>> +     int err;
>>> +
>>> +     /* Vendor ocf 0x001c sets the pcm parameters and 0x001d reads it */
>> 
>> Scrap this comment.
>> 
>>> +     skb = __hci_cmd_sync(hdev, 0xfc1c, 5, int_params, HCI_INIT_TIMEOUT);
>>> +     if (IS_ERR(skb)) {
>>> +             err = PTR_ERR(skb);
>>> +             bt_dev_err(hdev, "BCM: Write PCM int params failed (%d)", err);
>>> +             return err;
>>> +     }
>>> +     kfree_skb(skb);
>>> +
>>> +     return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(btbcm_write_pcm_int_params);
>>> +
>>> int btbcm_patchram(struct hci_dev *hdev, const struct firmware *fw)
>>> {
>> 
>> Otherwise this looks good.
>> 
>> Regards
>> 
>> Marcel
>> 
> 
> So generally, I've done a whole new patch series with every change.
> Would you prefer to see singular updates on the same email thread or
> should I keep doing new patch series?

That is fine by me. I will start applying individual patches if possible and we get the tested-by or ACKs for it where I need them.

Regards

Marcel


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

* Re: [PATCH v6 3/4] dt-bindings: net: broadcom-bluetooth: Add pcm config
  2019-11-18 19:21 ` [PATCH v6 3/4] dt-bindings: net: broadcom-bluetooth: Add pcm config Abhishek Pandit-Subedi
  2019-11-19  5:39   ` Marcel Holtmann
@ 2019-11-21 21:29   ` Rob Herring
  2019-11-22 12:34     ` Marcel Holtmann
  1 sibling, 1 reply; 25+ messages in thread
From: Rob Herring @ 2019-11-21 21:29 UTC (permalink / raw)
  To: Abhishek Pandit-Subedi
  Cc: Marcel Holtmann, Johan Hedberg, linux-bluetooth, dianders,
	devicetree, David S. Miller, netdev, linux-kernel, Ondrej Jirman,
	Mark Rutland, Chen-Yu Tsai

On Mon, Nov 18, 2019 at 11:21:22AM -0800, Abhishek Pandit-Subedi wrote:
> Add documentation for pcm parameters.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> 
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None

Really? I'm staring at v2 that looks a bit different.

>  .../bindings/net/broadcom-bluetooth.txt       | 16 ++++++++++
>  include/dt-bindings/bluetooth/brcm.h          | 32 +++++++++++++++++++
>  2 files changed, 48 insertions(+)
>  create mode 100644 include/dt-bindings/bluetooth/brcm.h
> 
> diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> index c749dc297624..8561e4684378 100644
> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> @@ -29,10 +29,20 @@ Optional properties:
>     - "lpo": external low power 32.768 kHz clock
>   - vbat-supply: phandle to regulator supply for VBAT
>   - vddio-supply: phandle to regulator supply for VDDIO
> + - brcm,bt-sco-routing: PCM, Transport, Codec, I2S
> + - brcm,bt-pcm-interface-rate: 128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps
> + - brcm,bt-pcm-frame-type: short, long
> + - brcm,bt-pcm-sync-mode: slave, master
> + - brcm,bt-pcm-clock-mode: slave, master

Little of this seems unique to Broadcom. We already have some standard 
audio related properties for audio interfaces such as 'format', 
'frame-master' and 'bitclock-master'. Ultimately, this would be tied 
into the audio complex of SoCs and need to work with the audio 
bindings. We also have HDMI audio bindings. 

Maybe sco-routing is unique to BT and still needed in some form though 
if you describe the connection to the SoC audio complex, then maybe 
not? I'd assume every BT chip has some audio routing configuration.

Rob

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

* Re: [PATCH v6 3/4] dt-bindings: net: broadcom-bluetooth: Add pcm config
  2019-11-21 21:29   ` Rob Herring
@ 2019-11-22 12:34     ` Marcel Holtmann
  2019-11-22 15:50       ` Rob Herring
  0 siblings, 1 reply; 25+ messages in thread
From: Marcel Holtmann @ 2019-11-22 12:34 UTC (permalink / raw)
  To: Rob Herring
  Cc: Abhishek Pandit-Subedi, Johan Hedberg, linux-bluetooth,
	Douglas Anderson, devicetree, David S. Miller, netdev,
	linux-kernel, Ondrej Jirman, Mark Rutland, Chen-Yu Tsai

Hi Rob,

>> Add documentation for pcm parameters.
>> 
>> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>> ---
>> 
>> Changes in v6: None
>> Changes in v5: None
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
> 
> Really? I'm staring at v2 that looks a bit different.
> 
>> .../bindings/net/broadcom-bluetooth.txt       | 16 ++++++++++
>> include/dt-bindings/bluetooth/brcm.h          | 32 +++++++++++++++++++
>> 2 files changed, 48 insertions(+)
>> create mode 100644 include/dt-bindings/bluetooth/brcm.h
>> 
>> diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>> index c749dc297624..8561e4684378 100644
>> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>> @@ -29,10 +29,20 @@ Optional properties:
>>    - "lpo": external low power 32.768 kHz clock
>>  - vbat-supply: phandle to regulator supply for VBAT
>>  - vddio-supply: phandle to regulator supply for VDDIO
>> + - brcm,bt-sco-routing: PCM, Transport, Codec, I2S
>> + - brcm,bt-pcm-interface-rate: 128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps
>> + - brcm,bt-pcm-frame-type: short, long
>> + - brcm,bt-pcm-sync-mode: slave, master
>> + - brcm,bt-pcm-clock-mode: slave, master
> 
> Little of this seems unique to Broadcom. We already have some standard 
> audio related properties for audio interfaces such as 'format', 
> 'frame-master' and 'bitclock-master'. Ultimately, this would be tied 
> into the audio complex of SoCs and need to work with the audio 
> bindings. We also have HDMI audio bindings. 
> 
> Maybe sco-routing is unique to BT and still needed in some form though 
> if you describe the connection to the SoC audio complex, then maybe 
> not? I'd assume every BT chip has some audio routing configuration.

so we tried to generalize this some time before and failed to get a proper consensus.

In general I am with you that we should just expose generic properties from the attached audio codec, but nobody has come up with anything like that. And I think aligning all chip manufacturers will take some time.

Maybe in the interim we just use brcm,bt-pcm-int-params = [00 00 ..] as initially proposed.

Regards

Marcel


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

* Re: [PATCH v6 3/4] dt-bindings: net: broadcom-bluetooth: Add pcm config
  2019-11-22 12:34     ` Marcel Holtmann
@ 2019-11-22 15:50       ` Rob Herring
  2019-11-22 16:14         ` Marcel Holtmann
  0 siblings, 1 reply; 25+ messages in thread
From: Rob Herring @ 2019-11-22 15:50 UTC (permalink / raw)
  To: Marcel Holtmann, Abhishek Pandit-Subedi
  Cc: Johan Hedberg, linux-bluetooth, Douglas Anderson, devicetree,
	David S. Miller, netdev, linux-kernel, Ondrej Jirman,
	Mark Rutland, Chen-Yu Tsai

On Fri, Nov 22, 2019 at 6:34 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Rob,
>
> >> Add documentation for pcm parameters.
> >>
> >> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> >> ---
> >>
> >> Changes in v6: None
> >> Changes in v5: None
> >> Changes in v4: None
> >> Changes in v3: None
> >> Changes in v2: None
> >
> > Really? I'm staring at v2 that looks a bit different.
> >
> >> .../bindings/net/broadcom-bluetooth.txt       | 16 ++++++++++
> >> include/dt-bindings/bluetooth/brcm.h          | 32 +++++++++++++++++++
> >> 2 files changed, 48 insertions(+)
> >> create mode 100644 include/dt-bindings/bluetooth/brcm.h
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> >> index c749dc297624..8561e4684378 100644
> >> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> >> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> >> @@ -29,10 +29,20 @@ Optional properties:
> >>    - "lpo": external low power 32.768 kHz clock
> >>  - vbat-supply: phandle to regulator supply for VBAT
> >>  - vddio-supply: phandle to regulator supply for VDDIO
> >> + - brcm,bt-sco-routing: PCM, Transport, Codec, I2S
> >> + - brcm,bt-pcm-interface-rate: 128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps
> >> + - brcm,bt-pcm-frame-type: short, long
> >> + - brcm,bt-pcm-sync-mode: slave, master
> >> + - brcm,bt-pcm-clock-mode: slave, master
> >
> > Little of this seems unique to Broadcom. We already have some standard
> > audio related properties for audio interfaces such as 'format',
> > 'frame-master' and 'bitclock-master'. Ultimately, this would be tied
> > into the audio complex of SoCs and need to work with the audio
> > bindings. We also have HDMI audio bindings.
> >
> > Maybe sco-routing is unique to BT and still needed in some form though
> > if you describe the connection to the SoC audio complex, then maybe
> > not? I'd assume every BT chip has some audio routing configuration.
>
> so we tried to generalize this some time before and failed to get a proper consensus.
>
> In general I am with you that we should just expose generic properties from the attached audio codec, but nobody has come up with anything like that. And I think aligning all chip manufacturers will take some time.
>

That shouldn't be hard. It's a solved problem for codecs and HDMI. I
don't think BT is any more complicated (ignoring phones). I suspect
it's not solved simply because no one wants to do the work beyond
their 1 BT device they care about ATM.

> Maybe in the interim we just use brcm,bt-pcm-int-params = [00 00 ..] as initially proposed.

What's the device using this? Some chromebook I suppose. I think it
would be better to first see how this fits in with the rest of the
audio subsystem. Until then, the driver should probably just default
to "transport" mode which I assume is audio routed over the UART
interface. That should work on any platform at least, but may not be
optimal.

Rob

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

* Re: [PATCH v6 3/4] dt-bindings: net: broadcom-bluetooth: Add pcm config
  2019-11-22 15:50       ` Rob Herring
@ 2019-11-22 16:14         ` Marcel Holtmann
  0 siblings, 0 replies; 25+ messages in thread
From: Marcel Holtmann @ 2019-11-22 16:14 UTC (permalink / raw)
  To: Rob Herring
  Cc: Abhishek Pandit-Subedi, Johan Hedberg, linux-bluetooth,
	Douglas Anderson, devicetree, David S. Miller, netdev,
	linux-kernel, Ondrej Jirman, Mark Rutland, Chen-Yu Tsai

Hi Rob,

>>>> Add documentation for pcm parameters.
>>>> 
>>>> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>>>> ---
>>>> 
>>>> Changes in v6: None
>>>> Changes in v5: None
>>>> Changes in v4: None
>>>> Changes in v3: None
>>>> Changes in v2: None
>>> 
>>> Really? I'm staring at v2 that looks a bit different.
>>> 
>>>> .../bindings/net/broadcom-bluetooth.txt       | 16 ++++++++++
>>>> include/dt-bindings/bluetooth/brcm.h          | 32 +++++++++++++++++++
>>>> 2 files changed, 48 insertions(+)
>>>> create mode 100644 include/dt-bindings/bluetooth/brcm.h
>>>> 
>>>> diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>>>> index c749dc297624..8561e4684378 100644
>>>> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>>>> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>>>> @@ -29,10 +29,20 @@ Optional properties:
>>>>   - "lpo": external low power 32.768 kHz clock
>>>> - vbat-supply: phandle to regulator supply for VBAT
>>>> - vddio-supply: phandle to regulator supply for VDDIO
>>>> + - brcm,bt-sco-routing: PCM, Transport, Codec, I2S
>>>> + - brcm,bt-pcm-interface-rate: 128KBps, 256KBps, 512KBps, 1024KBps, 2048KBps
>>>> + - brcm,bt-pcm-frame-type: short, long
>>>> + - brcm,bt-pcm-sync-mode: slave, master
>>>> + - brcm,bt-pcm-clock-mode: slave, master
>>> 
>>> Little of this seems unique to Broadcom. We already have some standard
>>> audio related properties for audio interfaces such as 'format',
>>> 'frame-master' and 'bitclock-master'. Ultimately, this would be tied
>>> into the audio complex of SoCs and need to work with the audio
>>> bindings. We also have HDMI audio bindings.
>>> 
>>> Maybe sco-routing is unique to BT and still needed in some form though
>>> if you describe the connection to the SoC audio complex, then maybe
>>> not? I'd assume every BT chip has some audio routing configuration.
>> 
>> so we tried to generalize this some time before and failed to get a proper consensus.
>> 
>> In general I am with you that we should just expose generic properties from the attached audio codec, but nobody has come up with anything like that. And I think aligning all chip manufacturers will take some time.
>> 
> 
> That shouldn't be hard. It's a solved problem for codecs and HDMI. I
> don't think BT is any more complicated (ignoring phones). I suspect
> it's not solved simply because no one wants to do the work beyond
> their 1 BT device they care about ATM.

we tried, but nobody can agree on these right now. I would be happy if others come forward and tell us how they wired up their hardware, but it hasn’t happened yet.

>> Maybe in the interim we just use brcm,bt-pcm-int-params = [00 00 ..] as initially proposed.
> 
> What's the device using this? Some chromebook I suppose. I think it
> would be better to first see how this fits in with the rest of the
> audio subsystem. Until then, the driver should probably just default
> to "transport" mode which I assume is audio routed over the UART
> interface. That should work on any platform at least, but may not be
> optimal.

SCO over UART doesn’t really work. Long time ago, some car kits might have done it, but in the Chromebook cases this will just not work. We need to configure the PCM settings of the Bluetooth chip.

If we don’t do it via DT, then this gets hardcoded in the driver source and that is not helping either. So until we get anything better, lets use brcm,bt-pcm-int-params = [00 00 ..] and get this supported upstream.

Regards

Marcel


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

* Re: [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support
  2019-11-18 19:21 [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support Abhishek Pandit-Subedi
                   ` (3 preceding siblings ...)
  2019-11-18 19:21 ` [PATCH v6 4/4] Bluetooth: hci_bcm: Support pcm params in dts Abhishek Pandit-Subedi
@ 2019-11-23 10:04 ` Marcel Holtmann
  2019-11-25 18:20   ` Abhishek Pandit-Subedi
  4 siblings, 1 reply; 25+ messages in thread
From: Marcel Holtmann @ 2019-11-23 10:04 UTC (permalink / raw)
  To: Abhishek Pandit-Subedi
  Cc: Johan Hedberg, Rob Herring, linux-bluetooth, dianders,
	devicetree, David S. Miller, netdev, linux-kernel, Ondrej Jirman,
	Mark Rutland, Chen-Yu Tsai

Hi Abhishek,

> While adding support for the BCM4354, I discovered a few more things
> that weren't working as they should have.
> 
> First, we disallow serdev from setting the baudrate on BCM4354. Serdev
> sets the oper_speed first before calling hu->setup() in
> hci_uart_setup(). On the BCM4354, this results in bcm_setup() failing
> when the hci reset times out.
> 
> Next, we add support for setting the PCM parameters, which consists of
> a pair of vendor specific opcodes to set the pcm parameters. The
> documentation for these params are available in the brcm_patchram_plus
> package (i.e. https://github.com/balena-os/brcm_patchram_plus). This is
> necessary for PCM to work properly.
> 
> All changes were tested with rk3288-veyron-minnie.dts.

so I have re-factored your patch set now to apply to latest bluetooth-next tree and posted it to the mailing list. Please have a look at it if this works for you. If it does, then we might just apply it this way and focus on getting detailed PCM codec configuration for all vendors in once we have a second vendor to unify it.

Regards

Marcel


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

* Re: [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support
  2019-11-23 10:04 ` [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support Marcel Holtmann
@ 2019-11-25 18:20   ` Abhishek Pandit-Subedi
  2019-11-26  7:19     ` Marcel Holtmann
  0 siblings, 1 reply; 25+ messages in thread
From: Abhishek Pandit-Subedi @ 2019-11-25 18:20 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Rob Herring, linux-bluetooth, Douglas Anderson,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	David S. Miller, netdev, LKML, Ondrej Jirman, Mark Rutland,
	Chen-Yu Tsai

Hey,

It looks about the same as one of my earlier patch series. Outside a
few nitpicks, I'm ok with merging this.

Thanks
Abhishek

On Sat, Nov 23, 2019 at 2:04 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Abhishek,
>
> > While adding support for the BCM4354, I discovered a few more things
> > that weren't working as they should have.
> >
> > First, we disallow serdev from setting the baudrate on BCM4354. Serdev
> > sets the oper_speed first before calling hu->setup() in
> > hci_uart_setup(). On the BCM4354, this results in bcm_setup() failing
> > when the hci reset times out.
> >
> > Next, we add support for setting the PCM parameters, which consists of
> > a pair of vendor specific opcodes to set the pcm parameters. The
> > documentation for these params are available in the brcm_patchram_plus
> > package (i.e. https://github.com/balena-os/brcm_patchram_plus). This is
> > necessary for PCM to work properly.
> >
> > All changes were tested with rk3288-veyron-minnie.dts.
>
> so I have re-factored your patch set now to apply to latest bluetooth-next tree and posted it to the mailing list. Please have a look at it if this works for you. If it does, then we might just apply it this way and focus on getting detailed PCM codec configuration for all vendors in once we have a second vendor to unify it.
>
> Regards
>
> Marcel
>

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

* Re: [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support
  2019-11-25 18:20   ` Abhishek Pandit-Subedi
@ 2019-11-26  7:19     ` Marcel Holtmann
  2019-11-26 20:40       ` Abhishek Pandit-Subedi
  0 siblings, 1 reply; 25+ messages in thread
From: Marcel Holtmann @ 2019-11-26  7:19 UTC (permalink / raw)
  To: Abhishek Pandit-Subedi
  Cc: Johan Hedberg, Rob Herring, Bluez mailing list, Douglas Anderson,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	David S. Miller, netdev, LKML, Ondrej Jirman, Mark Rutland,
	Chen-Yu Tsai

Hi Abhishek,

> It looks about the same as one of my earlier patch series. Outside a
> few nitpicks, I'm ok with merging this.

I fixed the nitpicks up and send a v2.

However we should still work towards a generic description of Bluetooth PCM settings for all vendors. Any ideas are welcome.

Regards

Marcel


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

* Re: [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support
  2019-11-26  7:19     ` Marcel Holtmann
@ 2019-11-26 20:40       ` Abhishek Pandit-Subedi
  2019-11-27  5:37         ` Marcel Holtmann
  0 siblings, 1 reply; 25+ messages in thread
From: Abhishek Pandit-Subedi @ 2019-11-26 20:40 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Rob Herring, Bluez mailing list, Douglas Anderson,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	David S. Miller, netdev, LKML, Ondrej Jirman, Mark Rutland,
	Chen-Yu Tsai

Hey Marcel,

The series looks good to me.

Thanks
Abhishek

On Mon, Nov 25, 2019 at 11:19 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Abhishek,
>
> > It looks about the same as one of my earlier patch series. Outside a
> > few nitpicks, I'm ok with merging this.
>
> I fixed the nitpicks up and send a v2.
>
> However we should still work towards a generic description of Bluetooth PCM settings for all vendors. Any ideas are welcome.
>
> Regards
>
> Marcel
>

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

* Re: [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support
  2019-11-26 20:40       ` Abhishek Pandit-Subedi
@ 2019-11-27  5:37         ` Marcel Holtmann
  2019-11-27 22:14           ` Abhishek Pandit-Subedi
  0 siblings, 1 reply; 25+ messages in thread
From: Marcel Holtmann @ 2019-11-27  5:37 UTC (permalink / raw)
  To: Abhishek Pandit-Subedi
  Cc: Johan Hedberg, Rob Herring, Bluez mailing list, Douglas Anderson,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	David S. Miller, netdev, LKML, Ondrej Jirman, Mark Rutland,
	Chen-Yu Tsai

Hi Abhishek,

> The series looks good to me.

you also tested it on your hardware?

Regards

Marcel


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

* Re: [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support
  2019-11-27  5:37         ` Marcel Holtmann
@ 2019-11-27 22:14           ` Abhishek Pandit-Subedi
  2019-11-27 22:18             ` Marcel Holtmann
  0 siblings, 1 reply; 25+ messages in thread
From: Abhishek Pandit-Subedi @ 2019-11-27 22:14 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: Johan Hedberg, Rob Herring, Bluez mailing list, Douglas Anderson,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	David S. Miller, netdev, LKML, Ondrej Jirman, Mark Rutland,
	Chen-Yu Tsai

On Tue, Nov 26, 2019 at 9:37 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Abhishek,
>
> > The series looks good to me.
>
> you also tested it on your hardware?
>
> Regards
>
> Marcel
>

I have tested it on my hardware and it looks good now.

Only problem is it looks like the documentation is slightly wrong:

+               brcm,bt-pcm-int-params = [1 2 0 1 1];
should be
+               brcm,bt-pcm-int-params = [01 02 00 01 01];
or
+               brcm,bt-pcm-int-params = /bits/ 8 <1 2 0 1 1>;

Thanks
Abhishek

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

* Re: [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support
  2019-11-27 22:14           ` Abhishek Pandit-Subedi
@ 2019-11-27 22:18             ` Marcel Holtmann
  0 siblings, 0 replies; 25+ messages in thread
From: Marcel Holtmann @ 2019-11-27 22:18 UTC (permalink / raw)
  To: Abhishek Pandit-Subedi
  Cc: Johan Hedberg, Rob Herring, Bluez mailing list, Douglas Anderson,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	David S. Miller, netdev, LKML, Ondrej Jirman, Mark Rutland,
	Chen-Yu Tsai

Hi Abhishek,

>>> The series looks good to me.
>> 
>> you also tested it on your hardware?
>> 
>> Regards
>> 
>> Marcel
>> 
> 
> I have tested it on my hardware and it looks good now.
> 
> Only problem is it looks like the documentation is slightly wrong:
> 
> +               brcm,bt-pcm-int-params = [1 2 0 1 1];
> should be
> +               brcm,bt-pcm-int-params = [01 02 00 01 01];
> or
> +               brcm,bt-pcm-int-params = /bits/ 8 <1 2 0 1 1>;
> 

since Johan already applied the patches, send a follow up patch for the docs.

Regards

Marcel


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

end of thread, other threads:[~2019-11-27 22:18 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-18 19:21 [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support Abhishek Pandit-Subedi
2019-11-18 19:21 ` [PATCH v6 1/4] Bluetooth: hci_bcm: Disallow set_baudrate for BCM4354 Abhishek Pandit-Subedi
2019-11-19  5:25   ` Marcel Holtmann
2019-11-18 19:21 ` [PATCH v6 2/4] Bluetooth: btbcm: Support pcm configuration Abhishek Pandit-Subedi
2019-11-19  5:35   ` Marcel Holtmann
2019-11-19 20:48     ` Abhishek Pandit-Subedi
2019-11-19 23:45       ` Marcel Holtmann
2019-11-18 19:21 ` [PATCH v6 3/4] dt-bindings: net: broadcom-bluetooth: Add pcm config Abhishek Pandit-Subedi
2019-11-19  5:39   ` Marcel Holtmann
2019-11-19 16:50     ` Doug Anderson
2019-11-21 21:29   ` Rob Herring
2019-11-22 12:34     ` Marcel Holtmann
2019-11-22 15:50       ` Rob Herring
2019-11-22 16:14         ` Marcel Holtmann
2019-11-18 19:21 ` [PATCH v6 4/4] Bluetooth: hci_bcm: Support pcm params in dts Abhishek Pandit-Subedi
2019-11-19  5:44   ` Marcel Holtmann
2019-11-19 20:45     ` Abhishek Pandit-Subedi
2019-11-19 23:42       ` Marcel Holtmann
2019-11-23 10:04 ` [PATCH v6 0/4] Bluetooth: hci_bcm: Additional changes for BCM4354 support Marcel Holtmann
2019-11-25 18:20   ` Abhishek Pandit-Subedi
2019-11-26  7:19     ` Marcel Holtmann
2019-11-26 20:40       ` Abhishek Pandit-Subedi
2019-11-27  5:37         ` Marcel Holtmann
2019-11-27 22:14           ` Abhishek Pandit-Subedi
2019-11-27 22:18             ` Marcel Holtmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).