Skip to content
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

Add axi master #1017

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

Add axi master #1017

wants to merge 27 commits into from

Conversation

DavidMartinPhios
Copy link

Added axi master with equally powerful implementation as axi lite master has

@DavidMartinPhios DavidMartinPhios changed the title AXI master Add axi master May 13, 2024
@DavidMartinPhios DavidMartinPhios marked this pull request as ready for review May 15, 2024 12:33
@LarsAsplund
Copy link
Collaborator

Thanks for getting this started. I'm aware of several implementations over the years but non of them made it to open source. It so happens that I have my eyes on yet another implementation in progress that may or not make it to the public. Even if that doesn't become open source, I can keep track of what features it requires such that the VC released by VUnit can meet those as well.

I will get back when I have more info.

@DavidMartinPhios
Copy link
Author

Thanks for getting this started. I'm aware of several implementations over the years but non of them made it to open source. It so happens that I have my eyes on yet another implementation in progress that may or not make it to the public. Even if that doesn't become open source, I can keep track of what features it requires such that the VC released by VUnit can meet those as well.

I will get back when I have more info.

As you might figured out already this implementation is not yet finished. I am still working on it. Basically I am now adding the functionallity to each single signal. At the moment I have deticated reserved time to implement this specific VC. It would be nice to know if the work will get obsolete some day or be replaced by another implementation. Otherwise I will keep going with the implementation. Hopefully with your early review / support.

@LarsAsplund
Do you think that it is necessary to move the signals id, last, etc. to the bus_master record or is it ok to implement them around on a higher level as it is right now?

@LarsAsplund
Copy link
Collaborator

Since you've started and the other implementation may not make it to open source, I think we should continue on what you have. What I hope is to align on the required feature set such that you both can work on the open implementation in the end. I will come back and review a bit later,

@DavidMartinPhios
Copy link
Author

Since you've started and the other implementation may not make it to open source, I think we should continue on what you have. What I hope is to align on the required feature set such that you both can work on the open implementation in the end. I will come back and review a bit later,

Thank you for the answer! I am currently working on the read burst behaviour. It is not finished yet but I think you could get an idea where the journey should go.

@DavidMartinPhios
Copy link
Author

@LarsAsplund
Do you already had the change to review the axi master? At the moment a very basic version is implemented but it should provide basic read and write possibilities for burst and none burst.

@LarsAsplund
Copy link
Collaborator

I have yet to review it. The other implementation I'm keeping my eyes on is settling now so I think I will be in a position to review shortly and then make sure that the one you have is "feature compatible". With that I don't mean that it has to have all features but rather that the API is such that it can evolve and eventually cover all the things the other project is requiring. That way there is a chance that the two efforts can be merged in the end.

@DavidMartinPhios
Copy link
Author

I have yet to review it. The other implementation I'm keeping my eyes on is settling now so I think I will be in a position to review shortly and then make sure that the one you have is "feature compatible". With that I don't mean that it has to have all features but rather that the API is such that it can evolve and eventually cover all the things the other project is requiring. That way there is a chance that the two efforts can be merged in the end.

@LarsAsplund
I will wait for your review before I continue further implementations. Just to be sure that it is not completely running in the wrong direction.

@LarsAsplund
Copy link
Collaborator

So let's review this in a number of iterations, starting with the public interface. Since the first implementation won't implement all of AXI, it is important that we have a public API that can be extended in the future without breaking backward compatibility. There are two references to consider:

  1. The AXI standard (AXI5 is the latest)
  2. The unreleased VUnit VC standard. This is a work that started a few years ago but was paused since modifying existing VCs will break backward compatibility and has to be done with a major release of VUnit. Since the next release is planned to be a major one, v5.0, this is the time to make the modification. For your IP, we better do it according to this standard right from the beginning. This standard does not affect the details of your implementation but focuses on good VUnit practices.

The VUnit VC standard comes with a support package that you will need but I need to modify it slightly to take the identity package into account. I will start with that and push it to a branch that you can rebase on. To get a feel for the new VC standard you can read this but be aware that there will be some changes. Regardless of those changes you can start by looking at rule 13 (and rule 11):


Rule 13
A VC shall only have one generic.

Rationale: Representing a VC with a single object makes it easier to handle in code. Since all fields of the handle are private future updates have less risk of breaking backward compatibility.


Rather than a bus_master handle and a number of additional generics you will have to create another handle that contains a bus_master and the other parameters.

With respect to the AXI standard, I also see two things that you can start with:

  1. AXI5 has replaced the master/slave terminology with manager/subordinate
  2. To comply with the subset of the standard you've implemented, you would need the areset_n input. See chapter A3.1.2 of the standard.

Let's start with that and create a new iteration when I pushed that VC standard support package.

@DavidMartinPhios
Copy link
Author

DavidMartinPhios commented Jul 18, 2024

@LarsAsplund I already added the axi_master_t single generic for the axi master VC. Did you already have the time to prepare the branch for the new VC standard? Then I would continue with the rebase.

@DavidMartinPhios
Copy link
Author

@LarsAsplund Hi, my branch is now synced with the latest master branch. What are the next steps? Is there something what I can do to keep up the progress?

@LarsAsplund
Copy link
Collaborator

@DavidMartinPhios The next step I want to do with the respect to the VC standard is something I will need to do on my own. It is second on my priority list and can be started once I've got some progress with improved support for vopt in Modelsim/Questa

@DavidMartinPhios
Copy link
Author

@LarsAsplund Ok. The reset functionality is also included right now. Let me know when I can continue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants