From 335dd2c0ce529b7addb8ce9d59bee16b290f5659 Mon Sep 17 00:00:00 2001 From: Niklas Eiling Date: Tue, 9 Jan 2024 12:45:20 +0100 Subject: [PATCH] fix bug where Dino IP always read 0 from i2c. Add hardware mutex to I2c and to I2c::Switch Signed-off-by: Niklas Eiling --- fpga/include/villas/fpga/ips/dino.hpp | 6 ++-- fpga/include/villas/fpga/ips/i2c.hpp | 10 ++++++- fpga/lib/ips/dino.cpp | 41 +++++++++++++++++++++------ fpga/lib/ips/i2c.cpp | 41 ++++++++------------------- 4 files changed, 58 insertions(+), 40 deletions(-) diff --git a/fpga/include/villas/fpga/ips/dino.hpp b/fpga/include/villas/fpga/ips/dino.hpp index 0c83fe4a5..d8bde7b6c 100644 --- a/fpga/include/villas/fpga/ips/dino.hpp +++ b/fpga/include/villas/fpga/ips/dino.hpp @@ -52,13 +52,15 @@ public: return getMasterPort(masterPort); } - IoextPorts getIoextDir(); - IoextPorts getIoextOut(); + IoextPorts getIoextDirectionRegister(); + IoextPorts getIoextOutputRegister(); protected: std::shared_ptr i2cdev; uint8_t i2c_channel; + IoextPorts getIoextDir(); + IoextPorts getIoextOut(); void setIoextDir(IoextPorts ports); void setIoextOut(IoextPorts ports); }; diff --git a/fpga/include/villas/fpga/ips/i2c.hpp b/fpga/include/villas/fpga/ips/i2c.hpp index 4623e504e..1579d41e0 100644 --- a/fpga/include/villas/fpga/ips/i2c.hpp +++ b/fpga/include/villas/fpga/ips/i2c.hpp @@ -44,10 +44,16 @@ public: class Switch { public: Switch(I2c *i2c, uint8_t address) - : i2c(i2c), address(address), channel(0), readOnce(false){}; + : i2c(i2c), address(address), channel(0), readOnce(false), + switchLock(){}; Switch(const Switch &other) = delete; Switch &operator=(const Switch &other) = delete; void setChannel(uint8_t channel); + void setAndLockChannel(uint8_t channel) { + switchLock.lock(); + setChannel(channel); + } + void unlockChannel() { switchLock.unlock(); } uint8_t getChannel(); void setAddress(uint8_t address) { this->address = address; } uint8_t getAddress() { return address; } @@ -57,6 +63,7 @@ public: uint8_t address; uint8_t channel; bool readOnce; + std::mutex switchLock; }; Switch &getSwitch(uint8_t address = I2C_SWTICH_ADDR) { if (switchInstance == nullptr) { @@ -84,6 +91,7 @@ private: std::list getMemoryBlocks() const { return {registerMemory}; } + // assumes hwLock is locked void waitForBusNotBusy(); void driverWriteBlocking(u8 *dataPtr, size_t size); void driverReadBlocking(u8 *dataPtr, size_t max_read); diff --git a/fpga/lib/ips/dino.cpp b/fpga/lib/ips/dino.cpp index a18fcade0..252dfd5b0 100644 --- a/fpga/lib/ips/dino.cpp +++ b/fpga/lib/ips/dino.cpp @@ -37,24 +37,44 @@ Dino::IoextPorts Dino::getIoextDir() { if (i2cdev == nullptr) { throw RuntimeError("I2C device not set"); } - std::vector data = {0}; + std::vector data; i2cdev->readRegister(I2C_IOEXT_ADDR, I2C_IOEXT_REG_DIR, data, 1); IoextPorts ports; ports.raw = data[0]; return ports; } +Dino::IoextPorts Dino::getIoextDirectionRegister() { + if (i2cdev == nullptr) { + throw RuntimeError("I2C device not set"); + } + i2cdev->getSwitch().setAndLockChannel(i2c_channel); + auto ret = getIoextDir(); + i2cdev->getSwitch().unlockChannel(); + return ret; +} + Dino::IoextPorts Dino::getIoextOut() { if (i2cdev == nullptr) { throw RuntimeError("I2C device not set"); } - std::vector data = {0}; + std::vector data; i2cdev->readRegister(I2C_IOEXT_ADDR, I2C_IOEXT_REG_OUT, data, 1); IoextPorts ports; ports.raw = data[0]; return ports; } +Dino::IoextPorts Dino::getIoextOutputRegister() { + if (i2cdev == nullptr) { + throw RuntimeError("I2C device not set"); + } + i2cdev->getSwitch().setAndLockChannel(i2c_channel); + auto ret = getIoextOut(); + i2cdev->getSwitch().unlockChannel(); + return ret; +} + DinoAdc::DinoAdc() : Dino() {} DinoAdc::~DinoAdc() {} @@ -63,7 +83,7 @@ void DinoAdc::configureHardware() { if (i2cdev == nullptr) { throw RuntimeError("I2C device not set"); } - i2cdev->getSwitch().setChannel(i2c_channel); + i2cdev->getSwitch().setAndLockChannel(i2c_channel); IoextPorts ioext = {.raw = 0}; ioext.fields.sat_detect = true; setIoextDir(ioext); @@ -75,6 +95,7 @@ void DinoAdc::configureHardware() { setIoextOut(ioext); ioext.fields.n_we = false; setIoextOut(ioext); + i2cdev->getSwitch().unlockChannel(); } DinoDac::DinoDac() : Dino() {} @@ -85,11 +106,12 @@ void DinoDac::configureHardware() { if (i2cdev == nullptr) { throw RuntimeError("I2C device not set"); } - i2cdev->getSwitch().setChannel(i2c_channel); + i2cdev->getSwitch().setAndLockChannel(i2c_channel); IoextPorts ioext = {.raw = 0}; setIoextDir(ioext); ioext.fields.status_led = true; setIoextOut(ioext); + i2cdev->getSwitch().unlockChannel(); setGain(GAIN_1); } @@ -97,21 +119,24 @@ void DinoDac::setGain(Gain gain) { if (i2cdev == nullptr) { throw RuntimeError("I2C device not set"); } - i2cdev->getSwitch().setChannel(i2c_channel); + i2cdev->getSwitch().setAndLockChannel(i2c_channel); IoextPorts ioext = getIoextOut(); ioext.fields.gain_lsb = gain & 0x1; ioext.fields.gain_msb = gain & 0x2; setIoextOut(ioext); + i2cdev->getSwitch().unlockChannel(); } Dino::Gain DinoDac::getGain() { if (i2cdev == nullptr) { throw RuntimeError("I2C device not set"); } - i2cdev->getSwitch().setChannel(i2c_channel); + i2cdev->getSwitch().setAndLockChannel(i2c_channel); IoextPorts ioext = getIoextOut(); - return static_cast((ioext.fields.gain_msb << 1) | - ioext.fields.gain_lsb); + auto ret = + static_cast((ioext.fields.gain_msb << 1) | ioext.fields.gain_lsb); + i2cdev->getSwitch().unlockChannel(); + return ret; } static char n_adc[] = "DINO ADC"; diff --git a/fpga/lib/ips/i2c.cpp b/fpga/lib/ips/i2c.cpp index fde1a612e..2f8aa362b 100644 --- a/fpga/lib/ips/i2c.cpp +++ b/fpga/lib/ips/i2c.cpp @@ -42,6 +42,7 @@ bool I2c::init() { return true; } xConfig.BaseAddress = getBaseAddr(registerMemory); + hwLock.lock(); ret = XIic_CfgInitialize(&xIic, &xConfig, xConfig.BaseAddress); if (ret != XST_SUCCESS) { throw RuntimeError("Failed to initialize I2C"); @@ -53,12 +54,13 @@ bool I2c::init() { irqs[i2cInterrupt].irqController->enableInterrupt(irqs[i2cInterrupt], polling); - + hwLock.unlock(); initDone = true; return true; } bool I2c::reset() { + // we cannot lock here because this may be called in a destructor XIic_Reset(&xIic); irqs[i2cInterrupt].irqController->disableInterrupt(irqs[i2cInterrupt]); initDone = false; @@ -141,6 +143,7 @@ bool I2c::write(u8 address, std::vector &data) { throw RuntimeError("I2C not initialized"); } + hwLock.lock(); ret = XIic_SetAddress(&xIic, XII_ADDR_TO_SEND_TYPE, static_cast(address)); if (ret != XST_SUCCESS) { @@ -158,6 +161,7 @@ bool I2c::write(u8 address, std::vector &data) { if (ret != XST_SUCCESS) { throw RuntimeError("Failed to stop I2C"); } + hwLock.unlock(); return true; } @@ -171,6 +175,7 @@ bool I2c::read(u8 address, std::vector &data, size_t max_read) { data.resize(data.size() + max_read); u8 *dataPtr = data.data() + data.size() - max_read; + hwLock.lock(); ret = XIic_SetAddress(&xIic, XII_ADDR_TO_SEND_TYPE, static_cast(address)); if (ret != XST_SUCCESS) { @@ -188,41 +193,19 @@ bool I2c::read(u8 address, std::vector &data, size_t max_read) { if (ret != XST_SUCCESS) { throw RuntimeError("Failed to stop I2C"); } + hwLock.unlock(); return XST_SUCCESS; } bool I2c::readRegister(u8 address, u8 reg, std::vector &data, size_t max_read) { - int ret; - if (!initDone) { - throw RuntimeError("I2C not initialized"); - } - - data.resize(data.size() + max_read); - u8 *dataPtr = data.data() + data.size() - max_read; - - ret = - XIic_SetAddress(&xIic, XII_ADDR_TO_SEND_TYPE, static_cast(address)); - if (ret != XST_SUCCESS) { - throw RuntimeError("Failed to set I2C address"); - } - - ret = XIic_Start(&xIic); - if (ret != XST_SUCCESS) { - throw RuntimeError("Failed to start I2C"); - } - - driverWriteBlocking(®, sizeof(reg)); - driverReadBlocking(dataPtr, max_read); - - ret = XIic_Stop(&xIic); - if (ret != XST_SUCCESS) { - throw RuntimeError("Failed to stop I2C"); - } - - return XST_SUCCESS; + std::vector regData = {reg}; + bool ret; + ret = write(address, regData); + ret &= read(address, data, max_read); + return ret; } static const uint8_t CHANNEL_MAP[] = I2C_SWITCH_CHANNEL_MAP;