| Message ID | 20220112173700.873002-3-robert.hancock@calian.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Xilinx axienet fixes | expand |
On Wed, Jan 12, 2022 at 11:36:53AM -0600, Robert Hancock wrote: > 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: 8a3b7a252dca9 ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver") > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > --- > drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > index f950342f6467..f425a8404a9b 100644 > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > @@ -516,6 +516,16 @@ static int __axienet_device_reset(struct axienet_local *lp) > 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; > + } > + Is this bit guaranteed to be clear before you start waiting for it? Andrew
On Wed, 2022-01-12 at 20:15 +0100, Andrew Lunn wrote: > On Wed, Jan 12, 2022 at 11:36:53AM -0600, Robert Hancock wrote: > > 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: 8a3b7a252dca9 ("drivers/net/ethernet/xilinx: added Xilinx AXI > > Ethernet driver") > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > > --- > > drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > index f950342f6467..f425a8404a9b 100644 > > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > @@ -516,6 +516,16 @@ static int __axienet_device_reset(struct axienet_local > > *lp) > > 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; > > + } > > + > > Is this bit guaranteed to be clear before you start waiting for it? The documentation for the IP core ( https://www.xilinx.com/content/dam/xilinx/support/documentation/ip_documentation/axi_ethernet/v7_2/pg138-axi-ethernet.pdf ) states for the phy_rst_n output signal: "This active-Low reset is held active for 10 ms after power is applied and during any reset. After the reset goes inactive, the PHY cannot be accessed for an additional 5 ms." The PhyRstComplt bit definition mentions "This signal does not transition to 1 for 5 ms after PHY_RST_N transitions to 1". Given that a reset of the core has just been completed above, the PHY reset should at least have been initiated as well, so it should be sufficient to just wait for the bit to become 1 at this point. > > Andrew
> > Is this bit guaranteed to be clear before you start waiting for it? > > The documentation for the IP core ( > https://www.xilinx.com/content/dam/xilinx/support/documentation/ip_documentation/axi_ethernet/v7_2/pg138-axi-ethernet.pdf > ) states for the phy_rst_n output signal: "This active-Low reset is held > active for 10 ms after power is applied and during any reset. After the reset > goes inactive, the PHY cannot be accessed for an additional 5 ms." The > PhyRstComplt bit definition mentions "This signal does not transition to 1 for > 5 ms after PHY_RST_N transitions to 1". Given that a reset of the core has just > been completed above, the PHY reset should at least have been initiated as > well, so it should be sufficient to just wait for the bit to become 1 at this > point. Great, thanks for checking. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
> -----Original Message----- > From: Robert Hancock <robert.hancock@calian.com> > Sent: Wednesday, January 12, 2022 11:07 PM > To: netdev@vger.kernel.org > Cc: Radhey Shyam Pandey <radheys@xilinx.com>; davem@davemloft.net; > kuba@kernel.org; linux-arm-kernel@lists.infradead.org; Michal Simek > <michals@xilinx.com>; ariane.keller@tik.ee.ethz.ch; daniel@iogearbox.net; > Robert Hancock <robert.hancock@calian.com> > Subject: [PATCH net v2 2/9] net: axienet: Wait for PhyRstCmplt after core reset > > 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 Just to understand - can you share 7- series design details. Based on documentation - This MgtRdy bit indicates if the TEMAC core is out of reset and ready for use. In systems that use an serial transceiver, this bit goes to 1 when the serial transceiver is ready to use. Also if we don't wait for phy reset - what is the issue we are seeing? > 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: 8a3b7a252dca9 ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet > driver") > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > --- > drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > index f950342f6467..f425a8404a9b 100644 > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > @@ -516,6 +516,16 @@ static int __axienet_device_reset(struct axienet_local > *lp) > 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; > } > > -- > 2.31.1
On Thu, 2022-01-13 at 11:53 +0000, Radhey Shyam Pandey wrote: > > -----Original Message----- > > From: Robert Hancock <robert.hancock@calian.com> > > Sent: Wednesday, January 12, 2022 11:07 PM > > To: netdev@vger.kernel.org > > Cc: Radhey Shyam Pandey <radheys@xilinx.com>; davem@davemloft.net; > > kuba@kernel.org; linux-arm-kernel@lists.infradead.org; Michal Simek > > <michals@xilinx.com>; ariane.keller@tik.ee.ethz.ch; daniel@iogearbox.net; > > Robert Hancock <robert.hancock@calian.com> > > Subject: [PATCH net v2 2/9] net: axienet: Wait for PhyRstCmplt after core > > reset > > > > 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 > > Just to understand - can you share 7- series design details. > Based on documentation - This MgtRdy bit indicates if the TEMAC core is > out of reset and ready for use. In systems that use an serial transceiver, > this bit goes to 1 when the serial transceiver is ready to use. From what I saw, the bit behaved as described on ZynqMP where it would go to 1 during initialization, but on a Kintex-7 design with this core, the bit never seemed to go to 1 until an actual link was established on the transceiver, so it wasn't really usable in this situation. > > Also if we don't wait for phy reset - what is the issue we are seeing? This is more for the case of using an external PHY device - we shouldn't be proceeding to MDIO initialization and potentially trying to talk to the PHY when it is potentially still in reset.. > > > 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: 8a3b7a252dca9 ("drivers/net/ethernet/xilinx: added Xilinx AXI > > Ethernet > > driver") > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > > --- > > drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > index f950342f6467..f425a8404a9b 100644 > > --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > > @@ -516,6 +516,16 @@ static int __axienet_device_reset(struct axienet_local > > *lp) > > 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; > > } > > > > -- > > 2.31.1
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c index f950342f6467..f425a8404a9b 100644 --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c @@ -516,6 +516,16 @@ static int __axienet_device_reset(struct axienet_local *lp) 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; }
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: 8a3b7a252dca9 ("drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver") Signed-off-by: Robert Hancock <robert.hancock@calian.com> --- drivers/net/ethernet/xilinx/xilinx_axienet_main.c | 10 ++++++++++ 1 file changed, 10 insertions(+)