From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: References: <280FCB2C-6DF1-4790-A89F-AF5BE3513AE5@holtmann.org> <20180608162009.22762-1-attitokes@gmail.com> <4F0D6729-AE77-47D4-9765-FBC44181D4DE@holtmann.org> From: =?UTF-8?B?QXR0aWxhIFTFkWvDqXM=?= Date: Tue, 12 Jun 2018 20:28:19 +0300 Message-ID: Subject: Re: [PATCH] Bluetooth: hci_bcm: Configure SCO routing automatically Content-Type: multipart/alternative; boundary="000000000000411ed9056e7531eb" To: Rob Herring Cc: Marcel Holtmann , "David S. Miller" , Mark Rutland , Johan Hedberg , Artiom Vaskov , netdev , devicetree , "linux-kernel@vger.kernel.org" , "open list:BLUETOOTH DRIVERS" List-ID: --000000000000411ed9056e7531eb Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 wrote: > On Mon, Jun 11, 2018 at 12:19 PM, Marcel Holtmann > wrote: > > Hi Rob, > > > >>>>>>> Added support to automatically configure the SCO packet routing a= t > 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 configur= e > 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=C5=91k=C3=A9s > >>>>>>> --- > >>>>>>> .../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.tx= t > >>>>>>> +++ b/Documentation/devicetree/bindings/net/broadcom-bluetooth.tx= t > >>>>>>> @@ -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 h= as > >>>> 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 > --000000000000411ed9056e7531eb Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Marcel, Rob,

I was thinking about do= ing these changes in two parts:

1. Making=C2=A0i= n hci_bcm.c=C2=A0the=C2=A0SCO routed over HCI by default
As=C2=A0in 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 backw= ard compatible.

2. Adding support to co= nfigure SCO routed over the other transports (PCM / I2C / codec),=C2=A0ba= sed on the DT

I don't think we have c= lear solution yet here.=C2=A0The implementation could be vendor spec= ific or a more=C2=A0generic one. And there is the modes / timing negotiat= ion 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, Marce= l Holtmann <marcel@holtmann.org> wrote:
> Hi Rob,
>
>>>>>>> Added support to automatically configure the S= CO packet routing at the
>>>>>>> device setup. The SCO packets are used with th= e 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 loa= ded from the device tree.
>>>>>>
>>>>>> Please wrap your commit msg.
>>>>>
>>>>>
>>>>> Sure.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Attila T=C5=91k=C3=A9s <attitokes@gmail.com>
>>>>>>> ---
>>>>>>> .../bindings/net/broadcom-bluetooth.txt= =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 7 ++
>>>>>>
>>>>>> Please split bindings to separate patch.
>>>>>
>>>>>
>>>>> Ok, I will split this in two.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>> drivers/bluetooth/hci_bcm.c=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| 72 ++++++++++++++++++= +
>>>>>>> 2 files changed, 79 insertions(+)
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/Documentation/devicetree/bindings/net/b= roadcom-bluetooth.txt
>>>>>>> b/Documentation/devicetree/bindings/net/b= roadcom-bluetooth.txt
>>>>>>> index 4194ff7e..aea3a094 100644
>>>>>>> --- a/Documentation/devicetree/bindings/n= et/broadcom-bluetooth.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/n= et/broadcom-bluetooth.txt
>>>>>>> @@ -21,6 +21,12 @@ Optional properties:
>>>>>>> - clocks: clock specifier if external clock pr= ovided to the controller
>>>>>>> - clock-names: should be "extclk" >>>>>>>
>>>>>>> + SCO routing parameters:
>>>>>>> + - sco-routing: 0-3 (PCM, Transport, Codec, I= 2S)
>>>>>>> + - pcm-interface-rate: 0-4 (128 Kbps - 2048 K= bps)
>>>>>>> + - 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 eithe= r vendor prefix or
>>>>>> to be documented in a common location. I think the= se look like the
>>>>>> latter.
>>>>>
>>>>>
>>>>> These will be used as parameters of a vendor specific = (Broadcom/Cypress)
>>>>> command configuring the SCO packet routing. See the Wr= ite_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 c= ertainly
>>>> vendor/chip specific, but that is all irrelevant to the bi= nding.
>>>>
>>>>> What would be the property names with a Broadcom / Cyp= ress vendor prefix?
>>>>>
>>>>>=C2=A0 =C2=A0brcm,sco-routing
>>>>>=C2=A0 =C2=A0brcm,pcm-interface-rate
>>>>>=C2=A0 =C2=A0brcm,pcm-frame-type
>>>>>=C2=A0 =C2=A0brcm,pcm-sync-mode
>>>>>=C2=A0 =C2=A0brcm,pcm-clock-mode
>>>>>
>>>>> ?
>>>>
>>>> Yes.
>>>
>>> we can do this. However all pcm-* are optional if you switch t= he HCI transport. And sco-routing should default to HCI if that is not pres= ent. 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 vendo= rs 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 of= ten
>> 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 thin= k 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.<= br>
We already have properties doing the same thing defined in
Documentation/devicetree/bindings/sound/simple-card.txt. Use and<= br> 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

--000000000000411ed9056e7531eb--