Message-ID: <ee295a9b-a9f6-4f18-8bb3-fc6871ecdf40@nvidia.com>
Date: Tue, 24 Sep 2024 11:33:13 +0100
From: Jon Hunter <jonathanh@...dia.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Revanth Kumar Uppala <ruppala@...dia.com>, "andrew@...n.ch"
 <andrew@...n.ch>, "hkallweit1@...il.com" <hkallweit1@...il.com>,
 "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH 3/4] net: phy: aquantia: Poll for TX ready at PHY system
 side


On 30/07/2024 13:25, Jon Hunter wrote:
> 
> On 30/07/2024 12:12, Russell King (Oracle) wrote:
> 
> ...
> 
>> Hmm. dwmac-tegra.c sets STMMAC_FLAG_SERDES_UP_AFTER_PHY_LINKUP, which
>> means that the serdes won't be powered up until after the PHY has
>> indicated that link is up. If the serdes is not powered up, then the
>> MAC facing interface on the PHY won't come up.
>>
>> Hence, the code you're adding will, in all probability, merely add a
>> two second delay to each and every time the PHY is polled for its
>> status when the PHY indicates that the media link is up until such
>> time that the stmmac side has processed that the link has come up.
>>
>> I also note that mgbe_uphy_lane_bringup_serdes_up() polls the link
>> status on the MAC PCS side, waiting for the link to become ready
>> on that side.
>>
>> So, what you have is:
>>
>> - you bring the interface up. The serdes interface remains powered down.
>> - phylib starts polling the PHY.
>> - the PHY indicates the media link is up.
>> - your new code polls the PHY's MAC facing interface for link up, but
>>    because the serdes interface is powered down, it ends up timing out
>>    after two seconds and then proceeds.
>> - phylib notifies phylink that the PHY has link.
>> - phylink brings the PCS and MAC side(s) up, calling
>>    stmmac_mac_link_up().
>> - stmmac_mac_link_up() calls mgbe_uphy_lane_bringup_serdes_up() which
>>    then does receive lane calibration (which is likely the reason why
>>    this is delayed to link-up, so the PHY is giving a valid serdes
>>    stream for the calibration to use.)
>> - mgbe_uphy_lane_bringup_serdes_up() enables the data path, and
>>    clears resets, and then waits for the serdes link with the PHY to
>>    come up.
>>
>> While stmmac_mac_link_up() is running, phylib will continue to try to
>> poll the PHY for its status once every second, and each time it does
>> if the PHY's MAC facing link reports that it's down, the phylib locks
>> will be held for _two_ seconds each time. That will mean you won't be
>> able to bring the interface down until those two seconds time out.
>>
>> So, I think one needs to go back and properly understand what is going
>> on to figure out what is going wrong.
> 
> Yes agree.
> 
>> You will likely find that inserting a two second delay at the start of
>> mgbe_uphy_lane_bringup_serdes_up() is just as effective at solving
>> the issue - although I am not suggesting that would be an acceptable
>> solution. It would help to confirm that the reasoning is correct.
> 
> I was wondering about something along those lines. We will go back and 
> look at this.


Just to circle back on this. After reviewing the link bring-up sequence 
in the mgbe_uphy_lane_bringup_serdes_up() function, the Tegra hardware 
team identified some specific sequence and timing requirements we needed 
to correct. Paritosh has posted a fix for this [0] and now we have 
verified that this fixes the issue we were seeing without making this 
change.

Thanks
Jon

[0] 
https://lore.kernel.org/linux-tegra/20240923134410.2111640-1-paritoshd@nvidia.com/

-- 
nvpublic

Powered by blists - more mailing lists