-
-
Notifications
You must be signed in to change notification settings - Fork 310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[regif] feature upgrade refactoring #1409
Conversation
1. remove BusIfVistor mode instead directly access for simplify 2. add RegInstGrp layer 3. refactor BusIf Document, but keep old api 4. add usefull Factory method(Interrupt, SCR etc..) 5. localbus
I know this is still a draft, but a few thoughts:
possibly more to follow, have to look at it some more |
|
…s_rdata/bus_rderr compare reg_rdata/bus_rderr
FIFO is really a special case, when writing to a FIFO it should be tested if it is writable first. So the data and status of FIFO might need to be added separately, which is a little bit different from the existing regif interface. On the other hand, before read, the host should check if there is data in the FIFO. |
yes the FIFO port cannot strictly guarantee no data loss,
right, If it is an effective read operation, the software does need to first determine whether there is data. to be honest, |
Ahhh,We use a lot while designing an instrument. |
i got it, so now If we sensitive to packet loss, we may need some peripheral auxiliary circuits or handshake to ensure data. Since that's the case, it's necessary to add this feature , but the specific data caching correctness needs to be guaranteed by the user themselves, is it OK? |
Yes, I think they should guarantee the correctness by software. We can only provide the option. |
At present, I think it ready for merge The update of the document is still in progress it will completed within this week. do you guys have any comments @andreasWallner @Readon @mrberman87 @Dolu1990 |
For me, you can merge you feel it. |
@jijingg |
hi, Let me merge first. If there are any issue need improvement, we will continue to initiate the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long time, but since this is nearly a rewrite I had to copy code to an external editor to have useful diffs - a PR of this size is not reviewable.
|
||
case class MemBusConfig(aw: Int, dw: Int) | ||
|
||
case class MemBus(c: MemBusConfig) extends Interface with IMasterSlave { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MemBus is a near duplicate of the BRAM bus (slight difference in WE, which would actually be a reasonable future feature request to support) and the AsyncMemoryBus (which can stall) - I think we should extend one of those with a parameter instead of adding yet another new Bus that is not reasonably different.
val sw = dw >> 3 | ||
} | ||
|
||
case class MinBus(c: MinBusConfig) extends Interface with IMasterSlave { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope you'll describe the meaning (and timing) of these signals somewhere - its really hard to know what some of them are supposed to be (e.g. the difference between rvld and rdy, what prot
does).
Apart from that this is a slight extension of AsyncMemoryBus and would maybe be better as some parameters there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prot
is widely used in the AMBA protocol, integrating the habits of the AMBA protocol to represent secure bits. and yes , adding some comments would be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that prot
is used by AMBA - but nobody can know that you are using the same names here compared to AMBA. This bus does in no way show a connection to the AMBA protocols. Thats why I think it needs at least a description of the intent of the signals + their timing.
import spinal.core._ | ||
import spinal.core.sim._ | ||
class MemVIP{ | ||
private val mem = scala.collection.mutable.HashMap[Long, BigInt]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would make sense if this would reuse SparseMemory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a Mem VIP (Verification IP), it must be Sparse, and here we emphasize that VIP , so did the MemVIP be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry - I don't understand what you are saying.
bus.ce #= true | ||
bus.wr #= false | ||
bus.addr #= address | ||
bus.wdat #= 1234 //.randomize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should use randomize like in the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done 8981f31
} | ||
} | ||
|
||
def hang(bus: MinBus, delay: Int = 10)(cd: ClockDomain): MemVIP = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does hang
mean - the only thing that comes to my mind is that it gets stuck which doesn't appear to be the case - looking at this and the one above shouldn't these just be apply
methods?
val wstrb: Bits = withStrb generate (Bits(strbWidth bit)) | ||
val wmask: Bits = withStrb generate (Bits(busDataWidth bit)) | ||
val wmaskn: Bits = withStrb generate (Bits(busDataWidth bit)) | ||
initStrbMasks() | ||
override def getModuleName = moduleName.name | ||
|
||
val readError = Bool() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these were one of the changes that I was referring to when I said that the PR can't be backwards compatible: since you are changing the name of a public field every code referring to it is broken after this
|
||
val readData: Bits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was one of the changes I was referring to when I said this PR is not backwards compatible. This change breaks ever custom bus implementation a user of the library has
XD , yes it's realy a significant refactoring, stemming from some feedback on our real project, i think it's necessary to do this upgrade. thank you for taking the time to conduct such a careful review and providing multiple valuable feedback. I will make some revisions to the PR again |
This update is based on practical feedback from real projects, and the changes are quite significant. In addition to adding some important features, many factory functions have been added, and common register file reuse scenarios have been solved. suggestions from software usage have been adopted to optimize.
.h
file.Despite such significant refactoring modifications, backward compatibility should be maintained in principle, and it will not affect old code.
Checklist