Update Zynq Ultrascale port for V4.x and Clean up by StefanBalt · Pull Request #1187 · FreeRTOS/FreeRTOS-Plus-TCP

StefanBalt · 2024-09-11T16:31:42Z

Description

This pull request introduces the V4.x port for the Xilinx Ultrascale, which basically merges the previous implementation, the Zynq7000 port and the suggestion by @pete-pjb here #1172

Key Changes:

V4.x Port: A combination of previous work, the Zynq7000 port, and the proposal in by @pete-pjb here [Feature Request] Xilinx Zynq and Ultrascale functional differences in 4.x #1172
Interrupt Mapping: A new header file, x_emac_map.h, has been introduced to handle interrupt IDs. Since there is no canonical definition for EMAC interrupt IDs, the header maps interrupt IDs based on the EMAC base address.
LLMNR Update: The LLMNR implementation now leverages the multicast MAC hash table rather than using a dedicated MAC entry.
Removal of Zynq7000 (GEM V2) Code: Zynq7000-related code has been removed, as the ports for the Zynq7000 and Ultrascale are quite distinct. Keeping the Zynq7000 code IMHO no longer adds value and was redundant.
Micrel PHY support is also from the .zip provided by @pete-pjb. However, this has not been tested and may require further validation.

I will provide inline comments on specific sections where I'm uncertain or need feedback.

Test Steps

I could only test a single interface with IPv4 so far. Junbo Frames and LLMNR also work fine.
I plan to test multiple interfaces on a Kria board I have at my hand soon.

Checklist:

I have tested my changes. No regression in existing tests.
I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

#1172

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

StefanBalt · 2024-09-11T16:41:25Z

source/portable/NetworkInterface/xilinx_ultrascale/NetworkInterface.c

+                XEmacPs_SetOptions( pxEMAC_PS, XEMACPS_MULTICAST_OPTION );
+            #endif /* ( ( ipconfigUSE_MDNS == 1 ) && ( ipconfigUSE_IPv6 != 0 ) ) */
+
+            /* In the Zynq7000 port the MAC-Address is stored on second endpoint here at position 4. Needed? */


As mentioned in the comment Zynq7000 does this here:

pxEndPoint = FreeRTOS_NextEndPoint( pxInterface, pxEndPoint ); if( pxEndPoint != NULL ) { /* If there is a second end-point, store the MAC * address at position 4.*/ XEmacPs_SetMacAddress( pxEMAC_PS, ( void * ) pxEndPoint->xMACAddress.ucBytes, 4 ); }

Do we also need this here?

source/portable/NetworkInterface/xilinx_ultrascale/NetworkInterface.c

StefanBalt · 2024-09-11T16:49:47Z

source/portable/NetworkInterface/xilinx_ultrascale/x_emacpsif_hw.c

                {
                    last_err_msg = "Receive DMA error";
-                    xNetworkInterfaceInitialise();
+                    vInitialiseOnIndex( xEMACIndex );


I am not sure whether these are any good as vInitialiseOnIndex() cannot be run again when it once ended up in xEMAC_Fatal or xEMAC_Ready.

htibosch

Hi @StefanBalt, that is great work! Until now the UltraScale driver would only work with the older single-interface version. You made it standard and compliant with 4.x.x.
In a later stage, I will also test it in hardware.

pxEndPoint = FreeRTOS_NextEndPoint( pxInterface, pxEndPoint );

if( pxEndPoint != NULL )
{
	/* If there is a second end-point, store the MAC
	 * address at position 4.*/
	XEmacPs_SetMacAddress( pxEMAC_PS, ( void * ) pxEndPoint->xMACAddress.ucBytes, 4 );
}

You wrote:

Do we also need this here?

It needs to be worked out.

We can have multiple interfaces each with multiple endpoints, long with different MAC-addresses.

The peripheral has space for 4 primary MAC-addresses. All other addresses will be set in the hash table.

I wish that PR 1019 were already approved and merged. That will contain a method of setting the MAC-addresses in a proper way.

    #if ( ( ipconfigUSE_MDNS == 1 ) && ( ipconfigUSE_IPv6 != 0 ) )
    XEmacPs_SetHash( pxEMAC_PS, ( void * ) xMDNS_MacAddress.ucBytes ); /* why for IPv6? */
    XEmacPs_SetHash( pxEMAC_PS, ( void * ) xMDNS_MACAddressIPv6.ucBytes );

Why only add MDNS multicast MAC to hash table when ( ipconfigUSE_IPv6 != 0 )?

You are right, that is not correct.

Also setting the "Solicited-Node Multicast" addresses { 0x33, 0x33, 0xff, 0, 0, 0 }; only depends on ipconfigUSE_IPv6 and not on ipconfigUSE_LLMNR.

    /* allow reception of multicast addresses programmed into hash */
    XEmacPs_SetOptions( pxEMAC_PS, XEMACPS_MULTICAST_OPTION );

These are also missing in the Zynq7000 port and most likely needed.
Should I add it?

I think so. The XEMACPS_MULTICAST_OPTION refers to XEMACPS_NWCFG_MCASTHASHEN_MASK, or MCAST (multicast) HASH (hash) EN (enable). That is indeed important.

    /* Allow multicast address filtering */
    if( ( Options & XEMACPS_MULTICAST_OPTION ) != 0x00000000U )
    {
        RegNewNetCfg |= XEMACPS_NWCFG_MCASTHASHEN_MASK;
    }

But I cannot test it.

By enabling and playing with mDNS?

Is it really necessary to add ucMACAddress to all endpoints?

The global ucMACAddress doesn't exists since release 4.x.

I would have thought XEmacPs_SetHash( pxEMAC_PS, ( void * ) xLLMNR_MacAddressIPv6.ucBytes ); would be sufficient.

I think so too.

htibosch · 2024-09-12T07:44:00Z

Every Ethernet peripheral, or most of them, has two ways of setting a MAC-address:

Up to four standard MAC-addresses. Need 100% match
A hash table with like 64 entries. Each entry is actually a bit.

Hash: when you add a MAC address whose calculated hash value is 12, then bit 12 in the hash table will get set.

( no news for most of you ).

Emil Popov and me came up with this idea:

First add primary MAC-addresses, i.e. the hardware addresses of the endpoint.
Secondly, add the MAC-addresses of the multicast addresses used (e.g. mDNS, LLMNR).

The first 4 (GMACSA_NUMBER) addresses will go to the table in the peripheral.
All subsequent addresses will be hashed.

MAC-addresses can be unhashed too: there will be 64 usage counters. When the counter reaches zero again, the hash value will be cleared.
You find an implementation in DriverSAM. Maybe we should put it in Common, so all drivers can use it.

For the Ultrascale port, I am thinking of using the same method.

This is basically a merge of the previous port, the Zynq7000 port and the port suggested by Pete Bone <pete-pjb@users.noreply.github.com>.

This is how it is supposed to be used. Also the set of the multi-cast hash enable bit was missing.

The same effect can be achieved but the code is simpler.

There are already a lot of differences between Zynq and xilinx_ultrascale port, so there is no need to keep compatibility.

This map makes sure the correct interrupt id is registered in the interrupt controller. E.g. 'XPAR_XEMACPS_0_BASEADDR' is Canonical for the first interface and can be mapped to any of the GEMs. 'XPAR_XEMACPS_0_INTR' on the other hand is fixed to GEM0. This is why this mapping is needed.

Authored-by: Pete Bone <pete-pjb@users.noreply.github.com >

Set solicited-node addresses independent of LLMNR. For mDNS set IPv4/6 MAC depending on ipconfigUSE_IPv6.

StefanBalt · 2024-09-12T12:55:20Z

Ah youre'right the Zynq7000 port uses XEMACPS_NWCFG_MCASTHASHEN_MASK directly instead of using the API function.
I added the discussed multicast fixes to the Zynq7000 port.

I noticed the promiscuous mode in the Zynq7000 port which is somewhat questionable as a default:

    /* As 'MCASTHASHEN' doesn't seem to work, use the promiscuous mode so that IPv6 multicast packets are received. */
    /* Allow promiscuous mode. */
    ulValue |= XEMACPS_NWCFG_COPYALLEN_MASK;

But I cannot test it.
By enabling and playing with mDNS?

I meant I do not have a Zynq7000 at hand. I did test mDNS now for the Ultrascale.

I do like the idea of sharing the MAC related code in Common, however I do not have the time to do it at the moment.

My testing is complete for now here is what I did and what I think should still be tested:

IPv4 including LLMNR, mDNS, Jumbo Frames
Two endpoints (Virtual interfaces) on the same physical interface
IPv6
More than one physical interface
Zynq7000 changes

tony-josi-aws · 2024-09-26T10:28:07Z

@StefanBalt

Thanks for taking time to contribute to FreeRTOS+TCP.

Can you help in fixing the CI checks:

To fix the spell checks these words [those listed in the logs Run spellings check] needs to be added to this file.
To fix the formatting please apply this patch.

Please let us know if you have questions or want us to take care of the CI.

htibosch

Thank you so far, Stefan for this PR. I don't have the time to test it, I hope you did.
It will still have some work to do, but let's wait for the multi-cast feature first.

htibosch · 2024-09-30T06:02:40Z

source/portable/NetworkInterface/Zynq/NetworkInterface.c

-
                XEmacPs_SetHash( pxEMAC_PS, ( void * ) xLLMNR_MacAddressIPv6.ucBytes );
            }
            #endif /* if ( ipconfigUSE_IPv6 == 0 ) */


The above is not correct, think of this:

#if( ipconfigUSE_LLMNR == 1 ) { #if( ipconfigIS_ENABLED( ipconfigUSE_IPv4 ) ) { XEmacPs_SetHash( pxEMAC_PS, ( void * ) xLLMNR_MacAddress.ucBytes ); } #endif #if( ipconfigIS_ENABLED( ipconfigUSE_IPv6 ) ) { XEmacPs_SetHash( pxEMAC_PS, ( void * ) xLLMNR_MacAddressIPv6.ucBytes ); } #endif } #endif /* ipconfigUSE_LLMNR == 1 */

It means that LLMNR can be enabled for IPv4 and on IPv6 at the same time.

Ah of course IPv4 and v6 can be both enabled. Done

htibosch · 2024-09-30T06:03:52Z

source/portable/NetworkInterface/Zynq/NetworkInterface.c

+            }
+            #endif /* if ( ipconfigUSE_IPv6 == 0 ) */
+        }
+        #endif /* ( ipconfigUSE_MDNS == 1 ) */


and also here:

#if ( ipconfigIS_ENABLED( ipconfigUSE_MDNS ) ) #if ( ipconfigIS_ENABLED( ipconfigUSE_IPv4 ) ) { XEmacPs_SetHash( pxEMAC_PS, ( void * ) xMDNS_MacAddress.ucBytes ); } #endif #if ( ipconfigIS_ENABLED( ipconfigUSE_IPv6 ) ) { XEmacPs_SetHash( pxEMAC_PS, ( void * ) xMDNS_MACAddressIPv6.ucBytes ); } #endif #endif /* ipconfigUSE_MDNS */

MDNS can also be active on both IP-versions.

htibosch · 2024-09-30T06:06:16Z

source/portable/NetworkInterface/xilinx_ultrascale/NetworkInterface.c

+            }
+        #endif /* ipconfigUSE_IPv6 */
+
        if( ( pxPacket->xICMPPacket.xEthernetHeader.usFrameType == ipIPv4_FRAME_TYPE ) &&


The driver should not bother about IPv4 frames, unless ipconfigUSE_IPv4 is defined.

tony-josi-aws · 2024-09-30T08:41:39Z

@StefanBalt
Since the multicast PR needs more time to be merged, can you please help in fixing the following proposals from @htibosch so that we can go ahead with merging the branch:

LLMNR
MDNS

Please feel free to let us know if you prefer us to make the suggested changes.
Thank you.

StefanBalt · 2024-09-30T09:52:16Z

Sure, I will push a fix (probably today).

StefanBalt · 2024-10-01T08:51:32Z

I don't have the time to test it, I hope you did.

As mentioned earlier, I tested some of the features but not all (i.e. IPv6, DHCP and more than one physical interface missing).

source/portable/NetworkInterface/xilinx_ultrascale/NetworkInterface.c

Co-authored-by: ActoryOu <jay2002824@gmail.com>

StefanBalt requested a review from a team as a code owner September 11, 2024 16:31

StefanBalt commented Sep 11, 2024

View reviewed changes

source/portable/NetworkInterface/xilinx_ultrascale/NetworkInterface.c Outdated Show resolved Hide resolved

StefanBalt commented Sep 11, 2024

View reviewed changes

source/portable/NetworkInterface/xilinx_ultrascale/NetworkInterface.c Outdated Show resolved Hide resolved

StefanBalt commented Sep 11, 2024

View reviewed changes

source/portable/NetworkInterface/xilinx_ultrascale/NetworkInterface.c Outdated Show resolved Hide resolved

StefanBalt commented Sep 11, 2024

View reviewed changes

htibosch reviewed Sep 12, 2024

View reviewed changes

StefanBalt added 7 commits September 12, 2024 14:09

Update Zynq Ultrascale port for V4.x

970911e

This is basically a merge of the previous port, the Zynq7000 port and the port suggested by Pete Bone <pete-pjb@users.noreply.github.com>.

Use MAC hash table for IPv4 LLMNR

e448ae8

This is how it is supposed to be used. Also the set of the multi-cast hash enable bit was missing.

Use XEmacPs_DMABLengthUpdate() API

9dc75a3

The same effect can be achieved but the code is simpler.

Remove Zynq7000 support from Ultrascale port

0dbda17

There are already a lot of differences between Zynq and xilinx_ultrascale port, so there is no need to keep compatibility.

Add Micrel PHY support to xilinx_ultrascale port

2eaa162

Authored-by: Pete Bone <pete-pjb@users.noreply.github.com >

Fix Zynq7000 EMAC MAC address setup

3e63765

Set solicited-node addresses independent of LLMNR. For mDNS set IPv4/6 MAC depending on ipconfigUSE_IPv6.

Merge branch 'main' into main

3e948ae

StefanBalt added 2 commits September 27, 2024 12:22

Uncrustify

60a7f1c

Fix CI spelling errors

5c2e46d

tony-josi-aws previously approved these changes Sep 30, 2024

View reviewed changes

aggarg previously approved these changes Sep 30, 2024

View reviewed changes

Merge branch 'main' into main

eada2d4

htibosch approved these changes Sep 30, 2024

View reviewed changes

Merge branch 'main' into main

0253c34

StefanBalt dismissed stale reviews from tony-josi-aws and aggarg via d5f99e0 October 1, 2024 08:45

Fix IPv4/6 preprocessor logic in Zynq/Ultrascale port

5aa836b

tony-josi-aws previously approved these changes Oct 1, 2024

View reviewed changes

ActoryOu reviewed Oct 1, 2024

View reviewed changes

source/portable/NetworkInterface/xilinx_ultrascale/NetworkInterface.c Outdated Show resolved Hide resolved

Add missing return in xilinx_ultrascale/NetworkInterface.c

f2cf4b2

Co-authored-by: ActoryOu <jay2002824@gmail.com>

StefanBalt dismissed tony-josi-aws’s stale review via f2cf4b2 October 2, 2024 09:13

ActoryOu approved these changes Oct 3, 2024

View reviewed changes

tony-josi-aws approved these changes Oct 3, 2024

View reviewed changes

aggarg approved these changes Oct 3, 2024

View reviewed changes

tony-josi-aws merged commit dd88502 into FreeRTOS:main Oct 3, 2024

Conversation

StefanBalt commented Sep 11, 2024

Description

Test Steps

Checklist:

Related Issue

Uh oh!

StefanBalt Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StefanBalt Sep 11, 2024

Choose a reason for hiding this comment

Uh oh!

htibosch left a comment

Choose a reason for hiding this comment

Uh oh!

htibosch commented Sep 12, 2024

Uh oh!

StefanBalt commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tony-josi-aws commented Sep 26, 2024

Uh oh!

htibosch left a comment

Choose a reason for hiding this comment

Uh oh!

htibosch Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

StefanBalt Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

htibosch Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

htibosch Sep 30, 2024 • 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!

StefanBalt Oct 1, 2024

Choose a reason for hiding this comment

Uh oh!

tony-josi-aws commented Sep 30, 2024

Uh oh!

StefanBalt commented Sep 30, 2024

Uh oh!

StefanBalt commented Oct 1, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

StefanBalt commented Sep 12, 2024 •

edited

Loading

htibosch Sep 30, 2024 •

edited

Loading