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 <[email protected]>
Reviewed-by: Sudhanshu Jha <[email protected]>
Reviewed-by: Giacomo Travaglini <[email protected]>
---
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 <[email protected]>
Gerrit-Reviewer: Giacomo Travaglini <[email protected]>
Gerrit-MessageType: newchange
_______________________________________________
gem5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/gem5-dev
