* [PATCH v2 net-next 0/3] Fixes and improvements to TJA1103 PHY driver @ 2021-06-14 12:38 Vladimir Oltean 2021-06-14 12:38 ` [PATCH v2 net-next 1/3] net: phy: nxp-c45-tja11xx: demote the "no PTP support" message to debug Vladimir Oltean ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Vladimir Oltean @ 2021-06-14 12:38 UTC (permalink / raw) To: Jakub Kicinski, David S. Miller, netdev Cc: Florian Fainelli, Andrew Lunn, Heiner Kallweit, Russell King, Radu Pirea, Vladimir Oltean From: Vladimir Oltean <vladimir.oltean@nxp.com> This series contains: - an erratum workaround for the TJA1103 PHY integrated in SJA1110 - an adaptation of the driver so it prints less unnecessary information when probing on SJA1110 - a PTP RX timestamping bug fix Targeting net-next since the PHY support is currently in net-next only. Changes in v2: Added a comment to the hardware workaround procedure. Vladimir Oltean (3): net: phy: nxp-c45-tja11xx: demote the "no PTP support" message to debug net: phy: nxp-c45-tja11xx: fix potential RX timestamp wraparound net: phy: nxp-c45-tja11xx: enable MDIO write access to the master/slave registers drivers/net/phy/nxp-c45-tja11xx.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 net-next 1/3] net: phy: nxp-c45-tja11xx: demote the "no PTP support" message to debug 2021-06-14 12:38 [PATCH v2 net-next 0/3] Fixes and improvements to TJA1103 PHY driver Vladimir Oltean @ 2021-06-14 12:38 ` Vladimir Oltean 2021-06-14 12:47 ` Russell King (Oracle) 2021-06-14 12:38 ` [PATCH v2 net-next 2/3] net: phy: nxp-c45-tja11xx: fix potential RX timestamp wraparound Vladimir Oltean 2021-06-14 12:38 ` [PATCH v2 net-next 3/3] net: phy: nxp-c45-tja11xx: enable MDIO write access to the master/slave registers Vladimir Oltean 2 siblings, 1 reply; 7+ messages in thread From: Vladimir Oltean @ 2021-06-14 12:38 UTC (permalink / raw) To: Jakub Kicinski, David S. Miller, netdev Cc: Florian Fainelli, Andrew Lunn, Heiner Kallweit, Russell King, Radu Pirea, Vladimir Oltean From: Vladimir Oltean <vladimir.oltean@nxp.com> The SJA1110 switch integrates these PHYs, and they do not have support for timestamping. This message becomes quite overwhelming: [ 10.056596] NXP C45 TJA1103 spi1.0-base-t1:01: the phy does not support PTP [ 10.112625] NXP C45 TJA1103 spi1.0-base-t1:02: the phy does not support PTP [ 10.167461] NXP C45 TJA1103 spi1.0-base-t1:03: the phy does not support PTP [ 10.223510] NXP C45 TJA1103 spi1.0-base-t1:04: the phy does not support PTP [ 10.278239] NXP C45 TJA1103 spi1.0-base-t1:05: the phy does not support PTP [ 10.332663] NXP C45 TJA1103 spi1.0-base-t1:06: the phy does not support PTP [ 15.390828] NXP C45 TJA1103 spi1.2-base-t1:01: the phy does not support PTP [ 15.445224] NXP C45 TJA1103 spi1.2-base-t1:02: the phy does not support PTP [ 15.499673] NXP C45 TJA1103 spi1.2-base-t1:03: the phy does not support PTP [ 15.554074] NXP C45 TJA1103 spi1.2-base-t1:04: the phy does not support PTP [ 15.608516] NXP C45 TJA1103 spi1.2-base-t1:05: the phy does not support PTP [ 15.662996] NXP C45 TJA1103 spi1.2-base-t1:06: the phy does not support PTP So reduce its log level to debug. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> --- v1->v2: none drivers/net/phy/nxp-c45-tja11xx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c index 512e4cb5d2c2..902fe1aa7782 100644 --- a/drivers/net/phy/nxp-c45-tja11xx.c +++ b/drivers/net/phy/nxp-c45-tja11xx.c @@ -1090,7 +1090,7 @@ static int nxp_c45_probe(struct phy_device *phydev) VEND1_PORT_ABILITIES); ptp_ability = !!(ptp_ability & PTP_ABILITY); if (!ptp_ability) { - phydev_info(phydev, "the phy does not support PTP"); + phydev_dbg(phydev, "the phy does not support PTP"); goto no_ptp_support; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 net-next 1/3] net: phy: nxp-c45-tja11xx: demote the "no PTP support" message to debug 2021-06-14 12:38 ` [PATCH v2 net-next 1/3] net: phy: nxp-c45-tja11xx: demote the "no PTP support" message to debug Vladimir Oltean @ 2021-06-14 12:47 ` Russell King (Oracle) 0 siblings, 0 replies; 7+ messages in thread From: Russell King (Oracle) @ 2021-06-14 12:47 UTC (permalink / raw) To: Vladimir Oltean Cc: Jakub Kicinski, David S. Miller, netdev, Florian Fainelli, Andrew Lunn, Heiner Kallweit, Radu Pirea, Vladimir Oltean On Mon, Jun 14, 2021 at 03:38:13PM +0300, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > The SJA1110 switch integrates these PHYs, and they do not have support > for timestamping. This message becomes quite overwhelming: > > [ 10.056596] NXP C45 TJA1103 spi1.0-base-t1:01: the phy does not support PTP > [ 10.112625] NXP C45 TJA1103 spi1.0-base-t1:02: the phy does not support PTP > [ 10.167461] NXP C45 TJA1103 spi1.0-base-t1:03: the phy does not support PTP > [ 10.223510] NXP C45 TJA1103 spi1.0-base-t1:04: the phy does not support PTP > [ 10.278239] NXP C45 TJA1103 spi1.0-base-t1:05: the phy does not support PTP > [ 10.332663] NXP C45 TJA1103 spi1.0-base-t1:06: the phy does not support PTP > [ 15.390828] NXP C45 TJA1103 spi1.2-base-t1:01: the phy does not support PTP > [ 15.445224] NXP C45 TJA1103 spi1.2-base-t1:02: the phy does not support PTP > [ 15.499673] NXP C45 TJA1103 spi1.2-base-t1:03: the phy does not support PTP > [ 15.554074] NXP C45 TJA1103 spi1.2-base-t1:04: the phy does not support PTP > [ 15.608516] NXP C45 TJA1103 spi1.2-base-t1:05: the phy does not support PTP > [ 15.662996] NXP C45 TJA1103 spi1.2-base-t1:06: the phy does not support PTP > > So reduce its log level to debug. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 net-next 2/3] net: phy: nxp-c45-tja11xx: fix potential RX timestamp wraparound 2021-06-14 12:38 [PATCH v2 net-next 0/3] Fixes and improvements to TJA1103 PHY driver Vladimir Oltean 2021-06-14 12:38 ` [PATCH v2 net-next 1/3] net: phy: nxp-c45-tja11xx: demote the "no PTP support" message to debug Vladimir Oltean @ 2021-06-14 12:38 ` Vladimir Oltean 2021-06-14 12:59 ` Russell King (Oracle) 2021-06-14 12:38 ` [PATCH v2 net-next 3/3] net: phy: nxp-c45-tja11xx: enable MDIO write access to the master/slave registers Vladimir Oltean 2 siblings, 1 reply; 7+ messages in thread From: Vladimir Oltean @ 2021-06-14 12:38 UTC (permalink / raw) To: Jakub Kicinski, David S. Miller, netdev Cc: Florian Fainelli, Andrew Lunn, Heiner Kallweit, Russell King, Radu Pirea, Vladimir Oltean, Richard Cochran From: Vladimir Oltean <vladimir.oltean@nxp.com> The reconstruction procedure for partial timestamps reads the current PTP time and fills in the low 2 bits of the second portion, as well as the nanoseconds portion, from the actual hardware packet timestamp. Critically, the reconstruction procedure works because it assumes that the current PTP time is strictly larger than the hardware timestamp was: it detects a 2-bit wraparound of the 'seconds' portion by checking whether the 'seconds' portion of the partial hardware timestamp is larger than the 'seconds' portion of the current time. That can only happen if the hardware timestamp was captured by the PHY during the last phase of a 'modulo 4 seconds' interval, and the current PTP time was read by the driver during the initial phase of the next 'modulo 4 seconds' interval. The partial RX timestamps are added to priv->rx_queue in nxp_c45_rxtstamp() and they are processed potentially in parallel by the aux worker thread in nxp_c45_do_aux_work(). This means that it is possible for nxp_c45_do_aux_work() to process more than one RX timestamp during the same schedule. There is one premature optimization that will cause issues: for RX timestamping, the driver reads the current time only once, and it uses that to reconstruct all PTP RX timestamps in the queue. For the second and later timestamps, this will be an issue if we are processing two RX timestamps which are to the left and to the right, respectively, of a 4-bit wraparound of the 'seconds' portion of the PTP time, and the current PTP time is also pre-wraparound. 0.000000000 4.000000000 8.000000000 12.000000000 |..................|..................|..................|............> ^ ^ ^ ^ time | | | | | | | process hwts 1 and hwts 2 | | | | | hwts 2 | | | read current PTP time | hwts 1 What will happen in that case is that hwts 2 (post-wraparound) will use a stale current PTP time that is pre-wraparound. But nxp_c45_reconstruct_ts will not detect this condition, because it is not coded up for it, so it will reconstruct hwts 2 with a current time from the previous 4 second interval (i.e. 0.something instead of 4.something). This is solvable by making sure that the full 64-bit current time is always read after the PHY has taken the partial RX timestamp. We do this by reading the current PTP time for every timestamp in the RX queue. Fixes: 514def5dd339 ("phy: nxp-c45-tja11xx: add timestamping support") Cc: Richard Cochran <richardcochran@gmail.com> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v1->v2: none drivers/net/phy/nxp-c45-tja11xx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c index 902fe1aa7782..118b393b1cbb 100644 --- a/drivers/net/phy/nxp-c45-tja11xx.c +++ b/drivers/net/phy/nxp-c45-tja11xx.c @@ -427,8 +427,8 @@ static long nxp_c45_do_aux_work(struct ptp_clock_info *ptp) nxp_c45_process_txts(priv, &hwts); } - nxp_c45_ptp_gettimex64(&priv->caps, &ts, NULL); while ((skb = skb_dequeue(&priv->rx_queue)) != NULL) { + nxp_c45_ptp_gettimex64(&priv->caps, &ts, NULL); ts_raw = __be32_to_cpu(NXP_C45_SKB_CB(skb)->header->reserved2); hwts.sec = ts_raw >> 30; hwts.nsec = ts_raw & GENMASK(29, 0); -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 net-next 2/3] net: phy: nxp-c45-tja11xx: fix potential RX timestamp wraparound 2021-06-14 12:38 ` [PATCH v2 net-next 2/3] net: phy: nxp-c45-tja11xx: fix potential RX timestamp wraparound Vladimir Oltean @ 2021-06-14 12:59 ` Russell King (Oracle) 0 siblings, 0 replies; 7+ messages in thread From: Russell King (Oracle) @ 2021-06-14 12:59 UTC (permalink / raw) To: Vladimir Oltean Cc: Jakub Kicinski, David S. Miller, netdev, Florian Fainelli, Andrew Lunn, Heiner Kallweit, Radu Pirea, Vladimir Oltean, Richard Cochran On Mon, Jun 14, 2021 at 03:38:14PM +0300, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > The reconstruction procedure for partial timestamps reads the current > PTP time and fills in the low 2 bits of the second portion, as well as > the nanoseconds portion, from the actual hardware packet timestamp. > Critically, the reconstruction procedure works because it assumes that > the current PTP time is strictly larger than the hardware timestamp was: > it detects a 2-bit wraparound of the 'seconds' portion by checking whether > the 'seconds' portion of the partial hardware timestamp is larger than > the 'seconds' portion of the current time. That can only happen if the > hardware timestamp was captured by the PHY during the last phase of a > 'modulo 4 seconds' interval, and the current PTP time was read by the > driver during the initial phase of the next 'modulo 4 seconds' interval. > > The partial RX timestamps are added to priv->rx_queue in > nxp_c45_rxtstamp() and they are processed potentially in parallel by the > aux worker thread in nxp_c45_do_aux_work(). This means that it is > possible for nxp_c45_do_aux_work() to process more than one RX timestamp > during the same schedule. > > There is one premature optimization that will cause issues: for RX > timestamping, the driver reads the current time only once, and it uses > that to reconstruct all PTP RX timestamps in the queue. For the second > and later timestamps, this will be an issue if we are processing two RX > timestamps which are to the left and to the right, respectively, of a > 4-bit wraparound of the 'seconds' portion of the PTP time, and the > current PTP time is also pre-wraparound. > > 0.000000000 4.000000000 8.000000000 12.000000000 > |..................|..................|..................|............> > ^ ^ ^ ^ time > | | | | > | | | process hwts 1 and hwts 2 > | | | > | | hwts 2 > | | > | read current PTP time > | > hwts 1 > > What will happen in that case is that hwts 2 (post-wraparound) will use > a stale current PTP time that is pre-wraparound. > But nxp_c45_reconstruct_ts will not detect this condition, because it is > not coded up for it, so it will reconstruct hwts 2 with a current time > from the previous 4 second interval (i.e. 0.something instead of > 4.something). > > This is solvable by making sure that the full 64-bit current time is > always read after the PHY has taken the partial RX timestamp. We do this > by reading the current PTP time for every timestamp in the RX queue. > > Fixes: 514def5dd339 ("phy: nxp-c45-tja11xx: add timestamping support") > Cc: Richard Cochran <richardcochran@gmail.com> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > v1->v2: none > > drivers/net/phy/nxp-c45-tja11xx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c > index 902fe1aa7782..118b393b1cbb 100644 > --- a/drivers/net/phy/nxp-c45-tja11xx.c > +++ b/drivers/net/phy/nxp-c45-tja11xx.c > @@ -427,8 +427,8 @@ static long nxp_c45_do_aux_work(struct ptp_clock_info *ptp) > nxp_c45_process_txts(priv, &hwts); > } > > - nxp_c45_ptp_gettimex64(&priv->caps, &ts, NULL); > while ((skb = skb_dequeue(&priv->rx_queue)) != NULL) { > + nxp_c45_ptp_gettimex64(&priv->caps, &ts, NULL); > ts_raw = __be32_to_cpu(NXP_C45_SKB_CB(skb)->header->reserved2); > hwts.sec = ts_raw >> 30; > hwts.nsec = ts_raw & GENMASK(29, 0); This looks good, but while reviewing it, I've been looking at nxp_c45_reconstruct_ts(). While it is logically correct, this makes it difficult to understand: if ((ts->tv_sec & TS_SEC_MASK) < (hwts->sec & TS_SEC_MASK)) ts->tv_sec -= BIT(2); The use of BIT() here seems completely inappropriate and misleading. The value we really want to be using here is TS_SEC_MASK + 1 - this has the advantage of making it completely clear that _this_ value depends on TS_SEC_MASK, and is not some unrelated value. In any case, for _this_ patch: Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 net-next 3/3] net: phy: nxp-c45-tja11xx: enable MDIO write access to the master/slave registers 2021-06-14 12:38 [PATCH v2 net-next 0/3] Fixes and improvements to TJA1103 PHY driver Vladimir Oltean 2021-06-14 12:38 ` [PATCH v2 net-next 1/3] net: phy: nxp-c45-tja11xx: demote the "no PTP support" message to debug Vladimir Oltean 2021-06-14 12:38 ` [PATCH v2 net-next 2/3] net: phy: nxp-c45-tja11xx: fix potential RX timestamp wraparound Vladimir Oltean @ 2021-06-14 12:38 ` Vladimir Oltean 2021-06-14 12:51 ` Russell King (Oracle) 2 siblings, 1 reply; 7+ messages in thread From: Vladimir Oltean @ 2021-06-14 12:38 UTC (permalink / raw) To: Jakub Kicinski, David S. Miller, netdev Cc: Florian Fainelli, Andrew Lunn, Heiner Kallweit, Russell King, Radu Pirea, Vladimir Oltean From: Vladimir Oltean <vladimir.oltean@nxp.com> The SJA1110 switch integrates TJA1103 PHYs, but in SJA1110 switch rev B silicon, there is a bug in that the registers for selecting the 100base-T1 autoneg master/slave roles are not writable. To enable write access to the master/slave registers, these additional PHY writes are necessary during initialization. The issue has been corrected in later SJA1110 silicon versions and is not present in the standalone PHY variants, but applying the workaround unconditionally in the driver should not do any harm. Suggested-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v1->v2: added a comment drivers/net/phy/nxp-c45-tja11xx.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c index 118b393b1cbb..f8b2ecd0d6bf 100644 --- a/drivers/net/phy/nxp-c45-tja11xx.c +++ b/drivers/net/phy/nxp-c45-tja11xx.c @@ -1035,6 +1035,12 @@ static int nxp_c45_config_init(struct phy_device *phydev) return ret; } + /* Bug workaround for SJA1110 rev B: enable write access + * to MDIO_MMD_PMAPMD + */ + phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x01F8, 1); + phy_write_mmd(phydev, MDIO_MMD_VEND1, 0x01F9, 2); + phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PHY_CONFIG, PHY_CONFIG_AUTO); -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 net-next 3/3] net: phy: nxp-c45-tja11xx: enable MDIO write access to the master/slave registers 2021-06-14 12:38 ` [PATCH v2 net-next 3/3] net: phy: nxp-c45-tja11xx: enable MDIO write access to the master/slave registers Vladimir Oltean @ 2021-06-14 12:51 ` Russell King (Oracle) 0 siblings, 0 replies; 7+ messages in thread From: Russell King (Oracle) @ 2021-06-14 12:51 UTC (permalink / raw) To: Vladimir Oltean Cc: Jakub Kicinski, David S. Miller, netdev, Florian Fainelli, Andrew Lunn, Heiner Kallweit, Radu Pirea, Vladimir Oltean On Mon, Jun 14, 2021 at 03:38:15PM +0300, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > The SJA1110 switch integrates TJA1103 PHYs, but in SJA1110 switch rev B > silicon, there is a bug in that the registers for selecting the 100base-T1 > autoneg master/slave roles are not writable. > > To enable write access to the master/slave registers, these additional > PHY writes are necessary during initialization. > > The issue has been corrected in later SJA1110 silicon versions and is > not present in the standalone PHY variants, but applying the workaround > unconditionally in the driver should not do any harm. > > Suggested-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-14 12:59 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-14 12:38 [PATCH v2 net-next 0/3] Fixes and improvements to TJA1103 PHY driver Vladimir Oltean 2021-06-14 12:38 ` [PATCH v2 net-next 1/3] net: phy: nxp-c45-tja11xx: demote the "no PTP support" message to debug Vladimir Oltean 2021-06-14 12:47 ` Russell King (Oracle) 2021-06-14 12:38 ` [PATCH v2 net-next 2/3] net: phy: nxp-c45-tja11xx: fix potential RX timestamp wraparound Vladimir Oltean 2021-06-14 12:59 ` Russell King (Oracle) 2021-06-14 12:38 ` [PATCH v2 net-next 3/3] net: phy: nxp-c45-tja11xx: enable MDIO write access to the master/slave registers Vladimir Oltean 2021-06-14 12:51 ` Russell King (Oracle)
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.