-
Notifications
You must be signed in to change notification settings - Fork 15
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
Feature/rmii 100M rt #70
Conversation
…rmii_100m_rt * commit '02343159640e8d46ba929d86c8c37cae0e9c45ac': Update test_support hash Use Xsi.get_xsi_tick_freq_hz() in timeout calculation Update tests/helpers.cmake Use custom test_support branch. Remove hardcoded xsi tick freq from tests Hardcode seed to 1 in test_avb_traffic.py. Update changelog and copyright Run tests with or without --seed based on Jenkinsfile parameter Add comments to shaper Renamed test ids in test_rx_queues.py Modified conftest.py to set the seed Added a --seed option to pytest to allow tests to run with a fixed seed Use custom branch for test_support Get test_shaper passing Fixed _max_mbps calculation in DataLimiter.get_ifg(). Increased END_OF_TEST_TIME to avoid test time outs. Re-enabled RGMII 125MHz group a and b tests Port more tests to xs3 architecture Test for xs3 architecture
…IFG_AS_REF_CLOCK_COUNT_4b, RMII_ETHERNET_IFG_AS_REF_CLOCK_COUNT_1b_NO_TAIL_BYTES and RMII_ETHERNET_IFG_AS_REF_CLOCK_COUNT_1b_TAIL_BYTES defines in the code.
e26e811
to
1be46f6
Compare
- 7 | ||
- 3 (1-bit), 2 (4-bit) or 4 (1-bit) | ||
- 2 | ||
- ~TBD k |
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 have addressed this in my next PR (board support one)
examples/test_rmii_rt/CMakeLists.txt
Outdated
@@ -0,0 +1,29 @@ | |||
cmake_minimum_required(VERSION 3.21) |
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 these test apps should be removed. They are for my own dev only and are not documented or helpful to the customer. We have proper test apps and we will have a new documented example when we get new HW.
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, makes sense to remove them
SERVER_INTERFACE(ethernet_tx_if, i_tx_lp[n_tx_lp]), static_const_unsigned_t n_tx_lp, | ||
nullable_streaming_chanend_t c_rx_hp, | ||
nullable_streaming_chanend_t c_tx_hp, | ||
in_port_t p_clk, rmii_data_port_t * unsafe p_rxd, in_port_t p_rxdv, |
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'm windoering if I can't remove the unsafe bit. I'll add an issue. Currently it means the call to rmii_ethernet_rt_mac has to be wrapped in unsafe which is not nice.
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'll be great if we can remove unsafe. I noticed that the example in the documentation doesn't say unsafe
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, I thought that too. Let me have a look. See #76
I has a very long battle with the XC compiler to get it as far as it :(
@@ -23,5 +23,9 @@ set(LIB_COMPILER_FLAGS_mii_ethernet_rt_mac.xc ${LIB_COMPILER_FLAGS} -Wno-unusu | |||
set(LIB_COMPILER_FLAGS_rgmii_buffering.xc ${LIB_COMPILER_FLAGS} -Wno-unusual-code) | |||
set(LIB_COMPILER_FLAGS_rgmii_ethernet_mac.xc ${LIB_COMPILER_FLAGS} -Wno-unusual-code) | |||
set(LIB_COMPILER_FLAGS_ethernet.xc ${LIB_COMPILER_FLAGS} -Wno-cast-align) | |||
set(LIB_COMPILER_FLAGS_rmii_ethernet_rt_mac.xc ${LIB_COMPILER_FLAGS} -Wno-unusual-code) | |||
set(LIB_COMPILER_FLAGS_rmii_master.xc ${LIB_COMPILER_FLAGS} -mdual-issue) |
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 needs tidying. I'll do that
tests/helpers.py
Outdated
""" Returns the time it takes the DUT to process a given frame | ||
""" | ||
if mac == 'standard': | ||
return 4000 / phy.get_clock().get_clock_cycle_to_bit_time_ratio() |
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.
Not sure what these numbers mean? 4000/6000/2000 - maybe a comment?
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.
Oh, this is not used. I'l remove it. The packet_processing_time() function just before it has these hardcoded numbers. It's supposed to return the processing time of a packet, instead, it ignores the data_bytes argument and returns a fixed number derived from these hardcoded values. I'm not sure why this was written in this way and haven't tried changing it. I'll add a TODO in the code atleast.
tests/helpers.py
Outdated
|
||
def get_rmii_4b_port_tx_phy(clk, rxd_4b_port_pin_assignment, verbose=False, test_ctrl=None, | ||
do_timeout=True, complete_fn=None, expect_loopback=True, | ||
dut_exit_time=(50 * px.Xsi.get_xsi_tick_freq_hz())/1e6, initial_delay=(130 * px.Xsi.get_xsi_tick_freq_hz())/1e6): |
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 due_exit_time is in microseconds? Be good to comment
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, its microseconds. I'll add a comment
@@ -7,7 +7,7 @@ | |||
class Clock(px.SimThread): | |||
|
|||
# Use the values that need to be presented in the RGMII data pins when DV inactive | |||
(CLK_125MHz, CLK_25MHz, CLK_2_5MHz) = (0x4, 0x2, 0x0) | |||
(CLK_125MHz, CLK_25MHz, CLK_2_5MHz, CLK_50MHz) = (0x4, 0x2, 0x0, 0x1) |
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.
Was just wondering if we need to support 5MHz for 10Mb RMII? Not for this PR..
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 seems 10mbps is supported using 50MHz.
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.
Hmm, didnt occur to me 10Mbps RMII could be a requirement
@@ -26,8 +27,15 @@ def do_test(capfd, mac, arch, rx_clk, rx_phy, tx_clk, tx_phy, seed): | |||
if tx_clk.get_rate == Clock.CLK_125MHz: | |||
max_fragment_len = 142 | |||
|
|||
if tx_phy.get_name() == "rmii": | |||
min_fragment_length = 16 # https://github.com/xmos/lib_ethernet/issues/73 |
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.
nice..
{"phy":"rmii", "mac":"rt_hp", "arch":["xs3"], "rx_width":"4b_upper", "tx_width":"4b_lower"}, | ||
{"phy":"rmii", "mac":"rt_hp", "arch":["xs3"], "rx_width":"1b", "tx_width":"4b_lower"}, | ||
|
||
{"phy":"rmii", "mac":"rt", "arch":["xs3"], "rx_width":"4b_lower", "tx_width":"4b_upper"}, |
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.
wow - see what you mean about combinations. I think when we get DP RMII we can focus more on the functionality rather testing different port combos.
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!
@@ -281,6 +291,52 @@ def test_rx_queues(capfd, seed, params): | |||
weight_tagged=args.weight_tagged, weight_untagged=args.weight_untagged, | |||
max_hp_mbps=100) | |||
|
|||
elif params["phy"] == "rmii": |
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 (getting the PHY) is repeated quite a lot. Possibly worth being a helper. However, repetition is big thing in this test suite and would be a lot of effort to make more shared.
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 agree. Let me see if I can clean it a bit
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've added wrapper functions for rmii tx and rx phy creation. Hopefully that cleans up some repetitive code
lib_ethernet/src/rmii_master.xc
Outdated
/* Wait for the start of the packet and timestamp it */ | ||
unsigned sfd_preamble; | ||
|
||
// Ensure we have sufficiently sized buffer |
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.
Debug left in
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.
Thanks. Removed it
…rmii_100m_rt * commit '8475203f4d10bd0748a7e0eb18a4066d9a72f89b': Update lib_ethernet.rst Update lib_ethernet.rst Changelog Fix IFG calculation. Document the reasoning behind the RMII_ETHERNET_IFG_AS_REF_CLOCK_COUNT_4b, RMII_ETHERNET_IFG_AS_REF_CLOCK_COUNT_1b_NO_TAIL_BYTES and RMII_ETHERNET_IFG_AS_REF_CLOCK_COUNT_1b_TAIL_BYTES defines in the code. Fix copyright notice Modify test_4_1_2.py to not test fragments less than 16 bytes in length for RMII
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.
Looks good. A huge amount of work here - especially the tests.
A few comments in there. I have pushed my updates already.
|
||
on tile[0]: filler(0x1111); | ||
#if ETHERNET_SUPPORT_HP_QUEUES | ||
on tile[0]: test_rx(i_cfg[0], c_rx_hp, i_loopback, i_ctrl[0]); | ||
on tile[0]: test_rx_loopback(c_tx_hp, i_loopback); | ||
#else | ||
on tile[0]: filler(0x2222); |
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.
My previous comment got lost :(
I am not convinced we are properly testing timing. All other threads need to be run with fast mode (except the thread under test, which is hard as we'd need fast mode options in the main MAC) OR we need to constrain the clock frequency so the max MIPs is capped. Eg. 370 MHz / 5 = 74 MHz. I think we should add a basic test to see if we can support differing tail lengths under constrained MIPS. Doesn't need to be a big test or anything. What do you think? DOesn't need to be for this PR but we could add an issue.
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.
oh no :(. I thought the tests were testing timing properly. The threads under test, which I assume are rmii_master_rx_pins_4/1b, rmii_master_tx_pins, mii_ethernet_filter and mii_ethernet_server) are not in fast mode. Everything declared in the test file, so for test_rx for instance, test_rx(), the 2 filler threads and control() should all be in fast mode, only, I just noticed that the filler() threads randomly go in and out of fast mode (RANDOM_FAST_MODE is set to 1) :(
I'll set RANDOM_FAST_MODE to 0 and test if it meets timing. Would that be alright? or do you think we're still not testing timing properly. I didn't check with core freq set to 370MHz with 5 threads since I was getting weird warning s about 100MHz tick not generated correctly so wasn't sure if it was running ok.
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 enabling fast mode properly is a good step. I am assuming that rx_pins for MII is the toughest case. I think to be absolutely sure we should limit the thread speed. That will result in unpleasant warnings but it's a good way to to set limit the pins threads. It can be a separate test and it would need its own XN. TBH, it could be based on my test code which I took out. I don't mind doing that. You have done quite enough testing for now I think!
…eanup to address review comments
…t into feature/rmii_100m_rt
80aa95e
to
c2d6242
Compare
Add RMII (2b data @ 50MHz) version of the RT 100M MAC.
https://xmosjira.atlassian.net/browse/ETH-14
https://xmosjira.atlassian.net/browse/ETH-12