-
Notifications
You must be signed in to change notification settings - Fork 267
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
Support for Avalon-MM burst transfers #364
Conversation
procedure write_bus(signal net : inout network_t; | ||
constant bus_handle : bus_master_t; | ||
constant address : std_logic_vector; | ||
constant burstsize : positive; |
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.
At least in AXI terminology it would be called burst length and not burst size.
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.
In Avalon it is burstcount. The another option is simple "burst". However, for me, "burstlength" sounds good.
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 think we should either keep Avalon naming or use our current naming conventions all the way with underscore separators. burstlength would be something in between.
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.
Queues are very explicit about data order but not explicit about what data types that it accepts. I'm on my mobile and haven't looked at the code carefully but I think an integer queue is assumed. A user may find it more natural to represent the data as a vector. Or create the burst out of order. Maybe we should list some pros cons when comparing with integer_array_t.
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.
So lets stay at 'burst_length'?
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.
Currently std_logic_vector queue is assumed. Is it possible to restrict queue to accept only single type? What do you mean by out of order burst?
Using some array type is another option. I think the little disadvantage is the user would have to allocate array type with some fixed size. Does std logic vector array type exist in vunit?
Yet another option is to give up using special types for input burst data and split it in command and payload phases:
write_burst(address, burst_length);
write_burst(data0);
write_burst(data1);
...
However this kind of interface is a little bit harder to implement internally. Also need to check if user doest not violate burst transfer rules, for example by reading bus before writing last data word to the bus.
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 think the queue is fine for now. There is nothing stopping other data-types from being supported in the future if the message format consists of the data by value.
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.
Ok so it seems no more activity here. I will change to burst_length.
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.
Ok burst_length is a good name
constant bus_handle : bus_master_t; | ||
constant address : std_logic_vector; | ||
constant burstsize : positive; | ||
constant burstdata : queue_t); |
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 comment is valid for all functions added. The ownership of the queues should be documented in the API. For example is ownership of the queue transferred into the function?, does the queue need to be deallocated afterwards?, is the queue consumed by the function?
This is important for the user to know.
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.
Yes, it need to be documented. I assumed that queue is allocated in tb main which is responsible for its deallocation. In that case single queue could be shared by multiple reads/writes in different verification components instantiations.
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.
Is it enough to document queue API in vhdl package code or also in general vunit doc?
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 is enough to document it in a comment above the function in the package header.
Note: For many VUnit packages we have separated the package header from the package body to include the package header in verbatim on the webpage as a simple form of API documentation lacking any better autodoc for VHDL like there is for Python.
Failed Travis build seems not related to this pull request |
@kraigher do you have idea what happened to travis now? |
@slaweksiluk Looks like an environment error. I restarted the build manually. |
@kraigher I have not idea how to fix it
|
It is an environment problem and not your fault. We are working on it. |
Is this PR still being worked on? @kraigher @slaweksiluk |
The PR needs to be rebased and the CI checks must pass. Then it is probably ready to merge after a review. |
Still time-out on travis |
Are the new test you added long running tests? Travis has a timeout for builds. Maybe we need to start splitting the running of all run.py files into several builds. |
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 am almost ready to merge this I just have a few comments regarding the naming.
@@ -96,6 +98,34 @@ package body bus_master_pkg is | |||
write_bus(net, bus_handle, to_address(bus_handle, address), data, byte_enable); | |||
end; | |||
|
|||
procedure write_bus(signal net : inout network_t; |
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 think the function should be called burst_write_bus to disambiguate it from the normal write_bus.
constant bus_handle : bus_master_t; | ||
constant address : std_logic_vector; | ||
constant burst_length : positive; | ||
constant burstdata : queue_t) is |
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.
Maybe just data is enough for this name if the procedure is called burst_write_bus
@@ -158,6 +188,30 @@ package body bus_master_pkg is | |||
read_bus(net, bus_handle, to_address(bus_handle, address), reference); | |||
end; | |||
|
|||
procedure read_bus(signal net : inout network_t; |
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.
burst_read_bus
@@ -191,6 +264,26 @@ package body bus_master_pkg is | |||
read_bus(net, bus_handle, to_address(bus_handle, address), data); | |||
end; | |||
|
|||
procedure read_bus(signal net : inout network_t; |
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.
burst_read_bus
await_read_bus_reply(net, bus_handle, burstdata, reference); | ||
end procedure; | ||
|
||
procedure read_bus(signal net : inout network_t; |
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.
burst_read_bus
send(net, bus_handle.p_actor, request_msg); | ||
end procedure; | ||
|
||
procedure read_bus(signal net : inout network_t; |
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.
burst_read_bus
-- deallocation of burstdata queue. Procedure cunsumes burst_length data words | ||
-- from burstdata queue. If burstdata queue has less data words, all data | ||
-- words are consumed and pop from empty queue error is raised. | ||
procedure write_bus(signal net : inout network_t; |
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.
burst_write_bus, also maybe burst_data can just be named data.
@slaweksiluk I just pushed a commit to master which moves the verification components to a separate build. Hopefully it will make it more robust. |
I think this is ready to merge except for the names I commented on, what do you think about the names @slaweksiluk. Is it not more clear to have different names for burst read and normal read or do you have a different opinion? |
I had doubts about it and decided to use the same names as in non-burst transfers. It is impossible to confuse burst and non burst transfers as they have different interfaces. On the other side having the same names has no clear advantages. Its up to you, I can implement burst_read/write_bus naming if needed. |
I prefer having different names to make the behavioral difference more obvious when reading the code. |
@kraigher I've updated names |
Master
For support for burst transfers current bus_master had to be extended and some way of passing multiple data words had to be chosen. I decided to use VUnit queues as it seems the most natural for me. Please comment if somebody see the better options. Currently data words from queue are pushed to bus burst msg, however it is also possible to push queue reference instead of single data chunks.
In bursts transactions byteenable signals is always high - probably need to extend it.
Slave
Avalon Slave was rewritten - write and read handling is separated. New implementation does not prevent simultaneous write and read .The another event which should be forbidden is mixed write/read bursts. In general bus monitor should be added to capture and report such a invalid transactions.