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

avalon_slave.vhd: added support for byte enable #872

Merged
merged 3 commits into from
Dec 16, 2022

Conversation

fst-21
Copy link
Contributor

@fst-21 fst-21 commented Nov 7, 2022

Hello,

first of all thank you for the great and helpful work.

I have extended the avalon_slave.vhd to include the byte enable when writing the data.

With best regards,
fst-21

@LarsAsplund
Copy link
Collaborator

The only thing I miss is a test for byte enable in tb_avalon_slave.vhd. Currently all bits are always set:

signal byteenable : std_logic_vector(tb_cfg.data_width/8 -1 downto 0) := (others => '1');

@fst-21
Copy link
Contributor Author

fst-21 commented Nov 10, 2022

I have added a test and hope this is satisfactory.

-- Burst write in progress
if pending_writes > 1 then
addr := addr + byteenable'length;
pending_writes := pending_writes -1;
write_word(avalon_slave.p_memory, addr, writedata);
for idx in 0 to word_i'length/8-1 loop
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you move line 60-64 after the if statement you get rid of the code duplication in lines 69-73

@LarsAsplund
Copy link
Collaborator

Thanks for the added test. Linting failed with a trailing whitespace on line 139 in tb_avalon_slave.vhd. One final request is to remove the code duplication in avalon_slave.vhd. I made a new inline comment on that.

@fst-21
Copy link
Contributor Author

fst-21 commented Nov 14, 2022

Thanks for the tips. I have processed both points

@fst-21
Copy link
Contributor Author

fst-21 commented Dec 14, 2022

Is there anything left for me to do for a merge?

The failed test seems to be due to problems with the tools...

@LarsAsplund LarsAsplund merged commit f1eb251 into VUnit:master Dec 16, 2022
@LarsAsplund
Copy link
Collaborator

No, I think that was a glitch. Couldn't see the re-run button so I simply merged it. We'll see what happens.

@LarsAsplund
Copy link
Collaborator

It worked. Thanks for your contribution.

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

Successfully merging this pull request may close these issues.

3 participants