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

["Attila Tőkés" <attitokes@gmail.com>]

[-- 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 --]