* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Paul Walmsley @ 2012-02-04 2:06 UTC (permalink / raw)
To: NeilBrown
Cc: greg, khilman, govindraj.raja, tomi.valkeinen, linux-serial,
linux-omap, linux-arm-kernel
In-Reply-To: <20120204110131.7378b8fe@notabene.brown>
Hi Neil
On Sat, 4 Feb 2012, NeilBrown wrote:
> Guess what happens if I set autosuspend_delay_ms to 0?
> Massive transmit problems. Driver can hardly get anything out before the
> UART's fclk is cut...
Just reproduced this on 35xx BeagleBoard. Looks like the UART is indeed
going idle while the TX FIFO has bytes in it.
Here's a patch that helps. It seems to work down to an
autosuspend_delay_ms of 1 ms. Without it, the best I can get is 8 ms.
Of course, ideally it should work fine at autosuspend_delay_ms = 0, so
likely there's some other infelicity that we're currently missing.
Neil, care to give this a test and confirm it on your setup?
Will also give the CLOCKACTIVITY bits a quick test.
- Paul
>From 3b8a272e7af23abe472c594da9bce514a0468a80 Mon Sep 17 00:00:00 2001
From: Paul Walmsley <paul@pwsan.com>
Date: Fri, 3 Feb 2012 19:00:03 -0700
Subject: [PATCH] UART idle TX bug test
The UART driver messes around with the SYSCONFIG bits behind the hwmod
code's back. For debugging purposes, prevent the hwmod code from changing
the SYSCONFIG register. That in turn should allow the SIDLEMODE no-idle
setting to persist through the length of the MPU's involvement with the
transmit operation, which it currently does not.
Then, prevent the UART from being put back into no-idle until we get the
TX empty interrupt from the UART. That should ensure that the TX FIFO is
drained before allowing the UART to go into idle.
---
arch/arm/mach-omap2/omap_hwmod.c | 4 ++--
drivers/tty/serial/omap-serial.c | 9 +++------
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 5192cab..bfd7e24 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -980,7 +980,7 @@ static void _enable_sysc(struct omap_hwmod *oh)
v = oh->_sysc_cache;
sf = oh->class->sysc->sysc_flags;
- if (sf & SYSC_HAS_SIDLEMODE) {
+ if (strcmp(oh->name, "uart3") && sf & SYSC_HAS_SIDLEMODE) {
idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
HWMOD_IDLEMODE_NO : HWMOD_IDLEMODE_SMART;
_set_slave_idlemode(oh, idlemode, &v);
@@ -1047,7 +1047,7 @@ static void _idle_sysc(struct omap_hwmod *oh)
v = oh->_sysc_cache;
sf = oh->class->sysc->sysc_flags;
- if (sf & SYSC_HAS_SIDLEMODE) {
+ if (strcmp(oh->name, "uart3") && sf & SYSC_HAS_SIDLEMODE) {
idlemode = (oh->flags & HWMOD_SWSUP_SIDLE) ?
HWMOD_IDLEMODE_FORCE : HWMOD_IDLEMODE_SMART;
_set_slave_idlemode(oh, idlemode, &v);
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 72fa783..fbefcf2 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -159,9 +159,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
serial_out(up, UART_IER, up->ier);
}
- if (!up->use_dma && pdata->set_forceidle)
- pdata->set_forceidle(up->pdev);
-
pm_runtime_mark_last_busy(&up->pdev->dev);
pm_runtime_put_autosuspend(&up->pdev->dev);
}
@@ -251,6 +248,7 @@ ignore_char:
static void transmit_chars(struct uart_omap_port *up)
{
struct circ_buf *xmit = &up->port.state->xmit;
+ struct omap_uart_port_info *pdata = up->pdev->dev.platform_data;
int count;
if (up->port.x_char) {
@@ -261,6 +259,8 @@ static void transmit_chars(struct uart_omap_port *up)
}
if (uart_circ_empty(xmit) || uart_tx_stopped(&up->port)) {
serial_omap_stop_tx(&up->port);
+ if (!up->use_dma && pdata->set_forceidle)
+ pdata->set_forceidle(up->pdev);
return;
}
count = up->port.fifosize / 4;
@@ -274,9 +274,6 @@ static void transmit_chars(struct uart_omap_port *up)
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
uart_write_wakeup(&up->port);
-
- if (uart_circ_empty(xmit))
- serial_omap_stop_tx(&up->port);
}
static inline void serial_omap_enable_ier_thri(struct uart_omap_port *up)
--
1.7.9
^ permalink raw reply related
* RE: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Woodruff, Richard @ 2012-02-04 1:46 UTC (permalink / raw)
To: Paul Walmsley
Cc: Hilman, Kevin, NeilBrown, greg@kroah.com, R, Govindraj,
Valkeinen, Tomi, Grazvydas Ignotas, linux-serial@vger.kernel.org,
linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org
In-Reply-To: <alpine.DEB.2.00.1202031731500.27947@utopia.booyaka.com>
> From: Paul Walmsley [mailto:paul@pwsan.com]
> Sent: Friday, February 03, 2012 7:00 PM
> > One irritation was some internal interrupt sources were not linked to
> > low power wakeup events. If you were in interrupt mode and got
> > characters below watermark you could sleep before interrupt status
> > showed up (as you had to wait several frame times before functional
> > interrupt asserted) but there was no wake at anticipated frame timeout
> > because lack of linking of internal event to wake event.
>
> Indeed, it seems that we are just now working around these wakeup-related
> bugs. Kind of surprising that no errata showed up for them.
There have been errata over time in this area. Several I hit were updated at 3630 time. UART did get IER2 but I don't recall all details for UART. Probably that is not being used.
> What's particularly remarkable is that it looks like the UARTs will
> idle-ack while their transmit FIFOs have data in them (!)
Generally a module can ACK its ICLK if it is not used internally. The FCLK can push data out with out ICLK and is controlled separately always (omap4 changed encoding, to optional clock). This allows interconnect to idle during tx to save power. The trick is to ensure all module wakeup plumbing is enabled so a functional tx irq will flow. Audits last showed several drivers missing steps (omap specific). Some drivers seemed to rely on static dependencies or coincident neighbor activity to allow their functional interrupt to flow... to many interdependent custom details... and yes some errata.
Anyway, even with all SOC specific wake bits you may lose the character with latency of restart. Point I was raising was external chip hook can not be neglected as its part of equation.
Regards,
Richard W.
^ permalink raw reply
* RE: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Paul Walmsley @ 2012-02-04 0:59 UTC (permalink / raw)
To: Woodruff, Richard
Cc: NeilBrown, Grazvydas Ignotas, greg@kroah.com, Hilman, Kevin,
R, Govindraj, Valkeinen, Tomi, linux-serial@vger.kernel.org,
linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org
In-Reply-To: <EF62F09C0797D947AD4180A1043C0DF73492FFC3@DLEE10.ent.ti.com>
On Sat, 4 Feb 2012, Woodruff, Richard wrote:
> When you have aggressive PM working at the SOC level you many times lost
> a character on UART every since OMAP2. A strange but true statement is
> it is nice to see it losing a character on mainline as it as in
> indication that PM is likely working.
We've been losing wakeup characters in mainline for many years now ;-)
> One irritation was some internal interrupt sources were not linked to
> low power wakeup events. If you were in interrupt mode and got
> characters below watermark you could sleep before interrupt status
> showed up (as you had to wait several frame times before functional
> interrupt asserted) but there was no wake at anticipated frame timeout
> because lack of linking of internal event to wake event.
Indeed, it seems that we are just now working around these wakeup-related
bugs. Kind of surprising that no errata showed up for them.
What's particularly remarkable is that it looks like the UARTs will
idle-ack while their transmit FIFOs have data in them (!)
- Paul
^ permalink raw reply
* RE: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Woodruff, Richard @ 2012-02-04 0:23 UTC (permalink / raw)
To: NeilBrown, Grazvydas Ignotas
Cc: Paul Walmsley, Greg KH, greg@kroah.com, Hilman, Kevin,
R, Govindraj, Valkeinen, Tomi, linux-serial@vger.kernel.org,
linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20120203231113.25ae2d3a@notabene.brown>
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of NeilBrown
> > Not sure if it's the same problem but with 3530 on 3.2 with
> > sleep_timeout set, I usually get first char dropped (as expected) but
> > sometimes I get corrupted char instead too. Corrupt char seems to
> > almost always happen if I set cpufreq to powersave, on performace it's
> > almost always ok, so maybe it's some timing problem,
>
> I see that too - I'm glad someone else does :-)
When you have aggressive PM working at the SOC level you many times lost a character on UART every since OMAP2. A strange but true statement is it is nice to see it losing a character on mainline as it as in indication that PM is likely working.
If you just hook up simple RX and TX lines and not other flow control it is very likely especially with older OMAPs you can lose the 'wake' character on debug console. The UART operates on a derived clock from a 96MHz DPLL which was probably stopped. When the wakeup event hits the IO ring many internals may need to repower and its source DPLL needs to relock. This all can take a while and you can lose the start bit at high baud rate. If you use flow control you might be able to get ahead of it.
As the silicon process has shrunk from 90nm (omap2) to 65nm (omap3) to 45nm (omap4) the DPLL relock times have dropped a lot. With certain DPLL parameters it could take hundreds of uS to relock in OMAP2. And there are more variables to latency stack up than just DPLL. Most of these have improved (gotten smaller) over time.
Always the hack for debug console was activity timer along with denying idle while SW and HW TX FIFOs had characters in them. This made debug console useable.
One irritation was some internal interrupt sources were not linked to low power wakeup events. If you were in interrupt mode and got characters below watermark you could sleep before interrupt status showed up (as you had to wait several frame times before functional interrupt asserted) but there was no wake at anticipated frame timeout because lack of linking of internal event to wake event.
Outside of debug console, this loss has not been huge. Protocols like irda would retransmit their magic wake packets. You can move between DMA and interrupt modes with activity. So far there has been a work around per attached device.
If your screen blanks and you hit a char to wakeup, do you expect to see the character or do you throw it away? You can set some timeout or policy to deny lower c-states which can ensure a debug console doesn't have any issue. If your application is a phone it is not likely something you would do.
Maybe your issue today is due to some other software bug... but at the end of the trail you still may have an issue unless your attached hardware accommodates.
Regards,
Richard W.
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: NeilBrown @ 2012-02-04 0:01 UTC (permalink / raw)
To: Paul Walmsley
Cc: greg, khilman, govindraj.raja, tomi.valkeinen, linux-serial,
linux-omap, linux-arm-kernel
In-Reply-To: <alpine.DEB.2.00.1202031532540.27947@utopia.booyaka.com>
[-- Attachment #1: Type: text/plain, Size: 3372 bytes --]
On Fri, 3 Feb 2012 16:02:42 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
> On Sat, 4 Feb 2012, NeilBrown wrote:
>
> > On Fri, 3 Feb 2012 13:10:28 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
> >
> > > Considering your theory that the UART clocks are being cut while there's
> > > still data in the FIFO, you might consider removing this code at the end
> > > of transmit_chars():
> > >
> > > if (uart_circ_empty(xmit))
> > > serial_omap_stop_tx(&up->port);
> >
> > I read the code and chickened out of just removing that.
> > serial_omap_stop_tx seem to do 2 things:
> > 1/ tell the uart to stop sending interrupts when the tx fifo is empty
> > 2/ set forceidle (really smartidle) on the uart.
> >
> > I didn't feel comfortable removing '1' as I thought it might generate an
> > interrupt storm .. maybe not.
>
> Might be worth a try. In theory, since the current UART driver sets the
> TX_EMPTY flag in the SCR register, the UART should only raise a TX
> interrupt when the FIFO + shift register are totally empty. So hopefully
> you should only get one extra interrupt per TTY transmit operation.
>
> > Instead I just removed '2'. In fact I replaced the 'set_forceidle' call with
> > 'set_noidle'. So the uart should never report that it was idle.
> >
> > I did this with my other patch removed so pm_runtime_put() was still being
> > called.
> >
> > Result: I still get corruption.
> > So having the UART say "no, I'm not idle" does *not* stop the clock
> > being turned off when we use omap_hwmod_idle() to turn off the clocks.
>
> Hmm that's doubtful. If that's really so, then we should be seeing
> massive UART transmit problems. I'd expect that the driver wouldn't be
> able to get any transmit buffers out the door at all before the UART's
> fclk is cut.
Guess what happens if I set autosuspend_delay_ms to 0?
Massive transmit problems. Driver can hardly get anything out before the
UART's fclk is cut...
>
> What's probably happening in this case is that the hwmod code is rewriting
> the UART SIDLEMODE bits in the hwmod code's _idle() function. This gets
> called as part of the PM runtime suspend operation. So it's bypassing
> your debugging hack :-) The hwmod code expects to control the SYSCONFIG
> register bits itself, and the current way that the UART driver messes with
> the SYSCONFIG bits is a total hack that that hwmod code is not expecting.
> You could try disabling that behavior in _idle_sysc() by adding a hack to
> skip it if it's the UART3 hwmod.
>
> > When we turn off a clock, if that is the last clock in the clock-domain, we
> > also turn off the clock-domain (I think).
>
> That's only true if the clockdomain is programmed to use
> software-supervised idle. CORE & PER should both be programmed to
> hardware-supervised idle by mach-omap2/pm34xx.c. In that case, we let the
> PRCM put the clockdomain to sleep by itself.
>
> > Could it be that the clock-domain doesn't do any handshaking with modules,
> > and so turns off the clocks even though they are being used?
>
> Probably not -- I'd think that hardware-supervised idle wouldn't work at
> all if that were true.
Thanks for those hints. Next time I dive into the code/doco they might help
me understand a bit more.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Paul Walmsley @ 2012-02-03 23:02 UTC (permalink / raw)
To: NeilBrown
Cc: greg, khilman, govindraj.raja, tomi.valkeinen, linux-serial,
linux-omap, linux-arm-kernel
In-Reply-To: <20120204085940.2de44594@notabene.brown>
On Sat, 4 Feb 2012, NeilBrown wrote:
> On Fri, 3 Feb 2012 13:10:28 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
>
> > Considering your theory that the UART clocks are being cut while there's
> > still data in the FIFO, you might consider removing this code at the end
> > of transmit_chars():
> >
> > if (uart_circ_empty(xmit))
> > serial_omap_stop_tx(&up->port);
>
> I read the code and chickened out of just removing that.
> serial_omap_stop_tx seem to do 2 things:
> 1/ tell the uart to stop sending interrupts when the tx fifo is empty
> 2/ set forceidle (really smartidle) on the uart.
>
> I didn't feel comfortable removing '1' as I thought it might generate an
> interrupt storm .. maybe not.
Might be worth a try. In theory, since the current UART driver sets the
TX_EMPTY flag in the SCR register, the UART should only raise a TX
interrupt when the FIFO + shift register are totally empty. So hopefully
you should only get one extra interrupt per TTY transmit operation.
> Instead I just removed '2'. In fact I replaced the 'set_forceidle' call with
> 'set_noidle'. So the uart should never report that it was idle.
>
> I did this with my other patch removed so pm_runtime_put() was still being
> called.
>
> Result: I still get corruption.
> So having the UART say "no, I'm not idle" does *not* stop the clock
> being turned off when we use omap_hwmod_idle() to turn off the clocks.
Hmm that's doubtful. If that's really so, then we should be seeing
massive UART transmit problems. I'd expect that the driver wouldn't be
able to get any transmit buffers out the door at all before the UART's
fclk is cut.
What's probably happening in this case is that the hwmod code is rewriting
the UART SIDLEMODE bits in the hwmod code's _idle() function. This gets
called as part of the PM runtime suspend operation. So it's bypassing
your debugging hack :-) The hwmod code expects to control the SYSCONFIG
register bits itself, and the current way that the UART driver messes with
the SYSCONFIG bits is a total hack that that hwmod code is not expecting.
You could try disabling that behavior in _idle_sysc() by adding a hack to
skip it if it's the UART3 hwmod.
> When we turn off a clock, if that is the last clock in the clock-domain, we
> also turn off the clock-domain (I think).
That's only true if the clockdomain is programmed to use
software-supervised idle. CORE & PER should both be programmed to
hardware-supervised idle by mach-omap2/pm34xx.c. In that case, we let the
PRCM put the clockdomain to sleep by itself.
> Could it be that the clock-domain doesn't do any handshaking with modules,
> and so turns off the clocks even though they are being used?
Probably not -- I'd think that hardware-supervised idle wouldn't work at
all if that were true.
- Paul
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Paul Walmsley @ 2012-02-03 22:30 UTC (permalink / raw)
To: NeilBrown
Cc: Grazvydas Ignotas, Greg KH, greg, khilman, govindraj.raja,
tomi.valkeinen, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <20120204091048.00c7e027@notabene.brown>
On Sat, 4 Feb 2012, NeilBrown wrote:
> On Fri, 3 Feb 2012 14:42:09 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
>
> > On Fri, 3 Feb 2012, Paul Walmsley wrote:
> >
> > > On Fri, 3 Feb 2012, NeilBrown wrote:
> > >
> > > > My theory is that there is a delay between the falling RX line waking the
> > > > system up, and the CPU enabling the UART - whether enabling the clocks or
> > > > doing a full config, I am not sure - though I think the former.
> > > >
> > > > Maybe if we could enable the UART clocks immediately after returning from the
> > > > WFI instruction we could avoid the corruption....
> > >
> > > The PRCM should be re-enabling the UART's functional clock itself, with no
> > > kernel involvement. The sequence should go something like this
> > > (simplified):
> > >
> > > 1. I/O wakeup occurs
> > >
> > > 2. CORE & PER powerdomains are awakened
> > >
> > > 3. The UART notices an event on its input lines and deasserts its idle-ack
> >
> > It just occurred to me that, supposedly, the only UART input line that is
> > attached to the SWAKEUP signal is CTS. So the UART may not in fact be
> > able to deassert its idle-ack autonomously at this point.
>
> How does that relate to the RX_CTS_WU_EN bit which enables an interrupt on
> "a falling edge of pins RX, nCTS, or nDSR"
That's the bit I'm talking about :-) Maybe it might work appropriately,
then, if it also tests RX. Section 19.3.2.3 "Wake-up Request" only
mentions the CTS lines. Flip a coin ;-)
> This seems to be a "wakeup interrupt", bit it isn't clear what it wakes us.
The UART. It should send an SWAKEUP to the PRCM and bring the UART out of
idle-ack.
- Paul
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: NeilBrown @ 2012-02-03 22:10 UTC (permalink / raw)
To: Paul Walmsley
Cc: Grazvydas Ignotas, Greg KH, greg, khilman, govindraj.raja,
tomi.valkeinen, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <alpine.DEB.2.00.1202031432080.27947@utopia.booyaka.com>
[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]
On Fri, 3 Feb 2012 14:42:09 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
> One correction on this part...
>
> On Fri, 3 Feb 2012, Paul Walmsley wrote:
>
> > On Fri, 3 Feb 2012, NeilBrown wrote:
> >
> > > My theory is that there is a delay between the falling RX line waking the
> > > system up, and the CPU enabling the UART - whether enabling the clocks or
> > > doing a full config, I am not sure - though I think the former.
> > >
> > > Maybe if we could enable the UART clocks immediately after returning from the
> > > WFI instruction we could avoid the corruption....
> >
> > The PRCM should be re-enabling the UART's functional clock itself, with no
> > kernel involvement. The sequence should go something like this
> > (simplified):
> >
> > 1. I/O wakeup occurs
> >
> > 2. CORE & PER powerdomains are awakened
> >
> > 3. The UART notices an event on its input lines and deasserts its idle-ack
>
> It just occurred to me that, supposedly, the only UART input line that is
> attached to the SWAKEUP signal is CTS. So the UART may not in fact be
> able to deassert its idle-ack autonomously at this point.
How does that relate to the RX_CTS_WU_EN bit which enables an interrupt on
"a falling edge of pins RX, nCTS, or nDSR"
This seems to be a "wakeup interrupt", bit it isn't clear what it wakes us.
>
> So you might want to give your clock re-enable after WFI idea a shot! It
> would be interesting if it helps.
Might be a bit beyond me at the moment :-(
Thanks,
NeilBrown
>
> I regret the oversight,
>
>
> - Paul
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: NeilBrown @ 2012-02-03 21:59 UTC (permalink / raw)
To: Paul Walmsley
Cc: greg, khilman, govindraj.raja, tomi.valkeinen, linux-serial,
linux-omap, linux-arm-kernel
In-Reply-To: <alpine.DEB.2.00.1202031255330.27947@utopia.booyaka.com>
[-- Attachment #1: Type: text/plain, Size: 2514 bytes --]
On Fri, 3 Feb 2012 13:10:28 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
> One other comment..
>
> On Fri, 3 Feb 2012, NeilBrown wrote:
>
> > On Thu, 2 Feb 2012 22:45:53 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
> >
> > > On Fri, 3 Feb 2012, NeilBrown wrote:
> > >
> > > > I can remove this effect with:
> > > >
> > > > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> > > > index f809041..c7ef760 100644
> > > > --- a/drivers/tty/serial/omap-serial.c
> > > > +++ b/drivers/tty/serial/omap-serial.c
> > > > @@ -440,7 +440,8 @@ static unsigned int serial_omap_tx_empty(struct uart_port *port)
> > > > spin_lock_irqsave(&up->port.lock, flags);
> > > > ret = serial_in(up, UART_LSR) & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
> > > > spin_unlock_irqrestore(&up->port.lock, flags);
> > > > - pm_runtime_put(&up->pdev->dev);
> > > > + pm_runtime_mark_last_busy(&up->pdev->dev);
> > > > + pm_runtime_put_autosuspend(&up->pdev->dev);
> > > > return ret;
> > > > }
>
> It's a little surprising that this helps. The pm_runtime_get*() and
> _put*() in serial_omap_tx_empty() are just intended to ensure that the
> UART's clocks are running for that LSR register read.
>
> Considering your theory that the UART clocks are being cut while there's
> still data in the FIFO, you might consider removing this code at the end
> of transmit_chars():
>
> if (uart_circ_empty(xmit))
> serial_omap_stop_tx(&up->port);
I read the code and chickened out of just removing that.
serial_omap_stop_tx seem to do 2 things:
1/ tell the uart to stop sending interrupts when the tx fifo is empty
2/ set forceidle (really smartidle) on the uart.
I didn't feel comfortable removing '1' as I thought it might generate an
interrupt storm .. maybe not.
Instead I just removed '2'. In fact I replaced the 'set_forceidle' call with
'set_noidle'. So the uart should never report that it was idle.
I did this with my other patch removed so pm_runtime_put() was still being
called.
Result: I still get corruption.
So having the UART say "no, I'm not idle" does *not* stop the clock
being turned off when we use omap_hwmod_idle() to turn off the clocks.
When we turn off a clock, if that is the last clock in the clock-domain, we
also turn off the clock-domain (I think).
Could it be that the clock-domain doesn't do any handshaking with modules,
and so turns off the clocks even though they are being used?
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Paul Walmsley @ 2012-02-03 21:42 UTC (permalink / raw)
To: NeilBrown
Cc: Grazvydas Ignotas, Greg KH, greg, khilman, govindraj.raja,
tomi.valkeinen, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <alpine.DEB.2.00.1202031243270.27947@utopia.booyaka.com>
One correction on this part...
On Fri, 3 Feb 2012, Paul Walmsley wrote:
> On Fri, 3 Feb 2012, NeilBrown wrote:
>
> > My theory is that there is a delay between the falling RX line waking the
> > system up, and the CPU enabling the UART - whether enabling the clocks or
> > doing a full config, I am not sure - though I think the former.
> >
> > Maybe if we could enable the UART clocks immediately after returning from the
> > WFI instruction we could avoid the corruption....
>
> The PRCM should be re-enabling the UART's functional clock itself, with no
> kernel involvement. The sequence should go something like this
> (simplified):
>
> 1. I/O wakeup occurs
>
> 2. CORE & PER powerdomains are awakened
>
> 3. The UART notices an event on its input lines and deasserts its idle-ack
It just occurred to me that, supposedly, the only UART input line that is
attached to the SWAKEUP signal is CTS. So the UART may not in fact be
able to deassert its idle-ack autonomously at this point.
So you might want to give your clock re-enable after WFI idea a shot! It
would be interesting if it helps.
I regret the oversight,
- Paul
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Paul Walmsley @ 2012-02-03 21:04 UTC (permalink / raw)
To: NeilBrown
Cc: Grazvydas Ignotas, Greg KH, greg, khilman, govindraj.raja,
tomi.valkeinen, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <20120204074427.5091fef5@notabene.brown>
On Sat, 4 Feb 2012, NeilBrown wrote:
> On Fri, 3 Feb 2012 12:42:22 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
>
> > Case 1 is expected and is almost certainly not a bug. As Neil mentioned
> > it should be bps-rate dependent. It occurs when the first character
> > transmitted to the OMAP wakes the chip up via I/O ring/chain wakeup.
> > I/O ring/chain wakeup is driven by a 32KiHz clock and is therefore
> > relatively high-latency. So this could easily cause the first character
> > or first few characters to be lost or corrupted, depending on the exact
> > sequence of events, the low power state that the chip was in, etc.
>
> A 32KiHz cycles every 30mSec.
> At 115200 bps, there is one bit every 8.7 microseconds.
>
> When I type "return" - which looks like 0101100001111... on the wire,
> I see '0xC3' which looks like 011000011111... on the wire.
> So we lost exactly 2 bits, or a delay around 17 microseconds.
>
> I find it hard to reconcile that delay with the cause being a 32KiHZ clock.
If you're not disabling the HF clock oscillator via the AUTOEXTCLKMODE
bits, the wakeup logic may be getting clocked by the sys_clk, which is
quite a bit faster. That might explain the corruption patterns you're
seeing.
- Paul
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: NeilBrown @ 2012-02-03 20:44 UTC (permalink / raw)
To: Paul Walmsley
Cc: Grazvydas Ignotas, Greg KH, greg, khilman, govindraj.raja,
tomi.valkeinen, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <alpine.DEB.2.00.1202031234120.27947@utopia.booyaka.com>
[-- Attachment #1: Type: text/plain, Size: 3575 bytes --]
On Fri, 3 Feb 2012 12:42:22 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
> On Fri, 3 Feb 2012, Grazvydas Ignotas wrote:
>
> > On Fri, Feb 3, 2012 at 11:54 AM, NeilBrown <neilb@suse.de> wrote:
> > > On Thu, 2 Feb 2012 22:45:53 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
> > >> On Fri, 3 Feb 2012, NeilBrown wrote:
> > >>
> > >> > then CPUIDLE enters lower states and I think it uses less power but I
> > >> > sometimes lose the first char I type (that is known) but also I sometimes get
> > >> > corruption on output.
> > >>
> > >> I don't see any output corruption on 35xx BeagleBoard, either with or
> > >> without these patches. So this is presumably a 37xx-centric problem, and
> > >> unrelated to these patches, I would guess.
> > >
> > > Maybe it is 37xx specific. I think this is a DM3730.
> >
> > Not sure if it's the same problem but with 3530 on 3.2 with
> > sleep_timeout set, I usually get first char dropped (as expected) but
> > sometimes I get corrupted char instead too. Corrupt char seems to
> > almost always happen if I set cpufreq to powersave, on performace it's
> > almost always ok, so maybe it's some timing problem,
>
> OK so let's distinguish between two corruption situations:
>
> 1. The first character transmitted to the OMAP UART in a serial console
> when the UART powerdomain is in a non-functional, low power state (e.g.,
> RET or below) is corrupted. This is not actually output corruption, this
> is input corruption.
>
> 2. Characters are corrupted while the OMAP UART is transmitting data, but
> there has been no recent data sent to the OMAP.
>
> Case 1 is expected and is almost certainly not a bug. As Neil mentioned
> it should be bps-rate dependent. It occurs when the first character
> transmitted to the OMAP wakes the chip up via I/O ring/chain wakeup.
> I/O ring/chain wakeup is driven by a 32KiHz clock and is therefore
> relatively high-latency. So this could easily cause the first character
> or first few characters to be lost or corrupted, depending on the exact
> sequence of events, the low power state that the chip was in, etc.
A 32KiHz cycles every 30mSec.
At 115200 bps, there is one bit every 8.7 microseconds.
When I type "return" - which looks like 0101100001111... on the wire,
I see '0xC3' which looks like 011000011111... on the wire.
So we lost exactly 2 bits, or a delay around 17 microseconds.
I find it hard to reconcile that delay with the cause being a 32KiHZ clock.
If the first char I type is a space (0x20 or 0000001001111111) then
the character received is 0x90 (0000010011111) which is exactly 1 bit missing,
so an 8 or 9 usec delay.
If the first char I type is 'o' (0x6f, or 0111101101111111) then the character
received is 0xfb (01101111111111) which misses 5 bits.
I think it misses the first bit, then waits for a start bit (0), then takes
the next 8 bits.
At 230400 bps, I always lose at least 2 bits.
At 460800 bps, I seem lose at least 3 bits.
(above there, nothing works at all ... could be my USB/serial cable at fault).
So it looks a lot like a delay which is a small number of microseconds.
Could be the wake-up latency in the I/O ring/chain, but doesn't look like the
32 KiHz clock :-)
>
> Case 2 is not expected. That is likely a bug somewhere. Neil, this is
> what I understood that you are experiencing. Is that correct?
Correct.
Thanks,
NeilBrown
>
> Gražvydas, are you seeing case 1 or 2 (or something completely different
> ;-) ?
>
>
> - Paul
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Paul Walmsley @ 2012-02-03 20:34 UTC (permalink / raw)
To: NeilBrown
Cc: Grazvydas Ignotas, Greg KH, greg, khilman, govindraj.raja,
tomi.valkeinen, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <alpine.DEB.2.00.1202031243270.27947@utopia.booyaka.com>
On Fri, 3 Feb 2012, Paul Walmsley wrote:
> 2. CORE & PER powerdomains are awakened
Incidentally, there might be a missing clkdm_add_wakeup() in
mach-omap2/pm34xx.c to wake up PER when CORE is awakened. For people who
are seeing some input character corruption when CORE/PER enters a
low-power state, that might be worth experimenting with.
- Paul
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Paul Walmsley @ 2012-02-03 20:10 UTC (permalink / raw)
To: NeilBrown
Cc: greg, khilman, govindraj.raja, tomi.valkeinen, linux-serial,
linux-omap, linux-arm-kernel
In-Reply-To: <20120203205401.5ddf241d@notabene.brown>
One other comment..
On Fri, 3 Feb 2012, NeilBrown wrote:
> On Thu, 2 Feb 2012 22:45:53 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
>
> > On Fri, 3 Feb 2012, NeilBrown wrote:
> >
> > > I can remove this effect with:
> > >
> > > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> > > index f809041..c7ef760 100644
> > > --- a/drivers/tty/serial/omap-serial.c
> > > +++ b/drivers/tty/serial/omap-serial.c
> > > @@ -440,7 +440,8 @@ static unsigned int serial_omap_tx_empty(struct uart_port *port)
> > > spin_lock_irqsave(&up->port.lock, flags);
> > > ret = serial_in(up, UART_LSR) & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
> > > spin_unlock_irqrestore(&up->port.lock, flags);
> > > - pm_runtime_put(&up->pdev->dev);
> > > + pm_runtime_mark_last_busy(&up->pdev->dev);
> > > + pm_runtime_put_autosuspend(&up->pdev->dev);
> > > return ret;
> > > }
It's a little surprising that this helps. The pm_runtime_get*() and
_put*() in serial_omap_tx_empty() are just intended to ensure that the
UART's clocks are running for that LSR register read.
Considering your theory that the UART clocks are being cut while there's
still data in the FIFO, you might consider removing this code at the end
of transmit_chars():
if (uart_circ_empty(xmit))
serial_omap_stop_tx(&up->port);
- Paul
^ permalink raw reply
* Re: [Patch] tty: fix a build failure on sparc
From: Paul Gortmaker @ 2012-02-03 16:06 UTC (permalink / raw)
To: Cong Wang
Cc: linux-kernel, Andrew Morton, Greg Kroah-Hartman, Alan Cox,
Jamie Iles, Stephen Warren, linux-serial
In-Reply-To: <1328246540-5824-1-git-send-email-xiyou.wangcong@gmail.com>
On 12-02-03 12:22 AM, Cong Wang wrote:
> On sparc, there is a build failure:
>
> drivers/tty/serial/8250/8250.c:48:21: error: suncore.h: No such file or directory
> drivers/tty/serial/8250/8250.c:3275: error: implicit declaration of function 'sunserial_register_minors'
> drivers/tty/serial/8250/8250.c:3305: error: implicit declaration of function 'sunserial_unregister_minors'
>
> this is due to commit 9bef3d4197379a995fa80f81950bbbf8d32e9e8b
> (serial: group all the 8250 related code together) moved these files
> into 8250/ subdirectory, but forgot to change the reference
> to drivers/tty/serial/suncore.h.
Thanks, I explicitly left sunsu out of the move but missed
that 8250 was sourcing its related header. I did defconfig
builds, but in going back to check, I see that neither sparc
32/64 defconfig enable 8250 by default, so those builds didn't
help me much at all... :(
Acked-by: Paul Gortmaker <paul.gortmaker@windriver.com>
>
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
>
> ---
> diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> index 9f50c4e..9b7336f 100644
> --- a/drivers/tty/serial/8250/8250.c
> +++ b/drivers/tty/serial/8250/8250.c
> @@ -45,7 +45,7 @@
> #include "8250.h"
>
> #ifdef CONFIG_SPARC
> -#include "suncore.h"
> +#include "../suncore.h"
> #endif
>
> /*
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Paul Walmsley @ 2012-02-03 19:54 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: NeilBrown, Govindraj, khilman, greg, govindraj.raja,
tomi.valkeinen, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <20120203122053.GF1275@n2100.arm.linux.org.uk>
On Fri, 3 Feb 2012, Russell King - ARM Linux wrote:
> If there's an on-going transmission, why is the runtime PM count zero,
> meaning that the UART can sleep at the point where serial_omap_tx_empty()
> is being called - and obviously there's still characters in the FIFO?
In the case of OMAP, that's the point of the idle protocol. If there's
data left in the TX FIFO, the UART should prevent the rest of the chip
from cutting its clocks until the TX FIFO is drained. Even if the MPU has
requested that the UART clocks be cut.
Of course, there may well be a bug that prevents this from working the
way it was intended.
- Paul
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Paul Walmsley @ 2012-02-03 19:49 UTC (permalink / raw)
To: NeilBrown
Cc: Grazvydas Ignotas, Greg KH, greg, khilman, govindraj.raja,
tomi.valkeinen, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <20120203231113.25ae2d3a@notabene.brown>
On Fri, 3 Feb 2012, NeilBrown wrote:
> My theory is that there is a delay between the falling RX line waking the
> system up, and the CPU enabling the UART - whether enabling the clocks or
> doing a full config, I am not sure - though I think the former.
>
> Maybe if we could enable the UART clocks immediately after returning from the
> WFI instruction we could avoid the corruption....
The PRCM should be re-enabling the UART's functional clock itself, with no
kernel involvement. The sequence should go something like this
(simplified):
1. I/O wakeup occurs
2. CORE & PER powerdomains are awakened
3. The UART notices an event on its input lines and deasserts its idle-ack
4. The PRCM re-enables the clocks to the UART
5. The UART receives the input character into its FIFO and raises an MPU
interrupt
That's the theory, anyway.
- Paul
^ permalink raw reply
* Re: [Patch] tty: fix a build failure on sparc
From: Paul Gortmaker @ 2012-02-03 19:46 UTC (permalink / raw)
To: Cong Wang
Cc: linux-kernel, Andrew Morton, Greg Kroah-Hartman, Alan Cox,
Jamie Iles, Stephen Warren, linux-serial
In-Reply-To: <1328246540-5824-1-git-send-email-xiyou.wangcong@gmail.com>
[Resending with updated address for Greg]
On 12-02-03 12:22 AM, Cong Wang wrote:
> On sparc, there is a build failure:
>
> drivers/tty/serial/8250/8250.c:48:21: error: suncore.h: No such file or directory
> drivers/tty/serial/8250/8250.c:3275: error: implicit declaration of function 'sunserial_register_minors'
> drivers/tty/serial/8250/8250.c:3305: error: implicit declaration of function 'sunserial_unregister_minors'
>
> this is due to commit 9bef3d4197379a995fa80f81950bbbf8d32e9e8b
> (serial: group all the 8250 related code together) moved these files
> into 8250/ subdirectory, but forgot to change the reference
> to drivers/tty/serial/suncore.h.
Thanks, I explicitly left sunsu out of the move but missed
that 8250 was sourcing its related header. I did defconfig
builds, but in going back to check, I see that neither sparc
32/64 defconfig enable 8250 by default, so those builds didn't
help me much at all... :(
Acked-by: Paul Gortmaker <paul.gortmaker@windriver.com>
>
> Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
>
> ---
> diff --git a/drivers/tty/serial/8250/8250.c b/drivers/tty/serial/8250/8250.c
> index 9f50c4e..9b7336f 100644
> --- a/drivers/tty/serial/8250/8250.c
> +++ b/drivers/tty/serial/8250/8250.c
> @@ -45,7 +45,7 @@
> #include "8250.h"
>
> #ifdef CONFIG_SPARC
> -#include "suncore.h"
> +#include "../suncore.h"
> #endif
>
> /*
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Paul Walmsley @ 2012-02-03 19:42 UTC (permalink / raw)
To: Grazvydas Ignotas
Cc: NeilBrown, Greg KH, greg, khilman, govindraj.raja, tomi.valkeinen,
linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <CANOLnONsZKHJPbEx2+ic3EZ1PqcD30VT4Z2bR5iW-JNQJQL0GA@mail.gmail.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2236 bytes --]
On Fri, 3 Feb 2012, Grazvydas Ignotas wrote:
> On Fri, Feb 3, 2012 at 11:54 AM, NeilBrown <neilb@suse.de> wrote:
> > On Thu, 2 Feb 2012 22:45:53 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
> >> On Fri, 3 Feb 2012, NeilBrown wrote:
> >>
> >> > then CPUIDLE enters lower states and I think it uses less power but I
> >> > sometimes lose the first char I type (that is known) but also I sometimes get
> >> > corruption on output.
> >>
> >> I don't see any output corruption on 35xx BeagleBoard, either with or
> >> without these patches. So this is presumably a 37xx-centric problem, and
> >> unrelated to these patches, I would guess.
> >
> > Maybe it is 37xx specific. I think this is a DM3730.
>
> Not sure if it's the same problem but with 3530 on 3.2 with
> sleep_timeout set, I usually get first char dropped (as expected) but
> sometimes I get corrupted char instead too. Corrupt char seems to
> almost always happen if I set cpufreq to powersave, on performace it's
> almost always ok, so maybe it's some timing problem,
OK so let's distinguish between two corruption situations:
1. The first character transmitted to the OMAP UART in a serial console
when the UART powerdomain is in a non-functional, low power state (e.g.,
RET or below) is corrupted. This is not actually output corruption, this
is input corruption.
2. Characters are corrupted while the OMAP UART is transmitting data, but
there has been no recent data sent to the OMAP.
Case 1 is expected and is almost certainly not a bug. As Neil mentioned
it should be bps-rate dependent. It occurs when the first character
transmitted to the OMAP wakes the chip up via I/O ring/chain wakeup.
I/O ring/chain wakeup is driven by a 32KiHz clock and is therefore
relatively high-latency. So this could easily cause the first character
or first few characters to be lost or corrupted, depending on the exact
sequence of events, the low power state that the chip was in, etc.
Case 2 is not expected. That is likely a bug somewhere. Neil, this is
what I understood that you are experiencing. Is that correct?
Gražvydas, are you seeing case 1 or 2 (or something completely different
;-) ?
- Paul
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Paul Walmsley @ 2012-02-03 19:34 UTC (permalink / raw)
To: NeilBrown
Cc: Greg KH, greg, khilman, govindraj.raja, tomi.valkeinen,
linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <20120203205401.5ddf241d@notabene.brown>
On Fri, 3 Feb 2012, NeilBrown wrote:
> On Thu, 2 Feb 2012 22:45:53 -0700 (MST) Paul Walmsley <paul@pwsan.com> wrote:
>
> > On Fri, 3 Feb 2012, NeilBrown wrote:
> >
> > > If I enable runtime pm by setting power/autosuspend_delay_ms (e.g. to 5000)
> >
> > Runtime PM should be enabled even with power/autosuspend_delay_ms = 0.
> > I think you are simply enabling the autosuspend timer. There was a
> > significant behavior change here from 3.2, I believe.
>
> However the default autosuspend_delay_ms is "-1", not "0", and "-1" does
> disable runbtime_pm completely. It increments usage_count (see
> update_autosuspend()) so runtime_pm can never suspend the device.
OK good, so you are indeed keeping the clocks enabled, then.
> Hmm... I thought it was the other way around - CLKEN could gate the clock
> off, and PRCM could also turn it off after a handshake. But I finally found
> the relevant text:
>
> The software effect is immediate and direct. The functional clock is
> turned on as soon as the bit is set, and turned off if the bit is cleared
> and the clock is not required by any module.
>
> so it seems I was wrong.
Well, one shouldn't take the TRM too seriously. But in this case, yes I
think you had a slightly different idea than what the hardware people
intended ;-)
> Still - something is definitely going wrong because I definitely an regularly
> see garbage characters. And the patch fixes it. So some runtime-suspend
> handler must be doing something bad, and it must happen while characters
> are being written.
It's certainly possible that there is another idle bug in the UART where
it could be mistakenly idle-acking when there are bytes left to be
transmitted. But the patch 'tty: serial: OMAP: block idle while the UART
is transferring data in PIO mode' should fix that. Are you running with
those patches applied? Also, are you using PIO or DMA?
> > > i.e. when the tx buffer is empty, so turn the clocks off immediately,
> > > but wait a while for the fifo to empty. Hopefully the auto-suspend time
> > > is enough.
> >
> > Hmm, this statement is a little unclear to me. The clocks won't be turned
> > off immediately - the request to disable the clocks only happens when the
> > autosuspend timer expires. And even then, as mentioned above, it's just a
> > request. The hardware should not actually disable the functional clock
> > until the UART FIFO is drained...
>
> If you pm_runtime_put_autosuspend(), the suspend won't happen immediately but
> will wait the timeout.
> If you pm_runtime_put_sync(), the suspend happens straight away.
> If you pm_runtime_put(), the suspend is schedule immediately in another
> thread, so it happens very soon. It doesn't wait for a timer to expire (no
> timer is ticking at this point).
>
> Just because an autosuspend timeout was set earlier, that won't cause
> pm_runtime_put() to delay in suspending the device when it is called.
>
> So I think it does request that the clocks be turned off immediately.
I was under the impression you were referring to the behavior after your
patch was applied, rather than before it. My misunderstanding.
> > Anyway, consider trying a different CLOCKACTIVITY setting for the UARTs.
>
> My TRM saids that SYSC for the UARTs doesn't have CLOCKACTIVITY. Only
> AUTOIDLE SOFTRESET ENAWAKEUP IDLEMODE
>
> Is it worth trying anyway?
Sure, why not. It's fast to try, and if it happens to work, it's better
than hacking the driver..
> > Incidentally, I have some patches to fix the latency calculation here that
> > are in the works, similar to the ones you describe. The current
> > calculation in the driver is pretty broken, but since the changes to fix
> > the calculation are rather involved, Kevin and I thought it would be best
> > to defer most of them to the v3.4 merge window. The calculation fix in
> > the 3.3-rc series is simply intended to deal with a very basic power
> > management regression, as you know - not to make it ideal, which is more
> > complicated. Anyway, the patches are at git://git.pwsan.com/linux-2.6 in
> > the branch 'omap_serial_fix_pm_qos_formula_devel_3.4'. Keep in mind that
> > at least one patch in that branch is broken, but perhaps you might get an
> > idea of where they are trying to go. Comments welcome.
> >
> > > However I am using a lot more power than in 3.2.
> >
> > Is this without disabling the UART clocks? If the driver is keeping the
> > UART clocks enabled, then increased power consumption is definitely
> > expected.
>
> Both. With clocks kept on (autosuspend == -1) I'm using about 30 mA more than
> before. With clocks allowed to go off it is closer to 40mA more !!! Weird,
> isn't it?
Interesting. A few questions. Do you have the PMIC and the OMAP
configured to scale voltage in retention? Also, does the power effect
differ if you use different autosuspend timeouts?
> > > If I then enabled runtime_pm with a 5 second autosuspend delay:
> > > CORE is still permanently ON (I think the USB might be doing that).
> > > PER is OFF (7 seconds) RET (15 seconds) and ON (8 seconds).
> > > but more surprising
> > > MPU is OFF (8 sec) RET (8 sec) INA (9 sec) ON (4 secs).
> > >
> > > So we get to turn PER off at the cost of turning the MPU on a lot.
> > > (the periods in each state are only repeatable to a precision of 20%-50%).
Hmmm yeah, that does seem weird that the MPU would stay out of a low power
state just because the autosuspend timer was enabled.
> > > p.s. who should I formally submit OMAP-UART patches to? I have a couple of
> > > others such as the below that I should submit somewhere.
> >
> > Would suggest sending them to linux-serial@vger.kernel.org,
> > linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, and Alan
> > Cox since he is the serial maintainer? And you should probably also cc
> > me, since I seem to be stuck with fixing this driver so I can test my
> > other patches; and cc Govindraj as well.
>
> Thanks for your time.
Thanks for helping us clean up this driver :-)
By the way, you might want to cc Kevin Hilman on any serial patches too,
since he has been working in this area also.
- Paul
^ permalink raw reply
* Re: [PATCH 1/7 v2] dmaengine: add a simple dma library
From: Guennadi Liakhovetski @ 2012-02-03 16:38 UTC (permalink / raw)
To: Vinod Koul
Cc: linux-kernel, linux-sh, Magnus Damm, Yoshihiro Shimoda, linux-mmc,
alsa-devel, linux-serial, Paul Mundt
In-Reply-To: <1328074726.1610.1.camel@vkoul-udesk3>
Hi Vinod
On Wed, 1 Feb 2012, Vinod Koul wrote:
> On Tue, 2012-01-31 at 09:59 +0100, Guennadi Liakhovetski wrote:
> > Hi Vinod
> >
> > Thanks for your reply.
> >
> > On Tue, 31 Jan 2012, Vinod Koul wrote:
> >
> > > On Mon, 2012-01-30 at 10:34 +0100, Guennadi Liakhovetski wrote:
[snip]
> > > > How would you like to do this? Don't you think, it would be good to allow
> > > > both: either implement a dmaengine driver directly, exactly as all drivers
> > > > are doing now, or use the additional helper library for suitable (simple)
> > > > hardware types? I see it similar to I2C, where you either implement an I2C
> > > > driver directly, or you use the bitbanging abstraction for simpler
> > > > hardware.
> > > I think it would be good to have both, this can be used by folks who
> > > don't have sg support available.
> >
> > Ok, so, should I just rename the driver to sgsoft? Is this the only
> > change, that you'd like to see?
> That would be one :)
> Also, I would review the other patch patches by today/tomorrow, you can
> add any changes in next version as well
Do I understand it right, that I'm waiting for your review before
submitting the next version?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [Patch] tty: fix a build failure on sparc
From: Greg KH @ 2012-02-03 15:53 UTC (permalink / raw)
To: Cong Wang
Cc: linux-kernel, Andrew Morton, Paul Gortmaker, Alan Cox, Jamie Iles,
Stephen Warren, linux-serial
In-Reply-To: <1328246540-5824-1-git-send-email-xiyou.wangcong@gmail.com>
On Fri, Feb 03, 2012 at 01:22:10PM +0800, Cong Wang wrote:
> On sparc, there is a build failure:
>
> drivers/tty/serial/8250/8250.c:48:21: error: suncore.h: No such file or directory
> drivers/tty/serial/8250/8250.c:3275: error: implicit declaration of function 'sunserial_register_minors'
> drivers/tty/serial/8250/8250.c:3305: error: implicit declaration of function 'sunserial_unregister_minors'
>
> this is due to commit 9bef3d4197379a995fa80f81950bbbf8d32e9e8b
> (serial: group all the 8250 related code together) moved these files
> into 8250/ subdirectory, but forgot to change the reference
> to drivers/tty/serial/suncore.h.
Ah, thanks, I'll go queue this up, sorry for the problem.
greg k-h
^ permalink raw reply
* [PATCH/RFC] usb: fix renesas_usbhs to not schedule in atomic context
From: Guennadi Liakhovetski @ 2012-02-03 15:43 UTC (permalink / raw)
To: Shimoda, Yoshihiro
Cc: linux-kernel, linux-sh, Vinod Koul, Magnus Damm, linux-mmc,
alsa-devel, linux-serial, Paul Mundt, linux-usb
In-Reply-To: <4F2B9F34.60308@renesas.com>
The current renesas_usbhs driver triggers
BUG: scheduling while atomic: ksoftirqd/0/3/0x00000102
with enabled CONFIG_DEBUG_ATOMIC_SLEEP, by submitting DMA transfers from
an atomic (tasklet) context, which is not supported by the shdma dmaengine
driver. Fix it by switching to a work. Also simplify some list
manipulations.
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
Shimoda-san, this is the problem, that you were observing. However, it
exists with the present version of shdma just as well as with the new one
- on top of the simple DMA library. I marked it an RFC because (1) I only
lightly tested it with the gadget device on mackerel with the mass storage
gadget, and (2) I am somewhat concerned about races. Currently the work
function runs with no locking and there are no usual cancel_work_sync()
points in the patch. However, it has also been like this before with the
tasklet implementation, which is not much better, and it looks like there
are no asynchronous operations on the same packets like timeouts. Only
asynchronous events, that I can think about are things like unloading the
driver or unplugging the cable, but these have been there before too. It
would become worse on SMP, I think. Comments welcome.
diff --git a/drivers/usb/renesas_usbhs/fifo.c b/drivers/usb/renesas_usbhs/fifo.c
index 72339bd..4d739ec 100644
--- a/drivers/usb/renesas_usbhs/fifo.c
+++ b/drivers/usb/renesas_usbhs/fifo.c
@@ -75,8 +75,7 @@ void usbhs_pkt_push(struct usbhs_pipe *pipe, struct usbhs_pkt *pkt,
pipe->handler = &usbhsf_null_handler;
}
- list_del_init(&pkt->node);
- list_add_tail(&pkt->node, &pipe->list);
+ list_move_tail(&pkt->node, &pipe->list);
/*
* each pkt must hold own handler.
@@ -106,7 +105,7 @@ static struct usbhs_pkt *__usbhsf_pkt_get(struct usbhs_pipe *pipe)
if (list_empty(&pipe->list))
return NULL;
- return list_entry(pipe->list.next, struct usbhs_pkt, node);
+ return list_first_entry(&pipe->list, struct usbhs_pkt, node);
}
struct usbhs_pkt *usbhs_pkt_pop(struct usbhs_pipe *pipe, struct usbhs_pkt *pkt)
@@ -762,9 +761,9 @@ static int __usbhsf_dma_map_ctrl(struct usbhs_pkt *pkt, int map)
}
static void usbhsf_dma_complete(void *arg);
-static void usbhsf_dma_prepare_tasklet(unsigned long data)
+static void xfer_work(struct work_struct *work)
{
- struct usbhs_pkt *pkt = (struct usbhs_pkt *)data;
+ struct usbhs_pkt *pkt = container_of(work, struct usbhs_pkt, work);
struct usbhs_pipe *pipe = pkt->pipe;
struct usbhs_fifo *fifo = usbhs_pipe_to_fifo(pipe);
struct usbhs_priv *priv = usbhs_pipe_to_priv(pipe);
@@ -844,11 +843,8 @@ static int usbhsf_dma_prepare_push(struct usbhs_pkt *pkt, int *is_done)
pkt->trans = len;
- tasklet_init(&fifo->tasklet,
- usbhsf_dma_prepare_tasklet,
- (unsigned long)pkt);
-
- tasklet_schedule(&fifo->tasklet);
+ INIT_WORK(&pkt->work, xfer_work);
+ schedule_work(&pkt->work);
return 0;
@@ -938,11 +934,8 @@ static int usbhsf_dma_try_pop(struct usbhs_pkt *pkt, int *is_done)
pkt->trans = len;
- tasklet_init(&fifo->tasklet,
- usbhsf_dma_prepare_tasklet,
- (unsigned long)pkt);
-
- tasklet_schedule(&fifo->tasklet);
+ INIT_WORK(&pkt->work, xfer_work);
+ schedule_work(&pkt->work);
return 0;
diff --git a/drivers/usb/renesas_usbhs/fifo.h b/drivers/usb/renesas_usbhs/fifo.h
index f68609c..c31731a 100644
--- a/drivers/usb/renesas_usbhs/fifo.h
+++ b/drivers/usb/renesas_usbhs/fifo.h
@@ -19,6 +19,7 @@
#include <linux/interrupt.h>
#include <linux/sh_dma.h>
+#include <linux/workqueue.h>
#include <asm/dma.h>
#include "pipe.h"
@@ -31,7 +32,6 @@ struct usbhs_fifo {
u32 ctr; /* xFIFOCTR */
struct usbhs_pipe *pipe;
- struct tasklet_struct tasklet;
struct dma_chan *tx_chan;
struct dma_chan *rx_chan;
@@ -53,6 +53,7 @@ struct usbhs_pkt {
struct usbhs_pkt_handle *handler;
void (*done)(struct usbhs_priv *priv,
struct usbhs_pkt *pkt);
+ struct work_struct work;
dma_addr_t dma;
void *buf;
int length;
^ permalink raw reply related
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Russell King - ARM Linux @ 2012-02-03 12:20 UTC (permalink / raw)
To: NeilBrown
Cc: Govindraj, khilman, Paul Walmsley, greg, govindraj.raja,
tomi.valkeinen, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <20120203230720.28666cc6@notabene.brown>
On Fri, Feb 03, 2012 at 11:07:20PM +1100, NeilBrown wrote:
> However what it really shows me is that I was misunderstanding the code
> (If I spent the time to verify every conclusion that I jumped to, I'd never
> get anywhere :-( ). Better clarify that now.
>
> So this function - serial_omap_tx_empty() is called to test if the
> tx queue is empty.
Not just the queue. The documentation for that function is accurate:
tx_empty(port)
This function tests whether the transmitter fifo and shifter
for the port described by 'port' is empty. If it is empty,
this function should return TIOCSER_TEMT, otherwise return 0.
If the port does not support this operation, then it should
return TIOCSER_TEMT.
So, if this is returning TIOCSER_TEMT while there's still a transmission
in progress, then it's buggy.
> So when I
>
> cat somefile
>
> to the serial console, most of the file comes out fine. But after 'cat'
> finishes and returns to the shell - while some chars are still in the FIFO -
> the shell does an 'stty' ioctl to make sure the settings are still OK.
> This ioctl calls serial_omap_tx_empty which calls pm_resume_put() which
> immediately suspends the uart, which seems to stop some clock - even though
> we think it shouldn't.
If there's an on-going transmission, why is the runtime PM count zero,
meaning that the UART can sleep at the point where serial_omap_tx_empty()
is being called - and obviously there's still characters in the FIFO?
I guess this is highlighting a problem with doing runtime PM with
serial ports: you don't know when the port has _actually_ finished
sending its final character, which is even more of a problem if the
port does hardware CTS flow control itself.
It seems that runtime PM should be checking whether the TX FIFO is empty
before shutting the port down - and that probably means a hook into the
idle stuff.
Note though, that serial_omap_tx_empty() can be called via ioctl from
userspace, so this function would still need the runtime callbacks in
it so that the register is readable at _any_ time that the port is open.
^ permalink raw reply
* Re: patch "tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA" added to tty tree
From: Felipe Contreras @ 2012-02-03 12:12 UTC (permalink / raw)
To: NeilBrown
Cc: Paul Walmsley, Greg KH, greg, khilman, govindraj.raja,
tomi.valkeinen, linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <20120203150708.386951d2@notabene.brown>
On Fri, Feb 3, 2012 at 6:07 AM, NeilBrown <neilb@suse.de> wrote:
> If I then enabled runtime_pm with a 5 second autosuspend delay:
> CORE is still permanently ON (I think the USB might be doing that).
Maybe related to this:
http://thread.gmane.org/gmane.linux.usb.general/56096
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox