From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38448) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WI6W4-0008KD-Av for qemu-devel@nongnu.org; Mon, 24 Feb 2014 20:09:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WI6Vz-0005Xp-ET for qemu-devel@nongnu.org; Mon, 24 Feb 2014 20:09:00 -0500 Received: from [222.73.24.84] (port=47905 helo=song.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WI6Vy-0005Vz-KM for qemu-devel@nongnu.org; Mon, 24 Feb 2014 20:08:55 -0500 Message-ID: <530BED08.1090901@cn.fujitsu.com> Date: Tue, 25 Feb 2014 09:08:24 +0800 From: Li Guang MIME-Version: 1.0 References: <1392659003-8264-1-git-send-email-b.galvani@gmail.com> <1392659003-8264-3-git-send-email-b.galvani@gmail.com> <5302D85F.6040209@cn.fujitsu.com> <20140218232228.GB24042@gmail.com> <530410BC.9030601@cn.fujitsu.com> <20140222142007.GA13168@gmail.com> <530AEA72.40105@cn.fujitsu.com> <20140224225000.GA12154@gmail.com> In-Reply-To: <20140224225000.GA12154@gmail.com> Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=ISO-8859-1; format=flowed Subject: Re: [Qemu-devel] [PATCH 2/7] allwinner-a10-pic: fix interrupt clear behaviour List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Beniamino Galvani Cc: Peter Maydell , Peter Crosthwaite , qemu-devel@nongnu.org Beniamino Galvani wrote: > On Mon, Feb 24, 2014 at 02:45:06PM +0800, Li Guang wrote: > >> Beniamino Galvani wrote: >> >>> On Wed, Feb 19, 2014 at 10:02:36AM +0800, Li Guang wrote: >>> >>>> Beniamino Galvani wrote: >>>> >>>>> On Tue, Feb 18, 2014 at 11:49:51AM +0800, Li Guang wrote: >>>>> >>>>>> Beniamino Galvani wrote: >>>>>> >>>>>>> According to this mail thread [1], writing to pending register seems >>>>>>> to have no effect on actual pending status of interrupts. This means >>>>>>> that the only way to clear a pending interrupt is to clear the >>>>>>> interrupt source. This patch implements such behaviour. >>>>>>> >>>>>>> [1] http://lkml.org/lkml/2013/7/6/59 >>>>>>> >>>>>>> Signed-off-by: Beniamino Galvani >>>>>>> --- >>>>>>> hw/intc/allwinner-a10-pic.c | 6 ++++-- >>>>>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c >>>>>>> index bb2351f..afd57ef 100644 >>>>>>> --- a/hw/intc/allwinner-a10-pic.c >>>>>>> +++ b/hw/intc/allwinner-a10-pic.c >>>>>>> @@ -49,6 +49,8 @@ static void aw_a10_pic_set_irq(void *opaque, int irq, int level) >>>>>>> >>>>>>> if (level) { >>>>>>> set_bit(irq % 32, (void *)&s->irq_pending[irq / 32]); >>>>>>> + } else { >>>>>>> + clear_bit(irq % 32, (void *)&s->irq_pending[irq / 32]); >>>>>>> } >>>>>>> aw_a10_pic_update(s); >>>>>>> } >>>>>>> @@ -105,10 +107,10 @@ static void aw_a10_pic_write(void *opaque, hwaddr offset, uint64_t value, >>>>>>> s->nmi = value; >>>>>>> break; >>>>>>> case AW_A10_PIC_IRQ_PENDING ... AW_A10_PIC_IRQ_PENDING + 8: >>>>>>> - s->irq_pending[index]&= ~value; >>>>>>> + /* Nothing to do */ >>>>>>> break; >>>>>>> case AW_A10_PIC_FIQ_PENDING ... AW_A10_PIC_FIQ_PENDING + 8: >>>>>>> - s->fiq_pending[index]&= ~value; >>>>>>> + /* Ditto */ >>>>>>> break; >>>>>>> case AW_A10_PIC_SELECT ... AW_A10_PIC_SELECT + 8: >>>>>>> s->select[index] = value; >>>>>>> >>>>>> pending registers are also clear registers by a10 datasheet, >>>>>> also you found bits are marked as 'R', so, ..., contradict itself. >>>>>> >>>>> Yes, the datasheet is inconsistent about this because the register >>>>> can't be read-only and 'clear' at the same time. >>>>> >>>>> Unfortunately at the moment I cannot test if the clearing >>>>> functionality of the pending register works on real hardware but the >>>>> idea I got from the linked discussion is that it's either not >>>>> implemented or broken and therefore interrupts remain pending until >>>>> they are disabled at the source. >>>>> >>>>> Do you have a chance to try it on a real board? >>>>> >>>>> >>>> Ah? even kernel code from allwinner wrote pending registers >>>> to clear pending interrupt, didn't you see it? >>>> so should be no doubt that these registers are writable. >>>> >>> Well, if you look closely at that code, it's a bit strange: >>> >>> void sw_irq_ack(struct irq_data *irqd) >>> { >>> unsigned int irq = irqd->irq; >>> >>> [...] >>> writel(readl(SW_INT_IRQ_PENDING_REG0) | (1<>> [...] >>> } >>> >>> In order to clear a single interrupt, it is or'ing the irq bit with >>> the previous value of the register, so it's basically clearing all >>> pending interrupts. >>> >>> This is discussed in the mentioned thread from lkml and the final >>> explanation is that the pending register is always updated with the >>> actual status of irq lines, even if you write to it. In other words, >>> writes to the register are ignored, otherwise the code above would >>> produce random loss of interrupts. >>> >>> >> Hmm..., sorry, I also can't test this operation on A10 board now, >> but why not they just wipe out these writings(kernel 3.12)? >> > I don't know, there was a proposed patch that removed those writes in > the lkml discussion but probably it never reached mainline. > > To be on the safe side I can restore the writability of the register > and then, when we will figure out how it really works, correct it if > needed. > > > Agreed, Thanks!