dev, arm: Clean up PL011 and rewrite interrupt handling

The ARM PL011 UART model didn't clear and raise interrupts
correctly. This changeset rewrites the whole interrupt handling and
makes it both simpler and fixes several cases where the correct
interrupts weren't raised or cleared. Additionally, it cleans up many
other aspects of the code.

2 files changed, 144 insertions, 152 deletions

diff --git a/src/dev/arm/pl011.cc b/src/dev/arm/pl011.cc
index d690ee9cb..2abd82e96 100644
--- a/src/dev/arm/pl011.cc
+++ b/src/dev/arm/pl011.cc
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010 ARM Limited
+ * Copyright (c) 2010, 2015 ARM Limited
  * All rights reserved
  *
  * The license below extends only to copyright in the software and shall
@@ -38,23 +38,29 @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  *
  * Authors: Ali Saidi
+ *          Andreas Sandberg
  */
 
+#include "dev/arm/pl011.hh"
+
 #include "base/trace.hh"
 #include "debug/Checkpoint.hh"
 #include "debug/Uart.hh"
 #include "dev/arm/amba_device.hh"
 #include "dev/arm/base_gic.hh"
-#include "dev/arm/pl011.hh"
 #include "dev/terminal.hh"
 #include "mem/packet.hh"
 #include "mem/packet_access.hh"
 #include "sim/sim_exit.hh"
-
-Pl011::Pl011(const Params *p)
-    : Uart(p, 0xfff), control(0x300), fbrd(0), ibrd(0), lcrh(0), ifls(0x12),
-      imsc(0), rawInt(0), maskInt(0), intNum(p->int_num), gic(p->gic),
-      endOnEOT(p->end_on_eot), intDelay(p->int_delay), intEvent(this)
+#include "params/Pl011.hh"
+
+Pl011::Pl011(const Pl011Params *p)
+    : Uart(p, 0xfff),
+      intEvent(this),
+      control(0x300), fbrd(0), ibrd(0), lcrh(0), ifls(0x12),
+      imsc(0), rawInt(0),
+      gic(p->gic), endOnEOT(p->end_on_eot), intNum(p->int_num),
+      intDelay(p->int_delay)
 {
 }
 
@@ -75,17 +81,23 @@ Pl011::read(PacketPtr pkt)
     switch(daddr) {
       case UART_DR:
         data = 0;
-        if (term->dataAvailable())
+        if (term->dataAvailable()) {
             data = term->in();
+            // Since we don't simulate a FIFO for incoming data, we
+            // assume it's empty and clear RXINTR and RTINTR.
+            clearInterrupts(UART_RXINTR | UART_RTINTR);
+        }
         break;
       case UART_FR:
-        // For now we're infintely fast, so TX is never full, always empty,
-        // always clear to send
-        data = UART_FR_TXFE | UART_FR_CTS;
-        if (!term->dataAvailable())
-            data |= UART_FR_RXFE;
-        DPRINTF(Uart, "Reading FR register as %#x rawInt=0x%x imsc=0x%x maskInt=0x%x\n",
-                data, rawInt, imsc, maskInt);
+        data =
+            UART_FR_CTS | // Clear To Send
+            (!term->dataAvailable() ? UART_FR_RXFE : 0) | // RX FIFO Empty
+            UART_FR_TXFE; // TX FIFO empty
+
+        DPRINTF(Uart,
+                "Reading FR register as %#x rawInt=0x%x "
+                "imsc=0x%x maskInt=0x%x\n",
+                data, rawInt, imsc, maskInt());
         break;
       case UART_CR:
         data = control;
@@ -110,8 +122,8 @@ Pl011::read(PacketPtr pkt)
         DPRINTF(Uart, "Reading Raw Int status as 0x%x\n", rawInt);
         break;
       case UART_MIS:
-        DPRINTF(Uart, "Reading Masked Int status as 0x%x\n", rawInt);
-        data = maskInt;
+        DPRINTF(Uart, "Reading Masked Int status as 0x%x\n", maskInt());
+        data = maskInt();
         break;
       default:
         if (readId(pkt, AMBA_ID, pioAddr)) {
@@ -182,15 +194,11 @@ Pl011::write(PacketPtr pkt)
             exitSimLoop("UART received EOT", 0);
 
         term->out(data & 0xFF);
-
-        //raw interrupt is set regardless of imsc.txim
-        rawInt.txim = 1;
-        if (imsc.txim) {
-            DPRINTF(Uart, "TX int enabled, scheduling interruptt\n");
-            if (!intEvent.scheduled())
-                schedule(intEvent, curTick() + intDelay);
-        }
-
+        // We're supposed to clear TXINTR when this register is
+        // written to, however. since we're also infinitely fast, we
+        // need to immediately raise it again.
+        clearInterrupts(UART_TXINTR);
+        raiseInterrupts(UART_TXINTR);
         break;
       case UART_CR:
         control = data;
@@ -208,35 +216,13 @@ Pl011::write(PacketPtr pkt)
         ifls = data;
         break;
       case UART_IMSC:
-        imsc = data;
-
-        if (imsc.feim || imsc.peim || imsc.beim || imsc.oeim || imsc.rsvd)
-            panic("Unknown interrupt enabled\n");
-
-        // rimim, ctsmim, dcdmim, dsrmim can be enabled but are ignored
-        // they are supposed to interrupt on a change of status in the line
-        // which we should never have since our terminal is happy to always
-        // receive bytes.
-
-        if (imsc.txim) {
-            DPRINTF(Uart, "Writing to IMSC: TX int enabled, scheduling interruptt\n");
-            rawInt.txim = 1;
-            if (!intEvent.scheduled())
-                schedule(intEvent, curTick() + intDelay);
-        }
-
+        DPRINTF(Uart, "Setting interrupt mask 0x%x\n", data);
+        setInterruptMask(data);
         break;
 
       case UART_ICR:
         DPRINTF(Uart, "Clearing interrupts 0x%x\n", data);
-        rawInt = rawInt & ~data;
-        maskInt = rawInt & imsc;
-
-        DPRINTF(Uart, " -- Masked interrupts 0x%x\n", maskInt);
-
-        if (!maskInt)
-            gic->clearInt(intNum);
-
+        clearInterrupts(data);
         break;
       default:
         panic("Tried to write PL011 at offset %#x that doesn't exist\n", daddr);
@@ -251,27 +237,36 @@ Pl011::dataAvailable()
 {
     /*@todo ignore the fifo, just say we have data now
      * We might want to fix this, or we might not care */
-    rawInt.rxim = 1;
-    rawInt.rtim = 1;
-
     DPRINTF(Uart, "Data available, scheduling interrupt\n");
-
-    if (!intEvent.scheduled())
-        schedule(intEvent, curTick() + intDelay);
+    raiseInterrupts(UART_RXINTR | UART_RTINTR);
 }
 
 void
 Pl011::generateInterrupt()
 {
     DPRINTF(Uart, "Generate Interrupt: imsc=0x%x rawInt=0x%x maskInt=0x%x\n",
-            imsc, rawInt, maskInt);
-    maskInt = imsc & rawInt;
+            imsc, rawInt, maskInt());
 
-    if (maskInt.rxim || maskInt.rtim || maskInt.txim) {
+    if (maskInt()) {
         gic->sendInt(intNum);
         DPRINTF(Uart, " -- Generated\n");
     }
+}
+
+void
+Pl011::setInterrupts(uint16_t ints, uint16_t mask)
+{
+    const bool old_ints(!!maskInt());
+
+    imsc = mask;
+    rawInt = ints;
 
+    if (!old_ints && maskInt()) {
+        if (!intEvent.scheduled())
+            schedule(intEvent, curTick() + intDelay);
+    } else if (old_ints && !maskInt()) {
+        gic->clearInt(intNum);
+    }
 }
 
 
@@ -286,17 +281,9 @@ Pl011::serialize(std::ostream &os)
     SERIALIZE_SCALAR(lcrh);
     SERIALIZE_SCALAR(ifls);
 
-    uint16_t imsc_serial = imsc;
-    SERIALIZE_SCALAR(imsc_serial);
-
-    uint16_t rawInt_serial = rawInt;
-    SERIALIZE_SCALAR(rawInt_serial);
-
-    uint16_t maskInt_serial = maskInt;
-    SERIALIZE_SCALAR(maskInt_serial);
-
-    SERIALIZE_SCALAR(endOnEOT);
-    SERIALIZE_SCALAR(intDelay);
+    // Preserve backwards compatibility by giving these silly names.
+    paramOut(os, "imsc_serial", imsc);
+    paramOut(os, "rawInt_serial", rawInt);
 }
 
 void
@@ -310,20 +297,9 @@ Pl011::unserialize(Checkpoint *cp, const std::string &section)
     UNSERIALIZE_SCALAR(lcrh);
     UNSERIALIZE_SCALAR(ifls);
 
-    uint16_t imsc_serial;
-    UNSERIALIZE_SCALAR(imsc_serial);
-    imsc = imsc_serial;
-
-    uint16_t rawInt_serial;
-    UNSERIALIZE_SCALAR(rawInt_serial);
-    rawInt = rawInt_serial;
-
-    uint16_t maskInt_serial;
-    UNSERIALIZE_SCALAR(maskInt_serial);
-    maskInt = maskInt_serial;
-
-    UNSERIALIZE_SCALAR(endOnEOT);
-    UNSERIALIZE_SCALAR(intDelay);
+    // Preserve backwards compatibility by giving these silly names.
+    paramIn(cp, section, "imsc_serial", imsc);
+    paramIn(cp, section, "rawInt_serial", rawInt);
 }
 
 Pl011 *
diff --git a/src/dev/arm/pl011.hh b/src/dev/arm/pl011.hh
index b5c55beab..f2587c08d 100644
--- a/src/dev/arm/pl011.hh
+++ b/src/dev/arm/pl011.hh
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010 ARM Limited
+ * Copyright (c) 2010-2015 ARM Limited
  * All rights reserved
  *
  * The license below extends only to copyright in the software and shall
@@ -38,6 +38,7 @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  *
  * Authors: Ali Saidi
+ *          Andreas Sandberg
  */
 
 
@@ -48,18 +49,73 @@
 #ifndef __DEV_ARM_PL011_H__
 #define __DEV_ARM_PL011_H__
 
-#include "base/bitfield.hh"
-#include "base/bitunion.hh"
 #include "dev/arm/amba_device.hh"
-#include "dev/io_device.hh"
 #include "dev/uart.hh"
-#include "params/Pl011.hh"
 
 class BaseGic;
+struct Pl011Params;
 
 class Pl011 : public Uart, public AmbaDevice
 {
-  protected:
+  public:
+    Pl011(const Pl011Params *p);
+
+    void serialize(std::ostream &os) M5_ATTR_OVERRIDE;
+    void unserialize(Checkpoint *cp, const std::string &sec) M5_ATTR_OVERRIDE;
+
+  public: // PioDevice
+    Tick read(PacketPtr pkt) M5_ATTR_OVERRIDE;
+    Tick write(PacketPtr pkt) M5_ATTR_OVERRIDE;
+
+  public: // Uart
+    void dataAvailable() M5_ATTR_OVERRIDE;
+
+
+  protected: // Interrupt handling
+    /** Function to generate interrupt */
+    void generateInterrupt();
+
+    /**
+     * Assign new interrupt values and update interrupt signals
+     *
+     * A new interrupt is scheduled signalled if the set of unmasked
+     * interrupts goes empty to non-empty. Conversely, if the set of
+     * unmasked interrupts goes from non-empty to empty, the interrupt
+     * signal is cleared.
+     *
+     * @param ints New <i>raw</i> interrupt status
+     * @param mask New interrupt mask
+     */
+    void setInterrupts(uint16_t ints, uint16_t mask);
+    /**
+     * Convenience function to update the interrupt mask
+     *
+     * @see setInterrupts
+     * @param mask New interrupt mask
+     */
+    void setInterruptMask(uint16_t mask) { setInterrupts(rawInt, mask); }
+    /**
+     * Convenience function to raise a new interrupt
+     *
+     * @see setInterrupts
+     * @param ints Set of interrupts to raise
+     */
+    void raiseInterrupts(uint16_t ints) { setInterrupts(rawInt | ints, imsc); }
+    /**
+     * Convenience function to clear interrupts
+     *
+     * @see setInterrupts
+     * @param ints Set of interrupts to clear
+     */
+    void clearInterrupts(uint16_t ints) { setInterrupts(rawInt & ~ints, imsc); }
+
+    /** Masked interrupt status register */
+    const inline uint16_t maskInt() const { return rawInt & imsc; }
+
+    /** Wrapper to create an event out of the thing */
+    EventWrapper<Pl011, &Pl011::generateInterrupt> intEvent;
+
+  protected: // Registers
     static const uint64_t AMBA_ID = ULL(0xb105f00d00341011);
     static const int UART_DR = 0x000;
     static const int UART_FR = 0x018;
@@ -76,6 +132,18 @@ class Pl011 : public Uart, public AmbaDevice
     static const int UART_MIS  = 0x040;
     static const int UART_ICR  = 0x044;
 
+    static const uint16_t UART_RIINTR = 1 << 0;
+    static const uint16_t UART_CTSINTR = 1 << 1;
+    static const uint16_t UART_CDCINTR = 1 << 2;
+    static const uint16_t UART_DSRINTR = 1 << 3;
+    static const uint16_t UART_RXINTR = 1 << 4;
+    static const uint16_t UART_TXINTR = 1 << 5;
+    static const uint16_t UART_RTINTR = 1 << 6;
+    static const uint16_t UART_FEINTR = 1 << 7;
+    static const uint16_t UART_PEINTR = 1 << 8;
+    static const uint16_t UART_BEINTR = 1 << 9;
+    static const uint16_t UART_OEINTR = 1 << 10;
+
     uint16_t control;
 
     /** fractional baud rate divisor. Not used for anything but reporting
@@ -94,76 +162,24 @@ class Pl011 : public Uart, public AmbaDevice
      * written value */
     uint16_t ifls;
 
-    BitUnion16(INTREG)
-        Bitfield<0> rimim;
-        Bitfield<1> ctsmim;
-        Bitfield<2> dcdmim;
-        Bitfield<3> dsrmim;
-        Bitfield<4> rxim;
-        Bitfield<5> txim;
-        Bitfield<6> rtim;
-        Bitfield<7> feim;
-        Bitfield<8> peim;
-        Bitfield<9> beim;
-        Bitfield<10> oeim;
-        Bitfield<15,11> rsvd;
-    EndBitUnion(INTREG)
-
     /** interrupt mask register. */
-    INTREG imsc;
+    uint16_t imsc;
 
     /** raw interrupt status register */
-    INTREG rawInt;
-
-    /** Masked interrupt status register */
-    INTREG maskInt;
-
-    /** Interrupt number to generate */
-    int intNum;
+    uint16_t rawInt;
 
+  protected: // Configuration
     /** Gic to use for interrupting */
-    BaseGic *gic;
+    BaseGic * const gic;
 
     /** Should the simulation end on an EOT */
-    bool endOnEOT;
-
-    /** Delay before interrupting */
-    Tick intDelay;
-
-    /** Function to generate interrupt */
-    void generateInterrupt();
-
-    /** Wrapper to create an event out of the thing */
-    EventWrapper<Pl011, &Pl011::generateInterrupt> intEvent;
+    const bool endOnEOT;
 
-  public:
-   typedef Pl011Params Params;
-   const Params *
-    params() const
-    {
-        return dynamic_cast<const Params *>(_params);
-    }
-    Pl011(const Params *p);
-
-    virtual Tick read(PacketPtr pkt);
-    virtual Tick write(PacketPtr pkt);
-
-    /**
-     * Inform the uart that there is data available.
-     */
-    virtual void dataAvailable();
-
-
-    /**
-     * Return if we have an interrupt pending
-     * @return interrupt status
-     * @todo fix me when implementation improves
-     */
-    virtual bool intStatus() { return false; }
-
-    virtual void serialize(std::ostream &os);
-    virtual void unserialize(Checkpoint *cp, const std::string &section);
+    /** Interrupt number to generate */
+    const int intNum;
 
+    /** Delay before interrupting */
+    const Tick intDelay;
 };
 
 #endif //__DEV_ARM_PL011_H__