Re: [PATCH] PL011: fix reverse logic for interrupt mask register

[Julien Grall <julien.grall@linaro.org>]

On 08/27/2013 11:45 AM, Ian Campbell wrote:
> On Tue, 2013-08-27 at 11:37 +0100, Ian Campbell wrote:
>> On Mon, 2013-08-26 at 16:55 +0100, Julien Grall wrote:
>>> On 08/22/2013 08:23 AM, Ian Campbell wrote:
>>>> On Wed, 2013-08-21 at 17:11 +0100, Julien Grall wrote:
>>>>> On 08/13/2013 04:12 PM, Andre Przywara wrote:
>>>>>> The PL011 IMSC register description is somehow fuzzy in the
>>>>>> documentation; by comparing it with the Linux implementation one can
>>>>>> see that the logic is actually reversed to Xen's implementation:
>>>>>> A "0" in field means interrupt disabled, a "1" enables it.
>>>>>> Therefore we enabled all interrupts instead of disabling them in the
>>>>>> beginning and later on masked the wrong interrupts.
>>>>>> Unclear how this worked on the Versatile Express, but this fix is
>>>>>> needed to get Calxeda Midway running (and works on VExpress, too).
>>>>>
>>>>> On my Versatile Express, the keyboard is unusable with this patch.
>>>>> Xen receives random keys and sometimes nothing is printed on the serial
>>>>> port.
>>>>>
>>>>> By reverting this patch on my tree, I'm able to use correctly the serial
>>>>> port.
>>>>
>>>> :-/ Andre did say this patch worked on vexpress for him.
>>>>
>>>> I'm pretty certain Andre's patch is correct in its own right. But the
>>>> fact that it worked before does seem to imply that there are other
>>>> issues with the pl011 driver, it's likely that this change has just
>>>> exposed a latent one.
>>>>
>>>> Could this be related somehow to the baud rate setting change? I
>>>> wouldn't have expected so but "random keys" and nothingness could be a
>>>> symptom of incorrect baud too.
>>>>
>>>> Does anyone have time to look into this?
>>>>
>>>
>>> If RTI interrupt are enabled (see small patch below), the UART works perfectly
>>> on the versatile express.
>>>
>>> The PL011 documentation says the bit is used to mask/unmask receive interrupt
>>> timeout. I don't understand why this interrupt is useful and the documentation
>>> doesn't help me...
>>
>> Docs say:
>>         The receive timeout interrupt is asserted when the receive FIFO
>>         is not empty, and no more data is received during a 32-bit
>>         period.
> 
> I see now that we do actually enable the FIFOs, so this functionality
> makes sense -- if your RX FIFO trigger level is e.g. 1/2 and the FIFO is
> 1/4 full when the sender stops then this interrupt provides a backstop
> such that you will receive those bytes without waiting for the sender to
> resume (which might be a long time).
> 
> So I think enabling and handling these timeout interrupts makes sense. I
> cannot explain why it only affects vexpress though.

I only see the issue with the serial input. I guess this is needed on
midway but we don't see anything because the serial console doesn't
receive input.

I will send a correct patch + commit message this afternoon.

-- 
Julien Grall