* Re: omap-serial RX DMA polling?
From: Paul Walmsley @ 2012-01-23 19:48 UTC (permalink / raw)
To: Govindraj
Cc: govindraj.raja, khilman, linux-serial, linux-omap,
linux-arm-kernel
In-Reply-To: <CAAL8m4wBuFQYkyFUwNWrUZTBWuQHggy=y3Oi=6DBHNB_rqU9tA@mail.gmail.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2568 bytes --]
On Mon, 23 Jan 2012, Govindraj wrote:
> On Mon, Jan 23, 2012 at 4:17 PM, Paul Walmsley <paul@pwsan.com> wrote:
> > On Mon, 23 Jan 2012, Govindraj wrote:
> >
> >> On Mon, Jan 23, 2012 at 6:03 AM, Paul Walmsley <paul@pwsan.com> wrote:
> >> >
> >> > while trying to track down some of the serial-related PM issues in
> >> > v3.3-rc1, I noticed that the omap-serial.c driver sets a 1 microsecond
> >> > polling timer when DMA is enabled (uart_dma.rx_timer) (!) This seems
> >> > quite broken from both the DMA and PM points of view.
> >>
> >> Poll rate is used for doing tty_insert_flip_string for pushing data to
> >> user space to keep faster response to any client device over uart, some
> >> Bt chips expect faster response when data on uart arrives and packet
> >> should be pushed out immediately.
> >
> > Hmm. Let's say that the BT transceiver uses the fastest transmission rate
> > supported by the OMAP UARTs -- 3,686,400 bits per second, according to
> > Table 17-1 in the 34xx TRM vZR. So the RX poll timer would go off about
> > ~2.7 times per input character[1]. That seems like overkill...
>
> Yes correct, Looks like the poll rate is to aggressive
> it should be calculated based on baud rate provided from user apace
> in termios function.
>
> I had a patch to do the same in termios but,
> if you have something similar you can post out as I am currently busy with
> some other activities and may take more time.
>
> > For minimum receive latency, how about calling tty_insert_flip_string()
> > from the RX DMA callback, and using a smaller transfer count? Or even
> > better, use PIO for the receive path and set the RX FIFO threshold to 1?
> >
> > No poll timer should be needed in either case.
>
> I remember doing similar excercise with BT + uart on zoom board
> but performance numbers where impacted.
>
> I made buffer size as 1 byte and removed polling function and got rx_callback
> for every byte completion and pushed same to tty layer.
>
> BT FTP throughput got impacted a lot.
The point is that if you want tty_insert_flip_string() to be called after
every character is received, there seems little point in using RX DMA.
It should be less efficient than PIO. And the current way that it is used
is pointless from a power management point of view.
In general, RX DMA would seem to be inappropriate for a latency-sensitive
application, unless the application can somehow communicate how many bytes
it's expecting so the driver can adjust its DMA transfer size.
- Paul
^ permalink raw reply
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Paul E. McKenney @ 2012-01-23 16:45 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Simon Glass, Alan Cox, LKML, Greg Kroah-Hartman, linux-serial
In-Reply-To: <201201210049.35254.rjw@sisk.pl>
On Sat, Jan 21, 2012 at 12:49:35AM +0100, Rafael J. Wysocki wrote:
> On Friday, January 20, 2012, Paul E. McKenney wrote:
> > On Fri, Jan 20, 2012 at 01:03:34AM +0100, Rafael J. Wysocki wrote:
> > > On Thursday, January 19, 2012, Paul E. McKenney wrote:
> > > > On Thu, Jan 19, 2012 at 01:02:58AM +0100, Rafael J. Wysocki wrote:
> > > > > On Wednesday, January 18, 2012, Paul E. McKenney wrote:
> > > > > > On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
> > > [...]
> > > > > Yes, you can, but then I'd say it's not necessary for user space to
> > > > > be able to carry that out in a tight loop. So, it seems, alternatively,
> > > > > we could make that loop a bit less tight, e.g. by adding an arbitrary
> > > > > sleep to the user space interface for the "disable" case.
> > > >
> > > > Good point, that would work just as well and be simpler.
> > >
> > > Thanks for the confirmation! :-)
> > >
> > > By the way, I wonder, would it help to add synchronize_rcu() to
> > > wakeup_source_add() too? Then, even if device_wakeup_enable() and
> > > device_wakeup_disable() are executed in a tight loop for the same
> > > device, the list_add/list_del operations will always happen in
> > > different RCU cycles (or at least it seems so).
> >
> > I cannot immediately see how adding a synchronize_rcu() to
> > wakeup_source_add() would help anything. You only need to wait for a
> > grace period on removal, not (normally) on addition. The single grace
> > period during removal will catch up all other asynchronous RCU grace
> > period requests on that CPU.
> >
> > Or am I missing your point?
>
> Well, I was thinking about the failure scenario you mentioned where
> executing enable/disable in a tight loop might exhaust system memory
> (if I understood it correctly).
Ah, got it. If they are executing this in a tight loop, there will be
little difference between doing one synchronize_rcu() per pass through
the loop or doing two. So we should be just fine with the single instance
of synchronize_rcu() per loop.
Thanx, Paul
^ permalink raw reply
* Re: omap-serial RX DMA polling?
From: Govindraj @ 2012-01-23 14:36 UTC (permalink / raw)
To: Paul Walmsley
Cc: govindraj.raja, khilman, linux-serial, linux-omap,
linux-arm-kernel
In-Reply-To: <alpine.DEB.2.00.1201230304310.29673@utopia.booyaka.com>
On Mon, Jan 23, 2012 at 4:17 PM, Paul Walmsley <paul@pwsan.com> wrote:
> On Mon, 23 Jan 2012, Govindraj wrote:
>
>> On Mon, Jan 23, 2012 at 6:03 AM, Paul Walmsley <paul@pwsan.com> wrote:
>> >
>> > while trying to track down some of the serial-related PM issues in
>> > v3.3-rc1, I noticed that the omap-serial.c driver sets a 1 microsecond
>> > polling timer when DMA is enabled (uart_dma.rx_timer) (!) This seems
>> > quite broken from both the DMA and PM points of view.
>>
>> Poll rate is used for doing tty_insert_flip_string for pushing data to
>> user space to keep faster response to any client device over uart, some
>> Bt chips expect faster response when data on uart arrives and packet
>> should be pushed out immediately.
>
> Hmm. Let's say that the BT transceiver uses the fastest transmission rate
> supported by the OMAP UARTs -- 3,686,400 bits per second, according to
> Table 17-1 in the 34xx TRM vZR. So the RX poll timer would go off about
> ~2.7 times per input character[1]. That seems like overkill...
>
Yes correct, Looks like the poll rate is to aggressive
it should be calculated based on baud rate provided from user apace
in termios function.
I had a patch to do the same in termios but,
if you have something similar you can post out as I am currently busy with
some other activities and may take more time.
> For minimum receive latency, how about calling tty_insert_flip_string()
> from the RX DMA callback, and using a smaller transfer count? Or even
> better, use PIO for the receive path and set the RX FIFO threshold to 1?
>
> No poll timer should be needed in either case.
I remember doing similar excercise with BT + uart on zoom board
but performance numbers where impacted.
I made buffer size as 1 byte and removed polling function and got rx_callback
for every byte completion and pushed same to tty layer.
BT FTP throughput got impacted a lot.
--
Thanks,
Govindraj.R
note: I little busy currently and replies might be delayed.
sorry for any inconvenience.
>
>
> - Paul
>
> 1. At 10 line bits per character (start + byte + stop), each character
> should take about 2.7 microseconds to transfer (the reciprocal of (3 686
> 400 line bits per second / 10 line bits per character / 1 000 000
> microseconds per second)).
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" 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
* Re: omap-serial RX DMA polling?
From: Paul Walmsley @ 2012-01-23 10:47 UTC (permalink / raw)
To: Govindraj
Cc: govindraj.raja, khilman, linux-serial, linux-omap,
linux-arm-kernel
In-Reply-To: <CAAL8m4wcSfhjxg0Lyo-W7DQ85rikb6asRds95hwQQNTDXe2YKQ@mail.gmail.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1490 bytes --]
On Mon, 23 Jan 2012, Govindraj wrote:
> On Mon, Jan 23, 2012 at 6:03 AM, Paul Walmsley <paul@pwsan.com> wrote:
> >
> > while trying to track down some of the serial-related PM issues in
> > v3.3-rc1, I noticed that the omap-serial.c driver sets a 1 microsecond
> > polling timer when DMA is enabled (uart_dma.rx_timer) (!) This seems
> > quite broken from both the DMA and PM points of view.
>
> Poll rate is used for doing tty_insert_flip_string for pushing data to
> user space to keep faster response to any client device over uart, some
> Bt chips expect faster response when data on uart arrives and packet
> should be pushed out immediately.
Hmm. Let's say that the BT transceiver uses the fastest transmission rate
supported by the OMAP UARTs -- 3,686,400 bits per second, according to
Table 17-1 in the 34xx TRM vZR. So the RX poll timer would go off about
~2.7 times per input character[1]. That seems like overkill...
For minimum receive latency, how about calling tty_insert_flip_string()
from the RX DMA callback, and using a smaller transfer count? Or even
better, use PIO for the receive path and set the RX FIFO threshold to 1?
No poll timer should be needed in either case.
- Paul
1. At 10 line bits per character (start + byte + stop), each character
should take about 2.7 microseconds to transfer (the reciprocal of (3 686
400 line bits per second / 10 line bits per character / 1 000 000
microseconds per second)).
^ permalink raw reply
* Re: [PATCH 1/2] tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA mode
From: Govindraj @ 2012-01-23 10:25 UTC (permalink / raw)
To: Paul Walmsley
Cc: linux-omap, linux-serial, linux-arm-kernel, Govindraj Raja,
Kevin Hilman, Tomi Valkeinen, alan
In-Reply-To: <CAAL8m4xHzdESmWy8eKwX5-Zb1z8FSRzBxF67bkf=N3JjCL9rBg@mail.gmail.com>
On Mon, Jan 23, 2012 at 3:51 PM, Govindraj <govindraj.ti@gmail.com> wrote:
> On Sat, Jan 21, 2012 at 12:57 PM, Paul Walmsley <paul@pwsan.com> wrote:
>> Ensure FIFO levels are set correctly in non-DMA mode (the default).
>> This patch will cause a receive FIFO threshold interrupt to be raised when
>> there is at least one byte in the RX FIFO. It will also cause a transmit
>> FIFO threshold interrupt when there is only one byte remaining in the TX
>> FIFO.
>>
>> These changes fix the receive interrupt problem and part of the
>> transmit interrupt problem. A separate set of issues must be worked
>> around for the transmit path to have a basic level of functionality; a
>> subsequent patch will address these.
>>
>> DMA operation is unaffected by this patch.
>>
>> Signed-off-by: Paul Walmsley <paul@pwsan.com>
>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Cc: Govindraj Raja <govindraj.r@ti.com>
>> Cc: Kevin Hilman <khilman@ti.com>
>> ---
>> drivers/tty/serial/omap-serial.c | 35 +++++++++++++++++++++++++++++++----
>> 1 files changed, 31 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index d192dcb..9de7d71 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -46,6 +46,18 @@
>>
>> #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)
>> +
>> +/* FCR register bitmasks */
>> +#define OMAP_UART_FCR_RX_FIFO_TRIG_SHIFT 6
>> +#define OMAP_UART_FCR_RX_FIFO_TRIG_MASK (0x3 << 6)
>> +#define OMAP_UART_FCR_TX_FIFO_TRIG_SHIFT 4
>> +
>> +/* TLR register bitmasks */
>> +#define OMAP_UART_TLR_TX_FIFO_TRIG_DMA_SHIFT 0
>> +
>> static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
>>
>> /* Forward declaration of functions */
>> @@ -694,6 +706,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>> unsigned char efr = 0;
>> unsigned long flags = 0;
>> unsigned int baud, quot;
>> + u32 tlr;
>>
>> switch (termios->c_cflag & CSIZE) {
>> case CS5:
>> @@ -811,14 +824,28 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>> up->mcr = serial_in(up, UART_MCR);
>> serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
>> /* FIFO ENABLE, DMA MODE */
>> - serial_out(up, UART_FCR, up->fcr);
>> - serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>> +
>> + up->scr |= OMAP_UART_SCR_TX_TRIG_GRANU1_MASK;
>> + up->scr |= OMAP_UART_SCR_RX_TRIG_GRANU1_MASK;
>>
>> if (up->use_dma) {
>> - serial_out(up, UART_TI752_TLR, 0);
>> - up->scr |= (UART_FCR_TRIGGER_4 | UART_FCR_TRIGGER_8);
>
> Any reasons for removing scr config for dma mode ?
>
Please ignore.. was in hurry.
>
>> + tlr = 0;
>> + } else {
>> + up->scr &= ~OMAP_UART_SCR_TX_EMPTY;
>> +
>> + /* Set receive FIFO threshold to 1 */
>> + up->fcr &= ~OMAP_UART_FCR_RX_FIFO_TRIG_MASK;
>> + up->fcr |= (0x1 << OMAP_UART_FCR_RX_FIFO_TRIG_SHIFT);
>> +
>> + /* Set TX FIFO threshold to "63" (actually 1) */
>> + up->fcr |= (0x3 << OMAP_UART_FCR_TX_FIFO_TRIG_SHIFT);
>> + tlr = (0xf << OMAP_UART_TLR_TX_FIFO_TRIG_DMA_SHIFT);
>> }
>>
>> + serial_out(up, UART_TI752_TLR, tlr);
>> + serial_out(up, UART_FCR, up->fcr);
>> + serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>> +
>> serial_out(up, UART_OMAP_SCR, up->scr);
>>
>> serial_out(up, UART_EFR, up->efr);
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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
* Re: [PATCH 1/2] tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA mode
From: Govindraj @ 2012-01-23 10:21 UTC (permalink / raw)
To: Paul Walmsley
Cc: linux-omap, linux-serial, linux-arm-kernel, Govindraj Raja,
Kevin Hilman, Tomi Valkeinen, alan
In-Reply-To: <20120121072737.18707.77811.stgit@dusk>
On Sat, Jan 21, 2012 at 12:57 PM, Paul Walmsley <paul@pwsan.com> wrote:
> Ensure FIFO levels are set correctly in non-DMA mode (the default).
> This patch will cause a receive FIFO threshold interrupt to be raised when
> there is at least one byte in the RX FIFO. It will also cause a transmit
> FIFO threshold interrupt when there is only one byte remaining in the TX
> FIFO.
>
> These changes fix the receive interrupt problem and part of the
> transmit interrupt problem. A separate set of issues must be worked
> around for the transmit path to have a basic level of functionality; a
> subsequent patch will address these.
>
> DMA operation is unaffected by this patch.
>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Govindraj Raja <govindraj.r@ti.com>
> Cc: Kevin Hilman <khilman@ti.com>
> ---
> drivers/tty/serial/omap-serial.c | 35 +++++++++++++++++++++++++++++++----
> 1 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index d192dcb..9de7d71 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -46,6 +46,18 @@
>
> #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)
> +
> +/* FCR register bitmasks */
> +#define OMAP_UART_FCR_RX_FIFO_TRIG_SHIFT 6
> +#define OMAP_UART_FCR_RX_FIFO_TRIG_MASK (0x3 << 6)
> +#define OMAP_UART_FCR_TX_FIFO_TRIG_SHIFT 4
> +
> +/* TLR register bitmasks */
> +#define OMAP_UART_TLR_TX_FIFO_TRIG_DMA_SHIFT 0
> +
> static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
>
> /* Forward declaration of functions */
> @@ -694,6 +706,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
> unsigned char efr = 0;
> unsigned long flags = 0;
> unsigned int baud, quot;
> + u32 tlr;
>
> switch (termios->c_cflag & CSIZE) {
> case CS5:
> @@ -811,14 +824,28 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
> up->mcr = serial_in(up, UART_MCR);
> serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
> /* FIFO ENABLE, DMA MODE */
> - serial_out(up, UART_FCR, up->fcr);
> - serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
> +
> + up->scr |= OMAP_UART_SCR_TX_TRIG_GRANU1_MASK;
> + up->scr |= OMAP_UART_SCR_RX_TRIG_GRANU1_MASK;
>
> if (up->use_dma) {
> - serial_out(up, UART_TI752_TLR, 0);
> - up->scr |= (UART_FCR_TRIGGER_4 | UART_FCR_TRIGGER_8);
Any reasons for removing scr config for dma mode ?
> + tlr = 0;
> + } else {
> + up->scr &= ~OMAP_UART_SCR_TX_EMPTY;
> +
> + /* Set receive FIFO threshold to 1 */
> + up->fcr &= ~OMAP_UART_FCR_RX_FIFO_TRIG_MASK;
> + up->fcr |= (0x1 << OMAP_UART_FCR_RX_FIFO_TRIG_SHIFT);
> +
> + /* Set TX FIFO threshold to "63" (actually 1) */
> + up->fcr |= (0x3 << OMAP_UART_FCR_TX_FIFO_TRIG_SHIFT);
> + tlr = (0xf << OMAP_UART_TLR_TX_FIFO_TRIG_DMA_SHIFT);
> }
>
> + serial_out(up, UART_TI752_TLR, tlr);
> + serial_out(up, UART_FCR, up->fcr);
> + serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
> +
> serial_out(up, UART_OMAP_SCR, up->scr);
>
> serial_out(up, UART_EFR, up->efr);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" 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
* Re: omap-serial RX DMA polling?
From: Govindraj @ 2012-01-23 10:00 UTC (permalink / raw)
To: Paul Walmsley
Cc: khilman, govindraj.raja, linux-omap, linux-arm-kernel,
linux-serial
In-Reply-To: <alpine.DEB.2.00.1201221718190.26641@utopia.booyaka.com>
On Mon, Jan 23, 2012 at 6:03 AM, Paul Walmsley <paul@pwsan.com> wrote:
>
> Hello Govindraj
>
> while trying to track down some of the serial-related PM issues in
> v3.3-rc1, I noticed that the omap-serial.c driver sets a 1 microsecond
> polling timer when DMA is enabled (uart_dma.rx_timer) (!) This seems
> quite broken from both the DMA and PM points of view.
>
Poll rate is used for doing tty_insert_flip_string for pushing data to
user space
to keep faster response to any client device over uart, some Bt chips
expect faster
response when data on uart arrives and packet should be pushed out immediately.
> From a DMA point of view, the DMA transfer should automatically start
> when there is data to transfer, and stop when there is no data left, based
> on the DMA request lines. So timer-driven polling should not be needed at
> all.
>
its for tty_insert pushing data to tty stack.
> From a PM point of view, this short timer will effectively prevent the MPU
> from going into a low-power state whenever there is data in the FIFO.
> This will more than erase any energy consumption or CPU efficiency
> benefits of doing DMA. Interrupt-driven PIO should be much more efficient
> than this, since at least the MPU can enter a low-power state while
> waiting for the FIFO to fill.
>
yes we maintain 3 secs timeout period and keep uart active in DMA case.
since rx is asynchronous we cant afford to stop and start dma every time after
x bytes after every rx data completion call back.
> So basically, the broken timeout calculations used in the interrupt-driven
> PIO mode (which set a 1 microsecond PM QoS constraint), plus the 1
> microsecond polling timer used in DMA mode, mean that this driver is
> pretty bad from a PM perspective.
>
> I sent some patches to fix the interrupt-driven PIO receive part of the
> problem, which you've probably seen. But I'm hoping that you can describe
> further why the driver needs this RX DMA polling timer? Shouldn't it be
> unnecessary? If it's truly unavoidable, then we should presumably not
> even bother with RX DMA at all.
>
>
>
> - Paul
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" 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
* Re: [PATCH 2/2] tty: serial: OMAP: transmit FIFO threshold interrupts don't wake the chip
From: Govindraj @ 2012-01-23 9:50 UTC (permalink / raw)
To: Paul Walmsley
Cc: linux-omap, linux-serial, linux-arm-kernel, Govindraj Raja,
Kevin Hilman, Tomi Valkeinen, alan
In-Reply-To: <CAAL8m4xAzAJWFZXaTeQQpTE2DbadH4zovSGcQsLjP=43GNiRiA@mail.gmail.com>
On Mon, Jan 23, 2012 at 2:20 PM, Govindraj <govindraj.ti@gmail.com> wrote:
> On Sat, Jan 21, 2012 at 12:57 PM, Paul Walmsley <paul@pwsan.com> wrote:
>> It seems that when the transmit FIFO threshold is reached on OMAP
>> UARTs, it does not result in a PRCM wakeup. This appears to be a
>> silicon bug. This means that if the MPU powerdomain is in a low-power
>> state, the MPU will not be awakened to refill the FIFO until the next
>> interrupt from another device.
>>
>> The best solution, at least for the short term, would be for the OMAP
>> serial driver to call a OMAP subarchitecture function to prevent the
>> MPU powerdomain from entering a low power state while the FIFO has
>> data to transmit. However, we no longer have a clean way to do this,
>> since patches that add platform_data function pointers have been
>> deprecated by the OMAP maintainer. So we attempt to work around this
>> as well. The workarounds depend on the setting of CONFIG_CPU_IDLE.
>>
>> When CONFIG_CPU_IDLE=n, the driver will now only transmit one byte at
>> a time. This causes the transmit FIFO threshold interrupt to stay
>> active until there is no more data to be sent. Thus, the MPU
>> powerdomain stays on during transmits. Aside from that energy
>> consumption penalty, each transmitted byte results in a huge number of
>> UART interrupts -- about five per byte. This wastes CPU time and is
>> quite inefficient, but is probably the most expedient workaround in
>> this case.
>>
>> When CONFIG_CPU_IDLE=y, there is a slightly more direct workaround:
>> the PM QoS constraint can be abused to keep the MPU powerdomain on.
>> This results in a normal number of interrupts, but, similar to the
>> above workaround, wastes power by preventing the MPU from entering
>> WFI.
>>
>> Future patches are planned for the 3.4 merge window to implement more
>> efficient, but also more disruptive, workarounds to these problems.
>>
>
> With these two patches number of interrupts seems to increase by 24x
> after boot up.
>
> [...]
>
> / # cat /proc/interrupts | grep "UART2"
> 74: 3902 INTC OMAP UART2
> / #
> [...]
>
> without these two patches + cpu_idle enabled
>
> [...]
>
> / #
> / # cat /proc/interrupts | grep "UART2"
> 74: 158 INTC OMAP UART2
> / #
>
> [...]
>
>
> I am using beagle xm and 3.3rc1
>
> Looks like there are far two many uart irqs which
> keeps the mpu busy and thus preventing uart sluggishness.
>
after looking in closely,
looks like every byte is getting transferred in case
of non cpu_idle cases causing 1 irq for each tx.
> --
> Thanks,
> Govindraj.R
>
>
>> DMA operation is unaffected by this patch.
>>
>> Signed-off-by: Paul Walmsley <paul@pwsan.com>
>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Cc: Govindraj Raja <govindraj.r@ti.com>
>> Cc: Kevin Hilman <khilman@ti.com>
>> ---
>> arch/arm/plat-omap/include/plat/omap-serial.h | 1
>> drivers/tty/serial/omap-serial.c | 51 +++++++++++++++++++++++++
>> 2 files changed, 51 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
>> index 9ff4444..12a64eb 100644
>> --- a/arch/arm/plat-omap/include/plat/omap-serial.h
>> +++ b/arch/arm/plat-omap/include/plat/omap-serial.h
>> @@ -131,6 +131,7 @@ struct uart_omap_port {
>> u32 context_loss_cnt;
>> u32 errata;
>> u8 wakeups_enabled;
>> + u8 max_tx_count;
>>
>> struct pm_qos_request pm_qos_request;
>> u32 latency;
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index 9de7d71..621fde5 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -88,6 +88,49 @@ static inline void serial_omap_clear_fifos(struct uart_omap_port *up)
>> serial_out(up, UART_FCR, 0);
>> }
>>
>> +/**
>> + * serial_omap_block_cpu_low_power_state - prevent MPU pwrdm from leaving ON
>> + * @up: struct uart_omap_port *
>> + *
>> + * Prevent the MPU powerdomain from entering a power state lower than
>> + * ON. (It should be sufficient to prevent it from entering INACTIVE,
>> + * but there is presently no easy way to do this.) This works around
>> + * a suspected silicon bug in the OMAP UART IP blocks. The UARTs should
>> + * wake the PRCM when the transmit FIFO threshold interrupt is raised, but
>> + * they do not. See also serial_omap_allow_cpu_low_power_state(). No
>> + * return value.
>> + */
>> +static void serial_omap_block_cpu_low_power_state(struct uart_omap_port *up)
>> +{
>> +#ifdef CONFIG_CPU_IDLE
>> + up->latency = 1;
>> + schedule_work(&up->qos_work);
>> +#else
>> + up->max_tx_count = 1;
>> +#endif
>> +}
>> +
>> +/**
>> + * serial_omap_allow_cpu_low_power_state - remove power state restriction on MPU
>> + * @up: struct uart_omap_port *
>> + *
>> + * Cancel the effects of serial_omap_block_cpu_low_power_state().
>> + * This should allow the MPU powerdomain to enter a power state lower
>> + * than ON, assuming the rest of the kernel is not restricting it.
>> + * This works around a suspected silicon bug in the OMAP UART IP
>> + * blocks. The UARTs should wake the PRCM when the transmit FIFO
>> + * threshold interrupt is raised, but they do not. No return value.
>> + */
>> +static void serial_omap_allow_cpu_low_power_state(struct uart_omap_port *up)
>> +{
>> +#ifdef CONFIG_CPU_IDLE
>> + up->latency = up->calc_latency;
>> + schedule_work(&up->qos_work);
>> +#else
>> + up->max_tx_count = up->port.fifosize / 4;
>> +#endif
>> +}
>> +
>> /*
>> * serial_omap_get_divisor - calculate divisor value
>> * @port: uart port info
>> @@ -163,6 +206,9 @@ static void serial_omap_stop_tx(struct uart_port *port)
>> serial_out(up, UART_IER, up->ier);
>> }
>>
>> + if (!up->use_dma)
>> + serial_omap_allow_cpu_low_power_state(up);
>> +
>> pm_runtime_mark_last_busy(&up->pdev->dev);
>> pm_runtime_put_autosuspend(&up->pdev->dev);
>> }
>> @@ -264,7 +310,7 @@ static void transmit_chars(struct uart_omap_port *up)
>> serial_omap_stop_tx(&up->port);
>> return;
>> }
>> - count = up->port.fifosize / 4;
>> + count = up->max_tx_count;
>> do {
>> serial_out(up, UART_TX, xmit->buf[xmit->tail]);
>> xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
>> @@ -297,6 +343,7 @@ static void serial_omap_start_tx(struct uart_port *port)
>>
>> if (!up->use_dma) {
>> pm_runtime_get_sync(&up->pdev->dev);
>> + serial_omap_block_cpu_low_power_state(up);
>> serial_omap_enable_ier_thri(up);
>> pm_runtime_mark_last_busy(&up->pdev->dev);
>> pm_runtime_put_autosuspend(&up->pdev->dev);
>> @@ -1421,6 +1468,8 @@ static int serial_omap_probe(struct platform_device *pdev)
>> up->port.fifosize = 64;
>> up->port.ops = &serial_omap_pops;
>>
>> + up->max_tx_count = up->port.fifosize / 4;
>> +
>> if (pdev->dev.of_node)
>> up->port.line = of_alias_get_id(pdev->dev.of_node, "serial");
>> else
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
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
* Re: [PATCH 2/2] tty: serial: OMAP: transmit FIFO threshold interrupts don't wake the chip
From: Paul Walmsley @ 2012-01-23 9:49 UTC (permalink / raw)
To: Govindraj
Cc: linux-omap, linux-serial, linux-arm-kernel, Govindraj Raja,
Kevin Hilman, Tomi Valkeinen, alan
In-Reply-To: <CAAL8m4xAzAJWFZXaTeQQpTE2DbadH4zovSGcQsLjP=43GNiRiA@mail.gmail.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 4476 bytes --]
On Mon, 23 Jan 2012, Govindraj wrote:
> On Sat, Jan 21, 2012 at 12:57 PM, Paul Walmsley <paul@pwsan.com> wrote:
> > It seems that when the transmit FIFO threshold is reached on OMAP
> > UARTs, it does not result in a PRCM wakeup. This appears to be a
> > silicon bug. This means that if the MPU powerdomain is in a low-power
> > state, the MPU will not be awakened to refill the FIFO until the next
> > interrupt from another device.
> >
> > The best solution, at least for the short term, would be for the OMAP
> > serial driver to call a OMAP subarchitecture function to prevent the
> > MPU powerdomain from entering a low power state while the FIFO has
> > data to transmit. However, we no longer have a clean way to do this,
> > since patches that add platform_data function pointers have been
> > deprecated by the OMAP maintainer. So we attempt to work around this
> > as well. The workarounds depend on the setting of CONFIG_CPU_IDLE.
> >
> > When CONFIG_CPU_IDLE=n, the driver will now only transmit one byte at
> > a time. This causes the transmit FIFO threshold interrupt to stay
> > active until there is no more data to be sent. Thus, the MPU
> > powerdomain stays on during transmits. Aside from that energy
> > consumption penalty, each transmitted byte results in a huge number of
> > UART interrupts -- about five per byte. This wastes CPU time and is
> > quite inefficient, but is probably the most expedient workaround in
> > this case.
> >
> > When CONFIG_CPU_IDLE=y, there is a slightly more direct workaround:
> > the PM QoS constraint can be abused to keep the MPU powerdomain on.
> > This results in a normal number of interrupts, but, similar to the
> > above workaround, wastes power by preventing the MPU from entering
> > WFI.
> >
> > Future patches are planned for the 3.4 merge window to implement more
> > efficient, but also more disruptive, workarounds to these problems.
> >
>
> With these two patches number of interrupts seems to increase by 24x
> after boot up.
If you re-read the patch description that you quoted above, you'll see
that this behavior was clearly mentioned:
> > When CONFIG_CPU_IDLE=n, the driver will now only transmit one byte at
> > a time. This causes the transmit FIFO threshold interrupt to stay
> > active until there is no more data to be sent. Thus, the MPU
> > powerdomain stays on during transmits. Aside from that energy
> > consumption penalty, each transmitted byte results in a huge number of
> > UART interrupts -- about five per byte. This wastes CPU time and is
> > quite inefficient, but is probably the most expedient workaround in
> > this case.
> / # cat /proc/interrupts | grep "UART2"
> 74: 3902 INTC OMAP UART2
> / #
> [...]
>
> without these two patches + cpu_idle enabled
You should compare apples to apples. The omap-serial.c without these two
patches is unusable as a console when CONFIG_CPU_IDLE=n; there are long
lags between transmit FIFO drains. CONFIG_CPU_IDLE is the setting in the
standard omap2plus_defconfig.
I guess it was never tested with that standard config? Or were the
v3.3 omap-serial patch series submitted with this bug known?
> [...]
>
> / #
> / # cat /proc/interrupts | grep "UART2"
> 74: 158 INTC OMAP UART2
> / #
The only reason why the driver 'works' when CONFIG_CPU_IDLE=y is because
the omap-serial.c RX path wakeup latency constraint calculation is broken.
It adds a 1 microsecond latency constraint to the CPU, when it should be
adding a ~ 1100 microsecond latency constraint to the CPU. (Well, ~ 5500
microsecond, but that's another issue.)
And this keeps the CPU from entering a low-power state not just during
transmits, but during the entire time the driver is active for the console
UART.
> I am using beagle xm and 3.3rc1
>
> Looks like there are far two many uart irqs which
> keeps the mpu busy and thus preventing uart sluggishness.
Yes, that's known behavior, as described in the quoted patch description.
It's not clear why the number of interrupts is ~5x the number of
transmitted bytes, rather than say 2x the number of transmitted bytes.
But the IRQ handler in omap-serial.c is also a mess, so that may have
something to do with it.
Do you have a better workaround for the CONFIG_CPU_IDLE=n case that
is acceptable for the -rc series? If so, perhaps you can post it?
- Paul
^ permalink raw reply
* Re: [PATCH 2/2] tty: serial: OMAP: transmit FIFO threshold interrupts don't wake the chip
From: Govindraj @ 2012-01-23 8:50 UTC (permalink / raw)
To: Paul Walmsley
Cc: linux-omap, linux-serial, linux-arm-kernel, Govindraj Raja,
Kevin Hilman, Tomi Valkeinen, alan
In-Reply-To: <20120121072740.18707.87632.stgit@dusk>
On Sat, Jan 21, 2012 at 12:57 PM, Paul Walmsley <paul@pwsan.com> wrote:
> It seems that when the transmit FIFO threshold is reached on OMAP
> UARTs, it does not result in a PRCM wakeup. This appears to be a
> silicon bug. This means that if the MPU powerdomain is in a low-power
> state, the MPU will not be awakened to refill the FIFO until the next
> interrupt from another device.
>
> The best solution, at least for the short term, would be for the OMAP
> serial driver to call a OMAP subarchitecture function to prevent the
> MPU powerdomain from entering a low power state while the FIFO has
> data to transmit. However, we no longer have a clean way to do this,
> since patches that add platform_data function pointers have been
> deprecated by the OMAP maintainer. So we attempt to work around this
> as well. The workarounds depend on the setting of CONFIG_CPU_IDLE.
>
> When CONFIG_CPU_IDLE=n, the driver will now only transmit one byte at
> a time. This causes the transmit FIFO threshold interrupt to stay
> active until there is no more data to be sent. Thus, the MPU
> powerdomain stays on during transmits. Aside from that energy
> consumption penalty, each transmitted byte results in a huge number of
> UART interrupts -- about five per byte. This wastes CPU time and is
> quite inefficient, but is probably the most expedient workaround in
> this case.
>
> When CONFIG_CPU_IDLE=y, there is a slightly more direct workaround:
> the PM QoS constraint can be abused to keep the MPU powerdomain on.
> This results in a normal number of interrupts, but, similar to the
> above workaround, wastes power by preventing the MPU from entering
> WFI.
>
> Future patches are planned for the 3.4 merge window to implement more
> efficient, but also more disruptive, workarounds to these problems.
>
With these two patches number of interrupts seems to increase by 24x
after boot up.
[...]
/ # cat /proc/interrupts | grep "UART2"
74: 3902 INTC OMAP UART2
/ #
[...]
without these two patches + cpu_idle enabled
[...]
/ #
/ # cat /proc/interrupts | grep "UART2"
74: 158 INTC OMAP UART2
/ #
[...]
I am using beagle xm and 3.3rc1
Looks like there are far two many uart irqs which
keeps the mpu busy and thus preventing uart sluggishness.
--
Thanks,
Govindraj.R
> DMA operation is unaffected by this patch.
>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Govindraj Raja <govindraj.r@ti.com>
> Cc: Kevin Hilman <khilman@ti.com>
> ---
> arch/arm/plat-omap/include/plat/omap-serial.h | 1
> drivers/tty/serial/omap-serial.c | 51 +++++++++++++++++++++++++
> 2 files changed, 51 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
> index 9ff4444..12a64eb 100644
> --- a/arch/arm/plat-omap/include/plat/omap-serial.h
> +++ b/arch/arm/plat-omap/include/plat/omap-serial.h
> @@ -131,6 +131,7 @@ struct uart_omap_port {
> u32 context_loss_cnt;
> u32 errata;
> u8 wakeups_enabled;
> + u8 max_tx_count;
>
> struct pm_qos_request pm_qos_request;
> u32 latency;
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 9de7d71..621fde5 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -88,6 +88,49 @@ static inline void serial_omap_clear_fifos(struct uart_omap_port *up)
> serial_out(up, UART_FCR, 0);
> }
>
> +/**
> + * serial_omap_block_cpu_low_power_state - prevent MPU pwrdm from leaving ON
> + * @up: struct uart_omap_port *
> + *
> + * Prevent the MPU powerdomain from entering a power state lower than
> + * ON. (It should be sufficient to prevent it from entering INACTIVE,
> + * but there is presently no easy way to do this.) This works around
> + * a suspected silicon bug in the OMAP UART IP blocks. The UARTs should
> + * wake the PRCM when the transmit FIFO threshold interrupt is raised, but
> + * they do not. See also serial_omap_allow_cpu_low_power_state(). No
> + * return value.
> + */
> +static void serial_omap_block_cpu_low_power_state(struct uart_omap_port *up)
> +{
> +#ifdef CONFIG_CPU_IDLE
> + up->latency = 1;
> + schedule_work(&up->qos_work);
> +#else
> + up->max_tx_count = 1;
> +#endif
> +}
> +
> +/**
> + * serial_omap_allow_cpu_low_power_state - remove power state restriction on MPU
> + * @up: struct uart_omap_port *
> + *
> + * Cancel the effects of serial_omap_block_cpu_low_power_state().
> + * This should allow the MPU powerdomain to enter a power state lower
> + * than ON, assuming the rest of the kernel is not restricting it.
> + * This works around a suspected silicon bug in the OMAP UART IP
> + * blocks. The UARTs should wake the PRCM when the transmit FIFO
> + * threshold interrupt is raised, but they do not. No return value.
> + */
> +static void serial_omap_allow_cpu_low_power_state(struct uart_omap_port *up)
> +{
> +#ifdef CONFIG_CPU_IDLE
> + up->latency = up->calc_latency;
> + schedule_work(&up->qos_work);
> +#else
> + up->max_tx_count = up->port.fifosize / 4;
> +#endif
> +}
> +
> /*
> * serial_omap_get_divisor - calculate divisor value
> * @port: uart port info
> @@ -163,6 +206,9 @@ static void serial_omap_stop_tx(struct uart_port *port)
> serial_out(up, UART_IER, up->ier);
> }
>
> + if (!up->use_dma)
> + serial_omap_allow_cpu_low_power_state(up);
> +
> pm_runtime_mark_last_busy(&up->pdev->dev);
> pm_runtime_put_autosuspend(&up->pdev->dev);
> }
> @@ -264,7 +310,7 @@ static void transmit_chars(struct uart_omap_port *up)
> serial_omap_stop_tx(&up->port);
> return;
> }
> - count = up->port.fifosize / 4;
> + count = up->max_tx_count;
> do {
> serial_out(up, UART_TX, xmit->buf[xmit->tail]);
> xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
> @@ -297,6 +343,7 @@ static void serial_omap_start_tx(struct uart_port *port)
>
> if (!up->use_dma) {
> pm_runtime_get_sync(&up->pdev->dev);
> + serial_omap_block_cpu_low_power_state(up);
> serial_omap_enable_ier_thri(up);
> pm_runtime_mark_last_busy(&up->pdev->dev);
> pm_runtime_put_autosuspend(&up->pdev->dev);
> @@ -1421,6 +1468,8 @@ static int serial_omap_probe(struct platform_device *pdev)
> up->port.fifosize = 64;
> up->port.ops = &serial_omap_pops;
>
> + up->max_tx_count = up->port.fifosize / 4;
> +
> if (pdev->dev.of_node)
> up->port.line = of_alias_get_id(pdev->dev.of_node, "serial");
> else
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" 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
* Re: [PATCH 0/2] tty: serial: OMAP: work around broken driver, IP block
From: Tomi Valkeinen @ 2012-01-23 8:15 UTC (permalink / raw)
To: Paul Walmsley
Cc: linux-omap, linux-serial, linux-arm-kernel, Govindraj Raja,
Kevin Hilman, alan
In-Reply-To: <20120121071934.18707.64258.stgit@dusk>
[-- Attachment #1: Type: text/plain, Size: 737 bytes --]
On Sat, 2012-01-21 at 00:27 -0700, Paul Walmsley wrote:
> [ This series is targeted for merging during v3.3-rc ]
>
> On v3.3-rc1, the OMAP serial console doesn't behave properly when
> power management is enabled (the default with omap2plus_defconfig).
> This seems to be due to a combination of a silicon bug in the UART IP
> block and broken FIFO settings in the OMAP serial driver.
>
> This patch series attempts to work around these problems. The workarounds
> are not perfect; see the patch descriptions for more details.
>
> Tested on OMAP3530 BeagleBoard. This series will need to be tested on
> OMAP2 and OMAP4 before it is ready to merge.
Thanks, this makes the console usable on my OMAP3 overo.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* omap-serial RX DMA polling?
From: Paul Walmsley @ 2012-01-23 0:33 UTC (permalink / raw)
To: govindraj.raja; +Cc: khilman, linux-serial, linux-omap, linux-arm-kernel
Hello Govindraj
while trying to track down some of the serial-related PM issues in
v3.3-rc1, I noticed that the omap-serial.c driver sets a 1 microsecond
polling timer when DMA is enabled (uart_dma.rx_timer) (!) This seems
quite broken from both the DMA and PM points of view.
>From a DMA point of view, the DMA transfer should automatically start
when there is data to transfer, and stop when there is no data left, based
on the DMA request lines. So timer-driven polling should not be needed at
all.
>From a PM point of view, this short timer will effectively prevent the MPU
from going into a low-power state whenever there is data in the FIFO.
This will more than erase any energy consumption or CPU efficiency
benefits of doing DMA. Interrupt-driven PIO should be much more efficient
than this, since at least the MPU can enter a low-power state while
waiting for the FIFO to fill.
So basically, the broken timeout calculations used in the interrupt-driven
PIO mode (which set a 1 microsecond PM QoS constraint), plus the 1
microsecond polling timer used in DMA mode, mean that this driver is
pretty bad from a PM perspective.
I sent some patches to fix the interrupt-driven PIO receive part of the
problem, which you've probably seen. But I'm hoping that you can describe
further why the driver needs this RX DMA polling timer? Shouldn't it be
unnecessary? If it's truly unavoidable, then we should presumably not
even bother with RX DMA at all.
- Paul
^ permalink raw reply
* Re: [PATCH 0/2] tty: serial: OMAP: work around broken driver, IP block
From: Paul Walmsley @ 2012-01-22 14:04 UTC (permalink / raw)
To: linux-omap, linux-serial, linux-arm-kernel, alan, greg
Cc: Govindraj Raja, Kevin Hilman, Tomi Valkeinen
In-Reply-To: <20120121071934.18707.64258.stgit@dusk>
On Sat, 21 Jan 2012, Paul Walmsley wrote:
> [ This series is targeted for merging during v3.3-rc ]
>
> On v3.3-rc1, the OMAP serial console doesn't behave properly when
> power management is enabled (the default with omap2plus_defconfig).
> This seems to be due to a combination of a silicon bug in the UART IP
> block and broken FIFO settings in the OMAP serial driver.
>
> This patch series attempts to work around these problems. The workarounds
> are not perfect; see the patch descriptions for more details.
>
> Tested on OMAP3530 BeagleBoard. This series will need to be tested on
> OMAP2 and OMAP4 before it is ready to merge.
Just did a quick test on an OMAP4430 ES2 Pandaboard and the
(OMAP2420-based) N800. This series has no obvious effect on either N800
or OMAP4430 Pandaboard, aside from the expected increase in interrupts in
the non-CPUIdle case. Probably this is due to the relative immaturity of
the PM code on those two chip families.
- Paul
^ permalink raw reply
* Re: [PATCH 0/2] tty: serial: OMAP: work around broken driver, IP block
From: Alan Cox @ 2012-01-21 20:46 UTC (permalink / raw)
To: Paul Walmsley
Cc: Greg KH, linux-omap, linux-serial, linux-arm-kernel,
Govindraj Raja, Kevin Hilman, Tomi Valkeinen
In-Reply-To: <alpine.DEB.2.00.1201211323410.28153@utopia.booyaka.com>
> I will do it, although I do cling to the hope that others will help.
I always work on the basis that if its had some coverage testing then the
other folks who might have been affected should have reviewed the patch.
They opted not to, within reason - their lookout.
They'll only test it later when it his the tree if they don't do it
earlier 8)
Alan
^ permalink raw reply
* Re: [PATCH 0/2] tty: serial: OMAP: work around broken driver, IP block
From: Paul Walmsley @ 2012-01-21 20:31 UTC (permalink / raw)
To: Greg KH
Cc: linux-omap, linux-serial, linux-arm-kernel, Govindraj Raja,
Kevin Hilman, Tomi Valkeinen, alan
In-Reply-To: <20120121125522.GA19027@kroah.com>
On Sat, 21 Jan 2012, Greg KH wrote:
> On Sat, Jan 21, 2012 at 12:27:37AM -0700, Paul Walmsley wrote:
> > [ This series is targeted for merging during v3.3-rc ]
> >
> > On v3.3-rc1, the OMAP serial console doesn't behave properly when
> > power management is enabled (the default with omap2plus_defconfig).
> > This seems to be due to a combination of a silicon bug in the UART IP
> > block and broken FIFO settings in the OMAP serial driver.
> >
> > This patch series attempts to work around these problems. The workarounds
> > are not perfect; see the patch descriptions for more details.
> >
> > Tested on OMAP3530 BeagleBoard. This series will need to be tested on
> > OMAP2 and OMAP4 before it is ready to merge.
>
> Who is going to do that testing?
I will do it, although I do cling to the hope that others will help.
> When?
I'll do it within the next two or three days, unless others beat me to it.
> Why isn't it done already?
That's always the question about testing, isn't it?
In fact, now that you mention it, why haven't these patches been tested
yet on every possible combination of OMAP SoC and console UART?
Unbelievable, how negligent people are.
> I guess this means I can ignore them for now, right?
Yes. I'll follow up with a pull request (or revisions) after I've tested
them on some OMAP2 and OMAP4 boards.
regards
- Paul
^ permalink raw reply
* Re: [PATCH 0/2] tty: serial: OMAP: work around broken driver, IP block
From: Greg KH @ 2012-01-21 12:55 UTC (permalink / raw)
To: Paul Walmsley
Cc: linux-omap, linux-serial, linux-arm-kernel, Govindraj Raja,
Kevin Hilman, Tomi Valkeinen, alan
In-Reply-To: <20120121071934.18707.64258.stgit@dusk>
On Sat, Jan 21, 2012 at 12:27:37AM -0700, Paul Walmsley wrote:
> [ This series is targeted for merging during v3.3-rc ]
>
> On v3.3-rc1, the OMAP serial console doesn't behave properly when
> power management is enabled (the default with omap2plus_defconfig).
> This seems to be due to a combination of a silicon bug in the UART IP
> block and broken FIFO settings in the OMAP serial driver.
>
> This patch series attempts to work around these problems. The workarounds
> are not perfect; see the patch descriptions for more details.
>
> Tested on OMAP3530 BeagleBoard. This series will need to be tested on
> OMAP2 and OMAP4 before it is ready to merge.
Who is going to do that testing? When? Why isn't it done already?
I guess this means I can ignore them for now, right?
greg k-h
^ permalink raw reply
* [PATCH 0/2] tty: serial: OMAP: work around broken driver, IP block
From: Paul Walmsley @ 2012-01-21 7:27 UTC (permalink / raw)
To: linux-omap, linux-serial, linux-arm-kernel
Cc: Govindraj Raja, Kevin Hilman, Tomi Valkeinen, alan
[ This series is targeted for merging during v3.3-rc ]
On v3.3-rc1, the OMAP serial console doesn't behave properly when
power management is enabled (the default with omap2plus_defconfig).
This seems to be due to a combination of a silicon bug in the UART IP
block and broken FIFO settings in the OMAP serial driver.
This patch series attempts to work around these problems. The workarounds
are not perfect; see the patch descriptions for more details.
Tested on OMAP3530 BeagleBoard. This series will need to be tested on
OMAP2 and OMAP4 before it is ready to merge.
- Paul
---
Paul Walmsley (2):
tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA mode
tty: serial: OMAP: transmit FIFO threshold interrupts don't wake the chip
arch/arm/plat-omap/include/plat/omap-serial.h | 1
drivers/tty/serial/omap-serial.c | 86 ++++++++++++++++++++++++-
2 files changed, 82 insertions(+), 5 deletions(-)
^ permalink raw reply
* [PATCH 1/2] tty: serial: OMAP: ensure FIFO levels are set correctly in non-DMA mode
From: Paul Walmsley @ 2012-01-21 7:27 UTC (permalink / raw)
To: linux-omap, linux-serial, linux-arm-kernel
Cc: Govindraj Raja, Kevin Hilman, Tomi Valkeinen, alan
In-Reply-To: <20120121071934.18707.64258.stgit@dusk>
Ensure FIFO levels are set correctly in non-DMA mode (the default).
This patch will cause a receive FIFO threshold interrupt to be raised when
there is at least one byte in the RX FIFO. It will also cause a transmit
FIFO threshold interrupt when there is only one byte remaining in the TX
FIFO.
These changes fix the receive interrupt problem and part of the
transmit interrupt problem. A separate set of issues must be worked
around for the transmit path to have a basic level of functionality; a
subsequent patch will address these.
DMA operation is unaffected by this patch.
Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Govindraj Raja <govindraj.r@ti.com>
Cc: Kevin Hilman <khilman@ti.com>
---
drivers/tty/serial/omap-serial.c | 35 +++++++++++++++++++++++++++++++----
1 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index d192dcb..9de7d71 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -46,6 +46,18 @@
#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)
+
+/* FCR register bitmasks */
+#define OMAP_UART_FCR_RX_FIFO_TRIG_SHIFT 6
+#define OMAP_UART_FCR_RX_FIFO_TRIG_MASK (0x3 << 6)
+#define OMAP_UART_FCR_TX_FIFO_TRIG_SHIFT 4
+
+/* TLR register bitmasks */
+#define OMAP_UART_TLR_TX_FIFO_TRIG_DMA_SHIFT 0
+
static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
/* Forward declaration of functions */
@@ -694,6 +706,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
unsigned char efr = 0;
unsigned long flags = 0;
unsigned int baud, quot;
+ u32 tlr;
switch (termios->c_cflag & CSIZE) {
case CS5:
@@ -811,14 +824,28 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
up->mcr = serial_in(up, UART_MCR);
serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
/* FIFO ENABLE, DMA MODE */
- serial_out(up, UART_FCR, up->fcr);
- serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+
+ up->scr |= OMAP_UART_SCR_TX_TRIG_GRANU1_MASK;
+ up->scr |= OMAP_UART_SCR_RX_TRIG_GRANU1_MASK;
if (up->use_dma) {
- serial_out(up, UART_TI752_TLR, 0);
- up->scr |= (UART_FCR_TRIGGER_4 | UART_FCR_TRIGGER_8);
+ tlr = 0;
+ } else {
+ up->scr &= ~OMAP_UART_SCR_TX_EMPTY;
+
+ /* Set receive FIFO threshold to 1 */
+ up->fcr &= ~OMAP_UART_FCR_RX_FIFO_TRIG_MASK;
+ up->fcr |= (0x1 << OMAP_UART_FCR_RX_FIFO_TRIG_SHIFT);
+
+ /* Set TX FIFO threshold to "63" (actually 1) */
+ up->fcr |= (0x3 << OMAP_UART_FCR_TX_FIFO_TRIG_SHIFT);
+ tlr = (0xf << OMAP_UART_TLR_TX_FIFO_TRIG_DMA_SHIFT);
}
+ serial_out(up, UART_TI752_TLR, tlr);
+ serial_out(up, UART_FCR, up->fcr);
+ serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+
serial_out(up, UART_OMAP_SCR, up->scr);
serial_out(up, UART_EFR, up->efr);
^ permalink raw reply related
* [PATCH 2/2] tty: serial: OMAP: transmit FIFO threshold interrupts don't wake the chip
From: Paul Walmsley @ 2012-01-21 7:27 UTC (permalink / raw)
To: linux-omap, linux-serial, linux-arm-kernel
Cc: Govindraj Raja, Kevin Hilman, Tomi Valkeinen, alan
In-Reply-To: <20120121071934.18707.64258.stgit@dusk>
It seems that when the transmit FIFO threshold is reached on OMAP
UARTs, it does not result in a PRCM wakeup. This appears to be a
silicon bug. This means that if the MPU powerdomain is in a low-power
state, the MPU will not be awakened to refill the FIFO until the next
interrupt from another device.
The best solution, at least for the short term, would be for the OMAP
serial driver to call a OMAP subarchitecture function to prevent the
MPU powerdomain from entering a low power state while the FIFO has
data to transmit. However, we no longer have a clean way to do this,
since patches that add platform_data function pointers have been
deprecated by the OMAP maintainer. So we attempt to work around this
as well. The workarounds depend on the setting of CONFIG_CPU_IDLE.
When CONFIG_CPU_IDLE=n, the driver will now only transmit one byte at
a time. This causes the transmit FIFO threshold interrupt to stay
active until there is no more data to be sent. Thus, the MPU
powerdomain stays on during transmits. Aside from that energy
consumption penalty, each transmitted byte results in a huge number of
UART interrupts -- about five per byte. This wastes CPU time and is
quite inefficient, but is probably the most expedient workaround in
this case.
When CONFIG_CPU_IDLE=y, there is a slightly more direct workaround:
the PM QoS constraint can be abused to keep the MPU powerdomain on.
This results in a normal number of interrupts, but, similar to the
above workaround, wastes power by preventing the MPU from entering
WFI.
Future patches are planned for the 3.4 merge window to implement more
efficient, but also more disruptive, workarounds to these problems.
DMA operation is unaffected by this patch.
Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Govindraj Raja <govindraj.r@ti.com>
Cc: Kevin Hilman <khilman@ti.com>
---
arch/arm/plat-omap/include/plat/omap-serial.h | 1
drivers/tty/serial/omap-serial.c | 51 +++++++++++++++++++++++++
2 files changed, 51 insertions(+), 1 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
index 9ff4444..12a64eb 100644
--- a/arch/arm/plat-omap/include/plat/omap-serial.h
+++ b/arch/arm/plat-omap/include/plat/omap-serial.h
@@ -131,6 +131,7 @@ struct uart_omap_port {
u32 context_loss_cnt;
u32 errata;
u8 wakeups_enabled;
+ u8 max_tx_count;
struct pm_qos_request pm_qos_request;
u32 latency;
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 9de7d71..621fde5 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -88,6 +88,49 @@ static inline void serial_omap_clear_fifos(struct uart_omap_port *up)
serial_out(up, UART_FCR, 0);
}
+/**
+ * serial_omap_block_cpu_low_power_state - prevent MPU pwrdm from leaving ON
+ * @up: struct uart_omap_port *
+ *
+ * Prevent the MPU powerdomain from entering a power state lower than
+ * ON. (It should be sufficient to prevent it from entering INACTIVE,
+ * but there is presently no easy way to do this.) This works around
+ * a suspected silicon bug in the OMAP UART IP blocks. The UARTs should
+ * wake the PRCM when the transmit FIFO threshold interrupt is raised, but
+ * they do not. See also serial_omap_allow_cpu_low_power_state(). No
+ * return value.
+ */
+static void serial_omap_block_cpu_low_power_state(struct uart_omap_port *up)
+{
+#ifdef CONFIG_CPU_IDLE
+ up->latency = 1;
+ schedule_work(&up->qos_work);
+#else
+ up->max_tx_count = 1;
+#endif
+}
+
+/**
+ * serial_omap_allow_cpu_low_power_state - remove power state restriction on MPU
+ * @up: struct uart_omap_port *
+ *
+ * Cancel the effects of serial_omap_block_cpu_low_power_state().
+ * This should allow the MPU powerdomain to enter a power state lower
+ * than ON, assuming the rest of the kernel is not restricting it.
+ * This works around a suspected silicon bug in the OMAP UART IP
+ * blocks. The UARTs should wake the PRCM when the transmit FIFO
+ * threshold interrupt is raised, but they do not. No return value.
+ */
+static void serial_omap_allow_cpu_low_power_state(struct uart_omap_port *up)
+{
+#ifdef CONFIG_CPU_IDLE
+ up->latency = up->calc_latency;
+ schedule_work(&up->qos_work);
+#else
+ up->max_tx_count = up->port.fifosize / 4;
+#endif
+}
+
/*
* serial_omap_get_divisor - calculate divisor value
* @port: uart port info
@@ -163,6 +206,9 @@ static void serial_omap_stop_tx(struct uart_port *port)
serial_out(up, UART_IER, up->ier);
}
+ if (!up->use_dma)
+ serial_omap_allow_cpu_low_power_state(up);
+
pm_runtime_mark_last_busy(&up->pdev->dev);
pm_runtime_put_autosuspend(&up->pdev->dev);
}
@@ -264,7 +310,7 @@ static void transmit_chars(struct uart_omap_port *up)
serial_omap_stop_tx(&up->port);
return;
}
- count = up->port.fifosize / 4;
+ count = up->max_tx_count;
do {
serial_out(up, UART_TX, xmit->buf[xmit->tail]);
xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
@@ -297,6 +343,7 @@ static void serial_omap_start_tx(struct uart_port *port)
if (!up->use_dma) {
pm_runtime_get_sync(&up->pdev->dev);
+ serial_omap_block_cpu_low_power_state(up);
serial_omap_enable_ier_thri(up);
pm_runtime_mark_last_busy(&up->pdev->dev);
pm_runtime_put_autosuspend(&up->pdev->dev);
@@ -1421,6 +1468,8 @@ static int serial_omap_probe(struct platform_device *pdev)
up->port.fifosize = 64;
up->port.ops = &serial_omap_pops;
+ up->max_tx_count = up->port.fifosize / 4;
+
if (pdev->dev.of_node)
up->port.line = of_alias_get_id(pdev->dev.of_node, "serial");
else
^ permalink raw reply related
* Re: [PATCH 3/3] serial: 8250: Add a wakeup_capable module param
From: Rafael J. Wysocki @ 2012-01-20 23:49 UTC (permalink / raw)
To: paulmck; +Cc: Simon Glass, Alan Cox, LKML, Greg Kroah-Hartman, linux-serial
In-Reply-To: <20120120061214.GA2551@linux.vnet.ibm.com>
On Friday, January 20, 2012, Paul E. McKenney wrote:
> On Fri, Jan 20, 2012 at 01:03:34AM +0100, Rafael J. Wysocki wrote:
> > On Thursday, January 19, 2012, Paul E. McKenney wrote:
> > > On Thu, Jan 19, 2012 at 01:02:58AM +0100, Rafael J. Wysocki wrote:
> > > > On Wednesday, January 18, 2012, Paul E. McKenney wrote:
> > > > > On Wed, Jan 18, 2012 at 02:15:59PM -0800, Simon Glass wrote:
> > [...]
> > > > Yes, you can, but then I'd say it's not necessary for user space to
> > > > be able to carry that out in a tight loop. So, it seems, alternatively,
> > > > we could make that loop a bit less tight, e.g. by adding an arbitrary
> > > > sleep to the user space interface for the "disable" case.
> > >
> > > Good point, that would work just as well and be simpler.
> >
> > Thanks for the confirmation! :-)
> >
> > By the way, I wonder, would it help to add synchronize_rcu() to
> > wakeup_source_add() too? Then, even if device_wakeup_enable() and
> > device_wakeup_disable() are executed in a tight loop for the same
> > device, the list_add/list_del operations will always happen in
> > different RCU cycles (or at least it seems so).
>
> I cannot immediately see how adding a synchronize_rcu() to
> wakeup_source_add() would help anything. You only need to wait for a
> grace period on removal, not (normally) on addition. The single grace
> period during removal will catch up all other asynchronous RCU grace
> period requests on that CPU.
>
> Or am I missing your point?
Well, I was thinking about the failure scenario you mentioned where
executing enable/disable in a tight loop might exhaust system memory
(if I understood it correctly).
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH v2] serial: trivial checkpatch fixes in drivers/tty/serial/8250.c
From: Jiri Slaby @ 2012-01-20 15:43 UTC (permalink / raw)
To: Simon Glass
Cc: LKML, Jiri Kosina, Greg Kroah-Hartman, linux-serial, Joe Perches
In-Reply-To: <1327019067-19115-1-git-send-email-sjg@chromium.org>
On 01/20/2012 01:24 AM, Simon Glass wrote:
> @@ -2006,8 +2010,8 @@ static int serial8250_startup(struct uart_port *port)
> */
> if (!(up->port.flags & UPF_BUGGY_UART) &&
> (serial_inp(up, UART_LSR) == 0xff)) {
> - printk_ratelimited(KERN_INFO "ttyS%d: LSR safety check engaged!\n",
> - serial_index(&up->port));
> + pr_info_ratelimited(
FWIW I hate these macros. Beginners ask what the heck stands "pr" for?
> @@ -3053,8 +3063,8 @@ static int __devinit serial8250_probe(struct platform_device *dev)
> port.irqflags |= irqflag;
> ret = serial8250_register_port(&port);
> if (ret < 0) {
> - dev_err(&dev->dev, "unable to register port at index %d "
> - "(IO%lx MEM%llx IRQ%d): %d\n", i,
> + dev_err(&dev->dev, "unable to register port at "
> + "index %d (IO%lx MEM%llx IRQ%d): %d\n", i,
Nack to this. It makes grepping harder.
thanks,
--
js
^ permalink raw reply
* Re: [PATCH 3/5] omap-serial: Fix the error handling in the omap_serial probe
From: Shubhrajyoti @ 2012-01-20 9:35 UTC (permalink / raw)
To: Govindraj; +Cc: linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <CAAL8m4xVOiGjOHvf=Uy4m9QbqxO21pFeB2HcC_zGijj-+VF_jw@mail.gmail.com>
On Friday 20 January 2012 02:19 PM, Govindraj wrote:
> On Mon, Jan 16, 2012 at 3:52 PM, Shubhrajyoti D <shubhrajyoti@ti.com> wrote:
>> The patch does the following
>>
>> - The pm_runtime_disable is called in the remove not in the error
>> case of probe.The patch calls the pm_runtime_disable in the error
>> case.
>> - The up is not freed in the error path. Fix the memory leak by calling
>> kfree in the error path.
>> - Also the iounmap is not called fix the same by calling iounmap in the
>> error case of probe and remove .
>> - Make the name of the error tags more meaningful.
>>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>> drivers/tty/serial/omap-serial.c | 27 +++++++++++++++++----------
>> 1 files changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index 1c24269..8c6f137 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -1369,14 +1369,14 @@ static int serial_omap_probe(struct platform_device *pdev)
>>
>> dma_rx = platform_get_resource_byname(pdev, IORESOURCE_DMA, "rx");
>> if (!dma_rx) {
>> - ret = -EINVAL;
>> - goto err;
>> + ret = -ENXIO;
>> + goto do_release_region;
>> }
>>
>> dma_tx = platform_get_resource_byname(pdev, IORESOURCE_DMA, "tx");
>> if (!dma_tx) {
>> - ret = -EINVAL;
>> - goto err;
>> + ret = -ENXIO;
>> + goto do_release_region;
>> }
>>
>> up = kzalloc(sizeof(*up), GFP_KERNEL);
>> @@ -1403,7 +1403,7 @@ static int serial_omap_probe(struct platform_device *pdev)
>> dev_err(&pdev->dev, "failed to get alias/pdev id, errno %d\n",
>> up->port.line);
>> ret = -ENODEV;
>> - goto err;
>> + goto err_port_line;
>> }
>>
>> sprintf(up->name, "OMAP UART%d", up->port.line);
>> @@ -1412,7 +1412,7 @@ static int serial_omap_probe(struct platform_device *pdev)
>> if (!up->port.membase) {
>> dev_err(&pdev->dev, "can't ioremap UART\n");
>> ret = -ENOMEM;
>> - goto err;
>> + goto err_ioremap;
>> }
>>
>> up->port.flags = omap_up_info->flags;
>> @@ -1458,16 +1458,22 @@ static int serial_omap_probe(struct platform_device *pdev)
>>
>> ret = uart_add_one_port(&serial_omap_reg, &up->port);
>> if (ret != 0)
>> - goto do_release_region;
>> + goto err_add_port;
>>
>> pm_runtime_put(&pdev->dev);
>> platform_set_drvdata(pdev, up);
>> return 0;
>> -err:
>> - dev_err(&pdev->dev, "[UART%d]: failure [%s]: %d\n",
>> - pdev->id, __func__, ret);
>> +
>> +err_add_port:
>> + pm_runtime_disable(&pdev->dev);
>> + iounmap(up->port.membase);
>> +err_ioremap:
>> +err_port_line:
>> + kfree(up);
>> do_release_region:
>> release_mem_region(mem->start, resource_size(mem));
>> + dev_err(&pdev->dev, "[UART%d]: failure [%s]: %d\n",
>> + pdev->id, __func__, ret);
>> return ret;
>> }
>>
>> @@ -1476,6 +1482,7 @@ static int serial_omap_remove(struct platform_device *dev)
>> struct uart_omap_port *up = platform_get_drvdata(dev);
>>
>> if (up) {
>> + iounmap(up->port.membase);
> you can build omap-serial as module insmod and rmmod
> the module and test this patch.
>
> This can be done on zoom board which uses a non-omap uart
> as console.
Yes will do that and post another version.
> --
> Thanks,
> Govindraj.R
^ permalink raw reply
* Re: [PATCH 4/5] ARM : OMAP : serial : Make context_loss_cnt signed
From: Govindraj @ 2012-01-20 8:52 UTC (permalink / raw)
To: Shubhrajyoti D; +Cc: linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <1326709360-26020-5-git-send-email-shubhrajyoti@ti.com>
On Mon, Jan 16, 2012 at 3:52 PM, Shubhrajyoti D <shubhrajyoti@ti.com> wrote:
> get_context_loss_count returns an int however it is stored in
> unsigned integer context_loss_cnt . This patch tries to make
> context_loss_cnt int. So that in case of errors(which may be negative)
> the value is not interpreted wrongly.
>
This change should be part [1] patch of itself
[1]:
[PATCH 5/5] OMAP : serial : Check for error in get_context_loss_count
--
Thanks,
Govindraj.R
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
> arch/arm/plat-omap/include/plat/omap-serial.h | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
> index 9ff4444..b7fb6dc 100644
> --- a/arch/arm/plat-omap/include/plat/omap-serial.h
> +++ b/arch/arm/plat-omap/include/plat/omap-serial.h
> @@ -128,7 +128,7 @@ struct uart_omap_port {
> unsigned char msr_saved_flags;
> char name[20];
> unsigned long port_activity;
> - u32 context_loss_cnt;
> + int context_loss_cnt;
> u32 errata;
> u8 wakeups_enabled;
>
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" 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
* Re: [PATCH 5/5] OMAP : serial : Check for error in get_context_loss_count
From: Govindraj @ 2012-01-20 8:51 UTC (permalink / raw)
To: Shubhrajyoti D; +Cc: linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <1326709360-26020-6-git-send-email-shubhrajyoti@ti.com>
On Mon, Jan 16, 2012 at 3:52 PM, Shubhrajyoti D <shubhrajyoti@ti.com> wrote:
> In serial_omap_runtime_resume in case of errors returned by
> get_context_loss_count print a warning and do a restore.
>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
> drivers/tty/serial/omap-serial.c | 10 ++++++++--
> 1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 8c6f137..e1c1a0f 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -1602,10 +1602,16 @@ static int serial_omap_runtime_resume(struct device *dev)
>
> if (up) {
> if (pdata->get_context_loss_count) {
> - u32 loss_cnt = pdata->get_context_loss_count(dev);
> + int loss_cnt = pdata->get_context_loss_count(dev);
Looks ok to me,
Can you ensure off mode is tested with this patch.
--
Thanks,
Govindraj.R
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" 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
* Re: [PATCH 3/5] omap-serial: Fix the error handling in the omap_serial probe
From: Govindraj @ 2012-01-20 8:49 UTC (permalink / raw)
To: Shubhrajyoti D; +Cc: linux-serial, linux-omap, linux-arm-kernel
In-Reply-To: <1326709360-26020-4-git-send-email-shubhrajyoti@ti.com>
On Mon, Jan 16, 2012 at 3:52 PM, Shubhrajyoti D <shubhrajyoti@ti.com> wrote:
> The patch does the following
>
> - The pm_runtime_disable is called in the remove not in the error
> case of probe.The patch calls the pm_runtime_disable in the error
> case.
> - The up is not freed in the error path. Fix the memory leak by calling
> kfree in the error path.
> - Also the iounmap is not called fix the same by calling iounmap in the
> error case of probe and remove .
> - Make the name of the error tags more meaningful.
>
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
> drivers/tty/serial/omap-serial.c | 27 +++++++++++++++++----------
> 1 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 1c24269..8c6f137 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -1369,14 +1369,14 @@ static int serial_omap_probe(struct platform_device *pdev)
>
> dma_rx = platform_get_resource_byname(pdev, IORESOURCE_DMA, "rx");
> if (!dma_rx) {
> - ret = -EINVAL;
> - goto err;
> + ret = -ENXIO;
> + goto do_release_region;
> }
>
> dma_tx = platform_get_resource_byname(pdev, IORESOURCE_DMA, "tx");
> if (!dma_tx) {
> - ret = -EINVAL;
> - goto err;
> + ret = -ENXIO;
> + goto do_release_region;
> }
>
> up = kzalloc(sizeof(*up), GFP_KERNEL);
> @@ -1403,7 +1403,7 @@ static int serial_omap_probe(struct platform_device *pdev)
> dev_err(&pdev->dev, "failed to get alias/pdev id, errno %d\n",
> up->port.line);
> ret = -ENODEV;
> - goto err;
> + goto err_port_line;
> }
>
> sprintf(up->name, "OMAP UART%d", up->port.line);
> @@ -1412,7 +1412,7 @@ static int serial_omap_probe(struct platform_device *pdev)
> if (!up->port.membase) {
> dev_err(&pdev->dev, "can't ioremap UART\n");
> ret = -ENOMEM;
> - goto err;
> + goto err_ioremap;
> }
>
> up->port.flags = omap_up_info->flags;
> @@ -1458,16 +1458,22 @@ static int serial_omap_probe(struct platform_device *pdev)
>
> ret = uart_add_one_port(&serial_omap_reg, &up->port);
> if (ret != 0)
> - goto do_release_region;
> + goto err_add_port;
>
> pm_runtime_put(&pdev->dev);
> platform_set_drvdata(pdev, up);
> return 0;
> -err:
> - dev_err(&pdev->dev, "[UART%d]: failure [%s]: %d\n",
> - pdev->id, __func__, ret);
> +
> +err_add_port:
> + pm_runtime_disable(&pdev->dev);
> + iounmap(up->port.membase);
> +err_ioremap:
> +err_port_line:
> + kfree(up);
> do_release_region:
> release_mem_region(mem->start, resource_size(mem));
> + dev_err(&pdev->dev, "[UART%d]: failure [%s]: %d\n",
> + pdev->id, __func__, ret);
> return ret;
> }
>
> @@ -1476,6 +1482,7 @@ static int serial_omap_remove(struct platform_device *dev)
> struct uart_omap_port *up = platform_get_drvdata(dev);
>
> if (up) {
> + iounmap(up->port.membase);
you can build omap-serial as module insmod and rmmod
the module and test this patch.
This can be done on zoom board which uses a non-omap uart
as console.
--
Thanks,
Govindraj.R
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" 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