From: Huang Shijie <[email protected]>
In the uart_handle_cts_change(), uart_write_wakeup() is called after
we call @uart_port->ops->start_tx().
The Documentation/serial/driver tells us:
-----------------------------------------------
start_tx(port)
Start transmitting characters.
Locking: port->lock taken.
Interrupts: locally disabled.
-----------------------------------------------
So when the uart_write_wakeup() is called, the port->lock is taken by
the upper. See the following callstack:
|_ uart_write_wakeup
|_ tty_wakeup
|_ ld->ops->write_wakeup
With the port->lock held, we call the @write_wakeup. Some implemetation of
the @write_wakeup does not notice that the port->lock is held, and it still
tries to send data with uart_write() which will try to grab the prot->lock.
A dead lock occurs, see the following log caught in the Bluetooth by uart:
--------------------------------------------------------------------
BUG: spinlock lockup suspected on CPU#0, swapper/0/0
lock: 0xdc3f4410, .magic: dead4ead, .owner: swapper/0/0, .owner_cpu: 0
CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 3.10.17-16839-ge4a1bef #1320
[<80014cbc>] (unwind_backtrace+0x0/0x138) from [<8001251c>] (show_stack+0x10/0x14)
[<8001251c>] (show_stack+0x10/0x14) from [<802816ac>] (do_raw_spin_lock+0x108/0x184)
[<802816ac>] (do_raw_spin_lock+0x108/0x184) from [<806a22b0>] (_raw_spin_lock_irqsave+0x54/0x60)
[<806a22b0>] (_raw_spin_lock_irqsave+0x54/0x60) from [<802f5754>] (uart_write+0x38/0xe0)
[<802f5754>] (uart_write+0x38/0xe0) from [<80455270>] (hci_uart_tx_wakeup+0xa4/0x168)
[<80455270>] (hci_uart_tx_wakeup+0xa4/0x168) from [<802dab18>] (tty_wakeup+0x50/0x5c)
[<802dab18>] (tty_wakeup+0x50/0x5c) from [<802f81a4>] (imx_rtsint+0x50/0x80)
[<802f81a4>] (imx_rtsint+0x50/0x80) from [<802f88f4>] (imx_int+0x158/0x17c)
[<802f88f4>] (imx_int+0x158/0x17c) from [<8007abe0>] (handle_irq_event_percpu+0x50/0x194)
[<8007abe0>] (handle_irq_event_percpu+0x50/0x194) from [<8007ad60>] (handle_irq_event+0x3c/0x5c)
--------------------------------------------------------------------
This patch adds more limits to the @write_wakeup, the one who wants to
implemet the @write_wakeup should follow the limits which avoid the deadlock.
Signed-off-by: Huang Shijie <[email protected]>
Signed-off-by: Felipe Balbi <[email protected]>
---
include/linux/tty_ldisc.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index add26da..00c9d68 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -92,7 +92,10 @@
* This function is called by the low-level tty driver to signal
* that line discpline should try to send more characters to the
* low-level driver for transmission. If the line discpline does
- * not have any more data to send, it can just return.
+ * not have any more data to send, it can just return. If the line
+ * discipline does have some data to send, please arise a tasklet
+ * or workqueue to do the real data transfer. Do not send data in
+ * this hook, it may leads to a deadlock.
*
* int (*hangup)(struct tty_struct *)
*
--
1.9.2.459.g68773ac
> I had a quick look and I guess that uart_change_pm() is the most likely
> candidate for what you are referring to.
> I can see how that interfaces to the specific piece of uart hardware, such as
> omap-serial.c
> But I cannot see how you would plumb that though to the device that was
> plugged in to the serial port. I guess if that device could be registered as
> a child of the omap_serial device, then power management inheritance might
> come in to play, but I cannot see any way to tell a serial port that it has
> some arbitrary child device.
If your device is powered by something like a regulator then you can
drive the regulator from the uart_pm helpers.
> In one case the "power on" sequence is substantially more complex that just a
> gpio or regulator. I would need to write a device driver for the (GPS) chip
> which could receive a poweron/poweroff signal from the uart and do the
> required magic.
In which case giving the tty a child device (or devices) would probably
be more sensible yes. No problem with that either.
> I would really like to see the "runtime interpreted power sequences" become a
> real thing. Then serial-core could activate a "RIPS", and that would have
> the flexibility to do whatever is needed without adding complexity to
> serial-core.
Something like ACPI has you mean ? When we put the device into D0 the
ACPI methods can do stuff.
> So ... with your "serial" hat on, if I were to write/test a patch to allow
> any serial port to have a "power-gpio" described in DT and the gpio would be
> driven in uart_change_pm(), would you consider accepting that patch, or did I
> misunderstand?
We are going to need it anyway for other devices fairly soon. It's common
for new PC class hardware to have gpio management for the bluetooth,
gps and and sometimes sensor devices attached to the tty. That's true
irrespective of whether you happen to choose to use it for virtual gpio
hacks.
Alan
> As soon as this patch
> (http://www.spinics.net/lists/arm-kernel/msg325197.html) will be
> applied, we don't really need this DTR GPIO any more.
For the omap specific case, not for the general case of sorting out power
hierarchies.
Alan
On Fri, Apr 25, 2014 at 11:34 AM, NeilBrown <[email protected]> wrote:
> On Thu, 24 Apr 2014 15:19:14 +0100 One Thousand Gnomes
> <[email protected]> wrote:
>
>> > > But I don't have discrete hardware. I have a bunch of stuff soldered onto a
>> > > board with ad-hoc connections chosen to make the life of the hardware builder
>> > > easy rather than chosen to make the life of the software developer easy
>> > > (which I think is the correct choice).
>> > >
>> > > So I need to tell DT "This device is plugged into this UART, and there is no
>> > > DTR line, but when the UARTs DTR line would be asserted (if it had one), then
>> > > I need that regulator of there turned on". or maybe "I need to toggle this
>> > > GPIO with exactly this pattern, while watching that GPIO to see if it is
>> > > working".
>> > >
>> > > So I thought:
>> > >
>> > > 1/ give the UART a "virtual" DTR so it could drive any GPIO
>> > > 2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio
>> > > and responded to state changes on that GPIO by turning on or off the
>> > > regulator
>> > > 3/ create a dedicated driver which knows how to toggle the magic GPIO while
>> > > watching the other GPIO to convince the silly device to wakeup, or go to
>> > > sleep, as required, and have this appear as a (virtual) GPIO.
>>
>> Unless you are using it as a "real' DTR line then I think this is the
>> wrong approach. This problem actually is turning up in both PC class and
>> ARM boxes now all over the place for three reasons I am seeing.
>>
>> 1. We are getting a lot of hardware moving to serial attached
>> bluetooth/gps/etc because of the power win. In addition ACPI can describe
>> power relationships and what is on the other end of a UART embedded in
>> the device
>>
>> 2. We've got cheap hardware with modem lines being "retrofitted" via gpio
>>
>> 3. There are a subset of devices that have extra control lines beyond the
>> usual serial port ones. These range from additional control lines for
>> things like smartcard programmers to port muxing.
>>
>> > I have no problem either way, just that unused code doesn't have to be
>> > sitting in the tree and I'm not entirely sure this GPIO should be
>> > handled by omap-serial.c, perhaps something more generic inside
>> > serial-core so other UART drivers can benefit from it.
>>
>> serial-core provides power hooks. If the goal is that this comes on when
>> you power up the uart and goes away on the last close then the hooks are
>> there already.
>
> Could you be a bit more explicit, or point to an example user of these hooks?
>
> I had a quick look and I guess that uart_change_pm() is the most likely
> candidate for what you are referring to.
> I can see how that interfaces to the specific piece of uart hardware, such as
> omap-serial.c
> But I cannot see how you would plumb that though to the device that was
> plugged in to the serial port. I guess if that device could be registered as
> a child of the omap_serial device, then power management inheritance might
> come in to play, but I cannot see any way to tell a serial port that it has
> some arbitrary child device.
>
> So maybe I'm missing something.
>
>> If its ldisc specific then the tty layer also calls the
>> device on ldisc changes precisely so it can make hardware specific
>> twiddles in such cases.
>>
>> A set of gpios on the tty_port object would not go amiss and would also
>> address the PC case.
>>
>> > considering this is a BTUART device, why didn't you do this at the ldisc
>> > level ? hci_uart_open() sounds like a good choice from a quick thinking.
>>
>> ldiscs are hardware independent. Nothing about hardware belongs in an
>> ldisc. Any ldisc should within reason work on any port.
>>
>> What I propsed when it came up ages ago was to expose some gpio settings
>> in the tty, to provide ways for them to be set by default and also ioctls
>> to configure them. I still think this (but moved into the tty_port as its
>> a persistent hardware property) is a good idea now that we are starting
>> to see more use cases than weird dongles and muxes on pre-production
>> reference boards.
>>
>> With my tty and serial hat on I think a power gpio is a no-brainer even
>> without doing the other work and therefore it should probably be described
>> generically for a serial port in the DT as well as in the ACPI data. If
>> you should also be able to give it a regulator to use as well that also
>> seems to make complete sense.
>
> In one case the "power on" sequence is substantially more complex that just a
> gpio or regulator. I would need to write a device driver for the (GPS) chip
> which could receive a poweron/poweroff signal from the uart and do the
> required magic.
>
> Having serial-core know about gpios and regulators and random other stuff
> seems wrong.
> I would really like to see the "runtime interpreted power sequences" become a
> real thing. Then serial-core could activate a "RIPS", and that would have
> the flexibility to do whatever is needed without adding complexity to
> serial-core.
> Using a virtual GPIO was my poor-mans RIPS. Using gpiolib, and driver can
> pretend to be a gpio so it is a simple "pipe" to send a power-on/power-off
> signal over.
>
> So ... with your "serial" hat on, if I were to write/test a patch to allow
> any serial port to have a "power-gpio" described in DT and the gpio would be
> driven in uart_change_pm(), would you consider accepting that patch, or did I
> misunderstand?
As soon as this patch
(http://www.spinics.net/lists/arm-kernel/msg325197.html) will be
applied, we don't really need this DTR GPIO any more.
DTR_active is the only stuff, that is missing.
Yegor
On Thu, 24 Apr 2014 15:19:14 +0100 One Thousand Gnomes
<[email protected]> wrote:
> > > But I don't have discrete hardware. I have a bunch of stuff soldered onto a
> > > board with ad-hoc connections chosen to make the life of the hardware builder
> > > easy rather than chosen to make the life of the software developer easy
> > > (which I think is the correct choice).
> > >
> > > So I need to tell DT "This device is plugged into this UART, and there is no
> > > DTR line, but when the UARTs DTR line would be asserted (if it had one), then
> > > I need that regulator of there turned on". or maybe "I need to toggle this
> > > GPIO with exactly this pattern, while watching that GPIO to see if it is
> > > working".
> > >
> > > So I thought:
> > >
> > > 1/ give the UART a "virtual" DTR so it could drive any GPIO
> > > 2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio
> > > and responded to state changes on that GPIO by turning on or off the
> > > regulator
> > > 3/ create a dedicated driver which knows how to toggle the magic GPIO while
> > > watching the other GPIO to convince the silly device to wakeup, or go to
> > > sleep, as required, and have this appear as a (virtual) GPIO.
>
> Unless you are using it as a "real' DTR line then I think this is the
> wrong approach. This problem actually is turning up in both PC class and
> ARM boxes now all over the place for three reasons I am seeing.
>
> 1. We are getting a lot of hardware moving to serial attached
> bluetooth/gps/etc because of the power win. In addition ACPI can describe
> power relationships and what is on the other end of a UART embedded in
> the device
>
> 2. We've got cheap hardware with modem lines being "retrofitted" via gpio
>
> 3. There are a subset of devices that have extra control lines beyond the
> usual serial port ones. These range from additional control lines for
> things like smartcard programmers to port muxing.
>
> > I have no problem either way, just that unused code doesn't have to be
> > sitting in the tree and I'm not entirely sure this GPIO should be
> > handled by omap-serial.c, perhaps something more generic inside
> > serial-core so other UART drivers can benefit from it.
>
> serial-core provides power hooks. If the goal is that this comes on when
> you power up the uart and goes away on the last close then the hooks are
> there already.
Could you be a bit more explicit, or point to an example user of these hooks?
I had a quick look and I guess that uart_change_pm() is the most likely
candidate for what you are referring to.
I can see how that interfaces to the specific piece of uart hardware, such as
omap-serial.c
But I cannot see how you would plumb that though to the device that was
plugged in to the serial port. I guess if that device could be registered as
a child of the omap_serial device, then power management inheritance might
come in to play, but I cannot see any way to tell a serial port that it has
some arbitrary child device.
So maybe I'm missing something.
> If its ldisc specific then the tty layer also calls the
> device on ldisc changes precisely so it can make hardware specific
> twiddles in such cases.
>
> A set of gpios on the tty_port object would not go amiss and would also
> address the PC case.
>
> > considering this is a BTUART device, why didn't you do this at the ldisc
> > level ? hci_uart_open() sounds like a good choice from a quick thinking.
>
> ldiscs are hardware independent. Nothing about hardware belongs in an
> ldisc. Any ldisc should within reason work on any port.
>
> What I propsed when it came up ages ago was to expose some gpio settings
> in the tty, to provide ways for them to be set by default and also ioctls
> to configure them. I still think this (but moved into the tty_port as its
> a persistent hardware property) is a good idea now that we are starting
> to see more use cases than weird dongles and muxes on pre-production
> reference boards.
>
> With my tty and serial hat on I think a power gpio is a no-brainer even
> without doing the other work and therefore it should probably be described
> generically for a serial port in the DT as well as in the ACPI data. If
> you should also be able to give it a regulator to use as well that also
> seems to make complete sense.
In one case the "power on" sequence is substantially more complex that just a
gpio or regulator. I would need to write a device driver for the (GPS) chip
which could receive a poweron/poweroff signal from the uart and do the
required magic.
Having serial-core know about gpios and regulators and random other stuff
seems wrong.
I would really like to see the "runtime interpreted power sequences" become a
real thing. Then serial-core could activate a "RIPS", and that would have
the flexibility to do whatever is needed without adding complexity to
serial-core.
Using a virtual GPIO was my poor-mans RIPS. Using gpiolib, and driver can
pretend to be a gpio so it is a simple "pipe" to send a power-on/power-off
signal over.
So ... with your "serial" hat on, if I were to write/test a patch to allow
any serial port to have a "power-gpio" described in DT and the gpio would be
driven in uart_change_pm(), would you consider accepting that patch, or did I
misunderstand?
Thanks,
NeilBrown
> > But I don't have discrete hardware. I have a bunch of stuff soldered onto a
> > board with ad-hoc connections chosen to make the life of the hardware builder
> > easy rather than chosen to make the life of the software developer easy
> > (which I think is the correct choice).
> >
> > So I need to tell DT "This device is plugged into this UART, and there is no
> > DTR line, but when the UARTs DTR line would be asserted (if it had one), then
> > I need that regulator of there turned on". or maybe "I need to toggle this
> > GPIO with exactly this pattern, while watching that GPIO to see if it is
> > working".
> >
> > So I thought:
> >
> > 1/ give the UART a "virtual" DTR so it could drive any GPIO
> > 2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio
> > and responded to state changes on that GPIO by turning on or off the
> > regulator
> > 3/ create a dedicated driver which knows how to toggle the magic GPIO while
> > watching the other GPIO to convince the silly device to wakeup, or go to
> > sleep, as required, and have this appear as a (virtual) GPIO.
Unless you are using it as a "real' DTR line then I think this is the
wrong approach. This problem actually is turning up in both PC class and
ARM boxes now all over the place for three reasons I am seeing.
1. We are getting a lot of hardware moving to serial attached
bluetooth/gps/etc because of the power win. In addition ACPI can describe
power relationships and what is on the other end of a UART embedded in
the device
2. We've got cheap hardware with modem lines being "retrofitted" via gpio
3. There are a subset of devices that have extra control lines beyond the
usual serial port ones. These range from additional control lines for
things like smartcard programmers to port muxing.
> I have no problem either way, just that unused code doesn't have to be
> sitting in the tree and I'm not entirely sure this GPIO should be
> handled by omap-serial.c, perhaps something more generic inside
> serial-core so other UART drivers can benefit from it.
serial-core provides power hooks. If the goal is that this comes on when
you power up the uart and goes away on the last close then the hooks are
there already. If its ldisc specific then the tty layer also calls the
device on ldisc changes precisely so it can make hardware specific
twiddles in such cases.
A set of gpios on the tty_port object would not go amiss and would also
address the PC case.
> considering this is a BTUART device, why didn't you do this at the ldisc
> level ? hci_uart_open() sounds like a good choice from a quick thinking.
ldiscs are hardware independent. Nothing about hardware belongs in an
ldisc. Any ldisc should within reason work on any port.
What I propsed when it came up ages ago was to expose some gpio settings
in the tty, to provide ways for them to be set by default and also ioctls
to configure them. I still think this (but moved into the tty_port as its
a persistent hardware property) is a good idea now that we are starting
to see more use cases than weird dongles and muxes on pre-production
reference boards.
With my tty and serial hat on I think a power gpio is a no-brainer even
without doing the other work and therefore it should probably be described
generically for a serial port in the DT as well as in the ACPI data. If
you should also be able to give it a regulator to use as well that also
seems to make complete sense.
Alan
Dear Felipe Balbi,
On Wed, Apr 23, 2014 at 09:58:26AM -0500, Felipe Balbi wrote:
> LDISCs shouldn't call tty->ops->write() from within
> ->write_wakeup().
>
> ->write_wakeup() is called with port lock taken and
> IRQs disabled, tty->ops->write() will try to acquire
> the same port lock and we will deadlock.
>
> Acked-by: Marcel Holtmann <[email protected]>
> Reviewed-by: Peter Hurley <[email protected]>
> Reported-by: Huang Shijie <[email protected]>
> Signed-off-by: Felipe Balbi <[email protected]>
Tested-by: Andreas Bie?mann <[email protected]>
on custom TI AM37xx board with 3.4.87. Therefore I vote for adding this to
stable branches (at least 3.4). It applies with a slight line shifting
(init_work is not available there).
I wonder if you have seen my approach [1] to fix this deadlock. This stuff is
quiet new to me. As I understood the work_queue has a bit more overhead than a
tasklet. Therefore I implemented my fix with a tasklet. Would be great if one
could shed some light on that.
Best regards
Andreas Bie?mann
[1] https://lkml.org/lkml/2014/4/16/425
> ---
> drivers/bluetooth/hci_ldisc.c | 24 +++++++++++++++++++-----
> drivers/bluetooth/hci_uart.h | 1 +
> 2 files changed, 20 insertions(+), 5 deletions(-)
On Wed, 23 Apr 2014 20:43:34 -0500 Felipe Balbi <[email protected]> wrote:
> so, Ack for $subject or not ?
>
Just at the moment I'm finding it hard to care.
So
Acked-by: NeilBrown <[email protected]>
Whatever....
NeilBrown
Hi,
On Thu, Apr 24, 2014 at 11:41:15AM +1000, NeilBrown wrote:
> On Wed, 23 Apr 2014 20:21:00 -0500 Felipe Balbi <[email protected]> wrote:
>
> > I have no problem either way, just that unused code doesn't have to be
> > sitting in the tree and I'm not entirely sure this GPIO should be
> > handled by omap-serial.c, perhaps something more generic inside
> > serial-core so other UART drivers can benefit from it.
>
> Perhaps. But there there are more people I need to convince :-)
heh, Greg is in Cc, that'd be a good start.
> > > On the other hand, if you can point out to me what I'm missing, and how I can
> > > solve my problem with any virtual GPIOs, I'm all ears.
> > >
> > > To make my problem simple and explicit: I have a device attached to a UART
> > > which has a separate regulator. The regulator should be powered on if and
> >
> > So you're using DTR to power the GPIO and hoping that the regulator
> > stabilizes quickly enough so that by the time your open() finishes you
> > don't have to add nonsensical msleep() calls before writing to the
> > device. Sounds a bit fragile to me.
>
> The gpio_set call is synchronous, and the gpio-regulator driver could add a
sure, but it's synchronous towards toggling the GPIO, pulling it high.
It doesn't guarantee that the far-end regulator's output will be fully
changed.
> delay (I think).
yeah, that'd be part of the regulator-gpio with the startup-delay-ns
property (IIRC)
> > > only if the /dev/ttyXX interface to the UART is open. The device is a
> > > bluetooth transceiver.
> >
> > considering this is a BTUART device, why didn't you do this at the ldisc
> > level ? hci_uart_open() sounds like a good choice from a quick thinking.
> >
>
> I'll have a look into that, thanks.
so, Ack for $subject or not ?
--
balbi
On Wed, 23 Apr 2014 20:21:00 -0500 Felipe Balbi <[email protected]> wrote:
> I have no problem either way, just that unused code doesn't have to be
> sitting in the tree and I'm not entirely sure this GPIO should be
> handled by omap-serial.c, perhaps something more generic inside
> serial-core so other UART drivers can benefit from it.
Perhaps. But there there are more people I need to convince :-)
>
> > On the other hand, if you can point out to me what I'm missing, and how I can
> > solve my problem with any virtual GPIOs, I'm all ears.
> >
> > To make my problem simple and explicit: I have a device attached to a UART
> > which has a separate regulator. The regulator should be powered on if and
>
> So you're using DTR to power the GPIO and hoping that the regulator
> stabilizes quickly enough so that by the time your open() finishes you
> don't have to add nonsensical msleep() calls before writing to the
> device. Sounds a bit fragile to me.
The gpio_set call is synchronous, and the gpio-regulator driver could add a
delay (I think).
>
> > only if the /dev/ttyXX interface to the UART is open. The device is a
> > bluetooth transceiver.
>
> considering this is a BTUART device, why didn't you do this at the ldisc
> level ? hci_uart_open() sounds like a good choice from a quick thinking.
>
I'll have a look into that, thanks.
NeilBrown
Hi,
On Thu, Apr 24, 2014 at 10:13:29AM +1000, NeilBrown wrote:
> On Wed, 23 Apr 2014 18:01:21 -0500 Felipe Balbi <[email protected]> wrote:
>
> > Hi,
> >
> > On Thu, Apr 24, 2014 at 08:43:05AM +1000, NeilBrown wrote:
> > > On Wed, 23 Apr 2014 10:35:04 -0500 Nishanth Menon <[email protected]> wrote:
> > >
> > > > On 04/23/2014 09:58 AM, Felipe Balbi wrote:
> > > > > nobody passes a DTR_gpio to this driver, so
> > > > > this code is not necessary.
> > > > >
> > > > > Signed-off-by: Felipe Balbi <[email protected]>
> > > > > ---
> > > >
> > > > Niel,
> > > > this seems to revert the functionality introduced in
> > > > commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100
> > > > (OMAP/serial: Add support for driving a GPIO as DTR.)
> > > >
> > > > would you like to Ack this change?
> > >
> > > I have a couple of out-of-tree drivers that use this support.
> > >
> > > I hope to get back to working on that code one day and even get those drivers
> > > upstream. So I would really prefer this code to remain if it isn't causing
> > > any actual problems.
> >
> > it causes problem with DT (not really). That suport is only available on
> > legacy platform_data-based boot, it's not available on DT. I hear Tony
> > is pretty close to turning OMAP3 DT-only.
> >
> > > Of course, I can always re-submit it when I need it again, but that it just
> > > extra work all around.
> >
> > I wonder how you will pass those attributes through DT considering they
> > are *really* SW configuration. Why can't you use the real DTR pin, btw ?
> >
>
> This myth that DT is only about hardware is probably one of the
> reasons that I haven't got these out-of-tree drivers upstream yet.
take that up with DT maintainers
> There is no "real" DTR pin.
heh, my bad... confused with RTS which is supported in this HW.
> When I open /dev/ttyWHATEVER, I need the driver for the device that is
> permanently connected to that serial port to be told that the serial-device
> has been opened so that it can "power on" or "wake up" the device.
>
> I don't much care how that happens, or how I tell DT that it has to happen.
> I just need it to happen.
>
> In discrete hardware devices, the DTR line is what you would use. The RS232
> port would raise (or lower or whatever) DTR when /dev/ttyWHATEVER was open,
> and the device that was plugged in would detect the level change and do
> stuff.
>
> But I don't have discrete hardware. I have a bunch of stuff soldered onto a
> board with ad-hoc connections chosen to make the life of the hardware builder
> easy rather than chosen to make the life of the software developer easy
> (which I think is the correct choice).
>
> So I need to tell DT "This device is plugged into this UART, and there is no
> DTR line, but when the UARTs DTR line would be asserted (if it had one), then
> I need that regulator of there turned on". or maybe "I need to toggle this
> GPIO with exactly this pattern, while watching that GPIO to see if it is
> working".
>
> So I thought:
>
> 1/ give the UART a "virtual" DTR so it could drive any GPIO
> 2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio
> and responded to state changes on that GPIO by turning on or off the
> regulator
> 3/ create a dedicated driver which knows how to toggle the magic GPIO while
> watching the other GPIO to convince the silly device to wakeup, or go to
> sleep, as required, and have this appear as a (virtual) GPIO.
>
> Then I can just tell DT that these (virtual) GPIOs are connected together,
> and it will all just work. And it does.
>
> But given the whole "no no, DT is for describing hardware, and you are
> describing software" attitude, it seems that I lost motivation for a while
> (that wasn't the only reason, but it didn't help).
>
> I have a patch which converts the OMAP serial driver to use DT to configure
> these virtual DTR lines. I'm very happy to submit that if there is some
> chance it might be accepted and will keep the current DTR code in place.
I have no problem either way, just that unused code doesn't have to be
sitting in the tree and I'm not entirely sure this GPIO should be
handled by omap-serial.c, perhaps something more generic inside
serial-core so other UART drivers can benefit from it.
> On the other hand, if you can point out to me what I'm missing, and how I can
> solve my problem with any virtual GPIOs, I'm all ears.
>
> To make my problem simple and explicit: I have a device attached to a UART
> which has a separate regulator. The regulator should be powered on if and
So you're using DTR to power the GPIO and hoping that the regulator
stabilizes quickly enough so that by the time your open() finishes you
don't have to add nonsensical msleep() calls before writing to the
device. Sounds a bit fragile to me.
> only if the /dev/ttyXX interface to the UART is open. The device is a
> bluetooth transceiver.
considering this is a BTUART device, why didn't you do this at the ldisc
level ? hci_uart_open() sounds like a good choice from a quick thinking.
--
balbi
On Wed, 23 Apr 2014 18:01:21 -0500 Felipe Balbi <[email protected]> wrote:
> Hi,
>
> On Thu, Apr 24, 2014 at 08:43:05AM +1000, NeilBrown wrote:
> > On Wed, 23 Apr 2014 10:35:04 -0500 Nishanth Menon <[email protected]> wrote:
> >
> > > On 04/23/2014 09:58 AM, Felipe Balbi wrote:
> > > > nobody passes a DTR_gpio to this driver, so
> > > > this code is not necessary.
> > > >
> > > > Signed-off-by: Felipe Balbi <[email protected]>
> > > > ---
> > >
> > > Niel,
> > > this seems to revert the functionality introduced in
> > > commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100
> > > (OMAP/serial: Add support for driving a GPIO as DTR.)
> > >
> > > would you like to Ack this change?
> >
> > I have a couple of out-of-tree drivers that use this support.
> >
> > I hope to get back to working on that code one day and even get those drivers
> > upstream. So I would really prefer this code to remain if it isn't causing
> > any actual problems.
>
> it causes problem with DT (not really). That suport is only available on
> legacy platform_data-based boot, it's not available on DT. I hear Tony
> is pretty close to turning OMAP3 DT-only.
>
> > Of course, I can always re-submit it when I need it again, but that it just
> > extra work all around.
>
> I wonder how you will pass those attributes through DT considering they
> are *really* SW configuration. Why can't you use the real DTR pin, btw ?
>
This myth that DT is only about hardware is probably one of the reasons that I
haven't got these out-of-tree drivers upstream yet.
There is no "real" DTR pin.
When I open /dev/ttyWHATEVER, I need the driver for the device that is
permanently connected to that serial port to be told that the serial-device
has been opened so that it can "power on" or "wake up" the device.
I don't much care how that happens, or how I tell DT that it has to happen.
I just need it to happen.
In discrete hardware devices, the DTR line is what you would use. The RS232
port would raise (or lower or whatever) DTR when /dev/ttyWHATEVER was open,
and the device that was plugged in would detect the level change and do
stuff.
But I don't have discrete hardware. I have a bunch of stuff soldered onto a
board with ad-hoc connections chosen to make the life of the hardware builder
easy rather than chosen to make the life of the software developer easy
(which I think is the correct choice).
So I need to tell DT "This device is plugged into this UART, and there is no
DTR line, but when the UARTs DTR line would be asserted (if it had one), then
I need that regulator of there turned on". or maybe "I need to toggle this
GPIO with exactly this pattern, while watching that GPIO to see if it is
working".
So I thought:
1/ give the UART a "virtual" DTR so it could drive any GPIO
2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio
and responded to state changes on that GPIO by turning on or off the
regulator
3/ create a dedicated driver which knows how to toggle the magic GPIO while
watching the other GPIO to convince the silly device to wakeup, or go to
sleep, as required, and have this appear as a (virtual) GPIO.
Then I can just tell DT that these (virtual) GPIOs are connected together,
and it will all just work. And it does.
But given the whole "no no, DT is for describing hardware, and you are
describing software" attitude, it seems that I lost motivation for a while
(that wasn't the only reason, but it didn't help).
I have a patch which converts the OMAP serial driver to use DT to configure
these virtual DTR lines. I'm very happy to submit that if there is some
chance it might be accepted and will keep the current DTR code in place.
On the other hand, if you can point out to me what I'm missing, and how I can
solve my problem with any virtual GPIOs, I'm all ears.
To make my problem simple and explicit: I have a device attached to a UART
which has a separate regulator. The regulator should be powered on if and
only if the /dev/ttyXX interface to the UART is open. The device is a
bluetooth transceiver.
(I have another device which is a GPS receiver which has similar
but more complicated requirements)
Thanks,
NeilBrown
Hi,
On Thu, Apr 24, 2014 at 08:43:05AM +1000, NeilBrown wrote:
> On Wed, 23 Apr 2014 10:35:04 -0500 Nishanth Menon <[email protected]> wrote:
>
> > On 04/23/2014 09:58 AM, Felipe Balbi wrote:
> > > nobody passes a DTR_gpio to this driver, so
> > > this code is not necessary.
> > >
> > > Signed-off-by: Felipe Balbi <[email protected]>
> > > ---
> >
> > Niel,
> > this seems to revert the functionality introduced in
> > commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100
> > (OMAP/serial: Add support for driving a GPIO as DTR.)
> >
> > would you like to Ack this change?
>
> I have a couple of out-of-tree drivers that use this support.
>
> I hope to get back to working on that code one day and even get those drivers
> upstream. So I would really prefer this code to remain if it isn't causing
> any actual problems.
it causes problem with DT (not really). That suport is only available on
legacy platform_data-based boot, it's not available on DT. I hear Tony
is pretty close to turning OMAP3 DT-only.
> Of course, I can always re-submit it when I need it again, but that it just
> extra work all around.
I wonder how you will pass those attributes through DT considering they
are *really* SW configuration. Why can't you use the real DTR pin, btw ?
--
balbi
On Wed, 23 Apr 2014 10:35:04 -0500 Nishanth Menon <[email protected]> wrote:
> On 04/23/2014 09:58 AM, Felipe Balbi wrote:
> > nobody passes a DTR_gpio to this driver, so
> > this code is not necessary.
> >
> > Signed-off-by: Felipe Balbi <[email protected]>
> > ---
>
> Niel,
> this seems to revert the functionality introduced in
> commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100
> (OMAP/serial: Add support for driving a GPIO as DTR.)
>
> would you like to Ack this change?
I have a couple of out-of-tree drivers that use this support.
I hope to get back to working on that code one day and even get those drivers
upstream. So I would really prefer this code to remain if it isn't causing
any actual problems.
Of course, I can always re-submit it when I need it again, but that it just
extra work all around.
Sorry that I have pushed those drivers already, but sometimes life gets in
the way :-)
Thanks,
NeilBrown
>
> > drivers/tty/serial/omap-serial.c | 39 ---------------------------------------
> > 1 file changed, 39 deletions(-)
> >
> > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> > index b46aaf3..6654682 100644
> > --- a/drivers/tty/serial/omap-serial.c
> > +++ b/drivers/tty/serial/omap-serial.c
> > @@ -163,10 +163,6 @@ struct uart_omap_port {
> > u8 wakeups_enabled;
> > u32 features;
> >
> > - int DTR_gpio;
> > - int DTR_inverted;
> > - int DTR_active;
> > -
> > struct serial_rs485 rs485;
> > int rts_gpio;
> >
> > @@ -694,16 +690,6 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
> > serial_out(up, UART_MCR, up->mcr);
> > pm_runtime_mark_last_busy(up->dev);
> > pm_runtime_put_autosuspend(up->dev);
> > -
> > - if (gpio_is_valid(up->DTR_gpio) &&
> > - !!(mctrl & TIOCM_DTR) != up->DTR_active) {
> > - up->DTR_active = !up->DTR_active;
> > - if (gpio_cansleep(up->DTR_gpio))
> > - schedule_work(&up->qos_work);
> > - else
> > - gpio_set_value(up->DTR_gpio,
> > - up->DTR_active != up->DTR_inverted);
> > - }
> > }
> >
> > static void serial_omap_break_ctl(struct uart_port *port, int break_state)
> > @@ -847,9 +833,6 @@ static void serial_omap_uart_qos_work(struct work_struct *work)
> > qos_work);
> >
> > pm_qos_update_request(&up->pm_qos_request, up->latency);
> > - if (gpio_is_valid(up->DTR_gpio))
> > - gpio_set_value_cansleep(up->DTR_gpio,
> > - up->DTR_active != up->DTR_inverted);
> > }
> >
> > static void
> > @@ -1672,28 +1655,6 @@ static int serial_omap_probe(struct platform_device *pdev)
> > if (IS_ERR(base))
> > return PTR_ERR(base);
> >
> > - if (gpio_is_valid(omap_up_info->DTR_gpio) &&
> > - omap_up_info->DTR_present) {
> > - ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio,
> > - "omap-serial");
> > - if (ret < 0)
> > - return ret;
> > - ret = gpio_direction_output(omap_up_info->DTR_gpio,
> > - omap_up_info->DTR_inverted);
> > - if (ret < 0)
> > - return ret;
> > - }
> > -
> > - if (gpio_is_valid(omap_up_info->DTR_gpio) &&
> > - omap_up_info->DTR_present) {
> > - up->DTR_gpio = omap_up_info->DTR_gpio;
> > - up->DTR_inverted = omap_up_info->DTR_inverted;
> > - } else {
> > - up->DTR_gpio = -EINVAL;
> > - }
> > -
> > - up->DTR_active = 0;
> > -
> > up->dev = &pdev->dev;
> > up->port.dev = &pdev->dev;
> > up->port.type = PORT_OMAP;
> >
>
>
On Wed, Apr 23, 2014 at 12:27:59PM -0300, Fabio Estevam wrote:
> On Wed, Apr 23, 2014 at 11:58 AM, Felipe Balbi <[email protected]> wrote:
>
> > @@ -1658,12 +1657,9 @@ static int serial_omap_probe(struct platform_device *pdev)
> > omap_up_info = of_get_uart_port_info(&pdev->dev);
> > pdev->dev.platform_data = omap_up_info;
> > } else {
> > - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > - if (!irq) {
> > - dev_err(&pdev->dev, "no irq resource?\n");
> > - return -ENODEV;
> > - }
> > - uartirq = irq->start;
> > + uartirq = platform_get_irq(pdev, 0);
> > + if (uartirq < 0)
> > + return -EPROBE_DEFER;
>
>
> Maybe you could just do a 'return uartirq' here instead.
I don't mind either way, I'm only returning -EPROBE_DEFER because that's
what the other branch of this conditional returns. Tony, what do you
prefer ?
--
balbi
On 04/23/2014 09:58 AM, Felipe Balbi wrote:
> nobody passes a DTR_gpio to this driver, so
> this code is not necessary.
>
> Signed-off-by: Felipe Balbi <[email protected]>
> ---
Niel,
this seems to revert the functionality introduced in
commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100
(OMAP/serial: Add support for driving a GPIO as DTR.)
would you like to Ack this change?
> drivers/tty/serial/omap-serial.c | 39 ---------------------------------------
> 1 file changed, 39 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index b46aaf3..6654682 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -163,10 +163,6 @@ struct uart_omap_port {
> u8 wakeups_enabled;
> u32 features;
>
> - int DTR_gpio;
> - int DTR_inverted;
> - int DTR_active;
> -
> struct serial_rs485 rs485;
> int rts_gpio;
>
> @@ -694,16 +690,6 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
> serial_out(up, UART_MCR, up->mcr);
> pm_runtime_mark_last_busy(up->dev);
> pm_runtime_put_autosuspend(up->dev);
> -
> - if (gpio_is_valid(up->DTR_gpio) &&
> - !!(mctrl & TIOCM_DTR) != up->DTR_active) {
> - up->DTR_active = !up->DTR_active;
> - if (gpio_cansleep(up->DTR_gpio))
> - schedule_work(&up->qos_work);
> - else
> - gpio_set_value(up->DTR_gpio,
> - up->DTR_active != up->DTR_inverted);
> - }
> }
>
> static void serial_omap_break_ctl(struct uart_port *port, int break_state)
> @@ -847,9 +833,6 @@ static void serial_omap_uart_qos_work(struct work_struct *work)
> qos_work);
>
> pm_qos_update_request(&up->pm_qos_request, up->latency);
> - if (gpio_is_valid(up->DTR_gpio))
> - gpio_set_value_cansleep(up->DTR_gpio,
> - up->DTR_active != up->DTR_inverted);
> }
>
> static void
> @@ -1672,28 +1655,6 @@ static int serial_omap_probe(struct platform_device *pdev)
> if (IS_ERR(base))
> return PTR_ERR(base);
>
> - if (gpio_is_valid(omap_up_info->DTR_gpio) &&
> - omap_up_info->DTR_present) {
> - ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio,
> - "omap-serial");
> - if (ret < 0)
> - return ret;
> - ret = gpio_direction_output(omap_up_info->DTR_gpio,
> - omap_up_info->DTR_inverted);
> - if (ret < 0)
> - return ret;
> - }
> -
> - if (gpio_is_valid(omap_up_info->DTR_gpio) &&
> - omap_up_info->DTR_present) {
> - up->DTR_gpio = omap_up_info->DTR_gpio;
> - up->DTR_inverted = omap_up_info->DTR_inverted;
> - } else {
> - up->DTR_gpio = -EINVAL;
> - }
> -
> - up->DTR_active = 0;
> -
> up->dev = &pdev->dev;
> up->port.dev = &pdev->dev;
> up->port.type = PORT_OMAP;
>
--
Regards,
Nishanth Menon
+Neil
On Wed, Apr 23, 2014 at 09:58:34AM -0500, Felipe Balbi wrote:
> nobody passes a DTR_gpio to this driver, so
> this code is not necessary.
>
> Signed-off-by: Felipe Balbi <[email protected]>
> ---
> drivers/tty/serial/omap-serial.c | 39 ---------------------------------------
> 1 file changed, 39 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index b46aaf3..6654682 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -163,10 +163,6 @@ struct uart_omap_port {
> u8 wakeups_enabled;
> u32 features;
>
> - int DTR_gpio;
> - int DTR_inverted;
> - int DTR_active;
> -
> struct serial_rs485 rs485;
> int rts_gpio;
>
> @@ -694,16 +690,6 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
> serial_out(up, UART_MCR, up->mcr);
> pm_runtime_mark_last_busy(up->dev);
> pm_runtime_put_autosuspend(up->dev);
> -
> - if (gpio_is_valid(up->DTR_gpio) &&
> - !!(mctrl & TIOCM_DTR) != up->DTR_active) {
> - up->DTR_active = !up->DTR_active;
> - if (gpio_cansleep(up->DTR_gpio))
> - schedule_work(&up->qos_work);
> - else
> - gpio_set_value(up->DTR_gpio,
> - up->DTR_active != up->DTR_inverted);
> - }
> }
>
> static void serial_omap_break_ctl(struct uart_port *port, int break_state)
> @@ -847,9 +833,6 @@ static void serial_omap_uart_qos_work(struct work_struct *work)
> qos_work);
>
> pm_qos_update_request(&up->pm_qos_request, up->latency);
> - if (gpio_is_valid(up->DTR_gpio))
> - gpio_set_value_cansleep(up->DTR_gpio,
> - up->DTR_active != up->DTR_inverted);
> }
>
> static void
> @@ -1672,28 +1655,6 @@ static int serial_omap_probe(struct platform_device *pdev)
> if (IS_ERR(base))
> return PTR_ERR(base);
>
> - if (gpio_is_valid(omap_up_info->DTR_gpio) &&
> - omap_up_info->DTR_present) {
> - ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio,
> - "omap-serial");
> - if (ret < 0)
> - return ret;
> - ret = gpio_direction_output(omap_up_info->DTR_gpio,
> - omap_up_info->DTR_inverted);
> - if (ret < 0)
> - return ret;
> - }
> -
> - if (gpio_is_valid(omap_up_info->DTR_gpio) &&
> - omap_up_info->DTR_present) {
> - up->DTR_gpio = omap_up_info->DTR_gpio;
> - up->DTR_inverted = omap_up_info->DTR_inverted;
> - } else {
> - up->DTR_gpio = -EINVAL;
> - }
> -
> - up->DTR_active = 0;
> -
> up->dev = &pdev->dev;
> up->port.dev = &pdev->dev;
> up->port.type = PORT_OMAP;
> --
> 1.9.2.459.g68773ac
>
--
balbi
On Wed, Apr 23, 2014 at 11:58 AM, Felipe Balbi <[email protected]> wrote:
> @@ -1658,12 +1657,9 @@ static int serial_omap_probe(struct platform_device *pdev)
> omap_up_info = of_get_uart_port_info(&pdev->dev);
> pdev->dev.platform_data = omap_up_info;
> } else {
> - irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> - if (!irq) {
> - dev_err(&pdev->dev, "no irq resource?\n");
> - return -ENODEV;
> - }
> - uartirq = irq->start;
> + uartirq = platform_get_irq(pdev, 0);
> + if (uartirq < 0)
> + return -EPROBE_DEFER;
Maybe you could just do a 'return uartirq' here instead.
Fix the following Sparse warnings:
drivers/tty/serial/omap-serial.c:1418:49: warning: incorrect \
type in argument 2 (different address spaces)
drivers/tty/serial/omap-serial.c:1418:49: expected void const \
[noderef] <asn:1>*from
drivers/tty/serial/omap-serial.c:1418:49: got struct serial_rs485 \
*<noident>
drivers/tty/serial/omap-serial.c:1426:35: warning: incorrect \
type in argument 1 (different address spaces)
drivers/tty/serial/omap-serial.c:1426:35: expected void [noderef] \
<asn:1>*to
drivers/tty/serial/omap-serial.c:1426:35: got struct serial_rs485 \
*<noident>
Reported-by: Nishanth Menon <[email protected]>
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index ab22dab..d017cec 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1398,7 +1398,7 @@ serial_omap_ioctl(struct uart_port *port, unsigned int cmd, unsigned long arg)
switch (cmd) {
case TIOCSRS485:
- if (copy_from_user(&rs485conf, (struct serial_rs485 *) arg,
+ if (copy_from_user(&rs485conf, (void __user *) arg,
sizeof(rs485conf)))
return -EFAULT;
@@ -1406,7 +1406,7 @@ serial_omap_ioctl(struct uart_port *port, unsigned int cmd, unsigned long arg)
break;
case TIOCGRS485:
- if (copy_to_user((struct serial_rs485 *) arg,
+ if (copy_to_user((void __user *) arg,
&(to_uart_omap_port(port)->rs485),
sizeof(rs485conf)))
return -EFAULT;
--
1.9.2.459.g68773ac
NOT COMPLETE
NYET-Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/8250/8250_omap.c | 233 ++++++++++++++++++++++++++++++++++++
drivers/tty/serial/8250/Kconfig | 7 ++
drivers/tty/serial/8250/Makefile | 1 +
3 files changed, 241 insertions(+)
create mode 100644 drivers/tty/serial/8250/8250_omap.c
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
new file mode 100644
index 0000000..e8ae479
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -0,0 +1,233 @@
+/*
+ * 8250-omap.c - OMAP adaptation for 8250 driver
+ *
+ * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Author: Felipe Balbi <[email protected]>
+ *
+ * Based on omap-serial.c:
+ *
+ * Copyright (C) 2010 Texas Instruments
+ *
+ * Authors:
+ * Govindraj R <[email protected]>
+ * Thara Gopinath <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * Note: This driver is made separate from 8250 driver as we cannot
+ * over load 8250 driver with omap platform specific configuration for
+ * features like DMA, it makes easier to implement features like DMA and
+ * hardware flow control and software flow control configuration with
+ * this driver as required for the omap-platform.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/serial_8250.h>
+#include <linux/serial_core.h>
+#include <linux/serial_reg.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_data/serial-omap.h>
+
+#include <dt-bindings/gpio/gpio.h>
+
+#define OMAP_MAX_HSUART_PORTS 6
+
+#define UART_BUILD_REVISION(x, y) (((x) << 8) | (y))
+
+#define OMAP_UART_REV_42 0x0402
+#define OMAP_UART_REV_46 0x0406
+#define OMAP_UART_REV_52 0x0502
+#define OMAP_UART_REV_63 0x0603
+
+#define OMAP_UART_TX_WAKEUP_EN BIT(7)
+
+/* Feature flags */
+#define OMAP_UART_WER_HAS_TX_WAKEUP BIT(0)
+
+#define UART_ERRATA_i202_MDR1_ACCESS BIT(0)
+#define UART_ERRATA_i291_DMA_FORCEIDLE BIT(1)
+
+#define DEFAULT_CLK_SPEED 48000000 /* 48Mhz*/
+
+/* SCR register bitmasks */
+#define OMAP_UART_SCR_RX_TRIG_GRANU1_MASK (1 << 7)
+#define OMAP_UART_SCR_TX_TRIG_GRANU1_MASK (1 << 6)
+#define OMAP_UART_SCR_TX_EMPTY (1 << 3)
+
+/* FCR register bitmasks */
+#define OMAP_UART_FCR_RX_FIFO_TRIG_MASK (0x3 << 6)
+#define OMAP_UART_FCR_TX_FIFO_TRIG_MASK (0x3 << 4)
+
+/* MVR register bitmasks */
+#define OMAP_UART_MVR_SCHEME_SHIFT 30
+
+#define OMAP_UART_LEGACY_MVR_MAJ_MASK 0xf0
+#define OMAP_UART_LEGACY_MVR_MAJ_SHIFT 4
+#define OMAP_UART_LEGACY_MVR_MIN_MASK 0x0f
+
+#define OMAP_UART_MVR_MAJ_MASK 0x700
+#define OMAP_UART_MVR_MAJ_SHIFT 8
+#define OMAP_UART_MVR_MIN_MASK 0x3f
+
+#define OMAP_UART_DMA_CH_FREE -1
+
+#define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
+#define OMAP_MODE13X_SPEED 230400
+
+/* WER = 0x7F
+ * Enable module level wakeup in WER reg
+ */
+#define OMAP_UART_WER_MOD_WKUP 0X7F
+
+/* Enable XON/XOFF flow control on output */
+#define OMAP_UART_SW_TX 0x08
+
+/* Enable XON/XOFF flow control on input */
+#define OMAP_UART_SW_RX 0x02
+
+#define OMAP_UART_SW_CLR 0xF0
+
+#define OMAP_UART_TCR_TRIG 0x0F
+
+struct uart_omap_port {
+ struct uart_8250_port uart;
+ struct device *dev;
+ int wakeirq;
+
+ unsigned char ier;
+ unsigned char lcr;
+ unsigned char mcr;
+ unsigned char fcr;
+ unsigned char efr;
+ unsigned char dll;
+ unsigned char dlh;
+ unsigned char mdr1;
+ unsigned char scr;
+ unsigned char wer;
+
+ int line;
+
+ /*
+ * Some bits in registers are cleared on a read, so they must
+ * be saved whenever the register is read but the bits will not
+ * be immediately processed.
+ */
+ unsigned int lsr_break_flag;
+ unsigned char msr_saved_flags;
+ char name[20];
+ unsigned long port_activity;
+ int context_loss_cnt;
+ u32 errata;
+ u8 wakeups_enabled;
+ u32 features;
+};
+
+#define to_uart_omap_port(p) ((container_of((p), struct uart_omap_port, port)))
+
+static int serial_omap_probe(struct platform_device *pdev)
+{
+ struct uart_omap_port *up;
+ struct resource *mem;
+ void __iomem *base;
+ int uartirq = 0;
+ int ret;
+
+ if (!pdev->dev.of_node)
+ return -ENODEV;
+
+ uartirq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+ if (!uartirq)
+ return -EPROBE_DEFER;
+
+ up = devm_kzalloc(&pdev->dev, sizeof(*up), GFP_KERNEL);
+ if (!up)
+ return -ENOMEM;
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ base = devm_ioremap_resource(&pdev->dev, mem);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ up->dev = &pdev->dev;
+ up->uart.port.mapbase = mem->start;
+ up->uart.port.membase = base;
+ up->uart.port.irq = uartirq;
+ up->uart.port.dev = &pdev->dev;
+ up->uart.port.type = PORT_16750;
+ up->uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_PORT;
+ up->uart.port.iotype = UPIO_MEM;
+
+ if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
+ &up->uart.port.uartclk)) {
+ up->uart.port.uartclk = DEFAULT_CLK_SPEED;
+ dev_warn(&pdev->dev,
+ "No clock speed specified: using default: %d\n",
+ DEFAULT_CLK_SPEED);
+ }
+
+ platform_set_drvdata(pdev, up);
+
+ device_init_wakeup(up->dev, true);
+ pm_runtime_irq_safe(&pdev->dev);
+ pm_runtime_enable(&pdev->dev);
+ pm_runtime_get_sync(&pdev->dev);
+
+ ret = serial8250_register_8250_port(&up->uart);
+ if (ret < 0)
+ goto err_add_port;
+
+ up->line = ret;
+
+ return 0;
+
+err_add_port:
+ pm_runtime_put(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+ dev_err(&pdev->dev, "[UART%d]: failure [%s]: %d\n",
+ pdev->id, __func__, ret);
+ return ret;
+}
+
+static int serial_omap_remove(struct platform_device *dev)
+{
+ struct uart_omap_port *up = platform_get_drvdata(dev);
+
+ serial8250_unregister_port(up->line);
+
+ pm_runtime_put_sync(up->dev);
+ pm_runtime_disable(up->dev);
+
+ return 0;
+}
+
+static const struct of_device_id omap_serial_of_match[] = {
+ { .compatible = "ti,am335x-uart" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, omap_serial_of_match);
+
+static struct platform_driver serial_omap_driver = {
+ .probe = serial_omap_probe,
+ .remove = serial_omap_remove,
+ .driver = {
+ .name = DRIVER_NAME,
+ .of_match_table = of_match_ptr(omap_serial_of_match),
+ },
+};
+
+module_platform_driver(serial_omap_driver);
+
+MODULE_DESCRIPTION("OMAP High Speed UART driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Texas Instruments Incorporated");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 2332991..ad092d5 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -287,6 +287,13 @@ config SERIAL_8250_DW
Selecting this option will enable handling of the extra features
present in the Synopsys DesignWare APB UART.
+config SERIAL_8250_OMAP
+ tristate "Support for OMAP 8250 quirks"
+ depends on SERIAL_8250
+ help
+ Selecting this option will enable handling of the extra features
+ present in the TI OMAP UART.
+
config SERIAL_8250_EM
tristate "Support for Emma Mobile integrated serial port"
depends on SERIAL_8250 && ARM && HAVE_CLK
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 36d68d0..0119a11 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -19,4 +19,5 @@ obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554) += 8250_exar_st16c554.o
obj-$(CONFIG_SERIAL_8250_HUB6) += 8250_hub6.o
obj-$(CONFIG_SERIAL_8250_FSL) += 8250_fsl.o
obj-$(CONFIG_SERIAL_8250_DW) += 8250_dw.o
+obj-$(CONFIG_SERIAL_8250_OMAP) += 8250_omap.o
obj-$(CONFIG_SERIAL_8250_EM) += 8250_em.o
--
1.9.2.459.g68773ac
it wasn't used by anything, just remove it.
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 6654682..ab22dab 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -180,8 +180,6 @@ static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
/* Forward declaration of functions */
static void serial_omap_mdr1_errataset(struct uart_omap_port *up, u8 mdr1);
-static struct workqueue_struct *serial_omap_uart_wq;
-
static inline unsigned int serial_in(struct uart_omap_port *up, int offset)
{
offset <<= up->port.regshift;
@@ -1701,7 +1699,6 @@ static int serial_omap_probe(struct platform_device *pdev)
up->calc_latency = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE;
pm_qos_add_request(&up->pm_qos_request,
PM_QOS_CPU_DMA_LATENCY, up->latency);
- serial_omap_uart_wq = create_singlethread_workqueue(up->name);
INIT_WORK(&up->qos_work, serial_omap_uart_qos_work);
platform_set_drvdata(pdev, up);
--
1.9.2.459.g68773ac
nobody passes a DTR_gpio to this driver, so
this code is not necessary.
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 39 ---------------------------------------
1 file changed, 39 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index b46aaf3..6654682 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -163,10 +163,6 @@ struct uart_omap_port {
u8 wakeups_enabled;
u32 features;
- int DTR_gpio;
- int DTR_inverted;
- int DTR_active;
-
struct serial_rs485 rs485;
int rts_gpio;
@@ -694,16 +690,6 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
serial_out(up, UART_MCR, up->mcr);
pm_runtime_mark_last_busy(up->dev);
pm_runtime_put_autosuspend(up->dev);
-
- if (gpio_is_valid(up->DTR_gpio) &&
- !!(mctrl & TIOCM_DTR) != up->DTR_active) {
- up->DTR_active = !up->DTR_active;
- if (gpio_cansleep(up->DTR_gpio))
- schedule_work(&up->qos_work);
- else
- gpio_set_value(up->DTR_gpio,
- up->DTR_active != up->DTR_inverted);
- }
}
static void serial_omap_break_ctl(struct uart_port *port, int break_state)
@@ -847,9 +833,6 @@ static void serial_omap_uart_qos_work(struct work_struct *work)
qos_work);
pm_qos_update_request(&up->pm_qos_request, up->latency);
- if (gpio_is_valid(up->DTR_gpio))
- gpio_set_value_cansleep(up->DTR_gpio,
- up->DTR_active != up->DTR_inverted);
}
static void
@@ -1672,28 +1655,6 @@ static int serial_omap_probe(struct platform_device *pdev)
if (IS_ERR(base))
return PTR_ERR(base);
- if (gpio_is_valid(omap_up_info->DTR_gpio) &&
- omap_up_info->DTR_present) {
- ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio,
- "omap-serial");
- if (ret < 0)
- return ret;
- ret = gpio_direction_output(omap_up_info->DTR_gpio,
- omap_up_info->DTR_inverted);
- if (ret < 0)
- return ret;
- }
-
- if (gpio_is_valid(omap_up_info->DTR_gpio) &&
- omap_up_info->DTR_present) {
- up->DTR_gpio = omap_up_info->DTR_gpio;
- up->DTR_inverted = omap_up_info->DTR_inverted;
- } else {
- up->DTR_gpio = -EINVAL;
- }
-
- up->DTR_active = 0;
-
up->dev = &pdev->dev;
up->port.dev = &pdev->dev;
up->port.type = PORT_OMAP;
--
1.9.2.459.g68773ac
cleanup only, no functional changes.
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 07d4273..3813740 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1641,10 +1641,13 @@ static int serial_omap_probe_rs485(struct uart_omap_port *up,
static int serial_omap_probe(struct platform_device *pdev)
{
- struct uart_omap_port *up;
- struct resource *mem, *irq;
struct omap_uart_port_info *omap_up_info = dev_get_platdata(&pdev->dev);
- int ret, uartirq = 0, wakeirq = 0;
+ struct uart_omap_port *up;
+ struct resource *mem;
+ struct resource *irq;
+ int uartirq = 0;
+ int wakeirq = 0;
+ int ret;
/* The optional wakeirq may be specified in the board dts file */
if (pdev->dev.of_node) {
--
1.9.2.459.g68773ac
just using helper function to remove some duplicated
code a bit. While at that, also move allocation of
struct uart_omap_port higher in the code so that
we return much earlier in case of no memory.
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 32 +++++++++-----------------------
1 file changed, 9 insertions(+), 23 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index cb45e88..b46aaf3 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1644,6 +1644,7 @@ static int serial_omap_probe(struct platform_device *pdev)
struct omap_uart_port_info *omap_up_info = dev_get_platdata(&pdev->dev);
struct uart_omap_port *up;
struct resource *mem;
+ void __iomem *base;
int uartirq = 0;
int wakeirq = 0;
int ret;
@@ -1662,17 +1663,14 @@ static int serial_omap_probe(struct platform_device *pdev)
return -EPROBE_DEFER;
}
- mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!mem) {
- dev_err(&pdev->dev, "no mem resource?\n");
- return -ENODEV;
- }
+ up = devm_kzalloc(&pdev->dev, sizeof(*up), GFP_KERNEL);
+ if (!up)
+ return -ENOMEM;
- if (!devm_request_mem_region(&pdev->dev, mem->start, resource_size(mem),
- pdev->dev.driver->name)) {
- dev_err(&pdev->dev, "memory region already claimed\n");
- return -EBUSY;
- }
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ base = devm_ioremap_resource(&pdev->dev, mem);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
if (gpio_is_valid(omap_up_info->DTR_gpio) &&
omap_up_info->DTR_present) {
@@ -1686,10 +1684,6 @@ static int serial_omap_probe(struct platform_device *pdev)
return ret;
}
- up = devm_kzalloc(&pdev->dev, sizeof(*up), GFP_KERNEL);
- if (!up)
- return -ENOMEM;
-
if (gpio_is_valid(omap_up_info->DTR_gpio) &&
omap_up_info->DTR_present) {
up->DTR_gpio = omap_up_info->DTR_gpio;
@@ -1732,14 +1726,7 @@ static int serial_omap_probe(struct platform_device *pdev)
sprintf(up->name, "OMAP UART%d", up->port.line);
up->port.mapbase = mem->start;
- up->port.membase = devm_ioremap(&pdev->dev, mem->start,
- resource_size(mem));
- if (!up->port.membase) {
- dev_err(&pdev->dev, "can't ioremap UART\n");
- ret = -ENOMEM;
- goto err_ioremap;
- }
-
+ up->port.membase = base;
up->port.flags = omap_up_info->flags;
up->port.uartclk = omap_up_info->uartclk;
if (!up->port.uartclk) {
@@ -1786,7 +1773,6 @@ static int serial_omap_probe(struct platform_device *pdev)
err_add_port:
pm_runtime_put(&pdev->dev);
pm_runtime_disable(&pdev->dev);
-err_ioremap:
err_rs485:
err_port_line:
dev_err(&pdev->dev, "[UART%d]: failure [%s]: %d\n",
--
1.9.2.459.g68773ac
this will make sure gpio gets freed automatically
when this device is destroyed.
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index f456f46..07d4273 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1611,7 +1611,7 @@ static int serial_omap_probe_rs485(struct uart_omap_port *up,
/* check for tx enable gpio */
up->rts_gpio = of_get_named_gpio_flags(np, "rts-gpio", 0, &flags);
if (gpio_is_valid(up->rts_gpio)) {
- ret = gpio_request(up->rts_gpio, "omap-serial");
+ ret = devm_gpio_request(up->dev, up->rts_gpio, "omap-serial");
if (ret < 0)
return ret;
ret = gpio_direction_output(up->rts_gpio,
@@ -1677,7 +1677,8 @@ static int serial_omap_probe(struct platform_device *pdev)
if (gpio_is_valid(omap_up_info->DTR_gpio) &&
omap_up_info->DTR_present) {
- ret = gpio_request(omap_up_info->DTR_gpio, "omap-serial");
+ ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio,
+ "omap-serial");
if (ret < 0)
return ret;
ret = gpio_direction_output(omap_up_info->DTR_gpio,
--
1.9.2.459.g68773ac
this way we can remove one pointer declaration.
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 3813740..cb45e88 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1644,7 +1644,6 @@ static int serial_omap_probe(struct platform_device *pdev)
struct omap_uart_port_info *omap_up_info = dev_get_platdata(&pdev->dev);
struct uart_omap_port *up;
struct resource *mem;
- struct resource *irq;
int uartirq = 0;
int wakeirq = 0;
int ret;
@@ -1658,12 +1657,9 @@ static int serial_omap_probe(struct platform_device *pdev)
omap_up_info = of_get_uart_port_info(&pdev->dev);
pdev->dev.platform_data = omap_up_info;
} else {
- irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
- if (!irq) {
- dev_err(&pdev->dev, "no irq resource?\n");
- return -ENODEV;
- }
- uartirq = irq->start;
+ uartirq = platform_get_irq(pdev, 0);
+ if (uartirq < 0)
+ return -EPROBE_DEFER;
}
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
--
1.9.2.459.g68773ac
per CodingStyle we should have those braces, no
functional changes.
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 837f6c1..f456f46 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1694,8 +1694,10 @@ static int serial_omap_probe(struct platform_device *pdev)
omap_up_info->DTR_present) {
up->DTR_gpio = omap_up_info->DTR_gpio;
up->DTR_inverted = omap_up_info->DTR_inverted;
- } else
+ } else {
up->DTR_gpio = -EINVAL;
+ }
+
up->DTR_active = 0;
up->dev = &pdev->dev;
@@ -1757,6 +1759,7 @@ static int serial_omap_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, up);
if (omap_up_info->autosuspend_timeout == 0)
omap_up_info->autosuspend_timeout = -1;
+
device_init_wakeup(up->dev, true);
pm_runtime_use_autosuspend(&pdev->dev);
pm_runtime_set_autosuspend_delay(&pdev->dev,
--
1.9.2.459.g68773ac
This reverts commit 0324a821029e1f54e7a7f8fed48693cfce42dc0e.
That commit tried to fix a deadlock problem when using
hci_ldisc, but it turns out the bug was in hci_ldsic
all along where it was calling ->write() from within
->write_wakeup() callback.
The problem is that ->write_wakeup() was called with
port lock held and ->write() tried to grab the same
port lock.
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/tty/serial/omap-serial.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 08b6b94..837f6c1 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -398,11 +398,8 @@ static void transmit_chars(struct uart_omap_port *up, unsigned int lsr)
break;
} while (--count > 0);
- if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) {
- spin_unlock(&up->port.lock);
+ if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
uart_write_wakeup(&up->port);
- spin_lock(&up->port.lock);
- }
if (uart_circ_empty(xmit))
serial_omap_stop_tx(&up->port);
--
1.9.2.459.g68773ac
UART IRQ Identification bitfield is 3
bits long (bits 3:1) but current mask only
masks 2 bits. Fix it.
Signed-off-by: Felipe Balbi <[email protected]>
---
include/uapi/linux/serial_reg.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h
index e632260..99b4705 100644
--- a/include/uapi/linux/serial_reg.h
+++ b/include/uapi/linux/serial_reg.h
@@ -32,7 +32,7 @@
#define UART_IIR 2 /* In: Interrupt ID Register */
#define UART_IIR_NO_INT 0x01 /* No interrupts pending */
-#define UART_IIR_ID 0x06 /* Mask for the interrupt ID */
+#define UART_IIR_ID 0x0e /* Mask for the interrupt ID */
#define UART_IIR_MSI 0x00 /* Modem status interrupt */
#define UART_IIR_THRI 0x02 /* Transmitter holding register empty */
#define UART_IIR_RDI 0x04 /* Receiver data interrupt */
--
1.9.2.459.g68773ac
LDISCs shouldn't call tty->ops->write() from within
->write_wakeup().
->write_wakeup() is called with port lock taken and
IRQs disabled, tty->ops->write() will try to acquire
the same port lock and we will deadlock.
Acked-by: Marcel Holtmann <[email protected]>
Reviewed-by: Peter Hurley <[email protected]>
Reported-by: Huang Shijie <[email protected]>
Signed-off-by: Felipe Balbi <[email protected]>
---
drivers/bluetooth/hci_ldisc.c | 24 +++++++++++++++++++-----
drivers/bluetooth/hci_uart.h | 1 +
2 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index f1fbf4f..e00f8f5 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -118,10 +118,6 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
int hci_uart_tx_wakeup(struct hci_uart *hu)
{
- struct tty_struct *tty = hu->tty;
- struct hci_dev *hdev = hu->hdev;
- struct sk_buff *skb;
-
if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
return 0;
@@ -129,6 +125,22 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
BT_DBG("");
+ schedule_work(&hu->write_work);
+
+ return 0;
+}
+
+static void hci_uart_write_work(struct work_struct *work)
+{
+ struct hci_uart *hu = container_of(work, struct hci_uart, write_work);
+ struct tty_struct *tty = hu->tty;
+ struct hci_dev *hdev = hu->hdev;
+ struct sk_buff *skb;
+
+ /* REVISIT: should we cope with bad skbs or ->write() returning
+ * and error value ?
+ */
+
restart:
clear_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
@@ -153,7 +165,6 @@ restart:
goto restart;
clear_bit(HCI_UART_SENDING, &hu->tx_state);
- return 0;
}
static void hci_uart_init_work(struct work_struct *work)
@@ -282,6 +293,7 @@ static int hci_uart_tty_open(struct tty_struct *tty)
tty->receive_room = 65536;
INIT_WORK(&hu->init_ready, hci_uart_init_work);
+ INIT_WORK(&hu->write_work, hci_uart_write_work);
spin_lock_init(&hu->rx_lock);
@@ -319,6 +331,8 @@ static void hci_uart_tty_close(struct tty_struct *tty)
if (hdev)
hci_uart_close(hdev);
+ cancel_work_sync(&hu->write_work);
+
if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
if (hdev) {
if (test_bit(HCI_UART_REGISTERED, &hu->flags))
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index fffa61f..12df101 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -68,6 +68,7 @@ struct hci_uart {
unsigned long hdev_flags;
struct work_struct init_ready;
+ struct work_struct write_work;
struct hci_uart_proto *proto;
void *priv;
--
1.9.2.459.g68773ac
Hi
Am Freitag, den 22.08.2014, 10:51 -0500 schrieb Greg KH:
> On Fri, Aug 22, 2014 at 03:45:07PM +0200, Tim Niemeyer wrote:
> > This Patch was applied to 3.10 and 3.2. It's probably missed on 3.4.
>
> Does it apply there?
The original patch applies not cleanly on 3.4. The init_work is not
available there.
I backported it and it's the same result as the backport from Andreas.
He tested the patch on 3.4.87. The attached patch of my last mail
applies.
> What is the git id of the commit?
Sorry, missed it:
commit da64c27d3c93ee9f89956b9de86c4127eb244494 upstream
commit a22d29e6e5757b1daed7d0b409a815eb33f66e4e stable-3.10
commit 87520218746b8d973670e37237666f174590898c stable-3.2
Tim
On Fri, Aug 22, 2014 at 03:45:07PM +0200, Tim Niemeyer wrote:
> This Patch was applied to 3.10 and 3.2. It's probably missed on 3.4.
Does it apply there?
What is the git id of the commit?
thanks,
greg k-h
This Patch was applied to 3.10 and 3.2. It's probably missed on 3.4.
---------------------
From: Felipe Balbi <[email protected]>
LDISCs shouldn't call tty->ops->write() from within
->write_wakeup().
->write_wakeup() is called with port lock taken and
IRQs disabled, tty->ops->write() will try to acquire
the same port lock and we will deadlock.
Acked-by: Marcel Holtmann <[email protected]>
Reviewed-by: Peter Hurley <[email protected]>
Reported-by: Huang Shijie <[email protected]>
Signed-off-by: Felipe Balbi <[email protected]>
Tested-by: Andreas Bie=C3=9Fmann <[email protected]>
[[email protected]: rebased on 3.4.103]
Signed-off-by: Tim Niemeyer <[email protected]>
---
drivers/bluetooth/hci_ldisc.c | 25 ++++++++++++++++++++-----
drivers/bluetooth/hci_uart.h | 2 ++
2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.=
c
index 98a8c05..d4550f9 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -118,10 +118,6 @@ static inline struct sk_buff *hci_uart_dequeue(struc=
t hci_uart *hu)
=20
int hci_uart_tx_wakeup(struct hci_uart *hu)
{
- struct tty_struct *tty =3D hu->tty;
- struct hci_dev *hdev =3D hu->hdev;
- struct sk_buff *skb;
-
if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
return 0;
@@ -129,6 +125,22 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
=20
BT_DBG("");
=20
+ schedule_work(&hu->write_work);
+
+ return 0;
+}
+
+static void hci_uart_write_work(struct work_struct *work)
+{
+ struct hci_uart *hu =3D container_of(work, struct hci_uart, write_work)=
;
+ struct tty_struct *tty =3D hu->tty;
+ struct hci_dev *hdev =3D hu->hdev;
+ struct sk_buff *skb;
+
+ /* REVISIT: should we cope with bad skbs or ->write() returning
+ * and error value ?
+ */
+
restart:
clear_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
=20
@@ -153,7 +165,6 @@ restart:
goto restart;
=20
clear_bit(HCI_UART_SENDING, &hu->tx_state);
- return 0;
}
=20
/* ------- Interface to HCI layer ------ */
@@ -264,6 +275,8 @@ static int hci_uart_tty_open(struct tty_struct *tty)
hu->tty =3D tty;
tty->receive_room =3D 65536;
=20
+ INIT_WORK(&hu->write_work, hci_uart_write_work);
+
spin_lock_init(&hu->rx_lock);
=20
/* Flush any pending characters in the driver and line discipline. */
@@ -298,6 +311,8 @@ static void hci_uart_tty_close(struct tty_struct *tty=
)
if (hdev)
hci_uart_close(hdev);
=20
+ cancel_work_sync(&hu->write_work);
+
if (test_and_clear_bit(HCI_UART_PROTO_SET, &hu->flags)) {
if (hdev) {
hci_unregister_dev(hdev);
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 6cf6ab22..af93d83 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -66,6 +66,8 @@ struct hci_uart {
unsigned long flags;
unsigned long hdev_flags;
=20
+ struct work_struct write_work;
+
struct hci_uart_proto *proto;
void *priv;
=20
--=20
1.7.10.4