[Qemu-devel] [PATCH v2 0/4] ARM pl011 fixes

[Qemu-devel] [PATCH v2 0/4] 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 3 other patches are problems
I noticed while debugging this issue. They are more for correctness of
the model than fixing any observed issues.

Rob

Rob Herring (4):
  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
  pl011: re-evaluate rx interrupt when fifo trigger changes

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

-- 
1.8.3.2

[Qemu-devel] [PATCH v2 1/4] 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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index a8ae6f4..f0c3fa3 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -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;
+        }
         s->lcr = value;
         pl011_set_read_trigger(s);
         break;
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v2 1/4] pl011: reset the fifo when enabled or disabled

[Peter Maydell]

On 14 March 2014 18:22, 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 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.

> 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 | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index a8ae6f4..f0c3fa3 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -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?

>          s->lcr = value;

>          pl011_set_read_trigger(s);
>          break;
> --
> 1.8.3.2
>

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/4] pl011: reset the fifo when enabled or disabled

[Peter Maydell]

On 16 March 2014 16:16, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 14 March 2014 18:22, 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 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.
>
>> 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 | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>> index a8ae6f4..f0c3fa3 100644
>> --- a/hw/char/pl011.c
>> +++ b/hw/char/pl011.c
>> @@ -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.

That said, I'm fairly sure that read_count = read_pos = 0
is indeed the correct behaviour when the FIFO is disabled.

thanks
-- PMM

[Qemu-devel] [PATCH v2 2/4] 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 | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index f0c3fa3..920ba3f 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.  */
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v2 2/4] pl011: fix UARTRSR accesses corrupting the UARTCR value

[Peter Maydell]

On 14 March 2014 18:22, Rob Herring <robherring2@gmail.com> wrote:
> 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 | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index f0c3fa3..920ba3f 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;

This needs a corresponding entry in the vmstate (and
the vmstate version_id/minimum_version_id/minimum_version_id_old
all need to be incremented).

>      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
>

thanks
-- PMM

[Qemu-devel] [PATCH v2 3/4] 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>
---
 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 920ba3f..5e664f4 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -220,7 +220,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 v2 3/4] pl011: fix incorrect logic to set the RXFF flag

[Peter Maydell]

On 14 March 2014 18:22, Rob Herring <robherring2@gmail.com> wrote:
> 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>

thanks
-- PMM

[Qemu-devel] [PATCH v2 4/4] pl011: re-evaluate rx interrupt when fifo trigger changes

[Rob Herring]

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

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

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 5e664f4..3903933 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -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;
+    }
 }
 
 static void pl011_write(void *opaque, hwaddr offset,
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v2 4/4] pl011: re-evaluate rx interrupt when fifo trigger changes

[Peter Maydell]

On 14 March 2014 18:22, Rob Herring <robherring2@gmail.com> wrote:
> From: Rob Herring <rob.herring@linaro.org>
>
> 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 <rob.herring@linaro.org>
> ---
>  hw/char/pl011.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 5e664f4..3903933 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -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?

Also if you're updating int_level then you need to call
pl011_update().

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 4/4] pl011: re-evaluate rx interrupt when fifo trigger changes

[Peter Maydell]

On 16 March 2014 15:57, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 14 March 2014 18:22, Rob Herring <robherring2@gmail.com> wrote:
>> From: Rob Herring <rob.herring@linaro.org>
>>
>> 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 <rob.herring@linaro.org>
>> ---
>>  hw/char/pl011.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>> index 5e664f4..3903933 100644
>> --- a/hw/char/pl011.c
>> +++ b/hw/char/pl011.c
>> @@ -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?

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.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 4/4] pl011: re-evaluate rx interrupt when fifo trigger changes

[Rob Herring]

On Sun, Mar 16, 2014 at 11:44 AM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 16 March 2014 15:57, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 14 March 2014 18:22, Rob Herring <robherring2@gmail.com> wrote:
>>> From: Rob Herring <rob.herring@linaro.org>
>>>
>>> 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 <rob.herring@linaro.org>
>>> ---
>>>  hw/char/pl011.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>>> index 5e664f4..3903933 100644
>>> --- a/hw/char/pl011.c
>>> +++ b/hw/char/pl011.c
>>> @@ -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?
>
> On closer inspection of the TRM it's not quite that simple (section 2.8.2).

Humm, a section not in r1p4 rev I was looking at, but in r1p5.

> 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.

I'm not even sure that is correct. I read it as the interrupt would
only assert when receiving a character that causes the fifo to be
equal to the new trigger level. I think changing the fifo trigger
level on the fly is just asking for trouble. It is all just
speculation and I don't have hardware to test actual behavior.

Rob