| Message ID | 20220316075707.61321-1-andy.chiu@sifive.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: axiemac: initialize PHY before device reset | expand |
On Wed, 16 Mar 2022 15:57:07 +0800 Andy Chiu wrote: > Related-to: 'd836ed73a3cb ("net: axienet: reset core on initialization prior to MDIO access")' What's Related-to signifying? You can have multiple Fixes tags if you need to. > Fixes: '1a02556086fc ("net: axienet: Properly handle PCS/PMA PHY for 1000BaseX mode")' There should be no ' quotes around the tag, please fix and repost.
Re: https://lore.kernel.org/all/20220316075707.61321-1-andy.chiu@sifive.com/ (looks like I was CCed with the wrong email address): I'm not sure about this patch. It seems odd/possibly unsafe to reset the whole core (including the MDIO interface) after connecting the PHY which communicates over MDIO, so it's not obvious to me that this is more correct than the original order?
Thanks for pointing that out.
Though it is weird, it should be safe to do that. The reset of the
MDIO interface would not affect any r/w through the bus afterwards
since the driver would re-initialize the MDIO interface whenever it
uses `mdiobus_write()` or `mdiobus_read()` for bus transactions.
However, some of the very first packet on the rx side might get
processed incompletely since `phylink_of_phy_connect()` will
eventually call `phy_resume()`, which brings the phy active earlier
than the reset of the core.
The reason why we have this change in ordering is that the clock of
our PCS/PMA PHY is sourced from the SGMII ref clock of the external
PHY, which is not enabled by default. The only way to get the PCS/PMA
PHY stable here is to start the clock (initialize the external PHY)
before the reset takes place. We have limited clock sources on the
vcu118 FPGA board, and this happens to be our way to configure it. I
think it is a hack on both sw and hw, but still wonder if anyone under
such hw configuration, if exist, would like to have the patch.
|<---ref clock-----|
+----------+---^---+ +------+
| Xilinx's | PCS/ | | Ti's |
| Ethernet | PMA |--SGMII--| PHY |
| | PHY | | |
+----------+-------+ +------+
|--------|--- MDIO---------|
loop-in: radhey.shyam.pandey@xilinx.com
Andy
Andy
On Fri, 2022-03-18 at 01:37 +0800, Andy Chiu wrote: > Thanks for pointing that out. > > Though it is weird, it should be safe to do that. The reset of the > MDIO interface would not affect any r/w through the bus afterwards > since the driver would re-initialize the MDIO interface whenever it > uses `mdiobus_write()` or `mdiobus_read()` for bus transactions. > However, some of the very first packet on the rx side might get > processed incompletely since `phylink_of_phy_connect()` will > eventually call `phy_resume()`, which brings the phy active earlier > than the reset of the core. > > The reason why we have this change in ordering is that the clock of > our PCS/PMA PHY is sourced from the SGMII ref clock of the external > PHY, which is not enabled by default. The only way to get the PCS/PMA > PHY stable here is to start the clock (initialize the external PHY) > before the reset takes place. We have limited clock sources on the > vcu118 FPGA board, and this happens to be our way to configure it. I > think it is a hack on both sw and hw, but still wonder if anyone under > such hw configuration, if exist, would like to have the patch. I haven't looked at the clock setup on the VCU118 in detail, but we have used a setup with this Ethernet core on the ZCU102 board to feed one of the SFP cages. In that case we used the Si570 USER_MGT clock to feed the PCS/PMA clock by changing its clock frequency to 156.25 MHz and routing that to the Ethernet mgt_clk with the core set to accept that frequency. It looks like a similar clock input is available on VCU118, I'm not sure if you can do something similar in your setup? Since I assume this is all hardware on the standard VCU118 board, Xilinx should likely have some example design for this setup, I'm not sure what that is using? Likely using a fixed board clock rather than one from the PHY is better if possible, as you don't have this issue of the clock dependency going backwards up the chain.. > > |<---ref clock-----| > +----------+---^---+ +------+ > > Xilinx's | PCS/ | | Ti's | > > Ethernet | PMA |--SGMII--| PHY | > > | PHY | | | > +----------+-------+ +------+ > |--------|--- MDIO---------| > > loop-in: radhey.shyam.pandey@xilinx.com > > Andy > > > Andy
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c index c7eb05e4a6bf..6fd5157f0a6d 100644 --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c @@ -1141,6 +1141,12 @@ static int axienet_open(struct net_device *ndev) dev_dbg(&ndev->dev, "axienet_open()\n"); + ret = phylink_of_phy_connect(lp->phylink, lp->dev->of_node, 0); + if (ret) { + dev_err(lp->dev, "phylink_of_phy_connect() failed: %d\n", ret); + return ret; + } + /* When we do an Axi Ethernet reset, it resets the complete core * including the MDIO. MDIO must be disabled before resetting. * Hold MDIO bus lock to avoid MDIO accesses during the reset. @@ -1149,12 +1155,6 @@ static int axienet_open(struct net_device *ndev) ret = axienet_device_reset(ndev); axienet_unlock_mii(lp); - ret = phylink_of_phy_connect(lp->phylink, lp->dev->of_node, 0); - if (ret) { - dev_err(lp->dev, "phylink_of_phy_connect() failed: %d\n", ret); - return ret; - } - phylink_start(lp->phylink); /* Enable worker thread for Axi DMA error handling */
On some platforms, the clock of internal (Xilinx's PCS/PMA) PHY was sourced externally and was not enabled by the time the FPGA logic was loaded. Specifically, the clock was souced from an external PHY's SGMII ref clock, which would not start until the driver configured it , on vcu118. Under such condition, the core would boot up in a state where the PCS PHY could not be found on the bus. Or, even if the PCS PHY could be found, the link would be broken and A/N would not complete. To fix this, the Ethernet should be reset every time after the clock being restarted at phylink_of_phy_connect(). Since phylink_of_phy_connect() configures the external PHY base on DT only, it is safe to move it prior to the device reset. Related-to: 'd836ed73a3cb ("net: axienet: reset core on initialization prior to MDIO access")' Fixes: '1a02556086fc ("net: axienet: Properly handle PCS/PMA PHY for 1000BaseX mode")' Signed-off-by: Andy Chiu <andy.chiu@sifive.com> Reviewed-by: Greentime Hu <greentime.hu@sifive.com> --- drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)