-
Notifications
You must be signed in to change notification settings - Fork 16
RMII 8b tx #85
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
base: develop
Are you sure you want to change the base?
RMII 8b tx #85
Conversation
Conflicts: lib_ethernet/api/ethernet.h lib_ethernet/src/rmii_ethernet_rt_mac.xc lib_ethernet/src/rmii_master.h lib_ethernet/src/rmii_master.xc
Conflicts: Jenkinsfile examples/app_rmii_100Mbit_icmp/src/icmp.h examples/app_rmii_100Mbit_icmp/src/icmp.xc examples/app_rmii_100Mbit_icmp/src/main.xc examples/app_rmii_100Mbit_icmp/src/xk-eth-xu316-dual-100m.xn examples/deps.cmake lib_ethernet/api/ethernet.h lib_ethernet/api/smi.h lib_ethernet/src/mii_ethernet_rt_mac.xc lib_ethernet/src/rgmii_buffering.xc lib_ethernet/src/rmii_ethernet_rt_mac.xc lib_ethernet/src/rmii_master.h lib_ethernet/src/rmii_master.xc lib_ethernet/src/smi.xc requirements.txt tests/CMakeLists.txt tests/bringup_xk_eth_xu316_dual_100m/CMakeLists.txt tests/bringup_xk_eth_xu316_dual_100m/src/icmp.h tests/bringup_xk_eth_xu316_dual_100m/src/icmp.xc tests/bringup_xk_eth_xu316_dual_100m/src/main.xc tests/bringup_xk_eth_xu316_dual_100m/src/xk-eth-xu316-dual-100m.xn tests/test_smi/src/main.xc
Conflicts: examples/app_rmii_100Mbit_icmp/src/main.xc lib_ethernet/api/ethernet.h
# Conflicts: # examples/app_ethernet_diagnostics/src/main.xc # lib_ethernet/api/ethernet.h # lib_ethernet/api/smi.h
# Conflicts: # tests/bringup_xk_eth_xu316_dual_100m/CMakeLists.txt
- Extend test_tx, test_time_tx and test_rmii_timing to test 8b TX configs - Updated the TX IFG calculation for 8b TX, although, the turnaround time between consecutive TX is greater than the required IFG wait so the IFG wait is never really exercised to 8b TX
humphrey-xmos
left 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.
Good stuff
| unsigned time; | ||
| const unsigned poly = 0xEDB88320; | ||
| unsigned * unsafe dptr = &buf->data[0]; | ||
| unsigned * unsafe wrap_ptr = mii_get_wrap_ptr(tx_mem);; |
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.
Double semi-colon
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.
Pull Request Overview
This PR adds support for 8-bit RMII TX functionality to the Ethernet library. The implementation extends the existing RMII TX capabilities to support 8-bit port configurations in addition to the existing 4-bit and 1-bit options.
Key changes:
- Adds new 8-bit TX assembly implementation for improved performance
- Updates test framework to support 8-bit TX test configurations
- Refactors pin assignment handling to support flexible bit positioning for 8-bit ports
Reviewed Changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib_ethernet/src/rmii_master_tx_pins_8b.S | New assembly implementation for 8-bit RMII TX with optimized performance |
| lib_ethernet/src/rmii_master.xc | Core logic updates to support 8-bit TX including lookup tables and packet transmission |
| lib_ethernet/api/ethernet.h | API updates to support 8-bit port configuration and pin assignment macros |
| tests/rmii_phy.py | Test infrastructure updates to handle 8-bit port pin assignments |
| tests/helpers.py | Helper functions for 8-bit RMII PHY configuration |
| tests/helpers.cmake | CMake macros to parse 8-bit TX width configurations |
| Various test files | Updated test parameters and configurations to include 8-bit TX test cases |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| on tile[0]: { | ||
| rmii_ethernet_rt_mac( i_cfg, NUM_CFG_IF, |
Copilot
AI
Sep 17, 2025
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.
[nitpick] The addition of braces around the rmii_ethernet_rt_mac call creates inconsistent formatting with the MII case above. Consider applying the same formatting to both cases for consistency.
| #ifndef __icmp_h__ | ||
| #define __icmp_h__ | ||
| #include <ethernet.h> | ||
|
|
Copilot
AI
Sep 17, 2025
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.
[nitpick] The removal of blank lines creates inconsistent spacing. The blank line should be preserved to maintain consistent formatting with the rest of the codebase.
| case 8: { | ||
| time = rmii_transmit_packet_8b(tx_mem, buf, *p_mii_txd_0, lookup_8b_tx, ifg_tmr, ifg_time, eof_time); | ||
| eof_time = ifg_time; | ||
| ifg_time += RMII_ETHERNET_IFG_AS_REF_CLOCK_COUNT_8b; // TODO WORK ME OUT |
Copilot
AI
Sep 17, 2025
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.
The TODO comment indicates incomplete IFG calculations for 8-bit mode. This should be resolved before merging to production as it affects timing correctness.
| static void init_8b_tx_lookup(unsigned lookup[], int bitpos_0, int bitpos_1){ | ||
| for(int i = 0; i < 256; i++){ | ||
| lookup[i] = 0; // All unused pins driven low | ||
| lookup[i] |= (i & 0x1) << (bitpos_0 + 0); | ||
| lookup[i] |= (i & 0x2) << (bitpos_1 - 1); | ||
| lookup[i] |= (i & 0x4) << (bitpos_0 + 6); | ||
| lookup[i] |= (i & 0x8) << (bitpos_1 + 5); | ||
| lookup[i] |= (i & 0x10) << (bitpos_0 + 12); | ||
| lookup[i] |= (i & 0x20) << (bitpos_1 + 11); | ||
| lookup[i] |= (i & 0x40) << (bitpos_0 + 18); |
Copilot
AI
Sep 17, 2025
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.
The bit manipulation logic uses magic numbers without explanation. Consider adding comments to explain the bit positioning logic, especially the relationship between the input byte bits and output port positions.
| static void init_8b_tx_lookup(unsigned lookup[], int bitpos_0, int bitpos_1){ | |
| for(int i = 0; i < 256; i++){ | |
| lookup[i] = 0; // All unused pins driven low | |
| lookup[i] |= (i & 0x1) << (bitpos_0 + 0); | |
| lookup[i] |= (i & 0x2) << (bitpos_1 - 1); | |
| lookup[i] |= (i & 0x4) << (bitpos_0 + 6); | |
| lookup[i] |= (i & 0x8) << (bitpos_1 + 5); | |
| lookup[i] |= (i & 0x10) << (bitpos_0 + 12); | |
| lookup[i] |= (i & 0x20) << (bitpos_1 + 11); | |
| lookup[i] |= (i & 0x40) << (bitpos_0 + 18); | |
| static void init_8b_tx_lookup(unsigned lookup[], int bitpos_0, int bitpos_1){ | |
| /* | |
| * This function creates a lookup table mapping each possible 8-bit value (0-255) | |
| * to a port output value, where each bit in the input byte is mapped to a specific | |
| * output port position. The mapping uses two base bit positions: bitpos_0 and bitpos_1, | |
| * which correspond to the physical pin positions for the two RMII TXD ports. | |
| * | |
| * The mapping is as follows: | |
| * - Input bit 0 (0x01): output at bitpos_0 + 0 | |
| * - Input bit 1 (0x02): output at bitpos_1 - 1 | |
| * - Input bit 2 (0x04): output at bitpos_0 + 6 | |
| * - Input bit 3 (0x08): output at bitpos_1 + 5 | |
| * - Input bit 4 (0x10): output at bitpos_0 + 12 | |
| * - Input bit 5 (0x20): output at bitpos_1 + 11 | |
| * - Input bit 6 (0x40): output at bitpos_0 + 18 | |
| * - Input bit 7 (0x80): output at bitpos_1 + 17 | |
| * | |
| * This arrangement is determined by the hardware wiring and protocol requirements. | |
| * The use of bitpos_0 and bitpos_1 allows for flexible assignment of output pins. | |
| */ | |
| for(int i = 0; i < 256; i++){ | |
| lookup[i] = 0; // All unused pins driven low | |
| // Map input bit 0 (LSB) to output at bitpos_0 + 0 | |
| lookup[i] |= (i & 0x1) << (bitpos_0 + 0); | |
| // Map input bit 1 to output at bitpos_1 - 1 | |
| lookup[i] |= (i & 0x2) << (bitpos_1 - 1); | |
| // Map input bit 2 to output at bitpos_0 + 6 | |
| lookup[i] |= (i & 0x4) << (bitpos_0 + 6); | |
| // Map input bit 3 to output at bitpos_1 + 5 | |
| lookup[i] |= (i & 0x8) << (bitpos_1 + 5); | |
| // Map input bit 4 to output at bitpos_0 + 12 | |
| lookup[i] |= (i & 0x10) << (bitpos_0 + 12); | |
| // Map input bit 5 to output at bitpos_1 + 11 | |
| lookup[i] |= (i & 0x20) << (bitpos_1 + 11); | |
| // Map input bit 6 to output at bitpos_0 + 18 | |
| lookup[i] |= (i & 0x40) << (bitpos_0 + 18); | |
| // Map input bit 7 (MSB) to output at bitpos_1 + 17 |
humphrey-xmos
left 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.
Sorry, should have approved the other day.
Other than a few formatting changes and maybe clearing up some 'TODO's its good
See https://xmosjira.atlassian.net/wiki/spaces/Ethernet/pages/4478271544/8b+port+RMII+Tx+Feasibility
This has been tested in a local 8b_tx -> 1b_rx loopback app only but results are positive so far and it passes at 70MHz min thread speed so just meets our goal of 75MHz. The wrapping logic took this from 60 to 70MHz.
Captured in #88
It has been verified using the tests/bringup app (ping) also in hw.
This still needs the sim/hw tests extended and IFG calculations done as well as extending the docs.