| Message ID | 20220111211358.2699350-2-robert.hancock@calian.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Xilinx axienet fixes | expand |
On Tue, 2022-01-11 at 15:13 -0600, Robert Hancock wrote: > In some cases where the Xilinx Ethernet core was used in 1000Base-X or > SGMII modes, which use the internal PCS/PMA PHY, and the MGT > transceiver clock source for the PCS was not running at the time the > FPGA logic was loaded, the core would come up in a state where the > PCS could not be found on the MDIO bus. To fix this, the Ethernet core > (including the PCS) should be reset after enabling the clocks, prior to > attempting to access the PCS using of_mdio_find_device. > > Also, when resetting the device, wait for the PhyRstCmplt bit to be set > in the interrupt status register before continuing initialization, to > ensure that the core is actually ready. The MgtRdy bit could also be > waited for, but unfortunately when using 7-series devices, the bit does > not appear to work as documented (it seems to behave as some sort of > link state indication and not just an indication the transceiver is > ready) so it can't really be relied on. > > Fixes: 3e08fd4a8298 (net: axienet: Properly handle PCS/PMA PHY for 1000BaseX > mode) Patchwork points out that this commit doesn't exist in mainline, it's from the 5.10 stable tree. The corresponding mainline commit is 1a02556086fc0eb16e0a0d09043e9ffb0e31c7db. > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > --- > .../net/ethernet/xilinx/xilinx_axienet_main.c | 34 +++++++++++++------ > 1 file changed, 24 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > index 90144ac7aee8..f4ae035bed35 100644 > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > @@ -496,7 +496,8 @@ static void axienet_setoptions(struct net_device *ndev, > u32 options) > > static int __axienet_device_reset(struct axienet_local *lp) > { > - u32 timeout; > + u32 value; > + int ret; > > /* Reset Axi DMA. This would reset Axi Ethernet core as well. The reset > * process of Axi DMA takes a while to complete as all pending > @@ -506,15 +507,23 @@ static int __axienet_device_reset(struct axienet_local > *lp) > * they both reset the entire DMA core, so only one needs to be used. > */ > axienet_dma_out32(lp, XAXIDMA_TX_CR_OFFSET, XAXIDMA_CR_RESET_MASK); > - timeout = DELAY_OF_ONE_MILLISEC; > - while (axienet_dma_in32(lp, XAXIDMA_TX_CR_OFFSET) & > - XAXIDMA_CR_RESET_MASK) { > - udelay(1); > - if (--timeout == 0) { > - netdev_err(lp->ndev, "%s: DMA reset timeout!\n", > - __func__); > - return -ETIMEDOUT; > - } > + ret = read_poll_timeout(axienet_dma_in32, value, > + !(value & XAXIDMA_CR_RESET_MASK), > + DELAY_OF_ONE_MILLISEC, 50000, false, lp, > + XAXIDMA_TX_CR_OFFSET); > + if (ret) { > + dev_err(lp->dev, "%s: DMA reset timeout!\n", __func__); > + return ret; > + } > + > + /* Wait for PhyRstCmplt bit to be set, indicating the PHY reset has > finished */ > + ret = read_poll_timeout(axienet_ior, value, > + value & XAE_INT_PHYRSTCMPLT_MASK, > + DELAY_OF_ONE_MILLISEC, 50000, false, lp, > + XAE_IS_OFFSET); > + if (ret) { > + dev_err(lp->dev, "%s: timeout waiting for PhyRstCmplt\n", > __func__); > + return ret; > } > > return 0; > @@ -2046,6 +2055,11 @@ static int axienet_probe(struct platform_device *pdev) > lp->coalesce_count_rx = XAXIDMA_DFT_RX_THRESHOLD; > lp->coalesce_count_tx = XAXIDMA_DFT_TX_THRESHOLD; > > + /* Reset core now that clocks are enabled, prior to accessing MDIO */ > + ret = __axienet_device_reset(lp); > + if (ret) > + goto cleanup_clk; > + > lp->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0); > if (lp->phy_node) { > ret = axienet_mdio_setup(lp);
On Wed, 12 Jan 2022 00:30:33 +0000 Robert Hancock wrote: > On Tue, 2022-01-11 at 15:13 -0600, Robert Hancock wrote: > > In some cases where the Xilinx Ethernet core was used in 1000Base-X or > > SGMII modes, which use the internal PCS/PMA PHY, and the MGT > > transceiver clock source for the PCS was not running at the time the > > FPGA logic was loaded, the core would come up in a state where the > > PCS could not be found on the MDIO bus. To fix this, the Ethernet core > > (including the PCS) should be reset after enabling the clocks, prior to > > attempting to access the PCS using of_mdio_find_device. > > > > Also, when resetting the device, wait for the PhyRstCmplt bit to be set > > in the interrupt status register before continuing initialization, to > > ensure that the core is actually ready. The MgtRdy bit could also be > > waited for, but unfortunately when using 7-series devices, the bit does > > not appear to work as documented (it seems to behave as some sort of > > link state indication and not just an indication the transceiver is > > ready) so it can't really be relied on. Shouldn't these be two separate fixes?
On Tue, 2022-01-11 at 19:24 -0800, Jakub Kicinski wrote: > On Wed, 12 Jan 2022 00:30:33 +0000 Robert Hancock wrote: > > On Tue, 2022-01-11 at 15:13 -0600, Robert Hancock wrote: > > > In some cases where the Xilinx Ethernet core was used in 1000Base-X or > > > SGMII modes, which use the internal PCS/PMA PHY, and the MGT > > > transceiver clock source for the PCS was not running at the time the > > > FPGA logic was loaded, the core would come up in a state where the > > > PCS could not be found on the MDIO bus. To fix this, the Ethernet core > > > (including the PCS) should be reset after enabling the clocks, prior to > > > attempting to access the PCS using of_mdio_find_device. > > > > > > Also, when resetting the device, wait for the PhyRstCmplt bit to be set > > > in the interrupt status register before continuing initialization, to > > > ensure that the core is actually ready. The MgtRdy bit could also be > > > waited for, but unfortunately when using 7-series devices, the bit does > > > not appear to work as documented (it seems to behave as some sort of > > > link state indication and not just an indication the transceiver is > > > ready) so it can't really be relied on. > > Shouldn't these be two separate fixes? Yeah, this could likely be broken up into 2 patches (or possibly 3).
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c index 90144ac7aee8..f4ae035bed35 100644 --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c @@ -496,7 +496,8 @@ static void axienet_setoptions(struct net_device *ndev, u32 options) static int __axienet_device_reset(struct axienet_local *lp) { - u32 timeout; + u32 value; + int ret; /* Reset Axi DMA. This would reset Axi Ethernet core as well. The reset * process of Axi DMA takes a while to complete as all pending @@ -506,15 +507,23 @@ static int __axienet_device_reset(struct axienet_local *lp) * they both reset the entire DMA core, so only one needs to be used. */ axienet_dma_out32(lp, XAXIDMA_TX_CR_OFFSET, XAXIDMA_CR_RESET_MASK); - timeout = DELAY_OF_ONE_MILLISEC; - while (axienet_dma_in32(lp, XAXIDMA_TX_CR_OFFSET) & - XAXIDMA_CR_RESET_MASK) { - udelay(1); - if (--timeout == 0) { - netdev_err(lp->ndev, "%s: DMA reset timeout!\n", - __func__); - return -ETIMEDOUT; - } + ret = read_poll_timeout(axienet_dma_in32, value, + !(value & XAXIDMA_CR_RESET_MASK), + DELAY_OF_ONE_MILLISEC, 50000, false, lp, + XAXIDMA_TX_CR_OFFSET); + if (ret) { + dev_err(lp->dev, "%s: DMA reset timeout!\n", __func__); + return ret; + } + + /* Wait for PhyRstCmplt bit to be set, indicating the PHY reset has finished */ + ret = read_poll_timeout(axienet_ior, value, + value & XAE_INT_PHYRSTCMPLT_MASK, + DELAY_OF_ONE_MILLISEC, 50000, false, lp, + XAE_IS_OFFSET); + if (ret) { + dev_err(lp->dev, "%s: timeout waiting for PhyRstCmplt\n", __func__); + return ret; } return 0; @@ -2046,6 +2055,11 @@ static int axienet_probe(struct platform_device *pdev) lp->coalesce_count_rx = XAXIDMA_DFT_RX_THRESHOLD; lp->coalesce_count_tx = XAXIDMA_DFT_TX_THRESHOLD; + /* Reset core now that clocks are enabled, prior to accessing MDIO */ + ret = __axienet_device_reset(lp); + if (ret) + goto cleanup_clk; + lp->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0); if (lp->phy_node) { ret = axienet_mdio_setup(lp);
In some cases where the Xilinx Ethernet core was used in 1000Base-X or SGMII modes, which use the internal PCS/PMA PHY, and the MGT transceiver clock source for the PCS was not running at the time the FPGA logic was loaded, the core would come up in a state where the PCS could not be found on the MDIO bus. To fix this, the Ethernet core (including the PCS) should be reset after enabling the clocks, prior to attempting to access the PCS using of_mdio_find_device. Also, when resetting the device, wait for the PhyRstCmplt bit to be set in the interrupt status register before continuing initialization, to ensure that the core is actually ready. The MgtRdy bit could also be waited for, but unfortunately when using 7-series devices, the bit does not appear to work as documented (it seems to behave as some sort of link state indication and not just an indication the transceiver is ready) so it can't really be relied on. Fixes: 3e08fd4a8298 (net: axienet: Properly handle PCS/PMA PHY for 1000BaseX mode) Signed-off-by: Robert Hancock <robert.hancock@calian.com> --- .../net/ethernet/xilinx/xilinx_axienet_main.c | 34 +++++++++++++------ 1 file changed, 24 insertions(+), 10 deletions(-)