From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andre Przywara Subject: Re: [PATCH] PL011: fix reverse logic for interrupt mask register Date: Wed, 21 Aug 2013 15:58:05 +0200 Message-ID: <5214C76D.1000409@linaro.org> References: <1376406755-23703-1-git-send-email-andre.przywara@linaro.org> <1377079470.31937.43.camel@hastur.hellion.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1377079470.31937.43.camel@hastur.hellion.org.uk> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: julien.grall@linaro.org, xen-devel@lists.xen.org, patches@linaro.org, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.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 > > Ack + applied, thanks! > >