Skip to content

Commit

Permalink
registers: fix a mask issue
Browse files Browse the repository at this point in the history 
- the default mask should be (1ULL << (sizeof(TYPE) * CHAR_BIT)) - 1ULL
instead of (1ULL << (sizeof(TYPE)) - 1ULL.
- add default register mask test case.
- add 64 bit register access test case.

Signed-off-by: Mahmoud Kamel <quic_mkamel@quicinc.com>
  • Loading branch information
Mahmoud Kamel committed Dec 4, 2024
1 parent ae3e3ec commit efb0403
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 18 deletions.
29 changes: 19 additions & 10 deletions systemc-components/common/include/registers.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,17 @@
#include <tlm_sockets_buswidth.h>
#include <vector>
#include <algorithm>
#include <limits>
#include <bitset>

namespace gs {

template <typename T>
static T gs_full_mask()
{
return static_cast<T>(std::bitset<std::numeric_limits<T>::digits>(0).flip().to_ullong());
}

/**
* @brief Provide a class to provide b_transport callback
*
Expand Down Expand Up @@ -244,7 +252,7 @@ class proxy_data_array
if (m_dmi) {
SCP_TRACE(())("Set value (DMI) : [{:#x}]", fmt::join(std::vector<TYPE>(&src[0], &src[length]), ","));
TYPE curr_val = m_dmi[idx];
if (!use_mask || (p_mask.get_value() == ((1ULL << sizeof(TYPE)) - 1ULL))) {
if (!use_mask || (p_mask.get_value() == gs_full_mask<TYPE>())) {
memcpy(&m_dmi[idx], src, sizeof(TYPE) * length);
} else {
write_with_mask(src, reinterpret_cast<TYPE*>(&m_dmi[idx]), length);
Expand All @@ -253,7 +261,7 @@ class proxy_data_array
tlm::tlm_generic_payload m_txn;
sc_core::sc_time dummy;
std::vector<unsigned char> curr_data;
if (p_mask.get_value() == ((1ULL << sizeof(TYPE)) - 1ULL) || !use_mask) {
if ((p_mask.get_value() == gs_full_mask<TYPE>()) || !use_mask) {
m_txn.set_data_ptr(reinterpret_cast<unsigned char*>(src));
} else {
curr_data.resize(sizeof(TYPE) * length);
Expand Down Expand Up @@ -306,8 +314,8 @@ class proxy_data_array
}
}
SCP_TRACE(())("Access value (DMI) using operator [] at idx {}", idx);
if (p_mask.get_value() != ((1ULL << sizeof(TYPE)) - 1ULL)) {
SCP_FATAL(()) << "operator[](): Register Mask: " << std::hex << p_mask.get_value()
if (p_mask.get_value() != gs_full_mask<TYPE>()) {
SCP_FATAL(()) << "operator[](): Register Mask: 0x" << std::hex << p_mask.get_value()
<< " has readonly bits, please use set(TYPE* src, uint64_t idx, uint64_t length) and "
"get(TYPE* dst, uint64_t idx, uint64_t length) instead";
}
Expand All @@ -316,7 +324,7 @@ class proxy_data_array
void invalidate_direct_mem_ptr(sc_dt::uint64 start, sc_dt::uint64 end) { m_dmi = nullptr; }

proxy_data_array(scp::scp_logger_cache& logger, std::string name, std::string path_name, uint64_t _offset = 0,
uint64_t number = 1, TYPE mask = (1ULL << sizeof(TYPE)) - 1ULL)
uint64_t number = 1, TYPE mask = gs_full_mask<TYPE>())
: SCP_LOGGER_NAME()(logger)
, m_path_name(path_name)
, p_number(path_name + ".number", number, "number of elements in this register")
Expand Down Expand Up @@ -356,7 +364,7 @@ class proxy_data : public proxy_data_array<TYPE>

proxy_data(scp::scp_logger_cache& logger, std::string name, std::string path_name, uint64_t offset = 0,
uint64_t number = 1, uint64_t start = 0, uint64_t length = sizeof(TYPE) * 8,
TYPE mask = (1ULL << sizeof(TYPE)) - 1ULL)
TYPE mask = gs_full_mask<TYPE>())
: proxy_data_array<TYPE>(logger, name, path_name, offset, number, mask), SCP_LOGGER_NAME()(logger)
{
static_assert(std::is_unsigned<TYPE>::value, "Register types must be unsigned");
Expand Down Expand Up @@ -386,7 +394,7 @@ class gs_register : public port_fnct, public proxy_data<TYPE>
public:
gs_register() = delete;
gs_register(std::string _name, std::string path = "", uint64_t offset = 0, uint64_t number = 1,
TYPE mask = (1ULL << sizeof(TYPE)) - 1ULL)
TYPE mask = gs_full_mask<TYPE>())
: m_regname(_name)
, m_path(path)
, port_fnct(_name, path)
Expand Down Expand Up @@ -423,7 +431,7 @@ class gs_register : public port_fnct, public proxy_data<TYPE>
void operator-=(TYPE other) { proxy_data<TYPE>::set(proxy_data<TYPE>::get() - other); }
void operator/=(TYPE other)
{
if (other == 0) SCP_FATAL(())("Trying to devide a register by 0!");
if (other == 0) SCP_FATAL(())("Trying to divide a register value by 0!");
proxy_data<TYPE>::set(proxy_data<TYPE>::get() / other);
}
void operator*=(TYPE other) { proxy_data<TYPE>::set(proxy_data<TYPE>::get() * other); }
Expand All @@ -447,12 +455,13 @@ class gs_register : public port_fnct, public proxy_data<TYPE>
uint64_t get_size() const { return proxy_data<TYPE>::p_size.get_value(); }
void set_mask(TYPE mask) // mask is allowed to be set to implement e.g. write-once semantics
{
SCP_TRACE(()) << "Set Mask to 0x" << std::hex << mask;
proxy_data<TYPE>::p_mask = mask;
}
TYPE get_mask() const { return proxy_data<TYPE>::p_mask.get_value(); }
void capture_txn_pre(tlm::tlm_generic_payload& txn) override
{
if ((proxy_data<TYPE>::p_mask.get_value() == ((1ULL << sizeof(TYPE)) - 1ULL)) ||
if ((proxy_data<TYPE>::p_mask.get_value() == gs_full_mask<TYPE>()) ||
(txn.get_command() != tlm::tlm_command::TLM_WRITE_COMMAND)) {
return;
}
Expand All @@ -472,7 +481,7 @@ class gs_register : public port_fnct, public proxy_data<TYPE>
}
void handle_mask_post(tlm::tlm_generic_payload& txn) override
{
if ((proxy_data<TYPE>::p_mask.get_value() == ((1ULL << sizeof(TYPE)) - 1ULL)) ||
if ((proxy_data<TYPE>::p_mask.get_value() == gs_full_mask<TYPE>()) ||
(txn.get_command() != tlm::tlm_command::TLM_WRITE_COMMAND)) {
return;
}
Expand Down
1 change: 1 addition & 0 deletions tests/base-components/gs_register/gs_register-bench.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ class RegisterTestBench : public TestBench
gs::gs_register<uint32_t> CMD0;
gs::gs_field<uint32_t> CMD0_OPCODE;
gs::gs_field<uint32_t> CMD0_PARAM;
gs::gs_register<uint64_t> STATUS64;
uint32_t last_written_value;
uint64_t last_written_idx;
uint32_t last_used_mask;
Expand Down
50 changes: 42 additions & 8 deletions tests/base-components/gs_register/gs_register-tests.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@
#include "gs_register-bench.h"
#include <cci/utils/broker.h>

#define REG_MEM_ADDR 0x0UL
#define REG_MEM_SZ 0x10000UL
#define FIFO0_ADDR 0x100UL
#define FIFO0_LEN 16UL
#define CMD0_ADDR 0x0UL
#define CMD0_LEN 1UL
#define REG_MEM_ADDR 0x0UL
#define REG_MEM_SZ 0x10000UL
#define FIFO0_ADDR 0x100UL
#define FIFO0_LEN 16UL
#define CMD0_ADDR 0x0UL
#define CMD0_LEN 1UL
#define STATUS64_ADDR 0x200UL
#define STATUS64_LEN 1UL

RegisterTestBench::RegisterTestBench(const sc_core::sc_module_name& n)
: TestBench(n)
Expand All @@ -23,9 +25,10 @@ RegisterTestBench::RegisterTestBench(const sc_core::sc_module_name& n)
, CMD0("CMD0", "CMD0", CMD0_ADDR, CMD0_LEN)
, CMD0_OPCODE(CMD0, CMD0.get_regname() + ".OPCODE", 27UL, 5UL)
, CMD0_PARAM(CMD0, CMD0.get_regname() + ".PARAM", 0UL, 27UL)
, STATUS64("STATUS64", "STATUS64", STATUS64_ADDR, STATUS64_LEN)
, last_written_value(0)
, last_written_idx(0)
, last_used_mask((1 << sizeof(uint32_t)) - 1)
, last_used_mask(gs::gs_full_mask<uint32_t>())
, is_array(false)
{
m_initiator.socket.bind(m_reg_router.target_socket);
Expand All @@ -39,14 +42,18 @@ RegisterTestBench::RegisterTestBench(const sc_core::sc_module_name& n)
CMD0.initiator_socket.bind(m_reg_memory.socket);
m_reg_router.initiator_socket.bind(CMD0);
m_reg_router.rename_last(std::string(this->name()) + ".CMD0.target_socket");

STATUS64.initiator_socket.bind(m_reg_memory.socket);
m_reg_router.initiator_socket.bind(STATUS64);
m_reg_router.rename_last(std::string(this->name()) + ".STATUS64.target_socket");
}

void RegisterTestBench::end_of_elaboration()
{
// first post_write registered callback
FIFO0.post_write([&](tlm::tlm_generic_payload& trans, sc_core::sc_time& delay) {
if (!is_array) {
if (last_used_mask == ((1 << sizeof(uint32_t)) - 1)) {
if (last_used_mask == gs::gs_full_mask<uint32_t>()) {
ASSERT_EQ(FIFO0[last_written_idx], last_written_value);
} else {
uint32_t read_value = 0;
Expand Down Expand Up @@ -85,12 +92,19 @@ void RegisterTestBench::end_of_elaboration()
CMD0.pre_read([&](tlm::tlm_generic_payload& trans, sc_core::sc_time& delay) { CMD0 += 1; });

CMD0.post_read([&](tlm::tlm_generic_payload& trans, sc_core::sc_time& delay) { CMD0 *= 2; });

STATUS64.pre_write([&](tlm::tlm_generic_payload& trans, sc_core::sc_time& delay) {
STATUS64.set_mask(0x0ULL); /*Read Only*/
});
}

TEST_BENCH(RegisterTestBench, test_registers)
{
uint32_t write_value = 0UL;
SCP_DEBUG(()) << "****************Testing register array****************" << std::endl;
SCP_DEBUG(()) << "Assert FIFO0 array register default mask is 0xFFFFFFFF";
ASSERT_EQ(FIFO0.get_mask(), 0xFFFFFFFFUL);

SCP_DEBUG(()) << "write 1 value to the register array with default mask";
write_value = 0xABABABABUL;
last_written_idx = 0;
Expand Down Expand Up @@ -170,6 +184,12 @@ TEST_BENCH(RegisterTestBench, test_registers)
ASSERT_EQ(m_initiator.do_b_transport(trans), tlm::TLM_OK_RESPONSE);

SCP_DEBUG(()) << "****************Testing normal register (not register array case)****************" << std::endl;
SCP_DEBUG(()) << "Assert CMD0 register default mask is 0xFFFFFFFF";
ASSERT_EQ(CMD0.get_mask(), 0xFFFFFFFFUL);

SCP_DEBUG(()) << "Assert STATUS64 register default mask is 0xFFFFFFFFFFFFFFFF";
ASSERT_EQ(STATUS64.get_mask(), 0xFFFFFFFFFFFFFFFFULL);

SCP_DEBUG(()) << "write value to the register with default mask";
write_value = 0xABABABABUL;
last_written_value = write_value;
Expand Down Expand Up @@ -233,6 +253,20 @@ TEST_BENCH(RegisterTestBench, test_registers)
ASSERT_EQ(reg_value, 26UL); // because of pre_read CB
reg_value = CMD0;
ASSERT_EQ(reg_value, 52UL); // because of post_read CB

SCP_DEBUG(()) << "test 64 bit reagister access";
uint64_t write_value_64 = 0xABABABABCDCDCDCDULL;
uint64_t mask_64 = gs::gs_full_mask<uint64_t>();
STATUS64.set_mask(mask_64);
STATUS64 = write_value_64;
uint64_t read_value_64 = 0x0ULL;
m_initiator.do_read<uint64_t>(STATUS64_ADDR, read_value_64);
ASSERT_EQ(read_value_64, write_value_64);
uint64_t new_write_value_64 = 0xFF00FF00FF00FF00ULL;
m_initiator.do_write<uint64_t>(STATUS64_ADDR, new_write_value_64);
m_initiator.do_read<uint64_t>(STATUS64_ADDR, read_value_64);
ASSERT_EQ(read_value_64, write_value_64); // 0x0ULL mask will be applied in pre_pread callback, so the value of the
// register shouldn't change at write
}

int sc_main(int argc, char* argv[])
Expand Down

0 comments on commit efb0403

Please sign in to comment.