x86: Use shared ISR for I2C and GPIO

This patch permits interrupts to be generated by both the I2C and GPIO
controllers for simultaneously-executing applications. The controllers
share a single interrupt pin, INTC. Prior to this patch,
quarkX1000_gpio_init() routed INTA to PIRQC and IRQ 10 (due to an
incorrect assumption that INTA is connected to the GPIO controller),
and quarkX1000_i2c_init() routed INTC to PIRQC and IRQ 9. The I2C
controller initialization is a prerequisite for GPIO initialization,
so the final configuration was that INTA and INTC were both routed to
PIRQC and IRQ 10. Thus, only the GPIO ISR was being invoked, even if
the I2C controller was actually responsible for the interrupt.

This patch refactors the I2C and GPIO ISR setup and handler code so
that the shared portions are combined in
cpu/x86/drivers/legacy_pc/shared-isr.[ch].  The I2C and GPIO drivers
communicate their interrupt information to the shared component by
placing structures in a specific section of the binary.

cpu/x86/Makefile.x86_quarkX1000

@@ -2,7 +2,7 @@ include $(CONTIKI)/cpu/x86/Makefile.x86_common
 
 CONTIKI_CPU_DIRS += drivers/legacy_pc drivers/quarkX1000 init/legacy_pc
 
-CONTIKI_SOURCEFILES += bootstrap_quarkX1000.S rtc.c pit.c pic.c irq.c nmi.c pci.c uart-16x50.c uart.c gpio.c i2c.c eth.c
+CONTIKI_SOURCEFILES += bootstrap_quarkX1000.S rtc.c pit.c pic.c irq.c nmi.c pci.c uart-16x50.c uart.c gpio.c i2c.c eth.c shared-isr.c
 
 CFLAGS += -m32 -march=i586 -mtune=i586
 LDFLAGS += -m32 -Xlinker -T -Xlinker $(CONTIKI)/cpu/x86/quarkX1000.ld

cpu/x86/drivers/legacy_pc/shared-isr.c

@@ -0,0 +1,107 @@
+/*
+ * Copyright (C) 2016, Intel Corporation. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * 3. Neither the name of the copyright holder nor the names of its
+ *    contributors may be used to endorse or promote products derived
+ *    from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE
+ * COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+ * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
+ * OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <assert.h>
+#include "idt.h"
+#include "interrupt.h"
+#include "pic.h"
+#include "shared-isr.h"
+
+/* Defined in linker script */
+extern shared_isr_client_t _sdata_shared_isr, _edata_shared_isr;
+
+static void __attribute__((used))
+shared_handler(void)
+{
+  shared_isr_client_t *client;
+  for(client = &_sdata_shared_isr; client < &_edata_shared_isr; client++) {
+    if(client->handler()) {
+      pic_eoi(client->irq);
+      return;
+    }
+  }
+}
+
+/**
+ * \brief Initialize shared ISR by iterating through all of its clients and
+ *        configuring their interrupts to route to the shared ISR.
+ */
+void
+shared_isr_init(void)
+{
+  shared_isr_client_t *client = &_sdata_shared_isr;
+  shared_isr_client_t *consistency_check_client;
+  bool prev_conf;
+
+  void shared_isr_stub(void);
+  __asm__ __volatile__ (
+    ISR_STUB("shared_isr_stub", 0, "shared_handler")
+  );
+
+  while(client < &_edata_shared_isr) {
+    consistency_check_client = &_sdata_shared_isr;
+
+    prev_conf = false;
+
+    while(consistency_check_client < client) {
+      if((client->irq == consistency_check_client->irq) ||
+         (client->pin == consistency_check_client->pin) ||
+         (client->pirq == consistency_check_client->pirq)) {
+
+        prev_conf = true;
+
+        /* This interrupt was previously configured. */
+        break;
+      }
+
+      consistency_check_client++;
+    }
+
+    if(prev_conf) {
+      /* The requested configurations for each IRQ must be consistent. */
+      assert((client->irq == consistency_check_client->irq) &&
+             (client->agent == consistency_check_client->agent) &&
+             (client->pin == consistency_check_client->pin) &&
+             (client->pirq == consistency_check_client->pirq));
+    } else {
+      idt_set_intr_gate_desc(PIC_INT(client->irq), (uint32_t)shared_isr_stub);
+
+      assert(pci_irq_agent_set_pirq(client->agent,
+                                    client->pin,
+                                    client->pirq) == 0);
+
+      pci_pirq_set_irq(client->pirq, client->irq, 1);
+
+      pic_unmask_irq(client->irq);
+    }
+
+    client++;
+  }
+}

cpu/x86/drivers/legacy_pc/shared-isr.h

@@ -0,0 +1,67 @@
+/*
+ * Copyright (C) 2016, Intel Corporation. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * 3. Neither the name of the copyright holder nor the names of its
+ *    contributors may be used to endorse or promote products derived
+ *    from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE
+ * COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+ * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
+ * OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifndef CPU_X86_DRIVERS_LEGACY_PC_SHARED_ISR_H_
+#define CPU_X86_DRIVERS_LEGACY_PC_SHARED_ISR_H_
+
+#include <stdbool.h>
+#include "pci.h"
+
+/**
+ * The handler function should return true if and only if it handled the
+ * interrupt.
+ */
+typedef bool (*shared_isr_handler_t)(void);
+
+typedef struct shared_isr_client {
+  uint8_t irq;
+  IRQAGENT agent;
+  INTR_PIN pin;
+  PIRQ pirq;
+  shared_isr_handler_t handler;
+} shared_isr_client_t;
+
+/* Unlike a non-shared interrupt handler function, an individual interrupt
+ * handler for a shared interrupt must not issue an EOI. The EOI is issued by
+ * the shared-isr subsystem.
+ */
+#define DEFINE_SHARED_IRQ(irq_, agent_, pin_, pirq_, handler_)                \
+static struct shared_isr_client                                               \
+  __attribute__((used, section(".shared_isr_data"))) _shared_irq_##irq_ = {   \
+  .irq = irq_,                                                                \
+  .agent = agent_,                                                            \
+  .pin = pin_,                                                                \
+  .pirq = pirq_,                                                              \
+  .handler = handler_                                                         \
+}
+
+void shared_isr_init(void);
+
+#endif /* CPU_X86_DRIVERS_LEGACY_PC_SHARED_ISR_H_ */

cpu/x86/drivers/quarkX1000/gpio.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015, Intel Corporation. All rights reserved.
+ * Copyright (C) 2015-2016, Intel Corporation. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -29,9 +29,9 @@
  */
 
 #include "gpio.h"
+
 #include "helpers.h"
-#include "interrupt.h"
+#include "shared-isr.h"
-#include "pic.h"
 
 /* GPIO Controler Registers */
 #define SWPORTA_DR    0x00
@@ -49,8 +49,7 @@
 
 #define PINS 8
 
-#define GPIO_IRQ 10
+#define GPIO_IRQ 9
-#define GPIO_INT PIC_INT(GPIO_IRQ)
 
 struct gpio_internal_data {
   pci_driver_t pci;
@@ -87,17 +86,23 @@ set_bit(uint32_t offset, uint32_t bit, uint32_t value)
   write(offset, reg);
 }
 
-static void
+static bool
 gpio_isr(void)
 {
   uint32_t int_status;
 
   int_status = read(INTSTATUS);
 
+  if(int_status == 0) {
+    return false;
+  }
+
   if (data.callback)
     data.callback(int_status);
 
   write(PORTA_EOI, -1);
+
+  return true;
 }
 
 static void
@@ -211,13 +216,7 @@ quarkX1000_gpio_clock_disable(void)
   set_bit(LS_SYNC, 0, 0);
 }
 
-static void
+DEFINE_SHARED_IRQ(GPIO_IRQ, IRQAGENT3, INTC, PIRQC, gpio_isr);
-gpio_handler(void)
-{
-  gpio_isr();
-
-  pic_eoi(GPIO_IRQ);
-}
 
 int
 quarkX1000_gpio_init(void)
@@ -232,13 +231,6 @@ quarkX1000_gpio_init(void)
 
   pci_command_enable(pci_addr, PCI_CMD_1_MEM_SPACE_EN);
 
-  SET_INTERRUPT_HANDLER(GPIO_INT, 0, gpio_handler);
-
-  if (pci_irq_agent_set_pirq(IRQAGENT3, INTA, PIRQC) < 0)
-    return -1;
-
-  pci_pirq_set_irq(PIRQC, GPIO_IRQ, 1);
-
   pci_init(&data.pci, pci_addr, 0);
 
   data.callback = 0;
@@ -250,7 +242,5 @@ quarkX1000_gpio_init(void)
   write(INTMASK, 0);
   write(PORTA_EOI, 0);
 
-  pic_unmask_irq(GPIO_IRQ);
-
   return 0;
 }

cpu/x86/drivers/quarkX1000/i2c.c

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015, Intel Corporation. All rights reserved.
+ * Copyright (C) 2015-2016, Intel Corporation. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -30,9 +30,9 @@
 
 #include "contiki.h"
 #include "i2c.h"
+
 #include "i2c-registers.h"
-#include "interrupt.h"
+#include "shared-isr.h"
-#include "pic.h"
 
 #define I2C_CLOCK_SPEED 25 /* kHz */
 #define I2C_FIFO_DEPTH  16
@@ -48,7 +48,6 @@
 #define I2C_POLLING_TIMEOUT (CLOCK_SECOND / 10)
 
 #define I2C_IRQ 9
-#define I2C_INT PIC_INT(I2C_IRQ)
 
 typedef enum {
   I2C_DIRECTION_READ,
@@ -169,9 +168,11 @@ i2c_data_send(void)
   device.tx_buffer += i;
 }
 
-static void
+static bool
 i2c_isr(void)
 {
+  bool handled = false;
+
   if (read(QUARKX1000_IC_INTR_STAT) & QUARKX1000_IC_INTR_STAT_STOP_DET_MASK) {
     i2c_data_read();
 
@@ -185,6 +186,8 @@ i2c_isr(void)
       if (device.config.cb_rx)
         device.config.cb_rx();
     }
+
+    handled = true;
   }
 
   if (read(QUARKX1000_IC_INTR_STAT) & QUARKX1000_IC_INTR_STAT_TX_EMPTY_MASK) {
@@ -195,11 +198,16 @@ i2c_isr(void)
       set_value(QUARKX1000_IC_INTR_MASK,
         QUARKX1000_IC_INTR_STAT_STOP_DET_MASK, QUARKX1000_IC_INTR_STAT_STOP_DET_SHIFT, 1);
     }
+
+    handled = true;
   }
 
-  if (read(QUARKX1000_IC_INTR_STAT) & QUARKX1000_IC_INTR_STAT_RX_FULL_MASK)
+  if(read(QUARKX1000_IC_INTR_STAT) & QUARKX1000_IC_INTR_STAT_RX_FULL_MASK) {
     i2c_data_read();
 
+    handled = true;
+  }
+
   if (read(QUARKX1000_IC_INTR_STAT) & (QUARKX1000_IC_INTR_STAT_TX_ABRT_MASK
     | QUARKX1000_IC_INTR_STAT_TX_OVER_MASK | QUARKX1000_IC_INTR_STAT_RX_OVER_MASK
     | QUARKX1000_IC_INTR_STAT_RX_UNDER_MASK)) {
@@ -208,7 +216,11 @@ i2c_isr(void)
 
     if (device.config.cb_err)
       device.config.cb_err();
+
+    handled = true;
   }
+
+  return handled;
 }
 
 int
@@ -480,13 +492,7 @@ quarkX1000_i2c_is_available(void)
   return device.pci.mmio ? 1 : 0;
 }
 
-static void
+DEFINE_SHARED_IRQ(I2C_IRQ, IRQAGENT3, INTC, PIRQC, i2c_isr);
-i2c_handler()
-{
-  i2c_isr();
-
-  pic_eoi(I2C_IRQ);
-}
 
 int
 quarkX1000_i2c_init(void)
@@ -501,16 +507,7 @@ quarkX1000_i2c_init(void)
 
   pci_command_enable(pci_addr, PCI_CMD_1_MEM_SPACE_EN);
 
-  SET_INTERRUPT_HANDLER(I2C_INT, 0, i2c_handler);
-
-  if (pci_irq_agent_set_pirq(IRQAGENT3, INTC, PIRQC) < 0)
-    return -1;
-
-  pci_pirq_set_irq(PIRQC, I2C_IRQ, 1);
-
   pci_init(&device.pci, pci_addr, 0);
 
-  pic_unmask_irq(I2C_IRQ);
-
   return 0;
 }

cpu/x86/init/common/interrupt.h

@@ -48,6 +48,19 @@ struct interrupt_context {
   uint32_t eip;
 };
 
+#define ISR_STUB(label_str, has_error_code, handler_str)         \
+  "jmp 2f\n\t"                                                   \
+  ".align 4\n\t"                                                 \
+  label_str ":\n\t"                                              \
+  "         pushal\n\t"                                          \
+  "         call " handler_str "\n\t"                            \
+  "         popal\n\t"                                           \
+  "         .if " #has_error_code "\n\t"                         \
+  "         add $4, %%esp\n\t"                                   \
+  "         .endif\n\t"                                          \
+  "         iret\n\t"                                            \
+  "2:\n\t"
+
 /* Helper macro to register interrupt handler function.
  *
  * num:             Interrupt number (0-255)
@@ -75,17 +88,7 @@ struct interrupt_context {
       "push %0\n\t"                                              \
       "call %P1\n\t"                                             \
       "add $8, %%esp\n\t"                                        \
-      "jmp 2f\n\t"                                               \
+      ISR_STUB("1", has_error_code, "%P2")                       \
-      ".align 4\n\t"                                             \
-      "1:\n\t"                                                   \
-      "         pushal\n\t"                                      \
-      "         call %P2\n\t"                                    \
-      "         popal\n\t"                                       \
-      "         .if " #has_error_code "\n\t"                     \
-      "         add $4, %%esp\n\t"                               \
-      "         .endif\n\t"                                      \
-      "         iret\n\t"                                        \
-      "2:\n\t"                                                   \
       :: "g" (num), "i" (idt_set_intr_gate_desc), "i" (handler)  \
       : "eax", "ecx", "edx"                                      \
     );                                                           \

cpu/x86/quarkX1000.ld

@@ -61,6 +61,10 @@ SECTIONS {
     .rodata ALIGN (32) :
     {
         *(.rodata*)
+
+        _sdata_shared_isr = .;
+        KEEP(*(.shared_isr_data*))
+        _edata_shared_isr = .;
     }
 
     .data ALIGN (32) :

platform/galileo/contiki-main.c

@@ -33,9 +33,10 @@
 #include "contiki.h"
 #include "contiki-net.h"
 #include "cpu.h"
-#include "interrupt.h"
-#include "uart.h"
 #include "eth-conf.h"
+#include "interrupt.h"
+#include "shared-isr.h"
+#include "uart.h"
 
 PROCINIT(  &etimer_process
          , &tcpip_process
@@ -64,6 +65,8 @@ main(void)
 
   eth_init();
 
+  shared_isr_init();
+
   while(1) {
     process_run();
   }