From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller Date: Tue, 21 Oct 2014 12:00:18 +0100 Message-ID: <1413889218.23337.24.camel@citrix.com> References: <1412328051-20015-1-git-send-email-talex5@gmail.com> <1412328051-20015-3-git-send-email-talex5@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XgXAt-0000Vg-HB for xen-devel@lists.xenproject.org; Tue, 21 Oct 2014 11:00:23 +0000 In-Reply-To: <1412328051-20015-3-git-send-email-talex5@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Thomas Leonard Cc: Dave.Scott@eu.citrix.com, anil@recoil.org, stefano.stabellini@eu.citrix.com, samuel.thibault@ens-lyon.org, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On Fri, 2014-10-03 at 10:20 +0100, Thomas Leonard wrote: > +static inline uint32_t REG_READ32(volatile uint32_t *addr) > +{ > + uint32_t value; > + __asm__ __volatile__("ldr %0, [%1]":"=&r"(value):"r"(addr)); > + rmb(); I'm not 100% convinced that you need this rmb(). > + return value; > +} > + > +static inline void REG_WRITE32(volatile uint32_t *addr, unsigned int value) > +{ > + __asm__ __volatile__("str %0, [%1]"::"r"(value), "r"(addr)); > + wmb(); > +} > + > +static void gic_set_priority(struct gic *gic, int irq_number, unsigned char priority) > +{ > + uint32_t value; > + uint32_t *addr = REG(gicd(gic, GICD_IPRIORITYR)) + (irq_number >> 2); > + value = REG_READ32(addr); > + value &= ~(0xff << (8 * (irq_number & 0x3))); // clear old priority > + value |= priority << (8 * (irq_number & 0x3)); // set new priority > + REG_WRITE32(addr, value); I believe the IPRIORITYR registers are byte addressable, so you can just do a strb of the new value without needing to read-modify-write. > +static void gic_route_interrupt(struct gic *gic, int irq_number, unsigned char cpu_set) > +{ > + uint32_t value; > + uint32_t *addr = REG(gicd(gic, GICD_ITARGETSR)) + (irq_number >> 2); > + value = REG_READ32(addr); > + value &= ~(0xff << (8 * (irq_number & 0x3))); // clear old target > + value |= cpu_set << (8 * (irq_number & 0x3)); // set new target > + REG_WRITE32(addr, value); Same for ITARGETSR. > +/* Note: not thread safe (but we only support one CPU for now anyway) */ > +static void gic_enable_interrupt(struct gic *gic, int irq_number, > + unsigned char cpu_set, unsigned char level_sensitive, unsigned char ppi) > +{ > + void *set_enable_reg; > + void *cfg_reg; > + > + // set priority > + gic_set_priority(gic, irq_number, 0x0); > + > + // set target cpus for this interrupt > + gic_route_interrupt(gic, irq_number, cpu_set); > + > + // set level/edge triggered > + cfg_reg = (void *)gicd(gic, GICD_ICFGR); > + if (level_sensitive) { > + clear_bit_non_atomic((irq_number * 2) + 1, cfg_reg); > + } else { > + set_bit_non_atomic((irq_number * 2) + 1, cfg_reg); > + } > + if (ppi) missing else? Or should be folded in above as level_sensitive||ppi > + clear_bit_non_atomic((irq_number * 2), cfg_reg); > + > + wmb(); > + > + // enable forwarding interrupt from distributor to cpu interface > + set_enable_reg = (void *)gicd(gic, GICD_ISENABLER); > + set_bit_non_atomic(irq_number, set_enable_reg); I think the semantics of ISENABLER are that you write the bit you want to enable as 1 and that zeroes are ignored. IOW you can just use a normal write with the correct shift. A clear is done via the separate ICENABLER, not by writing 0 to this register. > +//FIXME Get event_irq from dt > +#define EVENTS_IRQ 31 > +#define VIRTUALTIMER_IRQ 27 > + > +static void gic_handler(void) { > + unsigned int irq = gic_readiar(&gic); > + > + DEBUG("IRQ received : %i\n", irq); > + switch(irq) { > + case EVENTS_IRQ: > + do_hypervisor_callback(NULL); > + break; > + case VIRTUALTIMER_IRQ: > + /* We need to get this event to wake us up from block_domain, > + * but we don't need to do anything special with it. */ > + break; There are a few magic values in the 1020..1023 range which you may see in practice. In particular 1023 (I think) is a spurious interrupt, which you should silently ignore. > + gic_enable_interrupt(&gic, EVENTS_IRQ /* interrupt number */, 0x1 /*cpu_set*/, 1 /*level_sensitive*/, 0 /* ppi */); > + gic_enable_interrupt(&gic, VIRTUALTIMER_IRQ /* interrupt number */, 0x1 /*cpu_set*/, 1 /*level_sensitive*/, 1 /* ppi */); BTW, ppi or not is implicit in the irq number. > +}