Re: [PATCH ARM v8 2/4] mini-os: arm: interrupt controller

[Ian Campbell <Ian.Campbell@citrix.com>]

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.


> +}