[PATCH] serial: pl011: implement workaround for CTS clear event issue

[PATCH] serial: pl011: implement workaround for CTS clear event issue

[Linus Walleij]

From: "Rajanikanth H.V" <rajanikanth.hv@stericsson.com>

Problem Observed:
- interrupt status is set by rising or falling edge on CTS line
- interrupt status is cleared on a .0. to .1. transition of the
  interrupt-clear register bit 1.
- interrupt-clear register is reset by hardware once the interrupt
  status is .0..
  Remark: It seems not possible to read this register back by the
  CPU though, but internally this register exists.
- when simultaneous set and reset event on the interrupt status
  happens, then the set-event has priority and the status remains
  .1.. As a result the interrupt-clear register is not reset to
  .0., and no new .0. to .1. transition can be detected on it when
  writing a .1. to it.
  This implies race condition, the clear must be performed at least
  one UARTCLK the riding edge of CTS RIS interrupt.

Fix:
  Instead of resetting UART as done in commit
  c16d51a32bbb61ac8fd96f78b5ce2fccfe0fb4c3
  "amba pl011: workaround for uart registers lockup" do the
  following:

  write .0. and then  .1. to the interrupt-clear register to make
  sure that this transition is detected. According to the datasheet
  writing a .0. does not have any effect, but actually it allows to
  reset the internal interrupt-clear register.

  Take into account:
  The .0. needs to last at least for one clk_uart clock period
  (~ 38 MHz, 26.08ns)

This way we can do away with the tasklet and keep only a tiny
fix triggered by the variant flag introduced in this patch.

Signed-off-by: Guillaume Jaunet <guillaume.jaunet@stericsson.com>
Signed-off-by: Christophe Arnal <christophe.arnal@stericsson.com>
Signed-off-by: Matthias Locher  <Matthias.Locher@stericsson.com>
Signed-off-by: Rajanikanth H.V <rajanikanth.hv@stericsson.com>
Reviewed-by: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/tty/serial/amba-pl011.c |  114 +++++++--------------------------------
 1 file changed, 18 insertions(+), 96 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 4ed35c5..e48211d 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -61,35 +61,9 @@
 #define SERIAL_AMBA_MINOR	64
 #define SERIAL_AMBA_NR		UART_NR
 
-#define AMBA_ISR_PASS_LIMIT	256
-
 #define UART_DR_ERROR		(UART011_DR_OE|UART011_DR_BE|UART011_DR_PE|UART011_DR_FE)
 #define UART_DUMMY_DR_RX	(1 << 16)
 
-
-#define UART_WA_SAVE_NR 14
-
-static void pl011_lockup_wa(unsigned long data);
-static const u32 uart_wa_reg[UART_WA_SAVE_NR] = {
-	ST_UART011_DMAWM,
-	ST_UART011_TIMEOUT,
-	ST_UART011_LCRH_RX,
-	UART011_IBRD,
-	UART011_FBRD,
-	ST_UART011_LCRH_TX,
-	UART011_IFLS,
-	ST_UART011_XFCR,
-	ST_UART011_XON1,
-	ST_UART011_XON2,
-	ST_UART011_XOFF1,
-	ST_UART011_XOFF2,
-	UART011_CR,
-	UART011_IMSC
-};
-
-static u32 uart_wa_regdata[UART_WA_SAVE_NR];
-static DECLARE_TASKLET(pl011_lockup_tlet, pl011_lockup_wa, 0);
-
 /* There is by now@least one vendor with differing details, so handle it */
 struct vendor_data {
 	unsigned int		ifls;
@@ -99,6 +73,7 @@ struct vendor_data {
 	bool			oversampling;
 	bool			interrupt_may_hang;   /* vendor-specific */
 	bool			dma_threshold;
+	bool			cts_event_workaround;
 };
 
 static struct vendor_data vendor_arm = {
@@ -108,6 +83,7 @@ static struct vendor_data vendor_arm = {
 	.lcrh_rx		= UART011_LCRH,
 	.oversampling		= false,
 	.dma_threshold		= false,
+	.cts_event_workaround	= false,
 };
 
 static struct vendor_data vendor_st = {
@@ -118,6 +94,7 @@ static struct vendor_data vendor_st = {
 	.oversampling		= true,
 	.interrupt_may_hang	= true,
 	.dma_threshold		= true,
+	.cts_event_workaround	= true,
 };
 
 static struct uart_amba_port *amba_ports[UART_NR];
@@ -1038,69 +1015,6 @@ static inline bool pl011_dma_rx_running(struct uart_amba_port *uap)
 #define pl011_dma_flush_buffer	NULL
 #endif
 
-
-/*
- * pl011_lockup_wa
- * This workaround aims to break the deadlock situation
- * when after long transfer over uart in hardware flow
- * control, uart interrupt registers cannot be cleared.
- * Hence uart transfer gets blocked.
- *
- * It is seen that during such deadlock condition ICR
- * don't get cleared even on multiple write. This leads
- * pass_counter to decrease and finally reach zero. This
- * can be taken as trigger point to run this UART_BT_WA.
- *
- */
-static void pl011_lockup_wa(unsigned long data)
-{
-	struct uart_amba_port *uap = amba_ports[0];
-	void __iomem *base = uap->port.membase;
-	struct circ_buf *xmit = &uap->port.state->xmit;
-	struct tty_struct *tty = uap->port.state->port.tty;
-	int buf_empty_retries = 200;
-	int loop;
-
-	/* Stop HCI layer from submitting data for tx */
-	tty->hw_stopped = 1;
-	while (!uart_circ_empty(xmit)) {
-		if (buf_empty_retries-- == 0)
-			break;
-		udelay(100);
-	}
-
-	/* Backup registers */
-	for (loop = 0; loop < UART_WA_SAVE_NR; loop++)
-		uart_wa_regdata[loop] = readl(base + uart_wa_reg[loop]);
-
-	/* Disable UART so that FIFO data is flushed out */
-	writew(0x00, uap->port.membase + UART011_CR);
-
-	/* Soft reset UART module */
-	if (uap->port.dev->platform_data) {
-		struct amba_pl011_data *plat;
-
-		plat = uap->port.dev->platform_data;
-		if (plat->reset)
-			plat->reset();
-	}
-
-	/* Restore registers */
-	for (loop = 0; loop < UART_WA_SAVE_NR; loop++)
-		writew(uart_wa_regdata[loop] ,
-				uap->port.membase + uart_wa_reg[loop]);
-
-	/* Initialise the old status of the modem signals */
-	uap->old_status = readw(uap->port.membase + UART01x_FR) &
-		UART01x_FR_MODEM_ANY;
-
-	if (readl(base + UART011_MIS) & 0x2)
-		printk(KERN_EMERG "UART_BT_WA: ***FAILED***\n");
-
-	/* Start Tx/Rx */
-	tty->hw_stopped = 0;
-}
-
 static void pl011_stop_tx(struct uart_port *port)
 {
 	struct uart_amba_port *uap = (struct uart_amba_port *)port;
@@ -1227,14 +1141,28 @@ static irqreturn_t pl011_int(int irq, void *dev_id)
 {
 	struct uart_amba_port *uap = dev_id;
 	unsigned long flags;
-	unsigned int status, pass_counter = AMBA_ISR_PASS_LIMIT;
+	unsigned int status;
 	int handled = 0;
+	unsigned int dummy_read;
 
 	spin_lock_irqsave(&uap->port.lock, flags);
 
 	status = readw(uap->port.membase + UART011_MIS);
 	if (status) {
 		do {
+			if (uap->vendor->cts_event_workaround) {
+				/* workaround to make sure that all bits are unlocked.. */
+				writew(0x00, uap->port.membase + UART011_ICR);
+
+				/*
+				 * WA: introduce 26ns(1 uart clk) delay before W1C;
+				 * single apb access will incur 2 pclk(133.12Mhz) delay,
+				 * so add 2 dummy reads
+				 */
+				dummy_read = readw(uap->port.membase + UART011_ICR);
+				dummy_read = readw(uap->port.membase + UART011_ICR);
+			}
+
 			writew(status & ~(UART011_TXIS|UART011_RTIS|
 					  UART011_RXIS),
 			       uap->port.membase + UART011_ICR);
@@ -1251,12 +1179,6 @@ static irqreturn_t pl011_int(int irq, void *dev_id)
 			if (status & UART011_TXIS)
 				pl011_tx_chars(uap);
 
-			if (pass_counter-- == 0) {
-				if (uap->interrupt_may_hang)
-					tasklet_schedule(&pl011_lockup_tlet);
-				break;
-			}
-
 			status = readw(uap->port.membase + UART011_MIS);
 		} while (status != 0);
 		handled = 1;
-- 
1.7.9.2

[PATCH] serial: pl011: implement workaround for CTS clear event issue

[Russell King - ARM Linux]

On Mon, Mar 26, 2012 at 10:01:08AM +0200, Linus Walleij wrote:
> -	unsigned int status, pass_counter = AMBA_ISR_PASS_LIMIT;
> +	unsigned int status;

Why are you removing the pass counter and associated code, which was
there before your work-around patch was applied?

[PATCH] serial: pl011: implement workaround for CTS clear event issue

[Linus Walleij]

On Mon, Mar 26, 2012 at 10:17 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Mar 26, 2012 at 10:01:08AM +0200, Linus Walleij wrote:
>> - ? ? unsigned int status, pass_counter = AMBA_ISR_PASS_LIMIT;
>> + ? ? unsigned int status;
>
> Why are you removing the pass counter and associated code, which was
> there before your work-around patch was applied?

Good point, I'll cook a v2 which restores that counter an break clause to
what it was before the workaround was added.

Yours,
Linus Walleij

[PATCH] serial: pl011: implement workaround for CTS clear event issue

[Rajanikanth H V]

Russell,

As per my understanding, pass counter helps to identify/break the 
Deadlock situation and triggers bottom half handler in which
Uart controller will be forced to reset (save and restore of UART register happens).

It has been found from hardware analysis that race condition(explained in my commit message)
was causing the situation to persist (i.e. CTS ack was not happening in spite of W1C),
hence, assuming UART controller in deadlock state is wrong.


Thanks,
Rajanikanth

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk] 
Sent: Monday, March 26, 2012 1:48 PM
To: Linus WALLEIJ
Cc: Greg Kroah-Hartman; linux-serial at vger.kernel.org; linux-arm-kernel at lists.infradead.org; Christophe ARNAL; Guillaume JAUNET; Matthias LOCHER; Chanho Min; Linus Walleij; Rajanikanth H V
Subject: Re: [PATCH] serial: pl011: implement workaround for CTS clear event issue

On Mon, Mar 26, 2012 at 10:01:08AM +0200, Linus Walleij wrote:
> -	unsigned int status, pass_counter = AMBA_ISR_PASS_LIMIT;
> +	unsigned int status;

Why are you removing the pass counter and associated code, which was
there before your work-around patch was applied?

[PATCH] serial: pl011: implement workaround for CTS clear event issue

[Linus Walleij]

On Mon, Mar 26, 2012 at 11:07 AM, Rajanikanth H V
<rajanikanth.hv@stericsson.com> wrote:

> As per my understanding, pass counter helps to identify/break the
> Deadlock situation and triggers bottom half handler in which
> Uart controller will be forced to reset (save and restore of UART register happens).

It was there before the fix for the deadlock situation found in ST hardwares,
which was solved by the tasklet, was ever introduced.

It has been around forever as a simple guard to stop the ISR from
going into a loop... which is nice coding practice anyway so let's
keep it.

Yours,
Linus Walleij