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

[Andre Przywara <andre.przywara@linaro.org>]

On 08/21/2013 12:04 PM, Ian Campbell wrote:
> On Tue, 2013-08-13 at 17:12 +0200, Andre Przywara wrote:
>> The PL011 IMSC register description is somehow fuzzy in the
>> documentation;
>
> OMG pl011 docs r1p5 issue g says:
>          On a read this register returns the current value of the mask on
>          the relevant interrupt. On a write of 1 to the particular bit,
>          it sets the corresponding mask of that interrupt. A write of 0
>          clears the corresponding mask.
>
> Which is perfectly obvious if you read it understanding "mask an
> interrupt" in the normal way, but in the introduction to the section on
> interrupts it says:
>
>          You can enable or disable the individual interrupts by changing
>          the mask bits in the Interrupt Mask Set/Clear Register, UARTIMSC
>          on page 3-17. Setting the appropriate mask bit HIGH enables the
>          interrupt.
>
> So IMSC is actually the "mask" of the interrupts which are enabled.
>
> What crappy docs!

I agree, I stumbled on this by reading IMSC and wondered how it could be 
just 0 in the first place, then looking at the Linux code...
And honestly I didn't even find the first paragraph, but was wondering 
why they repeat that "1 sets the mask, 0 clears it" all over the place 
instead of explicitly stating the meaning for the interrupt ;-)

Regards,
Andre.

>>   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).
>
> How on earth we got away with this I've idea!
>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>
> Ack + applied, thanks!
>
>