[PATCH] net: zynq_gem: Add a 10ms delay in zynq_gem_init()

[Stefan Roese]

Hi Harini,

On 1/11/23 12:20, Katakam, Harini wrote:
> +
> Hi Stefan,
> 
>> -----Original Message-----
>> From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Stefan Roese
>> Sent: Wednesday, January 11, 2023 1:01 PM
>> To: u-boot at lists.denx.de
>> Cc: Michal Simek <michal.simek at amd.com>; Ramon Fried
>> <rfried.dev at gmail.com>; Sean Anderson <sean.anderson at seco.com>
>> Subject: [PATCH] net: zynq_gem: Add a 10ms delay in zynq_gem_init()
>>
>> In our system using ZynqMP with an external SGMII PHY it's necessary to
>> wait a short while after the configuration in zynq_gem_init() before the xfer
>> starts. Otherwise the first packet(s) might get dropped, resulting in a delay at
>> the start of the ethernet transfers.
>>
>> This patch adds a minimal delay of 10ms which fixes problems of dropped
>> first packages.
>>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> Cc: Michal Simek <michal.simek at amd.com>
>> Cc: Ramon Fried <rfried.dev at gmail.com>
>> Cc: Sean Anderson <sean.anderson at seco.com>
>> ---
>>   drivers/net/zynq_gem.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index
>> 507b19b75975..26e468766871 100644
>> --- a/drivers/net/zynq_gem.c
>> +++ b/drivers/net/zynq_gem.c
>> @@ -522,6 +522,13 @@ static int zynq_gem_init(struct udevice *dev)
>>   			return ret;
>>   		}
>>   	}
>> +
>> +	/*
>> +	 * Some additional minimal delay seems to be needed so that
>> +	 * the first packet will be sent correctly
>> +	 */
>> +	mdelay(10);
>> +
> 
> Thanks for the patch. The PCS status check in your v1 seems valid.
> However, this delay of 10 msecs should not be necessary. If the PCS
> status (when autoneg is enabled with the external PHY on your board)
> shows link up, that's enough.

Unfortunately this is not the case. Regardless of using the PCS check
from the v1 patch from yesterday, such a minimal delay is necessary on
our setup.

> Could you please consider the following
> to investigate why initial packets are lost?
> -> If the external PHY on your board has a hardware reset please
> consider updating the reset-assert-us and reset-deassert-us properties
> to ensure PHY is ready before access.

Not sure if this will help. See my comment at the end please for this.

> -> Can you please check if there's any link stability issues in the initial
> msecs? Monitoring the serdes/GTR SGMII lane that's setup on ZynqMP
> will be useful.

I tested 2 situations:

a) Starting network traffic very early in U-Boot, before the aneg link
establishment has finished (preboot=ping 192.168.1.5):

[8.448005 0.007878] Net:
[8.453711 0.005706] ZYNQ GEM: ff0d0000, mdio bus ff0d0000, phyaddr 0, 
interface sgmii
[8.454449 0.000739] eth0: ethernet at ff0d0000
[8.454752 0.000303] ethernet at ff0d0000 Waiting for PHY auto negotiation 
to complete..... done
[10.915882 2.461130] Using ethernet at ff0d0000 device
[10.916228 0.000346] host 192.168.1.5 is alive
[10.917247 0.001019] Hit any key to stop autoboot:  0

b) Late in U-Boot, after waiting for some seconds (or even minutes, it
makes no difference) on the prompt  (preboot=sleep 10;ping 192.168.1.5):

[4.655932 0.007849] Net:
[4.661612 0.005680] ZYNQ GEM: ff0d0000, mdio bus ff0d0000, phyaddr 0, 
interface sgmii
[4.662367 0.000755] eth0: ethernet at ff0d0000
[14.671655 10.009288] Using ethernet at ff0d0000 device
[14.672018 0.000364] host 192.168.1.5 is alive
[14.673032 0.001014] Hit any key to stop autoboot:  0

The delay is needed in both cases. In a) you see, that the PHY aneg
takes a bit of time to finish. The link is established at this time.

My feeling is that configuration of "pcscntrl" (or some other register)
after phy_startup() in zynq_gem_init() introduces this necessity of this
additional delay.

Thanks,
Stefan