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

[Stefan Roese]

Hi Michal,

On 1/24/23 14:12, Michal Simek wrote:
> Hi Stefan,
> 
> On 1/11/23 12:38, Stefan Roese wrote:
>> 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.
> 
> I think this will require further investigation what it is really 
> happening.
> If this is related to SGMII only or also RGMII. Obviously adding some 
> delay without proper explanation is not the right way to go.

Agreed. I'm also not 100% satisfied with this "solution".

> Pretty much if something is not stable there should be a way to pooling 
> any bit.

You mean polling (instead of pooling)? Again, the link is already
established when "pcscntrl" is configured. I can try to double-check
again, if I can poll for something in this "pcscntrl".

> There is ongoing effort on moving clock from direct register access to 
> using firmware interface. And maybe this issue will be resolve by that 
> that simply clock is not stable at the time when driver tries to enable 
> RX/TX.
> And current clock driver is not waiting for clock to be stable but 
> firmware base driver does it.

As mentioned above, 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. If this is the case, then
changes in the clock access won't make any difference here IIUTC.

Still I will do some more tests with this "pcscntrl" hopefully later
today and will report back. Stay tuned...

Thanks,
Stefan