Add APB examples and fix APB pin level adapters #76
Conversation
1. Handle strobe signals correctly 2. Free the payload after usage to avoid deadlock 3. Remove redundant code
|
@JingJerYen THX for the PR. I need to review the code in more detail as these are significant changes. Did you run it with a verilated design? Or coupled with a Verilog simulator? |
| PENABLE_o.write(false); | ||
| PSELx_o.write(false); | ||
| if(trans->has_mm()) | ||
| trans->release(); |
There was a problem hiding this comment.
This line fixes the deadlock when running examples/apb
|
For this PR, I didn't run it with any RTL-level design. I just added the examples and found they didn't work, so I tried to fix the pin-level adapters. And the waveform was produced by examples/apb. I can add some comments on the code to explain some parts of the commit |
| } | ||
|
|
||
| template <unsigned DATA_WIDTH, unsigned ADDR_WIDTH> inline void initiator<DATA_WIDTH, ADDR_WIDTH>::bus_task() { | ||
| auto& hready = PREADY_i.read(); |
There was a problem hiding this comment.
remove the unused variable
| auto upper = addr_offset + trans->get_data_length(); | ||
| if(!PSTRB_o.get_interface() && addr_offset && upper != (DATA_WIDTH / 8)) { | ||
| SCCERR(SCMOD) << "Narrow accesses are not supported as there is no PSTRB signal! Skipping " << *trans; | ||
| if(!PSTRB_o.get_interface() && (addr_offset || upper != (DATA_WIDTH / 8))) { |
There was a problem hiding this comment.
As I know, both start address and size should align with bus data width before APB4 where strobe signal hasn't been introduced, so I changed this line. Correct me if i'm wrong.
| tsckt->nb_transport_bw(*trans, phase, delay); | ||
| } else { | ||
| SCCDEBUG(SCMOD) << "Recv beg req for read to addr 0x" << std::hex << trans->get_address() << ", starting APB setup phase, "; | ||
| auto bytes_exp = scc::ilog2(trans->get_data_length()); |
There was a problem hiding this comment.
These are dead code, so i removed them
| data_t data{0}; | ||
| strb_t strb{0}; | ||
| // Handle TLM byte enables if present | ||
| if(trans->get_byte_enable_ptr() && trans->get_byte_enable_length() > 0) { |
There was a problem hiding this comment.
add this part to handle TLM payload with byte_enable array , this is not handled in the original code
| if(PSTRB_o.get_interface()) | ||
| PSTRB_o.write(strb); | ||
| } else if(PSTRB_o.get_interface()) { | ||
| // From spec : For read transfers the bus master must drive all bits of PSTRB LOW |
| tsckt->nb_transport_bw(*trans, phase, delay); | ||
| SCCDEBUG(SCMOD) << "Starting APB access phase"; | ||
| PENABLE_o.write(true); | ||
| wait(1_ps); |
| } else | ||
| for(size_t j = 0; j < DATA_WIDTH / 8; ++j) | ||
| *(trans->get_data_ptr() + j) = data(8 * j + 7, 8 * j).to_uint(); | ||
| wait(sc_core::SC_ZERO_TIME); |
There was a problem hiding this comment.
The indentation makes it look to be significant changes, but actually only few lines are changed. It waits for PSEL signals here. And i think wait( zero_time) is more elegant than wait(1_ps) for the log message ?
| trans->set_read(); | ||
| } | ||
| sc_core::sc_time delay; | ||
| tlm::tlm_phase phase{tlm::BEGIN_REQ}; |
There was a problem hiding this comment.
Just the indentation changed.
I've verified this patch using my company's Verilated CPU IP. The waveform is identical and correct before and after applying the patch, but the simulation speed is much slower before the patch when collecting waveform. I guess this is caused by the wait(1_ps) statement?
|
2 participants
I initially added an example for APB, but later found that the test didn’t work correctly due to several bugs. So I rewrote it to fix those issues.
Fix details:
Strobe signals were not handled properly — only consecutive 0xFF values were considered, but the APB spec does not guarantee they are contiguous.
A deadlock occurred due to a missing generic payload free.
Removed unused variables, redundant code, and replaced wait(1_ps) with wait(SC_ZERO_TIME).
The waveform below shows the correct behavior after applying this patch.