From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56812) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKb3a-0004QQ-M1 for qemu-devel@nongnu.org; Mon, 03 Mar 2014 17:09:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WKb3V-0000EJ-8L for qemu-devel@nongnu.org; Mon, 03 Mar 2014 17:09:54 -0500 Received: from mail-ea0-x234.google.com ([2a00:1450:4013:c01::234]:33097) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WKb3U-0000Do-Ua for qemu-devel@nongnu.org; Mon, 03 Mar 2014 17:09:49 -0500 Received: by mail-ea0-f180.google.com with SMTP id m10so4822947eaj.39 for ; Mon, 03 Mar 2014 14:09:48 -0800 (PST) Date: Mon, 3 Mar 2014 23:09:22 +0100 From: Beniamino Galvani Message-ID: <20140303220921.GA7506@gmail.com> References: <1393769202-4551-1-git-send-email-b.galvani@gmail.com> <1393769202-4551-3-git-send-email-b.galvani@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v2 2/7] allwinner-a10-pic: update pending register when an irq is cleared List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: Peter Maydell , "qemu-devel@nongnu.org Developers" , Li Guang On Mon, Mar 03, 2014 at 09:56:07PM +1000, Peter Crosthwaite wrote: > On Mon, Mar 3, 2014 at 12:06 AM, Beniamino Galvani wrote: > > The value of pending register was updated only when an irq was raised > > from a device; it should also be updated when an interrupt is cleared. > > So the reason for doing this is obviously the need for level sensitive > interrupts. Current implementation works under the assumption that all > interrupts are edge sensitive. This patch goes full on the other way > and mandates that all interrupts the edge-sensitive. In the absence of > definitive documentation, I guess that ok, but you do effecitively > defeature the interrupt controller transforming and edge to a level > (which is a very common interrupt controller feature). It is possible > to implement both schemes concurrently with some extra state. I.e. one > set of registers for the external pin state (no latching) and one set > for interrupts that have been detected and not serviced yet (via wtc > to IRQ_PENDING). If an interrupt is serviced while the pin state is > still high then it insta-retriggers (correct level sens. behav.). > > You have left in the support for clearing the register from software. > Although I can see some wierd use cases of this. I.e. an interrupt can > be triggered and "serviced" (i.e. irq_pending wtc'd) by the intc then > the device level service is delayed while the device irq line remains > high. To, me this actually corresponds to a disabling of the interrupt > (assuming it is level sensitive!) rather than a servicing of a pending > interrupt. This match the kernel driver? Hi, the kernel driver uses this function to ack an interrupt [1]: static void sun4i_irq_ack(struct irq_data *irqd) { unsigned int irq = irqd_to_hwirq(irqd); unsigned int irq_off = irq % 32; int reg = irq / 32; u32 val; val = readl(sun4i_irq_base + SUN4I_IRQ_PENDING_REG(reg)); writel(val | (1 << irq_off), sun4i_irq_base + SUN4I_IRQ_PENDING_REG(reg)); } I don't understand how this function can work, because if we assume that the SUN4I_IRQ_PENDING register is a wtc register as specified in the datasheet [2] at p.114 (and ignoring that the register is also described as read-only), the function would clear all the bits every time is called. As reported in the comment for v1, there was a thread on LKML [3] in which a drop of all the writes to the pending register was proposed, and a mail from the driver author explained that writes to the register are basically uneffective. So my initial idea was to remove the writability of pending register and to change all interrupts to level-triggered; but since I was not sure about the first point, I kept the writability. Beniamino [1] http://lxr.free-electrons.com/source/drivers/irqchip/irq-sun4i.c#L41 [2] http://dl.linux-sunxi.org/A10/A10%20User%20Manual%20-%20v1.20%20%282012-04-09,%20DECRYPTED%29.pdf [3] https://lkml.org/lkml/2013/7/6/59 > Regards, > Peter > > > Signed-off-by: Beniamino Galvani > > --- > > hw/intc/allwinner-a10-pic.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/hw/intc/allwinner-a10-pic.c b/hw/intc/allwinner-a10-pic.c > > index 00f3c11..9011c82 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); > > } > > -- > > 1.7.10.4 > > > >