2010-12-03 09:16:39

by Par-Gunnar Hjalmdahl

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

2010/10/29 Alan Cox <[email protected]>:
>> 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.
>
> 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


2010-12-03 11:44:07

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Hi Par,

On Fri, Dec 3, 2010 at 10:16 AM, Par-Gunnar Hjalmdahl
<[email protected]> wrote:
> 2010/10/29 Alan Cox <[email protected]>:

> 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.

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

2010-12-06 09:06:07

by Par-Gunnar Hjalmdahl

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Hi Vitaly,

2010/12/3 Vitaly Wool <[email protected]>:
> Hi Par,
>
> On Fri, Dec 3, 2010 at 10:16 AM, Par-Gunnar Hjalmdahl
> <[email protected]> wrote:
>> 2010/10/29 Alan Cox <[email protected]>:
>
>> 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.
>
> 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?
>

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
might be able to use it.

> Isn't it better to have a new line discipline that the standard
> drivers (H4, LL, ...) will be applicable to?
>
> ~Vitaly
>

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

2010-12-06 09:46:46

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Hi Par,

On Mon, Dec 6, 2010 at 10:06 AM, Par-Gunnar Hjalmdahl
<[email protected]> wrote:

>> 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?
>>
>
> 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
> might be able to use it.

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
hardcoding the GPIO numbers?

>> Isn't it better to have a new line discipline that the standard
>> drivers (H4, LL, ...) will be applicable to?

> 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.

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

2010-12-06 12:01:50

by Par-Gunnar Hjalmdahl

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Hi Vitaly,

2010/12/6 Vitaly Wool <[email protected]>:
> Hi Par,
>
> On Mon, Dec 6, 2010 at 10:06 AM, Par-Gunnar Hjalmdahl
> <[email protected]> wrote:
>
>>> 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?
>>>
>>
>> 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
>> might be able to use it.
>
> 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
> hardcoding the GPIO numbers?
>

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
will ever change with how the chip startup is handled.

>>> Isn't it better to have a new line discipline that the standard
>>> drivers (H4, LL, ...) will be applicable to?
>
>> 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.
>
> 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
>

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

2010-12-06 12:25:46

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Par,

> 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.

Right, but this gives me the hard time seeing how your implementation
is applicable to other multi-functional chips with similar
functionality.

> 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.

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
discipline driver.

> 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.

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

2010-12-06 14:06:15

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Monday 06 December 2010, Vitaly Wool wrote:

> On Mon, Dec 6, 2010 at 10:06 AM, Par-Gunnar Hjalmdahl
> <[email protected]> wrote:

> >> Isn't it better to have a new line discipline that the standard
> >> drivers (H4, LL, ...) will be applicable to?
>
> > 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.
>
> 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?

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

2010-12-06 14:49:13

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Monday 06 December 2010, Vitaly Wool wrote:
> > 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.
>
> 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.

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

2010-12-06 14:54:42

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Hi Arnd,

On Mon, Dec 6, 2010 at 3:06 PM, Arnd Bergmann <[email protected]> wrote:

> 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.

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

2010-12-06 14:57:46

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Mon, Dec 6, 2010 at 3:49 PM, Arnd Bergmann <[email protected]> wrote:

> 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.

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

2010-12-06 15:15:42

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Monday 06 December 2010, Vitaly Wool wrote:
> On Mon, Dec 6, 2010 at 3:06 PM, Arnd Bergmann <[email protected]> wrote:
>
> > 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.
>
> 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.

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

2010-12-06 15:29:00

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Mon, Dec 6, 2010 at 4:15 PM, Arnd Bergmann <[email protected]> wrote:
> 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?

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
such devices.

> 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.

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

2010-12-06 16:54:57

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Monday 06 December 2010, Vitaly Wool wrote:
> On Mon, Dec 6, 2010 at 4:15 PM, Arnd Bergmann <[email protected]> wrote:
> > 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?
>
> 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
> such devices.

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
use MFD infrastructure?

> > 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.
>
> 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?

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

2010-12-06 21:24:36

by Vitaly Wool

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Mon, Dec 6, 2010 at 5:54 PM, Arnd Bergmann <[email protected]> wrote:

>> 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
>> such devices.
>
> 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?

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
sure about the mainline support for those.

> 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
> use MFD infrastructure?

I guess so but it's probably more of a detail than what I'm up to now :)

>> 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?
>
> 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.

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

2010-12-06 23:07:34

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Monday 06 December 2010 22:24:34 Vitaly Wool wrote:
> On Mon, Dec 6, 2010 at 5:54 PM, Arnd Bergmann <[email protected]> wrote:
>
> >> 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
> >> such devices.
> >
> > 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?
>
> 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
> sure about the mainline support for those.

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
cg2900 code ;-).

> > 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
> > use MFD infrastructure?
>
> I guess so but it's probably more of a detail than what I'm up to now :)

Agreed.

> >> 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?
> >
> > 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.
>
> 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.

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

2010-12-08 07:41:39

by Pavan Savoy

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Tue, Dec 7, 2010 at 4:37 AM, Arnd Bergmann <[email protected]> wrote:
> On Monday 06 December 2010 22:24:34 Vitaly Wool wrote:
>> On Mon, Dec 6, 2010 at 5:54 PM, Arnd Bergmann <[email protected]> wrote:
>>
>> >> 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
>> >> such devices.
>> >
>> > 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?
>>
>> 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
>> sure about the mainline support for those.
>
> 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
> cg2900 code ;-).
>
>> > 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
>> > use MFD infrastructure?
>>
>> I guess so but it's probably more of a detail than what I'm up to now :)
>
> Agreed.
>
>> >> 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?
>> >
>> > 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.
>>
>> 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.
>
> 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    ...

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
drivers falling into the client/protocol drivers....



> 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

2010-12-08 12:22:03

by Par-Gunnar Hjalmdahl

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

Hi Pavan et al,

2010/12/8 Pavan Savoy <[email protected]>:
> On Tue, Dec 7, 2010 at 4:37 AM, Arnd Bergmann <[email protected]> wrote:
>> On Monday 06 December 2010 22:24:34 Vitaly Wool wrote:
>>> On Mon, Dec 6, 2010 at 5:54 PM, Arnd Bergmann <[email protected]> wrote:
>>>
>>> >> 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
>>> >> such devices.
>>> >
>>> > 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?
>>>
>>> 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
>>> sure about the mainline support for those.
>>
>> 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
>> cg2900 code ;-).
>>
>>> > 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
>>> > use MFD infrastructure?
>>>
>>> I guess so but it's probably more of a detail than what I'm up to now :)
>>
>> Agreed.
>>
>>> >> 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?
>>> >
>>> > 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.
>>>
>>> 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.
>>
>> 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 ? ?...
>
> 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
> drivers falling into the client/protocol drivers....
>

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.

/P-G

>
>
>> 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
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>>
>

2010-12-08 12:51:51

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.

On Wednesday 08 December 2010, Par-Gunnar Hjalmdahl wrote:
> > 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
> > drivers falling into the client/protocol drivers....
>
> 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