ARM pl011 fixes

From: Rob Herring <[email protected]>

Intermittent issues have been seen where no serial input occurs. It appears the pl011 gets in a state where the rx interrupt never fires because the rx interrupt only asserts when crossing the fifo trigger level. The fifo state appears to get out of sync when the pl011 is re-configured. This combined with the rx timeout interrupt not being modeled results in no more rx interrupts.

Disabling the fifo is the recommended way to clear the tx fifo in the TRM (section 3.3.8). The behavior in this case for the rx fifo is undefined in the TRM, but having fifo contents to be maintained during configuration changes is not likely expected behavior. Reseting the fifo state when the fifo size is changed is the simplest solution.

Signed-off-by Rob Herring <[email protected]>

---
 hw/char/pl011.c | 4 ++++
 1 file changed, 4 insertions(+)

hw/char/pl011.c +0 -0

index a8ae6f4..f0c3fa3 100644

hw/char/pl011.c +0 -0

         s->lcr = value;
 
         pl011_set_read_trigger(s);

         break;

--
1.8.3.2

Quoted 8 lines · Rob Herring

From: Rob Herring <[email protected]>

Intermittent issues have been seen where no serial input occurs. It appears the pl011 gets in a state where the rx interrupt never fires because the rx interrupt only asserts when crossing the fifo trigger level. The fifo state appears to get out of sync when the pl011 is re-configured. This combined with the rx timeout interrupt not being modeled results in no more rx interrupts.

This sounds like the bug is that we're not asserting the rx interrupt when we should, which is not what this patch is trying to change.

Among other things, this patch doesn't actually cause a write to this register to make us recalculate and re-send the interrupt state, because there is no call to pl011_update() in this code path.

Quoted 16 lines

Disabling the fifo is the recommended way to clear the tx fifo in the TRM (section 3.3.8). The behavior in this case for the rx fifo is undefined in the TRM, but having fifo contents to be maintained during configuration changes is not likely expected behavior. Reseting the fifo state when the fifo size is changed is the simplest solution.

Signed-off-by Rob Herring <[email protected]>

---
 hw/char/pl011.c | 4 ++++
 1 file changed, 4 insertions(+)

hw/char/pl011.c +0 -0

index a8ae6f4..f0c3fa3 100644

hw/char/pl011.c static void pl011_write(void *opaque, hwaddr offset, · line 168

Quoted 7 lines

hw/char/pl011.c +4 -0

@@ -162,6 +162,10 @@ static void pl011_write(void *opaque, hwaddr offset,
         s->fbrd = value;
         break;
     case 11: /* UARTLCR_H */
+        if (!((s->lcr ^ value) & 0x10)) {
+            s->read_count = 0;
+            s->read_pos = 0;
+        }

Am I misreading this, or do we clear the fifo on every write to this register where the fifo-enable bit is not changed?

I really think this patch is just covering over the actual problem and we need to identify and fix the actual bug.

From: Rob Herring <[email protected]>

Offset 4 is UARTRSR/UARTECR, not the UARTCR. The UARTCR would be corrupted if the UARTRSR is ever written. Fix by implementing a correct model of the UARTRSR/UARTECR register. Reads of this register simply reflect the error bits in data register. Only breaks can be triggered in QEMU. With the pl011_can_receive function, we effectively have flow control between the host and the model. Framing and parity errors simply don't make sense in the model and will never occur.

Signed-off-by Rob Herring <[email protected]>

---
 hw/char/pl011.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

hw/char/pl011.c +0 -0

index f0c3fa3..920ba3f 100644

hw/char/pl011.c +5 -5

     uint32_t cr;
     uint32_t dmacr;
     uint32_t int_enabled;

@@ -81,13 +82,14 @@ static uint64_t pl011_read(void *opaque, hwaddr offset,
         }
         if (s->read_count == s->read_trigger - 1)
             s->int_level &= ~ PL011_INT_RX;
+        s->rsr = c >> 8;
         pl011_update(s);
         if (s->chr) {
             qemu_chr_accept_input(s->chr);
         }
         return c;
-    case 1: /* UARTCR */
-        return 0;
+    case 1: /* UARTRSR */
+        return s->rsr;
     case 6: /* UARTFR */
         return s->flags;
     case 8: /* UARTILPR */

@@ -146,8 +148,8 @@ static void pl011_write(void *opaque, hwaddr offset,
         s->int_level |= PL011_INT_TX;
         pl011_update(s);
         break;
-    case 1: /* UARTCR */
-        s->cr = value;
+    case 1: /* UARTRSR/UARTECR */
+        s->rsr = 0;
         break;
     case 6: /* UARTFR */
         /* Writes to Flag register are ignored.  */
--

1.8.3.2

From: Rob Herring <[email protected]>

The receive fifo full bit should be set when 1 character is received and the fifo is disabled or when 16 characters are in the fifo.

Signed-off-by Rob Herring <[email protected]>

From: Rob Herring <[email protected]>

When setting the fifo trigger level, the rx interrupt needs to be asserted if the current fifo level matches. This is more for correctness as the level is currently never changed.

Signed-off-by Rob Herring <[email protected]>

---
 hw/char/pl011.c | 4 ++++
 1 file changed, 4 insertions(+)

hw/char/pl011.c +4 -0

index 5e664f4..3903933 100644
@@ -131,6 +131,10 @@ static void pl011_set_read_trigger(PL011State *s)
     else
 #endif
         s->read_trigger = 1;
+
+    if (s->read_count == s->read_trigger) {
+        s->int_level |= PL011_INT_RX;
+    }
 

=, surely?

Quoted 23 lines · Rob Herring

From: Rob Herring <[email protected]>

When setting the fifo trigger level, the rx interrupt needs to be asserted if the current fifo level matches. This is more for correctness as the level is currently never changed.

Signed-off-by Rob Herring <[email protected]>

---
 hw/char/pl011.c | 4 ++++
 1 file changed, 4 insertions(+)

hw/char/pl011.c +4 -0

index 5e664f4..3903933 100644
@@ -131,6 +131,10 @@ static void pl011_set_read_trigger(PL011State *s)
     else
 #endif
         s->read_trigger = 1;
+
+    if (s->read_count == s->read_trigger) {
+        s->int_level |= PL011_INT_RX;
+    }

Quoted 1 line

=, surely?

Quoted 25 lines · Peter Maydell

Quoted 23 lines · Rob Herring

From: Rob Herring <[email protected]>

When setting the fifo trigger level, the rx interrupt needs to be asserted if the current fifo level matches. This is more for correctness as the level is currently never changed.

Signed-off-by Rob Herring <[email protected]>

---
 hw/char/pl011.c | 4 ++++
 1 file changed, 4 insertions(+)

hw/char/pl011.c +4 -0

index 5e664f4..3903933 100644
@@ -131,6 +131,10 @@ static void pl011_set_read_trigger(PL011State *s)
     else
 #endif
         s->read_trigger = 1;
+
+    if (s->read_count == s->read_trigger) {
+        s->int_level |= PL011_INT_RX;
+    }

Quoted 1 line

Quoted 1 line

=, surely?

On closer inspection of the TRM it's not quite that simple (section 2.8.2).

We need to assert the interrupt only if the change in the read_trigger means the FIFO is now above the new threshold but wasn't above the old one, and only if the FIFOs are enabled -- if the FIFO is disabled the threshold doesn't figure into the interrupt level.