login
Search
This patch adds UART support for the ST-Ericsson CG2900 Connectivity Combo controller. This patch registers to the TTY framework as a line discipline driver for the N_HCI ldisc. When opened it registers as a transport to the CG2900 framework. This patch also handles the low power operation (suspend/resume) for the CG2900 when using UART transport. Signed-off-by: Par-Gunnar Hjalmdahl <par-gunnar.p.hjalmdahl@stericsson.com> --- drivers/mfd/Kconfig | 7 + drivers/mfd/cg2900/Makefile | 2 + drivers/mfd/cg2900/cg2900_uart.c | 1851 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 1860 insertions(+), 0 deletions(-) create mode 100644 drivers/mfd/cg2900/cg2900_uart.c diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index a8e790f..6fcd8b6 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -282,6 +282,13 @@ config MFD_STLC2690_CHIP help Support for ST-Ericsson STLC2690 Connectivity Controller +config MFD_CG2900_UART + tristate "Support CG2900 UART transport" + depends on MFD_CG2900 + help + Support for UART as transport for ST-Ericsson CG2900 Connectivity + Controller + config PMIC_DA903X bool "Dialog Semiconductor DA9030/DA9034 PMIC Support" depends on I2C=y diff --git a/drivers/mfd/cg2900/Makefile b/drivers/mfd/cg2900/Makefile index 4935daa..c8dd713 100644 --- a/drivers/mfd/cg2900/Makefile +++ b/drivers/mfd/cg2900/Makefile @@ -10,3 +10,5 @@ obj-$(CONFIG_MFD_CG2900) += cg2900_char_devices.o obj-$(CONFIG_MFD_CG2900_CHIP) += cg2900_chip.o obj-$(CONFIG_MFD_STLC2690_CHIP) += stlc2690_chip.o +obj-$(CONFIG_MFD_CG2900_UART) += cg2900_uart.o + diff --git a/drivers/mfd/cg2900/cg2900_uart.c b/drivers/mfd/cg2900/cg2900_uart.c new file mode 100644 index 0000000..f5e287d --- /dev/null +++ b/drivers/mfd/cg2900/cg2900_uart.c @@ -0,0 +1,1851 @@ +/* + * drivers/mfd/cg2900/cg2900_uart.c + * + * Copyright (C) ST-Ericsson SA 2010 + * Authors: + * Par-Gunnar Hjalmdahl (par-gunnar.p.hjalmdahl@stericsson.com) ...
What if the device doesn't have a tiocmget ? You must check this. If you want to call into the ttys own tiocmget froma sleeping context fine, but see tty_tiocmget and you'll notice you need to check the op is non NULL somewhere. You could do this when the ldisc is opened and refuse to open So instead of a tiny object embedded in your device you've got a whole error path to worry about, a printk disguised in a macro and a text string longer than the struct size ? Surely this should be part of the Please use the standard dev_dbg etc functionality - or pr_err etc when you have no device pointer. The newest kernel tty object has a device This shouldn't be needed - the tty_encode_baud_rate logic works out of You must respect the kref handling in the kernel tty code. Take This needs some restructuring I think A line discipline should contain no hardware awareness, that goes in the actual uart hardware driver. So we shouldn't have magic irq code in this part of things. You also need to sort out the tty kref handling in open/close and the fact you've got magic hardcoded single instance stuff buried i nit. Finally tty ldiscs don't go buried in the mfd directory, or they'll get missed during chanegs/updates. The ldisc probably belongs in bluetooth a and the hardware support in the mfd directory. So - NAK this for the moment, it needs to be split cleanly into ldisc (thing which speaks the protocol and eg sees "speed change required" and acts on it) and device (thing which knows about the hardware). --
Hi and thanks for your comments Alan. See below for answers. /P-G The existence of the callback is checked in the function uart_tty_open. If either break_ctl or tiocmget is NULL we do not allow OK. I like the debug system we have now (using module parameter to set debug level in runtime), but I've received comments regarding this before so I assume we will have to switch to standard printouts. Is it OK to use defines or inline functions to add "CG2900" before and I will have to check with the guy who wrote this code. I think he put The guy responsible for this is thinking through a new design, where I don't quite understand your comment here. This particular code is inspired of the Bluetooth ldisc code and there it is used like. It seems to work fine so we do it the same way. We use a define to get a debug printout when setting state, but I understand it's not so popular so I guess we will have to skip it. I guess it won't help if we would use an inline function instead of a OK. We will try to figure out a new design. I'm not too happy about putting the ldisc part in Bluetooth though since it is only partly Bluetooth, it is also GPS and FM. Better could maybe be under char/? --
The pr_fmt bit will do what you want for a non dev_dbg type thing. See That may go away when you clean up the device side. The line discipline can be attached to any device so must be multi-device aware, the hardware driver can certainly be single device only if you can only ever have one [Although its a good idea to design it so it can be fixed because you never know when you'll find your sales team just sold someone a two You copied a security hole from the bluetooth driver which we found a Check it is valid, in your case probably on open just refuse to attach to Works for me - there is an ongoing "we must move tty ldiscs and core tty code somewhere more sensible of their own" discussion, so if it is dropped into char, it'll move with them just fine. Alan --
Thanks again for your comments Alan. Next patch set will contain resolution to all your comments. See below. /P-G Again, thanks for the comments. /P-G --
After getting a better idea of what the base mfd driver does, my impression
is now that you should not register a second N_HCI line discipline at all,
but instead extend the existing line discipline with this number.
I'm not sure what happens if you need two modules that try to register
the same ldisc number, but I imagine it is not good.
Shouldn't you instead be using the drivers/bluetooth/hci_{ldisc,h4} code?
Arnd
--
We also need the ldisc code to handle events from FM and GPS and since that is chip specific we cannot add that to the generic hci_ldisc code. I agree that we might run into problems if two drivers try to register the same line discipline. It might then be better to introduce a new line discipline then even though that could cause other problems. I do not know if it is possible to add a condition in Kconfig otherwise so the CG2900 ldisc cannot be active while the "normal" ldisc driver is selected. /P-G --
Hi again, I might have been a bit too quick there. The actual channel matching and packet creation is done in hci_h4.c while ldisc registration is done in hci_ldisc.c. So it might to be enough to create a new hci_h4-cg2900.c (or similar name) that can separate the right channels. We must however do changes to hci_ldisc as well since it seems to always register to the Bluetooth stack here, which we definitely don't want since that is handled by btcg2900.c. Also note that this ldisc issue is only valid when using UART as transport. We will also support SPI and then we will probably run into completely new, interesting problems. :-) /P-G --
That sounds good, but would that still be h4? There are currently six UART protocols that have drivers in Linux: H4, bcsp, 3Weire, h4ds, ll and ath3k. Can cg2900 be simply another one of those, or is it different Can you elaborate? You said earlier that cg2900 is a standard HCI with some extensions. If that's the case, why do you need your own Is the link layer on SPI different from the UART variant? Maybe you cna just add a SPI TTY driver if that doesn't exist yet and bind the same ldisc to the SPI device. Arnd --
If your ldisc is written properly it shouldn't matter. Each tty has a Not sure I follow the concern here ? --
It's about the ldisc number. Both bluetooth and cg2900 register themselves to ldisc 15 (N_HCI). A user doing TIOCSETD can only get one of the two. The solution would not be to make the two ldisc implementations mutually exclusive, but to make cg2900 use a new ldisc number, e.g. N_HCI_CG2900. However, I'm still not convinced that this is actually necessary, we might be able to add hooks into the existing N_HCI implementation for the extensions in cg2900. Arnd --
Ah I see - I had assumed any actual final merge would be assigning it a new LDISC code as is required. --
Hi, Sorry for not answering quicker. We in my department have been discussing new design as well as trying out some solutions. As an answer to previous comments, both from Arnd and Alan, we would like to do the following: - Introduce a new ldisc called N_H4_COMBO with a ldisc driver accordingly that will be placed under drivers/char - Keep CG2900 driver as an MFD. We will however register the MFD cells in each driver and remove the function API. The functions (write, reset, etc) will instead be placed in each MFD cell using function pointers. This way we can use different functions for different channels as needed. By placing the MFD cell registration in each chip driver we will also only expose the channels that are actually supported by each chip. This way we can also remove the pretty ugly matching between the devices and respective H4 channel as well as the add_h4_user function and the similar other functions. We were thinking about if we could re-use the existing hci_ldisc driver. As stated before the big problem here is however that hci_ldisc directly registers to the Bluetooth stack. We could separate the data for FM and GPS in the protocol driver, but it is pretty ugly to have two separate data paths within the same driver. As I've state earlier this would also not work with other transports such as SPI.I appreciate Arnd's suggestion to create a TTY for SPI, but I think that would overcomplicate the situation and create a solution more complex than actually needed. We think that creating a new ldisc creates a cleaner solution. It will to some extent remind of hci_ldisc, but it will not register to any framework except tty. We've thought about if we should switch cg2900 to a bus, but after discussing this we feel that CG2900 has such individual characteristics that it could be hard to create a bus that would suite it. We therefore will keep the MFD type. We might in the future add new chips to the driver, but they will most likely not require so much rework ...
That seems sensible. That code should also be entirely hardware Ok. So presumably both the SPI and the tty interfaces would then use the No particular opinion either way. Whatever works best and keeps it modular. --
I'm not convinced, although that's what Alan was suggesting. I'd like to hear from the bluetooth people what they think about this. Could you summarize why you think that cg2900 is different enough from an HCI to require a different line discipline? From your previous explanation it sounded like it was mostly added functionality, One of us must be misreading the code. As far as I can tell, hci_ldisc does not register to the bluetooth stack at all, it just provides the basic multiplex for multiple HCI protocols, while hci_h4.c communicates to the bluetooth stack. If I read it correctly, that means that you can still use hci_ldisc, but need to add another protocol next to hci_h4 and hci_bcsp for How would registering ony to tty solve the problem of SPI? If you just add another hci_cg2900 module, it can register to both Yes, makes sense. Arnd --
If you look in function hci_ldisc.c/hci_uart_register_dev(), it here registers as a driver to the Bluetooth stack. This means that received Bluetooth packets would go ldisc->protocol->ldisc->bluetooth, while FM and GPS would go ldisc->protocol->(FM/GPS)stack. I think it's quite bad to have two different data paths like this. The new ldisc we're creating looks a lot like hci_ldisc, but it does not register itself to an overlaying stack directly. One option would of course be to modify the existing hci_ldisc.c but I feel that be rather dangerous and which could create a lot of problems. /P-G --
After looking again at the code I see that I was wrong. For the receiving path the data will go ldics->protocol->stack. It's actually the TX path (to the chip) that is a bit strange where Bluetooth data is going to stack->ldisc->protocol->ldisc->uart. Here we would have a separate path for FM and GPS. I still don't like the idea of making a tty/ldisc for the SPI transport. I definitely would prefer instead a new ldisc which doesn't register to any stack on top. My preference would be to keep the solutions independent, where we use tty/ldisc for UART and a direct transport protocol driver for SPI (i.e. registering using spi_register_driver). /P-G --
Ok, but is that a problem? You really should not be afraid of touching existing code in order to make it more generic or removing strange code paths like the one you just described. Cleaning up code is usually better than duplicating it when you notice something wrong with the existing implementation. Simply calling the underlying ldisc module from the FM and GPS modules Yes, agreed. You had already convinced me in the previous reply, sorry Yes. However, you can probably share substantial parts of the code between the spi_driver and the hci_uart_proto implementations. You can do this either by making them a single module that registers to both hci_ldisc and spi_bus, or having a common cg2900 base module that does the common parts and add separate modules for spi and hci_uart that register a small wrapper to the respective subsystems. Arnd --
I've tried now to use 3 separate work_structs in the device (one for each work function used), but this doesn't work out. The reason is that the work is generated so often that a work is not finished before next work of same type comes. This is especially true for transmit and receive. Then I get 0 back when queuing the work and there is no real way to solve it from what I can see than to allocate new work structures every time. /P-G --
So if that is the case what bounds your memory usage - can a busy box end up with thousands of work queue slos used ? It sounds like your model is perhaps wrong - if there is a continual stream of work maybe you should simply have a kernel thread to handle it if it cannot be deferred - remember ldisc code is able to sleep in most paths. --
Hi Alan, I went through my old mails here and realized I hadn't answered you for this question. Basically most of the time we are able to handle the works in time for the next work to be created. But there are occasions where next work is created really quickly and we end up with an error situation when starting the work. But if we allocate the work it will then be handled when the first one is ready. It's not like we will never handle it. The data transfers can be received in a bit bursty way and then there are no interrupts for a while. So in the end all works are handled rather quickly and queue will never build up. We have tested with several large file transfers and data pumps without issues. By the way, I will soon release a new patch set for CG2900 (hopefully next week), which contains major rework. It's hard to explain everything here and now but changes include: - Reuse of existing HCI line discipline under /drivers/bluetooth/. Line discipline has been modified so it is selectable from protocol if line discipline should register to Bluetooth stack or not. - Architecture modification: core, chip, and transport is new created from platform specific files (/arch/). Each chip file then spawns MFD devices for the channels it support. - Core file now only handles detection of connected chip. All chip dependent code is moved to transport and chip files. /P-G --
Hi Par, On Fri, Dec 3, 2010 at 10:16 AM, Par-Gunnar Hjalmdahl So is it true that if there's the other chip with similar functionality I have to implement support for, I'll have to pretty much duplicate your line discipline driver? Isn't it better to have a new line discipline that the standard drivers (H4, LL, ...) will be applicable to? ~Vitaly --
Hi Vitaly, What will be in the protocol file, i.e. the same level as hci_h4.c, hci_bcsp.c, etc, will be to ~80% vendor specific. The only H4 channels that are in a standardized specification are the Bluetooth channels 1-4. Other channels such as FM and GPS are vendor specific. I've seen that for example TI has the same channels for FM and GPS, but these channels, 8 and 9, are in no way standardized. The CG2900 also carries a number of other channels, such as debug and device channels, which I'm pretty sure is individual for this chip. This identification of the channels is also only one function, even though it is an important function. There are also a lot of other code regarding baud rate settings, low power handling, etc that I'm positive will not apply to chips of other vendors. It is of course possible that other chips from ST-Ericsson, current and future, will be able to reuse this file, but as I said I doubt that other vendors I'm not certain what you mean. We are modifying the line discipline a bit, but the only thing we have needed to do with existing protocol drivers has been to add a boolean parameter for the protocol settings (so they will register to the Bluetooth stack). I don't see why we should create a new line discipline driver and then move the existing protocol drivers to this. /P-G --
Hi Par, On Mon, Dec 6, 2010 at 10:06 AM, Par-Gunnar Hjalmdahl Wait. What level of abstraction is this file at if even some future STE chips are not guaranteed to work with it? Is it at a level of Okay, let me try to be more specific. There's for instance drivers/bluetooth/hci_ll.c which is the line discipline driver that is meant to work with any BT chip supporting LL protocol. Your solution seems to imply that one will have to create a variation of this implementation for each BT chip supporting LL that will use your shared transport implementation. Is it really the case? Thanks, Vitaly --
Hi Vitaly, There are no GPIOs or similar in the driver code; those are defined and used in the board specific files under the /arch/ files. What could be a problem with a future chip would be if for example the HCI command to change baud rate would change. We have no indication that this will change, but I cannot give a lifetime guarantee that nothing Well, from what I can see LL is an extension of the H4, basically it adds sleep mode handling in a vendor specific way to the normal H4 protocol. So it is also based on the hci_h4 just as our file is. But our file has basically nothing in common with what has been for the LL file. We don't support any of the LL sleep commands for example. So if I would make a driver for a combo chip supporting LL, I would either modify the existing hci_ll.c or I would make a new file based on hci_ll.c. There is not much you could really reuse from our new file. Basically it would be the handling of any common channels, so if you would for example have the same specification of FM and GPS you could maybe save around 20 lines of common code, but you would probably have to add a lot of more code just to keep the solution generic. One major difference is also that hci_ll never changes baud rate or other settings. I assume that is done from hciattach during startup instead. But we cannot run with that since we have to shut down the chip when no user is connected in order to save power. This means that we have to add vendor specific commands in order to for example set baud rate. And then you run into these vendor specific problems. If there would be a standardized specification on how to set baud rate and how to put chip in sleep I assume things could be solved differently, but that is not the case. As a quick answer to your question: that would depend on the difference between the different controllers, I guess. But CG2900 doesn't support the LL protocol so it is not an issue for that. /P-G --
Right, but this gives me the hard time seeing how your implementation is applicable to other multi-functional chips with similar Again, there are at least TI and Broadcom chips that support HCI_LL and if they were to use your implementation of the core, they would have had to add 2 more implementations of the corresponding line Right, but if you are only aiming cg2000, why would you create a framework for that? I initially thought your solution was generic enough to handle other "many-in-one" Bluetooth chips but I'm completely unsure about that now. Thanks, Vitaly --
As far as I understand, the point is that it's no longer a 'solution' at the core, i.e. there is no replacement for hci_ldisc or any of these, just modules for the additional h4 protocols that don't have a linux implementation yet. The patch set that was originally posted here had a new framework, but after the comments from Alan and me, Par-Gunnar agreed to use the existing framework instead and extend it in a useful way. Please read all of the discussion we already had. You made a good point here, but I fear you had both Par-Gunnar and me confused because it was a point that already got resolved. Arnd --
Yeah I've read through that thread but the only conclusion I'd been able to make was that you agreed on not introducing yet another line discipline. I'm not sure if I could make a conclusion that Par gave up on his attempt to make his patchset generic/extensible/reusable for other chips of similar type. But if that's really the case, I see no point in this patchset at all, as opposed to generalizing the TI shared transport thingie and using that one. Thanks, Vitaly --
hci_ll is not the line discipline but the a TI specific uart protocol. The line discipline driver (hci_ldisc.c) is shared across all protocols (h4, ll, ...) and gets extended slightly so it can deal with cg2900 in addition to the existing HCIs. The rest of the cg2900 support is about adding more hci_uart_protos. Arnd --
Hi Arnd, I was referring to hci_ll as to the line discipline driver, not the line discipline itself. The thing is, there are different Bluetooth combo chips which use HCI to encapsulate commands to the sub-chips behind, and there's also the implementation for TI chip doing that which is in the mainline. So instead of keeping reinventing the wheel, I think it's fairly beneficial for everything if we finally agree on something that works for all the combo chips of the type. Thanks, Vitaly --
Yes, that makes sense. I'm probably missing something here, but it seems to me that hci_ll is only about the power management stuff on TI (and broadcom, as you say) chips, and the multi-protocol support is currently handled in hci_ldisc by allowing multiple protocols like h4 and ll to be registered. It that correct? One aspect that Par-Gunnar mentioned was that the multi-protocol stuff for cg2900 and I suspect other similar devices would also need to work with non-UART-based HCIs, which don't use hci_uart_proto but would need something similar. Also, hci_uart is currently not modular, e.g. you cannot build hci_ll as a loadable module as you'd need for dynamic registration. Arnd --
Yeah, it's basic for TI and it's supported by Broadcom although they have their own protocol for power saving. But I was trying to make a different point here. On a basic level, there's this cg2000 chip from STE that does BT, FM and GPS. There's the chip from TI that does BT, FM and GPS, and there's the Broadcom chip that does BT+FM. They all use HCI to access the other functions of the combo chip and they do it in a really simiar way, with the differences mostly in power management techniques. So I think it's quite sensible to have some kind of framework that is suitable for But generally speaking, isn't a line discipline/driver attached to a tty? We can use dumb tty for e. g. SPI and still be able to use hci_ll, right? Thanks, Vitaly --
Yes, I agree 100% in principle. I could not find the code that Broadcom/TI FM and GPS stuff so far, can you point us to that? The cg2900 solution for this was to use MFD (plus another layer in the posted version, but that will go away I assume). Using MFD is not the only possibility here, but I could not see anything wrong with it either. Do you think we can move them all over to I suggested that as well, but the point was made that this would add an unnecessary indirection for the SPI case, which is not really much like a serial port. It's certainly possible to do it like you say, but if we add a way to register the high-level protocols with an HCI-like multi-function device, we could also do it in a way that does not rely on tty-ldisc but keeps it as one of the options. Arnd --
Sure, the TI "shared transport" code is mostly at drivers/misc/ti-st. Some Broadcom BCM43xx chips work in a similar way AFAIK but I'm not I actually don't think it's such a big indirection, I prefer to think of it more as of the abstraction layer. If not use this, are we going to have direct SPI device drivers? I'm afraid we might end up with a huge duplication of code in that case. Thanks, Vitaly --
Ah, it had not occured to me to look in drivers/misc. Looking at the
code over there, it becomes much clearer what your point is. Evidently
the ti-st driver implements a line discipline similar to, but conflicting
with hci_ldisc, and it has its own copy of the hci_ll code.
I guess this is exactly what we're trying to avoid having with the
The question is how it should be modeled in a better way than today.
I believe we already agree it should be something like
bt ti-radio st-e-radio ti-gps st-e-gps broadcom-radio ...
| | | | | | |
+---------+----------+---------+--+----------+----------+---------+
|
common-hci-module
|
+-----------+-----------+
| | | |
serial spi i2c ...
Any objections to this?
If you agree, I think we should discuss the alternatives for the common
module. I think you are saying we should make the common module a single
ldisc doing the multiplexing and have all transports register as ttys into
it, right?
What we discussed before was to split it into multiple modules, one for
the tty ldisc and one for the common code, and then have the spi/i2c/...
drivers feed into the common multiplexer without going through tty.
I don't currently see a significant advantage or disadvantage with either
approach.
Arnd
--
Yes, this sort of architecture would certainly help... However, I suppose the most common question as one of you stated above was how can the channel -ID and other factors such as offset-header packet format be generalized? As in over here, will the common-hci-module work fine, If say ti-radio says he is expecting packets starting from id-8 which is of length 10? and also work for st-e-radio which would say expecting data from id-6 with 15 max bytes? Answering this I suppose would solve a lot of our problems.... Marcel did previously suggest a bus-driver sort of a structure, where the transport are sort of adapter drivers, the data interpretation logic like splitting HCI-events, FM CH-8 packets all fall into the algos drivers and the actual Bluetooth driver or GPS or FM-V4L2 --
Hi Pavan et al, What the cg2900 driver has now turned into is almost like the cg2900_core acting as a bus, identifying which chip is connected and choosing correct chip driver from this. This cg2900_core module should not contain any vendor specific code. When identified, the chip driver then uses MFD to setup channels according to the functionality it supports (one MFD device per H4 channel). It's hard to explain everything of the new architecture here. I would like you to check the new patch set I'm trying hard to create at the moment. I will try to get it out on Friday, but it's hard to promise anything (there is a lot of work to do with it). There you could then see if it is something you could consider useful. --
It sounds really good, I'm looking forward to your new patch set! If it's generic enough to replace the ti-st code, that would be a clear sign that you are on the right track ;-) I'm not asking to fix their code yourself, but it might be good to look at it and see if it would work. Arnd --
