[Qemu-devel] [PATCH] pl011: reset the fifo when enabled or disabled

[Qemu-devel] [PATCH] 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 fifo in the TRM,
but none of the fifo state was getting reset when the fifo was disabled.
Ensure all the fifo state is reset when the fifo is enabled or disabled.

When setting the fifo trigger level, the rx interrupt needs to be asserted
if the current fifo level matches.

Also, fix incorrect logic to set the RXFF flag.

Signed-off-by: Rob Herring <rob.herring@linaro.org>
---
 hw/char/pl011.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/char/pl011.c b/hw/char/pl011.c
index 8ced7cd..9260a3d 100644
--- a/hw/char/pl011.c
+++ b/hw/char/pl011.c
@@ -129,6 +129,9 @@ 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,
@@ -162,6 +165,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;
@@ -214,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] pl011: reset the fifo when enabled or disabled

[Peter Maydell]

On 28 February 2014 00:57, 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.
>
> Disabling the fifo is the recommended way to clear the fifo in the TRM,
> but none of the fifo state was getting reset when the fifo was disabled.
> Ensure all the fifo state is reset when the fifo is enabled or disabled.
>
> When setting the fifo trigger level, the rx interrupt needs to be asserted
> if the current fifo level matches.
>
> Also, fix incorrect logic to set the RXFF flag.

This seems to be trying to fix two or three separate bugs
in a single patch -- please split it up.

> Signed-off-by: Rob Herring <rob.herring@linaro.org>
> ---
>  hw/char/pl011.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> index 8ced7cd..9260a3d 100644
> --- a/hw/char/pl011.c
> +++ b/hw/char/pl011.c
> @@ -129,6 +129,9 @@ 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;

Missing braces -- please use checkpatch.pl.

Also I don't believe this change can be fixing your
problems for two reasons:
 1) given the #if'd out code here, read_trigger is always 1,
     so calling this function can never result in the read_count
     being at or over the read_trigger threshold when it was not
     already so
 2) the calling points for the function don't subsequently
     call pl011_update() so this change alone won't cause us
     to assert our outbound IRQ

I don't suppose you have a nice reliable reproduce case?

>  }
>
>  static void pl011_write(void *opaque, hwaddr offset,
> @@ -162,6 +165,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;
> +        }

Can you point me at the bit of the TRM that says this is the behaviour?
All I found was a bit that said that "fifo integrity wasn't guaranteed" if
you disable the fifo when it has data in it and then reenable it. Probably
I'm just looking in the wrong part of the TRM.

This could use a comment:
     /* FIFO being enabled/disabled: reset it to empty */

>          s->lcr = value;
>          pl011_set_read_trigger(s);
>          break;
> @@ -214,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;
>      }

Agreed -- RXFF is set if the fifo is full or the
fifo is disabled (in which case 1 byte is "full").

>      if (s->read_count == s->read_trigger) {
> --
> 1.8.3.2
>

thanks
-- PMM

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

[Rob Herring]

On Sun, Mar 2, 2014 at 2:20 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 28 February 2014 00:57, 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.
>>
>> Disabling the fifo is the recommended way to clear the fifo in the TRM,
>> but none of the fifo state was getting reset when the fifo was disabled.
>> Ensure all the fifo state is reset when the fifo is enabled or disabled.
>>
>> When setting the fifo trigger level, the rx interrupt needs to be asserted
>> if the current fifo level matches.
>>
>> Also, fix incorrect logic to set the RXFF flag.
>
> This seems to be trying to fix two or three separate bugs
> in a single patch -- please split it up.
>
>> Signed-off-by: Rob Herring <rob.herring@linaro.org>
>> ---
>>  hw/char/pl011.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
>> index 8ced7cd..9260a3d 100644
>> --- a/hw/char/pl011.c
>> +++ b/hw/char/pl011.c
>> @@ -129,6 +129,9 @@ 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;
>
> Missing braces -- please use checkpatch.pl.
>
> Also I don't believe this change can be fixing your
> problems for two reasons:
>  1) given the #if'd out code here, read_trigger is always 1,
>      so calling this function can never result in the read_count
>      being at or over the read_trigger threshold when it was not
>      already so

Yes, agreed. I added it more for correctness in that if we set
read_trigger we should be doing this test. This could be wrong in that
we may have to receive more data to re-trigger the interrupt, but
since we don't model the timeout, we can't be completely accurate in
this regard.

>  2) the calling points for the function don't subsequently
>      call pl011_update() so this change alone won't cause us
>      to assert our outbound IRQ
>
> I don't suppose you have a nice reliable reproduce case?

Given the intermittent nature of this, no. Nor do I have real h/w to
really confirm actual behavior. While I can observe the state the
model gets into, the exact sequence that gets it into that state is
not perfectly clear.

>>  }
>>
>>  static void pl011_write(void *opaque, hwaddr offset,
>> @@ -162,6 +165,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;
>> +        }
>
> Can you point me at the bit of the TRM that says this is the behaviour?
> All I found was a bit that said that "fifo integrity wasn't guaranteed" if
> you disable the fifo when it has data in it and then reenable it. Probably
> I'm just looking in the wrong part of the TRM.

It was the UARTCR register description, but re-reading that it is only
talking about the transmit fifo. It's unclear about the rx fifo. Given
that it is undefined, clearing it is just as correct as doing nothing.
No sane person would expect to maintain data while changing fifo
sizes.

Rob