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