[Qemu-devel] [PATCH v3 0/3] ARM pl011 fixes

[Qemu-devel] [PATCH v3 0/3] ARM pl011 fixes

[Rob Herring]

From: Rob Herring <rob.herring@linaro.org>

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

[Qemu-devel] [PATCH v3 1/3] pl011: reset the fifo when enabled or disabled

[Rob Herring]

From: Rob Herring <rob.herring@linaro.org>

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 <rob.herring@linaro.org>
---
 hw/char/pl011.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index a8ae6f4..8103e2e 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -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

[Qemu-devel] [PATCH v3 2/3] pl011: fix UARTRSR accesses corrupting the UARTCR value

[Rob Herring]

From: Rob Herring <rob.herring@linaro.org>

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 <rob.herring@linaro.org>
---
 hw/char/pl011.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 8103e2e..11c3a75 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -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

[Qemu-devel] [PATCH v3 3/3] pl011: fix incorrect logic to set the RXFF flag

[Rob Herring]

From: Rob Herring <rob.herring@linaro.org>

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 <rob.herring@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/char/pl011.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 11c3a75..644aad7 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -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

Re: [Qemu-devel] [PATCH v3 0/3] ARM pl011 fixes

[Peter Maydell]

On 18 March 2014 18:18, Rob Herring <robherring2@gmail.com> wrote:
> From: Rob Herring <rob.herring@linaro.org>
>
> 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

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM