Hello Giacomo Travaglini,

I'd like you to do a code review. Please visit

    https://gem5-review.googlesource.com/9769

to review the following change.


Change subject: dev, arm: Cleanup Pl050 interrupt handling
......................................................................

dev, arm: Cleanup Pl050 interrupt handling

Add support for TX interrupts and cleanup existing RX interrupt
handling.

Change-Id: If2e5b0c0cc6fbeb2dce09e7e9d935647516b2c47
Signed-off-by: Andreas Sandberg <andreas.sandb...@arm.com>
Reviewed-by: Sudhanshu Jha <sudhanshu....@arm.com>
Reviewed-by: Giacomo Travaglini <giacomo.travagl...@arm.com>
---
M src/dev/arm/RealView.py
M src/dev/arm/kmi.cc
M src/dev/arm/kmi.hh
3 files changed, 65 insertions(+), 74 deletions(-)



diff --git a/src/dev/arm/RealView.py b/src/dev/arm/RealView.py
index 7661db1..9b91f46 100644
--- a/src/dev/arm/RealView.py
+++ b/src/dev/arm/RealView.py
@@ -466,7 +466,6 @@
 class Pl050(AmbaIntDevice):
     type = 'Pl050'
     cxx_header = "dev/arm/kmi.hh"
-    int_delay = '1us'
     amba_id = 0x00141050

     ps2 = Param.PS2Device("PS/2 device")
diff --git a/src/dev/arm/kmi.cc b/src/dev/arm/kmi.cc
index d80bc14..e6e54a4 100644
--- a/src/dev/arm/kmi.cc
+++ b/src/dev/arm/kmi.cc
@@ -55,10 +55,9 @@
 Pl050::Pl050(const Pl050Params *p)
     : AmbaIntDevice(p, 0xfff), control(0), status(0x43), clkdiv(0),
       rawInterrupts(0),
-      intEvent([this]{ generateInterrupt(); }, name()),
       ps2(p->ps2)
 {
-    ps2->hostRegDataAvailable([this]() { this->updateIntStatus(); });
+    ps2->hostRegDataAvailable([this]() { this->updateRxInt(); });
 }

 Tick
@@ -84,8 +83,8 @@

       case kmiData:
         data = ps2->hostDataAvailable() ? ps2->hostRead() : 0;
+        updateRxInt();
         DPRINTF(Pl050, "Read Data: %#x\n", (uint32_t)data);
-        updateIntStatus();
         break;

       case kmiClkDiv:
@@ -108,21 +107,7 @@
         break;
     }

-    switch(pkt->getSize()) {
-      case 1:
-        pkt->set<uint8_t>(data);
-        break;
-      case 2:
-        pkt->set<uint16_t>(data);
-        break;
-      case 4:
-        pkt->set<uint32_t>(data);
-        break;
-      default:
-        panic("KMI read size too big?\n");
-        break;
-    }
-
+    pkt->setUintX(data, LittleEndianByteOrder);
     pkt->makeAtomicResponse();
     return pioDelay;
 }
@@ -134,29 +119,33 @@
assert(pkt->getAddr() >= pioAddr && pkt->getAddr() < pioAddr + pioSize);

     Addr daddr = pkt->getAddr() - pioAddr;
+    const uint32_t data = pkt->getUintX(LittleEndianByteOrder);

     assert(pkt->getSize() == sizeof(uint8_t));

-
     switch (daddr) {
       case kmiCr:
DPRINTF(Pl050, "Write Commmand: %#x\n", (uint32_t)pkt->get<uint8_t>());
-        control = pkt->get<uint8_t>();
-        updateIntStatus();
+        // Use the update interrupts helper to make sure any interrupt
+        // mask changes are handled correctly.
+        updateRawInts(0, 0, (uint8_t)data);
         break;

       case kmiData:
         DPRINTF(Pl050, "Write Data: %#x\n", (uint32_t)pkt->get<uint8_t>());
-        ps2->hostWrite(pkt->get<uint8_t>());
-        updateIntStatus();
+        // Clear the TX interrupt before writing new data.
+        setTxInt(false);
+        ps2->hostWrite((uint8_t)data);
+        // Data is written in 0 time, so raise the TX interrupt again.
+        setTxInt(true);
         break;

       case kmiClkDiv:
-        clkdiv = pkt->get<uint8_t>();
+        clkdiv = (uint8_t)data;
         break;

       default:
- warn("Tried to write PL050 at offset %#x that doesn't exist\n", daddr);
+        warn("PL050: Unhandled write of %#x to offset %#x\n", data, daddr);
         break;
     }

@@ -164,18 +153,44 @@
     return pioDelay;
 }

+void
+Pl050::setTxInt(bool value)
+{
+    InterruptReg set = 0, clear = 0;
+
+    set.tx = value ? 1 : 0;
+    clear.tx = !value ? 1 : 0;
+
+    updateRawInts(set, clear, control);
+}

 void
-Pl050::updateIntStatus()
+Pl050::updateRxInt()
 {
-    const bool old_interrupt(getInterrupt());
+    InterruptReg set = 0, clear = 0;

-    rawInterrupts.rx = ps2->hostDataAvailable() ? 1 : 0;
+    set.rx = ps2->hostDataAvailable() ? 1 : 0;
+    clear.rx = !ps2->hostDataAvailable() ? 1 : 0;

-    if ((!old_interrupt && getInterrupt()) && !intEvent.scheduled()) {
-        schedule(intEvent, curTick() + intDelay);
-    } else if (old_interrupt && !(getInterrupt())) {
-            gic->clearInt(intNum);
+    updateRawInts(set, clear, control);
+}
+
+void
+Pl050::updateRawInts(InterruptReg set, InterruptReg clear, ControlReg control)
+{
+    const bool old_pending(getInterrupt());
+    this->control = control;
+    rawInterrupts = (rawInterrupts & ~clear) | set;
+    const bool new_pending(getInterrupt());
+
+    if (!old_pending && new_pending) {
+        DPRINTF(Pl050, "Generate interrupt: rawInt=%#x ctrl=%#x int=%#x\n",
+                rawInterrupts, control, getInterrupt());
+        gic->sendInt(intNum);
+    } else if (old_pending && !new_pending) {
+        DPRINTF(Pl050, "Clear interrupt: rawInt=%#x ctrl=%#x int=%#x\n",
+                rawInterrupts, control, getInterrupt());
+        gic->clearInt(intNum);
     }
 }

@@ -191,47 +206,21 @@
 }

 void
-Pl050::generateInterrupt()
-{
-    DPRINTF(Pl050, "Generate Interrupt: rawInt=%#x ctrl=%#x int=%#x\n",
-            rawInterrupts, control, getInterrupt());
-
-    if (getInterrupt()) {
-        gic->sendInt(intNum);
-        DPRINTF(Pl050, " -- Generated\n");
-    }
-}
-
-void
 Pl050::serialize(CheckpointOut &cp) const
 {
-    uint8_t ctrlreg = control;
-    SERIALIZE_SCALAR(ctrlreg);
-
-    uint8_t stsreg = status;
-    SERIALIZE_SCALAR(stsreg);
+    paramOut(cp, "ctrlreg", control);
+    paramOut(cp, "stsreg", status);
     SERIALIZE_SCALAR(clkdiv);
-
-    uint8_t raw_ints = rawInterrupts;
-    SERIALIZE_SCALAR(raw_ints);
+    paramOut(cp, "raw_ints", rawInterrupts);
 }

 void
 Pl050::unserialize(CheckpointIn &cp)
 {
-    uint8_t ctrlreg;
-    UNSERIALIZE_SCALAR(ctrlreg);
-    control = ctrlreg;
-
-    uint8_t stsreg;
-    UNSERIALIZE_SCALAR(stsreg);
-    status = stsreg;
-
+    paramIn(cp, "ctrlreg", control);
+    paramIn(cp, "stsreg", status);
     UNSERIALIZE_SCALAR(clkdiv);
-
-    uint8_t raw_ints;
-    UNSERIALIZE_SCALAR(raw_ints);
-    rawInterrupts = raw_ints;
+    paramIn(cp, "raw_ints", rawInterrupts);
 }

 Pl050 *
diff --git a/src/dev/arm/kmi.hh b/src/dev/arm/kmi.hh
index 16a61a1..3a53007 100644
--- a/src/dev/arm/kmi.hh
+++ b/src/dev/arm/kmi.hh
@@ -106,19 +106,22 @@
     /** raw interrupt register (unmasked) */
     InterruptReg rawInterrupts;

- /** Update the status of the interrupt registers and schedule an interrupt
-     * if required */
-    void updateIntStatus();
+    /** Set or clear the TX interrupt */
+    void setTxInt(bool value);

-    /** Function to generate interrupt */
-    void generateInterrupt();
+    /** Update the RX interrupt using PS/2 device state */
+    void updateRxInt();

-    /** Get interrupt value */
+    /**
+     * Update the status of the interrupt registers and deliver an
+     * interrupt if required.
+     */
+    void updateRawInts(InterruptReg set, InterruptReg clear,
+                       ControlReg control);
+
+    /** Get current interrupt value */
     InterruptReg getInterrupt() const;

-    /** Wrapper to create an event out of the thing */
-    EventFunctionWrapper intEvent;
-
     /** PS2 device connected to this KMI interface */
     PS2Device *ps2;


--
To view, visit https://gem5-review.googlesource.com/9769
To unsubscribe, or for help writing mail filters, visit https://gem5-review.googlesource.com/settings

Gerrit-Project: public/gem5
Gerrit-Branch: master
Gerrit-Change-Id: If2e5b0c0cc6fbeb2dce09e7e9d935647516b2c47
Gerrit-Change-Number: 9769
Gerrit-PatchSet: 1
Gerrit-Owner: Andreas Sandberg <andreas.sandb...@arm.com>
Gerrit-Reviewer: Giacomo Travaglini <giacomo.travagl...@arm.com>
Gerrit-MessageType: newchange
_______________________________________________
gem5-dev mailing list
gem5-dev@gem5.org
http://m5sim.org/mailman/listinfo/gem5-dev

Reply via email to