hps: Remove register read retries
dev.cc interface was retrying every register read and write. With these
retries operating we dont have a good idea what operations are failing or
why. Remove the unconditional retries so we can add them back where
necessary.
BUG=b:198098967
BUG=b:191716856
TEST=env FEATURES=test emerge-brya hpsd hps-tool
TEST=hps dl 0 /usr/lib/firmware/hps/mcu_stage1.bin
TEST=hps boot /dev/null /dev/null 4294967295
TEST=hps watch 0
Change-Id: If2f4470a6f9b79d75b84b24b52df099ef3f87cde
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3105850
Tested-by: Evan Benn <evanbenn@chromium.org>
Commit-Queue: Evan Benn <evanbenn@chromium.org>
Reviewed-by: Edward O'Callaghan <quasisec@chromium.org>
Reviewed-by: Stuart Langley <slangley@chromium.org>
diff --git a/hps/dev.cc b/hps/dev.cc
index 715f202..f7536cd 100644
--- a/hps/dev.cc
+++ b/hps/dev.cc
@@ -14,12 +14,6 @@
namespace hps {
-/*
- * TODO(amcrae): It is questionable whether this layer should be using
- * retries, since a retry shim layer can be added separately.
- */
-static const int kIoRetries = 5;
-
bool DevInterface::Read(uint8_t cmd, uint8_t* data, size_t len) {
if (this->ReadDevice(cmd, data, len)) {
VLOG(2) << "Read: " << cmd << " " << len << " OK";
@@ -45,7 +39,9 @@
int DevInterface::ReadReg(HpsReg r) {
uint8_t res[2];
- for (int i = 0; i < kIoRetries; i++) {
+ // TODO(evanbenn) MCP hal requires a retry on kBankReady
+ // b/191716856
+ for (int i = 0; i < 2; i++) {
if (this->ReadDevice(I2cReg(r), res, sizeof(res))) {
int ret = (static_cast<int>(res[0]) << 8) | static_cast<int>(res[1]);
VLOG(2) << "ReadReg: " << HpsRegToString(r) << " " << ret << " OK";
@@ -62,17 +58,19 @@
*/
bool DevInterface::WriteReg(HpsReg r, uint16_t data) {
uint8_t buf[2];
+
buf[0] = data >> 8;
buf[1] = data & 0xFF;
- for (int i = 0; i < kIoRetries; i++) {
- if (this->WriteDevice(I2cReg(r), buf, sizeof(buf))) {
- VLOG(2) << "WriteReg: " << HpsRegToString(r) << " " << data << " OK";
- return true;
- }
+
+ if (this->WriteDevice(I2cReg(r), buf, sizeof(buf))) {
+ VLOG(2) << "WriteReg: " << HpsRegToString(r) << " " << data << " OK";
+ return true;
+ } else {
+ VLOG(2) << "WriteReg: " << HpsRegToString(r) << " " << data << " FAILED";
+ return false;
}
- VLOG(2) << "WriteReg: " << HpsRegToString(r) << " " << data << " FAILED";
- return false;
}
+
/*
* Return the maximum download block size (in bytes).
* Default is 256 bytes.
diff --git a/hps/dev_test.cc b/hps/dev_test.cc
index 249b146..945bcc0 100644
--- a/hps/dev_test.cc
+++ b/hps/dev_test.cc
@@ -11,7 +11,6 @@
namespace {
-static int const kRetry = 5;
static int const kBlockSizeBytes = 128;
// Fake that implements a DevInterface.
@@ -102,50 +101,6 @@
}
/*
- * Verify that a Read will fail once the retries are exceeded.
- */
-TEST_F(DevInterfaceTest, ReadFail) {
- dev_.fails_ = kRetry;
- int d = dev_.ReadReg(hps::HpsReg::kMagic);
- EXPECT_EQ(d, -1);
- EXPECT_EQ(dev_.reads_, kRetry);
-}
-
-/*
- * Verify that a Write will fail once the retries are exceeded.
- */
-TEST_F(DevInterfaceTest, WriteFail) {
- dev_.fails_ = kRetry;
- EXPECT_FALSE(dev_.WriteReg(hps::HpsReg::kMagic, 0x1234));
- EXPECT_EQ(dev_.writes_, kRetry);
-}
-
-/*
- * Verify that a failed Read will be retried and succeed with
- * the second attempt.
- */
-TEST_F(DevInterfaceTest, ReadRetry) {
- dev_.fails_ = 1;
- dev_.data_[0] = 0x12;
- dev_.data_[1] = 0x34;
- int d = dev_.ReadReg(hps::HpsReg::kMagic);
- EXPECT_EQ(d, 0x1234);
- // One failed read, one success.
- EXPECT_EQ(dev_.reads_, 2);
-}
-
-/*
- * Verify that a failed Write will be retried and succeed with
- * the second attempt.
- */
-TEST_F(DevInterfaceTest, WriteRetry) {
- dev_.fails_ = 1;
- EXPECT_TRUE(dev_.WriteReg(hps::HpsReg::kMagic, 0x1234));
- // One failed write, one success.
- EXPECT_EQ(dev_.writes_, 2);
-}
-
-/*
* Verify that the correct block size is selected.
*/
TEST_F(DevInterfaceTest, BlockSize) {
