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

treewide: Add Ethernet peripheral #104

Draft
wants to merge 51 commits into
base: main
Choose a base branch
from
Draft

treewide: Add Ethernet peripheral #104

wants to merge 51 commits into from

Conversation

chaoqun-liang
Copy link
Contributor

@chaoqun-liang chaoqun-liang commented Feb 22, 2024

@thommythomaso @alex96295

  • Dependencies update (axi-rt, idma)
  • idma regs update
  • pulp-ethernet integration

@chaoqun-liang chaoqun-liang changed the title idma and rt dependency update, minor fixes ethernet integration, idma and rt dependency update, minor fixes Feb 22, 2024
@alex96295 alex96295 changed the title ethernet integration, idma and rt dependency update, minor fixes treewide: Add Ethernet peripheral Feb 23, 2024
@alex96295 alex96295 self-requested a review February 23, 2024 07:56
Bender.lock Outdated
@@ -34,6 +34,13 @@ packages:
- common_verification
- register_interface
- tech_cells_generic
axi_mem_if:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is added because the dependency is in pulp-ethernet, please remove it as from my review (https://github.com/pulp-platform/pulp-ethernet/pull/3/files#r1500435712), it should not be needed.

SpiHost : 1,
Gpio : 1,
Dma : 1,
Dma : 0,
Copy link
Collaborator

@alex96295 alex96295 Feb 23, 2024

Choose a reason for hiding this comment

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

I know this is WIP, but make sure you test correct working of the ethernet integration when the system DMA is enabled. Just a reminder :)

output logic eth_txck_o,
output logic [3:0] eth_txd_o,
output logic eth_txctl_o,
output logic eth_rstn_o,
Copy link
Collaborator

Choose a reason for hiding this comment

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

As the lint action on github suggests, there are some trailing spaces. Please remove trailing spaces before pushing

.reg_rsp_o ( reg_out_rsp[RegOut.ethernet] ) // req from cheshire def, but inside ethernet, it awaits for the type
);

end else begin : gen_no_ethernet
Copy link
Collaborator

@alex96295 alex96295 Feb 23, 2024

Choose a reason for hiding this comment

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

Since the port is incoming into the crossbar, req is an input and resp is an output. Tie input to '0 and leave outputs floating

@@ -0,0 +1,72 @@
#include <stdio.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing license + autorship

@@ -12,7 +12,7 @@ package tb_cheshire_pkg;
// A dedicated RT config
function automatic cheshire_cfg_t gen_cheshire_rt_cfg();
cheshire_cfg_t ret = DefaultCfg;
ret.AxiRt = 1;
ret.AxiRt = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should work when AxiRt is enabled, same reminder as for the system DMA :)

@@ -12,7 +12,7 @@ set TESTBENCH tb_cheshire_soc
# Set voptargs only if not already set to make overridable.
# Default on fast simulation flags.
if {![info exists VOPTARGS]} {
set VOPTARGS "-O5 +acc=p+tb_cheshire_soc. +noacc=p+cheshire_soc. +acc=r+stream_xbar"
set VOPTARGS "-O5 +acc=p+tb_cheshire_soc. +acc=p+cheshire_soc. +acc=r+stream_xbar"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do not upstream these changes, keep them local or set VOPTARGS when locally running the simulation :) let the default as is now

.reg_req_t ( reg_req_t ),
.reg_rsp_t ( reg_rsp_t )
) i_tx_eth_idma_wrap (
.clk_i,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion: Update the ethernet here with the version with exposed clocks; this is important so that we can handle clock generation in Cheshire

@thommythomaso do you think we should handle clock generation for ethernet in cheshire, or expose these clocks also in cheshire and let the end user provide the correct clock? In the latter case, we can generate them in the cheshire testbench as for cheshire's main clock

Aquaticfuller pushed a commit that referenced this pull request Jul 16, 2024
@paulsc96 paulsc96 added astral and removed astral labels Sep 11, 2024
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.

3 participants