[PATCH] Bluetooth: hci_bcm: Configure SCO routing automatically

[PATCH] Bluetooth: hci_bcm: Configure SCO routing automatically

[attitokes]

From: Attila Tőkés <attitokes@gmail.com>

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 <attitokes@gmail.com>
---
 .../bindings/net/broadcom-bluetooth.txt       |  7 ++
 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
--- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
+++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
@@ -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) */
        };
 };
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index ddbd8c6a..0e729534 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -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 */
+	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);
+	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 +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;
 }
 
@@ -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

Re: [PATCH] Bluetooth: hci_bcm: Configure SCO routing automatically

[Attila Tőkés]

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

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

On Fri, Jun 8, 2018 at 7:20 PM, <attitokes@gmail.com> wrote:

> From: Attila Tőkés <attitokes@gmail.com>
>
> 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 <attitokes@gmail.com>
> ---
>  .../bindings/net/broadcom-bluetooth.txt       |  7 ++
>  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
> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> @@ -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) */
>         };
>  };
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index ddbd8c6a..0e729534 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -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 */
> +       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);
> +       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 +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;
>  }
>
> @@ -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
>
>

[-- Attachment #2: Type: text/html, Size: 10063 bytes --]

Re: [PATCH] Bluetooth: hci_bcm: Configure SCO routing automatically

[Rob Herring]

On Fri, Jun 8, 2018 at 10:20 AM,  <attitokes@gmail.com> wrote:
> From: Attila Tőkés <attitokes@gmail.com>
>
> 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.

>
> Signed-off-by: Attila Tőkés <attitokes@gmail.com>
> ---
>  .../bindings/net/broadcom-bluetooth.txt       |  7 ++

Please split bindings to separate patch.

>  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
> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> @@ -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).


>
>  Example:
>
> @@ -31,5 +37,6 @@ Example:
>         bluetooth {
>                 compatible = "brcm,bcm43438-bt";
>                 max-speed = <921600>;
> +               sco-routing = <1>; /* 1 = transport (UART) */
>         };
>  };
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index ddbd8c6a..0e729534 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -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 */
> +       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);
> +       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 +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);

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

Rob

Re: [PATCH] Bluetooth: hci_bcm: Configure SCO routing automatically

[Attila Tőkés]

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

Hello Rob,

On Fri, Jun 8, 2018 at 8:25 PM, Rob Herring <robh+dt@kernel.org> wrote:

> On Fri, Jun 8, 2018 at 10:20 AM,  <attitokes@gmail.com> wrote:
> > From: Attila Tőkés <attitokes@gmail.com>
> >
> > 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.

>
> >
> > Signed-off-by: Attila Tőkés <attitokes@gmail.com>
> > ---
> >  .../bindings/net/broadcom-bluetooth.txt       |  7 ++
>
> Please split bindings to separate patch.


Ok, I will split this in two.

>


> >  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
> > --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> > +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> > @@ -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.


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

?


>
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 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.

>
>
> >
> >  Example:
> >
> > @@ -31,5 +37,6 @@ Example:
> >         bluetooth {
> >                 compatible = "brcm,bcm43438-bt";
> >                 max-speed = <921600>;
> > +               sco-routing = <1>; /* 1 = transport (UART) */
> >         };
> >  };
> > diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> > index ddbd8c6a..0e729534 100644
> > --- a/drivers/bluetooth/hci_bcm.c
> > +++ b/drivers/bluetooth/hci_bcm.c
> > @@ -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 */
> > +       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);
> > +       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 +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);
>
> 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
>

Thanks,
Attila

[-- Attachment #2: Type: text/html, Size: 14216 bytes --]

Re: [PATCH] Bluetooth: hci_bcm: Configure SCO routing automatically

[Rob Herring]

On Sat, Jun 9, 2018 at 12:26 AM, Attila Tőkés <attitokes@gmail.com> wrote:
> Hello Rob,
>
> On Fri, Jun 8, 2018 at 8:25 PM, Rob Herring <robh+dt@kernel.org> wrote:
>>
>> On Fri, Jun 8, 2018 at 10:20 AM,  <attitokes@gmail.com> wrote:
>> > From: Attila Tőkés <attitokes@gmail.com>
>> >
>> > 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.
>>
>>
>> >
>> > Signed-off-by: Attila Tőkés <attitokes@gmail.com>
>> > ---
>> >  .../bindings/net/broadcom-bluetooth.txt       |  7 ++
>>
>> Please split bindings to separate patch.
>
>
> Ok, I will split this in two.
>>
>>
>>
>>
>> >  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
>> > --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>> > +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>> > @@ -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.
>
>
> 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.

> 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.

>
>>
>>
>> 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 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.

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.

> 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).


>> >  Example:
>> >
>> > @@ -31,5 +37,6 @@ Example:
>> >         bluetooth {
>> >                 compatible = "brcm,bcm43438-bt";
>> >                 max-speed = <921600>;
>> > +               sco-routing = <1>; /* 1 = transport (UART) */
>> >         };
>> >  };
>> > diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> > index ddbd8c6a..0e729534 100644
>> > --- a/drivers/bluetooth/hci_bcm.c
>> > +++ b/drivers/bluetooth/hci_bcm.c
>> > @@ -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 */
>> > +       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);
>> > +       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 +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);
>>
>> 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?
Ah, NM, I missed the wrapper.

Rob

Re: [PATCH] Bluetooth: hci_bcm: Configure SCO routing automatically

[Marcel Holtmann]

Hi Rob,

>>>> 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.
>>> 
>>> 
>>>> 
>>>> Signed-off-by: Attila Tőkés <attitokes@gmail.com>
>>>> ---
>>>> .../bindings/net/broadcom-bluetooth.txt       |  7 ++
>>> 
>>> Please split bindings to separate patch.
>> 
>> 
>> Ok, I will split this in two.
>>> 
>>> 
>>> 
>>> 
>>>> 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
>>>> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>>>> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>>>> @@ -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.
>> 
>> 
>> 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.
> 
>> 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.

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

Re: [PATCH] Bluetooth: hci_bcm: Configure SCO routing automatically

[Rob Herring]

On Mon, Jun 11, 2018 at 9:47 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Rob,
>
>>>>> 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.
>>>>
>>>>
>>>>>
>>>>> Signed-off-by: Attila Tőkés <attitokes@gmail.com>
>>>>> ---
>>>>> .../bindings/net/broadcom-bluetooth.txt       |  7 ++
>>>>
>>>> Please split bindings to separate patch.
>>>
>>>
>>> Ok, I will split this in two.
>>>>
>>>>
>>>>
>>>>
>>>>> 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
>>>>> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>>>>> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>>>>> @@ -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.
>>>
>>>
>>> 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.
>>
>>> 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.
>
> 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.

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

Re: [PATCH] Bluetooth: hci_bcm: Configure SCO routing automatically

[Marcel Holtmann]

Hi Rob,

>>>>>> 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.
>>>>> 
>>>>> 
>>>>>> 
>>>>>> Signed-off-by: Attila Tőkés <attitokes@gmail.com>
>>>>>> ---
>>>>>> .../bindings/net/broadcom-bluetooth.txt       |  7 ++
>>>>> 
>>>>> Please split bindings to separate patch.
>>>> 
>>>> 
>>>> Ok, I will split this in two.
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>>> 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
>>>>>> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>>>>>> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>>>>>> @@ -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.
>>>> 
>>>> 
>>>> 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.
>>> 
>>>> 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.
>> 
>> 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.
> 
> 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

Re: [PATCH] Bluetooth: hci_bcm: Configure SCO routing automatically

[Rob Herring]

On Mon, Jun 11, 2018 at 12:19 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Rob,
>
>>>>>>> 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.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Attila Tőkés <attitokes@gmail.com>
>>>>>>> ---
>>>>>>> .../bindings/net/broadcom-bluetooth.txt       |  7 ++
>>>>>>
>>>>>> Please split bindings to separate patch.
>>>>>
>>>>>
>>>>> Ok, I will split this in two.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>> 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
>>>>>>> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
>>>>>>> @@ -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.
>>>>>
>>>>>
>>>>> 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.
>>>>
>>>>> 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.
>>>
>>> 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.
>>
>> 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

Re: [PATCH] Bluetooth: hci_bcm: Configure SCO routing automatically

[Attila Tőkés]

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

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

On Mon, Jun 11, 2018 at 10:05 PM, Rob Herring <robh+dt@kernel.org> wrote:

> On Mon, Jun 11, 2018 at 12:19 PM, Marcel Holtmann <marcel@holtmann.org>
> wrote:
> > Hi Rob,
> >
> >>>>>>> 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.
> >>>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> Signed-off-by: Attila Tőkés <attitokes@gmail.com>
> >>>>>>> ---
> >>>>>>> .../bindings/net/broadcom-bluetooth.txt       |  7 ++
> >>>>>>
> >>>>>> Please split bindings to separate patch.
> >>>>>
> >>>>>
> >>>>> Ok, I will split this in two.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>> 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
> >>>>>>> --- a/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> >>>>>>> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.txt
> >>>>>>> @@ -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.
> >>>>>
> >>>>>
> >>>>> 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.
> >>>>
> >>>>> 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.
> >>>
> >>> 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.
> >>
> >> 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
>

[-- Attachment #2: Type: text/html, Size: 8736 bytes --]