Fix strb of Axi4Master (spinal.lib.bus.amba4.axi.sim) #1692
Conversation
Nik-Sch
force-pushed
the
fix-axi4-master
branch
from
b4e3c2b to
5df9a22
Compare
|
Hi, (~((BigInt(1) << padBack) - 1)) & ((BigInt(1) << bytePerBeat) - 1) Does it work for burst where size is smaller than the whole bus width ? |
|
Yes, you are correct, there were some weird bugs... |
Nik-Sch
force-pushed
the
fix-axi4-master
branch
2 times, most recently
from
1cd5f09 to
6e32289
Compare
lib/src/main/scala/spinal/lib/bus/amba4/axi/sim/Axi4Master.scala
Outdated
Show resolved
Hide resolved
Nik-Sch
force-pushed
the
fix-axi4-master
branch
2 times, most recently
from
3586a0f to
e5afb9f
Compare
| case 0 => fullStrb << padFront | ||
| case `len` => fullStrb >> padBack | ||
| case _ => fullStrb | ||
| }) & fullStrb |
There was a problem hiding this comment.
Overall i think there is still two cases which aren't handled (and were not before the pr either ?)
- Single beat burst with address not being word aligned.
- burst with size smaller than word size
@KireinaHoro
Can you double check ?
There was a problem hiding this comment.
I added a test for that and it was already working in my branch. This was handled by padData in line 284.
Did I understand your points correctly?
There was a problem hiding this comment.
Looks like narrow bursts will be handled in the proposed version (since you use fullStrb).
Not so sure about the unaligned address case -- address is rounded at the beginning through padData... @Dolu1990 can you elaborate?
Nik-Sch
force-pushed
the
fix-axi4-master
branch
from
4849de6 to
2e01821
Compare
| if (busConfig.useStrb) w.strb #= strb | ||
| if (busConfig.useLast) w.last #= beat == len | ||
| log("W", f"data ${data.bytesToHex} strb $strb%#x last ${beat == len}") | ||
| log("W", f"data ${data.reverse.bytesToHex} strb $strb%#x last ${beat == len}") |
There was a problem hiding this comment.
If you're reversing the print for W, please also do the same for R... otherwise they won't match and will cause confusions
| } else { | ||
| len | ||
| } | ||
| writeSingle(addr, slice, id, burst, thisLen, size)(handleTransaction(addr, transactionId, remaining)) |
There was a problem hiding this comment.
Not sure if silently down-capping AxLEN is a good idea, with the current semantics of len. I see one of two ways:
- change
lento something likemaxLen, so that it's used to limit the max burst size, or - assert false here to complain that the user specified too big of
AxLEN
| @@ -278,7 +282,8 @@ case class Axi4Master(axi: Axi4, clockDomain: ClockDomain, name: String = "unnam | |||
| val bytePerBus = 1 << log2Up(busConfig.dataWidth / 8) | |||
|
|
|||
| val (roundedAddress, padFront, padBack, paddedData) = padData(address, data) | |||
| val realLen = data.length | |||
| assert(data.length <= (len + 1) * bytePerBeat, "data is too long and cannot be sent in one transaction") | |||
| assert(data.length >= len * bytePerBeat, f"data is too short (${data.length}) for len=$len (would result in erroneous zero transfers)") | |||
There was a problem hiding this comment.
This assertion is not true: you can have holes in the transaction, e.g. issue a 16B write but use STRB to mask it down.
| case 0 => fullStrb << padFront | ||
| case `len` => fullStrb >> padBack | ||
| case _ => fullStrb | ||
| }) & fullStrb |
There was a problem hiding this comment.
Looks like narrow bursts will be handled in the proposed version (since you use fullStrb).
Not so sure about the unaligned address case -- address is rounded at the beginning through padData... @Dolu1990 can you elaborate?
Context, Motivation & Description
The
strbfor theAxi4Master(sim) was wrong for aligned transfers withlen>0. I fixed this and added a simple testbench that verifies theAxi4Masteragainst theAxiMemorySim.Impact on code generation
None
Checklist
API changes are or will be documented:using Scaladoc comments:/** */?on RTD?thanks to a new tracking issue on RTD?