Hi Rob. -----Original Message----- From: linux-bluetooth-owner@xxxxxxxxxxxxxxx [mailto:linux-bluetooth-owner@xxxxxxxxxxxxxxx] On Behalf Of Rob Herring Sent: Thursday, June 18, 2015 3:41 PM To: Ilya Faenson Cc: marcel@xxxxxxxxxxxx; Arend Van Spriel; devicetree@xxxxxxxxxxxxxxx; linux-bluetooth@xxxxxxxxxxxxxxx Subject: Re: FW: [PATCH v4 1/4] Broadcom Bluetooth UART Device Tree bindings On Thu, Jun 18, 2015 at 1:54 PM, Ilya Faenson <ifaenson@xxxxxxxxxxxx> wrote: > Hi Rob, Your emails are base64 encoded. They should be plain text for the list. IF: The Outlook is configured to respond in the sender's format. I therefore respond in the encoding you've used. > -----Original Message----- > From: Rob Herring [mailto:robherring2@xxxxxxxxx] > Sent: Thursday, June 18, 2015 11:03 AM > To: Ilya Faenson > Cc: marcel@xxxxxxxxxxxx; Arend Van Spriel; devicetree@xxxxxxxxxxxxxxx; linux-bluetooth@xxxxxxxxxxxxxxx > Subject: Re: FW: [PATCH v4 1/4] Broadcom Bluetooth UART Device Tree bindings > > On Wed, Jun 17, 2015 at 6:11 PM, Ilya Faenson <ifaenson@xxxxxxxxxxxx> wrote: >> + devicetree lists [...] >> diff --git a/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt b/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt >> new file mode 100644 >> index 0000000..5dbcd57 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt >> @@ -0,0 +1,86 @@ >> +btbcm >> +------ >> + >> +Required properties: >> + >> + - compatible : must be "brcm,brcm-bt-uart". > > You need to describe the chip, not the interface. > > IF: This driver is for all Broadcom Bluetooth UART based chips. BT only chips? Most/many Broadcom chips are combo chips. I understand the driver for BT is *mostly* separate from other chip functions like WiFi, GPS and NFC, but the h/w is a single chip. I say most because likely there are some parts shared: a voltage rail, reset line, or power down line. I think some can do BT over the SDIO interface too, so the interface may be shared. The DT is a description of the h/w (i.e. what part # for a chip) and not a driver data structure. You need to describe the whole chip in the binding. It is a Linux problem if there needs to be multiple separate drivers. IF: Defining complete DT description for the Broadcom combo chips for multiple interfaces is well beyond the scope of what I am doing. I just need to define a DT node for the input and output GPIOs connected to the BT UART chip. BT may or may not be part of the combo chip: it does not really matter for this exercise. I thought I would take this opportunity to place some BT device parameters into that node as well. If you're not comfortable with this, I could just call it a "GPIO set" to avoid mentioning BT and UART at all but it would make little sense. Still, I could consider it. Would you have "GPIO set" recommendations? Alternatively, NFC Marvell code you're referring to has parameters configured as Linux module parameters. I could do the same too, avoiding this device tree discussion. Let me know. Generally speaking (pontification hat on :-)), what you're trying to do (description of the whole chip) is well beyond what say ACPI has done (I was involved some in the BT ACPI exercise a few years ago). BT UART interface is described in ACPI independently of other parts of the same combo chip. Requirements like that slow down the DT development in my opinion as companies are understandably reluctant to work with unrealistic goals. End of pontification. :-) > >> + - tty : tty device connected to this Bluetooth device. > > "tty" is a bit of a Linuxism and "ttyS0" certainly is. Further, there > is no guarantee which uart is assigned ttyS0. > > This should be a phandle to the connected uart if not a sub node of > the uart. This is complicated since these chips have multiple host > connections. It needs to go under either uart or SDIO host and have a > reference back to the one it is not under. Given the SDIO interface is > discoverable (although sideband gpios and regulators are not), I would > put this under the uart node as that is never discoverable. > > As I've mentioned previously, there's several cases of bindings for > UART slave devices being posted. This all needs to be coordinated to > use a common structure. > > IF: This driver does not really access the UART. If just needs to have a string of some sort to map instances of the BlueZ protocol into platform devices to employ the right GPIOs and interrupts. I could use anything you recommend available through the tty_struct coming to the protocol from the BlueZ line discipline. Moreover, every end user platform I've ever dealt with in 16 years of working with Bluetooth had a single BT UART device. So these are really rare (typically test platforms) cases only. The mapping is not needed for most platforms at all. I suspect the right thing to do would be to make this parameter optional. The mapping would be done only if the parameter is present. I will use anything tty_struct derived you specify. Makes sense? You've missed my point. I'm not talking about connecting multiple devices to a UART at once. There are several instances of people trying to add UART connected devices into DT[1][2]. My point is these devices all need to have the DT binding done in a common way across different platforms. Otherwise, we can not have common code to parse the DT and find devices attached to a UART. IF: Chances are I was not clear enough. I was not talking about connecting multiple devices to a UART. I was talking about connecting one Broadcom BT device to one serial port and another Broadcom BT device to another serial port (rare enough setup). I do understand your goals though. I would be happy to participate in that exercise (subject to the management approval) once DT has published the UART device parameters and the Linux bluetooth-next has support for DT enumerated devices. I don’t see it happening soon though. Marvell example you've referred me to has nothing of the sort. What do you think of allowing us something to ship now with an understanding that we would support your UART enumerated devices once they are published? > >> + >> +Optional properties: >> + >> + - bt-host-wake-gpios : bt-host-wake input GPIO to be used as an interrupt. >> + >> + - bt-wake-gpios : bt-wake output GPIO to be used to suspend / resume device. >> + >> + - bt-reg-on-gpios : reg-on output GPIO to be used to power device on/off. >> + >> + - oper-speed : Bluetooth device operational baud rate. >> + Default: 3000000. >> + >> + - manual-fc : flow control UART in suspend / resume scenarios. >> + Default: 0. > > Can be boolean? > > I don't really follow the description. > > IF: Okay, I will make it boolean. To clarify the description, it controls whether the BlueZ protocol needs to flow control the UART when the BT device is suspended and un-flow control it when the device is resumed. Okay. Discussion of BlueZ is not relevant to bindings. I would say something like "The hardware requires the host UART flow-control to be de-asserted during suspend." IF: Okay. [...] >> + - configure-audio : configure platform PCM SCO flag. >> + Default: false. > > So ignore the rest of the settings if not set? Perhaps combine with pcm-routing: > > <blank> - no audio > audio-mode = "pcm"; > audio-mode = "hci"; (or "sco-hci"?) > > IF: That's right: the rest of the parameters are not needed if configure-audio is false. Talking about your suggestions, this driver does nothing if the audio is either sent inbound or not used at all. Would you agree to something like the configure-pcm-audio flag? So why not combine with pcm-routing? IF: Okay, I can derive one from another. [...] > Use the actual rate rather than an enumeration. It is a simple div by > 128k and log2 to convert in the driver. This makes the property more > compatible with other chips. > > IF: These rates are subject to change in future chips with no guarantees of the pattern holding. I would prefer to use the actual value expected by the firmware if you don't mind to avoid maintaining the extra driver code. Exactly my point. If the next chip has 0-64k, 1-256k, 2-2048k, your binding is broken. Just put the bit rate in the binding and do the mapping to register value in the driver. IF: Okay, will implement. > What does incall mean? What is the bit rate when not in a call? > > IF: That's the name given to me by the hardware guys. What do you think about the "pcm-interface-rate" instead? That's somewhat ambiguous. Is that the clock, sample rate, or bit rate (sample rate * sample size * channels). IF: Okay, will use the original name. Rob [1] https://lwn.net/Articles/643878/ [2] http://www.spinics.net/lists/devicetree/msg83596.html -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
