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

AXI DW Conversion #43

Merged
merged 40 commits into from
Mar 24, 2020
Merged

AXI DW Conversion #43

merged 40 commits into from
Mar 24, 2020

Conversation

suehtamacv
Copy link
Contributor

This PR proposes two modules, axi_dw_upsizer and axi_dw_downsizer, for data with conversion, together with a third axi_dw_converter module that instantiates one of the previous depending on the relationship between AxiMstDataWidth and AxiSlvDataWidth.

Given the growing divergence between the axi_dwc branch and master, instead of rebasing, I considered that it was easier to clean up the work previously hosted there, and create a new branch altogether.

Copy link
Contributor

@bluewww bluewww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all very nice work!

I added some minor nitpicks.
The thing about elaboration time asserts maybe needs some more discussing. I know Manuel used them quite extensively.

CHANGELOG.md Outdated Show resolved Hide resolved
src/axi_dw_converter.sv Outdated Show resolved Hide resolved
src/axi_dw_converter.sv Show resolved Hide resolved
src/axi_dw_downsizer.sv Show resolved Hide resolved
src/axi_dw_downsizer.sv Outdated Show resolved Hide resolved
src/axi_dw_upsizer.sv Outdated Show resolved Hide resolved
src/axi_dw_upsizer.sv Outdated Show resolved Hide resolved
test/tb_axi_dw_downsizer.do Outdated Show resolved Hide resolved
test/tb_axi_dw_upsizer.do Outdated Show resolved Hide resolved
test/tb_axi_dw_upsizer.sv Show resolved Hide resolved
@WRoenninger
Copy link
Collaborator

I'd like to point out that since v0.8.0 the default of describing the AXI ports has changed to use structs as defined in include/axi/typedef.svh instead of interfaces.

The reason is that not all tools are capable of handling interfaces and that interfaces can lead to unintended behaviour when there is a mismatch in the interface definitions.
An example how the top level module should look like is: src/axi_cut.sv

I would recommend adding type parameters in axi_dw_converter and propagate them down the hierarchy over subsequent parametrization. The interface should be in its own *_intf module in the same file as axi_dw_converter. It should define the required types over the typedef macros and the module can then be used for the testbench.

@suehtamacv
Copy link
Contributor Author

Thanks, everyone for the review. I pushed a few commits handling the issues that were raised.

Copy link
Contributor

@andreaskurth andreaskurth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the latest updates, @suehtamacv!

Just to be sure, this represents the latest code from the 'dwc' branch, right? Are there any limitations that are not usual in AXI?

README.md Outdated Show resolved Hide resolved
src/axi_dw_converter.sv Outdated Show resolved Hide resolved
src/axi_dw_converter.sv Outdated Show resolved Hide resolved
src/axi_dw_downsizer.sv Outdated Show resolved Hide resolved
src/axi_dw_downsizer.sv Outdated Show resolved Hide resolved
@andreaskurth andreaskurth mentioned this pull request Feb 11, 2020
Closed
2 tasks
@suehtamacv
Copy link
Contributor Author

Yes, @accuminium, these changes correspond to the latest code from the dwc branch.

The DW converters do not serialize/pack WRAP bursts. These run slowly, but the converter should be compatible with them. Improving their performance should be easy enough. However, the upsizer does not support FIXED bursts, since these decompose into a myriad of states to handle corner cases in the data width conversion. Should I throw a $warning() on these, for the time being?

This version also includes the commits you did to support atomics transactions. Are they ready to be merged?

@andreaskurth
Copy link
Contributor

andreaskurth commented Feb 12, 2020

The DW converters do not serialize/pack WRAP bursts. These run slowly, but the converter should be compatible with them. Improving their performance should be easy enough.

Okay, I agree to leave it at this for the moment. However, please add a short documentation header to the _converter module and describe this limitation there.

However, the upsizer does not support FIXED bursts, since these decompose into a myriad of states to handle corner cases in the data width conversion. Should I throw a $warning() on these, for the time being?

What is the behavior of the upsizer upon a fixed burst? Raising an $error() seems more appropriate to me than a $warning(). Am I correctly assuming that replying with a slave error in the upsizer would be too complex?

This version also includes the commits you did to support atomics transactions. Are they ready to be merged?

Yes.

@suehtamacv suehtamacv force-pushed the axi_dwc_publish branch 2 times, most recently from b45247e to 1a1e412 Compare February 14, 2020 14:12
src/axi_dw_downsizer.sv Outdated Show resolved Hide resolved
@bluewww
Copy link
Contributor

bluewww commented Feb 14, 2020

Needs a rebase

@bluewww
Copy link
Contributor

bluewww commented Feb 18, 2020

Blocking issue #57

@andreaskurth
Copy link
Contributor

Thanks @suehtamacv, this looks very good now!

Now that the generalized error slave is available on master (#55), could you please rebase on master and use that error slave?

How about issue #57, has that been addressed?

@suehtamacv
Copy link
Contributor Author

suehtamacv commented Feb 28, 2020

Thanks @suehtamacv, this looks very good now!

Now that the generalized error slave is available on master (#55), could you please rebase on master and use that error slave?

Done.

How about issue #57, has that been addressed?

Yes, via a spill register, although further restructuring of the DWC might desirable to break the dependency between the channels without the extra latency.

@andreaskurth
Copy link
Contributor

Please implement #63 and let's get this released 🙂

suehtamacv and others added 23 commits March 23, 2020 18:02
Ensure that the AxiMst/SlvDataWidth parameters describe properties
of the ports of the modules.
@andreaskurth
Copy link
Contributor

andreaskurth commented Mar 24, 2020

7ad01b9 removes the ID queue and resolves which downsizer is handling each read request manually, by computing whether the rid matches with the arid of each downsizer. This gets rid of the bubble on the R stream, and there are no more stability violations of this channel.

Great, thanks!

I am waiting for benches to complete so this can be merged.

@andreaskurth andreaskurth merged commit d712435 into master Mar 24, 2020
@andreaskurth andreaskurth deleted the axi_dwc_publish branch April 21, 2020 13:56
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.

6 participants