Minres / SystemC-Components

A SystemC productivity library: https://minres.github.io/SystemC-Components/
https://www.minres.com/#opensource
Apache License 2.0
86 stars 24 forks source link

ACE target ar_t size #50

Closed mslijepc closed 3 days ago

mslijepc commented 10 months ago

This size is not always correct: https://github.com/Minres/SystemC-Components/blob/ed4a529680204b25797e35ab7c9875b1d3ada50d/src/bus_interfaces/axi/pin/ace_target.h#L368

According to spec (Page. D3-187 IHI0022H_c_amba_axi_protocol_spec.pdf )

The following transactions have a single read data channel transfer:

- CleanUnique
- MakeUnique
- CleanShared
- CleanInvalid
- MakeInvalid
- Barrier
- DVM

So in this case, first ar_snoop should be read, and then set the correct size, right?

eyck commented 10 months ago

Page D3-183 states

The following transactions must use AxLEN to indicate the correct cache line size, even though these transactions
do not transfer data:
• CleanUnique
• MakeUnique
• CleanShared
• CleanInvalid
• MakeInvalid

So ARSIZE and ARLEN shall indicate the cacheline size (page D3-182).

The target as it stand does not act correctly as it should respond only with a single rdata beat. This is because the actual target is there to test the ACE master. As a workaround the operations callback can adjust the ARLEN and set it to zero so that the target FSM only handles a singel beat.

mslijepc commented 10 months ago

Yes,it's just a matter of where should be set to 0, in pe or target

As I workaround I will put it to ace_target_pe::nb_transport_fw

auto* exta = trans.get_extension<ace_extension>();
if (exta != nullptr) {
  if (axi::is_dataless(exta)) {
    exta->set_length(0U);
    exta->set_size(0U);
  }
}
hliu71 commented 10 months ago

in my opinion the extension length and size of transaction should be set correctly by the driver or the interconnect. the ace_target pin level adapter should only receive these signals, convert them into value of extension and forward to the next target socket.

eyck commented 10 months ago

Well as a workaround the modification of the ace extension is ok. I duscussed this in person with @mslijepc at the SystemC evolution day: we may implement a full and correct ACE target Q2 next year...