[PATCH v2 0/3] tty: serial: OMAP: work around broken IP block, driver

[PATCH v2 0/3] tty: serial: OMAP: work around broken IP block, driver

[Paul Walmsley]

[ This series is targeted for merging during v3.3-rc ]

Hi

Here's an updated version of OMAP serial bugfix series against v3.3-rc1.
This revision has:

- reduced TX path interrupts by 5x compared to the first version

- a fix for the power management regression in v3.3-rc1 caused by the
  bogus wakeup latency computation

- left the the TX FIFO threshold unchanged - this is left for a 3.4 patch
  series

- improved commit messages

This series is also available via git in git://git.pwsan.com/linux-2.6
in the branch "omap_serial_fixes_3.3rc".

...

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 one or more silicon bugs in the UART IP block
and a bug in the OMAP serial driver.

This patch series works around these problems.  It's been tested under
the following conditions:

On 35xx Beagleboard
 - in PIO mode
   - with CPUidle enabled
     - with off-mode disabled
     - with off-mode enabled
   - with CPUidle disabled
     - with off-mode disabled
     - with off-mode enabled
 - in DMA mode
   - with CPUidle enabled
     - with off-mode disabled
     - with off-mode enabled
   - with CPUidle disabled
     - with off-mode disabled
     - with off-mode enabled

On N800 (242x)
 - in PIO mode
   - with CPUidle disabled

On 4430 ES2 Pandaboard
 - in PIO mode
   - with CPUidle disabled
 - in DMA mode
   - with CPUidle disabled


- Paul

---

omap_serial_fixes_3.3rc
   text	   data	    bss	    dec	    hex	filename
6592293	 678588	5590684	12861565	 c4407d	vmlinux.orig
6592429	 678588	5590684	12861701	 c44105	vmlinux.patched


Paul Walmsley (3):
      tty: serial: OMAP: use a 1-byte RX FIFO threshold in PIO mode
      tty: serial: OMAP: block idle while the UART is transferring data in PIO mode
      tty: serial: omap-serial: wakeup latency constraint is in microseconds, not milliseconds


 arch/arm/mach-omap2/serial.c     |    8 ++++----
 drivers/tty/serial/omap-serial.c |   30 +++++++++++++++++++++++++-----
 2 files changed, 29 insertions(+), 9 deletions(-)

[PATCH v2 1/3] tty: serial: OMAP: use a 1-byte RX FIFO threshold in PIO mode

[Paul Walmsley]

In the (default) PIO mode, use a one-byte RX FIFO threshold.  The OMAP
UART IP blocks do not appear to be capable of waking the system under
an RX timeout condition.  Since the previous RX FIFO threshold was 16
bytes, this meant that omap-serial.c did not become aware of any
received data until all those bytes arrived or until another UART
interrupt occurred.  This made the serial console and presumably other
serial applications (GPS, serial Bluetooth) unusable or extremely
slow.  A 1-byte RX FIFO threshold also allows the MPU to enter a
low-power consumption state while waiting for the FIFO to fill.

This can be verified using the serial console by comparing the
behavior when "0123456789abcde" is pasted in from another window, with
the behavior when "0123456789abcdef" is pasted in.  Since the former
string is less than sixteen bytes long, the string is not echoed for
some time, while the latter string is echoed immediately.

DMA operation is unaffected by this patch.

Thanks to Russell King - ARM Linux <linux@arm.linux.org.uk> for some
additional information on the standard behavior of the RX timeout
event, which was used to improve this commit description.

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>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Russell King - ARM Linux <linux@arm.linux.org.uk>
---
 drivers/tty/serial/omap-serial.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index d192dcb..c9c9ba2 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -46,6 +46,13 @@
 
 #define DEFAULT_CLK_SPEED 48000000 /* 48Mhz*/
 
+/* SCR register bitmasks */
+#define OMAP_UART_SCR_RX_TRIG_GRANU1_MASK		(1 << 7)
+
+/* FCR register bitmasks */
+#define OMAP_UART_FCR_RX_FIFO_TRIG_SHIFT		6
+#define OMAP_UART_FCR_RX_FIFO_TRIG_MASK			(0x3 << 6)
+
 static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
 
 /* Forward declaration of functions */
@@ -811,14 +818,21 @@ 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_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);
+		up->scr |= UART_FCR_TRIGGER_4;
+	} else {
+		/* Set receive FIFO threshold to 1 byte */
+		up->fcr &= ~OMAP_UART_FCR_RX_FIFO_TRIG_MASK;
+		up->fcr |= (0x1 << OMAP_UART_FCR_RX_FIFO_TRIG_SHIFT);
 	}
 
+	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);

[PATCH 2/3] tty: serial: OMAP: block idle while the UART is transferring data in PIO mode

[Paul Walmsley]

Prevent OMAP UARTs from going idle while they are still transferring
data in PIO mode.  This works around an oversight in the OMAP UART
hardware present in OMAP34xx and earlier: an idle UART won't send a
wakeup when the TX FIFO threshold is reached.  This causes long delays
during data transmission when the MPU powerdomain enters a low-power
mode.  The MPU interrupt controller is not able to respond to
interrupts when it's in a low-power state, so the TX buffer is not
refilled until another wakeup event occurs.

This fix changes the erratum i291 DMA idle workaround.  Rather than
toggling between force-idle and no-idle, it will toggle between
smart-idle and no-idle.  The important part of the workaround is the
no-idle part, so this shouldn't result in any change in behavior.

This fix should work on all OMAP UARTs.  Future patches intended for
the 3.4 merge window will make this workaround conditional on a
"feature" flag, and will use the OMAP36xx+ TX event wakeup support.

Thanks to Kevin Hilman <khilman@ti.com> for mentioning the erratum i291
workaround, which led to the development of this approach.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Govindraj.R <govindraj.raja@ti.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 arch/arm/mach-omap2/serial.c     |    8 ++++----
 drivers/tty/serial/omap-serial.c |    7 +++++++
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 247d894..f590afc 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -107,18 +107,18 @@ static void omap_uart_set_noidle(struct platform_device *pdev)
 	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_NO);
 }
 
-static void omap_uart_set_forceidle(struct platform_device *pdev)
+static void omap_uart_set_smartidle(struct platform_device *pdev)
 {
 	struct omap_device *od = to_omap_device(pdev);
 
-	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_FORCE);
+	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_SMART);
 }
 
 #else
 static void omap_uart_enable_wakeup(struct platform_device *pdev, bool enable)
 {}
 static void omap_uart_set_noidle(struct platform_device *pdev) {}
-static void omap_uart_set_forceidle(struct platform_device *pdev) {}
+static void omap_uart_set_smartidle(struct platform_device *pdev) {}
 #endif /* CONFIG_PM */
 
 #ifdef CONFIG_OMAP_MUX
@@ -349,7 +349,7 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
 	omap_up.uartclk = OMAP24XX_BASE_BAUD * 16;
 	omap_up.flags = UPF_BOOT_AUTOCONF;
 	omap_up.get_context_loss_count = omap_pm_get_dev_context_loss_count;
-	omap_up.set_forceidle = omap_uart_set_forceidle;
+	omap_up.set_forceidle = omap_uart_set_smartidle;
 	omap_up.set_noidle = omap_uart_set_noidle;
 	omap_up.enable_wakeup = omap_uart_enable_wakeup;
 	omap_up.dma_rx_buf_size = info->dma_rx_buf_size;
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index c9c9ba2..11fa156 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -136,6 +136,7 @@ static void serial_omap_enable_ms(struct uart_port *port)
 static void serial_omap_stop_tx(struct uart_port *port)
 {
 	struct uart_omap_port *up = (struct uart_omap_port *)port;
+	struct omap_uart_port_info *pdata = up->pdev->dev.platform_data;
 
 	if (up->use_dma &&
 		up->uart_dma.tx_dma_channel != OMAP_UART_DMA_CH_FREE) {
@@ -158,6 +159,9 @@ 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);
 }
@@ -286,6 +290,7 @@ static inline void serial_omap_enable_ier_thri(struct uart_omap_port *up)
 static void serial_omap_start_tx(struct uart_port *port)
 {
 	struct uart_omap_port *up = (struct uart_omap_port *)port;
+	struct omap_uart_port_info *pdata = up->pdev->dev.platform_data;
 	struct circ_buf *xmit;
 	unsigned int start;
 	int ret = 0;
@@ -293,6 +298,8 @@ static void serial_omap_start_tx(struct uart_port *port)
 	if (!up->use_dma) {
 		pm_runtime_get_sync(&up->pdev->dev);
 		serial_omap_enable_ier_thri(up);
+		if (pdata->set_noidle)
+			pdata->set_noidle(up->pdev);
 		pm_runtime_mark_last_busy(&up->pdev->dev);
 		pm_runtime_put_autosuspend(&up->pdev->dev);
 		return;

[PATCH v2 3/3] tty: serial: omap-serial: wakeup latency constraint is in microseconds, not milliseconds

[Paul Walmsley]

The receive FIFO wakeup latency estimate in the omap-serial driver is
three orders of magnitude too small.  This effectively prevents the
MPU from going to a low-power state when CONFIG_CPU_IDLE=y.  This is a
major power management regression and masks some other FIFO-related
bugs in the driver.

Fix by correcting the most egregious problem in the RX wakeup latency
estimate.  There are several other flaws in the estimator; these will
be fixed by a separate patch series intended for 3.4.

The difference in low-power states with this patch can be observed via
debugfs in pm_debug/count.

This estimate does not have any effect when CONFIG_CPU_IDLE=n.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Govindraj.R <govindraj.raja@ti.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/tty/serial/omap-serial.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 11fa156..72fa783 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -740,8 +740,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 	quot = serial_omap_get_divisor(port, baud);
 
 	/* calculate wakeup latency constraint */
-	up->calc_latency = (1000000 * up->port.fifosize) /
-				(1000 * baud / 8);
+	up->calc_latency = (USEC_PER_SEC * up->port.fifosize) / (baud / 8);
 	up->latency = up->calc_latency;
 	schedule_work(&up->qos_work);
 

[PATCH 2/3] tty: serial: OMAP: block idle while the UART is transferring data in PIO mode

[Paul Walmsley]

Hi

The subject line on this patch should have been 

"[PATCH v2 2/3] tty: serial: OMAP: block idle while the UART is 
          transferring data in PIO mode"


- Paul

[PATCH v2 3/3] tty: serial: omap-serial: wakeup latency constraint is in microseconds, not milliseconds

[Govindraj]

On Thu, Jan 26, 2012 at 8:20 AM, Paul Walmsley <paul@pwsan.com> wrote:
> The receive FIFO wakeup latency estimate in the omap-serial driver is
> three orders of magnitude too small. ?This effectively prevents the
> MPU from going to a low-power state when CONFIG_CPU_IDLE=y. ?This is a
> major power management regression and masks some other FIFO-related
> bugs in the driver.
>
> Fix by correcting the most egregious problem in the RX wakeup latency
> estimate. ?There are several other flaws in the estimator; these will
> be fixed by a separate patch series intended for 3.4.
>
> The difference in low-power states with this patch can be observed via
> debugfs in pm_debug/count.
>
> This estimate does not have any effect when CONFIG_CPU_IDLE=n.
>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Kevin Hilman <khilman@ti.com>
> Cc: Govindraj.R <govindraj.raja@ti.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Alan Cox <alan@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>

Acked-by: Govindraj.R <govindraj.raja@ti.com>

> ---
> ?drivers/tty/serial/omap-serial.c | ? ?3 +--
> ?1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 11fa156..72fa783 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -740,8 +740,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
> ? ? ? ?quot = serial_omap_get_divisor(port, baud);
>
> ? ? ? ?/* calculate wakeup latency constraint */
> - ? ? ? up->calc_latency = (1000000 * up->port.fifosize) /
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (1000 * baud / 8);
> + ? ? ? up->calc_latency = (USEC_PER_SEC * up->port.fifosize) / (baud / 8);
> ? ? ? ?up->latency = up->calc_latency;
> ? ? ? ?schedule_work(&up->qos_work);
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] tty: serial: OMAP: block idle while the UART is transferring data in PIO mode

[Govindraj]

On Thu, Jan 26, 2012 at 8:20 AM, Paul Walmsley <paul@pwsan.com> wrote:
> Prevent OMAP UARTs from going idle while they are still transferring
> data in PIO mode. ?This works around an oversight in the OMAP UART
> hardware present in OMAP34xx and earlier: an idle UART won't send a
> wakeup when the TX FIFO threshold is reached. ?This causes long delays
> during data transmission when the MPU powerdomain enters a low-power
> mode. ?The MPU interrupt controller is not able to respond to
> interrupts when it's in a low-power state, so the TX buffer is not
> refilled until another wakeup event occurs.
>
> This fix changes the erratum i291 DMA idle workaround. ?Rather than
> toggling between force-idle and no-idle, it will toggle between
> smart-idle and no-idle. ?The important part of the workaround is the
> no-idle part, so this shouldn't result in any change in behavior.
>
> This fix should work on all OMAP UARTs. ?Future patches intended for
> the 3.4 merge window will make this workaround conditional on a
> "feature" flag, and will use the OMAP36xx+ TX event wakeup support.
>
> Thanks to Kevin Hilman <khilman@ti.com> for mentioning the erratum i291
> workaround, which led to the development of this approach.
>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Kevin Hilman <khilman@ti.com>
> Cc: Govindraj.R <govindraj.raja@ti.com>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>
> Cc: Alan Cox <alan@linux.intel.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>

Acked-by: Govindraj.R <govindraj.raja@ti.com>

> ---
> ?arch/arm/mach-omap2/serial.c ? ? | ? ?8 ++++----
> ?drivers/tty/serial/omap-serial.c | ? ?7 +++++++
> ?2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 247d894..f590afc 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -107,18 +107,18 @@ static void omap_uart_set_noidle(struct platform_device *pdev)
> ? ? ? ?omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_NO);
> ?}
>
> -static void omap_uart_set_forceidle(struct platform_device *pdev)
> +static void omap_uart_set_smartidle(struct platform_device *pdev)
> ?{
> ? ? ? ?struct omap_device *od = to_omap_device(pdev);
>
> - ? ? ? omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_FORCE);
> + ? ? ? omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_SMART);
> ?}
>
> ?#else
> ?static void omap_uart_enable_wakeup(struct platform_device *pdev, bool enable)
> ?{}
> ?static void omap_uart_set_noidle(struct platform_device *pdev) {}
> -static void omap_uart_set_forceidle(struct platform_device *pdev) {}
> +static void omap_uart_set_smartidle(struct platform_device *pdev) {}
> ?#endif /* CONFIG_PM */
>
> ?#ifdef CONFIG_OMAP_MUX
> @@ -349,7 +349,7 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
> ? ? ? ?omap_up.uartclk = OMAP24XX_BASE_BAUD * 16;
> ? ? ? ?omap_up.flags = UPF_BOOT_AUTOCONF;
> ? ? ? ?omap_up.get_context_loss_count = omap_pm_get_dev_context_loss_count;
> - ? ? ? omap_up.set_forceidle = omap_uart_set_forceidle;
> + ? ? ? omap_up.set_forceidle = omap_uart_set_smartidle;
> ? ? ? ?omap_up.set_noidle = omap_uart_set_noidle;
> ? ? ? ?omap_up.enable_wakeup = omap_uart_enable_wakeup;
> ? ? ? ?omap_up.dma_rx_buf_size = info->dma_rx_buf_size;
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index c9c9ba2..11fa156 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -136,6 +136,7 @@ static void serial_omap_enable_ms(struct uart_port *port)
> ?static void serial_omap_stop_tx(struct uart_port *port)
> ?{
> ? ? ? ?struct uart_omap_port *up = (struct uart_omap_port *)port;
> + ? ? ? struct omap_uart_port_info *pdata = up->pdev->dev.platform_data;
>
> ? ? ? ?if (up->use_dma &&
> ? ? ? ? ? ? ? ?up->uart_dma.tx_dma_channel != OMAP_UART_DMA_CH_FREE) {
> @@ -158,6 +159,9 @@ 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);
> ?}
> @@ -286,6 +290,7 @@ static inline void serial_omap_enable_ier_thri(struct uart_omap_port *up)
> ?static void serial_omap_start_tx(struct uart_port *port)
> ?{
> ? ? ? ?struct uart_omap_port *up = (struct uart_omap_port *)port;
> + ? ? ? struct omap_uart_port_info *pdata = up->pdev->dev.platform_data;
> ? ? ? ?struct circ_buf *xmit;
> ? ? ? ?unsigned int start;
> ? ? ? ?int ret = 0;
> @@ -293,6 +298,8 @@ static void serial_omap_start_tx(struct uart_port *port)
> ? ? ? ?if (!up->use_dma) {
> ? ? ? ? ? ? ? ?pm_runtime_get_sync(&up->pdev->dev);
> ? ? ? ? ? ? ? ?serial_omap_enable_ier_thri(up);
> + ? ? ? ? ? ? ? if (pdata->set_noidle)
> + ? ? ? ? ? ? ? ? ? ? ? pdata->set_noidle(up->pdev);
> ? ? ? ? ? ? ? ?pm_runtime_mark_last_busy(&up->pdev->dev);
> ? ? ? ? ? ? ? ?pm_runtime_put_autosuspend(&up->pdev->dev);
> ? ? ? ? ? ? ? ?return;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/3] tty: serial: OMAP: work around broken IP block, driver

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> [ This series is targeted for merging during v3.3-rc ]
>
> Hi
>
> Here's an updated version of OMAP serial bugfix series against v3.3-rc1.
> This revision has:

Reviewed-by: Kevin Hilman <khilman@ti.com>
Tested-by: Kevin Hilman <khilman@ti.com>

Tested on 3430/n900, 3530/Overo and 4430/Panda

Kevin

[PATCH 2/3] tty: serial: OMAP: block idle while the UART is transferring data in PIO mode

[Cousson, Benoit]

+ Rajendra

Hi Paul,

3.3-rc4 is broken in the DT case because of the serial driver. And it looks like it is due to this fix.
We cannot rely on pdata anymore in DT, and in that case it leads to an Oops due to NULL pdata.

[    2.120605] Unable to handle kernel NULL pointer dereference at virtual address 00000028
[    2.120605] pgd = ed568000
[    2.131958] [00000028] *pgd=ad73f831, *pte=00000000, *ppte=00000000
[    2.131958] Internal error: Oops: 17 [#1] SMP
[    2.131958] Modules linked in:
[    2.131958] CPU: 1    Not tainted  (3.3.0-rc4-00001-g6851380-dirty #244)
[    2.153350] PC is at serial_omap_start_tx+0x1c0/0x20c
[    2.153350] LR is at serial_omap_start_tx+0x1b8/0x20c
[    2.153350] pc : [<c02b4244>]    lr : [<c02b423c>]    psr: 60000193
[    2.163940] sp : ed579e68  ip : c07024b0  fp : a0000113
[    2.163940] r10: ed780800  r9 : ed6aca00  r8 : 00000017
[    2.163940] r7 : 00000004  r6 : 00000007  r5 : 00000000  r4 : ed6aca00
[    2.188262] r3 : ef0ef540  r2 : fa020000  r1 : 00000000  r0 : c02b423c
[    2.188262] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
[    2.188262] Control: 10c53c7d  Table: ad56804a  DAC: 00000015
[    2.188262] Process rcS (pid: 494, stack limit = 0xed5782f8)
[    2.214630] Stack: (0xed579e68 to 0xed57a000)
[    2.214630] 9e60:                   ed6aca00 ed780800 a0000113 c0476f94 00000002 ed6aca00
[    2.227752] 9e80: ed780800 20000113 00000000 c02ac2ec 60000193 c02acbd4 00000000 ed6a22d0
[    2.236328] 9ea0: ef0bf017 c02ae270 00000017 ed780800 ed780cfc 00000017 ef0bf000 ed578000
[    2.236328] 9ec0: c049dde8 ef0b5380 00000000 c0297488 60000113 c0292f84 ef0bf000 ed780994
[    2.236328] 9ee0: ef0003c0 00000000 ef0ef540 c0073cfc ed7809b4 ed7809b4 ed780cb4 ef0b5380
[    2.236328] 9f00: ed780800 b6f15000 00000017 ed578000 00000400 00000000 00000000 c0292fd0
[    2.236328] 9f20: c06e413c 00000017 c02972b4 ed778800 ed579f80 ed578000 00000000 ef0b5380
[    2.236328] 9f40: 00000017 b6f15000 ed579f80 00000017 ed578000 00000000 b6f16000 c0107df8
[    2.287750] 9f60: c0013ec0 ef0ef540 ef0b5380 b6f15000 00000000 00000000 00000017 c0108080
[    2.287750] 9f80: 00000000 00000000 b6e47600 00000000 00000017 b6f15000 b6e47600 00000004
[    2.287750] 9fa0: c0013f68 c0013da0 00000017 b6f15000 00000001 b6f15000 00000017 00000000
[    2.287750] 9fc0: 00000017 b6f15000 b6e47600 00000004 00000017 00000000 00000001 b6f16000
[    2.287750] 9fe0: b6f15000 beb025d8 b6d8d120 b6ddb1bc 60000110 00000001 0da805a1 84808223
[    2.330627] [<c02b4244>] (serial_omap_start_tx+0x1c0/0x20c) from [<c02ac2ec>] (__uart_start+0x40/0x44)
[    2.330627] [<c02ac2ec>] (__uart_start+0x40/0x44) from [<c02acbd4>] (uart_start+0x24/0x34)
[    2.340393] [<c02acbd4>] (uart_start+0x24/0x34) from [<c02ae270>] (uart_write+0xc0/0xe4)
[    2.349060] [<c02ae270>] (uart_write+0xc0/0xe4) from [<c0297488>] (n_tty_write+0x1d4/0x404)
[    2.349060] [<c0297488>] (n_tty_write+0x1d4/0x404) from [<c0292fd0>] (tty_write+0x138/0x22c)
[    2.366302] [<c0292fd0>] (tty_write+0x138/0x22c) from [<c0107df8>] (vfs_write+0xb4/0x148)
[    2.366302] [<c0107df8>] (vfs_write+0xb4/0x148) from [<c0108080>] (sys_write+0x40/0x70)
[    2.366302] [<c0108080>] (sys_write+0x40/0x70) from [<c0013da0>] (ret_fast_syscall+0x0/0x3c)


I guess we should stop accessing the pdata from the driver except during probe and populate the needed information inside the drvdata instead.

And then we will have to add the support for all these OMAP custom hooks without pdata.

A basic fix (below) for the moment is to test for valid pdata inside the driver.
I'll repost it properly if you are fine with it.

Regards,
Benoit

---
>From af9f18e15e0ef0e227b3efa42489b7bd8a20c2a9 Mon Sep 17 00:00:00 2001
From: Benoit Cousson <b-cousson@ti.com>
Date: Mon, 20 Feb 2012 12:19:24 +0100
Subject: [PATCH] tty: serial: OMAP: Fix oops due to NULL pdata in DT boot

The following commit: be4b0281956c5cae4f63f31f11d07625a6988766
(tty: serial: OMAP: block idle while the UART is transferring data in PIO mode),
is introducing a oops if OMAP is booted using device tree blob because
the pdata will not be initialized.

Check if pdata is set before de-referencing it.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
---
 drivers/tty/serial/omap-serial.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index f809041..0121486 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -159,7 +159,7 @@ static void serial_omap_stop_tx(struct uart_port *port)
 		serial_out(up, UART_IER, up->ier);
 	}
 
-	if (!up->use_dma && pdata->set_forceidle)
+	if (!up->use_dma && pdata && pdata->set_forceidle)
 		pdata->set_forceidle(up->pdev);
 
 	pm_runtime_mark_last_busy(&up->pdev->dev);
@@ -298,7 +298,7 @@ static void serial_omap_start_tx(struct uart_port *port)
 	if (!up->use_dma) {
 		pm_runtime_get_sync(&up->pdev->dev);
 		serial_omap_enable_ier_thri(up);
-		if (pdata->set_noidle)
+		if (pdata && pdata->set_noidle)
 			pdata->set_noidle(up->pdev);
 		pm_runtime_mark_last_busy(&up->pdev->dev);
 		pm_runtime_put_autosuspend(&up->pdev->dev);
@@ -1613,7 +1613,7 @@ static int serial_omap_runtime_resume(struct device *dev)
 	struct uart_omap_port *up = dev_get_drvdata(dev);
 	struct omap_uart_port_info *pdata = dev->platform_data;
 
-	if (up) {
+	if (up && pdata) {
 		if (pdata->get_context_loss_count) {
 			u32 loss_cnt = pdata->get_context_loss_count(dev);
 
-- 
1.7.0.4


On 1/26/2012 3:50 AM, Paul Walmsley wrote:
> Prevent OMAP UARTs from going idle while they are still transferring
> data in PIO mode.  This works around an oversight in the OMAP UART
> hardware present in OMAP34xx and earlier: an idle UART won't send a
> wakeup when the TX FIFO threshold is reached.  This causes long delays
> during data transmission when the MPU powerdomain enters a low-power
> mode.  The MPU interrupt controller is not able to respond to
> interrupts when it's in a low-power state, so the TX buffer is not
> refilled until another wakeup event occurs.
> 
> This fix changes the erratum i291 DMA idle workaround.  Rather than
> toggling between force-idle and no-idle, it will toggle between
> smart-idle and no-idle.  The important part of the workaround is the
> no-idle part, so this shouldn't result in any change in behavior.
> 
> This fix should work on all OMAP UARTs.  Future patches intended for
> the 3.4 merge window will make this workaround conditional on a
> "feature" flag, and will use the OMAP36xx+ TX event wakeup support.
> 
> Thanks to Kevin Hilman<khilman@ti.com>  for mentioning the erratum i291
> workaround, which led to the development of this approach.
> 
> Signed-off-by: Paul Walmsley<paul@pwsan.com>
> Cc: Kevin Hilman<khilman@ti.com>
> Cc: Govindraj.R<govindraj.raja@ti.com>
> Cc: Greg Kroah-Hartman<gregkh@suse.de>
> Cc: Alan Cox<alan@linux.intel.com>
> Cc: Tomi Valkeinen<tomi.valkeinen@ti.com>
> ---
>   arch/arm/mach-omap2/serial.c     |    8 ++++----
>   drivers/tty/serial/omap-serial.c |    7 +++++++
>   2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 247d894..f590afc 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -107,18 +107,18 @@ static void omap_uart_set_noidle(struct platform_device *pdev)
>   	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_NO);
>   }
> 
> -static void omap_uart_set_forceidle(struct platform_device *pdev)
> +static void omap_uart_set_smartidle(struct platform_device *pdev)
>   {
>   	struct omap_device *od = to_omap_device(pdev);
> 
> -	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_FORCE);
> +	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_SMART);
>   }
> 
>   #else
>   static void omap_uart_enable_wakeup(struct platform_device *pdev, bool enable)
>   {}
>   static void omap_uart_set_noidle(struct platform_device *pdev) {}
> -static void omap_uart_set_forceidle(struct platform_device *pdev) {}
> +static void omap_uart_set_smartidle(struct platform_device *pdev) {}
>   #endif /* CONFIG_PM */
> 
>   #ifdef CONFIG_OMAP_MUX
> @@ -349,7 +349,7 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
>   	omap_up.uartclk = OMAP24XX_BASE_BAUD * 16;
>   	omap_up.flags = UPF_BOOT_AUTOCONF;
>   	omap_up.get_context_loss_count = omap_pm_get_dev_context_loss_count;
> -	omap_up.set_forceidle = omap_uart_set_forceidle;
> +	omap_up.set_forceidle = omap_uart_set_smartidle;
>   	omap_up.set_noidle = omap_uart_set_noidle;
>   	omap_up.enable_wakeup = omap_uart_enable_wakeup;
>   	omap_up.dma_rx_buf_size = info->dma_rx_buf_size;
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index c9c9ba2..11fa156 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -136,6 +136,7 @@ static void serial_omap_enable_ms(struct uart_port *port)
>   static void serial_omap_stop_tx(struct uart_port *port)
>   {
>   	struct uart_omap_port *up = (struct uart_omap_port *)port;
> +	struct omap_uart_port_info *pdata = up->pdev->dev.platform_data;
> 
>   	if (up->use_dma&&
>   		up->uart_dma.tx_dma_channel != OMAP_UART_DMA_CH_FREE) {
> @@ -158,6 +159,9 @@ 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);
>   }
> @@ -286,6 +290,7 @@ static inline void serial_omap_enable_ier_thri(struct uart_omap_port *up)
>   static void serial_omap_start_tx(struct uart_port *port)
>   {
>   	struct uart_omap_port *up = (struct uart_omap_port *)port;
> +	struct omap_uart_port_info *pdata = up->pdev->dev.platform_data;
>   	struct circ_buf *xmit;
>   	unsigned int start;
>   	int ret = 0;
> @@ -293,6 +298,8 @@ static void serial_omap_start_tx(struct uart_port *port)
>   	if (!up->use_dma) {
>   		pm_runtime_get_sync(&up->pdev->dev);
>   		serial_omap_enable_ier_thri(up);
> +		if (pdata->set_noidle)
> +			pdata->set_noidle(up->pdev);
>   		pm_runtime_mark_last_busy(&up->pdev->dev);
>   		pm_runtime_put_autosuspend(&up->pdev->dev);
>   		return;
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] tty: serial: OMAP: block idle while the UART is transferring data in PIO mode

[Paul Walmsley]

Salut Beno?t,

On Mon, 20 Feb 2012, Cousson, Benoit wrote:

> 3.3-rc4 is broken in the DT case because of the serial driver. And it 
> looks like it is due to this fix.  We cannot rely on pdata anymore in 
> DT, and in that case it leads to an Oops due to NULL pdata.

Sorry about the breakage.  I agree with the diagnosis.  That code was just 
copied from the DMA errata part of the driver, so it will need to be fixed 
as well.  Otherwise the same problem will happen when DMA is enabled.

> And then we will have to add the support for all these OMAP custom hooks without pdata.

That's really the key question for the medium- to long-term...

> A basic fix (below) for the moment is to test for valid pdata inside the driver.
> I'll repost it properly if you are fine with it.

Looks fine to me, although I'd suggest fixing the DMA workaround path as 
well.

Reviewed-by: Paul Walmsley <paul@pwsan.com>


- Paul

[PATCH 2/3] tty: serial: OMAP: block idle while the UART is transferring data in PIO mode

[Cousson, Benoit]

Salut Paul,

On 2/21/2012 11:02 PM, Paul Walmsley wrote:
> Salut Beno?t,
>
> On Mon, 20 Feb 2012, Cousson, Benoit wrote:
>
>> 3.3-rc4 is broken in the DT case because of the serial driver. And it
>> looks like it is due to this fix.  We cannot rely on pdata anymore in
>> DT, and in that case it leads to an Oops due to NULL pdata.
>
> Sorry about the breakage.  I agree with the diagnosis.  That code was just
> copied from the DMA errata part of the driver, so it will need to be fixed
> as well.  Otherwise the same problem will happen when DMA is enabled.
>
>> And then we will have to add the support for all these OMAP custom hooks without pdata.
>
> That's really the key question for the medium- to long-term...

I know you know, I'm always hoping that someone will step up and propose 
some cool idea to fix that :-)

>> A basic fix (below) for the moment is to test for valid pdata inside the driver.
>> I'll repost it properly if you are fine with it.
>
> Looks fine to me, although I'd suggest fixing the DMA workaround path as
> well.

Mmm, I've tried to fix every instances of pdata in the code, you think I 
missed some?

I'll check again.

> Reviewed-by: Paul Walmsley<paul@pwsan.com>

Thanks,
Benoit

[PATCH 2/3] tty: serial: OMAP: block idle while the UART is transferring data in PIO mode

[Paul Walmsley]

Hi

On Fri, 24 Feb 2012, Cousson, Benoit wrote:

> On 2/21/2012 11:02 PM, Paul Walmsley wrote:
>
> > Looks fine to me, although I'd suggest fixing the DMA workaround path as
> > well.
> 
> Mmm, I've tried to fix every instances of pdata in the code, you think I
> missed some?
> 
> I'll check again.
> 
> > Reviewed-by: Paul Walmsley<paul@pwsan.com>

I just doublechecked the code paths and looks like you covered them all.  
Sorry about the confusion.

Reviewed-by: Paul Walmsley <paul@pwsan.com>

- Paul

[PATCH] tty: serial: OMAP: Fix oops due to NULL pdata in DT boot

[Cousson, Benoit]

The following commit: be4b0281956c5cae4f63f31f11d07625a6988766
(tty: serial: OMAP: block idle while the UART is transferring data in PIO mode),
is introducing an oops if OMAP is booted using device tree blob because
the pdata will not be initialized.

Check if pdata is set before de-referencing it.

Signed-off-by: Benoit Cousson <b-cousson@ti.com>
Reviewed-by: Paul Walmsley <paul@pwsan.com>
---
 drivers/tty/serial/omap-serial.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index f809041..0121486 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -159,7 +159,7 @@ static void serial_omap_stop_tx(struct uart_port *port)
 		serial_out(up, UART_IER, up->ier);
 	}
 
-	if (!up->use_dma && pdata->set_forceidle)
+	if (!up->use_dma && pdata && pdata->set_forceidle)
 		pdata->set_forceidle(up->pdev);
 
 	pm_runtime_mark_last_busy(&up->pdev->dev);
@@ -298,7 +298,7 @@ static void serial_omap_start_tx(struct uart_port *port)
 	if (!up->use_dma) {
 		pm_runtime_get_sync(&up->pdev->dev);
 		serial_omap_enable_ier_thri(up);
-		if (pdata->set_noidle)
+		if (pdata && pdata->set_noidle)
 			pdata->set_noidle(up->pdev);
 		pm_runtime_mark_last_busy(&up->pdev->dev);
 		pm_runtime_put_autosuspend(&up->pdev->dev);
@@ -1613,7 +1613,7 @@ static int serial_omap_runtime_resume(struct device *dev)
 	struct uart_omap_port *up = dev_get_drvdata(dev);
 	struct omap_uart_port_info *pdata = dev->platform_data;
 
-	if (up) {
+	if (up && pdata) {
 		if (pdata->get_context_loss_count) {
 			u32 loss_cnt = pdata->get_context_loss_count(dev);
 
-- 
1.7.0.4

[PATCH] tty: serial: OMAP: Fix oops due to NULL pdata in DT boot

[Greg KH]

On Tue, Feb 28, 2012 at 06:22:12PM +0100, Cousson, Benoit wrote:
> The following commit: be4b0281956c5cae4f63f31f11d07625a6988766
> (tty: serial: OMAP: block idle while the UART is transferring data in PIO mode),
> is introducing an oops if OMAP is booted using device tree blob because
> the pdata will not be initialized.

Is that something that happens today on systems?  Does this need to go
in for 3.3-final, or can it wait for 3.4?

thanks,

greg k-h

[PATCH] tty: serial: OMAP: Fix oops due to NULL pdata in DT boot

[Kevin Hilman]

Greg KH <gregkh@linuxfoundation.org> writes:

> On Tue, Feb 28, 2012 at 06:22:12PM +0100, Cousson, Benoit wrote:
>> The following commit: be4b0281956c5cae4f63f31f11d07625a6988766
>> (tty: serial: OMAP: block idle while the UART is transferring data in PIO mode),
>> is introducing an oops if OMAP is booted using device tree blob because
>> the pdata will not be initialized.
>
> Is that something that happens today on systems?  Does this need to go
> in for 3.3-final, or can it wait for 3.4?

It is needed for 3.3-final.

Thanks,

Kevin