On Sat, Jun 9, 2018 at 12:26 AM, Attila Tőkés <attitokes@xxxxxxxxx> wrote: > Hello Rob, > > On Fri, Jun 8, 2018 at 8:25 PM, Rob Herring <robh+dt@xxxxxxxxxx> wrote: >> >> On Fri, Jun 8, 2018 at 10:20 AM, <attitokes@xxxxxxxxx> wrote: >> > From: Attila Tőkés <attitokes@xxxxxxxxx> >> > >> > 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@xxxxxxxxx> >> > --- >> > .../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), ¶ms, >> > 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
