RMII dual port implementation #91

shuchitak · 2025-01-17T11:38:09Z

https://xmosjira.atlassian.net/browse/ETH-27

Adds support for the RMII dual MAC with some basic testing.

* develop: (21 commits) Remove unused part of test FIx changelog changelog re-order Fix table saying RMII supported Remove slicekit MII example Test RMII in a MIPS constrained environment Added pytest option to specify the test level. Not used by most tests right now Set RANDOM_FAST_MODE to 0 so the filler threads are always running in fast mode Added wrapper functions for rmii tx and rx phy creation. Some more cleanup to address review comments Remove Ed's test app Tidy lib makefiles 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 Initial RMII docs Another round of cleanup on the tests Extend more tests to test rmii configs ...

… ports

…ual_phy_feasibility

ed-xmos · 2025-01-22T14:46:16Z

lib_ethernet/api/ethernet.h

   */
  void disable_link_status_notification(size_t client_num);

+  /** When forwarding received packets, put them in the high priority queue,


I don't know yet if this the right thing to do. I'm concerned that forwarded LP traffic may block HP egress traffic from the internal port. However this is all still WIP so just noting that for now.

This is only for testing. I added this cfg function so I could test forwarding in HP queues. One of the test config has the application enable this but its disabled by default. In reality, there must be some other way for the mac to learn what the priority of a forwarding stream is.

ed-xmos · 2025-01-22T14:50:54Z

lib_ethernet/src/client_state.xc

-    client_state[i].rd_index = 0;
-    client_state[i].wr_index = 0;
-    client_state[i].status_update_state = STATUS_UPDATE_WAITING;
+    for(int p=0; p<MAX_ETHERNET_PORTS; p++)


Ah this looks like sc_ethernet!

Yeah, all of these need to be arrays of the max supported ethernet ports.

ed-xmos · 2025-01-22T14:54:16Z

lib_ethernet/src/mii_buffering.c

  memset(info->ptrs, 0, sizeof(info->ptrs));
 }

+void mii_init_packet_queue_new(packet_queue_info_t *queue)


Is this something you intended to leave here? Looks the same as mii_init_packet_queue() and not sure what new means!?

Just realised the type of queue is different - maybe improve the name to something other than _new.. I guess this is a backwards compatibility thing?

Oh, I meant to remove that. I was just testing if it would be possible to directly pass packet_queue_info_t* ptr to the function rather than mii_packet_queue_t and then typecasting to packet_queue_info_t* inside the function.
I don't understand the purpose of the mii_packet_queue_t type. Maybe eventually we can change the buffering APIs to pass packet_queue_info_t* directly? For now, I'll just delete this function.

ed-xmos · 2025-01-22T14:59:17Z

lib_ethernet/src/mii_filter.xc

    select {
 #pragma xta endpoint "rx_packet"
-    case c_conf :> int:
+    case c_conf :> int i:


Neat - we now use the notification token

yes, I extended the cfg commands that can be send to the filter to add one that says all packets need to be forwarded as high priority. This was just so I could test that forwarding packets as high priority works. I think in reality there'd be some other way to communicating to the mac that a given stream needs to be forwarded as high priority

ed-xmos · 2025-01-22T15:00:22Z

lib_ethernet/src/mii_filter.xc

    }

-    buf->src_port = 0;
+    buf->src_port = current_port;


Ha - you can see the original code had multi-port in mind

ed-xmos · 2025-01-22T15:01:15Z

lib_ethernet/src/mii_filter.xc

-    if (ethernet_filter_result_is_hp(filter_result)) {
-      if (!mii_packet_queue_full(rx_packets_hp)) {
-        mii_add_packet(rx_packets_hp, buf);
+    if(!buf->filter_result)


I think at some point we will want to allow an external filter (via an API) to make this decision. But fine for now. Comments are helpful.

ed-xmos · 2025-01-22T15:02:19Z

lib_ethernet/src/rgmii_buffering.xc

        client_wants_packet &= passed_etype_filter;
      }

      if (client_wants_packet) {


Ah of course - RGMII needed to updated so it handles an array of 1

ed-xmos · 2025-01-22T15:03:52Z

lib_ethernet/src/rmii_master.h

-                               phy_100mb_t phy_type);
+                               chanend c_rx_pins_exit[],
+                               phy_100mb_t phy_type,
+                               static const unsigned num_mac_ports);


Just wondering whether we can find a better term than num_mac_ports. Looks like we used master_ports before. num_phy_ports? Not sure...

I'm not sure what the master in master_ports stands for. I could use num_ethernet_ports or num_phy_ports. Maybe num_ethernet_ports?

ok, as discussed, leaving it as is for now.

tests/include/ports.h

ed-xmos · 2025-01-22T15:06:28Z

tests/test_link_status/src/main.xc


  size_t index = rx.get_index();

+  index = 0; // This is the mac port index and not the client index


Helpful comment but better to change the var name to make this clear?

ed-xmos · 2025-01-22T15:08:29Z

tests/test_rmii_dual_basic/test_params.json

@@ -0,0 +1,6 @@
+{
+    "PROFILES": [
+        {"phy":"rmii", "mac":"rt", "arch":["xs3"], "rx_width":"4b_lower", "tx_width":"4b_lower"},


Could be an annoying amount of work but do we need "Mac":"rt_hp_dual"?

yes, I guess that's the goal. Have the existing tests have the option to run with rt_hp_dual mac. I'll leave it for a later PR. For the dual mac would we still want to test all combinations of rx and tx widths for both the Macs?

ed-xmos · 2025-01-22T15:12:02Z

lib_ethernet/src/rmii_master.xc

                }

-                mii_free_current(packets_lp);
+                mii_free_current(pkt_queue);


Is this right? Are LP and HP all in the same queue?

No, the code above that assigns pkt_queue to either packets_hp, forwarding_packets_hp, packets_lp or forwarding_packets_lp in that order

ed-xmos · 2025-01-22T15:13:44Z

tests/helpers.py

 def create_expect(packets, filename):
    """ Create the expect file for what packets should be reported by the DUT
    """
+    ndim = np.array(packets).ndim


SOrry a naming thing again. ndim = number of packet streams?

ndim is number of ethernet ports. [num ethernet ports][packets seen on a givent port]. I'll change the name.

actually ndim does make sense coz its literally finding the array dimension. I'll add comments explaining what ndim=1 vs ndim=2 mean

ed-xmos · 2025-01-22T15:17:21Z

lib_ethernet/src/mii_ethernet_rt_mac.xc


          // Store the index into the packet queue
-          client_state.fifo[wr_index] = (void *)rd_index;
+          client_state.fifo[current_port][wr_index] = (void *)rd_index;


#pragma unsafe arrays may be worth considering with so much array bounds checking now (once we are confident it all works, which the tests should prove.

The array bound checking has helpfully pointed out issues when I added support for multiple queues so will keep it on till we're done implementing all the features. I agree it makes sense turning off once we're confident about the code.

ed-xmos · 2025-01-22T16:51:24Z

lib_ethernet/src/mii_ethernet_rt_mac.xc

    int client_wants_packet = 0;
    if (buf->filter_result && ethernet_filter_result_is_hp(buf->filter_result)) {
-      client_wants_packet = (buf->filter_result & 1);
+      client_wants_packet = (buf->filter_result & 1); // there's only one hp client??


Yes - I think this is a statement rather than a question- we only support 1 HP client.

ed-xmos · 2025-01-22T16:56:18Z

lib_ethernet/src/mii_ethernet_rt_mac.xc

+        mii_commit(tx_mem_hp[dst_port], dptr);
+        mii_add_packet((mii_packet_queue_t)&tx_packets_hp[dst_port], buf);
+        tx_client_state_hp[0].send_buffer[dst_port] = null;
+        prioritize_rx = 3;


WHat is the significance of 3 here? I see it is carried over from the previous codebase.

Wonder if a enum is more appropriate?

prioritize_rx I think lets the ethernet_server prioritise rx over tx. Its used as a counter, so something like do a tx once every 3 iterations of the while loop.

lib_ethernet/src/rmii_ethernet_rt_mac.xc

ed-xmos · 2025-01-22T17:03:13Z

lib_ethernet/src/rmii_ethernet_rt_mac.xc

+  // Establish types of data ports presented
+    unsafe{
+        // Setup buffering
+        unsigned int rx_data[rx_bufsize_words ][2];


I think this needs documenting carefully - or perhaps each port gets half of the total?

yes, each port gets half. It should be rx_data[2][rx_bufsize_words] ideally but xc wouldn't allow rx_bufsize_words in the inner dimension so left it like this. mem pool init only uses the start ptr and size so should be okay.

mii_init_mempool(&rx_data[0][0] + irx_bufsize_words, rx_bufsize_words4);

ed-xmos

This is great - a huge amount of work and most of the machinery is now in place. Great that you added a reasonable amount of test coverage too and that the existing regressions are passing. I think we need to consider the forwarding path a little more - perhaps another look at sc_ethernet? But for now this is most of the leg work done.
It would be really great if you could produce a digram (or modify this https://xmosjira.atlassian.net/wiki/spaces/Ethernet/pages/4468867073/RMII+dual+port+feasibility#Proposed-lib_ethernet-dual-port-RMII-Architecture) to show how it really works under the hood. Especially the separate queues. Some basic notes on how it works on a confluence page would be good too, especially as we don't know when/who will be doing the next step of DP and the fact this feasibility has gone to stage where are amount of implementation is done. There a couple of items which would be good to clarify (see individual comments) first before approval if that's OK. Happy to discuss in person tomorrow if easier than comments on here.

shuchitak · 2025-01-23T16:18:51Z

This is great - a huge amount of work and most of the machinery is now in place. Great that you added a reasonable amount of test coverage too and that the existing regressions are passing. I think we need to consider the forwarding path a little more - perhaps another look at sc_ethernet? But for now this is most of the leg work done. It would be really great if you could produce a digram (or modify this https://xmosjira.atlassian.net/wiki/spaces/Ethernet/pages/4468867073/RMII+dual+port+feasibility#Proposed-lib_ethernet-dual-port-RMII-Architecture) to show how it really works under the hood. Especially the separate queues. Some basic notes on how it works on a confluence page would be good too, especially as we don't know when/who will be doing the next step of DP and the fact this feasibility has gone to stage where are amount of implementation is done. There a couple of items which would be good to clarify (see individual comments) first before approval if that's OK. Happy to discuss in person tomorrow if easier than comments on here.

I'm added some implementation details as part of the spec (https://xmosjira.atlassian.net/wiki/spaces/Ethernet/pages/4435902469/Ethernet+Milestone+A#%F0%9F%94%A7-Implementation-Detail) but you're right, we need more description of the queue handling which is quite complicated. I'll add this in a separate confluence page.

ed-xmos and others added 18 commits December 17, 2024 16:47

Initial API and test prog

0c3625b

Initial API and pins for dual MAC

7e3d4a2

Use setup helpers in rmii_ethernet_rt_mac too

5c44988

Update initial local dual mac test app

d016936

Fix null pointer issue with init_rx_ports

defd772

Initial dual mac basic test

c2f069d

Add support for second RMII PHY in tests

a9eb986

Add extra PHYs to test_dual_basic

febe6ab

More WIP dual rmii eval

811cc80

Allocate memory pool and queues per ethernet port

a880f7c

Modified rmii_master_tx_pins to pass forwarding queues

ddb1e59

Add support for hp packets

661fcf4

Fix tx timestamp andlink status update code to work with multiple mac…

8bf59f9

… ports

Merge develop and resolve conflicts

cd0c227

Set NUM_ETHERNET_PORTS to 1 by default.

3983959

Changes to ensure the existing single port rmii tests are not broken

d4339dd

Merge branch 'develop' of github.com:xmos/lib_ethernet into feature/d…

50c2317

…ual_phy_feasibility

shuchitak force-pushed the feature/dual_phy_feasibility branch 4 times, most recently from 2fd0361 to 552aebd Compare January 17, 2025 15:55

Fix test failures

fb50876

shuchitak force-pushed the feature/dual_phy_feasibility branch from 552aebd to fb50876 Compare January 17, 2025 16:31

Merge develop and resolve conflicts

d28e70d

shuchitak force-pushed the feature/dual_phy_feasibility branch from 32439d3 to def3fa9 Compare January 21, 2025 15:22

Added rmii dual phy forwarding only test

ab9645b

shuchitak force-pushed the feature/dual_phy_feasibility branch from def3fa9 to ab9645b Compare January 21, 2025 16:53

Shuchita Khare added 2 commits January 21, 2025 21:02

Sync between sim threads by writing to ports

8f4288c

Use multiprocessing.Pipe for synchronisation between threads

17e9a5c

Remove dependence on NUM_ETHERNET_PORTS define

b3ec298

shuchitak changed the title ~~Feature/dual phy feasibility~~ RMII dual port implementation Jan 22, 2025

shuchitak marked this pull request as ready for review January 22, 2025 12:02

shuchitak requested a review from ed-xmos January 22, 2025 12:02

shuchitak force-pushed the feature/dual_phy_feasibility branch 2 times, most recently from 36e1ad1 to bb0989b Compare January 22, 2025 12:55

Removed examples/test_rmii_dual

c9375e6

shuchitak force-pushed the feature/dual_phy_feasibility branch from bb0989b to c9375e6 Compare January 22, 2025 13:39

ed-xmos reviewed Jan 22, 2025

View reviewed changes

tests/include/ports.h Show resolved Hide resolved

ed-xmos reviewed Jan 22, 2025

View reviewed changes

lib_ethernet/src/rmii_ethernet_rt_mac.xc Show resolved Hide resolved

ed-xmos reviewed Jan 22, 2025

View reviewed changes

review comments

50230da

humphrey-xmos mentioned this pull request Aug 12, 2025

Adding dual ethernet 316 dev-kit xmos/lib_board_support#66

Merged


		size_t index = rx.get_index();

		index = 0; // This is the mac port index and not the client index

RMII dual port implementation #91

Are you sure you want to change the base?

RMII dual port implementation #91

Conversation

shuchitak commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ed-xmos Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shuchitak Jan 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ed-xmos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

shuchitak commented Jan 17, 2025 •

edited

Loading

ed-xmos Jan 22, 2025 •

edited

Loading

shuchitak Jan 23, 2025 •

edited

Loading

ed-xmos left a comment •

edited

Loading