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

Update Zynq Ultrascale port for V4.x and Clean up #1187

Merged
merged 14 commits into from
Oct 3, 2024

Conversation

StefanBalt
Copy link
Contributor

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 StefanBalt requested a review from a team as a code owner September 11, 2024 16:31
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? */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

@@ -173,7 +181,7 @@ static void emacps_handle_error( void * arg,
if( ( ErrorWord & XEMACPS_RXSR_HRESPNOK_MASK ) != 0 )
{
last_err_msg = "Receive DMA error";
xNetworkInterfaceInitialise();
vInitialiseOnIndex( xEMACIndex );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@htibosch htibosch left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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 <[email protected]>.
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.
Set solicited-node addresses independent of LLMNR.
For mDNS set IPv4/6 MAC depending on ipconfigUSE_IPv6.
@StefanBalt
Copy link
Contributor Author

StefanBalt commented Sep 12, 2024

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
Copy link
Member

@StefanBalt

Thanks for taking time to contribute to FreeRTOS+TCP.

Can you help in fixing the CI checks:

  1. To fix the spell checks these words [those listed in the logs Run spellings check] needs to be added to this file.
  2. 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.

tony-josi-aws
tony-josi-aws previously approved these changes Sep 30, 2024
aggarg
aggarg previously approved these changes Sep 30, 2024
Copy link
Contributor

@htibosch htibosch left a comment

Choose a reason for hiding this comment

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

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.

XEmacPs_SetHash( pxEMAC_PS, ( void * ) ucMACAddress );
}
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
#endif /* if ( ipconfigUSE_IPv6 == 0 ) */
}
#endif /* ( ipconfigUSE_MDNS == 1 ) */
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

usGenerateProtocolChecksum( pxBuffer->pucEthernetBuffer, pxBuffer->xDataLength, pdTRUE );
}
#endif /* ipconfigUSE_IPv6 */

if( ( pxPacket->xICMPPacket.xEthernetHeader.usFrameType == ipIPv4_FRAME_TYPE ) &&
Copy link
Contributor

@htibosch htibosch Sep 30, 2024

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@tony-josi-aws
Copy link
Member

@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:

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

@StefanBalt
Copy link
Contributor Author

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

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

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).

tony-josi-aws
tony-josi-aws previously approved these changes Oct 1, 2024
@tony-josi-aws tony-josi-aws merged commit dd88502 into FreeRTOS:main Oct 3, 2024
9 of 10 checks passed
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.

5 participants