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.

This problem is fixed by the 1st patch. The 2 other patches are problems I noticed while debugging this issue. They are more for correctness of the model than fixing any observed issues.

Changes in v3:
- Fix logic for checking if FIFO enable bit has changed
- Add vmstate for UARTRSR register
- Dropped patch 4

Rob

Rob Herring (3):
pl011: reset the fifo when enabled or disabled
pl011: fix UARTRSR accesses corrupting the UARTCR value
pl011: fix incorrect logic to set the RXFF flag

 hw/char/pl011.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

1.8.3.2

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.

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

index a8ae6f4..8103e2e 100644
@@ -162,6 +162,11 @@ static void pl011_write(void *opaque, hwaddr offset,
         s->fbrd = value;
         break;
     case 11: /* UARTLCR_H */
+        /* Reset the FIFO state on FIFO enable or disable */
+        if ((s->lcr ^ value) & 0x10) {
+            s->read_count = 0;
+            s->read_pos = 0;
+        }
         s->lcr = value;
         pl011_set_read_trigger(s);
         break;

1.8.3.2

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.

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

index 8103e2e..11c3a75 100644
@@ -20,6 +20,7 @@ typedef struct PL011State {
     uint32_t readbuff;
     uint32_t flags;
     uint32_t lcr;
+    uint32_t rsr;
     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.  */

@@ -247,13 +249,14 @@ static const MemoryRegionOps pl011_ops = {
 
 static const VMStateDescription vmstate_pl011 = {
     .name = "pl011",
-    .version_id = 1,
-    .minimum_version_id = 1,
-    .minimum_version_id_old = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
     .fields      = (VMStateField[]) {
         VMSTATE_UINT32(readbuff, PL011State),
         VMSTATE_UINT32(flags, PL011State),
         VMSTATE_UINT32(lcr, PL011State),
+        VMSTATE_UINT32(rsr, PL011State),
         VMSTATE_UINT32(cr, PL011State),
         VMSTATE_UINT32(dmacr, PL011State),
         VMSTATE_UINT32(int_enabled, PL011State),

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.

---
 hw/char/pl011.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 11c3a75..644aad7 100644
@@ -221,7 +221,7 @@ static void pl011_put_fifo(void *opaque, uint32_t value)
     s->read_fifo[slot] = value;
     s->read_count++;
     s->flags &= ~PL011_FLAG_RXFE;
-    if (s->cr & 0x10 || s->read_count == 16) {
+    if (!(s->lcr & 0x10) || s->read_count == 16) {
         s->flags |= PL011_FLAG_RXFF;
     }
     if (s->read_count == s->read_trigger) {

1.8.3.2

thanks
-- PMM