Hello Giacomo Travaglini,
I'd like you to do a code review. Please visit
https://gem5-review.googlesource.com/9765
to review the following change.
Change subject: ps2: Unify device data buffering
......................................................................
ps2: Unify device data buffering
All PS/2 device currently implement various ad-hoc mechanisms to
handle multi-byte commands. This is error-prone and makes it hard to
implement new devices. Create a buffering mechanism in the base class
to avoid this.
Change-Id: If5638b0ab68decea8de7631ecead0a9ebad1547b
Signed-off-by: Andreas Sandberg <[email protected]>
Reviewed-by: Giacomo Travaglini <[email protected]>
---
M src/dev/ps2/device.cc
M src/dev/ps2/device.hh
M src/dev/ps2/keyboard.cc
M src/dev/ps2/keyboard.hh
M src/dev/ps2/mouse.cc
M src/dev/ps2/mouse.hh
M src/dev/ps2/touchkit.cc
M src/dev/ps2/touchkit.hh
8 files changed, 111 insertions(+), 126 deletions(-)
diff --git a/src/dev/ps2/device.cc b/src/dev/ps2/device.cc
index 7019423..073e015 100644
--- a/src/dev/ps2/device.cc
+++ b/src/dev/ps2/device.cc
@@ -44,12 +44,14 @@
#include "dev/ps2/device.hh"
#include "base/logging.hh"
+#include "debug/PS2.hh"
#include "dev/ps2.hh"
#include "params/PS2Device.hh"
PS2Device::PS2Device(const PS2DeviceParams *p)
: SimObject(p)
{
+ inBuffer.reserve(16);
}
void
@@ -58,6 +60,8 @@
std::vector<uint8_t> buffer(outBuffer.size());
std::copy(outBuffer.begin(), outBuffer.end(), buffer.begin());
arrayParamOut(cp, "outBuffer", buffer);
+
+ SERIALIZE_CONTAINER(inBuffer);
}
void
@@ -67,6 +71,8 @@
arrayParamIn(cp, "outBuffer", buffer);
for (auto c : buffer)
outBuffer.push_back(c);
+
+ UNSERIALIZE_CONTAINER(inBuffer);
}
void
@@ -89,7 +95,10 @@
void
PS2Device::hostWrite(uint8_t c)
{
- recv(c);
+ DPRINTF(PS2, "PS2: Host -> device: %#x\n", c);
+ inBuffer.push_back(c);
+ if (recv(inBuffer))
+ inBuffer.clear();
}
void
@@ -97,6 +106,7 @@
{
assert(data || size == 0);
while (size) {
+ DPRINTF(PS2, "PS2: Device -> host: %#x\n", *data);
outBuffer.push_back(*(data++));
size--;
}
diff --git a/src/dev/ps2/device.hh b/src/dev/ps2/device.hh
index 342a8c2..b485c5e 100644
--- a/src/dev/ps2/device.hh
+++ b/src/dev/ps2/device.hh
@@ -45,6 +45,7 @@
#define __DEV_PS2_DEVICE_HH__
#include <deque>
+#include <vector>
#include "sim/sim_object.hh"
@@ -92,8 +93,18 @@
protected: /* Device interface */
/**
* Data received from host.
+ *
+ * This method is called whenever the host sends a byte to the
+ * device. The device model may request buffering in the base
+ * class by returning false. Once all data in the buffer has been
+ * processed, the method should return true which clears the
+ * buffer.
+ *
+ * @param data Pending input data (at least one byte)
+ * @return false if more data is needed to process the current
+ * command, true otherwise.
*/
- virtual void recv(uint8_t data) = 0;
+ virtual bool recv(const std::vector<uint8_t> &data) = 0;
/**
* Send data from a PS/2 device to a host
@@ -125,6 +136,9 @@
/** Device -> host FIFO */
std::deque<uint8_t> outBuffer;
+ /** Host -> device buffer */
+ std::vector<uint8_t> inBuffer;
+
std::function<void()> dataAvailableCallback;
};
diff --git a/src/dev/ps2/keyboard.cc b/src/dev/ps2/keyboard.cc
index 46b89fa..1f8b544 100644
--- a/src/dev/ps2/keyboard.cc
+++ b/src/dev/ps2/keyboard.cc
@@ -52,7 +52,6 @@
PS2Keyboard::PS2Keyboard(const PS2KeyboardParams *p)
: PS2Device(p),
- lastCommand(NoCommand),
shiftDown(false),
enabled(false)
{
@@ -64,7 +63,6 @@
PS2Keyboard::serialize(CheckpointOut &cp) const
{
PS2Device::serialize(cp);
- SERIALIZE_SCALAR(lastCommand);
SERIALIZE_SCALAR(shiftDown);
SERIALIZE_SCALAR(enabled);
}
@@ -73,40 +71,28 @@
PS2Keyboard::unserialize(CheckpointIn &cp)
{
PS2Device::unserialize(cp);
- UNSERIALIZE_SCALAR(lastCommand);
UNSERIALIZE_SCALAR(shiftDown);
UNSERIALIZE_SCALAR(enabled);
}
-void
-PS2Keyboard::recv(uint8_t data)
+bool
+PS2Keyboard::recv(const std::vector<uint8_t> &data)
{
- if (lastCommand != NoCommand) {
- switch (lastCommand) {
- case LEDWrite:
+ switch (data[0]) {
+ case LEDWrite:
+ if (data.size() == 1) {
+ DPRINTF(PS2, "Got LED write command.\n");
+ sendAck();
+ return false;
+ } else {
DPRINTF(PS2, "Setting LEDs: "
"caps lock %s, num lock %s, scroll lock %s\n",
- bits(data, 2) ? "on" : "off",
- bits(data, 1) ? "on" : "off",
- bits(data, 0) ? "on" : "off");
+ bits(data[1], 2) ? "on" : "off",
+ bits(data[1], 1) ? "on" : "off",
+ bits(data[1], 0) ? "on" : "off");
sendAck();
- lastCommand = NoCommand;
- break;
- case TypematicInfo:
- DPRINTF(PS2, "Setting typematic info to %#02x.\n", data);
- sendAck();
- lastCommand = NoCommand;
- break;
+ return true;
}
- return;
- }
-
- switch (data) {
- case LEDWrite:
- DPRINTF(PS2, "Got LED write command.\n");
- sendAck();
- lastCommand = LEDWrite;
- break;
case DiagnosticEcho:
panic("Keyboard diagnostic echo unimplemented.\n");
case AlternateScanCodes:
@@ -115,27 +101,32 @@
DPRINTF(PS2, "Got keyboard read ID command.\n");
sendAck();
send((uint8_t *)&ID, sizeof(ID));
- break;
+ return true;
case TypematicInfo:
- DPRINTF(PS2, "Setting typematic info.\n");
- sendAck();
- lastCommand = TypematicInfo;
- break;
+ if (data.size() == 1) {
+ DPRINTF(PS2, "Setting typematic info.\n");
+ sendAck();
+ return false;
+ } else {
+ DPRINTF(PS2, "Setting typematic info to %#02x.\n", data[1]);
+ sendAck();
+ return true;
+ }
case Enable:
DPRINTF(PS2, "Enabling the keyboard.\n");
enabled = true;
sendAck();
- break;
+ return true;
case Disable:
DPRINTF(PS2, "Disabling the keyboard.\n");
enabled = false;
sendAck();
- break;
+ return true;
case DefaultsAndDisable:
DPRINTF(PS2, "Disabling and resetting the keyboard.\n");
enabled = false;
sendAck();
- break;
+ return true;
case AllKeysToTypematic:
panic("Setting all keys to typemantic unimplemented.\n");
case AllKeysToMakeRelease:
@@ -156,7 +147,7 @@
case Reset:
panic("Keyboard reset unimplemented.\n");
default:
- panic("Unknown keyboard command %#02x.\n", data);
+ panic("Unknown keyboard command %#02x.\n", data[0]);
}
}
diff --git a/src/dev/ps2/keyboard.hh b/src/dev/ps2/keyboard.hh
index f5d8304..33db1cc 100644
--- a/src/dev/ps2/keyboard.hh
+++ b/src/dev/ps2/keyboard.hh
@@ -74,11 +74,8 @@
Resend = 0xFE,
Reset = 0xFF
};
- static const uint16_t NoCommand = (uint16_t)(-1);
- uint16_t lastCommand;
-
/** is the shift key currently down */
bool shiftDown;
@@ -92,7 +89,7 @@
void unserialize(CheckpointIn &cp) override;
protected: // PS2Device
- void recv(uint8_t data) override;
+ bool recv(const std::vector<uint8_t> &data) override;
public: // VncKeyboard
void keyPress(uint32_t key, bool down) override;
diff --git a/src/dev/ps2/mouse.cc b/src/dev/ps2/mouse.cc
index 1bdf39c..5b6765b 100644
--- a/src/dev/ps2/mouse.cc
+++ b/src/dev/ps2/mouse.cc
@@ -52,57 +52,42 @@
PS2Mouse::PS2Mouse(const PS2MouseParams *p)
: PS2Device(p),
- lastCommand(NoCommand),
status(0), resolution(4), sampleRate(100)
{
}
-void
-PS2Mouse::recv(uint8_t data)
+bool
+PS2Mouse::recv(const std::vector<uint8_t> &data)
{
- if (lastCommand != NoCommand) {
- switch(lastCommand) {
- case SetResolution:
- DPRINTF(PS2, "Mouse resolution set to %d.\n", data);
- resolution = data;
- sendAck();
- lastCommand = NoCommand;
- break;
- case SampleRate:
- DPRINTF(PS2, "Mouse sample rate %d samples "
- "per second.\n", data);
- sampleRate = data;
- sendAck();
- lastCommand = NoCommand;
- break;
- default:
- panic("Not expecting data for a mouse command.\n");
- }
- return;
- }
- switch (data) {
+ switch (data[0]) {
case Scale1to1:
DPRINTF(PS2, "Setting mouse scale to 1:1.\n");
status.twoToOne = 0;
sendAck();
- break;
+ return true;
case Scale2to1:
DPRINTF(PS2, "Setting mouse scale to 2:1.\n");
status.twoToOne = 1;
sendAck();
- break;
+ return true;
case SetResolution:
- DPRINTF(PS2, "Setting mouse resolution.\n");
- lastCommand = SetResolution;
- sendAck();
- break;
+ if (data.size() == 1) {
+ DPRINTF(PS2, "Setting mouse resolution.\n");
+ sendAck();
+ return false;
+ } else {
+ DPRINTF(PS2, "Mouse resolution set to %d.\n", data[1]);
+ resolution = data[1];
+ sendAck();
+ return true;
+ }
case GetStatus:
DPRINTF(PS2, "Getting mouse status.\n");
sendAck();
send((uint8_t *)&(status), 1);
send(&resolution, sizeof(resolution));
send(&sampleRate, sizeof(sampleRate));
- break;
+ return true;
case ReadData:
panic("Reading mouse data unimplemented.\n");
case ResetWrapMode:
@@ -115,22 +100,29 @@
DPRINTF(PS2, "Mouse ID requested.\n");
sendAck();
send(ID, sizeof(ID));
- break;
+ return true;
case SampleRate:
- DPRINTF(PS2, "Setting mouse sample rate.\n");
- lastCommand = SampleRate;
- sendAck();
- break;
+ if (data.size() == 1) {
+ DPRINTF(PS2, "Setting mouse sample rate.\n");
+ sendAck();
+ return false;
+ } else {
+ DPRINTF(PS2, "Mouse sample rate %d samples "
+ "per second.\n", data[1]);
+ sampleRate = data[1];
+ sendAck();
+ return true;
+ }
case DisableReporting:
DPRINTF(PS2, "Disabling data reporting.\n");
status.enabled = 0;
sendAck();
- break;
+ return true;
case EnableReporting:
DPRINTF(PS2, "Enabling data reporting.\n");
status.enabled = 1;
sendAck();
- break;
+ return true;
case DefaultsAndDisable:
DPRINTF(PS2, "Disabling and resetting mouse.\n");
sampleRate = 100;
@@ -138,7 +130,7 @@
status.twoToOne = 0;
status.enabled = 0;
sendAck();
- break;
+ return true;
case Resend:
panic("Mouse resend unimplemented.\n");
case Reset:
@@ -150,11 +142,11 @@
sendAck();
send(&BatSuccessful, sizeof(BatSuccessful));
send(ID, sizeof(ID));
- break;
+ return true;
default:
- warn("Unknown mouse command %#02x.\n", data);
+ warn("Unknown mouse command %#02x.\n", data[0]);
send(Resend);
- break;
+ return true;
}
}
@@ -163,8 +155,6 @@
{
PS2Device::serialize(cp);
- SERIALIZE_SCALAR(lastCommand);
-
SERIALIZE_SCALAR(status);
SERIALIZE_SCALAR(resolution);
SERIALIZE_SCALAR(sampleRate);
@@ -175,8 +165,6 @@
{
PS2Device::unserialize(cp);
- UNSERIALIZE_SCALAR(lastCommand);
-
UNSERIALIZE_SCALAR(status);
UNSERIALIZE_SCALAR(resolution);
UNSERIALIZE_SCALAR(sampleRate);
diff --git a/src/dev/ps2/mouse.hh b/src/dev/ps2/mouse.hh
index e0b4c53..9150f3f 100644
--- a/src/dev/ps2/mouse.hh
+++ b/src/dev/ps2/mouse.hh
@@ -71,7 +71,6 @@
Resend = 0xFE,
Reset = 0xFF
};
- static const uint16_t NoCommand = (uint16_t)(-1);
BitUnion8(Status)
Bitfield<6> remote;
@@ -81,8 +80,6 @@
Bitfield<0> rightButton;
EndBitUnion(Status)
- uint16_t lastCommand;
-
Status status;
uint8_t resolution;
uint8_t sampleRate;
@@ -94,7 +91,7 @@
void unserialize(CheckpointIn &cp) override;
protected: // PS2Device
- void recv(uint8_t data) override;
+ bool recv(const std::vector<uint8_t> &data) override;
};
#endif // __DEV_PS2_MOUSE_hH__
diff --git a/src/dev/ps2/touchkit.cc b/src/dev/ps2/touchkit.cc
index e5ee3ef..1617561 100644
--- a/src/dev/ps2/touchkit.cc
+++ b/src/dev/ps2/touchkit.cc
@@ -54,7 +54,6 @@
PS2TouchKit::PS2TouchKit(const PS2TouchKitParams *p)
: PS2Device(p),
vnc(p->vnc),
- ackNext(false),
driverInitialized(false)
{
if (vnc)
@@ -66,7 +65,6 @@
{
PS2Device::serialize(cp);
- SERIALIZE_SCALAR(ackNext);
SERIALIZE_SCALAR(driverInitialized);
}
@@ -75,36 +73,28 @@
{
PS2Device::unserialize(cp);
- UNSERIALIZE_SCALAR(ackNext);
UNSERIALIZE_SCALAR(driverInitialized);
}
-void
-PS2TouchKit::recv(uint8_t data)
+bool
+PS2TouchKit::recv(const std::vector<uint8_t> &data)
{
- if (ackNext) {
- ackNext--;
- sendAck();
- return;
- }
-
- switch (data) {
+ switch (data[0]) {
case Ps2::Ps2Reset:
sendAck();
send(Ps2::SelfTestPass);
- break;
+ return true;
case Ps2::SetResolution:
case Ps2::SetRate:
case Ps2::SetStatusLed:
sendAck();
- ackNext = 1;
- break;
+ return data.size() == 2;
case Ps2::ReadId:
sendAck();
send((const uint8_t *)&ID, sizeof(ID));
- break;
+ return true;
case Ps2::TpReadId:
// We're not a trackpoint device, this should make the probe
@@ -113,7 +103,7 @@
send(0);
send(0);
sendAck();
- break;
+ return true;
case Ps2::SetScaling1_1:
case Ps2::SetScaling1_2:
@@ -121,27 +111,32 @@
case Ps2::Enable:
case Ps2::SetDefaults:
sendAck();
- break;
+ return true;
case Ps2::StatusRequest:
sendAck();
send(0);
send(2); // default resolution
send(100); // default sample rate
- break;
+ return true;
case Ps2::TouchKitId:
- ackNext = 2;
sendAck();
- send(Ps2::TouchKitId);
- send(1);
- send('A');
+ if (data.size() == 1) {
+ send(Ps2::TouchKitId);
+ send(1);
+ send('A');
- driverInitialized = true;
- break;
+ return false;
+ } else if (data.size() == 3) {
+ driverInitialized = true;
+ return true;
+ } else {
+ return false;
+ }
default:
- panic("Unknown byte received: %d\n", data);
+ panic("Unknown byte received: %d\n", data[0]);
}
}
diff --git a/src/dev/ps2/touchkit.hh b/src/dev/ps2/touchkit.hh
index 6f23132..f5ef398 100644
--- a/src/dev/ps2/touchkit.hh
+++ b/src/dev/ps2/touchkit.hh
@@ -57,7 +57,7 @@
void unserialize(CheckpointIn &cp) override;
protected: // PS2Device
- void recv(uint8_t data) override;
+ bool recv(const std::vector<uint8_t> &data) override;
public: // VncMouse
void mouseAt(uint16_t x, uint16_t y, uint8_t buttons) override;
@@ -67,13 +67,6 @@
VncInput *const vnc;
/**
- * If the controller should ignore the next data byte and
- * acknowledge it. The driver is attempting to setup some feature
- * we don't care about.
- */
- int ackNext;
-
- /**
* If the linux driver has initialized the device yet and thus can
* we send mouse data.
*/
--
To view, visit https://gem5-review.googlesource.com/9765
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: If5638b0ab68decea8de7631ecead0a9ebad1547b
Gerrit-Change-Number: 9765
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
