I’m wondering if that workaround needs to be added in the code or if just documenting it is enough.
Does manually defining XPAR_PS7_ETHERNET_0_ENET_SLCR_x_DIVxwork for you?
Right now even if @LinkwitzRiley manually defines XPAR_PS7_ETHERNET_0_ENET_SLCR_x_DIVx the value will not be written to the register because I put the write behind the #ifndef SDT. What I am thinking of changing it to, is that it will write to the register if the macro exists (for whatever speed is being used). This would make the Zynq portable code closer to the example code.
So in my case I do not have the macro defined and I do not write to the register (the way it seems intended to work). For old drivers and the workaround they do have the macro defined so they do write to the register.
From my point of view it is not that simple since the XPAR_PS7_ETHERNET_0_ENET_SLCR_x_DIVx really depend on the clock settings in Vivado. Offloading this config to the embedded developer is not desirable.
My guess is that there is a different way to automatically configure this with the change to SDT based flow, I just don’t know how.
@LinkwitzRiley Try out this code. Replace the one function SetUpSLCRDivisors with these two functions. This maybe needs more cleanup, but I tested this is working for me with both old and new drivers.
This will write the divisor values if they are defined. So as long as you need to define them for your workaround this will write them.
static void WriteSLCRDivisors(int mac_baseaddr, u32 SlcrDiv0, u32 SlcrDiv1)
{
volatile u32 slcrBaseAddress;
if( ( unsigned long ) mac_baseaddr == EMAC0_BASE_ADDRESS )
{
slcrBaseAddress = SLCR_GEM0_CLK_CTRL_ADDR;
}
else
{
slcrBaseAddress = SLCR_GEM1_CLK_CTRL_ADDR;
}
u32 SlcrTxClkCntrl = *( volatile unsigned int * ) ( slcrBaseAddress );
SlcrTxClkCntrl &= EMACPS_SLCR_DIV_MASK;
SlcrTxClkCntrl |= ( SlcrDiv1 << 20 );
SlcrTxClkCntrl |= ( SlcrDiv0 << 8 );
*( volatile unsigned int * ) ( slcrBaseAddress ) = SlcrTxClkCntrl;
}
static void SetUpSLCRDivisors( int mac_baseaddr,
int speed )
{
*( volatile unsigned int * ) ( SLCR_UNLOCK_ADDR ) = SLCR_UNLOCK_KEY_VALUE;
#ifdef PEEP
volatile u32 slcrBaseAddress;
if( ( unsigned long ) mac_baseaddr == EMAC0_BASE_ADDRESS )
{
slcrBaseAddress = SLCR_GEM0_CLK_CTRL_ADDR;
}
else
{
slcrBaseAddress = SLCR_GEM1_CLK_CTRL_ADDR;
}
if( speed == 1000 )
{
*( volatile unsigned int * ) ( slcrBaseAddress ) =
SLCR_GEM_1G_CLK_CTRL_VALUE;
}
else if( speed == 100 )
{
*( volatile unsigned int * ) ( slcrBaseAddress ) =
SLCR_GEM_100M_CLK_CTRL_VALUE;
}
else
{
*( volatile unsigned int * ) ( slcrBaseAddress ) =
SLCR_GEM_10M_CLK_CTRL_VALUE;
}
#else /* ifdef PEEP */
if( speed == 1000 )
{
if( ( unsigned long ) mac_baseaddr == EMAC0_BASE_ADDRESS )
{
#ifdef XPAR_PS7_ETHERNET_0_ENET_SLCR_1000MBPS_DIV0
u32 SlcrDiv0 = XPAR_PS7_ETHERNET_0_ENET_SLCR_1000MBPS_DIV0;
u32 SlcrDiv1 = XPAR_PS7_ETHERNET_0_ENET_SLCR_1000MBPS_DIV1;
WriteSLCRDivisors(mac_baseaddr, SlcrDiv0, SlcrDiv1);
#endif
}
else
{
#ifdef XPAR_PS7_ETHERNET_1_ENET_SLCR_1000MBPS_DIV0
u32 SlcrDiv0 = XPAR_PS7_ETHERNET_1_ENET_SLCR_1000MBPS_DIV0;
u32 SlcrDiv1 = XPAR_PS7_ETHERNET_1_ENET_SLCR_1000MBPS_DIV1;
WriteSLCRDivisors(mac_baseaddr, SlcrDiv0, SlcrDiv1);
#endif
}
}
else if( speed == 100 )
{
if( ( unsigned long ) mac_baseaddr == EMAC0_BASE_ADDRESS )
{
#ifdef XPAR_PS7_ETHERNET_0_ENET_SLCR_100MBPS_DIV0
u32 SlcrDiv0 = XPAR_PS7_ETHERNET_0_ENET_SLCR_100MBPS_DIV0;
u32 SlcrDiv1 = XPAR_PS7_ETHERNET_0_ENET_SLCR_100MBPS_DIV1;
WriteSLCRDivisors(mac_baseaddr, SlcrDiv0, SlcrDiv1);
#endif
}
else
{
#ifdef XPAR_PS7_ETHERNET_1_ENET_SLCR_100MBPS_DIV0
u32 SlcrDiv0 = XPAR_PS7_ETHERNET_1_ENET_SLCR_100MBPS_DIV0;
u32 SlcrDiv1 = XPAR_PS7_ETHERNET_1_ENET_SLCR_100MBPS_DIV1;
WriteSLCRDivisors(mac_baseaddr, SlcrDiv0, SlcrDiv1);
#endif
}
}
else
{
if( ( unsigned long ) mac_baseaddr == EMAC0_BASE_ADDRESS )
{
#ifdef XPAR_PS7_ETHERNET_0_ENET_SLCR_10MBPS_DIV0
u32 SlcrDiv0 = XPAR_PS7_ETHERNET_0_ENET_SLCR_10MBPS_DIV0;
u32 SlcrDiv1 = XPAR_PS7_ETHERNET_0_ENET_SLCR_10MBPS_DIV1;
WriteSLCRDivisors(mac_baseaddr, SlcrDiv0, SlcrDiv1);
#endif
}
else
{
#ifdef XPAR_PS7_ETHERNET_1_ENET_SLCR_10MBPS_DIV0
u32 SlcrDiv0 = XPAR_PS7_ETHERNET_1_ENET_SLCR_10MBPS_DIV0;
u32 SlcrDiv1 = XPAR_PS7_ETHERNET_1_ENET_SLCR_10MBPS_DIV1;
WriteSLCRDivisors(mac_baseaddr, SlcrDiv0, SlcrDiv1);
#endif
}
}
#endif /* ifdef PEEP */
*( volatile unsigned int * ) ( SLCR_LOCK_ADDR ) = SLCR_LOCK_KEY_VALUE;
}
I think the workaround that needs to be done is that the user needs to define the XPAR_PS7_ETHERNET_0_ENET_SLCR_x_DIVx macros in the FreeRTOSIPConfig.h. I agree that this is not ideal because it depends on the hardware configuration, but it seems to be a bug that Xilinx is not providing these values that are necessary for operation.
BTW, I think I finally ran into the same issue you are having. A colleague brought me a 100Mbit switch they were trying to use. It was fortunate timing, because we ended up needing to use the code example I posted and also defining XPAR_PS7_ETHERNET_0_ENET_SLCR_100MBPS_DIV0 and XPAR_PS7_ETHERNET_0_ENET_SLCR_100MBPS_DIV1 in the FreeRTOSIPConfig.h. So for 1Gbit operation I do not need to write divider values, but for 100Mbit operation I need to make use of the workaround we are talking about.
The whole point is, Vivado should calculate the divider values (and provide the corresponding macros for this) based on the configured clocking.
The embedded developer simply might not know how the clocking is configured and maybe even cannot define the macros correctly.
Also while I haven’t tested this explicitely, I don’t think it would have worked with 1Gbit on my end without writing the SCLR since I have the device conneted to an interface that is capable of 1Gbit, but I might have had the 100Mbit/s config in FreeRTOS in the way there, so I’m not sure.
I agree with everything you are saying, but what other option is there? I don’t know the details of how the divider values were calculated with the old drivers, but with the new drivers the values are not being defined so as a workaround I can manually define the same values that were being used previously. Then if Xilinx fixes the issue in a future version the manual definition can be removed.
I think that in order to report this to Xilinx we would need to see if we could reproduce the problem with lwip. Unfortunately, despite Xilinx supporting the FreeRTOS kernel, they don’t care about FreeRTOS-TCP at all. I would think that the same problem should be reproducible with lwip but I have not had time to try. So far I haven’t seen anyone else report this issue.
True that. I am in discussion with AMD/Xilinx about this, let’s what they have to say.
I have given the Xilinx example with lwip only a brief look and did not find any setting of the SCLR. However, this example also didn’t work for me.
Is your discussion in the Xilinx forums? Could you share the link to that?
1 Like
I was able to reproduce the same issue with the lwip echo example code. Same exact behavior where the lwip example works with 1Gbit switch but does not work with the 100Mbit switch.
I also found one interesting thing when looking at Xilinx’s lwip port layer xemacpsif_physpeed.c. Clearly some bits of the FreeRTOS TCP portable were inspired from the lwip port because file and function names are the same. But if you look at the SetUpSLCRDivisors function, the entire inside of the function is inside #ifndef SDT. In the case of SDT drivers the function does nothing except xil_printf("Using default Speed from design\r\n"); Very informative message 
I’ll post an issue in the github repo and see if anybody responds. For now, I think the workaround I posted is the best option for the FreeRTOS TCP portable code. I will need to use the workaround in order to support systems that are using 100Mbit.
Thanks for making the effort!
I think that it is more or less by coincidence, that even the 1Gb works; since the dividers are not set and remain in default which is all 0, 1Gb will also only work with a particular clock speed/PLL setting.
@LinkwitzRiley I opened a PR with the workaround we discussed. If you could double check that it is still working for you it would be appreciated! Thanks.
2 Likes