| Message ID | 20230616135323.98215-14-radu-nicolae.pirea@oss.nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add TJA1120 support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 45 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, Jun 16, 2023 at 04:53:22PM +0300, Radu Pirea (NXP OSS) wrote: > During PTP testing on early TJA1120 engineering samples I observed that > if the link is lost and recovered, the tx timestamps will be randomly > lost. To avoid this HW issue, the PCS should be reseted. > > Resetting the PCS will break the link and we should reset the PCS on > LINK UP -> LINK DOWN transition, otherwise we will trigger and infinite > loop of LINK UP -> LINK DOWN events. > +static int tja1120_read_status(struct phy_device *phydev) > +{ > + unsigned int link = phydev->link; > + int ret; Maybe consider using link_change_notify: /** * @link_change_notify: Called to inform a PHY device driver * when the core is about to change the link state. This * callback is supposed to be used as fixup hook for drivers * that need to take action when the link state * changes. Drivers are by no means allowed to mess with the * PHY device structure in their implementations. */ void (*link_change_notify)(struct phy_device *dev); Seems like a fixup to me. Andrew
On 17.06.2023 00:00, Andrew Lunn wrote: > On Fri, Jun 16, 2023 at 04:53:22PM +0300, Radu Pirea (NXP OSS) wrote: >> During PTP testing on early TJA1120 engineering samples I observed that >> if the link is lost and recovered, the tx timestamps will be randomly >> lost. To avoid this HW issue, the PCS should be reseted. >> >> Resetting the PCS will break the link and we should reset the PCS on >> LINK UP -> LINK DOWN transition, otherwise we will trigger and infinite >> loop of LINK UP -> LINK DOWN events. > >> +static int tja1120_read_status(struct phy_device *phydev) >> +{ >> + unsigned int link = phydev->link; >> + int ret; > > Maybe consider using link_change_notify: > > /** > * @link_change_notify: Called to inform a PHY device driver > * when the core is about to change the link state. This > * callback is supposed to be used as fixup hook for drivers > * that need to take action when the link state > * changes. Drivers are by no means allowed to mess with the > * PHY device structure in their implementations. > */ > void (*link_change_notify)(struct phy_device *dev); > > Seems like a fixup to me. > > Andrew Ok. link_change_notify seems the right callback for this fix. Thank you.
diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c index 0ed96d696bad..0d22eb7534dc 100644 --- a/drivers/net/phy/nxp-c45-tja11xx.c +++ b/drivers/net/phy/nxp-c45-tja11xx.c @@ -56,6 +56,9 @@ #define VEND1_PHY_CONFIG 0x8108 #define PHY_CONFIG_AUTO BIT(0) +#define TJA1120_EPHY_RESETS 0x810A +#define EPHY_PCS_RESET BIT(3) + #define VEND1_SIGNAL_QUALITY 0x8320 #define SQI_VALID BIT(14) #define SQI_MASK GENMASK(2, 0) @@ -1325,6 +1328,28 @@ static int nxp_c45_get_sqi(struct phy_device *phydev) return reg; } +static int tja1120_read_status(struct phy_device *phydev) +{ + unsigned int link = phydev->link; + int ret; + + ret = genphy_c45_read_status(phydev); + if (ret) + return ret; + + /* Bug workaround for TJA1120 enegineering samples: fix egress + * timestamps lost after link recovery. + */ + if (link && !phydev->link) { + phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, + TJA1120_EPHY_RESETS, EPHY_PCS_RESET); + phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, + TJA1120_EPHY_RESETS, EPHY_PCS_RESET); + } + + return ret; +} + static int nxp_c45_get_sqi_max(struct phy_device *phydev) { return MAX_SQI; @@ -1879,7 +1904,7 @@ static struct phy_driver nxp_c45_driver[] = { .config_init = nxp_c45_config_init, .config_intr = tja1120_config_intr, .handle_interrupt = nxp_c45_handle_interrupt, - .read_status = genphy_c45_read_status, + .read_status = tja1120_read_status, .suspend = genphy_c45_pma_suspend, .resume = genphy_c45_pma_resume, .get_sset_count = nxp_c45_get_sset_count,
During PTP testing on early TJA1120 engineering samples I observed that if the link is lost and recovered, the tx timestamps will be randomly lost. To avoid this HW issue, the PCS should be reseted. Resetting the PCS will break the link and we should reset the PCS on LINK UP -> LINK DOWN transition, otherwise we will trigger and infinite loop of LINK UP -> LINK DOWN events. Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com> --- drivers/net/phy/nxp-c45-tja11xx.c | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)