qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH] pl011: reset the fifo when enabled or disabled
Date: Sun, 2 Mar 2014 20:20:51 +0000

On 28 February 2014 00:57, Rob Herring <address@hidden> wrote:
> From: Rob Herring <address@hidden>
>
> 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 <address@hidden>
> ---
>  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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]