Bluetooth: hci_bcm: Configure SCO routing automatically

PATCH In review Attila Tőkés 3 participants 12 messages Jun 5, 2018 – Jun 12, 2018

Subsystems (4)

Networking Network drivers Device tree bindings Bluetooth drivers

lore

Share & tools

lore

Attila Tőkés

author

2018-06-05 19:17 UTC #0 lore raw

PATCH 1/1 Bluetooth: hci_bcm: Configure SCO routing automatically

Signed-off-by Attila Tőkés <[email protected]>

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

drivers/bluetooth/hci_bcm.c +61 -0

index ddbd8c6a..50c73eef 100644
@@ -83,6 +83,16 @@
  * @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
+ *
+ *  SCO routing parameters:
+ *   used as the parameters for the bcm_set_pcm_int_params command
+ *	@sco_routing:
+ *	 >= 255 (skip SCO routing configuration)
+ *	 0-3 (PCM, Transport, Codec, I2S)
+ *	@pcm_interface_rate: 0-4 (128 Kbps - 2048 Kbps)
+ *	@pcm_frame_type: 0 (short), 1 (long)
+ *	@pcm_sync_mode: 0 (slave), 1 (master)
+ *	@pcm_clock_mode: 0 (slave), 1 (master)
  */
 struct bcm_device {
 	/* Must be the first member, hci_serdev.c expects this. */

@@ -114,6 +124,13 @@ struct bcm_device {
 	struct hci_uart		*hu;
 	bool			is_suspended;
 #endif
+
+	/* SCO routing parameters */
+	u32			sco_routing;
+	u32			pcm_interface_rate;
+	u32			pcm_frame_type;
+	u32			pcm_sync_mode;
+	u32			pcm_clock_mode;
 };
 
 /* generic bcm uart resources */

@@ -189,6 +206,35 @@ static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
 	return 0;
 }
 
+static int bcm_configure_sco_routing(struct hci_uart *hu, struct bcm_device *bcm_dev)
+{
+	struct hci_dev *hdev = hu->hdev;
+	struct sk_buff *skb;
+	struct bcm_set_pcm_int_params params;
+
+	bt_dev_dbg(hdev, "BCM: Configuring SCO routing (%d %d %d %d %d)",
+			bcm_dev->sco_routing, bcm_dev->pcm_interface_rate, bcm_dev->pcm_frame_type,
+			bcm_dev->pcm_sync_mode,	bcm_dev->pcm_clock_mode);
+
+	params.routing = bcm_dev->sco_routing;
+	params.rate = bcm_dev->pcm_interface_rate;
+	params.frame_sync = bcm_dev->pcm_frame_type;
+	params.sync_mode = bcm_dev->pcm_sync_mode;
+	params.clock_mode = bcm_dev->pcm_clock_mode;
+
+	// send the SCO routing configuration command
+	skb = __hci_cmd_sync(hdev, 0xfc1c, sizeof(params), &params, HCI_CMD_TIMEOUT);
+	if (IS_ERR(skb)) {
+		int err = PTR_ERR(skb);
+		bt_dev_err(hdev, "BCM: failed to configure SCO routing (%d)", err);
+		return err;
+	}
+
+	kfree_skb(skb);
+
+	return 0;
+}
+
 /* bcm_device_exists should be protected by bcm_device_lock */
 static bool bcm_device_exists(struct bcm_device *device)
 {

@@ -534,6 +580,10 @@ static int bcm_setup(struct hci_uart *hu)
 			host_set_baudrate(hu, speed);
 	}
 
+	if (bcm->dev->sco_routing < 0xff) {
+		bcm_configure_sco_routing(hu, bcm->dev);
+	}
+
 finalize:
 	release_firmware(fw);

@@ -1007,6 +1057,11 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 static int bcm_of_probe(struct bcm_device *bdev)
 {
 	device_property_read_u32(bdev->dev, "max-speed", &bdev->oper_speed);
+	device_property_read_u32(bdev->dev, "sco-routing", &bdev->sco_routing);
+	device_property_read_u32(bdev->dev, "pcm-interface-rate", &bdev->pcm_interface_rate);
+	device_property_read_u32(bdev->dev, "pcm-frame-type", &bdev->pcm_frame_type);
+	device_property_read_u32(bdev->dev, "pcm-sync-mode", &bdev->pcm_sync_mode);
+	device_property_read_u32(bdev->dev, "pcm-clock-mode", &bdev->pcm_clock_mode);
 	return 0;
 }

@@ -1022,6 +1077,9 @@ static int bcm_probe(struct platform_device *pdev)
 	dev->dev = &pdev->dev;
 	dev->irq = platform_get_irq(pdev, 0);
 
+	/* SCO routing configuration is disabled by default */
+	dev->sco_routing = 0xff;
+
 	if (has_acpi_companion(&pdev->dev)) {
 		ret = bcm_acpi_probe(dev);
 		if (ret)

@@ -1281,6 +1339,9 @@ static int bcm_serdev_probe(struct serdev_device *serdev)
 	bcmdev->serdev_hu.serdev = serdev;
 	serdev_device_set_drvdata(serdev, bcmdev);
 
+	/* SCO routing configuration is disabled by default */
+	bcmdev->sco_routing = 0xff;
+
 	if (has_acpi_companion(&serdev->dev))
 		err = bcm_acpi_probe(bcmdev);
 	else

Signature · 1 line

2.17.0

[email protected]

author

2018-06-08 16:20 UTC #2 lore raw

PATCH 1/1 Bluetooth: hci_bcm: Configure SCO routing automatically

From: Attila Tőkés <[email protected]>

Added support to automatically configure the SCO packet routing at the device setup. The SCO packets are used with the HSP / HFP profiles, but in some devices (ex. CYW43438) they are routed to a PCM output by default. This change allows sending the vendor specific HCI command to configure the SCO routing. The parameters of the command are loaded from the device tree.

Signed-off-by Attila Tőkés <[email protected]>

---
 .../bindings/net/broadcom-bluetooth.txt       |  7 ++
 drivers/bluetooth/hci_bcm.c                   | 72 +++++++++++++++++++
 2 files changed, 79 insertions(+)

Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +7 -0

index 4194ff7e..aea3a094 100644
@@ -21,6 +21,12 @@ Optional properties:
  - clocks: clock specifier if external clock provided to the controller
  - clock-names: should be "extclk"
 
+ SCO routing parameters:
+ - sco-routing: 0-3 (PCM, Transport, Codec, I2S)
+ - pcm-interface-rate: 0-4 (128 Kbps - 2048 Kbps)
+ - pcm-frame-type: 0 (short), 1 (long)
+ - pcm-sync-mode: 0 (slave), 1 (master)
+ - pcm-clock-mode: 0 (slave), 1 (master)
 
 Example:

@@ -31,5 +37,6 @@ Example:
        bluetooth {
                compatible = "brcm,bcm43438-bt";
                max-speed = <921600>;
+               sco-routing = <1>; /* 1 = transport (UART) */
        };
 };

drivers/bluetooth/hci_bcm.c +72 -0

117 lines not on this page · raw message

Signature · 1 line

2.17.0

Attila Tőkés

author

2018-06-08 16:33 UTC #3 lore raw

Hi Marcel,

Fixed the problems from the first patch.

Related to the possibility of missing device tree parameters, the pcm-* parameters are only relevant for the sco-routing = 0 (PCM) case, in which case you probably want to explicitly set that parameters. For sco-routing = 1-3 (transport, codec, i2s) the pcm- parameters seems to be ignored, so I think it's safe to just send zeros.

Thanks,
Attila Tőkés

Quoted 185 lines · On Fri, Jun 8, 2018 at 7:20 PM, <[email protected]> wrote:

From: Attila Tőkés <[email protected]>

Signed-off-by Attila Tőkés <[email protected]>

---
 .../bindings/net/broadcom-bluetooth.txt       |  7 ++
 drivers/bluetooth/hci_bcm.c                   | 72 +++++++++++++++++++
 2 files changed, 79 insertions(+)

Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +7 -0

index 4194ff7e..aea3a094 100644
@@ -21,6 +21,12 @@ Optional properties:
  - clocks: clock specifier if external clock provided to the controller
  - clock-names: should be "extclk"
 
+ SCO routing parameters:
+ - sco-routing: 0-3 (PCM, Transport, Codec, I2S)
+ - pcm-interface-rate: 0-4 (128 Kbps - 2048 Kbps)
+ - pcm-frame-type: 0 (short), 1 (long)
+ - pcm-sync-mode: 0 (slave), 1 (master)
+ - pcm-clock-mode: 0 (slave), 1 (master)
 
 Example:

@@ -31,5 +37,6 @@ Example:
        bluetooth {
                compatible = "brcm,bcm43438-bt";
                max-speed = <921600>;
+               sco-routing = <1>; /* 1 = transport (UART) */
        };
 };

drivers/bluetooth/hci_bcm.c +0 -0

index ddbd8c6a..0e729534 100644
@@ -83,6 +83,16 @@
  * @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
+ *
+ * SCO routing parameters:
+ * used as the parameters for the bcm_set_pcm_int_params command
+ * @sco_routing:
+ * >= 255 (skip SCO routing configuration)
+ * 0-3 (PCM, Transport, Codec, I2S)

+ *     @pcm_interface_rate: 0-4 (128 Kbps - 2048 Kbps)
+ *     @pcm_frame_type: 0 (short), 1 (long)
+ *     @pcm_sync_mode: 0 (slave), 1 (master)
+ *     @pcm_clock_mode: 0 (slave), 1 (master)
  */
 struct bcm_device {
        /* Must be the first member, hci_serdev.c expects this. */

drivers/bluetooth/hci_bcm.c +30 -0

@@ -114,6 +124,13 @@ struct bcm_device {
        struct hci_uart         *hu;
        bool                    is_suspended;
 #endif
+
+       /* SCO routing parameters */
+       u8                      sco_routing;
+       u8                      pcm_interface_rate;
+       u8                      pcm_frame_type;
+       u8                      pcm_sync_mode;
+       u8                      pcm_clock_mode;
 };
 
 /* generic bcm uart resources */

@@ -189,6 +206,40 @@ static int bcm_set_baudrate(struct hci_uart *hu,
 unsigned int speed)
        return 0;
 }
 
+static int bcm_configure_sco_routing(struct hci_uart *hu, struct
 bcm_device *bcm_dev)
+{
+       struct hci_dev *hdev = hu->hdev;
+       struct sk_buff *skb;
+       struct bcm_set_pcm_int_params params;
+
+       if (bcm_dev->sco_routing >= 0xff) {
+               /* SCO routing configuration should be skipped */
+               return 0;
+       }
+
+       bt_dev_dbg(hdev, "BCM: Configuring SCO routing (%d %d %d %d %d)",
+                       bcm_dev->sco_routing, bcm_dev->pcm_interface_rate,
 bcm_dev->pcm_frame_type,
+                       bcm_dev->pcm_sync_mode, bcm_dev->pcm_clock_mode);
+
+       params.routing = bcm_dev->sco_routing;
+       params.rate = bcm_dev->pcm_interface_rate;
+       params.frame_sync = bcm_dev->pcm_frame_type;
+       params.sync_mode = bcm_dev->pcm_sync_mode;
+       params.clock_mode = bcm_dev->pcm_clock_mode;
+
+       /* Send the SCO routing configuration command */
+       skb = __hci_cmd_sync(hdev, 0xfc1c, sizeof(params), &params,

HCI_CMD_TIMEOUT);

drivers/bluetooth/hci_bcm.c +3 -0

+       if (IS_ERR(skb)) {
+               int err = PTR_ERR(skb);
+               bt_dev_err(hdev, "BCM: failed to configure SCO routing

(%d)", err);

drivers/bluetooth/hci_bcm.c +20 -0

+               return err;
+       }
+
+       kfree_skb(skb);
+
+       return 0;
+}
+
 /* bcm_device_exists should be protected by bcm_device_lock */
 static bool bcm_device_exists(struct bcm_device *device)
 {

@@ -534,6 +585,9 @@ static int bcm_setup(struct hci_uart *hu)
                        host_set_baudrate(hu, speed);
        }
 
+       /* Configure SCO routing if needed */
+       bcm_configure_sco_routing(hu, bcm->dev);
+
 finalize:
        release_firmware(fw);

@@ -1004,9 +1058,21 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 }
 #endif /* CONFIG_ACPI */
 
+static void read_u8_device_property(struct device *device, const char
 *property, u8 *destination) {
+       u32 temp;
+       if (device_property_read_u32(device, property, &temp) == 0) {
+               *destination = temp & 0xff;
+       }
+}
+
 static int bcm_of_probe(struct bcm_device *bdev)
 {
        device_property_read_u32(bdev->dev, "max-speed",
 &bdev->oper_speed);
+       read_u8_device_property(bdev->dev, "sco-routing",
 &bdev->sco_routing);
+       read_u8_device_property(bdev->dev, "pcm-interface-rate",

&bdev->pcm_interface_rate);
+ read_u8_device_property(bdev->dev, "pcm-frame-type",
&bdev->pcm_frame_type);
+ read_u8_device_property(bdev->dev, "pcm-sync-mode",
&bdev->pcm_sync_mode);
+ read_u8_device_property(bdev->dev, "pcm-clock-mode",

&bdev->pcm_clock_mode);
        return 0;

}

drivers/bluetooth/hci_bcm.c +6 -0

@@ -1022,6 +1088,9 @@ static int bcm_probe(struct platform_device *pdev)
        dev->dev = &pdev->dev;
        dev->irq = platform_get_irq(pdev, 0);
 
+       /* SCO routing configuration is disabled by default */
+       dev->sco_routing = 0xff;
+
        if (has_acpi_companion(&pdev->dev)) {
                ret = bcm_acpi_probe(dev);
                if (ret)

@@ -1281,6 +1350,9 @@ static int bcm_serdev_probe(struct serdev_device
 *serdev)
        bcmdev->serdev_hu.serdev = serdev;
        serdev_device_set_drvdata(serdev, bcmdev);
 
+       /* SCO routing configuration is disabled by default */
+       bcmdev->sco_routing = 0xff;
+
        if (has_acpi_companion(&serdev->dev))
                err = bcm_acpi_probe(bcmdev);

else
--
2.17.0

Rob Herring

2018-06-08 17:25 UTC #4 lore raw

In reply to [email protected]

Quoted 3 lines · On Fri, Jun 8, 2018 at 10:20 AM, <[email protected]> wrote:

From: Attila T=C5=91k=C3=A9s <[email protected]>
Added support to automatically configure the SCO packet routing at the de=

vice setup. The SCO packets are used with the HSP / HFP profiles, but in so= me devices (ex. CYW43438) they are routed to a PCM output by default. This = change allows sending the vendor specific HCI command to configure the SCO = routing. The parameters of the command are loaded from the device tree.

Please wrap your commit msg.

Quoted 4 lines

Signed-off-by Attila T=C5=91k=C3=A9s <[email protected]>
---
 .../bindings/net/broadcom-bluetooth.txt       |  7 ++

Please split bindings to separate patch.

Review

Quoted 4 lines

 drivers/bluetooth/hci_bcm.c                   | 72 +++++++++++++++++++
 2 files changed, 79 insertions(+)

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt

Documentation/devicetree/bindings/net/broadcom-bluetooth.txt Optional properties: · line 29

Quoted 13 lines

index 4194ff7e..aea3a094 100644

Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +6 -0

@@ -21,6 +21,12 @@ Optional properties:
  - clocks: clock specifier if external clock provided to the controller
  - clock-names: should be "extclk"
 
+ SCO routing parameters:
+ - sco-routing: 0-3 (PCM, Transport, Codec, I2S)
+ - pcm-interface-rate: 0-4 (128 Kbps - 2048 Kbps)
+ - pcm-frame-type: 0 (short), 1 (long)
+ - pcm-sync-mode: 0 (slave), 1 (master)
+ - pcm-clock-mode: 0 (slave), 1 (master)

Are these Broadcom specific? Properties need either vendor prefix or to be documented in a common location. I think these look like the latter.

However, this also looks incomplete to me. For example, which SoC I2S/PCM port is BT audio connected to and how does it fit into the existing audio related bindings? There's been work on HDMI audio bindings which would be similar (except for the SCO over UART at least).

Quoted 15 lines

Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +1 -0

 Example:

@@ -31,5 +37,6 @@ Example:
        bluetooth {
                compatible =3D "brcm,bcm43438-bt";
                max-speed =3D <921600>;
+               sco-routing =3D <1>; /* 1 =3D transport (UART) */
        };
 };

drivers/bluetooth/hci_bcm.c +0 -0

index ddbd8c6a..0e729534 100644

drivers/bluetooth/hci_bcm.c line 84

Quoted 2 lines

drivers/bluetooth/hci_bcm.c +0 -0

@@ -83,6 +83,16 @@
  * @hu: pointer to HCI UART controller struct,
  *     used to disable flow control during runtime suspend and system sl=

eep

drivers/bluetooth/hci_bcm.c static int bcm_set_baudrate(struct hci_uart *hu, uns=

Quoted 29 lines

drivers/bluetooth/hci_bcm.c +17 -0

  * @is_suspended: whether flow control is currently disabled
+ *
+ *  SCO routing parameters:
+ *   used as the parameters for the bcm_set_pcm_int_params command
+ *     @sco_routing:
+ *      >=3D 255 (skip SCO routing configuration)
+ *      0-3 (PCM, Transport, Codec, I2S)
+ *     @pcm_interface_rate: 0-4 (128 Kbps - 2048 Kbps)
+ *     @pcm_frame_type: 0 (short), 1 (long)
+ *     @pcm_sync_mode: 0 (slave), 1 (master)
+ *     @pcm_clock_mode: 0 (slave), 1 (master)
  */
 struct bcm_device {
        /* Must be the first member, hci_serdev.c expects this. */

@@ -114,6 +124,13 @@ struct bcm_device {
        struct hci_uart         *hu;
        bool                    is_suspended;
 #endif
+
+       /* SCO routing parameters */
+       u8                      sco_routing;
+       u8                      pcm_interface_rate;
+       u8                      pcm_frame_type;
+       u8                      pcm_sync_mode;
+       u8                      pcm_clock_mode;
 };
 
 /* generic bcm uart resources */

@@ -189,6 +206,40 @@ static int bcm_set_baudrate(struct hci_uart *hu, uns=

igned int speed)

drivers/bluetooth/hci_bcm.c static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)

Quoted 4 lines

drivers/bluetooth/hci_bcm.c +1 -0

        return 0;
 }
 
+static int bcm_configure_sco_routing(struct hci_uart *hu, struct bcm_dev=

ice *bcm_dev)

drivers/bluetooth/hci_bcm.c static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)

Quoted 12 lines

drivers/bluetooth/hci_bcm.c +12 -0

+{
+       struct hci_dev *hdev =3D hu->hdev;
+       struct sk_buff *skb;
+       struct bcm_set_pcm_int_params params;
+
+       if (bcm_dev->sco_routing >=3D 0xff) {
+               /* SCO routing configuration should be skipped */
+               return 0;
+       }
+
+       bt_dev_dbg(hdev, "BCM: Configuring SCO routing (%d %d %d %d %d)",
+                       bcm_dev->sco_routing, bcm_dev->pcm_interface_rate=

, bcm_dev->pcm_frame_type,

drivers/bluetooth/hci_bcm.c static int bcm_set_baudrate(struct hci_uart *hu, uns= · line 231

Quoted 10 lines

drivers/bluetooth/hci_bcm.c +10 -0

+                       bcm_dev->pcm_sync_mode, bcm_dev->pcm_clock_mode);
+
+       params.routing =3D bcm_dev->sco_routing;
+       params.rate =3D bcm_dev->pcm_interface_rate;
+       params.frame_sync =3D bcm_dev->pcm_frame_type;
+       params.sync_mode =3D bcm_dev->pcm_sync_mode;
+       params.clock_mode =3D bcm_dev->pcm_clock_mode;
+
+       /* Send the SCO routing configuration command */
+       skb =3D __hci_cmd_sync(hdev, 0xfc1c, sizeof(params), &params, HCI=

_CMD_TIMEOUT);

drivers/bluetooth/hci_bcm.c static int bcm_set_baudrate(struct hci_uart *hu, uns= · line 234

Quoted 3 lines

drivers/bluetooth/hci_bcm.c +3 -0

+       if (IS_ERR(skb)) {
+               int err =3D PTR_ERR(skb);
+               bt_dev_err(hdev, "BCM: failed to configure SCO routing (%=

d)", err);

Quoted 22 lines

drivers/bluetooth/hci_bcm.c +11 -0

+               return err;
+       }
+
+       kfree_skb(skb);
+
+       return 0;
+}
+
 /* bcm_device_exists should be protected by bcm_device_lock */
 static bool bcm_device_exists(struct bcm_device *device)
 {

@@ -534,6 +585,9 @@ static int bcm_setup(struct hci_uart *hu)
                        host_set_baudrate(hu, speed);
        }
 
+       /* Configure SCO routing if needed */
+       bcm_configure_sco_routing(hu, bcm->dev);
+
 finalize:
        release_firmware(fw);

drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *dev) · line 1061

Quoted 4 lines

drivers/bluetooth/hci_bcm.c +1 -0

@@ -1004,9 +1058,21 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 }
 #endif /* CONFIG_ACPI */
 
+static void read_u8_device_property(struct device *device, const char *p=

roperty, u8 *destination) {

drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *dev)

Quoted 9 lines

drivers/bluetooth/hci_bcm.c +6 -0

+       u32 temp;
+       if (device_property_read_u32(device, property, &temp) =3D=3D 0) {
+               *destination =3D temp & 0xff;
+       }
+}
+
 static int bcm_of_probe(struct bcm_device *bdev)
 {
        device_property_read_u32(bdev->dev, "max-speed", &bdev->oper_spee=

d);

drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *dev) · line 1071

Quoted 1 line

drivers/bluetooth/hci_bcm.c +1 -0

+       read_u8_device_property(bdev->dev, "sco-routing", &bdev->sco_rout=

ing);

drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *dev) · line 1072

Quoted 1 line

drivers/bluetooth/hci_bcm.c +1 -0

+       read_u8_device_property(bdev->dev, "pcm-interface-rate", &bdev->p=

cm_interface_rate);

drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *dev) · line 1073

Quoted 1 line

drivers/bluetooth/hci_bcm.c +1 -0

+       read_u8_device_property(bdev->dev, "pcm-frame-type", &bdev->pcm_f=

rame_type);

drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *dev) · line 1074

Quoted 1 line

drivers/bluetooth/hci_bcm.c +1 -0

+       read_u8_device_property(bdev->dev, "pcm-sync-mode", &bdev->pcm_sy=

nc_mode);

drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *dev) · line 1075

Quoted 1 line

drivers/bluetooth/hci_bcm.c +1 -0

+       read_u8_device_property(bdev->dev, "pcm-clock-mode", &bdev->pcm_c=

lock_mode);

These are actually broken because the DT properties are 32-bit.

Rob

Attila Tőkés

author

2018-06-09 06:26 UTC #5 lore raw

Hello Rob,

Quoted 11 lines · Rob Herring

Quoted 3 lines · On Fri, Jun 8, 2018 at 10:20 AM, <[email protected]> wrote:
From: Attila Tőkés <[email protected]>
Added support to automatically configure the SCO packet routing at the
device setup. The SCO packets are used with the HSP / HFP profiles, but in some devices (ex. CYW43438) they are routed to a PCM output by default. This change allows sending the vendor specific HCI command to configure the SCO routing. The parameters of the command are loaded from the device tree.
Please wrap your commit msg.

Sure.

Quoted 7 lines

Quoted 5 lines
Signed-off-by Attila Tőkés <[email protected]>
---
 .../bindings/net/broadcom-bluetooth.txt       |  7 ++
Please split bindings to separate patch.

Ok, I will split this in two.

Quoted 15 lines

Review
Quoted 7 lines
 drivers/bluetooth/hci_bcm.c                   | 72 +++++++++++++++++++
 2 files changed, 79 insertions(+)
(quoted diff) +0 -0
b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
Quoted 7 lines
index 4194ff7e..aea3a094 100644
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +0 -0
@@ -21,6 +21,12 @@ Optional properties:
  - clocks: clock specifier if external clock provided to the controller
  - clock-names: should be "extclk"
 

Quoted 10 lines

Documentation/devicetree/bindings/net/broadcom-bluetooth.txt Optional properties: · line 29
Quoted 6 lines
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +6 -0
+ SCO routing parameters:
+ - sco-routing: 0-3 (PCM, Transport, Codec, I2S)
+ - pcm-interface-rate: 0-4 (128 Kbps - 2048 Kbps)
+ - pcm-frame-type: 0 (short), 1 (long)
+ - pcm-sync-mode: 0 (slave), 1 (master)
+ - pcm-clock-mode: 0 (slave), 1 (master)
Are these Broadcom specific? Properties need either vendor prefix or to be documented in a common location. I think these look like the latter.

These will be used as parameters of a vendor specific (Broadcom/Cypress) command configuring the SCO packet routing. See the Write_SCO_PCM_Int_Param command from: http://www.cypress.com/file/298311/download.

What would be the property names with a Broadcom / Cypress vendor prefix?

brcm,sco-routing
brcm,pcm-interface-rate
brcm,pcm-frame-type
brcm,pcm-sync-mode
brcm,pcm-clock-mode

Quoted 1 line

However, this also looks incomplete to me. For example, which SoC

Quoted 5 lines

I2S/PCM port is BT audio connected to and how does it fit into the existing audio related bindings? There's been work on HDMI audio bindings which would be similar (except for the SCO over UART at least).

The I2S / PCM pins of the Bluetooth chip most likely will not be connected to the host SoC. When used with a SoC we probably want tor route the SCO packets over the UART transport. A2DP data already goes here and we probably want the same for HSP / HFP.

For example in the Raspberry Pi 3 B, the CYW43438's PCM output is not connected (there is no I2S output), But it may be connected to any other device in other hardware.

The purpose of this command is to tell the BT chip where to send the SCO packets. From the driver's perspective, it does not really matters where these pins are connected.

Quoted 126 lines

Quoted 17 lines

Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +1 -0

 Example:

@@ -31,5 +37,6 @@ Example:
        bluetooth {
                compatible = "brcm,bcm43438-bt";
                max-speed = <921600>;
+               sco-routing = <1>; /* 1 = transport (UART) */
        };
 };

drivers/bluetooth/hci_bcm.c +0 -0

index ddbd8c6a..0e729534 100644

drivers/bluetooth/hci_bcm.c line 84

Quoted 2 lines

drivers/bluetooth/hci_bcm.c +0 -0

@@ -83,6 +83,16 @@
  * @hu: pointer to HCI UART controller struct,
  *     used to disable flow control during runtime suspend and system

sleep

drivers/bluetooth/hci_bcm.c static int bcm_set_baudrate(struct hci_uart *hu,

Quoted 29 lines

drivers/bluetooth/hci_bcm.c +17 -0

  * @is_suspended: whether flow control is currently disabled
+ *
+ *  SCO routing parameters:
+ *   used as the parameters for the bcm_set_pcm_int_params command
+ *     @sco_routing:
+ *      >= 255 (skip SCO routing configuration)
+ *      0-3 (PCM, Transport, Codec, I2S)
+ *     @pcm_interface_rate: 0-4 (128 Kbps - 2048 Kbps)
+ *     @pcm_frame_type: 0 (short), 1 (long)
+ *     @pcm_sync_mode: 0 (slave), 1 (master)
+ *     @pcm_clock_mode: 0 (slave), 1 (master)
  */
 struct bcm_device {
        /* Must be the first member, hci_serdev.c expects this. */

@@ -114,6 +124,13 @@ struct bcm_device {
        struct hci_uart         *hu;
        bool                    is_suspended;
 #endif
+
+       /* SCO routing parameters */
+       u8                      sco_routing;
+       u8                      pcm_interface_rate;
+       u8                      pcm_frame_type;
+       u8                      pcm_sync_mode;
+       u8                      pcm_clock_mode;
 };
 
 /* generic bcm uart resources */

@@ -189,6 +206,40 @@ static int bcm_set_baudrate(struct hci_uart *hu,

unsigned int speed)

drivers/bluetooth/hci_bcm.c static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)

Quoted 4 lines

drivers/bluetooth/hci_bcm.c +1 -0

        return 0;
 }
 
+static int bcm_configure_sco_routing(struct hci_uart *hu, struct

bcm_device *bcm_dev)

drivers/bluetooth/hci_bcm.c static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)

Quoted 12 lines

drivers/bluetooth/hci_bcm.c +12 -0

+{
+       struct hci_dev *hdev = hu->hdev;
+       struct sk_buff *skb;
+       struct bcm_set_pcm_int_params params;
+
+       if (bcm_dev->sco_routing >= 0xff) {
+               /* SCO routing configuration should be skipped */
+               return 0;
+       }
+
+       bt_dev_dbg(hdev, "BCM: Configuring SCO routing (%d %d %d %d %d)",
+                       bcm_dev->sco_routing,

bcm_dev->pcm_interface_rate, bcm_dev->pcm_frame_type,

drivers/bluetooth/hci_bcm.c static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)

Quoted 10 lines

drivers/bluetooth/hci_bcm.c +10 -0

+                       bcm_dev->pcm_sync_mode, bcm_dev->pcm_clock_mode);
+
+       params.routing = bcm_dev->sco_routing;
+       params.rate = bcm_dev->pcm_interface_rate;
+       params.frame_sync = bcm_dev->pcm_frame_type;
+       params.sync_mode = bcm_dev->pcm_sync_mode;
+       params.clock_mode = bcm_dev->pcm_clock_mode;
+
+       /* Send the SCO routing configuration command */
+       skb = __hci_cmd_sync(hdev, 0xfc1c, sizeof(params), &params,

HCI_CMD_TIMEOUT);

drivers/bluetooth/hci_bcm.c static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)

Quoted 3 lines

drivers/bluetooth/hci_bcm.c +3 -0

+       if (IS_ERR(skb)) {
+               int err = PTR_ERR(skb);
+               bt_dev_err(hdev, "BCM: failed to configure SCO routing

(%d)", err);

Quoted 22 lines

drivers/bluetooth/hci_bcm.c +11 -0

+               return err;
+       }
+
+       kfree_skb(skb);
+
+       return 0;
+}
+
 /* bcm_device_exists should be protected by bcm_device_lock */
 static bool bcm_device_exists(struct bcm_device *device)
 {

@@ -534,6 +585,9 @@ static int bcm_setup(struct hci_uart *hu)
                        host_set_baudrate(hu, speed);
        }
 
+       /* Configure SCO routing if needed */
+       bcm_configure_sco_routing(hu, bcm->dev);
+
 finalize:
        release_firmware(fw);

drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *dev) · line 1061

Quoted 4 lines

drivers/bluetooth/hci_bcm.c +1 -0

@@ -1004,9 +1058,21 @@ static int bcm_acpi_probe(struct bcm_device *dev)
 }
 #endif /* CONFIG_ACPI */
 
+static void read_u8_device_property(struct device *device, const char

*property, u8 *destination) {

drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *dev)

Quoted 9 lines

drivers/bluetooth/hci_bcm.c +6 -0

+       u32 temp;
+       if (device_property_read_u32(device, property, &temp) == 0) {
+               *destination = temp & 0xff;
+       }
+}
+
 static int bcm_of_probe(struct bcm_device *bdev)
 {
        device_property_read_u32(bdev->dev, "max-speed",

&bdev->oper_speed);

drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *dev) · line 1071

Quoted 1 line

drivers/bluetooth/hci_bcm.c +1 -0
+       read_u8_device_property(bdev->dev, "sco-routing",

&bdev->sco_routing);

drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *dev) · line 1072

Quoted 1 line

drivers/bluetooth/hci_bcm.c +1 -0
+       read_u8_device_property(bdev->dev, "pcm-interface-rate",

&bdev->pcm_interface_rate);

drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *dev) · line 1073

Quoted 1 line

drivers/bluetooth/hci_bcm.c +1 -0
+       read_u8_device_property(bdev->dev, "pcm-frame-type",

&bdev->pcm_frame_type);

Quoted 8 lines

drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *dev) · line 1074
Quoted 1 line
drivers/bluetooth/hci_bcm.c +1 -0
+       read_u8_device_property(bdev->dev, "pcm-sync-mode",
&bdev->pcm_sync_mode);
drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *dev) · line 1075
Quoted 1 line
drivers/bluetooth/hci_bcm.c +1 -0
+       read_u8_device_property(bdev->dev, "pcm-clock-mode",
&bdev->pcm_clock_mode);
These are actually broken because the DT properties are 32-bit.
The properties from device tree are read as 32-bit values, then they are

truncated to 8-bit (see the read_u8_device_property function above). Is anything wrong with this?

Rob

Quoted 1 line

Thanks,
Attila

Rob Herring

2018-06-11 15:34 UTC #6 lore raw

Quoted 9 lines · On Sat, Jun 9, 2018 at 12:26 AM, Attila T=C5=91k=C3=A9s <[email protected]=…

Hello Rob,
Quoted 6 lines · Rob Herring
Quoted 4 lines · On Fri, Jun 8, 2018 at 10:20 AM, <[email protected]> wrote:
From: Attila T=C5=91k=C3=A9s <[email protected]>
Added support to automatically configure the SCO packet routing at the device setup. The SCO packets are used with the HSP / HFP profiles, bu=

t in

Quoted 1 line

Quoted 1 line
Quoted 1 line
some devices (ex. CYW43438) they are routed to a PCM output by default=

. This

Quoted 1 line

Quoted 1 line
Quoted 1 line
change allows sending the vendor specific HCI command to configure the=

SCO

Quoted 1 line

Quoted 1 line
Quoted 1 line
routing. The parameters of the command are loaded from the device tree=

Quoted 8 lines

Quoted 2 lines
Please wrap your commit msg.
Sure.
Quoted 3 lines
Quoted 3 lines

Quoted 13 lines

Quoted 5 lines
Quoted 3 lines
Signed-off-by Attila T=C5=91k=C3=A9s <[email protected]>
---
 .../bindings/net/broadcom-bluetooth.txt       |  7 ++
Please split bindings to separate patch.
Ok, I will split this in two.
Quoted 5 lines
Quoted 5 lines
drivers/bluetooth/hci_bcm.c | 72 ++++++++++++++++++=

Quoted 10 lines

Quoted 10 lines
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt Optional properties: · line 21
Quoted 10 lines
 2 files changed, 79 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt index 4194ff7e..aea3a094 100644
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +0 -0
@@ -21,6 +21,12 @@ Optional properties:
  - clocks: clock specifier if external clock provided to the controll=

Quoted 9 lines

Quoted 8 lines

Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +6 -0

  - clock-names: should be "extclk"
 
+ SCO routing parameters:
+ - sco-routing: 0-3 (PCM, Transport, Codec, I2S)
+ - pcm-interface-rate: 0-4 (128 Kbps - 2048 Kbps)
+ - pcm-frame-type: 0 (short), 1 (long)
+ - pcm-sync-mode: 0 (slave), 1 (master)
+ - pcm-clock-mode: 0 (slave), 1 (master)

Quoted 7 lines

Quoted 3 lines
Are these Broadcom specific? Properties need either vendor prefix or to be documented in a common location. I think these look like the latter.
These will be used as parameters of a vendor specific (Broadcom/Cypress) command configuring the SCO packet routing. See the Write_SCO_PCM_Int_Par=

Quoted 1 line

command from: http://www.cypress.com/file/298311/download.

The DT should just describe how the h/w is hooked-up. What the s/w has to do based on that is the driver's problem which is certainly vendor/chip specific, but that is all irrelevant to the binding.

Quoted 9 lines

What would be the property names with a Broadcom / Cypress vendor prefix?
brcm,sco-routing
brcm,pcm-interface-rate
brcm,pcm-frame-type
brcm,pcm-sync-mode
brcm,pcm-clock-mode
?

Yes.

Quoted 11 lines

Quoted 8 lines
However, this also looks incomplete to me. For example, which SoC I2S/PCM port is BT audio connected to and how does it fit into the existing audio related bindings? There's been work on HDMI audio bindings which would be similar (except for the SCO over UART at least).
The I2S / PCM pins of the Bluetooth chip most likely will not be connecte=

Quoted 2 lines

to the host SoC. When used with a SoC we probably want tor route the SCO packets over the UART transport. A2DP data already goes here and we proba=

bly

Quoted 1 line

want the same for HSP / HFP.

Then that should be the default with no properties.

I imagine for phones, vendors want I2S hooked up for voice calls so audio can be routed directly to/from the modem and the power hungry apps processor can be kept in low power modes.

Quoted 7 lines

For example in the Raspberry Pi 3 B, the CYW43438's PCM output is not connected (there is no I2S output), But it may be connected to any other device in other hardware.
The purpose of this command is to tell the BT chip where to send the SCO packets. From the driver's perspective, it does not really matters where these pins are connected.

Bindings are for h/w description (and config to some extent).

Quoted 97 lines

Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +1 -0

 Example:

@@ -31,5 +37,6 @@ Example:
        bluetooth {
                compatible =3D "brcm,bcm43438-bt";
                max-speed =3D <921600>;
+               sco-routing =3D <1>; /* 1 =3D transport (UART) */
        };
 };

drivers/bluetooth/hci_bcm.c +0 -0

index ddbd8c6a..0e729534 100644
@@ -83,6 +83,16 @@
  * @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
+ *
+ * SCO routing parameters:
+ * used as the parameters for the bcm_set_pcm_int_params command
+ * @sco_routing:
+ * >=3D 255 (skip SCO routing configuration)
+ * 0-3 (PCM, Transport, Codec, I2S)

+ *     @pcm_interface_rate: 0-4 (128 Kbps - 2048 Kbps)
+ *     @pcm_frame_type: 0 (short), 1 (long)
+ *     @pcm_sync_mode: 0 (slave), 1 (master)
+ *     @pcm_clock_mode: 0 (slave), 1 (master)
  */
 struct bcm_device {
        /* Must be the first member, hci_serdev.c expects this. */

drivers/bluetooth/hci_bcm.c +20 -0

@@ -114,6 +124,13 @@ struct bcm_device {
        struct hci_uart         *hu;
        bool                    is_suspended;
 #endif
+
+       /* SCO routing parameters */
+       u8                      sco_routing;
+       u8                      pcm_interface_rate;
+       u8                      pcm_frame_type;
+       u8                      pcm_sync_mode;
+       u8                      pcm_clock_mode;
 };
 
 /* generic bcm uart resources */

@@ -189,6 +206,40 @@ static int bcm_set_baudrate(struct hci_uart *hu,
 unsigned int speed)
        return 0;
 }
 
+static int bcm_configure_sco_routing(struct hci_uart *hu, struct
 bcm_device *bcm_dev)
+{
+       struct hci_dev *hdev =3D hu->hdev;
+       struct sk_buff *skb;
+       struct bcm_set_pcm_int_params params;
+
+       if (bcm_dev->sco_routing >=3D 0xff) {
+               /* SCO routing configuration should be skipped */
+               return 0;
+       }
+
+       bt_dev_dbg(hdev, "BCM: Configuring SCO routing (%d %d %d %d
 %d)",
+                       bcm_dev->sco_routing,

bcm_dev->pcm_interface_rate, bcm_dev->pcm_frame_type,
+ bcm_dev->pcm_sync_mode,
bcm_dev->pcm_clock_mode);

drivers/bluetooth/hci_bcm.c +9 -0

+
+       params.routing =3D bcm_dev->sco_routing;
+       params.rate =3D bcm_dev->pcm_interface_rate;
+       params.frame_sync =3D bcm_dev->pcm_frame_type;
+       params.sync_mode =3D bcm_dev->pcm_sync_mode;
+       params.clock_mode =3D bcm_dev->pcm_clock_mode;
+
+       /* Send the SCO routing configuration command */
+       skb =3D __hci_cmd_sync(hdev, 0xfc1c, sizeof(params), &params,

HCI_CMD_TIMEOUT);

drivers/bluetooth/hci_bcm.c +3 -0

+       if (IS_ERR(skb)) {
+               int err =3D PTR_ERR(skb);
+               bt_dev_err(hdev, "BCM: failed to configure SCO routing

(%d)", err);

drivers/bluetooth/hci_bcm.c +8 -0

+               return err;
+       }
+
+       kfree_skb(skb);
+
+       return 0;
+}
+
 /* bcm_device_exists should be protected by bcm_device_lock */
 static bool bcm_device_exists(struct bcm_device *device)
 {

@@ -534,6 +585,9 @@ static int bcm_setup(struct hci_uart *hu)
                        host_set_baudrate(hu, speed);
        }

Quoted 7 lines

Quoted 7 lines
drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *de=
Quoted 7 lines
drivers/bluetooth/hci_bcm.c +3 -0
+       /* Configure SCO routing if needed */
+       bcm_configure_sco_routing(hu, bcm->dev);
+
 finalize:
        release_firmware(fw);
 
@@ -1004,9 +1058,21 @@ static int bcm_acpi_probe(struct bcm_device *de=

Quoted 7 lines

Quoted 7 lines
drivers/bluetooth/hci_bcm.c static int bcm_acpi_probe(struct bcm_device *dev)
Quoted 7 lines
drivers/bluetooth/hci_bcm.c +3 -0
 }
 #endif /* CONFIG_ACPI */
 
+static void read_u8_device_property(struct device *device, const char
 *property, u8 *destination) {
+       u32 temp;
+       if (device_property_read_u32(device, property, &temp) =3D=3D 0=

) {

Quoted 21 lines

Quoted 12 lines
Quoted 12 lines
drivers/bluetooth/hci_bcm.c +6 -0
+               *destination =3D temp & 0xff;
+       }
+}
+
 static int bcm_of_probe(struct bcm_device *bdev)
 {
        device_property_read_u32(bdev->dev, "max-speed",
 &bdev->oper_speed);
+       read_u8_device_property(bdev->dev, "sco-routing",
 &bdev->sco_routing);
+       read_u8_device_property(bdev->dev, "pcm-interface-rate",
&bdev->pcm_interface_rate);
Quoted 9 lines
Quoted 6 lines
+ read_u8_device_property(bdev->dev, "pcm-frame-type",
&bdev->pcm_frame_type);
+ read_u8_device_property(bdev->dev, "pcm-sync-mode",
&bdev->pcm_sync_mode);
+ read_u8_device_property(bdev->dev, "pcm-clock-mode",
&bdev->pcm_clock_mode);
These are actually broken because the DT properties are 32-bit.

Quoted 3 lines

The properties from device tree are read as 32-bit values, then they are truncated to 8-bit (see the read_u8_device_property function above). Is anything wrong with this?

Ah, NM, I missed the wrapper.

Rob

Marcel Holtmann

maintainer

2018-06-11 15:47 UTC #7 lore raw

Hi Rob,

Quoted 66 lines

Quoted 50 lines
Quoted 7 lines
Quoted 5 lines
Added support to automatically configure the SCO packet routing at the device setup. The SCO packets are used with the HSP / HFP profiles, but in some devices (ex. CYW43438) they are routed to a PCM output by default. This change allows sending the vendor specific HCI command to configure the SCO routing. The parameters of the command are loaded from the device tree.
Please wrap your commit msg.
Sure.
Quoted 8 lines
Quoted 6 lines
Signed-off-by Attila Tőkés <[email protected]>
---
.../bindings/net/broadcom-bluetooth.txt | 7 ++
Please split bindings to separate patch.
Ok, I will split this in two.
Quoted 17 lines
Quoted 17 lines
drivers/bluetooth/hci_bcm.c | 72 +++++++++++++++++++
2 files changed, 79 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt index 4194ff7e..aea3a094 100644
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +0 -0
@@ -21,6 +21,12 @@ Optional properties:
 - clocks: clock specifier if external clock provided to the controller
 - clock-names: should be "extclk"
 
Quoted 10 lines
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt Optional properties: · line 29
Quoted 6 lines
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +6 -0
+ SCO routing parameters:
+ - sco-routing: 0-3 (PCM, Transport, Codec, I2S)
+ - pcm-interface-rate: 0-4 (128 Kbps - 2048 Kbps)
+ - pcm-frame-type: 0 (short), 1 (long)
+ - pcm-sync-mode: 0 (slave), 1 (master)
+ - pcm-clock-mode: 0 (slave), 1 (master)
Are these Broadcom specific? Properties need either vendor prefix or to be documented in a common location. I think these look like the latter.
Quoted 3 lines
These will be used as parameters of a vendor specific (Broadcom/Cypress) command configuring the SCO packet routing. See the Write_SCO_PCM_Int_Param command from: http://www.cypress.com/file/298311/download.
The DT should just describe how the h/w is hooked-up. What the s/w has to do based on that is the driver's problem which is certainly vendor/chip specific, but that is all irrelevant to the binding.
Quoted 8 lines
What would be the property names with a Broadcom / Cypress vendor prefix?
brcm,sco-routing
brcm,pcm-interface-rate
brcm,pcm-frame-type
brcm,pcm-sync-mode
brcm,pcm-clock-mode

Quoted 3 lines

Quoted 1 line
?
Yes.

we can do this. However all pcm-* are optional if you switch the HCI transport. And sco-routing should default to HCI if that is not present. Meaning a driver should actively trying to change this. Nevertheless, it would be good if a driver reads the current settings.

In theory we could make sco-routing generic, but so many vendors have different modes, that we better keep this vendor specific.

Regards

Marcel

Rob Herring

2018-06-11 17:54 UTC #8 lore raw

On Mon, Jun 11, 2018 at 9:47 AM, Marcel Holtmann <[email protected]> wrot= e:

Quoted 3 lines

Hi Rob,
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Added support to automatically configure the SCO packet routing at th=

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
device setup. The SCO packets are used with the HSP / HFP profiles, b=

ut in

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
some devices (ex. CYW43438) they are routed to a PCM output by defaul=

t. This

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
change allows sending the vendor specific HCI command to configure th=

e SCO

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
routing. The parameters of the command are loaded from the device tre=

Quoted 15 lines

Quoted 15 lines
Quoted 15 lines
Quoted 2 lines
Please wrap your commit msg.
Sure.
Quoted 8 lines
Quoted 6 lines
Signed-off-by Attila T=C5=91k=C3=A9s <[email protected]>
---
.../bindings/net/broadcom-bluetooth.txt | 7 ++
Please split bindings to separate patch.

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Ok, I will split this in two.
Quoted 5 lines
Quoted 5 lines
drivers/bluetooth/hci_bcm.c | 72 ++++++++++++++++++=

Quoted 10 lines

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt Optional properties: · line 21
Quoted 10 lines
2 files changed, 79 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt index 4194ff7e..aea3a094 100644
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +0 -0
@@ -21,6 +21,12 @@ Optional properties:
 - clocks: clock specifier if external clock provided to the controll=

Quoted 9 lines

Quoted 8 lines

Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +6 -0

 - clock-names: should be "extclk"
 
+ SCO routing parameters:
+ - sco-routing: 0-3 (PCM, Transport, Codec, I2S)
+ - pcm-interface-rate: 0-4 (128 Kbps - 2048 Kbps)
+ - pcm-frame-type: 0 (short), 1 (long)
+ - pcm-sync-mode: 0 (slave), 1 (master)
+ - pcm-clock-mode: 0 (slave), 1 (master)

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Are these Broadcom specific? Properties need either vendor prefix or to be documented in a common location. I think these look like the latter.
These will be used as parameters of a vendor specific (Broadcom/Cypress=

)

Quoted 1 line

Quoted 1 line
Quoted 1 line
command configuring the SCO packet routing. See the Write_SCO_PCM_Int_P=

aram

Quoted 7 lines

Quoted 7 lines
Quoted 1 line
command from: http://www.cypress.com/file/298311/download.
The DT should just describe how the h/w is hooked-up. What the s/w has to do based on that is the driver's problem which is certainly vendor/chip specific, but that is all irrelevant to the binding.
Quoted 1 line
What would be the property names with a Broadcom / Cypress vendor prefi=

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
brcm,sco-routing
brcm,pcm-interface-rate
brcm,pcm-frame-type
brcm,pcm-sync-mode
brcm,pcm-clock-mode

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
?
Yes.
we can do this. However all pcm-* are optional if you switch the HCI tran=

sport. And sco-routing should default to HCI if that is not present. Meanin= g a driver should actively trying to change this. Nevertheless, it would be=
good if a driver reads the current settings.

Quoted 2 lines

In theory we could make sco-routing generic, but so many vendors have dif=

ferent modes, that we better keep this vendor specific.

Even if vendor specific, the properties for not HCI transport case are still incomplete IMO.

By modes, you mean PCM vs. I2S and all the flavors of timings you can have within those or something else? For the former, that's often going to be a process of solving what each end support and if that doesn't work, then IIRC we already have properties for setting modes/timing. All the same issues exist with audio codecs and this is really not any different.

Rob

Marcel Holtmann

maintainer

2018-06-11 18:19 UTC #9 lore raw

Hi Rob,

Quoted 77 lines

Quoted 66 lines
Quoted 66 lines
Quoted 50 lines
Quoted 7 lines
Quoted 5 lines
Added support to automatically configure the SCO packet routing at the device setup. The SCO packets are used with the HSP / HFP profiles, but in some devices (ex. CYW43438) they are routed to a PCM output by default. This change allows sending the vendor specific HCI command to configure the SCO routing. The parameters of the command are loaded from the device tree.
Please wrap your commit msg.
Sure.
Quoted 8 lines
Quoted 6 lines
Signed-off-by Attila Tőkés <[email protected]>
---
.../bindings/net/broadcom-bluetooth.txt | 7 ++
Please split bindings to separate patch.
Ok, I will split this in two.
Quoted 17 lines
Quoted 17 lines
drivers/bluetooth/hci_bcm.c | 72 +++++++++++++++++++
2 files changed, 79 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt index 4194ff7e..aea3a094 100644
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +0 -2
@@ -21,6 +21,12 @@ Optional properties:
- clocks: clock specifier if external clock provided to the controller
- clock-names: should be "extclk"
 
Quoted 10 lines
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt Optional properties: · line 29
Quoted 6 lines
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +6 -0
+ SCO routing parameters:
+ - sco-routing: 0-3 (PCM, Transport, Codec, I2S)
+ - pcm-interface-rate: 0-4 (128 Kbps - 2048 Kbps)
+ - pcm-frame-type: 0 (short), 1 (long)
+ - pcm-sync-mode: 0 (slave), 1 (master)
+ - pcm-clock-mode: 0 (slave), 1 (master)
Are these Broadcom specific? Properties need either vendor prefix or to be documented in a common location. I think these look like the latter.
Quoted 3 lines
These will be used as parameters of a vendor specific (Broadcom/Cypress) command configuring the SCO packet routing. See the Write_SCO_PCM_Int_Param command from: http://www.cypress.com/file/298311/download.
The DT should just describe how the h/w is hooked-up. What the s/w has to do based on that is the driver's problem which is certainly vendor/chip specific, but that is all irrelevant to the binding.
Quoted 8 lines
What would be the property names with a Broadcom / Cypress vendor prefix?
brcm,sco-routing
brcm,pcm-interface-rate
brcm,pcm-frame-type
brcm,pcm-sync-mode
brcm,pcm-clock-mode
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
?
Yes.
we can do this. However all pcm-* are optional if you switch the HCI transport. And sco-routing should default to HCI if that is not present. Meaning a driver should actively trying to change this. Nevertheless, it would be good if a driver reads the current settings.
In theory we could make sco-routing generic, but so many vendors have different modes, that we better keep this vendor specific.
Even if vendor specific, the properties for not HCI transport case are still incomplete IMO.

Quoted 6 lines

By modes, you mean PCM vs. I2S and all the flavors of timings you can have within those or something else? For the former, that's often going to be a process of solving what each end support and if that doesn't work, then IIRC we already have properties for setting modes/timing. All the same issues exist with audio codecs and this is really not any different.

this is what Broadcom uses to configure their PCM transport. So I think for now, we make them brcm, specific and see how that goes. We can always generalize them later if enough chip manufactures provide support for it.

Regards

Marcel

Rob Herring

2018-06-11 19:05 UTC #10 lore raw

On Mon, Jun 11, 2018 at 12:19 PM, Marcel Holtmann <[email protected]> wro= te:

Quoted 3 lines

Hi Rob,
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Added support to automatically configure the SCO packet routing at =

the

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
device setup. The SCO packets are used with the HSP / HFP profiles,=

but in

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
some devices (ex. CYW43438) they are routed to a PCM output by defa=

ult. This

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
change allows sending the vendor specific HCI command to configure =

the SCO

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
routing. The parameters of the command are loaded from the device t=

ree.

Quoted 15 lines

Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 2 lines
Please wrap your commit msg.
Sure.
Quoted 8 lines
Quoted 6 lines
Signed-off-by Attila T=C5=91k=C3=A9s <[email protected]>
---
.../bindings/net/broadcom-bluetooth.txt | 7 ++
Please split bindings to separate patch.

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Ok, I will split this in two.
Quoted 5 lines
Quoted 5 lines
drivers/bluetooth/hci_bcm.c | 72 ++++++++++++++++=

+++

Quoted 10 lines

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt Optional properties: · line 21
Quoted 10 lines
2 files changed, 79 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt index 4194ff7e..aea3a094 100644
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +0 -1
@@ -21,6 +21,12 @@ Optional properties:
- clocks: clock specifier if external clock provided to the control=

ler

Quoted 9 lines

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +6 -1
- clock-names: should be "extclk"
 
+ SCO routing parameters:
+ - sco-routing: 0-3 (PCM, Transport, Codec, I2S)
+ - pcm-interface-rate: 0-4 (128 Kbps - 2048 Kbps)
+ - pcm-frame-type: 0 (short), 1 (long)
+ - pcm-sync-mode: 0 (slave), 1 (master)
+ - pcm-clock-mode: 0 (slave), 1 (master)

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Are these Broadcom specific? Properties need either vendor prefix or to be documented in a common location. I think these look like the latter.
These will be used as parameters of a vendor specific (Broadcom/Cypre=

ss)

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
command configuring the SCO packet routing. See the Write_SCO_PCM_Int=

_Param

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 1 line
command from: http://www.cypress.com/file/298311/download.
The DT should just describe how the h/w is hooked-up. What the s/w has to do based on that is the driver's problem which is certainly vendor/chip specific, but that is all irrelevant to the binding.
Quoted 1 line
What would be the property names with a Broadcom / Cypress vendor pre=

fix?

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
brcm,sco-routing
brcm,pcm-interface-rate
brcm,pcm-frame-type
brcm,pcm-sync-mode
brcm,pcm-clock-mode

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
?
Yes.
we can do this. However all pcm-* are optional if you switch the HCI tr=

ansport. And sco-routing should default to HCI if that is not present. Mean= ing a driver should actively trying to change this. Nevertheless, it would = be good if a driver reads the current settings.

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
In theory we could make sco-routing generic, but so many vendors have d=

ifferent modes, that we better keep this vendor specific.

Quoted 4 lines

Quoted 4 lines
Even if vendor specific, the properties for not HCI transport case are still incomplete IMO.

Quoted 8 lines

Quoted 6 lines
By modes, you mean PCM vs. I2S and all the flavors of timings you can have within those or something else? For the former, that's often going to be a process of solving what each end support and if that doesn't work, then IIRC we already have properties for setting modes/timing. All the same issues exist with audio codecs and this is really not any different.
this is what Broadcom uses to configure their PCM transport. So I think f=

or now, we make them brcm, specific and see how that goes. We can always ge= neralize them later if enough chip manufactures provide support for it.

We already have properties doing the same thing defined in Documentation/devicetree/bindings/sound/simple-card.txt. Use and extend that. We don't need new properties especially for something that is not complete. For example If I have 2 host ports (every SoC has at least 2), how do I indicate which one is connected to BT.

Rob

Attila Tőkés

author

2018-06-12 17:28 UTC #11 lore raw

Hi Marcel, Rob,

I was thinking about doing these changes in two parts:

1. Making in hci_bcm.c the SCO routed over HCI by default

As in the Broadcom chips the SCO seems to be routed to PCM by default, this may break the backward compatibility with some devices. I guess this would be a problem, so we will need a way to keep this backward compatible.

2. Adding support to configure SCO routed over the other transports (PCM / I2C / codec), based on the DT

I don't think we have clear solution yet here. The implementation could be vendor specific or a more generic one. And there is the modes / timing negotiation part that is unclear (at least for me).

What do you think?

Attila

Quoted 114 lines · Rob Herring

Quoted 3 lines · Marcel Holtmann
Hi Rob,
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Added support to automatically configure the SCO packet routing at
the
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
device setup. The SCO packets are used with the HSP / HFP
profiles, but in
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
some devices (ex. CYW43438) they are routed to a PCM output by
default. This
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
change allows sending the vendor specific HCI command to configure
the SCO
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
routing. The parameters of the command are loaded from the device
tree.
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 2 lines
Please wrap your commit msg.
Sure.
Quoted 8 lines
Quoted 6 lines
Signed-off-by Attila Tőkés <[email protected]>
---
.../bindings/net/broadcom-bluetooth.txt | 7 ++
Please split bindings to separate patch.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Ok, I will split this in two.
Quoted 5 lines
Quoted 5 lines
drivers/bluetooth/hci_bcm.c | 72
+++++++++++++++++++
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt Optional properties: · line 21
Quoted 10 lines
2 files changed, 79 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt index 4194ff7e..aea3a094 100644
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +0 -1
@@ -21,6 +21,12 @@ Optional properties:
- clocks: clock specifier if external clock provided to the
controller
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Documentation/devicetree/bindings/net/broadcom-bluetooth.txt +6 -1
- clock-names: should be "extclk"
 
+ SCO routing parameters:
+ - sco-routing: 0-3 (PCM, Transport, Codec, I2S)
+ - pcm-interface-rate: 0-4 (128 Kbps - 2048 Kbps)
+ - pcm-frame-type: 0 (short), 1 (long)
+ - pcm-sync-mode: 0 (slave), 1 (master)
+ - pcm-clock-mode: 0 (slave), 1 (master)
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Are these Broadcom specific? Properties need either vendor prefix or to be documented in a common location. I think these look like the latter.
These will be used as parameters of a vendor specific
(Broadcom/Cypress)
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
command configuring the SCO packet routing. See the
Write_SCO_PCM_Int_Param
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 1 line
command from: http://www.cypress.com/file/298311/download.
The DT should just describe how the h/w is hooked-up. What the s/w has to do based on that is the driver's problem which is certainly vendor/chip specific, but that is all irrelevant to the binding.
Quoted 1 line
What would be the property names with a Broadcom / Cypress vendor
prefix?
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
brcm,sco-routing
brcm,pcm-interface-rate
brcm,pcm-frame-type
brcm,pcm-sync-mode
brcm,pcm-clock-mode
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
?
Yes.
we can do this. However all pcm-* are optional if you switch the HCI
transport. And sco-routing should default to HCI if that is not present. Meaning a driver should actively trying to change this. Nevertheless, it would be good if a driver reads the current settings.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
In theory we could make sco-routing generic, but so many vendors have
different modes, that we better keep this vendor specific.
Quoted 4 lines
Quoted 4 lines
Even if vendor specific, the properties for not HCI transport case are still incomplete IMO.
Quoted 8 lines
Quoted 6 lines
By modes, you mean PCM vs. I2S and all the flavors of timings you can have within those or something else? For the former, that's often going to be a process of solving what each end support and if that doesn't work, then IIRC we already have properties for setting modes/timing. All the same issues exist with audio codecs and this is really not any different.
this is what Broadcom uses to configure their PCM transport. So I think
for now, we make them brcm, specific and see how that goes. We can always generalize them later if enough chip manufactures provide support for it.
We already have properties doing the same thing defined in Documentation/devicetree/bindings/sound/simple-card.txt. Use and extend that. We don't need new properties especially for something that is not complete. For example If I have 2 host ports (every SoC has at least 2), how do I indicate which one is connected to BT.
Rob