Re: [PATCH] Bluetooth: hci_bcm: Configure SCO routing automatically
From: Rob Herring <robh+dt@kernel.org>
Date: 2018-06-11 15:34:36
On Sat, Jun 9, 2018 at 12:26 AM, Attila Tőkés [off-list ref] wrote:
Hello Rob, On Fri, Jun 8, 2018 at 8:25 PM, Rob Herring [off-list ref] wrote:quoted
On Fri, Jun 8, 2018 at 10:20 AM, [off-list ref] wrote:quoted
From: Attila Tőkés <redacted> 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
quoted
Signed-off-by: Attila Tőkés <redacted> --- .../bindings/net/broadcom-bluetooth.txt | 7 ++Please split bindings to separate patch.Ok, I will split this in two.quoted
quoted
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.
quoted
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).
quoted
quoted
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 systemsleep * @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