On 28 February 2014 00:57, Rob Herring <robherri...@gmail.com> wrote:
> From: Rob Herring <rob.herr...@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.herr...@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
